From 0225b0e40d306e19ca4ff860551b6cbf3faa7902 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Sat, 22 May 2021 21:20:23 +0200 Subject: [PATCH 1/8] Audit DAST site profile actions This commit only adds auditing for DAST site profile creation, but I'm using it to create the changelog so the commit subject is a bit misleading. Changelog: added EE: true --- .../dast/site_profiles/create_service.rb | 12 +++++++++++ .../dast/site_profiles/create_service_spec.rb | 21 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/ee/app/services/app_sec/dast/site_profiles/create_service.rb b/ee/app/services/app_sec/dast/site_profiles/create_service.rb index 727fba940cbf2d..c274ffc67f5206 100644 --- a/ee/app/services/app_sec/dast/site_profiles/create_service.rb +++ b/ee/app/services/app_sec/dast/site_profiles/create_service.rb @@ -28,6 +28,8 @@ def execute(name:, target_url:, **params) create_secret_variable!(::Dast::SiteProfileSecretVariable::PASSWORD, params[:auth_password]) create_secret_variable!(::Dast::SiteProfileSecretVariable::REQUEST_HEADERS, params[:request_headers]) + create_audit_event + ServiceResponse.success(payload: dast_site_profile) end rescue Rollback => e @@ -68,6 +70,16 @@ def find_existing_dast_site_validation url_base: url_base ).execute.first end + + def create_audit_event + ::Gitlab::Audit::Auditor.audit( + name: 'dast_site_profile_create', + author: current_user, + scope: project, + target: dast_site_profile, + message: "Added DAST site profile" + ) + end end end end diff --git a/ee/spec/services/app_sec/dast/site_profiles/create_service_spec.rb b/ee/spec/services/app_sec/dast/site_profiles/create_service_spec.rb index c622976cddd8be..7b16fc65965421 100644 --- a/ee/spec/services/app_sec/dast/site_profiles/create_service_spec.rb +++ b/ee/spec/services/app_sec/dast/site_profiles/create_service_spec.rb @@ -76,6 +76,27 @@ expect(payload).to be_a(DastSiteProfile) end + it 'audits the creation' do + profile = payload + + audit_event = AuditEvent.last + + aggregate_failures do + expect(audit_event.author).to eq(user) + expect(audit_event.entity).to eq(project) + expect(audit_event.target_id).to eq(profile.id) + expect(audit_event.target_type).to eq('DastSiteProfile') + expect(audit_event.target_details).to eq(profile.name) + expect(audit_event.details).to eq({ + author_name: user.name, + custom_message: 'Added DAST site profile', + target_id: profile.id, + target_type: 'DastSiteProfile', + target_details: profile.name + }) + end + end + context 'when the dast_site already exists' do before do create(:dast_site, project: project, url: target_url) -- GitLab From 755c10d52dffa575e96bf892dc63123e06a22474 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Sat, 22 May 2021 21:29:53 +0200 Subject: [PATCH 2/8] Audit DAST site profile deletion --- .../dast/site_profiles/destroy_service.rb | 12 +++++++++++ .../site_profiles/destroy_service_spec.rb | 21 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/ee/app/services/app_sec/dast/site_profiles/destroy_service.rb b/ee/app/services/app_sec/dast/site_profiles/destroy_service.rb index 50a8bab97c7079..bb85cf1cf3cab6 100644 --- a/ee/app/services/app_sec/dast/site_profiles/destroy_service.rb +++ b/ee/app/services/app_sec/dast/site_profiles/destroy_service.rb @@ -14,6 +14,8 @@ def execute(id:) return ServiceResponse.error(message: _('Cannot delete %{profile_name} referenced in security policy') % { profile_name: dast_site_profile.name }) if referenced_in_security_policy?(dast_site_profile) if dast_site_profile.destroy + create_audit_event(dast_site_profile) + ServiceResponse.success(payload: dast_site_profile) else ServiceResponse.error(message: _('Site profile failed to delete')) @@ -37,6 +39,16 @@ def can_delete_site_profile? def find_dast_site_profile(id) project.dast_site_profiles.id_in(id).first end + + def create_audit_event(profile) + ::Gitlab::Audit::Auditor.audit( + name: 'dast_site_profile_destroy', + author: current_user, + scope: project, + target: profile, + message: "Removed DAST site profile" + ) + end end end end diff --git a/ee/spec/services/app_sec/dast/site_profiles/destroy_service_spec.rb b/ee/spec/services/app_sec/dast/site_profiles/destroy_service_spec.rb index 6425b57c775380..18c6d3b15649df 100644 --- a/ee/spec/services/app_sec/dast/site_profiles/destroy_service_spec.rb +++ b/ee/spec/services/app_sec/dast/site_profiles/destroy_service_spec.rb @@ -51,6 +51,27 @@ expect(payload).to be_a(DastSiteProfile) end + it 'audits the deletion' do + profile = payload + + audit_event = AuditEvent.last + + aggregate_failures do + expect(audit_event.author).to eq(user) + expect(audit_event.entity).to eq(project) + expect(audit_event.target_id).to eq(profile.id) + expect(audit_event.target_type).to eq('DastSiteProfile') + expect(audit_event.target_details).to eq(profile.name) + expect(audit_event.details).to eq({ + author_name: user.name, + custom_message: 'Removed DAST site profile', + target_id: profile.id, + target_type: 'DastSiteProfile', + target_details: profile.name + }) + end + end + context 'when the dast_site_profile does not exist' do let(:dast_site_profile_id) do Gitlab::GlobalId.build(nil, model_name: 'DastSiteProfile', id: 'does_not_exist') -- GitLab From ab28d878d9ed5de428f4499e3d3d0270793a82b2 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Tue, 25 May 2021 14:32:03 +0200 Subject: [PATCH 3/8] Audit DAST site profile updates The audit event messages remove details about changes to the request_headers and auth_password fields, since those fields are secret --- .../dast/site_profiles/update_service.rb | 38 +++++++- .../dast/site_profiles/update_service_spec.rb | 94 ++++++++++++++++++- 2 files changed, 130 insertions(+), 2 deletions(-) diff --git a/ee/app/services/app_sec/dast/site_profiles/update_service.rb b/ee/app/services/app_sec/dast/site_profiles/update_service.rb index 4d5cf8c0d7434c..6794bc8980dbfa 100644 --- a/ee/app/services/app_sec/dast/site_profiles/update_service.rb +++ b/ee/app/services/app_sec/dast/site_profiles/update_service.rb @@ -22,6 +22,11 @@ def execute(id:, **params) return ServiceResponse.error(message: _('Cannot modify %{profile_name} referenced in security policy') % { profile_name: dast_site_profile.name }) if referenced_in_security_policy? ActiveRecord::Base.transaction do + params_for_audit = params.dup + old_params = dast_site_profile.attributes.symbolize_keys.merge( + target_url: dast_site_profile.dast_site.url + ) + if target_url = params.delete(:target_url) params[:dast_site] = DastSites::FindOrCreateService.new(project, current_user).execute!(url: target_url) end @@ -30,8 +35,8 @@ def execute(id:, **params) handle_secret_variable!(params, :auth_password, ::Dast::SiteProfileSecretVariable::PASSWORD) params.compact! - dast_site_profile.update!(params) + create_audit_events(params_for_audit, old_params) ServiceResponse.success(payload: dast_site_profile) end @@ -93,6 +98,37 @@ def delete_secret_variable!(key) response end # rubocop: enable CodeReuse/ActiveRecord + + def create_audit_events(params, old_params) + params.each do |property, new_value| + old_value = old_params[property] + + next if old_value == new_value + + ::Gitlab::Audit::Auditor.audit( + name: 'dast_site_profile_update', + author: current_user, + scope: project, + target: dast_site_profile, + message: audit_message(property, new_value, old_value) + ) + end + end + + def audit_message(property, new_value, old_value) + case property + when :auth_password || :request_headers + "Changed DAST site profile #{property} (secret value omitted)" + when :excluded_urls + "Changed DAST site profile #{property} from #{urls_to_s(old_value)} to #{urls_to_s(new_value)}" + else + "Changed DAST site profile #{property} from #{old_value} to #{new_value}" + end + end + + def urls_to_s(urls) + [urls].flatten.join(', ') + end end end end diff --git a/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb b/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb index aaeb0ce2c6841e..04424da6e171a2 100644 --- a/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb +++ b/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb @@ -17,6 +17,7 @@ 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_it_be(:new_auth_username) { generate(:email) } let(:default_params) do { @@ -29,7 +30,7 @@ auth_url: new_auth_url, auth_username_field: 'login[username]', auth_password_field: 'login[password]', - auth_username: generate(:email), + auth_username: new_auth_username, auth_password: new_auth_password } end @@ -81,6 +82,97 @@ expect(payload).to be_a(DastSiteProfile) end + it 'audits the update' do + profile = payload.reload + audit_events = AuditEvent.all + audit_events_details = audit_events.map(&:details) + + aggregate_failures do + expect(audit_events.count).to be(9) + + audit_events.each do |event| + expect(event.author).to eq(user) + expect(event.entity).to eq(project) + expect(event.target_id).to eq(profile.id) + expect(event.target_type).to eq('DastSiteProfile') + expect(event.target_details).to eq(profile.name) + end + + expect(audit_events_details).to include( + a_hash_including( + author_name: user.name, + custom_message: "Changed DAST site profile name from #{dast_profile.name} to #{new_profile_name}", + target_id: profile.id, + target_type: 'DastSiteProfile', + target_details: new_profile_name + ) + ) + expect(audit_events_details).to include( + a_hash_including( + author_name: user.name, + custom_message: "Changed DAST site profile target_url from #{dast_profile.dast_site.url} to #{new_target_url}", + target_id: profile.id, + target_type: 'DastSiteProfile', + target_details: new_profile_name + ) + ) + expect(audit_events_details).to include( + a_hash_including( + author_name: user.name, + custom_message: "Changed DAST site profile excluded_urls from #{dast_profile.excluded_urls.join(', ')} to #{new_excluded_urls.join(', ')}", + target_id: profile.id, + target_type: 'DastSiteProfile', + target_details: new_profile_name + ) + ) + expect(audit_events_details).to include( + a_hash_including( + author_name: user.name, + custom_message: "Changed DAST site profile auth_url from #{dast_profile.auth_url} to #{new_auth_url}", + target_id: profile.id, + target_type: 'DastSiteProfile', + target_details: new_profile_name + ) + ) + expect(audit_events_details).to include( + a_hash_including( + author_name: user.name, + custom_message: "Changed DAST site profile auth_username_field from #{dast_profile.auth_username_field} to login[username]", + target_id: profile.id, + target_type: 'DastSiteProfile', + target_details: new_profile_name + ) + ) + expect(audit_events_details).to include( + a_hash_including( + author_name: user.name, + custom_message: "Changed DAST site profile auth_password_field from #{dast_profile.auth_password_field} to login[password]", + target_id: profile.id, + target_type: 'DastSiteProfile', + target_details: new_profile_name + ) + ) + expect(audit_events_details).to include( + a_hash_including( + author_name: user.name, + custom_message: "Changed DAST site profile auth_username from #{dast_profile.auth_username} to #{new_auth_username}", + target_id: profile.id, + target_type: 'DastSiteProfile', + target_details: new_profile_name + ) + ) + expect(audit_events_details).to include( + a_hash_including( + author_name: user.name, + custom_message: "Changed DAST site profile auth_password (secret value omitted)", + target_id: profile.id, + target_type: 'DastSiteProfile', + target_details: new_profile_name + ) + ) + end + end + context 'when the target url is localhost' do let(:new_target_url) { 'http://localhost:3000/hello-world' } -- GitLab From 0085ce24574baf906f4975fd8b4306326ac9e6f0 Mon Sep 17 00:00:00 2001 From: Philip Cunningham Date: Wed, 26 May 2021 08:27:37 +0000 Subject: [PATCH 4/8] Apply 2 suggestion(s) to 2 file(s) --- ee/app/services/app_sec/dast/site_profiles/create_service.rb | 2 +- ee/app/services/app_sec/dast/site_profiles/destroy_service.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/services/app_sec/dast/site_profiles/create_service.rb b/ee/app/services/app_sec/dast/site_profiles/create_service.rb index c274ffc67f5206..78142a0493c192 100644 --- a/ee/app/services/app_sec/dast/site_profiles/create_service.rb +++ b/ee/app/services/app_sec/dast/site_profiles/create_service.rb @@ -77,7 +77,7 @@ def create_audit_event author: current_user, scope: project, target: dast_site_profile, - message: "Added DAST site profile" + message: 'Added DAST site profile' ) end end diff --git a/ee/app/services/app_sec/dast/site_profiles/destroy_service.rb b/ee/app/services/app_sec/dast/site_profiles/destroy_service.rb index bb85cf1cf3cab6..dfbd9a0b4db564 100644 --- a/ee/app/services/app_sec/dast/site_profiles/destroy_service.rb +++ b/ee/app/services/app_sec/dast/site_profiles/destroy_service.rb @@ -46,7 +46,7 @@ def create_audit_event(profile) author: current_user, scope: project, target: profile, - message: "Removed DAST site profile" + message: 'Removed DAST site profile' ) end end -- GitLab From 690dbc5e1ce5c3bf54f51c80159c346e1b5eafc6 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Wed, 26 May 2021 11:33:29 +0200 Subject: [PATCH 5/8] Exclude excluded_urls value from message The value could be quite long, which would not provide a good user experience --- .../app_sec/dast/site_profiles/update_service.rb | 12 ++++++------ .../dast/site_profiles/update_service_spec.rb | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/ee/app/services/app_sec/dast/site_profiles/update_service.rb b/ee/app/services/app_sec/dast/site_profiles/update_service.rb index 6794bc8980dbfa..17d36e262e6b48 100644 --- a/ee/app/services/app_sec/dast/site_profiles/update_service.rb +++ b/ee/app/services/app_sec/dast/site_profiles/update_service.rb @@ -103,7 +103,11 @@ def create_audit_events(params, old_params) params.each do |property, new_value| old_value = old_params[property] - next if old_value == new_value + if new_value.is_a?(Array) + next if old_value.sort == new_value.sort + else + next if old_value == new_value + end ::Gitlab::Audit::Auditor.audit( name: 'dast_site_profile_update', @@ -120,15 +124,11 @@ def audit_message(property, new_value, old_value) when :auth_password || :request_headers "Changed DAST site profile #{property} (secret value omitted)" when :excluded_urls - "Changed DAST site profile #{property} from #{urls_to_s(old_value)} to #{urls_to_s(new_value)}" + "Changed DAST site profile #{property} (long value omitted)" else "Changed DAST site profile #{property} from #{old_value} to #{new_value}" end end - - def urls_to_s(urls) - [urls].flatten.join(', ') - end end end end diff --git a/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb b/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb index 04424da6e171a2..0e015a5dd9ea3b 100644 --- a/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb +++ b/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb @@ -119,7 +119,7 @@ expect(audit_events_details).to include( a_hash_including( author_name: user.name, - custom_message: "Changed DAST site profile excluded_urls from #{dast_profile.excluded_urls.join(', ')} to #{new_excluded_urls.join(', ')}", + custom_message: 'Changed DAST site profile excluded_urls (long value omitted)', target_id: profile.id, target_type: 'DastSiteProfile', target_details: new_profile_name -- GitLab From 8b94a2eeee89480dc18ca33da81274a0691cb150 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Wed, 26 May 2021 12:29:23 +0200 Subject: [PATCH 6/8] Extract update audit logic to a new service This neatly encapsulates the logic, which is too complex to keep in the site profile update service --- .../site_profiles/audit/update_service.rb | 56 +++++++++++++++ .../dast/site_profiles/update_service.rb | 44 +++--------- .../audit/update_service_spec.rb | 72 +++++++++++++++++++ 3 files changed, 136 insertions(+), 36 deletions(-) create mode 100644 ee/app/services/app_sec/dast/site_profiles/audit/update_service.rb create mode 100644 ee/spec/services/app_sec/dast/site_profiles/audit/update_service_spec.rb diff --git a/ee/app/services/app_sec/dast/site_profiles/audit/update_service.rb b/ee/app/services/app_sec/dast/site_profiles/audit/update_service.rb new file mode 100644 index 00000000000000..593e3227b598ef --- /dev/null +++ b/ee/app/services/app_sec/dast/site_profiles/audit/update_service.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module AppSec + module Dast + module SiteProfiles + module Audit + class UpdateService < BaseService + def execute + new_params.each do |property, new_value| + old_value = old_params[property] + + if new_value.is_a?(Array) + next if old_value.sort == new_value.sort + else + next if old_value == new_value + end + + ::Gitlab::Audit::Auditor.audit( + name: 'dast_site_profile_update', + author: current_user, + scope: project, + target: dast_site_profile, + message: audit_message(property, new_value, old_value) + ) + end + end + + private + + def dast_site_profile + params[:dast_site_profile] + end + + def new_params + params[:new_params] + end + + def old_params + params[:old_params] + end + + def audit_message(property, new_value, old_value) + case property + when :auth_password, :request_headers + "Changed DAST site profile #{property} (secret value omitted)" + when :excluded_urls + "Changed DAST site profile #{property} (long value omitted)" + else + "Changed DAST site profile #{property} from #{old_value} to #{new_value}" + end + end + end + end + end + end +end diff --git a/ee/app/services/app_sec/dast/site_profiles/update_service.rb b/ee/app/services/app_sec/dast/site_profiles/update_service.rb index 17d36e262e6b48..7a1c2510026938 100644 --- a/ee/app/services/app_sec/dast/site_profiles/update_service.rb +++ b/ee/app/services/app_sec/dast/site_profiles/update_service.rb @@ -22,10 +22,13 @@ def execute(id:, **params) return ServiceResponse.error(message: _('Cannot modify %{profile_name} referenced in security policy') % { profile_name: dast_site_profile.name }) if referenced_in_security_policy? ActiveRecord::Base.transaction do - params_for_audit = params.dup - old_params = dast_site_profile.attributes.symbolize_keys.merge( - target_url: dast_site_profile.dast_site.url - ) + auditor = Audit::UpdateService.new(project, current_user, { + dast_site_profile: dast_site_profile, + new_params: params.dup, + old_params: dast_site_profile.attributes.symbolize_keys.merge( + target_url: dast_site_profile.dast_site.url + ) + }) if target_url = params.delete(:target_url) params[:dast_site] = DastSites::FindOrCreateService.new(project, current_user).execute!(url: target_url) @@ -36,7 +39,7 @@ def execute(id:, **params) params.compact! dast_site_profile.update!(params) - create_audit_events(params_for_audit, old_params) + auditor.execute ServiceResponse.success(payload: dast_site_profile) end @@ -98,37 +101,6 @@ def delete_secret_variable!(key) response end # rubocop: enable CodeReuse/ActiveRecord - - def create_audit_events(params, old_params) - params.each do |property, new_value| - old_value = old_params[property] - - if new_value.is_a?(Array) - next if old_value.sort == new_value.sort - else - next if old_value == new_value - end - - ::Gitlab::Audit::Auditor.audit( - name: 'dast_site_profile_update', - author: current_user, - scope: project, - target: dast_site_profile, - message: audit_message(property, new_value, old_value) - ) - end - end - - def audit_message(property, new_value, old_value) - case property - when :auth_password || :request_headers - "Changed DAST site profile #{property} (secret value omitted)" - when :excluded_urls - "Changed DAST site profile #{property} (long value omitted)" - else - "Changed DAST site profile #{property} from #{old_value} to #{new_value}" - end - end end end end diff --git a/ee/spec/services/app_sec/dast/site_profiles/audit/update_service_spec.rb b/ee/spec/services/app_sec/dast/site_profiles/audit/update_service_spec.rb new file mode 100644 index 00000000000000..5a8c6ca9dd9501 --- /dev/null +++ b/ee/spec/services/app_sec/dast/site_profiles/audit/update_service_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AppSec::Dast::SiteProfiles::Audit::UpdateService do + let_it_be(:profile) { create(:dast_site_profile) } + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + describe '#execute' do + it 'audits the changes in the given properties', :aggregate_failures do + auditor = described_class.new(project, user, { + dast_site_profile: profile, + new_params: { name: 'Updated DAST profile' }, + old_params: { name: 'Old DAST profile' } + }) + + auditor.execute + + audit_event = AuditEvent.last + expect(audit_event.author).to eq(user) + expect(audit_event.entity).to eq(project) + expect(audit_event.target_id).to eq(profile.id) + expect(audit_event.target_type).to eq('DastSiteProfile') + expect(audit_event.details[:custom_message]).to eq( + 'Changed DAST site profile name from Old DAST profile to Updated DAST profile' + ) + end + + it 'omits the values for secret properties' do + auditor = described_class.new(project, user, { + dast_site_profile: profile, + new_params: { auth_password: 'newpassword', request_headers: 'A new header' }, + old_params: { auth_password: 'oldpassword', request_headers: 'An old header' } + }) + + auditor.execute + + audit_events = AuditEvent.all + audit_events_messages = audit_events.map(&:details).pluck(:custom_message) + expect(audit_events_messages).to contain_exactly( + 'Changed DAST site profile auth_password (secret value omitted)', + 'Changed DAST site profile request_headers (secret value omitted)' + ) + end + + it 'omits the values for properties too long to be displayed' do + auditor = described_class.new(project, user, { + dast_site_profile: profile, + new_params: { excluded_urls: ['https://target.test/signout'] }, + old_params: { excluded_urls: ['https://target.test/signin'] } + }) + + auditor.execute + + audit_event = AuditEvent.last + expect(audit_event.details[:custom_message]).to eq( + 'Changed DAST site profile excluded_urls (long value omitted)' + ) + end + + it 'sorts properties that are arrays before comparing them' do + auditor = described_class.new(project, user, { + dast_site_profile: profile, + new_params: { excluded_urls: ['https://target.test/signin', 'https://target.test/signout'] }, + old_params: { excluded_urls: ['https://target.test/signout', 'https://target.test/signin'] } + }) + + expect { auditor.execute }.not_to change { AuditEvent.count } + end + end +end -- GitLab From 8f9b2e44241cdc682d2ee8cc560314f433d97af8 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Wed, 16 Jun 2021 15:53:34 +0200 Subject: [PATCH 7/8] Prevent spec flakiness Relying on .last and .all can lead to flaky specs on CI --- .../app_sec/dast/site_profiles/audit/update_service_spec.rb | 6 +++--- .../app_sec/dast/site_profiles/create_service_spec.rb | 2 +- .../app_sec/dast/site_profiles/destroy_service_spec.rb | 2 +- .../app_sec/dast/site_profiles/update_service_spec.rb | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ee/spec/services/app_sec/dast/site_profiles/audit/update_service_spec.rb b/ee/spec/services/app_sec/dast/site_profiles/audit/update_service_spec.rb index 5a8c6ca9dd9501..ee09b005126644 100644 --- a/ee/spec/services/app_sec/dast/site_profiles/audit/update_service_spec.rb +++ b/ee/spec/services/app_sec/dast/site_profiles/audit/update_service_spec.rb @@ -17,7 +17,7 @@ auditor.execute - audit_event = AuditEvent.last + audit_event = AuditEvent.find_by(author_id: user.id) expect(audit_event.author).to eq(user) expect(audit_event.entity).to eq(project) expect(audit_event.target_id).to eq(profile.id) @@ -36,7 +36,7 @@ auditor.execute - audit_events = AuditEvent.all + audit_events = AuditEvent.where(author_id: user.id) audit_events_messages = audit_events.map(&:details).pluck(:custom_message) expect(audit_events_messages).to contain_exactly( 'Changed DAST site profile auth_password (secret value omitted)', @@ -53,7 +53,7 @@ auditor.execute - audit_event = AuditEvent.last + audit_event = AuditEvent.find_by(author_id: user.id) expect(audit_event.details[:custom_message]).to eq( 'Changed DAST site profile excluded_urls (long value omitted)' ) diff --git a/ee/spec/services/app_sec/dast/site_profiles/create_service_spec.rb b/ee/spec/services/app_sec/dast/site_profiles/create_service_spec.rb index 7b16fc65965421..5586357d8f3498 100644 --- a/ee/spec/services/app_sec/dast/site_profiles/create_service_spec.rb +++ b/ee/spec/services/app_sec/dast/site_profiles/create_service_spec.rb @@ -79,7 +79,7 @@ it 'audits the creation' do profile = payload - audit_event = AuditEvent.last + audit_event = AuditEvent.find_by(author_id: user.id) aggregate_failures do expect(audit_event.author).to eq(user) diff --git a/ee/spec/services/app_sec/dast/site_profiles/destroy_service_spec.rb b/ee/spec/services/app_sec/dast/site_profiles/destroy_service_spec.rb index 18c6d3b15649df..3178fda02c50b2 100644 --- a/ee/spec/services/app_sec/dast/site_profiles/destroy_service_spec.rb +++ b/ee/spec/services/app_sec/dast/site_profiles/destroy_service_spec.rb @@ -54,7 +54,7 @@ it 'audits the deletion' do profile = payload - audit_event = AuditEvent.last + audit_event = AuditEvent.find_by(author_id: user.id) aggregate_failures do expect(audit_event.author).to eq(user) diff --git a/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb b/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb index 0e015a5dd9ea3b..62402d592cf47b 100644 --- a/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb +++ b/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb @@ -84,7 +84,7 @@ it 'audits the update' do profile = payload.reload - audit_events = AuditEvent.all + audit_events = AuditEvent.where(author_id: user.id) audit_events_details = audit_events.map(&:details) aggregate_failures do -- GitLab From 6db9edef4743882937e263c4f93c09d334fecfe0 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 17 Jun 2021 16:01:48 +0200 Subject: [PATCH 8/8] Reduce duplication in spec expectations --- .../dast/site_profiles/update_service_spec.rb | 86 +++---------------- 1 file changed, 13 insertions(+), 73 deletions(-) diff --git a/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb b/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb index 62402d592cf47b..44bba6ab55d0c5 100644 --- a/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb +++ b/ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb @@ -85,7 +85,6 @@ it 'audits the update' do profile = payload.reload audit_events = AuditEvent.where(author_id: user.id) - audit_events_details = audit_events.map(&:details) aggregate_failures do expect(audit_events.count).to be(9) @@ -98,78 +97,19 @@ expect(event.target_details).to eq(profile.name) end - expect(audit_events_details).to include( - a_hash_including( - author_name: user.name, - custom_message: "Changed DAST site profile name from #{dast_profile.name} to #{new_profile_name}", - target_id: profile.id, - target_type: 'DastSiteProfile', - target_details: new_profile_name - ) - ) - expect(audit_events_details).to include( - a_hash_including( - author_name: user.name, - custom_message: "Changed DAST site profile target_url from #{dast_profile.dast_site.url} to #{new_target_url}", - target_id: profile.id, - target_type: 'DastSiteProfile', - target_details: new_profile_name - ) - ) - expect(audit_events_details).to include( - a_hash_including( - author_name: user.name, - custom_message: 'Changed DAST site profile excluded_urls (long value omitted)', - target_id: profile.id, - target_type: 'DastSiteProfile', - target_details: new_profile_name - ) - ) - expect(audit_events_details).to include( - a_hash_including( - author_name: user.name, - custom_message: "Changed DAST site profile auth_url from #{dast_profile.auth_url} to #{new_auth_url}", - target_id: profile.id, - target_type: 'DastSiteProfile', - target_details: new_profile_name - ) - ) - expect(audit_events_details).to include( - a_hash_including( - author_name: user.name, - custom_message: "Changed DAST site profile auth_username_field from #{dast_profile.auth_username_field} to login[username]", - target_id: profile.id, - target_type: 'DastSiteProfile', - target_details: new_profile_name - ) - ) - expect(audit_events_details).to include( - a_hash_including( - author_name: user.name, - custom_message: "Changed DAST site profile auth_password_field from #{dast_profile.auth_password_field} to login[password]", - target_id: profile.id, - target_type: 'DastSiteProfile', - target_details: new_profile_name - ) - ) - expect(audit_events_details).to include( - a_hash_including( - author_name: user.name, - custom_message: "Changed DAST site profile auth_username from #{dast_profile.auth_username} to #{new_auth_username}", - target_id: profile.id, - target_type: 'DastSiteProfile', - target_details: new_profile_name - ) - ) - expect(audit_events_details).to include( - a_hash_including( - author_name: user.name, - custom_message: "Changed DAST site profile auth_password (secret value omitted)", - target_id: profile.id, - target_type: 'DastSiteProfile', - target_details: new_profile_name - ) - ) + custom_messages = audit_events.map(&:details).pluck(:custom_message) + expected_custom_messages = [ + "Changed DAST site profile name from #{dast_profile.name} to #{new_profile_name}", + "Changed DAST site profile target_url from #{dast_profile.dast_site.url} to #{new_target_url}", + 'Changed DAST site profile excluded_urls (long value omitted)', + "Changed DAST site profile auth_url from #{dast_profile.auth_url} to #{new_auth_url}", + "Changed DAST site profile auth_username_field from #{dast_profile.auth_username_field} to login[username]", + "Changed DAST site profile auth_password_field from #{dast_profile.auth_password_field} to login[password]", + "Changed DAST site profile auth_username from #{dast_profile.auth_username} to #{new_auth_username}", + "Changed DAST site profile auth_password (secret value omitted)", + "Changed DAST site profile request_headers (secret value omitted)" + ] + expect(custom_messages).to match_array(expected_custom_messages) end end -- GitLab