Only allow insecure token lookup if a flag is given

lib/devise.rb

@@ -50,9 +50,10 @@ module Devise
   mattr_accessor :secret_key
   @@secret_key = nil
 
-  # Secret key used by the key generator
+  # Allow insecure token lookup. Must be used
-  mattr_accessor :token_generator
+  # temporarily just for migration.
-  @@token_generator = nil
+  mattr_accessor :allow_insecure_token_lookup
+  @@allow_insecure_tokens_lookup = false
 
   # Custom domain or key for cookies. Not set by default
   mattr_accessor :rememberable_options
@@ -260,6 +261,10 @@ module Devise
   mattr_accessor :paranoid
   @@paranoid = false
 
+  # Stores the token generator
+  mattr_accessor :token_generator
+  @@token_generator = nil
+
   # Default way to setup Devise. Run rails generate devise_install to create
   # a fresh initializer with all configuration values.
   def self.setup

lib/devise/models/confirmable.rb

@@ -278,11 +278,14 @@ module Devise
         def confirm_by_token(confirmation_token)
           original_token     = confirmation_token
           confirmation_token = Devise.token_generator.digest(self, :confirmation_token, confirmation_token)
+
           confirmable = find_or_initialize_with_error_by(:confirmation_token, confirmation_token)
-          unless confirmable.persisted?
+          if !confirmable.persisted? && Devise.allow_insecure_token_lookup
             confirmable = find_or_initialize_with_error_by(:confirmation_token, original_token)
           end
+
           confirmable.confirm! if confirmable.persisted?
+          confirmable.confirmation_token = original_token
           confirmable
         end
 

lib/devise/models/lockable.rb

@@ -165,11 +165,12 @@ module Devise
           unlock_token   = Devise.token_generator.digest(self, :unlock_token, unlock_token)
 
           lockable = find_or_initialize_with_error_by(:unlock_token, unlock_token)
-          unless lockable.persisted?
+          if !lockable.persisted? && Devise.allow_insecure_token_lookup
             lockable = find_or_initialize_with_error_by(:unlock_token, original_token)
           end
 
           lockable.unlock_access! if lockable.persisted?
+          lockable.unlock_token = original_token
           lockable
         end
 

lib/devise/models/recoverable.rb

@@ -116,7 +116,7 @@ module Devise
           reset_password_token = Devise.token_generator.digest(self, :reset_password_token, original_token)
 
           recoverable = find_or_initialize_with_error_by(:reset_password_token, reset_password_token)
-          unless recoverable.persisted?
+          if !recoverable.persisted? && Devise.allow_insecure_token_lookup
             recoverable = find_or_initialize_with_error_by(:reset_password_token, original_token)
           end
 
@@ -127,6 +127,8 @@ module Devise
               recoverable.errors.add(:reset_password_token, :expired)
             end
           end
+
+          recoverable.reset_password_token = original_token
           recoverable
         end
 

lib/devise/token_generator.rb

@@ -10,24 +10,23 @@ module Devise
     end
 
     def digest(klass, column, value)
-      value.present? && OpenSSL::HMAC.hexdigest("SHA1", key_for(klass, column), value.to_s)
+      value.present? && OpenSSL::HMAC.hexdigest("SHA1", key_for(column), value.to_s)
     end
 
     def generate(klass, column)
-      adapter = klass.to_adapter
+      key = key_for(column)
-      key     = key_for(klass, column)
 
       loop do
         raw = Devise.friendly_token
         enc = OpenSSL::HMAC.hexdigest("SHA1", key, raw)
-        break [raw, enc] unless adapter.find_first({ column => enc })
+        break [raw, enc] unless klass.to_adapter.find_first({ column => enc })
       end
     end
 
     private
     
-    def key_for(klass, column)
+    def key_for(column)
-      @key_generator.generate_key("#{klass.name} #{column}")
+      @key_generator.generate_key(column.to_s)
     end
   end
 

test/controllers/passwords_controller_test.rb

@@ -4,16 +4,15 @@ class PasswordsControllerTest < ActionController::TestCase
   tests Devise::PasswordsController
   include Devise::TestHelpers
 
-  def setup
+  setup do
     request.env["devise.mapping"] = Devise.mappings[:user]
-
     @user = create_user
-    @user.send_reset_password_instructions
+    @raw  = @user.send_reset_password_instructions
   end
 
   def put_update_with_params
     put :update, "user" => {
-      "reset_password_token" => @user.reset_password_token, "password" => "123456", "password_confirmation" => "123456"
+      "reset_password_token" => @raw, "password" => "123456", "password_confirmation" => "123456"
     }
   end
 

test/integration/confirmable_test.rb

@@ -28,9 +28,7 @@ class ConfirmationTest < ActionDispatch::IntegrationTest
 
   test 'user should receive a confirmation from a custom mailer' do
     User.any_instance.stubs(:devise_mailer).returns(Users::Mailer)
-
     resend_confirmation
-
     assert_equal ['custom@example.com'], ActionMailer::Base.deliveries.first.from
   end
 
@@ -43,7 +41,7 @@ class ConfirmationTest < ActionDispatch::IntegrationTest
   test 'user with valid confirmation token should be able to confirm an account' do
     user = create_user(:confirm => false)
     assert_not user.confirmed?
-    visit_user_confirmation_with_token(user.confirmation_token)
+    visit_user_confirmation_with_token(user.raw_confirmation_token)
 
     assert_contain 'Your account was successfully confirmed.'
     assert_current_url '/'
@@ -54,7 +52,7 @@ class ConfirmationTest < ActionDispatch::IntegrationTest
     swap Devise, :confirm_within => 3.days do
       user = create_user(:confirm => false, :confirmation_sent_at => 4.days.ago)
       assert_not user.confirmed?
-      visit_user_confirmation_with_token(user.confirmation_token)
+      visit_user_confirmation_with_token(user.raw_confirmation_token)
 
       assert_have_selector '#error_explanation'
       assert_contain /needs to be confirmed within 3 days/
@@ -66,7 +64,7 @@ class ConfirmationTest < ActionDispatch::IntegrationTest
     swap Devise, :confirm_within => 3.days do
       user = create_user(:confirm => false, :confirmation_sent_at => 2.days.ago)
       assert_not user.confirmed?
-      visit_user_confirmation_with_token(user.confirmation_token)
+      visit_user_confirmation_with_token(user.raw_confirmation_token)
 
       assert_contain 'Your account was successfully confirmed.'
       assert_current_url '/'
@@ -78,7 +76,7 @@ class ConfirmationTest < ActionDispatch::IntegrationTest
     Devise::ConfirmationsController.any_instance.stubs(:after_confirmation_path_for).returns("/?custom=1")
 
     user = create_user(:confirm => false)
-    visit_user_confirmation_with_token(user.confirmation_token)
+    visit_user_confirmation_with_token(user.raw_confirmation_token)
 
     assert_current_url "/?custom=1"
   end
@@ -87,7 +85,7 @@ class ConfirmationTest < ActionDispatch::IntegrationTest
     user = create_user(:confirm => false)
     user.confirmed_at = Time.now
     user.save
-    visit_user_confirmation_with_token(user.confirmation_token)
+    visit_user_confirmation_with_token(user.raw_confirmation_token)
 
     assert_have_selector '#error_explanation'
     assert_contain 'already confirmed'
@@ -98,7 +96,7 @@ class ConfirmationTest < ActionDispatch::IntegrationTest
     user.confirmed_at = Time.now
     user.save
 
-    visit_user_confirmation_with_token(user.confirmation_token)
+    visit_user_confirmation_with_token(user.raw_confirmation_token)
     assert_contain 'already confirmed'
 
     fill_in 'email', :with => user.email
@@ -108,14 +106,14 @@ class ConfirmationTest < ActionDispatch::IntegrationTest
 
   test 'sign in user automatically after confirming its email' do
     user = create_user(:confirm => false)
-    visit_user_confirmation_with_token(user.confirmation_token)
+    visit_user_confirmation_with_token(user.raw_confirmation_token)
 
     assert warden.authenticated?(:user)
   end
 
   test 'increases sign count when signed in through confirmation' do
     user = create_user(:confirm => false)
-    visit_user_confirmation_with_token(user.confirmation_token)
+    visit_user_confirmation_with_token(user.raw_confirmation_token)
 
     user.reload
     assert_equal 1, user.sign_in_count
@@ -175,7 +173,7 @@ class ConfirmationTest < ActionDispatch::IntegrationTest
 
   test 'confirm account with valid confirmation token in XML format should return valid response' do
     user = create_user(:confirm => false)
-    get user_confirmation_path(:confirmation_token => user.confirmation_token, :format => 'xml')
+    get user_confirmation_path(:confirmation_token => user.raw_confirmation_token, :format => 'xml')
     assert_response :success
     assert response.body.include? %(<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<user>)
   end
@@ -256,7 +254,7 @@ class ConfirmationOnChangeTest < ActionDispatch::IntegrationTest
     admin = create_admin
     admin.update_attributes(:email => 'new_test@example.com')
     assert_equal 'new_test@example.com', admin.unconfirmed_email
-    visit_admin_confirmation_with_token(admin.confirmation_token)
+    visit_admin_confirmation_with_token(admin.raw_confirmation_token)
 
     assert_contain 'Your account was successfully confirmed.'
     assert_current_url '/admin_area/home'
@@ -269,15 +267,17 @@ class ConfirmationOnChangeTest < ActionDispatch::IntegrationTest
     admin.update_attributes(:email => 'first_test@example.com')
     assert_equal 'first_test@example.com', admin.unconfirmed_email
 
-    confirmation_token = admin.confirmation_token
+    raw_confirmation_token = admin.raw_confirmation_token
+    admin = Admin.find(admin.id)
+
     admin.update_attributes(:email => 'second_test@example.com')
     assert_equal 'second_test@example.com', admin.unconfirmed_email
 
-    visit_admin_confirmation_with_token(confirmation_token)
+    visit_admin_confirmation_with_token(raw_confirmation_token)
     assert_have_selector '#error_explanation'
     assert_contain(/Confirmation token(.*)invalid/)
 
-    visit_admin_confirmation_with_token(admin.confirmation_token)
+    visit_admin_confirmation_with_token(admin.raw_confirmation_token)
     assert_contain 'Your account was successfully confirmed.'
     assert_current_url '/admin_area/home'
     assert admin.reload.confirmed?
@@ -291,7 +291,7 @@ class ConfirmationOnChangeTest < ActionDispatch::IntegrationTest
 
     create_second_admin(:email => "new_admin_test@example.com")
 
-    visit_admin_confirmation_with_token(admin.confirmation_token)
+    visit_admin_confirmation_with_token(admin.raw_confirmation_token)
     assert_have_selector '#error_explanation'
     assert_contain(/Email.*already.*taken/)
     assert admin.reload.pending_reconfirmation?

test/integration/lockable_test.rb

@@ -13,6 +13,7 @@ class LockTest < ActionDispatch::IntegrationTest
     visit new_user_session_path
     click_link "Didn't receive unlock instructions?"
 
+    Devise.stubs(:friendly_token).returns("abcdef")
     fill_in 'email', :with => user.email
     click_button 'Resend unlock instructions'
   end
@@ -22,8 +23,11 @@ class LockTest < ActionDispatch::IntegrationTest
 
     assert_template 'sessions/new'
     assert_contain 'You will receive an email with instructions about how to unlock your account in a few minutes'
+
+    mail = ActionMailer::Base.deliveries.last
     assert_equal 1, ActionMailer::Base.deliveries.size
-    assert_equal ['please-change-me@config-initializers-devise.com'], ActionMailer::Base.deliveries.first.from
+    assert_equal ['please-change-me@config-initializers-devise.com'], mail.from
+    assert_match user_unlock_path(unlock_token: 'abcdef'), mail.body.encoded
   end
 
   test 'user should receive the instructions from a custom mailer' do
@@ -75,23 +79,15 @@ class LockTest < ActionDispatch::IntegrationTest
   end
 
   test "locked user should be able to unlock account" do
-    user = create_user(:locked => true)
+    user = create_user
-    assert user.access_locked?
+    raw  = user.lock_access!
-
+    visit_user_unlock_with_token(raw)
-    visit_user_unlock_with_token(user.unlock_token)
 
     assert_current_url "/users/sign_in"
     assert_contain 'Your account has been unlocked successfully. Please sign in to continue.'
-
     assert_not user.reload.access_locked?
   end
 
-  test "redirect user to sign in page after unlocking its account" do
-    user = create_user(:locked => true)
-    visit_user_unlock_with_token(user.unlock_token)
-    assert_not warden.authenticated?(:user)
-  end
-
   test "user should not send a new e-mail if already locked" do
     user = create_user(:locked => true)
     user.failed_attempts = User.maximum_attempts + 1
@@ -153,9 +149,10 @@ class LockTest < ActionDispatch::IntegrationTest
   end
 
   test 'user with valid unlock token should be able to unlock account via XML request' do
-    user = create_user(:locked => true)
+    user = create_user()
+    raw  = user.lock_access!
     assert user.access_locked?
-    get user_unlock_path(:format => 'xml', :unlock_token => user.unlock_token)
+    get user_unlock_path(:format => 'xml', :unlock_token => raw)
     assert_response :success
     assert response.body.include? %(<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<user>)
   end

test/integration/recoverable_test.rb

@@ -14,12 +14,16 @@ class PasswordTest < ActionDispatch::IntegrationTest
 
     fill_in 'email', :with => 'user@test.com'
     yield if block_given?
+
+    Devise.stubs(:friendly_token).returns("abcdef")
     click_button 'Send me reset password instructions'
   end
 
   def reset_password(options={}, &block)
-    visit edit_user_password_path(:reset_password_token => options[:reset_password_token]) unless options[:visit] == false
+    unless options[:visit] == false
+      visit edit_user_password_path(:reset_password_token => options[:reset_password_token] || "abcdef")
       assert_response :success
+    end
 
     fill_in 'New password', :with => '987654321'
     fill_in 'Confirm new password', :with => '987654321'
@@ -45,7 +49,10 @@ class PasswordTest < ActionDispatch::IntegrationTest
     request_forgot_password do
       fill_in 'email', :with => 'foo@bar.com'
     end
-    assert_equal ['custom@example.com'], ActionMailer::Base.deliveries.last.from
+
+    mail = ActionMailer::Base.deliveries.last
+    assert_equal ['custom@example.com'], mail.from
+    assert_match edit_user_password_path(reset_password_token: 'abcdef'), mail.body.encoded
   end
 
   test 'reset password with email of different case should fail when email is NOT the list of case insensitive keys' do
@@ -146,7 +153,7 @@ class PasswordTest < ActionDispatch::IntegrationTest
   test 'not authenticated user with valid reset password token but invalid password should not be able to change his password' do
     user = create_user
     request_forgot_password
-    reset_password :reset_password_token => user.reload.reset_password_token do
+    reset_password do
       fill_in 'Confirm new password', :with => 'other_password'
     end
 
@@ -161,7 +168,7 @@ class PasswordTest < ActionDispatch::IntegrationTest
   test 'not authenticated user with valid data should be able to change his password' do
     user = create_user
     request_forgot_password
-    reset_password :reset_password_token => user.reload.reset_password_token
+    reset_password
 
     assert_current_url '/'
     assert_contain 'Your password was changed successfully. You are now signed in.'
@@ -171,14 +178,13 @@ class PasswordTest < ActionDispatch::IntegrationTest
   test 'after entering invalid data user should still be able to change his password' do
     user = create_user
     request_forgot_password
-    reset_password :reset_password_token => user.reload.reset_password_token do
+
-      fill_in 'Confirm new password', :with => 'other_password'
+    reset_password {  fill_in 'Confirm new password', :with => 'other_password' }
-    end
     assert_response :success
     assert_have_selector '#error_explanation'
     assert_not user.reload.valid_password?('987654321')
 
-    reset_password :reset_password_token => user.reload.reset_password_token, :visit => false
+    reset_password :visit => false
     assert_contain 'Your password was changed successfully.'
     assert user.reload.valid_password?('987654321')
   end
@@ -186,7 +192,7 @@ class PasswordTest < ActionDispatch::IntegrationTest
   test 'sign in user automatically after changing its password' do
     user = create_user
     request_forgot_password
-    reset_password :reset_password_token => user.reload.reset_password_token
+    reset_password
 
     assert warden.authenticated?(:user)
   end
@@ -196,7 +202,7 @@ class PasswordTest < ActionDispatch::IntegrationTest
       swap Devise, :unlock_strategy => strategy do
         user = create_user(:locked => true)
         request_forgot_password
-        reset_password :reset_password_token => user.reload.reset_password_token
+        reset_password
 
         assert_contain 'Your password was changed successfully.'
         assert_not_contain 'You are now signed in.'
@@ -210,7 +216,7 @@ class PasswordTest < ActionDispatch::IntegrationTest
     swap Devise, :unlock_strategy => :email do
       user = create_user(:locked => true)
       request_forgot_password
-      reset_password :reset_password_token => user.reload.reset_password_token
+      reset_password
 
       assert_contain 'Your password was changed successfully.'
       assert !user.reload.access_locked?
@@ -222,7 +228,7 @@ class PasswordTest < ActionDispatch::IntegrationTest
     swap Devise, :unlock_strategy => :both do
       user = create_user(:locked => true)
       request_forgot_password
-      reset_password :reset_password_token => user.reload.reset_password_token
+      reset_password
 
       assert_contain 'Your password was changed successfully.'
       assert !user.reload.access_locked?
@@ -233,7 +239,7 @@ class PasswordTest < ActionDispatch::IntegrationTest
   test 'sign in user automatically and confirm after changing its password if it\'s not confirmed' do
     user = create_user(:confirm => false)
     request_forgot_password
-    reset_password :reset_password_token => user.reload.reset_password_token
+    reset_password
 
     assert warden.authenticated?(:user)
     assert user.reload.confirmed?
@@ -265,7 +271,9 @@ class PasswordTest < ActionDispatch::IntegrationTest
   test 'change password with valid parameters in XML format should return valid response' do
     user = create_user
     request_forgot_password
-    put user_password_path(:format => 'xml'), :user => {:reset_password_token => user.reload.reset_password_token, :password => '987654321', :password_confirmation => '987654321'}
+    put user_password_path(:format => 'xml'), :user => {
+      :reset_password_token => 'abcdef', :password => '987654321', :password_confirmation => '987654321'
+    }
     assert_response :success
     assert warden.authenticated?(:user)
   end
@@ -326,7 +334,7 @@ class PasswordTest < ActionDispatch::IntegrationTest
 
     assert_equal 10, user.failed_attempts
     request_forgot_password
-    reset_password :reset_password_token => user.reload.reset_password_token
+    reset_password
 
     assert warden.authenticated?(:user)
     user.reload

test/models/confirmable_test.rb

@@ -52,15 +52,17 @@ class ConfirmableTest < ActiveSupport::TestCase
   end
 
   test 'DEPRECATED: should find and confirm a user automatically' do
+    swap Devise, allow_insecure_token_lookup: true do
       user = create_user
       confirmed_user = User.confirm_by_token(user.confirmation_token)
       assert_equal confirmed_user, user
       assert user.reload.confirmed?
     end
+  end
 
   test 'should find and confirm a user automatically based on the raw token' do
     user = create_user
-    raw  = user.instance_variable_get(:@raw_confirmation_token)
+    raw  = user.raw_confirmation_token
     confirmed_user = User.confirm_by_token(raw)
     assert_equal confirmed_user, user
     assert user.reload.confirmed?
@@ -82,7 +84,7 @@ class ConfirmableTest < ActiveSupport::TestCase
     user = create_user
     user.confirmed_at = Time.now
     user.save
-    confirmed_user = User.confirm_by_token(user.confirmation_token)
+    confirmed_user = User.confirm_by_token(user.raw_confirmation_token)
     assert confirmed_user.confirmed?
     assert_equal "was already confirmed, please try signing in", confirmed_user.errors[:email].join
   end
@@ -272,7 +274,7 @@ class ConfirmableTest < ActiveSupport::TestCase
   def confirm_user_by_token_with_confirmation_sent_at(confirmation_sent_at)
     user = create_user
     user.update_attribute(:confirmation_sent_at, confirmation_sent_at)
-    confirmed_user = User.confirm_by_token(user.confirmation_token)
+    confirmed_user = User.confirm_by_token(user.raw_confirmation_token)
     assert_equal confirmed_user, user
     user.reload.confirmed?
   end

test/models/lockable_test.rb

@@ -140,12 +140,14 @@ class LockableTest < ActiveSupport::TestCase
   end
 
   test 'DEPRECATED: should find and unlock a user automatically' do
+    swap Devise, allow_insecure_token_lookup: true do
       user = create_user
       user.lock_access!
       locked_user = User.unlock_access_by_token(user.unlock_token)
       assert_equal locked_user, user
       assert_not user.reload.access_locked?
     end
+  end
 
   test 'should find and unlock a user automatically based on raw token' do
     user = create_user

test/models/recoverable_test.rb

@@ -109,12 +109,14 @@ class RecoverableTest < ActiveSupport::TestCase
   end
 
   test 'DEPRECATED: should find a user to reset his password based on reset_password_token' do
+    swap Devise, allow_insecure_token_lookup: true do
       user = create_user
       user.send_reset_password_instructions
 
       reset_password_user = User.reset_password_by_token(:reset_password_token => user.reset_password_token)
       assert_equal reset_password_user, user
     end
+  end
 
   test 'should find a user to reset his password based on the raw token' do
     user = create_user

test/rails_app/lib/shared_admin.rb

@@ -11,4 +11,7 @@ module SharedAdmin
     validates_uniqueness_of :email, :allow_blank => true, :if => :email_changed?
   end
 
+  def raw_confirmation_token
+    @raw_confirmation_token
+  end
 end

test/rails_app/lib/shared_user.rb

@@ -12,6 +12,10 @@ module SharedUser
     extend ExtendMethods
   end
 
+  def raw_confirmation_token
+    @raw_confirmation_token
+  end
+
   module ExtendMethods
     def new_with_session(params, session)
       super.tap do |user|