diff --git a/backend/cases/approvals.py b/backend/cases/approvals.py index 0b073a54..876f5345 100644 --- a/backend/cases/approvals.py +++ b/backend/cases/approvals.py @@ -148,15 +148,20 @@ def closing_date(case, *, status, closed_on): themselves, each with its own idea of the org's timezone, and one of them could not date a close at all for an org stored under a legacy zone name such as ``US/Eastern``. Every close path calls this: the serializer (POST, - PUT, PATCH, bulk and macro apply) and the board move. + PUT, PATCH, bulk and macro apply), the board move, close-with-children, + and the ``Case`` pre-save signal for every other writer (CSV import, the + packs applier). - Only the transition is dated: a ticket already Closed keeps whatever it - has, so an edit to an old closed ticket does not re-date it. + A write that leaves the ticket Closed always leaves a date. One that sends + none, or sends ``null``, to a ticket already Closed keeps the stored date, + so an edit to an old closed ticket does not re-date it; only when there is + no stored date either is it dated today. A write that leaves the ticket + open returns what it sent; the signal clears the date on the way out. """ if closed_on or status != "Closed": return closed_on - if case is not None and case.status == "Closed": - return closed_on + if case is not None and case.status == "Closed" and case.closed_on: + return case.closed_on return timezone.localdate() diff --git a/backend/cases/inbound/parser.py b/backend/cases/inbound/parser.py index 94b19525..a11930a3 100644 --- a/backend/cases/inbound/parser.py +++ b/backend/cases/inbound/parser.py @@ -43,6 +43,10 @@ class ParsedEmail: received_at: datetime attachments: list[ParsedAttachment] = field(default_factory=list) is_bounce: bool = False + # Every `Delivered-To` and `X-Original-To` address, the headers a relay + # stamps with the envelope recipient it delivered to. Read by the webhook + # to tell whether a raw message was addressed to its mailbox. + delivered_to: list[str] = field(default_factory=list) def _strip_brackets(value: str) -> str: @@ -193,6 +197,12 @@ def parse_raw_email(raw: bytes | str) -> ParsedEmail: received_at=_coerce_timestamp(raw_headers.get("Date")), attachments=_extract_attachments(msg), is_bounce=_is_bounce(msg), + delivered_to=[ + addr + for name in ("Delivered-To", "X-Original-To") + for value in msg.get_all(name) or [] + for addr in _addresses_from(str(value)) + ], ) diff --git a/backend/cases/inbound/sns.py b/backend/cases/inbound/sns.py index 4aa6be82..02fa5971 100644 --- a/backend/cases/inbound/sns.py +++ b/backend/cases/inbound/sns.py @@ -112,6 +112,28 @@ _SUBSCRIPTION_KEYS = ( "Type", ) +# An SNS topic ARN, `arn:aws:sns:::`, in the +# commercial `aws` partition only: SES email receiving, the one producer the +# inbound webhook serves, does not run in the China or GovCloud partitions. A +# standard topic name is 1-256 letters, digits, hyphens and underscores; SES +# cannot publish to a FIFO topic, so a `.fifo` name is refused too. Applied +# with `fullmatch` for the trailing-newline reason given above. +_TOPIC_ARN_RE = re.compile( + r"arn:aws:sns:[a-z]{2}(?:-[a-z]+)+-\d{1,2}:(\d{12}):[A-Za-z0-9_-]{1,256}" +) + + +def topic_account_id(arn) -> str | None: + """The 12-digit AWS account id in a well-formed SNS topic ARN, else None. + + Anything that is not a string, or not exactly the shape above, is None, so + a caller comparing the result against a set of allowed ids refuses it. + """ + if not isinstance(arn, str): + return None + match = _TOPIC_ARN_RE.fullmatch(arn) + return match.group(1) if match else None + class SNSVerificationError(Exception): """Raised when an SNS message fails signature verification.""" diff --git a/backend/cases/inbound_views.py b/backend/cases/inbound_views.py index 5c2a062a..eed69d15 100644 --- a/backend/cases/inbound_views.py +++ b/backend/cases/inbound_views.py @@ -13,16 +13,33 @@ it takes both. The signature alone is not enough: because the pipeline threads replies onto existing cases, means injecting messages into live customer conversations, not just spam tickets. -A mailbox with no pin rejects notifications outright; it acquires one from the -first signature-verified SubscriptionConfirmation and will not silently re-pin. +A mailbox gets its pin one of two ways. An admin enters the Topic ARN on the +mailbox, or, while it has none, a signature-verified SubscriptionConfirmation +pins its TopicArn, but only when that topic belongs to an AWS account listed in +the `INBOUND_SNS_ACCOUNT_IDS` setting. SNS lets any AWS account subscribe any +HTTPS endpoint to its own topic, so pinning whichever confirmation came first +handed the mailbox to whoever subscribed it first. With no pin and no allowed +account, every message is refused. Once pinned, only that exact ARN is accepted +and a confirmation never re-pins. + +Neither check says the mail was meant for *this* mailbox. One topic may fan +out to many mailboxes (a platform-wide SES receipt rule, say, now that +`INBOUND_SNS_ACCOUNT_IDS` lets every mailbox pin a topic in the platform +account), so each would receive every org's mail. A notification is therefore +ingested only when the mailbox's address is one of its recipients, and is +otherwise acknowledged and discarded without writing anything in this org. """ from __future__ import annotations +import base64 +import binascii +import email.utils import json import logging from datetime import timedelta +from django.conf import settings from django.db.models import Count, Max from django.utils import timezone from drf_spectacular.utils import extend_schema, inline_serializer @@ -36,11 +53,15 @@ from cases.inbound.pipeline import ingest from cases.inbound.sns import ( SNSVerificationError, confirm_subscription, + topic_account_id, verify_sns_message, ) from cases.models import EmailMessage, InboundMailbox from cases.serializer import InboundMailboxSerializer +from common.models import PortalAccessToken +from common.org_time import activate_org_timezone from common.permissions import HasOrgContext, is_org_admin +from common.portal_tokens import resolve_portal_org from common.tasks import set_rls_context logger = logging.getLogger(__name__) @@ -96,6 +117,50 @@ def _admin_required(): ) +def _unwrap_sns_message(message): + """Split an SNS Notification's `Message` into (raw email, recipients). + + SES's SNS receipt action publishes a JSON notification: the raw email in + `content`, and in `receipt.recipients` the envelope RCPT TO addresses the + receipt rule matched. SES sets those, Bcc included, and the sender cannot + forge them. A `Message` that is not a JSON object is taken to be the raw + email itself, and the recipients come back as None: the caller then has + only the message's own headers to go on. + """ + try: + envelope = json.loads(message) + except (ValueError, TypeError): + return message, None + if not isinstance(envelope, dict): + return message, None + receipt = envelope.get("receipt") + recipients = receipt.get("recipients") if isinstance(receipt, dict) else None + if not isinstance(recipients, list): + recipients = [] + content = envelope.get("content") + if not isinstance(content, str): + content = "" + # The SNS action's Encoding option may be Base64. A raw email always has a + # `Name: value` header, and ":" is outside the base64 alphabet, so content + # without one is the encoded form. + if content and ":" not in content: + try: + content = base64.b64decode("".join(content.split()), validate=True) + except (binascii.Error, ValueError): + pass + return content, [r for r in recipients if isinstance(r, str)] + + +def _addressed_to(mailbox, recipients): + """Whether `mailbox.address` is one of `recipients`, ignoring case, spaces + and display names. Exact otherwise: `support+x@` is a different address.""" + target = mailbox.address.strip().lower() + return any( + addr.strip().lower() == target + for _, addr in email.utils.getaddresses(recipients) + ) + + def _topic_rejected(): """Deliberately as opaque as the signature failure. A caller probing the webhook shouldn't learn whether a mailbox is pinned or to what.""" @@ -111,6 +176,14 @@ class InboundMailboxWebhookView(APIView): URL: `/api/cases/inbound//`. The mailbox lookup also acts as the org boundary, the URL embeds the per-mailbox UUID so the webhook can't be confused for one belonging to a different tenant. + + The request is anonymous, so it carries no org, and `inbound_mailbox` is + org-scoped: under the non-superuser production role an empty RLS context + hides the very row that would name the org. The org therefore comes first + from the unscoped `PortalAccessToken` lookup (registered when the mailbox + is created, see cases/signals.py), then the context is set, and only then + is the mailbox read, within that org. `RequireOrgContext` exempts this one + route by name for the same reason. """ authentication_classes = () @@ -140,21 +213,33 @@ class InboundMailboxWebhookView(APIView): }, ) def post(self, request, mailbox_id, *args, **kwargs): + # Don't leak which UUIDs exist: an unknown id, one whose mailbox is + # inactive and one whose mailbox is gone all get this same 404. + not_found = Response( + {"error": True, "errors": "Mailbox not found"}, + status=status.HTTP_404_NOT_FOUND, + ) + # `mailbox_id` is already the canonical UUID string: the `uid` path + # converter normalises every form it accepts (any case, bare hex, + # braces, URN), and the lookup is keyed on that canonical form. + org_id = resolve_portal_org(mailbox_id, PortalAccessToken.INBOUND_MAILBOX) + if org_id is None: + return not_found + # No middleware sets a context for an anonymous request, so set it + # before any ORM read/write touches an org-scoped table. + set_rls_context(org_id) mailbox = ( - InboundMailbox.objects.filter(pk=mailbox_id, is_active=True) + InboundMailbox.objects.filter(pk=mailbox_id, org_id=org_id, is_active=True) .select_related("org") .first() ) if mailbox is None: - # Don't leak which UUIDs exist; return a generic 404. - return Response( - {"error": True, "errors": "Mailbox not found"}, - status=status.HTTP_404_NOT_FOUND, - ) + return not_found - # The webhook bypasses RLSContextMiddleware, so set it manually before - # any ORM read/write touches an org-scoped table. - set_rls_context(mailbox.org_id) + # Likewise the org's day: the reopen window a reply is judged against + # counts days since close on the org's calendar, not UTC's. + # `GetProfileAndOrg` deactivates it when the request ends. + activate_org_timezone(mailbox.org) if mailbox.provider != "ses": # Other providers wired into the same URL space land here. @@ -204,18 +289,27 @@ class InboundMailboxWebhookView(APIView): topic_arn, ) return _topic_rejected() - elif msg_type == "SubscriptionConfirmation" and topic_arn: - # Trust-on-first-use: the confirmation is signature-verified above, - # and this is the only moment a mailbox can acquire its pin. + elif ( + msg_type == "SubscriptionConfirmation" + and topic_account_id(topic_arn) in settings.INBOUND_SNS_ACCOUNT_IDS + ): + # No admin-entered ARN, and the topic belongs to an AWS account + # the operator allows. The confirmation is signature-verified + # above, so it really came from that account's topic. A topic in + # any other account pins nothing: SNS lets anyone subscribe this + # URL, and the first to do so would otherwise own the mailbox. mailbox.topic_arn = topic_arn mailbox.save(update_fields=["topic_arn"]) logger.info("Pinned mailbox=%s to TopicArn=%r", mailbox.id, topic_arn) else: - # Unpinned mailbox, and this message can't establish a pin. + # Unpinned mailbox, and this message can't establish a pin: it is + # a notification, or its topic is malformed or in an AWS account + # missing from INBOUND_SNS_ACCOUNT_IDS. logger.warning( - "Rejecting SNS message for unpinned mailbox=%s (type=%r)", + "Rejecting SNS message for unpinned mailbox=%s (type=%r, topic=%r)", mailbox.id, msg_type, + topic_arn, ) return _topic_rejected() @@ -240,17 +334,10 @@ class InboundMailboxWebhookView(APIView): status=status.HTTP_400_BAD_REQUEST, ) - # SES with "SNS Notification with full content" puts the raw RFC-5322 - # email in payload.Message as a plain string. Older SES configurations - # send a JSON envelope (`{"notificationType":"Received","content":"..."}`); - # peel that off if we see it. - raw_message = payload.get("Message", "") or "" - try: - envelope = json.loads(raw_message) - if isinstance(envelope, dict) and "content" in envelope: - raw_message = envelope.get("content") or "" - except (ValueError, TypeError): - pass + # SES's SNS action sends a JSON notification carrying the raw email; a + # bare raw RFC 5322 string (another publisher on the pinned topic) is + # accepted too. See `_unwrap_sns_message`. + raw_message, recipients = _unwrap_sns_message(payload.get("Message") or "") if not raw_message: return Response( @@ -259,6 +346,26 @@ class InboundMailboxWebhookView(APIView): ) parsed = parse_raw_email(raw_message) + if recipients is None: + # No SES envelope, so fall back to the headers a relay stamps with + # the delivery address, then To and Cc. + recipients = [ + *parsed.delivered_to, + *parsed.to_addresses, + *parsed.cc_addresses, + ] + if not _addressed_to(mailbox, recipients): + # The topic also serves other mailboxes and this mail belongs to + # one of them. Storing anything here, even a dropped audit row, + # would copy another tenant's mail into this org. A 200 stops SNS + # retrying; the log names the mailbox only, never an address. + logger.warning( + "Dropping SNS notification not addressed to mailbox=%s", mailbox.id + ) + return Response( + {"ok": True, "dropped": True, "reason": "not_addressed_to_mailbox"}, + status=status.HTTP_200_OK, + ) result = ingest(parsed, mailbox) return Response( diff --git a/backend/cases/migrations/0032_inboundmailbox_topic_arn_help_text.py b/backend/cases/migrations/0032_inboundmailbox_topic_arn_help_text.py new file mode 100644 index 00000000..211bd01a --- /dev/null +++ b/backend/cases/migrations/0032_inboundmailbox_topic_arn_help_text.py @@ -0,0 +1,22 @@ +# Generated by Django 6.1.1 on 2026-09-27 04:07 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("cases", "0031_escalationpolicy_next_response_hours"), + ] + + operations = [ + migrations.AlterField( + model_name="inboundmailbox", + name="topic_arn", + field=models.CharField( + blank=True, + default="", + help_text="SNS Topic ARN this mailbox accepts mail from. A valid SNS signature only proves a message came from some topic in some AWS account, so the webhook also requires this exact match. An admin can enter it. Left blank, it is pinned from the first verified SubscriptionConfirmation whose topic belongs to an AWS account listed in the INBOUND_SNS_ACCOUNT_IDS setting, and a topic in any other account pins nothing. Until it is set, every message is rejected.", + max_length=256, + ), + ), + ] diff --git a/backend/cases/models.py b/backend/cases/models.py index 78c539d6..48efa2eb 100644 --- a/backend/cases/models.py +++ b/backend/cases/models.py @@ -799,10 +799,12 @@ class InboundMailbox(BaseModel): blank=True, default="", help_text="SNS Topic ARN this mailbox accepts mail from. A valid SNS " - "signature only proves a message came from some topic, so the webhook " - "also requires this exact match. Pinned automatically from the first " - "verified SubscriptionConfirmation when left blank; until it is set, " - "notifications are rejected.", + "signature only proves a message came from some topic in some AWS " + "account, so the webhook also requires this exact match. An admin can " + "enter it. Left blank, it is pinned from the first verified " + "SubscriptionConfirmation whose topic belongs to an AWS account listed " + "in the INBOUND_SNS_ACCOUNT_IDS setting, and a topic in any other " + "account pins nothing. Until it is set, every message is rejected.", ) # Reserved for future IMAP support (Tier 1+ follow-up). diff --git a/backend/cases/serializer.py b/backend/cases/serializer.py index 3328507e..04b1e961 100644 --- a/backend/cases/serializer.py +++ b/backend/cases/serializer.py @@ -10,6 +10,7 @@ from cases.access import ( visible_cases_qs, ) from cases.approvals import Approval, ApprovalRule, close_refusal, closing_date +from cases.inbound.sns import topic_account_id from cases.models import ( Case, CasePipeline, @@ -376,8 +377,9 @@ class CaseCreateSerializer(serializers.ModelSerializer): if refusal: raise serializers.ValidationError({"parent": refusal}) - # A close that sends no date is dated today in the org's timezone; - # one the caller sends wins. See `closing_date`. + # A ticket left Closed always has a date: one the caller sends wins, + # an absent or null one keeps the stored date, else today in the org's + # timezone. See `closing_date`. closed_on = attrs.get("closed_on", getattr(self.instance, "closed_on", None)) dated = closing_date(self.instance, status=new_status, closed_on=closed_on) if dated != closed_on: @@ -808,14 +810,15 @@ class InboundMailboxSerializer(serializers.ModelSerializer): write_only=True, required=False, allow_blank=True, max_length=128 ) has_webhook_secret = serializers.SerializerMethodField() - # Whether the mailbox has acquired its SNS TopicArn pin. + # Whether the mailbox has its SNS TopicArn pin. # # Not a credential and not admin-only: it is delivery status. An active SES - # mailbox with no pin rejects every notification (`InboundMailboxWebhookView - # .post` answers `_topic_rejected()` until a signature-verified - # SubscriptionConfirmation sets one), so without this a client can only - # report `is_active` and would draw a never-connected address as working. - # The ARN itself stays admin-only below, since it embeds the AWS account id. + # mailbox with no pin rejects every message (`InboundMailboxWebhookView + # .post` answers `_topic_rejected()` until an admin enters the ARN or a + # verified SubscriptionConfirmation from an allowed AWS account sets it), + # so without this a client can only report `is_active` and would draw a + # never-connected address as working. The ARN itself stays admin-only + # below, since it embeds the AWS account id. has_topic_arn = serializers.SerializerMethodField() class Meta: @@ -885,6 +888,17 @@ class InboundMailboxSerializer(serializers.ModelSerializer): ) return value + def validate_topic_arn(self, value): + """Blank clears the pin; anything else must be a well-formed SNS topic + ARN, since the webhook compares it byte for byte and a near miss would + refuse every delivery without a word.""" + if value and topic_account_id(value) is None: + raise serializers.ValidationError( + "Enter an SNS topic ARN, like " + "arn:aws:sns:us-east-1:123456789012:inbound-mail." + ) + return value + def get_has_webhook_secret(self, instance) -> bool: return bool(instance.webhook_secret) @@ -893,8 +907,9 @@ class InboundMailboxSerializer(serializers.ModelSerializer): def to_representation(self, instance): """`topic_arn` is the webhook's TopicArn pin and embeds the AWS account - id, so it is shown only to admins, who manage the integration, and only - when the view passes the request in context. `has_webhook_secret` rides + id, so it is shown only to admins, who manage the integration and may + set, change or clear it, and only when the view passes the request in + context. Every write to this serializer is admin-only in the views. `has_webhook_secret` rides with it because both describe the same admin-only integration config. `has_topic_arn` deliberately does NOT ride with them. It says whether diff --git a/backend/cases/signals.py b/backend/cases/signals.py index b5ca2e46..3acf66fb 100644 --- a/backend/cases/signals.py +++ b/backend/cases/signals.py @@ -30,9 +30,11 @@ from django.db.models.signals import ( from django.dispatch import receiver from django.utils import timezone -from cases.models import Case, ReopenPolicy, Solution, TimeEntry +from cases.approvals import closing_date +from cases.models import Case, InboundMailbox, ReopenPolicy, Solution, TimeEntry from cases.workflow import resolve_sla_targets -from common.models import Activity, Comment +from common.models import Activity, Comment, PortalAccessToken +from common.portal_tokens import register_portal_token REOPEN_TRIGGER_STATUS = "Closed" REOPEN_DEFAULTS = { @@ -240,6 +242,7 @@ def case_pre_save_sla_pause(sender, instance, **kwargs): RESOLVED_STATUSES = ("Closed",) +OPEN_STATUSES = ("New", "Assigned", "Pending") @receiver(pre_save, sender=Case) @@ -266,16 +269,32 @@ def case_pre_save_stamp_resolved_at(sender, instance, **kwargs): would keep it instead of being dated today. The customer-reply reopen path in `_evaluate_reopen` has always cleared it; a status change made by hand did not. + + A Closed ticket is also given its `closed_on` here when the writer left it + empty, by the same `closing_date` rule the API uses. The API paths date the + close themselves; this covers the writers that go straight to the model, + the CSV import and the packs applier among them, which created Closed + tickets with no date. """ old = getattr(instance, "_audit_old", None) was_resolved = old is not None and old.status in RESOLVED_STATUSES is_resolved = instance.status in RESOLVED_STATUSES + if is_resolved: + instance.closed_on = closing_date( + old, status=instance.status, closed_on=instance.closed_on + ) if is_resolved and not was_resolved and instance.resolved_at is None: instance.resolved_at = timezone.now() elif was_resolved and not is_resolved: instance.resolved_at = None instance.closed_on = None + if instance.status in OPEN_STATUSES: + # An open ticket never carries a close date, however it was sent: a + # PATCH of {"status": "New", "closed_on": ...} would otherwise store + # one, and the next close would keep it instead of dating today. + # Rejected and Duplicate are left alone (a merge dates Duplicates). + instance.closed_on = None def _maybe_stamp_first_response(case, comment): @@ -800,3 +819,35 @@ def comment_post_save_emit_activity(sender, instance, created, **kwargs): "after": new_is_internal, } _create_activity(case, "COMMENT", metadata, actor=actor) + + +# --------------------------------------------------------------------------- +# Inbound mailbox -> org lookup for the anonymous webhook +# +# SNS POSTs to `/api/cases/inbound//` with no credential, so the +# request carries no org, and under RLS an empty context hides the +# `inbound_mailbox` row that would name one. The webhook resolves the org from +# the unscoped `PortalAccessToken` row kept here, keyed on the SHA-256 of the +# mailbox id, then sets the context and reads the mailbox. Registered once on +# create (the id never changes) and removed on delete. Deactivating keeps the +# row: the webhook's scoped query filters on `is_active`, so an inactive +# mailbox still answers 404, and reactivating it needs no re-registration. +# --------------------------------------------------------------------------- + + +@receiver(post_save, sender=InboundMailbox) +def inbound_mailbox_register_lookup(sender, instance, created, raw=False, **kwargs): + if created and not raw: + register_portal_token( + str(instance.pk), + instance.org_id, + PortalAccessToken.INBOUND_MAILBOX, + instance.pk, + ) + + +@receiver(post_delete, sender=InboundMailbox) +def inbound_mailbox_remove_lookup(sender, instance, **kwargs): + PortalAccessToken.objects.filter( + resource_type=PortalAccessToken.INBOUND_MAILBOX, resource_id=instance.pk + ).delete() diff --git a/backend/cases/tests/test_close_dated_by_server.py b/backend/cases/tests/test_close_dated_by_server.py index aace4e41..3bef1ebd 100644 --- a/backend/cases/tests/test_close_dated_by_server.py +++ b/backend/cases/tests/test_close_dated_by_server.py @@ -16,10 +16,15 @@ import datetime from unittest.mock import patch import pytest +from django.core.files.uploadedfile import SimpleUploadedFile +from django.utils import timezone from rest_framework import status from cases.approvals import Approval, ApprovalRule from cases.models import Case +from common.org_time import activate_org_timezone +from common.packs.applier import apply_pack +from common.packs.loader import get_pack pytestmark = pytest.mark.django_db @@ -341,3 +346,262 @@ class TestOtherClosePaths: "Closed", datetime.date(2026, 8, 7), ) + + +STORED = datetime.date(2026, 7, 1) + + +def _closed(case, closed_on=STORED): + """Put ``case`` in Closed straight in the table, bypassing the signal, as a + ticket closed before the date rule existed would be.""" + Case.objects.filter(pk=case.pk).update(status="Closed", closed_on=closed_on) + + +class TestExplicitNullNeverLeavesAClosedTicketUndated: + """A write whose resulting status is Closed always leaves a `closed_on`. + + `null` (or no key) on a ticket already Closed used to pass through + untouched, because `closing_date` only dated the transition into Closed: + `PATCH {"closed_on": null}` left a Closed ticket with no date. It now keeps + the stored date, and only when there is none is it dated today. + """ + + def test_patch_null_keeps_the_stored_date(self, admin_client, case_a): + _closed(case_a) + resp = admin_client.patch(_detail(case_a), {"closed_on": None}, format="json") + assert resp.status_code == status.HTTP_200_OK, resp.content + case_a.refresh_from_db() + assert (case_a.status, case_a.closed_on) == ("Closed", STORED) + + def test_patch_null_with_status_closed_keeps_the_stored_date( + self, admin_client, case_a + ): + _closed(case_a) + resp = admin_client.patch( + _detail(case_a), {"status": "Closed", "closed_on": None}, format="json" + ) + assert resp.status_code == status.HTTP_200_OK, resp.content + case_a.refresh_from_db() + assert case_a.closed_on == STORED + + def test_patch_on_a_closed_ticket_with_no_date_dates_it_today( + self, admin_client, case_a, org_a, frozen + ): + _closed(case_a, closed_on=None) + _in_zone(org_a, "Asia/Kolkata") + with frozen(KOLKATA_AHEAD): + resp = admin_client.patch( + _detail(case_a), {"closed_on": None}, format="json" + ) + assert resp.status_code == status.HTTP_200_OK, resp.content + case_a.refresh_from_db() + assert case_a.closed_on == datetime.date(2026, 8, 7) + + def test_put_null_keeps_the_stored_date(self, admin_client, case_a, mailer): + _closed(case_a) + resp = admin_client.put( + _detail(case_a), + { + "name": case_a.name, + "status": "Closed", + "priority": "Low", + "closed_on": None, + }, + format="json", + ) + assert resp.status_code == status.HTTP_200_OK, resp.content + case_a.refresh_from_db() + assert case_a.closed_on == STORED + + def test_put_null_on_a_transition_is_dated_today( + self, admin_client, case_a, org_a, frozen, mailer + ): + _in_zone(org_a, "Asia/Kolkata") + with frozen(KOLKATA_AHEAD): + resp = admin_client.put( + _detail(case_a), + { + "name": case_a.name, + "status": "Closed", + "priority": "Low", + "closed_on": None, + }, + format="json", + ) + assert resp.status_code == status.HTTP_200_OK, resp.content + case_a.refresh_from_db() + assert (case_a.status, case_a.closed_on) == ( + "Closed", + datetime.date(2026, 8, 7), + ) + + def test_bulk_null_keeps_the_stored_date(self, admin_client, case_a, mailer): + _closed(case_a) + resp = admin_client.post( + "/api/cases/bulk/update/", + { + "ids": [str(case_a.pk)], + "fields": {"status": "Closed", "closed_on": None}, + }, + content_type="application/json", + ) + assert resp.json()["updated"] == 1, resp.content + case_a.refresh_from_db() + assert case_a.closed_on == STORED + + def test_bulk_null_on_a_transition_is_dated_today( + self, admin_client, case_a, org_a, frozen, mailer + ): + _in_zone(org_a, "Asia/Kolkata") + with frozen(KOLKATA_AHEAD): + resp = admin_client.post( + "/api/cases/bulk/update/", + { + "ids": [str(case_a.pk)], + "fields": {"status": "Closed", "closed_on": None}, + }, + content_type="application/json", + ) + assert resp.json()["updated"] == 1, resp.content + case_a.refresh_from_db() + assert (case_a.status, case_a.closed_on) == ( + "Closed", + datetime.date(2026, 8, 7), + ) + + def _macro_close(self, client, org, case): + from macros.models import Macro + + macro = Macro.objects.create( + org=org, title="Wrap up", body="", scope="org", set_status="Closed" + ) + return client.post( + f"/api/macros/{macro.pk}/apply/", {"case_id": str(case.pk)}, format="json" + ) + + def test_macro_close_on_a_closed_ticket_keeps_the_stored_date( + self, admin_client, case_a, org_a, mailer + ): + _closed(case_a) + resp = self._macro_close(admin_client, org_a, case_a) + assert resp.status_code == status.HTTP_200_OK, resp.content + case_a.refresh_from_db() + assert case_a.closed_on == STORED + + def test_macro_close_on_an_undated_closed_ticket_dates_it_today( + self, admin_client, case_a, org_a, frozen, mailer + ): + _closed(case_a, closed_on=None) + _in_zone(org_a, "US/Eastern") + with frozen(EASTERN_BEHIND): + resp = self._macro_close(admin_client, org_a, case_a) + assert resp.status_code == status.HTTP_200_OK, resp.content + case_a.refresh_from_db() + assert case_a.closed_on == datetime.date(2026, 8, 6) + + def test_a_null_on_an_open_ticket_stays_null(self, admin_client, case_a): + resp = admin_client.patch(_detail(case_a), {"closed_on": None}, format="json") + assert resp.status_code == status.HTTP_200_OK, resp.content + case_a.refresh_from_db() + assert (case_a.status, case_a.closed_on) == ("New", None) + + def test_leaving_closed_with_a_date_still_clears_it(self, admin_client, case_a): + _closed(case_a) + resp = admin_client.patch( + _detail(case_a), + {"status": "New", "closed_on": "2026-08-01"}, + format="json", + ) + assert resp.status_code == status.HTTP_200_OK, resp.content + case_a.refresh_from_db() + assert (case_a.status, case_a.closed_on) == ("New", None) + + +class TestWritersThatGoStraightToTheModel: + """The pre-save signal applies `closing_date` to every save, so a writer + that skips the serializer (CSV import, the packs applier) cannot leave a + Closed ticket undated either.""" + + def test_create_closed_without_a_date(self, org_a, frozen): + _in_zone(org_a, "Asia/Kolkata") + activate_org_timezone(org_a) + try: + with frozen(KOLKATA_AHEAD): + case = Case.objects.create( + name="Imported closed", status="Closed", priority="Low", org=org_a + ) + finally: + timezone.deactivate() + assert case.closed_on == datetime.date(2026, 8, 7) + case.refresh_from_db() + assert case.closed_on == datetime.date(2026, 8, 7) + + def test_save_with_none_on_a_closed_ticket_keeps_the_stored_date(self, case_a): + _closed(case_a) + case_a.refresh_from_db() + case_a.closed_on = None + case_a.save() + case_a.refresh_from_db() + assert case_a.closed_on == STORED + + def test_csv_import_of_a_closed_row_without_a_date( + self, admin_client, org_a, frozen + ): + body = b"name,status,priority\nImported closed,Closed,Low\n" + _in_zone(org_a, "Asia/Kolkata") + with frozen(KOLKATA_AHEAD): + resp = admin_client.post( + "/api/cases/import/commit/", + {"file": SimpleUploadedFile("t.csv", body, "text/csv")}, + format="multipart", + ) + assert resp.status_code == status.HTTP_200_OK, resp.content + case = Case.objects.get(name="Imported closed") + assert (case.status, case.closed_on) == ("Closed", datetime.date(2026, 8, 7)) + + def test_pack_sample_tickets_that_are_closed_are_dated(self, org_a, admin_profile): + apply_pack(org_a, get_pack("real-estate"), admin_profile) + closed = Case.objects.filter(org=org_a, is_sample=True, status="Closed") + assert closed.exists() + assert not closed.filter(closed_on__isnull=True).exists() + + +class TestAnOpenTicketCarriesNoCloseDate: + """An open ticket never holds a `closed_on`, however it was sent, so the + next close is dated then rather than inheriting a stale date.""" + + def test_date_sent_with_an_open_status_is_dropped(self, admin_client, case_a): + resp = admin_client.patch( + _detail(case_a), + {"status": "New", "closed_on": "2020-01-01"}, + format="json", + ) + assert resp.status_code == status.HTTP_200_OK, resp.content + case_a.refresh_from_db() + assert case_a.status == "New" + assert case_a.closed_on is None + + def test_next_close_is_dated_today_not_the_stale_date( + self, admin_client, case_a, org_a, frozen + ): + _in_zone(org_a, "Asia/Kolkata") + admin_client.patch( + _detail(case_a), {"status": "New", "closed_on": "2020-01-01"}, format="json" + ) + with frozen(KOLKATA_AHEAD): + resp = admin_client.patch( + _detail(case_a), {"status": "Closed"}, format="json" + ) + assert resp.status_code == status.HTTP_200_OK, resp.content + case_a.refresh_from_db() + assert case_a.closed_on == datetime.date(2026, 8, 7) + + def test_rejected_keeps_a_date_it_was_given(self, admin_client, case_a): + resp = admin_client.patch( + _detail(case_a), + {"status": "Rejected", "closed_on": "2026-08-01"}, + format="json", + ) + assert resp.status_code == status.HTTP_200_OK, resp.content + case_a.refresh_from_db() + assert case_a.closed_on == datetime.date(2026, 8, 1) diff --git a/backend/cases/tests/test_inbound_email.py b/backend/cases/tests/test_inbound_email.py index e90f3bd5..133c34da 100644 --- a/backend/cases/tests/test_inbound_email.py +++ b/backend/cases/tests/test_inbound_email.py @@ -1032,8 +1032,11 @@ class TestTopicArnPinning: assert response.status_code == 403 def test_subscription_confirmation_pins_an_unpinned_mailbox( - self, admin_client, org_a + self, admin_client, org_a, settings ): + # Only a topic in an allowed AWS account may pin; see + # test_inbound_topic_pin.py for the refusals. + settings.INBOUND_SNS_ACCOUNT_IDS = frozenset({"123456789012"}) mailbox = _make_mailbox(org_a, topic_arn="") with ( patch("cases.inbound_views.verify_sns_message"), diff --git a/backend/cases/tests/test_inbound_recipient.py b/backend/cases/tests/test_inbound_recipient.py new file mode 100644 index 00000000..1f7ba62d --- /dev/null +++ b/backend/cases/tests/test_inbound_recipient.py @@ -0,0 +1,262 @@ +"""The inbound webhook ingests only mail addressed to its own mailbox. + +Signature and TopicArn checks prove a notification came from the mailbox's +topic, not that the mail was meant for the mailbox. One topic can fan out to +many mailboxes (a platform-wide SES receipt rule, with every mailbox pinned to +a topic in the platform account), and before this check every subscribed org +ingested every org's mail as tickets. + +The recipients come from SES's envelope (`receipt.recipients`, the RCPT TO +addresses, Bcc included, which the sender cannot forge) when the SNS Message is +SES's JSON notification, and from Delivered-To, X-Original-To, To and Cc when +it is a bare raw email. Every signature below is genuinely verified. +""" + +from __future__ import annotations + +import base64 +import json +import logging + +import pytest +from cryptography.hazmat.primitives.asymmetric import rsa +from rest_framework.test import APIClient + +from cases.inbound.parser import parse_raw_email +from cases.models import Case, EmailMessage +from cases.tests.test_inbound_email import SNS_TOPIC, _make_mailbox, _raw_email +from cases.tests.test_inbound_sns import _signed_payload +from cases.tests.test_inbound_topic_pin import _post, _real_verification + +NOT_ADDRESSED = {"ok": True, "dropped": True, "reason": "not_addressed_to_mailbox"} + + +@pytest.fixture +def anon(): + return APIClient() + + +@pytest.fixture(scope="module") +def signing_key(): + return rsa.generate_private_key(public_exponent=65537, key_size=2048) + + +def _ses_json(raw, recipients, *, encode=False): + """SES's SNS-action notification, shaped as AWS documents it.""" + return json.dumps( + { + "notificationType": "Received", + "receipt": { + "recipients": recipients, + "action": {"type": "SNS", "topicArn": SNS_TOPIC}, + }, + "mail": {"destination": recipients, "messageId": "ses-1"}, + "content": base64.b64encode(raw.encode()).decode() if encode else raw, + } + ) + + +def _deliver(client, mailbox, key, message): + payload = _signed_payload(key, topic_arn=SNS_TOPIC, Message=message) + with _real_verification(key): + return _post(client, mailbox, payload) + + +def _counts(org): + return ( + Case.objects.filter(org=org).count(), + EmailMessage.objects.filter(org=org).count(), + ) + + +@pytest.mark.django_db +class TestSesEnvelope: + def test_mail_for_the_mailbox_is_ingested(self, anon, org_a, signing_key): + mailbox = _make_mailbox(org_a) + response = _deliver( + anon, mailbox, signing_key, _ses_json(_raw_email(), ["support@acme.com"]) + ) + assert response.status_code == 200, response.content + assert response.json()["created_case"] is True + assert _counts(org_a) == (1, 1) + + def test_mail_for_another_orgs_mailbox_is_dropped_and_stores_nothing( + self, anon, org_a, org_b, signing_key, caplog + ): + """The leak itself: both mailboxes on one topic, mail for org_b posted + to org_a's webhook. Nothing may land in either org.""" + mailbox_a = _make_mailbox(org_a) + _make_mailbox(org_b, address="help@beta.com") + raw = _raw_email(to="help@beta.com", subject="Beta payroll question") + + with caplog.at_level(logging.WARNING, logger="cases.inbound_views"): + response = _deliver( + anon, mailbox_a, signing_key, _ses_json(raw, ["help@beta.com"]) + ) + + assert response.status_code == 200, response.content + assert response.json() == NOT_ADDRESSED + assert _counts(org_a) == (0, 0) + assert _counts(org_b) == (0, 0) + logged = " ".join(r.getMessage() for r in caplog.records) + assert str(mailbox_a.id) in logged + assert "beta.com" not in logged and "acme.com" not in logged + + def test_a_fanned_out_notification_lands_only_in_the_addressed_org( + self, anon, org_a, org_b, signing_key + ): + mailbox_a = _make_mailbox(org_a) + mailbox_b = _make_mailbox(org_b, address="help@beta.com") + message = _ses_json(_raw_email(to="help@beta.com"), ["help@beta.com"]) + + dropped = _deliver(anon, mailbox_a, signing_key, message) + ingested = _deliver(anon, mailbox_b, signing_key, message) + + assert dropped.json() == NOT_ADDRESSED + assert ingested.json()["created_case"] is True + assert _counts(org_a) == (0, 0) + assert _counts(org_b) == (1, 1) + assert EmailMessage.objects.get(org=org_b).mailbox_id == mailbox_b.id + + def test_bcc_delivery_is_ingested_although_to_and_cc_name_others( + self, anon, org_a, signing_key + ): + mailbox = _make_mailbox(org_a) + raw = _raw_email( + to="someone@elsewhere.com", extra_headers="Cc: other@elsewhere.com" + ) + response = _deliver( + anon, mailbox, signing_key, _ses_json(raw, ["support@acme.com"]) + ) + assert response.status_code == 200, response.content + assert response.json()["created_case"] is True + assert _counts(org_a) == (1, 1) + + def test_a_to_header_naming_the_mailbox_does_not_override_the_envelope( + self, anon, org_a, signing_key + ): + """Headers are the sender's to write. With an SES envelope present, a + To naming this mailbox must not pull in mail SES delivered elsewhere.""" + mailbox = _make_mailbox(org_a) + raw = _raw_email(to="support@acme.com") + response = _deliver( + anon, mailbox, signing_key, _ses_json(raw, ["help@beta.com"]) + ) + assert response.json() == NOT_ADDRESSED + assert _counts(org_a) == (0, 0) + + def test_match_ignores_case_whitespace_and_display_names( + self, anon, org_a, signing_key + ): + mailbox = _make_mailbox(org_a, address="Support@Acme.com") + response = _deliver( + anon, + mailbox, + signing_key, + _ses_json(_raw_email(), [" Help Desk "]), + ) + assert response.status_code == 200, response.content + assert response.json()["created_case"] is True + + def test_a_plus_address_is_a_different_mailbox(self, anon, org_a, signing_key): + mailbox = _make_mailbox(org_a) + response = _deliver( + anon, + mailbox, + signing_key, + _ses_json(_raw_email(), ["support+billing@acme.com"]), + ) + assert response.json() == NOT_ADDRESSED + assert _counts(org_a) == (0, 0) + + def test_an_envelope_without_recipients_is_dropped(self, anon, org_a, signing_key): + """Fail closed: a JSON envelope with no `receipt.recipients` does not + fall back to the forgeable headers.""" + mailbox = _make_mailbox(org_a) + message = json.dumps({"notificationType": "Received", "content": _raw_email()}) + response = _deliver(anon, mailbox, signing_key, message) + assert response.json() == NOT_ADDRESSED + assert _counts(org_a) == (0, 0) + + def test_base64_content_is_decoded(self, anon, org_a, signing_key): + """The SNS action's Encoding option may be Base64. Read as-is, the + content has no headers and was recorded as a missing-Message-ID drop.""" + mailbox = _make_mailbox(org_a) + raw = _raw_email(subject="Café login", body="Ça ne marche pas.") + response = _deliver( + anon, + mailbox, + signing_key, + _ses_json(raw, ["support@acme.com"], encode=True), + ) + assert response.status_code == 200, response.content + assert response.json()["created_case"] is True + row = EmailMessage.objects.get(org=org_a) + assert row.message_id == "m1@example.com" + assert row.subject == "Café login" + assert "Ça ne marche pas." in row.body_text + + def test_base64_content_addressed_elsewhere_is_still_dropped( + self, anon, org_a, signing_key + ): + mailbox = _make_mailbox(org_a) + response = _deliver( + anon, + mailbox, + signing_key, + _ses_json(_raw_email(), ["help@beta.com"], encode=True), + ) + assert response.json() == NOT_ADDRESSED + assert _counts(org_a) == (0, 0) + + +@pytest.mark.django_db +class TestRawMessageHeaders: + @pytest.mark.parametrize( + "to, extra", + [ + ("support@acme.com", ""), + ("someone@elsewhere.com", "Cc: Support "), + ("someone@elsewhere.com", "Delivered-To: support@acme.com"), + ("someone@elsewhere.com", "X-Original-To: support@acme.com"), + ], + ids=["to", "cc", "delivered-to", "x-original-to"], + ) + def test_a_matching_header_is_ingested(self, anon, org_a, signing_key, to, extra): + mailbox = _make_mailbox(org_a) + raw = _raw_email(to=to, extra_headers=extra) + response = _deliver(anon, mailbox, signing_key, raw) + assert response.status_code == 200, response.content + assert response.json()["created_case"] is True + assert _counts(org_a) == (1, 1) + + def test_no_matching_header_is_dropped_and_stores_nothing( + self, anon, org_a, org_b, signing_key + ): + mailbox = _make_mailbox(org_a) + raw = _raw_email( + to="help@beta.com", + extra_headers="Cc: other@beta.com\r\nDelivered-To: help@beta.com", + ) + response = _deliver(anon, mailbox, signing_key, raw) + assert response.status_code == 200, response.content + assert response.json() == NOT_ADDRESSED + assert _counts(org_a) == (0, 0) + assert _counts(org_b) == (0, 0) + + +def test_parser_collects_every_delivered_to_and_x_original_to(): + parsed = parse_raw_email( + _raw_email( + extra_headers=( + "Delivered-To: First@Relay.test\r\n" + "Delivered-To: second@relay.test\r\n" + "X-Original-To: Desk " + ) + ) + ) + assert parsed.delivered_to == [ + "first@relay.test", + "second@relay.test", + "orig@relay.test", + ] diff --git a/backend/cases/tests/test_inbound_topic_pin.py b/backend/cases/tests/test_inbound_topic_pin.py new file mode 100644 index 00000000..54714a98 --- /dev/null +++ b/backend/cases/tests/test_inbound_topic_pin.py @@ -0,0 +1,454 @@ +"""Who may pin an inbound mailbox to an SNS topic. + +The webhook used to pin whatever TopicArn the first signature-valid +SubscriptionConfirmation carried. SNS lets any AWS account subscribe any HTTPS +endpoint to its own topic, and any org member can read mailbox ids from +`GET /api/cases/mailboxes/`, so an attacker could subscribe an unpinned +mailbox's URL to their own topic, win the pin, and then open tickets as any +sender while the real topic was refused. + +A mailbox now pins only (a) the ARN an admin entered, or (b) while it has none, +a confirmation whose topic belongs to an AWS account in +`INBOUND_SNS_ACCOUNT_IDS`. With neither, every message is refused. +""" + +from __future__ import annotations + +import base64 +import functools +import os +import subprocess +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest +from cryptography.hazmat.primitives import hashes +from cryptography.hazmat.primitives.asymmetric import padding, rsa +from rest_framework.test import APIClient + +from cases.inbound import sns +from cases.inbound.sns import topic_account_id, verify_sns_message +from cases.models import Case, EmailMessage +from cases.tests.test_inbound_email import _make_mailbox, _raw_email +from cases.tests.test_inbound_sns import _make_cert, _pem, _signed_payload + +MAILBOXES_URL = "/api/cases/mailboxes/" +BACKEND_DIR = Path(__file__).resolve().parents[2] + +OWNER_ACCOUNT = "123456789012" +ATTACKER_ACCOUNT = "999999999999" +OWNER_TOPIC = f"arn:aws:sns:us-east-1:{OWNER_ACCOUNT}:acme-inbound" +OWNER_OTHER_TOPIC = f"arn:aws:sns:eu-west-1:{OWNER_ACCOUNT}:other-inbound" +ATTACKER_TOPIC = f"arn:aws:sns:us-east-1:{ATTACKER_ACCOUNT}:attacker-topic" +REJECTED = {"error": True, "errors": "Signature verification failed"} + + +@pytest.fixture +def anon(): + return APIClient() + + +@pytest.fixture(scope="module") +def signing_key(): + return rsa.generate_private_key(public_exponent=65537, key_size=2048) + + +def _real_verification(signing_key): + """The production verifier with only the certificate download stubbed, so + every signature below is genuinely checked. A valid signature is exactly + what an attacker's own topic gets from AWS.""" + cert = _make_cert(signing_key) + return patch( + "cases.inbound_views.verify_sns_message", + functools.partial(verify_sns_message, fetch_cert=lambda url: _pem(cert)), + ) + + +def _signed_confirmation(key, topic_arn): + payload = { + "Type": "SubscriptionConfirmation", + "MessageId": "c-1", + "Token": "tok", + "TopicArn": topic_arn, + "Message": "You have chosen to subscribe to the topic.", + "SubscribeURL": "https://sns.us-east-1.amazonaws.com/?Action=ConfirmSubscription", + "Timestamp": "2026-05-09T12:00:00.000Z", + "SignatureVersion": "1", + "SigningCertURL": ( + "https://sns.us-east-1.amazonaws.com/SimpleNotificationService-x.pem" + ), + } + string_to_sign = sns._build_string_to_sign(payload, sns._SUBSCRIPTION_KEYS) + payload["Signature"] = base64.b64encode( + key.sign(string_to_sign, padding.PKCS1v15(), hashes.SHA1()) + ).decode() + return payload + + +def _post(client, mailbox, payload): + return client.post(f"/api/cases/inbound/{mailbox.id}/", payload, format="json") + + +def _confirm(client, mailbox, key, topic_arn): + """Post a signed confirmation; return (response, whether SNS was called).""" + with ( + _real_verification(key), + patch("cases.inbound_views.confirm_subscription") as confirm, + ): + response = _post(client, mailbox, _signed_confirmation(key, topic_arn)) + return response, confirm.called + + +def _notify(client, mailbox, key, topic_arn, **email): + payload = _signed_payload(key, topic_arn=topic_arn, Message=_raw_email(**email)) + with _real_verification(key): + return _post(client, mailbox, payload) + + +# --------------------------------------------------------------------------- +# The reviewer's reproduction, kept as a regression test +# --------------------------------------------------------------------------- + + +class TestAttackerCannotWinThePin: + def test_reviewer_reproduction(self, anon, user_client, org_a, signing_key): + """Any member reads the id, the attacker subscribes it to their own + topic, then mails in as the CEO. Every step must now fail, and the + real topic entered by an admin must still work afterwards.""" + mailbox = _make_mailbox(org_a, topic_arn="") + listed = user_client.get(MAILBOXES_URL).json()["mailboxes"] + assert [row["id"] for row in listed] == [str(mailbox.id)] + + response, subscribed = _confirm(anon, mailbox, signing_key, ATTACKER_TOPIC) + assert response.status_code == 403 + assert response.json() == REJECTED + assert subscribed is False, "must not fetch an attacker's SubscribeURL" + mailbox.refresh_from_db() + assert mailbox.topic_arn == "" + + forged = _notify( + anon, mailbox, signing_key, ATTACKER_TOPIC, from_="CEO " + ) + assert forged.status_code == 403 + assert Case.objects.filter(org=org_a).count() == 0 + assert EmailMessage.objects.filter(org=org_a).count() == 0 + + mailbox.topic_arn = OWNER_TOPIC + mailbox.save(update_fields=["topic_arn"]) + real = _notify(anon, mailbox, signing_key, OWNER_TOPIC) + assert real.status_code == 200, real.content + assert Case.objects.filter(org=org_a).count() == 1 + + def test_empty_setting_refuses_every_confirmation( + self, anon, org_a, signing_key, settings + ): + settings.INBOUND_SNS_ACCOUNT_IDS = frozenset() + mailbox = _make_mailbox(org_a, topic_arn="") + + for topic in (ATTACKER_TOPIC, OWNER_TOPIC): + response, subscribed = _confirm(anon, mailbox, signing_key, topic) + assert response.status_code == 403 + assert subscribed is False + mailbox.refresh_from_db() + assert mailbox.topic_arn == "" + + def test_a_different_allowed_account_does_not_admit_the_attacker( + self, anon, org_a, signing_key, settings + ): + settings.INBOUND_SNS_ACCOUNT_IDS = frozenset({OWNER_ACCOUNT}) + mailbox = _make_mailbox(org_a, topic_arn="") + + response, subscribed = _confirm(anon, mailbox, signing_key, ATTACKER_TOPIC) + + assert response.status_code == 403 + assert response.json() == REJECTED + assert subscribed is False + mailbox.refresh_from_db() + assert mailbox.topic_arn == "" + + +# --------------------------------------------------------------------------- +# The two ways a pin is allowed to happen +# --------------------------------------------------------------------------- + + +class TestAllowedPins: + def test_a_confirmation_from_an_allowed_account_pins( + self, anon, org_a, signing_key, settings + ): + settings.INBOUND_SNS_ACCOUNT_IDS = frozenset({ATTACKER_ACCOUNT, OWNER_ACCOUNT}) + mailbox = _make_mailbox(org_a, topic_arn="") + + response, subscribed = _confirm(anon, mailbox, signing_key, OWNER_TOPIC) + + assert response.status_code == 200, response.content + assert subscribed is True + mailbox.refresh_from_db() + assert mailbox.topic_arn == OWNER_TOPIC + + # Pinned now: the same account's other topic is refused, and so is a + # later confirmation trying to move the pin. + response, subscribed = _confirm(anon, mailbox, signing_key, OWNER_OTHER_TOPIC) + assert response.status_code == 403 + assert subscribed is False + mailbox.refresh_from_db() + assert mailbox.topic_arn == OWNER_TOPIC + + def test_an_admin_entered_arn_accepts_exactly_that_topic( + self, anon, admin_client, org_a, signing_key, settings + ): + # The account is even allowed; the entered ARN still wins. + settings.INBOUND_SNS_ACCOUNT_IDS = frozenset({OWNER_ACCOUNT}) + mailbox = _make_mailbox(org_a, topic_arn="") + put = admin_client.put( + f"{MAILBOXES_URL}{mailbox.id}/", {"topic_arn": OWNER_TOPIC}, format="json" + ) + assert put.status_code == 200, put.content + + for topic in (ATTACKER_TOPIC, OWNER_OTHER_TOPIC): + response, subscribed = _confirm(anon, mailbox, signing_key, topic) + assert response.status_code == 403 + assert subscribed is False + assert _notify(anon, mailbox, signing_key, topic).status_code == 403 + assert Case.objects.filter(org=org_a).count() == 0 + + response, subscribed = _confirm(anon, mailbox, signing_key, OWNER_TOPIC) + assert response.status_code == 200, response.content + assert subscribed is True + assert _notify(anon, mailbox, signing_key, OWNER_TOPIC).status_code == 200 + assert Case.objects.filter(org=org_a).count() == 1 + mailbox.refresh_from_db() + assert mailbox.topic_arn == OWNER_TOPIC + + def test_an_admin_entered_arn_works_with_no_allowed_account( + self, anon, org_a, signing_key, settings + ): + settings.INBOUND_SNS_ACCOUNT_IDS = frozenset() + mailbox = _make_mailbox(org_a, topic_arn=OWNER_TOPIC) + + response, subscribed = _confirm(anon, mailbox, signing_key, OWNER_TOPIC) + + assert response.status_code == 200, response.content + assert subscribed is True + + def test_a_notification_before_any_pin_is_refused( + self, anon, org_a, signing_key, settings + ): + """Only a confirmation can pin, even from an allowed account.""" + settings.INBOUND_SNS_ACCOUNT_IDS = frozenset({OWNER_ACCOUNT}) + mailbox = _make_mailbox(org_a, topic_arn="") + + response = _notify(anon, mailbox, signing_key, OWNER_TOPIC) + + assert response.status_code == 403 + assert response.json() == REJECTED + assert Case.objects.filter(org=org_a).count() == 0 + mailbox.refresh_from_db() + assert mailbox.topic_arn == "" + + +# --------------------------------------------------------------------------- +# Strict ARN parsing +# --------------------------------------------------------------------------- + +# Wrong shapes. The API refuses each one with a 400. +BAD_SHAPES = [ + f"arn:aws-cn:sns:cn-north-1:{OWNER_ACCOUNT}:t", # China partition + f"arn:aws-us-gov:sns:us-gov-west-1:{OWNER_ACCOUNT}:t", # GovCloud partition + f"arn:aws:sqs:us-east-1:{OWNER_ACCOUNT}:t", # not SNS + f"arn:aws:sns:us-east-1:{OWNER_ACCOUNT}:t.fifo", # SES cannot publish to FIFO + f"arn:aws:sns:us-east-1:{OWNER_ACCOUNT}", # no topic name + f"arn:aws:sns:us-east-1:{OWNER_ACCOUNT}:t:extra", + f"arn:aws:sns::{OWNER_ACCOUNT}:t", # no region + "arn:aws:sns:us-east-1:12345678901:t", # 11 digits + "arn:aws:sns:us-east-1:1234567890123:t", # 13 digits + "arn:aws:sns:us-east-1:12345678901x:t", + f"arn:aws:sns:us-east-1:{OWNER_ACCOUNT}:" + "t" * 257, +] +# A well-formed ARN with whitespace around it. The webhook compares bytes and +# refuses these; the API trims them like every other text field, so it does not. +MALFORMED_ARNS = [ + *BAD_SHAPES, + f"{OWNER_TOPIC}\n", # `$` would match before this newline; fullmatch does not + f" {OWNER_TOPIC}", + "", +] + + +class TestStrictArnParsing: + def test_a_well_formed_arn_yields_its_account(self): + assert topic_account_id(OWNER_TOPIC) == OWNER_ACCOUNT + assert topic_account_id(ATTACKER_TOPIC) == ATTACKER_ACCOUNT + assert ( + topic_account_id(f"arn:aws:sns:ap-southeast-2:{OWNER_ACCOUNT}:A_b-9") + == OWNER_ACCOUNT + ) + + @pytest.mark.parametrize("arn", [*MALFORMED_ARNS, None, 123, [OWNER_TOPIC]]) + def test_anything_else_yields_none(self, arn): + assert topic_account_id(arn) is None + + @pytest.mark.parametrize("arn", MALFORMED_ARNS) + def test_a_malformed_arn_in_a_confirmation_pins_nothing( + self, anon, org_a, signing_key, settings, arn + ): + settings.INBOUND_SNS_ACCOUNT_IDS = frozenset({OWNER_ACCOUNT}) + mailbox = _make_mailbox(org_a, topic_arn="") + + response, subscribed = _confirm(anon, mailbox, signing_key, arn) + + assert response.status_code == 403 + assert subscribed is False + mailbox.refresh_from_db() + assert mailbox.topic_arn == "" + + +# --------------------------------------------------------------------------- +# The setting is checked when the process starts +# --------------------------------------------------------------------------- + + +def _import_settings(account_ids, env_type): + env = { + key: value + for key, value in os.environ.items() + if key + not in ( + "SECRET_KEY", + "ENV_TYPE", + "FRONTEND_URL", + "DOMAIN_NAME", + "INBOUND_SNS_ACCOUNT_IDS", + ) + } + env["SECRET_KEY"] = "k" * 48 + env["ENV_TYPE"] = env_type + env["FRONTEND_URL"] = "https://app.example.com" + env["DOMAIN_NAME"] = "https://api.example.com" + env["INBOUND_SNS_ACCOUNT_IDS"] = account_ids + return subprocess.run( + [ + sys.executable, + "-c", + "import crm.settings as s; print(sorted(s.INBOUND_SNS_ACCOUNT_IDS))", + ], + cwd=BACKEND_DIR, + env=env, + capture_output=True, + text=True, + ) + + +class TestSettingValidation: + @pytest.mark.parametrize( + "value", + [ + "12345678901", + "1234567890123", + "arn:aws:sns:us-east-1:123456789012:t", + "123456789012,abc", + "１２３４５６７８９０１２", # full-width digits are not ASCII + ], + ) + @pytest.mark.parametrize("env_type", ["production", "dev"]) + def test_a_malformed_id_refuses_to_start(self, value, env_type): + result = _import_settings(value, env_type) + assert result.returncode != 0 + assert "INBOUND_SNS_ACCOUNT_IDS" in result.stderr + + def test_ids_are_split_on_commas_and_trimmed(self): + result = _import_settings(" 123456789012 , 210987654321,", "production") + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == "['123456789012', '210987654321']" + + def test_unset_allows_no_account(self): + result = _import_settings("", "production") + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == "[]" + + +# --------------------------------------------------------------------------- +# Admin entry of the Topic ARN through the API +# --------------------------------------------------------------------------- + + +class TestAdminEntersTheArn: + def test_admin_can_set_change_and_clear_it(self, admin_client, org_a): + mailbox = _make_mailbox(org_a, topic_arn="") + url = f"{MAILBOXES_URL}{mailbox.id}/" + + for value in (OWNER_TOPIC, OWNER_OTHER_TOPIC, ""): + response = admin_client.put(url, {"topic_arn": value}, format="json") + assert response.status_code == 200, response.content + assert response.json()["topic_arn"] == value + assert response.json()["has_topic_arn"] is bool(value) + mailbox.refresh_from_db() + assert mailbox.topic_arn == value + + def test_admin_can_set_it_on_create(self, admin_client, org_a): + response = admin_client.post( + MAILBOXES_URL, + {"address": "new@acme.com", "provider": "ses", "topic_arn": OWNER_TOPIC}, + format="json", + ) + assert response.status_code == 201, response.content + assert response.json()["topic_arn"] == OWNER_TOPIC + assert response.json()["has_topic_arn"] is True + + @pytest.mark.parametrize("arn", BAD_SHAPES) + def test_a_malformed_arn_is_a_400(self, admin_client, org_a, arn): + mailbox = _make_mailbox(org_a, topic_arn=OWNER_TOPIC) + + response = admin_client.put( + f"{MAILBOXES_URL}{mailbox.id}/", {"topic_arn": arn}, format="json" + ) + + assert response.status_code == 400 + assert "topic_arn" in response.json()["errors"] + mailbox.refresh_from_db() + assert mailbox.topic_arn == OWNER_TOPIC + + def test_a_malformed_arn_on_create_is_a_400(self, admin_client, org_a): + response = admin_client.post( + MAILBOXES_URL, + {"address": "new@acme.com", "provider": "ses", "topic_arn": "nope"}, + format="json", + ) + assert response.status_code == 400 + assert "topic_arn" in response.json()["errors"] + + def test_admin_reads_it(self, admin_client, org_a): + _make_mailbox(org_a, topic_arn=OWNER_TOPIC) + row = admin_client.get(MAILBOXES_URL).json()["mailboxes"][0] + assert row["topic_arn"] == OWNER_TOPIC + assert row["has_topic_arn"] is True + + def test_member_reads_only_whether_it_is_set(self, user_client, org_a): + mailbox = _make_mailbox(org_a, topic_arn=OWNER_TOPIC) + row = user_client.get(MAILBOXES_URL).json()["mailboxes"][0] + assert "topic_arn" not in row + assert row["has_topic_arn"] is True + detail = user_client.get(f"{MAILBOXES_URL}{mailbox.id}/").json() + assert "topic_arn" not in detail + assert detail["has_topic_arn"] is True + + @pytest.mark.parametrize("value", [ATTACKER_TOPIC, ""]) + def test_member_cannot_write_it(self, user_client, org_a, value): + mailbox = _make_mailbox(org_a, topic_arn=OWNER_TOPIC) + + response = user_client.put( + f"{MAILBOXES_URL}{mailbox.id}/", {"topic_arn": value}, format="json" + ) + + assert response.status_code == 403 + mailbox.refresh_from_db() + assert mailbox.topic_arn == OWNER_TOPIC + + def test_member_cannot_create_one_with_it(self, user_client, org_a): + response = user_client.post( + MAILBOXES_URL, + {"address": "x@acme.com", "provider": "ses", "topic_arn": ATTACKER_TOPIC}, + format="json", + ) + assert response.status_code == 403 diff --git a/backend/cases/tests/test_inbound_webhook_reachability.py b/backend/cases/tests/test_inbound_webhook_reachability.py new file mode 100644 index 00000000..e2e1af6b --- /dev/null +++ b/backend/cases/tests/test_inbound_webhook_reachability.py @@ -0,0 +1,447 @@ +"""The inbound email webhook works for the caller it exists for: AWS SNS. + +SNS POSTs with no credential, so the request has no JWT and no org. The +webhook was dead in production twice over for that caller: + +1. `RequireOrgContext` refused it with 403 "Organization context is required", + because the path was not exempt. Every earlier webhook test signed in as an + admin, which supplied the org claim SNS never has, and hid it. +2. Past that, the view read `inbound_mailbox` (org-scoped, FORCE RLS) before it + set any context, so under the non-superuser production role the lookup saw + nothing and answered 404. + +The fix exempts this one route by URL name and resolves the org from the +unscoped `PortalAccessToken` lookup before the mailbox is read. Every test +here posts anonymously. The `postgres_only` tests at the foot prove the chain +under a role RLS actually binds; on SQLite and a superuser Postgres they would +pass vacuously, so they assert the empty context hides the mailbox first. +""" + +from __future__ import annotations + +import functools +import importlib +import uuid +from types import SimpleNamespace +from unittest.mock import patch + +import pytest +from cryptography.hazmat.primitives.asymmetric import rsa +from django.apps import apps as django_apps +from django.db import connection +from rest_framework.test import APIClient + +from cases.inbound.sns import verify_sns_message +from cases.models import Case, InboundMailbox +from cases.tests.test_inbound_email import ( + SNS_TOPIC, + _make_mailbox, + _notification, + _raw_email, +) +from cases.tests.test_inbound_sns import _make_cert, _pem, _signed_payload +from common.middleware.rls_context import RequireOrgContext +from common.models import Org, PortalAccessToken +from common.portal_tokens import ( + portal_token_hash, + register_portal_token, + resolve_portal_org, +) +from conftest import restore_rls_context, rls_org + +MAILBOXES_URL = "/api/cases/mailboxes/" +ORG_CONTEXT_REQUIRED = "Organization context is required. Please login again." +NOT_FOUND = {"error": True, "errors": "Mailbox not found"} + +backfill_migration = importlib.import_module( + "common.migrations.0049_portal_token_inbound_mailbox" +) + + +def _webhook(mailbox_id): + return f"/api/cases/inbound/{mailbox_id}/" + + +def _post(client, mailbox_id, payload): + return client.post(_webhook(mailbox_id), payload, format="json") + + +def _lookup_rows(mailbox): + return PortalAccessToken.objects.filter( + resource_type=PortalAccessToken.INBOUND_MAILBOX, resource_id=mailbox.id + ) + + +@pytest.fixture +def anon(): + return APIClient() + + +@pytest.fixture(scope="module") +def signing_key(): + return rsa.generate_private_key(public_exponent=65537, key_size=2048) + + +def _real_verification(signing_key): + """The production verifier with only the certificate download stubbed, so + the signature over the payload is genuinely checked.""" + cert = _make_cert(signing_key) + return patch( + "cases.inbound_views.verify_sns_message", + functools.partial(verify_sns_message, fetch_cert=lambda url: _pem(cert)), + ) + + +def _run_backfill(forward=True): + schema_editor = SimpleNamespace(connection=connection) + if forward: + backfill_migration.register_mailboxes(django_apps, schema_editor) + else: + backfill_migration.unregister_mailboxes(django_apps, schema_editor) + # The migration clears `app.current_org` on the way out, as it must. + restore_rls_context() + + +# --------------------------------------------------------------------------- +# Reachable with no credential at all +# --------------------------------------------------------------------------- + + +class TestAnonymousDelivery: + def test_signed_notification_creates_the_case(self, anon, org_a, signing_key): + mailbox = _make_mailbox(org_a) + payload = _signed_payload( + signing_key, topic_arn=SNS_TOPIC, Message=_raw_email() + ) + + with _real_verification(signing_key): + response = _post(anon, mailbox.id, payload) + + assert response.status_code == 200, response.content + assert response.json()["created_case"] is True + assert Case.objects.filter(org=org_a).count() == 1 + + def test_a_tampered_notification_is_still_refused(self, anon, org_a, signing_key): + """Reachability must not cost the signature check.""" + mailbox = _make_mailbox(org_a) + payload = _signed_payload( + signing_key, topic_arn=SNS_TOPIC, Message=_raw_email() + ) + payload["Message"] = _raw_email(subject="Forged after signing") + + with _real_verification(signing_key): + response = _post(anon, mailbox.id, payload) + + assert response.status_code == 403 + assert Case.objects.filter(org=org_a).count() == 0 + + def test_subscription_confirmation_pins_the_mailbox(self, anon, org_a, settings): + settings.INBOUND_SNS_ACCOUNT_IDS = frozenset({"123456789012"}) + mailbox = _make_mailbox(org_a, topic_arn="") + with ( + patch("cases.inbound_views.verify_sns_message"), + patch("cases.inbound_views.confirm_subscription") as confirm, + ): + response = _post( + anon, + mailbox.id, + { + "Type": "SubscriptionConfirmation", + "SubscribeURL": "https://sns.us-east-1.amazonaws.com/x", + "TopicArn": SNS_TOPIC, + }, + ) + + assert response.status_code == 200, response.content + assert confirm.called + mailbox.refresh_from_db() + assert mailbox.topic_arn == SNS_TOPIC + + @pytest.mark.parametrize( + "form", [str.upper, lambda s: s.replace("-", ""), lambda s: "{%s}" % s] + ) + def test_every_id_form_the_router_accepts_resolves(self, anon, org_a, form): + """The `uid` converter canonicalises before the view hashes the id, + so the lookup key matches however the id was written.""" + mailbox = _make_mailbox(org_a) + with patch("cases.inbound_views.verify_sns_message"): + response = _post( + anon, form(str(mailbox.id)), _notification(SNS_TOPIC, _raw_email()) + ) + assert response.status_code == 200, response.content + + def test_unknown_inactive_and_deleted_ids_answer_the_same_404(self, anon, org_a): + inactive = _make_mailbox(org_a, is_active=False) + deleted = _make_mailbox(org_a, address="gone@acme.com") + deleted_id = deleted.id + deleted.delete() + + responses = [ + _post(anon, mailbox_id, _notification(SNS_TOPIC)) + for mailbox_id in (uuid.uuid4(), inactive.id, deleted_id) + ] + + assert [r.status_code for r in responses] == [404, 404, 404] + assert [r.json() for r in responses] == [NOT_FOUND] * 3 + + def test_a_malformed_id_404s_without_reaching_the_view(self, anon): + with patch("cases.inbound_views.resolve_portal_org") as resolve: + response = _post(anon, "not-a-uuid", _notification(SNS_TOPIC)) + assert response.status_code == 404 + assert not resolve.called + + +# --------------------------------------------------------------------------- +# The exemption covers the webhook and nothing beside it +# --------------------------------------------------------------------------- + + +class TestExemptionIsOnlyTheWebhook: + def test_mailbox_list_and_create_still_require_org_context(self, anon, org_a): + for response in (anon.get(MAILBOXES_URL), anon.post(MAILBOXES_URL, {})): + assert response.status_code == 403 + assert response.json() == {"detail": ORG_CONTEXT_REQUIRED} + + def test_mailbox_detail_still_requires_org_context(self, anon, org_a): + url = f"{MAILBOXES_URL}{_make_mailbox(org_a).id}/" + for response in (anon.get(url), anon.put(url, {}), anon.delete(url)): + assert response.status_code == 403 + assert response.json() == {"detail": ORG_CONTEXT_REQUIRED} + + def test_the_exemption_is_by_name_not_by_path_prefix(self): + middleware = RequireOrgContext(get_response=None) + for path in ( + _webhook(uuid.uuid4()), + "/api/cases/inbound/", + MAILBOXES_URL, + "/api/cases/", + ): + assert middleware._is_exempt(path) is False + assert RequireOrgContext.EXEMPT_VIEW_NAMES == frozenset( + {"common_urls:api_cases:inbound_webhook"} + ) + + def test_a_lookalike_path_under_the_webhook_is_not_served(self, anon, org_a): + mailbox = _make_mailbox(org_a) + response = anon.post( + f"{_webhook(mailbox.id)}extra/", _notification(SNS_TOPIC), format="json" + ) + assert response.status_code == 404 + + +# --------------------------------------------------------------------------- +# Org resolution never lands in the wrong tenant +# --------------------------------------------------------------------------- + + +class TestOrgResolution: + def test_each_mailbox_resolves_to_its_own_org(self, anon, org_a, org_b): + mine = _make_mailbox(org_a) + theirs = _make_mailbox(org_b, address="help@beta.com") + + assert resolve_portal_org(str(mine.id), "inbound_mailbox") == str(org_a.id) + assert resolve_portal_org(str(theirs.id), "inbound_mailbox") == str(org_b.id) + + with patch("cases.inbound_views.verify_sns_message"): + response = _post( + anon, + theirs.id, + _notification(SNS_TOPIC, _raw_email(to="help@beta.com")), + ) + + assert response.status_code == 200, response.content + assert Case.objects.filter(org=org_a).count() == 0 + with rls_org(org_b): + assert Case.objects.filter(org=org_b).count() == 1 + + def test_a_lookup_row_naming_another_org_finds_no_mailbox(self, anon, org_a, org_b): + """The scoped read re-checks the org, so a lookup row that disagrees + with the mailbox (stale, or planted) cannot carry mail across.""" + theirs = _make_mailbox(org_b, address="help@beta.com") + _lookup_rows(theirs).update(org=org_a) + + with patch("cases.inbound_views.verify_sns_message"): + response = _post(anon, theirs.id, _notification(SNS_TOPIC, _raw_email())) + + assert response.status_code == 404 + assert response.json() == NOT_FOUND + assert Case.objects.filter(org=org_a).count() == 0 + + def test_a_token_of_another_resource_type_does_not_resolve(self, anon, org_a): + mailbox = _make_mailbox(org_a) + _lookup_rows(mailbox).update(resource_type=PortalAccessToken.INVOICE) + + with patch("cases.inbound_views.verify_sns_message"): + response = _post(anon, mailbox.id, _notification(SNS_TOPIC, _raw_email())) + + assert response.status_code == 404 + assert response.json() == NOT_FOUND + + +# --------------------------------------------------------------------------- +# The lookup row follows the mailbox +# --------------------------------------------------------------------------- + + +class TestRegistration: + def test_creating_through_the_api_registers_the_mailbox(self, admin_client, org_a): + response = admin_client.post( + MAILBOXES_URL, {"address": "new@acme.com", "provider": "ses"}, format="json" + ) + assert response.status_code == 201, response.content + + row = PortalAccessToken.objects.get( + token_hash=portal_token_hash(response.json()["id"]) + ) + assert row.org_id == org_a.id + assert row.resource_type == PortalAccessToken.INBOUND_MAILBOX + assert str(row.resource_id) == response.json()["id"] + + def test_deleting_through_the_api_removes_the_row(self, admin_client, org_a): + mailbox = _make_mailbox(org_a) + assert _lookup_rows(mailbox).count() == 1 + + response = admin_client.delete(f"{MAILBOXES_URL}{mailbox.id}/") + + assert response.status_code == 200 + assert _lookup_rows(mailbox).count() == 0 + + def test_an_update_neither_duplicates_nor_drops_the_row(self, admin_client, org_a): + mailbox = _make_mailbox(org_a) + response = admin_client.put( + f"{MAILBOXES_URL}{mailbox.id}/", {"is_active": False}, format="json" + ) + assert response.status_code == 200 + assert _lookup_rows(mailbox).count() == 1 + + def test_deactivating_keeps_the_row_and_the_webhook_404s_until_reactivated( + self, admin_client, anon, org_a + ): + mailbox = _make_mailbox(org_a) + url = f"{MAILBOXES_URL}{mailbox.id}/" + + admin_client.put(url, {"is_active": False}, format="json") + with patch("cases.inbound_views.verify_sns_message"): + off = _post(anon, mailbox.id, _notification(SNS_TOPIC, _raw_email())) + assert off.status_code == 404 + assert off.json() == NOT_FOUND + + admin_client.put(url, {"is_active": True}, format="json") + with patch("cases.inbound_views.verify_sns_message"): + on = _post(anon, mailbox.id, _notification(SNS_TOPIC, _raw_email())) + assert on.status_code == 200, on.content + + +# --------------------------------------------------------------------------- +# Backfill (common/0049) +# --------------------------------------------------------------------------- + + +class TestBackfill: + def test_forward_registers_every_mailbox_under_its_own_org(self, org_a, org_b): + mine = _make_mailbox(org_a) + paused = _make_mailbox(org_a, address="paused@acme.com", is_active=False) + theirs = _make_mailbox(org_b, address="help@beta.com") + # As the rows stood before this release: nothing registered. + PortalAccessToken.objects.filter(resource_type="inbound_mailbox").delete() + + _run_backfill() + + for mailbox, org in ((mine, org_a), (paused, org_a), (theirs, org_b)): + row = PortalAccessToken.objects.get( + token_hash=portal_token_hash(str(mailbox.id)) + ) + assert (row.org_id, row.resource_type, row.resource_id) == ( + org.id, + "inbound_mailbox", + mailbox.id, + ) + + def test_forward_is_idempotent(self, org_a): + mailbox = _make_mailbox(org_a) + _run_backfill() + _run_backfill() + assert _lookup_rows(mailbox).count() == 1 + + def test_reverse_removes_only_the_mailbox_rows(self, org_a): + mailbox = _make_mailbox(org_a) + register_portal_token("an-invoice-token", org_a.id, "invoice", uuid.uuid4()) + + _run_backfill(forward=False) + + assert _lookup_rows(mailbox).count() == 0 + assert PortalAccessToken.objects.filter(resource_type="invoice").count() == 1 + + +# --------------------------------------------------------------------------- +# Under a role RLS binds (RLS_ENFORCE run, --ds=crm.test_settings_postgres) +# --------------------------------------------------------------------------- + + +def _context(value): + with connection.cursor() as cursor: + cursor.execute("SELECT set_config('app.current_org', %s, false)", [value]) + + +def _enforced_mailbox(): + """An org and an active pinned mailbox, then an empty context, asserting + that the empty context really hides the mailbox (else the run is vacuous).""" + org = Org.objects.create(name="RLS Inbound Org") + _context(str(org.id)) + try: + mailbox = InboundMailbox.objects.create( + org=org, address="support@rls.test", provider="ses", topic_arn=SNS_TOPIC + ) + finally: + _context("") + assert not InboundMailbox.objects.filter(pk=mailbox.pk).exists(), ( + "The empty context exposed inbound_mailbox: this role bypasses RLS." + ) + return org, mailbox + + +@pytest.mark.postgres_only +def test_webhook_delivers_under_enforced_rls(anon): + if connection.vendor != "postgresql": + pytest.skip("RLS requires PostgreSQL") + org, mailbox = _enforced_mailbox() + + with patch("cases.inbound_views.verify_sns_message"): + response = _post( + anon, + mailbox.id, + _notification(SNS_TOPIC, _raw_email(to=mailbox.address)), + ) + + assert response.status_code == 200, response.content + assert response.json()["created_case"] is True + _context(str(org.id)) + try: + assert Case.objects.filter(org=org).count() == 1 + finally: + _context("") + + +@pytest.mark.postgres_only +def test_backfill_finds_mailboxes_the_empty_context_hides(): + if connection.vendor != "postgresql": + pytest.skip("RLS requires PostgreSQL") + org, mailbox = _enforced_mailbox() + _lookup_rows(mailbox).delete() + + _run_backfill() + + assert resolve_portal_org(str(mailbox.id), "inbound_mailbox") == str(org.id) + + +@pytest.mark.postgres_only +def test_admin_mailbox_endpoints_refuse_without_org_context(anon): + if connection.vendor != "postgresql": + pytest.skip("RLS requires PostgreSQL") + _org, mailbox = _enforced_mailbox() + for response in ( + anon.get(MAILBOXES_URL), + anon.get(f"{MAILBOXES_URL}{mailbox.id}/"), + ): + assert response.status_code == 403 + assert response.json() == {"detail": ORG_CONTEXT_REQUIRED} diff --git a/backend/cases/tests/test_next_response_time.py b/backend/cases/tests/test_next_response_time.py index df0c4dd8..32eb7b44 100644 --- a/backend/cases/tests/test_next_response_time.py +++ b/backend/cases/tests/test_next_response_time.py @@ -17,7 +17,13 @@ from django.contrib.contenttypes.models import ContentType from django.utils import timezone from cases import analytics -from cases.models import Case, CaseWatcher, EmailMessage, EscalationPolicy +from cases.models import ( + Case, + CaseWatcher, + EmailMessage, + EscalationPolicy, + ReopenPolicy, +) from common.models import Comment NOW = timezone.now().replace(microsecond=0) @@ -228,6 +234,9 @@ class TestBreaches: def test_unanswered_last_word_on_a_closed_ticket_is_not_owed( self, org_a, admin_user, admin_profile ): + # A Closed ticket is always dated now, so a customer reply inside the + # reopen window would reopen it; the policy is off to keep it Closed. + ReopenPolicy.objects.create(org=org_a, is_enabled=False) case = _case(org_a, admin_user, status="Closed") _comment(case, _h(1), by=admin_profile) _comment(case, NOW - timedelta(hours=20)) # "thanks!" diff --git a/backend/common/audit_log.py b/backend/common/audit_log.py index c48f69bf..7fe8c3ab 100644 --- a/backend/common/audit_log.py +++ b/backend/common/audit_log.py @@ -66,6 +66,13 @@ def _login_failure_row_allowed(request): return count <= LOGIN_FAILURE_ROWS_PER_IP_PER_HOUR +_CALENDAR_FEED_DESCRIPTIONS = { + "CALENDAR_FEED_ENABLED": "Calendar feed enabled", + "CALENDAR_FEED_REGENERATED": "Calendar feed URL regenerated", + "CALENDAR_FEED_DISABLED": "Calendar feed disabled", +} + + class SecurityAuditLog(BaseModel): """ Model to store security audit events. @@ -92,6 +99,11 @@ class SecurityAuditLog(BaseModel): ("WEBHOOK_REENABLED", "Webhook Re-enabled"), ("WEBHOOK_CHANGED", "Webhook Destination Changed"), ("RECORD_MERGED", "Record Merged"), + ("CALENDAR_FEED_ENABLED", "Calendar Feed Enabled"), + ("CALENDAR_FEED_REGENERATED", "Calendar Feed Regenerated"), + ("CALENDAR_FEED_DISABLED", "Calendar Feed Disabled"), + ("API_TOKEN_CREATED", "API Token Created"), + ("API_TOKEN_REVOKED", "API Token Revoked"), ) event_type = models.CharField(max_length=50, choices=EVENT_TYPES, db_index=True) @@ -431,6 +443,54 @@ class AuditLogger: request=request, ) + def calendar_feed(self, event_type, user, org, request=None): + """Log a member enabling, regenerating or disabling their own task + calendar feed (G14). ``event_type`` is one of the three + ``CALENDAR_FEED_*`` events. + + The feed URL is a standing credential, so its lifecycle is recorded + here. Nothing about the token is: not the URL, not the raw token, not + its hash. + """ + self._log( + event_type, + user=user, + org=org, + description=_CALENDAR_FEED_DESCRIPTIONS[event_type], + request=request, + ) + + def api_token_created(self, user, pat, request=None): + """Log a personal access token created by its owner.""" + self._api_token("API_TOKEN_CREATED", "created", user, pat, request) + + def api_token_revoked(self, user, pat, request=None): + """Log a personal access token revoked, by its owner or by an admin + from ``/api/org/tokens/``.""" + self._api_token("API_TOKEN_REVOKED", "revoked", user, pat, request) + + def _api_token(self, event_type, verb, user, pat, request): + """``user`` is who acted; ``owner_id`` and ``owner_name`` say whose + token it was. Only the display prefix the token list already shows is + kept, never the raw token or its hash. + """ + owner = pat.profile.user + self._log( + event_type, + user=user, + org=pat.org, + description=f"API token {pat.token_prefix} {verb}", + metadata={ + "token_id": str(pat.id), + "token_prefix": pat.token_prefix, + "token_name": pat.name, + "scopes": list(pat.scopes or []), + "owner_id": str(owner.id), + "owner_name": owner.name or owner.email, + }, + request=request, + ) + def suspicious_activity(self, user, org, activity_type, details, request=None): """Log suspicious activity for security review.""" self._log( diff --git a/backend/common/management/commands/seed_data.py b/backend/common/management/commands/seed_data.py index 1414fdd9..9efee6a5 100644 --- a/backend/common/management/commands/seed_data.py +++ b/backend/common/management/commands/seed_data.py @@ -25,6 +25,7 @@ from django.utils import timezone from faker import Faker from common.models import Org, Profile, Tags, Teams, User +from common.org_time import activate_org_timezone from common.rls import get_set_context_sql from common.utils import ( CASE_TYPE, @@ -551,6 +552,8 @@ class Command(BaseCommand): self.seed_all(options) except Exception as e: raise CommandError(f"Seeding failed: {e}") from e + finally: + timezone.deactivate() elapsed = (timezone.now() - start_time).total_seconds() self.print_summary(elapsed) @@ -618,6 +621,9 @@ class Command(BaseCommand): org = self.create_org(options["currency"], options["country"], i) # Set RLS context for this org before creating org-scoped data self.set_rls_context(org.id) + # Seeded dates ("today", "overdue", issue dates) in the org's day, + # as a request would see them; a command runs no middleware. + activate_org_timezone(org) profiles = self.create_profiles( org, options["users_per_org"], options["password"] ) diff --git a/backend/common/middleware/rls_context.py b/backend/common/middleware/rls_context.py index f1a4b032..5f25a262 100644 --- a/backend/common/middleware/rls_context.py +++ b/backend/common/middleware/rls_context.py @@ -219,6 +219,24 @@ class RequireOrgContext: "/api/auth/logout/", ] + # Routes exempt by their resolved, namespaced URL name, for an anonymous + # route whose path carries an id and so cannot be an exact path, and which + # sits beside authenticated routes that a prefix would also catch. + # + # The inbound email webhook (`/api/cases/inbound//`) is POSTed + # by AWS SNS with no credential, so it has no org claim to require, and + # without this entry every delivery answered 403. The view authenticates + # the delivery itself (SNS signature plus the mailbox's TopicArn pin) and + # resolves the org from the unscoped `PortalAccessToken` lookup before it + # sets the RLS context and reads `inbound_mailbox`. See + # cases/inbound_views.py. + # + # By name, not by prefix: "/api/cases/" or "/api/cases/inbound/" in + # EXEMPT_PATHS would reach further than this one route as soon as anything + # else is mounted there, and the admin mailbox endpoints under /api/cases/ + # must keep requiring org context like everything else. + EXEMPT_VIEW_NAMES = frozenset({"common_urls:api_cases:inbound_webhook"}) + def __init__(self, get_response): self.get_response = get_response @@ -230,11 +248,13 @@ class RequireOrgContext: from django.urls.exceptions import Resolver404 try: - resolve(request.path) + match = resolve(request.path) except Resolver404: return self.get_response(request) - if not hasattr(request, "org") or request.org is None: + if match.view_name not in self.EXEMPT_VIEW_NAMES and ( + not hasattr(request, "org") or request.org is None + ): return JsonResponse( {"detail": "Organization context is required. Please login again."}, status=403, diff --git a/backend/common/migrations/0048_securityauditlog_credential_events.py b/backend/common/migrations/0048_securityauditlog_credential_events.py new file mode 100644 index 00000000..f8d612b9 --- /dev/null +++ b/backend/common/migrations/0048_securityauditlog_credential_events.py @@ -0,0 +1,44 @@ +# Generated by Django 6.1.1 on 2026-09-27 01:02 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("common", "0047_calendar_feed_token"), + ] + + operations = [ + migrations.AlterField( + model_name="securityauditlog", + name="event_type", + field=models.CharField( + choices=[ + ("LOGIN_SUCCESS", "Login Success"), + ("LOGIN_FAILURE", "Login Failure"), + ("LOGOUT", "Logout"), + ("ORG_SWITCH", "Organization Switch"), + ("TOKEN_REFRESH", "Token Refresh"), + ("TOKEN_REVOKED", "Token Revoked"), + ("PERMISSION_DENIED", "Permission Denied"), + ("CROSS_ORG_ATTEMPT", "Cross-Org Access Attempt"), + ("API_KEY_USED", "API Key Used"), + ("API_KEY_INVALID", "Invalid API Key"), + ("MEMBERSHIP_REVOKED", "Membership Revoked"), + ("SUSPICIOUS_ACTIVITY", "Suspicious Activity"), + ("SAMPLE_DATA_CLEARED", "Vertical Pack Sample Data Cleared"), + ("WEBHOOK_PAUSED", "Webhook Paused"), + ("WEBHOOK_REENABLED", "Webhook Re-enabled"), + ("WEBHOOK_CHANGED", "Webhook Destination Changed"), + ("RECORD_MERGED", "Record Merged"), + ("CALENDAR_FEED_ENABLED", "Calendar Feed Enabled"), + ("CALENDAR_FEED_REGENERATED", "Calendar Feed Regenerated"), + ("CALENDAR_FEED_DISABLED", "Calendar Feed Disabled"), + ("API_TOKEN_CREATED", "API Token Created"), + ("API_TOKEN_REVOKED", "API Token Revoked"), + ], + db_index=True, + max_length=50, + ), + ), + ] diff --git a/backend/common/migrations/0049_portal_token_inbound_mailbox.py b/backend/common/migrations/0049_portal_token_inbound_mailbox.py new file mode 100644 index 00000000..5f45313a --- /dev/null +++ b/backend/common/migrations/0049_portal_token_inbound_mailbox.py @@ -0,0 +1,89 @@ +"""Register every existing inbound mailbox in the unscoped token -> org lookup. + +The inbound email webhook (`/api/cases/inbound//`) is anonymous, +so it now resolves the mailbox's org from `portal_access_token` before it sets +the RLS context and reads `inbound_mailbox`. New mailboxes are registered when +they are created (cases/signals.py); this gives the existing ones their row, +keyed on the SHA-256 of the mailbox id in canonical UUID form, exactly as +`common.portal_tokens.portal_token_hash(str(mailbox.id))` computes it. + +THE ORG LOOP IS NOT DECORATION + +`inbound_mailbox` is under FORCE ROW LEVEL SECURITY keyed on +`app.current_org`, which is empty during a migration, so an unscoped read sees +no mailbox in production while working on a superuser dev database and on +SQLite. This walks the orgs, which are not org-scoped, and sets the context for +each, the same shape as opportunity/0020. `portal_access_token` itself has no +policy, so the writes need no context. + +Idempotent (an upsert on `token_hash`) and reversible: the reverse deletes the +inbound mailbox rows and nothing else. Deactivated mailboxes are registered +too; the webhook's own scoped query filters on `is_active`. +""" + +import hashlib + +from django.db import migrations, models + +INBOUND_MAILBOX = "inbound_mailbox" + + +def register_mailboxes(apps, schema_editor): + connection = schema_editor.connection + is_postgres = connection.vendor == "postgresql" + + def set_context(value): + if not is_postgres: + return + with connection.cursor() as cursor: + cursor.execute("SELECT set_config('app.current_org', %s, false)", [value]) + + Org = apps.get_model("common", "Org") + InboundMailbox = apps.get_model("cases", "InboundMailbox") + PortalAccessToken = apps.get_model("common", "PortalAccessToken") + + try: + for org_id in Org.objects.values_list("id", flat=True).iterator(): + set_context(str(org_id)) + for mailbox_id in InboundMailbox.objects.filter(org_id=org_id).values_list( + "id", flat=True + ): + PortalAccessToken.objects.update_or_create( + token_hash=hashlib.sha256(str(mailbox_id).encode()).hexdigest(), + defaults={ + "org_id": org_id, + "resource_type": INBOUND_MAILBOX, + "resource_id": mailbox_id, + }, + ) + finally: + set_context("") + + +def unregister_mailboxes(apps, schema_editor): + PortalAccessToken = apps.get_model("common", "PortalAccessToken") + PortalAccessToken.objects.filter(resource_type=INBOUND_MAILBOX).delete() + + +class Migration(migrations.Migration): + dependencies = [ + ("common", "0048_securityauditlog_credential_events"), + ("cases", "0031_escalationpolicy_next_response_hours"), + ] + + operations = [ + migrations.AlterField( + model_name="portalaccesstoken", + name="resource_type", + field=models.CharField( + choices=[ + ("invoice", "Invoice"), + ("estimate", "Estimate"), + ("csat", "CSAT survey"), + ("inbound_mailbox", "Inbound mailbox"), + ], + max_length=16, + ), + ), + migrations.RunPython(register_mailboxes, unregister_mailboxes), + ] diff --git a/backend/common/models.py b/backend/common/models.py index 73484a88..a355420a 100644 --- a/backend/common/models.py +++ b/backend/common/models.py @@ -134,9 +134,10 @@ class Org(BaseModel): ) # The org's calendar day. Every "today", "overdue" and "this month" in the # app is resolved against this, because a day boundary is a fact about the - # people using the org, not about the server. `RequireOrgContext` activates - # it per request so `timezone.localdate()` answers in it; Celery has no - # middleware and must call `common.tasks.set_org_timezone` itself. + # people using the org, not about the server. `GetProfileAndOrg` activates + # it per request so `timezone.localdate()` answers in it; Celery tasks and + # management commands have no middleware and must call + # `common.org_time.activate_org_timezone` themselves. # # UTC by default so a client that never sends one, an older mobile build in # particular, still creates a usable org. Stored as an IANA name rather than @@ -1196,10 +1197,13 @@ class PortalAccessToken(models.Model): The key is ``sha256(url_token)``. For invoices and estimates the URL token is the raw ``public_token``; for CSAT it is the signed token whose SHA-256 is already the stored ``csat_survey.token_hash``, so a single hash resolves - all three. The row leaks only the *existence* of a token to somebody who - already holds it; the resource contents stay behind RLS, and a disabled or - deleted resource still 404s because the view's own scoped query re-checks - ``public_link_enabled`` / existence after the context is set. + all three. The inbound email webhook uses it the same way: SNS delivers to + ``/api/cases/inbound//`` anonymously, and the key there is the + SHA-256 of the mailbox id in its canonical UUID form. The row leaks only + the *existence* of a token to somebody who already holds it; the resource + contents stay behind RLS, and a disabled or deleted resource still 404s + because the view's own scoped query re-checks ``public_link_enabled`` / + ``is_active`` / existence after the context is set. See ``docs/PORTAL_RLS.md``. """ @@ -1207,10 +1211,12 @@ class PortalAccessToken(models.Model): INVOICE = "invoice" ESTIMATE = "estimate" CSAT = "csat" + INBOUND_MAILBOX = "inbound_mailbox" RESOURCE_CHOICES = ( (INVOICE, "Invoice"), (ESTIMATE, "Estimate"), (CSAT, "CSAT survey"), + (INBOUND_MAILBOX, "Inbound mailbox"), ) id = models.UUIDField(default=uuid.uuid4, primary_key=True) diff --git a/backend/common/org_time.py b/backend/common/org_time.py index 393f9259..99d395b5 100644 --- a/backend/common/org_time.py +++ b/backend/common/org_time.py @@ -12,6 +12,7 @@ re-audited. Adding a per-user timezone later is a change to """ import logging +from contextlib import contextmanager from datetime import datetime, timedelta from datetime import timezone as dt_timezone from zoneinfo import ZoneInfo, ZoneInfoNotFoundError, available_timezones @@ -54,6 +55,21 @@ def activate_org_timezone(org): timezone.activate(ZoneInfo(settings.TIME_ZONE)) +@contextmanager +def org_timezone(org): + """``activate_org_timezone`` for one block, then put back what was active. + + For a Celery task that does one piece of day math: a worker runs no + middleware to deactivate the zone afterwards, and its threads are reused, + so a zone activated and left behind would hand this org's day to the next + task. ``timezone.override(None)`` records the zone active on entry and + restores it on exit, raised or not. + """ + with timezone.override(None): + activate_org_timezone(org) + yield + + # Region-prefixed legacy aliases. Every one of these has a modern equivalent in # the list (`US/Eastern` is `America/New_York`), so offering both doubles the # list without adding a single choice. `Etc/*` goes too: `Etc/GMT+5` counts its diff --git a/backend/common/public_tokens.py b/backend/common/public_tokens.py index 0a9fd707..dc87999f 100644 --- a/backend/common/public_tokens.py +++ b/backend/common/public_tokens.py @@ -8,8 +8,13 @@ a Sentry event's request block, transaction, breadcrumbs and spans) would hand that credential on. Sentry's event scrubber matches keys, not values inside a URL, so it cannot catch this. -Imported by ``crm/server_settings.py`` and by ``LOGGING`` while settings load, -so it must not need Django. +The app server's access log (uvicorn's ``uvicorn.access``, gunicorn's +``gunicorn.access``) records every path too; ``crm/settings.py`` attaches +``RedactAccessLog`` to both. A reverse proxy's access log is outside the +process and needs its own redaction (see the self-hosting security docs). + +Imported by ``crm/server_settings.py`` and by ``crm/settings.py`` while +settings load, so it must not need Django. """ import logging @@ -66,3 +71,22 @@ class RedactPublicTokens(logging.Filter): record.msg = redacted record.args = None return True + + +class RedactAccessLog(logging.Filter): + """The same redaction for an app server's access log, keeping the args. + + Both access formatters read ``record.args`` rather than the message: + uvicorn's ``AccessFormatter`` unpacks it as a five-item tuple, and gunicorn + formats its ``access_log_format`` against a dict of atoms whose own type + answers ``-`` for a header the request lacked. So each argument is + redacted where it stands instead of being formatted in and dropped. + """ + + def filter(self, record): + if isinstance(record.args, dict): + for key, item in record.args.items(): + record.args[key] = redact(item) + else: + record.args = redact(record.args) + return True diff --git a/backend/common/serializer.py b/backend/common/serializer.py index 114c5707..5edb8b55 100644 --- a/backend/common/serializer.py +++ b/backend/common/serializer.py @@ -51,7 +51,7 @@ class OrgAwareRefreshToken(RefreshToken): - role: User's role in the org (ADMIN/USER) - is_organization_admin: ``is_org_admin(profile)``, the one admin fact both clients gate admin UI on (the role, or a superuser's profile) - - org_settings: Currency and locale settings + - org_settings: Currency, locale and timezone settings The clients read these claims for display and to hide controls only. The API never trusts them: every request re-resolves the profile from the @@ -95,6 +95,9 @@ class OrgAwareRefreshToken(RefreshToken): org.default_currency or "USD", "$" ), "default_country": org.default_country, + # The org's day, for a form's "today" default. Display + # only: the API dates what it owns itself. + "timezone": org.timezone or "UTC", } # Add role if profile provided (avoids /api/auth/profile call) diff --git a/backend/common/tests/test_auth.py b/backend/common/tests/test_auth.py index 9e3ee887..83f73255 100644 --- a/backend/common/tests/test_auth.py +++ b/backend/common/tests/test_auth.py @@ -63,6 +63,29 @@ class TestTokenRefreshView: assert "access" in response.data assert "refresh" in response.data + def test_org_settings_claim_carries_the_org_timezone( + self, unauthenticated_client, admin_user, org_a, admin_profile + ): + """The web's "today" defaults read the org's zone from this claim, so + a refresh re-derives it: a zone changed in settings reaches the next + token rather than waiting for a new sign-in.""" + from rest_framework_simplejwt.tokens import AccessToken + + org_a.timezone = "Asia/Kolkata" + org_a.save(update_fields=["timezone"]) + token = OrgAwareRefreshToken.for_user_and_org(admin_user, org_a, admin_profile) + claim = AccessToken(str(token.access_token))["org_settings"] + assert claim["timezone"] == "Asia/Kolkata" + + org_a.timezone = "America/New_York" + org_a.save(update_fields=["timezone"]) + response = unauthenticated_client.post( + self.url, {"refresh": str(token)}, format="json" + ) + assert response.status_code == status.HTTP_200_OK + claim = AccessToken(response.data["access"])["org_settings"] + assert claim["timezone"] == "America/New_York" + def test_refresh_invalid_token(self, unauthenticated_client): response = unauthenticated_client.post( self.url, diff --git a/backend/common/tests/test_credential_audit.py b/backend/common/tests/test_credential_audit.py new file mode 100644 index 00000000..fd890b05 --- /dev/null +++ b/backend/common/tests/test_credential_audit.py @@ -0,0 +1,277 @@ +"""Credential lifecycle events in the security audit log. + +A calendar feed URL and a personal access token are both standing credentials, +so turning one on, replacing it and taking it away each write one row an org +admin can read at ``/api/org/audit-log/``: + +* ``CALENDAR_FEED_ENABLED``, ``CALENDAR_FEED_REGENERATED``, + ``CALENDAR_FEED_DISABLED`` from ``/api/profile/calendar-feed/``; +* ``API_TOKEN_CREATED`` from ``/api/profile/tokens/`` and ``API_TOKEN_REVOKED`` + from ``/api/profile/tokens//`` (the owner) or ``/api/org/tokens//`` + (an admin), naming who acted and whose token it was. + +No row ever holds a raw token or its hash, a no-op (disabling a feed that is +off, revoking a revoked token) writes nothing, and a failed audit write never +fails the action it records. +""" + +from unittest import mock + +import pytest +from django.core.cache import cache + +from common.audit_log import SecurityAuditLog +from common.models import CalendarFeedToken, PersonalAccessToken + +FEED = "/api/profile/calendar-feed/" +TOKENS = "/api/profile/tokens/" +VIEWER = "/api/org/audit-log/" + + +@pytest.fixture(autouse=True) +def _fresh_cache(): + cache.clear() + yield + cache.clear() + + +def _rows(event_type): + return list(SecurityAuditLog.objects.filter(event_type=event_type)) + + +def _only(event_type): + rows = _rows(event_type) + assert len(rows) == 1, rows + return rows[0] + + +def _assert_no_secret(row, *secrets): + text = str(SecurityAuditLog.objects.filter(pk=row.pk).values().get()) + for secret in secrets: + assert secret not in text + + +def _feed_secrets(url): + raw = url.rsplit("/", 1)[1].removesuffix(".ics") + return raw, CalendarFeedToken.hash_token(raw) + + +class TestCalendarFeed: + def test_enable_writes_one_enabled_row( + self, user_client, user_profile, regular_user, org_a + ): + resp = user_client.post(FEED) + assert resp.status_code == 201 + row = _only("CALENDAR_FEED_ENABLED") + assert row.org_id == org_a.id + assert row.user_id == regular_user.id + assert row.success is True + assert row.ip_address == "127.0.0.1" + assert row.request_path == FEED + assert row.metadata == {} + _assert_no_secret(row, *_feed_secrets(resp.json()["url"])) + assert SecurityAuditLog.objects.count() == 1 + + def test_regenerate_writes_one_regenerated_row( + self, user_client, user_profile, regular_user, org_a + ): + first = user_client.post(FEED).json()["url"] + second = user_client.post(FEED).json()["url"] + row = _only("CALENDAR_FEED_REGENERATED") + assert (row.org_id, row.user_id) == (org_a.id, regular_user.id) + assert len(_rows("CALENDAR_FEED_ENABLED")) == 1 + _assert_no_secret(row, *_feed_secrets(first)) + _assert_no_secret(row, *_feed_secrets(second)) + + def test_disable_writes_one_disabled_row(self, user_client, regular_user, org_a): + user_client.post(FEED) + assert user_client.delete(FEED).status_code == 200 + row = _only("CALENDAR_FEED_DISABLED") + assert (row.org_id, row.user_id) == (org_a.id, regular_user.id) + + def test_disabling_a_feed_that_is_off_writes_nothing(self, user_client): + assert user_client.delete(FEED).status_code == 200 + assert SecurityAuditLog.objects.count() == 0 + + def test_a_failed_audit_write_does_not_fail_the_action( + self, user_client, user_profile + ): + with mock.patch.object( + SecurityAuditLog.objects, "create", side_effect=RuntimeError("db down") + ): + assert user_client.post(FEED).status_code == 201 + assert user_client.post(FEED).status_code == 201 + assert CalendarFeedToken.objects.filter(profile=user_profile).exists() + assert user_client.delete(FEED).status_code == 200 + assert not CalendarFeedToken.objects.filter(profile=user_profile).exists() + assert SecurityAuditLog.objects.count() == 0 + + +class TestOwnApiTokens: + def test_create_writes_one_row_naming_the_token( + self, user_client, regular_user, org_a + ): + resp = user_client.post( + TOKENS, {"name": "CI script", "scopes": ["*:read"]}, format="json" + ) + assert resp.status_code == 201, resp.content + raw = resp.json()["token"] + pat = PersonalAccessToken.objects.get() + row = _only("API_TOKEN_CREATED") + assert (row.org_id, row.user_id) == (org_a.id, regular_user.id) + assert row.ip_address == "127.0.0.1" + assert row.metadata == { + "token_id": str(pat.id), + "token_prefix": pat.token_prefix, + "token_name": "CI script", + "scopes": ["*:read"], + "owner_id": str(regular_user.id), + "owner_name": regular_user.name or regular_user.email, + } + _assert_no_secret(row, raw, pat.token_hash) + + def test_a_refused_create_writes_nothing(self, user_client): + assert user_client.post(TOKENS, {"name": " "}, format="json").status_code == 400 + assert SecurityAuditLog.objects.count() == 0 + + def test_revoke_writes_one_row_and_a_second_revoke_writes_none( + self, user_client, user_profile, regular_user, org_a + ): + raw, pat = PersonalAccessToken.generate(profile=user_profile, name="cli") + assert user_client.delete(f"{TOKENS}{pat.id}/").status_code == 200 + assert user_client.delete(f"{TOKENS}{pat.id}/").status_code == 200 + row = _only("API_TOKEN_REVOKED") + assert (row.org_id, row.user_id) == (org_a.id, regular_user.id) + assert row.metadata["owner_id"] == str(regular_user.id) + assert row.metadata["token_id"] == str(pat.id) + _assert_no_secret(row, raw, pat.token_hash) + pat.refresh_from_db() + assert pat.revoked_at is not None + + def test_another_members_token_is_404_and_writes_nothing( + self, user_client, admin_profile + ): + _, pat = PersonalAccessToken.generate(profile=admin_profile, name="theirs") + assert user_client.delete(f"{TOKENS}{pat.id}/").status_code == 404 + assert SecurityAuditLog.objects.count() == 0 + pat.refresh_from_db() + assert pat.revoked_at is None + + def test_a_failed_audit_write_does_not_fail_create_or_revoke(self, user_client): + with mock.patch.object( + SecurityAuditLog.objects, "create", side_effect=RuntimeError("db down") + ): + resp = user_client.post(TOKENS, {"name": "cli"}, format="json") + assert resp.status_code == 201 + pat = PersonalAccessToken.objects.get() + assert user_client.delete(f"{TOKENS}{pat.id}/").status_code == 200 + pat.refresh_from_db() + assert pat.revoked_at is not None + assert SecurityAuditLog.objects.count() == 0 + + +class TestAdminRevoke: + def test_records_the_admin_as_actor_and_the_member_as_owner( + self, admin_client, admin_user, user_profile, regular_user, org_a + ): + raw, pat = PersonalAccessToken.generate(profile=user_profile, name="laptop") + assert admin_client.delete(f"/api/org/tokens/{pat.id}/").status_code == 200 + row = _only("API_TOKEN_REVOKED") + assert (row.org_id, row.user_id) == (org_a.id, admin_user.id) + assert row.metadata["owner_id"] == str(regular_user.id) + assert row.metadata["token_name"] == "laptop" + _assert_no_secret(row, raw, pat.token_hash) + + assert admin_client.delete(f"/api/org/tokens/{pat.id}/").status_code == 200 + assert len(_rows("API_TOKEN_REVOKED")) == 1 + + def test_a_member_is_refused_and_nothing_is_written( + self, user_client, admin_profile + ): + _, pat = PersonalAccessToken.generate(profile=admin_profile, name="x") + assert user_client.delete(f"/api/org/tokens/{pat.id}/").status_code == 403 + assert SecurityAuditLog.objects.count() == 0 + + def test_another_orgs_token_is_404_and_writes_nothing( + self, admin_client, profile_b + ): + _, pat = PersonalAccessToken.generate(profile=profile_b, name="x") + assert admin_client.delete(f"/api/org/tokens/{pat.id}/").status_code == 404 + assert SecurityAuditLog.objects.count() == 0 + + +class TestTheViewer: + @pytest.fixture + def logged(self, user_client, user_profile): + """One of each new event, all written by a member of org A.""" + user_client.post(FEED) + user_client.post(FEED) + user_client.delete(FEED) + user_client.post(TOKENS, {"name": "CI"}, format="json") + pat = PersonalAccessToken.objects.get() + user_client.delete(f"{TOKENS}{pat.id}/") + return pat + + NEW = ( + "CALENDAR_FEED_ENABLED", + "CALENDAR_FEED_REGENERATED", + "CALENDAR_FEED_DISABLED", + "API_TOKEN_CREATED", + "API_TOKEN_REVOKED", + ) + + def test_an_admin_reads_them_and_the_filter_returns_each( + self, admin_client, logged + ): + body = admin_client.get(VIEWER).json() + assert {e["event_type"] for e in body["results"]} == set(self.NEW) + offered = {t["value"] for t in body["event_types"]} + assert offered >= set(self.NEW) + for event_type in self.NEW: + results = admin_client.get(f"{VIEWER}?event_type={event_type}").json()[ + "results" + ] + assert [e["event_type"] for e in results] == [event_type] + + def test_a_token_row_shows_its_details_and_a_feed_row_shows_none( + self, admin_client, regular_user, logged + ): + created = admin_client.get(f"{VIEWER}?event_type=API_TOKEN_CREATED").json()[ + "results" + ][0] + assert created["event_label"] == "API Token Created" + assert created["details"] == { + "token_id": str(logged.id), + "token_prefix": logged.token_prefix, + "token_name": "CI", + "scopes": [], + "owner_id": str(regular_user.id), + "owner_name": regular_user.name or regular_user.email, + } + assert logged.token_hash not in str(created) + feed = admin_client.get( + f"{VIEWER}?event_type=CALENDAR_FEED_REGENERATED" + ).json()["results"][0] + assert feed["event_label"] == "Calendar Feed Regenerated" + assert feed["details"] == {} + + def test_token_keys_are_not_let_through_on_any_other_event( + self, admin_client, org_a + ): + SecurityAuditLog.objects.create( + org=org_a, + event_type="SUSPICIOUS_ACTIVITY", + metadata={"token_name": "typed by a caller", "owner_name": "x"}, + ) + assert admin_client.get(VIEWER).json()["results"][0]["details"] == {} + + def test_a_member_cannot_read_them(self, user_client, logged): + assert user_client.get(VIEWER).status_code == 403 + + def test_another_orgs_admin_never_sees_them(self, org_b_client, logged): + body = org_b_client.get(VIEWER).json() + assert body["count"] == 0 + assert ( + org_b_client.get(f"{VIEWER}?event_type=API_TOKEN_CREATED").json()["count"] + == 0 + ) diff --git a/backend/common/tests/test_public_paths_org_day.py b/backend/common/tests/test_public_paths_org_day.py new file mode 100644 index 00000000..6fd70d59 --- /dev/null +++ b/backend/common/tests/test_public_paths_org_day.py @@ -0,0 +1,252 @@ +"""Anonymous endpoints and Celery tasks keep the org's day, not the server's. + +No one is signed in on a public link, a web form or an inbound-mail webhook, +so the middleware finds no org and the server's UTC day is active. Each of +these paths resolves its org from the token, form or mailbox and now activates +that org's timezone as well; a Celery task does the same around its day math. + +The clock is frozen at 23:30 UTC on 31 December, which is already 1 January in +Asia/Kolkata. Every test pairs the Kolkata org with a UTC org given the same +data, in that order, so a day computed in UTC fails the first half and a +Kolkata zone left active fails the second. +""" + +import datetime +from unittest.mock import patch + +import pytest +from django.core import mail +from django.utils import timezone +from rest_framework.test import APIClient + +from cases.inbound.parser import parse_raw_email +from cases.inbound.pipeline import ingest +from cases.models import Case +from cases.tests.test_inbound_email import SNS_TOPIC, _make_mailbox, _notification +from cases.tests.test_inbound_email import _raw_email as raw_email +from common.portal_auth import mint_portal_token +from conftest import rls_org +from contacts.models import Contact +from invoices.models import Estimate, Invoice +from invoices.tasks import send_payment_reminder +from webforms.models import WebForm, WebFormDailyStat + +pytestmark = pytest.mark.django_db + +NEW_YEARS_EVE_2330_UTC = datetime.datetime( + 2026, 12, 31, 23, 30, tzinfo=datetime.timezone.utc +) +NEW_YEARS_EVE = datetime.date(2026, 12, 31) +NEW_YEARS_DAY = datetime.date(2027, 1, 1) + + +@pytest.fixture +def frozen(): + """`localdate()` reads the module-level `now`, so this moves both.""" + with patch("django.utils.timezone.now", return_value=NEW_YEARS_EVE_2330_UTC): + yield + + +@pytest.fixture +def kolkata(org_a): + org_a.timezone = "Asia/Kolkata" + org_a.save(update_fields=["timezone"]) + return org_a + + +@pytest.fixture +def utc(org_b): + assert org_b.timezone == "UTC" + return org_b + + +def test_the_premise_the_two_days_differ(frozen): + assert timezone.now().date() == NEW_YEARS_EVE + with timezone.override("Asia/Kolkata"): + assert timezone.localdate() == NEW_YEARS_DAY + + +class TestPublicEstimateAccept: + """`is_expired` compares the expiry with the day; accepting past it is refused.""" + + def _estimate(self, org): + with rls_org(org): + return Estimate.objects.create( + title="Fit-out", + client_name="Dana", + client_email="dana@buyer.example", + status="Sent", + issue_date=datetime.date(2026, 12, 1), + expiry_date=NEW_YEARS_EVE, + org=org, + ) + + def _accept(self, estimate): + return APIClient().post( + f"/api/public/estimate/{estimate.public_token}/accept/", + {"name": "Dana", "email": "dana@buyer.example"}, + format="json", + ) + + def test_expired_in_kolkata_still_valid_in_utc(self, kolkata, utc, frozen): + in_kolkata = self._estimate(kolkata) + in_utc = self._estimate(utc) + + refused = self._accept(in_kolkata) + accepted = self._accept(in_utc) + + assert refused.status_code == 400 + assert "expired" in refused.json()["message"] + assert accepted.status_code == 200, accepted.content + with rls_org(utc): + in_utc.refresh_from_db() + assert in_utc.status == "Accepted" + assert timezone.get_current_timezone_name() == "UTC" + + +class TestWebFormDailyViews: + def _form(self, org): + with rls_org(org): + return WebForm.objects.create(name="Contact", org=org, is_published=True) + + def test_a_view_is_counted_on_the_org_day(self, kolkata, utc, frozen): + in_kolkata = self._form(kolkata) + in_utc = self._form(utc) + client = APIClient() + + first = client.get(f"/api/public/forms/{kolkata.id}/{in_kolkata.id}/embed/") + second = client.get(f"/api/public/forms/{utc.id}/{in_utc.id}/embed/") + + assert (first.status_code, second.status_code) == (200, 200) + with rls_org(kolkata): + assert WebFormDailyStat.objects.get(form=in_kolkata).date == NEW_YEARS_DAY + with rls_org(utc): + assert WebFormDailyStat.objects.get(form=in_utc).date == NEW_YEARS_EVE + + +class TestReopenWindowOnTheOrgDay: + """Closed on 24 December with the default 7-day window. + + On 31 December that is 7 days, inside the window; on 1 January it is 8, + outside it. So the same reply reopens the UTC org's ticket and not the + Kolkata org's. + """ + + CLOSED_ON = datetime.date(2026, 12, 24) + + def _closed_case(self, org, address): + mailbox = _make_mailbox(org, address=address) + with rls_org(org): + case = ingest( + parse_raw_email( + raw_email(message_id=f"", to=address) + ), + mailbox, + ).case + case.status = "Closed" + case.closed_on = self.CLOSED_ON + case.save() + return mailbox, case + + def _status(self, org, case): + with rls_org(org): + return Case.objects.get(pk=case.pk).status + + def test_an_inbound_reply(self, kolkata, utc, frozen, settings): + # `RequireOrgContext` does not exempt `/api/cases/inbound/`, so today an + # anonymous SNS post is refused 403 before the view runs (every other + # webhook test signs in, which hides that). Dropping it here runs the + # view the way it would run once exempt; `GetProfileAndOrg`, which + # deactivates the zone after the request, stays in. + settings.MIDDLEWARE = [ + m + for m in settings.MIDDLEWARE + if m != "common.middleware.rls_context.RequireOrgContext" + ] + kolkata_box, kolkata_case = self._closed_case(kolkata, "help@kolkata.example") + utc_box, utc_case = self._closed_case(utc, "help@utc.example") + client = APIClient() + + with patch("cases.inbound_views.verify_sns_message"): + for mailbox in (kolkata_box, utc_box): + reply = raw_email( + message_id=f"", + in_reply_to=f"", + to=mailbox.address, + ) + response = client.post( + f"/api/cases/inbound/{mailbox.id}/", + _notification(SNS_TOPIC, reply), + format="json", + ) + assert response.status_code == 200, response.content + + assert self._status(kolkata, kolkata_case) == "Closed" + assert self._status(utc, utc_case) == "Pending" + + def test_a_portal_reply(self, kolkata, utc, frozen): + """Portal requests carry a token, so the middleware sets the org day.""" + cases = {} + for org in (kolkata, utc): + with rls_org(org): + contact = Contact.objects.create( + org=org, first_name="Pat", email=f"pat@{org.id}.example" + ) + case = Case.objects.create( + org=org, + name="Login", + status="Closed", + priority="High", + closed_on=self.CLOSED_ON, + ) + case.contacts.add(contact) + cases[org] = (contact, case) + + for org in (kolkata, utc): + contact, case = cases[org] + client = APIClient() + client.credentials( + HTTP_AUTHORIZATION=f"Bearer {mint_portal_token(contact)}" + ) + response = client.post( + f"/api/portal/cases/{case.id}/comment/", + {"comment": "Still broken"}, + format="json", + ) + assert response.status_code == 201, response.content + + assert self._status(kolkata, cases[kolkata][1]) == "Closed" + assert self._status(utc, cases[utc][1]) == "Pending" + + +class TestPaymentReminderTask: + """A worker runs no middleware; the task sets the org day for its count.""" + + def _invoice(self, org, number): + with rls_org(org): + return Invoice.objects.create( + invoice_number=number, + invoice_title="Hosting", + client_name="Dana", + client_email="dana@buyer.example", + status="Sent", + issue_date=datetime.date(2026, 12, 1), + due_date=datetime.date(2026, 12, 30), + total_amount=100, + org=org, + ) + + def test_days_overdue_on_the_org_day_and_no_zone_left_behind( + self, kolkata, utc, frozen + ): + in_kolkata = self._invoice(kolkata, "INV-K-1") + in_utc = self._invoice(utc, "INV-U-1") + + send_payment_reminder(str(in_kolkata.id), str(kolkata.id)) + assert timezone.get_current_timezone_name() == "UTC" + send_payment_reminder(str(in_utc.id), str(utc.id)) + + kolkata_mail, utc_mail = mail.outbox + assert "2 days overdue" in kolkata_mail.body + assert "1 days overdue" in utc_mail.body + assert timezone.get_current_timezone_name() == "UTC" diff --git a/backend/common/tests/test_public_tokens.py b/backend/common/tests/test_public_tokens.py index dd9d4c9f..40de7b4f 100644 --- a/backend/common/tests/test_public_tokens.py +++ b/backend/common/tests/test_public_tokens.py @@ -3,15 +3,19 @@ The calendar feed, a satisfaction survey, and an invoice's or estimate's portal link each carry their credential as a path segment under `/api/public/`. Django's `django.request` logger writes `Not Found: ` and -`Too Many Requests: `, and a Sentry event records the path in several -places, so both are scrubbed by `common.public_tokens`. +`Too Many Requests: `, the app server's access log writes every path, +and a Sentry event records the path in several places, so all three are +scrubbed by `common.public_tokens`. """ +import io import logging import pytest from django.core.cache import cache +from gunicorn.glogging import SafeAtoms from rest_framework.test import APIClient +from uvicorn.logging import AccessFormatter from common.models import CalendarFeedToken from common.public_tokens import redact, scrub_public_tokens @@ -124,3 +128,85 @@ class TestApplicationLog: caplog.set_level(logging.WARNING, logger="django.request") APIClient().get("/api/public/help/no-such-org/") assert "Not Found: /api/public/help/no-such-org/" in self._messages(caplog) + + +class TestAppServerAccessLog: + """The access loggers are the app server's, set up before Django loads. + + Each test logs exactly as the server does and formats with the server's + own formatter, so it also proves the filter left the arguments in the + shape that formatter reads. The filter itself comes from settings. + """ + + def _emit(self, logger_name, formatter, msg, *args): + logger = logging.getLogger(logger_name) + stream = io.StringIO() + handler = logging.StreamHandler(stream) + handler.setFormatter(formatter) + level = logger.level + logger.setLevel(logging.INFO) # both servers configure INFO + logger.addHandler(handler) + try: + logger.info(msg, *args) + finally: + logger.removeHandler(handler) + logger.setLevel(level) + return stream.getvalue() + + def _uvicorn(self, path): + # httptools_impl.py / h11_impl.py, uvicorn 0.54 + return self._emit( + "uvicorn.access", + AccessFormatter( + '%(levelprefix)s %(client_addr)s - "%(request_line)s" %(status_code)s', + use_colors=False, + ), + '%s - "%s %s HTTP/%s" %d', + "203.0.113.9:5123", + "GET", + path, + "1.1", + 200, + ) + + def _gunicorn(self, path): + # gunicorn.glogging.Logger.access, with the default access_log_format + # plus a header this request did not send. + atoms = SafeAtoms( + { + "h": "203.0.113.9", + "r": f"GET {path} HTTP/1.1", + "U": path.split("?")[0], + "s": "200", + "f": "-", + } + ) + return self._emit( + "gunicorn.access", + logging.Formatter("%(message)s"), + '%(h)s "%(r)s" %(s)s "%(f)s" %(U)s %({x-missing}i)s', + atoms, + ) + + @pytest.mark.parametrize("path", TOKEN_PATHS) + def test_uvicorn_names_no_token(self, path): + line = self._uvicorn(f"{path}?x=1") + prefix = path[: path.index(TOKEN)] + assert f'"GET {prefix}[Filtered]' in line + assert "?x=1 HTTP/1.1" in line and "200 OK" in line + assert TOKEN not in line + + @pytest.mark.parametrize("path", TOKEN_PATHS) + def test_gunicorn_names_no_token(self, path): + line = self._gunicorn(path) + prefix = path[: path.index(TOKEN)] + assert f'"GET {prefix}[Filtered]' in line + assert f'" {prefix}[Filtered]' in line # the %(U)s atom + # SafeAtoms kept its type, so a missing header still reads "-". + assert line.endswith(" -\n") + assert TOKEN not in line + + def test_other_paths_are_logged_as_they_were(self): + path = "/api/public/help/acme/?page=2" + assert f'"GET {path} HTTP/1.1" 200 OK' in self._uvicorn(path) + assert f'"GET {path} HTTP/1.1" 200' in self._gunicorn(path) diff --git a/backend/common/tests/test_timezone_dates.py b/backend/common/tests/test_timezone_dates.py index 84ad0b4a..75842ee3 100644 --- a/backend/common/tests/test_timezone_dates.py +++ b/backend/common/tests/test_timezone_dates.py @@ -221,3 +221,25 @@ class TestOverdueUsesTheOrgDay: ) assert invoice.is_overdue is False + + +@pytest.mark.django_db +class TestJoiningDate: + """An invited member's `date_of_joining` is the org's day. + + It was set from `timezone.now()`, which a `DateField` stores as the UTC + day, so a member invited in the Kolkata morning joined "yesterday". + """ + + def test_invited_member_joins_on_the_org_day(self, admin_client, org_a, frozen): + from common.models import Profile + + org_a.timezone = "Asia/Kolkata" + org_a.save(update_fields=["timezone"]) + response = admin_client.post( + "/api/users/", {"email": "joiner@test.com", "role": "USER"}, format="json" + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + joined = Profile.objects.get(org=org_a, user__email="joiner@test.com") + assert joined.date_of_joining == datetime.date(2026, 8, 7) diff --git a/backend/common/views/audit_log_views.py b/backend/common/views/audit_log_views.py index 44b22483..cc784569 100644 --- a/backend/common/views/audit_log_views.py +++ b/backend/common/views/audit_log_views.py @@ -23,7 +23,8 @@ What leaves the server is chosen field by field rather than dumped: user switched from, which is another tenant. * `metadata` is cut down to `SAFE_DETAILS`, keys whose values are ids, counts or sentences the server wrote, plus `EVENT_DETAILS` for the one event that - wrote them (a merge's two record names). Other keys can hold what a caller supplied: + wrote them (a merge's two record names, an API token's name, prefix, scopes + and owner). Other keys can hold what a caller supplied: `suspicious_activity(details=...)`, the email a failed login tried, an API key prefix. * A path under `/api/public/` can carry a bearer token in the URL (the CSAT @@ -58,10 +59,22 @@ SAFE_DETAILS = ( # Keys allowed for one event only. A merge row names both records, so it keeps # their display names (the merged one is deleted, and this row is the only -# place its name survives). Names are text people typed, so they are shown for -# the event that wrote them and never let through on any other row. +# place its name survives). An API token row names the token and whose it was, +# by the display prefix the token list already shows (never the token or its +# hash). Names are text people typed, so they are shown for the event that +# wrote them and never let through on any other row. +_API_TOKEN_DETAILS = ( + "token_id", + "token_prefix", + "token_name", + "scopes", + "owner_id", + "owner_name", +) EVENT_DETAILS = { "RECORD_MERGED": ("entity", "kept_id", "kept_name", "merged_id", "merged_name"), + "API_TOKEN_CREATED": _API_TOKEN_DETAILS, + "API_TOKEN_REVOKED": _API_TOKEN_DETAILS, } PUBLIC_PREFIX = "/api/public/" diff --git a/backend/common/views/calendar_feed_views.py b/backend/common/views/calendar_feed_views.py index dd88e59f..f3e49c30 100644 --- a/backend/common/views/calendar_feed_views.py +++ b/backend/common/views/calendar_feed_views.py @@ -7,7 +7,9 @@ managing their own feed. It is on the credential deny-list in ``common/scopes.py``, so no personal access token and not the org API key can read or mint a feed URL: the URL is itself a standing credential, and minting one from a token would outlive revoking that token. Every query filters on -``org=request.profile.org`` AND ``profile=request.profile``. +``org=request.profile.org`` AND ``profile=request.profile``. Enabling, +regenerating and disabling each write one ``CALENDAR_FEED_*`` row to the org's +security audit log. ``PublicCalendarFeedView`` (``/api/public/calendar/.ics``) is what a calendar app polls. It takes no credential except the token in the path. @@ -24,6 +26,7 @@ member could not open in the app today. from datetime import timedelta +from django.db import transaction from django.db.models import Q from django.http import HttpResponse from django.utils import timezone @@ -36,9 +39,10 @@ from rest_framework.response import Response from rest_framework.throttling import SimpleRateThrottle from rest_framework.views import APIView +from common.audit_log import audit_log from common.calendar_feed import FEED_PATH_PREFIX, render_calendar from common.links import api_url, frontend_url -from common.models import CalendarFeedToken +from common.models import CalendarFeedToken, Profile from common.org_time import activate_org_timezone from common.permissions import HasOrgContext from common.request_meta import client_ip @@ -96,7 +100,19 @@ class CalendarFeedView(APIView): ), ) def post(self, request): - raw, feed = CalendarFeedToken.issue(request.profile) + with transaction.atomic(): + # The lock `issue` takes, taken first, so that of two racing + # requests the second sees the first's row and is audited as the + # regenerate it is. + Profile.objects.select_for_update().filter(pk=request.profile.pk).first() + replaced = self._mine(request).exists() + raw, feed = CalendarFeedToken.issue(request.profile) + audit_log.calendar_feed( + "CALENDAR_FEED_REGENERATED" if replaced else "CALENDAR_FEED_ENABLED", + request.user, + request.profile.org, + request, + ) data = _state(feed) # Not from the request: the web app asks from its own server, whose # view of this host is internal (see `common.links.api_url`). @@ -105,7 +121,12 @@ class CalendarFeedView(APIView): @extend_schema(tags=["Calendar feed"], operation_id="calendar_feed_disable") def delete(self, request): - self._mine(request).delete() + deleted, _ = self._mine(request).delete() + # Disabling a feed that is already off changes nothing and is not logged. + if deleted: + audit_log.calendar_feed( + "CALENDAR_FEED_DISABLED", request.user, request.profile.org, request + ) return Response(_state(None)) diff --git a/backend/common/views/pat_views.py b/backend/common/views/pat_views.py index 9567d752..13dce04b 100644 --- a/backend/common/views/pat_views.py +++ b/backend/common/views/pat_views.py @@ -8,6 +8,8 @@ A user manages ONLY their own tokens. Both list and revoke are filtered by ``org=request.profile.org`` AND ``profile=request.profile`` so another user's token id 404s rather than leaking or being revoked (IDOR guard). The raw token is returned exactly once, on create; ``token_hash`` is never serialized. +Creating a token and revoking one (here or by an admin below) each write one +``API_TOKEN_*`` row to the org's security audit log. The ``Org*`` views below are the admin's *oversight* half. Org-wide read and revoke, gated to admins, and are deliberately kept SEPARATE from the @@ -26,6 +28,7 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView +from common.audit_log import audit_log from common.models import PersonalAccessToken from common.permissions import HasOrgContext, IsOrgAdmin from common.serializer import ( @@ -34,6 +37,17 @@ from common.serializer import ( ) +def _revoke(request, pat): + """Revoke ``pat`` and audit it, once. Revoking an already-revoked token is + a no-op and writes no row. The conditional update means two revokes racing + on one token still write one row between them.""" + revoked = PersonalAccessToken.objects.filter( + pk=pat.pk, revoked_at__isnull=True + ).update(revoked_at=timezone.now()) + if revoked: + audit_log.api_token_revoked(request.user, pat, request) + + class PersonalAccessTokenListCreateView(APIView): permission_classes = (IsAuthenticated, HasOrgContext) @@ -68,6 +82,7 @@ class PersonalAccessTokenListCreateView(APIView): scopes=ser.validated_data.get("scopes", []), expires_at=ser.validated_data.get("expires_at"), ) + audit_log.api_token_created(request.user, pat, request) data = PersonalAccessTokenListSerializer(pat).data data["token"] = raw # shown ONCE, never retrievable again return Response({"error": False, **data}, status=status.HTTP_201_CREATED) @@ -79,14 +94,12 @@ class PersonalAccessTokenDetailView(APIView): @extend_schema(tags=["API Tokens"], operation_id="pat_revoke") def delete(self, request, pk): pat = get_object_or_404( - PersonalAccessToken, + PersonalAccessToken.objects.select_related("profile__user"), pk=pk, org=request.profile.org, profile=request.profile, ) - if pat.revoked_at is None: - pat.revoked_at = timezone.now() - pat.save(update_fields=["revoked_at"]) + _revoke(request, pat) return Response({"error": False}, status=status.HTTP_200_OK) @@ -179,8 +192,10 @@ class OrgAccessTokenDetailView(APIView): @extend_schema(tags=["API Tokens"], operation_id="org_pat_revoke") def delete(self, request, pk): - pat = get_object_or_404(PersonalAccessToken, pk=pk, org=request.profile.org) - if pat.revoked_at is None: - pat.revoked_at = timezone.now() - pat.save(update_fields=["revoked_at"]) + pat = get_object_or_404( + PersonalAccessToken.objects.select_related("profile__user"), + pk=pk, + org=request.profile.org, + ) + _revoke(request, pat) return Response({"error": False}, status=status.HTTP_200_OK) diff --git a/backend/common/views/user_views.py b/backend/common/views/user_views.py index 54123a85..97e6f61a 100644 --- a/backend/common/views/user_views.py +++ b/backend/common/views/user_views.py @@ -146,7 +146,7 @@ class UsersListView(APIView, LimitOffsetPagination): Profile.objects.create( user=user, - date_of_joining=timezone.now(), + date_of_joining=timezone.localdate(), role=profile_serializer.validated_data["role"], address=address_obj, org=request.profile.org, diff --git a/backend/crm/settings.py b/backend/crm/settings.py index fd97e9b1..39c33915 100644 --- a/backend/crm/settings.py +++ b/backend/crm/settings.py @@ -1,3 +1,4 @@ +import logging import os import warnings from datetime import timedelta @@ -6,6 +7,8 @@ from urllib.parse import urlparse from corsheaders.defaults import default_headers from dotenv import load_dotenv +from common.public_tokens import RedactAccessLog + # Build paths inside the project like this: os.path.join(BASE_DIR, ...) BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) @@ -353,6 +356,14 @@ LOGGING = { }, } +# The app server's access log names every path, public-link tokens included. +# uvicorn and gunicorn set up their access loggers before Django loads, and +# naming one in LOGGING above would strip the server's own handler from it, so +# the filter goes onto the existing logger instead. Django's dictConfig leaves +# it there (disable_existing_loggers is False). +for _access_logger in ("uvicorn.access", "gunicorn.access"): + logging.getLogger(_access_logger).addFilter(RedactAccessLog()) + APPLICATION_NAME = "bottlecrm" SETTINGS_EXPORT = ["APPLICATION_NAME"] @@ -454,6 +465,31 @@ if RELAY_SECRET and len(RELAY_SECRET) < 32: "or leave it unset to turn relayed client addresses off." ) +# AWS accounts whose SNS topics may pin an inbound mailbox that has no Topic ARN +# yet (`cases.inbound_views.InboundMailboxWebhookView`). SNS lets any AWS +# account subscribe any HTTPS endpoint to its own topic, so a mailbox pins a +# confirmation's topic only when it belongs to one of these; with none listed, +# an admin has to enter each mailbox's Topic ARN. Comma-separated 12-digit ids. +# A malformed id is refused rather than dropped, since dropping it would leave +# an operator believing their account is allowed while every mailbox stays +# unpinned. +INBOUND_SNS_ACCOUNT_IDS = frozenset( + part.strip() + for part in os.environ.get("INBOUND_SNS_ACCOUNT_IDS", "").split(",") + if part.strip() +) +_bad_account_ids = sorted( + i + for i in INBOUND_SNS_ACCOUNT_IDS + if not (len(i) == 12 and i.isascii() and i.isdigit()) +) +if _bad_account_ids: + raise ValueError( + f"INBOUND_SNS_ACCOUNT_IDS contains {', '.join(map(repr, _bad_account_ids))}, " + "which is not a 12-digit AWS account id. List account ids separated by " + "commas, for example 123456789012,210987654321, or leave it unset." + ) + SPECTACULAR_SETTINGS = { "TITLE": "BottleCRM API", "DESCRIPTION": "Open source CRM application", diff --git a/backend/docs/PORTAL_RLS.md b/backend/docs/PORTAL_RLS.md index a861e29d..a79abe6e 100644 --- a/backend/docs/PORTAL_RLS.md +++ b/backend/docs/PORTAL_RLS.md @@ -87,6 +87,20 @@ Do not reach for `BYPASSRLS` here. That role exists for the superadmin dashboard and every addition to its surface is a step toward the escalation path the threat model is built around. +## The inbound email webhook uses the same lookup + +`POST /api/cases/inbound//` had both defects at once until 1.13.0: +`RequireOrgContext` answered 403 to every SNS delivery, and past that, the view +read `inbound_mailbox` before it set any context and answered 404. The route is +now exempt by URL name (`RequireOrgContext.EXEMPT_VIEW_NAMES`, deliberately not +a prefix), and the view resolves the org from `portal_access_token` (resource +type `inbound_mailbox`, key `sha256` of the canonical mailbox id) before it sets +the context and reads the mailbox. Rows are registered on mailbox create and +removed on delete (`cases/signals.py`); `common/0049` backfilled existing +mailboxes org by org. Tests: `cases/tests/test_inbound_webhook_reachability.py`, +including `postgres_only` cases that assert the empty context hides the mailbox +before proving the delivery lands. + ## What must not be done - Do not make the app's DB user a superuser to "fix" the portal. That disables diff --git a/backend/invoices/migrations/0013_org_day_date_defaults.py b/backend/invoices/migrations/0013_org_day_date_defaults.py new file mode 100644 index 00000000..94ccc916 --- /dev/null +++ b/backend/invoices/migrations/0013_org_day_date_defaults.py @@ -0,0 +1,42 @@ +# Generated by Django 6.1.1 on 2026-09-27 01:16 + +import django.utils.timezone +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("invoices", "0012_tax_rate_and_shipping_validators"), + ] + + operations = [ + migrations.AlterField( + model_name="estimate", + name="issue_date", + field=models.DateField( + default=django.utils.timezone.localdate, verbose_name="Issue Date" + ), + ), + migrations.AlterField( + model_name="invoice", + name="issue_date", + field=models.DateField( + default=django.utils.timezone.localdate, verbose_name="Issue Date" + ), + ), + migrations.AlterField( + model_name="recurringinvoice", + name="next_generation_date", + field=models.DateField( + default=django.utils.timezone.localdate, + verbose_name="Next Generation Date", + ), + ), + migrations.AlterField( + model_name="recurringinvoice", + name="start_date", + field=models.DateField( + default=django.utils.timezone.localdate, verbose_name="Start Date" + ), + ), + ] diff --git a/backend/invoices/models.py b/backend/invoices/models.py index c6e579f3..146f1d2a 100644 --- a/backend/invoices/models.py +++ b/backend/invoices/models.py @@ -1,4 +1,3 @@ -import datetime import secrets from decimal import ROUND_HALF_UP, Decimal @@ -415,7 +414,7 @@ class Invoice(AssignableMixin, BaseModel): ) # Dates & Terms - issue_date = models.DateField(_("Issue Date"), default=datetime.date.today) + issue_date = models.DateField(_("Issue Date"), default=timezone.localdate) due_date = models.DateField(_("Due Date"), blank=True, null=True) payment_terms = models.CharField( _("Payment Terms"), max_length=20, choices=PAYMENT_TERMS, default="NET_30" @@ -543,7 +542,7 @@ class Invoice(AssignableMixin, BaseModel): def generate_invoice_number(self): """Next number in this org's sequence: INV-YYYYMMDD-XXXX""" - prefix = f"INV-{datetime.datetime.now():%Y%m%d}-" + prefix = f"INV-{timezone.localdate():%Y%m%d}-" return _next_number(Invoice, "invoice_number", prefix, self.org_id) def calculate_due_date(self): @@ -904,7 +903,7 @@ class Estimate(AssignableMixin, BaseModel): ) # Dates - issue_date = models.DateField(_("Issue Date"), default=datetime.date.today) + issue_date = models.DateField(_("Issue Date"), default=timezone.localdate) expiry_date = models.DateField(_("Expiry Date"), null=True, blank=True) # Status Timestamps @@ -1008,7 +1007,7 @@ class Estimate(AssignableMixin, BaseModel): def generate_estimate_number(self): """Next number in this org's sequence: EST-YYYYMMDD-XXXX""" - prefix = f"EST-{datetime.datetime.now():%Y%m%d}-" + prefix = f"EST-{timezone.localdate():%Y%m%d}-" return _next_number(Estimate, "estimate_number", prefix, self.org_id) def recalculate_totals(self): @@ -1167,10 +1166,10 @@ class RecurringInvoice(AssignableMixin, BaseModel): custom_days = models.PositiveIntegerField( _("Custom Days Interval"), null=True, blank=True ) - start_date = models.DateField(_("Start Date"), default=datetime.date.today) + start_date = models.DateField(_("Start Date"), default=timezone.localdate) end_date = models.DateField(_("End Date"), null=True, blank=True) next_generation_date = models.DateField( - _("Next Generation Date"), default=datetime.date.today + _("Next Generation Date"), default=timezone.localdate ) # Settings diff --git a/backend/invoices/public_views.py b/backend/invoices/public_views.py index 8ce6c4b3..f8fe3199 100644 --- a/backend/invoices/public_views.py +++ b/backend/invoices/public_views.py @@ -16,6 +16,8 @@ from rest_framework import status from rest_framework.response import Response from rest_framework.views import APIView +from common.models import Org +from common.org_time import activate_org_timezone from common.portal_tokens import resolve_portal_org from common.request_meta import client_ip from common.tasks import set_rls_context @@ -42,10 +44,17 @@ def _resolve_org_context(token, resource_type): An unknown token leaves the context empty; the scoped query then returns nothing and the caller 404s. The same answer a disabled link gives, so a stranger learns nothing about whether a token is real. + + The org's timezone is activated too. No user is signed in, so the + middleware found no org and left the server's UTC day active, and every + day check below (an estimate's expiry, the PDF's "(Expired)" and year) + would have run on it. `GetProfileAndOrg` deactivates it when the request + ends, so it cannot reach the next request on this worker. """ org_id = resolve_portal_org(token, resource_type) if org_id: set_rls_context(org_id) + activate_org_timezone(Org.objects.filter(id=org_id).first()) return org_id diff --git a/backend/invoices/serializer.py b/backend/invoices/serializer.py index c4ec1d04..7abce5f3 100644 --- a/backend/invoices/serializer.py +++ b/backend/invoices/serializer.py @@ -1,6 +1,7 @@ import re from decimal import Decimal +from django.utils import timezone from rest_framework import serializers from accounts.access import visible_accounts_qs @@ -1563,6 +1564,31 @@ class RecurringInvoiceCreateSerializer(serializers.ModelSerializer): } ) + # Billing never starts before the schedule does. The generator reads + # only `next_generation_date`, so an omitted one used to take the model + # default (today) and bill a schedule starting next month today. On + # create an omitted date now follows `start_date` (itself the org's + # today when omitted). A date before the start is refused on create + # and on any update that sends either date; an update that sends + # neither is not checked, so an older row can still be edited. + if self.instance is None: + attrs.setdefault("start_date", timezone.localdate()) + attrs.setdefault("next_generation_date", attrs["start_date"]) + if {"start_date", "next_generation_date"} & attrs.keys(): + start = attrs.get("start_date", getattr(self.instance, "start_date", None)) + first_run = attrs.get( + "next_generation_date", + getattr(self.instance, "next_generation_date", None), + ) + if first_run < start: + raise serializers.ValidationError( + { + "next_generation_date": ( + "The next generation date cannot be before the start date." + ) + } + ) + # Lines are optional on a schedule, but not one that mails each # invoice it raises: that would send the client a blank 0.00 invoice. # Checked only when the request touches either side, so an older row diff --git a/backend/invoices/tasks.py b/backend/invoices/tasks.py index 75879141..843db024 100644 --- a/backend/invoices/tasks.py +++ b/backend/invoices/tasks.py @@ -20,7 +20,7 @@ from django.utils import timezone from common.links import frontend_url from common.models import Org, Profile -from common.org_time import activate_org_timezone +from common.org_time import activate_org_timezone, org_timezone from common.tasks import clear_rls_context, set_rls_context logger = logging.getLogger(__name__) @@ -50,9 +50,6 @@ def send_email(invoice_id, recipients, org_id, domain="localhost", protocol="htt logger.warning("Invoice %s not found", invoice_id) return - # "N days overdue" in the email is counted on the customer's calendar. - activate_org_timezone(invoice.org) - for user_id in recipients: profile = Profile.objects.filter(id=user_id, is_active=True).first() if profile and profile.user and profile.user.email: @@ -142,7 +139,9 @@ def send_invoice_to_client( # Attach PDF if requested if include_pdf: try: - pdf_content = generate_invoice_pdf(invoice) + # The PDF's copyright year is the org's, not the UTC one. + with org_timezone(invoice.org): + pdf_content = generate_invoice_pdf(invoice) filename = generate_invoice_filename(invoice) msg.attach(filename, pdf_content, "application/pdf") except Exception as e: @@ -456,13 +455,17 @@ def send_payment_reminder(invoice_id, org_id, domain="localhost", protocol="http if invoice.public_link_enabled and invoice.public_token: public_url = frontend_url(f"/portal/invoice/{invoice.public_token}") + # Days overdue on the org's calendar: the task runs no middleware, so the + # worker's day is UTC's. + days_overdue = 0 + if invoice.due_date: + with org_timezone(invoice.org): + days_overdue = (timezone.localdate() - invoice.due_date).days context = { "invoice": invoice, "public_url": public_url, "org": invoice.org, - "days_overdue": (timezone.localdate() - invoice.due_date).days - if invoice.due_date - else 0, + "days_overdue": days_overdue, } html_content = render_to_string( "invoices/emails/payment_reminder.html", context=context @@ -683,7 +686,9 @@ def send_estimate_to_client( if include_pdf: try: - pdf_content = generate_estimate_pdf(estimate) + # "(Expired)" on the PDF is judged on the org's day. + with org_timezone(estimate.org): + pdf_content = generate_estimate_pdf(estimate) filename = generate_estimate_filename(estimate) msg.attach(filename, pdf_content, "application/pdf") except Exception as e: diff --git a/backend/invoices/tests/test_document_numbers.py b/backend/invoices/tests/test_document_numbers.py index ae546f73..24f54bff 100644 --- a/backend/invoices/tests/test_document_numbers.py +++ b/backend/invoices/tests/test_document_numbers.py @@ -8,12 +8,12 @@ Uniqueness is now `(org, number)`, which is what the generator can actually guarantee. """ -import datetime import threading import time import pytest from django.db import IntegrityError, connection, transaction +from django.utils import timezone from common.tasks import set_rls_context from conftest import rls_org @@ -21,7 +21,7 @@ from invoices.models import Estimate, Invoice def _today(prefix): - return f"{prefix}-{datetime.datetime.now():%Y%m%d}-" + return f"{prefix}-{timezone.localdate():%Y%m%d}-" def _invoice(org, **fields): diff --git a/backend/invoices/tests/test_org_day_date_defaults.py b/backend/invoices/tests/test_org_day_date_defaults.py new file mode 100644 index 00000000..30ccdd58 --- /dev/null +++ b/backend/invoices/tests/test_org_day_date_defaults.py @@ -0,0 +1,171 @@ +"""A document that sends no date is dated in the org's day, not the server's. + +`issue_date` on invoices and estimates, and `start_date` and +`next_generation_date` on a recurring schedule, defaulted to +`datetime.date.today`, which reads the server clock and ignores the org's +timezone. The invoice and estimate numbers (`INV-YYYYMMDD-`) read the server's +naive local clock too. Both now go through `timezone.localdate()`, which is the +org's day inside a request (`GetProfileAndOrg` activates it) and inside the +recurring generator (which activates each org's zone itself). + +The clock is frozen across a year boundary: 23:30 UTC on 31 December is +already 1 January in Asia/Kolkata, and 02:00 UTC on 1 January is still +31 December in America/New_York. A UTC or server-local date fails one of each +pair. Patching `django.utils.timezone.now` moves `localdate()` but not +`datetime.date.today`, so the old defaults cannot pass these by accident. +""" + +import datetime +from unittest.mock import patch + +import pytest + +from accounts.models import Account +from contacts.models import Contact +from invoices.models import Estimate, Invoice, RecurringInvoice +from invoices.tasks import generate_recurring_invoices + +pytestmark = pytest.mark.django_db + +KOLKATA_AHEAD = datetime.datetime(2026, 12, 31, 23, 30, tzinfo=datetime.timezone.utc) +NEW_YORK_BEHIND = datetime.datetime(2027, 1, 1, 2, 0, tzinfo=datetime.timezone.utc) +NEW_YEARS_DAY = datetime.date(2027, 1, 1) +NEW_YEARS_EVE = datetime.date(2026, 12, 31) + +CASES = [ + pytest.param("Asia/Kolkata", KOLKATA_AHEAD, NEW_YEARS_DAY, id="ahead-of-utc"), + pytest.param("America/New_York", NEW_YORK_BEHIND, NEW_YEARS_EVE, id="behind-utc"), +] + + +def _frozen(instant): + return patch("django.utils.timezone.now", return_value=instant) + + +def _in_zone(org, name): + org.timezone = name + org.save(update_fields=["timezone"]) + + +@pytest.fixture +def account(org_a): + return Account.objects.create(name="Acme", org=org_a) + + +@pytest.fixture +def contact(org_a): + return Contact.objects.create(first_name="Ada", last_name="Buyer", org=org_a) + + +@pytest.fixture(autouse=True) +def no_history(): + with patch("invoices.api_views.create_invoice_history.delay"): + yield + + +@pytest.mark.parametrize("zone,instant,day", CASES) +def test_invoice_without_an_issue_date( + admin_client, org_a, account, contact, zone, instant, day +): + _in_zone(org_a, zone) + with _frozen(instant): + response = admin_client.post( + "/api/invoices/", + { + "invoice_title": "Undated", + "account_id": str(account.id), + "contact_id": str(contact.id), + "currency": "USD", + }, + format="json", + ) + assert response.status_code == 201, response.content + invoice = Invoice.objects.get(invoice_title="Undated") + assert invoice.issue_date == day + assert invoice.invoice_number == f"INV-{day:%Y%m%d}-0001" + + +@pytest.mark.parametrize("zone,instant,day", CASES) +def test_estimate_without_an_issue_date( + admin_client, org_a, account, contact, zone, instant, day +): + _in_zone(org_a, zone) + with _frozen(instant): + response = admin_client.post( + "/api/invoices/estimates/", + { + "title": "Undated", + "account_id": str(account.id), + "contact_id": str(contact.id), + "currency": "USD", + }, + format="json", + ) + assert response.status_code == 201, response.content + estimate = Estimate.objects.get(title="Undated") + assert estimate.issue_date == day + assert estimate.expiry_date is None + assert estimate.estimate_number == f"EST-{day:%Y%m%d}-0001" + + +@pytest.mark.parametrize("zone,instant,day", CASES) +def test_recurring_schedule_without_dates( + admin_client, org_a, account, contact, zone, instant, day +): + _in_zone(org_a, zone) + with _frozen(instant): + response = admin_client.post( + "/api/invoices/recurring/", + { + "title": "Undated", + "account_id": str(account.id), + "contact_id": str(contact.id), + "frequency": "MONTHLY", + }, + format="json", + ) + assert response.status_code == 201, response.content + recurring = RecurringInvoice.objects.get(title="Undated") + assert (recurring.start_date, recurring.next_generation_date) == (day, day) + + +def test_a_date_the_caller_sends_wins(admin_client, org_a, account, contact): + _in_zone(org_a, "Asia/Kolkata") + with _frozen(KOLKATA_AHEAD): + response = admin_client.post( + "/api/invoices/", + { + "invoice_title": "Dated", + "account_id": str(account.id), + "contact_id": str(contact.id), + "currency": "USD", + "issue_date": "2026-12-15", + }, + format="json", + ) + assert response.status_code == 201, response.content + assert Invoice.objects.get(invoice_title="Dated").issue_date == datetime.date( + 2026, 12, 15 + ) + + +@pytest.mark.parametrize("zone,instant,day", CASES) +def test_recurring_generator_dates_and_numbers_in_the_org_day( + org_a, account, contact, zone, instant, day +): + """The Celery task runs no middleware; it activates each org's zone.""" + _in_zone(org_a, zone) + RecurringInvoice.objects.create( + title="Monthly", + account=account, + contact=contact, + frequency="MONTHLY", + start_date=day, + next_generation_date=day, + org=org_a, + ) + with _frozen(instant): + generate_recurring_invoices() + invoice = Invoice.objects.get(org=org_a) + assert invoice.issue_date == day + assert invoice.invoice_number == f"INV-{day:%Y%m%d}-0001" diff --git a/backend/invoices/tests/test_recurring_first_run_not_before_start.py b/backend/invoices/tests/test_recurring_first_run_not_before_start.py new file mode 100644 index 00000000..60956ab0 --- /dev/null +++ b/backend/invoices/tests/test_recurring_first_run_not_before_start.py @@ -0,0 +1,165 @@ +"""A recurring schedule never bills before its start date. + +The generator reads only `next_generation_date`. A create that sent a future +`start_date` and no `next_generation_date` took the model default (today), so +a schedule meant to start next month billed its first invoice today. An +omitted date now follows `start_date`, and a date before the start is refused +on create and on update. +""" + +import datetime + +import pytest +from django.utils import timezone + +from accounts.models import Account +from contacts.models import Contact +from invoices.models import RecurringInvoice + +pytestmark = pytest.mark.django_db + +URL = "/api/invoices/recurring/" + + +@pytest.fixture +def account(org_a): + return Account.objects.create(name="Acme", org=org_a) + + +@pytest.fixture +def contact(org_a): + return Contact.objects.create(first_name="Ada", last_name="Buyer", org=org_a) + + +@pytest.fixture +def body(account, contact): + return { + "title": "Hosting", + "account_id": str(account.id), + "contact_id": str(contact.id), + "frequency": "MONTHLY", + } + + +@pytest.fixture +def schedule(account, contact, org_a): + today = timezone.localdate() + return RecurringInvoice.objects.create( + title="Hosting", + account=account, + contact=contact, + frequency="MONTHLY", + start_date=today, + next_generation_date=today + datetime.timedelta(days=10), + org=org_a, + ) + + +def _future(days): + return timezone.localdate() + datetime.timedelta(days=days) + + +def test_an_omitted_first_run_follows_a_future_start(admin_client, body): + start = _future(30) + response = admin_client.post(URL, {**body, "start_date": str(start)}, format="json") + + assert response.status_code == 201, response.content + recurring = RecurringInvoice.objects.get(title="Hosting") + assert recurring.start_date == start + assert recurring.next_generation_date == start + + +def test_a_first_run_before_the_start_is_refused_on_create(admin_client, body): + response = admin_client.post( + URL, + { + **body, + "start_date": str(_future(30)), + "next_generation_date": str(_future(29)), + }, + format="json", + ) + + assert response.status_code == 400 + assert "next_generation_date" in response.json()["errors"] + assert not RecurringInvoice.objects.filter(title="Hosting").exists() + + +def test_a_first_run_after_the_start_is_accepted(admin_client, body): + response = admin_client.post( + URL, + { + **body, + "start_date": str(_future(1)), + "next_generation_date": str(_future(15)), + }, + format="json", + ) + + assert response.status_code == 201, response.content + recurring = RecurringInvoice.objects.get(title="Hosting") + assert recurring.next_generation_date == _future(15) + + +def test_a_first_run_before_an_omitted_start_is_refused(admin_client, body): + """An omitted start is the org's today, so yesterday is before it.""" + response = admin_client.post( + URL, {**body, "next_generation_date": str(_future(-1))}, format="json" + ) + + assert response.status_code == 400 + assert "next_generation_date" in response.json()["errors"] + + +def test_moving_the_start_past_the_next_run_is_refused(admin_client, schedule): + response = admin_client.put( + f"{URL}{schedule.id}/", {"start_date": str(_future(11))}, format="json" + ) + + assert response.status_code == 400 + assert "next_generation_date" in response.json()["errors"] + schedule.refresh_from_db() + assert schedule.start_date == timezone.localdate() + + +def test_moving_the_next_run_before_the_start_is_refused(admin_client, schedule): + response = admin_client.put( + f"{URL}{schedule.id}/", + {"next_generation_date": str(_future(-1))}, + format="json", + ) + + assert response.status_code == 400 + assert "next_generation_date" in response.json()["errors"] + schedule.refresh_from_db() + assert schedule.next_generation_date == _future(10) + + +def test_moving_either_date_within_the_rule_is_accepted(admin_client, schedule): + response = admin_client.put( + f"{URL}{schedule.id}/", + {"start_date": str(_future(5)), "next_generation_date": str(_future(5))}, + format="json", + ) + + assert response.status_code == 200, response.content + schedule.refresh_from_db() + assert (schedule.start_date, schedule.next_generation_date) == ( + _future(5), + _future(5), + ) + + +def test_an_older_row_can_still_be_edited_without_touching_its_dates( + admin_client, schedule +): + """A row saved before the rule may already break it; renaming it is fine.""" + RecurringInvoice.objects.filter(pk=schedule.pk).update(start_date=_future(20)) + + response = admin_client.put( + f"{URL}{schedule.id}/", {"title": "Renamed"}, format="json" + ) + + assert response.status_code == 200, response.content + schedule.refresh_from_db() + assert schedule.title == "Renamed" diff --git a/backend/webforms/public_views.py b/backend/webforms/public_views.py index e2055945..4eef13e0 100644 --- a/backend/webforms/public_views.py +++ b/backend/webforms/public_views.py @@ -33,6 +33,7 @@ from rest_framework.response import Response from rest_framework.views import APIView from common.links import api_url +from common.org_time import activate_org_timezone from common.request_meta import client_ip, referer from common.tasks import set_rls_context from webforms import captcha @@ -58,11 +59,21 @@ class PublicWebFormMixin: Sets the RLS context first. Every caller answers 404 for None: missing, unpublished, and belonging to another org are deliberately indistinguishable, so the id space cannot be used to enumerate forms. + + A found form's org timezone is activated, so the daily view counter and + anything the submission writes use the org's day rather than the UTC + one an anonymous request starts with. `GetProfileAndOrg` deactivates it + when the request ends. """ set_rls_context(org_id) - return WebForm.objects.filter( - id=form_id, org_id=org_id, is_published=True - ).first() + form = ( + WebForm.objects.select_related("org") + .filter(id=form_id, org_id=org_id, is_published=True) + .first() + ) + if form is not None: + activate_org_timezone(form.org) + return form def origin_allowed(self, request, form): """Whether this request's origin may use this form. diff --git a/docs/api/audit-log.md b/docs/api/audit-log.md index a95ae38e..e9b03edc 100644 --- a/docs/api/audit-log.md +++ b/docs/api/audit-log.md @@ -1,7 +1,8 @@ # Security audit log `GET /api/org/audit-log/` lets an org's admins read its security audit log: sign-ins, org switches, -refused requests, record merges, paused webhooks and the like. The view is +refused requests, record merges, paused webhooks, API token and calendar feed changes and the like. +The view is `SecurityAuditLogListView` (`backend/common/views/audit_log_views.py`), routed in `common/urls.py`; the events are written by `AuditLogger` in `common/audit_log.py`. See [Conventions](conventions.md#pagination) for `limit` and `offset`, and [Errors](errors.md) for the @@ -93,9 +94,44 @@ What each row returns is chosen field by field rather than copied from the table (`lead`, `contact` or `account`), `kept_id`, `kept_name`, `merged_id` and `merged_name`. The merged record is deleted, so this is the only place its name survives. Those keys are allowed on that event only; the same key on any other row is left out. +- **An API token row also names the token** (`EVENT_DETAILS`): `API_TOKEN_CREATED` and + `API_TOKEN_REVOKED` rows add `token_id`, `token_prefix`, `token_name`, `scopes`, `owner_id` and + `owner_name`. See [Credential changes](#credential-changes). Allowed on those two events only. - **Paths under `/api/public/` are cut to that prefix.** Some public links carry a token in the URL (a satisfaction-survey link, for one), and the log should not repeat it. +## Credential changes + +From django-crm 1.13.0 the two credentials a member can mint for themselves are audited, because +each one keeps working until someone takes it away. Every row is written against the acting org, so +the org's admins see it here, with the caller's client IP, user agent, method and path like any +other row. + +| `event_type` | Label | Written by | `actor` | +| --- | --- | --- | --- | +| `CALENDAR_FEED_ENABLED` | Calendar Feed Enabled | `POST /api/profile/calendar-feed/` when the member had no feed | the member | +| `CALENDAR_FEED_REGENERATED` | Calendar Feed Regenerated | `POST /api/profile/calendar-feed/` when it replaced a feed (the old URL stops working) | the member | +| `CALENDAR_FEED_DISABLED` | Calendar Feed Disabled | `DELETE /api/profile/calendar-feed/` when a feed existed | the member | +| `API_TOKEN_CREATED` | API Token Created | `POST /api/profile/tokens/` | the owner | +| `API_TOKEN_REVOKED` | API Token Revoked | `DELETE /api/profile/tokens//` (the owner) or `DELETE /api/org/tokens//` (an admin) | whoever revoked it | + +A calendar feed row carries no `details`. An API token row carries: + +| Key | Value | +| --- | --- | +| `token_id` | The token's id | +| `token_prefix` | The display prefix the token list already shows (`bcrm_pat_` and four characters) | +| `token_name` | The name its owner gave it | +| `scopes` | Its scopes; an empty list is a token with its owner's full access | +| `owner_id`, `owner_name` | The user whose token it is, so an admin's revoke of someone else's token names both people | + +No row holds the feed URL, a raw token or a token's hash. A no-op writes nothing: disabling a feed +that is already off, or revoking a token that is already revoked (on either endpoint), still answers +`200` but adds no row, so the log holds one revoke per token. A refused call (a `400` create, a `403` +member on `/api/org/tokens/`, a `404` token that is not yours or not in your org) writes nothing +either. If the audit row cannot be written the action still succeeds; the failure goes to the +`security.audit` logger, as for every other event. + ## Failed sign-ins From django-crm 1.13.0 a refused staff sign-in writes a `LOGIN_FAILURE` row (`AuditLogger.login_failure`, diff --git a/docs/api/cases.md b/docs/api/cases.md index 60a8ec4e..c3d1148b 100644 --- a/docs/api/cases.md +++ b/docs/api/cases.md @@ -163,6 +163,14 @@ already-closed case can still be edited. Two things happen on that move: and an org stored under a legacy zone name such as `US/Eastern` is dated like any other. Leaving `"Closed"` clears `closed_on` (and `resolved_at`), so a reopened case that is closed again is dated afresh rather than keeping the old date. +- **A Closed case always has a date.** From django-crm 1.13.0 this holds for every write whose + resulting status is `"Closed"`, not only the move into it. An absent or `null` `closed_on` on a case + that is already Closed keeps the stored date (so `PATCH {"closed_on": null}` is a no-op there rather + than leaving it undated), and only when there is no stored date is it dated today in the org's + timezone. The same rule runs on `PUT`, `PATCH`, bulk update, macro apply, the board move and + close-with-children, and the `Case` pre-save signal applies it to writers that skip the API + serializer (CSV import rows with `status` `Closed` and no `closed_on`, vertical-pack sample + tickets). - **The approval gate runs.** When an active `pre_close` `ApprovalRule` matches the case's priority/case_type/team (the incoming values, so a request cannot re-target the case out of the rule and close it at once), an `Approval` row in state `approved` for that case and rule is @@ -437,7 +445,7 @@ there: see [Retrieve, update, delete](#retrieve-update-delete). | `status` | one of `STATUS_CHOICE` | **required** (POST/PUT) | `New`, `Assigned`, `Pending`, `Closed`, `Rejected`, `Duplicate` | | `priority` | one of `PRIORITY_CHOICE` | **required** (POST/PUT) | `Low`, `Normal`, `High`, `Urgent` | | `case_type` | one of `CASE_TYPE` | optional | `Question`, `Incident`, `Problem` | -| `closed_on` | date | optional | A close that sends none is dated today in the org's timezone; a sent date wins. See [Retrieve, update, delete](#retrieve-update-delete) | +| `closed_on` | date | optional | A Closed case always has one: a sent date wins, an absent or `null` one keeps the stored date, else today in the org's timezone. See [Retrieve, update, delete](#retrieve-update-delete) | | `description` | text | optional | | | `is_active` | boolean | optional | Defaults `true`; soft-delete flag, hidden from `GET /api/cases/` by default | | `account` | uuid | optional on create; **write-once** | Must belong to the caller's org; silently `read_only` on `PUT`/`PATCH`. See [Retrieve, update, delete](#retrieve-update-delete) | diff --git a/docs/api/customer-portal.md b/docs/api/customer-portal.md index aec01337..1b9b3de9 100644 --- a/docs/api/customer-portal.md +++ b/docs/api/customer-portal.md @@ -202,7 +202,9 @@ private thread, and sending `"is_internal": true` in the body has no effect. On the agent side the reply arrives as an ordinary comment with `commented_by` null and `commented_by_contact` naming the customer, which is the same shape an inbound email reply produces (see [Cases](cases.md)). It counts as a customer reply everywhere that distinction matters: -first-response SLA stamping, and auto-reopen on a closed ticket. +first-response SLA stamping, and auto-reopen on a closed ticket. The reopen window is counted in +days on the org's calendar (`Org.timezone`): the portal token names the org, and the middleware +activates its timezone for the request, as it does for a signed-in user. ## Help articles diff --git a/docs/api/invoices.md b/docs/api/invoices.md index f9ddd57f..10ae1135 100644 --- a/docs/api/invoices.md +++ b/docs/api/invoices.md @@ -253,6 +253,13 @@ concrete instance of it for invoices and estimates. Every endpoint below additio `public_link_enabled=True` on the record, independent of the token resolving at all. An admin can revoke a previously-shared link without rotating the token. +**Dates on these endpoints are the org's, not the server's.** No one is signed in, so nothing else +would set a timezone and every "today" would be the server's UTC day. Once the token resolves, +`_resolve_org_context` also activates the org's timezone (`Org.timezone`) for the rest of the request. +So whether an estimate has expired (the accept check below, and the "(Expired)" marker on the estimate +PDF) is judged on the org's calendar day: at 23:30 UTC on 31 December an estimate expiring that day is +already expired for an org in `Asia/Kolkata` and still acceptable for an org in `UTC`. + - `GET /api/public/invoice/{token}/` (`PublicInvoiceView.get`, `:61-163`). Returns the invoice's client-facing fields, billing address, line items and payment history. First view stamps `viewed_at` and flips `Sent` → `Viewed`. @@ -293,7 +300,7 @@ and `PUT /api/invoices/{id}/` accept. | `discount_type` | one of `PERCENTAGE`, `FIXED` | optional | | | `discount_value`, `tax_rate`, `shipping_amount` | decimal | optional | Default `0` | | `currency` | one of `CURRENCY_CODES` | optional | Defaults `USD` | -| `issue_date` | date | optional | Defaults today | +| `issue_date` | date | optional | Defaults to today in the org's timezone (`Org.timezone`), not the server's UTC day | | `due_date` | date | optional | | | `payment_terms` | one of `PAYMENT_TERMS` | optional | Defaults `NET_30` | | `reminder_enabled`, `reminder_days_before`, `reminder_days_after`, `reminder_frequency` |, | optional | | @@ -304,14 +311,17 @@ and `PUT /api/invoices/{id}/` accept. | `status` | one of `INVOICE_STATUS` | **read-only** | Always created `Draft`; changed only via `send`/`mark-paid`/`cancel` | `invoice_number` is not part of the serializer at all, `Invoice.save()` generates and assigns it -server-side the first time the row is saved (`invoices/models.py:384-411`). +server-side the first time the row is saved (`invoices/models.py:384-411`), as `INV-YYYYMMDD-NNNN` +with the date taken from the org's day. `EstimateCreateSerializer.Meta.fields` (`invoices/serializer.py:904-928`) mirrors the invoice shape closely: `title` (**required**), `account_id`/`contact_id` (**required**, org-validated), `opportunity_id` (optional, org-validated), the same client/address/discount/tax/currency/notes/terms -fields, `expiry_date` (optional. This is what the public accept endpoint checks), `public_link_enabled`, +fields, `issue_date` (optional, defaults to today in the org's timezone), `expiry_date` (optional, no +default. This is what the public accept endpoint checks), `public_link_enabled`, and `line_items`. **`status` is writable here**. See the caveat in [Estimates](#estimates). Like -invoices, `estimate_number` is server-generated on first save, not part of the serializer. +invoices, `estimate_number` is server-generated on first save (`EST-YYYYMMDD-NNNN`, the org's day), +not part of the serializer. `GET` responses on both (`InvoiceSerializer`/`EstimateSerializer`, `invoices/serializer.py:444-539, 873-889`) additionally return, but never accept as input: `id`, `subtotal`, `discount_amount`, @@ -320,3 +330,21 @@ invoices, `estimate_number` is server-generated on first save, not part of the s `sent_at`/`viewed_at`/`accepted_at`/`declined_at`/`accepted_by_*` (estimate), `public_token`, `public_url`, `created_by`, `created_at`, `updated_at`, and, on the estimate only, `converted_to_invoice` (see [Estimates](#estimates)). + +### Recurring schedule dates + +`POST /api/invoices/recurring/` and `PUT /api/invoices/recurring/{id}/` (`RecurringInvoiceCreateSerializer`) +take `start_date` and `next_generation_date`. The daily generator bills a schedule when +`next_generation_date` is on or before today in the org's timezone and never reads `start_date`, so +the two are tied together on the server: + +- On create, an omitted `start_date` is today in the org's timezone, and an omitted + `next_generation_date` is the `start_date` (sent or defaulted). A schedule created to start next month + with no `next_generation_date` first bills next month. +- A `next_generation_date` earlier than `start_date` is refused with `400` and + `{"errors": {"next_generation_date": ["The next generation date cannot be before the start date."]}}`, + on create and on any update that sends either date (the other is read from the stored row). An update + that sends neither date is not checked, so a schedule saved before this rule can still be renamed or + paused. +- A `next_generation_date` later than `start_date` is allowed (a schedule that starts now and first + bills at the end of the month). diff --git a/docs/integrations/inbound-email.md b/docs/integrations/inbound-email.md index 73e1aa6a..774dbc38 100644 --- a/docs/integrations/inbound-email.md +++ b/docs/integrations/inbound-email.md @@ -39,10 +39,14 @@ steps for every message that reaches a configured mailbox: `default_priority`/`default_case_type`, and if the mailbox has a `default_assignee`, that profile is added to the new case's `assigned_to`. A match reuses the existing case, attaches the contact if not already linked (which, after the rule above, only a header match can do), and can reopen a closed case (subject to the org's reopen policy) - if the reply arrives inside the configured reopen window. + if the reply arrives inside the configured reopen window. The window counts calendar days since + `closed_on` on the org's day (`Org.timezone`), which the webhook activates once it has found the + mailbox's org; the request is anonymous, so otherwise it would be the server's UTC day. -Every message, including dropped ones, is recorded as exactly one `EmailMessage` row -(`org` + `message_id` is the idempotency key, so a provider retry never creates a duplicate case). +Every message that reaches the pipeline, including dropped ones, is recorded as exactly one +`EmailMessage` row (`org` + `message_id` is the idempotency key, so a provider retry never creates +a duplicate case). Mail addressed to a different mailbox never reaches it: see +[Which mailbox a notification belongs to](#which-mailbox-a-notification-belongs-to). That gives admins a forensic trail even for the mail that never became a ticket, and it's what [the mailbox list's per-mailbox counts](#configuring-a-mailbox) are computed from. @@ -102,7 +106,8 @@ Fields worth knowing when creating one: | `provider` | Must be `ses` for the mailbox to accept mail (see above). | | `webhook_secret` | Optional, and unused today. Nothing is generated for you, and the API never returns it. See [Securing the endpoint](#securing-the-endpoint). | | `has_webhook_secret` | Read-only boolean, admin-only, telling you whether a secret is stored without disclosing it. | -| `topic_arn` | Leave blank on create. It's pinned automatically from the first verified SNS `SubscriptionConfirmation`, not set by hand. See below. | +| `topic_arn` | The SNS Topic ARN this mailbox accepts mail from, in the form `arn:aws:sns::<12-digit account id>:`. Admin-only, to read and to write: an admin can set it on create, change it, or clear it with `""`. Anything that is not a well-formed topic ARN in the commercial `aws` partition (no `aws-cn`, no `aws-us-gov`, no `.fifo` topic) is a `400`. Left blank, it can be pinned by a confirmation from an allowed AWS account. See [Securing the endpoint](#securing-the-endpoint). | +| `has_topic_arn` | Read-only boolean, visible to every member: whether the mailbox has a Topic ARN yet. Without one it refuses every message. | | `default_priority`, `default_case_type` | Set directly on any new case this mailbox creates; both optional. There is no `default_status`. A new case's `status` is always `"New"`, not configurable per mailbox (see [above](#how-inbound-email-becomes-a-ticket)). | | `default_assignee_id` | Optional. If set, that profile is added to the new case's `assigned_to` (a many-to-many "add", not a status or ownership field of its own). | | `is_active` | An inactive mailbox's webhook returns `404` for every notification, same as a mailbox id that doesn't exist. Set this to disable a mailbox without deleting its history. | @@ -119,8 +124,7 @@ address still received mail even if the pipeline discarded it. Neither field is ## The SNS webhook -Point an AWS SES receipt rule (via an SNS topic, "SNS Notification with full content" or the -older JSON-envelope form, the parser handles both) at: +Point an AWS SES receipt rule's SNS action at a topic, and subscribe the topic to: ``` POST /api/cases/inbound// @@ -128,11 +132,42 @@ POST /api/cases/inbound// This route is deliberately public: `authentication_classes = ()`, `permission_classes = (AllowAny,)`, because SNS has no way to send your BottleCRM credentials. The `` in the -URL is what scopes each notification to one org: the view looks the mailbox up by that id (and -`is_active=True`) before doing anything else, and because the webhook bypasses -`RLSContextMiddleware` entirely (there's no authenticated session to derive an org from), the view -sets the RLS session variable by hand from `mailbox.org_id` before touching any other org-scoped -table. +URL is what scopes each notification to one org, in this order: + +1. **The route is reachable without an org claim.** `RequireOrgContext` + (`backend/common/middleware/rls_context.py`) refuses every request that has no org with `403 + "Organization context is required"`. This one route is exempt by its URL name + (`EXEMPT_VIEW_NAMES`), not by a path prefix, so the mailbox admin endpoints beside it still + require an org like everything else. +2. **The org is resolved before any org-scoped read.** `inbound_mailbox` is protected by RLS, and + an anonymous request starts with an empty `app.current_org`, under which a non-superuser + database role sees no mailbox at all. So the view first looks the org up in the unscoped + `portal_access_token` table, keyed on the SHA-256 of the mailbox id in canonical UUID form + (resource type `inbound_mailbox`), the same mechanism the invoice, estimate and CSAT links use + (`backend/docs/PORTAL_RLS.md`). A row is registered when a mailbox is created and removed when + it is deleted; migration `common/0049` registered every mailbox that already existed. +3. **The context is set, then the mailbox is read** within that org, with `is_active=True`. Only + then do the provider check, the signature check, the topic pin and the recipient check below + run. + +An id with no lookup row, an inactive mailbox, and a deleted mailbox all answer the same `404` +`{"error": true, "errors": "Mailbox not found"}`. Deactivating keeps the lookup row, so +reactivating needs nothing else. An id that is not a UUID at all never reaches the view: the +router answers a plain `404` for it, as it does on every id route. Any UUID spelling the router +accepts (upper case, no hyphens, braces) resolves to the same mailbox. + +**Before django-crm 1.13.0 this endpoint never delivered on a correctly configured production +database**, for both reasons above: the middleware answered `403` to every SNS request, and past +that, the mailbox lookup ran under an empty context and answered `404`. A superuser database role +(the usual local setup) bypasses RLS and hid the second, and every test signed in as an admin, +which hid the first. Upgrading changes no URL, and `manage.py migrate` runs the backfill. The +`SubscriptionConfirmation` was refused too, though, so a subscription created while the endpoint +was failing is still pending in AWS and its mailbox is unpinned (`has_topic_arn` is `false`). +Once the upgrade is live, either enter the mailbox's Topic ARN in the app, or set +[`INBOUND_SNS_ACCOUNT_IDS`](../reference/environment-variables.md#inbound-email) to the AWS +account that owns the topic. Then request confirmation again for that subscription in the SNS +console, or delete and recreate it if AWS has already expired it. A confirmation from any other +account pins nothing. On a `Notification`, the view acks with: @@ -145,6 +180,42 @@ above; the response is still `200` even when the message was dropped, and delibe already accepted the message from the original sender, and returning a `4xx` here would just trigger pointless provider retries of a message that was correctly classified as noise, not lost. +### Which mailbox a notification belongs to + +The SNS `Message` is read in one of two shapes: + +- **SES's JSON notification** (`"notificationType": "Received"`), which is what the SNS receipt + action publishes. The raw email is in `content`. When the action's **Encoding** is Base64 the + content is decoded first; UTF-8 content is used as it is. Both encodings work. +- **A bare raw RFC 5322 email**, for a publisher other than SES posting straight to the topic. + +A notification is ingested only when the mailbox's `address` is one of the message's recipients. +With the SES notification the recipients are `receipt.recipients`: the envelope `RCPT TO` +addresses the receipt rule matched. SES sets them, they include Bcc recipients, and the sender +cannot forge them, so the `To` and `Cc` headers are ignored in this shape. An SES notification +with no `receipt.recipients` matches nothing. With a bare raw email there is no envelope, so the +recipients are every `Delivered-To` and `X-Original-To` header plus `To` and `Cc`; those are +headers, written by whoever published the message, so this shape is only as trustworthy as the +publisher on the pinned topic. Matching ignores case, surrounding spaces and display names, and is +otherwise exact: `support+billing@acme.com` is not `support@acme.com`. + +A notification for any other address is acknowledged and discarded: + +```json +{"ok": true, "dropped": true, "reason": "not_addressed_to_mailbox"} +``` + +It is a `200`, so SNS does not retry, and nothing is written in the mailbox's org: no case, and +unlike a spam drop, no `EmailMessage` row either, because the mail belongs to another mailbox +and a row here would copy it into this org. The server logs a warning naming the mailbox id only. + +This is what makes it safe for **one SNS topic to serve several mailboxes**, in one org or many: +subscribe each mailbox's URL to the topic (a platform-wide SES receipt rule, say, with every +mailbox pinned to a topic in the account listed in `INBOUND_SNS_ACCOUNT_IDS`), and each mailbox +ingests only its own mail. Before django-crm 1.13.0 there was no recipient check, so every +mailbox subscribed to a shared topic turned every other mailbox's mail, from every org on it, +into its own tickets. + ## Securing the endpoint Because the endpoint takes no credential, trust is established entirely by verifying the message @@ -157,16 +228,34 @@ itself, in two layers, and it takes both: validity window before its public key is trusted. This proves the payload was genuinely signed by *some* SNS topic in *some* AWS account, nothing more. 2. **TopicArn pinning.** A valid SNS signature alone isn't enough, because anyone with an AWS - account can create a topic and have SNS sign messages for it. Each mailbox is pinned to the - exact `TopicArn` it should accept: the pin is captured automatically, trust-on-first-use, from - the first signature-verified `SubscriptionConfirmation` a mailbox receives, and every - `Notification` after that must carry the exact same `TopicArn` or it's rejected. A mailbox with - no pin yet rejects every `Notification` outright (only a `SubscriptionConfirmation` can - establish the pin), and once pinned, a mailbox does not silently re-pin to a different topic, - without this, anyone who learned a mailbox's UUID could point their own SNS topic at it and have - AWS legitimately sign forged mail on their behalf, which, because the pipeline threads replies - onto existing cases. Means injecting messages into a live customer conversation, not just - spam tickets. + account can create a topic, have SNS sign messages for it, and subscribe any HTTPS endpoint + to it, including this one. Each mailbox is therefore pinned to the exact `TopicArn` it should + accept, and every message, `SubscriptionConfirmation` or `Notification`, must carry that exact + `TopicArn` or it is refused. A mailbox gets its pin one of two ways: + - **An admin enters it** (`topic_arn` on the mailbox, from the web or mobile inbound email + settings or the API). From then on only that ARN is accepted, whatever + `INBOUND_SNS_ACCOUNT_IDS` says. + - **A confirmation from an allowed account.** While a mailbox has no pin, a + signature-verified `SubscriptionConfirmation` pins its `TopicArn` only when the topic's AWS + account id (the fifth `:`-separated field of the ARN) is listed in + [`INBOUND_SNS_ACCOUNT_IDS`](../reference/environment-variables.md#inbound-email). The + confirmation is then fetched from its `SubscribeURL` as usual. + + With neither, every message is refused: a confirmation from any other account, one with a + malformed or missing `TopicArn`, and every `Notification` to an unpinned mailbox. A refused + confirmation is never fetched, so the subscription stays pending in AWS. Once pinned, a + mailbox never re-pins itself; only an admin can change or clear the ARN. Before django-crm + 1.13.0 the first confirmation pinned whatever topic it came from, so anyone who read a + mailbox id (every org member can list them) could subscribe it to their own topic first and + then open tickets as any sender, including replies threaded into live customer conversations. + + Which to use: + - **Self-hosting:** set `INBOUND_SNS_ACCOUNT_IDS` to the AWS account that owns your SES + receipt-rule topics, then subscribe each mailbox URL and it pins itself. Or leave it unset + and paste each topic's ARN into the mailbox before subscribing. + - **Hosted BottleCRM:** the platform sets its own AWS account, so mailboxes wired up by the + platform pin themselves. An org that routes mail through a topic in its own AWS account + enters that Topic ARN on the mailbox instead. Both failure modes return the same generic `403` (`"Signature verification failed"`) rather than distinguishing "bad signature" from "wrong topic". A caller who gets past the existence check @@ -194,7 +283,7 @@ which are implemented. Two consequences follow, and both changed recently: could read every mailbox's stored secret out of a list response. Admins get `has_webhook_secret`, a boolean, instead. If you lose a stored key, overwrite it with a `PUT`. -`topic_arn` is still returned, to admins only, and is stripped for regular members along with -`has_webhook_secret`: it embeds your AWS account id. That is the reason to treat mailbox +`topic_arn` is returned to admins only, and is stripped for regular members along with +`has_webhook_secret`: it embeds your AWS account id. Members still get `has_topic_arn`. That is the reason to treat mailbox configuration as admin-only, rather than any forgery risk from `webhook_secret`, which cannot exist until a provider integration starts checking it. diff --git a/docs/integrations/web-forms.md b/docs/integrations/web-forms.md index ff964c87..4b90cb65 100644 --- a/docs/integrations/web-forms.md +++ b/docs/integrations/web-forms.md @@ -212,8 +212,10 @@ active `assign_to`. Rejected submissions notify nobody, because an org told abou and then misses the real one. `GET /api/webforms//analytics/` returns a fixed trailing 30 days of views, submissions, spam -and a conversion rate. Views are counted per form per day when an embed renders; submissions are -counted from the submission rows, so each number has exactly one source of truth. The series is +and a conversion rate. Views are counted per form per day when an embed renders, on the org's +calendar day (`Org.timezone`, which the anonymous embed activates once it has found the form), so the +series and its "today" agree with the analytics page; submissions are counted from the submission +rows, so each number has exactly one source of truth. The series is zero-filled, so a quiet day is a real zero rather than a gap. ## Multi-tenancy diff --git a/docs/reference/environment-variables.md b/docs/reference/environment-variables.md index aae32196..13e1723f 100644 --- a/docs/reference/environment-variables.md +++ b/docs/reference/environment-variables.md @@ -126,6 +126,12 @@ Pooling uses psycopg 3's `psycopg_pool` through Django's PostgreSQL backend. It | `DOMAIN_NAME` | `http://localhost:8000` | **Yes**, once self-hosted under a real domain (from django-crm 1.13.0) | The **backend's** public origin, for example `https://api.example.com`. From django-crm 1.13.0 it is the base of every URL the API hands out for use somewhere else (`common.links.api_url`): the task calendar feed URL a calendar app subscribes to, and a web form's embed snippets and the submit URL inside the embedded form. They used to be built from the incoming request, so the web app, which asks from its own server at `PUBLIC_DJANGO_API_URL` (`http://backend:8000` in Docker), showed a host nobody outside could reach, and `http://` unless `TRUST_PROXY_SSL_HEADER` was set. When `ENV_TYPE` is anything other than `dev`, `crm/settings.py` raises at import if this points at a loopback host (`localhost`, `127.0.0.1`, `0.0.0.0`, `::1`) or is not an absolute `http`/`https` URL, the same check `FRONTEND_URL` gets. A trailing slash is stripped. It no longer reaches any emailed link: those come from `FRONTEND_URL`, and the four `domain=` kwargs still passed to invoice Celery tasks (`invoices/tasks.py`, `invoices/api_views.py`) are ignored by those tasks. | | `FRONTEND_URL` | `http://localhost:5181` | **Yes**, once self-hosted under a real domain | Base URL of the SvelteKit frontend, and the single source for every link that leaves the system in an email: the welcome email and magic-link sign-in URL (`common/tasks.py`), the emailed invoice and estimate portal links, the CSAT survey link, and the "assigned to you" invoice link (all via `common.links.frontend_url`). The customer-facing ones go to people outside the organization, so leaving the `localhost` default in a real deployment emails dead links to customers. **That is now refused rather than emailed:** when `ENV_TYPE` is anything other than `dev`, `crm/settings.py` raises at import if this points at a loopback host (`localhost`, `127.0.0.1`, `0.0.0.0`, `::1`) or is not an absolute `http`/`https` URL, so the deploy fails to start instead of failing silently on somebody else's inbox. Plain HTTP against a real host is allowed, for a deployment behind its own TLS terminator. A trailing slash is stripped, so `https://app.example.com` and `https://app.example.com/` behave the same. | +### Inbound email + +| Variable | Default | Required | Purpose | +| --- | --- | --- | --- | +| `INBOUND_SNS_ACCOUNT_IDS` | *(empty, no account allowed)* | No. Without it, an admin enters each mailbox's SNS Topic ARN by hand | Comma-separated 12-digit AWS account ids, for example `123456789012,210987654321` (whitespace around each is ignored). New in django-crm 1.13.0. An [inbound mailbox](../integrations/inbound-email.md#securing-the-endpoint) that has no Topic ARN yet pins the topic of the first signature-verified SNS `SubscriptionConfirmation` it receives **only when that topic belongs to one of these accounts**; a confirmation from any other account is refused with `403` and pins nothing, and so is every notification until a pin exists. SNS lets any AWS account subscribe any HTTPS endpoint to its own topic, so before 1.13.0 whoever subscribed a mailbox's URL first owned it. Set it to the account that owns your SES receipt-rule topics (a hosted platform sets its own account). A mailbox whose admin entered a Topic ARN ignores this setting: only that exact ARN is accepted. `crm/settings.py` raises at import, in every environment, if any entry is not exactly 12 ASCII digits, so a typo fails the deploy instead of leaving every mailbox silently unpinned. | + ### Google OAuth | Variable | Default | Required | Purpose | diff --git a/docs/self-hosting/postgresql-and-rls.md b/docs/self-hosting/postgresql-and-rls.md index 3bd76be6..13594bb7 100644 --- a/docs/self-hosting/postgresql-and-rls.md +++ b/docs/self-hosting/postgresql-and-rls.md @@ -220,10 +220,12 @@ silent, the task reports success having done nothing at all. - **Assuming the anonymous client-portal endpoints (invoice/estimate links, CSAT surveys) are either broken or unprotected. They're a deliberate, narrow exception, and testing them locally can be misleading either way.** `RequireOrgContext.EXEMPT_PATHS` in - `backend/common/middleware/rls_context.py` exempts exactly `/api/public/invoice/`, - `/api/public/estimate/` and `/api/public/csat/` from requiring org context, because these are - genuinely anonymous requests (a customer clicking an emailed link) authorized by a possession - token rather than a JWT. See `backend/docs/PORTAL_RLS.md`. Exemption from *requiring* context + `backend/common/middleware/rls_context.py` exempts `/api/public/invoice/`, + `/api/public/estimate/` and `/api/public/csat/` (among the other anonymous prefixes listed there) + from requiring org context, because these are genuinely anonymous requests (a customer clicking + an emailed link) authorized by a possession token rather than a JWT. The inbound email webhook + (`/api/cases/inbound//`) is exempt the same way, by URL name in + `EXEMPT_VIEW_NAMES` rather than by prefix. See `backend/docs/PORTAL_RLS.md`. Exemption from *requiring* context is not the same as *setting* one: `_set_org_context` returns early when `request.org` is `None`, so these requests run with `app.current_org` empty, which, per the fail-safe design above, means the `invoice`/`estimate`/`csat_survey` rows the view is trying to read are invisible under diff --git a/docs/self-hosting/production-deploy.md b/docs/self-hosting/production-deploy.md index b85f044f..f211d0cb 100644 --- a/docs/self-hosting/production-deploy.md +++ b/docs/self-hosting/production-deploy.md @@ -137,7 +137,11 @@ HSTS configuration silently never takes effect. Point your proxy at whichever port your application server binds (`8000` in the examples above), and route `/` there. There's no repo-provided proxy config (nginx site file, Caddyfile, or -similar) to point to. The specifics are yours to write. +similar) to point to. The specifics are yours to write, with one requirement: the proxy's access +log records the calendar feed, invoice, estimate and survey links, whose path carries a working +credential. The application redacts its own logs but cannot reach the proxy's. See +[Public-link tokens in logs](security-hardening.md#public-link-tokens-in-logs) for an nginx +`log_format` that does it. The URLs the API hands out for use somewhere else are built from `DOMAIN_NAME`, not from the request that reached it: the web form embed snippets and the form's submit URL and, from django-crm 1.13.0, diff --git a/docs/self-hosting/security-hardening.md b/docs/self-hosting/security-hardening.md index ea80ebbc..95482d60 100644 --- a/docs/self-hosting/security-hardening.md +++ b/docs/self-hosting/security-hardening.md @@ -24,6 +24,9 @@ elsewhere in Self-hosting. This page is the checklist, with pointers to the deta `SECURE_HSTS_INCLUDE_SUBDOMAINS`, `SECURE_HSTS_PRELOAD`) silently never get sent. See [Production deployment](production-deploy.md#reverse-proxy). - **A non-superuser database role**: see [Database role](#database-role) below. +- **Redact public-link tokens from your reverse proxy's access log.** The application scrubs its + own logs, but nginx, Caddy or a load balancer in front of it records every path it forwards. See + [Public-link tokens in logs](#public-link-tokens-in-logs) below. - **Don't set `ENV_TYPE=prod` casually.** It's not just a flag: it makes `AWS_BUCKET_NAME`, `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, `AWS_SES_REGION_NAME`, `AWS_SES_REGION_ENDPOINT` and `SENTRY_DSN` mandatory (a `KeyError` at startup if any is missing), switches email to Amazon SES, @@ -95,6 +98,68 @@ Three different credential types exist, with different blast radii if one leaks: credential that outlives its own revocation. Credential management requires an interactive sign-in. +## Public-link tokens in logs + +Four kinds of link carry their credential as a path segment. Whoever holds the URL can use it +without signing in, so a log line that records the path hands the credential to anyone who can +read that log, or to wherever the log is shipped. + +| Link | API path | Web app path (the one emailed to customers) | +|---|---|---| +| Task calendar feed | `/api/public/calendar/.ics` | none | +| Satisfaction survey | `/api/public/csat//` | `/csat/` | +| Invoice | `/api/public/invoice//`, `.../pdf/` | `/portal/invoice/` | +| Estimate | `/api/public/estimate//`, `.../pdf/`, `.../accept/`, `.../decline/` | `/portal/estimate/` | + +From django-crm 1.13.0 the application writes `[Filtered]` in place of the token everywhere it logs +a path: Django's request log (`Not Found: `, `Too Many Requests: `), Sentry events, and +the app server's access log. The last is a logging filter that `crm/settings.py` attaches to the +`uvicorn.access` and `gunicorn.access` loggers, so it applies to `uvicorn` (which logs every request +by default) and to `gunicorn --access-logfile` alike, with no flag to set. A line reads, for +example, `"GET /api/public/invoice/[Filtered]/pdf/ HTTP/1.1" 200`. The patterns live in +`backend/common/public_tokens.py`. + +**Your reverse proxy is outside the process and needs its own redaction.** nginx's default +`combined` format records the full request line and the `Referer`, and the web app's portal pages +are same-origin, so their token shows up in the `Referer` of every asset they load. For nginx, put +this in the `http {}` context (on Debian and Ubuntu, a new file under `/etc/nginx/conf.d/`): + +```nginx +# BottleCRM: public-link tokens are credentials; log them as [Filtered]. +map $request_uri $bottlecrm_log_uri { + "~^((?:/api/public/(?:calendar|csat|invoice|estimate)|/portal/(?:invoice|estimate)|/csat)/)[^/?]+(.*)$" "$1[Filtered]$2"; + default $request_uri; +} + +map $http_referer $bottlecrm_log_referer { + "~^((?:https?://[^/]+)?(?:/api/public/(?:calendar|csat|invoice|estimate)|/portal/(?:invoice|estimate)|/csat)/)[^/?]+(.*)$" "$1[Filtered]$2"; + default $http_referer; +} + +# nginx's "combined" format, with the request line and Referer redacted. +log_format bottlecrm_redacted '$remote_addr - $remote_user [$time_local] ' + '"$request_method $bottlecrm_log_uri $server_protocol" ' + '$status $body_bytes_sent "$bottlecrm_log_referer" "$http_user_agent"'; +``` + +Then name that format on every `access_log` line in the `server {}` blocks for the API and the web +app, keeping each block's existing path. A block with no `access_log` line inherits the global one, +so add one: + +```nginx +access_log /var/log/nginx/access.log bottlecrm_redacted; +``` + +Check every `location {}` inside those blocks too: one with its own `access_log` does not inherit +the server's. Run `nginx -t` before reloading. With Caddy, Traefik or a cloud load balancer, apply the +same pattern with its own log-filter feature, or stop it logging those paths. + +Two limits worth knowing. nginx's `error_log` has no format, so an upstream failure (a 502 or 504 on +one of these paths) still writes the raw request line there. And lines written before you made +these changes still hold working tokens: shorten their retention or remove them. A member can cut +off a leaked calendar feed by regenerating it, and a survey link expires, but an invoice's or +estimate's link cannot be rotated or switched off from the app or the API. + ## What to monitor `SecurityAuditLog` (`backend/common/audit_log.py`, table `security_audit_log`) is a table of diff --git a/frontend/src/app.d.ts b/frontend/src/app.d.ts index 7c9ce88f..2eda3318 100644 --- a/frontend/src/app.d.ts +++ b/frontend/src/app.d.ts @@ -11,6 +11,7 @@ declare global { default_currency?: string; currency_symbol?: string; default_country?: string | null; + timezone?: string; }; profile?: { role?: string; diff --git a/frontend/src/hooks.server.js b/frontend/src/hooks.server.js index 46db2c86..cd2de6c6 100644 --- a/frontend/src/hooks.server.js +++ b/frontend/src/hooks.server.js @@ -21,7 +21,7 @@ const API_BASE_URL = `${env.PUBLIC_DJANGO_API_URL}/api`; const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; /** - * @typedef {{ default_currency?: string, currency_symbol?: string, default_country?: string|null }} OrgSettingsPayload + * @typedef {{ default_currency?: string, currency_symbol?: string, default_country?: string|null, timezone?: string }} OrgSettingsPayload * @typedef {{ org_id?: string, org_name?: string, role?: string, is_organization_admin?: boolean, user_id?: string, user_name?: string, user_email?: string, user_profile_pic?: string, exp?: number, iat?: number, org_settings?: OrgSettingsPayload }} JWTPayload * @typedef {{ id: string, name: string }} OrgInfo * @typedef {{ org: OrgInfo, role?: string, is_organization_admin?: boolean }} ProfileInfo diff --git a/frontend/src/lib/server/v2/bulk-form.js b/frontend/src/lib/server/v2/bulk-form.js index 8ee67c2c..724eee17 100644 --- a/frontend/src/lib/server/v2/bulk-form.js +++ b/frontend/src/lib/server/v2/bulk-form.js @@ -3,7 +3,8 @@ export const M2M_FIELDS = new Set(['assigned_to', 'tags']); /** * Turn the bulk form into { ids, fields }. `field` names which field to set; * `value` is repeatable, so an m2m field arrives as an array and a scalar as a - * single string. A Close also carries `closed_on`. + * single string. A Close carries `closed_on` only when a date was picked; + * without one the API dates it today in the org's timezone. * @param {FormData} formData */ export function parseBulkForm(formData) { diff --git a/frontend/src/lib/server/v2/inbound-email.js b/frontend/src/lib/server/v2/inbound-email.js index ad8e139f..9a3ae31a 100644 --- a/frontend/src/lib/server/v2/inbound-email.js +++ b/frontend/src/lib/server/v2/inbound-email.js @@ -8,19 +8,18 @@ * exactly via the `EmailMessage.mailbox` FK. Totals carry `count`, `active`, and * the org's `cases_last_30d`. * - * NO SECRETS. `webhook_secret` is the credential that proves a delivery really - * came from the provider. Anything holding it can forge tickets into this org. - * The backend already strips it for non-admins, but this layer drops it for - * everyone: it is never needed to render the page, so it never reaches the - * browser, admin or not. `topic_arn` embeds the AWS account id and is held to - * the same bar. Rotation and a one-time reveal on create are an explicit, - * separate feature this task does not build, not a field on a page you can - * browse to. + * NO SECRETS. `webhook_secret` is write-only server-side, reserved for + * providers that sign deliveries with a shared secret (none are implemented), + * and nothing in the backend compares it. This layer neither reads nor sends + * it; see the comment above `CREATE_FIELDS`. * - * The rule extends to the write side, below: `CREATE_FIELDS` and - * `UPDATE_FIELDS` exclude both fields too, on purpose, not by oversight. See - * the comment above `CREATE_FIELDS` for why omitting them is strictly better - * than any form could do. + * `topic_arn` is not a secret, but it embeds the AWS account id, so the backend + * returns it to admins only. It is the mailbox's SNS topic pin: the webhook + * refuses every message whose TopicArn is not exactly this one, and a mailbox + * with none accepts nothing until an admin enters it here or a confirmation + * arrives from an AWS account the server allows (`INBOUND_SNS_ACCOUNT_IDS`). + * So an admin reads, sets, changes and clears it on this page; a member gets + * only `has_topic_arn`. * * Create, edit, turn off, turn on and delete are wired below. The backend * returns `default_assignee` as a full profile object (name under @@ -61,7 +60,10 @@ export async function getMailboxes({ cookies }) { // address as one creating tickets. See `./delivery.js`. Defaults to // false, which reads as "not connected yet": failing closed is the safe // direction for a status this page draws a green pill from. - has_topic_arn: m.has_topic_arn === true + has_topic_arn: m.has_topic_arn === true, + // The ARN itself, which the backend sends to admins only. Null for a + // member, whose page has no form to put it in. + topic_arn: typeof m.topic_arn === 'string' ? m.topic_arn : null })), totals: totals ?? { count: 0, active: 0, cases_last_30d: 0 }, // A display hint: POST/PUT/DELETE on `/cases/mailboxes/` each start with @@ -72,7 +74,7 @@ export async function getMailboxes({ cookies }) { }; } -/** No `webhook_secret`, no `topic_arn`. +/** No `webhook_secret`. * * `webhook_secret` is reserved for providers that sign deliveries with a * shared secret, none of which are implemented: nothing in the backend @@ -85,16 +87,18 @@ export async function getMailboxes({ cookies }) { * for it would mean a credential travels to a browser and back on every edit * while an empty one blanks the column, so this page has neither. * - * `topic_arn` is set by the webhook from the first verified - * SubscriptionConfirmation, so nothing here should be writing it either. Its - * presence reaches the page as the `has_topic_arn` boolean above. */ + * `topic_arn` is here, and an empty string is how an admin clears it. The + * form only hands one over when the admin changed it (`topicArnEdit`), so + * saving an edit opened before the webhook pinned the mailbox leaves the new + * pin alone rather than blanking it. */ const CREATE_FIELDS = [ 'address', 'provider', 'default_priority', 'default_case_type', 'default_assignee_id', - 'is_active' + 'is_active', + 'topic_arn' ]; /** `address` is the mailbox's identity and its uniqueness key. It is not @@ -122,9 +126,29 @@ function buildBody(allowed, values) { if (body.address !== undefined) { body.address = String(body.address).trim().toLowerCase(); } + if (body.topic_arn !== undefined) body.topic_arn = String(body.topic_arn ?? '').trim(); return body; } +/** + * The `topic_arn` a form save should send, or undefined to leave it alone. + * + * Only a change is sent. The form is prefilled with the ARN as it stood when + * the page loaded, and the webhook can pin a mailbox after that (a + * confirmation from an allowed AWS account), so resending the stale value, or + * the empty one, would silently undo a pin nobody on this page chose to undo. + * Clearing the field on purpose is a change, and sends `''`. + * + * @param {FormDataEntryValue | null | undefined} submitted the field's value + * @param {FormDataEntryValue | null | undefined} loaded what it was prefilled with + * @returns {string | undefined} + */ +export function topicArnEdit(submitted, loaded) { + if (submitted == null) return undefined; + const next = String(submitted).trim(); + return next === String(loaded ?? '').trim() ? undefined : next; +} + /** @param {{ cookies: import('@sveltejs/kit').Cookies }} event */ export async function createMailbox({ cookies }, values) { const body = buildBody(CREATE_FIELDS, values); @@ -143,8 +167,8 @@ export async function updateMailbox({ cookies }, id, values) { * Delete a mailbox, permanently. * * `InboundMailboxDetailView.delete` calls `obj.delete()`. Hard delete, and it - * takes the row's `webhook_secret` with it, so mail already in flight from the - * provider stops verifying the moment this runs, not just stops opening tickets. + * takes the row's topic pin and its webhook lookup with it, so mail already in + * flight from AWS is refused the moment this runs. * * @param {{ cookies: import('@sveltejs/kit').Cookies }} event */ diff --git a/frontend/src/lib/server/v2/inbound-email.test.js b/frontend/src/lib/server/v2/inbound-email.test.js index 0d21b174..9958abd4 100644 --- a/frontend/src/lib/server/v2/inbound-email.test.js +++ b/frontend/src/lib/server/v2/inbound-email.test.js @@ -3,7 +3,10 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const apiRequest = vi.fn(); vi.mock('$lib/api-helpers.js', () => ({ apiRequest: (...a) => apiRequest(...a) })); -const { createMailbox, updateMailbox, deleteMailbox } = await import('./inbound-email.js'); +const { createMailbox, updateMailbox, deleteMailbox, getMailboxes, topicArnEdit } = + await import('./inbound-email.js'); + +const ARN = 'arn:aws:sns:us-east-1:123456789012:inbound'; const cookies = /** @type {any} */ ({ get: () => 'token' }); const event = /** @type {any} */ ({ cookies }); @@ -44,16 +47,18 @@ describe('createMailbox', () => { expect(opts.body.created_by).toBeUndefined(); }); - it('never sends a webhook_secret or topic_arn, even when handed one', async () => { + it('never sends a webhook_secret, even when handed one', async () => { apiRequest.mockResolvedValue({}); - await createMailbox(event, { - ...base, - webhook_secret: 'ATTACKER-CHOSEN', - topic_arn: 'arn:aws:sns:us-east-1:999999999999:x' - }); + await createMailbox(event, { ...base, webhook_secret: 'ATTACKER-CHOSEN' }); const [, opts] = apiRequest.mock.calls[0]; expect(opts.body.webhook_secret).toBeUndefined(); - expect(opts.body.topic_arn).toBeUndefined(); + }); + + it('sends an admin-entered topic_arn, trimmed', async () => { + apiRequest.mockResolvedValue({}); + await createMailbox(event, { ...base, topic_arn: ` ${ARN} ` }); + const [, opts] = apiRequest.mock.calls[0]; + expect(opts.body.topic_arn).toBe(ARN); }); it('trims and lowercases the address before sending', async () => { @@ -133,16 +138,25 @@ describe('updateMailbox', () => { expect(body.created_by).toBeUndefined(); }); - it('never sends a webhook_secret or topic_arn, even when handed one', async () => { + it('never sends a webhook_secret, even when handed one', async () => { apiRequest.mockResolvedValue({}); - await updateMailbox(event, 'm1', { - ...base, - webhook_secret: 'ATTACKER-CHOSEN', - topic_arn: 'arn:aws:sns:us-east-1:999999999999:x' - }); + await updateMailbox(event, 'm1', { ...base, webhook_secret: 'ATTACKER-CHOSEN' }); const { body } = apiRequest.mock.calls[0][1]; expect(body.webhook_secret).toBeUndefined(); - expect(body.topic_arn).toBeUndefined(); + }); + + it('sends a changed topic_arn, and an empty one clears it', async () => { + apiRequest.mockResolvedValue({}); + await updateMailbox(event, 'm1', { topic_arn: ARN }); + await updateMailbox(event, 'm1', { topic_arn: '' }); + expect(apiRequest.mock.calls[0][1].body).toEqual({ topic_arn: ARN }); + expect(apiRequest.mock.calls[1][1].body).toEqual({ topic_arn: '' }); + }); + + it('leaves topic_arn out when the form did not change it', async () => { + apiRequest.mockResolvedValue({}); + await updateMailbox(event, 'm1', { ...base, topic_arn: undefined }); + expect('topic_arn' in apiRequest.mock.calls[0][1].body).toBe(false); }); it('sends exactly one key for a minimal { is_active: true } body (the turn-on path)', async () => { @@ -195,3 +209,52 @@ describe('deleteMailbox', () => { expect(apiRequest).not.toHaveBeenCalled(); }); }); + +describe('topicArnEdit', () => { + it('is undefined when the field was not on the form', () => { + expect(topicArnEdit(null, '')).toBeUndefined(); + expect(topicArnEdit(undefined, ARN)).toBeUndefined(); + }); + + it('is undefined when the value is what the form was prefilled with', () => { + // A stale form must not undo a pin the webhook set after the page loaded. + expect(topicArnEdit('', '')).toBeUndefined(); + expect(topicArnEdit(` ${ARN} `, ARN)).toBeUndefined(); + }); + + it('is the new value, trimmed, when the admin changed it', () => { + expect(topicArnEdit(` ${ARN}`, '')).toBe(ARN); + expect(topicArnEdit('arn:aws:sns:eu-west-1:123456789012:other', ARN)).toBe( + 'arn:aws:sns:eu-west-1:123456789012:other' + ); + }); + + it('is the empty string when the admin cleared it', () => { + expect(topicArnEdit(' ', ARN)).toBe(''); + }); +}); + +describe('getMailboxes', () => { + beforeEach(() => { + apiRequest.mockReset(); + }); + + const row = { id: 'm1', address: 'a@b.io', provider: 'ses', is_active: true }; + + it('carries the ARN the backend sent an admin', async () => { + apiRequest.mockResolvedValue({ + mailboxes: [{ ...row, has_topic_arn: true, topic_arn: ARN, webhook_secret: 'x' }] + }); + const { mailboxes } = await getMailboxes(event); + expect(mailboxes[0].topic_arn).toBe(ARN); + expect(mailboxes[0].has_topic_arn).toBe(true); + expect('webhook_secret' in mailboxes[0]).toBe(false); + }); + + it('has a null ARN for a member, who is sent only has_topic_arn', async () => { + apiRequest.mockResolvedValue({ mailboxes: [{ ...row, has_topic_arn: true }] }); + const { mailboxes } = await getMailboxes(event); + expect(mailboxes[0].topic_arn).toBeNull(); + expect(mailboxes[0].has_topic_arn).toBe(true); + }); +}); diff --git a/frontend/src/lib/server/v2/tickets.js b/frontend/src/lib/server/v2/tickets.js index 129a939b..d564b7ed 100644 --- a/frontend/src/lib/server/v2/tickets.js +++ b/frontend/src/lib/server/v2/tickets.js @@ -603,6 +603,10 @@ function toBody(values) { for (const field of EDITABLE_FIELDS) { if (!(field in values)) continue; const value = values[field]; + // An empty close date is left off: the API dates a close that carries + // none today in the org's timezone, while a null would erase the date of + // a ticket that is already Closed. + if (field === 'closed_on' && value === '') continue; body[field] = value === '' ? null : value; } // Single-select owner, so the list is empty or one long. Only present when diff --git a/frontend/src/lib/server/v2/timesheet.js b/frontend/src/lib/server/v2/timesheet.js index f16087e1..5092b988 100644 --- a/frontend/src/lib/server/v2/timesheet.js +++ b/frontend/src/lib/server/v2/timesheet.js @@ -24,35 +24,20 @@ */ import { apiRequest } from '$lib/api-helpers.js'; -/** - * Mon..Sun ISO-week range for `date` (UTC), as YYYY-MM-DD strings. - * @param {Date} date - */ -function isoWeekRange(date) { - const d = new Date(Date.UTC(date.getUTCFullYear(), date.getUTCMonth(), date.getUTCDate())); - const dow = (d.getUTCDay() + 6) % 7; // Mon=0 - d.setUTCDate(d.getUTCDate() - dow); - const start = d.toISOString().slice(0, 10); - d.setUTCDate(d.getUTCDate() + 6); - const end = d.toISOString().slice(0, 10); - return { start, end }; -} - /** * The caller's timesheet for a Mon..Sun week, shaped for the page. * + * With no explicit week the range is left off and the API picks this week in + * the org's timezone. Computing it here would take UTC's week, which is last + * week for a Monday morning east of UTC. + * * @param {{ cookies: import('@sveltejs/kit').Cookies }} event - * @param {{ start?: string, end?: string }} [range] explicit week; defaults to this ISO week + * @param {{ start?: string, end?: string }} [range] explicit week; the API needs both ends * @returns {Promise<{ week: any }>} */ export async function getTimesheet({ cookies }, { start, end } = {}) { - if (!start || !end) { - const range = isoWeekRange(new Date()); - start = start || range.start; - end = end || range.end; - } - const qs = new URLSearchParams({ start, end }); - const week = await apiRequest(`/time-entries/timesheet/?${qs.toString()}`, {}, { cookies }); + const qs = start && end ? `?${new URLSearchParams({ start, end }).toString()}` : ''; + const week = await apiRequest(`/time-entries/timesheet/${qs}`, {}, { cookies }); return { week }; } diff --git a/frontend/src/lib/server/v2/timesheet.test.js b/frontend/src/lib/server/v2/timesheet.test.js index 1b861135..2b9bbf5d 100644 --- a/frontend/src/lib/server/v2/timesheet.test.js +++ b/frontend/src/lib/server/v2/timesheet.test.js @@ -10,7 +10,8 @@ const { logTicketTime, setEntryBillable, deleteEntry, - getTimeReport + getTimeReport, + getTimesheet } = await import('$lib/server/v2/timesheet.js'); // Cast rather than shaping a full Cookies mock, matching leads.test.js and // tags.test.js: stopTimer only ever calls `cookies.get`, and `apiRequest` @@ -187,3 +188,22 @@ describe('the time report', () => { expect(sentParams()).toEqual({ group_by: 'agent' }); }); }); + +describe('the week', () => { + beforeEach(() => { + apiRequest.mockReset(); + apiRequest.mockResolvedValue({ start: '2026-01-05', end: '2026-01-11', days: [] }); + }); + + it('leaves the range off with no explicit week, so the API picks the org’s week', async () => { + await getTimesheet(event); + expect(apiRequest.mock.calls[0][0]).toBe('/time-entries/timesheet/'); + }); + + it('sends an explicit week as given', async () => { + await getTimesheet(event, { start: '2026-01-05', end: '2026-01-11' }); + expect(apiRequest.mock.calls[0][0]).toBe( + '/time-entries/timesheet/?start=2026-01-05&end=2026-01-11' + ); + }); +}); diff --git a/frontend/src/lib/v2/audit-log.js b/frontend/src/lib/v2/audit-log.js index 7c7cae68..88125533 100644 --- a/frontend/src/lib/v2/audit-log.js +++ b/frontend/src/lib/v2/audit-log.js @@ -19,7 +19,7 @@ export function auditActor(entry) { /** * One line on what happened, or '' when the event label says it all. - * @param {{ event_type?: string, details?: Record }} entry + * @param {{ event_type?: string, actor?: { id?: string } | null, details?: Record }} entry */ export function auditDetail(entry) { const d = entry?.details ?? {}; @@ -31,6 +31,17 @@ export function auditDetail(entry) { const tag = (/** @type {any} */ id) => (same && id ? ` (${String(id).slice(0, 8)})` : ''); return `Merged ${d.entity || 'record'} "${d.merged_name}"${tag(d.merged_id)} into "${d.kept_name}"${tag(d.kept_id)}.`; } + if ( + (entry?.event_type === 'API_TOKEN_CREATED' || entry?.event_type === 'API_TOKEN_REVOKED') && + d.token_prefix + ) { + // An empty scope list is a token with the owner's full access. The owner + // is named only when someone else acted: an admin revoking their token. + const scopes = + Array.isArray(d.scopes) && d.scopes.length ? `scopes ${d.scopes.join(', ')}` : 'full access'; + const owner = d.owner_id && d.owner_id !== entry?.actor?.id ? `, owned by ${d.owner_name}` : ''; + return `Token "${d.token_name}" (${d.token_prefix}), ${scopes}${owner}.`; + } if (entry?.event_type === 'WEBHOOK_REENABLED') { return 'Turned back on, and now answers for the webhook.'; } diff --git a/frontend/src/lib/v2/audit-log.test.js b/frontend/src/lib/v2/audit-log.test.js index 814b40a8..06d32224 100644 --- a/frontend/src/lib/v2/audit-log.test.js +++ b/frontend/src/lib/v2/audit-log.test.js @@ -72,6 +72,52 @@ describe('auditDetail', () => { ).toBe('Merged contact "Rosalind Beck" (1a2b3c4d) into "Rosalind Beck" (5e6f7a8b).'); }); + it('names a created token and says an unscoped one has full access', () => { + expect( + auditDetail({ + event_type: 'API_TOKEN_CREATED', + actor: { id: 'u1' }, + details: { + token_prefix: 'bcrm_pat_ab12', + token_name: 'CI script', + scopes: [], + owner_id: 'u1', + owner_name: 'Asha' + } + }) + ).toBe('Token "CI script" (bcrm_pat_ab12), full access.'); + }); + + it('names the owner when an admin revoked a member token', () => { + expect( + auditDetail({ + event_type: 'API_TOKEN_REVOKED', + actor: { id: 'admin' }, + details: { + token_prefix: 'bcrm_pat_ab12', + token_name: 'laptop', + scopes: ['leads:read', 'contacts:read'], + owner_id: 'u1', + owner_name: 'Asha' + } + }) + ).toBe('Token "laptop" (bcrm_pat_ab12), scopes leads:read, contacts:read, owned by Asha.'); + }); + + it('adds nothing for a calendar feed event, whose label says it all', () => { + for (const event_type of [ + 'CALENDAR_FEED_ENABLED', + 'CALENDAR_FEED_REGENERATED', + 'CALENDAR_FEED_DISABLED' + ]) { + expect(auditDetail({ event_type, details: {} })).toBe(''); + } + }); + + it('adds nothing for an event type it has never heard of', () => { + expect(auditDetail({ event_type: 'SOMETHING_NEW', details: { whatever: 1 } })).toBe(''); + }); + it('is empty when there is nothing to add', () => { expect(auditDetail({ event_type: 'LOGIN_SUCCESS', details: {} })).toBe(''); }); diff --git a/frontend/src/lib/v2/components/BulkActionBar.svelte b/frontend/src/lib/v2/components/BulkActionBar.svelte index aedc179b..76ae6e82 100644 --- a/frontend/src/lib/v2/components/BulkActionBar.svelte +++ b/frontend/src/lib/v2/components/BulkActionBar.svelte @@ -23,8 +23,6 @@ // from `action` so leaving and re-entering the delete branch through Back // does not skip the confirm on the next attempt. let armed = $state(false); - // Today, for the Close date default. yyyy-mm-dd. - const today = new Date().toISOString().slice(0, 10); // Duplicate is reached only by merging; the bulk endpoint refuses it. const SETTABLE_STATUSES = CASE_STATUSES.filter((v) => v !== 'Duplicate'); @@ -116,7 +114,16 @@ {#each SETTABLE_STATUSES as v (v)}{/each} {#if statusValue === 'Closed'} - + + + Empty: dated today in the org timezone. {/if} {/if} diff --git a/frontend/src/lib/v2/dates.js b/frontend/src/lib/v2/dates.js new file mode 100644 index 00000000..0169ad1f --- /dev/null +++ b/frontend/src/lib/v2/dates.js @@ -0,0 +1,44 @@ +/** + * Calendar dates for a form's defaults, as `YYYY-MM-DD`. + * + * "Today" is the org's day, not UTC's and not the browser's: an invoice raised + * at 09:00 in Kolkata is dated that day, although UTC still reads yesterday. + * The zone comes from the layout (`data.org.timezone`, the JWT's org claim). + */ + +/** + * Today in `timeZone`. A zone the runtime does not know falls back to UTC, the + * API's own default zone, rather than throwing on a form's first render. + * + * @param {string} timeZone IANA name, e.g. `Asia/Kolkata` + * @param {Date} [now] + */ +export function todayIn(timeZone, now = new Date()) { + let parts; + try { + parts = new Intl.DateTimeFormat('en-US', { + timeZone, + year: 'numeric', + month: '2-digit', + day: '2-digit' + }).formatToParts(now); + } catch { + return now.toISOString().slice(0, 10); + } + /** @param {string} type */ + const part = (type) => parts.find((p) => p.type === type)?.value; + return `${part('year')}-${part('month')}-${part('day')}`; +} + +/** + * `days` after a `YYYY-MM-DD` date. Pure calendar arithmetic in UTC, so no + * zone can move the answer. + * + * @param {string} isoDate + * @param {number} days + */ +export function addDays(isoDate, days) { + const d = new Date(`${isoDate}T00:00:00Z`); + d.setUTCDate(d.getUTCDate() + days); + return d.toISOString().slice(0, 10); +} diff --git a/frontend/src/lib/v2/dates.test.js b/frontend/src/lib/v2/dates.test.js new file mode 100644 index 00000000..906ef027 --- /dev/null +++ b/frontend/src/lib/v2/dates.test.js @@ -0,0 +1,39 @@ +import { describe, it, expect } from 'vitest'; +import { todayIn, addDays } from '$lib/v2/dates.js'; + +describe('todayIn', () => { + // 23:30 UTC on 31 Dec is 05:00 on 1 Jan in Kolkata (UTC+5:30). + const lateNewYearsEve = new Date('2025-12-31T23:30:00Z'); + + it('is the org’s day across a UTC midnight boundary', () => { + expect(todayIn('Asia/Kolkata', lateNewYearsEve)).toBe('2026-01-01'); + }); + + it('is still the old day west of UTC', () => { + expect(todayIn('America/New_York', lateNewYearsEve)).toBe('2025-12-31'); + expect(todayIn('UTC', lateNewYearsEve)).toBe('2025-12-31'); + }); + + it('is the prior day in the Americas after UTC midnight', () => { + expect(todayIn('America/Los_Angeles', new Date('2026-03-01T03:00:00Z'))).toBe('2026-02-28'); + }); + + it('reads a legacy zone name as its IANA target', () => { + expect(todayIn('US/Eastern', lateNewYearsEve)).toBe('2025-12-31'); + }); + + it('falls back to UTC for a zone the runtime does not know', () => { + expect(todayIn('Not/AZone', lateNewYearsEve)).toBe('2025-12-31'); + }); +}); + +describe('addDays', () => { + it('crosses a month and a year', () => { + expect(addDays('2026-12-15', 30)).toBe('2027-01-14'); + expect(addDays('2026-01-31', 1)).toBe('2026-02-01'); + }); + + it('is not moved by a DST change', () => { + expect(addDays('2026-03-07', 2)).toBe('2026-03-09'); + }); +}); diff --git a/frontend/src/routes/(app)/+layout.server.js b/frontend/src/routes/(app)/+layout.server.js index 22ab41c6..a3ac2f16 100644 --- a/frontend/src/routes/(app)/+layout.server.js +++ b/frontend/src/routes/(app)/+layout.server.js @@ -136,7 +136,11 @@ export async function load(event) { // gets an addition that should not have happened, and no symbol repairs // it. Read from the JWT's org settings, which is also where a new deal's // currency comes from. - currency: /** @type {any} */ (event.locals).org_settings?.default_currency || 'USD' + currency: /** @type {any} */ (event.locals).org_settings?.default_currency || 'USD', + // The org's IANA zone, from the same claim, for a form's "today" + // default (`todayIn`, `$lib/v2/dates.js`). UTC only for a token minted + // before the claim existed; the next refresh carries it. + timezone: /** @type {any} */ (event.locals).org_settings?.timezone || 'UTC' }, // Server-derived from the JWT (never the client), and read only through // `isOrgAdmin` (`$lib/admin.js`). Display-only: it lets the shell hide diff --git a/frontend/src/routes/(app)/invoices/estimates/new/+page.svelte b/frontend/src/routes/(app)/invoices/estimates/new/+page.svelte index 661c2b17..f6b4f322 100644 --- a/frontend/src/routes/(app)/invoices/estimates/new/+page.svelte +++ b/frontend/src/routes/(app)/invoices/estimates/new/+page.svelte @@ -36,13 +36,15 @@ num, taxRateError } from '$lib/v2/line-items.js'; + import { addDays, todayIn } from '$lib/v2/dates.js'; /** @type {{ data: any, form: any }} */ let { data, form } = $props(); const CURRENCIES = CURRENCY_CODES.filter((c) => c.value); - const today = new Date().toISOString().slice(0, 10); - const in30 = new Date(Date.now() + 30 * 86_400_000).toISOString().slice(0, 10); + // The org's day: a UTC date is yesterday for a morning east of UTC. + const today = untrack(() => todayIn(data.org.timezone)); + const in30 = addDays(today, 30); /* * The starting values, read once. A prefill naming a record the pickers do diff --git a/frontend/src/routes/(app)/invoices/new/+page.svelte b/frontend/src/routes/(app)/invoices/new/+page.svelte index 66056a25..bbc74c35 100644 --- a/frontend/src/routes/(app)/invoices/new/+page.svelte +++ b/frontend/src/routes/(app)/invoices/new/+page.svelte @@ -40,6 +40,7 @@ import { PAYMENT_TERMS_LABEL } from '$lib/v2/enums.js'; import { CURRENCY_CODES } from '$lib/constants/filters.js'; import { longDate } from '$lib/v2/format.js'; + import { todayIn } from '$lib/v2/dates.js'; import { blankLine, documentDiscountError, @@ -52,7 +53,7 @@ } from '$lib/v2/line-items.js'; import { Info } from '@lucide/svelte'; - /** @type {{ data: { products: any[], accounts: any[], contacts: any[], org: { currency: string } }, form: any }} */ + /** @type {{ data: { products: any[], accounts: any[], contacts: any[], org: { currency: string, timezone: string } }, form: any }} */ let { data, form } = $props(); /* The estimate builder's list. The org's default currency to start, read @@ -69,7 +70,8 @@ let accountId = $state(''); let contactId = $state(''); let title = $state(''); - let issueDate = $state(new Date().toISOString().slice(0, 10)); + // The org's day: a UTC date is yesterday for a morning east of UTC. + let issueDate = $state(untrack(() => todayIn(data.org.timezone))); let paymentTerms = $state('NET_30'); let customDueDate = $state(''); let discountType = $state(''); diff --git a/frontend/src/routes/(app)/invoices/recurring/new/+page.svelte b/frontend/src/routes/(app)/invoices/recurring/new/+page.svelte index 48261804..ef785c3b 100644 --- a/frontend/src/routes/(app)/invoices/recurring/new/+page.svelte +++ b/frontend/src/routes/(app)/invoices/recurring/new/+page.svelte @@ -35,6 +35,7 @@ * guard over a real backend gap, not a mirror of a server rule. */ import { enhance } from '$app/forms'; + import { untrack } from 'svelte'; import PageHeader from '$lib/v2/components/PageHeader.svelte'; import SectionTabs from '$lib/v2/components/SectionTabs.svelte'; import PortalLineItems from '$lib/v2/components/PortalLineItems.svelte'; @@ -49,11 +50,13 @@ num, taxRateError } from '$lib/v2/line-items.js'; + import { todayIn } from '$lib/v2/dates.js'; - /** @type {{ data: { products: any[], accounts: any[], contacts: any[] }, form: any }} */ + /** @type {{ data: { products: any[], accounts: any[], contacts: any[], org: { timezone: string } }, form: any }} */ let { data, form } = $props(); - const today = new Date().toISOString().slice(0, 10); + // The org's day: a UTC date is yesterday for a morning east of UTC. + const today = untrack(() => todayIn(data.org.timezone)); /** * The currency codes `RecurringInvoice.currency` accepts (`common.utils. @@ -91,6 +94,9 @@ let startDate = $state(today); let endDate = $state(''); let nextGenerationDate = $state(today); + // The first run follows the start date until someone picks it themselves; + // the server refuses a first run before the start. + let nextPicked = $state(false); let autoSend = $state(false); let discountType = $state(''); let discountValue = $state(0); @@ -265,12 +271,23 @@