Merge branch 'philipcunningham-error-when-dast-profiles-not-found-216514' into 'master'

Present error when DAST profile not found

See merge request gitlab-org/gitlab!64679

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

@@ -5,26 +5,35 @@ module AppSec
     module Profiles
       class BuildConfigService < BaseProjectService
         def execute
-          return ServiceResponse.error(message: 'Insufficient permissions') unless allowed?
+          return ServiceResponse.error(message: 'Insufficient permissions for dast_configuration keyword') unless allowed?
 
-          ServiceResponse.success(payload: { dast_site_profile: site_profile, dast_scanner_profile: scanner_profile })
+          build_config = { dast_site_profile: site_profile, dast_scanner_profile: scanner_profile }
+
+          return ServiceResponse.error(message: errors) unless errors.empty?
+
+          ServiceResponse.success(payload: build_config)
         end
 
         private
 
         def allowed?
-          container.licensed_feature_available?(:security_on_demand_scans)
+          can?(current_user, :create_on_demand_dast_scan, project) &&
+            ::Feature.enabled?(:dast_configuration_ui, project, default_enabled: :yaml)
+        end
+
+        def errors
+          @errors ||= []
         end
 
         def site_profile
           fetch_profile(params[:dast_site_profile]) do |name|
-            DastSiteProfilesFinder.new(project_id: container.id, name: name)
+            DastSiteProfilesFinder.new(project_id: project.id, name: name)
           end
         end
 
         def scanner_profile
           fetch_profile(params[:dast_scanner_profile]) do |name|
-            DastScannerProfilesFinder.new(project_ids: [container.id], name: name)
+            DastScannerProfilesFinder.new(project_ids: [project.id], name: name)
           end
         end
 
@@ -33,7 +42,10 @@ module AppSec
 
           profile = yield(name).execute.first
 
-          return unless can?(current_user, :read_on_demand_scans, profile)
+          unless profile && can?(current_user, :read_on_demand_scans, profile)
+            errors.append("DAST profile not found: #{name}")
+            return
+          end
 
           profile
         end

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

@@ -17,29 +17,32 @@ module EE
 
             override :attributes
             def attributes
-              super.deep_merge(dast_attributes)
+              super.deep_merge(dast_configuration.payload)
+            end
+
+            override :errors
+            def errors
+              strong_memoize(:errors) do
+                super.concat(dast_configuration.errors)
+              end
             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
+            def dast_configuration
+              return ServiceResponse.success unless @dast_configuration && @seed_attributes[:stage] == 'dast'
+
+              strong_memoize(:dast_attributes) do
+                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
+              end
             end
             # rubocop:enable Gitlab/ModuleWithInstanceVariables
           end

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

@@ -5,6 +5,7 @@ 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_it_be(:outsider) { create(:user) }
 
   let(:pipeline) { build(:ci_empty_pipeline, project: project, user: user) }
   let(:seed_context) { double(pipeline: pipeline, root_variables: []) }
@@ -31,8 +32,18 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do
         end
       end
 
+      shared_examples 'an insufficient permissions error' do
+        it 'communicates failure' do
+          expect(seed_build.errors).to include('Insufficient permissions for dast_configuration keyword')
+        end
+      end
+
       context 'when the feature is not licensed' do
         it_behaves_like 'it does not change build attributes'
+
+        it 'communicates failure' do
+          expect(seed_build.errors).to contain_exactly('Insufficient permissions for dast_configuration keyword')
+        end
       end
 
       context 'when the feature is licensed' do
@@ -40,12 +51,20 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do
           stub_licensed_features(security_on_demand_scans: true)
         end
 
+        context 'when the user cannot create dast scans' do
+          let_it_be(:user) { outsider }
+
+          it_behaves_like 'it does not change build attributes'
+          it_behaves_like 'an insufficient permissions error'
+        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'
+          it_behaves_like 'an insufficient permissions error'
         end
 
         context 'when the feature is enabled' do
@@ -53,7 +72,9 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do
             stub_feature_flags(dast_configuration_ui: true)
           end
 
-          shared_examples 'it looks up dast profiles in the database' do |key|
+          shared_examples 'it looks up dast profiles in the database' do |dast_profile_name_key|
+            let(:profile_name) { public_send(dast_profile_name_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)
@@ -67,21 +88,38 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do
             end
 
             context 'when the profile is not provided' do
-              let(key) { nil }
+              let(dast_profile_name_key) { nil }
 
               it_behaves_like 'it has no effect'
             end
 
-            context 'when the profile does not exist' do
-              let(key) { SecureRandom.hex }
+            context 'when the stage is not dast' do
+              let(:stage) { 'test' }
 
               it_behaves_like 'it has no effect'
             end
 
-            context 'when the stage is not dast' do
-              let(:stage) { 'test' }
+            context 'when the profile does not exist' do
+              let(dast_profile_name_key) { SecureRandom.hex }
 
-              it_behaves_like 'it has no effect'
+              it 'communicates failure' do
+                expect(seed_build.errors).to contain_exactly("DAST profile not found: #{profile_name}")
+              end
+            end
+
+            context 'when the profile cannot be read' do
+              let_it_be(:user) { outsider }
+
+              before do
+                allow_next_instance_of(AppSec::Dast::Profiles::BuildConfigService) do |service|
+                  allow(service).to receive(:can?).and_call_original
+                  allow(service).to receive(:can?).with(user, :create_on_demand_dast_scan, project).and_return(true)
+                end
+              end
+
+              it 'communicates failure' do
+                expect(seed_build.errors).to include("DAST profile not found: #{profile_name}")
+              end
             end
           end
 

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

@@ -7,6 +7,7 @@ RSpec.describe AppSec::Dast::Profiles::BuildConfigService 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_it_be(:user) { create(:user, developer_projects: [project] ) }
+  let_it_be(:outsider) { create(:user) }
 
   let(:dast_site_profile_name) { dast_site_profile.name }
   let(:dast_scanner_profile_name) { dast_scanner_profile.name }
@@ -20,7 +21,17 @@ RSpec.describe AppSec::Dast::Profiles::BuildConfigService do
       stub_licensed_features(security_on_demand_scans: true)
     end
 
+    shared_examples 'an error occurred' do
+      it 'communicates failure', :aggregate_failures do
+        expect(subject).to be_error
+        expect(subject.payload[profile.class.underscore.to_sym]).to be_nil
+        expect(subject.errors).to include(error_message)
+      end
+    end
+
     shared_examples 'a fetch operation' do |dast_profile_name_key|
+      let(:profile_name) { public_send(dast_profile_name_key) }
+
       context 'when licensed' do
         context 'when the profile exists' do
           it 'includes the profile in the payload', :aggregate_failures do
@@ -41,18 +52,31 @@ RSpec.describe AppSec::Dast::Profiles::BuildConfigService do
         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
+          it_behaves_like 'an error occurred' do
+            let(:error_message) { "DAST profile not found: #{profile_name}" }
+          end
+        end
+
+        context 'when the profile cannot be read' do
+          let_it_be(:user) { outsider }
+
+          before do
+            allow_next_instance_of(AppSec::Dast::Profiles::BuildConfigService) do |service|
+              allow(service).to receive(:can?).and_call_original
+              allow(service).to receive(:can?).with(user, :create_on_demand_dast_scan, project).and_return(true)
+            end
+          end
+
+          it_behaves_like 'an error occurred' do
+            let(:error_message) { "DAST profile not found: #{profile_name}" }
           end
         end
 
-        context 'when the user does not have access to the profile' do
+        context 'when the user cannot create dast scans' 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
+          it_behaves_like 'an error occurred' do
+            let(:error_message) { 'Insufficient permissions for dast_configuration keyword' }
           end
         end
       end
@@ -62,8 +86,8 @@ RSpec.describe AppSec::Dast::Profiles::BuildConfigService do
           stub_licensed_features(security_on_demand_scans: false)
         end
 
-        it 'communicates failure' do
-          expect(subject).to have_attributes(status: :error, message: 'Insufficient permissions')
+        it_behaves_like 'an error occurred' do
+          let(:error_message) { 'Insufficient permissions for dast_configuration keyword' }
         end
       end
     end

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

@@ -79,7 +79,9 @@ RSpec.describe Ci::CreatePipelineService do
         stub_feature_flags(dast_configuration_ui: false)
       end
 
-      it_behaves_like 'it does not expand the dast variables'
+      it 'communicates failure' do
+        expect(subject.yaml_errors).to eq('Insufficient permissions for dast_configuration keyword')
+      end
     end
 
     context 'when the feature is enabled' do
@@ -101,6 +103,26 @@ RSpec.describe Ci::CreatePipelineService do
             expect(dast_variables).to include(*dast_secret_variables)
           end
         end
+
+        shared_examples 'a missing profile' do
+          it 'communicates failure' do
+            expect(subject.yaml_errors).to eq("DAST profile not found: #{profile.name}")
+          end
+        end
+
+        context 'when the site profile does not exist' do
+          let(:dast_site_profile) { double(DastSiteProfile, name: SecureRandom.hex) }
+          let(:profile) { dast_site_profile }
+
+          it_behaves_like 'a missing profile'
+        end
+
+        context 'when the scanner profile does not exist' do
+          let(:dast_scanner_profile) { double(DastScannerProfile, name: SecureRandom.hex) }
+          let(:profile) { dast_scanner_profile }
+
+          it_behaves_like 'a missing profile'
+        end
       end
 
       context 'when the stage is not dast' do