Merge branch '38564-cant-leave-subgroup' into 'master'

Change the way it is checked if the user is last group owner

Closes #38564

See merge request gitlab-org/gitlab-ce!26718

4 files changed, 139 insertions, 4 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index c77586c4cdc..ac66815705c 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -228,22 +228,21 @@ class Group < Namespace
   def has_owner?(user)
     return false unless user
 
-    members_with_parents.owners.where(user_id: user).any?
+    members_with_parents.owners.exists?(user_id: user)
   end
 
   def has_maintainer?(user)
     return false unless user
 
-    members_with_parents.maintainers.where(user_id: user).any?
+    members_with_parents.maintainers.exists?(user_id: user)
   end
 
   # @deprecated
   alias_method :has_master?, :has_maintainer?
 
   # Check if user is a last owner of the group.
-  # Parent owners are ignored for nested groups.
   def last_owner?(user)
-    owners.include?(user) && owners.size == 1
+    has_owner?(user) && members_with_parents.owners.size == 1
   end
 
   def ldap_synced?
diff --git a/changelogs/unreleased/38564-cant-leave-subgroup.yml b/changelogs/unreleased/38564-cant-leave-subgroup.yml
new file mode 100644
index 00000000000..a6397062343
--- /dev/null
+++ b/changelogs/unreleased/38564-cant-leave-subgroup.yml
@@ -0,0 +1,5 @@
+---
+title: Allow removing last owner from subgroup if parent group has owners
+merge_request: 26718
+author:
+type: changed
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 2c6abddca17..b2ffd5812ab 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -364,6 +364,32 @@ describe Group do
     it { expect(group.has_maintainer?(nil)).to be_falsey }
   end
 
+  describe '#last_owner?' do
+    before do
+      @members = setup_group_members(group)
+    end
+
+    it { expect(group.last_owner?(@members[:owner])).to be_truthy }
+
+    context 'with two owners' do
+      before do
+        create(:group_member, :owner, group: group)
+      end
+
+      it { expect(group.last_owner?(@members[:owner])).to be_falsy }
+    end
+
+    context 'with owners from a parent', :postgresql do
+      before do
+        parent_group = create(:group)
+        create(:group_member, :owner, group: parent_group)
+        group.update(parent: parent_group)
+      end
+
+      it { expect(group.last_owner?(@members[:owner])).to be_falsy }
+    end
+  end
+
   describe '#lfs_enabled?' do
     context 'LFS enabled globally' do
       before do
diff --git a/spec/policies/group_member_policy_spec.rb b/spec/policies/group_member_policy_spec.rb
new file mode 100644
index 00000000000..7bd7184cffe
--- /dev/null
+++ b/spec/policies/group_member_policy_spec.rb
@@ -0,0 +1,105 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe GroupMemberPolicy do
+  let(:guest) { create(:user) }
+  let(:owner) { create(:user) }
+  let(:group) { create(:group, :private) }
+
+  before do
+    group.add_guest(guest)
+    group.add_owner(owner)
+  end
+
+  let(:member_related_permissions) do
+    [:update_group_member, :destroy_group_member]
+  end
+
+  let(:membership) { current_user.members.first }
+
+  subject { described_class.new(current_user, membership) }
+
+  def expect_allowed(*permissions)
+    permissions.each { |p| is_expected.to be_allowed(p) }
+  end
+
+  def expect_disallowed(*permissions)
+    permissions.each { |p| is_expected.not_to be_allowed(p) }
+  end
+
+  context 'with guest user' do
+    let(:current_user) { guest }
+
+    it do
+      expect_disallowed(:member_related_permissions)
+    end
+  end
+
+  context 'with one owner' do
+    let(:current_user) { owner }
+
+    it do
+      expect_disallowed(:destroy_group_member)
+      expect_disallowed(:update_group_member)
+    end
+  end
+
+  context 'with more than one owner' do
+    let(:current_user) { owner }
+
+    before do
+      group.add_owner(create(:user))
+    end
+
+    it do
+      expect_allowed(:destroy_group_member)
+      expect_allowed(:update_group_member)
+    end
+  end
+
+  context 'with the group parent', :postgresql do
+    let(:current_user) { create :user }
+    let(:subgroup) { create(:group, :private, parent: group)}
+
+    before do
+      group.add_owner(owner)
+      subgroup.add_owner(current_user)
+    end
+
+    it do
+      expect_allowed(:destroy_group_member)
+      expect_allowed(:update_group_member)
+    end
+  end
+
+  context 'without group parent' do
+    let(:current_user) { create :user }
+    let(:subgroup) { create(:group, :private)}
+
+    before do
+      subgroup.add_owner(current_user)
+    end
+
+    it do
+      expect_disallowed(:destroy_group_member)
+      expect_disallowed(:update_group_member)
+    end
+  end
+
+  context 'without group parent with two owners' do
+    let(:current_user) { create :user }
+    let(:other_user) { create :user }
+    let(:subgroup) { create(:group, :private)}
+
+    before do
+      subgroup.add_owner(current_user)
+      subgroup.add_owner(other_user)
+    end
+
+    it do
+      expect_allowed(:destroy_group_member)
+      expect_allowed(:update_group_member)
+    end
+  end
+end