Skip to content

Fix incorrect parsing of audit event action

Tan Le requested to merge tancnle/fix-parsing-audit-event-action into master

What does this MR do?

When 'from' and 'to' value being of type Boolean (i.e. false) the action text drops the 'from ' portion due to the generic truthy check. This change ensures we convert the values to string first and then check for present to cater for Boolean values.

This is noted during my development work on !25959 (merged). The audit event was correctly recorded in Project-level audit event table but not in Instance-level one.

For Project-level view, the from and to value are coerced to string

%span= sanitize(human_text(event.details), tags: %w(strong))

(https://gitlab.com/gitlab-org/gitlab/blob/tancnle%2Ffix-parsing-audit-event-action/ee/app/views/shared/audit_events/_event_table.html.haml#L23)

while for Instance-level view, this is taken directly from details field deserialization process

%td= sanitize(event.action, tags: %w(strong))

(https://gitlab.com/gitlab-org/gitlab/blob/tancnle%2Ffix-parsing-audit-event-action/ee/app/views/admin/audit_logs/index.html.haml#L72)

As much as I'd like to unify these two presentation logic, I feel it could be part of #198047 (closed) or else. This fix is to make sure when !25959 (merged) is released, the MR approval permission change event on Instance-level audit events page does not appear broken.

Steps to reproduce

  1. Update project MR approvals settings (https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html#allowing-merge-request-authors-to-approve-their-own-merge-requests or https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html#allowing-merge-request-authors-to-approve-their-own-merge-requests)
  2. Observe an audit event is recorded against the action on Step 1 on Admin Area > Monitoring > Audit Log (https://docs.gitlab.com/ee/administration/audit_events.html#instance-events-premium-only)

Screen_Shot_2020-03-04_at_3.08.45_pm

Expected behaviour

The action text should read

Changed prevent merge request approval from authors from true to false

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Tan Le

Merge request reports