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

Project share to use low urgency project authorization refresh

See merge request gitlab-org/gitlab!32864

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/services/projects/group_links/create_service.rb

@@ -13,12 +13,32 @@ module Projects
         )
 
         if link.save
-          group.refresh_members_authorized_projects
+          setup_authorizations(group)
           success(link: link)
         else
           error(link.errors.full_messages.to_sentence, 409)
         end
       end
+
+      private
+
+      def setup_authorizations(group)
+        if Feature.enabled?(:specialized_project_authorization_project_share_worker)
+          AuthorizedProjectUpdate::ProjectGroupLinkCreateWorker.perform_async(project.id, group.id)
+
+          # AuthorizedProjectsWorker uses an exclusive lease per user but
+          # specialized workers might have synchronization issues. Until we
+          # compare the inconsistency rates of both approaches, we still run
+          # AuthorizedProjectsWorker but with some delay and lower urgency as a
+          # safety net.
+          group.refresh_members_authorized_projects(
+            blocking: false,
+            priority: UserProjectAccessChangedService::LOW_PRIORITY
+          )
+        else
+          group.refresh_members_authorized_projects(blocking: false)
+        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/requests/api/projects_spec.rb

@@ -2133,7 +2133,7 @@ describe API::Projects do
       expect(json_response['expires_at']).to eq(expires_at.to_s)
     end
 
-    it 'updates project authorization' do
+    it 'updates project authorization', :sidekiq_inline do
       expect do
         post api("/projects/#{project.id}/share", user), params: { group_id: group.id, group_access: Gitlab::Access::DEVELOPER }
       end.to(

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/services/projects/group_links/create_service_spec.rb

@@ -23,7 +23,7 @@ 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
+  it 'updates authorization', :sidekiq_inline do
     expect { subject.execute(group) }.to(
       change { Ability.allowed?(user, :read_project, project) }
         .from(false).to(true))
@@ -36,4 +36,50 @@ describe Projects::GroupLinks::CreateService, '#execute' do
   it 'returns error if user is not allowed to share with a group' do
     expect { subject.execute(create(:group)) }.not_to change { project.project_group_links.count }
   end
+
+  context 'with specialized_project_authorization_workers' do
+    let_it_be(:other_user) { create(:user) }
+
+    before do
+      group.add_developer(other_user)
+    end
+
+    it 'schedules authorization update for users with access to group' do
+      expect(AuthorizedProjectsWorker).not_to(
+        receive(:bulk_perform_async)
+      )
+      expect(AuthorizedProjectUpdate::ProjectGroupLinkCreateWorker).to(
+        receive(:perform_async).and_call_original
+      )
+      expect(AuthorizedProjectUpdate::UserRefreshWithLowUrgencyWorker).to(
+        receive(:bulk_perform_in)
+          .with(1.hour,
+                array_including([user.id], [other_user.id]),
+                batch_delay: 30.seconds, batch_size: 100)
+          .and_call_original
+      )
+
+      subject.execute(group)
+    end
+
+    context 'when feature is disabled' do
+      before do
+        stub_feature_flags(specialized_project_authorization_project_share_worker: false)
+      end
+
+      it 'uses AuthorizedProjectsWorker' do
+        expect(AuthorizedProjectsWorker).to(
+          receive(:bulk_perform_async).with(array_including([user.id], [other_user.id])).and_call_original
+        )
+        expect(AuthorizedProjectUpdate::ProjectCreateWorker).not_to(
+          receive(:perform_async)
+        )
+        expect(AuthorizedProjectUpdate::UserRefreshWithLowUrgencyWorker).not_to(
+          receive(:bulk_perform_in)
+        )
+
+        subject.execute(group)
+      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