Factor create_account param validation into a form

common/djangoapps/student/forms.py

@@ -2,16 +2,23 @@
 Utility functions for validating forms
 """
 from django import forms
+from django.core.exceptions import ValidationError
 from django.contrib.auth.models import User
 from django.contrib.auth.forms import PasswordResetForm
 from django.contrib.auth.hashers import UNUSABLE_PASSWORD
 from django.contrib.auth.tokens import default_token_generator
 
 from django.utils.http import int_to_base36
+from django.utils.translation import ugettext_lazy as _
 from django.template import loader
 
 from django.conf import settings
 from microsite_configuration import microsite
+from util.password_policy_validators import (
+    validate_password_length,
+    validate_password_complexity,
+    validate_password_dictionary,
+)
 
 
 class PasswordResetFormNoActive(PasswordResetForm):
@@ -70,3 +77,160 @@ class PasswordResetFormNoActive(PasswordResetForm):
             subject = subject.replace('\n', '')
             email = loader.render_to_string(email_template_name, context)
             send_mail(subject, email, from_email, [user.email])
+
+
+class TrueField(forms.BooleanField):
+    """
+    A boolean field that only accepts "true" (case-insensitive) as true
+    """
+    def to_python(self, value):
+        # CheckboxInput converts string to bool by case-insensitive match to "true" or "false"
+        if value is True:
+            return value
+        else:
+            return None
+
+
+_USERNAME_TOO_SHORT_MSG = _("Username must be minimum of two characters long")
+_EMAIL_INVALID_MSG = _("A properly formatted e-mail is required")
+_PASSWORD_INVALID_MSG = _("A valid password is required")
+_NAME_TOO_SHORT_MSG = _("Your legal name must be a minimum of two characters long")
+
+
+class AccountCreationForm(forms.Form):
+    """
+    A form to for account creation data. It is currently only used for
+    validation, not rendering.
+    """
+    # TODO: Resolve repetition
+    username = forms.SlugField(
+        min_length=2,
+        max_length=30,
+        error_messages={
+            "required": _USERNAME_TOO_SHORT_MSG,
+            "invalid": _("Username should only consist of A-Z and 0-9, with no spaces."),
+            "min_length": _USERNAME_TOO_SHORT_MSG,
+            "max_length": _("Username cannot be more than %(limit_value)s characters long"),
+        }
+    )
+    email = forms.EmailField(
+        max_length=75,  # Limit per RFCs is 254, but User's email field in django 1.4 only takes 75
+        error_messages={
+            "required": _EMAIL_INVALID_MSG,
+            "invalid": _EMAIL_INVALID_MSG,
+            "max_length": _("Email cannot be more than %(limit_value)s characters long"),
+        }
+    )
+    password = forms.CharField(
+        min_length=2,
+        error_messages={
+            "required": _PASSWORD_INVALID_MSG,
+            "min_length": _PASSWORD_INVALID_MSG,
+        }
+    )
+    name = forms.CharField(
+        min_length=2,
+        error_messages={
+            "required": _NAME_TOO_SHORT_MSG,
+            "min_length": _NAME_TOO_SHORT_MSG,
+        }
+    )
+
+    def __init__(
+            self,
+            data=None,
+            extra_fields=None,
+            extended_profile_fields=None,
+            enforce_username_neq_password=False,
+            enforce_password_policy=False,
+            tos_required=True
+    ):
+        super(AccountCreationForm, self).__init__(data)
+
+        extra_fields = extra_fields or {}
+        self.extended_profile_fields = extended_profile_fields or {}
+        self.enforce_username_neq_password = enforce_username_neq_password
+        self.enforce_password_policy = enforce_password_policy
+        if tos_required:
+            self.fields["terms_of_service"] = TrueField(
+                error_messages={"required": _("You must accept the terms of service.")}
+            )
+
+        # TODO: These messages don't say anything about minimum length
+        error_message_dict = {
+            "level_of_education": _("A level of education is required"),
+            "gender": _("Your gender is required"),
+            "year_of_birth": _("Your year of birth is required"),
+            "mailing_address": _("Your mailing address is required"),
+            "goals": _("A description of your goals is required"),
+            "city": _("A city is required"),
+            "country": _("A country is required")
+        }
+        for field_name, field_value in extra_fields.items():
+            if field_name not in self.fields:
+                if field_name == "honor_code":
+                    if field_value == "required":
+                        self.fields[field_name] = TrueField(
+                            error_messages={
+                                "required": _("To enroll, you must follow the honor code.")
+                            }
+                        )
+                else:
+                    required = field_value == "required"
+                    min_length = 1 if field_name in ("gender", "level_of_education") else 2
+                    error_message = error_message_dict.get(
+                        field_name,
+                        _("You are missing one or more required fields")
+                    )
+                    self.fields[field_name] = forms.CharField(
+                        required=required,
+                        min_length=min_length,
+                        error_messages={
+                            "required": error_message,
+                            "min_length": error_message,
+                        }
+                    )
+
+        for field in self.extended_profile_fields:
+            if field not in self.fields:
+                self.fields[field] = forms.CharField(required=False)
+
+    def clean_password(self):
+        """Enforce password policies (if applicable)"""
+        password = self.cleaned_data["password"]
+        if (
+                self.enforce_username_neq_password and
+                "username" in self.cleaned_data and
+                self.cleaned_data["username"] == password
+        ):
+            raise ValidationError(_("Username and password fields cannot match"))
+        if self.enforce_password_policy:
+            try:
+                validate_password_length(password)
+                validate_password_complexity(password)
+                validate_password_dictionary(password)
+            except ValidationError, err:
+                raise ValidationError(_("Password: ") + "; ".join(err.messages))
+        return password
+
+    def clean_year_of_birth(self):
+        """
+        Parse year_of_birth to an integer, but just use None instead of raising
+        an error if it is malformed
+        """
+        try:
+            year_str = self.cleaned_data["year_of_birth"]
+            return int(year_str) if year_str is not None else None
+        except ValueError:
+            return None
+
+    @property
+    def cleaned_extended_profile(self):
+        """
+        Return a dictionary containing the extended_profile_fields and values
+        """
+        return {
+            key: value
+            for key, value in self.cleaned_data.items()
+            if key in self.extended_profile_fields and value is not None
+        }

common/djangoapps/student/management/commands/create_random_users.py

@@ -8,26 +8,30 @@ from student.models import CourseEnrollment
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from student.forms import AccountCreationForm
 from student.views import _do_create_account
 
 
-def get_random_post_override():
+def make_random_form():
     """
     Generate unique user data for dummy users.
     """
     identification = uuid.uuid4().hex[:8]
-    return {
+    return AccountCreationForm(
+        data={
             'username': 'user_{id}'.format(id=identification),
             'email': 'email_{id}@example.com'.format(id=identification),
             'password': '12345',
             'name': 'User {id}'.format(id=identification),
-    }
+        },
+        tos_required=False
+    )
 
 
 def create(num, course_key):
     """Create num users, enrolling them in course_key if it's not None"""
     for idx in range(num):
