webadmin: improve audit log for user profile #132

rszwajko · 2022-03-09T12:28:17Z

Before, a generic audit log message was used for all operations on
user profile virtual entity. The message included only the name of the
user that performed the operation.

Changes:

include target user ID - use case: admin creating/deleting
properties for a regular user
include property name
use different messages for create/remove/replace operations

Notes about implementation:

prefix Account Settings: was added for Notification Drawer. In the audit log we have more information i.e. message code. In the UI this context is missing so message needs to be explicit.
~~the current DB layer does not provide the name of the user linked with the property - we have only user ID. Retrieving the user name can be done under different PR.~~

rszwajko · 2022-03-09T12:29:10Z

Example logs

2022-03-09 13:24:51,913+01 INFO  [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector] (default task-3) [c56549de-1bea-44a2-8bd5-5b731bda98da] EVENT_ID: USER_PROFILE_REMOVE_PROPERTY(878), Account Settings: admin@internal-authz removed property 'vmPortal.refreshInterval' for user 1cdf57d4-1179-11ec-8563-52540012eda2.
2022-03-09 13:24:52,162+01 INFO  [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector] (default task-3) [4e461f00-f3ba-4f92-8d8b-032cad0d23eb] EVENT_ID: USER_PROFILE_CREATE_PROPERTY(876), Account Settings: admin@internal-authz created property 'vmPortal.refreshInterval' for user 1cdf57d4-1179-11ec-8563-52540012eda2.
2022-03-09 13:26:22,740+01 INFO  [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector] (default task-3) [5467f362-d961-4334-83db-f600ceacbbce] EVENT_ID: USER_PROFILE_REPLACE_PROPERTY(880), Account Settings: admin@internal-authz replaced property 'SSH_PUBLIC_KEY' for user 1cdf57d4-1179-11ec-8563-52540012eda2.

rszwajko · 2022-03-09T12:30:24Z

Notification drawer in Web Admin

sgratch

The whole idea of adding those audit log messages is for tracking the management of properties settings. But since our behavior with those properties is not consistent comparing admin/vm portals and also comparing between the different properties, it's a bit hard to understand which operation was actually done by the user on a higher level (update a property, reset a property or all properties, created one?).

For example:

When updating an existing user property via the VM portal, it always break down to: removed + created the property. While on admin portal it always break down into replaced the property only. This is confusing and bottom line doesn't really reflects a consistent operation.
When resetting all settings - not all properties are actually removed. Part of them are set back to a default value. so the 'Rest Settings' operation is broke down into: removing few of properties but updating few others. And then when a user set them again - part of them are created and part are updated. So there is no way to know that the user was actually resetting back to defaults.

Bottom line, logging from the commands level is very confusing and inconsistent. i wonder if there is a way to generate those logs on a higher level for the vm portal and for reset settings at least.

backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties

...nager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddUserProfilePropertyCommand.java

rszwajko · 2022-03-14T10:41:48Z

it's a bit hard to understand which operation was actually done by the user on a higher level (update a property, reset a property or all properties, created one?).

Audit log was not key priority. The properties do not impact engine operation or resource allocation and are limited to improving user experience in the UI. If we have a use case where admin needs to understand this kind of changes then we can improve audit capabilities.

For example:

When updating an existing user property via the VM portal, it always break down to: removed + created the property. While on admin portal it always break down into replaced the property only. This is confusing and bottom line doesn't really reflects a consistent operation.

Audit log is a low level log that reflects the changes on the backend. The same flow in the UI is implemented differently for REST API and for GWT. The new REST API cannot use "replace" as "update" so it uses "delete" + "add" instead. The GWT version can access the command layer directly and is able to take advantage of "replace" command.

When resetting all settings - not all properties are actually removed. Part of them are set back to a default value. so the 'Rest Settings' operation is broke down into: removing few of properties but updating few others.

The exact flow of reset settings operation is an implementation detail. It might be implemented by removal of the property or by replacing with property that has "default" value. Having said that, all currently supported properties use "remove" approach. If you have noticed different approach then please let me know.

And then when a user set them again - part of them are created and part are updated. So there is no way to know that the user was actually resetting back to defaults.

The reset is limited to (chosen) properties displayed in the Account Setting dialog - other settings are no affected by design.
The changes are listed in the confirmation dialog and later on reflected in the dialog itself. This works the same as "reset settings" in VM Portal. Please provide more details on how the flow should look like in your opinion.

Bottom line, logging from the commands level is very confusing and inconsistent. i wonder if there is a way to generate those logs on a higher level for the vm portal and for reset settings at least.

As mentioned above, we are abusing the audit log here: it simply reflects the true flow executed on the backend(for audit purpose). This doesn't match the UI flow but having 1:1 mapping between UI and backend is not one of the project goals.

