From d2a0d683f10fbd017bc0ec30a237f43699ee39a1 Mon Sep 17 00:00:00 2001
From: Abdul Wadood <awadood@gitlab.com>
Date: Mon, 19 Feb 2024 12:51:59 +0530
Subject: [PATCH 1/4] Mask membership source if the current user cannot access
 the source

As part of https://gitlab.com/gitlab-org/gitlab/-/issues/219230, we will
start showing the invited group members on the project/group members
page. However, the current user might not have access to the invited
group so depending on different cases we are hiding the source of
the invited group member or the invited member itself.

If the invited group is public then we will always show its members on
the shared project/group page. But if it's private and it's invited to a
public group/project then the following cases are there:

1. Current user is unauthenticated - The user won't see the members from
the invited group on the shared project/group members page.
2. Current user is a non-member of the invited group and the shared
group/project - This is the same as point 1.
3. Current user is a member of the shared group/project but not of the
invited group - The user will see the members of the invited group
but the source of membership will be masked.
4. Current user is a member of the invited group - The user will be able
to see the source of membership.
5. Current user is the maintainer/owner of the shared project or owner
of the group - The user can see the source of membership to manage
the project/group memberships.
---
 app/models/member.rb                          |   3 +
 ...ed_private_group_accessibility_assigner.rb |  63 ++++++++
 .../members/members/members_with_parents.rb   |   2 +-
 ...er_max_access_level_in_groups_preloader.rb |   2 +-
 app/serializers/member_entity.rb              |   9 +-
 app/serializers/member_serializer.rb          |   2 +
 ...ctive_access_level_per_user_finder_spec.rb |   8 +-
 .../projects/project_members_helper_spec.rb   |   3 +
 ...ivate_group_accessibility_assigner_spec.rb | 151 ++++++++++++++++++
 spec/serializers/member_entity_spec.rb        |  48 +++++-
 spec/serializers/member_serializer_spec.rb    |  95 ++++++++++-
 11 files changed, 372 insertions(+), 14 deletions(-)
 create mode 100644 app/models/members/members/invited_private_group_accessibility_assigner.rb
 create mode 100644 spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb

diff --git a/app/models/member.rb b/app/models/member.rb
index d3c502410affd6..d9517c6c1ad576 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -293,6 +293,9 @@ class Member < ApplicationRecord
   end
 
   attribute :notification_level, default: -> { NotificationSetting.levels[:global] }
+  # Only false when the current user is a member of the shared group or project but not of the invited private group
+  # so the current user can't see the source of the membership.
+  attribute :is_source_accessible_to_current_user, default: true
 
   class << self
     def search(query)
diff --git a/app/models/members/members/invited_private_group_accessibility_assigner.rb b/app/models/members/members/invited_private_group_accessibility_assigner.rb
new file mode 100644
index 00000000000000..3a7e56180d6687
--- /dev/null
+++ b/app/models/members/members/invited_private_group_accessibility_assigner.rb
@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+module Members
+  # We allow the current user to see the invited private group when the current user is a member of the shared group to
+  # allow better collaboration between the two groups even though the current user is not a member of the invited group.
+  # We don't allow the current user to see the source of membership i.e. the group name, path, and other group info as
+  # it's sensitive information if the current user is not an owner of the group or at least maintainer of the project.
+  # This class deals with setting `is_source_accessible_to_current_user` which is used to hide or show the source of
+  # memberships as per the above cases.
+  class InvitedPrivateGroupAccessibilityAssigner
+    include Gitlab::Allowable
+    include Gitlab::Utils::StrongMemoize
+
+    def initialize(members, source:, current_user:)
+      @members = members
+      @source = source
+      @current_user = current_user
+    end
+
+    def execute
+      # We don't need to calculate the access level of the current user in the invited groups if:
+      #
+      # 1. The current user is unauthenticated we don't return private members from the finder itself.
+      # 2. The current user can admin members then the user should be able to see the source of all memberships
+      #    to enable management of group/project memberships.
+      # 3. There are no members invited from a private group.
+      return if current_user.nil? || can_admin_members? || private_invited_group_members.nil?
+
+      # rubocop:disable Database/AvoidUsingPluckWithoutLimit -- Plucking array so it's fine
+      private_invited_group_ids = private_invited_group_members.pluck(:source_id)
+      # rubocop:enable Database/AvoidUsingPluckWithoutLimit
+
+      max_access_for_group_ids = current_user.max_member_access_for_group_ids(private_invited_group_ids)
+
+      private_invited_group_members.each do |member|
+        member.is_source_accessible_to_current_user =
+          max_access_for_group_ids[member.source_id] >= Gitlab::Access::GUEST
+      end
+    end
+
+    private
+
+    attr_reader :members, :source, :current_user
+
+    def private_invited_group_members
+      members.reject do |member|
+        # The user can see those project/group members where:
+        # - The project/group is public.
+        # - The member is direct or inherited.
+        member.source.visibility_level == Gitlab::VisibilityLevel::PUBLIC ||
+          member.source_id == source.id || # ProjectMember type isn't inherited or shared so they are rejected here
+          member.source.traversal_ids.include?(source.id)
+      end
+    end
+    strong_memoize_attr(:private_invited_group_members)
+
+    def can_admin_members?
+      return can?(current_user, :admin_project_member, source) if source.is_a?(Project)
+
+      can?(current_user, :admin_group_member, source)
+    end
+  end
+end
diff --git a/app/models/members/members/members_with_parents.rb b/app/models/members/members/members_with_parents.rb
index 61ce99e1f3e56a..3598a146e8df9e 100644
--- a/app/models/members/members/members_with_parents.rb
+++ b/app/models/members/members/members_with_parents.rb
@@ -75,7 +75,7 @@ def members_from_self_and_ancestor_group_shares
 
       # Instead of members.access_level, we need to maximize that access_level at
       # the respective group_group_links.group_access.
