Merge branch 'mmj-async-auth-refresh-during-member-destroy' into 'master'

Async auth refresh during member destroy

See merge request gitlab-org/gitlab!66424

app/models/member.rb

@@ -178,7 +178,13 @@ class Member < ApplicationRecord
   after_destroy :post_destroy_hook, unless: :pending?, if: :hook_prerequisites_met?
   after_save :log_invitation_token_cleanup
 
-  after_commit :refresh_member_authorized_projects, unless: :importing?
+  after_commit on: [:create, :update], unless: :importing? do
+    refresh_member_authorized_projects(blocking: true)
+  end
+
+  after_commit on: [:destroy], unless: :importing? do
+    refresh_member_authorized_projects(blocking: Feature.disabled?(:member_destroy_async_auth_refresh, type: :ops))
+  end
 
   default_value_for :notification_level, NotificationSetting.levels[:global]
 
@@ -395,8 +401,8 @@ class Member < ApplicationRecord
   # transaction has been committed, resulting in the job either throwing an
   # error or not doing any meaningful work.
   # rubocop: disable CodeReuse/ServiceClass
-  def refresh_member_authorized_projects
-    UserProjectAccessChangedService.new(user_id).execute
+  def refresh_member_authorized_projects(blocking:)
+    UserProjectAccessChangedService.new(user_id).execute(blocking: blocking)
   end
   # rubocop: enable CodeReuse/ServiceClass
 

app/models/members/group_member.rb

@@ -50,8 +50,10 @@ class GroupMember < Member
     { group: group }
   end
 
+  private
+
   override :refresh_member_authorized_projects
-  def refresh_member_authorized_projects
+  def refresh_member_authorized_projects(blocking:)
     # Here, `destroyed_by_association` will be present if the
     # GroupMember is being destroyed due to the `dependent: :destroy`
     # callback on Group. In this case, there is no need to refresh the
@@ -63,8 +65,6 @@ class GroupMember < Member
     super
   end
 
-  private
-
   def access_level_inclusion
     return if access_level.in?(Gitlab::Access.all_values)
 

app/models/members/project_member.rb

@@ -90,24 +90,28 @@ class ProjectMember < Member
     { project: project }
   end
 
+  private
+
   override :refresh_member_authorized_projects
-  def refresh_member_authorized_projects
+  def refresh_member_authorized_projects(blocking:)
     return super unless Feature.enabled?(:specialized_service_for_project_member_auth_refresh)
     return unless user
 
     # rubocop:disable CodeReuse/ServiceClass
-    AuthorizedProjectUpdate::ProjectRecalculatePerUserService.new(project, user).execute
+    if blocking
+      AuthorizedProjectUpdate::ProjectRecalculatePerUserService.new(project, user).execute
+    else
+      AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker.perform_async(project.id, user.id)
+    end
 
     # Until we compare the inconsistency rates of the new, specialized service and
     # the old approach, we still run AuthorizedProjectsWorker
     # but with some delay and lower urgency as a safety net.
     UserProjectAccessChangedService.new(user_id)
-      .execute(blocking: false, priority: UserProjectAccessChangedService::LOW_PRIORITY)
+                                   .execute(blocking: false, priority: UserProjectAccessChangedService::LOW_PRIORITY)
     # rubocop:enable CodeReuse/ServiceClass
   end
 
-  private
-
   def send_invite
     run_after_commit_or_now { notification_service.invite_project_member(self, @raw_invite_token) }
 

app/workers/all_queues.yml

@@ -30,6 +30,15 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: authorized_project_update:authorized_project_update_project_recalculate_per_user
+  :worker_name: AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker
+  :feature_category: :authentication_and_authorization
+  :has_external_dependencies:
+  :urgency: :high
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: authorized_project_update:authorized_project_update_user_refresh_from_replica
   :worker_name: AuthorizedProjectUpdate::UserRefreshFromReplicaWorker
   :feature_category: :authentication_and_authorization

app/workers/authorized_project_update/project_recalculate_per_user_worker.rb

+# frozen_string_literal: true
+
+module AuthorizedProjectUpdate
+  class ProjectRecalculatePerUserWorker < ProjectRecalculateWorker
+    data_consistency :always
+
+    feature_category :authentication_and_authorization
+    urgency :high
+    queue_namespace :authorized_project_update
+
+    deduplicate :until_executing, including_scheduled: true
+    idempotent!
+
+    def perform(project_id, user_id)
+      project = Project.find_by_id(project_id)
+      user = User.find_by_id(user_id)
+
+      return unless project && user
+
+      in_lock(lock_key(project), ttl: 10.seconds) do
+        AuthorizedProjectUpdate::ProjectRecalculatePerUserService.new(project, user).execute
+      end
+    end
+  end
+end

app/workers/authorized_project_update/project_recalculate_worker.rb

@@ -26,7 +26,9 @@ module AuthorizedProjectUpdate
     private
 
     def lock_key(project)
-      "#{self.class.name.underscore}/projects/#{project.id}"
+      # The self.class.name.underscore value is hardcoded here as the prefix, so that the same
+      # lock_key for this superclass will be used by the ProjectRecalculatePerUserWorker subclass.
+      "authorized_project_update/project_recalculate_worker/projects/#{project.id}"
     end
   end
 end

config/feature_flags/ops/member_destroy_async_auth_refresh.yml

+---
+name: member_destroy_async_auth_refresh
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66424
+rollout_issue_url:
+milestone: '14.4'
+type: ops
+group: group::access
+default_enabled: false

ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb

@@ -282,7 +282,7 @@ RSpec.describe EE::Gitlab::Auth::Ldap::Sync::Group do
             .to eq(::Gitlab::Access::OWNER)
         end
 
-        it 'updates projects authorizations' do
+        it 'updates projects authorizations', :sidekiq_inline do
           project = create(:project, namespace: group)
           group.add_user(user, Gitlab::Access::MAINTAINER)
 

ee/spec/models/approval_state_spec.rb

@@ -1336,7 +1336,7 @@ RSpec.describe ApprovalState do
             expect(subject.can_approve?(nil)).to be_falsey
           end
 
-          context 'when an approver does not have access to the merge request' do
+          context 'when an approver does not have access to the merge request', :sidekiq_inline do
             before do
               project.members.find_by(user_id: developer.id).destroy!
             end

ee/spec/services/todo_service_spec.rb

@@ -324,7 +324,7 @@ RSpec.describe TodoService do
     let(:project) { create(:project, :private, :repository) }
     let(:merge_request) { create(:merge_request, source_project: project, author: author) }
 
-    context 'an approver has lost access to the project' do
+    context 'an approver has lost access to the project', :sidekiq_inline do
       before do
         create(:approver, user: non_member, target: project)
         project.members.find_by(user_id: non_member.id).destroy

spec/controllers/projects/branches_controller_spec.rb

@@ -239,7 +239,7 @@ RSpec.describe Projects::BranchesController do
         end
       end
 
-      context 'without issue feature access' do
+      context 'without issue feature access', :sidekiq_inline do
         before do
           project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
           project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)

spec/controllers/projects/compare_controller_spec.rb

@@ -409,7 +409,7 @@ RSpec.describe Projects::CompareController do
         end
       end
 
-      context 'when the user does not have access to the project' do
+      context 'when the user does not have access to the project', :sidekiq_inline do
         before do
           project.team.truncate
           project.update!(visibility: 'private')

spec/graphql/types/project_type_spec.rb

@@ -187,7 +187,7 @@ RSpec.describe GitlabSchema.types['Project'] do
       expect(analyzer['enabled']).to eq(true)
     end
 
-    context "with guest user" do
+    context 'with guest user' do
       before do
         project.add_guest(user)
       end
@@ -195,7 +195,7 @@ RSpec.describe GitlabSchema.types['Project'] do
       context 'when project is private' do
         let(:project) { create(:project, :private, :repository) }
 
-        it "returns no configuration" do
+        it 'returns no configuration' do
           secure_analyzers_prefix = subject.dig('data', 'project', 'sastCiConfiguration')
           expect(secure_analyzers_prefix).to be_nil
         end
@@ -215,7 +215,7 @@ RSpec.describe GitlabSchema.types['Project'] do
       end
     end
 
-    context "with non-member user" do
+    context 'with non-member user', :sidekiq_inline do
       before do
         project.team.truncate
       end
@@ -223,7 +223,7 @@ RSpec.describe GitlabSchema.types['Project'] do
       context 'when project is private' do
         let(:project) { create(:project, :private, :repository) }
 
-        it "returns no configuration" do
+        it 'returns no configuration' do
           secure_analyzers_prefix = subject.dig('data', 'project', 'sastCiConfiguration')
           expect(secure_analyzers_prefix).to be_nil
         end
@@ -241,7 +241,7 @@ RSpec.describe GitlabSchema.types['Project'] do
         end
 
         context 'when repository is accessible only by team members' do
