Add config option for always sending emails from server email.

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

bleblan2
Review Board
release-3.0.x
4578
9216
175f7c2...
reviewboard, students

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.

  • 0
  • 0
  • 11
  • 0
  • 11
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

bleblan2
chipx86
  1. 
      
  2. 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.

  3. 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).
    
  4. 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.

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

    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)
     
     

    Missing a period at the end of the sentence.

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

    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)
     
     
     
     
     
     
     
     

    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
Review request changed

Change Summary:

Update to address comments on review.

Description:

~  

Emails get sent with the user's email address in the 'From' header. This can get those emails caught as spam. With the new option, the 'From' header is set to match the configured 'Sender e-mail address'.

  ~

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.

   
~  

The 'From' address used is also changed to be the settings.DEFAULT_FROM_EMAIL setting instead of the settings.SERVER_EMAIL setting which is inherited from django (root@localhost).

  ~

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.

Testing Done:

~  

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.

  ~

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.

Depends On:

+9216 - Change e-mail sending to use <Name> via Review Board when e-mail spoofing disabled.

Commit:

-58f32ae9494b22477b8072d8eb6aa38fd60caaac
+175f7c26cba55430839670d46a57febd06b1d686

Diff:

Revision 3 (+106 -19)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...