JujaLabs / itevents

Resource to subscribe on it events
Apache License 2.0
7 stars 5 forks source link

192: Code 403 if logging with incorrect password #200

Closed IgorMaksymov closed 8 years ago

IgorMaksymov commented 8 years ago

connect to #192

vaa25 commented 8 years ago

@IgorMaksymov Why 403?

IgorMaksymov commented 8 years ago

@vaa25 trying to acces restricted area. or its better to 401? or 400?

vaa25 commented 8 years ago

@IgorMaksymov I think 404 is better, the same as with wrong otp

IgorMaksymov commented 8 years ago

@vaa25 broken link is not incorrect login

IgorMaksymov commented 8 years ago

@vaa25 i think, that we need to throw EntityNotFound exception if login/password incorrect, not telling what exactly is incorrect.

IgorMaksymov commented 8 years ago

@vaa25 should we remove InvalidPasswordException and change it to EntityNotFound with message "user or password incorrect"?

vaa25 commented 8 years ago

@IgorMaksymov do as @alex-anakin said at tonight meeting. One exception for both login and password fails.

IgorMaksymov commented 8 years ago

@vaa25 @alex-anakin said that 401 is correct code

romach commented 8 years ago

@IgorMaksymov need to merge master

IgorMaksymov commented 8 years ago

@romach fixed, but this branch has no conflicts with master, it is faster and easier to merge by yourself, than waiting for me.

vkuchyn commented 8 years ago

@IgorMaksymov it is best practices before PR merge master into feature branch. It will prevent from lots of problems in future. It is responsibility of developer, not reviewer. So, for future - please keep in mind this point

IgorMaksymov commented 8 years ago

@vkuchyn yes, i merge my branch with master before PR. Idea was to merge master into feature branch by reviewer if it has no conflict with master. If there is some conflicts - author solves them by himself.

vkuchyn commented 8 years ago

@IgorMaksymov I am not agree with your point. Only author of the branch is responsible to do some changes. If master was updated when issue was in review and no conflicts - ok, could happen. But keep in mind - every time you do some changes to branch - check if there are no any updates from master and merge them if so

IgorMaksymov commented 8 years ago

@vkuchyn ok, i will merge master even if there is no conflicts before making new commits to PR. we need to add this to workflow

romach commented 8 years ago

@AndriyBaibak 30 minutes spent