diff --git a/AUTHORS b/AUTHORS index 39a819cf42..f78b199d20 100644 --- a/AUTHORS +++ b/AUTHORS @@ -284,3 +284,4 @@ Ivan Ivić Brandon Baker Salah Alomari Shirley He +Po Tsui diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 1ffff1e39c..a1d24d0b2c 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -1723,6 +1723,11 @@ def _do_create_account(form, custom_form=None): custom_model.save() except IntegrityError: # Figure out the cause of the integrity error + # TODO duplicate email is already handled by form.errors above as a ValidationError. + # The checks for duplicate email/username should occur in the same place with an + # 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 len(User.objects.filter(username=user.username)) > 0: raise AccountValidationError( _("An account with the Public Username '{username}' already exists.").format(username=user.username), @@ -1796,6 +1801,14 @@ def create_account_with_params(request, params): * The user-facing text is rather unfriendly (e.g. "Username must be a minimum of two characters long" rather than "Please use a username of at least two characters"). + * Duplicate email raises a ValidationError (rather than the expected + AccountValidationError). Duplicate username returns an inconsistent + user message (i.e. "An account with the Public Username '{username}' + already exists." rather than "It looks like {username} belongs to an + existing account. Try again with a different username.") The two checks + occur at different places in the code; as a result, registering with + both a duplicate username and email raises only a ValidationError for + email only. """ # Copy params so we can modify it; we can't just do dict(params) because if # params is request.POST, that results in a dict containing lists of values diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index 359987928a..389407bb6b 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/tests/test_views.py @@ -772,6 +772,142 @@ class PasswordResetViewTest(UserAPITestCase): ]) +@ddt.ddt +@skip_unless_lms +class RegistrationViewValidationErrorTest(ThirdPartyAuthTestMixin, UserAPITestCase): + """ + Tests for catching duplicate email and username validation errors within + the registration end-points of the User API. + """ + + maxDiff = None + + USERNAME = "bob" + EMAIL = "bob@example.com" + PASSWORD = "password" + NAME = "Bob Smith" + EDUCATION = "m" + YEAR_OF_BIRTH = "1998" + ADDRESS = "123 Fake Street" + CITY = "Springfield" + COUNTRY = "us" + GOALS = "Learn all the things!" + + def setUp(self): + super(RegistrationViewValidationErrorTest, self).setUp() + self.url = reverse("user_api_registration") + + @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 = [] + # 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) + + # Try to create a second user with the same email address + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": "Someone Else", + "username": "someone_else", + "password": self.PASSWORD, + "honor_code": "true", + }) + self.assertEqual(response.status_code, 400) + response_json = json.loads(response.content) + self.assertEqual( + response_json, + { + "email": [{ + "user_message": ( + "It looks like {} belongs to an existing account. " + "Try again with a different email address." + ).format( + self.EMAIL + ) + }] + } + ) + + @mock.patch('openedx.core.djangoapps.user_api.views.check_account_exists') + def test_register_duplicate_username_account_validation_error(self, dummy_check_account_exists): + dummy_check_account_exists.return_value = [] + # 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) + + # 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, + { + u"username": [{ + u"user_message": ( + u"An account with the Public Username '{}' already exists." + ).format( + self.USERNAME + ) + }] + } + ) + + @mock.patch('openedx.core.djangoapps.user_api.views.check_account_exists') + def test_register_duplicate_username_and_email_validation_errors(self, dummy_check_account_exists): + dummy_check_account_exists.return_value = [] + # 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) + + # Try to create a second user with the same username + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": "Someone Else", + "username": self.USERNAME, + "password": self.PASSWORD, + "honor_code": "true", + }) + self.assertEqual(response.status_code, 400) + response_json = json.loads(response.content) + self.assertEqual( + response_json, + { + "email": [{ + "user_message": ( + "It looks like {} belongs to an existing account. " + "Try again with a different email address." + ).format( + self.EMAIL + ) + }] + } + ) + + @ddt.ddt @skip_unless_lms class RegistrationViewTest(ThirdPartyAuthTestMixin, UserAPITestCase): diff --git a/openedx/core/djangoapps/user_api/views.py b/openedx/core/djangoapps/user_api/views.py index 6d644e1a30..487a31477b 100644 --- a/openedx/core/djangoapps/user_api/views.py +++ b/openedx/core/djangoapps/user_api/views.py @@ -28,7 +28,7 @@ from openedx.core.lib.api.permissions import ApiKeyHeaderPermission from openedx.features.enterprise_support.api import enterprise_customer_for_request from student.cookies import set_logged_in_cookies from student.forms import get_registration_extension_form -from student.views import create_account_with_params +from student.views import create_account_with_params, AccountValidationError from util.json_request import JsonResponse from .accounts import ( @@ -371,6 +371,11 @@ class RegistrationView(APIView): try: user = create_account_with_params(request, data) + except AccountValidationError as err: + errors = { + err.field: [{"user_message": err.message}] + } + return JsonResponse(errors, status=409) except ValidationError as err: # Should only get non-field errors from this function assert NON_FIELD_ERRORS not in err.message_dict