Skip to content

Commit fea619d

Browse files
authored
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 element-hq/element-web#29858
1 parent f322f32 commit fea619d

File tree

2 files changed

+64
-5
lines changed

2 files changed

+64
-5
lines changed

spec/unit/http-api/fetch.spec.ts

Lines changed: 61 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -368,13 +368,22 @@ describe("FetchHttpApi", () => {
368368
expect(emitter.emit).not.toHaveBeenCalledWith(HttpApiEvent.SessionLoggedOut, unknownTokenErr);
369369
});
370370

371-
it("should only try to refresh the token once", async () => {
371+
it("should not try to refresh the token if it has plenty of time left before expiry", async () => {
372+
// We can't specify an expiry for the initial token, so this should:
373+
// * Try once, fail
374+
// * Attempt a refresh, get a token that's not expired
375+
// * Try again, still fail
376+
// * Not refresh the token because it's not expired
377+
// ...which is TWO attempts and ONE refresh (which doesn't really
378+
// count because it's only to get a token with an expiry)
372379
const newAccessToken = "new-access-token";
373380
const newRefreshToken = "new-refresh-token";
374-
const tokenRefreshFunction = jest.fn().mockResolvedValue({
381+
const tokenRefreshFunction = jest.fn().mockReturnValue({
375382
accessToken: newAccessToken,
376383
refreshToken: newRefreshToken,
377-
expiry: new Date(Date.now() + 1000),
384+
// This needs to be sufficiently high that it's over the threshold for
385+
// 'plenty of time' (which is a minute in practice).
386+
expiry: new Date(Date.now() + 5 * 60 * 1000),
378387
});
379388

380389
// fetch doesn't like our new or old tokens
@@ -394,7 +403,7 @@ describe("FetchHttpApi", () => {
394403
unknownTokenErr,
395404
);
396405

397-
// tried to refresh the token once
406+
// tried to refresh the token once (to get the one with an expiry)
398407
expect(tokenRefreshFunction).toHaveBeenCalledWith(refreshToken);
399408
expect(tokenRefreshFunction).toHaveBeenCalledTimes(1);
400409

@@ -405,6 +414,54 @@ describe("FetchHttpApi", () => {
405414
// logged out after refreshed access token is rejected
406415
expect(emitter.emit).toHaveBeenCalledWith(HttpApiEvent.SessionLoggedOut, unknownTokenErr);
407416
});
417+
418+
it("should try to refresh the token if it will expire soon", async () => {
419+
const newAccessToken = "new-access-token";
420+
const newRefreshToken = "new-refresh-token";
421+
422+
// first refresh is to get a token with an expiry at all, because we
423+
// can't specify an expiry on the token we inject
424+
const tokenRefreshFunction = jest.fn().mockResolvedValueOnce({
425+
accessToken: newAccessToken,
426+
refreshToken: newRefreshToken,
427+
expiry: new Date(Date.now() + 1000),
428+
});
429+
430+
// next refresh is to return a token that will expire 'soon'
431+
tokenRefreshFunction.mockResolvedValueOnce({
432+
accessToken: newAccessToken,
433+
refreshToken: newRefreshToken,
434+
expiry: new Date(Date.now() + 1000),
435+
});
436+
437+
// ...and finally we return a token that has adequate time left
438+
// so that it will cease retrying and fail the request.
439+
tokenRefreshFunction.mockResolvedValueOnce({
440+
accessToken: newAccessToken,
441+
refreshToken: newRefreshToken,
442+
expiry: new Date(Date.now() + 5 * 60 * 1000),
443+
});
444+
445+
const fetchFn = jest.fn().mockResolvedValue(unknownTokenResponse);
446+
447+
const emitter = new TypedEventEmitter<HttpApiEvent, HttpApiEventHandlerMap>();
448+
jest.spyOn(emitter, "emit");
449+
const api = new FetchHttpApi(emitter, {
450+
baseUrl,
451+
prefix,
452+
fetchFn,
453+
tokenRefreshFunction,
454+
accessToken,
455+
refreshToken,
456+
});
457+
await expect(api.authedRequest(Method.Post, "/account/password")).rejects.toThrow(
458+
unknownTokenErr,
459+
);
460+
461+
// We should have seen the 3 token refreshes, as above.
462+
expect(tokenRefreshFunction).toHaveBeenCalledWith(refreshToken);
463+
expect(tokenRefreshFunction).toHaveBeenCalledTimes(3);
464+
});
408465
});
409466
});
410467
});

src/http-api/refresh.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,9 @@ export class TokenRefresher {
104104
if (snapshot?.expiry) {
105105
// If our token is unknown, but it should not have expired yet, then we should not refresh
106106
const expiresIn = snapshot.expiry.getTime() - Date.now();
107-
if (expiresIn <= REFRESH_ON_ERROR_IF_TOKEN_EXPIRES_WITHIN_MS) {
107+
// If it still has plenty of time left on the clock, we assume something else must be wrong and
108+
// do not refresh. Otherwise if it's expired, or will soon, we try refreshing.
109+
if (expiresIn >= REFRESH_ON_ERROR_IF_TOKEN_EXPIRES_WITHIN_MS) {
108110
return TokenRefreshOutcome.Logout;
109111
}
110112
}

0 commit comments

Comments
 (0)