search

getgrav / grav-plugin-email

Grav Email Plugin
http://getgrav.org
MIT License
37 stars 29 forks source link

Saving config through admin overwrites SMTP password #134

Closed torohill closed 4 years ago

torohill commented 4 years ago

When the Email plugin settings are saved through the admin a value is always written to mailer.smtp.password even if no password is submitted. This means if the password is already set and I go back and edit the Email plugin settings again, but don't re-enter a password, an empty value will be saved over the existing password.

Reproduction steps on Grav v1.6.25, Admin v1.9.14, and Email v3.0.8:

  1. Go to /admin/plugins/email in your browser, enter a value in the SMTP password field and save.
  2. Check the mailer.smtp.password field in user/config/plugin/email.yaml and confirm the password you entered was saved.
  3. Go back to /admin/plugins/email, change the host field because you made a mistake entering it last time, but don't re-enter the password, and then save.
  4. Check the mailer.smtp.password field in user/config/plugin/email.yaml and see that it is set to null.

Maybe there needs to be additional logic when saving password fields to avoid this issue. A couple of thoughts:

I'm not sure if this should actually be filed on the Admin plugin (I checked and didn't see an existing issue there), as I'm guessing this issue affects all password fields in the admin, and not just the SMTP password field.

torohill commented 4 years ago

I also tested with Grav v1.7.0-rc.11, Admin v1.10.0-rc.11, and Email v3.0.8 and found the same behaviour.

Should this issue be filed on the admin plugin instead of here?

rhukster commented 4 years ago

Fixed for next release

torohill commented 4 years ago

@rhukster Looks good.

What do you think should happen in the (far less common) case of someone wanting to save an empty password when there is already an existing password? Is it worth creating another issue for that?

ricardo118 commented 4 years ago

edit the files since validation on the admin plugin wont let you save it empty anyone (now that the bug is fixed)