From fc0edb56a33aab0e0304a577b52b6956c86432f4 Mon Sep 17 00:00:00 2001 From: Saleem Latif <saleem_ee@hotmail.com> Date: Thu, 14 Dec 2017 14:27:33 +0500 Subject: [PATCH] Feedback changes --- common/djangoapps/third_party_auth/pipeline.py | 20 +++++++++----------- common/djangoapps/third_party_auth/utils.py | 28 ++++++++++++++++++++++++++++ lms/djangoapps/student_account/test/test_views.py | 1 - lms/djangoapps/student_account/views.py | 19 +++++-------------- lms/static/sass/views/_login-register.scss | 20 ++------------------ lms/templates/student_account/form_field.underscore | 3 +-- openedx/core/djangoapps/user_api/api.py | 66 ++++++++++++++++++++++++++++++------------------------------------ openedx/core/djangoapps/user_api/helpers.py | 6 +++--- 8 files changed, 78 insertions(+), 85 deletions(-) create mode 100644 common/djangoapps/third_party_auth/utils.py diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 1fa0a67..4fd41e8 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -84,6 +84,7 @@ import student from edxmako.shortcuts import render_to_string from eventtracking import tracker from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from third_party_auth.utils import user_exists from . import provider @@ -501,7 +502,7 @@ def set_pipeline_timeout(strategy, user, *args, **kwargs): # choice of the user. -def redirect_to_custom_form(request, auth_entry, kwargs): +def redirect_to_custom_form(request, auth_entry, details, kwargs): """ If auth_entry is found in AUTH_ENTRY_CUSTOM, this is used to send provider data to an external server's registration/login page. @@ -520,7 +521,7 @@ def redirect_to_custom_form(request, auth_entry, kwargs): "auth_entry": auth_entry, "backend_name": backend_name, "provider_id": provider_id, - "user_details": kwargs['details'], + "user_details": details, }) digest = hmac.new(secret_key, msg=data_str, digestmod=hashlib.sha256).digest() # Store the data in the session temporarily, then redirect to a page that will POST it to @@ -535,7 +536,7 @@ def redirect_to_custom_form(request, auth_entry, kwargs): @partial.partial def ensure_user_information(strategy, auth_entry, backend=None, user=None, social=None, current_partial=None, - allow_inactive_user=False, *args, **kwargs): + allow_inactive_user=False, details=None, *args, **kwargs): """ Ensure that we have the necessary information about a user (either an existing account or registration data) to proceed with the pipeline. @@ -567,13 +568,10 @@ def ensure_user_information(strategy, auth_entry, backend=None, user=None, socia (current_provider.skip_email_verification or current_provider.send_to_registration_first)) if not user: - details = kwargs.get('details', {}) - if 'email' in details or 'username' in details: - # pylint: disable=invalid-name - qs = {'email': details['email']} if details.get('email') else \ - {'username': details.get('username')} - if User.objects.filter(**qs).exists(): - return dispatch_to_login() + if user_exists(details or {}): + # User has not already authenticated and the details sent over from + # identity provider belong to an existing user. + return dispatch_to_login() if is_api(auth_entry): return HttpResponseBadRequest() @@ -591,7 +589,7 @@ def ensure_user_information(strategy, auth_entry, backend=None, user=None, socia raise AuthEntryError(backend, 'auth_entry is wrong. Settings requires a user.') elif auth_entry in AUTH_ENTRY_CUSTOM: # Pass the username, email, etc. via query params to the custom entry page: - return redirect_to_custom_form(strategy.request, auth_entry, kwargs) + return redirect_to_custom_form(strategy.request, auth_entry, details or {}, kwargs) else: raise AuthEntryError(backend, 'auth_entry invalid') diff --git a/common/djangoapps/third_party_auth/utils.py b/common/djangoapps/third_party_auth/utils.py new file mode 100644 index 0000000..07edb18 --- /dev/null +++ b/common/djangoapps/third_party_auth/utils.py @@ -0,0 +1,28 @@ +""" +Utility functions for third_party_auth +""" + +from django.contrib.auth.models import User + + +def user_exists(details): + """ + Return True if user with given details exist in the system. + + Arguments: + details (dict): dictionary containing user infor like email, username etc. + + Returns: + (bool): True if user with given details exists, `False` otherwise. + """ + # Send the user to the login page if we already have an + # account that matches the authentication details. + user_queryset_filter = {} + email = details.get('email') + username = details.get('username') + if email: + user_queryset_filter['email'] = email + elif username: + user_queryset_filter['username'] = username + + return user_queryset_filter and User.objects.filter(user_queryset_filter).exists() diff --git a/lms/djangoapps/student_account/test/test_views.py b/lms/djangoapps/student_account/test/test_views.py index d3ac278..e1034c0 100644 --- a/lms/djangoapps/student_account/test/test_views.py +++ b/lms/djangoapps/student_account/test/test_views.py @@ -641,7 +641,6 @@ class StudentAccountLoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMi "errorMessage": None, "registerFormSubmitButtonText": "Create Account", "hideSignInLink": False, - "syncLearnerProfileData": False, } if expected_ec is not None: # If we set an EnterpriseCustomer, third-party auth providers ought to be hidden. diff --git a/lms/djangoapps/student_account/views.py b/lms/djangoapps/student_account/views.py index 528399a..e661b9e 100644 --- a/lms/djangoapps/student_account/views.py +++ b/lms/djangoapps/student_account/views.py @@ -47,6 +47,7 @@ from student.views import register_user as old_register_view from student.views import signin_user as old_login_view from third_party_auth import pipeline from third_party_auth.decorators import xframe_allow_whitelisted +from third_party_auth.utils import user_exists from util.bad_request_rate_limiter import BadRequestRateLimiter from util.date_utils import strftime_localized @@ -133,12 +134,10 @@ def login_and_registration_form(request, initial_mode="login"): running_pipeline = pipeline.get(request) if running_pipeline: pipeline_user_details = running_pipeline['kwargs']['details'] - if 'email' in pipeline_user_details or 'username' in pipeline_user_details: - # pylint: disable=invalid-name - qs = {'email': pipeline_user_details['email']} if pipeline_user_details.get('email') else \ - {'username': pipeline_user_details.get('username')} - if User.objects.filter(**qs).exists(): - account_creation_allowed = False + if user_exists(pipeline_user_details): + # Details sent over from identity provider belong to an existing user. + # So, force user to login and do not allow account creation + account_creation_allowed = False # Otherwise, render the combined login/registration page context = { @@ -252,7 +251,6 @@ def update_context_for_enterprise(request, context): context = context.copy() sidebar_context = enterprise_sidebar_context(request) - sync_learner_profile_data = context['data']['third_party_auth']['syncLearnerProfileData'] if sidebar_context: context['data']['registration_form_desc']['fields'] = enterprise_fields_only( @@ -264,11 +262,6 @@ def update_context_for_enterprise(request, context): else: context['enable_enterprise_sidebar'] = False - if sync_learner_profile_data: - context['data']['hide_auth_warnings'] = True - enterprise_customer = enterprise_customer_for_request(request) - context['data']['enterprise_name'] = enterprise_customer and enterprise_customer['name'] - return context @@ -349,7 +342,6 @@ def _third_party_auth_context(request, redirect_to, tpa_hint=None): "finishAuthUrl": None, "errorMessage": None, "registerFormSubmitButtonText": _("Create Account"), - "syncLearnerProfileData": False, "hideSignInLink": False, } @@ -383,7 +375,6 @@ def _third_party_auth_context(request, redirect_to, tpa_hint=None): context["currentProvider"] = current_provider.name context["finishAuthUrl"] = pipeline.get_complete_url(current_provider.backend_name) context["hideSignInLink"] = bool(current_provider.sync_learner_profile_data) - context["syncLearnerProfileData"] = current_provider.sync_learner_profile_data if current_provider.skip_registration_form: # For enterprise (and later for everyone), we need to get explicit consent to the diff --git a/lms/static/sass/views/_login-register.scss b/lms/static/sass/views/_login-register.scss index 85bafaa..bebaba5 100644 --- a/lms/static/sass/views/_login-register.scss +++ b/lms/static/sass/views/_login-register.scss @@ -259,20 +259,6 @@ display: none; } - input[readonly] { - font-family: OpenSans-Regular; - background-color: #e6e4e4; - color: #292b2c; - font-size: 22px; - border: none; - margin: 5px 0; - - &:focus { - outline: none; - } - } - - .auto-register-message { font-size: 1.1em; line-height: 1.3em; @@ -460,8 +446,7 @@ outline: solid 1px $gray-l3; cursor: pointer; - &:active, - &:focus { + &:active, &:focus { outline: auto; } } @@ -477,8 +462,7 @@ } } - .tip, - .label-optional { + .tip, .label-optional { @extend %t-copy-sub1; color: $uxpl-gray-base; diff --git a/lms/templates/student_account/form_field.underscore b/lms/templates/student_account/form_field.underscore index cbe5b3c..4cba9a5 100644 --- a/lms/templates/student_account/form_field.underscore +++ b/lms/templates/student_account/form_field.underscore @@ -91,8 +91,7 @@ <% } %> <% if ( restrictions.min_length ) { %> minlength="<%- restrictions.min_length %>"<% } %> <% if ( restrictions.max_length ) { %> maxlength="<%- restrictions.max_length %>"<% } %> - <% if ( restrictions.readonly ) { %> readonly <% } %> - <% if ( required ) { %> required <% } %> + <% if ( required ) { %> required<% } %> <% if ( typeof errorMessages !== 'undefined' ) { _.each(errorMessages, function( msg, type ) {%> data-errormsg-<%- type %>="<%- msg %>" diff --git a/openedx/core/djangoapps/user_api/api.py b/openedx/core/djangoapps/user_api/api.py index 2d39a05..a7c51ff 100644 --- a/openedx/core/djangoapps/user_api/api.py +++ b/openedx/core/djangoapps/user_api/api.py @@ -835,10 +835,38 @@ class RegistrationFormFactory(object): current_provider = third_party_auth.provider.Registry.get_from_pipeline(running_pipeline) if current_provider: - self._override_registration_fields_using_identity_provider_configuration( - request, current_provider, running_pipeline, form_desc + # Override username / email / full name + field_overrides = current_provider.get_register_form_data( + running_pipeline.get('kwargs') ) + # When the TPA Provider is configured to skip the registration form and we are in an + # enterprise context, we need to hide all fields except for terms of service and + # ensure that the user explicitly checks that field. + hide_registration_fields_except_tos = ( + ( + current_provider.skip_registration_form and enterprise_customer_for_request(request) + ) or current_provider.sync_learner_profile_data + ) + + for field_name in self.DEFAULT_FIELDS + self.EXTRA_FIELDS: + if field_name in field_overrides: + form_desc.override_field_properties( + field_name, default=field_overrides[field_name] + ) + + if (field_name not in ['terms_of_service', 'honor_code'] + and field_overrides[field_name] + and hide_registration_fields_except_tos): + + form_desc.override_field_properties( + field_name, + field_type="hidden", + label="", + instructions="", + ) + + # Hide the password field form_desc.override_field_properties( "password", default="", @@ -856,37 +884,3 @@ class RegistrationFormFactory(object): default=current_provider.name if current_provider.name else "Third Party", required=False, ) - - # pylint: disable=invalid-name - def _override_registration_fields_using_identity_provider_configuration( - self, request, current_provider, running_pipeline, form_desc - ): - """ - Apply form field overrides based on configuration of third party auth provider. - """ - field_overrides = current_provider.get_register_form_data( - running_pipeline.get('kwargs') - ) - # When the TPA Provider is configured to skip the registration form and we are in an - # enterprise context, we need to hide all fields except for terms of service and - # ensure that the user explicitly checks that field. - hide_registration_fields_except_tos = ( - ( - current_provider.skip_registration_form and enterprise_customer_for_request(request) - ) or current_provider.sync_learner_profile_data - ) - - for field_name in self.DEFAULT_FIELDS + self.EXTRA_FIELDS: - if field_name in field_overrides: - form_desc.override_field_properties( - field_name, default=field_overrides[field_name] - ) - - if field_name not in ['terms_of_service', 'honor_code'] and field_overrides[field_name] and \ - hide_registration_fields_except_tos: - form_desc.override_field_properties( - field_name, - field_type="hidden", - label="", - instructions="", - ) diff --git a/openedx/core/djangoapps/user_api/helpers.py b/openedx/core/djangoapps/user_api/helpers.py index 382e251..a2bff07 100644 --- a/openedx/core/djangoapps/user_api/helpers.py +++ b/openedx/core/djangoapps/user_api/helpers.py @@ -124,9 +124,9 @@ class FormDescription(object): ALLOWED_TYPES = ["text", "email", "select", "textarea", "checkbox", "password", "hidden"] ALLOWED_RESTRICTIONS = { - "text": ["min_length", "max_length", "readonly"], - "password": ["min_length", "max_length", "readonly"], - "email": ["min_length", "max_length", "readonly"], + "text": ["min_length", "max_length"], + "password": ["min_length", "max_length"], + "email": ["min_length", "max_length"], } FIELD_TYPE_MAP = { -- libgit2 0.26.0