Merge branch 'fix/group_members_max_access_level' into 'master'

Fix effective access level of group members

See merge request gitlab-org/gitlab!56677

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/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