Merge branch '205424-add-api-for-share-groups-with-groups' into 'master'

Resolve "Add API for "Share groups with groups""

See merge request gitlab-org/gitlab!32008

app/controllers/groups/group_links_controller.rb

@@ -27,7 +27,7 @@ class Groups::GroupLinksController < Groups::ApplicationController
   end
 
   def destroy
-    Groups::GroupLinks::DestroyService.new(nil, nil).execute(@group_link)
+    Groups::GroupLinks::DestroyService.new(group, current_user).execute(@group_link)
 
     respond_to do |format|
       format.html do

app/models/group.rb

@@ -496,6 +496,11 @@ class Group < Namespace
     # TODO: group hooks https://gitlab.com/gitlab-org/gitlab/-/issues/216904
   end
 
+  def preload_shared_group_links
+    preloader = ActiveRecord::Associations::Preloader.new
+    preloader.preload(self, shared_with_group_links: [shared_with_group: :route])
+  end
+
   private
 
   def update_two_factor_requirement

app/models/group_group_link.rb

@@ -14,6 +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
 
   def self.access_options
     Gitlab::Access.options_with_owner

app/services/groups/group_links/create_service.rb

@@ -5,7 +5,7 @@ module Groups
     class CreateService < BaseService
       def execute(shared_group)
         unless group && shared_group &&
-               can?(current_user, :admin_group, shared_group) &&
+               can?(current_user, :admin_group_member, shared_group) &&
                can?(current_user, :read_group, group)
           return error('Not Found', 404)
         end

app/services/groups/group_links/destroy_service.rb

@@ -3,7 +3,11 @@
 module Groups
   module GroupLinks
     class DestroyService < BaseService
-      def execute(one_or_more_links)
+      def execute(one_or_more_links, skip_authorization: false)
+        unless skip_authorization || group && can?(current_user, :admin_group_member, group)
+          return error('Not Found', 404)
+        end
+
         links = Array(one_or_more_links)
 
         if GroupGroupLink.delete(links)

app/workers/remove_expired_group_links_worker.rb

@@ -12,7 +12,7 @@ class RemoveExpiredGroupLinksWorker # rubocop:disable Scalability/IdempotentWork
     end
 
     GroupGroupLink.expired.find_in_batches do |link_batch|
-      Groups::GroupLinks::DestroyService.new(nil, nil).execute(link_batch)
+      Groups::GroupLinks::DestroyService.new(nil, nil).execute(link_batch, skip_authorization: true)
     end
   end
 end

changelogs/unreleased/205424-add-api-for-share-groups-with-groups.yml

+---
+title: Add API support for sharing groups with groups
+merge_request: 32008
+author:
+type: added

doc/api/groups.md

@@ -428,6 +428,15 @@ Example response:
   "file_template_project_id": 1,
   "parent_id": null,
   "created_at": "2020-01-15T12:36:29.590Z",
+  "shared_with_groups": [
+    {
+      "group_id": 28,
+      "group_name": "H5bp",
+      "group_full_path": "h5bp",
+      "group_access_level": 20,
+      "expires_at": null
+    }
+  ],
   "projects": [ // Deprecated and will be removed in API v5
     {
       "id": 7,
@@ -1101,3 +1110,35 @@ Read more in the [Group Badges](group_badges.md) documentation.
 ## Group Import/Export
 
 Read more in the [Group Import/Export](group_import_export.md) documentation.
+
+## Share Groups with Groups
+
+These endpoints create and delete links for sharing a group with another group. For more information, see the related discussion in the [GitLab Groups](../user/group/index.md#sharing-a-group-with-another-group) page.
+
+### Create a link to share a group with another group
+
+Share group with another group. Returns `200` and the [group details](#details-of-a-group) on success.
+
+```plaintext
+POST /groups/:id/share
+```
+
+| Attribute | Type           | Required | Description |
+| --------- | -------------- | -------- | ----------- |
+| `id`      | integer/string | yes      | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
+| `group_id` | integer | yes | The ID of the group to share with |
+| `group_access` | integer | yes | The [permissions level](members.md) to grant the group |
+| `expires_at` | string | no | Share expiration date in ISO 8601 format: 2016-09-26 |
+
+### Delete link sharing group with another group
+
+Unshare the group from another group. Returns `204` and no content on success.
+
+```plaintext
+DELETE /groups/:id/share/:group_id
+```
+
+| Attribute | Type           | Required | Description |
+| --------- | -------------- | -------- | ----------- |
+| `id`      | integer/string | yes      | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
+| `group_id` | integer | yes | The ID of the group to share with |

ee/lib/ee/api/groups.rb

@@ -134,6 +134,7 @@ module EE
             break not_found! unless user_group.feature_available?(:adjourned_deletion_for_projects_and_groups)
 
             result = ::Groups::RestoreService.new(user_group, current_user).execute
+            user_group.preload_shared_group_links
 
             if result[:status] == :success
               present user_group, with: ::API::Entities::GroupDetail, current_user: current_user

lib/api/entities/group_detail.rb

@@ -3,6 +3,9 @@
 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]))
+      end
       expose :runners_token, if: lambda { |group, options| options[:user_can_admin_group] }
       expose :projects, using: Entities::Project do |group, options|
         projects = GroupProjectsFinder.new(

lib/api/entities/project.rb

@@ -90,7 +90,7 @@ module API
       expose :build_coverage_regex
       expose :ci_config_path, if: -> (project, options) { Ability.allowed?(options[:current_user], :download_code, project) }
       expose :shared_with_groups do |project, options|
-        SharedGroup.represent(project.project_group_links, options)
+        SharedGroupWithProject.represent(project.project_group_links, options)
       end
       expose :only_allow_merge_if_pipeline_succeeds
       expose :request_access_enabled

lib/api/entities/shared_group_with_group.rb

+# frozen_string_literal: true
+
+module API
+  module Entities
+    class SharedGroupWithGroup < Grape::Entity
+      expose :shared_with_group_id, as: :group_id
+      expose :group_name do |group_link|
+        group_link.shared_with_group.name
+      end
+      expose :group_full_path do |group_link|
+        group_link.shared_with_group.full_path
+      end
+      expose :group_access, as: :group_access_level
+      expose :expires_at
+    end
+  end
+end

lib/api/entities/shared_group.rb → lib/api/entities/shared_group_with_project.rb

@@ -2,7 +2,7 @@
 
 module API
   module Entities
-    class SharedGroup < Grape::Entity
+    class SharedGroupWithProject < Grape::Entity
       expose :group_id
       expose :group_name do |group_link, options|
         group_link.group.name

lib/api/groups.rb

@@ -151,6 +151,7 @@ module API
         end
 
         group = create_group
+        group.preload_shared_group_links
 
         if group.persisted?
           present group, with: Entities::GroupDetail, current_user: current_user
@@ -175,6 +176,8 @@ module API
       end
       put ':id' do
         group = find_group!(params[:id])
+        group.preload_shared_group_links
+
         authorize! :admin_group, group
 
         if update_group(group)
@@ -193,6 +196,7 @@ module API
       end
       get ":id" do
         group = find_group!(params[:id])
+        group.preload_shared_group_links
 
         options = {
           with: params[:with_projects] ? Entities::GroupDetail : Entities::Group,
@@ -299,6 +303,7 @@ module API
       post ":id/projects/:project_id", requirements: { project_id: /.+/ } do
         authenticated_as_admin!
         group = find_group!(params[:id])
+        group.preload_shared_group_links
         project = find_project!(params[:project_id])
         result = ::Projects::TransferService.new(project, current_user).execute(group)
 
@@ -308,6 +313,49 @@ module API
           render_api_error!("Failed to transfer project #{project.errors.messages}", 400)
         end
       end
+
+      desc 'Share a group with a group' do
+        success Entities::GroupDetail
+      end
+      params do
+        requires :group_id, type: Integer, desc: 'The ID of the group to share'
+        requires :group_access, type: Integer, values: Gitlab::Access.all_values, desc: 'The group access level'
+        optional :expires_at, type: Date, desc: 'Share expiration date'
+      end
+      post ":id/share" do
+        shared_group = find_group!(params[:id])
+        shared_with_group = find_group!(params[:group_id])
+
+        group_link_create_params = {
+          shared_group_access: params[:group_access],
+          expires_at: params[:expires_at]
+        }
+
+        result = ::Groups::GroupLinks::CreateService.new(shared_with_group, current_user, group_link_create_params).execute(shared_group)
+        shared_group.preload_shared_group_links
+
+        if result[:status] == :success
+          present shared_group, with: Entities::GroupDetail, current_user: current_user
+        else
+          render_api_error!(result[:message], result[:http_status])
+        end
+      end
+
+      params do
+        requires :group_id, type: Integer, desc: 'The ID of the shared group'
+      end
+      # rubocop: disable CodeReuse/ActiveRecord
+      delete ":id/share/:group_id" do
+        shared_group = find_group!(params[:id])
+
+        link = shared_group.shared_with_group_links.find_by(shared_with_group_id: params[:group_id])
+        not_found!('Group Link') unless link
+
+        ::Groups::GroupLinks::DestroyService.new(shared_group, current_user).execute(link)
+
+        no_content!
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
     end
   end
 end

spec/models/group_group_link_spec.rb

@@ -29,6 +29,32 @@ 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/requests/api/groups_spec.rb

@@ -436,6 +436,8 @@ describe API::Groups do
       it "returns one of user1's groups" do
         project = create(:project, namespace: group2, path: 'Foo')
         create(:project_group_link, project: project, group: group1)
+        group = create(:group)
+        link = create(:group_group_link, shared_group: group1, shared_with_group: group)
 
         get api("/groups/#{group1.id}", user1)
 
@@ -460,6 +462,13 @@ describe API::Groups do
         expect(json_response['full_path']).to eq(group1.full_path)
         expect(json_response['parent_id']).to eq(group1.parent_id)
         expect(json_response['created_at']).to be_present
+        expect(json_response['shared_with_groups']).to be_an Array
+        expect(json_response['shared_with_groups'].length).to eq(1)
+        expect(json_response['shared_with_groups'][0]['group_id']).to eq(group.id)
+        expect(json_response['shared_with_groups'][0]['group_name']).to eq(group.name)
+        expect(json_response['shared_with_groups'][0]['group_full_path']).to eq(group.full_path)
+        expect(json_response['shared_with_groups'][0]['group_access_level']).to eq(link.group_access)
+        expect(json_response['shared_with_groups'][0]).to have_key('expires_at')
         expect(json_response['projects']).to be_an Array
         expect(json_response['projects'].length).to eq(2)
         expect(json_response['shared_projects']).to be_an Array
@@ -526,7 +535,7 @@ describe API::Groups do
           .to contain_exactly(projects[:public].id, projects[:internal].id)
       end
 
-      it 'avoids N+1 queries' do
+      it 'avoids N+1 queries with project links' do
         get api("/groups/#{group1.id}", admin)
 
         control_count = ActiveRecord::QueryRecorder.new do
@@ -539,6 +548,24 @@ describe API::Groups do
           get api("/groups/#{group1.id}", admin)
         end.not_to exceed_query_limit(control_count)
       end
+
+      it 'avoids N+1 queries with shared group links' do
+        # setup at least 1 shared group, so that we record the queries that preload the nested associations too.
+        create(:group_group_link, shared_group: group1, shared_with_group: create(:group))
+
+        control_count = ActiveRecord::QueryRecorder.new do
+          get api("/groups/#{group1.id}", admin)
+        end.count
+
+        # setup "n" more shared groups
+        create(:group_group_link, shared_group: group1, shared_with_group: create(:group))
+        create(:group_group_link, shared_group: group1, shared_with_group: create(:group))
+
+        # test that no of queries for 1 shared group is same as for n shared groups
+        expect do
+          get api("/groups/#{group1.id}", admin)
+        end.not_to exceed_query_limit(control_count)
+      end
     end
 
     context "when authenticated as admin" do
@@ -1528,4 +1555,173 @@ describe API::Groups do
       group2.add_owner(user1)
     end
   end
+
+  describe "POST /groups/:id/share" do
+    shared_examples 'shares group with group' do
+      it "shares group with group" do
+        expires_at = 10.days.from_now.to_date
+
+        expect do
+          post api("/groups/#{group.id}/share", user), params: { group_id: shared_with_group.id, group_access: Gitlab::Access::DEVELOPER, expires_at: expires_at }
+        end.to change { group.shared_with_group_links.count }.by(1)
+
+        expect(response).to have_gitlab_http_status(:created)
+        expect(json_response['shared_with_groups']).to be_an Array
+        expect(json_response['shared_with_groups'].length).to eq(1)
+        expect(json_response['shared_with_groups'][0]['group_id']).to eq(shared_with_group.id)
+        expect(json_response['shared_with_groups'][0]['group_name']).to eq(shared_with_group.name)
+        expect(json_response['shared_with_groups'][0]['group_full_path']).to eq(shared_with_group.full_path)
+        expect(json_response['shared_with_groups'][0]['group_access_level']).to eq(Gitlab::Access::DEVELOPER)
+        expect(json_response['shared_with_groups'][0]['expires_at']).to eq(expires_at.to_s)
+      end
+
+      it "returns a 400 error when group id is not given" do
+        post api("/groups/#{group.id}/share", user), params: { group_access: Gitlab::Access::DEVELOPER }
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
+
+      it "returns a 400 error when access level is not given" do
+        post api("/groups/#{group.id}/share", user), params: { group_id: shared_with_group.id }
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
+
+      it 'returns a 404 error when group does not exist' do
+        post api("/groups/#{group.id}/share", user), params: { group_id: non_existing_record_id, group_access: Gitlab::Access::DEVELOPER }
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+
+      it "returns a 400 error when wrong params passed" do
+        post api("/groups/#{group.id}/share", user), params: { group_id: shared_with_group.id, group_access: non_existing_record_access_level }
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['error']).to eq 'group_access does not have a valid value'
+      end
+
+      it "returns a 409 error when link is not saved" do
+        allow(::Groups::GroupLinks::CreateService).to receive_message_chain(:new, :execute)
+          .and_return({ status: :error, http_status: 409, message: 'error' })
+
+        post api("/groups/#{group.id}/share", user), params: { group_id: shared_with_group.id, group_access: Gitlab::Access::DEVELOPER }
+
+        expect(response).to have_gitlab_http_status(:conflict)
+      end
+    end
+
+    context 'when authenticated as owner' do
+      let(:owner_group) { create(:group) }
+      let(:owner_user) { create(:user) }
+
+      before do
+        owner_group.add_owner(owner_user)
+      end
+
+      it_behaves_like 'shares group with group' do
+        let(:user) { owner_user }
+        let(:group) { owner_group }
+        let(:shared_with_group) { create(:group) }
+      end
+    end
+
+    context 'when the user is not the owner of the group' do
+      let(:group) { create(:group) }
+      let(:user4) { create(:user) }
+      let(:expires_at) { 10.days.from_now.to_date }
+
+      before do
+        group1.add_maintainer(user4)
+      end
+
+      it 'does not create group share' do
+        post api("/groups/#{group1.id}/share", user4), params: { group_id: group.id, group_access: Gitlab::Access::DEVELOPER, expires_at: expires_at }
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'when authenticated as admin' do
+      it_behaves_like 'shares group with group' do
+        let(:user) { admin }
+        let(:group) { create(:group) }
+        let(:shared_with_group) { create(:group) }
+      end
+    end
+  end
+
+  describe 'DELETE /groups/:id/share/:group_id' do
+    shared_examples 'deletes group share' do
+      it 'deletes a group share' do
+        expect do
+          delete api("/groups/#{shared_group.id}/share/#{shared_with_group.id}", user)
+
+          expect(response).to have_gitlab_http_status(:no_content)
+          expect(shared_group.shared_with_group_links).to be_empty
+        end.to change { shared_group.shared_with_group_links.count }.by(-1)
+      end
+
+      it 'requires the group id to be an integer' do
+        delete api("/groups/#{shared_group.id}/share/foo", user)
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
+
+      it 'returns a 404 error when group link does not exist' do
+        delete api("/groups/#{shared_group.id}/share/#{non_existing_record_id}", user)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+
+      it 'returns a 404 error when group does not exist' do
+        delete api("/groups/123/share/#{non_existing_record_id}", user)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'when authenticated as owner' do
+      let(:group_a) { create(:group) }
+
+      before do
+        create(:group_group_link, shared_group: group1, shared_with_group: group_a)
+      end
+
+      it_behaves_like 'deletes group share' do
+        let(:user) { user1 }
+        let(:shared_group) { group1 }
+        let(:shared_with_group) { group_a }
+      end
+    end
+
+    context 'when the user is not the owner of the group' do
+      let(:group_a) { create(:group) }
+      let(:user4) { create(:user) }
+
+      before do
+        group1.add_maintainer(user4)
+        create(:group_group_link, shared_group: group1, shared_with_group: group_a)
+      end
+
+      it 'does not remove group share' do
+        expect do
+          delete api("/groups/#{group1.id}/share/#{group_a.id}", user4)
+
+          expect(response).to have_gitlab_http_status(:no_content)
+        end.not_to change { group1.shared_with_group_links }
+      end
+    end
+
+    context 'when authenticated as admin' do
+      let(:group_b) { create(:group) }
+
+      before do
+        create(:group_group_link, shared_group: group2, shared_with_group: group_b)
+      end
+
+      it_behaves_like 'deletes group share' do
+        let(:user) { admin }
+        let(:shared_group) { group2 }
+        let(:shared_with_group) { group_b }
+      end
+    end
+  end
 end

spec/services/groups/group_links/destroy_service_spec.rb

@@ -8,14 +8,20 @@ describe Groups::GroupLinks::DestroyService, '#execute' do
   let_it_be(:group) { create(:group, :private) }
   let_it_be(:shared_group) { create(:group, :private) }
   let_it_be(:project) { create(:project, group: shared_group) }
+  let_it_be(:owner) { create(:user) }
 
-  subject { described_class.new(nil, nil) }
+  before do
+    group.add_developer(owner)
+    shared_group.add_owner(owner)
+  end
+
+  subject { described_class.new(shared_group, owner) }
 
   context 'single link' do
     let!(:link) { create(:group_group_link, shared_group: shared_group, shared_with_group: group) }
 
     it 'destroys link' do
-      expect { subject.execute(link) }.to change { GroupGroupLink.count }.from(1).to(0)
+      expect { subject.execute(link) }.to change { shared_group.shared_with_group_links.count }.from(1).to(0)
     end
 
     it 'revokes project authorization' do