Add CRUD services for pipeline trigger tokens

This change adds centralized service classes for pipeline trigger token
CRUD in order to standardize behavior and reduce inconsistent behaviors
across different endpoints and uses the new CreateService.

Changelog: other

app/controllers/projects/triggers_controller.rb

 # frozen_string_literal: true
 
 class Projects::TriggersController < Projects::ApplicationController
-  before_action :authorize_admin_build!
+  before_action :authorize_manage_trigger_on_project!
   before_action :authorize_manage_trigger!, except: [:index, :create]
+
   before_action :authorize_admin_trigger!, only: [:edit, :update]
   before_action :trigger, only: [:edit, :update, :destroy]
 
@@ -16,12 +17,18 @@ def index
   end
 
   def create
-    @trigger = project.triggers.create(trigger_params.merge(owner: current_user))
+    response = ::Ci::PipelineTriggers::CreateService.new(
+      project: project,
+      user: current_user,
+      description: trigger_params[:description]
+    ).execute
+
+    @trigger = response.payload[:trigger]
 
-    if @trigger.valid?
-      flash[:notice] = _('Trigger was created successfully.')
+    if response.success?
+      flash[:notice] = _('Trigger token was created successfully.')
     else
-      flash[:alert] = _('You could not create a new trigger.')
+      flash[:alert] = response.message
     end
 
     redirect_to project_settings_ci_cd_path(@project, anchor: 'js-pipeline-triggers')
@@ -54,6 +61,10 @@ def authorize_manage_trigger!
     access_denied! unless can?(current_user, :manage_trigger, trigger)
   end
 
+  def authorize_manage_trigger_on_project!
+    access_denied! unless can?(current_user, :manage_trigger, project)
+  end
+
   def authorize_admin_trigger!
     access_denied! unless can?(current_user, :admin_trigger, trigger)
   end

app/graphql/mutations/ci/pipeline_trigger/create.rb

@@ -8,7 +8,7 @@ class Create < BaseMutation
 
         include FindsProject
 
-        authorize :admin_build
+        authorize :manage_trigger
 
         argument :project_path, GraphQL::Types::ID,
           required: true,
@@ -25,7 +25,10 @@ class Create < BaseMutation
         def resolve(project_path:, description:)
           project = authorized_find!(project_path)
 
-          trigger = project.triggers.create(owner: current_user, description: description)
+          response = ::Ci::PipelineTriggers::CreateService.new(project: project, user: current_user,
+            description: description).execute
+
+          trigger = response.payload[:trigger]
 
           {
             pipeline_trigger: trigger,

app/graphql/types/ci/pipeline_trigger_type.rb

@@ -8,7 +8,7 @@ class PipelineTriggerType < BaseObject
       present_using ::Ci::TriggerPresenter
       connection_type_class Types::CountableConnectionType
 
-      authorize :admin_build
+      authorize :manage_trigger
 
       field :can_access_project, GraphQL::Types::Boolean,
         null: false,

app/policies/ci/trigger_policy.rb

@@ -9,9 +9,7 @@ class TriggerPolicy < BasePolicy
     with_score 0
     condition(:is_owner) { @user && @subject.owner_id == @user.id }
 
-    rule { ~can?(:admin_build) }.prevent :admin_trigger
+    rule { ~can?(:manage_trigger) }.prevent :admin_trigger
     rule { is_owner }.enable :admin_trigger
-
-    rule { can?(:admin_build) }.enable :manage_trigger
   end
 end

app/policies/project_policy.rb

@@ -595,6 +595,8 @@ class ProjectPolicy < BasePolicy
     enable :read_import_error
   end
 
+  rule { can?(:admin_build) }.enable :manage_trigger
+
   rule { public_project & metrics_dashboard_allowed }.policy do
     enable :metrics_dashboard
   end

app/services/ci/pipeline_triggers/create_service.rb

+# frozen_string_literal: true
+
+module Ci
+  module PipelineTriggers
+    class CreateService
+      include Gitlab::Allowable
+
+      attr_reader :project, :current_user, :description
+
+      def initialize(project:, user:, description:)
+        @project = project
+        @current_user = user
+        @description = description
+      end
+
+      def execute
+        unless can?(current_user, :manage_trigger, project)
+          return ServiceResponse.error(
+            message: _('The current user is not authorized to create a pipeline trigger token'),
+            payload: { trigger: nil },
+            reason: :forbidden
+          )
+        end
+
+        trigger = project.triggers.create(**create_params)
+
+        if trigger.present? && trigger.persisted?
+          ServiceResponse.success(payload: { trigger: trigger })
+        elsif trigger.present? && trigger.errors.any?
+          ServiceResponse.error(
+            message: trigger.errors.to_json,
+            payload: { trigger: trigger },
+            reason: :validation_error
+          )
+        else
+          raise "Unexpected Ci::Trigger creation failure. Description: #{@description}"
+        end
+      end
+
+      private
+
+      def create_params
+        { description: description, owner: current_user }
+      end
+    end
+  end
+end

app/services/ci/pipeline_triggers/destroy_service.rb

+# frozen_string_literal: true
+
+module Ci
+  module PipelineTriggers
+    class DestroyService
+      include Gitlab::Allowable
+
+      attr_reader :project, :current_user, :description, :trigger
+
+      def initialize(user:, trigger:)
+        @current_user = user
+        @trigger = trigger
+      end
+
+      def execute
+        unless can?(current_user, :manage_trigger, trigger)
+          return ServiceResponse.error(
+            message: _('The current user is not authorized to manage the pipeline trigger token'),
+            reason: :forbidden
+          )
+        end
+
+        trigger.destroy
+
+        unless trigger.destroyed?
+          return ServiceResponse.error(
+            message: _('Attempted to destroy the pipeline trigger token but failed')
+          )
+        end
+
+        ServiceResponse.success
+      end
+    end
+  end
+end

app/services/ci/pipeline_triggers/update_service.rb

+# frozen_string_literal: true
+
+module Ci
+  module PipelineTriggers
+    class UpdateService
+      include Gitlab::Allowable
+
+      attr_reader :current_user, :description, :trigger
+
+      def initialize(user:, trigger:, description:)
+        @current_user = user
+        @description = description
+        @trigger = trigger
+      end
+
+      def execute
+        unless can?(current_user, :admin_trigger, trigger)
+          return ServiceResponse.error(
+            message: _('The current user is not authorized to update the pipeline trigger token'),
+            payload: { trigger: trigger },
+            reason: :forbidden
+          )
+        end
+
+        if trigger.update(**update_params)
+          ServiceResponse.success(payload: { trigger: trigger })
+        else
+          ServiceResponse.error(
+            message: _('Attempted to update the pipeline trigger token but failed'),
+            payload: { trigger: trigger }
+          )
+        end
+      end
+
+      private
+
+      def update_params
+        { description: description }
+      end
+    end
+  end
+end

app/services/environments/destroy_service.rb

@@ -13,7 +13,7 @@ def execute(environment)
 
       unless environment.destroyed?
         return ServiceResponse.error(
-          message: 'Attemped to destroy the environment but failed'
+          message: 'Attempted to destroy the environment but failed'
         )
       end
 

app/services/environments/stop_service.rb

@@ -20,7 +20,7 @@ def execute(environment)
 
       unless environment.saved_change_to_attribute?(:state)
         return ServiceResponse.error(
-          message: 'Attemped to stop the environment but failed to change the status',
+          message: 'Attempted to stop the environment but failed to change the status',
           payload: { environment: environment }
         )
       end

lib/api/ci/triggers.rb

@@ -56,7 +56,7 @@ class Triggers < ::API::Base
           end
         end
 
-        desc 'Get triggers list' do
+        desc 'Get trigger tokens list' do
           success code: 200, model: Entities::Trigger
           failure [
             { code: 401, message: 'Unauthorized' },
@@ -79,7 +79,7 @@ class Triggers < ::API::Base
         end
         # rubocop: enable CodeReuse/ActiveRecord
 
-        desc 'Get specific trigger of a project' do
+        desc 'Get specific trigger token of a project' do
           success code: 200, model: Entities::Trigger
           failure [
             { code: 401, message: 'Unauthorized' },
@@ -88,7 +88,7 @@ class Triggers < ::API::Base
           ]
         end
         params do
-          requires :trigger_id, type: Integer, desc: 'The trigger ID', documentation: { example: 10 }
+          requires :trigger_id, type: Integer, desc: 'The trigger token ID', documentation: { example: 10 }
         end
         get ':id/triggers/:trigger_id' do
           authenticate!
@@ -100,7 +100,7 @@ class Triggers < ::API::Base
           present trigger, with: Entities::Trigger, current_user: current_user
         end
 
-        desc 'Create a trigger' do
+        desc 'Create a trigger token' do
           success code: 201, model: Entities::Trigger
           failure [
             { code: 400, message: 'Bad request' },
@@ -110,20 +110,26 @@ class Triggers < ::API::Base
           ]
         end
         params do
-          requires :description, type: String, desc: 'The trigger description',
-                                 documentation: { example: 'my trigger description' }
+          requires :description, type: String, desc: 'The trigger token description',
+                                 documentation: { example: 'my trigger token description' }
         end
         post ':id/triggers' do
           authenticate!
-          authorize! :admin_build, user_project
-
-          trigger = user_project.triggers.create(
-            declared_params(include_missing: false).merge(owner: current_user))
-
-          if trigger.valid?
-            present trigger, with: Entities::Trigger, current_user: current_user
+          authorize! :manage_trigger, user_project
+
+          response =
+            ::Ci::PipelineTriggers::CreateService.new(
+              project: user_project,
+              user: current_user,
+              description: declared_params(include_missing: false)[:description]
+            ).execute
+
+          if response.success?
+            present response.payload[:trigger], with: Entities::Trigger, current_user: current_user
+          elsif response.reason == :forbidden
+            forbidden!(response.message)
           else
-            render_validation_error!(trigger)
+            bad_request!(response.message)
           end
         end
 

locale/gitlab.pot

@@ -6943,6 +6943,12 @@ msgstr ""
 msgid "Attempted sign in to %{host} using an incorrect verification code"
 msgstr ""
+msgid "Attempted to destroy the pipeline trigger token but failed"
+msgstr ""
+
+msgid "Attempted to update the pipeline trigger token but failed"
+msgstr ""
+
 msgid "Audit Events"
 msgstr ""
@@ -49444,12 +49450,18 @@ msgstr ""
 msgid "The current user is not authorized to access the job log."
 msgstr ""
+msgid "The current user is not authorized to create a pipeline trigger token"
+msgstr ""
+
 msgid "The current user is not authorized to create the pipeline schedule"
 msgstr ""
 msgid "The current user is not authorized to create the pipeline schedule variables"
 msgstr ""
+msgid "The current user is not authorized to manage the pipeline trigger token"
+msgstr ""
+
 msgid "The current user is not authorized to set pipeline schedule variables"
 msgstr ""
@@ -49459,6 +49471,9 @@ msgstr ""
 msgid "The current user is not authorized to update the pipeline schedule variables"
 msgstr ""
+msgid "The current user is not authorized to update the pipeline trigger token"
+msgstr ""
+
 msgid "The data in this pipeline is too old to be rendered as a graph. Please check the Jobs tab to access historical data."
 msgstr ""
@@ -52232,13 +52247,13 @@ msgstr ""
 msgid "Trigger repository check"
 msgstr ""
-msgid "Trigger token:"
+msgid "Trigger token was created successfully."
 msgstr ""
-msgid "Trigger variables"
+msgid "Trigger token:"
 msgstr ""
-msgid "Trigger was created successfully."
+msgid "Trigger variables"
 msgstr ""
 msgid "Trigger was successfully updated."
@@ -56788,9 +56803,6 @@ msgstr ""
 msgid "You can’t edit files directly in this project. Fork this project and submit a merge request with your changes."
 msgstr ""
-msgid "You could not create a new trigger."
-msgstr ""
-
 msgid "You currently have no custom domains."
 msgstr ""

spec/features/triggers_spec.rb

@@ -40,11 +40,35 @@
         click_button 'Create pipeline trigger token'
 
         aggregate_failures 'display creation notice and trigger is created' do
-          expect(page.find('[data-testid="alert-info"]')).to have_content 'Trigger was created successfully.'
+          expect(page.find('[data-testid="alert-info"]')).to have_content 'Trigger token was created successfully.'
           expect(page.find('.triggers-list')).to have_content 'trigger desc'
           expect(page.find('.triggers-list .trigger-owner')).to have_content user.name
         end
       end
+
+      context 'when trigger is not saved' do
+        before do
+          allow_next_instance_of(Ci::PipelineTriggers::CreateService) do |instance|
+            allow(instance).to receive(:execute).and_return(
+              ServiceResponse.error(
+                message: 'Validation error',
+                payload: { trigger: { description: ['is missing'] } },
+                reason: :validation_error
+              )
+            )
+          end
+        end
+
+        it 'trigger.errors has an error' do
+          click_button 'Add new token'
+          fill_in 'trigger_description', with: 'trigger desc'
+          click_button 'Create pipeline trigger token'
+
+          expect(page.find('.flash-container')).to(
+            have_content("Validation error")
+          )
+        end
+      end
     end
 
     describe 'edit trigger workflow' do

spec/graphql/mutations/environments/delete_spec.rb

@@ -56,7 +56,7 @@
       end
 
       it 'returns errors' do
-        expect(subject[:errors]).to include("Attemped to destroy the environment but failed")
+        expect(subject[:errors]).to include("Attempted to destroy the environment but failed")
       end
     end
 

spec/graphql/mutations/environments/stop_spec.rb

@@ -42,7 +42,7 @@
         expect(subject)
           .to eq({
             environment: environment,
-            errors: ['Attemped to stop the environment but failed to change the status']
+            errors: ['Attempted to stop the environment but failed to change the status']
           })
       end
     end

spec/requests/api/ci/triggers_spec.rb

@@ -251,9 +251,49 @@
 
       context 'without required parameters' do
         it 'does not create trigger' do
-          post api("/projects/#{project.id}/triggers", user)
+          expect do
+            post api("/projects/#{project.id}/triggers", user)
+          end.not_to change { project.triggers.count }
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+      end
+
+      context 'when the CreateService returns a permissions error' do
+        before do
+          failure_response = instance_double(ServiceResponse, success?: false, reason: :forbidden, message: "Permissions error message")
+
+          allow_next_instance_of(::Ci::PipelineTriggers::CreateService) do |instance|
+            allow(instance).to receive(:execute)
+                      .and_return(failure_response)
+          end
+        end
+
+        it 'returns forbidden' do
+          post api("/projects/#{project.id}/triggers", user),
+            params: { description: 'trigger' }
+
+          expect(response).to have_gitlab_http_status(:forbidden)
+          expect(json_response['message']).to eq('403 Forbidden - Permissions error message')
+        end
+      end
+
+      context 'when trigger fails to save' do
+        before do
+          failure_response = instance_double(ServiceResponse, success?: false, reason: :validation_error, message: "Unexpected Ci::Trigger creation failure")
+
+          allow_next_instance_of(::Ci::PipelineTriggers::CreateService) do |instance|
+            allow(instance).to receive(:execute)
+                      .and_return(failure_response)
+          end
+        end
+
+        it 'returns bad request' do
+          post api("/projects/#{project.id}/triggers", user),
+            params: { description: 'trigger' }
 
           expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['message']).to eq('400 Bad request - Unexpected Ci::Trigger creation failure')
         end
       end
     end

spec/services/ci/pipeline_triggers/create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::PipelineTriggers::CreateService, feature_category: :continuous_integration do
+  let_it_be(:developer) { create(:user) }
+  let_it_be_with_reload(:user) { create(:user) }
+  let_it_be_with_reload(:project) { create(:project, :public) }
+
+  subject(:service) { described_class.new(project: project, user: user, description: description) }
+
+  before_all do
+    project.add_maintainer(user)
+    project.add_developer(developer)
+  end
+
+  describe "execute" do
+    context 'when user does not have permission' do
+      subject(:service) { described_class.new(project: project, user: developer, description: {}) }
+
+      it 'returns ServiceResponse.error' do
+        response = service.execute
+
+        expect(response).to be_a(ServiceResponse)
+        expect(response.error?).to be(true)
+
+        error_message = _('The current user is not authorized to create a pipeline trigger token')
+        expect(response.message).to eq(error_message)
+        expect(response.errors).to match_array([error_message])
+      end
+    end
+
+    context 'when user has permission' do
+      let(:description) { "My snazzy pipeline trigger token" }
+
+      it 'creates a pipeline trigger token' do
+        response = service.execute
+
+        expect(response).to be_a(ServiceResponse)
+        expect(response.success?).to be(true)
+        trigger = response.payload[:trigger]
+        expect(trigger).to be_a(Ci::Trigger)
+        expect(trigger).to be_persisted
+        expect(trigger.description).to eq(description)
+        expect(trigger.owner).to eq(user)
+        expect(trigger.project).to eq(project)
+      end
+
+      context 'when create fails' do
+        before do
+          allow(project.triggers).to receive(:create).and_return(nil)
+        end
+
+        it 'raises a RuntimeError' do
+          expect { service.execute }.to raise_error(RuntimeError, /Unexpected Ci::Trigger creation failure/)
+        end
+      end
+
+      context 'when trigger exists but has errors' do
+        before do
+          trigger_with_errors = instance_double('Ci::Trigger', present?: true, persisted?: false,
+            errors: ['Validation error'])
+          allow(project.triggers).to receive(:create).and_return(trigger_with_errors)
+        end
+
+        it 'returns ServiceResponse.error' do
+          response = service.execute
+
+          expect(response).to be_a(ServiceResponse)
+          expect(response.error?).to be(true)
+          expect(response.message).to eq('["Validation error"]')
+          expect(response.reason).to eq(:validation_error)
+        end
+      end
+    end
+  end
+end

spec/services/ci/pipeline_triggers/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::PipelineTriggers::DestroyService, feature_category: :continuous_integration do
+  describe '#execute' do
+    let_it_be(:developer) { create(:user) }
+    let_it_be_with_reload(:user) { create(:user) }
+    let_it_be_with_reload(:project) { create(:project) }
+
+    let(:pipeline_trigger) { create(:ci_trigger, project: project, owner: user) }
+
+    subject(:service) { described_class.new(user: user, trigger: pipeline_trigger) }
+
+    before_all do
+      project.add_maintainer(user)
+      project.add_developer(developer)
+    end
+
+    context 'when user does not have permission' do
+      subject(:service) { described_class.new(user: developer, trigger: pipeline_trigger) }
+
+      it 'returns an error' do
+        response = service.execute
+
+        expect(response).to be_a(ServiceResponse)
+        expect(response.error?).to be(true)
+
+        error_message = _('The current user is not authorized to manage the pipeline trigger token')
+        expect(response.message).to eq(error_message)
+        expect(response.errors).to match_array([error_message])
+      end
+    end
+
+    context 'when user has permission' do
+      it 'deletes the pipeline trigger token' do
+        response = service.execute
+
+        expect(response).to be_a(ServiceResponse)
+        expect(response.success?).to be(true)
+        expect(Ci::Trigger.find_by(id: pipeline_trigger.id)).to be_nil
+      end
+
+      context 'when destroy fails' do
+        before do
+          allow(pipeline_trigger).to receive(:destroy).and_return(false)
+        end
+
+        it 'returns ServiceResponse.error' do
+          result = service.execute
+
+          expect(result).to be_a(ServiceResponse)
+          expect(result.error?).to be(true)
+          expect(result.message).to eq('Attempted to destroy the pipeline trigger token but failed')
+        end
+      end
+    end
+  end
+end

spec/services/ci/pipeline_triggers/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::PipelineTriggers::UpdateService, feature_category: :continuous_integration do
+  let_it_be_with_reload(:user) { create(:user) }
+  let_it_be_with_reload(:project) { create(:project, :public) }
+  let_it_be_with_reload(:pipeline_trigger) do
+    create(:ci_trigger, project: project, owner: user, description: "Old description")
+  end
+
+  let_it_be(:another_maintainer) { create(:user) }
+
+  subject(:service) { described_class.new(user: user, trigger: pipeline_trigger, description: description) }
+
+  before_all do
+    project.add_maintainer(user)
+    project.add_maintainer(another_maintainer)
+
+    pipeline_trigger.reload
+  end
+
+  describe "execute" do
+    context 'when user does not have permission' do
+      subject(:service) { described_class.new(trigger: pipeline_trigger, user: another_maintainer, description: {}) }
+
+      it 'returns ServiceResponse.error' do
+        response = service.execute
+
+        expect(response).to be_a(ServiceResponse)
+        expect(response.error?).to be(true)
+
+        error_message = _('The current user is not authorized to update the pipeline trigger token')
+        expect(response.message).to eq(error_message)
+        expect(response.errors).to match_array([error_message])
+      end
+    end
+
+    context 'when user has permission' do
+      let(:description) { 'My updated description' }
+
+      it 'updates database values with passed description param' do
+        expect { service.execute }
+          .to change { pipeline_trigger.reload.description }.from('Old description').to('My updated description')
+      end
+
+      it 'returns ServiceResponse.success' do
+        response = service.execute
+
+        expect(response).to be_a(ServiceResponse)
+        expect(response.success?).to be(true)
+        expect(response.payload[:trigger].description).to eq('My updated description')
+      end
+
+      context 'when update fails' do
+        before do
+          allow(pipeline_trigger).to receive(:update).and_return(false)
+        end
+
+        it 'returns ServiceResponse.error' do
+          response = service.execute
+
+          expect(response).to be_a(ServiceResponse)
+          expect(response.error?).to be(true)
+          expect(response.message).to eq('Attempted to update the pipeline trigger token but failed')
+        end
+      end
+    end
+  end
+end

spec/services/environments/destroy_service_spec.rb

@@ -43,7 +43,7 @@
       end
 
       it 'returns errors' do
-        expect(subject.message).to include("Attemped to destroy the environment but failed")
+        expect(subject.message).to include("Attempted to destroy the environment but failed")
       end
     end
   end