webadmin: add Account Settings sub-tab #130

rszwajko · 2022-03-08T13:51:03Z

Depends on #129

Add a new sub-tab under Administration -> Users -> {user} that allows
viewing user settings. The settings are displayed unformated as JSON
or plain text depending on the property type. Only remove operation is
supported.

Main use case is troubleshooting problems related to user settings.
With the new view, the admin is able to restore the default
configuration by removing incorrect user properties.

rszwajko · 2022-03-08T13:52:04Z

sgratch · 2022-03-08T17:24:32Z

As we discussed offline, I really don't like this new suggested tab and IMHO it causes a lot of problems and complexity:

it is written on our design assumptions: "user settings dialog is set for current logged-in user (and not for all registered oVirt users/super users))" since with old design it was raised but we decided to omit it (old design)
I really can't see any added value for that - each logged in user should set his own user's settings and there is no real admin motivation for settings other user's preferences..
The webadmin is not used for troubleshooting problems caused by wrong rest api calls and specifically not for configuration ones.
This allows adding invalid properties via the UI - settings webadmin properties for a non admin user which will be never used, removing properties for logged in users, adding properties for old users that were not logged in for a long time, mismatch with local storage persistence etc.
We'll need to support cleaning policy for users not logged on for a long time, validations for super users logged in to admin portal which might won't have full permissions to set other admin users etc.

rszwajko · 2022-03-09T13:05:45Z

As we discussed offline, I really don't like this new suggested tab and IMHO it causes a lot of problems and complexity:

it is written on our design assumptions: "user settings dialog is set for current logged-in user (and not for all registered oVirt users/super users))" since with old design it was raised but we decided to omit it (old design)

This is a new proposal - not covered by our previous design. Not sure what were the reasons for dropping this kind of functionality but the area changed significantly in the last 3 years so IMHO changes are justified.

I really can't see any added value for that - each logged in user should set his own user's settings and there is no real admin motivation for settings other user's preferences..

Please take another look at the PR description.
The main use case is well described there.

The webadmin is not used for troubleshooting problems caused by wrong rest api calls and specifically not for configuration ones.

Administration section seems a natural place to solve problems with user accounts
problems with user settings may have various causes - including malformed property added by REST. The focus here is on the solution - ability to fix the configuration.

This allows adding invalid properties via the UI - settings webadmin properties for a non admin user which will be never used, removing properties for logged in users, adding properties for old users that were not logged in for a long time, mismatch with local storage persistence etc.

Please take another look at the PR description.
Only property removal is supported. Adding properties was dropped as it requires more effort.

We'll need to support cleaning policy for users not logged on for a long time, validations for super users logged in to admin portal which might won't have full permissions to set other admin users etc.

Please provide more details about those issues.
I don't expect any problems here as we are not adding any new functionality form backend point of view:

the screen shows exactly the same data that admin user can obtain via REST API
available operations (remove) are only a subset of those available via REST (basically full access)

sgratch · 2022-03-27T13:08:41Z

@rszwajko I'm still not convinced about the necessarily of this dialog. We are not adding dialogs just for troubleshooting problems on webadmin. The only motivation is if there is a functional requirement for that and I'm not sure if this is the case. Not all functionality supported by REST should be supported by the UI unless it's required.

When only remove action is supported, it's similar to cleanup of user properties on DB like 'reset settings' (for vmportal will be similar once oVirt/ovirt-web-ui#1576 is solved). That is not required/important but can be nice - an option to clean all DB persisted properties for a user. Which role is required for that? Are we ok with an admin viewing/cleaning settings for other admin user?

In that case, I would recommend dividing the properties into vm portal settings and webadmin settings and also have an option to clean all and add a confirmation dialog with the number of removed properties etc.

In case of an error, the following ACTION_TYPE_FAILED_USER_PROFILE_PROPERTY_NOT_EXISTS message appears:

The action is set to edit instead of remove and no other details provided, it's not added as an event as well

rszwajko · 2022-03-28T13:15:28Z

