Merge branch 'philipcunningham-creating-dast-site-profile-secret-variable-225406' into 'master'

Allow additional DAST scan config to be created

See merge request gitlab-org/gitlab!57311

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

@@ -42,29 +42,35 @@ module Mutations
 
       authorize :create_on_demand_dast_scan
 
-      def resolve(full_path:, profile_name:, target_url: nil, excluded_urls: [], request_headers: nil, auth: nil)
+      def resolve(full_path:, profile_name:, target_url: nil, **params)
         project = authorized_find!(full_path)
 
-        service = ::DastSiteProfiles::CreateService.new(project, current_user)
-        result = service.execute(
+        auth_params = feature_flagged(project, params[:auth], default: {})
+
+        dast_site_profile_params = {
           name: profile_name,
           target_url: target_url,
-          excluded_urls: feature_flagged_excluded_urls(project, excluded_urls)
-        )
-
-        if result.success?
-          { id: result.payload.to_global_id, errors: [] }
-        else
-          { errors: result.errors }
-        end
+          excluded_urls: feature_flagged(project, params[:excluded_urls]),
+          request_headers: feature_flagged(project, params[:request_headers]),
+          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::CreateService.new(project, current_user).execute(**dast_site_profile_params)
+
+        { 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/create_or_update_service.rb

+# frozen_string_literal: true
+
+module Dast
+  module SiteProfileSecretVariables
+    class CreateOrUpdateService < BaseContainerService
+      def execute
+        return error_response('Insufficient permissions') unless allowed?
+
+        return error_response('Dast site profile param is missing') unless site_profile
+        return error_response('Key param is missing') unless key
+        return error_response('Raw value param is missing') unless raw_value
+
+        secret_variable = find_or_create_secret_variable
+
+        return error_response(secret_variable.errors.full_messages) unless secret_variable.valid? && secret_variable.persisted?
+
+        success_response(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 site_profile
+        params[:dast_site_profile]
+      end
+
+      def key
+        params[:key]
+      end
+
+      def raw_value
+        params[:raw_value]
+      end
+
+      def success_response(secret_variable)
+        ServiceResponse.success(payload: secret_variable)
+      end
+
+      def error_response(message)
+        ServiceResponse.error(message: message)
+      end
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      def find_or_create_secret_variable
+        secret_variable = Dast::SiteProfileSecretVariable.find_or_initialize_by(dast_site_profile: site_profile, key: key)
+        secret_variable.update(raw_value: raw_value)
+
+        secret_variable
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+    end
+  end
+end

ee/app/services/dast_site_profiles/create_service.rb

@@ -2,25 +2,33 @@
 
 module DastSiteProfiles
   class CreateService < BaseService
-    def execute(name:, target_url:, excluded_urls: [])
+    class Rollback < StandardError
+      attr_reader :errors
+
+      def initialize(errors)
+        @errors = errors
+      end
+    end
+
+    attr_reader :dast_site_profile
+
+    def execute(name:, target_url:, **params)
       return ServiceResponse.error(message: 'Insufficient permissions') unless allowed?
 
       ActiveRecord::Base.transaction do
-        service = DastSites::FindOrCreateService.new(project, current_user)
-        dast_site = service.execute!(url: target_url)
+        dast_site = DastSites::FindOrCreateService.new(project, current_user).execute!(url: target_url)
+        params.merge!(project: project, dast_site: dast_site, name: name).compact!
 
-        dast_site_profile = DastSiteProfile.create!(
-          project: project,
-          dast_site: dast_site,
-          name: name,
-          excluded_urls: excluded_urls || []
-        )
+        @dast_site_profile = DastSiteProfile.create!(params.except(:request_headers, :auth_password))
+        create_secret_variable!(Dast::SiteProfileSecretVariable::PASSWORD, params[:auth_password])
+        create_secret_variable!(Dast::SiteProfileSecretVariable::REQUEST_HEADERS, params[:request_headers])
 
         ServiceResponse.success(payload: dast_site_profile)
       end
-
-    rescue ActiveRecord::RecordInvalid => err
-      ServiceResponse.error(message: err.record.errors.full_messages)
+    rescue Rollback => e
+      ServiceResponse.error(message: e.errors)
+    rescue ActiveRecord::RecordInvalid => e
+      ServiceResponse.error(message: e.record.errors.full_messages)
     end
 
     private
@@ -28,5 +36,19 @@ module DastSiteProfiles
     def allowed?
       Ability.allowed?(current_user, :create_on_demand_dast_scan, project)
     end
+
+    def create_secret_variable!(key, value)
+      return ServiceResponse.success unless 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
   end
 end

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

@@ -11,6 +11,18 @@ RSpec.describe Mutations::DastSiteProfiles::Create do
   let(:profile_name) { SecureRandom.hex }
   let(:target_url) { generate(:url) }
   let(:excluded_urls) { ["#{target_url}/signout"] }
+  let(:request_headers) { 'Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0' }
+
+  let(:auth) do
+    {
+      enabled: true,
+      url: "#{target_url}/login",
+      username_field: 'session[username]',
+      password_field: 'session[password]',
+      username: generate(:email),
+      password: SecureRandom.hex
+    }
+  end
 
   let(:dast_site_profile) { DastSiteProfile.find_by(project: project, name: profile_name) }
 
@@ -29,15 +41,8 @@ RSpec.describe Mutations::DastSiteProfiles::Create do
         profile_name: profile_name,
         target_url: target_url,
         excluded_urls: 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: "#{target_url}/login",
-          username_field: 'session[username]',
-          password_field: 'session[password]',
-          username: generate(:email),
-          password: SecureRandom.hex
-        }
+        request_headers: request_headers,
+        auth: auth
       )
     end
 
@@ -55,15 +60,47 @@ RSpec.describe Mutations::DastSiteProfiles::Create do
           project.add_developer(user)
         end
 
+        it 'creates a dast_site_profile and dast_site_profile_secret_variables', :aggregate_failures do
+          dast_site_profile = subject[:id].find
+
+          expect(dast_site_profile).to have_attributes(
+            name: profile_name,
+            excluded_urls: excluded_urls,
+            auth_enabled: auth[:enabled],
+            auth_url: auth[:url],
+            auth_username_field: auth[:username_field],
+            auth_password_field: auth[:password_field],
+            auth_username: auth[:username],
+            dast_site: have_attributes(url: target_url)
+          )
+
+          password_variable = dast_site_profile.secret_variables.find_by!(key: Dast::SiteProfileSecretVariable::PASSWORD)
+          expect(password_variable.value).to eq(Base64.strict_encode64(auth[:password]))
+
+          request_headers_variable = dast_site_profile.secret_variables.find_by!(key: Dast::SiteProfileSecretVariable::REQUEST_HEADERS)
+          expect(request_headers_variable.value).to eq(Base64.strict_encode64(request_headers))
+        end
+
         it 'returns the dast_site_profile id' do
           expect(subject[:id]).to eq(dast_site_profile.to_global_id)
         end
 
         it 'calls the dast_site_profile creation service' do
-          service = double(described_class)
-          result = double('result', success?: false, errors: [])
-
-          service_params = { name: profile_name, target_url: target_url, excluded_urls: excluded_urls }
+          service = double(DastSiteProfiles::CreateService)
+          result = ServiceResponse.error(message: '')
+
+          service_params = {
+            name: profile_name,
+            target_url: target_url,
+            excluded_urls: excluded_urls,
+            request_headers: request_headers,
+            auth_enabled: auth[:enabled],
+            auth_url: auth[:url],
+            auth_username_field: auth[:username_field],
+            auth_password_field: auth[:password_field],
+            auth_username: auth[:username],
+            auth_password: auth[:password]
+          }
 
           expect(DastSiteProfiles::CreateService).to receive(:new).and_return(service)
           expect(service).to receive(:execute).with(service_params).and_return(result)
@@ -85,23 +122,39 @@ RSpec.describe Mutations::DastSiteProfiles::Create do
           end
         end
 
-        context 'when excluded_urls is supplied as a param' do
-          context 'when the feature flag security_dast_site_profiles_additional_fields is disabled' do
-            it 'does not set the excluded_urls' do
-              stub_feature_flags(security_dast_site_profiles_additional_fields: false)
+        context 'when the feature flag security_dast_site_profiles_additional_fields is disabled' do
+          before do
+            stub_feature_flags(security_dast_site_profiles_additional_fields: false)
+          end
 
-              subject
+          it 'does not set the request_headers or the password dast_site_profile_secret_variables' do
+            subject
 
-              expect(dast_site_profile.excluded_urls).to be_empty
-            end
+            expect(dast_site_profile.secret_variables).to be_empty
           end
 
-          context 'when the feature flag security_dast_site_profiles_additional_fields is enabled' do
-            it 'sets the excluded_urls' do
-              subject
+          it 'does not set non-secret auth fields' do
+            subject
+
+            expect(dast_site_profile).to have_attributes(
+              auth_enabled: false,
+              auth_url: nil,
+              auth_username_field: nil,
+              auth_password_field: nil,
+              auth_username: nil
+            )
+          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(dast_site_profile.excluded_urls).to eq(excluded_urls)
-            end
+            expect(subject).to include(errors: ['Oops'])
           end
         end
       end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Dast::SiteProfileSecretVariables::CreateOrUpdateService do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:dast_profile) { create(:dast_profile, project: project) }
+  let_it_be(:developer) { create(:user, developer_projects: [project] ) }
+
+  let_it_be(:default_params) do
+    {
+      dast_site_profile: dast_profile.dast_site_profile,
+      key: 'DAST_PASSWORD_BASE64',
+      raw_value: SecureRandom.hex
+    }
+  end
+
+  let(:params) { default_params }
+
+  subject { described_class.new(container: project, current_user: developer, params: params).execute }
+
+  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)
+
+        aggregate_failures do
+          expect(subject.status).to eq(:error)
+          expect(subject.message).to include('Insufficient permissions')
+        end
+      end
+    end
+
+    context 'when the feature is enabled' do
+      before do
+        stub_licensed_features(security_on_demand_scans: true)
+      end
+
+      shared_examples 'it errors when a required param is missing' do |parameter|
+        context "when #{parameter} param is missing" do
+          let(:params) { default_params.except(parameter) }
+
+          it 'communicates failure', :aggregate_failures do
+            expect(subject.status).to eq(:error)
+            expect(subject.message).to eq("#{parameter.to_s.humanize} param is missing")
+          end
+        end
+      end
+
+      shared_examples 'it errors when there is a validation failure' do
+        let(:params) { default_params.merge(raw_value: '') }
+
+        it 'communicates failure', :aggregate_failures do
+          expect(subject.status).to eq(:error)
+          expect(subject.message).to include('Value is invalid')
+        end
+      end
+
+      it_behaves_like 'it errors when a required param is missing', :dast_site_profile
+      it_behaves_like 'it errors when a required param is missing', :key
+      it_behaves_like 'it errors when a required param is missing', :raw_value
+      it_behaves_like 'it errors when there is a validation failure'
+
+      it 'communicates success' do
+        expect(subject.status).to eq(:success)
+      end
+
+      it 'creates a dast_site_profile_secret_variable', :aggregate_failures do
+        expect { subject }.to change { Dast::SiteProfileSecretVariable.count }.by(1)
+
+        expect(subject.payload.value).to eq(Base64.strict_encode64(params[:raw_value]))
+      end
+
+      context 'when a variable already exists' do
+        let_it_be(:dast_site_profile_secret_variable) do
+          create(:dast_site_profile_secret_variable, key: default_params[:key], dast_site_profile: dast_profile.dast_site_profile)
+        end
+
+        let(:params) { default_params.merge(raw_value: 'hello, world') }
+
+        it_behaves_like 'it errors when there is a validation failure'
+
+        it 'does not create a dast_site_profile_secret_variable' do
+          expect { subject }.not_to change { Dast::SiteProfileSecretVariable.count }
+        end
+
+        it 'updates the existing dast_site_profile_secret_variable' do
+          subject
+
+          expect(dast_site_profile_secret_variable.reload.value).to eq(Base64.strict_encode64(params[:raw_value]))
+        end
+      end
+
+      context 'when the feature is disabled' do
+        it 'communicates failure', :aggregate_failures do
+          stub_feature_flags(security_dast_site_profiles_additional_fields: false)
+
+          expect(subject.status).to eq(:error)
+          expect(subject.message).to include('Insufficient permissions')
+        end
+      end
+    end
+  end
+end

ee/spec/services/dast_site_profiles/create_service_spec.rb

@@ -3,18 +3,36 @@
 require 'spec_helper'
 
 RSpec.describe DastSiteProfiles::CreateService do
-  let(:user) { create(:user) }
-  let(:project) { create(:project, :repository, creator: user) }
-  let(:name) { FFaker::Company.catch_phrase }
-  let(:target_url) { generate(:url) }
-  let(:excluded_urls) { ["#{target_url}/signout"] }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :repository, creator: user) }
+  let_it_be(:name) { FFaker::Company.catch_phrase }
+  let_it_be(:target_url) { generate(:url) }
+  let_it_be(:excluded_urls) { ["#{target_url}/signout"] }
+  let_it_be(:request_headers) { "Authorization: Bearer #{SecureRandom.hex}" }
+
+  let(:default_params) do
+    {
+      name: name,
+      target_url: target_url,
+      excluded_urls: excluded_urls,
+      request_headers: request_headers,
+      auth_enabled: true,
+      auth_url: "#{target_url}/login",
+      auth_username_field: 'session[username]',
+      auth_password_field: 'session[password]',
+      auth_username: generate(:email),
+      auth_password: SecureRandom.hex
+    }
+  end
+
+  let(:params) { default_params }
 
   before do
     stub_licensed_features(security_on_demand_scans: true)
   end
 
   describe '#execute' do
-    subject { described_class.new(project, user).execute(name: name, target_url: target_url, excluded_urls: excluded_urls) }
+    subject { described_class.new(project, user).execute(**params) }
 
     let(:status) { subject.status }
     let(:message) { subject.message }
@@ -48,6 +66,12 @@ RSpec.describe DastSiteProfiles::CreateService do
         expect { subject }.to change(DastSite, :count).by(1)
       end
 
+      it 'sets attributes correctly' do
+        expect(payload).to have_attributes(
+          params.except(:request_headers, :auth_password, :target_url).merge(dast_site: have_attributes(url: target_url))
+        )
+      end
+
       it 'returns a dast_site_profile payload' do
         expect(payload).to be_a(DastSiteProfile)
       end
@@ -87,13 +111,74 @@ RSpec.describe DastSiteProfiles::CreateService do
       end
 
       context 'when excluded_urls is not supplied' do
-        subject { described_class.new(project, user).execute(name: name, target_url: target_url) }
+        let(:params) { default_params.except(:excluded_urls) }
 
         it 'defaults to an empty array' do
           expect(payload.excluded_urls).to be_empty
         end
       end
 
+      context 'when auth values are not supplied' do
+        let(:params) { default_params.except(:auth_enabled, :auth_url, :auth_username_field, :auth_password_field, :auth_password_field, :auth_username) }
+
+        it 'uses sensible defaults' do
+          expect(payload).to have_attributes(
+            auth_enabled: false,
+            auth_url: nil,
+            auth_username_field: nil,
+            auth_password_field: nil,
+            auth_username: nil
+          )
+        end
+      end
+
+      shared_examples 'it handles secret variable creation' 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 create a secret variable' do
+            stub_feature_flags(security_dast_site_profiles_additional_fields: false)
+
+            expect { subject }.not_to change { Dast::SiteProfileSecretVariable.count }
+          end
+        end
+      end
+
+      shared_examples 'it handles secret variable creation 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
+
+      context 'when request_headers are supplied' do
+        let(:key) { 'DAST_REQUEST_HEADERS_BASE64' }
+        let(:raw_value) { params[:request_headers] }
+
+        it_behaves_like 'it handles secret variable creation'
+        it_behaves_like 'it handles secret variable creation failure'
+      end
+
+      context 'when auth_password is supplied' do
+        let(:key) { 'DAST_PASSWORD_BASE64' }
+        let(:raw_value) { params[:auth_password] }
+
+        it_behaves_like 'it handles secret variable creation'
+        it_behaves_like 'it handles secret variable creation failure'
+      end
+
       context 'when on demand scan licensed feature is not available' do
         before do
           stub_licensed_features(security_on_demand_scans: false)