-
-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: field-level access is incorrectly rejected when there're no allow rules #1510
Conversation
WalkthroughWalkthroughThe changes introduce a more detailed conditional logic to the Changes
Sequence Diagram(s)N/A Assessment against linked issues
Warning Review ran into problemsProblems (1)
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/schema/src/plugins/enhancer/policy/utils.ts (1 hunks)
- tests/integration/tests/enhancements/with-policy/field-level-policy.test.ts (2 hunks)
Additional comments not posted (4)
packages/schema/src/plugins/enhancer/policy/utils.ts (1)
Line range hint
1-422
: Ensure comprehensive test coverage for policy utility functions.Given the complexity and importance of the functions in this file, it is crucial to verify that there is comprehensive test coverage, especially for the new logic introduced. This can help prevent regressions and ensure that the policy enforcement behaves as expected under various conditions.
#!/bin/bash # Description: Verify test coverage for policy utility functions. # Test: Search for test cases covering the utility functions. Expect: Tests for all major paths and edge cases. rg --type typescript --files-with-matches $'describe\\(".*utils.*",'tests/integration/tests/enhancements/with-policy/field-level-policy.test.ts (3)
30-30
: The addition of the new fieldz
with a@deny('read', x <= 0)
policy is crucial for testing the changes made in the PR. This line effectively tests the scenario described in the issue, ensuring that the policy does not block reads when it shouldn't.
44-44
: The extensive tests for the visibility of fieldz
across various scenarios are well-implemented. These tests check the read access permissions based on the value ofx
and ensure that the policy is enforced correctly. The tests are comprehensive and cover multiple methods of accessing the data (findUnique
,create
, etc.), which is good for ensuring robustness.
[APROVED]Also applies to: 47-47, 51-51, 55-55, 59-59, 63-63, 67-67, 71-71, 76-76, 81-81, 86-86, 91-91, 97-97, 102-102, 104-104, 107-107, 110-110, 114-114, 116-116, 119-119, 127-127, 132-132, 134-134, 137-137, 139-139, 143-143
134-134
: The test cases for thez
field whenx
is greater than 0 (thus makingz
readable according to the policy) are correctly implemented. These tests validate the fix by ensuring that the policy behaves as expected when the condition is true.Also applies to: 137-137, 139-139
Fixes #1501