mirror of
https://github.com/matrix-org/matrix-js-sdk.git
synced 2025-04-18 07:04:03 +03:00
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>
This commit is contained in:
Michael Telatynski
GitHub
parent
a3bbc49e02
commit
72b997d1f3
@ -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",
|
||||
|
||||
@ -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",
|
||||
|
||||
@ -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<any, any>(), {
|
||||
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<any, any>(), {
|
||||
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<any, any>(), {
|
||||
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<any, any>(), {
|
||||
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<any, any>(), {
|
||||
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<any, any>(), { 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<any, any>(), {
|
||||
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");
|
||||
});
|
||||
});
|
||||
|
||||
@ -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<any, any>(), { baseUrl, prefix, fetchFn });
|
||||
upload = api.uploadContent({} as File);
|
||||
await upload;
|
||||
expect(fetchFn).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
|
||||
@ -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));
|
||||
|
||||
@ -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<T> extends Response {
|
||||
json(): Promise<T>;
|
||||
@ -106,6 +107,12 @@ export class FetchHttpApi<O extends IHttpOpts> {
|
||||
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<unknown>;
|
||||
|
||||
/**
|
||||
* Perform an authorised request to the homeserver.
|
||||
* @param method - The HTTP method e.g. "GET".
|
||||
@ -162,13 +169,17 @@ export class FetchHttpApi<O extends IHttpOpts> {
|
||||
}
|
||||
|
||||
try {
|
||||
// Await any ongoing token refresh
|
||||
await this.tokenRefreshPromise;
|
||||
const response = await this.request<T>(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<O extends IHttpOpts> {
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// 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<O extends IHttpOpts> {
|
||||
* 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<boolean> {
|
||||
if (!this.opts.refreshToken || !this.opts.tokenRefreshFunction) {
|
||||
return false;
|
||||
|
||||
39
src/utils/decorators.ts
Normal file
39
src/utils/decorators.ts
Normal file
@ -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<This, Args extends unknown[], Return>(
|
||||
target: (this: This, ...args: Args) => Promise<Return>,
|
||||
): (this: This, ...args: Args) => Promise<Return> {
|
||||
let promise: Promise<Return> | undefined;
|
||||
|
||||
async function replacementMethod(this: This, ...args: Args): Promise<Return> {
|
||||
if (promise) return promise;
|
||||
try {
|
||||
promise = target.call(this, ...args);
|
||||
await promise;
|
||||
return promise;
|
||||
} finally {
|
||||
promise = undefined;
|
||||
}
|
||||
}
|
||||
|
||||
return replacementMethod;
|
||||
}
|
||||
@ -5,7 +5,6 @@
|
||||
"declarationMap": true,
|
||||
"sourceMap": true,
|
||||
"noEmit": false,
|
||||
"emitDecoratorMetadata": true,
|
||||
"outDir": "./lib",
|
||||
"rootDir": "src"
|
||||
},
|
||||
|
||||
@ -1,7 +1,7 @@
|
||||
{
|
||||
"compilerOptions": {
|
||||
"target": "es2022",
|
||||
"experimentalDecorators": true,
|
||||
"experimentalDecorators": false,
|
||||
"esModuleInterop": true,
|
||||
"module": "commonjs",
|
||||
"moduleResolution": "node",
|
||||
|
||||
16
yarn.lock
16
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"
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user