Worker updating project authoirzations during project share

Specialized worker can bulk insert only the necessary records, instead
of the current approach which recalculates every project authorizations
for members.

app/services/authorized_project_update/project_create_service.rb

@@ -21,7 +21,7 @@ module AuthorizedProjectUpdate
           { user_id: member.user_id, project_id: project.id, access_level: member.access_level }
         end
 
-        ProjectAuthorization.insert_all(attributes)
+        ProjectAuthorization.insert_all(attributes) unless attributes.empty?
       end
 
       ServiceResponse.success

app/services/authorized_project_update/project_group_link_create_service.rb

+# frozen_string_literal: true
+
+module AuthorizedProjectUpdate
+  class ProjectGroupLinkCreateService < BaseService
+    include Gitlab::Utils::StrongMemoize
+
+    BATCH_SIZE = 1000
+
+    def initialize(project, group)
+      @project = project
+      @group = group
+    end
+
+    def execute
+      group.members_from_self_and_ancestors_with_effective_access_level
+           .each_batch(of: BATCH_SIZE, column: :user_id) do |members|
+        existing_authorizations = existing_project_authorizations(members)
+        authorizations_to_create = []
+        user_ids_to_delete = []
+
+        members.each do |member|
+          existing_access_level = existing_authorizations[member.user_id]
+
+          if existing_access_level
+            # User might already have access to the project unrelated to the
+            # current project share
+            next if existing_access_level >= member.access_level
+
+            user_ids_to_delete << member.user_id
+          end
+
+          authorizations_to_create << { user_id: member.user_id,
+                                        project_id: project.id,
+                                        access_level: member.access_level }
+        end
+
+        update_authorizations(user_ids_to_delete, authorizations_to_create)
+      end
+
+      ServiceResponse.success
+    end
+
+    private
+
+    attr_reader :project, :group
+
+    def existing_project_authorizations(members)
+      user_ids = members.map(&:user_id)
+
+      ProjectAuthorization.where(project_id: project.id, user_id: user_ids) # rubocop: disable CodeReuse/ActiveRecord
+                          .select(:user_id, :access_level)
+                          .each_with_object({}) do |authorization, hash|
+        hash[authorization.user_id] = authorization.access_level
+      end
+    end
+
+    def update_authorizations(user_ids_to_delete, authorizations_to_create)
+      ProjectAuthorization.transaction do
+        if user_ids_to_delete.any?
+          ProjectAuthorization.where(project_id: project.id, user_id: user_ids_to_delete) # rubocop: disable CodeReuse/ActiveRecord
+                              .delete_all
+        end
+
+        if authorizations_to_create.any?
+          ProjectAuthorization.insert_all(authorizations_to_create)
+        end
+      end
+    end
+  end
+end

app/workers/all_queues.yml

@@ -11,6 +11,14 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: authorized_project_update:authorized_project_update_project_group_link_create
+  :feature_category: :authentication_and_authorization
+  :has_external_dependencies: 
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: authorized_project_update:authorized_project_update_user_refresh_with_low_urgency
   :feature_category: :authentication_and_authorization
   :has_external_dependencies: 

app/workers/authorized_project_update/project_group_link_create_worker.rb

+# frozen_string_literal: true
+
+module AuthorizedProjectUpdate
+  class ProjectGroupLinkCreateWorker
+    include ApplicationWorker
+
+    feature_category :authentication_and_authorization
+    urgency :low
+    queue_namespace :authorized_project_update
+
+    idempotent!
+
+    def perform(project_id, group_id)
+      project = Project.find(project_id)
+      group = Group.find(group_id)
+
+      AuthorizedProjectUpdate::ProjectGroupLinkCreateService.new(project, group)
+                                                            .execute
+    end
+  end
+end

spec/services/authorized_project_update/project_group_link_create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe AuthorizedProjectUpdate::ProjectGroupLinkCreateService do
+  let_it_be(:group_parent) { create(:group, :private) }
+  let_it_be(:group) { create(:group, :private, parent: group_parent) }
+  let_it_be(:group_child) { create(:group, :private, parent: group) }
+
+  let_it_be(:parent_group_user) { create(:user) }
+  let_it_be(:group_user) { create(:user) }
+
+  let_it_be(:project) { create(:project, :private, group: create(:group, :private)) }
+
+  let(:access_level) { Gitlab::Access::MAINTAINER }
+
+  subject(:service) { described_class.new(project, group) }
+
+  describe '#perform' do
+    context 'direct group members' do
+      before do
+        create(:group_member, access_level: access_level, group: group, user: group_user)
+        ProjectAuthorization.delete_all
+      end
+
+      it 'creates project authorization' do
+        expect { service.execute }.to(
+          change { ProjectAuthorization.count }.from(0).to(1))
+
+        project_authorization = ProjectAuthorization.where(
+          project_id: project.id,
+          user_id: group_user.id,
+          access_level: access_level)
+
+        expect(project_authorization).to exist
+      end
+    end
+
+    context 'inherited group members' do
+      before do
+        create(:group_member, access_level: access_level, group: group_parent, user: parent_group_user)
+        ProjectAuthorization.delete_all
+      end
+
+      it 'creates project authorization' do
+        expect { service.execute }.to(
+          change { ProjectAuthorization.count }.from(0).to(1))
+
+        project_authorization = ProjectAuthorization.where(
+          project_id: project.id,
+          user_id: parent_group_user.id,
+          access_level: access_level)
+        expect(project_authorization).to exist
+      end
+    end
+
+    context 'membership overrides' do
+      before do
+        create(:group_member, access_level: Gitlab::Access::REPORTER, group: group_parent, user: group_user)
+        create(:group_member, access_level: Gitlab::Access::DEVELOPER, group: group, user: group_user)
+        ProjectAuthorization.delete_all
+      end
+
+      it 'creates project authorization' do
+        expect { service.execute }.to(
+          change { ProjectAuthorization.count }.from(0).to(1))
+
+        project_authorization = ProjectAuthorization.where(
+          project_id: project.id,
+          user_id: group_user.id,
+          access_level: Gitlab::Access::DEVELOPER)
+        expect(project_authorization).to exist
+      end
+    end
+
+    context 'no group member' do
+      it 'does not create project authorization' do
+        expect { service.execute }.not_to(
+          change { ProjectAuthorization.count }.from(0))
+      end
+    end
+
+    context 'unapproved access requests' do
+      before do
+        create(:group_member, :guest, :access_request, user: group_user, group: group)
+      end
+
+      it 'does not create project authorization' do
+        expect { service.execute }.not_to(
+          change { ProjectAuthorization.count }.from(0))
+      end
+    end
+
+    context 'project has more users than BATCH_SIZE' do
+      let(:batch_size) { 2 }
+      let(:users) { create_list(:user, batch_size + 1 ) }
+
+      before do
+        stub_const("#{described_class.name}::BATCH_SIZE", batch_size)
+
+        users.each do |user|
+          create(:group_member, access_level: access_level, group: group_parent, user: user)
+        end
+
+        ProjectAuthorization.delete_all
+      end
+
+      it 'bulk creates project authorizations in batches' do
+        users.each_slice(batch_size) do |batch|
+          attributes = batch.map do |user|
+            { user_id: user.id, project_id: project.id, access_level: access_level }
+          end
+
+          expect(ProjectAuthorization).to(
+            receive(:insert_all).with(array_including(attributes)).and_call_original)
+        end
+
+        expect { service.execute }.to(
+          change { ProjectAuthorization.count }.from(0).to(batch_size + 1))
+      end
+    end
+
+    context 'users have existing project authorizations' do
+      before do
+        create(:group_member, access_level: access_level, group: group, user: group_user)
+        ProjectAuthorization.delete_all
+
+        create(:project_authorization, user_id: group_user.id,
+                                       project_id: project.id,
+                                       access_level: existing_access_level)
+      end
+
+      context 'when access level is the same' do
+        let(:existing_access_level) { access_level }
+
+        it 'does not create project authorization' do
+          project_authorization = ProjectAuthorization.where(
+            project_id: project.id,
+            user_id: group_user.id,
+            access_level: existing_access_level)
+
+          expect(ProjectAuthorization).not_to receive(:insert_all)
+
+          expect { service.execute }.not_to(
+            change { project_authorization.reload.exists? }.from(true))
+        end
+      end
+
+      context 'when existing access level is lower' do
+        let(:existing_access_level) { Gitlab::Access::DEVELOPER }
+
+        it 'creates new project authorization' do
+          project_authorization = ProjectAuthorization.where(
+            project_id: project.id,
+            user_id: group_user.id,
+            access_level: access_level)
+
+          expect { service.execute }.to(
+            change { project_authorization.reload.exists? }.from(false).to(true))
+        end
+
+        it 'deletes previous project authorization' do
+          project_authorization = ProjectAuthorization.where(
+            project_id: project.id,
+            user_id: group_user.id,
+            access_level: existing_access_level)
+
+          expect { service.execute }.to(
+            change { project_authorization.reload.exists? }.from(true).to(false))
+        end
+      end
+
+      context 'when existing access level is higher' do
+        let(:existing_access_level) { Gitlab::Access::OWNER }
+
+        it 'does not create project authorization' do
+          project_authorization = ProjectAuthorization.where(
+            project_id: project.id,
+            user_id: group_user.id,
+            access_level: existing_access_level)
+
+          expect(ProjectAuthorization).not_to receive(:insert_all)
+
+          expect { service.execute }.not_to(
+            change { project_authorization.reload.exists? }.from(true))
+        end
+      end
+    end
+  end
+end

spec/workers/authorized_project_update/project_create_worker_spec.rb

@@ -27,7 +27,7 @@ describe AuthorizedProjectUpdate::ProjectCreateWorker do
 
   context 'idempotence' do
     before do
-      create(:group_member, access_level: Gitlab::Access::MAINTAINER, group: group, user: group_user)
+      create(:group_member, access_level: access_level, group: group, user: group_user)
       ProjectAuthorization.delete_all
     end
 

spec/workers/authorized_project_update/project_group_link_create_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe AuthorizedProjectUpdate::ProjectGroupLinkCreateWorker do
+  let_it_be(:group) { create(:group, :private) }
+  let_it_be(:group_project) { create(:project, group: group) }
+  let_it_be(:shared_with_group) { create(:group, :private) }
+  let_it_be(:user) { create(:user) }
+
+  let(:access_level) { Gitlab::Access::MAINTAINER }
+
+  subject(:worker) { described_class.new }
+
+  it 'calls AuthorizedProjectUpdate::ProjectCreateService' do
+    expect_next_instance_of(AuthorizedProjectUpdate::ProjectGroupLinkCreateService) do |service|
+      expect(service).to(receive(:execute))
+    end
+
+    worker.perform(group_project.id, shared_with_group.id)
+  end
+
+  it 'returns ServiceResponse.success' do
+    result = worker.perform(group_project.id, shared_with_group.id)
+
+    expect(result.success?).to be_truthy
+  end
+
+  context 'idempotence' do
+    before do
+      create(:group_member, group: shared_with_group, user: user, access_level: access_level)
+      create(:project_group_link, project: group_project, group: shared_with_group)
+      ProjectAuthorization.delete_all
+    end
+
+    include_examples 'an idempotent worker' do
+      let(:job_args) { [group_project.id, shared_with_group.id] }
+
+      it 'creates project authorization' do
+        subject
+
+        project_authorization = ProjectAuthorization.where(
+          project_id: group_project.id,
+          user_id: user.id,
+          access_level: access_level)
+
+        expect(project_authorization).to exist
+        expect(ProjectAuthorization.count).to eq(1)
+      end
+    end
+  end
+end