From 761090d96d0ab2d8fbd6dfca787d86596f03e24c Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Mon, 16 Oct 2023 11:19:46 -0500 Subject: [PATCH] use common plumbing --- internal/mesh/internal/types/xroute_test.go | 65 ++++----------------- internal/resource/resourcetest/acls.go | 53 ++++++++++++----- 2 files changed, 52 insertions(+), 66 deletions(-) diff --git a/internal/mesh/internal/types/xroute_test.go b/internal/mesh/internal/types/xroute_test.go index fcbd61dd3ad7..5ba39e24cc07 100644 --- a/internal/mesh/internal/types/xroute_test.go +++ b/internal/mesh/internal/types/xroute_test.go @@ -12,7 +12,6 @@ import ( "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/protobuf/types/known/wrapperspb" - "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/internal/resource/resourcetest" @@ -381,37 +380,12 @@ func testXRouteACLs[R XRouteData](t *testing.T, newRoute func(t *testing.T, pare return res } - type testcase struct { - res *pbresource.Resource - rules string - check func(t *testing.T, authz acl.Authorizer, res *pbresource.Resource) - readOK string - writeOK string - } - const ( - DENY = "deny" - ALLOW = "allow" - DEFAULT = "default" + DENY = resourcetest.DENY + ALLOW = resourcetest.ALLOW + DEFAULT = resourcetest.DEFAULT ) - checkF := func(t *testing.T, name string, expect string, got error) { - switch expect { - case ALLOW: - if acl.IsErrPermissionDenied(got) { - t.Fatal(name + " should be allowed") - } - case DENY: - if !acl.IsErrPermissionDenied(got) { - t.Fatal(name + " should be denied") - } - case DEFAULT: - require.Nil(t, got, name+" expected fallthrough decision") - default: - t.Fatalf(name+" unexpected expectation: %q", expect) - } - } - serviceRef := func(tenancy, name string) *pbresource.Reference { return newRefWithTenancy(pbcatalog.ServiceType, tenancy, name) } @@ -461,26 +435,9 @@ func testXRouteACLs[R XRouteData](t *testing.T, newRoute func(t *testing.T, pare ) } - run := func(t *testing.T, name string, tc testcase) { + run := func(t *testing.T, name string, tc resourcetest.ACLTestCase) { t.Run(name, func(t *testing.T) { - config := acl.Config{ - WildcardName: structs.WildcardSpecifier, - } - authz, err := acl.NewAuthorizerFromRules(tc.rules, &config, nil) - require.NoError(t, err) - authz = acl.NewChainedAuthorizer([]acl.Authorizer{authz, acl.DenyAll()}) - - reg, ok := registry.Resolve(tc.res.Id.GetType()) - require.True(t, ok) - - authCtx := &acl.AuthorizerContext{} // unused - - err = reg.ACLs.Read(authz, authCtx, tc.res.Id, nil) - require.ErrorIs(t, err, resource.ErrNeedResource, "read hook should require the data payload") - - checkF(t, "read", tc.readOK, reg.ACLs.Read(authz, authCtx, tc.res.Id, tc.res)) - checkF(t, "write", tc.writeOK, reg.ACLs.Write(authz, authCtx, tc.res)) - checkF(t, "list", DEFAULT, reg.ACLs.List(authz, authCtx)) + resourcetest.RunACLTestCase(t, tc, registry) }) } @@ -500,11 +457,13 @@ func testXRouteACLs[R XRouteData](t *testing.T, newRoute func(t *testing.T, pare } assert := func(t *testing.T, name string, rules string, res *pbresource.Resource, readOK, writeOK string) { - tc := testcase{ - res: res, - rules: rules, - readOK: readOK, - writeOK: writeOK, + tc := resourcetest.ACLTestCase{ + Rules: rules, + Res: res, + ReadOK: readOK, + WriteOK: writeOK, + ListOK: DEFAULT, + ReadHookRequiresResource: true, } run(t, name, tc) } diff --git a/internal/resource/resourcetest/acls.go b/internal/resource/resourcetest/acls.go index 4aff9e30327b..4ce8cc9d7ae0 100644 --- a/internal/resource/resourcetest/acls.go +++ b/internal/resource/resourcetest/acls.go @@ -39,27 +39,49 @@ var checkF = func(t *testing.T, expect string, got error) { } type ACLTestCase struct { - Rules string - Data protoreflect.ProtoMessage - Owner *pbresource.ID - Typ *pbresource.Type + Rules string + + // One of either Res or Data/Owner/Typ should be set. + Res *pbresource.Resource + Data protoreflect.ProtoMessage + Owner *pbresource.ID + Typ *pbresource.Type + ReadOK string WriteOK string ListOK string + + ReadHookRequiresResource bool } func RunACLTestCase(t *testing.T, tc ACLTestCase, registry resource.Registry) { - reg, ok := registry.Resolve(tc.Typ) - require.True(t, ok) + var ( + typ *pbresource.Type + res *pbresource.Resource + ) + if tc.Res != nil { + require.Nil(t, tc.Data) + require.Nil(t, tc.Owner) + require.Nil(t, tc.Typ) + typ = tc.Res.Id.GetType() + res = tc.Res + } else { + require.NotNil(t, tc.Data) + require.NotNil(t, tc.Typ) + typ = tc.Typ - resolvedType, ok := registry.Resolve(tc.Typ) - require.True(t, ok) + resolvedType, ok := registry.Resolve(typ) + require.True(t, ok) - res := Resource(tc.Typ, "test"). - WithTenancy(DefaultTenancyForType(t, resolvedType)). - WithOwner(tc.Owner). - WithData(t, tc.Data). - Build() + res = Resource(tc.Typ, "test"). + WithTenancy(DefaultTenancyForType(t, resolvedType)). + WithOwner(tc.Owner). + WithData(t, tc.Data). + Build() + } + + reg, ok := registry.Resolve(typ) + require.True(t, ok) ValidateAndNormalize(t, registry, res) @@ -70,6 +92,11 @@ func RunACLTestCase(t *testing.T, tc ACLTestCase, registry resource.Registry) { require.NoError(t, err) authz = acl.NewChainedAuthorizer([]acl.Authorizer{authz, acl.DenyAll()}) + if tc.ReadHookRequiresResource { + err = reg.ACLs.Read(authz, &acl.AuthorizerContext{}, res.Id, nil) + require.ErrorIs(t, err, resource.ErrNeedResource, "read hook should require the data payload") + } + t.Run("read", func(t *testing.T) { err := reg.ACLs.Read(authz, &acl.AuthorizerContext{}, res.Id, res) checkF(t, tc.ReadOK, err)