Merge branch 'if-217637-project_share_to_avoid_after_commit_hook' into 'master'

Project share to avoid after_commit hook

See merge request gitlab-org/gitlab!33144

app/controllers/projects/group_links_controller.rb

@@ -22,8 +22,7 @@ class Projects::GroupLinksController < Projects::ApplicationController
 
   def update
     @group_link = @project.project_group_links.find(params[:id])
-
-    @group_link.update(group_link_params)
+    Projects::GroupLinks::UpdateService.new(@group_link).execute(group_link_params)
   end
 
   def destroy

app/models/project_group_link.rb

@@ -15,8 +15,6 @@ class ProjectGroupLink < ApplicationRecord
 
   scope :non_guests, -> { where('group_access > ?', Gitlab::Access::GUEST) }
 
-  after_commit :refresh_group_members_authorized_projects
-
   alias_method :shared_with_group, :group
 
   def self.access_options
@@ -49,10 +47,6 @@ class ProjectGroupLink < ApplicationRecord
       errors.add(:base, _("Project cannot be shared with the group it is in or one of its ancestors."))
     end
   end
-
-  def refresh_group_members_authorized_projects
-    group.refresh_members_authorized_projects
-  end
 end
 
 ProjectGroupLink.prepend_if_ee('EE::ProjectGroupLink')

app/services/projects/group_links/create_service.rb

@@ -13,6 +13,7 @@ module Projects
         )
 
         if link.save
+          group.refresh_members_authorized_projects
           success(link: link)
         else
           error(link.errors.full_messages.to_sentence, 409)

app/services/projects/group_links/destroy_service.rb

@@ -12,7 +12,9 @@ module Projects
           TodosDestroyer::ConfidentialIssueWorker.perform_in(Todo::WAIT_FOR_DELETE, nil, project.id)
         end
 
-        group_link.destroy
+        group_link.destroy.tap do |link|
+          link.group.refresh_members_authorized_projects
+        end
       end
     end
   end

app/services/projects/group_links/update_service.rb

