Merge branch 'philipcunningham-pull-in-dast-profile-variables-216514' into 'master'

Deliver DAST on-demand CI variables to builds

See merge request gitlab-org/gitlab!63382

ee/app/services/app_sec/dast/profiles/build_config_service.rb

+# frozen_string_literal: true
+
+module AppSec
+  module Dast
+    module Profiles
+      class BuildConfigService < BaseProjectService
+        def execute
+          return ServiceResponse.error(message: 'Insufficient permissions') unless allowed?
+
+          ServiceResponse.success(payload: { dast_site_profile: site_profile, dast_scanner_profile: scanner_profile })
+        end
+
+        private
+
+        def allowed?
+          container.licensed_feature_available?(:security_on_demand_scans)
+        end
+
+        def site_profile
+          fetch_profile(params[:dast_site_profile]) do |name|
+            DastSiteProfilesFinder.new(project_id: container.id, name: name)
+          end
+        end
+
+        def scanner_profile
+          fetch_profile(params[:dast_scanner_profile]) do |name|
+            DastScannerProfilesFinder.new(project_ids: [container.id], name: name)
+          end
+        end
+
+        def fetch_profile(name)
+          return unless name
+
+          profile = yield(name).execute.first
+
+          return unless can?(current_user, :read_on_demand_scans, profile)
+
+          profile
+        end
+      end
+    end
+  end
+end

ee/lib/ee/gitlab/ci/pipeline/seed/build.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Ci
+      module Pipeline
+        module Seed
+          module Build
+            extend ::Gitlab::Utils::Override
+
+            override :attributes
+            def initialize(context, attributes, previous_stages)
+              super
+
+              @dast_configuration = attributes.dig(:options, :dast_configuration)
+            end
+
+            override :attributes
+            def attributes
+              super.deep_merge(dast_attributes)
+            end
+
+            private
+
+            # rubocop:disable Gitlab/ModuleWithInstanceVariables
+            def dast_attributes
+              return {} unless @dast_configuration
+              return {} unless @seed_attributes[:stage] == 'dast'
+              return {} unless ::Feature.enabled?(:dast_configuration_ui, @pipeline.project, default_enabled: :yaml)
+
+              result = AppSec::Dast::Profiles::BuildConfigService.new(
+                project: @pipeline.project,
+                current_user: @pipeline.user,
+                params: {
+                  dast_site_profile: @dast_configuration[:site_profile],
+                  dast_scanner_profile: @dast_configuration[:scanner_profile]
+                }
+              ).execute
+
+              return {} unless result.success?
+
+              result.payload
+            end
+            # rubocop:enable Gitlab/ModuleWithInstanceVariables
+          end
+        end
+      end
+    end
+  end
+end

ee/lib/ee/gitlab/ci/yaml_processor/result.rb

@@ -11,7 +11,9 @@ module EE
           def build_attributes(name)
             job = jobs.fetch(name.to_sym, {})
 
-            super.deep_merge(dast_configuration: job[:dast_configuration]).compact
+            super.deep_merge(
+              options: { dast_configuration: job[:dast_configuration] }.compact
+            )
           end
         end
       end

