Commit 87f877e7 authored by Lin Jen-Shin's avatar Lin Jen-Shin

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

Allow additional DAST scan config to be updated

See merge request gitlab-org/gitlab!57310
parents d6f08ed8 c8c71ac0
......@@ -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
......
# 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
......@@ -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
......
......@@ -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
......
......@@ -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
......
# 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
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment