From 6e617fe2a6397c11ad2cc8c1f89e170c306ccbb3 Mon Sep 17 00:00:00 2001 From: Nishant Karandikar Date: Fri, 10 Feb 2017 13:48:26 -0800 Subject: [PATCH 1/5] Catch exception with duplicate username/email Previously, there was no catch for the AccountValidationError exception raised by the account creation function. If, for some reason, the user made it past the first check for a duplicate username/email, then the exception was raised, uncaught, and crashed the server. --- openedx/core/djangoapps/user_api/views.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/user_api/views.py b/openedx/core/djangoapps/user_api/views.py index 5ed7f19b72..a2410df245 100644 --- a/openedx/core/djangoapps/user_api/views.py +++ b/openedx/core/djangoapps/user_api/views.py @@ -25,7 +25,7 @@ from openedx.core.lib.api.authentication import SessionAuthenticationAllowInacti from openedx.core.lib.api.permissions import ApiKeyHeaderPermission 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 ( @@ -364,6 +364,9 @@ 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 From a807aa041ebf29b6b7de9f0906b2d49efce9a825 Mon Sep 17 00:00:00 2001 From: Po Tsui Date: Tue, 11 Jul 2017 16:28:16 -0700 Subject: [PATCH 2/5] Add myself to authors --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index 0cc6414431..3ac25ff370 100644 --- a/AUTHORS +++ b/AUTHORS @@ -282,3 +282,4 @@ Jeff LaJoie Ivan Ivić Brandon Baker Shirley He +Po Tsui From dd5e8d072c49c4b202b787f45aa911047347edc2 Mon Sep 17 00:00:00 2001 From: Po Tsui Date: Tue, 11 Jul 2017 17:47:40 -0700 Subject: [PATCH 3/5] Fix PEP8 issues --- openedx/core/djangoapps/user_api/views.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/user_api/views.py b/openedx/core/djangoapps/user_api/views.py index a2410df245..f31b798fea 100644 --- a/openedx/core/djangoapps/user_api/views.py +++ b/openedx/core/djangoapps/user_api/views.py @@ -365,7 +365,9 @@ class RegistrationView(APIView): try: user = create_account_with_params(request, data) except AccountValidationError as err: - errors = { err.field: [{"user_message": err.message}] } + 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 From af96ad260f1467715f02f808f66036c0ef774b23 Mon Sep 17 00:00:00 2001 From: Po Tsui Date: Wed, 12 Jul 2017 17:03:20 -0700 Subject: [PATCH 4/5] Add tests for duplicate email/username error catching Test all errors raised by account creation function as a result of duplicate email/username will be caught, if the user somehow managed to pass the first check, `check_account_exists`. --- .../djangoapps/user_api/tests/test_views.py | 136 ++++++++++++++++++ 1 file changed, 136 insertions(+) diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index 2d0d527d28..d07e98ff80 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): From 52197fba88c122eae2cfc4c062a12cda671e6959 Mon Sep 17 00:00:00 2001 From: Po Tsui Date: Thu, 13 Jul 2017 11:08:15 -0700 Subject: [PATCH 5/5] Add TODO for duplicate email/username inconsistency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function `create_account_with_params` calls `_do_create_account`, which exhibits some discrepant behavior with throwing errors when handling duplicate email and/or username. A duplicate email raises a `ValidationError` (rather than the expected `AccountValidationError`) from the first part of `_do_create_account`, when errors from `form` (the `AccountCreationForm`) are raised. A duplicate username raises the expected `AccountValidationError`, but from a later part of `_do_create_account`. As a result, registering with both duplicate username and email raises a `ValidationError` for email only. The user message for username is “An account with the Public Username '{username}' already exists.” which differs from that of email, “It looks like {email} belongs to an existing account. Try again with a different email." The latter is more consistent with other user messages. --- common/djangoapps/student/views.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 56b98ebf73..4519a42801 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -1728,6 +1728,11 @@ def _do_create_account(form, custom_form=None, site=None): UserAttribute.set_user_attribute(user, 'created_on_site', site.domain) 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), @@ -1794,6 +1799,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