Merge branch 'trigger-ondemand-dast-scan-using-existing-ci-service-218685' into 'master'

Move DAST scan service to use existing CI service

See merge request gitlab-org/gitlab!34603

app/models/ci/pipeline_enums.rb

@@ -31,7 +31,7 @@ module Ci
         merge_request_event: 10,
         external_pull_request_event: 11,
         parent_pipeline: 12,
-        ondemand_scan: 13
+        ondemand_dast_scan: 13
       }
     end
 

ee/app/graphql/mutations/pipelines/run_dast_scan.rb

@@ -33,11 +33,14 @@ module Mutations
         project = authorized_find!(full_path: project_path)
         raise_resource_not_available_error! unless Feature.enabled?(:security_on_demand_scans_feature_flag, project)
 
-        service = Ci::RunDastScanService.new(project: project, user: current_user)
-        pipeline = service.execute(branch: branch, target_url: target_url)
-        success_response(project: project, pipeline: pipeline)
-      rescue Ci::RunDastScanService::RunError => err
-        error_response(err)
+        service = Ci::RunDastScanService.new(project, current_user)
+        result = service.execute(branch: branch, target_url: target_url)
+
+        if result.success?
+          success_response(project: project, pipeline: result.payload)
+        else
+          error_response(result.message)
+        end
       end
 
       private
@@ -57,8 +60,8 @@ module Mutations
         }
       end
 
-      def error_response(err)
-        { errors: err.full_messages }
+      def error_response(message)
+        { errors: [message] }
       end
     end
   end

ee/app/services/ci/run_dast_scan_service.rb

 # frozen_string_literal: true
 
 module Ci
-  class RunDastScanService
-    DEFAULT_SHA_FOR_PROJECTS_WITHOUT_COMMITS = :placeholder
+  class RunDastScanService < BaseService
+    DAST_CI_TEMPLATE = 'lib/gitlab/ci/templates/Security/DAST.gitlab-ci.yml'.freeze
 
-    class RunError < StandardError
-      attr_reader :full_messages
-
-      def initialize(msg, full_messages = [])
-        @full_messages = full_messages.unshift(msg)
-        super(msg)
-      end
+    def self.ci_template
+      @ci_template ||= File.open(DAST_CI_TEMPLATE, 'r') { |f| YAML.safe_load(f.read) }.tap do |template|
+        template['stages'] = ['dast']
+        template['dast'].delete('rules')
       end
-
-    def initialize(project:, user:)
-      @project = project
-      @user = user
     end
 
     def execute(branch:, target_url:)
-      unless allowed?
-        msg = Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR
-        raise RunError.new(msg)
-      end
+      return ServiceResponse.error(message: 'Insufficient permissions') unless allowed?
+
+      service = Ci::CreatePipelineService.new(project, current_user, ref: branch)
+      pipeline = service.execute(:ondemand_dast_scan, content: ci_yaml(target_url))
 
-      ActiveRecord::Base.transaction do
-        pipeline = create_pipeline!(branch)
-        stage = create_stage!(pipeline)
-        build = create_build!(pipeline, stage, branch, target_url)
-        enqueue!(build)
-        pipeline
+      if pipeline.created_successfully?
+        ServiceResponse.success(payload: pipeline)
+      else
+        ServiceResponse.error(message: pipeline.full_error_messages)
       end
     end
 
     private
 
-    attr_reader :project, :user
-
     def allowed?
-      Ability.allowed?(user, :create_pipeline, project)
-    end
-
-    def create_pipeline!(branch)
-      reraise!(msg: 'Pipeline could not be created') do
-        Ci::Pipeline.create!(
-          project: project,
-          ref: branch,
-          sha: project.repository.commit&.id || DEFAULT_SHA_FOR_PROJECTS_WITHOUT_COMMITS,
-          source: :ondemand_scan,
-          user: user
-        )
-      end
-    end
-
-    def create_stage!(pipeline)
-      reraise!(msg: 'Stage could not be created') do
-        Ci::Stage.create!(
-          name: 'dast',
-          pipeline: pipeline,
-          project: project
-        )
-      end
-    end
-
-    def create_build!(pipeline, stage, branch, target_url)
-      reraise!(msg: 'Build could not be created') do
-        Ci::Build.create!(
-          name: 'DAST Scan',
-          pipeline: pipeline,
-          project: project,
-          ref: branch,
-          scheduling_type: :stage,
-          stage: stage.name,
-          options: options,
-          yaml_variables: yaml_variables(target_url)
-        )
-      end
-    end
-
-    def enqueue!(build)
-      reraise!(msg: 'Build could not be enqueued') do
-        build.enqueue!
-      end
-    end
-
-    def reraise!(msg:)
-      yield
-    rescue ActiveRecord::RecordInvalid => err
-      Gitlab::ErrorTracking.track_exception(err)
-      raise RunError.new(msg, err.record.errors.full_messages)
-    rescue => err
-      Gitlab::ErrorTracking.track_exception(err)
-      raise RunError.new(msg)
-    end
-
-    def options
-      {
-        image: {
-          name: '$SECURE_ANALYZERS_PREFIX/dast:$DAST_VERSION'
-        },
-        artifacts: {
-          reports: {
-            dast: [
-              'gl-dast-report.json'
-            ]
-          }
-        },
-        script: [
-          'export DAST_WEBSITE=${DAST_WEBSITE:-$(cat environment_url.txt)}',
-          '/analyze'
-        ]
-      }
+      Ability.allowed?(current_user, :run_ondemand_dast_scan, project)
     end
 
-    def yaml_variables(target_url)
-      [
-        {
-          key: 'DAST_VERSION',
-          value: '1',
-          public: true
-        },
-        {
-          key: 'SECURE_ANALYZERS_PREFIX',
-          value: 'registry.gitlab.com/gitlab-org/security-products/analyzers',
-          public: true
-        },
-        {
-          key: 'DAST_WEBSITE',
-          value: target_url,
-          public: true
-        },
-        {
-          key: 'GIT_STRATEGY',
-          value: 'none',
-          public: true
-        }
-      ]
+    def ci_yaml(target_url)
+      self.class.ci_template.deep_merge(
+        'variables' => { 'DAST_WEBSITE' => target_url }
+      ).to_yaml
     end
   end
 end

ee/spec/graphql/mutations/pipelines/run_dast_scan_spec.rb

@@ -4,11 +4,11 @@ require 'spec_helper'
 
 RSpec.describe Mutations::Pipelines::RunDastScan do
   let(:group) { create(:group) }
-  let(:project) { create(:project, group: group) }
+  let(:project) { create(:project, :repository, group: group) }
   let(:user) { create(:user) }
   let(:project_path) { project.full_path }
   let(:target_url) { FFaker::Internet.uri(:https) }
-  let(:branch) { SecureRandom.hex }
+  let(:branch) { project.default_branch }
   let(:scan_type) { Types::DastScanTypeEnum.enum[:passive] }
 
   subject(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }

ee/spec/requests/api/graphql/mutations/pipelines/run_dast_scan_spec.rb

@@ -5,11 +5,11 @@ require 'spec_helper'
 RSpec.describe 'Running a DAST Scan' do
   include GraphqlHelpers
 
-  let(:project) { create(:project) }
+  let(:project) { create(:project, :repository, creator: current_user) }
   let(:current_user) { create(:user) }
   let(:project_path) { project.full_path }
   let(:target_url) { FFaker::Internet.uri(:https) }
-  let(:branch) { SecureRandom.hex }
+  let(:branch) { project.default_branch }
   let(:scan_type) { Types::DastScanTypeEnum.enum[:passive] }
 
   let(:mutation) do
@@ -58,36 +58,13 @@ RSpec.describe 'Running a DAST Scan' do
         expect(mutation_response['pipelineUrl']).to eq(expected_url)
       end
 
-      context 'when the pipeline could not be created' do
+      context 'when pipeline creation fails' do
         before do
-          allow(Ci::Pipeline).to receive(:create!).and_raise(StandardError)
+          allow_any_instance_of(Ci::Pipeline).to receive(:created_successfully?).and_return(false)
+          allow_any_instance_of(Ci::Pipeline).to receive(:full_error_messages).and_return('error message')
         end
 
-        it_behaves_like 'a mutation that returns errors in the response', errors: ['Pipeline could not be created']
-      end
-
-      context 'when the stage could not be created' do
-        before do
-          allow(Ci::Stage).to receive(:create!).and_raise(StandardError)
-        end
-
-        it_behaves_like 'a mutation that returns errors in the response', errors: ['Stage could not be created']
-      end
-
-      context 'when the build could not be created' do
-        before do
-          allow(Ci::Build).to receive(:create!).and_raise(StandardError)
-        end
-
-        it_behaves_like 'a mutation that returns errors in the response', errors: ['Build could not be created']
-      end
-
-      context 'when the build could not be enqueued' do
-        before do
-          allow_any_instance_of(Ci::Build).to receive(:enqueue!).and_raise(StandardError)
-        end
-
-        it_behaves_like 'a mutation that returns errors in the response', errors: ['Build could not be enqueued']
+        it_behaves_like 'a mutation that returns errors in the response', errors: ['error message']
       end
     end
   end

ee/spec/services/ci/run_dast_scan_service_spec.rb

@@ -3,19 +3,39 @@
 require 'spec_helper'
 
 RSpec.describe Ci::RunDastScanService do
-  let(:project) { create(:project) }
-  let(:branch) { SecureRandom.hex }
-  let(:target_url) { FFaker::Internet.uri(:http) }
   let(:user) { create(:user) }
+  let(:project) { create(:project, :repository, creator: user) }
+  let(:branch) { project.default_branch }
+  let(:target_url) { FFaker::Internet.uri(:http) }
+
+  describe '.ci_template' do
+    it 'builds a hash' do
+      expect(described_class.ci_template).to be_a(Hash)
+    end
+
+    it 'has only one stage' do
+      expect(described_class.ci_template['stages']).to eq(['dast'])
+    end
+
+    it 'has has no rules' do
+      expect(described_class.ci_template['dast']['rules']).to be_nil
+    end
+  end
 
   describe '#execute' do
-    subject { described_class.new(project: project, user: user).execute(branch: branch, target_url: target_url) }
+    subject { described_class.new(project, user).execute(branch: branch, target_url: target_url) }
+
+    let(:status) { subject.status }
+    let(:pipeline) { subject.payload }
+    let(:message) { subject.message }
 
     context 'when the user does not have permission to run a dast scan' do
-      it 'raises an exeception with #full_messages populated' do
-        expect { subject }.to raise_error(Ci::RunDastScanService::RunError) do |error|
-          expect(error.full_messages[0]).to include('you don\'t have permission to perform this action')
+      it 'returns an error status' do
+        expect(status).to eq(:error)
       end
+
+      it 'populates message' do
+        expect(message).to eq('Insufficient permissions')
       end
     end
 
@@ -24,8 +44,12 @@ RSpec.describe Ci::RunDastScanService do
         project.add_developer(user)
       end
 
+      it 'returns a success status' do
+        expect(status).to eq(:success)
+      end
+
       it 'returns a pipeline' do
-        expect(subject).to be_a(Ci::Pipeline)
+        expect(pipeline).to be_a(Ci::Pipeline)
       end
 
       it 'creates a pipeline' do
@@ -33,11 +57,11 @@ RSpec.describe Ci::RunDastScanService do
       end
 
       it 'sets the pipeline ref to the branch' do
-        expect(subject.ref).to eq(branch)
+        expect(pipeline.ref).to eq(branch)
       end
 
       it 'sets the source to indicate an ondemand scan' do
-        expect(subject.source).to eq('ondemand_scan')
+        expect(pipeline.source).to eq('ondemand_dast_scan')
       end
 
       it 'creates a stage' do
@@ -49,18 +73,19 @@ RSpec.describe Ci::RunDastScanService do
       end
 
       it 'sets the build name to indicate a DAST scan' do
-        build = subject.builds.first
-        expect(build.name).to eq('DAST Scan')
+        build = pipeline.builds.first
+        expect(build.name).to eq('dast')
       end
 
       it 'creates a build with appropriate options' do
-        build = subject.builds.first
+        build = pipeline.builds.first
         expected_options = {
           'image' => {
             'name' => '$SECURE_ANALYZERS_PREFIX/dast:$DAST_VERSION'
           },
           'script' => [
             'export DAST_WEBSITE=${DAST_WEBSITE:-$(cat environment_url.txt)}',
+            'if [ -z "$DAST_WEBSITE$DAST_API_SPECIFICATION" ]; then echo "Either DAST_WEBSITE or DAST_API_SPECIFICATION must be set. See https://docs.gitlab.com/ee/user/application_security/dast/#configuration for more details." && exit 1; fi',
             '/analyze'
           ],
           'artifacts' => {
@@ -73,7 +98,7 @@ RSpec.describe Ci::RunDastScanService do
       end
 
       it 'creates a build with appropriate variables' do
-        build = subject.builds.first
+        build = pipeline.builds.first
         expected_variables = [
           {
             'key' => 'DAST_VERSION',
@@ -97,90 +122,24 @@ RSpec.describe Ci::RunDastScanService do
       end
 
       it 'enqueues a build' do
-        build = subject.builds.first
+        build = pipeline.builds.first
         expect(build.queued_at).not_to be_nil
       end
 
-      context 'when the repository has no commits' do
-        it 'uses a placeholder' do
-          expect(subject.sha).to eq('placeholder')
-        end
-      end
-
-      context 'when the pipeline could not be created' do
+      context 'when the pipeline fails to save' do
         before do
-          allow(Ci::Pipeline).to receive(:create!).and_raise(StandardError)
-        end
-
-        it 'raises an exeception with #full_messages populated' do
-          expect { subject }.to raise_error(Ci::RunDastScanService::RunError) do |error|
-            expect(error.full_messages).to include('Pipeline could not be created')
-          end
-        end
+          allow_any_instance_of(Ci::Pipeline).to receive(:created_successfully?).and_return(false)
+          allow_any_instance_of(Ci::Pipeline).to receive(:full_error_messages).and_return(full_error_messages)
         end
 
-      context 'when the stage could not be created' do
-        before do
-          allow(Ci::Stage).to receive(:create!).and_raise(StandardError)
-        end
+        let(:full_error_messages) { SecureRandom.hex }
 
-        it 'raises an exeception with #full_messages populated' do
-          expect { subject }.to raise_error(Ci::RunDastScanService::RunError) do |error|
-            expect(error.full_messages).to include('Stage could not be created')
-          end
+        it 'returns an error status' do
+          expect(status).to eq(:error)
         end
 
-        it 'does not create a pipeline' do
-          expect { subject rescue nil }.not_to change(Ci::Pipeline, :count)
-        end
-      end
-
-      context 'when the build could not be created' do
-        before do
-          allow(Ci::Build).to receive(:create!).and_raise(StandardError)
-        end
-
-        it 'raises an exeception with #full_messages populated' do
-          expect { subject }.to raise_error(Ci::RunDastScanService::RunError) do |error|
-            expect(error.full_messages).to include('Build could not be created')
-          end
-        end
-
-        it 'does not create a stage' do
-          expect { subject rescue nil }.not_to change(Ci::Pipeline, :count)
-        end
-      end
-
-      context 'when the build could not be enqueued' do
-        before do
-          allow_any_instance_of(Ci::Build).to receive(:enqueue!).and_raise(StandardError)
-        end
-
-        it 'raises an exeception with #full_messages populated' do
-          expect { subject }.to raise_error(Ci::RunDastScanService::RunError) do |error|
-            expect(error.full_messages).to include('Build could not be enqueued')
-          end
-        end
-
-        it 'does not create a build' do
-          expect { subject rescue nil }.not_to change(Ci::Pipeline, :count)
-        end
-      end
-
-      context 'when a validation error is raised' do
-        before do
-          klass = Ci::Pipeline
-          allow(klass).to receive(:create!).and_raise(
-            ActiveRecord::RecordInvalid, klass.new.tap do |pl|
-              pl.errors.add(:sha, 'can\'t be blank')
-            end
-          )
-        end
-
-        it 'raises an exeception with #full_messages populated' do
-          expect { subject }.to raise_error(Ci::RunDastScanService::RunError) do |error|
-            expect(error.full_messages).to include('Sha can\'t be blank')
-          end
+        it 'populates message' do
+          expect(message).to eq(full_error_messages)
         end
       end
     end