Commit ca1c492b authored by Drew Blessing's avatar Drew Blessing

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.
parent de25604f
...@@ -7,7 +7,6 @@ class RegistrationsController < Devise::RegistrationsController ...@@ -7,7 +7,6 @@ class RegistrationsController < Devise::RegistrationsController
end end
def create def create
if !Gitlab::Recaptcha.load_configurations! || verify_recaptcha
# To avoid duplicate form fields on the login page, the registration form # To avoid duplicate form fields on the login page, the registration form
# names fields using `new_user`, but Devise still wants the params in # names fields using `new_user`, but Devise still wants the params in
# `user`. # `user`.
...@@ -15,9 +14,10 @@ class RegistrationsController < Devise::RegistrationsController ...@@ -15,9 +14,10 @@ class RegistrationsController < Devise::RegistrationsController
params[resource_name] = params.delete(:"new_#{resource_name}") params[resource_name] = params.delete(:"new_#{resource_name}")
end end
if !Gitlab::Recaptcha.load_configurations! || verify_recaptcha
super super
else 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 flash.delete :recaptcha_error
render action: 'new' render action: 'new'
end end
......
---
title: Properly handle failed reCAPTCHA on user registration
merge_request: 8403
author:
...@@ -2,31 +2,61 @@ require 'spec_helper' ...@@ -2,31 +2,61 @@ require 'spec_helper'
describe RegistrationsController do describe RegistrationsController do
describe '#create' 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| around(:each) do |example|
perform_enqueued_jobs do perform_enqueued_jobs do
example.run example.run
end end
end end
let(:user_params) { { user: { name: "new_user", username: "new_username", email: "new@user.com", password: "Any_password" } } } context 'when send_user_confirmation_email is false' do
it 'signs the user in' do
context 'when sending email confirmation' do allow_any_instance_of(ApplicationSetting).to receive(:send_user_confirmation_email).and_return(false)
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 { post(:create, user_params) }.not_to change{ ActionMailer::Base.deliveries.size }
expect(subject.current_user).not_to be_nil expect(subject.current_user).not_to be_nil
end end
end end
context 'when not sending email confirmation' do context 'when send_user_confirmation_email is true' do
before { allow_any_instance_of(ApplicationSetting).to receive(:send_user_confirmation_email).and_return(true) }
it 'does not authenticate user and sends confirmation email' 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) post(:create, user_params)
expect(ActionMailer::Base.deliveries.last.to.first).to eq(user_params[:user][:email]) expect(ActionMailer::Base.deliveries.last.to.first).to eq(user_params[:user][:email])
expect(subject.current_user).to be_nil expect(subject.current_user).to be_nil
end end
end 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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment