From 3f71a72c1a9d47f4bb58cd3654bcac79ed2e64ac Mon Sep 17 00:00:00 2001 From: Karl Persson <23356117+kalleep@users.noreply.github.com> Date: Wed, 15 Jan 2025 09:23:56 +0100 Subject: [PATCH] Authz: Remove "wrapper" interface and only check feature toggle for grpc mode (#98933) * Remove "wrapper" interface and only check feature toggle for grpc and cloud mode * Only set name for update checks * Set dashboard permissions for admin user --- .../backgroundsvcs/background_services.go | 3 +- pkg/services/authz/client.go | 37 ++++++------------- pkg/storage/unified/client.go | 6 +-- pkg/storage/unified/resource/server.go | 10 +++-- pkg/storage/unified/sql/server.go | 4 +- pkg/tests/apis/helper.go | 9 ++++- 6 files changed, 32 insertions(+), 37 deletions(-) diff --git a/pkg/registry/backgroundsvcs/background_services.go b/pkg/registry/backgroundsvcs/background_services.go index 3a42a83ead7..8435b86fede 100644 --- a/pkg/registry/backgroundsvcs/background_services.go +++ b/pkg/registry/backgroundsvcs/background_services.go @@ -15,7 +15,6 @@ import ( grafanaapiserver "github.com/grafana/grafana/pkg/services/apiserver" "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/authn/authnimpl" - "github.com/grafana/grafana/pkg/services/authz" "github.com/grafana/grafana/pkg/services/cleanup" "github.com/grafana/grafana/pkg/services/cloudmigration" "github.com/grafana/grafana/pkg/services/dashboardsnapshots" @@ -73,7 +72,7 @@ func ProvideBackgroundServiceRegistry( _ dashboardsnapshots.Service, _ serviceaccounts.Service, _ *guardian.Provider, _ *plugindashboardsservice.DashboardUpdater, _ *sanitizer.Provider, - _ *grpcserver.HealthService, _ authz.Client, _ *grpcserver.ReflectionService, + _ *grpcserver.HealthService, _ *grpcserver.ReflectionService, _ *ldapapi.Service, _ *apiregistry.Service, _ auth.IDService, _ *teamapi.TeamAPI, _ ssosettings.Service, _ cloudmigration.Service, _ authnimpl.Registration, ) *BackgroundServiceRegistry { diff --git a/pkg/services/authz/client.go b/pkg/services/authz/client.go index ccb88d4a399..c3399811a00 100644 --- a/pkg/services/authz/client.go +++ b/pkg/services/authz/client.go @@ -2,6 +2,7 @@ package authz import ( "context" + "errors" "github.com/fullstorydev/grpchan" "github.com/fullstorydev/grpchan/inprocgrpc" @@ -27,56 +28,40 @@ import ( // `authzService` is hardcoded in authz-service const authzServiceAudience = "authzService" -type Client interface { - authzlib.AccessClient -} - // ProvideAuthZClient provides an AuthZ client and creates the AuthZ service. func ProvideAuthZClient( cfg *setting.Cfg, features featuremgmt.FeatureToggles, grpcServer grpcserver.Provider, tracer tracing.Tracer, db db.DB, -) (Client, error) { - if !features.IsEnabledGlobally(featuremgmt.FlagAuthZGRPCServer) { - return nil, nil - } - +) (authzlib.AccessClient, error) { authCfg, err := ReadCfg(cfg) if err != nil { return nil, err } - var client Client + isRemoteServer := authCfg.mode == ModeCloud || authCfg.mode == ModeGRPC + if !features.IsEnabledGlobally(featuremgmt.FlagAuthZGRPCServer) && isRemoteServer { + return nil, errors.New("authZGRPCServer feature toggle is required for cloud and grpc mode") + } // Register the server sql := legacysql.NewDatabaseProvider(db) server := rbac.NewService(sql, legacy.NewLegacySQLStores(sql), log.New("authz-grpc-server"), tracer) switch authCfg.mode { - case ModeInProc: - client, err = newInProcLegacyClient(server, tracer) - if err != nil { - return nil, err - } case ModeGRPC: - client, err = newGrpcLegacyClient(authCfg, tracer) - if err != nil { - return nil, err - } + return newGrpcLegacyClient(authCfg, tracer) case ModeCloud: - client, err = newCloudLegacyClient(authCfg, tracer) - if err != nil { - return nil, err - } + return newCloudLegacyClient(authCfg, tracer) + default: + return newInProcLegacyClient(server, tracer) } - - return client, err } // ProvideStandaloneAuthZClient provides a standalone AuthZ client, without registering the AuthZ service. // You need to provide a remote address in the configuration func ProvideStandaloneAuthZClient( cfg *setting.Cfg, features featuremgmt.FeatureToggles, tracer tracing.Tracer, -) (Client, error) { +) (authzlib.AccessClient, error) { if !features.IsEnabledGlobally(featuremgmt.FlagAuthZGRPCServer) { return nil, nil } diff --git a/pkg/storage/unified/client.go b/pkg/storage/unified/client.go index 558a0b05187..dc4f8fa57c2 100644 --- a/pkg/storage/unified/client.go +++ b/pkg/storage/unified/client.go @@ -12,12 +12,12 @@ import ( "google.golang.org/grpc/credentials/insecure" authnlib "github.com/grafana/authlib/authn" + "github.com/grafana/authlib/authz" infraDB "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/apiserver/options" "github.com/grafana/grafana/pkg/services/authn/grpcutils" - "github.com/grafana/grafana/pkg/services/authz" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/storage/legacysql" @@ -35,7 +35,7 @@ func ProvideUnifiedStorageClient( db infraDB.DB, tracer tracing.Tracer, reg prometheus.Registerer, - authzc authz.Client, + authzc authz.AccessClient, docs resource.DocumentBuilderSupplier, ) (resource.ResourceClient, error) { // See: apiserver.ApplyGrafanaConfig(cfg, features, o) @@ -62,7 +62,7 @@ func newClient(opts options.StorageOptions, db infraDB.DB, tracer tracing.Tracer, reg prometheus.Registerer, - authzc authz.Client, + authzc authz.AccessClient, docs resource.DocumentBuilderSupplier, ) (resource.ResourceClient, error) { ctx := context.Background() diff --git a/pkg/storage/unified/resource/server.go b/pkg/storage/unified/resource/server.go index cc16bf64814..bfefdf3e2df 100644 --- a/pkg/storage/unified/resource/server.go +++ b/pkg/storage/unified/resource/server.go @@ -372,7 +372,7 @@ func (s *server) newEvent(ctx context.Context, user claims.AuthInfo, key *Resour } check := authz.CheckRequest{ - Verb: "create", + Verb: utils.VerbCreate, Group: key.Group, Resource: key.Resource, Namespace: key.Namespace, @@ -386,7 +386,7 @@ func (s *server) newEvent(ctx context.Context, user claims.AuthInfo, key *Resour if oldValue == nil { event.Type = WatchEvent_ADDED } else { - check.Verb = "update" + check.Verb = utils.VerbUpdate temp := &unstructured.Unstructured{} err = temp.UnmarshalJSON(oldValue) @@ -437,8 +437,12 @@ func (s *server) newEvent(ctx context.Context, user claims.AuthInfo, key *Resour return nil, err } + // We only set name for update checks + if check.Verb == utils.VerbUpdate { + check.Name = key.Name + } + check.Folder = obj.GetFolder() - check.Name = key.Name a, err := s.access.Check(ctx, user, check) if err != nil { return nil, AsErrorResult(err) diff --git a/pkg/storage/unified/sql/server.go b/pkg/storage/unified/sql/server.go index 3e0a93d966f..0269e395961 100644 --- a/pkg/storage/unified/sql/server.go +++ b/pkg/storage/unified/sql/server.go @@ -7,13 +7,13 @@ import ( "path/filepath" "strings" + "github.com/grafana/authlib/authz" "github.com/prometheus/client_golang/prometheus" "github.com/grafana/grafana/pkg/storage/unified/search" infraDB "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/tracing" - "github.com/grafana/grafana/pkg/services/authz" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/storage/unified/resource" @@ -23,7 +23,7 @@ import ( // Creates a new ResourceServer func NewResourceServer(ctx context.Context, db infraDB.DB, cfg *setting.Cfg, features featuremgmt.FeatureToggles, docs resource.DocumentBuilderSupplier, - tracer tracing.Tracer, reg prometheus.Registerer, ac authz.Client) (resource.ResourceServer, error) { + tracer tracing.Tracer, reg prometheus.Registerer, ac authz.AccessClient) (resource.ResourceServer, error) { apiserverCfg := cfg.SectionWithEnvOverrides("grafana-apiserver") opts := resource.ResourceServerOptions{ Tracer: tracer, diff --git a/pkg/tests/apis/helper.go b/pkg/tests/apis/helper.go index 7d1d3084e81..13ccc13d2e6 100644 --- a/pkg/tests/apis/helper.go +++ b/pkg/tests/apis/helper.go @@ -442,7 +442,14 @@ func (c *K8sTestHelper) LoadYAMLOrJSON(body string) *unstructured.Unstructured { func (c *K8sTestHelper) createTestUsers(orgName string) OrgUsers { c.t.Helper() users := OrgUsers{ - Admin: c.CreateUser("admin", orgName, org.RoleAdmin, nil), + Admin: c.CreateUser("admin", orgName, org.RoleAdmin, []resourcepermissions.SetResourcePermissionCommand{ + { + Actions: []string{"dashboards:read", "dashboards:write", "dashboards:create", "dashboards:delete"}, + Resource: "dashboards", + ResourceAttribute: "uid", + ResourceID: "*", + }, + }), Editor: c.CreateUser("editor", orgName, org.RoleEditor, nil), Viewer: c.CreateUser("viewer", orgName, org.RoleViewer, nil), }