+# frozen_string_literal: true
+
+module Projects
+  module GroupLinks
+    class UpdateService < BaseService
+      def initialize(group_link, user = nil)
+        super(group_link.project, 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.group.refresh_members_authorized_projects
+        end
+      end
+
+      private
+
+      attr_reader :group_link
+
+      def requires_authorization_refresh?(params)
+        params.include?(:group_access)
+      end
+    end
+  end
+end

app/workers/remove_expired_group_links_worker.rb

@@ -7,7 +7,9 @@ class RemoveExpiredGroupLinksWorker # rubocop:disable Scalability/IdempotentWork
   feature_category :authentication_and_authorization
 
   def perform
-    ProjectGroupLink.expired.destroy_all # rubocop: disable Cop/DestroyAll
+    ProjectGroupLink.expired.find_each do |link|
+      Projects::GroupLinks::DestroyService.new(link.project, nil).execute(link)
+    end
 
     GroupGroupLink.expired.find_in_batches do |link_batch|
       Groups::GroupLinks::DestroyService.new(nil, nil).execute(link_batch)

ee/spec/lib/gitlab/git_access_spec.rb

@@ -477,9 +477,8 @@ describe Gitlab::GitAccess do
     def self.run_group_permission_checks(permissions_matrix)
       permissions_matrix.each_pair do |role, matrix|
         it "has the correct permissions for group #{role}s" do
-          project
-            .project_group_links
-            .create(group: group, group_access: Gitlab::Access.sym_options[role])
+          create(:project_group_link, role, group: group,
+                                            project: project)
 
           protected_branch.save
 

lib/api/projects.rb

@@ -502,7 +502,9 @@ module API
         link = user_project.project_group_links.find_by(group_id: params[:group_id])
         not_found!('Group Link') unless link
 
-        destroy_conditionally!(link)
+        destroy_conditionally!(link) do
+          ::Projects::GroupLinks::DestroyService.new(user_project, current_user).execute(link)
+        end
       end
       # rubocop: enable CodeReuse/ActiveRecord
 

spec/factories/project_group_links.rb

@@ -5,10 +5,15 @@ FactoryBot.define do
     project
     group
     expires_at { nil }
+    group_access { Gitlab::Access::DEVELOPER }
 
     trait(:guest) { group_access { Gitlab::Access::GUEST } }
     trait(:reporter) { group_access { Gitlab::Access::REPORTER } }
     trait(:developer) { group_access { Gitlab::Access::DEVELOPER } }
     trait(:maintainer) { group_access { Gitlab::Access::MAINTAINER } }
+
+    after(:create) do |project_group_link, evaluator|
+      project_group_link.group.refresh_members_authorized_projects
+    end
   end
 end

spec/models/project_group_link_spec.rb

@@ -49,22 +49,6 @@ describe ProjectGroupLink do
     end
   end
 
-  describe "destroying a record", :delete do
-    it "refreshes group users' authorized projects" do
-      project     = create(:project, :private)
-      group       = create(:group)
-      reporter    = create(:user)
-      group_users = group.users
-
-      group.add_reporter(reporter)
-      project.project_group_links.create(group: group)
-      group_users.each { |user| expect(user.authorized_projects).to include(project) }
-
-      project.project_group_links.destroy_all # rubocop: disable Cop/DestroyAll
-      group_users.each { |user| expect(user.authorized_projects).not_to include(project) }
-    end
-  end
-
   describe 'search by group name' do
     let_it_be(:project_group_link) { create(:project_group_link) }
     let_it_be(:group) { project_group_link.group }

spec/models/project_team_spec.rb

@@ -96,11 +96,9 @@ describe ProjectTeam do
 
       it 'returns invited members of a group' do
         group_member = create(:group_member)
-
-        project.project_group_links.create!(
-          group: group_member.group,
-          group_access: Gitlab::Access::GUEST
-        )
+        create(:project_group_link, group: group_member.group,
+                                    project: project,
+                                    group_access: Gitlab::Access::GUEST)
 
         expect(project.team.members)
           .to contain_exactly(group_member.user, project.owner)
@@ -108,11 +106,9 @@ describe ProjectTeam do
 
       it 'returns invited members of a group of a specified level' do
         group_member = create(:group_member)
-
-        project.project_group_links.create!(
-          group: group_member.group,
-          group_access: Gitlab::Access::REPORTER
-        )
+        create(:project_group_link, group: group_member.group,
+                                    project: project,
+                                    group_access: Gitlab::Access::REPORTER)
 
         expect(project.team.guests).to be_empty
         expect(project.team.reporters).to contain_exactly(group_member.user)

spec/models/user_spec.rb

@@ -2845,10 +2845,10 @@ describe User do
     it "includes projects shared with user's group" do
       user    = create(:user)
       project = create(:project, :private)
-      group   = create(:group)
-
+      group   = create(:group) do |group|
         group.add_reporter(user)
-      project.project_group_links.create(group: group)
+      end
+      create(:project_group_link, group: group, project: project)
 
       expect(user.authorized_projects).to include(project)
     end

spec/requests/api/projects_spec.rb

@@ -2055,10 +2055,12 @@ describe API::Projects do
   end
 
   describe "POST /projects/:id/share" do
-    let(:group) { create(:group) }
+    let_it_be(:group) { create(:group, :private) }
+    let_it_be(:group_user) { create(:user) }
 
     before do
       group.add_developer(user)
+      group.add_developer(group_user)
     end
 
     it "shares project with group" do
@@ -2074,6 +2076,14 @@ describe API::Projects do
       expect(json_response['expires_at']).to eq(expires_at.to_s)
     end
 
+    it 'updates project authorization' do
+      expect do
+        post api("/projects/#{project.id}/share", user), params: { group_id: group.id, group_access: Gitlab::Access::DEVELOPER }
+      end.to(
+        change { group_user.can?(:read_project, project) }.from(false).to(true)
+      )
+    end
+
     it "returns a 400 error when group id is not given" do
       post api("/projects/#{project.id}/share", user), params: { group_access: Gitlab::Access::DEVELOPER }
       expect(response).to have_gitlab_http_status(:bad_request)
@@ -2123,9 +2133,12 @@ describe API::Projects do
 
   describe 'DELETE /projects/:id/share/:group_id' do
     context 'for a valid group' do
-      let(:group) { create(:group, :public) }
+      let_it_be(:group) { create(:group, :private) }
+      let_it_be(:group_user) { create(:user) }
 
       before do
+        group.add_developer(group_user)
+
         create(:project_group_link, group: group, project: project)
       end
 
@@ -2136,6 +2149,14 @@ describe API::Projects do
         expect(project.project_group_links).to be_empty
       end
 
+      it 'updates project authorization' do
+        expect do
+          delete api("/projects/#{project.id}/share/#{group.id}", user)
+        end.to(
+          change { group_user.can?(:read_project, project) }.from(true).to(false)
+        )
+      end
+
       it_behaves_like '412 response' do
         let(:request) { api("/projects/#{project.id}/share/#{group.id}", user) }
       end

spec/services/projects/group_links/create_service_spec.rb

@@ -3,16 +3,17 @@
 require 'spec_helper'
 
 describe Projects::GroupLinks::CreateService, '#execute' do
-  let(:user) { create :user }
-  let(:group) { create :group }
-  let(:project) { create :project }
+  let_it_be(:user) { create :user }
+  let_it_be(:group) { create :group }
+  let_it_be(:project) { create :project }
   let(:opts) do
     {
       link_group_access: '30',
       expires_at: nil
     }
   end
-  let(:subject) { described_class.new(project, user, opts) }
+
+  subject { described_class.new(project, user, opts) }
 
   before do
     group.add_developer(user)
@@ -22,6 +23,12 @@ describe Projects::GroupLinks::CreateService, '#execute' do
     expect { subject.execute(group) }.to change { project.project_group_links.count }.from(0).to(1)
   end
 
+  it 'updates authorization' do
+    expect { subject.execute(group) }.to(
+      change { Ability.allowed?(user, :read_project, project) }
+        .from(false).to(true))
+  end
+
   it 'returns false if group is blank' do
     expect { subject.execute(nil) }.not_to change { project.project_group_links.count }
   end

spec/services/projects/group_links/destroy_service_spec.rb

@@ -3,15 +3,25 @@
 require 'spec_helper'
 
 describe Projects::GroupLinks::DestroyService, '#execute' do
-  let(:project) { create(:project, :private) }
-  let!(:group_link) { create(:project_group_link, project: project) }
-  let(:user) { create :user }
-  let(:subject) { described_class.new(project, user) }
+  let_it_be(:user) { create :user }
+  let_it_be(:project) { create(:project, :private) }
+  let_it_be(:group) { create(:group) }
+  let!(:group_link) { create(:project_group_link, project: project, group: group) }
+
+  subject { described_class.new(project, user) }
 
   it 'removes group from project' do
     expect { subject.execute(group_link) }.to change { project.project_group_links.count }.from(1).to(0)
   end
 
+  it 'updates authorization' do
+    group.add_maintainer(user)
+
+    expect { subject.execute(group_link) }.to(
+      change { Ability.allowed?(user, :read_project, project) }
+        .from(true).to(false))
+  end
+
   it 'returns false if group_link is blank' do
     expect { subject.execute(nil) }.not_to change { project.project_group_links.count }
   end

spec/services/projects/group_links/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::GroupLinks::UpdateService, '#execute' do
+  let_it_be(:user) { create :user }
+  let_it_be(:group) { create :group }
+  let_it_be(:project) { create :project }
+  let!(:link) { create(:project_group_link, project: project, 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(user)
+  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 { user.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

spec/workers/remove_expired_group_links_worker_spec.rb

@@ -23,30 +23,53 @@ describe RemoveExpiredGroupLinksWorker do
         subject.perform
         expect(non_expiring_project_group_link.reload).to be_present
       end
-    end
 
-    context 'GroupGroupLinks' do
-      let(:mock_destroy_service) { instance_double(Groups::GroupLinks::DestroyService) }
+      it 'removes project authorization' do
+        user = create(:user)
 
-      before do
-        allow(Groups::GroupLinks::DestroyService).to(
-          receive(:new).and_return(mock_destroy_service))
+        project = expired_project_group_link.project
+        group = expired_project_group_link.group
+
+        group.add_maintainer(user)
+
+        expect { subject.perform }.to(
+          change { user.can?(:read_project, project) }.from(true).to(false))
+      end
     end
 
+    context 'GroupGroupLinks' do
       context 'expired GroupGroupLink exists' do
-        before do
-          create(:group_group_link, expires_at: 1.hour.ago)
-        end
+        let!(:group_group_link) { create(:group_group_link, expires_at: 1.hour.ago) }
 
         it 'calls Groups::GroupLinks::DestroyService' do
+          mock_destroy_service = instance_double(Groups::GroupLinks::DestroyService)
+          allow(Groups::GroupLinks::DestroyService).to(
+            receive(:new).and_return(mock_destroy_service))
+
           expect(mock_destroy_service).to receive(:execute).once
 
           subject.perform
         end
+
+        it 'removes project authorization' do
+          shared_group = group_group_link.shared_group
+          shared_with_group = group_group_link.shared_with_group
+          project = create(:project, group: shared_group)
+
+          user = create(:user)
+          shared_with_group.add_maintainer(user)
+
+          expect { subject.perform }.to(
+            change { user.can?(:read_project, project) }.from(true).to(false))
+        end
       end
 
       context 'expired GroupGroupLink does not exist' do
         it 'does not call Groups::GroupLinks::DestroyService' do
+          mock_destroy_service = instance_double(Groups::GroupLinks::DestroyService)
+          allow(Groups::GroupLinks::DestroyService).to(
+            receive(:new).and_return(mock_destroy_service))
+
           expect(mock_destroy_service).not_to receive(:execute)
 
           subject.perform