From aacdce17d4894170979210ac3935ca838d1b0e7c Mon Sep 17 00:00:00 2001 From: John Eskew Date: Mon, 24 Sep 2018 19:44:59 -0400 Subject: [PATCH] Check for retired usernames as well as existing ones in validation. --- common/djangoapps/student/helpers.py | 5 +- common/djangoapps/student/models.py | 7 ++ .../student/tests/test_retirement.py | 85 +++++++++---------- .../core/djangoapps/user_api/accounts/api.py | 4 +- .../djangoapps/user_api/tests/test_views.py | 45 +++++++++- 5 files changed, 96 insertions(+), 50 deletions(-) diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index 0a7e62898b..c76c3a1bd5 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -44,7 +44,8 @@ from student.models import ( UserAttribute, UserProfile, unique_id_for_user, - email_exists_or_retired + email_exists_or_retired, + username_exists_or_retired ) @@ -628,7 +629,7 @@ def do_create_account(form, custom_form=None): # AccountValidationError and a consistent user message returned (i.e. both should # return "It looks like {username} belongs to an existing account. Try again with a # different username.") - if User.objects.filter(username=user.username): + if username_exists_or_retired(user.username): raise AccountValidationError( USERNAME_EXISTS_MSG_FMT.format(username=proposed_username), field="username" diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 4c9b540aae..2906d2fd66 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -233,6 +233,13 @@ def is_username_retired(username): raise +def username_exists_or_retired(username): + """ + Check a username for existence -or- retirement against the User model. + """ + return User.objects.filter(username=username).exists() or is_username_retired(username) + + def is_email_retired(email): """ Checks to see if the given email has been previously retired diff --git a/common/djangoapps/student/tests/test_retirement.py b/common/djangoapps/student/tests/test_retirement.py index d1dc5ffd7a..be21062b74 100644 --- a/common/djangoapps/student/tests/test_retirement.py +++ b/common/djangoapps/student/tests/test_retirement.py @@ -74,13 +74,12 @@ def check_email_against_fmt(hashed_email): assert hashed_email.endswith(settings.RETIRED_EMAIL_DOMAIN) -def test_get_retired_username(): +def test_get_retired_username(retirement_user): """ Basic testing of getting retired usernames. The hasher is opaque to us, we just care that it's succeeding and using our format. """ - user = UserFactory() - hashed_username = get_retired_username_by_username(user.username) + hashed_username = get_retired_username_by_username(retirement_user.username) check_username_against_fmt(hashed_username) @@ -94,13 +93,12 @@ def test_get_retired_username_status_exists(retirement_user, retirement_status): assert retirement_status.retired_username == hashed_username -def test_get_all_retired_usernames_by_username(): +def test_get_all_retired_usernames_by_username(retirement_user): """ Check that all salts are used for this method and return expected formats. """ - user = UserFactory() - hashed_usernames = list(get_all_retired_usernames_by_username(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: @@ -110,58 +108,53 @@ def test_get_all_retired_usernames_by_username(): assert len(hashed_usernames) == len(set(hashed_usernames)) -def test_is_username_retired_is_retired(): +def test_is_username_retired_is_retired(retirement_user): """ Check functionality of is_username_retired when username is retired """ - user = UserFactory() - original_username = user.username - retired_username = get_retired_username_by_username(user.username) + original_username = retirement_user.username + retired_username = get_retired_username_by_username(retirement_user.username) # Fake username retirement. - user.username = retired_username - user.save() + retirement_user.username = retired_username + retirement_user.save() assert is_username_retired(original_username) -def test_is_username_retired_not_retired(): +def test_is_username_retired_not_retired(retirement_user): """ Check functionality of is_username_retired when username is not retired """ - user = UserFactory() - assert not is_username_retired(user.username) + assert not is_username_retired(retirement_user.username) -def test_is_email_retired_is_retired(): +def test_is_email_retired_is_retired(retirement_user): """ Check functionality of is_email_retired when email is retired """ - user = UserFactory() - original_email = user.email - retired_email = get_retired_email_by_email(user.email) + original_email = retirement_user.email + retired_email = get_retired_email_by_email(retirement_user.email) # Fake email retirement. - user.email = retired_email - user.save() + retirement_user.email = retired_email + retirement_user.save() assert is_email_retired(original_email) -def test_is_email_retired_not_retired(): +def test_is_email_retired_not_retired(retirement_user): """ Check functionality of is_email_retired when email is not retired """ - user = UserFactory() - assert not is_email_retired(user.email) + assert not is_email_retired(retirement_user.email) -def test_get_retired_email(): +def test_get_retired_email(retirement_user): """ Basic testing of retired emails. """ - user = UserFactory() - hashed_email = get_retired_email_by_email(user.email) + hashed_email = get_retired_email_by_email(retirement_user.email) check_email_against_fmt(hashed_email) @@ -175,13 +168,12 @@ def test_get_retired_email_status_exists(retirement_user, retirement_status): # assert retirement_status.retired_email == hashed_email -def test_get_all_retired_email_by_email(): +def test_get_all_retired_email_by_email(retirement_user): """ Check that all salts are used for this method and return expected formats. """ - user = UserFactory() - hashed_emails = list(get_all_retired_emails_by_email(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: @@ -191,32 +183,30 @@ def test_get_all_retired_email_by_email(): assert len(hashed_emails) == len(set(hashed_emails)) -def test_get_potentially_retired_user_username_match(): +def test_get_potentially_retired_user_username_match(retirement_user): """ Check that we can pass in an un-retired username and get the user-to-be-retired back. """ - user = UserFactory() - hashed_username = get_retired_username_by_username(user.username) - assert get_potentially_retired_user_by_username_and_hash(user.username, hashed_username) == user + hashed_username = get_retired_username_by_username(retirement_user.username) + assert get_potentially_retired_user_by_username_and_hash(retirement_user.username, hashed_username) == retirement_user -def test_get_potentially_retired_user_hashed_match(): +def test_get_potentially_retired_user_hashed_match(retirement_user): """ Check that we can pass in a hashed username and get the user-to-be-retired back. """ - user = UserFactory() - orig_username = user.username + orig_username = retirement_user.username hashed_username = get_retired_username_by_username(orig_username) # Fake username retirement. - user.username = hashed_username - user.save() + retirement_user.username = hashed_username + retirement_user.save() # Check to find the user by original username should fail, # 2nd check by hashed username should succeed. - assert get_potentially_retired_user_by_username_and_hash(orig_username, hashed_username) == user + assert get_potentially_retired_user_by_username_and_hash(orig_username, hashed_username) == retirement_user def test_get_potentially_retired_user_does_not_exist(): @@ -248,6 +238,11 @@ class TestRegisterRetiredUsername(TestCase): """ Tests to ensure that retired usernames can no longer be used in registering new accounts. """ + # The returned message here varies depending on whether a ValidationError -or- + # an AccountValidationError occurs. + INVALID_ACCT_ERR_MSG = ('An account with the Public Username', 'already exists.') + INVALID_ERR_MSG = ('It looks like', 'belongs to an existing account. Try again with a different username.') + def setUp(self): super(TestRegisterRetiredUsername, self).setUp() self.url = reverse('user_api_registration') @@ -260,16 +255,16 @@ class TestRegisterRetiredUsername(TestCase): 'honor_code': 'true', } - def _validate_exiting_username_response(self, orig_username, response): + def _validate_exiting_username_response(self, orig_username, response, start_msg=INVALID_ACCT_ERR_MSG[0], end_msg=INVALID_ACCT_ERR_MSG[1]): """ - Validates a response stating that a username already exists. + Validates a response stating that a username already exists -or- is invalid. """ assert response.status_code == 409 obj = json.loads(response.content) username_msg = obj['username'][0]['user_message'] - assert username_msg.startswith('An account with the Public Username') - assert username_msg.endswith('already exists.') + assert username_msg.startswith(start_msg) + assert username_msg.endswith(end_msg) assert orig_username in username_msg def test_retired_username(self): @@ -286,7 +281,7 @@ class TestRegisterRetiredUsername(TestCase): # Attempt to create another account with the same username that's been retired. self.url_params['username'] = orig_username response = self.client.post(self.url, self.url_params) - self._validate_exiting_username_response(orig_username, response) + self._validate_exiting_username_response(orig_username, response, self.INVALID_ERR_MSG[0], self.INVALID_ERR_MSG[1]) def test_username_close_to_retired_format_active(self): """ diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index da2947bff4..6ffadb263f 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -14,7 +14,7 @@ from django.http import HttpResponseForbidden from openedx.core.djangoapps.theming.helpers import get_current_request from six import text_type -from student.models import User, UserProfile, Registration, email_exists_or_retired +from student.models import User, UserProfile, Registration, email_exists_or_retired, username_exists_or_retired from student import forms as student_forms from student import views as student_views from util.model_utils import emit_setting_changed_event @@ -695,7 +695,7 @@ def _validate_username_doesnt_exist(username): :return: None :raises: errors.AccountUsernameAlreadyExists """ - if username is not None and User.objects.filter(username=username).exists(): + if username is not None and username_exists_or_retired(username): raise errors.AccountUsernameAlreadyExists(_(accounts.USERNAME_CONFLICT_MSG).format(username=username)) diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index 9f48b0a6d3..97002dbfa4 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/tests/test_views.py @@ -28,7 +28,7 @@ from openedx.core.lib.time_zone_utils import get_display_time_zone from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms from student.tests.factories import UserFactory -from student.models import get_retired_email_by_email +from student.models import get_retired_email_by_email, get_retired_username_by_username from third_party_auth.tests.testutil import simulate_running_pipeline, ThirdPartyAuthTestMixin from third_party_auth.tests.utils import ( ThirdPartyOAuthTestMixin, ThirdPartyOAuthTestMixinFacebook, ThirdPartyOAuthTestMixinGoogle @@ -811,6 +811,7 @@ class RegistrationViewValidationErrorTest(ThirdPartyAuthTestMixin, UserAPITestCa """ user = User.objects.get(username=self.USERNAME) UserRetirementStatus.create_retirement(user) + user.username = get_retired_username_by_username(user.username) user.email = get_retired_email_by_email(user.email) user.set_unusable_password() user.save() @@ -897,6 +898,48 @@ class RegistrationViewValidationErrorTest(ThirdPartyAuthTestMixin, UserAPITestCa } ) + def test_register_duplicate_retired_username_account_validation_error(self): + # Register the first user + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + "honor_code": "true", + }) + self.assertHttpOK(response) + + # Initiate retirement for the above user. + self._retireRequestUser() + + with mock.patch('openedx.core.djangoapps.user_authn.views.register.do_create_account') as dummy_do_create_acct: + # do_create_account should *not* be called - the duplicate retired username + # should be detected by check_account_exists before account creation is called. + dummy_do_create_acct.side_effect = Exception('do_create_account should *not* have been called!') + # Try to create a second user with the same username. + response = self.client.post(self.url, { + "email": "someone+else@example.com", + "name": "Someone Else", + "username": self.USERNAME, + "password": self.PASSWORD, + "honor_code": "true", + }) + self.assertEqual(response.status_code, 409) + response_json = json.loads(response.content) + self.assertEqual( + response_json, + { + "username": [{ + "user_message": ( + "It looks like {} belongs to an existing account. " + "Try again with a different username." + ).format( + self.USERNAME + ) + }] + } + ) + @mock.patch('openedx.core.djangoapps.user_api.views.check_account_exists') def test_register_duplicate_email_validation_error(self, dummy_check_account_exists): dummy_check_account_exists.return_value = []