Merge branch 'group-links-create-service-prevent-sharing-groups' into 'master'

Check Group Hierarchy Share Settings in GroupLinks CreateService

See merge request gitlab-org/gitlab!63107

app/controllers/groups/group_links_controller.rb

@@ -11,8 +11,8 @@ class Groups::GroupLinksController < Groups::ApplicationController
 
     if shared_with_group
       result = Groups::GroupLinks::CreateService
-                 .new(shared_with_group, current_user, group_link_create_params)
-                 .execute(group)
+                 .new(group, shared_with_group, current_user, group_link_create_params)
+                 .execute
 
       return render_404 if result[:http_status] == 404
 

app/models/group.rb

@@ -442,6 +442,12 @@ class Group < Namespace
     end
   end
 
+  def self_and_descendants_ids
+    strong_memoize(:self_and_descendants_ids) do
+      self_and_descendants.pluck(:id)
+    end
+  end
+
   def direct_members
     GroupMember.active_without_invites_and_requests
                .non_minimal_access

app/services/groups/group_links/create_service.rb

@@ -3,27 +3,51 @@
 module Groups
   module GroupLinks
     class CreateService < Groups::BaseService
-      def execute(shared_group)
-        unless group && shared_group &&
+      def initialize(shared_group, shared_with_group, user, params)
+        @shared_group = shared_group
+        super(shared_with_group, user, params)
+      end
+
+      def execute
+        unless shared_with_group && shared_group &&
                can?(current_user, :admin_group_member, shared_group) &&
-               can?(current_user, :read_group, group)
+               can?(current_user, :read_group, shared_with_group) &&
+               sharing_allowed?
           return error('Not Found', 404)
         end
 
         link = GroupGroupLink.new(
           shared_group: shared_group,
-          shared_with_group: group,
+          shared_with_group: shared_with_group,
           group_access: params[:shared_group_access],
           expires_at: params[:expires_at]
         )
 
         if link.save
-          group.refresh_members_authorized_projects(direct_members_only: true)
+          shared_with_group.refresh_members_authorized_projects(direct_members_only: true)
           success(link: link)
         else
           error(link.errors.full_messages.to_sentence, 409)
         end
       end
+
+      private
+
+      attr_reader :shared_group
+
+      alias_method :shared_with_group, :group
+
+      def sharing_allowed?
+        sharing_outside_hierarchy_allowed? || within_hierarchy?
+      end
+
+      def sharing_outside_hierarchy_allowed?
+        !shared_group.root_ancestor.namespace_settings.prevent_sharing_groups_outside_hierarchy
+      end
+
+      def within_hierarchy?
+        shared_group.root_ancestor.self_and_descendants_ids.include?(shared_with_group.id)
+      end
     end
   end
 end

lib/api/groups.rb

@@ -372,7 +372,7 @@ module API
           expires_at: params[:expires_at]
         }
 
-        result = ::Groups::GroupLinks::CreateService.new(shared_with_group, current_user, group_link_create_params).execute(shared_group)
+        result = ::Groups::GroupLinks::CreateService.new(shared_group, shared_with_group, current_user, group_link_create_params).execute
         shared_group.preload_shared_group_links
 
         if result[:status] == :success

spec/services/groups/group_links/create_service_spec.rb

@@ -11,8 +11,8 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do
   let_it_be(:group) { create(:group, :private, parent: group_parent) }
   let_it_be(:group_child) { create(:group, :private, parent: group) }
 
-  let_it_be(:shared_group_parent) { create(:group, :private) }
-  let_it_be(:shared_group) { create(:group, :private, parent: shared_group_parent) }
+  let_it_be(:shared_group_parent, refind: true) { create(:group, :private) }
+  let_it_be(:shared_group, refind: true) { create(:group, :private, parent: shared_group_parent) }
   let_it_be(:shared_group_child) { create(:group, :private, parent: shared_group) }
 
   let_it_be(:project_parent) { create(:project, group: shared_group_parent) }
@@ -28,7 +28,7 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do
 
   let(:user) { group_user }
 
-  subject { described_class.new(group, user, opts) }
+  subject { described_class.new(shared_group, group, user, opts) }
 
   before do
     group.add_guest(group_user)
@@ -36,11 +36,11 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do
   end
 
   it 'adds group to another group' do
-    expect { subject.execute(shared_group) }.to change { group.shared_group_links.count }.from(0).to(1)
+    expect { subject.execute }.to change { group.shared_group_links.count }.from(0).to(1)
   end
 
   it 'returns false if shared group is blank' do
-    expect { subject.execute(nil) }.not_to change { group.shared_group_links.count }
+    expect { described_class.new(nil, group, user, opts) }.not_to change { group.shared_group_links.count }
   end
 
   context 'user does not have access to group' do
@@ -51,7 +51,7 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do
     end
 
     it 'returns error' do
-      result = subject.execute(shared_group)
+      result = subject.execute
 
       expect(result[:status]).to eq(:error)
       expect(result[:http_status]).to eq(404)
@@ -67,7 +67,7 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do
     end
 
     it 'returns error' do
-      result = subject.execute(shared_group)
+      result = subject.execute
 
       expect(result[:status]).to eq(:error)
       expect(result[:http_status]).to eq(404)
@@ -85,7 +85,7 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do
       it 'is executed only for the direct members of the group' do
         expect(UserProjectAccessChangedService).to receive(:new).with(contain_exactly(group_user.id)).and_call_original
 
-        subject.execute(shared_group)
+        subject.execute
       end
     end
 
@@ -94,7 +94,7 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do
         let(:user) { group_user }
 
         it 'create proper authorizations' do
-          subject.execute(shared_group)
+          subject.execute
 
           expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey
           expect(Ability.allowed?(user, :read_project, project)).to be_truthy
@@ -106,7 +106,7 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do
         let(:user) { parent_group_user }
 
         it 'create proper authorizations' do
-          subject.execute(shared_group)
+          subject.execute
 
           expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey
           expect(Ability.allowed?(user, :read_project, project)).to be_falsey
@@ -118,7 +118,7 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do
         let(:user) { child_group_user }
 
         it 'create proper authorizations' do
-          subject.execute(shared_group)
+          subject.execute
 
           expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey
           expect(Ability.allowed?(user, :read_project, project)).to be_falsey
@@ -127,4 +127,28 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do
       end
     end
   end
+
+  context 'sharing outside the hierarchy is disabled' do
+    before do
+      shared_group_parent.namespace_settings.update!(prevent_sharing_groups_outside_hierarchy: true)
+    end
+
+    it 'prevents sharing with a group outside the hierarchy' do
+      result = subject.execute
+
+      expect(group.reload.shared_group_links.count).to eq(0)
+      expect(result[:status]).to eq(:error)
+      expect(result[:http_status]).to eq(404)
+    end
+
+    it 'allows sharing with a group within the hierarchy' do
+      sibling_group = create(:group, :private, parent: shared_group_parent)
+      sibling_group.add_guest(group_user)
+
+      result = described_class.new(shared_group, sibling_group, user, opts).execute
+
+      expect(sibling_group.reload.shared_group_links.count).to eq(1)
+      expect(result[:status]).to eq(:success)
+    end
+  end
 end