Merge pull request #1928 from josephschorr/wildcard-error

Return a proper error code if a wildcard subject is specified
authzed · Jun 10, 2024 · d11400f · d11400f
2 parents da3cde2 + 79a71db
commit d11400f
Show file tree

Hide file tree

Showing 10 changed files with 55 additions and 22 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)