Skip to content

Commit 102a1dd

Browse files
t3chguyt3chguy
authored
Fix token expiry racing with login causing wrong error to be shown (#29566)
* Fix token expiry racing with login causing wrong error to be shown Signed-off-by: Michael Telatynski <[email protected]> * Add tests Signed-off-by: Michael Telatynski <[email protected]> * yay jest Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> --------- Signed-off-by: Michael Telatynski <[email protected]>
1 parent 99ea51c commit 102a1dd

File tree

5 files changed

+91
-14
lines changed

5 files changed

+91
-14
lines changed

‎playwright/e2e/oidc/oidc-native.spec.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,4 +73,33 @@ test.describe("OIDC Native", { tag: ["@no-firefox", "@no-webkit"] }, () => {
7373
await revokeAccessTokenPromise;
7474
await revokeRefreshTokenPromise;
7575
});
76+
77+
test(
78+
"it should log out the user & wipe data when logging out via MAS",
79+
{ tag: "@screenshot" },
80+
async ({ mas, page, mailpitClient }, testInfo) => {
81+
// We use this over the `user` fixture to ensure we get an OIDC session rather than a compatibility one
82+
await page.goto("/#/login");
83+
await page.getByRole("button", { name: "Continue" }).click();
84+
85+
const userId = `alice_${testInfo.testId}`;
86+
await registerAccountMas(page, mailpitClient, userId, "[email protected]", "Pa$sW0rD!");
87+
88+
await expect(page.getByText("Welcome")).toBeVisible();
89+
await page.goto("about:blank");
90+
91+
// @ts-expect-error
92+
const result = await mas.manage("kill-sessions", userId);
93+
expect(result.output).toContain("Ended 1 active OAuth 2.0 session");
94+
95+
await page.goto("http://localhost:8080");
96+
await expect(
97+
page.getByText("For security, this session has been signed out. Please sign in again."),
98+
).toBeVisible();
99+
await expect(page).toMatchScreenshot("token-expired.png", { includeDialogBackground: true });
100+
101+
const localStorageKeys = await page.evaluate(() => Object.keys(localStorage));
102+
expect(localStorageKeys).toHaveLength(0);
103+
},
104+
);
76105
});

‎playwright/snapshots/oidc/oidc-native.spec.ts/token-expired-linux.png

957 KB
Loading

‎src/Lifecycle.ts

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ interface ILoadSessionOpts {
149149
ignoreGuest?: boolean;
150150
defaultDeviceDisplayName?: string;
151151
fragmentQueryParams?: QueryDict;
152+
abortSignal?: AbortSignal;
152153
}
153154

