Skip to content

[Cherrypick] Introduces a change in the REST POST, PATCH and PUT API response construction#1824

Merged
seantleonard merged 5 commits intorelease/0.9from
dev/seantleonard/0.9-patch-02-cherrypick
Oct 21, 2023
Merged

[Cherrypick] Introduces a change in the REST POST, PATCH and PUT API response construction#1824
seantleonard merged 5 commits intorelease/0.9from
dev/seantleonard/0.9-patch-02-cherrypick

Conversation

@seantleonard
Copy link
Copy Markdown
Contributor

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.

severussundar and others added 2 commits October 17, 2023 15:15
…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
Copy link
Copy Markdown
Contributor

@severussundar severussundar left a comment

Choose a reason for hiding this comment

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

Validated common REST and GraphQL scenarios manually in local. LGTM

@Aniruddh25 Aniruddh25 added this to the 0.9 milestone Oct 19, 2023
@seantleonard
Copy link
Copy Markdown
Contributor Author

merge pending #1828 merge to main and then subsequent cherry pick to the 0.9 release branch.

@seantleonard seantleonard merged commit f3c3ca0 into release/0.9 Oct 21, 2023
@seantleonard seantleonard deleted the dev/seantleonard/0.9-patch-02-cherrypick branch October 21, 2023 01:21
@seantleonard seantleonard changed the title Introduces a change in the REST POST, PATCH and PUT API response construction - CHERRYPICK [Cherrypick] Introduces a change in the REST POST, PATCH and PUT API response construction Nov 20, 2023
@seantleonard seantleonard added the 🍒Cherrypick Cherry-picking another commit/PR label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🍒Cherrypick Cherry-picking another commit/PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants