Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MSC2965 OIDC Discovery implementation #4064

Merged
merged 15 commits into from
Feb 23, 2024
Merged

Update MSC2965 OIDC Discovery implementation #4064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
96d003f
Remove m.authentication well-known handling as the MSC moved away fro…
t3chguy Feb 19, 2024
bae525f
Read account management endpoint from the OIDC issuer well-known
t3chguy Feb 19, 2024
0b07c2c
Change OIDC API methods shape to no longer rely on defunct well-known…
t3chguy Feb 19, 2024
c1d104b
Merge branch 'develop' of github.com:matrix-org/matrix-js-sdk into t3…
t3chguy Feb 20, 2024
2f812c4
Iterate
t3chguy Feb 21, 2024
0518006
Add account_management_actions_supported
t3chguy Feb 21, 2024
a4b9572
Simplify
t3chguy Feb 21, 2024
9baef5e
Validate `account_management_uri` and `account_management_actions_sup…
t3chguy Feb 21, 2024
8ec4e9d
Merge branch 't3chguy/oidc-extensions' of github.com:matrix-org/matri…
t3chguy Feb 21, 2024
0a48f4a
Merge branch 'develop' into t3chguy/fix/26908
t3chguy Feb 21, 2024
b91263b
Fix comment
t3chguy Feb 21, 2024
02bf791
Merge remote-tracking branch 'origin/t3chguy/fix/26908' into t3chguy/…
t3chguy Feb 21, 2024
664108d
Iterate comments
t3chguy Feb 22, 2024
5c6240b
Iterate
t3chguy Feb 22, 2024
a37d204
Merge branch 'develop' into t3chguy/fix/26908
t3chguy Feb 22, 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
6 changes: 2 additions & 4 deletions spec/test-utils/oidc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { OidcClientConfig } from "../../src";
import { ValidatedIssuerMetadata } from "../../src/oidc/validate";
import { OidcClientConfig, ValidatedIssuerMetadata } from "../../src";

/**
* Makes a valid OidcClientConfig with minimum valid values
Expand All @@ -26,8 +25,7 @@ export const makeDelegatedAuthConfig = (issuer = "https://auth.org/"): OidcClien
const metadata = mockOpenIdConfiguration(issuer);

return {
issuer,
account: issuer + "account",
accountManagementEndpoint: issuer + "account",
registrationEndpoint: metadata.registration_endpoint,
authorizationEndpoint: metadata.authorization_endpoint,
tokenEndpoint: metadata.token_endpoint,
Expand Down
99 changes: 1 addition & 98 deletions spec/unit/autodiscovery.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,10 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import fetchMock from "fetch-mock-jest";
import MockHttpBackend from "matrix-mock-request";

import { AutoDiscoveryAction, M_AUTHENTICATION } from "../../src";
import { AutoDiscoveryAction } from "../../src";
import { AutoDiscovery } from "../../src/autodiscovery";
import { OidcError } from "../../src/oidc/error";
import { makeDelegatedAuthConfig } from "../test-utils/oidc";

// keep to reset the fetch function after using MockHttpBackend
// @ts-ignore private property
Expand Down Expand Up @@ -409,10 +406,6 @@ describe("AutoDiscovery", function () {
error: null,
base_url: null,
},
"m.authentication": {
state: "IGNORE",
error: OidcError.NotSupported,
},
};

expect(conf).toEqual(expected);
Expand Down Expand Up @@ -450,10 +443,6 @@ describe("AutoDiscovery", function () {
error: null,
base_url: null,
},
"m.authentication": {
state: "IGNORE",
error: OidcError.NotSupported,
},
};

expect(conf).toEqual(expected);
Expand All @@ -476,9 +465,6 @@ describe("AutoDiscovery", function () {
// Note: we also expect this test to trim the trailing slash
base_url: "https://chat.example.org/",
},
"m.authentication": {
invalid: true,
},
});
return Promise.all([
httpBackend.flushAllExpected(),
Expand All @@ -494,10 +480,6 @@ describe("AutoDiscovery", function () {
error: null,
base_url: null,
},
"m.authentication": {
state: "FAIL_ERROR",
error: OidcError.Misconfigured,
},
};

expect(conf).toEqual(expected);
Expand Down Expand Up @@ -728,10 +710,6 @@ describe("AutoDiscovery", function () {
error: null,
base_url: "https://identity.example.org",
},
"m.authentication": {
state: "IGNORE",
error: OidcError.NotSupported,
},
};

expect(conf).toEqual(expected);
Expand Down Expand Up @@ -784,10 +762,6 @@ describe("AutoDiscovery", function () {
"org.example.custom.property": {
cupcakes: "yes",
},
"m.authentication": {
state: "IGNORE",
error: OidcError.NotSupported,
},
};

expect(conf).toEqual(expected);
Expand Down Expand Up @@ -897,75 +871,4 @@ describe("AutoDiscovery", function () {
}),
]);
});

describe("m.authentication", () => {
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate these tests don't really apply any more, but it worries me to see tests being ripped out without new ones to replace them. Shouldn't we have similar tests for the new endpoint?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a test for the new endpoint now, it landed as its own PR.

As for the rest of it, its just removing code and changing types, given tests have been updated with stubs matching the new types I'd consider that tested

const homeserverName = "example.org";
const homeserverUrl = "https://chat.example.org/";
const issuer = "https://auth.org/";

beforeAll(() => {
// make these tests independent from fetch mocking above
AutoDiscovery.setFetchFn(realAutoDiscoveryFetch);
});

beforeEach(() => {
fetchMock.resetBehavior();
fetchMock.get(`${homeserverUrl}_matrix/client/versions`, { versions: ["v1.5"] });

fetchMock.get("https://example.org/.well-known/matrix/client", {
"m.homeserver": {
// Note: we also expect this test to trim the trailing slash
base_url: "https://chat.example.org/",
},
"m.authentication": {
issuer,
},
});
});

it("should return valid authentication configuration", async () => {
const config = makeDelegatedAuthConfig(issuer);

fetchMock.get(`${config.metadata.issuer}.well-known/openid-configuration`, config.metadata);
fetchMock.get(`${config.metadata.issuer}jwks`, {
status: 200,
headers: {
"Content-Type": "application/json",
},
keys: [],
});

const result = await AutoDiscovery.findClientConfig(homeserverName);

expect(result[M_AUTHENTICATION.stable!]).toEqual({
state: AutoDiscovery.SUCCESS,
...config,
signingKeys: [],
account: undefined,
error: null,
});
});

it("should set state to error for invalid authentication configuration", async () => {
const config = makeDelegatedAuthConfig(issuer);
// authorization_code is required
config.metadata.grant_types_supported = ["openid"];

fetchMock.get(`${config.metadata.issuer}.well-known/openid-configuration`, config.metadata);
fetchMock.get(`${config.metadata.issuer}jwks`, {
status: 200,
headers: {
"Content-Type": "application/json",
},
keys: [],
});

const result = await AutoDiscovery.findClientConfig(homeserverName);

expect(result[M_AUTHENTICATION.stable!]).toEqual({
state: AutoDiscovery.FAIL_ERROR,
error: OidcError.OpSupport,
});
});
});
});
9 changes: 6 additions & 3 deletions spec/unit/oidc/authorize.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ const realSubtleCrypto = crypto.subtleCrypto;

describe("oidc authorization", () => {
const delegatedAuthConfig = makeDelegatedAuthConfig();
const authorizationEndpoint = delegatedAuthConfig.metadata.authorization_endpoint;
const tokenEndpoint = delegatedAuthConfig.metadata.token_endpoint;
const authorizationEndpoint = delegatedAuthConfig.authorizationEndpoint;
const tokenEndpoint = delegatedAuthConfig.tokenEndpoint;
const clientId = "xyz789";
const baseUrl = "https://test.com";

Expand All @@ -58,7 +58,10 @@ describe("oidc authorization", () => {
jest.spyOn(logger, "warn");
jest.setSystemTime(now);

fetchMock.get(delegatedAuthConfig.issuer + ".well-known/openid-configuration", mockOpenIdConfiguration());
fetchMock.get(
delegatedAuthConfig.metadata.issuer + ".well-known/openid-configuration",
mockOpenIdConfiguration(),
);

Object.defineProperty(window, "crypto", {
value: {
Expand Down
17 changes: 6 additions & 11 deletions spec/unit/oidc/register.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ import fetchMockJest from "fetch-mock-jest";

import { OidcError } from "../../../src/oidc/error";
import { OidcRegistrationClientMetadata, registerOidcClient } from "../../../src/oidc/register";
import { makeDelegatedAuthConfig } from "../../test-utils/oidc";

describe("registerOidcClient()", () => {
const issuer = "https://auth.com/";
const registrationEndpoint = "https://auth.com/register";
const clientName = "Element";
const baseUrl = "https://just.testing";
const metadata: OidcRegistrationClientMetadata = {
Expand All @@ -35,25 +35,20 @@ describe("registerOidcClient()", () => {
};
const dynamicClientId = "xyz789";

const delegatedAuthConfig = {
issuer,
registrationEndpoint,
authorizationEndpoint: issuer + "auth",
tokenEndpoint: issuer + "token",
};
const delegatedAuthConfig = makeDelegatedAuthConfig(issuer);
beforeEach(() => {
fetchMockJest.mockClear();
fetchMockJest.resetBehavior();
});

it("should make correct request to register client", async () => {
fetchMockJest.post(registrationEndpoint, {
fetchMockJest.post(delegatedAuthConfig.registrationEndpoint!, {
status: 200,
body: JSON.stringify({ client_id: dynamicClientId }),
});
expect(await registerOidcClient(delegatedAuthConfig, metadata)).toEqual(dynamicClientId);
expect(fetchMockJest).toHaveBeenCalledWith(
registrationEndpoint,
delegatedAuthConfig.registrationEndpoint!,
expect.objectContaining({
headers: {
"Accept": "application/json",
Expand All @@ -77,7 +72,7 @@ describe("registerOidcClient()", () => {
});

it("should throw when registration request fails", async () => {
fetchMockJest.post(registrationEndpoint, {
fetchMockJest.post(delegatedAuthConfig.registrationEndpoint!, {
status: 500,
});
await expect(() => registerOidcClient(delegatedAuthConfig, metadata)).rejects.toThrow(
Expand All @@ -86,7 +81,7 @@ describe("registerOidcClient()", () => {
});

it("should throw when registration response is invalid", async () => {
fetchMockJest.post(registrationEndpoint, {
fetchMockJest.post(delegatedAuthConfig.registrationEndpoint!, {
status: 200,
// no clientId in response
body: "{}",
Expand Down
32 changes: 16 additions & 16 deletions spec/unit/oidc/tokenRefresher.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe("OidcTokenRefresher", () => {
keys: [],
});

fetchMock.post(config.metadata.token_endpoint, {
fetchMock.post(config.tokenEndpoint, {
status: 200,
headers: {
"Content-Type": "application/json",
Expand All @@ -88,7 +88,7 @@ describe("OidcTokenRefresher", () => {
},
{ overwriteRoutes: true },
);
const refresher = new OidcTokenRefresher(authConfig, clientId, redirectUri, deviceId, idTokenClaims);
const refresher = new OidcTokenRefresher(authConfig.issuer, clientId, redirectUri, deviceId, idTokenClaims);
await expect(refresher.oidcClientReady).rejects.toThrow();
expect(logger.error).toHaveBeenCalledWith(
"Failed to initialise OIDC client.",
Expand All @@ -98,7 +98,7 @@ describe("OidcTokenRefresher", () => {
});

it("initialises oidc client", async () => {
const refresher = new OidcTokenRefresher(authConfig, clientId, redirectUri, deviceId, idTokenClaims);
const refresher = new OidcTokenRefresher(authConfig.issuer, clientId, redirectUri, deviceId, idTokenClaims);
await refresher.oidcClientReady;

// @ts-ignore peek at private property to see we initialised the client correctly
Expand All @@ -114,19 +114,19 @@ describe("OidcTokenRefresher", () => {

describe("doRefreshAccessToken()", () => {
it("should throw when oidcClient has not been initialised", async () => {
const refresher = new OidcTokenRefresher(authConfig, clientId, redirectUri, deviceId, idTokenClaims);
const refresher = new OidcTokenRefresher(authConfig.issuer, clientId, redirectUri, deviceId, idTokenClaims);
await expect(refresher.doRefreshAccessToken("token")).rejects.toThrow(
"Cannot get new token before OIDC client is initialised.",
);
});

it("should refresh the tokens", async () => {
const refresher = new OidcTokenRefresher(authConfig, clientId, redirectUri, deviceId, idTokenClaims);
const refresher = new OidcTokenRefresher(authConfig.issuer, clientId, redirectUri, deviceId, idTokenClaims);
await refresher.oidcClientReady;

const result = await refresher.doRefreshAccessToken("refresh-token");

expect(fetchMock).toHaveFetched(config.metadata.token_endpoint, {
expect(fetchMock).toHaveFetched(config.tokenEndpoint, {
method: "POST",
});

Expand All @@ -137,7 +137,7 @@ describe("OidcTokenRefresher", () => {
});

it("should persist the new tokens", async () => {
const refresher = new OidcTokenRefresher(authConfig, clientId, redirectUri, deviceId, idTokenClaims);
const refresher = new OidcTokenRefresher(authConfig.issuer, clientId, redirectUri, deviceId, idTokenClaims);
await refresher.oidcClientReady;
// spy on our stub
jest.spyOn(refresher, "persistTokens");
Expand All @@ -153,7 +153,7 @@ describe("OidcTokenRefresher", () => {
it("should only have one inflight refresh request at once", async () => {
fetchMock
.postOnce(
config.metadata.token_endpoint,
config.tokenEndpoint,
{
status: 200,
headers: {
Expand All @@ -164,7 +164,7 @@ describe("OidcTokenRefresher", () => {
{ overwriteRoutes: true },
)
.postOnce(
config.metadata.token_endpoint,
config.tokenEndpoint,
{
status: 200,
headers: {
Expand All @@ -175,7 +175,7 @@ describe("OidcTokenRefresher", () => {
{ overwriteRoutes: false },
);

const refresher = new OidcTokenRefresher(authConfig, clientId, redirectUri, deviceId, idTokenClaims);
const refresher = new OidcTokenRefresher(authConfig.issuer, clientId, redirectUri, deviceId, idTokenClaims);
await refresher.oidcClientReady;
// reset call counts
fetchMock.resetHistory();
Expand All @@ -188,7 +188,7 @@ describe("OidcTokenRefresher", () => {
const result2 = await first;

// only one call to token endpoint
expect(fetchMock).toHaveFetchedTimes(1, config.metadata.token_endpoint);
expect(fetchMock).toHaveFetchedTimes(1, config.tokenEndpoint);
expect(result1).toEqual({
accessToken: "first-new-access-token",
refreshToken: "first-new-refresh-token",
Expand All @@ -208,7 +208,7 @@ describe("OidcTokenRefresher", () => {

it("should log and rethrow when token refresh fails", async () => {
fetchMock.post(
config.metadata.token_endpoint,
config.tokenEndpoint,
{
status: 503,
headers: {
Expand All @@ -218,7 +218,7 @@ describe("OidcTokenRefresher", () => {
{ overwriteRoutes: true },
);

const refresher = new OidcTokenRefresher(authConfig, clientId, redirectUri, deviceId, idTokenClaims);
const refresher = new OidcTokenRefresher(authConfig.issuer, clientId, redirectUri, deviceId, idTokenClaims);
await refresher.oidcClientReady;

await expect(refresher.doRefreshAccessToken("refresh-token")).rejects.toThrow();
Expand All @@ -228,7 +228,7 @@ describe("OidcTokenRefresher", () => {
// make sure inflight request is cleared after a failure
fetchMock
.postOnce(
config.metadata.token_endpoint,
config.tokenEndpoint,
{
status: 503,
headers: {
Expand All @@ -238,7 +238,7 @@ describe("OidcTokenRefresher", () => {
{ overwriteRoutes: true },
)
.postOnce(
config.metadata.token_endpoint,
config.tokenEndpoint,
{
status: 200,
headers: {
Expand All @@ -249,7 +249,7 @@ describe("OidcTokenRefresher", () => {
{ overwriteRoutes: false },
);

const refresher = new OidcTokenRefresher(authConfig, clientId, redirectUri, deviceId, idTokenClaims);
const refresher = new OidcTokenRefresher(authConfig.issuer, clientId, redirectUri, deviceId, idTokenClaims);
await refresher.oidcClientReady;
// reset call counts
fetchMock.resetHistory();
Expand Down
Loading
Loading