Use HasPermission in API controllers

[dtslvr]

With the groundwork of #2693 it is now possible to guard endpoints with the following code:

  @UseGuards(HasPermissionGuard)  // can be on the controller or handler
  @HasPermission('somePermissionKey') // on the handler

See comment by @underwater in https://github.com/ghostfolio/ghostfolio/issues/2693#issuecomment-1833383105

[dtslvr]

@underwater are you interested in contributing that?

[underwater]

Hi @dtslvr, I suppose you are referring to refactoring code that explicitly verifies permission in the controllers, with the usage of the guard / decorator. Yes, sure... I thought before starting though, to write a spec that is alongside the controller to allow me that the behavior around the controller is still the same ? I can do it for a simple controller like CacheController and make a p/r so you can see what describing, and give me your feedback.

[underwater]

good morning @dtslvr

Before starting, wanted your feedback on the below:

1 - I plan to register HasPermissionGuard globally (it won't impact any handlers that don't specifically request a specific permission)

2 - For controllers that impose the same permission on all handlers (example : admin.controller.ts), I could place the @HasPermission decorator at the class level,

3 - Conversely for those that have different permissions for each handler (tag.controller.ts), I can add @HasPermission at the handler level.

4 - for controllers that have checks for various other concerns in addition permission checking I plan to leave those parts of the code in place ... For example order.controller.ts, checks the order is valid and belongs to the same logged in user before allowing a delete.

// order.controller.ts
  @Delete(':id')
  @UseGuards(AuthGuard('jwt'))
  public async deleteOrder(@Param('id') id: string): Promise<OrderModel> {
    const order = await this.orderService.order({ id });

    if (
      !hasPermission(this.request.user.permissions, permissions.deleteOrder) ||
      !order ||
      order.userId !== this.request.user.id
    ) {
      throw new HttpException(
        getReasonPhrase(StatusCodes.FORBIDDEN),
        StatusCodes.FORBIDDEN
      );
    }

Do you agree with this approach ? If so, I can do the refactoring...

this will impact the below controllers

Your thoughts appreciated P.S. I am travelling next 2 days, but will attend upon my return.

[dtslvr]

Thank you for this overview, @underwater.

1. Okay, that makes it less verbose
2. I would add @hasPermission explicitly for each endpoint, otherwise you need to think about the impact of all endpoints when something changes
3. I think this is the way to go
4. What do you think about this?

// order.controller.ts
  @Delete(':id')
  @HasPermission(permissions.deleteOrder)
  @UseGuards(AuthGuard('jwt'))
  public async deleteOrder(@Param('id') id: string): Promise<OrderModel> {
    const order = await this.orderService.order({ id });

    if (
      !order ||
      order.userId !== this.request.user.id
    ) {
      throw new HttpException(
        getReasonPhrase(StatusCodes.FORBIDDEN),
        StatusCodes.FORBIDDEN
      );
    }

[underwater]

thanks for quick @dtslvr

(points 2,3) I agree about positioning of decorators, will do as you suggest.

(point 4) yes .... I wasn't clear in my initial message, but I intended to take out only that code relating to permission checking, and only keep the code relating to business rule and validation :-)

[underwater]

@dtslvr just heads up that I will be absent for next 10 days due to medical reasons, and won't manage to work on this, but the permiter is clear, and I will do it upon my return.

[dtslvr]

Thanks for letting me know. Take care of yourself and get well soon, @underwater!

[underwater]

Hi @dtslvr I am back, and will be looking at this over the weekend

[underwater]

first stage of refactoring controllers to make use of @HasPermission() decorator Merged #2771

Still pending to do:

• extending the Guard / Decorator to handle multiple permissions, to allow us complete the refactoring of those controllers that check multiple roles.