heartcombo / devise

Flexible authentication solution for Rails with Warden.
http://blog.plataformatec.com.br/tag/devise/
MIT License
24.03k stars 5.55k forks source link

undefined method `new_session_path` for omniauth_callbacks controller #1390

Closed riffraff closed 10 years ago

riffraff commented 13 years ago

this error appeared in our codebase since an upgrade from 1.3.4 to 1.4.8, and it seems somewhat related to similar errors reported recently. It consistently happens whenever an authorization is denied by the user and we end up on the failure side of the omniauth controller. Our code in callbacks controller is pretty much identical to that in the docs/wiki and worked fine in the past. We also see the error when hitting the recovery password page, similarly to an already reported and closed issue.

In our case the route code looks like

devise_for :users , :controllers => { 
  :omniauth_callbacks => "omniauth_callbacks" ,
  :registrations => 'registrations',
  :sessions =>'sessions'
}

and the quick fix was to simply add

def new_session_path *args 
  new_user_session_path *args
end

to our ApplicationController

kevinrutherford commented 13 years ago

I also have this error in my app since an upgrade to 1.4.8. It occurs whenever a user requests a password reset email or a re-send of confirmation instructions. We're using rails 3.0.3.

jdx commented 13 years ago

+1 received this error as well going from 1.4.7 to 1.4.8

krisleech commented 13 years ago

I can confirm that downgrading to 1.4.7 fixes this issue.

Rails 3.0.3, basic Devise setup

josevalim commented 13 years ago

This should be fixed in master and v1.4 branches. Please confirm and I will gladly release 1.4.9. Thanks!

riffraff commented 13 years ago

yep seems to work for me, fixing the error both in password recovery and omniauth failure.

krisleech commented 13 years ago

Thanks, I can also confirm HEAD fixes this for Rails 3.0.3 / JRuby.

patbenatar commented 12 years ago

This issue seems to have regressed in 1.5.3 with omniauth 1.0.1

rebo commented 12 years ago

Yes I am getting this issue again

krzkrzkrz commented 12 years ago

Same, needs to be reopened

josevalim commented 12 years ago

Can someone provide a way to reproduce the issue? Otherwise, there is no way we can fix it.

josevalim commented 12 years ago

I am closing this again for the same reason: lack of ways to reproduce the issue. Thanks.

jeperkins4 commented 12 years ago

I came across this issue in an app that I am migrating from ruby 1.8.7/rails 3.0.7 to 1.9.3/3.2.8. Make sure the following is not in your config/application.rb:

config.middleware.insert_before(Warden::Manager, Rack::OpenID)

myitcv commented 11 years ago

I encountered this whilst using Devise as confirmable only. It occurs for me when a user tries to confirm an email address using a token that has expired. Clearly it's necessary to define a route for this eventuality - so just to confirm, is defining a route like:

match "a/b", :via => [:get, :put], :as => "new_session"

the 'correct' way to define the route?

twelve17 commented 11 years ago

I ran into this today, but it was a narrow use case:

I'm writing an engine that uses devise version 2.2.4 plus omniauth-oauth2, and includes a custom subclass of Devise::OmniauthCallbacksController. I am only using :omniauthable in my User model.

The error only happens in the test/dummy app, which itself extends the engine's OmniauthCallbacksController (to implement a stub method), but not in a 'real' app that is using the gem in the same fashion. I'm in the process of trying to figure out if there is a notable difference in between this app and the test dummy app's configurations.

I'm running an integration test in the engine with the dummy app. I put a breakpoint in Devise.generate_helpers!, and see the new_session_path being generated, but for whatever reason, it's not available in the custom OmniauthCallbacksController subclass in the dummy app. class.ancestors in that controller does show the Devise::Controllers::UrlHelpers being included, so I must be missing something. I'll keep digging.

RKushnir commented 11 years ago

In the docs it says

If you are using ONLY omniauth authentication, you need to define a route named new_user_session (if not defined, root will be used)

but in fact it raises the aforementioned error and doesn't use the root. I'm on a brand new Rails4 app with gems

devise (3.1.0)
omniauth (1.1.4)
omniauth-linkedin-oauth2 (0.1.1)
omniauth-oauth2 (1.1.1)
printercu commented 10 years ago

@josevalim, got same issue on 3.2.4. Faced this problem when omniauth provider returns error and https://github.com/plataformatec/devise/blob/master/app/controllers/devise/omniauth_callbacks_controller.rb#L28 is called.

I use only

class User < ActiveRecord::Base
  devise :omniauthable, :rememberable, :trackable
  # ...
end

so seems like new_session_path haven't been generated.

I have routes for new_user_session_path but it doesn't help:

  devise_scope :user do
    get '/session/new(.:format)'        =>  'devise/sessions#new',      as: :new_user_session
    get '/session/sign_out(.:format)'   =>  'devise/sessions#destroy',  as: :destroy_user_session
  end

This fix works for me:

class ApplicationController < ActionController::Base
  # ...
  def new_session_path(scope)
    new_user_session_path
  end
end

Should we reopen issue?

laurocaetano commented 10 years ago

It should work, because the new_session_path is generated by Devise.

