Skip to content

MSC2454: Support UI auth for SSO#2454

Merged
turt2live merged 8 commits intomasterfrom
clokep/ui-interactive-auth-for-sso
May 5, 2020
Merged

MSC2454: Support UI auth for SSO#2454
turt2live merged 8 commits intomasterfrom
clokep/ui-interactive-auth-for-sso

Conversation

@clokep
Copy link
Copy Markdown
Member

@clokep clokep commented Mar 9, 2020 •
edited by richvdh
Loading

@clokep clokep mentioned this pull request Mar 9, 2020
Closed
@turt2live turt2live changed the title Support UI interactive auth for SSO MSC2454: Support UI interactive auth for SSO Mar 9, 2020
@turt2live turt2live added proposal A matrix spec change proposal proposal-in-review labels Mar 9, 2020
@turt2live turt2live self-requested a review March 9, 2020 19:21
ara4n
ara4n reviewed Mar 9, 2020
View reviewed changes
Comment thread proposals/2454-ui-interactive-auth-for-sso.md Outdated
@@ -0,0 +1,266 @@
# User-Interactive Auth for SSO-backed homeserver
Copy link
Copy Markdown
Member

@ara4n ara4n Mar 9, 2020

Choose a reason for hiding this comment

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

This looks cool and very comprehensive, but i’m a bit confused on what bits are actually proposed changes to the spec rather than implementation notes?

I think the bit which is confusing me is:

In theory, any clients that already implement the fallback process for unknown authentication types will work fine without modification.

If clients don’t need changes, why is a spec change needed?

Copy link
Copy Markdown
Member Author

@clokep clokep Mar 10, 2020

Choose a reason for hiding this comment

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

I'll work to clarify the proposal section of this, but the short version is to add an m.login.sso type to the list of possible ui-auth types: https://matrix.org/docs/spec/client_server/r0.6.0#authentication-types.

The quoted sentence is meant to mean "if the client implements the fallback process already for other workflows it will probably work fine here too". I moved that around from Rich's original proposal and likely made it more confusing, sorry about that!

Copy link
Copy Markdown
Member Author

@clokep clokep Mar 10, 2020

Choose a reason for hiding this comment

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

I removed a couple pieces and tried to clarify the value and changes that this is proposing.

@richvdh richvdh self-requested a review March 17, 2020 10:55
uhoreg
uhoreg reviewed Mar 26, 2020
View reviewed changes
Comment thread proposals/2454-ui-interactive-auth-for-sso.md Outdated
@clokep @uhoreg
Markdown formatting.
eb48863 
Co-Authored-By: Hubert Chathi <[email protected]>
turt2live
turt2live requested changes Mar 27, 2020
View reviewed changes
Comment thread proposals/2454-ui-interactive-auth-for-sso.md
Comment thread proposals/2454-ui-interactive-auth-for-sso.md
This was referenced Mar 27, 2020
Closed
Merged
richvdh
richvdh reviewed Mar 30, 2020
View reviewed changes
Copy link
Copy Markdown
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.

I think I wrote most of this, so obviously it lgtm :).

I'd echo @turt2live's comments that we should emphasise that the only new thing here is the first paragraph of the proposal (a new authentication type). The rest is just a demonstration of how it works.

@anoadragon453
Copy link
Copy Markdown
Member

anoadragon453 commented Apr 9, 2020

@mscbot fcp merge

@mscbot
Copy link
Copy Markdown
Collaborator

mscbot commented Apr 9, 2020 •
edited by erikjohnston
Loading

Team member @anoadragon453 has proposed to merge this. The next step is review by the rest of the tagged people:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Apr 9, 2020
@turt2live turt2live self-requested a review April 9, 2020 15:46
uhoreg
uhoreg reviewed Apr 14, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@uhoreg uhoreg 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 grammar nits/suggestions. I'm not an expert on UIAuth or SSO, but this seems largely sane.

Comment thread proposals/2454-ui-interactive-auth-for-sso.md Outdated
Comment thread proposals/2454-ui-interactive-auth-for-sso.md Outdated
Comment thread proposals/2454-ui-interactive-auth-for-sso.md Outdated
@clokep @uhoreg
Fix minor grammatical fixes.
f6879c8 
Co-Authored-By: Hubert Chathi <[email protected]>
@turt2live turt2live added kind:feature MSC for not-core and not-maintenance stuff and removed proposal-in-review labels Apr 20, 2020
@matrix-org matrix-org deleted a comment from mscbot Apr 27, 2020
@mscbot
Copy link
Copy Markdown
Collaborator

mscbot commented Apr 30, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Apr 30, 2020
@mscbot
Copy link
Copy Markdown
Collaborator

mscbot commented May 5, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels May 5, 2020
@turt2live turt2live merged commit 4cd4e19 into master May 5, 2020
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels May 5, 2020
@uhoreg uhoreg changed the title MSC2454: Support UI interactive auth for SSO MSC2454: Support UI auth for SSO May 7, 2020
@clokep clokep mentioned this pull request May 8, 2020
Merged
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review merged A proposal whose PR has merged into the spec! and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels May 8, 2020
@turt2live
Copy link
Copy Markdown
Member

turt2live commented May 11, 2020

Spec PR was #2532

@turt2live
Copy link
Copy Markdown
Member

turt2live commented May 11, 2020

Merged 🎉

@clokep clokep mentioned this pull request May 19, 2020
Closed
@jplatte jplatte mentioned this pull request Jun 5, 2020
Closed
19 tasks
@clokep clokep deleted the clokep/ui-interactive-auth-for-sso branch May 17, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uhoreg uhoreg uhoreg left review comments

@ara4n ara4n ara4n left review comments

@richvdh richvdh richvdh left review comments

@turt2live turt2live turt2live approved these changes

Assignees

No one assigned

Labels

kind:feature MSC for not-core and not-maintenance stuff merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@clokep @anoadragon453 @mscbot @turt2live @uhoreg @ara4n @richvdh