Skip to content

Introduces a change in the REST POST, PATCH and PUT API response construction#1731

Merged
severussundar merged 55 commits intomainfrom
dev/shyamsundarj/rest-post-put-patch-response-behavior-change
Oct 17, 2023
Merged

Introduces a change in the REST POST, PATCH and PUT API response construction#1731
severussundar merged 55 commits intomainfrom
dev/shyamsundarj/rest-post-put-patch-response-behavior-change

Conversation

@severussundar
Copy link
Copy Markdown
Contributor

@severussundar severussundar commented Sep 20, 2023

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

image

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?

  • Integration Tests
  • Manual Tests

@severussundar severussundar self-assigned this Sep 20, 2023
Comment thread src/Service.Tests/SqlTests/RestApiTests/Patch/PatchApiTestBase.cs
@seantleonard seantleonard added this to the 0.9rc milestone Sep 20, 2023
Comment thread src/Core/Authorization/AuthorizationResolver.cs Outdated
Comment thread src/Core/Resolvers/CosmosQueryEngine.cs
Comment thread src/Core/Resolvers/SqlQueryEngine.cs Outdated
Comment thread src/Service.Tests/SqlTests/RestApiTests/Insert/InsertApiTestBase.cs
Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Service.Tests/dab-config.MsSql.json Outdated
Copy link
Copy Markdown
Collaborator

@Aniruddh25 Aniruddh25 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 overall, some questions on advancement of resultRow in case of multiple result sets.

Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Core/Resolvers/SqlMutationEngine.cs
Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Core/Resolvers/SqlResponseHelpers.cs
Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
@ayush3797
Copy link
Copy Markdown
Contributor

ayush3797 commented Oct 12, 2023

Thanks for handling all the edge cases and the patience while resolving all the comments @severussundar. LGTM!

Copy link
Copy Markdown
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.

few nits and required mitigation of undisposed JsonDocument objects

Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Core/Resolvers/SqlMutationEngine.cs Outdated
Comment thread src/Core/Resolvers/SqlResponseHelpers.cs Outdated
Comment thread src/Core/Resolvers/SqlResponseHelpers.cs Outdated
Copy link
Copy Markdown
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.

last fix needed with where you using finally and should use using where jsondoc is declared.

Comment thread src/Config/DataApiBuilderException.cs Outdated
Comment thread src/Core/Resolvers/SqlMutationEngine.cs
Comment thread src/Core/Resolvers/SqlMutationEngine.cs
Copy link
Copy Markdown
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.

Thank you for all your efforts addressing feedback and iterating accordingly.

@severussundar severussundar dismissed Aniruddh25’s stale review October 17, 2023 04:26

Hey Aniruddh, I've incorporated all the suggestions. So, dismissing this review to enable merge

@severussundar severussundar merged commit 49ac53a into main Oct 17, 2023
@severussundar severussundar deleted the dev/shyamsundarj/rest-post-put-patch-response-behavior-change branch October 17, 2023 04:27
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

![image](https://github.com/Azure/data-api-builder/assets/11196553/4e777d5d-8465-4fa1-a776-8e6aa97e65ba)

**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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Introduce new substatus codes to help users with an actionable item based on the error type

4 participants