Fix effective access level of group members

app/finders/group_members_finder.rb

@@ -21,28 +21,13 @@ class GroupMembersFinder < UnionFinder
   end
 
   def execute(include_relations: DEFAULT_RELATIONS)
-    group_members = group_members_list
-    relations = []
+    return filter_members(group_members_list) if include_relations == [:direct]
 
-    return filter_members(group_members) if include_relations == [:direct]
+    groups = groups_by_relations(include_relations)
+    return GroupMember.none unless groups
 
-    relations << group_members if include_relations.include?(:direct)
+    members = all_group_members(groups).distinct_on_user_with_max_access_level
 
-    if include_relations.include?(:inherited) && group.parent
-      parents_members = relation_group_members(group.ancestors)
-
-      relations << parents_members
-    end
-
-    if include_relations.include?(:descendants)
-      descendant_members = relation_group_members(group.descendants)
-
-      relations << descendant_members
-    end
-
-    return GroupMember.none if relations.empty?
-
-    members = find_union(relations, GroupMember)
     filter_members(members)
   end
 
@@ -50,6 +35,25 @@ class GroupMembersFinder < UnionFinder
 
   attr_reader :user, :group
 
+  def groups_by_relations(include_relations)
+    case include_relations.sort
+    when [:inherited]
+      group.ancestors
+    when [:descendants]
+      group.descendants
+    when [:direct, :inherited]
+      group.self_and_ancestors
+    when [:descendants, :direct]
+      group.self_and_descendants
+    when [:descendants, :inherited]
+      find_union([group.ancestors, group.descendants], Group)
+    when [:descendants, :direct, :inherited]
+      group.self_and_hierarchy
+    else
+      nil
+    end
+  end
+
   def filter_members(members)
     members = members.search(params[:search]) if params[:search].present?
     members = members.sort_by_attribute(params[:sort]) if params[:sort].present?
@@ -69,17 +73,13 @@ class GroupMembersFinder < UnionFinder
     group.members
   end
 
-  def relation_group_members(relation)
-    all_group_members(relation).non_minimal_access
+  def all_group_members(groups)
+    members_of_groups(groups).non_minimal_access
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
-  def all_group_members(relation)
-    GroupMember.non_request
-      .where(source_id: relation.select(:id))
-      .where.not(user_id: group.users.select(:id))
+  def members_of_groups(groups)
+    GroupMember.non_request.of_groups(groups)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 end
 
 GroupMembersFinder.prepend_if_ee('EE::GroupMembersFinder')

app/models/member.rb

@@ -137,6 +137,12 @@ class Member < ApplicationRecord
   scope :with_source_id, ->(source_id) { where(source_id: source_id) }
   scope :including_source, -> { includes(:source) }
 
+  scope :distinct_on_user_with_max_access_level, -> do
+    distinct_members = select('DISTINCT ON (user_id, invite_email) *')
+                       .order('user_id, invite_email, access_level DESC, expires_at DESC, created_at ASC')
+    Member.from(distinct_members, :members)
+  end
+
   scope :order_name_asc, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.name', 'ASC')) }
   scope :order_name_desc, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.name', 'DESC')) }
   scope :order_recent_sign_in, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.last_sign_in_at', 'DESC')) }

changelogs/unreleased/fix-group_members_max_access_level.yml

+---
+title: Fix derivation of effective permissions (access level) of group members
+merge_request: 56677
+author: Jonas Wälter @wwwjon
+type: fixed

doc/api/members.md

@@ -90,8 +90,8 @@ Example response:
 
 Gets a list of group or project members viewable by the authenticated user, including inherited members and permissions through ancestor groups.
 
