mcordell / grape_devise_token_auth

Grape compatibility for devise_token_auth + devise + rails setup
MIT License
41 stars 29 forks source link

Usage of warden to store sessions #7

Closed ArneZsng closed 8 years ago

ArneZsng commented 8 years ago

I am a little bit confused by the warden logic in this gem. From my understanding, each request in devise_token_auth is authenticated individually by its request headers. As I don't have a deeper understanding of Warden, this might be complete BS, so any help is appreciated:

In my example, I am using a Grape endpoint that needs to be available in a "light" version if the user is not logged in and will return more information if the user is logged in.

Thus, I played around with the request headers and noticed that the user is constantly logged in after the first log in, no matter if I changed the request headers or not. Is this the intended behaviour by devise_token_auth, as the "native" devise_token_auth endpoints behave differently?

I therefore removed the warden logic from this gem to authenticate each request by its header parameters individually. The changes can be found in #6.

Please help me to find out if this breaks stuff (like batch requests for example) and there is a much simpler solution to my problem or if this could actually make sense.

mcordell commented 8 years ago

hey @ArneZsng thank you for the issue and the pr.

I agree something is wrong here, but I don't think removing warden is the solution. warden allows us to interact with signed in users through devise / other warden apps, and it is in devise_token_auth. The goal with that is to not have users sign in again when first signing in to front end JS apps.

Could you elaborate on how you are serving your front end code (ng-token-auth, j-toker, whatever)? Is it being accessed by a point where the user is already signed in to the rails app through devise?

ianchen06 commented 8 years ago

@ArneZsng @mcordell I'm also experiencing a similar issue with devise_token_auth and grape_devise_token_auth regarding the header and session

My issue was that if I first sign in with User1, then sign out of User1. Then sign in with User2, I will still be recognized as signed in as User1. Though the initial return of the header is correct(User2 still got the correct uid after sign in), but next requests authorized by grape_devise_token_auth, the server will return the wrong header(with the uid and token of previous signed in User1)

This issue was also raised at the devise_token_auth repository.

https://github.com/lynndylanhurley/devise_token_auth/issues/375

Thanks

ArneZsng commented 8 years ago

@mcordell Thank you for looking into this. We are using a React.js single page app for the frontend with no specific auth framework. (No j-toker as we are not relying on jQuery) However, my issue is unrelated to the frontend as I notice the issue when using plain curl requests.

The problem I am having with the current implementation is the same as @ianchen06 described: I sign in User1, call validate token on User1, sign out User1, sign in User2, call validate token on User2, sign out User2. Everything is fine.

