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.

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

MSC2454: Support UI auth for SSO #2454

Merged
merged 8 commits into from May 5, 2020
Merged

Conversation

clokep
Copy link
Member

@clokep clokep commented Mar 9, 2020

@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 proposal-in-review labels Mar 9, 2020
@turt2live turt2live self-requested a review Mar 9, 2020
@@ -0,0 +1,266 @@
# User-Interactive Auth for SSO-backed homeserver
Copy link
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
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
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 Mar 17, 2020
Co-Authored-By: Hubert Chathi <hubert@uhoreg.ca>
proposals/2454-ui-interactive-auth-for-sso.md Outdated Show resolved Hide resolved
proposals/2454-ui-interactive-auth-for-sso.md Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

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
Member

@anoadragon453 anoadragon453 commented Apr 9, 2020

@mscbot fcp merge

@mscbot
Copy link
Collaborator

@mscbot mscbot commented Apr 9, 2020

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 labels Apr 9, 2020
@turt2live turt2live self-requested a review Apr 9, 2020
Copy link
Member

@uhoreg uhoreg left a comment

A few grammar nits/suggestions. I'm not an expert on UIAuth or SSO, but this seems largely sane.

proposals/2454-ui-interactive-auth-for-sso.md Outdated Show resolved Hide resolved
proposals/2454-ui-interactive-auth-for-sso.md Outdated Show resolved Hide resolved
proposals/2454-ui-interactive-auth-for-sso.md Outdated Show resolved Hide resolved
Co-Authored-By: Hubert Chathi <hubert@uhoreg.ca>
@turt2live turt2live added kind:feature and removed proposal-in-review labels Apr 20, 2020
@matrix-org matrix-org deleted a comment from mscbot Apr 27, 2020
@mscbot
Copy link
Collaborator

@mscbot mscbot commented Apr 30, 2020

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

@mscbot mscbot added final-comment-period and removed proposed-final-comment-period labels Apr 30, 2020
@mscbot
Copy link
Collaborator

@mscbot 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 labels May 5, 2020
@turt2live turt2live merged commit 4cd4e19 into master May 5, 2020
8 checks passed
@turt2live turt2live added spec-pr-missing 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
@turt2live turt2live added spec-pr-in-review merged and removed spec-pr-missing spec-pr-in-review labels May 8, 2020
@turt2live
Copy link
Member

@turt2live turt2live commented May 11, 2020

Spec PR was #2532

@turt2live
Copy link
Member

@turt2live turt2live commented May 11, 2020

Merged 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature merged proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants