Allow passing Hash on secure password validations

[liljack]

Summary

I was running into a case where I didn't want to just disabled the validations and add my own. In fact, I would very much like to keep the default validation but just de-activate it on some scenario: e.g. Inviting a user without having to set a password for them yet so they can add it themselves later when they receive an email invitation to finish setting up their account.

My understanding of the validations flag originally intended was to just disabled them and if you needed something more custom, you could run your own validations instead.

This would be an acceptable solution, but it would add more code to my controller. Instead validations can receive a Hash which is then use to apply validations rules to validate

This is just a suggestion, I am not sure if there is a need, and I am happy to be told to set validations to false and do my own validations if this is what's expected here.

I am aware this PR is probably far from perfect. Any feedback welcome.

Other Information

[jonathanhefner]

e.g. Inviting a user without having to set a password for them yet so they can add it themselves later when they receive an email invitation to finish setting up their account.

Your models may be different than I'm imagining, but I think it would be good to give the Proc access to the model instance, no? That way it could be based on e.g. user.guest? instead of requiring a GuestUser model class.

So what about reusing the :if / :unless logic that validates provides:

index 7cc207204e..bd584483bd 100644
--- a/activemodel/lib/active_model/secure_password.rb
+++ b/activemodel/lib/active_model/secure_password.rb
@@ -93,15 +93,17 @@ def has_secure_password(attribute = :password, validations: true)
         if validations
           include ActiveModel::Validations

+          validation_options = validations.is_a?(Hash) ? validations : {}
+
           # This ensures the model has a password by checking whether the password_digest
           # is present, so that this works with both new and existing records. However,
           # when there is an error, the message is added to the password attribute instead
           # so that the error message will make sense to the end-user.
-          validate do |record|
+          validate(validation_options) do |record|
             record.errors.add(attribute, :blank) unless record.public_send("#{attribute}_digest").present?
           end

-          validate do |record|
+          validate(validation_options) do |record|
             if challenge = record.public_send(:"#{attribute}_challenge")
               digest_was = record.public_send(:"#{attribute}_digest_was") if record.respond_to?(:"#{attribute}_digest_was")

@@ -111,8 +113,8 @@ def has_secure_password(attribute = :password, validations: true)
             end
           end

-          validates_length_of attribute, maximum: ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED
-          validates_confirmation_of attribute, allow_blank: true
+          validates_length_of attribute, **validation_options, maximum: ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED
+          validates_confirmation_of attribute, **validation_options, allow_blank: true
         end
       end
     end

[liljack]

Thanks for your reply.

So the give your more context, My app allows someone to register. You have to fill in the whole registration form (password included) at that stage.
Once you are all setup, The user is created along with a "company" they will belong to.

After that, they can now decide to invite users to their company.

When you do that, it's still the same User model, I don't have the notion of GuestUser, all I am doing, or wanting to do at that point, is bypassing validations for password when my user is being created "internally".
What I was wanting to do was to set a flag which would turn a variable to false so the validations is not triggered

has_secure_password validations: Proc.new { |user| user.skip_password_validations? }

That's for the context.

Now looking at your comment, this makes a lot of sense, and definitely looks like a better implementation than what I have done (it does give more flexibility). I am happy to update my test and the code to reflect that 👍

On the last 2 lines you changed, does it matter to have the **validation_options at the end of the line rather than before the other rules ? just a preference from me to have it at the end

[liljack]

This has been updated.
I also took the opportunity to rename my test model as it wasn't reflecting what I was trying to do.

Let me know what you think.

[jonathanhefner]

On the last 2 lines you changed, does it matter to have the **validation_options at the end of the line rather than before the other rules ?

The difference between the two is that if validation_options has :maximum or :allow_blank keys, it will override those values for validates_length_of and validates_confirmation_of. It's probably fine either way, but, just in case someone re-uses an options hash (which would be a semi-plausible way to reuse :if / :unless Procs), putting **validation_options first would be safer.

[liljack]

Hmm, I can see both points to be valid points (one being safer as you say). On the other hand, if you did want to set :allow_blank it might not be obvious that this is overwritten and therefore you might be left wondering why it doesn't work. For the :maximum, I am with you on this, this should definitely be kept as the default and shouldn't be overridden, not sure what I was thinking yesterday night.

