feat: field-level access control #557
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. 📝 WalkthroughWalkthroughAdds field-level policy enforcement with guarded per-field selections and nested guarded subqueries; disallows field-level policies on relation and computed fields; narrows allowed policy operations; improves DB query error messages; adds column-name resolver; refactors join-scope construction; and adds extensive policy tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 Field-Level Access Control PR ReviewI've thoroughly reviewed the field-level access control implementation. Here's my detailed analysis: Review Completed ✅
Code Quality Assessment🟢 Strengths
🟡 Areas for Improvement
🔴 Potential Issues
🛠 Technical Implementation
Recommendations
Security Assessment ✅The field-level access control implementation follows secure design principles:
This is a well-implemented feature that significantly enhances ZenStack's access control capabilities while maintaining the existing security model. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/plugins/policy/src/policy-handler.ts (1)
562-570: Fix return type annotation.The return type is inferred as
unknownbut should bebooleanfor type safety.🔎 Proposed fix
- private hasFieldLevelPolicies(model: string): unknown { + private hasFieldLevelPolicies(model: string): boolean { const modelDef = QueryUtils.getModel(this.client.$schema, model); if (!modelDef) { return false; } - return Object.values(modelDef.fields).some((fieldDef) => + return !!Object.values(modelDef.fields).some((fieldDef) => fieldDef.attributes?.some((attr) => ['@allow', '@deny'].includes(attr.name)), ); }tests/e2e/orm/policy/migrated/field-level-policy.test.ts (2)
72-74: Duplicate test block.Lines 72-74 are identical to lines 64-66. This appears to be unintentional duplication.
Suggested fix: remove duplicate
- r = await db.user.findUnique({ where: { id: 1 }, select: { models: { select: { y: true } } } }); - expect(r.models[0].y).toBeNull(); - expect(r.models[0].z).toBeUndefined(); - r = await db.model.findUnique({ select: { x: true }, where: { id: 1 } });
592-592: Consider adding a TODO comment explaining why update tests are skipped.The entire
update testsdescribe block (lines 592-1238) is skipped without explanation. Since this is a WIP PR, adding a brief TODO comment would help track what remains to be implemented.Suggested improvement
- describe.skip('update tests', () => { + // TODO: Enable after field-level update policy enforcement is implemented + describe.skip('update tests', () => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/language/src/utils.tspackages/language/src/validators/attribute-application-validator.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/plugins/policy/plugin.zmodelpackages/plugins/policy/src/policy-handler.tspackages/plugins/policy/src/utils.tstests/e2e/orm/policy/migrated/field-level-policy.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
**/*.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zmodel: Always runzenstack generateafter modifying ZModel schemas
ZModel schema files should define database structure and policies that compile to TypeScript viazenstack generate
Files:
packages/plugins/policy/plugin.zmodel
🧠 Learnings (6)
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/plugins/policy/src/utils.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/language/src/validators/attribute-application-validator.tspackages/plugins/policy/src/policy-handler.tspackages/language/src/utils.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/orm/src/client/crud/operations/base.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/orm/src/client/crud/operations/base.tstests/e2e/orm/policy/migrated/field-level-policy.test.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/orm/src/client/crud/operations/base.tstests/e2e/orm/policy/migrated/field-level-policy.test.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
🧬 Code graph analysis (4)
packages/orm/src/client/crud/operations/base.ts (1)
packages/orm/src/client/errors.ts (1)
createDBQueryError(122-129)
tests/e2e/orm/policy/migrated/field-level-policy.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(258-269)
packages/orm/src/client/executor/zenstack-query-executor.ts (2)
packages/orm/src/client/errors.ts (1)
createDBQueryError(122-129)packages/orm/src/client/executor/zenstack-driver.ts (1)
compiledQuery(181-188)
packages/language/src/utils.ts (2)
packages/language/src/generated/ast.ts (2)
DataField(333-340)DataField(342-342)packages/sdk/src/model-utils.ts (1)
hasAttribute(59-64)
⏰ 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). (5)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
- GitHub Check: build-test (22.x, sqlite)
- GitHub Check: build-test (22.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (16)
packages/language/src/utils.ts (1)
152-157: LGTM!The
isComputedFieldutility follows the established pattern ofisRelationshipFieldand correctly uses the existinghasAttributehelper. This provides a clean abstraction for checking computed fields in policy validation.packages/orm/src/client/executor/zenstack-query-executor.ts (2)
118-123: LGTM! Improved error visibility for query failures.Including the error details in the message improves debuggability while still preserving the original error as the
cause. ThecreateDBQueryErrorhelper also extractsdbErrorMessageseparately, providing comprehensive error context.
491-496: Consistent error handling.Same pattern applied here as in
executeQuery, ensuring uniform error reporting across both query execution paths.packages/orm/src/client/crud/operations/base.ts (1)
183-185: LGTM!Consistent with the error handling improvements in
zenstack-query-executor.ts, providing better error visibility for query failures in the read path.packages/plugins/policy/src/utils.ts (1)
159-174: LGTM!The
getColumnNameutility correctly mirrors thegetTableNamepattern, recursively unwrappingAliasNodeandReferenceNodeto extract the underlying column name. This provides necessary support for column-name resolution in policy evaluation.packages/plugins/policy/src/policy-handler.ts (3)
583-592: Verify the default allow behavior for field-level policies.When no
@allowrules exist for a field, access is allowed by default (line 588). This differs from model-level policies where no allows means denial. Confirm this is the intended behavior—fields are accessible by default unless explicitly restricted.
251-305: Good separation of field-level vs model-level policy handling.The refactored
transformSelectQueryappropriately distinguishes between field-level and model-level policies:
- Field-level: Creates nested queries with per-field
CASE WHENguards- Model-level: Merges filters into WHERE for simpler SQL generation
This design maintains SQL efficiency when field-level policies aren't needed.
442-503: LGTM! Post-update policy support.The implementation correctly:
- Collects fields accessed via
before()in policies- Loads pre-update entity state
- Ensures ID fields are included for entity matching
- Validates before/after row ID matching
packages/plugins/policy/plugin.zmodel (2)
13-16: LGTM! Narrowed operation scope for field-level policies.The restriction to
'read','update', and'all'for field-level@allowmakes semantic sense—createanddeleteoperate at the entity level, not individual fields.
29-32: Consistent with @Allow changes.The
@denyfield-level attribute properly mirrors the@allowoperation restrictions.As per coding guidelines, remember to run
zenstack generateafter modifying ZModel schemas.packages/language/src/validators/attribute-application-validator.ts (1)
265-281: LGTM! Comprehensive field-level policy validation.The validator correctly enforces that field-level policies:
- Only support
read,update, andalloperations- Cannot be applied to relation fields (which would require nested access control)
- Cannot be applied to computed fields (which don't have stored values)
Clear, actionable error messages help users understand the constraints.
tests/e2e/orm/policy/migrated/field-level-policy.test.ts (5)
1-3: LGTM!Imports are minimal and appropriate for the test file.
163-182: LGTM!Good test validating the interaction between model-level and field-level read policies. The test correctly verifies that when
x = 1, the model is readable (satisfies@@allow('read', x > 0)) but the fieldxitself is null (fails@allow('read', x > 1)).
184-259: Skipped test may need schema update when enabled.This test is marked as skipped pending override implementation. However, line 200 applies a field-level policy to a relation field (
owner User @relation(...) @allow('read', x > 1, true)), which would be rejected by the validation added in this PR. When this test is enabled, the schema will need to be restructured to avoid field-level policies on relation fields.
567-589: LGTM!Good validation test ensuring that field-level policies on relation fields are properly rejected. This aligns with the PR's validation changes.
4-1438: Comprehensive E2E test suite for field-level policies.The test file provides thorough coverage of field-level policy enforcement across various query patterns including:
- Read operations with
@allowand@denyrules- Model-level and field-level policy interactions
- Auth-based and relation-based policy conditions
- Multiple ORM APIs and query builder
- Validation of constraints on relation fields
The coverage aligns well with the guideline that E2E tests should validate real-world schema compatibility. Based on learnings, ORM package tests should include comprehensive policy tests, which this file accomplishes well.
There was a problem hiding this comment.
Pull request overview
This WIP PR implements field-level access control for ZenStack, enabling fine-grained authorization at the field level rather than just at the model level. The implementation adds @allow and @deny attributes that can be applied to individual fields to control read and update operations.
Key changes:
- Field-level policy attributes for
readandupdateoperations (excludingcreateanddelete) - SQL generation using CASE WHEN expressions to conditionally return NULL for restricted fields
- Integration with existing model-level policies and validation framework
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/orm/policy/migrated/field-level-policy.test.ts | Comprehensive test suite covering field-level read/update policies with various scenarios including auth context, relations, and nested queries |
| packages/plugins/policy/src/policy-handler.ts | Core implementation of field-level policies using nested SELECT queries with CASE WHEN guards and policy filter compilation |
| packages/plugins/policy/src/utils.ts | Added utility function getColumnName for extracting column names from operation nodes and exported isTrueNode for policy evaluation |
| packages/plugins/policy/plugin.zmodel | Updated attribute definitions to document field-level @allow and @deny supporting only 'read' and 'update' operations |
| packages/language/src/validators/attribute-application-validator.ts | Enhanced validation to reject field-level policies on relation and computed fields, and disallow before() expressions |
| packages/language/src/utils.ts | Added isComputedField utility function to identify computed fields |
| packages/orm/src/client/executor/zenstack-query-executor.ts | Enhanced error messages to include error details for better debugging |
| packages/orm/src/client/crud/operations/base.ts | Enhanced error messages to include error details for better debugging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Claude finished @ymc9's task —— View job Field-Level Access Control PR ReviewI've thoroughly reviewed the field-level access control implementation. Here's my detailed analysis: Review Completed ✅
Code Quality Assessment🟢 Strengths
🟡 Areas for Improvement
🔴 Potential Issues
🛠 Technical Implementation
Recommendations
Security Assessment ✅The field-level access control implementation follows secure design principles:
This is a well-implemented feature that significantly enhances ZenStack's access control capabilities while maintaining the existing security model. |
…stack-v3 into feat/field-policy
|
Claude finished @ymc9's task —— View job Field-Level Access Control PR ReviewI've thoroughly reviewed the field-level access control implementation. Here's my detailed analysis: Review Completed ✅
Code Quality Assessment🟢 Strengths
🟡 Areas for Improvement
🔴 Potential Issues
🛠 Technical Implementation
Recommendations
Security Assessment ✅The field-level access control implementation follows secure design principles:
This is a well-implemented feature that significantly enhances ZenStack's access control capabilities while maintaining the existing security model. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/plugins/policy/src/policy-handler.ts (2)
261-305: LGTM with note on grammar.The dual-path approach is well-designed: when field-level policies exist, nested queries with CASE guards provide per-field access control; otherwise, simpler SQL with direct WHERE clause filters suffices.
Note: Line 273 comment has a grammatical error ("policies is" should be "policies are"), as already flagged in previous reviews.
562-570: Correct the return type.The return type should be
booleaninstead ofunknown, as already noted in previous reviews. The method returns a boolean value from the.some()call orfalse.
🧹 Nitpick comments (2)
packages/plugins/policy/src/policy-handler.ts (2)
572-605: Consider tightening the parameter type for consistency.The
operationparameter is typed asExclude<CRUD_EXT, 'delete'>, which includes'create' | 'read' | 'update' | 'post-update'. However,FieldLevelPolicyOperations(used at call sites) excludes both'create'and'delete', resulting in'read' | 'update' | 'post-update'.While the current wider type is technically safe (call sites pass narrower types), aligning the parameter type with
FieldLevelPolicyOperationswould make the intent clearer and prevent potential misuse.💡 Suggested refinement
- private buildFieldPolicyFilter(model: string, field: string, operation: Exclude<CRUD_EXT, 'delete'>) { + private buildFieldPolicyFilter(model: string, field: string, operation: FieldLevelPolicyOperations) {
1091-1120: Consider extracting the duplicated helper function.The
extractOperationshelper (lines 1095-1102) is duplicated betweengetFieldPoliciesandgetModelPolicies(lines 1060-1067). Extracting this to a shared private method would reduce duplication.💡 Suggested refactor
Extract to a class-level private method:
private extractPolicyOperations(expr: Expression): PolicyOperation[] { invariant(ExpressionUtils.isLiteral(expr), 'expecting a literal'); invariant(typeof expr.value === 'string', 'expecting a string literal'); return expr.value .split(',') .filter((v) => !!v) .map((v) => v.trim()) as PolicyOperation[]; }Then use in both methods:
operations: this.extractPolicyOperations(attr.args![0]!.value),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/policy/src/policy-handler.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/plugins/policy/src/policy-handler.ts
🧬 Code graph analysis (1)
packages/plugins/policy/src/policy-handler.ts (8)
packages/orm/src/client/contract.ts (2)
CRUD_EXT(260-260)CRUD_EXT(270-270)packages/plugins/policy/src/utils.ts (6)
isTrueNode(36-38)conjunction(50-68)isBeforeInvocation(176-178)trueNode(22-24)disjunction(70-88)buildIsFalse(128-140)packages/orm/src/client/plugin.ts (1)
ProceedKyselyQueryFunction(197-197)packages/schema/src/expression.ts (1)
MemberExpression(27-31)packages/language/src/utils.ts (1)
isBeforeInvocation(469-471)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/plugins/policy/src/types.ts (2)
Policy(17-21)PolicyOperation(12-12)packages/schema/src/expression-utils.ts (1)
ExpressionUtils(19-123)
⏰ 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). (3)
- GitHub Check: claude-review
- GitHub Check: build-test (22.x, postgresql)
- GitHub Check: build-test (22.x, sqlite)
🔇 Additional comments (11)
packages/plugins/policy/src/policy-handler.ts (11)
2-3: LGTM!The new imports (
CRUD_EXT,ProceedKyselyQueryFunction,RejectedByPolicyReason,isTrueNode) are correctly added and appropriately used throughout the file to support field-level policy functionality.Also applies to: 57-57
65-65: LGTM!The
FieldLevelPolicyOperationstype correctly excludes'create'and'delete'operations, as field-level policies apply only to read and update operations on individual fields, not to entity-level creation or deletion.
79-80: LGTM!The region markers (
#region main entry point,#endregion) improve code organization and make the file structure clearer.Also applies to: 233-234
98-99: LGTM!The early rejection check for non-existent models enhances security by failing fast before attempting any mutation operations.
256-258: LGTM!The early rejection check for non-existing tables in the FROM clause is a good security enhancement that prevents queries against undefined models.
308-341: LGTM!The
transformJoinoverride correctly handles join tables with field-level policies by wrapping them in nested queries with guarded selections, while leaving tables without policies unchanged for simpler SQL generation.
424-426: LGTM!The rejection check for non-existing tables in DELETE queries with USING clauses maintains consistent security validation across all query types.
442-502: LGTM!The post-update scaffolding is well-designed:
loadBeforeUpdateEntitiesefficiently loads only the fields accessed inbefore()invocations- The custom
ExpressionVisitorcorrectly traverses policy expressions to collect field accesses- The invariant at line 478 ensures
before()is only used with scalar field access (e.g.,before(this).statusnotbefore(this).user.status)- ID fields are automatically included for proper row matching
508-542: LGTM!The
createSelectAllFieldsWithPoliciesmethod correctly:
- Filters out relation, computed, and inherited fields (lines 516-519), as field-level policies only apply to owned scalar fields
- Creates CASE guards for each field
- Applies model-level policies at the nested query level
- Returns both the query and a flag indicating whether policies exist
544-560: LGTM!The
createFieldSelectionWithPolicymethod generates the correct SQL pattern (CASE WHEN <filter> THEN <field> ELSE NULL END) to guard individual field access based on field-level policies.
1215-1222: LGTM!The
tryRejectNonexistingTableshelper provides consistent validation across query types, correctly handling cases where table names cannot be extracted (e.g., subqueries).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/plugins/policy/src/policy-handler.ts (2)
269-314: Complex but correct field-level policy handling.The two-branch approach is well-designed:
- With field-level policies: wraps each FROM table in a nested query with CASE WHEN guards per field
- Without field-level policies: merges model-level filters directly into WHERE for simpler SQL
Note: Line 281's comment has a grammatical error ("policies is" should be "policies are") that was flagged in a previous review.
573-581: Missing explicit return type annotation.The method returns
booleanbut lacks an explicit type annotation. While TypeScript infers it correctly, an explicit annotation improves code clarity.🔎 Suggested fix
- private hasFieldLevelPolicies(model: string) { + private hasFieldLevelPolicies(model: string): boolean { const modelDef = QueryUtils.getModel(this.client.$schema, model);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/orm/src/client/executor/name-mapper.tspackages/plugins/policy/src/policy-handler.tstests/e2e/orm/policy/migrated/field-level-policy.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/orm/policy/migrated/field-level-policy.test.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/orm/src/client/executor/name-mapper.tspackages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/orm/src/client/executor/name-mapper.tspackages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/orm/src/client/executor/name-mapper.tspackages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/plugins/policy/src/policy-handler.ts
⏰ 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). (2)
- GitHub Check: build-test (22.x, sqlite)
- GitHub Check: build-test (22.x, postgresql)
🔇 Additional comments (11)
packages/orm/src/client/executor/name-mapper.ts (2)
81-89: LGTM! Incremental scope accumulation correctly matches SQL semantics.The refactored approach properly ensures that each join is processed with visibility to all FROM tables and previous joins, which aligns with SQL's join scoping rules. The incremental accumulation of scopes (starting with FROM scopes, then extending with each processed join) is both correct and more explicit than a functional batching approach.
93-93: Correct use of cumulative scopes for outer transformation.The cumulative scopes variable now includes all FROM and join scopes, providing the correct visibility for transforming selections, WHERE clauses, and other query components that may reference any table or join in the query.
packages/plugins/policy/src/policy-handler.ts (9)
2-3: LGTM: Import optimizations.Type-only imports for
CRUD_EXTandProceedKyselyQueryFunctionimprove tree-shaking, and theisTrueNodeimport is properly used in the new field-level policy logic.Also applies to: 57-57
79-79: LGTM: Helpful code organization.The region markers clearly delineate the policy handling lifecycle (pre-mutation, execution, post-mutation, read-back) and separate concerns (field-level vs model-level policies), improving code navigation and maintainability.
Also applies to: 101-101, 131-131, 137-137, 139-139, 218-218, 221-221, 238-238, 241-241, 450-450, 513-513, 514-514, 618-618
264-265: LGTM: Table existence validation.Properly validates that all tables in the FROM clause exist before proceeding with query transformation.
316-349: LGTM: Consistent join policy handling.The transformation correctly:
- Rejects non-existent models (line 324)
- Preserves many-to-many join tables as-is (lines 326-329)
- Wraps policy-protected tables in nested queries (lines 333-348)
The logic flow is sound: after
tryRejectNonexistentModel, the model either exists or is a m2m join table.
432-433: LGTM: USING clause validation.Consistently validates table existence in DELETE USING clauses, preventing unauthorized access attempts via non-existent table references.
452-513: LGTM: Thorough post-update policy enforcement.The before/after comparison logic is complex but necessary to enforce policies that reference pre-update field values via
before(). The visitor pattern correctly extracts field accesses, and id fields are properly included for row matching.
516-571: LGTM: Well-structured field-level policy implementation.The nested query approach with CASE WHEN guards per field is correct:
- Excludes relation/computed/inherited fields appropriately (lines 524-527)
- Combines field-level and model-level policies (lines 537-540)
- Uses NULL as the fallback for denied field access (line 566)
The implementation correctly handles both field-level and model-level policy evaluation.
583-616: LGTM: Field policy filter logic mirrors model-level policies.The default-allow behavior (line 599) for field-level policies is appropriate, contrasting with model-level policies that default-deny most operations. The allow/deny combination logic is correctly implemented.
1226-1233: LGTM: Table validation helper.Simple, focused helper that delegates to
tryRejectNonexistentModelfor consistent validation across table lists.
|
Claude finished @ymc9's task —— View job Field-Level Access Control PR ReviewI've thoroughly reviewed the field-level access control implementation. Here's my detailed analysis: Review Completed ✅
Code Quality Assessment🟢 Strengths
🟡 Areas for Improvement
🔴 Potential Issues
🛠 Technical Implementation
Recommendations
Security Assessment ✅The field-level access control implementation follows secure design principles:
This is a well-implemented feature that significantly enhances ZenStack's access control capabilities while maintaining the existing security model. |
|
Claude finished @ymc9's task —— View job Field-Level Access Control PR ReviewI've thoroughly reviewed the field-level access control implementation. Here's my detailed analysis: Review Completed ✅
Code Quality Assessment🟢 Strengths
🟡 Areas for Improvement
🔴 Potential Issues
🛠 Technical Implementation
Recommendations
Security Assessment ✅The field-level access control implementation follows secure design principles:
This is a well-implemented feature that significantly enhances ZenStack's access control capabilities while maintaining the existing security model. |
|
Claude finished @ymc9's task —— View job Field-Level Access Control PR ReviewI've thoroughly reviewed the field-level access control implementation. Here's my detailed analysis: Review Completed ✅
Code Quality Assessment🟢 Strengths
🟡 Areas for Improvement
🔴 Potential Issues
🛠 Technical Implementation
Recommendations
Security Assessment ✅The field-level access control implementation follows secure design principles:
This is a well-implemented feature that significantly enhances ZenStack's access control capabilities while maintaining the existing security model. |
ymc9
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TODO.md (1)
102-107: Fix markdown list indentation to comply with linting rules.The nested items under "Access Policy" use 4-space indentation but the markdown linter (MD007) expects 2-space indentation for unordered list nesting.
🔎 Proposed fix
- [ ] Access Policy - - [ ] Short-circuit pre-create check for scalar-field only policies - - [x] Field-level policies - - [x] Inject "on conflict do update" - - [x] `check` function - - [ ] Custom functions - - [x] Accessing tables not in the schema + - [ ] Short-circuit pre-create check for scalar-field only policies + - [x] Field-level policies + - [x] Inject "on conflict do update" + - [x] `check` function + - [ ] Custom functions + - [x] Accessing tables not in the schema
🧹 Nitpick comments (1)
packages/plugins/policy/src/policy-handler.ts (1)
1112-1176: Document the semantic difference in 'all' operation handling.The policy extraction methods have an intentional but undocumented difference:
- Model-level (line 1139):
'all'explicitly excludes'post-update'- Field-level (line 1172):
'all'includes'post-update'This reflects the different default behaviors (model-level denies by default, field-level allows by default), but the distinction is subtle and could confuse future maintainers.
💡 Suggested documentation
Add clarifying comments to both methods:
.filter( (policy) => + // Note: model-level 'all' excludes 'post-update' because post-update is allowed by default (operation !== 'post-update' && policy.operations.includes('all')) || policy.operations.includes(operation), ),.filter((policy) => policy.operations.includes('all') || policy.operations.includes(operation)), + // Note: field-level 'all' includes 'post-update' for consistency with other operations );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
TODO.mdpackages/plugins/policy/src/policy-handler.tstests/e2e/orm/policy/migrated/field-level-policy.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
🧠 Learnings (7)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/policy/migrated/field-level-policy.test.tsTODO.mdpackages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
tests/e2e/orm/policy/migrated/field-level-policy.test.tspackages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
📚 Learning: 2025-12-30T15:07:10.092Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:10.092Z
Learning: In the zenstackhq/zenstack-v3 codebase, ts-expect-error should be heavily avoided in production code (packages/*). It is acceptable in test files but production code should use proper type annotations, type assertions, or refactoring to resolve type errors instead.
Applied to files:
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/plugins/policy/src/policy-handler.ts
🧬 Code graph analysis (1)
packages/plugins/policy/src/policy-handler.ts (8)
packages/orm/src/client/contract.ts (2)
CRUD_EXT(260-260)CRUD_EXT(270-270)packages/orm/src/client/plugin.ts (1)
ProceedKyselyQueryFunction(197-197)packages/plugins/policy/src/utils.ts (2)
createRejectedByPolicyError(180-189)isBeforeInvocation(176-178)packages/orm/src/client/errors.ts (1)
createRejectedByPolicyError(131-141)packages/schema/src/expression.ts (1)
MemberExpression(27-31)packages/language/src/utils.ts (1)
isBeforeInvocation(469-471)packages/plugins/policy/src/types.ts (2)
Policy(17-21)PolicyOperation(12-12)packages/schema/src/expression-utils.ts (1)
ExpressionUtils(19-123)
🪛 markdownlint-cli2 (0.18.1)
TODO.md
103-103: 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). (3)
- GitHub Check: claude-review
- GitHub Check: build-test (22.x, postgresql)
- GitHub Check: build-test (22.x, sqlite)
🔇 Additional comments (8)
tests/e2e/orm/policy/migrated/field-level-policy.test.ts (1)
1-1519: Comprehensive test coverage for field-level policies.This test suite provides excellent coverage of field-level policy enforcement across read, update, and various edge cases. The tests validate:
- Field-level read/update policies with
@allowand@deny- Interaction between model-level and field-level policies
- Policy evaluation with auth context and relation filters
- All ORM find APIs (findUnique, findFirst, findMany, etc.)
- Query builder integration
- Nested mutations and relation operations
- Collection predicates and special expressions
The test structure is clear and assertions correctly distinguish between denied fields (null) and unselected fields (undefined). The skipped tests for policy override are acceptable for this WIP PR.
Based on learnings, E2E tests should validate real-world schema compatibility. While these tests use synthetic schemas, this is appropriate for feature-level validation of the field-level policy mechanism itself.
packages/plugins/policy/src/policy-handler.ts (7)
1-66: LGTM! Clean import organization and type definition.The separation of type-only and runtime imports is clear, and the
FieldLevelPolicyOperationstype correctly narrows the allowed operations for field-level policies to 'read', 'update', and 'post-update'.
80-230: LGTM! Well-structured mutation pipeline with clear regions.The refactored entry point clearly separates pre-mutation checks, mutation execution, post-mutation work, and read-back validation. The region markers improve code readability, and the extracted helper methods (
preCreateCheck,preUpdateCheck) make the flow easier to follow.
232-297: LGTM! Pre-mutation checks correctly enforce field-level policies.The extracted
preCreateCheckandpreUpdateCheckmethods properly validate policies before mutations:
preCreateCheckoptimizes for constant policies and handles many-to-many join tablespreUpdateCheckprevents partial updates by counting rows that would be rejected by field-level policies and failing the entire operation if any violations existThis all-or-nothing approach maintains transactional consistency.
303-393: LGTM! Smart query transformation based on policy presence.The enhanced
transformSelectQueryandtransformJoinmethods intelligently adapt the generated SQL:
- When field-level policies exist: creates nested queries with
CASE WHENguards for per-field access control- When only model-level policies exist: generates simpler SQL by merging filters directly into WHERE clauses
This optimization balances correctness with SQL simplicity.
395-493: LGTM! Mutation transformations correctly enforce policies.The mutation query transformations properly:
- Apply update policies to
ON CONFLICT DO UPDATEclauses- Validate that all referenced tables exist before proceeding
- Ensure ID fields are returned for post-mutation read-back validation
- Merge join table policy filters into WHERE clauses for updates/deletes with joins
499-661: LGTM! Field-level policy implementation is sound.The field-level policy helpers correctly:
- Load before-update state for post-update validation using
before()expression extraction- Create nested queries with
CASE WHEN <policy> THEN <field> ELSE NULL ENDguards for denied fields- Combine allow/deny policies with appropriate defaults (allow by default for fields)
- Use the
FieldLevelPolicyOperationstype consistentlyThe implementation properly distinguishes between model-level (default deny) and field-level (default allow) policy semantics.
1271-1292: LGTM! Clean helper methods for validation and result processing.The
tryRejectNonexistingTablesmethod properly validates all referenced tables exist, andpostProcessMutationResultcorrectly adjusts the result shape when returning clauses were added internally for read-back purposes.
|
Claude finished @ymc9's task —— View job Field-Level Access Control PR ReviewI've thoroughly reviewed the field-level access control implementation. Here's my detailed analysis: Review Completed ✅
Code Quality Assessment🟢 Strengths
🟡 Areas for Improvement
🔴 Potential Issues
🛠 Technical Implementation
Recommendations
Security Assessment ✅The field-level access control implementation follows secure design principles:
This is a well-implemented feature that significantly enhances ZenStack's access control capabilities while maintaining the existing security model. |
|
Claude finished @ymc9's task —— View job Field-Level Access Control PR ReviewI've thoroughly reviewed the field-level access control implementation. Here's my detailed analysis: Review Completed ✅
Code Quality Assessment🟢 Strengths
🟡 Areas for Improvement
🔴 Potential Issues
🛠 Technical Implementation
Recommendations
Security Assessment ✅The field-level access control implementation follows secure design principles:
This is a well-implemented feature that significantly enhances ZenStack's access control capabilities while maintaining the existing security model. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/language/test/attribute-application.test.ts (1)
297-432: Add test coverage for invalid field-level policy operations.The field-level policy tests validate that 'read', 'update', and 'all' are accepted, but there's no test verifying that invalid operations like 'create', 'delete', or 'post-update' are properly rejected at the field level. According to the AI summary, field-level policies should only support read/update/all operations.
🔎 Suggested test case for invalid field-level operations
Add a test after line 329 to verify invalid operations are rejected:
it('rejects invalid policy kinds for field-level policies', async () => { await loadSchemaWithError( ` datasource db { provider = 'sqlite' url = 'file:./dev.db' } model Foo { id Int @id @default(autoincrement()) x Int @allow('create', true) @@allow('all', true) } `, `Invalid policy rule kind`, ); });Consider also testing 'delete' and 'post-update' rejections for completeness.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/language/test/attribute-application.test.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/language/test/attribute-application.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
packages/language/test/attribute-application.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
packages/language/test/attribute-application.test.ts
⏰ 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). (2)
- GitHub Check: build-test (22.x, postgresql)
- GitHub Check: build-test (22.x, sqlite)
🔇 Additional comments (2)
packages/language/test/attribute-application.test.ts (2)
5-295: LGTM! Comprehensive model-level policy test coverage.The test suite thoroughly validates model-level policy attributes including valid/invalid operations, before() usage restrictions, relation field constraints, and complex policy expressions. The distinction between owned and non-owned relation handling in create policies is properly tested.
434-457: LGTM! Relation optionality validation is correct.The test properly validates that relation and foreign key fields must have consistent optionality. The placement outside the policy-specific describe blocks is appropriate since this is a general relation constraint.
|
Claude Code is working… I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/orm/policy/migrated/field-level-policy.test.ts (1)
184-259: Track TODO for field-level policy override.This test (and similar ones at lines 794-870 and 1389-1416) are skipped pending implementation of field-level policy override functionality. While this is acceptable for incremental development, these TODOs should be tracked.
Would you like me to create tracking issues for implementing the field-level policy override feature and enabling these tests?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
🧠 Learnings (4)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
📚 Learning: 2025-12-30T15:07:10.092Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:10.092Z
Learning: In the zenstackhq/zenstack-v3 codebase, ts-expect-error should be heavily avoided in production code (packages/*). It is acceptable in test files but production code should use proper type annotations, type assertions, or refactoring to resolve type errors instead.
Applied to files:
tests/e2e/orm/policy/migrated/field-level-policy.test.ts
🧬 Code graph analysis (1)
tests/e2e/orm/policy/migrated/field-level-policy.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(258-269)
⏰ 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). (3)
- GitHub Check: build-test (22.x, sqlite)
- GitHub Check: claude-review
- GitHub Check: build-test (22.x, postgresql)
🔇 Additional comments (8)
tests/e2e/orm/policy/migrated/field-level-policy.test.ts (8)
1-4: LGTM - Clean test structure.The imports and test organization follow standard patterns for e2e testing with vitest and the testtools framework.
6-161: LGTM - Comprehensive field-level read policy coverage.This test thoroughly validates field-level read policies across multiple scenarios:
- Create/update/delete read-back operations
- Various ORM find APIs (findUnique, findFirst, findMany)
- Explicit selection vs. default selection
- Nested relation queries
- Count and aggregate operations
The assertions correctly distinguish between denied fields (null) and unselected fields (undefined).
1485-1513: LGTM - Correct 'this' expression evaluation.The test correctly validates that field-level policies using
auth() == thisproperly restrict access based on the authenticated user's identity. The assertions correctly expectnullfor denied access.
567-589: LGTM - Good validation of constraint.The test correctly validates that field-level policies cannot be applied to relation fields, which is an important constraint for this feature.
591-718: LGTM - Excellent edge case coverage.These tests validate important edge cases:
- Computed fields based on non-readable fields evaluate to null
- Query builder synthesized selections (expressions, function calls) respect field-level policies
This demonstrates the policy enforcement extends beyond simple field access to computed and synthesized values.
721-1482: LGTM - Comprehensive update policy coverage.The update tests thoroughly validate field-level update policies across:
- Simple updates
- Nested to-many and to-one relations
- Connect/disconnect operations
- updateMany operations
- Query builder updates
- Interaction with model-level policies
The test patterns correctly distinguish between policy violations (toBeRejectedByPolicy) and not-found scenarios (toBeRejectedNotFound).
1484-1681: LGTM - Good coverage of advanced scenarios.The misc tests cover important advanced scenarios:
thisexpression in policies- Collection predicates with nested conditions
- Deny-only policies (with and without field access)
These validate the flexibility and composability of the policy system.
1-1682: Excellent comprehensive test coverage for field-level policies.This e2e test suite provides thorough validation of the field-level policy feature, covering:
✓ Read and update operations across all ORM APIs
✓ Nested relations and mutations
✓ Query builder integration
✓ Edge cases (computed fields, synthesized selections)
✓ Interaction with model-level policies
✓ Real-world patterns (user ownership, admin access, role-based permissions)The test patterns consistently distinguish between:
- Denied fields (policy violation) →
null- Unselected fields (not in selection) →
undefined- Allowed fields → actual value
As per coding guidelines, these tests validate real-world schema compatibility with common access control patterns.
Based on learnings, e2e tests should validate real-world schema compatibility with established projects, which this test suite accomplishes through comprehensive policy scenario coverage.
|
Claude Code is working… I'll analyze this and get back to you. |
|
Claude finished @ymc9's task —— View job Field-Level Access Control PR ReviewI've thoroughly reviewed the field-level access control implementation. Here's my detailed analysis: Review Completed ✅
Code Quality Assessment🟢 Strengths
🟡 Areas for Improvement
🔴 Potential Issues
🛠 Technical Implementation
Recommendations
Security Assessment ✅The field-level access control implementation follows secure design principles:
This is a well-implemented feature that significantly enhances ZenStack's access control capabilities while maintaining the existing security model. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/plugins/policy/src/policy-handler.ts (1)
626-632: Consider explicit boolean return type.The method correctly checks for field-level policies, but the return type is implicit. As noted in a past review, explicitly declaring
: booleanwould improve type clarity.🔎 Suggested improvement
- private hasFieldLevelPolicies(model: string, operation: FieldLevelPolicyOperations) { + private hasFieldLevelPolicies(model: string, operation: FieldLevelPolicyOperations): boolean {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/policy/src/policy-handler.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/plugins/policy/src/policy-handler.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/plugins/policy/src/policy-handler.ts
⏰ 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). (3)
- GitHub Check: claude-review
- GitHub Check: build-test (22.x, sqlite)
- GitHub Check: build-test (22.x, postgresql)
🔇 Additional comments (13)
packages/plugins/policy/src/policy-handler.ts (13)
2-3: LGTM! Import organization follows best practices.The type-only imports for
CRUD_EXTandProceedKyselyQueryFunctionare correctly separated, and the new utility function imports (isTrueNode,logicalNot) are appropriately added to support field-level policy logic.Also applies to: 57-59
66-66: Type alias is well-defined.The
FieldLevelPolicyOperationstype correctly excludes'create'and'delete'operations, which aligns with the PR's objective to narrow allowed policy operations for field-level policies.Note: A past review comment has already flagged that method signatures at lines 583 and 1102 should use this type alias for consistency.
232-250: LGTM! Clean extraction of pre-create logic.The
preCreateCheckmethod properly:
- Handles many-to-many join tables (which can't have policies)
- Optimizes by checking constant policies first
- Short-circuits when unconditionally allowed or denied
252-297: LGTM! Pre-update field-level policy enforcement is well-designed.This method prevents partial updates where field-level policies would reject access to specific fields. The approach:
- Extracts fields being updated
- Builds a conjunction of field-level update policies
- Counts rows that would be rejected (using
SUM(CAST(NOT <filter> AS integer)))- Rejects the entire update if any rows would be affected
This conservative approach ensures data consistency by preventing updates that would violate field-level policies.
303-358: LGTM! Smart optimization for field-level policies in SELECT queries.The implementation intelligently branches:
- With field-level policies: Uses nested subqueries with
CASE WHENguards on each field, ensuring field-level access control- Without field-level policies: Directly merges model-level filters into the WHERE clause for simpler, more efficient SQL
This optimization reduces query complexity when field-level policies aren't in use.
360-393: LGTM! JOIN transformation correctly applies policies.The method properly:
- Rejects non-existent models early
- Preserves non-model tables (e.g., many-to-many join tables) as-is
- Replaces model tables with nested guarded subqueries when policies exist
- Keeps joins without policies unchanged for efficiency
563-606: LGTM! Core field-level policy method is well-structured.This method builds the foundation for field-level access control by:
- Iterating through scalar fields (excluding relations, computed, and inherited fields)
- Creating guarded selections via
createFieldSelectionWithPolicy- Applying model-level policy filters
- Optimizing to
SELECT *when no field-level policies exist- Wrapping the result in a properly aliased subquery
608-624: LGTM! Field selection guarding is correctly implemented.Returns either:
- A direct field reference when no policies apply
- A
CASE WHEN <policy> THEN <field> ELSE NULL ENDexpression to mask denied fieldsThis ensures field-level access control at the database level.
634-667: LGTM! Field policy filter correctly combines allow/deny rules.The implementation mirrors the model-level policy logic with an important difference: field access is allowed by default (line 650) when no allow rules are defined, which is appropriate for field-level policies.
1153-1182: LGTM! Field policy extraction is correctly implemented.This method extracts
@allowand@denyattributes from field definitions and filters by operation. Note that line 1178 includes'all'operations without excluding'post-update', which differs from model-level policy handling. This semantic difference has been noted in past reviews and appears intentional.
499-557: LGTM! Post-update policy support is well-implemented.These methods handle post-update policy validation by:
loadBeforeUpdateEntities: Loads pre-update state for rows that will be updatedgetFieldsAccessForBeforeUpdatePolicies: Extracts fields referenced viabefore()in post-update policies using a visitor patternhasPostUpdatePolicies: Checks if post-update policies existThe visitor pattern correctly identifies
before()invocations and ensures ID fields are included for matching rows.
1277-1284: LGTM! Consistent non-existent table rejection.This helper method ensures all tables in FROM/USING clauses are validated, maintaining consistency with the existing
tryRejectNonexistentModelpattern.
1286-1298: LGTM! Result post-processing handles internal RETURNING additions.When the policy handler internally adds a
RETURNINGclause (for ID tracking), this method corrects the result shape to match the original query's expectations, ensuring:
- Rows are cleared when RETURNING wasn't requested
numAffectedRowsis properly set from row count
Summary by CodeRabbit
New Features
Bug Fixes
Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.