From 27a824f078ea18ddbd3785729a7baea2d79d0452 Mon Sep 17 00:00:00 2001 From: Constantijn Schepens Date: Mon, 23 Nov 2015 18:13:04 +0000 Subject: [PATCH 1/5] 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 | 9 +- .../registrations_controller.rb | 2 + .../devise_token_auth/sessions_controller.rb | 26 ++-- app/models/devise_token_auth/concerns/user.rb | 128 ++++++++++++++++-- .../devise_token_auth_create_users.rb.erb | 4 +- test/controllers/demo_mang_controller_test.rb | 2 +- test/controllers/demo_user_controller_test.rb | 2 +- .../omniauth_callbacks_controller_test.rb | 53 ++++++-- .../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 | 59 +++++++- test/fixtures/multi_auth_users.yml | 11 ++ test/models/multi_auth_user_test.rb | 66 +++++++++ test/models/user_test.rb | 63 +++++++++ 23 files changed, 618 insertions(+), 86 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 6754a9295..2fc56c0b5 100644 --- a/README.md +++ b/README.md @@ -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: 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`. @@ -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 `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 044eb510d..34bf8966e 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 @@ -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 ["bob@home.com", "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) else sign_in(:user, user, 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 diff --git a/app/controllers/devise_token_auth/confirmations_controller.rb b/app/controllers/devise_token_auth/confirmations_controller.rb index 72777b7d3..b6ea8cef5 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 && @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 9ef844f1f..02ec1966d 100644 --- a/app/controllers/devise_token_auth/omniauth_callbacks_controller.rb +++ b/app/controllers/devise_token_auth/omniauth_callbacks_controller.rb @@ -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? 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 diff --git a/app/controllers/devise_token_auth/passwords_controller.rb b/app/controllers/devise_token_auth/passwords_controller.rb index bc7cf9faa..86638591d 100644 --- a/app/controllers/devise_token_auth/passwords_controller.rb +++ b/app/controllers/devise_token_auth/passwords_controller.rb @@ -28,8 +28,10 @@ def create end @email = get_case_insensitive_field_from_resource_params(:email) - @resource = find_resource(:uid, @email) + field = resource_class.authentication_field_for(resource_params.keys.map(&:to_sym))f + + @resource = resource_class.find_resource(resource_params[field], field) if field @errors = nil @error_status = 400 @@ -48,7 +50,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 4bed83447..bedac8dd0 100644 --- a/app/controllers/devise_token_auth/registrations_controller.rb +++ b/app/controllers/devise_token_auth/registrations_controller.rb @@ -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? @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 cebfa03c2..04877dd20 100644 --- a/app/controllers/devise_token_auth/sessions_controller.rb +++ b/app/controllers/devise_token_auth/sessions_controller.rb @@ -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) && (!@resource.respond_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? @@ -67,10 +67,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 6a5b8fde3..5da51ecc7 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 password_required? super 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 @@ -109,8 +120,77 @@ def send_unlock_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? database_exists? && table_exists? && self.columns_hash['tokens'] && self.columns_hash['tokens'].type.in?([:json, :jsonb]) @@ -181,7 +261,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) @@ -199,13 +279,29 @@ def create_new_auth_token(client_id=nil) updated_at: Time.now } - return build_auth_header(token, client_id) - end + 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 + + self.save! + 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] @@ -223,11 +319,10 @@ def build_auth_header(token, client_id='default') DeviseTokenAuth.headers_names[:"token-type"] => "Bearer", DeviseTokenAuth.headers_names[:"client"] => client_id, DeviseTokenAuth.headers_names[:"expiry"] => expiry.to_s, - DeviseTokenAuth.headers_names[:"uid"] => self.uid + DeviseTokenAuth.headers_names[:"uid"] => "#{provider_id} #{provider}" } end - def build_auth_url(base_url, args) args[:uid] = self.uid args[:expiry] = self.tokens[args[:client_id]]['expiry'] @@ -236,10 +331,10 @@ 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 - return build_auth_header(token, client_id) + return build_auth_header(token, client_id, provider_id, provider) end def confirmed? @@ -258,10 +353,25 @@ def token_lifespan protected + # only validate unique email among users that registered by email + def unique_email_user + 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 + def set_empty_token_hash self.tokens ||= {} if has_attribute?(:tokens) end + def sync_uid + if provider == 'email' && has_attribute?(:uid) + self.uid = email + end + end + def destroy_expired_tokens if self.tokens self.tokens.delete_if do |cid, v| 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 c1c02cb08..ee892b885 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<%= "[#{Rails::VERSION::STRING[0..2]}]" if Rails::VERSION::MAJOR > 4 %> 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 85b542fed..b23f079ec 100644 --- a/test/controllers/demo_mang_controller_test.rb +++ b/test/controllers/demo_mang_controller_test.rb @@ -67,7 +67,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 83c6d4872..cde160834 100644 --- a/test/controllers/demo_user_controller_test.rb +++ b/test/controllers/demo_user_controller_test.rb @@ -68,7 +68,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 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 c411c45ed..4b7343476 100644 --- a/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb +++ b/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb @@ -54,7 +54,7 @@ def get_parsed_data_json 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'] @@ -144,7 +144,7 @@ def get_parsed_data_json end end - describe 'oauth registration attr' do + describe "oauth registration attr" do after do User.any_instance.unstub(:new_record?) end @@ -153,21 +153,38 @@ def get_parsed_data_json before do User.any_instance.expects(:new_record?).returns(true).at_least_once end + test 'registers the new user' do + user_count = User.count - test 'response contains oauth_registration attr' do - get '/auth/facebook', - params: { auth_origin_url: @redirect_url, - omniauth_window_type: 'newWindow' } + get_via_redirect '/auth/facebook', { + auth_origin_url: @redirect_url, + omniauth_window_type: 'newWindow' + } - follow_all_redirects! + assert_equal(user_count + 1, User.count) + end - assert_equal true, controller.auth_params[:oauth_registration] + test 'response contains correct attributes' do + get_via_redirect '/auth/facebook', { + auth_origin_url: @redirect_url, + omniauth_window_type: 'newWindow' + } + + 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 @@ -175,9 +192,25 @@ def get_parsed_data_json params: { auth_origin_url: @redirect_url, omniauth_window_type: 'newWindow' } + end + + test 'does not register a new user' do + user_count = User.count + follow_all_redirects! - assert_equal false, controller.auth_params.key?(:oauth_registration) + assert_equal(user_count, User.count) + 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 diff --git a/test/dummy/app/controllers/overrides/sessions_controller.rb b/test/dummy/app/controllers/overrides/sessions_controller.rb index 10545a354..f5be38aa6 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 292cf3ee8..59b9ba814 100644 --- a/test/dummy/db/schema.rb +++ b/test/dummy/db/schema.rb @@ -62,6 +62,12 @@ t.index ["unlock_token"], name: "index_lockable_users_on_unlock_token", unique: true end + 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 @@ -94,6 +100,45 @@ t.index ["uid", "provider"], name: "index_mangs_on_uid_and_provider", unique: true end + add_index "mangs", ["confirmation_token"], name: "index_mangs_on_confirmation_token", unique: true + add_index "mangs", ["email"], name: "index_mangs_on_email" + 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 @@ -245,14 +290,22 @@ t.string "uid", default: "", null: false t.text "tokens" t.datetime "created_at" - t.datetime "updated_at" - t.integer "operating_thetan" - t.string "favorite_color" + t.datetime "updated_at"s t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["email"], name: "index_users_on_email" t.index ["nickname"], name: "index_users_on_nickname", unique: true t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true t.index ["uid", "provider"], name: "index_users_on_uid_and_provider", unique: true + 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 52e113833..3e232e9dc 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -155,5 +155,68 @@ def @resource.token_lifespan 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 dbf91cddacd1ebb3cfadadd043e1fc32cdace9b6 Mon Sep 17 00:00:00 2001 From: Constantijn Schepens Date: Thu, 26 Nov 2015 10:44:55 +0000 Subject: [PATCH 2/5] 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 3e232e9dc..b0912f004 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -183,7 +183,7 @@ def @resource.token_lifespan 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 From fe22f39995bd63451474d99921753c0b404bd243 Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Sun, 22 Oct 2017 17:15:15 -0400 Subject: [PATCH 3/5] Trying to fix the test suite... --- .../concerns/set_user_by_token.rb | 4 +- .../devise_token_auth/passwords_controller.rb | 2 +- .../devise_token_auth/sessions_controller.rb | 4 + .../omniauth_callbacks_controller_test.rb | 26 +++++-- .../20151124151819_add_twitter_id_to_users.rb | 2 +- ...vise_token_auth_create_multi_auth_users.rb | 5 +- ...token_auth_create_create_facebook_users.rb | 2 +- test/dummy/db/schema.rb | 77 ++++++++----------- test/test_helper.rb | 3 + 9 files changed, 68 insertions(+), 57 deletions(-) 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 34bf8966e..420e6bb4d 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 @@ -77,9 +77,9 @@ def set_user_by_token(mapping=nil) # 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 = resource else diff --git a/app/controllers/devise_token_auth/passwords_controller.rb b/app/controllers/devise_token_auth/passwords_controller.rb index 86638591d..2aae9b5b8 100644 --- a/app/controllers/devise_token_auth/passwords_controller.rb +++ b/app/controllers/devise_token_auth/passwords_controller.rb @@ -29,7 +29,7 @@ def create @email = get_case_insensitive_field_from_resource_params(:email) - field = resource_class.authentication_field_for(resource_params.keys.map(&:to_sym))f + 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 diff --git a/app/controllers/devise_token_auth/sessions_controller.rb b/app/controllers/devise_token_auth/sessions_controller.rb index 04877dd20..bc1f35c43 100644 --- a/app/controllers/devise_token_auth/sessions_controller.rb +++ b/app/controllers/devise_token_auth/sessions_controller.rb @@ -67,6 +67,10 @@ 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/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb b/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb index 4b7343476..044547206 100644 --- a/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb +++ b/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb @@ -1,5 +1,6 @@ require 'test_helper' require 'mocha/test_unit' +require 'uri' # was the web request successful? # was the user redirected to the right page? @@ -156,20 +157,24 @@ def get_parsed_data_json test 'registers the new user' do user_count = User.count - get_via_redirect '/auth/facebook', { + get '/auth/facebook', params: { auth_origin_url: @redirect_url, omniauth_window_type: 'newWindow' } + follow_redirect! + assert_equal(user_count + 1, User.count) end test 'response contains correct attributes' do - get_via_redirect '/auth/facebook', { + get '/auth/facebook', params: { auth_origin_url: @redirect_url, omniauth_window_type: 'newWindow' } + follow_redirect! + assert_match(/"oauth_registration":true/, response.body) assert_match(/"email":"chongbong@aol.com"/, response.body) assert_match(/"id":#{User.last.id}/, response.body) @@ -197,20 +202,29 @@ def get_parsed_data_json test 'does not register a new user' do user_count = User.count + get '/auth/facebook', params: { + auth_origin_url: @redirect_url, + omniauth_window_type: 'newWindow' + } + follow_all_redirects! assert_equal(user_count, User.count) end test 'response contains correct attributes' do - get_via_redirect '/auth/facebook', { + + get '/auth/facebook', params: { 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) + follow_all_redirects! + + data = get_parsed_data_json + refute_match(/"oauth_registration":true/, data['oauth_registration']) + assert_match(/"email":"#{@user.email}"/, data['email']) + assert_match(/"id":#{@user.id}/, data['id']) end end end 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 index b065c710f..c83750666 100644 --- a/test/dummy/db/migrate/20151124151819_add_twitter_id_to_users.rb +++ b/test/dummy/db/migrate/20151124151819_add_twitter_id_to_users.rb @@ -1,4 +1,4 @@ -class AddTwitterIdToUsers < ActiveRecord::Migration +class AddTwitterIdToUsers < ActiveRecord::Migration[4.2] def change add_column :users, :twitter_id, :integer add_index :users, :twitter_id 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 index d88dc92cf..3e8e91a66 100644 --- 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 @@ -1,6 +1,6 @@ include MigrationDatabaseHelper -class DeviseTokenAuthCreateMultiAuthUsers < ActiveRecord::Migration +class DeviseTokenAuthCreateMultiAuthUsers < ActiveRecord::Migration[4.2] # This was largely copied from DeviseTokenAuthCreateUsers def change @@ -45,6 +45,9 @@ def change t.integer :twitter_id t.integer :facebook_user_id + # Omniauthable + t.string :provider + ## Tokens if json_supported_database? t.json :tokens 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 index 95aa56fc2..b3daffa66 100644 --- 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 @@ -1,4 +1,4 @@ -class DeviseTokenAuthCreateCreateFacebookUsers < ActiveRecord::Migration +class DeviseTokenAuthCreateCreateFacebookUsers < ActiveRecord::Migration[4.2] def change create_table :facebook_users do |t| t.integer :facebook_id diff --git a/test/dummy/db/schema.rb b/test/dummy/db/schema.rb index 59b9ba814..290811f35 100644 --- a/test/dummy/db/schema.rb +++ b/test/dummy/db/schema.rb @@ -43,6 +43,11 @@ t.index ["uid", "provider"], name: "index_evil_users_on_uid_and_provider", unique: true end + create_table "facebook_users", force: :cascade do |t| + t.integer "facebook_id" + t.index ["facebook_id"], name: "index_facebook_users_on_facebook_id" + end + create_table "lockable_users", force: :cascade do |t| t.string "provider", null: false t.string "uid", default: "", null: false @@ -62,12 +67,6 @@ t.index ["unlock_token"], name: "index_lockable_users_on_unlock_token", unique: true end - 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 @@ -100,45 +99,39 @@ t.index ["uid", "provider"], name: "index_mangs_on_uid_and_provider", unique: true end - add_index "mangs", ["confirmation_token"], name: "index_mangs_on_confirmation_token", unique: true - add_index "mangs", ["email"], name: "index_mangs_on_email" - 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.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.string "reset_password_redirect_url" t.datetime "remember_created_at" - t.integer "sign_in_count", default: 0, null: false + 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.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.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" + t.index ["confirmation_token"], name: "index_multi_auth_users_on_confirmation_token", unique: true + t.index ["email"], name: "index_multi_auth_users_on_email" + t.index ["facebook_user_id"], name: "index_multi_auth_users_on_facebook_user_id", unique: true + t.index ["nickname"], name: "index_multi_auth_users_on_nickname", unique: true + t.index ["reset_password_token"], name: "index_multi_auth_users_on_reset_password_token", unique: true + t.index ["twitter_id"], name: "index_multi_auth_users_on_twitter_id", unique: true 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 @@ -290,22 +283,16 @@ t.string "uid", default: "", null: false t.text "tokens" t.datetime "created_at" - t.datetime "updated_at"s + t.datetime "updated_at" + t.integer "operating_thetan" + t.string "favorite_color" + t.integer "twitter_id" t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["email"], name: "index_users_on_email" t.index ["nickname"], name: "index_users_on_nickname", unique: true t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true + t.index ["twitter_id"], name: "index_users_on_twitter_id" t.index ["uid", "provider"], name: "index_users_on_uid_and_provider", unique: true - 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/test_helper.rb b/test/test_helper.rb index db413534d..7750843d5 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -66,3 +66,6 @@ class ActionController::TestCase @request.env['devise.mapping'] = Devise.mappings[:user] end end + +# Warnings are too verbose, easier to troubleshoot this way. +$VERBOSE=nil \ No newline at end of file From c997b77e4aec159c20c6592a2d63bb6ca68c0c3f Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Sun, 22 Oct 2017 17:30:49 -0400 Subject: [PATCH 4/5] More test fixing. --- .../omniauth_callbacks_controller_test.rb | 16 +++++++++++++--- ..._devise_token_auth_create_multi_auth_users.rb | 1 + 2 files changed, 14 insertions(+), 3 deletions(-) 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 044547206..05e6966c3 100644 --- a/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb +++ b/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb @@ -155,6 +155,16 @@ def get_parsed_data_json User.any_instance.expects(:new_record?).returns(true).at_least_once end test 'registers the new user' do + # setup do + OmniAuth.config.mock_auth[:facebook] = OmniAuth::AuthHash.new( + provider: 'facebook', + uid: '123545', + info: { + name: 'chong', + email: 'chongbong2@aol.com' + } + ) + # end user_count = User.count get '/auth/facebook', params: { @@ -222,9 +232,9 @@ def get_parsed_data_json follow_all_redirects! data = get_parsed_data_json - refute_match(/"oauth_registration":true/, data['oauth_registration']) - assert_match(/"email":"#{@user.email}"/, data['email']) - assert_match(/"id":#{@user.id}/, data['id']) + assert_equal(false, data['oauth_registration']) + assert_equal(@user.email, data['email']) + assert_equal(@user.id, data['id']) end 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 index 3e8e91a66..e69da972f 100644 --- 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 @@ -47,6 +47,7 @@ def change # Omniauthable t.string :provider + t.string :uid ## Tokens if json_supported_database? From ab5b84388de7f1ec45a8777fb2df4835f784b24d Mon Sep 17 00:00:00 2001 From: Zach Feldman Date: Sun, 22 Oct 2017 18:17:10 -0400 Subject: [PATCH 5/5] Continue to fix tests. --- .../omniauth_callbacks_controller_test.rb | 23 ++++++------------- test/dummy/db/schema.rb | 3 +++ test/fixtures/multi_auth_users.yml | 4 +++- test/models/multi_auth_user_test.rb | 6 +++-- 4 files changed, 17 insertions(+), 19 deletions(-) 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 05e6966c3..2cf1dae49 100644 --- a/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb +++ b/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb @@ -155,16 +155,6 @@ def get_parsed_data_json User.any_instance.expects(:new_record?).returns(true).at_least_once end test 'registers the new user' do - # setup do - OmniAuth.config.mock_auth[:facebook] = OmniAuth::AuthHash.new( - provider: 'facebook', - uid: '123545', - info: { - name: 'chong', - email: 'chongbong2@aol.com' - } - ) - # end user_count = User.count get '/auth/facebook', params: { @@ -172,7 +162,7 @@ def get_parsed_data_json omniauth_window_type: 'newWindow' } - follow_redirect! + follow_all_redirects! assert_equal(user_count + 1, User.count) end @@ -183,11 +173,12 @@ def get_parsed_data_json omniauth_window_type: 'newWindow' } - follow_redirect! + follow_all_redirects! + - assert_match(/"oauth_registration":true/, response.body) - assert_match(/"email":"chongbong@aol.com"/, response.body) - assert_match(/"id":#{User.last.id}/, response.body) + assert_equal(true, data['oauth_registration']) + assert_equal('chongbong2@aol.com', data['email']) + assert_equal(@user.last.id, data['id']) end end @@ -232,7 +223,7 @@ def get_parsed_data_json follow_all_redirects! data = get_parsed_data_json - assert_equal(false, data['oauth_registration']) + assert_equal(nil, data['oauth_registration']) assert_equal(@user.email, data['email']) assert_equal(@user.id, data['id']) end diff --git a/test/dummy/db/schema.rb b/test/dummy/db/schema.rb index 290811f35..035f929ab 100644 --- a/test/dummy/db/schema.rb +++ b/test/dummy/db/schema.rb @@ -124,6 +124,9 @@ t.text "tokens" t.datetime "created_at" t.datetime "updated_at" + t.string "email" + t.string "provider" + t.string "uid" t.index ["confirmation_token"], name: "index_multi_auth_users_on_confirmation_token", unique: true t.index ["email"], name: "index_multi_auth_users_on_email" t.index ["facebook_user_id"], name: "index_multi_auth_users_on_facebook_user_id", unique: true diff --git a/test/fixtures/multi_auth_users.yml b/test/fixtures/multi_auth_users.yml index 3bd58c117..e02f80fdb 100644 --- a/test/fixtures/multi_auth_users.yml +++ b/test/fixtures/multi_auth_users.yml @@ -1,11 +1,13 @@ <% timestamp = DateTime.parse(2.weeks.ago.to_s).to_time.strftime("%F %T") %> +<% email = Faker::Internet.email %> multi_authed_user: - email: '<%= Faker::Internet.email %>' + email: '<%=email%>' nickname: 'stimpy' twitter_id: '12345' confirmed_at: '<%= timestamp %>' created_at: '<%= timestamp %>' updated_at: '<%= timestamp %>' encrypted_password: <%= User.new.send(:password_digest, 'secret123') %> + uid: '<%=email%>' diff --git a/test/models/multi_auth_user_test.rb b/test/models/multi_auth_user_test.rb index d8786ec52..09ab22031 100644 --- a/test/models/multi_auth_user_test.rb +++ b/test/models/multi_auth_user_test.rb @@ -44,13 +44,15 @@ class MultiAuthUserTest < ActiveSupport::TestCase @resource = MultiAuthUser.create!( email: email, password: 'somepassword', - password_confirmation: 'somepassword' + password_confirmation: 'somepassword', + uid: email ) @new_resource = MultiAuthUser.new( email: @resource.email, password: 'anotherpassword', - password_confirmation: 'anotherpassword' + password_confirmation: 'anotherpassword', + uid: email ) end