fix(policy): relation filter should respect field-level policies #1495
Conversation
WalkthroughWalkthroughThe recent changes enhance field-level read authorization in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PolicyUtil
participant PolicyGenerator
participant Database
Client->>PolicyUtil: Call getFieldReadAuthGuard()
PolicyUtil->>PolicyUtil: Retrieve field-level read authorization guard
Client->>PolicyGenerator: Request policy generation
PolicyGenerator->>PolicyGenerator: Generate policy expressions and guard functions
Client->>Database: Perform query with injected guards
Database->>Client: Return filtered results based on field-level policies
Assessment against linked issues
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (6)
packages/runtime/src/enhancements/types.ts (1)
Line range hint
19-19: Consider replacinganywith more specific types to enhance type safety and maintainability.- prismaModule?: any; + prismaModule?: typeof PrismaClient;Also applies to: 30-30
packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts (1)
Line range hint
726-728: Consider removing the unnecessary else clause to simplify the control flow and enhance code readability.- } else { - throw new Error('should not happen'); - }packages/runtime/src/enhancements/policy/policy-utils.ts (4)
Line range hint
34-34: Avoid usinganytype forprismaModule.Consider specifying a more precise type for
prismaModuleto enhance type safety and code maintainability.
Line range hint
65-69: Remove unnecessary else clauses.These else clauses are redundant because the previous branches break early. Removing them can simplify the code and improve readability.
Also applies to: 67-69, 79-83, 81-83, 92-96, 94-96, 105-110, 116-118, 378-380
Line range hint
142-142: Specify a more precise type instead ofany.Using
anydisables many type checking rules. Consider specifying more precise types to enhance type safety and maintainability.Also applies to: 143-143, 157-157, 174-174, 191-191, 265-265, 388-388
Line range hint
162-162: Use literal keys directly in the computed expressions.The computed expressions can be simplified by using literal keys directly, which can improve code clarity and reduce complexity.
Also applies to: 179-179, 200-200
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- packages/runtime/src/enhancements/policy/policy-utils.ts (4 hunks)
- packages/runtime/src/enhancements/types.ts (1 hunks)
- packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts (2 hunks)
- tests/integration/tests/enhancements/with-policy/relation-one-to-one-filter.test.ts (4 hunks)
- tests/regression/tests/issue-1454.test.ts (1 hunks)
Additional context used
Biome
packages/runtime/src/enhancements/types.ts
[error] 19-19: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 30-30: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts
[error] 726-728: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
packages/runtime/src/enhancements/policy/policy-utils.ts
[error] 34-34: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 65-69: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 67-69: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 79-83: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 81-83: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 92-96: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 94-96: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 105-110: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 116-118: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 142-142: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 143-143: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 157-157: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 162-162: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 174-174: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 179-179: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 191-191: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 200-200: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 265-265: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 378-380: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 388-388: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
Additional comments not posted (5)
tests/regression/tests/issue-1454.test.ts (3)
1-34: The test caseregression1correctly simulates the scenario where a user's username is used to filter purchases. It ensures that the policy changes are effective in respecting field-level access controls.
36-67: The test caseregression2effectively tests the scenario where the username field is not readable due to explicit policy settings. This ensures that the system respects the field-level access controls as intended.
70-115: The test caseregression3thoroughly tests the new policy logic by using complex filtering conditions involving sensitive information. This ensures robustness in policy enforcement under various scenarios.packages/runtime/src/enhancements/types.ts (1)
217-221: The addition of theguardproperty toFieldReadDefis a crucial enhancement for supporting field-level policy enforcement. This change aligns well with the PR objectives of enhancing field-level authorization.tests/integration/tests/enhancements/with-policy/relation-one-to-one-filter.test.ts (1)
Line range hint
187-231: The test caseisNot filtereffectively checks the system's behavior under various conditions using theisNotfilter. It ensures that the policies are correctly enforced, especially in scenarios involving deleted entities.Also applies to: 264-279
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
packages/runtime/src/enhancements/policy/policy-utils.ts (4)
Line range hint
34-34: Consider replacinganywith a more specific type.Using
anydisables many TypeScript type checking features, which can lead to less robust code. Consider specifying a more precise type forprismaModule.
Line range hint
65-69: Consider removing unnecessaryelseclauses.The
elseclauses in your conditional statements are redundant because all preceding branches of the conditionals end with areturnstatement or break the loop. Removing these can simplify the code and improve readability.Also applies to: 67-69, 79-83, 81-83, 92-96, 94-96, 105-110, 116-118, 378-380
Line range hint
142-142: Consider replacinganywith a more specific type.The use of
anytype at several places in your code reduces the safety and predictability of the code. It's recommended to use more specific types to take full advantage of TypeScript's type system.Also applies to: 143-143, 157-157, 174-174, 191-191, 265-265, 388-388
Line range hint
162-162: Consider using literal keys directly in computed expressions.Instead of using string literals for keys in computed expressions, you can directly use the keys. This simplifies the code and improves its readability.
Also applies to: 179-179, 200-200
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/runtime/src/enhancements/policy/policy-utils.ts (3 hunks)
Additional context used
Biome
packages/runtime/src/enhancements/policy/policy-utils.ts
[error] 34-34: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 65-69: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 67-69: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 79-83: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 81-83: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 92-96: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 94-96: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 105-110: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 116-118: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 142-142: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 143-143: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 157-157: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 162-162: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 174-174: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 179-179: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 191-191: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 200-200: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 265-265: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 378-380: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 388-388: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- tests/integration/tests/enhancements/with-policy/relation-one-to-many-filter.test.ts (2 hunks)
- tests/integration/tests/enhancements/with-policy/relation-one-to-one-filter.test.ts (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/integration/tests/enhancements/with-policy/relation-one-to-one-filter.test.ts
Additional comments not posted (3)
tests/integration/tests/enhancements/with-policy/relation-one-to-many-filter.test.ts (3)
Line range hint
3-449: The tests for basic "Relation one-to-many filter" scenarios are well-structured and cover a variety of cases including presence and absence of related entities, and different filter conditions. This ensures that the basic functionality remains intact after the new changes.
449-828: The addition of tests for "Relation one-to-many filter with field-level rules" is crucial for verifying the new functionality introduced in this PR. These tests effectively check the behavior of the system when field-level access controls are applied, aligning with the PR's objectives to respect field-level policies in relation filters.
829-1026: The tests for "Relation one-to-many filter with field-level override rules" are essential for ensuring that the override policies are correctly applied. These tests validate the system's behavior when exceptions to general access rules are specified, which is a key part of the PR's functionality.
[APROVED]
Fixes #1454