search

RaspberryPiFoundation / editor-ui

Code Editor web component
https://editor-static.raspberrypi.org
Apache License 2.0
37 stars 8 forks source link

Avoid using stale access tokens in requests to Editor API #1044

Closed chrisroos closed 1 week ago

chrisroos commented 2 weeks ago

We've been seeing quite a few Faraday::Unauthorized errors reported by Sentry in Editor API. These errors are being caused by stale access tokens being sent in the requests from the WebComponent.

I've narrowed the problem down to this line in WebComponentLoader which was introduced in this commit by James M. James's fix works in the case where the user stored in localstorage has an active (i.e. not expired) access token but causes failures if the access token has expired. I contemplated fixing this by checking the expiry date of the access token in WebComponentLoader but that doesn't feel quite right because that must already be happening somewhere else in the app (maybe in a library we're using for oidc)? Instead, I think it might be better if the WebComponentLoader only reads the user from state (and not from localstorage), and we rely on the component re-rendering when the user becomes available in the state. We know that the WebComponentLoader component renders (and therefore makes a request to editor-api with no Authorization header) before the user is available in state. This is fine for public projects (i.e. those where user_id is null) but fails for user projects (i.e. those where user_id is set). When the user becomes available in state I'd expect the component to re-render and make a request to editor-api with the Authorization header set correctly. At present, this second request doesn't happen because of the condition in this line of syncProject.

create-issue-branch[bot] commented 2 weeks ago

Branch issues/1044-Avoid_using_stale_access_tokens_in_requests_to_Editor_API created!

chrisroos commented 2 weeks ago

I've been playing around with some ideas for this in PR #1043.

chrisroos commented 2 weeks ago

I wonder whether it might be worth going with the quicker fix for now, of not loading the user from localstorage if the access token has expired, as I think that might help reduce the errors we're seeing in production?

floehopper commented 2 weeks ago

As suggested by @chrisroos I've worked up the quick fix into https://github.com/RaspberryPiFoundation/editor-ui/pull/1046 to address this issue.

floehopper commented 2 weeks ago

After discussing this with @chrisroos & @sra405 this morning, we agreed to revert #1046, because it has been addressed by RaspberryPi/editor-api#337. It has highlighted some other potential issues which I've tried to detail in https://github.com/RaspberryPiFoundation/editor-ui/issues/1050. Moving this issue to "Ready to deploy", but maybe we want to close it...?