@rszwajko I'm still not convinced about the necessarily of this dialog. We are not adding dialogs just for troubleshooting problems on webadmin. The only motivation is if there is a functional requirement for that and I'm not sure if this is the case. Not all functionality supported by REST should be supported by the UI unless it's required.

We are adding more and more features that rely on user options so it makes sense to provide some management capabilities.
Remove is just a start here. If there will be more justified scenarios we can extend this screen.

When only remove action is supported, it's similar to cleanup of user properties on DB like 'reset settings' (for vmportal will be similar once oVirt/ovirt-web-ui#1576 is solved). That is not required/important but can be nice - an option to clean all DB persisted properties for a user. Which role is required for that? Are we ok with an admin viewing/cleaning settings for other admin user?

No change here compared with current situation - user options are outside of permission system so any admin have full access.

In that case, I would recommend dividing the properties into vm portal settings and webadmin settings

We don't have a flag to separate both types. We mark it only with prefix but this is more a convention.

and also have an option to clean all

+1
However this is more a shortcut/ advanced usage.

and add a confirmation dialog with the number of removed properties etc.

the props are visible all the time in the table so not sure if this is needed.

In case of an error, the following ACTION_TYPE_FAILED_USER_PROFILE_PROPERTY_NOT_EXISTS message appears:

sgratch · 2022-03-28T14:53:55Z

@rszwajko

No change here compared with current situation - user options are outside of permission system so any admin have full access

As long as 'Manipulate Users' permission and maybe other user related permissions that allow admins/superusers to create/remove/set users is granted. This is the default for admins but in case an admin/superuser without those permissions is set, he shouldn't remove properties. You can try creating a new admin role without that permission and check that properties for other admin users can't be removed by this dialog as well. As long as we are based on user's permissions then we are ok but just to make sure.

We don't have a flag to separate both types. We mark it only with prefix but this is more a convention.

Sorting by property name is possible, right? This might help diving between those 2 types.

add a confirmation dialog with the number of removed properties etc

I think that a confirmation dialog with number of removed properties is needed here since you are about to remove properties for another user, need an option to verify that.

In case of an error, the following ACTION_TYPE_FAILED_USER_PROFILE_PROPERTY_NOT_EXISTS message appears:
...
The action is set to edit instead of remove and no other details provided, it's not added as an event as well

?

rszwajko · 2022-03-28T16:19:26Z

@sgratch

As long as 'Manipulate Users' permission and maybe other user related permissions that allow admins/superusers to create/remove/set users is granted. This is the default for admins but in case an admin/superuser without those permissions is set, he shouldn't remove properties.

This seems a new requirement. The only permission check currently performed is isAdmin.

Sorting by property name is possible, right? This might help diving between those 2 types.

Yes. It's already implemented in this version.

I think that a confirmation dialog with number of removed properties is needed here since you are about to remove properties for another user, need an option to verify that.

+1

In case of an error, the following ACTION_TYPE_FAILED_USER_PROFILE_PROPERTY_NOT_EXISTS message appears:
...
The action is set to edit instead of remove and no other details provided, it's not added as an event as well

No changes here - it's the same in the current code. This the backend validation message:

 Cannot ${action} ${type}. User Profile property with the provided ID does not exist.

The edit part is due to the action type used which is VAR__ACTION__UPDATE.

sgratch · 2022-03-28T21:55:03Z

The edit part is due to the action type used which is VAR__ACTION__UPDATE

Why UpdateUserProfilePropertyCommand is called instead of RemoveUserProfilePropertyCommand?

rszwajko · 2022-03-29T08:35:32Z

The edit part is due to the action type used which is VAR__ACTION__UPDATE

Why UpdateUserProfilePropertyCommand is called instead of RemoveUserProfilePropertyCommand?

The command is correct. The action name is update because it used to be a change to user profile (profile was updated).
Source:

ovirt-engine/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveUserProfilePropertyCommand.java

Line 52 in cdd24a4

addValidationMessage(EngineMessage.VAR__ACTION__UPDATE);

sgratch · 2022-03-29T09:42:07Z

The edit part is due to the action type used which is VAR__ACTION__UPDATE