-      member_columns = GroupMember.attribute_names.map do |column_name|
+      member_columns = GroupMember.column_names.map do |column_name|
         if column_name == 'access_level'
           smallest_value_arel([cte_alias[:group_access], group_member_table[:access_level]], 'access_level')
         else
diff --git a/app/models/preloaders/user_max_access_level_in_groups_preloader.rb b/app/models/preloaders/user_max_access_level_in_groups_preloader.rb
index aaa54e0228b775..4615b22cfbab82 100644
--- a/app/models/preloaders/user_max_access_level_in_groups_preloader.rb
+++ b/app/models/preloaders/user_max_access_level_in_groups_preloader.rb
@@ -48,7 +48,7 @@ def alter_direct_memberships_to_make_it_act_like_memberships_in_shared_groups
       group_group_link_table = GroupGroupLink.arel_table
       group_member_table = GroupMember.arel_table
 
-      altered_columns = GroupMember.attribute_names.map do |column_name|
+      altered_columns = GroupMember.column_names.map do |column_name|
         case column_name
         when 'access_level'
           # Consider the limiting effect of group share's access level
diff --git a/app/serializers/member_entity.rb b/app/serializers/member_entity.rb
index f259cba65a6ecf..450937d9cb6efc 100644
--- a/app/serializers/member_entity.rb
+++ b/app/serializers/member_entity.rb
@@ -11,7 +11,8 @@ class MemberEntity < Grape::Entity
   end
   expose :requested_at
 
-  expose :created_by, if: -> (member) { member.created_by.present? } do |member|
+  expose :created_by,
+    if: -> (member) { member.created_by.present? && member.is_source_accessible_to_current_user } do |member|
     UserEntity.represent(member.created_by, only: [:name, :web_url])
   end
 
@@ -38,10 +39,14 @@ class MemberEntity < Grape::Entity
 
   expose :custom_permissions
 
-  expose :source do |member|
+  expose :source, if: -> (member) { member.is_source_accessible_to_current_user } do |member|
     GroupEntity.represent(member.source, only: [:id, :full_name, :web_url])
   end
 
+  expose :is_shared_with_group_private do |member|
+    !member.is_source_accessible_to_current_user
+  end
+
   expose :type
 
   expose :valid_level_roles, as: :valid_roles
diff --git a/app/serializers/member_serializer.rb b/app/serializers/member_serializer.rb
index ad258b0ef1e972..aa79a6ebb80187 100644
--- a/app/serializers/member_serializer.rb
+++ b/app/serializers/member_serializer.rb
@@ -5,6 +5,8 @@ class MemberSerializer < BaseSerializer
 
   def represent(members, opts = {})
     LastGroupOwnerAssigner.new(opts[:group], members).execute unless opts[:source].is_a?(Project)
+    Members::InvitedPrivateGroupAccessibilityAssigner
+      .new(members, source: opts[:source], current_user: opts[:current_user]).execute
 
     super(members, opts)
   end
diff --git a/spec/finders/projects/members/effective_access_level_per_user_finder_spec.rb b/spec/finders/projects/members/effective_access_level_per_user_finder_spec.rb
index 3872938d20e8d0..91452fe0d88629 100644
--- a/spec/finders/projects/members/effective_access_level_per_user_finder_spec.rb
+++ b/spec/finders/projects/members/effective_access_level_per_user_finder_spec.rb
@@ -26,13 +26,13 @@
     end
 
     it 'includes the highest access level from all avenues of memberships for the specific user alone' do
-      expect(subject).to eq(
-        [{
+      expect(subject.first).to match(hash_including(
+        {
           'user_id' => user.id,
           'access_level' => Gitlab::Access::MAINTAINER, # From project_group_link
           'id' => nil
-        }]
-      )
+        }
+      ))
     end
   end
 end
diff --git a/spec/helpers/projects/project_members_helper_spec.rb b/spec/helpers/projects/project_members_helper_spec.rb
index 2cc87e8aeb99df..0a85b28c91911b 100644
--- a/spec/helpers/projects/project_members_helper_spec.rb
+++ b/spec/helpers/projects/project_members_helper_spec.rb
@@ -113,6 +113,9 @@
           let_it_be(:top_group) { create(:group) }
           let_it_be(:sub_group) { create(:group, parent: top_group) }
           let_it_be(:project) { create(:project, group: sub_group) }
+          let_it_be(:members) { create_list(:project_member, 2, project: project) }
+          let_it_be(:invited) { create_list(:project_member, 2, :invited, project: project) }
+          let_it_be(:access_requests) { create_list(:project_member, 2, :access_request, project: project) }
           let_it_be(:group_link_1) { create(:group_group_link, shared_group: top_group, shared_with_group: shared_with_group_1, group_access: Gitlab::Access::GUEST) }
           let_it_be(:group_link_2) { create(:group_group_link, shared_group: top_group, shared_with_group: shared_with_group_4, group_access: Gitlab::Access::GUEST) }
           let_it_be(:group_link_3) { create(:group_group_link, shared_group: top_group, shared_with_group: shared_with_group_5, group_access: Gitlab::Access::DEVELOPER) }
diff --git a/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb b/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb
new file mode 100644
index 00000000000000..b1a4a6d7379b32
--- /dev/null
+++ b/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb
@@ -0,0 +1,151 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Members::InvitedPrivateGroupAccessibilityAssigner, feature_category: :groups_and_projects do
+  using RSpec::Parameterized::TableSyntax
+
+  let_it_be(:user) { create(:user) }
+
+  describe '#execute' do
+    shared_examples 'assigns is_source_accessible_to_current_user' do
+      let(:current_user) { user }
+      let_it_be(:member_user) { create(:user) }
+
+      subject(:assigner) { described_class.new(members, source: source, current_user: current_user) }
+
+      context 'for direct members' do
+        let_it_be(:source) { create(source_type) } # rubocop:disable Rails/SaveBang -- Using factory, not ActiveRecord
+        let_it_be(:direct_member) { source.add_developer(member_user) }
+        let(:members) { [direct_member] }
+
+        it 'sets is_source_accessible_to_current_user to true for all members' do
+          assigner.execute
+
+          expect(members.map(&:is_source_accessible_to_current_user)).to all(be_truthy)
+        end
+      end
+
+      context 'for inherited members' do
+        let_it_be(:parent) { create(:group) }
+        let_it_be(:source) { create(source_type, parent_key => parent) }
+        let_it_be(:inherited_member) { parent.add_developer(member_user) }
+        let(:members) { [inherited_member] }
+
+        it 'sets is_source_accessible_to_current_user to true for all members' do
+          assigner.execute
+
+          expect(members.first.is_source_accessible_to_current_user).to eq(true)
+        end
+      end
+
+      context 'for shared source members' do
+        let(:shared_source) { create(source_type, shared_source_visibility) }
+        let(:invited_group) { create(:group, invited_group_visibility) }
+        let(:source) { shared_source }
+        let(:invited_member) { invited_group.add_developer(member_user) }
+        let(:members) { [invited_member] }
+        let!(:link) { create_link(source, invited_group) }
+
+        shared_examples 'sets correct is_source_accessible_to_current_user for invited members' do
+          with_them do
+            specify do
+              assigner.execute
+
+              expect(members.first.is_source_accessible_to_current_user).to eq(can_see_invited_members_source?)
+            end
+
+            context 'with multiple members belonging to different sources' do
+              it 'avoid N+1 queries' do
+                assigner # Initialize objects in let blocks
+                recorder = ActiveRecord::QueryRecorder.new { assigner.execute }
+
+                members = create_list(:group, 3, invited_group_visibility).map do |invited_group|
+                  create_link(shared_source, invited_group)
+                  create(:group_member, group: invited_group)
+                end
+
+                assigner = described_class.new(members, source: shared_source, current_user: current_user)
+                expect { assigner.execute }.not_to exceed_query_limit(recorder)
+              end
+            end
+          end
+        end
+
+        context 'when current user is unauthenticated' do
+          let(:current_user) { nil }
+
+          where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
+            :public  | :public  | true
+            :public  | :private | true # Not setting to false as the finder must not return private members with no user
+          end
+
+          include_examples 'sets correct is_source_accessible_to_current_user for invited members'
+        end
+
+        context 'when current user non-member of invited group' do
+          where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
+            :public  | :public  | true
+            :public  | :private | false
+          end
+
+          include_examples 'sets correct is_source_accessible_to_current_user for invited members'
+        end
+
+        context 'when current user a member of shared group but not of invited group' do
+          before do
+            shared_source.add_developer(current_user)
+          end
+
+          where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
+            :public  | :public  | true
+            :public  | :private | false
+            :private | :public  | true
+            :private | :private | false
+          end
+
+          include_examples 'sets correct is_source_accessible_to_current_user for invited members'
+        end
+
+        context 'when current user can manage member of shared group but not of invited group' do
+          before do
+            shared_source.add_member(current_user, admin_member_access)
+          end
+
+          where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
+            :public  | :public  | true
+            :public  | :private | true
+            :private | :public  | true
+            :private | :private | true
+          end
+
+          include_examples 'sets correct is_source_accessible_to_current_user for invited members'
+        end
+      end
+    end
+
+    context 'for project members' do
+      let_it_be(:source_type) { 'project' }
+      let_it_be(:admin_member_access) { Gitlab::Access::MAINTAINER }
+      let_it_be(:parent_key) { :group }
+
+      it_behaves_like 'assigns is_source_accessible_to_current_user'
+
+      def create_link(shared, invited)
+        create(:project_group_link, project: shared, group: invited)
+      end
+    end
+
+    context 'for group members' do
+      let_it_be(:source_type) { 'group' }
+      let_it_be(:admin_member_access) { Gitlab::Access::OWNER }
+      let_it_be(:parent_key) { :parent }
+
+      it_behaves_like 'assigns is_source_accessible_to_current_user'
+
+      def create_link(shared, invited)
+        create(:group_group_link, shared_group: shared, shared_with_group: invited)
+      end
+    end
+  end
+end
diff --git a/spec/serializers/member_entity_spec.rb b/spec/serializers/member_entity_spec.rb
index 350355eb72d066..2f4a803d61bd79 100644
--- a/spec/serializers/member_entity_spec.rb
+++ b/spec/serializers/member_entity_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe MemberEntity do
+RSpec.describe MemberEntity, feature_category: :groups_and_projects do
   let_it_be(:current_user) { create(:user) }
 
   let(:entity) { described_class.new(member, { current_user: current_user, group: group, source: source }) }
@@ -24,6 +24,28 @@
 
       expect(entity_hash[:can_remove]).to be(true)
     end
+
+    context 'when is_source_accessible_to_current_user is true' do
+      before do
+        allow(member).to receive(:is_source_accessible_to_current_user).and_return(true)
+      end
+
+      it 'exposes source and created_by' do
+        expect(entity_hash[:source]).to be_present
+        expect(entity_hash[:created_by]).to be_present
+      end
+    end
+
+    context 'when is_source_accessible_to_current_user is false' do
+      before do
+        allow(member).to receive(:is_source_accessible_to_current_user).and_return(false)
+      end
+
+      it 'does not exposes source and created_by' do
+        expect(entity_hash[:source]).to be_nil
+        expect(entity_hash[:created_by]).to be_nil
+      end
+    end
   end
 
   shared_examples 'invite' do
@@ -72,12 +94,20 @@
   context 'group member' do
     let(:group) { create(:group) }
     let(:source) { group }
-    let(:member) { GroupMemberPresenter.new(create(:group_member, group: group), current_user: current_user) }
+    let(:member) do
+      GroupMemberPresenter.new(
+        create(:group_member, group: group, created_by: current_user), current_user: current_user
+      )
+    end
 
     it_behaves_like 'member.json'
 
     context 'invite' do
-      let(:member) { GroupMemberPresenter.new(create(:group_member, :invited, group: group), current_user: current_user) }
+      let(:member) do
+        GroupMemberPresenter.new(
+          create(:group_member, :invited, group: group, created_by: current_user), current_user: current_user
+        )
+      end
 
       it_behaves_like 'member.json'
       it_behaves_like 'invite'
@@ -125,12 +155,20 @@
     let(:project) { create(:project) }
     let(:group) { project.group }
     let(:source) { project }
-    let(:member) { ProjectMemberPresenter.new(create(:project_member, project: project), current_user: current_user) }
+    let(:member) do
+      ProjectMemberPresenter.new(
+        create(:project_member, project: project, created_by: current_user), current_user: current_user
+      )
+    end
 
     it_behaves_like 'member.json'
 
     context 'invite' do
-      let(:member) { ProjectMemberPresenter.new(create(:project_member, :invited, project: project), current_user: current_user) }
+      let(:member) do
+        ProjectMemberPresenter.new(
+          create(:project_member, :invited, project: project, created_by: current_user), current_user: current_user
+        )
+      end
 
       it_behaves_like 'member.json'
       it_behaves_like 'invite'
diff --git a/spec/serializers/member_serializer_spec.rb b/spec/serializers/member_serializer_spec.rb
index c35ecf5f63606d..9ec121efd557a5 100644
--- a/spec/serializers/member_serializer_spec.rb
+++ b/spec/serializers/member_serializer_spec.rb
@@ -2,8 +2,9 @@
 
 require 'spec_helper'
 
-RSpec.describe MemberSerializer do
+RSpec.describe MemberSerializer, feature_category: :groups_and_projects do
   include MembersPresentation
+  using RSpec::Parameterized::TableSyntax
 
   let_it_be(:current_user) { create(:user) }
 
@@ -15,6 +16,80 @@
     it { is_expected.to match_schema('members') }
   end
 
+  shared_examples 'shared source members' do
+    let_it_be(:member_user) { create(:user) }
+    let(:shared_source) { create(source_type, shared_source_visibility) }
+    let(:invited_group) { create(:group, invited_group_visibility) }
+    let(:source) { shared_source }
+    let(:group) { source.is_a?(Project) ? source.group : source }
+    let(:invited_member) { invited_group.add_developer(member_user) }
+    let(:members) { present_members([invited_member]) }
+
+    shared_examples 'exposes source correctly' do
+      with_them do
+        before do
+          create_link(source, invited_group)
+        end
+
+        specify do
+          representation
+
+          expect(invited_member.is_source_accessible_to_current_user).to eq(can_see_invited_members_source?)
+        end
+      end
+    end
+
+    context 'when current user is unauthenticated' do
+      let(:current_user) { nil }
+
+      where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
+        :public  | :public  | true
+        :public  | :private | true # Not setting to false as the finder must not return private members with no user
+      end
+
+      include_examples 'exposes source correctly'
+    end
+
+    context 'when current user non-member of invited group' do
+      where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
+        :public  | :public  | true
+        :public  | :private | false
+      end
+
+      include_examples 'exposes source correctly'
+    end
+
+    context 'when current user a member of shared group but not of invited group' do
+      before do
+        shared_source.add_developer(current_user)
+      end
+
+      where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
+        :public  | :public  | true
+        :public  | :private | false
+        :private | :public  | true
+        :private | :private | false
+      end
+
+      include_examples 'exposes source correctly'
+    end
+
+    context 'when current user can manage member of shared group but not of invited group' do
+      before do
+        shared_source.add_member(current_user, admin_member_access)
+      end
+
+      where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
+        :public  | :public  | true
+        :public  | :private | true
+        :private | :public  | true
+        :private | :private | true
+      end
+
+      include_examples 'exposes source correctly'
+    end
+  end
+
   context 'group member' do
     let_it_be(:group) { create(:group) }
     let_it_be(:members) { present_members(create_list(:group_member, 1, group: group)) }
