rancher / dashboard

The Rancher UI
https://rancher.com
Apache License 2.0
454 stars 257 forks source link

'View Yaml' action from list views fails for users who can list but not get a resource #8606

Open mantis-toboggan-md opened 1 year ago

mantis-toboggan-md commented 1 year ago

To reproduce:

  1. Create a custom cluster to role to list a resource
  2. Create a standard user
  3. Explore a cluster, navigate to 'Cluster & Project Members'
  4. Add the standard user to the cluster with the custom role created in step 1
  5. Login as the standard user and navigate to the relevant resource's list view
  6. Use the kebab menu to select 'view as yaml' for one of the resources listed Result: The yaml doesn't load and an error is seen in the console:
    console.js:31 TypeError: Cannot read properties of null (reading 'doneOverride')
    at f.<anonymous> (index.vue?46a7:1:1374)
    at t._render (vue.runtime.esm.js:3569:22)
    at f.r (vue.runtime.esm.js:4081:21)
    at Tn.get (vue.runtime.esm.js:4495:25)
    at Tn.run (vue.runtime.esm.js:4570:22)
    at Sn (vue.runtime.esm.js:4326:13)
    at Array.<anonymous> (vue.runtime.esm.js:1989:12)
    at ce (vue.runtime.esm.js:1915:12)

    Expected result: The user can view the yaml of the resource

It's worth noting that 'view config' still works in this scenario. This is probably because when we go from a list view to 'view config' we use data already loaded from the request to find all of that resource, but when we 'view yaml' we re-query the api for that specific resource in yaml form.

gaktive commented 1 year ago

Pushing to Q3.

cnotv commented 1 year ago

Example configuration with screenshots:

Note: Cluster Role and not Global Role

Screenshot 2023-07-03 at 14 36 44

Screenshot 2023-06-28 at 18 10 21

Screenshot 2023-06-28 at 18 17 00

Screenshot 2023-06-28 at 18 18 59

Screenshot 2023-06-28 at 18 19 29

cnotv commented 1 year ago

Another point which is worth mention, it's possible to access the configuration and switch to YAML, but it would not show anything in it.

Screenshot 2023-06-28 at 18 40 37 Screenshot 2023-06-28 at 18 40 48

cnotv commented 1 year ago

@mantis-toboggan-md I think we will need to remove the possibility to navigate in the configuration or retrieve the values in somehow. When I refresh the page I get this error: Screenshot 2023-06-30 at 14 23 08

cnotv commented 1 year ago

Based on our logic, the ability to link to configuration and YAML editor are based on the presence of links, which in this case are present and allow to view.

Screenshot 2023-07-03 at 16 38 32

Although when I try to open the defined URL that's forbidden 🤔

Steve Screenshot 2023-07-03 at 16 40 06

K8S Screenshot 2023-07-05 at 16 29 22

Something I can do as Admin

Steve Screenshot 2023-07-05 at 16 31 40

K8S Screenshot 2023-07-03 at 16 41 11

cnotv commented 1 year ago

As discussed and verified with @richard-cox and @axeal, the schema returns the GET as resourceMethod, although it returns 403 when trying to load the resource itself. In the example using a Pod.

image image

image

The resource is correctly listed:

image

richard-cox commented 1 year ago

We need someone from the auth team to confirm these two points

To resolve this specific issue the UI needs to confirm the user can get a resource. Without working this out via roles, we assume the best way is via the resources schema's resourceMethods GET value, or specifically via the resources link.self value (if the resource was fetched via a list request).

There's two bugs

We need someone from the auth team to confirm the two bugs

MbolotSuse commented 1 year ago

@richard-cox

Long story short, there's a steve issue here and I would recommend opening an issue in rancher/rancher so that we can address it. To respond to your specific questions:

Users who have permission to list a resource do NOT automatically get the get permission. This needs to be explicitly set alongside list

This is true, and extends to steve as well.

We support the use case where role template gives a resource the list permission without the get permission. Is this illogical or are there geniune use cases?

Near as I can tell, there are very limited use cases for this. The only thing that I could come up with is using resourceNames along with list - this will require users to use a fieldSelector in the list request (so in theory you could give users get access on a few resources and list access on a few resources, and this method would at least give those users some ability to use a list command rather than requiring a raw get every time). That being said, my personal opinion would be that we need to stick with upstream here, diverging from them and giving users "get" automatically when they have "list" could lead to bad issues.

We need someone from the auth team to confirm the two bugs

User should NOT be able to see the steve schema's resourceMethods GET value if the user does not have GET permissions

My answer here is subject to some conditions (specifically surrounding the schema's inability to filter by namespace/resources names as seen in rancher/rancher#34731 among others), but generally speaking you are correct here - resourceMethods should not include GET if the user can only 'list'. I'd recommend opening up an issue (as recommended above) so the backend can triage this.

The user should not see the links.self value if the user does not have GET permissions (resource fetched via LIST permission)

This I cannot confirm. Near as I can tell, the self value is included in any object. So if a user fetches the resources through list, the entries in that list will contain a self link. I would not recommend using this value as a source of truth on user's get permissions on an object, and I don't anticipate us making changes to this value to make it work that way.

In short, I would not consider this second behavior a bug.

gaktive commented 1 year ago

Pushing to Q4 since backend still needs to work on this. Not sure if https://github.com/rancher/rancher/issues/34731 or a new ticket is for UI tracking on the blocked status but I know both @richard-cox and @MbolotSuse are on this. If someone else has to work on this though, let's make it clear where the backend work is tracked.