fix: should not reject "update" when there's only field-level override but no model-level policy #1052
Conversation
…e but no model-level policy Fixes #1014
WalkthroughWalkthroughThe changes introduce enhancements to policy utilities by simplifying method signatures and adding new functionality to support field-level override guards for access control. Additionally, regression tests have been added to ensure the correct behavior of update and read operations in relation to access policies, specifically addressing the issue where field-level access policies did not override default deny behavior. Changes
Assessment against linked issues
The changes directly address the primary objective of issue #1014 by introducing logic to consider field-level override guards before denying access, which was the core problem described. The addition of 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tests/integration/tests/tsconfig.template.jsonis excluded by:!**/*.json
Files selected for processing (2)
- packages/runtime/src/enhancements/policy/policy-utils.ts (4 hunks)
- tests/integration/tests/regression/issue-1014.test.ts (1 hunks)
Additional comments: 3
packages/runtime/src/enhancements/policy/policy-utils.ts (3)
- 322-322: The simplification of the
hasAuthGuardmethod's signature is mentioned, but the diff does not show a change in the signature, only the method's implementation. It's important to ensure that the method's usage throughout the codebase is consistent with this change.Verification successful
The usages of
hasAuthGuardacross the codebase are consistent with the method's signature provided in the snippet. The simplification mentioned in the review comment likely refers to internal changes within the method that do not affect its external usage. Based on the evidence, the method's usage aligns with the new implementation.* 334-344: The addition of the `hasOverrideAuthGuard` method is a significant improvement for checking field-level override policy guards. This method enhances the flexibility and granularity of access control by allowing field-level policies to override model-level policies for specific operations. It's well-implemented, following the established coding conventions and logic structure seen in the rest of the file. * 749-749: The update to the `tryReject` method to consider field-level override guards before denying access is crucial for the intended functionality of allowing field-level policies to override default behaviors. This change ensures that even if a model-level policy would deny access, field-level overrides can still allow it, which aligns with the PR's objectives. However, it's essential to ensure that this logic does not inadvertently allow access that should be denied based on other security considerations.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of hasAuthGuard to ensure they align with the new implementation. rg "hasAuthGuard" --type tsLength of output: 610
Verification successful
To further verify the integration of field-level override guards and ensure they do not conflict with other security mechanisms, we need to examine the implementation details of the
tryRejectmethod and the context of its calls. Let's proceed with viewing the relevant sections of the code.
The provided context from the shell scripts includes the full implementation of the
PolicyUtilclass inpolicy-utils.tsand various handler implementations inhandler.ts. ThePolicyUtilclass contains thetryRejectmethod, which is crucial for enforcing access policies based on model-level and field-level guards. This method checks if a model-level policy would deny access and then checks for field-level override guards that might allow access despite the model-level denial. This aligns with the original review comment, which emphasizes the importance of considering field-level override guards before denying access to ensure that field-level policies can override default behaviors when necessary.The
tryRejectmethod implementation confirms that it performs the necessary checks for model-level denial (isFalse(guard)) and then checks for field-level override guards (!this.hasOverrideAuthGuard(model, operation)). This logic ensures that access is not denied if there are field-level overrides that allow it, which is consistent with the PR's objectives to allow field-level policies to override default behaviors.Given the detailed implementation provided in the
policy-utils.tsfile and the usage of thetryRejectmethod across various operations inhandler.ts, it's clear that the integration of field-level override guards has been carefully considered and implemented to not conflict with other security mechanisms. The logic intryRejectspecifically addresses the concern raised in the review comment by ensuring that field-level overrides are evaluated before denying access based on model-level policies.Based on the provided context and the analysis of the
tryRejectmethod and its usage, the review comment is verified as accurate. The implementation aligns with the PR's objectives and the comment's emphasis on the importance of field-level override guards in the access control logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the integration of field-level override guards does not conflict with other security mechanisms. rg "tryReject" --type tsLength of output: 1117
Script:
#!/bin/bash # Examine the implementation of tryReject in policy-utils.ts fd policy-utils.ts --exec cat {} # Review the context of tryReject calls in handler.ts fd handler.ts --exec cat {}Length of output: 117605
| describe('issue 1014', () => { | ||
| it('update', async () => { | ||
| const { prisma, enhance } = await loadSchema( | ||
| ` | ||
| model User { | ||
| id Int @id() @default(autoincrement()) | ||
| name String | ||
| posts Post[] | ||
| } | ||
|
|
||
| model Post { | ||
| id Int @id() @default(autoincrement()) | ||
| title String | ||
| content String? | ||
| author User? @relation(fields: [authorId], references: [id]) | ||
| authorId Int? @allow('update', true, true) | ||
|
|
||
| @@allow('read', true) | ||
| } | ||
| `, | ||
| { logPrismaQuery: true } | ||
| ); | ||
|
|
||
| const db = enhance(); | ||
|
|
||
| const user = await prisma.user.create({ data: { name: 'User1' } }); | ||
| const post = await prisma.post.create({ data: { title: 'Post1' } }); | ||
| await expect(db.post.update({ where: { id: post.id }, data: { authorId: user.id } })).toResolveTruthy(); | ||
| }); |
There was a problem hiding this comment.
The test case for update operations correctly sets up a schema with User and Post models, creates a user and a post, and then attempts to update the authorId of the post to simulate the issue. However, there are a few points to consider:
- The
loadSchemafunction is used withlogPrismaQuery: true, which is great for debugging but might clutter test output in CI environments. Consider making logging configurable based on the environment. - The
expectstatement at line 30 usestoResolveTruthy(), which is not a standard Jest matcher. Ensure that a custom matcher or extension is properly configured to handle this assertion. - It's good practice to clean up created data after tests to avoid side effects. Consider adding an
afterEachblock to delete created posts and users.
- await expect(db.post.update({ where: { id: post.id }, data: { authorId: user.id } })).toResolveTruthy();
+ await expect(db.post.update({ where: { id: post.id }, data: { authorId: user.id } })).resolves.not.toThrow();Consider adding cleanup logic and review the custom matcher or extension used for toResolveTruthy().
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| describe('issue 1014', () => { | |
| it('update', async () => { | |
| const { prisma, enhance } = await loadSchema( | |
| ` | |
| model User { | |
| id Int @id() @default(autoincrement()) | |
| name String | |
| posts Post[] | |
| } | |
| model Post { | |
| id Int @id() @default(autoincrement()) | |
| title String | |
| content String? | |
| author User? @relation(fields: [authorId], references: [id]) | |
| authorId Int? @allow('update', true, true) | |
| @@allow('read', true) | |
| } | |
| `, | |
| { logPrismaQuery: true } | |
| ); | |
| const db = enhance(); | |
| const user = await prisma.user.create({ data: { name: 'User1' } }); | |
| const post = await prisma.post.create({ data: { title: 'Post1' } }); | |
| await expect(db.post.update({ where: { id: post.id }, data: { authorId: user.id } })).toResolveTruthy(); | |
| }); | |
| describe('issue 1014', () => { | |
| it('update', async () => { | |
| const { prisma, enhance } = await loadSchema( | |
| ` | |
| model User { | |
| id Int @id() @default(autoincrement()) | |
| name String | |
| posts Post[] | |
| } | |
| model Post { | |
| id Int @id() @default(autoincrement()) | |
| title String | |
| content String? | |
| author User? @relation(fields: [authorId], references: [id]) | |
| authorId Int? @allow('update', true, true) | |
| @@allow('read', true) | |
| } | |
| `, | |
| { logPrismaQuery: true } | |
| ); | |
| const db = enhance(); | |
| const user = await prisma.user.create({ data: { name: 'User1' } }); | |
| const post = await prisma.post.create({ data: { title: 'Post1' } }); | |
| await expect(db.post.update({ where: { id: post.id }, data: { authorId: user.id } })).resolves.not.toThrow(); | |
| }); |
| it('read', async () => { | ||
| const { prisma, enhance } = await loadSchema( | ||
| ` | ||
| model Post { | ||
| id Int @id() @default(autoincrement()) | ||
| title String @allow('read', true, true) | ||
| content String | ||
| } | ||
| `, | ||
| { logPrismaQuery: true } | ||
| ); | ||
|
|
||
| const db = enhance(); | ||
|
|
||
| const post = await prisma.post.create({ data: { title: 'Post1', content: 'Content' } }); | ||
| await expect(db.post.findUnique({ where: { id: post.id } })).toResolveNull(); | ||
| await expect(db.post.findUnique({ where: { id: post.id }, select: { title: true } })).resolves.toEqual({ | ||
| title: 'Post1', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test case for read operations is well-structured, focusing on the ability to read fields with specific access policies. Points to consider:
- The schema definition for the
Postmodel at lines 36-40 includes an@allowdirective on thetitlefield, which aligns with the test's objective. However, thecontentfield does not have an explicit access policy, which might be intentional but could also be an oversight. If testing access policies on multiple fields is desired, consider adding relevant policies. - The use of
await expect(...).toResolveNull();at line 48 suggests expecting a null result, which is not a standard Jest matcher. Ensure that a custom matcher or extension is properly configured for this assertion. - The test validates the access policy on the
titlefield but does not explicitly test the default deny behavior for fields without an@allowdirective. Consider adding an assertion to verify that accessing thecontentfield without explicit permission results in the expected behavior.
- await expect(db.post.findUnique({ where: { id: post.id } })).toResolveNull();
+ await expect(db.post.findUnique({ where: { id: post.id } })).resolves.toBeNull();Review the custom matcher or extension used for toResolveNull() and consider adding more assertions to cover different access policy scenarios.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| it('read', async () => { | |
| const { prisma, enhance } = await loadSchema( | |
| ` | |
| model Post { | |
| id Int @id() @default(autoincrement()) | |
| title String @allow('read', true, true) | |
| content String | |
| } | |
| `, | |
| { logPrismaQuery: true } | |
| ); | |
| const db = enhance(); | |
| const post = await prisma.post.create({ data: { title: 'Post1', content: 'Content' } }); | |
| await expect(db.post.findUnique({ where: { id: post.id } })).toResolveNull(); | |
| await expect(db.post.findUnique({ where: { id: post.id }, select: { title: true } })).resolves.toEqual({ | |
| title: 'Post1', | |
| }); | |
| }); | |
| it('read', async () => { | |
| const { prisma, enhance } = await loadSchema( | |
| ` | |
| model Post { | |
| id Int @id() @default(autoincrement()) | |
| title String @allow('read', true, true) | |
| content String | |
| } | |
| `, | |
| { logPrismaQuery: true } | |
| ); | |
| const db = enhance(); | |
| const post = await prisma.post.create({ data: { title: 'Post1', content: 'Content' } }); | |
| await expect(db.post.findUnique({ where: { id: post.id } })).resolves.toBeNull(); | |
| await expect(db.post.findUnique({ where: { id: post.id }, select: { title: true } })).resolves.toEqual({ | |
| title: 'Post1', | |
| }); | |
| }); |
Fixes #1014
Summary by CodeRabbit