search

HackYourFuture-CPH / FP-class11

Class11 Final Project - A collaboration between HYF and Seasony
MIT License
5 stars 8 forks source link

Swagger update batch default values #234

Closed ricardoaguiar closed 3 years ago

ricardoaguiar commented 4 years ago

Partially resolves #232

cecastosic commented 4 years ago

Great job 💪 I have one more comment as a general comment. For every endpoint there is a description for the authentication part like this: Authentication: super_admin admin user But that's related to authorization and the authorization middleware, which we use for several endpoints, but not on all. I would suggest change to: Authentication: user and for endpoints that use the authorization middleware Authorization: super_admin admin or Authorization: super_admin depends on the endpoint.

ricardoaguiar commented 4 years ago

Do you mean to remove the super_admin and adminfor the endpoints that are user authorized?

I have changed them to: Authentication: super_admin admin Authorization: user

Isn't a bit more clear than having just one level, i.e. either authentication or authorization?

Thanks

cecastosic commented 4 years ago

Authentication: super_admin admin

Authorization is related to roles (super_admin, admin...), authentication is related to is the user logged in or not. This endpoint doesn't have anything with roles ie there is no usage of authorization middleware. IMO all endpoints that don't use authorization middleware shouldn't have authorization in the description, only authentication.

ricardoaguiar commented 4 years ago

Final revision. We are still missing a couple of endpoints that we may have to implement at some point. All we need for now is to approve and merge the PR.

Thanks.