Improve client metadata used for OIDC dynamic registration (#12257)

matrix-org · Feb 16, 2024 · cd8679c · cd8679c
1 parent e8ce9cb
commit cd8679c
Show file tree

Hide file tree

Showing 7 changed files with 80 additions and 41 deletions.
diff --git a/src/@types/common.ts b/src/@types/common.ts
@@ -16,6 +16,8 @@ limitations under the License.
 
 import { JSXElementConstructor } from "react";
 
+export type { NonEmptyArray } from "matrix-js-sdk/src/matrix";
+
 // Based on https://stackoverflow.com/a/53229857/3532235
 export type Without<T, U> = { [P in Exclude<keyof T, keyof U>]?: never };
 export type XOR<T, U> = T | U extends object ? (Without<T, U> & U) | (Without<U, T> & T) : T | U;
@@ -38,8 +40,6 @@ export type KeysStartingWith<Input extends object, Str extends string> = {
     [P in keyof Input]: P extends `${Str}${infer _X}` ? P : never; // we don't use _X
 }[keyof Input];
 
-export type NonEmptyArray<T> = [T, ...T[]];
-
 export type Defaultize<P, D> = P extends any
     ? string extends keyof P
         ? P

diff --git a/src/BasePlatform.ts b/src/BasePlatform.ts
@@ -17,7 +17,14 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { MatrixClient, MatrixEvent, Room, SSOAction, encodeUnpaddedBase64 } from "matrix-js-sdk/src/matrix";
+import {
+    MatrixClient,
+    MatrixEvent,
+    Room,
+    SSOAction,
+    encodeUnpaddedBase64,
+    OidcRegistrationClientMetadata,
+} from "matrix-js-sdk/src/matrix";
 import { logger } from "matrix-js-sdk/src/logger";
 
 import dis from "./dispatcher/dispatcher";
@@ -30,6 +37,7 @@ import { MatrixClientPeg } from "./MatrixClientPeg";
 import { idbLoad, idbSave, idbDelete } from "./utils/StorageManager";
 import { ViewRoomPayload } from "./dispatcher/payloads/ViewRoomPayload";
 import { IConfigOptions } from "./IConfigOptions";
+import SdkConfig from "./SdkConfig";
 
 export const SSO_HOMESERVER_URL_KEY = "mx_sso_hs_url";
 export const SSO_ID_SERVER_URL_KEY = "mx_sso_is_url";
@@ -426,7 +434,7 @@ export default abstract class BasePlatform {
     /**
      * Delete a previously stored pickle key from storage.
      * @param {string} userId the user ID for the user that the pickle key is for.
-     * @param {string} userId the device ID that the pickle key is for.
+     * @param {string} deviceId the device ID that the pickle key is for.
      */
     public async destroyPickleKey(userId: string, deviceId: string): Promise<void> {
         try {
@@ -443,4 +451,31 @@ export default abstract class BasePlatform {
         window.sessionStorage.clear();
         window.localStorage.clear();
     }
+
+    /**
+     * Base URL to use when generating external links for this client, for platforms e.g. Desktop this will be a different instance
+     */
+    public get baseUrl(): string {
+        return window.location.origin + window.location.pathname;
+    }
+
+    /**
+     * Metadata to use for dynamic OIDC client registrations
+     */
+    public async getOidcClientMetadata(): Promise<OidcRegistrationClientMetadata> {
+        const config = SdkConfig.get();
+        return {
+            clientName: config.brand,
+            clientUri: this.baseUrl,
+            redirectUris: [this.getSSOCallbackUrl().href],
+            logoUri: new URL("vector-icons/1024.png", this.baseUrl).href,
+            applicationType: "web",
+            // XXX: We break the spec by not consistently supplying these required fields
+            // contacts: [],
+            // @ts-ignore
+            tosUri: config.terms_and_conditions_links?.[0]?.url,
+            // @ts-ignore
+            policyUri: config.privacy_policy_url,
+        };
+    }
 }
diff --git a/src/Login.ts b/src/Login.ts
@@ -120,7 +120,6 @@ export default class Login {
             try {
                 const oidcFlow = await tryInitOidcNativeFlow(
                     this.delegatedAuthentication,
-                    SdkConfig.get().brand,
                     SdkConfig.get().oidc_static_clients,
                     isRegistration,
                 );
@@ -223,23 +222,21 @@ export interface OidcNativeFlow extends ILoginFlow {
  * results.
  *
  * @param delegatedAuthConfig  Auth config from ValidatedServerConfig
- * @param clientName Client name to register with the OP, eg 'Element', used during client registration with OP
  * @param staticOidcClientIds static client config from config.json, used during client registration with OP
  * @param isRegistration true when we are attempting registration
  * @returns Promise<OidcNativeFlow> when oidc native authentication flow is supported and correctly configured
  * @throws when client can't register with OP, or any unexpected error
  */
 const tryInitOidcNativeFlow = async (
     delegatedAuthConfig: OidcClientConfig,
-    brand: string,
-    oidcStaticClients?: IConfigOptions["oidc_static_clients"],
+    staticOidcClientIds?: IConfigOptions["oidc_static_clients"],
     isRegistration?: boolean,
 ): Promise<OidcNativeFlow> => {
     // if registration is not supported, bail before attempting to get the clientId
     if (isRegistration && !isUserRegistrationSupported(delegatedAuthConfig)) {
         throw new Error("Registration is not supported by OP");
     }
-    const clientId = await getOidcClientId(delegatedAuthConfig, brand, window.location.origin, oidcStaticClients);
+    const clientId = await getOidcClientId(delegatedAuthConfig, staticOidcClientIds);
 
     const flow = {
         type: "oidcNativeFlow",

diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts
@@ -19,6 +19,7 @@ import { registerOidcClient } from "matrix-js-sdk/src/oidc/register";
 
 import { IConfigOptions } from "../../IConfigOptions";
 import { ValidatedDelegatedAuthConfig } from "../ValidatedServerConfig";
+import PlatformPeg from "../../PlatformPeg";
 
 /**
  * Get the statically configured clientId for the issuer
@@ -40,22 +41,18 @@ const getStaticOidcClientId = (
  * Checks statically configured clientIds first
  * Then attempts dynamic registration with the OP
  * @param delegatedAuthConfig Auth config from ValidatedServerConfig
- * @param clientName Client name to register with the OP, eg 'Element'
- * @param baseUrl URL of the home page of the Client, eg 'https://app.element.io/'
  * @param staticOidcClients static client config from config.json
  * @returns Promise<string> resolves with clientId
  * @throws if no clientId is found
  */
 export const getOidcClientId = async (
     delegatedAuthConfig: ValidatedDelegatedAuthConfig,
-    clientName: string,
-    baseUrl: string,
     staticOidcClients?: IConfigOptions["oidc_static_clients"],
 ): Promise<string> => {
     const staticClientId = getStaticOidcClientId(delegatedAuthConfig.issuer, staticOidcClients);
     if (staticClientId) {
         logger.debug(`Using static clientId for issuer ${delegatedAuthConfig.issuer}`);
         return staticClientId;
     }
-    return await registerOidcClient(delegatedAuthConfig, clientName, baseUrl);
+    return await registerOidcClient(delegatedAuthConfig, await PlatformPeg.get()!.getOidcClientMetadata());
 };
diff --git a/test/components/structures/auth/Login-test.tsx b/test/components/structures/auth/Login-test.tsx
@@ -415,12 +415,7 @@ describe("Login", function () {
             // tried to register
             expect(fetchMock).toHaveBeenCalledWith(delegatedAuth.registrationEndpoint, expect.any(Object));
             // called with values from config
-            expect(registerClientUtils.getOidcClientId).toHaveBeenCalledWith(
-                delegatedAuth,
-                "test-brand",
-                "http://localhost",
-                oidcStaticClientsConfig,
-            );
+            expect(registerClientUtils.getOidcClientId).toHaveBeenCalledWith(delegatedAuth, oidcStaticClientsConfig);
         });
 
         it("should fallback to normal login when client registration fails", async () => {

diff --git a/test/test-utils/platform.ts b/test/test-utils/platform.ts
@@ -22,7 +22,7 @@ import PlatformPeg from "../../src/PlatformPeg";
 // doesn't implement abstract
 // @ts-ignore
 class MockPlatform extends BasePlatform {
-    constructor(platformMocks: Partial<Record<MethodLikeKeys<BasePlatform>, unknown>>) {
+    constructor(platformMocks: Partial<Record<keyof BasePlatform, unknown>>) {
         super();
         Object.assign(this, platformMocks);
     }

diff --git a/test/utils/oidc/registerClient-test.ts b/test/utils/oidc/registerClient-test.ts
@@ -19,6 +19,8 @@ import { OidcError } from "matrix-js-sdk/src/oidc/error";
 
 import { getOidcClientId } from "../../../src/utils/oidc/registerClient";
 import { ValidatedDelegatedAuthConfig } from "../../../src/utils/ValidatedServerConfig";
+import { mockPlatformPeg } from "../../test-utils";
+import PlatformPeg from "../../../src/PlatformPeg";
 
 describe("getOidcClientId()", () => {
     const issuer = "https://auth.com/";
@@ -41,10 +43,21 @@ describe("getOidcClientId()", () => {
     beforeEach(() => {
         fetchMockJest.mockClear();
         fetchMockJest.resetBehavior();
+        mockPlatformPeg();
+        Object.defineProperty(PlatformPeg.get(), "baseUrl", {
+            get(): string {
+                return baseUrl;
+            },
+        });
+        Object.defineProperty(PlatformPeg.get(), "getSSOCallbackUrl", {
+            value: () => ({
+                href: baseUrl,
+            }),
+        });
     });
 
     it("should return static clientId when configured", async () => {
-        expect(await getOidcClientId(delegatedAuthConfig, clientName, baseUrl, staticOidcClients)).toEqual("abc123");
+        expect(await getOidcClientId(delegatedAuthConfig, staticOidcClients)).toEqual("abc123");
         // didn't try to register
         expect(fetchMockJest).toHaveFetchedTimes(0);
     });
@@ -55,9 +68,9 @@ describe("getOidcClientId()", () => {
             issuer: "https://issuerWithoutStaticClientId.org/",
             registrationEndpoint: undefined,
         };
-        await expect(
-            getOidcClientId(authConfigWithoutRegistration, clientName, baseUrl, staticOidcClients),
-        ).rejects.toThrow(OidcError.DynamicRegistrationNotSupported);
+        await expect(getOidcClientId(authConfigWithoutRegistration, staticOidcClients)).rejects.toThrow(
+            OidcError.DynamicRegistrationNotSupported,
+        );
         // didn't try to register
         expect(fetchMockJest).toHaveFetchedTimes(0);
     });
@@ -67,7 +80,7 @@ describe("getOidcClientId()", () => {
             ...delegatedAuthConfig,
             registrationEndpoint: undefined,
         };
-        await expect(getOidcClientId(authConfigWithoutRegistration, clientName, baseUrl)).rejects.toThrow(
+        await expect(getOidcClientId(authConfigWithoutRegistration)).rejects.toThrow(
             OidcError.DynamicRegistrationNotSupported,
         );
         // didn't try to register
@@ -79,15 +92,20 @@ describe("getOidcClientId()", () => {
             status: 200,
             body: JSON.stringify({ client_id: dynamicClientId }),
         });
-        expect(await getOidcClientId(delegatedAuthConfig, clientName, baseUrl)).toEqual(dynamicClientId);
+        expect(await getOidcClientId(delegatedAuthConfig)).toEqual(dynamicClientId);
         // didn't try to register
-        expect(fetchMockJest).toHaveBeenCalledWith(registrationEndpoint, {
-            headers: {
-                "Accept": "application/json",
-                "Content-Type": "application/json",
-            },
-            method: "POST",
-            body: JSON.stringify({
+        expect(fetchMockJest).toHaveBeenCalledWith(
+            registrationEndpoint,
+            expect.objectContaining({
+                headers: {
+                    "Accept": "application/json",
+                    "Content-Type": "application/json",
+                },
+                method: "POST",
+            }),
+        );
+        expect(JSON.parse(fetchMockJest.mock.calls[0][1]!.body as string)).toEqual(
+            expect.objectContaining({
                 client_name: clientName,
                 client_uri: baseUrl,
                 response_types: ["code"],
@@ -96,17 +114,16 @@ describe("getOidcClientId()", () => {
                 id_token_signed_response_alg: "RS256",
                 token_endpoint_auth_method: "none",
                 application_type: "web",
+                logo_uri: `${baseUrl}/vector-icons/1024.png`,
             }),
-        });
+        );
     });
 
     it("should throw when registration request fails", async () => {
         fetchMockJest.post(registrationEndpoint, {
             status: 500,
         });
-        await expect(getOidcClientId(delegatedAuthConfig, clientName, baseUrl)).rejects.toThrow(
-            OidcError.DynamicRegistrationFailed,
-        );
+        await expect(getOidcClientId(delegatedAuthConfig)).rejects.toThrow(OidcError.DynamicRegistrationFailed);
     });
 
     it("should throw when registration response is invalid", async () => {
@@ -115,8 +132,6 @@ describe("getOidcClientId()", () => {
             // no clientId in response
             body: "{}",
         });
-        await expect(getOidcClientId(delegatedAuthConfig, clientName, baseUrl)).rejects.toThrow(
-            OidcError.DynamicRegistrationInvalid,
-        );
+        await expect(getOidcClientId(delegatedAuthConfig)).rejects.toThrow(OidcError.DynamicRegistrationInvalid);
     });
 });