Skip to content

Conversation

ayush3797
Copy link
Contributor

@ayush3797 ayush3797 commented Dec 22, 2023

Why make this change?

Since GraphQL insertions now support nested insertions, we need to authorize entity and fields not only for the top-level entity in the insertion, but also the nested entities and fields. This PR aims to address that logic of collecting all the unique entities and fields belonging to those entities in a data structure, and then sequentially iterate over all entities and fields to check whether the given role is authorized to perform the action (here nested insertion).

What is this change?

  1. The presence of the model directive on the output type of the mutation object will be used to determine whether we are dealing with a point mutation or a nested insertion. Presence of model directive indicates we are doing a point insertion while its absence indicates the opposite. This logic is added to SqlMutationEngine.ExecuteAsync() method. This logic determines whether the input argument name is item (for point mutation) or items (for insert many).
  2. A new method SqlMutationEngine.AuthorizeEntityAndFieldsForMutation() is added. The name is kept generic (instead of using 'Insertion') because the same method can be used later for nested updates as well. As the name indicates, this method iterates over all the entities and fields and does the required authorization checks.
  3. The method mentioned in the point above depends on another newly added method SqlMutationEngine.PopulateMutationFieldsToAuthorize() whose job is to populate all the unique entities referred in the mutation and their corresponding fields into a data structure of the format:
    Dictionary<string, HashSet<string>> entityAndFieldsToAuthorize - where for each entry in the dictionary:
    -> Key represents the entity name
    -> Value represents the unique set of fields referenced from the entity
  4. The method SqlMutationEngine.PopulateMutationFieldsToAuthorize() recursively calls itself for nested entities based on different criteria explained in code comments.
  5. When the field in a nested mutation is a list of ObjectFieldNode (fieldName: fieldValue) or the field has an object value, the fields are added to the data structure mentioned in (3) using a newly added method: SqlMutationEngine.ProcessObjectFieldNodesForAuthZ() which sequentially goes over all the fields and add it to the list of fields to be authorized. Since a field might represent a relationship- and hence a nested entity, this method again calls its parent caller i.e. SqlMutationEngine.PopulateMutationFieldsToAuthorize().
  6. The method SqlMutationEngine.ProcessObjectFieldNodesForAuthZ() contains the logic to ensure that the fields belonging to linking tables are not added to the list of fields to be authorized.
  7. Moved the method GetRoleOfGraphQLRequest() from Cosmos/SqlMutationEngine to AuthorizationResolver.

How was this tested?

To be added.

Sample Request(s)

  1. Config:
    image
    image

  2. Request/Response - AuthZ failure because piecesAvailable field is not accessible to test_role_with_excluded_fields_on_create role.
    image

  3. Request/Response: Removing piecesAvailable field from request body leads to successful authz checks (request fails during query generation).

image

@ayush3797 ayush3797 changed the base branch from main to dev/agarwalayush/schemaGeneration December 22, 2023 05:36
@ayush3797 ayush3797 self-assigned this Dec 22, 2023
@ayush3797 ayush3797 added enhancement New feature or request auth labels Dec 22, 2023
@ayush3797 ayush3797 added this to the 0.11rc milestone Dec 22, 2023
@ayush3797 ayush3797 linked an issue Dec 22, 2023 that may be closed by this pull request
@ayush3797 ayush3797 added the Multiple mutations Fixes/enhancements related to nested mutations. label Jan 2, 2024
@ayush3797
Copy link
Contributor Author

/azp run

@Azure Azure deleted a comment from azure-pipelines bot Mar 21, 2024
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Base automatically changed from dev/agarwalayush/schemaGeneration to dev/NestedMutations March 21, 2024 14:26
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Azure Azure deleted a comment from azure-pipelines bot Mar 21, 2024
@Azure Azure deleted a comment from azure-pipelines bot Mar 21, 2024
@seantleonard
Copy link
Contributor

The PR description example doesn't touch on authorization of "multiple mutations." Yes, there is a relationship field in the mutation, but your example doesn't demonstrate authorization of fields/entities defined by that relationship field.

Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far. I'd say only thing that this PR would benefit from is testing authz of sub-entity fields. Unless i missed it, i only saw tests for Top-level entity and fields and sub-entity (no fields)

@ayush3797
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@ayush3797 ayush3797 merged commit 9233c41 into dev/NestedMutations Mar 21, 2024
@ayush3797 ayush3797 deleted the dev/agarwalayush/AuthZForNestedInsertions branch March 21, 2024 22:54
@severussundar severussundar mentioned this pull request Mar 26, 2024
1 task
severussundar added a commit that referenced this pull request Mar 29, 2024
## Why make this change?

- All code changes for **Multiple Create** feature was being merged into
`dev/NestedMutations` branch.
- This PR attempts to merge all these changes to the `main` branch in
preparation for the `0.12.* rc1` release

## What is this change?

- Right now, `dev/NestedMutations` branch contains the code changes for
the following components of Multiple Create feature
- Schema Generation -
#1902
  - AuthZ - #1943
- Feature flag - CLI changes
#1983
- Feature flag - Re-naming changes
#2103
- Feature flag - Engine changes
#2116
- Each specified PR was reviewed before merging into
`dev/NestedMutations` branch.
- This PR aims to merge all the changes into `main` branch
  
## How was this tested?

- [x] Unit, Integration and Manual tests were performed on each PR
before merging into `dev/NestedMutations`

---------

Co-authored-by: Shyam Sundar J <[email protected]>
Co-authored-by: Sean Leonard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth enhancement New feature or request Multiple mutations Fixes/enhancements related to nested mutations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AuthZ validation - Nested inserts
5 participants