From 7fc20e69f480ec707c7ac05252dfc82aebda78cb Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Fri, 21 Jan 2022 17:09:39 +0000 Subject: [PATCH] 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):