openfire-restAPI-plugin icon indicating copy to clipboard operation
openfire-restAPI-plugin copied to clipboard

Audit logging

Open Fishbowler opened this issue 2 years ago • 4 comments

To resolve #115

Fishbowler avatar Jun 29 '22 07:06 Fishbowler

Is there no way to this with a generic filter, or something like that?

Totally with you on this. The maintenance effort is far more than zero. I've sunk a bunch of time into AspectJ to grab info on method calls and parameters, but not only is it a bit odd, it'd require Openfire changes to support "weaving" at runtime.

Instead, I'd like to improve the logging in the AuthFilter - include the user, log the endpoint being hit, consider logging the parameters.

The only problem I've hit is that whilst I can wire the username to the service (through the request context), but not to the controller (have we named all of our classes backwards - shouldn't the controllers have the endpoints, and the services have the business logic?) so if we want to tie auditing of a result to tie to the request, we still end up passing 1 param from the endpoint to the controller method. The alternative is instantiating a controller for each user so that the object has the information once, but in some environments, I fear that could get unweildy.

Fishbowler avatar Nov 18 '22 22:11 Fishbowler

I've pushed some untested code to show what better logging in the AuthFilter might look like.

Fishbowler avatar Nov 18 '22 22:11 Fishbowler

I've got some questions (both on your last comment, as on the code changes). I realized that these questions might stem from a misalignment on what we're actually trying to achieve with this feature.

What data do we, and/or what data don't we want to have logged?

You're digging quite deep into the business logic (which adds to the complexity). When I am tasked to create logs than this, I typically go with the easiest (from a technical perspective) approach: log the HTTP request and response: either metadata directly from both (IP address, URL, response status code, etc) or even the full, raw data.

Granted, that is very much motivated by implementing something that is easy to build. I did not give much thought to the usability of that approach (for users that have to process the logged data) - but I assume that much data can be parsed from those logs. One thing that you'd miss this way is the state of whatever is being modified, prior to the modification taking place (but I don't think your implementation logs that either).

guusdk avatar Nov 20 '22 18:11 guusdk

Ideally, it'd be:

"User [user] fetched [something] and was told [X]" ..or.. "User [user] changed [something] from [X] to [Y]".

That's incredibly stateful, and logging aside, we regularly don't observe prior state before acting, since the activity is "set the state of [something] to [Y]" which we do, so long as Y is valid.

We get most of the way there through the stuff I've messed with in the AuthFilter (and well named endpoints) where we can infer:

"User [user] wanted to fetch [something]" ..or.. "User [user] wanted to set [something] to [Y]"

Perhaps this would be better as a separate filter that runs after Auth where we can gather the response too? With informative endpoints (e.g. when adding X to a list, return the resultant list) we'd be most of the way there.

We can't get the contextual stuff deeper into the business logic without wiring the Auth much deeper, creating noise on every method. It's gross. I think there are probably some good points in the business logic to be loud about important changes though.

Fishbowler avatar Nov 20 '22 19:11 Fishbowler