From 1e877029c18a9a93ffa4e446c016ba981a9af4d9 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 21 Jun 2018 23:17:38 +0000 Subject: Merge branch 'sh-add-recaptcha-all-logins' into 'master' Show a reCAPTCHA on signin page if custom header is set Closes #48184 See merge request gitlab-org/gitlab-ce!20076 --- app/controllers/sessions_controller.rb | 31 ++++++++++++++++- app/views/devise/sessions/_new_base.html.haml | 4 +++ doc/integration/recaptcha.md | 19 ++++++++++- spec/controllers/sessions_controller_spec.rb | 39 +++++++++++++++++++--- .../devise/shared/_signin_box.html.haml_spec.rb | 1 + 5 files changed, 88 insertions(+), 6 deletions(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 1a339f76d26..7aa277b3614 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -3,21 +3,27 @@ class SessionsController < Devise::SessionsController include AuthenticatesWithTwoFactor include Devise::Controllers::Rememberable include Recaptcha::ClientHelper + include Recaptcha::Verify skip_before_action :check_two_factor_requirement, only: [:destroy] prepend_before_action :check_initial_setup, only: [:new] prepend_before_action :authenticate_with_two_factor, if: :two_factor_enabled?, only: [:create] + prepend_before_action :check_captcha, only: [:create] prepend_before_action :store_redirect_uri, only: [:new] + prepend_before_action :ldap_servers, only: [:new, :create] before_action :auto_sign_in_with_provider, only: [:new] before_action :load_recaptcha after_action :log_failed_login, only: [:new], if: :failed_login? + helper_method :captcha_enabled? + + CAPTCHA_HEADER = 'X-GitLab-Show-Login-Captcha'.freeze + def new set_minimum_password_length - @ldap_servers = Gitlab::Auth::LDAP::Config.available_servers super end @@ -46,6 +52,25 @@ class SessionsController < Devise::SessionsController private + def captcha_enabled? + request.headers[CAPTCHA_HEADER] && Gitlab::Recaptcha.enabled? + end + + # From https://github.com/plataformatec/devise/wiki/How-To:-Use-Recaptcha-with-Devise#devisepasswordscontroller + def check_captcha + return unless user_params[:password].present? + return unless captcha_enabled? + return unless Gitlab::Recaptcha.load_configurations! + + unless verify_recaptcha + self.resource = resource_class.new + flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' + flash.delete :recaptcha_error + + respond_with_navigational(resource) { render :new } + end + end + def log_failed_login Gitlab::AppLogger.info("Failed Login: username=#{user_params[:login]} ip=#{request.remote_ip}") end @@ -152,6 +177,10 @@ class SessionsController < Devise::SessionsController Gitlab::Recaptcha.load_configurations! end + def ldap_servers + @ldap_servers ||= Gitlab::Auth::LDAP::Config.available_servers + end + def authentication_method if user_params[:otp_attempt] "two-factor" diff --git a/app/views/devise/sessions/_new_base.html.haml b/app/views/devise/sessions/_new_base.html.haml index c45d2214592..0ee563ac066 100644 --- a/app/views/devise/sessions/_new_base.html.haml +++ b/app/views/devise/sessions/_new_base.html.haml @@ -12,5 +12,9 @@ %span Remember me .float-right.forgot-password = link_to "Forgot your password?", new_password_path(:user) + %div + - if captcha_enabled? + = recaptcha_tags + .submit-container.move-submit-down = f.submit "Sign in", class: "btn btn-save" diff --git a/doc/integration/recaptcha.md b/doc/integration/recaptcha.md index a301d1a613c..932cd479d56 100644 --- a/doc/integration/recaptcha.md +++ b/doc/integration/recaptcha.md @@ -20,4 +20,21 @@ To use reCAPTCHA, first you must create a site and private key. 6. Check the `Enable reCAPTCHA` checkbox -7. Save the configuration. +7. Save the configuration. + +## Enabling reCAPTCHA for user logins via passwords + +By default, reCAPTCHA is only enabled for user registrations. To enable it for +user logins via passwords, the `X-GitLab-Show-Login-Captcha` HTTP header must +be set. For example, in NGINX, this can be done via the `proxy_set_header` +configuration variable: + +``` +proxy_set_header X-GitLab-Show-Login-Captcha 1; +``` + +In GitLab Omnibus, this can be configured via `/etc/gitlab/gitlab.rb`: + +```ruby +nginx['proxy_set_headers'] = { 'X-GitLab-Show-Login-Captcha' => 1 } +``` diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 555b186fe31..7e048843e2f 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -53,21 +53,22 @@ describe SessionsController do include UserActivitiesHelpers let(:user) { create(:user) } + let(:user_params) { { login: user.username, password: user.password } } it 'authenticates user correctly' do - post(:create, user: { login: user.username, password: user.password }) + post(:create, user: user_params) expect(subject.current_user). to eq user end it 'creates an audit log record' do - expect { post(:create, user: { login: user.username, password: user.password }) }.to change { SecurityEvent.count }.by(1) + expect { post(:create, user: user_params) }.to change { SecurityEvent.count }.by(1) expect(SecurityEvent.last.details[:with]).to eq('standard') end include_examples 'user login request with unique ip limit', 302 do def request - post(:create, user: { login: user.username, password: user.password }) + post(:create, user: user_params) expect(subject.current_user).to eq user subject.sign_out user end @@ -75,10 +76,40 @@ describe SessionsController do it 'updates the user activity' do expect do - post(:create, user: { login: user.username, password: user.password }) + post(:create, user: user_params) end.to change { user_activity(user) } end end + + context 'when reCAPTCHA is enabled' do + let(:user) { create(:user) } + let(:user_params) { { login: user.username, password: user.password } } + + before do + stub_application_setting(recaptcha_enabled: true) + request.headers[described_class::CAPTCHA_HEADER] = 1 + 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: user_params) + + expect(response).to render_template(:new) + expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' + expect(subject.current_user).to be_nil + end + + it 'successfully logs in a user when reCAPTCHA is solved' do + # Avoid test ordering issue and ensure `verify_recaptcha` returns true + Recaptcha.configuration.skip_verify_env << 'test' + + post(:create, user: user_params) + + expect(subject.current_user).to eq user + end + end end context 'when using two-factor authentication via OTP' do diff --git a/spec/views/devise/shared/_signin_box.html.haml_spec.rb b/spec/views/devise/shared/_signin_box.html.haml_spec.rb index 0870b8f09f9..66c064e3fba 100644 --- a/spec/views/devise/shared/_signin_box.html.haml_spec.rb +++ b/spec/views/devise/shared/_signin_box.html.haml_spec.rb @@ -6,6 +6,7 @@ describe 'devise/shared/_signin_box' do stub_devise assign(:ldap_servers, []) allow(view).to receive(:current_application_settings).and_return(Gitlab::CurrentSettings.current_application_settings) + allow(view).to receive(:captcha_enabled?).and_return(false) end it 'is shown when Crowd is enabled' do -- cgit v1.2.1