Merge pull request #19558 from edx/juliasq/return_proper_user_post_retirement

Fix same username w/different case special-case.

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?')

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