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 resources to auth with multiple methods #453

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@seddy

seddy commented Nov 25, 2015

TL;DR

This change allows resources (e.g. User) to authenticate through multiple methods for the same real life person. Prior to this, each auth method (e.g. facebook, twitter, etc) would create a new row in User when it was used.

Summary

It's inspired off the back of a number of issues raised around multiple authentication methods (particularly with oauth), primarily a longstanding issue #23. Within that issue there is suggestion of plans A and B, which in short are:

  • Plan A - A new model will be created for additional 3rd party accounts. This model will have a belongs_to relationship with the original User model.
  • Plan B - All providers (email + OAuth) exist as belongs_to associations of a core User model. Authentication will be done against the provider models instead of the core User model.

These are both valid ways of accomplishing this end, so we've largely followed:

  • Plan C - leave it up to the developer.

We've done this by trying to avoid being opinionated about how users should implement their third party domain objects (e.g. FacebookUser or TwitterUser) and what relationship they may have to the authenticating resource.

For more information on the approach we've taken, check out the README changes we've made, specifically this section.

Core changes

There are a couple of conceptual changes worth highlighting in this PR:

1. Dropping usage of provider/uid columns when allowing multiple auth

The main issue we had with the existing implementation was the reliance on the provider and uid columns which had to be added to each resource. It means that whoever uses the gem must model their third party model implementations around these columns in order to have omniauth. It also had the consequence of real life users potentially ending up with multiple rows in the users table, which wasn't something we wanted to allow in our system.

2. Restructuring the uid field in the http header

The main thing which we'd like opinions on is the change to the uid field. In the previous implementation, when we call set_user_by_token, we would:

  1. Pull the uid out of the http header
  2. Query for the uid in the resource table to get the row to authenticate.

We wouldn't need to worry about what provider they originally signed in with (e.g. facebook, twitter, email, etc). However, in order to give developers freedom to decide how they model their third party domain objects (and therefore how they are associated to the resource) we need the provider field.

So the big question is how do we pass around the provider field between requests? As we see it, there are three options:

  1. Add a new provider field to the http token headers
  2. Append the provider to the existing uid value in the http token headers
  3. Change the uid value in the http token to being the resource's id.

A quick table of (what we think) the pros and cons are:

Option Pro Con
1 Simple and clear to implement Inefficient resource_finder_for lambdas are problematic
uid field remains unchanged, so it's actually a uid
Easily backwards compatible
2 Clear about what's going on, but not as much as 1 Polluted uid field
Backwards compatible (in the current implementation) Inefficient resource_finder_for lambdas are problematic
Already implemented... May break implementations where people are already storing uid in client systems
3 No danger of inefficient resource_finder_for implementatoins Not easily backwards compatible
Exposes internal id fields, which may not be desirable

We've implemented option 2, but aren't particularly set on it. The primary reason was a concern about being backwards compatible and not adding more fields to the http header, as there are already quite a few of them.

Ultimately though, we're leaning towards option 1 now that we've finished the implementation, so input here would be much appreciated!

Allow resources to auth with multiple methods
This change allows resources (e.g. User) to authenticate through
multiple methods for the same real life person. Prior to this, each auth
method (e.g. facebook, twitter, etc) would create a new row in User when
it was used.

See notes in the README that have changed for this commit for more
information.
@lynndylanhurley

This comment has been minimized.

Owner

lynndylanhurley commented Nov 25, 2015

This is very interesting!! I'll review this ASAP!!

@vic700208

This comment has been minimized.

vic700208 commented Nov 26, 2015

liking option 1 as well

@ram535ii

This comment has been minimized.

ram535ii commented Dec 1, 2015

Hey @lynndylanhurley, just wanted to check if you'd had a chance to look at this yet? Thanks!

@hbarrington

This comment has been minimized.

hbarrington commented Apr 8, 2016

this would solve a problem for me. as it is, I guess I'll implement my own solution until this gets merged

@adis-io

This comment has been minimized.

Contributor

adis-io commented Jun 25, 2016

Any updates on this?

@BrahimDahmani

This comment has been minimized.

BrahimDahmani commented Dec 15, 2016

@lynndylanhurley Any updates here??

@seddy

This comment has been minimized.

seddy commented Jan 3, 2017

Just an FYI for anyone coming to this - this is a fairly stale PR, and I'm not sure when/if it will get reviewed.

I no longer have any projects that use devise (let alone devise_token_auth) so am not planning on doing anything with this, even if code review comes back. Mainly because as far as I know (@ram535ii can correct me on this!) I don't think this version of the code is running anywhere in production. Consequently, I would think it sensible to have a reasonably chunky change like this running in a production environment (which it was when we raised the PR) before having a merge to master.

I'll leave it open in case it's useful for anyone (or if anyone else is happy to rebase/refactor/make review changes), but use with care as it's pretty far behind master and has a load of conflicts which would need to be resolved before any merge.

@lynndylanhurley - if you don't think this is something you want to incorporate into the gem, I have no problem with you closing this given all this.

@seddy seddy closed this Oct 2, 2017

@zachfeldman

This comment has been minimized.

Collaborator

zachfeldman commented Oct 22, 2017

Wanted to reopen this PR as this specific functionality would actually be really useful to my org.

@seddy obvs no pressure to keep contributing. I think I'll work on it though! Really appreciate your foundational work here.

@seddy

This comment has been minimized.

seddy commented Dec 20, 2017

Closing this in favour of #990 - it's coming up in my list of open pull requests 😄

Glad to see you're carrying on the torch @zachfeldman!

@seddy seddy closed this Dec 20, 2017

@zachfeldman

This comment has been minimized.

Collaborator

zachfeldman commented Dec 20, 2017

I'm trying to @seddy ! still haven't had time to finish up that PR....it's massive. Hope all is well, Happy Holidays

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