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

feat: Remove VERIFY_USER_CHANGE_UNCONDITIONAL toggle (now always enabled) (#29324)

Browse Source 
We didn't see any errors after enabling this feature toggle, so remove it
in favor of the "True" setting.

Compare to PR #29306, which created the toggle.

ref: ARCHBOM-1952
This commit is contained in:
Tim McCormack
2021-12-01 17:02:47 +00:00
committed by GitHub
parent 2f4fedc237
commit 89699ed6a4
2 changed files with 2 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -91,7 +91,6 @@ 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
@@ -110,28 +109,6 @@ 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__)
@@ -421,20 +398,11 @@ 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
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:
# 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.

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

@@ -291,19 +291,9 @@ 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()
@@ -331,21 +321,10 @@ 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)