Merge branch 'fix/prevent-to-remove-last-blocked-group-owner' into 'master'

Prevent removal of last blocked group owner [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!54587

app/models/group.rb

@@ -340,6 +340,13 @@ class Group < Namespace
     has_owner?(user) && members_with_parents.owners.size == 1
   end
 
+  def last_blocked_owner?(user)
+    return false if members_with_parents.owners.any?
+
+    blocked_owners = members.blocked.where(access_level: Gitlab::Access::OWNER)
+    blocked_owners.size == 1 && blocked_owners.exists?(user_id: user)
+  end
+
   def ldap_synced?
     false
   end

app/models/member.rb

@@ -75,7 +75,20 @@ class Member < ApplicationRecord
 
     left_join_users
       .where(user_ok)
-      .where(requested_at: nil)
+      .non_request
+      .non_minimal_access
+      .reorder(nil)
+  end
+
+  scope :blocked, -> do
+    is_external_invite = arel_table[:user_id].eq(nil).and(arel_table[:invite_token].not_eq(nil))
+    user_is_blocked = User.arel_table[:state].eq(:blocked)
+
+    user_ok = Arel::Nodes::Grouping.new(is_external_invite).or(user_is_blocked)
+
+    left_join_users
+      .where(user_ok)
+      .non_request
       .non_minimal_access
       .reorder(nil)
   end

app/policies/group_member_policy.rb

@@ -4,7 +4,7 @@ class GroupMemberPolicy < BasePolicy
   delegate :group
 
   with_scope :subject
-  condition(:last_owner) { @subject.group.last_owner?(@subject.user) }
+  condition(:last_owner) { @subject.group.last_owner?(@subject.user) || @subject.group.last_blocked_owner?(@subject.user) }
 
   desc "Membership is users' own"
   with_score 0

changelogs/unreleased/fix-prevent-to-remove-last-blocked-group-owner.yml

+---
+title: Prevent removal of the last group owner if the last group owner is a blocked user
+merge_request: 54587
+author: Jonas Wälter @wwwjon
+type: fixed

spec/models/group_spec.rb

@@ -565,6 +565,42 @@ RSpec.describe Group do
     end
   end
 
+  describe '#last_blocked_owner?' do
+    let(:blocked_user) { create(:user, :blocked) }
+
+    before do
+      group.add_user(blocked_user, GroupMember::OWNER)
+    end
+
+    it { expect(group.last_blocked_owner?(blocked_user)).to be_truthy }
+
+    context 'with another active owner' do
+      before do
+        group.add_user(create(:user), GroupMember::OWNER)
+      end
+
+      it { expect(group.last_blocked_owner?(blocked_user)).to be_falsy }
+    end
+
+    context 'with 2 blocked owners' do
+      before do
+        group.add_user(create(:user, :blocked), GroupMember::OWNER)
+      end
+
+      it { expect(group.last_blocked_owner?(blocked_user)).to be_falsy }
+    end
+
+    context 'with owners from a parent' do
+      before do
+        parent_group = create(:group)
+        create(:group_member, :owner, group: parent_group)
+        group.update(parent: parent_group)
+      end
+
+      it { expect(group.last_blocked_owner?(blocked_user)).to be_falsy }
+    end
+  end
+
   describe '#lfs_enabled?' do
     context 'LFS enabled globally' do
       before do

spec/models/member_spec.rb

@@ -130,14 +130,18 @@ RSpec.describe Member do
       @maintainer_user = create(:user).tap { |u| project.add_maintainer(u) }
       @maintainer = project.members.find_by(user_id: @maintainer_user.id)
 
-      @blocked_user = create(:user).tap do |u|
+      @blocked_maintainer_user = create(:user).tap do |u|
         project.add_maintainer(u)
+
+        u.block!
+      end
+      @blocked_developer_user = create(:user).tap do |u|
         project.add_developer(u)
 
         u.block!
       end
-      @blocked_maintainer = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::MAINTAINER)
-      @blocked_developer = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::DEVELOPER)
+      @blocked_maintainer = project.members.find_by(user_id: @blocked_maintainer_user.id, access_level: Gitlab::Access::MAINTAINER)
+      @blocked_developer = project.members.find_by(user_id: @blocked_developer_user.id, access_level: Gitlab::Access::DEVELOPER)
 
       @invited_member = create(:project_member, :developer,
                               project: project,
@@ -161,7 +165,7 @@ RSpec.describe Member do
 
     describe '.access_for_user_ids' do
       it 'returns the right access levels' do
-        users = [@owner_user.id, @maintainer_user.id, @blocked_user.id]
+        users = [@owner_user.id, @maintainer_user.id, @blocked_maintainer_user.id]
         expected = {
           @owner_user.id => Gitlab::Access::OWNER,
           @maintainer_user.id => Gitlab::Access::MAINTAINER
@@ -382,6 +386,20 @@ RSpec.describe Member do
       it { is_expected.not_to include @member_with_minimal_access }
     end
 
+    describe '.blocked' do
+      subject { described_class.blocked.to_a }
+
+      it { is_expected.not_to include @owner }
+      it { is_expected.not_to include @maintainer }
+      it { is_expected.not_to include @invited_member }
+      it { is_expected.not_to include @accepted_invite_member }
+      it { is_expected.not_to include @requested_member }
+      it { is_expected.not_to include @accepted_request_member }
+      it { is_expected.to include @blocked_maintainer }
+      it { is_expected.to include @blocked_developer }
+      it { is_expected.not_to include @member_with_minimal_access }
+    end
+
     describe '.active_without_invites_and_requests' do
       subject { described_class.active_without_invites_and_requests.to_a }
 

spec/policies/group_member_policy_spec.rb

@@ -90,6 +90,14 @@ RSpec.describe GroupMemberPolicy do
     specify { expect_allowed(:read_group) }
   end
 
+  context 'with one blocked owner' do
+    let(:owner) { create(:user, :blocked) }
+    let(:current_user) { owner }
+
+    specify { expect_disallowed(*member_related_permissions) }
+    specify { expect_disallowed(:read_group) }
+  end
+
   context 'with more than one owner' do
     let(:current_user) { owner }