Merge branch 'philipcunningham-add-dast-database-constraints-300740' into 'master'

Associate successful DAST validations with sites

See merge request gitlab-org/gitlab!70140

ee/app/graphql/mutations/dast_site_validations/create.rb

@@ -38,7 +38,7 @@ module Mutations
 
         dast_site_token = dast_site_token_id.find
 
-        response = ::DastSiteValidations::CreateService.new(
+        response = ::DastSiteValidations::FindOrCreateService.new(
           container: project,
           current_user: current_user,
           params: {

ee/app/services/dast_site_validations/create_service.rb → ee/app/services/dast_site_validations/find_or_create_service.rb

 # frozen_string_literal: true
 
 module DastSiteValidations
-  class CreateService < BaseContainerService
+  class FindOrCreateService < BaseContainerService
     def execute
       return ServiceResponse.error(message: 'Insufficient permissions') unless allowed?
-      return ServiceResponse.success(payload: existing_validation) if existing_validation
 
-      dast_site_validation = create_validation!
+      dast_site_validation = existing_successful_validation || create_validation!
 
       return ServiceResponse.error(message: 'Site does not exist for profile') unless dast_site_validation.dast_site
 
       associate_dast_site!(dast_site_validation)
 
+      return ServiceResponse.success(payload: dast_site_validation) if dast_site_validation.passed?
+
       perform_runner_validation(dast_site_validation)
     rescue ActiveRecord::RecordInvalid => err
       ServiceResponse.error(message: err.record.errors.full_messages)
@@ -38,8 +39,8 @@ module DastSiteValidations
       @validation_strategy ||= params.fetch(:validation_strategy)
     end
 
-    def existing_validation
-      @existing_validation ||= find_latest_successful_dast_site_validation
+    def existing_successful_validation
+      @existing_successful_validation ||= find_latest_successful_dast_site_validation
     end
 
     def url_base

ee/spec/services/dast_site_validations/create_service_spec.rb → ee/spec/services/dast_site_validations/find_or_create_service_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe DastSiteValidations::CreateService do
+RSpec.describe DastSiteValidations::FindOrCreateService do
   let_it_be(:project) { create(:project, :repository) }
   let_it_be(:developer) { create(:user, developer_projects: [project]) }
   let_it_be(:dast_site) { create(:dast_site, project: project) }
@@ -12,98 +12,96 @@ RSpec.describe DastSiteValidations::CreateService do
 
   subject { described_class.new(container: project, current_user: developer, params: params).execute }
 
-  shared_examples 'the licensed feature is not available' do
-    it 'communicates failure' do
-      stub_licensed_features(security_on_demand_scans: false)
+  describe 'execute', :clean_gitlab_redis_shared_state do
+    context 'when the licensed feature is available' do
+      it 'communicates failure' do
+        stub_licensed_features(security_on_demand_scans: false)
 
-      aggregate_failures do
-        expect(subject.status).to eq(:error)
-        expect(subject.message).to eq('Insufficient permissions')
+        aggregate_failures do
+          expect(subject.status).to eq(:error)
+          expect(subject.message).to eq('Insufficient permissions')
+        end
       end
     end
-  end
 
-  shared_examples 'the licensed feature is available' do
-    before do
-      stub_licensed_features(security_on_demand_scans: true)
-    end
+    context 'when the licensed feature is available' do
+      before do
+        stub_licensed_features(security_on_demand_scans: true)
+      end
 
-    it 'communicates success' do
-      expect(subject.status).to eq(:success)
-    end
+      it 'communicates success' do
+        expect(subject.status).to eq(:success)
+      end
 
-    it 'creates a new record in the database' do
-      expect { subject }.to change { DastSiteValidation.count }.by(1)
-    end
+      it 'creates a new record in the database' do
+        expect { subject }.to change { DastSiteValidation.count }.by(1)
+      end
 
-    it 'associates the dast_site_validation with the dast_site' do
-      expect(subject.payload).to eq(dast_site.reload.dast_site_validation)
-    end
+      it 'associates the dast_site_validation with the dast_site' do
+        expect(subject.payload).to eq(dast_site.reload.dast_site_validation)
+      end
 
-    context 'when a param is missing' do
-      let(:params) { { dast_site_token: dast_site_token, validation_strategy: :text_file } }
+      it 'attempts to validate' do
+        expected_args = { project: project, current_user: developer, params: { dast_site_validation: instance_of(DastSiteValidation) } }
 
-      it 'communicates failure' do
-        aggregate_failures do
-          expect(subject.status).to eq(:error)
-          expect(subject.message).to eq('Key not found: :url_path')
-        end
+        expect(AppSec::Dast::SiteValidations::RunnerService).to receive(:new).with(expected_args).and_call_original
+
+        subject
       end
-    end
 
-    context 'when the dast_site_token.project and container do not match' do
-      let_it_be(:dast_site_token) { create(:dast_site_token, project: create(:project), url: dast_site.url) }
+      context 'when a param is missing' do
+        let(:params) { { dast_site_token: dast_site_token, validation_strategy: :text_file } }
 
-      it 'communicates failure' do
-        aggregate_failures do
-          expect(subject.status).to eq(:error)
-          expect(subject.message).to eq('Insufficient permissions')
+        it 'communicates failure' do
+          aggregate_failures do
+            expect(subject.status).to eq(:error)
+            expect(subject.message).to eq('Key not found: :url_path')
+          end
         end
       end
-    end
 
-    context 'when the dast_site_token does not have a related dast_site via its url' do
-      let_it_be(:dast_site_token) { create(:dast_site_token, project: project, url: generate(:url)) }
+      context 'when the dast_site_token.project and container do not match' do
+        let_it_be(:dast_site_token) { create(:dast_site_token, project: create(:project), url: dast_site.url) }
 
-      it 'communicates failure' do
-        aggregate_failures do
-          expect(subject.status).to eq(:error)
-          expect(subject.message).to eq('Site does not exist for profile')
+        it 'communicates failure' do
+          aggregate_failures do
+            expect(subject.status).to eq(:error)
+            expect(subject.message).to eq('Insufficient permissions')
+          end
         end
       end
-    end
-  end
-
-  shared_examples 'a dast_site_validation already exists' do
-    let!(:dast_site_validation) { create(:dast_site_validation, dast_site_token: dast_site_token, state: :passed) }
 
-    it 'returns the existing successful dast_site_validation' do
-      expect(subject.payload).to eq(dast_site_validation)
-    end
-
-    it 'does not create a new record in the database' do
-      expect { subject }.not_to change { DastSiteValidation.count }
-    end
-  end
+      context 'when the dast_site_token does not have a related dast_site via its url' do
+        let_it_be(:dast_site_token) { create(:dast_site_token, project: project, url: generate(:url)) }
 
-  describe 'execute', :clean_gitlab_redis_shared_state do
-    it_behaves_like 'the licensed feature is not available'
+        it 'communicates failure' do
+          aggregate_failures do
+            expect(subject.status).to eq(:error)
+            expect(subject.message).to eq('Site does not exist for profile')
+          end
+        end
+      end
 
-    it_behaves_like 'the licensed feature is available' do
-      it 'attempts to validate' do
-        expected_args = { project: project, current_user: developer, params: { dast_site_validation: instance_of(DastSiteValidation) } }
+      context 'when the site has already passed validation' do
+        let_it_be(:dast_site_validation) { create(:dast_site_validation, dast_site_token: dast_site_token, state: :passed) }
 
-        expect(AppSec::Dast::SiteValidations::RunnerService).to receive(:new).with(expected_args).and_call_original
+        it 'returns the existing dast_site_validation' do
+          expect(subject.payload).to eq(dast_site_validation)
+        end
 
-        subject
-      end
+        it 'does not create a new record in the database' do
+          expect { subject }.not_to change { DastSiteValidation.count }
+        end
 
-      it_behaves_like 'a dast_site_validation already exists' do
         it 'does not attempt to re-validate' do
           expect(AppSec::Dast::SiteValidations::RunnerService).not_to receive(:new)
 
           subject
         end
+
+        it 'associates the dast_site_validation with the dast_site' do
+          expect(subject.payload).to eq(dast_site.reload.dast_site_validation)
+        end
       end
     end
   end