From 15e9aced75319e6e2b48f4b19fcba1346103f772 Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Tue, 25 Jun 2019 22:32:54 +0000 Subject: [PATCH] New RecaptchaExperimentHelper modules RecaptchaExperimentHelper contains helper methods to assist in the controller and view layers. --- app/controllers/registrations_controller.rb | 30 ++++++++++++------- app/helpers/recaptcha_experiment_helper.rb | 7 +++++ .../application_settings/_spam.html.haml | 5 +++- app/views/devise/shared/_signup_box.html.haml | 2 +- locale/gitlab.pot | 3 ++ .../registrations_controller_spec.rb | 17 ++++++++++- .../recaptcha_experiment_helper_spec.rb | 23 ++++++++++++++ 7 files changed, 74 insertions(+), 13 deletions(-) create mode 100644 app/helpers/recaptcha_experiment_helper.rb create mode 100644 spec/helpers/recaptcha_experiment_helper_spec.rb diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 07b38371ab9..b2b151bbcf0 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -3,6 +3,7 @@ class RegistrationsController < Devise::RegistrationsController include Recaptcha::Verify include AcceptsPendingInvitations + include RecaptchaExperimentHelper prepend_before_action :check_captcha, only: :create before_action :whitelist_query_limiting, only: [:destroy] @@ -15,13 +16,6 @@ class RegistrationsController < Devise::RegistrationsController end def create - # To avoid duplicate form fields on the login page, the registration form - # names fields using `new_user`, but Devise still wants the params in - # `user`. - if params["new_#{resource_name}"].present? && params[resource_name].blank? - params[resource_name] = params.delete(:"new_#{resource_name}") - end - accept_pending_invitations super do |new_user| @@ -74,19 +68,35 @@ class RegistrationsController < Devise::RegistrationsController end def after_sign_up_path_for(user) - Gitlab::AppLogger.info("User Created: username=#{user.username} email=#{user.email} ip=#{request.remote_ip} confirmed:#{user.confirmed?}") + Gitlab::AppLogger.info(user_created_message(confirmed: user.confirmed?)) user.confirmed? ? stored_location_for(user) || dashboard_projects_path : users_almost_there_path end def after_inactive_sign_up_path_for(resource) - Gitlab::AppLogger.info("User Created: username=#{resource.username} email=#{resource.email} ip=#{request.remote_ip} confirmed:false") + Gitlab::AppLogger.info(user_created_message) users_almost_there_path end private + def user_created_message(confirmed: false) + "User Created: username=#{resource.username} email=#{resource.email} ip=#{request.remote_ip} confirmed:#{confirmed}" + end + + def ensure_correct_params! + # To avoid duplicate form fields on the login page, the registration form + # names fields using `new_user`, but Devise still wants the params in + # `user`. + if params["new_#{resource_name}"].present? && params[resource_name].blank? + params[resource_name] = params.delete(:"new_#{resource_name}") + end + end + def check_captcha - return unless Feature.enabled?(:registrations_recaptcha, default_enabled: true) + ensure_correct_params! + + return unless Feature.enabled?(:registrations_recaptcha, default_enabled: true) # reCAPTCHA on the UI will still display however + return unless show_recaptcha_sign_up? return unless Gitlab::Recaptcha.load_configurations! return if verify_recaptcha diff --git a/app/helpers/recaptcha_experiment_helper.rb b/app/helpers/recaptcha_experiment_helper.rb new file mode 100644 index 00000000000..d2eb9ac54f6 --- /dev/null +++ b/app/helpers/recaptcha_experiment_helper.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module RecaptchaExperimentHelper + def show_recaptcha_sign_up? + !!Gitlab::Recaptcha.enabled? + end +end diff --git a/app/views/admin/application_settings/_spam.html.haml b/app/views/admin/application_settings/_spam.html.haml index a34fc15acb1..d24e46b2815 100644 --- a/app/views/admin/application_settings/_spam.html.haml +++ b/app/views/admin/application_settings/_spam.html.haml @@ -7,7 +7,10 @@ = f.check_box :recaptcha_enabled, class: 'form-check-input' = f.label :recaptcha_enabled, class: 'form-check-label' do Enable reCAPTCHA - %span.form-text.text-muted#recaptcha_help_block Helps prevent bots from creating accounts + - recaptcha_v2_link_url = 'https://developers.google.com/recaptcha/docs/versions' + - recaptcha_v2_link_start = ''.html_safe % { url: recaptcha_v2_link_url } + %span.form-text.text-muted#recaptcha_help_block + = _('Helps prevent bots from creating accounts. We currently only support %{recaptcha_v2_link_start}reCAPTCHA v2%{recaptcha_v2_link_end}').html_safe % { recaptcha_v2_link_start: recaptcha_v2_link_start, recaptcha_v2_link_end: ''.html_safe } .form-group = f.label :recaptcha_site_key, 'reCAPTCHA Site Key', class: 'label-bold' diff --git a/app/views/devise/shared/_signup_box.html.haml b/app/views/devise/shared/_signup_box.html.haml index eae3ee6339f..034273558bb 100644 --- a/app/views/devise/shared/_signup_box.html.haml +++ b/app/views/devise/shared/_signup_box.html.haml @@ -33,7 +33,7 @@ = accept_terms_label.html_safe = render_if_exists 'devise/shared/email_opted_in', f: f %div - - if Gitlab::Recaptcha.enabled? + - if show_recaptcha_sign_up? = recaptcha_tags .submit-container = f.submit _("Register"), class: "btn-register btn qa-new-user-register-button" diff --git a/locale/gitlab.pot b/locale/gitlab.pot index fd6d3ed624c..1a9e8ae7288 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5016,6 +5016,9 @@ msgstr "" msgid "Help page text and support page url." msgstr "" +msgid "Helps prevent bots from creating accounts. We currently only support %{recaptcha_v2_link_start}reCAPTCHA v2%{recaptcha_v2_link_end}" +msgstr "" + msgid "Hide archived projects" msgstr "" diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index 9a598790ff2..faf3c990cb2 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -6,7 +6,8 @@ describe RegistrationsController do include TermsHelper describe '#create' do - let(:user_params) { { user: { name: 'new_user', username: 'new_username', email: 'new@user.com', password: 'Any_password' } } } + let(:base_user_params) { { name: 'new_user', username: 'new_username', email: 'new@user.com', password: 'Any_password' } } + let(:user_params) { { user: base_user_params } } context 'email confirmation' do around do |example| @@ -105,6 +106,20 @@ describe RegistrationsController do expect(subject.current_user.terms_accepted?).to be(true) end end + + it "logs a 'User Created' message" do + stub_feature_flags(registrations_recaptcha: false) + + expect(Gitlab::AppLogger).to receive(:info).with(/\AUser Created: username=new_username email=new@user.com.+\z/).and_call_original + + post(:create, params: user_params) + end + + it 'handles when params are new_user' do + post(:create, params: { new_user: base_user_params }) + + expect(subject.current_user).not_to be_nil + end end describe '#destroy' do diff --git a/spec/helpers/recaptcha_experiment_helper_spec.rb b/spec/helpers/recaptcha_experiment_helper_spec.rb new file mode 100644 index 00000000000..775c2caa082 --- /dev/null +++ b/spec/helpers/recaptcha_experiment_helper_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe RecaptchaExperimentHelper, type: :helper do + describe '.show_recaptcha_sign_up?' do + context 'when reCAPTCHA is disabled' do + it 'returns false' do + stub_application_setting(recaptcha_enabled: false) + + expect(helper.show_recaptcha_sign_up?).to be(false) + end + end + + context 'when reCAPTCHA is enabled' do + it 'returns true' do + stub_application_setting(recaptcha_enabled: true) + + expect(helper.show_recaptcha_sign_up?).to be(true) + end + end + end +end