RBAC: Adding action set resolver (action set read path) #86801
Conversation
IevaVasiljeva
added
no-backport
| actionSets map[string][]string | ||
| log log.Logger | ||
| actionSetsToActions map[string][]string | ||
| actionsToActionSets map[string][]string |
There was a problem hiding this comment.
change this from actionsToActionSets since we are taking in a action and not multiple actions
There was a problem hiding this comment.
I feel like this is a matter of preference on whether to use plural or singular for map naming. I usually go for a plural (my thinking is - we are creating a mapping between actions and action sets, which will hold several entries, so plural makes sense). But I don't have a strong preference as long as we're consistent. So if you feel strongly about actionToActionSets instead of actionsToActionSets, we should also go with actionSetToActions instead of actionSetsToActions.
| if features.IsEnabled(context.Background(), featuremgmt.FlagAccessActionSets) { | ||
| actionSetService.StoreActionSet(options.Resource, permission, actions) | ||
| } |
There was a problem hiding this comment.
Note: putting this behind a feature toggle - there shouldn't be anything risky with registering an action set, but better safe than sorry.
|
❌ Failed to run Playwright plugin e2e tests. |
… due to the amount of test changes
IevaVasiljeva
requested review from
rwwiv,
JacobsonMT,
yuri-tceretian and
grobinson-grafana
and removed request for
a team
| @@ -48,6 +49,11 @@ func (a *AccessControl) Evaluate(ctx context.Context, user identity.Requester, e | |||
| return false, nil | |||
| } | |||
|
|
|||
| // TODO update this to use featuremgmt.FeatureToggles instead of checking the config | |||
There was a problem hiding this comment.
Will tackle this in a separate PR, as it will involve a lot of test changes, and I don't want to add noise to this PR.
| @@ -1,12 +1,17 @@ | |||
| package acimpl | |||
| package acimpl_test | |||
There was a problem hiding this comment.
renaming to avoid import cycles
| @@ -1,4 +1,4 @@ | |||
| package database | |||
| package database_test | |||
There was a problem hiding this comment.
renaming to avoid import cycles
What is this feature?
Adding action set resolver.
Why do we need this feature?
We are transitioning over to using action sets when storing resource permissions in a DB instead of storing each individual permission.
We added logic to store action sets in #86108. This PR adds logic to consider action sets when we check for users permissions. Eg, when checking if a user has the permission
folders:write, we checkfolders:writeas well asfolders:editandfolders:admin(the latter two being action sets).This is done by adding an action set resolver, that expands the evaluator to include action sets, so evaluator checking
folders:viewbecomes an evaluator checkingany(folders:view folders:edit folders:admin folders:read)(the scope remains unchanged).Who is this feature for?
Currently internal only (behind
accessActionSetsfeature toggle).Which issue(s) does this PR fix?:
Fixes https://github.com/grafana/identity-access-team/issues/662
Special notes for your reviewer:
OSS counter part: https://github.com/grafana/grafana-enterprise/pull/6574