feat: improve RBAC preconditions for Insights endpoint #8794
Conversation
| if len(arg.TemplateIDs) == 0 { | ||
| if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
This doesn't align with our documented permissions here.
Is the reason for making the required permission "update" to ensure that only template owners can view template insights? If so, we should make a separate TemplateInsights resource and make appropriate modifications to our built-in roles.
Otherwise, why not just allow anyone with read permissions to view this data?
There was a problem hiding this comment.
Is the reason for making the required permission "update" to ensure that only template owners can view template insights?
Otherwise, why not just allow anyone with read permissions to view this data?
I suppose that we don't want to share user details for template usage and latency with regular users.
This doesn't align with our documented permissions here.
The Roles table does not describe the use case when a regular user can be granted admin access for a single template using Template ACLs (enterprise feature). This is why I picked the for-loop with "update template" check to see if the user can modify a template, ergo can access insights.
Please let me know what is your recommendation in this case.
There was a problem hiding this comment.
For the moment, this is OK as a bandage to stop the FE spinner issue. However, the current situation with regard to ACLs is not ideal in terms of integration with the rest of the RBAC package.
Fixes: #8763
This PR modifies RBAC rules for Insights API, so that regular users with appropriate template ACL rights and template admins can see insights for templates they own. Current permission is too narrow, it is limited only to owners, and it prevents UI from loading properly.
Note:
This PR seems to be relatively large, but the vast majority of it are RBAC unit tests.