Introduces a change in the REST POST, PATCH and PUT API response construction#1731
Merged
severussundar merged 55 commits intomainfrom Oct 17, 2023
Merged
Conversation
severussundar
commented
Sep 20, 2023
ayush3797
reviewed
Sep 20, 2023
ayush3797
reviewed
Sep 20, 2023
ayush3797
reviewed
Sep 20, 2023
ayush3797
reviewed
Sep 20, 2023
severussundar
commented
Sep 21, 2023
severussundar
commented
Sep 21, 2023
severussundar
commented
Sep 21, 2023
severussundar
commented
Sep 21, 2023
severussundar
commented
Sep 21, 2023
Aniruddh25
previously requested changes
Oct 7, 2023
Collaborator
Aniruddh25
left a comment
There was a problem hiding this comment.
Looks good overall, some questions on advancement of resultRow in case of multiple result sets.
ayush3797
reviewed
Oct 10, 2023
Contributor
|
Thanks for handling all the edge cases and the patience while resolving all the comments @severussundar. LGTM! |
ayush3797
approved these changes
Oct 12, 2023
seantleonard
requested changes
Oct 12, 2023
Contributor
seantleonard
left a comment
There was a problem hiding this comment.
few nits and required mitigation of undisposed JsonDocument objects
seantleonard
requested changes
Oct 13, 2023
Contributor
seantleonard
left a comment
There was a problem hiding this comment.
last fix needed with where you using finally and should use using where jsondoc is declared.
seantleonard
approved these changes
Oct 16, 2023
Contributor
seantleonard
left a comment
There was a problem hiding this comment.
Thank you for all your efforts addressing feedback and iterating accordingly.
Hey Aniruddh, I've incorporated all the suggestions. So, dismissing this review to enable merge
seantleonard
pushed a commit
that referenced
this pull request
Oct 17, 2023
…truction (#1731) ## Why make this change? - REST POST, PUT and PATCH requests, when successful, return all the fields present in the entity without taking into consideration the `read` configuration for the entity. In cases, where a certain role does not have read permissions, or excludes certain fields for read action, these are not honored. In this case, a role that is not intended to read any field/ some fields will be able to read all the fields of this entity. - This PR introduces a change in the way responses for REST requests are constructed - During the construction of the response for POST, PUT and PATCH API requests, the `read` action setup is taken into account. ## Response construction for different scenarios There is no logic change in the way the insert, update operation works. All the changes are concerned only with what to return as a response. **1. Read action undefined:** **Response**: An empty response is returned. **Roundtrips**: Only one roundtrip to the database is performed. **Database Query**: No change in the number/type of database queries executed **2. Read action without any include/exclude fields or database policy**: **Response**: A non-empty response containing all the fields. **Roundtrips**: Only one roundtrip to the database is performed. **Database Query**: No change in the number/type of database queries executed. **3. Read action with include/exclude fields but without database policy**: **Response**: A response containing only the fields that are allowed as per the include/exclude configuration setup for the read action. **Roundtrips**: Only one roundtrip to the database is performed. **Database Query**: No change in the number/type of database queries executed. The additional fields (if any) in the response are removed at DAB layer. **4. Read action with database policy and with/without include/exclude fields**: **Response**: A response that takes into account both the include/exclude fields and the database policy setup for read action **Roundtrips**: Two roundtrips to the database are performed. First roundtrip is to perform the database query associated with the insert or upsert operation. The second roundtrip is performed to execute a select query to fetch results that honors the database policy + include/exclude configuration setup for read action. **Database Query**: An additional select query that honors the database policy + include/exclude configuration setup for read action. For example: In the read action for Book entity, `publisher_id` field is excluded and a database policy `@item.title ne 'Test'` setup; the following database query which honors both these setups is generated and executed  **5. Special case with insert/update policy:** When a database policy is defined for insert/update operation and the insert/update operation does not go through due to the respective database policy, a `403 Forbidden` error will be returned, ## Special cases with System Roles `Anonymous` and `Authenticated` are the system roles defined by DAB. `Authenticated` role derives permissions from `Anonymous` roles when `Authenticated` role is not defined in the config file. This section explains how the presence or absence of `Anonymous` role affects the behavior of `Authenticated` role in different cases. Note: The behaviors described below is applicable when the REST API request is executed in the context of `Authenticated` role. **1. Authenticated role is defined in the config file:** When `Authenticated` role is defined, the behavior is very straightfoward - only the actions configured for the `Authenticated` role are considered. The presence or absence of `Anonymous` role has no effect. For example, if `Authenticated` role does not have `create` action, then `POST` request will error out (`403 Forbidden`). Same case with `update` action for PUT and PATCH requests. If the necessary create/update actions are present, then the presence or absence of `read` action determines the response returned. **2. Authenticated role is not defined in the config file:** In this case, the `Authenticated` role derives permissions from the `Anonymous` role. The `Authenticated` role has the same exact permissions as `Anonymous` and the request is evaluated against those set of permissions. If `Anonymous` role is also absent from the config file, then `403 Forbidden` error is thrown. **3. Random undefined role in the config file:** The derivation of permissions from `Anonymous` role is only applicable for `Authenticated` role. If a request is executed in the context of some random role which is not defined in the config file, a `403 Forbidden` error will be returned. ## What is the change: - `SqlQueryEngine`: The return type of `ExecuteAsync` method is updated to `Task<JsonDocument>`. The logic for formatting of the response is pushed to the callers of this method. - `SqlMutationEngine`: Introduces changes to perform the second roundtrip to the database (if needed ~ more details in the earlier section). - `IAuthorizationResolver`: `GetDBPolicyForRequest` method is introduced to facilitate determination of whether a database policy is defined for a role and action or not - `SqlResponseHeaders` - New class that contains helper methods to assist with the creation of API requests executed against SQL database types - `Location` response header is not returned for PUT and PATCH requests (which result in the creation of a new item) anymore as they don't provide any additional information to the user. For POST requests, an empty string is returned when no read permission is setup or when one or more PKs are excluded. When `read` permission is applicable for all PKs, a non-empty header is returned. - `*-commands.txt`: DAB commands to create the necessary roles for testing - `dab-config.*Sql.json`: Sample config files are updated to the way CLI generates them with the latest changes. ## How was this tested? - [x] Integration Tests - [x] Manual Tests
seantleonard
added a commit
that referenced
this pull request
Oct 21, 2023
…truction - CHERRYPICK (#1824) merges changes from #1731 as patch to 0.9. includes merge conflict resolution. let tests run before merge. spot checked REST find, put , and some graphql tests and they succeeded. --------- Co-authored-by: Shyam Sundar J <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why make this change?
readconfiguration for the entity. In cases, where a certain role does not have read permissions, or excludes certain fields for read action, these are not honored. In this case, a role that is not intended to read any field/ some fields will be able to read all the fields of this entity.readaction setup is taken into account.Response construction for different scenarios
There is no logic change in the way the insert, update operation works. All the changes are concerned only with what to return as a response.
1. Read action undefined:
Response: An empty response is returned.
Roundtrips: Only one roundtrip to the database is performed.
Database Query: No change in the number/type of database queries executed
2. Read action without any include/exclude fields or database policy:
Response: A non-empty response containing all the fields.
Roundtrips: Only one roundtrip to the database is performed.
Database Query: No change in the number/type of database queries executed.
3. Read action with include/exclude fields but without database policy:
Response: A response containing only the fields that are allowed as per the include/exclude configuration setup for the read action.
Roundtrips: Only one roundtrip to the database is performed.
Database Query: No change in the number/type of database queries executed. The additional fields (if any) in the response are removed at DAB layer.
4. Read action with database policy and with/without include/exclude fields:
Response: A response that takes into account both the include/exclude fields and the database policy setup for read action
Roundtrips: Two roundtrips to the database are performed. First roundtrip is to perform the database query associated with the insert or upsert operation. The second roundtrip is performed to execute a select query to fetch results that honors the database policy + include/exclude configuration setup for read action.
Database Query: An additional select query that honors the database policy + include/exclude configuration setup for read action.
For example: In the read action for Book entity,
publisher_idfield is excluded and a database policy@item.title ne 'Test'setup; the following database query which honors both these setups is generated and executed5. Special case with insert/update policy:
When a database policy is defined for insert/update operation and the insert/update operation does not go through due to the respective database policy, a
403 Forbiddenerror will be returned,Special cases with System Roles
AnonymousandAuthenticatedare the system roles defined by DAB.Authenticatedrole derives permissions fromAnonymousroles whenAuthenticatedrole is not defined in the config file. This section explains how the presence or absence ofAnonymousrole affects the behavior ofAuthenticatedrole in different cases.Note: The behaviors described below is applicable when the REST API request is executed in the context of
Authenticatedrole.1. Authenticated role is defined in the config file:
When
Authenticatedrole is defined, the behavior is very straightfoward - only the actions configured for theAuthenticatedrole are considered. The presence or absence ofAnonymousrole has no effect.For example, if
Authenticatedrole does not havecreateaction, thenPOSTrequest will error out (403 Forbidden). Same case withupdateaction for PUT and PATCH requests. If the necessary create/update actions are present, then the presence or absence ofreadaction determines the response returned.2. Authenticated role is not defined in the config file:
In this case, the
Authenticatedrole derives permissions from theAnonymousrole. TheAuthenticatedrole has the same exact permissions asAnonymousand the request is evaluated against those set of permissions.If
Anonymousrole is also absent from the config file, then403 Forbiddenerror is thrown.3. Random undefined role in the config file:
The derivation of permissions from
Anonymousrole is only applicable forAuthenticatedrole. If a request is executed in the context of some random role which is not defined in the config file, a403 Forbiddenerror will be returned.What is the change:
SqlQueryEngine: The return type ofExecuteAsyncmethod is updated toTask<JsonDocument>. The logic for formatting of the response is pushed to the callers of this method.SqlMutationEngine: Introduces changes to perform the second roundtrip to the database (if needed ~ more details in the earlier section).IAuthorizationResolver:GetDBPolicyForRequestmethod is introduced to facilitate determination of whether a database policy is defined for a role and action or notSqlResponseHeaders- New class that contains helper methods to assist with the creation of API requests executed against SQL database typesLocationresponse header is not returned for PUT and PATCH requests (which result in the creation of a new item) anymore as they don't provide any additional information to the user. For POST requests, an empty string is returned when no read permission is setup or when one or more PKs are excluded. Whenreadpermission is applicable for all PKs, a non-empty header is returned.*-commands.txt: DAB commands to create the necessary roles for testingdab-config.*Sql.json: Sample config files are updated to the way CLI generates them with the latest changes.How was this tested?