RBAC: Expand action sets when fetching permissions #87967
Conversation
… are fetched from the DB
IevaVasiljeva
added
no-backport
IevaVasiljeva
changed the title
There was a problem hiding this comment.
This change is slightly out of scope of this PR, so I wanted to highlight it. We didn't use to deduplicate the scopes, which became more obvious with action sets if we're double-writing (so we'd have, eg, dashboards:read permission from the action set as well as from the fine-grained permissions written to the DB). So I've added some deduplication logic here.
There was a problem hiding this comment.
could we add more comments to the function call to indicate that we are deduping as well and for what purpose.
much like your comment here
| Resolve(action string) []string | ||
| ResolveAction(action string) []string | ||
| ResolveActionSet(actionSet string) []string | ||
| ExpandActions(permissions []Permission) []Permission |
There was a problem hiding this comment.
so we get permissions and get permissions out?
it's just that naming is a bit off, I completely understand why. Do you think we could change the interface to use
ExpandActions(actionSets []string) []PermissionThere was a problem hiding this comment.
There's no way to transition from actionSets []string to []Permission. Permissions have additional information about scopes, roles etc, while action set list only has info about actions. I could rename the method if you have a better name candidate?
There was a problem hiding this comment.
yea, apologize. i should come up with a solution as well.
perhaps we could use
ExpandActionSetsToActionsExpandActionSetsThere was a problem hiding this comment.
Ok, I'll go with ExpandActionSets 👍
There was a problem hiding this comment.
nit: we can always change this back if it's just about naming convention. I'd prefer to be a bit more descriptive to ExpandingActionSetsToActions , to indicate what is happening
Maybe even using something like this (toooo long, but to get the description of what is happening):
ExtendPermissionsWithActionSetsToActions
| @@ -756,7 +757,7 @@ type InMemoryActionSets struct { | |||
| } | |||
|
|
|||
| // NewActionSetService returns a new instance of InMemoryActionSetService. | |||
| func NewActionSetService(a *acimpl.AccessControl) ActionSetService { | |||
| func NewActionSetService(a *acimpl.AccessControl) *InMemoryActionSets { | |||
There was a problem hiding this comment.
| func NewActionSetService(a *acimpl.AccessControl) *InMemoryActionSets { | |
| func NewActionSetService(a *acimpl.AccessControl) *InMemoryActionSets { |
what is the difference here when we return the pointer to a struct?
I think we changed this previously to a interface, if i am not mistaken
There was a problem hiding this comment.
Yes, there's a slight difference with what wire supports, and I couldn't find an easy way around it. I'm planning to change this back once this PR is merged and I can do a follow up PR to clean up the action resolvers, as we won't need those anymore. So this is just a temporary change. Sorry for making it briefly messy 🙈
There was a problem hiding this comment.
nada! just wanted to understand what this change was about and if i was missing something.
What is this feature?
When user permissions are fetched, always expand action sets into the underlying actions.
Also adding logic to deduplicate scopes when we group permissions: https://github.com/grafana/grafana/pull/87967/files#r1603476920
Why do we need this feature?
This way we don't need to make changes to any permission checks that don't directly query the DB (see the notes below on what else will need to be covered). This is safer, as we have permission checks in many places, and it is easy to miss adding an action resolver in some of them.
This means that we can undo changes introduced in #86801.
Who is this feature for?
Internal only
Which issue(s) does this PR fix?:
Fixes https://github.com/grafana/identity-access-team/issues/615
Special notes for your reviewer:
There is more work to take action sets into account for permission filters and for permission search. These will be covered in separate PRs.
Enterprise PR: https://github.com/grafana/grafana-enterprise/pull/6647