From fea619d34cfdf4ddb9871f1f11c214711db6d5f7 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 6 May 2025 10:39:13 +0100 Subject: [PATCH] Fix token refresh behaviour for non-expired tokens (#4825) The condition was inverted here, but the tests were passing because they didn't add enough expiry time for the token expiry to be over the threshold. Fix the condition and tests, add another test and generally add a bunch of comments so hopefully this is less confusing for the next person. Fixes https://github.com/element-hq/element-web/issues/29858 --- spec/unit/http-api/fetch.spec.ts | 65 ++++++++++++++++++++++++++++++-- src/http-api/refresh.ts | 4 +- 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/spec/unit/http-api/fetch.spec.ts b/spec/unit/http-api/fetch.spec.ts index ad30d8672..ae4ee8017 100644 --- a/spec/unit/http-api/fetch.spec.ts +++ b/spec/unit/http-api/fetch.spec.ts @@ -368,13 +368,22 @@ describe("FetchHttpApi", () => { expect(emitter.emit).not.toHaveBeenCalledWith(HttpApiEvent.SessionLoggedOut, unknownTokenErr); }); - it("should only try to refresh the token once", async () => { + it("should not try to refresh the token if it has plenty of time left before expiry", async () => { + // We can't specify an expiry for the initial token, so this should: + // * Try once, fail + // * Attempt a refresh, get a token that's not expired + // * Try again, still fail + // * Not refresh the token because it's not expired + // ...which is TWO attempts and ONE refresh (which doesn't really + // count because it's only to get a token with an expiry) const newAccessToken = "new-access-token"; const newRefreshToken = "new-refresh-token"; - const tokenRefreshFunction = jest.fn().mockResolvedValue({ + const tokenRefreshFunction = jest.fn().mockReturnValue({ accessToken: newAccessToken, refreshToken: newRefreshToken, - expiry: new Date(Date.now() + 1000), + // This needs to be sufficiently high that it's over the threshold for + // 'plenty of time' (which is a minute in practice). + expiry: new Date(Date.now() + 5 * 60 * 1000), }); // fetch doesn't like our new or old tokens @@ -394,7 +403,7 @@ describe("FetchHttpApi", () => { unknownTokenErr, ); - // tried to refresh the token once + // tried to refresh the token once (to get the one with an expiry) expect(tokenRefreshFunction).toHaveBeenCalledWith(refreshToken); expect(tokenRefreshFunction).toHaveBeenCalledTimes(1); @@ -405,6 +414,54 @@ describe("FetchHttpApi", () => { // logged out after refreshed access token is rejected expect(emitter.emit).toHaveBeenCalledWith(HttpApiEvent.SessionLoggedOut, unknownTokenErr); }); + + it("should try to refresh the token if it will expire soon", async () => { + const newAccessToken = "new-access-token"; + const newRefreshToken = "new-refresh-token"; + + // first refresh is to get a token with an expiry at all, because we + // can't specify an expiry on the token we inject + const tokenRefreshFunction = jest.fn().mockResolvedValueOnce({ + accessToken: newAccessToken, + refreshToken: newRefreshToken, + expiry: new Date(Date.now() + 1000), + }); + + // next refresh is to return a token that will expire 'soon' + tokenRefreshFunction.mockResolvedValueOnce({ + accessToken: newAccessToken, + refreshToken: newRefreshToken, + expiry: new Date(Date.now() + 1000), + }); + + // ...and finally we return a token that has adequate time left + // so that it will cease retrying and fail the request. + tokenRefreshFunction.mockResolvedValueOnce({ + accessToken: newAccessToken, + refreshToken: newRefreshToken, + expiry: new Date(Date.now() + 5 * 60 * 1000), + }); + + const fetchFn = jest.fn().mockResolvedValue(unknownTokenResponse); + + const emitter = new TypedEventEmitter(); + jest.spyOn(emitter, "emit"); + const api = new FetchHttpApi(emitter, { + baseUrl, + prefix, + fetchFn, + tokenRefreshFunction, + accessToken, + refreshToken, + }); + await expect(api.authedRequest(Method.Post, "/account/password")).rejects.toThrow( + unknownTokenErr, + ); + + // We should have seen the 3 token refreshes, as above. + expect(tokenRefreshFunction).toHaveBeenCalledWith(refreshToken); + expect(tokenRefreshFunction).toHaveBeenCalledTimes(3); + }); }); }); }); diff --git a/src/http-api/refresh.ts b/src/http-api/refresh.ts index 4bde53c1b..3ebff66d2 100644 --- a/src/http-api/refresh.ts +++ b/src/http-api/refresh.ts @@ -104,7 +104,9 @@ export class TokenRefresher { if (snapshot?.expiry) { // If our token is unknown, but it should not have expired yet, then we should not refresh const expiresIn = snapshot.expiry.getTime() - Date.now(); - if (expiresIn <= REFRESH_ON_ERROR_IF_TOKEN_EXPIRES_WITHIN_MS) { + // If it still has plenty of time left on the clock, we assume something else must be wrong and + // do not refresh. Otherwise if it's expired, or will soon, we try refreshing. + if (expiresIn >= REFRESH_ON_ERROR_IF_TOKEN_EXPIRES_WITHIN_MS) { return TokenRefreshOutcome.Logout; } }