- Download Diff

Summary:

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

Review Request #9216 — Created Sept. 24, 2017 and submitted Feb. 11, 2019, 11:19 a.m.

Information
Owner:	bleblan2
Repository:	Djblets
Branch:	release-0.10.x
Bugs:	4578
Depends On:
Blocks:	9209
Commit:	2179201...
Reviewers
Groups:	djblets, students
People:

Description

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

Testing Done

The full testing suite for djblets was run.

Issues

Description	From	Last Updated
Can you run all of djblets tests?	brennie	Oct. 3, 2017, 12:38 p.m.
Please add the bug number in the bugs field.	brennie	Oct. 1, 2017, 2:04 p.m.
This needs unit tests for the new conditionals. Right now, I'm not 100% sure that this doesn't regress functionality.	chipx86	Oct. 18, 2017, 6:28 p.m.
Format as: if (not enable_smart_spoofing or parsed_from_email != parsed_sender_email and not is_email_allowed_by_dmarc(parsed_from_email)):	brennie	Oct. 3, 2017, 12:30 p.m.
Use %-formatting: '"doc via example.com" <%s>' % settings.DEFAULT_FROM_EMAIL	brennie	Oct. 3, 2017, 12:22 p.m.
Same here re: formatting.	brennie	Oct. 3, 2017, 12:22 p.m.
The comments for all this logic no longer match the conditional, making it harder to understand the impact of the ...	chipx86	Oct. 18, 2017, 6:20 p.m.

flake8 passed.

JSHint passed.

Show all issues
```
Can you run all of djblets tests?
```

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)):

djblets/mail/tests.py (Diff revision 1)

Show all issues

Use %-formatting:

'"doc via example.com" <%s>' % settings.DEFAULT_FROM_EMAIL

djblets/mail/tests.py (Diff revision 1)
Show all issues
```
Same here re: formatting.
```

Show all issues

Please add the bug number in the bugs field.

Change Summary:

Fix formatting issues brought up in review comments.

Testing Done:

~		The testing suite for djblets mail was run.
	~	The full testing suite for djblets was run.

Commit:

-	7185029c7484540f459491e6c77146e19b8b449f
+	cf262ca146f17d7f493eae5f84ef86ba8e672faa

Diff:

Revision 2 (+13 -6)

Show changes

	djblets/mail/message.py
	djblets/mail/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Show all issues

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

bleblan2 Oct. 18, 2017, 6:31 p.m.

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.

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.

bleblan2 Oct. 18, 2017, 6:20 p.m.

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.

Change Summary:

Fix comments to reflect changes to code.

Commit:

-	cf262ca146f17d7f493eae5f84ef86ba8e672faa
+	779e35857216724e8a37361aaf34fb622d1932b3

Diff:

Revision 3 (+26 -16)

Show changes

	djblets/mail/message.py
	djblets/mail/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Add test case for enable_smart_spoofing=False.

Commit:

-	779e35857216724e8a37361aaf34fb622d1932b3
+	2179201c0db5dfbd7d4ed0a6741893b1f812ffd6

Diff:

Revision 4 (+59 -16)

Show changes

	djblets/mail/message.py
	djblets/mail/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

Making some small updates and landing. Thanks!

Status: Closed (submitted)

Change Summary:

Pushed to release-1.0.x (62f218d)

You have a pending review.

Review Board 4.0 alpha 0 (dev)

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

Change Summary:

Testing Done:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Status: Closed (submitted)

Change Summary: