search

michaelbanfield / devise-pwned_password

Devise extension that checks user passwords against the PwnedPasswords dataset
https://rubygems.org/gems/devise-pwned_password
MIT License
156 stars 29 forks source link

Fix “no implicit conversion of nil into String” #6

Closed damonmorgan closed 6 years ago

damonmorgan commented 6 years ago

If the devise model is :trackable, the model will be updated and saved at which point validations are run. However password is cleared on authentication so Digest::SHA1.hexdigest(password).upcase fails because password is nil. Add a guard clause to prevent the validation from running if the password isn’t present.

michaelbanfield commented 6 years ago

Change looks good.

Just to clarify a bit what was the impact of this? Does devise by default use :trackable ?

I'll try and cover this in a unit test before the next release so we dont make the same mistake again

damonmorgan commented 6 years ago

No modules are used by default in devise but trackable is a popular one as it tracks sign in count, timestamps and IP address. The impact was that sign ins threw an uncaught exception if the trackable module was used. And I haven't tested it, but I think this would also break update_without_password (with no other modules included).

You can see where other extensions do this - https://github.com/HCLarsen/devise-uncommon_password/blob/master/lib/devise/uncommon_password/model.rb#L24