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

Refactoring of audit events #2687

Merged
merged 42 commits into from Aug 18, 2016
Merged

Refactoring of audit events #2687

merged 42 commits into from Aug 18, 2016

Conversation

@bernd
Copy link
Member

@bernd bernd commented Aug 15, 2016

Description

  • Merge audit object and action into action
  • Create constants for each audit action
  • Create PluginAuditActions interface to make audit actions pluggable
  • Rename AuditLog annotation to the more generic AuditEvent
  • Enable captureRequestEntity and captureResponseEntity by default in AuditEvent
  • Create excludedFields in AuditEvent to make it possible to skip fields like passwords in the audit trail
  • Add test to check that all REST resources annotated with POST, PUT or DELETE also have an
    AuditEvent annotation or a NoAuditEvent annotation if they should not be included in the audit trail
  • Add Jersey model processor to check that all loaded REST resources annotated with POST, PUT and DELETE have a AuditEvent or NoAuditEvent annotation
  • Add missing AuditEvent annotations to REST resources
  • Remove now unused Actions class
  • Rename AuditLogger interface to the more generic AuditEventSender
  • Rename auditlog package to audit
  • Use URNs for audit actors

Motivation and Context

  • Make it possible to iterate over all existing audit actions
  • Ensure that all REST resources which change state have an @AuditEvent annotation
  • Unify audit actions
  • More generic naming
@bernd bernd added this to the 2.1.0 milestone Aug 15, 2016
@bernd
Copy link
Member Author

@bernd bernd commented Aug 15, 2016

Sorry, long PR is long... 😕

@kroepke kroepke self-assigned this Aug 16, 2016
@bernd bernd removed the ready-for-review label Aug 16, 2016
@bernd bernd force-pushed the refactor-audit-events branch 2 times, most recently from 90bd9cc to f978611 Aug 16, 2016
bernd added 23 commits Aug 13, 2016
- Replace `object` and `action` with just `action`
- Create constants for all actions
- Rename `subject` to `actor`
- Add `excludedFields` field
- Capture request and response entities by default

This also adds some missing `@AuditLog` annotations to resources.
Also add `NoAuditEvent` annotation to be able to exclude some resources
from the test.
If a POST, PUT or DELETE resource does not have an audit annotation, a
warning will be written.
This makes it possible to differentiate between users and the system.
Create a more use friendly WARN message and a DEBUG that is useful for
developers.
Avoids a system notification on every server startup.
@bernd bernd force-pushed the refactor-audit-events branch from bc8f430 to 06f99a1 Aug 17, 2016
/**
* Checks all POST, PUT and DELETE resource methods for {@link AuditEvent} annotations and reports missing ones.
*
* It does not report methods which have a {@link NoAuditEvent} annotation.

This comment has been minimized.

@kroepke

kroepke Aug 18, 2016
Member

I would report @NoAuditEvent annotations that use an empty string here, but that's nitpicking.


void success(String subject, String action, String object, Map<String, Object> context);
void success(AuditActor actor, AuditEventType type, Map<String, Object> context);

This comment has been minimized.

@kroepke

kroepke Aug 18, 2016
Member

I'd add a couple of convenience methods here, to make the call sites a bit more concise:

With

default void success(String actor, String type, Map<String, Object> context) {
    success(AuditActor.user(actor), AuditEventType.create(type), context);
}

it would be much easier to read on the call sites.

There are a couple of more options, too, such as allowing to append a varargs of Object pairs to implicitly create the map (or just a single String, Object pair)

This comment has been minimized.

@bernd

bernd Aug 18, 2016
Author Member

I can add a default method for success(AuditActor, String, Map<String, Object>). I would not create one with a String argument for AuditActor because there are two different methods (AuditActor.system(NodeId) and AuditActor.user(String) that do different things. So this can easily break.

Creating AuditEventType from a string is fine because AuditEventType.create() will throw an error if the string is not in the correct format.

default void success(AuditActor actor, String type, Map<String, Object> context) {
    success(actor, AuditEventType.create(type), context);
}
bernd added 4 commits Aug 18, 2016
Use this in AuditEventModelProcessor and PrintModelProcessor instead of
duplicating the code.
Switch internal use to those new methods.
@bernd
Copy link
Member Author

@bernd bernd commented Aug 18, 2016

Addressed review comments.

@bernd
Copy link
Member Author

@bernd bernd commented Aug 18, 2016

Merging this on behalf of @kroepke who gave his "LGTM 👍" offline.

@bernd bernd merged commit 47d80be into master Aug 18, 2016
4 checks passed
4 checks passed
ci-server-integration Jenkins build graylog2-server-integration-pr 1300 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 783 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@bernd bernd deleted the refactor-audit-events branch Aug 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants