From 53b0e0cbb4a7659499ef79f603174e35cbcb7884 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Mon, 10 Jul 2023 12:13:43 +0530 Subject: [PATCH 1/7] Added audit events for update destroy destinations --- .../base.rb | 4 ++-- .../destroy.rb | 3 ++- .../update.rb | 13 ++++++++++++- .../destroy_spec.rb | 13 +++++++++++++ .../update_spec.rb | 18 +++++++++++++++++- 5 files changed, 46 insertions(+), 5 deletions(-) diff --git a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb index fc68738d118024..eaccbf168e5d98 100644 --- a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb +++ b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb @@ -26,7 +26,7 @@ def find_object(destination_gid) destination end - def audit(destination, action:) + def audit(destination, action:, extra_context: {}) audit_context = { name: "#{action}_instance_event_streaming_destination", author: current_user, @@ -35,7 +35,7 @@ def audit(destination, action:) message: "#{action.capitalize} instance event streaming destination #{destination.destination_url}" } - ::Gitlab::Audit::Auditor.audit(audit_context) + ::Gitlab::Audit::Auditor.audit(audit_context.merge(extra_context)) end end end diff --git a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/destroy.rb b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/destroy.rb index 36093d8861645d..e87c56e187ece5 100644 --- a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/destroy.rb +++ b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/destroy.rb @@ -13,7 +13,8 @@ class Destroy < Base description: 'ID of the external instance audit event destination to destroy.' def resolve(id:) - find_object(id).destroy + destination = find_object(id) + audit(destination, action: :destroy) if destination.destroy { instance_external_audit_event_destination: nil, diff --git a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb index 944a83549d12e3..7009af329e1079 100644 --- a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb +++ b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb @@ -24,13 +24,24 @@ class Update < Base def resolve(id:, destination_url:) destination = find_object(id) - destination.update(destination_url: destination_url) + audit_update(destination) if destination.update(destination_url: destination_url) { instance_external_audit_event_destination: (destination if destination.persisted?), errors: Array(destination.errors) } end + + private + + def audit_update(destination) + return unless destination.previous_changes.any? + + message = "Updated instance event streaming destination from #{destination.previous_changes['destination_url'] + .join(' to ')}" + + audit(destination, action: :update, extra_context: { message: message }) + end end end end diff --git a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/destroy_spec.rb b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/destroy_spec.rb index d76d607803c049..9d76342a9fd8dc 100644 --- a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/destroy_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/destroy_spec.rb @@ -29,6 +29,11 @@ it_behaves_like 'a mutation that returns top-level errors', errors: ['You do not have access to this mutation.'] + + it 'does not audit the destruction' do + expect { post_graphql_mutation(mutation, current_user: current_user) } + .not_to change { AuditEvent.count } + end end context 'when feature is licensed' do @@ -43,6 +48,14 @@ .to change { AuditEvents::InstanceExternalAuditEventDestination.count }.by(-1) end + it 'audits the destruction' do + expect { post_graphql_mutation(mutation, current_user: admin) } + .to change { AuditEvent.count }.by(1) + + expect(AuditEvent.last.details[:custom_message]) + .to eq("Destroy instance event streaming destination #{destination.destination_url}") + end + context 'when the destination id is invalid' do let_it_be(:invalid_destination_input) do { diff --git a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb index 0bd0f16a45e55c..c0546cfa2b93f1 100644 --- a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb @@ -7,7 +7,7 @@ let_it_be(:admin) { create(:admin) } let_it_be(:user) { create(:user) } - let_it_be(:destination) { create(:instance_external_audit_event_destination) } + let_it_be_with_reload(:destination) { create(:instance_external_audit_event_destination) } let_it_be(:destination_url) { 'https://example.com/test' } let(:input) do @@ -33,6 +33,11 @@ it_behaves_like 'a mutation that returns top-level errors', errors: ['You do not have access to this mutation.'] + + it 'does not audit the update' do + expect { post_graphql_mutation(mutation, current_user: current_user) } + .not_to change { AuditEvent.count } + end end context 'when feature is licensed' do @@ -52,6 +57,17 @@ expect(mutation_response['instanceExternalAuditEventDestination']['verificationToken']).not_to be_empty end + it 'audits the update' do + expect { post_graphql_mutation(mutation, current_user: admin) } + .to change { AuditEvent.count }.by(1) + + expected_message = "Updated instance event streaming destination from #{destination.destination_url} " \ + "to #{destination.reload.destination_url}" + + expect(AuditEvent.last.details[:custom_message]) + .to eq(expected_message) + end + context 'when the destination id is invalid' do let_it_be(:invalid_destination_input) do { -- GitLab From ac91e4d855a8f489e9805ddf74ee4fa39b5ad4c3 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Wed, 12 Jul 2023 16:46:16 +0530 Subject: [PATCH 2/7] Added test case for same destination --- .../update_spec.rb | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb index c0546cfa2b93f1..e3943d10863a53 100644 --- a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb @@ -68,6 +68,27 @@ .to eq(expected_message) end + context 'when destination is same as previous one' do + let(:input) { super().merge(destinationUrl: destination.reload.destination_url) } + + it 'updates the destination with correct response' do + old_destination_url = destination.reload.destination_url + + expect { post_graphql_mutation(mutation, current_user: admin) } + .not_to change { destination.reload.destination_url } + + expect(mutation_response['errors']).to be_empty + expect(mutation_response['instanceExternalAuditEventDestination']['destinationUrl']).to eq(old_destination_url) + expect(mutation_response['instanceExternalAuditEventDestination']['id']).not_to be_empty + expect(mutation_response['instanceExternalAuditEventDestination']['verificationToken']).not_to be_empty + end + + it 'does not audit the update' do + expect { post_graphql_mutation(mutation, current_user: current_user) } + .not_to change { AuditEvent.count } + end + end + context 'when the destination id is invalid' do let_it_be(:invalid_destination_input) do { -- GitLab From 4d5063db5b3a286e664bf431121b860f7ccabb4c Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Wed, 12 Jul 2023 16:48:39 +0530 Subject: [PATCH 3/7] Fixed lint offence --- .../instance_external_audit_event_destinations/update_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb index e3943d10863a53..be53bf7a8493ee 100644 --- a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb @@ -78,7 +78,8 @@ .not_to change { destination.reload.destination_url } expect(mutation_response['errors']).to be_empty - expect(mutation_response['instanceExternalAuditEventDestination']['destinationUrl']).to eq(old_destination_url) + expect(mutation_response['instanceExternalAuditEventDestination']['destinationUrl']) + .to eq(old_destination_url) expect(mutation_response['instanceExternalAuditEventDestination']['id']).not_to be_empty expect(mutation_response['instanceExternalAuditEventDestination']['verificationToken']).not_to be_empty end -- GitLab From 642bb9b7cc22da7aebf62620a673b939e07139bd Mon Sep 17 00:00:00 2001 From: Aakriti Gupta Date: Wed, 12 Jul 2023 11:28:44 +0000 Subject: [PATCH 4/7] Adding readability suggestions --- .../instance_external_audit_event_destinations/update.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb index 7009af329e1079..b94322fa8fa7af 100644 --- a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb +++ b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb @@ -35,10 +35,10 @@ def resolve(id:, destination_url:) private def audit_update(destination) - return unless destination.previous_changes.any? + return unless destination.previous_changes.key?('destination_url') - message = "Updated instance event streaming destination from #{destination.previous_changes['destination_url'] - .join(' to ')}" + from_url, to_url = destination.previous_changes['destination_url'] + message = "Updated instance event streaming destination from #{from_url} to #{to_url}." audit(destination, action: :update, extra_context: { message: message }) end -- GitLab From e50a9c6ea8d8b604499b34fdbf6c985c149bda12 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Wed, 12 Jul 2023 12:10:11 +0000 Subject: [PATCH 5/7] Added fullstop to rspec --- .../instance_external_audit_event_destinations/update_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb index be53bf7a8493ee..d44aef9f6632e6 100644 --- a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb @@ -62,7 +62,7 @@ .to change { AuditEvent.count }.by(1) expected_message = "Updated instance event streaming destination from #{destination.destination_url} " \ - "to #{destination.reload.destination_url}" + "to #{destination.reload.destination_url}." expect(AuditEvent.last.details[:custom_message]) .to eq(expected_message) -- GitLab From b1d116afae72427a84fc6798433b128af743d18b Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Fri, 14 Jul 2023 14:29:32 +0530 Subject: [PATCH 6/7] Updated audit update method --- .../update.rb | 19 ++++-- ee/lib/audit/changes.rb | 2 +- .../update_spec.rb | 47 +------------- .../update_spec.rb | 28 +++------ .../streaming_destinations_shared_examples.rb | 61 +++++++++++++++++++ 5 files changed, 86 insertions(+), 71 deletions(-) create mode 100644 ee/spec/support/shared_examples/audit_events/streaming_destinations_shared_examples.rb diff --git a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb index 862177c6fc5b9b..fc8c39d6a1c96c 100644 --- a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb +++ b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb @@ -6,8 +6,12 @@ module InstanceExternalAuditEventDestinations class Update < Base graphql_name 'InstanceExternalAuditEventDestinationUpdate' + include ::Audit::Changes + authorize :admin_instance_external_audit_events + UPDATE_EVENT_NAME = 'update_event_instance_streaming_destination' + argument :id, ::Types::GlobalIDType[::AuditEvents::InstanceExternalAuditEventDestination], required: true, description: 'ID of the external instance audit event destination to update.' @@ -41,12 +45,15 @@ def resolve(id:, destination_url: nil, name: nil) private def audit_update(destination) - return unless destination.previous_changes.key?('destination_url') - - from_url, to_url = destination.previous_changes['destination_url'] - message = "Updated instance event streaming destination from #{from_url} to #{to_url}." - - audit(destination, action: :update, extra_context: { message: message }) + [:destination_url, :name].each do |column| + audit_changes( + column, + as: column.to_s, + entity: Gitlab::Audit::InstanceScope.new, + model: destination, + event_type: UPDATE_EVENT_NAME + ) + end end end end diff --git a/ee/lib/audit/changes.rb b/ee/lib/audit/changes.rb index 1c8333ea940170..24c36db5fc9e84 100644 --- a/ee/lib/audit/changes.rb +++ b/ee/lib/audit/changes.rb @@ -100,7 +100,7 @@ def audit_enabled? return true if ::License.feature_available?(:admin_audit_log) return true if ::License.feature_available?(:extended_audit_events) - entity.respond_to?(:feature_available?) && entity.licensed_feature_available?(:audit_events) + entity.respond_to?(:licensed_feature_available?) && entity.licensed_feature_available?(:audit_events) end end end diff --git a/ee/spec/requests/api/graphql/mutations/audit_events/external_audit_event_destinations/update_spec.rb b/ee/spec/requests/api/graphql/mutations/audit_events/external_audit_event_destinations/update_spec.rb index 59899c7f276ecd..d2db700dbd9176 100644 --- a/ee/spec/requests/api/graphql/mutations/audit_events/external_audit_event_destinations/update_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/audit_events/external_audit_event_destinations/update_spec.rb @@ -77,51 +77,8 @@ end.to change { destination.reload.name }.to("New Destination") end - context 'when both destination url and destination name are updated' do - it 'audits the update' do - expect { post_graphql_mutation(mutation, current_user: owner) } - .to change { AuditEvent.count }.by(2) - - audit_events = AuditEvent.last(2) - expect(audit_events[0].details[:custom_message]).to match("Changed destination_url " \ - "from https://example.com/old to https://example.com/new") - expect(audit_events[1].details[:custom_message]).to match("Changed name from Old Destination " \ - "to New Destination") - end - end - - context 'when only destination url is updated' do - let(:input) do - { - 'id': GitlabSchema.id_from_object(destination).to_s, - 'destinationUrl': "https://example.com/new" - } - end - - it 'audits the update' do - expect { post_graphql_mutation(mutation, current_user: owner) } - .to change { AuditEvent.count }.by(1) - - expect(AuditEvent.last.details[:custom_message]).to match("Changed destination_url from " \ - "https://example.com/old to https://example.com/new") - end - end - - context 'when only destination name is updated' do - let(:input) do - { - 'id': GitlabSchema.id_from_object(destination).to_s, - 'name': "New Destination" - } - end - - it 'audits the update' do - expect { post_graphql_mutation(mutation, current_user: owner) } - .to change { AuditEvent.count }.by(1) - - expect(AuditEvent.last.details[:custom_message]).to match("Changed name from Old Destination " \ - "to New Destination") - end + it_behaves_like 'audits update to external streaming destination' do + let_it_be(:current_user) { owner } end end diff --git a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb index b968480b5c0c3a..08b6d1c32daad2 100644 --- a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/update_spec.rb @@ -5,9 +5,15 @@ RSpec.describe 'Update an instance external audit event destination', feature_category: :audit_events do include GraphqlHelpers + let_it_be(:old_destination_url) { "https://example.com/old" } let_it_be(:admin) { create(:admin) } let_it_be(:user) { create(:user) } - let_it_be_with_reload(:destination) { create(:instance_external_audit_event_destination) } + let_it_be_with_reload(:destination) do + create(:instance_external_audit_event_destination, + name: "Old Destination", + destination_url: old_destination_url) + end + let_it_be(:destination_url) { 'https://example.com/test' } let_it_be(:name) { "My Destination" } @@ -60,23 +66,12 @@ expect(mutation_response['instanceExternalAuditEventDestination']['verificationToken']).not_to be_empty end - it 'audits the update' do - expect { post_graphql_mutation(mutation, current_user: admin) } - .to change { AuditEvent.count }.by(1) - - expected_message = "Updated instance event streaming destination from #{destination.destination_url} " \ - "to #{destination.reload.destination_url}." - - expect(AuditEvent.last.details[:custom_message]) - .to eq(expected_message) - end + it_behaves_like 'audits update to external streaming destination' context 'when destination is same as previous one' do - let(:input) { super().merge(destinationUrl: destination.reload.destination_url) } + let(:input) { super().merge(destinationUrl: old_destination_url) } it 'updates the destination with correct response' do - old_destination_url = destination.reload.destination_url - expect { post_graphql_mutation(mutation, current_user: admin) } .not_to change { destination.reload.destination_url } @@ -86,11 +81,6 @@ expect(mutation_response['instanceExternalAuditEventDestination']['id']).not_to be_empty expect(mutation_response['instanceExternalAuditEventDestination']['verificationToken']).not_to be_empty end - - it 'does not audit the update' do - expect { post_graphql_mutation(mutation, current_user: current_user) } - .not_to change { AuditEvent.count } - end end context 'when the destination id is invalid' do diff --git a/ee/spec/support/shared_examples/audit_events/streaming_destinations_shared_examples.rb b/ee/spec/support/shared_examples/audit_events/streaming_destinations_shared_examples.rb new file mode 100644 index 00000000000000..abfe865c007113 --- /dev/null +++ b/ee/spec/support/shared_examples/audit_events/streaming_destinations_shared_examples.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +# It expects a variable `current_user` which will be a user who has authorization for the resource , +# it maybe a group owner or instance admin as per the type of destination. +# It also assumes the old name and url of the destination are `Old Destination` and `https://example.com/old` +# respectively. +RSpec.shared_examples 'audits update to external streaming destination' do + context 'when both destination url and destination name are updated' do + let(:input) do + { + id: GitlabSchema.id_from_object(destination).to_s, + destinationUrl: "https://example.com/new", + name: "New Destination" + } + end + + it 'audits the update' do + expect { post_graphql_mutation(mutation, current_user: current_user) } + .to change { AuditEvent.count }.by(2) + + audit_events = AuditEvent.last(2) + expect(audit_events[0].details[:custom_message]) + .to match("Changed destination_url from https://example.com/old to https://example.com/new") + expect(audit_events[1].details[:custom_message]) + .to match("Changed name from Old Destination to New Destination") + end + end + + context 'when only destination url is updated' do + let(:input) do + { + id: GitlabSchema.id_from_object(destination).to_s, + destinationUrl: "https://example.com/new" + } + end + + it 'audits the update' do + expect { post_graphql_mutation(mutation, current_user: current_user) } + .to change { AuditEvent.count }.by(1) + + expect(AuditEvent.last.details[:custom_message]) + .to match("Changed destination_url from https://example.com/old to https://example.com/new") + end + end + + context 'when only destination name is updated' do + let(:input) do + { + id: GitlabSchema.id_from_object(destination).to_s, + name: "New Destination" + } + end + + it 'audits the update' do + expect { post_graphql_mutation(mutation, current_user: current_user) } + .to change { AuditEvent.count }.by(1) + + expect(AuditEvent.last.details[:custom_message]).to match("Changed name from Old Destination to New Destination") + end + end +end -- GitLab From ff94ab52a30a960ec61267618fe30fd22073109a Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Fri, 14 Jul 2023 14:45:35 +0530 Subject: [PATCH 7/7] Added audit event files --- .../instance_external_audit_event_destinations/update.rb | 2 +- .../destroy_instance_event_streaming_destination.yml | 8 ++++++++ .../types/update_instance_event_streaming_destination.yml | 8 ++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 ee/config/audit_events/types/destroy_instance_event_streaming_destination.yml create mode 100644 ee/config/audit_events/types/update_instance_event_streaming_destination.yml diff --git a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb index fc8c39d6a1c96c..b01e4c4f429509 100644 --- a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb +++ b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/update.rb @@ -10,7 +10,7 @@ class Update < Base authorize :admin_instance_external_audit_events - UPDATE_EVENT_NAME = 'update_event_instance_streaming_destination' + UPDATE_EVENT_NAME = 'update_instance_event_streaming_destination' argument :id, ::Types::GlobalIDType[::AuditEvents::InstanceExternalAuditEventDestination], required: true, diff --git a/ee/config/audit_events/types/destroy_instance_event_streaming_destination.yml b/ee/config/audit_events/types/destroy_instance_event_streaming_destination.yml new file mode 100644 index 00000000000000..0adcf2c85d1e58 --- /dev/null +++ b/ee/config/audit_events/types/destroy_instance_event_streaming_destination.yml @@ -0,0 +1,8 @@ +name: destroy_instance_event_streaming_destination +description: Event triggered when an instance level external audit event destination is deleted +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/404730 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/125846 +feature_category: audit_events +milestone: "16.2" +saved_to_database: true +streamed: true diff --git a/ee/config/audit_events/types/update_instance_event_streaming_destination.yml b/ee/config/audit_events/types/update_instance_event_streaming_destination.yml new file mode 100644 index 00000000000000..2d256d1475ffcd --- /dev/null +++ b/ee/config/audit_events/types/update_instance_event_streaming_destination.yml @@ -0,0 +1,8 @@ +name: update_instance_event_streaming_destination +description: Event triggered when an instance level external audit event destination is updated +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/404730 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/125846 +feature_category: audit_events +milestone: "16.2" +saved_to_database: true +streamed: true -- GitLab