Merge branch 'revoke-token-delete-project-bot' into 'master'

Delete project bot when token is revoked

Closes #244545

See merge request gitlab-org/gitlab!43373

app/services/resource_access_tokens/revoke_service.rb

@@ -14,18 +14,15 @@ module ResourceAccessTokens
     end
 
     def execute
+      return error("#{current_user.name} cannot delete #{bot_user.name}") unless can_destroy_bot_member?
       return error("Failed to find bot user") unless find_member
 
-      PersonalAccessToken.transaction do
-        access_token.revoke!
+      access_token.revoke!
 
-        raise RevokeAccessTokenError, "Failed to remove #{bot_user.name} member from: #{resource.name}" unless remove_member
+      destroy_bot_user
 
-        raise RevokeAccessTokenError, "Migration to ghost user failed" unless migrate_to_ghost_user
-      end
-
-      success("Revoked access token: #{access_token.name}")
-    rescue ActiveRecord::ActiveRecordError, RevokeAccessTokenError => error
+      success("Access token #{access_token.name} has been revoked and the bot user has been scheduled for deletion.")
+    rescue StandardError => error
       log_error("Failed to revoke access token for #{bot_user.name}: #{error.message}")
       error(error.message)
     end
@@ -34,12 +31,18 @@ module ResourceAccessTokens
 
     attr_reader :current_user, :access_token, :bot_user, :resource
 
-    def remove_member
-      ::Members::DestroyService.new(current_user).execute(find_member, destroy_bot: true)
+    def destroy_bot_user
+      DeleteUserWorker.perform_async(current_user.id, bot_user.id, skip_authorization: true)
     end
 
-    def migrate_to_ghost_user
-      ::Users::MigrateToGhostUserService.new(bot_user).execute
+    def can_destroy_bot_member?
+      if resource.is_a?(Project)
+        can?(current_user, :admin_project_member, @resource)
+      elsif resource.is_a?(Group)
+        can?(current_user, :admin_group_member, @resource)
+      else
+        false
+      end
     end
 
     def find_member

app/services/users/destroy_service.rb

@@ -26,7 +26,7 @@ module Users
     def execute(user, options = {})
       delete_solo_owned_groups = options.fetch(:delete_solo_owned_groups, options[:hard_delete])
 
-      unless Ability.allowed?(current_user, :destroy_user, user)
+      unless Ability.allowed?(current_user, :destroy_user, user) || options[:skip_authorization]
         raise Gitlab::Access::AccessDeniedError, "#{current_user} tried to destroy user #{user}!"
       end
 

changelogs/unreleased/revoke-token-delete-project-bot.yml

+---
+title: Delete project bot when token is revoked
+merge_request: 43373
+author:
+type: fixed

spec/controllers/projects/settings/access_tokens_controller_spec.rb

@@ -143,15 +143,15 @@ RSpec.describe Projects::Settings::AccessTokensController do
 
     it_behaves_like 'feature unavailable'
 
-    context 'when feature is available' do
+    context 'when feature is available', :sidekiq_inline do
       before do
         enable_feature
       end
 
-      it 'revokes token access' do
-        subject
+      it 'calls delete user worker' do
+        expect(DeleteUserWorker).to receive(:perform_async).with(user.id, bot_user.id, skip_authorization: true)
 
-        expect(project_access_token.reload.revoked?).to be true
+        subject
       end
 
       it 'removed membership of bot user' do
@@ -160,12 +160,6 @@ RSpec.describe Projects::Settings::AccessTokensController do
         expect(project.reload.bots).not_to include(bot_user)
       end
 
-      it 'blocks project bot user' do
-        subject
-
-        expect(bot_user.reload.blocked?).to be true
-      end
-
       it 'converts issuables of the bot user to ghost user' do
         issue = create(:issue, author: bot_user)
 
@@ -173,6 +167,12 @@ RSpec.describe Projects::Settings::AccessTokensController do
 
         expect(issue.reload.author.ghost?).to be true
       end
+
+      it 'deletes project bot user' do
+        subject
+
+        expect(User.exists?(bot_user.id)).to be_falsy
+      end
     end
   end
 

spec/services/resource_access_tokens/revoke_service_spec.rb

@@ -8,17 +8,17 @@ RSpec.describe ResourceAccessTokens::RevokeService do
   let_it_be(:user) { create(:user) }
   let(:access_token) { create(:personal_access_token, user: resource_bot) }
 
-  describe '#execute' do
+  describe '#execute', :sidekiq_inline do
     # Created shared_examples as it will easy to include specs for group bots in https://gitlab.com/gitlab-org/gitlab/-/issues/214046
     shared_examples 'revokes access token' do
       it { expect(subject.success?).to be true }
 
