From 99a388cc78b941514ef6305871d39f3b41b166ea Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 15 Feb 2024 17:01:47 +0000 Subject: [PATCH 1/5] Reuse exported common type Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/@types/common.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/@types/common.ts b/src/@types/common.ts index 4169429bbe0..4141418ac4e 100644 --- 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 = { [P in Exclude]?: never }; export type XOR = T | U extends object ? (Without & U) | (Without & T) : T | U; @@ -38,8 +40,6 @@ export type KeysStartingWith = { [P in keyof Input]: P extends `${Str}${infer _X}` ? P : never; // we don't use _X }[keyof Input]; -export type NonEmptyArray = [T, ...T[]]; - export type Defaultize = P extends any ? string extends keyof P ? P From b04f49c5154942d93626d1374ddb4cecfc1f091d Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 15 Feb 2024 17:03:19 +0000 Subject: [PATCH 2/5] Improve client metadata used for OIDC dynamic registration Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/BasePlatform.ts | 38 +++++++++++++++++++++++++++++++- src/Login.ts | 7 ++---- src/utils/oidc/registerClient.ts | 7 ++---- 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/src/BasePlatform.ts b/src/BasePlatform.ts index 9de3882124f..f998f0c6634 100644 --- 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"; @@ -443,4 +451,32 @@ 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 { + const baseUrl = window.location.origin; + const config = SdkConfig.get(); + return { + clientName: config.brand, + clientUri: 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 index 447542a64d8..4f198fc634e 100644 --- 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,7 +222,6 @@ 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 when oidc native authentication flow is supported and correctly configured @@ -231,15 +229,14 @@ export interface OidcNativeFlow extends ILoginFlow { */ const tryInitOidcNativeFlow = async ( delegatedAuthConfig: OidcClientConfig, - brand: string, - oidcStaticClients?: IConfigOptions["oidc_static_clients"], + staticOidcClientIds?: IConfigOptions["oidc_static_clients"], isRegistration?: boolean, ): Promise => { // 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 index 309709e18f1..9f112293b69 100644 --- 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,16 +41,12 @@ 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 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 => { const staticClientId = getStaticOidcClientId(delegatedAuthConfig.issuer, staticOidcClients); @@ -57,5 +54,5 @@ export const getOidcClientId = async ( 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()); }; From ca6a1128058f730bdb25123445c2341cc3b0e4f4 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 16 Feb 2024 12:56:13 +0000 Subject: [PATCH 3/5] Fix typo Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/BasePlatform.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/BasePlatform.ts b/src/BasePlatform.ts index f998f0c6634..e8934f63f85 100644 --- a/src/BasePlatform.ts +++ b/src/BasePlatform.ts @@ -434,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 { try { From 507447c5aabf84fbb8373db80831435502d22f92 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 16 Feb 2024 13:00:52 +0000 Subject: [PATCH 4/5] Fix test Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/BasePlatform.ts | 3 +- test/test-utils/platform.ts | 2 +- test/utils/oidc/registerClient-test.ts | 55 ++++++++++++++++---------- 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/src/BasePlatform.ts b/src/BasePlatform.ts index e8934f63f85..b343fcab495 100644 --- a/src/BasePlatform.ts +++ b/src/BasePlatform.ts @@ -463,11 +463,10 @@ export default abstract class BasePlatform { * Metadata to use for dynamic OIDC client registrations */ public async getOidcClientMetadata(): Promise { - const baseUrl = window.location.origin; const config = SdkConfig.get(); return { clientName: config.brand, - clientUri: baseUrl, + clientUri: this.baseUrl, redirectUris: [this.getSSOCallbackUrl().href], logoUri: new URL("vector-icons/1024.png", this.baseUrl).href, applicationType: "web", diff --git a/test/test-utils/platform.ts b/test/test-utils/platform.ts index 513e301dcbe..1030b0698f0 100644 --- 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, unknown>>) { + constructor(platformMocks: Partial>) { super(); Object.assign(this, platformMocks); } diff --git a/test/utils/oidc/registerClient-test.ts b/test/utils/oidc/registerClient-test.ts index 4ebf754eaa0..2a187b31555 100644 --- 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); }); }); From 2035c3e1cb8edce32d85630b97faccf2c34f4c6f Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 16 Feb 2024 13:11:55 +0000 Subject: [PATCH 5/5] Fix test Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- test/components/structures/auth/Login-test.tsx | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/test/components/structures/auth/Login-test.tsx b/test/components/structures/auth/Login-test.tsx index 1e0e0de78a7..93a02f31db0 100644 --- 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 () => {