From 157746a03e8a8ce4b3018474da27f9090894a42b Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Mon, 8 Feb 2016 23:54:00 -0500 Subject: [PATCH] Fix class detection in reset_otp_state_for(user) **Why**: When looking up a class with `Object.const_defined?`, `false` will be returned the first time it is called, even when the class exists in the Rails app. I think this might be due to the way Rails loads classes. **How**: Use `ActiveSupport::Inflector#constantize`, which returns the class all the time when the class exists, and throws a `NameError` when it doesn't. The only way I was able to properly test this was to create the `UserOtpSender` class as a real file in the test Rails app, and create a Devise Admin user to test the scenario where `AdminOtpSender` does not exist. I verified that with the old code, `reset_otp_state` was not being called when it should be, and that the new code makes the tests pass. --- .../hooks/two_factor_authenticatable.rb | 10 ++- .../two_factor_authenticatable_spec.rb | 61 ++++++------------- spec/rails_app/app/models/admin.rb | 6 ++ .../rails_app/app/services/user_otp_sender.rb | 9 +++ spec/rails_app/config/initializers/devise.rb | 4 +- spec/rails_app/config/routes.rb | 1 + .../20140403184646_devise_create_users.rb | 2 +- .../20160209032439_devise_create_admins.rb | 42 +++++++++++++ spec/rails_app/db/schema.rb | 20 +++++- spec/support/authenticated_model_helper.rb | 4 ++ 10 files changed, 111 insertions(+), 48 deletions(-) create mode 100644 spec/rails_app/app/models/admin.rb create mode 100644 spec/rails_app/app/services/user_otp_sender.rb create mode 100644 spec/rails_app/db/migrate/20160209032439_devise_create_admins.rb diff --git a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb index 5d19ac2..b423cca 100644 --- a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb @@ -22,11 +22,17 @@ end def reset_otp_state_for(user) klass_string = "#{user.class}OtpSender" - return unless Object.const_defined?(klass_string) + klass = class_from_string(klass_string) - klass = Object.const_get(klass_string) + return unless klass otp_sender = klass.new(user) otp_sender.reset_otp_state if otp_sender.respond_to?(:reset_otp_state) end + +def class_from_string(string) + string.constantize +rescue NameError + false +end diff --git a/spec/features/two_factor_authenticatable_spec.rb b/spec/features/two_factor_authenticatable_spec.rb index 753d69f..dd37f31 100644 --- a/spec/features/two_factor_authenticatable_spec.rb +++ b/spec/features/two_factor_authenticatable_spec.rb @@ -187,29 +187,16 @@ feature "User of two factor authentication" do describe 'signing in' do let(:user) { create_user } + let(:admin) { create_admin } scenario 'when UserOtpSender#reset_otp_state is defined' do - klass = stub_const 'UserOtpSender', Class.new - - klass.class_eval do - def reset_otp_state; end - end - - otp_sender = instance_double(UserOtpSender) - expect(UserOtpSender).to receive(:new).with(user).and_return(otp_sender) - expect(otp_sender).to receive(:reset_otp_state) - visit new_user_session_path complete_sign_in_form_for(user) + + expect(user.reload.email).to eq 'updated@example.com' end scenario 'when UserOtpSender#reset_otp_state is not defined' do - klass = stub_const 'UserOtpSender', Class.new - - klass.class_eval do - def reset_otp_state; end - end - otp_sender = instance_double(UserOtpSender) allow(otp_sender).to receive(:respond_to?).with(:reset_otp_state).and_return(false) @@ -219,44 +206,34 @@ feature "User of two factor authentication" do visit new_user_session_path complete_sign_in_form_for(user) end + + scenario 'when AdminOtpSender is not defined' do + visit new_admin_session_path + complete_sign_in_form_for(admin) + + expect(page).to have_content('Signed in successfully.') + end end describe 'signing out' do let(:user) { create_user } + let(:admin) { create_admin } scenario 'when UserOtpSender#reset_otp_state is defined' do visit new_user_session_path complete_sign_in_form_for(user) - - klass = stub_const 'UserOtpSender', Class.new - klass.class_eval do - def reset_otp_state; end - end - - otp_sender = instance_double(UserOtpSender) - - expect(UserOtpSender).to receive(:new).with(user).and_return(otp_sender) - expect(otp_sender).to receive(:reset_otp_state) - + user.update_attributes(email: 'foo@example.com') visit destroy_user_session_path - end - - scenario 'when UserOtpSender#reset_otp_state is not defined' do - visit new_user_session_path - complete_sign_in_form_for(user) - klass = stub_const 'UserOtpSender', Class.new - klass.class_eval do - def reset_otp_state; end - end - - otp_sender = instance_double(UserOtpSender) - allow(otp_sender).to receive(:respond_to?).with(:reset_otp_state).and_return(false) + expect(user.reload.email).to eq 'updated@example.com' + end - expect(UserOtpSender).to receive(:new).with(user).and_return(otp_sender) - expect(otp_sender).to_not receive(:reset_otp_state) + scenario 'when AdminOtpSender is not defined' do + visit new_admin_session_path + complete_sign_in_form_for(admin) + visit destroy_admin_session_path - visit destroy_user_session_path + expect(page).to have_content('Signed out successfully.') end end end diff --git a/spec/rails_app/app/models/admin.rb b/spec/rails_app/app/models/admin.rb new file mode 100644 index 0000000..41b494a --- /dev/null +++ b/spec/rails_app/app/models/admin.rb @@ -0,0 +1,6 @@ +class Admin < ActiveRecord::Base + # Include default devise modules. Others available are: + # :confirmable, :lockable, :timeoutable and :omniauthable + devise :database_authenticatable, :registerable, + :recoverable, :rememberable, :trackable, :validatable +end diff --git a/spec/rails_app/app/services/user_otp_sender.rb b/spec/rails_app/app/services/user_otp_sender.rb new file mode 100644 index 0000000..b20a92d --- /dev/null +++ b/spec/rails_app/app/services/user_otp_sender.rb @@ -0,0 +1,9 @@ +class UserOtpSender + def initialize(user) + @user = user + end + + def reset_otp_state + @user.update_attributes(email: 'updated@example.com') + end +end diff --git a/spec/rails_app/config/initializers/devise.rb b/spec/rails_app/config/initializers/devise.rb index 4d6b064..9083d7f 100644 --- a/spec/rails_app/config/initializers/devise.rb +++ b/spec/rails_app/config/initializers/devise.rb @@ -206,11 +206,11 @@ Devise.setup do |config| # Configure the default scope given to Warden. By default it's the first # devise role declared in your routes (usually :user). - # config.default_scope = :user + config.default_scope = :user # Set this configuration to false if you want /users/sign_out to sign out # only the current scope. By default, Devise signs out all scopes. - # config.sign_out_all_scopes = true + config.sign_out_all_scopes = false # ==> Navigation configuration # Lists the formats that should be treated as navigational. Formats like diff --git a/spec/rails_app/config/routes.rb b/spec/rails_app/config/routes.rb index 3de7336..a954635 100644 --- a/spec/rails_app/config/routes.rb +++ b/spec/rails_app/config/routes.rb @@ -1,4 +1,5 @@ Dummy::Application.routes.draw do + devise_for :admins root to: "home#index" match "/dashboard", to: "home#dashboard", as: :dashboard, via: [:get] diff --git a/spec/rails_app/db/migrate/20140403184646_devise_create_users.rb b/spec/rails_app/db/migrate/20140403184646_devise_create_users.rb index cf497c2..74adf30 100644 --- a/spec/rails_app/db/migrate/20140403184646_devise_create_users.rb +++ b/spec/rails_app/db/migrate/20140403184646_devise_create_users.rb @@ -31,7 +31,7 @@ class DeviseCreateUsers < ActiveRecord::Migration # t.datetime :locked_at - t.timestamps + t.timestamps null: false end add_index :users, :email, unique: true diff --git a/spec/rails_app/db/migrate/20160209032439_devise_create_admins.rb b/spec/rails_app/db/migrate/20160209032439_devise_create_admins.rb new file mode 100644 index 0000000..e598f42 --- /dev/null +++ b/spec/rails_app/db/migrate/20160209032439_devise_create_admins.rb @@ -0,0 +1,42 @@ +class DeviseCreateAdmins < ActiveRecord::Migration + def change + create_table(:admins) do |t| + ## Database authenticatable + t.string :email, null: false, default: "" + t.string :encrypted_password, null: false, default: "" + + ## Recoverable + t.string :reset_password_token + t.datetime :reset_password_sent_at + + ## 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 :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 + + + t.timestamps null: false + end + + add_index :admins, :email, unique: true + add_index :admins, :reset_password_token, unique: true + # add_index :admins, :confirmation_token, unique: true + # add_index :admins, :unlock_token, unique: true + end +end diff --git a/spec/rails_app/db/schema.rb b/spec/rails_app/db/schema.rb index 0c641c4..5f01161 100644 --- a/spec/rails_app/db/schema.rb +++ b/spec/rails_app/db/schema.rb @@ -11,7 +11,25 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20151228230340) do +ActiveRecord::Schema.define(version: 20160209032439) do + + create_table "admins", force: :cascade do |t| + t.string "email", default: "", null: false + t.string "encrypted_password", default: "", null: false + t.string "reset_password_token" + t.datetime "reset_password_sent_at" + 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.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + add_index "admins", ["email"], name: "index_admins_on_email", unique: true + add_index "admins", ["reset_password_token"], name: "index_admins_on_reset_password_token", unique: true create_table "users", force: :cascade do |t| t.string "email", default: "", null: false diff --git a/spec/support/authenticated_model_helper.rb b/spec/support/authenticated_model_helper.rb index 50b2c6e..38f2aae 100644 --- a/spec/support/authenticated_model_helper.rb +++ b/spec/support/authenticated_model_helper.rb @@ -9,6 +9,10 @@ module AuthenticatedModelHelper User.create!(valid_attributes(attributes)) end + def create_admin + Admin.create!(valid_attributes.except(:nickname)) + end + def valid_attributes(attributes={}) { nickname: 'Marissa',