From 870929b4637532d07e0aac6e69c6be22877091e4 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Wed, 14 Sep 2022 09:13:10 +0200 Subject: [PATCH] RBAC: Add cache for oss rbac permissions (#55098) * RBAC: Add cache for oss permissions * RBAC: include service account actions * RBAC: revert changes to fetch service account permissions * Update comment for setting * RBAC: Disable permission chache for tests --- conf/defaults.ini | 2 +- pkg/api/common_test.go | 3 +- pkg/services/accesscontrol/acimpl/service.go | 60 +++++++++++++++++-- .../accesscontrol/acimpl/service_test.go | 2 + .../ossaccesscontrol/permissions_services.go | 18 +++++- pkg/tests/testinfra/testinfra.go | 5 ++ 6 files changed, 81 insertions(+), 9 deletions(-) diff --git a/conf/defaults.ini b/conf/defaults.ini index 8cad3c0a891..6b640bf452a 100644 --- a/conf/defaults.ini +++ b/conf/defaults.ini @@ -666,7 +666,7 @@ managed_identity_client_id = #################################### Role-based Access Control ########### [rbac] -# If enabled, cache permissions in a in memory cache (Enterprise only) +# If enabled, cache permissions in a in memory cache permission_cache = true #################################### SMTP / Emailing ##################### diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 74111f26669..e64c9ae8116 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -17,6 +17,7 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/infra/fs" + "github.com/grafana/grafana/pkg/infra/localcache" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/remotecache" "github.com/grafana/grafana/pkg/infra/tracing" @@ -379,7 +380,7 @@ func setupHTTPServerWithCfgDb( acService = acmock } else { var err error - acService, err = acimpl.ProvideService(cfg, database.ProvideService(db), routeRegister) + acService, err = acimpl.ProvideService(cfg, database.ProvideService(db), routeRegister, localcache.ProvideService()) require.NoError(t, err) ac = acimpl.ProvideAccessControl(cfg) } diff --git a/pkg/services/accesscontrol/acimpl/service.go b/pkg/services/accesscontrol/acimpl/service.go index dcfeb604214..cd4aaf72a20 100644 --- a/pkg/services/accesscontrol/acimpl/service.go +++ b/pkg/services/accesscontrol/acimpl/service.go @@ -2,8 +2,11 @@ package acimpl import ( "context" + "fmt" + "time" "github.com/grafana/grafana/pkg/api/routing" + "github.com/grafana/grafana/pkg/infra/localcache" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/metrics" "github.com/grafana/grafana/pkg/services/accesscontrol" @@ -14,8 +17,12 @@ import ( "github.com/prometheus/client_golang/prometheus" ) -func ProvideService(cfg *setting.Cfg, store accesscontrol.Store, routeRegister routing.RouteRegister) (*Service, error) { - service := ProvideOSSService(cfg, store) +const ( + cacheTTL = 10 * time.Second +) + +func ProvideService(cfg *setting.Cfg, store accesscontrol.Store, routeRegister routing.RouteRegister, cache *localcache.CacheService) (*Service, error) { + service := ProvideOSSService(cfg, store, cache) if !accesscontrol.IsDisabled(cfg) { api.NewAccessControlAPI(routeRegister, service).RegisterAPIEndpoints() @@ -27,11 +34,12 @@ func ProvideService(cfg *setting.Cfg, store accesscontrol.Store, routeRegister r return service, nil } -func ProvideOSSService(cfg *setting.Cfg, store accesscontrol.Store) *Service { +func ProvideOSSService(cfg *setting.Cfg, store accesscontrol.Store, cache *localcache.CacheService) *Service { s := &Service{ cfg: cfg, store: store, log: log.New("accesscontrol.service"), + cache: cache, roles: accesscontrol.BuildBasicRoleDefinitions(), } @@ -43,6 +51,7 @@ type Service struct { log log.Logger cfg *setting.Cfg store accesscontrol.Store + cache *localcache.CacheService registrations accesscontrol.RegistrationList roles map[string]*accesscontrol.RoleDTO } @@ -63,12 +72,19 @@ var actionsToFetch = append( ) // GetUserPermissions returns user permissions based on built-in roles -func (s *Service) GetUserPermissions(ctx context.Context, user *user.SignedInUser, _ accesscontrol.Options) ([]accesscontrol.Permission, error) { +func (s *Service) GetUserPermissions(ctx context.Context, user *user.SignedInUser, options accesscontrol.Options) ([]accesscontrol.Permission, error) { timer := prometheus.NewTimer(metrics.MAccessPermissionsSummary) defer timer.ObserveDuration() - permissions := make([]accesscontrol.Permission, 0) + if !s.cfg.RBACPermissionCache || !user.HasUniqueId() { + return s.getUserPermissions(ctx, user, options) + } + return s.getCachedUserPermissions(ctx, user, options) +} + +func (s *Service) getUserPermissions(ctx context.Context, user *user.SignedInUser, options accesscontrol.Options) ([]accesscontrol.Permission, error) { + permissions := make([]accesscontrol.Permission, 0) for _, builtin := range accesscontrol.GetOrgRoles(user) { if basicRole, ok := s.roles[builtin]; ok { permissions = append(permissions, basicRole.Permissions...) @@ -89,6 +105,32 @@ func (s *Service) GetUserPermissions(ctx context.Context, user *user.SignedInUse return append(permissions, dbPermissions...), nil } +func (s *Service) getCachedUserPermissions(ctx context.Context, user *user.SignedInUser, options accesscontrol.Options) ([]accesscontrol.Permission, error) { + key, err := permissionCacheKey(user) + if err != nil { + return nil, err + } + + if !options.ReloadCache { + permissions, ok := s.cache.Get(key) + if ok { + s.log.Debug("using cached permissions", "key", key) + return permissions.([]accesscontrol.Permission), nil + } + } + + s.log.Debug("fetch permissions from store", "key", key) + permissions, err := s.getUserPermissions(ctx, user, options) + if err != nil { + return nil, err + } + + s.log.Debug("cache permissions", "key", key) + s.cache.Set(key, permissions, cacheTTL) + + return permissions, nil +} + func (s *Service) DeleteUserPermissions(ctx context.Context, orgID int64, userID int64) error { return s.store.DeleteUserPermissions(ctx, orgID, userID) } @@ -140,3 +182,11 @@ func (s *Service) RegisterFixedRoles(ctx context.Context) error { func (s *Service) IsDisabled() bool { return accesscontrol.IsDisabled(s.cfg) } + +func permissionCacheKey(user *user.SignedInUser) (string, error) { + key, err := user.GetCacheKey() + if err != nil { + return "", err + } + return fmt.Sprintf("rbac-permissions-%s", key), nil +} diff --git a/pkg/services/accesscontrol/acimpl/service_test.go b/pkg/services/accesscontrol/acimpl/service_test.go index 9d948467b37..d0dcc50b9c6 100644 --- a/pkg/services/accesscontrol/acimpl/service_test.go +++ b/pkg/services/accesscontrol/acimpl/service_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/api/routing" + "github.com/grafana/grafana/pkg/infra/localcache" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" @@ -59,6 +60,7 @@ func TestUsageMetrics(t *testing.T) { cfg, database.ProvideService(sqlstore.InitTestDB(t)), routing.NewRouteRegister(), + localcache.ProvideService(), ) require.NoError(t, errInitAc) assert.Equal(t, tt.expectedValue, s.GetUsageStats(context.Background())["stats.oss.accesscontrol.enabled.count"]) diff --git a/pkg/services/accesscontrol/ossaccesscontrol/permissions_services.go b/pkg/services/accesscontrol/ossaccesscontrol/permissions_services.go index 0535e783621..bab1e8faed7 100644 --- a/pkg/services/accesscontrol/ossaccesscontrol/permissions_services.go +++ b/pkg/services/accesscontrol/ossaccesscontrol/permissions_services.go @@ -258,6 +258,20 @@ func (e DatasourcePermissionsService) MapActions(permission accesscontrol.Resour return "" } +var ( + ServiceAccountEditActions = []string{ + serviceaccounts.ActionRead, + serviceaccounts.ActionWrite, + } + ServiceAccountAdminActions = []string{ + serviceaccounts.ActionRead, + serviceaccounts.ActionWrite, + serviceaccounts.ActionDelete, + serviceaccounts.ActionPermissionsRead, + serviceaccounts.ActionPermissionsWrite, + } +) + type ServiceAccountPermissionsService struct { *resourcepermissions.Service } @@ -283,8 +297,8 @@ func ProvideServiceAccountPermissions( BuiltInRoles: false, }, PermissionsToActions: map[string][]string{ - "Edit": {serviceaccounts.ActionRead, serviceaccounts.ActionWrite}, - "Admin": {serviceaccounts.ActionRead, serviceaccounts.ActionWrite, serviceaccounts.ActionDelete, serviceaccounts.ActionPermissionsRead, serviceaccounts.ActionPermissionsWrite}, + "Edit": ServiceAccountEditActions, + "Admin": ServiceAccountAdminActions, }, ReaderRoleName: "Service account permission reader", WriterRoleName: "Service account permission writer", diff --git a/pkg/tests/testinfra/testinfra.go b/pkg/tests/testinfra/testinfra.go index 4ac4e6db647..8693d722cfe 100644 --- a/pkg/tests/testinfra/testinfra.go +++ b/pkg/tests/testinfra/testinfra.go @@ -202,6 +202,11 @@ func CreateGrafDir(t *testing.T, opts ...GrafanaOpts) (string, string) { _, err = alertingSect.NewKey("max_attempts", "3") require.NoError(t, err) + rbacSect, err := cfg.NewSection("rbac") + require.NoError(t, err) + _, err = rbacSect.NewKey("permission_cache", "false") + require.NoError(t, err) + getOrCreateSection := func(name string) (*ini.Section, error) { section, err := cfg.GetSection(name) if err != nil {