diff --git a/lib/devise.rb b/lib/devise.rb index 595257d3..647f746e 100644 --- a/lib/devise.rb +++ b/lib/devise.rb @@ -50,9 +50,10 @@ module Devise mattr_accessor :secret_key @@secret_key = nil - # Secret key used by the key generator - mattr_accessor :token_generator - @@token_generator = nil + # Allow insecure token lookup. Must be used + # temporarily just for migration. + 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 diff --git a/lib/devise/models/confirmable.rb b/lib/devise/models/confirmable.rb index ff029253..4442d29c 100644 --- a/lib/devise/models/confirmable.rb +++ b/lib/devise/models/confirmable.rb @@ -276,13 +276,16 @@ module Devise # If the user is already confirmed, create an error for the user # Options must have the confirmation_token def confirm_by_token(confirmation_token) - original_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 diff --git a/lib/devise/models/lockable.rb b/lib/devise/models/lockable.rb index 4dd5f821..a9c8ad5a 100644 --- a/lib/devise/models/lockable.rb +++ b/lib/devise/models/lockable.rb @@ -162,14 +162,15 @@ module Devise # Options must have the unlock_token def unlock_access_by_token(unlock_token) original_token = unlock_token - unlock_token = Devise.token_generator.digest(self, :unlock_token, unlock_token) + 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 diff --git a/lib/devise/models/recoverable.rb b/lib/devise/models/recoverable.rb index 855d3b65..b063603e 100644 --- a/lib/devise/models/recoverable.rb +++ b/lib/devise/models/recoverable.rb @@ -112,11 +112,11 @@ module Devise # containing an error in reset_password_token attribute. # Attributes must contain reset_password_token, password and confirmation def reset_password_by_token(attributes={}) - original_token = attributes[:reset_password_token] + original_token = attributes[:reset_password_token] 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 diff --git a/lib/devise/token_generator.rb b/lib/devise/token_generator.rb index 4608cc3e..3f41ec8a 100644 --- a/lib/devise/token_generator.rb +++ b/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(klass, column) + key = key_for(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) - @key_generator.generate_key("#{klass.name} #{column}") + def key_for(column) + @key_generator.generate_key(column.to_s) end end diff --git a/test/controllers/passwords_controller_test.rb b/test/controllers/passwords_controller_test.rb index 4d3dfc0b..a7fa06d6 100644 --- a/test/controllers/passwords_controller_test.rb +++ b/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 diff --git a/test/integration/confirmable_test.rb b/test/integration/confirmable_test.rb index 77787329..db09a9fb 100644 --- a/test/integration/confirmable_test.rb +++ b/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? %(\n) 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? diff --git a/test/integration/lockable_test.rb b/test/integration/lockable_test.rb index 1a5d9997..3ab7fd60 100644 --- a/test/integration/lockable_test.rb +++ b/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) - assert user.access_locked? - - visit_user_unlock_with_token(user.unlock_token) + user = create_user + raw = user.lock_access! + visit_user_unlock_with_token(raw) 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? %(\n) end diff --git a/test/integration/recoverable_test.rb b/test/integration/recoverable_test.rb index 2ed0bafa..4b6d495c 100644 --- a/test/integration/recoverable_test.rb +++ b/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 - assert_response :success + 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' - end + + reset_password { fill_in 'Confirm new password', :with => 'other_password' } 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 diff --git a/test/models/confirmable_test.rb b/test/models/confirmable_test.rb index 7e4b1c82..0e82e7d2 100644 --- a/test/models/confirmable_test.rb +++ b/test/models/confirmable_test.rb @@ -52,15 +52,17 @@ class ConfirmableTest < ActiveSupport::TestCase end test 'DEPRECATED: should find and confirm a user automatically' do - user = create_user - confirmed_user = User.confirm_by_token(user.confirmation_token) - assert_equal confirmed_user, user - assert user.reload.confirmed? + 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 diff --git a/test/models/lockable_test.rb b/test/models/lockable_test.rb index 32211f3e..a399849a 100644 --- a/test/models/lockable_test.rb +++ b/test/models/lockable_test.rb @@ -140,11 +140,13 @@ class LockableTest < ActiveSupport::TestCase end test 'DEPRECATED: should find and unlock a user automatically' 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? + 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 diff --git a/test/models/recoverable_test.rb b/test/models/recoverable_test.rb index 701a4986..3fb42d0a 100644 --- a/test/models/recoverable_test.rb +++ b/test/models/recoverable_test.rb @@ -109,11 +109,13 @@ class RecoverableTest < ActiveSupport::TestCase end test 'DEPRECATED: should find a user to reset his password based on reset_password_token' do - user = create_user - user.send_reset_password_instructions + 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 + 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 diff --git a/test/rails_app/lib/shared_admin.rb b/test/rails_app/lib/shared_admin.rb index 5b38341e..0bb40a4b 100644 --- a/test/rails_app/lib/shared_admin.rb +++ b/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 diff --git a/test/rails_app/lib/shared_user.rb b/test/rails_app/lib/shared_user.rb index e4bd8712..9d7b34ef 100644 --- a/test/rails_app/lib/shared_user.rb +++ b/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|