fix: support check() policy function call in permission checkers
#1820
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant enhancements to authorization checks and constraints within the policy management system. Key modifications include the addition of new types and methods to handle delegated constraints, improved error handling, and expanded test coverage for permission checks. The changes primarily affect the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
packages/runtime/src/enhancements/node/types.ts (3)
21-24: Enhance JSDoc documentation for the CRUD type.The type definition is correct, but consider expanding the documentation to describe the purpose and usage of this type in the context of permission checking.
/** - * CRUD operations + * Represents the standard CRUD (Create, Read, Update, Delete) operations + * that can be performed on a model. This type is used in permission + * checking to specify which operation is being delegated. */ export type CRUD = 'create' | 'read' | 'update' | 'delete';
82-91: Enhance documentation for DelegateConstraint type.The type definition correctly implements the delegation mechanism needed for
check(parent)policy support. Consider enhancing the documentation with an example./** * Constraint delegated to another model through `check()` function call * on a relation field. + * + * @example + * // In a policy definition: + * check(parent) // Delegates to parent model's policy + * check(owner, 'read') // Delegates read operation to owner model's policy */ export type DelegateConstraint = { kind: 'delegate'; model: string; relation: string; operation?: CRUD; };
93-101: Improve type union formatting for better readability.The addition of
DelegateConstraintto the union type is correct and necessary.export type PermissionCheckerConstraint = - | ValueConstraint - | VariableConstraint - | ComparisonConstraint - | LogicalConstraint - | DelegateConstraint; + | ValueConstraint // Constant value constraints + | VariableConstraint // Free variable constraints + | ComparisonConstraint // Comparison operations + | LogicalConstraint // Logical operations (AND, OR, NOT) + | DelegateConstraint; // Delegated model checkstests/integration/tests/enhancements/with-policy/checker.test.ts (2)
694-715: Consider expanding test coverage for explicit policy delegation.While the test verifies the negative case (denial of read operation), it should also verify:
- The positive case where read is allowed
- Other operations (create, update, delete) with explicit operation checking
Consider adding these test cases:
it('supports policy delegation explicit', async () => { const { enhance } = await load( ` model Foo { id Int @id @default(autoincrement()) model Model? @@allow('all', true) @@deny('update', true) } model Model { id Int @id @default(autoincrement()) foo Foo @relation(fields: [fooId], references: [id]) fooId Int @unique @@allow('read', check(foo, 'update')) } `, { preserveTsFiles: true } ); await expect(enhance().model.check({ operation: 'read' })).toResolveFalsy(); + // Test other operations + await expect(enhance().model.check({ operation: 'create' })).toResolveFalsy(); + await expect(enhance().model.check({ operation: 'update' })).toResolveFalsy(); + await expect(enhance().model.check({ operation: 'delete' })).toResolveFalsy(); + + // Test with authenticated user + await expect(enhance({ id: 1 }).model.check({ operation: 'read' })).toResolveFalsy(); });
717-762: LGTM: Comprehensive test for combined policy delegation.The test effectively covers:
- Policy delegation with additional conditions (
check(foo) && value > 0)- Combination of allow and deny rules
- Various value conditions and authentication states
One minor suggestion: Consider adding a test case for the delete operation to maintain consistency with other test cases.
await expect(enhance({ id: 1 }).model.check({ operation: 'update', where: { value: 0 } })).toResolveFalsy(); await expect(enhance({ id: 1 }).model.check({ operation: 'update', where: { value: 1 } })).toResolveFalsy(); + + // Test delete operation + await expect(enhance().model.check({ operation: 'delete' })).toResolveFalsy(); + await expect(enhance({ id: 1 }).model.check({ operation: 'delete', where: { value: 1 } })).toResolveTruthy(); + await expect(enhance({ id: 1 }).model.check({ operation: 'delete', where: { value: 0 } })).toResolveFalsy(); });packages/runtime/src/enhancements/node/policy/policy-utils.ts (1)
709-709: Consider refactoring to avoid disabling ESLint'sno-this-aliasruleCurrently,
const that = this;is used along with an ESLint disable comment to aliasthis. You can refactor the code to eliminate the need forthatand the ESLint override. Since thetraverselibrary provides a context wherethisrefers to the traversal context, consider using a different approach to access the outerthis, such as passing it as an argument or leveraging closures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/runtime/src/enhancements/node/policy/policy-utils.ts(3 hunks)packages/runtime/src/enhancements/node/types.ts(2 hunks)packages/schema/src/plugins/enhancer/policy/constraint-transformer.ts(5 hunks)tests/integration/tests/enhancements/with-policy/checker.test.ts(2 hunks)
🔇 Additional comments (3)
packages/runtime/src/enhancements/node/types.ts (1)
Line range hint 21-101: Verify type usage across the codebase.
Let's ensure the new types are properly imported and used in related files.
✅ Verification successful
Let me analyze the usage of these types with one more verification to ensure proper integration:
Based on the analysis of the code, I can now provide a final response:
Types are properly integrated and used across the codebase
The verification shows proper integration:
-
The
DelegateConstrainttype is correctly used in:packages/schema/src/plugins/enhancer/policy/constraint-transformer.tsfor transforming policy constraintspackages/runtime/src/enhancements/node/policy/policy-utils.tsfor policy enforcement
-
The
CRUDtype is properly used throughout the codebase for:- Policy definitions and operations
- Permission checking
- API handlers across different frameworks (Express, Fastify, Next.js, SvelteKit)
The types are well-integrated into the policy enforcement system and are used consistently across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check imports of the new types
echo "Checking imports of new types..."
rg -l "import.*(?:CRUD|DelegateConstraint).*from.*types"
# Check usage of DelegateConstraint
echo -e "\nChecking DelegateConstraint usage..."
rg "kind:\s*['\"]delegate['\"]" --type ts
# Check for potential missed imports
echo -e "\nChecking for potential missed imports..."
rg "(?:CRUD|DelegateConstraint)" --type ts
Length of output: 4354
Script:
#!/bin/bash
# Check the implementation in constraint-transformer.ts to verify proper type usage
echo "Checking constraint-transformer.ts implementation..."
cat packages/schema/src/plugins/enhancer/policy/constraint-transformer.ts
# Check policy-utils.ts to verify proper imports and type usage
echo -e "\nChecking policy-utils.ts implementation..."
cat packages/runtime/src/enhancements/node/policy/policy-utils.ts
Length of output: 75188
tests/integration/tests/enhancements/with-policy/checker.test.ts (2)
360-360: LGTM: Improved test name clarity.
The rename from "unsolvable" to "unsatisfiable" better reflects the mathematical terminology used in constraint satisfaction problems.
653-692: LGTM: Well-structured test for basic policy delegation.
The test effectively covers:
- Policy delegation from
ModeltoFoousingcheck(foo) - Permission inheritance for read, create, and update operations
- Proper authentication checks through the relationship chain
fixes #1612