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.

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

:database_authenticatable issue with clean_passwords #4245

Closed
kaelumania opened this issue Aug 10, 2016 · 6 comments
Closed

:database_authenticatable issue with clean_passwords #4245

kaelumania opened this issue Aug 10, 2016 · 6 comments

Comments

@kaelumania
Copy link

kaelumania commented Aug 10, 2016

Within the :database_authenticatable there is an issue at Line 40

Whenever the passwords are cleaned (set to nil), e.g. in the RegistrationsController the encrypted_password remains dirty, which can have heavy security affecting side-effects.

I propose to set the encrypted_password also to nil or to revert to its original value. Here is a little test scenario:

model.password = 'abc'
model.password = nil

model.valid_password? 'abc' => true
@lucasmazza
Copy link
Contributor

@kaelumania sounds reasonable, can you please send a Pull Request for that? Thanks! 😄

@sivagollapalli
Copy link
Contributor

@lucasmazza Should we need reset encrypted_password to nil if the password is nil ? If yes, I think we need to remove NOT NULL constraint for encrypted_password from migration. I just want to confirm prior implementation.

@lucasmazza
Copy link
Contributor

@sivagollapalli no, we don't need to remove the constraint - from @kaelumania's report the goal is to ensure that both attributes are properly in sync, and not let a null password be persisted by the default.

@sivagollapalli
Copy link
Contributor

sivagollapalli commented Aug 17, 2016

@lucasmazza So, the above test case should return false but currently, it returns true. Am I right?

@kaelumania
Copy link
Author

kaelumania commented Aug 18, 2016

@sivagollapalli yes, I think you are right. We use devise database_authenticatable while allowing nil passwords - which results in no access. Because not every entity in our system has a password or supports access by password. Therefore our barrier to reject access with NO (nil) password is line https://github.com/plataformatec/devise/blob/master/lib/devise/encryptor.rb#L13 that states if the password is nil it is not valid!

sivagollapalli pushed a commit to sivagollapalli/devise that referenced this issue Aug 18, 2016
sivagollapalli pushed a commit to sivagollapalli/devise that referenced this issue Sep 8, 2016
tegon pushed a commit that referenced this issue Nov 13, 2018
* [#4245] Allowing password to nil

* Set encrypted password to nil if password is nil

* [#4245] Fixing the build

* Removed unnecessary code
@tegon
Copy link
Member

tegon commented Nov 13, 2018

Closed via #4261

@tegon tegon closed this as completed Nov 13, 2018
mracos added a commit that referenced this issue Mar 25, 2019
mracos added a commit that referenced this issue Mar 25, 2019
mracos added a commit that referenced this issue Mar 26, 2019
mracos added a commit that referenced this issue Mar 26, 2019
tegon added a commit that referenced this issue Mar 26, 2019
…d-password-to-nil-if-password-is-nil

Reverts both "[#4245] Allow password to nil (#4261)" and "Add more tests (#4970)"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants