[#4245] Allowing password to nil#4261
Conversation
|
@lucasmazza Let me know your comments on this fix. |
| def password=(new_password) | ||
| @password = new_password | ||
| self.encrypted_password = password_digest(@password) if @password.present? | ||
| self.encrypted_password = password_digest(@password) |
There was a problem hiding this comment.
password_digest won't return nil for nil values:
BCrypt::Password.create(nil).to_s # => "$2a$10$LcRvA3AlhRYr6GWAsZaH7e3crh/iyhepNpIzZEQOUDnJsOV7C2/IG"
BCrypt::Password.create(nil).to_s # => "$2a$10$fpABQ3uqeESmjjk8hQ/4quWCxkfU6IKi6XVrQBUnDzRkl3.jYTMtS"
BCrypt::Password.create(nil).to_s # => "$2a$10$BB1N8TflH1jufcWAyHqC4ed2oStZ.7sya4HxrMI9IZQpEr8Pya56G"
BCrypt::Password.create(nil).to_s # => "$2a$10$jIDzMUZXJBJGoTLxoiu.8emLtpukL6NNxbbPbl.oztNrASgA3iNie"
BCrypt::Password.create(nil).to_s # => "$2a$10$f5F7x7UZXPmkRlMKo8RaRu/5lg/rZ/IER9bN7pFfY7UTFRH2MFpVu"Maybe we should set it explicity to nil (that should probably fix the broken tests in our test suite).
There was a problem hiding this comment.
@lucasmazza Yes, I know that. Since we have put NOT NULL constraint for encrypted_password on migration. So, I am generating password digest for nil values as well.
There was a problem hiding this comment.
So, I am generating password digest for nil values as well.
We shouldn't - nil values should set the encrypted_password as nil otherwise the validations won't be aware that the provided value is nil.
There was a problem hiding this comment.
@sivagollapalli @lucasmazza this change breaks things outside of devise that end up calling password=, like activeadmin. The code here setting encrypted_password to a nil value while keeping the NOT NULL validation in the database doesn't make any sense, one or the other needs to change as well. see #5033
| def password=(new_password) | ||
| @password = new_password | ||
| self.encrypted_password = password_digest(@password) if @password.present? | ||
| self.encrypted_password = password_digest(@password) |
There was a problem hiding this comment.
@lucasmazza Most of the test cases has been failed because of I have removed @password.present? condition. I couldn't find the reason that why it is failing. Could you help me?
There was a problem hiding this comment.
By the looks of it we need to update clean_up_passwords to clear the instance variables directly, instead of calling password=.
def clean_up_passwords
- self.password = self.password_confirmation = nil
+ @password = @password_confirmation = nil
end|
@lucasmazza I tweaked some test cases. Now build is passing. Let me know your comments. |
|
@lucasmazza @sivagollapalli Any news? |
tegon
left a comment
There was a problem hiding this comment.
@sivagollapalli Your pull request looks good, I just made some minor comments. If you can take a look at them before we merge, that would be great.
Sorry for taking so long to reviews this, there's a lot of pull requests to review and I was only able to review this one today.
|
|
||
| # Verifies whether a password (ie from sign in) is the user password. | ||
| def valid_password?(password) | ||
| return false if password.blank? |
There was a problem hiding this comment.
I believe we can remove this condition since it's already present inside Devise::Encryptor.compare
There was a problem hiding this comment.
No, Devise::Encryptor.compare is checking that encrypted_password is present (or at least it is as of now).
There was a problem hiding this comment.
Yeah, but since we're returning nil from #password_digest when the password is blank, encrypted_password will always be nil, and the .compare method will return in the return false if hashed_password.blank? condition.
| # See https://github.com/plataformatec/devise-encryptable for examples | ||
| # of other hashing engines. | ||
| def password_digest(password) | ||
| return nil if password.blank? |
There was a problem hiding this comment.
You can omit the nil here because it's the default return's value
test/models/recoverable_test.rb
Outdated
| raw = user.send_reset_password_instructions | ||
|
|
||
| reset_password_user = User.reset_password_by_token(reset_password_token: raw) | ||
| reset_password_user = User.reset_password_by_token(reset_password_token: raw, password: '1234567') |
|
@tegon Made changes. Could you please check? |
|
@sivagollapalli Thanks! Sorry again for taking so long. |
After merging #4261, I realized that we could add a couple more of tests, to ensure the new behavior added to `#valid_password?` - which is that it should return `false` when the password is either `nil` or blank (''). I've also removed [this condition](https://github.com/plataformatec/devise/blob/master/lib/devise/models/database_authenticatable.rb#L68) because it's already present at `Devise::Encryptor` module in the `.compare` [method](https://github.com/plataformatec/devise/blob/master/lib/devise/encryptor.rb#L15).
After merging #4261, I realized that we could add a couple more tests, to ensure the new behavior added to `#valid_password?` - which is that it should return `false` when the password is either `nil` or blank (''). I've also removed [this condition](https://github.com/plataformatec/devise/blob/master/lib/devise/models/database_authenticatable.rb#L68) because it's already present at `Devise::Encryptor` module in the `.compare` [method](https://github.com/plataformatec/devise/blob/master/lib/devise/encryptor.rb#L15).
After merging #4261, I realized that we could add a couple more tests, to ensure the new behavior added to `#valid_password?` - which is that it should return `false` when the password is either `nil` or blank (''). I've also removed [this condition](https://github.com/plataformatec/devise/blob/master/lib/devise/models/database_authenticatable.rb#L68) because it's already present at `Devise::Encryptor` module in the `.compare` [method](https://github.com/plataformatec/devise/blob/master/lib/devise/encryptor.rb#L15).
#4245