From 2383fb3fa6f43f048b3df9d1007499afa06a5658 Mon Sep 17 00:00:00 2001 From: Adeel Khan Date: Wed, 6 May 2020 15:41:13 +0500 Subject: [PATCH] Improving user locked out logic. This patch improves on the user locked out logic by providing a helping message near locked out. This would help reduce retries by giving user the option to use password reset flow to fix the issue. PROD-1505 --- common/djangoapps/student/models.py | 10 ++++ .../js/spec/student_account/login_spec.js | 6 +-- .../student_account/password_reset_spec.js | 7 +++ .../js/student_account/views/AccessView.js | 6 +++ .../js/student_account/views/FormView.js | 51 ++++++++++++++++++- .../js/student_account/views/LoginView.js | 10 +++- .../views/PasswordResetView.js | 10 +++- lms/static/sass/views/_login-register.scss | 19 ++++++- .../student_account/form_field.underscore | 15 +++++- .../core/djangoapps/user_authn/views/login.py | 38 +++++++++++++- 10 files changed, 160 insertions(+), 12 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index a11ad557f9..231e224df5 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -970,6 +970,16 @@ class LoginFailures(models.Model): record.save() + @classmethod + def check_user_reset_password_threshold(cls, user): + """ + Checks if the user is above threshold for reset password message. + """ + record, _ = LoginFailures.objects.get_or_create(user=user) + max_failures_allowed = settings.MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED + + return record.failure_count >= max_failures_allowed / 2, record.failure_count + @classmethod def clear_lockout_counter(cls, user): """ diff --git a/lms/static/js/spec/student_account/login_spec.js b/lms/static/js/spec/student_account/login_spec.js index 678d1b6b21..a2aa534c8f 100644 --- a/lms/static/js/spec/student_account/login_spec.js +++ b/lms/static/js/spec/student_account/login_spec.js @@ -193,11 +193,11 @@ expect($('.button-oa2-facebook')).toBeVisible(); }); - it('displays a link to the password reset form', function() { + it('displays a link to the signin help', function() { createLoginView(this); - // Verify that the password reset link is displayed - expect($('.forgot-password')).toBeVisible(); + // Verify that the Signin help link is displayed + expect($('.login-help')).toBeVisible(); }); it('displays a link to the enterprise slug login', function() { diff --git a/lms/static/js/spec/student_account/password_reset_spec.js b/lms/static/js/spec/student_account/password_reset_spec.js index 8846655312..20c1d92ed7 100644 --- a/lms/static/js/spec/student_account/password_reset_spec.js +++ b/lms/static/js/spec/student_account/password_reset_spec.js @@ -105,6 +105,13 @@ expect($(view.el).html().length).toEqual(0); }); + it('displays a link to the password reset help', function() { + createPasswordResetView(this); + + // Verify that the password reset help link is displayed + expect($('.reset-help')).toBeVisible(); + }); + it('validates the email field', function() { createPasswordResetView(this); diff --git a/lms/static/js/student_account/views/AccessView.js b/lms/static/js/student_account/views/AccessView.js index 1289e73095..9d547d3639 100644 --- a/lms/static/js/student_account/views/AccessView.js +++ b/lms/static/js/student_account/views/AccessView.js @@ -264,6 +264,12 @@ // Load the form. Institution login is always refreshed since it changes based on the previous form. if (!this.form.isLoaded($form) || type == 'institution_login') { + + // We need a special case for loading reset form as there is mismatch of form id + // value ie 'password-reset' vs load function name ie 'reset' + if (type === 'password-reset') { + type = 'reset'; + } this.loadForm(type); } this.activeForm = type; diff --git a/lms/static/js/student_account/views/FormView.js b/lms/static/js/student_account/views/FormView.js index dd136da322..676816b763 100644 --- a/lms/static/js/student_account/views/FormView.js +++ b/lms/static/js/student_account/views/FormView.js @@ -144,12 +144,16 @@ $form = this.$form, elements = $form[0].elements, i, + $n, + tpl, len = elements.length, $el, $label, key = '', errors = [], - validation = {}; + validation = {}, + $desc, + $validationNode; for (i = 0; i < len; i++) { $el = $(elements[i]); @@ -163,12 +167,29 @@ } if (key) { + if (this.interesting_fields($el)) { + this.remove_validation_error($el, $form); + } validation = this.validate(elements[i]); if (validation.isValid) { obj[key] = $el.attr('type') === 'checkbox' ? $el.is(':checked') : $el.val(); $el.removeClass('error'); $label.removeClass('error'); } else { + if (this.interesting_fields($el)) { + $validationNode = this.get_error_validation_node($el, $form); + if ($validationNode) { + $n = $.parseHTML(validation.message); + tpl = HtmlUtils.template(''); + + HtmlUtils.prepend($n, tpl()); + HtmlUtils.append($validationNode, HtmlUtils.HTML($n)); + } + + $desc = $form.find('#' + $el.attr('id') + '-desc'); + $desc.remove(); + } + errors.push(validation.message); $el.addClass('error'); $label.addClass('error'); @@ -180,6 +201,34 @@ return obj; }, + remove_validation_error: function($el, $form) { + var $validationNode = this.get_error_validation_node($el, $form); + if ($validationNode && $validationNode.find('li').length > 0) { + $validationNode.empty(); + } + }, + + get_error_validation_node: function($el, $form) { + var $node = $form.find('#' + $el.attr('id') + '-validation-error-msg'); + return $node.find('ul'); + }, + + interesting_fields: function($el) { + return ($el.attr('name') === 'email' || $el.attr('name') === 'password'); + }, + + toggleHelp: function(event, $help) { + var $el = $(event.currentTarget); + var $i = $el.find('i'); + + if ($help.css('display') === 'block') { + $help.css('display', 'none'); + $i.addClass('fa-caret-right').removeClass('fa-caret-down'); + } else { + $help.css('display', 'block'); + $i.addClass('fa-caret-down').removeClass('fa-caret-right'); + } + }, saveError: function(error) { this.errors = [ diff --git a/lms/static/js/student_account/views/LoginView.js b/lms/static/js/student_account/views/LoginView.js index 703ed4f6c7..3b0fa99785 100644 --- a/lms/static/js/student_account/views/LoginView.js +++ b/lms/static/js/student_account/views/LoginView.js @@ -24,7 +24,8 @@ 'click .js-login': 'submitForm', 'click .forgot-password': 'forgotPassword', 'click .login-provider': 'thirdPartyAuth', - 'click .enterprise-login': 'enterpriseSlugLogin' + 'click .enterprise-login': 'enterpriseSlugLogin', + 'click .login-help': 'toggleLoginHelp' }, formType: 'login', requiredStr: '', @@ -139,6 +140,13 @@ this.clearPasswordResetSuccess(); }, + toggleLoginHelp: function(event) { + var $help; + event.preventDefault(); + $help = $('#login-help'); + this.toggleHelp(event, $help); + }, + enterpriseSlugLogin: function(event) { event.preventDefault(); if (this.enterpriseSlugLoginURL) { diff --git a/lms/static/js/student_account/views/PasswordResetView.js b/lms/static/js/student_account/views/PasswordResetView.js index 46007ce311..4dc6142d57 100644 --- a/lms/static/js/student_account/views/PasswordResetView.js +++ b/lms/static/js/student_account/views/PasswordResetView.js @@ -11,7 +11,8 @@ tpl: '#password_reset-tpl', events: { - 'click .js-reset': 'submitForm' + 'click .js-reset': 'submitForm', + 'click .reset-help': 'toggleResetHelp' }, formType: 'password-reset', @@ -27,6 +28,13 @@ this.listenTo(this.model, 'sync', this.saveSuccess); }, + toggleResetHelp: function(event) { + var $help; + event.preventDefault(); + $help = $('#reset-help'); + this.toggleHelp(event, $help); + }, + saveSuccess: function() { this.trigger('password-email-sent'); diff --git a/lms/static/sass/views/_login-register.scss b/lms/static/sass/views/_login-register.scss index 102e4c536d..42e23f098b 100644 --- a/lms/static/sass/views/_login-register.scss +++ b/lms/static/sass/views/_login-register.scss @@ -98,6 +98,16 @@ } } +#login-help, #reset-help { + padding-left: 8px; +} +ul.fa-ul{ + margin: 0 0 0 0; + i { + margin-right: 5px; + } +} + .login-register { $grid-columns: 12; @@ -373,8 +383,8 @@ @extend %t-copy-sub2; display: block; - margin-bottom: ($baseline/2); - margin-top: ($baseline/4); + margin-bottom: 5px; + margin-top: 5px; border: none; padding: 0; background: transparent; @@ -391,6 +401,11 @@ &:focus { text-decoration: underline; } + > i { + border: none; + padding: 0; + margin: 0 2px 0 0; + } } input, diff --git a/lms/templates/student_account/form_field.underscore b/lms/templates/student_account/form_field.underscore index 8e6f3713fc..080e38b8ba 100644 --- a/lms/templates/student_account/form_field.underscore +++ b/lms/templates/student_account/form_field.underscore @@ -127,13 +127,24 @@ - +
    <% if ( instructions ) { %> <%- instructions %><% } %> <% } %> <% if( form === 'login' && name === 'password' ) { %> - + + <% } %> + <% if( form === 'password-reset' && name === 'email' ) { %> + + + <% } %> diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index b8c712aca1..7d6068b311 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -113,8 +113,21 @@ def _check_excessive_login_attempts(user): """ if user and LoginFailures.is_feature_enabled(): if LoginFailures.is_user_locked_out(user): - raise AuthFailedError(_('This account has been temporarily locked due ' - 'to excessive login failures. Try again later.')) + _generate_locked_out_error_message() + + +def _generate_locked_out_error_message(): + + locked_out_period_in_sec = settings.MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS + raise AuthFailedError(Text(_('To protect your account, it’s been temporarily ' + 'locked. Try again in {locked_out_period} minutes.' + '{li_start}To be on the safe side, you can reset your ' + 'password {link_start}here{link_end} before you try again.')).format( + link_start=HTML(''), + link_end=HTML(''), + li_start=HTML('
  • '), + li_end=HTML('
    • '), + locked_out_period=int(locked_out_period_in_sec / 60))) def _enforce_password_policy_compliance(request, user): @@ -230,6 +243,27 @@ def _handle_failed_authentication(user, authenticated_user): else: AUDIT_LOG.warning(u"Login failed - password for {0} is invalid".format(user.email)) + if user and LoginFailures.is_feature_enabled(): + blocked_threshold, failure_count = LoginFailures.check_user_reset_password_threshold(user) + if blocked_threshold: + if not LoginFailures.is_user_locked_out(user): + max_failures_allowed = settings.MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED + remaining_attempts = max_failures_allowed - failure_count + raise AuthFailedError(Text(_('Email or password is incorrect.' + '{li_start}You have {remaining_attempts} more sign-in ' + 'attempts before your account is temporarily locked.{li_end}' + '{li_start}If you\'ve forgotten your password, click ' + '{link_start}here{link_end} to reset.{li_end}' + )) + .format( + link_start=HTML(''), + link_end=HTML(''), + li_start=HTML('
  • '), + li_end=HTML('
    • '), + remaining_attempts=remaining_attempts)) + else: + _generate_locked_out_error_message() + raise AuthFailedError(_('Email or password is incorrect.'))