Fix token refresh behaviour for non-expired tokens (#4825) (#4827)

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

(cherry picked from commit fea619d)

Co-authored-by: David Baker <dbkr@users.noreply.github.com>

Author: RiotRobot

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<HttpApiEvent, HttpApiEventHandlerMap>();
+                        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);
+                    });
                 });
             });
         });

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;
             }
         }