Implement #login! helper #332
Conversation
Spone
force-pushed
the
login-bang-method
branch
from
18f0b56
to
1f5ce59
Compare
There was a problem hiding this comment.
Please update the error class to mirror the sorcery/errors.rb in v1. That would help with porting this forward into the v1 codebase.
I'll need to take a closer look at the tests and run them locally to understand better what's going on there. It might be fine as-is, but as the login method is critical to the security of Sorcery, I want to be very thorough with testing its functionality.
lib/sorcery/controller.rb
Outdated
| @@ -63,6 +65,16 @@ def login(*credentials) | |||
| end | |||
| end | |||
|
|
|||
| def login!(*credentials) | |||
There was a problem hiding this comment.
We'll need to test this and see how it interacts when given a code block to yield to.
There was a problem hiding this comment.
I'm not sure what the behavior should be when this method is called with a block. What do you suggest?
There was a problem hiding this comment.
Essentially, sanity checks that the method performs as expected when passing a block.
This wiki page has an example of using the block for raising different error messages during login: https://github.com/Sorcery/sorcery/wiki/Distinguish-login-failure-reasons
When given a code block, we can test that it succeeds when given valid credentials, fails when given invalid credentials, and raises your new error when given a blank string or nil value.
There was a problem hiding this comment.
@joshbuker I added specs for the block version of #login!. I have a few notes:
- I can't find tests for the block version of
#login - I'm not sure what the behavior should be when calling
#login!with a block and with wrong credentials. I would expect it to always raise theInvalidCredentialsexception, else I don't see the value in using#login!
|
Just fixed conflicts, if you want to review again @joshbuker |
Close #327
This PR adds a
login!method, that behaves likeloginexcept it raises an exception if the login fails.See #327 for more details.
Please ensure your pull request includes the following: