neo4j / graphql

A GraphQL to Cypher query execution layer for Neo4j and JavaScript GraphQL implementations.
https://neo4j.com/docs/graphql-manual/current/
Apache License 2.0
507 stars 149 forks source link

Filters of `@authorization` directive are not generated as `WHERE` clauses for subqueries which match on the node for which these filters are defined. #5534

Open andreloeffelmann opened 2 months ago

andreloeffelmann commented 2 months ago

Describe the bug When defining filters via the @authorization directive, these filters are not rendered as WHERE clauses inside subqueries which match/reference on the (root)-node for which these filters are defined.

Type definitions

type Product @mutation(operations: []) @authorization(filter: [{requireAuthentication: false, operations: [READ,AGGREGATE], where: {AND: [{node:{isPublic:true}}, {node:{isEmpty:false}}] }}]) @subscription(events: []) {
    """Unique Identifier of this product"""
    productId: Int! @unique
    isEmpty: Boolean! @default(value: false)
    isPublic: Boolean! @default(value: false)
    """The product variants belonging to this product"""
    variants: [Product!]! @relationship (type: "PRODUCT_HAS_FAMILY_PRODUCT", direction: IN, nestedOperations: [], queryDirection: DEFAULT_DIRECTED) @settable(onCreate: false, onUpdate: false)
}

To Reproduce Steps to reproduce the behavior:

  1. Run a server with the type definitions and DEBUG enabled
  2. Execute the following Query:
    query{
    products(options:{limit:1} where:{variantsAggregate:{count:1}}){
    productId  
    variantsAggregate{
      count
    }
    }
    }
  3. See the generated cypher-query:
    
    MATCH (this:Product)
    CALL {
    WITH this
    MATCH (this)<-[this0:PRODUCT_HAS_FAMILY_PRODUCT]-(this1:Product)
    RETURN count(this1) = $param0 AS var2
    }
    WITH *
    WHERE (var2 = true AND (($param1 IS NOT NULL AND this.isEmpty = $param1) AND ($param2 IS NOT NULL AND this.isPublic = $param2)))
    WITH *

LIMIT $param3 CALL { WITH this MATCH (this)<-[this3:PRODUCT_HAS_FAMILY_PRODUCT]-(this4:Product) WHERE (($param4 IS NOT NULL AND this4.isEmpty = $param4) AND ($param5 IS NOT NULL AND this4.isPublic = $param5)) RETURN count(this4) AS var5 } RETURN this { .productId, variantsAggregate: { count: var5 } } AS this cypher params: { param0: Integer { low: 1, high: 0 }, param1: false, param2: true, param3: Integer { low: 1, high: 0 }, param4: false, param5: true }

The generated query misses the authorization filters inside the subquery on the referenced `Product` node. The query should look like this (see 4th line):
````cypher
MATCH (this:Product)
CALL {
    WITH this
    MATCH (this)<-[this0:PRODUCT_HAS_FAMILY_PRODUCT]-(this1:Product) WHERE (($param1 IS NOT NULL AND this.isEmpty = $param1) AND ($param2 IS NOT NULL AND this.isPublic = $param2))
    RETURN count(this1) = $param0 AS var2
}
WITH *
WHERE (var2 = true AND (($param1 IS NOT NULL AND this.isEmpty = $param1) AND ($param2 IS NOT NULL AND this.isPublic = $param2)))
WITH *

LIMIT $param3
CALL {
    WITH this
    MATCH (this)<-[this3:PRODUCT_HAS_FAMILY_PRODUCT]-(this4:Product)
    WHERE (($param4 IS NOT NULL AND this4.isEmpty = $param4) AND ($param5 IS NOT NULL AND this4.isPublic = $param5))
    RETURN count(this4) AS var5
}
RETURN this { .productId, variantsAggregate: { count: var5 } } AS this
cypher params: {
  param0: Integer { low: 1, high: 0 },
  param1: false,
  param2: true,
  param3: Integer { low: 1, high: 0 },
  param4: false,
  param5: true
}

Expected behavior Authorization filters from the @authorization directive should be generated as WHERE clauses on all subqueries/queries which match on a node for which these filters are defined.

System

andreloeffelmann commented 2 months ago

Corresponding ZenDesk ticket here

darrellwarde commented 2 months ago

Hey @andreloeffelmann, I'm going to relabel this as a feature request, because this isn't intended to work this way - this would essentially be classed as something like a new operation FILTER inside the @authorization directive which isn't currently available. The READ and AGGREGATE operations apply to the fields inside the selection set, and fields within the input objects.

andreloeffelmann commented 2 months ago

Hey @darrellwarde, I see your point from a developer's perspective. However, the current state of neo4j-graphql is just not consistent. When executing the query of the example above I receive products which have exactly 1 variants but also those which have 0 or more than 1. The filter of the query just does not work. And this is something which is not transparent/logically conclusive for the API consumer. Even worse, it was pure coincidence that we figured this out! The documentation does not mention that @authorization will not work inside filters of a simple READ query.

If you see this as a feature request - OK. But if so, neo4j-graphql should NOT render relationship fields into the filter *Where type which connect to a type for which an @authorization directive is declared. Better to not offer a functionality than offering it in a buggy way!

So, from my point of view, I see the following solutions:

Either way, when do you think can we expect a solution on this? This is an internal high-prio case which I already reported via our enterprise support.

andreloeffelmann commented 3 days ago

Hey @darrellwarde , any news on this? Was hoping that this will be addressed in v6, but according to the docs it was not?

darrellwarde commented 1 day ago

Hey @andreloeffelmann, I totally missed your last message in September, my apologies!

When executing the query of the example above I receive products which have exactly 1 variants but also those which have 0 or more than 1. The filter of the query just does not work.

I'm reading this issue over and over and I can't see how this would be related to this authorization feature request - the variantsAggregate filter that you're applying to your query is being translated into Cypher so this shouldn't be the case even without this authorization filters. Or am I missing something here?

Either way, when do you think can we expect a solution on this? This is an internal high-prio case which I already reported via our enterprise support.

We will have to talk about if and how we address this for Version 7 if it could be a breaking change. However, my view still stands that the authorization features of the library exist to control the reading (i.e. the return of data to consumers) and writing (i.e. database writes) through the API. The usage of the authorization features in this manner, to achieve some kind of "default filter", was never really the intended purpose of the feature. Our primary focus for this feature is that data is secure, which I still believe is the case with the state of things in this issue.

This is not to say that we will not look at this, but just to offer some explanation as why this has not been looked at yet in this time where we have a bunch of high priority architectural changes to work on.