Merge pull request #6215 from erichmenge/fix_has_secure_password

Fix has secure password

5 files changed, 43 insertions, 15 deletions

diff --git a/activemodel/lib/active_model/secure_password.rb b/activemodel/lib/active_model/secure_password.rb
index 8711b24124..3eab745c89 100644
--- a/activemodel/lib/active_model/secure_password.rb
+++ b/activemodel/lib/active_model/secure_password.rb
@@ -6,8 +6,9 @@ module ActiveModel
       # Adds methods to set and authenticate against a BCrypt password.
       # This mechanism requires you to have a password_digest attribute.
       #
-      # Validations for presence of password, confirmation of password (using
+      # Validations for presence of password on create, confirmation of password (using
       # a "password_confirmation" attribute) are automatically added.
+      # If you wish to turn off validations, pass 'validations: false' as an argument.
       # You can add more validations by hand if need be.
       #
       # You need to add bcrypt-ruby (~> 3.0.0) to Gemfile to use has_secure_password:
@@ -31,16 +32,20 @@ module ActiveModel
       #   user.authenticate("mUc3m00RsqyRe")                             # => user
       #   User.find_by_name("david").try(:authenticate, "notright")      # => false
       #   User.find_by_name("david").try(:authenticate, "mUc3m00RsqyRe") # => user
-      def has_secure_password
+      def has_secure_password(options = {})
         # Load bcrypt-ruby only when has_secure_password is used.
         # This is to avoid ActiveModel (and by extension the entire framework) being dependent on a binary library.
         gem 'bcrypt-ruby', '~> 3.0.0'
         require 'bcrypt'
 
         attr_reader :password
-
-        validates_confirmation_of :password
-        validates_presence_of     :password_digest
+        
+        if options.fetch(:validations, true)
+          validates_confirmation_of :password
+          validates_presence_of     :password, :on => :create
+        end
+        
+        before_create { raise "Password digest missing on new record" if password_digest.blank? }
 
         include InstanceMethodsOnActivation
 
diff --git a/activemodel/test/cases/secure_password_test.rb b/activemodel/test/cases/secure_password_test.rb
index c451cc1aca..5f18909301 100644
--- a/activemodel/test/cases/secure_password_test.rb
+++ b/activemodel/test/cases/secure_password_test.rb
@@ -7,16 +7,19 @@ class SecurePasswordTest < ActiveModel::TestCase
 
   setup do
     @user = User.new
+    @visitor = Visitor.new
   end
 
   test "blank password" do
-    @user.password = ''
-    assert !@user.valid?, 'user should be invalid'
+    @user.password = @visitor.password = ''
+    assert !@user.valid?(:create), 'user should be invalid'
+    assert @visitor.valid?(:create), 'visitor should be valid'
   end
 
   test "nil password" do
-    @user.password = nil
-    assert !@user.valid?, 'user should be invalid'
+    @user.password = @visitor.password = nil
+    assert !@user.valid?(:create), 'user should be invalid'
+    assert @visitor.valid?(:create), 'visitor should be valid'
   end
 
   test "blank password doesn't override previous password" do
@@ -26,15 +29,16 @@ class SecurePasswordTest < ActiveModel::TestCase
   end
 
   test "password must be present" do
-    assert !@user.valid?
+    assert !@user.valid?(:create)
     assert_equal 1, @user.errors.size
   end
 
-  test "password must match confirmation" do
-    @user.password = "thiswillberight"
-    @user.password_confirmation = "wrong"
+  test "match confirmation" do
+    @user.password = @visitor.password = "thiswillberight"
+    @user.password_confirmation = @visitor.password_confirmation = "wrong"
 
     assert !@user.valid?
+    assert @visitor.valid?
 
     @user.password_confirmation = "thiswillberight"
 
@@ -59,4 +63,14 @@ class SecurePasswordTest < ActiveModel::TestCase
     assert !active_authorizer.include?(:password_digest)
     assert active_authorizer.include?(:name)
   end
+  
+  test "User should not be created with blank digest" do
+    assert_raise RuntimeError do 
+      @user.run_callbacks :create
+    end
+    @user.password = "supersecretpassword"
+    assert_nothing_raised do
+      @user.run_callbacks :create
+    end
+  end
 end
diff --git a/activemodel/test/models/administrator.rb b/activemodel/test/models/administrator.rb
index a48f8b064f..2d6d34b3e2 100644
--- a/activemodel/test/models/administrator.rb
+++ b/activemodel/test/models/administrator.rb
@@ -1,7 +1,10 @@
 class Administrator
+  extend ActiveModel::Callbacks
   include ActiveModel::Validations
   include ActiveModel::SecurePassword
   include ActiveModel::MassAssignmentSecurity
+  
+  define_model_callbacks :create
 
   attr_accessor :name, :password_digest
   attr_accessible :name
diff --git a/activemodel/test/models/user.rb b/activemodel/test/models/user.rb
index e221bb8091..4b11df12bf 100644
--- a/activemodel/test/models/user.rb
+++ b/activemodel/test/models/user.rb
@@ -1,6 +1,9 @@
 class User
+  extend ActiveModel::Callbacks
   include ActiveModel::Validations
   include ActiveModel::SecurePassword
+  
+  define_model_callbacks :create
 
   has_secure_password
 
diff --git a/activemodel/test/models/visitor.rb b/activemodel/test/models/visitor.rb
index 36c0a16688..d15f448516 100644
--- a/activemodel/test/models/visitor.rb
+++ b/activemodel/test/models/visitor.rb
@@ -1,9 +1,12 @@
 class Visitor
+  extend ActiveModel::Callbacks
   include ActiveModel::Validations
   include ActiveModel::SecurePassword
   include ActiveModel::MassAssignmentSecurity
+  
+  define_model_callbacks :create
 
-  has_secure_password
+  has_secure_password(validations: false)
 
-  attr_accessor :password_digest
+  attr_accessor :password_digest, :password_confirmation
 end