Rewrite simple_token_authentication to a Devise strategy

[adamniedzielski]

WIP on #57

[jbender]

After delving into the Devise source code itself, I ended up trying to take this into a slightly different direction to make it more "devise-like." While I haven't gotten it working fully yet, I can push what I have so far if you'd like to take a look.

[jbender]

See changes in 1329f30

[gonzalo-bulnes]

@adamniedzielski If necessary, could you please cherry-pick the @jbender commit(s) in order to stay in sync?

[gonzalo-bulnes]

IMO, making the strategy as devise-like as possible is full of sense.

Since the change will be disruptive, I opened a v2.0.0 milestone for this feature. Once that done, while care should be taken to ensure the features of the v1.x series are provided by the strategy, there is no need to care about changing the way the gem must be installed or used and Devise-likeliness is very welcome.

[adamniedzielski]

OK, so these are all the commits to make my application work after the rewrite.

The changes which were necessary on my app's side:

 class User < ActiveRecord::Base
-  acts_as_token_authenticatable

   devise :database_authenticatable, :registerable,
-         :recoverable, :rememberable, :trackable, :validatable
+         :recoverable, :rememberable, :trackable, :validatable,
+         :simple_token_authenticatable

 class UsersController < ApplicationController

-  acts_as_token_authentication_handler_for User
+  before_action :authenticate_user!

Can you please check whether it works for you?

[austinpray]

This is great, guys. Good job all!

[adamniedzielski]

@gonzalo-bulnes We should try to preserve all the features of v1.x series. However, there are some things which might work differently:

a) fallback_to_devise option

I'm not quite sure how multiple authentication strategies are handled. The logic behind this should be somewhere in Warden, but its code is difficult for me to read.

As far as I can tell, at the beginning strategies are selected based on their valid? method. Then authenticate! method of the first strategy is invoked. It can fail or pass the execution to the next strategy (example: https://github.com/plataformatec/devise/blob/master/lib/devise/strategies/rememberable.rb#L24).

I am not sure what the ordering of strategies is. I will explore it further.

b) multiple acts_as_token_authentication_handler_for

  # Several token authenticatable models can be handled by the same controller.
  # If so, for all of them except the last, the fallback_to_devise should be disabled.
  #
  # Please do notice that the order of declaration defines the order of precedence.
  #
  # acts_as_token_authentication_handler_for Admin, fallback_to_devise: false
  # acts_as_token_authentication_handler_for SpecialUser, fallback_to_devise: false
  # acts_as_token_authentication_handler_for User # the last fallback is up to you

This can be difficult to preserve too. I found similar functionality in Devise - https://github.com/plataformatec/devise/blob/master/lib/devise/controllers/helpers.rb#L75

I think we should aim for basic backwards compatibility even in v2.0. We can create aliases for acts_as_token_authenticatable and acts_as_token_authentication_handler_for to make the transition easier.

I don't know how to work together on this PR. I see two possibilities:
a) I will give push rights to my fork for @jbender, @gonzalo-bulnes and everybody interested. Then every commit will appear here, in this PR.
b) @gonzalo-bulnes will merge this PR to a separate branch (not master) and everybody will be able to make new PR against that branch

[donbobka]

b) multiple acts_as_token_authentication_handler_for

Idea of multiple scopes for me was to separate actions between different scopes with only/except parameters:

acts_as_token_authentication_handler_for User, only: [:new, :edit]
acts_as_token_authentication_handler_for Admin, only: [:delete]

Code to achieve this logic with devise:

before_action :authenticate_user!, only: [:new, :edit]
before_action :authenticate_admin!, only: [:delete]

Logic documented in README is a side-effect of two features: fallback_to_devise and multiple scopes. IMO it's very strange usage.
Code to achieve that logic with devise:

before_action :current_admin
before_action :current_special_user
before_action :authenticate_user!

[adamniedzielski]

@donbobka - thanks for your input! I agree that:

acts_as_token_authentication_handler_for Admin, fallback_to_devise: false
acts_as_token_authentication_handler_for SpecialUser, fallback_to_devise: false
acts_as_token_authentication_handler_for User # the last fallback is up to you

is strange and I cannot see any real use-case for it. However, it's good to know that it would be possible to achieve after the rewrite.

[gonzalo-bulnes]

Hi @adamniedzielski,

When it became possible, I documented how an unique controller could handle token authentication for several models because of #39. But its definitely a rare use case, and I would consider it myself as a bad design smell; that consideration goes beyond the scope of the gem however, and that's why I documented it anyway.

The fallback_to_devise option is only necessary if required to fix # 49. If that security issue can be handled using protect_from_forgery :null_session as stated by @donbobka in #67, we could consider removing it. I didn't knew about that possibility before he mentioned it and am just looking for a confirmation ; )

Concerning the PR organization, which one do you prefer, a) or b)?

[jbender]

With regards to fallback_to_devise, I think that can be side-stepped by simply setting up Devise with only simple_token_authentication as a default_strategy, i.e.

config.warden do |manager|
  manager.default_strategies :simple_token_authentication
  # Alternatively, to have it just called first, you could use this:
  manager.default_strategies(scope: :user).unshift :simple_token_authentication
end

[adamniedzielski]

@jbender - you are right, that's the way to limit available strategies. However, this does not solve the problem with existing sessions. If you look at:
https://github.com/hassox/warden/blob/master/lib/warden/proxy.rb#L320
Before applying any strategy, Warden is trying to log in user based on session.

In such a case protect_from_forgery with: :null_session should be used. I have never used it, but if it does what the name says then it looks like a solution.

If we describe all this configuration in README fallback_to_devise will be no longer necessary.

@gonzalo-bulnes - PR organization - b) should work better

[gonzalo-bulnes]

Here is the branch: spike-refactor-concerns-to-devise-strategy @adamniedzielski

[adamniedzielski]

Thanks @gonzalo-bulnes ! So basically, everybody can now make PR against spike-refactor-concerns-to-devise-strategy branch as I did here - #77

I need your (community) help! There is still a lot of things to do - especially updating the docs and tests.

[gonzalo-bulnes]

Thank to you @adamniedzielski!