-      it { expect(subject.message).to eq("Revoked access token: #{access_token.name}") }
+      it { expect(subject.message).to eq("Access token #{access_token.name} has been revoked and the bot user has been scheduled for deletion.") }
 
-      it 'revokes token access' do
-        subject
+      it 'calls delete user worker' do
+        expect(DeleteUserWorker).to receive(:perform_async).with(user.id, resource_bot.id, skip_authorization: true)
 
-        expect(access_token.reload.revoked?).to be true
+        subject
       end
 
       it 'removes membership of bot user' do
@@ -34,6 +34,12 @@ RSpec.describe ResourceAccessTokens::RevokeService do
 
         expect(issue.reload.author.ghost?).to be true
       end
+
+      it 'deletes project bot user' do
+        subject
+
+        expect(User.exists?(resource_bot.id)).to be_falsy
+      end
     end
 
     shared_examples 'rollback revoke steps' do
@@ -56,49 +62,71 @@ RSpec.describe ResourceAccessTokens::RevokeService do
 
         expect(issue.reload.author.ghost?).to be false
       end
+
+      it 'does not destroy project bot user' do
+        subject
+
+        expect(User.exists?(resource_bot.id)).to be_truthy
+      end
     end
 
     context 'when resource is a project' do
       let_it_be(:resource) { create(:project, :private) }
-      let_it_be(:resource_bot) { create(:user, :project_bot) }
+      let(:resource_bot) { create(:user, :project_bot) }
 
-      before_all do
+      before do
         resource.add_maintainer(user)
         resource.add_maintainer(resource_bot)
       end
 
       it_behaves_like 'revokes access token'
 
-      context 'when revoke fails' do
-        context 'invalid resource type' do
-          subject { described_class.new(user, resource, access_token).execute }
+      context 'revoke fails' do
+        let_it_be(:other_user) { create(:user) }
 
-          let_it_be(:resource) { double }
-          let_it_be(:resource_bot) { create(:user, :project_bot) }
+        context 'when access token does not belong to this project' do
+          it 'does not find the bot' do
+            other_access_token = create(:personal_access_token, user: other_user)
 
-          it 'returns error response' do
-            response = subject
+            response = described_class.new(user, resource, other_access_token).execute
 
             expect(response.success?).to be false
             expect(response.message).to eq("Failed to find bot user")
+            expect(access_token.reload.revoked?).to be false
           end
-
-          it { expect { subject }.not_to change(access_token.reload, :revoked) }
         end
 
-        context 'when migration to ghost user fails' do
-          before do
-            allow_next_instance_of(::Members::DestroyService) do |service|
-              allow(service).to receive(:execute).and_return(false)
+        context 'when user does not have permission to destroy bot' do
+          context 'when non-project member tries to delete project bot' do
+            it 'does not allow other user to delete bot' do
+              response = described_class.new(other_user, resource, access_token).execute
+
+              expect(response.success?).to be false
+              expect(response.message).to eq("#{other_user.name} cannot delete #{access_token.user.name}")
+              expect(access_token.reload.revoked?).to be false
             end
           end
 
-          it_behaves_like 'rollback revoke steps'
+          context 'when non-maintainer project member tries to delete project bot' do
+            let(:developer) { create(:user) }
+
+            before do
+              resource.add_developer(developer)
+            end
+
+            it 'does not allow developer to delete bot' do
+              response = described_class.new(developer, resource, access_token).execute
+
+              expect(response.success?).to be false
+              expect(response.message).to eq("#{developer.name} cannot delete #{access_token.user.name}")
+              expect(access_token.reload.revoked?).to be false
+            end
+          end
         end
 
-        context 'when migration to ghost user fails' do
+        context 'when deletion of bot user fails' do
           before do
-            allow_next_instance_of(::Users::MigrateToGhostUserService) do |service|
+            allow_next_instance_of(::ResourceAccessTokens::RevokeService) do |service|
               allow(service).to receive(:execute).and_return(false)
             end
           end

spec/services/users/destroy_service_spec.rb

@@ -234,6 +234,14 @@ RSpec.describe Users::DestroyService do
 
         expect(User.exists?(user.id)).to be(false)
       end
+
+      it 'allows user to be deleted if skip_authorization: true' do
+        other_user = create(:user)
+
+        described_class.new(user).execute(other_user, skip_authorization: true)
+
+        expect(User.exists?(other_user.id)).to be(false)
+      end
     end
 
     context "migrating associated records" do