lynndylanhurley / devise_token_auth

Token based authentication for Rails JSON APIs. Designed to work with jToker and ng-token-auth.
Do What The F*ck You Want To Public License
3.54k stars 1.13k forks source link

Allow multiple openauth providers #23

Open ACPK opened 10 years ago

ACPK commented 10 years ago

In my app, I'm using a seperate model for openauth keys so that a user can connect with multiple providers. Will this be something that you'll be supporting down the road?

lynndylanhurley commented 10 years ago

I'm open to supporting multiple OAuth providers, but I'm worried that problems will arise that may not have a definitive solution. For example:

I can think of different use-cases that would require different solutions to these problems. I guess my question is, is there a single best way to do this?

lynndylanhurley commented 10 years ago

After some thought, here is my proposal for this feature. I can think of two possible approaches to this, which I'm calling "Plan A" and "Plan B".

Plan A

  1. Account creation / authentication continues to exist in its current state. Users will continue use a single auth provider (OAuth or email) to sign-in.
  2. A new model will be created for additional 3rd party accounts. This model will have a belongs_to relationship with the original User model.

This has the following benefits:

This plan has the following drawbacks:

Plan B

An alternative plan would be to use belongs_to associations for all providers.

  1. All providers (email + OAuth) exist as belongs_to associations of a core User model.
  2. Authentication will be done against the provider models instead of the core User model. This will allow login via multiple provider accounts (OAuth + email).
  3. The original User model is reduced to a unique uid column. The model will used only to bind the various provider models together via has_many associations.

This plan has the following benefits:

But this plan has worse drawbacks in my opinion:


I can see some benefit to "Plan A", and I hate "Plan B". I'll leave this open for a while to see if anyone has any comments before I get started.

skyriverbend commented 9 years ago

I used this method in the past to hook up multiple providers on one account and it worked really well:

http://sourcey.com/rails-4-omniauth-using-devise-with-twitter-facebook-and-linkedin/

asoules commented 9 years ago

Plan C, leave it up to the developer? I've always used Omniauth to achieve multiple provider auth. Omniauth cleans up the authentication strategies, but does not enforce any particular way of storing the provider data. That way the developer gets to choose what makes sense for their implementation. Personally, I'm going to implement what you described in Plan B, one way or another, because that's how my application has to work. That said, I've also worked on apps that only support Facebook auth, so leaving things the way you have them would be fine for that.

In short, I would prefer to implement the provider storage myself, in whatever models make sense for the app I'm building.

lynndylanhurley commented 9 years ago

@jonlemmon - I'm having trouble accessing that link. Can you double-check that it's correct?

@asoules - I definitely don't want to do anything that would complicate things for developers. I'm curious - do you think this gem is doing anything now that makes it difficult to implement multiple provider auth?

@Urbanviking - I'm not sure if I understand what you're suggesting in your second comment. Can you please clarify?

asoules commented 9 years ago

@lynndylanhurley perhaps I'm mistaken, but it seems to make the assumption that a single provider was used to sign up, and the details are stored in User#provider and User#uid. I tried to leave these fields out, but I hit validation errors when devise_token_auth tried to validate that no other users existed with the same provider and uid. The OmniauthCallbacksController also assumes that User#provider and uid will be used. This makes it difficult to connect with multiple providers out of the box.

ghost commented 9 years ago

@lynndylanhurley

I removed my previous comment to make myself more clear.

I suggest staying with the single auth provider concept - email or OAuth provider - and remove the User Info attributes including the email attribute.

It would make Devise-Token-Auth simple to use (and understand) for beginners and more experience can override the default controllers and create additional tables to add multiple auth providers and user info as needed.

It seems messy to have some user info attributes in user (authentication) table and particularly the email attribute when email address is also stored in the uid attribute.

lynndylanhurley commented 9 years ago

@Urbanviking - thanks again for your comments.

It would make Devise-Token-Auth simple to use (and understand) for beginners and more experience can override the default controllers and create additional tables to add multiple auth providers and user info as needed.

I expect to be able to perform these basic operations without any trouble:

With the current implementation, these operations are trivial. But if the name, nickname, email and image fields are removed, then these features will need to be added manually. I think that removing these fields will make the use of this gem more complicated for most developers.

Also, you can override the default implementation as you like.

It seems messy to have some user info attributes in user (authentication) table and particularly the email attribute when email address is also stored in the uid attribute.

"Email" is just another provider, where the unique id (uid) is the email address itself. It would be messier (in my opinion) to authenticate against one field for some users, and another field for other users.

lynndylanhurley commented 9 years ago

@asoules - I may have a proposal to solve that issue, but I'll need time to work it out. I'll post back ASAP.

ghost commented 9 years ago

Apologies, I offended you with the "messy", but it is not clear that UID column can be an email address when you also have Email column.

The UID and PROVIDER need to form an unique key together. All User Info columns should be optional including EMAIL.

Don't you need a SECRET column on user table for e.g. Facebook authentication?

lynndylanhurley commented 9 years ago

Apologies, I offended you with the "messy"

@Urbanviking - no offense taken! I appreciate your feedback :smiley:

it is not clear that UID column can be an email address when you also have Email column.

uid stands for "unique id". It's the field that is used to identify users (along with the provider field). When users register using email, their email address is their unique id. I don't understand why this would be a problem.

The UID and PROVIDER need to form an unique key together. All User Info columns should be optional including EMAIL.

That is how it works currently.

Don't you need a SECRET column on user table for e.g. Facebook authentication?

The secret is defined in the provider's initializer. There are instructions here.

ghost commented 9 years ago

@lynndylanhurley Only UID is a unique key add_index :users, :uid, unique: true I am suggesting add_index :users, [:provider, :uid], unique: true

If EMAIL is not used for authentication, perhaps it should be listed under User Info instead of Database Authentication in devise_token_auth_create_users.rb migration file :-)

lynndylanhurley commented 9 years ago

@Urbanviking - that makes sense. Can you please post that as a new issue? I'd like to steer this issue back to multi-user authentication if possible :jack_o_lantern:

ACPK commented 9 years ago

@lynndylanhurley Was there any progress made with this? :)

mkhatib commented 9 years ago

fwiw, I've just implemented my own single-account login using any account (email+password, facebook and Google). The single account ID is the email.

Here's the changes I had to make in order to get this to work, mostly overriding an action in a controller (from devise_token_auth) just to drop some hard-coded things like the AND provider='email' clause in session controller to allow non-email provider to login using email+password. And dropping a check for provider=='email' in PasswordsController#update to allow non-email provider to update and set their passwords.

Here's the change in my project. https://github.com/manshar/manshar/pull/177/files

phs1919 commented 9 years ago

I agree with @asoules I am trying to implement multiple providers in reference to https://github.com/intridea/omniauth/wiki/Managing-Multiple-Providers (I think this is plan B @lynndylanhurley ) but I have to override so many controllers and concern (anything which assumes user has provider and uid). I would really appreciate it if the gem is flexible enough to allow multiple providers.

harmoney-justins commented 9 years ago

+1 @phs1919 @mkhatib

frankjwu commented 9 years ago

@mkhatib One problem I see with using email as the unique account ID is that users might not use the same email address across their different OAuth provider accounts. Some providers also don't provide email (Twitter for example). Your solution appears to work, but to me, it doesn't seem nearly as robust as Plan B suggested by @lynndylanhurley.

@phs1919, were you able to successfully implement that? I'm planning on doing something similar, but I'm a bit wary of overriding so much code.

harmoney-justins commented 9 years ago

@frankjwu i've done something similar with https://github.com/intridea/omniauth/wiki/Managing-Multiple-Providers

But i agree plan B is more robust, allow users to associate with a primary account

frankjwu commented 9 years ago

@harmoney-justins What do you use for a User's UID and provider? (assuming you now store that information in the Identities table)

justinsoong commented 9 years ago

@frankjwu https://gist.github.com/justinsoong/b273626fb8f994dfade3

frankjwu commented 9 years ago

@justinsoong I might be missing something, but that looks like standard Devise and not DeviseTokenAuth?

The problem I was referring to earlier is that DeviseTokenAuth uses the uid value in the header to lookup and validate tokens (with users identified by a unique uid and provider pair). So if that information is instead now stored in the Identities table, you'll have to go in and make changes to any code that assumes that the User model has uid and provider.

seddy commented 8 years ago

If anyone comes across this issue, would be good to get feedback on PR #453 to see if that would solve your problems.

wangzuo commented 3 years ago

I made some quick changes on https://github.com/wangzuo/devise_token_auth for multiple provider support