diff options
-rw-r--r-- | .prettierrc | 1 | ||||
-rw-r--r-- | app/controllers/omniauth_callbacks_controller.rb | 12 | ||||
-rw-r--r-- | changelogs/unreleased/43525-limit-number-of-failed-logins-using-ldap.yml | 5 | ||||
-rw-r--r-- | doc/development/i18n/proofreader.md | 1 | ||||
-rw-r--r-- | spec/controllers/omniauth_callbacks_controller_spec.rb | 122 |
5 files changed, 98 insertions, 43 deletions
diff --git a/.prettierrc b/.prettierrc index a20502b7f06..5e2863a11f6 100644 --- a/.prettierrc +++ b/.prettierrc @@ -1,4 +1,5 @@ { + "printWidth": 100, "singleQuote": true, "trailingComma": "all" } diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index fff249577a2..5e6676ea513 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -18,6 +18,18 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController end end + # Extend the standard implementation to also increment + # the number of failed sign in attempts + def failure + if params[:username].present? && AuthHelper.form_based_provider?(failed_strategy.name) + user = User.by_login(params[:username]) + + user&.increment_failed_attempts! + end + + super + end + # Extend the standard message generation to accept our custom exception def failure_message exception = env["omniauth.error"] diff --git a/changelogs/unreleased/43525-limit-number-of-failed-logins-using-ldap.yml b/changelogs/unreleased/43525-limit-number-of-failed-logins-using-ldap.yml new file mode 100644 index 00000000000..f30fea3c4a7 --- /dev/null +++ b/changelogs/unreleased/43525-limit-number-of-failed-logins-using-ldap.yml @@ -0,0 +1,5 @@ +--- +title: Limit the number of failed logins when using LDAP for authentication +merge_request: 43525 +author: +type: added diff --git a/doc/development/i18n/proofreader.md b/doc/development/i18n/proofreader.md index 960eabd5538..cf62314bc29 100644 --- a/doc/development/i18n/proofreader.md +++ b/doc/development/i18n/proofreader.md @@ -10,6 +10,7 @@ are very appreciative of the work done by translators and proofreaders! - Huang Tao - [GitLab](https://gitlab.com/htve), [Crowdin](https://crowdin.com/profile/htve) - Chinese Traditional - Huang Tao - [GitLab](https://gitlab.com/htve), [Crowdin](https://crowdin.com/profile/htve) + - Weizhe Ding - [GitLab](https://gitlab.com/d.weizhe), [Crowdin](https://crowdin.com/profile/d.weizhe) - Chinese Traditional, Hong Kong - Huang Tao - [GitLab](https://gitlab.com/htve), [Crowdin](https://crowdin.com/profile/htve) - Dutch diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index 9fd129e4ee9..5f0e8c5eca9 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -10,83 +10,119 @@ describe OmniauthCallbacksController do stub_omniauth_provider(provider, context: request) end - context 'github' do + context 'when the user is on the last sign in attempt' do let(:extern_uid) { 'my-uid' } - let(:provider) { :github } - it 'allows sign in' do - post provider - - expect(request.env['warden']).to be_authenticated + before do + user.update(failed_attempts: User.maximum_attempts.pred) + subject.response = ActionDispatch::Response.new end - shared_context 'sign_up' do - let(:user) { double(email: 'new@example.com') } + context 'when using a form based provider' do + let(:provider) { :ldap } + + it 'locks the user when sign in fails' do + allow(subject).to receive(:params).and_return(ActionController::Parameters.new(username: user.username)) + request.env['omniauth.error.strategy'] = OmniAuth::Strategies::LDAP.new(nil) + + subject.send(:failure) - before do - stub_omniauth_setting(block_auto_created_users: false) + expect(user.reload).to be_access_locked end end - context 'sign up' do - include_context 'sign_up' + context 'when using a button based provider' do + let(:provider) { :github } - it 'is allowed' do - post provider + it 'does not lock the user when sign in fails' do + request.env['omniauth.error.strategy'] = OmniAuth::Strategies::GitHub.new(nil) - expect(request.env['warden']).to be_authenticated + subject.send(:failure) + + expect(user.reload).not_to be_access_locked end end + end - context 'when OAuth is disabled' do - before do - stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') - settings = Gitlab::CurrentSettings.current_application_settings - settings.update(disabled_oauth_sign_in_sources: [provider.to_s]) - end + context 'strategies' do + context 'github' do + let(:extern_uid) { 'my-uid' } + let(:provider) { :github } - it 'prevents login via POST' do + it 'allows sign in' do post provider - expect(request.env['warden']).not_to be_authenticated + expect(request.env['warden']).to be_authenticated end - it 'shows warning when attempting login' do - post provider + shared_context 'sign_up' do + let(:user) { double(email: 'new@example.com') } - expect(response).to redirect_to new_user_session_path - expect(flash[:alert]).to eq('Signing in using GitHub has been disabled') + before do + stub_omniauth_setting(block_auto_created_users: false) + end end - it 'allows linking the disabled provider' do - user.identities.destroy_all - sign_in(user) + context 'sign up' do + include_context 'sign_up' + + it 'is allowed' do + post provider - expect { post provider }.to change { user.reload.identities.count }.by(1) + expect(request.env['warden']).to be_authenticated + end end - context 'sign up' do - include_context 'sign_up' + context 'when OAuth is disabled' do + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + settings = Gitlab::CurrentSettings.current_application_settings + settings.update(disabled_oauth_sign_in_sources: [provider.to_s]) + end - it 'is prevented' do + it 'prevents login via POST' do post provider expect(request.env['warden']).not_to be_authenticated end + + it 'shows warning when attempting login' do + post provider + + expect(response).to redirect_to new_user_session_path + expect(flash[:alert]).to eq('Signing in using GitHub has been disabled') + end + + it 'allows linking the disabled provider' do + user.identities.destroy_all + sign_in(user) + + expect { post provider }.to change { user.reload.identities.count }.by(1) + end + + context 'sign up' do + include_context 'sign_up' + + it 'is prevented' do + post provider + + expect(request.env['warden']).not_to be_authenticated + end + end end end - end - context 'auth0' do - let(:extern_uid) { '' } - let(:provider) { :auth0 } + context 'auth0' do + let(:extern_uid) { '' } + let(:provider) { :auth0 } - it 'does not allow sign in without extern_uid' do - post 'auth0' + it 'does not allow sign in without extern_uid' do + post 'auth0' - expect(request.env['warden']).not_to be_authenticated - expect(response.status).to eq(302) - expect(controller).to set_flash[:alert].to('Wrong extern UID provided. Make sure Auth0 is configured correctly.') + expect(request.env['warden']).not_to be_authenticated + expect(response.status).to eq(302) + expect(controller).to set_flash[:alert].to('Wrong extern UID provided. Make sure Auth0 is configured correctly.') + end end end end |