Skip to content

Updated GraphQL Nested Filter integration tests and GraphQL filter processing#1407

Merged
seantleonard merged 13 commits intomainfrom
dev/seantleonard/nestedFilterGraphQLAccess
Apr 5, 2023
Merged

Updated GraphQL Nested Filter integration tests and GraphQL filter processing#1407
seantleonard merged 13 commits intomainfrom
dev/seantleonard/nestedFilterGraphQLAccess

Conversation

@seantleonard
Copy link
Copy Markdown
Contributor

@seantleonard seantleonard commented Apr 4, 2023

Why make this change?

What is this change?

  • Updated processing of nested graphql filters such that the target entity is included in the @relationship directive metadata in addition to a reference to the graphql object name.
  • Updated Nested filtering tests to add validation to role, entity, and field evaluation.
  • Updated the authorization resolver to return fewer exceptions and return "false" when certain permissions aren't met.
  • Runtime config entities with the suffix NF represent "nested filter" entities. These entities are exclusively for nested filter tests so that the anonymous role can be left out. (allowing only authenticated roles, allowing us to demonstrate authorization rule processing for graphql.) these entities have the same backing database object as the entities without the NF suffix.

How was this tested?

  • Integration Tests
  • Unit Tests

Comment thread src/Service.Tests/dab-config.MySql.json
Comment thread src/Service.Tests/SqlTests/GraphQLFilterTests/GraphQLFilterTestBase.cs Outdated
Comment thread src/Service.Tests/SqlTests/GraphQLFilterTests/MsSqlGQLFilterTests.cs Outdated
Comment thread src/Service.GraphQLBuilder/Directives/RelationshipDirective.cs Outdated
Comment thread src/Service/Models/GraphQLFilterParsers.cs Outdated
Comment thread src/Service/Authorization/AuthorizationResolver.cs
Comment thread src/Service/Authorization/AuthorizationResolver.cs Outdated
Comment thread src/Service/Authorization/AuthorizationResolver.cs Outdated
Comment thread src/Service/Authorization/AuthorizationResolver.cs
Comment thread src/Service/Models/GraphQLFilterParsers.cs Outdated
Comment thread src/Service.GraphQLBuilder/Directives/RelationshipDirective.cs Outdated
…o longer needed after later changes were made. Updates constant referring to error message to not have nested ref.
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.

LGTM, thanks for adding tests and improving our GraphQL filtering processing!

@seantleonard seantleonard merged commit 893f503 into main Apr 5, 2023
@seantleonard seantleonard deleted the dev/seantleonard/nestedFilterGraphQLAccess branch April 5, 2023 04:02
tarazou9 added a commit that referenced this pull request Apr 13, 2023
## Why make this change?

- Closes this issue referenced here
#1423. Additional
issue related to this change
#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](#1407) 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](https://github.com/Azure/data-api-builder/blob/1431ffc8c11bcd80f62d51c0c0fd6f15383b147a/src/Service.Tests/CosmosTests/TestBase.cs#L77-L82)
that's added in this PR [recent
update](#1407), 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)

```json
query planets {
  planets (filter: {id: {eq: 1000}}) {
    items {
      id
    }
  }
}
```
tarazou9 added a commit that referenced this pull request Apr 25, 2023
## Why make this change?

- Closes this issue referenced here
#1423. Additional
issue related to this change
#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](#1407) 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](https://github.com/Azure/data-api-builder/blob/1431ffc8c11bcd80f62d51c0c0fd6f15383b147a/src/Service.Tests/CosmosTests/TestBase.cs#L77-L82)
that's added in this PR [recent
update](#1407), 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)

```json
query planets {
  planets (filter: {id: {eq: 1000}}) {
    items {
      id
    }
  }
}
```
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.

Test nested filter and AuthZ database policy rules

3 participants