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

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

Conversation

timkrins
Copy link

If the password attribute was set to a blank or nil, this was also setting the encrypted_password to nil (password_digest returns nil for a blank)

This means that calling update on a model with a blank password always sets encrypted_password to nil, which usually errors as a database constraint issue.

Introduced by https://github.com/plataformatec/devise/pull/4261/files#diff-19b4dd928714c72f1338874351e8ff2dR40

Related to #5033 and #5038

@timkrins
Copy link
Author

Hmm, need to determine thought behind original change. (reasons for keeping 'password' in sync with encrypted_password)

@JulienItard
Copy link

Hello, indeed now i have database constraint issue PG::NotNullViolation: ERROR: null value in column "encrypted_password" violates not-null constraint

@hayesr
Copy link

hayesr commented Mar 20, 2019

Also having the problem @JulienItard is having in my specs. I'm not setting a blank password, so I'm not sure if this is really the issue, but others are pointing to it: #4981 (comment)

Edit: I thought this was just a factory issue in my spec, but it is in fact trying to update a user record without changing their password.

@hayesr
Copy link

hayesr commented Mar 20, 2019

Other relevant info:
#5033
#4261

@hayesr
Copy link

hayesr commented Mar 20, 2019

@timkrins My bet is that the original thought was to allow for changing other attributes on user records without changing the password.

@timkrins
Copy link
Author

timkrins commented Mar 21, 2019

@hayesr yeah, I understand the original change now.

The problem is that devise users will expect the encrypted_password attribute not to change, even though they have called update with a blank password, as this is what it did before.

@NikoRoberts
Copy link

NikoRoberts commented Mar 22, 2019

Yeah everyone has to put additional logic in to get the same behaviour now. It was a very common pattern to rely on Devise for the logic to ignore a blank password update.

It looks based on allow_blank: true that this was an intentional feature.

I’m not sure if this behaviour was taken into account when the PR was made.
#4245

@NikoRoberts
Copy link

@timkrins regarding the impetus for the change. Why would password need to be in sync with encrypted password if it isn’t persisted?
The point of the accessor is that it is only used as a stepping stone for validation and should never represent what is in encrypted password.

@tegon
Copy link
Member

tegon commented Mar 25, 2019

We're going to revert the change since there were no real exploits based on the attributes being unsynced. We thought this would be a simple change but since it's causing so much trouble for existing applications, we're going to revert it.
See #5033 (comment)

Thanks.

@pirj pirj mentioned this pull request Oct 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants