MPP-3119: complaint notification disables mask #5045

groovecoder · 2024-09-19T19:04:11Z

Disable mask on complaint notification

See https://mozilla-hub.atlassian.net/browse/MPP-3119 for more context.

When a user (or their email provider) marks a Relay email as spam, we receive a complaint notification. To prevent further spam from reaching the user (causing more complaints), we want to disable the email mask that forwarded the spam email, and notify the user.

Screenshot (if applicable)

How to test

This will be hard to test until it's on a server with fully operational AWS SES. When it is:

Sign up for Relay with an FXA that has a gmail, yahoo, or outlook primary email address
In the django admin site, activate the new disable_mask_on_complaint flag for the user
Create an email mask
Send an email to that mask
In the final destination mailbox, mark that email as spam
- Note: It's unclear if all email providers send these complaint notifications, but gmail, yahoo, outlook, and other popular providers seem to do so.
- Note 2: It's also unclear how long it takes for AWS to process complaint notifications and send them to Relay
At some point, Relay should receive the complaint notification
- Verify the mask is disabled
- Verify the user received an email notification that their mask was disabled

Checklist (Definition of Done)

Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
Customer Experience team has seen or waived a demo of functionality.
All acceptance criteria are met.
Jira ticket has been updated (if needed) to match changes made during the development process.
~~[x] I've added or updated relevant docs in the docs/ directory~~
Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.
All UI revisions follow the coding standards, and use Protocol tokens where applicable (see /frontend/src/styles/tokens.scss).
Commits in this PR are minimal and have descriptive commit messages.
l10n changes have been submitted to the l10n repository, if any.

groovecoder · 2024-09-19T19:10:29Z

emails/views.py

@@ -1634,6 +1645,22 @@ def _handle_complaint(message_json: AWS_SNSMessageJSON) -> HttpResponse:
        profile.auto_block_spam = True
        profile.save()

+    for destination_address in message_json.get("mail", {}).get("destination", []):
+        try:
+            address = _get_address(destination_address, False)


Note: I changed the function call signature on this to accept a new bool parameter to control if the function will/won't create the necessary DomainAddress object.

There was enough logic in the _get_address function that I didn't want to re-write it. I considered re-factoring into 2 separate _get_address and _get_or_create_address functions, but that would require changing all 17 calls _get_address, which seemed like a more disruptive change.

jwhitlock

Thank @groovecoder, looks good. The required changes are the same line:

Move the code inside the loop, where user is set and so you can handle multiple recipients
Use flag_is_active_in_task to correctly handle everyone=True

emails/templates/emails/disabled_mask_for_spam.html

emails/views.py

emails/tests/views_tests.py

emails/views.py

jwhitlock · 2024-09-24T15:22:43Z

emails/tests/fixtures/disabled_mask_for_spam_expected.email

+                </h2>
+                <p style=3D"line-height: 200%; margin-bottom: 30px; padding: =
+20px 60px 20px 60px">
+                Firefox Relay received a spam complaint for an email sent to =


🤔 interesting, if the ftl_id ends with -html, FTL does not add the unicode FSI and PDI codepoints. Maybe we need a { -brand-name-firefox-relay-html } for these cases?

jwhitlock · 2024-09-24T15:24:15Z

emails/tests/fixtures/disabled_mask_for_spam_expected.email

+                </p>=20
+                <a href=3D"http://127.0.0.1:8000/accounts/profile/#w41fwbt4q%=
+40test.com">
+                    Visit your =E2=81=A8Firefox Relay=E2=81=A9 dashboard to r=


Here's what the FSI and PDI codepoints look like in the HTML. The convert_fsi_to_span filter changes these to <span dir="auto">...</span>, but it can be a pain to use.

say-yawn

Thanks for getting this code worked on so quickly. I have some questions on why the create parameter was added. I have some additional recommendations that will definitely scope creep:

Add a field on the mask that the mask was blocked due to complaint. This can later on be used to filter mask for better UX and/or measuring how many masks were set to block in this way.
Consider using a threshold to decide complaint resulting in blocking all emails. FxA tallies the number of complaint and bounces before they stop sending emails.

emails/tests/views_tests.py

say-yawn · 2024-09-24T16:18:26Z

emails/views.py

+    If an unknown email address is for a valid subdomain, and create is True,
+    a new DomainAddress will be created.


(question, blocking) Can you explain why this was added? When will the create be set to False?

