Merge pull request #1916 from cisagov/rjm/1814-401

Issue #1814: Enhance state mismatch error handling and messaging

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):

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)

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

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.
+    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)