-        (user, user_profile, __) = _do_create_account(get_random_post_override())
+        (user, _, _) = _do_create_account(make_random_form())
         if course_key is not None:
             CourseEnrollment.enroll(user, course_key)
 

common/djangoapps/student/management/commands/create_user.py

@@ -8,6 +8,7 @@ from django.utils import translation
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from student.forms import AccountCreationForm
 from student.models import CourseEnrollment, Registration, create_comments_service_user
 from student.views import _do_create_account, AccountValidationError
 from track.management.tracked_command import TrackedCommand
@@ -80,21 +81,22 @@ class Command(TrackedCommand):
             except InvalidKeyError:
                 course = SlashSeparatedCourseKey.from_deprecated_string(options['course'])
 
-        post_data = {
+        form = AccountCreationForm(
+            data={
                 'username': username,
                 'email': options['email'],
                 'password': options['password'],
                 'name': name,
-            'honor_code': u'true',
-            'terms_of_service': u'true',
-        }
+            },
+            tos_required=False
+        )
         # django.utils.translation.get_language() will be used to set the new
         # user's preferred language.  This line ensures that the result will
         # match this installation's default locale.  Otherwise, inside a
         # management command, it will always return "en-us".
         translation.activate(settings.LANGUAGE_CODE)
         try:
