PermissionRegistry: Error on unknown kind (#91469)

* PermissionRegistry: Error on unknown kind

* Account for PR feedback

Co-authored-by: Eric Leijonmarck <eric.leijonmarck@gmail.com>

* Add missing alerting scope

---------

Co-authored-by: Eric Leijonmarck <eric.leijonmarck@gmail.com>

pkg/services/accesscontrol/acimpl/service.go

@@ -406,7 +406,9 @@ func (s *Service) DeclareFixedRoles(registrations ...accesscontrol.RoleRegistrat
 		}
 
 		for i := range r.Role.Permissions {
-			s.permRegistry.RegisterPermission(r.Role.Permissions[i].Action, r.Role.Permissions[i].Scope)
+			if err := s.permRegistry.RegisterPermission(r.Role.Permissions[i].Action, r.Role.Permissions[i].Scope); err != nil {
+				return err
+			}
 		}
 
 		s.registrations.Append(r)
@@ -464,7 +466,9 @@ func (s *Service) DeclarePluginRoles(ctx context.Context, ID, name string, regs
 		for i := range r.Role.Permissions {
 			// Register plugin actions and their possible scopes for permission validation
 			s.permRegistry.RegisterPluginScope(r.Role.Permissions[i].Scope)
-			s.permRegistry.RegisterPermission(r.Role.Permissions[i].Action, r.Role.Permissions[i].Scope)
+			if err := s.permRegistry.RegisterPermission(r.Role.Permissions[i].Action, r.Role.Permissions[i].Scope); err != nil {
+				return err
+			}
 		}
 
 		s.log.Debug("Registering plugin role", "role", r.Role.Name)

pkg/services/accesscontrol/permreg/permreg.go

@@ -14,6 +14,8 @@ var (
 
 	ErrUnknownActionTplt = "unknown action: {{.Public.Action}}, was not found in the list of valid actions"
 	ErrBaseUnknownAction = errutil.BadRequest("permreg.unknown-action").MustTemplate(ErrUnknownActionTplt, errutil.WithPublic(ErrUnknownActionTplt))
+
+	ErrBaseUnknownKind = errutil.BadRequest("permreg.unknown-kind").MustTemplate("unknown kind: {{.Public.Kind}}")
 )
 
 func ErrInvalidScope(scope string, action string, validScopePrefixes PrefixSet) error {
@@ -28,6 +30,10 @@ func ErrUnknownAction(action string) error {
 	return ErrBaseUnknownAction.Build(errutil.TemplateData{Public: map[string]any{"Action": action}})
 }
 
+func ErrUnknownKind(kind string) error {
+	return ErrBaseUnknownKind.Build(errutil.TemplateData{Public: map[string]any{"Kind": kind}})
+}
+
 func generateValidScopeFormats(acceptedScopePrefixes PrefixSet) []string {
 	if len(acceptedScopePrefixes) == 0 {
 		return []string{}
@@ -48,7 +54,7 @@ func generateValidScopeFormats(acceptedScopePrefixes PrefixSet) []string {
 
 type PermissionRegistry interface {
 	RegisterPluginScope(scope string)
-	RegisterPermission(action, scope string)
+	RegisterPermission(action, scope string) error
 	IsPermissionValid(action, scope string) error
 	GetScopePrefixes(action string) (PrefixSet, bool)
 }
@@ -87,6 +93,7 @@ func newPermissionRegistry() *permissionRegistry {
 		"global.users":    "global.users:id:",
 		"roles":           "roles:uid:",
 		"services":        "services:",
+		"receivers":       "receivers:uid:",
 	}
 	return &permissionRegistry{
 		actionScopePrefixes: make(map[string]PrefixSet, 200),
@@ -101,42 +108,47 @@ func (pr *permissionRegistry) RegisterPluginScope(scope string) {
 	}
 
 	scopeParts := strings.Split(scope, ":")
+	kind := scopeParts[0]
+
 	// If the scope is already registered, return
-	if _, found := pr.kindScopePrefix[scopeParts[0]]; found {
+	if _, found := pr.kindScopePrefix[kind]; found {
 		return
 	}
 
 	// If the scope contains an attribute part, register the kind and attribute
 	if len(scopeParts) > 2 {
-		kind, attr := scopeParts[0], scopeParts[1]
+		attr := scopeParts[1]
 		pr.kindScopePrefix[kind] = kind + ":" + attr + ":"
 		pr.logger.Debug("registered scope prefix", "kind", kind, "scope_prefix", kind+":"+attr+":")
 		return
 	}
 
-	pr.logger.Debug("registered scope prefix", "kind", scopeParts[0], "scope_prefix", scopeParts[0]+":")
+	pr.logger.Debug("registered scope prefix", "kind", kind, "scope_prefix", kind+":")
-	pr.kindScopePrefix[scopeParts[0]] = scopeParts[0] + ":"
+	pr.kindScopePrefix[kind] = kind + ":"
 }
 
-func (pr *permissionRegistry) RegisterPermission(action, scope string) {
+func (pr *permissionRegistry) RegisterPermission(action, scope string) error {
-	if _, ok := pr.actionScopePrefixes[action]; !ok {
+	if _, ok := pr.actionScopePrefixes[action]; ok {
-		pr.actionScopePrefixes[action] = PrefixSet{}
+		// action already registered
+		return nil
 	}
+	pr.actionScopePrefixes[action] = PrefixSet{}
 
 	if scope == "" {
 		// scopeless action
-		return
+		return nil
 	}
 
 	kind := strings.Split(scope, ":")[0]
 	scopePrefix, ok := pr.kindScopePrefix[kind]
 	if !ok {
-		pr.logger.Warn("unknown scope prefix", "scope", scope)
+		pr.logger.Error("unknown kind: please update `kindScopePrefix` with the correct scope prefix", "kind", kind)
-		return
+		return ErrUnknownKind(kind)
 	}
 
 	// Add a new entry in case the scope is not empty
 	pr.actionScopePrefixes[action][scopePrefix] = true
+	return nil
 }
 
 func (pr *permissionRegistry) IsPermissionValid(action, scope string) error {

pkg/services/accesscontrol/permreg/permreg_test.go

@@ -51,7 +51,7 @@ func Test_permissionRegistry_RegisterPermission(t *testing.T) {
 		scope         string
 		wantKind      string
 		wantPrefixSet PrefixSet
-		wantSkip      bool
+		wantErr       bool
 	}{
 		{
 			name:          "register folders read",
@@ -67,16 +67,31 @@ func Test_permissionRegistry_RegisterPermission(t *testing.T) {
 			wantPrefixSet: PrefixSet{},
 		},
 		{
-			name:          "register an action on an unknown kind",
+			name:    "register an action on an unknown kind",
-			action:        "unknown:action",
+			action:  "unknown:action",
-			scope:         "unknown:uid:*",
+			scope:   "unknown:uid:*",
-			wantPrefixSet: PrefixSet{},
+			wantErr: true,
+		},
+		{
+			name:          "register an action that is already registered",
+			action:        "already:registered",
+			scope:         "already:uid:*",
+			wantPrefixSet: PrefixSet{"already:uid:": true},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			pr := newPermissionRegistry()
-			pr.RegisterPermission(tt.action, tt.scope)
+
+			// Pretend that an action is registered
+			pr.actionScopePrefixes["already:registered"] = PrefixSet{"already:uid:": true}
+
+			err := pr.RegisterPermission(tt.action, tt.scope)
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+
 			got, ok := pr.actionScopePrefixes[tt.action]
 			require.True(t, ok)
 			for k, v := range got {
@@ -88,8 +103,10 @@ func Test_permissionRegistry_RegisterPermission(t *testing.T) {
 
 func Test_permissionRegistry_IsPermissionValid(t *testing.T) {
 	pr := newPermissionRegistry()
-	pr.RegisterPermission("folders:read", "folders:uid:")
+	err := pr.RegisterPermission("folders:read", "folders:uid:")
-	pr.RegisterPermission("test-app.settings:read", "")
+	require.NoError(t, err)
+	err = pr.RegisterPermission("test-app.settings:read", "")
+	require.NoError(t, err)
 
 	tests := []struct {
 		name    string
@@ -166,8 +183,10 @@ func Test_permissionRegistry_IsPermissionValid(t *testing.T) {
 
 func Test_permissionRegistry_GetScopePrefixes(t *testing.T) {
 	pr := newPermissionRegistry()
-	pr.RegisterPermission("folders:read", "folders:uid:")
+	err := pr.RegisterPermission("folders:read", "folders:uid:")
-	pr.RegisterPermission("test-app.settings:read", "")
+	require.NoError(t, err)
+	err = pr.RegisterPermission("test-app.settings:read", "")
+	require.NoError(t, err)
 
 	tests := []struct {
 		name        string

pkg/services/accesscontrol/permreg/test/testreg.go

@@ -1,22 +1,38 @@
 package test
 
-import "github.com/grafana/grafana/pkg/services/accesscontrol/permreg"
+import (
+	"testing"
 
-func ProvidePermissionRegistry() permreg.PermissionRegistry {
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/grafana/pkg/services/accesscontrol/permreg"
+)
+
+func ProvidePermissionRegistry(t *testing.T) permreg.PermissionRegistry {
 	permReg := permreg.ProvidePermissionRegistry()
 	// Test core permissions
-	permReg.RegisterPermission("datasources:read", "datasources:uid:")
+	err := permReg.RegisterPermission("datasources:read", "datasources:uid:")
-	permReg.RegisterPermission("dashboards:read", "dashboards:uid:")
+	require.NoError(t, err)
-	permReg.RegisterPermission("dashboards:read", "folders:uid:")
+	err = permReg.RegisterPermission("dashboards:read", "dashboards:uid:")
-	permReg.RegisterPermission("folders:read", "folders:uid:")
+	require.NoError(t, err)
+	err = permReg.RegisterPermission("dashboards:read", "folders:uid:")
+	require.NoError(t, err)
+	err = permReg.RegisterPermission("folders:read", "folders:uid:")
+	require.NoError(t, err)
 	// Test plugins permissions
-	permReg.RegisterPermission("plugins.app:access", "plugins:id:")
+	err = permReg.RegisterPermission("plugins.app:access", "plugins:id:")
+	require.NoError(t, err)
 	// App
-	permReg.RegisterPermission("test-app:read", "")
+	err = permReg.RegisterPermission("test-app:read", "")
-	permReg.RegisterPermission("test-app.settings:read", "")
+	require.NoError(t, err)
-	permReg.RegisterPermission("test-app.projects:read", "")
+	err = permReg.RegisterPermission("test-app.settings:read", "")
+	require.NoError(t, err)
+	err = permReg.RegisterPermission("test-app.projects:read", "")
+	require.NoError(t, err)
 	// App 1
-	permReg.RegisterPermission("test-app1.catalog:read", "")
+	err = permReg.RegisterPermission("test-app1.catalog:read", "")
-	permReg.RegisterPermission("test-app1.announcements:read", "")
+	require.NoError(t, err)
+	err = permReg.RegisterPermission("test-app1.announcements:read", "")
+	require.NoError(t, err)
 	return permReg
 }