Skip to content

Audit status check response updates

The following discussion from !113926 (merged) should be addressed:

  • @huzaifaiftikhar1 started a discussion: (+4 comments)

    Question: Can you please help me answer the following questions?

    1. Do we want to create an audit event every time a status check response is updated? Won't this be a high volume event? If we want to capture this we should probably keep it as a stream only event.
    2. I believe this will create an audit event with a generic 'audit_operation' event type and we probably want to avoid doing that. @harsimarsandhu Should we continue using the Auditable concern or use ::Gitlab::Audit::Auditor.audit here? I am not too sure therefore wanted your opinion on this one.

Based on the discussions in the above thread and some investigations on how we're currently using ::Gitlab::Audit::Auditor, the audit event was removed from !113926 (merged)

!113926 (f7c4a28d)

Removed code

ee/app/models/merge_requests/status_check_response.rb (not pushed but working locally)

    after_update :log_audit_event

    def log_audit_event
      project = merge_request.project
      audit_context = {
        name: 'status_check_response_update',
        author: ::Gitlab::Audit::UnauthenticatedAuthor.new(name: '(System)'),
        scope: project,
        target: project,
        message: "Updated status check response #{id} for status check #{external_status_check.name}"
      }

      ::Gitlab::Audit::Auditor.audit(audit_context)
    end

ee/spec/services/external_status_checks/retry_service_spec.rb

    describe 'audit events' do
      let(:status) { 'failed' }

      context 'when licensed', :request_store do
        before do
          stub_licensed_features(audit_events: true)
          stub_licensed_features(external_status_checks: true)
        end

        context 'when rule retry operation succeeds' do
          let(:expected_message) do
            "Updated status check response #{status_check_response.id} for status check #{rule.name}"
          end

          it 'logs an audit event' do
            expect { subject }.to change { AuditEvent.count }.by(1)
            expect(AuditEvent.last.details).to match(
              {
                author_class: 'Gitlab::Audit::UnauthenticatedAuthor',
                author_name: '(System)',
                custom_message: expected_message,
                target_details: project.name,
                target_id: project.id,
                target_type: 'Project',
              }
            )
          end
        end

        context 'when rule destroy operation fails' do
          before do
            allow_any_instance_of(::MergeRequests::StatusCheckResponse).to receive(:update).and_return(false) # rubocop:disable RSpec/AnyInstanceOf - It's not the next instance
          end

          it 'does not log any audit event' do
            expect { subject }.not_to change { AuditEvent.count }
            expect(subject).to be_error
          end
        end
      end

      it_behaves_like 'does not create audit event when not licensed'
    end

Edited by Imam Hossain