diff --git a/app/email/headers.py b/app/email/headers.py index 3edb4dce..5d176ff9 100644 --- a/app/email/headers.py +++ b/app/email/headers.py @@ -19,6 +19,7 @@ DKIM_SIGNATURE = "DKIM-Signature" X_SPAM_STATUS = "X-Spam-Status" LIST_UNSUBSCRIBE = "List-Unsubscribe" LIST_UNSUBSCRIBE_POST = "List-Unsubscribe-Post" +RETURN_PATH = "Return-Path" # headers used to DKIM sign in order of preference DKIM_HEADERS = [ diff --git a/app/email_utils.py b/app/email_utils.py index 47e46e2c..f52ee8e6 100644 --- a/app/email_utils.py +++ b/app/email_utils.py @@ -1409,7 +1409,9 @@ def generate_verp_email( ).lower() -def get_verp_info_from_email(email: str) -> Optional[Tuple[VerpType, int]]: +def get_verp_info_from_email( + email: str, validate_time: bool = True +) -> Optional[Tuple[VerpType, int]]: """This method processes the email address, checks if it's a signed verp email generated by us to receive bounces and extracts the type of verp email and associated email log id/transactional email id stored as object_id """ @@ -1433,6 +1435,8 @@ def get_verp_info_from_email(email: str) -> Optional[Tuple[VerpType, int]]: # verp type, object_id, time if len(data) != 3: return None - if data[2] > (time.time() + VERP_MESSAGE_LIFETIME - VERP_TIME_START) / 60: + if validate_time and ( + data[2] > (time.time() + VERP_MESSAGE_LIFETIME - VERP_TIME_START) / 60 + ): return None return VerpType(data[0]), data[1] diff --git a/app/handler/provider_complaint.py b/app/handler/provider_complaint.py index a0711aa6..5ba2ec9f 100644 --- a/app/handler/provider_complaint.py +++ b/app/handler/provider_complaint.py @@ -20,6 +20,7 @@ from app.email_utils import ( send_email_with_rate_control, parse_address_list, get_header_unicode, + get_verp_info_from_email, ) from app.log import LOG from app.models import ( @@ -32,25 +33,44 @@ from app.models import ( Phase, ProviderComplaintState, RefusedEmail, + VerpType, + EmailLog, + Mailbox, ) @dataclass -class OriginalAddresses: - sender: str - recipient: str +class OriginalMessageInformation: + sender_address: str + rcpt_address: str + mailbox_address: Optional[str] class ProviderComplaintOrigin(ABC): @classmethod @abstractmethod - def get_original_addresses(cls, message: Message) -> Optional[OriginalAddresses]: + def get_original_addresses( + cls, message: Message + ) -> Optional[OriginalMessageInformation]: pass @classmethod - def sanitize_addresses( + def _get_mailbox_id(cls, return_path: Optional[str]) -> Optional[Mailbox]: + if not return_path: + return None + _, return_path = parse_full_address(get_header_unicode(return_path)) + verp_type, email_log_id = get_verp_info_from_email(return_path) + if verp_type == VerpType.transactional: + return None + email_log = EmailLog.get_by(id=email_log_id) + if email_log: + return email_log.mailbox.email + return None + + @classmethod + def sanitize_addresses_and_extract_mailbox_id( cls, rcpt_header: Optional[str], message: Message - ) -> Optional[OriginalAddresses]: + ) -> Optional[OriginalMessageInformation]: """ If the rcpt_header is not None, use it as the valid rcpt address, otherwise try to extract it from the To header of the original message, since in the original message there can be more than one recipients. @@ -65,8 +85,15 @@ class ProviderComplaintOrigin(ABC): LOG.w(f"Cannot find rcpt. Saved to {saved_file or 'nowhere'}") return None rcpt_address = rcpt_list[0][1] - _, sender_address = parse_full_address(message[headers.FROM]) - return OriginalAddresses(sender_address, rcpt_address) + _, sender_address = parse_full_address( + get_header_unicode(message[headers.FROM]) + ) + + return OriginalMessageInformation( + sender_address, + rcpt_address, + cls._get_mailbox_id(message[headers.RETURN_PATH]), + ) except ValueError: saved_file = save_email_for_debugging(message, "ComplaintOriginalAddress") LOG.w(f"Cannot parse from header. Saved to {saved_file or 'nowhere'}") @@ -105,7 +132,9 @@ class ProviderComplaintYahoo(ProviderComplaintOrigin): return None @classmethod - def get_original_addresses(cls, message: Message) -> Optional[OriginalAddresses]: + def get_original_addresses( + cls, message: Message + ) -> Optional[OriginalMessageInformation]: """ Try to get the proper recipient from the report that yahoo adds as a port of the complaint. If we cannot find the rcpt in the report or we can't find the report, use the first address in the original message from @@ -113,7 +142,7 @@ class ProviderComplaintYahoo(ProviderComplaintOrigin): report = cls.get_feedback_report(message) original = cls.get_original_message(message) rcpt_header = report["original-rcpt-to"] - return cls.sanitize_addresses(rcpt_header, original) + return cls.sanitize_addresses_and_extract_mailbox_id(rcpt_header, original) @classmethod def name(cls): @@ -134,13 +163,15 @@ class ProviderComplaintHotmail(ProviderComplaintOrigin): return None @classmethod - def get_original_addresses(cls, message: Message) -> Optional[OriginalAddresses]: + def get_original_addresses( + cls, message: Message + ) -> Optional[OriginalMessageInformation]: """ Try to get the proper recipient from original x-simplelogin-envelope-to header we add on delivery. If we can't find the header, use the first address in the original message from""" original = cls.get_original_message(message) rcpt_header = original["x-simplelogin-envelope-to"] - return cls.sanitize_addresses(rcpt_header, original) + return cls.sanitize_addresses_and_extract_mailbox_id(rcpt_header, original) @classmethod def name(cls): @@ -164,50 +195,55 @@ def find_alias_with_address(address: str) -> Optional[Alias]: def handle_complaint(message: Message, origin: ProviderComplaintOrigin) -> bool: - addresses = origin.get_original_addresses(message) - if not addresses: + msg_info = origin.get_original_addresses(message) + if not msg_info: return False - user = User.get_by(email=addresses.recipient) + user = User.get_by(email=msg_info.rcpt_address) if user: LOG.d(f"Handle provider {origin.name()} complaint for {user}") - report_complaint_to_user_in_transactional_phase(user, origin) + report_complaint_to_user_in_transactional_phase(user, origin, msg_info) return True - alias = find_alias_with_address(addresses.sender) + alias = find_alias_with_address(msg_info.sender_address) # the email is during a reply phase, from=alias and to=destination if alias: LOG.i( - f"Complaint from {origin.name} during reply phase {alias} -> {addresses.recipient}, {user}" + f"Complaint from {origin.name} during reply phase {alias} -> {msg_info.rcpt_address}, {user}" + ) + report_complaint_to_user_in_reply_phase( + alias, msg_info.rcpt_address, origin, msg_info ) - report_complaint_to_user_in_reply_phase(alias, addresses.recipient, origin) store_provider_complaint(alias, message) return True - contact = Contact.get_by(reply_email=addresses.sender) + contact = Contact.get_by(reply_email=msg_info.sender_address) if contact: alias = contact.alias else: - alias = find_alias_with_address(addresses.recipient) + alias = find_alias_with_address(msg_info.rcpt_address) if not alias: LOG.e( - f"Cannot find alias for address {addresses.recipient} or contact with reply {addresses.sender}" + f"Cannot find alias for address {msg_info.rcpt_address} or contact with reply {msg_info.sender_address}" ) return False - report_complaint_to_user_in_forward_phase(alias, origin) + report_complaint_to_user_in_forward_phase(alias, origin, msg_info) return True def report_complaint_to_user_in_reply_phase( - alias: Alias, to_address: str, origin: ProviderComplaintOrigin + alias: Alias, + to_address: str, + origin: ProviderComplaintOrigin, + msg_info: OriginalMessageInformation, ): capitalized_name = origin.name().capitalize() send_email_with_rate_control( alias.user, f"{ALERT_COMPLAINT_REPLY_PHASE}_{origin.name()}", - alias.user.email, + msg_info.mailbox_address or alias.mailbox.email, f"Abuse report from {capitalized_name}", render( "transactional/provider-complaint-reply-phase.txt.jinja2", @@ -222,13 +258,13 @@ def report_complaint_to_user_in_reply_phase( def report_complaint_to_user_in_transactional_phase( - user: User, origin: ProviderComplaintOrigin + user: User, origin: ProviderComplaintOrigin, msg_info: OriginalMessageInformation ): capitalized_name = origin.name().capitalize() send_email_with_rate_control( user, f"{ALERT_COMPLAINT_TRANSACTIONAL_PHASE}_{origin.name()}", - user.email, + msg_info.mailbox_address or user.email, f"Abuse report from {capitalized_name}", render( "transactional/provider-complaint-to-user.txt.jinja2", @@ -246,23 +282,24 @@ def report_complaint_to_user_in_transactional_phase( def report_complaint_to_user_in_forward_phase( - alias: Alias, origin: ProviderComplaintOrigin + alias: Alias, origin: ProviderComplaintOrigin, msg_info: OriginalMessageInformation ): capitalized_name = origin.name().capitalize() user = alias.user + mailbox_email = msg_info.mailbox_address or alias.mailbox.email send_email_with_rate_control( user, f"{ALERT_COMPLAINT_FORWARD_PHASE}_{origin.name()}", - user.email, + mailbox_email, f"Abuse report from {capitalized_name}", render( "transactional/provider-complaint-forward-phase.txt.jinja2", - user=user, + email=mailbox_email, provider=capitalized_name, ), render( "transactional/provider-complaint-forward-phase.html", - user=user, + email=mailbox_email, provider=capitalized_name, ), max_nb_alert=1, diff --git a/pytest.ini b/pytest.ini index c0f5472c..3d362baf 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,5 +1,5 @@ [pytest] -xaddopts = +addopts = --cov --cov-config coverage.ini --cov-report=html:htmlcov diff --git a/reset_local_db.sh b/reset_local_db.sh new file mode 100755 index 00000000..f194f335 --- /dev/null +++ b/reset_local_db.sh @@ -0,0 +1,7 @@ +#!/bin/sh + +export DB_URI=postgresql://myuser:mypassword@localhost:15432/simplelogin +echo 'drop schema public cascade; create schema public;' | psql $DB_URI + +poetry run alembic upgrade head +poetry run flask dummy-data diff --git a/reset_test_db.sh b/reset_test_db.sh new file mode 100755 index 00000000..ce392919 --- /dev/null +++ b/reset_test_db.sh @@ -0,0 +1,6 @@ +#!/bin/sh + +export DB_URI=postgresql://myuser:mypassword@localhost:15432/test +echo 'drop schema public cascade; create schema public;' | psql $DB_URI + +poetry run alembic upgrade head diff --git a/templates/emails/transactional/provider-complaint-forward-phase.html b/templates/emails/transactional/provider-complaint-forward-phase.html index a1502b1f..cefeedaf 100644 --- a/templates/emails/transactional/provider-complaint-forward-phase.html +++ b/templates/emails/transactional/provider-complaint-forward-phase.html @@ -6,7 +6,7 @@ {% endcall %} {% call text() %} - {{ provider }} has informed us about an email sent to {{ user.email }} that might have been considered as spam, + {{ provider }} has informed us about an email sent to {{ email }} that might have been considered as spam, either by you or by {{ provider }} spam filter. {% endcall %} diff --git a/templates/emails/transactional/provider-complaint-forward-phase.txt.jinja2 b/templates/emails/transactional/provider-complaint-forward-phase.txt.jinja2 index 428674e1..b76c6714 100644 --- a/templates/emails/transactional/provider-complaint-forward-phase.txt.jinja2 +++ b/templates/emails/transactional/provider-complaint-forward-phase.txt.jinja2 @@ -5,7 +5,7 @@ Hi, This is SimpleLogin team. -{{ provider }} has informed us about an email sent to {{ user.email }} that might have been considered as spam, +{{ provider }} has informed us about an email sent to {{ email }} that might have been considered as spam, either by you or by {{ provider }}. Please note that explicitly marking a SimpleLogin's forwarded email as Spam diff --git a/tests/handler/test_provider_complaints.py b/tests/handler/test_provider_complaints.py index 5437343a..8452cd71 100644 --- a/tests/handler/test_provider_complaints.py +++ b/tests/handler/test_provider_complaints.py @@ -8,12 +8,19 @@ from app.config import ( POSTMASTER, ) from app.db import Session -from app.email import headers +from app.email_utils import generate_verp_email from app.handler.provider_complaint import ( handle_hotmail_complaint, handle_yahoo_complaint, ) -from app.models import Alias, ProviderComplaint, SentAlert +from app.models import ( + Alias, + ProviderComplaint, + SentAlert, + EmailLog, + VerpType, + Contact, +) from tests.utils import create_new_user, load_eml_file origins = [ @@ -23,13 +30,28 @@ origins = [ def prepare_complaint( - provider_name: str, rcpt_address: str, sender_address: str + provider_name: str, alias: Alias, rcpt_address: str, sender_address: str ) -> Message: + contact = Contact.create( + user_id=alias.user.id, + alias_id=alias.id, + website_email="a@b.c", + reply_email="d@e.f", + commit=True, + ) + elog = EmailLog.create( + user_id=alias.user.id, + mailbox_id=alias.user.default_mailbox_id, + contact_id=contact.id, + commit=True, + bounced=True, + ) + return_path = generate_verp_email(VerpType.bounce_forward, elog.id) return load_eml_file( f"{provider_name}_complaint.eml", { "postmaster": POSTMASTER, - "return_path": "sl.something.other@simplelogin.co", + "return_path": return_path, "rcpt": rcpt_address, "sender": sender_address, "rcpt_comma_list": f"{rcpt_address},other_rcpt@somwhere.net", @@ -40,7 +62,9 @@ def prepare_complaint( @pytest.mark.parametrize("handle_ftor,provider", origins) def test_provider_to_user(flask_client, handle_ftor, provider): user = create_new_user() - complaint = prepare_complaint(provider, user.email, "nobody@nowhere.net") + alias = Alias.create_new_random(user) + Session.commit() + complaint = prepare_complaint(provider, alias, user.email, "nobody@nowhere.net") assert handle_ftor(complaint) found = ProviderComplaint.filter_by(user_id=user.id).all() assert len(found) == 0 @@ -54,7 +78,7 @@ def test_provider_forward_phase(flask_client, handle_ftor, provider): user = create_new_user() alias = Alias.create_new_random(user) Session.commit() - complaint = prepare_complaint(provider, "nobody@nowhere.net", alias.email) + complaint = prepare_complaint(provider, alias, "nobody@nowhere.net", alias.email) assert handle_ftor(complaint) found = ProviderComplaint.filter_by(user_id=user.id).all() assert len(found) == 1 @@ -68,12 +92,7 @@ def test_provider_reply_phase(flask_client, handle_ftor, provider): user = create_new_user() alias = Alias.create_new_random(user) Session.commit() - original_message = Message() - original_message[headers.TO] = alias.email - original_message[headers.FROM] = "no@no.no" - original_message.set_payload("Contents") - - complaint = prepare_complaint(provider, alias.email, "no@no.no") + complaint = prepare_complaint(provider, alias, alias.email, "no@no.no") assert handle_ftor(complaint) found = ProviderComplaint.filter_by(user_id=user.id).all() assert len(found) == 0