summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/models/group.rb7
-rw-r--r--changelogs/unreleased/38564-cant-leave-subgroup.yml5
-rw-r--r--spec/models/group_spec.rb26
-rw-r--r--spec/policies/group_member_policy_spec.rb105
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