From 62c879cc418c0e49a46e8ee1fd164f29572d0251 Mon Sep 17 00:00:00 2001 From: Constantijn Schepens Date: Mon, 23 Nov 2015 18:13:04 +0000 Subject: [PATCH 1/2] 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. --- README.md | 86 ++++++++++++- .../concerns/set_user_by_token.rb | 25 ++-- .../confirmations_controller.rb | 2 + .../omniauth_callbacks_controller.rb | 68 +++++----- .../devise_token_auth/passwords_controller.rb | 23 ++-- .../registrations_controller.rb | 2 + .../devise_token_auth/sessions_controller.rb | 41 ++---- app/models/devise_token_auth/concerns/user.rb | 117 ++++++++++++++++-- .../devise_token_auth_create_users.rb.erb | 4 +- test/controllers/demo_mang_controller_test.rb | 2 +- test/controllers/demo_user_controller_test.rb | 5 +- .../omniauth_callbacks_controller_test.rb | 49 +++++--- .../overrides/sessions_controller.rb | 2 +- test/dummy/app/models/facebook_user.rb | 3 + test/dummy/app/models/multi_auth_user.rb | 11 ++ test/dummy/app/models/user.rb | 2 + .../20151124151819_add_twitter_id_to_users.rb | 6 + ...vise_token_auth_create_multi_auth_users.rb | 65 ++++++++++ ...token_auth_create_create_facebook_users.rb | 9 ++ test/dummy/db/schema.rb | 44 ++++++- test/fixtures/multi_auth_users.yml | 11 ++ test/models/multi_auth_user_test.rb | 66 ++++++++++ test/models/user_test.rb | 63 ++++++++++ 23 files changed, 581 insertions(+), 125 deletions(-) create mode 100644 test/dummy/app/models/facebook_user.rb create mode 100644 test/dummy/app/models/multi_auth_user.rb create mode 100644 test/dummy/db/migrate/20151124151819_add_twitter_id_to_users.rb create mode 100644 test/dummy/db/migrate/20151124171407_devise_token_auth_create_multi_auth_users.rb create mode 100644 test/dummy/db/migrate/20151124172214_devise_token_auth_create_create_facebook_users.rb create mode 100644 test/fixtures/multi_auth_users.yml create mode 100644 test/models/multi_auth_user_test.rb diff --git a/README.md b/README.md index ddcf52014..c4e892a09 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,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) @@ -114,7 +115,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 @@ -123,6 +124,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). @@ -182,6 +184,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: bob@home.com | email | bob@home.com | +| 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`. @@ -197,6 +275,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. @@ -424,7 +504,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 consists of the following params: @@ -434,7 +514,7 @@ The authentication headers consists of the following params: | **`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 `a@b.com 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. diff --git a/app/controllers/devise_token_auth/concerns/set_user_by_token.rb b/app/controllers/devise_token_auth/concerns/set_user_by_token.rb index ebed505e8..5ef329381 100644 --- a/app/controllers/devise_token_auth/concerns/set_user_by_token.rb +++ b/app/controllers/devise_token_auth/concerns/set_user_by_token.rb @@ -37,6 +37,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 @@ -52,12 +54,17 @@ 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) - - if user && user.valid_token?(@token, @client_id) - sign_in(:user, user, store: false, bypass: true) - return @resource = user + # 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 ["bob@home.com", "email"] + resource = rc.find_resource(@provider_id, @provider) + + 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(:user, resource, store: false, bypass: true) + return @resource = resource else # zero all values previously set values @client_id = nil @@ -74,7 +81,7 @@ def update_auth_header @client_id = nil unless @used_auth_by_token if @used_auth_by_token and not DeviseTokenAuth.change_headers_on_each_request - 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) @@ -94,11 +101,11 @@ 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) # 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) diff --git a/app/controllers/devise_token_auth/confirmations_controller.rb b/app/controllers/devise_token_auth/confirmations_controller.rb index bf336e4d3..738a1ad04 100644 --- a/app/controllers/devise_token_auth/confirmations_controller.rb +++ b/app/controllers/devise_token_auth/confirmations_controller.rb @@ -5,6 +5,8 @@ def show if @resource and @resource.id # create client id + # + # REVIEW: Why isn't this using resource_class.create_new_auth_token? client_id = SecureRandom.urlsafe_base64(nil, false) token = SecureRandom.urlsafe_base64(nil, false) token_hash = BCrypt::Password.create(token) diff --git a/app/controllers/devise_token_auth/omniauth_callbacks_controller.rb b/app/controllers/devise_token_auth/omniauth_callbacks_controller.rb index 2cb044cf5..fa2bee97c 100644 --- a/app/controllers/devise_token_auth/omniauth_callbacks_controller.rb +++ b/app/controllers/devise_token_auth/omniauth_callbacks_controller.rb @@ -24,15 +24,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? 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 + DeviseTokenAuth.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 diff --git a/app/controllers/devise_token_auth/passwords_controller.rb b/app/controllers/devise_token_auth/passwords_controller.rb index 242a0032f..c496e3479 100644 --- a/app/controllers/devise_token_auth/passwords_controller.rb +++ b/app/controllers/devise_token_auth/passwords_controller.rb @@ -27,26 +27,14 @@ def create end end - # honor devise configuration for case_insensitive_keys - if resource_class.case_insensitive_keys.include?(:email) - @email = resource_params[:email].downcase - else - @email = resource_params[:email] - end - - q = "uid = ? AND provider='email'" - - # fix for mysql default case insensitivity - if ActiveRecord::Base.connection.adapter_name.downcase.starts_with? 'mysql' - q = "BINARY uid = ? AND provider='email'" - end - - @resource = resource_class.where(q, @email).first + field = resource_class.authentication_field_for(resource_params.keys.map(&:to_sym)) + @resource = resource_class.find_resource(resource_params[field], field) if field @errors = nil @error_status = 400 if @resource + @email = @resource.email yield if block_given? @resource.send_reset_password_instructions({ email: @email, @@ -61,7 +49,10 @@ def create @errors = @resource.errors end else - @errors = [I18n.t("devise_token_auth.passwords.user_not_found", email: @email)] + # TODO: The resource_params could be a "username" field depending on + # what keys the resource uses for authentication. This translation + # should be updated to reflect this. + @errors = [I18n.t("devise_token_auth.passwords.user_not_found", email: resource_params[field])] @error_status = 404 end diff --git a/app/controllers/devise_token_auth/registrations_controller.rb b/app/controllers/devise_token_auth/registrations_controller.rb index 039525258..34c2ed773 100644 --- a/app/controllers/devise_token_auth/registrations_controller.rb +++ b/app/controllers/devise_token_auth/registrations_controller.rb @@ -49,6 +49,8 @@ def create else # email auth has been bypassed, authenticate user + # + # REVIEW: Shouldn't this be calling resource_class.create_new_auth_token? @client_id = SecureRandom.urlsafe_base64(nil, false) @token = SecureRandom.urlsafe_base64(nil, false) diff --git a/app/controllers/devise_token_auth/sessions_controller.rb b/app/controllers/devise_token_auth/sessions_controller.rb index 893aecfb9..7f8cd767f 100644 --- a/app/controllers/devise_token_auth/sessions_controller.rb +++ b/app/controllers/devise_token_auth/sessions_controller.rb @@ -9,37 +9,24 @@ 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 = resource_params[field] - - if resource_class.case_insensitive_keys.include?(field) - q_value.downcase! - end - - q = "#{field.to_s} = ? AND provider='email'" - - if ActiveRecord::Base.connection.adapter_name.downcase.starts_with? 'mysql' - q = "BINARY " + q - end - - @resource = resource_class.where(q, q_value).first + @resource = resource_class.find_resource(resource_params[field], field) end - if @resource and valid_params?(field, q_value) and @resource.valid_password?(resource_params[:password]) and (!@resource.respond_to?(:active_for_authentication?) or @resource.active_for_authentication?) - # create client id - @client_id = SecureRandom.urlsafe_base64(nil, false) - @token = SecureRandom.urlsafe_base64(nil, false) + if @resource and @resource.valid_password?(resource_params[:password]) and (!@resource.respond_to?(:active_for_authentication?) or @resource.active_for_authentication?) + auth_values = @resource.create_new_auth_token(nil, resource_params[field], field) - @resource.tokens[@client_id] = { - token: BCrypt::Password.create(@token), - expiry: (Time.now + DeviseTokenAuth.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 if block_given? @@ -72,10 +59,6 @@ def destroy protected - def valid_params?(key, val) - resource_params[:password] && key && val - end - def get_auth_params auth_key = nil auth_val = nil diff --git a/app/models/devise_token_auth/concerns/user.rb b/app/models/devise_token_auth/concerns/user.rb index e8416d41e..70932eb61 100644 --- a/app/models/devise_token_auth/concerns/user.rb +++ b/app/models/devise_token_auth/concerns/user.rb @@ -15,6 +15,9 @@ def self.tokens_match?(token_hash, token) end included do + + class_variable_set(:@@finder_methods, {}) + # Hack to check if devise is already enabled unless self.method_defined?(:devise_modules) devise :database_authenticatable, :registerable, @@ -59,6 +62,14 @@ def email_changed? false end + def provider + if has_attribute?(:provider) + read_attribute(:provider) + else + self.class.authentication_keys.first.to_s + end + end + # override devise method to include additional info as opts hash def send_confirmation_instructions(opts=nil) unless @raw_confirmation_token @@ -93,8 +104,77 @@ def send_reset_password_instructions(opts=nil) end module ClassMethods - protected + # This attempts 4 different finds to try and get the resource, depending on + # how the resources have been configured and accounting for backwards + # compatibility prior to multiple authentication methods. + # + def find_resource(id, provider) + # 1. If a finder method has been registered for this provider, use it! + # + finder_method = finder_methods[provider.try(:to_sym)] + return finder_method.call(id) if finder_method + + # 2. This check is for backwards compatibility. On introducing multiple + # oauth methods, the uid header changed to include the provider. Prior + # to this change, however, the uid was only the identifier. + # Consequently, if we don't have the provider we fall back to the old + # behaviour of searching by uid. If we don't have a uid (i.e. we're + # allowing multiple auth methods) then we default to something sane. + # + if provider.nil? + field = column_names.include?("uid") ? "uid" : authentication_keys.first + return case_sensitive_find("#{field} = ?", id) + end + + id.downcase! if self.case_insensitive_keys.include?(provider.to_sym) + + # 3. We then search using {provider: provider, uid: uid} to cover the + # default behaviour which doesn't allow multiple authentication + # methods for a single resource + # + if column_names.include?("uid") && column_names.include?("provider") + resource = case_sensitive_find("uid = ? AND provider = ?", id, provider) + return resource if resource + end + + # 4. If we're at this point, we've either: + # + # A. Got someone who hasn't registered yet + # B. Are using a non-email field to identify users + # + # If A is the case, we likely won't have a column which corresponds to + # the value of "provider" (e.g. "twitter"). Consequently, bail out to + # avoid running a query selecting on a column we don't have. + # + return nil unless column_names.include?(provider.to_s) + + case_sensitive_find("#{provider} = ?", id) + end + + def case_sensitive_find(query, *args) + if ActiveRecord::Base.connection.adapter_name.downcase.starts_with? 'mysql' + query = "BINARY " + query + end + + where(query, *args).first + end + + def authentication_field_for(allowed_fields) + (allowed_fields & authentication_keys).first + end + + # These two methods must use .class_variable_get or the class variable gets + # set on this ClassMethods module, instead of the class including it + def resource_finder_for(resource, callable) + self.class_variable_get(:@@finder_methods)[resource.to_sym] = callable + end + + def finder_methods + self.class_variable_get(:@@finder_methods) + end + + protected def tokens_has_json_column_type? table_exists? && self.columns_hash['tokens'] && self.columns_hash['tokens'].type.in?([:json, :jsonb]) @@ -161,7 +241,7 @@ def token_can_be_reused?(token, client_id) # update user's auth token (should happen on each request) - def create_new_auth_token(client_id=nil) + def create_new_auth_token(client_id=nil, provider_id=nil, provider=nil) client_id ||= SecureRandom.urlsafe_base64(nil, false) last_token ||= nil token = SecureRandom.urlsafe_base64(nil, false) @@ -178,22 +258,30 @@ def create_new_auth_token(client_id=nil) last_token: last_token, updated_at: Time.now } - + max_clients = DeviseTokenAuth.max_number_of_devices while self.tokens.keys.length > 0 and max_clients < self.tokens.keys.length oldest_token = self.tokens.min_by { |cid, v| v[:expiry] || v["expiry"] } self.tokens.delete(oldest_token.first) - end + end self.save! - return build_auth_header(token, client_id) + return build_auth_header(token, client_id, provider_id, provider) end - - def build_auth_header(token, client_id='default') + def build_auth_header(token, client_id='default', provider_id, provider) client_id ||= 'default' + # If we've not been given a specific provider, intuit it. This may occur + # when logging in through standard devise (for example). See the check + # for DeviseTokenAuth.enable_standard_devise_support in: + # + # DeviseAuthToken::SetUserToken#set_user_token + # + provider = self.class.authentication_keys.first if provider.nil? + provider_id = self.send(provider) if provider_id.nil? + # client may use expiry to prevent validation request if expired # must be cast as string or headers will break expiry = self.tokens[client_id]['expiry'] || self.tokens[client_id][:expiry] @@ -203,11 +291,10 @@ def build_auth_header(token, client_id='default') "token-type" => "Bearer", "client" => client_id, "expiry" => expiry.to_s, - "uid" => self.uid + "uid" => "#{provider_id} #{provider}" } end - def build_auth_url(base_url, args) args[:uid] = self.uid args[:expiry] = self.tokens[args[:client_id]]['expiry'] @@ -216,11 +303,11 @@ def build_auth_url(base_url, args) end - def extend_batch_buffer(token, client_id) + def extend_batch_buffer(token, client_id, provider_id, provider) self.tokens[client_id]['updated_at'] = Time.now self.save! - return build_auth_header(token, client_id) + return build_auth_header(token, client_id, provider_id, provider) end def confirmed? @@ -238,7 +325,9 @@ def token_validation_response # only validate unique email among users that registered by email def unique_email_user - if provider == 'email' and self.class.where(provider: 'email', email: email).count > 0 + return true unless self.class.column_names.include?('provider') + + if self.class.where(provider: 'email', email: email).count > 0 errors.add(:email, :already_in_use) end end @@ -248,7 +337,9 @@ def set_empty_token_hash end def sync_uid - self.uid = email if provider == 'email' + if provider == 'email' && has_attribute?(:uid) + self.uid = email + end end def destroy_expired_tokens diff --git a/lib/generators/devise_token_auth/templates/devise_token_auth_create_users.rb.erb b/lib/generators/devise_token_auth/templates/devise_token_auth_create_users.rb.erb index 7179726aa..b5a5ebe8a 100644 --- a/lib/generators/devise_token_auth/templates/devise_token_auth_create_users.rb.erb +++ b/lib/generators/devise_token_auth/templates/devise_token_auth_create_users.rb.erb @@ -1,7 +1,9 @@ class DeviseTokenAuthCreate<%= user_class.pluralize %> < ActiveRecord::Migration def change create_table(:<%= user_class.pluralize.underscore %>) do |t| - ## Required + ## Required if using single method authentication per resource. See + # "Single vs Multiple authentication methods per resource" in the README + # for more information. t.string :provider, :null => false, :default => "email" t.string :uid, :null => false, :default => "" diff --git a/test/controllers/demo_mang_controller_test.rb b/test/controllers/demo_mang_controller_test.rb index 028de65a5..764b8c471 100644 --- a/test/controllers/demo_mang_controller_test.rb +++ b/test/controllers/demo_mang_controller_test.rb @@ -61,7 +61,7 @@ class DemoMangControllerTest < ActionDispatch::IntegrationTest end it "should return the user's uid in the auth header" do - assert_equal @resource.uid, @resp_uid + assert_equal "#{@resource.uid} email", @resp_uid end it 'should not treat this request as a batch request' do diff --git a/test/controllers/demo_user_controller_test.rb b/test/controllers/demo_user_controller_test.rb index d10b12a25..282e53edf 100644 --- a/test/controllers/demo_user_controller_test.rb +++ b/test/controllers/demo_user_controller_test.rb @@ -62,7 +62,7 @@ class DemoUserControllerTest < ActionDispatch::IntegrationTest end it "should return the user's uid in the auth header" do - assert_equal @resource.uid, @resp_uid + assert_equal "#{@resource.uid} email", @resp_uid end it 'should not treat this request as a batch request' do @@ -322,8 +322,7 @@ class DemoUserControllerTest < ActionDispatch::IntegrationTest it 'should not define current_mang' do refute_equal @resource, @controller.current_mang end - - + it 'should increase the number of tokens by a factor of 2 up to 11' do @first_token = @resource.tokens.keys.first diff --git a/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb b/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb index d82fda8eb..5e6c1e863 100644 --- a/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb +++ b/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb @@ -48,7 +48,7 @@ class OmniauthTest < ActionDispatch::IntegrationTest get_success client_id = controller.auth_params[:client_id] token = controller.auth_params[:auth_token] - expiry = controller.auth_params[:expiry] + expiry = controller.auth_params[:expiry].to_i # the expiry should have been set assert_equal expiry, @resource.tokens[client_id][:expiry] @@ -125,46 +125,63 @@ class OmniauthTest < ActionDispatch::IntegrationTest end describe "oauth registration attr" do - - after do - User.any_instance.unstub(:new_record?) - end - describe 'with new user' do + test 'registers the new user' do + user_count = User.count - before do - User.any_instance.expects(:new_record?).returns(true).at_least_once - end + get_via_redirect '/auth/facebook', { + auth_origin_url: @redirect_url, + omniauth_window_type: 'newWindow' + } - test 'response contains oauth_registration attr' do + assert_equal(user_count + 1, User.count) + end + test 'response contains correct attributes' do get_via_redirect '/auth/facebook', { auth_origin_url: @redirect_url, omniauth_window_type: 'newWindow' } - assert_equal true, controller.auth_params[:oauth_registration] + assert_match(/"oauth_registration":true/, response.body) + assert_match(/"email":"chongbong@aol.com"/, response.body) + assert_match(/"id":#{User.last.id}/, response.body) end end describe 'with existing user' do - before do - User.any_instance.expects(:new_record?).returns(false).at_least_once + @user = User.create!( + provider: 'facebook', + uid: '123545', + name: 'chong', + email: 'chongbong@aol.com', + password: 'somepassword', + ) end - test 'response does not contain oauth_registration attr' do + test 'does not register a new user' do + user_count = User.count get_via_redirect '/auth/facebook', { auth_origin_url: @redirect_url, omniauth_window_type: 'newWindow' } - assert_equal false, controller.auth_params.key?(:oauth_registration) + assert_equal(user_count, User.count) end - end + test 'response contains correct attributes' do + get_via_redirect '/auth/facebook', { + auth_origin_url: @redirect_url, + omniauth_window_type: 'newWindow' + } + refute_match(/"oauth_registration":true/, response.body) + assert_match(/"email":"#{@user.email}"/, response.body) + assert_match(/"id":#{@user.id}/, response.body) + end + end end describe 'using namespaces' do diff --git a/test/dummy/app/controllers/overrides/sessions_controller.rb b/test/dummy/app/controllers/overrides/sessions_controller.rb index 3be969c34..308e82a66 100644 --- a/test/dummy/app/controllers/overrides/sessions_controller.rb +++ b/test/dummy/app/controllers/overrides/sessions_controller.rb @@ -5,7 +5,7 @@ class SessionsController < DeviseTokenAuth::SessionsController def create @resource = resource_class.find_by_email(resource_params[:email]) - if @resource and valid_params?(:email, resource_params[:email]) and @resource.valid_password?(resource_params[:password]) and @resource.confirmed? + if @resource and @resource.valid_password?(resource_params[:password]) and @resource.confirmed? # create client id @client_id = SecureRandom.urlsafe_base64(nil, false) @token = SecureRandom.urlsafe_base64(nil, false) diff --git a/test/dummy/app/models/facebook_user.rb b/test/dummy/app/models/facebook_user.rb new file mode 100644 index 000000000..844e3e4aa --- /dev/null +++ b/test/dummy/app/models/facebook_user.rb @@ -0,0 +1,3 @@ +class FacebookUser < ActiveRecord::Base + has_one :user, class_name: MultiAuthUser +end diff --git a/test/dummy/app/models/multi_auth_user.rb b/test/dummy/app/models/multi_auth_user.rb new file mode 100644 index 000000000..3fda207c6 --- /dev/null +++ b/test/dummy/app/models/multi_auth_user.rb @@ -0,0 +1,11 @@ +class MultiAuthUser < ActiveRecord::Base + devise :database_authenticatable, :registerable + + include DeviseTokenAuth::Concerns::User + + resource_finder_for :twitter, ->(twitter_id) { find_by(twitter_id: twitter_id) } + resource_finder_for :facebook, ->(facebook_id) { FacebookUser.find_by(facebook_id: facebook_id).user } + + belongs_to :facebook_user + +end diff --git a/test/dummy/app/models/user.rb b/test/dummy/app/models/user.rb index 17df68f5e..7f278a1c6 100644 --- a/test/dummy/app/models/user.rb +++ b/test/dummy/app/models/user.rb @@ -1,6 +1,8 @@ class User < ActiveRecord::Base include DeviseTokenAuth::Concerns::User + resource_finder_for :twitter, ->(twitter_id) { find_by(twitter_id: twitter_id) } + validates :operating_thetan, numericality: true, allow_nil: true validate :ensure_correct_favorite_color diff --git a/test/dummy/db/migrate/20151124151819_add_twitter_id_to_users.rb b/test/dummy/db/migrate/20151124151819_add_twitter_id_to_users.rb new file mode 100644 index 000000000..b065c710f --- /dev/null +++ b/test/dummy/db/migrate/20151124151819_add_twitter_id_to_users.rb @@ -0,0 +1,6 @@ +class AddTwitterIdToUsers < ActiveRecord::Migration + def change + add_column :users, :twitter_id, :integer + add_index :users, :twitter_id + end +end diff --git a/test/dummy/db/migrate/20151124171407_devise_token_auth_create_multi_auth_users.rb b/test/dummy/db/migrate/20151124171407_devise_token_auth_create_multi_auth_users.rb new file mode 100644 index 000000000..d88dc92cf --- /dev/null +++ b/test/dummy/db/migrate/20151124171407_devise_token_auth_create_multi_auth_users.rb @@ -0,0 +1,65 @@ +include MigrationDatabaseHelper + +class DeviseTokenAuthCreateMultiAuthUsers < ActiveRecord::Migration + # This was largely copied from DeviseTokenAuthCreateUsers + + def change + create_table :multi_auth_users do |t| + ## Database authenticatable + t.string :email + t.string :encrypted_password, :null => false, :default => "" + + ## Recoverable + t.string :reset_password_token + t.datetime :reset_password_sent_at + t.string :reset_password_redirect_url + + ## Rememberable + t.datetime :remember_created_at + + ## Trackable + t.integer :sign_in_count, :default => 0, :null => false + t.datetime :current_sign_in_at + t.datetime :last_sign_in_at + t.string :current_sign_in_ip + t.string :last_sign_in_ip + + ## Confirmable + t.string :confirmation_token + t.datetime :confirmed_at + t.datetime :confirmation_sent_at + t.string :confirm_success_url + t.string :unconfirmed_email # Only if using reconfirmable + + ## Lockable + # t.integer :failed_attempts, :default => 0, :null => false # Only if lock strategy is :failed_attempts + # t.string :unlock_token # Only if unlock strategy is :email or :both + # t.datetime :locked_at + + ## User Info + t.string :name + t.string :nickname + t.string :image + + ## Identifiers used for allowing users to authenticate multiple ways + t.integer :twitter_id + t.integer :facebook_user_id + + ## Tokens + if json_supported_database? + t.json :tokens + else + t.text :tokens + end + + t.timestamps + end + + add_index :multi_auth_users, :email + add_index :multi_auth_users, :reset_password_token, :unique => true + add_index :multi_auth_users, :confirmation_token, :unique => true + add_index :multi_auth_users, :nickname, :unique => true + add_index :multi_auth_users, :twitter_id, :unique => true + add_index :multi_auth_users, :facebook_user_id, :unique => true + end +end diff --git a/test/dummy/db/migrate/20151124172214_devise_token_auth_create_create_facebook_users.rb b/test/dummy/db/migrate/20151124172214_devise_token_auth_create_create_facebook_users.rb new file mode 100644 index 000000000..95aa56fc2 --- /dev/null +++ b/test/dummy/db/migrate/20151124172214_devise_token_auth_create_create_facebook_users.rb @@ -0,0 +1,9 @@ +class DeviseTokenAuthCreateCreateFacebookUsers < ActiveRecord::Migration + def change + create_table :facebook_users do |t| + t.integer :facebook_id + end + + add_index :facebook_users, :facebook_id + end +end diff --git a/test/dummy/db/schema.rb b/test/dummy/db/schema.rb index b54f5cc68..0026b796f 100644 --- a/test/dummy/db/schema.rb +++ b/test/dummy/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20150708104536) do +ActiveRecord::Schema.define(version: 20151124172214) do create_table "evil_users", force: :cascade do |t| t.string "email" @@ -44,6 +44,12 @@ add_index "evil_users", ["reset_password_token"], name: "index_evil_users_on_reset_password_token", unique: true add_index "evil_users", ["uid", "provider"], name: "index_evil_users_on_uid_and_provider", unique: true + create_table "facebook_users", force: :cascade do |t| + t.integer "facebook_id" + end + + add_index "facebook_users", ["facebook_id"], name: "index_facebook_users_on_facebook_id" + create_table "mangs", force: :cascade do |t| t.string "email" t.string "encrypted_password", default: "", null: false @@ -77,6 +83,40 @@ add_index "mangs", ["reset_password_token"], name: "index_mangs_on_reset_password_token", unique: true add_index "mangs", ["uid", "provider"], name: "index_mangs_on_uid_and_provider", unique: true + create_table "multi_auth_users", force: :cascade do |t| + t.string "email" + t.string "encrypted_password", default: "", null: false + t.string "reset_password_token" + t.datetime "reset_password_sent_at" + t.string "reset_password_redirect_url" + t.datetime "remember_created_at" + t.integer "sign_in_count", default: 0, null: false + t.datetime "current_sign_in_at" + t.datetime "last_sign_in_at" + t.string "current_sign_in_ip" + t.string "last_sign_in_ip" + t.string "confirmation_token" + t.datetime "confirmed_at" + t.datetime "confirmation_sent_at" + t.string "confirm_success_url" + t.string "unconfirmed_email" + t.string "name" + t.string "nickname" + t.string "image" + t.integer "twitter_id" + t.integer "facebook_user_id" + t.text "tokens" + t.datetime "created_at" + t.datetime "updated_at" + end + + add_index "multi_auth_users", ["confirmation_token"], name: "index_multi_auth_users_on_confirmation_token", unique: true + add_index "multi_auth_users", ["email"], name: "index_multi_auth_users_on_email" + add_index "multi_auth_users", ["facebook_user_id"], name: "index_multi_auth_users_on_facebook_user_id", unique: true + add_index "multi_auth_users", ["nickname"], name: "index_multi_auth_users_on_nickname", unique: true + add_index "multi_auth_users", ["reset_password_token"], name: "index_multi_auth_users_on_reset_password_token", unique: true + add_index "multi_auth_users", ["twitter_id"], name: "index_multi_auth_users_on_twitter_id", unique: true + create_table "nice_users", force: :cascade do |t| t.string "provider", null: false t.string "uid", default: "", null: false @@ -203,12 +243,14 @@ t.datetime "updated_at" t.integer "operating_thetan" t.string "favorite_color" + t.integer "twitter_id" end add_index "users", ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true add_index "users", ["email"], name: "index_users_on_email" add_index "users", ["nickname"], name: "index_users_on_nickname", unique: true add_index "users", ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true + add_index "users", ["twitter_id"], name: "index_users_on_twitter_id" add_index "users", ["uid", "provider"], name: "index_users_on_uid_and_provider", unique: true end diff --git a/test/fixtures/multi_auth_users.yml b/test/fixtures/multi_auth_users.yml new file mode 100644 index 000000000..3bd58c117 --- /dev/null +++ b/test/fixtures/multi_auth_users.yml @@ -0,0 +1,11 @@ +<% timestamp = DateTime.parse(2.weeks.ago.to_s).to_time.strftime("%F %T") %> +multi_authed_user: + email: '<%= Faker::Internet.email %>' + nickname: 'stimpy' + twitter_id: '12345' + confirmed_at: '<%= timestamp %>' + created_at: '<%= timestamp %>' + updated_at: '<%= timestamp %>' + encrypted_password: <%= User.new.send(:password_digest, 'secret123') %> + + diff --git a/test/models/multi_auth_user_test.rb b/test/models/multi_auth_user_test.rb new file mode 100644 index 000000000..d8786ec52 --- /dev/null +++ b/test/models/multi_auth_user_test.rb @@ -0,0 +1,66 @@ +require 'test_helper' + +class MultiAuthUserTest < ActiveSupport::TestCase + describe MultiAuthUser do + describe '.find_resource' do + before do + @resource = multi_auth_users(:multi_authed_user) + @resource.save! + end + + test 'finding the resource with simple custom finder methods for a provider' do + found_resource = MultiAuthUser.find_resource(@resource.twitter_id, 'twitter') + assert_equal @resource, found_resource + end + + test 'finding the resource with complex custom finder methods for a provider' do + facebook_user = FacebookUser.create!(facebook_id: 98765) + @resource.update_attributes!(facebook_user: facebook_user) + + found_resource = MultiAuthUser.find_resource(facebook_user.facebook_id, 'facebook') + assert_equal @resource, found_resource + end + + test 'finding the resource successfully with no provider' do + # Searches just by default authorize key, which in this case is email + found_resource = MultiAuthUser.find_resource(@resource.email, nil) + assert_equal @resource, found_resource + end + + test 'finding the resource successfully with no custom finder method for email' do + found_resource = MultiAuthUser.find_resource(@resource.email, 'email') + assert_equal @resource, found_resource + end + + test 'finding the resource successfully with a non-email, non-oauth provider' do + found_resource = MultiAuthUser.find_resource(@resource.nickname, 'nickname') + assert_equal @resource, found_resource + end + end + + describe 'email uniqueness' do + before do + email = Faker::Internet.email + @resource = MultiAuthUser.create!( + email: email, + password: 'somepassword', + password_confirmation: 'somepassword' + ) + + @new_resource = MultiAuthUser.new( + email: @resource.email, + password: 'anotherpassword', + password_confirmation: 'anotherpassword' + ) + end + + # This is to avoid being opinionated about domain objects; who are we to + # say that emails should be unique or not? The reason for this test is + # that when provider/uid columns are in use, we are opinionated and *do* + # care. + test 'not validating uniqueness by default' do + assert(@new_resource.save) + end + end + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 0e745ce91..b92694353 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -99,5 +99,68 @@ class UserTest < ActiveSupport::TestCase assert @resource.save end end + + describe '.find_resource' do + before do + @resource = users(:confirmed_email_user) + @resource.skip_confirmation! + @resource.save! + end + + test 'finding the resource successfully with custom finder methods for a provider' do + @resource.update_attributes!(twitter_id: 98765) + found_resource = User.find_resource(@resource.twitter_id, 'twitter') + + assert_equal @resource, found_resource + end + + test 'finding the resource successfully with no provider' do + # Searches just by uid, which by default for this resource is email + found_resource = User.find_resource(@resource.email, nil) + assert_equal @resource, found_resource + end + + test 'finding the resource successfully with no custom finder method for email' do + found_resource = User.find_resource(@resource.email, 'email') + assert_equal @resource, found_resource + end + + test 'finding the resource successfully with no custom finder method for an oauth provider' do + @resource.update_attributes!(provider: 'facebook', uid: '12234567') + found_resource = User.find_resource(12234567, 'facebook') + assert_equal @resource, found_resource + end + + test 'finding the resource successfully with a non-email, non-oauth provider' do + found_resource = User.find_resource(@resource.nickname, 'nickname') + assert_equal @resource, found_resource + end + end + + describe 'email uniqueness' do + before do + email = Faker::Internet.email + @resource = User.create!( + email: email, + uid: email, + provider: 'email', + password: 'somepassword', + password_confirmation: 'somepassword' + ) + end + + test 'creating user with existing email adds an error' do + new_resource = User.new( + email: @resource.email, + uid: @resource.email, + provider: 'email', + password: 'anotherpassword', + password_confirmation: 'anotherpassword' + ) + + refute(new_resource.save) + assert_includes(new_resource.errors.keys, :email) + end + end end end From 37092a358958981316c87509a0ed64b673fd7703 Mon Sep 17 00:00:00 2001 From: Constantijn Schepens Date: Thu, 26 Nov 2015 10:44:55 +0000 Subject: [PATCH 2/2] uid is a string field, postgres did not like an integer in the test --- test/models/user_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index b92694353..ad3238bb8 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -127,7 +127,7 @@ class UserTest < ActiveSupport::TestCase test 'finding the resource successfully with no custom finder method for an oauth provider' do @resource.update_attributes!(provider: 'facebook', uid: '12234567') - found_resource = User.find_resource(12234567, 'facebook') + found_resource = User.find_resource('12234567', 'facebook') assert_equal @resource, found_resource end