Tidy up token refresh code (#31519)

* Tidy up token refresh code

This was a bit odd where the function to create a refresher sometimes
created a refresher and sometimes just returned null, including if the
init failed, in which case you would just end up with no token refresher.

Pairs with https://github.com/matrix-org/matrix-js-sdk/pull/5106 but
doesn't depend on either way.

* Remove deviceId property in favour of superclass one

* Fix tests

* Fix argument order in super call

redirect URI & device ID were swapped. It appears that gthe OIDS client
only actually sends the redirect URI when refreshing a token, so we will
have been sending a device ID for that when refreshing. I think this is safe
to fix since this is only when refreshing so it already would not have
matched what was passed at login time.

* Pass client ID into createOidcTokenRefresher

src/Lifecycle.ts

@@ -751,39 +751,38 @@ export async function hydrateSession(credentials: IMatrixClientCreds): Promise<M
  * When we have a authenticated via OIDC-native flow and have a refresh token
  * try to create a token refresher.
  * @param credentials from current session
- * @returns Promise that resolves to a TokenRefresher, or undefined
+ * @param clientId OIDC client ID
+ * @throws If credentials.refreshToken or credentials.deviceId is falsy, or if no token issuer is stored
+ * @returns Promise that resolves to a TokenRefresher
  */
-async function createOidcTokenRefresher(credentials: IMatrixClientCreds): Promise<OidcTokenRefresher | undefined> {
+async function createOidcTokenRefresher(
+    credentials: IMatrixClientCreds,
+    clientId: string,
+): Promise<OidcTokenRefresher> {
     if (!credentials.refreshToken) {
-        return;
+        throw new Error("A refresh token must be supplied in order to create an OIDC token refresher.");
     }
     // stored token issuer indicates we authenticated via OIDC-native flow
     const tokenIssuer = getStoredOidcTokenIssuer();
     if (!tokenIssuer) {
-        return;
+        throw new Error("Cannot create an OIDC token refresher as no stored OIDC token issuer was found.");
     }
-    try {
-        const clientId = getStoredOidcClientId();
-        const idTokenClaims = getStoredOidcIdTokenClaims();
-        const redirectUri = PlatformPeg.get()!.getOidcCallbackUrl().href;
-        const deviceId = credentials.deviceId;
-        if (!deviceId) {
-            throw new Error("Expected deviceId in user credentials.");
-        }
-        const tokenRefresher = new TokenRefresher(
-            tokenIssuer,
-            clientId,
-            redirectUri,
-            deviceId,
-            idTokenClaims!,
-            credentials.userId,
-        );
-        // wait for the OIDC client to initialise
-        await tokenRefresher.oidcClientReady;
-        return tokenRefresher;
-    } catch (error) {
-        logger.error("Failed to initialise OIDC token refresher", error);
+
+    const idTokenClaims = getStoredOidcIdTokenClaims();
+    const redirectUri = PlatformPeg.get()!.getOidcCallbackUrl().href;
+    const deviceId = credentials.deviceId;
+    if (!deviceId) {
+        throw new Error("Expected deviceId in user credentials.");
     }
+    const tokenRefresher = new TokenRefresher(
+        tokenIssuer,
+        clientId,
+        redirectUri,
+        deviceId,
+        idTokenClaims!,
+        credentials.userId,
+    );
+    return tokenRefresher;
 }
 
 /**
@@ -835,7 +834,17 @@ async function doSetLoggedIn(
         await abortLogin();
     }
 
-    const tokenRefresher = await createOidcTokenRefresher(credentials);
+    let storedClientid;
+    try {
+        storedClientid = getStoredOidcClientId();
+    } catch {}
+
+    let tokenRefresher;
+    if (credentials.refreshToken && storedClientid) {
+        tokenRefresher = await createOidcTokenRefresher(credentials, storedClientid);
+    } else {
+        logger.debug("No refresh token was supplied: access token will not be refreshed");
+    }
 
     // check the session lock just before creating the new client
     checkSessionLock();

src/utils/oidc/TokenRefresher.ts

@@ -17,8 +17,6 @@ import { persistAccessTokenInStorage, persistRefreshTokenInStorage } from "../to
  * Stores tokens in the same way as login flow in Lifecycle.
  */
 export class TokenRefresher extends OidcTokenRefresher {
-    private readonly deviceId!: string;
-
     public constructor(
         issuer: string,
         clientId: string,
@@ -27,8 +25,7 @@ export class TokenRefresher extends OidcTokenRefresher {
         idTokenClaims: IdTokenClaims,
         private readonly userId: string,
     ) {
-        super(issuer, clientId, deviceId, redirectUri, idTokenClaims);
-        this.deviceId = deviceId;
+        super(issuer, clientId, redirectUri, deviceId, idTokenClaims);
     }
 
     public async persistTokens({ accessToken, refreshToken }: AccessTokens): Promise<void> {

test/unit-tests/Lifecycle-test.ts

@@ -167,6 +167,8 @@ describe("Lifecycle", () => {
         mx_is_url: identityServerUrl,
         mx_user_id: userId,
         mx_device_id: deviceId,
+        mx_oidc_token_issuer: "test-issuer.dummy",
+        mx_oidc_client_id: "test-client-id",
     };
     const idbStorageSession = {
         account: {
@@ -374,7 +376,7 @@ describe("Lifecycle", () => {
                                 guest: false,
                                 pickleKey: undefined,
                             },
-                            undefined,
+                            expect.any(Function),
                         );
                     });
                 });
@@ -492,7 +494,7 @@ describe("Lifecycle", () => {
                                 guest: false,
                                 pickleKey: pickleKey,
                             },
-                            undefined,
+                            expect.any(Function),
                         );
                     });
                 });
@@ -657,6 +659,11 @@ describe("Lifecycle", () => {
             });
 
             it("should persist a refreshToken when present", async () => {
+                initLocalStorageMock({
+                    mx_oidc_token_issuer: "test-issuer.dummy",
+                    mx_oidc_client_id: "test-client-id",
+                });
+
                 await setLoggedIn({
                     ...credentials,
                     refreshToken,
@@ -838,12 +845,19 @@ describe("Lifecycle", () => {
             });
 
             it("should not try to create a token refresher without a deviceId", async () => {
-                await setLoggedIn({
-                    ...credentials,
-                    refreshToken,
-                    deviceId: undefined,
+                initLocalStorageMock({
+                    mx_oidc_token_issuer: "test-issuer.dummy",
+                    mx_oidc_client_id: "test-client-id",
                 });
 
+                await expect(
+                    setLoggedIn({
+                        ...credentials,
+                        refreshToken,
+                        deviceId: undefined,
+                    }),
+                ).rejects.toThrow("Expected deviceId in user credentials.");
+
                 // didn't try to initialise token refresher
                 expect(fetchMock).not.toHaveFetched(`${delegatedAuthConfig.issuer}.well-known/openid-configuration`);
             });
@@ -855,10 +869,12 @@ describe("Lifecycle", () => {
                     undefined,
                     idToken,
                 );
-                await setLoggedIn({
-                    ...credentials,
-                    refreshToken,
-                });
+                await expect(
+                    setLoggedIn({
+                        ...credentials,
+                        refreshToken,
+                    }),
+                ).rejects.toThrow("Cannot create an OIDC token refresher as no stored OIDC token issuer was found.");
 
                 // didn't try to initialise token refresher
                 expect(fetchMock).not.toHaveFetched(`${delegatedAuthConfig.issuer}.well-known/openid-configuration`);