-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Multi auth methods per resource #990
base: master
Are you sure you want to change the base?
Changes from all commits
27a824f
dbf91cd
fe22f39
c997b77
ab5b843
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,7 @@ Please read the [issue reporting guidelines](#issue-reporting) before posting is | |
* [Usage TL;DR](#usage-tldr) | ||
* [Configuration Continued](#configuration-cont) | ||
* [Initializer Settings](#initializer-settings) | ||
* [Single vs Multiple authentication methods](#single-vs-multiple-authentication-methods-per-resource) | ||
* [OmniAuth Authentication](#omniauth-authentication) | ||
* [OmniAuth Provider Settings](#omniauth-provider-settings) | ||
* [Email Authentication](#email-authentication) | ||
|
@@ -126,7 +127,7 @@ The following events will take place when using the install generator: | |
|
||
* A concern will be included by your application controller at `app/controllers/application_controller.rb`. [Read more](#controller-methods). | ||
|
||
* A migration file will be created in the `db/migrate` directory. Inspect the migrations file, add additional columns if necessary, and then run the migration: | ||
* A migration file will be created in the `db/migrate` directory. Inspect the migrations file, add or remove columns if necessary, and then run the migration: | ||
|
||
~~~bash | ||
rake db:migrate | ||
|
@@ -135,6 +136,7 @@ The following events will take place when using the install generator: | |
You may also need to configure the following items: | ||
|
||
* **OmniAuth providers** when using 3rd party oauth2 authentication. [Read more](#omniauth-authentication). | ||
* **Allow multiple authentication methods** for resource. [Read more](#single-vs-multiple-authentication-methods-per-resource) | ||
* **Cross Origin Request Settings** when using cross-domain clients. [Read more](#cors). | ||
* **Email** when using email registration. [Read more](#email-authentication). | ||
* **Multiple model support** may require additional steps. [Read more](#using-multiple-models). | ||
|
@@ -196,6 +198,82 @@ Devise.setup do |config| | |
end | ||
~~~ | ||
|
||
## Single vs Multiple authentication methods per resource | ||
|
||
By default, `devise_token_auth` only allows a single authentication per resource. | ||
|
||
What does this mean? Let's take the example of having a Customer model and you want to let people sign up with Facebook or with their email address. If they register with their Facebook account, then you'll have one row in your `customers` table, and if they then register with their email address, you'll have **another** row in your `customers` table. Both for the same real life person. | ||
|
||
This is because multiple sign in methods for a single resource are difficult to maintain and reason about, particularly when trying to build a suitable UX. The only problem is the expectation that users will always use the same authentication method. | ||
|
||
BUT, `devise_token_auth` is awesome enough (like `devise`) to let you manage multiple methods on a single resource without sacrificing your data integrity. Using our previous example, this means you can have a single Customer row which can be authenticated with **either** Facebook **or** their email address. | ||
|
||
### Setting up single authentication per resource (default behaviour) | ||
|
||
When you run `rails g devise_token_auth:install User auth`, you will have a migration setup which will look something like this: | ||
|
||
~~~ruby | ||
# db/migrate/20151116175322_add_devise_token_auth_fields_to_users.rb | ||
class AddDeviseTokenAuthFieldsToUsers < ActiveRecord::Migration | ||
t.string :provider, :null => false, :default => "email" | ||
t.string :uid, :null => false, :default => "" | ||
... | ||
end | ||
~~~ | ||
|
||
The `provider` and `uid` fields are used to record what method and what identifier we will use for identifying and authing a `User`. For example: | ||
|
||
| Signup method | provider | uid | | ||
|---|---|---| | ||
| email: [email protected] | email | [email protected] | | ||
| facebook user id: 12345 | facebook | 12345 | | ||
|
||
And that's pretty much all you have to do! | ||
|
||
**The good thing** about this method is that it's simplest to implement from a UX point of view and, consequently, the most common implementation you'll see at the moment. | ||
|
||
**The problem** is that you may end up with a single person creating multiple accounts when they don't mean to because they've forgotten how they originally authenticated. In order to make this happen, the gem has to be fairly opinionated about how to manage your domain objects (e.g. it allows multiple users with the same "email" field) | ||
|
||
### Setting up multiple authentication methods per resource | ||
|
||
You may want to let a user log in with multiple methods to the same account. In order to do this, the `devise_token_auth` gem is unopinionated on how you've built your model layer, and just requires that you declare how to look up various resources. | ||
|
||
If using this methodology, you **do not need provider/uid columns on your resource table**, so you can remove these from the generated migration when running `rails g devise_token_auth:install`. | ||
|
||
Instead, you need to register finder methods defining how to get to your resource from a particular provider. If you don't register one, it falls back to the default behaviour for single authentication of querying provider/uid (if those columns exist). | ||
|
||
An example of registering these finders is done as follows: | ||
|
||
~~~ruby | ||
class User < ActiveRecord::Base | ||
# In this example, the twitter id is simply stored directly on the User | ||
resource_finder_for :twitter, ->(twitter_id) { find_by(twitter_id: twitter_id) } | ||
|
||
# In this example, the external facebook user is modelled seperately from the | ||
# User, and we need to go through an association to find the User to | ||
# authenticate against | ||
resource_finder_for :facebook, ->(facebook_id) { FacebookUser.find_by(facebook_id: facebook_id).user } | ||
end | ||
~~~ | ||
|
||
You'll need to register a finder for each authentication method you want to allow users to have. Given a specific `uid` (for omniauth, this will most likely be the foreign key onto the third party object). You can register a `Proc` or a `Lambda` for this, and each time we get a request which has been authed in this manner, we will look up using it. | ||
|
||
**WARNING**: Bear in mind that these finder methods will get called on every authenticated request. So consider performance carefully. For example, with the `:facebook` finder above, we may want to add an `.includes(:user)` to keep the number of DB queries down. | ||
|
||
#### Default finders when using multiple authentication | ||
|
||
You don't need to define a `resource_finder_for` callback for something registered as a `Devise.authentication_key` (e.g. `:email` or `:username`, see the [Devise wiki](https://github.com/plataformatec/devise/wiki/How-To:-Allow-users-to-sign-in-using-their-username-or-email-address#user-content-tell-devise-to-use-login-in-the-authentication_keys)), then we will call a `find_by` using that column. Consequently: | ||
|
||
~~~ruby | ||
class Users < ActiveRecord::Base | ||
# We are allowing users to authenticating with either their email or username | ||
devise :database_authenticatable, authentication_keys: [:username, :email] | ||
|
||
# Therefore, we don't need the following: | ||
# resource_finder_for :username, ->(username) { find_by(username: username) } | ||
end | ||
~~~ | ||
|
||
## OmniAuth authentication | ||
|
||
If you wish to use omniauth authentication, add all of your desired authentication provider gems to your `Gemfile`. | ||
|
@@ -211,6 +289,8 @@ Then run `bundle install`. | |
|
||
[List of oauth2 providers](https://github.com/intridea/omniauth/wiki/List-of-Strategies) | ||
|
||
Consider whether you want to allow [single or multiple](#single-vs-multiple-authentication-methods-per-resource) authentication methods per resource. | ||
|
||
## OmniAuth provider settings | ||
|
||
In `config/initializers/omniauth.rb`, add the settings for each of your providers. | ||
|
@@ -438,7 +518,7 @@ The authentication information should be included by the client in the headers o | |
"token-type": "Bearer", | ||
"client": "xxxxx", | ||
"expiry": "yyyyy", | ||
"uid": "zzzzz" | ||
"uid": "zzzzz provider" | ||
~~~ | ||
|
||
The authentication headers (each one is a seperate header) consists of the following params: | ||
|
@@ -448,7 +528,7 @@ The authentication headers (each one is a seperate header) consists of the follo | |
| **`access-token`** | This serves as the user's password for each request. A hashed version of this value is stored in the database for later comparison. This value should be changed on each request. | | ||
| **`client`** | This enables the use of multiple simultaneous sessions on different clients. (For example, a user may want to be authenticated on both their phone and their laptop at the same time.) | | ||
| **`expiry`** | The date at which the current session will expire. This can be used by clients to invalidate expired tokens without the need for an API request. | | ||
| **`uid`** | A unique value that is used to identify the user. This is necessary because searching the DB for users by their access token will make the API susceptible to [timing attacks](http://codahale.com/a-lesson-in-timing-attacks/). | | ||
| **`uid`** | A unique value that is used to identify the user, concatenated with the provider the identifier is for (e.g. `12345 facebook` or `[email protected] email`). This is necessary because searching the DB for users by their access token will make the API susceptible to [timing attacks](http://codahale.com/a-lesson-in-timing-attacks/). | | ||
|
||
The authentication headers required for each request will be available in the response from the previous request. If you are using the [ng-token-auth](https://github.com/lynndylanhurley/ng-token-auth) AngularJS module or the [jToker](https://github.com/lynndylanhurley/j-toker) jQuery plugin, this functionality is already provided. | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,8 @@ def set_user_by_token(mapping=nil) | |
if devise_warden_user && devise_warden_user.tokens[@client_id].nil? | ||
@used_auth_by_token = false | ||
@resource = devise_warden_user | ||
# REVIEW: Why are we bothering to create an auth token here? It won't | ||
# get used anywhere by the looks of it...? | ||
@resource.create_new_auth_token | ||
end | ||
end | ||
|
@@ -64,17 +66,22 @@ def set_user_by_token(mapping=nil) | |
|
||
return false unless @token | ||
|
||
# mitigate timing attacks by finding by uid instead of auth token | ||
user = uid && rc.find_by(uid: uid) | ||
# NOTE: By searching for the user by an identifier instead of by token, we | ||
# mitigate timing attacks | ||
# | ||
@provider_id, @provider = uid.split # e.g. ["12345", "facebook"] or ["[email protected]", "email"] | ||
resource = rc.find_resource(@provider_id, @provider) | ||
|
||
if user && user.valid_token?(@token, @client_id) | ||
if resource && resource.valid_token?(@token, @client_id) | ||
# REVIEW: why is this looking at :user? Shouldn't it be mapping to handle | ||
# multiple devise models such as Admin? | ||
# sign_in with bypass: true will be deprecated in the next version of Devise | ||
if self.respond_to? :bypass_sign_in | ||
bypass_sign_in(user, scope: :user) | ||
bypass_sign_in(resource, scope: rc.to_s.downcase.to_sym) | ||
else | ||
sign_in(:user, user, store: false, bypass: true) | ||
sign_in(rc.to_s.downcase.to_sym, resource, store: false, bypass: true) | ||
end | ||
return @resource = user | ||
return @resource = resource | ||
else | ||
# zero all values previously set values | ||
@client_id = nil | ||
|
@@ -94,8 +101,7 @@ def update_auth_header | |
# cleared by sign out in the meantime | ||
return if @resource.reload.tokens[@client_id].nil? | ||
|
||
auth_header = @resource.build_auth_header(@token, @client_id) | ||
|
||
auth_header = @resource.build_auth_header(@token, @client_id, @provider_id, @provider) | ||
# update the response header | ||
response.headers.merge!(auth_header) | ||
|
||
|
@@ -117,7 +123,7 @@ def update_auth_header | |
# extend expiration of batch buffer to account for the duration of | ||
# this request | ||
if @is_batch_request | ||
auth_header = @resource.extend_batch_buffer(@token, @client_id) | ||
auth_header = @resource.extend_batch_buffer(@token, @client_id, @provider_id, @provider) | ||
|
||
# Do not return token for batch requests to avoid invalidated | ||
# tokens returned to the client in case of race conditions. | ||
|
@@ -129,7 +135,10 @@ def update_auth_header | |
|
||
# update Authorization response header with new token | ||
else | ||
auth_header = @resource.create_new_auth_token(@client_id) | ||
auth_header = @resource.create_new_auth_token(@client_id, @provider_id, @provider) | ||
|
||
# update the response header | ||
response.headers.merge!(auth_header) | ||
end | ||
|
||
# update the response header | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,8 @@ def show | |
|
||
if @resource && @resource.id | ||
# create client id | ||
# | ||
# REVIEW: Why isn't this using resource_class.create_new_auth_token? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This creates a new one without considering the previous tokens |
||
client_id = SecureRandom.urlsafe_base64(nil, false) | ||
token = SecureRandom.urlsafe_base64(nil, false) | ||
token_hash = BCrypt::Password.create(token) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,15 +27,14 @@ def redirect_callbacks | |
|
||
def omniauth_success | ||
get_resource_from_auth_hash | ||
create_token_info | ||
set_token_on_resource | ||
create_auth_params | ||
@auth_params = create_token_info | ||
|
||
if resource_class.devise_modules.include?(:confirmable) | ||
# don't send confirmation email!!! | ||
@resource.skip_confirmation! | ||
end | ||
|
||
# REVIEW: Shouldn't this be 'devise_mapping' instead of :user? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed |
||
sign_in(:user, @resource, store: false, bypass: false) | ||
|
||
@resource.save! | ||
|
@@ -157,30 +156,33 @@ def set_random_password | |
end | ||
|
||
def create_token_info | ||
# create token info | ||
@client_id = SecureRandom.urlsafe_base64(nil, false) | ||
@token = SecureRandom.urlsafe_base64(nil, false) | ||
@expiry = (Time.now + @resource.token_lifespan).to_i | ||
# These need to be instance variables so that we set the auth header info | ||
# correctly | ||
@provider_id = auth_hash['uid'] | ||
@provider = auth_hash['provider'] | ||
|
||
auth_values = @resource.create_new_auth_token(nil, @provider_id, @provider).symbolize_keys | ||
@client_id = auth_values['client'] | ||
@token = auth_values['access-token'] | ||
@expiry = auth_values['expiry'] | ||
@config = omniauth_params['config_name'] | ||
end | ||
|
||
def create_auth_params | ||
@auth_params = { | ||
auth_token: @token, | ||
client_id: @client_id, | ||
uid: @resource.uid, | ||
expiry: @expiry, | ||
config: @config | ||
} | ||
@auth_params.merge!(oauth_registration: true) if @oauth_registration | ||
@auth_params | ||
end | ||
|
||
def set_token_on_resource | ||
@resource.tokens[@client_id] = { | ||
token: BCrypt::Password.create(@token), | ||
expiry: @expiry | ||
} | ||
# The #create_new_auth_token values returned here have the token set as | ||
# the "access-token" value. Unfortunately, the previous implementation | ||
# would render this attribute out as "auth_token". Which is inconsistent | ||
# and wrong, but if people are using the body of the auth response | ||
# instead of the headers, they may see failures here. Not changing at the | ||
# moment as this would therefore be a breaking change. Same goes for | ||
# client_id/client. | ||
# | ||
# TODO: Fix this so that it consistently returns this in an | ||
# "access-token" field instead of an "auth_token". | ||
auth_values[:auth_token] = auth_values.delete(:"access-token") | ||
auth_values[:client_id] = auth_values.delete(:client) | ||
|
||
auth_values.merge!(config: @config) | ||
auth_values.merge!(oauth_registration: true) if @oauth_registration | ||
auth_values | ||
end | ||
|
||
def render_data(message, data) | ||
|
@@ -229,13 +231,15 @@ def fallback_render(text) | |
end | ||
|
||
def get_resource_from_auth_hash | ||
# find or create user by provider and provider uid | ||
@resource = resource_class.where({ | ||
uid: auth_hash['uid'], | ||
provider: auth_hash['provider'] | ||
}).first_or_initialize | ||
|
||
if @resource.new_record? | ||
@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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,8 @@ def create | |
|
||
else | ||
# email auth has been bypassed, authenticate user | ||
# | ||
# REVIEW: Shouldn't this be calling resource_class.create_new_auth_token? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should refactor |
||
@client_id = SecureRandom.urlsafe_base64(nil, false) | ||
@token = SecureRandom.urlsafe_base64(nil, false) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,14 +9,12 @@ def new | |
end | ||
|
||
def create | ||
# Check | ||
field = (resource_params.keys.map(&:to_sym) & resource_class.authentication_keys).first | ||
field = resource_class.authentication_field_for(resource_params.keys.map(&:to_sym)) | ||
|
||
@resource = nil | ||
if field | ||
q_value = get_case_insensitive_field_from_resource_params(field) | ||
|
||
@resource = find_resource(field, q_value) | ||
@resource = resource_class.find_resource(resource_params[field], field) | ||
end | ||
|
||
if @resource && valid_params?(field, q_value) && ([email protected]_to?(:active_for_authentication?) || @resource.active_for_authentication?) | ||
|
@@ -26,15 +24,17 @@ def create | |
return | ||
end | ||
# create client id | ||
@client_id = SecureRandom.urlsafe_base64(nil, false) | ||
@token = SecureRandom.urlsafe_base64(nil, false) | ||
auth_values = @resource.create_new_auth_token(nil, resource_params[field], field) | ||
|
||
@resource.tokens[@client_id] = { | ||
token: BCrypt::Password.create(@token), | ||
expiry: (Time.now + @resource.token_lifespan).to_i | ||
} | ||
@resource.save | ||
# These instance variables are required when updating the auth headers | ||
# at the end of the request, see: | ||
# DeviseTokenAuth::Concerns::SetUserByToken#update_auth_header | ||
@token = auth_values["access-token"] | ||
@client_id = auth_values["client"] | ||
@provider = "email" | ||
@provider_id = @resource.email | ||
|
||
# REVIEW: Shouldn't this be a "mapping" option, rather than a :user? | ||
sign_in(:user, @resource, store: false, bypass: false) | ||
|
||
yield @resource if block_given? | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it was previously logged with devise and doesn't have a token, maybe @lynndylanhurley can give some hint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Evan-M please feel free to remove the tests for now or do what you need to do to move the PR forward!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 togit 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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!