feat(@nestjs/passport): support graphql context in auth guard

[Edgar-P-yan]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

When adding AuthGuard to graphql resolver class/method you have to add custom logic for resolving the request object as described in Nest.js documentation https://docs.nestjs.com/techniques/authentication#graphql.
For example, to create a guard that would support both HTTP and GraphQL contexts you have to write something like this (the code is taken from one of my projects where both REST and GraphQL APIs should be supported):

import { AuthGuard } from '@nestjs/passport';
import { ExecutionContext, Injectable, ContextType } from '@nestjs/common';
import { GqlExecutionContext } from '@nestjs/graphql';
import { IncomingMessage } from 'http';

@Injectable()
export class JwtAuthGuard extends AuthGuard('jwt') {
  getRequest(context: ExecutionContext): any {
    if (context.getType<ContextType | 'graphql'>() === 'graphql') {
      const gqlExecCtx = GqlExecutionContext.create(context);
      const request: IncomingMessage = gqlExecCtx.getContext().req;
      return request;
    }
    return context.switchToHttp().getRequest();
  }
}

which works just perfect. But it is a boilerplate.

What is the new behavior?

Now, with the built-in check for graphql context type and request object resolving logic, users will not have to write boilerplate code in each guard.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[jmcdo29]

What if someone has a different value than req for the context? Like with Fastify the property is request.

[Edgar-P-yan]

Good catch! Didn't know that Fastify adapter uses different properties. Right now i don't have any good ideas on how to sole this issue. Also there are some other adapters, and their properties may be different too

[brandon-leapyear]

I don't think this depends on platform. Doesn't getContext() return the context provided to the GraphQLModule/ApolloServer constructor (docs)? So it would depend on how people initialize the context, e.g.

GraphQLModule.forRoot({
  context: ({ req }) => {
    return {
      foo: 'bar',
      contextRequest: req,
    }
  }
})

Maybe a better solution is to make this more first class; e.g.

https://github.com/nestjs/graphql/blob/f80f28770db48848160ddf82a81547f537663a5e/lib/services/gql-execution-context.ts#L8-L19

Maybe override the constructor to store the request

class GqlExecutionContext {
  constructor(readonly private originalContext: ExecutionContext, ...) {
    super(...)
  }

  static create(context: ExecutionContext) {
    ...
    new GqlExecutionContext(context, ...)
  }

  getRequest<T = any>() {
    return this.originalContext.switchToHttp().getRequest()
  }
}

or something?

[jmcdo29]

From the same docs you linked

The fields of the object passed to your context function differ if you're using middleware besides Express. See the API reference for details..

[brandon-leapyear]

👍 still, this assumes that someone set up the context key correctly and wouldn't account for someone using the snippet I posted above. Nothing wrong with it; it would just require documentation (admittedly, the workaround we have to do because of this problem should have also been documented)

[jmcdo29]

There is current documentation on using Passport with GraphQL, though it still is also only valid for working with Express (as at the time of writing the docs, apollo-server-fastify wasn't compatible). Now when setting the context for fastify, the context should be

context({ request, reply}) => ({ request, reply })

The snippet above looks promising, but I would have to check how well context.switchToHttp().getRequest() works on a GQL request in the first place, as the arguments for HTTP and GQL are different.

[kamilmysliwiec]

@jmcdo29 we should be able to fix this on the @nestjs/graphql level, here https://github.com/nestjs/graphql/blob/master/lib/utils/merge-defaults.util.ts#L9