U4-10795 - More granular logging of users actions in the back office

Created by Claus Jensen 02 Jan 2018, 08:07:49 Updated by Sebastiaan Janssen 16 Feb 2018, 13:25:00

Relates to: U4-10974

Subtask of: U4-10796

As a part of GDPR it should be possible to find out who has done what. For that purpose, we need to have more logging of when users does actions in the back office - especially around Users, Members and Forms sections.

For a v1 it’s enough that we log the actions, then we can focus on the reporting side later on. For instance:

User xxx has updated the value yyyy of Member zzzzz (but notice it doesn’t say what value)
User xxx has exported the entries of form yyyy
User xxx has given User yyyy permission to section zzzz

A new DB table will be required for this. Each entry should be about what a user is doing so it should have a foreign key to the user table.

We should have a look at jeffery's package that is available that does most of this - for inspiration. We don't need to copy what he's done but it could be worth having a look at the implementation.

Comments

Stephan 05 Feb 2018, 16:13:15

''Every breath you take Every move you make Every bond you break Every step you take I'll be watching you''


Stephan 07 Feb 2018, 14:53:55

PR https://github.com/umbraco/Umbraco-CMS/pull/2432

cannot merge now - still need to deal with member roles


Stephan 07 Feb 2018, 15:53:29

ready for review - should be a new umbracoAudit table now, with rows for

  • login in and out
  • login failure
  • password reset, change, etc
  • user group change, permissions, sectinos
  • user changes, group changes, etc
  • member changes, roles assigned and removed

happy testing!


Stephan 08 Feb 2018, 07:14:03

TODO

  • refactor how we get the "current" user (Slack disc. w/Shan)
  • ensure we can upgrade (what if the table does not exist?)
  • in fact not beeing able to log should NEVER be an issue


Stephan 08 Feb 2018, 09:18:08

Back in review. Code review for getting the current user (in AuditEventHandler, PerformingUser property). Also test it's possible to upgrade and once upgraded, stuff are logged (basically we detect whether the audit table exists and if not, we suspend logging 'till next restart)


Shannon Deminick 09 Feb 2018, 05:04:04

See comments on PR, just some small changes and questions


Stephan 09 Feb 2018, 08:33:31

pushed fixes - thoughts?


Shannon Deminick 12 Feb 2018, 06:46:16

I've pushed some changes with regards to validating the input to AuditService.Write

But there are other problems, the ctor for IdentityAuditEventArgs that doesn't specify a performingUser means that the INT value is going to be Zero and Zero is the ID of the Admin user. This means that if you do a failed login attempt with a username/password that doesn't match anything at all, it will be logged that the admin user failed to login ... fixing.


Shannon Deminick 12 Feb 2018, 07:07:03

Regarding what is stored, for the eventType, we are currently only storing:

umbraco/user and umbraco/member and then the details are in the free form field eventDetails but I don't think this is going to work the way we want. For example, this doesn't really cater for us being able to easily query for failed login attempts since to do that, we'd have to query against the free form eventDetails field which is probably not what we want.

shouldn't the eventType be a different type per thing? so we'd have:

  • umbraco/user/password/forgot/change
  • umbraco/user/password/forgot/request
  • umbraco/user/login/failed
  • umbraco/user/login/success
  • umbraco/user/password/change
  • umbraco/user/password/reset
  • umbraco/user/logout/success
  • umbraco/user/delete
  • umbraco/user/save
  • umbraco/user-group/permissions-change
  • umbraco/user-group/save
  • umbraco/member/save
  • umbraco/member/delete
  • umbraco/member/roles/assigned
  • umbraco/member/roles/removed

Since this field should be indexed, we can then query on type.


Stephan 12 Feb 2018, 07:12:20

I ''think'' it makes sense. first segment should be 'umbraco' (or 'ucommerce' etc), second should be the part ie 'user', 'member', 'forms', etc. and third should be action? then we can enforce the format?

Also would go from user-group/save to user/group/save?


Shannon Deminick 12 Feb 2018, 13:16:29

