From 5cf86c981fe6cadb67c19b56f6c6443c3561d769 Mon Sep 17 00:00:00 2001 From: Leonor Oliveira <9090754+leonorfmartins@users.noreply.github.com> Date: Mon, 28 Oct 2024 11:00:19 +0100 Subject: [PATCH] Test Folder's GetAuthorizer (#95266) * WIP: setup to test folders GetAuthorizer * Setup test * Extract authorizer fn for tests * Setup internal test fn * Better define test inputs * Add FolderAPI builder to the test * First test passing * Test getAuthorize for the create method * Change authorizerFunc's signature * [REVIEW] code readability * Name error * [REVIEW] add one more test case. Lint * Remove empty line --- pkg/registry/apis/folders/register.go | 85 +++++++++------ pkg/registry/apis/folders/register_test.go | 116 +++++++++++++++++++++ 2 files changed, 169 insertions(+), 32 deletions(-) create mode 100644 pkg/registry/apis/folders/register_test.go diff --git a/pkg/registry/apis/folders/register.go b/pkg/registry/apis/folders/register.go index 10239db549e..7ce913883fe 100644 --- a/pkg/registry/apis/folders/register.go +++ b/pkg/registry/apis/folders/register.go @@ -2,6 +2,7 @@ package folders import ( "context" + "errors" "github.com/prometheus/client_golang/prometheus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -30,6 +31,9 @@ var _ builder.APIGroupBuilder = (*FolderAPIBuilder)(nil) var resourceInfo = v0alpha1.FolderResourceInfo +var errNoUser = errors.New("valid user is required") +var errNoResource = errors.New("resource name is required") + // This is used just so wire has something unique to return type FolderAPIBuilder struct { gv schema.GroupVersion @@ -154,42 +158,59 @@ func (b *FolderAPIBuilder) PostProcessOpenAPI(oas *spec3.OpenAPI) (*spec3.OpenAP return oas, nil } +type authorizerParams struct { + user identity.Requester + evaluator accesscontrol.Evaluator +} + func (b *FolderAPIBuilder) GetAuthorizer() authorizer.Authorizer { - return authorizer.AuthorizerFunc( - func(ctx context.Context, attr authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { - verb := attr.GetVerb() - name := attr.GetName() - if (!attr.IsResourceRequest()) || (name == "" && verb != utils.VerbCreate) { - return authorizer.DecisionNoOpinion, "", nil + return authorizer.AuthorizerFunc(func(ctx context.Context, attr authorizer.Attributes) (authorizer.Decision, string, error) { + in, err := authorizerFunc(ctx, attr) + if err != nil { + if errors.Is(err, errNoUser) { + return authorizer.DecisionDeny, "", nil } + return authorizer.DecisionNoOpinion, "", nil + } + + ok, err := b.accessControl.Evaluate(ctx, in.user, in.evaluator) + if ok { + return authorizer.DecisionAllow, "", nil + } + return authorizer.DecisionDeny, "folder", err + }) +} - // require a user - user, err := identity.GetRequester(ctx) - if err != nil { - return authorizer.DecisionDeny, "valid user is required", err - } +func authorizerFunc(ctx context.Context, attr authorizer.Attributes) (*authorizerParams, error) { + verb := attr.GetVerb() + name := attr.GetName() + if (!attr.IsResourceRequest()) || (name == "" && verb != utils.VerbCreate) { + return nil, errNoResource + } - scope := dashboards.ScopeFoldersProvider.GetResourceScopeUID(name) - eval := accesscontrol.EvalPermission(dashboards.ActionFoldersRead, scope) + // require a user + user, err := identity.GetRequester(ctx) + if err != nil { + return nil, errNoUser + } - // "get" is used for sub-resources with GET http (parents, access, count) - switch verb { - case utils.VerbCreate: - eval = accesscontrol.EvalPermission(dashboards.ActionFoldersCreate) - case utils.VerbPatch: - fallthrough - case utils.VerbUpdate: - eval = accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, scope) - case utils.VerbDeleteCollection: - fallthrough - case utils.VerbDelete: - eval = accesscontrol.EvalPermission(dashboards.ActionFoldersDelete, scope) - } + scope := dashboards.ScopeFoldersProvider.GetResourceScopeUID(name) + var eval accesscontrol.Evaluator - ok, err := b.accessControl.Evaluate(ctx, user, eval) - if ok { - return authorizer.DecisionAllow, "", nil - } - return authorizer.DecisionDeny, "folder", err - }) + // "get" is used for sub-resources with GET http (parents, access, count) + switch verb { + case utils.VerbCreate: + eval = accesscontrol.EvalPermission(dashboards.ActionFoldersCreate) + case utils.VerbPatch: + fallthrough + case utils.VerbUpdate: + eval = accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, scope) + case utils.VerbDeleteCollection: + fallthrough + case utils.VerbDelete: + eval = accesscontrol.EvalPermission(dashboards.ActionFoldersDelete, scope) + default: + eval = accesscontrol.EvalPermission(dashboards.ActionFoldersRead, scope) + } + return &authorizerParams{evaluator: eval, user: user}, nil } diff --git a/pkg/registry/apis/folders/register_test.go b/pkg/registry/apis/folders/register_test.go new file mode 100644 index 00000000000..068bfafdce5 --- /dev/null +++ b/pkg/registry/apis/folders/register_test.go @@ -0,0 +1,116 @@ +package folders + +import ( + "context" + "testing" + + "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/apimachinery/utils" + "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" + "github.com/grafana/grafana/pkg/services/authz/zanzana" + "github.com/grafana/grafana/pkg/services/dashboards" + "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/folder/foldertest" + "github.com/grafana/grafana/pkg/services/user" + "github.com/stretchr/testify/require" + "k8s.io/apiserver/pkg/authorization/authorizer" +) + +func TestFolderAPIBuilder_getAuthorizerFunc(t *testing.T) { + type input struct { + user identity.Requester + verb string + } + type expect struct { + eval string + allow bool + err error + } + var orgID int64 = 1 + + tests := []struct { + name string + input input + expect expect + }{ + { + name: "user with create permissions should be able to create a folder", + input: input{ + user: &user.SignedInUser{ + UserID: 1, + OrgID: orgID, + Name: "123", + Permissions: map[int64]map[string][]string{ + orgID: {dashboards.ActionFoldersCreate: {}, dashboards.ActionFoldersWrite: {dashboards.ScopeFoldersAll}}, + }, + }, + verb: string(utils.VerbCreate), + }, + expect: expect{ + eval: "folders:create", + allow: true, + }, + }, + { + name: "not possible to create a folder without a user", + input: input{ + user: nil, + verb: string(utils.VerbCreate), + }, + expect: expect{ + eval: "folders:create", + err: errNoUser, + }, + }, + { + name: "user without permissions should not be able to create a folder", + input: input{ + user: &user.SignedInUser{}, + verb: string(utils.VerbCreate), + }, + expect: expect{ + eval: "folders:create", + }, + }, + { + name: "user in another orgId should not be able to create a folder ", + input: input{ + user: &user.SignedInUser{ + UserID: 1, + OrgID: 2, + Name: "123", + Permissions: map[int64]map[string][]string{ + orgID: {dashboards.ActionFoldersCreate: {}, dashboards.ActionFoldersWrite: {dashboards.ScopeFoldersAll}}, + }, + }, + verb: string(utils.VerbCreate), + }, + expect: expect{ + eval: "folders:create", + }, + }, + } + + b := &FolderAPIBuilder{ + gv: resourceInfo.GroupVersion(), + features: nil, + namespacer: func(_ int64) string { return "123" }, + folderSvc: foldertest.NewFakeService(), + accessControl: acimpl.ProvideAccessControl(featuremgmt.WithFeatures("nestedFolders"), zanzana.NewNoopClient()), + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + out, err := authorizerFunc(identity.WithRequester(ctx, tt.input.user), authorizer.AttributesRecord{User: tt.input.user, Verb: tt.input.verb, Resource: "folders", ResourceRequest: true, Name: "123"}) + if tt.expect.err != nil { + require.Error(t, err) + return + } + allow, _ := b.accessControl.Evaluate(ctx, out.user, out.evaluator) + require.NoError(t, err) + require.Equal(t, tt.expect.eval, out.evaluator.String()) + require.Equal(t, tt.expect.allow, allow) + }) + } +}