From 5d799722607c09240362b85c4d73f0867e3608f7 Mon Sep 17 00:00:00 2001
From: Robert Raposa <rraposa@edx.org>
Date: Fri, 7 Jan 2022 19:19:59 -0500
Subject: [PATCH] fix: safe session bug when request has no user (#29731)

* fix: safe session bug when request has no user

Fixes a bug during safe session monitoring when
request has no user.

ARCHBOM-1940

* fixup! add comment and loosen if condition
---
 .../djangoapps/safe_sessions/middleware.py    | 26 +++++++++++--------
 .../safe_sessions/tests/test_middleware.py    | 22 ++++++++++++++--
 2 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py
index 6d600b5713..b1d8d768a8 100644
--- a/openedx/core/djangoapps/safe_sessions/middleware.py
+++ b/openedx/core/djangoapps/safe_sessions/middleware.py
@@ -501,24 +501,28 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin):
 
             try:
 
-                if LOG_REQUEST_USER_CHANGE_HEADERS.is_enabled() and request.user.id is not None:
+                if LOG_REQUEST_USER_CHANGE_HEADERS.is_enabled():
 
                     # add a session hash custom attribute for all requests to help monitoring
-                    #   requests that come both before and after a mistmatch
+                    #   requests that come both before and after a mismatch
                     if hasattr(request, 'cookie_session_field'):
                         session_hash = obscure_token(request.cookie_session_field)
                         set_custom_attribute('safe_sessions.session_id_hash.parsed_cookie', session_hash)
 
-                    # log request header if this user id was involved in an earlier mismatch
-                    log_request_headers = cache.get(
-                        SafeSessionMiddleware._get_recent_user_change_cache_key(request.user.id), False
-                    )
-                    if log_request_headers:
-                        log.info(
-                            f'SafeCookieData request header for {request.user.id}: '
-                            f'{SafeSessionMiddleware._get_encrypted_request_headers(request)}'
+                    # In the off chance that either userid_in_session or request.user.id could
+                    #   be None while the other contains the actual user id, we'll use either.
+                    user_id = userid_in_session or hasattr(request, 'user') and request.user.id
+                    if user_id:
+                        # log request header if this user id was involved in an earlier mismatch
+                        log_request_headers = cache.get(
+                            SafeSessionMiddleware._get_recent_user_change_cache_key(user_id), False
                         )
-                        set_custom_attribute('safe_sessions.headers_logged', True)
+                        if log_request_headers:
+                            log.info(
+                                f'SafeCookieData request header for {user_id}: '
+                                f'{SafeSessionMiddleware._get_encrypted_request_headers(request)}'
+                            )
+                            set_custom_attribute('safe_sessions.headers_logged', True)
 
             except BaseException as e:
                 log.exception("SafeCookieData error while logging request headers.")
diff --git a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py
index 2b1657cb57..7fe109333c 100644
--- a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py
+++ b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py
@@ -2,7 +2,7 @@
 Unit tests for SafeSessionMiddleware
 """
 
-from unittest.mock import call, patch
+from unittest.mock import call, patch, MagicMock
 
 import ddt
 from crum import set_current_request
@@ -431,7 +431,7 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase
     @override_settings(LOG_REQUEST_USER_CHANGE_HEADERS=True)
     @patch("openedx.core.djangoapps.safe_sessions.middleware.LOG_REQUEST_USER_CHANGES", True)
     @patch("openedx.core.djangoapps.safe_sessions.middleware.cache")
-    def test_with_header_logging(self, mock_cache):
+    def test_user_change_with_header_logging(self, mock_cache):
         self.set_up_for_success()
         self.request.user = UserFactory.create()
         with self.assert_logged('SafeCookieData: Changing request user. ', log_level='warning'):
@@ -453,6 +453,24 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase
         #   simply verify that we are checking the cache.
         mock_cache.get.assert_called_with('safe_sessions.middleware.recent_user_change_detected_1', False)
 
+    @override_settings(LOG_REQUEST_USER_CHANGE_HEADERS=True)
+    @patch("openedx.core.djangoapps.safe_sessions.middleware.LOG_REQUEST_USER_CHANGES", True)
+    def test_no_request_user_with_header_logging(self):
+        """
+        Test header logging enabled with request not containing a user object.
+
+        Notes:
+        * In Production, failures happened for some requests that did not have
+            request.user set, so we test this case here.
+        * An attempt at creating a unit test for an anonymous user started
+            failing due to a missing request.session, which never happens in
+            Production, so this case assumes a working session object.
+        """
+        self.request.session = MagicMock()
+        del self.request.user
+        with self.assert_not_logged():
+            SafeSessionMiddleware().process_response(self.request, self.client.response)
+
     def test_no_warn_on_expected_user_change(self):
         """
         Verifies that no warnings is emitted when the user change is expected.