Merge branch 'security-sharing_group_to_update_project_authorization-master' into 'master'

Update ProjectAuthorization when deleting or updating GroupGroupLink

Closes #55

See merge request gitlab-org/security/gitlab!165

app/controllers/groups/group_links_controller.rb

@@ -24,7 +24,7 @@ class Groups::GroupLinksController < Groups::ApplicationController
   end
 
   def update
-    @group_link.update(group_link_params)
+    Groups::GroupLinks::UpdateService.new(@group_link).execute(group_link_params)
   end
 
   def destroy

app/services/groups/group_links/destroy_service.rb

@@ -6,19 +6,17 @@ module Groups
       def execute(one_or_more_links)
         links = Array(one_or_more_links)
 
-        GroupGroupLink.transaction do
-          GroupGroupLink.delete(links)
+        if GroupGroupLink.delete(links)
+          Gitlab::AppLogger.info(
+            "GroupGroupLinks with ids: #{links.map(&:id)} have been deleted.")
 
           groups_to_refresh = links.map(&:shared_with_group)
           groups_to_refresh.uniq.each do |group|
             group.refresh_members_authorized_projects
           end
-
-          Gitlab::AppLogger.info("GroupGroupLinks with ids: #{links.map(&:id)} have been deleted.")
-        rescue => ex
-          Gitlab::AppLogger.error(ex)
-
-          raise
+        else
+          Gitlab::AppLogger.info(
+            "Failed to delete GroupGroupLinks with ids: #{links.map(&:id)}.")
         end
       end
     end

app/services/groups/group_links/update_service.rb

+# frozen_string_literal: true
+
+module Groups
+  module GroupLinks
+    class UpdateService < BaseService
+      def initialize(group_link, user = nil)
+        super(group_link.shared_group, user)
+
+        @group_link = group_link
+      end
+
+      def execute(group_link_params)
+        group_link.update!(group_link_params)
+
+        if requires_authorization_refresh?(group_link_params)
+          group_link.shared_with_group.refresh_members_authorized_projects
+        end
+      end
+
+      private
+
+      attr_accessor :group_link
+
+      def requires_authorization_refresh?(params)
+        params.include?(:group_access)
+      end
+    end
+  end
+end

changelogs/unreleased/199035-sharing_group_to_update_project_authorization.yml

+---
+title: Update ProjectAuthorization when deleting or updating GroupGroupLink
+merge_request:
+author:
+type: security

spec/controllers/groups/group_links_controller_spec.rb

@@ -6,9 +6,13 @@ describe Groups::GroupLinksController do
   let(:shared_with_group) { create(:group, :private) }
   let(:shared_group) { create(:group, :private) }
   let(:user) { create(:user) }
+  let(:group_member) { create(:user) }
+  let!(:project) { create(:project, group: shared_group) }
 
   before do
     sign_in(user)
+
+    shared_with_group.add_developer(group_member)
   end
 
   describe '#create' do
@@ -40,13 +44,9 @@ describe Groups::GroupLinksController do
     end
 
     context 'when user has correct access to both groups' do
-      let(:group_member) { create(:user) }
-
       before do
         shared_with_group.add_developer(user)
         shared_group.add_owner(user)
-
-        shared_with_group.add_developer(group_member)
       end
 
       context 'when default access level is requested' do
@@ -68,6 +68,10 @@ describe Groups::GroupLinksController do
         end
       end
 
+      it 'updates project permissions' do
+        expect { subject }.to change { group_member.can?(:read_project, project) }.from(false).to(true)
+      end
+
       context 'when shared with group id is not present' do
         let(:shared_with_group_id) { nil }
 
@@ -153,6 +157,7 @@ describe Groups::GroupLinksController do
     context 'when user has admin access to the shared group' do
       before do
         shared_group.add_owner(user)
+        shared_with_group.refresh_members_authorized_projects
       end
 
       it 'updates existing link' do
@@ -166,6 +171,10 @@ describe Groups::GroupLinksController do
         expect(link.group_access).to eq(Gitlab::Access::GUEST)
         expect(link.expires_at).to eq(expiry_date)
       end
+
+      it 'updates project permissions' do
+        expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false)
+      end
     end
 
     context 'when user does not have admin access to the shared group' do
@@ -203,11 +212,16 @@ describe Groups::GroupLinksController do
     context 'when user has admin access to the shared group' do
       before do
         shared_group.add_owner(user)
+        shared_with_group.refresh_members_authorized_projects
       end
 
       it 'deletes existing link' do
         expect { subject }.to change(GroupGroupLink, :count).by(-1)
       end
+
+      it 'updates project permissions' do
+        expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false)
+      end
     end
 
     context 'when user does not have admin access to the shared group' do

spec/services/groups/group_links/destroy_service_spec.rb

@@ -40,24 +40,11 @@ describe Groups::GroupLinks::DestroyService, '#execute' do
     end
 
     it 'updates project authorization once per group' do
-      expect(GroupGroupLink).to receive(:delete)
+      expect(GroupGroupLink).to receive(:delete).and_call_original
       expect(group).to receive(:refresh_members_authorized_projects).once
       expect(another_group).to receive(:refresh_members_authorized_projects).once
 
       subject.execute(links)
     end
-
-    it 'rolls back changes when error happens' do
-      group.add_developer(user)
-
-      expect(group).to receive(:refresh_members_authorized_projects).once.and_call_original
-      expect(another_group).to(
-        receive(:refresh_members_authorized_projects).and_raise('boom'))
-
-      expect { subject.execute(links) }.to raise_error('boom')
-
-      expect(GroupGroupLink.count).to eq(links.length)
-      expect(Ability.allowed?(user, :read_project, project)).to be_truthy
-    end
   end
 end

spec/services/groups/group_links/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Groups::GroupLinks::UpdateService, '#execute' do
+  let(:user) { create(:user) }
+
+  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(:group_member) { create(:user) }
+  let!(:link) { create(:group_group_link, shared_group: shared_group, shared_with_group: group) }
+
+  let(:expiry_date) { 1.month.from_now.to_date }
+  let(:group_link_params) do
+    { group_access: Gitlab::Access::GUEST,
+      expires_at: expiry_date }
+  end
+
+  subject { described_class.new(link).execute(group_link_params) }
+
+  before do
+    group.add_developer(group_member)
+  end
+
+  it 'updates existing link' do
+    expect(link.group_access).to eq(Gitlab::Access::DEVELOPER)
+    expect(link.expires_at).to be_nil
+
+    subject
+
+    link.reload
+
+    expect(link.group_access).to eq(Gitlab::Access::GUEST)
+    expect(link.expires_at).to eq(expiry_date)
+  end
+
+  it 'updates project permissions' do
+    expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false)
+  end
+
+  it 'executes UserProjectAccessChangedService' do
+    expect_next_instance_of(UserProjectAccessChangedService) do |service|
+      expect(service).to receive(:execute)
+    end
+
+    subject
+  end
+
+  context 'with only param not requiring authorization refresh' do
+    let(:group_link_params) { { expires_at: Date.tomorrow } }
+
+    it 'does not execute UserProjectAccessChangedService' do
+      expect(UserProjectAccessChangedService).not_to receive(:new)
+
+      subject
+    end
+  end
+end