From d740f9fc60de6a6bfa8fea60cb050e9739e14092 Mon Sep 17 00:00:00 2001 From: Karl Persson <23356117+kalleep@users.noreply.github.com> Date: Thu, 23 Jan 2025 09:23:00 +0100 Subject: [PATCH] Authz: Simplify mapper and only check folders if its supported (#99357) * Simplify mapper and only check folders if its supported --- pkg/services/authz/mappers/rbac_mapper.go | 72 -------------------- pkg/services/authz/rbac/mapper.go | 81 +++++++++++++++++++++++ pkg/services/authz/rbac/service.go | 78 ++++++++++++++-------- pkg/services/authz/rbac/service_test.go | 19 +++--- 4 files changed, 142 insertions(+), 108 deletions(-) delete mode 100644 pkg/services/authz/mappers/rbac_mapper.go create mode 100644 pkg/services/authz/rbac/mapper.go diff --git a/pkg/services/authz/mappers/rbac_mapper.go b/pkg/services/authz/mappers/rbac_mapper.go deleted file mode 100644 index 86b97b95ef0..00000000000 --- a/pkg/services/authz/mappers/rbac_mapper.go +++ /dev/null @@ -1,72 +0,0 @@ -package mappers - -import ( - "fmt" - - "github.com/grafana/grafana/pkg/apimachinery/utils" -) - -const defaultAttribute = "uid" - -type VerbMapping map[string]string // e.g. "get" -> "read" -type ResourceVerbMapping map[string]VerbMapping // e.g. "dashboards" -> VerbToAction -type GroupResourceVerbMapping map[string]ResourceVerbMapping // e.g. "dashboard.grafana.app" -> ResourceVerbToAction - -type ResourceAttributeMapping map[string]string // e.g. "dashboards" -> "uid" -type GroupResourceAttributeMapping map[string]ResourceAttributeMapping // e.g. "dashboard.grafana.app" -> ResourceToAttribute - -type K8sRbacMapper struct { - GroupResourceVerbMapping GroupResourceVerbMapping - GroupResourceAttributeMapping GroupResourceAttributeMapping -} - -func NewK8sRbacMapper() *K8sRbacMapper { - defaultMapping := func(r string) VerbMapping { - return map[string]string{ - utils.VerbGet: fmt.Sprintf("%s:read", r), - utils.VerbList: fmt.Sprintf("%s:read", r), - utils.VerbWatch: fmt.Sprintf("%s:read", r), - utils.VerbCreate: fmt.Sprintf("%s:create", r), - utils.VerbUpdate: fmt.Sprintf("%s:write", r), - utils.VerbPatch: fmt.Sprintf("%s:write", r), - utils.VerbDelete: fmt.Sprintf("%s:delete", r), - utils.VerbDeleteCollection: fmt.Sprintf("%s:delete", r), - utils.VerbGetPermissions: fmt.Sprintf("%s.permissions:read", r), - utils.VerbSetPermissions: fmt.Sprintf("%s.permissions:write", r), - } - } - - return &K8sRbacMapper{ - GroupResourceAttributeMapping: GroupResourceAttributeMapping{}, - GroupResourceVerbMapping: GroupResourceVerbMapping{ - "dashboard.grafana.app": ResourceVerbMapping{"dashboards": defaultMapping("dashboards")}, - "folder.grafana.app": ResourceVerbMapping{"folders": defaultMapping("folders")}, - }, - } -} - -func (m *K8sRbacMapper) Action(group, resource, verb string) (string, bool) { - if resourceActions, ok := m.GroupResourceVerbMapping[group]; ok { - if actions, ok := resourceActions[resource]; ok { - if action, ok := actions[verb]; ok { - // If the action is explicitly set empty - // it means that the action is not allowed - if action == "" { - return "", false - } - return action, true - } - } - } - return "", false -} - -func (m *K8sRbacMapper) Scope(group, resource, name string) (string, bool) { - if resourceAttributes, ok := m.GroupResourceAttributeMapping[group]; ok { - if attribute, ok := resourceAttributes[resource]; ok { - return resource + ":" + attribute + ":" + name, true - } - } - - return resource + ":" + defaultAttribute + ":" + name, true -} diff --git a/pkg/services/authz/rbac/mapper.go b/pkg/services/authz/rbac/mapper.go new file mode 100644 index 00000000000..7635a193f7f --- /dev/null +++ b/pkg/services/authz/rbac/mapper.go @@ -0,0 +1,81 @@ +package rbac + +import ( + "fmt" + + "github.com/grafana/grafana/pkg/apimachinery/utils" +) + +type translation struct { + resource string + attribute string + verbMapping map[string]string + folderSupport bool +} + +func (t translation) action(verb string) (string, bool) { + action, ok := t.verbMapping[verb] + return action, ok +} + +func (t translation) scope(name string) string { + return t.resource + ":" + t.attribute + ":" + name +} + +func (t translation) prefix() string { + return t.resource + ":" + t.attribute + ":" +} + +func newResourceTranslation(resource string, attribute string, folderSupport bool) translation { + defaultMapping := func(r string) map[string]string { + return map[string]string{ + utils.VerbGet: fmt.Sprintf("%s:read", r), + utils.VerbList: fmt.Sprintf("%s:read", r), + utils.VerbWatch: fmt.Sprintf("%s:read", r), + utils.VerbCreate: fmt.Sprintf("%s:create", r), + utils.VerbUpdate: fmt.Sprintf("%s:write", r), + utils.VerbPatch: fmt.Sprintf("%s:write", r), + utils.VerbDelete: fmt.Sprintf("%s:delete", r), + utils.VerbDeleteCollection: fmt.Sprintf("%s:delete", r), + utils.VerbGetPermissions: fmt.Sprintf("%s.permissions:read", r), + utils.VerbSetPermissions: fmt.Sprintf("%s.permissions:write", r), + } + } + + return translation{ + resource: resource, + attribute: attribute, + verbMapping: defaultMapping(resource), + folderSupport: folderSupport, + } +} + +type mapper map[string]map[string]translation + +func newMapper() mapper { + return map[string]map[string]translation{ + "dashboard.grafana.app": { + "dashboards": newResourceTranslation("dashboards", "uid", true), + }, + "folder.grafana.app": { + "folders": newResourceTranslation("folders", "uid", true), + }, + "iam.grafana.app": { + "teams": newResourceTranslation("teams", "id", false), + }, + } +} + +func (m mapper) translation(group, resource string) (translation, bool) { + resources, ok := m[group] + if !ok { + return translation{}, false + } + + t, ok := resources[resource] + if !ok { + return translation{}, false + } + + return t, true +} diff --git a/pkg/services/authz/rbac/service.go b/pkg/services/authz/rbac/service.go index e600d43426a..b6a9bd9b863 100644 --- a/pkg/services/authz/rbac/service.go +++ b/pkg/services/authz/rbac/service.go @@ -23,7 +23,6 @@ import ( "github.com/grafana/grafana/pkg/registry/apis/iam/common" "github.com/grafana/grafana/pkg/registry/apis/iam/legacy" "github.com/grafana/grafana/pkg/services/accesscontrol" - "github.com/grafana/grafana/pkg/services/authz/mappers" authzextv1 "github.com/grafana/grafana/pkg/services/authz/proto/v1" "github.com/grafana/grafana/pkg/services/authz/rbac/store" "github.com/grafana/grafana/pkg/services/dashboards" @@ -44,7 +43,8 @@ type Service struct { store store.Store permissionStore store.PermissionStore identityStore legacy.LegacyIdentityStore - actionMapper *mappers.K8sRbacMapper + + mapper mapper logger log.Logger tracer tracing.Tracer @@ -72,9 +72,9 @@ func NewService( store: store.NewStore(sql, tracer), permissionStore: permissionStore, identityStore: identityStore, - actionMapper: mappers.NewK8sRbacMapper(), logger: logger, tracer: tracer, + mapper: newMapper(), idCache: localcache.New(longCacheTTL, longCleanupInterval), permCache: localcache.New(shortCacheTTL, shortCleanupInterval), teamCache: localcache.New(shortCacheTTL, shortCleanupInterval), @@ -236,14 +236,19 @@ func (s *Service) validateSubject(ctx context.Context, subject string) (string, func (s *Service) validateAction(ctx context.Context, group, resource, verb string) (string, error) { ctxLogger := s.logger.FromContext(ctx) - if group == "" || resource == "" || verb == "" { - return "", status.Error(codes.InvalidArgument, "group, resource and verb are required") - } - action, ok := s.actionMapper.Action(group, resource, verb) + + t, ok := s.mapper.translation(group, resource) if !ok { - ctxLogger.Error("could not find associated rbac action", "group", group, "resource", resource, "verb", verb) - return "", status.Error(codes.NotFound, "could not find associated rbac action") + ctxLogger.Error("unsupport resource", "group", group, "resource", resource) + return "", status.Error(codes.NotFound, "unsupported resource") } + + action, ok := t.action(verb) + if !ok { + ctxLogger.Error("unsupport verb", "group", group, "resource", resource, "verb", verb) + return "", status.Error(codes.NotFound, "unsupported verb") + } + return action, nil } @@ -437,15 +442,20 @@ func (s *Service) checkPermission(ctx context.Context, scopeMap map[string]bool, return true, nil } - scope, has := s.actionMapper.Scope(req.Group, req.Resource, req.Name) - if !has { - ctxLogger.Error("could not get attribute for resource", "resource", req.Resource) - return false, fmt.Errorf("could not get attribute for resource") + t, ok := s.mapper.translation(req.Group, req.Resource) + if !ok { + ctxLogger.Error("unsupport resource", "group", req.Group, "resource", req.Resource) + return false, status.Error(codes.NotFound, "unsupported resource") } - if scopeMap[scope] { + + if scopeMap[t.scope(req.Name)] { return true, nil } + if !t.folderSupport { + return false, nil + } + return s.checkInheritedPermissions(ctx, scopeMap, req) } @@ -556,25 +566,37 @@ func (s *Service) listPermission(ctx context.Context, scopeMap map[string]bool, defer span.End() ctxLogger := s.logger.FromContext(ctx) - folderMap, err := s.buildFolderTree(ctx, req.Namespace) - if err != nil { - ctxLogger.Error("could not build folder and dashboard tree", "error", err) - return nil, err + t, ok := s.mapper.translation(req.Group, req.Resource) + if !ok { + ctxLogger.Error("unsupport resource", "group", req.Group, "resource", req.Resource) + return nil, status.Error(codes.NotFound, "unsupported resource") + } + + var folderMap map[string]FolderNode + if t.folderSupport { + var err error + folderMap, err = s.buildFolderTree(ctx, req.Namespace) + if err != nil { + ctxLogger.Error("could not build folder and dashboard tree", "error", err) + return nil, err + } } folderSet := make(map[string]struct{}, len(scopeMap)) - dashSet := make(map[string]struct{}, len(scopeMap)) + + prefix := t.prefix() + itemSet := make(map[string]struct{}, len(scopeMap)) for scope := range scopeMap { if strings.HasPrefix(scope, "folders:uid:") { - identifier := scope[len("folders:uid:"):] + identifier := strings.TrimPrefix(scope, "folders:uid:") if _, ok := folderSet[identifier]; ok { continue } folderSet[identifier] = struct{}{} getChildren(folderMap, identifier, folderSet) - } else if strings.HasPrefix(scope, "dashboards:uid:") { - identifier := scope[len("dashboards:uid:"):] - dashSet[identifier] = struct{}{} + } else { + identifier := strings.TrimPrefix(scope, prefix) + itemSet[identifier] = struct{}{} } } @@ -583,13 +605,13 @@ func (s *Service) listPermission(ctx context.Context, scopeMap map[string]bool, folderList = append(folderList, folder) } - dashList := make([]string, 0, len(dashSet)) - for dash := range dashSet { - dashList = append(dashList, dash) + itemList := make([]string, 0, len(itemSet)) + for item := range itemSet { + itemList = append(itemList, item) } - span.SetAttributes(attribute.Int("num_folders", len(folderList)), attribute.Int("num_dashboards", len(dashList))) - return &authzv1.ListResponse{Folders: folderList, Items: dashList}, nil + span.SetAttributes(attribute.Int("num_folders", len(folderList)), attribute.Int("num_items", len(itemList))) + return &authzv1.ListResponse{Folders: folderList, Items: itemList}, nil } func getChildren(folderMap map[string]FolderNode, folderUID string, folderSet map[string]struct{}) { diff --git a/pkg/services/authz/rbac/service_test.go b/pkg/services/authz/rbac/service_test.go index 32c4cdf46dd..63d5f393cab 100644 --- a/pkg/services/authz/rbac/service_test.go +++ b/pkg/services/authz/rbac/service_test.go @@ -16,7 +16,6 @@ import ( "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/registry/apis/iam/legacy" "github.com/grafana/grafana/pkg/services/accesscontrol" - "github.com/grafana/grafana/pkg/services/authz/mappers" "github.com/grafana/grafana/pkg/services/authz/rbac/store" ) @@ -150,7 +149,11 @@ func TestService_checkPermission(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - s := &Service{logger: log.New("test"), actionMapper: mappers.NewK8sRbacMapper(), tracer: tracing.NewNoopTracerService()} + s := &Service{ + logger: log.NewNopLogger(), + tracer: tracing.NewNoopTracerService(), + mapper: newMapper(), + } got, err := s.checkPermission(context.Background(), getScopeMap(tc.permissions), &tc.check) require.NoError(t, err) assert.Equal(t, tc.expected, got) @@ -366,9 +369,9 @@ func TestService_getUserPermissions(t *testing.T) { store: store, permissionStore: store, identityStore: &fakeIdentityStore{teams: []int64{1, 2}}, - actionMapper: mappers.NewK8sRbacMapper(), - logger: log.New("test"), + logger: log.NewNopLogger(), tracer: tracing.NewNoopTracerService(), + mapper: newMapper(), idCache: localcache.New(longCacheTTL, longCleanupInterval), permCache: cacheService, sf: new(singleflight.Group), @@ -649,10 +652,10 @@ func TestService_listPermission(t *testing.T) { folderCache.Set(folderCacheKey("default"), tc.folderTree, 0) } s := &Service{ - logger: log.New("test"), - actionMapper: mappers.NewK8sRbacMapper(), - folderCache: folderCache, - tracer: tracing.NewNoopTracerService(), + logger: log.New("test"), + folderCache: folderCache, + mapper: newMapper(), + tracer: tracing.NewNoopTracerService(), } tc.list.Namespace = claims.NamespaceInfo{Value: "default", OrgID: 1} got, err := s.listPermission(context.Background(), getScopeMap(tc.permissions), &tc.list)