This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
UI Auth via SSO: redirect the user to an appropriate SSO. #9081
Merged
Merged
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
4f1477e
Collect identifiers for UIA session data together
richvdh 16a2af9
Rename user_id in validate_user_via_ui_auth
richvdh edaed4a
Store the requester's user id during UIA
richvdh 97c9147
UIAuth: only offer SSO for users with a mapping from a known IdP
richvdh 3e47573
UI Auth via SSO: redirect the user to an appropriate IdP.
richvdh 82808fb
changelog
richvdh 3dadd1b
Use the correct identifier for the session data
richvdh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1346,12 +1346,12 @@ def _do_validate_hash(checked_hash: bytes): | |
else: | ||
return False | ||
|
||
async def start_sso_ui_auth(self, redirect_url: str, session_id: str) -> str: | ||
async def start_sso_ui_auth(self, request: SynapseRequest, session_id: str) -> str: | ||
""" | ||
Get the HTML for the SSO redirect confirmation page. | ||
|
||
Args: | ||
redirect_url: The URL to redirect to the SSO provider. | ||
request: The incoming HTTP request | ||
session_id: The user interactive authentication session ID. | ||
|
||
Returns: | ||
|
@@ -1361,6 +1361,35 @@ async def start_sso_ui_auth(self, redirect_url: str, session_id: str) -> str: | |
session = await self.store.get_ui_auth_session(session_id) | ||
except StoreError: | ||
raise SynapseError(400, "Unknown session ID: %s" % (session_id,)) | ||
|
||
user_id_to_verify = await self.get_session_data( | ||
session_id, UIAuthSessionDataConstants.REGISTERED_USER_ID | ||
) # type: str | ||
|
||
idps = await self.hs.get_sso_handler().get_identity_providers_for_user( | ||
user_id_to_verify | ||
) | ||
|
||
if not idps: | ||
# we checked that the user had some remote identities before offering an SSO | ||
# flow, so either it's been deleted or the client has requested SSO despite | ||
# it not being offered. | ||
raise SynapseError(400, "User has no SSO identities") | ||
|
||
# for now, just pick one | ||
idp_id, sso_auth_provider = next(iter(idps.items())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This theoretically isn't stable before Python 3.7. I suspect we don't care since they can use any identity. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. indeed. the first is no better than the last. |
||
if len(idps) > 0: | ||
logger.warning( | ||
"User %r has previously logged in with multiple SSO IdPs; arbitrarily " | ||
"picking %r", | ||
user_id_to_verify, | ||
idp_id, | ||
) | ||
|
||
redirect_url = await sso_auth_provider.handle_redirect_request( | ||
request, None, session_id | ||
) | ||
|
||
return self._sso_auth_confirm_template.render( | ||
description=session.description, redirect_url=redirect_url, | ||
) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point in the flow should we be rendering JSON or HTML errors? (Should this use
sso_handler.render_error
?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that said... there are plenty of other places we are raising SynapseErrors in this method. Ideally they'd all be caught and sent to
sso_handler.render_error
instead, and while we're at it I thinkstart_sso_ui_auth
probably just wants to move intoSsoHandler
, but that feels like a bigger refactor.