[Cherrypick] Introduces a change in the REST POST, PATCH and PUT API response construction#1824
Merged
seantleonard merged 5 commits intorelease/0.9from Oct 21, 2023
Conversation
…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
severussundar
approved these changes
Oct 19, 2023
Contributor
severussundar
left a comment
There was a problem hiding this comment.
Validated common REST and GraphQL scenarios manually in local. LGTM
Aniruddh25
approved these changes
Oct 19, 2023
Contributor
Author
|
merge pending #1828 merge to main and then subsequent cherry pick to the 0.9 release branch. |
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.
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.