-WARNING:
-Due to [an issue](https://gitlab.com/gitlab-org/gitlab/-/issues/249523), the users effective `access_level` may actually be higher than returned value when listing group members.
+If a user is a member of this group or project and also of one or more ancestor groups, only its membership with the highest `access_level` is returned.
+This represents the effective permission of the user.
 
 This function takes pagination parameters `page` and `per_page` to restrict the list of users.
 

ee/app/finders/ee/group_members_finder.rb

@@ -21,9 +21,9 @@ module EE::GroupMembersFinder
     super
   end
 
-  override :relation_group_members
-  def relation_group_members(relation)
-    return all_group_members(relation) if group.minimal_access_role_allowed?
+  override :all_group_members
+  def all_group_members(groups)
+    return members_of_groups(groups) if group.minimal_access_role_allowed?
 
     super
   end

spec/finders/group_members_finder_spec.rb

@@ -3,174 +3,180 @@
 require 'spec_helper'
 
 RSpec.describe GroupMembersFinder, '#execute' do
-  let(:group) { create(:group) }
-  let(:nested_group) { create(:group, parent: group) }
-  let(:deeper_nested_group) { create(:group, parent: nested_group) }
-  let(:user1) { create(:user) }
-  let(:user2) { create(:user) }
-  let(:user3) { create(:user) }
-  let(:user4) { create(:user) }
-  let(:user5) { create(:user, :two_factor_via_otp) }
-
-  it 'returns members for top-level group' do
-    member1 = group.add_maintainer(user1)
-    member2 = group.add_maintainer(user2)
-    member3 = group.add_maintainer(user3)
-    create(:group_member, :minimal_access, user: create(:user), source: group)
-
-    result = described_class.new(group).execute
-
-    expect(result.to_a).to match_array([member3, member2, member1])
+  let(:group)         { create(:group) }
+  let(:sub_group)     { create(:group, parent: group) }
+  let(:sub_sub_group) { create(:group, parent: sub_group) }
+  let(:user1)         { create(:user) }
+  let(:user2)         { create(:user) }
+  let(:user3)         { create(:user) }
+  let(:user4)         { create(:user) }
+  let(:user5)         { create(:user, :two_factor_via_otp) }
+
+  let(:groups) do
+    {
+      group:         group,
+      sub_group:     sub_group,
+      sub_sub_group: sub_sub_group
+    }
   end
 
-  it 'returns members & inherited members for nested group by default' do
-    group.add_developer(user2)
-    nested_group.request_access(user4)
-    member1 = group.add_maintainer(user1)
-    member3 = nested_group.add_maintainer(user2)
-    member4 = nested_group.add_maintainer(user3)
-
-    result = described_class.new(nested_group).execute
-
-    expect(result.to_a).to match_array([member1, member3, member4])
+  context 'relations' do
+    let!(:members) do
+      {
+        user1_sub_sub_group: create(:group_member, :maintainer, group: sub_sub_group, user: user1),
+        user1_sub_group:     create(:group_member, :developer,  group: sub_group,     user: user1),
+        user1_group:         create(:group_member, :reporter,   group: group,         user: user1),
+        user2_sub_sub_group: create(:group_member, :reporter,   group: sub_sub_group, user: user2),
+        user2_sub_group:     create(:group_member, :developer,  group: sub_group,     user: user2),
+        user2_group:         create(:group_member, :maintainer, group: group,         user: user2),
+        user3_sub_sub_group: create(:group_member, :developer,  group: sub_sub_group, user: user3, expires_at: 1.day.from_now),
+        user3_sub_group:     create(:group_member, :developer,  group: sub_group,     user: user3, expires_at: 2.days.from_now),
+        user3_group:         create(:group_member, :reporter,   group: group,         user: user3),
+        user4_sub_sub_group: create(:group_member, :reporter,   group: sub_sub_group, user: user4),
+        user4_sub_group:     create(:group_member, :developer,  group: sub_group,     user: user4, expires_at: 1.day.from_now),
+        user4_group:         create(:group_member, :developer,  group: group,         user: user4, expires_at: 2.days.from_now)
+      }
+    end
+
+    using RSpec::Parameterized::TableSyntax
+
+    where(:subject_relations, :subject_group, :expected_members) do
+      nil                                 | :group         | [:user1_group,         :user2_group,         :user3_group,         :user4_group]
+      [:direct]                           | :group         | [:user1_group,         :user2_group,         :user3_group,         :user4_group]
+      [:inherited]                        | :group         | []
+      [:descendants]                      | :group         | [:user1_sub_sub_group, :user2_sub_group,     :user3_sub_group,     :user4_sub_group]
+      [:direct, :inherited]               | :group         | [:user1_group,         :user2_group,         :user3_group,         :user4_group]
+      [:direct, :descendants]             | :group         | [:user1_sub_sub_group, :user2_group,         :user3_sub_group,     :user4_group]
+      [:descendants, :inherited]          | :group         | [:user1_sub_sub_group, :user2_sub_group,     :user3_sub_group,     :user4_sub_group]
+      [:direct, :descendants, :inherited] | :group         | [:user1_sub_sub_group, :user2_group,         :user3_sub_group,     :user4_group]
+      nil                                 | :sub_group     | [:user1_sub_group,     :user2_group,         :user3_sub_group,     :user4_group]
+      [:direct]                           | :sub_group     | [:user1_sub_group,     :user2_sub_group,     :user3_sub_group,     :user4_sub_group]
+      [:inherited]                        | :sub_group     | [:user1_group,         :user2_group,         :user3_group,         :user4_group]
+      [:descendants]                      | :sub_group     | [:user1_sub_sub_group, :user2_sub_sub_group, :user3_sub_sub_group, :user4_sub_sub_group]
+      [:direct, :inherited]               | :sub_group     | [:user1_sub_group,     :user2_group,         :user3_sub_group,     :user4_group]
+      [:direct, :descendants]             | :sub_group     | [:user1_sub_sub_group, :user2_sub_group,     :user3_sub_group,     :user4_sub_group]
+      [:descendants, :inherited]          | :sub_group     | [:user1_sub_sub_group, :user2_group,         :user3_sub_sub_group, :user4_group]
+      [:direct, :descendants, :inherited] | :sub_group     | [:user1_sub_sub_group, :user2_group,         :user3_sub_group,     :user4_group]
+      nil                                 | :sub_sub_group | [:user1_sub_sub_group, :user2_group,         :user3_sub_group,     :user4_group]
+      [:direct]                           | :sub_sub_group | [:user1_sub_sub_group, :user2_sub_sub_group, :user3_sub_sub_group, :user4_sub_sub_group]
+      [:inherited]                        | :sub_sub_group | [:user1_sub_group,     :user2_group,         :user3_sub_group,     :user4_group]
+      [:descendants]                      | :sub_sub_group | []
+      [:direct, :inherited]               | :sub_sub_group | [:user1_sub_sub_group, :user2_group,         :user3_sub_group,     :user4_group]
+      [:direct, :descendants]             | :sub_sub_group | [:user1_sub_sub_group, :user2_sub_sub_group, :user3_sub_sub_group, :user4_sub_sub_group]
+      [:descendants, :inherited]          | :sub_sub_group | [:user1_sub_group,     :user2_group,         :user3_sub_group,     :user4_group]
+      [:direct, :descendants, :inherited] | :sub_sub_group | [:user1_sub_sub_group, :user2_group,         :user3_sub_group,     :user4_group]
+    end
+
+    with_them do
+      it 'returns correct members' do
+        result = if subject_relations
+                   described_class.new(groups[subject_group]).execute(include_relations: subject_relations)
+                 else
+                   described_class.new(groups[subject_group]).execute
+                 end
+
+        expect(result.to_a).to match_array(expected_members.map { |name| members[name] })
+      end
+    end
   end
 
-  it 'does not return inherited members for nested group if requested' do
-    group.add_maintainer(user1)
-    group.add_developer(user2)
-    member2 = nested_group.add_maintainer(user2)
-    member3 = nested_group.add_maintainer(user3)
+  context 'search' do
+    it 'returns searched members if requested' do
+      group.add_maintainer(user2)
+      group.add_developer(user3)
+      member = group.add_maintainer(user1)
 
-    result = described_class.new(nested_group).execute(include_relations: [:direct])
+      result = described_class.new(group, params: { search: user1.name }).execute
 
-    expect(result.to_a).to match_array([member2, member3])
-  end
+      expect(result.to_a).to match_array([member])
+    end
 
-  it 'returns only inherited members for nested group if requested' do
-    group.add_developer(user2)
-    nested_group.request_access(user4)
-    member1 = group.add_maintainer(user1)
-    nested_group.add_maintainer(user2)
-    nested_group.add_maintainer(user3)
+    it 'returns nothing if search only in inherited relation' do
+      group.add_maintainer(user2)
+      group.add_developer(user3)
+      group.add_maintainer(user1)
 
-    result = described_class.new(nested_group).execute(include_relations: [:inherited])
+      result = described_class.new(group, params: { search: user1.name }).execute(include_relations: [:inherited])
 
-    expect(result.to_a).to match_array([member1])
-  end
+      expect(result.to_a).to match_array([])
+    end
 
-  it 'does not return nil if `inherited only` relation is requested on root group' do
-    group.add_developer(user2)
+    it 'returns searched member only from sub_group if search only in inherited relation' do
+      group.add_maintainer(user2)
+      group.add_developer(user3)
+      sub_group.add_maintainer(create(:user, name: user1.name))
+      member = group.add_maintainer(user1)
 
-    result = described_class.new(group).execute(include_relations: [:inherited])
+      result = described_class.new(sub_group, params: { search: member.user.name }).execute(include_relations: [:inherited])
 
-    expect(result).not_to be_nil
+      expect(result.to_a).to contain_exactly(member)
+    end
   end
 
-  it 'returns members for descendant groups if requested' do
-    member1 = group.add_maintainer(user2)
-    member2 = group.add_maintainer(user1)
-    nested_group.add_maintainer(user2)
-    member3 = nested_group.add_maintainer(user3)
-    member4 = nested_group.add_maintainer(user4)
+  context 'filter by two-factor' do
+    it 'returns members with two-factor auth if requested by owner' do
+      group.add_owner(user2)
+      group.add_maintainer(user1)
+      member = group.add_maintainer(user5)
 
-    result = described_class.new(group).execute(include_relations: [:direct, :descendants])
+      result = described_class.new(group, user2, params: { two_factor: 'enabled' }).execute
 
-    expect(result.to_a).to match_array([member1, member2, member3, member4])
-  end
+      expect(result.to_a).to contain_exactly(member)
+    end
 
-  it 'returns searched members if requested' do
-    group.add_maintainer(user2)
-    group.add_developer(user3)
-    member = group.add_maintainer(user1)
+    it 'returns members without two-factor auth if requested by owner' do
+      member1 = group.add_owner(user2)
+      member2 = group.add_maintainer(user1)
+      member_with_2fa = group.add_maintainer(user5)
 
-    result = described_class.new(group, params: { search: user1.name }).execute
+      result = described_class.new(group, user2, params: { two_factor: 'disabled' }).execute
 
-    expect(result.to_a).to match_array([member])
-  end
+      expect(result.to_a).not_to include(member_with_2fa)
+      expect(result.to_a).to match_array([member1, member2])
+    end
 
-  it 'returns nothing if search only in inherited relation' do
-    group.add_maintainer(user2)
-    group.add_developer(user3)
-    group.add_maintainer(user1)
+    it 'returns direct members with two-factor auth if requested by owner' do
+      group.add_owner(user1)
+      group.add_maintainer(user2)
+      sub_group.add_maintainer(user3)
+      member_with_2fa = sub_group.add_maintainer(user5)
 
-    result = described_class.new(group, params: { search: user1.name }).execute(include_relations: [:inherited])
+      result = described_class.new(sub_group, user1, params: { two_factor: 'enabled' }).execute(include_relations: [:direct])
 
-    expect(result.to_a).to match_array([])
-  end
+      expect(result.to_a).to match_array([member_with_2fa])
+    end
 
-  it 'returns searched member only from nested_group if search only in inherited relation' do
-    group.add_maintainer(user2)
-    group.add_developer(user3)
-    nested_group.add_maintainer(create(:user, name: user1.name))
-    member = group.add_maintainer(user1)
+    it 'returns inherited members with two-factor auth if requested by owner' do
+      group.add_owner(user1)
+      member_with_2fa = group.add_maintainer(user5)
+      sub_group.add_maintainer(user2)
+      sub_group.add_maintainer(user3)
 
-    result = described_class.new(nested_group, params: { search: member.user.name }).execute(include_relations: [:inherited])
+      result = described_class.new(sub_group, user1, params: { two_factor: 'enabled' }).execute(include_relations: [:inherited])
 
-    expect(result.to_a).to contain_exactly(member)
-  end
-
-  it 'returns members with two-factor auth if requested by owner' do
-    group.add_owner(user2)
-    group.add_maintainer(user1)
-    member = group.add_maintainer(user5)
-
-    result = described_class.new(group, user2, params: { two_factor: 'enabled' }).execute
+      expect(result.to_a).to match_array([member_with_2fa])
+    end
 
-    expect(result.to_a).to contain_exactly(member)
-  end
-
-  it 'returns members without two-factor auth if requested by owner' do
-    member1 = group.add_owner(user2)
-    member2 = group.add_maintainer(user1)
-    member_with_2fa = group.add_maintainer(user5)
+    it 'returns direct members without two-factor auth if requested by owner' do
+      group.add_owner(user1)
+      group.add_maintainer(user2)
+      member3 = sub_group.add_maintainer(user3)
+      sub_group.add_maintainer(user5)
 
-    result = described_class.new(group, user2, params: { two_factor: 'disabled' }).execute
+      result = described_class.new(sub_group, user1, params: { two_factor: 'disabled' }).execute(include_relations: [:direct])
 
-    expect(result.to_a).not_to include(member_with_2fa)
-    expect(result.to_a).to match_array([member1, member2])
-  end
-
-  it 'returns direct members with two-factor auth if requested by owner' do
-    group.add_owner(user1)
-    group.add_maintainer(user2)
-    nested_group.add_maintainer(user3)
-    member_with_2fa = nested_group.add_maintainer(user5)
-
-    result = described_class.new(nested_group, user1, params: { two_factor: 'enabled' }).execute(include_relations: [:direct])
-
-    expect(result.to_a).to match_array([member_with_2fa])
-  end
-
-  it 'returns inherited members with two-factor auth if requested by owner' do
-    group.add_owner(user1)
-    member_with_2fa = group.add_maintainer(user5)
-    nested_group.add_maintainer(user2)
-    nested_group.add_maintainer(user3)
-
-    result = described_class.new(nested_group, user1, params: { two_factor: 'enabled' }).execute(include_relations: [:inherited])
-
-    expect(result.to_a).to match_array([member_with_2fa])
-  end
-
-  it 'returns direct members without two-factor auth if requested by owner' do
-    group.add_owner(user1)
-    group.add_maintainer(user2)
-    member3 = nested_group.add_maintainer(user3)
-    nested_group.add_maintainer(user5)
-
-    result = described_class.new(nested_group, user1, params: { two_factor: 'disabled' }).execute(include_relations: [:direct])
-
-    expect(result.to_a).to match_array([member3])
-  end
+      expect(result.to_a).to match_array([member3])
+    end
 
-  it 'returns inherited members without two-factor auth if requested by owner' do
-    member1 = group.add_owner(user1)
-    group.add_maintainer(user5)
-    nested_group.add_maintainer(user2)
-    nested_group.add_maintainer(user3)
+    it 'returns inherited members without two-factor auth if requested by owner' do
+      member1 = group.add_owner(user1)
+      group.add_maintainer(user5)
+      sub_group.add_maintainer(user2)
+      sub_group.add_maintainer(user3)
 
-    result = described_class.new(nested_group, user1, params: { two_factor: 'disabled' }).execute(include_relations: [:inherited])
+      result = described_class.new(sub_group, user1, params: { two_factor: 'disabled' }).execute(include_relations: [:inherited])
 
-    expect(result.to_a).to match_array([member1])
+      expect(result.to_a).to match_array([member1])
+    end
   end
 end

spec/models/member_spec.rb

@@ -413,6 +413,24 @@ RSpec.describe Member do
       it { is_expected.not_to include @blocked_developer }
       it { is_expected.not_to include @member_with_minimal_access }
     end
+
+    describe '.distinct_on_user_with_max_access_level' do
+      let_it_be(:other_group) { create(:group) }
+      let_it_be(:member_with_lower_access_level) { create(:group_member, :developer, group: other_group, user: @owner_user) }
+
+      subject { described_class.default_scoped.distinct_on_user_with_max_access_level.to_a }
+
+      it { is_expected.not_to include member_with_lower_access_level }
+      it { is_expected.to include @owner }
+      it { is_expected.to include @maintainer }
+      it { is_expected.to include @invited_member }
+      it { is_expected.to include @accepted_invite_member }
+      it { is_expected.to include @requested_member }
+      it { is_expected.to include @accepted_request_member }
+      it { is_expected.to include @blocked_maintainer }
+      it { is_expected.to include @blocked_developer }
+      it { is_expected.to include @member_with_minimal_access }
+    end
   end
 
   describe "Delegate methods" do