Use mapping.fullpath in omniauth callbacks #2227

Merged
merged 1 commit into from Nov 6, 2013

Projects

None yet

8 participants

@AlexanderZaytsev
Contributor

We need to use fullpath to make scoping work for omniauth callbacks.

Previously, the scope would not be applied to omniauth callbacks:

scope '/api' do
  devise_for :users, controllers: { omniauth_callbacks: 'users/omniauth_callbacks' }
end
...
/api/users/sign_out(.:format)          devise/sessions#destroy
/users/auth/:provider(.:format)        users/omniauth_callbacks#passthru {:provider=>/(?!)/}
...

With this patch, the routes look as follows:

...
/api/users/sign_out(.:format)          devise/sessions#destroy
/api/users/auth/:provider(.:format)        users/omniauth_callbacks#passthru {:provider=>/(?!)/}
...
@josevalim
Member

So, there is a reason to use path instead of full_path, as seen in this commit:

d8f33b8

Basically, the full path can contain :locale and other optional segments which won't work with omniauth, since it operates at the rack level. So we could either check if fullpath contains an optional segment and raise an exception or continue using path and asking developers to set Devise.omniauth_path_prefix instead.

@AlexanderZaytsev
Contributor

@josevalim I'm in favor of the first solution. What about you? Setting Devise.omniauth_path_prefix separately seems like a hack to me.

@josevalim
Member

@AlexanderZaytsev yeah, I also like 1) but it would have to wait until Devise 2.3 since it is backwards incompatible. Could you please send a pull request?

@AlexanderZaytsev
Contributor

I updated the pull request with the first solution.

@prajwalkman

Waiting for this... I was deploying my application to a suburi on Passenger, but repeatedly passenger simply stopped routing to the suburi, and requests went to the base application instead. I gave up and added all my routes to a scope and I noticed this.

@josevalim josevalim merged commit a7624c8 into plataformatec:master Nov 6, 2013

1 check passed

default The Travis build passed
Details
@moustafasallam

hello, this merge you did 17 hours ago caused my app to fail with the error message of "[DEVISE] Nesting omniauth callbacks under scopes with dynamic segments "
"is not supported. Please, use Devise.omniauth_path_prefix instead"
the problem is as the following I have
(:locale) scope
inside it I have devise_for

it was working. now it doesn't

and if I put it outside the scope I got no :locale
and if I set the Devise.omniauth_path_prefix in devise.rb it only affect the omniauth routes not all the devise routes.
And if I put :path_prefix to the devise_for I got the same error.

@josevalim
Member

Hello @moustafasallam, this means you will have to skip the omniauth routes and define them directly (it is a trade-off we have made by merging it). I am actually considering removing those routes altogether in favor of people manually defining them, so you would even be ahead of them. Here is how you can do it:

scope "/your/scope/:locale" do
  devise_for :users, skip: :omniauth_callbacks
end

# TODO: I don't think this route is required on latest OmniAuth
match "/users/auth/:provider",
  constraints: { provider: /\Agoogle|facebook/etc\z/ },
  to: "devise/omniauth_callbacks#passthru",
  as: :user_omniauth_authorize

match "/users/auth/:action/callback",
  constraints: { action: /\Agoogle|facebook/etc\z/ },
  to: "devise/omniauth_callbacks,
  as: :user_omniauth_callback

I will also improve the error message. Please let me know if it works!

@moustafasallam

Hello @josevalim, First Thank you for the prompt reply. it works now nice. I have to adjust the 'match' as I use rails 4
Here is the adjusted code in case anyone needs it

match "(:locale)/users/auth/:provider", constraints: { provider: /facebook/ },to: "users/omniauth_callbacks#passthru", as: :user_omniauth_authorize, via: [:get, :post]

match "(:locale)/users/auth/:action/callback", constraints: { action: /facebook/ }, to: "users/omniauth_callbacks", as: :user_omniauth_callback, via: [:get, :post]

I am new to Rails. Anyway I don't know why when I used "to: "#{controllers[:omniauth_callbacks]}#passthru"," it gives me an error, telling me that controllers is undefined.

Thank you again for the help

@moustafasallam

Hello @josevalim , I had error when I tried to sign in via facebook now, when I use you manual build to the omniauth as you described above the "#{controllers[:omniauth_callbacks]}#passthru", produces an error saying that the controllers is undefined, and when I replace controllers[:omniauth_callbacks] with string "omniauth_callbacks. the routes gets built correctly with no error. However when I try to sign in with facebook I got the following error:
Could not find devise mapping for path "/en/users/auth/facebook". This may happen for two reasons: 1) You forgot to wrap your route inside the scope block. For example: devise_scope :user do get "/some/route" => "some_devise_controller" end 2) You are testing a Devise controller bypassing the router. If so, you can explicitly tell Devise which mapping to use: @request.env["devise.mapping"] = Devise.mappings[:user]
could you please help me out here in this issue, what is the correct set up for the omniauth routes for only facebook.

@josevalim
Member

Sorry, it was a mistake in the code, I have updated it.

@moustafasallam

Hello again @josevalim I still get the same error:

Here is my match:

match "/users/auth/:provider",
constraints: { provider: /facebook/ },
to: "devise/omniauth_callbacks#passthru",
as: :user_omniauth_authorize,
via: [:get, :post]

match "/users/auth/:action/callback",
constraints: { action: /facebook/ },
to: "devise/omniauth_callbacks",
as: :user_omniauth_callback, via: [:get, :post]

@kashif
kashif commented Dec 18, 2013

@josevalim I also get the same error as @moustafasallam so I changed my routes to:

scope '(:locale)', locale: /#{I18n.available_locales.join("|")}/ do
    devise_for :users, skip: :omniauth_callbacks, controllers: {
      registrations:      'registrations',
      sessions:           'sessions'
    }

    devise_scope :user do
      match "/users/auth/:provider",
        constraints: { :provider => /facebook/ },
        to: "users/omniauth_callbacks#passthru",
        as: :user_omniauth_authorize,
        via: [:get, :post]

      match "/users/auth/:action/callback",
        constraints: { :action => /facebook/ },
        to: "users/omniauth_callbacks",
        as: :user_omniauth_callback,
        via: [:get, :post]
    end
end

However now request.env["omniauth.auth"] is nil in the Users::OmniauthCallbacksController... any idea? thanks!

@lucianosousa
Contributor

@kashif same here, did you find a solution?

@kashif
kashif commented Dec 31, 2013

@lucianosousa no not yet, but i have created an issue #2807

@moustafasallam

Hello, I didn't find the perfect solution, however I manage overcome this problem by defining the omniauth_callbacks outside the :locale scope, means the following:

scope "(:locale)" do
devise_for :users, :skip => :omniauth_callbacks
end

devise_for :users, :skip => [:passwords, :registrations, :sessions, :confirmations], :controllers => { :omniauth_callbacks => "users/omniauth_callbacks" }

This works for me I don't know if it will work for you as well, but anyway this is what I did.

@ncri
ncri commented Mar 7, 2014

I don't get this working as described. I get

Devise, OmniAuth & Facebook: “Not found. Authentication passthru.”

So I monkey patched the old behavior.

@rafaltrojanowski

@moustafasallam thanks! you can replace :skip with :only like this:
devise_for :users, only: [:omniauth_callbacks], :controllers => { :omniauth_callbacks => "users/omniauth_callbacks" }

@ncri
ncri commented Mar 12, 2014

Oh, @funkydrummer, that also works for me! I just wanted to avoid that clumsy syntax with :skip, so thanks for the tip with :only.

@moustafasallam

@funkydrummer You are welcome! and thank you for the input

@Envek Envek added a commit to Envek/devise that referenced this pull request Jun 24, 2015
@Envek Envek Provided another solution in error message when using omniauth callba…
…cks under a dynamic segment

Previous solution was too complex and wasn't worked for everyone. See discussion at plataformatec#2227

Fixes #3651 [ci skip]
4d8bec4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment