fix(policy): allow auth(). calls in filter functions
#1771
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several modifications to the validation logic within the schema language server. Key changes include the refactoring of 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: 2
🧹 Outside diff range and nitpick comments (6)
tests/regression/tests/issue-1745.test.ts (1)
1-4: Consider using a more descriptive test nameThe current test name 'regression' is quite generic. To improve clarity and maintainability, consider using a more descriptive name that reflects the specific issue being tested, such as 'allows auth() calls in filter functions'.
- it('regression', async () => { + it('allows auth() calls in filter functions', async () => {packages/schema/src/language-server/validator/utils.ts (3)
17-17: Approve new import, but consider removing unused import.The import of
isAuthInvocationis correctly added and used in the new function. However, theresolvedimport doesn't appear to be used in the visible changes of this file.Consider removing the unused import:
-import { isAuthInvocation, resolved } from '@zenstackhq/sdk'; +import { isAuthInvocation } from '@zenstackhq/sdk';
186-188: Approve new function, suggest adding documentation.The
isAuthOrAuthMemberAccessfunction correctly implements the logic to allowauth().calls in filter functions, as per the PR objectives. It handles both direct auth invocations and nested member access expressions, which is crucial for implementing dynamic permission controls based on user roles and types.Consider adding a brief documentation comment to explain the function's purpose and behavior:
+/** + * Checks if the given expression is an auth invocation or a member access to an auth invocation. + * This function allows for nested member access expressions, supporting complex auth checks. + * @param expr The expression to check + * @returns True if the expression is an auth invocation or a member access to an auth invocation + */ export function isAuthOrAuthMemberAccess(expr: Expression): boolean { return isAuthInvocation(expr) || (isMemberAccessExpr(expr) && isAuthOrAuthMemberAccess(expr.operand)); }
Line range hint
1-188: Summary: Changes successfully implement PR objectivesThe modifications to this file effectively address the PR objectives by introducing the
isAuthOrAuthMemberAccessfunction. This addition allows for more flexible and dynamic permission controls based on user roles and types, specifically enablingauth().calls in filter functions.The changes are minimal, focused, and don't affect existing functionality, ensuring backward compatibility. The implementation is correct and aligns well with the requirements outlined in the linked issue #1745.
As the codebase evolves, consider the following to maintain and improve the authorization system:
- Document the new authorization capabilities in the project's documentation to help developers understand and utilize these features correctly.
- Consider creating unit tests specifically for the
isAuthOrAuthMemberAccessfunction to ensure its behavior remains correct as the codebase evolves.- Monitor the usage of this new feature in real-world scenarios to identify any potential performance impacts or edge cases that may need further optimization or handling.
packages/schema/src/language-server/validator/expression-validator.ts (1)
299-299: LGTM: Consistent handling ofauth().callsThe use of the imported
isAuthOrAuthMemberAccessfunction maintains the existing functionality while allowing for consistent handling ofauth().calls across the codebase. This change supports the PR objectives of enablingauth().calls in filter functions.Consider updating the comment to reflect the more general nature of the check:
- // `auth()` access + // `auth()` or `auth().` member access isAuthOrAuthMemberAccess(expr) ||packages/schema/src/language-server/validator/function-invocation-validator.ts (1)
123-129: Consider Adding Unit Tests for the New Validation LogicTo ensure the extended validation logic functions correctly with
auth()expressions, please consider adding unit tests that cover scenarios involvingauth().expressions in both singular and array forms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/schema/src/language-server/validator/expression-validator.ts (2 hunks)
- packages/schema/src/language-server/validator/function-invocation-validator.ts (2 hunks)
- packages/schema/src/language-server/validator/utils.ts (2 hunks)
- packages/schema/tests/schema/validation/attribute-validation.test.ts (4 hunks)
- tests/integration/tests/e2e/filter-function-coverage.test.ts (1 hunks)
- tests/regression/tests/issue-1745.test.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (15)
tests/regression/tests/issue-1745.test.ts (4)
5-7: LGTM: Proper database setup and teardownThe creation and cleanup of the test database are handled correctly. The use of a try-finally block ensures that the database is always dropped, even if the test fails, which is a good practice for test isolation and resource management.
Also applies to: 94-97
10-90: LGTM: Implementedauth().calls in access control rulesThe schema successfully implements
auth().calls in the access control rules, particularly in theAdmodel. This aligns with the PR objective of allowingauth().calls in filter functions and enables more dynamic permission controls based on user roles and types.
8-93: LGTM: Appropriate schema loading for testingThe schema loading process is well-implemented for a test environment. Using the
loadSchemafunction withpushDb: falseensures that the schema can be validated without making actual changes to the database, which is ideal for testing purposes.
1-98: Overall implementation successfully addresses PR objectivesThis regression test file effectively addresses the main objectives of the PR:
- It implements
auth().calls in filter functions, particularly in the access control rules.- It provides a framework for testing polymorphic roles and dynamic permission controls.
The test structure, database handling, and schema definition are well-implemented. Minor suggestions for improvement have been made, including:
- Using a more descriptive test name.
- Considering the addition of an optional
buyerTypefield in theCompanymodel for increased flexibility.These changes would further enhance the test's clarity and the schema's ability to handle polymorphic roles as requested in issue #1745.
tests/integration/tests/e2e/filter-function-coverage.test.ts (2)
39-59: Excellent addition of the 'contains with auth()' test case.This new test case effectively demonstrates the use of
auth().calls in filter functions, which directly aligns with the PR objectives. It covers both positive and negative scenarios, ensuring that the policy works as expected when thestringfield contains the authenticated user's name.The test structure is consistent with other tests in the file, and the assertions logically cover the expected behavior. This addition significantly contributes to the overall coverage of filter functions and validates the new feature.
Line range hint
1-60: Overall, the changes to this file are well-implemented and valuable.The addition of the 'contains with auth()' test case complements the existing tests for other filter functions. It maintains the file's consistent structure and testing approach while enhancing the overall test coverage. This change effectively supports the PR's objective of allowing
auth().calls in filter functions.packages/schema/src/language-server/validator/expression-validator.ts (2)
27-27: LGTM: Improved code organizationThe import of
isAuthOrAuthMemberAccessfrom a separate utility file enhances code organization and reusability. This change aligns well with the PR objectives of allowingauth().calls in filter functions.
Line range hint
1-303: Summary: Successful implementation ofauth().call supportThe changes in this file effectively support the PR objectives of allowing
auth().calls in filter functions. The modifications improve code organization by moving theisAuthOrAuthMemberAccessfunction to a utility file and using it consistently within theExpressionValidatorclass. These changes maintain the existing validation logic while enhancing the flexibility of authorization checks in filter functions.To fully validate the impact of these changes:
Run the following script to check for any unintended side effects or usage of the old implementation:
This script will help ensure that the changes have been consistently applied across the codebase and that there are no remaining issues related to
auth()calls in filter functions.packages/schema/src/language-server/validator/function-invocation-validator.ts (3)
29-29: ImportingisAuthOrAuthMemberAccessCorrectlyThe import statement for
isAuthOrAuthMemberAccessfrom'./utils'is correctly added, allowing the validation logic to utilize this function.
112-113: Extending Validation to Includeauth()ExpressionsThe addition of
!isAuthOrAuthMemberAccess(secondArg)in the validation logic appropriately extends the criteria to accept expressions starting withauth()as valid second arguments. This aligns with the PR objective to allowauth().calls in filter functions.
123-129: Updating Error Message to Reflect New Validation RulesThe error message now accurately informs users that the second argument must be a literal, an enum, an expression starting with
auth()., or an array of them. This provides clearer guidance and improves developer experience when addressing validation errors.packages/schema/tests/schema/validation/attribute-validation.test.ts (4)
819-822: Appropriate addition ofefield inUsermodelThe inclusion of the
efield of enum typeEin theUsermodel is necessary for testing policy expressions involvingauth().e.
848-848: Correct use ofauth().ein thehasfunctionUsing
auth().ewithin thehas(es, auth().e)function accurately tests the new functionality that allowsauth().calls in filter functions.
899-901: Updated error message reflectsauth().expressionsThe error message now includes
auth().expressions as acceptable second arguments, aligning with the enhanced validation logic.
1033-1035: Consistent error message update to includeauth().expressions
packages/schema/src/language-server/validator/function-invocation-validator.ts
Show resolved
Hide resolved
Fixes #1745