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

Review Request #9216 — Created Sept. 24, 2017 and submitted

Information

Djblets
release-0.10.x
2179201...

Reviewers

When e-mail messages are created with enabled_smart_spoofing disabled,
the 'From' header should match a DMARC failure but instead the original
from_address is used.

This changes makes e-mail messages copy the same functionality as DMARC
not allowing spoofing when enable_smart_spoofing is disabled. This
changes the mail tests to expect the change in functionality when
enable_smart_spoofing is False.

This change is from the comments on the review for adding an option for
disabling e-mail smart spoofing (https://reviews.reviewboard.org/r/9209).

The full testing suite for djblets was run.

Description From Last Updated

Please add the bug number in the bugs field.

brenniebrennie

Can you run all of djblets tests?

brenniebrennie

This needs unit tests for the new conditionals. Right now, I'm not 100% sure that this doesn't regress functionality.

chipx86chipx86

Format as: if (not enable_smart_spoofing or parsed_from_email != parsed_sender_email and not is_email_allowed_by_dmarc(parsed_from_email)):

brenniebrennie

Use %-formatting: '"doc via example.com" <%s>' % settings.DEFAULT_FROM_EMAIL

brenniebrennie

Same here re: formatting.

brenniebrennie

The comments for all this logic no longer match the conditional, making it harder to understand the impact of the …

chipx86chipx86
brennie
  1. 
      
  2. Show all issues

    Can you run all of djblets tests?

  3. djblets/mail/message.py (Diff revision 1)
     
     
     
     
    Show all issues

    Format as:

    if (not enable_smart_spoofing or
        parsed_from_email != parsed_sender_email and            
        not is_email_allowed_by_dmarc(parsed_from_email)):
    
  4. djblets/mail/tests.py (Diff revision 1)
     
     
    Show all issues

    Use %-formatting:

    '"doc via example.com" <%s>' % settings.DEFAULT_FROM_EMAIL
    
  5. djblets/mail/tests.py (Diff revision 1)
     
     
    Show all issues

    Same here re: formatting.

  6. 
      
brennie
  1. 
      
  2. Show all issues

    Please add the bug number in the bugs field.

  3. 
      
bleblan2
brennie
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    This needs unit tests for the new conditionals. Right now, I'm not 100% sure that this doesn't regress functionality.

    1. I've added a test case for having enable_smart_spoofing=False. As for regressing functionality, that all depends on what the expected behaviour is for when enable_smart_spoofing is set to False.

  3. djblets/mail/message.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

    The comments for all this logic no longer match the conditional, making it harder to understand the impact of the change in behavior.

    1. Moving the check for enable_smart_spoofing was done to match the expected functionality I understood from your comment https://reviews.reviewboard.org/r/9209/#comment39603. Without this, passing in enable_smart_spoofing as False doesn't send the email as a <Name> via <service> but instead skips over the code at line 187 where build_email_address_via_service is called to avoid spoofing and sends with the original sender and from_email that were passed in.

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

  2. 
      
bleblan2
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.0.x (62f218d)