From ca1c492bafa024ad4c185e930af57e1f8090ca77 Mon Sep 17 00:00:00 2001 From: Drew Blessing 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