-            user, profile, reg = _do_create_account(post_data)
+            user, _, reg = _do_create_account(form)
             if options['staff']:
                 user.is_staff = True
                 user.save()

common/djangoapps/student/tests/test_create_account.py

@@ -93,13 +93,19 @@ class TestCreateAccount(TestCase):
     def test_profile_saved_no_optional_fields(self):
         profile = self.create_account_and_fetch_profile()
         self.assertEqual(profile.name, self.params["name"])
-        self.assertIsNone(profile.level_of_education)
-        self.assertIsNone(profile.gender)
-        self.assertIsNone(profile.mailing_address)
-        self.assertIsNone(profile.city)
+        self.assertEqual(profile.level_of_education, "")
+        self.assertEqual(profile.gender, "")
+        self.assertEqual(profile.mailing_address, "")
+        self.assertEqual(profile.city, "")
         self.assertEqual(profile.country, "")
-        self.assertIsNone(profile.goals)
-        self.assertEqual(profile.meta, "")
+        self.assertEqual(profile.goals, "")
+        self.assertEqual(
+            profile.get_meta(),
+            {
+                "extra1": "",
+                "extra2": "",
+            }
+        )
         self.assertIsNone(profile.year_of_birth)
 
     @unittest.skipUnless(
@@ -267,7 +273,7 @@ class TestCreateAccountValidation(TestCase):
 
         # Missing
         del params["username"]
-        assert_username_error("Error (401 username). E-mail us.")
+        assert_username_error("Username must be minimum of two characters long")
 
         # Empty, too short
         for username in ["", "a"]:
@@ -282,10 +288,6 @@ class TestCreateAccountValidation(TestCase):
         params["username"] = "invalid username"
         assert_username_error("Username should only consist of A-Z and 0-9, with no spaces.")
 
-        # Matching password
-        params["username"] = params["password"] = "test_username_and_password"
-        assert_username_error("Username and password fields cannot match")
-
     def test_email(self):
         params = dict(self.minimal_params)
 
@@ -298,7 +300,7 @@ class TestCreateAccountValidation(TestCase):
 
         # Missing
         del params["email"]
-        assert_email_error("Error (401 email). E-mail us.")
+        assert_email_error("A properly formatted e-mail is required")
 
         # Empty, too short
         for email in ["", "a"]:
@@ -311,7 +313,7 @@ class TestCreateAccountValidation(TestCase):
 
         # Invalid
         params["email"] = "not_an_email_address"
-        assert_email_error("Valid e-mail is required.")
+        assert_email_error("A properly formatted e-mail is required")
 
     def test_password(self):
         params = dict(self.minimal_params)
@@ -325,7 +327,7 @@ class TestCreateAccountValidation(TestCase):
 
         # Missing
         del params["password"]
-        assert_password_error("Error (401 password). E-mail us.")
+        assert_password_error("A valid password is required")
 
         # Empty, too short
         for password in ["", "a"]:
@@ -334,6 +336,10 @@ class TestCreateAccountValidation(TestCase):
 
         # Password policy is tested elsewhere
 
+        # Matching username
+        params["username"] = params["password"] = "test_username_and_password"
+        assert_password_error("Username and password fields cannot match")
+
     def test_name(self):
         params = dict(self.minimal_params)
 
@@ -346,7 +352,7 @@ class TestCreateAccountValidation(TestCase):
 
         # Missing
         del params["name"]
-        assert_name_error("Error (401 name). E-mail us.")
+        assert_name_error("Your legal name must be a minimum of two characters long")
 
         # Empty, too short
         for name in ["", "a"]:
@@ -369,13 +375,20 @@ class TestCreateAccountValidation(TestCase):
             assert_honor_code_error("To enroll, you must follow the honor code.")
 
             # Empty, invalid
-            for honor_code in ["", "false", "True"]:
+            for honor_code in ["", "false", "not_boolean"]:
                 params["honor_code"] = honor_code
                 assert_honor_code_error("To enroll, you must follow the honor code.")
 
+            # True
+            params["honor_code"] = "tRUe"
+            self.assert_success(params)
+
         with override_settings(REGISTRATION_EXTRA_FIELDS={"honor_code": "optional"}):
             # Missing
             del params["honor_code"]
+            # Need to change username/email because user was created above
+            params["username"] = "another_test_username"
+            params["email"] = "another_test_email@example.com"
             self.assert_success(params)
 
     def test_terms_of_service(self):
@@ -393,10 +406,14 @@ class TestCreateAccountValidation(TestCase):
         assert_terms_of_service_error("You must accept the terms of service.")
 
         # Empty, invalid
-        for terms_of_service in ["", "false", "True"]:
+        for terms_of_service in ["", "false", "not_boolean"]:
             params["terms_of_service"] = terms_of_service
             assert_terms_of_service_error("You must accept the terms of service.")
 
+        # True
+        params["terms_of_service"] = "tRUe"
+        self.assert_success(params)
+
     @ddt.data(
         ("level_of_education", 1, "A level of education is required"),
         ("gender", 1, "Your gender is required"),

common/djangoapps/student/views.py

@@ -56,7 +56,7 @@ from student.models import (
     CourseEnrollmentAllowed, UserStanding, LoginFailures,
     create_comments_service_user, PasswordHistory, UserSignupSource,
     DashboardConfiguration, LinkedInAddToProfileConfiguration)
-from student.forms import PasswordResetFormNoActive
+from student.forms import AccountCreationForm, PasswordResetFormNoActive
 
 from verify_student.models import SoftwareSecurePhotoVerification, MidcourseReverificationWindow
 from certificates.models import CertificateStatuses, certificate_status_for_student
@@ -1336,7 +1336,7 @@ def user_signup_handler(sender, **kwargs):  # pylint: disable=unused-argument
             log.info(u'user {} originated from a white labeled "Microsite"'.format(kwargs['instance'].id))
 
 
-def _do_create_account(post_vars, extended_profile=None):
+def _do_create_account(form):
     """
     Given cleaned post variables, create the User and UserProfile objects, as well as the
     registration for this user.
@@ -1345,10 +1345,15 @@ def _do_create_account(post_vars, extended_profile=None):
 
     Note: this function is also used for creating test users.
     """
-    user = User(username=post_vars['username'],
-                email=post_vars['email'],
-                is_active=False)
-    user.set_password(post_vars['password'])
+    if not form.is_valid():
+        raise ValidationError(form.errors)
+
+    user = User(
+        username=form.cleaned_data["username"],
+        email=form.cleaned_data["email"],
+        is_active=False
+    )
+    user.set_password(form.cleaned_data["password"])
     registration = Registration()
 
     # TODO: Rearrange so that if part of the process fails, the whole process fails.
@@ -1357,14 +1362,14 @@ def _do_create_account(post_vars, extended_profile=None):
         user.save()
     except IntegrityError:
         # Figure out the cause of the integrity error
-        if len(User.objects.filter(username=post_vars['username'])) > 0:
+        if len(User.objects.filter(username=user.username)) > 0:
             raise AccountValidationError(
-                _("An account with the Public Username '{username}' already exists.").format(username=post_vars['username']),
+                _("An account with the Public Username '{username}' already exists.").format(username=user.username),
                 field="username"
             )
-        elif len(User.objects.filter(email=post_vars['email'])) > 0:
+        elif len(User.objects.filter(email=user.email)) > 0:
             raise AccountValidationError(
-                _("An account with the Email '{email}' already exists.").format(email=post_vars['email']),
+                _("An account with the Email '{email}' already exists.").format(email=user.email),
                 field="email"
             )
         else:
@@ -1377,25 +1382,17 @@ def _do_create_account(post_vars, extended_profile=None):
 
     registration.register(user)
 
-    profile = UserProfile(user=user)
-    profile.name = post_vars['name']
-    profile.level_of_education = post_vars.get('level_of_education')
-    profile.gender = post_vars.get('gender')
-    profile.mailing_address = post_vars.get('mailing_address')
-    profile.city = post_vars.get('city')
-    profile.country = post_vars.get('country')
-    profile.goals = post_vars.get('goals')
-
-    # add any extended profile information in the denormalized 'meta' field in the profile
+    profile_fields = [
+        "name", "level_of_education", "gender", "mailing_address", "city", "country", "goals",
+        "year_of_birth"
+    ]
+    profile = UserProfile(
+        user=user,
+        **{key: form.cleaned_data.get(key) for key in profile_fields}
+    )
+    extended_profile = form.cleaned_extended_profile
     if extended_profile:
         profile.meta = json.dumps(extended_profile)
-
-    try:
-        profile.year_of_birth = int(post_vars['year_of_birth'])
-    except (ValueError, KeyError):
-        # If they give us garbage, just ignore it instead
-        # of asking them to put an integer.
-        profile.year_of_birth = None
     try:
         profile.save()
     except Exception:  # pylint: disable=broad-except
@@ -1447,19 +1444,15 @@ def create_account(request, post_override=None):  # pylint: disable-msg=too-many
         post_vars.update(dict(email=email, name=name, password=password))
         log.debug(u'In create_account with external_auth: user = %s, email=%s', name, email)
 
-    # Confirm we have a properly formed request
-    for req_field in ['username', 'email', 'password', 'name']:
-        if req_field not in post_vars:
-            js['value'] = _("Error (401 {field}). E-mail us.").format(field=req_field)
-            js['field'] = req_field
-            return JsonResponse(js, status=400)
-
-    if extra_fields.get('honor_code', 'required') == 'required' and \
-            post_vars.get('honor_code', 'false') != u'true':
-        js['value'] = _("To enroll, you must follow the honor code.")
-        js['field'] = 'honor_code'
-        return JsonResponse(js, status=400)
-
+    extra_fields = microsite.get_value(
+        'REGISTRATION_EXTRA_FIELDS',
+        getattr(settings, 'REGISTRATION_EXTRA_FIELDS', {})
+    )
+    extended_profile_fields = microsite.get_value('extended_profile_fields', [])
+    enforce_password_policy = (
+        settings.FEATURES.get("ENFORCE_PASSWORD_POLICY", False) and
+        not do_external_auth
+    )
     # Can't have terms of service for certain SHIB users, like at Stanford
     tos_required = (
         not settings.FEATURES.get("AUTH_USE_SHIB") or
@@ -1470,120 +1463,29 @@ def create_account(request, post_override=None):  # pylint: disable-msg=too-many
         )
     )
 
-    if tos_required:
-        if post_vars.get('terms_of_service', 'false') != u'true':
-            js['value'] = _("You must accept the terms of service.")
-            js['field'] = 'terms_of_service'
-            return JsonResponse(js, status=400)
-
-    # Confirm appropriate fields are there.
-    # TODO: Check e-mail format is correct.
-    # TODO: Confirm e-mail is not from a generic domain (mailinator, etc.)? Not sure if
-    # this is a good idea
-    # TODO: Check password is sane
-
-    required_post_vars = ['username', 'email', 'name', 'password']
-    required_post_vars += [fieldname for fieldname, val in extra_fields.items()
-                           if val == 'required']
-    if tos_required:
-        required_post_vars.append('terms_of_service')
-
-    for field_name in required_post_vars:
-        if field_name in ('gender', 'level_of_education'):
-            min_length = 1
-        else:
-            min_length = 2
-
-        if field_name not in post_vars or len(post_vars[field_name]) < min_length:
-            error_str = {
-                'username': _('Username must be minimum of two characters long'),
-                'email': _('A properly formatted e-mail is required'),
-                'name': _('Your legal name must be a minimum of two characters long'),
-                'password': _('A valid password is required'),
-                'terms_of_service': _('Accepting Terms of Service is required'),
-                'honor_code': _('Agreeing to the Honor Code is required'),
-                'level_of_education': _('A level of education is required'),
-                'gender': _('Your gender is required'),
-                'year_of_birth': _('Your year of birth is required'),
-                'mailing_address': _('Your mailing address is required'),
-                'goals': _('A description of your goals is required'),
-                'city': _('A city is required'),
-                'country': _('A country is required')
-            }
-
-            if field_name in error_str:
-                js['value'] = error_str[field_name]
-            else:
-                js['value'] = _('You are missing one or more required fields')
-
-            js['field'] = field_name
-            return JsonResponse(js, status=400)
-
-        max_length = 75
-        if field_name == 'username':
-            max_length = 30
-
-        if field_name in ('email', 'username') and len(post_vars[field_name]) > max_length:
-            error_str = {
-                'username': _('Username cannot be more than {num} characters long').format(num=max_length),
-                'email': _('Email cannot be more than {num} characters long').format(num=max_length)
-            }
-            js['value'] = error_str[field_name]
-            js['field'] = field_name
-            return JsonResponse(js, status=400)
-
-    try:
-        validate_email(post_vars['email'])
-    except ValidationError:
-        js['value'] = _("Valid e-mail is required.")
-        js['field'] = 'email'
-        return JsonResponse(js, status=400)
-
-    try:
-        validate_slug(post_vars['username'])
-    except ValidationError:
-        js['value'] = _("Username should only consist of A-Z and 0-9, with no spaces.")
-        js['field'] = 'username'
-        return JsonResponse(js, status=400)
-
-    # enforce password complexity as an optional feature
-    # but not if we're doing ext auth b/c those pws never get used and are auto-generated so might not pass validation
-    if settings.FEATURES.get('ENFORCE_PASSWORD_POLICY', False) and not do_external_auth:
-        try:
-            password = post_vars['password']
+    form = AccountCreationForm(
+        data=post_vars,
+        extra_fields=extra_fields,
+        extended_profile_fields=extended_profile_fields,
+        enforce_username_neq_password=True,
+        enforce_password_policy=enforce_password_policy,
+        tos_required=tos_required
+    )
 
-            validate_password_length(password)
-            validate_password_complexity(password)
-            validate_password_dictionary(password)
-        except ValidationError, err:
-            js['value'] = _('Password: ') + '; '.join(err.messages)
-            js['field'] = 'password'
-            return JsonResponse(js, status=400)
+    if not form.is_valid():
+        field, error_list = next(form.errors.iteritems())
+        return JsonResponse(
+            {
+                "success": False,
+                "field": field,
+                "value": error_list[0],
+            },
+            status=400
+        )
 
-    # allow microsites to define 'extended profile fields' which are
-    # captured on user signup (for example via an overriden registration.html)
-    # and then stored in the UserProfile
-    extended_profile_fields = microsite.get_value('extended_profile_fields', [])
-    extended_profile = None
-
-    for field in extended_profile_fields:
-        if field in post_vars:
-            if not extended_profile:
-                extended_profile = {}
-            extended_profile[field] = post_vars[field]
-
-    # Make sure that password and username fields do not match
-    username = post_vars['username']
-    password = post_vars['password']
-    if username == password:
-        js['value'] = _("Username and password fields cannot match")
-        js['field'] = 'username'
-        return JsonResponse(js, status=400)
-
-    # Ok, looks like everything is legit.  Create the account.
     try:
         with transaction.commit_on_success():
-            ret = _do_create_account(post_vars, extended_profile)
+            ret = _do_create_account(form)
     except AccountValidationError as exc:
         return JsonResponse({'success': False, 'value': exc.message, 'field': exc.field}, status=400)
 
@@ -1757,21 +1659,21 @@ def auto_auth(request):
     role_names = [v.strip() for v in request.GET.get('roles', '').split(',') if v.strip()]
     login_when_done = 'no_login' not in request.GET
 
-    # Get or create the user object
-    post_data = {
+    form = AccountCreationForm(
+        data={
             'username': username,
             'email': email,
             'password': password,
             'name': full_name,
-        'honor_code': u'true',
-        'terms_of_service': u'true',
-    }
+        },
+        tos_required=False
+    )
 
     # Attempt to create the account.
     # If successful, this will return a tuple containing
     # the new user object.
     try:
-        user, _profile, reg = _do_create_account(post_data)
+        user, _profile, reg = _do_create_account(form)
     except AccountValidationError:
         # Attempt to retrieve the existing user.
         user = User.objects.get(username=username)