From e26287a4c78d5c2e7cb4777d622efe4fb9e3f1ab Mon Sep 17 00:00:00 2001 From: Son Nguyen Kim Date: Wed, 14 Jul 2021 12:23:02 +0200 Subject: [PATCH] Revert "disable should_disable() for now" This reverts commit fb88654d84424ebc95ae07d6c56fe8e5c1eaa028. --- app/email_utils.py | 187 ++++++++++++++------------- tests/test_email_utils.py | 261 +++++++++++++++++++------------------- 2 files changed, 225 insertions(+), 223 deletions(-) diff --git a/app/email_utils.py b/app/email_utils.py index 70656f3a..3583bfc8 100644 --- a/app/email_utils.py +++ b/app/email_utils.py @@ -13,12 +13,13 @@ from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText from email.utils import make_msgid, formatdate, parseaddr from smtplib import SMTP, SMTPServerDisconnected -from typing import Optional +from typing import Tuple, List, Optional import arrow import dkim import spf from jinja2 import Environment, FileSystemLoader +from sqlalchemy import func from validate_email import validate_email from app.config import ( @@ -55,6 +56,7 @@ from app.models import ( SLDomain, Contact, Alias, + EmailLog, TransactionalEmail, ) from app.utils import ( @@ -973,100 +975,97 @@ def should_disable(alias: Alias) -> bool: LOG.warning("%s cannot be disabled", alias) return False - # todo: remove - return False + yesterday = arrow.now().shift(days=-1) + nb_bounced_last_24h = ( + db.session.query(EmailLog) + .join(Contact, EmailLog.contact_id == Contact.id) + .filter( + EmailLog.bounced.is_(True), + EmailLog.is_reply.is_(False), + EmailLog.created_at > yesterday, + ) + .filter(Contact.alias_id == alias.id) + .count() + ) + # if more than 12 bounces in 24h -> disable alias + if nb_bounced_last_24h > 12: + LOG.d("more than 12 bounces in the last 24h, disable alias %s", alias) + return True - # yesterday = arrow.now().shift(days=-1) - # nb_bounced_last_24h = ( - # db.session.query(EmailLog) - # .join(Contact, EmailLog.contact_id == Contact.id) - # .filter( - # EmailLog.bounced.is_(True), - # EmailLog.is_reply.is_(False), - # EmailLog.created_at > yesterday, - # ) - # .filter(Contact.alias_id == alias.id) - # .count() - # ) - # # if more than 12 bounces in 24h -> disable alias - # if nb_bounced_last_24h > 12: - # LOG.d("more than 12 bounces in the last 24h, disable alias %s", alias) - # return True - # - # # if more than 5 bounces but has bounces last week -> disable alias - # elif nb_bounced_last_24h > 5: - # one_week_ago = arrow.now().shift(days=-8) - # nb_bounced_7d_1d = ( - # db.session.query(EmailLog) - # .join(Contact, EmailLog.contact_id == Contact.id) - # .filter( - # EmailLog.bounced.is_(True), - # EmailLog.is_reply.is_(False), - # EmailLog.created_at > one_week_ago, - # EmailLog.created_at < yesterday, - # ) - # .filter(Contact.alias_id == alias.id) - # .count() - # ) - # if nb_bounced_7d_1d > 1: - # LOG.debug( - # "more than 5 bounces in the last 24h and more than 1 bounces in the last 7 days, " - # "disable alias %s", - # alias, - # ) - # return True - # else: - # # alias level - # # if bounces at least 9 days in the last 10 days -> disable alias - # query = ( - # db.session.query( - # func.date(EmailLog.created_at).label("date"), - # func.count(EmailLog.id).label("count"), - # ) - # .join(Contact, EmailLog.contact_id == Contact.id) - # .filter(Contact.alias_id == alias.id) - # .filter( - # EmailLog.created_at > arrow.now().shift(days=-10), - # EmailLog.bounced.is_(True), - # EmailLog.is_reply.is_(False), - # ) - # .group_by("date") - # ) - # - # if query.count() >= 9: - # LOG.d( - # "Bounces every day for at least 9 days in the last 10 days, disable alias %s", - # alias, - # ) - # return True - # - # # account level - # query = ( - # db.session.query( - # func.date(EmailLog.created_at).label("date"), - # func.count(EmailLog.id).label("count"), - # ) - # .filter(EmailLog.user_id == alias.user_id) - # .filter( - # EmailLog.created_at > arrow.now().shift(days=-10), - # EmailLog.bounced.is_(True), - # EmailLog.is_reply.is_(False), - # ) - # .group_by("date") - # ) - # - # # if an account has more than 10 bounces every day for at least 4 days in the last 10 days, disable alias - # date_bounces: List[Tuple[arrow.Arrow, int]] = list(query) - # if len(date_bounces) > 4: - # if all([v > 10 for _, v in date_bounces]): - # LOG.d( - # "+10 bounces for +4 days in the last 10 days on %s, disable alias %s", - # alias.user, - # alias, - # ) - # return True - # - # return False + # if more than 5 bounces but has bounces last week -> disable alias + elif nb_bounced_last_24h > 5: + one_week_ago = arrow.now().shift(days=-8) + nb_bounced_7d_1d = ( + db.session.query(EmailLog) + .join(Contact, EmailLog.contact_id == Contact.id) + .filter( + EmailLog.bounced.is_(True), + EmailLog.is_reply.is_(False), + EmailLog.created_at > one_week_ago, + EmailLog.created_at < yesterday, + ) + .filter(Contact.alias_id == alias.id) + .count() + ) + if nb_bounced_7d_1d > 1: + LOG.debug( + "more than 5 bounces in the last 24h and more than 1 bounces in the last 7 days, " + "disable alias %s", + alias, + ) + return True + else: + # alias level + # if bounces at least 9 days in the last 10 days -> disable alias + query = ( + db.session.query( + func.date(EmailLog.created_at).label("date"), + func.count(EmailLog.id).label("count"), + ) + .join(Contact, EmailLog.contact_id == Contact.id) + .filter(Contact.alias_id == alias.id) + .filter( + EmailLog.created_at > arrow.now().shift(days=-10), + EmailLog.bounced.is_(True), + EmailLog.is_reply.is_(False), + ) + .group_by("date") + ) + + if query.count() >= 9: + LOG.d( + "Bounces every day for at least 9 days in the last 10 days, disable alias %s", + alias, + ) + return True + + # account level + query = ( + db.session.query( + func.date(EmailLog.created_at).label("date"), + func.count(EmailLog.id).label("count"), + ) + .filter(EmailLog.user_id == alias.user_id) + .filter( + EmailLog.created_at > arrow.now().shift(days=-10), + EmailLog.bounced.is_(True), + EmailLog.is_reply.is_(False), + ) + .group_by("date") + ) + + # if an account has more than 10 bounces every day for at least 4 days in the last 10 days, disable alias + date_bounces: List[Tuple[arrow.Arrow, int]] = list(query) + if len(date_bounces) > 4: + if all([v > 10 for _, v in date_bounces]): + LOG.d( + "+10 bounces for +4 days in the last 10 days on %s, disable alias %s", + alias.user, + alias, + ) + return True + + return False def parse_id_from_bounce(email_address: str) -> int: diff --git a/tests/test_email_utils.py b/tests/test_email_utils.py index a501e576..036ea7bd 100644 --- a/tests/test_email_utils.py +++ b/tests/test_email_utils.py @@ -1,6 +1,8 @@ import email from email.message import EmailMessage +import arrow + from app.config import MAX_ALERT_24H, EMAIL_DOMAIN, BOUNCE_EMAIL from app.email_utils import ( get_email_domain_part, @@ -22,15 +24,16 @@ from app.email_utils import ( encode_text, EmailEncoding, replace, + should_disable, decode_text, parse_id_from_bounce, get_queue_id, ) from app.extensions import db -from app.models import User, CustomDomain - +from app.models import User, CustomDomain, Alias, Contact, EmailLog # flake8: noqa: E101, W191 +from tests.utils import login def test_get_email_domain_part(): @@ -590,139 +593,139 @@ def test_decode_text(): ) -# def test_should_disable(flask_client): -# user = User.create( -# email="a@b.c", -# password="password", -# name="Test User", -# activated=True, -# include_sender_in_reverse_alias=True, -# ) -# alias = Alias.create_new_random(user) -# db.session.commit() -# -# assert not should_disable(alias) -# -# # create a lot of bounce on this alias -# contact = Contact.create( -# user_id=user.id, -# alias_id=alias.id, -# website_email="contact@example.com", -# reply_email="rep@sl.local", -# commit=True, -# ) -# for _ in range(20): -# EmailLog.create( -# user_id=user.id, -# contact_id=contact.id, -# alias_id=contact.alias_id, -# commit=True, -# bounced=True, -# ) -# -# assert should_disable(alias) -# -# # should not affect another alias -# alias2 = Alias.create_new_random(user) -# db.session.commit() -# assert not should_disable(alias2) +def test_should_disable(flask_client): + user = User.create( + email="a@b.c", + password="password", + name="Test User", + activated=True, + include_sender_in_reverse_alias=True, + ) + alias = Alias.create_new_random(user) + db.session.commit() + + assert not should_disable(alias) + + # create a lot of bounce on this alias + contact = Contact.create( + user_id=user.id, + alias_id=alias.id, + website_email="contact@example.com", + reply_email="rep@sl.local", + commit=True, + ) + for _ in range(20): + EmailLog.create( + user_id=user.id, + contact_id=contact.id, + alias_id=contact.alias_id, + commit=True, + bounced=True, + ) + + assert should_disable(alias) + + # should not affect another alias + alias2 = Alias.create_new_random(user) + db.session.commit() + assert not should_disable(alias2) -# def test_should_disable_bounces_every_day(flask_client): -# """if an alias has bounces every day at least 9 days in the last 10 days, disable alias""" -# user = login(flask_client) -# alias = Alias.create_new_random(user) -# db.session.commit() -# -# assert not should_disable(alias) -# -# # create a lot of bounce on this alias -# contact = Contact.create( -# user_id=user.id, -# alias_id=alias.id, -# website_email="contact@example.com", -# reply_email="rep@sl.local", -# commit=True, -# ) -# for i in range(9): -# EmailLog.create( -# user_id=user.id, -# contact_id=contact.id, -# alias_id=contact.alias_id, -# commit=True, -# bounced=True, -# created_at=arrow.now().shift(days=-i), -# ) -# -# assert should_disable(alias) +def test_should_disable_bounces_every_day(flask_client): + """if an alias has bounces every day at least 9 days in the last 10 days, disable alias""" + user = login(flask_client) + alias = Alias.create_new_random(user) + db.session.commit() + + assert not should_disable(alias) + + # create a lot of bounce on this alias + contact = Contact.create( + user_id=user.id, + alias_id=alias.id, + website_email="contact@example.com", + reply_email="rep@sl.local", + commit=True, + ) + for i in range(9): + EmailLog.create( + user_id=user.id, + contact_id=contact.id, + alias_id=contact.alias_id, + commit=True, + bounced=True, + created_at=arrow.now().shift(days=-i), + ) + + assert should_disable(alias) -# def test_should_disable_bounces_account(flask_client): -# """if an account has more than 10 bounces every day for at least 5 days in the last 10 days, disable alias""" -# user = login(flask_client) -# alias = Alias.create_new_random(user) -# -# db.session.commit() -# -# # create a lot of bounces on alias -# contact = Contact.create( -# user_id=user.id, -# alias_id=alias.id, -# website_email="contact@example.com", -# reply_email="rep@sl.local", -# commit=True, -# ) -# -# for day in range(6): -# for _ in range(10): -# EmailLog.create( -# user_id=user.id, -# contact_id=contact.id, -# alias_id=contact.alias_id, -# commit=True, -# bounced=True, -# created_at=arrow.now().shift(days=-day), -# ) -# -# alias2 = Alias.create_new_random(user) -# assert should_disable(alias2) +def test_should_disable_bounces_account(flask_client): + """if an account has more than 10 bounces every day for at least 5 days in the last 10 days, disable alias""" + user = login(flask_client) + alias = Alias.create_new_random(user) + + db.session.commit() + + # create a lot of bounces on alias + contact = Contact.create( + user_id=user.id, + alias_id=alias.id, + website_email="contact@example.com", + reply_email="rep@sl.local", + commit=True, + ) + + for day in range(6): + for _ in range(10): + EmailLog.create( + user_id=user.id, + contact_id=contact.id, + alias_id=contact.alias_id, + commit=True, + bounced=True, + created_at=arrow.now().shift(days=-day), + ) + + alias2 = Alias.create_new_random(user) + assert should_disable(alias2) -# def test_should_disable_bounce_consecutive_days(flask_client): -# user = login(flask_client) -# alias = Alias.create_new_random(user) -# db.session.commit() -# -# contact = Contact.create( -# user_id=user.id, -# alias_id=alias.id, -# website_email="contact@example.com", -# reply_email="rep@sl.local", -# commit=True, -# ) -# -# # create 6 bounce on this alias in the last 24h: alias is not disabled -# for _ in range(6): -# EmailLog.create( -# user_id=user.id, -# contact_id=contact.id, -# alias_id=contact.alias_id, -# commit=True, -# bounced=True, -# ) -# assert not should_disable(alias) -# -# # create 2 bounces in the last 7 days: alias should be disabled -# for _ in range(2): -# EmailLog.create( -# user_id=user.id, -# contact_id=contact.id, -# alias_id=contact.alias_id, -# commit=True, -# bounced=True, -# created_at=arrow.now().shift(days=-3), -# ) -# assert should_disable(alias) +def test_should_disable_bounce_consecutive_days(flask_client): + user = login(flask_client) + alias = Alias.create_new_random(user) + db.session.commit() + + contact = Contact.create( + user_id=user.id, + alias_id=alias.id, + website_email="contact@example.com", + reply_email="rep@sl.local", + commit=True, + ) + + # create 6 bounce on this alias in the last 24h: alias is not disabled + for _ in range(6): + EmailLog.create( + user_id=user.id, + contact_id=contact.id, + alias_id=contact.alias_id, + commit=True, + bounced=True, + ) + assert not should_disable(alias) + + # create 2 bounces in the last 7 days: alias should be disabled + for _ in range(2): + EmailLog.create( + user_id=user.id, + contact_id=contact.id, + alias_id=contact.alias_id, + commit=True, + bounced=True, + created_at=arrow.now().shift(days=-3), + ) + assert should_disable(alias) def test_parse_id_from_bounce():