Skip to content

Commit

Permalink
Don't use expired access token in web component
Browse files Browse the repository at this point in the history
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
  • Loading branch information
floehopper committed Jun 19, 2024
1 parent 4be26c8 commit eac96dd
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 3 deletions.
3 changes: 2 additions & 1 deletion cypress/e2e/spec-wc.cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
7 changes: 6 additions & 1 deletion src/containers/WebComponentLoader.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
48 changes: 47 additions & 1 deletion src/containers/WebComponentLoader.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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(
<Provider store={store}>
<CookiesProvider cookies={cookies}>
<WebComponentLoader
code={code}
identifier={identifier}
senseHatAlwaysEnabled={true}
instructions={instructions}
authKey={authKey}
theme="light"
/>
</CookiesProvider>
</Provider>,
);
});

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(
Expand Down

0 comments on commit eac96dd

Please sign in to comment.