TriPSs/nestjs-query

Authorize decorator not working on CreateDTOClass and UpdateDTOClass

Closed this issue · 10 comments

JSee98 commented

Describe the bug
If we define a Authorize decorator over CreateDTOClass or UpdateDTOClass, those authorize functions are skipped over and the Authorize decorator defined over DTOClass (the default DTO class) in the resolver are only applied.

To Reproduce
Steps to reproduce the behavior:

  1. Create a separate DTO class for update and read dto
  2. Add an authorizer decorators on both DTOs with different logic
  3. You will notice the authorizer defined for update dto isn't called

Expected behavior
All the authorizers should work, whether it is defined over the DTOClass or UpdateDTOCLass or CreateDTOClass

Desktop (please complete the following information):

  • Node Version: 16.17.1
  • Nestjs-query Version: 1.1.1
TriPSs commented

The @Authorize decorator needs to be added to the DTOClass from the module, not to the hooks. See here, inside the context you can add specifics for create/update.

JSee98 commented

Yeah, I saw that we can use the authorization context and get the same done, however, the idea is that if we can define a separate DTO for create and update operations, we should be able to define authorizers for them over the CreateDTO and UpdateDTO classes themselves instead of adding the logic for the same under the DTOClass object.

This is more like a bug + good coding practice

TriPSs commented

I see what you mean, but I do not agree with that approach. The main dto is the holder of that information and therefore you can set the authoriser there, also if you don't define a update/create DTO you still want to be able to customise the auth.

This is more like a bug + good coding practice

I do not see this as a bug.

JSee98 commented

Well even if it is not bug this might still be a valid enhancement. I don't think we should consider a single DTO as the main DTO. Moreover, I agree with the point that if a single DTO is defined one should be able to define the authorizer over there, thus, a simple priority can be defined for the authorizer, i.e. use Update specific authorizer if defined if not use the one defined under the DTOClass

TriPSs commented

I don't think we should consider a single DTO as the main DTO.

Well, it is the main DTO cause that is the data the resolver will output and, based on that, input / update.

Sorry but I really do not see any added benefit in doing this, it will only cause more complexity to the code base and usage of the authoriser.

@smolinari @GP4cK @mrvinceit what do you guys think of this?

I'm not using the @Authorize decorator myself, but my understanding is, you can also create custom authorizers, which can determine the CRUD operation and can filter accordingly. So, you could create 4 authorizers for each CRUD operation, right?

Scott

TriPSs commented

Currently you can create one where you can add more/less filters based on the context (UPDATE/CREATE/DELETE/AGGREGATE). The issue proposed by @JSee98 wants to have the ability to also add the decorator on the create/update DTO's, then you could in total add that decorator on 3 different places and it will then select one (Context staying the same).

I'm in agreement with @TriPSs, the main DTOClass is very central to the various resolvers and must always have DTOClass defined, whereas the other CreateDTO/UpdateDTO are optional, so having the @Authorize defined at the DTOClass level makes for a consistent usage for all types especially since all CRUD operations are supported.

GP4cK commented

As far as I know/understand, the @Authorize decorator is used to filter the returned data. It seems a bit strange to me to filter out the result of a create / update. Maybe what you're looking for is more a guard? Then you can have different ones for create / update.

As far as I know/understand, the @Authorize decorator is used to filter the returned data. It seems a bit strange to me to filter out the result of a create / update. Maybe what you're looking for is more a guard? Then you can have different ones for create / update.

Ah. Yes. Good catch. Create and Update DTOs would be input types. 🤦

That being said, I could still see there might be use cases, when you would want to do some sort of filter logic for the update and create operations, so still good nestjs-query accounts for that.

@TriPSs

Currently you can create one where you can add more/less filters based on the context

How about adding a @UseAuthorizers decorator, similar to Nest's @UseGuards to at least have a way to break out the different operations into their own custom authorizer classes?

Something like:

@ObjectType('TodoItem')
@UseAuthorizers(CreateTodoItemAuthorizer, ReadTodoItemAuthorizer, UpdateTodoItemAuthorizer, DeleteTodoItemAuthorizer)
export class TodoItemDTO {
...
} 

🤔 😁

Scott