Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: Add feature toggle to allow broader safe-sessions user checking (#29306)

Browse Source 
Contingent on new feature toggle `VERIFY_USER_CHANGE_UNCONDITIONAL`, check
for request/response user mismatches on all requests, not just those
setting a session cookie on the response.

This is intended to *restore* an older behavior. I believe that almost all
requests used to set a new session cookie, and for some reason no longer
do, so this is really just an attempt to return to that previous behavior
no matter whether a new session cookie will be set. (Previously, the
cookie-to-be-deleted check would still have been in effect, so this is
actually a slight change from the earlier behavior -- the logout response
will now be included, and then quickly ignored due to a later check.)

The off-by-default switch moves several lines of code out of a try block,
but also out from under an if guard that checks for certain cookie
conditions. The movement out of the try block should be irrelevant, since
neither of the relocated lines should be raising a SafeCookieError.
However, there is some chance that they could raise other exceptions when
called from their new location (and new situations), hence the use of a
feature toggle -- we'll want to make it easy to switch the new behavior off
quickly if we start seeing an increase in errors.

Once the change is well-exercised, we can remove the toggle and the old
call locations.

I'm not entirely sure about the change to the `verify_error` utility
function in the unit tests, but it seems like even unauthenticated requests
in Django end up with a user and session on the request object, so this is
probably a close-enough way to mock that out.

I duplicated a couple of tests to test with feature toggle on/off.

ref: ARCHBOM-1952
This commit is contained in:
Tim McCormack
2021-11-15 15:28:59 +00:00
committed by GitHub
parent 29608af616
commit 0bef57591d
2 changed files with 56 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

37
openedx/core/djangoapps/safe_sessions/middleware.py
View File

@@ -91,6 +91,7 @@ from django.utils.crypto import get_random_string
from django.utils.deprecation import MiddlewareMixin
from edx_django_utils.monitoring import set_custom_attribute
from edx_toggles.toggles import SettingToggle
from openedx.core.lib.mobile_utils import is_request_from_mobile_app
@@ -109,6 +110,28 @@ from openedx.core.lib.mobile_utils import is_request_from_mobile_app
# .. toggle_tickets: https://openedx.atlassian.net/browse/ARCHBOM-1718
LOG_REQUEST_USER_CHANGES = getattr(settings, 'LOG_REQUEST_USER_CHANGES', False)
# .. toggle_name: VERIFY_USER_CHANGE_UNCONDITIONAL
# .. toggle_implementation: SettingToggle
# .. toggle_default: False
# .. toggle_description: If False, only check for user mismatch if a session cookie is being set in
# the response (the status quo behavior). If True, check for user mismatches regardless of
# response cookie status (the desired future behavior). Once this has been enabled for a week or
# so, the toggle can be removed in favor of the True setting.
# (This is intended to *restore* an older behavior. It seems that all requests used to set a new
# session cookie, and for some reason no longer do, so this is really just an attempt to return
# to that previous behavior no matter whether a new session cookie will be set. The only difference
# should be that now logout responses will also be checked, but those now use a call that declares
# them safe, so that part should be a no-op.)
# .. toggle_warnings: This will expose some analysis code to a greater volume of requests and
# possibly different request configurations, which may expose some latent bugs that could cause
# request failure. Watch error rates and be ready to toggle off again.
# .. toggle_use_cases: temporary
# .. toggle_creation_date: 2021-11-12
# .. toggle_target_removal_date: 2022-01-01
# .. toggle_tickets: https://openedx.atlassian.net/browse/ARCHBOM-1952
VERIFY_USER_CHANGE_UNCONDITIONAL = SettingToggle('VERIFY_USER_CHANGE_UNCONDITIONAL', default=False,
module_name=__name__)
log = getLogger(__name__)
@@ -398,16 +421,24 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin):
except: # pylint: disable=bare-except
pass
verify_all = VERIFY_USER_CHANGE_UNCONDITIONAL.is_enabled()
# One of the two places this can be called from (desired future location).
if verify_all:
user_id_in_session = self.get_user_id_from_session(request)
self._verify_user(request, response, user_id_in_session) # Step 2
if not _is_cookie_marked_for_deletion(request) and _is_cookie_present(response):
try:
user_id_in_session = self.get_user_id_from_session(request)
self._verify_user(request, response, user_id_in_session) # Step 2
# The other, older place this can be called from.
if not verify_all:
user_id_in_session = self.get_user_id_from_session(request)
self._verify_user(request, response, user_id_in_session) # Step 2
# Use the user_id marked in the session instead of the
# one in the request in case the user is not set in the
# request, for example during Anonymous API access.
self.update_with_safe_session_cookie(response.cookies, user_id_in_session) # Step 3
except SafeCookieError:
_mark_cookie_for_deletion(request)

22
openedx/core/djangoapps/safe_sessions/tests/test_middleware.py
View File

@@ -291,9 +291,19 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, TestCase):
response = SafeSessionMiddleware().process_response(self.request, self.client.response)
assert response.status_code == 200
@override_settings(VERIFY_USER_CHANGE_UNCONDITIONAL=False)
def test_success(self):
self.verify_success()
@override_settings(VERIFY_USER_CHANGE_UNCONDITIONAL=True)
def test_success_with_verify_all(self):
"""
Test with verify-all feature enabled (non-default case). Remove this
test when feature toggle is permanently enabled, and remove override
from the pre-existing test_success.
"""
self.verify_success()
def test_success_from_mobile_web_view(self):
self.request.path = '/xblock/block-v1:org+course+run+type@html+block@block_id'
self.verify_success()
@@ -308,6 +318,7 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, TestCase):
Verifies error path.
"""
self.request.COOKIES[settings.SESSION_COOKIE_NAME] = 'not-a-safe-cookie'
self.request.session = self.client.session
with self.assert_parse_error():
request_response = SafeSessionMiddleware().process_request(self.request)
@@ -320,10 +331,21 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, TestCase):
SafeSessionMiddleware().process_response(self.request, self.client.response)
assert mock_delete_cookie.called
@override_settings(VERIFY_USER_CHANGE_UNCONDITIONAL=False)
def test_error(self):
self.request.META['HTTP_ACCEPT'] = 'text/html'
self.verify_error(302)
@override_settings(VERIFY_USER_CHANGE_UNCONDITIONAL=True)
def test_error_with_verify_all(self):
"""
Test with verify-all feature enabled (non-default case). Remove this
test when feature toggle is permanently enabled, and remove override
from the pre-existing test_error.
"""
self.request.META['HTTP_ACCEPT'] = 'text/html'
self.verify_error(302)
@ddt.data(['text/html', 302], ['', 401])
@ddt.unpack
@override_settings(REDIRECT_TO_LOGIN_ON_SAFE_SESSION_AUTH_FAILURE=False)