Merge branch...

Merge branch '7309-geo-secondary-with-ldap-configured-can-t-update-last_credential_check_at-due-to-read-only-db' into 'master'

Resolve "Geo:  Secondary with LDAP configured can't update last_credential_check_at due to read-only DB"

Closes #7309

See merge request gitlab-org/gitlab-ee!6965

ee/changelogs/unreleased/7309-geo-secondary-with-ldap-configured-can-t-update-last_credential_check_at-due-to-read-only-db.yml

+---
+title: LDAP - Does not update permissions on a read-only database
+merge_request: 6965
+author:
+type: fixed

ee/lib/ee/gitlab/auth/ldap/access.rb

@@ -6,6 +6,19 @@ module EE
           extend ActiveSupport::Concern
           extend ::Gitlab::Utils::Override
 
+          override :update_user
+          def update_user
+            return if ::Gitlab::Database.read_only?
+
+            update_email
+            update_memberships
+            update_identity
+            update_ssh_keys if sync_ssh_keys?
+            update_kerberos_identity if import_kerberos_identities?
+          end
+
+          private
+
           override :find_ldap_user
           def find_ldap_user
             found_user = super
@@ -16,15 +29,6 @@ module EE
             end
           end
 
-          override :update_user
-          def update_user
-            update_email
-            update_memberships
-            update_identity
-            update_ssh_keys if sync_ssh_keys?
-            update_kerberos_identity if import_kerberos_identities?
-          end
-
           # Update user ssh keys if they changed in LDAP
           def update_ssh_keys
             remove_old_ssh_keys

ee/spec/lib/gitlab/auth/ldap/access_spec.rb

@@ -3,259 +3,252 @@ require 'spec_helper'
 describe Gitlab::Auth::LDAP::Access do
   include LdapHelpers
 
-  let(:access) { described_class.new user }
   let(:user) { create(:omniauth_user) }
+  let(:provider) { user.ldap_identity.provider }
 
-  describe '#find_ldap_user' do
-    it 'finds a user by email if the email came from LDAP' do
-      expect(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(nil)
-      expect(Gitlab::Auth::LDAP::Person).to receive(:find_by_email)
-
-      access.find_ldap_user
-    end
-  end
+  subject(:access) { described_class.new(user) }
 
   describe '#allowed?' do
-    subject { access.allowed? }
+    context 'LDAP user' do
+      it 'finds a user by dn first' do
+        expect(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(:ldap_user)
+        expect(Gitlab::Auth::LDAP::Person).not_to receive(:find_by_email)
 
-    context 'when the user cannot be found' do
-      before do
-        allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(nil)
-        allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(nil)
+        access.allowed?
       end
 
-      context 'when looking for a user by email' do
-        let(:user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'my-provider') }
+      it 'finds a user by email if not found by dn' do
+        expect(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(nil)
+        expect(Gitlab::Auth::LDAP::Person).to receive(:find_by_email)
 
-        it { is_expected.to be_falsey }
+        access.allowed?
+      end
+
+      it 'returns false if user cannot be found' do
+        stub_ldap_person_find_by_dn(nil)
+        stub_ldap_person_find_by_email(user.email, nil)
+
+        expect(access.allowed?).to be_falsey
       end
     end
   end
 
   describe '#update_user' do
-    subject { access.update_user }
-    let(:entry) do
-      Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com")
-    end
-    before do
-      allow(access).to(
-        receive_messages(
-          ldap_user: Gitlab::Auth::LDAP::Person.new(entry, user.ldap_identity.provider)
-        )
-      )
-    end
+    let(:entry) { Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com") }
 
-    it 'updates email address' do
-      expect(access).to receive(:update_email).once
+    context 'email address' do
+      before do
+        stub_ldap_person_find_by_dn(entry, provider)
+      end
 
-      subject
-    end
+      it 'does not update email if email attribute is not set' do
+        expect { access.update_user }.not_to change(user, :email)
+      end
 
-    it 'updates the group memberships' do
-      expect(access).to receive(:update_memberships).once
+      it 'does not update the email if the user has the same email in GitLab and in LDAP' do
+        entry['mail'] = [user.email]
 
-      subject
-    end
+        expect { access.update_user }.not_to change(user, :email)
+      end
 
-    it 'syncs ssh keys if enabled by configuration' do
-      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: true)
-      expect(access).to receive(:update_ssh_keys).once
+      it 'does not update the email if the user has the same email GitLab and in LDAP, but with upper case in LDAP' do
+        entry['mail'] = [user.email.upcase]
 
-      subject
-    end
+        expect { access.update_user }.not_to change(user, :email)
+      end
 
-    it 'update_kerberos_identity' do
-      allow(access).to receive_messages(import_kerberos_identities?: true)
-      expect(access).to receive(:update_kerberos_identity).once
+      it 'does not update the email when in a read-only GitLab instance' do
+        allow(Gitlab::Database).to receive(:read_only?).and_return(true)
 
-      subject
-    end
+        entry['mail'] = ['new_email@example.com']
 
-    it 'updates the ldap identity' do
-      expect(access).to receive(:update_identity)
+        expect { access.update_user }.not_to change(user, :email)
+      end
 
-      subject
-    end
-  end
+      it 'updates the email if the user email is different' do
+        entry['mail'] = ['new_email@example.com']
 
-  describe '#update_kerberos_identity' do
-    let(:entry) do
-      Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com")
+        expect { access.update_user }.to change(user, :email)
+      end
     end
 
-    before do
-      allow(access).to receive_messages(ldap_user: Gitlab::Auth::LDAP::Person.new(entry, user.ldap_identity.provider))
-    end
+    context 'group memberships' do
+      context 'when there is `memberof` param' do
+        before do
+          entry['memberof'] = [
+            'CN=Group1,CN=Users,DC=The dc,DC=com',
+            'CN=Group2,CN=Builtin,DC=The dc,DC=com'
+          ]
 
-    it "adds a Kerberos identity if it is in Active Directory but not in GitLab" do
-      allow_any_instance_of(EE::Gitlab::Auth::LDAP::Person).to receive_messages(kerberos_principal: "mylogin@FOO.COM")
+          stub_ldap_person_find_by_dn(entry, provider)
+        end
 
-      expect { access.update_kerberos_identity }.to change(user.identities.where(provider: :kerberos), :count).from(0).to(1)
-      expect(user.identities.where(provider: "kerberos").last.extern_uid).to eq("mylogin@FOO.COM")
-    end
+        it 'triggers a sync for all groups found in `memberof`' do
+          group_link_1 = create(:ldap_group_link, cn: 'Group1', provider: provider)
+          group_link_2 = create(:ldap_group_link, cn: 'Group2', provider: provider)
+          group_ids = [group_link_1, group_link_2].map(&:group_id)
 
-    it "updates existing Kerberos identity in GitLab if Active Directory has a different one" do
-      allow_any_instance_of(EE::Gitlab::Auth::LDAP::Person).to receive_messages(kerberos_principal: "otherlogin@BAR.COM")
-      user.identities.build(provider: "kerberos", extern_uid: "mylogin@FOO.COM").save
+          expect(LdapGroupSyncWorker).to receive(:perform_async)
+            .with(a_collection_containing_exactly(*group_ids), provider)
 
-      expect { access.update_kerberos_identity }.not_to change(user.identities.where(provider: "kerberos"), :count)
-      expect(user.identities.where(provider: "kerberos").last.extern_uid).to eq("otherlogin@BAR.COM")
-    end
+          access.update_user
+        end
 
-    it "does not remove Kerberos identities from GitLab if they are none in the LDAP provider" do
-      allow_any_instance_of(EE::Gitlab::Auth::LDAP::Person).to receive_messages(kerberos_principal: nil)
-      user.identities.build(provider: "kerberos", extern_uid: "otherlogin@BAR.COM").save
+        it "doesn't trigger a sync when in a read-only GitLab instance" do
+          allow(Gitlab::Database).to receive(:read_only?).and_return(true)
+          create(:ldap_group_link, cn: 'Group1', provider: provider)
+          create(:ldap_group_link, cn: 'Group2', provider: provider)
 
-      expect { access.update_kerberos_identity }.not_to change(user.identities.where(provider: "kerberos"), :count)
-      expect(user.identities.where(provider: "kerberos").last.extern_uid).to eq("otherlogin@BAR.COM")
-    end
+          expect(LdapGroupSyncWorker).not_to receive(:perform_async)
 
-    it "does not modify identities in GitLab if they are no kerberos principal in the LDAP provider" do
-      allow_any_instance_of(EE::Gitlab::Auth::LDAP::Person).to receive_messages(kerberos_principal: nil)
+          access.update_user
+        end
 
-      expect { access.update_kerberos_identity }.not_to change(user.identities, :count)
-    end
-  end
+        it "doesn't trigger a sync when there are no links for the provider" do
+          _another_provider = create(:ldap_group_link,
+                                     cn: 'Group1',
+                                     provider: 'not-this-ldap')
 
-  describe '#update_ssh_keys' do
-    let(:ssh_key) { "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCrSQHff6a1rMqBdHFt+FwIbytMZ+hJKN3KLkTtOWtSvNIriGhnTdn4rs+tjD/w+z+revytyWnMDM9dS7J8vQi006B16+hc9Xf82crqRoPRDnBytgAFFQY1G/55ql2zdfsC5yvpDOFzuwIJq5dNGsojS82t6HNmmKPq130fzsenFnj5v1pl3OJvk513oduUyKiZBGTroWTn7H/eOPtu7s9MD7pAdEjqYKFLeaKmyidiLmLqQlCRj3Tl2U9oyFg4PYNc0bL5FZJ/Z6t0Ds3i/a2RanQiKxrvgu3GSnUKMx7WIX373baL4jeM7cprRGiOY/1NcS+1cAjfJ8oaxQF/1dYj" }
-    let(:ssh_key_attribute_name) { 'altSecurityIdentities' }
-    let(:entry) do
-      Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: SSHKey:#{ssh_key}\n#{ssh_key_attribute_name}: KerberosKey:bogus")
-    end
-
-    before do
-      allow_any_instance_of(Gitlab::Auth::LDAP::Config).to receive_messages(sync_ssh_keys: ssh_key_attribute_name)
-      allow(access).to receive_messages(sync_ssh_keys?: true)
-    end
+          expect(LdapGroupSyncWorker).not_to receive(:perform_async)
 
-    it "adds a SSH key if it is in LDAP but not in gitlab" do
-      allow_any_instance_of(Gitlab::Auth::LDAP::Adapter).to receive(:user) { Gitlab::Auth::LDAP::Person.new(entry, 'ldapmain') }
+          access.update_user
+        end
+      end
 
-      expect { access.update_ssh_keys }.to change(user.keys, :count).from(0).to(1)
-    end
+      it "doesn't continue when there is no `memberOf` param" do
+        stub_ldap_person_find_by_dn(entry, provider)
 
-    it "adds a SSH key and give it a proper name" do
-      allow_any_instance_of(Gitlab::Auth::LDAP::Adapter).to receive(:user) { Gitlab::Auth::LDAP::Person.new(entry, 'ldapmain') }
+        expect(LdapGroupLink).not_to receive(:where)
+        expect(LdapGroupSyncWorker).not_to receive(:perform_async)
 
-      access.update_ssh_keys
-      expect(user.keys.last.title).to match(/LDAP/)
-      expect(user.keys.last.title).to match(/#{access.ldap_config.sync_ssh_keys}/)
+        access.update_user
+      end
     end
 
-    it "does not add a SSH key if it is invalid" do
-      entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: I am not a valid key")
-      allow_any_instance_of(Gitlab::Auth::LDAP::Adapter).to receive(:user) { Gitlab::Auth::LDAP::Person.new(entry, 'ldapmain') }
-
-      expect { access.update_ssh_keys }.not_to change(user.keys, :count)
-    end
+    context 'SSH keys' do
+      let(:ssh_key) { 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCrSQHff6a1rMqBdHFt+FwIbytMZ+hJKN3KLkTtOWtSvNIriGhnTdn4rs+tjD/w+z+revytyWnMDM9dS7J8vQi006B16+hc9Xf82crqRoPRDnBytgAFFQY1G/55ql2zdfsC5yvpDOFzuwIJq5dNGsojS82t6HNmmKPq130fzsenFnj5v1pl3OJvk513oduUyKiZBGTroWTn7H/eOPtu7s9MD7pAdEjqYKFLeaKmyidiLmLqQlCRj3Tl2U9oyFg4PYNc0bL5FZJ/Z6t0Ds3i/a2RanQiKxrvgu3GSnUKMx7WIX373baL4jeM7cprRGiOY/1NcS+1cAjfJ8oaxQF/1dYj' }
+      let(:ssh_key_attribute_name) { 'altSecurityIdentities' }
+      let(:entry) { Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: SSHKey:#{ssh_key}\n#{ssh_key_attribute_name}: KerberosKey:bogus") }
 
-    context 'user has at least one LDAPKey' do
       before do
-        user.keys.ldap.create key: ssh_key, title: 'to be removed'
+        stub_ldap_config(sync_ssh_keys: ssh_key_attribute_name, sync_ssh_keys?: true)
       end
 
-      it "removes a SSH key if it is no longer in LDAP" do
-        entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}:\n")
-        allow_any_instance_of(Gitlab::Auth::LDAP::Adapter).to receive(:user) { Gitlab::Auth::LDAP::Person.new(entry, 'ldapmain') }
+      it 'adds a SSH key if it is in LDAP but not in gitlab' do
+        stub_ldap_person_find_by_dn(entry, provider)
 
-        expect { access.update_ssh_keys }.to change(user.keys, :count).from(1).to(0)
+        expect { access.update_user }.to change(user.keys, :count).from(0).to(1)
       end
 
-      it "removes a SSH key if the ldap attribute was removed" do
-        entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com")
-        allow_any_instance_of(Gitlab::Auth::LDAP::Adapter).to receive(:user) { Gitlab::Auth::LDAP::Person.new(entry, 'ldapmain') }
+      it 'adds a SSH key and give it a proper name' do
+        stub_ldap_person_find_by_dn(entry, provider)
+
+        access.update_user
 
-        expect { access.update_ssh_keys }.to change(user.keys, :count).from(1).to(0)
+        expect(user.keys.last.title).to match(/LDAP/)
+        expect(user.keys.last.title).to match(/#{ssh_key_attribute_name}/)
       end
-    end
-  end
 
-  describe '#update_user_email' do
-    let(:entry) { Net::LDAP::Entry.new }
+      it 'does not add a SSH key if it is invalid' do
+        entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: I am not a valid key")
+        stub_ldap_person_find_by_dn(entry, provider)
 
-    before do
-      allow(access).to receive_messages(ldap_user: Gitlab::Auth::LDAP::Person.new(entry, user.ldap_identity.provider))
-    end
+        expect { access.update_user }.not_to change(user.keys, :count)
+      end
 
-    it "does not update email if email attribute is not set" do
-      expect { access.update_email }.not_to change(user, :email)
-    end
+      it 'does not add a SSH key when in a read-only GitLab instance' do
+        allow(Gitlab::Database).to receive(:read_only?).and_return(true)
+        stub_ldap_person_find_by_dn(entry, provider)
 
-    it "does not update the email if the user has the same email in GitLab and in LDAP" do
-      entry['mail'] = [user.email]
-      expect { access.update_email }.not_to change(user, :email)
-    end
+        expect { access.update_user }.not_to change(user.keys, :count)
+      end
 
-    it "does not update the email if the user has the same email GitLab and in LDAP, but with upper case in LDAP" do
-      entry['mail'] = [user.email.upcase]
-      expect { access.update_email }.not_to change(user, :email)
-    end
+      context 'user has at least one LDAPKey' do
+        before do
+          user.keys.ldap.create key: ssh_key, title: 'to be removed'
+        end
 
-    it "updates the email if the user email is different" do
-      entry['mail'] = ["new_email@example.com"]
-      expect { access.update_email }.to change(user, :email)
-    end
-  end
+        it 'removes a SSH key if it is no longer in LDAP' do
+          entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}:\n")
+          stub_ldap_person_find_by_dn(entry, provider)
 
-  describe '#update_memberships' do
-    let(:provider) { user.ldap_identity.provider }
-    let(:entry) { ldap_user_entry(user.ldap_identity.extern_uid) }
+          expect { access.update_user }.to change(user.keys, :count).from(1).to(0)
+        end
 
-    let(:person_with_memberof) do
-      entry['memberof'] = ['CN=Group1,CN=Users,DC=The dc,DC=com',
-                           'CN=Group2,CN=Builtin,DC=The dc,DC=com']
-      Gitlab::Auth::LDAP::Person.new(entry, provider)
+        it 'removes a SSH key if the ldap attribute was removed' do
+          entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com")
+          stub_ldap_person_find_by_dn(entry, provider)
+
+          expect { access.update_user }.to change(user.keys, :count).from(1).to(0)
+        end
+      end
     end
 
-    it 'triggers a sync for all groups found in `memberof`' do
-      group_link_1 = create(:ldap_group_link, cn: 'Group1', provider: provider)
-      group_link_2 = create(:ldap_group_link, cn: 'Group2', provider: provider)
-      group_ids = [group_link_1, group_link_2].map(&:group_id)
+    context 'kerberos identity' do
+      before do
+        stub_ldap_config(active_directory: true)
+        stub_kerberos_setting(enabled: true)
+        stub_ldap_person_find_by_dn(entry, provider)
+      end
 
-      allow(access).to receive(:ldap_user).and_return(person_with_memberof)
+      it 'adds a Kerberos identity if it is in Active Directory but not in GitLab' do
+        allow_any_instance_of(EE::Gitlab::Auth::LDAP::Person).to receive_messages(kerberos_principal: 'mylogin@FOO.COM')
 
-      expect(LdapGroupSyncWorker).to receive(:perform_async)
-        .with(a_collection_containing_exactly(*group_ids), provider)
+        expect { access.update_user }.to change(user.identities.where(provider: :kerberos), :count).from(0).to(1)
+        expect(user.identities.where(provider: 'kerberos').last.extern_uid).to eq('mylogin@FOO.COM')
+      end
 
-      access.update_memberships
-    end
+      it 'updates existing Kerberos identity in GitLab if Active Directory has a different one' do
+        allow_any_instance_of(EE::Gitlab::Auth::LDAP::Person).to receive_messages(kerberos_principal: 'otherlogin@BAR.COM')
+        user.identities.build(provider: 'kerberos', extern_uid: 'mylogin@FOO.COM').save
 
-    it "doesn't continue when there is no `memberOf` param" do
-      allow(access).to receive(:ldap_user)
-                         .and_return(Gitlab::Auth::LDAP::Person.new(entry, provider))
+        expect { access.update_user }.not_to change(user.identities.where(provider: 'kerberos'), :count)
+        expect(user.identities.where(provider: 'kerberos').last.extern_uid).to eq('otherlogin@BAR.COM')
+      end
 
-      expect(LdapGroupLink).not_to receive(:where)
-      expect(LdapGroupSyncWorker).not_to receive(:perform_async)
+      it 'does not remove Kerberos identities from GitLab if they are none in the LDAP provider' do
+        allow_any_instance_of(EE::Gitlab::Auth::LDAP::Person).to receive_messages(kerberos_principal: nil)
+        user.identities.build(provider: 'kerberos', extern_uid: 'otherlogin@BAR.COM').save
 
-      access.update_memberships
-    end
+        expect { access.update_user }.not_to change(user.identities.where(provider: 'kerberos'), :count)
+        expect(user.identities.where(provider: 'kerberos').last.extern_uid).to eq('otherlogin@BAR.COM')
+      end
 
-    it "doesn't trigger a sync when there are no links for the provider" do
-      _another_provider = create(:ldap_group_link,
-                                 cn: 'Group1',
-                                 provider: 'not-this-ldap')
+      it 'does not modify identities in GitLab if they are no kerberos principal in the LDAP provider' do
+        allow_any_instance_of(EE::Gitlab::Auth::LDAP::Person).to receive_messages(kerberos_principal: nil)
 
-      allow(access).to receive(:ldap_user).and_return(person_with_memberof)
+        expect { access.update_user }.not_to change(user.identities, :count)
+      end
 
-      expect(LdapGroupSyncWorker).not_to receive(:perform_async)
+      it 'does not add a Kerberos identity when in a read-only GitLab instance' do
+        allow(Gitlab::Database).to receive(:read_only?).and_return(true)
+        allow_any_instance_of(EE::Gitlab::Auth::LDAP::Person).to receive_messages(kerberos_principal: 'mylogin@FOO.COM')
 
-      access.update_memberships
+        expect { access.update_user }.not_to change(user.identities.where(provider: :kerberos), :count)
+      end
     end
-  end
 
-  describe '#update_identity' do
-    it 'updates the external UID if it changed in the entry' do
-      entry = ldap_user_entry('another uid')
-      provider = user.ldap_identity.provider
-      person = Gitlab::Auth::LDAP::Person.new(entry, provider)
+    context 'LDAP entity' do
+      context 'whent external UID changed in the entry' do
+        before do
+          stub_ldap_person_find_by_dn(ldap_user_entry('another uid'), provider)
+        end
 
-      allow(access).to receive(:ldap_user).and_return(person)
+        it 'updates the external UID' do
+          access.update_user
 
-      access.update_identity
+          expect(user.ldap_identity.reload.extern_uid)
+            .to eq('uid=another uid,ou=users,dc=example,dc=com')
+        end
 
-      expect(user.ldap_identity.reload.extern_uid)
-        .to eq('uid=another uid,ou=users,dc=example,dc=com')
+        it 'does not update the external UID when in a read-only GitLab instance' do
+          allow(Gitlab::Database).to receive(:read_only?).and_return(true)
+
+          access.update_user
+
+          expect(user.ldap_identity.reload.extern_uid).to eq('123456')
+        end
+      end
     end
   end
 end

lib/gitlab/auth/ldap/access.rb

@@ -21,8 +21,10 @@ module Gitlab
             # Whether user is allowed, or not, we should update
             # permissions to keep things clean
             if access.allowed?
-              access.update_user
-              Users::UpdateService.new(user, user: user, last_credential_check_at: Time.now).execute
+              unless Gitlab::Database.read_only?
+                access.update_user
+                Users::UpdateService.new(user, user: user, last_credential_check_at: Time.now).execute
+              end
 
               true
             else
@@ -62,6 +64,12 @@ module Gitlab
           false
         end
 
+        def update_user
+          # no-op in CE
+        end
+
+        private
+
         def adapter
           @adapter ||= Gitlab::Auth::LDAP::Adapter.new(provider)
         end
@@ -70,16 +78,16 @@ module Gitlab
           Gitlab::Auth::LDAP::Config.new(provider)
         end
 
-        def find_ldap_user
-          Gitlab::Auth::LDAP::Person.find_by_dn(ldap_identity.extern_uid, adapter)
-        end
-
         def ldap_user
           return unless provider
 
           @ldap_user ||= find_ldap_user
         end
 
+        def find_ldap_user
+          Gitlab::Auth::LDAP::Person.find_by_dn(ldap_identity.extern_uid, adapter)
+        end
+
         def block_user(user, reason)
           user.ldap_block
 
@@ -104,10 +112,6 @@ module Gitlab
             "unblocking Gitlab user \"#{user.name}\" (#{user.email})"
           )
         end
-
-        def update_user
-          # no-op in CE
-        end
       end
     end
   end

spec/lib/gitlab/auth/ldap/access_spec.rb

@@ -3,51 +3,61 @@ require 'spec_helper'
 describe Gitlab::Auth::LDAP::Access do
   include LdapHelpers
 
-  let(:access) { described_class.new user }
   let(:user) { create(:omniauth_user) }
 
+  subject(:access) { described_class.new(user) }
+
   describe '.allowed?' do
-    it 'updates the users `last_credential_check_at' do
+    before do
       allow(access).to receive(:update_user)
-      expect(access).to receive(:allowed?) { true }
-      expect(described_class).to receive(:open).and_yield(access)
+      allow(access).to receive(:allowed?).and_return(true)
+      allow(described_class).to receive(:open).and_yield(access)
+    end
 
+    it "updates the user's `last_credential_check_at`" do
       expect { described_class.allowed?(user) }
         .to change { user.last_credential_check_at }
     end
-  end
 
-  describe '#find_ldap_user' do
-    it 'finds a user by dn first' do
-      expect(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(:ldap_user)
+    it "does not update user's `last_credential_check_at` when in a read-only GitLab instance" do
+      allow(Gitlab::Database).to receive(:read_only?).and_return(true)
 
-      access.find_ldap_user
+      expect { described_class.allowed?(user) }
+        .not_to change { user.last_credential_check_at }
     end
   end
 
   describe '#allowed?' do
-    subject { access.allowed? }
-
     context 'when the user cannot be found' do
       before do
-        allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(nil)
-        allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(nil)
+        stub_ldap_person_find_by_dn(nil)
+        stub_ldap_person_find_by_email(user.email, nil)
       end
 
-      it { is_expected.to be_falsey }
+      it 'returns false' do
+        expect(access.allowed?).to be_falsey
+      end
 
       it 'blocks user in GitLab' do
-        expect(access).to receive(:block_user).with(user, 'does not exist anymore')
+        access.allowed?
+
+        expect(user).to be_blocked
+        expect(user).to be_ldap_blocked
+      end
+
+      it 'logs the reason' do
+        expect(Gitlab::AppLogger).to receive(:info).with(
+          "LDAP account \"123456\" does not exist anymore, " \
+          "blocking Gitlab user \"#{user.name}\" (#{user.email})"
+        )
 
         access.allowed?
       end
     end
 
     context 'when the user is found' do
-      let(:ldap_user) { Gitlab::Auth::LDAP::Person.new(Net::LDAP::Entry.new, 'ldapmain') }
-
       before do
-        allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user)
+        stub_ldap_person_find_by_dn(Net::LDAP::Entry.new)
       end
 
       context 'and the user is disabled via active directory' do
@@ -55,10 +65,22 @@ describe Gitlab::Auth::LDAP::Access do
           allow(Gitlab::Auth::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(true)
         end
 
-        it { is_expected.to be_falsey }
+        it 'returns false' do
+          expect(access.allowed?).to be_falsey
+        end
 
         it 'blocks user in GitLab' do
-          expect(access).to receive(:block_user).with(user, 'is disabled in Active Directory')
+          access.allowed?
+
+          expect(user).to be_blocked
+          expect(user).to be_ldap_blocked
+        end
+
+        it 'logs the reason' do
+          expect(Gitlab::AppLogger).to receive(:info).with(
+            "LDAP account \"123456\" is disabled in Active Directory, " \
+            "blocking Gitlab user \"#{user.name}\" (#{user.email})"
+          )
 
           access.allowed?
         end
@@ -92,7 +114,17 @@ describe Gitlab::Auth::LDAP::Access do
           end
 
           it 'unblocks user in GitLab' do
-            expect(access).to receive(:unblock_user).with(user, 'is not disabled anymore')
+            access.allowed?
+
+            expect(user).not_to be_blocked
+            expect(user).not_to be_ldap_blocked
+          end
+
+          it 'logs the reason' do
+            expect(Gitlab::AppLogger).to receive(:info).with(
+              "LDAP account \"123456\" is not disabled anymore, " \
+              "unblocking Gitlab user \"#{user.name}\" (#{user.email})"
+            )
 
             access.allowed?
           end
@@ -105,18 +137,32 @@ describe Gitlab::Auth::LDAP::Access do
           allow_any_instance_of(Gitlab::Auth::LDAP::Config).to receive(:active_directory).and_return(false)
         end
 
-        it { is_expected.to be_truthy }
+        it 'returns true' do
+          expect(access.allowed?).to be_truthy
+        end
 
         context 'when user cannot be found' do
           before do
-            allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(nil)
-            allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(nil)
+            stub_ldap_person_find_by_dn(nil)
+            stub_ldap_person_find_by_email(user.email, nil)
           end
 
-          it { is_expected.to be_falsey }
+          it 'returns false' do
+            expect(access.allowed?).to be_falsey
+          end
 
           it 'blocks user in GitLab' do
-            expect(access).to receive(:block_user).with(user, 'does not exist anymore')
+            access.allowed?
+
+            expect(user).to be_blocked
+            expect(user).to be_ldap_blocked
+          end
+
+          it 'logs the reason' do
+            expect(Gitlab::AppLogger).to receive(:info).with(
+              "LDAP account \"123456\" does not exist anymore, " \
+              "blocking Gitlab user \"#{user.name}\" (#{user.email})"
+            )
 
             access.allowed?
           end
@@ -128,7 +174,17 @@ describe Gitlab::Auth::LDAP::Access do
           end
 
           it 'unblocks the user if it exists' do
-            expect(access).to receive(:unblock_user).with(user, 'is available again')
+            access.allowed?
+
+            expect(user).not_to be_blocked
+            expect(user).not_to be_ldap_blocked
+          end
+
+          it 'logs the reason' do
+            expect(Gitlab::AppLogger).to receive(:info).with(
+              "LDAP account \"123456\" is available again, " \
+              "unblocking Gitlab user \"#{user.name}\" (#{user.email})"
+            )
 
             access.allowed?
           end
@@ -152,46 +208,4 @@ describe Gitlab::Auth::LDAP::Access do
       end
     end
   end
-
-  describe '#block_user' do
-    before do
-      user.activate
-      allow(Gitlab::AppLogger).to receive(:info)
-
-      access.block_user user, 'reason'
-    end
-
-    it 'blocks the user' do
-      expect(user).to be_blocked
-      expect(user).to be_ldap_blocked
-    end
-
-    it 'logs the reason' do
-      expect(Gitlab::AppLogger).to have_received(:info).with(
-        "LDAP account \"123456\" reason, " \
-        "blocking Gitlab user \"#{user.name}\" (#{user.email})"
-      )
-    end
-  end
-
-  describe '#unblock_user' do
-    before do
-      user.ldap_block
-      allow(Gitlab::AppLogger).to receive(:info)
-
-      access.unblock_user user, 'reason'
-    end
-
-    it 'activates the user' do
-      expect(user).not_to be_blocked
-      expect(user).not_to be_ldap_blocked
-    end
-
-    it 'logs the reason' do
-      Gitlab::AppLogger.info(
-        "LDAP account \"123456\" reason, " \
-        "unblocking Gitlab user \"#{user.name}\" (#{user.email})"
-      )
-    end
-  end
 end

spec/support/helpers/ldap_helpers.rb

@@ -45,6 +45,23 @@ module LdapHelpers
       .to receive(:find_by_uid).with(uid, any_args).and_return(return_value)
   end
 
+  def stub_ldap_person_find_by_dn(entry, provider = 'ldapmain')
+    person = ::Gitlab::Auth::LDAP::Person.new(entry, provider) if entry.present?
+
+    allow(::Gitlab::Auth::LDAP::Person)
+      .to receive(:find_by_dn)
+      .and_return(person)
+  end
+
+  def stub_ldap_person_find_by_email(email, entry, provider = 'ldapmain')
+    person = ::Gitlab::Auth::LDAP::Person.new(entry, provider) if entry.present?
+
+    allow(::Gitlab::Auth::LDAP::Person)
+      .to receive(:find_by_email)
+      .with(email, anything)
+      .and_return(person)
+  end
+
   # Create a simple LDAP user entry.
   def ldap_user_entry(uid)
     entry = Net::LDAP::Entry.new

spec/support/helpers/stub_configuration.rb

@@ -82,6 +82,10 @@ module StubConfiguration
     allow(Gitlab.config.repositories).to receive(:storages).and_return(Settingslogic.new(messages))
   end
 
+  def stub_kerberos_setting(messages)
+    allow(Gitlab.config.kerberos).to receive_messages(to_settings(messages))
+  end
+
   private
 
   # Modifies stubbed messages to also stub possible predicate versions