feat: filtered introspection queries

[Eomm]

Implements #43

I'm creating this draft PR to get some feedback about this implementation.

How it works

Every directive adds a preExecution hook.
The hook will:

• understand if the input query is an introspection
• if an introspection query is found:
  • it executes the directive's applyPolicy for every FIELDS the directive is attached to
  • the applyPolicy function runs without the parent object and the input arguments (it should be fine)
  • based on the applyPolicy result, the hook filters the GraphQLSchema object by filtering the FIELDS or OBJECTS associated with the directive itself
• the hook will return the filtered schema to the next preExecution function

Note that understanding if the query is an introspection one is necessary or the authorization errors will no more be visible.

Example:

• read an @auth Query
• the filter hook will understand that the user is not authenticated executing the applyPolicy function
• the @auth FIELD are evicted from the schema
• the query is executed against the filtered schema, so the directive's fields are no more in the schema, and they cannot trigger the auth error
• the output for those fields will be null without the corresponding error.

Todos

I think this PR requires many optimizations:

• improve the isIntrospection function
• execute the isIntrospection once (now it is executed one time for each directive)
• how could we run the wrapSchema filtering function once instead of one time for each directive. The main issue here is that the plugin is registered multiple times
• applyPolicy memoization. it cannot be executed once due different parameters. Example @hasRole(role: "admin") vs @hasRole(role: "reader")
• how to handle multiple queries into one request. EDIT: one inspection query triggers the filtering
• documentation

Let me know your thought

[simoneb]

A high level comment about the approach, considering that I have little familiarity with the implementation.

Every directive adds a preExecution hook.

I wonder if this approach won't turn out to be more complicated than having a single preExecution hook.

Here's the rationale:

• this plugin allows creating multiple directives to be used for authorization
• the effect of such directives could overlap in terms of whether the operation is allowed or not
• by having each directive set up a preExecution hook it means that each hook will run independently and may allow the operation or not.
  1. would this work? E.g. what if depending on the order of the execution of those hooks one allows the operation and the other doesn't? would it lead to the right behavior?
  2. would this perform well?

So my proposal (again I don't know if it's better or not than your proposed approach, just food for thought) would be, why don't have have a single preExecution hook which tries all directives applied to all the types/fields/whatever of the current operation?

[Eomm]

Right now:

• the checks are executed in registration order. For example, registering the @auth directive first and then the @hasRole one, the result will be:
  • all the GraphQL's @auth directives are executed and the GraphQL schema is filtered and returned
  • all the GraphQL's @hasRole directives are executed using the previously filtered schema

As you can see, the result is an AND operator, and I think this is correct.
The tests have this use case with double directives:

privateMessages(org: String): [Message!] @auth @hasRole(role: "admin")

have a single preExecution hook which tries all directives applied

I think this is the right direction. We need to change this plugin API a bit to let it works.
My idea to solve this is to introduce a namespace parameter:

app.register(mercuriusAuth, {
  authContext: authContext,
  applyPolicy: authPolicy,
  namespace: 'autorization-filtering',
  authDirective: 'auth'
})
app.register(mercuriusAuth, {
  authContext: hasRoleContext,
  applyPolicy: hasRolePolicy,
  namespace: 'autorization-filtering',
  authDirective: 'hasRole'
})

By doing so, we can collect the multiple hooks, into one single execution.

I thought to multiple directive registration too:

app.register(mercuriusAuth, {
  directives: [
    {
      authContext: authContext,
      applyPolicy: authPolicy,
      authDirective: 'auth'
    },
    {
      authContext: hasRoleContext,
      applyPolicy: hasRolePolicy,
      authDirective: 'hasRole'
    }
  ]
})

But in this case, the plugin is losing a lot of flexibility.

[jonnydgreen]

Agreed on the single preExecution hook as well

My idea to solve this is to introduce a namespace parameter:

Would this be an optional parameter with the default behaviour of the filtered introspection being off?

I'm keen to keep it as flexible as possible! How do you propose to implement this? In the situation you described, would you be running 2 preExecution hooks, one per plugin registration?

[Eomm]

Would this be an optional parameter with the default behaviour of the filtered introspection being off?

Yes, we can make it turn on/off for sure

How do you propose to implement this?

The namespace parameter aims to "connect" different plugins registration, which usually plugins are not aware of each other.
I'm not happy of the namespace parameter name, but the concept is grouping directives.

In the situation you described, would you be running 2 preExecution hooks, one per plugin registration?

No, I would leave just one filtering preExecution hook per namespace.

[jonnydgreen]

No, I would leave just one filtering preExecution hook per namespace.

So you'd register the hook in the first registration and then updating the existing hook with the new information in the second registration?

Just throwing another idea out here! In order to avoid any complications with the plugins needing to talk to each other and exhibiting different registration behaviour depending on the order of registration, I wonder if it would be better to have a preExecution hook per plugin registration. Each would register their own preExecution hook and pass the modifiedSchema to the next. That way, the plugins don't necessarily need to know about each other, it's potentially more flexible and we don't have any cross-plugin state considerations. What do you think?

Also, one thing before I forget is we should run some benchmarks on this - before and after!

[Eomm]

Each would register their own preExecution hook and pass the modifiedSchema to the next

This was the initial implementation. The issue within this approach is:

• the isIntrospection function execution for each directive
• the filterSchema function execution for each directive

All these GraphQL tree traversals may be expensive so we should reduce them at the minimum.

---

I have committed the single hook per namespace execution: now the filtering core is on the filterGraphQLSchemaHook function you may take a look there.

Now the isIntrospection function is executed once per namespace 🎉
I'm working to execute once the filterSchema function too.

Also, one thing before I forget is we should run some benchmarks on this - before and after!

Yeah sure!

[jonnydgreen]

This was the initial implementation. The issue within this approach is:

I see, I thought you were talking about registering a hook per directive usage of fields - my mistake! Either way, let's see how your current plan works out though as you make a good point about keeping tree traversals to a minimum!

Now the isIntrospection function is executed once per namespace 🎉

Nice - excited to see how this turns out!

[mcollina]

graphql-tools is not a depdency. Could we avoid it? Or just add it as a dependency?

[Eomm]

I have added it to the package.json

[jonnydgreen]

How feasible would it be to not include this dependency?

[Eomm]

I have checked it and it requires a lot of GraphQL knowledge about the spec and the graphqljs implementation to manage all the use cases.

I'm monitoring this issue: graphql/graphql-js#113
Or it could be best to include this feature in the official graphql implementation.

At the moment the issue is 5 years old but still active (people want it)

[Eomm]

I'm trying to create the GraphQLResolveInfo that contains information about the operation's execution state, including the field name.

I did not cover all the cases, but it will be slightly different from the info argument received as input by the resolver function.

Any suggestion to recreate it is highly appreciated

[jonnydgreen]

Apologies for the slow response, I've been very busy this week - I will have a look this weekend if that's okay!

[jonnydgreen]

There are a couple of functions exported from the graphql package that might be of interest here:

• buildExecutionContext
• buildResolveInfo (requires an execution context)

[Eomm]

I did not find a way to build the info object actually.

Would you mind defining a subset info object in the first place?

[jonnydgreen]

Do you anticipate this being problematic for the type definition of the GraphQLResolveInfo type?

Depending on the data included, I think this is okay for the initial release of this feature if it's not possible - we'd need to:

• Make this clear in the documentation

What was blocking you on this out of interest?

[Eomm]

Let me make an example:
if you assign the @auth directive to a type MyObject and you will set this type as output for two different Query, the info object depends on the query that is being executed - the resolver changes.

Instead, during the filtering, the applyPolicy function is not being executed as a resolver wrapper, but it is executed on the MyObject type without any other context.

For this reason I was unable to build the same argument

[simoneb]

Sharing a couple of thoughts about the user API of this feature to be considered for implementation:

• why do we need namespace to enable filtering? I do understand the technical reasons, but I'm not sure how user friendly this is
• can't we register multiple directives in a single plugin registration? Again I understand the technical reasons but as a user wanting to implement multiple auth directives in the same application it would be good if I could do that without having to register the plugin multiple times

There are minor compared to a working implementation of this feature of course, but I would nonetheless consider them because the feeling with the current public API is that implementation details are leaking into the public API.

[Eomm]

why do we need namespace to enable filtering?

Renamed the option to filterSchema: bool

can't we register multiple directives in a single plugin registration?

This interface can be implemented in a follow-up PR. it should not change the core logic given to the user this lightweight setup

---

Added a lot more test and use cases

Todos:

• test gateway refresh
• define a subset info object feat: filtered introspection queries #46 (comment)

[jonnydgreen]

Good stuff!! Could you also add the following please?

• Update to the API docs: https://github.com/mercurius-js/auth/blob/main/docs/api/options.md
• An example file as well

[Eomm]

Not sure why the CI stop working 🤔
Cannot replicate locally

not ok 3 - test/auth_on_type-gateway.js

[simoneb]

Very nice work here!

[jonnydgreen]

good stuff! LGTM

Need to change the package version when Mercurius 8.10.0 is released as per this comment: #46 (comment)

[jonnydgreen]

@Eomm I'll see if I can replicate that CI error

[jonnydgreen]

Able to replicate the CI error locally, my steps:

• rm -rf node_modules
• rm -f package-lock.json
• npm i
• npm run unit

I just ran the CI on the default branch and it doesn't fail, so I wonder if it's something to do with a recent commit to Mercurius that's not released yet. The only commit I can see in the last 12 hours is this one: mercurius-js/mercurius@7dfc5dc . The only change that might cause an issue is this one, but I'm not 100% - need to test this theory out: mercurius-js/mercurius@7dfc5dc#diff-54f4c0dbde15b4d086d534779d154989affc0010203b919446099b219b08fa6cR53

[jonnydgreen]

LGTM - awesome stuff!

[mcollina]

lgtm

[simoneb]

Folks this work is amazing 🚀 I'm not extremely familiar with other implementations but as far as I know Hasura is one of the few to have this provided out of the box