[#4245] Allowing password to nil #4261

sivagollapalli · 2016-08-18T14:07:44Z

sivagollapalli · 2016-08-18T14:08:38Z

@lucasmazza Let me know your comments on this fix.

lucasmazza · 2016-08-19T16:21:50Z

lib/devise/models/database_authenticatable.rb

@@ -37,11 +37,12 @@ def self.required_fields(klass)
      # the hashed password.
      def password=(new_password)
        @password = new_password
-        self.encrypted_password = password_digest(@password) if @password.present?
+        self.encrypted_password = password_digest(@password)


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).

@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.

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.

@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

sivagollapalli · 2016-09-07T13:23:48Z

lib/devise/models/database_authenticatable.rb

@@ -37,11 +37,12 @@ def self.required_fields(klass)
      # the hashed password.
      def password=(new_password)
        @password = new_password
-        self.encrypted_password = password_digest(@password) if @password.present?
+        self.encrypted_password = password_digest(@password) 


@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?

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

sivagollapalli · 2016-09-09T06:26:40Z

@lucasmazza I tweaked some test cases. Now build is passing. Let me know your comments.

pedrofurtado · 2018-08-06T12:53:27Z

@lucasmazza @sivagollapalli Any news?

tegon

@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.

tegon · 2018-08-20T20:52:22Z

lib/devise/models/database_authenticatable.rb

      end

      # Verifies whether a password (ie from sign in) is the user password.
      def valid_password?(password)
+        return false if password.blank?


I believe we can remove this condition since it's already present inside Devise::Encryptor.compare

No, Devise::Encryptor.compare is checking that encrypted_password is present (or at least it is as of now).

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.

tegon · 2018-08-20T20:53:54Z

lib/devise/models/database_authenticatable.rb

@@ -144,6 +145,7 @@ def send_password_change_notification
      # See https://github.com/plataformatec/devise-encryptable for examples
      # of other hashing engines.
      def password_digest(password)
+        return nil if password.blank?


You can omit the nil here because it's the default return's value

tegon · 2018-08-20T20:54:09Z

test/models/recoverable_test.rb

@@ -158,7 +158,7 @@ def setup
    user = create_user
    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')


Why do we need this change?

sivagollapalli · 2018-10-24T16:29:20Z

@tegon Made changes. Could you please check?

tegon · 2018-11-13T15:57:40Z

@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).

This reverts commit 3aedbf0.

…d-password-to-nil-if-password-is-nil Reverts both "[#4245] Allow password to nil (#4261)" and "Add more tests (#4970)"

[heartcombo#4245] Allowing password to nil

74f3fff

lucasmazza reviewed Aug 19, 2016
View reviewed changes

lucasmazza added the Needs more work label Aug 19, 2016

Set encrypted password to nil if password is nil

cc1dfcb

sivagollapalli reviewed Sep 7, 2016
View reviewed changes

[heartcombo#4245] Fixing the build

f17e1f3

tegon added Needs review and removed Needs more work labels Dec 14, 2017

tegon reviewed Aug 20, 2018

View reviewed changes

tegon added Needs more work and removed Needs review labels Aug 20, 2018

Removed unnecessary code

2eaad6c

tegon merged commit 3aedbf0 into heartcombo:master Nov 13, 2018

tegon mentioned this pull request Nov 13, 2018

:database_authenticatable issue with clean_passwords #4245

Closed

tegon mentioned this pull request Nov 13, 2018

Add more tests #4970

Merged

sivagollapalli deleted the issue_4245 branch November 14, 2018 15:17

philayres mentioned this pull request Feb 11, 2019

v4.6.0 breaks rspec controller tests #5021

Closed

gsar mentioned this pull request Feb 25, 2019

v4.6.0 breaks activeadmin forms that have password field #5033

Closed

timkrins mentioned this pull request Mar 15, 2019

#valid_password? returns false when updating resource with empty password fields #5038

Closed

tagCincy mentioned this pull request Mar 20, 2019

Remove Valid Password Constraints from DatabaseAuthenticatable Concern #5048

Closed

hayesr mentioned this pull request Mar 20, 2019

Do not set encrypted_password to nil if password is blank #5044

Closed

mracos added a commit that referenced this pull request Mar 25, 2019

Revert "[#4245] Allowing password to nil (#4261)"

5ba3632

This reverts commit 3aedbf0.

mracos added a commit that referenced this pull request Mar 25, 2019

Revert "[#4245] Allowing password to nil (#4261)"

8753c6f

This reverts commit 3aedbf0.

mracos added a commit that referenced this pull request Mar 26, 2019

Revert "[#4245] Allowing password to nil (#4261)"

61219ee

This reverts commit 3aedbf0.

mracos added a commit that referenced this pull request Mar 26, 2019

Revert "[#4245] Allowing password to nil (#4261)"

f9d13f0

This reverts commit 3aedbf0.

tegon added a commit that referenced this pull request Mar 26, 2019

Merge pull request #5051 from plataformatec/mf-revert-update-encrypte…

55e726e

…d-password-to-nil-if-password-is-nil Reverts both "[#4245] Allow password to nil (#4261)" and "Add more tests (#4970)"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#4245] Allowing password to nil #4261

[#4245] Allowing password to nil #4261

sivagollapalli commented Aug 18, 2016 •

edited

sivagollapalli commented Aug 18, 2016

lucasmazza Aug 19, 2016

sivagollapalli Aug 22, 2016 •

edited

lucasmazza Sep 4, 2016

gsar Feb 25, 2019

sivagollapalli Sep 7, 2016

lucasmazza Sep 7, 2016

sivagollapalli commented Sep 9, 2016

pedrofurtado commented Aug 6, 2018

tegon left a comment

tegon Aug 20, 2018

shioyama Sep 20, 2018

tegon Nov 13, 2018

tegon Aug 20, 2018

tegon Aug 20, 2018 •

edited

sivagollapalli commented Oct 24, 2018

tegon commented Nov 13, 2018

[#4245] Allowing password to nil #4261

[#4245] Allowing password to nil #4261

Conversation

sivagollapalli commented Aug 18, 2016 • edited

sivagollapalli commented Aug 18, 2016

Choose a reason for hiding this comment

sivagollapalli Aug 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivagollapalli commented Sep 9, 2016

pedrofurtado commented Aug 6, 2018

tegon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tegon Aug 20, 2018 • edited

Choose a reason for hiding this comment

sivagollapalli commented Oct 24, 2018

tegon commented Nov 13, 2018

sivagollapalli commented Aug 18, 2016 •

edited

sivagollapalli Aug 22, 2016 •

edited

tegon Aug 20, 2018 •

edited