Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authenticating an existing Warden/Devise User #200

Merged
merged 1 commit into from
Mar 31, 2015

Conversation

nickL
Copy link
Contributor

@nickL nickL commented Mar 30, 2015

Yo!

Alright, so I think this will help with #120, #168, & #196; In the event an app uses devise for normal session authentication and would like to also authenticate users via DeviseTokenAuth then I think this will help support both strategies and ease the transition. Small bit of interstitial code to check for the user in warden and, if they're missing authentication details, create a token for them and pass them through. Otherwise, the normal token auth strategy kicks in.

I had a couple of questions that are worth mentioning before merging this in, if it feels like its on the right approach:

  1. I added a test in DemoUserController integration test. Does that feel like a good place for it?
  2. I unfortunately can't use this commit in our own project because we're still on Rails 4.1, lol. Still wanted to contribute! We definitely have plans to upgrade soon, but is there anyway to support this in any earlier version? I suspect no, which is perfectly fine! Just more gas on the fire to get us to upgrade. I was just curious if there was a way to allow any Rails 4.1 apps to use it.
  3. Anything I missed? Let me know. I'll do the work. You're busy enough.

@lynndylanhurley
Copy link
Owner

Fantastic PR @nickL. You just made my day with these tests.

I added a test in DemoUserController integration test. Does that feel like a good place for it?

DemoUserController is the best place - that's where I'm currently doing all of the integration tests.

is there anyway to support this in any earlier version?

I have been getting asked this a lot. I don't remember exactly what the incompatibility is, but I do remember that I had to change some things for Rails 4.2. I'll downgrade to Rails 4.1 and see if the tests still pass.

Is there a way to have the test suite run against multiple versions of Rails?

lynndylanhurley added a commit that referenced this pull request Mar 31, 2015
Authenticating an existing Warden/Devise User
@lynndylanhurley lynndylanhurley merged commit 783fbcd into lynndylanhurley:master Mar 31, 2015
@casertap
Copy link

Thanks nickL for your work.
I need to use this code to be able to login with devise_token_auth and still have some old erb portal accessible for logged admin users.

I encounter two problems with the solution:

  1. the conditional in set_user_by_token
if devise_warden_user && devise_warden_user.tokens[@client_id].nil?

if never true because I am logged in with devise_token_auth and so my token is set.
I think, you should remove the second part of the conditional and check if this does not introduce some security vulnerability.

  1. when I visit my old pages, this code in in set_user_by_token
@token     = request.headers['access-token'] || params['access-token']

@token will be nil but it is still included in the header (as access-token)
This make crash rake/lib/rack/handler/webrick.rb:100

headers.each { |k, vs|
            next if k.downcase == "rack.hijack"
            if k.downcase == "set-cookie"
              res.cookies.concat vs.split("\n")
            else
              # Since WEBrick won't accept repeated headers,
              # merge the values per RFC 1945 section 4.2.
              res[k] = vs.split("\n").join(", ")
            end
          }

because he is trying the do nil.split("\n").join(", ") because access-token is nil.
I think, you should remove the access-token header if this one is nil.

Let me know. Thanks a lot.

@casertap
Copy link

Actually there are many more problems:
3)
If I log with a user 1 and then logout and log in with another user 2.
because the logout of warden was not called you have the user 2 that can connect using its password and access directly the account of the user 1.
I think, you should add a call to:

sign_out user

in the destroy method of the session controller. (Just like you called the sign_in method in the create method)

If you have an user with a tokens empty.
If this option is setup (that is my case in development mode)

DeviseTokenAuth.setup do |config|
  config.change_headers_on_each_request = true
end

than you will get this stack

NoMethodError - undefined method `[]' for nil:NilClass:
   () Users/pierrecaserta/.rvm/gems/ruby-2.2.1/bundler/gems/devise_token_auth-a561a9b74bd8/app/models/devise_token_auth/concerns/user.rb:169:in `build_auth_header'
   () Users/pierrecaserta/.rvm/gems/ruby-2.2.1/bundler/gems/devise_token_auth-a561a9b74bd8/app/controllers/devise_token_auth/concerns/set_user_by_token.rb:73:in `block in update_auth_header'
  activerecord (4.2.1) lib/active_record/locking/pessimistic.rb:72:in `block in with_lock'
  activerecord (4.2.1) lib/active_record/connection_adapters/abstract/database_statements.rb:213:in `block in transaction'
  activerecord (4.2.1) lib/active_record/connection_adapters/abstract/transaction.rb:188:in `within_new_transaction'
  activerecord (4.2.1) lib/active_record/connection_adapters/abstract/database_statements.rb:213:in `transaction'
  activerecord (4.2.1) lib/active_record/transactions.rb:220:in `transaction'
  activerecord (4.2.1) lib/active_record/transactions.rb:277:in `transaction'
  activerecord (4.2.1) lib/active_record/locking/pessimistic.rb:70:in `with_lock'
   () Users/pierrecaserta/.rvm/gems/ruby-2.2.1/bundler/gems/devise_token_auth-a561a9b74bd8/app/controllers/devise_token_auth/concerns/set_user_by_token.rb:64:in `update_auth_header'

the code responsible for this error is :

 def update_auth_header
    # cannot save object if model has invalid params
    return unless @resource and @resource.valid? and @client_id
    # Lock the user record during any auth_header updates to ensure
    # we don't have write contention from multiple threads
    @resource.with_lock do
      # determine batch request status after request processing, in case
      # another processes has updated it during that processing
      @is_batch_request = is_batch_request?(@resource, @client_id)
      auth_header = {}
      if not DeviseTokenAuth.change_headers_on_each_request
        auth_header = @resource.build_auth_header(@token, @client_id)

You are trying to build_auth_header but you are actually still connected with warden so @token is nil

@nickL
Copy link
Contributor Author

nickL commented Jun 16, 2015

Hi @casertap, I'm sorry you're running into such trouble! I'm not familiar with the error you describe re: warden/devise_token_auth and I'm having trouble reproducing. Though I'll do my best to help if I can. To get me started can you do either of the following to help me reproduce?

  1. Write a simple test-unit/rspec test that demonstrates the above error?
  2. Link me to a fork/dummy app with steps to reproduce
  3. Give a bit more detail (step by step) of how you're logging in and logging out your two different users. I'm a bit unclear how to reproduce this is a blank app (i just tried with no luck)

1 or 2 would be preferred, it'll help me try and reproduce this in the way you described. Happy to help if I can.

Thanks!

@booleanbetrayal
Copy link
Collaborator

there's a few PRs that have just gone in, so any work you might make against this issue, be sure to do so on a fresh pull of master. for instance, reset_session is now being called on session destroy.

@nbrustein
Copy link
Contributor

@casertap @nickL I'm running to what sounds like kind of a similar issue to @casertap, but I don't understand how.

We set the storage config value to 'localStorage'. We also have rails sessions turned on, only because we need them turned on to support omniauth. Unlike @casertap, the only way to login to our site is with devise token auth.

Yet, once every few days, we run into a state where a request comes in with no tokens, and the user is still found by: warden.user(rc.to_s.underscore.to_sym) As far as I understand, this means that the user does not have a token stored in localStorage, but they do have a session id stored in a cookie.

It's a long story about how we noticed this. I don't think it's causing us any direct problems, but it makes me nervous that I don't really understand what "logged in" really means on my site right now.

I can reproduce this issue by logging in and then deleting my local storage in the Chrome dev tools. This makes sense because, in that case, I still have a session_id stored in a cookie, so the next request sends up a session_id but no token. However, I do not believe that my real uses are out there deleting local storage values but not deleting cookies.

Before #251, this would happen after an explicit logout, since the logout removed the token from the auth headers but did not remove the session_id from the cookie, but #251 solved that.

So now I don't know how it's happening. Do you have any other explanation of how someone could get into this state where they send up no token but still get logged in via warden?

@booleanbetrayal
Copy link
Collaborator

@nickL / @casertap - Due to the issues resulting from this warden implementation, I'm inclined to make this warden lookup configurable under some sort of support_legacy_authentication flag of some sort, unless you can indicate why we would want to support this under all use cases. Please provide any insight you might have!

@ianchen06
Copy link

@nbrustein @booleanbetrayal - I'm running into the same issue as @casertap ,

3)
If I log with a user 1 and then logout and log in with another user 2.
because the logout of warden was not called you have the user 2 that can connect using its password and access directly the account of the user 1.

Please advise if there are workarounds or patches for this issue. We need the sessions enabled for omniauth login.

Thanks a lot for your help and a great gem.

@nickL
Copy link
Contributor Author

nickL commented Sep 1, 2015

@booleanbetrayal , @casertap : Sorry!! I missed the notifications on this thread and just saw your replies. My mistake for not replying sooner.

@booleanbetrayal : I like the idea of making this configurable I don't think it's a use-case for everyone as soome implementations may not use warden this way. I'd support a configurable toggle if that would be sufficient. I don't mind helping with this if you want to talk through it.

@lynndylanhurley
Copy link
Owner

@nickL - this commit seems to be causing problems with logout (see #375). I just pushed a change to address it (see c80cfce). It didn't break any tests, but can you please pull down version 0.1.37-beta1 and see if everything still works ok with your setup?

@booleanbetrayal
Copy link
Collaborator

@lynndylanhurley - we're seeing the sign_out(@resource) call clear out the user record prior to certain flows in our controller logic, such as during CanCan validations. Should we be deferring this somehow, or clearing the user in a way that only effects the session store for Devise?

@lynndylanhurley
Copy link
Owner

@booleanbetrayal - That's what I was afraid of.

This line seems to be the real problem:

devise_warden_user =  warden.user(rc.to_s.underscore.to_sym)

I think it was intended to pull the data from an existing devise session (standard devise, not devise_token_auth), but it seems to be also instructing the browser to set a cookie containing the user's devise session info.

This is problematic because the API shouldn't deal with cookies at all (that is the purpose of token based authentication). But I understand why this functionality is useful.

So the question is, can we accomplish what this line of code is doing (identifying the user from the standard devise session) without actually setting the cookie?

@booleanbetrayal
Copy link
Collaborator

@lynndylanhurley - i dunno ... i was toying with ripping that support out entirely or at the very least make it opt-in. here's how @nbrustein and I are dealing with it today. it's a hacky hodge-podge of reset_session calls:

application_controller.rb

    rescue_from NoMethodError, with: :catch_devise_error
    def capture_devise_no_method_error(exception)
        if exception.message == "undefined method `[]' for nil:NilClass" && 
            exception.backtrace[0].match(/build_auth_header/)
            # log the exception, etc etc ...
            reset_session
            return true
        else
            return false
        end
    end

home_controller.rb

before_action :reset_session # NOTE: this is a little misleading. the session is only used briefly and only during omniauth (?).

devise_token_auth.rb (monkey-patch)

require 'devise_token_auth/sessions_controller'
module DeviseTokenAuth
    class SessionsController < DeviseTokenAuth::ApplicationController

        after_action :reset_session, :only => [:destroy]

    end
end

@lynndylanhurley
Copy link
Owner

This would be easier if we had a failing test that we could solve against. I'll see what I can come up with tonight.

lynndylanhurley added a commit that referenced this pull request Oct 27, 2015
@lynndylanhurley
Copy link
Owner

@booleanbetrayal - can you please try version 0.1.37.beta3? I think I was able to get around the issue without calling sign_out.

@booleanbetrayal
Copy link
Collaborator

@lynndylanhurley - I'm still experiencing the same issue as when you were explicitly calling sign_out.

Can is wired up via a before_filter and by the time I'm seeing it register enter play, current_user is already nil

https://github.com/ryanb/cancan/blob/master/lib/cancan/controller_additions.rb#L357

 "/Users/brent/.rvm/gems/ruby-2.2.2@back_royal/gems/cancan-1.6.10/lib/cancan/controller_additions.rb:357:in `new'",
 "/Users/brent/.rvm/gems/ruby-2.2.2@back_royal/gems/cancan-1.6.10/lib/cancan/controller_additions.rb:357:in `current_ability'",
 "/Users/brent/.rvm/gems/ruby-2.2.2@back_royal/gems/cancan-1.6.10/lib/cancan/controller_additions.rb:338:in `authorize!'",
 "/Users/brent/.rvm/gems/ruby-2.2.2@back_royal/gems/cancan-1.6.10/lib/cancan/controller_resource.rb:41:in `authorize_resource'",
 "/Users/brent/.rvm/gems/ruby-2.2.2@back_royal/gems/cancan-1.6.10/lib/cancan/controller_resource.rb:10:in `block in add_before_filter'",
...

@lynndylanhurley
Copy link
Owner

@booleanbetrayal - current_user should be available after the set_user_by_token before_filter is called. Is it possible that the cancan filters are being run before the set_user_by_token filter?

@booleanbetrayal
Copy link
Collaborator

@lynndylanhurley - I'm always seeing set_user_by_token getting called prior to the CanCan initialization. maybe something internal to Devise? I'm wondering if we shouldn't just make this an opt-in setting and punt for now. Think it'd result in improved stability for most users.

@lynndylanhurley
Copy link
Owner

I'm wondering if we shouldn't just make this an opt-in setting and punt for now. Think it'd result in improved stability for most users.

Agreed. I was at this all night last night, and I'm still not sure how to write a test for this 😵

I'm speculating here, but it looks like Devise/warden is getting the browser to set a cookie that correlates to some kind of warden session.

That session is actually destroyed on the server when the sessions#destroy route is hit. But the cookie still persists on the browser, and the presence of that cookie seems to revive the session somehow (may be related to this issue: heartcombo/devise#3031).

The only way I could find around this was to prevent the creation of that cookie. It would be better to have the browser terminate the cookie upon logout. I'll keep looking for a way to do that.

@booleanbetrayal
Copy link
Collaborator

@lynndylanhurley - going to pick this up (configuration flag / new default) unless you've already started it

@lynndylanhurley
Copy link
Owner

@booleanbetrayal - please do.

I'll keep trying to figure out why the session persists after logout, and if there's anything that can be done to prevent it.

@booleanbetrayal
Copy link
Collaborator

@lynndylanhurley - maybe take a quick peek at #428 and let me know if you see anything off

@booleanbetrayal
Copy link
Collaborator

This is now configurable as per #428 - defaulting to disabled

@noahmatisoff
Copy link

I keep getting this error seems like it comes from this PR on this line: devise_warden_user = warden.user(rc.to_s.underscore.to_sym)

I'm getting this:

CasesControllerTest#test_should_get_index:
NoMethodError: undefined method `user' for nil:NilClass
    devise_token_auth (0.1.36) app/controllers/devise_token_auth/concerns/set_user_by_token.rb:35:in `set_user_by_token'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants