Access Control: Make the evaluator prefix match only (#38025)

* Make the evaluator prefix match only

* Handle empty scopes

* Bump version of settings read role

Co-authored-by: Karl Persson <kalle.persson@grafana.com>

pkg/services/accesscontrol/accesscontrol.go

@@ -2,6 +2,7 @@ package accesscontrol
 
 import (
 	"context"
+	"strings"
 
 	"github.com/grafana/grafana/pkg/models"
 )
@@ -53,3 +54,15 @@ func BuildPermissionsMap(permissions []*Permission) map[string]bool {
 
 	return permissionsMap
 }
+
+func ValidateScope(scope string) bool {
+	prefix, last := scope[:len(scope)-1], scope[len(scope)-1]
+	// verify that last char is either ':' or '/' if last character of scope is '*'
+	if len(prefix) > 0 && last == '*' {
+		lastChar := prefix[len(prefix)-1]
+		if lastChar != ':' && lastChar != '/' {
+			return false
+		}
+	}
+	return !strings.ContainsAny(prefix, "*?")
+}

pkg/services/accesscontrol/evaluator/evaluator.go

@@ -2,15 +2,18 @@ package evaluator
 
 import (
 	"context"
+	"fmt"
+	"strings"
 
-	"github.com/gobwas/glob"
-
+	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/metrics"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/prometheus/client_golang/prometheus"
 )
 
+var logger = log.New("accesscontrol.evaluator")
+
 // 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) {
@@ -37,20 +40,46 @@ func evaluateScope(dbScopes map[string]struct{}, targetScopes ...string) (bool,
 	}
 
 	for _, s := range targetScopes {
-		var match bool
 		for dbScope := range dbScopes {
-			rule, err := glob.Compile(dbScope, ':', '/')
-			if err != nil {
-				return false, err
+			if dbScope == "" {
+				continue
 			}
 
-			match = rule.Match(s)
-			if match {
+			if !accesscontrol.ValidateScope(dbScope) {
+				logger.Error(
+					"invalid scope",
+					"reason", fmt.Sprintf("%v should not contain meta-characters like * or ?, except in the last position", dbScope),
+					"scope", dbScope,
+				)
+				continue
+			}
+
+			prefix, last := dbScope[:len(dbScope)-1], dbScope[len(dbScope)-1]
+			//Prefix match
+			if last == '*' {
+				if strings.HasPrefix(s, prefix) {
+					logger.Debug(
+						"matched scope",
+						"reason", fmt.Sprintf("matched request scope %v against resource scope %v", dbScope, s),
+						"request scope", dbScope,
+						"resource scope", s,
+					)
+					return true, nil
+				}
+			}
+
+			if s == dbScope {
 				return true, nil
 			}
 		}
 	}
 
+	logger.Debug(
+		"access control failed",
+		"request scope", dbScopes,
+		"resource scope", targetScopes,
+		"reason", fmt.Sprintf("Could not match resource scopes  %v with request scopes %v", dbScopes, targetScopes),
+	)
 	return false, nil
 }
 

pkg/services/accesscontrol/evaluator/evaluator_test.go

@@ -14,7 +14,11 @@ func TestExtractPermission(t *testing.T) {
 	userPermissions := []*accesscontrol.Permission{
 		{
 			Action: "permissions:create",
-			Scope:  "teams:*/permissions:*",
+			Scope:  "teams:*",
+		},
+		{
+			Action: "permissions:create",
+			Scope:  "permissions:*",
 		},
 		{
 			Action: "permissions:remove",
@@ -22,7 +26,8 @@ func TestExtractPermission(t *testing.T) {
 		},
 	}
 	expectedScopes := map[string]struct{}{
-		"teams:*/permissions:*": {},
+		"permissions:*": {},
+		"teams:*":       {},
 	}
 	ok, scopes := extractScopes(userPermissions, targetPermission)
 	assert.True(t, ok)
@@ -45,9 +50,10 @@ func TestEvaluatePermissions(t *testing.T) {
 		{
 			Name: "No expected scope always returns true",
 			HasScopes: map[string]struct{}{
-				"teams:*/permissions:*": {},
-				"users:*":               {},
-				"permissions:delegate":  {},
+				"teams:*":              {},
+				"permissions:*":        {},
+				"users:*":              {},
+				"permissions:delegate": {},
 			},
 			NeedAnyScope: []string{},
 			Valid:        true,
@@ -55,27 +61,30 @@ func TestEvaluatePermissions(t *testing.T) {
 		{
 			Name: "Single scope from  list",
 			HasScopes: map[string]struct{}{
-				"teams:1/permissions:delegate": {},
+				"teams:1":              {},
+				"permissions:delegate": {},
 			},
-			NeedAnyScope: []string{"teams:1/permissions:delegate"},
+			NeedAnyScope: []string{"teams:1", "permissions:delegate"},
 			Valid:        true,
 		},
 		{
 			Name: "Single scope from glob list",
 			HasScopes: map[string]struct{}{
-				"teams:*/permissions:*": {},
-				"users:*":               {},
-				"permissions:delegate":  {},
+				"teams:*":              {},
+				"permissions:*":        {},
+				"users:*":              {},
+				"permissions:delegate": {},
 			},
-			NeedAnyScope: []string{"teams:1/permissions:delegate"},
+			NeedAnyScope: []string{"teams:1", "permissions:delegate"},
 			Valid:        true,
 		},
 		{
 			Name: "Either of two scopes from glob list",
 			HasScopes: map[string]struct{}{
-				"teams:*/permissions:*": {},
-				"users:*":               {},
-				"permissions:delegate":  {},
+				"teams:*":              {},
+				"permissions:*":        {},
+				"users:*":              {},
+				"permissions:delegate": {},
 			},
 			NeedAnyScope: []string{"global:admin", "permissions:delegate"},
 			Valid:        true,
@@ -83,11 +92,11 @@ func TestEvaluatePermissions(t *testing.T) {
 		{
 			Name: "No match found",
 			HasScopes: map[string]struct{}{
-				"teams:*/permissions:*": {},
-				"users:*":               {},
-				"permissions:delegate":  {},
+				"teams:*":              {},
+				"users:*":              {},
+				"permissions:delegate": {},
 			},
-			NeedAnyScope: []string{"teams1/permissions:delegate"},
+			NeedAnyScope: []string{"teams1", "permissions:nodelegate"},
 			Valid:        false,
 		},
 	}

pkg/services/accesscontrol/models.go

@@ -102,7 +102,7 @@ const (
 	ScopeUsersAll = "users:*"
 
 	// Settings scope
-	ScopeSettingsAll = "settings:**"
+	ScopeSettingsAll = "settings:*"
 )
 
 const RoleGrafanaAdmin = "Grafana Admin"

pkg/services/accesscontrol/roles.go

@@ -57,7 +57,7 @@ var (
 	}
 
 	settingsAdminReadRole = RoleDTO{
-		Version: 1,
+		Version: 2,
 		Name:    settingsAdminRead,
 		Permissions: []Permission{
 			{