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.

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

Use feature for admin #17444

Merged
merged 5 commits into from
May 31, 2018
Merged

Use feature for admin #17444

merged 5 commits into from
May 31, 2018

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented May 18, 2018

Problem:

Users can not create a custom group with admin privileges.
Our best practices say to create a custom group and not use the included Evm ones.
The reason for this request most often is focused on reporting, but also focuses on miq_request privileges.

Before:

  • We use the admin's name (admin, tenant admin, and super admin) to grant extra privileges. This is determined using MiqUserRole#admin_user?.
  • Tenant admin checks are in place to prevent the user from creating an administrator or super administrator (and essentially escalating privileges).
  • NOTE: special privileges of the administrator role has been whittled down to elevated report and miq_request privileges.

After:

  • super-administrator role (i.e.: user admin) now uses the the existing everything miq_product_feature to grant the special privilege.
  • Report functionality previously protected by admin_user? now uses the miq_product_feature miq_report_superadmin to grant special privileges. (This is needed for chargeback and other reports). Introduced report_admin_user? for transition.
  • MiqRequest functionality previously protected by admin_user? now uses miq_product feature miq_request_superadmin
  • The administrator role checks admin_user? is deprecated and no longer used. Essentially, this role has been removed.
  • The tenant-administrator checks have been simplified. Now, a super admin is the only one able to create a super admin.

Future work:

  • The code currently prevents users from viewing other groups. This needs to be fixed in admin.
  • A bunch of these checks could be moved further into Rbac.
  • There are quite a few report / report result checks that are tangled and non trivial to move to rbac.
  • There are 2 types of checks for report results - tread carefully.

Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

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

I think this looks fine, but admittedly I am not the best person to be making the final say.

I think my only real gripe is that MiqProductFeature seems like a weird place for this to live instead of MiqUserRole, specifically from a name standpoint. I can much more easily understand that "admin" and "superadmin" are roles versus "product features", but maybe that is just me.

There might be implementation details that make it more desirable to put this in MiqProductFeature versus MiqUserRole, so not really worth holding this up for, just an observation.

def self.with_current_user_groups(user = nil)
current_user = user || User.current_user
current_user.admin_user? ? all : where(:id => current_user.miq_group_ids)
# paralell to User.with_groups - only show these groups
Copy link
Member

Choose a reason for hiding this comment

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

Typo: parallel

where.not(:id => MiqUserRole.joins(:miq_product_features)
.where(:miq_product_features => {:identifier => identifier})
.select(:id)
)
Copy link
Member

Choose a reason for hiding this comment

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

Hot. 👍

