Skip to content

Commit

Permalink
Add extended relationships filtering to read relationships and delete…
Browse files Browse the repository at this point in the history 
… relationships APIs
  • Loading branch information
@josephschorr
josephschorr committed Mar 6, 2024
1 parent 5f819f6 commit 627bb6a
Show file tree
Hide file tree
Showing 5 changed files with 215 additions and 18 deletions.
40 changes: 38 additions & 2 deletions internal/services/v1/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,21 @@ func NewPreconditionFailedErr(precondition *v1.Precondition) error {
// GRPCStatus implements retrieving the gRPC status for the error.
func (err ErrPreconditionFailed) GRPCStatus() *status.Status {
metadata := map[string]string{
"precondition_resource_type": err.precondition.Filter.ResourceType,
"precondition_operation": v1.Precondition_Operation_name[int32(err.precondition.Operation)],
"precondition_operation": v1.Precondition_Operation_name[int32(err.precondition.Operation)],
}

if err.precondition.Filter.ResourceType != "" {
metadata["precondition_resource_type"] = err.precondition.Filter.ResourceType
}

if err.precondition.Filter.OptionalResourceId != "" {
metadata["precondition_resource_id"] = err.precondition.Filter.OptionalResourceId
}

if err.precondition.Filter.OptionalResourceIdPrefix != "" {
metadata["precondition_resource_id_prefix"] = err.precondition.Filter.OptionalResourceIdPrefix
}

if err.precondition.Filter.OptionalRelation != "" {
metadata["precondition_relation"] = err.precondition.Filter.OptionalRelation
}
Expand Down Expand Up @@ -337,6 +344,34 @@ func (err ErrInvalidCursor) GRPCStatus() *status.Status {
)
}

// ErrInvalidFilter indicates the specified relationship filter was invalid.
type ErrInvalidFilter struct {
error
}

// GRPCStatus implements retrieving the gRPC status for the error.
func (err ErrInvalidFilter) GRPCStatus() *status.Status {
// TODO(jschorr): Put a proper error reason in here.
return spiceerrors.WithCodeAndDetails(
err,
codes.InvalidArgument,
spiceerrors.ForReason(
v1.ErrorReason_ERROR_REASON_UNSPECIFIED,
map[string]string{},
),
)
}

// NewInvalidFilterErr constructs a new invalid filter error.
func NewInvalidFilterErr(reason string) ErrInvalidFilter {
return ErrInvalidFilter{
error: fmt.Errorf(
"the relationship filter provided is not valid: %s", reason,
),
}
}

// NewEmptyPreconditionErr constructs a new empty precondition error.
func NewEmptyPreconditionErr() ErrEmptyPrecondition {
return ErrEmptyPrecondition{
error: fmt.Errorf(
Expand All @@ -352,6 +387,7 @@ type ErrEmptyPrecondition struct {

// GRPCStatus implements retrieving the gRPC status for the error.
func (err ErrEmptyPrecondition) GRPCStatus() *status.Status {
// TODO(jschorr): Put a proper error reason in here.
return spiceerrors.WithCodeAndDetails(
err,
codes.InvalidArgument,
Expand Down
4 changes: 2 additions & 2 deletions internal/services/v1/preconditions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ func checkPreconditions(
preconditions []*v1.Precondition,
) error {
for _, precond := range preconditions {
filter, err := datastore.RelationshipsFilterFromPublicFilter(precond.Filter)
dsFilter, err := datastore.RelationshipsFilterFromPublicFilter(precond.Filter)
if err != nil {
return fmt.Errorf("error converting filter: %w", err)
}

iter, err := rwt.QueryRelationships(ctx, filter, options.WithLimit(&limitOne))
iter, err := rwt.QueryRelationships(ctx, dsFilter, options.WithLimit(&limitOne))
if err != nil {
return fmt.Errorf("error reading relationships: %w", err)
}
Expand Down
20 changes: 20 additions & 0 deletions internal/services/v1/preconditions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ var companyPlanFolder = &v1.RelationshipFilter{
},
}

var prefixMatch = &v1.RelationshipFilter{
OptionalResourceIdPrefix: "c",
}

var prefixNoMatch = &v1.RelationshipFilter{
OptionalResourceIdPrefix: "zzz",
}

func TestPreconditions(t *testing.T) {
require := require.New(t)
uninitialized, err := memdb.NewMemdbDatastore(0, 0, memdb.DisableGC)
Expand All @@ -43,6 +51,18 @@ func TestPreconditions(t *testing.T) {
Filter: companyPlanFolder,
},
}))
require.NoError(checkPreconditions(ctx, rwt, []*v1.Precondition{
{
Operation: v1.Precondition_OPERATION_MUST_MATCH,
Filter: prefixMatch,
},
}))
require.Error(checkPreconditions(ctx, rwt, []*v1.Precondition{
{
Operation: v1.Precondition_OPERATION_MUST_MATCH,
Filter: prefixNoMatch,
},
}))
return nil
})
require.NoError(err)
Expand Down
40 changes: 27 additions & 13 deletions internal/services/v1/relationships.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,15 @@ func (ps *permissionServer) checkFilterComponent(ctx context.Context, objectType
return namespace.CheckNamespaceAndRelation(ctx, objectType, relationToTest, allowEllipsis, ds)
}

func (ps *permissionServer) checkFilterNamespaces(ctx context.Context, filter *v1.RelationshipFilter, ds datastore.Reader) error {
if err := ps.checkFilterComponent(ctx, filter.ResourceType, filter.OptionalRelation, ds); err != nil {
return err
func (ps *permissionServer) validateRelationshipsFilter(ctx context.Context, filter *v1.RelationshipFilter, ds datastore.Reader) error {
// ResourceType is optional, so only check the relation if it is specified.
if filter.ResourceType != "" {
if err := ps.checkFilterComponent(ctx, filter.ResourceType, filter.OptionalRelation, ds); err != nil {
return err
}
}

// SubjectFilter is optional, so only check if it is specified.
if subjectFilter := filter.OptionalSubjectFilter; subjectFilter != nil {
subjectRelation := ""
if subjectFilter.OptionalRelation != nil {
Expand All @@ -138,6 +142,20 @@ func (ps *permissionServer) checkFilterNamespaces(ctx context.Context, filter *v
}
}

// Ensure the resource ID and the resource ID prefix are not set at the same time.
if filter.OptionalResourceId != "" && filter.OptionalResourceIdPrefix != "" {
return NewInvalidFilterErr("resource_id and resource_id_prefix cannot be set at the same time")
}

// Ensure that at least one field is set.
if filter.ResourceType == "" &&
filter.OptionalResourceId == "" &&
filter.OptionalResourceIdPrefix == "" &&
filter.OptionalRelation == "" &&
filter.OptionalSubjectFilter == nil {
return NewInvalidFilterErr("at least one field must be set")
}

return nil
}

Expand All @@ -150,7 +168,7 @@ func (ps *permissionServer) ReadRelationships(req *v1.ReadRelationshipsRequest,

ds := datastoremw.MustFromContext(ctx).SnapshotReader(atRevision)

if err := ps.checkFilterNamespaces(ctx, req.RelationshipFilter, ds); err != nil {
if err := ps.validateRelationshipsFilter(ctx, req.RelationshipFilter, ds); err != nil {
return ps.rewriteError(ctx, err)
}

Expand Down Expand Up @@ -192,15 +210,15 @@ func (ps *permissionServer) ReadRelationships(req *v1.ReadRelationshipsRequest,
}
}

filter, err := datastore.RelationshipsFilterFromPublicFilter(req.RelationshipFilter)
dsFilter, err := datastore.RelationshipsFilterFromPublicFilter(req.RelationshipFilter)
if err != nil {
return ps.rewriteError(ctx, err)
return ps.rewriteError(ctx, fmt.Errorf("error filtering: %w", err))
}

tupleIterator, err := pagination.NewPaginatedIterator(
ctx,
ds,
filter,
dsFilter,
pageSize,
options.ByResource,
startCursor,
Expand Down Expand Up @@ -301,11 +319,7 @@ func (ps *permissionServer) WriteRelationships(ctx context.Context, req *v1.Writ

// Validate the preconditions.
for _, precond := range req.OptionalPreconditions {
if err := ps.validatePrecondition(precond); err != nil {
return err
}

if err := ps.checkFilterNamespaces(ctx, precond.Filter, rwt); err != nil {
if err := ps.validateRelationshipsFilter(ctx, precond.Filter, rwt); err != nil {
return err
}
}
Expand Down Expand Up @@ -366,7 +380,7 @@ func (ps *permissionServer) DeleteRelationships(ctx context.Context, req *v1.Del
deletionProgress := v1.DeleteRelationshipsResponse_DELETION_PROGRESS_COMPLETE

revision, err := ds.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error {
if err := ps.checkFilterNamespaces(ctx, req.RelationshipFilter, rwt); err != nil {
if err := ps.validateRelationshipsFilter(ctx, req.RelationshipFilter, rwt); err != nil {
return err
}

Expand Down
129 changes: 128 additions & 1 deletion internal/services/v1/relationships_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/authzed/spicedb/internal/datastore/memdb"
tf "github.com/authzed/spicedb/internal/testfixtures"
"github.com/authzed/spicedb/internal/testserver"
"github.com/authzed/spicedb/pkg/datastore"
core "github.com/authzed/spicedb/pkg/proto/core/v1"
"github.com/authzed/spicedb/pkg/spiceerrors"
"github.com/authzed/spicedb/pkg/tuple"
Expand Down Expand Up @@ -78,6 +79,68 @@ func TestReadRelationships(t *testing.T) {
"document:healthplan#parent@folder:plans": {},
},
},
{
"resource id prefix and resource id",
&v1.RelationshipFilter{
OptionalResourceId: "master",
OptionalResourceIdPrefix: "master",
OptionalRelation: "parent",
},
codes.InvalidArgument,
nil,
},
{
"just relation",
&v1.RelationshipFilter{
OptionalRelation: "parent",
},
codes.OK,
map[string]struct{}{
"document:companyplan#parent@folder:company": {},
"document:masterplan#parent@folder:strategy": {},
"document:masterplan#parent@folder:plans": {},
"document:healthplan#parent@folder:plans": {},
"folder:strategy#parent@folder:company": {},
},
},
{
"just resource ID",
&v1.RelationshipFilter{
OptionalResourceId: "masterplan",
},
codes.OK,
map[string]struct{}{
"document:masterplan#parent@folder:strategy": {},
"document:masterplan#parent@folder:plans": {},
"document:masterplan#owner@user:product_manager": {},
"document:masterplan#viewer@user:eng_lead": {},
},
},
{
"just resource ID prefix",
&v1.RelationshipFilter{
OptionalResourceIdPrefix: "masterpl",
},
codes.OK,
map[string]struct{}{
"document:masterplan#parent@folder:strategy": {},
"document:masterplan#parent@folder:plans": {},
"document:masterplan#owner@user:product_manager": {},
"document:masterplan#viewer@user:eng_lead": {},
},
},
{
"relation and resource ID prefix",
&v1.RelationshipFilter{
OptionalRelation: "parent",
OptionalResourceIdPrefix: "masterpl",
},
codes.OK,
map[string]struct{}{
"document:masterplan#parent@folder:strategy": {},
"document:masterplan#parent@folder:plans": {},
},
},
{
"namespace and userset",
&v1.RelationshipFilter{
Expand Down Expand Up @@ -196,6 +259,15 @@ func TestReadRelationships(t *testing.T) {
codes.FailedPrecondition,
nil,
},
{
"invalid filter",
&v1.RelationshipFilter{
OptionalResourceId: "auditors",
OptionalResourceIdPrefix: "aud",
},
codes.InvalidArgument,
nil,
},
}

for _, pageSize := range []int{0, 1, 5, 10} {
Expand Down Expand Up @@ -248,6 +320,11 @@ func TestReadRelationships(t *testing.T) {

require.NoError(err)

dsFilter, err := datastore.RelationshipsFilterFromPublicFilter(tc.filter)
require.NoError(err)

require.True(dsFilter.Test(tuple.MustFromRelationship(rel.Relationship)), "relationship did not match filter: %v", rel.Relationship)

relString := tuple.MustRelString(rel.Relationship)
_, found := tc.expected[relString]
require.True(found, "relationship was not expected: %s", relString)
Expand Down Expand Up @@ -749,6 +826,40 @@ func TestDeleteRelationships(t *testing.T) {
"folder:auditors#viewer@user:auditor": {},
},
},
{
name: "delete by resource ID",
req: &v1.DeleteRelationshipsRequest{
RelationshipFilter: &v1.RelationshipFilter{
OptionalResourceId: "auditors",
},
},
deleted: map[string]struct{}{
"folder:auditors#viewer@user:auditor": {},
},
},
{
name: "delete by resource ID prefix",
req: &v1.DeleteRelationshipsRequest{
RelationshipFilter: &v1.RelationshipFilter{
OptionalResourceIdPrefix: "a",
},
},
deleted: map[string]struct{}{
"folder:auditors#viewer@user:auditor": {},
},
},
{
name: "delete by relation and resource ID prefix",
req: &v1.DeleteRelationshipsRequest{
RelationshipFilter: &v1.RelationshipFilter{
OptionalResourceIdPrefix: "s",
OptionalRelation: "editor",
},
},
deleted: map[string]struct{}{
"document:specialplan#editor@user:multiroleguy": {},
},
},
{
name: "delete resource + relation + subject type",
req: &v1.DeleteRelationshipsRequest{
Expand Down Expand Up @@ -945,7 +1056,12 @@ func TestDeleteRelationships(t *testing.T) {
OptionalResourceId: "someunknownid",
},
},
deleted: map[string]struct{}{},
expectedCode: codes.OK,
deleted: map[string]struct{}{
"document:specialplan#editor@user:multiroleguy": {},
"document:specialplan#viewer_and_editor@user:missingrolegal": {},
"document:specialplan#viewer_and_editor@user:multiroleguy": {},
},
},
{
name: "delete unknown resource type",
Expand Down Expand Up @@ -1006,6 +1122,17 @@ func TestDeleteRelationships(t *testing.T) {
expectedCode: codes.FailedPrecondition,
errorContains: "unable to satisfy write precondition",
},
{
name: "invalid filter",
req: &v1.DeleteRelationshipsRequest{
RelationshipFilter: &v1.RelationshipFilter{
OptionalResourceId: "auditors",
OptionalResourceIdPrefix: "aud",
},
},
expectedCode: codes.InvalidArgument,
errorContains: "resource_id and resource_id_prefix cannot be set at the same time",
},
}
for _, delta := range testTimedeltas {
delta := delta
Expand Down

0 comments on commit 627bb6a

Please sign in to comment.