kikisp / CIintegrationTests

MIT License
0 stars 0 forks source link

Code review #1

Open Atalanta opened 4 years ago

Atalanta commented 4 years ago
Atalanta commented 4 years ago

Could you explain how auth works in this project? It looks like you’re combining two different approaches - two different tokens... are you intending to use OAUTH for identity/authentication or for authorization/delegation?

Atalanta commented 4 years ago

A few more questions:

Atalanta commented 4 years ago

I noticed you have a path /verification/confirm-account, which is exposed as a GET request

Are you familiar with Roy Fielding's paper on REST?

https://www.ics.uci.edu/~fielding/pubs/dissertation/rest_arch_style.htm#sec_5_2_1_1

As a general rule of thumb we like to use nouns to represent resources - in your case confirm-account is an action, which is used with another action (GET, as stated by protocol). How would you feel about renaming it to, say, accountID or account-confirmations?

kikisp commented 4 years ago
* I like that you use database migrations, and current versions of Spring Boot and Java

* I also like that you're familiar with optionals and the streams API

* I noticed that there aren’t any tests - what sort of tests would you consider adding?

About tests let me just say that code in this repo is really basic type of skeleton code that was used for me just to check is basic functions.Newer version that we have on private repo and in prod has tests written.As for test we used unit tests to test some small pieces of code like does returned user has all necessary fields.And on a bigger scale we used integration tests to see for example did the registration succeeded and confirmation mails got sent without errors.

kikisp commented 4 years ago

Could you explain how auth works in this project? It looks like you’re combining two different approaches - two different tokens... are you intending to use OAUTH for identity/authentication or for authorization/delegation? Ok if i understand correctly you can see different approach with token because here we have 2 types of registration on is local registration where our system generate jjwt token and second is because we have to deal with 3rd party oauth providers like fb and google .. Also you can see my implementation of registration token. thats the token that get sent by email to confirm membership.

kikisp commented 4 years ago

A few more questions:

* Did you consider having some of the logic in dedicated services rather than in the controller?

* Could you clarify the hierarchy of roles/privileges?  For example the role can be set on the endpoint directly without any validation or authorization?  Who has permissions to set roles for the user?

I agree with with you that all logic should be in service layer.I look at that like this Controllers should be used to delegate work to service ,but as i said skeleton code :) As for roles that was a part of the design so to say .Because at the time we didnt have and functionality that can manage role on the client side .Here we have a thing were we just manually add it.In production code we have something like user system where you can have super admin ,admin and user and they have hierarchy established so for example admin can change permissions only for users that are delegated to him and super admins can manage admins.

kikisp commented 4 years ago

I noticed you have a path /verification/confirm-account, which is exposed as a GET request

Are you familiar with Roy Fielding's paper on REST?

https://www.ics.uci.edu/~fielding/pubs/dissertation/rest_arch_style.htm#sec_5_2_1_1

As a general rule of thumb we like to use nouns to represent resources - in your case confirm-account is an action, which is used with another action (GET, as stated by protocol). How would you feel about renaming it to, say, accountID or account-confirmations?

I agree with you on this. "account-confirmations" is suitable for me or maybe just /confirmations i would skip"accountID" because if i saw that url i would think that i would get back id of user that was confirmed and thats not the case