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

Store user state in Audit Trail (Lombiq Technologies: OFFI-260) #17518

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

sarahelsaig
Copy link
Contributor

Fixes #17507

Copy link
Contributor

@barthamark barthamark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. @Piedone or @BenedekFarkas can you please merge it? I don't have the permission.

@sebastienros
Copy link
Member

This can't be merged as-is because of privacy consideration. Right now every private information of a user is duplicated in audit trail and more importantly disclosed in the UI to anyone with audit trail permissions.

Also deleting a User would delete the information, so a user requesting their information to be deleted is supported by OC. There is no feature that I know of that can delete audit trail records for specific users, and this PR will make it harder (note that such a feature is a different request).

This PR should

  • only snapshot by default the properties without private information, like TwoFactorEnabled, LockoutEndUtc ... This is to prevent existing sensitive information like PasswordHash, Tokens from being stored and disclosed.

  • provide options in the admin, behind a permission, to add the properties that can be snapshotted. Including Custom Settings. For instance by setting the technical JSON accessor of the property (PhoneNumber, Properties.CustomUserSettings.Address). Otherwise any private information from the custom settings would be stored by default which nobody wants.

  • Some informations might be stored as hashed values to detect that is has changed, again only if defined in options. The hash should then vary by system (use a seed per tenant for instance) such that they can't be used with rainbow tables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store user state in Audit Trail on certain user events
3 participants