Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add audit logs #3454

Merged
merged 26 commits into from
Nov 1, 2023
Merged

feat: add audit logs #3454

merged 26 commits into from
Nov 1, 2023

Conversation

neatbyte-vnobis
Copy link
Collaborator

@neatbyte-vnobis neatbyte-vnobis commented Aug 7, 2023

Changes

Implements issue "Audit Logs".

Added new "Audit Logs" item in left side menu:

image

Audit Logs table:

image

Changes by package:

  • api-audit-logs-aco
    This package currently serves as API for audit logs. Most of the logic is copied from api-page-builder-aco package.
  • api-audit-logs
    This package contains all subscriptions for lifecycle events under /src/subscriptions. There is also a config file under /src/config, where all possible audit log apps, events, actions defined.
  • api-file-manager
    Added lifecycle events for file manager settings update. Also added test for new lifecycle events.
  • api-form-builder
    Added lifecycle events for form submissions export.
  • api-page-builder-import-export
    Added lifecycle events for import and export.
  • api-security
    Added lifecycle events for ApiKeys, Groups and Teams. Also added tests for new lifecycle events (except Teams since there were no tests for them at all).
  • app-audit-logs
    Audit logs UI (table, filters, modal, etc.)
  • app-file-manager
    Fixed wrong gql input data type for UpdateFileManagerSettings mutation. It was causing error when updating file manager settings.
  • app-serverless-cms
    Added audit logs to main app.
  • cwp-template-aws
    Copy of apps/api changes.
  • ui
    Added readOnly property to CodeEditor component and JSON mode for audit logs payload JSON preview.

How Has This Been Tested?

Manual

Documentation

None

@neatbyte-vnobis
Copy link
Collaborator Author

@brunozoric Please let me know if you have any question.
Hope to hear your feedback soon.

@SvenAlHamad
Copy link
Contributor

I did an initial test run of the implementation and I've noticed we're missing the security events in the audit logs (login successful, login unsuccessful, ...).
Additionally we need to look at a way of optimizing the Page Builder events. If you open a page, say Welcome to Webiny, and then start editing some text, and changing values for padding/margin, each of these small changes sends a save event to the API which triggers an entry in the audit log. I managed to create 20+ audit log updates while I was editing a page for 10 seconds. I don't think this will work in practice as we'll have millions of update entries for the page builder. We need to look for a way to optimize this.

@neatbyte-vnobis
Copy link
Collaborator Author

@SvenAlHamad
I will check security events.
Regarding Page builder - agree, but this is how it is working (saving changes via API after every change).
We may log only Publish events...
Or think about "grouping" such similar events into one (maybe if time difference is less then 1 minute?)

@neatbyte-vnobis
Copy link
Collaborator Author

@SvenAlHamad
Regarding security events:
Currently we have three events Login successful, Login unsuccessful and Password reset successful that are not implemented yet.
These events are a little different from others, since we don't use api-scurity package to process them. Instead they are executed directly from UI using aws-amplify package.
I will think on how we can implement audit logs for this events.

@brunozoric
Copy link
Contributor

@neatbyte-vnobis There are no tests for Audit Logs in this branch. Do you have them somewhere else?

@neatbyte-vnobis
Copy link
Collaborator Author

@neatbyte-vnobis There are no tests for Audit Logs in this branch. Do you have them somewhere else?

Currently I've only added tests for new lifecycle events in some API packages (like this).

Tests for Audit Logs are not implemented yet, since I didn't have confirmation on API implementation. If it is ok, then I can start working on them.

Audit Logs tests should be something similar to what we have in api-page-builder-aco package (the package I've used as reference). But I'm not sure if we need tests for all subscriptions.

@brunozoric
Copy link
Contributor

When having a record with a lot of content (eg, page larger than 400KB), audit logs are not stored.
This is due to the DynamoDB 400KB limit. You need to compress the payload so it can be stored.

@brunozoric
Copy link
Contributor

When APW is on and there is a CMS workflow, something is wrong when trying to request review on publish. It is working in next branch, so it's probably something you added in the Audit Logs.

@brunozoric
Copy link
Contributor

When trying to search for something in Audit Log Payload JSON box (using the CMD+F on Mac, probably CTRL+F on Windows), error is thrown.

@brunozoric
Copy link
Contributor

When creating or updating the API Key, do not store the token into the payload data - filter it out.

@neatbyte-vnobis
Copy link
Collaborator Author

When having a record with a lot of content (eg, page larger than 400KB), audit logs are not stored. This is due to the DynamoDB 400KB limit. You need to compress the payload so it can be stored.

I agree that compression needs to be done. Have a couple questions:

  1. Should the compression be applied to all Audit Log payloads for consistency, or do we only consider this approach for pages?
  2. For pages, do we need to keep the 'before' and 'after' changes in the payload (as it is now), or is just the 'after' enough to reduce the size of the payload?

@brunozoric brunozoric self-assigned this Nov 1, 2023
@brunozoric brunozoric merged commit ddbf234 into next Nov 1, 2023
63 checks passed
@brunozoric brunozoric deleted the neatbyte/audit-logs branch November 15, 2023 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants