Add hooks/callbacks to the API?

[gonzalo-bulnes]

I'm opening this issue to evaluate the possibility of adding hooks to the Simple Token Authentication API. The goals are:

• List the possible use cases
• Define the possbile sets of hooks
• Define the corresponding specs

Possible use cases

1. Disposable authentication token (see #199) - [confirmed]

   As a developer In order to implement disposable authentication tokens I want to be able to reset the authentication tokens automatically after a successful authentication

2. Customize Devise configuration when token authentication is performed (see #136)

   As a developer In order to skip the confirmable redirect before token authentication Even if Devise provides no skipping mecanism I want to be able to disable the confirmation strategy before performing token authentication

Possible set of hooks

• before token authentication (use case 2)
• after successful token authentication (use case 1)
• after token authentication failure (is that useful?)

[cspags]

Hi, a hook after successful token authentication would definitely be nice. But thank you for the solution you provided in #199 for that use case.

[gonzalo-bulnes]

Thank you for your feedback @cspags! (I'd like to move forward with this but definitely don't want to implement anything useless. I'd mark the first use case as confirmed.)

[gonzalo-bulnes]

A third possible use case for hooks:

1. Disposable authentication token (see comment above)
2. Customize Devise configuration when token authentication is performed (see comment above)

3. Expirable authentication token (see #52 and #199 for inspiration)

   As a developer In order to implement expirable authentication tokens I want to be able to store the token creation date when a new authentication token is generated And to be able to reset the authentication tokens before they are used if they are expired

The updated possible set of hooks:

• after token generation (use case 3)
• before token authentication (use cases 2 and 3)
• after successful token authentication (use case 1)
• after token authentication failure (is that useful?)

[gonzalo-bulnes]

I'm thinking in the following possible API for hooks (the examples are Rails-ish but don't need to be):

Option 1: Method override

Usage: override method called after_successful_token_authentication (the method is already defined behind the scenes, but does nothing by default).

# app/controller/products_controller.rb

class ProductsController < ActiveController::Base
  acts_as_token_authentication_handler_for User

  # ...

  private

    def after_successful_token_authentication
      # Let's say I want the authentication token to be disposable
      renew_authentication_token!
    end
end

Option 2: Callback

Usage: similar to the ActiveRecord callbacks (e.g. before_action).

# app/controller/products_controller.rb

class ProductsController < ActiveController::Base
  acts_as_token_authentication_handler_for User

  # Let's say I want the authentication token to be disposable
  after_successful_token_authentication :renew_authentication_token!

  # ...
end

Option 3: Block definition

# app/controller/products_controller.rb

class ProductsController < ActiveController::Base
  acts_as_token_authentication_handler_for User
  after_successful_token_authentication do
    # Let's say I want the authentication token to be disposable
    renew_authentication_token!
  end

  # ...
end

Discussion

• Option 1 ensures some consistency across all the token authenticatable classes (controllers in the Rails world) - all the custom behaviour is defined in methods of the same name. It is also the simplest implementation.
• Options 2 and 3 are fairly similar. They does not assume anything about the eventual presence of a after_successful_token_authentication, which could be good; but they require to store either the callback name or the block, which makes them a little bit more intrusive (one method definition + one variable to store the name/block). Indeed, they would probably assume the variable is not used -whatever name it gets- because I think that working around that would be really over-engineering.

Any thoughts @cspags ?

[gonzalo-bulnes]

I wrote an example implementation in the spike-add-after-successful-token-authentication-hook branch, with the corresponding usage instructions. As usual, feedback is welcome!

[gonzalo-bulnes]

Hi @cspags,

I released the after_successful_token_authentication hook in v1.15.0.

[cspags]

Awesome, thanks @gonzalo-bulnes!

[Startouf]

@gonzalo-bulnes I'm having a very hard time using your after_successful_token_authentication hook. It seems this hook cannot be used through inheritance or module mixin (with/without ActiveSupport::Concern)

Have you ever managed to successfully use an after_successful_token_authentication in some situation like

class API::ApplicationController < ActionController::API
  def after_successful_token_authentication
    set_exception_notifier_current_user
  end
end

class API::V1::FoosController < API::ApplicationController
  acts_as_token_authentication_handler_for User,
            fallback: :exception
end

The hook simply doesn't get run in my case, except when I copy the code of after_successful_token_authentication directly in my API::V1::FoosController class

[Startouf]

@gonzalo-bulnes it would seem that the current hook is not very mixin/inheritance-friendly, as it is actually being overriden by the default after_successful_token_authentication method injected by acts_as_token_authentication_handler_for

The thing is, in order for it to work, I'd need to have acts_as_token_authentication_handler_for before I override after_successful_token_authentication but this is simply not working if I want to have control over the fallback (depending on my controllers, I have fallbacks to :devise, :exception, or :none)

Either the method should be injected along with the default ActionController::Base mixin that also provides acts_as_token_authentication_handler_for OR the code should be clever and not override the method if it already exists... what do you think is better @gonzalo-bulnes ?

Actually, for a proper implementation, I'd suggest

• Not injecting the ActionController mixin so people can decide for themselves where/when they want it
• Use ActiveSupport::Callbacks (or the ones provided by ActionController for Rails), so people can register modules with low coupling

This is how I would register the callbacks

module ControllerMixin
 included do 
   define_callbacks :token_authentication
  end
end

so in your code you can just

      if token_correct?(record, entity, token_comparator)
        run_callbacks(:token_authentication) do
          perform_sign_in!(record, sign_in_handler)
        end
      end

Since this changes existing implementations, I have suggested a smaller patch instead that uses respond_to? so we don't have to declare that blank method in the PR #333

[gonzalo-bulnes]

Interesting. Thank you for the detailed report and pull request @Startouf!

Besides removing the test that checks that token auth. handlers respond to the hook —which is not what we want anymore— I'd be interested in adding a test case for the scenario you described. It seems like a common scenario and the regression test would be useful for any other hooks as well.

[gonzalo-bulnes]

If you @Startouf (or anyone else!) is keen on giving a try to the testing part, I'm happy to help getting this shipped :slightly_smiling_face:

[Startouf]

I'm adding this to my TODO for when I have free time to work on the test, I believe it should either be done in the next couple days or it's a bad sign since I'll be on holidays in a week ^^"

[gonzalo-bulnes]

Awesome! No hurry at all. Remove the failing test (I think it's the right thing to do). If you can't figure out a way that you find reasonable to add a regression test, let me know and I'll take a look too. :+1:

[Startouf]

@gonzalo-bulnes everything should be fine now. It's a bit hard to test a method that could or could not be defined, because in the specs calling expect(handler).to receive(:hook) does define the method hook even if it wasn't part of the class (and therefore, it will also respond_to that method). So I'm just checking the code does not crash when the method is not defined in one of the tests.

Because of this, I'm not actually convinced that my solution of just adding a if respond_to?() is very clean. It would probably be better to have a layer of metaprogramming to define this hook properly via a custom method (like ActiveRecord after_validation do), but then it's probably not worth the extra complexity of the code. Or, we could use ActiveSupport::Callbacks to handle this part in a clean manner (as AS is already a dependency)

[gonzalo-bulnes]

_Let's move the conversation to https://github.com/gonzalo-bulnes/simple_token_authentication/pull/333_ @Startouf : )