Generic error handling in Strategy #689
Conversation
|
Any word on this pull request? Its been open for quite awhile. This code is necessary for consumers of |
sferik
force-pushed
the
master
branch
2 times, most recently
from
a5c7282
to
76feeea
Compare
|
@joshjordan this had been open for 8 years, 7 since last activity, so I closed it as stale. If you're still interested in this, feel free to rebase your changes onto current master and we can go from there |
joshjordan
force-pushed
the
jsj-strategy-generic-error-handling
branch
from
7d4b496
to
e03a0b1
Compare
|
@BobbyMcWho understood, but last action was on omniauth to review, which is why I was surprised to see it closed with no comment. Updated to resolve conflicts with master. |
|
This would be a breaking change, and I'm not confident that the value that it provides is worth adding the breaking change here. As it is, any failures in the strategy do not call Is there a specific use case you can provide an example of that handling the error within one of those inherited implementations isn't an option? |
|
Sure thing -- that's the encoded in the tests. The simplest example is a generic exception connecting to the third party service when trying to swap e.g. a token for auth credentials. This isn't about strategies handling errors, its about the consumer being able to handle errors. We have the ability to set an
And that's the problem, wouldn't you agree?. They just cause the entire endpoint to return a 500, which is unexpected behavior and not useful. I would be quite surprised if anyone desires that behavior, based on the dozens of omniauth developers I've spoken with over the years. Even if it were desired, it can still be achieved because this PR puts it in the consumer's control what to do with the On that topic, I did a review of many of the most popular omniauth strategy implementations. Terribly few of them actually handle exceptions, and when I raised this issue with specific strategies' issue repos, the maintainers were surprised to find that omniauth was not already handling this. Critical implementations, such as |
|
To put it differently, this seems to me to be stricly an improvement. If you prefer, I could also allow gem consumers to opt-out of this functionality with a "no thanks, just blow up if there are exceptions" config flag. |
lib/omniauth/strategy.rb
Outdated
| return callback_call if on_callback_path? | ||
| return other_phase if respond_to?(:other_phase) | ||
| rescue => ex | ||
| return fail! ex.message, ex |
There was a problem hiding this comment.
| return fail! ex.message, ex | |
| return fail!(ex.message, ex) |
spec/omniauth/strategy_spec.rb
Outdated
| @@ -312,32 +312,41 @@ def make_env(path = '/auth/test', props = {}) | |||
| context 'disabled' do | |||
| it 'does not set omniauth.origin' do | |||
| @options = {:origin_param => false} | |||
| expect { strategy.call(make_env('/auth/test', 'QUERY_STRING' => 'return=/foo')) }.to raise_error('Request Phase') | |||
| strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) | |||
There was a problem hiding this comment.
Can you change these to the expect(strategy).to receive(:fail!) syntax?
|
I believe so — happy to make that change. Will do so after dinner.
…On Mon, Dec 7, 2020 at 5:23 PM Bobby McDonald ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In spec/omniauth/strategy_spec.rb
<#689 (comment)>:
> @@ -312,32 +312,41 @@ def make_env(path = '/auth/test', props = {})
context 'disabled' do
it 'does not set omniauth.origin' do
@options = {:origin_param => false}
- expect { strategy.call(make_env('/auth/test', 'QUERY_STRING' => 'return=/foo')) }.to raise_error('Request Phase')
+ strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError))
Can you change these to the expect(strategy).to receive(:fail!) syntax?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#689 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7LSSXLPVZIDSTKOZ2CGDSTVIWJANCNFSM4AGC45WQ>
.
|
|
You make good points, thanks for the detailed explanation. Now is probably a good time to merge this since I will also be making breaking changes to close the CVE mentioned here: #960 I'm going to point these changes at the 2_0_indev branch. |
|
Can you rebase onto 2_0_indev and resolve any conflicts? |
|
Sure thing! Should I repoint them merge target to that branch as well?
…On Mon, Dec 7, 2020 at 5:31 PM Bobby McDonald ***@***.***> wrote:
Can you rebase onto 2_0_indev and resolve any conflicts?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#689 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7LSRW3OSS6SXXUJARLLLSTVJVTANCNFSM4AGC45WQ>
.
|
|
I went ahead and pointed them at it in the GitHub GUI |
…ssage_key for FailureEndpoint handling Do not rescue around mock_call!
joshjordan
force-pushed
the
jsj-strategy-generic-error-handling
branch
from
e03a0b1
to
83d2d30
Compare
|
@BobbyMcWho thanks! I updated the PR with the requested changes |
As a consequence of the changes that were merged in #689, errors thrown by strategies that utilize other_phase (or more specifically call_app!), would be caught by omniauth, causing headaches for folks looking to have those errors handled by their application. This should allow for errors that come from the app to pass through, while passing errors that come from the authentication phases to the fail! handler.
Rescues from strategy calls by calling fail!, properly URL encodes message_key for FailureEndpoint handling