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

Enforce directives #895

Closed
RobinKamps opened this issue Oct 9, 2019 · 5 comments
Closed

Enforce directives #895

RobinKamps opened this issue Oct 9, 2019 · 5 comments
Labels
bug Something isn't working stale

Comments

@RobinKamps
Copy link

What happened?

directives on subscription (and perhaps on other elements) are not called if not provided via query:

on examples/chat the query
subscription @user(username:"tester") { messageAdded(roomName:"#gophers") { text createdBy } }
calls the directive directive @user(username: String!) on SUBSCRIPTION,
but if you do not provide it like
subscription { messageAdded(roomName:"#gophers") { text createdBy } }
the directive is not called at all.

This makes it impossible to
return an error like
return nil, fmt.Errorf("username must be provided")
directly from the directive.
(However you can implement an explicit check in all subscription resolvers, but whats the point of an directive then...)

What did you expect?

directives are called everytime, not only when they are provided by the user

Minimal graphql.schema and models to reproduce

See examples/chat
perhaps add

fmt.Println("Directive username: ")
fmt.Println(username)

to the directive and

fmt.Println("Resolver username: ")
fmt.Println(getUsername(ctx))

to the Resolver MessageAdded

versions

  • gqlgen version? latest
  • go version? latest
  • dep or go modules? modules
@vektah
Copy link
Collaborator

vektah commented Oct 9, 2019

Subscription execution directives should be supported

See #760

@vektah vektah added the bug Something isn't working label Oct 9, 2019
@RobinKamps
Copy link
Author

I took a deeper look at it and it seems like the same bug is on QUERY and MUTATION.

There would be an easy fix to generate the code different (no switch on directive name) if only one directive can be applied to query, mutation and subscription. I can make a PR.

For multiple directives on QUERY, MUTATION and SUBSCRIPTION however the e.g. _subscriptionMiddleware must be chained via next.

This could result in recursive or complicated nesting. Perhaps it would be an idea to use a middleware chainer (something equivalent to https://medium.com/@chrisgregory_83433/chaining-middleware-in-go-918cfbc5644d ).

@RobinKamps
Copy link
Author

I implemented a chain of multiple directives as middleware where all of them are always executed server side.

However after implementing it, i think there is no real bug in the codegeneration, but perhaps a misconception about directives / to less control over the directive generation process.

The chat example is a misleading usecase of a subscription directive.
It does not change the gql of the subscription.
Directives main functionallity is the manipulation of the graphql response by custom needs from the client (like skip, include or format etc.)
The username should be provided through an authentication middelware instead.
Or if this too much just for the purpose of the example, it should be implemented just as an input to the subscription resolver like

type Subscription {
    messageAdded(roomName: String!, username: String!): Message!
}

but not as an optional subscription directive.

The docs about hasRole directives ( https://gqlgen.com/reference/directives/ ) are also misleading, because with the current implementation the client can always choose to send the query without the @hasRole directive. @hasRole is than never called and the server won´t check the user role at all - if you just followed the docs you have a big security issue here.

Besides the main functionality to manipulate the gql directives could be used as an annotation and thats where those misconceptions come from, because the particular implementation specifies what exactly the annotation means.

The graphQl specs just say it could be used in other tooling like client side implementation/generation or server-side implementation/generation.
The schema could have special directives, wich are optional or mandatory and wich are executed on the server or client or both sides. The main approach other mostly node based graphQl servers is to supply special built in directives like auth or constraint etc., wich are always executed by the server and do not rely on client input.

gqlgen provides just one special case (optional and based on client query input) of possible use cases.
The question here is, how gqlgen should distinguish between:

  • just client side directives (no need to generate server code then)
  • client and server side directives, which are optional (different format output for time etc.) and a communication like format:"HH:MM" from client to server is necessary
  • server side directives, wich are always executed, and only rely on server side implementation (must not read directive inputs from client query - like hasRole example, or possible validation constraints)
  • shared client and server side directives, where no communication via directive from client to server is necessary (e.g. hasRole and validation constraints could be used in client and server separatly - client can do validation before sending the request (without directive) to the server etc.

However gqlgen as a server code generator needs a mechanism to decide wether to enforce a serverside directive, or make it optional and wether to read values from client requests or from the internal schema directive definition.

One approach could be to split up a schema to a client schema and a server schema (see the built in client directive to perform client side cache operations from the apollo client implementation
https://www.apollographql.com/docs/react/data/local-state/#client-side-schema ). But this is very unhandy.

Another approach to generate from a single graphql schema could be a naming convention like @_server, @_client, @_separated, @_optional, @_mandatory etc. perhaps combined with special inputs to trigger the optional part etc.

By the way: Serverside implementations of directives on query, mutation or subscription level require runtime transformations and therefore these directives are not fully implemented or getting a special treatment as well in other gql-server implementations (
see https://www.apollographql.com/docs/graphql-tools/schema-directives/#what-about-query-directives ).

@stale
Copy link

stale bot commented Dec 9, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 9, 2019
@stale stale bot closed this as completed Dec 16, 2019
@abagshaw
Copy link

Has this been resolved? Seems like it breaks the use case of using directives for enforcing authorization/permissions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale
Projects
None yet
Development

No branches or pull requests

3 participants