Permalink
Browse files

Improves error handling behavior. Closes #578.

* OmniAuth::FailureEndpoint will raise any encountered
  errors out if RACK_ENV is development.
* It will redirect compatibly with current OmniAuth if
  not in development.
* Now uses Rack::Response to include a content length
  in the failure response.
  • Loading branch information...
1 parent 5fbc419 commit d02f9d58f70f132c856199470f94b93c852d9bb0 Michael Bleigh committed Mar 6, 2012
Showing with 74 additions and 8 deletions.
  1. +1 −1 .rspec
  2. +9 −0 Gemfile
  3. +2 −0 lib/omniauth.rb
  4. +16 −1 lib/omniauth/failure_endpoint.rb
  5. +0 −5 omniauth.gemspec
  6. +45 −0 spec/omniauth/failure_endpoint_spec.rb
  7. +1 −1 spec/omniauth/strategy_spec.rb
View
2 .rspec
@@ -1,2 +1,2 @@
--color
---order random
+--order random
View
@@ -1,4 +1,13 @@
source 'https://rubygems.org'
gem 'rack', '~> 1.4'
+gem 'growl'
+gem 'guard'
+gem 'guard-bundler'
+gem 'guard-rspec'
+gem 'pry'
+gem 'pry-nav'
+gem 'plymouth'
+gem 'rb-fsevent'
+
gemspec
View
@@ -2,6 +2,8 @@
require 'singleton'
module OmniAuth
+ class Error < StandardError; end
+
module Strategies
autoload :Developer, 'omniauth/strategies/developer'
end
@@ -1,4 +1,10 @@
module OmniAuth
+ # This simple Rack endpoint that serves as the default
+ # 'failure' mechanism for OmniAuth. If a strategy fails for
+ # any reason this endpoint will be invoked. The default behavior
+ # is to redirect to `/auth/failure` except in the case of
+ # a development `RACK_ENV`, in which case an exception will
+ # be raised.
class FailureEndpoint
attr_reader :env
@@ -11,9 +17,18 @@ def initialize(env)
end
def call
+ raise_out! if ENV['RACK_ENV'].to_s == 'development'
@mkdynamic

mkdynamic May 19, 2012

Contributor

This seems undesirable, why would you not want to see the real failure flow in development?

@mbleigh

mbleigh May 19, 2012

Contributor

Because most of the time if an error happens in development it is something you are trying to fix, not expecting. You can still write tests assuming the full flow and look at the failure case in development just by visiting /auth/failure. It's a debugging aide.

@nickw

nickw May 19, 2012

Agreed. This caused me several hours of grief. I agree with the original proposal in ticket #578 that there should be some sort of raise_exceptions option.

@mkdynamic

mkdynamic May 19, 2012

Contributor

Got it. The debugging argument makes sense but I think I still disagree it's desirable.

In my experience, the primary scenario in development is building the failure case out, which would mean (by default) rendering /auth/failure so you would see what the end user sees.

It's not really an exception case (like you say, it's using exceptions as messaging for a debug aid), it's a failure case. Like a failed model validation. That is the way I see it at least.

My 2 cents :)

+ redirect_to_failure
+ end
+
+ def raise_out!
+ raise env['omniauth.error'] || OmniAuth::Error.new(env['omniauth.error.type'])
+ end
+
+ def redirect_to_failure
message_key = env['omniauth.error.type']
new_path = "#{env['SCRIPT_NAME']}#{OmniAuth.config.path_prefix}/failure?message=#{message_key}"
- [302, {'Location' => new_path, 'Content-Type'=> 'text/html'}, []]
+ Rack::Response.new(["302 Moved"], 302, 'Location' => new_path).finish
end
end
end
View
@@ -10,14 +10,9 @@ Gem::Specification.new do |gem|
gem.add_runtime_dependency 'rack'
gem.add_runtime_dependency 'hashie', '~> 1.2'
- gem.add_development_dependency 'growl'
- gem.add_development_dependency 'guard'
- gem.add_development_dependency 'guard-bundler'
- gem.add_development_dependency 'guard-rspec'
gem.add_development_dependency 'simplecov'
gem.add_development_dependency 'rack-test'
gem.add_development_dependency 'rake'
- gem.add_development_dependency 'rb-fsevent'
gem.add_development_dependency 'rdiscount'
gem.add_development_dependency 'rspec', '~> 2.8'
gem.add_development_dependency 'yard'
@@ -0,0 +1,45 @@
+require 'spec_helper'
+
+describe OmniAuth::FailureEndpoint do
+ subject{ OmniAuth::FailureEndpoint }
+
+ context 'development' do
+ before do
+ @rack_env = ENV['RACK_ENV']
+ ENV['RACK_ENV'] = 'development'
+ end
+
+ it 'should raise out the error' do
+ err = StandardError.new("Blah")
+ expect{ subject.call('omniauth.error' => err) }.to raise_error(err)
+ end
+
+ it 'should raise out an OmniAuth::Error if no omniauth.error is set' do
+ expect{ subject.call('omniauth.error.type' => 'example') }.to raise_error(OmniAuth::Error, "example")
+ end
+
+ after do
+ ENV['RACK_ENV'] = @rack_env
+ end
+ end
+
+ context 'non-development' do
+ let(:env){ {'omniauth.error.type' => 'invalid_request'} }
+
+ it 'should be a redirect' do
+ status, head, body = *subject.call(env)
+ status.should == 302
+ end
+
+ it 'should include the SCRIPT_NAME' do
+ status, head, body = *subject.call(env.merge('SCRIPT_NAME' => '/random'))
+ head['Location'].should == '/random/auth/failure?message=invalid_request'
+ end
+
+ it 'should respect configured path prefix' do
+ OmniAuth.config.stub(:path_prefix => '/boo')
+ status, head, body = *subject.call(env)
+ head["Location"].should == '/boo/failure?message=invalid_request'
+ end
+ end
+end
@@ -509,7 +509,7 @@ def make_env(path = '/auth/test', props = {})
end
it 'should be case insensitive on callback path' do
- strategy.call(make_env('/AUTH/TeSt/CaLlBAck')).should == strategy.call(make_env('/auth/test/callback'))
+ strategy.call(make_env('/AUTH/TeSt/CaLlBAck')).first.should == strategy.call(make_env('/auth/test/callback')).first
end
it 'should maintain query string parameters' do

0 comments on commit d02f9d5

Please sign in to comment.