From 9827a077aa77e578052a8afb102915baeea13bf0 Mon Sep 17 00:00:00 2001
From: Tim McCormack <tmccormack@edx.org>
Date: Sat, 15 Jan 2022 00:56:31 +0000
Subject: [PATCH] feat: Enable ENFORCE_SAFE_SESSIONS by default; improve docs

This toggle has been shown to work, so enable by default. Will need to be
documented in release notes for deployers.
---
 .../djangoapps/safe_sessions/middleware.py    | 20 ++++++++++---------
 .../safe_sessions/tests/test_middleware.py    |  8 +++++---
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py
index f61a4f7739..9000977f27 100644
--- a/openedx/core/djangoapps/safe_sessions/middleware.py
+++ b/openedx/core/djangoapps/safe_sessions/middleware.py
@@ -133,22 +133,24 @@ LOG_REQUEST_USER_CHANGE_HEADERS_DURATION = getattr(settings, 'LOG_REQUEST_USER_C
 
 # .. toggle_name: ENFORCE_SAFE_SESSIONS
 # .. toggle_implementation: SettingToggle
-# .. toggle_default: False
-# .. toggle_description: Turn this toggle on to enforce safe-sessions policy.
+# .. toggle_default: True
+# .. toggle_description: Invalidate session and response if mismatch detected.
 #   That is, when the `user` attribute of the request object gets changed or
 #   no longer matches the session, the session will be invalidated and the
 #   response cancelled (changed to an error). This is intended as a backup
 #   safety measure in case an attacker (or bug) is able to change the user
-#   on a session in an unexpected way.  The behavior will be available for
-#   the Nutmeg named release and will become permanent in Olive.
-# .. toggle_warnings: Before enabling, confirm that incidences of the string
-#   "SafeCookieData user at request" in the logs only show false positives,
-#   such as people logging in while in possession of an already-valid session
-#   cookie.
+#   on a session in an unexpected way.
+# .. toggle_warnings: Should be disabled if debugging mismatches using the
+#   LOG_REQUEST_USER_CHANGE_HEADERS toggle, otherwise series of mismatching
+#   requests from the same user cannot be investigated.  Additionally, if
+#   enabling for the first time, confirm that incidences of the string
+#   "SafeCookieData user at request" in the logs are very rare; if they are
+#   not, it is likely that there is either a bug or that a login or
+#   registration endpoint needs to call ``mark_user_change_as_expected``.
 # .. toggle_use_cases: opt_out
 # .. toggle_creation_date: 2021-12-01
 # .. toggle_tickets: https://openedx.atlassian.net/browse/ARCHBOM-1861
-ENFORCE_SAFE_SESSIONS = SettingToggle('ENFORCE_SAFE_SESSIONS', default=False)
+ENFORCE_SAFE_SESSIONS = SettingToggle('ENFORCE_SAFE_SESSIONS', default=True)
 
 log = getLogger(__name__)
 
diff --git a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py
index a38a7e6492..37260d3804 100644
--- a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py
+++ b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py
@@ -341,9 +341,10 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase
         self.request.META = {'HTTP_USER_AGENT': 'open edX Mobile App Version 2.1'}
         self.verify_error(401)
 
+    @override_settings(ENFORCE_SAFE_SESSIONS=False)
     def test_warn_on_user_change_before_response(self):
         """
-        Verifies that warnings are emitted and custom attributes set if
+        Verifies that when enforcement disabled, warnings are emitted and custom attributes set if
         the user changes unexpectedly between request and response.
         """
         self.set_up_for_success()
@@ -358,10 +359,9 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase
         set_attr_call_args = [call.args for call in mock_attr.call_args_list]
         assert ("safe_sessions.user_mismatch", "request-response-mismatch") in set_attr_call_args
 
-    @override_settings(ENFORCE_SAFE_SESSIONS=True)
     def test_enforce_on_user_change_before_response(self):
         """
-        Copy of test_warn_on_user_change_before_response but with enforcement enabled.
+        Copy of test_warn_on_user_change_before_response but with enforcement enabled (default).
         The differences should be the status code and the session deletion.
         """
         self.set_up_for_success()
@@ -379,6 +379,7 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase
         set_attr_call_args = [call.args for call in mock_attr.call_args_list]
         assert ("safe_sessions.user_mismatch", "request-response-mismatch") in set_attr_call_args
 
+    @override_settings(ENFORCE_SAFE_SESSIONS=False)
     def test_warn_on_user_change_from_session(self):
         """
         Verifies that warnings are emitted and custom attributes set if
@@ -395,6 +396,7 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase
         set_attr_call_args = [call.args for call in mock_attr.call_args_list]
         assert ("safe_sessions.user_mismatch", "request-session-mismatch") in set_attr_call_args
 
+    @override_settings(ENFORCE_SAFE_SESSIONS=False)
     def test_warn_on_user_change_in_both(self):
         """
         Verifies that warnings are emitted and custom attributes set if