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

Better handle errors that come from the actual app. #1035

Merged
merged 1 commit into from
Feb 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/omniauth/strategy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,8 @@ def call!(env) # rubocop:disable CyclomaticComplexity, PerceivedComplexity
return callback_call if on_callback_path?
return other_phase if respond_to?(:other_phase)
rescue StandardError => e
raise e if env.delete('omniauth.error.app')

return fail!(e.message, e)
end

Expand Down Expand Up @@ -302,6 +304,8 @@ def mock_call!(*)
return mock_request_call if on_request_path? && OmniAuth.config.allowed_request_methods.include?(request.request_method.downcase.to_sym)
return mock_callback_call if on_callback_path?
rescue StandardError => e
raise e if env.delete('omniauth.error.app')

return fail!(e.message, e)
end

Expand Down Expand Up @@ -462,6 +466,9 @@ def query_string

def call_app!(env = @env)
@app.call(env)
rescue StandardError => e

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if it would be possible to rescue a subclass of StandardError like OmniAuthError or something like that. I guess the challenge is that call_app is called from the other dependent gems (omniauth-saml, etc) and you can't really know what will be raised.

env['omniauth.error.app'] = true
raise e
end

def full_host
Expand Down
30 changes: 30 additions & 0 deletions spec/omniauth/strategy_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,36 @@ def make_env(path = '/auth/test', props = {})
expect(strategy.callback_url).to eq('http://example.com/myapp/override/callback')
end
end

context 'error during call_app!' do
class OtherPhaseStrategy < ExampleStrategy
def other_phase
call_app!
end
end
class AppError < StandardError; end
let(:app) { ->(_env) { raise(AppError.new('Some error in the app!')) } }
let(:strategy) { OtherPhaseStrategy.new(app) }

it 'raises the application error' do
expect { strategy.call(make_env('/some/path')) }.to raise_error(AppError, 'Some error in the app!')
end
end

context 'error during auth phase' do
class SomeStrategy < ExampleStrategy
def request_phase
raise AuthError.new('Some error in authentication')
end
end
class AuthError < StandardError; end
let(:strategy) { SomeStrategy.new(app) }

it 'passes the error to fail!()' do
expect(strategy).to receive(:fail!).with('Some error in authentication', kind_of(AuthError))
strategy.call(make_env('/auth/test', props))
end
end
end


Expand Down