Remove user from groups when they are removed from LDAP

lib/gitlab/ldap/access.rb

@@ -16,9 +16,11 @@ module Gitlab
 
       def self.allowed?(user)
         self.open(user) do |access|
-          if access.allowed?
+          # Whether user is allowed, or not, we should update
+          # permissions to keep things clean
           access.update_permissions
-            access.update_email
+          if access.allowed?
+            access.update_user
             user.last_credential_check_at = Time.now
             user.save
             true
@@ -67,22 +69,15 @@ module Gitlab
         @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter)
       end
 
-      def update_permissions
-        if sync_ssh_keys?
-          update_ssh_keys
-        end
-
+      def update_user
+        update_email
+        update_ssh_keys if sync_ssh_keys?
         update_kerberos_identity if import_kerberos_identities?
-
-        # Skip updating group permissions
-        # if instance does not use group_base setting
-        return true unless group_base.present?
-
-        update_ldap_group_links
-
-        if admin_group.present?
-          update_admin_status
       end
+
+      def update_permissions
+        update_ldap_group_links if group_base.present?
+        update_admin_status if admin_group.present?
       end
 
       # Update user ssh keys if they changed in LDAP
@@ -191,6 +186,7 @@ module Gitlab
 
       # returns a collection of cn strings to which the user has access
       def cns_with_access
+        return [] unless ldap_user.present?
         @ldap_groups_with_access ||= ldap_groups.select do |ldap_group|
           ldap_group.has_member?(ldap_user)
         end.map(&:cn)

spec/lib/gitlab/ldap/access_spec.rb

@@ -4,7 +4,7 @@ describe Gitlab::LDAP::Access, lib: true do
   let(:access) { Gitlab::LDAP::Access.new user }
   let(:user) { create(:omniauth_user) }
 
-  describe :allowed? do
+  describe '#allowed?' do
     subject { access.allowed? }
 
     context 'when the user cannot be found' do
@@ -40,7 +40,7 @@ describe Gitlab::LDAP::Access, lib: true do
         end
       end
 
-      context 'and has no disabled flag in active diretory' do
+      context 'and has no disabled flag in active directory' do
         before do
           allow(Gitlab::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(false)
         end
@@ -71,7 +71,7 @@ describe Gitlab::LDAP::Access, lib: true do
         end
       end
 
-      context 'withoud ActiveDirectory enabled' do
+      context 'without ActiveDirectory enabled' do
         before do
           allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true)
           allow_any_instance_of(Gitlab::LDAP::Config).to receive(:active_directory).and_return(false)
@@ -82,41 +82,67 @@ describe Gitlab::LDAP::Access, lib: true do
     end
   end
 
-  describe :update_permissions do
-    subject { access.update_permissions }
+  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::LDAP::Person.new(entry, user.ldap_identity.provider)
+        )
+      )
+    end
+
+    it 'updates email address' do
+      expect(access).to receive(:update_email).once
 
-    it "syncs ssh keys if enabled by configuration" do
-      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: 'sshpublickey', import_kerberos_identities?: false)
+      subject
+    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
 
       subject
     end
 
-    it "does update group permissions with a group base configured" do
-      allow(access).to receive_messages(group_base: 'my-group-base', sync_ssh_keys?: false, import_kerberos_identities?: false)
+    it 'update_kerberos_identity' do
+      allow(access).to receive_messages(import_kerberos_identities?: true)
+      expect(access).to receive(:update_kerberos_identity).once
+
+      subject
+    end
+  end
+
+  describe '#update_permissions' do
+    subject { access.update_permissions }
+
+    it 'does update group permissions with a group base configured' do
+      allow(access).to receive_messages(group_base: 'my-group-base')
       expect(access).to receive(:update_ldap_group_links)
 
       subject
     end
 
-    it "does not update group permissions without a group base configured" do
-      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: false, import_kerberos_identities?: false)
+    it 'does not update group permissions without a group base configured' do
+      allow(access).to receive_messages(group_base: '')
       expect(access).not_to receive(:update_ldap_group_links)
 
       subject
     end
 
-    it "does update admin group permissions if admin group is configured" do
-      allow(access).to receive_messages(admin_group: 'my-admin-group', update_ldap_group_links: nil, sync_ssh_keys?: false, import_kerberos_identities?: false)
+    it 'does update admin group permissions if admin group is configured' do
+      allow(access).to receive_messages(admin_group: 'my-admin-group')
       expect(access).to receive(:update_admin_status)
 
       subject
     end
 
-    it "does update Kerberos identities if Kerberos is enabled and the LDAP server is Active Directory" do
-      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: false, import_kerberos_identities?: true)
-
-      expect(access).to receive(:update_kerberos_identity)
+    it 'does not update admin status when admin group is not configured' do
+      allow(access).to receive_messages(admin_group: '')
+      expect(access).not_to receive(:update_admin_status)
 
       subject
     end
@@ -451,17 +477,33 @@ objectclass: posixGroup
       ]
     end
 
+    before do
+      allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
+      allow(access).to receive_messages(ldap_groups: ldap_groups)
+    end
+
+    context 'when the LDAP user exists' do
       let(:ldap_user) { Gitlab::LDAP::Person.new(Net::LDAP::Entry.new, user.ldap_identity.provider) }
 
       before do
         allow(access).to receive_messages(ldap_user: ldap_user)
         allow(ldap_user).to receive(:uid) { 'user1' }
-      allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
       end
 
-    it "only returns ldap cns to which the user has access" do
-      allow(access).to receive_messages(ldap_groups: ldap_groups)
-      expect(access.cns_with_access).to eql ['group1']
+      it 'only returns ldap cns to which the user has access' do
+        expect(access.cns_with_access).to eq(['group1'])
+      end
+    end
+
+    context 'when the LADP user does not exist' do
+      let(:ldap_user) { nil }
+      before do
+        allow(access).to receive_messages(ldap_user: ldap_user)
+      end
+
+      it 'returns an empty array' do
+        expect(access.cns_with_access).to eq([])
+      end
     end
   end
 end