Multi auth methods per resource

[zachfeldman]

This is a continuation of the great work by @seddy done here:
#453

I've solved conflicts with master and I have only 2 failing tests in the suite.

After reviewing the code, I do think adding a provider parameter to the auth hash is a really good idea (option 1 here #453 (comment)) rather than changing the uid parameter.

@lynndylanhurley @MaicolBen what do you think?

[MaicolBen]

Thanks for bringing back this. As we discussed, this should go in the next release.

When you can, please rebase

[MaicolBen]

I guess it was previously logged with devise and doesn't have a token, maybe @lynndylanhurley can give some hint

[Evan-M]

I tried just commenting the line out and re-running tests, and it led to a failing test that was added in PR #434. I repeated this on the master branch and it led to the same failing test.

Upon closer inspection of PR #434, I'm not sure that the tests defined actually test the feature added (limiting concurrent devices/tokens). Furthermore, it looks like testing maximum concurrent tokens happens inside the block that tests the standard devise auth functionality rather than token auth.

Can anyone confirm or refute this?

[Evan-M]

Thinking about this further, I'm having a hard time understanding a use case for this.

This block of code is only relevant if DeviseTokenAuth.enable_standard_devise_support = true, and if an existing user has been authenticated with warden/devise. Under these circumstances, it seems like not only is there no need to create a token, but the controller response should not contain any token auth related headers either.

Does anyone know if a use case for returning token auth headers despite authenticating via warden/standard devise?

[zachfeldman]

@Evan-M please feel free to remove the tests for now or do what you need to do to move the PR forward!!

[MaicolBen]

Don't remove tests that should be working despite it's using single or multi auth providers. I don't know why this line, but if you can't find the reason, just remove the comment above

[zachfeldman]

Sorry @MaicolBen ....just trying to help @Evan-M move this PR forward as it's been open for a while and would be awesome to get in the project!

[Evan-M]

@zachfeldman @MaicolBen: I am very reluctant to remove any tests!

However, I am trying to better understand the intent of these tests, and the implementation code that satisfies them. If warranted, I will re-write or re-factor some of these tests.

Admittedly, some of my questions/comments here are perhaps beyond the scope of this PR (allowing multiple auth methods per resource).

Currently, I've added 11 commits to my fork of this PR (13 files changed, 102 insertions(+), 40 deletions(-), according to git diff --shortstat). Additionally, there are prior the commits by @zachfeldman, @ram535ii, and others. Personally, I would prefer to provide shorter, and easier to understand (i.e. review) PRs.

To this end, I plan on spending time this weekend to extract some of the commits from my fork into separate PRs. I think most of these separate PRs will focus on cleaning up confusing or redundant code, and ensuring that certain tests do in fact fully test what they intend to.

Perhaps most crucially, I want to ensure everything remains backwards compatible!

[zachfeldman]

@Evan-M I totally agree that this PR is huge and it's pretty hard to move forward with such a monolith....looking forward to seeing your work!

[MaicolBen]

This creates a new one without considering the previous tokens

[MaicolBen]

Agreed

[MaicolBen]

We should refactor create_new_auth_token and contemplate this case

[gitcoinbot]

This issue now has a funding of 0.15 ETH (115.04 USD) attached to it.

• If you would like to work on this issue you can claim it here.
• If you've completed this issue and want to claim the bounty you can do so here
• Questions? Get help on the Gitcoin Slack
• $12029.79 more Funded OSS Work Available at: https://gitcoin.co/explorer

[owocki]

hi from gitcoin folks! @zachfeldman @seddy would you like for me to help source a dev for this task? happy to help.

what is the exact s cope of the task you're looking for a bounty hunter to do?

[tra38]

Hi, I saw this bounty from Gitcoin and am interested in working it. I would like to know what is the exact scope of the task.

[MaicolBen]

@tra38 Please read #453 first, fix the tests and test it with 1, 2 and more methods/providers (like facebook, twitter, email) with a frontend app or postman. Am I missing something @zachfeldman ?

[lynndylanhurley]

I need some time to review the approach for this PR - I just want to make sure we're covering all the use-cases for this. I've been crazy busy, but I'll have time this weekend to review.

[Evan-M]

I've been working on this from a fork. I've fixed the final two failing tests, and rebased it against the upstream master branch. (I'm really hoping to use this feature in a prod app soon!)

I am a bit confused by something though. I understand the resource_finder_for methods handle the lookups for multiple authentications, but if a finder method returns nil (i.e. there is no existing authentication and thus a new record must be created) it looks like the assign_provider_attrs call from within the following method attempts to assign the attributes in the auth_hash to the resource class:

# From 'app/controllers/devise_token_auth/omniauth_callbacks_controller.rb':

def get_resource_from_auth_hash
  @resource = resource_class.find_resource(
    auth_hash['uid'],
    auth_hash['provider']
  )

  if @resource.nil?
    @resource          = resource_class.new
    @resource.uid      = auth_hash['uid']      if @resource.has_attribute?(:uid)
    @resource.provider = auth_hash['provider'] if @resource.has_attribute?(:provider)
    @oauth_registration = true
    set_random_password
  end

  # sync user info with provider, update/generate auth token
  assign_provider_attrs(@resource, auth_hash)

  # assign any additional (whitelisted) attributes
  extra_params = whitelisted_params
  @resource.assign_attributes(extra_params) if extra_params

  @resource
end

My confusion is that with a multiple-authentication setup, the authentications are stored on a separate model associated with the resource class.

As a work-around in my application code, I've overridden the assign_provider_attrs() method to properly assign attributes to the correct model. However, it feels like there should be a better way.

Perhaps, in a similar vein to the resource_finder methods, there could be resource builder methods? Or if using ActiveRecord, then perhaps accepts_nested_attributes_for could enable attribute assignment through the associated authentication record.

Or even, in order to maximize developer flexibility, simply adding more documentation and examples on how to set up the controller overrides would be helpful here.

Thoughts? Advice?

UPDATE:

I've since pursued an alternative strategy for building associated authentication records.

As described in the README, I added a block inside a custom override of the DeviseTokenAuth::OmniauthCallbacksController in my rails app:

module DeviseTokenAuthOverrides
  class OmniauthCallbacksController < DeviseTokenAuth::OmniauthCallbacksController

    def omniauth_success
      ActiveRecord::Base.transaction do
        super do |resource|
          resource.authentications.create! do |auth|
            auth.provider = auth_hash[:provider]
            auth.uid = auth_hash[:uid]

            auth_hash[:info].tap do |i|
              auth.name = i[:name]
              auth.email = i[:email]
            end

            auth_hash[:credentials].tap do |c|
              auth.token = c[:token]
              auth.token_expires_at = Time.zone.at(c[:expires_at]) if c[:expires]
            end
          end
        end
      end
    end

end

NOTE: The above code is shown for simplicity. In my actual application code, I've defined the above #create! part as #create_from_auth! in an ActiveRecord Association Extension declared on the has_many :authentications association in my User model.

This lets me extract the logic around building Authentication records with an omniauth auth_hash out of the omniauth_callbacks_controller and into a module in my Authentication model (which is included in the has_may association with an extends: option). The resulting block in the controller override becomes a simple resource.authentications.create_from_auth!(auth_hash).

[Evan-M]

@zachfeldman:

After reviewing the code, I do think adding a provider parameter to the auth hash is a really good idea (option 1 here #453 (comment)) rather than changing the uid parameter.

While I agree with you about adding a provider parameter, as I've attempted to make this all work with ng-token-auth, I've had to make changes to my client code. While not a big deal, it could easily be a source of confusion down the road for others.

It seems with the addition of a provider parameter here, the ng-token-auth Angular module should be updated accordingly, in order to keep these two libs working together in a turnkey manner.

As a side note, my attempt to split uid into two params stemmed from having the space that separated the uid and provider string in the concatenated version escaped incorrectly (in the client, I think). This resulted in a failure to split the uid and provider strings correctly in the existing uid.split call.

[zachfeldman]

@Evan-M I think adding a provider parameter is fine, but we should probably default back to the, "old" behavior if it's not there so we don't break the old client libraries. At some point we could remove the old behavior. Would that be really hard to do?

[KelseyDH]

Anxiously awaiting for this PR to get worked out!

(Namely, so I can enable the ability to customize what 'provider' gets set from request parameters for creating a flow for username logins. #1066 If this PR can help with this, I will happily update devise_token_auth's docs with instructions for steps on doing this once this gets done.)

[vs77bb]

@tra38 @Evan-M @lynndylanhurley @owocki Hey guys, Vivek from Gitcoin here... checking in to see if this one is scoped / if Tariq is still potentially interested in moving forward, if so?

[tra38]

While I would be happy to work on the issue, so far I held off on it because it appears that @Evan-M is currently working on this issue. I'm always ready to help people out when necessary.

[jalevin]

@Evan-M I've been testing out your branch. I believe you need to disable omniauth callbacks in your devise_token_auth initializer. Can you confirm?

Would be worth adding to the docs.

[Evan-M]

@jalevin: you are correct! I had to disable the default omniauth callbacks, but I still utilized the DeviseTokenAuth::Concerns::UserOmniauthCallbacks concern in my model that holds the multiple provider authorizations.

I have the following setup in my rails app:

# config/initializers/devise_token_auth.rb:

DeviseTokenAuth.setup do |config|
  ...
  config.default_callbacks = false # for multi-omniauth
  ...
end

# app/models/user.rb:

class User < ActiveRecord::Base
  # model does NOT contain either :provider or :uid attributes

  include DeviseTokenAuth::Concerns::User
  has_many :authentications, inverse_of: :user, dependent: :destroy 
  ...
end

# app/models/authentication.rb:

class Authentication < ActiveRecord::Base
  # model contains :provider, :uid, and :email attributes

  include DeviseTokenAuth::Concerns::UserOmniauthCallbacks
  belongs_to :user, inverse_of: :authentications
  ...
end

Definitely will add a mention of this to the docs; I'll probably recycle this comment, and place it in the README.md.

[KelseyDH]

@Evan-M More details on this approach would be much appreciated, especially how you structured your data & query for has_many :authentications!

[jalevin]

@Evan-M I'm attempting to first require password authentication before I move onto omniauth. Does this pull request reset the value of current_user at some point when it loads? The mapping will have a local provider, and 2 omniauth providers, however, we need the local provider to authenticate before we associate the providers.

Also,

auth_hash[:credentials].tap do |c|
   auth.token = c[:token]
   auth.token_expires_at = Time.zone.at(c[:expires_at]) if c[:expires]
 end

Do you have any mechanisms in place to validate those expirations?

[vs77bb]

Hi @Evan-M are you still working this issue? cc @owocki

[Evan-M]

@vs77bb: I am, but have had limited time the past few weeks.

[zachfeldman]

Hey all - I'd like to move forward on this PR. If it requires us to break this up into 2 or 3 larger issues, or some more funding, let me know and I can make it happen! But it's an oft-requested feature.

We're starting to use Open Collective to fund the project:
https://opencollective.com/devise_token_auth

in case that helps.

[zachfeldman]

(I personally am not up to date on the work done by @Evan-M for instance and don't really have the time to dive in currently)

[Evan-M]

@zachfeldman Hey, I hear you on having limited time! My apologies for not being more communicative around the current state of my fork.

Why don't I spend a few minutes this evening and come up with a plan for how I see this getting broken up into a few separate PRs? That way if anyone else wants to help out, there is more direction as to what need to be done to get this merged.

My biggest concern is that in the interest of getting the feature working with our Rails app, I've introduced changes that are not backwards compatible. Probably the most significant change is the introduction of a separate :provider parameter (as opposed to concatenating the :provider onto the :uid parameter). This additional param is added to the token headers, and to the query strings on some of the redirects.

There is a bit of a paradigm shift that happens with Devise + multi-provider Omniauth. The has_many/belongs_to association between the User and Authentication models (or whatever they end up being called) means that you cannot reliably determine the :uid or :provider from a given User instance. Instead, they must be provided explicitly (since there could be multiple). While this is still possible via concatenating the :uid and :provider into a single string, everything is much easier when they are separate. There are a handful of different use-cases for which I need to tests to ensure that devise_token_auth works correctly with and without having :uid or :provider attributes present on the User.

[zachfeldman]

That sounds great! I think breaking it up into smaller issues that we can put bounties on would really help.

[ineverov]

@zachfeldman @Evan-M any updates on this? very desired feature

[MaicolBen]

@ineverov unfortunately, I don't think this will be resumed unless someone (if you can, awesome!) takes it over.