-
-
Notifications
You must be signed in to change notification settings - Fork 827
OIDC: Check static client registration and add login flow #11088
Changes from 12 commits
af63a90
9c89f41
422823e
aa49696
e1e22f3
eef05f1
9fc1907
63bc37a
6cbcbc8
9782917
cc34839
5913c43
63b83ff
fd14df3
f5be14b
5395585
50d04ef
8ab88fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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; | ||||||||
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?: 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( | ||||||||
|
@@ -40,6 +58,7 @@ export default class Login { | |||||||
opts: ILoginOptions, | ||||||||
) { | ||||||||
this.defaultDeviceDisplayName = opts.defaultDeviceDisplayName; | ||||||||
this.delegatedAuthentication = opts.delegatedAuthentication; | ||||||||
} | ||||||||
|
||||||||
public getHomeserverUrl(): string { | ||||||||
|
@@ -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 | ||||||||
|
@@ -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 { | ||||||||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
type: "oidcNativeFlow"; | ||||||||
// this client's id as registered with the configured OIDC OP | ||||||||
clientId: string; | ||||||||
} | ||||||||
/** | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, I guess I missed that conversation, but:
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
|
||||||||
* Returns OIDC native flow when client is ready to attempt login via OIDC native flow | ||||||||
* @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 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 | ||||||||
|
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 { | ||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
DynamicRegistrationNotSupported = "Dynamic registration not supported", | ||
DynamicRegistrationFailed = "Dynamic registration failed", | ||
DynamicRegistrationInvalid = "Dynamic registration invalid response", | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.