Merge branch '212523-delete-deploy-tokens' into 'master'

Fix deploy token API to delete underlying deploy_token record

See merge request gitlab-org/gitlab!28156

app/services/concerns/deploy_token_methods.rb

@@ -8,4 +8,10 @@ module DeployTokenMethods
       deploy_token.username = params[:username].presence
     end
   end
+
+  def destroy_deploy_token(entity, params)
+    deploy_token = entity.deploy_tokens.find_by_id!(params[:token_id])
+
+    deploy_token.destroy
+  end
 end

app/services/groups/deploy_tokens/destroy_service.rb

+# frozen_string_literal: true
+
+module Groups
+  module DeployTokens
+    class DestroyService < BaseService
+      include DeployTokenMethods
+
+      def execute
+        destroy_deploy_token(@group, params)
+      end
+    end
+  end
+end

app/services/projects/deploy_tokens/destroy_service.rb

+# frozen_string_literal: true
+
+module Projects
+  module DeployTokens
+    class DestroyService < BaseService
+      include DeployTokenMethods
+
+      def execute
+        destroy_deploy_token(@project, params)
+      end
+    end
+  end
+end

changelogs/unreleased/212523-delete-deploy-tokens.yml

+---
+title: Fix deploy token API to properly delete all associated deploy token records
+merge_request: 28156
+author:
+type: fixed

lib/api/deploy_tokens.rb

@@ -85,12 +85,10 @@ module API
       delete ':id/deploy_tokens/:token_id' do
         authorize!(:destroy_deploy_token, user_project)
 
-        deploy_token = user_project.project_deploy_tokens
-          .find_by_deploy_token_id(params[:token_id])
-
-        not_found!('Deploy Token') unless deploy_token
+        ::Projects::DeployTokens::DestroyService.new(
+          user_project, current_user, token_id: params[:token_id]
+        ).execute
 
-        deploy_token.destroy
         no_content!
       end
     end
@@ -144,13 +142,17 @@ module API
       desc 'Delete a group deploy token' do
         detail 'This feature was introduced in GitLab 12.9'
       end
+      params do
+        requires :token_id, type: Integer, desc: 'The deploy token ID'
+      end
       delete ':id/deploy_tokens/:token_id' do
         authorize!(:destroy_deploy_token, user_group)
 
-        deploy_token = user_group.group_deploy_tokens
-          .find_by_deploy_token_id!(params[:token_id])
+        ::Groups::DeployTokens::DestroyService.new(
+          user_group, current_user, token_id: params[:token_id]
+        ).execute
 
-        destroy_conditionally!(deploy_token)
+        no_content!
       end
     end
   end

spec/requests/api/deploy_tokens_spec.rb

@@ -175,8 +175,12 @@ describe API::DeployTokens do
 
       it { is_expected.to have_gitlab_http_status(:no_content) }
 
-      it 'deletes the deploy token' do
-        expect { subject }.to change { project.deploy_tokens.count }.by(-1)
+      it 'calls the deploy token destroy service' do
+        expect(::Projects::DeployTokens::DestroyService).to receive(:new)
+          .with(project, user, token_id: deploy_token.id)
+          .and_return(true)
+
+        subject
       end
 
       context 'invalid request' do
@@ -187,9 +191,13 @@ describe API::DeployTokens do
         end
 
         it 'returns bad_request with invalid token id' do
-          delete api("/projects/#{project.id}/deploy_tokens/123abc", user)
+          expect(::Projects::DeployTokens::DestroyService).to receive(:new)
+            .with(project, user, token_id: 999)
+            .and_raise(ActiveRecord::RecordNotFound)
+
+          delete api("/projects/#{project.id}/deploy_tokens/999", user)
 
-          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(response).to have_gitlab_http_status(:not_found)
         end
       end
     end
@@ -307,10 +315,12 @@ describe API::DeployTokens do
         group.add_maintainer(user)
       end
 
-      it 'deletes the deploy token' do
-        expect { subject }.to change { group.deploy_tokens.count }.by(-1)
+      it 'calls the deploy token destroy service' do
+        expect(::Groups::DeployTokens::DestroyService).to receive(:new)
+          .with(group, user, token_id: group_deploy_token.id)
+          .and_return(true)
 
-        expect(group.deploy_tokens).to be_empty
+        subject
       end
 
       context 'invalid request' do
@@ -321,7 +331,11 @@ describe API::DeployTokens do
         end
 
         it 'returns not found with invalid deploy token id' do
-          delete api("/groups/#{group.id}/deploy_tokens/bad_id", user)
+          expect(::Groups::DeployTokens::DestroyService).to receive(:new)
+            .with(group, user, token_id: 999)
+            .and_raise(ActiveRecord::RecordNotFound)
+
+          delete api("/groups/#{group.id}/deploy_tokens/999", user)
 
           expect(response).to have_gitlab_http_status(:not_found)
         end

spec/services/groups/deploy_tokens/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Groups::DeployTokens::DestroyService do
+  it_behaves_like 'a deploy token deletion service' do
+    let_it_be(:entity) { create(:group) }
+    let_it_be(:deploy_token_class) { GroupDeployToken }
+    let_it_be(:deploy_token) { create(:deploy_token, :group, groups: [entity]) }
+  end
+end

spec/services/projects/deploy_tokens/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::DeployTokens::DestroyService do
+  it_behaves_like 'a deploy token deletion service' do
+    let_it_be(:entity) { create(:project) }
+    let_it_be(:deploy_token_class) { ProjectDeployToken }
+    let_it_be(:deploy_token) { create(:deploy_token, projects: [entity]) }
+  end
+end

spec/support/services/deploy_token_shared_examples.rb

@@ -58,3 +58,25 @@ RSpec.shared_examples 'a deploy token creation service' do
     end
   end
 end
+
+RSpec.shared_examples 'a deploy token deletion service' do
+  let(:user) { create(:user) }
+  let(:deploy_token_params) { { token_id: deploy_token.id } }
+
+  describe '#execute' do
+    subject { described_class.new(entity, user, deploy_token_params).execute }
+
+    it "destroys a token record and it's associated DeployToken" do
+      expect { subject }.to change { deploy_token_class.count }.by(-1)
+        .and change { DeployToken.count }.by(-1)
+    end
+
+    context 'invalid token id' do
+      let(:deploy_token_params) { { token_id: 9999 } }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+  end
+end