diff options
| -rw-r--r-- | app/models/group.rb | 7 | ||||
| -rw-r--r-- | changelogs/unreleased/38564-cant-leave-subgroup.yml | 5 | ||||
| -rw-r--r-- | spec/models/group_spec.rb | 26 | ||||
| -rw-r--r-- | spec/policies/group_member_policy_spec.rb | 105 |
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 |