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] 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/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py index 6e8a54e2eb..23559805b1 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}' " @@ -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 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) 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) 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 8a47a2dc5f..3c202af712 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"),