Merge branch 'ldap_dont_remove_last_owner' into 'master'

Prevent LDAP from downgrading a group\'s last owner. Fixes #325

This should fix #325. I based it on the logic in !215 but optimized it a bit. I also added a new test and fixed an existing one that was prone to false-negatives. 

See merge request !216

CHANGELOG-EE

@@ -8,6 +8,9 @@ v 8.6.0 (unreleased)
   - [Elastic] Update index on push to wiki
   - [Elastic] Use subprocesses for ElasticSearch index jobs
 
+v 8.5.3
+  - Prevent LDAP from downgrading a group's last owner
+
 v 8.5.2
   - Update LDAP groups asynchronously
   - Fix an issue when weight text was displayed in Issuable collapsed sidebar

lib/gitlab/ldap/access.rb

@@ -175,7 +175,14 @@ module Gitlab
           active_group_links = group.ldap_group_links.where(cn: cns_with_access)
 
           if active_group_links.any?
-            group.add_users([user.id], active_group_links.maximum(:group_access), skip_notification: true)
+            max_access = active_group_links.maximum(:group_access)
+
+            # Ensure we don't leave a group without an owner
+            if max_access < Gitlab::Access::OWNER && group.last_owner?(user)
+              logger.warn "#{self.class.name}: LDAP group sync cannot demote #{user.name} (#{user.id}) from group #{group.name} (#{group.id}) as this is the group's last owner"
+            else
+              group.add_users([user.id], max_access, skip_notification: true)
+            end
           elsif group.last_owner?(user)
             logger.warn "#{self.class.name}: LDAP group sync cannot remove #{user.name} (#{user.id}) from group #{group.name} (#{group.id}) as this is the group's last owner"
           else

spec/lib/gitlab/ldap/access_spec.rb

@@ -384,33 +384,50 @@ objectclass: posixGroup
       end
     end
 
-    context "existing access as master for group-1, not allowed" do
+    context 'existing access as master for group-1, not allowed via LDAP' do
       before do
         gitlab_group_1.group_members.masters.create(user_id: user.id)
         gitlab_group_1.ldap_group_links.create(cn: 'ldap-group1', group_access: Gitlab::Access::MASTER, provider: 'ldapmain')
         allow(access).to receive_messages(cns_with_access: ['ldap-group2'])
       end
 
-      it "removes user from gitlab_group_1" do
+      it 'removes user from gitlab_group_1' do
         expect { access.update_ldap_group_links }.to \
           change{ gitlab_group_1.members.where(user_id: user).any? }.from(true).to(false)
       end
     end
 
-    context "existing access as owner for group-1 with no other owner, not allowed" do
+    context 'existing access as owner for group-1 with no other owner, not allowed via LDAP' do
       before do
         gitlab_group_1.group_members.owners.create(user_id: user.id)
         gitlab_group_1.ldap_group_links.create(cn: 'ldap-group1', group_access: Gitlab::Access::OWNER, provider: 'ldapmain')
         allow(access).to receive_messages(cns_with_access: ['ldap-group2'])
       end
 
-      it "does not remove the user from gitlab_group_1 since it's the last owner" do
-        expect { access.update_ldap_group_links }.not_to \
-          change{ gitlab_group_1.has_owner?(user) }
+      # Note: Don't use `has_owner?` or `has_master?` in this expectation.
+      # It leads to false negatives.
+      it 'does not remove the user from gitlab_group_1 since its the last owner' do
+        expect { access.update_ldap_group_links }.not_to  \
+          change { gitlab_group_1.members.owners.where(user_id: user).any? }
+      end
+    end
+
+    context 'existing access as owner for group-1 with no other owner, allowed via ldap-group1 as DEVELOPER' do
+      before do
+        gitlab_group_1.group_members.owners.create(user_id: user.id)
+        gitlab_group_1.ldap_group_links.create(cn: 'ldap-group1', group_access: Gitlab::Access::DEVELOPER, provider: 'ldapmain')
+        allow(access).to receive_messages(cns_with_access: ['ldap-group1'])
+      end
+
+      # Note: Don't use `has_owner?` or `has_master?` in this expectation.
+      # It leads to false negatives.
+      it 'does not remove the user from gitlab_group_1 since its the last owner' do
+        expect { access.update_ldap_group_links }.not_to  \
+          change { gitlab_group_1.members.owners.where(user_id: user).any? }
       end
     end
 
-    context "existing access as owner for group-1 while other owners present, not allowed" do
+    context 'existing access as owner for group-1 while other owners present, not allowed via LDAP' do
       before do
         owner2 = create(:user) # a 2nd owner
         gitlab_group_1.group_members.owners.create([{ user_id: user.id }, { user_id: owner2.id }])
@@ -418,7 +435,7 @@ objectclass: posixGroup
         allow(access).to receive_messages(cns_with_access: ['ldap-group2'])
       end
 
-      it "removes user from gitlab_group_1" do
+      it 'removes user from gitlab_group_1' do
         expect { access.update_ldap_group_links }.to \
           change{ gitlab_group_1.members.where(user_id: user).any? }.from(true).to(false)
       end