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

New user signup with/without reCAPTCHA A/B test

See merge request gitlab-org/gitlab-ee!14019

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

app/helpers/recaptcha_experiment_helper.rb

+# frozen_string_literal: true
+
+module RecaptchaExperimentHelper
+  def show_recaptcha_sign_up?
+    !!Gitlab::Recaptcha.enabled?
+  end
+end
+
+RecaptchaExperimentHelper.prepend(EE::RecaptchaExperimentHelper)

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'

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"

ee/app/controllers/ee/registrations_controller.rb

@@ -3,9 +3,17 @@
 module EE
   module RegistrationsController
     extend ActiveSupport::Concern
+    extend ::Gitlab::Utils::Override
 
     private
 
+    override :user_created_message
+    def user_created_message(confirmed: false)
+      experiments = "experiment_growth_recaptcha?#{show_recaptcha_sign_up?}"
+
+      super(confirmed: confirmed) + ", experiments:#{experiments}"
+    end
+
     def sign_up_params
       clean_params = params.require(:user).permit(:username, :email, :email_confirmation, :name, :password, :email_opted_in)
 

ee/app/helpers/ee/recaptcha_experiment_helper.rb

+# frozen_string_literal: true
+
+module EE
+  module RecaptchaExperimentHelper
+    include FlipperSessionHelper
+    extend ::Gitlab::Utils::Override
+
+    EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME = :experiment_growth_recaptcha
+
+    override :show_recaptcha_sign_up?
+    def show_recaptcha_sign_up?
+      super && experiment_enabled_for_session?
+    end
+
+    private
+
+    def experiment_enabled_for_session?
+      # If EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME is not set, we should show
+      # reCAPTCHA on the sign_up page
+      return true unless recaptcha_sign_up_experiment_set?
+
+      ::Feature.enabled?(EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME, flipper_session)
+    end
+
+    def recaptcha_sign_up_experiment_set?
+      ::Feature.persisted?(recaptcha_sign_up_experiment_feature)
+    end
+
+    def recaptcha_sign_up_experiment_feature
+      ::Feature.get(EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME)
+    end
+  end
+end

ee/spec/controllers/registrations_controller_spec.rb

@@ -27,5 +27,16 @@ describe RegistrationsController do
         expect(user.email_opted_in_at).to be_nil
       end
     end
+
+    context 'when recaptcha experiment enabled' do
+      it "logs a 'User Created' message including the experiment state" do
+        user_params = { user: attributes_for(:user) }
+        allow_any_instance_of(EE::RecaptchaExperimentHelper).to receive(:show_recaptcha_sign_up?).and_return(true)
+
+        expect(Gitlab::AppLogger).to receive(:info).with(/\AUser Created: .+experiment_growth_recaptcha\?true\z/).and_call_original
+
+        post :create, params: user_params
+      end
+    end
   end
 end

ee/spec/helpers/ee/recaptcha_experiment_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe RecaptchaExperimentHelper, type: :helper do
+  using RSpec::Parameterized::TableSyntax
+
+  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
+      before do
+        stub_application_setting(recaptcha_enabled: true)
+      end
+
+      context 'and experiment_growth_recaptcha has not been set' do
+        it 'returns true' do
+          expect(helper.show_recaptcha_sign_up?).to be(true)
+        end
+      end
+
+      context 'and experiment_growth_recaptcha has been set' do
+        let(:feature_name) { described_class::EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME }
+
+        before(:all) do
+          # We need to create a '50%' of actors feature flag before _any_ test
+          # runs and need to continue to use the same feature throughout the
+          # test duration.
+          fifty_percent = ::Feature.flipper.actors(50)
+          ::Feature.flipper[described_class::EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME].enable(fifty_percent)
+        end
+
+        after(:all) do
+          Feature.flipper.remove(described_class::EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME)
+        end
+
+        where(:flipper_session_id, :expected_result) do
+          '00687625-667c-480c-ae2a-9bf861ddd909' | true
+          'b8b78156-f7b8-4bf4-b906-06a899b84ea3' | false
+          'e622a262-6e48-41ba-b19f-2d91c24f17a3' | true
+          'd2c0aae1-bc08-4000-bbb2-0dd2802f67e2' | false
+        end
+
+        with_them do
+          it "returns expected_result" do
+            allow(Feature).to receive(:enabled?).and_call_original # Allow Feature to _really_ work.
+            allow(helper).to receive(:flipper_session).and_return(FlipperSession.new(flipper_session_id))
+
+            expect(helper.show_recaptcha_sign_up?).to eq(expected_result)
+          end
+        end
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -6803,6 +6803,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 ""
 

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

spec/helpers/recaptcha_experiment_helper_spec.rb

+# 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