Add a set_current_group method for users

[jntullo]

The current way of setting a user’s current_group via edit uses resource_search, which encounters some RBAC issues. For example, when a user changes from the super administrator group to the tenant group, they are no longer able to switch back even though they belong to the group because resource_search returns a forbidden error. By choosing a group based off of their current miq_groups, it resolves the issue and keeps the ability to change groups consistent with that in the classic-ui.

How it works in the classic ui:

The failure present in the SUI before the fix:

The successful API call with set_current_group:

In addition, with update_attributes! it raises a validation error saying that the superadministrator group is not in their groups, also appears to be due to RBAC. I looked to the classic UI to see how their behavior works, and chose to use update_attribute based off of that. Because this is different behavior than a typical edit, I felt that it also deserved a specialized method, thus removing this functionality from edit and moving it to set_current_group.

This is also a unique use case, and did not think it would apply as a collection action, but am open to discussion around that.

https://bugzilla.redhat.com/show_bug.cgi?id=1467364

@miq-bot add_label bug, gaprindashvili/yes
@miq-bot assign @abellotti

cc: @AllenBW

[abellotti]

does this mean if a user does not have that role they cannot change their current group ?

[jntullo]

@abellotti yeah, I guess so. I am not sure if users should always be allowed to change their own group or if they need that particular role?

[abellotti]

May be easy to verify, in classic UI login as a user without that role but a member of a couple of groups, see if you can change the current group in the upper right hand pull-down. If you can, we'll need to somehow handle that.

[jntullo]

@abellotti verified that a user in the classic UI without this edit role is still able to change their group. updated accordingly 👍

[abellotti]

With the role identifier removed, we probably need to make sure that the action is only run on the authenticated user, otherwise anyone can change anyone else's group. Can you verify this and add a test pertaining to that ? Thanks.

[abellotti]

can you use parse_group(data["current_group"]) here ? it support both id/href as well as the identifying attr,i.e. be able to set current group by description.

[abellotti]

or maybe the parse_fetch_group we used earlier.

[jntullo]

@abellotti can't use parse_fetch_group because it uses resource_search. Will update to use parse_group though

[AllenBW]

I know everyone is da 🐝, wanted to do a quick status check on this 👶

🙇‍♀️ ⭕️

[abellotti]

I think I'm good with this, @jntullo can you take care of the rubocop warning ? Thanks.

[jntullo]

@abellotti the rubocop warning can't be fixed, it is part of the solution to the current issue

[abellotti]

Hmm, can user.current_group = new_group be done instead ? If not, maybe simply add a comment in the code that update_attributes! cannot be used just so that it's not inadvertently "fixed" in the future. Thanks.

[miq-bot]

Checked commit jntullo@119312c with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 2 offenses detected

**

• 💣 💥 🔥 🚒 - Linter/Yaml - missing config files

app/controllers/api/users_controller.rb

• ❗ - Line 58, Col 14 - Rails/SkipsModelValidations - Avoid using update_attribute because it skips validations.

[abellotti]

Thanks @jntullo for updating this. 👍

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit 29014b7a216d843ee8a46106f0601d60e98aaa91
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Mon Nov 13 16:47:43 2017 -0500

    Merge pull request #176 from jntullo/bug/change_current_group
    
    Add a set_current_group method for users
    (cherry picked from commit 8f30e9cd9894e302add1729d7aa7bb54764db33b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1513191