From 72b997d1f3d93f5754875fa873e32d6777b8bb0b Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 27 Feb 2025 18:37:47 +0000 Subject: [PATCH] Fix idempotency issue around token refresh (#4730) * Fix idempotency issue around token refresh Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Improve test Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --------- Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- babel.config.cjs | 1 + package.json | 1 + spec/unit/http-api/fetch.spec.ts | 81 +++++++++++++++++++++++++++----- spec/unit/http-api/index.spec.ts | 3 +- spec/unit/matrix-client.spec.ts | 3 +- src/http-api/fetch.ts | 15 +++++- src/utils/decorators.ts | 39 +++++++++++++++ tsconfig-build.json | 1 - tsconfig.json | 2 +- yarn.lock | 16 +++++++ 10 files changed, 145 insertions(+), 17 deletions(-) create mode 100644 src/utils/decorators.ts diff --git a/babel.config.cjs b/babel.config.cjs index 23ab202ae..4c28757de 100644 --- a/babel.config.cjs +++ b/babel.config.cjs @@ -26,6 +26,7 @@ module.exports = { ], ], plugins: [ + ["@babel/plugin-proposal-decorators", { version: "2023-11" }], "@babel/plugin-transform-numeric-separator", "@babel/plugin-transform-class-properties", "@babel/plugin-transform-object-rest-spread", diff --git a/package.json b/package.json index cc859d7a0..290b5fa2f 100644 --- a/package.json +++ b/package.json @@ -72,6 +72,7 @@ "@babel/core": "^7.12.10", "@babel/eslint-parser": "^7.12.10", "@babel/eslint-plugin": "^7.12.10", + "@babel/plugin-proposal-decorators": "^7.25.9", "@babel/plugin-syntax-dynamic-import": "^7.8.3", "@babel/plugin-transform-class-properties": "^7.12.1", "@babel/plugin-transform-numeric-separator": "^7.12.7", diff --git a/spec/unit/http-api/fetch.spec.ts b/spec/unit/http-api/fetch.spec.ts index 5f6c15892..6f49526b4 100644 --- a/spec/unit/http-api/fetch.spec.ts +++ b/spec/unit/http-api/fetch.spec.ts @@ -125,7 +125,7 @@ describe("FetchHttpApi", () => { ).resolves.toBe(text); }); - it("should send token via query params if useAuthorizationHeader=false", () => { + it("should send token via query params if useAuthorizationHeader=false", async () => { const fetchFn = jest.fn().mockResolvedValue({ ok: true }); const api = new FetchHttpApi(new TypedEventEmitter(), { baseUrl, @@ -134,11 +134,11 @@ describe("FetchHttpApi", () => { accessToken: "token", useAuthorizationHeader: false, }); - api.authedRequest(Method.Get, "/path"); + await api.authedRequest(Method.Get, "/path"); expect(fetchFn.mock.calls[0][0].searchParams.get("access_token")).toBe("token"); }); - it("should send token via headers by default", () => { + it("should send token via headers by default", async () => { const fetchFn = jest.fn().mockResolvedValue({ ok: true }); const api = new FetchHttpApi(new TypedEventEmitter(), { baseUrl, @@ -146,7 +146,7 @@ describe("FetchHttpApi", () => { fetchFn, accessToken: "token", }); - api.authedRequest(Method.Get, "/path"); + await api.authedRequest(Method.Get, "/path"); expect(fetchFn.mock.calls[0][1].headers["Authorization"]).toBe("Bearer token"); }); @@ -163,7 +163,7 @@ describe("FetchHttpApi", () => { expect(fetchFn.mock.calls[0][1].headers["Authorization"]).toBeFalsy(); }); - it("should ensure no token is leaked out via query params if sending via headers", () => { + it("should ensure no token is leaked out via query params if sending via headers", async () => { const fetchFn = jest.fn().mockResolvedValue({ ok: true }); const api = new FetchHttpApi(new TypedEventEmitter(), { baseUrl, @@ -172,12 +172,12 @@ describe("FetchHttpApi", () => { accessToken: "token", useAuthorizationHeader: true, }); - api.authedRequest(Method.Get, "/path", { access_token: "123" }); + await api.authedRequest(Method.Get, "/path", { access_token: "123" }); expect(fetchFn.mock.calls[0][0].searchParams.get("access_token")).toBeFalsy(); expect(fetchFn.mock.calls[0][1].headers["Authorization"]).toBe("Bearer token"); }); - it("should not override manually specified access token via query params", () => { + it("should not override manually specified access token via query params", async () => { const fetchFn = jest.fn().mockResolvedValue({ ok: true }); const api = new FetchHttpApi(new TypedEventEmitter(), { baseUrl, @@ -186,11 +186,11 @@ describe("FetchHttpApi", () => { accessToken: "token", useAuthorizationHeader: false, }); - api.authedRequest(Method.Get, "/path", { access_token: "RealToken" }); + await api.authedRequest(Method.Get, "/path", { access_token: "RealToken" }); expect(fetchFn.mock.calls[0][0].searchParams.get("access_token")).toBe("RealToken"); }); - it("should not override manually specified access token via header", () => { + it("should not override manually specified access token via header", async () => { const fetchFn = jest.fn().mockResolvedValue({ ok: true }); const api = new FetchHttpApi(new TypedEventEmitter(), { baseUrl, @@ -199,16 +199,16 @@ describe("FetchHttpApi", () => { accessToken: "token", useAuthorizationHeader: true, }); - api.authedRequest(Method.Get, "/path", undefined, undefined, { + await api.authedRequest(Method.Get, "/path", undefined, undefined, { headers: { Authorization: "Bearer RealToken" }, }); expect(fetchFn.mock.calls[0][1].headers["Authorization"]).toBe("Bearer RealToken"); }); - it("should not override Accept header", () => { + it("should not override Accept header", async () => { const fetchFn = jest.fn().mockResolvedValue({ ok: true }); const api = new FetchHttpApi(new TypedEventEmitter(), { baseUrl, prefix, fetchFn }); - api.authedRequest(Method.Get, "/path", undefined, undefined, { + await api.authedRequest(Method.Get, "/path", undefined, undefined, { headers: { Accept: "text/html" }, }); expect(fetchFn.mock.calls[0][1].headers["Accept"]).toBe("text/html"); @@ -468,4 +468,61 @@ describe("FetchHttpApi", () => { ] `); }); + + it("should not make multiple concurrent refresh token requests", async () => { + const tokenInactiveError = new MatrixError({ errcode: "M_UNKNOWN_TOKEN", error: "Token is not active" }, 401); + + const deferredTokenRefresh = defer<{ accessToken: string; refreshToken: string }>(); + const fetchFn = jest.fn().mockResolvedValue({ + ok: false, + status: tokenInactiveError.httpStatus, + async text() { + return JSON.stringify(tokenInactiveError.data); + }, + async json() { + return tokenInactiveError.data; + }, + headers: { + get: jest.fn().mockReturnValue("application/json"), + }, + }); + const tokenRefreshFunction = jest.fn().mockReturnValue(deferredTokenRefresh.promise); + + const api = new FetchHttpApi(new TypedEventEmitter(), { + baseUrl, + prefix, + fetchFn, + doNotAttemptTokenRefresh: false, + tokenRefreshFunction, + accessToken: "ACCESS_TOKEN", + refreshToken: "REFRESH_TOKEN", + }); + + const prom1 = api.authedRequest(Method.Get, "/path1"); + const prom2 = api.authedRequest(Method.Get, "/path2"); + + await jest.advanceTimersByTimeAsync(10); // wait for requests to fire + expect(fetchFn).toHaveBeenCalledTimes(2); + fetchFn.mockResolvedValue({ + ok: true, + status: 200, + async text() { + return "{}"; + }, + async json() { + return {}; + }, + headers: { + get: jest.fn().mockReturnValue("application/json"), + }, + }); + deferredTokenRefresh.resolve({ accessToken: "NEW_ACCESS_TOKEN", refreshToken: "NEW_REFRESH_TOKEN" }); + + await prom1; + await prom2; + expect(fetchFn).toHaveBeenCalledTimes(4); // 2 original calls + 2 retries + expect(tokenRefreshFunction).toHaveBeenCalledTimes(1); + expect(api.opts.accessToken).toBe("NEW_ACCESS_TOKEN"); + expect(api.opts.refreshToken).toBe("NEW_REFRESH_TOKEN"); + }); }); diff --git a/spec/unit/http-api/index.spec.ts b/spec/unit/http-api/index.spec.ts index 0145c9aea..9eb321891 100644 --- a/spec/unit/http-api/index.spec.ts +++ b/spec/unit/http-api/index.spec.ts @@ -59,11 +59,12 @@ describe("MatrixHttpApi", () => { xhr.onreadystatechange?.(new Event("test")); }); - it("should fall back to `fetch` where xhr is unavailable", () => { + it("should fall back to `fetch` where xhr is unavailable", async () => { globalThis.XMLHttpRequest = undefined!; const fetchFn = jest.fn().mockResolvedValue({ ok: true, json: jest.fn().mockResolvedValue({}) }); const api = new MatrixHttpApi(new TypedEventEmitter(), { baseUrl, prefix, fetchFn }); upload = api.uploadContent({} as File); + await upload; expect(fetchFn).toHaveBeenCalled(); }); diff --git a/spec/unit/matrix-client.spec.ts b/spec/unit/matrix-client.spec.ts index 35a2016a0..a9c0d3f7d 100644 --- a/spec/unit/matrix-client.spec.ts +++ b/spec/unit/matrix-client.spec.ts @@ -2753,12 +2753,13 @@ describe("MatrixClient", function () { // WHEN we call `setAccountData` ... const setProm = client.setAccountData(eventType, content); + await jest.advanceTimersByTimeAsync(10); // THEN, the REST call should have happened, and had the correct content const lastCall = fetchMock.lastCall("put-account-data"); expect(lastCall).toBeDefined(); expect(lastCall?.[1]?.body).toEqual(JSON.stringify(content)); - // Even after waiting a bit, the method should not yet have returned + // Even after waiting a bit more, the method should not yet have returned await jest.advanceTimersByTimeAsync(10); let finished = false; setProm.finally(() => (finished = true)); diff --git a/src/http-api/fetch.ts b/src/http-api/fetch.ts index 258ce0b71..3943fb666 100644 --- a/src/http-api/fetch.ts +++ b/src/http-api/fetch.ts @@ -31,6 +31,7 @@ import { } from "./interface.ts"; import { anySignal, parseErrorResponse, timeoutSignal } from "./utils.ts"; import { type QueryDict } from "../utils.ts"; +import { singleAsyncExecution } from "../utils/decorators.ts"; interface TypedResponse extends Response { json(): Promise; @@ -106,6 +107,12 @@ export class FetchHttpApi { return this.requestOtherUrl(method, fullUri, body, opts); } + /** + * Promise used to block authenticated requests during a token refresh to avoid repeated expected errors. + * @private + */ + private tokenRefreshPromise?: Promise; + /** * Perform an authorised request to the homeserver. * @param method - The HTTP method e.g. "GET". @@ -162,13 +169,17 @@ export class FetchHttpApi { } try { + // Await any ongoing token refresh + await this.tokenRefreshPromise; const response = await this.request(method, path, queryParams, body, opts); return response; } catch (error) { const err = error as MatrixError; if (err.errcode === "M_UNKNOWN_TOKEN" && !opts.doNotAttemptTokenRefresh) { - const shouldRetry = await this.tryRefreshToken(); + const tokenRefreshPromise = this.tryRefreshToken(); + this.tokenRefreshPromise = Promise.allSettled([tokenRefreshPromise]); + const shouldRetry = await tokenRefreshPromise; // if we got a new token retry the request if (shouldRetry) { return this.authedRequest(method, path, queryParams, body, { @@ -177,6 +188,7 @@ export class FetchHttpApi { }); } } + // otherwise continue with error handling if (err.errcode == "M_UNKNOWN_TOKEN" && !opts?.inhibitLogoutEmit) { this.eventEmitter.emit(HttpApiEvent.SessionLoggedOut, err); @@ -193,6 +205,7 @@ export class FetchHttpApi { * On success, sets new access and refresh tokens in opts. * @returns Promise that resolves to a boolean - true when token was refreshed successfully */ + @singleAsyncExecution private async tryRefreshToken(): Promise { if (!this.opts.refreshToken || !this.opts.tokenRefreshFunction) { return false; diff --git a/src/utils/decorators.ts b/src/utils/decorators.ts new file mode 100644 index 000000000..f95391b8b --- /dev/null +++ b/src/utils/decorators.ts @@ -0,0 +1,39 @@ +/* +Copyright 2025 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +/** + * Method decorator to ensure that only one instance of the method is running at a time, + * and any concurrent calls will return the same promise as the original call. + * After execution is complete a new call will be able to run the method again. + */ +export function singleAsyncExecution( + target: (this: This, ...args: Args) => Promise, +): (this: This, ...args: Args) => Promise { + let promise: Promise | undefined; + + async function replacementMethod(this: This, ...args: Args): Promise { + if (promise) return promise; + try { + promise = target.call(this, ...args); + await promise; + return promise; + } finally { + promise = undefined; + } + } + + return replacementMethod; +} diff --git a/tsconfig-build.json b/tsconfig-build.json index 5437c65b2..eaeee235b 100644 --- a/tsconfig-build.json +++ b/tsconfig-build.json @@ -5,7 +5,6 @@ "declarationMap": true, "sourceMap": true, "noEmit": false, - "emitDecoratorMetadata": true, "outDir": "./lib", "rootDir": "src" }, diff --git a/tsconfig.json b/tsconfig.json index 9339d6dfd..6b99f40fa 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -1,7 +1,7 @@ { "compilerOptions": { "target": "es2022", - "experimentalDecorators": true, + "experimentalDecorators": false, "esModuleInterop": true, "module": "commonjs", "moduleResolution": "node", diff --git a/yarn.lock b/yarn.lock index 7b7f9e2a9..e7724b166 100644 --- a/yarn.lock +++ b/yarn.lock @@ -394,6 +394,15 @@ "@babel/helper-plugin-utils" "^7.25.9" "@babel/traverse" "^7.25.9" +"@babel/plugin-proposal-decorators@^7.25.9": + version "7.25.9" + resolved "https://registry.yarnpkg.com/@babel/plugin-proposal-decorators/-/plugin-proposal-decorators-7.25.9.tgz#8680707f943d1a3da2cd66b948179920f097e254" + integrity sha512-smkNLL/O1ezy9Nhy4CNosc4Va+1wo5w4gzSZeLe6y6dM4mmHfYOCPolXQPHQxonZCF+ZyebxN9vqOolkYrSn5g== + dependencies: + "@babel/helper-create-class-features-plugin" "^7.25.9" + "@babel/helper-plugin-utils" "^7.25.9" + "@babel/plugin-syntax-decorators" "^7.25.9" + "@babel/plugin-proposal-private-property-in-object@7.21.0-placeholder-for-preset-env.2": version "7.21.0-placeholder-for-preset-env.2" resolved "https://registry.yarnpkg.com/@babel/plugin-proposal-private-property-in-object/-/plugin-proposal-private-property-in-object-7.21.0-placeholder-for-preset-env.2.tgz#7844f9289546efa9febac2de4cfe358a050bd703" @@ -420,6 +429,13 @@ dependencies: "@babel/helper-plugin-utils" "^7.12.13" +"@babel/plugin-syntax-decorators@^7.25.9": + version "7.25.9" + resolved "https://registry.yarnpkg.com/@babel/plugin-syntax-decorators/-/plugin-syntax-decorators-7.25.9.tgz#986b4ca8b7b5df3f67cee889cedeffc2e2bf14b3" + integrity sha512-ryzI0McXUPJnRCvMo4lumIKZUzhYUO/ScI+Mz4YVaTLt04DHNSjEUjKVvbzQjZFLuod/cYEc07mJWhzl6v4DPg== + dependencies: + "@babel/helper-plugin-utils" "^7.25.9" + "@babel/plugin-syntax-dynamic-import@^7.8.3": version "7.8.3" resolved "https://registry.yarnpkg.com/@babel/plugin-syntax-dynamic-import/-/plugin-syntax-dynamic-import-7.8.3.tgz#62bf98b2da3cd21d626154fc96ee5b3cb68eacb3"