@@ -29,6 +104,15 @@
       expect { representation }.to change(group_member, :last_owner)
                                      .from(nil).to(true)
     end
+
+    it_behaves_like 'shared source members' do
+      let_it_be(:source_type) { :group }
+      let_it_be(:admin_member_access) { Gitlab::Access::OWNER }
+
+      def create_link(shared, invited)
+        create(:group_group_link, shared_group: shared, shared_with_group: invited)
+      end
+    end
   end
 
   context 'project member' do
@@ -45,5 +129,14 @@
 
       representation
     end
+
+    it_behaves_like 'shared source members' do
+      let_it_be(:source_type) { :project }
+      let_it_be(:admin_member_access) { Gitlab::Access::MAINTAINER }
+
+      def create_link(shared, invited)
+        create(:project_group_link, project: shared, group: invited)
+      end
+    end
   end
 end
-- 
GitLab


From 4246796823417f43f64cff2b7452dc11e877b312 Mon Sep 17 00:00:00 2001
From: Abdul Wadood <awadood@gitlab.com>
Date: Tue, 20 Feb 2024 13:18:25 +0530
Subject: [PATCH 2/4] Take group sharing access into account when calculating
 max access level

To mask the source group only when it's not accessible.
---
 ...ed_private_group_accessibility_assigner.rb | 10 ++++----
 ...ivate_group_accessibility_assigner_spec.rb | 23 ++++++++++++++++---
 spec/serializers/member_serializer_spec.rb    |  6 ++---
 3 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/app/models/members/members/invited_private_group_accessibility_assigner.rb b/app/models/members/members/invited_private_group_accessibility_assigner.rb
index 3a7e56180d6687..90b8b27b0a40a0 100644
--- a/app/models/members/members/invited_private_group_accessibility_assigner.rb
+++ b/app/models/members/members/invited_private_group_accessibility_assigner.rb
@@ -26,15 +26,13 @@ def execute
       # 3. There are no members invited from a private group.
       return if current_user.nil? || can_admin_members? || private_invited_group_members.nil?
 
-      # rubocop:disable Database/AvoidUsingPluckWithoutLimit -- Plucking array so it's fine
-      private_invited_group_ids = private_invited_group_members.pluck(:source_id)
+      # rubocop:disable Database/AvoidUsingPluckWithoutLimit -- Plucking array as we have limited results
+      private_invited_group_ids = private_invited_group_members.pluck(:source_id).uniq
+      authorized_groups = current_user.authorized_groups.id_in(private_invited_group_ids).pluck(:id)
       # rubocop:enable Database/AvoidUsingPluckWithoutLimit
 
-      max_access_for_group_ids = current_user.max_member_access_for_group_ids(private_invited_group_ids)
-
       private_invited_group_members.each do |member|
-        member.is_source_accessible_to_current_user =
-          max_access_for_group_ids[member.source_id] >= Gitlab::Access::GUEST
+        member.is_source_accessible_to_current_user = authorized_groups.include?(member.source_id)
       end
     end
 
diff --git a/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb b/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb
index b1a4a6d7379b32..627eca3575a884 100644
--- a/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb
+++ b/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb
@@ -83,7 +83,7 @@
           include_examples 'sets correct is_source_accessible_to_current_user for invited members'
         end
 
-        context 'when current user non-member of invited group' do
+        context 'when current user non-member of shared source' do
           where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
             :public  | :public  | true
             :public  | :private | false
@@ -92,7 +92,7 @@
           include_examples 'sets correct is_source_accessible_to_current_user for invited members'
         end
 
-        context 'when current user a member of shared group but not of invited group' do
+        context 'when current user a member of shared source but not of invited group' do
           before do
             shared_source.add_developer(current_user)
           end
@@ -107,7 +107,24 @@
           include_examples 'sets correct is_source_accessible_to_current_user for invited members'
         end
 
-        context 'when current user can manage member of shared group but not of invited group' do
+        context 'when current user a direct member of shared group and of invited group through sharing' do
+          before do
+            group = create(:group, :private)
+            group.add_developer(current_user)
+            create(:group_group_link, shared_group: invited_group, shared_with_group: group)
+          end
+
+          where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
+            :public  | :public  | true
+            :public  | :private | true
+            :private | :public  | true
+            :private | :private | true
+          end
+
+          include_examples 'sets correct is_source_accessible_to_current_user for invited members'
+        end
+
+        context 'when current user can manage member of shared group not invited group members' do
           before do
             shared_source.add_member(current_user, admin_member_access)
           end
diff --git a/spec/serializers/member_serializer_spec.rb b/spec/serializers/member_serializer_spec.rb
index 9ec121efd557a5..c13f4174e177b1 100644
--- a/spec/serializers/member_serializer_spec.rb
+++ b/spec/serializers/member_serializer_spec.rb
@@ -50,7 +50,7 @@
       include_examples 'exposes source correctly'
     end
 
-    context 'when current user non-member of invited group' do
+    context 'when current user non-member of shared source' do
       where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
         :public  | :public  | true
         :public  | :private | false
@@ -59,7 +59,7 @@
       include_examples 'exposes source correctly'
     end
 
-    context 'when current user a member of shared group but not of invited group' do
+    context 'when current user a member of shared source but not of invited group' do
       before do
         shared_source.add_developer(current_user)
       end
@@ -74,7 +74,7 @@
       include_examples 'exposes source correctly'
     end
 
-    context 'when current user can manage member of shared group but not of invited group' do
+    context 'when current user can manage member of shared group but not invited group members' do
       before do
         shared_source.add_member(current_user, admin_member_access)
       end
-- 
GitLab


From f656062e8f725fcf30d12b81462cc705734f6140 Mon Sep 17 00:00:00 2001
From: Abdul Wadood <awadood@gitlab.com>
Date: Thu, 29 Feb 2024 13:22:23 +0530
Subject: [PATCH 3/4] Compare source objects instead of ids

As Project and Group ids can clash and create an edge case where we
expose private group details to unauthorized user. The ActiveRecord `==`
method will check the type of object and the id so it will ensure the
`member.source` is equal to the `source`.
---
 .../members/invited_private_group_accessibility_assigner.rb     | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/members/members/invited_private_group_accessibility_assigner.rb b/app/models/members/members/invited_private_group_accessibility_assigner.rb
index 90b8b27b0a40a0..0003e8efda5830 100644
--- a/app/models/members/members/invited_private_group_accessibility_assigner.rb
+++ b/app/models/members/members/invited_private_group_accessibility_assigner.rb
@@ -46,7 +46,7 @@ def private_invited_group_members
         # - The project/group is public.
         # - The member is direct or inherited.
         member.source.visibility_level == Gitlab::VisibilityLevel::PUBLIC ||
-          member.source_id == source.id || # ProjectMember type isn't inherited or shared so they are rejected here
+          member.source == source || # ProjectMember type isn't inherited or shared so they are rejected here
           member.source.traversal_ids.include?(source.id)
       end
     end
-- 
GitLab


From 5c5d6fc066618c67eb7c791b8d84297bb41102eb Mon Sep 17 00:00:00 2001
From: Abdul Wadood <awadood@gitlab.com>
Date: Mon, 4 Mar 2024 12:58:49 +0530
Subject: [PATCH 4/4] Address review suggestions

- Calculate `is_source_accessible_to_current_user` even when
  current_user is nil to avoid consistencies in the code which are
  difficult to understand.
- Use `select` instead of `reject` for readability.
- Raise an argument error when `members` is not an instance of Array or
  MembersPresenter.
---
 ...ed_private_group_accessibility_assigner.rb | 41 +++++++++++--------
 ...ivate_group_accessibility_assigner_spec.rb | 14 ++++++-
 spec/serializers/member_serializer_spec.rb    |  2 +-
 3 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/app/models/members/members/invited_private_group_accessibility_assigner.rb b/app/models/members/members/invited_private_group_accessibility_assigner.rb
index 0003e8efda5830..cd92ed797725ee 100644
--- a/app/models/members/members/invited_private_group_accessibility_assigner.rb
+++ b/app/models/members/members/invited_private_group_accessibility_assigner.rb
@@ -12,6 +12,10 @@ class InvitedPrivateGroupAccessibilityAssigner
     include Gitlab::Utils::StrongMemoize
 
     def initialize(members, source:, current_user:)
+      if !members.is_a?(Array) && !members.is_a?(MembersPresenter)
+        raise ArgumentError, "members should be an instance of Array or MembersPresenter"
+      end
+
       @members = members
       @source = source
       @current_user = current_user
