From a86486bb934d90d7869a7685e4d4667b8bd08da2 Mon Sep 17 00:00:00 2001 From: Syed Sajjad Hussain Shah Date: Thu, 30 Nov 2023 13:06:47 +0500 Subject: [PATCH] feat: logout other sessions on email change --- cms/djangoapps/contentstore/tests/tests.py | 1 + .../djangoapps/student/signals/receivers.py | 10 +- .../student/tests/test_receivers.py | 25 +++- common/djangoapps/student/views/management.py | 2 +- lms/envs/common.py | 16 +++ .../lang_pref/tests/test_middleware.py | 2 + .../djangoapps/safe_sessions/middleware.py | 53 ++++++++ .../safe_sessions/tests/test_middleware.py | 124 ++++++++++++++++++ .../core/djangoapps/user_authn/views/login.py | 9 +- .../user_authn/views/tests/test_login.py | 51 ++++--- 10 files changed, 264 insertions(+), 29 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/tests.py b/cms/djangoapps/contentstore/tests/tests.py index bd2c3dbfd3c8..20685392927c 100644 --- a/cms/djangoapps/contentstore/tests/tests.py +++ b/cms/djangoapps/contentstore/tests/tests.py @@ -85,6 +85,7 @@ def activate_user(self, email): @ddt +@override_settings(ENFORCE_SESSION_EMAIL_MATCH=False) class AuthTestCase(ContentStoreTestCase): """Check that various permissions-related things work""" diff --git a/common/djangoapps/student/signals/receivers.py b/common/djangoapps/student/signals/receivers.py index 6d0cf4f061ca..b3efcf9668ce 100644 --- a/common/djangoapps/student/signals/receivers.py +++ b/common/djangoapps/student/signals/receivers.py @@ -23,6 +23,7 @@ ) from common.djangoapps.student.models_api import confirm_name_change from common.djangoapps.student.signals import USER_EMAIL_CHANGED +from openedx.core.djangoapps.safe_sessions.middleware import EmailChangeSessionInvalidationMiddleware from openedx.features.name_affirmation_api.utils import is_name_affirmation_installed logger = logging.getLogger(__name__) @@ -105,8 +106,13 @@ def listen_for_verified_name_approved(sender, user_id, profile_name, **kwargs): @receiver(USER_EMAIL_CHANGED) -def _listen_for_user_email_changed(sender, user, **kwargs): - """ If user has changed their email, update that in email Braze. """ +def _listen_for_user_email_changed(sender, user, request, **kwargs): + """ If user has changed their email, update that in session and Braze profile. """ + + if settings.ENFORCE_SESSION_EMAIL_MATCH: + # Store the user's email for session consistency (used by EmailChangeSessionInvalidationMiddleware) + EmailChangeSessionInvalidationMiddleware.register_email_change(request, user.email) + email = user.email user_id = user.id attributes = [{'email': email, 'external_id': user_id}] diff --git a/common/djangoapps/student/tests/test_receivers.py b/common/djangoapps/student/tests/test_receivers.py index 8ce869a73198..9daf0e064a5c 100644 --- a/common/djangoapps/student/tests/test_receivers.py +++ b/common/djangoapps/student/tests/test_receivers.py @@ -2,6 +2,7 @@ from unittest import skipUnless from unittest.mock import patch +from django.test.utils import override_settings from edx_toggles.toggles.testutils import override_waffle_flag @@ -9,7 +10,7 @@ from common.djangoapps.student.signals.signals import USER_EMAIL_CHANGED from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory, UserProfileFactory from lms.djangoapps.courseware.toggles import COURSEWARE_MICROFRONTEND_PROGRESS_MILESTONES -from openedx.core.djangolib.testing.utils import skip_unless_lms +from openedx.core.djangolib.testing.utils import skip_unless_lms, get_mock_request from openedx.features.name_affirmation_api.utils import is_name_affirmation_installed from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order @@ -75,10 +76,26 @@ def test_listen_for_verified_name_approved(self): @patch('common.djangoapps.student.signals.receivers.get_braze_client') def test_listen_for_user_email_changed(self, mock_get_braze_client): """ - Ensure that USER_EMAIL_CHANGED signal triggers correct calls to get_braze_client. + Ensure that USER_EMAIL_CHANGED signal triggers correct calls to + get_braze_client and update email in session if ENFORCE_SESSION_EMAIL_MATCH + is enabled. """ user = UserFactory(email='email@test.com', username='jdoe') + request = get_mock_request(user=user) + request.session = self.client.session - USER_EMAIL_CHANGED.send(sender=None, user=user) + # simulating email change + user.email = 'new_email@test.com' + user.save() - assert mock_get_braze_client.called + with override_settings(ENFORCE_SESSION_EMAIL_MATCH=False): + USER_EMAIL_CHANGED.send(sender=None, user=user, request=request) + + assert mock_get_braze_client.called + assert request.session.get('email', None) is None + + with override_settings(ENFORCE_SESSION_EMAIL_MATCH=True): + USER_EMAIL_CHANGED.send(sender=None, user=user, request=request) + + assert mock_get_braze_client.called + assert request.session.get('email', None) == user.email diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index cb3df1627fd9..d3e1c54180d3 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -910,7 +910,7 @@ def confirm_email_change(request, key): response = render_to_response("email_change_successful.html", address_context) - USER_EMAIL_CHANGED.send(sender=None, user=user) + USER_EMAIL_CHANGED.send(sender=None, user=user, request=request) return response diff --git a/lms/envs/common.py b/lms/envs/common.py index 59377b67cc5d..33557c90250a 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2224,6 +2224,9 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring #'django.contrib.auth.middleware.AuthenticationMiddleware', 'openedx.core.djangoapps.cache_toolbox.middleware.CacheBackedAuthenticationMiddleware', + # Middleware to flush user's session in other browsers when their email is changed. + 'openedx.core.djangoapps.safe_sessions.middleware.EmailChangeSessionInvalidationMiddleware', + 'common.djangoapps.student.middleware.UserStandingMiddleware', 'openedx.core.djangoapps.contentserver.middleware.StaticContentServer', @@ -5024,6 +5027,19 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring # .. toggle_tickets: https://openedx.atlassian.net/browse/VAN-838 ENABLE_DYNAMIC_REGISTRATION_FIELDS = False +############## Settings for EmailChangeSessionInvalidationMiddleware ############### + +# .. toggle_name: ENFORCE_SESSION_EMAIL_MATCH +# .. toggle_implementation: DjangoSetting +# .. toggle_default: False +# .. toggle_description: When enabled, this setting invalidates sessions in other browsers upon email change, +# while preserving the session validity in the browser where the email change occurs. +# .. toggle_use_cases: temporary +# .. toggle_creation_date: 2023-12-07 +# .. toggle_target_removal_date: None +# .. toggle_tickets: https://openedx.atlassian.net/browse/VAN-1694 +ENFORCE_SESSION_EMAIL_MATCH = False + LEARNER_HOME_MFE_REDIRECT_PERCENTAGE = 0 ############### Settings for the ace_common plugin ################# diff --git a/openedx/core/djangoapps/lang_pref/tests/test_middleware.py b/openedx/core/djangoapps/lang_pref/tests/test_middleware.py index e49c3aa66bf7..a860d0cbf688 100644 --- a/openedx/core/djangoapps/lang_pref/tests/test_middleware.py +++ b/openedx/core/djangoapps/lang_pref/tests/test_middleware.py @@ -11,6 +11,7 @@ from django.contrib.sessions.middleware import SessionMiddleware from django.http import HttpResponse from django.test.client import Client, RequestFactory +from django.test.utils import override_settings from django.urls import reverse from django.utils.translation.trans_real import parse_accept_lang_header @@ -30,6 +31,7 @@ @ddt.ddt +@override_settings(ENFORCE_SESSION_EMAIL_MATCH=False) class TestUserPreferenceMiddleware(CacheIsolationTestCase): """ Tests to make sure user preferences are getting properly set in the middleware. diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py index 5f4449d93c42..ad13ea0557d5 100644 --- a/openedx/core/djangoapps/safe_sessions/middleware.py +++ b/openedx/core/djangoapps/safe_sessions/middleware.py @@ -768,6 +768,59 @@ def _get_encrypted_request_headers(request): return encrypt_for_log(str(request.headers), getattr(settings, 'SAFE_SESSIONS_DEBUG_PUBLIC_KEY', None)) +class EmailChangeSessionInvalidationMiddleware(MiddlewareMixin): + """ + Middleware responsible for invalidating user sessions + by detecting a mismatch between the user's email in + the session and request.user.email. + + This middleware ensures that the sessions in other browsers + are invalidated when a user changes their email in one browser. + The active session in which the email change is made will remain valid. + + The user's email is stored in their session during login + and gets updated when the user changes their email. + This middleware checks for any mismatch between the stored email + and the current user's email in each request, and if found, + it invalidates/flushes the session and mark cookies for deletion in request + which are then deleted in the process_response of SafeSessionMiddleware. + """ + + def process_request(self, request): + """ + Invalidate the user session if there's a mismatch + between the email in the user's session and request.user.email. + """ + if settings.ENFORCE_SESSION_EMAIL_MATCH and request.user.is_authenticated: + user_session_email = request.session.get('email', None) + + if user_session_email is not None and request.user.email != user_session_email: + # Flush the session and mark cookies for deletion. + request.session.flush() + request.user = AnonymousUser() + _mark_cookie_for_deletion(request) + EmailChangeSessionInvalidationMiddleware._set_session_email_match_custom_attribute(False) + else: + EmailChangeSessionInvalidationMiddleware._set_session_email_match_custom_attribute(True) + + @staticmethod + def register_email_change(request, email): + """ + Sets email in session for comparison + """ + request.session['email'] = email + + @staticmethod + def _set_session_email_match_custom_attribute(value): + """ + Sets custom attribute of session_email_match + """ + # .. custom_attribute_name: session_email_match + # .. custom_attribute_description: Indicates whether there is a match between the + # email in the user's session and the current user's email in the request. + set_custom_attribute('session_email_match', value) + + def obscure_token(value: Union[str, None]) -> Union[str, None]: """ Return a short string that can be used to detect other occurrences diff --git a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py index 079cdcdd0c71..4e689d1b0d1b 100644 --- a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py +++ b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py @@ -17,6 +17,7 @@ from common.djangoapps.student.tests.factories import UserFactory from ..middleware import ( + EmailChangeSessionInvalidationMiddleware, SafeCookieData, SafeSessionMiddleware, mark_user_change_as_expected, @@ -615,3 +616,126 @@ def test_user_change_with_no_ids(self): request.user = object() assert len(request.debug_user_changes) == 2 assert "Changing request user but user has no id." in request.debug_user_changes[1] + + +class TestEmailChangeSessionInvalidationMiddleware(TestSafeSessionsLogMixin, TestCase): + """ + Test class for EmailChangeSessionInvalidationMiddleware + """ + + def setUp(self): + super().setUp() + self.TEST_PASSWORD = 'Password1234' + self.user = UserFactory.create(email='test@example.com', password=self.TEST_PASSWORD) + self.addCleanup(set_current_request, None) + self.request = get_mock_request(self.user) + self.client.response = HttpResponse() + self.client.response.cookies = SimpleCookie() + + @override_settings(ENFORCE_SESSION_EMAIL_MATCH=False) + @patch('openedx.core.djangoapps.safe_sessions.middleware._mark_cookie_for_deletion') + def test_process_request_settings_disabled(self, mock_mark_cookie_for_deletion): + """ + Calls EmailChangeSessionInvalidationMiddleware.process_request when no user is authenticated. + Verifies that session and cookies are not affected. + """ + # Log in the user + self.client.login(email=self.user.email, password=self.TEST_PASSWORD) + self.request.session = self.client.session + + # Ensure no email is stored in the session + self.assertIsNone(self.request.session.get('email')) + + # Call process_request without authenticating a user + EmailChangeSessionInvalidationMiddleware(get_response=lambda request: None).process_request(self.request) + + # Assert that session and cookies are not affected + # Assert that _mark_cookie_for_deletion not called + mock_mark_cookie_for_deletion.assert_not_called() + + @override_settings(ENFORCE_SESSION_EMAIL_MATCH=True) + @patch('openedx.core.djangoapps.safe_sessions.middleware._mark_cookie_for_deletion') + def test_process_request_not_authenticated(self, mock_mark_cookie_for_deletion): + """ + Calls EmailChangeSessionInvalidationMiddleware.process_request when no user is authenticated. + Verifies that session and cookies are not affected. + """ + # Unauthenticated User + self.request.user = AnonymousUser() + + # Call process_request without authenticating a user + EmailChangeSessionInvalidationMiddleware(get_response=lambda request: None).process_request(self.request) + + # Assert that session and cookies are not affected + # Assert that _mark_cookie_for_deletion not called + mock_mark_cookie_for_deletion.assert_not_called() + + @override_settings(ENFORCE_SESSION_EMAIL_MATCH=True) + def test_process_request_authenticated_no_session_email(self): + """ + Calls EmailChangeSessionInvalidationMiddleware.process_request when a user is authenticated + but there is no email in the session. Verifies that session and cookies are not affected. + """ + # Log in the user + self.client.login(email=self.user.email, password=self.TEST_PASSWORD) + self.request.session = self.client.session + + # Ensure no email is stored in the session + self.assertIsNone(self.request.session.get('email')) + + # Call process_request + EmailChangeSessionInvalidationMiddleware(get_response=lambda request: None).process_request(self.request) + + # Assert that the session and cookies are not affected + self.assertIsNone(self.request.session.get('email')) + self.assertEqual(len(self.client.response.cookies), 0) + + @override_settings(ENFORCE_SESSION_EMAIL_MATCH=True) + def test_process_request_authenticated_matching_session_email(self): + """ + Calls EmailChangeSessionInvalidationMiddleware.process_request when a user is authenticated + and the session email matches request.user.email. Verifies that session and cookies are not affected. + """ + # Log in the user + self.client.login(email=self.user.email, password=self.TEST_PASSWORD) + self.request.session = self.client.session + self.request.session['email'] = self.user.email # Set the session email to match request.user.email + self.client.response.set_cookie(settings.SESSION_COOKIE_NAME, 'authenticated') # Add some logged-in cookie + + # Ensure email matches in the session + self.assertEqual(self.request.session.get('email'), self.user.email) + # Ensure session cookie exist + self.assertEqual(len(self.client.response.cookies), 1) + + # Call process_request + EmailChangeSessionInvalidationMiddleware(get_response=lambda request: None).process_request(self.request) + + # Assert that the session and cookies are not affected + self.assertEqual(self.request.session.get('email'), self.user.email) + self.assertEqual(len(self.client.response.cookies), 1) + self.assertEqual(self.client.response.cookies[settings.SESSION_COOKIE_NAME].value, 'authenticated') + + @override_settings(ENFORCE_SESSION_EMAIL_MATCH=True) + @patch('openedx.core.djangoapps.safe_sessions.middleware._mark_cookie_for_deletion') + def test_process_request_email_mismatch(self, mock_mark_cookie_for_deletion): + """ + Calls EmailChangeSessionInvalidationMiddleware.process_request with + a mismatch between session email and user email. Verifies that session + is flushed and cookies are marked for deletion. + """ + # Log in the user + self.client.login(email=self.user.email, password=self.TEST_PASSWORD) + self.request.session = self.client.session + self.request.session['email'] = self.user.email # Set the session email to match request.user.email + self.client.response.set_cookie(settings.SESSION_COOKIE_NAME, 'authenticated') # Add some logged-in cookie + + # Modify session email to create a mismatch (Email changed) + self.request.session['email'] = 'mismatched_email@example.com' + + # Call process_request + EmailChangeSessionInvalidationMiddleware(get_response=lambda request: None).process_request(self.request) + + # Assert that the session is flushed and cookies marked for deletion + mock_mark_cookie_for_deletion.assert_called() + assert self.request.session.get(SESSION_KEY) is None + assert self.request.user == AnonymousUser() diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index 4880e58a433c..ccb9adaee816 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -39,7 +39,10 @@ from common.djangoapps.util.json_request import JsonResponse from common.djangoapps.util.password_policy_validators import normalize_password from openedx.core.djangoapps.password_policy import compliance as password_policy_compliance -from openedx.core.djangoapps.safe_sessions.middleware import mark_user_change_as_expected +from openedx.core.djangoapps.safe_sessions.middleware import ( + mark_user_change_as_expected, + EmailChangeSessionInvalidationMiddleware +) from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_api import accounts from openedx.core.djangoapps.user_authn.config.waffle import ENABLE_LOGIN_USING_THIRDPARTY_AUTH_ONLY @@ -315,6 +318,10 @@ def _handle_successful_authentication_and_login(user, request): request.session.set_expiry(604800 * 4) log.debug("Setting user session expiry to 4 weeks") + if settings.ENFORCE_SESSION_EMAIL_MATCH: + # Store the user's email for session consistency (used by EmailChangeSessionInvalidationMiddleware) + EmailChangeSessionInvalidationMiddleware.register_email_change(request, user.email) + # .. event_implemented_name: SESSION_LOGIN_COMPLETED SESSION_LOGIN_COMPLETED.send_event( user=UserData( diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_login.py b/openedx/core/djangoapps/user_authn/views/tests/test_login.py index 1e8a4c3ed510..126899ffee70 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_login.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_login.py @@ -47,6 +47,7 @@ @ddt.ddt +@override_settings(ENFORCE_SESSION_EMAIL_MATCH=False) class LoginTest(SiteMixin, CacheIsolationTestCase, OpenEdxEventsTestMixin): """ Test login_user() view @@ -1179,29 +1180,37 @@ def test_invalid_credentials(self): }) self.assertHttpBadRequest(response) - # Invalid email address - response = self.client.post(self.url, { - "email": "invalid@example.com", + @override_settings(ENFORCE_SESSION_EMAIL_MATCH=True) + def test_email_in_session_if_ENFORCE_SESSION_EMAIL_MATCH_is_enabled(self): + # Login and check email in session + data = { + "email": self.EMAIL, "password": self.PASSWORD, - }) - self.assertHttpBadRequest(response) + } - @ddt.data(True, False) - def test_missing_login_params(self, is_api_v1): - email_field_name = "email" if is_api_v1 else "email_or_username" - url = self.url if is_api_v1 else self.url_v2 - # Missing password - response = self.client.post(url, { - email_field_name: self.EMAIL, - }) - self.assertHttpBadRequest(response) + response = self.client.post(self.url, data) + self.assertHttpOK(response) + + # Ensure the login was successful + self.assertEqual(response.status_code, 200) - # Missing email - response = self.client.post(url, { + stored_email = self.client.session.get('email') + assert stored_email == self.EMAIL + + @override_settings(ENFORCE_SESSION_EMAIL_MATCH=False) + def test_email_is_not_set_in_session_if_ENFORCE_SESSION_EMAIL_MATCH_is_disabled(self): + # Login and check email in session + data = { + "email": self.EMAIL, "password": self.PASSWORD, - }) - self.assertHttpBadRequest(response) + } - # Missing both email and password - response = self.client.post(url, {}) - self.assertHttpBadRequest(response) + response = self.client.post(self.url, data) + self.assertHttpOK(response) + + # Ensure the login was successful + self.assertEqual(response.status_code, 200) + + # Verify that the email is not stored in the session + stored_email = self.client.session.get('email') + assert stored_email is None