RBAC: remove redundant role name field from plugin role registrations (#58166)

* RBAC: Remove name from role registration

* Inline accesscontrol service

* test fix

* use fmt

Co-Authored-By: marefr <marcus.efraimsson@gmail.com>

Co-authored-by: marefr <marcus.efraimsson@gmail.com>

pkg/coremodel/pluginmeta/coremodel.cue

@@ -158,7 +158,7 @@ seqs: [
 				// Example: the role 'Schedules Reader' bundles permissions to view all schedules of the plugin.
 				#Role: {
 					name: string,
-					displayName: string,
+					name: =~"^([A-Z][0-9A-Za-z ]+)$"
 					description: string,
 					permissions: [...#Permission]
 				}

pkg/coremodel/pluginmeta/pluginmeta_gen.go

@@ -537,7 +537,6 @@ type ReleaseState string
 // Equivalent Go types at stable import paths are provided in https://github.com/grafana/grok.
 type Role struct {
 	Description string `json:"description"`
-	DisplayName string `json:"displayName"`
 	Name        string `json:"name"`
 	Permissions []struct {
 		Action string  `json:"action"`
@@ -562,7 +561,6 @@ type RoleRegistration struct {
 	// RBAC role definition to bundle related RBAC permissions on the plugin.
 	Role struct {
 		Description string `json:"description"`
-		DisplayName string `json:"displayName"`
 		Name        string `json:"name"`
 		Permissions []struct {
 			Action string  `json:"action"`

pkg/plugins/manager/loader/loader_test.go

@@ -646,8 +646,7 @@ func TestLoader_Load_RBACReady(t *testing.T) {
 						Roles: []plugins.RoleRegistration{
 							{
 								Role: plugins.Role{
-									Name:        "plugins.app:test-app:reader",
-									DisplayName: "test-app reader",
+									Name:        "Reader",
 									Description: "View everything in the test-app plugin",
 									Permissions: []plugins.Permission{
 										{Action: "plugins.app:access", Scope: "plugins.app:id:test-app"},

pkg/plugins/manager/testdata/test-app-with-roles/MANIFEST.txt

@@ -12,20 +12,20 @@ Hash: SHA512
   ],
   "plugin": "test-app",
   "version": "1.0.0",
-  "time": 1666953431573,
+  "time": 1667484928676,
   "keyId": "7e4d0c6a708866e7",
   "files": {
-    "plugin.json": "8017d19868809409e54e70eab116366de263005aa70960d44a12dc4dc5582cee"
+    "plugin.json": "3348335ec100392b325f3eeb882a07c729e9cbf0f1ae331239f46840bb1a01eb"
   }
 }
 -----BEGIN PGP SIGNATURE-----
 Version: OpenPGP.js v4.10.10
 Comment: https://openpgpjs.org
 
-wrgEARMKAAYFAmNbsNcAIQkQfk0ManCIZucWIQTzOyW2kQdOhGNlcPN+TQxq
-cIhm5z2+AgYqtKZ4tU/VBo8kOI49LfV85JKunAxPOvfaU3pRseRnWSyRBS0X
-pKI2ekKebOSRZIs+zDPA0qTl1ihOY9bKe52pwwIJAf1IDq1P7G861dFilTuF
-jCHQq6aS3NGy5o1N480Xof8PZdrI/xYDqSoy2F+688FR76ShyAM4B00Skt7c
-9YSCsLx+
-=cVti
+wrgEARMKAAYFAmNjzQAAIQkQfk0ManCIZucWIQTzOyW2kQdOhGNlcPN+TQxq
+cIhm509bAgiY3ZHrA6i95x6vef1z2cS6Q6+zzeLrfZ31AFtxq2Y/OYIQKBZC
+BZIp9LufCLCEDnwp+ocMGtDQV7yk1vUKM/zz/QIJAYs8d8pVnao31eqUB5Hy
+8WdkLFYa3V6rx1Da3iM24A5JgJwpTgudVYRQFRH6XR/HZt/EBRckAeQPxsN6
+qodkjllo
+=TMGo
 -----END PGP SIGNATURE-----

pkg/plugins/manager/testdata/test-app-with-roles/plugin.json

@@ -17,8 +17,7 @@
   "roles": [
     {
       "role": {
-        "name": "plugins.app:test-app:reader",
-        "displayName": "test-app reader",
+        "name": "Reader",
         "description": "View everything in the test-app plugin",
         "permissions": [
           {

pkg/plugins/models.go

@@ -279,7 +279,6 @@ type RoleRegistration struct {
 // Role is the model for Role in RBAC.
 type Role struct {
 	Name        string       `json:"name"`
-	DisplayName string       `json:"displayName"`
 	Description string       `json:"description"`
 	Permissions []Permission `json:"permissions"`
 }

pkg/services/accesscontrol/acimpl/service.go

@@ -220,7 +220,7 @@ func (s *Service) DeclarePluginRoles(_ context.Context, ID, name string, regs []
 		return nil
 	}
 
-	acRegs := pluginutils.ToRegistrations(name, regs)
+	acRegs := pluginutils.ToRegistrations(ID, name, regs)
 	for _, r := range acRegs {
 		if err := pluginutils.ValidatePluginRole(ID, r.Role); err != nil {
 			return err

pkg/services/accesscontrol/acimpl/service_test.go

@@ -175,31 +175,19 @@ func TestService_DeclarePluginRoles(t *testing.T) {
 			pluginID: "test-app",
 			registrations: []plugins.RoleRegistration{
 				{
-					Role:   plugins.Role{Name: "plugins:test-app:test"},
+					Role:   plugins.Role{Name: "Tester"},
 					Grants: []string{"Admin"},
 				},
 			},
 			wantErr: false,
 		},
-		{
-			name:     "should fail registration invalid role name",
-			pluginID: "test-app",
-			registrations: []plugins.RoleRegistration{
-				{
-					Role:   plugins.Role{Name: "invalid.plugins:test-app:test"},
-					Grants: []string{"Admin"},
-				},
-			},
-			wantErr: true,
-			err:     &accesscontrol.ErrorInvalidRole{},
-		},
 		{
 			name:     "should add registration with valid permissions",
 			pluginID: "test-app",
 			registrations: []plugins.RoleRegistration{
 				{
 					Role: plugins.Role{
-						Name: "plugins:test-app:test",
+						Name: "Tester",
 						Permissions: []plugins.Permission{
 							{Action: "plugins.app:access"},
 							{Action: "test-app:read"},
@@ -217,7 +205,7 @@ func TestService_DeclarePluginRoles(t *testing.T) {
 			registrations: []plugins.RoleRegistration{
 				{
 					Role: plugins.Role{
-						Name: "plugins:test-app:test",
+						Name: "Tester",
 						Permissions: []plugins.Permission{
 							{Action: "invalid.test-app.resource:read"},
 						},
@@ -233,7 +221,7 @@ func TestService_DeclarePluginRoles(t *testing.T) {
 			pluginID: "test-app",
 			registrations: []plugins.RoleRegistration{
 				{
-					Role:   plugins.Role{Name: "plugins:test-app:test"},
+					Role:   plugins.Role{Name: "Tester"},
 					Grants: []string{"WrongAdmin"},
 				},
 			},
@@ -245,11 +233,11 @@ func TestService_DeclarePluginRoles(t *testing.T) {
 			pluginID: "test-app",
 			registrations: []plugins.RoleRegistration{
 				{
-					Role:   plugins.Role{Name: "plugins:test-app:test"},
+					Role:   plugins.Role{Name: "Tester"},
 					Grants: []string{"Admin"},
 				},
 				{
-					Role:   plugins.Role{Name: "plugins:test-app:test2"},
+					Role:   plugins.Role{Name: "Tester2"},
 					Grants: []string{"Admin"},
 				},
 			},
@@ -335,7 +323,8 @@ func TestService_RegisterFixedRoles(t *testing.T) {
 			registrations: []accesscontrol.RoleRegistration{
 				{
 					Role: accesscontrol.RoleDTO{
-						Name:        "plugins:test-app:test",
+						Name:        accesscontrol.PluginRolePrefix + "test-app:tester",
+						DisplayName: "Tester",
 						Permissions: []accesscontrol.Permission{{Action: "test-app:test"}},
 					},
 					Grants: []string{"Editor"},

pkg/services/accesscontrol/pluginutils/utils.go

@@ -1,6 +1,7 @@
 package pluginutils
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/grafana/grafana/pkg/plugins"
@@ -34,14 +35,14 @@ func ValidatePluginRole(pluginID string, role ac.RoleDTO) error {
 	return ValidatePluginPermissions(pluginID, role.Permissions)
 }
 
-func ToRegistrations(pluginName string, regs []plugins.RoleRegistration) []ac.RoleRegistration {
+func ToRegistrations(pluginID, pluginName string, regs []plugins.RoleRegistration) []ac.RoleRegistration {
 	res := make([]ac.RoleRegistration, 0, len(regs))
 	for i := range regs {
 		res = append(res, ac.RoleRegistration{
 			Role: ac.RoleDTO{
 				Version:     1,
-				Name:        regs[i].Role.Name,
-				DisplayName: regs[i].Role.DisplayName,
+				Name:        roleName(pluginID, regs[i].Role.Name),
+				DisplayName: regs[i].Role.Name,
 				Description: regs[i].Role.Description,
 				Group:       pluginName,
 				Permissions: toPermissions(regs[i].Role.Permissions),
@@ -53,6 +54,10 @@ func ToRegistrations(pluginName string, regs []plugins.RoleRegistration) []ac.Ro
 	return res
 }
 
+func roleName(pluginID, roleName string) string {
+	return fmt.Sprintf("%v%v:%v", ac.PluginRolePrefix, pluginID, strings.Replace(strings.ToLower(roleName), " ", "-", -1))
+}
+
 func toPermissions(perms []plugins.Permission) []ac.Permission {
 	res := make([]ac.Permission, 0, len(perms))
 	for i := range perms {

pkg/services/accesscontrol/pluginutils/utils_test.go

@@ -24,8 +24,7 @@ func TestToRegistrations(t *testing.T) {
 			regs: []plugins.RoleRegistration{
 				{
 					Role: plugins.Role{
-						Name:        "test:name",
-						DisplayName: "Test",
+						Name:        "Tester",
 						Description: "Test",
 						Permissions: []plugins.Permission{
 							{Action: "test:action"},
@@ -36,7 +35,7 @@ func TestToRegistrations(t *testing.T) {
 				},
 				{
 					Role: plugins.Role{
-						Name:        "test:name",
+						Name:        "Admin Validator",
 						Permissions: []plugins.Permission{},
 					},
 				},
@@ -45,10 +44,10 @@ func TestToRegistrations(t *testing.T) {
 				{
 					Role: ac.RoleDTO{
 						Version:     1,
-						Name:        "test:name",
-						DisplayName: "Test",
+						Name:        ac.PluginRolePrefix + "plugin-id:tester",
+						DisplayName: "Tester",
 						Description: "Test",
-						Group:       "PluginName",
+						Group:       "Plugin Name",
 						Permissions: []ac.Permission{
 							{Action: "test:action"},
 							{Action: "test:action", Scope: "test:scope"},
@@ -60,8 +59,9 @@ func TestToRegistrations(t *testing.T) {
 				{
 					Role: ac.RoleDTO{
 						Version:     1,
-						Name:        "test:name",
-						Group:       "PluginName",
+						Name:        ac.PluginRolePrefix + "plugin-id:admin-validator",
+						DisplayName: "Admin Validator",
+						Group:       "Plugin Name",
 						Permissions: []ac.Permission{},
 						OrgID:       ac.GlobalOrgID,
 					},
@@ -71,7 +71,7 @@ func TestToRegistrations(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := ToRegistrations("PluginName", tt.regs)
+			got := ToRegistrations("plugin-id", "Plugin Name", tt.regs)
 			require.Equal(t, tt.want, got)
 		})
 	}