Move configuration for identifier and token retrieval to the controller.

[lowjoel]

Addresses #207. I have not fixed the specs yet, because I'm not sure how the design will evolve.

This allows for a per-controller configuration of where identifiers and tokens should be searched. The idea is that there is clean separation between the model concerns from the controller concerns -- Entity seems to be very much closer to the model and should probably not interact with both the model and controller.

I am thinking of deprecating global configuration of which headers and which controller params to look up when deciding which entity to authenticate against. That makes the API inconsistent -- whoever is configuring the system needs to know how the model is translated to the entity name.

Here, instead of the global configuration, an extra configuration option search can be passed to acts_as_token_authenticatable:

acts_as_token_authentication_handler_for(User, search: {
  headers: {
    identifier: 'x-user-email',
    token: 'token'
  },
  params: {
    identifier: 'user',
    token: 'token'
  }
})

This would search for request.headers['x-user-email'], and request.headers['x-user-token'], as well as params['user'] and params['token'].

If you would like to disable any method,

acts_as_token_authentication_handler_for(User, search: {
  headers: {
    identifier: 'x-user-email',
    token: 'token'
  },
  params: false # or nil
})

Then params will not be looked up. The search options in the PR currently default to the v1.11 names. Global configuration with overrides need to be manually changed to the new format.

I'm not entirely sure how many different uses for global configuration there are, so I'm not sure if there are edge cases I've not considered, but comments welcome.

@gonzalo-bulnes 64 specs break, I'm not sure how to fix all of them.

[lowjoel]

Looking around, I'm not entirely sure which specs are testing which components -- having a list of specs to keep would be good...

[lowjoel]

@gonzalo-bulnes I've pushed some spec fixes, what's left are the entity specs (API change) and some feature specs (I'm changing the API...?)

[lowjoel]

Okay, specs pass. I removed a few, changed others. Please review and let me know what you think.

I'll squash the commits down when we finalise the design.

[gonzalo-bulnes]

Note to self (not necessarily related to the PR): Check this, there may be a missing example here.

[gonzalo-bulnes]

Hi @lowjoel,

There are several very large changes here, and I'll need some time to review them. A few general comments:

Reflecting on how global configuration options and token-authentication-handler-specific options are related and interact with each other is something that, I think, would be very beneficial, and I agree with that. (In that direction, I've been thinking in the possibility to include support for YAML configuration files, and for an -optionally- deeper options granularity.)

I don't think that implementing this feature requires to break backward compatibility, and I'm not keen of unnecessary backward-compatibility breakage.

There is a tension between convention and configuration in the design of any configurable tool, and I tend to prefer convention over configuration. A balance must be found in this matter. Independently of my own inclinations, I think that's true, and I would be very careful, when considering what's consistent or not, to specifiy consitent, or inconsitent with what, in order to be sure that the discussion helps finding that balance.

I'll start reviewing your PR in detail as soon as I can, so we can talk on more precise feedback. Thank you!

[lowjoel]

I have tried to cover all bases when implementing this design, it's just whether I missed any use cases out. I managed to fix most of the specs, removing only global config and entity methods.

I agree that in principle convention over configuration is better (indeed, that's the Rails philosophy) and I've tried to keep the defaults. I think only those who did global configuration would need to change things (and it's a change from global config to controller config)

Let me know how it goes!

[lowjoel]

Hey @gonzalo-bulnes, have you got a chance to review this in detail? 😄

[gonzalo-bulnes]

Hi @lowjoel!

No, not really - really not in fact. I'm moving and lately I only managed to check small things here and there. It's booked however, and a while ago I started a few experiments to check if the implementation can be made backwards compatible. Be sure I'll review this thorgoughly as soon as I can and I'll let you know when I do.

Happy new year ; )
Regards!

[lowjoel]

@gonzalo-bulnes happy new year to you too!

Let me know when you get a chance. I'd love to see this merged 😄

[lowjoel]

Hi @gonzalo-bulnes any news on this? :)

[gonzalo-bulnes]

Hi @lowjoel,

I intended to update you earlier this week, I'm sorry I didn't. So, I settled down, and have been giving priority to the Rails 5 support in the past couple of weeks. (That's pretty much done now.)

To be honest, I need to take a fresh look at the API you propose, because it seems more complex that necessary to me. That being said, as I said before, I think that the idea of enabling or disabling the params or headers lookup is absolutely fine, and it's part of my schedule.

[lowjoel]

glad to hear that! Let me know how I can be of help.

[lowjoel]

hey @gonzalo-bulnes, let me know if there's anything else I could help to push this PR through.