Merge branch 'ashmckenzie/new-user-signup-ab-test' into 'master'

CE port of new user signup with/without reCAPTCHA A/B test (no-op)

See merge request gitlab-org/gitlab-ce!29341

7 files changed, 74 insertions, 13 deletions

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 = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.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: '</a>'.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