Merge pull request #9545 from senny/9535_secure_password_blank

`has_secure_password` is not invalid when assigning empty Strings

5 files changed, 75 insertions, 3 deletions

diff --git a/activemodel/CHANGELOG.md b/activemodel/CHANGELOG.md
index 1fe6dbd4d9..8c54ec3d45 100644
--- a/activemodel/CHANGELOG.md
+++ b/activemodel/CHANGELOG.md
@@ -1,5 +1,30 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   `has_secure_password` does not fail the confirmation validation
+    when assigning empty String to `password` and `password_confirmation`.
+
+    Example:
+
+        # given User has_secure_password.
+        @user.password = ""
+        @user.password_confirmation = ""
+        @user.valid?(:update) # used to be false
+
+*   `validates_confirmation_of` does not override writer methods for
+    the confirmation attribute if no reader is defined.
+
+    Example:
+
+        class Blog
+            def title=(new_title)
+              @title = new_title.downcase
+            end
+
+            # previously this would override the setter above.
+            validates_confirmation_of :title
+        end
+
+    *Yves Senn*
 
 ## Rails 4.0.0.beta1 (February 25, 2013) ##
 
diff --git a/activemodel/lib/active_model/secure_password.rb b/activemodel/lib/active_model/secure_password.rb
index 6644b60609..9324a1ad0a 100644
--- a/activemodel/lib/active_model/secure_password.rb
+++ b/activemodel/lib/active_model/secure_password.rb
@@ -48,6 +48,8 @@ module ActiveModel
 
         attr_reader :password
 
+        include InstanceMethodsOnActivation
+
         if options.fetch(:validations, true)
           validates_confirmation_of :password
           validates_presence_of     :password, :on => :create
@@ -55,8 +57,6 @@ module ActiveModel
           before_create { raise "Password digest missing on new record" if password_digest.blank? }
         end
 
-        include InstanceMethodsOnActivation
-
         if respond_to?(:attributes_protected_by_default)
           def self.attributes_protected_by_default #:nodoc:
             super + ['password_digest']
@@ -99,6 +99,12 @@ module ActiveModel
           self.password_digest = BCrypt::Password.create(unencrypted_password, cost: cost)
         end
       end
+
+      def password_confirmation=(unencrypted_password)
+        unless unencrypted_password.blank?
+          @password_confirmation = unencrypted_password
+        end
+      end
     end
   end
 end
diff --git a/activemodel/lib/active_model/validations/confirmation.rb b/activemodel/lib/active_model/validations/confirmation.rb
index 3a3abce364..d14fb4dc53 100644
--- a/activemodel/lib/active_model/validations/confirmation.rb
+++ b/activemodel/lib/active_model/validations/confirmation.rb
@@ -10,9 +10,13 @@ module ActiveModel
       end
 
       def setup(klass)
-        klass.send(:attr_accessor, *attributes.map do |attribute|
+        klass.send(:attr_reader, *attributes.map do |attribute|
           :"#{attribute}_confirmation" unless klass.method_defined?(:"#{attribute}_confirmation")
         end.compact)
+
+        klass.send(:attr_writer, *attributes.map do |attribute|
+          :"#{attribute}_confirmation" unless klass.method_defined?(:"#{attribute}_confirmation=")
+        end.compact)
       end
     end
 
diff --git a/activemodel/test/cases/secure_password_test.rb b/activemodel/test/cases/secure_password_test.rb
index 7783bb25d5..02cd3b8a93 100644
--- a/activemodel/test/cases/secure_password_test.rb
+++ b/activemodel/test/cases/secure_password_test.rb
@@ -88,4 +88,10 @@ class SecurePasswordTest < ActiveModel::TestCase
     @user.password = "secret"
     assert_equal BCrypt::Engine::MIN_COST, @user.password_digest.cost
   end
+
+  test "blank password_confirmation does not result in a confirmation error" do
+    @user.password = ""
+    @user.password_confirmation = ""
+    assert @user.valid?(:update), "user should be valid"
+  end
 end
diff --git a/activemodel/test/cases/validations/confirmation_validation_test.rb b/activemodel/test/cases/validations/confirmation_validation_test.rb
index f7556a249f..814eec3f59 100644
--- a/activemodel/test/cases/validations/confirmation_validation_test.rb
+++ b/activemodel/test/cases/validations/confirmation_validation_test.rb
@@ -71,4 +71,35 @@ class ConfirmationValidationTest < ActiveModel::TestCase
     I18n.backend = @old_backend
   end
 
+  test "does not override confirmation reader if present" do
+    klass = Class.new do
+      include ActiveModel::Validations
+
+      def title_confirmation
+        "expected title"
+      end
+
+      validates_confirmation_of :title
+    end
+
+    assert_equal "expected title", klass.new.title_confirmation,
+     "confirmation validation should not override the reader"
+  end
+
+  test "does not override confirmation writer if present" do
+    klass = Class.new do
+      include ActiveModel::Validations
+
+      def title_confirmation=(value)
+        @title_confirmation = "expected title"
+      end
+
+      validates_confirmation_of :title
+    end
+
+    model = klass.new
+    model.title_confirmation = "new title"
+    assert_equal "expected title", model.title_confirmation,
+     "confirmation validation should not override the writer"
+  end
 end