From c2a78d146b6726bc23ae34276e73a8b9fd272370 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 7 Jun 2023 17:23:58 +1200 Subject: [PATCH 01/61] add delegatedauthentication to validated server config --- src/utils/AutoDiscoveryUtils.tsx | 17 ++++++++++ src/utils/ValidatedServerConfig.ts | 5 +++ test/utils/AutoDiscoveryUtils-test.tsx | 46 ++++++++++++++++++++++++++ 3 files changed, 68 insertions(+) diff --git a/src/utils/AutoDiscoveryUtils.tsx b/src/utils/AutoDiscoveryUtils.tsx index aaa602abb40..5f1a0448477 100644 --- a/src/utils/AutoDiscoveryUtils.tsx +++ b/src/utils/AutoDiscoveryUtils.tsx @@ -16,8 +16,10 @@ limitations under the License. import React, { ReactNode } from "react"; import { AutoDiscovery, ClientConfig } from "matrix-js-sdk/src/autodiscovery"; +import { IDelegatedAuthConfig, M_AUTHENTICATION } from "matrix-js-sdk/src/client"; import { logger } from "matrix-js-sdk/src/logger"; import { IClientWellKnown } from "matrix-js-sdk/src/matrix"; +import { ValidatedIssuerConfig } from "matrix-js-sdk/src/oidc/validate"; import { _t, UserFriendlyError } from "../languageHandler"; import SdkConfig from "../SdkConfig"; @@ -260,6 +262,20 @@ export default class AutoDiscoveryUtils { throw new UserFriendlyError("Unexpected error resolving homeserver configuration"); } + let delegatedAuthentication = undefined; + if (discoveryResult[M_AUTHENTICATION.stable!]?.state === AutoDiscovery.SUCCESS) { + const { authorizationEndpoint, registrationEndpoint, tokenEndpoint, account, issuer } = discoveryResult[ + M_AUTHENTICATION.stable! + ] as IDelegatedAuthConfig & ValidatedIssuerConfig; + delegatedAuthentication = { + authorizationEndpoint, + registrationEndpoint, + tokenEndpoint, + account, + issuer, + }; + } + return { hsUrl: preferredHomeserverUrl, hsName: preferredHomeserverName, @@ -268,6 +284,7 @@ export default class AutoDiscoveryUtils { isDefault: false, warning: hsResult.error, isNameResolvable: !isSynthetic, + delegatedAuthentication, } as ValidatedServerConfig; } } diff --git a/src/utils/ValidatedServerConfig.ts b/src/utils/ValidatedServerConfig.ts index bac271eef6a..4b58b1ef909 100644 --- a/src/utils/ValidatedServerConfig.ts +++ b/src/utils/ValidatedServerConfig.ts @@ -14,6 +14,9 @@ See the License for the specific language governing permissions and limitations under the License. */ +import { IDelegatedAuthConfig } from "matrix-js-sdk/src/client"; +import { ValidatedIssuerConfig } from "matrix-js-sdk/src/oidc/validate"; + export interface ValidatedServerConfig { hsUrl: string; hsName: string; @@ -26,4 +29,6 @@ export interface ValidatedServerConfig { isNameResolvable: boolean; warning: string | Error; + + delegatedAuthentication?: IDelegatedAuthConfig & ValidatedIssuerConfig; } diff --git a/test/utils/AutoDiscoveryUtils-test.tsx b/test/utils/AutoDiscoveryUtils-test.tsx index 7282f9a8241..a47532179c3 100644 --- a/test/utils/AutoDiscoveryUtils-test.tsx +++ b/test/utils/AutoDiscoveryUtils-test.tsx @@ -16,6 +16,7 @@ limitations under the License. import { AutoDiscovery, AutoDiscoveryAction, ClientConfig } from "matrix-js-sdk/src/autodiscovery"; import { logger } from "matrix-js-sdk/src/logger"; +import { M_AUTHENTICATION } from "matrix-js-sdk/src/client"; import AutoDiscoveryUtils from "../../src/utils/AutoDiscoveryUtils"; @@ -186,5 +187,50 @@ describe("AutoDiscoveryUtils", () => { warning: "Homeserver URL does not appear to be a valid Matrix homeserver", }); }); + + it("ignores delegated auth config when discovery was not successful", () => { + const discoveryResult = { + ...validIsConfig, + ...validHsConfig, + [M_AUTHENTICATION.stable!]: { + state: AutoDiscoveryAction.FAIL_ERROR, + error: "", + }, + }; + const syntaxOnly = true; + expect( + AutoDiscoveryUtils.buildValidatedConfigFromDiscovery(serverName, discoveryResult, syntaxOnly), + ).toEqual({ + ...expectedValidatedConfig, + delegatedAuthentication: undefined, + warning: undefined, + }); + }); + + it("sets delegated auth config when discovery was successful", () => { + const authConfig = { + issuer: "https://test.com/", + authorizationEndpoint: "https://test.com/auth", + registrationEndpoint: "https://test.com/registration", + tokenEndpoint: "https://test.com/token", + }; + const discoveryResult = { + ...validIsConfig, + ...validHsConfig, + [M_AUTHENTICATION.stable!]: { + state: AutoDiscoveryAction.SUCCESS, + error: null, + ...authConfig, + }, + }; + const syntaxOnly = true; + expect( + AutoDiscoveryUtils.buildValidatedConfigFromDiscovery(serverName, discoveryResult, syntaxOnly), + ).toEqual({ + ...expectedValidatedConfig, + delegatedAuthentication: authConfig, + warning: undefined, + }); + }); }); }); From f946b8588baee5125d317f9d6b10b3d71a524000 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 9 Jun 2023 18:03:17 +1200 Subject: [PATCH 02/61] dynamic client registration functions --- src/utils/oidc/registerClient.ts | 97 ++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 src/utils/oidc/registerClient.ts diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts new file mode 100644 index 00000000000..b88ff4ba846 --- /dev/null +++ b/src/utils/oidc/registerClient.ts @@ -0,0 +1,97 @@ +/* +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. +*/ + +import { IDelegatedAuthConfig } from "matrix-js-sdk/src/client"; +import { Method } from "matrix-js-sdk/src/http-api"; +import { ValidatedIssuerConfig } from "matrix-js-sdk/src/oidc/validate"; + +// "staticOidcClients": { +// "https://dev-6525741.okta.com/": { +// "client_id": "0oa5x44w64wpNsxi45d7" +// }, +// "https://keycloak-oidc.lab.element.dev/realms/master/": { +// "client_id": "hydrogen-oidc-playground" +// }, +// "https://id.thirdroom.io/realms/thirdroom/": { +// "client_id": "hydrogen-oidc-playground" +// } +// } + +export type OidcRegistrationClientMetadata = { + clientName: string; + clientUri: string; + redirectUris: string[]; +}; + +/** + * + * @param issuer issue URL + * @param clientMetadata registration metadata + * @returns {Promise} resolves to the registered client id when registration is successful + * @throws when registration fails, or issue does not support dynamic registration + */ +const doRegistration = async ( + registrationEndpoint: string, + clientMetadata: OidcRegistrationClientMetadata, +): Promise => { + // https://openid.net/specs/openid-connect-registration-1_0.html + const metadata = { + client_name: clientMetadata.clientName, + client_uri: clientMetadata.clientUri, + response_types: ["code"], + grant_types: ["authorization_code", "refresh_token"], + redirect_uris: clientMetadata.redirectUris, + id_token_signed_response_alg: "RS256", + token_endpoint_auth_method: "none", + application_type: "web", + }; + const headers = { + "Accept": "application/json", + "Content-Type": "application/json", + }; + const response = await fetch(registrationEndpoint, { + method: Method.Post, + headers, + body: JSON.stringify(metadata), + }); + + if (response.status >= 400) { + throw new Error("Failed to register client"); + } + + const body = await response.json(); + const clientId = body["client_id"]; + if (!clientId) { + throw new Error("Invalid clientId"); + } + + return clientId; +}; + +export const registerOidcClient = async ( + delegatedAuthConfig: IDelegatedAuthConfig & ValidatedIssuerConfig, + clientName: string, + baseUrl: string, +): Promise => { + const clientMetadata = { + clientName, + clientUri: baseUrl, + redirectUris: [baseUrl], + }; + const clientId = await doRegistration(delegatedAuthConfig.registrationEndpoint, clientMetadata); + + return clientId; +}; From 42017250d9446fae5ad58cd0427d7b331e935966 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 12 Jun 2023 13:40:09 +1200 Subject: [PATCH 03/61] test OP registration functions --- src/utils/oidc/error.ts | 25 +++++ src/utils/oidc/registerClient.ts | 57 +++++++++++- test/utils/oidc/registerClient-test.ts | 121 +++++++++++++++++++++++++ 3 files changed, 199 insertions(+), 4 deletions(-) create mode 100644 src/utils/oidc/error.ts create mode 100644 test/utils/oidc/registerClient-test.ts diff --git a/src/utils/oidc/error.ts b/src/utils/oidc/error.ts new file mode 100644 index 00000000000..ea5fa46c1d3 --- /dev/null +++ b/src/utils/oidc/error.ts @@ -0,0 +1,25 @@ +/* +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. +*/ + +import { OidcDiscoveryError } from "matrix-js-sdk/src/oidc/validate"; + +export enum OidcClientError { + DynamicRegistrationNotSupported = "Dynamic registration not supported", + DynamicRegistrationFailed = "Dynamic registration failed", + DynamicRegistrationInvalid = "Dynamic registration invalid response", +} + +export type OidcError = OidcClientError | OidcDiscoveryError; diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index b88ff4ba846..85973070bba 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -16,8 +16,11 @@ limitations under the License. import { IDelegatedAuthConfig } from "matrix-js-sdk/src/client"; import { Method } from "matrix-js-sdk/src/http-api"; +import { logger } from "matrix-js-sdk/src/logger"; import { ValidatedIssuerConfig } from "matrix-js-sdk/src/oidc/validate"; +import { OidcClientError } from "./error"; + // "staticOidcClients": { // "https://dev-6525741.okta.com/": { // "client_id": "0oa5x44w64wpNsxi45d7" @@ -37,7 +40,7 @@ export type OidcRegistrationClientMetadata = { }; /** - * + * Make the registration request * @param issuer issue URL * @param clientMetadata registration metadata * @returns {Promise} resolves to the registered client id when registration is successful @@ -69,19 +72,27 @@ const doRegistration = async ( }); if (response.status >= 400) { - throw new Error("Failed to register client"); + throw new Error(OidcClientError.DynamicRegistrationFailed); } const body = await response.json(); const clientId = body["client_id"]; if (!clientId) { - throw new Error("Invalid clientId"); + throw new Error(OidcClientError.DynamicRegistrationInvalid); } return clientId; }; -export const registerOidcClient = async ( +/** + * Attempts dynamic registration against the configured registration endpoint + * @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/' + * @returns Promise resolved with registered clientId + * @throws on failed request or invalid response + */ +const registerOidcClient = async ( delegatedAuthConfig: IDelegatedAuthConfig & ValidatedIssuerConfig, clientName: string, baseUrl: string, @@ -95,3 +106,41 @@ export const registerOidcClient = async ( return clientId; }; + +/** + * Get the configured clientId for the issuer + * @param issuer delegated auth OIDC issuer + * @param staticOidcClients static client config from config.json + * @returns clientId if found, otherwise undefined + */ +const getStaticOidcClientId = (issuer: string, staticOidcClients?: Record): string | undefined => { + return staticOidcClients?.[issuer]; +}; + +/** + * Get the clientId for configured oidc OP + * 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 or registration failed + */ +export const getOidcClientId = async ( + delegatedAuthConfig: IDelegatedAuthConfig & ValidatedIssuerConfig, + clientName: string, + baseUrl: string, + staticOidcClients?: Record, +): Promise => { + const staticClientId = getStaticOidcClientId(delegatedAuthConfig.issuer, staticOidcClients); + if (staticClientId) { + logger.debug(`Using static clientId for issuer ${delegatedAuthConfig.issuer}`); + return staticClientId; + } + if (!delegatedAuthConfig.registrationEndpoint) { + throw new Error(OidcClientError.DynamicRegistrationNotSupported); + } + return await registerOidcClient(delegatedAuthConfig, clientName, baseUrl); +}; diff --git a/test/utils/oidc/registerClient-test.ts b/test/utils/oidc/registerClient-test.ts new file mode 100644 index 00000000000..b5cb3d11bdb --- /dev/null +++ b/test/utils/oidc/registerClient-test.ts @@ -0,0 +1,121 @@ +/* +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. +*/ + +import fetchMockJest from "fetch-mock-jest"; + +import { OidcClientError } from "../../../src/utils/oidc/error"; +import { getOidcClientId } from "../../../src/utils/oidc/registerClient"; + +describe("getOidcClientId()", () => { + const issuer = "https://auth.com/"; + const registrationEndpoint = "https://auth.com/register"; + const clientName = "Element"; + const baseUrl = "https://just.testing"; + const dynamicClientId = "xyz789"; + const staticOidcClients = { + [issuer]: "abc123", + }; + const delegatedAuthConfig = { + issuer, + registrationEndpoint, + authorizationEndpoint: issuer + "auth", + tokenEndpoint: issuer + "token", + }; + + beforeEach(() => { + fetchMockJest.mockClear(); + fetchMockJest.resetBehavior(); + }); + + it("should return static clientId when configured", async () => { + expect(await getOidcClientId(delegatedAuthConfig, clientName, baseUrl, staticOidcClients)).toEqual( + staticOidcClients[issuer], + ); + // didn't try to register + expect(fetchMockJest).toHaveFetchedTimes(0); + }); + + it("should throw when no static clientId is configured and no registration endpoint", async () => { + const authConfigWithoutRegistration = { + ...delegatedAuthConfig, + issuer: "https://issuerWithoutStaticClientId.org/", + registrationEndpoint: undefined, + }; + expect( + async () => await getOidcClientId(authConfigWithoutRegistration, clientName, baseUrl, staticOidcClients), + ).rejects.toThrow(OidcClientError.DynamicRegistrationNotSupported); + // didn't try to register + expect(fetchMockJest).toHaveFetchedTimes(0); + }); + + it("should handle when staticOidcClients object is falsy", async () => { + const authConfigWithoutRegistration = { + ...delegatedAuthConfig, + registrationEndpoint: undefined, + }; + expect(async () => await getOidcClientId(authConfigWithoutRegistration, clientName, baseUrl)).rejects.toThrow( + OidcClientError.DynamicRegistrationNotSupported, + ); + // didn't try to register + expect(fetchMockJest).toHaveFetchedTimes(0); + }); + + 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", + }), + }); + }); + + it("should throw when registration request fails", async () => { + fetchMockJest.post(registrationEndpoint, { + status: 500, + }); + expect(() => getOidcClientId(delegatedAuthConfig, clientName, baseUrl)).rejects.toThrow( + OidcClientError.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( + OidcClientError.DynamicRegistrationInvalid, + ); + }); +}); From 314105d1d05f060644cc19f2e10b692293b15224 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 12 Jun 2023 15:05:04 +1200 Subject: [PATCH 04/61] add stubbed nativeOidc flow setup in Login --- src/IConfigOptions.ts | 2 + src/components/structures/auth/Login.tsx | 104 ++++++++++++++---- .../components/structures/auth/Login-test.tsx | 1 + 3 files changed, 84 insertions(+), 23 deletions(-) diff --git a/src/IConfigOptions.ts b/src/IConfigOptions.ts index 1fa873ec87a..5753c6e85da 100644 --- a/src/IConfigOptions.ts +++ b/src/IConfigOptions.ts @@ -192,6 +192,8 @@ export interface IConfigOptions { existing_issues_url: string; new_issue_url: string; }; + + oidc_static_clients?: Record; } export interface ISsoRedirectOptions { diff --git a/src/components/structures/auth/Login.tsx b/src/components/structures/auth/Login.tsx index 3299cae3fc3..2061facbd73 100644 --- a/src/components/structures/auth/Login.tsx +++ b/src/components/structures/auth/Login.tsx @@ -38,6 +38,11 @@ import AuthHeader from "../../views/auth/AuthHeader"; import AccessibleButton, { ButtonEvent } from "../../views/elements/AccessibleButton"; import { ValidatedServerConfig } from "../../../utils/ValidatedServerConfig"; import { filterBoolean } from "../../../utils/arrays"; +import { Features } from "../../../settings/Settings"; +import { getOidcClientId } from "../../../utils/oidc/registerClient"; +import SdkConfig from "../../../SdkConfig"; +import { IConfigOptions } from "../../../IConfigOptions"; +import { OidcClientError } from "../../../utils/oidc/error"; // These are used in several places, and come from the js-sdk's autodiscovery // stuff. We define them here so that they'll be picked up by i18n. @@ -98,6 +103,9 @@ interface IState { serverIsAlive: boolean; serverErrorIsFatal: boolean; serverDeadError?: ReactNode; + + // @TODO(kerrya) does this need to be in state? + oidcClientId?: string; } type OnPasswordLogin = { @@ -110,6 +118,7 @@ type OnPasswordLogin = { */ export default class LoginComponent extends React.PureComponent { private unmounted = false; + private oidcNativeFlowEnabled = false; private loginLogic!: Login; private readonly stepRendererMap: Record ReactNode>; @@ -117,6 +126,9 @@ export default class LoginComponent extends React.PureComponent public constructor(props: IProps) { super(props); + // only set on a config level, so we don't need to watch + this.oidcNativeFlowEnabled = SettingsStore.getValue(Features.OidcNativeFlow); + this.state = { busy: false, errorText: null, @@ -156,7 +168,8 @@ export default class LoginComponent extends React.PureComponent public componentDidUpdate(prevProps: IProps): void { if ( prevProps.serverConfig.hsUrl !== this.props.serverConfig.hsUrl || - prevProps.serverConfig.isUrl !== this.props.serverConfig.isUrl + prevProps.serverConfig.isUrl !== this.props.serverConfig.isUrl || + prevProps.serverConfig.delegatedAuthentication !== this.props.serverConfig.delegatedAuthentication ) { // Ensure that we end up actually logging in to the right place this.initLoginLogic(this.props.serverConfig); @@ -322,28 +335,7 @@ export default class LoginComponent extends React.PureComponent } }; - private async initLoginLogic({ hsUrl, isUrl }: ValidatedServerConfig): Promise { - let isDefaultServer = false; - if ( - this.props.serverConfig.isDefault && - hsUrl === this.props.serverConfig.hsUrl && - isUrl === this.props.serverConfig.isUrl - ) { - isDefaultServer = true; - } - - const fallbackHsUrl = isDefaultServer ? this.props.fallbackHsUrl! : null; - - const loginLogic = new Login(hsUrl, isUrl, fallbackHsUrl, { - defaultDeviceDisplayName: this.props.defaultDeviceDisplayName, - }); - this.loginLogic = loginLogic; - - this.setState({ - busy: true, - loginIncorrect: false, - }); - + private async checkServerLiveliness({ hsUrl, isUrl }: Pick): Promise { // Do a quick liveliness check on the URLs try { const { warning } = await AutoDiscoveryUtils.validateServerConfigWithStaticUrls(hsUrl, isUrl); @@ -364,6 +356,70 @@ export default class LoginComponent extends React.PureComponent ...AutoDiscoveryUtils.authComponentStateForError(e), }); } + } + + private async tryInitOidcNativeFlow(): Promise { + if (!this.props.serverConfig.delegatedAuthentication || !this.oidcNativeFlowEnabled) { + return false; + } + try { + const clientId = await getOidcClientId( + this.props.serverConfig.delegatedAuthentication, + SdkConfig.get().brand, + window.location.origin, + SdkConfig.get().oidc_static_clients, + ); + + if (!this.isSupportedFlow('oidcNativeFlow')) { + throw new Error('OIDC native login flow configured but UI not yet supported.'); + } + + this.setState({ + busy: false, + oidcClientId: clientId, + flows: ['oidcNativeFlow'] + }); + return true; + } catch (error) { + console.error(error); + return false; + } + } + + private async initLoginLogic({ hsUrl, isUrl }: ValidatedServerConfig): Promise { + let isDefaultServer = false; + if ( + this.props.serverConfig.isDefault && + hsUrl === this.props.serverConfig.hsUrl && + isUrl === this.props.serverConfig.isUrl + ) { + isDefaultServer = true; + } + + const fallbackHsUrl = isDefaultServer ? this.props.fallbackHsUrl! : null; + + this.setState({ + busy: true, + loginIncorrect: false, + oidcClientId: undefined, + }); + + await this.checkServerLiveliness({ hsUrl, isUrl }); + + const useOidcNativeFlow = await this.tryInitOidcNativeFlow(); + + console.log('hhh initLoginLogic', { useOidcNativeFlow }); + + // don't continue with setup + // as we are using oidc native flow + if (useOidcNativeFlow) { + return; + } + + const loginLogic = new Login(hsUrl, isUrl, fallbackHsUrl, { + defaultDeviceDisplayName: this.props.defaultDeviceDisplayName, + }); + this.loginLogic = loginLogic; loginLogic .getFlows() @@ -473,6 +529,8 @@ export default class LoginComponent extends React.PureComponent const errorText = this.state.errorText; + console.log('hhh render()', this.state); + let errorTextSection; if (errorText) { errorTextSection =
{errorText}
; diff --git a/test/components/structures/auth/Login-test.tsx b/test/components/structures/auth/Login-test.tsx index a84e88b17c1..3accb5772bd 100644 --- a/test/components/structures/auth/Login-test.tsx +++ b/test/components/structures/auth/Login-test.tsx @@ -189,6 +189,7 @@ describe("Login", function () { versions: [], }); rerender(getRawComponent("https://server2")); + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); fireEvent.click(container.querySelector(".mx_SSOButton")!); expect(platform.startSingleSignOn.mock.calls[1][0].baseUrl).toBe("https://server2"); From 6fe2d354cb0b6546223c628313b5993327752059 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 12 Jun 2023 15:39:26 +1200 Subject: [PATCH 05/61] cover more error cases in Login --- .../components/structures/auth/Login-test.tsx | 68 ++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-) diff --git a/test/components/structures/auth/Login-test.tsx b/test/components/structures/auth/Login-test.tsx index a84e88b17c1..8eb25d49097 100644 --- a/test/components/structures/auth/Login-test.tsx +++ b/test/components/structures/auth/Login-test.tsx @@ -1,5 +1,5 @@ /* -Copyright 2019 New Vector Ltd +Copyright 2019, 2023 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -50,6 +50,7 @@ describe("Login", function () { mockClient.baseUrl = opts.baseUrl; return mockClient; }); + fetchMock.resetBehavior(); fetchMock.get("https://matrix.org/_matrix/client/versions", { unstable_features: {}, versions: [], @@ -253,4 +254,69 @@ describe("Login", function () { const ssoButtons = container.querySelectorAll(".mx_SSOButton"); expect(ssoButtons.length).toBe(idpsWithIcons.length + 1); }); + + it("should display an error when homeserver doesn't offer any supported login flows", async () => { + mockClient.loginFlows.mockResolvedValue({ + flows: [ + { + type: "just something weird", + }, + ], + }); + + getComponent(); + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + + expect( + screen.getByText("This homeserver doesn't offer any login flows which are supported by this client."), + ).toBeInTheDocument(); + }); + + it("should display a connection error when getting login flows fails", async () => { + mockClient.loginFlows.mockRejectedValue("oups"); + + getComponent(); + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + + expect( + screen.getByText("There was a problem communicating with the homeserver, please try again later."), + ).toBeInTheDocument(); + }); + + it("should display an error when homeserver fails liveliness check", async () => { + fetchMock.resetBehavior(); + fetchMock.get("https://matrix.org/_matrix/client/versions", { + status: 400, + }); + getComponent(); + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + + // error displayed + expect(screen.getByText("Your test-brand is misconfigured")).toBeInTheDocument(); + }); + + it("should reset liveliness error when server config changes", async () => { + fetchMock.resetBehavior(); + // matrix.org is not alive + fetchMock.get("https://matrix.org/_matrix/client/versions", { + status: 400, + }); + // but server2 is + fetchMock.get("https://server2/_matrix/client/versions", { + unstable_features: {}, + versions: [], + }); + const { rerender } = render(getRawComponent()); + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + + // error displayed + expect(screen.getByText("Your test-brand is misconfigured")).toBeInTheDocument(); + + rerender(getRawComponent("https://server2")); + + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + + // error cleared + expect(screen.queryByText("Your test-brand is misconfigured")).not.toBeInTheDocument(); + }); }); From 19dc425de2b3d06d711eeea2a42f68cb901cc9ba Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 12 Jun 2023 15:49:19 +1200 Subject: [PATCH 06/61] tidy --- src/components/structures/auth/Login.tsx | 17 +++++++++-------- src/utils/oidc/registerClient.ts | 12 ------------ 2 files changed, 9 insertions(+), 20 deletions(-) diff --git a/src/components/structures/auth/Login.tsx b/src/components/structures/auth/Login.tsx index 2061facbd73..08716f73c88 100644 --- a/src/components/structures/auth/Login.tsx +++ b/src/components/structures/auth/Login.tsx @@ -41,8 +41,6 @@ import { filterBoolean } from "../../../utils/arrays"; import { Features } from "../../../settings/Settings"; import { getOidcClientId } from "../../../utils/oidc/registerClient"; import SdkConfig from "../../../SdkConfig"; -import { IConfigOptions } from "../../../IConfigOptions"; -import { OidcClientError } from "../../../utils/oidc/error"; // These are used in several places, and come from the js-sdk's autodiscovery // stuff. We define them here so that they'll be picked up by i18n. @@ -335,7 +333,10 @@ export default class LoginComponent extends React.PureComponent } }; - private async checkServerLiveliness({ hsUrl, isUrl }: Pick): Promise { + private async checkServerLiveliness({ + hsUrl, + isUrl, + }: Pick): Promise { // Do a quick liveliness check on the URLs try { const { warning } = await AutoDiscoveryUtils.validateServerConfigWithStaticUrls(hsUrl, isUrl); @@ -370,14 +371,14 @@ export default class LoginComponent extends React.PureComponent SdkConfig.get().oidc_static_clients, ); - if (!this.isSupportedFlow('oidcNativeFlow')) { - throw new Error('OIDC native login flow configured but UI not yet supported.'); + if (!this.isSupportedFlow("oidcNativeFlow")) { + throw new Error("OIDC native login flow configured but UI not yet supported."); } this.setState({ busy: false, oidcClientId: clientId, - flows: ['oidcNativeFlow'] + flows: ["oidcNativeFlow"], }); return true; } catch (error) { @@ -408,7 +409,7 @@ export default class LoginComponent extends React.PureComponent const useOidcNativeFlow = await this.tryInitOidcNativeFlow(); - console.log('hhh initLoginLogic', { useOidcNativeFlow }); + console.log("hhh initLoginLogic", { useOidcNativeFlow }); // don't continue with setup // as we are using oidc native flow @@ -529,7 +530,7 @@ export default class LoginComponent extends React.PureComponent const errorText = this.state.errorText; - console.log('hhh render()', this.state); + console.log("hhh render()", this.state); let errorTextSection; if (errorText) { diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 85973070bba..3f5dc5265f5 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -21,18 +21,6 @@ import { ValidatedIssuerConfig } from "matrix-js-sdk/src/oidc/validate"; import { OidcClientError } from "./error"; -// "staticOidcClients": { -// "https://dev-6525741.okta.com/": { -// "client_id": "0oa5x44w64wpNsxi45d7" -// }, -// "https://keycloak-oidc.lab.element.dev/realms/master/": { -// "client_id": "hydrogen-oidc-playground" -// }, -// "https://id.thirdroom.io/realms/thirdroom/": { -// "client_id": "hydrogen-oidc-playground" -// } -// } - export type OidcRegistrationClientMetadata = { clientName: string; clientUri: string; From 94d5c369baf9f1ad86f98704d7cdcece3d71b530 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 12 Jun 2023 18:13:14 +1200 Subject: [PATCH 07/61] test dynamic client registration in Login --- src/components/structures/auth/Login.tsx | 31 ++--- src/utils/oidc/registerClient.ts | 38 +++--- .../components/structures/auth/Login-test.tsx | 114 +++++++++++++++++- test/test-utils/test-utils.ts | 7 +- 4 files changed, 155 insertions(+), 35 deletions(-) diff --git a/src/components/structures/auth/Login.tsx b/src/components/structures/auth/Login.tsx index 08716f73c88..23486187d2d 100644 --- a/src/components/structures/auth/Login.tsx +++ b/src/components/structures/auth/Login.tsx @@ -17,7 +17,7 @@ limitations under the License. import React, { ReactNode } from "react"; import classNames from "classnames"; import { logger } from "matrix-js-sdk/src/logger"; -import { ISSOFlow, LoginFlow, SSOAction } from "matrix-js-sdk/src/@types/auth"; +import { ILoginFlow, ISSOFlow, LoginFlow, SSOAction } from "matrix-js-sdk/src/@types/auth"; import { _t, _td, UserFriendlyError } from "../../../languageHandler"; import Login from "../../../Login"; @@ -52,6 +52,11 @@ _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"); +export interface OidcNativeFlow extends ILoginFlow { + type: "oidcNativeFlow"; + clientId: string; +} +type ClientLoginFlow = LoginFlow | OidcNativeFlow; interface IProps { serverConfig: ValidatedServerConfig; @@ -87,7 +92,7 @@ interface IState { // can we attempt to log in or are there validation errors? canTryLogin: boolean; - flows?: LoginFlow[]; + flows?: ClientLoginFlow[]; // used for preserving form values when changing homeserver username: string; @@ -101,9 +106,6 @@ interface IState { serverIsAlive: boolean; serverErrorIsFatal: boolean; serverDeadError?: ReactNode; - - // @TODO(kerrya) does this need to be in state? - oidcClientId?: string; } type OnPasswordLogin = { @@ -371,18 +373,22 @@ export default class LoginComponent extends React.PureComponent SdkConfig.get().oidc_static_clients, ); - if (!this.isSupportedFlow("oidcNativeFlow")) { + const flow = { + type: "oidcNativeFlow", + clientId, + }; + + if (!this.isSupportedFlow(flow)) { throw new Error("OIDC native login flow configured but UI not yet supported."); } this.setState({ busy: false, - oidcClientId: clientId, - flows: ["oidcNativeFlow"], + flows: [flow], }); return true; } catch (error) { - console.error(error); + logger.error(error); return false; } } @@ -402,15 +408,12 @@ export default class LoginComponent extends React.PureComponent this.setState({ busy: true, loginIncorrect: false, - oidcClientId: undefined, }); await this.checkServerLiveliness({ hsUrl, isUrl }); const useOidcNativeFlow = await this.tryInitOidcNativeFlow(); - console.log("hhh initLoginLogic", { useOidcNativeFlow }); - // don't continue with setup // as we are using oidc native flow if (useOidcNativeFlow) { @@ -458,7 +461,7 @@ export default class LoginComponent extends React.PureComponent }); } - private isSupportedFlow = (flow: LoginFlow): boolean => { + private isSupportedFlow = (flow: ClientLoginFlow): boolean => { // technically the flow can have multiple steps, but no one does this // for login and loginLogic doesn't support it so we can ignore it. if (!this.stepRendererMap[flow.type]) { @@ -530,8 +533,6 @@ export default class LoginComponent extends React.PureComponent const errorText = this.state.errorText; - console.log("hhh render()", this.state); - let errorTextSection; if (errorText) { errorTextSection =
{errorText}
; diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 3f5dc5265f5..9279d043053 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -53,23 +53,33 @@ const doRegistration = async ( "Accept": "application/json", "Content-Type": "application/json", }; - const response = await fetch(registrationEndpoint, { - method: Method.Post, - headers, - body: JSON.stringify(metadata), - }); - if (response.status >= 400) { - throw new Error(OidcClientError.DynamicRegistrationFailed); - } + try { + const response = await fetch(registrationEndpoint, { + method: Method.Post, + headers, + body: JSON.stringify(metadata), + }); - const body = await response.json(); - const clientId = body["client_id"]; - if (!clientId) { - throw new Error(OidcClientError.DynamicRegistrationInvalid); - } + if (response.status >= 400) { + throw new Error(OidcClientError.DynamicRegistrationFailed); + } - return clientId; + const body = await response.json(); + const clientId = body["client_id"]; + if (!clientId) { + throw new Error(OidcClientError.DynamicRegistrationInvalid); + } + + return clientId; + } catch (error) { + if (Object.values(OidcClientError).includes(error.message)) { + throw error; + } else { + logger.error("Dynamic registration request failed", error); + throw new Error(OidcClientError.DynamicRegistrationFailed); + } + } }; /** diff --git a/test/components/structures/auth/Login-test.tsx b/test/components/structures/auth/Login-test.tsx index f5305434fbe..81294fb1d4d 100644 --- a/test/components/structures/auth/Login-test.tsx +++ b/test/components/structures/auth/Login-test.tsx @@ -17,19 +17,29 @@ limitations under the License. import React from "react"; import { fireEvent, render, screen, waitForElementToBeRemoved } from "@testing-library/react"; import { mocked, MockedObject } from "jest-mock"; -import { createClient, MatrixClient } from "matrix-js-sdk/src/matrix"; 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 SdkConfig from "../../../../src/SdkConfig"; import { mkServerConfig, mockPlatformPeg, unmockPlatformPeg } from "../../../test-utils"; import Login from "../../../../src/components/structures/auth/Login"; import BasePlatform from "../../../../src/BasePlatform"; +import SettingsStore from "../../../../src/settings/SettingsStore"; +import { Features } from "../../../../src/settings/Settings"; +import { ValidatedServerConfig } 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"); jest.useRealTimers(); +const oidcStaticClientsConfig = { + "https://staticallyregisteredissuer.org/": "static-clientId-123", +}; + describe("Login", function () { let platform: MockedObject; @@ -42,6 +52,7 @@ describe("Login", function () { SdkConfig.put({ brand: "test-brand", disable_custom_urls: true, + oidc_static_clients: oidcStaticClientsConfig, }); mockClient.login.mockClear().mockResolvedValue({}); mockClient.loginFlows.mockClear().mockResolvedValue({ flows: [{ type: "m.login.password" }] }); @@ -51,6 +62,7 @@ describe("Login", function () { return mockClient; }); fetchMock.resetBehavior(); + fetchMock.resetHistory(); fetchMock.get("https://matrix.org/_matrix/client/versions", { unstable_features: {}, versions: [], @@ -66,10 +78,14 @@ describe("Login", function () { unmockPlatformPeg(); }); - function getRawComponent(hsUrl = "https://matrix.org", isUrl = "https://vector.im") { + function getRawComponent( + hsUrl = "https://matrix.org", + isUrl = "https://vector.im", + delegatedAuthentication?: ValidatedServerConfig["delegatedAuthentication"], + ) { return ( {}} onRegisterClick={() => {}} onServerConfigChange={() => {}} @@ -77,8 +93,12 @@ describe("Login", function () { ); } - function getComponent(hsUrl?: string, isUrl?: string) { - return render(getRawComponent(hsUrl, isUrl)); + function getComponent( + hsUrl?: string, + isUrl?: string, + delegatedAuthentication?: ValidatedServerConfig["delegatedAuthentication"], + ) { + return render(getRawComponent(hsUrl, isUrl, delegatedAuthentication)); } it("should show form with change server link", async () => { @@ -320,4 +340,88 @@ describe("Login", function () { // error cleared expect(screen.queryByText("Your test-brand is misconfigured")).not.toBeInTheDocument(); }); + + describe("OIDC native flow", () => { + const hsUrl = "https://matrix.org"; + const isUrl = "https://vector.im"; + const issuer = "https://test.com/"; + const delegatedAuth = { + issuer, + registrationEndpoint: issuer + "register", + tokenEndpoint: issuer + "token", + authorizationEndpoint: issuer + "authorization", + }; + beforeEach(() => { + jest.spyOn(logger, "error").mockClear(); + jest.spyOn(SettingsStore, "getValue").mockImplementation( + (settingName) => settingName === Features.OidcNativeFlow, + ); + }); + + it("should not attempt registration when oidc native flow setting is disabled", async () => { + jest.spyOn(SettingsStore, "getValue").mockReturnValue(false); + + getComponent(hsUrl, isUrl, delegatedAuth); + + 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 + expect(screen.getByLabelText("Username")).toBeInTheDocument(); + }); + + 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, + "test-brand", + "http://localhost", + oidcStaticClientsConfig, + ); + }); + + 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…")); + + // tried to register + expect(fetchMock).toHaveBeenCalledWith(delegatedAuth.registrationEndpoint, expect.any(Object)); + expect(logger.error).toHaveBeenCalledWith(new Error(OidcClientError.DynamicRegistrationFailed)); + + // continued with normal setup + expect(mockClient.loginFlows).toHaveBeenCalled(); + // normal password login rendered + expect(screen.getByLabelText("Username")).toBeInTheDocument(); + }); + + it("should fallback to normal login while oidc native login is not supported in UI", async () => { + fetchMock.post(delegatedAuth.registrationEndpoint, { client_id: "abc123" }); + getComponent(hsUrl, isUrl, delegatedAuth); + + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + + expect(logger.error).toHaveBeenCalledWith( + new Error("OIDC native login flow configured but UI not yet supported."), + ); + + // continued with normal setup + expect(mockClient.loginFlows).toHaveBeenCalled(); + // normal password login rendered + expect(screen.getByLabelText("Username")).toBeInTheDocument(); + }); + }); }); diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts index 6eea4114d23..a2ca4313407 100644 --- a/test/test-utils/test-utils.ts +++ b/test/test-utils/test-utils.ts @@ -617,12 +617,17 @@ export function mkStubRoom( } as unknown as Room; } -export function mkServerConfig(hsUrl: string, isUrl: string): ValidatedServerConfig { +export function mkServerConfig( + hsUrl: string, + isUrl: string, + delegatedAuthentication?: ValidatedServerConfig["delegatedAuthentication"], +): ValidatedServerConfig { return { hsUrl, hsName: "TEST_ENVIRONMENT", hsNameIsDifferent: false, // yes, we lie isUrl, + delegatedAuthentication, } as ValidatedServerConfig; } From 67f6c2ede6fe243a38dd033361cf5e0486c94153 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 12 Jun 2023 18:13:31 +1200 Subject: [PATCH 08/61] comment oidc_static_clients --- src/IConfigOptions.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/IConfigOptions.ts b/src/IConfigOptions.ts index 5753c6e85da..bf8ca1ee866 100644 --- a/src/IConfigOptions.ts +++ b/src/IConfigOptions.ts @@ -193,6 +193,12 @@ export interface IConfigOptions { new_issue_url: string; }; + /** + * Configuration for OIDC issuers where a static client_id has been issued for the app. + * Otherwise dynamic client registration is attempted. + * The issuer URL must have a trailing `/`. + * OPTIONAL + */ oidc_static_clients?: Record; } From 8df092968e95416ebbe058fcd34b4322af04d0cb Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 12 Jun 2023 18:49:37 +1200 Subject: [PATCH 09/61] register oidc inside Login.getFlows --- src/Login.ts | 64 ++++++++++++++++++- src/components/structures/auth/Login.tsx | 55 ++-------------- .../components/structures/auth/Login-test.tsx | 44 +++++++++++-- 3 files changed, 104 insertions(+), 59 deletions(-) diff --git a/src/Login.ts b/src/Login.ts index 569363eeb05..6426550eaa6 100644 --- a/src/Login.ts +++ b/src/Login.ts @@ -19,18 +19,32 @@ limitations under the License. import { createClient } from "matrix-js-sdk/src/matrix"; import { MatrixClient } from "matrix-js-sdk/src/client"; import { logger } from "matrix-js-sdk/src/logger"; -import { DELEGATED_OIDC_COMPATIBILITY, ILoginParams, LoginFlow } from "matrix-js-sdk/src/@types/auth"; +import { OidcDiscoveryError } from "matrix-js-sdk/src/oidc/validate"; +import { DELEGATED_OIDC_COMPATIBILITY, ILoginFlow, ILoginParams, LoginFlow } from "matrix-js-sdk/src/@types/auth"; import { IMatrixClientCreds } from "./MatrixClientPeg"; import SecurityCustomisations from "./customisations/Security"; +import { ValidatedServerConfig } from "./utils/ValidatedServerConfig"; +import { getOidcClientId } from "./utils/oidc/registerClient"; +import { IConfigOptions } from "./IConfigOptions"; +import SdkConfig from "./SdkConfig"; + +export type ClientLoginFlow = LoginFlow | OidcNativeFlow; interface ILoginOptions { defaultDeviceDisplayName?: string; + /** + * Delegated auth config from server config + * When prop is passed we will attempt to use delegated auth + * Labs flag should be checked in parent + */ + delegatedAuthentication?: ValidatedServerConfig["delegatedAuthentication"]; } export default class Login { - private flows: Array = []; + private flows: Array = []; private readonly defaultDeviceDisplayName?: string; + private readonly delegatedAuthentication?: ValidatedServerConfig["delegatedAuthentication"]; private tempClient: MatrixClient | null = null; // memoize public constructor( @@ -40,6 +54,7 @@ export default class Login { opts: ILoginOptions, ) { this.defaultDeviceDisplayName = opts.defaultDeviceDisplayName; + this.delegatedAuthentication = opts.delegatedAuthentication; } public getHomeserverUrl(): string { @@ -75,7 +90,20 @@ export default class Login { return this.tempClient; } - public async getFlows(): Promise> { + public async getFlows(): Promise> { + // try to use oidc native flow + try { + const oidcFlow = await tryInitOidcNativeFlow( + this.delegatedAuthentication, + SdkConfig.get().brand, + SdkConfig.get().oidc_static_clients, + ); + return [oidcFlow]; + } catch (error) { + logger.error(error); + } + + // oidc native flow not supported, continue with matrix login const client = this.createTemporaryClient(); const { flows }: { flows: LoginFlow[] } = await client.loginFlows(); // If an m.login.sso flow is present which is also flagged as being for MSC3824 OIDC compatibility then we only @@ -151,6 +179,36 @@ export default class Login { } } +export interface OidcNativeFlow extends ILoginFlow { + type: "oidcNativeFlow"; + clientId: string; +} +/** + * Finds static clientId for configured issuer, or attempts dynamic registration with the OP + * @param delegatedAuthConfig Auth config from ValidatedServerConfig + * @param clientName Client name to register with the OP, eg 'Element', used during client registration with OP + * @param staticOidcClients static client config from config.json, used during client registration with OP + * @returns Promise when oidc native authentication flow is supported and correctly configured + * @throws when oidc native flow is not configured, client can't register with OP, or any unexpected error + */ +const tryInitOidcNativeFlow = async ( + delegatedAuthConfig: ValidatedServerConfig["delegatedAuthentication"] | undefined, + brand: string, + oidcStaticClients?: IConfigOptions["oidc_static_clients"], +): Promise => { + if (!delegatedAuthConfig) { + throw new Error(OidcDiscoveryError.NotSupported); + } + const clientId = await getOidcClientId(delegatedAuthConfig, brand, window.location.origin, oidcStaticClients); + + const flow = { + type: "oidcNativeFlow", + clientId, + } as OidcNativeFlow; + + return flow; +}; + /** * Send a login request to the given server, and format the response * as a MatrixClientCreds diff --git a/src/components/structures/auth/Login.tsx b/src/components/structures/auth/Login.tsx index 23486187d2d..c6095294e55 100644 --- a/src/components/structures/auth/Login.tsx +++ b/src/components/structures/auth/Login.tsx @@ -17,10 +17,10 @@ limitations under the License. import React, { ReactNode } from "react"; import classNames from "classnames"; import { logger } from "matrix-js-sdk/src/logger"; -import { ILoginFlow, ISSOFlow, LoginFlow, SSOAction } from "matrix-js-sdk/src/@types/auth"; +import { ISSOFlow, SSOAction } from "matrix-js-sdk/src/@types/auth"; import { _t, _td, UserFriendlyError } from "../../../languageHandler"; -import Login from "../../../Login"; +import Login, { ClientLoginFlow } from "../../../Login"; import { messageForConnectionError, messageForLoginError } from "../../../utils/ErrorUtils"; import AutoDiscoveryUtils from "../../../utils/AutoDiscoveryUtils"; import AuthPage from "../../views/auth/AuthPage"; @@ -39,8 +39,6 @@ import AccessibleButton, { ButtonEvent } from "../../views/elements/AccessibleBu import { ValidatedServerConfig } from "../../../utils/ValidatedServerConfig"; import { filterBoolean } from "../../../utils/arrays"; import { Features } from "../../../settings/Settings"; -import { getOidcClientId } from "../../../utils/oidc/registerClient"; -import SdkConfig from "../../../SdkConfig"; // These are used in several places, and come from the js-sdk's autodiscovery // stuff. We define them here so that they'll be picked up by i18n. @@ -52,12 +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"); -export interface OidcNativeFlow extends ILoginFlow { - type: "oidcNativeFlow"; - clientId: string; -} -type ClientLoginFlow = LoginFlow | OidcNativeFlow; - interface IProps { serverConfig: ValidatedServerConfig; // If true, the component will consider itself busy. @@ -361,38 +353,6 @@ export default class LoginComponent extends React.PureComponent } } - private async tryInitOidcNativeFlow(): Promise { - if (!this.props.serverConfig.delegatedAuthentication || !this.oidcNativeFlowEnabled) { - return false; - } - try { - const clientId = await getOidcClientId( - this.props.serverConfig.delegatedAuthentication, - SdkConfig.get().brand, - window.location.origin, - SdkConfig.get().oidc_static_clients, - ); - - const flow = { - type: "oidcNativeFlow", - clientId, - }; - - if (!this.isSupportedFlow(flow)) { - throw new Error("OIDC native login flow configured but UI not yet supported."); - } - - this.setState({ - busy: false, - flows: [flow], - }); - return true; - } catch (error) { - logger.error(error); - return false; - } - } - private async initLoginLogic({ hsUrl, isUrl }: ValidatedServerConfig): Promise { let isDefaultServer = false; if ( @@ -412,16 +372,11 @@ export default class LoginComponent extends React.PureComponent await this.checkServerLiveliness({ hsUrl, isUrl }); - const useOidcNativeFlow = await this.tryInitOidcNativeFlow(); - - // don't continue with setup - // as we are using oidc native flow - if (useOidcNativeFlow) { - return; - } - const loginLogic = new Login(hsUrl, isUrl, fallbackHsUrl, { defaultDeviceDisplayName: this.props.defaultDeviceDisplayName, + delegatedAuthentication: this.oidcNativeFlowEnabled + ? this.props.serverConfig.delegatedAuthentication + : undefined, }); this.loginLogic = loginLogic; diff --git a/test/components/structures/auth/Login-test.tsx b/test/components/structures/auth/Login-test.tsx index 81294fb1d4d..9ae8d09db1e 100644 --- a/test/components/structures/auth/Login-test.tsx +++ b/test/components/structures/auth/Login-test.tsx @@ -408,20 +408,52 @@ describe("Login", function () { expect(screen.getByLabelText("Username")).toBeInTheDocument(); }); - it("should fallback to normal login while oidc native login is not supported in UI", async () => { + // 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 () => { fetchMock.post(delegatedAuth.registrationEndpoint, { client_id: "abc123" }); getComponent(hsUrl, isUrl, delegatedAuth); await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); - expect(logger.error).toHaveBeenCalledWith( - new Error("OIDC native login flow configured but UI not yet supported."), - ); + // did not continue with matrix login + expect(mockClient.loginFlows).not.toHaveBeenCalled(); + // no oidc native UI yet + expect( + screen.getByText("This homeserver doesn't offer any login flows which are supported by this client."), + ).toBeInTheDocument(); + }); + + /** + * Oidc-aware flows still work while the oidc-native feature flag is disabled + */ + it("should show oidc-aware flow for oidc-enabled homeserver when oidc native flow setting is disabled", async () => { + jest.spyOn(SettingsStore, "getValue").mockReturnValue(false); + mockClient.loginFlows.mockResolvedValue({ + flows: [ + { + type: "m.login.sso", + [DELEGATED_OIDC_COMPATIBILITY.name]: true, + }, + { + type: "m.login.password", + }, + ], + }); + + const { container } = getComponent(hsUrl, isUrl, delegatedAuth); + + 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 - expect(screen.getByLabelText("Username")).toBeInTheDocument(); + // oidc-aware 'continue' button displayed + const ssoButtons = container.querySelectorAll(".mx_SSOButton"); + expect(ssoButtons.length).toBe(1); + expect(ssoButtons[0].textContent).toBe("Continue"); + // no password form visible + expect(container.querySelector("form")).toBeFalsy(); }); }); }); From 1a53ad084cdb3d8c20c54c1816281e4fd8979f13 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Tue, 13 Jun 2023 11:25:28 +1200 Subject: [PATCH 10/61] strict fixes --- src/components/structures/auth/Login.tsx | 2 +- src/utils/oidc/registerClient.ts | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/components/structures/auth/Login.tsx b/src/components/structures/auth/Login.tsx index c6095294e55..14801a93936 100644 --- a/src/components/structures/auth/Login.tsx +++ b/src/components/structures/auth/Login.tsx @@ -348,7 +348,7 @@ export default class LoginComponent extends React.PureComponent } catch (e) { this.setState({ busy: false, - ...AutoDiscoveryUtils.authComponentStateForError(e), + ...AutoDiscoveryUtils.authComponentStateForError(e as Error), }); } } diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 9279d043053..ac7136fb53a 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -67,13 +67,13 @@ const doRegistration = async ( const body = await response.json(); const clientId = body["client_id"]; - if (!clientId) { + if (!clientId || typeof clientId !== "string") { throw new Error(OidcClientError.DynamicRegistrationInvalid); } return clientId; } catch (error) { - if (Object.values(OidcClientError).includes(error.message)) { + if (Object.values(OidcClientError).includes((error as Error).message as OidcClientError)) { throw error; } else { logger.error("Dynamic registration request failed", error); @@ -100,6 +100,9 @@ const registerOidcClient = async ( clientUri: baseUrl, redirectUris: [baseUrl], }; + if (!delegatedAuthConfig.registrationEndpoint) { + throw new Error(OidcClientError.DynamicRegistrationNotSupported); + } const clientId = await doRegistration(delegatedAuthConfig.registrationEndpoint, clientMetadata); return clientId; @@ -137,8 +140,5 @@ export const getOidcClientId = async ( logger.debug(`Using static clientId for issuer ${delegatedAuthConfig.issuer}`); return staticClientId; } - if (!delegatedAuthConfig.registrationEndpoint) { - throw new Error(OidcClientError.DynamicRegistrationNotSupported); - } return await registerOidcClient(delegatedAuthConfig, clientName, baseUrl); }; From f9aaa28b88ff667f901d51ec4787c55dd0a63b7c Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Tue, 13 Jun 2023 11:29:40 +1200 Subject: [PATCH 11/61] remove unused code --- src/utils/oidc/error.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/utils/oidc/error.ts b/src/utils/oidc/error.ts index ea5fa46c1d3..35e7ba23c27 100644 --- a/src/utils/oidc/error.ts +++ b/src/utils/oidc/error.ts @@ -21,5 +21,3 @@ export enum OidcClientError { DynamicRegistrationFailed = "Dynamic registration failed", DynamicRegistrationInvalid = "Dynamic registration invalid response", } - -export type OidcError = OidcClientError | OidcDiscoveryError; From 9faa9c874c8a2e9d817cf4b37161b1b1105d8494 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Tue, 13 Jun 2023 11:29:54 +1200 Subject: [PATCH 12/61] and imports --- src/utils/oidc/error.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/utils/oidc/error.ts b/src/utils/oidc/error.ts index 35e7ba23c27..3604ee77c24 100644 --- a/src/utils/oidc/error.ts +++ b/src/utils/oidc/error.ts @@ -14,8 +14,6 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { OidcDiscoveryError } from "matrix-js-sdk/src/oidc/validate"; - export enum OidcClientError { DynamicRegistrationNotSupported = "Dynamic registration not supported", DynamicRegistrationFailed = "Dynamic registration failed", From e92022990d09c8660e6d924a3f510c75e37b3ba4 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 14 Jun 2023 10:36:09 +1200 Subject: [PATCH 13/61] comments --- src/utils/oidc/registerClient.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index ac7136fb53a..8daa2c22002 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -29,10 +29,10 @@ export type OidcRegistrationClientMetadata = { /** * Make the registration request - * @param issuer issue URL + * @param registrationEndpoint URL as configured on ValidatedServerConfig * @param clientMetadata registration metadata * @returns {Promise} resolves to the registered client id when registration is successful - * @throws when registration fails, or issue does not support dynamic registration + * @throws when registration request fails, or response is invalid */ const doRegistration = async ( registrationEndpoint: string, From 5389d5c74c51c8bd14c2be17332e0e763c99a6c3 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 14 Jun 2023 10:37:03 +1200 Subject: [PATCH 14/61] comments 2 --- src/utils/oidc/registerClient.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 8daa2c22002..7dc3998dc3d 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -28,7 +28,7 @@ export type OidcRegistrationClientMetadata = { }; /** - * Make the registration request + * Make the client registration request * @param registrationEndpoint URL as configured on ValidatedServerConfig * @param clientMetadata registration metadata * @returns {Promise} resolves to the registered client id when registration is successful @@ -88,7 +88,7 @@ const doRegistration = async ( * @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/' * @returns Promise resolved with registered clientId - * @throws on failed request or invalid response + * @throws when registration is not supported, on failed request or invalid response */ const registerOidcClient = async ( delegatedAuthConfig: IDelegatedAuthConfig & ValidatedIssuerConfig, @@ -109,7 +109,7 @@ const registerOidcClient = async ( }; /** - * Get the configured clientId for the issuer + * Get the statically configured clientId for the issuer * @param issuer delegated auth OIDC issuer * @param staticOidcClients static client config from config.json * @returns clientId if found, otherwise undefined @@ -119,7 +119,7 @@ const getStaticOidcClientId = (issuer: string, staticOidcClients?: Record Date: Wed, 14 Jun 2023 10:47:37 +1200 Subject: [PATCH 15/61] util functions to get static client id --- src/IConfigOptions.ts | 8 +++ src/utils/oidc/error.ts | 21 +++++++ src/utils/oidc/registerClient.ts | 66 ++++++++++++++++++++ test/utils/oidc/registerClient-test.ts | 86 ++++++++++++++++++++++++++ 4 files changed, 181 insertions(+) create mode 100644 src/utils/oidc/error.ts create mode 100644 src/utils/oidc/registerClient.ts create mode 100644 test/utils/oidc/registerClient-test.ts diff --git a/src/IConfigOptions.ts b/src/IConfigOptions.ts index 1fa873ec87a..bf8ca1ee866 100644 --- a/src/IConfigOptions.ts +++ b/src/IConfigOptions.ts @@ -192,6 +192,14 @@ export interface IConfigOptions { existing_issues_url: string; new_issue_url: string; }; + + /** + * Configuration for OIDC issuers where a static client_id has been issued for the app. + * Otherwise dynamic client registration is attempted. + * The issuer URL must have a trailing `/`. + * OPTIONAL + */ + oidc_static_clients?: Record; } export interface ISsoRedirectOptions { diff --git a/src/utils/oidc/error.ts b/src/utils/oidc/error.ts new file mode 100644 index 00000000000..3604ee77c24 --- /dev/null +++ b/src/utils/oidc/error.ts @@ -0,0 +1,21 @@ +/* +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. +*/ + +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 new file mode 100644 index 00000000000..425a62bcc23 --- /dev/null +++ b/src/utils/oidc/registerClient.ts @@ -0,0 +1,66 @@ +/* +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. +*/ + +import { IDelegatedAuthConfig } from "matrix-js-sdk/src/client"; +import { logger } from "matrix-js-sdk/src/logger"; +import { ValidatedIssuerConfig } from "matrix-js-sdk/src/oidc/validate"; + +import { OidcClientError } from "./error"; + +export type OidcRegistrationClientMetadata = { + clientName: string; + clientUri: string; + redirectUris: string[]; +}; + +/** + * Get the statically configured clientId for the issuer + * @param issuer delegated auth OIDC issuer + * @param staticOidcClients static client config from config.json + * @returns clientId if found, otherwise undefined + */ +const getStaticOidcClientId = (issuer: string, staticOidcClients?: Record): string | undefined => { + return staticOidcClients?.[issuer]; +}; + +/** + * Get the clientId for an OIDC OP + * 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 or registration failed + */ +export const getOidcClientId = async ( + delegatedAuthConfig: IDelegatedAuthConfig & ValidatedIssuerConfig, + // these are used in the following PR + _clientName: string, + _baseUrl: string, + staticOidcClients?: Record, +): Promise => { + const staticClientId = getStaticOidcClientId(delegatedAuthConfig.issuer, staticOidcClients); + if (staticClientId) { + 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); +}; diff --git a/test/utils/oidc/registerClient-test.ts b/test/utils/oidc/registerClient-test.ts new file mode 100644 index 00000000000..06f67671d06 --- /dev/null +++ b/test/utils/oidc/registerClient-test.ts @@ -0,0 +1,86 @@ +/* +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. +*/ + +import fetchMockJest from "fetch-mock-jest"; + +import { OidcClientError } from "../../../src/utils/oidc/error"; +import { getOidcClientId } from "../../../src/utils/oidc/registerClient"; + +describe("getOidcClientId()", () => { + const issuer = "https://auth.com/"; + const registrationEndpoint = "https://auth.com/register"; + const clientName = "Element"; + const baseUrl = "https://just.testing"; + const dynamicClientId = "xyz789"; + const staticOidcClients = { + [issuer]: "abc123", + }; + const delegatedAuthConfig = { + issuer, + registrationEndpoint, + authorizationEndpoint: issuer + "auth", + tokenEndpoint: issuer + "token", + }; + + beforeEach(() => { + fetchMockJest.mockClear(); + fetchMockJest.resetBehavior(); + }); + + it("should return static clientId when configured", async () => { + expect(await getOidcClientId(delegatedAuthConfig, clientName, baseUrl, staticOidcClients)).toEqual( + staticOidcClients[issuer], + ); + // didn't try to register + expect(fetchMockJest).toHaveFetchedTimes(0); + }); + + it("should throw when no static clientId is configured and no registration endpoint", async () => { + const authConfigWithoutRegistration = { + ...delegatedAuthConfig, + issuer: "https://issuerWithoutStaticClientId.org/", + registrationEndpoint: undefined, + }; + expect( + async () => await getOidcClientId(authConfigWithoutRegistration, clientName, baseUrl, staticOidcClients), + ).rejects.toThrow(OidcClientError.DynamicRegistrationNotSupported); + // didn't try to register + expect(fetchMockJest).toHaveFetchedTimes(0); + }); + + it("should handle when staticOidcClients object is falsy", async () => { + const authConfigWithoutRegistration = { + ...delegatedAuthConfig, + registrationEndpoint: undefined, + }; + expect(async () => await getOidcClientId(authConfigWithoutRegistration, clientName, baseUrl)).rejects.toThrow( + OidcClientError.DynamicRegistrationNotSupported, + ); + // didn't try to register + expect(fetchMockJest).toHaveFetchedTimes(0); + }); + + it("should throw while dynamic registration is not implemented", async () => { + fetchMockJest.post(registrationEndpoint, { + status: 200, + body: JSON.stringify({ client_id: dynamicClientId }), + }); + + expect(async () => await getOidcClientId(delegatedAuthConfig, clientName, baseUrl)).rejects.toThrow( + OidcClientError.DynamicRegistrationNotSupported, + ); + }); +}); From 9c89f414675168ebe56596fc0a0246053fb4ed5c Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 14 Jun 2023 10:55:10 +1200 Subject: [PATCH 16/61] check static client ids in login flow --- src/Login.ts | 64 +++++++- src/components/structures/auth/Login.tsx | 73 +++++---- src/utils/oidc/registerClient.ts | 3 +- .../components/structures/auth/Login-test.tsx | 140 +++++++++++++++++- test/test-utils/test-utils.ts | 7 +- 5 files changed, 247 insertions(+), 40 deletions(-) diff --git a/src/Login.ts b/src/Login.ts index 569363eeb05..6426550eaa6 100644 --- a/src/Login.ts +++ b/src/Login.ts @@ -19,18 +19,32 @@ limitations under the License. import { createClient } from "matrix-js-sdk/src/matrix"; import { MatrixClient } from "matrix-js-sdk/src/client"; import { logger } from "matrix-js-sdk/src/logger"; -import { DELEGATED_OIDC_COMPATIBILITY, ILoginParams, LoginFlow } from "matrix-js-sdk/src/@types/auth"; +import { OidcDiscoveryError } from "matrix-js-sdk/src/oidc/validate"; +import { DELEGATED_OIDC_COMPATIBILITY, ILoginFlow, ILoginParams, LoginFlow } from "matrix-js-sdk/src/@types/auth"; import { IMatrixClientCreds } from "./MatrixClientPeg"; import SecurityCustomisations from "./customisations/Security"; +import { ValidatedServerConfig } from "./utils/ValidatedServerConfig"; +import { getOidcClientId } from "./utils/oidc/registerClient"; +import { IConfigOptions } from "./IConfigOptions"; +import SdkConfig from "./SdkConfig"; + +export type ClientLoginFlow = LoginFlow | OidcNativeFlow; interface ILoginOptions { defaultDeviceDisplayName?: string; + /** + * Delegated auth config from server config + * When prop is passed we will attempt to use delegated auth + * Labs flag should be checked in parent + */ + delegatedAuthentication?: ValidatedServerConfig["delegatedAuthentication"]; } export default class Login { - private flows: Array = []; + private flows: Array = []; private readonly defaultDeviceDisplayName?: string; + private readonly delegatedAuthentication?: ValidatedServerConfig["delegatedAuthentication"]; private tempClient: MatrixClient | null = null; // memoize public constructor( @@ -40,6 +54,7 @@ export default class Login { opts: ILoginOptions, ) { this.defaultDeviceDisplayName = opts.defaultDeviceDisplayName; + this.delegatedAuthentication = opts.delegatedAuthentication; } public getHomeserverUrl(): string { @@ -75,7 +90,20 @@ export default class Login { return this.tempClient; } - public async getFlows(): Promise> { + public async getFlows(): Promise> { + // try to use oidc native flow + try { + const oidcFlow = await tryInitOidcNativeFlow( + this.delegatedAuthentication, + SdkConfig.get().brand, + SdkConfig.get().oidc_static_clients, + ); + return [oidcFlow]; + } catch (error) { + logger.error(error); + } + + // oidc native flow not supported, continue with matrix login const client = this.createTemporaryClient(); const { flows }: { flows: LoginFlow[] } = await client.loginFlows(); // If an m.login.sso flow is present which is also flagged as being for MSC3824 OIDC compatibility then we only @@ -151,6 +179,36 @@ export default class Login { } } +export interface OidcNativeFlow extends ILoginFlow { + type: "oidcNativeFlow"; + clientId: string; +} +/** + * Finds static clientId for configured issuer, or attempts dynamic registration with the OP + * @param delegatedAuthConfig Auth config from ValidatedServerConfig + * @param clientName Client name to register with the OP, eg 'Element', used during client registration with OP + * @param staticOidcClients static client config from config.json, used during client registration with OP + * @returns Promise when oidc native authentication flow is supported and correctly configured + * @throws when oidc native flow is not configured, client can't register with OP, or any unexpected error + */ +const tryInitOidcNativeFlow = async ( + delegatedAuthConfig: ValidatedServerConfig["delegatedAuthentication"] | undefined, + brand: string, + oidcStaticClients?: IConfigOptions["oidc_static_clients"], +): Promise => { + if (!delegatedAuthConfig) { + throw new Error(OidcDiscoveryError.NotSupported); + } + const clientId = await getOidcClientId(delegatedAuthConfig, brand, window.location.origin, oidcStaticClients); + + const flow = { + type: "oidcNativeFlow", + clientId, + } as OidcNativeFlow; + + return flow; +}; + /** * Send a login request to the given server, and format the response * as a MatrixClientCreds diff --git a/src/components/structures/auth/Login.tsx b/src/components/structures/auth/Login.tsx index 3299cae3fc3..14801a93936 100644 --- a/src/components/structures/auth/Login.tsx +++ b/src/components/structures/auth/Login.tsx @@ -17,10 +17,10 @@ limitations under the License. import React, { ReactNode } from "react"; import classNames from "classnames"; import { logger } from "matrix-js-sdk/src/logger"; -import { ISSOFlow, LoginFlow, SSOAction } from "matrix-js-sdk/src/@types/auth"; +import { ISSOFlow, SSOAction } from "matrix-js-sdk/src/@types/auth"; import { _t, _td, UserFriendlyError } from "../../../languageHandler"; -import Login from "../../../Login"; +import Login, { ClientLoginFlow } from "../../../Login"; import { messageForConnectionError, messageForLoginError } from "../../../utils/ErrorUtils"; import AutoDiscoveryUtils from "../../../utils/AutoDiscoveryUtils"; import AuthPage from "../../views/auth/AuthPage"; @@ -38,6 +38,7 @@ import AuthHeader from "../../views/auth/AuthHeader"; import AccessibleButton, { ButtonEvent } from "../../views/elements/AccessibleButton"; import { ValidatedServerConfig } from "../../../utils/ValidatedServerConfig"; import { filterBoolean } from "../../../utils/arrays"; +import { Features } from "../../../settings/Settings"; // These are used in several places, and come from the js-sdk's autodiscovery // stuff. We define them here so that they'll be picked up by i18n. @@ -49,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. @@ -84,7 +84,7 @@ interface IState { // can we attempt to log in or are there validation errors? canTryLogin: boolean; - flows?: LoginFlow[]; + flows?: ClientLoginFlow[]; // used for preserving form values when changing homeserver username: string; @@ -110,6 +110,7 @@ type OnPasswordLogin = { */ export default class LoginComponent extends React.PureComponent { private unmounted = false; + private oidcNativeFlowEnabled = false; private loginLogic!: Login; private readonly stepRendererMap: Record ReactNode>; @@ -117,6 +118,9 @@ export default class LoginComponent extends React.PureComponent public constructor(props: IProps) { super(props); + // only set on a config level, so we don't need to watch + this.oidcNativeFlowEnabled = SettingsStore.getValue(Features.OidcNativeFlow); + this.state = { busy: false, errorText: null, @@ -156,7 +160,8 @@ export default class LoginComponent extends React.PureComponent public componentDidUpdate(prevProps: IProps): void { if ( prevProps.serverConfig.hsUrl !== this.props.serverConfig.hsUrl || - prevProps.serverConfig.isUrl !== this.props.serverConfig.isUrl + prevProps.serverConfig.isUrl !== this.props.serverConfig.isUrl || + prevProps.serverConfig.delegatedAuthentication !== this.props.serverConfig.delegatedAuthentication ) { // Ensure that we end up actually logging in to the right place this.initLoginLogic(this.props.serverConfig); @@ -322,28 +327,10 @@ export default class LoginComponent extends React.PureComponent } }; - private async initLoginLogic({ hsUrl, isUrl }: ValidatedServerConfig): Promise { - let isDefaultServer = false; - if ( - this.props.serverConfig.isDefault && - hsUrl === this.props.serverConfig.hsUrl && - isUrl === this.props.serverConfig.isUrl - ) { - isDefaultServer = true; - } - - const fallbackHsUrl = isDefaultServer ? this.props.fallbackHsUrl! : null; - - const loginLogic = new Login(hsUrl, isUrl, fallbackHsUrl, { - defaultDeviceDisplayName: this.props.defaultDeviceDisplayName, - }); - this.loginLogic = loginLogic; - - this.setState({ - busy: true, - loginIncorrect: false, - }); - + private async checkServerLiveliness({ + hsUrl, + isUrl, + }: Pick): Promise { // Do a quick liveliness check on the URLs try { const { warning } = await AutoDiscoveryUtils.validateServerConfigWithStaticUrls(hsUrl, isUrl); @@ -361,9 +348,37 @@ export default class LoginComponent extends React.PureComponent } catch (e) { this.setState({ busy: false, - ...AutoDiscoveryUtils.authComponentStateForError(e), + ...AutoDiscoveryUtils.authComponentStateForError(e as Error), }); } + } + + private async initLoginLogic({ hsUrl, isUrl }: ValidatedServerConfig): Promise { + let isDefaultServer = false; + if ( + this.props.serverConfig.isDefault && + hsUrl === this.props.serverConfig.hsUrl && + isUrl === this.props.serverConfig.isUrl + ) { + isDefaultServer = true; + } + + const fallbackHsUrl = isDefaultServer ? this.props.fallbackHsUrl! : null; + + this.setState({ + busy: true, + loginIncorrect: false, + }); + + await this.checkServerLiveliness({ hsUrl, isUrl }); + + const loginLogic = new Login(hsUrl, isUrl, fallbackHsUrl, { + defaultDeviceDisplayName: this.props.defaultDeviceDisplayName, + delegatedAuthentication: this.oidcNativeFlowEnabled + ? this.props.serverConfig.delegatedAuthentication + : undefined, + }); + this.loginLogic = loginLogic; loginLogic .getFlows() @@ -401,7 +416,7 @@ export default class LoginComponent extends React.PureComponent }); } - private isSupportedFlow = (flow: LoginFlow): boolean => { + private isSupportedFlow = (flow: ClientLoginFlow): boolean => { // technically the flow can have multiple steps, but no one does this // for login and loginLogic doesn't support it so we can ignore it. if (!this.stepRendererMap[flow.type]) { diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 425a62bcc23..dfb07dfff43 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -39,13 +39,12 @@ const getStaticOidcClientId = (issuer: string, staticOidcClients?: Record resolves with clientId - * @throws if no clientId is found or registration failed + * @throws if no clientId is found */ export const getOidcClientId = async ( delegatedAuthConfig: IDelegatedAuthConfig & ValidatedIssuerConfig, diff --git a/test/components/structures/auth/Login-test.tsx b/test/components/structures/auth/Login-test.tsx index 8eb25d49097..bb59a24121e 100644 --- a/test/components/structures/auth/Login-test.tsx +++ b/test/components/structures/auth/Login-test.tsx @@ -17,19 +17,29 @@ limitations under the License. import React from "react"; import { fireEvent, render, screen, waitForElementToBeRemoved } from "@testing-library/react"; import { mocked, MockedObject } from "jest-mock"; -import { createClient, MatrixClient } from "matrix-js-sdk/src/matrix"; 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 SdkConfig from "../../../../src/SdkConfig"; import { mkServerConfig, mockPlatformPeg, unmockPlatformPeg } from "../../../test-utils"; import Login from "../../../../src/components/structures/auth/Login"; import BasePlatform from "../../../../src/BasePlatform"; +import SettingsStore from "../../../../src/settings/SettingsStore"; +import { Features } from "../../../../src/settings/Settings"; +import { ValidatedServerConfig } 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"); jest.useRealTimers(); +const oidcStaticClientsConfig = { + "https://staticallyregisteredissuer.org/": "static-clientId-123", +}; + describe("Login", function () { let platform: MockedObject; @@ -42,6 +52,7 @@ describe("Login", function () { SdkConfig.put({ brand: "test-brand", disable_custom_urls: true, + oidc_static_clients: oidcStaticClientsConfig, }); mockClient.login.mockClear().mockResolvedValue({}); mockClient.loginFlows.mockClear().mockResolvedValue({ flows: [{ type: "m.login.password" }] }); @@ -51,6 +62,7 @@ describe("Login", function () { return mockClient; }); fetchMock.resetBehavior(); + fetchMock.resetHistory(); fetchMock.get("https://matrix.org/_matrix/client/versions", { unstable_features: {}, versions: [], @@ -66,10 +78,14 @@ describe("Login", function () { unmockPlatformPeg(); }); - function getRawComponent(hsUrl = "https://matrix.org", isUrl = "https://vector.im") { + function getRawComponent( + hsUrl = "https://matrix.org", + isUrl = "https://vector.im", + delegatedAuthentication?: ValidatedServerConfig["delegatedAuthentication"], + ) { return ( {}} onRegisterClick={() => {}} onServerConfigChange={() => {}} @@ -77,8 +93,12 @@ describe("Login", function () { ); } - function getComponent(hsUrl?: string, isUrl?: string) { - return render(getRawComponent(hsUrl, isUrl)); + function getComponent( + hsUrl?: string, + isUrl?: string, + delegatedAuthentication?: ValidatedServerConfig["delegatedAuthentication"], + ) { + return render(getRawComponent(hsUrl, isUrl, delegatedAuthentication)); } it("should show form with change server link", async () => { @@ -190,6 +210,7 @@ describe("Login", function () { versions: [], }); rerender(getRawComponent("https://server2")); + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); fireEvent.click(container.querySelector(".mx_SSOButton")!); expect(platform.startSingleSignOn.mock.calls[1][0].baseUrl).toBe("https://server2"); @@ -319,4 +340,113 @@ describe("Login", function () { // error cleared expect(screen.queryByText("Your test-brand is misconfigured")).not.toBeInTheDocument(); }); + + describe("OIDC native flow", () => { + const hsUrl = "https://matrix.org"; + const isUrl = "https://vector.im"; + const issuer = "https://test.com/"; + const delegatedAuth = { + issuer, + registrationEndpoint: issuer + "register", + tokenEndpoint: issuer + "token", + authorizationEndpoint: issuer + "authorization", + }; + beforeEach(() => { + jest.spyOn(logger, "error").mockClear(); + jest.spyOn(SettingsStore, "getValue").mockImplementation( + (settingName) => settingName === Features.OidcNativeFlow, + ); + }); + + it("should not attempt registration when oidc native flow setting is disabled", async () => { + jest.spyOn(SettingsStore, "getValue").mockReturnValue(false); + + getComponent(hsUrl, isUrl, delegatedAuth); + + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + + // continued with normal setup + expect(mockClient.loginFlows).toHaveBeenCalled(); + // normal password login rendered + expect(screen.getByLabelText("Username")).toBeInTheDocument(); + }); + + it("should attempt to register oidc client", async () => { + // dont mock, spy so we can check config values were correctly passed + jest.spyOn(registerClientUtils, "getOidcClientId"); + getComponent(hsUrl, isUrl, delegatedAuth); + + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + + // called with values from config + expect(registerClientUtils.getOidcClientId).toHaveBeenCalledWith( + delegatedAuth, + "test-brand", + "http://localhost", + oidcStaticClientsConfig, + ); + }); + + it("should fallback to normal login when client does not have static clientId", async () => { + getComponent(hsUrl, isUrl, delegatedAuth); + + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + + expect(logger.error).toHaveBeenCalledWith(new Error(OidcClientError.DynamicRegistrationNotSupported)); + + // continued with normal setup + expect(mockClient.loginFlows).toHaveBeenCalled(); + // normal password login rendered + expect(screen.getByLabelText("Username")).toBeInTheDocument(); + }); + + // 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); + + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + + // did not continue with matrix login + expect(mockClient.loginFlows).not.toHaveBeenCalled(); + // no oidc native UI yet + expect( + screen.getByText("This homeserver doesn't offer any login flows which are supported by this client."), + ).toBeInTheDocument(); + }); + + /** + * Oidc-aware flows still work while the oidc-native feature flag is disabled + */ + it("should show oidc-aware flow for oidc-enabled homeserver when oidc native flow setting is disabled", async () => { + jest.spyOn(SettingsStore, "getValue").mockReturnValue(false); + mockClient.loginFlows.mockResolvedValue({ + flows: [ + { + type: "m.login.sso", + [DELEGATED_OIDC_COMPATIBILITY.name]: true, + }, + { + type: "m.login.password", + }, + ], + }); + + const { container } = getComponent(hsUrl, isUrl, delegatedAuth); + + await waitForElementToBeRemoved(() => screen.queryAllByLabelText("Loading…")); + + // continued with normal setup + expect(mockClient.loginFlows).toHaveBeenCalled(); + // oidc-aware 'continue' button displayed + const ssoButtons = container.querySelectorAll(".mx_SSOButton"); + expect(ssoButtons.length).toBe(1); + expect(ssoButtons[0].textContent).toBe("Continue"); + // no password form visible + expect(container.querySelector("form")).toBeFalsy(); + }); + }); }); diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts index 6eea4114d23..a2ca4313407 100644 --- a/test/test-utils/test-utils.ts +++ b/test/test-utils/test-utils.ts @@ -617,12 +617,17 @@ export function mkStubRoom( } as unknown as Room; } -export function mkServerConfig(hsUrl: string, isUrl: string): ValidatedServerConfig { +export function mkServerConfig( + hsUrl: string, + isUrl: string, + delegatedAuthentication?: ValidatedServerConfig["delegatedAuthentication"], +): ValidatedServerConfig { return { hsUrl, hsName: "TEST_ENVIRONMENT", hsNameIsDifferent: false, // yes, we lie isUrl, + delegatedAuthentication, } as ValidatedServerConfig; } From 422823e782ba388c370d5046c0a63c9fcd359dc0 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 14 Jun 2023 14:29:01 +1200 Subject: [PATCH 17/61] remove dead code --- src/utils/oidc/registerClient.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index dfb07dfff43..6cef00488da 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -20,12 +20,6 @@ import { ValidatedIssuerConfig } from "matrix-js-sdk/src/oidc/validate"; import { OidcClientError } from "./error"; -export type OidcRegistrationClientMetadata = { - clientName: string; - clientUri: string; - redirectUris: string[]; -}; - /** * Get the statically configured clientId for the issuer * @param issuer delegated auth OIDC issuer From a76cde9d33c4636304d69c1b58398864648d812d Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 14 Jun 2023 14:30:35 +1200 Subject: [PATCH 18/61] OidcRegistrationClientMetadata type --- src/utils/oidc/registerClient.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 0625c6cbedc..7dc3998dc3d 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -21,6 +21,12 @@ import { ValidatedIssuerConfig } from "matrix-js-sdk/src/oidc/validate"; import { OidcClientError } from "./error"; +export type OidcRegistrationClientMetadata = { + clientName: string; + clientUri: string; + redirectUris: string[]; +}; + /** * Make the client registration request * @param registrationEndpoint URL as configured on ValidatedServerConfig From bf45be2a1836a9b9810e0878198a5a01e7436fee Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 14 Jun 2023 17:04:32 +1200 Subject: [PATCH 19/61] navigate to oidc authorize url --- res/css/structures/auth/_Login.pcss | 5 + src/Lifecycle.ts | 1 + src/components/structures/auth/Login.tsx | 21 +++- src/utils/oidc/authorize.ts | 121 +++++++++++++++++++++++ src/utils/oidc/registerClient.ts | 5 +- 5 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 src/utils/oidc/authorize.ts diff --git a/res/css/structures/auth/_Login.pcss b/res/css/structures/auth/_Login.pcss index 2eba8cf3d14..eeca1e8e494 100644 --- a/res/css/structures/auth/_Login.pcss +++ b/res/css/structures/auth/_Login.pcss @@ -99,3 +99,8 @@ div.mx_AccessibleButton_kind_link.mx_Login_forgot { align-content: center; padding: 14px; } + +.mx_Login_fullWidthButton { + width: 100%; + margin-bottom: 16px; +} diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index 6f49a0a0b61..bf9293e4df6 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -196,6 +196,7 @@ export function attemptTokenLogin( defaultDeviceDisplayName?: string, fragmentAfterLogin?: string, ): Promise { + debugger; if (!queryParams.loginToken) { return Promise.resolve(false); } diff --git a/src/components/structures/auth/Login.tsx b/src/components/structures/auth/Login.tsx index 14801a93936..a39b79988bb 100644 --- a/src/components/structures/auth/Login.tsx +++ b/src/components/structures/auth/Login.tsx @@ -20,7 +20,7 @@ import { logger } from "matrix-js-sdk/src/logger"; import { ISSOFlow, SSOAction } from "matrix-js-sdk/src/@types/auth"; import { _t, _td, UserFriendlyError } from "../../../languageHandler"; -import Login, { ClientLoginFlow } from "../../../Login"; +import Login, { ClientLoginFlow, OidcNativeFlow } from "../../../Login"; import { messageForConnectionError, messageForLoginError } from "../../../utils/ErrorUtils"; import AutoDiscoveryUtils from "../../../utils/AutoDiscoveryUtils"; import AuthPage from "../../views/auth/AuthPage"; @@ -39,6 +39,7 @@ import AccessibleButton, { ButtonEvent } from "../../views/elements/AccessibleBu import { ValidatedServerConfig } from "../../../utils/ValidatedServerConfig"; import { filterBoolean } from "../../../utils/arrays"; import { Features } from "../../../settings/Settings"; +import { login, startOidcLogin } from "../../../utils/oidc/authorize"; // These are used in several places, and come from the js-sdk's autodiscovery // stuff. We define them here so that they'll be picked up by i18n. @@ -146,6 +147,7 @@ export default class LoginComponent extends React.PureComponent "m.login.cas": () => this.renderSsoStep("cas"), // eslint-disable-next-line @typescript-eslint/naming-convention "m.login.sso": () => this.renderSsoStep("sso"), + "oidcNativeFlow": () => this.renderOidcNativeStep(), }; } @@ -430,7 +432,7 @@ export default class LoginComponent extends React.PureComponent if (!this.state.flows) return null; // this is the ideal order we want to show the flows in - const order = ["m.login.password", "m.login.sso"]; + const order = ["oidcNativeFlow", "m.login.password", "m.login.sso"]; const flows = filterBoolean(order.map((type) => this.state.flows?.find((flow) => flow.type === type))); return ( @@ -463,6 +465,21 @@ export default class LoginComponent extends React.PureComponent ); }; + private renderOidcNativeStep = (): React.ReactNode => { + const flow = this.state.flows!.find((flow) => flow.type === "oidcNativeFlow")! as OidcNativeFlow; + return ( + { + await startOidcLogin(this.props.serverConfig.delegatedAuthentication!, flow.clientId, this.props.serverConfig.hsUrl); + }} + > + {_t("Continue")} + + ); + }; + private renderSsoStep = (loginType: "cas" | "sso"): JSX.Element => { const flow = this.state.flows?.find((flow) => flow.type === "m.login." + loginType) as ISSOFlow; diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts new file mode 100644 index 00000000000..aa0a6722c7a --- /dev/null +++ b/src/utils/oidc/authorize.ts @@ -0,0 +1,121 @@ +/* +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. +*/ + +import { logger } from "matrix-js-sdk/src/logger"; +import { randomString } from "matrix-js-sdk/src/randomstring"; +import { ValidatedServerConfig } from "../ValidatedServerConfig"; + +type AuthorizationParams = { + state: string; + scope: string; + redirectUri: string; + nonce?: string; + codeVerifier?: string; +}; + +const generateScope = (existingDeviceId?: string): string => { + const deviceId = existingDeviceId || randomString(10); + return `openid urn:matrix:org.matrix.msc2967.client:api:* urn:matrix:org.matrix.msc2967.client:device:${deviceId}`; +}; + +// https://www.rfc-editor.org/rfc/rfc7636 +const generateCodeChallenge = async (codeVerifier: string): Promise => { + if (!window.crypto || !window.crypto.subtle) { + // @TODO(kerrya) should this be allowed? configurable? + logger.warn("A secure context is required to generate code challenge. Using plain text code challenge"); + return codeVerifier; + } + const utf8 = new TextEncoder().encode(codeVerifier); + + const digest = await crypto.subtle.digest("SHA-256", utf8); + + return btoa(String.fromCharCode(...new Uint8Array(digest))) + .replace(/=/g, "") + .replace(/\+/g, "-") + .replace(/\//g, "_"); +}; + +const storeAuthorizationParams = async ( + { redirectUri, state, nonce, codeVerifier }: AuthorizationParams, + { issuer }: ValidatedServerConfig["delegatedAuthentication"], + clientId: string, + homeserver: string, +): Promise => { + window.sessionStorage.setItem(`oidc_${state}_nonce`, nonce); + window.sessionStorage.setItem(`oidc_${state}_redirectUri`, redirectUri); + window.sessionStorage.setItem(`oidc_${state}_codeVerifier`, codeVerifier); + window.sessionStorage.setItem(`oidc_${state}_clientId`, clientId); + window.sessionStorage.setItem(`oidc_${state}_issuer`, issuer); + window.sessionStorage.setItem(`oidc_${state}_homeserver`, homeserver); +}; + +const generateAuthorizationParams = ({ + deviceId, + redirectUri, +}: { + deviceId?: string; + redirectUri: string; +}): AuthorizationParams => ({ + scope: generateScope(deviceId), + redirectUri, + state: randomString(8), + nonce: randomString(8), + codeVerifier: randomString(64), // https://tools.ietf.org/html/rfc7636#section-4.1 length needs to be 43-128 characters +}); + +const generateAuthorizationUrl = async ( + authorizationUrl: string, + clientId: string, + { scope, redirectUri, state, nonce, codeVerifier }: AuthorizationParams, +): Promise => { + const url = new URL(authorizationUrl); + url.searchParams.append("response_mode", "fragment"); + url.searchParams.append("response_type", "code"); + url.searchParams.append("redirect_uri", redirectUri); + url.searchParams.append("client_id", clientId); + url.searchParams.append("state", state); + url.searchParams.append("scope", scope); + if (nonce) { + url.searchParams.append("nonce", nonce); + } + + if (codeVerifier) { + url.searchParams.append("code_challenge_method", "S256"); + url.searchParams.append("code_challenge", await generateCodeChallenge(codeVerifier)); + } + + return url.toString(); +}; + +export const startOidcLogin = async ( + delegatedAuthConfig: ValidatedServerConfig["delegatedAuthentication"], + clientId: string, + homeserver: string, +): Promise => { + // TODO(kerrya) afterloginfragment + const redirectUri = window.location.origin; + const authParams = generateAuthorizationParams({ redirectUri }); + + storeAuthorizationParams(authParams, delegatedAuthConfig, clientId, homeserver); + + const authorizationUrl = await generateAuthorizationUrl( + delegatedAuthConfig.authorizationEndpoint, + clientId, + authParams, + ); + + window.location.href = authorizationUrl; +}; diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 7dc3998dc3d..ab98c756211 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -115,7 +115,10 @@ const registerOidcClient = async ( * @returns clientId if found, otherwise undefined */ const getStaticOidcClientId = (issuer: string, staticOidcClients?: Record): string | undefined => { - return staticOidcClients?.[issuer]; + console.log('hhh getStaticClient', issuer, staticOidcClients); + // static_oidc_clients are configured with a trailing slash + const issuerWithTrailingSlash = issuer.endsWith('/') ? issuer : issuer + '/'; + return staticOidcClients?.[issuerWithTrailingSlash]; }; /** From 3b8938cf5b26812ce067aca840de834fa96c5d76 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 15 Jun 2023 18:11:11 +1200 Subject: [PATCH 20/61] exchange code for token --- src/Lifecycle.ts | 34 +++++- src/components/structures/MatrixChat.tsx | 4 +- src/components/structures/auth/Login.tsx | 8 +- src/utils/oidc/authorize.ts | 131 +++++++++++++++++++++-- src/utils/oidc/error.ts | 2 + src/utils/oidc/registerClient.ts | 3 +- 6 files changed, 169 insertions(+), 13 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index bf9293e4df6..5d9fdf2a7ff 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -64,6 +64,7 @@ import AbstractLocalStorageSettingsHandler from "./settings/handlers/AbstractLoc import { OverwriteLoginPayload } from "./dispatcher/payloads/OverwriteLoginPayload"; import { SdkContextClass } from "./contexts/SDKContext"; import { messageForLoginError } from "./utils/ErrorUtils"; +import { completeOidcLogin } from "./utils/oidc/authorize"; const HOMESERVER_URL_KEY = "mx_hs_url"; const ID_SERVER_URL_KEY = "mx_is_url"; @@ -181,6 +182,9 @@ export async function getStoredSessionOwner(): Promise<[string, boolean] | [null } /** + * If query string includes OIDC authorization code flow parameters attempt to login using oidc flow + * Else, we may be returning from SSO - attempt token login + * * @param {Object} queryParams string->string map of the * query-parameters extracted from the real query-string of the starting * URI. @@ -191,12 +195,40 @@ export async function getStoredSessionOwner(): Promise<[string, boolean] | [null * @returns {Promise} promise which resolves to true if we completed the token * login, else false */ +export async function attemptDelegatedAuthLogin( + queryParams: QueryDict, + defaultDeviceDisplayName?: string, + fragmentAfterLogin?: string, +): Promise { + if (queryParams.code && queryParams.state) { + try { + await completeOidcLogin(queryParams); + return true; + } catch (error) { + logger.error("Failed to login via OIDC", error) + return false; + } + } + + return attemptTokenLogin(queryParams, defaultDeviceDisplayName, fragmentAfterLogin); +} + +/** + * @param {QueryDict} queryParams string->string map of the + * query-parameters extracted from the real query-string of the starting + * URI. + * + * @param {string} defaultDeviceDisplayName + * @param {string} fragmentAfterLogin path to go to after a successful login, only used for "Try again" + * + * @returns {Promise} promise which resolves to true if we completed the token + * login, else false + */ export function attemptTokenLogin( queryParams: QueryDict, defaultDeviceDisplayName?: string, fragmentAfterLogin?: string, ): Promise { - debugger; if (!queryParams.loginToken) { return Promise.resolve(false); } diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index 6a03bc7aed3..fe3b3e83b8c 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -314,7 +314,7 @@ export default class MatrixChat extends React.PureComponent { // the first thing to do is to try the token params in the query-string // if the session isn't soft logged out (ie: is a clean session being logged in) if (!Lifecycle.isSoftLogout()) { - Lifecycle.attemptTokenLogin( + Lifecycle.attemptDelegatedAuthLogin( this.props.realQueryParams, this.props.defaultDeviceDisplayName, this.getFragmentAfterLogin(), @@ -324,6 +324,8 @@ export default class MatrixChat extends React.PureComponent { this.props.onTokenLoginCompleted(); } + // @TODO(kerrya) remove OIDC code and state from URL + if (loggedIn) { this.tokenLogin = true; diff --git a/src/components/structures/auth/Login.tsx b/src/components/structures/auth/Login.tsx index a39b79988bb..1f97d734ea1 100644 --- a/src/components/structures/auth/Login.tsx +++ b/src/components/structures/auth/Login.tsx @@ -39,7 +39,7 @@ import AccessibleButton, { ButtonEvent } from "../../views/elements/AccessibleBu import { ValidatedServerConfig } from "../../../utils/ValidatedServerConfig"; import { filterBoolean } from "../../../utils/arrays"; import { Features } from "../../../settings/Settings"; -import { login, startOidcLogin } from "../../../utils/oidc/authorize"; +import { startOidcLogin } from "../../../utils/oidc/authorize"; // These are used in several places, and come from the js-sdk's autodiscovery // stuff. We define them here so that they'll be picked up by i18n. @@ -472,7 +472,11 @@ export default class LoginComponent extends React.PureComponent className="mx_Login_fullWidthButton" kind="primary" onClick={async () => { - await startOidcLogin(this.props.serverConfig.delegatedAuthentication!, flow.clientId, this.props.serverConfig.hsUrl); + await startOidcLogin( + this.props.serverConfig.delegatedAuthentication!, + flow.clientId, + this.props.serverConfig.hsUrl, + ); }} > {_t("Continue")} diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index aa0a6722c7a..856beee6e60 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -14,17 +14,35 @@ See the License for the specific language governing permissions and limitations under the License. */ +import { Method } from "matrix-js-sdk/src/http-api"; import { logger } from "matrix-js-sdk/src/logger"; import { randomString } from "matrix-js-sdk/src/randomstring"; +import { QueryDict } from "matrix-js-sdk/src/utils"; + import { ValidatedServerConfig } from "../ValidatedServerConfig"; +import { OidcClientError } from "./error"; type AuthorizationParams = { state: string; scope: string; redirectUri: string; - nonce?: string; - codeVerifier?: string; + nonce: string; + codeVerifier: string; +}; + +type BearerToken = { + token_type: "Bearer"; + access_token: string; + refresh_token?: string; + expires_in?: number; + id_token?: string; }; +const isValidBearerToken = (t: any): t is BearerToken => + typeof t == "object" && + t["token_type"] === "Bearer" && + typeof t["access_token"] === "string" && + (!("refresh_token" in t) || typeof t["refresh_token"] === "string") && + (!("expires_in" in t) || typeof t["expires_in"] === "number"); const generateScope = (existingDeviceId?: string): string => { const deviceId = existingDeviceId || randomString(10); @@ -48,18 +66,41 @@ const generateCodeChallenge = async (codeVerifier: string): Promise => { .replace(/\//g, "_"); }; -const storeAuthorizationParams = async ( +const storeAuthorizationParams = ( { redirectUri, state, nonce, codeVerifier }: AuthorizationParams, - { issuer }: ValidatedServerConfig["delegatedAuthentication"], + delegatedAuthConfig: ValidatedServerConfig["delegatedAuthentication"], clientId: string, homeserver: string, -): Promise => { +): void => { window.sessionStorage.setItem(`oidc_${state}_nonce`, nonce); window.sessionStorage.setItem(`oidc_${state}_redirectUri`, redirectUri); window.sessionStorage.setItem(`oidc_${state}_codeVerifier`, codeVerifier); window.sessionStorage.setItem(`oidc_${state}_clientId`, clientId); - window.sessionStorage.setItem(`oidc_${state}_issuer`, issuer); window.sessionStorage.setItem(`oidc_${state}_homeserver`, homeserver); + window.sessionStorage.setItem(`oidc_${state}_delegatedAuthConfig`, JSON.stringify(delegatedAuthConfig)); +}; + +type StoredAuthorizationParams = Omit & { + delegatedAuthConfig: ValidatedServerConfig["delegatedAuthentication"]; + clientId: string; + homeserver: string; +}; +const retrieveAuthorizationParams = (state: string): StoredAuthorizationParams => { + const nonce = window.sessionStorage.getItem(`oidc_${state}_nonce`); + const redirectUri = window.sessionStorage.getItem(`oidc_${state}_redirectUri`); + const codeVerifier = window.sessionStorage.getItem(`oidc_${state}_codeVerifier`); + const clientId = window.sessionStorage.getItem(`oidc_${state}_clientId`); + const homeserver = window.sessionStorage.getItem(`oidc_${state}_homeserver`); + const delegatedAuthConfig = JSON.parse(window.sessionStorage.getItem(`oidc_${state}_delegatedAuthConfig`)); + + return { + nonce, + redirectUri, + codeVerifier, + clientId, + homeserver, + delegatedAuthConfig, + }; }; const generateAuthorizationParams = ({ @@ -82,7 +123,7 @@ const generateAuthorizationUrl = async ( { scope, redirectUri, state, nonce, codeVerifier }: AuthorizationParams, ): Promise => { const url = new URL(authorizationUrl); - url.searchParams.append("response_mode", "fragment"); + url.searchParams.append("response_mode", "query"); url.searchParams.append("response_type", "code"); url.searchParams.append("redirect_uri", redirectUri); url.searchParams.append("client_id", clientId); @@ -119,3 +160,79 @@ export const startOidcLogin = async ( window.location.href = authorizationUrl; }; + +/** + * Gets `code` and `state` query params + * + * @param queryParams + * @returns code and state + * @throws when code and state are not valid strings + */ +const getCodeAndStateFromQueryParams = (queryParams: QueryDict): { code: string; state: string } => { + const code = queryParams["code"]; + const state = queryParams["state"]; + + if (!code || typeof code !== "string" || !state || typeof state !== "string") { + throw new Error("TODO(kerrya) Invalid query parameters. `code` and `state` are required."); + } + return { code, state }; +}; + +/** + * Attempts to exchange authorization code for bearer token + * @param code authorization code as returned by OP during authorization + * @param { StoredAuthorizationParams } storedAuthorizationParams stored params from start of oidc login flow + * @returns {BearerToken} valid bearer token + * @throws when request fails, or returned token is invalid + */ +const completeAuthorizationCodeGrant = async ( + code: string, + { clientId, codeVerifier, redirectUri, delegatedAuthConfig }: StoredAuthorizationParams, +): Promise => { + const params = new URLSearchParams(); + params.append("grant_type", "authorization_code"); + params.append("client_id", clientId); + params.append("code_verifier", codeVerifier); + params.append("redirect_uri", redirectUri); + params.append("code", code); + const metadata = params.toString(); + + const headers = { "Content-Type": "application/x-www-form-urlencoded" }; + + const response = await fetch(delegatedAuthConfig.tokenEndpoint, { + method: Method.Post, + headers, + body: metadata, + }); + + if (response.status >= 400) { + throw new Error(OidcClientError.CodeExchangeFailed); + } + + const token = await response.json(); + + if (isValidBearerToken(token)) { + return token; + } + + throw new Error(OidcClientError.InvalidBearerToken); +}; + +/** + * Attempt to complete authorization code flow to login + * @param {QueryDict} queryParams the query-parameters extracted from the real query-string of the starting URI. + * @returns {Promise} Promise that resolves when login was successful + * @throws When login failed + */ +export const completeOidcLogin = async (queryParams: QueryDict): Promise => { + const { code, state } = getCodeAndStateFromQueryParams(queryParams); + + const storedAuthorizationParams = retrieveAuthorizationParams(state); + + await completeAuthorizationCodeGrant(code, storedAuthorizationParams); + + logger.debug("Got a valid token"); + + // TODO(kerrya) use the token somewhere :-) + throw new Error("OIDC login not fully implemented.") +}; diff --git a/src/utils/oidc/error.ts b/src/utils/oidc/error.ts index 3604ee77c24..37c4ddf9f86 100644 --- a/src/utils/oidc/error.ts +++ b/src/utils/oidc/error.ts @@ -18,4 +18,6 @@ export enum OidcClientError { DynamicRegistrationNotSupported = "Dynamic registration not supported", DynamicRegistrationFailed = "Dynamic registration failed", DynamicRegistrationInvalid = "Dynamic registration invalid response", + CodeExchangeFailed = "Failed to exchange code for token", + InvalidBearerToken = "Invalid bearer token", } diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index ab98c756211..5264e0fa757 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -115,9 +115,8 @@ const registerOidcClient = async ( * @returns clientId if found, otherwise undefined */ const getStaticOidcClientId = (issuer: string, staticOidcClients?: Record): string | undefined => { - console.log('hhh getStaticClient', issuer, staticOidcClients); // static_oidc_clients are configured with a trailing slash - const issuerWithTrailingSlash = issuer.endsWith('/') ? issuer : issuer + '/'; + const issuerWithTrailingSlash = issuer.endsWith("/") ? issuer : issuer + "/"; return staticOidcClients?.[issuerWithTrailingSlash]; }; From 52d03c22546a4eb945878ba3e0aa2b9fae04935e Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 14 Jun 2023 17:04:32 +1200 Subject: [PATCH 21/61] navigate to oidc authorize url --- res/css/structures/auth/_Login.pcss | 5 + src/Lifecycle.ts | 1 + src/components/structures/auth/Login.tsx | 21 +++- src/utils/oidc/authorize.ts | 121 +++++++++++++++++++++++ src/utils/oidc/registerClient.ts | 5 +- 5 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 src/utils/oidc/authorize.ts diff --git a/res/css/structures/auth/_Login.pcss b/res/css/structures/auth/_Login.pcss index 2eba8cf3d14..eeca1e8e494 100644 --- a/res/css/structures/auth/_Login.pcss +++ b/res/css/structures/auth/_Login.pcss @@ -99,3 +99,8 @@ div.mx_AccessibleButton_kind_link.mx_Login_forgot { align-content: center; padding: 14px; } + +.mx_Login_fullWidthButton { + width: 100%; + margin-bottom: 16px; +} diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index 6f49a0a0b61..bf9293e4df6 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -196,6 +196,7 @@ export function attemptTokenLogin( defaultDeviceDisplayName?: string, fragmentAfterLogin?: string, ): Promise { + debugger; if (!queryParams.loginToken) { return Promise.resolve(false); } diff --git a/src/components/structures/auth/Login.tsx b/src/components/structures/auth/Login.tsx index 14801a93936..a39b79988bb 100644 --- a/src/components/structures/auth/Login.tsx +++ b/src/components/structures/auth/Login.tsx @@ -20,7 +20,7 @@ import { logger } from "matrix-js-sdk/src/logger"; import { ISSOFlow, SSOAction } from "matrix-js-sdk/src/@types/auth"; import { _t, _td, UserFriendlyError } from "../../../languageHandler"; -import Login, { ClientLoginFlow } from "../../../Login"; +import Login, { ClientLoginFlow, OidcNativeFlow } from "../../../Login"; import { messageForConnectionError, messageForLoginError } from "../../../utils/ErrorUtils"; import AutoDiscoveryUtils from "../../../utils/AutoDiscoveryUtils"; import AuthPage from "../../views/auth/AuthPage"; @@ -39,6 +39,7 @@ import AccessibleButton, { ButtonEvent } from "../../views/elements/AccessibleBu import { ValidatedServerConfig } from "../../../utils/ValidatedServerConfig"; import { filterBoolean } from "../../../utils/arrays"; import { Features } from "../../../settings/Settings"; +import { login, startOidcLogin } from "../../../utils/oidc/authorize"; // These are used in several places, and come from the js-sdk's autodiscovery // stuff. We define them here so that they'll be picked up by i18n. @@ -146,6 +147,7 @@ export default class LoginComponent extends React.PureComponent "m.login.cas": () => this.renderSsoStep("cas"), // eslint-disable-next-line @typescript-eslint/naming-convention "m.login.sso": () => this.renderSsoStep("sso"), + "oidcNativeFlow": () => this.renderOidcNativeStep(), }; } @@ -430,7 +432,7 @@ export default class LoginComponent extends React.PureComponent if (!this.state.flows) return null; // this is the ideal order we want to show the flows in - const order = ["m.login.password", "m.login.sso"]; + const order = ["oidcNativeFlow", "m.login.password", "m.login.sso"]; const flows = filterBoolean(order.map((type) => this.state.flows?.find((flow) => flow.type === type))); return ( @@ -463,6 +465,21 @@ export default class LoginComponent extends React.PureComponent ); }; + private renderOidcNativeStep = (): React.ReactNode => { + const flow = this.state.flows!.find((flow) => flow.type === "oidcNativeFlow")! as OidcNativeFlow; + return ( + { + await startOidcLogin(this.props.serverConfig.delegatedAuthentication!, flow.clientId, this.props.serverConfig.hsUrl); + }} + > + {_t("Continue")} + + ); + }; + private renderSsoStep = (loginType: "cas" | "sso"): JSX.Element => { const flow = this.state.flows?.find((flow) => flow.type === "m.login." + loginType) as ISSOFlow; diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts new file mode 100644 index 00000000000..aa0a6722c7a --- /dev/null +++ b/src/utils/oidc/authorize.ts @@ -0,0 +1,121 @@ +/* +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. +*/ + +import { logger } from "matrix-js-sdk/src/logger"; +import { randomString } from "matrix-js-sdk/src/randomstring"; +import { ValidatedServerConfig } from "../ValidatedServerConfig"; + +type AuthorizationParams = { + state: string; + scope: string; + redirectUri: string; + nonce?: string; + codeVerifier?: string; +}; + +const generateScope = (existingDeviceId?: string): string => { + const deviceId = existingDeviceId || randomString(10); + return `openid urn:matrix:org.matrix.msc2967.client:api:* urn:matrix:org.matrix.msc2967.client:device:${deviceId}`; +}; + +// https://www.rfc-editor.org/rfc/rfc7636 +const generateCodeChallenge = async (codeVerifier: string): Promise => { + if (!window.crypto || !window.crypto.subtle) { + // @TODO(kerrya) should this be allowed? configurable? + logger.warn("A secure context is required to generate code challenge. Using plain text code challenge"); + return codeVerifier; + } + const utf8 = new TextEncoder().encode(codeVerifier); + + const digest = await crypto.subtle.digest("SHA-256", utf8); + + return btoa(String.fromCharCode(...new Uint8Array(digest))) + .replace(/=/g, "") + .replace(/\+/g, "-") + .replace(/\//g, "_"); +}; + +const storeAuthorizationParams = async ( + { redirectUri, state, nonce, codeVerifier }: AuthorizationParams, + { issuer }: ValidatedServerConfig["delegatedAuthentication"], + clientId: string, + homeserver: string, +): Promise => { + window.sessionStorage.setItem(`oidc_${state}_nonce`, nonce); + window.sessionStorage.setItem(`oidc_${state}_redirectUri`, redirectUri); + window.sessionStorage.setItem(`oidc_${state}_codeVerifier`, codeVerifier); + window.sessionStorage.setItem(`oidc_${state}_clientId`, clientId); + window.sessionStorage.setItem(`oidc_${state}_issuer`, issuer); + window.sessionStorage.setItem(`oidc_${state}_homeserver`, homeserver); +}; + +const generateAuthorizationParams = ({ + deviceId, + redirectUri, +}: { + deviceId?: string; + redirectUri: string; +}): AuthorizationParams => ({ + scope: generateScope(deviceId), + redirectUri, + state: randomString(8), + nonce: randomString(8), + codeVerifier: randomString(64), // https://tools.ietf.org/html/rfc7636#section-4.1 length needs to be 43-128 characters +}); + +const generateAuthorizationUrl = async ( + authorizationUrl: string, + clientId: string, + { scope, redirectUri, state, nonce, codeVerifier }: AuthorizationParams, +): Promise => { + const url = new URL(authorizationUrl); + url.searchParams.append("response_mode", "fragment"); + url.searchParams.append("response_type", "code"); + url.searchParams.append("redirect_uri", redirectUri); + url.searchParams.append("client_id", clientId); + url.searchParams.append("state", state); + url.searchParams.append("scope", scope); + if (nonce) { + url.searchParams.append("nonce", nonce); + } + + if (codeVerifier) { + url.searchParams.append("code_challenge_method", "S256"); + url.searchParams.append("code_challenge", await generateCodeChallenge(codeVerifier)); + } + + return url.toString(); +}; + +export const startOidcLogin = async ( + delegatedAuthConfig: ValidatedServerConfig["delegatedAuthentication"], + clientId: string, + homeserver: string, +): Promise => { + // TODO(kerrya) afterloginfragment + const redirectUri = window.location.origin; + const authParams = generateAuthorizationParams({ redirectUri }); + + storeAuthorizationParams(authParams, delegatedAuthConfig, clientId, homeserver); + + const authorizationUrl = await generateAuthorizationUrl( + delegatedAuthConfig.authorizationEndpoint, + clientId, + authParams, + ); + + window.location.href = authorizationUrl; +}; diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 7dc3998dc3d..ab98c756211 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -115,7 +115,10 @@ const registerOidcClient = async ( * @returns clientId if found, otherwise undefined */ const getStaticOidcClientId = (issuer: string, staticOidcClients?: Record): string | undefined => { - return staticOidcClients?.[issuer]; + console.log('hhh getStaticClient', issuer, staticOidcClients); + // static_oidc_clients are configured with a trailing slash + const issuerWithTrailingSlash = issuer.endsWith('/') ? issuer : issuer + '/'; + return staticOidcClients?.[issuerWithTrailingSlash]; }; /** From 3c499da47d035540fa2bf123da077a5b566b794b Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 15 Jun 2023 18:21:18 +1200 Subject: [PATCH 22/61] navigate to oidc authorize url --- src/Lifecycle.ts | 1 - src/components/structures/auth/Login.tsx | 8 ++++-- src/utils/oidc/authorize.ts | 25 +++++++++++++------ src/utils/oidc/registerClient.ts | 3 +-- .../components/structures/auth/Login-test.tsx | 7 ++---- 5 files changed, 27 insertions(+), 17 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index bf9293e4df6..6f49a0a0b61 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -196,7 +196,6 @@ export function attemptTokenLogin( defaultDeviceDisplayName?: string, fragmentAfterLogin?: string, ): Promise { - debugger; if (!queryParams.loginToken) { return Promise.resolve(false); } diff --git a/src/components/structures/auth/Login.tsx b/src/components/structures/auth/Login.tsx index a39b79988bb..1f97d734ea1 100644 --- a/src/components/structures/auth/Login.tsx +++ b/src/components/structures/auth/Login.tsx @@ -39,7 +39,7 @@ import AccessibleButton, { ButtonEvent } from "../../views/elements/AccessibleBu import { ValidatedServerConfig } from "../../../utils/ValidatedServerConfig"; import { filterBoolean } from "../../../utils/arrays"; import { Features } from "../../../settings/Settings"; -import { login, startOidcLogin } from "../../../utils/oidc/authorize"; +import { startOidcLogin } from "../../../utils/oidc/authorize"; // These are used in several places, and come from the js-sdk's autodiscovery // stuff. We define them here so that they'll be picked up by i18n. @@ -472,7 +472,11 @@ export default class LoginComponent extends React.PureComponent className="mx_Login_fullWidthButton" kind="primary" onClick={async () => { - await startOidcLogin(this.props.serverConfig.delegatedAuthentication!, flow.clientId, this.props.serverConfig.hsUrl); + await startOidcLogin( + this.props.serverConfig.delegatedAuthentication!, + flow.clientId, + this.props.serverConfig.hsUrl, + ); }} > {_t("Continue")} diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index aa0a6722c7a..80427f05ffe 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -16,6 +16,7 @@ limitations under the License. import { logger } from "matrix-js-sdk/src/logger"; import { randomString } from "matrix-js-sdk/src/randomstring"; + import { ValidatedServerConfig } from "../ValidatedServerConfig"; type AuthorizationParams = { @@ -48,6 +49,13 @@ const generateCodeChallenge = async (codeVerifier: string): Promise => { .replace(/\//g, "_"); }; +/** + * Store authorization params for retrieval when returning from OIDC OP + * @param { AuthorizationParams } authorizationParams + * @param { ValidatedServerConfig["delegatedAuthentication"] } delegatedAuthConfig used for future interactions with OP + * @param clientId this client's id as registered with configured issuer + * @param homeserver target homeserver + */ const storeAuthorizationParams = async ( { redirectUri, state, nonce, codeVerifier }: AuthorizationParams, { issuer }: ValidatedServerConfig["delegatedAuthentication"], @@ -88,18 +96,21 @@ const generateAuthorizationUrl = async ( url.searchParams.append("client_id", clientId); url.searchParams.append("state", state); url.searchParams.append("scope", scope); - if (nonce) { - url.searchParams.append("nonce", nonce); - } + url.searchParams.append("nonce", nonce); - if (codeVerifier) { - url.searchParams.append("code_challenge_method", "S256"); - url.searchParams.append("code_challenge", await generateCodeChallenge(codeVerifier)); - } + url.searchParams.append("code_challenge_method", "S256"); + url.searchParams.append("code_challenge", await generateCodeChallenge(codeVerifier)); return url.toString(); }; +/** + * Start OIDC authorization code flow + * Navigates to configured authorization endpoint + * @param delegatedAuthConfig from discovery + * @param clientId this client's id as registered with configured issuer + * @param homeserver target homeserver + */ export const startOidcLogin = async ( delegatedAuthConfig: ValidatedServerConfig["delegatedAuthentication"], clientId: string, diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index ab98c756211..5264e0fa757 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -115,9 +115,8 @@ const registerOidcClient = async ( * @returns clientId if found, otherwise undefined */ const getStaticOidcClientId = (issuer: string, staticOidcClients?: Record): string | undefined => { - console.log('hhh getStaticClient', issuer, staticOidcClients); // static_oidc_clients are configured with a trailing slash - const issuerWithTrailingSlash = issuer.endsWith('/') ? issuer : issuer + '/'; + const issuerWithTrailingSlash = issuer.endsWith("/") ? issuer : issuer + "/"; return staticOidcClients?.[issuerWithTrailingSlash]; }; diff --git a/test/components/structures/auth/Login-test.tsx b/test/components/structures/auth/Login-test.tsx index 9ae8d09db1e..e6744b2ac8f 100644 --- a/test/components/structures/auth/Login-test.tsx +++ b/test/components/structures/auth/Login-test.tsx @@ -409,7 +409,7 @@ 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 () => { + it("should show continue button when oidc native flow is correctly configured", async () => { fetchMock.post(delegatedAuth.registrationEndpoint, { client_id: "abc123" }); getComponent(hsUrl, isUrl, delegatedAuth); @@ -417,10 +417,7 @@ describe("Login", function () { // did not continue with matrix login expect(mockClient.loginFlows).not.toHaveBeenCalled(); - // no oidc native UI yet - expect( - screen.getByText("This homeserver doesn't offer any login flows which are supported by this client."), - ).toBeInTheDocument(); + expect(screen.getByText("Continue")).toBeInTheDocument(); }); /** From 551670f6580c9bda52a57a3325df336ad1fc56d9 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 15 Jun 2023 19:38:00 +1200 Subject: [PATCH 23/61] test --- src/utils/oidc/authorize.ts | 4 +- test/utils/oidc/authorize-test.ts | 152 ++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 test/utils/oidc/authorize-test.ts diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 80427f05ffe..68b45cad2e3 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -56,12 +56,12 @@ const generateCodeChallenge = async (codeVerifier: string): Promise => { * @param clientId this client's id as registered with configured issuer * @param homeserver target homeserver */ -const storeAuthorizationParams = async ( +const storeAuthorizationParams = ( { redirectUri, state, nonce, codeVerifier }: AuthorizationParams, { issuer }: ValidatedServerConfig["delegatedAuthentication"], clientId: string, homeserver: string, -): Promise => { +): void => { window.sessionStorage.setItem(`oidc_${state}_nonce`, nonce); window.sessionStorage.setItem(`oidc_${state}_redirectUri`, redirectUri); window.sessionStorage.setItem(`oidc_${state}_codeVerifier`, codeVerifier); diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts new file mode 100644 index 00000000000..5b1898e9c4b --- /dev/null +++ b/test/utils/oidc/authorize-test.ts @@ -0,0 +1,152 @@ +/* +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. +*/ + +import fetchMockJest from "fetch-mock-jest"; +import * as randomStringUtils from "matrix-js-sdk/src/randomstring"; +import { logger } from "matrix-js-sdk/src/logger"; + +import { startOidcLogin } from "../../../src/utils/oidc/authorize"; + +describe("startOidcLogin()", () => { + const issuer = "https://auth.com/"; + const authorizationEndpoint = "https://auth.com/authorization"; + const homeserver = "https://matrix.org"; + const clientId = "xyz789"; + const baseUrl = "https://test.com"; + + const delegatedAuthConfig = { + issuer, + registrationEndpoint: issuer + "registration", + authorizationEndpoint, + tokenEndpoint: issuer + "token", + }; + + const sessionStorageGetSpy = jest.spyOn(sessionStorage.__proto__, "setItem").mockReturnValue(undefined); + const randomStringMockImpl = (length) => new Array(length).fill("x").join(""); + + // to restore later + const realWindowLocation = window.location; + + beforeEach(() => { + fetchMockJest.mockClear(); + fetchMockJest.resetBehavior(); + + sessionStorageGetSpy.mockClear(); + + delete window.location; + // @ts-ignore ugly mocking + window.location = { + href: baseUrl, + origin: baseUrl, + }; + // @ts-ignore ugly mocking + window.crypto = undefined; + + jest.spyOn(randomStringUtils, "randomString").mockRestore(); + }); + + afterAll(() => { + window.location = realWindowLocation; + }); + + it("should store authorization params in session storage", async () => { + jest.spyOn(randomStringUtils, "randomString").mockReset().mockImplementation(randomStringMockImpl); + await startOidcLogin(delegatedAuthConfig, clientId, homeserver); + + const state = randomStringUtils.randomString(8); + + expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_nonce`, randomStringUtils.randomString(8)); + expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_redirectUri`, baseUrl); + expect(sessionStorageGetSpy).toHaveBeenCalledWith( + `oidc_${state}_codeVerifier`, + randomStringUtils.randomString(64), + ); + expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_clientId`, clientId); + expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_issuer`, issuer); + expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_homeserver`, homeserver); + }); + + it("navigates to authorization endpoint with correct parameters", async () => { + await startOidcLogin(delegatedAuthConfig, clientId, homeserver); + + const expectedScopeWithoutDeviceId = `openid urn:matrix:org.matrix.msc2967.client:api:* urn:matrix:org.matrix.msc2967.client:device:`; + + const authUrl = new URL(window.location.href); + + expect(authUrl.searchParams.get("response_mode")).toEqual("fragment"); + expect(authUrl.searchParams.get("response_type")).toEqual("code"); + expect(authUrl.searchParams.get("client_id")).toEqual(clientId); + expect(authUrl.searchParams.get("code_challenge_method")).toEqual("S256"); + + // scope ends with a 10char randomstring deviceId + const scope = authUrl.searchParams.get("scope"); + expect(scope.substring(0, scope.length - 10)).toEqual(expectedScopeWithoutDeviceId); + expect(scope.substring(scope.length - 10)).toBeTruthy(); + + // random string, just check they are set + expect(authUrl.searchParams.has("state")).toBeTruthy(); + expect(authUrl.searchParams.has("nonce")).toBeTruthy(); + expect(authUrl.searchParams.has("code_challenge")).toBeTruthy(); + }); + + it("uses a plain text code challenge when crypto is not available", async () => { + jest.spyOn(logger, "warn"); + await startOidcLogin(delegatedAuthConfig, clientId, homeserver); + + expect(logger.warn).toHaveBeenCalledWith( + "A secure context is required to generate code challenge. Using plain text code challenge", + ); + + const authUrl = new URL(window.location.href); + + // compare the code challenge in url to the code verifier we stored in session storage + const codeChallenge = authUrl.searchParams.get("code_challenge"); + const codeVerifier = sessionStorageGetSpy.mock.calls.find(([key]) => + (key as string).includes("codeVerifier"), + )[1]; + expect(codeVerifier).toEqual(codeChallenge); + }); + + it("uses a s256 code challenge when crypto is available", async () => { + delete window.crypto; + window.crypto = { + // @ts-ignore mock + subtle: { + digest: jest.fn().mockReturnValue(new ArrayBuffer(32)), + }, + }; + + await startOidcLogin(delegatedAuthConfig, clientId, homeserver); + + expect(logger.warn).toHaveBeenCalledWith( + "A secure context is required to generate code challenge. Using plain text code challenge", + ); + + const authUrl = new URL(window.location.href); + + // compare the code challenge in url to the code verifier we stored in session storage + const codeChallenge = authUrl.searchParams.get("code_challenge"); + const codeVerifier = sessionStorageGetSpy.mock.calls.find(([key]) => + (key as string).includes("codeVerifier"), + )[1]; + + expect(window.crypto.subtle.digest).toHaveBeenCalledWith("SHA-256", expect.any(Object)); + + expect(codeVerifier).not.toEqual(codeChallenge); + // result from new ArrayBuffer(32) + expect(codeChallenge).toEqual("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"); + }); +}); From b100d6d848f5638905a22ca23c0140a04a00779d Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 23 Jun 2023 12:34:27 +1200 Subject: [PATCH 24/61] adjust for js-sdk code --- src/utils/oidc/authorize.ts | 75 +++---------------------------- test/utils/oidc/authorize-test.ts | 51 --------------------- 2 files changed, 5 insertions(+), 121 deletions(-) diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 68b45cad2e3..4137b69188c 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -14,40 +14,9 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { logger } from "matrix-js-sdk/src/logger"; -import { randomString } from "matrix-js-sdk/src/randomstring"; +import { generateAuthorizationParams, generateAuthorizationUrl } from "matrix-js-sdk/src/oidc/authorize"; -import { ValidatedServerConfig } from "../ValidatedServerConfig"; - -type AuthorizationParams = { - state: string; - scope: string; - redirectUri: string; - nonce?: string; - codeVerifier?: string; -}; - -const generateScope = (existingDeviceId?: string): string => { - const deviceId = existingDeviceId || randomString(10); - return `openid urn:matrix:org.matrix.msc2967.client:api:* urn:matrix:org.matrix.msc2967.client:device:${deviceId}`; -}; - -// https://www.rfc-editor.org/rfc/rfc7636 -const generateCodeChallenge = async (codeVerifier: string): Promise => { - if (!window.crypto || !window.crypto.subtle) { - // @TODO(kerrya) should this be allowed? configurable? - logger.warn("A secure context is required to generate code challenge. Using plain text code challenge"); - return codeVerifier; - } - const utf8 = new TextEncoder().encode(codeVerifier); - - const digest = await crypto.subtle.digest("SHA-256", utf8); - - return btoa(String.fromCharCode(...new Uint8Array(digest))) - .replace(/=/g, "") - .replace(/\+/g, "-") - .replace(/\//g, "_"); -}; +import { ValidatedDelegatedAuthConfig } from "../ValidatedServerConfig"; /** * Store authorization params for retrieval when returning from OIDC OP @@ -57,8 +26,8 @@ const generateCodeChallenge = async (codeVerifier: string): Promise => { * @param homeserver target homeserver */ const storeAuthorizationParams = ( - { redirectUri, state, nonce, codeVerifier }: AuthorizationParams, - { issuer }: ValidatedServerConfig["delegatedAuthentication"], + { redirectUri, state, nonce, codeVerifier }: ReturnType, + { issuer }: ValidatedDelegatedAuthConfig, clientId: string, homeserver: string, ): void => { @@ -70,40 +39,6 @@ const storeAuthorizationParams = ( window.sessionStorage.setItem(`oidc_${state}_homeserver`, homeserver); }; -const generateAuthorizationParams = ({ - deviceId, - redirectUri, -}: { - deviceId?: string; - redirectUri: string; -}): AuthorizationParams => ({ - scope: generateScope(deviceId), - redirectUri, - state: randomString(8), - nonce: randomString(8), - codeVerifier: randomString(64), // https://tools.ietf.org/html/rfc7636#section-4.1 length needs to be 43-128 characters -}); - -const generateAuthorizationUrl = async ( - authorizationUrl: string, - clientId: string, - { scope, redirectUri, state, nonce, codeVerifier }: AuthorizationParams, -): Promise => { - const url = new URL(authorizationUrl); - url.searchParams.append("response_mode", "fragment"); - url.searchParams.append("response_type", "code"); - url.searchParams.append("redirect_uri", redirectUri); - url.searchParams.append("client_id", clientId); - url.searchParams.append("state", state); - url.searchParams.append("scope", scope); - url.searchParams.append("nonce", nonce); - - url.searchParams.append("code_challenge_method", "S256"); - url.searchParams.append("code_challenge", await generateCodeChallenge(codeVerifier)); - - return url.toString(); -}; - /** * Start OIDC authorization code flow * Navigates to configured authorization endpoint @@ -112,7 +47,7 @@ const generateAuthorizationUrl = async ( * @param homeserver target homeserver */ export const startOidcLogin = async ( - delegatedAuthConfig: ValidatedServerConfig["delegatedAuthentication"], + delegatedAuthConfig: ValidatedDelegatedAuthConfig, clientId: string, homeserver: string, ): Promise => { diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts index 5b1898e9c4b..74c27f71676 100644 --- a/test/utils/oidc/authorize-test.ts +++ b/test/utils/oidc/authorize-test.ts @@ -16,7 +16,6 @@ limitations under the License. import fetchMockJest from "fetch-mock-jest"; import * as randomStringUtils from "matrix-js-sdk/src/randomstring"; -import { logger } from "matrix-js-sdk/src/logger"; import { startOidcLogin } from "../../../src/utils/oidc/authorize"; @@ -52,8 +51,6 @@ describe("startOidcLogin()", () => { href: baseUrl, origin: baseUrl, }; - // @ts-ignore ugly mocking - window.crypto = undefined; jest.spyOn(randomStringUtils, "randomString").mockRestore(); }); @@ -101,52 +98,4 @@ describe("startOidcLogin()", () => { expect(authUrl.searchParams.has("nonce")).toBeTruthy(); expect(authUrl.searchParams.has("code_challenge")).toBeTruthy(); }); - - it("uses a plain text code challenge when crypto is not available", async () => { - jest.spyOn(logger, "warn"); - await startOidcLogin(delegatedAuthConfig, clientId, homeserver); - - expect(logger.warn).toHaveBeenCalledWith( - "A secure context is required to generate code challenge. Using plain text code challenge", - ); - - const authUrl = new URL(window.location.href); - - // compare the code challenge in url to the code verifier we stored in session storage - const codeChallenge = authUrl.searchParams.get("code_challenge"); - const codeVerifier = sessionStorageGetSpy.mock.calls.find(([key]) => - (key as string).includes("codeVerifier"), - )[1]; - expect(codeVerifier).toEqual(codeChallenge); - }); - - it("uses a s256 code challenge when crypto is available", async () => { - delete window.crypto; - window.crypto = { - // @ts-ignore mock - subtle: { - digest: jest.fn().mockReturnValue(new ArrayBuffer(32)), - }, - }; - - await startOidcLogin(delegatedAuthConfig, clientId, homeserver); - - expect(logger.warn).toHaveBeenCalledWith( - "A secure context is required to generate code challenge. Using plain text code challenge", - ); - - const authUrl = new URL(window.location.href); - - // compare the code challenge in url to the code verifier we stored in session storage - const codeChallenge = authUrl.searchParams.get("code_challenge"); - const codeVerifier = sessionStorageGetSpy.mock.calls.find(([key]) => - (key as string).includes("codeVerifier"), - )[1]; - - expect(window.crypto.subtle.digest).toHaveBeenCalledWith("SHA-256", expect.any(Object)); - - expect(codeVerifier).not.toEqual(codeChallenge); - // result from new ArrayBuffer(32) - expect(codeChallenge).toEqual("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"); - }); }); From 28a36ec7ed7ac4dd62f280028fb46760f88dbe05 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 23 Jun 2023 15:49:29 +1200 Subject: [PATCH 25/61] login with oidc native flow: messy version --- src/Lifecycle.ts | 72 +++++++++++++++++++++--- src/components/structures/MatrixChat.tsx | 2 +- src/utils/oidc/authorize.ts | 41 +++++++++++--- test/utils/oidc/authorize-test.ts | 7 ++- 4 files changed, 102 insertions(+), 20 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index 4eb3a7fcb2f..592a8e9fcd9 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -65,6 +65,7 @@ import { OverwriteLoginPayload } from "./dispatcher/payloads/OverwriteLoginPaylo import { SdkContextClass } from "./contexts/SDKContext"; import { messageForLoginError } from "./utils/ErrorUtils"; import { completeOidcLogin } from "./utils/oidc/authorize"; +import { OidcError } from "matrix-js-sdk/src/oidc/error"; const HOMESERVER_URL_KEY = "mx_hs_url"; const ID_SERVER_URL_KEY = "mx_is_url"; @@ -192,8 +193,8 @@ export async function getStoredSessionOwner(): Promise<[string, boolean] | [null * @param {string} defaultDeviceDisplayName * @param {string} fragmentAfterLogin path to go to after a successful login, only used for "Try again" * - * @returns {Promise} promise which resolves to true if we completed the token - * login, else false + * @returns {Promise} promise which resolves to true if we completed the delegated auth login + * else false */ export async function attemptDelegatedAuthLogin( queryParams: QueryDict, @@ -201,18 +202,71 @@ export async function attemptDelegatedAuthLogin( fragmentAfterLogin?: string, ): Promise { if (queryParams.code && queryParams.state) { - try { - await completeOidcLogin(queryParams); - return true; - } catch (error) { - logger.error("Failed to login via OIDC", error) - return false; - } + return attemptOidcNativeLogin(queryParams); } return attemptTokenLogin(queryParams, defaultDeviceDisplayName, fragmentAfterLogin); } +async function attemptOidcNativeLogin(queryParams: QueryDict): Promise { + try { + const { accessToken, homeserverUrl, identityServerUrl } = await completeOidcLogin(queryParams); + + // @TODO(kerrya) try catch with error code for this + const { + user_id: userId, + // @TODO(kerrya) should use this deviceId, or check they match? + device_id: deviceId, + // @TODO(kerrya) WhoAmIResponse needs to be updated + is_guest: isGuest, + } = await getUserIdFromAccessToken(accessToken, homeserverUrl, identityServerUrl); + + const credentials = { + accessToken, + homeserverUrl, + identityServerUrl, + deviceId, + userId, + isGuest, + }; + + // @TODO(kerrya) should clear storage? + // @TODO(kerrya) other token login doesnt use this codepath, why? + await doSetLoggedIn(credentials, true); + return true; + } catch (error) { + logger.error("Failed to login via OIDC", error); + return false; + } +} + +/** + * Gets information about the owner of a given access token. + * @param accessToken + * @param homeserverUrl + * @param identityServerUrl + * @returns Promise that resolves with whoami response + * @throws when whoami request fails + */ +async function getUserIdFromAccessToken( + accessToken: string, + homeserverUrl: string, + identityServerUrl?: string, +): Promise> { + try { + const client = createClient({ + baseUrl: homeserverUrl, + accessToken: accessToken, + idBaseUrl: identityServerUrl, + }); + + return await client.whoami(); + } catch (error) { + console.error("Failed to retrieve userId using accessToken", error); + throw new Error("Failed to retrieve userId using accessToken"); + } +} + /** * @param {QueryDict} queryParams string->string map of the * query-parameters extracted from the real query-string of the starting diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index a7200d25b23..c7096390559 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -325,7 +325,7 @@ export default class MatrixChat extends React.PureComponent { this.props.onTokenLoginCompleted(); } - // @TODO(kerrya) remove OIDC code and state from URL + // @TODO(kerrya) remove OIDC code and state from URL if (loggedIn) { this.tokenLogin = true; diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 445786cb4b9..ed8f0b6214c 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -15,7 +15,11 @@ limitations under the License. */ import { logger } from "matrix-js-sdk/src/logger"; -import { generateAuthorizationParams, generateAuthorizationUrl, completeAuthorizationCodeGrant } from "matrix-js-sdk/src/oidc/authorize"; +import { + generateAuthorizationParams, + generateAuthorizationUrl, + completeAuthorizationCodeGrant, +} from "matrix-js-sdk/src/oidc/authorize"; import { QueryDict } from "matrix-js-sdk/src/utils"; import { ValidatedDelegatedAuthConfig } from "../ValidatedServerConfig"; @@ -77,7 +81,7 @@ export const startOidcLogin = async ( clientId: string, homeserver: string, ): Promise => { - // TODO(kerrya) afterloginfragment + // TODO(kerrya) afterloginfragment https://github.com/vector-im/element-web/issues/25656 const redirectUri = window.location.origin; const authParams = generateAuthorizationParams({ redirectUri }); @@ -109,7 +113,16 @@ const getCodeAndStateFromQueryParams = (queryParams: QueryDict): { code: string; return { code, state }; }; - +export interface IMatrixClientCreds { + homeserverUrl: string; + identityServerUrl?: string; + userId: string; + deviceId?: string; + accessToken?: string; + guest?: boolean; + pickleKey?: string; + freshLogin?: boolean; +} /** * Attempt to complete authorization code flow to login @@ -117,15 +130,27 @@ const getCodeAndStateFromQueryParams = (queryParams: QueryDict): { code: string; * @returns {Promise} Promise that resolves when login was successful * @throws When login failed */ -export const completeOidcLogin = async (queryParams: QueryDict): Promise => { +export const completeOidcLogin = async ( + queryParams: QueryDict, +): Promise<{ + homeserverUrl: string; + identityServerUrl?: string; + accessToken: string; +}> => { const { code, state } = getCodeAndStateFromQueryParams(queryParams); const storedAuthorizationParams = retrieveAuthorizationParams(state); - await completeAuthorizationCodeGrant(code, storedAuthorizationParams); + const bearerToken = await completeAuthorizationCodeGrant(code, storedAuthorizationParams); + + // @TODO(kerrya) do I need to verify anything else in response? - logger.debug("Got a valid token"); + // @TODO(kerrya) do something with the refresh token - // TODO(kerrya) use the token somewhere :-) - throw new Error("OIDC login not fully implemented.") + return { + homeserverUrl: storedAuthorizationParams.homeserver, + accessToken: bearerToken.access_token, + // @TODO(kerrya) persist this in session storage with other auth params + // identityServerUrl + }; }; diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts index 74c27f71676..0f947ba6967 100644 --- a/test/utils/oidc/authorize-test.ts +++ b/test/utils/oidc/authorize-test.ts @@ -72,8 +72,11 @@ describe("startOidcLogin()", () => { randomStringUtils.randomString(64), ); expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_clientId`, clientId); - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_issuer`, issuer); expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_homeserver`, homeserver); + expect(sessionStorageGetSpy).toHaveBeenCalledWith( + `oidc_${state}_delegatedAuthConfig`, + JSON.stringify(delegatedAuthConfig), + ); }); it("navigates to authorization endpoint with correct parameters", async () => { @@ -83,7 +86,7 @@ describe("startOidcLogin()", () => { const authUrl = new URL(window.location.href); - expect(authUrl.searchParams.get("response_mode")).toEqual("fragment"); + expect(authUrl.searchParams.get("response_mode")).toEqual("query"); expect(authUrl.searchParams.get("response_type")).toEqual("code"); expect(authUrl.searchParams.get("client_id")).toEqual(clientId); expect(authUrl.searchParams.get("code_challenge_method")).toEqual("S256"); From a1b975de3863005db36b8c6c52bc939442bf9833 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 23 Jun 2023 16:03:09 +1200 Subject: [PATCH 26/61] tidy --- src/Lifecycle.ts | 3 +-- src/utils/oidc/authorize.ts | 14 +------------- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index 592a8e9fcd9..3035d87e95e 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -2,7 +2,7 @@ Copyright 2015, 2016 OpenMarket Ltd Copyright 2017 Vector Creations Ltd Copyright 2018 New Vector Ltd -Copyright 2019, 2020 The Matrix.org Foundation C.I.C. +Copyright 2019, 2020, 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. @@ -65,7 +65,6 @@ import { OverwriteLoginPayload } from "./dispatcher/payloads/OverwriteLoginPaylo import { SdkContextClass } from "./contexts/SDKContext"; import { messageForLoginError } from "./utils/ErrorUtils"; import { completeOidcLogin } from "./utils/oidc/authorize"; -import { OidcError } from "matrix-js-sdk/src/oidc/error"; const HOMESERVER_URL_KEY = "mx_hs_url"; const ID_SERVER_URL_KEY = "mx_is_url"; diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index ed8f0b6214c..8f372604a69 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -14,7 +14,6 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { logger } from "matrix-js-sdk/src/logger"; import { generateAuthorizationParams, generateAuthorizationUrl, @@ -113,17 +112,6 @@ const getCodeAndStateFromQueryParams = (queryParams: QueryDict): { code: string; return { code, state }; }; -export interface IMatrixClientCreds { - homeserverUrl: string; - identityServerUrl?: string; - userId: string; - deviceId?: string; - accessToken?: string; - guest?: boolean; - pickleKey?: string; - freshLogin?: boolean; -} - /** * Attempt to complete authorization code flow to login * @param {QueryDict} queryParams the query-parameters extracted from the real query-string of the starting URI. @@ -143,7 +131,7 @@ export const completeOidcLogin = async ( const bearerToken = await completeAuthorizationCodeGrant(code, storedAuthorizationParams); - // @TODO(kerrya) do I need to verify anything else in response? + // @TODO(kerrya) is there more verification to do here? // @TODO(kerrya) do something with the refresh token From 72ff46a30722c7f2b6a0fd31434c962d9e8a1afa Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 23 Jun 2023 16:36:40 +1200 Subject: [PATCH 27/61] update test for response_mode query --- test/utils/oidc/authorize-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts index 74c27f71676..569b226ab90 100644 --- a/test/utils/oidc/authorize-test.ts +++ b/test/utils/oidc/authorize-test.ts @@ -83,7 +83,7 @@ describe("startOidcLogin()", () => { const authUrl = new URL(window.location.href); - expect(authUrl.searchParams.get("response_mode")).toEqual("fragment"); + expect(authUrl.searchParams.get("response_mode")).toEqual("query"); expect(authUrl.searchParams.get("response_type")).toEqual("code"); expect(authUrl.searchParams.get("client_id")).toEqual(clientId); expect(authUrl.searchParams.get("code_challenge_method")).toEqual("S256"); From dd9944da850399e18456ba707f03409a9820bf7f Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 23 Jun 2023 17:02:31 +1200 Subject: [PATCH 28/61] tidy up some TODOs --- src/Lifecycle.ts | 5 +---- src/utils/oidc/authorize.ts | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index 3035d87e95e..655d3ae4641 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -211,12 +211,9 @@ async function attemptOidcNativeLogin(queryParams: QueryDict): Promise try { const { accessToken, homeserverUrl, identityServerUrl } = await completeOidcLogin(queryParams); - // @TODO(kerrya) try catch with error code for this const { user_id: userId, - // @TODO(kerrya) should use this deviceId, or check they match? device_id: deviceId, - // @TODO(kerrya) WhoAmIResponse needs to be updated is_guest: isGuest, } = await getUserIdFromAccessToken(accessToken, homeserverUrl, identityServerUrl); @@ -230,7 +227,7 @@ async function attemptOidcNativeLogin(queryParams: QueryDict): Promise }; // @TODO(kerrya) should clear storage? - // @TODO(kerrya) other token login doesnt use this codepath, why? + // @TODO(kerrya) other token login doesn't use this codepath, why? await doSetLoggedIn(credentials, true); return true; } catch (error) { diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 8f372604a69..1d0071a5d6d 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -107,7 +107,7 @@ const getCodeAndStateFromQueryParams = (queryParams: QueryDict): { code: string; const state = queryParams["state"]; if (!code || typeof code !== "string" || !state || typeof state !== "string") { - throw new Error("TODO(kerrya) Invalid query parameters. `code` and `state` are required."); + throw new Error("Invalid query parameters for OIDC native login. `code` and `state` are required."); } return { code, state }; }; @@ -115,7 +115,7 @@ const getCodeAndStateFromQueryParams = (queryParams: QueryDict): { code: string; /** * Attempt to complete authorization code flow to login * @param {QueryDict} queryParams the query-parameters extracted from the real query-string of the starting URI. - * @returns {Promise} Promise that resolves when login was successful + * @returns {Promise<{}>} Promise that resolves with accesstoken, identityServerUrl, and homeserverUrl when login was successful * @throws When login failed */ export const completeOidcLogin = async ( From 857df8d14be7869b967bb245bd347bf9ee139726 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 26 Jun 2023 14:15:28 +1200 Subject: [PATCH 29/61] use new types --- src/utils/oidc/authorize.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 4137b69188c..08be4fca175 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -14,7 +14,11 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { generateAuthorizationParams, generateAuthorizationUrl } from "matrix-js-sdk/src/oidc/authorize"; +import { + AuthorizationParams, + generateAuthorizationParams, + generateAuthorizationUrl, +} from "matrix-js-sdk/src/oidc/authorize"; import { ValidatedDelegatedAuthConfig } from "../ValidatedServerConfig"; @@ -26,7 +30,7 @@ import { ValidatedDelegatedAuthConfig } from "../ValidatedServerConfig"; * @param homeserver target homeserver */ const storeAuthorizationParams = ( - { redirectUri, state, nonce, codeVerifier }: ReturnType, + { redirectUri, state, nonce, codeVerifier }: AuthorizationParams, { issuer }: ValidatedDelegatedAuthConfig, clientId: string, homeserver: string, From 95107270ebf976f69214a71788af73b1daf4cbea Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 26 Jun 2023 17:35:37 +1200 Subject: [PATCH 30/61] add identityServerUrl to stored params --- src/components/structures/auth/Login.tsx | 1 + src/utils/oidc/authorize.ts | 26 ++++++++++++++++-------- test/utils/oidc/authorize-test.ts | 2 +- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/components/structures/auth/Login.tsx b/src/components/structures/auth/Login.tsx index e4ac7f88ce9..123807f13d0 100644 --- a/src/components/structures/auth/Login.tsx +++ b/src/components/structures/auth/Login.tsx @@ -479,6 +479,7 @@ export default class LoginComponent extends React.PureComponent this.props.serverConfig.delegatedAuthentication!, flow.clientId, this.props.serverConfig.hsUrl, + this.props.serverConfig.isUrl, ); }} > diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index bc7d0df5757..077cc0c87fe 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -35,28 +35,33 @@ const storeAuthorizationParams = ( { redirectUri, state, nonce, codeVerifier }: AuthorizationParams, delegatedAuthConfig: ValidatedDelegatedAuthConfig, clientId: string, - homeserver: string, + homeserverUrl: string, + identityServerUrl?: string, ): void => { window.sessionStorage.setItem(`oidc_${state}_nonce`, nonce); window.sessionStorage.setItem(`oidc_${state}_redirectUri`, redirectUri); window.sessionStorage.setItem(`oidc_${state}_codeVerifier`, codeVerifier); - window.sessionStorage.setItem(`oidc_${state}_homeserver`, homeserver); window.sessionStorage.setItem(`oidc_${state}_clientId`, clientId); window.sessionStorage.setItem(`oidc_${state}_delegatedAuthConfig`, JSON.stringify(delegatedAuthConfig)); - window.sessionStorage.setItem(`oidc_${state}_homeserver`, homeserver); + window.sessionStorage.setItem(`oidc_${state}_homeserverUrl`, homeserverUrl); + if (identityServerUrl) { + window.sessionStorage.setItem(`oidc_${state}_identityServerUrl`, identityServerUrl); + } }; type StoredAuthorizationParams = Omit, "state" | "scope"> & { delegatedAuthConfig: ValidatedDelegatedAuthConfig; clientId: string; - homeserver: string; + homeserverUrl: string; + identityServerUrl: string; }; const retrieveAuthorizationParams = (state: string): StoredAuthorizationParams => { const nonce = window.sessionStorage.getItem(`oidc_${state}_nonce`); const redirectUri = window.sessionStorage.getItem(`oidc_${state}_redirectUri`); const codeVerifier = window.sessionStorage.getItem(`oidc_${state}_codeVerifier`); const clientId = window.sessionStorage.getItem(`oidc_${state}_clientId`); - const homeserver = window.sessionStorage.getItem(`oidc_${state}_homeserver`); + const homeserverUrl = window.sessionStorage.getItem(`oidc_${state}_homeserverUrl`); + const identityServerUrl = window.sessionStorage.getItem(`oidc_${state}_identityServerUrl`) ?? ""; const delegatedAuthConfig = JSON.parse(window.sessionStorage.getItem(`oidc_${state}_delegatedAuthConfig`)); return { @@ -64,7 +69,8 @@ const retrieveAuthorizationParams = (state: string): StoredAuthorizationParams = redirectUri, codeVerifier, clientId, - homeserver, + homeserverUrl, + identityServerUrl, delegatedAuthConfig, }; }; @@ -79,13 +85,14 @@ const retrieveAuthorizationParams = (state: string): StoredAuthorizationParams = export const startOidcLogin = async ( delegatedAuthConfig: ValidatedDelegatedAuthConfig, clientId: string, - homeserver: string, + homeserverUrl: string, + identityServerUrl?: string, ): Promise => { // TODO(kerrya) afterloginfragment https://github.com/vector-im/element-web/issues/25656 const redirectUri = window.location.origin; const authParams = generateAuthorizationParams({ redirectUri }); - storeAuthorizationParams(authParams, delegatedAuthConfig, clientId, homeserver); + storeAuthorizationParams(authParams, delegatedAuthConfig, clientId, homeserverUrl, identityServerUrl); const authorizationUrl = await generateAuthorizationUrl( delegatedAuthConfig.authorizationEndpoint, @@ -137,7 +144,8 @@ export const completeOidcLogin = async ( // @TODO(kerrya) do something with the refresh token return { - homeserverUrl: storedAuthorizationParams.homeserver, + homeserverUrl: storedAuthorizationParams.homeserverUrl, + identityServerUrl: storedAuthorizationParams.identityServerUrl, accessToken: bearerToken.access_token, // @TODO(kerrya) persist this in session storage with other auth params // identityServerUrl diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts index 0f947ba6967..38ae440a0d1 100644 --- a/test/utils/oidc/authorize-test.ts +++ b/test/utils/oidc/authorize-test.ts @@ -72,7 +72,7 @@ describe("startOidcLogin()", () => { randomStringUtils.randomString(64), ); expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_clientId`, clientId); - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_homeserver`, homeserver); + expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_homeserverUrl`, homeserver); expect(sessionStorageGetSpy).toHaveBeenCalledWith( `oidc_${state}_delegatedAuthConfig`, JSON.stringify(delegatedAuthConfig), From 044beb781957a504057870d9f4394c3740892cda Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 26 Jun 2023 18:36:20 +1200 Subject: [PATCH 31/61] unit test completeOidcLogin --- src/utils/oidc/authorize.ts | 27 ++++- test/utils/oidc/authorize-test.ts | 189 +++++++++++++++++++++++------- 2 files changed, 166 insertions(+), 50 deletions(-) diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 077cc0c87fe..d19145dfc78 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -55,16 +55,35 @@ type StoredAuthorizationParams = Omit): StoredAuthorizationParams => { + const requiredStringProperties = ["nonce", "redirectUri", "codeVerifier", "clientId", "homeserverUrl"]; + if ( + requiredStringProperties.every((key: string) => params[key] && typeof params[key] === "string") && + (params.identityServerUrl === undefined || typeof params.identityServerUrl === "string") && + !!params.delegatedAuthConfig + ) { + return params as StoredAuthorizationParams; + } + throw new Error("Cannot complete OIDC login: required properties not found in session storage"); +}; + const retrieveAuthorizationParams = (state: string): StoredAuthorizationParams => { const nonce = window.sessionStorage.getItem(`oidc_${state}_nonce`); const redirectUri = window.sessionStorage.getItem(`oidc_${state}_redirectUri`); const codeVerifier = window.sessionStorage.getItem(`oidc_${state}_codeVerifier`); const clientId = window.sessionStorage.getItem(`oidc_${state}_clientId`); const homeserverUrl = window.sessionStorage.getItem(`oidc_${state}_homeserverUrl`); - const identityServerUrl = window.sessionStorage.getItem(`oidc_${state}_identityServerUrl`) ?? ""; + const identityServerUrl = window.sessionStorage.getItem(`oidc_${state}_identityServerUrl`) ?? undefined; const delegatedAuthConfig = JSON.parse(window.sessionStorage.getItem(`oidc_${state}_delegatedAuthConfig`)); - return { + return validateStoredAuthorizationParams({ nonce, redirectUri, codeVerifier, @@ -72,7 +91,7 @@ const retrieveAuthorizationParams = (state: string): StoredAuthorizationParams = homeserverUrl, identityServerUrl, delegatedAuthConfig, - }; + }); }; /** @@ -147,7 +166,5 @@ export const completeOidcLogin = async ( homeserverUrl: storedAuthorizationParams.homeserverUrl, identityServerUrl: storedAuthorizationParams.identityServerUrl, accessToken: bearerToken.access_token, - // @TODO(kerrya) persist this in session storage with other auth params - // identityServerUrl }; }; diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts index 38ae440a0d1..adcbc5f1354 100644 --- a/test/utils/oidc/authorize-test.ts +++ b/test/utils/oidc/authorize-test.ts @@ -14,15 +14,18 @@ See the License for the specific language governing permissions and limitations under the License. */ -import fetchMockJest from "fetch-mock-jest"; +import fetchMock from "fetch-mock-jest"; +import { Method } from "matrix-js-sdk/src/http-api"; +import { generateAuthorizationParams } from "matrix-js-sdk/src/oidc/authorize"; import * as randomStringUtils from "matrix-js-sdk/src/randomstring"; -import { startOidcLogin } from "../../../src/utils/oidc/authorize"; +import { completeOidcLogin, startOidcLogin } from "../../../src/utils/oidc/authorize"; -describe("startOidcLogin()", () => { +describe("OIDC authorization", () => { const issuer = "https://auth.com/"; const authorizationEndpoint = "https://auth.com/authorization"; const homeserver = "https://matrix.org"; + const identityServerUrl = "https://is.org"; const clientId = "xyz789"; const baseUrl = "https://test.com"; @@ -33,17 +36,20 @@ describe("startOidcLogin()", () => { tokenEndpoint: issuer + "token", }; - const sessionStorageGetSpy = jest.spyOn(sessionStorage.__proto__, "setItem").mockReturnValue(undefined); + const sessionStorageSetSpy = jest.spyOn(sessionStorage.__proto__, "setItem").mockReturnValue(undefined); + const sessionStorageGetSpy = jest.spyOn(sessionStorage.__proto__, "getItem").mockReturnValue(undefined); + const randomStringMockImpl = (length) => new Array(length).fill("x").join(""); // to restore later const realWindowLocation = window.location; beforeEach(() => { - fetchMockJest.mockClear(); - fetchMockJest.resetBehavior(); + fetchMock.mockClear(); + fetchMock.resetBehavior(); - sessionStorageGetSpy.mockClear(); + sessionStorageSetSpy.mockClear(); + sessionStorageGetSpy.mockReset(); delete window.location; // @ts-ignore ugly mocking @@ -59,46 +65,139 @@ describe("startOidcLogin()", () => { window.location = realWindowLocation; }); - it("should store authorization params in session storage", async () => { - jest.spyOn(randomStringUtils, "randomString").mockReset().mockImplementation(randomStringMockImpl); - await startOidcLogin(delegatedAuthConfig, clientId, homeserver); - - const state = randomStringUtils.randomString(8); - - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_nonce`, randomStringUtils.randomString(8)); - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_redirectUri`, baseUrl); - expect(sessionStorageGetSpy).toHaveBeenCalledWith( - `oidc_${state}_codeVerifier`, - randomStringUtils.randomString(64), - ); - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_clientId`, clientId); - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_homeserverUrl`, homeserver); - expect(sessionStorageGetSpy).toHaveBeenCalledWith( - `oidc_${state}_delegatedAuthConfig`, - JSON.stringify(delegatedAuthConfig), - ); + describe("startOidcLogin()", () => { + it("should store authorization params in session storage", async () => { + jest.spyOn(randomStringUtils, "randomString").mockReset().mockImplementation(randomStringMockImpl); + await startOidcLogin(delegatedAuthConfig, clientId, homeserver); + + const state = randomStringUtils.randomString(8); + + expect(sessionStorageSetSpy).toHaveBeenCalledWith(`oidc_${state}_nonce`, randomStringUtils.randomString(8)); + expect(sessionStorageSetSpy).toHaveBeenCalledWith(`oidc_${state}_redirectUri`, baseUrl); + expect(sessionStorageSetSpy).toHaveBeenCalledWith( + `oidc_${state}_codeVerifier`, + randomStringUtils.randomString(64), + ); + expect(sessionStorageSetSpy).toHaveBeenCalledWith(`oidc_${state}_clientId`, clientId); + expect(sessionStorageSetSpy).toHaveBeenCalledWith(`oidc_${state}_homeserverUrl`, homeserver); + expect(sessionStorageSetSpy).toHaveBeenCalledWith( + `oidc_${state}_delegatedAuthConfig`, + JSON.stringify(delegatedAuthConfig), + ); + }); + + it("navigates to authorization endpoint with correct parameters", async () => { + await startOidcLogin(delegatedAuthConfig, clientId, homeserver); + + const expectedScopeWithoutDeviceId = `openid urn:matrix:org.matrix.msc2967.client:api:* urn:matrix:org.matrix.msc2967.client:device:`; + + const authUrl = new URL(window.location.href); + + expect(authUrl.searchParams.get("response_mode")).toEqual("query"); + expect(authUrl.searchParams.get("response_type")).toEqual("code"); + expect(authUrl.searchParams.get("client_id")).toEqual(clientId); + expect(authUrl.searchParams.get("code_challenge_method")).toEqual("S256"); + + // scope ends with a 10char randomstring deviceId + const scope = authUrl.searchParams.get("scope"); + expect(scope.substring(0, scope.length - 10)).toEqual(expectedScopeWithoutDeviceId); + expect(scope.substring(scope.length - 10)).toBeTruthy(); + + // random string, just check they are set + expect(authUrl.searchParams.has("state")).toBeTruthy(); + expect(authUrl.searchParams.has("nonce")).toBeTruthy(); + expect(authUrl.searchParams.has("code_challenge")).toBeTruthy(); + }); }); - it("navigates to authorization endpoint with correct parameters", async () => { - await startOidcLogin(delegatedAuthConfig, clientId, homeserver); - - const expectedScopeWithoutDeviceId = `openid urn:matrix:org.matrix.msc2967.client:api:* urn:matrix:org.matrix.msc2967.client:device:`; - - const authUrl = new URL(window.location.href); - - expect(authUrl.searchParams.get("response_mode")).toEqual("query"); - expect(authUrl.searchParams.get("response_type")).toEqual("code"); - expect(authUrl.searchParams.get("client_id")).toEqual(clientId); - expect(authUrl.searchParams.get("code_challenge_method")).toEqual("S256"); - - // scope ends with a 10char randomstring deviceId - const scope = authUrl.searchParams.get("scope"); - expect(scope.substring(0, scope.length - 10)).toEqual(expectedScopeWithoutDeviceId); - expect(scope.substring(scope.length - 10)).toBeTruthy(); + describe("completeOidcLogin()", () => { + const authorizationParams = generateAuthorizationParams({ redirectUri: baseUrl }); + const storedAuthorizationParams = { + nonce: authorizationParams.nonce, + redirectUri: baseUrl, + codeVerifier: authorizationParams.codeVerifier, + clientId, + homeserverUrl: homeserver, + identityServerUrl: identityServerUrl, + delegatedAuthConfig: JSON.stringify(delegatedAuthConfig), + }; + const code = "test-code-777"; + const queryDict = { + code, + state: authorizationParams.state, + }; + const validBearerTokenResponse = { + token_type: "Bearer", + access_token: "test_access_token", + refresh_token: "test_refresh_token", + expires_in: 12345, + }; - // random string, just check they are set - expect(authUrl.searchParams.has("state")).toBeTruthy(); - expect(authUrl.searchParams.has("nonce")).toBeTruthy(); - expect(authUrl.searchParams.has("code_challenge")).toBeTruthy(); + beforeEach(() => { + sessionStorageGetSpy.mockImplementation((key: string) => { + const itemKey = key.split(`oidc_${authorizationParams.state}_`).pop(); + return storedAuthorizationParams[itemKey] || null; + }); + + fetchMock.post(delegatedAuthConfig.tokenEndpoint, { + status: 200, + body: validBearerTokenResponse, + }); + }); + + it("should throw when query params do not include state and code", async () => { + expect(async () => await completeOidcLogin({})).rejects.toThrow( + "Invalid query parameters for OIDC native login. `code` and `state` are required.", + ); + }); + + it("should throw when authorization params are not found in session storage", async () => { + const queryDict = { + code: "abc123", + state: "not-the-same-state-we-put-things-in-local-storage-with", + }; + + expect(async () => await completeOidcLogin(queryDict)).rejects.toThrow( + "Cannot complete OIDC login: required properties not found in session storage", + ); + // tried to retreive using state as part of storage key + expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${queryDict.state}_nonce`); + }); + + it("should make request to token endpoint", async () => { + await completeOidcLogin(queryDict); + + expect(fetchMock).toHaveBeenCalledWith(delegatedAuthConfig.tokenEndpoint, { + method: Method.Post, + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + body: `grant_type=authorization_code&client_id=${clientId}&code_verifier=${authorizationParams.codeVerifier}&redirect_uri=https%3A%2F%2Ftest.com&code=${code}`, + }); + }); + + it("should return accessToken, configured homeserver and identityServer", async () => { + const result = await completeOidcLogin(queryDict); + + expect(result).toEqual({ + accessToken: validBearerTokenResponse.access_token, + homeserverUrl: homeserver, + identityServerUrl, + }); + }); + + it("should handle when no identityServer is stored in session storage", async () => { + // session storage mock without identity server stored + const { identityServerUrl: _excludingThis, ...storedParamsWithoutIs } = storedAuthorizationParams; + sessionStorageGetSpy.mockImplementation((key: string) => { + const itemKey = key.split(`oidc_${authorizationParams.state}_`).pop(); + return storedParamsWithoutIs[itemKey] || null; + }); + const result = await completeOidcLogin(queryDict); + + expect(result).toEqual({ + accessToken: validBearerTokenResponse.access_token, + homeserverUrl: homeserver, + identityServerUrl: undefined, + }); + }); }); }); From f4ff6f51f01c2491716eb953b61d0aa010af9f97 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Tue, 27 Jun 2023 12:22:44 +1200 Subject: [PATCH 32/61] test tokenlogin --- src/Lifecycle.ts | 3 +- .../components/structures/MatrixChat-test.tsx | 233 +++++++++++++++--- 2 files changed, 203 insertions(+), 33 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index a1d00d86ad5..b31b3377b00 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -223,6 +223,7 @@ export function attemptTokenLogin( logger.log("Logged in with token"); return clearStorage().then(async (): Promise => { await persistCredentials(creds); + // remember that we just logged in sessionStorage.setItem("mx_fresh_login", String(true)); return true; @@ -664,7 +665,7 @@ async function persistCredentials(credentials: IMatrixClientCreds): Promise => { + // need to wait for different elements depending on which flow + // without security setup we go to a loading page + if (withoutSecuritySetup) { + // we think we are logged in, but are still waiting for the /sync to complete + await screen.findByText("Logout"); + // initial sync + client.emit(ClientEvent.Sync, SyncState.Prepared, null); + // wait for logged in view to load + await screen.findByLabelText("User menu"); + + // otherwise we stay on login and load from there for longer + } else { + // we are logged in, but are still waiting for the /sync to complete + await screen.findByText("Syncing…"); + // initial sync + client.emit(ClientEvent.Sync, SyncState.Prepared, null); + } + + // let things settle + await flushPromises(); + // and some more for good measure + // this proved to be a little flaky + await flushPromises(); +}; + describe("", () => { const userId = "@alice:server.org"; const deviceId = "qwertyui"; @@ -115,7 +141,13 @@ describe("", () => { }; const getComponent = (props: Partial> = {}) => render(); - const localStorageSpy = jest.spyOn(localStorage.__proto__, "getItem").mockReturnValue(undefined); + const localStorageSetSpy = jest.spyOn(localStorage.__proto__, "setItem"); + const localStorageGetSpy = jest.spyOn(localStorage.__proto__, "getItem").mockReturnValue(undefined); + const localStorageClearSpy = jest.spyOn(localStorage.__proto__, "clear"); + const sessionStorageSetSpy = jest.spyOn(sessionStorage.__proto__, "setItem"); + + // make test results readable + filterConsole("Failed to parse localStorage object"); beforeEach(async () => { mockClient = getMockClientWithEventEmitter(getMockClientMethods()); @@ -123,10 +155,14 @@ describe("", () => { unstable_features: {}, versions: [], }); - localStorageSpy.mockReset(); + localStorageGetSpy.mockReset(); + localStorageSetSpy.mockReset(); + sessionStorageSetSpy.mockReset(); jest.spyOn(StorageManager, "idbLoad").mockRestore(); jest.spyOn(StorageManager, "idbSave").mockResolvedValue(undefined); jest.spyOn(defaultDispatcher, "dispatch").mockClear(); + + await clearAllModals(); }); it("should render spinner while app is loading", () => { @@ -150,7 +186,7 @@ describe("", () => { }; beforeEach(() => { - localStorageSpy.mockImplementation((key: unknown) => mockLocalStorage[key as string] || ""); + localStorageGetSpy.mockImplementation((key: unknown) => mockLocalStorage[key as string] || ""); jest.spyOn(StorageManager, "idbLoad").mockImplementation(async (table, key) => { const safeKey = Array.isArray(key) ? key[0] : key; @@ -349,9 +385,6 @@ describe("", () => { const userName = "ernie"; const password = "ilovebert"; - // make test results readable - filterConsole("Failed to parse localStorage object"); - const getComponentAndWaitForReady = async (): Promise => { const renderResult = getComponent(); // wait for welcome page chrome render @@ -367,32 +400,6 @@ describe("", () => { return renderResult; }; - const waitForSyncAndLoad = async (client: MatrixClient, withoutSecuritySetup?: boolean): Promise => { - // need to wait for different elements depending on which flow - // without security setup we go to a loading page - if (withoutSecuritySetup) { - // we think we are logged in, but are still waiting for the /sync to complete - await screen.findByText("Logout"); - // initial sync - client.emit(ClientEvent.Sync, SyncState.Prepared, null); - // wait for logged in view to load - await screen.findByLabelText("User menu"); - - // otherwise we stay on login and load from there for longer - } else { - // we are logged in, but are still waiting for the /sync to complete - await screen.findByText("Syncing…"); - // initial sync - client.emit(ClientEvent.Sync, SyncState.Prepared, null); - } - - // let things settle - await flushPromises(); - // and some more for good measure - // this proved to be a little flaky - await flushPromises(); - }; - const getComponentAndLogin = async (withoutSecuritySetup?: boolean): Promise => { await getComponentAndWaitForReady(); @@ -481,4 +488,166 @@ describe("", () => { }); }); }); + + describe("when query params have a loginToken", () => { + const loginToken = "test-login-token"; + const realQueryParams = { + loginToken, + }; + + const mockLocalStorage: Record = { + mx_sso_hs_url: serverConfig.hsUrl, + mx_sso_is_url: serverConfig.isUrl, + // these are only going to be set during login + mx_hs_url: serverConfig.hsUrl, + mx_is_url: serverConfig.isUrl, + }; + + let loginClient!: ReturnType; + const userId = "@alice:server.org"; + const deviceId = "test-device-id"; + const accessToken = "test-access-token"; + const clientLoginResponse = { + user_id: userId, + device_id: deviceId, + access_token: accessToken, + }; + + beforeEach(() => { + loginClient = getMockClientWithEventEmitter(getMockClientMethods()); + // this is used to create a temporary client during login + jest.spyOn(MatrixJs, "createClient").mockReturnValue(loginClient); + + loginClient.login.mockClear().mockResolvedValue(clientLoginResponse); + + localStorageGetSpy.mockImplementation((key: unknown) => mockLocalStorage[key as string] || ""); + }); + + it("should show an error dialog when no homeserver is found in local storage", async () => { + localStorageGetSpy.mockReturnValue(undefined); + getComponent({ realQueryParams }); + + expect(localStorageGetSpy).toHaveBeenCalledWith("mx_sso_hs_url"); + expect(localStorageGetSpy).toHaveBeenCalledWith("mx_sso_is_url"); + + const dialog = await screen.findByRole("dialog"); + + // warning dialog + expect( + within(dialog).getByText( + "We asked the browser to remember which homeserver you use to let you sign in, " + + "but unfortunately your browser has forgotten it. Go to the sign in page and try again.", + ), + ).toBeInTheDocument(); + }); + + it("should attempt token login", async () => { + getComponent({ realQueryParams }); + + expect(loginClient.login).toHaveBeenCalledWith("m.login.token", { + initial_device_display_name: undefined, + token: loginToken, + }); + }); + + it("should call onTokenLoginCompleted", async () => { + const onTokenLoginCompleted = jest.fn(); + getComponent({ realQueryParams, onTokenLoginCompleted }); + + await flushPromises(); + + expect(onTokenLoginCompleted).toHaveBeenCalled(); + }); + + describe("when login fails", () => { + beforeEach(() => { + loginClient.login.mockRejectedValue(new Error("oups")); + }); + it("should show a dialog", async () => { + getComponent({ realQueryParams }); + + await flushPromises(); + + const dialog = await screen.findByRole("dialog"); + + // warning dialog + expect( + within(dialog).getByText( + "There was a problem communicating with the homeserver, please try again later.", + ), + ).toBeInTheDocument(); + }); + + it("should not clear storage", async () => { + getComponent({ realQueryParams }); + + await flushPromises(); + + expect(loginClient.clearStores).not.toHaveBeenCalled(); + }); + }); + + describe("when login succeeds", () => { + beforeEach(() => { + jest.spyOn(StorageManager, "idbLoad").mockImplementation(async (db: string, key: string) => { + if (key === "mx_access_token") { + return accessToken; + } + }); + }); + it("should clear storage", async () => { + getComponent({ realQueryParams }); + + await flushPromises(); + + // just check we called the clearStorage function + expect(loginClient.clearStores).toHaveBeenCalled(); + expect(localStorageClearSpy).toHaveBeenCalled(); + }); + + it("should persist login credentials", async () => { + getComponent({ realQueryParams }); + + await flushPromises(); + + expect(localStorageSetSpy).toHaveBeenCalledWith("mx_hs_url", serverConfig.hsUrl); + expect(localStorageSetSpy).toHaveBeenCalledWith("mx_user_id", userId); + expect(localStorageSetSpy).toHaveBeenCalledWith("mx_has_access_token", "true"); + expect(localStorageSetSpy).toHaveBeenCalledWith("mx_device_id", deviceId); + }); + + it("should set fresh login flag in session storage", async () => { + getComponent({ realQueryParams }); + + await flushPromises(); + + expect(sessionStorageSetSpy).toHaveBeenCalledWith("mx_fresh_login", "true"); + }); + + it("should override hsUrl in creds when login response wellKnown differs from config", async () => { + const hsUrlFromWk = "https://hsfromwk.org"; + const loginResponseWithWellKnown = { + ...clientLoginResponse, + well_known: { + "m.homeserver": { + base_url: hsUrlFromWk, + }, + }, + }; + loginClient.login.mockResolvedValue(loginResponseWithWellKnown); + getComponent({ realQueryParams }); + + await flushPromises(); + + expect(localStorageSetSpy).toHaveBeenCalledWith("mx_hs_url", hsUrlFromWk); + }); + + it("should continue to post login setup when no session is found in local storage", async () => { + getComponent({ realQueryParams }); + + // logged in but waiting for sync screen + await screen.findByText("Logout"); + }); + }); + }); }); From 599baa2b98300aef00948b98fabbbf390b58c71c Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Tue, 27 Jun 2023 15:59:28 +1200 Subject: [PATCH 33/61] strict --- test/utils/oidc/authorize-test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts index 569b226ab90..5abdb19862a 100644 --- a/test/utils/oidc/authorize-test.ts +++ b/test/utils/oidc/authorize-test.ts @@ -34,7 +34,7 @@ describe("startOidcLogin()", () => { }; const sessionStorageGetSpy = jest.spyOn(sessionStorage.__proto__, "setItem").mockReturnValue(undefined); - const randomStringMockImpl = (length) => new Array(length).fill("x").join(""); + const randomStringMockImpl = (length: number) => new Array(length).fill("x").join(""); // to restore later const realWindowLocation = window.location; @@ -45,6 +45,7 @@ describe("startOidcLogin()", () => { sessionStorageGetSpy.mockClear(); + // @ts-ignore allow delete of non-optional prop delete window.location; // @ts-ignore ugly mocking window.location = { @@ -89,7 +90,7 @@ describe("startOidcLogin()", () => { expect(authUrl.searchParams.get("code_challenge_method")).toEqual("S256"); // scope ends with a 10char randomstring deviceId - const scope = authUrl.searchParams.get("scope"); + const scope = authUrl.searchParams.get("scope")!; expect(scope.substring(0, scope.length - 10)).toEqual(expectedScopeWithoutDeviceId); expect(scope.substring(scope.length - 10)).toBeTruthy(); From 905bac0c6ac39efbb45acd9017038d4c87ae7591 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Tue, 27 Jun 2023 16:06:27 +1200 Subject: [PATCH 34/61] whitespace --- src/Lifecycle.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index b31b3377b00..9a2f6809508 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -223,7 +223,6 @@ export function attemptTokenLogin( logger.log("Logged in with token"); return clearStorage().then(async (): Promise => { await persistCredentials(creds); - // remember that we just logged in sessionStorage.setItem("mx_fresh_login", String(true)); return true; From 7c91acd713ffa4042b15b4638bedf9a613f23f46 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Tue, 27 Jun 2023 16:10:56 +1200 Subject: [PATCH 35/61] tidy --- .../components/structures/MatrixChat-test.tsx | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx index c68f934e636..af8719eb805 100644 --- a/test/components/structures/MatrixChat-test.tsx +++ b/test/components/structures/MatrixChat-test.tsx @@ -37,32 +37,6 @@ import { } from "../../test-utils"; import * as leaveRoomUtils from "../../../src/utils/leave-behaviour"; -const waitForSyncAndLoad = async (client: MatrixClient, withoutSecuritySetup?: boolean): Promise => { - // need to wait for different elements depending on which flow - // without security setup we go to a loading page - if (withoutSecuritySetup) { - // we think we are logged in, but are still waiting for the /sync to complete - await screen.findByText("Logout"); - // initial sync - client.emit(ClientEvent.Sync, SyncState.Prepared, null); - // wait for logged in view to load - await screen.findByLabelText("User menu"); - - // otherwise we stay on login and load from there for longer - } else { - // we are logged in, but are still waiting for the /sync to complete - await screen.findByText("Syncing…"); - // initial sync - client.emit(ClientEvent.Sync, SyncState.Prepared, null); - } - - // let things settle - await flushPromises(); - // and some more for good measure - // this proved to be a little flaky - await flushPromises(); -}; - describe("", () => { const userId = "@alice:server.org"; const deviceId = "qwertyui"; @@ -400,6 +374,32 @@ describe("", () => { return renderResult; }; + const waitForSyncAndLoad = async (client: MatrixClient, withoutSecuritySetup?: boolean): Promise => { + // need to wait for different elements depending on which flow + // without security setup we go to a loading page + if (withoutSecuritySetup) { + // we think we are logged in, but are still waiting for the /sync to complete + await screen.findByText("Logout"); + // initial sync + client.emit(ClientEvent.Sync, SyncState.Prepared, null); + // wait for logged in view to load + await screen.findByLabelText("User menu"); + + // otherwise we stay on login and load from there for longer + } else { + // we are logged in, but are still waiting for the /sync to complete + await screen.findByText("Syncing…"); + // initial sync + client.emit(ClientEvent.Sync, SyncState.Prepared, null); + } + + // let things settle + await flushPromises(); + // and some more for good measure + // this proved to be a little flaky + await flushPromises(); + }; + const getComponentAndLogin = async (withoutSecuritySetup?: boolean): Promise => { await getComponentAndWaitForReady(); From 5ba262c8a8c2b6c0cb5edac1a05ddfc6c4010976 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Tue, 27 Jun 2023 17:44:03 +1200 Subject: [PATCH 36/61] unit test oidc login flow in MatrixChat --- src/Lifecycle.ts | 2 +- src/components/structures/MatrixChat.tsx | 11 +- src/utils/oidc/authorize.ts | 4 +- .../components/structures/MatrixChat-test.tsx | 248 +++++++++++++++++- 4 files changed, 253 insertions(+), 12 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index e53e460b8df..b14fd91344e 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -258,7 +258,7 @@ async function getUserIdFromAccessToken( return await client.whoami(); } catch (error) { - console.error("Failed to retrieve userId using accessToken", error); + logger.error("Failed to retrieve userId using accessToken", error); throw new Error("Failed to retrieve userId using accessToken"); } } diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index c7096390559..46c4f19ae10 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -320,13 +320,15 @@ export default class MatrixChat extends React.PureComponent { this.props.defaultDeviceDisplayName, this.getFragmentAfterLogin(), ).then(async (loggedIn): Promise => { - if (this.props.realQueryParams?.loginToken) { - // remove the loginToken from the URL regardless + if ( + this.props.realQueryParams?.loginToken || + this.props.realQueryParams?.code || + this.props.realQueryParams?.state + ) { + // remove the loginToken or auth code from the URL regardless this.props.onTokenLoginCompleted(); } - // @TODO(kerrya) remove OIDC code and state from URL - if (loggedIn) { this.tokenLogin = true; @@ -340,7 +342,6 @@ export default class MatrixChat extends React.PureComponent { // if the user has followed a login or register link, don't reanimate // the old creds, but rather go straight to the relevant page const firstScreen = this.screenAfterLogin ? this.screenAfterLogin.screen : null; - const restoreSuccess = await this.loadSession(); if (restoreSuccess) { return true; diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index d19145dfc78..4e39984b9ce 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -81,7 +81,7 @@ const retrieveAuthorizationParams = (state: string): StoredAuthorizationParams = const clientId = window.sessionStorage.getItem(`oidc_${state}_clientId`); const homeserverUrl = window.sessionStorage.getItem(`oidc_${state}_homeserverUrl`); const identityServerUrl = window.sessionStorage.getItem(`oidc_${state}_identityServerUrl`) ?? undefined; - const delegatedAuthConfig = JSON.parse(window.sessionStorage.getItem(`oidc_${state}_delegatedAuthConfig`)); + const delegatedAuthConfig = window.sessionStorage.getItem(`oidc_${state}_delegatedAuthConfig`); return validateStoredAuthorizationParams({ nonce, @@ -90,7 +90,7 @@ const retrieveAuthorizationParams = (state: string): StoredAuthorizationParams = clientId, homeserverUrl, identityServerUrl, - delegatedAuthConfig, + delegatedAuthConfig: delegatedAuthConfig ? JSON.parse(delegatedAuthConfig) : undefined, }); }; diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx index af8719eb805..67cfdee077a 100644 --- a/test/components/structures/MatrixChat-test.tsx +++ b/test/components/structures/MatrixChat-test.tsx @@ -16,12 +16,15 @@ limitations under the License. import React, { ComponentProps } from "react"; import { fireEvent, render, RenderResult, screen, within } from "@testing-library/react"; -import fetchMockJest from "fetch-mock-jest"; +import fetchMock from "fetch-mock-jest"; import { ClientEvent, MatrixClient } from "matrix-js-sdk/src/client"; import { SyncState } from "matrix-js-sdk/src/sync"; import { MediaHandler } from "matrix-js-sdk/src/webrtc/mediaHandler"; import * as MatrixJs from "matrix-js-sdk/src/matrix"; import { MatrixEvent, Room } from "matrix-js-sdk/src/matrix"; +import { generateAuthorizationParams } from "matrix-js-sdk/src/oidc/authorize"; +import { logger } from "matrix-js-sdk/src/logger"; +import { OidcError } from "matrix-js-sdk/src/oidc/error"; import MatrixChat from "../../../src/components/structures/MatrixChat"; import * as StorageManager from "../../../src/utils/StorageManager"; @@ -64,6 +67,7 @@ describe("", () => { setAccountData: jest.fn(), store: { destroy: jest.fn(), + startup: jest.fn(), }, login: jest.fn(), loginFlows: jest.fn(), @@ -85,6 +89,7 @@ describe("", () => { isStored: jest.fn().mockReturnValue(null), }, getDehydratedDevice: jest.fn(), + whoami: jest.fn(), }); let mockClient = getMockClientWithEventEmitter(getMockClientMethods()); const serverConfig = { @@ -119,20 +124,21 @@ describe("", () => { const localStorageGetSpy = jest.spyOn(localStorage.__proto__, "getItem").mockReturnValue(undefined); const localStorageClearSpy = jest.spyOn(localStorage.__proto__, "clear"); const sessionStorageSetSpy = jest.spyOn(sessionStorage.__proto__, "setItem"); + const sessionStorageGetSpy = jest.spyOn(sessionStorage.__proto__, "getItem"); // make test results readable filterConsole("Failed to parse localStorage object"); beforeEach(async () => { mockClient = getMockClientWithEventEmitter(getMockClientMethods()); - fetchMockJest.get("https://test.com/_matrix/client/versions", { + fetchMock.get("https://test.com/_matrix/client/versions", { unstable_features: {}, versions: [], }); localStorageGetSpy.mockReset(); localStorageSetSpy.mockReset(); sessionStorageSetSpy.mockReset(); - jest.spyOn(StorageManager, "idbLoad").mockRestore(); + jest.spyOn(StorageManager, "idbLoad").mockReset(); jest.spyOn(StorageManager, "idbSave").mockResolvedValue(undefined); jest.spyOn(defaultDispatcher, "dispatch").mockClear(); @@ -415,7 +421,7 @@ describe("", () => { beforeEach(() => { loginClient = getMockClientWithEventEmitter(getMockClientMethods()); // this is used to create a temporary client during login - jest.spyOn(MatrixJs, "createClient").mockReturnValue(loginClient); + jest.spyOn(MatrixJs, "createClient").mockClear().mockReturnValue(loginClient); loginClient.login.mockClear().mockResolvedValue({}); loginClient.loginFlows.mockClear().mockResolvedValue({ flows: [{ type: "m.login.password" }] }); @@ -650,4 +656,238 @@ describe("", () => { }); }); }); + + describe("when query params have a OIDC params", () => { + const issuer = "https://auth.com/"; + const authorizationEndpoint = "https://auth.com/authorization"; + const homeserver = "https://matrix.org"; + const identityServerUrl = "https://is.org"; + const clientId = "xyz789"; + const baseUrl = "https://test.com"; + + const delegatedAuthConfig = { + issuer, + registrationEndpoint: issuer + "registration", + authorizationEndpoint, + tokenEndpoint: issuer + "token", + }; + const authorizationParams = generateAuthorizationParams({ redirectUri: baseUrl }); + const storedAuthorizationParams = { + nonce: authorizationParams.nonce, + redirectUri: baseUrl, + codeVerifier: authorizationParams.codeVerifier, + clientId, + homeserverUrl: homeserver, + identityServerUrl, + delegatedAuthConfig: JSON.stringify(delegatedAuthConfig), + }; + const code = "test-oidc-auth-code"; + const realQueryParams = { + code, + state: authorizationParams.state, + }; + + const mockLocalStorage: Record = { + mx_sso_hs_url: serverConfig.hsUrl, + mx_sso_is_url: serverConfig.isUrl, + // these are only going to be set during login + mx_hs_url: serverConfig.hsUrl, + mx_is_url: serverConfig.isUrl, + }; + + let loginClient!: ReturnType; + const userId = "@alice:server.org"; + const deviceId = "test-device-id"; + const accessToken = "test-access-token-from-oidc"; + + const validBearerTokenResponse = { + token_type: "Bearer", + access_token: accessToken, + refresh_token: "test_refresh_token", + expires_in: 12345, + }; + + // for now when OIDC fails for any reason we just bump back to welcome + // error handling screens in https://github.com/vector-im/element-web/issues/25665 + const expectOIDCError = async (): Promise => { + await flushPromises(); + // just check we're back on welcome page + expect(document.querySelector(".mx_Welcome")!).toBeInTheDocument(); + }; + + beforeEach(() => { + loginClient = getMockClientWithEventEmitter(getMockClientMethods()); + // this is used to create a temporary client during login + jest.spyOn(MatrixJs, "createClient").mockReturnValue(loginClient); + + jest.spyOn(logger, "error").mockClear(); + jest.spyOn(logger, "log").mockClear(); + + localStorageGetSpy.mockImplementation((key: unknown) => mockLocalStorage[key as string] || ""); + sessionStorageGetSpy.mockImplementation((key: string) => { + const itemKey = key.split(`oidc_${authorizationParams.state}_`).pop(); + return storedAuthorizationParams[itemKey] || null; + }); + + fetchMock.resetHistory(); + fetchMock.post( + delegatedAuthConfig.tokenEndpoint, + { + status: 200, + body: validBearerTokenResponse, + }, + { overwriteRoutes: true }, + ); + + loginClient.whoami.mockResolvedValue({ + user_id: userId, + device_id: deviceId, + is_guest: false, + }); + }); + + it("should fail when authorization params are not found in session storage", async () => { + sessionStorageGetSpy.mockReturnValue(undefined); + getComponent({ realQueryParams }); + + await flushPromises(); + + expect(logger.error).toHaveBeenCalledWith( + "Failed to login via OIDC", + new Error("Cannot complete OIDC login: required properties not found in session storage"), + ); + + await expectOIDCError(); + }); + + it("should attempt to get access token", async () => { + getComponent({ realQueryParams }); + + expect(fetchMock).toHaveFetched(delegatedAuthConfig.tokenEndpoint); + }); + + it("should look up userId using access token", async () => { + getComponent({ realQueryParams }); + + await flushPromises(); + + // check we used a client with the correct accesstoken + expect(MatrixJs.createClient).toHaveBeenCalledWith({ + baseUrl: homeserver, + accessToken, + idBaseUrl: identityServerUrl, + }); + expect(loginClient.whoami).toHaveBeenCalled(); + }); + + it("should log error and return to welcome page when userId lookup fails", async () => { + loginClient.whoami.mockRejectedValue(new Error("oups")); + getComponent({ realQueryParams }); + + await flushPromises(); + + expect(logger.error).toHaveBeenCalledWith( + "Failed to login via OIDC", + new Error("Failed to retrieve userId using accessToken"), + ); + await expectOIDCError(); + }); + + it("should call onTokenLoginCompleted", async () => { + const onTokenLoginCompleted = jest.fn(); + getComponent({ realQueryParams, onTokenLoginCompleted }); + + await flushPromises(); + + expect(onTokenLoginCompleted).toHaveBeenCalled(); + }); + + describe("when login fails", () => { + beforeEach(() => { + fetchMock.post( + delegatedAuthConfig.tokenEndpoint, + { + status: 500, + }, + { overwriteRoutes: true }, + ); + }); + + it("should log and return to welcome page", async () => { + getComponent({ realQueryParams }); + + await flushPromises(); + + expect(logger.error).toHaveBeenCalledWith( + "Failed to login via OIDC", + new Error(OidcError.CodeExchangeFailed), + ); + + // warning dialog + await expectOIDCError(); + }); + + it("should not clear storage", async () => { + getComponent({ realQueryParams }); + + await flushPromises(); + + expect(loginClient.clearStores).not.toHaveBeenCalled(); + }); + }); + + describe("when login succeeds", () => { + beforeEach(() => { + // jest.spyOn(StorageManager, "idbLoad").mockImplementation(async (db: string, key: string) => { + // if (key === "mx_access_token") { + // return accessToken; + // } + // }); + }); + it("should clear storage", async () => { + getComponent({ realQueryParams }); + + await flushPromises(); + await flushPromises(); + + // just check we called the clearStorage function + expect(loginClient.clearStores).toHaveBeenCalled(); + expect(localStorageClearSpy).toHaveBeenCalled(); + }); + + it("should persist login credentials", async () => { + getComponent({ realQueryParams }); + + await flushPromises(); + + expect(localStorageSetSpy).toHaveBeenCalledWith("mx_hs_url", storedAuthorizationParams.homeserverUrl); + expect(localStorageSetSpy).toHaveBeenCalledWith("mx_user_id", userId); + expect(localStorageSetSpy).toHaveBeenCalledWith("mx_has_access_token", "true"); + expect(localStorageSetSpy).toHaveBeenCalledWith("mx_device_id", deviceId); + }); + + it("should set logged in and start MatrixClient", async () => { + getComponent({ realQueryParams }); + + await flushPromises(); + await flushPromises(); + + expect(logger.log).toHaveBeenCalledWith( + "setLoggedIn: mxid: " + + userId + + " deviceId: " + + deviceId + + " guest: " + + false + + " hs: " + + storedAuthorizationParams.homeserverUrl + + " softLogout: " + + false, + " freshLogin: " + undefined, + ); + + expect(defaultDispatcher.dispatch).toHaveBeenCalledWith({ action: "client_started" }); + }); + }); + }); }); From 94167e5c2b932a5fefd69071fa0135acefa856bc Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Tue, 27 Jun 2023 18:49:17 +1200 Subject: [PATCH 37/61] strict --- test/components/structures/MatrixChat-test.tsx | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx index af8719eb805..d370a98ef09 100644 --- a/test/components/structures/MatrixChat-test.tsx +++ b/test/components/structures/MatrixChat-test.tsx @@ -589,11 +589,13 @@ describe("", () => { describe("when login succeeds", () => { beforeEach(() => { - jest.spyOn(StorageManager, "idbLoad").mockImplementation(async (db: string, key: string) => { - if (key === "mx_access_token") { - return accessToken; - } - }); + jest.spyOn(StorageManager, "idbLoad").mockImplementation( + async (_table: string, key: string | string[]) => { + if (key === "mx_access_token") { + return accessToken as any; + } + }, + ); }); it("should clear storage", async () => { getComponent({ realQueryParams }); From 88e0cf2cfa7021603f65d954f8499badeeea55ff Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 28 Jun 2023 15:05:30 +1200 Subject: [PATCH 38/61] tidy --- src/utils/oidc/authorize.ts | 8 +++++--- src/utils/oidc/error.ts | 21 --------------------- 2 files changed, 5 insertions(+), 24 deletions(-) delete mode 100644 src/utils/oidc/error.ts diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 08be4fca175..22e7a11bce1 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -24,8 +24,8 @@ import { ValidatedDelegatedAuthConfig } from "../ValidatedServerConfig"; /** * Store authorization params for retrieval when returning from OIDC OP - * @param { AuthorizationParams } authorizationParams - * @param { ValidatedServerConfig["delegatedAuthentication"] } delegatedAuthConfig used for future interactions with OP + * @param authorizationParams from `generateAuthorizationParams` + * @param delegatedAuthConfig used for future interactions with OP * @param clientId this client's id as registered with configured issuer * @param homeserver target homeserver */ @@ -45,17 +45,19 @@ const storeAuthorizationParams = ( /** * Start OIDC authorization code flow + * Generates auth params, stores them in session storage and * Navigates to configured authorization endpoint * @param delegatedAuthConfig from discovery * @param clientId this client's id as registered with configured issuer * @param homeserver target homeserver + * @returns Promise that resolves after we have navigated to auth endpoint */ export const startOidcLogin = async ( delegatedAuthConfig: ValidatedDelegatedAuthConfig, clientId: string, homeserver: string, ): Promise => { - // TODO(kerrya) afterloginfragment + // TODO(kerrya) afterloginfragment https://github.com/vector-im/element-web/issues/25656 const redirectUri = window.location.origin; const authParams = generateAuthorizationParams({ redirectUri }); diff --git a/src/utils/oidc/error.ts b/src/utils/oidc/error.ts deleted file mode 100644 index 3604ee77c24..00000000000 --- a/src/utils/oidc/error.ts +++ /dev/null @@ -1,21 +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. -*/ - -export enum OidcClientError { - DynamicRegistrationNotSupported = "Dynamic registration not supported", - DynamicRegistrationFailed = "Dynamic registration failed", - DynamicRegistrationInvalid = "Dynamic registration invalid response", -} From 26a745f850166a55296f5a7c3c97d9f6921f851b Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 28 Jun 2023 16:02:35 +1200 Subject: [PATCH 39/61] extract success/failure handlers from token login function --- src/Lifecycle.ts | 81 ++++++++++++++++-------- src/components/structures/MatrixChat.tsx | 2 + 2 files changed, 55 insertions(+), 28 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index 9a2f6809508..ba01c2cea2d 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -20,6 +20,7 @@ limitations under the License. import { createClient } from "matrix-js-sdk/src/matrix"; import { InvalidStoreError } from "matrix-js-sdk/src/errors"; import { MatrixClient } from "matrix-js-sdk/src/client"; +import { MatrixError } from "matrix-js-sdk/src/http-api"; import { decryptAES, encryptAES, IEncryptedPayload } from "matrix-js-sdk/src/crypto/aes"; import { QueryDict } from "matrix-js-sdk/src/utils"; import { logger } from "matrix-js-sdk/src/logger"; @@ -219,40 +220,64 @@ export function attemptTokenLogin( token: queryParams.loginToken as string, initial_device_display_name: defaultDeviceDisplayName, }) - .then(function (creds) { + .then(async function (creds) { logger.log("Logged in with token"); - return clearStorage().then(async (): Promise => { - await persistCredentials(creds); - // remember that we just logged in - sessionStorage.setItem("mx_fresh_login", String(true)); - return true; - }); + await onSuccessfulDelegatedAuthLogin(creds); + return true; }) - .catch((err) => { - Modal.createDialog(ErrorDialog, { - title: _t("We couldn't log you in"), - description: messageForLoginError(err, { - hsUrl: homeserver, - hsName: homeserver, - }), - button: _t("Try again"), - onFinished: (tryAgain) => { - if (tryAgain) { - const cli = createClient({ - baseUrl: homeserver, - idBaseUrl: identityServer, - }); - const idpId = localStorage.getItem(SSO_IDP_ID_KEY) || undefined; - PlatformPeg.get()?.startSingleSignOn(cli, "sso", fragmentAfterLogin, idpId, SSOAction.LOGIN); - } - }, - }); - logger.error("Failed to log in with login token:"); - logger.error(err); + .catch((error) => { + const tryAgainCallback = (tryAgain): void => { + if (tryAgain) { + const cli = createClient({ + baseUrl: homeserver, + idBaseUrl: identityServer, + }); + const idpId = localStorage.getItem(SSO_IDP_ID_KEY) || undefined; + PlatformPeg.get()?.startSingleSignOn(cli, "sso", fragmentAfterLogin, idpId, SSOAction.LOGIN); + } + }; + onFailedDelegatedAuthLogin(error, homeserver, tryAgainCallback); + logger.error("Failed to log in with login token:", error); return false; }); } +/** + * After a succesful token login or OIDC authorization + * Clear storage then save new credentials in storage + * @param credentials as returned from login + */ +async function onSuccessfulDelegatedAuthLogin(credentials: IMatrixClientCreds): Promise { + await clearStorage(); + await persistCredentials(credentials); + + // remember that we just logged in + sessionStorage.setItem("mx_fresh_login", String(true)); +} + +type TryAgainFunction = (shouldTryAgain?: boolean) => void; +/** + * Display a friendly error to the user when token login or OIDC authorization fails + * @param error + * @param homeserverUrl + * @param tryAgain OPTIONAL function to call on try again button from error dialog + */ +async function onFailedDelegatedAuthLogin( + error: MatrixError, + homeserverUrl: string, + tryAgain?: TryAgainFunction, +): Promise { + Modal.createDialog(ErrorDialog, { + title: _t("We couldn't log you in"), + description: messageForLoginError(error, { + hsUrl: homeserverUrl, + hsName: homeserverUrl, + }), + button: _t("Try again"), + onFinished: tryAgain, + }); +} + export function handleInvalidStoreError(e: InvalidStoreError): Promise | void { if (e.reason === InvalidStoreError.TOGGLED_LAZY_LOADING) { return Promise.resolve() diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index 40b13eef671..1edacb77de3 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -330,6 +330,8 @@ export default class MatrixChat extends React.PureComponent { this.tokenLogin = true; // Create and start the client + // accesses the new credentials just set in storage during attemptTokenLogin + // and sets logged in state await Lifecycle.restoreFromLocalStorage({ ignoreGuest: true, }); From 38ecd446d400e77c19d50c5e842f27035dc2869a Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 28 Jun 2023 16:05:24 +1200 Subject: [PATCH 40/61] typo --- src/Lifecycle.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index ba01c2cea2d..8c0496d58c3 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -243,7 +243,7 @@ export function attemptTokenLogin( } /** - * After a succesful token login or OIDC authorization + * After a successful token login or OIDC authorization * Clear storage then save new credentials in storage * @param credentials as returned from login */ @@ -260,12 +260,12 @@ type TryAgainFunction = (shouldTryAgain?: boolean) => void; * Display a friendly error to the user when token login or OIDC authorization fails * @param error * @param homeserverUrl - * @param tryAgain OPTIONAL function to call on try again button from error dialog + * @param tryAgain function to call on try again button from error dialog */ async function onFailedDelegatedAuthLogin( error: MatrixError, homeserverUrl: string, - tryAgain?: TryAgainFunction, + tryAgain: TryAgainFunction, ): Promise { Modal.createDialog(ErrorDialog, { title: _t("We couldn't log you in"), From 4890e66e5d4e145e8d3c6c6deb986045e22b2ba1 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 28 Jun 2023 16:29:20 +1200 Subject: [PATCH 41/61] use for no homeserver error dialog too --- src/Lifecycle.ts | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index 8c0496d58c3..8a44940a196 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -17,10 +17,10 @@ See the License for the specific language governing permissions and limitations under the License. */ +import { ReactNode } from "react"; import { createClient } from "matrix-js-sdk/src/matrix"; import { InvalidStoreError } from "matrix-js-sdk/src/errors"; import { MatrixClient } from "matrix-js-sdk/src/client"; -import { MatrixError } from "matrix-js-sdk/src/http-api"; import { decryptAES, encryptAES, IEncryptedPayload } from "matrix-js-sdk/src/crypto/aes"; import { QueryDict } from "matrix-js-sdk/src/utils"; import { logger } from "matrix-js-sdk/src/logger"; @@ -205,6 +205,12 @@ export function attemptTokenLogin( const identityServer = localStorage.getItem(SSO_ID_SERVER_URL_KEY) ?? undefined; if (!homeserver) { logger.warn("Cannot log in with token: can't determine HS URL to use"); + onFailedDelegatedAuthLogin( + _t( + "We asked the browser to remember which homeserver you use to let you sign in, " + + "but unfortunately your browser has forgotten it. Go to the sign in page and try again.", + ), + ); Modal.createDialog(ErrorDialog, { title: _t("We couldn't log you in"), description: _t( @@ -226,7 +232,7 @@ export function attemptTokenLogin( return true; }) .catch((error) => { - const tryAgainCallback = (tryAgain): void => { + const tryAgainCallback: TryAgainFunction = (tryAgain) => { if (tryAgain) { const cli = createClient({ baseUrl: homeserver, @@ -236,7 +242,13 @@ export function attemptTokenLogin( PlatformPeg.get()?.startSingleSignOn(cli, "sso", fragmentAfterLogin, idpId, SSOAction.LOGIN); } }; - onFailedDelegatedAuthLogin(error, homeserver, tryAgainCallback); + onFailedDelegatedAuthLogin( + messageForLoginError(error, { + hsUrl: homeserver, + hsName: homeserver, + }), + tryAgainCallback, + ); logger.error("Failed to log in with login token:", error); return false; }); @@ -258,21 +270,13 @@ async function onSuccessfulDelegatedAuthLogin(credentials: IMatrixClientCreds): type TryAgainFunction = (shouldTryAgain?: boolean) => void; /** * Display a friendly error to the user when token login or OIDC authorization fails - * @param error - * @param homeserverUrl - * @param tryAgain function to call on try again button from error dialog + * @param description error description + * @param tryAgain OPTIONAL function to call on try again button from error dialog */ -async function onFailedDelegatedAuthLogin( - error: MatrixError, - homeserverUrl: string, - tryAgain: TryAgainFunction, -): Promise { +async function onFailedDelegatedAuthLogin(description: string | ReactNode, tryAgain?: TryAgainFunction): Promise { Modal.createDialog(ErrorDialog, { title: _t("We couldn't log you in"), - description: messageForLoginError(error, { - hsUrl: homeserverUrl, - hsName: homeserverUrl, - }), + description, button: _t("Try again"), onFinished: tryAgain, }); From 225e4f5dc250e6c8f900188b452e12743e03b424 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 28 Jun 2023 18:13:30 +1200 Subject: [PATCH 42/61] reuse post-token login functions, test --- src/Lifecycle.ts | 8 ++-- src/i18n/strings/en_EN.json | 3 +- .../components/structures/MatrixChat-test.tsx | 47 ++++++++++--------- 3 files changed, 31 insertions(+), 27 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index 2b74a57c76b..fc5bf9dd333 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -227,12 +227,14 @@ async function attemptOidcNativeLogin(queryParams: QueryDict): Promise isGuest, }; - // @TODO(kerrya) should clear storage? - // @TODO(kerrya) other token login doesn't use this codepath, why? - await doSetLoggedIn(credentials, true); + logger.debug("Logged in via OIDC native flow"); + await onSuccessfulDelegatedAuthLogin(credentials); return true; } catch (error) { logger.error("Failed to login via OIDC", error); + + // TODO(kerrya) nice error messages https://github.com/vector-im/element-web/issues/25665 + await onFailedDelegatedAuthLogin(_t("Something went wrong.")); return false; } } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index d9f74d40a56..bb72f64203d 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -101,8 +101,9 @@ "Failed to transfer call": "Failed to transfer call", "Permission Required": "Permission Required", "You do not have permission to start a conference call in this room": "You do not have permission to start a conference call in this room", - "We couldn't log you in": "We couldn't log you in", + "Something went wrong.": "Something went wrong.", "We asked the browser to remember which homeserver you use to let you sign in, but unfortunately your browser has forgotten it. Go to the sign in page and try again.": "We asked the browser to remember which homeserver you use to let you sign in, but unfortunately your browser has forgotten it. Go to the sign in page and try again.", + "We couldn't log you in": "We couldn't log you in", "Try again": "Try again", "User is not logged in": "User is not logged in", "Database unexpectedly closed": "Database unexpectedly closed", diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx index d48cf3093da..460043956ef 100644 --- a/test/components/structures/MatrixChat-test.tsx +++ b/test/components/structures/MatrixChat-test.tsx @@ -743,18 +743,19 @@ describe("", () => { state: authorizationParams.state, }; + const userId = "@alice:server.org"; + const deviceId = "test-device-id"; + const accessToken = "test-access-token-from-oidc"; + const mockLocalStorage: Record = { - mx_sso_hs_url: serverConfig.hsUrl, - mx_sso_is_url: serverConfig.isUrl, // these are only going to be set during login - mx_hs_url: serverConfig.hsUrl, - mx_is_url: serverConfig.isUrl, + mx_hs_url: homeserver, + mx_is_url: identityServerUrl, + mx_user_id: userId, + mx_device_id: deviceId, }; let loginClient!: ReturnType; - const userId = "@alice:server.org"; - const deviceId = "test-device-id"; - const accessToken = "test-access-token-from-oidc"; const validBearerTokenResponse = { token_type: "Bearer", @@ -894,21 +895,13 @@ describe("", () => { describe("when login succeeds", () => { beforeEach(() => { - // jest.spyOn(StorageManager, "idbLoad").mockImplementation(async (db: string, key: string) => { - // if (key === "mx_access_token") { - // return accessToken; - // } - // }); - }); - it("should clear storage", async () => { - getComponent({ realQueryParams }); - - await flushPromises(); - await flushPromises(); - - // just check we called the clearStorage function - expect(loginClient.clearStores).toHaveBeenCalled(); - expect(localStorageClearSpy).toHaveBeenCalled(); + localStorageGetSpy.mockImplementation((key: unknown) => mockLocalStorage[key as string] || ""); + jest.spyOn(StorageManager, "idbLoad").mockImplementation( + async (_table: string, key: string | string[]) => (key === "mx_access_token" ? accessToken : null), + ); + loginClient.getProfileInfo.mockResolvedValue({ + displayname: "Ernie", + }); }); it("should persist login credentials", async () => { @@ -939,10 +932,18 @@ describe("", () => { storedAuthorizationParams.homeserverUrl + " softLogout: " + false, - " freshLogin: " + undefined, + " freshLogin: " + false, ); + // client successfully started expect(defaultDispatcher.dispatch).toHaveBeenCalledWith({ action: "client_started" }); + + // we think we are logged in, but are still waiting for the /sync to complete + await screen.findByText("Logout"); + // initial sync + loginClient.emit(ClientEvent.Sync, SyncState.Prepared, null); + // logged in! + await screen.findByLabelText("User menu"); }); }); }); From c7199e5e44bdf450b1cf566d15926b87c81ed017 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 28 Jun 2023 18:18:52 +1200 Subject: [PATCH 43/61] shuffle testing utils around --- .../components/structures/MatrixChat-test.tsx | 71 ++++++++++--------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx index 460043956ef..c970b1bd987 100644 --- a/test/components/structures/MatrixChat-test.tsx +++ b/test/components/structures/MatrixChat-test.tsx @@ -130,6 +130,37 @@ describe("", () => { // make test results readable filterConsole("Failed to parse localStorage object"); + /** + * Wait for a bunch of stuff to happen + * between deciding we are logged in and removing the spinner + * including waiting for initial sync + */ + const waitForSyncAndLoad = async (client: MatrixClient, withoutSecuritySetup?: boolean): Promise => { + // need to wait for different elements depending on which flow + // without security setup we go to a loading page + if (withoutSecuritySetup) { + // we think we are logged in, but are still waiting for the /sync to complete + await screen.findByText("Logout"); + // initial sync + client.emit(ClientEvent.Sync, SyncState.Prepared, null); + // wait for logged in view to load + await screen.findByLabelText("User menu"); + + // otherwise we stay on login and load from there for longer + } else { + // we are logged in, but are still waiting for the /sync to complete + await screen.findByText("Syncing…"); + // initial sync + client.emit(ClientEvent.Sync, SyncState.Prepared, null); + } + + // let things settle + await flushPromises(); + // and some more for good measure + // this proved to be a little flaky + await flushPromises(); + }; + beforeEach(async () => { mockClient = getMockClientWithEventEmitter(getMockClientMethods()); fetchMock.get("https://test.com/_matrix/client/versions", { @@ -381,32 +412,6 @@ describe("", () => { return renderResult; }; - const waitForSyncAndLoad = async (client: MatrixClient, withoutSecuritySetup?: boolean): Promise => { - // need to wait for different elements depending on which flow - // without security setup we go to a loading page - if (withoutSecuritySetup) { - // we think we are logged in, but are still waiting for the /sync to complete - await screen.findByText("Logout"); - // initial sync - client.emit(ClientEvent.Sync, SyncState.Prepared, null); - // wait for logged in view to load - await screen.findByLabelText("User menu"); - - // otherwise we stay on login and load from there for longer - } else { - // we are logged in, but are still waiting for the /sync to complete - await screen.findByText("Syncing…"); - // initial sync - client.emit(ClientEvent.Sync, SyncState.Prepared, null); - } - - // let things settle - await flushPromises(); - // and some more for good measure - // this proved to be a little flaky - await flushPromises(); - }; - const getComponentAndLogin = async (withoutSecuritySetup?: boolean): Promise => { await getComponentAndWaitForReady(); @@ -938,12 +943,14 @@ describe("", () => { // client successfully started expect(defaultDispatcher.dispatch).toHaveBeenCalledWith({ action: "client_started" }); - // we think we are logged in, but are still waiting for the /sync to complete - await screen.findByText("Logout"); - // initial sync - loginClient.emit(ClientEvent.Sync, SyncState.Prepared, null); - // logged in! - await screen.findByLabelText("User menu"); + await waitForSyncAndLoad(loginClient, true); + + // // we think we are logged in, but are still waiting for the /sync to complete + // await screen.findByText("Logout"); + // // initial sync + // loginClient.emit(ClientEvent.Sync, SyncState.Prepared, null); + // // logged in! + // await screen.findByLabelText("User menu"); }); }); }); From 56e93aeab25cdd5d0fbe67fe13a7566ef08dbe2f Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 28 Jun 2023 18:19:41 +1200 Subject: [PATCH 44/61] shuffle testing utils around --- test/components/structures/MatrixChat-test.tsx | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx index c970b1bd987..7413a3d05de 100644 --- a/test/components/structures/MatrixChat-test.tsx +++ b/test/components/structures/MatrixChat-test.tsx @@ -943,14 +943,8 @@ describe("", () => { // client successfully started expect(defaultDispatcher.dispatch).toHaveBeenCalledWith({ action: "client_started" }); + // check we get to logged in view await waitForSyncAndLoad(loginClient, true); - - // // we think we are logged in, but are still waiting for the /sync to complete - // await screen.findByText("Logout"); - // // initial sync - // loginClient.emit(ClientEvent.Sync, SyncState.Prepared, null); - // // logged in! - // await screen.findByLabelText("User menu"); }); }); }); From b9792bc40d8a96aeac7d6e8e713ea7aed6a32a46 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 28 Jun 2023 18:20:41 +1200 Subject: [PATCH 45/61] i18n --- src/i18n/strings/en_EN.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index d9f74d40a56..6afdce301e5 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -101,8 +101,8 @@ "Failed to transfer call": "Failed to transfer call", "Permission Required": "Permission Required", "You do not have permission to start a conference call in this room": "You do not have permission to start a conference call in this room", - "We couldn't log you in": "We couldn't log you in", "We asked the browser to remember which homeserver you use to let you sign in, but unfortunately your browser has forgotten it. Go to the sign in page and try again.": "We asked the browser to remember which homeserver you use to let you sign in, but unfortunately your browser has forgotten it. Go to the sign in page and try again.", + "We couldn't log you in": "We couldn't log you in", "Try again": "Try again", "User is not logged in": "User is not logged in", "Database unexpectedly closed": "Database unexpectedly closed", From a9ed7915d7f413b6ead15d673e3c380295989f7d Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 29 Jun 2023 08:44:23 +1200 Subject: [PATCH 46/61] tidy --- src/Lifecycle.ts | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index 8a44940a196..dbf908efae1 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -211,14 +211,6 @@ export function attemptTokenLogin( "but unfortunately your browser has forgotten it. Go to the sign in page and try again.", ), ); - Modal.createDialog(ErrorDialog, { - title: _t("We couldn't log you in"), - description: _t( - "We asked the browser to remember which homeserver you use to let you sign in, " + - "but unfortunately your browser has forgotten it. Go to the sign in page and try again.", - ), - button: _t("Try again"), - }); return Promise.resolve(false); } @@ -232,8 +224,8 @@ export function attemptTokenLogin( return true; }) .catch((error) => { - const tryAgainCallback: TryAgainFunction = (tryAgain) => { - if (tryAgain) { + const tryAgainCallback: TryAgainFunction = (shouldTryAgain) => { + if (shouldTryAgain) { const cli = createClient({ baseUrl: homeserver, idBaseUrl: identityServer, From 71f163383adef00a9a09632793ecd088a1b11b69 Mon Sep 17 00:00:00 2001 From: Kerry Date: Thu, 29 Jun 2023 08:44:43 +1200 Subject: [PATCH 47/61] Update src/Lifecycle.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/Lifecycle.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index dbf908efae1..71e9cf14338 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -247,7 +247,7 @@ export function attemptTokenLogin( } /** - * After a successful token login or OIDC authorization + * Called after a successful token login or OIDC authorization. * Clear storage then save new credentials in storage * @param credentials as returned from login */ From 11a6ccc3591d9ad4436cb24f87524dbf7ce2a76f Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 29 Jun 2023 14:16:15 +1200 Subject: [PATCH 48/61] tidy --- src/utils/oidc/authorize.ts | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 11006723a05..1ea1ae90b7c 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -142,10 +142,10 @@ const getCodeAndStateFromQueryParams = (queryParams: QueryDict): { code: string; }; /** - * Attempt to complete authorization code flow to login - * @param {QueryDict} queryParams the query-parameters extracted from the real query-string of the starting URI. - * @returns {Promise<{}>} Promise that resolves with accesstoken, identityServerUrl, and homeserverUrl when login was successful - * @throws When login failed + * Attempt to complete authorization code flow to get an access token + * @param queryParams the query-parameters extracted from the real query-string of the starting URI. + * @returns Promise that resolves with accessToken, identityServerUrl, and homeserverUrl when login was successful + * @throws When we failed to get a valid access token */ export const completeOidcLogin = async ( queryParams: QueryDict, @@ -158,15 +158,11 @@ export const completeOidcLogin = async ( const storedAuthorizationParams = retrieveAuthorizationParams(state); - const bearerToken = await completeAuthorizationCodeGrant(code, storedAuthorizationParams); - - // @TODO(kerrya) is there more verification to do here? - - // @TODO(kerrya) do something with the refresh token - + const bearerTokenResponse = await completeAuthorizationCodeGrant(code, storedAuthorizationParams); + // @TODO(kerrya) do something with the refresh token https://github.com/vector-im/element-web/issues/25444 return { homeserverUrl: storedAuthorizationParams.homeserverUrl, identityServerUrl: storedAuthorizationParams.identityServerUrl, - accessToken: bearerToken.access_token, + accessToken: bearerTokenResponse.access_token, }; }; From a7d4e44460df95ad1b04f1c77c171ffdbdfcebe4 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 29 Jun 2023 14:24:29 +1200 Subject: [PATCH 49/61] comment --- src/Lifecycle.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index a68cec087c9..20309da8b0e 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -208,6 +208,11 @@ export async function attemptDelegatedAuthLogin( return attemptTokenLogin(queryParams, defaultDeviceDisplayName, fragmentAfterLogin); } +/** + * Attempt to login by completing OIDC authorization code flow + * @param queryParams string->string map of the query-parameters extracted from the real query-string of the starting URI. + * @returns Promise that resolves to true when login succceeded, else false + */ async function attemptOidcNativeLogin(queryParams: QueryDict): Promise { try { const { accessToken, homeserverUrl, identityServerUrl } = await completeOidcLogin(queryParams); From e389fdecfcea127a44aab86c0a2b763b0a8df341 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 30 Jun 2023 09:56:57 +1200 Subject: [PATCH 50/61] update tests for id token validation --- test/components/structures/MatrixChat-test.tsx | 11 +++++++++++ test/utils/oidc/authorize-test.ts | 6 ++++++ 2 files changed, 17 insertions(+) diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx index 7413a3d05de..73d8c99e439 100644 --- a/test/components/structures/MatrixChat-test.tsx +++ b/test/components/structures/MatrixChat-test.tsx @@ -25,6 +25,7 @@ import { MatrixEvent, Room } from "matrix-js-sdk/src/matrix"; import { generateAuthorizationParams } from "matrix-js-sdk/src/oidc/authorize"; import { logger } from "matrix-js-sdk/src/logger"; import { OidcError } from "matrix-js-sdk/src/oidc/error"; +import * as OidcValidation from "matrix-js-sdk/src/oidc/validate"; import MatrixChat from "../../../src/components/structures/MatrixChat"; import * as StorageManager from "../../../src/utils/StorageManager"; @@ -766,6 +767,7 @@ describe("", () => { token_type: "Bearer", access_token: accessToken, refresh_token: "test_refresh_token", + id_token: "test_id_token", expires_in: 12345, }; @@ -777,6 +779,15 @@ describe("", () => { expect(document.querySelector(".mx_Welcome")!).toBeInTheDocument(); }; + beforeEach(() => { + // annoying to mock jwt decoding used in validateIdToken + jest.spyOn(OidcValidation, "validateIdToken") + .mockClear() + .mockImplementation(() => {}); + + jest.spyOn(logger, "error").mockClear(); + }); + beforeEach(() => { loginClient = getMockClientWithEventEmitter(getMockClientMethods()); // this is used to create a temporary client during login diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts index c03f3a5d1ab..9fb17f9c8b7 100644 --- a/test/utils/oidc/authorize-test.ts +++ b/test/utils/oidc/authorize-test.ts @@ -18,6 +18,7 @@ import fetchMock from "fetch-mock-jest"; import { Method } from "matrix-js-sdk/src/http-api"; import { generateAuthorizationParams } from "matrix-js-sdk/src/oidc/authorize"; import * as randomStringUtils from "matrix-js-sdk/src/randomstring"; +import * as OidcValidation from "matrix-js-sdk/src/oidc/validate"; import { completeOidcLogin, startOidcLogin } from "../../../src/utils/oidc/authorize"; @@ -60,6 +61,11 @@ describe("OIDC authorization", () => { }; jest.spyOn(randomStringUtils, "randomString").mockRestore(); + + // annoying to mock jwt decoding used in validateIdToken + jest.spyOn(OidcValidation, "validateIdToken") + .mockClear() + .mockImplementation(() => {}); }); afterAll(() => { From cb8832c31ef2d6d45b8ebcebba13f1db456dc91f Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 30 Jun 2023 10:06:32 +1200 Subject: [PATCH 51/61] move try again responsibility --- src/Lifecycle.ts | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index f95a62c35cb..2931e6c5ef1 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -224,15 +224,13 @@ export function attemptTokenLogin( return true; }) .catch((error) => { - const tryAgainCallback: TryAgainFunction = (shouldTryAgain) => { - if (shouldTryAgain) { - const cli = createClient({ - baseUrl: homeserver, - idBaseUrl: identityServer, - }); - const idpId = localStorage.getItem(SSO_IDP_ID_KEY) || undefined; - PlatformPeg.get()?.startSingleSignOn(cli, "sso", fragmentAfterLogin, idpId, SSOAction.LOGIN); - } + const tryAgainCallback: TryAgainFunction = () => { + const cli = createClient({ + baseUrl: homeserver, + idBaseUrl: identityServer, + }); + const idpId = localStorage.getItem(SSO_IDP_ID_KEY) || undefined; + PlatformPeg.get()?.startSingleSignOn(cli, "sso", fragmentAfterLogin, idpId, SSOAction.LOGIN); }; onFailedDelegatedAuthLogin( messageForLoginError(error, { @@ -259,7 +257,7 @@ async function onSuccessfulDelegatedAuthLogin(credentials: IMatrixClientCreds): sessionStorage.setItem("mx_fresh_login", String(true)); } -type TryAgainFunction = (shouldTryAgain?: boolean) => void; +type TryAgainFunction = () => void; /** * Display a friendly error to the user when token login or OIDC authorization fails * @param description error description @@ -270,7 +268,8 @@ async function onFailedDelegatedAuthLogin(description: string | ReactNode, tryAg title: _t("We couldn't log you in"), description, button: _t("Try again"), - onFinished: tryAgain, + // if we have a tryAgain callback, call it the primary 'try again' button was clicked in the dialog + onFinished: tryAgain ? (shouldTryAgain?: boolean) => shouldTryAgain && tryAgain() : undefined, }); } From ccadb2996691eb0183cc3fdcf52d2eec8f2338e9 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 3 Jul 2023 18:00:15 +1200 Subject: [PATCH 52/61] prettier --- test/utils/oidc/authorize-test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts index 9fb17f9c8b7..f911f750d3e 100644 --- a/test/utils/oidc/authorize-test.ts +++ b/test/utils/oidc/authorize-test.ts @@ -62,10 +62,10 @@ describe("OIDC authorization", () => { jest.spyOn(randomStringUtils, "randomString").mockRestore(); - // annoying to mock jwt decoding used in validateIdToken - jest.spyOn(OidcValidation, "validateIdToken") - .mockClear() - .mockImplementation(() => {}); + // annoying to mock jwt decoding used in validateIdToken + jest.spyOn(OidcValidation, "validateIdToken") + .mockClear() + .mockImplementation(() => {}); }); afterAll(() => { From 2afdaf4f527632b6a2f32b17fa7719a099042d4f Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 3 Jul 2023 18:35:18 +1200 Subject: [PATCH 53/61] use more future proof config for static clients --- src/IConfigOptions.ts | 7 ++++++- src/Login.ts | 6 +++--- src/utils/oidc/registerClient.ts | 10 +++++++--- test/components/structures/auth/Login-test.tsx | 6 ++++-- test/utils/oidc/registerClient-test.ts | 8 ++++---- 5 files changed, 24 insertions(+), 13 deletions(-) diff --git a/src/IConfigOptions.ts b/src/IConfigOptions.ts index 286942476b1..2df5e7fca1f 100644 --- a/src/IConfigOptions.ts +++ b/src/IConfigOptions.ts @@ -201,7 +201,12 @@ export interface IConfigOptions { * The issuer URL must have a trailing `/`. * OPTIONAL */ - oidc_static_client_ids?: Record; + oidc_static_clients?: Record< + string, + { + client_id: string; + } + >; } export interface ISsoRedirectOptions { diff --git a/src/Login.ts b/src/Login.ts index 27d3690ee9d..2877d9ba342 100644 --- a/src/Login.ts +++ b/src/Login.ts @@ -102,7 +102,7 @@ export default class Login { const oidcFlow = await tryInitOidcNativeFlow( this.delegatedAuthentication, SdkConfig.get().brand, - SdkConfig.get().oidc_static_client_ids, + SdkConfig.get().oidc_static_clients, ); return [oidcFlow]; } catch (error) { @@ -211,9 +211,9 @@ export interface OidcNativeFlow extends ILoginFlow { const tryInitOidcNativeFlow = async ( delegatedAuthConfig: ValidatedDelegatedAuthConfig, brand: string, - oidcStaticClientIds?: IConfigOptions["oidc_static_client_ids"], + oidcStaticClients?: IConfigOptions["oidc_static_clients"], ): Promise => { - const clientId = await getOidcClientId(delegatedAuthConfig, brand, window.location.origin, oidcStaticClientIds); + const clientId = await getOidcClientId(delegatedAuthConfig, brand, window.location.origin, oidcStaticClients); const flow = { type: "oidcNativeFlow", diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 4e2df7832c2..309709e18f1 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -17,6 +17,7 @@ limitations under the License. import { logger } from "matrix-js-sdk/src/logger"; import { registerOidcClient } from "matrix-js-sdk/src/oidc/register"; +import { IConfigOptions } from "../../IConfigOptions"; import { ValidatedDelegatedAuthConfig } from "../ValidatedServerConfig"; /** @@ -25,10 +26,13 @@ import { ValidatedDelegatedAuthConfig } from "../ValidatedServerConfig"; * @param staticOidcClients static client config from config.json * @returns clientId if found, otherwise undefined */ -const getStaticOidcClientId = (issuer: string, staticOidcClients?: Record): string | undefined => { +const getStaticOidcClientId = ( + issuer: string, + staticOidcClients?: IConfigOptions["oidc_static_clients"], +): string | undefined => { // static_oidc_clients are configured with a trailing slash const issuerWithTrailingSlash = issuer.endsWith("/") ? issuer : issuer + "/"; - return staticOidcClients?.[issuerWithTrailingSlash]; + return staticOidcClients?.[issuerWithTrailingSlash]?.client_id; }; /** @@ -46,7 +50,7 @@ export const getOidcClientId = async ( delegatedAuthConfig: ValidatedDelegatedAuthConfig, clientName: string, baseUrl: string, - staticOidcClients?: Record, + staticOidcClients?: IConfigOptions["oidc_static_clients"], ): Promise => { const staticClientId = getStaticOidcClientId(delegatedAuthConfig.issuer, staticOidcClients); if (staticClientId) { diff --git a/test/components/structures/auth/Login-test.tsx b/test/components/structures/auth/Login-test.tsx index dbb5bf0f90e..9595be925f8 100644 --- a/test/components/structures/auth/Login-test.tsx +++ b/test/components/structures/auth/Login-test.tsx @@ -37,7 +37,9 @@ jest.mock("matrix-js-sdk/src/matrix"); jest.useRealTimers(); const oidcStaticClientsConfig = { - "https://staticallyregisteredissuer.org/": "static-clientId-123", + "https://staticallyregisteredissuer.org/": { + client_id: "static-clientId-123", + }, }; describe("Login", function () { @@ -52,7 +54,7 @@ describe("Login", function () { SdkConfig.put({ brand: "test-brand", disable_custom_urls: true, - oidc_static_client_ids: oidcStaticClientsConfig, + oidc_static_clients: oidcStaticClientsConfig, }); mockClient.login.mockClear().mockResolvedValue({}); mockClient.loginFlows.mockClear().mockResolvedValue({ flows: [{ type: "m.login.password" }] }); diff --git a/test/utils/oidc/registerClient-test.ts b/test/utils/oidc/registerClient-test.ts index 1e08c0085e1..539ec332126 100644 --- a/test/utils/oidc/registerClient-test.ts +++ b/test/utils/oidc/registerClient-test.ts @@ -27,7 +27,9 @@ describe("getOidcClientId()", () => { const baseUrl = "https://just.testing"; const dynamicClientId = "xyz789"; const staticOidcClients = { - [issuer]: "abc123", + [issuer]: { + client_id: "abc123", + }, }; const delegatedAuthConfig = { issuer, @@ -42,9 +44,7 @@ describe("getOidcClientId()", () => { }); it("should return static clientId when configured", async () => { - expect(await getOidcClientId(delegatedAuthConfig, clientName, baseUrl, staticOidcClients)).toEqual( - staticOidcClients[issuer], - ); + expect(await getOidcClientId(delegatedAuthConfig, clientName, baseUrl, staticOidcClients)).toEqual("abc123"); // didn't try to register expect(fetchMockJest).toHaveFetchedTimes(0); }); From 3baeac796a2eddc1f665cb9ccc45c014ccac8ea7 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 6 Jul 2023 14:01:00 +1200 Subject: [PATCH 54/61] test util for oidcclientconfigs --- test/test-utils/oidc.ts | 52 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 test/test-utils/oidc.ts diff --git a/test/test-utils/oidc.ts b/test/test-utils/oidc.ts new file mode 100644 index 00000000000..526fb5fd319 --- /dev/null +++ b/test/test-utils/oidc.ts @@ -0,0 +1,52 @@ +/* +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. +*/ + +import { OidcClientConfig } from "matrix-js-sdk/src/autodiscovery"; +import { ValidatedIssuerMetadata } from "matrix-js-sdk/src/oidc/validate"; + +/** + * Makes a valid OidcClientConfig with minimum valid values + * @param issuer used as the base for all other urls + * @returns OidcClientConfig + */ +export const makeDelegatedAuthConfig = (issuer = "https://auth.org/"): OidcClientConfig => { + const metadata = mockOpenIdConfiguration(issuer); + + return { + issuer, + account: issuer + "account", + registrationEndpoint: metadata.registration_endpoint, + authorizationEndpoint: metadata.authorization_endpoint, + tokenEndpoint: metadata.token_endpoint, + metadata, + }; +}; + +/** + * Useful for mocking /.well-known/openid-configuration + * @param issuer used as the base for all other urls + * @returns ValidatedIssuerMetadata + */ +export const mockOpenIdConfiguration = (issuer = "https://auth.org/"): ValidatedIssuerMetadata => ({ + issuer, + revocation_endpoint: issuer + "revoke", + token_endpoint: issuer + "token", + authorization_endpoint: issuer + "auth", + registration_endpoint: issuer + "registration", + response_types_supported: ["query"], + grant_types_supported: ["code", "refreshToken"], + code_challenge_methods_supported: ["S256"], +}); From fcfcfd9c30a2da70898f674d1c561f4f8f474352 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 6 Jul 2023 14:01:17 +1200 Subject: [PATCH 55/61] rename type and lint --- src/utils/AutoDiscoveryUtils.tsx | 28 +++++++------- src/utils/ValidatedServerConfig.ts | 3 +- src/utils/oidc/authorize.ts | 59 ++++++++++++++---------------- src/utils/oidc/registerClient.ts | 1 + test/utils/oidc/authorize-test.ts | 28 +++++--------- 5 files changed, 53 insertions(+), 66 deletions(-) diff --git a/src/utils/AutoDiscoveryUtils.tsx b/src/utils/AutoDiscoveryUtils.tsx index 213bc5ea986..2c8a4daf29a 100644 --- a/src/utils/AutoDiscoveryUtils.tsx +++ b/src/utils/AutoDiscoveryUtils.tsx @@ -15,14 +15,14 @@ limitations under the License. */ import React, { ReactNode } from "react"; -import { AutoDiscovery, ClientConfig } from "matrix-js-sdk/src/autodiscovery"; +import { AutoDiscovery, ClientConfig, OidcClientConfig } from "matrix-js-sdk/src/autodiscovery"; import { M_AUTHENTICATION } from "matrix-js-sdk/src/client"; import { logger } from "matrix-js-sdk/src/logger"; import { IClientWellKnown } from "matrix-js-sdk/src/matrix"; import { _t, UserFriendlyError } from "../languageHandler"; import SdkConfig from "../SdkConfig"; -import { ValidatedDelegatedAuthConfig, ValidatedServerConfig } from "./ValidatedServerConfig"; +import { ValidatedServerConfig } from "./ValidatedServerConfig"; const LIVELINESS_DISCOVERY_ERRORS: string[] = [ AutoDiscovery.ERROR_INVALID_HOMESERVER, @@ -261,25 +261,25 @@ export default class AutoDiscoveryUtils { throw new UserFriendlyError("Unexpected error resolving homeserver configuration"); } - let delegatedAuthentication: - | { - authorizationEndpoint: string; - registrationEndpoint?: string; - tokenEndpoint: string; - account?: string; - issuer: string; - } - | undefined; + let delegatedAuthentication: OidcClientConfig | undefined; if (discoveryResult[M_AUTHENTICATION.stable!]?.state === AutoDiscovery.SUCCESS) { - const { authorizationEndpoint, registrationEndpoint, tokenEndpoint, account, issuer } = discoveryResult[ - M_AUTHENTICATION.stable! - ] as ValidatedDelegatedAuthConfig; + const { + authorizationEndpoint, + registrationEndpoint, + tokenEndpoint, + account, + issuer, + metadata, + signingKeys, + } = discoveryResult[M_AUTHENTICATION.stable!] as OidcClientConfig; delegatedAuthentication = Object.freeze({ authorizationEndpoint, registrationEndpoint, tokenEndpoint, account, issuer, + metadata, + signingKeys, }); } diff --git a/src/utils/ValidatedServerConfig.ts b/src/utils/ValidatedServerConfig.ts index cb3edf3a9c8..b41ac6a642c 100644 --- a/src/utils/ValidatedServerConfig.ts +++ b/src/utils/ValidatedServerConfig.ts @@ -14,6 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ +import { OidcClientConfig } from "matrix-js-sdk/src/autodiscovery"; import { IDelegatedAuthConfig } from "matrix-js-sdk/src/client"; import { ValidatedIssuerConfig } from "matrix-js-sdk/src/oidc/validate"; @@ -38,5 +39,5 @@ export interface ValidatedServerConfig { * From homeserver .well-known m.authentication, and issuer's .well-known/openid-configuration * Used for OIDC native flow authentication */ - delegatedAuthentication?: ValidatedDelegatedAuthConfig; + delegatedAuthentication?: OidcClientConfig; } diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 22e7a11bce1..3ae7de612d2 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -14,33 +14,24 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { - AuthorizationParams, - generateAuthorizationParams, - generateAuthorizationUrl, -} from "matrix-js-sdk/src/oidc/authorize"; +import { OidcClientConfig } from "matrix-js-sdk/src/autodiscovery"; +import { generateOidcAuthorizationUrl } from "matrix-js-sdk/src/oidc/authorize"; +import { randomString } from "matrix-js-sdk/src/randomstring"; -import { ValidatedDelegatedAuthConfig } from "../ValidatedServerConfig"; +import { SSO_HOMESERVER_URL_KEY, SSO_ID_SERVER_URL_KEY } from "../../BasePlatform"; /** - * Store authorization params for retrieval when returning from OIDC OP - * @param authorizationParams from `generateAuthorizationParams` - * @param delegatedAuthConfig used for future interactions with OP - * @param clientId this client's id as registered with configured issuer + * Store homeserver for retrieval when returning from OIDC OP * @param homeserver target homeserver + * @param identityServerUrl OPTIONAL target identity server */ -const storeAuthorizationParams = ( - { redirectUri, state, nonce, codeVerifier }: AuthorizationParams, - { issuer }: ValidatedDelegatedAuthConfig, - clientId: string, - homeserver: string, -): void => { - window.sessionStorage.setItem(`oidc_${state}_nonce`, nonce); - window.sessionStorage.setItem(`oidc_${state}_redirectUri`, redirectUri); - window.sessionStorage.setItem(`oidc_${state}_codeVerifier`, codeVerifier); - window.sessionStorage.setItem(`oidc_${state}_clientId`, clientId); - window.sessionStorage.setItem(`oidc_${state}_issuer`, issuer); - window.sessionStorage.setItem(`oidc_${state}_homeserver`, homeserver); +const persistAuthorizationParams = (nonce, homeserverUrl: string, identityServerUrl?: string): void => { + // persist hs url and is url for when the user is returned to the app + sessionStorage.setItem(`oidc_nonce`, nonce); + sessionStorage.setItem(SSO_HOMESERVER_URL_KEY, homeserverUrl); + if (identityServerUrl) { + sessionStorage.setItem(SSO_ID_SERVER_URL_KEY, identityServerUrl); + } }; /** @@ -49,25 +40,29 @@ const storeAuthorizationParams = ( * Navigates to configured authorization endpoint * @param delegatedAuthConfig from discovery * @param clientId this client's id as registered with configured issuer - * @param homeserver target homeserver + * @param homeserverUrl target homeserver + * @param identityServerUrl OPTIONAL target identity server * @returns Promise that resolves after we have navigated to auth endpoint */ export const startOidcLogin = async ( - delegatedAuthConfig: ValidatedDelegatedAuthConfig, + delegatedAuthConfig: OidcClientConfig, clientId: string, - homeserver: string, + homeserverUrl: string, + identityServerUrl?: string, ): Promise => { - // TODO(kerrya) afterloginfragment https://github.com/vector-im/element-web/issues/25656 const redirectUri = window.location.origin; - const authParams = generateAuthorizationParams({ redirectUri }); - storeAuthorizationParams(authParams, delegatedAuthConfig, clientId, homeserver); + const nonce = randomString(10); + + persistAuthorizationParams(nonce, homeserverUrl, identityServerUrl); - const authorizationUrl = await generateAuthorizationUrl( - delegatedAuthConfig.authorizationEndpoint, + const authorizationUrl = await generateOidcAuthorizationUrl({ + metadata: delegatedAuthConfig.metadata, + redirectUri, clientId, - authParams, - ); + homeserverUrl, + nonce, + }); window.location.href = authorizationUrl; }; diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 309709e18f1..355cc1c6303 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -52,6 +52,7 @@ export const getOidcClientId = async ( baseUrl: string, staticOidcClients?: IConfigOptions["oidc_static_clients"], ): Promise => { + console.log("hhh dele", delegatedAuthConfig); const staticClientId = getStaticOidcClientId(delegatedAuthConfig.issuer, staticOidcClients); if (staticClientId) { logger.debug(`Using static clientId for issuer ${delegatedAuthConfig.issuer}`); diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts index 5abdb19862a..d799da776f6 100644 --- a/test/utils/oidc/authorize-test.ts +++ b/test/utils/oidc/authorize-test.ts @@ -18,20 +18,15 @@ import fetchMockJest from "fetch-mock-jest"; import * as randomStringUtils from "matrix-js-sdk/src/randomstring"; import { startOidcLogin } from "../../../src/utils/oidc/authorize"; +import { makeDelegatedAuthConfig, mockOpenIdConfiguration } from "../../test-utils/oidc"; describe("startOidcLogin()", () => { const issuer = "https://auth.com/"; - const authorizationEndpoint = "https://auth.com/authorization"; const homeserver = "https://matrix.org"; const clientId = "xyz789"; const baseUrl = "https://test.com"; - const delegatedAuthConfig = { - issuer, - registrationEndpoint: issuer + "registration", - authorizationEndpoint, - tokenEndpoint: issuer + "token", - }; + const delegatedAuthConfig = makeDelegatedAuthConfig(issuer); const sessionStorageGetSpy = jest.spyOn(sessionStorage.__proto__, "setItem").mockReturnValue(undefined); const randomStringMockImpl = (length: number) => new Array(length).fill("x").join(""); @@ -53,6 +48,10 @@ describe("startOidcLogin()", () => { origin: baseUrl, }; + fetchMockJest.get( + delegatedAuthConfig.metadata.issuer + ".well-known/openid-configuration", + mockOpenIdConfiguration(), + ); jest.spyOn(randomStringUtils, "randomString").mockRestore(); }); @@ -60,21 +59,12 @@ describe("startOidcLogin()", () => { window.location = realWindowLocation; }); - it("should store authorization params in session storage", async () => { + it("should store auth params in session storage", async () => { jest.spyOn(randomStringUtils, "randomString").mockReset().mockImplementation(randomStringMockImpl); await startOidcLogin(delegatedAuthConfig, clientId, homeserver); - const state = randomStringUtils.randomString(8); - - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_nonce`, randomStringUtils.randomString(8)); - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_redirectUri`, baseUrl); - expect(sessionStorageGetSpy).toHaveBeenCalledWith( - `oidc_${state}_codeVerifier`, - randomStringUtils.randomString(64), - ); - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_clientId`, clientId); - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_issuer`, issuer); - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${state}_homeserver`, homeserver); + expect(sessionStorageGetSpy).toHaveBeenCalledWith(`mx_sso_hs_url`, homeserver); + expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_nonce`, expect.anything()); }); it("navigates to authorization endpoint with correct parameters", async () => { From 8824cb8f29a4e779628b6c6e04be89e065a72ee6 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 6 Jul 2023 16:07:29 +1200 Subject: [PATCH 56/61] correct oidc test util --- test/test-utils/oidc.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/test-utils/oidc.ts b/test/test-utils/oidc.ts index 526fb5fd319..569a09cff27 100644 --- a/test/test-utils/oidc.ts +++ b/test/test-utils/oidc.ts @@ -22,7 +22,7 @@ import { ValidatedIssuerMetadata } from "matrix-js-sdk/src/oidc/validate"; * @param issuer used as the base for all other urls * @returns OidcClientConfig */ -export const makeDelegatedAuthConfig = (issuer = "https://auth.org/"): OidcClientConfig => { + export const makeDelegatedAuthConfig = (issuer = "https://auth.org/"): OidcClientConfig => { const metadata = mockOpenIdConfiguration(issuer); return { @@ -46,7 +46,8 @@ export const mockOpenIdConfiguration = (issuer = "https://auth.org/"): Validated token_endpoint: issuer + "token", authorization_endpoint: issuer + "auth", registration_endpoint: issuer + "registration", - response_types_supported: ["query"], - grant_types_supported: ["code", "refreshToken"], + jwks_uri: issuer + "jwks", + response_types_supported: ["code"], + grant_types_supported: ["authorization_code", "refresh_token"], code_challenge_methods_supported: ["S256"], }); From 59c5d5e6f270fc22cae1d54c96e1d118747dc840 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 6 Jul 2023 17:08:03 +1200 Subject: [PATCH 57/61] store issuer and clientId pre auth navigation --- src/utils/oidc/authorize.ts | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 3ae7de612d2..614fb623ad6 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -15,22 +15,35 @@ limitations under the License. */ import { OidcClientConfig } from "matrix-js-sdk/src/autodiscovery"; +import { IDelegatedAuthConfig } from "matrix-js-sdk/src/client"; import { generateOidcAuthorizationUrl } from "matrix-js-sdk/src/oidc/authorize"; import { randomString } from "matrix-js-sdk/src/randomstring"; import { SSO_HOMESERVER_URL_KEY, SSO_ID_SERVER_URL_KEY } from "../../BasePlatform"; +type StoredAuthorizationParams = IDelegatedAuthConfig & { + nonce: string; + homeserverUrl: string; + identityServerUrl?: string; + clientId: string; +} + /** * Store homeserver for retrieval when returning from OIDC OP * @param homeserver target homeserver * @param identityServerUrl OPTIONAL target identity server */ -const persistAuthorizationParams = (nonce, homeserverUrl: string, identityServerUrl?: string): void => { +const persistAuthorizationParams = (state, { + nonce, homeserverUrl, identityServerUrl, issuer, account, clientId +}: StoredAuthorizationParams): void => { // persist hs url and is url for when the user is returned to the app - sessionStorage.setItem(`oidc_nonce`, nonce); - sessionStorage.setItem(SSO_HOMESERVER_URL_KEY, homeserverUrl); + sessionStorage.setItem(`mx_oidc_${state}_nonce`, nonce); + sessionStorage.setItem(`mx_oidc_${state}_issuer`, issuer); + sessionStorage.setItem(`mx_oidc_${state}_account`, account); + sessionStorage.setItem(`mx_oidc_${state}_clientId`, clientId); + sessionStorage.setItem(`mx_oidc_${state}_homeserverUrl`, homeserverUrl); if (identityServerUrl) { - sessionStorage.setItem(SSO_ID_SERVER_URL_KEY, identityServerUrl); + sessionStorage.setItem(`mx_oidc_${state}_identityServerUrl`, identityServerUrl); } }; @@ -53,8 +66,15 @@ export const startOidcLogin = async ( const redirectUri = window.location.origin; const nonce = randomString(10); - - persistAuthorizationParams(nonce, homeserverUrl, identityServerUrl); + const state = randomString(10); + persistAuthorizationParams(state, { + nonce, + issuer: delegatedAuthConfig.issuer, + account: delegatedAuthConfig.account, + homeserverUrl, + identityServerUrl, + clientId + }); const authorizationUrl = await generateOidcAuthorizationUrl({ metadata: delegatedAuthConfig.metadata, @@ -62,6 +82,7 @@ export const startOidcLogin = async ( clientId, homeserverUrl, nonce, + state, }); window.location.href = authorizationUrl; From b58509e82eca70caa8787026becb68edb9055dfe Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 7 Jul 2023 15:02:43 +1200 Subject: [PATCH 58/61] adjust for js-sdk changes --- src/utils/oidc/authorize.ts | 98 ++++---------------------------- src/utils/oidc/registerClient.ts | 1 - 2 files changed, 10 insertions(+), 89 deletions(-) diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 6e506c77e3b..7c803ce91ef 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -15,83 +15,13 @@ limitations under the License. */ import { - AuthorizationParams, - generateAuthorizationParams, - generateAuthorizationUrl, completeAuthorizationCodeGrant, } from "matrix-js-sdk/src/oidc/authorize"; import { QueryDict } from "matrix-js-sdk/src/utils"; import { OidcClientConfig } from "matrix-js-sdk/src/autodiscovery"; -import { IDelegatedAuthConfig } from "matrix-js-sdk/src/client"; import { generateOidcAuthorizationUrl } from "matrix-js-sdk/src/oidc/authorize"; import { randomString } from "matrix-js-sdk/src/randomstring"; - -type StoredAuthorizationParams = IDelegatedAuthConfig & { - nonce: string; - homeserverUrl: string; - identityServerUrl?: string; - clientId: string; -} - -/** - * Validate that stored params are present and valid - * @param params as retrieved from session storage - * @returns validated stored authorization params - * @throws when params are invalid or missing - */ -const validateStoredAuthorizationParams = (params: Partial): StoredAuthorizationParams => { - const requiredStringProperties = ["nonce", "clientId", "homeserverUrl", "issuer"]; - if ( - requiredStringProperties.every((key: string) => params[key] && typeof params[key] === "string") && - (params.identityServerUrl === undefined || typeof params.identityServerUrl === "string") - ) { - return params as StoredAuthorizationParams; - } - throw new Error("Cannot complete OIDC login: required properties not found in session storage"); -}; - -const retrieveAuthorizationParams = (state: string): StoredAuthorizationParams => { - const nonce = window.sessionStorage.getItem(`mx_oidc_${state}_nonce`); - const issuer = window.sessionStorage.getItem(`mx_oidc_${state}_issuer`); - const account = window.sessionStorage.getItem(`mx_oidc_${state}_account`) ?? undefined; - const clientId = window.sessionStorage.getItem(`mx_oidc_${state}_clientId`); - const homeserverUrl = window.sessionStorage.getItem(`mx_oidc_${state}_homeserverUrl`); - const identityServerUrl = window.sessionStorage.getItem(`mx_oidc_${state}_identityServerUrl`) ?? undefined; - - console.log('hhh', state, { - nonce, - clientId, - homeserverUrl, - identityServerUrl, - issuer, - account, - }) - - return validateStoredAuthorizationParams({ - nonce, - clientId, - homeserverUrl, - identityServerUrl, - issuer, - account, - }); -} - -const persistAuthorizationParams = (state, { - nonce, homeserverUrl, identityServerUrl, issuer, account, clientId -}: StoredAuthorizationParams): void => { - // persist hs url and is url for when the user is returned to the app - sessionStorage.setItem(`mx_oidc_${state}_nonce`, nonce); - sessionStorage.setItem(`mx_oidc_${state}_issuer`, issuer); - sessionStorage.setItem(`mx_oidc_${state}_account`, account); - sessionStorage.setItem(`mx_oidc_${state}_clientId`, clientId); - sessionStorage.setItem(`mx_oidc_${state}_homeserverUrl`, homeserverUrl); - if (identityServerUrl) { - sessionStorage.setItem(`mx_oidc_${state}_identityServerUrl`, identityServerUrl); - } -}; - /** * Start OIDC authorization code flow * Generates auth params, stores them in session storage and @@ -111,25 +41,14 @@ export const startOidcLogin = async ( const redirectUri = window.location.origin; const nonce = randomString(10); - const state = randomString(8); - const authorizationUrl = await generateOidcAuthorizationUrl({ metadata: delegatedAuthConfig.metadata, redirectUri, clientId, homeserverUrl, - nonce, - state, - }); - - persistAuthorizationParams(state, { - nonce, - issuer: delegatedAuthConfig.issuer, - account: delegatedAuthConfig.account, - homeserverUrl, identityServerUrl, - clientId + nonce, }); window.location.href = authorizationUrl; @@ -166,14 +85,17 @@ export const completeOidcLogin = async ( accessToken: string; }> => { const { code, state } = getCodeAndStateFromQueryParams(queryParams); + const { + homeserverUrl, + tokenResponse, + identityServerUrl, + oidcClientSettings, + } = await completeAuthorizationCodeGrant(code, state); - // const storedAuthorizationParams = retrieveAuthorizationParams(state); - - const bearerTokenResponse = await completeAuthorizationCodeGrant(code); // @TODO(kerrya) do something with the refresh token https://github.com/vector-im/element-web/issues/25444 return { - homeserverUrl: storedAuthorizationParams.homeserverUrl, - identityServerUrl: storedAuthorizationParams.identityServerUrl, - accessToken: bearerTokenResponse.access_token, + homeserverUrl: homeserverUrl, + identityServerUrl: identityServerUrl, + accessToken: tokenResponse.access_token, }; }; diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 355cc1c6303..309709e18f1 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -52,7 +52,6 @@ export const getOidcClientId = async ( baseUrl: string, staticOidcClients?: IConfigOptions["oidc_static_clients"], ): Promise => { - console.log("hhh dele", delegatedAuthConfig); const staticClientId = getStaticOidcClientId(delegatedAuthConfig.issuer, staticOidcClients); if (staticClientId) { logger.debug(`Using static clientId for issuer ${delegatedAuthConfig.issuer}`); From d2f7f55ce61acdf4b3850c7c2f27abc4188e3a9d Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 7 Jul 2023 15:42:58 +1200 Subject: [PATCH 59/61] update for js-sdk userstate, tidy --- src/utils/oidc/authorize.ts | 40 +------------------------------ src/utils/oidc/registerClient.ts | 1 - test/test-utils/oidc.ts | 2 +- test/utils/oidc/authorize-test.ts | 9 ------- 4 files changed, 2 insertions(+), 50 deletions(-) diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 614fb623ad6..823e3cfab4a 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -15,38 +15,9 @@ limitations under the License. */ import { OidcClientConfig } from "matrix-js-sdk/src/autodiscovery"; -import { IDelegatedAuthConfig } from "matrix-js-sdk/src/client"; import { generateOidcAuthorizationUrl } from "matrix-js-sdk/src/oidc/authorize"; import { randomString } from "matrix-js-sdk/src/randomstring"; -import { SSO_HOMESERVER_URL_KEY, SSO_ID_SERVER_URL_KEY } from "../../BasePlatform"; - -type StoredAuthorizationParams = IDelegatedAuthConfig & { - nonce: string; - homeserverUrl: string; - identityServerUrl?: string; - clientId: string; -} - -/** - * Store homeserver for retrieval when returning from OIDC OP - * @param homeserver target homeserver - * @param identityServerUrl OPTIONAL target identity server - */ -const persistAuthorizationParams = (state, { - nonce, homeserverUrl, identityServerUrl, issuer, account, clientId -}: StoredAuthorizationParams): void => { - // persist hs url and is url for when the user is returned to the app - sessionStorage.setItem(`mx_oidc_${state}_nonce`, nonce); - sessionStorage.setItem(`mx_oidc_${state}_issuer`, issuer); - sessionStorage.setItem(`mx_oidc_${state}_account`, account); - sessionStorage.setItem(`mx_oidc_${state}_clientId`, clientId); - sessionStorage.setItem(`mx_oidc_${state}_homeserverUrl`, homeserverUrl); - if (identityServerUrl) { - sessionStorage.setItem(`mx_oidc_${state}_identityServerUrl`, identityServerUrl); - } -}; - /** * Start OIDC authorization code flow * Generates auth params, stores them in session storage and @@ -66,23 +37,14 @@ export const startOidcLogin = async ( const redirectUri = window.location.origin; const nonce = randomString(10); - const state = randomString(10); - persistAuthorizationParams(state, { - nonce, - issuer: delegatedAuthConfig.issuer, - account: delegatedAuthConfig.account, - homeserverUrl, - identityServerUrl, - clientId - }); const authorizationUrl = await generateOidcAuthorizationUrl({ metadata: delegatedAuthConfig.metadata, redirectUri, clientId, homeserverUrl, + identityServerUrl, nonce, - state, }); window.location.href = authorizationUrl; diff --git a/src/utils/oidc/registerClient.ts b/src/utils/oidc/registerClient.ts index 355cc1c6303..309709e18f1 100644 --- a/src/utils/oidc/registerClient.ts +++ b/src/utils/oidc/registerClient.ts @@ -52,7 +52,6 @@ export const getOidcClientId = async ( baseUrl: string, staticOidcClients?: IConfigOptions["oidc_static_clients"], ): Promise => { - console.log("hhh dele", delegatedAuthConfig); const staticClientId = getStaticOidcClientId(delegatedAuthConfig.issuer, staticOidcClients); if (staticClientId) { logger.debug(`Using static clientId for issuer ${delegatedAuthConfig.issuer}`); diff --git a/test/test-utils/oidc.ts b/test/test-utils/oidc.ts index 569a09cff27..aa042eba5d4 100644 --- a/test/test-utils/oidc.ts +++ b/test/test-utils/oidc.ts @@ -22,7 +22,7 @@ import { ValidatedIssuerMetadata } from "matrix-js-sdk/src/oidc/validate"; * @param issuer used as the base for all other urls * @returns OidcClientConfig */ - export const makeDelegatedAuthConfig = (issuer = "https://auth.org/"): OidcClientConfig => { +export const makeDelegatedAuthConfig = (issuer = "https://auth.org/"): OidcClientConfig => { const metadata = mockOpenIdConfiguration(issuer); return { diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts index d799da776f6..3dda4da2e9f 100644 --- a/test/utils/oidc/authorize-test.ts +++ b/test/utils/oidc/authorize-test.ts @@ -29,7 +29,6 @@ describe("startOidcLogin()", () => { const delegatedAuthConfig = makeDelegatedAuthConfig(issuer); const sessionStorageGetSpy = jest.spyOn(sessionStorage.__proto__, "setItem").mockReturnValue(undefined); - const randomStringMockImpl = (length: number) => new Array(length).fill("x").join(""); // to restore later const realWindowLocation = window.location; @@ -59,14 +58,6 @@ describe("startOidcLogin()", () => { window.location = realWindowLocation; }); - it("should store auth params in session storage", async () => { - jest.spyOn(randomStringUtils, "randomString").mockReset().mockImplementation(randomStringMockImpl); - await startOidcLogin(delegatedAuthConfig, clientId, homeserver); - - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`mx_sso_hs_url`, homeserver); - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_nonce`, expect.anything()); - }); - it("navigates to authorization endpoint with correct parameters", async () => { await startOidcLogin(delegatedAuthConfig, clientId, homeserver); From 34013323f8c773cc59a9c102075dc6721123dff4 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 7 Jul 2023 16:20:03 +1200 Subject: [PATCH 60/61] update MatrixChat tests --- .../components/structures/MatrixChat-test.tsx | 106 +++++++----------- 1 file changed, 42 insertions(+), 64 deletions(-) diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx index 1e2b52a5b29..f1b586b2c71 100644 --- a/test/components/structures/MatrixChat-test.tsx +++ b/test/components/structures/MatrixChat-test.tsx @@ -17,15 +17,16 @@ limitations under the License. import React, { ComponentProps } from "react"; import { fireEvent, render, RenderResult, screen, within } from "@testing-library/react"; import fetchMock from "fetch-mock-jest"; +import { mocked } from "jest-mock"; import { ClientEvent, MatrixClient } from "matrix-js-sdk/src/client"; import { SyncState } from "matrix-js-sdk/src/sync"; import { MediaHandler } from "matrix-js-sdk/src/webrtc/mediaHandler"; import * as MatrixJs from "matrix-js-sdk/src/matrix"; import { MatrixEvent, Room } from "matrix-js-sdk/src/matrix"; -import { generateAuthorizationParams } from "matrix-js-sdk/src/oidc/authorize"; +import { completeAuthorizationCodeGrant } from "matrix-js-sdk/src/oidc/authorize"; import { logger } from "matrix-js-sdk/src/logger"; import { OidcError } from "matrix-js-sdk/src/oidc/error"; -import * as OidcValidation from "matrix-js-sdk/src/oidc/validate"; +import { BearerTokenResponse } from "matrix-js-sdk/src/oidc/validate"; import MatrixChat from "../../../src/components/structures/MatrixChat"; import * as StorageManager from "../../../src/utils/StorageManager"; @@ -41,6 +42,10 @@ import { } from "../../test-utils"; import * as leaveRoomUtils from "../../../src/utils/leave-behaviour"; +jest.mock("matrix-js-sdk/src/oidc/authorize", () => ({ + completeAuthorizationCodeGrant: jest.fn(), +})); + describe("", () => { const userId = "@alice:server.org"; const deviceId = "qwertyui"; @@ -725,32 +730,16 @@ describe("", () => { describe("when query params have a OIDC params", () => { const issuer = "https://auth.com/"; - const authorizationEndpoint = "https://auth.com/authorization"; - const homeserver = "https://matrix.org"; + const homeserverUrl = "https://matrix.org"; const identityServerUrl = "https://is.org"; const clientId = "xyz789"; const baseUrl = "https://test.com"; - const delegatedAuthConfig = { - issuer, - registrationEndpoint: issuer + "registration", - authorizationEndpoint, - tokenEndpoint: issuer + "token", - }; - const authorizationParams = generateAuthorizationParams({ redirectUri: baseUrl }); - const storedAuthorizationParams = { - nonce: authorizationParams.nonce, - redirectUri: baseUrl, - codeVerifier: authorizationParams.codeVerifier, - clientId, - homeserverUrl: homeserver, - identityServerUrl, - delegatedAuthConfig: JSON.stringify(delegatedAuthConfig), - }; const code = "test-oidc-auth-code"; + const state = "test-oidc-state"; const realQueryParams = { code, - state: authorizationParams.state, + state: state, }; const userId = "@alice:server.org"; @@ -759,22 +748,22 @@ describe("", () => { const mockLocalStorage: Record = { // these are only going to be set during login - mx_hs_url: homeserver, + mx_hs_url: homeserverUrl, mx_is_url: identityServerUrl, mx_user_id: userId, mx_device_id: deviceId, }; - let loginClient!: ReturnType; - - const validBearerTokenResponse = { - token_type: "Bearer", + const tokenResponse: BearerTokenResponse = { access_token: accessToken, - refresh_token: "test_refresh_token", - id_token: "test_id_token", - expires_in: 12345, + refresh_token: "def456", + scope: "test", + token_type: "Bearer", + expires_at: 12345, }; + let loginClient!: ReturnType; + // for now when OIDC fails for any reason we just bump back to welcome // error handling screens in https://github.com/vector-im/element-web/issues/25665 const expectOIDCError = async (): Promise => { @@ -784,10 +773,15 @@ describe("", () => { }; beforeEach(() => { - // annoying to mock jwt decoding used in validateIdToken - jest.spyOn(OidcValidation, "validateIdToken") - .mockClear() - .mockImplementation(() => {}); + mocked(completeAuthorizationCodeGrant).mockClear().mockResolvedValue({ + oidcClientSettings: { + clientId, + issuer, + }, + tokenResponse, + homeserverUrl, + identityServerUrl, + }); jest.spyOn(logger, "error").mockClear(); }); @@ -801,21 +795,6 @@ describe("", () => { jest.spyOn(logger, "log").mockClear(); localStorageGetSpy.mockImplementation((key: unknown) => mockLocalStorage[key as string] || ""); - sessionStorageGetSpy.mockImplementation((key: string) => { - const itemKey = key.split(`oidc_${authorizationParams.state}_`).pop(); - return storedAuthorizationParams[itemKey] || null; - }); - - fetchMock.resetHistory(); - fetchMock.post( - delegatedAuthConfig.tokenEndpoint, - { - status: 200, - body: validBearerTokenResponse, - }, - { overwriteRoutes: true }, - ); - loginClient.whoami.mockResolvedValue({ user_id: userId, device_id: deviceId, @@ -823,24 +802,29 @@ describe("", () => { }); }); - it("should fail when authorization params are not found in session storage", async () => { - sessionStorageGetSpy.mockReturnValue(undefined); - getComponent({ realQueryParams }); + it("should fail when query params do not include valid code and state", async () => { + const queryParams = { + code: 123, + state: "abc", + }; + getComponent({ realQueryParams: queryParams }); await flushPromises(); expect(logger.error).toHaveBeenCalledWith( "Failed to login via OIDC", - new Error("Cannot complete OIDC login: required properties not found in session storage"), + new Error("Invalid query parameters for OIDC native login. `code` and `state` are required."), ); await expectOIDCError(); }); - it("should attempt to get access token", async () => { + it("should make correct request to complete authorization", async () => { getComponent({ realQueryParams }); - expect(fetchMock).toHaveFetched(delegatedAuthConfig.tokenEndpoint); + await flushPromises(); + + expect(completeAuthorizationCodeGrant).toHaveBeenCalledWith(code, state); }); it("should look up userId using access token", async () => { @@ -850,7 +834,7 @@ describe("", () => { // check we used a client with the correct accesstoken expect(MatrixJs.createClient).toHaveBeenCalledWith({ - baseUrl: homeserver, + baseUrl: homeserverUrl, accessToken, idBaseUrl: identityServerUrl, }); @@ -881,13 +865,7 @@ describe("", () => { describe("when login fails", () => { beforeEach(() => { - fetchMock.post( - delegatedAuthConfig.tokenEndpoint, - { - status: 500, - }, - { overwriteRoutes: true }, - ); + mocked(completeAuthorizationCodeGrant).mockRejectedValue(new Error(OidcError.CodeExchangeFailed)); }); it("should log and return to welcome page", async () => { @@ -929,7 +907,7 @@ describe("", () => { await flushPromises(); - expect(localStorageSetSpy).toHaveBeenCalledWith("mx_hs_url", storedAuthorizationParams.homeserverUrl); + expect(localStorageSetSpy).toHaveBeenCalledWith("mx_hs_url", homeserverUrl); expect(localStorageSetSpy).toHaveBeenCalledWith("mx_user_id", userId); expect(localStorageSetSpy).toHaveBeenCalledWith("mx_has_access_token", "true"); expect(localStorageSetSpy).toHaveBeenCalledWith("mx_device_id", deviceId); @@ -949,7 +927,7 @@ describe("", () => { " guest: " + false + " hs: " + - storedAuthorizationParams.homeserverUrl + + homeserverUrl + " softLogout: " + false, " freshLogin: " + false, From 8a7b3f5c2cae64766cd97e6de124773716839830 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 7 Jul 2023 16:40:35 +1200 Subject: [PATCH 61/61] update tests --- src/utils/oidc/authorize.ts | 16 +-- .../components/structures/MatrixChat-test.tsx | 2 - test/utils/oidc/authorize-test.ts | 117 +++++------------- 3 files changed, 37 insertions(+), 98 deletions(-) diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts index 24a6b72eb9b..705278c63dc 100644 --- a/src/utils/oidc/authorize.ts +++ b/src/utils/oidc/authorize.ts @@ -14,9 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { - completeAuthorizationCodeGrant, -} from "matrix-js-sdk/src/oidc/authorize"; +import { completeAuthorizationCodeGrant } from "matrix-js-sdk/src/oidc/authorize"; import { QueryDict } from "matrix-js-sdk/src/utils"; import { OidcClientConfig } from "matrix-js-sdk/src/autodiscovery"; import { generateOidcAuthorizationUrl } from "matrix-js-sdk/src/oidc/authorize"; @@ -41,11 +39,7 @@ export const startOidcLogin = async ( const redirectUri = window.location.origin; const nonce = randomString(10); -<<<<<<< HEAD - -======= ->>>>>>> kerry/oidc-client-ts-start-auth const authorizationUrl = await generateOidcAuthorizationUrl({ metadata: delegatedAuthConfig.metadata, redirectUri, @@ -89,14 +83,10 @@ export const completeOidcLogin = async ( accessToken: string; }> => { const { code, state } = getCodeAndStateFromQueryParams(queryParams); - const { - homeserverUrl, - tokenResponse, - identityServerUrl, - oidcClientSettings, - } = await completeAuthorizationCodeGrant(code, state); + const { homeserverUrl, tokenResponse, identityServerUrl } = await completeAuthorizationCodeGrant(code, state); // @TODO(kerrya) do something with the refresh token https://github.com/vector-im/element-web/issues/25444 + return { homeserverUrl: homeserverUrl, identityServerUrl: identityServerUrl, diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx index f1b586b2c71..9b1ea991c7f 100644 --- a/test/components/structures/MatrixChat-test.tsx +++ b/test/components/structures/MatrixChat-test.tsx @@ -131,7 +131,6 @@ describe("", () => { const localStorageGetSpy = jest.spyOn(localStorage.__proto__, "getItem").mockReturnValue(undefined); const localStorageClearSpy = jest.spyOn(localStorage.__proto__, "clear"); const sessionStorageSetSpy = jest.spyOn(sessionStorage.__proto__, "setItem"); - const sessionStorageGetSpy = jest.spyOn(sessionStorage.__proto__, "getItem"); // make test results readable filterConsole("Failed to parse localStorage object"); @@ -733,7 +732,6 @@ describe("", () => { const homeserverUrl = "https://matrix.org"; const identityServerUrl = "https://is.org"; const clientId = "xyz789"; - const baseUrl = "https://test.com"; const code = "test-oidc-auth-code"; const state = "test-oidc-state"; diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts index bf6a1ce1935..f0543381276 100644 --- a/test/utils/oidc/authorize-test.ts +++ b/test/utils/oidc/authorize-test.ts @@ -15,34 +15,32 @@ limitations under the License. */ import fetchMock from "fetch-mock-jest"; -import { Method } from "matrix-js-sdk/src/http-api"; -import { generateAuthorizationParams } from "matrix-js-sdk/src/oidc/authorize"; +import { completeAuthorizationCodeGrant } from "matrix-js-sdk/src/oidc/authorize"; import * as randomStringUtils from "matrix-js-sdk/src/randomstring"; -import * as OidcValidation from "matrix-js-sdk/src/oidc/validate"; +import { BearerTokenResponse } from "matrix-js-sdk/src/oidc/validate"; +import { mocked } from "jest-mock"; import { completeOidcLogin, startOidcLogin } from "../../../src/utils/oidc/authorize"; -import { makeDelegatedAuthConfig, mockOpenIdConfiguration } from "../../test-utils/oidc"; +import { makeDelegatedAuthConfig } from "../../test-utils/oidc"; + +jest.mock("matrix-js-sdk/src/oidc/authorize", () => ({ + ...jest.requireActual("matrix-js-sdk/src/oidc/authorize"), + completeAuthorizationCodeGrant: jest.fn(), +})); describe("OIDC authorization", () => { const issuer = "https://auth.com/"; - const homeserver = "https://matrix.org"; + const homeserverUrl = "https://matrix.org"; const identityServerUrl = "https://is.org"; const clientId = "xyz789"; const baseUrl = "https://test.com"; const delegatedAuthConfig = makeDelegatedAuthConfig(issuer); - const sessionStorageGetSpy = jest.spyOn(sessionStorage.__proto__, "setItem").mockReturnValue(undefined); - // to restore later const realWindowLocation = window.location; beforeEach(() => { - fetchMock.mockClear(); - fetchMock.resetBehavior(); - - sessionStorageGetSpy.mockReset(); - // @ts-ignore allow delete of non-optional prop delete window.location; // @ts-ignore ugly mocking @@ -50,17 +48,11 @@ describe("OIDC authorization", () => { href: baseUrl, origin: baseUrl, }; - - fetchMockJest.get( - delegatedAuthConfig.metadata.issuer + ".well-known/openid-configuration", - mockOpenIdConfiguration(), - ); jest.spyOn(randomStringUtils, "randomString").mockRestore(); + }); - // annoying to mock jwt decoding used in validateIdToken - jest.spyOn(OidcValidation, "validateIdToken") - .mockClear() - .mockImplementation(() => {}); + beforeAll(() => { + fetchMock.get(`${delegatedAuthConfig.issuer}.well-known/openid-configuration`, delegatedAuthConfig.metadata); }); afterAll(() => { @@ -68,9 +60,8 @@ describe("OIDC authorization", () => { }); describe("startOidcLogin()", () => { - it("navigates to authorization endpoint with correct parameters", async () => { - await startOidcLogin(delegatedAuthConfig, clientId, homeserver); + await startOidcLogin(delegatedAuthConfig, clientId, homeserverUrl); const expectedScopeWithoutDeviceId = `openid urn:matrix:org.matrix.msc2967.client:api:* urn:matrix:org.matrix.msc2967.client:device:`; @@ -94,37 +85,30 @@ describe("OIDC authorization", () => { }); describe("completeOidcLogin()", () => { - const authorizationParams = generateAuthorizationParams({ redirectUri: baseUrl }); - const storedAuthorizationParams = { - nonce: authorizationParams.nonce, - redirectUri: baseUrl, - codeVerifier: authorizationParams.codeVerifier, - clientId, - homeserverUrl: homeserver, - identityServerUrl: identityServerUrl, - delegatedAuthConfig: JSON.stringify(delegatedAuthConfig), - }; + const state = "test-state-444"; const code = "test-code-777"; const queryDict = { code, - state: authorizationParams.state, + state: state, }; - const validBearerTokenResponse = { + + const tokenResponse: BearerTokenResponse = { + access_token: "abc123", + refresh_token: "def456", + scope: "test", token_type: "Bearer", - access_token: "test_access_token", - refresh_token: "test_refresh_token", - expires_in: 12345, + expires_at: 12345, }; beforeEach(() => { - sessionStorageGetSpy.mockImplementation((key: string) => { - const itemKey = key.split(`oidc_${authorizationParams.state}_`).pop(); - return storedAuthorizationParams[itemKey] || null; - }); - - fetchMock.post(delegatedAuthConfig.tokenEndpoint, { - status: 200, - body: validBearerTokenResponse, + mocked(completeAuthorizationCodeGrant).mockClear().mockResolvedValue({ + oidcClientSettings: { + clientId, + issuer, + }, + tokenResponse, + homeserverUrl, + identityServerUrl, }); }); @@ -134,53 +118,20 @@ describe("OIDC authorization", () => { ); }); - it("should throw when authorization params are not found in session storage", async () => { - const queryDict = { - code: "abc123", - state: "not-the-same-state-we-put-things-in-local-storage-with", - }; - - expect(async () => await completeOidcLogin(queryDict)).rejects.toThrow( - "Cannot complete OIDC login: required properties not found in session storage", - ); - // tried to retreive using state as part of storage key - expect(sessionStorageGetSpy).toHaveBeenCalledWith(`oidc_${queryDict.state}_nonce`); - }); - - it("should make request to token endpoint", async () => { + it("should make request complete authorization code grant", async () => { await completeOidcLogin(queryDict); - expect(fetchMock).toHaveBeenCalledWith(delegatedAuthConfig.tokenEndpoint, { - method: Method.Post, - headers: { "Content-Type": "application/x-www-form-urlencoded" }, - body: `grant_type=authorization_code&client_id=${clientId}&code_verifier=${authorizationParams.codeVerifier}&redirect_uri=https%3A%2F%2Ftest.com&code=${code}`, - }); + expect(completeAuthorizationCodeGrant).toHaveBeenCalledWith(code, state); }); it("should return accessToken, configured homeserver and identityServer", async () => { const result = await completeOidcLogin(queryDict); expect(result).toEqual({ - accessToken: validBearerTokenResponse.access_token, - homeserverUrl: homeserver, + accessToken: tokenResponse.access_token, + homeserverUrl, identityServerUrl, }); }); - - it("should handle when no identityServer is stored in session storage", async () => { - // session storage mock without identity server stored - const { identityServerUrl: _excludingThis, ...storedParamsWithoutIs } = storedAuthorizationParams; - sessionStorageGetSpy.mockImplementation((key: string) => { - const itemKey = key.split(`oidc_${authorizationParams.state}_`).pop(); - return storedParamsWithoutIs[itemKey] || null; - }); - const result = await completeOidcLogin(queryDict); - - expect(result).toEqual({ - accessToken: validBearerTokenResponse.access_token, - homeserverUrl: homeserver, - identityServerUrl: undefined, - }); - }); }); });