Skip to content

Commit

Permalink
feat: logout other sessions on email change (openedx#33846)
Browse files Browse the repository at this point in the history
* feat: logout other sessions on email change

* fix: updated the approach for session invalidation

* fix: update and add tests

* fix: update tests with descriptive comments

* feat: add integration tests

* fix: store email in session update

* fix: add setting for tests

* fix: fix tests

* feat: Upgrade Python dependency edx-drf-extensions (openedx#34135)

Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

Co-authored-by: syedsajjadkazmii <[email protected]>

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: syedsajjadkazmii <[email protected]>
  • Loading branch information
3 people authored Jan 29, 2024
1 parent 7f850d6 commit cb2a34e
Show file tree
Hide file tree
Showing 14 changed files with 893 additions and 24 deletions.
9 changes: 7 additions & 2 deletions common/djangoapps/student/signals/receivers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 EmailChangeMiddleware
from openedx.features.name_affirmation_api.utils import is_name_affirmation_installed

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -105,8 +106,12 @@ 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. """

# Store the user's email for session consistency (used by EmailChangeMiddleware)
EmailChangeMiddleware.register_email_change(request, user.email)

email = user.email
user_id = user.id
attributes = [{'email': email, 'external_id': user_id}]
Expand Down
14 changes: 11 additions & 3 deletions common/djangoapps/student/tests/test_receivers.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,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

Expand Down Expand Up @@ -75,10 +75,18 @@ 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.
"""
user = UserFactory(email='[email protected]', 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 = '[email protected]'
user.save()

USER_EMAIL_CHANGED.send(sender=None, user=user, request=request)

assert mock_get_braze_client.called
assert request.session.get('email', None) == user.email
2 changes: 1 addition & 1 deletion common/djangoapps/student/views/management.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down
6 changes: 3 additions & 3 deletions lms/djangoapps/certificates/apis/v0/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ def test_no_certificate(self):
def test_query_counts(self):
# Test student with no certificates
student_no_cert = UserFactory.create(password=self.user_password)
with self.assertNumQueries(17, table_ignorelist=WAFFLE_TABLES):
with self.assertNumQueries(21, table_ignorelist=WAFFLE_TABLES):
resp = self.get_response(
AuthType.jwt,
requesting_user=self.global_staff,
Expand All @@ -319,7 +319,7 @@ def test_query_counts(self):
assert len(resp.data) == 0

# Test student with 1 certificate
with self.assertNumQueries(12, table_ignorelist=WAFFLE_TABLES):
with self.assertNumQueries(13, table_ignorelist=WAFFLE_TABLES):
resp = self.get_response(
AuthType.jwt,
requesting_user=self.global_staff,
Expand Down Expand Up @@ -359,7 +359,7 @@ def test_query_counts(self):
download_url='www.google.com',
grade="0.88",
)
with self.assertNumQueries(12, table_ignorelist=WAFFLE_TABLES):
with self.assertNumQueries(13, table_ignorelist=WAFFLE_TABLES):
resp = self.get_response(
AuthType.jwt,
requesting_user=self.global_staff,
Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/course_api/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ def test_too_many_courses(self):
self.setup_user(self.audit_user)

# These query counts were found empirically
query_counts = [50, 46, 46, 46, 46, 46, 46, 46, 46, 46, 16]
query_counts = [53, 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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ def test_get_masqueraded_user(self):
assert self.client.get(self.url).data['username'] == self.user.username

def test_get_unknown_course(self):
self.client.logout()
url = reverse('course-home:course-metadata', args=['course-v1:unknown+course+2T2020'])
# Django TestCase wraps every test in a transaction, so we must specifically wrap this when we expect an error
with transaction.atomic():
Expand Down
17 changes: 17 additions & 0 deletions lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -2238,6 +2238,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.EmailChangeMiddleware',

'common.djangoapps.student.middleware.UserStandingMiddleware',
'openedx.core.djangoapps.contentserver.middleware.StaticContentServer',

Expand Down Expand Up @@ -5041,6 +5044,20 @@ 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 EmailChangeMiddleware ###############

# .. 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. This toggle is just being used for rollout.
# .. toggle_use_cases: temporary
# .. toggle_creation_date: 2023-12-07
# .. toggle_target_removal_date: 2024-04-01
# .. toggle_tickets: https://2u-internal.atlassian.net/browse/VAN-1797
ENFORCE_SESSION_EMAIL_MATCH = False

LEARNER_HOME_MFE_REDIRECT_PERCENTAGE = 0

############### Settings for the ace_common plugin #################
Expand Down
88 changes: 87 additions & 1 deletion openedx/core/djangoapps/safe_sessions/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@
from edx_django_utils.monitoring import set_custom_attribute
from edx_toggles.toggles import SettingToggle

from openedx.core.djangoapps.user_authn.cookies import delete_logged_in_cookies
from openedx.core.djangoapps.user_authn.cookies import delete_logged_in_cookies, set_logged_in_cookies
from openedx.core.lib.mobile_utils import is_request_from_mobile_app

# .. toggle_name: LOG_REQUEST_USER_CHANGES
Expand Down Expand Up @@ -768,6 +768,92 @@ def _get_encrypted_request_headers(request):
return encrypt_for_log(str(request.headers), getattr(settings, 'SAFE_SESSIONS_DEBUG_PUBLIC_KEY', None))


class EmailChangeMiddleware(MiddlewareMixin):
"""
Middleware responsible for performing the following
jobs on detecting an email change
1) It will update the session's email and update the JWT cookie
to match the new email.
2) It will invalidate any future session on other browsers where
the user's email does not match its session 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 and JWT cookies 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 request.user.is_authenticated:
user_session_email = request.session.get('email', None)
are_emails_mismatched = user_session_email is not None and request.user.email != user_session_email
EmailChangeMiddleware._set_session_email_match_custom_attributes(are_emails_mismatched)
if settings.ENFORCE_SESSION_EMAIL_MATCH and are_emails_mismatched:
# Flush the session and mark cookies for deletion.
log.info(
f'EmailChangeMiddleware invalidating session for user: {request.user.id} due to email mismatch.'
)
request.session.flush()
request.user = AnonymousUser()
_mark_cookie_for_deletion(request)

def process_response(self, request, response):
"""
1. Update the logged-in cookies if the email change was requested
2. Store user's email in session if not already
"""
if request.user.is_authenticated:
if request.session.get('email', None) is None:
# .. custom_attribute_name: session_with_no_email_found
# .. custom_attribute_description: Indicates that user's email was not
# yet stored in the user's session.
set_custom_attribute('session_with_no_email_found', True)
request.session['email'] = request.user.email

if request_cache.get_cached_response('email_change_requested').is_found:
# Update the JWT cookies with new user email
response = set_logged_in_cookies(request, response, request.user)

return response

@staticmethod
def register_email_change(request, email):
"""
Stores the fact that an email change happened.
1. Sets the email in session for later comparison.
2. Sets a request level variable to mark that the user email change was requested.
"""
request.session['email'] = email
request_cache.set('email_change_requested', True)

@staticmethod
def _set_session_email_match_custom_attributes(are_emails_mismatched):
"""
Sets custom attributes 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_mismatch', are_emails_mismatched)

# .. custom_attribute_name: is_enforce_session_email_match_enabled
# .. custom_attribute_description: Indicates whether session email match was enforced.
# When enforced/enabled, it invalidates sessions in other browsers upon email change,
# while preserving the session validity in the browser where the email change occurs.
set_custom_attribute('is_enforce_session_email_match_enabled', settings.ENFORCE_SESSION_EMAIL_MATCH)


def obscure_token(value: Union[str, None]) -> Union[str, None]:
"""
Return a short string that can be used to detect other occurrences
Expand Down
Loading

0 comments on commit cb2a34e

Please sign in to comment.