From e363917e9cc52cc9738210996ffb44ffa1d96bdb Mon Sep 17 00:00:00 2001 From: Klaas van Schelven Date: Mon, 28 Jul 2025 14:41:32 +0200 Subject: [PATCH] Per-month quota for email-sending Fix #34 --- bugsink/app_settings.py | 2 ++ bugsink/context_processors.py | 11 ++++++- bugsink/settings/development.py | 2 ++ bugsink/tests.py | 29 +++++++++++++++++++ bugsink/transaction.py | 13 +++++++-- bugsink/utils.py | 14 +++++++++ .../0002_installation_email_quota_usage.py | 18 ++++++++++++ phonehome/models.py | 27 +++++++++++++++++ 8 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 phonehome/migrations/0002_installation_email_quota_usage.py diff --git a/bugsink/app_settings.py b/bugsink/app_settings.py index 0374017..e51c97e 100644 --- a/bugsink/app_settings.py +++ b/bugsink/app_settings.py @@ -59,6 +59,8 @@ DEFAULTS = { "MAX_EVENTS_PER_PROJECT_PER_5_MINUTES": 1_000, "MAX_EVENTS_PER_PROJECT_PER_HOUR": 5_000, + "MAX_EMAILS_PER_MONTH": None, # None means "no limit"; for non-None values, the quota is per calendar month + # I don't think Sentry specifies this one, but we do: given the spec 8KiB should be enough by an order of magnitude. "MAX_HEADER_SIZE": 8 * _KIBIBYTE, diff --git a/bugsink/context_processors.py b/bugsink/context_processors.py index 6c96408..f797596 100644 --- a/bugsink/context_processors.py +++ b/bugsink/context_processors.py @@ -1,8 +1,9 @@ +import json from datetime import timedelta from collections import namedtuple -from django.conf import settings from django.utils import timezone +from django.conf import settings from django.utils.safestring import mark_safe from django.urls import reverse from django.contrib.auth.models import AnonymousUser @@ -119,6 +120,14 @@ def useful_settings_processor(request): system_warnings.append(SystemWarning(EMAIL_BACKEND_WARNING, ignore_url)) + if get_settings().MAX_EMAILS_PER_MONTH is not None: + email_quota_usage = json.loads(installation.email_quota_usage) + this_month_usage = email_quota_usage.get("per_month", {}).get(timezone.now().strftime("%Y-%m"), 0) + if this_month_usage >= get_settings().MAX_EMAILS_PER_MONTH: + system_warnings.append(SystemWarning( + f"Bugsink has sent {this_month_usage} emails this month, which is the maximum. " + "No more emails will be sent until the 1st of next month.", None)) + return system_warnings + get_snappea_warnings() return { diff --git a/bugsink/settings/development.py b/bugsink/settings/development.py index a9c2c26..f8e7127 100644 --- a/bugsink/settings/development.py +++ b/bugsink/settings/development.py @@ -98,6 +98,8 @@ BUGSINK = { "MAX_EVENTS_PER_PROJECT_PER_5_MINUTES": 1_000_000, "MAX_EVENTS_PER_PROJECT_PER_HOUR": 50_000_000, + "MAX_EMAILS_PER_MONTH": 10, # for development: a thing to tune if you want to the the quota system + "KEEP_ARTIFACT_BUNDLES": True, # in development: useful to preserve sourcemap uploads } diff --git a/bugsink/tests.py b/bugsink/tests.py index 7dbcb02..cc4dccd 100644 --- a/bugsink/tests.py +++ b/bugsink/tests.py @@ -7,13 +7,20 @@ from unittest import TestCase as RegularTestCase from django.test import TestCase as DjangoTestCase from django.test import override_settings from django.core.exceptions import SuspiciousOperation +from django.contrib.auth import get_user_model +from django.test.utils import CaptureQueriesContext +from django.db import connection from .wsgi import allowed_hosts_error_message +from .test_utils import TransactionTestCase25251 as TransactionTestCase +from .transaction import immediate_atomic from .volume_based_condition import VolumeBasedCondition from .streams import ( compress_with_zlib, GeneratorReader, WBITS_PARAM_FOR_GZIP, WBITS_PARAM_FOR_DEFLATE, MaxDataReader, MaxDataWriter, zlib_generator, brotli_generator) +User = get_user_model() + def apply_n(f, n, v): for i in range(n): @@ -421,3 +428,25 @@ class AllowedHostsMsgTestCase(DjangoTestCase): "'Host: teeestserver' as sent by browser/proxy not in ALLOWED_HOSTS=['testserver']. " "Add 'teeestserver' to ALLOWED_HOSTS or configure proxy to use 'Host: testserver'.", allowed_hosts_error_message("teeestserver", ["testserver"])) + + +class TestAtomicTransactions(TransactionTestCase): + + def test_only_if_needed(self): + with CaptureQueriesContext(connection) as queries_context: + with immediate_atomic(only_if_needed=True): + User.objects.create(username="testuser", password="testpass") + + self.assertTrue(User.objects.filter(username="testuser").exists()) + self.assertEquals([1], [1 for q in queries_context.captured_queries if q['sql'].startswith("BEGIN")]) + self.assertEquals([1], [1 for q in queries_context.captured_queries if q['sql'].startswith("COMMIT")]) + + with CaptureQueriesContext(connection) as queries_context: + with immediate_atomic(only_if_needed=True): + with immediate_atomic(only_if_needed=True): + with immediate_atomic(only_if_needed=True): + User.objects.create(username="testuser2", password="testpass2") + + self.assertTrue(User.objects.filter(username="testuser2").exists()) + self.assertEquals([1], [1 for q in queries_context.captured_queries if q['sql'].startswith("BEGIN")]) + self.assertEquals([1], [1 for q in queries_context.captured_queries if q['sql'].startswith("COMMIT")]) diff --git a/bugsink/transaction.py b/bugsink/transaction.py index 5129e58..29e5c2d 100644 --- a/bugsink/transaction.py +++ b/bugsink/transaction.py @@ -192,8 +192,10 @@ class ImmediateAtomic(SuperDurableAtomic): @contextmanager -def immediate_atomic(using=None, savepoint=True, durable=True): +def immediate_atomic(using=None, savepoint=True, only_if_needed=False): # this is the Django 4.2 db.transaction.atomic, but using ImmediateAtomic, and with durable=True by default + # only_if_needed is useful if you want to wrap some code in an as tight-as-possible transaction to avoid hogging the + # global write lock, while not worrying about whether the code is already in a transaction or not. # the following assertion is because "BEGIN IMMEDIATE" supposes a "BEGIN" (of a transaction), i.e. has no meaning # when this wrapper is not the outermost one. @@ -201,8 +203,15 @@ def immediate_atomic(using=None, savepoint=True, durable=True): # Side-note: the parameter `savepoint` is a bit of a misnomer, it is not about "is the current thing a savepoint", # but rather, "are savepoints allowed inside the current context". (The former would imply that it could never be # combined with durable=True, which is not the case.) - assert durable, "immediate_atomic should always be used with durable=True" + + durable = True # for the cases where only_if_needed is False or not applicable (i.e. 'needed') durable=True. + using = DEFAULT_DB_ALIAS if using is None else using # harmonize to "default" at the top for downstream lookups + connection = django_db_transaction.get_connection(using) + + if only_if_needed and connection.in_atomic_block: + yield + return if callable(using): immediate_atomic = ImmediateAtomic(DEFAULT_DB_ALIAS, savepoint, durable)(using) diff --git a/bugsink/utils.py b/bugsink/utils.py index def5795..991e6e9 100644 --- a/bugsink/utils.py +++ b/bugsink/utils.py @@ -1,6 +1,8 @@ +import logging from collections import defaultdict from urllib.parse import urlparse +from django.utils import timezone from django.core.mail import EmailMultiAlternatives from django.template.loader import get_template from django.apps import apps @@ -8,8 +10,20 @@ from django.db.models import ForeignKey, F from .version import version +logger = logging.getLogger("bugsink.email") + def send_rendered_email(subject, base_template_name, recipient_list, context=None): + from phonehome.models import Installation + + if not Installation.check_and_inc_email_quota(timezone.now()): + logger.warning( + "Email quota exceeded; not sending email with subject '%s' to %s", + subject, + recipient_list, + ) + return + if context is None: context = {} diff --git a/phonehome/migrations/0002_installation_email_quota_usage.py b/phonehome/migrations/0002_installation_email_quota_usage.py new file mode 100644 index 0000000..1392f1a --- /dev/null +++ b/phonehome/migrations/0002_installation_email_quota_usage.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.21 on 2025-07-28 12:04 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("phonehome", "0001_b_squashed_initial"), + ] + + operations = [ + migrations.AddField( + model_name="installation", + name="email_quota_usage", + field=models.TextField(default='{"per_month": {}}'), + ), + ] diff --git a/phonehome/models.py b/phonehome/models.py index d51a57d..dbbcf20 100644 --- a/phonehome/models.py +++ b/phonehome/models.py @@ -1,6 +1,10 @@ +import json import uuid from django.db import models +from bugsink.transaction import immediate_atomic + +from bugsink.app_settings import get_settings class Installation(models.Model): @@ -12,6 +16,29 @@ class Installation(models.Model): silence_email_system_warning = models.BooleanField(default=False) + email_quota_usage = models.TextField(null=False, default='{"per_month": {}}') + + @classmethod + @immediate_atomic(only_if_needed=True) # minimalize write-lock-hogging (while being callable within atomic blocks) + def check_and_inc_email_quota(cls, date): + obj = cls.objects.first() + + email_quota_usage = json.loads(obj.email_quota_usage) + + key = date.strftime('%Y-%m') + if key not in email_quota_usage["per_month"]: + email_quota_usage['per_month'] = {key: 0} # full overwrite: no need to keep old info around. + + if (get_settings().MAX_EMAILS_PER_MONTH is not None + and email_quota_usage['per_month'][key] >= get_settings().MAX_EMAILS_PER_MONTH): + return False + + email_quota_usage['per_month'][key] += 1 + + obj.email_quota_usage = json.dumps(email_quota_usage) + obj.save() + return True + class OutboundMessage(models.Model): attempted_at = models.DateTimeField(auto_now_add=True)