Merge branch '299215-extract-on-demand-dast-variables-creation-to-separate-service' into 'master'

Extract logic to prepare DAST job variables into separate service

See merge request gitlab-org/gitlab!54237

ee/app/services/ci/dast_scan_ci_configuration_service.rb

+# frozen_string_literal: true
+
+module Ci
+  module DastScanCiConfigurationService
+    ENV_MAPPING = {
+      spider_timeout: 'DAST_SPIDER_MINS',
+      target_timeout: 'DAST_TARGET_AVAILABILITY_TIMEOUT',
+      target_url: 'DAST_WEBSITE',
+      use_ajax_spider: 'DAST_USE_AJAX_SPIDER',
+      show_debug_messages: 'DAST_DEBUG',
+      full_scan_enabled: 'DAST_FULL_SCAN_ENABLED'
+    }.freeze
+
+    def self.execute(args)
+      variables = args.slice(*ENV_MAPPING.keys).compact.to_h do |key, val|
+        [ENV_MAPPING[key], to_env_value(val)]
+      end
+
+      {
+        'stages' => ['dast'],
+        'include' => [{ 'template' => 'DAST-On-Demand-Scan.gitlab-ci.yml' }],
+        'variables' => variables
+      }.to_yaml
+    end
+
+    def self.bool?(value)
+      !!value == value
+    end
+    private_class_method :bool?
+
+    def self.to_env_value(value)
+      bool?(value) ? value.to_s : value
+    end
+    private_class_method :to_env_value
+  end
+end

ee/app/services/ci/run_dast_scan_service.rb

@@ -2,28 +2,6 @@
 
 module Ci
   class RunDastScanService < BaseService
-    ENV_MAPPING = {
-      spider_timeout: 'DAST_SPIDER_MINS',
-      target_timeout: 'DAST_TARGET_AVAILABILITY_TIMEOUT',
-      target_url: 'DAST_WEBSITE',
-      use_ajax_spider: 'DAST_USE_AJAX_SPIDER',
-      show_debug_messages: 'DAST_DEBUG',
-      full_scan_enabled: 'DAST_FULL_SCAN_ENABLED'
-    }.freeze
-
-    def self.ci_template
-      @ci_template ||= YAML.safe_load(ci_template_raw)
-    end
-
-    def self.ci_template_raw
-      <<~YAML
-        stages:
-          - dast
-        include:
-          - template: DAST-On-Demand-Scan.gitlab-ci.yml
-      YAML
-    end
-
     def execute(branch:, **args)
       return ServiceResponse.error(message: 'Insufficient permissions') unless allowed?
 
@@ -44,16 +22,7 @@ module Ci
     end
 
     def ci_yaml(args)
-      variables = args.each_with_object({}) do |(key, val), hash|
-        next if val.nil? || !ENV_MAPPING[key]
-
-        hash[ENV_MAPPING[key]] = !!val == val ? val.to_s : val
-        hash
-      end
-
-      self.class.ci_template.deep_merge(
-        'variables' => variables
-      ).to_yaml
+      Ci::DastScanCiConfigurationService.execute(args)
     end
   end
 end

ee/app/services/dast_on_demand_scans/create_service.rb

@@ -4,7 +4,6 @@ module DastOnDemandScans
   class CreateService < BaseContainerService
     def execute
       return ServiceResponse.error(message: 'Insufficient permissions') unless allowed?
-      return ServiceResponse.error(message: 'Cannot run active scan against unvalidated target') unless active_scan_allowed?
 
       create_pipeline
     rescue KeyError => err
@@ -17,49 +16,6 @@ module DastOnDemandScans
       container.feature_available?(:security_on_demand_scans)
     end
 
