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.

Sign up for GitHub

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

Add my_settings_view product feature for the My Settings page #22779

Merged
merged 2 commits into from
Nov 14, 2023
Merged

Add my_settings_view product feature for the My Settings page #22779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8b19be
Add my_settings_view product feature for the My Settings page
Fryguy Nov 7, 2023
aa96c28
Fix issues where users can sometimes not see themselves from RBAC
Fryguy Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions app/models/miq_group.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,16 @@ def settings=(new_settings)
super(indifferent_settings)
end

def self.with_roles_excluding(identifier)
where.not(:id => MiqGroup.unscope(:select).joins(:miq_product_features)
.where(:miq_product_features => {:identifier => identifier})
.select(:id))
def self.with_roles_excluding(identifier, allowed_ids: nil)
scope = where.not(
:id => MiqGroup
.unscope(:select)
.joins(:miq_product_features)
.where(:miq_product_features => {:identifier => identifier})
.select(:id)
)
scope = scope.or(where(:id => allowed_ids)) if allowed_ids.present?
scope
end

def self.next_sequence
Expand Down
14 changes: 10 additions & 4 deletions app/models/miq_user_role.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,16 @@ def limited_self_service?(klass = nil)
(settings || {}).fetch_path(:restrictions, restriction_type(klass)) == :user
end

def self.with_roles_excluding(identifier)
where.not(:id => MiqUserRole.unscope(:select).joins(:miq_product_features)
.where(:miq_product_features => {:identifier => identifier})
.select(:id))
def self.with_roles_excluding(identifier, allowed_ids: nil)
scope = where.not(
:id => MiqUserRole
.unscope(:select)
.joins(:miq_product_features)
.where(:miq_product_features => {:identifier => identifier})
.select(:id)
)
scope = scope.or(where(:id => allowed_ids)) if allowed_ids.present?
scope
end

def self.seed
Expand Down
14 changes: 10 additions & 4 deletions app/models/user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,16 @@ class User < ApplicationRecord

scope :in_all_regions, ->(id) { where(:userid => User.default_scoped.where(:id => id).select(:userid)) }

def self.with_roles_excluding(identifier)
where.not(:id => User.unscope(:select).joins(:miq_groups => :miq_product_features)
.where(:miq_product_features => {:identifier => identifier})
.select(:id))
def self.with_roles_excluding(identifier, allowed_ids: nil)
scope = where.not(
:id => User
.unscope(:select)
.joins(:miq_groups => :miq_product_features)
.where(:miq_product_features => {:identifier => identifier})
.select(:id)
)
scope = scope.or(where(:id => allowed_ids)) if allowed_ids.present?
scope
end

def self.scope_by_tenant?
Expand Down
4 changes: 4 additions & 0 deletions db/fixtures/miq_product_features.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2815,6 +2815,10 @@
:feature_type: node
:identifier: my_settings
:children:
- :name: View
:description: View My Settings
:feature_type: view
:identifier: my_settings_view
Copy link
Member Author

Choose a reason for hiding this comment

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

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

