From 7fc20e69f480ec707c7ac05252dfc82aebda78cb Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Fri, 21 Jan 2022 17:09:39 +0000 Subject: [PATCH 1/5] feat: Allow safe-session exemption even for exceptions Change `mark_user_change_as_expected` to no longer take the response object and instead convey the expected-change information via RequestCache. This requires edx-django-utils 4.4.2, which fixes the bug where RequestCache was cleared in the exception phase. Also, no longer mark `ENFORCE_SAFE_SESSIONS` toggle as temporary. We'll want it as an opt-out. I was tempted to take this opportunity to move any existing `mark_user_change_as_expected` calls to be closer to where the actual change request.user occurs, reducing risk of both false positives and false negatives, but it would be better to do that one at a time in case a move breaks something. (Ideally it would be called right after any `django.contrib.auth` `login` or `logout` call; previously, we were constrained by having to make the call after a response object had been created.) These changes can be made later if it becomes necessary. --- .../core/djangoapps/auth_exchange/views.py | 2 +- .../djangoapps/content_libraries/views.py | 2 +- .../djangoapps/safe_sessions/middleware.py | 28 +++++++++++++++---- .../safe_sessions/tests/test_middleware.py | 2 +- .../core/djangoapps/user_authn/views/login.py | 2 +- .../djangoapps/user_authn/views/logout.py | 2 +- .../djangoapps/user_authn/views/register.py | 2 +- 7 files changed, 28 insertions(+), 12 deletions(-) diff --git a/openedx/core/djangoapps/auth_exchange/views.py b/openedx/core/djangoapps/auth_exchange/views.py index adba25b72c..054408e0c0 100644 --- a/openedx/core/djangoapps/auth_exchange/views.py +++ b/openedx/core/djangoapps/auth_exchange/views.py @@ -153,5 +153,5 @@ class LoginWithAccessTokenView(APIView): login(request, request.user) # login generates and stores the user's cookies in the session response = HttpResponse(status=204) # cookies stored in the session are returned with the response - mark_user_change_as_expected(response, request.user.id) + mark_user_change_as_expected(request.user.id) return response diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index fe9672a7a9..a71ac03998 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -1022,7 +1022,7 @@ class LtiToolLaunchView(TemplateResponseMixin, LtiToolView): # Render context and response. context = self.get_context_data() response = self.render_to_response(context) - mark_user_change_as_expected(response, edx_user.id) + mark_user_change_as_expected(edx_user.id) return response def handle_ags(self): diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py index b1d8d768a8..f61a4f7739 100644 --- a/openedx/core/djangoapps/safe_sessions/middleware.py +++ b/openedx/core/djangoapps/safe_sessions/middleware.py @@ -90,7 +90,7 @@ from django.core.cache import cache from django.http import HttpResponse from django.utils.crypto import get_random_string from django.utils.deprecation import MiddlewareMixin - +from edx_django_utils.cache import RequestCache from edx_django_utils.monitoring import set_custom_attribute from edx_toggles.toggles import SettingToggle @@ -145,14 +145,26 @@ LOG_REQUEST_USER_CHANGE_HEADERS_DURATION = getattr(settings, 'LOG_REQUEST_USER_C # "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. -# .. toggle_use_cases: temporary +# .. toggle_use_cases: opt_out # .. toggle_creation_date: 2021-12-01 -# .. toggle_target_removal_date: 2022-08-01 # .. toggle_tickets: https://openedx.atlassian.net/browse/ARCHBOM-1861 ENFORCE_SAFE_SESSIONS = SettingToggle('ENFORCE_SAFE_SESSIONS', default=False) log = getLogger(__name__) +# RequestCache for conveying information from views back up to the +# middleware -- specifically, information about expected changes to +# request.user +# +# Rejected alternatives for where to place the annotation: +# +# - request object: Different request objects are presented to middlewares +# and views, so the attribute would be lost. +# - response object: Doesn't help in cases where an exception is thrown +# instead of a response returned. Still want to validate that users don't +# change unexpectedly on a 404, for example. +request_cache = RequestCache(namespace="safe-sessions") + class SafeCookieError(Exception): """ @@ -662,7 +674,7 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin): # # The relevant views set a flag to indicate the exemption. - if getattr(response, 'safe_sessions_expected_user_change', None): + if request_cache.get_cached_response('expected_user_change').is_found: return no_mismatch_dict if not hasattr(request, 'safe_cookie_verified_user_id'): @@ -672,6 +684,10 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin): if hasattr(request.user, 'real_user'): # If a view overrode the request.user with a masqueraded user, this will # revert/clean-up that change during response processing. + # Known places this is set: + # + # - lms.djangoapps.courseware.masquerade::setup_masquerade + # - openedx.core.djangoapps.content.learning_sequences.views::CourseOutlineView request.user = request.user.real_user # determine if the request.user is different now than it was on the initial request @@ -887,7 +903,7 @@ def track_request_user_changes(request): request.__class__ = SafeSessionRequestWrapper -def mark_user_change_as_expected(response, new_user_id): +def mark_user_change_as_expected(new_user_id): """ Indicate to the safe-sessions middleware that it is expected that the user is changing between the request and response phase of @@ -896,4 +912,4 @@ def mark_user_change_as_expected(response, new_user_id): The new_user_id may be None or an LMS user ID, and may be the same as the previous user ID. """ - response.safe_sessions_expected_user_change = {'new_user_id': new_user_id} + request_cache.set('expected_user_change', new_user_id) diff --git a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py index 7fe109333c..a38a7e6492 100644 --- a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py +++ b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py @@ -482,7 +482,7 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase 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) + mark_user_change_as_expected(new_user.id) with self.assert_no_warning_logged(): with patch('openedx.core.djangoapps.safe_sessions.middleware.set_custom_attribute') as mock_attr: diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index 3667c33f84..e73bf84a60 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -605,7 +605,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) + mark_user_change_as_expected(user.id) return response except AuthFailedError as error: response_content = error.get_response() diff --git a/openedx/core/djangoapps/user_authn/views/logout.py b/openedx/core/djangoapps/user_authn/views/logout.py index 4d88433a4f..c00fa2a5da 100644 --- a/openedx/core/djangoapps/user_authn/views/logout.py +++ b/openedx/core/djangoapps/user_authn/views/logout.py @@ -82,7 +82,7 @@ class LogoutView(TemplateView): # Clear the cookie used by the edx.org marketing site delete_logged_in_cookies(response) - mark_user_change_as_expected(response, None) + mark_user_change_as_expected(None) return response def _build_logout_url(self, url): diff --git a/openedx/core/djangoapps/user_authn/views/register.py b/openedx/core/djangoapps/user_authn/views/register.py index 183421cca8..78062ab3df 100644 --- a/openedx/core/djangoapps/user_authn/views/register.py +++ b/openedx/core/djangoapps/user_authn/views/register.py @@ -589,7 +589,7 @@ class RegistrationView(APIView): path='/', secure=request.is_secure() ) # setting the cookie to show account activation dialogue in platform and learning MFE - mark_user_change_as_expected(response, user.id) + mark_user_change_as_expected(user.id) return response def _handle_duplicate_email_username(self, request, data): From e6536d0d0ef96babfbf0b64024e84754653e4f86 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Tue, 18 Jan 2022 22:33:30 +0000 Subject: [PATCH 2/5] test: Stop sharing API client between users in unit tests Using the same Client or APIClient instance for multiple users, where one user has an active session and the other is making an Authorization header call, results in a Safe Sessions violation. By using separate clients for different test users, we avoid this violation, allowing `ENFORCE_SAFE_SESSIONS` to be enabled by default. --- .../bulk_user_retirement/tests/test_views.py | 8 +++-- .../discussion/rest_api/tests/test_views.py | 24 ++++++++------- .../djangoapps/credit/tests/test_views.py | 5 ++-- .../enrollments/tests/test_views.py | 29 ++++++++++++------- 4 files changed, 40 insertions(+), 26 deletions(-) diff --git a/lms/djangoapps/bulk_user_retirement/tests/test_views.py b/lms/djangoapps/bulk_user_retirement/tests/test_views.py index f1cae5adaa..ac28c00f47 100644 --- a/lms/djangoapps/bulk_user_retirement/tests/test_views.py +++ b/lms/djangoapps/bulk_user_retirement/tests/test_views.py @@ -13,21 +13,21 @@ class BulkUserRetirementViewTests(APITestCase): """ def setUp(self): super().setUp() - self.client = APIClient() + login_client = APIClient() self.user1 = UserFactory.create( username='testuser1', email='test1@example.com', password='test1_password', profile__name="Test User1" ) - self.client.login(username=self.user1.username, password='test1_password') + login_client.login(username=self.user1.username, password='test1_password') self.user2 = UserFactory.create( username='testuser2', email='test2@example.com', password='test2_password', profile__name="Test User2" ) - self.client.login(username=self.user2.username, password='test2_password') + login_client.login(username=self.user2.username, password='test2_password') self.user3 = UserFactory.create( username='testuser3', email='test3@example.com', @@ -47,6 +47,8 @@ class BulkUserRetirementViewTests(APITestCase): required=True ) self.pending_state = RetirementState.objects.get(state_name='PENDING') + # Use a separate client for retirement worker (don't mix cookie state) + self.client = APIClient() self.client.force_authenticate(user=self.user1) def test_gdpr_user_retirement_api(self): diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index 391c797a28..9a24c81c59 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -528,6 +528,7 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): self.retirement.save() self.superuser = SuperuserFactory() + self.superuser_client = APIClient() self.retired_username = get_retired_username_by_username(self.user.username) self.url = reverse("retire_discussion_user") @@ -555,7 +556,7 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): self.register_get_user_retire_response(self.user) headers = self.build_jwt_headers(self.superuser) data = {'username': self.user.username} - response = self.client.post(self.url, data, **headers) + response = self.superuser_client.post(self.url, data, **headers) self.assert_response_correct(response, 204, b"") def test_downstream_forums_error(self): @@ -565,7 +566,7 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): self.register_get_user_retire_response(self.user, status=500, body="Server error") headers = self.build_jwt_headers(self.superuser) data = {'username': self.user.username} - response = self.client.post(self.url, data, **headers) + response = self.superuser_client.post(self.url, data, **headers) self.assert_response_correct(response, 500, '"Server error"') def test_nonexistent_user(self): @@ -576,7 +577,7 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): self.retired_username = get_retired_username_by_username(nonexistent_username) data = {'username': nonexistent_username} headers = self.build_jwt_headers(self.superuser) - response = self.client.post(self.url, data, **headers) + response = self.superuser_client.post(self.url, data, **headers) self.assert_response_correct(response, 404, None) def test_not_authenticated(self): @@ -594,8 +595,9 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """Tests for ReplaceUsernamesView""" def setUp(self): super().setUp() - self.client_user = UserFactory() - self.client_user.username = "test_replace_username_service_worker" + self.worker = UserFactory() + self.worker.username = "test_replace_username_service_worker" + self.worker_client = APIClient() self.new_username = "test_username_replacement" self.url = reverse("replace_discussion_username") @@ -616,11 +618,11 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): headers = {'HTTP_AUTHORIZATION': 'JWT ' + token} return headers - def call_api(self, user, data): + def call_api(self, user, client, data): """ Helper function to call API with data """ data = json.dumps(data) headers = self.build_jwt_headers(user) - return self.client.post(self.url, data, content_type='application/json', **headers) + return client.post(self.url, data, content_type='application/json', **headers) @ddt.data( [{}, {}], @@ -632,7 +634,7 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): data = { "username_mappings": mapping_data } - response = self.call_api(self.client_user, data) + response = self.call_api(self.worker, self.worker_client, data) assert response.status_code == 400 def test_auth(self): @@ -650,11 +652,11 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): # Test non-service worker random_user = UserFactory() - response = self.call_api(random_user, data) + response = self.call_api(random_user, APIClient(), data) assert response.status_code == 403 # Test service worker - response = self.call_api(self.client_user, data) + response = self.call_api(self.worker, self.worker_client, data) assert response.status_code == 200 def test_basic(self): @@ -669,7 +671,7 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): 'successful_replacements': data["username_mappings"] } self.register_get_username_replacement_response(self.user) - response = self.call_api(self.client_user, data) + response = self.call_api(self.worker, self.worker_client, data) assert response.status_code == 200 assert response.data == expected_response diff --git a/openedx/core/djangoapps/credit/tests/test_views.py b/openedx/core/djangoapps/credit/tests/test_views.py index 90f2a30a27..deb3c8726a 100644 --- a/openedx/core/djangoapps/credit/tests/test_views.py +++ b/openedx/core/djangoapps/credit/tests/test_views.py @@ -171,6 +171,7 @@ class CreditCourseViewSetTests(AuthMixin, UserMixin, TestCase): def test_oauth(self): """ Verify the endpoint supports OAuth, and only allows authorization for staff users. """ + client = Client() # avoid mixing cookies user = UserFactory(is_staff=False) oauth_client = ApplicationFactory.create() access_token = AccessTokenFactory.create(user=user, application=oauth_client).token @@ -179,13 +180,13 @@ class CreditCourseViewSetTests(AuthMixin, UserMixin, TestCase): } # Non-staff users should not have access to the API - response = self.client.get(self.path, **headers) + response = client.get(self.path, **headers) assert response.status_code == 403 # Staff users should have access to the API user.is_staff = True user.save() - response = self.client.get(self.path, **headers) + response = client.get(self.path, **headers) assert response.status_code == 200 def assert_course_created(self, course_id, response): diff --git a/openedx/core/djangoapps/enrollments/tests/test_views.py b/openedx/core/djangoapps/enrollments/tests/test_views.py index 3232ced200..104e6e6bcf 100644 --- a/openedx/core/djangoapps/enrollments/tests/test_views.py +++ b/openedx/core/djangoapps/enrollments/tests/test_views.py @@ -1349,6 +1349,7 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase): """ Create a course and user, then log in. """ super().setUp() self.superuser = SuperuserFactory() + self.superuser_client = Client() # Pass emit_signals when creating the course so it would be cached # as a CourseOverview. Enrollments require a cached CourseOverview. self.first_org_course = CourseFactory.create(emit_signals=True, org="org", course="course", run="run") @@ -1404,7 +1405,7 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase): def test_deactivate_enrollments(self): self._assert_active() self._create_test_retirement(self.user) - response = self._submit_unenroll(self.superuser, self.user.username) + response = self._submit_unenroll(self.user.username) assert response.status_code == status.HTTP_200_OK data = json.loads(response.content.decode('utf-8')) # order doesn't matter so compare sets @@ -1413,18 +1414,18 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase): def test_deactivate_enrollments_no_retirement_status(self): self._assert_active() - response = self._submit_unenroll(self.superuser, self.user.username) + response = self._submit_unenroll(self.user.username) assert response.status_code == status.HTTP_404_NOT_FOUND def test_deactivate_enrollments_unauthorized(self): self._assert_active() - response = self._submit_unenroll(self.user, self.user.username) + response = self._submit_unenroll(self.user.username, submitting_user=self.user, client=self.client) assert response.status_code == status.HTTP_403_FORBIDDEN self._assert_active() def test_deactivate_enrollments_no_username(self): self._assert_active() - response = self._submit_unenroll(self.superuser, None) + response = self._submit_unenroll(None) assert response.status_code == status.HTTP_404_NOT_FOUND data = json.loads(response.content.decode('utf-8')) assert data == 'Username not specified.' @@ -1433,23 +1434,23 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase): def test_deactivate_enrollments_empty_username(self): self._assert_active() self._create_test_retirement(self.user) - response = self._submit_unenroll(self.superuser, "") + response = self._submit_unenroll("") assert response.status_code == status.HTTP_404_NOT_FOUND self._assert_active() def test_deactivate_enrollments_invalid_username(self): self._assert_active() self._create_test_retirement(self.user) - response = self._submit_unenroll(self.superuser, "a made up username") + response = self._submit_unenroll("a made up username") assert response.status_code == status.HTTP_404_NOT_FOUND self._assert_active() def test_deactivate_enrollments_called_twice(self): self._assert_active() self._create_test_retirement(self.user) - response = self._submit_unenroll(self.superuser, self.user.username) + response = self._submit_unenroll(self.user.username) assert response.status_code == status.HTTP_200_OK - response = self._submit_unenroll(self.superuser, self.user.username) + response = self._submit_unenroll(self.user.username) assert response.status_code == status.HTTP_204_NO_CONTENT assert response.content.decode('utf-8') == '' self._assert_inactive() @@ -1465,14 +1466,22 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase): _, is_active = CourseEnrollment.enrollment_mode_for_user(self.user, course.id) assert not is_active - def _submit_unenroll(self, submitting_user, unenrolling_username): + def _submit_unenroll(self, unenrolling_username, submitting_user=None, client=None): + """ Submit enrollment, by default as superuser. """ + # Provide both or neither of the overrides + assert (submitting_user is None) == (client is None) + + # Avoid mixing cookies between two users + client = client or self.superuser_client + submitting_user = submitting_user or self.superuser + data = {} if unenrolling_username is not None: data['username'] = unenrolling_username url = reverse('unenrollment') headers = self.build_jwt_headers(submitting_user) - return self.client.post(url, json.dumps(data), content_type='application/json', **headers) + return client.post(url, json.dumps(data), content_type='application/json', **headers) @ddt.ddt From 4624bb7c3eec270f21d9efd16a768377ff6b55fd Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Wed, 19 Jan 2022 22:01:31 +0000 Subject: [PATCH 3/5] fix: Prevent SafeSessions false alarm in course outline alt-masquerading The course outline view has a way for a staff user to make a request as if they are another user, not just by using the masquerade mechanism but also by setting a request parameter. This can result in false positives in the safe-sessions middleware, and if `ENFORCE_SAFE_SESSIONS` is enabled the responses will be 401 errors. The fix here is to do the same thing that masquerading does in setting a `real_user` property on the new user object, which the safe-sessions middleware then undoes (restoring the request.user) before determing whether there's a mismatch. (Without this fix, enabling `ENFORCE_SAFE_SESSIONS` also causes some tests in `test_views.py` to fail.) --- .../core/djangoapps/content/learning_sequences/views.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content/learning_sequences/views.py b/openedx/core/djangoapps/content/learning_sequences/views.py index 72169afbb5..c410592473 100644 --- a/openedx/core/djangoapps/content/learning_sequences/views.py +++ b/openedx/core/djangoapps/content/learning_sequences/views.py @@ -213,7 +213,12 @@ class CourseOutlineView(APIView): target_username = request.GET.get("user") if target_username is not None: - return self._get_target_user(request, course_key, has_staff_access, target_username) + target_user = self._get_target_user(request, course_key, has_staff_access, target_username) + # Just like in masquerading, set real_user so that the + # SafeSessions middleware can see that the user didn't + # change unexpectedly. + target_user.real_user = request.user + return target_user _course_masquerade, user = setup_masquerade(request, course_key, has_staff_access) return user From fb1ad76e654f1c02303e83f908899df3460e0534 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Sat, 22 Jan 2022 01:53:31 +0000 Subject: [PATCH 4/5] fix: Exempt LTI in safe-sessions enforcement This LTI call was failing in unit tests when `ENFORCE_SAFE_SESSIONS` was enabled. I'm not sure why we didn't see failures in production when the toggle was enabled in config. --- lms/djangoapps/lti_provider/users.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lms/djangoapps/lti_provider/users.py b/lms/djangoapps/lti_provider/users.py index eabf821065..7c5a9c88eb 100644 --- a/lms/djangoapps/lti_provider/users.py +++ b/lms/djangoapps/lti_provider/users.py @@ -16,6 +16,7 @@ from django.db import IntegrityError, transaction from common.djangoapps.student.models import UserProfile from lms.djangoapps.lti_provider.models import LtiUser +from openedx.core.djangoapps.safe_sessions.middleware import mark_user_change_as_expected def authenticate_lti_user(request, lti_user_id, lti_consumer): @@ -96,6 +97,7 @@ def switch_user(request, lti_user, lti_consumer): # users by this point, but just in case we can return a 403. raise PermissionDenied() login(request, edx_user) + mark_user_change_as_expected(edx_user.id) def generate_random_edx_username(): From 9827a077aa77e578052a8afb102915baeea13bf0 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Sat, 15 Jan 2022 00:56:31 +0000 Subject: [PATCH 5/5] 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