I am perhaps over thinking things here though. I will happily make that change and this can be discussed if it is ever going to be a problem (which I hardly doubt it will)

[jonathanhefner]

With some Ruby magic, we can test this without an extra (statically defined) model:

Suggested change

	test "create a new user and bypassing the validations" do
	@person_skipping_secure_password_validations.password = ""
	assert @person_skipping_secure_password_validations.valid?
	end
	test "supports conditional validation" do
	model_class = Struct.new(:requires_password, :password_digest) do
	include ActiveModel::SecurePassword
	has_secure_password validations: { if: :requires_password }
	end
	assert_predicate model_class.new(false), :valid?
	assert_predicate model_class.new(true), :invalid?
	end

[liljack]

🙏 Thanks, yes indeed a Struct will work just fine here.

[liljack]

That's it updated. Thanks for your patience and for running me through things 🙏

[jonathanhefner]

Taking another pass at this.

First, let me apologize: when I wrote #45487 (comment), I didn't look closely at the validations that it touched. Upon closer inspection, I realize we only need to pass options to the first validation. I explain why in my comments. Sorry for the back and forth! 🙏

Also, we should add documentation for this enhancement. I'm thinking we should add it to this paragraph:

rails/activemodel/lib/active_model/secure_password.rb

Lines 38 to 40 in c704da6

	# All of the above validations can be omitted by passing
	# <tt>validations: false</tt> as an argument. This allows complete
	# customizability of validation behavior.

Something like:

      # The password presence validation can be conditionally enforced by 
      # passing an options hash to +:validations+ with the standard +:if+ / 
      # +:unless+ / +:on+ keys. (See ActiveModel::Validations::ClassMethods#validates 
      # for more information.)  Alternatively, all of the above validations can 
      # be omitted by passing <tt>validations: false</tt>. This allows complete 
      # customizability of validation behavior.

And we'll need a CHANGELOG entry.

[jonathanhefner]

Setting password_challenge is already not required. So if it has been explicitly set, I think we should validate it, regardless of other validations.

Suggested change

	validate(validation_options) do |record|
	validate do |record|

[liljack]

I am not sure this is the behaviour I would expect though. It could still hold true that you set a password_challenge but there might still be situation where you don't want to validate it. If I was to use this library, I would expect that doing:

validations { if: :my_condition }

would disable all the validations, not some of them.
What do you think ?

[liljack]

I understand that those are just params that will be set. So you wouldn't be setting those params if you wanted to skip validations, so I get what you are saying, but feels like it would still hold true that my expectation would be to disable all validations regardless (just in case you are still sending those params for some reasons). It's more in term of what the API looks like.

[jonathanhefner]

password_challenge is a gating mechanism that checks the already existing password when e.g. changing some sensitive field of the model. It is a temporary value, like password_confirmation. See #43688 for more information.

If a client sends a password_challenge we should definitely check it. If a client sends a password_challenge unintentionally, I would consider that an error (which the validation failure would then reflect). And if an application needs different behavior, it can still use validations: false as an escape hatch.

[liljack]

Yes, that makes sense. I agree it should be an error. Not sure what I was thinking, Probably poorly understand what the password_challenge was and didn't really look at it TBH. Thanks for that

[jonathanhefner]

password_confirmation is also already not required. So if it has been explicitly set, I think we should validate it, regardless of other validations.

Suggested change

	validates_confirmation_of attribute, **validation_options, allow_blank: true
	validates_confirmation_of attribute, allow_blank: true

[liljack]

Agreed on removing the **validation_options from validates_length_of and validates_confirmation_of

[jonathanhefner]

This validation enforces the maximum password length that BCrypt allows, and only applies when password has actually been set, so we should leave it unchanged.

Suggested change

	validates_length_of attribute, **validation_options, maximum: ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED
	validates_length_of attribute, maximum: ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED

[liljack]

👍

[liljack]

Hey - Super happy to make those changes. Yes thinking about it, it probably only makes sense to pass the validations to validate here.

Taking another pass at this.

First, let me apologize: when I wrote #45487 (comment), I didn't look closely at the validations that it touched. Upon closer inspection, I realize we only need to pass options to the first validation. I explain why in my comments. Sorry for the back and forth! 🙏

