From 7d14f0bbb925d962eb66cb2b88d2ba7b7bdfda70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= <jose.valim@gmail.com> Date: Mon, 29 Mar 2010 23:44:47 +0200 Subject: [PATCH] Allow several authentications to share a common path. --- Gemfile | 2 +- lib/devise/models/authenticatable.rb | 6 +++ lib/devise/models/database_authenticatable.rb | 28 +++-------- lib/devise/models/lockable.rb | 20 ++++++-- lib/devise/strategies/authenticatable.rb | 15 ++++++ lib/devise/strategies/base.rb | 8 ++-- .../strategies/database_authenticatable.rb | 20 ++------ test/integration/lockable_test.rb | 2 +- test/models/confirmable_test.rb | 7 --- test/models/lockable_test.rb | 47 +++++-------------- 10 files changed, 65 insertions(+), 90 deletions(-) diff --git a/Gemfile b/Gemfile index b54e7495..9ae55679 100644 --- a/Gemfile +++ b/Gemfile @@ -1,7 +1,7 @@ source "http://gemcutter.org" # Need to install Rails from source -gem "rails", :git => "git://github.com/rails/rails.git" +gem "rails", "3.0.0.beta1"#:git => "git://github.com/rails/rails.git" gem "warden", "0.10.2" gem "sqlite3-ruby", :require => "sqlite3" gem "webrat", "0.7" diff --git a/lib/devise/models/authenticatable.rb b/lib/devise/models/authenticatable.rb index 3a5045bd..7a2cae9b 100644 --- a/lib/devise/models/authenticatable.rb +++ b/lib/devise/models/authenticatable.rb @@ -14,6 +14,12 @@ module Devise module Authenticatable extend ActiveSupport::Concern + # Yields the given block. This method is overwritten by other modules to provide + # hooks around authentication. + def valid_for_authentication? + yield + end + module ClassMethods Devise::Models.config(self, :authentication_keys, :http_authenticatable) diff --git a/lib/devise/models/database_authenticatable.rb b/lib/devise/models/database_authenticatable.rb index 7cf8fe91..4a662b15 100644 --- a/lib/devise/models/database_authenticatable.rb +++ b/lib/devise/models/database_authenticatable.rb @@ -22,15 +22,13 @@ module Devise # # Examples: # - # User.authenticate('email@test.com', 'password123') # returns authenticated user or nil # User.find(1).valid_password?('password123') # returns true/false # module DatabaseAuthenticatable - extend ActiveSupport::Concern + extend ActiveSupport::Concern + include Devise::Models::Authenticatable included do - include Devise::Models::Authenticatable - attr_reader :password, :current_password attr_accessor :password_confirmation end @@ -51,11 +49,6 @@ module Devise password_digest(incoming_password) == self.encrypted_password end - # Checks if a resource is valid upon authentication. - def valid_for_authentication?(attributes) - valid_password?(attributes[:password]) - end - # Set password and password confirmation to nil def clean_up_passwords self.password = self.password_confirmation = nil @@ -82,23 +75,16 @@ module Devise result end - protected + protected - # Digests the password using the configured encryptor. - def password_digest(password) - self.class.encryptor_class.digest(password, self.class.stretches, self.password_salt, self.class.pepper) - end + # Digests the password using the configured encryptor. + def password_digest(password) + self.class.encryptor_class.digest(password, self.class.stretches, self.password_salt, self.class.pepper) + end module ClassMethods Devise::Models.config(self, :pepper, :stretches, :encryptor) - # Authenticate a user based on configured attribute keys. Returns the - # authenticated user if it's valid or nil. - def authenticate(conditions) - resource = find_for_database_authentication(conditions.except(:password)) - resource if resource.try(:valid_for_authentication?, conditions) - end - # Returns the class for the configured encryptor. def encryptor_class @encryptor_class ||= ::Devise::Encryptors.const_get(encryptor.to_s.classify) diff --git a/lib/devise/models/lockable.rb b/lib/devise/models/lockable.rb index 586780f0..5da4fbfc 100644 --- a/lib/devise/models/lockable.rb +++ b/lib/devise/models/lockable.rb @@ -18,12 +18,11 @@ module Devise # available when unlock_strategy is :time or :both. # module Lockable - extend ActiveSupport::Concern + extend ActiveSupport::Concern include Devise::Models::Activatable # Lock an user setting it's locked_at to actual time. def lock_access! - return true if access_locked? self.locked_at = Time.now if self.class.unlock_strategy_enabled?(:email) @@ -35,6 +34,7 @@ module Devise end # Unlock an user by cleaning locket_at and failed_attempts. + # TODO Check if unlock_token is available. def unlock_access! if_access_locked do self.locked_at = nil @@ -74,19 +74,31 @@ module Devise # Overwrites valid_for_authentication? from Devise::Models::Authenticatable # for verifying whether an user is allowed to sign in or not. If the user # is locked, it should never be allowed. - def valid_for_authentication?(attributes) + def valid_for_authentication? + return :locked if access_locked? + return super unless persisted? + if result = super self.failed_attempts = 0 else self.failed_attempts += 1 - lock_access! if failed_attempts > self.class.maximum_attempts + + if attempts_exceeded? + lock_access! + return :locked + end end + save(:validate => false) if changed? result end protected + def attempts_exceeded? + self.failed_attempts > self.class.maximum_attempts + end + # Generates unlock token def generate_unlock_token self.unlock_token = self.class.unlock_token diff --git a/lib/devise/strategies/authenticatable.rb b/lib/devise/strategies/authenticatable.rb index 1a4cd12b..b47b937d 100644 --- a/lib/devise/strategies/authenticatable.rb +++ b/lib/devise/strategies/authenticatable.rb @@ -2,6 +2,9 @@ require 'devise/strategies/base' module Devise module Strategies + # This strategy should be used as basis for authentication strategies. It retrieves + # parameters both from params or from http authorization headers. See database_authenticatable + # for an example. class Authenticatable < Base attr_accessor :authentication_hash, :password @@ -11,6 +14,18 @@ module Devise private + # Simply invokes valid_for_authentication? with the given block and deal with the result. + def validate(resource, &block) + result = resource && resource.valid_for_authentication?(&block) + + case result + when Symbol, String + fail!(result) + else + result + end + end + def valid_for_http_auth? mapping.to.http_authenticatable? && request.authorization && set_http_auth_hash end diff --git a/lib/devise/strategies/base.rb b/lib/devise/strategies/base.rb index 0944e5d9..28e5baad 100644 --- a/lib/devise/strategies/base.rb +++ b/lib/devise/strategies/base.rb @@ -12,10 +12,10 @@ module Devise end end - # TODO Move to a module - def success!(record) - if record.respond_to?(:active?) && !record.active? - fail!(record.inactive_message) + # Check if the resource is active before signing him in once and for all. + def success!(resource) + if resource.respond_to?(:active?) && !resource.active? + fail!(resource.inactive_message) else super end diff --git a/lib/devise/strategies/database_authenticatable.rb b/lib/devise/strategies/database_authenticatable.rb index 41b18dd3..ce9619a5 100644 --- a/lib/devise/strategies/database_authenticatable.rb +++ b/lib/devise/strategies/database_authenticatable.rb @@ -2,29 +2,17 @@ require 'devise/strategies/authenticatable' module Devise module Strategies - # Default strategy for signing in a user, based on his email and password. - # Redirects to sign_in page if it's not authenticated + # Default strategy for signing in a user, based on his email and password in the database. class DatabaseAuthenticatable < Authenticatable - # Authenticate a user based on email and password params, returning to warden - # success and the authenticated user if everything is okay. Otherwise redirect - # to sign in page. def authenticate! - if resource = mapping.to.authenticate(authentication_hash.merge(:password => password)) + resource = mapping.to.find_for_database_authentication(authentication_hash) + + if validate(resource){ resource.valid_password?(password) } success!(resource) else fail(:invalid) end end - - protected - - def valid_controller? - mapping.controllers[:sessions] == params[:controller] - end - - def valid_params? - params[scope] && params[scope][:password].present? - end end end end diff --git a/test/integration/lockable_test.rb b/test/integration/lockable_test.rb index 69b18291..45f438cb 100644 --- a/test/integration/lockable_test.rb +++ b/test/integration/lockable_test.rb @@ -85,7 +85,7 @@ class LockTest < ActionController::IntegrationTest ActionMailer::Base.deliveries.clear sign_in_as_user(:password => "invalid") - assert_contain 'Invalid email or password.' + assert_contain 'Your account is locked.' assert ActionMailer::Base.deliveries.empty? end diff --git a/test/models/confirmable_test.rb b/test/models/confirmable_test.rb index d0409cef..27243a2d 100644 --- a/test/models/confirmable_test.rb +++ b/test/models/confirmable_test.rb @@ -79,13 +79,6 @@ class ConfirmableTest < ActiveSupport::TestCase assert_equal "was already confirmed", confirmed_user.errors[:email].join end - test 'should authenticate a confirmed user' do - user = create_user - user.confirm! - authenticated_user = User.authenticate(:email => user.email, :password => user.password) - assert_equal authenticated_user, user - end - test 'should send confirmation instructions by email' do assert_email_sent do create_user diff --git a/test/models/lockable_test.rb b/test/models/lockable_test.rb index bc219ff4..c6ed3b71 100644 --- a/test/models/lockable_test.rb +++ b/test/models/lockable_test.rb @@ -1,42 +1,33 @@ require 'test_helper' class LockableTest < ActiveSupport::TestCase - def setup setup_mailer end - test "should increment failed attempts on unsuccessful authentication" do - user = create_user - assert_equal 0, user.failed_attempts - - authenticated_user = User.authenticate(:email => user.email, :password => "anotherpassword") - assert_equal 1, user.reload.failed_attempts - end - - test "should lock account base on maximum_attempts" do - user = create_user - attempts = Devise.maximum_attempts + 1 - attempts.times { authenticated_user = User.authenticate(:email => user.email, :password => "anotherpassword") } - assert user.reload.access_locked? - end - test "should respect maximum attempts configuration" do user = create_user swap Devise, :maximum_attempts => 2 do - 3.times { authenticated_user = User.authenticate(:email => user.email, :password => "anotherpassword") } + 3.times { user.valid_for_authentication?{ false } } assert user.reload.access_locked? end end - test "should clear failed_attempts on successfull sign in" do + test "should clear failed_attempts on successfull validation" do user = create_user - User.authenticate(:email => user.email, :password => "anotherpassword") + user.valid_for_authentication?{ false } assert_equal 1, user.reload.failed_attempts - User.authenticate(:email => user.email, :password => "123456") + user.valid_for_authentication?{ true } assert_equal 0, user.reload.failed_attempts end + test 'should be valid for authentication with a unlocked user' do + user = create_user + user.lock_access! + user.unlock_access! + assert user.valid_for_authentication?{ true } + end + test "should verify whether a user is locked or not" do user = create_user assert_not user.access_locked? @@ -64,14 +55,6 @@ class LockableTest < ActiveSupport::TestCase assert 0, user.reload.failed_attempts end - test "should not lock a locked account" do - user = create_user - user.lock_access! - assert_no_difference "ActionMailer::Base.deliveries.size" do - user.lock_access! - end - end - test 'should not unlock an unlocked user' do user = create_user assert_not user.unlock_access! @@ -166,14 +149,6 @@ class LockableTest < ActiveSupport::TestCase assert_equal "can't be blank", locked_user.errors[:unlock_token].join end - test 'should authenticate a unlocked user' do - user = create_user - user.lock_access! - user.unlock_access! - authenticated_user = User.authenticate(:email => user.email, :password => user.password) - assert_equal authenticated_user, user - end - test 'should find a user to send unlock instructions' do user = create_user user.lock_access!