From dc6134f3f8066545efe1be099224f3678feed419 Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Wed, 11 Sep 2024 15:15:09 +0200 Subject: [PATCH] refactor: use own types for Check and ListObjects inside accesscontrol package --- pkg/services/accesscontrol/accesscontrol.go | 8 +--- .../accesscontrol/acimpl/accesscontrol.go | 43 +++++++++++++------ pkg/services/accesscontrol/actest/fake.go | 8 ++-- pkg/services/accesscontrol/mock/mock.go | 8 ++-- pkg/services/accesscontrol/models.go | 12 ++++++ pkg/services/dashboards/service/zanzana.go | 14 +++--- pkg/services/ngalert/accesscontrol/testing.go | 8 ++-- pkg/services/ngalert/api/testing.go | 9 ++-- 8 files changed, 63 insertions(+), 47 deletions(-) diff --git a/pkg/services/accesscontrol/accesscontrol.go b/pkg/services/accesscontrol/accesscontrol.go index 8c9145964e0..ca5c6af8b6b 100644 --- a/pkg/services/accesscontrol/accesscontrol.go +++ b/pkg/services/accesscontrol/accesscontrol.go @@ -7,7 +7,6 @@ import ( "strings" "github.com/grafana/authlib/claims" - openfgav1 "github.com/openfga/api/proto/openfga/v1" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" @@ -29,8 +28,8 @@ type AccessControl interface { // specific scope prefix (ex: datasources:name:) RegisterScopeAttributeResolver(prefix string, resolver ScopeAttributeResolver) - Check(ctx context.Context, in *openfgav1.CheckRequest) (*openfgav1.CheckResponse, error) - ListObjects(ctx context.Context, in *openfgav1.ListObjectsRequest) (*openfgav1.ListObjectsResponse, error) + Check(ctx context.Context, req CheckRequest) (bool, error) + ListObjects(ctx context.Context, req ListObjectsRequest) ([]string, error) } type Service interface { @@ -62,9 +61,6 @@ type Service interface { SyncUserRoles(ctx context.Context, orgID int64, cmd SyncUserRolesCommand) error } -type ZanzanaChecker interface { -} - //go:generate mockery --name Store --structname MockStore --outpkg actest --filename store_mock.go --output ./actest/ type Store interface { GetUserPermissions(ctx context.Context, query GetUserPermissionsQuery) ([]Permission, error) diff --git a/pkg/services/accesscontrol/acimpl/accesscontrol.go b/pkg/services/accesscontrol/acimpl/accesscontrol.go index 107ca76e0a6..ef2827078f7 100644 --- a/pkg/services/accesscontrol/acimpl/accesscontrol.go +++ b/pkg/services/accesscontrol/acimpl/accesscontrol.go @@ -119,26 +119,24 @@ func (a *AccessControl) evaluateZanzana(ctx context.Context, user identity.Reque return eval.EvaluateCustom(func(action, scope string) (bool, error) { kind, _, identifier := accesscontrol.SplitScope(scope) - key, ok := zanzana.TranslateToTuple(user.GetUID(), action, kind, identifier, user.GetOrgID()) + tupleKey, ok := zanzana.TranslateToTuple(user.GetUID(), action, kind, identifier, user.GetOrgID()) if !ok { // unsupported translation return false, errAccessNotImplemented } - a.log.Debug("evaluating zanzana", "user", key.User, "relation", key.Relation, "object", key.Object) - res, err := a.zclient.Check(ctx, &openfgav1.CheckRequest{ - TupleKey: &openfgav1.CheckRequestTupleKey{ - User: key.User, - Relation: key.Relation, - Object: key.Object, - }, + a.log.Debug("evaluating zanzana", "user", tupleKey.User, "relation", tupleKey.Relation, "object", tupleKey.Object) + allowed, err := a.Check(ctx, accesscontrol.CheckRequest{ + User: tupleKey.User, + Relation: tupleKey.Relation, + Object: tupleKey.Object, }) if err != nil { return false, err } - return res.Allowed, nil + return allowed, nil }) } @@ -212,10 +210,29 @@ func (a *AccessControl) debug(ctx context.Context, ident identity.Requester, msg a.log.FromContext(ctx).Debug(msg, "id", ident.GetID(), "orgID", ident.GetOrgID(), "permissions", eval.GoString()) } -func (a *AccessControl) Check(ctx context.Context, in *openfgav1.CheckRequest) (*openfgav1.CheckResponse, error) { - return a.zclient.Check(ctx, in) +func (a *AccessControl) Check(ctx context.Context, req accesscontrol.CheckRequest) (bool, error) { + key := &openfgav1.CheckRequestTupleKey{ + User: req.User, + Relation: req.Relation, + Object: req.Object, + } + in := &openfgav1.CheckRequest{TupleKey: key} + res, err := a.zclient.Check(ctx, in) + if err != nil { + return false, err + } + return res.Allowed, err } -func (a *AccessControl) ListObjects(ctx context.Context, in *openfgav1.ListObjectsRequest) (*openfgav1.ListObjectsResponse, error) { - return a.zclient.ListObjects(ctx, in) +func (a *AccessControl) ListObjects(ctx context.Context, req accesscontrol.ListObjectsRequest) ([]string, error) { + in := &openfgav1.ListObjectsRequest{ + Type: req.Type, + User: req.User, + Relation: req.Relation, + } + res, err := a.zclient.ListObjects(ctx, in) + if err != nil { + return nil, err + } + return res.Objects, err } diff --git a/pkg/services/accesscontrol/actest/fake.go b/pkg/services/accesscontrol/actest/fake.go index 3d9638ff9cd..822999395d6 100644 --- a/pkg/services/accesscontrol/actest/fake.go +++ b/pkg/services/accesscontrol/actest/fake.go @@ -3,8 +3,6 @@ package actest import ( "context" - openfgav1 "github.com/openfga/api/proto/openfga/v1" - "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/services/accesscontrol" ) @@ -77,11 +75,11 @@ func (f FakeAccessControl) Evaluate(ctx context.Context, user identity.Requester func (f FakeAccessControl) RegisterScopeAttributeResolver(prefix string, resolver accesscontrol.ScopeAttributeResolver) { } -func (f FakeAccessControl) Check(ctx context.Context, in *openfgav1.CheckRequest) (*openfgav1.CheckResponse, error) { - return nil, nil +func (f FakeAccessControl) Check(ctx context.Context, in accesscontrol.CheckRequest) (bool, error) { + return false, nil } -func (f FakeAccessControl) ListObjects(ctx context.Context, in *openfgav1.ListObjectsRequest) (*openfgav1.ListObjectsResponse, error) { +func (f FakeAccessControl) ListObjects(ctx context.Context, in accesscontrol.ListObjectsRequest) ([]string, error) { return nil, nil } diff --git a/pkg/services/accesscontrol/mock/mock.go b/pkg/services/accesscontrol/mock/mock.go index 11c2da73674..6a48df938df 100644 --- a/pkg/services/accesscontrol/mock/mock.go +++ b/pkg/services/accesscontrol/mock/mock.go @@ -4,8 +4,6 @@ import ( "context" "errors" - openfgav1 "github.com/openfga/api/proto/openfga/v1" - "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/plugins" @@ -268,10 +266,10 @@ func (m *Mock) SyncUserRoles(ctx context.Context, orgID int64, cmd accesscontrol return nil } -func (m *Mock) Check(ctx context.Context, in *openfgav1.CheckRequest) (*openfgav1.CheckResponse, error) { - return nil, nil +func (m *Mock) Check(ctx context.Context, in accesscontrol.CheckRequest) (bool, error) { + return false, nil } -func (m *Mock) ListObjects(ctx context.Context, in *openfgav1.ListObjectsRequest) (*openfgav1.ListObjectsResponse, error) { +func (m *Mock) ListObjects(ctx context.Context, in accesscontrol.ListObjectsRequest) ([]string, error) { return nil, nil } diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index 7bc0ae81aa6..8f2471c301e 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -578,3 +578,15 @@ type QueryWithOrg struct { OrgId *int64 `json:"orgId"` Global bool `json:"global"` } + +type CheckRequest struct { + User string + Relation string + Object string +} + +type ListObjectsRequest struct { + Type string + Relation string + User string +} diff --git a/pkg/services/dashboards/service/zanzana.go b/pkg/services/dashboards/service/zanzana.go index a6228e326e2..f58dd6ca50d 100644 --- a/pkg/services/dashboards/service/zanzana.go +++ b/pkg/services/dashboards/service/zanzana.go @@ -8,9 +8,9 @@ import ( "sync" "time" - openfgav1 "github.com/openfga/api/proto/openfga/v1" "github.com/prometheus/client_golang/prometheus" + "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" @@ -170,7 +170,7 @@ func (dr *DashboardServiceImpl) findDashboardsZanzanaList(ctx context.Context, q } func (dr *DashboardServiceImpl) listResources(ctx context.Context, query *dashboards.FindPersistedDashboardsQuery, resourceType string) ([]string, error) { - res, err := dr.ac.ListObjects(ctx, &openfgav1.ListObjectsRequest{ + res, err := dr.ac.ListObjects(ctx, accesscontrol.ListObjectsRequest{ User: query.SignedInUser.GetUID(), Type: resourceType, Relation: "read", @@ -187,7 +187,7 @@ func (dr *DashboardServiceImpl) listResources(ctx context.Context, query *dashbo prefix := fmt.Sprintf("%s:%d-", resourceType, orgId) resourceUIDs := make([]string, 0) - for _, d := range res.Objects { + for _, d := range res { if uid, found := strings.CutPrefix(d, prefix); found { resourceUIDs = append(resourceUIDs, uid) } @@ -234,19 +234,17 @@ func (dr *DashboardServiceImpl) checkDashboards(ctx context.Context, query *dash objectType = zanzana.TypeFolder } object := zanzana.NewScopedTupleEntry(objectType, d.UID, "", strconv.FormatInt(orgId, 10)) - key := &openfgav1.CheckRequestTupleKey{ + req := accesscontrol.CheckRequest{ User: query.SignedInUser.GetUID(), Relation: "read", Object: object, } - checkRes, err := dr.ac.Check(ctx, &openfgav1.CheckRequest{ - TupleKey: key, - }) + allowed, err := dr.ac.Check(ctx, req) if err != nil { errChan <- err dr.log.Error("error checking access", "error", err) - } else if checkRes.Allowed { + } else if allowed { allowedResults <- d } } diff --git a/pkg/services/ngalert/accesscontrol/testing.go b/pkg/services/ngalert/accesscontrol/testing.go index f0bfc6a9d1c..6b26e6f7677 100644 --- a/pkg/services/ngalert/accesscontrol/testing.go +++ b/pkg/services/ngalert/accesscontrol/testing.go @@ -3,8 +3,6 @@ package accesscontrol import ( "context" - openfgav1 "github.com/openfga/api/proto/openfga/v1" - "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/services/accesscontrol" ) @@ -38,11 +36,11 @@ func (a *recordingAccessControlFake) IsDisabled() bool { return a.Disabled } -func (a *recordingAccessControlFake) Check(ctx context.Context, in *openfgav1.CheckRequest) (*openfgav1.CheckResponse, error) { - return nil, nil +func (a *recordingAccessControlFake) Check(ctx context.Context, in accesscontrol.CheckRequest) (bool, error) { + return false, nil } -func (a *recordingAccessControlFake) ListObjects(ctx context.Context, in *openfgav1.ListObjectsRequest) (*openfgav1.ListObjectsResponse, error) { +func (a *recordingAccessControlFake) ListObjects(ctx context.Context, in accesscontrol.ListObjectsRequest) ([]string, error) { return nil, nil } diff --git a/pkg/services/ngalert/api/testing.go b/pkg/services/ngalert/api/testing.go index b85fcf10de6..4d50155254d 100644 --- a/pkg/services/ngalert/api/testing.go +++ b/pkg/services/ngalert/api/testing.go @@ -7,10 +7,9 @@ import ( "testing" "time" - openfgav1 "github.com/openfga/api/proto/openfga/v1" - "github.com/grafana/grafana-plugin-sdk-go/data" "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/services/accesscontrol" ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/ngalert/eval" "github.com/grafana/grafana/pkg/services/ngalert/models" @@ -134,11 +133,11 @@ func (a *recordingAccessControlFake) IsDisabled() bool { return a.Disabled } -func (a *recordingAccessControlFake) Check(ctx context.Context, in *openfgav1.CheckRequest) (*openfgav1.CheckResponse, error) { - return nil, nil +func (a *recordingAccessControlFake) Check(ctx context.Context, in accesscontrol.CheckRequest) (bool, error) { + return false, nil } -func (a *recordingAccessControlFake) ListObjects(ctx context.Context, in *openfgav1.ListObjectsRequest) (*openfgav1.ListObjectsResponse, error) { +func (a *recordingAccessControlFake) ListObjects(ctx context.Context, in accesscontrol.ListObjectsRequest) ([]string, error) { return nil, nil }