diff --git a/common/djangoapps/student/forms.py b/common/djangoapps/student/forms.py index 48c956669b..7afbb383d6 100644 --- a/common/djangoapps/student/forms.py +++ b/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 + } diff --git a/common/djangoapps/student/management/commands/create_random_users.py b/common/djangoapps/student/management/commands/create_random_users.py index 088d4484ab..ba44c58020 100644 --- a/common/djangoapps/student/management/commands/create_random_users.py +++ b/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 { - 'username': 'user_{id}'.format(id=identification), - 'email': 'email_{id}@example.com'.format(id=identification), - 'password': '12345', - 'name': 'User {id}'.format(id=identification), - } + 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) diff --git a/common/djangoapps/student/management/commands/create_user.py b/common/djangoapps/student/management/commands/create_user.py index 2da0a6e4fa..4da9519baf 100644 --- a/common/djangoapps/student/management/commands/create_user.py +++ b/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 = { - 'username': username, - 'email': options['email'], - 'password': options['password'], - 'name': name, - 'honor_code': u'true', - 'terms_of_service': u'true', - } + form = AccountCreationForm( + data={ + 'username': username, + 'email': options['email'], + 'password': options['password'], + 'name': name, + }, + 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() diff --git a/common/djangoapps/student/tests/test_create_account.py b/common/djangoapps/student/tests/test_create_account.py index 68f97357d1..3a9a5b0180 100644 --- a/common/djangoapps/student/tests/test_create_account.py +++ b/common/djangoapps/student/tests/test_create_account.py @@ -1,4 +1,5 @@ "Tests for account creation" +import json import ddt import unittest @@ -9,6 +10,7 @@ from django.core.urlresolvers import reverse from django.contrib.auth.models import AnonymousUser from django.utils.importlib import import_module from django.test import TestCase, TransactionTestCase +from django.test.utils import override_settings import mock @@ -24,6 +26,21 @@ TEST_CS_URL = 'https://comments.service.test:123/' @ddt.ddt +@override_settings( + MICROSITE_CONFIGURATION={ + "microsite": { + "domain_prefix": "microsite", + "extended_profile_fields": ["extra1", "extra2"], + } + }, + REGISTRATION_EXTRA_FIELDS={ + key: "optional" + for key in [ + "level_of_education", "gender", "mailing_address", "city", "country", "goals", + "year_of_birth" + ] + } +) class TestCreateAccount(TestCase): "Tests for account creation" @@ -55,6 +72,109 @@ class TestCreateAccount(TestCase): self.assertEqual(response.status_code, 200) self.assertEqual(UserPreference.get_preference(user, LANGUAGE_KEY), lang) + def create_account_and_fetch_profile(self): + """ + Create an account with self.params, assert that the response indicates + success, and return the UserProfile object for the newly created user + """ + response = self.client.post(self.url, self.params, HTTP_HOST="microsite.example.com") + self.assertEqual(response.status_code, 200) + user = User.objects.get(username=self.username) + return user.profile + + def test_marketing_cookie(self): + response = self.client.post(self.url, self.params) + self.assertEqual(response.status_code, 200) + self.assertIn(settings.EDXMKTG_COOKIE_NAME, self.client.cookies) + + @unittest.skipUnless( + "microsite_configuration.middleware.MicrositeMiddleware" in settings.MIDDLEWARE_CLASSES, + "Microsites not implemented in this environment" + ) + def test_profile_saved_no_optional_fields(self): + profile = self.create_account_and_fetch_profile() + self.assertEqual(profile.name, self.params["name"]) + self.assertEqual(profile.level_of_education, "") + self.assertEqual(profile.gender, "") + self.assertEqual(profile.mailing_address, "") + self.assertEqual(profile.city, "") + self.assertEqual(profile.country, "") + self.assertEqual(profile.goals, "") + self.assertEqual( + profile.get_meta(), + { + "extra1": "", + "extra2": "", + } + ) + self.assertIsNone(profile.year_of_birth) + + @unittest.skipUnless( + "microsite_configuration.middleware.MicrositeMiddleware" in settings.MIDDLEWARE_CLASSES, + "Microsites not implemented in this environment" + ) + def test_profile_saved_all_optional_fields(self): + self.params.update({ + "level_of_education": "a", + "gender": "o", + "mailing_address": "123 Example Rd", + "city": "Exampleton", + "country": "US", + "goals": "To test this feature", + "year_of_birth": "2015", + "extra1": "extra_value1", + "extra2": "extra_value2", + }) + profile = self.create_account_and_fetch_profile() + self.assertEqual(profile.level_of_education, "a") + self.assertEqual(profile.gender, "o") + self.assertEqual(profile.mailing_address, "123 Example Rd") + self.assertEqual(profile.city, "Exampleton") + self.assertEqual(profile.country, "US") + self.assertEqual(profile.goals, "To test this feature") + self.assertEqual( + profile.get_meta(), + { + "extra1": "extra_value1", + "extra2": "extra_value2", + } + ) + self.assertEqual(profile.year_of_birth, 2015) + + @unittest.skipUnless( + "microsite_configuration.middleware.MicrositeMiddleware" in settings.MIDDLEWARE_CLASSES, + "Microsites not implemented in this environment" + ) + def test_profile_saved_empty_optional_fields(self): + self.params.update({ + "level_of_education": "", + "gender": "", + "mailing_address": "", + "city": "", + "country": "", + "goals": "", + "year_of_birth": "", + "extra1": "", + "extra2": "", + }) + profile = self.create_account_and_fetch_profile() + self.assertEqual(profile.level_of_education, "") + self.assertEqual(profile.gender, "") + self.assertEqual(profile.mailing_address, "") + self.assertEqual(profile.city, "") + self.assertEqual(profile.country, "") + self.assertEqual(profile.goals, "") + self.assertEqual( + profile.get_meta(), + {"extra1": "", "extra2": ""} + ) + self.assertEqual(profile.year_of_birth, None) + + def test_profile_year_of_birth_non_integer(self): + self.params["year_of_birth"] = "not_an_integer" + profile = self.create_account_and_fetch_profile() + self.assertIsNone(profile.year_of_birth) + def base_extauth_bypass_sending_activation_email(self, bypass_activation_email_for_extauth_setting): """ Tests user creation without sending activation email when @@ -112,6 +232,236 @@ class TestCreateAccount(TestCase): self.assertIsNone(preference) +@ddt.ddt +class TestCreateAccountValidation(TestCase): + """ + Test validation of various parameters in the create_account view + """ + def setUp(self): + super(TestCreateAccountValidation, self).setUp() + self.url = reverse("create_account") + self.minimal_params = { + "username": "test_username", + "email": "test_email@example.com", + "password": "test_password", + "name": "Test Name", + "honor_code": "true", + "terms_of_service": "true", + } + + def assert_success(self, params): + """ + Request account creation with the given params and assert that the + response properly indicates success + """ + response = self.client.post(self.url, params) + self.assertEqual(response.status_code, 200) + response_data = json.loads(response.content) + self.assertTrue(response_data["success"]) + + def assert_error(self, params, expected_field, expected_value): + """ + Request account creation with the given params and assert that the + response properly indicates an error with the given field and value + """ + response = self.client.post(self.url, params) + self.assertEqual(response.status_code, 400) + response_data = json.loads(response.content) + self.assertFalse(response_data["success"]) + self.assertEqual(response_data["field"], expected_field) + self.assertEqual(response_data["value"], expected_value) + + def test_minimal_success(self): + self.assert_success(self.minimal_params) + + def test_username(self): + params = dict(self.minimal_params) + + def assert_username_error(expected_error): + """ + Assert that requesting account creation results in the expected + error + """ + self.assert_error(params, "username", expected_error) + + # Missing + del params["username"] + assert_username_error("Username must be minimum of two characters long") + + # Empty, too short + for username in ["", "a"]: + params["username"] = username + assert_username_error("Username must be minimum of two characters long") + + # Too long + params["username"] = "this_username_has_31_characters" + assert_username_error("Username cannot be more than 30 characters long") + + # Invalid + params["username"] = "invalid username" + assert_username_error("Username should only consist of A-Z and 0-9, with no spaces.") + + def test_email(self): + params = dict(self.minimal_params) + + def assert_email_error(expected_error): + """ + Assert that requesting account creation results in the expected + error + """ + self.assert_error(params, "email", expected_error) + + # Missing + del params["email"] + assert_email_error("A properly formatted e-mail is required") + + # Empty, too short + for email in ["", "a"]: + params["email"] = email + assert_email_error("A properly formatted e-mail is required") + + # Too long + params["email"] = "this_email_address_has_76_characters_in_it_so_it_is_unacceptable@example.com" + assert_email_error("Email cannot be more than 75 characters long") + + # Invalid + params["email"] = "not_an_email_address" + assert_email_error("A properly formatted e-mail is required") + + def test_password(self): + params = dict(self.minimal_params) + + def assert_password_error(expected_error): + """ + Assert that requesting account creation results in the expected + error + """ + self.assert_error(params, "password", expected_error) + + # Missing + del params["password"] + assert_password_error("A valid password is required") + + # Empty, too short + for password in ["", "a"]: + params["password"] = password + assert_password_error("A valid password is required") + + # 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) + + def assert_name_error(expected_error): + """ + Assert that requesting account creation results in the expected + error + """ + self.assert_error(params, "name", expected_error) + + # Missing + del params["name"] + assert_name_error("Your legal name must be a minimum of two characters long") + + # Empty, too short + for name in ["", "a"]: + params["name"] = name + assert_name_error("Your legal name must be a minimum of two characters long") + + def test_honor_code(self): + params = dict(self.minimal_params) + + def assert_honor_code_error(expected_error): + """ + Assert that requesting account creation results in the expected + error + """ + self.assert_error(params, "honor_code", expected_error) + + with override_settings(REGISTRATION_EXTRA_FIELDS={"honor_code": "required"}): + # Missing + del params["honor_code"] + assert_honor_code_error("To enroll, you must follow the honor code.") + + # Empty, invalid + 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): + params = dict(self.minimal_params) + + def assert_terms_of_service_error(expected_error): + """ + Assert that requesting account creation results in the expected + error + """ + self.assert_error(params, "terms_of_service", expected_error) + + # Missing + del params["terms_of_service"] + assert_terms_of_service_error("You must accept the terms of service.") + + # Empty, invalid + 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"), + ("year_of_birth", 2, "Your year of birth is required"), + ("mailing_address", 2, "Your mailing address is required"), + ("goals", 2, "A description of your goals is required"), + ("city", 2, "A city is required"), + ("country", 2, "A country is required"), + ("custom_field", 2, "You are missing one or more required fields") + ) + @ddt.unpack + def test_extra_fields(self, field, min_length, expected_error): + params = dict(self.minimal_params) + + def assert_extra_field_error(): + """ + Assert that requesting account creation results in the expected + error + """ + self.assert_error(params, field, expected_error) + + with override_settings(REGISTRATION_EXTRA_FIELDS={field: "required"}): + # Missing + assert_extra_field_error() + + # Empty + params[field] = "" + assert_extra_field_error() + + # Too short + if min_length > 1: + params[field] = "a" + assert_extra_field_error() + + @mock.patch.dict("student.models.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) @mock.patch("lms.lib.comment_client.User.base_url", TEST_CS_URL) @mock.patch("lms.lib.comment_client.utils.requests.request", return_value=mock.Mock(status_code=200, text='{}')) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 25f32adffa..ee69b8bd10 100644 --- a/common/djangoapps/student/views.py +++ b/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 @@ -1337,7 +1337,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. @@ -1346,10 +1346,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. @@ -1358,14 +1363,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: @@ -1378,25 +1383,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 @@ -1408,15 +1405,23 @@ def _do_create_account(post_vars, extended_profile=None): return (user, profile, registration) -@csrf_exempt -def create_account(request, post_override=None): # pylint: disable-msg=too-many-statements +def create_account_with_params(request, params): """ - JSON call to create new edX account. - Used by form in signup_modal.html, which is included into navigation.html - """ - js = {'success': False} # pylint: disable-msg=invalid-name + Given a request and a dict of parameters (which may or may not have come + from the request), create an account for the requesting user, including + creating a comments service user object and sending an activation email. + This also takes external/third-party auth into account, updates that as + necessary, and authenticates the user for the request's session. - post_vars = post_override if post_override else request.POST + Does not return anything. + + Raises AccountValidationError if an account with the username or email + specified by params already exists, or ValidationError if any of the given + parameters is invalid for any other reason. + """ + # 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 + params = dict(params.items()) # allow for microsites to define their own set of required/optional/hidden fields extra_fields = microsite.get_value( @@ -1425,42 +1430,30 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many ) if third_party_auth.is_enabled() and pipeline.running(request): - post_vars = dict(post_vars.items()) - post_vars.update({'password': pipeline.make_random_password()}) + params["password"] = pipeline.make_random_password() # if doing signup for an external authorization, then get email, password, name from the eamap # don't use the ones from the form, since the user could have hacked those # unless originally we didn't get a valid email or name from the external auth + # TODO: We do not check whether these values meet all necessary criteria, such as email length do_external_auth = 'ExternalAuthMap' in request.session if do_external_auth: eamap = request.session['ExternalAuthMap'] try: validate_email(eamap.external_email) - email = eamap.external_email + params["email"] = eamap.external_email except ValidationError: - email = post_vars.get('email', '') - if eamap.external_name.strip() == '': - name = post_vars.get('name', '') - else: - name = eamap.external_name - password = eamap.internal_password - post_vars = dict(post_vars.items()) - 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) + pass + if eamap.external_name.strip() != '': + params["name"] = eamap.external_name + params["password"] = eamap.internal_password + log.debug(u'In create_account with external_auth: user = %s, email=%s', params["name"], params["email"]) + 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 @@ -1471,122 +1464,17 @@ 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) + form = AccountCreationForm( + data=params, + 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 + ) - # 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'] - - 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) - - # 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) - except AccountValidationError as exc: - return JsonResponse({'success': False, 'value': exc.message, 'field': exc.field}, status=400) + with transaction.commit_on_success(): + ret = _do_create_account(form) (user, profile, registration) = ret @@ -1598,14 +1486,12 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many dog_stats_api.increment("common.student.account_created") - email = post_vars['email'] - # Track the user's registration if settings.FEATURES.get('SEGMENT_IO_LMS') and hasattr(settings, 'SEGMENT_IO_LMS_KEY'): tracking_context = tracker.get_tracker().resolve_context() analytics.identify(user.id, { - 'email': email, - 'username': username, + 'email': user.email, + 'username': user.username, }) # If the user is registering via 3rd party auth, track which provider they use @@ -1620,7 +1506,7 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many "edx.bi.user.account.registered", { 'category': 'conversion', - 'label': request.POST.get('course_id'), + 'label': params.get('course_id'), 'provider': provider_name }, context={ @@ -1633,7 +1519,7 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many create_comments_service_user(user) context = { - 'name': post_vars['name'], + 'name': profile.name, 'key': registration.activation_key, } @@ -1664,16 +1550,11 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many user.email_user(subject, message, from_address) except Exception: # pylint: disable=broad-except log.error(u'Unable to send activation email to user from "%s"', from_address, exc_info=True) - js['value'] = _('Could not send activation e-mail.') - # What is the correct status code to use here? I think it's 500, because - # the problem is on the server's end -- but also, the account was created. - # Seems like the core part of the request was successful. - return JsonResponse(js, status=500) # Immediately after a user creates an account, we log them in. They are only # logged in until they close the browser. They can't log in again until they click # the activation link from the email. - new_user = authenticate(username=post_vars['username'], password=post_vars['password']) + new_user = authenticate(username=user.username, password=params['password']) login(request, new_user) request.session.set_expiry(0) @@ -1686,8 +1567,8 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many eamap.user = new_user eamap.dtsignup = datetime.datetime.now(UTC) eamap.save() - AUDIT_LOG.info(u"User registered with external_auth %s", post_vars['username']) - AUDIT_LOG.info(u'Updated ExternalAuthMap for %s to be %s', post_vars['username'], eamap) + AUDIT_LOG.info(u"User registered with external_auth %s", new_user.username) + AUDIT_LOG.info(u'Updated ExternalAuthMap for %s to be %s', new_user.username, eamap) if settings.FEATURES.get('BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH'): log.info('bypassing activation email') @@ -1695,7 +1576,55 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many new_user.save() AUDIT_LOG.info(u"Login activated on extauth account - {0} ({1})".format(new_user.username, new_user.email)) - dog_stats_api.increment("common.student.account_created") + +def set_marketing_cookie(request, response): + """ + Set the login cookie for the edx marketing site on the given response. Its + expiration will match that of the given request's session. + """ + if request.session.get_expire_at_browser_close(): + max_age = None + expires = None + else: + max_age = request.session.get_expiry_age() + expires_time = time.time() + max_age + expires = cookie_date(expires_time) + + # we want this cookie to be accessed via javascript + # so httponly is set to None + response.set_cookie( + settings.EDXMKTG_COOKIE_NAME, + 'true', + max_age=max_age, + expires=expires, + domain=settings.SESSION_COOKIE_DOMAIN, + path='/', + secure=None, + httponly=None + ) + + +@csrf_exempt +def create_account(request, post_override=None): + """ + JSON call to create new edX account. + Used by form in signup_modal.html, which is included into navigation.html + """ + try: + create_account_with_params(request, post_override or request.POST) + except AccountValidationError as exc: + return JsonResponse({'success': False, 'value': exc.message, 'field': exc.field}, status=400) + except ValidationError as exc: + field, error_list = next(exc.message_dict.iteritems()) + return JsonResponse( + { + "success": False, + "field": field, + "value": error_list[0], + }, + status=400 + ) + redirect_url = try_change_enrollment(request) # Resume the third-party-auth pipeline if necessary. @@ -1707,25 +1636,7 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many 'success': True, 'redirect_url': redirect_url, }) - - # set the login cookie for the edx marketing site - # we want this cookie to be accessed via javascript - # so httponly is set to None - - if request.session.get_expire_at_browser_close(): - max_age = None - expires = None - else: - max_age = request.session.get_expiry_age() - expires_time = time.time() + max_age - expires = cookie_date(expires_time) - - response.set_cookie(settings.EDXMKTG_COOKIE_NAME, - 'true', max_age=max_age, - expires=expires, domain=settings.SESSION_COOKIE_DOMAIN, - path='/', - secure=None, - httponly=None) + set_marketing_cookie(request, response) return response @@ -1764,21 +1675,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 = { - 'username': username, - 'email': email, - 'password': password, - 'name': full_name, - 'honor_code': u'true', - 'terms_of_service': u'true', - } + form = AccountCreationForm( + data={ + 'username': username, + 'email': email, + 'password': password, + 'name': full_name, + }, + 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) diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py index 07da9bdbfa..2a99c78c20 100644 --- a/common/test/acceptance/tests/discussion/test_cohort_management.py +++ b/common/test/acceptance/tests/discussion/test_cohort_management.py @@ -51,9 +51,9 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): ).visit().get_user_id() self.add_user_to_cohort(self.course_fixture, self.student_name, self.manual_cohort_id) - # create a user with unicode characters in their username - self.unicode_student_id = AutoAuthPage( - self.browser, username="Ωπ", email="unicode_student_user@example.com", + # create a second student user + self.other_student_id = AutoAuthPage( + self.browser, username="other_student_user", email="other_student_user@example.com", course_id=self.course_id, staff=False ).visit().get_user_id() @@ -389,12 +389,12 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): }).count(), 1 ) - # unicode_student_user (previously unassigned) is added to manual cohort + # other_student_user (previously unassigned) is added to manual cohort self.assertEqual( self.event_collection.find({ "name": "edx.cohort.user_added", "time": {"$gt": start_time}, - "event.user_id": {"$in": [int(self.unicode_student_id)]}, + "event.user_id": {"$in": [int(self.other_student_id)]}, "event.cohort_name": self.manual_cohort_name, }).count(), 1 diff --git a/common/test/acceptance/tests/test_cohorted_courseware.py b/common/test/acceptance/tests/test_cohorted_courseware.py index b702352a6d..00f42cd14e 100644 --- a/common/test/acceptance/tests/test_cohorted_courseware.py +++ b/common/test/acceptance/tests/test_cohorted_courseware.py @@ -47,7 +47,7 @@ class EndToEndCohortedCoursewareTest(ContainerBase): ).visit() # Create a student who will end up in the default cohort group - self.cohort_default_student_username = "cohort default student" + self.cohort_default_student_username = "cohort_default_student" self.cohort_default_student_email = "cohort_default_student@example.com" StudioAutoAuthPage( self.browser, username=self.cohort_default_student_username, diff --git a/common/test/data/uploads/cohort_users_both_columns.csv b/common/test/data/uploads/cohort_users_both_columns.csv index eced3db09d..ee13b56c7c 100644 --- a/common/test/data/uploads/cohort_users_both_columns.csv +++ b/common/test/data/uploads/cohort_users_both_columns.csv @@ -1,4 +1,4 @@ username,email,ignored_column,cohort instructor_user,,June,ManualCohort1 ,student_user@example.com,Spring,AutoCohort1 -Ωπ,,Fall,ManualCohort1 +other_student_user,,Fall,ManualCohort1 diff --git a/common/test/data/uploads/cohort_users_only_email.csv b/common/test/data/uploads/cohort_users_only_email.csv index 7835d455fb..7fb6a85400 100644 --- a/common/test/data/uploads/cohort_users_only_email.csv +++ b/common/test/data/uploads/cohort_users_only_email.csv @@ -1,5 +1,5 @@ email,cohort instructor_user@example.com,ManualCohort1 student_user@example.com,AutoCohort1 -unicode_student_user@example.com,ManualCohort1 +other_student_user@example.com,ManualCohort1 diff --git a/common/test/data/uploads/cohort_users_only_username.csv b/common/test/data/uploads/cohort_users_only_username.csv index 0c7588030a..f33b77a0a2 100644 --- a/common/test/data/uploads/cohort_users_only_username.csv +++ b/common/test/data/uploads/cohort_users_only_username.csv @@ -1,4 +1,4 @@ username,cohort instructor_user,ManualCohort1 student_user,AutoCohort1 -Ωπ,ManualCohort1 \ No newline at end of file +other_student_user,ManualCohort1 diff --git a/lms/static/js/spec/student_account/register_spec.js b/lms/static/js/spec/student_account/register_spec.js index bf3ad8ae2c..c3cdf65c50 100644 --- a/lms/static/js/spec/student_account/register_spec.js +++ b/lms/static/js/spec/student_account/register_spec.js @@ -261,12 +261,8 @@ define([ submitForm( true ); // Verify that the client sent the course ID for analytics - var expectedData = {}; - $.extend(expectedData, USER_DATA, { - analytics: JSON.stringify({ - enroll_course_id: COURSE_ID - }) - }); + var expectedData = {course_id: COURSE_ID}; + $.extend(expectedData, USER_DATA); AjaxHelpers.expectRequest( requests, 'POST', diff --git a/lms/static/js/student_account/models/RegisterModel.js b/lms/static/js/student_account/models/RegisterModel.js index ca8b953c5b..47a97c2367 100644 --- a/lms/static/js/student_account/models/RegisterModel.js +++ b/lms/static/js/student_account/models/RegisterModel.js @@ -32,20 +32,17 @@ var edx = edx || {}; sync: function(method, model) { var headers = { 'X-CSRFToken': $.cookie('csrftoken') }, data = {}, - analytics, courseId = $.url( '?course_id' ); // If there is a course ID in the query string param, // send that to the server as well so it can be included // in analytics events. if ( courseId ) { - analytics = JSON.stringify({ - enroll_course_id: decodeURIComponent( courseId ) - }); + data.course_id = decodeURIComponent(courseId); } // Include all form fields and analytics info in the data sent to the server - $.extend( data, model.attributes, { analytics: analytics }); + $.extend( data, model.attributes); $.ajax({ url: model.urlRoot, diff --git a/lms/static/js/student_account/views/RegisterView.js b/lms/static/js/student_account/views/RegisterView.js index ef7fc9854d..f93814603d 100644 --- a/lms/static/js/student_account/views/RegisterView.js +++ b/lms/static/js/student_account/views/RegisterView.js @@ -58,7 +58,22 @@ var edx = edx || {}; saveSuccess: function() { this.trigger('auth-complete'); - } + }, + saveError: function( error ) { + this.errors = _.flatten( + _.map( + JSON.parse(error.responseText), + function(error_list) { + return _.map( + error_list, + function(error) { return "