fix(delegate): @omit and field-level policies are not enforced when querying with a delegate model
#1718
Conversation
… querying with a delegate model Fixes #1710
WalkthroughWalkthroughThe changes introduced in this pull request enhance the functionality of the delegate proxy handler and related components by implementing methods for ensuring the selection of discriminator fields and sanitizing mutation payloads. The modifications include the addition of new methods for handling polymorphic models and improving the accuracy of field resolution in queries. Additionally, new tests have been added to validate the functionality of the Changes
Assessment against linked issues
Possibly related 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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/runtime/package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
Files selected for processing (9)
- packages/runtime/src/enhancements/node/delegate.ts (10 hunks)
- packages/runtime/src/enhancements/node/omit.ts (3 hunks)
- packages/runtime/src/enhancements/node/policy/policy-utils.ts (1 hunks)
- packages/runtime/src/enhancements/node/query-utils.ts (1 hunks)
- packages/schema/src/res/stdlib.zmodel (1 hunks)
- tests/integration/tests/enhancements/with-delegate/omit-interaction.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-delegate/validation.test.ts (1 hunks)
- tests/regression/tests/issue-1710.test.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/schema/src/res/stdlib.zmodel
Additional comments not posted (28)
tests/integration/tests/enhancements/with-delegate/validation.test.ts (2)
2-26: Test suite name accurately describes its purpose.The test suite name "Polymorphic input validation" clearly conveys the intent of the tests within it, which is to validate polymorphic input.
3-25: Test case correctly validates the rejection of auxiliary fields in mutations.The test case sets up the schema with the
AssetandPostmodels using the delegate pattern, creates aPostinstance, and attempts to update the correspondingAssetinstance by directly modifying thedelegate_aux_postfield. It correctly expects the update operation to fail with the specific error message "Auxiliary relation field "delegate_aux_post" cannot be set directly".This test case is crucial for ensuring that the system maintains the integrity of the polymorphic relationship and prevents potential data corruption by rejecting direct modifications to auxiliary fields during mutations.
tests/regression/tests/issue-1710.test.ts (5)
4-25: Schema definition looks good!The schema definition aligns with the PR objectives and follows best practices:
- The
Profilemodel serves as a base with common fields and annotations.- The
Usermodel extendsProfileand adds fields with appropriate access control directives.- The
Organizationmodel extendsProfilewithout adding any fields.- The
@@delegate(type)and@@allow('read,create', true)annotations are used correctly.The schema structure is clear and well-defined, enabling testing of access control mechanisms for polymorphic sub-models.
27-32: User creation and field access assertions are correct!The test case correctly creates a new user with sensitive fields and asserts that the
passwordfields are undefined in the created user object. This aligns with the PR objectives of ensuring that the@deny('read', true)and@omitdirectives are enforced during user creation.The test case follows best practices for creating test data and making assertions.
34-36: User retrieval and field access assertions are correct!The test case correctly retrieves the user by their profile ID and asserts that the
passwordfields are undefined in the retrieved user object. This aligns with the PR objectives of ensuring that the@deny('read', true)and@omitdirectives are enforced during user retrieval.The test case follows best practices for querying data and making assertions.
38-51: User update attempt and error expectation are correct!The test case correctly attempts to update the user's role using
db.profile.updatewith thedelegate_aux_userfield and theupdateoperation. This aligns with the PR objectives of ensuring that the@deny('read,update', true)directive prevents updating therolefield.The
expectstatement correctly verifies that the expected error with the message 'Auxiliary relation field' is thrown, indicating that the access control mechanism is functioning as intended.The test case follows best practices for attempting updates and expecting errors.
2-53: The test case is well-written and aligns with the PR objectives!The test case in
issue-1710.test.tsthoroughly covers the essential aspects of the PR objectives:
- It verifies the enforcement of
@omitand@denydirectives during user creation, retrieval, and update.- The schema definition, test data creation, assertions, and error expectations are all correct and follow best practices.
- The test case provides adequate coverage for the access control mechanisms related to polymorphic sub-models.
- It ensures that sensitive fields are not exposed and that unauthorized updates are prevented.
The test case is well-structured, maintainable, and extensible. It serves as a solid regression test for the issues addressed in this PR.
Great job on writing a comprehensive test case!
packages/runtime/src/enhancements/node/omit.ts (5)
8-8: LGTM!The import statement for
QueryUtilsis correctly added.
25-26: LGTM!The
queryUtilsproperty is correctly declared in theOmitHandlerclass.
29-29: LGTM!The
queryUtilsproperty is correctly initialized in the constructor.
74-75: LGTM!The
getDelegateConcreteModelmethod is correctly used to retrieve the concrete model associated with the entity data.
77-77: LGTM!The
resolveFieldfunction is correctly used to retrieve the field information based on the model metadata and the concrete model.tests/integration/tests/enhancements/with-delegate/omit-interaction.test.ts (4)
29-40: LGTM!The test case correctly validates the behavior of the
@omitdirective when querying a concrete model (Post). It ensures that thefooandbarfields, which are marked with@omit, are undefined in the query results.
42-54: LGTM!The test case correctly validates the behavior of the
@omitdirective when querying a base model (Asset). It ensures that thefooandbarfields, which are marked with@omit, are undefined in the query results, while thetitlefield remains accessible.
56-73: LGTM!The test case correctly validates the behavior of the
@omitdirective when the discriminator is not selected in the query. It ensures that thefooandbarfields, which are marked with@omit, are undefined in the query results even when the discriminator field is not explicitly selected.
75-90: LGTM!The test case correctly validates the behavior of the
@omitdirective when querying in a nested context. It ensures that thefooandbarfields, which are marked with@omit, are undefined in the query results when fetching the relatedassets(polymorphicAssetrecords) through theUsermodel, while thetitlefield remains accessible.packages/runtime/src/enhancements/node/query-utils.ts (1)
218-234: LGTM!The new
getDelegateConcreteModelfunction is a valuable addition to theQueryUtilsclass. It correctly determines the concrete model name based on the provided model string and associated data, handling polymorphic models effectively.The function follows a clear logic:
- It checks if the
dataparameter is valid and of type object. If not, it returns the original model name.- If the data is valid, it retrieves model information using
getModelInfo, looking for a discriminator property.- If a discriminator is present and the corresponding concrete model name is found in the data, that name is returned.
- If no concrete model is identified, it defaults to returning the original model name.
This ensures that the appropriate concrete model is selected based on the discriminator field in the data, while gracefully handling cases where the data is invalid or the discriminator is not found.
Overall, this enhancement improves the handling of polymorphic models in the
QueryUtilsclass.tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (1)
272-311: Excellent test case for validating field-level policy enforcement!This test case effectively validates the enforcement of field-level policies based on the user's ID. It covers the following scenarios:
- User with ID 1 can access both the
fooandbarfields of thePostmodel.- User with ID 2 can create a post, but the
fooandbarfields are restricted and returnundefinedwhen accessed.The test case ensures that the access control mechanisms are functioning as intended, with users having the appropriate permissions being able to access the restricted fields, while users without permissions receive
undefinedvalues.Great job in covering the different scenarios and asserting the expected behavior!
packages/runtime/src/enhancements/node/policy/policy-utils.ts (1)
1384-1387: The changes look good! Here's some additional context:The updated implementation introduces a step to determine the concrete model when the data is a polymorphic entity. This is achieved by calling
getDelegateConcreteModel, which retrieves the appropriate model based on the provided data.Consequently,
doPostProcessForReadis now called with the concrete model instead of the original one. This change enhances the flexibility and accuracy of the policy application, ensuring that the correct model is utilized during the read operation, particularly in scenarios involving polymorphism.packages/runtime/src/enhancements/node/delegate.ts (9)
5-5: Import 'traverse' ModuleYou've added the import statement for the 'traverse' module. Ensure that 'traverse' is properly added to your project's dependencies to avoid runtime errors.
81-83: Include Discriminator Field in Find OperationsBy adding
this.ensureDiscriminatorSelection(model, args);in thedoFindmethod, you're ensuring that the discriminator field is included in selection arguments. This is crucial for correctly determining the concrete model type during post-processing.
362-363: Sanitize Mutation Payload in 'create' MethodAdding
this.sanitizeMutationPayload(args.data);ensures that auxiliary relation fields prefixed withDELEGATE_AUX_RELATION_PREFIXcannot be set directly during creation. This is important for maintaining data integrity and preventing unauthorized mutations.
379-396: Ensure Proper Handling in 'sanitizeMutationPayload' MethodThe
sanitizeMutationPayloadmethod traverses the mutation payload to detect any attempt to set auxiliary relation fields directly. This prevents unintended data manipulation. Ensure that this method efficiently handles large or deeply nested payloads to avoid performance bottlenecks.
409-410: Sanitize Mutation Payload in 'createMany' MethodIncluding
this.sanitizeMutationPayload(args.data);in thecreateManymethod ensures that batch creations do not allow direct setting of auxiliary relation fields. This reinforces data integrity across bulk operations.
450-451: Sanitize Mutation Payload in 'createManyAndReturn' MethodBy adding
this.sanitizeMutationPayload(args.data);, you ensure that auxiliary relation fields are not improperly set duringcreateManyAndReturnoperations, maintaining consistent data validation.
638-639: Sanitize Mutation Payload in 'update' MethodIncorporating
this.sanitizeMutationPayload(args.data);into theupdatemethod prevents direct manipulation of auxiliary relation fields during updates, safeguarding against potential data corruption.
659-660: Sanitize Mutation Payload in 'updateMany' MethodAdding
this.sanitizeMutationPayload(args.data);to theupdateManymethod ensures that bulk updates adhere to data integrity rules by disallowing direct setting of auxiliary relation fields.
688-690: Sanitize Mutation Payloads in 'upsert' MethodBy sanitizing both
args.updateandargs.createusingthis.sanitizeMutationPayload, you ensure thatupsertoperations cannot bypass validation checks on auxiliary relation fields, upholding data integrity.
Fixes #1710