diff --git a/src/djangooidc/exceptions.py b/src/djangooidc/exceptions.py index 226337f54..f7de9c816 100644 --- a/src/djangooidc/exceptions.py +++ b/src/djangooidc/exceptions.py @@ -33,8 +33,8 @@ class AuthenticationFailed(OIDCException): friendly_message = "This login attempt didn't work." -class NoStateDefined(OIDCException): - friendly_message = "The session state is None." +class StateMismatch(OIDCException): + friendly_message = "State mismatch. This login attempt didn't work." class InternalError(OIDCException): diff --git a/src/djangooidc/oidc.py b/src/djangooidc/oidc.py index fa3aac08e..5e6cc5d7f 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -182,14 +182,20 @@ class Client(oic.Client): if authn_response["state"] != session.get("state", None): # this most likely means the user's Django session vanished - logger.error( - f"Received state not the same as expected for {state}" - f"authn_response['state'] = {authn_response['state']}" - f"session.get('state', None) = {session.get('state', None)}" - ) if session.get("state", None) is None: - raise o_e.NoStateDefined() - raise o_e.AuthenticationFailed(locator=state) + logger.error( + f"The OP state {state} does not match the session state." + f"The session state is None." + f"authn_response['state'] = {authn_response['state']}" + f"session.get('state', None) = {session.get('state', None)}" + ) + else: + logger.error( + f"The OP state {state} does not match the session state." + f"authn_response['state'] = {authn_response['state']}" + f"session.get('state', None) = {session.get('state', None)}" + ) + raise o_e.StateMismatch() if self.behaviour.get("response_type") == "code": # need an access token to get user info (and to log the user out later) diff --git a/src/djangooidc/tests/test_views.py b/src/djangooidc/tests/test_views.py index 0f734b80d..fe02c3ea8 100644 --- a/src/djangooidc/tests/test_views.py +++ b/src/djangooidc/tests/test_views.py @@ -4,7 +4,7 @@ from django.http import HttpResponse from django.test import Client, TestCase, RequestFactory from django.urls import reverse -from djangooidc.exceptions import NoStateDefined, InternalError +from djangooidc.exceptions import StateMismatch, InternalError from ..views import login_callback from .common import less_console_noise @@ -129,14 +129,14 @@ class ViewsTest(TestCase): self.assertContains(response, "Hi") def test_login_callback_with_no_session_state(self, mock_client): - """If the local session is None (ie the server restarted while user was logged out), + """If the local session does not match the OP session, we do not throw an exception. Rather, we attempt to login again.""" with less_console_noise(): # MOCK # mock the acr_value to some string - # mock the callback function to raise the NoStateDefined Exception + # mock the callback function to raise the StateMismatch Exception mock_client.get_default_acr_value.side_effect = self.create_acr - mock_client.callback.side_effect = NoStateDefined() + mock_client.callback.side_effect = StateMismatch() # TEST # test the login callback response = self.client.get(reverse("openid_login_callback")) diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 2d3c842d2..0a2a07d05 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -108,16 +108,24 @@ def login_callback(request): if user: login(request, user) logger.info("Successfully logged in user %s" % user) - # Double login bug (1507)? + # Clear the flag if the exception is not caught + request.session.pop("redirect_attempted", None) return redirect(request.session.get("next", "/")) else: raise o_e.BannedUser() - except o_e.NoStateDefined as nsd_err: - # In the event that a user is in the middle of a login when the app is restarted, - # their session state will no longer be available, so redirect the user to the - # beginning of login process without raising an error to the user. - logger.warning(f"No State Defined: {nsd_err}") - return redirect(request.session.get("next", "/")) + except o_e.StateMismatch as nsd_err: + # Check if the redirect has already been attempted + if not request.session.get("redirect_attempted", False): + # Set the flag to indicate that the redirect has been attempted + request.session["redirect_attempted"] = True + + # In the event of a sstate mismatch between OP and session, redirect the user to the + # beginning of login process without raising an error to the user. Attempt once. + logger.warning(f"No State Defined: {nsd_err}") + return redirect(request.session.get("next", "/")) + else: + # Clear the redirect_attempted flag? + return error_page(request, nsd_err) except Exception as err: return error_page(request, err)