Andal.LND/edx-platform
4
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Merge pull request #19045 from edx/jeskew/PLAT_2146_again_add_username_retirement_check

Browse Source 
Check for retired usernames as well as existing ones in validation.
This commit is contained in:
J Eskew
2018-10-03 14:57:50 -04:00
committed by GitHub
parent 0daf16852d aacdce17d4
commit d447c6c9b5
5 changed files with 96 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
common/djangoapps/student/helpers.py
View File

@@ -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
)
@@ -641,7 +642,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"

7
common/djangoapps/student/models.py
View File

@@ -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

85
common/djangoapps/student/tests/test_retirement.py
View File

@@ -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):
"""

4
openedx/core/djangoapps/user_api/accounts/api.py
View File

@@ -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))

45
openedx/core/djangoapps/user_api/tests/test_views.py
View File

@@ -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 = []