Skip to content

Fix include fields section behaviour in the config#1282

Merged
Aniruddh25 merged 12 commits intomainfrom
dev/agarwalayush/fixIncludeFieldsSectionBehaviour
Mar 2, 2023
Merged

Fix include fields section behaviour in the config#1282
Aniruddh25 merged 12 commits intomainfrom
dev/agarwalayush/fixIncludeFieldsSectionBehaviour

Conversation

@ayush3797
Copy link
Contributor

@ayush3797 ayush3797 commented Feb 28, 2023

Why make this change?

Fixes: #1208.

Need to see some test cases first!

What are we going to conclude from below examples:

If include is not specified: it will resolve to all the fields present
If exclude is not specified, it will resolve to an empty array

Consider the following cases for the fields section for a particular role/action combination for an entity:

  1. "fields":{ "include": [] }

    Since include is explicitly specified as an empty array, we won't resolve it to wildcard. (We were doing this before during config validation but not during authorization resolution stage). No field will be accessible for this configuration.

  2. "fields":{ "include": [], "exclude":[] }
    Again since include/exclude both are empty arrays- behavior is identical to first case.

  3. "fields":{ }
    In this case, included/excluded are not declared at all, and both would assume their default values, i.e., include will resolve to all the fields present in the entity and exclude will be empty. All the fields will be accessible in this configuration.

  4. "fields":{ "excluded": ["some_field"] }
    In this case, included is not declared at all, so, it would assume its default value, i.e., it will resolve to all the fields present in the entity. All the fields except 'some_field' will be accessible in this configuration. This is the very case which was mentioned in the linked issue "Not all the columns required by policy are accessible." error #1208.

What is this change?

include is resolved to contain all fields only when it is null (not declared in the config). The behavior was the same even before during authorization resolution stage, but during config validation, we were missing this resolution.

How was this tested?

  • Unit Tests added to ConfigValidationUnitTests.cs class.

@ayush3797 ayush3797 self-assigned this Feb 28, 2023
@ayush3797 ayush3797 added the bug Something isn't working label Feb 28, 2023
@ayush3797 ayush3797 added this to the Mar2023 milestone Feb 28, 2023
@ayush3797 ayush3797 linked an issue Feb 28, 2023 that may be closed by this pull request
Copy link
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.

Thanks for catching this and the quick fix!

Copy link
Contributor

@aaronburtle aaronburtle 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, just had a question about the set of fields being null.

@Aniruddh25 Aniruddh25 merged commit fe97885 into main Mar 2, 2023
@Aniruddh25 Aniruddh25 deleted the dev/agarwalayush/fixIncludeFieldsSectionBehaviour branch March 2, 2023 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

"Not all the columns required by policy are accessible." error

3 participants