No worries at all. Honestly I have been following blindly as well and could have look at things better. I will have a poke around and a closer look as well :-)

Also, we should add documentation for this enhancement. I'm thinking we should add it to this paragraph:

rails/activemodel/lib/active_model/secure_password.rb

Lines 38 to 40 in c704da6

	# All of the above validations can be omitted by passing
	# <tt>validations: false</tt> as an argument. This allows complete
	# customizability of validation behavior.

Something like:

      # The password presence validation can be conditionally enforced by 
      # passing an options hash to +:validations+ with the standard +:if+ / 
      # +:unless+ / +:on+ keys. (See ActiveModel::Validations::ClassMethods#validates 
      # for more information.)  Alternatively, all of the above validations can 
      # be omitted by passing <tt>validations: false</tt>. This allows complete 
      # customizability of validation behavior.

And we'll need a CHANGELOG entry.

According to to https://github.com/rails/rails/blob/main/activemodel/lib/active_model/validations.rb#L89, it will also add :prepend. I will make sure I update the documentation and I will add a note to the CHANGELOG

[jonathanhefner]

password_challenge is a gating mechanism that checks the already existing password when e.g. changing some sensitive field of the model. It is a temporary value, like password_confirmation. See #43688 for more information.

If a client sends a password_challenge we should definitely check it. If a client sends a password_challenge unintentionally, I would consider that an error (which the validation failure would then reflect). And if an application needs different behavior, it can still use validations: false as an escape hatch.

[jonathanhefner]

I realize :prepend is accepted, but I don't think we should document it. In fact, I considered writing validations.slice(:if, :unless, :on), but decided to keep the code simple instead.

I don't think there is a strong use case for supporting it. Furthermore, it could complicate adding more validations that use validation_options in the future. Now, if someone were to specify prepend: true, the password presence validation would be added to the top of validations list. In the future, if we add another validate(validation_options) block at the bottom of has_secure_password, then prepend: true would put that validation at the top of the list, breaking the expectation.

Given this and my suggestion regarding password_challenge, I would prefer to use the wording from #45487 (review):

      # The password presence validation can be conditionally enforced by 
      # passing an options hash to +:validations+ with the standard +:if+ / 
      # +:unless+ / +:on+ keys. (See ActiveModel::Validations::ClassMethods#validates 
      # for more information.)  Alternatively, all of the above validations can 
      # be omitted by passing <tt>validations: false</tt>. This allows complete 
      # customizability of validation behavior.

(Note: "This allows complete customizability of validation behavior" refers specifically to validations: false rather than the :validations option as a whole, so I am trying to keep those sentences adjacent.)

[liljack]

Hey, thanks for the feedback here. I will make those changes :-)

[liljack]

Hey @jonathanhefner,

really sorry for letting this hang around for so long - I have had a lot on which means I was only able to look back at this today.
I have addressed your 2 latest comment. Let me know how this is looking for you now. 🙏

[jonathanhefner]

@liljack Looks like there are some RuboCop failures due to trailing whitespace. If you are busy, I can push a commit to fix those. Just let me know. 😄

[liljack]

@liljack Looks like there are some RuboCop failures due to trailing whitespace. If you are busy, I can push a commit to fix those. Just let me know. 😄

🤦 So sorry - that should be fixed now 🙏

[jonathanhefner]

🤦 So sorry - that should be fixed now 🙏

No worries! It was probably my fault leaving them in #45487 (review). There was a new CHANGELOG merge conflict though, so I did push a commit to fix that.

Thank you for working on this, @liljack! 👍

[dhh]

Just had a chance to look at this. I think we can improve the readability of the API by switching to a layout style setup:

secure_password validations: :requires_password?

That is we accept either a boolean or a method reference that returns a boolean. We don't need the whole if/unless structure that makes it read clumsy.

[liljack]

Just had a chance to look at this. I think we can improve the readability of the API by switching to a layout style setup:

secure_password validations: :requires_password?

That is we accept either a boolean or a method reference that returns a boolean. We don't need the whole if/unless structure that makes it read clumsy.

Thanks @dhh - this seems to be closer to what my original pull request was: 7f8d1a4

Not sure why I wanted to have a proc where probably any method passed as a boolean would do the trick ?