From 79a71dbfb74fa61f93c4eb6f180bc8ae05945e67 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 7 Jun 2024 16:25:07 -0400 Subject: [PATCH] Return a proper error code if a wildcard subject is specified Fixes #1925 --- e2e/go.mod | 2 +- e2e/go.sum | 4 +-- go.mod | 2 +- go.sum | 4 +-- internal/graph/check.go | 2 +- internal/graph/errors.go | 31 +++++++++++++++++------- internal/graph/lookupresources.go | 2 +- internal/services/shared/errors.go | 2 -- internal/services/v1/permissions_test.go | 25 +++++++++++++++++++ pkg/development/devcontext.go | 3 --- 10 files changed, 55 insertions(+), 22 deletions(-) diff --git a/e2e/go.mod b/e2e/go.mod index a36766419f..f47ef02b7a 100644 --- a/e2e/go.mod +++ b/e2e/go.mod @@ -3,7 +3,7 @@ module github.com/authzed/spicedb/e2e go 1.22.2 require ( - github.com/authzed/authzed-go v0.12.0 + github.com/authzed/authzed-go v0.12.1-0.20240607163830-a28f71a1b0e5 github.com/authzed/grpcutil v0.0.0-20240123092924-129dc0a6a6e1 github.com/authzed/spicedb v1.29.5 github.com/brianvoe/gofakeit/v6 v6.23.2 diff --git a/e2e/go.sum b/e2e/go.sum index f0ec548a23..aa2b4525b0 100644 --- a/e2e/go.sum +++ b/e2e/go.sum @@ -27,8 +27,8 @@ github.com/Masterminds/squirrel v1.5.4 h1:uUcX/aBc8O7Fg9kaISIUsHXdKuqehiXAMQTYX8 github.com/Masterminds/squirrel v1.5.4/go.mod h1:NNaOrjSoIDfDA40n7sr2tPNZRfjzjA400rg+riTZj10= github.com/antlr4-go/antlr/v4 v4.13.0 h1:lxCg3LAv+EUK6t1i0y1V6/SLeUi0eKEKdhQAlS8TVTI= github.com/antlr4-go/antlr/v4 v4.13.0/go.mod h1:pfChB/xh/Unjila75QW7+VU4TSnWnnk9UTnmpPaOR2g= -github.com/authzed/authzed-go v0.12.0 h1:pj/uvS9atMez360xPP1eE/qxUuH/+LQ8XqQDuJPeD9s= -github.com/authzed/authzed-go v0.12.0/go.mod h1:YhXiglakXojf4tqu0yr4vqd1151x1ES9iKczbsNlXZs= +github.com/authzed/authzed-go v0.12.1-0.20240607163830-a28f71a1b0e5 h1:4qSUTp6vH2GN0PGmTGB3jtnhUceyvr3QEKFNdT4QgqU= +github.com/authzed/authzed-go v0.12.1-0.20240607163830-a28f71a1b0e5/go.mod h1:7PFGe0szvxtIJgTB8zLkcNzT8ZBXf9w1rQjVYg4kREc= github.com/authzed/cel-go v0.20.2 h1:GlmLecGry7Z8HU0k+hmaHHUV05ZHrsFxduXHtIePvck= github.com/authzed/cel-go v0.20.2/go.mod h1:pJHVFWbqUHV1J+klQoZubdKswlbxcsbojda3mye9kiU= github.com/authzed/grpcutil v0.0.0-20240123092924-129dc0a6a6e1 h1:zBfQzia6Hz45pJBeURTrv1b6HezmejB6UmiGuBilHZM= diff --git a/go.mod b/go.mod index 6bd70a5254..02d9a45b4c 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,7 @@ require ( github.com/IBM/pgxpoolprometheus v1.1.1 github.com/KimMachineGun/automemlimit v0.6.1 github.com/Masterminds/squirrel v1.5.4 - github.com/authzed/authzed-go v0.12.0 + github.com/authzed/authzed-go v0.12.1-0.20240607163830-a28f71a1b0e5 // NOTE: We are using a *copy* of `cel-go` here to ensure there isn't a conflict // with the version used in Kubernetes. This is a temporary measure until we can diff --git a/go.sum b/go.sum index 9fc8e718a3..533e47d65d 100644 --- a/go.sum +++ b/go.sum @@ -696,8 +696,8 @@ github.com/ashanbrown/forbidigo v1.6.0 h1:D3aewfM37Yb3pxHujIPSpTf6oQk9sc9WZi8ger github.com/ashanbrown/forbidigo v1.6.0/go.mod h1:Y8j9jy9ZYAEHXdu723cUlraTqbzjKF1MUyfOKL+AjcU= github.com/ashanbrown/makezero v1.1.1 h1:iCQ87C0V0vSyO+M9E/FZYbu65auqH0lnsOkf5FcB28s= github.com/ashanbrown/makezero v1.1.1/go.mod h1:i1bJLCRSCHOcOa9Y6MyF2FTfMZMFdHvxKHxgO5Z1axI= -github.com/authzed/authzed-go v0.12.0 h1:pj/uvS9atMez360xPP1eE/qxUuH/+LQ8XqQDuJPeD9s= -github.com/authzed/authzed-go v0.12.0/go.mod h1:YhXiglakXojf4tqu0yr4vqd1151x1ES9iKczbsNlXZs= +github.com/authzed/authzed-go v0.12.1-0.20240607163830-a28f71a1b0e5 h1:4qSUTp6vH2GN0PGmTGB3jtnhUceyvr3QEKFNdT4QgqU= +github.com/authzed/authzed-go v0.12.1-0.20240607163830-a28f71a1b0e5/go.mod h1:7PFGe0szvxtIJgTB8zLkcNzT8ZBXf9w1rQjVYg4kREc= github.com/authzed/cel-go v0.20.2 h1:GlmLecGry7Z8HU0k+hmaHHUV05ZHrsFxduXHtIePvck= github.com/authzed/cel-go v0.20.2/go.mod h1:pJHVFWbqUHV1J+klQoZubdKswlbxcsbojda3mye9kiU= github.com/authzed/consistent v0.1.0 h1:tlh1wvKoRbjRhMm2P+X5WQQyR54SRoS4MyjLOg17Mp8= diff --git a/internal/graph/check.go b/internal/graph/check.go index b0bd89c1bb..807eb46521 100644 --- a/internal/graph/check.go +++ b/internal/graph/check.go @@ -156,7 +156,7 @@ func (cc *ConcurrentChecker) checkInternal(ctx context.Context, req ValidatedChe // Ensure that we are not performing a check for a wildcard as the subject. if req.Subject.ObjectId == tuple.PublicWildcard { - return checkResultError(NewErrInvalidArgument(errors.New("cannot perform check on wildcard")), emptyMetadata) + return checkResultError(NewWildcardNotAllowedErr("cannot perform check on wildcard subject", "subject.object_id"), emptyMetadata) } // Deduplicate any incoming resource IDs. diff --git a/internal/graph/errors.go b/internal/graph/errors.go index 25eac68b8c..d93909511a 100644 --- a/internal/graph/errors.go +++ b/internal/graph/errors.go @@ -141,20 +141,33 @@ func NewRelationMissingTypeInfoErr(nsName string, relationName string) error { } } -// ErrInvalidArgument occurs when a request sent has an invalid argument. -type ErrInvalidArgument struct { +// ErrWildcardNotAllowed occurs when a request sent has an invalid wildcard argument. +type ErrWildcardNotAllowed struct { error + + fieldName string } -// NewErrInvalidArgument constructs a request sent has an invalid argument. -func NewErrInvalidArgument(baseErr error) error { - return ErrInvalidArgument{ - error: baseErr, - } +// GRPCStatus implements retrieving the gRPC status for the error. +func (err ErrWildcardNotAllowed) GRPCStatus() *status.Status { + return spiceerrors.WithCodeAndDetails( + err, + codes.InvalidArgument, + spiceerrors.ForReason( + v1.ErrorReason_ERROR_REASON_WILDCARD_NOT_ALLOWED, + map[string]string{ + "field": err.fieldName, + }, + ), + ) } -func (e ErrInvalidArgument) Unwrap() error { - return e.error +// NewWildcardNotAllowedErr constructs an error indicating that a wildcard was not allowed. +func NewWildcardNotAllowedErr(message string, fieldName string) error { + return ErrWildcardNotAllowed{ + error: fmt.Errorf("invalid argument: %s", message), + fieldName: fieldName, + } } // ErrUnimplemented is returned when some functionality is not yet supported. diff --git a/internal/graph/lookupresources.go b/internal/graph/lookupresources.go index dfe4aac7da..fccfd7b8a1 100644 --- a/internal/graph/lookupresources.go +++ b/internal/graph/lookupresources.go @@ -36,7 +36,7 @@ func (cl *CursoredLookupResources) LookupResources( parentStream dispatch.LookupResourcesStream, ) error { if req.Subject.ObjectId == tuple.PublicWildcard { - return NewErrInvalidArgument(errors.New("cannot perform lookup resources on wildcard")) + return NewWildcardNotAllowedErr("cannot perform lookup resources on wildcard subject", "subject.object_id") } lookupContext := parentStream.Context() diff --git a/internal/services/shared/errors.go b/internal/services/shared/errors.go index efbbf56546..9eb0b1c4fc 100644 --- a/internal/services/shared/errors.go +++ b/internal/services/shared/errors.go @@ -176,8 +176,6 @@ func RewriteError(ctx context.Context, err error, config *ConfigForErrors) error case errors.As(err, &datastore.ErrCounterNotRegistered{}): return spiceerrors.WithCodeAndReason(err, codes.FailedPrecondition, v1.ErrorReason_ERROR_REASON_COUNTER_NOT_REGISTERED) - case errors.As(err, &graph.ErrInvalidArgument{}): - return status.Errorf(codes.InvalidArgument, "%s", err) case errors.As(err, &graph.ErrRelationMissingTypeInfo{}): return status.Errorf(codes.FailedPrecondition, "failed precondition: %s", err) case errors.As(err, &graph.ErrAlwaysFail{}): diff --git a/internal/services/v1/permissions_test.go b/internal/services/v1/permissions_test.go index 7b2fa5fb9d..b763ca2c5c 100644 --- a/internal/services/v1/permissions_test.go +++ b/internal/services/v1/permissions_test.go @@ -315,6 +315,31 @@ func TestCheckPermissions(t *testing.T) { } } +func TestCheckPermissionWithWildcardSubject(t *testing.T) { + require := require.New(t) + conn, cleanup, _, revision := testserver.NewTestServer(require, testTimedeltas[0], memdb.DisableGC, true, tf.StandardDatastoreWithData) + client := v1.NewPermissionsServiceClient(conn) + t.Cleanup(cleanup) + + ctx := context.Background() + ctx = requestmeta.AddRequestHeaders(ctx, requestmeta.RequestDebugInformation) + + _, err := client.CheckPermission(ctx, &v1.CheckPermissionRequest{ + Consistency: &v1.Consistency{ + Requirement: &v1.Consistency_AtLeastAsFresh{ + AtLeastAsFresh: zedtoken.MustNewFromRevision(revision), + }, + }, + Resource: obj("document", "masterplan"), + Permission: "view", + Subject: sub("user", "*", ""), + }) + + require.Error(err) + require.ErrorContains(err, "invalid argument: cannot perform check on wildcard subject") + grpcutil.RequireStatus(t, codes.InvalidArgument, err) +} + func TestCheckPermissionWithDebugInfo(t *testing.T) { require := require.New(t) conn, cleanup, _, revision := testserver.NewTestServer(require, testTimedeltas[0], memdb.DisableGC, true, tf.StandardDatastoreWithData) diff --git a/pkg/development/devcontext.go b/pkg/development/devcontext.go index c017420f6a..fcb93a7552 100644 --- a/pkg/development/devcontext.go +++ b/pkg/development/devcontext.go @@ -386,9 +386,6 @@ func rewriteACLError(ctx context.Context, err error) error { case errors.As(err, &relNotFoundError): fallthrough - case errors.As(err, &maingraph.ErrInvalidArgument{}): - return status.Errorf(codes.InvalidArgument, "%s", err) - case errors.As(err, &datastore.ErrInvalidRevision{}): return status.Errorf(codes.OutOfRange, "invalid zookie: %s", err)