omniauth / omniauth-browserid

BrowserID strategy for OmniAuth
38 stars 12 forks source link

Including browser_id strategy breaks /auth/failure callback #2

Open mcoms opened 11 years ago

mcoms commented 11 years ago

In a rails app, adding provider :browser_id to the bottom of my initialiser block causes all strategy failures to be detected as browser_id failures and causes a redirect loop at /auth/failure.

Steps to reproduce:

A rails app with some strategies and an initializer:

# Gemfile
gem 'omniauth'
gem 'omniauth-openid'
gem 'omniauth-browserid'
gem 'omniauth-identity'
# config/initializers/omniauth.rb
Rails.application.config.middleware.use OmniAuth::Builder do
  provider :openid
  provider :open_id, :name => 'google', :identifier => 'https://www.google.com/accounts/o8/id'
  provider :identity, :fields => [:email]
  provider :browser_id
end

Comment browser_id from the initializer and visit /auth/google. Cause an error by failing authourization on the client. Note the redirect goes to /auth/failure?message=invalid_credentials&strategy=google which is the expected behaviour.

Uncomment browser_id and visit /auth/google. Cause an error by failing authourization on the client. Note the redirect goes to /auth/failure?message=invalid_credentials&strategy=browser_id and forms a redirect loop. Unexpected behaviour.

A faster way to test is to visit /auth/identity and submit a blank form, which will also correctly trigger a failure. This happens with all the strategies listed here, unless browser_id is removed.

Other than that, it works great :)

skorfmann commented 11 years ago

This is still a thing, just encountered the same issue

glsignal commented 11 years ago

I'm encountering this one as well, has anyone found a workaround for it at all?

skorfmann commented 11 years ago

No, haven't looked further into it yet. But I will probably do it the next couple of days, as we're actually going to implement it.

glsignal commented 11 years ago

Ah okay. Well from my (tired, quick) glance at the strategy, I'm not entirely certain why the other_phase method is being hit (or what it actually does), any ideas?

glsignal commented 11 years ago

At any rate, there's a hacky workaround by specifying a failure route which doesn't match "#{path_prefix}/failure" (lib/omniauth/strategies/browser_id.rb:24) in the browser_id provider configuration

#!config/initializers/omniauth.rb
Rails.application.config.middleware.use OmniAuth::Builder do
    ...
    provider :browser_id, :failure_path => '/failure'
    ...
end

and matching both /auth/failure and /failure to some controller action.

I feel bad for coming up with such a thing, quite frankly, but desperate times...

skorfmann commented 11 years ago

Well, better than nothing :) Thanks for sharing!

glsignal commented 11 years ago

No worries! Keen to see a solution to this eventually, might look into it myself if I come across any free time.