From 40148392426f626cb779c76d6bdda0f67bd6069d Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Tue, 2 Jun 2020 14:21:08 +0200 Subject: [PATCH] Fixes issue #2907 - Password strength settings are ignored when creating new customer accounts. Make login available to verified users only. --- .../app/controllers/email_verify.coffee | 51 ++++-------- .../javascripts/app/controllers/signup.coffee | 45 ++++++---- .../app/controllers/user_profile.coffee | 28 +++++++ .../widget/user_signup_check.coffee | 82 ------------------- .../app/views/signup/verify.jst.eco | 16 ++++ app/controllers/users_controller.rb | 26 +++--- app/models/user.rb | 2 +- lib/auth/internal.rb | 6 +- .../signup_password_change_and_reset_test.rb | 55 ++----------- 9 files changed, 116 insertions(+), 195 deletions(-) delete mode 100644 app/assets/javascripts/app/controllers/widget/user_signup_check.coffee create mode 100644 app/assets/javascripts/app/views/signup/verify.jst.eco diff --git a/app/assets/javascripts/app/controllers/email_verify.coffee b/app/assets/javascripts/app/controllers/email_verify.coffee index 1f0ad43a2..ae0c5b7d4 100644 --- a/app/assets/javascripts/app/controllers/email_verify.coffee +++ b/app/assets/javascripts/app/controllers/email_verify.coffee @@ -1,7 +1,6 @@ class Index extends App.Controller constructor: -> super - @authenticateCheckRedirect() @verifyCall() verifyCall: => @@ -11,45 +10,23 @@ class Index extends App.Controller url: "#{@apiPath}/users/email_verify" data: JSON.stringify(token: @token) processData: true - success: @success - error: @error - ) + success: (data, status, xhr) => + App.Auth.loginCheck() + @navigate '#' - success: => - new Success(el: @el) + @notify + type: 'success' + msg: App.i18n.translateContent('Woo hoo! Your email address has been verified!') + removeAll: true + timeout: 2000 - error: => - new Fail(el: @el) + error: (data, status, xhr) => + @navigate '#' -class Success extends App.ControllerContent - constructor: -> - super - @render() - - # rerender view, e. g. on language change - @bind 'ui:rerender', => - @render() - - render: => - @renderScreenSuccess( - detail: 'Woo hoo! Your email address has been verified!' - ) - delay = => - @navigate '#' - @delay(delay, 20500) - -class Fail extends App.ControllerContent - constructor: -> - super - @render() - - # rerender view, e. g. on language change - @bind 'ui:rerender', => - @render() - - render: => - @renderScreenError( - detail: 'Unable to verify email. Please contact your administrator.' + @notify + type: 'error' + msg: App.i18n.translateContent('Unable to verify email. Please contact your administrator.') + removeAll: true ) App.Config.set('email_verify/:token', Index, 'Routes') diff --git a/app/assets/javascripts/app/controllers/signup.coffee b/app/assets/javascripts/app/controllers/signup.coffee index be1ec2cea..ff3374c58 100644 --- a/app/assets/javascripts/app/controllers/signup.coffee +++ b/app/assets/javascripts/app/controllers/signup.coffee @@ -2,6 +2,7 @@ class Index extends App.ControllerContent events: 'submit form': 'submit' 'click .submit': 'submit' + 'click .js-submitResend': 'resend' 'click .cancel': 'cancel' constructor: -> @@ -61,31 +62,41 @@ class Index extends App.ControllerContent # save user user.save( done: (r) => - App.Auth.login( - data: - username: @params.login - password: @params.password - success: @success - error: @error + @html App.view('signup/verify')( + email: @params.email ) fail: (settings, details) => @formEnable(e) - @form.showAlert(details.error_human || details.error || 'Unable to update object!') + if _.isArray(details.error) + @form.showAlert( App.i18n.translateInline( details.error[0], details.error[1] ) ) + else + @form.showAlert(details.error_human || details.error || 'Unable to update object!') ) - success: (data, status, xhr) => + resend: (e) => + e.preventDefault() + @formDisable(e) + @resendParams = @formParam(e.target) - # login check - App.Auth.loginCheck() + @ajax( + id: 'email_verify_send' + type: 'POST' + url: @apiPath + '/users/email_verify_send' + data: JSON.stringify(email: @resendParams.email) + processData: true + success: (data, status, xhr) => + @formEnable(e) - # add notify - @notify - type: 'success' - msg: App.i18n.translateContent('Thanks for joining. Email sent to "%s". Please verify your email address.', @params.email) - removeAll: true + # add notify + @notify + type: 'success' + msg: App.i18n.translateContent('Email sent to "%s". Please verify your email address.', @params.email) + removeAll: true - # redirect to # - @navigate '#' + if data.token && @Config.get('developer_mode') is true + @navigate "#email_verify/#{data.token}" + error: @error + ) error: (xhr, statusText, error) => detailsRaw = xhr.responseText diff --git a/app/assets/javascripts/app/controllers/user_profile.coffee b/app/assets/javascripts/app/controllers/user_profile.coffee index 9ea7773cd..1fbfcf5c3 100644 --- a/app/assets/javascripts/app/controllers/user_profile.coffee +++ b/app/assets/javascripts/app/controllers/user_profile.coffee @@ -77,6 +77,8 @@ class App.UserProfile extends App.Controller class ActionRow extends App.ObserverActionRow model: 'User' observe: + verified: true + source: true organization_id: true showHistory: (user) => @@ -100,6 +102,25 @@ class ActionRow extends App.ObserverActionRow newTicket: (user) => @navigate("ticket/create/customer/#{user.id}") + resendVerificationEmail: (user) => + @ajax( + id: 'email_verify_send' + type: 'POST' + url: @apiPath + '/users/email_verify_send' + data: JSON.stringify(email: user.email) + processData: true + success: (data, status, xhr) => + @notify + type: 'success' + msg: App.i18n.translateContent('Email sent to "%s". Please let the user verify his email address.', user.email) + removeAll: true + error: (data, status, xhr) => + @notify + type: 'error' + msg: App.i18n.translateContent('Failed to sent Email "%s". Please contact an administrator.', user.email) + removeAll: true + ) + actions: (user) => actions = [ { @@ -121,6 +142,13 @@ class ActionRow extends App.ObserverActionRow callback: @editUser } + if user.verified isnt true && user.source is 'signup' + actions.push({ + name: 'resend_verification_email' + title: 'Resend verification email' + callback: @resendVerificationEmail + }) + actions class Object extends App.ObserverController diff --git a/app/assets/javascripts/app/controllers/widget/user_signup_check.coffee b/app/assets/javascripts/app/controllers/widget/user_signup_check.coffee deleted file mode 100644 index e42e3ba6d..000000000 --- a/app/assets/javascripts/app/controllers/widget/user_signup_check.coffee +++ /dev/null @@ -1,82 +0,0 @@ -class Widget extends App.Controller - constructor: -> - - # for browser test - App.Event.bind('user_signup_verify', (user) -> - new Modal(user: user) - 'user_signup_verify' - ) - - App.Event.bind('auth:login', (user) => - return if !user - @verifyLater(user.id) - 'user_signup_verify' - ) - user = App.User.current() - @verifyLater(user.id) if user? - - verifyLater: (userId) => - delay = => - @verify(userId) - @delay(delay, 5000, 'user_signup_verify_dialog') - - verify: (userId) -> - return if !userId - return if !App.User.exists(userId) - user = App.User.find(userId) - return if user.source isnt 'signup' - return if user.verified is true - currentTime = new Date().getTime() - createdAt = Date.parse(user.created_at) - diff = currentTime - createdAt - max = 1000 * 60 * 30 # show message if account is older then 30 minutes - return if diff < max - new Modal(user: user) - -class Modal extends App.ControllerModal - backdrop: false - keyboard: false - head: 'Account not verified' - small: true - buttonClose: false - buttonCancel: false - buttonSubmit: 'Resend verification email' - - constructor: -> - super - - content: => - if !@sent - return App.i18n.translateContent('Your account has not been verified. Please click the link in the verification email.') - content = App.i18n.translateContent('We\'ve sent an email to _%s_. Click the link in the email to verify your account.', @user.email) - content += '