However, when I call a grape endpoint authenticated by grape_devise_token_auth in between, User1 stays logged in, for example: I sign in User1, call an authenticated grape endpoint on User1, sign out User1, sign in User2, call an authenticated grape endpoint on User2. Not only does it still consider User1 as still logged in as the Warden session is still active, it even fails with a NoMethodError: NoMethodError (undefined method[]=' for nil:NilClass)`

This error is caused because the new client header cannot be found in the tokens for User1 who is still considered logged in.

As per my understanding, devise_token_auth is using Warden only to check if an active user is already logged in (via devise for example) but does not set a user who authenticated via the API in Warden. lynndylanhurley/devise_token_auth#200

Thus my proposal to remove Warden entirely. However, if Warden is relied on in other parts of the application, we should try to find a way for the user to only exist within a request and to end the Warden session after each request.

Can you clarify what you mean by "The goal with that is to not have users sign in again when first signing in to front end JS apps."? Do you refer to a setup where you can login with both, devise and devise_token_auth?

mcordell commented 8 years ago

@ArneZsng and @ianchen06 if you do what is proposed in that thread and pin devise_token_auth to 1.3.1, does that resolve your problem?

I'm having trouble reproducing the problem in the test app with:

Using devise_token_auth 0.1.34
Using grape_devise_token_auth 0.1.1
Using devise 3.5.1
mcordell commented 8 years ago

As per my understanding, devise_token_auth is using Warden only to check if an active user is already logged in (via devise for example) but does not set a user who authenticated via the API in Warden. lynndylanhurley/devise_token_auth#200

I do not believe that is the case, at least not in the current code. Yes, it checks for the devise user, here. But then if the token is valid, it signs the user into devise/warden here. Which is using this devise method. Whats confusing is the arguments, bypass: true will clobber the store: false because those options are not passed along in the devise method to warden. This may be the source of the problem linked in the devise_token_auth repo, I just don't have time to investigate it right now.

All that is to say, I think the intent is not to store the resource in the session. So, I changed the code here . @ArneZsng and @ianchen06 can you try that branch (warden-spike) and see if it resolves your problem?

@ArneZsng if the above does not resolve can you paste into this thread:

Thank you both for the help, I appreciate it especially since I am having trouble reproducing this.

justinsoong commented 8 years ago

Having the same issue with the Ionic App,

  1. /auth/sign_out doesn't work Sign Out doesn't work, keeps on 404ing with all the uid/etc passed, seems to work in Postman but not the app
  2. Session Store being used I had to manually wipe the session key on each response, and change the session key, but still observing this issue, my next try is to wipe the session storage completely even on sign_out 404
justinsoong commented 8 years ago

my work around is to delete the cookie session key on response back, everywhere in Rails and in Grape, not ideal, but yea..

justinsoong commented 8 years ago

@mcordell :+1 on your spike

mcordell commented 8 years ago

@justinsoong so the warden-spike branch resolved your problem?

dhosterman commented 8 years ago

@mcordell I'm having the exact same issue as described above. I've tried the warden-spike branch and I'm running into an issue where authenticate_user! is giving me an "ArgumentError Exception: wrong number of arguments (3 for 1..2)". Any thoughts?

dhosterman commented 8 years ago

I've resolved my issue for the time being by adding a before action of warden.session_serializer.delete('user').

mcordell commented 8 years ago

@dhosterman I fixed the warden-spike branch, could you try it out (without your above fix). It should resolve the 3 for 1..2 error and hopefully the larger issue at hand. I rebased the branch so you will likely need an update with bundler.

If someone will confirm that the issue is fixed I'll bump the version and merge it into master.

dhosterman commented 8 years ago

@mcordell Thanks for looking into this and the update! I can confirm that the 3 for 1..2 error has been resolved, but I'm still getting the same behavior with regards to users.

I've confirmed I received the changes and ran a bundle update on the gem to ensure the application was using the newest code.

Sorry!

mcordell commented 8 years ago

No problem, new tact.

@dhosterman I've added a new commit to warden-spike to disable the warden existing user checking.

Bundle update again from that branch.

And then where you have your GrapeDeviseToken.setup! line (probably in an initializer?) change it to do the following:

GrapeDeviseTokenAuth.setup! do |config|
  config.ignore_existing_warden_users = true
end

Let me know if that fixes it, thanks!

ginkel commented 8 years ago

@mcordell I can confirm that the current warden-spike branch fixes the problem @ArneZsng originally reported. Thanks for your help!

ArneZsng commented 8 years ago

@mcordell Thank you for the fix.

mcordell commented 8 years ago

Bumped to 0.1.3 and released. Thanks all

ArneZsng commented 8 years ago

I think I have to reopen this issue (or create a new one regarding this problem).

After calling devise_token_auth's validate_token, an error is thrown when I call any grape endpoint with validation due to valid? being undefined for Hash as warden.user(:user) is a hash representation of the user object.

NoMethodError (undefined method valid?' for #<Hash:0x6067f806>): /Users/arne/.rvm/gems/jruby-9.0.0.0/bundler/gems/grape_devise_token_auth-ee687ca86a03/lib/grape_devise_token_auth/auth_headers.rb:14:inheaders' /Users/arne/.rvm/gems/jruby-9.0.0.0/bundler/gems/grape_devise_token_auth-ee687ca86a03/lib/grape_devise_token_auth/middleware.rb:51:in responses_with_auth_headers' /Users/arne/.rvm/gems/jruby-9.0.0.0/bundler/gems/grape_devise_token_auth-ee687ca86a03/lib/grape_devise_token_auth/middleware.rb:14:incall' /Users/arne/.rvm/gems/jruby-9.0.0.0/bundler/gems/grape-b74a066dcb2a/lib/grape/middleware/auth/base.rb:37:in _call' /Users/arne/.rvm/gems/jruby-9.0.0.0/bundler/gems/grape-b74a066dcb2a/lib/grape/middleware/auth/base.rb:19:incall'

My guess is that though using the spike with storing in warden being turned off, a user is stored in Warden anyway by devise_token_auth's validate_token call.