-          it "returns no configuration" do
+          it 'returns no configuration' do
             project.project_feature.update!(
               merge_requests_access_level: ProjectFeature::DISABLED,
               builds_access_level: ProjectFeature::DISABLED,

spec/lib/gitlab/middleware/go_spec.rb

@@ -98,7 +98,7 @@ RSpec.describe Gitlab::Middleware::Go do
                     end
                   end
 
-                  context 'without access to the project' do
+                  context 'without access to the project', :sidekiq_inline do
                     before do
                       project.team.find_member(current_user).destroy
                     end

spec/lib/gitlab/slash_commands/issue_move_spec.rb

@@ -95,7 +95,7 @@ RSpec.describe Gitlab::SlashCommands::IssueMove, service: true do
       end
     end
 
-    context 'when the user cannot see the target project' do
+    context 'when the user cannot see the target project', :sidekiq_inline do
       it 'returns not found' do
         message = "issue move #{issue.iid} #{other_project.full_path}"
         other_project.team.truncate

spec/models/member_spec.rb

@@ -7,11 +7,11 @@ RSpec.describe Member do
 
   using RSpec::Parameterized::TableSyntax
 
-  describe "Associations" do
+  describe 'Associations' do
     it { is_expected.to belong_to(:user) }
   end
 
-  describe "Validation" do
+  describe 'Validation' do
     subject { described_class.new(access_level: Member::GUEST) }
 
     it { is_expected.to validate_presence_of(:user) }
@@ -28,7 +28,7 @@ RSpec.describe Member do
       subject { build(:project_member) }
     end
 
-    context "when an invite email is provided" do
+    context 'when an invite email is provided' do
       let_it_be(:project) { create(:project) }
 
       let(:member) { build(:project_member, source: project, invite_email: "user@example.com", user: nil) }
@@ -37,29 +37,29 @@ RSpec.describe Member do
         expect(member).to be_valid
       end
 
-      it "requires a valid invite email" do
+      it 'requires a valid invite email' do
         member.invite_email = "nope"
 
         expect(member).not_to be_valid
       end
 
-      it "requires a unique invite email scoped to this source" do
+      it 'requires a unique invite email scoped to this source' do
         create(:project_member, source: member.source, invite_email: member.invite_email)
 
         expect(member).not_to be_valid
       end
     end
 
-    context "when an invite email is not provided" do
+    context 'when an invite email is not provided' do
       let(:member) { build(:project_member) }
 
-      it "requires a user" do
+      it 'requires a user' do
         member.user = nil
 
         expect(member).not_to be_valid
       end
 
-      it "is valid otherwise" do
+      it 'is valid otherwise' do
         expect(member).to be_valid
       end
     end
@@ -107,13 +107,13 @@ RSpec.describe Member do
       end
     end
 
-    context "when a child member inherits its access level" do
+    context 'when a child member inherits its access level' do
       let(:user) { create(:user) }
       let(:member) { create(:group_member, :developer, user: user) }
       let(:child_group) { create(:group, parent: member.group) }
       let(:child_member) { build(:group_member, group: child_group, user: user) }
 
-      it "requires a higher level" do
+      it 'requires a higher level' do
         child_member.access_level = GroupMember::REPORTER
 
         child_member.validate
@@ -123,7 +123,7 @@ RSpec.describe Member do
 
       # Membership in a subgroup confers certain access rights, such as being
       # able to merge or push code to protected branches.
-      it "is valid with an equal level" do
+      it 'is valid with an equal level' do
         child_member.access_level = GroupMember::DEVELOPER
 
         child_member.validate
@@ -131,7 +131,7 @@ RSpec.describe Member do
         expect(child_member).to be_valid
       end
 
-      it "is valid with a higher level" do
+      it 'is valid with a higher level' do
         child_member.access_level = GroupMember::MAINTAINER
 
         child_member.validate
@@ -538,7 +538,7 @@ RSpec.describe Member do
     end
   end
 
-  describe "Delegate methods" do
+  describe 'Delegate methods' do
     it { is_expected.to respond_to(:user_name) }
     it { is_expected.to respond_to(:user_email) }
   end
@@ -608,29 +608,29 @@ RSpec.describe Member do
     end
   end
 
-  describe "#accept_invite!" do
+  describe '#accept_invite!' do
     let!(:member) { create(:project_member, invite_email: "user@example.com", user: nil) }
     let(:user) { create(:user) }
 
-    it "resets the invite token" do
+    it 'resets the invite token' do
       member.accept_invite!(user)
 
       expect(member.invite_token).to be_nil
     end
 
-    it "sets the invite accepted timestamp" do
+    it 'sets the invite accepted timestamp' do
       member.accept_invite!(user)
 
       expect(member.invite_accepted_at).not_to be_nil
     end
 
-    it "sets the user" do
+    it 'sets the user' do
       member.accept_invite!(user)
 
       expect(member.user).to eq(user)
     end
 
-    it "calls #after_accept_invite" do
+    it 'calls #after_accept_invite' do
       expect(member).to receive(:after_accept_invite)
 
       member.accept_invite!(user)
@@ -657,26 +657,26 @@ RSpec.describe Member do
     end
   end
 
-  describe "#decline_invite!" do
+  describe '#decline_invite!' do
     let!(:member) { create(:project_member, invite_email: "user@example.com", user: nil) }
 
-    it "destroys the member" do
+    it 'destroys the member' do
       member.decline_invite!
 
       expect(member).to be_destroyed
     end
 
-    it "calls #after_decline_invite" do
+    it 'calls #after_decline_invite' do
       expect(member).to receive(:after_decline_invite)
 
       member.decline_invite!
     end
   end
 
-  describe "#generate_invite_token" do
+  describe '#generate_invite_token' do
     let!(:member) { create(:project_member, invite_email: "user@example.com", user: nil) }
 
-    it "sets the invite token" do
+    it 'sets the invite token' do
       expect { member.generate_invite_token }.to change { member.invite_token }
     end
   end
@@ -684,12 +684,12 @@ RSpec.describe Member do
   describe 'generate invite token on create' do
     let!(:member) { build(:project_member, invite_email: "user@example.com") }
 
-    it "sets the invite token" do
+    it 'sets the invite token' do
       expect { member.save! }.to change { member.invite_token }.to(kind_of(String))
     end
 
     context 'when invite was already accepted' do
-      it "does not set invite token" do
+      it 'does not set invite token' do
         member.invite_accepted_at = 1.day.ago
 
         expect { member.save! }.not_to change { member.invite_token }.from(nil)
@@ -744,7 +744,7 @@ RSpec.describe Member do
     end
   end
 
-  describe "#invite_to_unknown_user?" do
+  describe '#invite_to_unknown_user?' do
     subject { member.invite_to_unknown_user? }
 
     let(:member) { create(:project_member, invite_email: "user@example.com", invite_token: '1234', user: user) }
@@ -762,7 +762,7 @@ RSpec.describe Member do
     end
   end
 
-  describe "destroying a record", :delete do
+  describe 'destroying a record', :delete, :sidekiq_inline do
     it "refreshes user's authorized projects" do
       project = create(:project, :private)
       user    = create(:user)

spec/models/members/project_member_spec.rb

@@ -244,12 +244,32 @@ RSpec.describe ProjectMember do
         project.add_user(user, Gitlab::Access::GUEST)
       end
 
-      it 'changes access level' do
-        expect { action }.to change { user.can?(:guest_access, project) }.from(true).to(false)
+      context 'when :member_destroy_async_auth_refresh feature flag is enabled' do
+        it 'changes access level', :sidekiq_inline do
+          expect { action }.to change { user.can?(:guest_access, project) }.from(true).to(false)
+        end
+
+        it 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker to recalculate authorizations' do
+          expect(AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker).to receive(:perform_async).with(project.id, user.id)
+
+          action
+        end
+
+        it_behaves_like 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations'
       end
 
-      it_behaves_like 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserService to recalculate authorizations'
-      it_behaves_like 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations'
+      context 'when :member_destroy_async_auth_refresh feature flag is disabled' do
+        before do
+          stub_feature_flags(member_destroy_async_auth_refresh: false)
+        end
+
+        it 'changes access level' do
+          expect { action }.to change { user.can?(:guest_access, project) }.from(true).to(false)
+        end
+
+        it_behaves_like 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserService to recalculate authorizations'
+        it_behaves_like 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations'
+      end
     end
 
     context 'when the feature flag `specialized_service_for_project_member_auth_refresh` is disabled' do
@@ -298,7 +318,7 @@ RSpec.describe ProjectMember do
           project.add_user(user, Gitlab::Access::GUEST)
         end
 
-        it 'changes access level' do
+        it 'changes access level', :sidekiq_inline do
           expect { action }.to change { user.can?(:guest_access, project) }.from(true).to(false)
         end
 

spec/requests/api/package_files_spec.rb

@@ -38,7 +38,7 @@ RSpec.describe API::PackageFiles do
           expect(response).to have_gitlab_http_status(:not_found)
         end
 
-        it 'returns 404 for a user without access to the project' do
+        it 'returns 404 for a user without access to the project', :sidekiq_inline do
           project.team.truncate
 
           get api(url, user)

spec/serializers/merge_request_poll_cached_widget_entity_spec.rb

@@ -275,7 +275,7 @@ RSpec.describe MergeRequestPollCachedWidgetEntity do
       expect(subject[:merge_pipeline]).to be_nil
     end
 
-    context 'when is merged' do
+    context 'when is merged', :sidekiq_inline do
       let(:resource) { create(:merged_merge_request, source_project: project, merge_commit_sha: project.commit.id) }
       let(:pipeline) { create(:ci_empty_pipeline, project: project, ref: resource.target_branch, sha: resource.merge_commit_sha) }
 

spec/services/merge_requests/assign_issues_service_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe MergeRequests::AssignIssuesService do
     expect(service.assignable_issues.map(&:id)).to include(issue.id)
   end
 
-  it 'ignores issues the user cannot update assignee on' do
+  it 'ignores issues the user cannot update assignee on', :sidekiq_inline do
     project.team.truncate
 
     expect(service.assignable_issues).to be_empty

spec/services/merge_requests/build_service_spec.rb

@@ -440,7 +440,7 @@ RSpec.describe MergeRequests::BuildService do
           expect(merge_request.title).to eq('Closes #1234 Second commit')
         end
 
-        it 'adds the remaining lines of the first multi-line commit message as the description' do
+        it 'adds the remaining lines of the first multi-line commit message as the description', :sidekiq_inline do
           expect(merge_request.description).to eq('Create the app')
         end
       end

spec/services/merge_requests/push_options_handler_service_spec.rb

@@ -701,7 +701,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do
     let(:push_options) { { create: true } }
     let(:changes) { new_branch_changes }
 
-    it 'records an error' do
+    it 'records an error', :sidekiq_inline do
       Members::DestroyService.new(user1).execute(ProjectMember.find_by!(user_id: user1.id))
 
       service.execute

spec/services/notes/quick_actions_service_spec.rb

@@ -47,7 +47,7 @@ RSpec.describe Notes::QuickActionsService do
         let(:note_text) { "/relate #{other_issue.to_reference}" }
         let(:note) { create(:note_on_issue, noteable: issue, project: project, note: note_text) }
 
-        context 'user cannot relate issues' do
+        context 'user cannot relate issues', :sidekiq_inline do
           before do
             project.team.find_member(maintainer.id).destroy!
             project.update!(visibility: Gitlab::VisibilityLevel::PUBLIC)

spec/services/notification_service_spec.rb

@@ -3155,7 +3155,7 @@ RSpec.describe NotificationService, :mailer do
             notification.pipeline_finished(pipeline)
           end
 
-          it 'does not send emails' do
+          it 'does not send emails', :sidekiq_inline do
             should_not_email_anyone
           end
         end

spec/services/projects/move_access_service_spec.rb

@@ -26,7 +26,7 @@ RSpec.describe Projects::MoveAccessService do
 
   describe '#execute' do
     shared_examples 'move the accesses' do
-      it do
+      it 'moves the accesses', :sidekiq_inline do
         expect(project_with_access.project_members.count).to eq 4
         expect(project_with_access.project_group_links.count).to eq 3
         expect(project_with_access.authorized_users.count).to eq 4

spec/workers/authorized_project_update/project_recalculate_per_user_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker do
+  include ExclusiveLeaseHelpers
+
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+
+  subject(:worker) { described_class.new }
+
+  include_examples 'an idempotent worker' do
+    let(:job_args) { [project.id, user.id] }
+
+    it 'does not change authorizations when run twice' do
+      project.add_developer(user)
+
+      user.project_authorizations.delete_all
+
+      expect { worker.perform(project.id, user.id) }.to change { project.project_authorizations.reload.size }.by(1)
+      expect { worker.perform(project.id, user.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, user.id)
+      end.not_to raise_error
+    end
+
+    it 'does not fail if the user does not exist' do
+      expect do
+        worker.perform(project.id, non_existing_record_id)
+      end.not_to raise_error
+    end
+
+    it 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserService' do
+      expect_next_instance_of(AuthorizedProjectUpdate::ProjectRecalculatePerUserService, project, user) do |service|
+        expect(service).to receive(:execute)
+      end
+
+      worker.perform(project.id, user.id)
+    end
+
+    context 'exclusive lease' do
+      let(:lock_key) { "#{described_class.superclass.name.underscore}/projects/#{project.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, user.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, user.id) }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError)
+        end
+      end
+    end
+  end
+end