154155
/**
@@ -196,7 +197,7 @@ export async function loadSession(opts: ILoadSessionOpts = {}): Promise<boolean>
196197

197198
if (enableGuest && guestHsUrl && fragmentQueryParams.guest_user_id && fragmentQueryParams.guest_access_token) {
198199
logger.log("Using guest access credentials");
199-
return doSetLoggedIn(
200+
await doSetLoggedIn(
200201
{
201202
userId: fragmentQueryParams.guest_user_id as string,
202203
accessToken: fragmentQueryParams.guest_access_token as string,
@@ -206,7 +207,8 @@ export async function loadSession(opts: ILoadSessionOpts = {}): Promise<boolean>
206207
},
207208
true,
208209
false,
209-
).then(() => true);
210+
);
211+
return true;
210212
}
211213
const success = await restoreSessionFromStorage({
212214
ignoreGuest: Boolean(opts.ignoreGuest),
@@ -225,6 +227,11 @@ export async function loadSession(opts: ILoadSessionOpts = {}): Promise<boolean>
225227
// fall back to welcome screen
226228
return false;
227229
} catch (e) {
230+
// We may be aborted e.g. because our token expired, so don't show an error here
231+
if (opts.abortSignal?.aborted) {
232+
return false;
233+
}
234+
228235
if (e instanceof AbortLoginAndRebuildStorage) {
229236
// If we're aborting login because of a storage inconsistency, we don't
230237
// need to show the general failure dialog. Instead, just go back to welcome.
@@ -236,7 +243,7 @@ export async function loadSession(opts: ILoadSessionOpts = {}): Promise<boolean>
236243
return false;
237244
}
238245

239-
return handleLoadSessionFailure(e);
246+
return handleLoadSessionFailure(e, opts);
240247
}
241248
}
242249

@@ -656,7 +663,7 @@ export async function restoreSessionFromStorage(opts?: { ignoreGuest?: boolean }
656663
}
657664
}
658665

659-
async function handleLoadSessionFailure(e: unknown): Promise<boolean> {
666+
async function handleLoadSessionFailure(e: unknown, loadSessionOpts?: ILoadSessionOpts): Promise<boolean> {
660667
logger.error("Unable to load session", e);
661668

662669
const modal = Modal.createDialog(SessionRestoreErrorDialog, {
@@ -671,7 +678,7 @@ async function handleLoadSessionFailure(e: unknown): Promise<boolean> {
671678
}
672679

673680
// try, try again
674-
return loadSession();
681+
return loadSession(loadSessionOpts);
675682
}
676683

677684
/**

‎src/components/structures/MatrixChat.tsx

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,7 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
235235
private themeWatcher?: ThemeWatcher;
236236
private fontWatcher?: FontWatcher;
237237
private readonly stores: SdkContextClass;
238+
private loadSessionAbortController = new AbortController();
238239

239240
public constructor(props: IProps) {
240241
super(props);
@@ -327,7 +328,7 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
327328
// When the session loads it'll be detected as soft logged out and a dispatch
328329
// will be sent out to say that, triggering this MatrixChat to show the soft
329330
// logout page.
330-
Lifecycle.loadSession();
331+
Lifecycle.loadSession({ abortSignal: this.loadSessionAbortController.signal });
331332
return;
332333
}
333334

@@ -552,6 +553,7 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
552553
guestHsUrl: this.getServerProperties().serverConfig.hsUrl,
553554
guestIsUrl: this.getServerProperties().serverConfig.isUrl,
554555
defaultDeviceDisplayName: this.props.defaultDeviceDisplayName,
556+
abortSignal: this.loadSessionAbortController.signal,
555557
});
556558
})
557559
.then((loadedSession) => {
@@ -1565,26 +1567,33 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
15651567
dis.fire(Action.FocusSendMessageComposer);
15661568
});
15671569

1568-
cli.on(HttpApiEvent.SessionLoggedOut, function (errObj) {
1570+
cli.on(HttpApiEvent.SessionLoggedOut, (errObj) => {
1571+
this.loadSessionAbortController.abort(errObj);
1572+
this.loadSessionAbortController = new AbortController();
1573+
15691574
if (Lifecycle.isLoggingOut()) return;
15701575

15711576
// A modal might have been open when we were logged out by the server
15721577
Modal.forceCloseAllModals();
15731578

1574-
if (errObj.httpStatus === 401 && errObj.data && errObj.data["soft_logout"]) {
1579+
if (errObj.httpStatus === 401 && errObj.data?.["soft_logout"]) {
15751580
logger.warn("Soft logout issued by server - avoiding data deletion");
15761581
Lifecycle.softLogout();
15771582
return;
15781583
}
15791584

1585+
dis.dispatch(
1586+
{
1587+
action: "logout",
1588+
},
1589+
true,
1590+
);
1591+
1592+
// The above dispatch closes all modals, so open the modal after calling it synchronously
15801593
Modal.createDialog(ErrorDialog, {
15811594
title: _t("auth|session_logged_out_title"),
15821595
description: _t("auth|session_logged_out_description"),
15831596
});
1584-
1585-
dis.dispatch({
1586-
action: "logout",
1587-
});
15881597
});
15891598
cli.on(HttpApiEvent.NoConsent, function (message, consentUri) {
15901599
Modal.createDialog(

‎test/unit-tests/Lifecycle-test.ts

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import { mocked, type MockedObject } from "jest-mock";
1515
import fetchMock from "fetch-mock-jest";
1616

1717
import StorageEvictedDialog from "../../src/components/views/dialogs/StorageEvictedDialog";
18-
import { logout, restoreSessionFromStorage, setLoggedIn } from "../../src/Lifecycle";
18+
import * as Lifecycle from "../../src/Lifecycle";
1919
import { MatrixClientPeg } from "../../src/MatrixClientPeg";
2020
import Modal from "../../src/Modal";
2121
import * as StorageAccess from "../../src/utils/StorageAccess";
@@ -28,19 +28,25 @@ import { Action } from "../../src/dispatcher/actions";
2828
import PlatformPeg from "../../src/PlatformPeg";
2929
import { persistAccessTokenInStorage, persistRefreshTokenInStorage } from "../../src/utils/tokens/tokens";
3030
import { encryptPickleKey } from "../../src/utils/tokens/pickling";
31+
import * as StorageManager from "../../src/utils/StorageManager.ts";
32+
import type BasePlatform from "../../src/BasePlatform.ts";
33+
34+
const { logout, restoreSessionFromStorage, setLoggedIn } = Lifecycle;
3135

3236
const webCrypto = new Crypto();
3337

3438
const windowCrypto = window.crypto;
3539

3640
describe("Lifecycle", () => {
37-
const mockPlatform = mockPlatformPeg();
41+
let mockPlatform: MockedObject<BasePlatform>;
3842

3943
const realLocalStorage = global.localStorage;
4044

4145
let mockClient!: MockedObject<MatrixJs.MatrixClient>;
4246

4347
beforeEach(() => {
48+
jest.restoreAllMocks();
49+
mockPlatform = mockPlatformPeg();
4450
mockClient = getMockClientWithEventEmitter({
4551
...mockClientMethodsUser(),
4652
stopClient: jest.fn(),
@@ -182,6 +188,32 @@ describe("Lifecycle", () => {
182188
mac: expect.any(String),
183189
};
184190

191+
describe("loadSession", () => {
192+
beforeEach(() => {
193+
// stub this out
194+
jest.spyOn(Modal, "createDialog").mockReturnValue(
195+
// @ts-ignore allow bad mock
196+
{ finished: Promise.resolve([true]) },
197+
);
198+
});
199+
200+
it("should not show any error dialog when checkConsistency throws but abortSignal has triggered", async () => {
201+
jest.spyOn(StorageManager, "checkConsistency").mockRejectedValue(new Error("test error"));
202+
203+
const abortController = new AbortController();
204+
const prom = Lifecycle.loadSession({
205+
enableGuest: true,
206+
guestHsUrl: "https://guest.server",
207+
fragmentQueryParams: { guest_user_id: "a", guest_access_token: "b" },
208+
abortSignal: abortController.signal,
209+
});
210+
abortController.abort();
211+
await expect(prom).resolves.toBeFalsy();
212+
213+
expect(Modal.createDialog).not.toHaveBeenCalled();
214+
});
215+
});
216+
185217
describe("restoreSessionFromStorage()", () => {
186218
beforeEach(() => {
187219
initLocalStorageMock();

0 commit comments

Comments
 (0)