feat: Don't warn about expected user changes in safe-sessions

lms/djangoapps/courseware/tests/tests.py

@@ -38,14 +38,6 @@ def test_logout(self):
         """
         self.logout()
 
-    def test_request_attr_on_logout(self):
-        """
-        Test request object after logging out to see whether it
-        has 'is_from_log_out' attribute set to true.
-        """
-        response = self.client.get(reverse('logout'))
-        assert getattr(response.wsgi_request, 'is_from_logout', False)
-
 
 class PageLoaderTestCase(LoginEnrollmentTestCase):
     """

openedx/core/djangoapps/safe_sessions/middleware.py

@@ -3,6 +3,17 @@
 SafeCookieData that cryptographically binds the user to the session id
 in the cookie.
 
+The primary goal is to avoid and detect situations where a session is
+corrupted and the client becomes logged in as the wrong user. This
+could happen via cache corruption (which we've seen before) or a
+request handling bug. It's unlikely to happen again, but would be a
+critical issue, so we've built in some checks to make sure the user on
+the session doesn't change over the course of the session or between
+the request and response phases.
+
+The secondary goal is to improve upon Django's session handling by
+including cryptographically enforced expiration.
+
 The implementation is inspired in part by the proposal in the paper
 <http://www.cse.msu.edu/~alexliu/publications/Cookie/cookie.pdf>
 but deviates in a number of ways; mostly it just uses the technique
@@ -66,9 +77,8 @@
 
 import inspect
 from base64 import b64encode
-from contextlib import contextmanager
 from hashlib import sha256
-from logging import ERROR, getLogger
+from logging import getLogger
 
 from django.conf import settings
 from django.contrib.auth import SESSION_KEY
@@ -278,7 +288,8 @@ def process_request(self, request):
 
         Step 4. Once the session is retrieved, verify that the user
         bound in the safe_cookie_data matches the user attached to the
-        server's session information.
+        server's session information. Otherwise, reject the request
+        (bypass the view and return an error or redirect).
 
         Step 5. If all is successful, the now verified user_id is stored
         separately in the request object so it is available for another
@@ -313,6 +324,8 @@ def process_request(self, request):
                 if LOG_REQUEST_USER_CHANGES:
                     log_request_user_changes(request)
             else:
+                # Return an error or redirect, and don't continue to
+                # the underlying view.
                 return self._on_user_authentication_failed(request)
 
     def process_response(self, request, response):
@@ -345,13 +358,12 @@ def process_response(self, request, response):
         if not _is_cookie_marked_for_deletion(request) and _is_cookie_present(response):
             try:
                 user_id_in_session = self.get_user_id_from_session(request)
-                with controlled_logging(request, log):
-                    self._verify_user(request, user_id_in_session)  # Step 2
+                self._verify_user(request, response, user_id_in_session)  # Step 2
 
-                    # Use the user_id marked in the session instead of the
-                    # one in the request in case the user is not set in the
-                    # request, for example during Anonymous API access.
-                    self.update_with_safe_session_cookie(response.cookies, user_id_in_session)  # Step 3
+                # Use the user_id marked in the session instead of the
+                # one in the request in case the user is not set in the
+                # request, for example during Anonymous API access.
+                self.update_with_safe_session_cookie(response.cookies, user_id_in_session)  # Step 3
 
             except SafeCookieError:
                 _mark_cookie_for_deletion(request)
@@ -378,12 +390,23 @@ def _on_user_authentication_failed(request):
         return redirect_to_login(request.path)
 
     @staticmethod
-    def _verify_user(request, userid_in_session):
+    def _verify_user(request, response, userid_in_session):
         """
         Logs an error if the user marked at the time of process_request
         does not match either the current user in the request or the
         given userid_in_session.
         """
+        # It's expected that a small number of views may change the
+        # user over the course of the request. We have exemptions for
+        # the user changing to/from None, but the login view can
+        # sometimes change the user from one value to another between
+        # the request and response phases, specifically when the login
+        # page is used during an active session.
+        #
+        # The relevant views set a flag to indicate the exemption.
+        if getattr(response, 'safe_sessions_expected_user_change', None):
+            return
+
         if hasattr(request, 'safe_cookie_verified_user_id'):
             if hasattr(request.user, 'real_user'):
                 # If a view overrode the request.user with a masqueraded user, this will
@@ -431,6 +454,7 @@ def get_user_id_from_session(request):
         except KeyError:
             return None
 
+    # TODO move to test code, maybe rename, get rid of old Django compat stuff
     @staticmethod
     def set_user_id_in_session(request, user):
         """
@@ -573,28 +597,13 @@ def __setattr__(self, name, value):
     request.__class__ = SafeSessionRequestWrapper
 
 
-def _is_from_logout(request):
+def mark_user_change_as_expected(response, new_user_id):
     """
-    Returns whether the request has come from logout action to see if
-    'is_from_logout' attribute is present.
-    """
-    return getattr(request, 'is_from_logout', False)
-
+    Indicate to the safe-sessions middleware that it is expected that
+    the user is changing between the request and response phase of
+    the current request.
 
-@contextmanager
-def controlled_logging(request, logger):
-    """
-    Control the logging by changing logger's level if
-    the request is from logout.
+    The new_user_id may be None or an LMS user ID, and may be the same
+    as the previous user ID.
     """
-    default_level = None
-    from_logout = _is_from_logout(request)
-    if from_logout:
-        default_level = logger.getEffectiveLevel()
-        logger.setLevel(ERROR)
-
-    try:
-        yield
-    finally:
-        if from_logout:
-            logger.setLevel(default_level)
+    response.safe_sessions_expected_user_change = {'new_user_id': new_user_id}

openedx/core/djangoapps/safe_sessions/tests/test_middleware.py

@@ -17,7 +17,7 @@
 from openedx.core.djangolib.testing.utils import get_mock_request
 from common.djangoapps.student.tests.factories import UserFactory
 
-from ..middleware import SafeCookieData, SafeSessionMiddleware, log_request_user_changes
+from ..middleware import SafeCookieData, SafeSessionMiddleware, log_request_user_changes, mark_user_change_as_expected
 from .test_utils import TestSafeSessionsLogMixin
 
 
@@ -263,9 +263,9 @@ def cookies_from_request_to_response(self):
                 settings.SESSION_COOKIE_NAME
             ]
 
-    def verify_success(self):
+    def set_up_for_success(self):
         """
-        Verifies success path.
+        Set up request for success path -- everything up until process_response().
         """
         self.client.login(username=self.user.username, password='test')
         self.request.user = self.user
@@ -281,6 +281,12 @@ def verify_success(self):
         assert self.request.safe_cookie_verified_user_id == self.user.id
         self.cookies_from_request_to_response()
 
+    def verify_success(self):
+        """
+        Verifies success path.
+        """
+        self.set_up_for_success()
+
         with self.assert_not_logged():
             response = SafeSessionMiddleware().process_response(self.request, self.client.response)
         assert response.status_code == 200
@@ -322,6 +328,41 @@ def test_error_from_mobile_app(self):
         self.request.META = {'HTTP_USER_AGENT': 'open edX Mobile App Version 2.1'}
         self.verify_error(401)
 
+    def test_warn_on_user_change(self):
+        """
+        Verifies that warnings are emitted and custom attributes set if
+        the user changes unexpectedly between request and response.
+        """
+        self.set_up_for_success()
+
+        # But then user changes unexpectedly
+        self.request.user = UserFactory.create()
+
+        with self.assert_logged_for_request_user_mismatch(self.user.id, self.request.user.id, 'warning', '/'):
+            with patch('openedx.core.djangoapps.safe_sessions.middleware.set_custom_attribute') as mock_attr:
+                response = SafeSessionMiddleware().process_response(self.request, self.client.response)
+        assert response.status_code == 200
+        mock_attr.assert_called_with("safe_sessions.user_mismatch", "request-response-mismatch")
+
+    def test_no_warn_on_expected_user_change(self):
+        """
+        Verifies that no warnings is emitted when the user change is expected.
+        This might happen on a login, for example.
+        """
+        self.set_up_for_success()
+
+        # User changes...
+        new_user = UserFactory.create()
+        self.request.user = new_user
+        # ...but so does session, and view sets a flag to say it's OK.
+        mark_user_change_as_expected(self.client.response, new_user.id)
+
+        with self.assert_no_warning_logged():
+            with patch('openedx.core.djangoapps.safe_sessions.middleware.set_custom_attribute') as mock_attr:
+                response = SafeSessionMiddleware().process_response(self.request, self.client.response)
+        assert response.status_code == 200
+        mock_attr.assert_not_called()
+
 
 @ddt.ddt
 class TestLogRequestUserChanges(TestCase):

openedx/core/djangoapps/user_authn/views/login.py

@@ -41,6 +41,7 @@
 from common.djangoapps.util.json_request import JsonResponse
 from common.djangoapps.util.password_policy_validators import normalize_password
 from openedx.core.djangoapps.password_policy import compliance as password_policy_compliance
+from openedx.core.djangoapps.safe_sessions.middleware import mark_user_change_as_expected
 from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
 from openedx.core.djangoapps.user_authn.config.waffle import ENABLE_LOGIN_USING_THIRDPARTY_AUTH_ONLY
 from openedx.core.djangoapps.user_authn.cookies import get_response_with_refreshed_jwt_cookies, set_logged_in_cookies
@@ -593,6 +594,7 @@ def login_user(request, api_version='v1'):
         set_custom_attribute('login_user_auth_failed_error', False)
         set_custom_attribute('login_user_response_status', response.status_code)
         set_custom_attribute('login_user_redirect_url', redirect_url)
+        mark_user_change_as_expected(response, user.id)
         return response
     except AuthFailedError as error:
         response_content = error.get_response()

openedx/core/djangoapps/user_authn/views/logout.py

@@ -11,6 +11,7 @@
 from django.views.generic import TemplateView
 from oauth2_provider.models import Application
 
+from openedx.core.djangoapps.safe_sessions.middleware import mark_user_change_as_expected
 from openedx.core.djangoapps.user_authn.cookies import delete_logged_in_cookies
 from openedx.core.djangoapps.user_authn.utils import is_safe_login_or_logout_redirect
 from common.djangoapps.third_party_auth import pipeline as tpa_pipeline
@@ -69,7 +70,6 @@ def target(self):
 
     def dispatch(self, request, *args, **kwargs):
         # We do not log here, because we have a handler registered to perform logging on successful logouts.
-        request.is_from_logout = True
 
         # Get third party auth provider's logout url
         self.tpa_logout_url = tpa_pipeline.get_idp_logout_url_from_running_pipeline(request)
@@ -81,6 +81,7 @@ def dispatch(self, request, *args, **kwargs):
         # Clear the cookie used by the edx.org marketing site
         delete_logged_in_cookies(response)
 
+        mark_user_change_as_expected(response, None)
         return response
 
     def _build_logout_url(self, url):