feat(policy): implementing check function
#255
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. WalkthroughRemoved the language-level Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 PR Review: Policy Check Function ImplementationReview Checklist
📋 PR SummaryThis PR implements a new
✅ Code Quality & Best PracticesStrengths:
Minor Issues:
🐛 Potential Bugs & Edge CasesIdentified Issues:
⚡ Performance ConsiderationsPotential Concerns:
🔒 Security AssessmentSecurity Strengths:
No Critical Security Issues Identified 🧪 Test Coverage AssessmentExcellent Test Coverage:
Test Quality:
📈 Architecture & IntegrationPositive Aspects:
🎯 RecommendationsCritical (Should Address Before Merge)
High Priority
Nice to Have
🏁 Overall AssessmentVerdict: ✅ APPROVE with Minor Concerns This is a well-implemented feature that adds significant value to the ZenStack policy system. The code quality is high, follows established patterns, and includes comprehensive testing. The main concern is the potential for circular reference issues in complex policy hierarchies, which should be addressed before production use. The implementation demonstrates:
Confidence Level: High - This feature appears ready for production with the circular reference protection added. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements the check function for ZenStack's policy system, enabling relation-based access control checks. The implementation moves the check function from the standard library to the policy plugin and adds comprehensive validation and testing.
Key changes include:
- Implements the
checkfunction to validate access permissions on related entities - Refactors computed field context parameter from
currentModeltomodelAliasfor consistency - Adds comprehensive test coverage for various scenarios including CRUD operations, nested operations, and error cases
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/src/plugins/policy/functions.ts | New implementation of the check function for relation-based access control |
| packages/runtime/test/policy/migrated/relation-check.test.ts | Comprehensive test suite covering all aspects of the check function |
| packages/runtime/src/plugins/policy/expression-transformer.ts | Refactored to support plugin functions and updated function context |
| packages/runtime/src/plugins/policy/plugin.zmodel | Moved check function definition from stdlib to policy plugin |
| packages/language/res/stdlib.zmodel | Removed check function from standard library |
| Multiple schema files | Updated parameter name from currentModel to modelAlias in computed field contexts |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Claude finished @ymc9's task —— View job Claude finished reviewing the policy check function implementation PR Review: Policy Check Function ImplementationReview Checklist
📋 PR SummaryThis PR implements a new
✅ Code Quality & Best PracticesStrengths:
Minor Issues:
🐛 Potential Bugs & Edge CasesIdentified Issues:
⚡ Performance ConsiderationsPotential Concerns:
🔒 Security AssessmentSecurity Strengths:
No Critical Security Issues Identified 🧪 Test Coverage AssessmentExcellent Test Coverage:
Test Quality:
📈 Architecture & IntegrationPositive Aspects:
🎯 RecommendationsCritical (Should Address Before Merge)
High Priority
Nice to Have
🏁 Overall AssessmentVerdict: ✅ APPROVE with Minor Concerns This is a well-implemented feature that adds significant value to the ZenStack policy system. The code quality is high, follows established patterns, and includes comprehensive testing. The main concern is the potential for circular reference issues in complex policy hierarchies, which should be addressed before production use. The implementation demonstrates:
Confidence Level: High - This feature appears ready for production with the circular reference protection added. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/language/src/validators/function-invocation-validator.ts (1)
222-231: Don’t abort traversal on unrelated cycles; continue instead.Breaking the loop on a non-start cycle can miss a cycle that does involve
startreachable via other queued paths. Usecontinueso remaining tasks are still explored.Apply this diff:
- break; + continue;
🧹 Nitpick comments (5)
packages/language/src/validators/function-invocation-validator.ts (1)
194-201: Centralize allowed operation literals.Hardcoded strings risk drift with the policy plugin. At minimum, hoist into a local constant; ideally source from a shared enum/constants owned by the plugin.
Apply this minimal tidy-up:
- const operation = getLiteral<string>(opArg); - if (!operation || !['read', 'create', 'update', 'delete'].includes(operation)) { + const operation = getLiteral<string>(opArg); + const allowedOps = ['read', 'create', 'update', 'delete'] as const; + if (!operation || !allowedOps.includes(operation as any)) { accept('error', 'argument must be a "read", "create", "update", or "delete"', { node: expr.args[1]! }); valid = false; }packages/runtime/src/client/kysely-utils.ts (1)
17-19: Potential issue with non-null assertion after optional chaining.Using
!after the optional chaining ininnerNode!could cause runtime errors ifstripAliasreturns{ node: undefined }. While unlikely with the current implementation, it's safer to handle this case explicitly.Apply this diff to make the function more robust:
export function extractModelName(node: OperationNode) { const { node: innerNode } = stripAlias(node); - return TableNode.is(innerNode!) ? innerNode!.table.identifier.name : undefined; + return innerNode && TableNode.is(innerNode) ? innerNode.table.identifier.name : undefined; }packages/runtime/src/plugins/policy/functions.ts (1)
17-31: Consider extracting the valid operations as a constant.The hardcoded array
['create', 'read', 'update', 'delete']should be extracted as a constant to avoid duplication and improve maintainability.+const VALID_OPERATIONS: CRUD[] = ['create', 'read', 'update', 'delete']; + export const check: ZModelFunction<any> = ( eb: ExpressionBuilder<any, any>, args: Expression<any>[], { client, model, modelAlias, operation }: ZModelFunctionContext<any>, ) => { invariant(args.length === 1 || args.length === 2, '"check" function requires 1 or 2 arguments'); const arg1Node = args[0]!.toOperationNode(); const arg2Node = args.length === 2 ? args[1]!.toOperationNode() : undefined; if (arg2Node) { invariant( ValueNode.is(arg2Node) && typeof arg2Node.value === 'string', '"operation" parameter must be a string literal when provided', ); invariant( - ['create', 'read', 'update', 'delete'].includes(arg2Node.value), + VALID_OPERATIONS.includes(arg2Node.value as CRUD), '"operation" parameter must be one of "create", "read", "update", "delete"', ); }packages/runtime/test/policy/migrated/relation-check.test.ts (1)
261-318: Consider removing or documenting the skipped tests.Multiple field-level policy tests are marked as skipped with TODO comments. Consider either:
- Creating GitHub issues to track the field-level policy support implementation
- Adding more detailed documentation about why these tests are skipped and when they will be enabled
Would you like me to create GitHub issues to track the implementation of field-level policy support?
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
382-396: Consider caching function lookups for performance.The
getFunctionImplmethod searches through plugins on every call. For frequently used functions, consider caching the resolved functions to avoid repeated lookups.+private functionCache = new Map<string, ZModelFunction<Schema> | undefined>(); + private getFunctionImpl(functionName: string) { + // Check cache first + if (this.functionCache.has(functionName)) { + return this.functionCache.get(functionName); + } + // check built-in functions let func = this.clientOptions.functions?.[functionName]; if (!func) { // check plugins for (const plugin of this.clientOptions.plugins ?? []) { if (plugin.functions?.[functionName]) { func = plugin.functions[functionName]; break; } } } + + // Cache the result (including undefined) + this.functionCache.set(functionName, func); return func; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
packages/language/res/stdlib.zmodel(0 hunks)packages/language/src/utils.ts(1 hunks)packages/language/src/validators/function-invocation-validator.ts(1 hunks)packages/runtime/src/client/executor/kysely-utils.ts(0 hunks)packages/runtime/src/client/executor/name-mapper.ts(4 hunks)packages/runtime/src/client/executor/zenstack-query-executor.ts(1 hunks)packages/runtime/src/client/kysely-utils.ts(1 hunks)packages/runtime/src/client/options.ts(1 hunks)packages/runtime/src/client/plugin.ts(2 hunks)packages/runtime/src/client/query-utils.ts(1 hunks)packages/runtime/src/plugins/policy/expression-transformer.ts(4 hunks)packages/runtime/src/plugins/policy/functions.ts(1 hunks)packages/runtime/src/plugins/policy/plugin.ts(2 hunks)packages/runtime/src/plugins/policy/plugin.zmodel(1 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(2 hunks)packages/runtime/test/client-api/computed-fields.test.ts(1 hunks)packages/runtime/test/policy/migrated/relation-check.test.ts(1 hunks)packages/runtime/test/schemas/typing/schema.ts(1 hunks)packages/sdk/src/ts-schema-generator.ts(2 hunks)samples/blog/zenstack/schema.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/runtime/src/client/executor/kysely-utils.ts
- packages/language/res/stdlib.zmodel
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/language/src/utils.tspackages/language/src/validators/function-invocation-validator.tspackages/runtime/test/policy/migrated/relation-check.test.tssamples/blog/zenstack/schema.tspackages/runtime/src/client/plugin.tspackages/runtime/src/client/executor/name-mapper.tspackages/runtime/src/client/query-utils.tspackages/sdk/src/ts-schema-generator.tspackages/runtime/test/schemas/typing/schema.tspackages/runtime/src/plugins/policy/plugin.tspackages/runtime/src/plugins/policy/functions.tspackages/runtime/src/client/options.tspackages/runtime/src/client/executor/zenstack-query-executor.tspackages/runtime/test/client-api/computed-fields.test.tspackages/runtime/src/client/kysely-utils.tspackages/runtime/src/plugins/policy/plugin.zmodelpackages/runtime/src/plugins/policy/policy-handler.tspackages/runtime/src/plugins/policy/expression-transformer.ts
**/schema.ts
📄 CodeRabbit inference engine (CLAUDE.md)
The generated TypeScript schema should be named
schema.ts
Files:
samples/blog/zenstack/schema.tspackages/runtime/test/schemas/typing/schema.ts
🧬 Code graph analysis (8)
packages/language/src/utils.ts (2)
packages/language/src/ast.ts (2)
AstNode(4-4)AstNode(66-71)packages/language/src/generated/ast.ts (1)
isInvocationExpr(542-544)
packages/runtime/test/policy/migrated/relation-check.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/client/plugin.ts (1)
packages/runtime/src/client/options.ts (1)
ZModelFunction(36-40)
packages/runtime/src/client/executor/name-mapper.ts (1)
packages/runtime/src/client/kysely-utils.ts (2)
extractModelName(17-20)extractFieldName(25-33)
packages/runtime/src/plugins/policy/functions.ts (6)
packages/runtime/src/client/options.ts (2)
ZModelFunction(36-40)ZModelFunctionContext(9-34)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/client/kysely-utils.ts (1)
extractFieldName(25-33)packages/runtime/src/client/query-utils.ts (2)
requireField(37-55)buildJoinPairs(213-231)packages/runtime/src/client/contract.ts (1)
CRUD(214-214)packages/runtime/src/plugins/policy/policy-handler.ts (1)
PolicyHandler(49-627)
packages/runtime/src/client/options.ts (2)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/sdk/src/schema/schema.ts (1)
GetModels(108-108)
packages/runtime/src/plugins/policy/policy-handler.ts (3)
packages/sdk/src/schema/schema.ts (1)
GetModels(108-108)packages/runtime/src/client/contract.ts (1)
CRUD(214-214)packages/runtime/src/plugins/policy/expression-transformer.ts (1)
ExpressionTransformer(71-625)
packages/runtime/src/plugins/policy/expression-transformer.ts (2)
packages/runtime/src/client/contract.ts (1)
ClientContract(50-159)packages/runtime/src/client/crud/dialects/index.ts (1)
getCrudDialect(8-16)
⏰ 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). (1)
- GitHub Check: claude-review
🔇 Additional comments (35)
packages/language/src/validators/function-invocation-validator.ts (1)
175-176: Remove blanket// @ts-expect-error; fix the decorator typing or method visibility.Do not suppress the error — either type the decorator factory to return a proper MethodDecorator (or an appropriately typed decorator signature) or make the handler method visible/typed so TypeScript accepts the decoration.
Location: packages/language/src/validators/function-invocation-validator.ts:175-176 (also see similar suppressions in packages/language/src/validators/attribute-application-validator.ts).
Run the compiler to reveal the exact error: npx tsc --noEmit -p tsconfig.json.packages/sdk/src/ts-schema-generator.ts (1)
379-392: Rename to modelAlias looks consistentThe generated computed-field context now uses
{ modelAlias: string }. Matches runtime usage. LGTM.packages/runtime/test/schemas/typing/schema.ts (1)
88-92: Typing update aligns with generator/runtime
modelAlias: stringin the stub signature is correct and consistent. No action needed.samples/blog/zenstack/schema.ts (1)
78-82: Sample schema updated correctlyContext rename to
modelAliasmatches SDK/runtime. Good.packages/runtime/test/client-api/computed-fields.test.ts (1)
240-244: Test updated to new context keyUsing
context.modelAliasinsql.refaligns with the runtime change. Looks good.packages/runtime/src/client/executor/zenstack-query-executor.ts (1)
29-29: Import path fixSwitching to the centralized
../kysely-utilslooks correct and reduces duplication. 👍packages/runtime/src/client/query-utils.ts (1)
197-197: Pass a non-empty alias to computed-field functionscomputer expects { modelAlias: string }; if modelAlias is undefined this will throw at runtime — fall back to the model name.
Apply:
- return computer(eb, { modelAlias }); + return computer(eb, { modelAlias: modelAlias ?? model });Caller found: packages/runtime/src/client/crud/dialects/base-dialect.ts:1144 passes modelAlias (may be undefined).
packages/language/src/utils.ts (1)
360-363: Over-broad detection forcheckinvocationsMatching any function named
checkrisks collisions with user-defined functions.
File: packages/language/src/utils.ts (≈ lines 360–363)
- Short term: gate on additional heuristics (originating module/namespace or available metadata) rather than name-only.
- Mid term: move this to the policy plugin (per TODO) so plugin metadata can disambiguate.
- Verification: repository scan here was inconclusive — run locally to confirm there are no non-policy
checkdefinitions/usages:rg -n --hidden --no-ignore -S -g '!**/dist/**' \ -e '\bfunction\s+check\b' \ -e '\bexport\s+function\s+check\b' \ -e '\b(const|let|var)\s+check\s*=' \ -e '\bcheck\s*\(' \ -e '\.\s*check\s*\(' \ -e '\bcheck\s*:' -C2 .packages/runtime/src/client/options.ts (1)
10-34: Expanded ZModelFunctionContext is sensible — approved
transformCall (packages/runtime/src/plugins/policy/expression-transformer.ts) constructs and passes client, dialect, model, modelAlias (alias ?? model), and operation to ZModelFunction, so call sites already populate these fields.packages/runtime/src/plugins/policy/plugin.ts (1)
19-23: LGTM! Clean implementation of functions getter.The addition of the
functionsgetter properly exposes thecheckfunction through the plugin's public API, aligning with the new runtime plugin interface that supports custom function implementations.packages/runtime/src/client/plugin.ts (1)
27-31: Well-designed extension to the plugin interface.The optional
functionsproperty provides a clean way for plugins to expose custom functions while maintaining backward compatibility. The use ofRecord<string, ZModelFunction<Schema>>allows flexible function registration.packages/runtime/src/client/executor/name-mapper.ts (2)
173-173: Good refactoring to use centralized utilities.The migration from local utility methods to the centralized
extractModelNamefunction fromkysely-utilsimproves code organization and reusability.
222-223: Consistent use of centralizedextractFieldName.The replacement of
this.extractFieldName()with the importedextractFieldName()function maintains behavioral parity while improving code organization.Also applies to: 428-428
packages/runtime/src/client/kysely-utils.ts (2)
1-13: Well-implemented utility functions with proper type guards.The
stripAliasfunction correctly handles both aliased and non-aliased nodes, providing a clean API for alias extraction.
25-33: Clean implementation of field name extraction.The function handles both
ReferenceNodeandColumnNodecases appropriately, providing comprehensive field name extraction.packages/runtime/src/plugins/policy/policy-handler.ts (3)
506-506: Good decision to makebuildPolicyFilterpublic.Changing the visibility from private to public enables the
checkfunction implementation to reuse this core policy filtering logic, promoting code reuse.
507-539: Well-structured policy composition logic.The implementation correctly handles the precedence of allow and deny policies:
- No policies → deny all (false)
- No allow policies → deny all (false)
- Combines allows with OR, denies with AND of negations
- Final result:
(allow1 OR allow2 OR ...) AND NOT(deny1) AND NOT(deny2) AND ...This provides the expected security semantics where any allow grants access unless explicitly denied.
587-593: Clean migration to client-based ExpressionTransformer.The change to instantiate
ExpressionTransformerwith the client instead of individual parameters (schema, options, auth) simplifies the API and provides better encapsulation.packages/runtime/src/plugins/policy/plugin.zmodel (1)
35-43: Empty body is intentional — no change requiredThis follows the established pattern for runtime-provided builtins (see packages/language/res/stdlib.zmodel where many functions are declared with empty bodies and @@@expressionContext); the runtime implements these, so do not add a TODO or placeholder.
Likely an incorrect or invalid review comment.
packages/runtime/src/plugins/policy/functions.ts (6)
1-7: LGTM! Clean imports with proper type safety.The imports are well-organized and include necessary type definitions and utility functions for implementing the relation checker.
12-16: LGTM! Well-typed function signature using generics.The function signature properly uses
ZModelFunction<any>with appropriate parameter destructuring for better readability.
33-39: LGTM! Comprehensive validation of the relation field.Good validation chain ensuring the field exists, is a relation, and is not a to-many relation. The error messages are clear and actionable.
41-44: LGTM! Clean operation resolution and handler instantiation.The fallback to context operation when the second argument is not provided is well-implemented.
45-50: LGTM! Well-structured join condition building.The code correctly handles both single and multi-field join scenarios using the appropriate Kysely expression builder methods.
52-62: Verify policy filter behavior when no alias is provided.buildPolicyFilter(relationModel, undefined, op) is invoked in packages/runtime/src/plugins/policy/functions.ts (lines 52–62). Confirm policy-handler.buildPolicyFilter treats a missing alias by defaulting to the model name; if it doesn't, either pass the relation's alias or make buildPolicyFilter use the model name as the default.
packages/runtime/test/policy/migrated/relation-check.test.ts (7)
1-3: LGTM! Standard test setup imports.
5-46: LGTM! Well-structured test for read operation with relation checks.The test properly validates that profiles are only readable when the associated user is public, demonstrating the
check(user, 'read')policy working correctly.
48-86: LGTM! Comprehensive test for create operation with relation checks.Good coverage of both success and failure cases, properly testing the policy enforcement during profile creation.
88-145: LGTM! Excellent nested create test with compound conditions.The test thoroughly validates complex policy conditions combining age checks with relation checks in nested create operations.
555-627: LGTM! Comprehensive error validation tests.Excellent coverage of error cases including:
- Non-relation field usage
- Array field usage
- Invalid operation parameters
The error messages are clear and helpful for developers.
629-692: LGTM! Good cyclic dependency detection.The tests properly validate that the system detects both direct and indirect cyclic dependencies in relation checks, preventing infinite recursion.
694-735: LGTM! Query builder integration test.Good to see that the
checkfunction works correctly with the query builder API, ensuring policy enforcement across different query methods.packages/runtime/src/plugins/policy/expression-transformer.ts (3)
23-24: LGTM! Constructor refactoring improves encapsulation.The refactoring to accept a
ClientContractinstead of individual parameters is a good design decision that improves encapsulation and reduces parameter coupling.
74-89: LGTM! Clean property accessors.The getter methods provide clean access to the client's internal properties while maintaining proper encapsulation.
363-380: LGTM! Well-structured function resolution and execution.The updated
transformCallmethod properly:
- Resolves functions through the new
getFunctionImplhelper- Passes the complete client context to functions
- Uses the appropriate alias defaulting to model name
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
samples/blog/main.ts (1)
14-14: Style: make both sides of whereRef explicit refs for symmetry (optional).
Current form is valid; using refs on both sides marginally improves readability and keeps quoting consistent.- .whereRef('Post.authorId', '=', sql.ref(`${modelAlias}.id`)) + .whereRef(sql.ref('Post.authorId'), '=', sql.ref(`${modelAlias}.id`))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/runtime/src/client/contract.ts(1 hunks)packages/runtime/src/plugins/policy/functions.ts(1 hunks)samples/blog/main.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/runtime/src/plugins/policy/functions.ts
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
samples/blog/main.tspackages/runtime/src/client/contract.ts
🧬 Code graph analysis (1)
samples/blog/main.ts (1)
packages/runtime/src/client/index.ts (1)
sql(9-9)
🔇 Additional comments (2)
samples/blog/main.ts (1)
11-11: Approve — sample rename to modelAlias is correct; verify remaining currentModel usagessamples/blog/main.ts uses modelAlias (postCount: (eb, { modelAlias }) =>). ripgrep still finds 'currentModel' in: packages/language/res/stdlib.zmodel:182; packages/language/src/validators/function-invocation-validator.ts:89; packages/runtime/src/client/functions.ts:102; packages/runtime/test/policy/migrated/current-model.test.ts. Confirm whether those occurrences are intentional or need renaming.
packages/runtime/src/client/contract.ts (1)
216-220: Action: avoid value/type name collision — rename exported value or require type-only importsExporting a value
CRUDalongside an existingexport type CRUDcan break TS 5+ consumers withverbatimModuleSyntaxor strict import elision.
Location: packages/runtime/src/client/contract.ts (≈ lines 216–220).Recommended (single source of truth):
export const CRUD_VALUES = ['create', 'read', 'update', 'delete'] as const; export type CRUD = (typeof CRUD_VALUES)[number];If keeping
export const CRUDfor API compatibility, require consumers to useimport type { CRUD }and consider exporting an aliasexport { CRUD as CRUD_VALUES }.Automated verification in the sandbox failed (rg skipped files / missing nl). Re-run locally (use --no-ignore -uu) or paste the file contents so I can re-verify.
Summary by CodeRabbit