Allow additional DAST scan config to be updated

- Add new services
- Update existing services
- Add specs

ee/app/graphql/mutations/dast_site_profiles/update.rb

@@ -47,33 +47,38 @@ module Mutations
 
       authorize :create_on_demand_dast_scan
 
-      def resolve(full_path:, id:, profile_name:, target_url: nil, excluded_urls: nil, request_headers: nil, auth: nil)
+      def resolve(full_path:, id:, profile_name:, target_url: nil, **params)
         project = authorized_find!(full_path)
 
+        auth_params = feature_flagged(project, params[:auth], default: {})
+
         # TODO: remove explicit coercion once compatibility layer has been removed
         # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
-        params = {
+        dast_site_profile_params = {
           id: SiteProfileID.coerce_isolated_input(id).model_id,
+          excluded_urls: feature_flagged(project, params[:excluded_urls]),
           name: profile_name,
+          request_headers: feature_flagged(project, params[:request_headers]),
           target_url: target_url,
-          excluded_urls: feature_flagged_excluded_urls(project, excluded_urls)
+          auth_enabled: auth_params[:enabled],
+          auth_url: auth_params[:url],
+          auth_username_field: auth_params[:username_field],
+          auth_password_field: auth_params[:password_field],
+          auth_username: auth_params[:username],
+          auth_password: auth_params[:password]
         }.compact
 
-        result = ::DastSiteProfiles::UpdateService.new(project, current_user).execute(**params)
+        result = ::DastSiteProfiles::UpdateService.new(project, current_user).execute(**dast_site_profile_params)
 
-        if result.success?
-          { id: result.payload.to_global_id, errors: [] }
-        else
-          { errors: result.errors }
-        end
+        { id: result.payload.try(:to_global_id), errors: result.errors }
       end
 
       private
 
-      def feature_flagged_excluded_urls(project, excluded_urls)
-        return unless Feature.enabled?(:security_dast_site_profiles_additional_fields, project, default_enabled: :yaml)
+      def feature_flagged(project, value, opts = {})
+        return opts[:default] unless Feature.enabled?(:security_dast_site_profiles_additional_fields, project, default_enabled: :yaml)
 
-        excluded_urls
+        value || opts[:default]
       end
     end
   end

ee/app/services/dast/site_profile_secret_variables/destroy_service.rb

+# frozen_string_literal: true
+
+module Dast
+  module SiteProfileSecretVariables
+    class DestroyService < BaseContainerService
+      def execute
+        return ServiceResponse.error(message: 'Insufficient permissions') unless allowed?
+        return ServiceResponse.error(message: 'Variable parameter missing') unless dast_site_profile_secret_variable
+        return ServiceResponse.error(message: 'Variable failed to delete') unless dast_site_profile_secret_variable.destroy
+
+        ServiceResponse.success(payload: dast_site_profile_secret_variable)
+      end
+
+      private
+
+      def allowed?
+        Feature.enabled?(:security_dast_site_profiles_additional_fields, container, default_enabled: :yaml) &&
+          Ability.allowed?(current_user, :create_on_demand_dast_scan, container)
+      end
+
+      def dast_site_profile_secret_variable
+        params[:dast_site_profile_secret_variable]
+      end
+    end
+  end
+end

ee/app/services/dast_site_profiles/update_service.rb

@@ -2,27 +2,43 @@
 
 module DastSiteProfiles
   class UpdateService < BaseService
+    class Rollback < StandardError
+      attr_reader :errors
+
+      def initialize(errors)
+        @errors = errors
+      end
+    end
+
+    attr_reader :dast_site_profile
+
     def execute(id:, **params)
       return ServiceResponse.error(message: 'Insufficient permissions') unless allowed?
 
-      dast_site_profile = find_dast_site_profile!(id)
+      find_dast_site_profile!(id)
 
-      return ServiceResponse.error(message: "Cannot modify #{dast_site_profile.name} referenced in security policy") if referenced_in_security_policy?(dast_site_profile)
+      return ServiceResponse.error(message: "Cannot modify #{dast_site_profile.name} referenced in security policy") if referenced_in_security_policy?
 
       ActiveRecord::Base.transaction do
         if target_url = params.delete(:target_url)
           params[:dast_site] = DastSites::FindOrCreateService.new(project, current_user).execute!(url: target_url)
         end
 
+        handle_secret_variable!(params, :request_headers, Dast::SiteProfileSecretVariable::REQUEST_HEADERS)
+        handle_secret_variable!(params, :auth_password, Dast::SiteProfileSecretVariable::PASSWORD)
+
+        params.compact!
+
         dast_site_profile.update!(params)
 
         ServiceResponse.success(payload: dast_site_profile)
       end
-
-    rescue ActiveRecord::RecordNotFound => err
-      ServiceResponse.error(message: "#{err.model} not found")
-    rescue ActiveRecord::RecordInvalid => err
-      ServiceResponse.error(message: err.record.errors.full_messages)
+    rescue Rollback => e
+      ServiceResponse.error(message: e.errors)
+    rescue ActiveRecord::RecordNotFound => e
+      ServiceResponse.error(message: "#{e.model} not found")
+    rescue ActiveRecord::RecordInvalid => e
+      ServiceResponse.error(message: e.record.errors.full_messages)
     end
 
     private
@@ -31,13 +47,48 @@ module DastSiteProfiles
       Ability.allowed?(current_user, :create_on_demand_dast_scan, project)
     end
 
-    def referenced_in_security_policy?(profile)
-      profile.referenced_in_security_policies.present?
+    def referenced_in_security_policy?
+      dast_site_profile.referenced_in_security_policies.present?
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
     def find_dast_site_profile!(id)
-      DastSiteProfilesFinder.new(project_id: project.id, id: id).execute.first!
+      @dast_site_profile = DastSiteProfilesFinder.new(project_id: project.id, id: id).execute.first!
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
+    def handle_secret_variable!(params, arg, key)
+      value = params.delete(arg)
+      return ServiceResponse.success unless value
+
+      return delete_secret_variable!(key) if value == ''
+
+      response = Dast::SiteProfileSecretVariables::CreateOrUpdateService.new(
+        container: project,
+        current_user: current_user,
+        params: { dast_site_profile: dast_site_profile, key: key, raw_value: value }
+      ).execute
+
+      raise Rollback, response.errors if response.error?
+
+      response
+    end
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def delete_secret_variable!(key)
+      variable = dast_site_profile.secret_variables.find_by(key: key)
+
+      return ServiceResponse.success unless variable
+
+      response = Dast::SiteProfileSecretVariables::DestroyService.new(
+        container: project,
+        current_user: current_user,
+        params: { dast_site_profile_secret_variable: variable }
+      ).execute
+
+      raise Rollback, response.errors if response.error?
+
+      response
     end
     # rubocop: enable CodeReuse/ActiveRecord
   end

ee/spec/graphql/mutations/dast_site_profiles/update_spec.rb

@@ -12,6 +12,18 @@ RSpec.describe Mutations::DastSiteProfiles::Update do
   let(:new_profile_name) { SecureRandom.hex }
   let(:new_target_url) { generate(:url) }
   let(:new_excluded_urls) { ["#{new_target_url}/signout"] }
+  let(:new_request_headers) { "Authorization: Bearer #{SecureRandom.hex}" }
+
+  let(:new_auth) do
+    {
+      enabled: true,
+      url: "#{new_target_url}/login",
+      username_field: 'login[username]',
+      password_field: 'login[password]',
+      username: generate(:email),
+      password: SecureRandom.hex
+    }
+  end
 
   subject(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
 
@@ -29,15 +41,8 @@ RSpec.describe Mutations::DastSiteProfiles::Update do
         profile_name: new_profile_name,
         target_url: new_target_url,
         excluded_urls: new_excluded_urls,
-        request_headers: 'Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0',
-        auth: {
-          enabled: true,
-          url: "#{new_target_url}/login",
-          username_field: 'session[username]',
-          password_field: 'session[password]',
-          username: generate(:email),
-          password: SecureRandom.hex
-        }
+        request_headers: new_request_headers,
+        auth: new_auth
       )
     end
 
@@ -55,21 +60,115 @@ RSpec.describe Mutations::DastSiteProfiles::Update do
           project.add_developer(user)
         end
 
+        it 'calls the dast_site_profile update service' do
+          service = double(DastSiteProfiles::UpdateService)
+          result = ServiceResponse.error(message: '')
+
+          service_params = {
+            id: dast_site_profile.id.to_s,
+            name: new_profile_name,
+            target_url: new_target_url,
+            excluded_urls: new_excluded_urls,
+            request_headers: new_request_headers,
+            auth_enabled: new_auth[:enabled],
+            auth_url: new_auth[:url],
+            auth_username_field: new_auth[:username_field],
+            auth_password_field: new_auth[:password_field],
+            auth_username: new_auth[:username],
+            auth_password: new_auth[:password]
+          }
+
+          expect(DastSiteProfiles::UpdateService).to receive(:new).and_return(service)
+          expect(service).to receive(:execute).with(service_params).and_return(result)
+
+          subject
+        end
+
         it 'updates the dast_site_profile', :aggregate_failures do
           dast_site_profile = subject[:id].find
 
-          expect(dast_site_profile.name).to eq(new_profile_name)
-          expect(dast_site_profile.dast_site.url).to eq(new_target_url)
-          expect(dast_site_profile.reload.excluded_urls).to eq(new_excluded_urls)
+          expect(dast_site_profile).to have_attributes(
+            name: new_profile_name,
+            excluded_urls: new_excluded_urls,
+            auth_enabled: new_auth[:enabled],
+            auth_url: new_auth[:url],
+            auth_username_field: new_auth[:username_field],
+            auth_password_field: new_auth[:password_field],
+            auth_username: new_auth[:username],
+            dast_site: have_attributes(url: new_target_url)
+          )
+
+          expect(dast_site_profile.secret_variables.map(&:key)).to include(Dast::SiteProfileSecretVariable::REQUEST_HEADERS)
+          expect(dast_site_profile.secret_variables.map(&:key)).to include(Dast::SiteProfileSecretVariable::PASSWORD)
+        end
+
+        context 'when secret variables already exist' do
+          let_it_be(:request_headers_variable) { create(:dast_site_profile_secret_variable, key: Dast::SiteProfileSecretVariable::REQUEST_HEADERS, dast_site_profile: dast_site_profile) }
+          let_it_be(:password_variable) { create(:dast_site_profile_secret_variable, key: Dast::SiteProfileSecretVariable::PASSWORD, dast_site_profile: dast_site_profile) }
+
+          context 'when the arguments are omitted' do
+            subject do
+              mutation.resolve(
+                full_path: full_path,
+                id: dast_site_profile.to_global_id,
+                profile_name: new_profile_name
+              )
+            end
+
+            it 'does not delete the secret variable' do
+              dast_site_profile = subject[:id].find
+
+              expect(dast_site_profile.secret_variables).not_to be_empty
+            end
+          end
+
+          context 'when the arguments are empty strings' do
+            subject do
+              mutation.resolve(
+                full_path: full_path,
+                id: dast_site_profile.to_global_id,
+                profile_name: new_profile_name,
+                request_headers: '',
+                auth: { password: '' }
+              )
+            end
+
+            it 'deletes secret variables' do
+              dast_site_profile = subject[:id].find
+
+              expect(dast_site_profile.secret_variables).to be_empty
+            end
+          end
+        end
+
+        context 'when variable creation fails' do
+          it 'returns an error and the dast_site_profile' do
+            service = double(Dast::SiteProfileSecretVariables::CreateOrUpdateService)
+            result = ServiceResponse.error(payload: create(:dast_site_profile), message: 'Oops')
+
+            allow(Dast::SiteProfileSecretVariables::CreateOrUpdateService).to receive(:new).and_return(service)
+            allow(service).to receive(:execute).and_return(result)
+
+            expect(subject).to include(errors: ['Oops'])
+          end
         end
 
         context 'when the feature flag security_dast_site_profiles_additional_fields is disabled' do
-          it 'does not set the branch_name' do
+          it 'does not update the feature flagged attributes', :aggregate_failures do
             stub_feature_flags(security_dast_site_profiles_additional_fields: false)
 
             dast_site_profile = subject[:id].find
 
-            expect(dast_site_profile.reload.excluded_urls).not_to eq(new_excluded_urls)
+            expect(dast_site_profile).not_to have_attributes(
+              excluded_urls: new_excluded_urls,
+              auth_enabled: new_auth[:enabled],
+              auth_url: new_auth[:url],
+              auth_username_field: new_auth[:username_field],
+              auth_password_field: new_auth[:password_field],
+              auth_username: new_auth[:username]
+            )
+
+            expect(dast_site_profile.secret_variables).to be_empty
           end
         end
       end

ee/spec/services/dast/site_profile_secret_variables/create_or_update_service_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe Dast::SiteProfileSecretVariables::CreateOrUpdateService do
   let_it_be(:default_params) do
     {
       dast_site_profile: dast_profile.dast_site_profile,
-      key: 'DAST_PASSWORD_BASE64',
+      key: Dast::SiteProfileSecretVariable::PASSWORD,
       raw_value: SecureRandom.hex
     }
   end

ee/spec/services/dast/site_profile_secret_variables/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Dast::SiteProfileSecretVariables::DestroyService do
+  include GraphqlHelpers
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:dast_site_profile) { create(:dast_site_profile, project: project) }
+  let_it_be(:dast_site_profile_secret_variable, reload: true) { create(:dast_site_profile_secret_variable, dast_site_profile: dast_site_profile) }
+
+  subject do
+    described_class.new(
+      container: project,
+      current_user: user,
+      params: { dast_site_profile_secret_variable: dast_site_profile_secret_variable }
+    ).execute
+  end
+
+  describe '#execute' do
+    context 'when on demand scan licensed feature is not available' do
+      it 'communicates failure' do
+        stub_licensed_features(security_on_demand_scans: false)
+
+        expect(subject).to have_attributes(
+          status: :error,
+          message: 'Insufficient permissions'
+        )
+      end
+    end
+
+    context 'when the feature is enabled' do
+      before do
+        stub_licensed_features(security_on_demand_scans: true)
+      end
+
+      context 'when the user cannot destroy a DAST site profile secret variable' do
+        it 'communicates failure' do
+          expect(subject).to have_attributes(
+            status: :error,
+            message: 'Insufficient permissions'
+          )
+        end
+      end
+
+      context 'when the user can destroy a DAST site profile secret variable' do
+        before do
+          project.add_developer(user)
+        end
+
+        it 'returns a success status' do
+          expect(subject.status).to eq(:success)
+        end
+
+        it 'deletes the dast_site_profile_secret_variable' do
+          expect { subject }.to change { Dast::SiteProfileSecretVariable.count }.by(-1)
+        end
+
+        it 'returns a dast_site_profile_secret_variable payload' do
+          expect(subject.payload).to be_a(Dast::SiteProfileSecretVariable)
+        end
+
+        context 'when the dast_site_profile_secret_variable fails to destroy' do
+          it 'communicates failure' do
+            allow(dast_site_profile_secret_variable).to receive(:destroy).and_return(false)
+
+            expect(subject).to have_attributes(
+              status: :error,
+              message: 'Variable failed to delete'
+            )
+          end
+        end
+
+        context 'when the dast_site_profile_secret_variable parameter is missing' do
+          let(:dast_site_profile_secret_variable) { nil }
+
+          it 'communicates failure' do
+            expect(subject).to have_attributes(
+              status: :error,
+              message: 'Variable parameter missing'
+            )
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/services/dast_site_profiles/update_service_spec.rb

