Fix OAuth logout bug if started offline

apps/web/src/Lifecycle.ts

@@ -10,7 +10,7 @@ Please see LICENSE files in the repository root for full details.
 */
 
 import { type ReactNode } from "react";
-import { MatrixClient, OAuth2, createClient, SSOAction, decodeBase64 } from "matrix-js-sdk/src/matrix";
+import { type MatrixClient, createClient, SSOAction, decodeBase64 } from "matrix-js-sdk/src/matrix";
 import { type AESEncryptedSecretStoragePayload } from "matrix-js-sdk/src/types";
 import { logger } from "matrix-js-sdk/src/logger";
 
@@ -52,7 +52,7 @@ import { SDKContextClass } from "./contexts/SDKContextClass";
 import { messageForLoginError } from "./utils/ErrorUtils";
 import { completeOAuthLogin, type CompleteOAuthLoginResponse } from "./utils/oauth/authorize";
 import { getOAuthErrorMessage } from "./utils/oauth/error";
-import { getOAuthParams, getStoredOAuthClientId, persistOAuthClientId } from "./utils/oauth/persistOAuthSettings";
+import { persistOAuthClientId } from "./utils/oauth/persistOAuthSettings";
 import {
     ACCESS_TOKEN_IV,
     ACCESS_TOKEN_STORAGE_KEY,
@@ -66,7 +66,6 @@ import {
 import { checkBrowserSupport } from "./SupportedBrowser";
 import { type URLParams } from "./vector/url_utils.ts";
 import { type OnLoggedInPayload } from "./dispatcher/payloads/OnLoggedInPayload.ts";
-import { filterBoolean } from "./utils/arrays.ts";
 import { clearUploadedMediaCache } from "./utils/UploadedMediaCache";
 import { CallStatusListener } from "./CallStatusListener.ts";
 import { CallStore } from "./stores/CallStore.ts";
@@ -811,14 +810,9 @@ async function doSetLoggedIn(
         await abortLogin();
     }
 
-    let auth: OAuth2 | undefined;
-    try {
-        auth = await hydrateAuth(credentials);
-    } catch {}
-
     // check the session lock just before creating the new client
     checkSessionLock();
-    MatrixClientPeg.set(createClientWithCreds(credentials, auth));
+    MatrixClientPeg.set(createClientWithCreds(credentials));
     const client = MatrixClientPeg.safeGet();
 
     setSentryUser(credentials.userId);
@@ -921,49 +915,13 @@ async function persistCredentials(credentials: IMatrixClientCreds): Promise<void
 
 let _isLoggingOut = false;
 
-/**
- * Logs out the current session.
- * When user has authenticated using OAuth2 native flow revoke tokens with OAuth2 provider.
- * Otherwise, call /logout on the homeserver.
- * @param client
- * @param oauth
- */
-async function doLogout(client: MatrixClient, oauth: OAuth2 | null): Promise<void> {
-    if (oauth) {
-        const accessToken = client.getAccessToken();
-        const refreshToken = client.getRefreshToken();
-
-        await Promise.all(
-            filterBoolean([
-                accessToken ? oauth.revokeToken(accessToken, "access_token") : null,
-                refreshToken ? oauth.revokeToken(refreshToken, "refresh_token") : null,
-            ]),
-        );
-
-        client.stopClient();
-        client.http.abort();
-    } else {
-        await client.logout(true);
-    }
-}
-
 /**
  * Logs the current session out and transitions to the logged-out state
  */
 export async function logout(): Promise<void> {
     const client = MatrixClientPeg.get();
     if (!client) return;
 
-    let oauth: OAuth2 | undefined;
-    try {
-        oauth = await hydrateAuth({
-            homeserverUrl: client.getHomeserverUrl(),
-            deviceId: client.getDeviceId()!,
-        });
-    } catch {
-        // This is fine
-    }
-
     PosthogAnalytics.instance.logout();
 
     if (client.isGuest()) {
@@ -977,17 +935,12 @@ export async function logout(): Promise<void> {
     _isLoggingOut = true;
     void PlatformPeg.get()?.destroyPickleKey(client.getSafeUserId(), client.getDeviceId() ?? "");
 
-    doLogout(client, oauth ?? null).then(onLoggedOut, (err) => {
-        // Just throwing an error here is going to be very unhelpful
-        // if you're trying to log out because your server's down and
-        // you want to log into a different server, so just forget the
-        // access token. It's annoying that this will leave the access
-        // token still valid, but we should fix this by having access
-        // tokens expire (and if you really think you've been compromised,
-        // change your password).
+    try {
+        await client.logout(true);
+    } catch (err) {
         logger.warn("Failed to call logout API: token will not be invalidated", err);
-        return onLoggedOut();
-    });
+    }
+    await onLoggedOut();
 }
 
 export function softLogout(): void {
@@ -1101,7 +1054,7 @@ async function startMatrixClient(
 
 /*
  * Stops a running client and all related services, and clears persistent
- * storage. Used after a session has been logged out.
+ * storage. Used after a session has been logged out (or at least attempted to be logged out).
  */
 export async function onLoggedOut(): Promise<void> {
     // Ensure that we dispatch a view change **before** stopping the client,
@@ -1228,18 +1181,3 @@ window.mxLoginWithAccessToken = async (hsUrl: string, accessToken: string): Prom
         false,
     );
 };
-
-/**
- * Instantiate an OAuth2 instance from storage
- * Returned promise will reject if the session or the server are not OAuth2-native.
- */
-export async function hydrateAuth(
-    credentials: Pick<IMatrixClientCreds, "homeserverUrl" | "deviceId">,
-): Promise<OAuth2> {
-    const storedClientId = getStoredOAuthClientId();
-
-    const tempClient = new MatrixClient({ baseUrl: credentials.homeserverUrl });
-    const authMetadata = await tempClient.getAuthMetadata();
-
-    return new OAuth2(authMetadata, { ...getOAuthParams(storedClientId), deviceId: credentials.deviceId });
-}

apps/web/src/components/structures/MatrixChat.test.tsx

@@ -554,6 +554,8 @@ describe("<MatrixChat />", () => {
             vi.spyOn(logger, "error").mockClear();
             vi.spyOn(logger, "log").mockClear();
 
+            mockPlatformPeg();
+
             loginClient.whoami.mockResolvedValue({
                 user_id: userId,
                 device_id: deviceId,
@@ -568,7 +570,6 @@ describe("<MatrixChat />", () => {
                     clientId,
                     codeVerifier: "123456",
                     deviceId,
-                    redirectUri: "https://cb",
                 },
             });
         });
@@ -595,21 +596,21 @@ describe("<MatrixChat />", () => {
         it("should make correct request to complete authorization", async () => {
             getComponent({ urlParams });
 
-            await flushPromises();
-
-            expect(OAuth2.prototype.completeAuthorizationCodeGrant).toHaveBeenCalledWith(code);
+            await waitFor(() => {
+                expect(OAuth2.prototype.completeAuthorizationCodeGrant).toHaveBeenCalledWith(code, expect.anything());
+            });
         });
 
         it("should look up userId using access token", async () => {
             getComponent({ urlParams });
 
-            await flushPromises();
-
-            // check we used a client with the correct accesstoken
-            expect(MatrixJs.createClient).toHaveBeenCalledWith({
-                baseUrl: homeserverUrl,
-                accessToken,
-                idBaseUrl: identityServerUrl,
+            await waitFor(() => {
+                // check we used a client with the correct accesstoken
+                expect(MatrixJs.createClient).toHaveBeenCalledWith({
+                    baseUrl: homeserverUrl,
+                    accessToken,
+                    idBaseUrl: identityServerUrl,
+                });
             });
             expect(loginClient.whoami).toHaveBeenCalled();
         });
@@ -618,12 +619,12 @@ describe("<MatrixChat />", () => {
             loginClient.whoami.mockRejectedValue(new Error("oups"));
             getComponent({ urlParams });
 
-            await flushPromises();
-
-            expect(logger.error).toHaveBeenCalledWith(
-                "Failed to login via OAuth",
-                new Error("Failed to retrieve userId using accessToken"),
-            );
+            await waitFor(() => {
+                expect(logger.error).toHaveBeenCalledWith(
+                    "Failed to login via OAuth",
+                    new Error("Failed to retrieve userId using accessToken"),
+                );
+            });
             await expectOAuthError();
         });
 

apps/web/src/utils/createMatrixClient.test.ts

@@ -5,10 +5,14 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com
 Please see LICENSE files in the repository root for full details.
 */
 
+// @vitest-environment happy-dom
+
 import { vi, describe, beforeEach, it, expect } from "vitest";
 import { type MatrixClient, RoomNameType } from "matrix-js-sdk/src/matrix";
+import { mockPlatformPeg } from "test-utils";
 
 import { createClientWithCreds } from "./createMatrixClient";
+import PlatformPeg from "../PlatformPeg";
 
 describe("createMatrixClient", () => {
     let client: MatrixClient;
@@ -150,4 +154,64 @@ describe("createMatrixClient", () => {
             });
         });
     });
+
+    describe("oauth2ClientConfig", () => {
+        const stubLocalStorage = (clientId: string | null): void => {
+            vi.stubGlobal("localStorage", {
+                getItem: vi.fn().mockImplementation((key: string) => (key === "mx_oidc_client_id" ? clientId : null)),
+                setItem: vi.fn(),
+                removeItem: vi.fn(),
+            });
+        };
+
+        beforeEach(() => {
+            mockPlatformPeg();
+            Object.defineProperty(PlatformPeg.get(), "getOAuthCallbackUrl", {
+                value: () => new URL("https://test.dummy/oauth/callback"),
+            });
+        });
+
+        it("should not be set when there is no refresh token", () => {
+            stubLocalStorage("test-client-id");
+
+            client = createClientWithCreds({
+                homeserverUrl: "https://test.dummy",
+                userId: "@user:test.dummy",
+                accessToken: "access_token",
+            });
+
+            expect(client.http.opts.oauth2ClientConfig).toBeUndefined();
+        });
+
+        it("should not be set when there is a refresh token but no stored OAuth2 client ID", () => {
+            stubLocalStorage(null);
+
+            client = createClientWithCreds({
+                homeserverUrl: "https://test.dummy",
+                userId: "@user:test.dummy",
+                accessToken: "access_token",
+                refreshToken: "refresh_token",
+            });
+
+            expect(client.http.opts.oauth2ClientConfig).toBeUndefined();
+        });
+
+        it("should be set from the stored OAuth2 client ID when there is a refresh token", () => {
+            stubLocalStorage("test-client-id");
+
+            client = createClientWithCreds({
+                homeserverUrl: "https://test.dummy",
+                userId: "@user:test.dummy",
+                accessToken: "access_token",
+                refreshToken: "refresh_token",
+            });
+
+            expect(client.http.opts.oauth2ClientConfig).toEqual(
+                expect.objectContaining({
+                    clientId: "test-client-id",
+                    getAuthMetadata: expect.any(Function),
+                }),
+            );
+        });
+    });
 });

apps/web/src/utils/createMatrixClient.ts

@@ -19,8 +19,6 @@ import {
     type RoomNameState,
     EventTimelineSet,
     EventTimeline,
-    type OAuth2,
-    TokenRefresher,
 } from "matrix-js-sdk/src/matrix";
 import { VerificationMethod } from "matrix-js-sdk/src/types";
 import { logger } from "matrix-js-sdk/src/logger";
@@ -32,6 +30,7 @@ import IdentityAuthClient from "../IdentityAuthClient";
 import { _t } from "../languageHandler";
 import { formatList } from "./FormattingUtils";
 import { persistTokens } from "./tokens/tokens.ts";
+import { getStoredOAuthClientId } from "./oauth/persistOAuthSettings";
 
 const localStorage = window.localStorage;
 
@@ -119,25 +118,26 @@ function roomNameGenerator(_: string, state: RoomNameState): string | null {
  * Create a new matrix client from credentials with all the options needed.
  *
  * @param creds The credentials to create the client with
- * @param oauth The OAuth2 instance for OAuth2-native sessions
  *
  * @returns {MatrixClient} the newly-created MatrixClient
  */
-export function createClientWithCreds(creds: IMatrixClientCreds, oauth?: OAuth2): MatrixClient {
-    let tokenRefreshFunction: ICreateClientOpts["tokenRefreshFunction"];
-    if (creds.refreshToken && oauth) {
-        const tokenRefresher = new TokenRefresher(oauth, persistTokens.bind(null, creds.pickleKey));
-        tokenRefreshFunction = tokenRefresher?.tokenRefreshFunction;
-    } else {
-        logger.debug("No refresh token was supplied: access token will not be refreshed");
+export function createClientWithCreds(creds: IMatrixClientCreds): MatrixClient {
+    let oauthClientId: string | undefined;
+    if (creds.refreshToken) {
+        try {
+            oauthClientId = getStoredOAuthClientId();
+        } catch (e) {
+            logger.warn("Have a refresh token but no stored OAuth2 client ID: tokens will not be refreshed", e);
+        }
     }
 
     const opts: ICreateClientOpts = {
         baseUrl: creds.homeserverUrl,
         idBaseUrl: creds.identityServerUrl,
         accessToken: creds.accessToken,
         refreshToken: creds.refreshToken,
-        tokenRefreshFunction,
+        onTokenRefresh: persistTokens.bind(null, creds.pickleKey),
+        oauthClientId,
         userId: creds.userId,
         deviceId: creds.deviceId,
         pickleKey: creds.pickleKey,

apps/web/src/utils/oauth/authorize.test.ts

@@ -40,6 +40,7 @@ describe("OAuth2 authorization", () => {
         window.location = {
             href: baseUrl,
             origin: baseUrl,
+            pathname: "",
         };
 
         mockPlatformPeg();
@@ -121,7 +122,6 @@ describe("OAuth2 authorization", () => {
                     codeVerifier: "123456",
                     clientId,
                     deviceId: "DEADB33F",
-                    redirectUri: "https://test.com/callback",
                 },
             });
         });
@@ -135,7 +135,10 @@ describe("OAuth2 authorization", () => {
         it("should make request complete authorization code grant", async () => {
             await completeOAuthLogin(params);
 
-            expect(OAuth2.prototype.completeAuthorizationCodeGrant).toHaveBeenCalledWith(code);
+            expect(OAuth2.prototype.completeAuthorizationCodeGrant).toHaveBeenCalledWith(
+                code,
+                "https://test.com/?no_universal_links=true",
+            );
         });
 
         it("should return accessToken, configured homeserver and identityServer", async () => {