-    def active_scan_allowed?
-      return true unless dast_scanner_profile&.full_scan_enabled?
-
-      dast_site_validation = DastSiteValidationsFinder.new(
-        project_id: container.id,
-        state: :passed,
-        url_base: url_base
-      ).execute.first
-
-      dast_site_validation.present?
-    end
-
-    def dast_site
-      @dast_site ||= params.fetch(:dast_site_profile).dast_site
-    end
-
-    def dast_scanner_profile
-      @dast_scanner_profile ||= params[:dast_scanner_profile]
-    end
-
-    def url_base
-      @url_base ||= DastSiteValidation.get_normalized_url_base(dast_site.url)
-    end
-
-    def default_config
-      {
-        branch: container.default_branch,
-        target_url: dast_site.url
-      }
-    end
-
-    def scanner_profile_config
-      return {} unless dast_scanner_profile
-
-      {
-        spider_timeout: dast_scanner_profile.spider_timeout,
-        target_timeout: dast_scanner_profile.target_timeout,
-        full_scan_enabled: dast_scanner_profile.full_scan_enabled?,
-        use_ajax_spider: dast_scanner_profile.use_ajax_spider,
-        show_debug_messages: dast_scanner_profile.show_debug_messages
-      }
-    end
-
     def success_response(pipeline)
       pipeline_url = Rails.application.routes.url_helpers.project_pipeline_url(
         container,
@@ -75,8 +31,11 @@ module DastOnDemandScans
     end
 
     def create_pipeline
-      params = default_config.merge(scanner_profile_config)
-      result = ::Ci::RunDastScanService.new(container, current_user).execute(**params)
+      params_result = DastOnDemandScans::ParamsCreateService.new(container: container, current_user: current_user, params: params).execute
+
+      return params_result unless params_result.success?
+
+      result = ::Ci::RunDastScanService.new(container, current_user).execute(**params_result.payload)
 
       return success_response(result.payload) if result.success?
 

ee/app/services/dast_on_demand_scans/params_create_service.rb

+# frozen_string_literal: true
+
+module DastOnDemandScans
+  class ParamsCreateService < BaseContainerService
+    def execute
+      return ServiceResponse.error(message: 'Site Profile was not provided') unless dast_site.present?
+      return ServiceResponse.error(message: 'Cannot run active scan against unvalidated target') unless active_scan_allowed?
+
+      ServiceResponse.success(
+        payload: default_config.merge(scanner_profile_config)
+      )
+    end
+
+    private
+
+    def active_scan_allowed?
+      return true unless dast_scanner_profile&.full_scan_enabled?
+
+      DastSiteValidationsFinder.new(
+        project_id: container.id,
+        state: :passed,
+        url_base: url_base
+      ).execute.present?
+    end
+
+    def dast_site
+      @dast_site ||= params[:dast_site_profile]&.dast_site
+    end
+
+    def dast_scanner_profile
+      @dast_scanner_profile ||= params[:dast_scanner_profile]
+    end
+
+    def url_base
+      @url_base ||= DastSiteValidation.get_normalized_url_base(dast_site&.url)
+    end
+
+    def default_config
+      {
+        branch: container.default_branch,
+        target_url: dast_site&.url
+      }
+    end
+
+    def scanner_profile_config
+      return {} unless dast_scanner_profile
+
+      {
+        spider_timeout: dast_scanner_profile.spider_timeout,
+        target_timeout: dast_scanner_profile.target_timeout,
+        full_scan_enabled: dast_scanner_profile.full_scan_enabled?,
+        use_ajax_spider: dast_scanner_profile.use_ajax_spider,
+        show_debug_messages: dast_scanner_profile.show_debug_messages
+      }
+    end
+  end
+end

ee/spec/services/ci/dast_scan_ci_configuration_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::DastScanCiConfigurationService do
+  describe '.execute' do
+    subject(:yaml_configuration) { described_class.execute(params) }
+
+    context 'when all variables are provided' do
+      let(:params) do
+        {
+          spider_timeout: 1000,
+          target_timeout: 100,
+          target_url: 'https://gitlab.local',
+          use_ajax_spider: true,
+          show_debug_messages: true,
+          full_scan_enabled: true
+        }
+      end
+
+      let(:expected_yaml_configuration) do
+        <<~YAML
+        ---
+        stages:
+        - dast
+        include:
+        - template: DAST-On-Demand-Scan.gitlab-ci.yml
+        variables:
+          DAST_SPIDER_MINS: 1000
+          DAST_TARGET_AVAILABILITY_TIMEOUT: 100
+          DAST_WEBSITE: https://gitlab.local
+          DAST_USE_AJAX_SPIDER: 'true'
+          DAST_DEBUG: 'true'
+          DAST_FULL_SCAN_ENABLED: 'true'
+        YAML
+      end
+
+      it 'return YAML configuration of the On-Demand DAST scan' do
+        expect(yaml_configuration).to eq(expected_yaml_configuration)
+      end
+    end
+
+    context 'when additional variables are provided' do
+      let(:params) do
+        {
+          target_url: 'https://gitlab.local',
+          use_ajax_spider: false,
+          show_debug_messages: nil,
+          full_scan_enabled: nil,
+          additional_argument: true,
+          additional_list: ['item a']
+        }
+      end
+
+      let(:expected_yaml_configuration) do
+        <<~YAML
+        ---
+        stages:
+        - dast
+        include:
+        - template: DAST-On-Demand-Scan.gitlab-ci.yml
+        variables:
+          DAST_WEBSITE: https://gitlab.local
+          DAST_USE_AJAX_SPIDER: 'false'
+        YAML
+      end
+
+      it 'return YAML configuration of the On-Demand DAST scan' do
+        expect(yaml_configuration).to eq(expected_yaml_configuration)
+      end
+    end
+
+    context 'when no variable is provided' do
+      let(:params) { {} }
+
+      let(:expected_yaml_configuration) do
+        <<~YAML
+        ---
+        stages:
+        - dast
+        include:
+        - template: DAST-On-Demand-Scan.gitlab-ci.yml
+        variables: {}
+        YAML
+      end
+
+      it 'return YAML configuration of the On-Demand DAST scan' do
+        expect(yaml_configuration).to eq(expected_yaml_configuration)
+      end
+    end
+  end
+end

ee/spec/services/ci/run_dast_scan_service_spec.rb

@@ -15,16 +15,6 @@ RSpec.describe Ci::RunDastScanService do
     stub_licensed_features(security_on_demand_scans: true)
   end
 
-  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
-  end
-
   describe '#execute' do
     subject { described_class.new(project, user).execute(branch: branch, target_url: target_url, spider_timeout: 42, target_timeout: 21, use_ajax_spider: use_ajax_spider, show_debug_messages: show_debug_messages, full_scan_enabled: full_scan_enabled) }
 
@@ -140,7 +130,7 @@ RSpec.describe Ci::RunDastScanService do
             'public' => true
           }
         ]
-        expect(build.yaml_variables).to eq(expected_variables)
+        expect(build.yaml_variables).to contain_exactly(*expected_variables)
       end
 
       it 'enqueues a build' do

ee/spec/services/dast_on_demand_scans/params_create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe DastOnDemandScans::ParamsCreateService do
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:dast_site_profile) { create(:dast_site_profile, project: project) }
+  let_it_be(:dast_scanner_profile) { create(:dast_scanner_profile, project: project) }
+
+  let(:params) { { dast_site_profile: dast_site_profile, dast_scanner_profile: dast_scanner_profile } }
+
+  subject { described_class.new(container: project, params: params).execute }
+
+  describe 'execute' do
+    context 'when dast_site_profile is not provided' do
+      let(:params) { { dast_site_profile: nil, dast_scanner_profile: dast_scanner_profile } }
+
+      it 'responds with error message', :aggregate_failures do
+        expect(subject).not_to be_success
+        expect(subject.message).to eq('Site Profile was not provided')
+      end
+    end
+
+    context 'when dast_site_profile is provided' do
+      context 'and when dast_scanner_profile is not provided' do
+        let(:params) { { dast_site_profile: dast_site_profile, dast_scanner_profile: nil } }
+
+        it 'returns prepared scanner params in the payload' do
+          expect(subject.payload).to eq(
+            branch: 'master',
+            target_url: dast_site_profile.dast_site.url
+          )
+        end
+      end
+
+      context 'and when dast_scanner_profile is provided' do
+        let(:params) { { dast_site_profile: dast_site_profile, dast_scanner_profile: dast_scanner_profile } }
+
+        it 'returns prepared scanner params in the payload' do
+          expect(subject.payload).to eq(
+            branch: 'master',
+            full_scan_enabled: false,
+            show_debug_messages: false,
+            spider_timeout: nil,
+            target_timeout: nil,
+            target_url: dast_site_profile.dast_site.url,
+            use_ajax_spider: false
+          )
+        end
+
+        context 'but target is not validated and an active scan is requested' do
+          let_it_be(:active_dast_scanner_profile) { create(:dast_scanner_profile, project: project, scan_type: 'active') }
+
+          let(:params) { { dast_site_profile: dast_site_profile, dast_scanner_profile: active_dast_scanner_profile } }
+
+          it 'responds with error message', :aggregate_failures do
+            expect(subject).not_to be_success
+            expect(subject.message).to eq('Cannot run active scan against unvalidated target')
+          end
+        end
+      end
+    end
+  end
+end