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

Token Authenticatable writes to session #17

Open
crododile opened this issue Feb 22, 2016 · 4 comments
Open

Token Authenticatable writes to session #17

crododile opened this issue Feb 22, 2016 · 4 comments

Comments

@crododile
Copy link

The token authenticatable strategy writes auth stuff into the session, this is a problem because logging out by throwing away authentication token does not work if browser does not throw out session ( hybrid, sometimes iOS ).

Token Auth issue could be solved by adding a method

def store?
  false
end

## OR more flexibly and inline with other 
def store?
  super && && !mapping.to.skip_session_storage.include?(:token_auth)
end

## and then adding :token_auth to devise.rb
  config.skip_session_storage = [:http_auth, :token_auth]

## the above method could be included by inheriting from Authenticateable 
## https://github.com/plataformatec/devise/blob/master/lib/devise/strategies/authenticatable.rb
## and adding attribute :authentication_type to token_authenticatable


## Current way to solve this as user of this gem is to pass { store: false } when authenticating.
## ( proposed fix makes this unnecessary )
  def self.authenticated!(options = {store: false})
    before_action :authenticate_user!, options
  end

A separate issue is that we log in with Database Authenticatable and that strategy also writes to session, but we can't tell database authenticatable to not store by default b/c that strategy is used by web app
solved that by simply deleting the auth info from the session, but would be nice to have

class API::User::SessionsController < Devise::SessionsController
  include APIDeviseOptions

  def create
    super
    session.delete 'warden.user.api_user.key'
  end
end
@lserman
Copy link
Contributor

lserman commented Feb 22, 2016

Can you PR with the skip_session_storage? Also does Devise have anything in super classes we can use so we don't have to put !mapping.to.skip_session_storage.include?(:token_auth) ? It seems like that would be re-used by all Devise strategies and it looks pretty low-level. This is something I would expect super to cover, or at least covered in some capacity by Devise.

I would prefer if we use skip_session_storage instead of just false... sometimes we will want to open a webview from a mobile app and then have the user go through a flow without having to send the auth token everytime.

Unclear on second problem... can't you just use store: false there as well?

@crododile
Copy link
Author

inheriting from Devise::Strategies::Authenticatable instead of Devise::Strategies::Base would include

    class Authenticatable < Base
      attr_accessor :authentication_hash, :authentication_type, :password

      def store?
        super && !mapping.to.skip_session_storage.include?(authentication_type)
      end
 ...

authentication_type gets set one step down the stack of "authenticatable#valid?" stack so it looks like we could inherit from it and set :authentication_type in the TokenAuthenticatable#valid? method. Authenticatable is otherwise a bunch of private methods ( tokenAuthenticatable overwrites the public ones ) so it should be ok.

@fbcouch
Copy link

fbcouch commented Feb 22, 2016

What I do in API's is just nullify the session on every request (in my API base controller) like so:

protect_from_forgery with: :null_session
before_action :nullify_session
# ...
def nullify_session
  forgery_protection_strategy.new(self).handle_unverified_request
end

That way you don't have to mess around with the session or devise

@lserman
Copy link
Contributor

lserman commented Feb 22, 2016

@crododile lets inherit from that and just provide an authentication_type method that returns :token_auth. That should work.

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

No branches or pull requests

3 participants