Issues that still exists:

  • Add/remove a user from a user group via the user group editor, this does not log any changes for the user but it should
  • --No property changes are logged/tracked for User Groups-- ... this one is odd ** Turns out ... and ok, yes this is partly due to a stupid automapper issue, see my TODOs in the PR :) ** Also turns out we weren't resetting events and managing dirty properties correctly with User Groups


Shannon Deminick 12 Feb 2018, 13:18:35

  • login in and out
  • login failure, both for a real username and non existing one
  • password reset, change, etc for both users and members
  • user group change, permissions, sections
  • user group change - add/remove users
  • user changes, group changes
  • member changes, roles assigned and removed


Stephan 13 Feb 2018, 13:33:00

Add/remove a user from a user group: apparently the user group editor receives the list of ''all'' users of that group (no idea what happens when the list is long?) and posts back the complete list of users. When saving, the list is cleared and re-created, in database. Meaning that to get a list of changes, we need to first query the database. And then, the event which is triggered does not contain the list of users. Needs to be updated too. Bah.


Stephan 13 Feb 2018, 14:33:38

pushed for you to review, it logs added/removed users in a separate statement 'cos we could not modify the already existing event. but it works. now i'm wondering: we have a 1024 chars limit for event details because it seemed to make sense... could we hit it in case we have too many details? I can see you are trimming but... is that ok with audit? we could make the row NTEXT but... then it's more difficult to query. or, we could split long lines over multiple audit entries... thoughts?


Shannon Deminick 13 Feb 2018, 23:25:20

I've made another change so the User events are more consistent. We now have: UserService.SavingUserGroup2 and UserService.SavedUserGroup2 the event args for this is SaveEventArgs<UserGroupWithUsers> to be consistent with all of our operations. We now raise both UserService.SavedUserGroup + SavingUserGroup2 as well as the Saved ones so we can just bind to the new event SavedUserGroup2 and there's no need to bind to both. The event handler now logs 3x for an updated user group: the normal user group values, the user changed values and the content granular permission values.

I'm unsure if we should ever need to query the eventDetails ? In that case NTEXT would be fine. I think with the changes we've made to eventType, that should cater for any queries we need to execute. The affectedDetails we'll need to query since for non-user entity types that is where we store the affected entity Id, so keeping that at 1024 i think is ok.

Thoughts?


Stephan 14 Feb 2018, 10:18:58

events are way cleaner now, great

re. querying, have been thinking:

if I remove user 'bob' from group 'editors' there will be one entry reporting 'umbraco/user-group/save' with eventDetails specifiying that 'bob' has been removed - but 'bob' does not show as affected -- OTOH if I modify 'bob', it shows in the affected column -- assuming I want to know what happened to 'bob', the query is... complex - and will need to hit eventDetails - unless we write 1 entry per user.

so, I'd rather keep all details text fields varchar - but raise their size to, say, 4096 for eventDetails - and maybe we need to split long details over multiple entries instead of truncating. "legally-wise", truncating sounds bad.

we're not going to be able to figure out queries... we'll need to see what ppl try to do... but I think we should log consistently - so...

  • write 1 entry per user, to ensure that affected is always correct?
  • batch users in eventDetails, but then should we keep 'affected' at all?

also... if we start logging multiple entries for one 'event', should we have a 'event id' or anything that would allow us to group entries - could be the 8 first digits of a guid to keep it simple, that would be enough - or is that over-thinking and the date is going to be enough?


Stephan 15 Feb 2018, 12:47:03

decision: 1 entry for 'saving the group' + 1 entry per modified user decision: multiple rows per event, but no need to gather them with an 'event id' decision: therefore, no need to raise the size of details fields decision: no idea about queries, hence no indexes for now

in progress


Stephan 15 Feb 2018, 13:01:22

pushed, ready for review


Sebastiaan Janssen 15 Feb 2018, 15:07:54

Happy with the decisions, agreed, let's worry about indexes when we get a bit further along.


Priority: Normal

Type: Task

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.9.0

Sprint: Sprint 78

Story Points: 5

Cycle: 8