Skip to content

Commit

Permalink
refactor: replace the 'not' (exclusion) keyword with logical operator…
Browse files Browse the repository at this point in the history 
…s like 'and' or 'or'
  • Loading branch information
@tolgaOzen
tolgaOzen committed May 23, 2023
1 parent 5cd6d05 commit 946b310
Show file tree
Hide file tree
Showing 26 changed files with 1,175 additions and 2,410 deletions.
361 changes: 213 additions & 148 deletions internal/engines/check.go
View file

Large diffs are not rendered by default.

41 changes: 20 additions & 21 deletions internal/engines/check_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ var _ = Describe("check-engine", func() {
Metadata: &base.PermissionCheckRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
Exclusion: false,
Depth: 20,
},
})
Expand Down Expand Up @@ -249,7 +248,6 @@ var _ = Describe("check-engine", func() {
Metadata: &base.PermissionCheckRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
Exclusion: false,
Depth: 20,
},
})
Expand Down Expand Up @@ -355,7 +353,6 @@ var _ = Describe("check-engine", func() {
Metadata: &base.PermissionCheckRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
Exclusion: false,
Depth: 20,
},
})
Expand Down Expand Up @@ -481,7 +478,6 @@ var _ = Describe("check-engine", func() {
Metadata: &base.PermissionCheckRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
Exclusion: false,
Depth: 20,
},
})
Expand Down Expand Up @@ -586,7 +582,6 @@ var _ = Describe("check-engine", func() {
Metadata: &base.PermissionCheckRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
Exclusion: false,
Depth: 20,
},
})
Expand Down Expand Up @@ -690,7 +685,6 @@ var _ = Describe("check-engine", func() {
Metadata: &base.PermissionCheckRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
Exclusion: false,
Depth: 20,
},
})
Expand Down Expand Up @@ -721,13 +715,13 @@ entity repo {
relation parent @parent
relation member @user
permission push = org.member and not parent.member
permission delete = not push
permission push = org.member not parent.member
permission delete = push
permission update = (org.member and not parent.member) and member
permission view = member or not update
permission manage = not update
permission admin = not manage
permission update = (org.member not parent.member) and member
permission view = member not update
permission manage = update
permission admin = manage
}
`

Expand Down Expand Up @@ -826,7 +820,6 @@ entity repo {
Metadata: &base.PermissionCheckRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
Exclusion: false,
Depth: 20,
},
})
Expand Down Expand Up @@ -933,7 +926,6 @@ entity repo {
Metadata: &base.PermissionCheckRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
Exclusion: false,
Depth: 20,
},
})
Expand Down Expand Up @@ -971,9 +963,10 @@ entity repo {
}{
relationships: []string{
"organization:1#member@user:1",
"organization:1#member@user:2",
"parent:1#admin@user:2",
"parent:1#member@user:1",
"parent:1#member@parent:1#admin",
"parent:1#member@user:2",
"repo:1#org@organization:1#...",
"repo:1#parent@parent:1#...",
},
Expand All @@ -982,7 +975,7 @@ entity repo {
entity: "repo:1",
subject: "user:2",
assertions: map[string]base.PermissionCheckResponse_Result{
"delete": base.PermissionCheckResponse_RESULT_ALLOWED,
"delete": base.PermissionCheckResponse_RESULT_DENIED,
},
},
},
Expand Down Expand Up @@ -1038,7 +1031,6 @@ entity repo {
Metadata: &base.PermissionCheckRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
Exclusion: false,
Depth: 20,
},
})
Expand Down Expand Up @@ -1076,13 +1068,23 @@ entity repo {
}{
relationships: []string{
"organization:1#member@user:1",

"parent:1#admin@user:2",
"parent:1#member@user:1",
"parent:1#member@parent:1#admin",

"repo:1#org@organization:1#...",
"repo:1#parent@parent:1#...",
"repo:1#member@user:2",
},
checks: []check{
{
entity: "repo:1",
subject: "user:2",
assertions: map[string]base.PermissionCheckResponse_Result{
"update": base.PermissionCheckResponse_RESULT_DENIED,
},
},
{
entity: "repo:1",
subject: "user:2",
Expand Down Expand Up @@ -1143,7 +1145,6 @@ entity repo {
Metadata: &base.PermissionCheckRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
Exclusion: false,
Depth: 20,
},
})
Expand Down Expand Up @@ -1192,7 +1193,7 @@ entity repo {
entity: "repo:1",
subject: "user:2",
assertions: map[string]base.PermissionCheckResponse_Result{
"view": base.PermissionCheckResponse_RESULT_ALLOWED,
"view": base.PermissionCheckResponse_RESULT_DENIED,
},
},
},
Expand Down Expand Up @@ -1248,7 +1249,6 @@ entity repo {
Metadata: &base.PermissionCheckRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
Exclusion: false,
Depth: 20,
},
})
Expand Down Expand Up @@ -1353,7 +1353,6 @@ entity repo {
Metadata: &base.PermissionCheckRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
Exclusion: false,
Depth: 20,
},
})
Expand Down
2 changes: 0 additions & 2 deletions internal/engines/entityFilter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ func (engine *EntityFilterEngine) EntityFilter(
SnapToken: request.GetMetadata().GetSnapToken(),
SchemaVersion: request.GetMetadata().GetSchemaVersion(),
Depth: request.GetMetadata().GetDepth(),
Exclusion: false,
}, base.PermissionCheckResponse_RESULT_UNKNOWN)
}

Expand Down Expand Up @@ -246,7 +245,6 @@ func (engine *EntityFilterEngine) l(
SnapToken: request.GetMetadata().GetSnapToken(),
SchemaVersion: request.GetMetadata().GetSchemaVersion(),
Depth: request.GetMetadata().GetDepth(),
Exclusion: false,
}, base.PermissionCheckResponse_RESULT_UNKNOWN)
return nil
}
Expand Down
69 changes: 27 additions & 42 deletions internal/engines/expand.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,22 +87,10 @@ func (engine *ExpandEngine) expand(ctx context.Context, request *base.Permission
}

child := permission.GetChild()

req := &base.PermissionExpandRequest{
TenantId: request.GetTenantId(),
Entity: request.GetEntity(),
Permission: request.GetPermission(),
Metadata: &base.PermissionExpandRequestMetadata{
SchemaVersion: request.GetMetadata().GetSchemaVersion(),
SnapToken: request.GetMetadata().GetSnapToken(),
Exclusion: request.GetMetadata().GetExclusion() != child.GetExclusion(),
},
}

if child.GetRewrite() != nil {
fn = engine.expandRewrite(ctx, req, child.GetRewrite())
fn = engine.expandRewrite(ctx, request, child.GetRewrite())
} else {
fn = engine.expandLeaf(ctx, req, child.GetLeaf())
fn = engine.expandLeaf(ctx, request, child.GetLeaf())
}
} else {
fn = engine.expandDirect(ctx, request)
Expand All @@ -121,17 +109,11 @@ func (engine *ExpandEngine) expand(ctx context.Context, request *base.Permission
func (engine *ExpandEngine) expandRewrite(ctx context.Context, request *base.PermissionExpandRequest, rewrite *base.Rewrite) ExpandFunction {
switch rewrite.GetRewriteOperation() {
case *base.Rewrite_OPERATION_UNION.Enum():
expandFunc := expandUnion
if request.GetMetadata().GetExclusion() {
expandFunc = expandIntersection
}
return engine.setChild(ctx, request, rewrite.GetChildren(), expandFunc)
return engine.setChild(ctx, request, rewrite.GetChildren(), expandUnion)
case *base.Rewrite_OPERATION_INTERSECTION.Enum():
expandFunc := expandIntersection
if request.GetMetadata().GetExclusion() {
expandFunc = expandUnion
}
return engine.setChild(ctx, request, rewrite.GetChildren(), expandFunc)
return engine.setChild(ctx, request, rewrite.GetChildren(), expandIntersection)
case *base.Rewrite_OPERATION_EXCLUSION.Enum():
return engine.setChild(ctx, request, rewrite.GetChildren(), expandExclusion)
default:
return expandFail(errors.New(base.ErrorCode_ERROR_CODE_UNDEFINED_CHILD_TYPE.String()))
}
Expand Down Expand Up @@ -170,23 +152,11 @@ func (engine *ExpandEngine) setChild(
) ExpandFunction {
var functions []ExpandFunction
for _, child := range children {

req := &base.PermissionExpandRequest{
TenantId: request.GetTenantId(),
Entity: request.GetEntity(),
Permission: request.GetPermission(),
Metadata: &base.PermissionExpandRequestMetadata{
SchemaVersion: request.GetMetadata().GetSchemaVersion(),
SnapToken: request.GetMetadata().GetSnapToken(),
Exclusion: request.GetMetadata().GetExclusion() != child.GetExclusion(),
},
}

switch child.GetType().(type) {
case *base.Child_Rewrite:
functions = append(functions, engine.expandRewrite(ctx, req, child.GetRewrite()))
functions = append(functions, engine.expandRewrite(ctx, request, child.GetRewrite()))
case *base.Child_Leaf:
functions = append(functions, engine.expandLeaf(ctx, req, child.GetLeaf()))
functions = append(functions, engine.expandLeaf(ctx, request, child.GetLeaf()))
default:
return expandFail(errors.New(base.ErrorCode_ERROR_CODE_UNDEFINED_CHILD_KIND.String()))
}
Expand Down Expand Up @@ -249,8 +219,7 @@ func (engine *ExpandEngine) expandDirect(ctx context.Context, request *base.Perm
expandChan <- ExpandResponse{
Response: &base.PermissionExpandResponse{
Tree: &base.Expand{
Target: target,
Exclusion: request.GetMetadata().GetExclusion(),
Target: target,
Node: &base.Expand_Leaf{
Leaf: &base.Subjects{
// Combine both user sets into one slice.
Expand Down Expand Up @@ -300,8 +269,7 @@ func (engine *ExpandEngine) expandDirect(ctx context.Context, request *base.Perm

// Add a new child to the Expand field.
expand.Children = append(expand.Children, &base.Expand{
Exclusion: request.GetMetadata().GetExclusion(),
Target: target,
Target: target,
Node: &base.Expand_Leaf{
Leaf: &base.Subjects{
// Combine both user sets into one slice.
Expand Down Expand Up @@ -532,6 +500,23 @@ func expandIntersection(ctx context.Context, target *base.EntityAndRelation, fun
return expandOperation(ctx, target, functions, base.ExpandTreeNode_OPERATION_INTERSECTION)
}

// expandExclusion is a helper function that executes multiple ExpandFunctions in parallel and returns an ExpandResponse
// containing the expanded user set that results from the exclusion operation. The function delegates to expandOperation
// with the EXCLUSION operation. If any of the ExpandFunctions return an error, the function returns an ExpandResponse
// with the error. If the context is cancelled before all ExpandFunctions complete, the function returns an ExpandResponse
// with an error indicating that the operation was cancelled.
//
// Parameters:
// - ctx: context.Context for the request
// - target: EntityAndRelation containing the entity and its relation for which the exclusion is calculated
// - functions: slice of ExpandFunctions to execute in parallel
//
// Returns:
// - ExpandResponse containing the expanded user sets from the exclusion operation, or an error if any of the ExpandFunctions failed
func expandExclusion(ctx context.Context, target *base.EntityAndRelation, functions []ExpandFunction) ExpandResponse {
return expandOperation(ctx, target, functions, base.ExpandTreeNode_OPERATION_EXCLUSION)
}

// expandFail is a helper function that returns an ExpandFunction that immediately sends an ExpandResponse with the specified error
// to the provided channel. The resulting ExpandResponse contains an empty ExpandTreeNode and the specified error.
//
Expand Down

0 comments on commit 946b310

Please sign in to comment.