Add my_settings_view product feature for the My Settings page #22779

Fryguy · 2023-11-07T19:26:28Z

Additionally fixes an RBAC issue where users can sometimes not see themselves.

Depended on by

Fryguy · 2023-11-07T21:27:34Z

db/fixtures/miq_product_features.yml

+  - :name: View
+    :description: View My Settings
+    :feature_type: view
+    :identifier: my_settings_view


Note to self - we might want a database migration adding this to any existing role that has one of the other my_settings permissions.

lib/rbac/filterer.rb

kbrock · 2023-11-08T01:30:05Z

lib/rbac/filterer.rb

+          #   a super admin by virtue of being in multiple groups, then they will be filtered by
+          #   with_roles_excluding. The allowed_ids option ensures the user can still see themselves.
+          #
+          # TODO: Determine if this same logic should apply to MiqUserRole as well


So we would want role here to prevent a tenant admin from creating a group with super admin privs? i.e.: "everything"?

I'm not really sure here. I need to allow the user themselves when it gets filtered out, but if a currently-non-admin user can be a super-admin, does that mean they should or shouldn't be able to see the super admin role? I'm just not sure.

the goal here is to prevent a tenant admin from creating a super user. (that they then know the password so they can escalate privileges.

You can do that via using a super group, or creating a group and assigning a super role

kbrock · 2023-11-08T01:31:36Z

lib/rbac/filterer.rb

+          # In the case that the user is not currently in a super admin group, but can _become_
+          #   a super admin by virtue of being in multiple groups, then they will be filtered by
+          #   with_roles_excluding. The allowed_ids option ensures the user can still see themselves.


yea, this edge case is only for when an admin changes to another group to determine what that other group can see. (to view what one of their users can see)

Such an edge case, but an important one to help admins understand the security implications

kbrock · 2023-11-08T02:03:43Z

spec/lib/rbac/filterer_spec.rb

      end

-      context "with self-service user" do
+      context "where the user is in another group that is a regular group" do


I really like using with for contexts.

Suggested change

context "where the user is in another group that is a regular group" do

context "with a regular group" do

But maybe where is better since the subject is the rbac and not the user

Suggested change

context "where the user is in another group that is a regular group" do

context "with the user also in a regular group" do

Yeah I was debating the wording - I like the latter there.

kbrock

Yea,

I think you want to create shared_group in the case where a group is shared by other_user.

But I think you want other_group to be present in all cases where the main user is not in other_group.

That is the sad path.
That way we truly understand how the user views other groups. (groups in this tenant, but not a member)

unsure about this idea:
Don't have other_user.miq_groups << my_group, but use shared_group instead?

spec/lib/rbac/filterer_spec.rb

kbrock · 2023-11-08T02:09:51Z

spec/lib/rbac/filterer_spec.rb

-        other_user.miq_groups << group
-        get_rbac_results_for_and_expect_objects(User, [user, other_user])
-      end
+      context "with other users in the group" do


I think the fact that other users are in the group is a given/base case and should not be a context. (starts to make it indention happy)

yeah I was struggling here - it's important to have multiple users to show which ones are or aren't coming back. These tests are bigger than the "self" case.

I decided to keep it. Reason it that otherwise you just have these floating its and it's hard to tell why they aren't in a context and what makes them special.

kbrock · 2023-11-08T02:11:36Z

spec/lib/rbac/filterer_spec.rb

        end

-        it "returns only the current user" do
-          get_rbac_results_for_and_expect_objects(User, [user])
+        context "and the current_group is the original regular group" do


Suggested change

context "and the current_group is the original regular group" do

context "and the current_group is a regular group" do

unsure the difference between this context and the next context.
Is it simply that there are or are not other users in the group?

Maybe just create a user in the group and not in the group and you'll handle the happy and sad paths?

aah. If you use shared_group, I think the language will end up better. (so you could use shared group for a context where the group is shared)

Yeah i used shared group

kbrock · 2023-11-08T02:14:38Z

spec/lib/rbac/filterer_spec.rb

        end

-        let!(:user_role) do
-          FactoryBot.create(:miq_user_role, :role => "user")
+        it "returns only the current user" do


is this avoiding returning other users in this group?

Yes, self_service is defined to only return yourself even if other users are present, whereas a non-self_service group is defined to returns all users in the groups.

kbrock · 2023-11-08T02:15:00Z

spec/lib/rbac/filterer_spec.rb

        end
+      end
+
+      context "with a self_service user" do


this is a regular self service user and not a limited self service user. right?

That's correct

kbrock · 2023-11-08T02:15:35Z

spec/lib/rbac/filterer_spec.rb

        end

+        context "that is in multiple groups" do


do we need separate tests for a single group vs multiple groups?

Probably not, but I was struggling here. It felt important to me to have a bunch of tests dedicated to multiple groups, so that future changes could take them into account specifically.

kbrock · 2023-11-08T02:17:22Z

spec/lib/rbac/filterer_spec.rb

+            end
+
+            it "returns user's groups" do
+              get_rbac_results_for_and_expect_objects(MiqGroup, [other_group]) # TODO: should this be [group, other_group] ???


no. I think this is right.
Per the other examples, other_group is the only active/current group

(yes, I wish current_group were not a concept and both were returned, but that is not the current concept)

kbrock · 2023-11-08T02:20:22Z

spec/lib/rbac/filterer_spec.rb

-          allow_any_instance_of(MiqGroup).to receive_messages(:self_service? => true)
+          other_user # ensure we create another user in the other group
+
+          user.miq_groups << other_group


if you want both users to be in the same group, please create a new group and put both users into it shared_group?

miq-bot · 2023-11-14T22:01:05Z

Checked commits Fryguy/manageiq@f8b19be~...aa96c28 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
6 files checked, 1 offense detected

lib/rbac/filterer.rb

❗ - Line 631, Col 10 - Rails/SafeNavigation - Use safe navigation (&.) instead of try!.

kbrock · 2023-11-14T22:45:36Z

spec/lib/rbac/filterer_spec.rb

-        let!(:super_administrator_user_role) do
-          FactoryBot.create(:miq_user_role, :role => "super_administrator")
+        before do
+          other_user # ensure we create another user in another group


thank you for including the unhappy path. this gives me a lot of confidence

Fryguy · 2023-11-14T23:19:36Z

Backported to quinteros in commit cb2ab3b.

commit cb2ab3b31626fc372a8e3a9eb9494c9372013d79
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Tue Nov 14 17:49:15 2023 -0500

    Merge pull request #22779 from Fryguy/fix_my_settings
    
    Add my_settings_view product feature for the My Settings page
    
    (cherry picked from commit 6ebac25d22a501a1522834a23a111ed5a101d407)

Add my_settings_view product feature for the My Settings page (cherry picked from commit 6ebac25)

Fryguy requested review from agrare, bdunne and kbrock as code owners November 7, 2023 19:26

This was referenced Nov 7, 2023

Use new dedicated my_settings_view permission ManageIQ/manageiq-ui-classic#8957

Merged

Allow the user to list and see themselves even without rbac_user_show ManageIQ/manageiq-api#1230

Merged

Fryguy force-pushed the fix_my_settings branch from bcc32bd to 4f50270 Compare November 7, 2023 21:17

Fryguy changed the title ~~Add my_settings_view product feature for the My Settings page~~ [WIP] Add my_settings_view product feature for the My Settings page Nov 7, 2023

Fryguy assigned kbrock Nov 7, 2023

Fryguy added the bug label Nov 7, 2023

miq-bot added the wip label Nov 7, 2023

Fryguy commented Nov 7, 2023

View reviewed changes

kbrock reviewed Nov 8, 2023

View reviewed changes

lib/rbac/filterer.rb Outdated Show resolved Hide resolved

kbrock reviewed Nov 8, 2023

View reviewed changes

Add my_settings_view product feature for the My Settings page

f8b19be

Fryguy force-pushed the fix_my_settings branch from 4f50270 to a8863e6 Compare November 14, 2023 21:41

Fryguy changed the title ~~[WIP] Add my_settings_view product feature for the My Settings page~~ Add my_settings_view product feature for the My Settings page Nov 14, 2023

miq-bot removed the wip label Nov 14, 2023

Fryguy force-pushed the fix_my_settings branch from a8863e6 to 238282a Compare November 14, 2023 21:56

Fix issues where users can sometimes not see themselves from RBAC

aa96c28

Fryguy force-pushed the fix_my_settings branch from 238282a to aa96c28 Compare November 14, 2023 21:57

Fryguy added the quinteros/yes label Nov 14, 2023

kbrock reviewed Nov 14, 2023

View reviewed changes

kbrock merged commit 6ebac25 into ManageIQ:master Nov 14, 2023
6 checks passed

Fryguy pushed a commit that referenced this pull request Nov 14, 2023

Merge pull request #22779 from Fryguy/fix_my_settings

cb2ab3b

Add my_settings_view product feature for the My Settings page (cherry picked from commit 6ebac25)

Fryguy removed the quinteros/yes label Nov 14, 2023

Fryguy added the quinteros/backported label Nov 14, 2023

Fryguy deleted the fix_my_settings branch November 14, 2023 23:20

kbrock mentioned this pull request Nov 16, 2023

Add my_settings_view to roles ManageIQ/manageiq-schema#713

Merged

1 task

GilbertCherrie mentioned this pull request Nov 23, 2023

Issues with My Settings permissions ManageIQ/manageiq-ui-classic#8970

Closed

jrafanie mentioned this pull request Jan 5, 2024

Update English Translations 2023-12-26T17:22:29Z #22789

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add my_settings_view product feature for the My Settings page #22779

Add my_settings_view product feature for the My Settings page #22779

Fryguy commented Nov 7, 2023 •

edited by kbrock

Loading

Fryguy Nov 7, 2023

kbrock Nov 8, 2023 •

edited

Loading

Fryguy Nov 8, 2023

kbrock Nov 8, 2023

kbrock Nov 8, 2023 •

edited

Loading

kbrock Nov 8, 2023 •

edited

Loading

Fryguy Nov 8, 2023

kbrock left a comment •

edited

Loading

kbrock Nov 8, 2023

Fryguy Nov 8, 2023

Fryguy Nov 14, 2023

kbrock Nov 8, 2023

kbrock Nov 8, 2023

Fryguy Nov 14, 2023

kbrock Nov 8, 2023

Fryguy Nov 8, 2023

kbrock Nov 8, 2023

Fryguy Nov 14, 2023

kbrock Nov 8, 2023

Fryguy Nov 8, 2023

kbrock Nov 8, 2023

kbrock Nov 8, 2023

miq-bot commented Nov 14, 2023

kbrock Nov 14, 2023

Fryguy commented Nov 14, 2023

	context "where the user is in another group that is a regular group" do
	context "with a regular group" do

	context "where the user is in another group that is a regular group" do
	context "with the user also in a regular group" do

	context "and the current_group is the original regular group" do
	context "and the current_group is a regular group" do

Add my_settings_view product feature for the My Settings page #22779

Add my_settings_view product feature for the My Settings page #22779

Conversation

Fryguy commented Nov 7, 2023 • edited by kbrock Loading

Choose a reason for hiding this comment

kbrock Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

kbrock Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Nov 14, 2023

Choose a reason for hiding this comment

Fryguy commented Nov 14, 2023

Fryguy commented Nov 7, 2023 •

edited by kbrock

Loading

kbrock Nov 8, 2023 •

edited

Loading

kbrock Nov 8, 2023 •

edited

Loading

kbrock Nov 8, 2023 •

edited

Loading

kbrock left a comment •

edited

Loading