ezpublish-legacy icon indicating copy to clipboard operation
ezpublish-legacy copied to clipboard

EZP-26593: Enable audit log by default

Open peterkeung opened this issue 7 years ago • 12 comments

https://jira.ez.no/browse/EZP-26593

This feature should be enabled by default: https://doc.ez.no/eZ-Publish/Technical-manual/4.x/Features/Audit-trailing

  • We eventually enable the audit log for most clients
  • The audit log usually gets enabled after something bad has happened (and at that point we waste a bunch of time trying to analyze the Apache logs)
  • There are only positives to having the audit log enabled
  • The log files automatically rotate, so there isn't additional configuration that we need to remember

peterkeung avatar Nov 11 '16 16:11 peterkeung

I'm not fully agree with this. Looking to, for example, symfony does in term of loggin, by default it tries to reduce the amount of logs it creates in prod environments. I would prefer to not have this enabled by default. Besides, is easier to activate on your own.

But pinging @emodric @wizhippo to know their opinions.

crevillo avatar Nov 11 '16 17:11 crevillo

I'm with @crevillo on this one. This is a feature that I believe should not be on by default.

wizhippo avatar Nov 11 '16 17:11 wizhippo

Agreed with @crevillo and @wizhippo! It's useful, but should not be on by default.

emodric avatar Nov 11 '16 18:11 emodric

Over the years I have found content hide, move, and delete history to be just as core as edit history. That's the main argument. Sure, it's easy to enable but catches people by surprise that it's not default.

peterkeung avatar Nov 11 '16 18:11 peterkeung

As long as there is a rotation by default I don't see a big harm on enabling it by default, to the opposite, sometimes we found ourselves asking to check the audit logs to see who deleted a certain content just to find out it was not enabled and we couldn't properly "audit" the issue.

thiagocamposviana avatar Nov 11 '16 19:11 thiagocamposviana

I'm in favour of having it on by default - the audit log is rotated, and the IO you get out of it is small anyway.

My main concern with it in fact is that it is not omni-comprehensive, and that it should log more operations. All operations except content publication should be logged: move/hide/set-state/set-section/trash/swap-nodes/add-or-remove-location/change-sorting/change-priority etc... (without the need to set up up dedicated workflow events)

Btw, is this even a feature in eZ5? Maybe we should have a budle for that?

Side note : @crevillo Sf does many things different than eZ by default/principle. The funny thing is: quite a few of them make lots of sense for a framework, but less for a CMS. Eg. the whole "throw your arms in the air if there is the smallest error anywhere" works fine for code and configuration, but not for code that touches content, as content always ends up inconsistent (and eZ had to turn a lot of screws to reintroduce graceful handling of those cases in version 5, instead of 'show blank page'. Heck, proper handling of bad object relations is still being worked on these days...)

gggeek avatar Nov 12 '16 12:11 gggeek

Looks we need a github feature to add polls here, but even though, i know can count 3 "yes" and 3 "no". :).

Who should we ping to break the tie here?

crevillo avatar Nov 12 '16 13:11 crevillo

What is the case for not having it be default behaviour? Thus far, we're looking at reducing the amount of log files created which is a secondary consideration here.

dougplant avatar Nov 13 '16 21:11 dougplant

I don't know what could be the reason to have it off by default. But looks like nobody though in having it enabled by default until now :).

Looks we have 4 yes and 3 no now. We can merge this, but we should ping the eZ doc team to change the doc @peterkeung pointed in the link where it says it's disabled by default.

Also i guess we should doc this is a BC change. Somebody can have a own audit doing whatever maybe needed and probably adding more overhead than the already existing already messages. And maybe he/she just want to have it enabled from time to time. We should warn user about this will be enabled by default.

@emodric @wizhippo ok to merge?

crevillo avatar Nov 13 '16 21:11 crevillo

damn, did i merge this by mistake? Edit: Sorry. looks its merged in the mugoweb repo, not here.

Besides, it looks i don't have access to merge this, even i thought i had...

crevillo avatar Nov 13 '16 22:11 crevillo

I'm still -1 for this change since it can be considered as a breaking change, due to it being a change in default settings.

I don't see a problem of keeping the config enabled in your overrides if you really need it. It's config, not code.

emodric avatar Nov 13 '16 22:11 emodric

Same here, if there was some new major/minor release planned and thus corresponding doc change it could make sense. But as there is not this is not something we'd put into a patch release, which master effectively is here.

andrerom avatar Nov 14 '16 08:11 andrerom