ee/spec/lib/ee/gitlab/ci/pipeline/seed/build_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user, developer_projects: [project]) }
+
+  let(:pipeline) { build(:ci_empty_pipeline, project: project, user: user) }
+  let(:seed_context) { double(pipeline: pipeline, root_variables: []) }
+  let(:stage) { 'dast' }
+  let(:attributes) { { name: 'rspec', ref: 'master', scheduling_type: :stage, stage: stage, options: { dast_configuration: dast_configuration } } }
+
+  let(:seed_build) { described_class.new(seed_context, attributes, []) }
+
+  describe '#attributes' do
+    subject { seed_build.attributes }
+
+    context 'dast' do
+      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(:dast_site_profile_name) { dast_site_profile.name }
+      let(:dast_scanner_profile_name) { dast_scanner_profile.name }
+
+      let(:dast_configuration) { { site_profile: dast_site_profile_name, scanner_profile: dast_scanner_profile_name } }
+
+      shared_examples 'it does not change build attributes' do
+        it 'does not add dast_site_profile or dast_scanner_profile' do
+          expect(subject.keys).not_to include(:dast_site_profile, :dast_scanner_profile)
+        end
+      end
+
+      context 'when the feature is not licensed' do
+        it_behaves_like 'it does not change build attributes'
+      end
+
+      context 'when the feature is licensed' do
+        before do
+          stub_licensed_features(security_on_demand_scans: true)
+        end
+
+        context 'when the feature is not enabled' do
+          before do
+            stub_feature_flags(dast_configuration_ui: false)
+          end
+
+          it_behaves_like 'it does not change build attributes'
+        end
+
+        context 'when the feature is enabled' do
+          before do
+            stub_feature_flags(dast_configuration_ui: true)
+          end
+
+          shared_examples 'it looks up dast profiles in the database' do |key|
+            context 'when the profile exists' do
+              it 'adds the profile to the build attributes' do
+                expect(subject).to include(profile.class.underscore.to_sym => profile)
+              end
+            end
+
+            shared_examples 'it has no effect' do
+              it 'does not add the profile to the build attributes' do
+                expect(subject).not_to include(profile.class.underscore.to_sym => profile)
+              end
+            end
+
+            context 'when the profile is not provided' do
+              let(key) { nil }
+
+              it_behaves_like 'it has no effect'
+            end
+
+            context 'when the profile does not exist' do
+              let(key) { SecureRandom.hex }
+
+              it_behaves_like 'it has no effect'
+            end
+
+            context 'when the stage is not dast' do
+              let(:stage) { 'test' }
+
+              it_behaves_like 'it has no effect'
+            end
+          end
+
+          context 'dast_site_profile' do
+            let(:profile) { dast_site_profile }
+
+            it_behaves_like 'it looks up dast profiles in the database', :dast_site_profile_name
+          end
+
+          context 'dast_scanner_profile' do
+            let(:profile) { dast_scanner_profile }
+
+            it_behaves_like 'it looks up dast profiles in the database', :dast_scanner_profile_name
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/ci/templates/dast_gitlab_ci_yaml_spec.rb

@@ -72,26 +72,6 @@ RSpec.describe 'DAST.gitlab-ci.yml' do
 
         context 'when cluster is not active' do
           context 'by default' do
-            it 'includes no jobs' do
-              expect { pipeline }.to raise_error(Ci::CreatePipelineService::CreateError)
-            end
-          end
-
-          context 'when DAST_WEBSITE is present' do
-            before do
-              create(:ci_variable, project: project, key: 'DAST_WEBSITE', value: 'http://example.com')
-            end
-
-            it 'includes dast job' do
-              expect(build_names).to match_array(%w[dast])
-            end
-          end
-
-          context 'when DAST_API_SPECIFICATION is present' do
-            before do
-              create(:ci_variable, project: project, key: 'DAST_API_SPECIFICATION', value: 'http://my.api/api-specification.yml')
-            end
-
             it 'includes dast job' do
               expect(build_names).to match_array(%w[dast])
             end

ee/spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -266,7 +266,7 @@ RSpec.describe Gitlab::Ci::YamlProcessor do
       end
 
       it 'creates a job with a valid specification' do
-        expect(subject.builds[0]).to include(dast_configuration: { site_profile: 'Site profile', scanner_profile: 'Scanner profile' })
+        expect(subject.builds[0][:options]).to include(dast_configuration: { site_profile: 'Site profile', scanner_profile: 'Scanner profile' })
       end
     end
   end

ee/spec/models/ci/build_spec.rb

@@ -221,7 +221,7 @@ RSpec.describe Ci::Build do
           context 'when dast_configuration is absent from the options' do
             let(:options) { {} }
 
-            it 'does not attempt look up any dast profiles', :aggregate_failures do
+            it 'does not attempt look up any dast profiles to avoid unnecessary queries', :aggregate_failures do
               expect(job).not_to receive(:dast_site_profile)
               expect(job).not_to receive(:dast_scanner_profile)
 
@@ -232,7 +232,7 @@ RSpec.describe Ci::Build do
           context 'when site_profile is absent from the dast_configuration' do
             let(:options) { { dast_configuration: { scanner_profile: dast_scanner_profile.name } } }
 
-            it 'does not attempt look up the site profile' do
+            it 'does not attempt look up the site profile to avoid unnecessary queries' do
               expect(job).not_to receive(:dast_site_profile)
 
               subject
@@ -242,7 +242,7 @@ RSpec.describe Ci::Build do
           context 'when scanner_profile is absent from the dast_configuration' do
             let(:options) { { dast_configuration: { site_profile: dast_site_profile.name } } }
 
-            it 'does not attempt look up the scanner profile' do
+            it 'does not attempt look up the scanner profile to avoid unnecessary queries' do
               expect(job).not_to receive(:dast_scanner_profile)
 
               subject

ee/spec/services/app_sec/dast/profiles/build_config_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe AppSec::Dast::Profiles::BuildConfigService 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_it_be(:user) { create(:user, developer_projects: [project] ) }
+
+  let(:dast_site_profile_name) { dast_site_profile.name }
+  let(:dast_scanner_profile_name) { dast_scanner_profile.name }
+
+  let(:params) { { dast_site_profile: dast_site_profile_name, dast_scanner_profile: dast_scanner_profile_name } }
+
+  subject { described_class.new(project: project, current_user: user, params: params).execute }
+
+  describe '#execute' do
+    before do
+      stub_licensed_features(security_on_demand_scans: true)
+    end
+
+    shared_examples 'a fetch operation' do |dast_profile_name_key|
+      context 'when licensed' do
+        context 'when the profile exists' do
+          it 'includes the profile in the payload', :aggregate_failures do
+            expect(subject).to be_success
+            expect(subject.payload[profile.class.underscore.to_sym]).to eq(profile)
+          end
+        end
+
+        context 'when the profile is not provided' do
+          let(dast_profile_name_key) { nil }
+
+          it 'does not include the profile in the payload', :aggregate_failures do
+            expect(subject).to be_success
+            expect(subject.payload[profile.class.underscore.to_sym]).to be_nil
+          end
+        end
+
+        context 'when the profile does not exist' do
+          let(dast_profile_name_key) { SecureRandom.hex }
+
+          it 'does not include the profile in the payload', :aggregate_failures do
+            expect(subject).to be_success
+            expect(subject.payload[profile.class.underscore.to_sym]).to be_nil
+          end
+        end
+
+        context 'when the user does not have access to the profile' do
+          let_it_be(:user) { build(:user) }
+
+          it 'does not include the profile in the payload', :aggregate_failures do
+            expect(subject).to be_success
+            expect(subject.payload[profile.class.underscore.to_sym]).to be_nil
+          end
+        end
+      end
+
+      context 'when not licensed' do
+        before do
+          stub_licensed_features(security_on_demand_scans: false)
+        end
+
+        it 'communicates failure' do
+          expect(subject).to have_attributes(status: :error, message: 'Insufficient permissions')
+        end
+      end
+    end
+
+    it_behaves_like 'a fetch operation', :dast_site_profile_name do
+      let(:profile) { dast_site_profile }
+    end
+
+    it_behaves_like 'a fetch operation', :dast_scanner_profile_name do
+      let(:profile) { dast_scanner_profile }
+    end
+
+    it 'includes all profiles in the payload' do
+      expect(subject.payload).to eq(dast_site_profile: dast_site_profile, dast_scanner_profile: dast_scanner_profile)
+    end
+  end
+end

ee/spec/services/ci/create_pipeline_service/dast_configuration_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::CreatePipelineService do
+  let_it_be(:project) { create(:project, :custom_repo, files: { 'README.txt' => '' }) }
+  let_it_be(:user) { create(:user, developer_projects: [project]) }
+  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(:dast_variables) do
+    dast_site_profile.ci_variables
+      .concat(dast_scanner_profile.ci_variables)
+      .to_runner_variables
+  end
+
+  let(:dast_secret_variables) do
+    dast_site_profile.secret_ci_variables(user)
+      .to_runner_variables
+  end
+
+  let(:config) do
+    <<~EOY
+    include:
+      - template: DAST.gitlab-ci.yml
+    stages:
+      - build
+      - dast
+    build:
+      stage: build
+      dast_configuration:
+        site_profile: #{dast_site_profile.name}
+        scanner_profile: #{dast_scanner_profile.name}
+      script:
+        - env
+    dast:
+      dast_configuration:
+        site_profile: #{dast_site_profile.name}
+        scanner_profile: #{dast_scanner_profile.name}
+    EOY
+  end
+
+  let(:dast_build) { subject.builds.find_by(name: 'dast') }
+  let(:dast_build_variables) { dast_build.variables.to_runner_variables }
+
+  let(:build_variables) do
+    subject.builds
+      .find_by(name: 'build')
+      .variables
+      .to_runner_variables
+  end
+
+  subject { described_class.new(project, user, ref: 'refs/heads/master').execute(:push) }
+
+  before do
+    stub_ci_pipeline_yaml_file(config)
+  end
+
+  shared_examples 'it does not expand the dast variables' do
+    it 'does not include the profile variables' do
+      expect(build_variables).not_to include(*dast_variables)
+    end
+  end
+
+  context 'when the feature is not licensed' do
+    it_behaves_like 'it does not expand the dast variables'
+  end
+
+  context 'when the feature is licensed' do
+    before do
+      stub_licensed_features(dast: true, security_on_demand_scans: true)
+
+      project_features = project.licensed_features
+      allow(project).to receive(:licensed_features).and_return(project_features << :dast)
+    end
+
+    context 'when the feature is not enabled' do
+      before do
+        stub_feature_flags(dast_configuration_ui: false)
+      end
+
+      it_behaves_like 'it does not expand the dast variables'
+    end
+
+    context 'when the feature is enabled' do
+      before do
+        stub_feature_flags(dast_configuration_ui: true)
+      end
+
+      context 'when the stage is dast' do
+        it 'persists dast_configuration in build options' do
+          expect(dast_build.options).to include(dast_configuration: { site_profile: dast_site_profile.name, scanner_profile: dast_scanner_profile.name })
+        end
+
+        it 'expands the dast variables' do
+          expect(dast_variables).to include(*dast_variables)
+        end
+
+        context 'when the user has permission' do
+          it 'expands the secret dast variables' do
+            expect(dast_variables).to include(*dast_secret_variables)
+          end
+        end
+      end
+
+      context 'when the stage is not dast' do
+        it_behaves_like 'it does not expand the dast variables'
+      end
+    end
+  end
+end

lib/gitlab/ci/pipeline/seed/build.rb

@@ -222,3 +222,5 @@ module Gitlab
     end
   end
 end
+
+Gitlab::Ci::Pipeline::Seed::Build.prepend_mod_with('Gitlab::Ci::Pipeline::Seed::Build')

lib/gitlab/ci/templates/Security/DAST.gitlab-ci.yml

@@ -43,15 +43,10 @@ dast:
           $CI_DEFAULT_BRANCH == $CI_COMMIT_REF_NAME
       when: never
     - if: $CI_DEFAULT_BRANCH != $CI_COMMIT_REF_NAME &&
-          $REVIEW_DISABLED && $DAST_WEBSITE == null &&
-          $DAST_API_SPECIFICATION == null
+          $REVIEW_DISABLED
       when: never
     - if: $CI_COMMIT_BRANCH &&
           $CI_KUBERNETES_ACTIVE &&
           $GITLAB_FEATURES =~ /\bdast\b/
     - if: $CI_COMMIT_BRANCH &&
-          $GITLAB_FEATURES =~ /\bdast\b/ &&
-          $DAST_WEBSITE
-    - if: $CI_COMMIT_BRANCH &&
-          $GITLAB_FEATURES =~ /\bdast\b/ &&
-          $DAST_API_SPECIFICATION
+          $GITLAB_FEATURES =~ /\bdast\b/