From e642e1a804d4b9c4260dfcaa1a01105efe3aad30 Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Thu, 10 Oct 2024 17:38:15 +0200 Subject: [PATCH] Zanzana: Pass parent folder for the checks in search queries (#94541) * Pass parent folder as a contextual tuple in Check request * Search by listing folders and dashboards * skip dashboards listing if limit reached * remove unused * add some comments * only add ContextualTuples if parent provided * Remove parent relation for dashboards from schema and perform separate checks --- .../accesscontrol/acimpl/accesscontrol.go | 40 +++++- .../accesscontrol/migrator/zanzana.go | 43 +------ pkg/services/accesscontrol/models.go | 9 +- .../authz/zanzana/schema/dashboard.fga | 23 ++-- pkg/services/authz/zanzana/zanzana.go | 5 + pkg/services/dashboards/service/zanzana.go | 119 +++++++----------- 6 files changed, 109 insertions(+), 130 deletions(-) diff --git a/pkg/services/accesscontrol/acimpl/accesscontrol.go b/pkg/services/accesscontrol/acimpl/accesscontrol.go index d540474abfc..ed3ee6eba45 100644 --- a/pkg/services/accesscontrol/acimpl/accesscontrol.go +++ b/pkg/services/accesscontrol/acimpl/accesscontrol.go @@ -3,12 +3,15 @@ package acimpl import ( "context" "errors" + "strconv" "time" openfgav1 "github.com/openfga/api/proto/openfga/v1" "github.com/prometheus/client_golang/prometheus" "go.opentelemetry.io/otel" + "github.com/grafana/authlib/claims" + "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/metrics" @@ -127,6 +130,7 @@ func (a *AccessControl) evaluateZanzana(ctx context.Context, user identity.Reque a.log.Debug("evaluating zanzana", "user", tupleKey.User, "relation", tupleKey.Relation, "object", tupleKey.Object) allowed, err := a.Check(ctx, accesscontrol.CheckRequest{ + // Namespace: claims.OrgNamespaceFormatter(user.GetOrgID()), User: tupleKey.User, Relation: tupleKey.Relation, Object: tupleKey.Object, @@ -226,12 +230,44 @@ func (a *AccessControl) Check(ctx context.Context, req accesscontrol.CheckReques Relation: req.Relation, Object: req.Object, } - in := &openfgav1.CheckRequest{TupleKey: key} + + in := &openfgav1.CheckRequest{ + TupleKey: key, + } + + // Check direct access to resource first res, err := a.zclient.Check(ctx, in) if err != nil { return false, err } - return res.Allowed, err + + // no need to check folder access + if res.Allowed || req.Parent == "" { + return res.Allowed, nil + } + + // Check access through the parent folder + ns, err := claims.ParseNamespace(req.Namespace) + if err != nil { + return false, err + } + + folderKey := &openfgav1.CheckRequestTupleKey{ + User: req.User, + Relation: zanzana.TranslateToFolderRelation(req.Relation, req.ObjectType), + Object: zanzana.NewScopedTupleEntry(zanzana.TypeFolder, req.Parent, "", strconv.FormatInt(ns.OrgID, 10)), + } + + folderReq := &openfgav1.CheckRequest{ + TupleKey: folderKey, + } + + folderRes, err := a.zclient.Check(ctx, folderReq) + if err != nil { + return false, err + } + + return folderRes.Allowed, nil } func (a *AccessControl) ListObjects(ctx context.Context, req accesscontrol.ListObjectsRequest) ([]string, error) { diff --git a/pkg/services/accesscontrol/migrator/zanzana.go b/pkg/services/accesscontrol/migrator/zanzana.go index f27316f2630..7c27f31b624 100644 --- a/pkg/services/accesscontrol/migrator/zanzana.go +++ b/pkg/services/accesscontrol/migrator/zanzana.go @@ -39,7 +39,6 @@ func NewZanzanaSynchroniser(client zanzana.Client, store db.DB, collectors ...Tu teamMembershipCollector(store), managedPermissionsCollector(store), folderTreeCollector(store), - dashboardFolderCollector(store), basicRolesCollector(store), customRolesCollector(store), basicRoleAssignemtCollector(store), @@ -58,6 +57,7 @@ func NewZanzanaSynchroniser(client zanzana.Client, store db.DB, collectors ...Tu // Sync runs all collectors and tries to write all collected tuples. // It will skip over any "sync group" that has already been written. func (z *ZanzanaSynchroniser) Sync(ctx context.Context) error { + z.log.Info("Starting zanzana permissions sync") ctx, span := tracer.Start(ctx, "accesscontrol.migrator.Sync") defer span.End() @@ -246,47 +246,6 @@ func folderTreeCollector(store db.DB) TupleCollector { } } -// dashboardFolderCollector collects information about dashboards parent folders -func dashboardFolderCollector(store db.DB) TupleCollector { - return func(ctx context.Context, tuples map[string][]*openfgav1.TupleKey) error { - ctx, span := tracer.Start(ctx, "accesscontrol.migrator.dashboardFolderCollector") - defer span.End() - - const collectorID = "folder" - query := ` - SELECT org_id, uid, folder_uid, is_folder FROM dashboard - WHERE is_folder = ` + store.GetDialect().BooleanStr(false) + ` - AND folder_uid IS NOT NULL - ` - type dashboard struct { - OrgID int64 `xorm:"org_id"` - UID string `xorm:"uid"` - ParentUID string `xorm:"folder_uid"` - } - - var dashboards []dashboard - err := store.WithDbSession(ctx, func(sess *db.Session) error { - return sess.SQL(query).Find(&dashboards) - }) - - if err != nil { - return err - } - - for _, d := range dashboards { - tuple := &openfgav1.TupleKey{ - User: zanzana.NewScopedTupleEntry(zanzana.TypeFolder, d.ParentUID, "", strconv.FormatInt(d.OrgID, 10)), - Object: zanzana.NewScopedTupleEntry(zanzana.TypeDashboard, d.UID, "", strconv.FormatInt(d.OrgID, 10)), - Relation: zanzana.RelationParent, - } - - tuples[collectorID] = append(tuples[collectorID], tuple) - } - - return nil - } -} - // basicRolesCollector migrates basic roles to OpenFGA tuples func basicRolesCollector(store db.DB) TupleCollector { return func(ctx context.Context, tuples map[string][]*openfgav1.TupleKey) error { diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index c28a01429e1..9ee384540c1 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -589,9 +589,12 @@ type QueryWithOrg struct { } type CheckRequest struct { - User string - Relation string - Object string + Namespace string + User string + Relation string + Object string + ObjectType string + Parent string } type ListObjectsRequest struct { diff --git a/pkg/services/authz/zanzana/schema/dashboard.fga b/pkg/services/authz/zanzana/schema/dashboard.fga index ee08372ac7f..b9e77da81a1 100644 --- a/pkg/services/authz/zanzana/schema/dashboard.fga +++ b/pkg/services/authz/zanzana/schema/dashboard.fga @@ -28,18 +28,17 @@ extend type org type dashboard relations define org: [org] - define parent: [folder] - define read: [user, team#member, role#assignee] or dashboard_read from parent or dashboard_read from org - define write: [user, team#member, role#assignee] or dashboard_write from parent or dashboard_write from org - define delete: [user, team#member, role#assignee] or dashboard_delete from parent or dashboard_delete from org - define create: [user, team#member, role#assignee] or dashboard_create from parent or dashboard_create from org - define permissions_read: [user, team#member, role#assignee] or dashboard_permissions_read from parent or dashboard_permissions_read from org - define permissions_write: [user, team#member, role#assignee] or dashboard_permissions_write from parent or dashboard_permissions_write from org + define read: [user, team#member, role#assignee] or dashboard_read from org + define write: [user, team#member, role#assignee] or dashboard_write from org + define delete: [user, team#member, role#assignee] or dashboard_delete from org + define create: [user, team#member, role#assignee] or dashboard_create from org + define permissions_read: [user, team#member, role#assignee] or dashboard_permissions_read from org + define permissions_write: [user, team#member, role#assignee] or dashboard_permissions_write from org - define public_write: [user, team#member, role#assignee] or dashboard_public_write from parent or dashboard_public_write from org or write - define annotations_create: [user, team#member, role#assignee] or dashboard_annotations_create from parent or dashboard_annotations_create from org - define annotations_read: [user, team#member, role#assignee] or dashboard_annotations_read from parent or dashboard_annotations_read from org - define annotations_write: [user, team#member, role#assignee] or dashboard_annotations_write from parent or dashboard_annotations_write from org - define annotations_delete: [user, team#member, role#assignee] or dashboard_annotations_delete from parent or dashboard_annotations_delete from org + define public_write: [user, team#member, role#assignee] or dashboard_public_write from org or write + define annotations_create: [user, team#member, role#assignee] or dashboard_annotations_create from org + define annotations_read: [user, team#member, role#assignee] or dashboard_annotations_read from org + define annotations_write: [user, team#member, role#assignee] or dashboard_annotations_write from org + define annotations_delete: [user, team#member, role#assignee] or dashboard_annotations_delete from org diff --git a/pkg/services/authz/zanzana/zanzana.go b/pkg/services/authz/zanzana/zanzana.go index 03bfe9f36ed..ece56e98179 100644 --- a/pkg/services/authz/zanzana/zanzana.go +++ b/pkg/services/authz/zanzana/zanzana.go @@ -118,3 +118,8 @@ func TranslateFixedRole(role string) string { role = strings.ReplaceAll(role, ".", "_") return role } + +// Translate "read" for the dashboard into "dashboard_read" for folder +func TranslateToFolderRelation(relation, objectType string) string { + return fmt.Sprintf("%s_%s", objectType, relation) +} diff --git a/pkg/services/dashboards/service/zanzana.go b/pkg/services/dashboards/service/zanzana.go index f7bfb4d23f6..7441df302cf 100644 --- a/pkg/services/dashboards/service/zanzana.go +++ b/pkg/services/dashboards/service/zanzana.go @@ -10,10 +10,11 @@ import ( "github.com/prometheus/client_golang/prometheus" + "github.com/grafana/authlib/claims" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/authz/zanzana" "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/sqlstore/searchstore" ) const ( @@ -193,9 +194,16 @@ func (dr *DashboardServiceImpl) checkDashboards(ctx context.Context, query dashb } req := accesscontrol.CheckRequest{ - User: query.SignedInUser.GetUID(), - Relation: "read", - Object: zanzana.NewScopedTupleEntry(objectType, d.UID, "", strconv.FormatInt(orgId, 10)), + Namespace: claims.OrgNamespaceFormatter(orgId), + User: query.SignedInUser.GetUID(), + Relation: "read", + Object: zanzana.NewScopedTupleEntry(objectType, d.UID, "", strconv.FormatInt(orgId, 10)), + } + + if objectType != zanzana.TypeFolder { + // Pass parentn folder for the correct check + req.Parent = d.FolderUID + req.ObjectType = objectType } allowed, err := dr.ac.Check(ctx, req) @@ -238,45 +246,47 @@ func (dr *DashboardServiceImpl) findDashboardsZanzanaList(ctx context.Context, q ctx, span := tracer.Start(ctx, "dashboards.service.findDashboardsZanzanaList") defer span.End() - resourceUIDs, err := dr.listUserResources(ctx, query) - if err != nil { - return nil, err - } - if len(resourceUIDs) == 0 { - return []dashboards.DashboardSearchProjection{}, nil - } + var result []dashboards.DashboardSearchProjection - query.DashboardUIDs = resourceUIDs - query.SkipAccessControlFilter = true - return dr.dashboardStore.FindDashboards(ctx, &query) -} - -func (dr *DashboardServiceImpl) listUserResources(ctx context.Context, query dashboards.FindPersistedDashboardsQuery) ([]string, error) { - tasks := make([]func() ([]string, error), 0) - var resourceTypes []string - - // For some search types we need dashboards or folders only - switch query.Type { - case searchstore.TypeDashboard: - resourceTypes = []string{zanzana.TypeDashboard} - case searchstore.TypeFolder, searchstore.TypeAlertFolder: - resourceTypes = []string{zanzana.TypeFolder} - default: - resourceTypes = []string{zanzana.TypeDashboard, zanzana.TypeFolder} - } - - for _, resourceType := range resourceTypes { - tasks = append(tasks, func() ([]string, error) { - return dr.listAllowedResources(ctx, query, resourceType) - }) - } - - uids, err := runBatch(tasks) + allowedFolders, err := dr.listAllowedResources(ctx, query, zanzana.TypeFolder) if err != nil { return nil, err } - return uids, nil + if len(allowedFolders) > 0 { + // Find dashboards in folders that user has access to + query.SkipAccessControlFilter = true + query.FolderUIDs = allowedFolders + result, err = dr.dashboardStore.FindDashboards(ctx, &query) + if err != nil { + return nil, err + } + } + + // skip if limit reached + rest := query.Limit - int64(len(result)) + if rest <= 0 { + return result, nil + } + + // Run second query to find dashboards with direct permission assignments + allowedDashboards, err := dr.listAllowedResources(ctx, query, zanzana.TypeDashboard) + if err != nil { + return nil, err + } + + if len(allowedDashboards) > 0 { + query.FolderUIDs = []string{} + query.DashboardUIDs = allowedDashboards + query.Limit = rest + dashboardRes, err := dr.dashboardStore.FindDashboards(ctx, &query) + if err != nil { + return nil, err + } + result = append(result, dashboardRes...) + } + + return result, err } func (dr *DashboardServiceImpl) listAllowedResources(ctx context.Context, query dashboards.FindPersistedDashboardsQuery, resourceType string) ([]string, error) { @@ -307,36 +317,3 @@ func (dr *DashboardServiceImpl) listAllowedResources(ctx context.Context, query return resourceUIDs, nil } - -func runBatch(tasks []func() ([]string, error)) ([]string, error) { - var wg sync.WaitGroup - tasksNum := len(tasks) - resChan := make(chan []string, tasksNum) - errChan := make(chan error, tasksNum) - - for _, task := range tasks { - wg.Add(1) - go func() { - defer wg.Done() - res, err := task() - resChan <- res - errChan <- err - }() - } - - wg.Wait() - close(resChan) - close(errChan) - - for err := range errChan { - if err != nil { - return nil, err - } - } - - result := make([]string, 0) - for res := range resChan { - result = append(result, res...) - } - return result, nil -}