Skip to content

Hotfix for CosmosDb_NoSql filtering issue on March Release#1448

Merged
tarazou9 merged 5 commits intorelease/Mar2023from
users/tarazou/Mar2023_HotPatch1
Apr 25, 2023
Merged

Hotfix for CosmosDb_NoSql filtering issue on March Release#1448
tarazou9 merged 5 commits intorelease/Mar2023from
users/tarazou/Mar2023_HotPatch1

Conversation

@tarazou9
Copy link
Copy Markdown
Contributor

@tarazou9 tarazou9 commented Apr 24, 2023 •
edited
Loading

Why make this change?

  • Closes this issue referenced here Getting Access forbidden message when trying to use filter on graphql queries #1423. Additional issue related to this change [Known Issue] Azure Cosmos DB Support for Authorization Policies #597
    • Cosmos DB currently doesn't support field level authorization, it's expected that we are not honoring operationToColumnMap.Excluded and operationToColumnMap.Included and by introducing field level auth check into the GQLFilterParser recent update results the issue that we are seeing.
    • Cosmos have tests to exercise the filter parser, the reason the tests didn't catch this issue is because the changes here added lines that's added in this PR recent update, this mocks up the field auth check to always return true for all Cosmos filter tests.
    • CosmosMetadataProvider does not translate a field wild card (*) or list of hardcoded include columns to the list of AllowedFields in the engine's AuthorizationResolver currently, but we can add a pass through to the TryGetExposedColumnName as suggested so it doesn't throw a NotImplementedException when user accidently passed in the fields settings in the Cosmos configuration file.

What is this change?

  • Address this by skipping the field auth check inside of the GQLFilterParser when the database type is Cosmos

How was this tested?

  • [ x ] Integration Tests
  • [ x ] Unit Tests

Sample Request(s)

query planets {
  planets (filter: {id: {eq: 1000}}) {
    items {
      id
    }
  }
}

@tarazou9 tarazou9 changed the base branch from main to release/Mar2023 April 24, 2023 20:27
@Mathos1432
Copy link
Copy Markdown
Contributor

Mathos1432 commented Apr 24, 2023

Can you update the PR description please?

Aniruddh25
Aniruddh25 approved these changes Apr 24, 2023
View reviewed changes
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.

Thanks for the hot patch! LGTM, after fixing the description.

@tarazou9
Copy link
Copy Markdown
Contributor Author

tarazou9 commented Apr 24, 2023

Can you update the PR description please?

Updated, thank you for pointing this out!

mbhaskar
mbhaskar approved these changes Apr 25, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mbhaskar mbhaskar left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks Tara

seantleonard
seantleonard requested changes Apr 25, 2023
View reviewed changes
Comment thread src/Service/Services/MetadataProviders/CosmosSqlMetadataProvider.cs Outdated
@tarazou9 tarazou9 merged commit b21a924 into release/Mar2023 Apr 25, 2023
@tarazou9 tarazou9 deleted the users/tarazou/Mar2023_HotPatch1 branch April 25, 2023 01:11
@Aniruddh25 Aniruddh25 changed the title Hot Patch for March Release Hotfix for CosmosDb_NoSql filtering issue on March Release Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seantleonard seantleonard seantleonard approved these changes

@mbhaskar mbhaskar mbhaskar approved these changes

@Aniruddh25 Aniruddh25 Aniruddh25 approved these changes

@JelteF JelteF Awaiting requested review from JelteF

@Mathos1432 Mathos1432 Awaiting requested review from Mathos1432 Mathos1432 is a code owner

@gledis69 gledis69 Awaiting requested review from gledis69

@aaronburtle aaronburtle Awaiting requested review from aaronburtle aaronburtle is a code owner

@milismsft milismsft Awaiting requested review from milismsft milismsft is a code owner

@ayush3797 ayush3797 Awaiting requested review from ayush3797

@abhishekkumams abhishekkumams Awaiting requested review from abhishekkumams

@aaronpowell aaronpowell Awaiting requested review from aaronpowell aaronpowell is a code owner

@severussundar severussundar Awaiting requested review from severussundar severussundar is a code owner

@ravishetye ravishetye Awaiting requested review from ravishetye

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tarazou9 @Mathos1432 @mbhaskar @Aniruddh25 @seantleonard