From 358c37ad698c8c3b662618e936730fa13ca48141 Mon Sep 17 00:00:00 2001 From: Kerry Date: Fri, 23 Jun 2023 08:59:03 +1200 Subject: [PATCH] OIDC: attempt dynamic client registration (#11074) * add delegatedauthentication to validated server config * dynamic client registration functions * test OP registration functions * add stubbed nativeOidc flow setup in Login * cover more error cases in Login * tidy * test dynamic client registration in Login * comment oidc_static_clients * register oidc inside Login.getFlows * strict fixes * remove unused code * and imports * comments * comments 2 * util functions to get static client id * check static client ids in login flow * remove dead code * OidcRegistrationClientMetadata type * use registerClient from js-sdk * use OidcError from js-sdk --- src/components/structures/auth/Login.tsx | 1 - src/utils/oidc/error.ts | 24 ---------- src/utils/oidc/registerClient.ts | 12 ++--- .../components/structures/auth/Login-test.tsx | 23 +++++---- test/utils/oidc/registerClient-test.ts | 47 ++++++++++++++++--- 5 files changed, 61 insertions(+), 46 deletions(-) delete mode 100644 src/utils/oidc/error.ts diff --git a/src/components/structures/auth/Login.tsx b/src/components/structures/auth/Login.tsx index ea816d35d7..b69c7f3e09 100644 --- a/src/components/structures/auth/Login.tsx +++ b/src/components/structures/auth/Login.tsx @@ -50,7 +50,6 @@ _td("Invalid identity server discovery response"); _td("Invalid base_url for m.identity_server"); _td("Identity server URL does not appear to be a valid identity server"); _td("General failure"); - interface IProps { serverConfig: ValidatedServerConfig; // If true, the component will consider itself busy. diff --git a/src/utils/oidc/error.ts b/src/utils/oidc/error.ts deleted file mode 100644 index 0b4633ab01..0000000000 --- a/src/utils/oidc/error.ts +++ /dev/null @@ -1,24 +0,0 @@ -/* -Copyright 2023 The Matrix.org Foundation C.I.C. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -/** - * OIDC error strings, intended for logging - */ -export enum OidcClientError { - DynamicRegistrationNotSupported = "Dynamic registration not supported", - DynamicRegistrationFailed = "Dynamic registration failed", - DynamicRegistrationInvalid = "Dynamic registration invalid response", -} diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 83dc77bd1d..f292bf5a80 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -15,9 +15,9 @@ limitations under the License. */ import { logger } from "matrix-js-sdk/src/logger"; +import { registerOidcClient } from "matrix-js-sdk/src/oidc/register"; import { ValidatedDelegatedAuthConfig } from "../ValidatedServerConfig"; -import { OidcClientError } from "./error"; /** * Get the statically configured clientId for the issuer @@ -34,6 +34,7 @@ const getStaticOidcClientId = (issuer: string, staticOidcClients?: Record, ): Promise => { const staticClientId = getStaticOidcClientId(delegatedAuthConfig.issuer, staticOidcClients); @@ -53,8 +54,5 @@ export const getOidcClientId = async ( logger.debug(`Using static clientId for issuer ${delegatedAuthConfig.issuer}`); return staticClientId; } - - // TODO attempt dynamic registration - logger.error("Dynamic registration not yet implemented."); - throw new Error(OidcClientError.DynamicRegistrationNotSupported); + return await registerOidcClient(delegatedAuthConfig, clientName, baseUrl); }; diff --git a/test/components/structures/auth/Login-test.tsx b/test/components/structures/auth/Login-test.tsx index 19faecaf97..29a1bfa08e 100644 --- a/test/components/structures/auth/Login-test.tsx +++ b/test/components/structures/auth/Login-test.tsx @@ -21,6 +21,7 @@ import fetchMock from "fetch-mock-jest"; import { DELEGATED_OIDC_COMPATIBILITY, IdentityProviderBrand } from "matrix-js-sdk/src/@types/auth"; import { logger } from "matrix-js-sdk/src/logger"; import { createClient, MatrixClient } from "matrix-js-sdk/src/matrix"; +import { OidcError } from "matrix-js-sdk/src/oidc/error"; import SdkConfig from "../../../../src/SdkConfig"; import { mkServerConfig, mockPlatformPeg, unmockPlatformPeg } from "../../../test-utils"; @@ -30,7 +31,6 @@ import SettingsStore from "../../../../src/settings/SettingsStore"; import { Features } from "../../../../src/settings/Settings"; import { ValidatedDelegatedAuthConfig } from "../../../../src/utils/ValidatedServerConfig"; import * as registerClientUtils from "../../../../src/utils/oidc/registerClient"; -import { OidcClientError } from "../../../../src/utils/oidc/error"; jest.mock("matrix-js-sdk/src/matrix"); @@ -365,6 +365,8 @@ describe("Login", function () { await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + // didn't try to register + expect(fetchMock).not.toHaveBeenCalledWith(delegatedAuth.registrationEndpoint); // continued with normal setup expect(mockClient.loginFlows).toHaveBeenCalled(); // normal password login rendered @@ -374,10 +376,13 @@ describe("Login", function () { it("should attempt to register oidc client", async () => { // dont mock, spy so we can check config values were correctly passed jest.spyOn(registerClientUtils, "getOidcClientId"); + fetchMock.post(delegatedAuth.registrationEndpoint, { status: 500 }); getComponent(hsUrl, isUrl, delegatedAuth); await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + // tried to register + expect(fetchMock).toHaveBeenCalledWith(delegatedAuth.registrationEndpoint, expect.any(Object)); // called with values from config expect(registerClientUtils.getOidcClientId).toHaveBeenCalledWith( delegatedAuth, @@ -387,12 +392,15 @@ describe("Login", function () { ); }); - it("should fallback to normal login when client does not have static clientId", async () => { + it("should fallback to normal login when client registration fails", async () => { + fetchMock.post(delegatedAuth.registrationEndpoint, { status: 500 }); getComponent(hsUrl, isUrl, delegatedAuth); await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); - expect(logger.error).toHaveBeenCalledWith(new Error(OidcClientError.DynamicRegistrationNotSupported)); + // tried to register + expect(fetchMock).toHaveBeenCalledWith(delegatedAuth.registrationEndpoint, expect.any(Object)); + expect(logger.error).toHaveBeenCalledWith(new Error(OidcError.DynamicRegistrationFailed)); // continued with normal setup expect(mockClient.loginFlows).toHaveBeenCalled(); @@ -402,11 +410,8 @@ describe("Login", function () { // short term during active development, UI will be added in next PRs it("should show error when oidc native flow is correctly configured but not supported by UI", async () => { - const delegatedAuthWithStaticClientId = { - ...delegatedAuth, - issuer: "https://staticallyregisteredissuer.org/", - }; - getComponent(hsUrl, isUrl, delegatedAuthWithStaticClientId); + fetchMock.post(delegatedAuth.registrationEndpoint, { client_id: "abc123" }); + getComponent(hsUrl, isUrl, delegatedAuth); await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); @@ -439,6 +444,8 @@ describe("Login", function () { await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + // didn't try to register + expect(fetchMock).not.toHaveBeenCalledWith(delegatedAuth.registrationEndpoint); // continued with normal setup expect(mockClient.loginFlows).toHaveBeenCalled(); // oidc-aware 'continue' button displayed diff --git a/test/utils/oidc/registerClient-test.ts b/test/utils/oidc/registerClient-test.ts index 06f67671d0..0a45f2011b 100644 --- a/test/utils/oidc/registerClient-test.ts +++ b/test/utils/oidc/registerClient-test.ts @@ -15,8 +15,8 @@ limitations under the License. */ import fetchMockJest from "fetch-mock-jest"; +import { OidcError } from "matrix-js-sdk/src/oidc/error"; -import { OidcClientError } from "../../../src/utils/oidc/error"; import { getOidcClientId } from "../../../src/utils/oidc/registerClient"; describe("getOidcClientId()", () => { @@ -56,7 +56,7 @@ describe("getOidcClientId()", () => { }; expect( async () => await getOidcClientId(authConfigWithoutRegistration, clientName, baseUrl, staticOidcClients), - ).rejects.toThrow(OidcClientError.DynamicRegistrationNotSupported); + ).rejects.toThrow(OidcError.DynamicRegistrationNotSupported); // didn't try to register expect(fetchMockJest).toHaveFetchedTimes(0); }); @@ -67,20 +67,55 @@ describe("getOidcClientId()", () => { registrationEndpoint: undefined, }; expect(async () => await getOidcClientId(authConfigWithoutRegistration, clientName, baseUrl)).rejects.toThrow( - OidcClientError.DynamicRegistrationNotSupported, + OidcError.DynamicRegistrationNotSupported, ); // didn't try to register expect(fetchMockJest).toHaveFetchedTimes(0); }); - it("should throw while dynamic registration is not implemented", async () => { + it("should make correct request to register client", async () => { fetchMockJest.post(registrationEndpoint, { status: 200, body: JSON.stringify({ client_id: dynamicClientId }), }); + expect(await getOidcClientId(delegatedAuthConfig, clientName, baseUrl)).toEqual(dynamicClientId); + // didn't try to register + expect(fetchMockJest).toHaveBeenCalledWith(registrationEndpoint, { + headers: { + "Accept": "application/json", + "Content-Type": "application/json", + }, + method: "POST", + body: JSON.stringify({ + client_name: clientName, + client_uri: baseUrl, + response_types: ["code"], + grant_types: ["authorization_code", "refresh_token"], + redirect_uris: [baseUrl], + id_token_signed_response_alg: "RS256", + token_endpoint_auth_method: "none", + application_type: "web", + }), + }); + }); - expect(async () => await getOidcClientId(delegatedAuthConfig, clientName, baseUrl)).rejects.toThrow( - OidcClientError.DynamicRegistrationNotSupported, + it("should throw when registration request fails", async () => { + fetchMockJest.post(registrationEndpoint, { + status: 500, + }); + expect(() => getOidcClientId(delegatedAuthConfig, clientName, baseUrl)).rejects.toThrow( + OidcError.DynamicRegistrationFailed, + ); + }); + + it("should throw when registration response is invalid", async () => { + fetchMockJest.post(registrationEndpoint, { + status: 200, + // no clientId in response + body: "{}", + }); + expect(() => getOidcClientId(delegatedAuthConfig, clientName, baseUrl)).rejects.toThrow( + OidcError.DynamicRegistrationInvalid, ); }); });