(suggestion, blocking) Also, if the create parameter is only used to get/create DomainAddress logic, I recommend making the create parameter more descriptive like create_domainaddress.

This: #5045 (comment)

This other comment is where create is set to False, and the comment is right: it's "handling for getting a complaint on a DomainAddress that does not exist".

Let me consider how to address this.

emails/views.py

say-yawn · 2024-09-24T16:34:18Z

emails/views.py

+def _disable_masks_for_complaint(message_json: dict) -> None:
+    for destination_address in message_json.get("mail", {}).get("destination", []):
+        try:
+            address = _get_address(destination_address, False)


(question, blocking) Why would the create for DomainAddress be set to False? Is this handling for getting a complaint on a DomainAddress that does not exist? If so, I think a different error should be raised from ObjectDoesNotExist.

Yes, it's meant to handle if we ever receive a complaint for a DomainAddress that doesn't exist. E.g.:

User creates a domain address

User receives some email thru the address

User deletes the domain address from Relay

User sends a spam complaint about a past email sent thru the domain address

In this scenario, we don't want to re-create the old domain address.

It's raising ObjectDoesNotExist just like _handle_received and _reply_allowed and the other code in _get_domain_address. What other error do you think it should raise?

(suggestion, non-blocking) DomainAddress no longer re-creates deleted DomainAddress:

fx-private-relay/emails/validators.py

Line 75 in 4cde843

def valid_address(address: str, domain: str, subdomain: str | None = None) -> bool:

If we are handling for getting a complaint on a DomainAddress or RelayAddress that never existed I think throwing a SuspiciousOperations or ValidationError so it is not responding with bad request and separately handle and log in _disable_masks_for_complaint as:

logger.error( "Received a complaint from a destination address that never existed as " "a Relay address.", )

emails/views.py

jwhitlock

Thanks for cleaning up the code. I still think _handle_complaint could be better. I'd support the get_address rename instead of the new parameter, if you are ambitious.

Also, the expected email goes to no one, which is either a bug in the new code or the test setup, and should be fixed.

emails/tests/fixtures/disabled_mask_for_spam_expected.email

emails/views.py

groovecoder · 2024-09-27T16:09:47Z

I made 75e4d85 to address #5045 (comment).

Need to dig into #5045 (comment).

jwhitlock

I think the blank "To" is problematic, but we're also behind a feature flag, so I'm OK getting some code into main and iterating.

emails/views.py

emails/tests/fixtures/disabled_mask_for_spam_expected.email

emails/views.py

…ents loop To ensure the Relay mask belongs to the user from whom we received the spam complaint, moves _disable_masks_for_complaint() into complained_recipients loop. Also, change _disable_masks_for_complaint to search the original mail "source" field for a Relay mask address to find the mask thru which the spam email was sent.

…real email address

say-yawn

I agree with what @jwhitlock said about this being behind a feature flag, so I'm OK getting some code into main and iterating. But I made some suggestions that can be included now or in the future.

Scope creep UX idea:
I am wondering if it would be helpful to mention how many emails were blocked for the address when the service automatically blocks the email so the user can know they are getting more emails from that mask and make decision to re-enable it later.

say-yawn · 2024-09-27T18:29:16Z

emails/views.py

+        try:
+            address = _get_address(mask_address, False)
+            # ensure the mask belongs to the user for whom Relay received a complaint
+            if address.user != user:


Suggested change

if address.user != user:

if address.user != user or address.enabled == False:

(suggestion, non-blocking) Should we send the disabled mask email if the mask was already set to blocked? This can happen when when complaints was filed after the mask was already set to block by us or by the user.

say-yawn · 2024-09-27T18:41:41Z

emails/views.py

+def _disable_masks_for_complaint(message_json: dict) -> None:
+    for destination_address in message_json.get("mail", {}).get("destination", []):
+        try:
+            address = _get_address(destination_address, False)


(suggestion, non-blocking) DomainAddress no longer re-creates deleted DomainAddress:

fx-private-relay/emails/validators.py

Line 75 in 4cde843

def valid_address(address: str, domain: str, subdomain: str | None = None) -> bool:

If we are handling for getting a complaint on a DomainAddress or RelayAddress that never existed I think throwing a SuspiciousOperations or ValidationError so it is not responding with bad request and separately handle and log in _disable_masks_for_complaint as:

