Commit 8190473e authored by Drew Blessing's avatar Drew Blessing

Unblock LDAP blocked user on sign-in with other auth methods

Many organizations use LDAP in conjunction with other authentication
methods such as SAML or OAuth. If transient LDAP errors cause the
user to become `ldap_blocked` it is desirable to also unblock the
user if the issue resolves itself. Otherwise, the user is unable
to sign-in again without manual intervention or to sign-in once
via LDAP directly. This change enables any sign-in to recheck
LDAP if the user is `ldap_blocked`.

Changelog: added
parent b574ddd8
...@@ -466,7 +466,11 @@ class User < ApplicationRecord ...@@ -466,7 +466,11 @@ class User < ApplicationRecord
end end
def active_for_authentication? def active_for_authentication?
super && can?(:log_in) return false unless super
check_ldap_if_ldap_blocked!
can?(:log_in)
end end
# The messages for these keys are defined in `devise.en.yml` # The messages for these keys are defined in `devise.en.yml`
...@@ -2151,6 +2155,13 @@ class User < ApplicationRecord ...@@ -2151,6 +2155,13 @@ class User < ApplicationRecord
def ci_job_token_scope_cache_key def ci_job_token_scope_cache_key
"users:#{id}:ci:job_token_scope" "users:#{id}:ci:job_token_scope"
end end
# An `ldap_blocked` user will be unblocked if LDAP indicates they are allowed.
def check_ldap_if_ldap_blocked!
return unless ::Gitlab::Auth::Ldap::Config.enabled? && ldap_blocked?
::Gitlab::Auth::Ldap::Access.allowed?(self)
end
end end
User.prepend_mod_with('User') User.prepend_mod_with('User')
...@@ -6,6 +6,7 @@ RSpec.describe User do ...@@ -6,6 +6,7 @@ RSpec.describe User do
include ProjectForksHelper include ProjectForksHelper
include TermsHelper include TermsHelper
include ExclusiveLeaseHelpers include ExclusiveLeaseHelpers
include LdapHelpers
it_behaves_like 'having unique enum values' it_behaves_like 'having unique enum values'
...@@ -5817,7 +5818,7 @@ RSpec.describe User do ...@@ -5817,7 +5818,7 @@ RSpec.describe User do
end end
describe '#active_for_authentication?' do describe '#active_for_authentication?' do
subject { user.active_for_authentication? } subject(:active_for_authentication?) { user.active_for_authentication? }
let(:user) { create(:user) } let(:user) { create(:user) }
...@@ -5827,6 +5828,14 @@ RSpec.describe User do ...@@ -5827,6 +5828,14 @@ RSpec.describe User do
end end
it { is_expected.to be false } it { is_expected.to be false }
it 'does not check if LDAP is allowed' do
stub_ldap_setting(enabled: true)
expect(Gitlab::Auth::Ldap::Access).not_to receive(:allowed?)
active_for_authentication?
end
end end
context 'when user is a ghost user' do context 'when user is a ghost user' do
...@@ -5837,6 +5846,28 @@ RSpec.describe User do ...@@ -5837,6 +5846,28 @@ RSpec.describe User do
it { is_expected.to be false } it { is_expected.to be false }
end end
context 'when user is ldap_blocked' do
before do
user.ldap_block
end
it 'rechecks if LDAP is allowed when LDAP is enabled' do
stub_ldap_setting(enabled: true)
expect(Gitlab::Auth::Ldap::Access).to receive(:allowed?)
active_for_authentication?
end
it 'does not check if LDAP is allowed when LDAP is not enabled' do
stub_ldap_setting(enabled: false)
expect(Gitlab::Auth::Ldap::Access).not_to receive(:allowed?)
active_for_authentication?
end
end
context 'based on user type' do context 'based on user type' do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
......
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