Preventing update/insert of read-only fields in a table by user#1596
Merged
Preventing update/insert of read-only fields in a table by user#1596
Conversation
ayush3797
added
bug
This was
linked to
issues
Jul 20, 2023
ayush3797
changed the title
2 tasks
seantleonard
reviewed
Sep 8, 2023
seantleonard
reviewed
Sep 8, 2023
seantleonard
reviewed
Sep 8, 2023
seantleonard
reviewed
Sep 8, 2023
seantleonard
reviewed
Sep 8, 2023
seantleonard
reviewed
Sep 8, 2023
seantleonard
reviewed
Sep 8, 2023
seantleonard
requested changes
Sep 8, 2023
Contributor
seantleonard
left a comment
seantleonard
left a comment
There was a problem hiding this comment.
Clarification on how tests validate behavior -> would be good to be explicit in comments across tests.
Question about how post mutation select uses TableDefinition.AllColumns() which may result in not honoring read permissions or may not honor GraphQL selection set fields / REST request querystring $select fields.
Collaborator
Question about how post mutation select uses I think Shyam's PR should handle that separately. |
Aniruddh25
approved these changes
Sep 9, 2023
Collaborator
Aniruddh25
left a comment
Aniruddh25
left a comment
There was a problem hiding this comment.
LGTM, few nits. Thanks!
…ttps://github.com/Azure/data-api-builder into dev/agarwalayush/PreventingUpdateForReadOnlyColumns
Contributor
seantleonard
left a comment
seantleonard
left a comment
There was a problem hiding this comment.
Circled back to a few areas where the validation query for a test doesn't look to actually validate what you are checking for. Some tests were updated to accommodate, but there are still some leftovers to address.
…e computed columns are not nulled out when excluded from a patch update request.
seantleonard
approved these changes
Sep 12, 2023
Contributor
seantleonard
left a comment
seantleonard
left a comment
There was a problem hiding this comment.
Thank you for addressing comments and implementing this feature! I added one last commit to address a comment that is consistent with the other changes you were making for the sake of time.
ayush3797
deleted the
dev/agarwalayush/PreventingUpdateForReadOnlyColumns
branch
ayush3797
added a commit
that referenced
this pull request
Sep 18, 2023
## Why make this change? Add support for allowing extraneous fields in request body as per: #1606. ## What is this change? DAB will expose another CLI option in the `init` command `rest.request-body-strict`, which when set to true/false, would set the property `runtime.rest.request-body-strict` as `true`/`false` in the generated runtime config file. What this essentially means is that, as part of the REST mutations (POST/PUT/PATCH), there can be extraneous fields present in the request body and they will be ignored. Default behavior of DAB will continue to be the same as what it is today, i.e. any extraneous fields in the request body would return an error. So to say, the default value of `runtime.rest.request-body-strict` is `true`. ## Additional change Refactored `RequestValidator.cs` class to be a singleton dependent on `ISqlMetaDataProvider` and `RuntimeConfigProvider` services. Better presents its dependency on the two services and simplifies the code. ## What are the different extraneous fields? 1. Fields in request body that do not map to any backing column in the table/view. They will be ignored. 2. Fields that are repeated - present as PK in the request URL for PUT/PATCH requests and also present in the request body. The ones in the request body will be ignored. 3. Read-only fields. The support for this will be added in a subsequent PR: #1596. -> Read-only field are: **Computed** (or Generated) / **AutoGenerated** fields in MsSql/MySql/PgSql -> **timestamp** fields in MsSql ## Questions: **1. What about authorization of extraneous fields?** When operating in flexible mode, the user is _not_ authorized for extraneous fields NOT defined on table because those fields are ignored. For all other cases, authorization goes as usual. ## How was this tested? - [x] Integration Tests - Added tests to validate that extraneous fields are allowed in the request body for all the 3 relational DBs in the `{*}RestBodyNonStrictModeTests.cs` classes inheriting from the base class `RestBodyNonStrictModeTests.cs`. (* = MsSql,MySql,PostgreSql) - [x] Unit Tests - Added unit test to validate the functionality of the newly added --`rest.request-body-strict` feature in the init command. --------- 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
3 participants
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?
There are certain type of columns in databases which cannot be provided a value (not even null value) for by the user. Eg. in MsSql tables, we cannot provide values for columns that are computed base on other columns in the table or have a datatype of timestamp/rowversion. Similarly for PgSql,/MySql we cannot provide a value for generated columns. This PR prevents DAB from allowing user to attempt an update/insert of such columns. The way DAB will do this is by collecting metadata about columns during the startup phase and populate the
IsReadOnlyproperty of theColumnDefinitionof each column in the table. More details about the specifics can be found on the linked issues for MsSql(#1453), PgSql(#1584), MySql(#1583).NOTE: The columns which are autogenerated are also considered as read-only columns irrespective of the database flavor (MsSql/PgSql/MySql).
What is this change?
IsReadOnlytoColumnDefinitionclass which if true, will indicate that the column is read-only, i.e. it cannot be provided a value for (via INSERT/UPDATE).GetQueryToGetReadOnlyColumns()toIQueryBuilder.csclass which will have its overridden implementation in Pg/My/MsSql. This method will return the query that will fetch the metadata about the whether a column is read-only.RequestValidator.csclass which will ensure that no read-only column is present the request body either for update (viaUpsert/UpsertIncremental/Update/UpdateIncremental) or insert operations.SqlInsertQueryStructure.cs/SqlUpdateQueryStructure.cs- classes as the request validation stage is not a part of the codeflow for GQL requests.AutoGeneratedDirectiveTypewhen a column is read-only. Previous this directive was only added for autogenerated fields (which is a subset of read-only fields).Implemented Behaviors:
When a read-only field is included in the request body for mutation:This is a bad request as we cannot provide values for read-only fields. We will throw an exception no matter what operation is being executed (PUT/PATCH/POST via REST). For create/update via GQL, we would see a warning in the BCP UI for read-only fields as well that the field is not valid for mutation (just like we see currently for autogen fields).When a read-only field is excluded from the request body for mutation: This is a valid scenario. In this case (for PUT/PATCH operations via REST), we won't NULL out the field's value but let the responsibility on the database to deal with it. For other operations, we really don't need to do anything as the field is not present in the request body.How was this tested?