@@ -3,13 +3,38 @@
 require 'spec_helper'
 
 RSpec.describe DastSiteProfiles::UpdateService do
-  let(:project) { dast_profile.project }
-  let(:user) { create(:user) }
-  let(:dast_profile) { create(:dast_site_profile) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, creator: user) }
+  let_it_be(:dast_site_profile) { create(:dast_site_profile, project: project) }
+  let_it_be(:dast_site_profile_id) { dast_site_profile.id }
+
+  let_it_be(:request_headers_variable) { create(:dast_site_profile_secret_variable, key: Dast::SiteProfileSecretVariable::REQUEST_HEADERS, dast_site_profile: dast_site_profile) }
+  let_it_be(:password_variable) { create(:dast_site_profile_secret_variable, key: Dast::SiteProfileSecretVariable::PASSWORD, dast_site_profile: dast_site_profile) }
+
+  let_it_be(:new_profile_name) { SecureRandom.hex }
+  let_it_be(:new_target_url) { generate(:url) }
+  let_it_be(:new_excluded_urls) { ["#{new_target_url}/signout"] }
+  let_it_be(:new_request_headers) { "Authorization: Bearer #{SecureRandom.hex}" }
+  let_it_be(:new_auth_url) { "#{new_target_url}/login" }
+  let_it_be(:new_auth_password) { SecureRandom.hex }
+
+  let(:default_params) do
+    {
+      id: dast_site_profile_id,
+      name: new_profile_name,
+      target_url: new_target_url,
+      excluded_urls: new_excluded_urls,
+      request_headers: new_request_headers,
+      auth_enabled: true,
+      auth_url: new_auth_url,
+      auth_username_field: 'login[username]',
+      auth_password_field: 'login[password]',
+      auth_username: generate(:email),
+      auth_password: new_auth_password
+    }
+  end
 
-  let(:new_profile_name) { SecureRandom.hex }
-  let(:new_target_url) { generate(:url) }
-  let(:new_excluded_urls) { ["#{new_target_url}/signout"] }
+  let(:params) { default_params }
 
   before do
     stub_licensed_features(security_on_demand_scans: true)
@@ -17,12 +42,7 @@ RSpec.describe DastSiteProfiles::UpdateService do
 
   describe '#execute' do
     subject do
-      described_class.new(project, user).execute(
-        id: dast_profile.id,
-        name: new_profile_name,
-        target_url: new_target_url,
-        excluded_urls: new_excluded_urls
-      )
+      described_class.new(project, user).execute(**params)
     end
 
     let(:status) { subject.status }
@@ -53,9 +73,7 @@ RSpec.describe DastSiteProfiles::UpdateService do
         updated_dast_site_profile = payload.reload
 
         expect(updated_dast_site_profile).to have_attributes(
-          name: new_profile_name,
-          excluded_urls: new_excluded_urls,
-          dast_site: have_attributes(url: new_target_url)
+          params.except(:request_headers, :auth_password, :target_url).merge(dast_site: have_attributes(url: new_target_url))
         )
       end
 
@@ -76,8 +94,7 @@ RSpec.describe DastSiteProfiles::UpdateService do
       end
 
       context 'when the target url is nil' do
-        let(:new_target_url) { nil }
-        let(:new_excluded_urls) { [generate(:url)] }
+        let(:params) { default_params.merge(target_url: nil) }
 
         it 'returns a success status' do
           expect(status).to eq(:success)
@@ -85,21 +102,18 @@ RSpec.describe DastSiteProfiles::UpdateService do
 
         it 'does not attempt to change the associated dast_site' do
           finder = double(DastSiteProfilesFinder)
-          profile = double(DastSiteProfile, referenced_in_security_policies: [])
 
           allow(DastSiteProfilesFinder).to receive(:new).and_return(finder)
-          allow(finder).to receive_message_chain(:execute, :first!).and_return(profile)
+          allow(finder).to receive_message_chain(:execute, :first!).and_return(dast_site_profile)
 
-          expect(profile).to receive(:update!).with(hash_excluding(dast_profile.dast_site))
+          expect(dast_site_profile).to receive(:update!).with(hash_excluding(dast_site_profile.dast_site))
 
           subject
         end
       end
 
       context 'when the dast_site_profile doesn\'t exist' do
-        before do
-          dast_profile.destroy!
-        end
+        let(:dast_site_profile_id) { 0 }
 
         it 'returns an error status' do
           expect(status).to eq(:error)
@@ -110,6 +124,22 @@ RSpec.describe DastSiteProfiles::UpdateService do
         end
       end
 
+      context 'when excluded_urls is nil' do
+        let(:params) { default_params.merge(excluded_urls: nil) }
+
+        it 'does not change excluded_urls' do
+          expect(payload.excluded_urls).to eq(dast_site_profile.excluded_urls)
+        end
+      end
+
+      context 'when excluded_urls is not supplied' do
+        let(:params) { default_params.except(:excluded_urls) }
+
+        it 'does not change excluded_urls' do
+          expect(payload.excluded_urls).to eq(dast_site_profile.excluded_urls)
+        end
+      end
+
       context 'when on demand scan licensed feature is not available' do
         before do
           stub_licensed_features(security_on_demand_scans: false)
@@ -124,7 +154,102 @@ RSpec.describe DastSiteProfiles::UpdateService do
         end
       end
 
-      include_examples 'restricts modification if referenced by policy', :modify
+      shared_examples 'it handles secret variable updating' do
+        it 'correctly sets the value' do
+          variable = Dast::SiteProfileSecretVariable.find_by(key: key, dast_site_profile: payload)
+
+          expect(Base64.strict_decode64(variable.value)).to eq(raw_value)
+        end
+
+        context 'when the feature flag is disabled' do
+          it 'does not update the secret variable' do
+            stub_feature_flags(security_dast_site_profiles_additional_fields: false)
+
+            variable = Dast::SiteProfileSecretVariable.find_by(key: key, dast_site_profile: dast_site_profile)
+
+            expect { subject }.not_to change { variable.reload.value }
+          end
+        end
+      end
+
+      shared_examples 'it handles secret variable updating failure' do
+        before do
+          allow_next_instance_of(Dast::SiteProfileSecretVariables::CreateOrUpdateService) do |service|
+            response = ServiceResponse.error(message: 'Something went wrong')
+
+            allow(service).to receive(:execute).and_return(response)
+          end
+        end
+
+        it 'returns an error response', :aggregate_failures do
+          expect(status).to eq(:error)
+          expect(message).to include('Something went wrong')
+        end
+      end
+
+      shared_examples 'it handles secret variable deletion' do
+        context 'when the input value is an empty string' do
+          let(:params) { default_params.merge(argument => '') }
+
+          it 'deletes the variable' do
+            variable = Dast::SiteProfileSecretVariable.find_by(key: key, dast_site_profile: dast_site_profile)
+
+            subject
+
+            expect { variable.reload }.to raise_error(ActiveRecord::RecordNotFound)
+          end
+        end
+
+        context 'when the input value is absent' do
+          let(:params) { default_params.except(argument) }
+
+          it 'does not delete the secret variable' do
+            variable = Dast::SiteProfileSecretVariable.find_by(key: key, dast_site_profile: dast_site_profile)
+
+            expect { subject }.not_to change { variable.reload.value }
+          end
+        end
+
+        context 'when the feature flag is disabled' do
+          let(:params) { default_params.merge(argument => '') }
+
+          it 'does not delete the secret variable' do
+            stub_feature_flags(security_dast_site_profiles_additional_fields: false)
+
+            variable = Dast::SiteProfileSecretVariable.find_by(key: key, dast_site_profile: dast_site_profile)
+
+            expect { variable.reload }.not_to raise_error
+          end
+        end
+      end
+
+      context 'when request_headers are supplied' do
+        let(:key) { 'DAST_REQUEST_HEADERS_BASE64' }
+        let(:raw_value) { default_params[:request_headers] }
+
+        it_behaves_like 'it handles secret variable updating'
+        it_behaves_like 'it handles secret variable updating failure'
+
+        it_behaves_like 'it handles secret variable deletion' do
+          let(:argument) { :request_headers }
+        end
+      end
+
+      context 'when auth_password is supplied' do
+        let(:key) { 'DAST_PASSWORD_BASE64' }
+        let(:raw_value) { default_params[:auth_password] }
+
+        it_behaves_like 'it handles secret variable updating'
+        it_behaves_like 'it handles secret variable updating failure'
+
+        it_behaves_like 'it handles secret variable deletion' do
+          let(:argument) { :auth_password }
+        end
+      end
+
+      include_examples 'restricts modification if referenced by policy', :modify do
+        let(:dast_profile) { dast_site_profile }
+      end
     end
   end
 end