From edfed1bf68ea564be7b01d3194e8723358bba1a6 Mon Sep 17 00:00:00 2001 From: Daniel Holbach Date: Wed, 28 Oct 2026 12:14:59 +0100 Subject: [PATCH 1/8] fix(security): bind the dev stack's gunicorn to loopback docker-compose.yml runs django with network_mode: host and bound gunicorn to 0.0.0.0:8000 with DEBUG on, so the dev stack (which mounts the real data directories) was reachable from the local network over plain HTTP. Bind 127.0.0.1 like the postgres service and the prod compose default, and add a guardrail test. Co-Authored-By: Claude Opus 5.5 --- app/my_practice/tests/test_prod_network.py | 7 +++++++ docker-compose.yml | 5 ++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/my_practice/tests/test_prod_network.py b/app/my_practice/tests/test_prod_network.py index e29b38a..f14fdee 100644 --- a/app/my_practice/tests/test_prod_network.py +++ b/app/my_practice/tests/test_prod_network.py @@ -47,6 +47,13 @@ def test_port_is_published_on_loopback_by_default(self): ports = re.findall(r'^\s+- "(.*:8000:8000)"', compose, re.M) self.assertEqual(ports, ["${MY_PRACTICE_BIND_ADDRESS:-127.0.0.1}:8000:8000"]) + def test_dev_stack_binds_loopback(self): + # The dev stack runs with network_mode: host, so gunicorn's bind address + # is the only thing keeping it off the LAN. + compose = (REPO_ROOT / "docker-compose.yml").read_text() + self.assertIn("network_mode: host", compose) + self.assertEqual(re.findall(r"gunicorn --bind (\S+)", compose), ["127.0.0.1:8000"]) + def test_bind_address_documented_in_env_example(self): self.assertIn("MY_PRACTICE_BIND_ADDRESS=", (REPO_ROOT / ".env.example").read_text()) diff --git a/docker-compose.yml b/docker-compose.yml index 6d7f177..7b02b07 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -81,6 +81,9 @@ services: depends_on: postgres: condition: service_healthy + # gunicorn binds loopback, like postgres above: network_mode: host makes the + # bind address the only thing between the DEBUG stack (with the real data + # directories mounted) and the local network. # compilemessages: the .mo catalogs are gitignored, and the ./app bind mount # hides the copy the image compiled, so the container compiles its own on # start. --reload-extra-file then picks up a recompile (e.g. from the @@ -89,7 +92,7 @@ services: sh -c "python manage.py migrate --noinput && python manage.py collectstatic --noinput --ignore tailwind.css && python manage.py compilemessages -v 0 && - gunicorn --bind 0.0.0.0:8000 --workers 1 --timeout 300 --reload --reload-extra-file locale/de/LC_MESSAGES/django.mo --reload-extra-file locale/en/LC_MESSAGES/django.mo config.wsgi:application" + gunicorn --bind 127.0.0.1:8000 --workers 1 --timeout 300 --reload --reload-extra-file locale/de/LC_MESSAGES/django.mo --reload-extra-file locale/en/LC_MESSAGES/django.mo config.wsgi:application" tailwind: build: From 9614486732a2c455b571de8ac14782b5faaacbbc Mon Sep 17 00:00:00 2001 From: Daniel Holbach Date: Wed, 28 Oct 2026 12:15:08 +0100 Subject: [PATCH 2/8] fix(security): NFKC-normalize the login throttle key AuthenticationForm normalizes the username with NFKC before the account lookup, but the lockout key only lowercased the raw input. Fullwidth and other compatibility spellings therefore reached the same account under a fresh counter, defeating the 5-attempt limit. Co-Authored-By: Claude Opus 5.5 --- app/my_practice/tests/test_login_throttle.py | 12 ++++++++++++ app/my_practice/views/auth_views.py | 6 +++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/my_practice/tests/test_login_throttle.py b/app/my_practice/tests/test_login_throttle.py index c4a817b..2304ab2 100644 --- a/app/my_practice/tests/test_login_throttle.py +++ b/app/my_practice/tests/test_login_throttle.py @@ -61,6 +61,18 @@ def test_username_case_does_not_bypass_the_limit(self): self._attempt("wrong", username="Therapist") self.assertEqual(self._attempt("testpass123").status_code, 429) + def test_compatibility_spelling_does_not_bypass_the_limit(self): + # "therapist" (fullwidth) NFKC-normalizes to "therapist", which is + # the account Django's login form authenticates. + fullwidth = "".join(chr(ord(c) + 0xFEE0) for c in "therapist") + for _ in range(LOGIN_FAILURE_LIMIT): + self._attempt("wrong") + + response = self._attempt("testpass123", username=fullwidth) + + self.assertEqual(response.status_code, 429) + self.assertNotIn("_auth_user_id", self.client.session) + class SessionIdleTimeoutTests(TestCase): def test_session_expires_after_idle_hours_not_two_weeks(self): diff --git a/app/my_practice/views/auth_views.py b/app/my_practice/views/auth_views.py index d99dd8e..9928053 100644 --- a/app/my_practice/views/auth_views.py +++ b/app/my_practice/views/auth_views.py @@ -23,6 +23,7 @@ import hashlib import logging import time +import unicodedata from django.conf import settings from django.contrib import messages @@ -57,7 +58,10 @@ def _throttle_key(request: HttpRequest) -> str: - username = request.POST.get("username", "").strip().lower() + # NFKC first: AuthenticationForm's UsernameField normalizes that way before the + # account lookup, so without it a fullwidth or other compatibility spelling + # reaches the same account under a fresh counter. + username = unicodedata.normalize("NFKC", request.POST.get("username", "")).strip().lower() address = request.META.get("REMOTE_ADDR", "") # Hashed so the cache table does not hold usernames in clear. digest = hashlib.sha256(f"{username}|{address}".encode()).hexdigest() From 7ee11fc6a0a87e17aa588c7cad0a5b5d7217e743 Mon Sep 17 00:00:00 2001 From: Daniel Holbach Date: Wed, 28 Oct 2026 12:15:13 +0100 Subject: [PATCH 3/8] fix(gdpr): complete the Art. 17 erasure before notifying the client - Anonymise the inquiry the client was converted from and delete their imported calendar events; both only SET_NULL their link and kept the client's details after the erasure. - Delete GebueH service records before sessions (PROTECT), which made the erasure roll back for those clients. - Send the notification email only after the transaction has committed. Co-Authored-By: Claude Opus 5.5 --- app/my_practice/tests/test_views_client.py | 63 ++++++++++++++++++++++ app/my_practice/views/client_views.py | 29 +++++++--- 2 files changed, 85 insertions(+), 7 deletions(-) diff --git a/app/my_practice/tests/test_views_client.py b/app/my_practice/tests/test_views_client.py index 27f460f..786f94e 100644 --- a/app/my_practice/tests/test_views_client.py +++ b/app/my_practice/tests/test_views_client.py @@ -15,9 +15,14 @@ from ..models import ( Client, ClientDocument, + ClientInquiry, ClientTag, + GebuhZiffer, + InquiryStatus, Invoice, InvoiceItem, + Leistungserfassung, + PendingCalendarEvent, Practice, ServiceType, Session, @@ -967,6 +972,64 @@ def test_delete_continues_when_email_send_fails(self, mock_send): page_messages = list(response.context["messages"]) self.assertTrue(any("konnte nicht gesendet werden" in str(m) for m in page_messages)) + @patch("my_practice.views.client_views.EmailMessage.send") + def test_delete_anonymises_source_inquiry_and_drops_calendar_events(self, mock_send): + inquiry = ClientInquiry.objects.create( + practice=self.practice, + full_name="Max Mustermann", + email="max@example.com", + phone="+49 123 456789", + source=ClientInquiry._meta.get_field("source").choices[0][0], + status=InquiryStatus.CONVERTED, + converted_client=self.eligible_client, + ) + PendingCalendarEvent.objects.create( + practice=self.practice, + google_event_id="evt-gdpr-1", + summary="TC", + event_date=self.old_date, + duration_minutes=60, + matched_client=self.eligible_client, + ) + + self.http.post(reverse("client_gdpr_delete", kwargs={"pk": self.eligible_client.pk})) + + inquiry.refresh_from_db() + self.assertEqual((inquiry.full_name, inquiry.email, inquiry.phone), ("", "", "")) + self.assertIsNotNone(inquiry.anonymised_at) + self.assertFalse(PendingCalendarEvent.objects.filter(google_event_id="evt-gdpr-1").exists()) + + @patch("my_practice.views.client_views.EmailMessage.send") + def test_delete_succeeds_for_client_with_gebueh_services(self, mock_send): + # Leistungserfassung.session is PROTECT: it used to abort the erasure + # after the "your data has been deleted" email had already gone out. + ziffer = GebuhZiffer.objects.create( + nummer="T1", bezeichnung="Test", satz_min=Decimal("10"), satz_max=Decimal("50") + ) + Leistungserfassung.objects.create( + session=self.eligible_client.sessions.get(), + ziffer=ziffer, + betrag=Decimal("50"), + vereinbarter_betrag=Decimal("80"), + ) + client_pk = self.eligible_client.pk + + response = self.http.post(reverse("client_gdpr_delete", kwargs={"pk": client_pk})) + + self.assertRedirects(response, reverse("client_list")) + self.assertFalse(Client.objects.filter(pk=client_pk).exists()) + mock_send.assert_called_once() + + @patch("my_practice.views.client_views.EmailMessage.send") + def test_no_notification_when_erasure_fails(self, mock_send): + with ( + patch.object(Client, "delete", side_effect=RuntimeError("db down")), + self.assertRaises(RuntimeError), + ): + self.http.post(reverse("client_gdpr_delete", kwargs={"pk": self.eligible_client.pk})) + mock_send.assert_not_called() + self.assertTrue(Client.objects.filter(pk=self.eligible_client.pk).exists()) + def test_delete_client_without_email_skips_notification(self): self.eligible_client.email = "" self.eligible_client.save() diff --git a/app/my_practice/views/client_views.py b/app/my_practice/views/client_views.py index 50cc65b..7a9eca7 100644 --- a/app/my_practice/views/client_views.py +++ b/app/my_practice/views/client_views.py @@ -12,7 +12,7 @@ from django.contrib.postgres.search import SearchQuery, SearchRank, SearchVector from django.core.mail import EmailMessage from django.db import models, transaction -from django.db.models import Max, QuerySet +from django.db.models import Max, Q, QuerySet from django.http import HttpRequest, HttpResponse, JsonResponse from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse, reverse_lazy @@ -25,9 +25,12 @@ INVOICE_RETENTION_YEARS, Client, ClientDocument, + ClientInquiry, ClientTag, Invoice, InvoiceItem, + Leistungserfassung, + PendingCalendarEvent, ) from ..utils import ( RevenueCalculator, @@ -432,7 +435,7 @@ def client_gdpr_delete_confirm(request: HttpRequest, pk: int) -> HttpResponse: def _send_gdpr_deletion_email(request: HttpRequest, client: Client, practice) -> None: - """Best-effort notification email to the client before their data is erased.""" + """Best-effort notification email to the client once their data has been erased.""" if not (client.email and practice): return try: @@ -476,7 +479,7 @@ def _delete_files_from_disk(paths: list[str]) -> None: @require_POST def client_gdpr_delete(request: HttpRequest, pk: int) -> HttpResponse: - """GDPR Art. 17 deletion: send notification email then erase client data.""" + """GDPR Art. 17 deletion: erase client data, then send the notification email.""" client = get_object_or_404(Client.objects.for_current_practice(request), pk=pk) last_session = client.sessions.aggregate(last=Max("session_date"))["last"] @@ -493,18 +496,30 @@ def client_gdpr_delete(request: HttpRequest, pk: int) -> HttpResponse: practice = request.current_practice client_code = client.client_code - _send_gdpr_deletion_email(request, client, practice) doc_file_paths = _collect_document_file_paths(client) - # Delete in FK dependency order: invoices first, because InvoiceItem.session - # is PROTECT — deleting the sessions while billed items still point at them - # raises ProtectedError for any client who was ever invoiced. + # Delete in FK dependency order: invoices and GebueH service records first, + # because InvoiceItem.session and Leistungserfassung.session are PROTECT — + # deleting the sessions while either still points at them raises + # ProtectedError. The inquiry the client was converted from and imported + # calendar events only SET_NULL their link, so they would otherwise keep the + # client's details after the erasure the email announces. with transaction.atomic(): + source_inquiry = ClientInquiry.objects.filter(converted_client=client).first() + if source_inquiry: + source_inquiry.anonymise() + PendingCalendarEvent.objects.filter( + Q(matched_client=client) | Q(session__client=client) + ).delete() + Leistungserfassung.objects.filter(session__client=client).delete() client.invoices.all().delete() # cascades InvoiceItem client.sessions.all().delete() # cascades SessionLog client.delete() # cascades ClientDocument records, ClientProfile, # SupervisionItem, ClientNote, ClientAlias + # Only after the erasure has committed: the email says it has happened. + _send_gdpr_deletion_email(request, client, practice) + # Delete document files from disk after DB records are gone _delete_files_from_disk(doc_file_paths) From 8940f4c1a248dde3df8694e66b41221663f705ec Mon Sep 17 00:00:00 2001 From: Daniel Holbach Date: Wed, 28 Oct 2026 12:16:41 +0100 Subject: [PATCH 4/8] fix(security): make the scoped CRUD views fail closed PracticeScoped{List,Update,Delete}View only filtered by practice when the model's queryset had for_current_practice, and silently returned every row otherwise. MarketingPeriod has a practice FK but a plain manager, so its edit and delete views resolved any practice's row by pk. Give MarketingPeriod the PracticeScopedManager, make the mixins raise ImproperlyConfigured for a practice-FK model without one, and add a guardrail test over every scoped view. Co-Authored-By: Claude Opus 5.5 --- app/my_practice/models/inquiry.py | 2 + .../tests/test_practice_scoping.py | 57 +++++++++++++++++++ app/my_practice/views/crud_mixins.py | 37 +++++++----- 3 files changed, 82 insertions(+), 14 deletions(-) diff --git a/app/my_practice/models/inquiry.py b/app/my_practice/models/inquiry.py index c225c07..375cbae 100644 --- a/app/my_practice/models/inquiry.py +++ b/app/my_practice/models/inquiry.py @@ -268,6 +268,8 @@ class MarketingPeriod(TimestampedModel): help_text=_('e.g. "Google Ads €5/day" or "It\'s Complicated Premium"'), ) + objects = PracticeScopedManager() + class Meta: ordering = ["-start_date"] verbose_name = _("Marketing period") diff --git a/app/my_practice/tests/test_practice_scoping.py b/app/my_practice/tests/test_practice_scoping.py index a20b7b3..6078955 100644 --- a/app/my_practice/tests/test_practice_scoping.py +++ b/app/my_practice/tests/test_practice_scoping.py @@ -26,6 +26,7 @@ Client, CompanyExpense, Invoice, + MarketingPeriod, Practice, UserPractice, scope_to_practice, @@ -33,6 +34,12 @@ from ..utils import RevenueCalculator from ..utils.analytics_utils import ClientAnalyzer, ExpenseAnalyzer, RevenueAnalyzer from ..utils.practice_analysis import PracticeAnalyzer +from ..views.crud_mixins import ( + PracticeScopedDeleteView, + PracticeScopedListView, + PracticeScopedUpdateView, +) +from .test_helpers import create_test_practice, link_user_to_practice APP_DIR = Path(settings.BASE_DIR) / "my_practice" @@ -193,3 +200,53 @@ def test_analytics_page_for_member_shows_its_clients(self): self.assertEqual( [row["client"].client_code for row in response.context["top_clients"]], ["XX"] ) + + +def _subclasses(cls): + for sub in cls.__subclasses__(): + yield sub + yield from _subclasses(sub) + + +class ScopedViewGuardrailTests(SimpleTestCase): + def test_every_scoped_view_model_with_a_practice_can_be_scoped(self): + # The mixins used to skip scoping silently when the model's queryset had + # no for_current_practice, which let MarketingPeriod be edited across + # practices. They now raise instead; this catches it before a request does. + import my_practice.urls # noqa: F401 (imports every view module) + + offenders = [] + for base in (PracticeScopedListView, PracticeScopedUpdateView, PracticeScopedDeleteView): + for view in _subclasses(base): + model = getattr(view, "model", None) + if model is None: + continue + has_practice = any(f.name == "practice" for f in model._meta.get_fields()) + if has_practice and not hasattr( + model._default_manager.all(), "for_current_practice" + ): + offenders.append(f"{view.__name__} ({model.__name__})") + self.assertEqual(offenders, []) + + +class MarketingPeriodScopingTests(TestCase): + def setUp(self): + other = create_test_practice(name="Test GmbH", slug="test-gmbh") + self.period = MarketingPeriod.objects.create( + practice=other, start_date=date(2026, 1, 1), description="XX-1 test period" + ) + user = User.objects.create_user(username="member", password="testpass123") + link_user_to_practice(user, create_test_practice(name="Own Practice", slug="own")) + self.client.login(username="member", password="testpass123") + + def test_other_practices_period_cannot_be_viewed_or_edited(self): + url = reverse("marketing_period_edit", args=[self.period.pk]) + self.assertEqual(self.client.get(url).status_code, 404) + self.client.post(url, {"description": "changed", "start_date": "2026-01-01"}) + self.period.refresh_from_db() + self.assertEqual(self.period.description, "XX-1 test period") + + def test_other_practices_period_cannot_be_deleted(self): + url = reverse("marketing_period_delete", args=[self.period.pk]) + self.assertEqual(self.client.post(url).status_code, 404) + self.assertTrue(MarketingPeriod.objects.filter(pk=self.period.pk).exists()) diff --git a/app/my_practice/views/crud_mixins.py b/app/my_practice/views/crud_mixins.py index de6431b..6d4c63c 100644 --- a/app/my_practice/views/crud_mixins.py +++ b/app/my_practice/views/crud_mixins.py @@ -6,8 +6,10 @@ from django.contrib import messages from django.contrib.auth.mixins import LoginRequiredMixin +from django.core.exceptions import ImproperlyConfigured +from django.db.models import QuerySet from django.forms import ModelForm -from django.http import HttpResponse +from django.http import HttpRequest, HttpResponse from django.shortcuts import redirect from django.utils.translation import gettext as _ from django.views.generic import CreateView, DeleteView, ListView, UpdateView @@ -16,6 +18,23 @@ from ..utils.view_helpers import safe_next +def scope_to_current_practice(queryset: QuerySet, request: HttpRequest) -> QuerySet: + """Scope a view's queryset to the current practice, failing closed. + + A model with a practice FK but a plain manager used to fall through + unfiltered, so its edit/delete views resolved any practice's row by pk. + Such a model is now a configuration error; models without a practice + field (shared ones like TimeOff and ClientTag) pass through unchanged. + """ + if hasattr(queryset, "for_current_practice"): + return queryset.for_current_practice(request) + if any(field.name == "practice" for field in queryset.model._meta.get_fields()): + raise ImproperlyConfigured( + f"{queryset.model.__name__} has a practice field but no PracticeScopedManager" + ) + return queryset + + class PracticeOwnerRequiredMixin: """ Restricts an Update/DeleteView to users who own the object's practice. @@ -164,11 +183,7 @@ def get_queryset(self): """Get queryset scoped to current practice""" queryset = super().get_queryset() - # Apply practice scoping via manager - if hasattr(queryset, "for_current_practice"): - queryset = queryset.for_current_practice(self.request) - - return queryset + return scope_to_current_practice(queryset, self.request) class PracticeScopedCreateView(LoginRequiredMixin, CreateView): @@ -229,11 +244,7 @@ def get_queryset(self): """Get queryset scoped to current practice""" queryset = super().get_queryset() - # Apply practice scoping via manager - if hasattr(queryset, "for_current_practice"): - queryset = queryset.for_current_practice(self.request) - - return queryset + return scope_to_current_practice(queryset, self.request) def form_valid(self, form: ModelForm) -> HttpResponse: """Save with success message""" @@ -268,9 +279,7 @@ class ExpenseDeleteView(PracticeScopedDeleteView): def get_queryset(self): """Get queryset scoped to current practice""" queryset = super().get_queryset() - if hasattr(queryset, "for_current_practice"): - queryset = queryset.for_current_practice(self.request) - return queryset + return scope_to_current_practice(queryset, self.request) def form_valid(self, form: ModelForm) -> HttpResponse: """Delete with optional success message""" From bae0a282f5ce2a63e2812bae0f806eba6dad976f Mon Sep 17 00:00:00 2001 From: Daniel Holbach Date: Wed, 28 Oct 2026 12:17:47 +0100 Subject: [PATCH 5/8] fix(security): calendar token lookup with no practice means no practice get_active_token/get_service(None) returned the newest active token of any practice, and save_token(practice=None) deactivated every practice's token. A signed-in user without a practice could therefore read another practice's calendar, rewrite its calendar_id, or break its sync. None now selects only the practice-less fallback token, and the guardrail no longer exempts google_calendar.py. Co-Authored-By: Claude Opus 5.5 --- app/my_practice/tests/test_google_calendar.py | 37 ++++++++++++++++++- .../tests/test_practice_scoping.py | 3 +- app/my_practice/utils/google_calendar.py | 29 ++++++++------- 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/app/my_practice/tests/test_google_calendar.py b/app/my_practice/tests/test_google_calendar.py index 0ecec91..9c59879 100644 --- a/app/my_practice/tests/test_google_calendar.py +++ b/app/my_practice/tests/test_google_calendar.py @@ -96,12 +96,45 @@ def test_save_token_deactivates_old_tokens(self): mock_creds.expiry = None # Save new token - GoogleCalendarOAuth.save_token(mock_creds) + GoogleCalendarOAuth.save_token(mock_creds, practice=self.practice) # Verify old token deactivated old_token.refresh_from_db() self.assertFalse(old_token.is_active) + def _practice_token(self): + return GoogleCalendarToken.objects.create( + token="practice-token", + refresh_token="practice-refresh", + token_uri="https://oauth2.googleapis.com/token", + client_id="practice-client", + client_secret="practice-secret", + scopes=["https://www.googleapis.com/auth/calendar.readonly"], + is_active=True, + practice=self.practice, + ) + + def test_no_practice_does_not_pick_up_a_practice_token(self): + # A user without a practice passes None; that used to return any + # practice's token and so its calendar. + self._practice_token() + self.assertIsNone(GoogleCalendarOAuth.get_active_token(None)) + + def test_saving_a_practice_less_token_leaves_practice_tokens_active(self): + token = self._practice_token() + mock_creds = Mock( + token="new-token", + refresh_token="new-refresh", + token_uri="https://oauth2.googleapis.com/token", + client_id="new-client", + client_secret="new-secret", + scopes=["https://www.googleapis.com/auth/calendar.readonly"], + expiry=None, + ) + GoogleCalendarOAuth.save_token(mock_creds, practice=None) + token.refresh_from_db() + self.assertTrue(token.is_active) + @patch("my_practice.utils.google_calendar.build") def test_get_service_with_valid_token(self, mock_build): """Test getting calendar service with valid token.""" @@ -117,7 +150,7 @@ def test_get_service_with_valid_token(self, mock_build): practice=self.practice, ) - service = GoogleCalendarOAuth.get_service() + service = GoogleCalendarOAuth.get_service(practice=self.practice) # Verify service built self.assertIsNotNone(service) diff --git a/app/my_practice/tests/test_practice_scoping.py b/app/my_practice/tests/test_practice_scoping.py index 6078955..c8cecb8 100644 --- a/app/my_practice/tests/test_practice_scoping.py +++ b/app/my_practice/tests/test_practice_scoping.py @@ -54,8 +54,7 @@ } ) -# The calendar token lookup falls back to the practice-less token on purpose. -KNOWN_FAIL_OPEN_FILES = frozenset({"utils/google_calendar.py"}) +KNOWN_FAIL_OPEN_FILES: frozenset[str] = frozenset() # `if practice:` guarding a filter, or `{...: practice} if practice else {}` — # both hand out unfiltered data when practice is None. diff --git a/app/my_practice/utils/google_calendar.py b/app/my_practice/utils/google_calendar.py index c87b180..8031da7 100644 --- a/app/my_practice/utils/google_calendar.py +++ b/app/my_practice/utils/google_calendar.py @@ -70,11 +70,11 @@ def save_token(credentials, practice=None) -> GoogleCalendarToken: Returns: Created GoogleCalendarToken instance """ - # Deactivate old tokens for this practice (or all if practice is None) - qs = GoogleCalendarToken.objects.filter(is_active=True) - if practice is not None: - qs = qs.filter(practice=practice) - qs.update(is_active=False) + # Deactivate old tokens for this practice. practice=None is the + # practice-less fallback token, never "every practice's tokens". + GoogleCalendarToken.objects.filter(is_active=True, practice=practice).update( + is_active=False + ) # Convert expiry to timezone-aware datetime if present expires_at = None @@ -105,16 +105,19 @@ def get_active_token(practice=None) -> "GoogleCalendarToken | None": practice-less fallback token in single-practice setups). Args: - practice: Optional Practice instance; when provided only that - practice's token is considered. + practice: Practice instance, or None for the practice-less + fallback token only. None used to match any practice's + token, which handed a user without a practice another + practice's calendar. Returns: The most recently created active token, or None if none exists. """ - token_qs = GoogleCalendarToken.objects.filter(is_active=True) - if practice is not None: - token_qs = token_qs.filter(practice=practice) - return token_qs.order_by("-created_at").first() + return ( + GoogleCalendarToken.objects.filter(is_active=True, practice=practice) + .order_by("-created_at") + .first() + ) @staticmethod def get_service(practice=None): @@ -123,8 +126,8 @@ def get_service(practice=None): Automatically refreshes expired tokens. Args: - practice: Optional Practice instance; when provided only that - practice's token is used. + practice: Practice instance, or None for the practice-less + fallback token. Returns: Google Calendar API service object or None if no valid token exists From aa661f956b6c3789131006b124d23cbf85f4403b Mon Sep 17 00:00:00 2001 From: Daniel Holbach Date: Wed, 28 Oct 2026 12:20:32 +0100 Subject: [PATCH 6/8] fix(security): match calendar events only against the practice's clients CalendarEventParser fell back to every client of every practice, and none of the callers passed a scoped list. A client code in practice A's calendar (anyone can send an invite) therefore matched practice B's client: the fetch command booked sessions onto it, and the approval queue showed it with B's draft invoices. - clients is now a required argument; callers pass practice_clients(). - Importing, the quick action and the billing and approval views ignore a stored match that belongs to another practice. Co-Authored-By: Claude Opus 5.5 --- .../commands/fetch_calendar_events.py | 3 +- app/my_practice/tests/test_google_calendar.py | 72 ++++++++++++++----- .../utils/calendar_event_processor.py | 6 +- .../utils/calendar_import_helpers.py | 7 +- app/my_practice/utils/google_calendar.py | 29 ++++---- app/my_practice/views/billing_views.py | 10 ++- app/my_practice/views/calendar_views.py | 6 +- 7 files changed, 94 insertions(+), 39 deletions(-) diff --git a/app/my_practice/management/commands/fetch_calendar_events.py b/app/my_practice/management/commands/fetch_calendar_events.py index 928f696..a741004 100644 --- a/app/my_practice/management/commands/fetch_calendar_events.py +++ b/app/my_practice/management/commands/fetch_calendar_events.py @@ -17,6 +17,7 @@ from ...utils.google_calendar import ( CalendarEventParser, GoogleCalendarOAuth, + practice_clients, ) from ...utils.tag_helpers import sync_no_next_session_tag @@ -126,7 +127,7 @@ def _fetch_for_practice(self, practice, calendar_id, dry_run, forced_days, futur return live_ids = {e.get("id") for e in raw_events if e.get("id")} - parsed = CalendarEventParser.parse_events(raw_events) + parsed = CalendarEventParser.parse_events(raw_events, practice_clients(practice)) # Clients whose Session rows changed in this run — their # no-next-session tag is re-synced at the end instead of waiting diff --git a/app/my_practice/tests/test_google_calendar.py b/app/my_practice/tests/test_google_calendar.py index 9c59879..41562cc 100644 --- a/app/my_practice/tests/test_google_calendar.py +++ b/app/my_practice/tests/test_google_calendar.py @@ -18,10 +18,12 @@ ServiceType, Session, ) +from my_practice.utils.calendar_import_helpers import _resolve_client from my_practice.utils.google_calendar import ( CalendarEventParser, GoogleCalendarOAuth, list_calendars, + practice_clients, ) @@ -264,7 +266,7 @@ def test_match_client_exact_match(self): practice=self.practice, ) - result = CalendarEventParser.match_client("JD 60min session") + result = CalendarEventParser.match_client("JD 60min session", Client.objects.all()) self.assertEqual(result, client) def test_match_client_case_insensitive(self): @@ -276,7 +278,7 @@ def test_match_client_case_insensitive(self): practice=self.practice, ) - result = CalendarEventParser.match_client("js therapy session") + result = CalendarEventParser.match_client("js therapy session", Client.objects.all()) self.assertEqual(result, client) def test_match_client_no_match(self): @@ -288,7 +290,7 @@ def test_match_client_no_match(self): practice=self.practice, ) - result = CalendarEventParser.match_client("Unknown client XX") + result = CalendarEventParser.match_client("Unknown client XX", Client.objects.all()) self.assertIsNone(result) def test_match_client_empty_code(self): @@ -300,7 +302,7 @@ def test_match_client_empty_code(self): practice=self.practice, ) - result = CalendarEventParser.match_client("AJ session") + result = CalendarEventParser.match_client("AJ session", Client.objects.all()) self.assertIsNone(result) def _make_clients(self, *codes): @@ -325,7 +327,7 @@ def test_match_client_ignores_code_inside_a_word(self): for summary in ("Nat", "CAR", "LBW"): with self.subTest(summary=summary): - self.assertIsNone(CalendarEventParser.match_client(summary)) + self.assertIsNone(CalendarEventParser.match_client(summary, Client.objects.all())) def test_match_client_ignores_code_inside_a_german_or_status_word(self): """Ordinary calendar words must not match a client code.""" @@ -333,27 +335,37 @@ def test_match_client_ignores_code_inside_a_german_or_status_word(self): for summary in ("Frei", "Mail", "Blocked", "cancel"): with self.subTest(summary=summary): - self.assertIsNone(CalendarEventParser.match_client(summary)) + self.assertIsNone(CalendarEventParser.match_client(summary, Client.objects.all())) def test_match_client_prefers_the_longest_code(self): """A summary equal to a longer code matches that code, not its prefix.""" clients = self._make_clients("TO", "TOB") - self.assertEqual(CalendarEventParser.match_client("TOB"), clients["TOB"]) - self.assertEqual(CalendarEventParser.match_client("TO"), clients["TO"]) + self.assertEqual( + CalendarEventParser.match_client("TOB", Client.objects.all()), clients["TOB"] + ) + self.assertEqual( + CalendarEventParser.match_client("TO", Client.objects.all()), clients["TO"] + ) def test_match_client_hyphen_is_part_of_the_code(self): """A hyphen does not delimit a code, so MM does not match inside MM-G.""" clients = self._make_clients("MM", "MM-G") - self.assertEqual(CalendarEventParser.match_client("MM-G 60min"), clients["MM-G"]) - self.assertEqual(CalendarEventParser.match_client("MM 60min"), clients["MM"]) + self.assertEqual( + CalendarEventParser.match_client("MM-G 60min", Client.objects.all()), clients["MM-G"] + ) + self.assertEqual( + CalendarEventParser.match_client("MM 60min", Client.objects.all()), clients["MM"] + ) def test_match_client_with_cancel_suffix(self): """A cancelled session still matches its own code, not one inside 'cancel'.""" clients = self._make_clients("ZK", "EL") - self.assertEqual(CalendarEventParser.match_client("ZK (cancel)"), clients["ZK"]) + self.assertEqual( + CalendarEventParser.match_client("ZK (cancel)", Client.objects.all()), clients["ZK"] + ) def test_is_cancelled_with_parentheses(self): """Test cancelled detection with (cancel) format.""" @@ -382,7 +394,7 @@ def test_parse_event_complete(self): "end": {"dateTime": "2025-12-23T15:00:00Z"}, } - result = CalendarEventParser.parse_event(event) + result = CalendarEventParser.parse_event(event, Client.objects.all()) self.assertEqual(result["summary"], "TC 60min therapy") self.assertIsNotNone(result["start"]) @@ -400,7 +412,7 @@ def test_parse_event_cancelled(self): "end": {"dateTime": "2025-12-23T15:00:00Z"}, } - result = CalendarEventParser.parse_event(event) + result = CalendarEventParser.parse_event(event, Client.objects.all()) self.assertTrue(result["is_cancelled"]) self.assertIsNone(result["matched_client"]) @@ -432,7 +444,7 @@ def test_parse_events_multiple(self): }, ] - results = CalendarEventParser.parse_events(events) + results = CalendarEventParser.parse_events(events, Client.objects.all()) self.assertEqual(len(results), 2) self.assertEqual(results[0]["matched_client"], client1) @@ -502,7 +514,7 @@ def test_parse_event_includes_service_type_suggestion(self): "end": {"dateTime": "2024-01-15T15:00:00+01:00"}, # 60 minutes } - result = CalendarEventParser.parse_event(event) + result = CalendarEventParser.parse_event(event, Client.objects.all()) self.assertIsNotNone(result["suggested_service_type_obj"]) self.assertEqual(result["suggested_service_type_obj"].code, "therapy_60") @@ -516,7 +528,7 @@ def test_parse_event_cancelled_overrides_service_type(self): "end": {"dateTime": "2024-01-15T15:00:00+01:00"}, # 60 minutes } - result = CalendarEventParser.parse_event(event) + result = CalendarEventParser.parse_event(event, Client.objects.all()) self.assertTrue(result["is_cancelled"]) self.assertIsNotNone(result["suggested_service_type_obj"]) @@ -796,3 +808,31 @@ def test_stale_event_cancellation_collects_client(self): self.assertIn(self.client_obj, affected) session.refresh_from_db() self.assertTrue(session.cancelled) + + +class PracticeScopedMatchingTests(TestCase): + """A practice's calendar only ever matches that practice's clients.""" + + def setUp(self): + self.own = Practice.objects.create(name="Own Practice", slug="own-calendar", title="T") + other = Practice.objects.create(name="Test GmbH", slug="other-calendar", title="T") + self.foreign = Client.objects.create( + client_code="YY", full_name="Anna Schmidt", practice=other + ) + + def test_other_practices_client_code_does_not_match(self): + event = { + "id": "evt-1", + "summary": "YY 60min", + "start": {"dateTime": "2026-10-01T10:00:00+02:00"}, + "end": {"dateTime": "2026-10-01T11:00:00+02:00"}, + } + parsed = CalendarEventParser.parse_events([event], practice_clients(self.own)) + self.assertIsNone(parsed[0]["matched_client"]) + + def test_stored_foreign_match_is_not_imported(self): + client, error = _resolve_client( + {"matched_client": self.foreign, "summary": "YY"}, {}, self.own + ) + self.assertIsNone(client) + self.assertIsNotNone(error) diff --git a/app/my_practice/utils/calendar_event_processor.py b/app/my_practice/utils/calendar_event_processor.py index ee78001..7734c06 100644 --- a/app/my_practice/utils/calendar_event_processor.py +++ b/app/my_practice/utils/calendar_event_processor.py @@ -11,7 +11,7 @@ from django.utils import timezone from ..models import Client, InvoiceItem, PendingCalendarEvent, ServiceType -from .google_calendar import CalendarEventParser +from .google_calendar import CalendarEventParser, practice_clients logger = logging.getLogger(__name__) @@ -41,7 +41,7 @@ def __init__(self, request) -> None: def fetch_and_parse(self, service, calendar_id: str, start_date, end_date) -> list[dict]: """Fetch all events from a calendar (paginated) and parse them.""" raw = self._paginate(service, calendar_id, start_date, end_date) - return CalendarEventParser.parse_events(raw) + return CalendarEventParser.parse_events(raw, practice_clients(self.practice)) def _paginate(self, service, calendar_id: str, start_date, end_date) -> list: events = [] @@ -184,7 +184,7 @@ def fetch_specific_events(self, service, calendar_id: str, event_ids: list[str]) fetched.append(event) except Exception: logger.warning("Could not fetch calendar event %s; skipping", event_id) - return CalendarEventParser.parse_events(fetched) + return CalendarEventParser.parse_events(fetched, practice_clients(self.practice)) def _ensure_aware(dt): diff --git a/app/my_practice/utils/calendar_import_helpers.py b/app/my_practice/utils/calendar_import_helpers.py index d8c9166..b728b2e 100644 --- a/app/my_practice/utils/calendar_import_helpers.py +++ b/app/my_practice/utils/calendar_import_helpers.py @@ -28,8 +28,11 @@ def _resolve_client(event: dict, overrides: dict, practice) -> tuple[Client | No "id": client_id, "summary": event.get("summary"), } - if event.get("matched_client"): - return event["matched_client"], None + matched = event.get("matched_client") + # A stored match from before matching was scoped may name another + # practice's client; never write sessions or invoice items onto it. + if matched and practice is not None and matched.practice_id == practice.id: + return matched, None return None, _("No client for event: %(summary)s") % {"summary": event.get("summary")} diff --git a/app/my_practice/utils/google_calendar.py b/app/my_practice/utils/google_calendar.py index 8031da7..e84a252 100644 --- a/app/my_practice/utils/google_calendar.py +++ b/app/my_practice/utils/google_calendar.py @@ -8,6 +8,7 @@ from typing import cast from django.conf import settings +from django.db.models import QuerySet from django.utils import timezone from google.oauth2.credentials import Credentials from google_auth_oauthlib.flow import Flow @@ -266,7 +267,7 @@ def calculate_duration(start_dt: datetime | None, end_dt: datetime | None) -> in return 0 @staticmethod - def match_client(summary: str, clients=None) -> "Client | None": + def match_client(summary: str, clients) -> "Client | None": """ Match client by client_code appearing as a standalone token in the summary. @@ -280,15 +281,13 @@ def match_client(summary: str, clients=None) -> "Client | None": Args: summary: Event title/summary - clients: List/QuerySet of clients (fetched if not provided) + clients: The importing practice's clients. Required: defaulting to + every client let one practice's calendar match (and book + sessions onto) another practice's clients. Returns: Matched Client instance or None """ - if clients is None: - # Use optimized query - only fetch needed fields - clients = Client.objects.only("id", "client_code") - candidates = sorted( (c for c in clients if c.client_code), key=lambda c: len(c.client_code), @@ -331,13 +330,13 @@ def is_blocked(summary: str) -> bool: return "blocked" in summary.lower() @classmethod - def parse_event(cls, event: dict, clients=None) -> dict: + def parse_event(cls, event: dict, clients) -> dict: """ Parse a Google Calendar event into structured data. Args: event: Raw event dict from Google Calendar API - clients: Optional QuerySet of clients for matching + clients: The importing practice's clients, for matching Returns: Dict with parsed event data: @@ -388,21 +387,18 @@ def parse_event(cls, event: dict, clients=None) -> dict: } @classmethod - def parse_events(cls, events: list, clients=None) -> list: + def parse_events(cls, events: list, clients) -> list: """ Parse multiple Google Calendar events. Args: events: List of raw event dicts from Google Calendar API - clients: Optional QuerySet of clients (fetched once if not provided) + clients: The importing practice's clients, for matching Returns: List of parsed event dicts """ - if clients is None: - # Use optimized query - only fetch needed fields - clients = Client.objects.only("id", "client_code") - + clients = list(clients) # evaluated once, not per event return [ cls.parse_event(event, clients) for event in events @@ -430,3 +426,8 @@ def list_calendars(service) -> list[dict]: } for entry in calendar_list.get("items", []) ] + + +def practice_clients(practice) -> "QuerySet[Client]": + """The clients a practice's calendar events may be matched against.""" + return Client.objects.for_practice(practice).only("id", "client_code") diff --git a/app/my_practice/views/billing_views.py b/app/my_practice/views/billing_views.py index 490b39b..6224d1c 100644 --- a/app/my_practice/views/billing_views.py +++ b/app/my_practice/views/billing_views.py @@ -300,7 +300,11 @@ def billing_open_overview(request): | set(pending_by_client) | set(cancelled_billed_by_client) ) - clients = Client.objects.filter(pk__in=all_client_ids).order_by("client_code") + clients = ( + Client.objects.for_practice(practice) + .filter(pk__in=all_client_ids) + .order_by("client_code") + ) rows = _build_client_rows( clients, invoices_by_client, @@ -384,7 +388,9 @@ def monthly_billing_overview(request, month): | set(pending_by_client) | set(cancelled_billed_by_client) ) - clients = Client.objects.filter(pk__in=all_client_ids).order_by("client_code") + clients = ( + Client.objects.for_practice(practice).filter(pk__in=all_client_ids).order_by("client_code") + ) rows = _build_client_rows( clients, invoices_by_client, diff --git a/app/my_practice/views/calendar_views.py b/app/my_practice/views/calendar_views.py index 74fd637..3cb792a 100644 --- a/app/my_practice/views/calendar_views.py +++ b/app/my_practice/views/calendar_views.py @@ -317,7 +317,8 @@ def calendar_approval_queue(request: HttpRequest) -> HttpResponse: # This avoids N+1 queries and lets the template show useful info in the dropdown. client_ids = [g["client"].pk for g in grouped if g["client"] is not None] draft_invoices = ( - Invoice.objects.filter(client_id__in=client_ids, status="draft") + Invoice.objects.for_practice(practice) + .filter(client_id__in=client_ids, status="draft") .annotate(item_count=Count("items")) .order_by("-invoice_date") ) @@ -609,6 +610,9 @@ def calendar_event_quick_action(request: HttpRequest, pk: int) -> HttpResponse: return redirect("client_list") client = event.matched_client + # A match stored before matching was scoped may name another practice's client. + if client and client.practice_id != event.practice_id: + client = None redirect_target = ( redirect("client_detail", pk=client.pk) if client else redirect("calendar_approval_queue") ) From a7f6fc06f108efea3b69020ff92dfd835c2d8a09 Mon Sep 17 00:00:00 2001 From: Daniel Holbach Date: Wed, 28 Oct 2026 12:22:05 +0100 Subject: [PATCH 7/8] fix(security): key pending calendar events per practice PendingCalendarEvent.google_event_id was unique across all practices and fetch_calendar_events looked rows up by event id alone. When two practices' calendars returned the same event id, the second practice's sync got the first practice's row back and its cancel/reschedule transitions rewrote that practice's Session. The key is now (practice, google_event_id), and every lookup in the command includes the practice. Co-Authored-By: Claude Opus 5.5 --- .../commands/fetch_calendar_events.py | 28 +++++++++++-------- ...endingcalendarevent_unique_per_practice.py | 22 +++++++++++++++ app/my_practice/models/calendar.py | 10 +++++-- .../tests/test_fetch_calendar_events.py | 26 +++++++++++++++++ 4 files changed, 73 insertions(+), 13 deletions(-) create mode 100644 app/my_practice/migrations/0009_pendingcalendarevent_unique_per_practice.py diff --git a/app/my_practice/management/commands/fetch_calendar_events.py b/app/my_practice/management/commands/fetch_calendar_events.py index a741004..6c050f0 100644 --- a/app/my_practice/management/commands/fetch_calendar_events.py +++ b/app/my_practice/management/commands/fetch_calendar_events.py @@ -1,8 +1,8 @@ """ Management command to fetch Google Calendar events into the pending queue (P-013). -Run every few hours via systemd timer. Idempotent: google_event_id unique constraint -prevents duplicates. On each run, also marks previously-fetched events as 'cancelled' +Run every few hours via systemd timer. Idempotent: the (practice, google_event_id) +unique constraint prevents duplicates. On each run, also marks previously-fetched events as 'cancelled' if they no longer appear in Google Calendar — unless the same session simply moved to a new Google event id, which _supersede_moved_events() re-binds first. """ @@ -13,7 +13,7 @@ from django.core.management.base import BaseCommand from django.utils import timezone -from ...models import GoogleCalendarToken, PendingCalendarEvent, Session +from ...models import GoogleCalendarToken, PendingCalendarEvent, Practice, Session from ...utils.google_calendar import ( CalendarEventParser, GoogleCalendarOAuth, @@ -267,7 +267,7 @@ def _supersede_moved_events( Returns the number of rows re-bound. """ - replacements = self._collect_replacement_candidates(parsed_events, live_ids) + replacements = self._collect_replacement_candidates(parsed_events, live_ids, practice) if not replacements: return 0 @@ -288,13 +288,15 @@ def _supersede_moved_events( return moved @classmethod - def _collect_replacement_candidates(cls, parsed_events: list, live_ids: set) -> dict: + def _collect_replacement_candidates( + cls, parsed_events: list, live_ids: set, practice: "Practice" + ) -> dict: """Group live events that have no PendingCalendarEvent row yet by (client_id, date) — the pool a stale row can be re-bound to.""" known_ids = set( - PendingCalendarEvent.objects.filter(google_event_id__in=live_ids).values_list( - "google_event_id", flat=True - ) + PendingCalendarEvent.objects.filter( + practice=practice, google_event_id__in=live_ids + ).values_list("google_event_id", flat=True) ) candidates: dict = {} for event in parsed_events: @@ -402,9 +404,9 @@ def _upsert_event( return "created" obj, was_created = PendingCalendarEvent.objects.get_or_create( + practice=practice, google_event_id=event_id, defaults={ - "practice": practice, "summary": event.get("summary", ""), "event_date": event_date, "event_time": event_time, @@ -455,7 +457,7 @@ def _transition_to_cancelled( obj: "PendingCalendarEvent", event_id: str, affected_clients: set | None ) -> None: PendingCalendarEvent.objects.filter( - google_event_id=event_id, + pk=obj.pk, status=PendingCalendarEvent.Status.PENDING, ).update(status=PendingCalendarEvent.Status.CANCELLED) if obj.session_id: @@ -638,5 +640,9 @@ def _auto_create_session(self, event, event_id, event_date, event_time, status): }, ) if not PendingCalendarEvent.objects.filter(session=session).exists(): - PendingCalendarEvent.objects.filter(google_event_id=event_id).update(session=session) + # matched_client comes from the practice's own clients, so its + # practice is the one whose row this is. + PendingCalendarEvent.objects.filter( + practice_id=matched_client.practice_id, google_event_id=event_id + ).update(session=session) return matched_client diff --git a/app/my_practice/migrations/0009_pendingcalendarevent_unique_per_practice.py b/app/my_practice/migrations/0009_pendingcalendarevent_unique_per_practice.py new file mode 100644 index 0000000..adb1c9c --- /dev/null +++ b/app/my_practice/migrations/0009_pendingcalendarevent_unique_per_practice.py @@ -0,0 +1,22 @@ +# Generated by Django 6.1.2 on 2026-10-28 11:21 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('my_practice', '0008_inquiry_lapsed_status_anonymised_at'), + ] + + operations = [ + migrations.AlterField( + model_name='pendingcalendarevent', + name='google_event_id', + field=models.CharField(max_length=255, verbose_name='Google Event ID'), + ), + migrations.AddConstraint( + model_name='pendingcalendarevent', + constraint=models.UniqueConstraint(fields=('practice', 'google_event_id'), name='pce_practice_event_uniq'), + ), + ] diff --git a/app/my_practice/models/calendar.py b/app/my_practice/models/calendar.py index 7b1e98d..22b0ad7 100644 --- a/app/my_practice/models/calendar.py +++ b/app/my_practice/models/calendar.py @@ -84,7 +84,7 @@ class PendingCalendarEvent(models.Model): command and held here until approved/imported to an invoice, skipped, or detected as cancelled in Google Calendar. - google_event_id is unique so repeated fetches are idempotent. + (practice, google_event_id) is unique so repeated fetches are idempotent. """ class Status(StrEnum): @@ -108,9 +108,10 @@ class Status(StrEnum): related_name="pending_calendar_events", verbose_name=_("Practice"), ) + # Unique per practice, not globally: a global key let one practice's sync + # pick up — and cancel or move — another practice's row for the same event. google_event_id = models.CharField( max_length=255, - unique=True, verbose_name=_("Google Event ID"), ) summary = models.CharField(max_length=500, verbose_name=_("Summary")) @@ -163,6 +164,11 @@ class Meta: models.Index(fields=["practice", "status"], name="pce_practice_status_idx"), models.Index(fields=["matched_client", "event_date"], name="pce_client_date_idx"), ] + constraints = [ + models.UniqueConstraint( + fields=["practice", "google_event_id"], name="pce_practice_event_uniq" + ), + ] def __str__(self) -> str: client = self.matched_client.client_code if self.matched_client else "?" diff --git a/app/my_practice/tests/test_fetch_calendar_events.py b/app/my_practice/tests/test_fetch_calendar_events.py index 882896b..c3c67d7 100644 --- a/app/my_practice/tests/test_fetch_calendar_events.py +++ b/app/my_practice/tests/test_fetch_calendar_events.py @@ -902,3 +902,29 @@ def test_dry_run_reports_would_create(self): call_command("fetch_calendar_events", "--dry-run", stdout=out) self.assertIn("[dry-run] Would create", out.getvalue()) self.assertFalse(PendingCalendarEvent.objects.exists()) + + +class EventIdPerPracticeTest(UpsertEventTestBase): + """The same Google event id in two practices' calendars gives two rows.""" + + def setUp(self): + super().setUp() + self.other = Practice.objects.create(name="Test GmbH", slug="upsert-other", title="T") + self.cmd._upsert_event(self._event(), self.practice, dry_run=False) + self.own_row = PendingCalendarEvent.objects.get(practice=self.practice) + self.session = Session.objects.get(client=self.test_client) + + def test_other_practice_gets_its_own_row(self): + result = self.cmd._upsert_event(self._event(matched_client=None), self.other, dry_run=False) + self.assertEqual(result, "created") + self.assertEqual(PendingCalendarEvent.objects.filter(google_event_id="evt-1").count(), 2) + + def test_other_practices_cancellation_leaves_this_practice_alone(self): + self.cmd._upsert_event(self._event(matched_client=None), self.other, dry_run=False) + self.cmd._upsert_event( + self._event(matched_client=None, is_cancelled=True), self.other, dry_run=False + ) + self.own_row.refresh_from_db() + self.session.refresh_from_db() + self.assertEqual(self.own_row.status, PendingCalendarEvent.Status.PENDING) + self.assertFalse(self.session.cancelled) From 6d06d3c66a221b6f1f3001c68540c63fb83c8f51 Mon Sep 17 00:00:00 2001 From: Daniel Holbach Date: Wed, 28 Oct 2026 12:23:37 +0100 Subject: [PATCH 8/8] docs: changelog and recent activity for the security review fixes Co-Authored-By: Claude Opus 5.5 --- PROJECTS.md | 2 +- docs/CHANGELOG.md | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/PROJECTS.md b/PROJECTS.md index adee2fa..512df8f 100644 --- a/PROJECTS.md +++ b/PROJECTS.md @@ -31,8 +31,8 @@ grep -rho "P-[0-9]\{3\}" --include=*.md --include=*.py --include=*.html \ ## 🔍 Recent Activity +- **2026-10-28 — Security review fixes**: source review of views, settings and calendar import; dev stack bound to loopback, login-throttle NFKC bypass closed, GDPR erasure made complete, four latent practice-isolation gaps closed. Details in [docs/CHANGELOG.md](docs/CHANGELOG.md) (Unreleased). - **2026-10-27 — v0.9.0 released**: inquiry lapse/anonymise and charts (#533, #534), same-day session crash fix (#537), systemd timer fixes (#535), Pillow 13 / python-dateutil 3.0. Highlights in [docs/CHANGELOG.md](docs/CHANGELOG.md). -- **2026-10-21 — Inquiry charts, lapse + anonymise**: `/inquiries` gets a monthly chart with marketing periods drawn as lanes (generic `lanes` option on `MPChart.bar`) and an open-inquiry ageing list (PR #533). Follow-up adds a *Lapsed* status and `manage.py tidy_inquiries` (lapse after 60 days without a step, anonymise closed non-client inquiries 180 days after last contact); not on the scheduled-jobs timer yet — the retention period is still to be confirmed with the Verband. > Ältere Einträge: [docs/CHANGELOG.md](docs/CHANGELOG.md) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index e0f806b..c4f2d4f 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -2,6 +2,17 @@ Major features and milestones in chronological order. +## Unreleased + +Security fixes from a source review of the views, settings and calendar import. + +**Upgrading:** one migration runs on start (`0009`, makes the pending-calendar-event key unique per practice instead of globally; existing rows already satisfy it). Restart the dev stack (`./dev.py restart`) so gunicorn rebinds to loopback. + +- **Security — dev stack no longer listens on the local network**: `docker-compose.yml` runs django with `network_mode: host` and bound gunicorn to `0.0.0.0:8000` with DEBUG on, so the stack holding the real data directories was reachable from other machines over plain HTTP. It binds `127.0.0.1` now, like Postgres and the prod default. +- **Security — login lockout could be sidestepped**: the throttle key was not NFKC-normalised while Django's login form is, so fullwidth and other compatibility spellings of a username each got a fresh 5-attempt allowance. +- **Fix — GDPR erasure left personal data behind**: the inquiry a client was converted from and their imported calendar events survived the Art. 17 deletion; they are now anonymised or deleted. GebüH service records no longer make the erasure roll back, and the notification email goes out only after the deletion has committed. +- **Security — practice isolation (latent while single-practice)**: the scoped CRUD views fail closed for a model without the practice-scoped manager (MarketingPeriod could be edited and deleted across practices); calendar events match only the importing practice's clients; a user without a practice no longer gets another practice's Google Calendar token; pending calendar events are keyed per practice. + ## 2026-10-27 — v0.9.0 minor release Inquiry outcomes you can trust, a crash fix for same-day sessions, and scheduled jobs that no longer fire before the app is up.