doug-martin / nestjs-query

Easy CRUD for GraphQL.
https://doug-martin.github.io/nestjs-query
MIT License
822 stars 142 forks source link

Add support for restricting fields returned and fields can be filtered if the user doesn't have permissions #37

Closed izundo-viennv closed 4 years ago

izundo-viennv commented 4 years ago

@doug-martin I created this ticket for discussion. Because sometimes, I don't want the user can filter fields and fields will be null if they don't have permissions for that. I don't know a good solution for that

izundo-viennv commented 4 years ago

@doug-martin How do you think? It can be a permission issue. But I don't know in a real app, it will expose all fields or not if the user doesn't have permissions. Or we will create another dto to expose what we want, but that will create a lot of dtos

doug-martin commented 4 years ago

I think this is a question better directed towards @nestjs/graphql or the typegraphql repos. There seems to be some discrepancy between them on how to restrict fields in general in a graphql API.

Once there is a way to do this with type-graphql that nestjs supports I'll look into it more.

izundo-viennv commented 4 years ago

@doug-martin You're right, but I just want to discuss with you, maybe you can help me to have a better solution to implement.

For example in a real world app you have a user entity with 10 fields (email, firstName, lastName, password, isDeleted, ...)

How's about your solution for that?

izundo-viennv commented 4 years ago

@doug-martin Could you help me to answer this question?

izundo-viennv commented 4 years ago

@johannesschobel Do you have any idea about that?

johannesschobel commented 4 years ago

I think the go-to strategy would be to create various endpoints that return different type of objects.. for example it can return a User or a RestrctedUser object type depending on the method (query / mutation) you call. And respective method may be guarded with your custom logic!

Hope this helps

izundo-viennv commented 4 years ago

@johannesschobel So, we have to create a lot of dtos, but with that we will address performance issues

johannesschobel commented 4 years ago

Other possibility would be to create dedicated property resolver functions for each property and check within this function whether the user is allowed to get this data. Otherwise return null

doug-martin commented 4 years ago

@izundo-viennv so, I played with this some, and I think given the current state of nestjs and type-grapqhl I think the best way to approach this currently, is to keep your DTOs as they are, and to add property resolver to your resolver for the restricted fields.

For example...

Assume you have the following DTO.

import { FilterableField } from '@nestjs-query/query-graphql';
import { ObjectType, ID, GraphQLISODateTime, Field } from 'type-graphql';

@ObjectType('TodoItem')
export class TodoItemDTO {
  @FilterableField(() => ID)
  id!: string;

  @FilterableField()
  title!: string;

  @FilterableField({ nullable: true })
  description?: string;

  @FilterableField()
  completed!: boolean;

  @FilterableField(() => GraphQLISODateTime)
  created!: Date;

  @FilterableField(() => GraphQLISODateTime)
  updated!: Date;

  @Field()
  age!: number;
}

You could have the following resolver to check the title field.

NOTE the @ResolveProperty() for the title property, you can add guards to to that field.

import { Filter } from '@nestjs-query/core';
import { ConnectionType, CRUDResolver } from '@nestjs-query/query-graphql';
import { Args, Query, Resolver, Parent, ResolveProperty } from '@nestjs/graphql';
import { UseGuards } from '@nestjs/common';
import { AuthGuard } from '../auth.guard';
import { SubTaskDTO } from '../sub-task/dto/sub-task.dto';
import { TagDTO } from '../tag/dto/tag.dto';
import { TodoItemInputDTO } from './dto/todo-item-input.dto';
import { TodoItemUpdateDTO } from './dto/todo-item-update.dto';
import { TodoItemDTO } from './dto/todo-item.dto';
import { TodoItemService } from './todo-item.service';

const guards = [AuthGuard];

@Resolver(() => TodoItemDTO)
export class TodoItemResolver extends CRUDResolver(TodoItemDTO, {
  CreateDTOClass: TodoItemInputDTO,
  UpdateDTOClass: TodoItemUpdateDTO,
  create: { guards },
  update: { guards },
  delete: { guards },
  relations: {
    many: {
      subTasks: { DTO: SubTaskDTO, disableRemove: true, guards },
      tags: { DTO: TagDTO, guards },
    },
  },
}) {
  constructor(readonly service: TodoItemService) {
    super(service);
  }

  @UseGuards(AuthGuard)
  @ResolveProperty()
  title(@Parent() todoItem: TodoItemDTO): string {
    return todoItem.title;
  }
}

To make this work you should add the following line to your graphql module

fieldResolverEnhancers: ['guards', 'interceptors'],

For more detail see https://github.com/nestjs/graphql/issues/295#issuecomment-511191060

johannesschobel commented 4 years ago

That is what I meant in my previous post. However I was not sure if you could directly add the guards to the method itself!

Great work

izundo-viennv commented 4 years ago

@doug-martin Can it be a performance issue or not?

johannesschobel commented 4 years ago

i think, it may be slower, as additional methods with respective logic have to be called and performed. However, I don't think that it will dramatically decrease the performance of your API..

izundo-viennv commented 4 years ago

But I think service shouldn't return DTO, because we don't just simple to create and update directly to db, we need to add some logic (transaction, ...) before insert and update to db. That will be good if converting performed at resolvers without services. Services should only work with entities

johannesschobel commented 4 years ago

Dear @izundo-viennv ,

that the service should or should not return an ObjectType is another issue that has nothing to do with your current issue of restricting access to particular fields.

The proposed solution of limiting the access via @ResolveProperty() methods would be implemented in the respective Resolver of the ObjectType and, therefore, has nothing to do with the underlying Service.

Finally, it will most likely be no performance boost in respect to the service layer (and the underlying database) if you don't query all fields (i.e., request only particular fields itself) because returning an entire tuple (i.e., row of the table) is always the fastest operation for the DB, as no additional logic is required. Therefore, it is better to filter in a "higher level".

All the best