@@ -25,14 +25,15 @@ class User < ApplicationRecord
belongs_to :current_group, :class_name => "MiqGroup"
has_and_belongs_to_many :miq_groups
scope :superadmins, lambda {
joins(:miq_groups => :miq_user_role).where(:miq_user_roles => {:name => MiqUserRole::SUPER_ADMIN_ROLE_NAME })
joins(:miq_groups => {:miq_user_role => :miq_product_features})
.where(:miq_product_features => {:identifier => MiqProductFeature::SUPER_ADMIN_FEATURE })
Copy link
Member

Choose a reason for hiding this comment

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

First off, totally fine with this query, and it probably doesn't need to change.

But am kinda curious where it is being used. Since we aren't setting a .select on this, do we get all the columns back on this, or does Rails always add a table_name.* by default instead of just a SELECT *?

(might be the former now that I think about it)

Anyway, you really aren't changing how this worked much before, so you probably don't have to worry about it.

if user_or_group.disallowed_roles
scope = scope.with_allowed_roles_for(user_or_group)
# hide creating admin group / roles from tenant administrators
if !user_or_group.miq_user_role&.admin_user?
Copy link
Member

Choose a reason for hiding this comment

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

<rant> God... this is the ugliest syntax addition to Ruby I have ever seen... bleh </rant>

Copy link
Member

Choose a reason for hiding this comment

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

More of a serious, but minor question: Is there an extra query being hit here now by calling out to miq_user_role?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. But I figure features need to be brought back for the rest of the UI, so I've been looking the other way

def self.with_current_user_groups(user = nil)
user ||= current_user
user.admin_user? ? all : includes(:miq_groups).where(:miq_groups => {:id => user.miq_group_ids})
# paralell to MiqGroup.with_groups - only show users with these groups
Copy link
Member

Choose a reason for hiding this comment

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

Typo: parallel

@@ -171,7 +175,8 @@ def self.lookup_by_identity(username)
end

def self.authorize_user(userid)
return if userid.blank? || admin?(userid)
# Don't authorize users with userid == "admin"
return if userid.blank? || userid == "admin"
Copy link
Member

Choose a reason for hiding this comment

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

Few things:

  • At the very least, can we .freeze this string? Or use an existing constant?
  • Is there a reason this doesn't go through MiqUserRole or MiqProductFeature? I guess for simplicity and reducing risk? (cool with that being the case, just making sure)

end

if MiqUserRole != klass
if MiqUserRole == klass
scope
Copy link
Member

Choose a reason for hiding this comment

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

What is the rational behind adding this conditional in and pushing scope_by_ids inside the else? Just don't see the context/rational from the diff.

Copy link
Member Author

Choose a reason for hiding this comment

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

think I'll roll it back - yea, probably an unneeded change

Copy link
Member

Choose a reason for hiding this comment

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

Not sure you need to go that far, was just curious what the rational was.

@@ -60,8 +60,8 @@ def settings=(new_settings)
super(indifferent_settings)
end

def self.with_allowed_roles_for(user_or_group)
includes(:miq_user_role).where.not({:miq_user_roles => {:name => user_or_group.disallowed_roles}})
def self.with_roles_excluding(disallowed_roles)
Copy link
Member

Choose a reason for hiding this comment

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

Are all of the callers for this updated? I don't see any changed here.

Copy link
Member

Choose a reason for hiding this comment

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

Thankfully these ones got hidden due to recent commits. Just noticed where these were being used, so this question and others like it can be ignored.


def self.with_allowed_roles_for(user_or_group)
where.not(:name => user_or_group.disallowed_roles)
def self.with_roles_excluding(disallowed_roles)
Copy link
Member

Choose a reason for hiding this comment

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

Are all of the callers for this updated? I don't see any changed here. (repeat question)

@@ -50,8 +50,8 @@ class User < ApplicationRecord

scope :with_same_userid, ->(id) { where(:userid => User.find(id).userid) }

def self.with_allowed_roles_for(user_or_group)
includes(:miq_groups => :miq_user_role).where.not(:miq_user_roles => {:name => user_or_group.disallowed_roles})
def self.with_roles_excluding(disallowed_roles)
Copy link
Member

Choose a reason for hiding this comment

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

Are all of the callers for this updated? I don't see any changed here. (repeat question)

@kbrock
Copy link
Member Author

kbrock commented May 23, 2018

@bascar / @gtanzillo new thoughts:

Currently we have a single "administrator" Role. the name of the role makes it an admin.

This is not transferrable to other roles, so instead of using name, we're going to use a product feature. Also, we are getting away from a single admin concept.

If you look at the product features granted to admins, they fall in 2 different areas. It makes sense to split up the admin role into 2 types of functionality: reports and requests.

Additionally, there currently are provisions to prevent tenant admins from creating an administrator user. While it makes sense to limit creation of a super admin, the admin does not have significant added privileges, so this no longer seems necessary. Instead it prevents non super admins from creating super admins.


Current functionality granted to Administrators

NOTE: While super administrators do get all administrator functionality, they are different roles

  • reports
    • advanced search (delete filters, global search)
    • other user's saved reports
    • export other reports
    • fetch other user's saved reports
    • other user's report results
    • other user's time profiles
    • other user's charge back reports
    • other groups' reports
    • report results from other users
    • overwrite on report imports
    • view other user groups / (potential create user issue)
  • requests
    • delete other user's miq_request messages
    • find service requests from other users (API)
    • find requests from other users
    • provision requests from other users
    • automation requests from other users

Not sure if this can address

@kbrock
Copy link
Member Author

kbrock commented May 25, 2018

This made the following changes:

role admin

move hardcoded admin logic over to rbac
Admins have 2 types of escalated privileges
report and request

This separates the types (with 2 different product features)
@miq-bot
Copy link
Member

miq-bot commented May 29, 2018

Checked commits kbrock/manageiq@9c9a52f~...f6c02e8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
14 files checked, 0 offenses detected
Everything looks fine. 🏆

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

This looks awesome 👍 I just have the one small question/comment

@@ -15,11 +15,11 @@ class MiqGroup < ApplicationRecord
has_many :miq_widget_sets, :as => :owner, :dependent => :destroy
has_many :miq_product_features, :through => :miq_user_role

virtual_column :miq_user_role_name, :type => :string, :uses => :miq_user_role
virtual_delegate :name, :to => :miq_user_role, :allow_nil => true, :prefix => true
Copy link
Member

Choose a reason for hiding this comment

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

How come you changed this to just :name? It seems ambiguous as it can be misconstrued as the name of the group when it's really the name of the groups role.

Copy link
Member Author

@kbrock kbrock May 31, 2018

Choose a reason for hiding this comment

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

This did not change

The :name refers to Role#name. The prefix => true tacks on the association name so the attribute name will be called miq_user_role_name (same as it originally was)

@gtanzillo gtanzillo added this to the Sprint 87 Ending Jun 4, 2018 milestone May 31, 2018
@gtanzillo gtanzillo merged commit f197805 into ManageIQ:master May 31, 2018
@kbrock kbrock deleted the super_admin_feature branch May 31, 2018 19:51
@ZitaNemeckova
Copy link
Contributor

ZitaNemeckova commented Jun 1, 2018

This broke UI master :)

1) ReportController#schedule_edit reset rbac testing
     Failure/Error: users_in_current_groups = User.with_current_user_groups.distinct.sort_by { |u| u.name.downcase }
     
     NoMethodError:
       undefined method `with_current_user_groups' for #<Class:0x000000000a44fa98>
     # ./app/controllers/application_controller.rb:764:in `build_user_emails_for_edit'
     # ./app/controllers/report_controller/schedules.rb:433:in `schedule_build_edit_screen'
     # ./app/controllers/report_controller/schedules.rb:252:in `schedule_edit'
     # ./spec/controllers/report_controller/schedules_spec.rb:21:in `block (3 levels) in <top (required)>'
  2) MiqPolicyController::Alerts test click on toolbar button alert edit
     Failure/Error: expect(response.status).to eq(200)
     
       expected: 200
            got: 500
     
       (compared using ==)
     # ./spec/controllers/miq_policy_controller/alerts_spec.rb:118:in `block (4 levels) in <top (required)>'
  3) MiqPolicyController::Alerts test click on toolbar button alert copy
     Failure/Error: expect(response.status).to eq(200)
     
       expected: 200
            got: 500
     
       (compared using ==)
     # ./spec/controllers/miq_policy_controller/alerts_spec.rb:123:in `block (4 levels) in <top (required)>'
  4) MiqPolicyController::Alerts alert edit #alert_build_edit_screen it should select correct record when editing an existing alert
     Failure/Error: users_in_current_groups = User.with_current_user_groups.distinct.sort_by { |u| u.name.downcase }
     
     NoMethodError:
       undefined method `with_current_user_groups' for #<Class:0x000000000a44fa98>
     # ./app/controllers/application_controller.rb:764:in `build_user_emails_for_edit'
     # ./app/controllers/miq_policy_controller/alerts.rb:288:in `alert_build_edit_screen'
     # ./spec/controllers/miq_policy_controller/alerts_spec.rb:46:in `block (5 levels) in <top (required)>'
  5) MiqPolicyController::Alerts alert edit #alert_build_edit_screen it should skip id when copying all attributes of an existing alert
     Failure/Error: users_in_current_groups = User.with_current_user_groups.distinct.sort_by { |u| u.name.downcase }
     
     NoMethodError:
       undefined method `with_current_user_groups' for #<Class:0x000000000a44fa98>
     # ./app/controllers/application_controller.rb:764:in `build_user_emails_for_edit'
     # ./app/controllers/miq_policy_controller/alerts.rb:288:in `alert_build_edit_screen'
     # ./spec/controllers/miq_policy_controller/alerts_spec.rb:40:in `block (5 levels) in <top (required)>'
  6) ApplicationController#build_user_emails_for_edit listing users's emails which belongs to current user's groups and some of them was already selected
     Failure/Error: users_in_current_groups = User.with_current_user_groups.distinct.sort_by { |u| u.name.downcase }
     
     NoMethodError:
       undefined method `with_current_user_groups' for #<Class:0x000000000a44fa98>
     # ./app/controllers/application_controller.rb:764:in `build_user_emails_for_edit'
     # ./spec/controllers/application_controller_spec.rb:291:in `block (4 levels) in <top (required)>'
     # ./spec/controllers/application_controller_spec.rb:290:in `block (3 levels) in <top (required)>'
  7) ApplicationController#build_user_emails_for_edit finds users with groups which belongs to current user's groups
     Failure/Error: user_ids = User.with_current_user_groups.collect(&:userid)
     
     NoMethodError:
       undefined method `with_current_user_groups' for #<Class:0x000000000a44fa98>
     # ./spec/controllers/application_controller_spec.rb:264:in `block (3 levels) in <top (required)>'
  8) ApplicationController#build_user_emails_for_edit listing users's emails which belongs to current user's groups
     Failure/Error: users_in_current_groups = User.with_current_user_groups.distinct.sort_by { |u| u.name.downcase }
     
     NoMethodError:
       undefined method `with_current_user_groups' for #<Class:0x000000000a44fa98>
     # ./app/controllers/application_controller.rb:764:in `build_user_emails_for_edit'
     # ./spec/controllers/application_controller_spec.rb:273:in `block (4 levels) in <top (required)>'
     # ./spec/controllers/application_controller_spec.rb:272:in `block (3 levels) in <top (required)>'