Why UpdateUserProfilePropertyCommand is called instead of RemoveUserProfilePropertyCommand?

The command is correct. The action name is update because it used to be a change to user profile (profile was updated). Source:

ovirt-engine/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveUserProfilePropertyCommand.java

Line 52 in cdd24a4

addValidationMessage(EngineMessage.VAR__ACTION__UPDATE);

This is a bit confusing....but ok

This seems a new requirement. The only permission check currently performed is isAdmin.

I guess it will work like on rest but need to verify

sandrobonazzola · 2022-03-30T08:11:07Z

@rszwajko is there a BZ related to this PR? Can this wait for 4.5.1 or should this be included in 4.5.0?

rszwajko · 2022-03-30T08:28:54Z

is there a BZ related to this PR?

No specific BZ but it may get included into the umbrella BZ https://bugzilla.redhat.com/2049782
@sgratch what do you think?

Can this wait for 4.5.1 or should this be included in 4.5.0?

It's definitely 4.5.1

michalskrivanek · 2022-06-16T08:01:18Z

missed 4.5.1 as well.

sgratch · 2022-07-12T13:32:52Z

@rszwajko
There are still open issues mentioned above that need to be handled or rejected before we can decide about this PR. The two important ones are:

1) Are we ok with a super user that doesn't include Manipulate Users permissions viewing/cleaning settings for other admin users e.g. removing the SSH KEY? I think that If the user can't update user permissions, add users etc then he can't remove or view account settings for other users. .It should be the same logic for rest as well but even more important when accessed via the ui.

user options are outside of permission system so any admin have full access

I disagree. For non admin users this is blocked since each user can manipulate his own account settings only (via rest and vm portal). So we are ok with that.
For admin users with full users permissions - no blocking is needed since if the admin user can add/remove other users, he can manipulate their account settings as well.
For admin users that don't have users manipulation permissions - this account settings dialog should be blocked as well. Currently it's not the case

2)

a confirmation dialog with the number of removed properties is needed here since you are about to remove properties for another user, need an option to verify that.

I think it worth the effort...

rszwajko · 2022-07-14T14:29:04Z

@sgratch
Added the following confirmation using the default ConfirmationModel

For admin users that don't have users manipulation permissions - this account settings dialog should be blocked as well. Currently it's not the case

Please provide more details about users manipulation permissions. According to my tests user options are outside of permission system so any admin have full access.
My test case: user foobar with ClusterAdmin permissions(see below for full list) is able to remove user properties that belong to user admin (the super user). This works both in Web Admin and via REST.

rszwajko · 2022-07-14T14:51:44Z

/ost

sgratch · 2022-09-01T12:17:44Z

@rszwajko

Please provide more details about users manipulation permissions. According to my tests user options are outside of permission system so any admin have full access.

As an example you can have a user with a customized admin role that doesn't include the Manipulate Users actionGroup

ovirt-engine/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/ActionGroup.java

Line 74 in 0dabcf4

MANIPULATE_USERS(500, RoleType.ADMIN, true),

so that the user can't add/remove users. in that case, I don;t think that specific admin should be able to manipulate other user's settings as well.
For reproducing that, you can create a customized admin role via administration->configure->roles with system->'manipulate users' action disabled.

rszwajko · 2022-09-12T07:59:30Z

@sgratch
The test user, that I used in my tests, is exactly the kind of user you describe (see below). However he is able to access user settings via REST API. As for now accessing user setting is outside of the permission system. If you think that we should change that then this is an enhancement.

sgratch · 2022-09-22T11:41:34Z

@rszwajko
1) The confirmation dialog for removing properties looks good.

2) I think that 'reset settings' reachable from the kebab menu for columns is a bit confusing when reachable from the account settings sub tab, since it's not clear which settings to reset here. But let's leave it as is for now since it is consistent with other tabs and we can always rephrase the label in the future if asked for

sgratch · 2022-09-22T11:45:24Z

@rszwajko @mwperina
3) Regarding the old permissions issue discussion raised here, after checking more deeply and based on your comments, it seems that the following scenarios currently exist:

Non admin/super user permissions (i..e. regular users) don't have permissions to delete other user's profile properties (ssh and other user options). A non admin user can only delete his own profile properties. This is reproduced via REST since deleting other user's properties by a non admin is not supported via the UI.
Users with admin/superuser permissions do have permissions to delete other user's profile properties (ssh and other user options) via both REST and the new account settings UI. This is the concern that I'm raising here.

There is no option to manage a role for that since the userProfileEditor role is enabled for all and can't be disabled via the UI.
So bottom line, admin/superuser users can delete all other admin/superuser properties and ssh keys without an option to manage that by permissions granting.

It seems like the logic for that is implemented here:

ovirt-engine/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/UserProfileValidator.java

Lines 43 to 49 in 4655358

    
           public ValidationResult authorized(DbUser currentUser, Guid targetUserId) { 
        
               return ValidationResult 
        
                       .failWith(EngineMessage.USER_NOT_AUTHORIZED_TO_PERFORM_ACTION) 
        
                       .when(currentUser == null 
        
                               || currentUser.getId() == null 
        
                               || !Objects.equals(currentUser.getId(), targetUserId) && !currentUser.isAdmin()); 
        
           }

I can see that checking if a user profile property can be handled by other user is checked here:

ovirt-engine/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/UserProfileValidator.java

Lines 58 to 62 in 4655358

    
           public ValidationResult sameOwner(Guid currentId, Guid targetId) { 
        
               return ValidationResult 
        
                       .failWith(EngineMessage.USER_NOT_AUTHORIZED_TO_PERFORM_ACTION) 
        
                       .when(!Objects.equals(currentId, targetId)); 
        
           }

and is applied only for adding/updating a user property but doesn't apply for deleting a user property.

So bottom line, this is all part of the user settings feature implementation and I'm not sure what was the original implementation for SSH keys removal and do we want to leave it as is or treat it as a bug before exposing this account settings tab.

@mwperina Please review this third issue comment. Do you think it is a bug or remember if in old implementation (prior account settings), admin users could delete other admin user's ssh keys without an option to block it by permissions?

michalskrivanek · 2022-09-29T13:54:24Z

I think we should change the check to SuperUser, that should be fine enough - just let the SuperUser(s) to be able to delete profiles, that's it

Before, in order to display a toast notification, the component needed direct access to NotificationPresenterWidget. With this patch this restriction has been removed. The notification handler registers itself in the Frontend which allows public access.

Add a new sub-tab under Administration -> Users -> {user} that allows viewing user settings. The settings are displayed unformated as JSON or plain text depending on the property type. Only remove operation is supported. Main use case is troubleshooting problems related to user settings. With the new view, the admin is able to restore the default configuration by removing incorrect user properties.

rszwajko · 2022-09-29T14:11:11Z

@michalskrivanek

I think we should change the check to SuperUser, that should be fine enough - just let the SuperUser(s) to be able to delete profiles, that's it

you mean i.e. via this helper

ovirt-engine/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java

Line 1165 in c25c3f3

public boolean isSystemSuperUser() {

rszwajko · 2022-09-29T15:03:19Z

@michalskrivanek @sgratch
I've submitted a draft PR with the change suggested above - see #688
Unfortunatetly, today I cannot test it on my dev env.

sgratch · 2022-09-29T16:19:47Z

@michalskrivanek

I think we should change the check to SuperUser, that should be fine enough - just let the SuperUser(s) to be able to delete profiles, that's it

you mean i.e. via this helper

ovirt-engine/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java

Line 1165 in c25c3f3

public boolean isSystemSuperUser() {

@rszwajko yes.
The suggested solution is that all users can view profile properties and delete their own properties but only if isSystemSuperUser() == true then the user can delete other users properties in addition to his own. This will be supported on backed for rest api as well.
On UI - the Remove button for the new sub tab can be enabled but an error will appear.

sgratch · 2022-10-27T12:19:34Z

/ost

sgratch · 2022-10-31T20:17:13Z

/ost

rszwajko requested review from sgratch and ljelinkova as code owners March 8, 2022 13:51

rszwajko changed the title ~~Settings tab~~ webadmin: add Account Settings sub-tab Mar 8, 2022

sgratch added the UX label Mar 8, 2022

rszwajko requested a review from sjd78 March 9, 2022 13:06

sgratch force-pushed the settingsTab branch from 1c082f1 to 62b2088 Compare March 26, 2022 19:20

sandrobonazzola assigned rszwajko Mar 30, 2022

sandrobonazzola added this to the ovirt-4.5.1 milestone Mar 30, 2022

sandrobonazzola modified the milestones: ovirt-4.5.1, ovirt-4.5.2 Jun 16, 2022

rszwajko force-pushed the settingsTab branch from 62b2088 to 3d41dbf Compare July 14, 2022 14:18

sandrobonazzola removed this from the ovirt-4.5.2 milestone Sep 13, 2022

rszwajko added 3 commits September 29, 2022 15:54

webadmin: confirm removing user profile properties

8ba58ac

michalskrivanek force-pushed the settingsTab branch from 3d41dbf to 8ba58ac Compare September 29, 2022 13:54

rszwajko mentioned this pull request Oct 4, 2022

Restrict user profile write acces to owner and super user #688

Merged

ljelinkova approved these changes Oct 14, 2022

View reviewed changes

Merge branch 'master' into settingsTab

5e29307

sgratch mentioned this pull request Nov 1, 2022

webadmin: display toast notifications via Frontend #129

Merged

sgratch approved these changes Nov 1, 2022

View reviewed changes

sgratch merged commit f28f68f into oVirt:master Nov 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webadmin: add Account Settings sub-tab #130

webadmin: add Account Settings sub-tab #130

rszwajko commented Mar 8, 2022

rszwajko commented Mar 8, 2022

sgratch commented Mar 8, 2022

rszwajko commented Mar 9, 2022

sgratch commented Mar 27, 2022 •

edited

Loading

rszwajko commented Mar 28, 2022

sgratch commented Mar 28, 2022

rszwajko commented Mar 28, 2022

sgratch commented Mar 28, 2022

rszwajko commented Mar 29, 2022

sgratch commented Mar 29, 2022

sandrobonazzola commented Mar 30, 2022

rszwajko commented Mar 30, 2022

michalskrivanek commented Jun 16, 2022

sgratch commented Jul 12, 2022

rszwajko commented Jul 14, 2022

rszwajko commented Jul 14, 2022

sgratch commented Sep 1, 2022

rszwajko commented Sep 12, 2022

sgratch commented Sep 22, 2022

sgratch commented Sep 22, 2022

michalskrivanek commented Sep 29, 2022

rszwajko commented Sep 29, 2022

rszwajko commented Sep 29, 2022

sgratch commented Sep 29, 2022

sgratch commented Oct 27, 2022

sgratch commented Oct 31, 2022

webadmin: add Account Settings sub-tab #130

webadmin: add Account Settings sub-tab #130

Conversation

rszwajko commented Mar 8, 2022

rszwajko commented Mar 8, 2022

sgratch commented Mar 8, 2022

rszwajko commented Mar 9, 2022

sgratch commented Mar 27, 2022 • edited Loading

rszwajko commented Mar 28, 2022

sgratch commented Mar 28, 2022

rszwajko commented Mar 28, 2022

sgratch commented Mar 28, 2022

rszwajko commented Mar 29, 2022

sgratch commented Mar 29, 2022

sandrobonazzola commented Mar 30, 2022

rszwajko commented Mar 30, 2022

michalskrivanek commented Jun 16, 2022

sgratch commented Jul 12, 2022

rszwajko commented Jul 14, 2022

rszwajko commented Jul 14, 2022

sgratch commented Sep 1, 2022

rszwajko commented Sep 12, 2022

sgratch commented Sep 22, 2022

sgratch commented Sep 22, 2022

michalskrivanek commented Sep 29, 2022

rszwajko commented Sep 29, 2022

rszwajko commented Sep 29, 2022

sgratch commented Sep 29, 2022

sgratch commented Oct 27, 2022

sgratch commented Oct 31, 2022

sgratch commented Mar 27, 2022 •

edited

Loading