From 6173a160ac334fd66cda60555e348d5136b08e01 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Fri, 22 Nov 2013 13:46:59 -0700 Subject: [PATCH] Validating presence of a secure password may raise an error If you have a model that declares `has_secure_password` and you also have a presence validation on the password, and you write a test against this validation using an instance of your model where the password is already set, then your test will fail. This is because has_secure_password (at least on Rails 4) defines #password= such that if it is given nil, then the password will not be overwritten with nil. This interferes with how our validate_presence_of matcher works. Unfortunately there is not a great way to get around this (using \#write_attribute won't work, either). So in this case we raise a helpful error message that instructs the user to use an empty record against `validates_presence_of`. --- NEWS.md | 7 ++++ .../active_model/allow_value_matcher.rb | 20 +++++++++- lib/shoulda/matchers/active_model/errors.rb | 39 +++++++++++++++++++ .../validate_presence_of_matcher.rb | 7 +++- lib/shoulda/matchers/error.rb | 21 +++++++++- .../validate_presence_of_matcher_spec.rb | 19 +++++++++ 6 files changed, 110 insertions(+), 3 deletions(-) diff --git a/NEWS.md b/NEWS.md index c12baebf..88915679 100644 --- a/NEWS.md +++ b/NEWS.md @@ -37,6 +37,13 @@ `greater_than`, `greater_than_or_equal_to`, `less_than`, `less_than_or_equal_ to` or `equal_to` is not appropriate. +* Change `validate_presence_of` under Rails 4 so that if you are using it with a + user whose model `has_secure_password` and whose password is set to a value, + you will be instructed to use a user whose password is blank instead. The + reason for this change is due to the fact that Rails 4's version of + `has_secure_password` defines #password= such that `nil` will be ignored, + which interferes with how `validate_presence_of` works. + # v 2.5.0 * Fix Rails/Test::Unit integration to ensure that the test case classes we are diff --git a/lib/shoulda/matchers/active_model/allow_value_matcher.rb b/lib/shoulda/matchers/active_model/allow_value_matcher.rb index d0d9c49a..8f19c6e0 100644 --- a/lib/shoulda/matchers/active_model/allow_value_matcher.rb +++ b/lib/shoulda/matchers/active_model/allow_value_matcher.rb @@ -68,7 +68,7 @@ module Shoulda # :nodoc: values_to_match.none? do |value| self.value = value - instance.__send__("#{attribute_to_set}=", value) + set_and_double_check_attribute!(attribute_to_set, value) errors_match? end end @@ -93,6 +93,24 @@ module Shoulda # :nodoc: :instance, :attribute_to_set, :attribute_to_check_message_against, :context, :value, :matched_error + def set_and_double_check_attribute!(attribute_name, value) + instance.__send__("#{attribute_name}=", value) + + if value.nil? + ensure_attribute_was_cleared!(attribute_name) + end + end + + def ensure_attribute_was_cleared!(attribute_name) + if instance.respond_to?(attribute_name) + actual_value = instance.__send__(attribute_name) + + if !actual_value.nil? + raise Shoulda::Matchers::ActiveModel::CouldNotClearAttribute.create(actual_value) + end + end + end + def errors_match? has_messages? && errors_for_attribute_match? end diff --git a/lib/shoulda/matchers/active_model/errors.rb b/lib/shoulda/matchers/active_model/errors.rb index 099aeed4..5ca447f0 100644 --- a/lib/shoulda/matchers/active_model/errors.rb +++ b/lib/shoulda/matchers/active_model/errors.rb @@ -2,7 +2,46 @@ module Shoulda # :nodoc: module Matchers module ActiveModel # :nodoc: class CouldNotDetermineValueOutsideOfArray < RuntimeError; end + class NonNullableBooleanError < Shoulda::Matchers::Error; end + + class CouldNotClearAttribute < Shoulda::Matchers::Error + def self.create(actual_value) + super(actual_value: actual_value) + end + + attr_accessor :actual_value + + def message + "Expected value to be nil, but was #{actual_value.inspect}." + end + end + + class CouldNotSetPasswordError < Shoulda::Matchers::Error + def self.create(model) + super(model: model) + end + + attr_accessor :model + + def message + <<-EOT.strip +The validation failed because your #{model_name} model declares `has_secure_password`, and +`validate_presence_of` was called on a #{record_name} which has `password` already set to a value. +Please use a #{record_name} with an empty `password` instead. + EOT + end + + private + + def model_name + model.name + end + + def record_name + model_name.humanize.downcase + end + end end end end diff --git a/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb b/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb index 724bef29..61ba5241 100644 --- a/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb +++ b/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb @@ -20,7 +20,6 @@ module Shoulda # :nodoc: end class ValidatePresenceOfMatcher < ValidationMatcher # :nodoc: - def with_message(message) @expected_message = message if message self @@ -30,6 +29,12 @@ module Shoulda # :nodoc: super(subject) @expected_message ||= :blank disallows_value_of(blank_value, @expected_message) + rescue Shoulda::Matchers::ActiveModel::CouldNotClearAttribute => error + if @attribute == :password + raise Shoulda::Matchers::ActiveModel::CouldNotSetPasswordError.create(subject.class) + else + raise error + end end def description diff --git a/lib/shoulda/matchers/error.rb b/lib/shoulda/matchers/error.rb index f34c9176..bf820621 100644 --- a/lib/shoulda/matchers/error.rb +++ b/lib/shoulda/matchers/error.rb @@ -1,5 +1,24 @@ module Shoulda module Matchers - class Error < StandardError; end + class Error < StandardError + def self.create(attributes) + allocate.tap do |error| + attributes.each do |name, value| + error.__send__("#{name}=", value) + end + + error.__send__(:initialize) + end + end + + def initialize(*args) + super + @message = message + end + + def message + "" + end + end end end diff --git a/spec/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb b/spec/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb index bee804e0..ae586287 100644 --- a/spec/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb +++ b/spec/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb @@ -144,6 +144,25 @@ describe Shoulda::Matchers::ActiveModel::ValidatePresenceOfMatcher do end end + if rails_4_x? + context 'against a pre-set password in a model that has_secure_password' do + it 'raises an error to instruct the user' do + user_class = define_model :user, password_digest: :string do + has_secure_password + validates_presence_of :password + end + + user = user_class.new + user.password = 'something' + + error_class = Shoulda::Matchers::ActiveModel::CouldNotSetPasswordError + expect { + expect(user).to validate_presence_of(:password) + }.to raise_error(error_class) + end + end + end + def matcher validate_presence_of(:attr) end