-
Notifications
You must be signed in to change notification settings - Fork 881
feat: add auditing to user routes #3961
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
Conversation
Tests? |
af6f0c9
to
1f5f69a
Compare
@@ -387,6 +390,7 @@ func TestPostUsers(t *testing.T) { | |||
Password: "testing", | |||
}) | |||
require.NoError(t, err) | |||
assert.Len(t, auditor.AuditLogs, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be good to assert more more about the log, like action, resource type & name, status, etc.
same goes for the other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the API tests it's really only important that we assert the audit log is being generated and the action. The other fields are testable elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are you suggesting we test the other fields, and which ones do you think will be covered?
I feel like this is an ideal place to test that user API calls generate audit logs with a resource type user, for example. Or that a successful API call results in an audit log recording status 200.
Also, that RequestID is non-empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resource Type/ID/Target: Derived from the generic InitRequest
is called with, so it is type enforced to be correct
Status code: Logic is all in the audit package, so it can just be tested there instead
Request ID: Logic is also in the audit package
No description provided.