From eac96dd91d1af03dd8e2d09bbbd862a9d1c2ef0f Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 19 Jun 2024 11:34:30 +0100 Subject: [PATCH] Don't use expired access token in web component 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 --- cypress/e2e/spec-wc.cy.js | 3 +- src/containers/WebComponentLoader.jsx | 7 +++- src/containers/WebComponentLoader.test.js | 48 ++++++++++++++++++++++- 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/cypress/e2e/spec-wc.cy.js b/cypress/e2e/spec-wc.cy.js index c27dc7444..e9a10b118 100644 --- a/cypress/e2e/spec-wc.cy.js +++ b/cypress/e2e/spec-wc.cy.js @@ -94,7 +94,8 @@ describe("default behaviour", () => { describe("when load_remix_disabled is true, e.g. in editor-standalone", () => { const authKey = `oidc.user:https://auth-v1.raspberrypi.org:editor-api`; - const user = { access_token: "dummy-access-token" }; + const oneHourFromNow = (new Date().getTime() + 60 * 60 * 1000) / 1000; + const user = { access_token: "dummy-access-token", expires_at: oneHourFromNow }; const originalIdentifier = "blank-python-starter"; const urlFor = (identifier) => { diff --git a/src/containers/WebComponentLoader.jsx b/src/containers/WebComponentLoader.jsx index dca43d3c4..142c3281c 100644 --- a/src/containers/WebComponentLoader.jsx +++ b/src/containers/WebComponentLoader.jsx @@ -52,7 +52,12 @@ const WebComponentLoader = (props) => { if (authKey) { const user = JSON.parse(localStorage.getItem(authKey)); if (user) { - return user; + const expiresAt = new Date(user.expires_at * 1000); + if (new Date() < expiresAt) { + return user; + } else { + return null; + } } else { return null; } diff --git a/src/containers/WebComponentLoader.test.js b/src/containers/WebComponentLoader.test.js index 689e7625f..82fdd0ed9 100644 --- a/src/containers/WebComponentLoader.test.js +++ b/src/containers/WebComponentLoader.test.js @@ -26,7 +26,8 @@ const identifier = "My amazing project"; const steps = [{ quiz: false, title: "Step 1", content: "Do something" }]; const instructions = { currentStepPosition: 3, project: { steps: steps } }; const authKey = "my_key"; -const user = { access_token: "my_token" }; +const oneHourFromNow = (new Date().getTime() + 60 * 60 * 1000) / 1000; +const user = { access_token: "my_token", expires_at: oneHourFromNow }; describe("When no user is in state", () => { beforeEach(() => { @@ -232,6 +233,51 @@ describe("When no user is in state", () => { }); }); + describe("with user set in local storage, but access token has expired", () => { + beforeEach(() => { + const oneHourAgo = (new Date().getTime() - 60 * 60 * 1000) / 1000; + user.expires_at = oneHourAgo; + + localStorage.setItem(authKey, JSON.stringify(user)); + localStorage.setItem("authKey", authKey); + render( + + + + + , + ); + }); + + test("Calls useProject hook as if user was not set in local storage", () => { + expect(useProject).toHaveBeenCalledWith({ + projectIdentifier: identifier, + code, + accessToken: undefined, + loadRemix: false, + loadCache: true, + remixLoadFailed: false, + }); + }); + + test("Calls useProjectPersistence hook as if user was not set in local storage", () => { + expect(useProjectPersistence).toHaveBeenCalledWith({ + user: null, + project: { components: [] }, + hasShownSavePrompt: true, + justLoaded: false, + saveTriggered: false, + }); + }); + }); + describe("When theme is not set", () => { beforeEach(() => { render(