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

Update MSC2965 OIDC Discovery implementation #12245

Merged
merged 39 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
5a1ce08
Update MSC2965 OIDC Discovery implementation
t3chguy Feb 13, 2024
5099410
Remove unused interface
t3chguy Feb 13, 2024
ee26f17
Remove spurious test setup
t3chguy Feb 13, 2024
def3b9d
Remove spurious `--rm`
t3chguy Feb 14, 2024
14fca80
Fix OIDC-aware throwing error before resolving account management url
t3chguy Feb 14, 2024
d69a917
Split playwright postgres class out into ABC
t3chguy Feb 14, 2024
6b7d53b
Tidy up playwright fixtures
t3chguy Feb 14, 2024
9593e70
Add oidc-aware playwright test
t3chguy Feb 14, 2024
f2ddc5c
Avoid race condition where delegated account management is not shown …
t3chguy Feb 14, 2024
23bad14
delint
t3chguy Feb 14, 2024
b49125d
Make the app fixture lazy to avoid clobbering network routing
t3chguy Feb 14, 2024
a37c7a4
Iterate
t3chguy Feb 14, 2024
c887fd6
Fix authenticatedIssuer
t3chguy Feb 14, 2024
bf29874
Merge branch 'develop' of github.com:matrix-org/matrix-react-sdk into…
t3chguy Feb 14, 2024
07cb4cb
Merge branch 'develop' of github.com:matrix-org/matrix-react-sdk into…
t3chguy Feb 20, 2024
acdc987
Resolve race condition between opening settings & well-known check in…
t3chguy Feb 16, 2024
4d01fff
Add OIDC-aware and OIDC-native tests using MAS
t3chguy Feb 16, 2024
7f641a4
Merge branch 't3chguy/e2e-oidc-aware' of github.com:matrix-org/matrix…
t3chguy Feb 21, 2024
7688e97
Fix bad merge
t3chguy Feb 21, 2024
c5b50f0
Iterate
t3chguy Feb 21, 2024
9ebe5ab
Merge branch 'develop' of github.com:matrix-org/matrix-react-sdk into…
t3chguy Feb 21, 2024
a1ab218
Gather validated OIDC config during autodiscovery
t3chguy Feb 21, 2024
2f1a39b
Fix tests
t3chguy Feb 21, 2024
60d9b8c
Simplify
t3chguy Feb 21, 2024
68f8ea6
Add tests
t3chguy Feb 21, 2024
563274e
Simplify test
t3chguy Feb 22, 2024
0c4b03c
Iterate
t3chguy Feb 22, 2024
cacf98d
Iterate
t3chguy Feb 22, 2024
78a240a
Fix tests
t3chguy Feb 22, 2024
e9f5760
Fix tests
t3chguy Feb 22, 2024
9bd4b41
Pass error back instead of throwing it
t3chguy Feb 22, 2024
cffab76
Iterate
t3chguy Feb 22, 2024
212e883
Fix test
t3chguy Feb 22, 2024
10a2bc0
Fix tests
t3chguy Feb 22, 2024
9ead3eb
Merge branch 'develop' into t3chguy/fix/26908
t3chguy Feb 22, 2024
22f63b4
Iterate
t3chguy Feb 23, 2024
708a7d2
Merge remote-tracking branch 'origin/t3chguy/fix/26908' into t3chguy/…
t3chguy Feb 23, 2024
3dc8272
Apply suggestions from code review
t3chguy Feb 23, 2024
ef84256
Merge branch 'develop' into t3chguy/fix/26908
t3chguy Feb 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Lifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -795,7 +795,7 @@ async function createOidcTokenRefresher(credentials: IMatrixClientCreds): Promis
throw new Error("Expected deviceId in user credentials.");
}
const tokenRefresher = new TokenRefresher(
{ issuer: tokenIssuer },
tokenIssuer,
clientId,
redirectUri,
deviceId,
Expand Down
8 changes: 4 additions & 4 deletions src/components/views/dialogs/ServerPickerDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,6 @@ export default class ServerPickerDialog extends React.PureComponent<IProps, ISta
this.setState({ otherHomeserver: ev.target.value });
};

// TODO: Do we want to support .well-known lookups here?
// If for some reason someone enters "matrix.org" for a URL, we could do a lookup to
// find their homeserver without demanding they use "https://matrix.org"
private validate = withValidation<this, { error?: string }>({
deriveData: async ({ value }): Promise<{ error?: string }> => {
let hsUrl = (value ?? "").trim(); // trim to account for random whitespace
Expand All @@ -91,7 +88,10 @@ export default class ServerPickerDialog extends React.PureComponent<IProps, ISta
if (!hsUrl.includes("://")) {
try {
const discoveryResult = await AutoDiscovery.findClientConfig(hsUrl);
this.validatedConf = AutoDiscoveryUtils.buildValidatedConfigFromDiscovery(hsUrl, discoveryResult);
this.validatedConf = await AutoDiscoveryUtils.buildValidatedConfigFromDiscovery(
hsUrl,
discoveryResult,
);
return {}; // we have a validated config, we don't need to try the other paths
} catch (e) {
logger.error(`Attempted ${hsUrl} as a server_name but it failed`, e);
Expand Down
33 changes: 19 additions & 14 deletions src/stores/oidc/OidcClientStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,20 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { IDelegatedAuthConfig, MatrixClient, M_AUTHENTICATION } from "matrix-js-sdk/src/matrix";
import { discoverAndValidateAuthenticationConfig } from "matrix-js-sdk/src/oidc/discovery";
import { MatrixClient, discoverAndValidateOIDCIssuerWellKnown } from "matrix-js-sdk/src/matrix";
import { logger } from "matrix-js-sdk/src/logger";
import { OidcClient } from "oidc-client-ts";

import { getStoredOidcTokenIssuer, getStoredOidcClientId } from "../../utils/oidc/persistOidcSettings";
import { getDelegatedAuthAccountUrl } from "../../utils/oidc/getDelegatedAuthAccountUrl";
import PlatformPeg from "../../PlatformPeg";

/**
* @experimental
* Stores information about configured OIDC provider
* In OIDC Native mode the client is registered with OIDC directly and maintains an OIDC token
* In OIDC Aware mode, the client is aware that the Server is using OIDC, but is using the standard Matrix APIs for most things,
* notable exceptions are account management where a link to the account management endpoint will be provided instead.
* Otherwise, the store is not operating, the auth is then in Legacy mode and everything uses normal Matrix APIs.
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
*/
export class OidcClientStore {
private oidcClient?: OidcClient;
Expand All @@ -47,8 +49,16 @@ export class OidcClientStore {
if (this.authenticatedIssuer) {
await this.getOidcClient();
} else {
const wellKnown = await this.matrixClient.waitForClientWellKnown();
this._accountManagementEndpoint = getDelegatedAuthAccountUrl(wellKnown);
// We are not in OIDC Native mode as we have no locally stored issuer, check if the server delegates auth to OIDC.
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
try {
const authIssuer = await this.matrixClient.getAuthIssuer();
const { accountManagementEndpoint, metadata } = await discoverAndValidateOIDCIssuerWellKnown(
authIssuer.issuer,
);
this._accountManagementEndpoint = accountManagementEndpoint ?? metadata.issuer;
} catch (e) {
console.log("Auth issuer not found", e);
}
}
}

Expand Down Expand Up @@ -127,23 +137,18 @@ export class OidcClientStore {
* @returns promise that resolves when initialising OidcClient succeeds or fails
*/
private async initOidcClient(): Promise<void> {
const wellKnown = await this.matrixClient.waitForClientWellKnown();
if (!wellKnown && !this.authenticatedIssuer) {
if (!this.authenticatedIssuer) {
logger.error("Cannot initialise OIDC client without issuer.");
return;
}
const delegatedAuthConfig =
(wellKnown && M_AUTHENTICATION.findIn<IDelegatedAuthConfig>(wellKnown)) ?? undefined;

try {
const clientId = getStoredOidcClientId();
const { account, metadata, signingKeys } = await discoverAndValidateAuthenticationConfig(
// if HS has valid delegated auth config in .well-known, use it
// otherwise fallback to the known issuer
delegatedAuthConfig ?? { issuer: this.authenticatedIssuer! },
const { accountManagementEndpoint, metadata, signingKeys } = await discoverAndValidateOIDCIssuerWellKnown(
this.authenticatedIssuer,
);
// if no account endpoint is configured default to the issuer
this._accountManagementEndpoint = account ?? metadata.issuer;
this._accountManagementEndpoint = accountManagementEndpoint ?? metadata.issuer;
this.oidcClient = new OidcClient({
...metadata,
authority: metadata.issuer,
Expand Down
44 changes: 20 additions & 24 deletions src/utils/AutoDiscoveryUtils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@ import {
AutoDiscovery,
AutoDiscoveryError,
ClientConfig,
OidcClientConfig,
M_AUTHENTICATION,
discoverAndValidateOIDCIssuerWellKnown,
IClientWellKnown,
MatrixClient,
MatrixError,
OidcClientConfig,
} from "matrix-js-sdk/src/matrix";
import { logger } from "matrix-js-sdk/src/logger";

Expand Down Expand Up @@ -217,12 +219,12 @@ export default class AutoDiscoveryUtils {
* @param {boolean} isSynthetic If true, then the discoveryResult was synthesised locally.
* @returns {Promise<ValidatedServerConfig>} Resolves to the validated configuration.
*/
public static buildValidatedConfigFromDiscovery(
public static async buildValidatedConfigFromDiscovery(
serverName?: string,
discoveryResult?: ClientConfig,
syntaxOnly = false,
isSynthetic = false,
): ValidatedServerConfig {
): Promise<ValidatedServerConfig> {
if (!discoveryResult?.["m.homeserver"]) {
// This shouldn't happen without major misconfiguration, so we'll log a bit of information
// in the log so we can find this bit of code but otherwise tell the user "it broke".
Expand Down Expand Up @@ -293,26 +295,20 @@ export default class AutoDiscoveryUtils {
throw new UserFriendlyError("auth|autodiscovery_unexpected_error_hs");
}

// This isn't inherently auto-discovery but used to be in an earlier incarnation of the MSC,
// and shuttling the data together makes a lot of sense
let delegatedAuthentication: OidcClientConfig | undefined;
if (discoveryResult[M_AUTHENTICATION.stable!]?.state === AutoDiscovery.SUCCESS) {
const {
authorizationEndpoint,
registrationEndpoint,
tokenEndpoint,
account,
issuer,
metadata,
signingKeys,
} = discoveryResult[M_AUTHENTICATION.stable!] as OidcClientConfig;
delegatedAuthentication = Object.freeze({
authorizationEndpoint,
registrationEndpoint,
tokenEndpoint,
account,
issuer,
metadata,
signingKeys,
});
let delegatedAuthenticationError: Error | undefined;
try {
const tempClient = new MatrixClient({ baseUrl: preferredHomeserverUrl });
const { issuer } = await tempClient.getAuthIssuer();
delegatedAuthentication = await discoverAndValidateOIDCIssuerWellKnown(issuer);
} catch (e) {
if (e instanceof MatrixError && e.httpStatus === 404 && e.errcode === "M_UNRECOGNIZED") {
// 404 M_UNRECOGNIZED means the server does not support OIDC
} else {
delegatedAuthenticationError = e as Error;
}
}

return {
Expand All @@ -321,7 +317,7 @@ export default class AutoDiscoveryUtils {
hsNameIsDifferent: url.hostname !== preferredHomeserverName,
isUrl: preferredIdentityUrl,
isDefault: false,
warning: hsResult.error,
warning: hsResult.error ?? delegatedAuthenticationError ?? null,
isNameResolvable: !isSynthetic,
delegatedAuthentication,
} as ValidatedServerConfig;
Expand Down
11 changes: 4 additions & 7 deletions src/utils/ValidatedServerConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { OidcClientConfig, IDelegatedAuthConfig } from "matrix-js-sdk/src/matrix";
import { ValidatedIssuerConfig } from "matrix-js-sdk/src/oidc/validate";

export type ValidatedDelegatedAuthConfig = IDelegatedAuthConfig & ValidatedIssuerConfig;
import { OidcClientConfig } from "matrix-js-sdk/src/matrix";

export interface ValidatedServerConfig {
hsUrl: string;
Expand All @@ -34,9 +31,9 @@ export interface ValidatedServerConfig {

/**
* 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
* Included when delegated auth is configured and valid, otherwise undefined.
* From issuer's .well-known/openid-configuration.
* Used for OIDC native flow authentication.
*/
delegatedAuthentication?: OidcClientConfig;
}
6 changes: 3 additions & 3 deletions src/utils/oidc/TokenRefresher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { IDelegatedAuthConfig, OidcTokenRefresher, AccessTokens } from "matrix-js-sdk/src/matrix";
import { OidcTokenRefresher, AccessTokens } from "matrix-js-sdk/src/matrix";
import { IdTokenClaims } from "oidc-client-ts";

import PlatformPeg from "../../PlatformPeg";
Expand All @@ -28,14 +28,14 @@ export class TokenRefresher extends OidcTokenRefresher {
private readonly deviceId!: string;

public constructor(
authConfig: IDelegatedAuthConfig,
issuer: string,
clientId: string,
redirectUri: string,
deviceId: string,
idTokenClaims: IdTokenClaims,
private readonly userId: string,
) {
super(authConfig, clientId, deviceId, redirectUri, idTokenClaims);
super(issuer, clientId, deviceId, redirectUri, idTokenClaims);
this.deviceId = deviceId;
}

Expand Down
27 changes: 0 additions & 27 deletions src/utils/oidc/getDelegatedAuthAccountUrl.ts

This file was deleted.

9 changes: 4 additions & 5 deletions src/utils/oidc/registerClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,9 @@ limitations under the License.
*/

import { logger } from "matrix-js-sdk/src/logger";
import { registerOidcClient } from "matrix-js-sdk/src/oidc/register";
import { registerOidcClient, OidcClientConfig } from "matrix-js-sdk/src/matrix";

import { IConfigOptions } from "../../IConfigOptions";
import { ValidatedDelegatedAuthConfig } from "../ValidatedServerConfig";
import PlatformPeg from "../../PlatformPeg";

/**
Expand Down Expand Up @@ -46,12 +45,12 @@ const getStaticOidcClientId = (
* @throws if no clientId is found
*/
export const getOidcClientId = async (
delegatedAuthConfig: ValidatedDelegatedAuthConfig,
delegatedAuthConfig: OidcClientConfig,
staticOidcClients?: IConfigOptions["oidc_static_clients"],
): Promise<string> => {
const staticClientId = getStaticOidcClientId(delegatedAuthConfig.issuer, staticOidcClients);
const staticClientId = getStaticOidcClientId(delegatedAuthConfig.metadata.issuer, staticOidcClients);
if (staticClientId) {
logger.debug(`Using static clientId for issuer ${delegatedAuthConfig.issuer}`);
logger.debug(`Using static clientId for issuer ${delegatedAuthConfig.metadata.issuer}`);
return staticClientId;
}
return await registerOidcClient(delegatedAuthConfig, await PlatformPeg.get()!.getOidcClientMetadata());
Expand Down
22 changes: 11 additions & 11 deletions test/Lifecycle-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -683,10 +683,10 @@ describe("Lifecycle", () => {

beforeAll(() => {
fetchMock.get(
`${delegatedAuthConfig.issuer}.well-known/openid-configuration`,
`${delegatedAuthConfig.metadata.issuer}.well-known/openid-configuration`,
delegatedAuthConfig.metadata,
);
fetchMock.get(`${delegatedAuthConfig.issuer}jwks`, {
fetchMock.get(`${delegatedAuthConfig.metadata.issuer}jwks`, {
status: 200,
headers: {
"Content-Type": "application/json",
Expand All @@ -696,12 +696,6 @@ describe("Lifecycle", () => {
});

beforeEach(() => {
// mock oidc config for oidc client initialisation
mockClient.waitForClientWellKnown.mockResolvedValue({
"m.authentication": {
issuer: issuer,
},
});
initSessionStorageMock();
// set values in session storage as they would be after a successful oidc authentication
persistOidcAuthenticatedSettings(clientId, issuer, idTokenClaims);
Expand All @@ -711,7 +705,9 @@ describe("Lifecycle", () => {
await setLoggedIn(credentials);

// didn't try to initialise token refresher
expect(fetchMock).not.toHaveFetched(`${delegatedAuthConfig.issuer}.well-known/openid-configuration`);
expect(fetchMock).not.toHaveFetched(
`${delegatedAuthConfig.metadata.issuer}.well-known/openid-configuration`,
);
});

it("should not try to create a token refresher without a deviceId", async () => {
Expand All @@ -722,7 +718,9 @@ describe("Lifecycle", () => {
});

// didn't try to initialise token refresher
expect(fetchMock).not.toHaveFetched(`${delegatedAuthConfig.issuer}.well-known/openid-configuration`);
expect(fetchMock).not.toHaveFetched(
`${delegatedAuthConfig.metadata.issuer}.well-known/openid-configuration`,
);
});

it("should not try to create a token refresher without an issuer in session storage", async () => {
Expand All @@ -738,7 +736,9 @@ describe("Lifecycle", () => {
});

// didn't try to initialise token refresher
expect(fetchMock).not.toHaveFetched(`${delegatedAuthConfig.issuer}.well-known/openid-configuration`);
expect(fetchMock).not.toHaveFetched(
`${delegatedAuthConfig.metadata.issuer}.well-known/openid-configuration`,
);
});

it("should create a client with a tokenRefreshFunction", async () => {
Expand Down
5 changes: 5 additions & 0 deletions test/components/structures/MatrixChat-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,11 @@ describe("<MatrixChat />", () => {
unstable_features: {},
versions: SERVER_SUPPORTED_MATRIX_VERSIONS,
});
fetchMock.catch({
status: 404,
body: '{"errcode": "M_UNRECOGNIZED", "error": "Unrecognized request"}',
headers: { "content-type": "application/json" },
});

jest.spyOn(StorageManager, "idbLoad").mockReset();
jest.spyOn(StorageManager, "idbSave").mockResolvedValue(undefined);
Expand Down
Loading
Loading