Merge branch...

Merge branch '223851-create-specialized-worker-for-refreshing-project-authorizations-during-project-share-deletion' into 'master'

Use specialized worker to refresh authorizations on group share removal [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!60904

app/services/authorized_project_update/project_recalculate_service.rb

+# frozen_string_literal: true
+
+module AuthorizedProjectUpdate
+  class ProjectRecalculateService
+    # Service for refreshing all the authorizations to a particular project.
+    include Gitlab::Utils::StrongMemoize
+    BATCH_SIZE = 1000
+
+    def initialize(project)
+      @project = project
+    end
+
+    def execute
+      refresh_authorizations if needs_refresh?
+      ServiceResponse.success
+    end
+
+    private
+
+    attr_reader :project
+
+    def needs_refresh?
+      user_ids_to_remove.any? ||
+        authorizations_to_create.any?
+    end
+
+    def current_authorizations
+      strong_memoize(:current_authorizations) do
+        project.project_authorizations
+          .pluck(:user_id, :access_level) # rubocop: disable CodeReuse/ActiveRecord
+      end
+    end
+
+    def fresh_authorizations
+      strong_memoize(:fresh_authorizations) do
+        result = []
+
+        Projects::Members::EffectiveAccessLevelFinder.new(project)
+          .execute
+          .each_batch(of: BATCH_SIZE, column: :user_id) do |member_batch|
+            result += member_batch.pluck(:user_id, 'MAX(access_level)') # rubocop: disable CodeReuse/ActiveRecord
+          end
+
+        result
+      end
+    end
+
+    def user_ids_to_remove
+      strong_memoize(:user_ids_to_remove) do
+        (current_authorizations - fresh_authorizations)
+          .map {|user_id, _| user_id }
+      end
+    end
+
+    def authorizations_to_create
+      strong_memoize(:authorizations_to_create) do
+        (fresh_authorizations - current_authorizations).map do |user_id, access_level|
+          {
+            user_id: user_id,
+            access_level: access_level,
+            project_id: project.id
+          }
+        end
+      end
+    end
+
+    def refresh_authorizations
+      ProjectAuthorization.transaction do
+        if user_ids_to_remove.any?
+          ProjectAuthorization.where(project_id: project.id, user_id: user_ids_to_remove) # 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/destroy_service.rb

@@ -13,9 +13,27 @@ module Projects
         end
 
         group_link.destroy.tap do |link|
-          link.group.refresh_members_authorized_projects
+          if Feature.enabled?(:use_specialized_worker_for_project_auth_recalculation)
+            refresh_project_authorizations_asynchronously(link.project)
+
+            # Until we compare the inconsistency rates of the new specialized worker and
+            # the old approach, we still run AuthorizedProjectsWorker
+            # but with some delay and lower urgency as a safety net.
+            link.group.refresh_members_authorized_projects(
+              blocking: false,
+              priority: UserProjectAccessChangedService::LOW_PRIORITY
+            )
+          else
+            link.group.refresh_members_authorized_projects
+          end
         end
       end
+
+      private
+
+      def refresh_project_authorizations_asynchronously(project)
+        AuthorizedProjectUpdate::ProjectRecalculateWorker.perform_async(project.id)
+      end
     end
   end
 end

app/workers/all_queues.yml

@@ -39,6 +39,15 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: authorized_projects:authorized_project_update_project_recalculate
+  :worker_name: AuthorizedProjectUpdate::ProjectRecalculateWorker
+  :feature_category: :authentication_and_authorization
+  :has_external_dependencies:
+  :urgency: :high
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: auto_devops:auto_devops_disable
   :worker_name: AutoDevops::DisableWorker
   :feature_category: :auto_devops

app/workers/authorized_project_update/project_recalculate_worker.rb

+# frozen_string_literal: true
+
+module AuthorizedProjectUpdate
+  class ProjectRecalculateWorker
+    include ApplicationWorker
+    include Gitlab::ExclusiveLeaseHelpers
+
+    feature_category :authentication_and_authorization
+    urgency :high
+    queue_namespace :authorized_projects
+
+    deduplicate :until_executing, including_scheduled: true
+    idempotent!
+
+    def perform(project_id)
+      project = Project.find_by_id(project_id)
+      return unless project
+
+      in_lock(lock_key(project), ttl: 10.seconds) do
+        AuthorizedProjectUpdate::ProjectRecalculateService.new(project).execute
+      end
+    end
+
+    private
+
+    def lock_key(project)
+      "#{self.class.name.underscore}/#{project.root_namespace.id}"
+    end
+  end
+end

config/feature_flags/development/use_specialized_worker_for_project_auth_recalculation.yml

+---
+name: use_specialized_worker_for_project_auth_recalculation
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60904
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/331073
+milestone: '14.0'
+type: development
+group: group::access
+default_enabled: false

config/sidekiq_queues.yml

@@ -44,6 +44,8 @@
   - 2
 - - authorized_project_update
   - 1
+- - authorized_projects
+  - 1
 - - authorized_projects
   - 2
 - - auto_devops

spec/requests/api/projects_spec.rb

@@ -2755,7 +2755,7 @@ RSpec.describe API::Projects do
         expect(project.project_group_links).to be_empty
       end
 
-      it 'updates project authorization' do
+      it 'updates project authorization', :sidekiq_inline do
         expect do
           delete api("/projects/#{project.id}/share/#{group.id}", user)
         end.to(

spec/services/authorized_project_update/project_recalculate_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe AuthorizedProjectUpdate::ProjectRecalculateService, '#execute' do
+  let_it_be(:project) { create(:project) }
+
+  subject(:execute) { described_class.new(project).execute }
+
+  it 'returns success' do
+    expect(execute.success?).to eq(true)
+  end
+
+  context 'when there are no changes to be made' do
+    it 'does not change authorizations' do
+      expect { execute }.not_to(change { ProjectAuthorization.count })
+    end
+  end
+
+  context 'when there are changes to be made' do
+    let(:user) { create(:user) }
+
+    context 'when addition is required' do
+      before do
+        project.add_developer(user)
+        project.project_authorizations.where(user: user).delete_all
+      end
+
+      it 'adds a new authorization record' do
+        expect { execute }.to(
+          change { project.project_authorizations.where(user: user).count }
+          .from(0).to(1)
+        )
+      end
+
+      it 'adds a new authorization record with the correct access level' do
+        execute
+
+        project_authorization = project.project_authorizations.where(
+          user: user,
+          access_level: Gitlab::Access::DEVELOPER
+        )
+
+        expect(project_authorization).to exist
+      end
+    end
+
+    context 'when removal is required' do
+      before do
+        create(:project_authorization, user: user, project: project)
+      end
+
+      it 'removes the authorization record' do
+        expect { execute }.to(
+          change { project.project_authorizations.where(user: user).count }
+          .from(1).to(0)
+        )
+      end
+    end
+
+    context 'when an update in access level is required' do
+      before do
+        project.add_developer(user)
+        project.project_authorizations.where(user: user).delete_all
+        create(:project_authorization, project: project, user: user, access_level: Gitlab::Access::GUEST)
+      end
+
+      it 'updates the authorization of the user to the correct access level' do
+        expect { execute }.to(
+          change { project.project_authorizations.find_by(user: user).access_level }
+            .from(Gitlab::Access::GUEST).to(Gitlab::Access::DEVELOPER)
+        )
+      end
+    end
+  end
+end

spec/services/projects/group_links/destroy_service_spec.rb

@@ -14,12 +14,60 @@ RSpec.describe Projects::GroupLinks::DestroyService, '#execute' 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)
+  context 'project authorizations refresh' do
+    before do
+      group.add_maintainer(user)
+    end
+
+    context 'when the feature flag `use_specialized_worker_for_project_auth_recalculation` is enabled' do
+      before do
+        stub_feature_flags(use_specialized_worker_for_project_auth_recalculation: true)
+      end
+
+      it 'calls AuthorizedProjectUpdate::ProjectRecalculateWorker to update project authorizations' do
+        expect(AuthorizedProjectUpdate::ProjectRecalculateWorker)
+          .to receive(:perform_async).with(group_link.project.id)
+
+        subject.execute(group_link)
+      end
+
+      it 'calls AuthorizedProjectUpdate::UserRefreshWithLowUrgencyWorker with a delay to update project authorizations' do
+        expect(AuthorizedProjectUpdate::UserRefreshWithLowUrgencyWorker).to(
+          receive(:bulk_perform_in)
+            .with(1.hour,
+                  [[user.id]],
+                  batch_delay: 30.seconds, batch_size: 100)
+        )
+
+        subject.execute(group_link)
+      end
 
-    expect { subject.execute(group_link) }.to(
-      change { Ability.allowed?(user, :read_project, project) }
-        .from(true).to(false))
+      it 'updates project authorizations of users who had access to the project via the group share', :sidekiq_inline do
+        expect { subject.execute(group_link) }.to(
+          change { Ability.allowed?(user, :read_project, project) }
+            .from(true).to(false))
+      end
+    end
+
+    context 'when the feature flag `use_specialized_worker_for_project_auth_recalculation` is disabled' do
+      before do
+        stub_feature_flags(use_specialized_worker_for_project_auth_recalculation: false)
+      end
+
+      it 'calls UserProjectAccessChangedService to update project authorizations' do
+        expect_next_instance_of(UserProjectAccessChangedService, [user.id]) do |service|
+          expect(service).to receive(:execute)
+        end
+
+        subject.execute(group_link)
+      end
+
+      it 'updates project authorizations of users who had access to the project via the group share' do
+        expect { subject.execute(group_link) }.to(
+          change { Ability.allowed?(user, :read_project, project) }
+            .from(true).to(false))
+      end
+    end
   end
 
   it 'returns false if group_link is blank' do

spec/workers/authorized_project_update/project_recalculate_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe AuthorizedProjectUpdate::ProjectRecalculateWorker do
+  include ExclusiveLeaseHelpers
+
+  let_it_be(:project) { create(:project) }
+
+  subject(:worker) { described_class.new }
+
+  it 'is labeled as high urgency' do
+    expect(described_class.get_urgency).to eq(:high)
+  end
+
+  include_examples 'an idempotent worker' do
+    let(:job_args) { project.id }
+
+    it 'does not change authorizations when run twice' do
+      user = create(:user)
+      project.add_developer(user)
+
+      user.project_authorizations.delete_all
+
+      expect { worker.perform(project.id) }.to change { project.project_authorizations.reload.size }.by(1)
+      expect { worker.perform(project.id) }.not_to change { project.project_authorizations.reload.size }
+    end
+  end
+
+  describe '#perform' do
+    it 'does not fail if the project does not exist' do
+      expect do
+        worker.perform(non_existing_record_id)
+      end.not_to raise_error
+    end
+
+    it 'calls AuthorizedProjectUpdate::ProjectRecalculateService' do
+      expect_next_instance_of(AuthorizedProjectUpdate::ProjectRecalculateService, project) do |service|
+        expect(service).to receive(:execute)
+      end
+
+      worker.perform(project.id)
+    end
+
+    context 'exclusive lease' do
+      let(:lock_key) { "#{described_class.name.underscore}/#{project.root_namespace.id}" }
+      let(:timeout) { 10.seconds }
+
+      context 'when exclusive lease has not been taken' do
+        it 'obtains a new exclusive lease' do
+          expect_to_obtain_exclusive_lease(lock_key, timeout: timeout)
+
+          worker.perform(project.id)
+        end
+      end
+
+      context 'when exclusive lease has already been taken' do
+        before do
+          stub_exclusive_lease_taken(lock_key, timeout: timeout)
+        end
+
+        it 'raises an error' do
+          expect { worker.perform(project.id) }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError)
+        end
+      end
+    end
+  end
+end

spec/workers/remove_expired_group_links_worker_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe RemoveExpiredGroupLinksWorker do
         expect(non_expiring_project_group_link.reload).to be_present
       end
 
-      it 'removes project authorization' do
+      it 'removes project authorization', :sidekiq_inline do
         user = create(:user)
 
         project = expired_project_group_link.project