From 455033458c1eca8260c57ec97129406e6592999d Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 2 Apr 2021 13:43:50 -0400 Subject: [PATCH] feat!: Replace logging WaffleSwitch with a django settinge. This was initially introduced as a temporary flag to be able to get more information. But if we get this kind of issue again, we'll need something like this logging to determine the source of the session collision. Rather than removing the code and adding it back in later, convert this temporary switch into an opt-in setting that can be used again in the future. BREAKING_CHANGE: 'safe_session.log_request_user_changes' switch no longer exists and is replaced with the 'LOG_REQUEST_USER_CHANGES' django setting which defaults to 'False' --- openedx/core/djangoapps/safe_sessions/middleware.py | 12 +++++------- .../safe_sessions/tests/test_middleware.py | 10 ++++------ 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py index 23559805b1..3abb622ed5 100644 --- a/openedx/core/djangoapps/safe_sessions/middleware.py +++ b/openedx/core/djangoapps/safe_sessions/middleware.py @@ -76,21 +76,19 @@ from django.utils.crypto import get_random_string from django.utils.deprecation import MiddlewareMixin from django.utils.encoding import python_2_unicode_compatible from edx_django_utils.monitoring import set_custom_attribute -from edx_toggles.toggles import WaffleSwitch from openedx.core.lib.mobile_utils import is_request_from_mobile_app -# .. toggle_name: safe_session.log_request_user_changes -# .. toggle_implementation: WaffleSwitch +# .. toggle_name: LOG_REQUEST_USER_CHANGES +# .. toggle_implementation: SettingToggle # .. toggle_default: False # .. toggle_description: Turn this toggle on to log anytime the `user` attribute of the request object gets # changed. This will also log the location where the change is coming from to quickly find issues. # .. toggle_warnings: This logging will be very verbose and so should probably not be left on all the time. -# .. toggle_use_cases: temporary +# .. toggle_use_cases: opt_in # .. toggle_creation_date: 2021-03-25 -# .. toggle_target_removal_date: 2021-05-01 # .. toggle_tickets: https://openedx.atlassian.net/browse/ARCHBOM-1718 -LOG_REQUEST_USER_CHANGES_FLAG = WaffleSwitch('safe_session.log_request_user_changes', __name__) +LOG_REQUEST_USER_CHANGES = getattr(settings, 'LOG_REQUEST_USER_CHANGES', False) log = getLogger(__name__) @@ -308,7 +306,7 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin): user_id = self.get_user_id_from_session(request) if safe_cookie_data.verify(user_id): # Step 4 request.safe_cookie_verified_user_id = user_id # Step 5 - if LOG_REQUEST_USER_CHANGES_FLAG.is_enabled(): + if LOG_REQUEST_USER_CHANGES: log_request_user_changes(request) else: return self._on_user_authentication_failed(request) diff --git a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py index cf6f6009ef..5686086152 100644 --- a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py +++ b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py @@ -71,10 +71,9 @@ class TestSafeSessionProcessRequest(TestSafeSessionsLogMixin, TestCase): """ assert SafeSessionMiddleware.get_user_id_from_session(self.request) == self.user.id + @patch("openedx.core.djangoapps.safe_sessions.middleware.LOG_REQUEST_USER_CHANGES", False) @patch("openedx.core.djangoapps.safe_sessions.middleware.log_request_user_changes") - @patch("openedx.core.djangoapps.safe_sessions.middleware.LOG_REQUEST_USER_CHANGES_FLAG") - def test_success(self, mock_log_request_user_changes_flag, mock_log_request_user_changes): - mock_log_request_user_changes_flag.is_enabled.return_value = False + def test_success(self, mock_log_request_user_changes): self.client.login(username=self.user.username, password='test') session_id = self.client.session.session_key safe_cookie_data = SafeCookieData.create(session_id, self.user.id) @@ -99,10 +98,9 @@ class TestSafeSessionProcessRequest(TestSafeSessionsLogMixin, TestCase): # verify extra request_user_logging not called. assert not mock_log_request_user_changes.called + @patch("openedx.core.djangoapps.safe_sessions.middleware.LOG_REQUEST_USER_CHANGES", True) @patch("openedx.core.djangoapps.safe_sessions.middleware.log_request_user_changes") - @patch("openedx.core.djangoapps.safe_sessions.middleware.LOG_REQUEST_USER_CHANGES_FLAG") - def test_log_request_user_flag_on(self, mock_log_request_user_changes_flag, mock_log_request_user_changes): - mock_log_request_user_changes_flag.is_enabled.return_value = True + def test_log_request_user_on(self, mock_log_request_user_changes): self.client.login(username=self.user.username, password='test') session_id = self.client.session.session_key safe_cookie_data = SafeCookieData.create(session_id, self.user.id)