Update MSC2965 OIDC Discovery implementation #4064

t3chguy · 2024-02-13T15:09:41Z

https://github.com/sandhose/matrix-doc/blob/msc/sandhose/oidc-discovery/proposals/2965-oidc-discovery.md

Switches from m.authentication in .well-known/matrix/client to GET /auth_issuer API as the MSC has done.

Requires #4074

Here's what your changelog entry will look like:

✨ Features

Update MSC2965 OIDC Discovery implementation (#4064).

richvdh

Help me out here. This is a chunky change, and is going to be time-consuming to review without any context.

What is the actual change in behaviour here, in more detail than "update"? I see the link to the MSC doc, but that in itself is a chunky document, and presumably some of it was previously implemented?

Is it possible to break up the change a bit, to make it easier to review (eg, it looks like one of the changes is to strip out support for m.authentication? I don't really understand why that's happening, given the MSC still seems to talk about it, but can you pull that out to its own commit or its own PR?)

t3chguy · 2024-02-14T12:57:14Z

What is the actual change in behaviour here, in more detail than "update"?

I don't really understand why that's happening, given the MSC still seems to talk about it, but can you pull that out to its own commit or its own PR?)

The MSC moved from advertising the OIDC issuer via .well-known/matrix/client to a dedicated API GET /auth_issuer - the references in the MSC to m.authentication are in the Alternatives section for this reason. Its quite entangled so not sure how to sanely split this out without the individual commits not being atomic.

richvdh · 2024-02-15T13:44:46Z

Its quite entangled so not sure how to sanely split this out without the individual commits not being atomic.

A few ideas to kick you off:

OidcClientConfig is moving to a new file. Pull out the move to its own commit.
There seems to be a formatting change to spec/unit/oidc/authorize.spec.ts ? Let's separate formatting cleanups from functional changes.
Make adding support for account_management_endpoint its own commit.
validateDiscoveryAuthenticationConfig isn't currently used anywhere afaict? Make ripping it out its own commit.

Those are just a few ideas I found quickly. I'm sure there are others; like I say, I'm finding it hard to follow at the moment so I'm not in the best place to advise. It doesn't matter if some of those commits end up being trivial; it all helps to reduce the amount of state a reviewer has to keep in their head at once.

richvdh · 2024-02-15T13:45:33Z

I realise a lot of this is marked @experimental, but surely there is something using it somewhere that is going to be broken? What's the plan for managing that?

richvdh

A few drive-by comments

src/client.ts

src/autodiscovery.ts

src/oidc/index.ts

richvdh · 2024-02-15T13:35:57Z

spec/unit/autodiscovery.spec.ts

@@ -897,75 +871,4 @@ describe("AutoDiscovery", function () {
            }),
        ]);
    });
-
-    describe("m.authentication", () => {


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?

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

src/oidc/discovery.ts

t3chguy · 2024-02-19T10:56:11Z

Have split out #4071 - will rebase this PR and split into sane commits afterwards

…m it Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

… response Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy · 2024-02-19T16:39:09Z

What's the plan for managing that?

matrix-org/matrix-react-sdk#12245

…chguy/fix/26908 # Conflicts: # spec/unit/oidc/authorize.spec.ts

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

…ported` from OIDC Issuer well-known Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

…x-js-sdk into t3chguy/fix/26908 # Conflicts: # src/oidc/validate.ts

richvdh

Seems generally sensible. A nit, and a question.

src/oidc/index.ts

src/oidc/register.ts

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

…fix/26908

richvdh

Was there an answer to my question at #4064 (comment)?

Also would like to see a mention of .well-known/openid-configuration somewhere in the doc-comments, per #4064 (comment).

LGTM otherwise

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy added the T-Enhancement label Feb 13, 2024

t3chguy mentioned this pull request Feb 13, 2024

Update MSC2965 OIDC Discovery implementation matrix-org/matrix-react-sdk#12245

Merged

github-actions bot deployed to PR Documentation Preview February 13, 2024 15:11 View deployment

github-actions bot deployed to PR Documentation Preview February 13, 2024 15:18 View deployment

github-actions bot deployed to PR Documentation Preview February 13, 2024 15:33 View deployment

t3chguy marked this pull request as ready for review February 13, 2024 16:15

t3chguy requested a review from a team as a code owner February 13, 2024 16:15

t3chguy requested review from dbkr and richvdh February 13, 2024 16:15

richvdh requested changes Feb 14, 2024

View reviewed changes

richvdh self-requested a review February 14, 2024 16:50

richvdh reviewed Feb 15, 2024

View reviewed changes

t3chguy mentioned this pull request Feb 16, 2024

Add getAuthIssuer method for MSC2965 #4071

Merged

t3chguy marked this pull request as draft February 16, 2024 17:51

t3chguy added 3 commits February 19, 2024 12:58

Remove m.authentication well-known handling as the MSC moved away fro…

96d003f

…m it Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

Read account management endpoint from the OIDC issuer well-known

bae525f

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

Change OIDC API methods shape to no longer rely on defunct well-known…

0b07c2c

… response Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy force-pushed the t3chguy/fix/26908 branch from 67e5e90 to 0b07c2c Compare February 19, 2024 14:18

github-actions bot deployed to PR Documentation Preview February 19, 2024 14:21 View deployment

Merge branch 'develop' of github.com:matrix-org/matrix-js-sdk into t3…

c1d104b

…chguy/fix/26908 # Conflicts: # spec/unit/oidc/authorize.spec.ts

github-actions bot deployed to PR Documentation Preview February 21, 2024 10:45 View deployment

Iterate

2f812c4

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

github-actions bot deployed to PR Documentation Preview February 21, 2024 11:38 View deployment

Add account_management_actions_supported

0518006

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

github-actions bot deployed to PR Documentation Preview February 21, 2024 12:05 View deployment

Simplify

a4b9572

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

github-actions bot deployed to PR Documentation Preview February 21, 2024 12:14 View deployment

Validate account_management_uri and `account_management_actions_sup…

9baef5e

…ported` from OIDC Issuer well-known Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy mentioned this pull request Feb 21, 2024

Validate account_management_uri and account_management_actions_supported from OIDC Issuer well-known #4074

Merged

t3chguy changed the base branch from develop to t3chguy/oidc-extensions February 21, 2024 13:30

Merge branch 't3chguy/oidc-extensions' of github.com:matrix-org/matri…

8ec4e9d

…x-js-sdk into t3chguy/fix/26908 # Conflicts: # src/oidc/validate.ts

github-actions bot deployed to PR Documentation Preview February 21, 2024 13:32 View deployment

Base automatically changed from t3chguy/oidc-extensions to develop February 21, 2024 15:17

Merge branch 'develop' into t3chguy/fix/26908

0a48f4a

github-actions bot deployed to PR Documentation Preview February 21, 2024 15:19 View deployment

t3chguy marked this pull request as ready for review February 21, 2024 15:20

t3chguy requested a review from richvdh February 21, 2024 15:20

richvdh reviewed Feb 21, 2024

View reviewed changes

src/oidc/index.ts Outdated Show resolved Hide resolved

src/oidc/register.ts Show resolved Hide resolved

t3chguy added 2 commits February 21, 2024 17:40

Fix comment

b91263b

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

Merge remote-tracking branch 'origin/t3chguy/fix/26908' into t3chguy/…

02bf791

…fix/26908

t3chguy requested a review from richvdh February 21, 2024 17:41

github-actions bot deployed to PR Documentation Preview February 21, 2024 17:42 View deployment

richvdh approved these changes Feb 22, 2024

View reviewed changes

Iterate comments

664108d

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

github-actions bot deployed to PR Documentation Preview February 22, 2024 09:36 View deployment

Iterate

5c6240b

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

github-actions bot deployed to PR Documentation Preview February 22, 2024 13:53 View deployment

Merge branch 'develop' into t3chguy/fix/26908

a37d204

github-actions bot deployed to PR Documentation Preview February 22, 2024 17:58 View deployment

t3chguy merged commit a26fc46 into develop Feb 23, 2024
23 checks passed

t3chguy deleted the t3chguy/fix/26908 branch February 23, 2024 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MSC2965 OIDC Discovery implementation #4064

Update MSC2965 OIDC Discovery implementation #4064

t3chguy commented Feb 13, 2024 •

edited

Loading

richvdh left a comment

t3chguy commented Feb 14, 2024

richvdh commented Feb 15, 2024 •

edited

Loading

richvdh commented Feb 15, 2024

richvdh left a comment

richvdh Feb 15, 2024

t3chguy Feb 22, 2024

t3chguy commented Feb 19, 2024

t3chguy commented Feb 19, 2024

richvdh left a comment

richvdh left a comment

Update MSC2965 OIDC Discovery implementation #4064

Update MSC2965 OIDC Discovery implementation #4064

Conversation

t3chguy commented Feb 13, 2024 • edited Loading

✨ Features

richvdh left a comment

Choose a reason for hiding this comment

t3chguy commented Feb 14, 2024

richvdh commented Feb 15, 2024 • edited Loading

richvdh commented Feb 15, 2024

richvdh left a comment

Choose a reason for hiding this comment

richvdh Feb 15, 2024

Choose a reason for hiding this comment

t3chguy Feb 22, 2024

Choose a reason for hiding this comment

t3chguy commented Feb 19, 2024

t3chguy commented Feb 19, 2024

richvdh left a comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

t3chguy commented Feb 13, 2024 •

edited

Loading

richvdh commented Feb 15, 2024 •

edited

Loading