' - content += App.i18n.translateContent('If you don\'t see the email, check other places it might be, like your junk, spam, social, or other folders.') - content - - onSubmit: => - @ajax( - id: 'email_verify_send' - type: 'POST' - url: @apiPath + '/users/email_verify_send' - data: JSON.stringify(email: @user.email) - processData: true - success: @success - error: @error - ) - - success: (data) => - @sent = true - @update() - - # if in developer mode, redirect to verify - if data.token && @Config.get('developer_mode') is true - redirect = => - @close() - @navigate "#email_verify/#{data.token}" - App.Delay.set(redirect, 4000) - - error: => - @contentInline = App.i18n.translateContent('Unable to send verify email.') - @update() - -App.Config.set('user_signup', Widget, 'Widgets') diff --git a/app/assets/javascripts/app/views/signup/verify.jst.eco b/app/assets/javascripts/app/views/signup/verify.jst.eco new file mode 100644 index 000000000..fe9814eae --- /dev/null +++ b/app/assets/javascripts/app/views/signup/verify.jst.eco @@ -0,0 +1,16 @@ +
+
+
+

<%- @T('Registration successful!') %>

+

<%- @T('Thanks for joining. Email sent to "%s".', @email) %>

+

<%- @T('Please click the link in the verification email.') %> <%- @T('If you don\'t see the email, check other places it might be, like your junk, spam, social, or other folders.') %>

+
+ +
+ <%- @T('Go Back') %> + +
+
+
+
+
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 45c3080fb..d8cd88313 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -4,7 +4,7 @@ class UsersController < ApplicationController include ChecksUserAttributesByCurrentUserPermission prepend_before_action -> { authorize! }, only: %i[import_example import_start search history] - prepend_before_action :authentication_check, except: %i[create password_reset_send password_reset_verify image] + prepend_before_action :authentication_check, except: %i[create password_reset_send password_reset_verify image email_verify email_verify_send] prepend_before_action :authentication_check_only, only: [:create] # @path [GET] /users @@ -140,6 +140,15 @@ class UsersController < ApplicationController exists = User.exists?(email: clean_params[:email].downcase.strip) raise Exceptions::UnprocessableEntity, "Email address '#{clean_params[:email].downcase.strip}' is already used for other user." if exists + # check password policy + if clean_params[:password].present? + result = password_policy(clean_params[:password]) + if result != true + render json: { error: result }, status: :unprocessable_entity + return + end + end + user = User.new(clean_params) user.associations_from_param(params) user.updated_by_id = 1 @@ -499,6 +508,8 @@ curl http://localhost/api/v1/users/email_verify -v -u #{login}:#{password} -H "C user = User.signup_verify_via_token(params[:token], current_user) raise Exceptions::UnprocessableEntity, 'Invalid token!' if !user + current_user_set(user) + render json: { message: 'ok', user_email: user.email }, status: :ok end @@ -527,17 +538,12 @@ curl http://localhost/api/v1/users/email_verify_send -v -u #{login}:#{password} raise Exceptions::UnprocessableEntity, 'No email!' if !params[:email] user = User.find_by(email: params[:email].downcase) - if !user + if !user || user.verified == true # result is always positive to avoid leaking of existing user accounts render json: { message: 'ok' }, status: :ok return end - #if user.verified == true - # render json: { error: 'Already verified!' }, status: :unprocessable_entity - # return - #end - Token.create(action: 'Signup', user_id: user.id) result = User.signup_new_token(user) @@ -1029,13 +1035,13 @@ curl http://localhost/api/v1/users/avatar -v -u #{login}:#{password} -H "Content def password_policy(password) if Setting.get('password_min_size').to_i > password.length - return ["Can\'t update password, it must be at least %s characters long!", Setting.get('password_min_size')] + return ['Invalid password, it must be at least %s characters long!', Setting.get('password_min_size')] end if Setting.get('password_need_digit').to_i == 1 && password !~ /\d/ - return ["Can't update password, it must contain at least 1 digit!"] + return ['Invalid password, it must contain at least 1 digit!'] end if Setting.get('password_min_2_lower_2_upper_characters').to_i == 1 && ( password !~ /[A-Z].*[A-Z]/ || password !~ /[a-z].*[a-z]/ ) - return ["Can't update password, it must contain at least 2 lowercase and 2 uppercase characters!"] + return ['Invalid password, it must contain at least 2 lowercase and 2 uppercase characters!'] end true diff --git a/app/models/user.rb b/app/models/user.rb index c5b3a3e1d..d4076a9ed 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -586,7 +586,7 @@ returns return if !user # reset password - user.update!(password: password) + user.update!(password: password, verified: true) # delete token Token.find_by(action: 'PasswordReset', name: token).destroy diff --git a/lib/auth/internal.rb b/lib/auth/internal.rb index 5da889903..60ccb771b 100644 --- a/lib/auth/internal.rb +++ b/lib/auth/internal.rb @@ -12,7 +12,11 @@ class Auth return true end - PasswordHash.verified?(user.password, password) + password_verified = PasswordHash.verified?(user.password, password) + + raise Exceptions::NotAuthorized, 'Please verify your account before you can login!' if !user.verified && user.source == 'signup' && password_verified + + password_verified end private diff --git a/test/browser/signup_password_change_and_reset_test.rb b/test/browser/signup_password_change_and_reset_test.rb index 333329384..aa868db56 100644 --- a/test/browser/signup_password_change_and_reset_test.rb +++ b/test/browser/signup_password_change_and_reset_test.rb @@ -23,65 +23,26 @@ class SignupPasswordChangeAndResetTest < TestCase ) set( css: 'input[name="password"]', - value: 'some-pass', + value: 'some-pass-123', ) set( css: 'input[name="password_confirm"]', - value: 'some-pass', + value: 'some-pass-123', ) click(css: 'button.js-submit') - watch_for_disappear( - css: '.signup', - timeout: 10, - ) - - match( - css: '.user-menu .user a', - value: signup_user_email, - attribute: 'title', - ) - - # check email verify - location(url: "#{browser_url}#email_verify/not_existing") watch_for( css: '#content', - value: 'Unable to verify email', + value: 'Registration successful!', ) - logout() - login( - username: signup_user_email, - password: 'some-pass', - url: "#{browser_url}#email_verify/not_existing2", - ) - watch_for( - css: '#content', - value: 'Unable to verify email', - ) - execute( - js: 'App.Event.trigger("user_signup_verify", App.Session.get())', - ) - modal_ready() - click(css: '.modal .js-submit') + # auto login via token trick in dev mode + click(css: '.signup .js-submitResend') - execute( - js: 'App.Auth.logout()', - ) - sleep 6 watch_for( - css: '#login', + css: '.content.active', + value: 'Welcome!', ) - login( - username: signup_user_email, - password: 'some-pass', - ) - watch_for( - css: '#content', - value: 'Your email address has been verified', - ) - modal_disappear() - sleep 2 # change password click(css: '.navbar-items-personal .user a') @@ -109,7 +70,7 @@ class SignupPasswordChangeAndResetTest < TestCase set( css: 'input[name="password_old"]', - value: 'some-pass', + value: 'some-pass-123', ) set( css: 'input[name="password_new_confirm"]',