feat(policy): support comparing auth() with auth model
#244
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. Caution Review failedThe pull request is closed. WalkthroughThe PR updates devDependencies, tweaks a validation error message, enhances test utilities and adds new language tests, changes NotFoundError to accept optional details and updates its call site, adjusts unary NOT transformation in policy code, and replaces a runtime policy test with two more comprehensive ones. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✨ 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: feat(policy): support comparing
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for comparing auth() with auth model types in policy expressions. The changes enable equality/inequality comparisons between the auth() function and model instances within access control policies.
- Refactored unary expression handling to use logical NOT instead of binary comparison
- Updated error messages for better clarity and specificity
- Added comprehensive test coverage for auth equality comparisons
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/test/policy/auth-equality.test.ts | Added new test file with comprehensive auth equality/inequality test cases |
| packages/runtime/src/plugins/policy/expression-transformer.ts | Simplified unary expression transformation to use logical NOT |
| packages/runtime/src/client/errors.ts | Enhanced NotFoundError constructor with optional details parameter |
| packages/runtime/src/client/crud/operations/base.ts | Updated error message for relation connection failures |
| packages/language/test/utils.ts | Improved test utilities with better error handling and plugin model loading |
| packages/language/test/expression-validationt.test.ts | Added validation tests for model comparison expressions |
| packages/language/src/validators/expression-validator.ts | Updated error message for model comparison validation |
| packages/language/package.json | Reordered dependencies alphabetically |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/language/src/validators/expression-validator.ts (1)
195-201: Prefer strict reference comparison for model typesUse
!==instead of!=when comparingleftTypeandrightType(both objects) to avoid abstract equality semantics.packages/runtime/src/plugins/policy/expression-transformer.ts (1)
198-206: Minor clarity: document '^' vs '!' collection semanticsCurrent logic implements: '^' = none (count == 0), '!' = every (negated predicate then count == 0). A short comment would prevent future confusion.
Also applies to: 239-244
packages/runtime/src/client/crud/operations/base.ts (1)
570-574: Great: richer NotFoundError for connect failuresPassing relation context improves debuggability for nested connects. Consider gradually adding similar details to other NotFoundError throws in this file for consistency.
packages/runtime/test/policy/auth-equality.test.ts (1)
63-108: Nice negative coverage for inequalityValidates auth()!=this/author policy behavior. Consider adding a tiny case for
auth() == nullacceptance/rejection semantics in the future.packages/language/test/expression-validationt.test.ts (1)
1-101: File name likely has a stray 't'Suggest renaming to
expression-validation.test.tsfor consistency with suite names and discoverability.packages/language/test/utils.ts (1)
1-3: ESM-safe __dirname, glob usage, and UUID generation
- In ESM, prefer
fileURLToPath(import.meta.url)over__dirname.- Use
globSyncfor clarity withglob@11.- Avoid relying on global
crypto; importrandomUUIDexplicitly.Apply this diff:
-import { glob } from 'glob'; -import fs from 'node:fs'; +import { globSync } from 'glob'; +import fs from 'node:fs'; +import { randomUUID } from 'node:crypto'; +import { fileURLToPath } from 'node:url'; import os from 'node:os'; import path from 'node:path'; import { expect } from 'vitest'; import { loadDocument } from '../src'; +const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ - const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`); + const tempFile = path.join(os.tmpdir(), `zenstack-schema-${randomUUID()}.zmodel`); @@ - const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`); + const tempFile = path.join(os.tmpdir(), `zenstack-schema-${randomUUID()}.zmodel`); @@ -function getPluginModels() { - return glob.sync(path.resolve(__dirname, '../../runtime/src/plugins/**/plugin.zmodel')); -} +function getPluginModels() { + return globSync(path.resolve(__dirname, '../../runtime/src/plugins/**/plugin.zmodel')); +}Also applies to: 41-43
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
packages/language/package.json(1 hunks)packages/language/src/validators/expression-validator.ts(1 hunks)packages/language/test/expression-validationt.test.ts(1 hunks)packages/language/test/utils.ts(2 hunks)packages/runtime/src/client/crud/operations/base.ts(1 hunks)packages/runtime/src/client/errors.ts(1 hunks)packages/runtime/src/plugins/policy/expression-transformer.ts(1 hunks)packages/runtime/test/policy/auth-equality.test.ts(1 hunks)packages/runtime/test/policy/ref-equality.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/runtime/test/policy/ref-equality.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/language/src/validators/expression-validator.tspackages/runtime/src/client/errors.tspackages/runtime/src/plugins/policy/expression-transformer.tspackages/language/test/expression-validationt.test.tspackages/runtime/src/client/crud/operations/base.tspackages/runtime/test/policy/auth-equality.test.tspackages/language/package.jsonpackages/language/test/utils.ts
🧬 Code graph analysis (6)
packages/language/src/validators/expression-validator.ts (2)
packages/language/src/utils.ts (1)
isDataFieldReference(138-140)packages/language/src/generated/ast.ts (1)
isThisExpr(721-723)
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
packages/runtime/src/plugins/policy/utils.ts (1)
logicalNot(88-95)
packages/language/test/expression-validationt.test.ts (1)
packages/language/test/utils.ts (2)
loadSchemaWithError(22-40)loadSchema(9-20)
packages/runtime/src/client/crud/operations/base.ts (1)
packages/runtime/src/client/errors.ts (1)
NotFoundError(27-31)
packages/runtime/test/policy/auth-equality.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/language/test/utils.ts (1)
packages/language/src/index.ts (1)
loadDocument(21-132)
⏰ 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: build-test (20.x)
🔇 Additional comments (6)
packages/language/src/validators/expression-validator.ts (1)
210-216: Error wording change is good; matches testsSwitching to "comparison between models is not supported" is clearer and aligns with new tests.
packages/runtime/src/plugins/policy/expression-transformer.ts (2)
303-309: Unary "!" rewrite via logicalNot is correctThis produces proper SQL
NOT (...)with precedence handled by utils (paren-wrapping And/Or).
252-296: Auth reference equality looks solid (composite IDs included)Building equality on all auth model id fields, with
!=handled via logical NOT, covers composite keys and matches policy behavior.packages/language/package.json (1)
61-70: Dev deps update looks goodAdding
globand consolidating tmp/types supports new test utils. No issues spotted.packages/runtime/src/client/errors.ts (1)
28-30: Optional details on NotFoundError is a useful improvementMessage composition is clear and backward-compatible.
packages/runtime/test/policy/auth-equality.test.ts (1)
4-61: Covers equality paths wellHappy path and policy rejections for auth()==this/author are validated, including connect with composite IDs.
|
Claude finished @ymc9's task —— View job PR Review: feat(policy): support comparing
|
Summary by CodeRabbit