New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple openauth providers #23

Open
ACPK opened this Issue Sep 4, 2014 · 23 comments

Comments

Projects
None yet
10 participants
@ACPK

ACPK commented Sep 4, 2014

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

This comment has been minimized.

Owner

lynndylanhurley commented Sep 4, 2014

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:

  • How will users go about associating their accounts?
  • Which account settings will take precedence?
  • If a user has already registered separate accounts using different OAuth providers (i.e. Twitter and Facebook), and now the user wants to merge their Twitter account into their Facebook account, how do we proceed? Is the user still allowed to keep the separate accounts? If not, what do we do with any data that was associated with the old account?

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

This comment has been minimized.

Owner

lynndylanhurley commented Sep 30, 2014

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:

  • Users will have access to their various accounts. For example:
    • A user that authenticated using Github will be able to easily import their LinkedIn data.
    • A user that signed up using email could easily access their facebook photos.
  • Sign-up and sign-in remain simple and performant (faster than Plan B).

This plan has the following drawbacks:

  • Users will only be able to sign-in with a single account.
  • If a user de-activates the provider account that they use for sign-in, they will be locked out of the app.

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:

  • Users can sign-in using multiple accounts.
  • If a user de-activates one of their provider accounts (i.e. deletes their facebook profile), the user could still sign in using an alternative account.

But this plan has worse drawbacks in my opinion:

  • 99% of users will only use a single account. This plan increases the complexity of the most common use case, and is slightly less performant due to excessive association lookups.
  • It's not exactly clear how to merge the accounts in terms of UX. Here is one un-resolvable issue for example:
    • What if a user creates separate, isolated accounts using multiple providers, but then wants to merge the accounts? Will this be possible, or is the user stuck with two separate accounts?
    • What if a user creates a merged account, but then wants to break them apart into isolated accounts? Is this possible, or is the user stuck with merged accounts?
  • This will probably break compatibility with previous versions.

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.

@jonlemmon

This comment has been minimized.

jonlemmon commented Oct 21, 2014

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

This comment has been minimized.

asoules commented Oct 23, 2014

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

This comment has been minimized.

Owner

lynndylanhurley commented Oct 27, 2014

@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

This comment has been minimized.

asoules commented Oct 27, 2014

@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

This comment has been minimized.

ghost commented Oct 27, 2014

@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

This comment has been minimized.

Owner

lynndylanhurley commented Oct 27, 2014

@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:

  • show a user's name when they sign in.
  • display a user's nickname + avatar image next to their user activity (comments, etc).
  • display a list of all users with their names + email addresses (for admin moderation)

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

This comment has been minimized.

Owner

lynndylanhurley commented Oct 27, 2014

@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

This comment has been minimized.

ghost commented Oct 27, 2014

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

This comment has been minimized.

Owner

lynndylanhurley commented Oct 27, 2014

Apologies, I offended you with the "messy"

@Urbanviking - no offense taken! I appreciate your feedback 😃

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

This comment has been minimized.

ghost commented Oct 27, 2014

@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

This comment has been minimized.

Owner

lynndylanhurley commented Oct 27, 2014

@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 🎃

@ACPK

This comment has been minimized.

ACPK commented Jan 12, 2015

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

@mkhatib

This comment has been minimized.

mkhatib commented Feb 25, 2015

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

This comment has been minimized.

phs1919 commented Mar 4, 2015

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

This comment has been minimized.

harmoney-justins commented Jul 14, 2015

@frankjwu

This comment has been minimized.

frankjwu commented Jul 14, 2015

@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

This comment has been minimized.

harmoney-justins commented Jul 14, 2015

@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

This comment has been minimized.

frankjwu commented Jul 14, 2015

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

@justinsoong

This comment has been minimized.

@frankjwu

This comment has been minimized.

frankjwu commented Jul 14, 2015

@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

This comment has been minimized.

seddy commented Apr 14, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment