Defined and used permissions #11
Conversation
| @@ -60,7 +77,14 @@ export function attachControllers( | |||
|
|
|||
| // Get all mailing lists | |||
| binder.attach( | |||
| {endPoint: endpoints.mailinglist.list}, | |||
| { | |||
| endPoint: endpoints.mailinglist.list, | |||
There was a problem hiding this comment.
This is used to list all available mailing list, therefore a user with no permission wouldn't be able to see the mailing list available for himself.
There was a problem hiding this comment.
we probably need 2 different endpoints to solve that actually. I would just comment this with a fixme with a link to an issue to solve that problem
There was a problem hiding this comment.
Then, what is this one for? https://github.com/my-koop/module.mailing-list/blob/final/lib/controllers/index.ts#L67
Unless "in the registration page" doesn't imply "public", which I thought it did.
There was a problem hiding this comment.
actually it the opposite, public doesn't imply "in the registration page", that route is use to get only mailing in the new user registration page, therefore there has to be another route for GetAvailableMailingList to the requesting user
There was a problem hiding this comment.
I just meant that I thought they were one and the same. If they're not, then there do is a problem with the way you currently retrieve mailing lists for the "my account" panel.
There was a problem hiding this comment.
indeed, they are not the same, and we have a problem :P
|
CR Done |
|
Didn't we agreed to remove the permission on this route for now ? |
|
Please place your comment in the code next to where it's relevant, because I have no clue what route you are talking about... I removed the permissions for the only one that was mentioned in the review: 46e6be7 |
|
46e6be7#diff-47ac35e58ff445a62be39a4cbf24b360R157 how is this a removal ? you put a fixme |
|
That is a route that exposes data that the user shouldn't have access to (all mailing lists), doesn't it deserve a fixme? (We discussed we would make two controllers later on) |
|
Yes but we also said that this shouldn't have a permission attached to it until said second route existed. Also this is a public route at this point because mailing don't have permissions attached to them. Therefore there should be no permissions on that route |
|
But there is none... I commented it. I'll admit I have absolutely no clue what else you want me to do here... |
|
... hurray for no code coloring on github. that |
|
😓 Please look at diffs more carefully when explicitly linked to. 😛 |
Addresses my-koop/service.website#346 for this module. Really did my best to be thorough and test different combinations of permissions, but I obviously couldn't test all of them.