You can check the code here https://github.com/plataformatec/devise/blob/b8ed2f31608eccb6df6d5bb2e66238d6dfc4bcfc/lib/devise/controllers/url_helpers.rb#L3-27 and here https://github.com/plataformatec/devise/blob/b8ed2f31608eccb6df6d5bb2e66238d6dfc4bcfc/lib/devise/controllers/url_helpers.rb#L48-51

Could you provide a sample app to reproduce this bug?

josevalim commented 10 years ago

@laurocaetano we just generate those routes depending on the devise functionality you are using:

https://github.com/plataformatec/devise/blob/b8ed2f31608eccb6df6d5bb2e66238d6dfc4bcfc/lib/devise/controllers/url_helpers.rb#L36-L39

I am not sure what is the best way to proceed here. Maybe a respond_to? check with a proper error message? Or maybe leave it as is and say new_session_path or after_omniauth_failure_path_for must be redefined. We just need to agree on something.

printercu commented 10 years ago

Мауbe just create this helper if :omniauthable is used? There is notice in documentation that one should add new_user_session route manually if uses :omniauthable without :database_authenticatable. So the route should be present, we only need a helper.

josevalim commented 10 years ago

@printercu yeah, that is likely a better option!

laurocaetano commented 10 years ago

I got something like https://gist.github.com/laurocaetano/55cf2edd5c08881030c6, but I'm not familiar with this part of the code and maybe I'm missing something.

josevalim commented 10 years ago

@laurocaetano that seems to be the way to go. @printercu could you please give it a try?

printercu commented 10 years ago

I can't test its behaviour with omniauth now, but i've got this in console:

2.1.1 :001 > Sessions::OmniauthCallbacksController.instance_method(:new_session_path)
NameError: undefined method `new_session_path' for class `Sessions::OmniauthCallbacksController'
    from (irb):1:in `instance_method'
    from (irb):1
    from /Users/max/.rvm/gems/ruby-2.1.1/gems/railties-4.1.0/lib/rails/commands/console.rb:90:in `start'
    from /Users/max/.rvm/gems/ruby-2.1.1/gems/railties-4.1.0/lib/rails/commands/console.rb:9:in `start'
    from /Users/max/.rvm/gems/ruby-2.1.1/gems/railties-4.1.0/lib/rails/commands/commands_tasks.rb:69:in `console'
    from /Users/max/.rvm/gems/ruby-2.1.1/gems/railties-4.1.0/lib/rails/commands/commands_tasks.rb:40:in `run_command!'
    from /Users/max/.rvm/gems/ruby-2.1.1/gems/railties-4.1.0/lib/rails/commands.rb:17:in `<top (required)>'
    from bin/rails:4:in `require'
    from bin/rails:4:in `<main>'
2.1.1 :002 > Sessions::OmniauthCallbacksController.instance_method(:omniauth_callback_path).source_location
 => ["/Users/max/.rvm/gems/ruby-2.1.1/gems/devise-3.2.4/lib/devise/omniauth/url_helpers.rb", 12] 

I tried this, it works in console, but i don't know how) Is it expected behaviour? I'll try it later on server.

# d.add_module :omniauthable, controller: :omniauth_callbacks,  route: {session: [:new]}

2.1.1 :001 > Sessions::OmniauthCallbacksController.instance_method(:new_session_path).source_location
 => ["/Users/max/.rvm/gems/ruby-2.1.1/gems/devise-3.2.4/lib/devise/controllers/url_helpers.rb", 48] 
2.1.1 :002 > Sessions::OmniauthCallbacksController.instance_method(:omniauth_callback_path).source_location
 => ["/Users/max/.rvm/gems/ruby-2.1.1/gems/devise-3.2.4/lib/devise/omniauth/url_helpers.rb", 12] 

Update. I've got error undefined method 'user_omniauth_authorize_path'.

laurocaetano commented 10 years ago

The solution I mentioned before won't work because add_module will only get the Hash's first key. https://github.com/plataformatec/devise/blob/master/lib/devise.rb#L383

@josevalim should we change the add_module method to accept more than one key?

josevalim commented 10 years ago

Definitely!

laurocaetano commented 10 years ago

Well, it also won't work :cry:, because it will only have access to the routes defined by the module -> https://github.com/plataformatec/devise/blob/master/lib/devise.rb#L392

Discussing with @ulissesalmeida we came up with some solutions:

  1. Just let this way and add a documentation to warn users that new_session_path(scope) must be defined.
  2. Add the documentation and raise an error saying that this helper must be defined.
  3. Rewrite how the routes and helpers work, so that it can accept the helpers defined in other modules.
  4. Make :omniauthable work without any external dependency, creating it's own view, action for failures and etc.

What you guys think about it?

cc @lucasmazza @carlosantoniodasilva

lucasmazza commented 10 years ago

Giving a quick look on this I would go with options 2 or/then 3. :smile:

printercu commented 10 years ago

Vote for 1. There is also documentation for it https://github.com/plataformatec/devise/wiki/OmniAuth%3A-Overview#using-omniauth-without-other-authentications . Just add few lines there.

laurocaetano commented 10 years ago

@printercu I agree.

Just updated https://github.com/plataformatec/devise/wiki/OmniAuth%3A-Overview#using-omniauth-without-other-authentications. Could you take a look?

Thanks.

printercu commented 10 years ago

:+1: Thank you!

ulissesalmeida commented 10 years ago

:+1:

laurocaetano commented 10 years ago

Thanks guys, I'm closing this for now.

:heart: