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

Jump to bottom

Update MSC2965 OIDC Discovery implementation #4064

Merged
merged 15 commits into from
Feb 23, 2024
Merged

Update MSC2965 OIDC Discovery implementation #4064

merged 15 commits into from
Feb 23, 2024

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Feb 13, 2024 •
edited
Loading

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

@t3chguy t3chguy mentioned this pull request Feb 13, 2024
Merged
@t3chguy t3chguy marked this pull request as ready for review February 13, 2024 16:15
@t3chguy t3chguy requested a review from a team as a code owner February 13, 2024 16:15
@t3chguy t3chguy requested review from dbkr and richvdh February 13, 2024 16:15
richvdh
richvdh requested changes Feb 14, 2024
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

t3chguy commented Feb 14, 2024

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 richvdh self-requested a review February 14, 2024 16:50
@richvdh
Copy link
Member

richvdh commented Feb 15, 2024 •
edited
Loading

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
Copy link
Member

richvdh commented Feb 15, 2024

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
richvdh reviewed Feb 15, 2024
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

A few drive-by comments

src/client.ts Outdated Show resolved Hide resolved
src/autodiscovery.ts Outdated Show resolved Hide resolved
src/oidc/index.ts Show resolved Hide resolved
spec/unit/autodiscovery.spec.ts
@@ -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

src/oidc/discovery.ts Show resolved Hide resolved
@t3chguy t3chguy mentioned this pull request Feb 16, 2024
Merged
@t3chguy t3chguy marked this pull request as draft February 16, 2024 17:51
@t3chguy
Copy link
Member Author

t3chguy commented Feb 19, 2024

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

@t3chguy
Remove m.authentication well-known handling as the MSC moved away fro…
96d003f 
…m it

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Read account management endpoint from the OIDC issuer well-known
bae525f 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Change OIDC API methods shape to no longer rely on defunct well-known…
0b07c2c 
… response

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Copy link
Member Author

t3chguy commented Feb 19, 2024

What's the plan for managing that?

matrix-org/matrix-react-sdk#12245

@t3chguy
Merge branch 'develop' of github.com:matrix-org/matrix-js-sdk into t3…
c1d104b 
…chguy/fix/26908

# Conflicts:
#	spec/unit/oidc/authorize.spec.ts
@t3chguy
Iterate
2f812c4 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Add account_management_actions_supported
0518006 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Simplify
a4b9572 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
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 t3chguy mentioned this pull request Feb 21, 2024
Merged
@t3chguy t3chguy changed the base branch from develop to t3chguy/oidc-extensions February 21, 2024 13:30
@t3chguy
Merge branch 't3chguy/oidc-extensions' of github.com:matrix-org/matri…
8ec4e9d 
…x-js-sdk into t3chguy/fix/26908

# Conflicts:
#	src/oidc/validate.ts
Base automatically changed from t3chguy/oidc-extensions to develop February 21, 2024 15:17
@t3chguy t3chguy marked this pull request as ready for review February 21, 2024 15:20
@t3chguy t3chguy requested a review from richvdh February 21, 2024 15:20
richvdh
richvdh reviewed Feb 21, 2024
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Seems generally sensible. A nit, and a question.

src/oidc/index.ts Outdated Show resolved Hide resolved
src/oidc/register.ts Show resolved Hide resolved
@t3chguy
Fix comment
b91263b 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Merge remote-tracking branch 'origin/t3chguy/fix/26908' into t3chguy/…
02bf791 
…fix/26908
@t3chguy t3chguy requested a review from richvdh February 21, 2024 17:41
richvdh
richvdh approved these changes Feb 22, 2024
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

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

@t3chguy
Iterate comments
664108d 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Iterate
5c6240b 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy t3chguy merged commit a26fc46 into develop Feb 23, 2024
23 checks passed
@t3chguy t3chguy deleted the t3chguy/fix/26908 branch February 23, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richvdh richvdh richvdh approved these changes

@dbkr dbkr Awaiting requested review from dbkr dbkr is a code owner automatically assigned from matrix-org/element-web-reviewers

Assignees
No one assigned
Labels
T-Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@t3chguy @richvdh