Skip to content
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

Improve response and dispatch event in AuthenticationFailureHandler #28

Merged
merged 1 commit into from
Sep 16, 2014

Conversation

EmmanuelVella
Copy link
Contributor

Response status code is changed from 401 (Unauthorized) to 400 (Bad Request) (see http://stackoverflow.com/a/1960453). I think this makes more sense if the login parameters are invalid.

Response is now a valid json object containing a code and a message attributes.

The handler now dispatch a failure event.

@carlcraig
Copy link

The updated Json Response is good, although i am unsure if 400 is the appropriate response code. I would normally consider 400 a result of malformed request. i.e. missing parameters or data etc.

I think if the username/password is incorrect it should return a 401.
If the username or password parameter is missing, should return 400.
What do you think?

The failure event will be useful too.

@EmmanuelVella
Copy link
Contributor Author

Yeah I'm not 100% sure about the 400 status code in this case, so I think we can keep the old 401 (PR is updated).

@slashfan
Copy link
Contributor

401 is the proper code to use in my opinion. But nice work !
I'm waiting for travis to finish the build. Is it ok to merge for you ?

@EmmanuelVella
Copy link
Contributor Author

Travis build passed, you can merge it if @carlcraig is ok ;)

@carlcraig
Copy link

👍 Looks Good

slashfan added a commit that referenced this pull request Sep 16, 2014
Improve response and dispatch event in AuthenticationFailureHandler
@slashfan slashfan merged commit b804d94 into lexik:master Sep 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants