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/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(): 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/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 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/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 diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py index b1d8d768a8..9000977f27 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 @@ -133,26 +133,40 @@ 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. -# .. toggle_use_cases: temporary +# 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_target_removal_date: 2022-08-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__) +# 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 +676,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 +686,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 +905,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 +914,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..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 @@ -482,7 +484,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):