Skip to content
This repository was 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 @@ -194,6 +194,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
74 changes: 71 additions & 3 deletions src/Login.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,36 @@ 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 { DELEGATED_OIDC_COMPATIBILITY, ILoginFlow, ILoginParams, LoginFlow } from "matrix-js-sdk/src/@types/auth";

import { IMatrixClientCreds } from "./MatrixClientPeg";
import SecurityCustomisations from "./customisations/Security";
import { ValidatedDelegatedAuthConfig } from "./utils/ValidatedServerConfig";
import { getOidcClientId } from "./utils/oidc/registerClient";
import { IConfigOptions } from "./IConfigOptions";
import SdkConfig from "./SdkConfig";

/**
* Login flows supported by this client
* LoginFlow type use the client API /login endpoint
* OidcNativeFlow is specific to this client
*/
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?: ValidatedDelegatedAuthConfig;
}

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

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

public getHomeserverUrl(): string {
Expand Down Expand Up @@ -75,7 +94,22 @@ 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 if we have delegated auth config
if (this.delegatedAuthentication) {
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 +185,40 @@ export default class Login {
}
}

/**
* Describes the OIDC native login flow
* Separate from js-sdk's `LoginFlow` as this does not use the same /login flow
* to which that type belongs.
*/
export interface OidcNativeFlow extends ILoginFlow {
type: "oidcNativeFlow";
// this client's id as registered with the configured OIDC OP
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
* Returns OIDC native flow when client is ready to attempt login via OIDC native flow
* @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<OidcNativeFlow> when oidc native authentication flow is supported and correctly configured
* @throws when client can't register with OP, or any unexpected error
*/
const tryInitOidcNativeFlow = async (
delegatedAuthConfig: ValidatedDelegatedAuthConfig,
brand: string,
oidcStaticClients?: IConfigOptions["oidc_static_clients"],
): Promise<OidcNativeFlow> => {
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
75 changes: 47 additions & 28 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 Down Expand Up @@ -84,7 +85,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 +111,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 +161,10 @@ 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 ||
// delegatedAuthentication is only set by buildValidatedConfigFromDiscovery and won't be modified
// so shallow comparison is fine
prevProps.serverConfig.delegatedAuthentication !== this.props.serverConfig.delegatedAuthentication
) {
// Ensure that we end up actually logging in to the right place
this.initLoginLogic(this.props.serverConfig);
Expand Down Expand Up @@ -322,28 +330,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 +351,38 @@ 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,
// if native OIDC is enabled in the client pass the server's delegated auth settings
delegatedAuthentication: this.oidcNativeFlowEnabled
? this.props.serverConfig.delegatedAuthentication
: undefined,
});
this.loginLogic = loginLogic;

loginLogic
.getFlows()
Expand Down Expand Up @@ -401,7 +420,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
11 changes: 5 additions & 6 deletions src/utils/AutoDiscoveryUtils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,13 @@ 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 { 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";
import { ValidatedServerConfig } from "./ValidatedServerConfig";
import { ValidatedDelegatedAuthConfig, ValidatedServerConfig } from "./ValidatedServerConfig";

const LIVELINESS_DISCOVERY_ERRORS: string[] = [
AutoDiscovery.ERROR_INVALID_HOMESERVER,
Expand Down Expand Up @@ -266,14 +265,14 @@ export default class AutoDiscoveryUtils {
if (discoveryResult[M_AUTHENTICATION.stable!]?.state === AutoDiscovery.SUCCESS) {
const { authorizationEndpoint, registrationEndpoint, tokenEndpoint, account, issuer } = discoveryResult[
M_AUTHENTICATION.stable!
] as IDelegatedAuthConfig & ValidatedIssuerConfig;
delegatedAuthentication = {
] as ValidatedDelegatedAuthConfig;
delegatedAuthentication = Object.freeze({
authorizationEndpoint,
registrationEndpoint,
tokenEndpoint,
account,
issuer,
};
});
}

return {
Expand Down
10 changes: 9 additions & 1 deletion src/utils/ValidatedServerConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
import { IDelegatedAuthConfig } from "matrix-js-sdk/src/client";
import { ValidatedIssuerConfig } from "matrix-js-sdk/src/oidc/validate";

export type ValidatedDelegatedAuthConfig = IDelegatedAuthConfig & ValidatedIssuerConfig;

export interface ValidatedServerConfig {
hsUrl: string;
hsName: string;
Expand All @@ -30,5 +32,11 @@ export interface ValidatedServerConfig {

warning: string | Error;

delegatedAuthentication?: IDelegatedAuthConfig & ValidatedIssuerConfig;
/**
* Config related to delegated authentication
* Included when delegated auth is configured and valid, otherwise undefined
* From homeserver .well-known m.authentication, and issuer's ./well-known/openid-configuration
* Used for OIDC native flow authentication
*/
delegatedAuthentication?: ValidatedDelegatedAuthConfig;
}
24 changes: 24 additions & 0 deletions src/utils/oidc/error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
Copyright 2023 The Matrix.org Foundation C.I.C.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

/**
* OIDC error strings, intended for logging
*/
export enum OidcClientError {
DynamicRegistrationNotSupported = "Dynamic registration not supported",
DynamicRegistrationFailed = "Dynamic registration failed",
DynamicRegistrationInvalid = "Dynamic registration invalid response",
}
Loading