Merge pull request #441 from cfra/fix/clear-state-on-error

Remove state from session for failed authentication attempts

mozilla_django_oidc/views.py

@@ -60,6 +60,15 @@ def get(self, request):
 
         if request.GET.get('error'):
             # Ouch! Something important failed.
+
+            # Delete the state entry also for failed authentication attempts
+            # to prevent replay attacks.
+            if ('state' in request.GET
+                    and 'oidc_states' in request.session
+                    and request.GET['state'] in request.session['oidc_states']):
+                del request.session['oidc_states'][request.GET['state']]
+                request.session.save()
+
             # Make sure the user doesn't get to continue to be logged in
             # otherwise the refresh middleware will force the user to
             # redirect to authorize again if the session refresh has

tests/test_views.py

@@ -143,6 +143,40 @@ def test_get_auth_failure_inactive_user(self):
         self.assertEqual(response.status_code, 302)
         self.assertEqual(response.url, '/failure')
 
+    @override_settings(LOGIN_REDIRECT_URL_FAILURE='/failure')
+    def test_get_auth_error(self):
+        """Test authentication error handling.
+
+        Sttate should be removed from session and user should be logged out.
+        """
+        user = User.objects.create_user('example_username')
+
+        get_data = {
+            'error': 'example_code',
+            'state': 'example_state'
+        }
+        url = reverse('oidc_authentication_callback')
+        request = self.factory.get(url, get_data)
+        client = Client()
+        request.session = client.session
+        request.session['oidc_states'] = {
+            'example_state': {'nonce': None, 'added_on': time.time()},
+        }
+        request.user = user
+        callback_view = views.OIDCAuthenticationCallbackView.as_view()
+
+        with patch('mozilla_django_oidc.views.auth.logout') as mock_logout:
+            def clear_user(request):
+                # Assert state is cleared prior to logout
+                self.assertEqual(request.session['oidc_states'], {})
+                request.user = AnonymousUser()
+            mock_logout.side_effect = clear_user
+            response = callback_view(request)
+            mock_logout.assert_called_once()
+
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response.url, '/failure')
+
     @override_settings(OIDC_USE_NONCE=False)
     @override_settings(LOGIN_REDIRECT_URL_FAILURE='/failure')
     def test_get_auth_dirty_data(self):