Merge pull request #15538 from potsui/fix-duplicate-account-crash

Catch exception with duplicate username/email

AUTHORS

@@ -284,3 +284,4 @@ Ivan Ivić <iivic@edx.org>
 Brandon Baker <bcbaker@wesleyan.edu>
 Salah Alomari <salomari@qrf.org>
 Shirley He <she@edx.org>
+Po Tsui <potsui@stanford.edu>

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

openedx/core/djangoapps/user_api/tests/test_views.py

@@ -774,6 +774,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):
     """Tests for the registration end-points of the User API. """
 

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