From ca1c492bafa024ad4c185e930af57e1f8090ca77 Mon Sep 17 00:00:00 2001
From: Drew Blessing <drew@gitlab.com>
Date: Tue, 3 Jan 2017 01:08:21 -0600
Subject: [PATCH] Properly handle failed reCAPTCHA on user registration

If a user presses the 'Register' button too quickly after attempting
to solve the reCAPTCHA, or the reCAPTCHA is not solved at all, the
user would experience a 500 error. Now, the case is properly
handled and the user will be sent back to the registration page
with a clear error message and can try again.
---
 app/controllers/registrations_controller.rb   | 16 ++---
 changelogs/unreleased/recaptcha_500.yml       |  4 ++
 .../registrations_controller_spec.rb          | 64 ++++++++++++++-----
 3 files changed, 59 insertions(+), 25 deletions(-)
 create mode 100644 changelogs/unreleased/recaptcha_500.yml

diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb
index c45196cc3e9..a16dbb20cdf 100644
--- a/app/controllers/registrations_controller.rb
+++ b/app/controllers/registrations_controller.rb
@@ -7,17 +7,17 @@ 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`.
-      if params["new_#{resource_name}"].present? && params[resource_name].blank?
-        params[resource_name] = params.delete(:"new_#{resource_name}")
-      end
+    # 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
 
+    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
diff --git a/changelogs/unreleased/recaptcha_500.yml b/changelogs/unreleased/recaptcha_500.yml
new file mode 100644
index 00000000000..de9ef183d5e
--- /dev/null
+++ b/changelogs/unreleased/recaptcha_500.yml
@@ -0,0 +1,4 @@
+---
+title: Properly handle failed reCAPTCHA on user registration
+merge_request: 8403
+author:
diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb
index 026f41c926b..42fbfe89368 100644
--- a/spec/controllers/registrations_controller_spec.rb
+++ b/spec/controllers/registrations_controller_spec.rb
@@ -2,30 +2,60 @@ require 'spec_helper'
 
 describe RegistrationsController do
   describe '#create' do
-    around(:each) do |example|
-      perform_enqueued_jobs do
-        example.run
+    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
+
+      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)
+
+          expect { post(:create, user_params) }.not_to change{ ActionMailer::Base.deliveries.size }
+          expect(subject.current_user).not_to be_nil
+        end
+      end
+
+      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
 
-    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) }
-
-      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
+    context 'when reCAPTCHA is enabled' do
+      before do
+        stub_application_setting(recaptcha_enabled: true)
       end
-    end
 
-    context 'when not sending email confirmation' do
-      before { allow_any_instance_of(ApplicationSetting).to receive(:send_user_confirmation_email).and_return(true) }
+      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')
 
-      it 'does not authenticate user and sends confirmation email' do
         post(:create, user_params)
-        expect(ActionMailer::Base.deliveries.last.to.first).to eq(user_params[:user][:email])
-        expect(subject.current_user).to be_nil
+
+        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