rszwajko · 2022-03-17T15:19:50Z

User changing his own props

2022-03-17 15:19:38,323+01 INFO  [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector] (default task-2) [5c608af6-b442-46df-b30d-e6a9d3b3983d] EVENT_ID: USER_PROFILE_REMOVE_OWN_PROPERTY(882), Account Settings: admin@internal-authz removed property 'vmPortal.refreshInterval'.
2022-03-17 15:19:38,591+01 INFO  [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector] (default task-2) [a8732bca-c02d-4063-a9dd-8079bb6f0961] EVENT_ID: USER_PROFILE_CREATE_OWN_PROPERTY(878), Account Settings: admin@internal-authz created property 'vmPortal.refreshInterval'.
2022-03-17 15:20:00,576+01 INFO  [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector] (default task-2) [739e0dea-fd76-44e5-b012-dbc883d3998c] EVENT_ID: USER_PROFILE_REPLACE_OWN_PROPERTY(886), Account Settings: admin@internal-authz replaced property 'SSH_PUBLIC_KEY'.

User changing other users props

2022-03-17 15:52:28,698+01 INFO  [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector] (default task-2) [d91d8485-6ab6-4ad9-aaca-392b7387b2db] EVENT_ID: USER_PROFILE_CREATE_PROPERTY(876), Account Settings: admin@internal-authz created property 'webAdmin.showNotifications' for user user_a@internal-authz.
2022-03-17 15:53:16,120+01 INFO  [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector] (default task-2) [1dfe9860-6ec4-4a1f-b819-7c83c9394ac1] EVENT_ID: USER_PROFILE_REMOVE_PROPERTY(880), Account Settings: admin@internal-authz removed property 'webAdmin.showNotifications' for user user_a@internal-authz.
2022-03-17 15:56:35,987+01 INFO  [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector] (default task-2) [d1f79f99-d14b-47f7-944c-1496056e87e4] EVENT_ID: USER_PROFILE_REPLACE_PROPERTY(884), Account Settings: admin@internal-authz replaced property 'SSH_PUBLIC_KEY' for user user_a@internal-authz.

rszwajko · 2022-03-17T15:53:14Z

@sgratch
Please check the newest version - the log message now contains target user name if needed. The implementation is based on extra query to fetch target user.

sgratch

@rszwajko

regenerated (for updating) suggest mutation which is incorrect since a new (immutable) entity is created.

Correct, but the admin user is not necessarily a DBA so he might not understand the meaning as we do. Replacing means changing the property with what? Messages should be accurate but low level details are sometimes confusing and not enough. Maybe just add a description of replacing with what, e.g.
${UserName} replaced property '${profile_property} with the same property name and an updated value.

Please check the newest version - the log message now contains target user name if needed. The implementation is based on extra query to fetch target user.

+1, used by other command as well and by audit log too so I'm ok with that.

Audit log was not key priority. The properties do not impact engine operation or resource allocation and are limited to improving user experience in the UI. If we have a use case where admin needs to understand this kind of changes then we can improve audit capabilities.

The point is that based on logs it's impossible for admins and for trouble shooting to understand what the user configured with user settings up till now and I think it's needed. We could implement "reset" as part of REST as well if needed instead of asking the user to reset one by one via the apis. That's not an excuse for the problem of tracking user actions. Most of user actions are logged (at least on DEBUG log level) so that it will be easy to understand what the user tried to do.

But since it will require extra effort to enable that on a upper level now and since webadmin and web ui are different then let's leave it as is for now, even though not ideal.

The exact flow of reset settings operation is an implementation detail. It might be implemented by removal of the property or by replacing with property that has "default" value. Having said that, all currently supported properties use "remove" approach. If you have noticed different approach then please let me know.

This is correct for webadmin properties and not correct for all vmportal ones. All vmportal properties are not just removed when calling reset settings, they are removed and then re-created with the default value. While for webadmin all user's properties are removed except SSH_PUBLIC_KEY and webadmin (grids setting) . Can we at least keep consistency on that level?

backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java

rszwajko · 2022-03-24T11:09:20Z

Maybe just add a description of replacing with what, e.g.
${UserName} replaced property '${profile_property} with the same property name and an updated value.

New message uses instance to differentiate between old and new property. Note that only the object identity is guaranteed to change - the value may stay the same.

But since it will require extra effort to enable that on a upper level now and since webadmin and web ui are different then let's leave it as is for now, even though not ideal.

+1

The exact flow of reset settings operation is an implementation detail. It might be implemented by removal of the property or by replacing with property that has "default" value. Having said that, all currently supported properties use "remove" approach. If you have noticed different approach then please let me know.

This is correct for webadmin properties and not correct for all vmportal ones. All vmportal properties are not just removed when calling reset settings, they are removed and then re-created with the default value. While for webadmin all user's properties are removed except SSH_PUBLIC_KEY and webadmin (grids setting) . Can we at least keep consistency on that level?

This is definitely out of scope of this PR. Please create another issue and let's discuss it there.

rszwajko · 2022-03-24T13:32:50Z

@sgratch
Fixed problems detected by unit tests:

duplicate IDs in AuditLogType
test failing due to incomplete mocks - UserProfileProperty guarantees non-null user ID but mocks do not.

Before, a generic audit log message was used for all operations on user profile virtual entity. The message included only the name of the user that performed the operation. Changes: 1. include target user name if different than current user - use case: admin creating/deleting properties for a regular user 2. include property name 3. use different messages for create/remove/replace operations

sgratch · 2022-03-26T10:23:00Z

/ost

sgratch · 2022-03-26T18:34:56Z

@rszwajko

New message uses instance to differentiate between old and new property. Note that only the object identity is guaranteed to change - the value may stay the same.

+1, much better

This is definitely out of scope of this PR. Please create another issue and let's discuss it there.

This is definitely out of scope, it is just reflected by this fix more easily.
I opened a new issue: oVirt/ovirt-web-ui#1576

sgratch

LGTM

rszwajko requested review from ahadas, bennyz, emesika, mwperina, michalskrivanek, oliel, sgratch and didib as code owners March 9, 2022 12:28

rszwajko mentioned this pull request Mar 9, 2022

webadmin: reset settings in Options dialog #74

Merged

sgratch added the UX label Mar 9, 2022

sgratch force-pushed the userAuditLog branch 2 times, most recently from ae11fda to 237fb44 Compare March 13, 2022 14:50

sgratch requested changes Mar 13, 2022

View reviewed changes

backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties Outdated Show resolved Hide resolved

...nager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddUserProfilePropertyCommand.java Outdated Show resolved Hide resolved

rszwajko force-pushed the userAuditLog branch from cef5056 to 6b386b6 Compare March 17, 2022 15:46

rszwajko requested a review from sgratch March 17, 2022 15:53

sgratch reviewed Mar 22, 2022

View reviewed changes

backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java Outdated Show resolved Hide resolved

rszwajko force-pushed the userAuditLog branch from 6b386b6 to b31f90f Compare March 24, 2022 10:58

rszwajko force-pushed the userAuditLog branch from b31f90f to 0ec382f Compare March 24, 2022 11:10

rszwajko requested a review from sgratch March 24, 2022 11:10

rszwajko force-pushed the userAuditLog branch 2 times, most recently from 1e3fd56 to 21e7f7e Compare March 24, 2022 13:30

sgratch force-pushed the userAuditLog branch from 21e7f7e to a4c0217 Compare March 24, 2022 16:54

sgratch mentioned this pull request Mar 26, 2022

user settings - "reset settings" behaves differently on web-ui and webadmin concerning the properties removal/replacement issue oVirt/ovirt-web-ui#1576

Open

sgratch approved these changes Mar 26, 2022

View reviewed changes

sgratch merged commit fab564c into oVirt:master Mar 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webadmin: improve audit log for user profile #132

webadmin: improve audit log for user profile #132

rszwajko commented Mar 9, 2022 •

edited

Loading

rszwajko commented Mar 9, 2022 •

edited

Loading

rszwajko commented Mar 9, 2022

sgratch left a comment

rszwajko commented Mar 14, 2022

rszwajko commented Mar 17, 2022 •

edited

Loading

rszwajko commented Mar 17, 2022

sgratch left a comment

rszwajko commented Mar 24, 2022

rszwajko commented Mar 24, 2022

sgratch commented Mar 26, 2022

sgratch commented Mar 26, 2022 •

edited

Loading

sgratch left a comment

webadmin: improve audit log for user profile #132

webadmin: improve audit log for user profile #132

Conversation

rszwajko commented Mar 9, 2022 • edited Loading

rszwajko commented Mar 9, 2022 • edited Loading

Example logs

rszwajko commented Mar 9, 2022

Notification drawer in Web Admin

sgratch left a comment

Choose a reason for hiding this comment

rszwajko commented Mar 14, 2022

rszwajko commented Mar 17, 2022 • edited Loading

User changing his own props

User changing other users props

rszwajko commented Mar 17, 2022

sgratch left a comment

Choose a reason for hiding this comment

rszwajko commented Mar 24, 2022

rszwajko commented Mar 24, 2022

sgratch commented Mar 26, 2022

sgratch commented Mar 26, 2022 • edited Loading

sgratch left a comment

Choose a reason for hiding this comment

rszwajko commented Mar 9, 2022 •

edited

Loading

rszwajko commented Mar 9, 2022 •

edited

Loading

rszwajko commented Mar 17, 2022 •

edited

Loading

sgratch commented Mar 26, 2022 •

edited

Loading