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/app/my_practice/management/commands/fetch_calendar_events.py b/app/my_practice/management/commands/fetch_calendar_events.py index 928f696..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,10 +13,11 @@ 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, + 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 @@ -266,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 @@ -287,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: @@ -401,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, @@ -454,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: @@ -637,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/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_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) diff --git a/app/my_practice/tests/test_google_calendar.py b/app/my_practice/tests/test_google_calendar.py index 0ecec91..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, ) @@ -96,12 +98,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 +152,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) @@ -231,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): @@ -243,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): @@ -255,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): @@ -267,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): @@ -292,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.""" @@ -300,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.""" @@ -349,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"]) @@ -367,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"]) @@ -399,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) @@ -469,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") @@ -483,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"]) @@ -763,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/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/tests/test_practice_scoping.py b/app/my_practice/tests/test_practice_scoping.py index a20b7b3..c8cecb8 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" @@ -47,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. @@ -193,3 +199,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/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/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/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 c87b180..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 @@ -70,11 +71,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 +106,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 +127,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 @@ -263,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. @@ -277,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), @@ -328,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: @@ -385,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 @@ -427,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/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() 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") ) 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) 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""" 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: 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.