- :name: Modify
:description: Modify My Settings
:feature_type: admin
Expand Down
14 changes: 14 additions & 0 deletions db/fixtures/miq_user_roles.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@
- host_initiator
- host_initiator_group
- miq_task_my_ui
- my_settings_view
- my_settings_default_views
- my_settings_time_profiles
- my_settings_visuals
Expand Down Expand Up @@ -232,6 +233,7 @@
- host_perf
- host_tag
- host_timeline
- my_settings_view
- my_settings_default_views
- my_settings_time_profiles
- my_settings_visuals
Expand Down Expand Up @@ -334,6 +336,7 @@
- host_check_compliance
- host_tag
- miq_task_my_ui
- my_settings_view
- my_settings_default_views
- my_settings_time_profiles
- my_settings_visuals
Expand Down Expand Up @@ -419,6 +422,7 @@
- miq_template_show
- miq_template_show_list
- miq_template_timeline
- my_settings_view
- my_settings_default_views
- my_settings_time_profiles
- my_settings_visuals
Expand Down Expand Up @@ -526,6 +530,7 @@
- host_perf
- host_tag
- host_timeline
- my_settings_view
- my_settings_default_views
- my_settings_time_profiles
- my_settings_visuals
Expand Down Expand Up @@ -621,6 +626,7 @@
- host_perf
- host_tag
- host_timeline
- my_settings_view
- my_settings_default_views
- my_settings_time_profiles
- my_settings_visuals
Expand Down Expand Up @@ -707,6 +713,7 @@
- host_tag
- host_timeline
- miq_task_my_ui
- my_settings_view
- my_settings_default_views
- my_settings_time_profiles
- my_settings_visuals
Expand Down Expand Up @@ -795,6 +802,7 @@
- host_tag
- host_timeline
- miq_task_my_ui
- my_settings_view
- my_settings_default_views
- my_settings_time_profiles
- my_settings_visuals
Expand Down Expand Up @@ -900,6 +908,7 @@
- miq_request_admin
- miq_request_view
- miq_task_my_ui
- my_settings_view
- my_settings_default_views
- my_settings_time_profiles
- my_settings_visuals
Expand Down Expand Up @@ -956,6 +965,7 @@
- miq_request_admin
- miq_request_view
- miq_task_my_ui
- my_settings_view
- my_settings_default_views
- my_settings_visuals
- service_edit
Expand Down Expand Up @@ -1053,6 +1063,7 @@
- host_initiator_group_show_list
- host_initiator_group_edit
- miq_task_my_ui
- my_settings_view
- my_settings_default_views
- my_settings_time_profiles
- my_settings_visuals
Expand Down Expand Up @@ -1159,6 +1170,7 @@
- miq_template_snapshot
- miq_template_tag
- miq_template_timeline
- my_settings_view
- my_settings_default_views
- my_settings_visuals
- miq_request_admin
Expand Down Expand Up @@ -1222,6 +1234,7 @@
- miq_template_snapshot
- miq_template_tag
- miq_template_timeline
- my_settings_view
- my_settings_default_views
- my_settings_visuals
- vm_check_compliance
Expand Down Expand Up @@ -1394,6 +1407,7 @@
- dashboard
- chargeback
- miq_report
- my_settings_view

- :name: EvmRole-container_administrator
:read_only: true
Expand Down
26 changes: 22 additions & 4 deletions lib/rbac/filterer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,17 +628,35 @@ def scope_to_cloud_tenant(scope, user, miq_group)
def scope_for_user_role_group(klass, scope, miq_group, user, managed_filters)
user_or_group = miq_group || user

if user_or_group.try!(:self_service?) && MiqUserRole != klass
if user_or_group.try!(:self_service?) && klass != MiqUserRole
scope.where(:id => klass == User ? user.id : miq_group.id)
else
role = user_or_group.miq_user_role
# hide creating admin group / roles from non-super administrators

# Exclude users/groups/roles tied to the super admin user if the current user isn't also
# a super admin user. This prevents a tenant admin from creating a super admin and then
# escalating privileges.
unless role&.super_admin_user?
scope = scope.with_roles_excluding(MiqProductFeature::SUPER_ADMIN_FEATURE)
# 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.
Comment on lines +640 to +642
Copy link
Member

@kbrock kbrock Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

#
# TODO: Determine if this same logic should apply to MiqUserRole as well
Copy link
Member

@kbrock kbrock Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

with_roles_excluding_options =
if klass == User
{:allowed_ids => user.id}
elsif klass == MiqGroup
{:allowed_ids => miq_group.id}
else
{}
end

scope = scope.with_roles_excluding(MiqProductFeature::SUPER_ADMIN_FEATURE, **with_roles_excluding_options)
end

if MiqUserRole != klass
if klass != MiqUserRole
filtered_ids = pluck_ids(get_managed_filter_object_ids(scope, managed_filters))

# Non tenant admins can only see their own groups. Note - a super admin is also a tenant admin
scope = scope.with_groups(user.miq_group_ids) unless role&.tenant_admin_user?
end
Expand Down
Loading