Handle unexpected token refresh failures gracefully (#4731)

* Fix idempotency issue around token refresh

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Improve test

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Handle unexpected token refresh failures gracefully

e.g. connection errors, proxy errors differently from token invalidated errors

Signed-off-by: Michael Telatynski <[email protected]>

---------

Signed-off-by: Michael Telatynski <[email protected]>

Author: t3chguy

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

@@ -20,6 +20,7 @@ import { FetchHttpApi } from "../../../src/http-api/fetch";
 import { TypedEventEmitter } from "../../../src/models/typed-event-emitter";
 import {
     ClientPrefix,
+    ConnectionError,
     HttpApiEvent,
     type HttpApiEventHandlerMap,
     IdentityPrefix,
@@ -288,7 +289,7 @@ describe("FetchHttpApi", () => {
 
                 describe("with a tokenRefreshFunction", () => {
                     it("should emit logout and throw when token refresh fails", async () => {
-                        const error = new Error("uh oh");
+                        const error = new MatrixError();
                         const tokenRefreshFunction = jest.fn().mockRejectedValue(error);
                         const fetchFn = jest.fn().mockResolvedValue(unknownTokenResponse);
                         const emitter = new TypedEventEmitter<HttpApiEvent, HttpApiEventHandlerMap>();
@@ -308,6 +309,27 @@ describe("FetchHttpApi", () => {
                         expect(emitter.emit).toHaveBeenCalledWith(HttpApiEvent.SessionLoggedOut, unknownTokenErr);
                     });
 
+                    it("should not emit logout but still throw when token refresh fails due to transitive fault", async () => {
+                        const error = new ConnectionError("transitive fault");
+                        const tokenRefreshFunction = jest.fn().mockRejectedValue(error);
+                        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,
+                        );
+                        expect(tokenRefreshFunction).toHaveBeenCalledWith(refreshToken);
+                        expect(emitter.emit).not.toHaveBeenCalledWith(HttpApiEvent.SessionLoggedOut, unknownTokenErr);
+                    });
+
                     it("should refresh token and retry request", async () => {
                         const newAccessToken = "new-access-token";
                         const newRefreshToken = "new-refresh-token";

src/http-api/errors.ts

@@ -197,3 +197,18 @@ export class ConnectionError extends Error {
         return "ConnectionError";
     }
 }
+
+/**
+ * Construct a TokenRefreshError. This indicates that a request failed due to the token being expired,
+ * and attempting to refresh said token also failed but in a way which was not indicative of token invalidation.
+ * Assumed to be a temporary failure.
+ */
+export class TokenRefreshError extends Error {
+    public constructor(cause?: Error) {
+        super(cause?.message ?? "");
+    }
+
+    public get name(): string {
+        return "TokenRefreshError";
+    }
+}

src/http-api/fetch.ts

@@ -18,10 +18,12 @@ limitations under the License.
  * This is an internal module. See {@link MatrixHttpApi} for the public class.
  */
 
+import { ErrorResponse as OidcAuthError } from "oidc-client-ts";
+
 import { checkObjectHasKeys, encodeParams } from "../utils.ts";
 import { type TypedEventEmitter } from "../models/typed-event-emitter.ts";
 import { Method } from "./method.ts";
-import { ConnectionError, type MatrixError } from "./errors.ts";
+import { ConnectionError, MatrixError, TokenRefreshError } from "./errors.ts";
 import {
     HttpApiEvent,
     type HttpApiEventHandlerMap,
@@ -43,6 +45,12 @@ export type ResponseType<T, O extends IHttpOpts> = O extends undefined
       ? T
       : TypedResponse<T>;
 
+const enum TokenRefreshOutcome {
+    Success = "success",
+    Failure = "failure",
+    Logout = "logout",
+}
+
 export class FetchHttpApi<O extends IHttpOpts> {
     private abortController = new AbortController();
 
@@ -174,29 +182,36 @@ export class FetchHttpApi<O extends IHttpOpts> {
             const response = await this.request<T>(method, path, queryParams, body, opts);
             return response;
         } catch (error) {
-            const err = error as MatrixError;
+            if (!(error instanceof MatrixError)) {
+                throw error;
+            }
 
-            if (err.errcode === "M_UNKNOWN_TOKEN" && !opts.doNotAttemptTokenRefresh) {
+            if (error.errcode === "M_UNKNOWN_TOKEN" && !opts.doNotAttemptTokenRefresh) {
                 const tokenRefreshPromise = this.tryRefreshToken();
                 this.tokenRefreshPromise = Promise.allSettled([tokenRefreshPromise]);
-                const shouldRetry = await tokenRefreshPromise;
-                // if we got a new token retry the request
-                if (shouldRetry) {
+                const outcome = await tokenRefreshPromise;
+
+                if (outcome === TokenRefreshOutcome.Success) {
+                    // if we got a new token retry the request
                     return this.authedRequest(method, path, queryParams, body, {
                         ...paramOpts,
                         doNotAttemptTokenRefresh: true,
                     });
                 }
+                if (outcome === TokenRefreshOutcome.Failure) {
+                    throw new TokenRefreshError(error);
+                }
+                // Fall through to SessionLoggedOut handler below
             }
 
             // otherwise continue with error handling
-            if (err.errcode == "M_UNKNOWN_TOKEN" && !opts?.inhibitLogoutEmit) {
-                this.eventEmitter.emit(HttpApiEvent.SessionLoggedOut, err);
-            } else if (err.errcode == "M_CONSENT_NOT_GIVEN") {
-                this.eventEmitter.emit(HttpApiEvent.NoConsent, err.message, err.data.consent_uri);
+            if (error.errcode == "M_UNKNOWN_TOKEN" && !opts?.inhibitLogoutEmit) {
+                this.eventEmitter.emit(HttpApiEvent.SessionLoggedOut, error);
+            } else if (error.errcode == "M_CONSENT_NOT_GIVEN") {
+                this.eventEmitter.emit(HttpApiEvent.NoConsent, error.message, error.data.consent_uri);
             }
 
-            throw err;
+            throw error;
         }
     }
 
@@ -206,20 +221,23 @@ export class FetchHttpApi<O extends IHttpOpts> {
      * @returns Promise that resolves to a boolean - true when token was refreshed successfully
      */
     @singleAsyncExecution
-    private async tryRefreshToken(): Promise<boolean> {
+    private async tryRefreshToken(): Promise<TokenRefreshOutcome> {
         if (!this.opts.refreshToken || !this.opts.tokenRefreshFunction) {
-            return false;
+            return TokenRefreshOutcome.Logout;
         }
 
         try {
             const { accessToken, refreshToken } = await this.opts.tokenRefreshFunction(this.opts.refreshToken);
             this.opts.accessToken = accessToken;
             this.opts.refreshToken = refreshToken;
             // successfully got new tokens
-            return true;
+            return TokenRefreshOutcome.Success;
         } catch (error) {
             this.opts.logger?.warn("Failed to refresh token", error);
-            return false;
+            if (error instanceof OidcAuthError || error instanceof MatrixError) {
+                return TokenRefreshOutcome.Logout;
+            }
+            return TokenRefreshOutcome.Failure;
         }
     }