Fix changing email requires new password

[viddo]

Fixes #2650 (and #2770 while at it)

Same behaviour as on current account page

[viddo]

ping @Kartones can you please review since you did the last changes related to the new account page? Basically, only change password if any password field is set.

Although, from a security perspective I wonder I wonder if we really should allow changing email without the user confirming it and/or require current password?

cc @xavijam

[viddo]

cc @rochoa for reporting the issue

[Kartones]

• @juanignaciosl Google+ signed users don't even have a password :/

That's why current code flow (at user.rb) doesn't triggers any password change if the old password field comes nil, but does as soon as that one comes (and will trigger validation errors so UI can display proper messages).

With your change there would be no error feedback to the user if forgot to set any or two of the three password fields...

[viddo]

The behaviour you describe sounds great, but that is not how it works I'm afraid. With the change I did it displays the proper messages. I propose you checkout the branch and try yourself then.

Without this change you'll get the unwanted behaviour @rochoa reported

[Kartones]

Ok, I now read #2650 , the different title mislead me.

I would enforce the 3 fields only if attributes[:email].present? is true. But as will probably be always, the issue here is we should have a "new email" field, so if that field gets filled we trigger an email change. Right now is quite dangerous, without any confirmation we change the user email (and yes, that forces a password to be set, the issue).

[Kartones]

If it shows the messages in the ui I'm ok. Backend has tests which is the part that worries me really, other stuff is UI/UX so if works... fine by me :P

[juanignaciosl]

Google+ users have a random password for security reasons. If you need to reason about actual password existence you must user last_password_change_date, if it's not null it's because user changed it.

[viddo]

@Kartones @juanignaciosl so.. to sum up, what would you have me change here? Or are the changes OK to go as they are now?

[Cartofante]

Frontend tests were OK 👍 (details)

[Kartones]

Sorry for drifting out of the main conversation.

For me:

• Your change is ok, and we can go with that...
• Except that I'm not sure it covers Google scenario then. If I understood correctly, for detecting a "from Google first password change" we need last_password_change_date.nil? && (attributes[:new_password].present? || attributes[:confirm_password].present?) ? *
• We definetly need to document this at the user model, in the change password and/or validation code

[juanignaciosl]

There's a can_change_email method that "document" this ;-)

1026   def can_change_email
1027     return !self.google_sign_in || self.last_password_change_date.present?
1028   end

[viddo]

@juanignaciosl Looking at existing code I could do something similar as where this method is used on organization pages, i.e. disable input field and show message indicating that a password must be set:

But does it make sense to show "old password"? What do user.change_password require in this case/how does it behave?

[juanignaciosl]

Uhm. It looks like this haven't been brought from Central, where previous user account page belonged. Code suggestion:

def should_display_old_password?
  self.google_sign_in.nil? || !self.google_sign_in || !self.last_password_change_date.nil?
end

And validate_old_password should not validate the first password change of a Google user:

def validate_old_password(old_password)
  (self.class.password_digest(old_password, self.salt) == self.crypted_password) || (self.google_sign_in && self.last_password_change_date.nil?)
end

I think this is all you need, but please let me know if it doesn't work as expected.

[viddo]

Finally managed to set up staging to verify this. So with latest modifications to the user model. I've added a test case for the google sign-in to verify password change working.

So now the user would see a message why email can't be changed just yet, in addition to knowing that the account is linked with a google account:

Once a password is set the email and/or password can be changed as for a normal user:

Please review the code again and let me know if there is anything more that needs to be done, or if this is good to go, please, @juanignaciosl @Kartones @xavijam

[Kartones]

+1 looks great!

[juanignaciosl]

Just a note about this: in production we detected there's a rake task that passed through password change date and thus triggered last_password_change_date change. That's why it's done in confirmation in Central. 1.- Please check that you can sign up with Google a new user in staging and last_password_change is still nil after a minute or two. 2.- After deploying this to production, check it as well (both environments have different tasks running).

[viddo]

In staging:

irb(main):003:0> u.first.google_sign_in
=> false
irb(main):004:0> u.first.last_password_change_date
=> nil

[juanignaciosl]

That would mean it's not working, either you've taken a wrong user, or it's not deployed or something similar...

[viddo]

Btw, do you mean signup using google sign-in? Or both a normal sign-up and using the google sign-in?

[viddo]

(...the console out above was using the normal sign-up btw)

[viddo]

OK, re-created an user (using google sign-up this time), yields same result:

irb(main):007:0> u.google_sign_in
=> true
irb(main):008:0> u.last_password_change_date
=> nil

[juanignaciosl]

Well, google_sign_in is true, that's what scared me the most 😄

[viddo]

Ack, will merge & deploy, and reassure that it's the same for production afterwards.

[viddo]

FYI, confirmed

[juanignaciosl]

Great, thanks :)

[juanignaciosl]

👍 , but take a look at my comment about environments and deployments.