Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#4245] Allowing password to nil #4261

Merged
merged 4 commits into from
Nov 13, 2018
Merged

[#4245] Allowing password to nil #4261

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
74f3fff
[#4245] Allowing password to nil
Aug 18, 2016
cc1dfcb
Set encrypted password to nil if password is nil
Sep 7, 2016
f17e1f3
[#4245] Fixing the build
Sep 8, 2016
2eaad6c
Removed unnecessary code
sivagollapalli Oct 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/devise/models/database_authenticatable.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

end

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Devise::Encryptor.compare(self.class, encrypted_password, password)
end

Expand Down
8 changes: 8 additions & 0 deletions test/models/database_authenticatable_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,4 +266,12 @@ def setup
]
end
end

test 'rehash encrypt password if password is nil' do
user = User.create(email: "HEllO@example.com", password: "12345678")
user.password = nil
user.save
refute user.valid_password?('12345678')
refute user.valid_password?(nil)
end
end