Devise: v4.6.0 breaks activeadmin forms that have `password` field

[gsar]

Environment

• Ruby 2.6.1
• Rails 5.2.2
• Devise 4.6.1

Current behavior

It appears that PR #4261 introduced a change where encrypted_password could be set to nil even when no attempt was made to change password. The relevant comment is here https://github.com/plataformatec/devise/pull/4261#discussion_r260024837

This results in user model forms that have a password field in activeadmin to break with a validation failure when the password field remains empty on submit:

ActiveRecord::NotNullViolation (PG::NotNullViolation: ERROR:  null value in column "encrypted_password" violates not-null constraint

Expected behavior

No attempt should be made to set encrypted_password to nil if the password field doesn't have a value. This was the case in devise versions prior to v4.6.0.

[gsar]

In case it helps others, I am using this workaround in the activeadmin pages that have an editable devise password field:

  before_save do |user|
    if user.will_save_change_to_encrypted_password?
      user.restore_encrypted_password! unless user.encrypted_password.present?
    end
  end

[zucler]

This is occurring in our application as well. In our case, it's happening when trying to update a user record as a part of minitest framework test suite. Please fix.

[chiperific]

Same issue here.
RailsAdmin form for user with password and password_confirmation fields won't submit, throws this error.

@gsar's monkey patch works.

[phlegx]

I have the same problem on my application. I want to update an account without changing the password but changing some other fields. I get a validation error: encrypted_password can't be blank

Parameters password and password_confirmation are set to nil on update. This has worked for me on all previous versions of devise 4.6.0.

[nosretep]

Broke for me too. Just add encrypted_password: "" and that should satisfy the not null, and is also the default value of "".

t.string :encrypted_password, null: false, default: "" from the migration

[phlegx]

@nosretep your solution breaks logins! If I submit an account form to update the account and leave password and password_confirmation empty, then the account should be updated without set encrypted_password to blank! If it get set to blank, then the account can no more login with his password!

[nosretep]

@phlegx works for me on my specs, login seems to have not broken for me. I'll keep on the lookout though.

[Alexey-Lukin]

In case it helps others, I am using this workaround in the activeadmin pages that have an editable devise password field:

  before_save do |user|
    if user.will_save_change_to_encrypted_password?
      user.restore_encrypted_password! unless user.encrypted_password.present?
    end
  end

before_save do |user|
  if user.will_save_change_to_encrypted_password?
    user.restore_encrypted_password! if user.encrypted_password.blank?
  end
end

[jeremywadsack]

This is affecting our application, _unrelated to ActiveAdmin_ (we don't use it). We have a form that allows an account admin to change a user's password through the UsersController (as opposed to the user changing their own password which runs through the RegistrationController). The form includes a password field — prior to updating Devise (to address CVE-2019-5421) leaving this field blank updated the record without changing the password. After the update it raises the ActiveRecord::NotNullViolation in the OP.

Specifically, this fails to update:

  def update
    if @user.update(user_params)
      ...
    end
  end

  def user_params
    params.require(:user).permit(%i[email password name])
  end

To work around this I've changed user_params to only include :password when the value is present.

  def user_params
    fields = %i[email name]
    fields << :password if params[:user][:password].present?
    params.require(:user).permit(fields)
  end

This is clearly a breaking change and I can't see how this doesn't affect every application that has a password field on the users#update form.

[NikoRoberts]

Also experiencing this with a custom form.

The patch on 4.6.0 was introduced for security from what I can tell so setting a password to blank would set the encrypted_password to nil because it was “expected” to do so and was a security risk not to.

As encrypted password is NOT NULL column, this isn’t the expected behaviour and I doubt ever was.

My understanding of the expected behaviour was that setting password to nil or blank would not update the users’ ability to access their account and was a common way for forms to not update the password field

You can see a more relevant discussion on #5044

[tegon]

Hi everyone, sorry for the delay.

The original change was meant to keep the attributes in sync (password and encrypted_password) - which seemed to make sense - and also because they being different could cause a security breach.
But since we didn't know for now of a real exploit and this update is breaking so many existing applications, we decided to revert the original change.
If someday in the future we find an exploit based on this, we can then think in another way to fix it without causing that much trouble or even include it in a major version.

We'll be releasing a patch version later today.

Thanks again and sorry for all the trouble.

[tegon]

Released in version 4.6.2. Please let us know if something is missing.

[OpakAlex]

Great works guys!
Really, what you can say to people who changed their application with your prev version?
Save God OSS.

[NikoRoberts]

Is that supposed to be sarcasm? (if yes, it isn't entirely clear. The internet is a very poor medium for sarcasm)

In the open source world having upstream dependencies without carefully assessing updates can be very dangerous.
Example: https://blog.cotten.io/hacking-node-js-may-i-have-this-repo-5c16bb6a0da7

Not to say that it isn't regrettable that you made a lot of changes because of that minor release.
I hope you removed your wiping the password field rather than making your encrypted_password column nullable.
If that is the case and nothing is failing, that is probably the best case scenario for you if that work is ever re-released in the future as a major version change.

[OpakAlex]

It's not a sarcasm man. Look around, it's real world not sarcasm.

Good to known about next major version thanks. Because to do work twice is boring.

[NikoRoberts]

I’m not a maintainer, so can’t say if the reason for the change was a real concern and if it will be addressed in the future. It was just mentioned in the PR to revert it that if this change did come back it would probably be part of a major release. Not a minor one.

In the economics world it is called “sunk costs”. If you do work and it gets thrown away, the cost is already paid so no use worrying about it.

[tegon]

Really, what you can say to people who changed their application with your prev version?

@OpakAlex You're asking what do to now if you already included the monkey patch in your application? If that's the case, I'd advise to remove it and update when you can. I say "when you can" because this will depend on your context. It might be more important for you to finish a feature and do this later but it's important to update later.

[tegon]

I’m not a maintainer, so can’t say if the reason for the change was a real concern and if it will be addressed in the future. It was just mentioned in the PR to revert it that if this change did come back it would probably be part of a major release. Not a minor one.

@NikoRoberts Yeah, that's correct. There are no guarantees that this will come back but if it does it will come in a major version and we'll do something to make the transition smoother, like emit deprecation warnings and write upgrade guides.

[OpakAlex]

@tegon maybe i am wrong, but fix application for gem version it's not monkey patch.
But if you say that prev gem version was incorrect this will be right, i did monkey patch. ;)

[OpakAlex]

Why you can not say, you did wrong merge? It's more easy, and everybody will understand, prev version was wrong. it's normal.

[tegon]

maybe i am wrong, but fix application for gem version it's not monkey patch.
But if you say that prev gem version was incorrect this will be right, i did monkey patch. ;)

@OpakAlex By monkey patch, I meant custom code to change a library behavior that better fits an application. I just looked up at the example and this case it wasn't needed to open any of the Devise's classes so I guess it's not monkey patch.

Why you can not say, you did wrong merge? It's more easy, and everybody will understand, prev version was wrong. it's normal.

I believe I explained the reasoning behind this on a comment above.

[kailanichu]

I know this issue is closed but I figured this might help some others that might encounter a problem we had. I'm not sure how our problem relates, but upgrading to the 4.6.2 release from 4.6.1 fixed it.

We have a name and email field for a user. The email field is required and the name field is only required under certain circumstances. Upgrading to 4.6.1 caused this name field to be autopopulated with the value of email if left to be nil when using factory bot. Upgrading to 4.6.2 then fixed the issue without any changes in our codebase.

Thanks for all your good work!

[chiperific]

In case it helps others, I am using this workaround in the activeadmin pages that have an editable devise password field:

  before_save do |user|
    if user.will_save_change_to_encrypted_password?
      user.restore_encrypted_password! unless user.encrypted_password.present?
    end
  end

before_save do |user|
  if user.will_save_change_to_encrypted_password?
    user.restore_encrypted_password! if user.encrypted_password.blank?
  end
end

Rubocop recommends not nesting a sole nested conditional:

before_save do |user|
    user.restore_encrypted_password! if user.will_save_change_to_encrypted_password? && user.encrypted_password.blank?
  end