feat(policy): support arbitrary collection traversal from auth()
#439
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis change adds auth() invocation validation, expands typedef member-access scope for auth contexts, refactors expression visitor traversal to support short-circuit aborting, changes policy expression evaluation to propagate nulls for non-equality operators, extends the policy expression-transformer context, adds collection-predicate operator types, tweaks auth model lookup to include TypeDefs named "User", updates a test config, and adds extensive auth-related e2e tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-21T16:09:31.218ZApplied to files:
🧬 Code graph analysis (1)packages/plugins/policy/src/expression-transformer.ts (8)
🔇 Additional comments (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @ymc9's task —— View job Code Review Complete ✅This PR successfully adds support for arbitrary collection traversal from ✅ Code Quality & ArchitectureStrong Points:
Areas for improvement:
✅ Implementation AnalysisCore Changes:
✅ Security AssessmentNo security concerns identified:
✅ Performance ConsiderationsEfficient implementation:
Potential areas to monitor:
✅ Test CoverageComprehensive test suite (
✅ Bug AnalysisNo critical bugs identified:
Minor observations:
✅ API DesignWell-designed feature:
📋 Recommendations
🎯 Overall AssessmentExcellent implementation that successfully delivers the requested feature. The code is well-structured, thoroughly tested, and maintains the security and performance characteristics of the existing system. This is a solid addition that will significantly enhance ZenStack's access control capabilities. Recommendation: ✅ APPROVE |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/orm/src/utils/schema-utils.ts (1)
76-96: Clean implementation of MatchingExpressionVisitor.The visitor correctly:
- Uses a predicate to find matching expressions
- Sets
foundflag and returns abort signal on match- Delegates to parent for continued traversal otherwise
One minor observation: the
findmethod doesn't resetfoundtofalsebefore traversal. If the same visitor instance is reused across multiplefindcalls, it could return stale results.Consider resetting state for reusability:
find(expr: Expression) { + this.found = false; this.visit(expr); return this.found; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
TODO.md(1 hunks)packages/language/src/validators/function-invocation-validator.ts(1 hunks)packages/language/src/zmodel-scope.ts(3 hunks)packages/orm/src/utils/schema-utils.ts(2 hunks)packages/plugins/policy/src/expression-evaluator.ts(1 hunks)packages/plugins/policy/src/expression-transformer.ts(19 hunks)packages/plugins/policy/src/policy-handler.ts(1 hunks)packages/plugins/policy/src/types.ts(1 hunks)packages/sdk/src/model-utils.ts(1 hunks)tests/e2e/apps/rally/rally.test.ts(0 hunks)tests/e2e/orm/policy/auth-access.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/apps/rally/rally.test.ts
🧰 Additional context used
🧬 Code graph analysis (4)
tests/e2e/orm/policy/auth-access.test.ts (2)
packages/testtools/src/client.ts (1)
createPolicyTestClient(254-265)packages/plugins/policy/src/expression-transformer.ts (1)
schema(126-128)
packages/orm/src/utils/schema-utils.ts (1)
packages/schema/src/expression.ts (9)
LiteralExpression(12-15)ArrayExpression(17-20)FieldExpression(22-25)MemberExpression(27-31)BinaryExpression(39-44)UnaryExpression(33-37)CallExpression(46-50)ThisExpression(52-54)NullExpression(56-58)
packages/language/src/validators/function-invocation-validator.ts (1)
packages/language/src/generated/ast.ts (2)
InvocationExpr(533-538)InvocationExpr(540-540)
packages/language/src/zmodel-scope.ts (2)
packages/language/src/utils.ts (1)
isAuthOrAuthMemberAccess(133-135)packages/language/src/generated/ast.ts (1)
isTypeDef(737-739)
🪛 markdownlint-cli2 (0.18.1)
TODO.md
6-6: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
7-7: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
9-9: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
11-11: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (26)
packages/sdk/src/model-utils.ts (1)
104-112: LGTM! Fallback now consistently includes TypeDef.The change aligns the fallback "User" lookup with the primary
@@authattribute search by includingisTypeDef(d). This correctly supports the new feature of usingtype User { @@auth }as an auth declaration.TODO.md (1)
6-12: Backlog updates noted.packages/plugins/policy/src/expression-evaluator.ts (1)
82-85: Null propagation logic looks correct for SQL-like semantics.For non-equality operators, returning
nullwhen either operand isnullfollows SQL three-valued logic.One consideration: logical operators
&&and||could alternatively use JavaScript short-circuit semantics (e.g.,false && null → false,true || null → true). If intentional to treat them uniformly with SQL semantics, this is fine.packages/plugins/policy/src/types.ts (1)
22-31: LGTM! Clean type definition for collection predicate operators.Good use of
as constwith derived type for type-safe operator handling. The operators (?,!,^) align with their usage in the expression evaluator.packages/language/src/zmodel-scope.ts (2)
139-142: LGTM! Properly enables typedef scope for auth-based member access.The condition
isAuthOrAuthMemberAccess(node.operand) || !!AstUtils.getContainerOfType(node, isTypeDef)correctly allows typedef field resolution when accessing fromauth()or when already inside a typedef context.
186-189: Collection predicate typedef scope looks correct.Enabling typedef scope via
isAuthOrAuthMemberAccess(collection)allows collection predicates likeauth().profiles?[age > 18]to properly resolve typedef members.The TODO comment suggests this is partial support—is there a tracking issue for full typedef member access support?
packages/language/src/validators/function-invocation-validator.ts (1)
182-191: LGTM! Helpful validation for unresolved auth() invocations.The validator provides clear guidance when
auth()cannot be resolved, directing users to either use@@authattribute or name their model/type "User". This follows the existing decorator pattern used by other function validators.tests/e2e/orm/policy/auth-access.test.ts (6)
1-47: Excellent test coverage for basic auth scenarios.The tests cover both model-based (
model User) and type-based (type User { @@auth }) auth declarations with appropriate assertions for unauthenticated, underage, and valid auth states.
49-100: Good coverage of nested auth value access.Tests properly verify that deep property access like
auth().profile.ageworks for both model relations and nested type structures, including null-safety when intermediate values are missing.
102-164: Collection predicate tests with?(some) operator look correct.The tests cover:
- Simple collection predicates (
profiles?[age > 18])- Predicates with field references (
profiles?[age >= this.requiredAge])- Empty arrays returning falsy (no match possible)
240-303: Vacuous truth semantics correctly tested for!and^operators.Lines 267 and 300: Empty arrays correctly return truthy for both
every(!) andnone(^) predicates, which follows standard vacuous truth semantics.
305-376: Complex nested collection predicate test is well-designed.The test at lines 305-376 exercises a realistic permission model with:
- Nested
roles?[permissions![...]](some role where all permissions match)- Array field predicates (
this.type in canReadTypes)- Various edge cases including empty arrays and partial matches
Line 341 correctly expects truthy for empty permissions (vacuous truth of
every).
378-435: Regression test provides good real-world coverage.The test validates a practical permission-based update scenario with
canUpdateChannelByIdarrays. Good assertions for both forbidden and allowed updates.packages/orm/src/utils/schema-utils.ts (3)
15-30: Well-designed short-circuit traversal pattern.The
VisitResulttype and its propagation through the visitor pattern is clean. The use ofts-pattern'sexhaustive()ensures all expression kinds are handled.
34-41: Correctly propagates abort from array items.The early return on
abortproperly short-circuits the traversal when a match is found.
49-56: Correctly propagates abort from binary expression children.The short-circuit logic properly returns when the left operand signals abort, avoiding unnecessary traversal of the right operand.
packages/plugins/policy/src/policy-handler.ts (1)
882-890: Context passing looks correct.The new context fields are properly initialized:
modelOrTypereplaces the previousmodelfieldthisTypeandthisAliascapture the root model context forthiskeyword resolution during nested transformationsThe comments on lines 885-886 clarify the invariant that these never change during transformation, which aligns with how they're used in the transformer.
packages/plugins/policy/src/expression-transformer.ts (9)
61-104: Well-documented context shape with clear semantics.The new context fields are well-documented with JSDoc comments explaining their purpose. The distinction between
modelOrType(current resolution context) andthisType(root model forthiskeyword) is clear.
169-188: Correctly handles field evaluation against value objects.When
contextValueis present, fields are evaluated directly from the value object rather than generating SQL references. This enables the auth() collection traversal feature.
256-273: Improved null check handling with SQL semantics.The updated
transformNullCheckcorrectly handles:
- Equality comparisons with
IS NULL/IS NOT NULL- Static evaluation when comparing ValueNode with null
- Other comparisons return
null(SQL semantics wherex > nullis null)
302-321: Auth member collection predicates now supported.The condition
this.isAuthMember(expr.left) || context.contextValuecorrectly identifies when the LHS should be evaluated as a value rather than transformed into SQL.
625-634: Correctly handlesthis.relationwith proper context reset.The
thiskeyword resolution now correctly usesthisAliasandthisTypefrom context, and explicitly clearscontextValueto ensurethisalways refers to the database model, not a value object.
707-730: RefactoredvalueMemberAccesshandles multi-level traversal correctly.The method now:
- Properly handles null/undefined at any level
- Tracks the current type through the traversal
- Transforms the final value with the correct type
498-512: Good defensive null handling intransformValue.The nullish coalescing to
null(line 504) ensures undefined values are converted to null, maintaining SQL compatibility.
377-426: Based on my verification of the codebase, I have examined the implementations ofdisjunction,conjunction, andlogicalNotinpackages/plugins/policy/src/utils.ts.The empty array handling is correct:
disjunction([])returnsfalseNode(dialect)(line 73-74) — correct for "some" (no element satisfies = false)conjunction([])returnstrueNode(dialect)(line 53-54) — correct for "every" (vacuous truth)logicalNot(disjunction([]))returnstrueNode(line 96-100) — correct for "none" (no element satisfies = true)The implementation already handles empty arrays correctly according to set theory semantics, so the concern in the original review comment has been verified as not being an issue.
Empty array handling in collection predicates is correct per set theory semantics and requires no changes.
The
disjunctionandconjunctionfunctions inpackages/plugins/policy/src/utils.tsproperly handle empty arrays:
?(some): disjunction([]) → false ✓!(every): conjunction([]) → true ✓^(none): logicalNot(disjunction([])) → true ✓
428-440: The review comment's characterization of the issue requires clarification.The
getMemberTypemethod does indirectly handle the receiver field through thereceiverTypeparameter, but the concern about incomplete handling may be valid depending on the call site context. Looking at line 317, the caller determinesbaseTypeas eitherthis.authType(ifisAuthMember(expr.left)) orcontext.modelOrType(otherwise).For cases where
expr.leftis aMemberExpression, the method iterates throughexpr.memberssequentially starting from the receiver's type. However, if thereceiveritself is a complex expression (not justauth(),before(), or a simpleFieldExpression), thebaseTypedetermination at the call site may not account for that complexity correctly.The comment correctly identifies that for expressions like
auth().profile.roles, whereprofileis in the receiver's field conceptually, the implementation should be verified to ensure the receiver type is properly resolved before each member traversal. However, based on how the grammar flattens chained member access into a singleMemberAccessExprwith all members in an array, the current implementation may actually work correctly despite the design concern.Manual verification is needed to confirm whether there are edge cases where
baseTypeis not properly determined for all possible receiver types inMemberExpression.
There was a problem hiding this comment.
Pull request overview
This PR adds support for arbitrary collection traversal from auth() in access policies, enabling users to traverse through collections in auth objects (both model-based and type-based) using collection predicate operators (?, !, ^). This significantly expands the expressiveness of access control rules.
Key changes include:
- Support for collection predicates on auth() with arbitrary nesting depth (e.g.,
auth().roles?[permissions![this.type in canReadTypes]]) - Support for both model-based and type-based auth declarations
- Proper handling of
thiskeyword references within collection predicates to refer back to the entity being accessed
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/orm/policy/auth-access.test.ts | Comprehensive test suite covering simple/deep auth access, collection predicates with various operators (?/!/^), and regression tests |
| tests/e2e/apps/rally/rally.test.ts | Removed debug flag (cleanup) |
| packages/sdk/src/model-utils.ts | Extended getAuthDecl to also check for TypeDef with User name, not just DataModel |
| packages/plugins/policy/src/types.ts | Added CollectionPredicateOperator constant and type definitions |
| packages/plugins/policy/src/policy-handler.ts | Updated to pass thisType and thisAlias context for proper this keyword resolution |
| packages/plugins/policy/src/expression-transformer.ts | Major implementation: added contextValue support for value-based transformations, transformValueCollectionPredicate for handling auth collections, enhanced context tracking |
| packages/plugins/policy/src/expression-evaluator.ts | Added null handling for non-equality comparisons (follows SQL semantics) |
| packages/orm/src/utils/schema-utils.ts | Enhanced ExpressionVisitor with abort mechanism, added MatchingExpressionVisitor for finding specific expression patterns |
| packages/language/src/zmodel-scope.ts | Enabled typedef scope resolution for auth() member access and collection predicates |
| packages/language/src/validators/function-invocation-validator.ts | Added validation for auth() function to ensure it resolves properly |
| TODO.md | Updated task completion status |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
Claude finished @ymc9's task —— View job Code Review for PR #439: Auth Collection Traversal SupportReviewing this PR that adds support for arbitrary collection traversal from Todo List:
🎯 Overall AssessmentExcellent implementation that successfully delivers a powerful new feature for ZenStack's access control system. The code enables complex nested authorization patterns like ✅ Code Quality & ArchitectureStrengths:
Areas for improvement:
✅ Implementation AnalysisCore Changes:
✅ Security AssessmentNo security concerns identified:
✅ Performance ConsiderationsEfficient implementation:
Areas to monitor:
✅ Test CoverageComprehensive test suite (
🔍 Minor Issues Found
🎯 Recommendations
📋 SummaryThis is a high-quality implementation that significantly enhances ZenStack's access control capabilities. The feature is well-tested, secure, and follows good architectural patterns. The minor issues identified are easily addressable and don't impact the core functionality. Recommendation: ✅ APPROVE with suggested minor improvements. |
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.