[ ] Regression
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.
The /graphql
route is accessible.
The /graphql
should be not accessible.
@Module({
imports: [GraphQLModule, AuthModule],
controllers: [AppController]
})
export class ApplicationModule implements NestModule {
constructor(private readonly graphQLFactory: GraphQLFactory) {}
configure(consumer: MiddlewaresConsumer) {
const typeDefs = this.graphQLFactory.mergeTypesByPaths('./**/*.graphql');
const schema = this.graphQLFactory.createSchema({ typeDefs });
consumer
.apply(graphiqlExpress({ endpointURL: '/graphql' }))
.forRoutes({ path: '/graphiql', method: RequestMethod.GET })
.apply(graphqlExpress(req => ({ schema, rootValue: req })))
.forRoutes({ path: '/graphql', method: RequestMethod.ALL });
}
}
@Module({
components: [AuthService, JwtStrategy],
controllers: [AuthController],
})
export class AuthModule implements NestModule {
public configure(consumer: MiddlewaresConsumer) {
consumer
.apply(passport.authenticate('jwt', { session: false }))
.forRoutes({ path: '/graphql', method: RequestMethod.ALL });
}
}
Nest version: 4.5.10
For Tooling issues:
- Node version: 9.4.0
- Platform: Mac
Others:
ApplicationModule
is initialized as first before AuthModule
.
When request come GraphQLMiddleware
is called before PassportMiddleware
. GraphQLMiddleware
doesn't call next, so PassportMiddleware
will never be called.
@mchmielarski thanks for the heads up. Now I'm using guards and this works fine - but I have no clue how to set context in graphqlExpress.
In my old implementation I could just do:
App.use(
'/graphql',
passport.authenticate('jwt', { session: false }),
graphqlExpress(({ user }) => ({
schema: Schema,
context: {
userId: user,
},
})),
Here is my current implementation:
@Guard()
export class AuthGuard implements CanActivate {
async canActivate(dataOrRequest, context: ExecutionContext) {
const isAuthenticated = await new Promise<boolean>((resolve, reject) => {
passport.authenticate('jwt', (err, user) => resolve(user))(
dataOrRequest.res.req,
dataOrRequest.res,
dataOrRequest.next
);
});
if (!isAuthenticated) {
throw new UnauthorizedException();
}
return true;
}
}
@UseGuards(AuthGuard)
@Query('currentUser')
async currentUser(obj, args, context, info) {
const { userId } = context; // the userID is null
return this.usersService.findAllById(userId);
}
Any help would be appreciated
change
.apply(graphqlExpress(req => ({ schema, rootValue: req })))
to
.apply(graphqlExpress(req => ({ schema, context: req })))
Changing this line
.apply(graphqlExpress(req => ({ schema, context: req })))
causes the dataOrRequest
to be undefined in
async canActivate(dataOrRequest, context: ExecutionContext) {
// dataOrRequest is now undefined
const isAuthenticated = await new Promise<boolean>((resolve, reject) => {
/// ...
}
return true;
}
.apply(graphqlExpress(req => ({ schema, rootValue: req, context: { userId: req.user }})))
Mh it didn't work either. I've create a minimal project to reproduce the problem.
https://github.com/cschroeter/nestjs-403/
I think it is a very common case to have a context working with GraphQL - if we get this going I would like to improve the current documentation
It doesn't work because you set user
in AuthGuard
and it's after GraphQL context set.
Easiest solution
In ApplicationModule.configure
:
graphqlExpress
)graphqlExpress
graphiqlExpress
In AuthGuard
:
user
- don't set it via passport@mchmielarski Thank you. It works :)
But the solution has at least two disadvantages.
AuthGuard
is kind of obsolete because you would never reach a resolver function unauthorized. This destroys the concept of the @Guards
馃槥 AppModule
is taking care of authorization stuff. Which kind of soften the responsibility of the AuthorizationModule
I'm not sure if there is any better solution ?!
For anyone interesting in the implementation:
@Module({
imports: [AuthModule, UsersModule, GraphQLModule]
})
export class ApplicationModule implements NestModule {
constructor(private readonly graphQLFactory: GraphQLFactory) {}
configure(consumer: MiddlewaresConsumer) {
const typeDefs = this.graphQLFactory.mergeTypesByPaths("./**/*.graphql");
const schema = this.graphQLFactory.createSchema({ typeDefs });
consumer
.apply(passport.authenticate("jwt", { session: false }))
.forRoutes({ path: "/graphql", method: RequestMethod.ALL })
.apply(
graphqlExpress(req => ({ schema, rootValue: req, context: req.user }))
)
.forRoutes({ path: "/graphql", method: RequestMethod.ALL })
.apply(graphiqlExpress({ endpointURL: "/graphql" }))
.forRoutes({ path: "/graphiql", method: RequestMethod.GET });
}
}
@Guard()
export class AuthGuard implements CanActivate {
async canActivate(dataOrRequest, context: ExecutionContext) {
const { user } = dataOrRequest.res.req;
if (user) {
return true;
}
throw new UnauthorizedException();
}
}
For me current implementation of nestjs/graphql has two problems.
it use apollo-server-express
directly, so response is send directly from this middleware and connection is closed - it breaks nest flow.
guards use rootValue
as dataOrRequest
for canActivate
, it's hard to use if we would like to use guards on deeper level than root - I think that it should to use context from GraphQL
here is very simple example with some changes which try to resolve above problems: https://github.com/mchmielarski/nest-graphql-example
@mchmielarski I've taken a look at your example and with that approach - handling authentication and authorization in GraphQL would be a breeze - given the fact that there is no real good solution available right now.
Hi @mchmielarski @cschroeter,
The solution with AuthGuard
is fine. Just use this snippet:
.apply(graphqlExpress(req => ({ schema, rootValue: req, context: req})))
Thanks to that each guard & interceptor will be able to grab req
from rootValue
but also, you could attach anything to req
object and use it from any resolver, for example context.user
. Additionally, you don't have to use this:
dataOrRequest.res.req,
Since dataOrRequest.res.req
is equal to dataOrRequest
.
Thank you very much - If you don't mind I would like to create a PR to explain other users how this works
That would be awesome! :)
@kamilmysliwiec quick question, maybe I'm wrong but I think that it will not work:
type Post {
id: ID!
title: String
author: Author
}
type Author {
id: ID!
name: String
}
query Query {
post: Post
}
@Resolver('Post')
@UseGuards(AuthGuard)
class PostsResolver {
@Query()
post() { // ... }
@ResolveProperty()
author() { // ... }
}
and when we send query:
{ post { author { name } } }
then AuthGuard
for author()
receives data from post()
so we can't check permissions
Hi @mchmielarski,
Yes, property resolvers don't have an access to both context
and rootValue
but this's a limitation that comes from apollo package. In this case, you'd have to use async-hooks
or some kind zone
implementation. Looking forward to PR @cschroeter! 馃
Hi @kamilmysliwiec
A resolver function receives three arguments:
- obj: The previous object, which for a field on the root Query type is often not used.
- args: The arguments provided to the field in the GraphQL query.
- context: A value which is provided to every resolver and holds important contextual information like the currently logged in user, or access to a database.
It's from graphql.org, so access to rootValue
in resolvers is only special case when resolved field belong to root / Query
In my opinion limitation comes from https://github.com/nestjs/nest/blob/master/src/core/helpers/external-context-creator.ts#L34 not from apollo
Actually, this line: https://github.com/nestjs/nest/blob/master/src/core/helpers/external-context-creator.ts#L34 has nothing to do with that, but still you're right, will investigate some time in that. If apollo supports it, nest would do it as well
Why mentioned line has nothing with that? it will be called for each resolver which has type other than Subscription
- https://github.com/nestjs/graphql/blob/master/lib/resolvers-explorer.service.ts#L66
You can check my "battlefield with nestjs/graphql", maybe it will help something ;)
https://github.com/mchmielarski/nest-graphql-example/blob/master/src/modules/graphql/resolver/resolver-context-creator.ts#L69
@mchmielarski Thanks for linking these issues!
I did it somewhat different but experience the same problem. Applying app.useGlobalGuards(aGuard) will not trigger that guard for calls to a graphql endpoint.
How I did authentication/authorisation:
Instead of crearing a JwtStrategy object and putting it before the routes, inside a global guard called ClaimsGuard I capture all requests. In the guard, if controller endpoints or resolvers are annotated with @Unauthorised() I allow everything. If they are not, I look for a jwt token, verify it myself with the secret in my database and extract its payload. Its payload is an object of type BearerToken that contains claims.
I then match the claims that user has with the claims combined of controller/resolver level and controller/resolver endpoint.
I set the strategy on where to get the token by simple inheritance
@Guard()
export class HttpClaimsGuard extends ClaimsGuard {
constructor(authConfigService : AuthConfigService, reflector: Reflector) {
super(authConfigService,
reflector,
ExtractJwt.fromAuthHeaderAsBearerToken())
}
}
On server start, I then register the guard.
const authGuard = app.select(AuthModule)
.get(HttpClaimsGuard);
app.useGlobalGuards(authGuard);
This strategy works for controller endpoints and the first line (console.log) of the guard is called at each controller endpoint. (I still need to test all use cases so might still contain some bugs)
My issue, and what I think is a bug, is that that console.log is not called when accessing graphql endpoints. So the guard is for some reason ignored.
More info in #434
P.S.
My Graphql Endpoint configuration:
configure(consumer: MiddlewaresConsumer) {
const typeDefs = this.graphQLFactory.mergeTypesByPaths('./**/*.gql');
const schema = this.graphQLFactory.createSchema({ typeDefs });
// Dev Graphiql endpoint
if(!production)
consumer
.apply(graphiqlExpress({ endpointURL: '/graphql' }))
.forRoutes({ path: '/graphiql', method: RequestMethod.GET })
consumer
.apply(graphqlExpress(req => ({ schema, rootValue: req, context: req })))
.forRoutes({ path: '/graphql', method: RequestMethod.ALL });
}
wouldn't it be better to pass the graphql resolver context
into the guard instead of dataOrRequest
since it's more consistent across resolvers? and (I'm not sure about this) this change won't require changes to apollo-server
.
the only problem I see would be inconsistencies when using the guard with a normal controller; but even then guards always get the request on controller endpoints and it always gets data on something like websockets. guards on graphql resolvers would get the context. so the first argument depends on usage.
there _is_ precedence for the change. :man_shrugging:
@mchmielarski this line doesn't mean anything related to this subject. It just an array destruction to pick a req
object which is then passed to guards and interceptors as well, nothing else. Furthermore, I did some investigation, and Nest GraphQL module doesn't reduce a list of arguments in fact. You're able to access the same parameters as mentioned here https://www.apollographql.com/docs/graphql-tools/resolvers.html - obj
, args
, context
, info
. Hence, you're able to access both context
and rootValue
(info.rootValue
).
@jrosseel duplicate of https://github.com/nestjs/nest/issues/434
Hey @kamilmysliwiec, do you know if there is an easy way of accessing the graphql args (the parsed ones that are passed to the resolvers) inside of Guards?
Sorry if this is the wrong place to ask, I can make it into a feature request if needed.
@arjitkhullar is this similar?
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Most helpful comment
For me current implementation of nestjs/graphql has two problems.
it use
apollo-server-express
directly, so response is send directly from this middleware and connection is closed - it breaks nest flow.guards use
rootValue
asdataOrRequest
forcanActivate
, it's hard to use if we would like to use guards on deeper level than root - I think that it should to use context from GraphQLhere is very simple example with some changes which try to resolve above problems: https://github.com/mchmielarski/nest-graphql-example