MPP-3932: Disable masks on complaints, round 2 #5115

jwhitlock · 2024-10-16T23:44:30Z

This replaces #5114, and goes a lot farther.

This rewrites _handle_complaint, moving most of the logic into three new helper functions:

_get_complaint_data gets the relevant data out of the complaint notification, with lots of checks and logging if the message is not what we're expecting.
_gather_complainers cross-checks the sender and receiver data against the database, getting the related users and masks (RelayAddress, DomainAddress). It also checks for the developer "simulate_complaint" mode, and swaps the real user email back in.
_reduce_future_complaints takes the actions to reduce future complaints.
- On the first complaint, turns on the auto_block_spam flag to drop emails that AWS SES identifies as spam (same as before)
- On the second complaint, it turns the associated mask to block emails and sends an email to the user. This has been altered to look at the From: header data, which has the mask, rather than the source field, which is our reply address.
- On future complaints (how?), it does nothing but takes note that it did nothing (new)

This leaves _handle_complaint to log the complaint in developer mode, log what happened for each detected Relay user, and emit a counter.

I also extended the developer mode to correctly work for domain masks, added _get_address_if_exists, removed a mypy override, and fixed Heroku Redis... This PR can definitely be broken into smaller bits.

How to test:

The test suite has been expanded to cover the new code. There are lots of new tests.

I pushed this branch to Heroku for manual testing. There are two issues ~~I've seen~~ that are now fixed on Heroku:

Each forwarded email appears to result in two complaints - Misconfiguration of SES on dev server
In the email "Firefox Relay⁩ has deactivated one of your email masks", there's some extra ??? and the links do not work. - Fixed the HTML template

Here's the test process:

Deploy this branch
Send a test email to a mask. It should be forwarded.
Load the Papertrail app in Heroku. Filter on program:"app/worker.1". You'll see logs related to your email.
Add the developer_mode flag to your Heroku user. You can run a Heroku bash shell:
heroku run --app fx-private-relay bash
then run
./manage.py waffle_flag developer_mode --append --user "<[email protected]>"
Create a random mask, give it the description "DEV:simulate_complaint" (no spaces)
Send an email to that mask
- The email is not delivered to you, but instead to the AWS SES Complaint Simulator address
- You should see several logs with "msg": "_handle_received: developer_mode". The "notification_gza85" fields can be combined and passed to emails.utils.decode_dict_gza85 to get the original delivery notification.
- After a few more messages, you should see a log with "msg": "_handle_complaint: developer mode". The "notification_gza85" fields can be combined and passed to emails.utils.decode_dict_gza85 to get the original complaint notification.
- The next log is "msg": "complaint_notification", and has the standard log for a complaint. The "relay_action" should be "auto_block_spam"
- There will be other logs as well, but these are the important ones.
Send a second email to that mask
- The logs repeat, with "relay_action" as no_action since auto_block_spam is already on.
Add the disable_mask_on_complaint flag to your Heroku user. You can run a Heroku bash shell:
heroku run --app fx-private-relay bash
then run
./manage.py waffle_flag disable_mask_on_complaint --append --user "<[email protected]>"
Send a third email to that mask.
- The logs repeat. The "relay_action" is "disable_mask" (and then no_action if there is a second complaint)
Check your email after a minute
- You should get the email "Firefox Relay⁩ has deactivated one of your email masks." to your real address. The links should go to the email dashboard.
Go to the Email Dashboard, refreshing if needed (such as going to FAQ and back to the dashboard)
- The mask is now set to Blocking

jwhitlock

Whoa this is big for just 3 files. You should probably skim emails/tests/views_tests.py, and focus on emails/views.py. Here's some comments that might help.

emails/tests/views_tests.py

emails/views.py

jwhitlock · 2024-10-17T00:02:58Z

pyproject.toml

@@ -58,7 +58,6 @@ module = [
    "django_ftl",
    "django_ftl.bundles",
    "google.*",
-    "google_measurement_protocol",


This is the only line I meant to change in here. The rest were done by my editor's TOML formatter.

jwhitlock · 2024-10-17T16:57:51Z

There are indeed 2 complaints for each time the simulator is used on dev. For one pair, the first has a timestamp and arrivalDate of 2024-10-16T23:15:27.100Z. For the second, the timestamp was "timestamp": "2024-10-16T23:15:27.000Z (0.1 seconds earlier) and arrivalDate was 2024-10-16T23:15:27.167Z (0.06 seconds later). The other differences

The first had eventType: Complaint and the second had notificationType: Complaint
The second had mail.sourceIP and mail.callerIdentity, the first did not
The first had mail.commonHeaders.messageId, the second did not
The first had mail.tags with several items list ses:source-tls-version, the second did not.

I'm going to see if there is a similar pattern in production complaints. If not, this may be a dev misconfiguration, or a quirk of the simulator. It looks like we might want to ignore either eventType or notificationType.

jwhitlock · 2024-10-17T19:18:47Z

No, production does not have this pattern.

Digging in, there are two ways to monitor complaints:

Set up event publishing with a configuration set, and specify this configuration set when sending email. This will send "eventType": "Complaint" objects to your configured destination (SNS -> SQS for us)
Set up a domain identity for your email sending domain, and configure Email feedback forwarding. This will send "notificationType": "Complaint" objects to your configured destination (SNS -> SQS for us).

We have both configured on the development server. Looking at my notes, it is possible I enabled this, as I was looking into complaint formats in May 2022. I turned off email feedback forwarding of complaints for our dev domain identity. This will also turn off duplicate bounce notifications and the unhandled delivery notifications.

On stage, we have not configured the SNS topics for the email feedback forwarding, no changes there.

groovecoder

Great update! I haven't looked at the test file yet, and it looks like you updated it again since I started the review, but here's my first few comments.

groovecoder · 2024-10-17T20:26:29Z

emails/views.py

+        if key in source:
+            return source[key], True
+        logger.error(
+            "_get_complaint_data: Unexpected message",


suggestion (non-blocking): this seems more like "unexpected message format" - i.e., a message that's missing a key we expect, rather than an unexpected message?

Suggested change

"_get_complaint_data: Unexpected message",

"_get_complaint_data: Unexpected message format",

groovecoder · 2024-10-17T20:57:57Z

emails/views.py

+    # Only present for feedback reports
+    user_agent = complaint.get("userAgent", "")


question (non-blocking): does this mean we can use the userAgent field or value to know whether the end user or their email service provider marked the email as spam?

Nope. The agent is the mail provider's agent. All complaints from yahoo.com, ymail.com, yahoo.com.mx, yahoo.de, etc. have agent Yahoo!-Mail-Feedback/2.0. A lot use ReturnPathFBL/2.0. Microsoft leaves it blank. See the link "Relay Bounces and Complaints v2" on the Relay Engineering page on Confluence.

groovecoder · 2024-10-17T20:59:13Z

emails/views.py

+
+    T = TypeVar("T")
+
+    def get_or_log(


comment (non-blocking): This felt almost generic enough to try to turn into a more general re-usable function. But after a few minutes with ChatGPT, I gave up trying to refactor it.

groovecoder · 2024-10-17T21:03:18Z

emails/views.py

+    for email_address, extra_data in complaint_data.complained_recipients:
+        local, domain = email_address.split("@", 1)
+
+        # For developer mode complaint simulation, swap with developer's email


nit (non-blocking): this code seems specific to AWS complaint simulator, but not necessarily to the "developer mode" controlled by the mask label?

If it is, can we link from here to a doc/comment about the "developer mode"?

Suggested change

# For developer mode complaint simulation, swap with developer's email

# For AWS complaint simulation, swap with developer's email

groovecoder

All comments are "dupes" of 4 main issues:

suggestion (non-blocking): When I woke up this morning I had to remember what the new "developer mode" did. It would be great to copy some of the explanation from that PR into a more permanent doc or comment in the code-base somewhere.

suggestion (non-blocking): Similarly, a lot of the doc-blocks for the new tests assume high-context knowledge, which we may not have next time we have to touch this code. So I made some suggestions for updating the doc-blocks to add more context.

correction (blocking): A couple of the test doc-blocks have copy-pasted incorrect text.

question (blocking): It looks like, unlike the code in process_emails_from_sqs, this complaint-handling code is run in the context of an HTTP callback request from AWS SNS. What does SNS do if it receives a 404 from Relay? Answer

privaterelay/settings.py

groovecoder · 2024-10-18T15:03:49Z

emails/tests/views_tests.py

+
+    @override_flag("developer_mode", active=True)
+    def test_complaint_developer_mode_missing_mask(self):
+        """If the simulator email can not be turned into a mask, it is not changed."""


sugguestion (non-blocking): add a little more detail here? I don't understand what "simulator email can not be turned into a mask" means.

I'm going to move these "cover all the edge cases" tests to a new class, so ComplaintHandlingTest is more like integration tests.

groovecoder · 2024-10-18T15:04:35Z

emails/tests/views_tests.py

+
+    @override_flag("developer_mode", active=True)
+    def test_complaint_developer_mode_domain_address(self):
+        """If the simulator email can not be turned into a mask, it is not changed."""


same sugguestion (non-blocking): add a little more detail here? I don't understand what "simulator email can not be turned into a mask" means.

I'm moving these tests of _gather_complainers out of ComplaintHandlingTest into a focused test case.

groovecoder · 2024-10-18T15:16:54Z

emails/tests/views_tests.py

+        assert rec2.msg == "complaint_notification"
+
+    def test_complaint_no_complained_recipients_error_logged(self):
+        """Without complained recipients, an error is logged, but matches on From"""


suggestion (non-blocking): I had to read thru the code and test fixture to understand what this test was doing.

Suggested change

"""Without complained recipients, an error is logged, but matches on From"""

"""

Without complainedRecipients, we can still match the user on From:,

perform the appropriate action, and log an error

"""

groovecoder · 2024-10-18T15:18:44Z

emails/tests/views_tests.py

+        )
+
+    def test_complaint_empty_complained_recipients_error_logged(self):
+        """With empty complained recipients, an error is logged, but matches on From"""


same suggestion (non-blocking):

Suggested change

"""With empty complained recipients, an error is logged, but matches on From"""

"""

With complainedRecipients empty, we can still match the user on From:,

perform the appropriate action, and log an error

"""

emails/tests/views_tests.py

The "source" was the mask, but AWS SES uses the generic Reply-To address. The messageId is set, probably by SES when sending. The null values in commonHeaders are not present. These changes break the mask disable test, which uses the "source". It is marked as an expected failure for now.

If _get_address is called with create=False, pass create=False when calling _get_domain_address. When create=False, _get_domain_address logs less for bad domains, subdomains, etc. It raises DomainAddress.DoesNotExist(). The new _get_address_if_exists calls _get_address(create=False), catches the expected exceptions, and returns None. Add tests for existing and new behavior.

Rewrite _handle_complaint to merge user collection and mask processing. This changes how complaints are handled: * On the first complaint, the auto_block_spam flag is set * On the second complaint, the mask is blocked * If the complaint comes from the AWS SES complaint simulator, swap in the developer's email address for the simulator email address * Log an error if the JSON message is missing mail.commonHeaders.from * Log what action was taken: auto_block_spam, disable_mask, no_action * Log if a mask was found when disabling the mask

Rather than initializing waffle flags each time they are used, initialize once in _sns_message.

This will match '_handle_received: developer_mode'.

jwhitlock · 2024-10-21T20:08:58Z

@groovecoder, this should be ready for recheck.

Main changes:

Added docs/developer_mode.md
Use the metrics ID in the complaint simulator email, like [email protected]. Added _get_mask_by_metrics_id function and GetMaskByMetricsIdTest test case.
Added GetComplaintDataTest test case to test _get_complaint_data, and GatherComplainersTest test case to test _gather_complainers. These now have "happy path" tests, as well as the edge case tests to get full coverage. This makes these tests more focused and the test data easier to understand. I've moved the similar tests out of ComplaintHandlingTest, leaving it to test the most important end-to-end test cases. I could do something similar for _reduce_further_complaints, but that seemed unneeded.

Smaller changes:

Added a log_group_id to the developer_mode log, to make it a bit easier to get all the logs for a long notification.
Updated docstrings and comments as suggested
Rebase on main

groovecoder

Looks good! 1 small change to the doc file name to make it easier to find for future ENGRs (including us).

groovecoder · 2024-10-24T15:49:57Z

emails/tests/views_tests.py

@@ -1325,6 +1414,532 @@ def test_build_disabled_mask_for_spam_email(self):
        )


+class GetComplaintDataTest(TestCase):


praise: this is a great test that spells out all the scenarios very clearly. thanks!

groovecoder · 2024-10-24T15:52:18Z

emails/tests/views_tests.py

+        assert getattr(err_log, "found_keys") == "complainedRecipients"
+
+
+class GatherComplainersTest(TestCase):


praise: another clear test case. I was able to quickly scan the docstrings to understand each test.

emails/views.py

jwhitlock requested a review from groovecoder October 16, 2024 23:44

jwhitlock commented Oct 17, 2024

View reviewed changes

groovecoder reviewed Oct 17, 2024

View reviewed changes

groovecoder requested changes Oct 18, 2024

View reviewed changes

jwhitlock mentioned this pull request Oct 21, 2024

Add setting for Redis TLS with self-signed certs #5131

Merged

jwhitlock added 23 commits October 21, 2024 14:59

Move dev logging inside loop

4f50b0a

Log complaints from unknown users

dbd2dfc

Detect complaint simulator

4a70554

Drop unused original_spam_email arg

8b96430

Remove google-measurement-protocol

6802e40

Customize metrics for disabled email

d6d6760

Add init_waffle_flags to init once

7951e00

Rather than initializing waffle flags each time they are used, initialize once in _sns_message.

Extract _get_complaint_data

15ddc13

Keep developer mode, remove Jira reference

31785a7

Test missing complainedRecipients

b34f2ef

Test, fix error cases in _get_complaint_data

1f0edce

Remove ComplaintData structure

e90e5b1

Convert to count

a3ddf5f

Remove UserComplaintData

6c12303

Extract _gather_complainers

63b8b68

Track where the complainer was found

f83e040

Fix, test error conditions in _gather_complainers

0bce33b

Extract _reduce_future_complaints

f363691

Test more complaint error cases

b53a466

Update docs, remove found_in from metrics

c279650

jwhitlock added 13 commits October 21, 2024 14:59

Allow domain addresses for complaint simulation

ce69d60

Use full URLs for links

93e9af8

Remove unused translation

775f2c5

Update expected email for disabled mask

b8eabdb

Use '_handle_complaint: developer_mode' for logs

81bd1d0

This will match '_handle_received: developer_mode'.

Add docs for developer mode

5293485

Use mask metrics_id in simulator address

beeb796

Add log_group_id

50ec4ac

Add GetComplaintDataTest, move missing key tests

ac1688e

Change error log message

39262e5

Add GatherComplainersTest, move tests

363282e

Rewrite comment on SES simulator

888c93f

Add tests for _get_mask_by_metrics_id

a0b19f9

jwhitlock force-pushed the fix-dev-complaint-log-mpp-3932 branch from 1bd8030 to a0b19f9 Compare October 21, 2024 20:00

jwhitlock requested a review from groovecoder October 21, 2024 20:09

groovecoder requested changes Oct 24, 2024

View reviewed changes

Rename to standard developer_mode.md

7e1163b

jwhitlock requested a review from groovecoder October 24, 2024 16:41

groovecoder approved these changes Oct 24, 2024

View reviewed changes

groovecoder added this pull request to the merge queue Oct 24, 2024

Merged via the queue into main with commit 9cd189e Oct 24, 2024
29 checks passed

groovecoder deleted the fix-dev-complaint-log-mpp-3932 branch October 24, 2024 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MPP-3932: Disable masks on complaints, round 2 #5115

MPP-3932: Disable masks on complaints, round 2 #5115

jwhitlock commented Oct 16, 2024 •

edited

Loading

jwhitlock left a comment

jwhitlock Oct 17, 2024

jwhitlock commented Oct 17, 2024

jwhitlock commented Oct 17, 2024

groovecoder left a comment

groovecoder Oct 17, 2024

groovecoder Oct 17, 2024

jwhitlock Oct 21, 2024

groovecoder Oct 17, 2024

groovecoder Oct 17, 2024

groovecoder left a comment •

edited

Loading

groovecoder Oct 18, 2024

jwhitlock Oct 21, 2024

groovecoder Oct 18, 2024

jwhitlock Oct 21, 2024

groovecoder Oct 18, 2024

groovecoder Oct 18, 2024

jwhitlock commented Oct 21, 2024

groovecoder left a comment

groovecoder Oct 24, 2024

groovecoder Oct 24, 2024

	"_get_complaint_data: Unexpected message",
	"_get_complaint_data: Unexpected message format",

		# Only present for feedback reports
		user_agent = complaint.get("userAgent", "")

	# For developer mode complaint simulation, swap with developer's email
	# For AWS complaint simulation, swap with developer's email

-        """Without complained recipients, an error is logged, but matches on From"""
+        """
+        Without complainedRecipients, we can still match the user on From:,
+        perform the appropriate action, and log an error
+        """

		@@ -1325,6 +1414,532 @@ def test_build_disabled_mask_for_spam_email(self):
		)


		class GetComplaintDataTest(TestCase):

		assert getattr(err_log, "found_keys") == "complainedRecipients"


		class GatherComplainersTest(TestCase):

MPP-3932: Disable masks on complaints, round 2 #5115

MPP-3932: Disable masks on complaints, round 2 #5115

Conversation

jwhitlock commented Oct 16, 2024 • edited Loading

How to test:

jwhitlock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhitlock commented Oct 17, 2024

jwhitlock commented Oct 17, 2024

groovecoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhitlock commented Oct 21, 2024

groovecoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhitlock commented Oct 16, 2024 •

edited

Loading

groovecoder left a comment •

edited

Loading