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.
This commit is contained in:
Tim McCormack
2022-01-21 17:09:39 +00:00
parent a3c65012af
commit 7fc20e69f4
7 changed files with 28 additions and 12 deletions

View File

@@ -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

View File

@@ -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):

View File

@@ -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)

View File

@@ -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:

View File

@@ -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()

View File

@@ -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):

View File

@@ -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):