Fixed in ManageIQ/manageiq-ui-classic#4023 (Merged)

ZitaNemeckova added a commit to ZitaNemeckova/manageiq-ui-classic that referenced this pull request Jun 1, 2018
Fixes mangeiq-ui-classic master failures

Introduced by ManageIQ/manageiq#17444
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 13, 2018
We introduced feature miq_request_superadmin in f6c02e8 / ManageIQ#17444 
The feature miq_request_approval already existed.
Merging the 2 together.

This fixes an inconsistency between API and UI
The issue existed before this feature was introduced
so this works with 17444 to fix the BZ

ManageIQ#17444
https://bugzilla.redhat.com/show_bug.cgi?id=1608554
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 13, 2018
In ManageIQ#17444 and related PRs
References to generic admin_user and used request_admin_user/others

This removes the method for good
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 13, 2018
In ManageIQ#17444 and related PRs
References to generic admin_user and used request_admin_user/others

This removes the method for good
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 13, 2018
In ManageIQ#17444 and related PRs
References to generic admin_user and used request_admin_user/others

This removes the method for good
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 13, 2018
In ManageIQ#17444 and related PRs
References to generic admin_user and used request_admin_user/others

This removes the method for good
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 14, 2018
We introduced feature miq_request_superadmin in f6c02e8 / ManageIQ#17444 
The feature miq_request_approval already existed.
Merging the 2 together.

This fixes an inconsistency between API and UI
The issue existed before this feature was introduced
so this works with 17444 to fix the BZ

ManageIQ#17444
https://bugzilla.redhat.com/show_bug.cgi?id=1608554
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 14, 2018
We introduced feature miq_request_superadmin in f6c02e8 / ManageIQ#17444 
The feature miq_request_approval already existed.
Merging the 2 together.

This fixes an inconsistency between API and UI
The issue existed before this feature was introduced
so this works with 17444 to fix the BZ

ManageIQ#17444
https://bugzilla.redhat.com/show_bug.cgi?id=1608554
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 15, 2018
We introduced feature miq_request_superadmin in f6c02e8 / ManageIQ#17444
The feature miq_request_approval already existed.
Merging the 2 together.

This fixes an inconsistency between API and UI
The issue existed before this feature was introduced
so this works with 17444 to fix the BZ

ManageIQ#17444
https://bugzilla.redhat.com/show_bug.cgi?id=1608554
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 15, 2018
We introduced feature miq_request_superadmin in f6c02e8 / ManageIQ#17444
The feature miq_request_approval already existed.
Merging the 2 together.

This fixes an inconsistency between API and UI
The issue existed before this feature was introduced
so this works with 17444 to fix the BZ

ManageIQ#17444
https://bugzilla.redhat.com/show_bug.cgi?id=1608554
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 16, 2018
We introduced feature miq_request_superadmin in f6c02e8 / ManageIQ#17444
The feature miq_request_approval already existed.
Merging the 2 together.

This fixes an inconsistency between API and UI
The issue existed before this feature was introduced
so this works with 17444 to fix the BZ

ManageIQ#17444
https://bugzilla.redhat.com/show_bug.cgi?id=1608554
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 16, 2018
In ManageIQ#17444 and related PRs
References to generic admin_user and used request_admin_user/others

This removes the method for good
Ladas pushed a commit to Ladas/manageiq that referenced this pull request Aug 21, 2018
We introduced feature miq_request_superadmin in f6c02e8 / ManageIQ#17444 
The feature miq_request_approval already existed.
Merging the 2 together.

This fixes an inconsistency between API and UI
The issue existed before this feature was introduced
so this works with 17444 to fix the BZ