@@ -20,19 +24,13 @@ def initialize(members, source:, current_user:)
     def execute
       # We don't need to calculate the access level of the current user in the invited groups if:
       #
-      # 1. The current user is unauthenticated we don't return private members from the finder itself.
-      # 2. The current user can admin members then the user should be able to see the source of all memberships
+      # 1. The current user can admin members then the user should be able to see the source of all memberships
       #    to enable management of group/project memberships.
-      # 3. There are no members invited from a private group.
-      return if current_user.nil? || can_admin_members? || private_invited_group_members.nil?
-
-      # rubocop:disable Database/AvoidUsingPluckWithoutLimit -- Plucking array as we have limited results
-      private_invited_group_ids = private_invited_group_members.pluck(:source_id).uniq
-      authorized_groups = current_user.authorized_groups.id_in(private_invited_group_ids).pluck(:id)
-      # rubocop:enable Database/AvoidUsingPluckWithoutLimit
+      # 2. There are no members invited from a private group.
+      return if can_admin_members? || private_invited_group_members.nil?
 
       private_invited_group_members.each do |member|
-        member.is_source_accessible_to_current_user = authorized_groups.include?(member.source_id)
+        member.is_source_accessible_to_current_user = authorized_group_ids.include?(member.source_id)
       end
     end
 
@@ -40,14 +38,23 @@ def execute
 
     attr_reader :members, :source, :current_user
 
+    def authorized_group_ids
+      return [] if current_user.nil?
+
+      private_invited_group_ids = private_invited_group_members.map(&:source_id).uniq
+      current_user.authorized_groups.id_in(private_invited_group_ids).map(&:id)
+    end
+    strong_memoize_attr(:authorized_group_ids)
+
     def private_invited_group_members
-      members.reject do |member|
-        # The user can see those project/group members where:
-        # - The project/group is public.
-        # - The member is direct or inherited.
-        member.source.visibility_level == Gitlab::VisibilityLevel::PUBLIC ||
-          member.source == source || # ProjectMember type isn't inherited or shared so they are rejected here
-          member.source.traversal_ids.include?(source.id)
+      members.select do |member|
+        # The user can see those members where:
+        # - The source is public.
+        # - The member is direct or inherited. ProjectMember type is always direct.
+        member.is_a?(GroupMember) &&
+          member.source.visibility_level != Gitlab::VisibilityLevel::PUBLIC &&
+          member.source_id != source.id && # Exclude direct member
+          member.source.traversal_ids.exclude?(source.id) # Exclude inherited member
       end
     end
     strong_memoize_attr(:private_invited_group_members)
diff --git a/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb b/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb
index 627eca3575a884..1f720b779cf44e 100644
--- a/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb
+++ b/spec/models/members/members/invited_private_group_accessibility_assigner_spec.rb
@@ -7,6 +7,18 @@
 
   let_it_be(:user) { create(:user) }
 
+  describe '#initialize' do
+    it 'raises an error when initializing using ActiveRecord::Relation' do
+      members = Member.all
+      expect { described_class.new(members, source: nil, current_user: user) }.to raise_error(ArgumentError)
+    end
+
+    it 'does not raise an error when initializing using Array or MembersPresenter' do
+      expect { described_class.new([], source: nil, current_user: user) }.not_to raise_error
+      expect { described_class.new(MembersPresenter.new(nil), source: nil, current_user: user) }.not_to raise_error
+    end
+  end
+
   describe '#execute' do
     shared_examples 'assigns is_source_accessible_to_current_user' do
       let(:current_user) { user }
@@ -77,7 +89,7 @@
 
           where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
             :public  | :public  | true
-            :public  | :private | true # Not setting to false as the finder must not return private members with no user
+            :public  | :private | false
           end
 
           include_examples 'sets correct is_source_accessible_to_current_user for invited members'
diff --git a/spec/serializers/member_serializer_spec.rb b/spec/serializers/member_serializer_spec.rb
index c13f4174e177b1..ee198733ef2593 100644
--- a/spec/serializers/member_serializer_spec.rb
+++ b/spec/serializers/member_serializer_spec.rb
@@ -44,7 +44,7 @@
 
       where(:shared_source_visibility, :invited_group_visibility, :can_see_invited_members_source?) do
         :public  | :public  | true
-        :public  | :private | true # Not setting to false as the finder must not return private members with no user
+        :public  | :private | false
       end
 
       include_examples 'exposes source correctly'
-- 
GitLab