Merge branch 'fix-authorized_groups-with-sub-groups' into 'master'

Groups API: fix shared_with_groups to include all authorized groups

See merge request gitlab-org/gitlab!76556

app/models/group.rb

@@ -780,6 +780,10 @@ class Group < Namespace
     crm_settings&.enabled?
   end
 
+  def shared_with_group_links_visible_to_user(user)
+    shared_with_group_links.preload_shared_with_groups.filter { |link| Ability.allowed?(user, :read_group, link.shared_with_group) }
+  end
+
   private
 
   def max_member_access(user_ids)

app/models/group_group_link.rb

@@ -14,7 +14,7 @@ class GroupGroupLink < ApplicationRecord
                            presence: true
 
   scope :non_guests, -> { where('group_access > ?', Gitlab::Access::GUEST) }
-  scope :public_or_visible_to_user, ->(group, user) { where(shared_group: group, shared_with_group: Group.public_or_visible_to_user(user)) } # rubocop:disable Cop/GroupPublicOrVisibleToUser
+  scope :preload_shared_with_groups, -> { preload(:shared_with_group) }
 
   def self.access_options
     Gitlab::Access.options_with_owner

lib/api/entities/group_detail.rb

@@ -4,7 +4,7 @@ module API
   module Entities
     class GroupDetail < Group
       expose :shared_with_groups do |group, options|
-        SharedGroupWithGroup.represent(group.shared_with_group_links.public_or_visible_to_user(group, options[:current_user]))
+        SharedGroupWithGroup.represent(group.shared_with_group_links_visible_to_user(options[:current_user]))
       end
       expose :runners_token, if: lambda { |group, options| options[:user_can_admin_group] }
       expose :prevent_sharing_groups_outside_hierarchy, if: ->(group) { group.root? }

spec/models/group_group_link_spec.rb

@@ -29,32 +29,6 @@ RSpec.describe GroupGroupLink do
                                                           ])
       end
     end
-
-    describe '.public_or_visible_to_user' do
-      let!(:user_with_access) { create :user }
-      let!(:user_without_access) { create :user }
-      let!(:shared_with_group) { create :group, :private }
-      let!(:shared_group) { create :group }
-      let!(:private_group_group_link) { create(:group_group_link, shared_group: shared_group, shared_with_group: shared_with_group) }
-
-      before do
-        shared_group.add_owner(user_with_access)
-        shared_group.add_owner(user_without_access)
-        shared_with_group.add_developer(user_with_access)
-      end
-
-      context 'when user can access shared group' do
-        it 'returns the private group' do
-          expect(described_class.public_or_visible_to_user(shared_group, user_with_access)).to include(private_group_group_link)
-        end
-      end
-
-      context 'when user does not have access to shared group' do
-        it 'does not return private group' do
-          expect(described_class.public_or_visible_to_user(shared_group, user_without_access)).not_to include(private_group_group_link)
-        end
-      end
-    end
   end
 
   describe 'validation' do

spec/models/group_spec.rb

@@ -2859,4 +2859,55 @@ RSpec.describe Group do
       expect(described_class.get_ids_by_ids_or_paths([new_group_id], [group_path])).to match_array([group_id, new_group_id])
     end
   end
+
+  describe '#shared_with_group_links_visible_to_user' do
+    let_it_be(:admin) { create :admin }
+    let_it_be(:normal_user) { create :user }
+    let_it_be(:user_with_access) { create :user }
+    let_it_be(:user_with_parent_access) { create :user }
+    let_it_be(:user_without_access) { create :user }
+    let_it_be(:shared_group) { create :group }
+    let_it_be(:parent_group) { create :group, :private }
+    let_it_be(:shared_with_private_group) { create :group, :private, parent: parent_group }
+    let_it_be(:shared_with_internal_group) { create :group, :internal }
+    let_it_be(:shared_with_public_group) { create :group, :public }
+    let_it_be(:private_group_group_link) { create(:group_group_link, shared_group: shared_group, shared_with_group: shared_with_private_group) }
+    let_it_be(:internal_group_group_link) { create(:group_group_link, shared_group: shared_group, shared_with_group: shared_with_internal_group) }
+    let_it_be(:public_group_group_link) { create(:group_group_link, shared_group: shared_group, shared_with_group: shared_with_public_group) }
+
+    before do
+      shared_with_private_group.add_developer(user_with_access)
+      parent_group.add_developer(user_with_parent_access)
+    end
+
+    context 'when user is admin', :enable_admin_mode do
+      it 'returns all existing shared group links' do
+        expect(shared_group.shared_with_group_links_visible_to_user(admin)).to contain_exactly(private_group_group_link, internal_group_group_link, public_group_group_link)
+      end
+    end
+
+    context 'when user is nil' do
+      it 'returns only link of public shared group' do
+        expect(shared_group.shared_with_group_links_visible_to_user(nil)).to contain_exactly(public_group_group_link)
+      end
+    end
+
+    context 'when user has no access to private shared group' do
+      it 'returns links of internal and public shared groups' do
+        expect(shared_group.shared_with_group_links_visible_to_user(normal_user)).to contain_exactly(internal_group_group_link, public_group_group_link)
+      end
+    end
+
+    context 'when user is member of private shared group' do
+      it 'returns links of private, internal and public shared groups' do
+        expect(shared_group.shared_with_group_links_visible_to_user(user_with_access)).to contain_exactly(private_group_group_link, internal_group_group_link, public_group_group_link)
+      end
+    end
+
+    context 'when user is inherited member of private shared group' do
+      it 'returns links of private, internal and public shared groups' do
+        expect(shared_group.shared_with_group_links_visible_to_user(user_with_parent_access)).to contain_exactly(private_group_group_link, internal_group_group_link, public_group_group_link)
+      end
+    end
+  end
 end

spec/requests/api/groups_spec.rb

@@ -801,6 +801,54 @@ RSpec.describe API::Groups do
         expect(json_response['shared_projects'].count).to eq(limit)
       end
     end
+
+    context 'when a group is shared', :aggregate_failures do
+      let_it_be(:shared_group) { create(:group) }
+      let_it_be(:group2_sub) { create(:group, :private, parent: group2) }
+      let_it_be(:group_link_1) { create(:group_group_link, shared_group: shared_group, shared_with_group: group1) }
+      let_it_be(:group_link_2) { create(:group_group_link, shared_group: shared_group, shared_with_group: group2_sub) }
+
+      subject(:shared_with_groups) { json_response['shared_with_groups'].map { _1['group_id']} }
+
+      context 'when authenticated as admin' do
+        it 'returns all groups that share the group' do
+          get api("/groups/#{shared_group.id}", admin)
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(shared_with_groups).to contain_exactly(group_link_1.shared_with_group_id, group_link_2.shared_with_group_id)
+        end
+      end
+
+      context 'when unauthenticated' do
+        it 'returns only public groups that share the group' do
+          get api("/groups/#{shared_group.id}")
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(shared_with_groups).to contain_exactly(group_link_1.shared_with_group_id)
+        end
+      end
+
+      context 'when authenticated as a member of a parent group that has shared the group' do
+        it 'returns private group if direct member' do
+          group2_sub.add_guest(user3)
+
+          get api("/groups/#{shared_group.id}", user3)
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(shared_with_groups).to contain_exactly(group_link_1.shared_with_group_id, group_link_2.shared_with_group_id)
+        end
+
+        it 'returns private group if inherited member' do
+          inherited_guest_member = create(:user)
+          group2.add_guest(inherited_guest_member)
+
+          get api("/groups/#{shared_group.id}", inherited_guest_member)
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(shared_with_groups).to contain_exactly(group_link_1.shared_with_group_id, group_link_2.shared_with_group_id)
+        end
+      end
+    end
   end
 
   describe 'PUT /groups/:id' do