Prevent login! failure blocks from masking InvalidCredentials#440
Prevent login! failure blocks from masking InvalidCredentials#440willnet wants to merge 3 commits into
Conversation
login! is documented to raise Sorcery::InvalidCredentials when authentication fails. When authenticate yields a failure reason, forwarding the caller's block to login lets controller code in the failure branch run before login! can raise, so errors such as a missing template can replace InvalidCredentials. Call login without forwarding the block, then yield to the caller only after a successful authentication. This keeps login's failure-reason block behavior intact while preserving login!'s raise-on-failure contract.
There was a problem hiding this comment.
Pull request overview
This PR adjusts Sorcery::Controller#login! to preserve its documented contract of raising Sorcery::InvalidCredentials on authentication failure, even when the underlying authenticate implementation yields a failure reason (which previously allowed caller-provided blocks to run and potentially raise other errors first).
Changes:
- Update
login!to callloginwithout forwarding the caller’s block, then yield only after a successful authentication. - Update the controller spec to stub
User.authenticateusing a yielded failure reason for the#login!failure case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| spec/controllers/sorcery_controller_spec.rb | Updates a failure stub to simulate authenticate yielding a failure reason. |
| lib/sorcery/controller.rb | Prevents login! from executing caller blocks on authentication failure; yields only after success. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def login!(*credentials) | ||
| user = login(*credentials) | ||
|
|
||
| raise Sorcery::InvalidCredentials if user.nil? | ||
|
|
||
| user | ||
| block_given? ? yield(user, nil) : user |
There was a problem hiding this comment.
Please add/adjust a spec for #login! with block where User.authenticate yields (user, :invalid_password) and assert that the request raises Sorcery::InvalidCredentials
The spec updated in this PR is the one you're referring to.
There was a problem hiding this comment.
I think here there is still an omission. user was always returned previously, now it's only returned if a block wasn't given. Someone might be using a block and then also persisting the return value of login!.
| def login!(*credentials) | ||
| user = login(*credentials) | ||
|
|
||
| raise Sorcery::InvalidCredentials if user.nil? | ||
|
|
||
| user | ||
| block_given? ? yield(user, nil) : user |
There was a problem hiding this comment.
I think here there is still an omission. user was always returned previously, now it's only returned if a block wasn't given. Someone might be using a block and then also persisting the return value of login!.
| context 'when fails' do | ||
| before do | ||
| allow(User).to receive(:authenticate).with('bla@example.com', 'opensesame!').and_return(nil) | ||
| allow(User).to receive(:authenticate).with('bla@example.com', 'opensesame!').and_yield(user, :invalid_password) |
There was a problem hiding this comment.
It doesn't really matter, but this would be more correct if it yielded nil as the second parameter since that's what the real method does. You could probably add .and_return(user) to the end also. None of these paths are even followed in the failure case though. You could potentially have neither and_yield and and_return and it would be fine?
You could add some more tests that test that when you rescue the error, that the user is yielded along with a nil second value, and that the user is returned after the block is executed? And a test where there is no block passed.
Keep yielding to a login! success block for compatibility, but do not let the block return value replace the authenticated user returned by login!.
Move the yielded failure regression coverage onto the block form of login!, where controller failure handling can otherwise mask Sorcery::InvalidCredentials. Also assert that the success block form still assigns the authenticated user, matching login!'s user-returning contract.
This PR fixes a bug that I found while trying to remove the mock in #438.
login! is documented to raise Sorcery::InvalidCredentials when authentication fails. When authenticate yields a failure reason, forwarding the caller's block to login lets controller code in the failure branch run before login! can raise, so errors such as a missing template can replace InvalidCredentials.
Call login without forwarding the block, then yield to the caller only after a successful authentication. This keeps login's failure-reason block behavior intact while preserving login!'s raise-on-failure contract.
Please ensure your pull request includes the following: