Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address filter access deny issue for Cosmos #1436

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

tarazou9
Copy link
Contributor

@tarazou9 tarazou9 commented Apr 13, 2023

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
    }
  }
}

Copy link
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 making these changes, @tarazou9! one nit, but looks good overall.

src/Service/Models/GraphQLFilterParsers.cs Outdated Show resolved Hide resolved
@tarazou9
Copy link
Contributor Author

Thank you for making these changes, @tarazou9! one nit, but looks good overall.

Thank you so much for pointing this issue out, Sean!

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

@tarazou9 tarazou9 merged commit b292511 into main Apr 13, 2023
12 checks passed
@tarazou9 tarazou9 deleted the users/tarazou/cosmos_filter_auth branch April 13, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants