diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 1accfac5ca..976c114322 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -1404,15 +1404,23 @@ def _do_create_account(form): 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( @@ -1421,33 +1429,25 @@ 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) + 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"]) - 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 @@ -1464,7 +1464,7 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many ) form = AccountCreationForm( - data=post_vars, + data=params, extra_fields=extra_fields, extended_profile_fields=extended_profile_fields, enforce_username_neq_password=True, @@ -1472,35 +1472,19 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many tos_required=tos_required ) - if not form.is_valid(): - field, error_list = next(form.errors.iteritems()) - return JsonResponse( - { - "success": False, - "field": field, - "value": error_list[0], - }, - status=400 - ) - - try: - with transaction.commit_on_success(): - ret = _do_create_account(form) - 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 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 @@ -1515,7 +1499,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={ @@ -1528,7 +1512,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, } @@ -1559,16 +1543,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) @@ -1581,8 +1560,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') @@ -1590,7 +1569,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. @@ -1602,25 +1629,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 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/openedx/core/djangoapps/user_api/helpers.py b/openedx/core/djangoapps/user_api/helpers.py index bb85be6fa9..a99c53767c 100644 --- a/openedx/core/djangoapps/user_api/helpers.py +++ b/openedx/core/djangoapps/user_api/helpers.py @@ -374,16 +374,6 @@ def shim_student_view(view_func, check_logged_in=False): ) ) - # Backwards compatibility: the student view expects both - # terms of service and honor code values. Since we're combining - # these into a single checkbox, the only value we may get - # from the new view is "honor_code". - # Longer term, we will need to make this more flexible to support - # open source installations that may have separate checkboxes - # for TOS, privacy policy, etc. - if request.POST.get("honor_code") is not None and request.POST.get("terms_of_service") is None: - request.POST["terms_of_service"] = request.POST.get("honor_code") - # Call the original view to generate a response. # We can safely modify the status code or content # of the response, but to be safe we won't mess diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index 616315d8cf..5f2d6172a0 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/tests/test_views.py @@ -1233,6 +1233,7 @@ class RegistrationViewTest(ApiTestCase): "honor_code": "true", }) self.assertHttpOK(response) + self.assertIn(settings.EDXMKTG_COOKIE_NAME, self.client.cookies) # Verify that the user exists self.assertEqual( diff --git a/openedx/core/djangoapps/user_api/views.py b/openedx/core/djangoapps/user_api/views.py index eaf7828d02..24c7f16fc9 100644 --- a/openedx/core/djangoapps/user_api/views.py +++ b/openedx/core/djangoapps/user_api/views.py @@ -8,7 +8,7 @@ from django.conf import settings from django.contrib.auth.models import User from django.http import HttpResponse from django.core.urlresolvers import reverse -from django.core.exceptions import ImproperlyConfigured +from django.core.exceptions import ImproperlyConfigured, ValidationError from django.utils.translation import ugettext as _ from django.utils.decorators import method_decorator from django.views.decorators.csrf import ensure_csrf_cookie, csrf_protect @@ -25,7 +25,9 @@ from django_comment_common.models import Role from opaque_keys.edx.locations import SlashSeparatedCourseKey from edxmako.shortcuts import marketing_link +from student.views import create_account_with_params, set_marketing_cookie from util.authentication import SessionAuthenticationAllowInactiveUser +from util.json_request import JsonResponse from .api import account as account_api, profile as profile_api from .helpers import FormDescription, shim_student_view, require_post_params from .models import UserPreference, UserProfile @@ -232,10 +234,8 @@ class RegistrationView(APIView): You must send all required form fields with the request. - You can optionally send an `analytics` param with a JSON-encoded - object with additional info to include in the registration analytics event. - Currently, the only supported field is "enroll_course_id" to indicate - that the user registered while enrolling in a particular course. + You can optionally send a "course_id" param to indicate in analytics + events that the user registered while enrolling in a particular course. Arguments: request (HTTPRequest) @@ -243,11 +243,13 @@ class RegistrationView(APIView): Returns: HttpResponse: 200 on success HttpResponse: 400 if the request is not valid. - HttpResponse: 302 if redirecting to another page. - + HttpResponse: 409 if an account with the given username or email + address already exists """ - email = request.POST.get('email') - username = request.POST.get('username') + data = request.POST.copy() + + email = data.get('email') + username = data.get('username') # Handle duplicate email/username conflicts = account_api.check_account_exists(email=email, username=username) @@ -278,10 +280,29 @@ class RegistrationView(APIView): content_type="text/plain" ) - # For the initial implementation, shim the existing login view - # from the student Django app. - from student.views import create_account - return shim_student_view(create_account)(request) + # Backwards compatibility: the student view expects both + # terms of service and honor code values. Since we're combining + # these into a single checkbox, the only value we may get + # from the new view is "honor_code". + # Longer term, we will need to make this more flexible to support + # open source installations that may have separate checkboxes + # for TOS, privacy policy, etc. + if data.get("honor_code") and "terms_of_service" not in data: + data["terms_of_service"] = data["honor_code"] + + try: + create_account_with_params(request, data) + except ValidationError as err: + error_list = next(err.message_dict.itervalues()) + return HttpResponse( + status=400, + content=error_list[0], + content_type="text/plain" + ) + + response = JsonResponse({"success": True}) + set_marketing_cookie(request, response) + return response def _add_email_field(self, form_desc, required=True): """Add an email field to a form description.