adonisjs / auth

Official Authentication package for AdonisJS
https://docs.adonisjs.com/guides/auth/introduction
MIT License
192 stars 65 forks source link

feat(jwt): add login() method to jwt scheme #72

Closed radmen closed 6 years ago

radmen commented 6 years ago

When using Jwt scheme auth.login(user) fails. It's due to lack of such method in Schemes/Jwt.

This PR introduces login() method which is an alias for generate().

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.003%) to 98.381% when pulling 734cf6d64f5d96c5dd1d2f855580c24f3f696833 on radmen:login-alias-for-jwt-scheme into 8aa688685b7a3de35994a1c54b89fbb964cb3a72 on adonisjs:develop.

thetutlage commented 6 years ago

Ideally there should never be a login method in JWT, since login means, you have a stateful loggedin state, which is not true with JWT.

So it's fine to throw the exception and set right expectations

radmen commented 6 years ago

I took more detailed peek at the docs and found information about why there's a difference. I guess I can modify this PR so that login() will throw an exception.

thetutlage commented 6 years ago

But what make you think that login should work with JWT?

radmen commented 6 years ago

But what make you think that login should work with JWT?

thetutlage commented 6 years ago

When Laravel doesn't existed, in stateless auth, you cannot login ( which is way more important than reading the Laravel source code for me )

radmen commented 6 years ago

I'm using Laravel as an analogy.

Also, I think that if there's a facade (eg Auth) which acts as a proxy for underlying implementations those implementations should have a common interface.

If you think that Jwt::login() should not do anything, that's fine. Yet I think that this method should exist and simply tell the developer that one should use generate() instead.

thetutlage commented 6 years ago

Yes I am in favour of throwing a meaningful exception

radmen commented 6 years ago

Ok, I'll do it.

Should I update this branch, or create a new one with new PR?

thetutlage commented 6 years ago

You can update this branch itself

radmen commented 6 years ago

Ok, will do so. Thanks!

radmen commented 6 years ago

@thetutlage I've pushed update to PR. Now, login() will throw an exception.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.003%) to 98.381% when pulling 9f6fe35eaa98326b1dc4ece4847bcd4cecb6785a on radmen:login-alias-for-jwt-scheme into 8aa688685b7a3de35994a1c54b89fbb964cb3a72 on adonisjs:develop.

thetutlage commented 6 years ago

Looks great thanks 😄

radmen commented 6 years ago

Thank you :)