diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index e3f3672dd0..0142b7cb41 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -291,7 +291,7 @@ def get_retired_email_by_email(email): return user_util.get_retired_email(email, settings.RETIRED_USER_SALTS, settings.RETIRED_EMAIL_FMT) -def get_all_retired_usernames_by_username(username): +def _get_all_retired_usernames_by_username(username): """ Returns a generator of "retired usernames", one hashed with each configured salt. Used for finding out if the given username has @@ -300,7 +300,7 @@ def get_all_retired_usernames_by_username(username): return user_util.get_all_retired_usernames(username, settings.RETIRED_USER_SALTS, settings.RETIRED_USERNAME_FMT) -def get_all_retired_emails_by_email(email): +def _get_all_retired_emails_by_email(email): """ Returns a generator of "retired emails", one hashed with each configured salt. Used for finding out if the given email has @@ -315,7 +315,7 @@ def get_potentially_retired_user_by_username(username): does not exist, then any hashed username salted with the historical salts. """ - locally_hashed_usernames = list(get_all_retired_usernames_by_username(username)) + locally_hashed_usernames = list(_get_all_retired_usernames_by_username(username)) locally_hashed_usernames.append(username) potential_users = User.objects.filter(username__in=locally_hashed_usernames) @@ -329,11 +329,22 @@ def get_potentially_retired_user_by_username(username): if not potential_users: raise User.DoesNotExist() - # If there are 2, one of two things should be true: - # - The user we want is un-retired and has the same case-match username - # - Or retired one was the case-match + # For a brief period, users were able to retire accounts and make another account with + # the same differently-cased username, like "testuser" and "TestUser". + # If there are two users found, return the one that's the *actual* case-matching username, + # whether retired or not. if len(potential_users) == 2: - return potential_users[0] if potential_users[0].username == username else potential_users[1] + # Figure out which user has been retired. + if potential_users[0].username.startswith(settings.RETIRED_USERNAME_PREFIX): + retired = potential_users[0] + active = potential_users[1] + else: + retired = potential_users[1] + active = potential_users[0] + + # If the active (non-retired) user's username doesn't *exactly* match (including case), + # then the retired account must be the one that exactly matches. + return active if active.username == username else retired # We should have, at most, a retired username and an active one with a username # differing only by case. If there are more we need to disambiguate them by hand. @@ -349,7 +360,7 @@ def get_potentially_retired_user_by_username_and_hash(username, hashed_username) does not exist, the any hashed username salted with the historical salts. """ - locally_hashed_usernames = list(get_all_retired_usernames_by_username(username)) + locally_hashed_usernames = list(_get_all_retired_usernames_by_username(username)) if hashed_username not in locally_hashed_usernames: raise Exception('Mismatched hashed_username, bad salt?') diff --git a/common/djangoapps/student/tests/test_retirement.py b/common/djangoapps/student/tests/test_retirement.py index be21062b74..e9b597625e 100644 --- a/common/djangoapps/student/tests/test_retirement.py +++ b/common/djangoapps/student/tests/test_retirement.py @@ -12,8 +12,9 @@ from django.test import TestCase import pytest from student.models import ( - get_all_retired_emails_by_email, - get_all_retired_usernames_by_username, + _get_all_retired_emails_by_email, + _get_all_retired_usernames_by_username, + get_potentially_retired_user_by_username, get_potentially_retired_user_by_username_and_hash, get_retired_email_by_email, get_retired_username_by_username, @@ -57,6 +58,17 @@ def retirement_status(retirement_user): # pylint: disable=redefined-outer-name return status +@pytest.fixture +def two_users_same_username_different_case(retirement_status): + user1 = UserFactory.create(username='TestUser') + user2 = UserFactory.create(username='testuser') + UserRetirementStatus = apps.get_model('user_api', 'UserRetirementStatus') + status = UserRetirementStatus.create_retirement(user1) + user1.username = status.retired_username + user1.save() + return status, user1, user2 + + def check_username_against_fmt(hashed_username): """ Checks that the given username is formatted correctly using our settings. @@ -98,7 +110,7 @@ def test_get_all_retired_usernames_by_username(retirement_user): Check that all salts are used for this method and return expected formats. """ - hashed_usernames = list(get_all_retired_usernames_by_username(retirement_user.username)) + hashed_usernames = list(_get_all_retired_usernames_by_username(retirement_user.username)) assert len(hashed_usernames) == len(settings.RETIRED_USER_SALTS) for hashed_username in hashed_usernames: @@ -173,7 +185,7 @@ def test_get_all_retired_email_by_email(retirement_user): Check that all salts are used for this method and return expected formats. """ - hashed_emails = list(get_all_retired_emails_by_email(retirement_user.email)) + hashed_emails = list(_get_all_retired_emails_by_email(retirement_user.email)) assert len(hashed_emails) == len(settings.RETIRED_USER_SALTS) for hashed_email in hashed_emails: @@ -183,6 +195,17 @@ def test_get_all_retired_email_by_email(retirement_user): assert len(hashed_emails) == len(set(hashed_emails)) +def test_get_correct_user_varying_by_case_only(two_users_same_username_different_case): + """ + Check that two users - one retired, one active - with the same username except for case can be found. + """ + retired_status, retired_user, active_user = two_users_same_username_different_case + first_user = get_potentially_retired_user_by_username(retired_status.original_username) + second_user = get_potentially_retired_user_by_username(active_user.username) + assert first_user.username != second_user.username + assert second_user.username == active_user.username + + def test_get_potentially_retired_user_username_match(retirement_user): """ Check that we can pass in an un-retired username and get the