diff --git a/.env.example b/.env.example index b1bee12..7edaa4b 100644 --- a/.env.example +++ b/.env.example @@ -90,9 +90,26 @@ DATA_UPLOAD_MAX_MEMORY_SIZE=67108864 SITE_ADDRESS=:80 # Mailbox shown to users on error pages and in the footer for support -# requests. +# requests — and the sole recipient of the software-request notification +# email (nothing ever emails a requester; the app stores no user addresses). SUPPORT_EMAIL=support@ilifu.ac.za +# --- Email ------------------------------------------------------------------ +# The SMTP relay the request notification goes out through. Dev and test +# never open an SMTP socket (console and locmem backends), so everything +# here can stay commented locally. Production selects the real SMTP backend +# and requires EMAIL_HOST and DEFAULT_FROM_EMAIL — the other five keep +# defaults matching smtp-relay.gmail.com in IP-allowlist mode (587, +# STARTTLS, no credentials). +# EMAIL_HOST=smtp-relay.gmail.com +# EMAIL_PORT=587 +# EMAIL_USE_TLS=1 +# EMAIL_USE_SSL=0 +# EMAIL_HOST_USER= +# EMAIL_HOST_PASSWORD= +# DEFAULT_FROM_EMAIL=catalogue@ilifu.ac.za +# EMAIL_TIMEOUT=10 + # --- OIDC ----------------------------------------------------------------- # Left commented deliberately: the defaults in settings/base.py already point # at `ilifu_catalog.devauth`, the fake in-process issuer mounted only when diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a54a9ab..35f47c7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -161,6 +161,12 @@ jobs: walking.py client.py cli.py + # Matches BOTH tasks.py files (find matches by basename): the + # ingest apply and the support notification. Same silent-failure + # profile as the rest of this list — a bug in either loses work + # (a snapshot, a notification) without erroring where anyone + # looks. + tasks.py ) found_files=() diff --git a/ANSIBLE_DEPLOY.md b/ANSIBLE_DEPLOY.md index 22187ba..9d2cfdd 100644 --- a/ANSIBLE_DEPLOY.md +++ b/ANSIBLE_DEPLOY.md @@ -108,6 +108,9 @@ quietly running on a shared placeholder. | `OIDC_OP_USER_ENDPOINT` | back-channel | | `OIDC_OP_JWKS_ENDPOINT` | back-channel | | `ILIFU_ADMIN_EMAILS` | comma-separated email addresses; parsed as a plain string like the hosts list. **Nobody can reach the admin screen without this**, which is why it has no default in production | +| `EMAIL_HOST` | SMTP relay for the software-request notification (e.g. `smtp-relay.gmail.com`); port/TLS/credentials have working defaults for that relay's IP-allowlist mode | +| `DEFAULT_FROM_EMAIL` | the notification's From address — relays reject Django's `webmaster@localhost` fallback, so a deployment must state a real one | +| `EMAIL_USE_SSL` | only for an implicit-SSL relay (usually port 465) — and then set **both** `EMAIL_USE_SSL=1` *and* `EMAIL_USE_TLS=0`. TLS defaults on for the 587+STARTTLS mode, and the pair together is refused **at boot**: setting only `EMAIL_USE_SSL=1` takes the whole site down, not just email | Against real Keycloak all four endpoints are externally reachable and the front/back-channel distinction disappears; it only matters against the in-process development issuer. diff --git a/CLAUDE.md b/CLAUDE.md index 2365066..a2eaddf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -82,8 +82,9 @@ that lacks it silently tests the renderer only against a fake. **Two coverage gates, both enforced in CI.** 95% project-wide, plus a second pass requiring **100%** on `ingest.py`, `manpages.py`, `search.py`, `categories.py`, `claims.py`, `backends.py`, `permissions.py`, `modules.py`, `containers.py`, `middleware.py`, `lmod.py`, -`libraries.py`, `walking.py`, `client.py` and `cli.py` (the last two are the collector's -transport — the code that carries the bearer token). Reaching that with assertion-free tests +`libraries.py`, `walking.py`, `client.py`, `cli.py` (the last two are the collector's +transport — the code that carries the bearer token) and `tasks.py` (matched by basename, so +both of them: the ingest apply and the support notification). Reaching that with assertion-free tests defeats the purpose; the gate exists to make those modules trustworthy — they are the ones whose bugs are silent, losing most of a scan without erroring, or quietly granting or refusing access. The authoritative list is `domain_module_names` in @@ -175,7 +176,9 @@ which is the difference between reading the code and arguing with it. licences). No build step, no CDN — the cluster environment can be offline. Fonts are self-hosted `.woff2` too; nothing loads from Google Fonts. - **Celery + Redis**, not django-q. `beat` runs one job: the nightly public-list cache - regeneration at 02:00. Ingest applies are the other task. + regeneration at 02:00. The workers run two more: ingest applies, and the + software-request support notification (`requests_app/tasks.py`) — the one whose failure + is user-visible, surfacing as the admin queue's un-notified `!` marker. - **mozilla-django-oidc against Keycloak.** Any user the realm authenticates can sign in and gets an account on first login. **Admin is an email allow-list in this app, not a Keycloak group** — `ILIFU_ADMIN_EMAILS`, compared against the `email` claim at each login, with the diff --git a/README.md b/README.md index 38a657a..206c0e1 100644 --- a/README.md +++ b/README.md @@ -212,8 +212,9 @@ for the annotated list. Every variable has a development default, which is why the stack runs on a fresh clone. **Production removes the defaults that must not be shared** — `ProductionSettings` makes the -secret key, the allowed hosts, all six OIDC fields and the admin allow-list required, so a -deployment that omits one fails to boot rather than quietly running on a placeholder. +secret key, the allowed hosts, all six OIDC fields, the admin allow-list and the mail relay +pair (`EMAIL_HOST`, `DEFAULT_FROM_EMAIL`) required, so a deployment that omits one fails to +boot rather than quietly running on a placeholder. | Variable | Purpose | Required in prod | |---|---|---| @@ -231,7 +232,12 @@ deployment that omits one fails to boot rather than quietly running on a placeho | `REDIS_MAXMEMORY` | ceiling for the Redis container; sized to hold a full staged snapshot alongside the cache | no | | `DATA_UPLOAD_MAX_MEMORY_SIZE` | largest request body Django will read. Django's 2.5 MB default is far below one ingest batch — see `.env.example` | no | | `SITE_ADDRESS` | Caddy site address; `:80` in dev, real hostname in prod | yes | -| `SUPPORT_EMAIL` | shown to users; receives software requests | no | +| `SUPPORT_EMAIL` | shown to users, and the **sole recipient** of the software-request notification email | no | +| `EMAIL_HOST` | SMTP relay for the request notification | **yes** | +| `EMAIL_PORT` / `EMAIL_USE_TLS` / `EMAIL_USE_SSL` | relay port and encryption mode; defaults are 587 + STARTTLS | no | +| `EMAIL_HOST_USER` / `_PASSWORD` | relay credentials; empty for a relay in IP-allowlist mode | no | +| `DEFAULT_FROM_EMAIL` | the notification's From address — relays reject Django's `webmaster@localhost` fallback | **yes** | +| `EMAIL_TIMEOUT` | seconds before a hung SMTP send is abandoned; Django's own backend would wait forever | no | | `OIDC_RP_CLIENT_ID` / `_SECRET` | Keycloak client credentials | **yes** | | `OIDC_OP_AUTHORIZATION_ENDPOINT` | front-channel: the browser is redirected here | **yes** | | `OIDC_OP_TOKEN_ENDPOINT` | back-channel: fetched by the app server | **yes** | @@ -345,7 +351,13 @@ rather than overlooked. apply never gets picked up inside that window is gone, and the collector has already been told 202. `tasks.py` logs it at ERROR so it is visible rather than silent, and the next scan re-uploads — but closing it properly means staging somewhere durable. -10. **`docker compose up` needs a `.env` despite the settings defaults.** +10. **A permanently failed support notification has no resend control.** The admin queue's + `!` marker says the notification never went out, but the only way to act on it is a + shell (`notify_support_of_request.delay(pk)`); a resend control on the triage form + would close the loop the marker opens. Deliberately deferred: the marker itself was the + fix for a silent failure, and a control that re-emails support belongs with a broader + decision about what else triage should be able to trigger. +11. **`docker compose up` needs a `.env` despite the settings defaults.** `settings/base.py` defaults every variable so the stack runs on a fresh clone, but `docker/entrypoint.sh` reads `os.environ['DATABASE_URL']` directly, with no default, so without a `.env` the web container loops on `postgres not ready yet ('DATABASE_URL')` diff --git a/catalog/src/ilifu_catalog/accounts/backends.py b/catalog/src/ilifu_catalog/accounts/backends.py index 4273223..3bc1c08 100644 --- a/catalog/src/ilifu_catalog/accounts/backends.py +++ b/catalog/src/ilifu_catalog/accounts/backends.py @@ -37,7 +37,7 @@ def _admin_emails() -> Collection[str]: """`settings.ILIFU_ADMIN_EMAILS`, read through `getattr` for django-stubs' sake. - See `software.views_catalog._support_email`'s docstring for why the ignore + See `ilifu_catalog.support.support_email`'s docstring for why the ignore is narrowed to a single accessor rather than repeated at every call site: django-stubs types `django.conf.settings` against Django's own `global_settings`, and this project's settings modules build their diff --git a/catalog/src/ilifu_catalog/requests_app/migrations/0002_softwarerequest_support_notified_at.py b/catalog/src/ilifu_catalog/requests_app/migrations/0002_softwarerequest_support_notified_at.py new file mode 100644 index 0000000..0b9b61b --- /dev/null +++ b/catalog/src/ilifu_catalog/requests_app/migrations/0002_softwarerequest_support_notified_at.py @@ -0,0 +1,17 @@ +# Generated by Django 6.0.7 on 2026-08-01 13:48 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ('requests_app', '0001_initial'), + ] + + operations = [ + migrations.AddField( + model_name='softwarerequest', + name='support_notified_at', + field=models.DateTimeField(blank=True, default=None, null=True), + ), + ] diff --git a/catalog/src/ilifu_catalog/requests_app/migrations/0003_backfill_support_notified_at.py b/catalog/src/ilifu_catalog/requests_app/migrations/0003_backfill_support_notified_at.py new file mode 100644 index 0000000..e6f1ab2 --- /dev/null +++ b/catalog/src/ilifu_catalog/requests_app/migrations/0003_backfill_support_notified_at.py @@ -0,0 +1,38 @@ +from django.db import migrations +from django.db.models import F + + +def stamp_rows_the_synchronous_code_notified(apps, schema_editor): + """Stamp every un-stamped row with its own `created_at`. + + Rows that predate 0002 were notified synchronously — the pre-Celery code + emailed support inside the request cycle, before the response went out — + so their NULL stamp is an artefact of the column arriving later, not a + fact about support. Left NULL, each would wear the admin queue's + "support not notified" marker forever, a permanent false alarm on the + one screen where the marker is meant to prompt action. `created_at` is + an approximation and looks like one: it is visibly older than any send + the Celery task will ever record. + + Runs on whatever rows exist at migration time, which on a fresh install + is none — the marker's real semantics start with the first row created + after this deploy. + """ + software_request_model = apps.get_model('requests_app', 'SoftwareRequest') + software_request_model.objects.filter(support_notified_at__isnull=True).update( + support_notified_at=F('created_at') + ) + + +class Migration(migrations.Migration): + dependencies = [ + ('requests_app', '0002_softwarerequest_support_notified_at'), + ] + + operations = [ + # Reverse is a no-op rather than re-NULLing: rolling back the *code* + # to the synchronous version makes the column unread, and un-stamping + # rows would destroy real send records if the migration is ever + # reversed after the task has run. + migrations.RunPython(stamp_rows_the_synchronous_code_notified, migrations.RunPython.noop), + ] diff --git a/catalog/src/ilifu_catalog/requests_app/models.py b/catalog/src/ilifu_catalog/requests_app/models.py index 3094c26..b2b3b40 100644 --- a/catalog/src/ilifu_catalog/requests_app/models.py +++ b/catalog/src/ilifu_catalog/requests_app/models.py @@ -93,6 +93,14 @@ class SoftwareRequest(models.Model): ) admin_note = models.TextField(blank=True, default='') created_at = models.DateTimeField(auto_now_add=True) + # When `requests_app.tasks.notify_support_of_request` got its email out + # to SUPPORT_EMAIL. A nullable timestamp, not a state machine: NULL means + # "support has not been told" whether the send is still pending or has + # been given up on — retries are Celery's business, not the schema's — + # and the admin queue's marker only asks that binary question. Stamped + # via a queryset `.update()`, never instance `.save()`, so the task + # cannot clobber a concurrent admin triage with a stale row. + support_notified_at = models.DateTimeField(null=True, blank=True, default=None) # Nullable and `SET_NULL`, not `CASCADE`: a `SoftwareRequest` is the # record that someone asked, and that record must survive even if the # package it was eventually fulfilled by is later removed from the diff --git a/catalog/src/ilifu_catalog/requests_app/tasks.py b/catalog/src/ilifu_catalog/requests_app/tasks.py new file mode 100644 index 0000000..381c1f4 --- /dev/null +++ b/catalog/src/ilifu_catalog/requests_app/tasks.py @@ -0,0 +1,114 @@ +"""The support notification, off the request cycle. + +`views._save_and_notify` saves the `SoftwareRequest` row and enqueues this +task; the row is the record of truth and this email is best-effort delivery +of the news. That split is the whole design: the requester's submit never +waits on SMTP and never fails because of it — a relay outage surfaces as +the admin queue's "support not notified" marker (`support_notified_at` +still NULL) instead of a 500 on a request that was in fact saved. + +Retry policy: three retries at a fixed sixty seconds — four attempts over +roughly three minutes covers a relay blip without pretending to be a mail +queue. Exhaustion logs ERROR and returns rather than raising: by the time +the budget is spent there is no requester left to hand an error to, and a +FAILURE state in a result backend nobody polls helps nobody. (Under +`CELERY_TASK_ALWAYS_EAGER` the *retry* path does raise `Retry` straight +back into the submitting request — the view's guarded enqueue is what +absorbs it; see `views._save_and_notify`.) +""" + +from __future__ import annotations + +import logging + +from django.core.mail import send_mail +from django.utils import timezone + +from ilifu_catalog.celery import app +from ilifu_catalog.support import support_email + +from .models import SoftwareRequest + +logger = logging.getLogger(__name__) + + +# Bound to `ilifu_catalog.celery.app` explicitly rather than `@shared_task`, +# for the reason spelled out at `software.tasks.apply_pending_snapshot`; the +# `type: ignore` is the same celery-ships-no-py.typed caveat as there. +@app.task( # type: ignore[untyped-decorator] + name='ilifu_catalog.requests_app.notify_support_of_request', + bind=True, + max_retries=3, + default_retry_delay=60, +) +def notify_support_of_request(self, request_id: int) -> None: # type: ignore[no-untyped-def] + """Email SUPPORT_EMAIL that `request_id` landed, and stamp the row. + + Logs and returns — rather than raising — when the row no longer exists: + by the time this runs there is no HTTP requester left to hand an error + back to, and a retry can never make a deleted row reappear. + + The requester is named by `get_username()` — Keycloak's + `preferred_username` — explicitly rather than by interpolating the user + object and relying on `User.__str__` to be the same thing. It has + silently been the OIDC subject UUID before. + + There is deliberately no reply-to address: this application stores none + (see `accounts.claims`), so the last line says where to look the + username up instead of implying support can just hit reply. + """ + software_request = SoftwareRequest.objects.select_related('user').filter(pk=request_id).first() + if software_request is None: + logger.error('request %s no longer exists; support was never notified', request_id) + return + # A redelivered message (worker restart mid-task, a retry whose first + # delivery did complete) must not become a duplicate email in support's + # inbox or a fresher stamp over the real send time — the same + # running-twice posture `software.tasks.apply_pending_snapshot` takes. + if software_request.support_notified_at is not None: + return + + username = software_request.user.get_username() + try: + send_mail( + subject=( + f'[ilifu software] request: {software_request.name} {software_request.version}' + ), + message=( + f'{username} requested {software_request.name} {software_request.version}.\n\n' + f'Source: {software_request.source_url}\n' + f'Licence: {software_request.get_licence_display()}\n' + f'Delivery preference: {software_request.get_delivery_pref_display()}\n\n' + f'{software_request.justification}\n\n' + f'To contact the requester, look up the Keycloak user {username!r} — ' + f'the catalogue stores no email addresses.' + ), + from_email=None, + recipient_list=[support_email()], + fail_silently=False, + ) + except Exception as exc: + # Give up *before* calling retry once the budget is spent: on + # exhaustion `Task.retry(exc=...)` re-raises the original exception + # rather than `MaxRetriesExceededError`, so "catch the exhaustion + # error" is a branch that never runs. Counting attempts ourselves is + # the version with no such trapdoor. + if self.request.retries >= self.max_retries: + logger.error( + 'support was never notified of request %s after %s attempts: %s ' + '(it shows as un-notified on the admin queue)', + request_id, + self.request.retries + 1, + exc, + ) + return + raise self.retry(exc=exc) from exc + + # A queryset update, not instance `.save()`: an admin may have triaged + # the request while the email was in flight, and this task must not + # write its stale copy of those fields back over theirs. The isnull + # filter keeps the *first* send's timestamp if two deliveries of one + # message ever race past the check above. + SoftwareRequest.objects.filter(pk=request_id, support_notified_at__isnull=True).update( + support_notified_at=timezone.now() + ) diff --git a/catalog/src/ilifu_catalog/requests_app/views.py b/catalog/src/ilifu_catalog/requests_app/views.py index 27ed5e0..e5540da 100644 --- a/catalog/src/ilifu_catalog/requests_app/views.py +++ b/catalog/src/ilifu_catalog/requests_app/views.py @@ -40,12 +40,11 @@ from __future__ import annotations +import logging from dataclasses import dataclass from typing import Final, cast -from django.conf import settings from django.contrib.auth.models import User -from django.core.mail import send_mail from django.http import HttpRequest, HttpResponse from django.shortcuts import redirect, render from django.urls import reverse @@ -55,10 +54,14 @@ from ilifu_catalog.header import header_display_name, signed_in_header_context from ilifu_catalog.software import search as search_layer from ilifu_catalog.software.views_catalog import DOCS_URL, _index_stats, _initials +from ilifu_catalog.support import support_email from ilifu_catalog.theming import theme_for_request from .forms import SoftwareRequestForm from .models import RequestStatus, SoftwareRequest +from .tasks import notify_support_of_request + +logger = logging.getLogger(__name__) #: Query parameter `views_catalog`'s empty state prefills this screen with. #: See the module docstring's "Prefilling from the catalog's empty state". @@ -140,68 +143,38 @@ def request_name_check(request: HttpRequest) -> HttpResponse: def _save_and_notify(request: HttpRequest, form: SoftwareRequestForm) -> SoftwareRequest: - """Persist the request, then best-effort email support about it. - - Order matters: the row is saved *first*. A failed send below must never - lose the request — the row is the record of truth, per the milestone - brief — so `_notify_support` runs strictly after `save()` rather than - inside a transaction it could roll back, and any exception it raises - propagates as a 500 on an already-persisted request rather than - discarding the ask silently. + """Persist the request, then hand the support notification to Celery. + + Order matters: the row is saved *first*, and it is the record of truth — + the notification (`requests_app.tasks.notify_support_of_request`) is + best-effort delivery of the news, off the request cycle. The enqueue is + guarded, unlike the ingest API's bare `.delay()`: there the caller is a + retrying collector, here it is a human whose contract is "your request + was saved". A dead broker therefore logs and falls through to the same + redirect, and the un-sent notification surfaces where support already + looks — the admin queue's "support not notified" marker, which reads + `SoftwareRequest.support_notified_at`. """ software_request = form.save(commit=False) software_request.user = cast(User, request.user) software_request.save() - _notify_support(software_request) + # NOTE: a bare `.delay()`, not `transaction.on_commit`, which depends on + # this view running in autocommit — true today (nothing sets + # ATOMIC_REQUESTS and no `atomic()` wraps this). If that ever changes, + # the worker can dequeue before the commit lands, and the task's + # missing-row branch returns *without retrying* — the notification would + # be lost silently. `on_commit` isn't used now because it would move the + # enqueue past this try/except, dismantling the guarded-enqueue contract + # the docstring above describes. + try: + notify_support_of_request.delay(software_request.pk) + except Exception: + logger.exception( + 'could not enqueue the support notification for request %s', software_request.pk + ) return software_request -def _notify_support(software_request: SoftwareRequest) -> None: - """Tell support a request landed, and how to reach the person who made it. - - The requester is named by `get_username()` — Keycloak's - `preferred_username` — explicitly rather than by interpolating the user - object and relying on `User.__str__` to be the same thing. It has silently - been the OIDC subject UUID until now, which is what this email said. - - There is deliberately no reply-to address: this application stores none - (see `accounts.claims`), so the last line says where to look the username - up instead of implying support can just hit reply. - - # TODO: synchronous SMTP in the request thread — a slow or unreachable - # relay holds a worker thread for the full SMTP timeout on every submit. - # Moving it to a Celery task also moves the failure after the response, - # which costs the deliberate 500-on-an-already-saved-row contract in - # `_save_and_notify` — choose a new answer there (e.g. surface failed - # notifications on the admin queue) as part of the same change. - """ - username = software_request.user.get_username() - send_mail( - subject=f'[ilifu software] request: {software_request.name} {software_request.version}', - message=( - f'{username} requested {software_request.name} {software_request.version}.\n\n' - f'Source: {software_request.source_url}\n' - f'Licence: {software_request.get_licence_display()}\n' - f'Delivery preference: {software_request.get_delivery_pref_display()}\n\n' - f'{software_request.justification}\n\n' - f'To contact the requester, look up the Keycloak user {username!r} — ' - f'the catalogue stores no email addresses.' - ), - from_email=None, - recipient_list=[_support_email()], - fail_silently=False, - ) - - -def _support_email() -> str: - """`settings.SUPPORT_EMAIL`. See `views_catalog._support_email`'s docstring - - for why the `type: ignore` is narrowed to one accessor rather than - repeated at every call site — the same reasoning applies here. - """ - return str(settings.SUPPORT_EMAIL) # type: ignore[misc] - - def _name_check_context(candidate_name: str) -> dict[str, object]: """Context for `partials/_request_name_check.html`, built once for both call sites.""" query = candidate_name.strip() @@ -267,7 +240,7 @@ def _header_context(request: HttpRequest) -> dict[str, object]: 'header_stats': _index_stats(), 'header_nav_links': [ {'label': 'docs', 'url': DOCS_URL}, - {'label': 'support', 'url': f'mailto:{_support_email()}'}, + {'label': 'support', 'url': f'mailto:{support_email()}'}, ], 'header_user': { 'initials': _initials(display_name), @@ -281,7 +254,7 @@ def _context(request: HttpRequest, form: SoftwareRequestForm, name_query: str) - return { 'theme': theme_for_request(request), 'form': form, - 'support_email': _support_email(), + 'support_email': support_email(), 'oss_confirmed_initial': _oss_confirmed_initial(form), 'your_requests': _sidebar_requests(request), **_name_check_context(name_query), diff --git a/catalog/src/ilifu_catalog/settings/base.py b/catalog/src/ilifu_catalog/settings/base.py index fbd401d..59c64b8 100644 --- a/catalog/src/ilifu_catalog/settings/base.py +++ b/catalog/src/ilifu_catalog/settings/base.py @@ -22,7 +22,7 @@ from celery.schedules import ( crontab, # type: ignore[import-untyped] # celery ships no py.typed marker ) -from pydantic import field_validator +from pydantic import field_validator, model_validator from pydantic_settings import BaseSettings, SettingsConfigDict # catalog/src/ilifu_catalog/settings/base.py -> catalog/ @@ -83,6 +83,31 @@ class Settings(BaseSettings): ingest_redis_url: str = 'redis://redis:6379/2' site_address: str = ':80' support_email: str = 'support@ilifu.ac.za' + # The SMTP relay for the one email this application sends: the + # software-request notification to SUPPORT_EMAIL (`requests_app.tasks`). + # Port/TLS defaults match smtp-relay.gmail.com's 587+STARTTLS mode, so a + # 465-implicit-SSL relay is two env vars, not a release; the credential + # pair defaults empty because a relay in IP-allowlist mode needs none. + # The empty host is honest in dev and test — their backends (console, + # locmem) never open a socket — while `ProductionSettings` strips the + # defaults from `email_host` and `default_from_email`: prod.py selects + # the real SMTP backend, under which Django's own fallbacks mean + # unauthenticated localhost:25 as webmaster@localhost — a combination no + # relay accepts and no container can serve, failing in a Celery worker + # where nobody is watching. + email_host: str = '' + email_port: int = 587 + email_use_tls: bool = True + email_use_ssl: bool = False + email_host_user: str = '' + email_host_password: str = '' + default_from_email: str = 'webmaster@localhost' + # Django's SMTP backend sets NO socket timeout of its own, so a hung + # relay would hold the sending thread for as long as the OS keeps the + # socket alive — and after the move to `requests_app.tasks` that thread + # is the single-core host's one prefork Celery worker, which also applies + # ingest snapshots. Same reasoning as `_REDIS_SOCKET_OPTIONS` below. + email_timeout: int = 10 # Django's own default is 2.5 MB, which a real ingest batch exceeds many # times over: a batch carries whole man pages as raw roff, and `openmpi` # alone ships ~16 MB across its four installed versions. Above the limit @@ -194,6 +219,23 @@ def _reject_an_unusable_admin_list(cls, value: str) -> str: ) return value + @model_validator(mode='after') + def _reject_tls_and_ssl_together(self) -> 'Settings': + """Refuse the TLS/SSL pair Django itself refuses — but at boot. + + Django's SMTP backend raises exactly this complaint at *send* time, + which after the move to a Celery task is post-response in a worker + where nobody is watching. Boot-time refusal states the problem at the + only moment anyone is. + """ + if self.email_use_tls and self.email_use_ssl: + raise ValueError( + 'EMAIL_USE_TLS and EMAIL_USE_SSL are mutually exclusive: ' + 'STARTTLS upgrades a plain connection (587), implicit SSL ' + 'opens an encrypted one (465) — pick the one the relay speaks' + ) + return self + @property def allowed_hosts(self) -> list[str]: """`DJANGO_ALLOWED_HOSTS` split into the list Django's ALLOWED_HOSTS expects.""" @@ -259,6 +301,14 @@ def django_settings_from(settings: Settings) -> dict[str, Any]: 'DEBUG': settings.django_debug, 'ALLOWED_HOSTS': settings.allowed_hosts, 'SUPPORT_EMAIL': settings.support_email, + 'EMAIL_HOST': settings.email_host, + 'EMAIL_PORT': settings.email_port, + 'EMAIL_USE_TLS': settings.email_use_tls, + 'EMAIL_USE_SSL': settings.email_use_ssl, + 'EMAIL_HOST_USER': settings.email_host_user, + 'EMAIL_HOST_PASSWORD': settings.email_host_password, + 'EMAIL_TIMEOUT': settings.email_timeout, + 'DEFAULT_FROM_EMAIL': settings.default_from_email, 'SITE_ADDRESS': settings.site_address, 'DATA_UPLOAD_MAX_MEMORY_SIZE': settings.data_upload_max_memory_size, 'ROOT_URLCONF': 'ilifu_catalog.urls', @@ -394,6 +444,24 @@ def django_settings_from(settings: Settings) -> dict[str, Any]: # requirement. 'ILIFU_ADMIN_EMAILS': settings.admin_emails, 'CELERY_BROKER_URL': settings.celery_broker_url, + # The same bounds as the cache aliases below, for the same reason — + # kombu sets no socket timeouts of its own, so a *hung* (rather than + # refused) broker would hold the publishing thread for as long as the + # OS keeps the socket alive. The one publisher on the request cycle is + # the request form's guarded enqueue (`requests_app.views`): its guard + # only helps if the publish returns. Safe for the worker's idle BRPOP + # wait, which happens in kombu's epoll hub, not in a socket read — + # the read is only issued once the socket is already readable. + 'CELERY_BROKER_TRANSPORT_OPTIONS': dict(_REDIS_SOCKET_OPTIONS), + # One quick publish retry rather than kombu's default three: the + # request-form caller is a human whose row is already saved, and every + # retry multiplies the connect timeout above into their response time. + 'CELERY_TASK_PUBLISH_RETRY_POLICY': { + 'max_retries': 1, + 'interval_start': 0, + 'interval_step': 0.5, + 'interval_max': 0.5, + }, 'CELERY_RESULT_BACKEND': settings.redis_url, 'CELERY_ACCEPT_CONTENT': ['json'], 'CELERY_TASK_SERIALIZER': 'json', diff --git a/catalog/src/ilifu_catalog/settings/prod.py b/catalog/src/ilifu_catalog/settings/prod.py index faddb8c..8b081d1 100644 --- a/catalog/src/ilifu_catalog/settings/prod.py +++ b/catalog/src/ilifu_catalog/settings/prod.py @@ -36,6 +36,15 @@ class ProductionSettings(Settings): oidc_op_user_endpoint: str oidc_op_jwks_endpoint: str ilifu_admin_emails: str + # Required for the same reason as the allow-list: their base defaults + # (empty host → localhost:25, webmaster@localhost) can only produce a + # deployment whose request notifications die in a Celery worker with + # nothing anywhere saying why — this module hardcodes the real SMTP + # backend below. Port, TLS mode and credentials keep their defaults + # (587+STARTTLS, no auth), which are real for smtp-relay.gmail.com in + # IP-allowlist mode. + email_host: str + default_from_email: str settings = ProductionSettings() diff --git a/catalog/src/ilifu_catalog/settings/test.py b/catalog/src/ilifu_catalog/settings/test.py index 603e59a..c2ba952 100644 --- a/catalog/src/ilifu_catalog/settings/test.py +++ b/catalog/src/ilifu_catalog/settings/test.py @@ -24,6 +24,20 @@ ingest_redis_url='redis://localhost:6379/5', site_address=':80', support_email='support@ilifu.ac.za', + # The locmem backend below never opens a socket, so these are inert — but + # they are still passed explicitly to keep this module's own contract + # ("every value explicit, so the ambient environment cannot leak in"). + # The TLS/SSL pair is the one that bites: an exported EMAIL_USE_TLS=1 + # EMAIL_USE_SSL=1 would otherwise fail the whole suite at settings import, + # in the boot-time validator that pair exists to trip in production. + email_host='', + email_port=587, + email_use_tls=True, + email_use_ssl=False, + email_host_user='', + email_host_password='', + email_timeout=10, + default_from_email='webmaster@localhost', # Matches `devauth.fixtures.ADMIN_USER.email`, so the devauth round-trip # test in `test_auth_flow.py` still proves that signing in as the admin # fixture actually earns the `ilifu-software-admin` group. Tests that need diff --git a/catalog/src/ilifu_catalog/software/detail_panels.py b/catalog/src/ilifu_catalog/software/detail_panels.py index b017947..da8b2bc 100644 --- a/catalog/src/ilifu_catalog/software/detail_panels.py +++ b/catalog/src/ilifu_catalog/software/detail_panels.py @@ -49,7 +49,6 @@ from typing import TYPE_CHECKING, Final from urllib.parse import urlencode -from django.conf import settings from django.urls import reverse from django.utils.timesince import timesince @@ -61,6 +60,7 @@ SoftwareKind, Version, ) +from ilifu_catalog.support import support_email if TYPE_CHECKING: # pragma: no cover - import cycle broken at runtime from ilifu_catalog.software.views_catalog import Link @@ -651,15 +651,8 @@ def _provenance_panel(package: Package, version: Version) -> ProvenancePanel: def _report_url(package: Package, version: Version) -> str: - """A prefilled mail to support, so "report a problem" actually reports one. - - # NOTE: `settings.SUPPORT_EMAIL` is read through `str(...)` with a - # narrowed ignore for the same django-stubs reason `views_catalog - # ._support_email` documents at length — the settings namespace is built - # dynamically, so the plugin cannot see any project-defined setting. - """ - support_email = str(settings.SUPPORT_EMAIL) # type: ignore[misc] + """A prefilled mail to support, so "report a problem" actually reports one.""" subject = urlencode( {'subject': f'Software catalogue correction: {package.name}/{version.version}'} ) - return f'mailto:{support_email}?{subject}' + return f'mailto:{support_email()}?{subject}' diff --git a/catalog/src/ilifu_catalog/software/views_admin.py b/catalog/src/ilifu_catalog/software/views_admin.py index 0f50f9c..38c3f93 100644 --- a/catalog/src/ilifu_catalog/software/views_admin.py +++ b/catalog/src/ilifu_catalog/software/views_admin.py @@ -266,6 +266,7 @@ class QueueRow: version: str requester: str admin_note: str + support_notified: bool @dataclass(frozen=True) @@ -956,6 +957,7 @@ def _queue_row(item: SoftwareRequest) -> QueueRow: version=item.version, requester=item.user.get_username(), admin_note=item.admin_note, + support_notified=item.support_notified_at is not None, ) diff --git a/catalog/src/ilifu_catalog/software/views_catalog.py b/catalog/src/ilifu_catalog/software/views_catalog.py index 308b0ec..a387168 100644 --- a/catalog/src/ilifu_catalog/software/views_catalog.py +++ b/catalog/src/ilifu_catalog/software/views_catalog.py @@ -69,7 +69,6 @@ from typing import Final, cast from urllib.parse import urlencode -from django.conf import settings from django.core.cache import cache from django.core.exceptions import ValidationError from django.db import transaction @@ -101,6 +100,7 @@ Version, version_sort_key, ) +from ilifu_catalog.support import support_email from ilifu_catalog.theming import theme_for_request #: URL name of the shared root route, mounted by the M2 public list @@ -571,7 +571,7 @@ def _header_context(request: HttpRequest) -> dict[str, object]: 'header_nav_links': [ {'label': 'request software', 'url': reverse(REQUEST_FORM_ROUTE)}, {'label': 'docs', 'url': DOCS_URL}, - {'label': 'support', 'url': f'mailto:{_support_email()}'}, + {'label': 'support', 'url': f'mailto:{support_email()}'}, ], 'header_user': { 'initials': _initials(display_name), @@ -581,21 +581,6 @@ def _header_context(request: HttpRequest) -> dict[str, object]: } -def _support_email() -> str: - """`settings.SUPPORT_EMAIL`, read through `getattr` for django-stubs' sake. - - django-stubs types `django.conf.settings` against Django's own - `global_settings`, and this project's settings modules build their - namespace with `globals().update(django_settings_from(...))` — nothing - static for the plugin to read — so any project-defined setting is an - error however real it is. `SUPPORT_EMAIL` is set on every environment - (`settings/base.py`, from the environment variable of the same name). - The ignore is narrowed to this one accessor rather than repeated at each - call site. - """ - return str(settings.SUPPORT_EMAIL) # type: ignore[misc] - - def _initials(display_name: str) -> str: """`Nomsa Mokoena` and `n.mokoena` -> `nm`; the 10px avatar holds two characters. diff --git a/catalog/src/ilifu_catalog/support.py b/catalog/src/ilifu_catalog/support.py new file mode 100644 index 0000000..e34f6a1 --- /dev/null +++ b/catalog/src/ilifu_catalog/support.py @@ -0,0 +1,22 @@ +"""The one accessor for `settings.SUPPORT_EMAIL`. + +django-stubs types `django.conf.settings` against Django's own +`global_settings`, and this project's settings modules build their +namespace with `globals().update(django_settings_from(...))` — nothing +static for the plugin to read — so any project-defined setting is an +error however real it is. `SUPPORT_EMAIL` is set on every environment +(`settings/base.py`, from the environment variable of the same name). + +Reading it lived as a per-module copy of the same one-liner-plus-ignore in +every screen module and the notification task; this module narrows the +`type: ignore` to a single site the way `accounts.backends._admin_emails` +does for `ILIFU_ADMIN_EMAILS`. +""" + +from __future__ import annotations + +from django.conf import settings + + +def support_email() -> str: + return str(settings.SUPPORT_EMAIL) # type: ignore[misc] diff --git a/catalog/src/ilifu_catalog/templates/software/partials/admin/_queue_card.html b/catalog/src/ilifu_catalog/templates/software/partials/admin/_queue_card.html index 148852c..5e1899b 100644 --- a/catalog/src/ilifu_catalog/templates/software/partials/admin/_queue_card.html +++ b/catalog/src/ilifu_catalog/templates/software/partials/admin/_queue_card.html @@ -6,6 +6,16 @@ new / accepted / building / installed / declined and records the `admin_note` the requester sees on their own sidebar. Saving swaps this whole card, so the badge, the count and the note can never drift apart. + + The `!` marker is where a failed notification email surfaces + (`requests_app.tasks` leaves `support_notified_at` NULL when the relay + never answered or the broker was down). A glyph rather than the phrase: + the row's package name is its only shrinkable element, and a worded + badge ellipsized it on exactly the rows needing attention — the wording + lives in the glyph's title/aria-label instead. It is --warn rather than + --err deliberately: a just-submitted row is legitimately un-notified for + the seconds before the worker runs, so the marker also shows transiently + on healthy rows — "needs a look", not "broken". {% endcomment %}
@@ -18,6 +28,12 @@ {{ row.status }} {{ row.name }} {{ row.version }} {{ row.requester }} + {% if not row.support_notified %} + ! + {% endif %}
None: + """`support_notified_at` still NULL means the notification email never + + went out (relay down past the retry budget, or a dead broker) — the + admin queue is the one place that failure is designed to surface. + + The marker is a `!` glyph, not the phrase: the queue row lives in a + 300px sidebar where the package name is the only shrinkable element, + so a worded badge ellipsized the name on exactly the rows needing + attention. The wording survives as the glyph's title/aria-label. + """ + _software_request(plain_user) + + body = client.get(SCAN_URL).content.decode() + + assert 'title="support not notified"' in body + + +def test_should_not_flag_a_request_when_support_was_notified( + client: Client, plain_user: User +) -> None: + software_request = _software_request(plain_user) + SoftwareRequest.objects.filter(pk=software_request.pk).update( + support_notified_at=timezone.now() + ) + + body = client.get(SCAN_URL).content.decode() + + assert 'support not notified' not in body + + # -------------------------------------------------------------------------- # the sidebar — category mapping # -------------------------------------------------------------------------- diff --git a/catalog/tests/test_request_form.py b/catalog/tests/test_request_form.py index dbfb9c4..4954686 100644 --- a/catalog/tests/test_request_form.py +++ b/catalog/tests/test_request_form.py @@ -17,13 +17,18 @@ from __future__ import annotations +from collections.abc import Iterator +from importlib import import_module +from smtplib import SMTPException from unittest import mock import pytest +from django.apps import apps from django.conf import settings from django.contrib.auth.models import User from django.core import mail -from django.test import Client +from django.db import connection +from django.test import Client, override_settings from django.urls import reverse from django.utils import timezone @@ -33,6 +38,7 @@ RequestStatus, SoftwareRequest, ) +from ilifu_catalog.requests_app.tasks import notify_support_of_request from ilifu_catalog.software import views_catalog from ilifu_catalog.software.models import ( Binary, @@ -356,17 +362,196 @@ def test_should_not_promise_the_requester_an_email(client: Client, user: User) - assert 'emailed' not in body -def test_should_keep_the_request_row_when_the_notification_email_fails_to_send( +def test_should_still_show_the_requester_success_when_the_notification_email_fails_to_send( client: Client, user: User ) -> None: - with ( - mock.patch( - 'ilifu_catalog.requests_app.views.send_mail', side_effect=RuntimeError('smtp is down') - ), - pytest.raises(RuntimeError), + """The row is the record of truth; the notification is best-effort. + + This screen used to 500 on an already-saved row when the relay was down, + which read as total failure to the one person who could not tell the + difference. Now the submit succeeds either way and the un-notified state + surfaces on the admin queue instead. + """ + with mock.patch( + 'ilifu_catalog.requests_app.tasks.send_mail', side_effect=RuntimeError('smtp is down') + ): + response = client.post(REQUEST_URL, _valid_payload()) + + assert response.status_code == 302 + assert SoftwareRequest.objects.filter(user=user, name='bwa-meth').exists() + + +def test_should_stamp_support_notified_at_when_the_notification_sends(client: Client) -> None: + client.post(REQUEST_URL, _valid_payload()) + + request = SoftwareRequest.objects.get() + assert request.support_notified_at is not None + assert len(mail.outbox) == 1 + + +def test_should_leave_support_notified_at_null_when_the_send_fails( + client: Client, caplog: pytest.LogCaptureFixture +) -> None: + with mock.patch( + 'ilifu_catalog.requests_app.tasks.send_mail', side_effect=RuntimeError('smtp is down') ): - client.post(REQUEST_URL, _valid_payload()) + response = client.post(REQUEST_URL, _valid_payload()) + + assert response.status_code == 302 + assert SoftwareRequest.objects.get().support_notified_at is None + # Under the suite's eager-propagating celery the task's `Retry` escapes + # `.delay()` into the view's guarded enqueue, so the ERROR on record is + # the guard's — the task's own give-up path is exercised by + # `test_should_give_up_and_log_when_the_relay_never_recovers` below. + assert any( + 'could not enqueue the support notification' in record.getMessage() + for record in caplog.records + ) + +# The two retry-path tests drive the task with `.apply()` under +# `CELERY_TASK_EAGER_PROPAGATES = False` rather than through the view. With +# propagation on (the suite's default), trace re-raises `Retry` before +# `Task.apply`'s eager re-execution branch can run, so through the view a +# transient failure looks identical to a permanent one. With it off, the +# `Retry` becomes a return value and `apply()` re-runs the signature with +# `retries + 1` — the same requeue-and-re-run loop the real worker performs, +# minus the sixty-second wait. `override_settings` works here because +# celery's `config_from_object('django.conf:settings', ...)` chain reads the +# Django settings object live — writes to `app.conf` itself are shadowed by +# that same chain, which is why this is a Django override and not a conf edit. + + +@pytest.fixture +def eager_retries() -> Iterator[None]: + with override_settings(CELERY_TASK_EAGER_PROPAGATES=False): + yield + + +def test_should_retry_and_notify_when_the_relay_fails_once_then_recovers( + user: User, eager_retries: None +) -> None: + request = SoftwareRequest.objects.create( + user=user, name='bwa-meth', version='0.2.7', source_url='https://example.org' + ) + send_mail = mock.patch( + 'ilifu_catalog.requests_app.tasks.send_mail', + side_effect=[SMTPException('greeting refused'), 1], + ) + with send_mail as patched: + notify_support_of_request.apply(args=[request.pk]) + + assert patched.call_count == 2 + request.refresh_from_db() + assert request.support_notified_at is not None + + +def test_should_give_up_and_log_when_the_relay_never_recovers( + user: User, eager_retries: None, caplog: pytest.LogCaptureFixture +) -> None: + """Exhaustion must return, not raise — there is no requester to 500 at.""" + request = SoftwareRequest.objects.create( + user=user, name='bwa-meth', version='0.2.7', source_url='https://example.org' + ) + send_mail = mock.patch( + 'ilifu_catalog.requests_app.tasks.send_mail', + side_effect=SMTPException('relay is gone'), + ) + with send_mail as patched: + notify_support_of_request.apply(args=[request.pk]) + + # the first attempt plus max_retries=3 + assert patched.call_count == 4 + request.refresh_from_db() + assert request.support_notified_at is None + assert any('never notified' in record.getMessage() for record in caplog.records) + + +def test_should_not_email_again_when_support_was_already_notified(user: User) -> None: + """A broker can redeliver a message (worker restart mid-task, a retry the + + first delivery of which did in fact complete), and the task must not turn + redelivery into a duplicate email in support's inbox or a fresher stamp + over the real send time. The row itself records whether the news went + out, so the task checks it before sending — the same running-twice + posture `software.tasks.apply_pending_snapshot` documents. + """ + request = SoftwareRequest.objects.create( + user=user, name='bwa-meth', version='0.2.7', source_url='https://example.org' + ) + first_send_at = timezone.now() + SoftwareRequest.objects.filter(pk=request.pk).update(support_notified_at=first_send_at) + + notify_support_of_request(request.pk) + + assert len(mail.outbox) == 0 + request.refresh_from_db() + assert request.support_notified_at == first_send_at + + +def test_should_log_and_skip_when_the_request_row_no_longer_exists( + caplog: pytest.LogCaptureFixture, +) -> None: + """A row can be deleted between enqueue and run; by then there is nobody + + on the other end to hand an error back to, so the task logs and returns + rather than raising into a retry loop that can never succeed. + """ + notify_support_of_request(999_999) + + assert len(mail.outbox) == 0 + assert any(record.levelname == 'ERROR' for record in caplog.records) + + +def test_should_backfill_the_notified_stamp_when_migrating_rows_the_old_code_served( + user: User, +) -> None: + """Rows that predate `support_notified_at` were notified *synchronously* — + + the old code emailed before the response went out, or 500ed trying — so + a NULL stamp on them is an artefact of the column arriving later, not a + fact about support. Left NULL, every pre-existing row would wear the + admin queue's "support not notified" marker forever, which is how a + marker gets trained out of an admin's attention. The backfill stamps + them with `created_at`: honest about being an approximation, and + visibly older than any real send. + """ + backfill_migration = import_module( + 'ilifu_catalog.requests_app.migrations.0003_backfill_support_notified_at' + ) + + legacy = SoftwareRequest.objects.create( + user=user, name='bwa-meth', version='0.2.7', source_url='https://example.org' + ) + already_stamped_at = timezone.now() + stamped = SoftwareRequest.objects.create( + user=user, name='samtools', version='1.19', source_url='https://example.org' + ) + SoftwareRequest.objects.filter(pk=stamped.pk).update(support_notified_at=already_stamped_at) + + with connection.schema_editor() as schema_editor: + backfill_migration.stamp_rows_the_synchronous_code_notified(apps, schema_editor) + + legacy.refresh_from_db() + stamped.refresh_from_db() + assert legacy.support_notified_at == legacy.created_at + assert stamped.support_notified_at == already_stamped_at + + +def test_should_still_save_the_request_when_the_broker_is_down(client: Client, user: User) -> None: + """Enqueueing is guarded, unlike the ingest API's `.delay()`: there the + + caller is a retrying collector; here it is a human whose contract is + "always success". A broker outage surfaces as the admin queue's + un-notified marker, not a 500. + """ + with mock.patch( + 'ilifu_catalog.requests_app.views.notify_support_of_request.delay', + side_effect=RuntimeError('broker is down'), + ): + response = client.post(REQUEST_URL, _valid_payload()) + + assert response.status_code == 302 assert SoftwareRequest.objects.filter(user=user, name='bwa-meth').exists() diff --git a/catalog/tests/test_settings.py b/catalog/tests/test_settings.py index 8632d3f..511560c 100644 --- a/catalog/tests/test_settings.py +++ b/catalog/tests/test_settings.py @@ -142,6 +142,52 @@ def test_should_bound_every_redis_socket_operation_with_a_timeout() -> None: assert options['socket_timeout'] == 2, alias +def test_should_export_smtp_connection_settings_when_configured() -> None: + settings = Settings( + email_host='smtp-relay.gmail.com', + email_port=587, + email_use_tls=True, + email_host_user='catalogue', + email_host_password='a-relay-password', + default_from_email='catalogue@ilifu.ac.za', + ) + + django_settings = django_settings_from(settings) + + assert django_settings['EMAIL_HOST'] == 'smtp-relay.gmail.com' + assert django_settings['EMAIL_PORT'] == 587 + assert django_settings['EMAIL_USE_TLS'] is True + assert django_settings['EMAIL_USE_SSL'] is False + assert django_settings['EMAIL_HOST_USER'] == 'catalogue' + assert django_settings['EMAIL_HOST_PASSWORD'] == 'a-relay-password' + assert django_settings['DEFAULT_FROM_EMAIL'] == 'catalogue@ilifu.ac.za' + + +def test_should_bound_every_smtp_send_with_a_socket_timeout_by_default() -> None: + """Django's SMTP backend sets no socket timeout of its own, so a hung + + relay would otherwise hold the Celery worker process for as long as the + OS keeps the socket alive — on the single-core production host that is + the same process that applies ingest snapshots. + """ + assert django_settings_from(Settings())['EMAIL_TIMEOUT'] == 10 + + +def test_should_refuse_tls_and_ssl_together_when_both_are_set( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Django refuses this pair too — but only at send time, which after the + + move to a Celery task is post-response in a worker where nobody is + watching. Refusing at boot states the problem while someone still is. + """ + monkeypatch.setenv('EMAIL_USE_TLS', '1') + monkeypatch.setenv('EMAIL_USE_SSL', '1') + + with pytest.raises(ValueError, match='EMAIL_USE_TLS and EMAIL_USE_SSL'): + Settings() + + def test_should_reject_the_configuration_when_the_database_url_is_not_postgres( monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -172,6 +218,26 @@ def test_should_route_celery_the_cache_and_ingest_staging_to_separate_redis_data assert django_settings['CACHES'][INGEST_CACHE_ALIAS]['LOCATION'] == 'redis://redis:6379/2' +def test_should_bound_the_broker_publish_when_the_broker_hangs_rather_than_refuses() -> None: + """The request form's enqueue is guarded, but a guard only helps if the + + publish *returns*. A refused connection fails fast on its own; a hung + broker (dropped packets, a wedged host) holds the socket for as long as + the OS allows, multiplied by kombu's default three publish retries — + the same unbounded-wait shape the async send removed from SMTP, back in + the request path. Same two-second bounds as the cache aliases, and one + quick publish retry rather than three: the caller is a human whose + submit already succeeded, not a collector that can wait. + """ + django_settings = django_settings_from(Settings()) + + transport_options = django_settings['CELERY_BROKER_TRANSPORT_OPTIONS'] + assert transport_options['socket_connect_timeout'] == 2 + assert transport_options['socket_timeout'] == 2 + retry_policy = django_settings['CELERY_TASK_PUBLISH_RETRY_POLICY'] + assert retry_policy['max_retries'] == 1 + + def test_should_keep_sessions_readable_when_the_cache_loses_everything() -> None: """Sessions must not be a pure-cache value. @@ -374,11 +440,19 @@ def test_should_default_to_the_devauth_admin_fixture( 'OIDC_OP_USER_ENDPOINT': 'https://sso.example.ac.za/userinfo', 'OIDC_OP_JWKS_ENDPOINT': 'https://sso.example.ac.za/certs', 'ILIFU_ADMIN_EMAILS': 'admin@example.ac.za', + 'EMAIL_HOST': 'smtp-relay.example.ac.za', + 'DEFAULT_FROM_EMAIL': 'catalogue@example.ac.za', } -class TestProductionRequiresTheAllowList: - """A production deployment must not be able to boot with no admin configured. +class TestProductionRequiresTheFieldsWithUnusableDefaults: + """A production deployment must not be able to boot half-configured. + + The admin allow-list defaults to a devauth fixture user who cannot exist + in production; the mail settings default to Django's own + `localhost:25` / `webmaster@localhost`, which in the container means every + request notification dies in a worker where nobody is watching. Both are + the same shape: a default that silently produces a broken deployment. These import `settings.prod` for real, because the guarantee lives in the *absence* of a default on the model — which only bites at instantiation, @@ -427,6 +501,26 @@ def test_should_refuse_to_boot_without_the_admin_allow_list( with pytest.raises(ValueError, match='ilifu_admin_emails'): self._import_prod(monkeypatch, without_allow_list) + def test_should_refuse_to_boot_without_an_email_host( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + without_email_host = { + name: value for name, value in PROD_ENV.items() if name != 'EMAIL_HOST' + } + + with pytest.raises(ValueError, match='email_host'): + self._import_prod(monkeypatch, without_email_host) + + def test_should_refuse_to_boot_without_a_from_address( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + without_from_address = { + name: value for name, value in PROD_ENV.items() if name != 'DEFAULT_FROM_EMAIL' + } + + with pytest.raises(ValueError, match='default_from_email'): + self._import_prod(monkeypatch, without_from_address) + @pytest.mark.parametrize('setting_name', ['LOGIN_REDIRECT_URL', 'LOGOUT_REDIRECT_URL']) def test_should_resolve_the_post_auth_redirect_targets_to_a_real_view(setting_name: str) -> None: diff --git a/compose.prod.yaml b/compose.prod.yaml index 62dc7e9..40020c9 100644 --- a/compose.prod.yaml +++ b/compose.prod.yaml @@ -29,11 +29,11 @@ services: # them alongside Django's own structured JSON logging. # # --threads switches to the gthread worker: 4×4 = 16 concurrent requests - # instead of 4, so one slow database round-trip (or the request form's - # synchronous send_mail) occupies a thread rather than a quarter of the - # site — and static files, served in-process by WhiteNoise, stop queuing - # behind slow dynamic requests. Workers stay at 4 rather than scaling - # with CPUs: threads deliver the concurrency without multiplying memory. + # instead of 4, so one slow database round-trip occupies a thread rather + # than a quarter of the site — and static files, served in-process by + # WhiteNoise, stop queuing behind slow dynamic requests. Workers stay at + # 4 rather than scaling with CPUs: threads deliver the concurrency + # without multiplying memory. # # --worker-tmp-dir puts gunicorn's worker heartbeat file in RAM. Its # default is a disk-backed tmp dir, and the arbiter treats a heartbeat