From 59bd7af7d6c2b1126e28ea40c5a7618bbbe754ab Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Wed, 9 Oct 2024 11:11:17 -0400 Subject: [PATCH] fix: OP prompts for logout when no OP session (#1517) The OAuth provider is prompting users who no longer have an user session with the OAuth Provider to logout of the OP. This happens in scenarios given the user has logged out of the OP directly or via another client. In cases where the user does not have a session on the OP we should not prompt them to log out of the OP as there is no session, but we should still clear out their tokens to terminate the session for the Application. --- CHANGELOG.md | 6 +++- oauth2_provider/views/oidc.py | 67 ++++++++++++++++++++++++++++++----- tests/test_oidc_views.py | 35 ++++++++++++++---- 3 files changed, 91 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38b1d8b78..f16317265 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,12 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [unreleased] ### Added -* Support for Wildcard Origin and Redirect URIs, https://github.com/jazzband/django-oauth-toolkit/issues/1506 +* #1506 Support for Wildcard Origin and Redirect URIs ### Fixed +* #1517 OP prompts for logout when no OP session +* #1512 client_secret not marked sensitive + diff --git a/oauth2_provider/views/oidc.py b/oauth2_provider/views/oidc.py index c746c30ce..a252f1be4 100644 --- a/oauth2_provider/views/oidc.py +++ b/oauth2_provider/views/oidc.py @@ -367,17 +367,66 @@ def validate_logout_request(self, id_token_hint, client_id, post_logout_redirect return application, id_token.user if id_token else None def must_prompt(self, token_user): - """Indicate whether the logout has to be confirmed by the user. This happens if the - specifications force a confirmation, or it is enabled by `OIDC_RP_INITIATED_LOGOUT_ALWAYS_PROMPT`. + """ + per: https://openid.net/specs/openid-connect-rpinitiated-1_0.html + + > At the Logout Endpoint, the OP SHOULD ask the End-User whether to log + > out of the OP as well. Furthermore, the OP MUST ask the End-User this + > question if an id_token_hint was not provided or if the supplied ID + > Token does not belong to the current OP session with the RP and/or + > currently logged in End-User. - A logout without user interaction (i.e. no prompt) is only allowed - if an ID Token is provided that matches the current user. """ - return ( - oauth2_settings.OIDC_RP_INITIATED_LOGOUT_ALWAYS_PROMPT - or token_user is None - or token_user != self.request.user - ) + + if not self.request.user.is_authenticated: + """ + > the OP MUST ask ask the End-User whether to log out of the OP as + + If the user does not have an active session with the OP, they cannot + end their OP session, so there is nothing to prompt for. This occurs + in cases where the user has logged out of the OP via another channel + such as the OP's own logout page, session timeout or another RP's + logout page. + """ + return False + + if oauth2_settings.OIDC_RP_INITIATED_LOGOUT_ALWAYS_PROMPT: + """ + > At the Logout Endpoint, the OP SHOULD ask the End-User whether to + > log out of the OP as well + + The admin has configured the OP to always prompt the userfor logout + per the SHOULD recommendation. + """ + return True + + if token_user is None: + """ + > the OP MUST ask ask the End-User whether to log out of the OP as + > well if the supplied ID Token does not belong to the current OP + > session with the RP. + + token_user will only be populated if an ID token was found for the + RP (Application) that is requesting the logout. If token_user is not + then we must prompt the user. + """ + return True + + if token_user != self.request.user: + """ + > the OP MUST ask ask the End-User whether to log out of the OP as + > well if the supplied ID Token does not belong to the logged in + > End-User. + + is_authenticated indicates that there is a logged in user and was + tested in the first condition. + token_user != self.request.user indicates that the token does not + belong to the logged in user, Therefore we need to prompt the user. + """ + return True + + """ We didn't find a reason to prompt the user """ + return False def do_logout(self, application=None, post_logout_redirect_uri=None, state=None, token_user=None): user = token_user or self.request.user diff --git a/tests/test_oidc_views.py b/tests/test_oidc_views.py index 8bdf18360..65197cbd1 100644 --- a/tests/test_oidc_views.py +++ b/tests/test_oidc_views.py @@ -311,6 +311,10 @@ def test_must_prompt(oidc_tokens, other_user, rp_settings, ALWAYS_PROMPT): == ALWAYS_PROMPT ) assert RPInitiatedLogoutView(request=mock_request_for(other_user)).must_prompt(oidc_tokens.user) is True + assert ( + RPInitiatedLogoutView(request=mock_request_for(AnonymousUser())).must_prompt(oidc_tokens.user) + is False + ) def test__load_id_token(): @@ -577,13 +581,14 @@ def test_token_deletion_on_logout(oidc_tokens, logged_in_client, rp_settings): @pytest.mark.django_db(databases=retrieve_current_databases()) -def test_token_deletion_on_logout_expired_session(oidc_tokens, client, rp_settings): +def test_token_deletion_on_logout_without_op_session_get(oidc_tokens, client, rp_settings): AccessToken = get_access_token_model() IDToken = get_id_token_model() RefreshToken = get_refresh_token_model() assert AccessToken.objects.count() == 1 assert IDToken.objects.count() == 1 assert RefreshToken.objects.count() == 1 + rsp = client.get( reverse("oauth2_provider:rp-initiated-logout"), data={ @@ -591,15 +596,31 @@ def test_token_deletion_on_logout_expired_session(oidc_tokens, client, rp_settin "client_id": oidc_tokens.application.client_id, }, ) - assert rsp.status_code == 200 + assert rsp.status_code == 302 assert not is_logged_in(client) # Check that all tokens are active. - access_token = AccessToken.objects.get() - assert not access_token.is_expired() - id_token = IDToken.objects.get() - assert not id_token.is_expired() + assert AccessToken.objects.count() == 0 + assert IDToken.objects.count() == 0 + assert RefreshToken.objects.count() == 1 + + with pytest.raises(AccessToken.DoesNotExist): + AccessToken.objects.get() + + with pytest.raises(IDToken.DoesNotExist): + IDToken.objects.get() + refresh_token = RefreshToken.objects.get() - assert refresh_token.revoked is None + assert refresh_token.revoked is not None + + +@pytest.mark.django_db(databases=retrieve_current_databases()) +def test_token_deletion_on_logout_without_op_session_post(oidc_tokens, client, rp_settings): + AccessToken = get_access_token_model() + IDToken = get_id_token_model() + RefreshToken = get_refresh_token_model() + assert AccessToken.objects.count() == 1 + assert IDToken.objects.count() == 1 + assert RefreshToken.objects.count() == 1 rsp = client.post( reverse("oauth2_provider:rp-initiated-logout"),