-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add soft logout possibility for OIDC backchannel_logout #15976
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Allow setting OIDC backchannel logout to be a soft-logout in OIDC provider configuration via `backchannel_logout_is_soft` which defaults to false. Contributed by @hachem2001. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1232,6 +1232,91 @@ async def revoke_sessions_for_provider_session_id( | |
) | ||
await self._device_handler.delete_devices(user_id, [device_id]) | ||
|
||
async def invalidate_sessions_for_provider_session_id( | ||
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 feels like it should be instead a parameter on the 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. I agree, though I avoided that in case this redundancy improves visibility, and in case we don't want to change the behavior of one of the functions in the long-run. But merging it with 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. I suspect it would improve readability if there's less duplicated code, so we can see what the differences are based on the flag. 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. @hachem2001 Do you have interest in making this change? |
||
self, | ||
auth_provider_id: str, | ||
auth_provider_session_id: str, | ||
expected_user_id: Optional[str] = None, | ||
) -> None: | ||
"""Invalidates all access tokens and in-flight login tokens tied to a provider | ||
session. | ||
This causes them to be soft-logged out. | ||
|
||
Can only be called from the main process. | ||
|
||
Args: | ||
auth_provider_id: A unique identifier for this SSO provider, e.g. | ||
"oidc" or "saml". | ||
auth_provider_session_id: The session ID from the provider to logout | ||
expected_user_id: The user we're expecting to logout. If set, it will ignore | ||
sessions belonging to other users and log an error. | ||
""" | ||
|
||
# It is expected that this is the main process. | ||
assert isinstance( | ||
self._device_handler, DeviceHandler | ||
), "invalidating SSO sessions can only be called on the main process" | ||
|
||
# Invalidate any running user-mapping sessions | ||
to_delete = [] | ||
for session_id, session in self._username_mapping_sessions.items(): | ||
if ( | ||
session.auth_provider_id == auth_provider_id | ||
and session.auth_provider_session_id == auth_provider_session_id | ||
): | ||
to_delete.append(session_id) | ||
|
||
for session_id in to_delete: | ||
logger.info("Revoking mapping session %s", session_id) | ||
del self._username_mapping_sessions[session_id] | ||
|
||
# Invalidate any in-flight login tokens | ||
await self._store.invalidate_login_tokens_by_session_id( | ||
auth_provider_id=auth_provider_id, | ||
auth_provider_session_id=auth_provider_session_id, | ||
) | ||
|
||
# Fetch any device(s) in the store associated with the session ID. | ||
devices = await self._store.get_devices_by_auth_provider_session_id( | ||
auth_provider_id=auth_provider_id, | ||
auth_provider_session_id=auth_provider_session_id, | ||
) | ||
|
||
# Iterate over the list of devices and soft-log them out one by one. | ||
for device in devices: | ||
user_id = device["user_id"] | ||
device_id = device["device_id"] | ||
|
||
# If the user_id associated with that device/session is not the one we got | ||
# out of the `sub` claim, skip that device and show log an error. | ||
if expected_user_id is not None and user_id != expected_user_id: | ||
logger.error( | ||
"Received a (soft) logout notification from SSO provider " | ||
f"{auth_provider_id!r} for the user {expected_user_id!r}, but with " | ||
f"a session ID ({auth_provider_session_id!r}) which belongs to " | ||
f"{user_id!r}. This may happen when the SSO provider user mapper " | ||
"uses something else than the standard attribute as mapping ID. " | ||
"For OIDC providers, set `backchannel_logout_ignore_sub` to `true` " | ||
"in the provider config if that is the case." | ||
) | ||
continue | ||
|
||
logger.info( | ||
"Soft-logging out %r (device %r) via SSO (%r) soft-logout notification (session %r).", | ||
user_id, | ||
device_id, | ||
auth_provider_id, | ||
auth_provider_session_id, | ||
) | ||
|
||
# Invalidate all tokens of user_id associated with device_id | ||
await self._store.user_set_account_tokens_validity( | ||
user_id, | ||
validity_until_ms=self._clock.time_msec(), | ||
except_token_id=None, | ||
device_id=device_id, | ||
) | ||
|
||
|
||
def get_username_mapping_session_cookie_from_request(request: IRequest) -> str: | ||
"""Extract the session ID from the cookie | ||
|
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.
@sandhose Do you have any thoughts on if it makes sense for this to be configurable or not? (Previous conversation at #11414 (comment))
The backchannel-logout is used when a user manually logs out of an IdP? Or is it also used when a user's session expires with an IdP?
The former seems like it would make sense with the current (hard-)logout, while the latter should likely be a soft-logout.
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.
I think keeping the option to do both is good. Usually, a backchannel logout happens because the user expressed the intent of logging out, but it might also happen when for example an admin kicks out a user to force logging back in, and in this case you might want to do a soft_logout in clients.
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.
It's indeed for the case where the user doesn't launch the backchannel logout on the server directly. Backchannel logout on synapse can occur without the user intentionally provoking it in some cases.
I made this as an option for safety upon upgrade, not to break any OIDC behavior servers currently rely on. Although I don't envision cases where a server would prefer a hard-logout for users, it's better to have the choice I suppose.
Another alternative would be to set
backchannel_logout_is_soft
to true by default, and in a further version eventually rule out the option and make it default behavior (basically an ephemeral temporary option). This is the best compromise in my opinion.