omniauth-multi-provider icon indicating copy to clipboard operation
omniauth-multi-provider copied to clipboard

/auth/failure never reached due to warden throw

Open sbauch opened this issue 5 years ago • 2 comments

My understanding of omniauth is that, when a strategy failure occurs, strategy.fail! should be called such that the user is redirected to /auth/failure.

I've looked through a handful of strategies, and haven't seen a throw :warden like we see here -

https://github.com/salsify/omniauth-multi-provider/blob/master/lib/omni_auth/multi_provider/handler.rb#L67

Should throwing a warden error be the responsibility of the consumer of this lib?

I can't find a way to rescue this error (I am not using warden, nor devise) and as such haven't found a reasonable way to get the user to /auth/failure when there's an error in the identity_provider_options_generator block.

The only thing I can really do is something like this, which results in invalid ticket errors, which isn't really what's going on here:

do |identity_provider_id, env|
  
  Models::IdentityProvider.find(identity_provider_id).
    saml_options

  rescue ActiveRecord::RecordNotFound
    {}
end

If my block throws an error, the user should be redirected to /auth/failure per OmniAuth. Instead, i get an UncaughtThrowError, which in prod is not going be something i can really rescue to display a useful message.

sbauch avatar Sep 19 '20 14:09 sbauch

Hmm. That does sound like a problem that we've let an implicit warden dependency sneak into this gem. Ideally this gem would work with plain omniauth or omniauth + warden + devise. Unfortunately I don't have the bandwidth to investigate this right not but PRs are always welcome!

jturkel avatar Oct 05 '20 21:10 jturkel

I'm happy to take a stab, especially if it comes with a FREE TEE SHIRT 👻 😱 🎃

any thoughts on how you would like to approach backwards compatibility? I'm not terribly familiar with warden, it might be nice to be able to still throw the warden error if warden is used? kinda seems like env['warden'] might tell us if this strategy is being used with warden?

sbauch avatar Oct 05 '20 23:10 sbauch