has_secure_password validations bypass Base.save(validate: false)

[why-el]

Hey,

In the process of upgrading an app to Rails 4 (from 3.2.15), some tests using ActiveRecord.save(validate: false) failed because apparently has_secure_password's validations bypass that option.

This has been introduced in this commit I believe ad7f9cd

I accept that there are use cases where you want your own validations for has_secure_password, but the validations API documentation specifically says that passing validate: false will bypass all your validations. Given the semantics of has_secure_password, namely that it gets included on the fly, it's reasonable to assume that its validations will be treated equally.

I see two solutions to this and I can take care of both if there are no objections:

1. Update the documentation to exclude has_secure_password from the list of bypassed validations when calling validate: false (Brittle)
2. Make has_secure_password pick up validate: false while keeping its own options if people rely on it.

[carlosantoniodasilva]

I'm not sure I understand your the issue you're trying to describe, but has sec password only adds normal validations to the model, and they should respect the validate option given to save. The commit you link only changes a before_create callback, so I'm unsure how it relates to the issue.

In any case, maybe you could clarify the issue with some code that demonstrates it? Thanks.

[why-el]

Absolutely. Here is a failing test.

unless File.exist?('Gemfile')
  File.write('Gemfile', <<-GEMFILE)
    source 'https://rubygems.org'
    gem 'rails', github: 'rails/rails'
    gem 'arel', github: 'rails/arel'
    gem 'bcrypt-ruby', '~> 3.0.0'
    gem 'sqlite3'
  GEMFILE

  system 'bundle'
end

require 'bundler'
Bundler.setup(:default)

require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :users do |t|
    t.string 'password_digest'
  end
end

class User < ActiveRecord::Base
  has_secure_password
end


class BugTest < Minitest::Test
  def test_should_bypass_has_secure_password_validations_when_saving
    user = User.new
    # This will raise an error and it shouldn't.
    # Specifically, it will raise the following: 
    # RuntimeError: Password digest missing on new record
    user.save!(validate: false)
  end
end

[lukesarnacki]

@why-el the commit that it was actually added is 0e1e527. In commit you pasted it was moved inside if, which checks validations flag. Which is actually good thing, because (kind of hakish) workaround would be to set validations: false and then add those two validations by yourself.

But I also think that this is unexpected behaviour.

[lukesarnacki]

Actually why we even need this line: https://github.com/rails/rails/blob/master/activemodel/lib/active_model/secure_password.rb#L64 ?

  user.save!
  # => ActiveRecord::RecordInvalid: Validation failed: Password can't be blank

  user.save!(validate: false)
  # =>  RuntimeError: Password digest missing on new record

  user.save
  # => false

  user.save(validate: false)
  # RuntimeError: Password digest missing on new record

UPDATE: ok, I found

rails/activemodel/test/cases/secure_password_test.rb

Lines 61 to 69 in 87e1e86

	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

, thanks @why-el

[why-el]

thats needed when people dont have their own validarions. i can try amd work something out if @carlossantoniodasilva also thinks this is unexpected.

[chancancode]

I have a potential patch for this at #13772, you guys might want to review that!