nestjsx / crud

NestJs CRUD for RESTful APIs
https://github.com/nestjsx/crud/wiki
MIT License
4.04k stars 533 forks source link

NestJS Crud Controller and Service does not allow to access user context #699

Open jimjaeger opened 3 years ago

jimjaeger commented 3 years ago

Frameworks like PassportJS extend Express Request Context with User information attribute. This attribute can be used NestJS CRUD @CrudAuth annotation but is not available in the parsed request object (CrudRequest). I am not sure the idea of desired NestJS Crud access control handling. But as result it is impossible to access the user information in the NestJS Crudservice.

The usecase is access control based on user context and resource info, like a user can only modify owned resources or has role admin.

Can you provide way to handle it or add user context to CrudRequest. Thanks.

rewiko commented 3 years ago

Hello,

I have seen a way to filter based on the user (payload decoded by PassportJS), example https://github.com/nestjsx/crud/blob/master/integration/crud-typeorm/users/me.controller.ts#L33

@CrudAuth({
  filter: (req) => ({
    id: req.user.id,
  }),
})

If you want to override the method example getMany, I guess the user should be available on the request.

export class RolesController implements CrudController<Role> {
  constructor(public service: RolesService) {}

  get base(): CrudController<Role> {
    return this;
  }

  @Override()
  getMany(@ParsedRequest() req: CrudRequest) {
    return this.base.getManyBase(req);
  }
}
jimjaeger commented 3 years ago

Hey Anthony thanks for the update. As mentioned,

Nice, with your descriptions you discover the limitation aswell :)

rewiko commented 3 years ago

you cannot rely on user provided payload, because the user could fakes the user info and you can only verify the information after the service loaded the entry from the database.

You can trust the information in req.user because the token has been verified by jwt library (only if you have the right jwt middleware to verify the token and put the result into req.user). That's the whole purpose of JWT token, using the method verify from your jwt library will validate if the token has not been changed by anyone. The user cannot directly change req.user, it can only provide req.params, req.query and req.body.

the getMany and other methods have ParsedRequest argument and it does not have addtional user context. That's true this is a typescript type.

@Post('login')
async login(@Request() req, @Response() res) {

you can remove ParsedRequest type and you might be able to get the req.user.

For access control in update/replace/delete

I would suggest using a guard to validate the users permissions.

 @Injectable()
export class AuthzGuard implements CanActivate {
  constructor(
    private rolesService: RolesService,
    @Inject(CACHE_MANAGER) private cacheManager: Cache
  ) {}

  async canActivate(ctx: ExecutionContext): Promise<boolean> {
    const handler = ctx.getHandler();

    const feature = ctx
      .getClass()
      .name.replace(/Controller$/gi, '')
      .toLowerCase();
    const action = getAction(handler).toLowerCase();
    const routePermission = `${feature}:${action}`;
    const { user } = ctx.switchToHttp().getRequest();
rewiko commented 3 years ago

But I get what you mean it would be nice to have access to the user info via ParsedRequest because @Request() is not working.

jimjaeger commented 3 years ago

I talk about using Nest Crud controller with Crud decorator and CrudService, to get CRUD for resource and not implement the mehods on my own. May we do not talk about the same thing. With "user provided payload", I mean the body information like the resource and not the user context provided by authentication information. I do not talk about about authentication (user is identified and validated), I talk about authorization (user is allowed to execute the dedicated action). :)

For example, Access control enforces that only the owner of the resource or admin is allowed to modify (PATCH,PUT,DELETE) the resource and the owner informations is stored as part of the resource, e.g. as owner key. The controller get a request and delegates to the service, the service loads the resource and execute the action.

If I would do the authorization check in the guard, the guard has to load the resource, and when successful authorization the service would load the resource again. To avoid double load, the service could do the authorization handling after load the original resource document. To be able to do this the user context needs to be passed into the service.

Guard (for authentication) --> CrudController --> CrudService (do authorization and execute action).

So I am looking how NestJS design support it efficient and may it is a gap that could be solved.

rewiko commented 3 years ago

https://github.com/nestjsx/crud/blob/master/packages/crud/src/decorators/parsed-request.decorator.ts#L8 R.getContextRequest(ctx).user returns the data present into the jwt but it is not mapped into ParsedRequest

rewiko commented 3 years ago

I would suggest you to read https://docs.nestjs.com/security/authorization. I don't think the checking on the service is valid for optimization. If your query get some posts as a resource you will have to get the roles/permissions associated to the user, so the service posts will not have the knowledge about users/roles/permissions.

If you want to optimize the query I would suggest you to have 2 guards one for authentication (jwt validation) and another one for authorization (getting the roles/permissions associated to a user). You can store the roles into the jwt token and have a in memory/redis cache to cache the mapping between roles and permissions.

import {
  Injectable,
  CanActivate,
  ExecutionContext,
  Inject,
  CACHE_MANAGER,
  HttpStatus,
} from '@nestjs/common';
import { getAction } from '@nestjsx/crud';
import { RolesService } from '../roles/roles.service';
import { Cache } from 'cache-manager';
import { HttpException } from '@nestjs/common/exceptions/http.exception';

@Injectable()
export class AuthzGuard implements CanActivate {
  constructor(
    private rolesService: RolesService,
    @Inject(CACHE_MANAGER) private cacheManager: Cache
  ) {}

  async canActivate(ctx: ExecutionContext): Promise<boolean> {
    const handler = ctx.getHandler();

    const feature = ctx
      .getClass()
      .name.replace(/Controller$/gi, '')
      .toLowerCase();
    const action = getAction(handler).toLowerCase();
    const routePermission = `${feature}:${action}`;
    const { user } = ctx.switchToHttp().getRequest();

    // get roles from in memory cache
    // user.roles  get ids
    const rolesIds = user.roles.map(function (el) {
      return el.id;
    });
    const cacheKey = 'NestJS_Authz_Guard_roles_' + rolesIds.join('_');

    let rolesCached = await this.cacheManager.get(cacheKey);

    console.log('Log cache keys: ', rolesCached);
    if (!rolesCached) {
      const roles = await this.rolesService.find({
        where: user.roles,
        cache: true,
        relations: ['permissions'],
      });
      await this.cacheManager.set(cacheKey, roles);
      rolesCached = roles;
    }

    if (!rolesCached) {
      throw new HttpException('Roles not found.', HttpStatus.UNAUTHORIZED);
    }

    let foundPermission = false;
    if (Array.isArray(rolesCached)) {
      rolesCached.forEach((role) => {
          role.permissions.forEach((permission) => {
          if (permission.name == routePermission) {
            foundPermission = true;
          }
        });
      });
    } else {
      throw new HttpException(
        'Roles is not an array.',
        HttpStatus.UNAUTHORIZED
      );
    }

    // rolesCache
    console.log(`Authz guard: ${routePermission}`); // e.g. 'groups-read-all'
    console.log('Authz guard: req user: ', user);
    console.log('Authz guard: roles related: ', rolesCached);

    return foundPermission;
  }
}
jimjaeger commented 3 years ago

Thanks Anthony for the update.

I have to admit I already searched for and found the documentation you linked. The documentation talks about isomorphic authorization and I would like to reference that I asked for a user owned resource permission check. And It is my idea to implement such CASL based permission, as described in the NestJS doc::

Now let's review and refine our requirements for this example:

  • Admins can manage (create/read/update/delete) all entities
  • Users have read-only access to everything
  • Users can update their articles (article.authorId === userId)

As result, I need the user object and original document from the database.

const user = new User(); user.id = 1;

const article = new Article(); article.authorId = user.id;

const ability = this.caslAbilityFactory.createForUser(user); ability.can(Action.Update, article); // true

If I would implement it as Guard, I have to retrieve the original document twice per request, in the guard and in the service. To avoid double database requests I could cache the resource (feels like workaround for a Crud lib limitation) or I could do the casl execution in the CrudService, direct when the service retrieved the original document.

So, yes NestJS in general give me the pattern to do a user owned permission check but NestJS Crud does not allow because it does not pass the needed infomation in the business logic service.

As result I request to extend the ParsedRequest information with an optional user object.

hakimio commented 3 years ago

@jimjaeger take a look at the following example project I made. I use Fastify instead of Express but it should be almost the same.

vladi-strilets commented 3 years ago

Had a similar issue.

I have 2 guards: AuthGuard and RoleGuard based on NestJS documentaion.

Appling RoleGuard as global throws an error: user is undefined. Meanwhile, using both guards inside a controller solves the problem.

@Crud({
  model: {
    type: Project,
  },
  routes: {
    only: ['getOneBase', 'getManyBase'],
    getOneBase: {
      decorators: [Roles(USER_ROLE.CLIENT)],
    },
    getManyBase: {
      decorators: [Roles(USER_ROLE.CLIENT)],
    },
  },
  ... 
})
@CrudAuth({
  filter: (user: User) => {
    return {
      userId: user.id,
    };
  },
})
@UseGuards(RolesGuard) // <-- use RolesGuard inside the controller, now Roles decorators works fine
@UseGuards(AuthGuard())
@Controller('/me/projects')
export class ProjectsController implements CrudController<Project> {
  constructor(public service: ProjectsService) {}
}
yepes commented 2 years ago

@jimjaeger what did you end up doing to avoid duplicate queries?

jimjaeger commented 2 years ago

finally I had to create my own complete controller architecture to solve the issue. Unfortunately after a year it is still valid and a limitation in the framework.