Avoid using stale access tokens in requests to Editor API #1044
Assignees
Comments
|
I've been playing around with some ideas for this in PR #1043. |
|
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
added a commit
that referenced
this issue
Jun 19, 2024
This is a quick fix for #1044. We've been seeing quite a few `Faraday::Unauthorized` exceptions reported by Sentry in Editor API [1]. These errors are being caused by expired access tokens being sent in the requests from the web component. This can happen if a user is logged in and is viewing a non-public project, but they close their browser and return to the project some time (1 hour?) later when their access token has expired. In this scenario the project will not load, because of the exception and the user will see the "Loading" text instead of the editor. This commit prevents the web component from using an expired access token in `WebComponentLoader` and should thus avoid the exeptions we've been seeing. However, it's not clear this is actually the correct place to fix the problem, because this ignoring/removal of expired access tokens must already be happening (maybe in `oidc-client`, `redux-oidc`, or associated code?). It might be better if the `WebComponentLoader` only reads the `user` from the redux state (and not also from local storage) and then 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 we'd expect the component to re-render and make a request to `editor-api` with the `Authorization` header set correctly. However, this second request doesn't currently happen because of the condition in this line [2] of `syncProject` in `EditorSlice`, so we'd need to make a change there too. The main downside of not making the proper fix is that there may be times when the logged-in "state" of different bits of the UI (e.g. the "Save" button, the global nav, etc) might be inconsistent in some circumstances. However, since the proper fix is likely to be a more significant bit of work, it seems sensible to ship this change now and tackle the proper fix separately. [1]: https://rpf.sentry.io/issues/5135885959/ [2]: https://github.com/RaspberryPiFoundation/editor-ui/blob/e3495a24ff296cf0c3c22910db25d02547c6dfd9/src/redux/EditorSlice.js#L65
floehopper
added a commit
that referenced
this issue
Jun 19, 2024
This is a quick fix for #1044. We've been seeing quite a few `Faraday::Unauthorized` exceptions reported by Sentry in Editor API [1]. These errors are being caused by expired access tokens being sent in the requests from the web component. This can happen if a user is logged in and is viewing a non-public project, but they close their browser and return to the project some time (1 hour?) later when their access token has expired. In this scenario the project will not load, because of the exception and the user will see the "Loading" text instead of the editor. This commit prevents the web component from using an expired access token in `WebComponentLoader` and should thus avoid the exeptions we've been seeing. However, it's not clear this is actually the correct place to fix the problem, because this ignoring/removal of expired access tokens must already be happening (maybe in `oidc-client`, `redux-oidc`, or associated code?). It might be better if the `WebComponentLoader` only reads the `user` from the redux state (and not also from local storage) and then 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 we'd expect the component to re-render and make a request to `editor-api` with the `Authorization` header set correctly. However, this second request doesn't currently happen because of the condition in this line [2] of `syncProject` in `EditorSlice`, so we'd need to make a change there too. The main downside of not making the proper fix is that there may be times when the logged-in "state" of different bits of the UI (e.g. the "Save" button, the global nav, etc) might be inconsistent in some circumstances. However, since the proper fix is likely to be a more significant bit of work, it seems sensible to ship this change now and tackle the proper fix separately. [1]: https://rpf.sentry.io/issues/5135885959/ [2]: https://github.com/RaspberryPiFoundation/editor-ui/blob/e3495a24ff296cf0c3c22910db25d02547c6dfd9/src/redux/EditorSlice.js#L65
|
As suggested by @chrisroos I've worked up the quick fix into #1046 to address this issue. |
floehopper
added a commit
to RaspberryPiFoundation/editor-api
that referenced
this issue
Jun 19, 2024
Previously, when an API request was made using an expired or invalid access token, a `Faraday::UnauthorizedError` was raised and the request failed. Recently we've seen a bunch of these exceptions happening due to some other problems in editor-standalone and/or in the editor-ui web component - see this issue [1] for more details. Failing hard with an exception like this seems a bit over the top when the user is trying to view a public project for which they don't need to be logged-in. And it seems as if `User.from_token` might have been expecting `HydraPublicApiClient.fetch_oauth_user` to return `nil` when the token was invalid [2]. This commit rescues the `Faraday::UnauthorizedError` exception, captures the exception in Sentry in case we want to know about it, but then returns `nil`. This means that if the user is trying to carry out an action that does not require them to be logged-in, they can still do so despite their access token not being valid. [1]: RaspberryPiFoundation/editor-ui#1044 [2]: https://github.com/RaspberryPiFoundation/editor-api/blob/055741503b0ad295e44993c9f55b2fc95e912beb/app/models/user.rb#L91
floehopper
added a commit
to RaspberryPiFoundation/editor-api
that referenced
this issue
Jun 19, 2024
Previously, when an API request was made using an expired or invalid access token, a `Faraday::UnauthorizedError` was raised and the request failed. Recently we've seen a bunch of these exceptions [1] happening due to some other problems in editor-standalone and/or in the editor-ui web component - see this issue [2] for more details. Failing hard with an exception like this seems a bit over the top when the user is trying to view a public project for which they don't need to be logged-in. And it seems as if `User.from_token` might have been expecting [3] `HydraPublicApiClient.fetch_oauth_user` to return `nil` when the token was invalid when in fact it returns a `401 Unauthorized` HTTP status code which results in a `Faraday::UnauthorizedError` exception being raised. This commit rescues the `Faraday::UnauthorizedError` exception, captures the exception in Sentry in case we want to know about it, but then returns `nil`. This means that if the user is trying to carry out an action that does not require them to be logged-in, they can still do so despite their access token not being valid. [1]: https://rpf.sentry.io/issues/5135885959/events/?project=6143981 [2]: RaspberryPiFoundation/editor-ui#1044 [3]: https://github.com/RaspberryPiFoundation/editor-api/blob/055741503b0ad295e44993c9f55b2fc95e912beb/app/models/user.rb#L91
|
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 #1050. Moving this issue to "Ready to deploy", but maybe we want to close it...? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We've been seeing quite a few
Faraday::Unauthorizederrors 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
WebComponentLoaderbut 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 theWebComponentLoaderonly 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 theWebComponentLoadercomponent renders (and therefore makes a request to editor-api with noAuthorizationheader) before the user is available in state. This is fine for public projects (i.e. those whereuser_idis null) but fails for user projects (i.e. those whereuser_idis set). When the user becomes available in state I'd expect the component to re-render and make a request to editor-api with theAuthorizationheader set correctly. At present, this second request doesn't happen because of the condition in this line ofsyncProject.The text was updated successfully, but these errors were encountered: