Add config option for always sending emails from server email.

Review Request #9209 — Created Sept. 22, 2017 and submitted

Information

Review Board
release-3.0.x
175f7c2...

Reviewers

Review Board tries to send e-mails on behalf of users by spoofing their
e-mail address, in order to offer better comment threads. While it tries
to be smart about when to do this (by checking DNS records to see whether
it can safely spoof the e-mail address), it's sometimes ideal to simply
turn off this behavior and send from the server's configured e-mail
address instead.

This change adds an option to force the server's e-mail address to be
used for all e-mails. When enabled, e-mails will still appear to come
from the user, but only by name (using "<Name> via Review Board
server@email" as the From address).

In order for this change to correctly send e-mails using the "<Name> via
Review Board server@email" format, the mail message in djblets must be
updated to send e-mails using that format when email_smart_spoofing is
set to False.

The notifications tests were updated to check that the email used is
settings.DEFAULT_FROM_EMAIL instead of settings.SERVER_EMAIL. The
reviewboard.notifications.tests suite was run. Tests added for new
review emails and review reply emails with enable_smart_spoofing
disabled.

Description From Last Updated

The description should wrap at 70-75 characters. I'd like to see some changes to how this is described. How about: …

chipx86chipx86

This change needs unit tests that cover the new option. There should be tests for loading/saving the option (based on …

chipx86chipx86

Not sure whether we should be switching the default to SERVER_EMAIL in this change. That will have an impact on …

chipx86chipx86

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

I'd suggest reversing this option and making it about sending on behalf of users, enabled by default. So something like …

chipx86chipx86

Missing a period at the end of the sentence.

chipx86chipx86

Blank line between these. You want one separating block-creating statements (like an if/else) from other code (like an assignment).

chipx86chipx86

EmailMessage takes an enable_smart_spoofing option, which you should turn off based on the setting. Otherwise, it's going to try to …

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

bleblan2
chipx86
  1. 
      
  2. Show all issues

    The description should wrap at 70-75 characters.

    I'd like to see some changes to how this is described. How about:

    Review Board tries to send e-mails on behalf of users by spoofing their e-mail
    address, in order to offer better comment threads. While it tries to be smart
    about when to do this (by checking DNS records to see whether it can safely
    spoof the e-mail address), it's sometimes ideal to simply turn off this behavior
    and send from the server's configured e-mail address instead.
    
    This change adds an option to force the server's e-mail address to be used for
    all e-mails. When enabled, e-mails will still appear to come from the user, but
    only by name (using "<Name> via Review Board <server@email>" as the From address).
    
  3. Show all issues

    This change needs unit tests that cover the new option. There should be tests for loading/saving the option (based on my suggestions in this review), and for sending e-mail with spoofing disabled.

  4. Show all issues

    Not sure whether we should be switching the default to SERVER_EMAIL in this change. That will have an impact on existing Review Board installs. Best to keep this change solely focused on the new option instead. In general, you want your changes to be heavily focused and not do "Also fixed ..." work (this isn't a black-and-white rule, but is a good goal).

    1. I added the change from settings.SERVER_EMAIL to settings.DEFAULT_FROM_EMAIL on @brennie's suggestion. According to @brennie, the root@localhost email is showing up on production.

  5. reviewboard/admin/forms.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    I'd suggest reversing this option and making it about sending on behalf of users, enabled by default. So something like mail_enable_smart_spoofing (to keep consistent with internal terminology and changing the label/help text accordingly.

    Right now, we actually have something in setting.py for this, which is EMAIL_ENABLE_SMART_SPOOFING. Some companies are going to have this turn on/off, so we should back the siteconfig value from this setting and sync both ways (look in the settings_map variable in admin/siteconfig.py).

  6. reviewboard/admin/forms.py (Diff revision 2)
     
     
    Show all issues

    Missing a period at the end of the sentence.

  7. reviewboard/notifications/email/message.py (Diff revision 2)
     
     
     
    Show all issues

    Blank line between these. You want one separating block-creating statements (like an if/else) from other code (like an assignment).

  8. reviewboard/notifications/email/message.py (Diff revision 2)
     
     
     
     
     
     
     
     
    Show all issues

    EmailMessage takes an enable_smart_spoofing option, which you should turn off based on the setting. Otherwise, it's going to try to see if it needs to spoof the default header instead.

  9. 
      
bleblan2
david
  1. Making some small updates and landing. Thanks!

  2. 
      
bleblan2
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.0.x (b75e73c)