From 5bf6d7dad8e977fe5df151ca2d5e4e2965311dda Mon Sep 17 00:00:00 2001 From: Vardan Torosyan Date: Tue, 27 Apr 2021 18:22:18 +0200 Subject: [PATCH] Access control: Update evaluator to authorize when at least one of the scopes is a match (#33393) --- .../accesscontrol/evaluator/evaluator.go | 28 ++++---- .../accesscontrol/evaluator/evaluator_test.go | 65 +++++++++++++++++++ pkg/services/accesscontrol/roles.go | 10 +-- pkg/services/accesscontrol/roles_test.go | 4 +- 4 files changed, 87 insertions(+), 20 deletions(-) create mode 100644 pkg/services/accesscontrol/evaluator/evaluator_test.go diff --git a/pkg/services/accesscontrol/evaluator/evaluator.go b/pkg/services/accesscontrol/evaluator/evaluator.go index 79f1ac2e50d..c250e9478d3 100644 --- a/pkg/services/accesscontrol/evaluator/evaluator.go +++ b/pkg/services/accesscontrol/evaluator/evaluator.go @@ -9,18 +9,24 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol" ) -// Evaluate evaluates access to the given resource, using provided AccessControl instance -func Evaluate(ctx context.Context, ac accesscontrol.AccessControl, user *models.SignedInUser, permission string, scope ...string) (bool, error) { - res, err := ac.GetUserPermissions(ctx, user) +// Evaluate evaluates access to the given resource, using provided AccessControl instance. +// Scopes are evaluated with an `OR` relationship. +func Evaluate(ctx context.Context, ac accesscontrol.AccessControl, user *models.SignedInUser, action string, scope ...string) (bool, error) { + userPermissions, err := ac.GetUserPermissions(ctx, user) if err != nil { return false, err } - ok, dbScopes := extractPermission(res, permission) + ok, dbScopes := extractScopes(userPermissions, action) if !ok { return false, nil } - for _, s := range scope { + + return evaluateScope(dbScopes, scope...) +} + +func evaluateScope(dbScopes map[string]struct{}, targetScopes ...string) (bool, error) { + for _, s := range targetScopes { var match bool for dbScope := range dbScopes { rule, err := glob.Compile(dbScope, ':', '/') @@ -30,19 +36,15 @@ func Evaluate(ctx context.Context, ac accesscontrol.AccessControl, user *models. match = rule.Match(s) if match { - break + return true, nil } } - - if !match { - return false, nil - } } - return true, nil + return false, nil } -func extractPermission(permissions []*accesscontrol.Permission, permission string) (bool, map[string]struct{}) { +func extractScopes(permissions []*accesscontrol.Permission, targetAction string) (bool, map[string]struct{}) { scopes := map[string]struct{}{} ok := false @@ -50,7 +52,7 @@ func extractPermission(permissions []*accesscontrol.Permission, permission strin if p == nil { continue } - if p.Action == permission { + if p.Action == targetAction { ok = true scopes[p.Scope] = struct{}{} } diff --git a/pkg/services/accesscontrol/evaluator/evaluator_test.go b/pkg/services/accesscontrol/evaluator/evaluator_test.go new file mode 100644 index 00000000000..dd417896b4a --- /dev/null +++ b/pkg/services/accesscontrol/evaluator/evaluator_test.go @@ -0,0 +1,65 @@ +package evaluator + +import ( + "testing" + + "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestExtractPermission(t *testing.T) { + const targetPermission = "permissions:create" + userPermissions := []*accesscontrol.Permission{ + { + Action: "permissions:create", + Scope: "teams:*/permissions:*", + }, + { + Action: "permissions:remove", + Scope: "permissions:*", + }, + } + expectedScopes := map[string]struct{}{ + "teams:*/permissions:*": {}, + } + ok, scopes := extractScopes(userPermissions, targetPermission) + assert.True(t, ok) + assert.Equal(t, expectedScopes, scopes) +} + +func TestEvaluatePermissions(t *testing.T) { + scopes := map[string]struct{}{ + "teams:*/permissions:*": {}, + "users:*": {}, + "permissions:delegate": {}, + } + + ok, err := evaluateScope(scopes, "teams:1/permissions:delegate") + require.NoError(t, err) + assert.True(t, ok) +} + +func TestEvaluatePermissions_WhenAtLeastOneScopeIsMatched_ReturnsTrue(t *testing.T) { + scopes := map[string]struct{}{ + "teams:*/permissions:*": {}, + "users:*": {}, + "permissions:delegate": {}, + } + + ok, err := evaluateScope(scopes, "global:admin", "permissions:delegate") + require.NoError(t, err) + assert.True(t, ok) +} + +func TestEvaluatePermissions_WhenNoMatchFound_ReturnsFalse(t *testing.T) { + scopes := map[string]struct{}{ + "teams:*/permissions:*": {}, + "users:*": {}, + "permissions:delegate": {}, + } + + ok, err := evaluateScope(scopes, "teams1/permissions:delegate") + require.NoError(t, err) + assert.False(t, ok) +} diff --git a/pkg/services/accesscontrol/roles.go b/pkg/services/accesscontrol/roles.go index 068506e72ae..f10c4cf141e 100644 --- a/pkg/services/accesscontrol/roles.go +++ b/pkg/services/accesscontrol/roles.go @@ -18,7 +18,7 @@ var ldapAdminReadRole = RoleDTO{ var ldapAdminEditRole = RoleDTO{ Name: ldapAdminEdit, Version: 1, - Permissions: concat(ldapAdminReadRole.Permissions, []Permission{ + Permissions: ConcatPermissions(ldapAdminReadRole.Permissions, []Permission{ { Action: ActionLDAPUsersSync, }, @@ -39,7 +39,7 @@ var orgsAdminReadRole = RoleDTO{ var orgsAdminEditRole = RoleDTO{ Name: orgsAdminEdit, Version: 1, - Permissions: concat(orgsAdminReadRole.Permissions, []Permission{ + Permissions: ConcatPermissions(orgsAdminReadRole.Permissions, []Permission{ { Action: ActionOrgUsersAdd, Scope: ScopeOrgAllUsersAll, @@ -69,7 +69,7 @@ var orgsCurrentReadRole = RoleDTO{ var orgsCurrentEditRole = RoleDTO{ Name: orgsCurrentEdit, Version: 1, - Permissions: concat(orgsCurrentReadRole.Permissions, []Permission{ + Permissions: ConcatPermissions(orgsCurrentReadRole.Permissions, []Permission{ { Action: ActionOrgUsersAdd, Scope: ScopeOrgCurrentUsersAll, @@ -111,7 +111,7 @@ var usersAdminReadRole = RoleDTO{ var usersAdminEditRole = RoleDTO{ Name: usersAdminEdit, Version: 1, - Permissions: concat(usersAdminReadRole.Permissions, []Permission{ + Permissions: ConcatPermissions(usersAdminReadRole.Permissions, []Permission{ { Action: ActionUsersPasswordUpdate, Scope: ScopeUsersAll, @@ -205,7 +205,7 @@ var PredefinedRoleGrants = map[string][]string{ }, } -func concat(permissions ...[]Permission) []Permission { +func ConcatPermissions(permissions ...[]Permission) []Permission { if permissions == nil { return nil } diff --git a/pkg/services/accesscontrol/roles_test.go b/pkg/services/accesscontrol/roles_test.go index 586cc0d9446..f10a951847e 100644 --- a/pkg/services/accesscontrol/roles_test.go +++ b/pkg/services/accesscontrol/roles_test.go @@ -34,7 +34,7 @@ func TestPredefinedRoleGrants(t *testing.T) { } } -func TestConcat(t *testing.T) { +func TestConcatPermissions(t *testing.T) { perms1 := []Permission{ { Action: "test", @@ -67,6 +67,6 @@ func TestConcat(t *testing.T) { }, } - perms := concat(perms1, perms2) + perms := ConcatPermissions(perms1, perms2) assert.ElementsMatch(t, perms, expected) }