Features dependency for an API-driven UI

[skateman]

We're moving the classic UI to be much more an API-driven and this is already causing problems for roles with limited access to certain resources. For example the about modal would need at least read-only access to the information about the current Zone and Region. However, exposing the related features (if they exist) to the given role would also expose parts of the UI that we might not want to display. Also it's a bad user experience to select 1+N features in the features tree if we just need that one single feature.

There will be a similar problem with the Set Ownership form for VMs/Services if it becomes API driven as the role needs additional access to the /api/users/ and /api/groups. As the matching read features are tied to the UI and live under Configuration -> Access Control we might not want to expose them in the menu.

For now we don't have many places where we're using the API, but the problem will be growing in the future as our plan is to use the API more. Therefore, it is inevitable to make some changes on our current RBAC architecture.

I'm proposing an additional field in the miq_features.yml for each API-feature, let's call it dependent_api_features where we could define a list of features that would be exposed through the API when enabling the given feature. This would not expose anything unwanted on the UI and also prevent the 1+N UX problem from happening.

      - :name: Edit
        :description: Edit a VM
        :feature_type: admin
        :identifier: vm_edit
        :dependent_api_features:
          - rbac_user_show_list
          - rbac_group_show_list

If the above feature is selected, it would be allowed strictly via API only to also list users and groups. So when editing the ownership of the VM, we are able to build the dropdown for selecting the user/group for a limited role without exposing the Settings -> Access control.

I think we need this feature before we start having a lot of API-driven forms so it's easier to fill out the yaml with the dependencies when we're working on the same stuff in the UI area.

@kbrock @lpichler @martinpovolny @himdel @Fryguy

[himdel]

I think we need to distinguish between 2 sets of rbac features:

• actual features - a screen, a magic button on that screen
• rights - can access users, can access groups

Right now, we're conflating the 2 kinds, because we started with actual features only (the first kind), but wrote the API assuming we only have the second kind.

---

But that may be more of a semantic distinction, so that we can say that each "feature" can have a set of dependent "rights", which is pretty much what @skateman proposes :+1:

[skateman]

Whyrights when we can have mandates? :trollface: But yeah I agree with the semantical distinguishing. :thumbsup:

[himdel]

An alternative would be to rewrite the API based on user actions as opposed to collections and instances.

That way, we could have a "show about modal (data)" action in the API, based on a "show about modal" rbac feature.

IMO that still makes a lot of sense when writing an API to back an existing UI, but that ship may have already sailed, and would be problematic re other REST API usecases.

[skateman]

Yup, I was trying to find the least-invasive solution that does minimal harm to the holy compatibility.

[martinpovolny]

An alternative would be to rewrite the API based on user actions as opposed to collections and instances.

That might have been an option in the past, but imo no longer because we are already pretty far the other way. Also it makes little sence if/when the API is used outside the UI (I cannot imagine the API/core guys agreeing with this ;-))

I like the suggestion. That is something we have been discussing for a while aready w/o the idea of where to actually put it.

From the few examples that we discussed so far this seems enough to me. Important part of this needs to be documentation. So that there are no surprises for people who set the permissions (and no BZs).

[lpichler]

I am good with this approach. I was facing couple BZs as well with this topic.

From the few examples that we discussed so far this seems enough to me. Important part of this needs to be documentation. So that there are no surprises for people who set the permissions (and no BZs).

Imho I would like to let user know about feature dependencies in UI to know for example: When you are allowing set ownership screen, you are also allowing them to see list of user, groups. It could be important information for them.

Of course I can take care of implementation :)

[skateman]

When you are allowing set ownership screen, you are also allowing them to see list of user, groups. It could be important information for them.

It's important that we'd be exposing these stuff only via the API and not through the UI.

[lpichler]

https://github.com/ManageIQ/manageiq/pull/18770

[skateman]

@lpichler the fix you're referring to cannot be applied universally, so we can't get away with not having some kind of solution described above...

[lpichler]

@lpichler the fix you're referring to cannot be applied universally, so we can't get away with not having some kind of solution described above...

oh yes I am sorry for mystification, of course I know, I am just trying to track related things - so maybe we should do it in some doc or something ?

[skateman]

@lpichler I added a new column in our big bad interactions spreadsheet

[Fryguy]

cc @agrare

[kbrock]

I will use @himdel nomenclature. features, rights

This reminds me of CanCan (and followup gem CanCanCan). ASIDE: the reaction to this code was pundit to simplify the large monolith of privilege code. But they seem very rights based.

CanCan uses is_admin? (feature) and other features to describe (mostly) CRUD rights on resources.

It would be really cool to have all code check rights and not privileges. It would be really cool to have the miq_features.yml map privileges to rights.

Is it possible to have the api check rights instead of privileges?

[martinpovolny]

@kbrock : would you be willing to to a short introduction into what you suggest, explain things around etc. to people interested? Surely myself, @skateman, @lpichler, @himdel , maybe other people.

So that we do more informed work moving forward. A BJ meeting on this?

[martinpovolny]

To move forward before we decide on some redesign:

In the API it is possible for each action to give e LIST of RBAC features. And if ANY of the list is enabled for the role, then the API action is available.

In your example:

        :identifier: vm_edit
        :dependent_api_features:
          - rbac_user_show_list
          - rbac_group_show_list

That would mean that in the API repo in config/api.yml where rbac_user_show_list or rbac_group_show_list are used we would add vm_edit.

You can see this already used in places. (Search for any :identifier: key that has an array value.).

Does this make sense to you, @skateman, @himdel ?

[martinpovolny]

@abellotti : please, chime in, if I am not being a good apprentice ;-)

[skateman]

Well, if I understand your proposal correctly, it is way too complicated and requires a lot of duplication. I agree that it's cheap, but think about the maintenance and the big risk of errors.

[martinpovolny]

Is it?

lucifer - [~/Projects/manageiq-api] (cloud_volumes_tags)$ grep rbac_user_show_list config/api.yml 
        :identifier: rbac_user_show_list
        :identifier: rbac_user_show_list
lucifer - [~/Projects/manageiq-api] (cloud_volumes_tags)$ grep rbac_group_show_list config/api.yml
        :identifier: rbac_group_show_list
        :identifier: rbac_group_show_list
        :identifier: rbac_group_show_list

Looks like 5 lines of config file at most to me vs 3 lines of config file in your case (+ the necessary code changes).

Am I missing something?

We can also keep adding those to a list or even in the miq_features.yml do it a better way once we agree on it and have the cycles for a more systematic solution.

[skateman]

Yeah, let's talk about it when you're back...

[himdel]

Sounds like the partial resolution from gitter April 6, 2020 2:09 PM is...

outside of form options, prefer to introduce new API entrypoints, gated by the relevant product feature

[skateman]

@gtanzillo we need a solution for this because it will kick us hard as we convert forms to API-driven DDF

[skateman]

This is how the screen-based feature setting looks like in the UI:

I would want to keep it as it is IMO super comfortable for admins as it follows the hierarchy of the menu from the top.

[kbrock]

OK, so talking this one through with @himdel a while ago. sorry I didn't already include our conclusions.

As we go through the controllers and convert them to rbac, we are finding that we don't exactly know all the relationships. So coming up with the mapping would be a slow labor intensive process.

It does sound like this is a good way to go, but just highlighting that the learning may be the biggest cost to this feature. It would be best to have this mechanism available sooner rather than later. So when we are going through the ui controllers to add the logic to the api, we would be adding the logic into the proper spot.

[NickLaMuro]

Idea suggested offline (not necessarily the final solution, though I might move this to manageiq-design or some other new issue elsewhere):

Implement this is MiqProductFeatures as something like sub_permissions that each feature allows, instead of the config/api.yml so that this isn't something that is a fully API concern, just something the API implements. Otherwise we just have all this permission logic living in the API when it really is a permission problem.

This is similar in implementation to what was suggested here https://github.com/ManageIQ/manageiq/issues/19110#issuecomment-542894644 but instead moves the location of it to the db/fixtures/miq_product_features.yml instead of config/api.yml. This makes it a core concern that can be applied anywhere (even outside of the API) instead of logic that only lives in the API.

Implementation Details

Rbac

For the first pass, this is probably going to be an extra option that we can pass to Rbac.filtered/Rbac.search that . As @kbrock has (exhaustively) mentioned, our Rbac implementation does more than permission, and has kind of morphed into general "MiqQueryInterface".

For now, we will still be just tacking on the to existing Rbac, but in the future, we might move to some sort of MiqQueryInterface and refactor pieces of Rbac out.

MiqProductFeature

Current though is that we will have "sub permissions", or something to that extent, possibly a "hidden" feature that is also available.

We do need to be concerned with performance when doing permission looks so this doesn't get too slow, because it will effect every action. This will most likely affect the design of this.

Models

"What attributes are exposed" ideally would be very similar across most models for the *_limited, but for the cases where exceptions need to be made, a constant on the model or method override will probably be how this is handled.

API

Hopefully very little will change, but the main pieces of concern for the API are:

• Initial permission check for a route (prior to Rbac)
• How permissions will now be passed to Rbac from the API
• How filtering attributes will be handled when a user only has a *_limited permission
  • Forbidden v.s. silently changing what columns are viewed

[Fryguy]

For MiqProductFeature there is already a concept of hidden, so these would just be new features...

For example,

      - :name: Edit
        :description: Edit a VM
        :feature_type: admin
        :identifier: vm_edit
# ...
    - :name: Edit
      :description: Edit a Containers Provider
      :feature_type: admin
      :identifier: ems_container_edit

would become

      - :name: Edit
        :description: Edit a VM
        :feature_type: admin
        :identifier: vm_edit
        :children:
        - :name: List (Limited)
           :description: Display Lists of Users (Limited)
           :feature_type: view
           :hidden: true
           :identifier: rbac_user_show_list_limited
#...
    - :name: Edit
      :description: Edit a Containers Provider
      :feature_type: admin
      :identifier: ems_container_edit
      :children:
      - :name: List (Limited)
         :description: Display Lists of Users (Limited)
         :feature_type: view
         :hidden: true
         :identifier: rbac_user_show_list_limited    

Then on the API side, that extra permission is just included in the api.yml

  :users:
    :description: Users
    :identifier:
      rbac_user # Side note, this feels wrong...I would think the identifier would be more granular here like `rbac_user_view`
      rbac_user_show_list_limited

From a seeding perspective we would have to take care not to create the row twice since it could be in the yaml more than once.

---

Note that if we want to avoid duplication in the yaml file, we could use anchor references (though that gets tricky with Arrays, so we may have to deal with that in seeding

:rbac_user_show_list_limited: &rbac_user_show_list_limited
  :name: List (Limited)
  :description: Display Lists of Users (true)
  :feature_type: view
  :hidden: true
  :identifier: rbac_user_show_list_limited
#...
      - :name: Edit
        :description: Edit a VM
        :feature_type: admin
        :identifier: vm_edit
        :children:
        - *rbac_user_show_list_limited
#...
    - :name: Edit
      :description: Edit a Containers Provider
      :feature_type: admin
      :identifier: ems_container_edit
      :children:
      - *rbac_user_show_list_limited

[NickLaMuro]

@Fryguy If we want to go with child permissions, I think there are a few problems that need to be solved:

• duplication of limited roles
• lookup of these nested limited permissions
• updating users when the sub permissions are added.

I think you have covered the duplication issue, but not so much the other two

With the lookup, if you make this a child permission, there is no way from a, say /api/user route, to know if there is a /api/containers child permission that gives a limited user route permissions.

If you have something like I was calling a sub_permission, or probably better called sub_permission_identifier on a product feature that references a "(Limited)" permission, then the only look up that needs to happen is to find all features that reference rbac_user_show_list_limited and see if the user has those. If this is just an enumerable field referencing these identifiers, or even a join table if we want to go so far, then the lookup should be able to be done in pure SQL. In contrast, I would think the child permissions format would not be as SQL friendly, and potentially require pulling back all the MiqProductFeature records and enumerating them until we find a matching permission (I think...)

The sub_permission_identifier route most assuredly has its drawbacks, and probably is not going to be an easy SQL query when it comes to implementation details, but I think it is less so than what you are proposing (unless I am missing something obvious here).

---

That said, I think both approaches will have a bit of issues when it comes to updating existing user permissions, and some form of seeding after an update of the permissions is probably going to be required (whether via a migration, or just a resync of the db/fixtures/miq_product_features.yml).

If we go the migration route for updates, I feel like the process of updating the UI will be creating a lot of small migrations as we slow convert screens, which might be less than ideal.

[Fryguy]

updating users when the sub permissions are added.

This is not necessary. If they are child features then the user gets them automatically when we add them to product features. That is, a record is not required in the miq_roles_features table. To demonstrate, currently an admin role only has 1 permission in the miq_roles_features table (everything). By having that features that automatically get all other features under it in the tree. By extension, if someone has, say, vm_edit, they only need vm_edit in the database, and will get all of the child features automatically.

The only time we really need data migrations are when a feature is renamed or deleted or moved within the tree to a new hierarchy.

[Fryguy]

With the lookup, if you make this a child permission, there is no way from a, say /api/user route, to know if there is a /api/containers child permission that gives a limited user route permissions.

/api/containers isn't a permission...it's just an endpoint. You know what to look up because you look for that specific feature by name. That is, in the api.yml for /api/user it would be described as:

  :users:
    :description: Users
    :identifier:
      rbac_user
      rbac_user_show_list_limited

which ultimately turns into a call to (roughly)

RBAC.role_allows?(:identifier => ["rbac_user", "rbac_user_show_list_limited"], :any => true, :user => foo)

Which in turn will call a bunch of methods on MiqProductFeature+MiqUserRole to see if the user has those particular identifiers.

[kbrock]

• [ ] Update product feature cache to support multiple parents (non-trivial)
• [ ] Verify feature tree for the ui still works.
• [ ] link screens to feature_limit dropdowns and add to feature seed.
• [ ] link api endpoints to also show feature_limit views in api.yml
• [ ] update something so fewer columns come back from the api (hoping most of this is over in rbac land)

[miq-bot]

This issue has been automatically marked as stale because it has not been updated for at least 3 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

[miq-bot]

This issue has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this issue if this issue is still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.