From 5b7caf45d675696e07176ebe9c5e5a70b4219629 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Mon, 29 Mar 2021 11:44:10 -0400 Subject: [PATCH 1/5] fix: Don't log warnings on logout. When a user logs out, there are warnings logged right now because the session user_id mismatches(it becomes None on logout). Previously we would log the request mismatch on debug and the session mismatch as normal. This change will result in us logging nothing if the session change is not abnormal. --- .../core/djangoapps/safe_sessions/middleware.py | 16 ++++++++-------- .../safe_sessions/tests/test_middleware.py | 3 +-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py index 6e8a54e2eb..6b551ccb70 100644 --- a/openedx/core/djangoapps/safe_sessions/middleware.py +++ b/openedx/core/djangoapps/safe_sessions/middleware.py @@ -387,21 +387,21 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin): # If a view overrode the request.user with a masqueraded user, this will # revert/clean-up that change during response processing. request.user = request.user.real_user - if request.safe_cookie_verified_user_id != request.user.id: - # The user at response time is expected to be None when the user - # is logging out. To prevent extra noise in the logs, - # conditionally set the log level. - log_func = log.debug if request.user.id is None else log.warning - log_func( + # The user at response time is expected to be None when the user + # is logging out. We won't log that. + if request.safe_cookie_verified_user_id != request.user.id and request.user.id is not None: + log.warning( ( "SafeCookieData user at request '{0}' does not match user at response: '{1}' " "for request path '{2}'" - ).format( + ).format( # pylint: disable=logging-format-interpolation request.safe_cookie_verified_user_id, request.user.id, request.path, ), ) set_custom_attribute("safe_sessions.user_mismatch", "request-response-mismatch") - if request.safe_cookie_verified_user_id != userid_in_session: + # The user session at response time is expected to be None when the user + # is logging out. We won't log that. + if request.safe_cookie_verified_user_id != userid_in_session and userid_in_session is not None: log.warning( ( "SafeCookieData user at request '{0}' does not match user in session: '{1}' " diff --git a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py index c1e99ce93c..cf6f6009ef 100644 --- a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py +++ b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py @@ -213,8 +213,7 @@ class TestSafeSessionProcessResponse(TestSafeSessionsLogMixin, TestCase): self.request.user = AnonymousUser() self.request.session[SESSION_KEY] = self.user.id with self.assert_no_error_logged(): - with self.assert_logged_for_request_user_mismatch(self.user.id, None, 'debug', self.request.path): - self.assert_response(set_request_user=False, set_session_cookie=True) + self.assert_response(set_request_user=False, set_session_cookie=True) def test_update_cookie_data_at_step_3(self): self.assert_response(set_request_user=True, set_session_cookie=True) From 23f2b758d414e95c84313c87e9f03e8db4902bca Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 1 Apr 2021 10:25:46 -0400 Subject: [PATCH 2/5] fix: Print more stack frames on requset tracing. Six frames was not enough because for DRF views the request gets wrapped in a proxy object and so we need more of the stack to see what part of the code we're in that actually invokes the use change. --- openedx/core/djangoapps/safe_sessions/middleware.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py index 6b551ccb70..23559805b1 100644 --- a/openedx/core/djangoapps/safe_sessions/middleware.py +++ b/openedx/core/djangoapps/safe_sessions/middleware.py @@ -535,7 +535,7 @@ def log_request_user_changes(request): if name == 'user': stack = inspect.stack() # Written this way in case you need more of the stack for debugging. - location = "\n".join("%30s : %s:%d" % (t[3], t[1], t[2]) for t in stack[0:6]) + location = "\n".join("%30s : %s:%d" % (t[3], t[1], t[2]) for t in stack[0:12]) if not hasattr(request, name): original_user = value From 118f095110a91202a8e3704b5d908d3cf53b7568 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 1 Apr 2021 10:27:19 -0400 Subject: [PATCH 3/5] fix: Assume logged in if user has a valid session. Previously they also had to have a valid JWT cookie which led to a weird corner case where a user was logged in but still showed the login form resulting in some confusion and odd behavior. This change gives precedence to the session token to determine whether or not someone is logged into the LMS but ensures that if you go through the login flow, you refresh your JWT cookies. This should not cause any breakage for MFE flows that might redirect to the LMS login page since the JWT would get refreshed if it's out of date but the session is valid. --- .../core/djangoapps/user_authn/views/login_form.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/user_authn/views/login_form.py b/openedx/core/djangoapps/user_authn/views/login_form.py index aa21a50cf0..2b851d49ab 100644 --- a/openedx/core/djangoapps/user_authn/views/login_form.py +++ b/openedx/core/djangoapps/user_authn/views/login_form.py @@ -23,7 +23,7 @@ from openedx.core.djangoapps.user_api.accounts.utils import ( is_secondary_email_feature_enabled ) from openedx.core.djangoapps.user_api.helpers import FormDescription -from openedx.core.djangoapps.user_authn.cookies import are_logged_in_cookies_set +from openedx.core.djangoapps.user_authn.cookies import set_logged_in_cookies from openedx.core.djangoapps.user_authn.toggles import should_redirect_to_authn_microfrontend from openedx.core.djangoapps.user_authn.views.password_reset import get_password_reset_form from openedx.core.djangoapps.user_authn.views.registration_form import RegistrationFormFactory @@ -150,11 +150,13 @@ def login_and_registration_form(request, initial_mode="login"): redirect_to = get_next_url_for_login_page(request) # If we're already logged in, redirect to the dashboard - # Note: We check for the existence of login-related cookies in addition to is_authenticated + # Note: If the session is valid, we update all logged_in cookies(in particular JWTs) # since Django's SessionAuthentication middleware auto-updates session cookies but not - # the other login-related cookies. See ARCH-282. - if request.user.is_authenticated and are_logged_in_cookies_set(request): - return redirect(redirect_to) + # the other login-related cookies. See ARCH-282 and ARCHBOM-1718 + if request.user.is_authenticated: + response = redirect(redirect_to) + response = set_logged_in_cookies(request, response, request.user) + return response # Retrieve the form descriptions from the user API form_descriptions = _get_form_descriptions(request) From c45ffd75099bc8af8e034ac3e5ae1a6f76b52481 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 1 Apr 2021 16:41:20 -0400 Subject: [PATCH 4/5] test: Test login redirects prefer session cookies. Add a test to ensure that the login page redirect as long as we have a valid session even if we have expired on non-existent JWT cookies. --- lms/djangoapps/support/tests/test_views.py | 3 +++ .../user_authn/views/tests/test_logistration.py | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/lms/djangoapps/support/tests/test_views.py b/lms/djangoapps/support/tests/test_views.py index 9bc8111e0b..cf0c26fbcd 100644 --- a/lms/djangoapps/support/tests/test_views.py +++ b/lms/djangoapps/support/tests/test_views.py @@ -81,6 +81,9 @@ class SupportViewManageUserTests(SupportViewTestCase): """ Tests password assistance """ + # Ensure that user is not logged in if they need + # password assistance. + self.client.logout() url = '/password_assistance' response = self.client.get(url) assert response.status_code == 200 diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py b/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py index c5c4ef7ded..b7e5536ad9 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py @@ -27,7 +27,9 @@ from common.djangoapps.course_modes.models import CourseMode from lms.djangoapps.branding.api import get_privacy_url from openedx.core.djangoapps.site_configuration.tests.mixins import SiteMixin from openedx.core.djangoapps.theming.tests.test_util import with_comprehensive_theme_context +from openedx.core.djangoapps.user_authn.cookies import JWT_COOKIE_NAMES from openedx.core.djangoapps.user_authn.toggles import REDIRECT_TO_AUTHN_MICROFRONTEND +from openedx.core.djangoapps.user_authn.tests.utils import setup_login_oauth_client from openedx.core.djangoapps.user_authn.views.login_form import login_and_registration_form from openedx.core.djangolib.js_utils import dump_js_escaped_json from openedx.core.djangolib.markup import HTML, Text @@ -145,8 +147,10 @@ class LoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMixin, ModuleSto response = self.client.get(login_url) assert response.status_code == 200 + @mock.patch.dict("django.conf.settings.FEATURES", {"DISABLE_SET_JWT_COOKIES_FOR_TESTS": False}) @ddt.data("signin_user", "register_user") def test_login_and_registration_form_already_authenticated(self, url_name): + setup_login_oauth_client() # call the account registration api that sets the login cookies url = reverse('user_api_registration') request_data = { @@ -167,6 +171,19 @@ class LoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMixin, ModuleSto response = self.client.get(reverse(url_name)) self.assertRedirects(response, reverse("dashboard")) + # Refresh login even if JWT cookies are expired. + # (Give precedence to the session.) + for name in JWT_COOKIE_NAMES: + del self.client.cookies[name] + + # Verify that we're still redirected to the dashboard + response = self.client.get(reverse(url_name)) + self.assertRedirects(response, reverse("dashboard")) + + # Verify that we got new JWT cookies. + for name in JWT_COOKIE_NAMES: + assert name in self.client.cookies + @ddt.data( (None, "signin_user"), (None, "register_user"), From 4f725aa152a98144e6aacf7d142dd6a905054b29 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 1 Apr 2021 16:04:47 -0400 Subject: [PATCH 5/5] test: Add a test for setup_masquerade. Test to verify the side-effects of calling this function since we now rely on one of them in the SafeSessionMiddleware. --- .../courseware/tests/test_masquerade.py | 49 ++++++++++++++++++- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index 3c75002919..489fe254ef 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -6,11 +6,12 @@ Unit tests for masquerade. import json import pickle from datetime import datetime +from importlib import import_module from unittest.mock import patch import pytest import ddt from django.conf import settings -from django.test import TestCase +from django.test import TestCase, RequestFactory from django.urls import reverse from edx_toggles.toggles.testutils import override_waffle_flag from pytz import UTC @@ -18,9 +19,11 @@ from xblock.runtime import DictKeyValueStore from capa.tests.response_xml_factory import OptionResponseXMLFactory from lms.djangoapps.courseware.masquerade import ( + MASQUERADE_SETTINGS_KEY, CourseMasquerade, MasqueradingKeyValueStore, - get_masquerading_user_group + get_masquerading_user_group, + setup_masquerade, ) from lms.djangoapps.courseware.tests.factories import StaffFactory from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase, MasqueradeMixin, masquerade_as_group_member @@ -535,3 +538,45 @@ class CourseMasqueradeTest(TestCase): pickled_cmasq = pickle.dumps(cmasq) unpickled_cmasq = pickle.loads(pickled_cmasq) assert unpickled_cmasq.user_name is None + + +class SetupMasqueradeTests(SharedModuleStoreTestCase,): + """ + Tests for the setup_masquerade function. + """ + def setUp(self): + super().setUp() + self.course = CourseFactory.create(number='setup-masquerade-test', metadata={'start': datetime.now(UTC)}) + self.request = RequestFactory().request() + self.staff = StaffFactory(course_key=self.course.id) + self.student = UserFactory() + + CourseEnrollment.enroll(self.student, self.course.id) + + session_key = "abcdef" + self.request.user = self.staff + self.request.session = import_module(settings.SESSION_ENGINE).SessionStore(session_key) + + def test_setup_masquerade(self): + masquerade_settings = { + self.course.id: CourseMasquerade( + course_key=self.course.id, + role='student', + user_name=self.student.username + ) + } + self.request.session[MASQUERADE_SETTINGS_KEY] = masquerade_settings + + course_masquerade, masquerade_user = setup_masquerade( + self.request, + self.course.id, + staff_access=True + ) + + # Warning: the SafeSessions middleware relies on the `real_user` attribute to see if a + # user is masquerading as another user. If the name of this attribute is changing, please update + # the check in SafeSessionMiddleware._verify_user as well. + assert masquerade_user.real_user == self.staff + assert masquerade_user == self.student + assert self.request.user.masquerade_settings == masquerade_settings + assert course_masquerade == masquerade_settings[self.course.id]