-
-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use HasPermission in API controllers #2706
Comments
@underwater are you interested in contributing that? |
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. |
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.
Do you agree with this approach ? If so, I can do the refactoring... this will impact the below controllers Your thoughts appreciated |
Thank you for this overview, @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 :-) |
@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. |
Thanks for letting me know. Take care of yourself and get well soon, @underwater! |
Hi @dtslvr I am back, and will be looking at this over the weekend |
first stage of refactoring controllers to make use of @hasPermission() decorator Merged #2771 Still pending to do:
|
With the groundwork of #2693 it is now possible to guard endpoints with the following code:
See comment by @underwater in #2693 (comment)
The text was updated successfully, but these errors were encountered: