Bug fix graphql 403

[markgrahamdawson]

Closes #1334

I don't have a good understanding of the issue on this one so I have just implemented the solution that was suggested on the issue by @oliver-sanders .
This issue is hard to reproduce so its difficult to understand if this solution fixes the problem, but the tests still run locally at least.

Check List

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
• Tests are included (or explain why tests are not needed).
• CHANGES.md entry included if this is a change that can affect users
• Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
• If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

[oliver-sanders]

To test (manually), we could try patching the UI Server so that it always raises a 403 error, then inspect the network tab in the developer console to ensure that it is retrying the query as intended.

[markgrahamdawson]

To test (manually), we could try patching the UI Server so that it always raises a 403 error, then inspect the network tab in the developer console to ensure that it is retrying the query as intended.

To test this I added a
raise web.HTTPError(403) to the UIServer UIServerGraphQLHandler .

This causes the UIServer to keep sending back 403s
Then in the browser the network tab repeatedly makes requests and gets sent back 500s
As I understand it this is what is expected (graphQL will either send 200 or 500 cant send 403)

[oliver-sanders]

Tested with the following cylc-uiserver patch which simulates four failed requests before working:

diff --git a/cylc/uiserver/handlers.py b/cylc/uiserver/handlers.py
index 0ec9341..0a6f334 100644
--- a/cylc/uiserver/handlers.py
+++ b/cylc/uiserver/handlers.py
@@ -269,6 +269,9 @@ class UserProfileHandler(CylcAppHandler):
         self.write(json.dumps(user_info))
 
 
+COUNT = 0
+
+
 class UIServerGraphQLHandler(CylcAppHandler, TornadoGraphQLHandler):
     """Endpoint for performing GraphQL queries.
 
@@ -321,7 +324,11 @@ class UIServerGraphQLHandler(CylcAppHandler, TornadoGraphQLHandler):
 
     @web.authenticated  # type: ignore[arg-type]
     async def execute(self, *args, **kwargs) -> 'ExecutionResult':
+        global COUNT
         # Use own backend, and TornadoGraphQLHandler already does validation.
+        COUNT += 1
+        if COUNT < 5:
+            raise web.HTTPError(403, reason='authorization insufficient')
         return await self.schema.execute(
             *args,
             backend=self.backend,

This PR does the job, in the network tab we can see the four failed requests (note we get the error code 400 rather than 403 due to internal implementation details):

The fifth request succeeds. I tested this by opening the mutation menu (which shows the loading status), and waiting for the successful request. The mutation list then populated automatically.

[oliver-sanders]

There are some funky commits on this branch which shouldn't be here.

The easiest way to get rid of them IMO, is a rebase.

# make a safe copy of the branch
git checkout bug-fix-graphql-403
git branch bug-fix-graphql-403.safe

# rebase onto the latest upstream/master
$ git fetch upstream/master
$ git rebase -i upstream/master

In the editor window that opens, delete any commits which aren't related to this branch.

In this case there should only be one commit "added try catch when calling IntrospectionQuery".

Then save and close the rebase file.

[MetRonnie]

Seems to work well. However, instead of retrying indefinitely, might it be better to retry a certain number of times then present an error if it still hasn't worked by that point?

[MetRonnie]

You can save yourself some typing 🙂

Suggested change

	let response = null
	let response

[oliver-sanders]

Maintaining the retry state may be tricky, and websockets will constantly try to reconnect too so I don't think the infinite retry thing is a big issue.

[MetRonnie]

I was thinking more about alerting the user to the problem otherwise there will just be a loading bar instead of the menu, rather than worrying about the cost of retrying indefinitely. But I guess other requests would also fail if it can't fetch the introspection query at all.

[MetRonnie]

But I guess other requests would also fail if it can't fetch the introspection query at all.

Actually this has turned out to be false, there are problems with jupyterhub versions 4.1.x to do with the XRSF cookie that mean only the introspection query fails and so it keeps retrying indefinitely with no success and no error message presented in the UI.

As for what is the cause of the problems with this range of jupyterhub versions, I've dug around and not really found anything illuminating, just that they have been fiddling with the XRSF cookie handling in several quick releases: https://jupyterhub.readthedocs.io/en/stable/reference/changelog.html