Merge branch 'recaptcha_500' into 'master'

Properly handle failed reCAPTCHA on user registration

See merge request !8403

app/controllers/registrations_controller.rb

@@ -7,7 +7,6 @@ class RegistrationsController < Devise::RegistrationsController
   end
 
   def create
-    if !Gitlab::Recaptcha.load_configurations! || verify_recaptcha
     # 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`.
@@ -15,9 +14,10 @@ class RegistrationsController < Devise::RegistrationsController
       params[resource_name] = params.delete(:"new_#{resource_name}")
     end
 
+    if !Gitlab::Recaptcha.load_configurations! || verify_recaptcha
       super
     else
-      flash[:alert] = "There was an error with the reCAPTCHA code below. Please re-enter the code."
+      flash[:alert] = 'There was an error with the reCAPTCHA. Please re-solve the reCAPTCHA.'
       flash.delete :recaptcha_error
       render action: 'new'
     end

changelogs/unreleased/recaptcha_500.yml

+---
+title: Properly handle failed reCAPTCHA on user registration
+merge_request: 8403
+author:

spec/controllers/registrations_controller_spec.rb

@@ -2,31 +2,61 @@ require 'spec_helper'
 
 describe RegistrationsController do
   describe '#create' do
+    let(:user_params) { { user: { name: 'new_user', username: 'new_username', email: 'new@user.com', password: 'Any_password' } } }
+
+    context 'email confirmation' do
       around(:each) do |example|
         perform_enqueued_jobs do
           example.run
         end
       end
 
-    let(:user_params) { { user: { name: "new_user", username: "new_username", email: "new@user.com", password: "Any_password" } } }
-
-    context 'when sending email confirmation' do
-      before { allow_any_instance_of(ApplicationSetting).to receive(:send_user_confirmation_email).and_return(false) }
+      context 'when send_user_confirmation_email is false' do
+        it 'signs the user in' do
+          allow_any_instance_of(ApplicationSetting).to receive(:send_user_confirmation_email).and_return(false)
 
-      it 'logs user in directly' do
           expect { post(:create, user_params) }.not_to change{ ActionMailer::Base.deliveries.size }
           expect(subject.current_user).not_to be_nil
         end
       end
 
-    context 'when not sending email confirmation' do
-      before { allow_any_instance_of(ApplicationSetting).to receive(:send_user_confirmation_email).and_return(true) }
-
+      context 'when send_user_confirmation_email is true' do
         it 'does not authenticate user and sends confirmation email' do
+          allow_any_instance_of(ApplicationSetting).to receive(:send_user_confirmation_email).and_return(true)
+
           post(:create, user_params)
+
           expect(ActionMailer::Base.deliveries.last.to.first).to eq(user_params[:user][:email])
           expect(subject.current_user).to be_nil
         end
       end
     end
+
+    context 'when reCAPTCHA is enabled' do
+      before do
+        stub_application_setting(recaptcha_enabled: true)
+      end
+
+      it 'displays an error when the reCAPTCHA is not solved' do
+        # Without this, `verify_recaptcha` arbitraily returns true in test env
+        Recaptcha.configuration.skip_verify_env.delete('test')
+
+        post(:create, user_params)
+
+        expect(response).to render_template(:new)
+        expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please re-solve the reCAPTCHA.'
+      end
+
+      it 'redirects to the dashboard when the recaptcha is solved' do
+        # Avoid test ordering issue and ensure `verify_recaptcha` returns true
+        unless Recaptcha.configuration.skip_verify_env.include?('test')
+          Recaptcha.configuration.skip_verify_env << 'test'
+        end
+
+        post(:create, user_params)
+
+        expect(flash[:notice]).to include 'Welcome! You have signed up successfully.'
+      end
+    end
+  end
 end