Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

OIDC: Check static client registration and add login flow #11088

Merged
merged 18 commits into from
Jun 22, 2023
Merged
8 changes: 8 additions & 0 deletions src/IConfigOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, string>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there an EW PR to document this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it be called oidc_static_client_ids ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is very much still in development I'm not sure it should be documented yet, I'll add a task to the ticket so I don't forget

Copy link
Member

@richvdh richvdh Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about renaming it? I think it would be helpful to reflect the fact that the target of the map is a client_id.

}

export interface ISsoRedirectOptions {
Expand Down
64 changes: 61 additions & 3 deletions src/Login.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
richvdh marked this conversation as resolved.
Show resolved Hide resolved

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
kerryarchibald marked this conversation as resolved.
Show resolved Hide resolved
*/
delegatedAuthentication?: ValidatedServerConfig["delegatedAuthentication"];
}

export default class Login {
private flows: Array<LoginFlow> = [];
private flows: Array<ClientLoginFlow> = [];
private readonly defaultDeviceDisplayName?: string;
private readonly delegatedAuthentication?: ValidatedServerConfig["delegatedAuthentication"];
private tempClient: MatrixClient | null = null; // memoize

public constructor(
Expand All @@ -40,6 +54,7 @@ export default class Login {
opts: ILoginOptions,
) {
this.defaultDeviceDisplayName = opts.defaultDeviceDisplayName;
this.delegatedAuthentication = opts.delegatedAuthentication;
}

public getHomeserverUrl(): string {
Expand Down Expand Up @@ -75,7 +90,20 @@ export default class Login {
return this.tempClient;
}

public async getFlows(): Promise<Array<LoginFlow>> {
public async getFlows(): Promise<Array<ClientLoginFlow>> {
// 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
Expand Down Expand Up @@ -151,6 +179,36 @@ export default class Login {
}
}

export interface OidcNativeFlow extends ILoginFlow {
richvdh marked this conversation as resolved.
Show resolved Hide resolved
type: "oidcNativeFlow";
clientId: string;
}
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
/**

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agreed some time ago in the web chapter that formatting nitpicks are not good PR feedback. If we want to enforce some formatting it should be configured in prettier/eslint

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, I guess I missed that conversation, but:

  1. I don't think it's even possible to come up with a set of rules for when it is appropriate to leave blank lines between sections of code. You might as well try and formulate a set of rules about when you should start a new paragraph between sentences in a document: it's entirely context-dependent.
  2. In any case, we don't yet have prettier/eslint rules to enforce this, so in its absence I don't think it's unreasonable to include suggestions like this.

In this instance, I think that running the type definition straight into the doc-comment for the next function is visually cluttered and it's easier to skim with a bit more whitespace. But feel free to disagree.

* Finds static clientId for configured issuer, or attempts dynamic registration with the OP
richvdh marked this conversation as resolved.
Show resolved Hide resolved
* @param delegatedAuthConfig Auth config from ValidatedServerConfig
kerryarchibald marked this conversation as resolved.
Show resolved Hide resolved
* @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<OidcNativeFlow> 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<OidcNativeFlow> => {
if (!delegatedAuthConfig) {
richvdh marked this conversation as resolved.
Show resolved Hide resolved
throw new Error(OidcDiscoveryError.NotSupported);
}
richvdh marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
73 changes: 44 additions & 29 deletions src/components/structures/auth/Login.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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.
Expand All @@ -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");

richvdh marked this conversation as resolved.
Show resolved Hide resolved
interface IProps {
serverConfig: ValidatedServerConfig;
// If true, the component will consider itself busy.
Expand Down Expand Up @@ -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;
Expand All @@ -110,13 +110,17 @@ type OnPasswordLogin = {
*/
export default class LoginComponent extends React.PureComponent<IProps, IState> {
private unmounted = false;
private oidcNativeFlowEnabled = false;
private loginLogic!: Login;

private readonly stepRendererMap: Record<string, () => ReactNode>;

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,
Expand Down Expand Up @@ -156,7 +160,8 @@ export default class LoginComponent extends React.PureComponent<IProps, IState>
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
richvdh marked this conversation as resolved.
Show resolved Hide resolved
) {
// Ensure that we end up actually logging in to the right place
this.initLoginLogic(this.props.serverConfig);
Expand Down Expand Up @@ -322,28 +327,10 @@ export default class LoginComponent extends React.PureComponent<IProps, IState>
}
};

private async initLoginLogic({ hsUrl, isUrl }: ValidatedServerConfig): Promise<void> {
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<ValidatedServerConfig, "hsUrl" | "isUrl">): Promise<void> {
// Do a quick liveliness check on the URLs
try {
const { warning } = await AutoDiscoveryUtils.validateServerConfigWithStaticUrls(hsUrl, isUrl);
Expand All @@ -361,9 +348,37 @@ export default class LoginComponent extends React.PureComponent<IProps, IState>
} catch (e) {
this.setState({
busy: false,
...AutoDiscoveryUtils.authComponentStateForError(e),
...AutoDiscoveryUtils.authComponentStateForError(e as Error),
});
}
}

private async initLoginLogic({ hsUrl, isUrl }: ValidatedServerConfig): Promise<void> {
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,
richvdh marked this conversation as resolved.
Show resolved Hide resolved
});
this.loginLogic = loginLogic;

loginLogic
.getFlows()
Expand Down Expand Up @@ -401,7 +416,7 @@ export default class LoginComponent extends React.PureComponent<IProps, IState>
});
}

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]) {
Expand Down
21 changes: 21 additions & 0 deletions src/utils/oidc/error.ts
Original file line number Diff line number Diff line change
@@ -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 {
richvdh marked this conversation as resolved.
Show resolved Hide resolved
DynamicRegistrationNotSupported = "Dynamic registration not supported",
DynamicRegistrationFailed = "Dynamic registration failed",
DynamicRegistrationInvalid = "Dynamic registration invalid response",
}
65 changes: 65 additions & 0 deletions src/utils/oidc/registerClient.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
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, string>): string | undefined => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to prefer the const foo = () => {...} syntax rather than function foo { ...}?

Likewise getOidcClientId

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just style

return staticOidcClients?.[issuer];
};

/**
* Get the clientId for an OIDC OP
* Checks statically configured clientIds first
* @param delegatedAuthConfig Auth config from ValidatedServerConfig
* @param clientName Client name to register with the OP, eg 'Element'
* @param baseUrl URL of the home page of the Client, eg 'https://app.element.io/'
* @param staticOidcClients static client config from config.json
* @returns Promise<string> resolves with clientId
* @throws if no clientId is found
*/
export const getOidcClientId = async (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an async function because dynamic registration is async and will happen here 🔜

delegatedAuthConfig: IDelegatedAuthConfig & ValidatedIssuerConfig,
// these are used in the following PR
_clientName: string,
_baseUrl: string,
staticOidcClients?: Record<string, string>,
): Promise<string> => {
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);
};
Loading