From 455033458c1eca8260c57ec97129406e6592999d Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 2 Apr 2021 13:43:50 -0400 Subject: [PATCH 1/2] 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) From 8e7144ae2e78ad5ac04a02a17544ffab37609ba3 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 2 Apr 2021 14:42:55 -0400 Subject: [PATCH 2/2] revert: "test: Update query counts." This reverts commit c2eabf6ccaf941c414bfe37758e2e5b16b68c850. We are changing this from a waffle setting to a django setting so we can undo this query count bump. --- lms/djangoapps/course_api/tests/test_views.py | 2 +- .../djangoapps/user_api/accounts/tests/test_views.py | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index bc7496930d..6b8a1a7737 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -387,7 +387,7 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear self.setup_user(self.audit_user) # These query counts were found empirically - query_counts = [55, 46, 46, 46, 46, 46, 46, 46, 46, 46, 16] + query_counts = [54, 46, 46, 46, 46, 46, 46, 46, 46, 46, 16] ordered_course_ids = sorted([str(cid) for cid in (course_ids + [c.id for c in self.courses])]) self.clear_caches() diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py index 6f7fa8e889..c3e2fa9dd8 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -171,7 +171,7 @@ class TestOwnUsernameAPI(CacheIsolationTestCase, UserAPITestCase): Test that a client (logged in) can get her own username. """ self.client.login(username=self.user.username, password=TEST_PASSWORD) - self._verify_get_own_username(18) + self._verify_get_own_username(17) def test_get_username_inactive(self): """ @@ -181,7 +181,7 @@ class TestOwnUsernameAPI(CacheIsolationTestCase, UserAPITestCase): self.client.login(username=self.user.username, password=TEST_PASSWORD) self.user.is_active = False self.user.save() - self._verify_get_own_username(18) + self._verify_get_own_username(17) def test_get_username_not_logged_in(self): """ @@ -355,7 +355,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): """ self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) self.create_mock_profile(self.user) - with self.assertNumQueries(25): + with self.assertNumQueries(24): response = self.send_get(self.different_client) self._verify_full_shareable_account_response(response, account_privacy=ALL_USERS_VISIBILITY) @@ -370,7 +370,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): """ self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) self.create_mock_profile(self.user) - with self.assertNumQueries(25): + with self.assertNumQueries(24): response = self.send_get(self.different_client) self._verify_private_account_response(response) @@ -517,7 +517,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): assert data['accomplishments_shared'] is False self.client.login(username=self.user.username, password=TEST_PASSWORD) - verify_get_own_information(23) + verify_get_own_information(22) # Now make sure that the user can get the same information, even if not active self.user.is_active = False @@ -537,7 +537,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): legacy_profile.save() self.client.login(username=self.user.username, password=TEST_PASSWORD) - with self.assertNumQueries(23): + with self.assertNumQueries(22): response = self.send_get(self.client) for empty_field in ("level_of_education", "gender", "country", "state", "bio",): assert response.data[empty_field] is None