logger.error( "Received a complaint from a destination address that never existed as " "a Relay address.", )

…he mask

groovecoder commented Sep 19, 2024

View reviewed changes

groovecoder force-pushed the block-all-emails-thru-mask-after-spam-complaint-mpp-3119 branch 7 times, most recently from 7e17853 to fc1c28d Compare September 24, 2024 12:15

groovecoder marked this pull request as ready for review September 24, 2024 12:44

groovecoder added 3 commits September 24, 2024 07:48

MPP-3119: complaint notification disables mask

6faaa8b

MPP-3119: add disabled_mask_for_spam email

a156885

MPP-3119: add disable_mask_on_complaint waffle flag

324695d

groovecoder force-pushed the block-all-emails-thru-mask-after-spam-complaint-mpp-3119 branch from edd78ff to 324695d Compare September 24, 2024 12:48

groovecoder self-assigned this Sep 24, 2024

jwhitlock self-requested a review September 24, 2024 14:03

groovecoder changed the title ~~MPP-3119: wip complaint notification disables mask~~ MPP-3119: complaint notification disables mask Sep 24, 2024

jwhitlock requested changes Sep 24, 2024

View reviewed changes

MPP-3119: use flag_is_active_in_task and move to check each address user

5d2e79d

say-yawn requested changes Sep 24, 2024

View reviewed changes

say-yawn reviewed Sep 24, 2024

View reviewed changes

emails/views.py Outdated Show resolved Hide resolved

jwhitlock requested changes Sep 24, 2024

View reviewed changes

emails/tests/fixtures/disabled_mask_for_spam_expected.email Outdated Show resolved Hide resolved

emails/views.py Outdated Show resolved Hide resolved

jwhitlock approved these changes Sep 27, 2024

View reviewed changes

emails/views.py Outdated Show resolved Hide resolved

emails/tests/fixtures/disabled_mask_for_spam_expected.email Outdated Show resolved Hide resolved

emails/views.py Show resolved Hide resolved

groovecoder force-pushed the block-all-emails-thru-mask-after-spam-complaint-mpp-3119 branch from 75e4d85 to ab264a3 Compare September 27, 2024 17:26

MPP-3119: fix test_build_disabled_mask_for_spam_email user to have a …

53855a3

…real email address

say-yawn approved these changes Sep 27, 2024

View reviewed changes

groovecoder added 2 commits September 30, 2024 13:08

MPP-3119: skip sending disabled mask email if user already disabled t…

7011fe3

…he mask

MPP-3119: add waffle_flag_by_fxa_uid command

add4afd

jwhitlock approved these changes Sep 30, 2024

View reviewed changes

groovecoder added this pull request to the merge queue Sep 30, 2024

Merged via the queue into main with commit 153a920 Sep 30, 2024
29 checks passed

groovecoder deleted the block-all-emails-thru-mask-after-spam-complaint-mpp-3119 branch September 30, 2024 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MPP-3119: complaint notification disables mask #5045

MPP-3119: complaint notification disables mask #5045

groovecoder commented Sep 19, 2024 •

edited

Loading

groovecoder Sep 19, 2024

jwhitlock left a comment

jwhitlock Sep 24, 2024

jwhitlock Sep 24, 2024

say-yawn left a comment

say-yawn Sep 24, 2024

groovecoder Sep 25, 2024

say-yawn Sep 24, 2024

groovecoder Sep 25, 2024

say-yawn Sep 27, 2024

jwhitlock left a comment

groovecoder commented Sep 27, 2024

jwhitlock left a comment

say-yawn left a comment

say-yawn Sep 27, 2024

say-yawn Sep 27, 2024

		If an unknown email address is for a valid subdomain, and create is True,
		a new DomainAddress will be created.

	if address.user != user:
	if address.user != user or address.enabled == False:

MPP-3119: complaint notification disables mask #5045

MPP-3119: complaint notification disables mask #5045

Conversation

groovecoder commented Sep 19, 2024 • edited Loading

Disable mask on complaint notification

Screenshot (if applicable)

How to test

Checklist (Definition of Done)

Choose a reason for hiding this comment

jwhitlock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

say-yawn 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

jwhitlock left a comment

Choose a reason for hiding this comment

groovecoder commented Sep 27, 2024

jwhitlock left a comment

Choose a reason for hiding this comment

say-yawn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder commented Sep 19, 2024 •

edited

Loading