diff --git a/src/djangooidc/exceptions.py b/src/djangooidc/exceptions.py index 226337f54..000c47649 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(AuthenticationFailed): + 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 bff766bb4..95ed322f5 100644 --- a/src/djangooidc/oidc.py +++ b/src/djangooidc/oidc.py @@ -182,10 +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("Received state not the same as expected for %s" % state) 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..f10afcbaf 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,21 +129,35 @@ 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 get_default_acr_value and the callback to raise StateMismatch + # error when called mock_client.get_default_acr_value.side_effect = self.create_acr - mock_client.callback.side_effect = NoStateDefined() - # TEST - # test the login callback + mock_client.callback.side_effect = StateMismatch() + # TEST receiving a response from login.gov response = self.client.get(reverse("openid_login_callback")) - # ASSERTIONS - # assert that the user is redirected to the start of the login process + # ASSERT self.assertEqual(response.status_code, 302) self.assertEqual(response.url, "/") + # Check that the redirect_attempted flag is set in the session + self.assertTrue(self.client.session.get("redirect_attempted", False)) + + def test_login_callback_with_no_session_state_attempt_again_only_once(self, mock_client): + """We only attempt to relogin once. After that, it's the error page for you.""" + with less_console_noise(): + # MOCK get_default_acr_value, redirect_attempted to True and the callback + # to raise StateMismatch error when called + mock_client.get_default_acr_value.side_effect = self.create_acr + mock_client.callback.side_effect = StateMismatch() + session = self.client.session + session["redirect_attempted"] = True + session.save() + # TEST receiving a response from login.gov + response = self.client.get(reverse("openid_login_callback")) + # ASSERT + self.assertEqual(response.status_code, 401) def test_login_callback_reads_next(self, mock_client): """If the next value is set in the session, test that login_callback returns diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 8e112769b..ab81ccff1 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -101,16 +101,25 @@ 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 state 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 flag if the exception is not caught + request.session.pop("redirect_attempted", None) + return error_page(request, nsd_err) except Exception as err: return error_page(request, err)