Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-vj4m-83m8-xpw5
* fix: return error if wildcard is encountered in ttu evaluation

* chore: address one of maria's comments

* chore: fix public facing error message and test name

* fix: return error if '*' encountered in tupleset relation in Expand
  • Loading branch information
jon-whit committed Oct 20, 2022
1 parent a7bd2e6 commit b466769
Show file tree
Hide file tree
Showing 5 changed files with 196 additions and 19 deletions.
24 changes: 22 additions & 2 deletions server/commands/check.go
Expand Up @@ -2,6 +2,7 @@ package commands

import (
"context"
"fmt"
"sync"

"github.com/go-errors/errors"
Expand All @@ -21,7 +22,7 @@ import (
"golang.org/x/sync/errgroup"
)

const AllUsers = "*"
const Wildcard = "*"

// A CheckQuery can be used to Check if a User has a Relation to an Object
// CheckQuery instances may be safely shared by multiple go-routines
Expand Down Expand Up @@ -401,7 +402,9 @@ func (query *CheckQuery) resolveTupleToUserset(ctx context.Context, rc *resoluti
if relation == "" {
relation = rc.tk.GetRelation()
}
findTK := &openfgapb.TupleKey{Object: rc.tk.GetObject(), Relation: relation}

findTK := tupleUtils.NewTupleKey(rc.tk.GetObject(), relation, "")

tracer := rc.tracer.AppendTupleToUserset().AppendString(tupleUtils.ToObjectRelationString(findTK.GetObject(), relation))
iter, err := rc.read(ctx, query.datastore, findTK)
if err != nil {
Expand Down Expand Up @@ -429,6 +432,23 @@ func (query *CheckQuery) resolveTupleToUserset(ctx context.Context, rc *resoluti

userObj, userRel := tupleUtils.SplitObjectRelation(tuple.GetUser())

if userObj == Wildcard {
objectType, _ := tupleUtils.SplitObject(rc.tk.GetObject())

query.logger.WarnWithContext(
ctx,
fmt.Sprintf("unexpected wildcard evaluated on tupleset relation '%s'", relation),
zap.String("store_id", rc.store),
zap.String("authorization_model_id", rc.modelID),
zap.String("object_type", objectType),
)

return serverErrors.InvalidTuple(
fmt.Sprintf("unexpected wildcard evaluated on relation '%s#%s'", objectType, relation),
tupleUtils.NewTupleKey(rc.tk.GetObject(), relation, Wildcard),
)
}

if !tupleUtils.IsValidObject(userObj) {
continue // TupleToUserset tuplesets should be of the form 'objectType:id' or 'objectType:id#relation' but are not guaranteed to be because it is neither a user or userset
}
Expand Down
2 changes: 1 addition & 1 deletion server/commands/check_utils.go
Expand Up @@ -195,7 +195,7 @@ func (u *userSet) Get(value string) (resolutionTracer, bool) {
var found bool
var rt resolutionTracer
if rt, found = u.u[value]; !found {
if rt, found = u.u[AllUsers]; !found {
if rt, found = u.u[Wildcard]; !found {
return nil, false
}
}
Expand Down
52 changes: 47 additions & 5 deletions server/commands/expand.go
Expand Up @@ -2,6 +2,7 @@ package commands

import (
"context"
"fmt"

"github.com/openfga/openfga/pkg/logger"
tupleUtils "github.com/openfga/openfga/pkg/tuple"
Expand All @@ -11,6 +12,7 @@ import (
"github.com/openfga/openfga/storage"
openfgapb "go.buf.build/openfga/go/openfga/api/openfga/v1"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"
"golang.org/x/sync/errgroup"
)

Expand Down Expand Up @@ -58,7 +60,13 @@ func (query *ExpandQuery) Execute(ctx context.Context, req *openfgapb.ExpandRequ
}, nil
}

func (query *ExpandQuery) resolveUserset(ctx context.Context, store, modelID string, userset *openfgapb.Userset, tk *openfgapb.TupleKey, metadata *utils.ResolutionMetadata) (*openfgapb.UsersetTree_Node, error) {
func (query *ExpandQuery) resolveUserset(
ctx context.Context,
store, modelID string,
userset *openfgapb.Userset,
tk *openfgapb.TupleKey,
metadata *utils.ResolutionMetadata,
) (*openfgapb.UsersetTree_Node, error) {
ctx, span := query.tracer.Start(ctx, "resolveUserset")
defer span.End()

Expand All @@ -68,7 +76,7 @@ func (query *ExpandQuery) resolveUserset(ctx context.Context, store, modelID str
case *openfgapb.Userset_ComputedUserset:
return query.resolveComputedUserset(ctx, us.ComputedUserset, tk, metadata)
case *openfgapb.Userset_TupleToUserset:
return query.resolveTupleToUserset(ctx, store, us.TupleToUserset, tk, metadata)
return query.resolveTupleToUserset(ctx, store, modelID, us.TupleToUserset, tk, metadata)
case *openfgapb.Userset_Union:
return query.resolveUnionUserset(ctx, store, modelID, us.Union, tk, metadata)
case *openfgapb.Userset_Difference:
Expand Down Expand Up @@ -152,14 +160,24 @@ func (query *ExpandQuery) resolveComputedUserset(ctx context.Context, userset *o
}

// resolveTupleToUserset creates a new leaf node containing the result of expanding a TupleToUserset rewrite.
func (query *ExpandQuery) resolveTupleToUserset(ctx context.Context, store string, userset *openfgapb.TupleToUserset, tk *openfgapb.TupleKey, metadata *utils.ResolutionMetadata) (*openfgapb.UsersetTree_Node, error) {
func (query *ExpandQuery) resolveTupleToUserset(
ctx context.Context,
store, modelID string,
userset *openfgapb.TupleToUserset,
tk *openfgapb.TupleKey,
metadata *utils.ResolutionMetadata,
) (*openfgapb.UsersetTree_Node, error) {
ctx, span := query.tracer.Start(ctx, "resolveTupleToUserset")
defer span.End()

targetObject := tk.GetObject()

tupleset := userset.GetTupleset().GetRelation()
tsKey := &openfgapb.TupleKey{
Object: tk.GetObject(),
Relation: userset.GetTupleset().GetRelation(),
Object: targetObject,
Relation: tupleset,
}

if tsKey.GetRelation() == "" {
tsKey.Relation = tk.GetRelation()
}
Expand All @@ -170,6 +188,7 @@ func (query *ExpandQuery) resolveTupleToUserset(ctx context.Context, store strin
return nil, serverErrors.HandleError("", err)
}
defer iter.Stop()

var computed []*openfgapb.UsersetTree_Computed
seen := make(map[string]bool)
for {
Expand All @@ -181,29 +200,52 @@ func (query *ExpandQuery) resolveTupleToUserset(ctx context.Context, store strin
return nil, serverErrors.HandleError("", err)
}
user := tuple.GetKey().GetUser()

if user == Wildcard {
objectType, _ := tupleUtils.SplitObject(targetObject)

query.logger.WarnWithContext(
ctx,
fmt.Sprintf("unexpected wildcard evaluated on tupleset relation '%s'", tupleset),
zap.String("store_id", store),
zap.String("authorization_model_id", modelID),
zap.String("object_type", objectType),
)

return nil, serverErrors.InvalidTuple(
fmt.Sprintf("unexpected wildcard evaluated on relation '%s#%s'", objectType, tupleset),
tupleUtils.NewTupleKey(targetObject, tupleset, Wildcard),
)
}

// user must contain a type (i.e., be an object or userset)
if tupleUtils.GetType(user) == "" {
continue
}

tObject, tRelation := tupleUtils.SplitObjectRelation(user)
// We only proceed in the case that tRelation == userset.GetComputedUserset().GetRelation().
// tRelation may be empty, and in this case, we set it to userset.GetComputedUserset().GetRelation().
if tRelation == "" {
tRelation = userset.GetComputedUserset().GetRelation()
}

if tRelation != userset.GetComputedUserset().GetRelation() {
continue
}

cs := &openfgapb.TupleKey{
Object: tObject,
Relation: tRelation,
}

computedRelation := toObjectRelation(cs)
if !seen[computedRelation] {
computed = append(computed, &openfgapb.UsersetTree_Computed{Userset: computedRelation})
seen[computedRelation] = true
}
}

return &openfgapb.UsersetTree_Node{
Name: toObjectRelation(tk),
Value: &openfgapb.UsersetTree_Node_Leaf{
Expand Down
106 changes: 99 additions & 7 deletions server/test/check.go
Expand Up @@ -1167,6 +1167,104 @@ func TestCheckQuery(t *testing.T, datastore storage.OpenFGADatastore) {
Allowed: true,
},
},
{
name: "Error if * encountered in TupleToUserset evaluation",
resolveNodeLimit: defaultResolveNodeLimit,
request: &openfgapb.CheckRequest{
TupleKey: tuple.NewTupleKey("document:doc1", "viewer", "user:anne"),
},
typeDefinitions: []*openfgapb.TypeDefinition{
{
Type: "document",
Relations: map[string]*openfgapb.Userset{
"parent": typesystem.This(),
"viewer": typesystem.TupleToUserset("parent", "viewer"),
},
Metadata: &openfgapb.Metadata{
Relations: map[string]*openfgapb.RelationMetadata{
"parent": {
DirectlyRelatedUserTypes: []*openfgapb.RelationReference{
typesystem.RelationReference("folder", ""),
},
},
},
},
},
{
Type: "folder",
Relations: map[string]*openfgapb.Userset{
"viewer": typesystem.This(),
},
Metadata: &openfgapb.Metadata{
Relations: map[string]*openfgapb.RelationMetadata{
"viewer": {
DirectlyRelatedUserTypes: []*openfgapb.RelationReference{
typesystem.RelationReference("user", ""),
},
},
},
},
},
},
tuples: []*openfgapb.TupleKey{
tuple.NewTupleKey("document:doc1", "parent", "*"), // wildcard not allowed on tupleset relations
tuple.NewTupleKey("folder:folder1", "viewer", "user:anne"),
},
err: serverErrors.InvalidTuple(
"unexpected wildcard evaluated on relation 'document#parent'",
tuple.NewTupleKey("document:doc1", "parent", commands.Wildcard),
),
},
{
name: "Error if * encountered in TTU evaluation including ContextualTuples",
resolveNodeLimit: defaultResolveNodeLimit,
request: &openfgapb.CheckRequest{
TupleKey: tuple.NewTupleKey("document:doc1", "viewer", "user:anne"),
ContextualTuples: &openfgapb.ContextualTupleKeys{
TupleKeys: []*openfgapb.TupleKey{
tuple.NewTupleKey("document:doc1", "parent", "*"), // wildcard not allowed on tupleset relations
tuple.NewTupleKey("folder:folder1", "viewer", "user:anne"),
},
},
},
typeDefinitions: []*openfgapb.TypeDefinition{
{
Type: "document",
Relations: map[string]*openfgapb.Userset{
"parent": typesystem.This(),
"viewer": typesystem.TupleToUserset("parent", "viewer"),
},
Metadata: &openfgapb.Metadata{
Relations: map[string]*openfgapb.RelationMetadata{
"parent": {
DirectlyRelatedUserTypes: []*openfgapb.RelationReference{
typesystem.RelationReference("folder", ""),
},
},
},
},
},
{
Type: "folder",
Relations: map[string]*openfgapb.Userset{
"viewer": typesystem.This(),
},
Metadata: &openfgapb.Metadata{
Relations: map[string]*openfgapb.RelationMetadata{
"viewer": {
DirectlyRelatedUserTypes: []*openfgapb.RelationReference{
typesystem.RelationReference("user", ""),
},
},
},
},
},
},
err: serverErrors.InvalidTuple(
"unexpected wildcard evaluated on relation 'document#parent'",
tuple.NewTupleKey("document:doc1", "parent", commands.Wildcard),
),
},
}

ctx := context.Background()
Expand Down Expand Up @@ -1196,13 +1294,7 @@ func TestCheckQuery(t *testing.T, datastore storage.OpenFGADatastore) {
test.request.AuthorizationModelId = model.Id
resp, gotErr := cmd.Execute(ctx, test.request)

if test.err == nil {
require.NoError(t, gotErr)
}

if test.err != nil {
require.EqualError(t, test.err, gotErr.Error())
}
require.ErrorIs(t, gotErr, test.err)

if test.response != nil {
require.NoError(t, gotErr)
Expand Down
31 changes: 27 additions & 4 deletions server/test/expand.go
Expand Up @@ -4,12 +4,12 @@ import (
"context"
"testing"

"github.com/go-errors/errors"
"github.com/google/go-cmp/cmp"
"github.com/openfga/openfga/pkg/id"
"github.com/openfga/openfga/pkg/logger"
"github.com/openfga/openfga/pkg/telemetry"
"github.com/openfga/openfga/pkg/testutils"
"github.com/openfga/openfga/pkg/tuple"
"github.com/openfga/openfga/pkg/typesystem"
"github.com/openfga/openfga/server/commands"
serverErrors "github.com/openfga/openfga/server/errors"
Expand Down Expand Up @@ -811,6 +811,31 @@ func TestExpandQueryErrors(t *testing.T, datastore storage.OpenFGADatastore) {
Relation: "baz",
}),
},
{
name: "TupleToUserset involving wildcard returns error",
typeDefinitions: []*openfgapb.TypeDefinition{
{
Type: "document",
Relations: map[string]*openfgapb.Userset{
"parent": typesystem.This(),
"viewer": typesystem.Union(
typesystem.This(), typesystem.TupleToUserset("parent", "viewer"),
),
},
},
},
tuples: []*openfgapb.TupleKey{
tuple.NewTupleKey("document:1", "parent", "*"),
tuple.NewTupleKey("document:X", "viewer", "jon"),
},
request: &openfgapb.ExpandRequest{
TupleKey: tuple.NewTupleKey("document:1", "viewer", ""),
},
expected: serverErrors.InvalidTuple(
"unexpected wildcard evaluated on relation 'document#parent'",
tuple.NewTupleKey("document:1", "parent", "*"),
),
},
}

require := require.New(t)
Expand All @@ -830,9 +855,7 @@ func TestExpandQueryErrors(t *testing.T, datastore storage.OpenFGADatastore) {
test.request.AuthorizationModelId = modelID

_, err = query.Execute(ctx, test.request)
if !errors.Is(err, test.expected) {
t.Fatalf("'%s': Execute(), err = %v, want %v", test.name, err, test.expected)
}
require.ErrorIs(err, test.expected)
})
}
}

0 comments on commit b466769

Please sign in to comment.