ManageIQ#17444
https://bugzilla.redhat.com/show_bug.cgi?id=1608554
NickLaMuro added a commit to NickLaMuro/manageiq-api that referenced this pull request Dec 5, 2018
The fix here simply matches up the `if` checks used in:

- `Api::ReportsController#reports_search_conditions`
- `MiqReport.for_user`

to both use `User#report_admin_user?`

* * *

Further background info:

This bug is the result of a collision between the following two PRs:

- ManageIQ/manageiq#15472
- ManageIQ/manageiq#17444

The first PR, ManageIQ/manageiq#15472, introduces:

`Api::ReportsController#reports_search_conditions`

Which uses `unless User.current_user.admin?` to check if it should use
the `where_clause` from the `MiqReport.for_user` for the search
conditions (passed into a `ActiveRecord` `.where` farther up the stack).
The idea for this initial check is building this where clause only makes
sense if it isn't an `all`, otherwise where it is used will cause an
error in Postgres syntax because of `... WHERE ()` being passed

This, however, doesn't match the `user.admin_user?`(at the time of
ManageIQ/manageiq#15472) check that is done in `.for_user` method,
which means it would be called for users that don't match the `admin`
username, but still are part of either of the following roles

- `"EvmRole-super_administrator"`
- `"EvmRole-administrator"`

The bug in question:

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

tested this with a custom role, which doesn't match either of those two
roles, so the `User#admin?` check and `User#admin_user?` check happened
to be equivalent in `5.9` (see comment ManageIQ#4 in BZ link), but not `5.10`.
But, if the replication steps had instead said to:

- Create a user, "User1", that is part of the "EvmGroup-administrator"
  group

This would have also failed in the same way on `5.9` as it does on
`5.10` currently.

This fails on 5.10, though, because in ManageIQ/manageiq#17444, the
`admin_user?` method changes to `report_admin_user?`, and it's
functionality also now checks if the role is valid for querying the
group role's product features the user has access to, and it only has an
eject for the role that happens to be `admin?`.  Meaning that the
`admin` "super user" still has no problem with this query, but any other
user that could still count as a `report_admin_user?` would (both users
in the `EvmGroup-administrator` role and just having the proper features
enabled)
NickLaMuro added a commit to NickLaMuro/manageiq-api that referenced this pull request Dec 5, 2018
The fix here simply matches up the `if` checks used in:

- `Api::ReportsController#reports_search_conditions`
- `MiqReport.for_user`

to both use `User#report_admin_user?`

* * *

Further background info:

This bug is the result of a collision between the following two PRs:

- ManageIQ/manageiq#15472
- ManageIQ/manageiq#17444

The first PR, ManageIQ/manageiq#15472, introduces:

`Api::ReportsController#reports_search_conditions`

Which uses `unless User.current_user.admin?` to check if it should use
the `where_clause` from the `MiqReport.for_user` for the search
conditions (passed into a `ActiveRecord` `.where` farther up the stack).
The idea for this initial check is building this where clause only makes
sense if it isn't an `all`, otherwise where it is used will cause an
error in Postgres syntax because of `... WHERE ()` being passed

This, however, doesn't match the `user.admin_user?`(at the time of
ManageIQ/manageiq#15472) check that is done in `.for_user` method,
which means it would be called for users that don't match the `admin`
username, but still are part of either of the following roles

- `"EvmRole-super_administrator"`
- `"EvmRole-administrator"`

The bug in question:

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

tested this with a custom role, which doesn't match either of those two
roles, so the `User#admin?` check and `User#admin_user?` check happened
to be equivalent in `5.9` (see comment ManageIQ#4 in BZ link), but not `5.10`.
But, if the replication steps had instead said to:

- Create a user, "User1", that is part of the "EvmGroup-administrator"
  group

This would have also failed in the same way on `5.9` as it does on
`5.10` currently.

This fails on 5.10, though, because in ManageIQ/manageiq#17444, the
`admin_user?` method changes to `report_admin_user?`, and it's
functionality also now checks if the role is valid for querying the
group role's product features the user has access to, and it only has an
eject for the role that happens to be `admin?`.  Meaning that the
`admin` "super user" still has no problem with this query, but any other
user that could still count as a `report_admin_user?` would (both users
in the `EvmGroup-administrator` role and just having the proper features
enabled)

Fixes:  https://bugzilla.redhat.com/show_bug.cgi?id=1650531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants