nestjs / graphql

GraphQL (TypeScript) module for Nest framework (node.js) 🍷
https://docs.nestjs.com/graphql/quick-start
MIT License
1.46k stars 396 forks source link

Make possible to use guards on ResolveProperty decorator #295

Closed Ajax-ua closed 5 years ago

Ajax-ua commented 5 years ago

Feature Request

Is your feature request related to a problem? Please describe.

I'm using GraphQL with code first approach. I need to restrict the access to method with ResolveProperty decorator. Nest has UseGuards decorator for this particular purpose but it works with Resolve decorator only.

Describe the solution you'd like

I want to use UseGuards decorator. I've checked the src of nestjs/graphql lib and as far as I understood the contextOptions variable from this line is responsible for proper working of guards and interceptors with ResolveProperty decorator. Now that works fine with resolver types Query, Mutation and Subscription only, and ResolveProperty has custom type so the guard or interceptor is not executed with it.

Teachability, Documentation, Adoption, Migration Strategy

Users will be able to use guard the same way as with Resolve decorator.

What is the motivation / use case for changing the behavior?

For example, only admin should be able to get some additional info about user.

Rmannn commented 5 years ago

We need this too, we use oauth2 scopes to give access to resolvers and we have to check the scopes inside each property resolver. Using guards will help us a lot.

kamilmysliwiec commented 5 years ago

Using either guards or interceptors at the @ResolveProperty() is very dangerous and could lead to memory/performance issues (see this issue https://github.com/nestjs/graphql/issues/208). Keep in mind that @ResolveProperty() has to be called for every property and the evaluation context will have to be created on every execution (pair values with decorators, run pipes etc). For interceptors that means, that we have to actually wrap the entire execution flow into RxJS observables + use recursion, hence, it reduces the performance heavily due to the huge overhead. Guards aren't that "tough" (no rxjs + they run sequentially), but still, they will have an impact on your performance. If your @ResolveProperty() is intended to run for thousands of objects, it's strongly recommended to avoid using both guards and interceptors.

Also, another thing is that your "local" guards and interceptors will be merged with class-level and global enhancers. That means, that if you mount, let's say, an authentication guard at the class-level, it's gonna run for every property - and this is unacceptable. Similarly with interceptors - if you're using logger interceptor - it will log on each resolve property call making your GraphQL API much slower.

Since lots of people were asking for this feature, I have added fieldResolverEnhancers in 6.4.0 release of @nestjs/graphql that allows you to enable guards, filters, and interceptors at the @ResolveProperty(). They will be disabled by default (as before).

Example usage:

GraphQLModule.forRoot({
  // your options
  fieldResolverEnhancers: ['guards', 'interceptors']
}),

Please, keep in mind that this feature should be used very carefully and doesn't fit many scenarios.

sgarner commented 5 years ago

I have a similar problem to the OP, but if guards are not recommended on properties I am wondering if there is another solution that doesn't have the same performance implications?

Let's imagine the following scenario. You have two objects, User and Book. A user can purchase multiple books, and you want to be able to retrieve a list of the books owned by a user. However, that information is private, so only the user themselves, or a user with administrator privileges, should be able to view such list.

You could model this in GraphQL with a userBooks() query that accepts a userId parameter and is protected with an AuthGuard, retrieves the authenticated user from context and checks for authorization.

But the book list could be large, so you need pagination. I would prefer to approach this Relay-style as a "Connection" between User -> Books. In that case, it makes more sense to me to model this as a booksConnection property on User, using @ResolveProperty().

The query would then look something like:

query BooksOwnedByMe {
  loggedInUser {
    booksConnection(first: 10) {
      pageInfo {
        hasNextPage
        hasPreviousPage
        startCursor
        endCursor
      }
      edges {
        cursor
        node {
          id
          title
          author {
            name
          }
          dateOfPublication
          synopsis
        }
      }
    }
  }
}

However, code like the following doesn't work.

@Resolver(_of => User)
export class UserResolver {
  @ResolveProperty('booksConnection', _type => UserBookConnection)
  @UseGuards(GqlAuthGuard)
  public async booksConnection(
    @Root() user: User,
    @Args() connectionArgs: ConnectionArgs,
    @Context() context: GqlExecutionContext<User>,
  ): Promise<UserBookConnection> {
    const authenticatedUser = context.request.user;

    if (user.id !== authenticatedUser.id && !authenticatedUser.isAdmin()) {
      throw new UnauthorizedException('Not permitted.');
    }

    // retrieve the books etc...
    return {}
  }
}

The @UseGuards(GqlAuthGuard) has no effect as described previously. But presumably as a result of that(?), the context.request also does not contain a user when accessed inside a property resolver, so authenticatedUser is undefined (even though the request is authenticated) and this falls apart. I'm OK with the guard itself not working, but I have not been able to find any other way to reference the authenticated user from a property resolver.

Note I am using @nestjs/passport like the following, and context.request.user works fine from inside query and mutation resolvers:

PassportModule.register({
  defaultStrategy: 'jwt',
  property: 'user',
});

Is my solution to enable fieldResolverEnhancers or is there another way to resolve this issue?

Rmannn commented 5 years ago

@sgarner I had the same pb regarding user missing in context in property resolvers. Trying to get the user from the request when the context is created was not working for me. request.user was not yet set. But it will be later after auth guards were executed.

I had to create a context provider class with getters to get the user and other data that an Auth guard would set on the request.

export interface IAppContext {
    user?: any;
    authInfo?: any;
}

export class AppContext implements IAppContext {
    constructor(public request: IAppRequest) {}

    get authInfo(): any {
        return this.request.authInfo;
    }

    get user(): any {
        return this.request.user;
    }
}

export class ContextProvider {
    constructor(protected readonly request: IAppRequest) {}
    getContext(): IAppContext {
        return new AppContext(this.request);
    }
}

A custom context decorator, re-creating the context from request after auth guards execution

export const Context = createParamDecorator((data, req) => {
    const provider = new ContextProvider(req);
    return provider.getContext();
});

Then in the graphql module options you can do something like this

GraphQLModule.forRootAsync({
    useFactory: (config: IConfig) => {
        return {
            autoSchemaFile: 'schema.gql',
            debug: true,
            playground: true,
            context: ({ req }) => new ContextProvider(req).getContext()
        };
    },
    inject: [getConfigToken()]
}),