diff --git a/PLATFORM_ROADMAP.md b/PLATFORM_ROADMAP.md index 127d832..66c1e1e 100644 --- a/PLATFORM_ROADMAP.md +++ b/PLATFORM_ROADMAP.md @@ -1785,3 +1785,26 @@ Der AI Assistent ist ein paralleles System das die Kommunikation-Plattform dupli - Suchergebnis verlinkt direkt auf Seite + Sprungmarke **Abhängigkeiten:** P4 benötigt M1 (Universal-Registry). P1-P3, P5 unabhängig startbar. + +## Phase Q — Frontend-Plugin-Architektur vollenden (geplant, 2026-09-13 aus externem Audit abgeleitet) + +**Ziel:** Die letzten verbliebenen Plugin-Grenzverletzungen im Frontend beseitigen — ein Plugin soll sein Backend, Manifest UND React-Seite liefern können, ohne dass zentrale Frontend-Dateien angefasst werden müssen. Basis: externes Architektur-Audit (2026-09-13), dessen Backend-Punkte bereits gefixt sind (siehe PROGRESS.md „Externer Architektur-Audit"); die vier Frontend-Punkte sind bewusst als eigene Phase geplant, weil sie ein durchdachtes Build-Time-Discovery-Konzept erfordern (Vite kann dynamische Imports zur Laufzeit im Production-Bundle nicht zuverlässig auflösen). + +### Q1 — Statische Plugin-Routen aus routes/index.tsx entfernen (Doppel-Architektur) +- Status quo: `/calendar`, `/dms`, `/mail`, `/reports`, `/tasks`, `/communication`, `/workflows`, `/import-export`, `/wiki`, `/agents`, `/automation` sind statisch im zentralen Router eingetragen UND kommen gleichzeitig über die Plugin-Manifeste via PluginRouteRenderer. +- Ziel: Nur noch PluginRouteRenderer bedient Plugin-Seiten; statische Einträge nur für echte Core-Seiten (Dashboard, Settings-Shell, Login, Trash, Approvals bis Core-Migration). +- Risiko: Manifest-Routen müssen Permissions, Layout-Einbindung (AppShell-Children vs. eigenständig) und Ladezustände 1:1 abbilden. + +### Q2 — Statische Settings-Routen ausdünnen +- Status quo: settings/roles, users, groups, mail, notifications, ai, ai-proactive, automation, documents sind statisch UND via settings_pages der Manifeste vorhanden. +- Ziel: settings_pages (Manifest) wird einzige Wahrheit für Plugin-Settings-Seiten; statische Einträge nur für Core-Settings (theme, system, backup, webhooks, menu, workspaces). + +### Q3 — STATIC_COMPONENT_MAP ersetzen durch Build-Time-Discovery +- Status quo: PluginLoader.tsx hält eine zentrale Komponenten-Liste (~26 Einträge). Ein neues Plugin muss die Leo-Frontend-Codebasis anfassen. +- Ziel: Build-Skript scannt app/plugins/builtins/*/plugin.py auf FrontendPageRoute/SettingsPage/Component-Pfade und generiert automatisch eine Import-Map (generated, committet), die Vite statisch chunken kann. Keine manuelle Zentral-Liste mehr. + +### Q4 — widgetRegistry in MiniAppHost durch generierte Map ersetzen +- Status quo: 11 Widget-Komponenten sind zentral hardcodiert (RecentContactsWidget, TasksSummaryWidget, ...). +- Ziel: Q3-Mechanismus deckt auch dashboard_widgets/miniapps component-Pfade ab; MiniAppHost nutzt dieselbe generierte Import-Map. + +**Reihenfolge:** Q3 zuerst (löst den Mechanismus), dann Q1/Q2 (Routen auf generierte Map umstellen), dann Q4. Jeder Schritt mit Vitest-Sicherung der betroffenen Seiten und Production-Build-Verifikation (Chunk-Existenz prüfen). diff --git a/PROGRESS.md b/PROGRESS.md index b2b1b3f..07c188e 100644 --- a/PROGRESS.md +++ b/PROGRESS.md @@ -1,10 +1,33 @@ # LeoPlatform — Fortschritts-Tracking -## Weitermachen (2026-08-30, für das nächste Modell) +## Externer Architektur-Audit — 13 Backend-Fixes verifiziert & umgesetzt (2026-09-13) ✅ + +**Ausgangslage:** Externes KI-Audit (leocrm-full.zip, Stand 86cea5d) meldete 17 Findings. Verifikation gegen den echten Code: **alle 17 BESTÄTIGT** (inkl. exakt der 12 gemeldeten fehlenden Permission-Keys — per AST-Scan 1:1 reproduziert). 13 Backend-/Lifecycle-Punkte sofort gefixt; die 4 Frontend-Plugin-Architektur-Punkte sind als **Phase Q** in die Roadmap eingeplant (Begründung dort). + +**Fixes (alle mit Live-Verifikation, `tests/test_audit_architecture_fixes.py` 17/17):** + +| # | Finding | Fix | Verifikation | +|---|---|---|---| +| P1 | `GET /workspaces` lieferte `modules: []` → Workspace-Editor überschrieb Konfig | `list_workspaces()` lädt Module+User-Counts gebündelt (2 Queries statt N+1) | test_f1: modules mit is_visible-Flags 1:1 | +| P1 | `/plugins/active-manifests` ignorierte Tenant-Deaktivierung (UI zeigte 403-Menüs) | Registry/Service/Route tragen `tenant_id` durch, filtern `tenant_plugin_activation.is_active=false` | test_f2: Plugin im Manifest ohne Filter, gefiltert mit Tenant-Eintrag | +| P1 | `uninstall()` umging PluginService-Cleanup (stale Permissions/Entity-Models) | `uninstall_plugin()` ruft `deactivate_plugin()` VOR `registry.uninstall()` | test_f3: Quellcode-Verifikation + Lifecycle-Verhalten | +| P1 | Contract-Lazy-Loading kannte DB-Aktivstatus nicht (Restart-Edge-Case) | Startup markiert `active=False`-Plugins (`mark_db_inactive`), Guard in `get_contract()`, Re-Activate cleart | test_f4: fail-closed + reopen | +| P1/P2 | `register_field_definitions()` ohne Unregister, nicht im Runtime-Lifecycle | `unregister_field_definitions()` + Aktivierung/Deaktivierung registrieren/entfernen Field-Defs | test_f5: voller Lifecycle über PluginService | +| P1/P2 | 39 Contact-Felddefinitionen lagen im Core (`CORE_FIELD_DEFINITIONS`) | Verschoben ins ContactsPlugin-Manifest (`field_definitions=`); Core behält nur users-Felder; `sensitive_data.py` nutzt jetzt die Registry-Gesamtsicht | test_f10: Core ohne contacts-Module, Plugin mit 39 Defs, Sensitivities erhalten | +| P1 | 12 verwendete Permission-Keys nicht registriert | 9 in CORE_PERMISSIONS (automation:admin, bank-accounts:*, delegations:*, policies:*, templates:*), 2 im permissions-Plugin (permissions:read/admin), 1 im forgejo-Reporter (system:read) | AST-Re-Scan: 146 Keys, **0 fehlend**; test_f9 | +| P2 | `contact_folder` als Core-Entity | Ins ContactsPlugin verschoben; `register_entity_model(..., plugin_name=...)` befüllt jetzt ENTITY_PLUGIN_OWNERS (war tot) | test_f15: `get_entity_read_permission('contact_folder') == 'contacts:read'` via Owner | +| P2 | Entity-Permission-Fallback `contacts:read` | Fail-closed Sentinel `__unmapped__:read` (nicht grantbar → 403); unbekannte Entities werden vorher via 422 abgelehnt | test_f15 | +| P2 | Forgejo-Error-Reporter `is_core=True` trotz „test/staging only" | `is_core=False` (deaktivierbar) | test_f11 | +| P1/P2 | Core-FK `entity_attachments.files` vs. „DMS = Plugin" Widerspruch | **ADR-020:** DMS als Plattform-Core-Plugin deklariert (`is_core=True`) — FK-Richtung ist damit legitim, Registry erzwingt Nicht-Deaktivierbarkeit | test_f12 | +| P2 | Core-Worker importierte Contact für Trash-Cleanup | `cleanup_contacts_trash` ins Contacts-Plugin ausgelagert (jobs.py, `get_job_modules()`-Discovery wie knowledge), Cron 04:15 | test_f13: kein `app.models.contact`-Import im Worker + Job registriert | +| P1/P2 | DSGVO-Export doppelt (Legacy-Route kannte Contacts direkt) | `GET /dsgvo-export` delegiert an `_dsar_collect_user_data` (autoritativer DSAR-Collector, Plugin-Contracts) | test_f14: Delegation, kein Contact-Import | +| P2 | False-green Tests (`or True`, irreführender Name, veraltete >100-Routes-Assertion) | 3 Assertions durch echte Prüfungen ersetzt; Test umbenannt (`_simulated`); Route-Count-Assertion auf Plugin-Architektur umgestellt (vorher schon auf HEAD rot — pre-existing) | Suite grün | + +**Nicht als Code-Fix, sondern als Phase Q geplant** (Roadmap „Phase Q **Produktion läuft stabil auf dem Phase-L-Deploy** (Commits b311ab7 + 559bba6, Health healthy, Alembic 0143, RLS 112 Tabellen). Alle Forgejo-Issues #351–#358 geschlossen. Forgejo ist komplett aktuell (HEAD = origin/main, 0 ungepushte Commits). -**Kürzlich abgeschlossen:** Phase L vollständig (L1 Block-System, L2 Drag&Drop-Editor, L3 Renderer, L4 KI-Steuerung, L5 XRechnung-Format-Layer) — Details siehe Phase-L-Sections unten. +**Kürzlich abgeschlossen:** Externer Architektur-Audit verifiziert + 13 Backend-Fixes (2026-09-13, siehe Audit-Section oben). Phase L vollständig (L1-L5). UI-Backlog Modul 1 (Approvals) erledigt, Module 2-16 offen. **Offene Roadmap-Phasen (user-abgestimmt, startklar):** - **Phase M** — MiniApp-Plattform & Dashboard-Builder (M1-M6). **M1 ✓** (Universal-Registry, `/api/v1/miniapps`), **M2 ✓** (persönliche Dashboards: Tabelle, CRUD, Seed, RLS), **M3 ✓** (Dashboard-Builder: Edit-Modus, Drag&Drop, Palette, Tabs), **M4 ✓** (System-Rückbau, Core = reiner Host), **M5 ✓** (Plugin-MiniApps), **M6 ✓ erledigt — PHASE M KOMPLETT** (Windows-Host + AI-Agenten-Tool send_miniapp — siehe Phase-M6-Section). @@ -18,7 +41,7 @@ **Wichtig:** AGENTS.md-Regeln zuerst lesen (§0.0 Sub-Agents nur für einfache Jobs, §0.2 auf bestehendem Code aufbauen, §10 'PROGRESS.md als Source of Truth'). -> **Letztes Update:** 2026-08-30 +> **Letztes Update:** 2026-09-13 ## Produktions-Bugfixes (2026-08-27) diff --git a/app/core/permission_registry.py b/app/core/permission_registry.py index 3a95e4c..0d47566 100644 --- a/app/core/permission_registry.py +++ b/app/core/permission_registry.py @@ -70,6 +70,17 @@ CORE_PERMISSIONS: list[dict[str, str]] = [ {"key": "dashboard:read", "label": "Dashboard: Read", "category": "core", "module": "dashboard"}, {"key": "dashboard:write", "label": "Dashboard: Write", "category": "core", "module": "dashboard"}, {"key": "system:admin", "label": "System: Admin (cross-tenant)", "category": "system", "module": "system"}, + # Audit P1 (permission catalog): these keys were required by core routes + # but never registered, so non-admin roles could never be granted them. + {"key": "automation:admin", "label": "Automation: Admin (backups, self-improvement)", "category": "core", "module": "automation"}, + {"key": "bank-accounts:read", "label": "Bank Accounts: Read", "category": "core", "module": "bank_accounts"}, + {"key": "bank-accounts:write", "label": "Bank Accounts: Write", "category": "core", "module": "bank_accounts"}, + {"key": "delegations:read", "label": "Delegations: Read", "category": "core", "module": "delegations"}, + {"key": "delegations:write", "label": "Delegations: Write", "category": "core", "module": "delegations"}, + {"key": "policies:read", "label": "Policies: Read", "category": "core", "module": "policies"}, + {"key": "policies:write", "label": "Policies: Write", "category": "core", "module": "policies"}, + {"key": "templates:read", "label": "Permission Templates: Read", "category": "core", "module": "templates"}, + {"key": "templates:write", "label": "Permission Templates: Write", "category": "core", "module": "templates"}, # NOTE: Plugin permissions (calendar, dms, mail, tasks, comm, automation, ai, # tags, entity_links, reports, search, mcp, permissions, agents) # are registered dynamically via register_plugin_permissions() from plugin @@ -79,50 +90,12 @@ CORE_PERMISSIONS: list[dict[str, str]] = [ # ── Core field definitions for field-level permissions ── CORE_FIELD_DEFINITIONS: list[dict[str, str]] = [ - # ── Contact fields ── - {"module": "contacts", "field": "firstname", "label": "First Name", "sensitivity": "normal"}, - {"module": "contacts", "field": "surname", "label": "Last Name", "sensitivity": "normal"}, - {"module": "contacts", "field": "displayname", "label": "Display Name", "sensitivity": "normal"}, - {"module": "contacts", "field": "name", "label": "Name", "sensitivity": "normal"}, - {"module": "contacts", "field": "email_1", "label": "Email 1", "sensitivity": "normal"}, - {"module": "contacts", "field": "email_2", "label": "Email 2", "sensitivity": "normal"}, - {"module": "contacts", "field": "phone_1", "label": "Phone 1", "sensitivity": "normal"}, - {"module": "contacts", "field": "phone_2", "label": "Phone 2", "sensitivity": "normal"}, - {"module": "contacts", "field": "mobilephone", "label": "Mobile", "sensitivity": "sensitive"}, - {"module": "contacts", "field": "function", "label": "Position", "sensitivity": "normal"}, - {"module": "contacts", "field": "website", "label": "Website", "sensitivity": "normal"}, - {"module": "contacts", "field": "status", "label": "Status", "sensitivity": "normal"}, - {"module": "contacts", "field": "type", "label": "Type", "sensitivity": "normal"}, - {"module": "contacts", "field": "gender", "label": "Gender", "sensitivity": "normal"}, - {"module": "contacts", "field": "suffix", "label": "Suffix", "sensitivity": "normal"}, - {"module": "contacts", "field": "ext_name_line", "label": "Extra Name Line", "sensitivity": "normal"}, - {"module": "contacts", "field": "country", "label": "Country", "sensitivity": "normal"}, - # ── Financial / sensitive fields ── - {"module": "contacts", "field": "code", "label": "Code", "sensitivity": "sensitive"}, - {"module": "contacts", "field": "accounting_code", "label": "Accounting Code", "sensitivity": "sensitive"}, - {"module": "contacts", "field": "vendor_accounting_code", "label": "Vendor Accounting Code", "sensitivity": "sensitive"}, - {"module": "contacts", "field": "vat_code", "label": "VAT Code", "sensitivity": "sensitive"}, - {"module": "contacts", "field": "fiscal_code", "label": "Fiscal Code", "sensitivity": "sensitive"}, - {"module": "contacts", "field": "commerce_code", "label": "Commerce Code", "sensitivity": "sensitive"}, - {"module": "contacts", "field": "purchase_number", "label": "Purchase Number", "sensitivity": "sensitive"}, - {"module": "contacts", "field": "bic", "label": "BIC", "sensitivity": "sensitive"}, - # ── Addresses ── - {"module": "contacts", "field": "mailing_street", "label": "Mailing Street", "sensitivity": "normal"}, - {"module": "contacts", "field": "mailing_city", "label": "Mailing City", "sensitivity": "normal"}, - {"module": "contacts", "field": "mailing_postalcode", "label": "Mailing Postal Code", "sensitivity": "normal"}, - {"module": "contacts", "field": "mailing_country", "label": "Mailing Country", "sensitivity": "normal"}, - {"module": "contacts", "field": "visit_street", "label": "Visit Street", "sensitivity": "normal"}, - {"module": "contacts", "field": "visit_city", "label": "Visit City", "sensitivity": "normal"}, - {"module": "contacts", "field": "visit_postalcode", "label": "Visit Postal Code", "sensitivity": "normal"}, - {"module": "contacts", "field": "visit_country", "label": "Visit Country", "sensitivity": "normal"}, - {"module": "contacts", "field": "invoice_street", "label": "Invoice Street", "sensitivity": "normal"}, - {"module": "contacts", "field": "invoice_city", "label": "Invoice City", "sensitivity": "normal"}, - {"module": "contacts", "field": "invoice_postalcode", "label": "Invoice Postal Code", "sensitivity": "normal"}, - {"module": "contacts", "field": "invoice_country", "label": "Invoice Country", "sensitivity": "normal"}, - # ── Notes & Tags ── - {"module": "contacts", "field": "notes", "label": "Notes", "sensitivity": "sensitive"}, - {"module": "contacts", "field": "tags", "label": "Tags", "sensitivity": "sensitive"}, - # ── User fields ── + # Audit P1/P2 (contact field definitions): all contacts:* field + # definitions moved to the ContactsPlugin manifest (field_definitions=) + # so the plugin fully owns its field structure. The core keeps only + # genuinely core-owned fields (users). Plugin field definitions are + # registered at activation time via register_field_definitions(). + # ── User fields (core-owned) ── {"module": "users", "field": "email", "label": "Email", "sensitivity": "normal"}, {"module": "users", "field": "name", "label": "Name", "sensitivity": "normal"}, {"module": "users", "field": "role", "label": "Role", "sensitivity": "normal"}, @@ -229,6 +202,18 @@ class PermissionRegistry: self._field_definitions[plugin_name] = field_defs logger.info("Registered %d field definitions for plugin '%s'", len(field_defs), plugin_name) + def unregister_field_definitions(self, plugin_name: str) -> None: + """Remove field definitions of a deactivated/uninstalled plugin. + + Audit P1/P2 (field-definitions lifecycle): the contribution type was + only half-integrated — register_field_definitions() existed but no + matching unregister, so a deactivated plugin kept serving its field + definitions in the permission UI. + """ + removed = self._field_definitions.pop(plugin_name, None) + if removed is not None: + logger.info("Unregistered %d field definitions for plugin '%s'", len(removed), plugin_name) + def get_all_field_definitions(self) -> list[dict[str, str]]: """Return all registered field definitions.""" result = list(self._core_field_definitions) diff --git a/app/core/sensitive_data.py b/app/core/sensitive_data.py index 5b152e0..8f94638 100644 --- a/app/core/sensitive_data.py +++ b/app/core/sensitive_data.py @@ -173,12 +173,15 @@ def _derive_policy_from_sensitivity( if field_name in entity_policy: return dict(entity_policy[field_name]) - # Try to get sensitivity from permission registry (lazy import to avoid - # circular dependencies at module load time). + # Try to get sensitivity from the permission registry (lazy import to + # avoid circular dependencies at module load time). Use the registry's + # combined view (core + plugin field definitions) — contact fields moved + # to the ContactsPlugin manifest (audit P1/P2), so CORE_FIELD_DEFINITIONS + # alone no longer covers them. try: - from app.core.permission_registry import CORE_FIELD_DEFINITIONS + from app.core.permission_registry import get_permission_registry - for fd in CORE_FIELD_DEFINITIONS: + for fd in get_permission_registry().get_all_field_definitions(): if fd.get("module") == entity_type and fd.get("field") == field_name: sensitivity = fd.get("sensitivity", "normal") return dict(_SENSITIVITY_DEFAULTS.get(sensitivity, _ALL_ALLOWED)) diff --git a/app/core/worker.py b/app/core/worker.py index 285f06e..2ba7534 100644 --- a/app/core/worker.py +++ b/app/core/worker.py @@ -396,7 +396,6 @@ async def cleanup_trash_job(ctx: dict[str, Any]) -> None: from sqlalchemy import text as sa_text from app.core.db import get_worker_session_factory - from app.models.contact import Contact from app.models.entity_attachment import EntityAttachment factory = get_worker_session_factory() @@ -414,16 +413,9 @@ async def cleanup_trash_job(ctx: dict[str, Any]) -> None: {"tid": str(tenant_id)}, ) - # Delete soft-deleted contacts - result = await db.execute( - sa_delete(Contact).where( - Contact.deleted_at.is_not(None), - Contact.deleted_at < cutoff, - ) - ) - total_deleted += result.rowcount - # Delete soft-deleted entity attachments + # (Contacts trash cleanup moved to the contacts plugin: + # cleanup_contacts_trash — audit P2, no core->contacts import) result = await db.execute( sa_delete(EntityAttachment).where( EntityAttachment.deleted_at.is_not(None), @@ -488,6 +480,12 @@ class WorkerSettings: _wrap_cron_with_lock("cleanup_trash", cleanup_trash_job, ttl_seconds=300), hour=4, minute=0, ), + # Contacts trash cleanup — daily at 04:15, owned by the contacts + # plugin (audit P2: no core->contacts import in the worker). + cron( + _wrap_cron_with_lock("cleanup_contacts_trash", get_job("cleanup_contacts_trash"), ttl_seconds=300), + hour=4, minute=15, + ), # Knowledge retention cleanup — daily at 05:00 (90 days, keeps approved). # Function comes from the knowledge plugin via the job registry. cron( diff --git a/app/main.py b/app/main.py index b1e3cd1..9ab2c4e 100644 --- a/app/main.py +++ b/app/main.py @@ -337,6 +337,22 @@ async def lifespan(app: FastAPI): if plugin and plugin.manifest.permissions: register_plugin_permissions(record.name, plugin.manifest.permissions) + # Audit P1 (contract lazy loading, restart edge case): plugins that + # were already inactive in the DB when this process started never get + # a runtime deactivate() call, so the ContractRegistry would + # lazy-load their contracts module and resurrect the contract. + # Mark them once here so get_contract() fails closed for them. + inactive_result = await db.execute( + sa_select(PluginModel.name).where(PluginModel.active == False) # noqa: E712 + ) + inactive_names = {row[0] for row in inactive_result} + if inactive_names: + from app.plugins.builtins.contracts import get_contract_registry + get_contract_registry().mark_db_inactive(inactive_names) + logger.info( + "Contract registry: %d plugins marked DB-inactive", len(inactive_names) + ) + init_permission_registry(active_plugin_names) logger.info("Permission registry initialized with %d active plugins", len(active_plugin_names)) @@ -359,7 +375,7 @@ async def lifespan(app: FastAPI): plugin = registry.get_plugin(name) if plugin: for entity_type, model_class in plugin.get_entity_models().items(): - register_entity_model(entity_type, model_class) + register_entity_model(entity_type, model_class, plugin_name=name) logger.info("Entity models registered for %d active plugins", len(active_plugin_names)) # Register field definitions from active plugins only diff --git a/app/plugins/builtins/contacts/jobs.py b/app/plugins/builtins/contacts/jobs.py new file mode 100644 index 0000000..aae9a1e --- /dev/null +++ b/app/plugins/builtins/contacts/jobs.py @@ -0,0 +1,64 @@ +"""ARQ background jobs for the contacts plugin. + +Registered via ``register_job()`` at import time; the worker discovers this +module through ``ContactsPlugin.get_job_modules()`` — the core worker must +not import contact models directly (audit P2: hidden core->contacts +coupling in the trash cleanup). +""" + +from __future__ import annotations + +import logging +from datetime import UTC, datetime, timedelta +from typing import Any + +from sqlalchemy import delete as sa_delete +from sqlalchemy import text as sa_text + +from app.core.job_registry import register_job + +logger = logging.getLogger(__name__) + +_TRASH_RETENTION_DAYS = 90 + + +async def cleanup_contacts_trash_job(ctx: dict[str, Any]) -> None: + """Permanently delete soft-deleted contacts older than the retention window. + + Runs daily. Iterates per-tenant for RLS compliance. + Moved from app.core.worker.cleanup_trash_job (audit P2) so the core + worker only handles core-owned entities (entity_attachments). + """ + from app.core.db import get_worker_session_factory + from app.models.contact import Contact + + factory = get_worker_session_factory() + async with factory() as db: + try: + tenant_result = await db.execute(sa_text("SELECT id FROM tenants")) + tenant_ids = [row[0] for row in tenant_result] + + cutoff = datetime.now(UTC) - timedelta(days=_TRASH_RETENTION_DAYS) + total_deleted = 0 + for tenant_id in tenant_ids: + await db.execute( + sa_text("SELECT set_config('app.current_tenant_id', :tid, true)"), + {"tid": str(tenant_id)}, + ) + result = await db.execute( + sa_delete(Contact).where( + Contact.deleted_at.is_not(None), + Contact.deleted_at < cutoff, + ) + ) + total_deleted += result.rowcount + await db.commit() + + if total_deleted: + logger.info("Contacts trash cleanup: permanently deleted %d old contacts", total_deleted) + except Exception: + logger.error("Contacts trash cleanup failed", exc_info=True) + await db.rollback() + + +register_job("cleanup_contacts_trash", cleanup_contacts_trash_job) diff --git a/app/plugins/builtins/contacts/plugin.py b/app/plugins/builtins/contacts/plugin.py index 2f68598..c014c6c 100644 --- a/app/plugins/builtins/contacts/plugin.py +++ b/app/plugins/builtins/contacts/plugin.py @@ -10,6 +10,7 @@ import logging from app.plugins.base import BasePlugin from app.plugins.manifest import ( + FieldDefinition, FrontendDashboardWidget, FrontendMenuItem, FrontendPageRoute, @@ -106,18 +107,70 @@ class ContactsPlugin(BasePlugin): "contacts:write", "contacts:delete", ], + # Audit P1/P2: contact field definitions are plugin-owned (moved + # from CORE_FIELD_DEFINITIONS) — registered at activation time via + # register_field_definitions() and removed on deactivation. + field_definitions=[ + FieldDefinition(module="contacts", field="firstname", label="First Name", sensitivity="normal"), + FieldDefinition(module="contacts", field="surname", label="Last Name", sensitivity="normal"), + FieldDefinition(module="contacts", field="displayname", label="Display Name", sensitivity="normal"), + FieldDefinition(module="contacts", field="name", label="Name", sensitivity="normal"), + FieldDefinition(module="contacts", field="email_1", label="Email 1", sensitivity="normal"), + FieldDefinition(module="contacts", field="email_2", label="Email 2", sensitivity="normal"), + FieldDefinition(module="contacts", field="phone_1", label="Phone 1", sensitivity="normal"), + FieldDefinition(module="contacts", field="phone_2", label="Phone 2", sensitivity="normal"), + FieldDefinition(module="contacts", field="mobilephone", label="Mobile", sensitivity="sensitive"), + FieldDefinition(module="contacts", field="function", label="Position", sensitivity="normal"), + FieldDefinition(module="contacts", field="website", label="Website", sensitivity="normal"), + FieldDefinition(module="contacts", field="status", label="Status", sensitivity="normal"), + FieldDefinition(module="contacts", field="type", label="Type", sensitivity="normal"), + FieldDefinition(module="contacts", field="gender", label="Gender", sensitivity="normal"), + FieldDefinition(module="contacts", field="suffix", label="Suffix", sensitivity="normal"), + FieldDefinition(module="contacts", field="ext_name_line", label="Extra Name Line", sensitivity="normal"), + FieldDefinition(module="contacts", field="country", label="Country", sensitivity="normal"), + FieldDefinition(module="contacts", field="code", label="Code", sensitivity="sensitive"), + FieldDefinition(module="contacts", field="accounting_code", label="Accounting Code", sensitivity="sensitive"), + FieldDefinition(module="contacts", field="vendor_accounting_code", label="Vendor Accounting Code", sensitivity="sensitive"), + FieldDefinition(module="contacts", field="vat_code", label="VAT Code", sensitivity="sensitive"), + FieldDefinition(module="contacts", field="fiscal_code", label="Fiscal Code", sensitivity="sensitive"), + FieldDefinition(module="contacts", field="commerce_code", label="Commerce Code", sensitivity="sensitive"), + FieldDefinition(module="contacts", field="purchase_number", label="Purchase Number", sensitivity="sensitive"), + FieldDefinition(module="contacts", field="bic", label="BIC", sensitivity="sensitive"), + FieldDefinition(module="contacts", field="mailing_street", label="Mailing Street", sensitivity="normal"), + FieldDefinition(module="contacts", field="mailing_city", label="Mailing City", sensitivity="normal"), + FieldDefinition(module="contacts", field="mailing_postalcode", label="Mailing Postal Code", sensitivity="normal"), + FieldDefinition(module="contacts", field="mailing_country", label="Mailing Country", sensitivity="normal"), + FieldDefinition(module="contacts", field="visit_street", label="Visit Street", sensitivity="normal"), + FieldDefinition(module="contacts", field="visit_city", label="Visit City", sensitivity="normal"), + FieldDefinition(module="contacts", field="visit_postalcode", label="Visit Postal Code", sensitivity="normal"), + FieldDefinition(module="contacts", field="visit_country", label="Visit Country", sensitivity="normal"), + FieldDefinition(module="contacts", field="invoice_street", label="Invoice Street", sensitivity="normal"), + FieldDefinition(module="contacts", field="invoice_city", label="Invoice City", sensitivity="normal"), + FieldDefinition(module="contacts", field="invoice_postalcode", label="Invoice Postal Code", sensitivity="normal"), + FieldDefinition(module="contacts", field="invoice_country", label="Invoice Country", sensitivity="normal"), + FieldDefinition(module="contacts", field="notes", label="Notes", sensitivity="sensitive"), + FieldDefinition(module="contacts", field="tags", label="Tags", sensitivity="sensitive"), + ], is_core=True, author="LeoCRM Team", min_app_version="1.0.0", contract_version="1.0.0", ) + def get_job_modules(self) -> list[str]: + """Worker discovers the contacts trash-cleanup job here (audit P2).""" + return ["app.plugins.builtins.contacts.jobs"] + def get_entity_models(self) -> dict[str, type]: from app.models.contact import Contact + from app.models.contact_folder import ContactFolder return { "contact": Contact, "contacts": Contact, "company": Contact, + # Audit P2: contact_folder is contacts-plugin-owned domain data + # (moved from the static core ENTITY_MODELS map). + "contact_folder": ContactFolder, } async def on_activate(self, db, service_container, event_bus) -> None: diff --git a/app/plugins/builtins/contracts.py b/app/plugins/builtins/contracts.py index be1224c..e16b73e 100644 --- a/app/plugins/builtins/contracts.py +++ b/app/plugins/builtins/contracts.py @@ -60,6 +60,9 @@ class ContractRegistry: cls._instance._contracts: dict[str, Any] = {} cls._instance._loaded: set[str] = set() cls._instance._unregistered: set[str] = set() + # Plugins whose DB record says active=False (audit restart edge + # case) — marked once at API startup, see main.py lifespan. + cls._instance._db_inactive: set[str] = set() return cls._instance # ─── registration ─── @@ -92,20 +95,51 @@ class ContractRegistry: On first access the registry attempts to lazy-load the plugin's ``contracts`` module, which will register itself on import. - """ - if plugin_name in self._contracts: - return self._contracts[plugin_name] + Audit P1 (contract lazy loading): the DB activation state is checked + BEFORE serving or lazy-loading. A plugin that was already inactive + when the process started never lands in ``_unregistered`` (it was + never deactivated at runtime), so the old guard alone let the lazy + loader import its contracts module and resurrect the contract. + The permission registry mirrors ``PluginModel.active`` at startup, + so an inactive plugin fails closed here. When the permission + registry is NOT initialized (worker process, early bootstrap) + the legacy lazy-load behaviour is kept. + """ # Explicitly unregistered (deactivated): never resurrect via # lazy-loading (ARCH-014) — the deactivated contract must stay gone. if plugin_name in self._unregistered: return None + # DB activation guard (audit restart edge case): plugins whose DB + # record was already inactive when the process started never land in + # _unregistered (they were never deactivated at runtime), so lazy + # loading could resurrect their contracts. main.py marks them once + # at startup; activation clears the marker again. + if plugin_name in self._db_inactive: + return None + + if plugin_name in self._contracts: + return self._contracts[plugin_name] + if plugin_name not in self._loaded: self._try_lazy_load(plugin_name) return self._contracts.get(plugin_name) + def mark_db_inactive(self, plugin_names: set[str]) -> None: + """Mark plugins as DB-inactive (startup, audit restart edge case). + + Called once from main.py lifespan with the names of plugins whose DB + record has active=False. get_contract() fails closed for these. + """ + self._db_inactive.update(plugin_names) + + def mark_plugin_active(self, plugin_name: str) -> None: + """Clear inactive markers (plugin activated/reinstalled at runtime).""" + self._db_inactive.discard(plugin_name) + self._unregistered.discard(plugin_name) + def require_contract(self, plugin_name: str) -> Any: """Like :meth:`get_contract` but raise if unavailable.""" contract = self.get_contract(plugin_name) @@ -148,6 +182,7 @@ class ContractRegistry: """Clear all state — for unit tests only.""" self._contracts.clear() self._loaded.clear() + self._db_inactive.clear() # ─── module-level helpers ─── diff --git a/app/plugins/builtins/dms/plugin.py b/app/plugins/builtins/dms/plugin.py index 1b6e8d2..e32a86e 100644 --- a/app/plugins/builtins/dms/plugin.py +++ b/app/plugins/builtins/dms/plugin.py @@ -20,6 +20,11 @@ class DmsPlugin(BasePlugin): version="1.0.0", display_name="DMS", description="Document management: folder hierarchy, file upload, PDF preview, Collabora edit sessions, internal sharing, search, bulk ops.", + # Audit P1/P2 (ADR-020): DMS is a platform core plugin — the core schema + # (entity_attachments.files-FK) builds on the DMS files table, so DMS + # cannot be deactivated. Declared is_core=True so the registry enforces + # this instead of the FK being silently invalid. + is_core=True, dependencies=["permissions"], routes=[ PluginRouteDef( diff --git a/app/plugins/builtins/forgejo_error_reporter/plugin.py b/app/plugins/builtins/forgejo_error_reporter/plugin.py index 6ec9cfe..2dd9e33 100644 --- a/app/plugins/builtins/forgejo_error_reporter/plugin.py +++ b/app/plugins/builtins/forgejo_error_reporter/plugin.py @@ -23,7 +23,9 @@ class ForgejoErrorReporterPlugin(BasePlugin): version="1.0.0", display_name="Forgejo Error Reporter", description="Automatically reports errors to Forgejo as issues. Test environment only.", - is_core=True, + # Audit P2 (classification): a test/staging-only plugin must be + # deactivatable — is_core=True contradicts its own production guard. + is_core=False, dependencies=[], events=[], migrations=[], @@ -33,7 +35,8 @@ class ForgejoErrorReporterPlugin(BasePlugin): path="/api/v1/forgejo-error-reporter", module="app.plugins.builtins.forgejo_error_reporter.routes", router_attr="router", - ), + permissions=["system:read"], + ), ], author="LeoCRM Team", diff --git a/app/plugins/builtins/permissions/plugin.py b/app/plugins/builtins/permissions/plugin.py index b8a8910..9ece650 100644 --- a/app/plugins/builtins/permissions/plugin.py +++ b/app/plugins/builtins/permissions/plugin.py @@ -30,7 +30,12 @@ class PermissionsPlugin(BasePlugin): ], events=[], migrations=["0001_initial.sql"], - permissions=[], + # Audit P1 (permission catalog): routes and settings pages use + # permissions:admin / permissions:read — they must be grantable. + permissions=[ + "permissions:read", + "permissions:admin", + ], is_core=True, settings_pages=[ FrontendSettingsPage(path='roles', label_key='settings.roles', label='Roles', component='@/pages/SettingsRoles', icon='Shield', order=10, permission='permissions:read'), diff --git a/app/plugins/registry.py b/app/plugins/registry.py index fef434c..afb46c4 100644 --- a/app/plugins/registry.py +++ b/app/plugins/registry.py @@ -4,6 +4,7 @@ from __future__ import annotations import importlib import logging +import uuid from pathlib import Path from typing import Any @@ -852,19 +853,43 @@ class PluginRegistry: # ── Active UI Manifests (Phase 3) ── - async def get_active_manifests(self, db: AsyncSession) -> list[dict[str, Any]]: + async def get_active_manifests( + self, db: AsyncSession, tenant_id: uuid.UUID | None = None + ) -> list[dict[str, Any]]: """Return UI manifests for all active plugins. Each entry contains the plugin name and its frontend UI contributions (menu_items, page_routes, detail_tabs, settings_pages, dashboard_widgets). + + Audit P1 (tenant manifests): when *tenant_id* is given, plugins that are + deactivated for that tenant (tenant_plugin_activation.is_active=False) + are excluded — the manifest output must mirror require_active_plugin() + semantics so the UI never offers menus/routes the backend then blocks + with 403. No tenant row = default active (same as the API gate). """ result = await db.execute(select(PluginModel).where(PluginModel.active.is_(True))) active_records = {row.name: row for row in result.scalars().all()} + # Per-tenant deactivations (same table/semantics as deps.require_active_plugin) + tenant_disabled: set[str] = set() + if tenant_id is not None: + from sqlalchemy import text as sa_text + + rows = await db.execute( + sa_text( + "SELECT plugin_name FROM tenant_plugin_activation " + "WHERE tenant_id = :tid AND is_active = false" + ), + {"tid": tenant_id}, + ) + tenant_disabled = {row[0] for row in rows} + manifests: list[dict[str, Any]] = [] for name, plugin in self._plugins.items(): if name not in active_records: continue + if name in tenant_disabled: + continue m = plugin.manifest manifests.append( { diff --git a/app/routes/plugins.py b/app/routes/plugins.py index da35d3b..620f75e 100644 --- a/app/routes/plugins.py +++ b/app/routes/plugins.py @@ -58,9 +58,21 @@ async def get_active_manifests( dashboard_widgets contributed by each active plugin. Used by the frontend PluginRegistry to dynamically register routes, sidebar items, settings pages, and detail tabs. + + Audit P1 (tenant manifests): plugins deactivated for the caller's + tenant are excluded so UI and API gates agree (no 403-on-click menus). """ + import uuid as uuid_mod + service = get_plugin_service() - manifests = await service.get_active_manifests(db) + tenant_id: uuid_mod.UUID | None = None + raw_tid = current_user.get("tenant_id") + if raw_tid: + try: + tenant_id = uuid_mod.UUID(str(raw_tid)) + except (ValueError, TypeError): + tenant_id = None + manifests = await service.get_active_manifests(db, tenant_id=tenant_id) return {"plugins": manifests, "total": len(manifests)} diff --git a/app/routes/system_settings.py b/app/routes/system_settings.py index c10c93f..29bb03d 100644 --- a/app/routes/system_settings.py +++ b/app/routes/system_settings.py @@ -192,25 +192,20 @@ async def dsgvo_export( ): """Export all personal data for a user (DSGVO/GDPR data subject access request). - Returns a JSON file with all data associated with the user: - - User profile - - Contacts owned by user - - Audit log entries - - Mail accounts - - Tasks assigned to user - - Calendar events - - Communication messages + Audit P1/P2 (DSGVO duplicate): this route previously held a second, + contact-aware export implementation parallel to the newer DSAR job + pipeline. It now delegates to the single authoritative collector + ``app.core.jobs._dsar_collect_user_data`` — core-owned categories are + collected there, plugin-owned categories (contacts, mail, tasks, + calendar, communication, ...) are contributed by the plugin contracts. + No core->contacts coupling here anymore. """ import io import json - from datetime import UTC, datetime from fastapi.responses import StreamingResponse - from sqlalchemy import select as sa_select - from app.models.audit import AuditLog - from app.models.contact import Contact - from app.models.user import User + from app.core.jobs import _dsar_collect_user_data tenant_id = uuid.UUID(current_user["tenant_id"]) try: @@ -218,34 +213,7 @@ async def dsgvo_export( except ValueError: raise HTTPException(400, detail={"detail": "Invalid user_id", "code": "invalid_id"}) from None - export_data = {"user_id": str(uid), "exported_at": datetime.now(UTC).isoformat(), "data": {}} - - # User profile - user_result = await db.execute(sa_select(User).where(User.id == uid)) - user = user_result.scalar_one_or_none() - if user: - export_data["data"]["profile"] = { - "email": user.email, "name": user.name, "role": user.role, - "is_active": user.is_active, "created_at": user.created_at.isoformat() if user.created_at else None, - } - - # Contacts owned by user - contacts_result = await db.execute( - sa_select(Contact).where(Contact.tenant_id == tenant_id, Contact.owner_id == uid, Contact.deleted_at.is_(None)) - ) - export_data["data"]["contacts"] = [ - {"id": str(c.id), "type": c.type, "displayname": c.displayname, "email_1": c.email_1, "email_2": c.email_2} - for c in contacts_result.scalars().all() - ] - - # Audit log entries - audit_result = await db.execute( - sa_select(AuditLog).where(AuditLog.tenant_id == tenant_id, AuditLog.user_id == uid).limit(1000) - ) - export_data["data"]["audit_log"] = [ - {"action": a.action, "entity_type": a.entity_type, "timestamp": a.timestamp.isoformat() if a.timestamp else None} - for a in audit_result.scalars().all() - ] + export_data = await _dsar_collect_user_data(db, str(tenant_id), str(uid)) # Log the DSGVO export from app.core.audit import log_audit diff --git a/app/services/entity_permission_service.py b/app/services/entity_permission_service.py index 7a4dc9b..5c175ab 100644 --- a/app/services/entity_permission_service.py +++ b/app/services/entity_permission_service.py @@ -29,7 +29,9 @@ from app.core.notifications import post_system_message from app.models.address import Address from app.models.attachment import Attachment from app.models.bank_account import BankAccount -from app.models.contact_folder import ContactFolder + +# NOTE (audit P2): ContactFolder moved to ContactsPlugin.get_entity_models() — +# the core entity registry no longer imports contact domain models. from app.models.custom_field_definition import CustomFieldDefinition from app.models.entity_permission import EntityPermission from app.models.group import Group, UserGroup @@ -66,7 +68,8 @@ ENTITY_MODELS: dict[str, type] = { "webhook": Webhook, "notification": Notification, "custom_field_definition": CustomFieldDefinition, - "contact_folder": ContactFolder, + # Audit P2: contact_folder moved to ContactsPlugin.get_entity_models() + # (it is plugin-owned domain data, not a core entity). } # W4b: Tracks which plugin registered which entity_type — used to derive @@ -92,7 +95,12 @@ def get_entity_read_permission(entity_type: str) -> str: candidates = _core_module_keys(module, "read") if candidates: return candidates[0] - return "contacts:read" + # Audit P2: fail closed. An entity that cannot be mapped to an owning + # module must NOT silently default to contacts:read — the sentinel is + # not grantable to any role, so check_entity_read_permission() denies. + # Unknown entity types are already rejected earlier by + # validate_entity_type() (422) before this fallback can matter. + return "__unmapped__:read" def _core_module_keys(module: str, action: str) -> list[str]: diff --git a/app/services/plugin_service.py b/app/services/plugin_service.py index 32031e3..7604891 100644 --- a/app/services/plugin_service.py +++ b/app/services/plugin_service.py @@ -118,7 +118,19 @@ class PluginService: if plugin: from app.services.entity_permission_service import register_entity_model for entity_type, model_class in plugin.get_entity_models().items(): - register_entity_model(entity_type, model_class) + register_entity_model(entity_type, model_class, plugin_name=name) + + # Register field definitions for field-level permissions + # (audit: contribution type now fully lifecycle-integrated) + if plugin: + field_defs = plugin.get_field_definitions() + if field_defs: + get_permission_registry().register_field_definitions(name, field_defs) + + # Contract registry: clear inactive markers so contracts of + # a re-activated plugin are served again (audit P1). + from app.plugins.builtins.contracts import get_contract_registry + get_contract_registry().mark_plugin_active(name) if tenant_id and user_id: await log_audit( @@ -188,6 +200,15 @@ class PluginService: for entity_type in plugin.get_entity_models(): unregister_entity_model(entity_type) + # Unregister field definitions (audit: full lifecycle) + get_permission_registry().unregister_field_definitions(name) + + # Contract registry: fail closed for the deactivated plugin + # (ARCH-014 / audit P1 — central, so every plugin is covered + # even if its own on_deactivate forgets the unregister). + from app.plugins.builtins.contracts import get_contract_registry + get_contract_registry().unregister(name) + if tenant_id and user_id: await log_audit( db, @@ -225,6 +246,16 @@ class PluginService: Deactivates, calls on_uninstall hook, optionally drops tables, removes DB record. """ try: + # Audit P1 (uninstall lifecycle): run the FULL service-level + # deactivation first. registry.uninstall()'s internal fallback + # (registry.deactivate) does NOT clean PermissionRegistry, + # _active_plugins or ENTITY_MODELS — an active plugin uninstalled + # directly through the registry left stale registrations behind. + pre = await self._registry._get_plugin_record(db, name) + if pre is not None and pre.active: + await self.deactivate_plugin( + db, name, tenant_id=tenant_id, user_id=user_id + ) record = await self._registry.uninstall(db, name, remove_data=remove_data) dropped_tables = getattr(record, "dropped_tables", []) if tenant_id and user_id: @@ -305,9 +336,16 @@ class PluginService: return MANIFEST_SCHEMA_DOC.model_dump() - async def get_active_manifests(self, db: AsyncSession) -> list[dict[str, Any]]: - """Return UI manifests for all active plugins.""" - return await self._registry.get_active_manifests(db) + async def get_active_manifests( + self, db: AsyncSession, tenant_id: uuid.UUID | None = None + ) -> list[dict[str, Any]]: + """Return UI manifests for all active plugins. + + Audit P1 (tenant manifests): *tenant_id* filters out plugins that are + deactivated for the caller's tenant (tenant_plugin_activation), + mirroring require_active_plugin() so UI and backend agree. + """ + return await self._registry.get_active_manifests(db, tenant_id=tenant_id) # Global service instance diff --git a/app/services/workspace_service.py b/app/services/workspace_service.py index 138c013..64f298a 100644 --- a/app/services/workspace_service.py +++ b/app/services/workspace_service.py @@ -52,16 +52,44 @@ async def list_workspaces( result = await db.execute(q) workspaces = result.scalars().all() - items = [] - for ws in workspaces: - # Count users - count_q = select(func.count()).select_from(WorkspaceUser).where( - WorkspaceUser.workspace_id == ws.id, + if not workspaces: + return {"items": [], "total": 0} + + # Audit P1 (Workspace-Editor): load modules for ALL workspaces in one + # query. Previously list_workspaces() returned modules: [] for every + # workspace, so the WorkspaceManager module editor showed all modules + # as hidden (is_visible=false) and saving OVERWROTE the existing config. + ws_ids = [ws.id for ws in workspaces] + mod_result = await db.execute( + select(WorkspaceModule).where( + WorkspaceModule.workspace_id.in_(ws_ids), + WorkspaceModule.tenant_id == tenant_id, + ).order_by(WorkspaceModule.menu_order) + ) + modules_by_ws: dict[uuid.UUID, list[WorkspaceModule]] = {} + for mod in mod_result.scalars().all(): + modules_by_ws.setdefault(mod.workspace_id, []).append(mod) + + # User counts for all workspaces in one query (avoids N+1) + count_result = await db.execute( + select(WorkspaceUser.workspace_id, func.count()) + .where( + WorkspaceUser.workspace_id.in_(ws_ids), WorkspaceUser.tenant_id == tenant_id, ) - count_result = await db.execute(count_q) - user_count = count_result.scalar() or 0 - items.append(_workspace_to_dict(ws, user_count=user_count)) + .group_by(WorkspaceUser.workspace_id) + ) + counts_by_ws: dict[uuid.UUID, int] = dict(count_result.all()) + + items = [] + for ws in workspaces: + items.append( + _workspace_to_dict( + ws, + modules=modules_by_ws.get(ws.id, []), + user_count=counts_by_ws.get(ws.id, 0), + ) + ) return {"items": items, "total": len(items)} diff --git a/docs/permissions.md b/docs/permissions.md index 5d4e0dd..c8ac5f2 100644 --- a/docs/permissions.md +++ b/docs/permissions.md @@ -60,6 +60,16 @@ Historical note (ARCH-008/009): migration 0019 seeded default roles with dead 3-segment patterns (`core:*:read` etc.); migration 0141 converts existing role data to the canonical form. +**Catalog completeness (audit fix 2026-09-13):** 12 previously used-but-unregistered keys are now in the catalog so roles can actually be granted them: + +- `CORE_PERMISSIONS` additions: `automation:admin`, `bank-accounts:read`, `bank-accounts:write`, `delegations:read`, `delegations:write`, `policies:read`, `policies:write`, `templates:read`, `templates:write` +- `permissions` plugin manifest: `permissions:read`, `permissions:admin` +- `forgejo_error_reporter` plugin manifest: `system:read` + +Verified via AST scan (used keys vs. catalog): 146 registered, 0 missing. + +**Entity-permission mapping fails closed (audit fix):** `get_entity_read_permission()` no longer falls back to `contacts:read` for unmapped entities — it returns the un-grantable sentinel `__unmapped__:read` (generic services then deny). Unknown entity types are rejected earlier with 422 by `validate_entity_type()`. Plugin-owned entities resolve correctly via `ENTITY_PLUGIN_OWNERS` (now populated through `register_entity_model(..., plugin_name=...)`). + --- ## 2. Data Model diff --git a/docs/plugin-development-guide.md b/docs/plugin-development-guide.md index ad92213..b30bc33 100644 --- a/docs/plugin-development-guide.md +++ b/docs/plugin-development-guide.md @@ -491,6 +491,17 @@ async def on_deactivate( self._event_handlers.clear() ``` +**Runtime lifecycle (service-managed, audit fix 2026-09-13):** `PluginService` registers/unregisters these contributions automatically on activate/deactivate — plugins do NOT need to do this manually: + +- **Permissions** (`manifest.permissions`) — registered/unregistered in the permission registry +- **Entity models** (`get_entity_models()`) — registered in `ENTITY_MODELS` **with `plugin_name`**, so `get_entity_read_permission()` derives the owning module's read permission (e.g. `contacts:read` for `contact_folder`) +- **Field definitions** (`get_field_definitions()` / `manifest.field_definitions`) — registered/unregistered in the permission registry's field-definition store (full lifecycle since the audit fix) +- **Contracts** — the `ContractRegistry` fails closed for deactivated plugins (central unregister on deactivate; `mark_db_inactive()` at startup covers plugins already inactive in the DB — restart edge case) + +**ADR-020 (is_core classification):** `is_core=True` means the plugin CANNOT be deactivated (`registry.deactivate()` rejects it). DMS is declared `is_core=True` because the core schema (`entity_attachments.files` FK) builds on its `files` table. Test-only plugins (e.g. `forgejo_error_reporter`) must be `is_core=False` so they stay deactivatable. + +**Uninstall ordering (audit P1 fix):** `uninstall_plugin()` runs the FULL service deactivation first (`deactivate_plugin()`), then `registry.uninstall()` — so no stale permission/entity/contract registrations remain. + ### 4.4 Uninstallation (`on_uninstall`) Called when the plugin is uninstalled (before data tables are dropped). Override to clean up external resources. diff --git a/tests/conftest.py b/tests/conftest.py index 572da45..19f4d00 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -77,7 +77,7 @@ for _plugin_name in _registry.list_discovered(): # every plugin's entity models are registered in ENTITY_MODELS — # the core registry itself carries core entities only. for _entity_type, _model_class in _entity_models.items(): - register_entity_model(_entity_type, _model_class) + register_entity_model(_entity_type, _model_class, plugin_name=_plugin_name) # Also import the plugin's __init__ to ensure all models are loaded import importlib try: @@ -105,7 +105,7 @@ def _ensure_plugin_entity_models(): if _plugin is None: continue for _entity_type, _model_class in _plugin.get_entity_models().items(): - register_entity_model(_entity_type, _model_class) + register_entity_model(_entity_type, _model_class, plugin_name=_plugin_name) yield # Also import core models that may be missing diff --git a/tests/test_audit_architecture_fixes.py b/tests/test_audit_architecture_fixes.py new file mode 100644 index 0000000..8af3172 --- /dev/null +++ b/tests/test_audit_architecture_fixes.py @@ -0,0 +1,503 @@ +"""Regression tests for the external architecture audit (leocrm-full.zip, 2026-09). + +Covers the confirmed findings with live counterchecks (AGENTS.md rule — +no finding is marked done without a measurement): + +- F1 list_workspaces() returns modules (WorkspaceManager overwrite bug) +- F2 active-manifests respects tenant deactivation (signature level) +- F4 ContractRegistry fails closed for DB-inactive plugins + (restart edge case) and reopens on activation +- F5 field definitions have a full lifecycle (register/unregister) +- F9 permission catalog: every used key is registered +- F10 contact field definitions are plugin-owned (not in core list) +- F11 forgejo_error_reporter is not is_core +- F12 dms is a platform core plugin (ADR-020) +- F13 core worker has no contact import; contacts trash cleanup is a + plugin job discovered via get_job_modules() +- F14 the legacy dsgvo-export route delegates to the single DSAR collector +- F15 get_entity_read_permission fails closed instead of contacts:read + +The frontend architecture items (static plugin routes, STATIC_COMPONENT_MAP, +widgetRegistry) are tracked as Phase Q — not covered here. +""" + +from __future__ import annotations + +import uuid + +import pytest +from sqlalchemy.ext.asyncio import AsyncSession + +from app.models.tenant import Tenant +from app.models.user import User, UserTenant +from app.services import workspace_service + + + +@pytest.fixture(autouse=True) +def _restore_global_registries(): + """Snapshot & restore global registries mutated by these tests. + + The F5 lifecycle test deactivates the real 'contacts' plugin through + PluginService, which (since the audit fix) centrally unregisters its + CONTRACT and clears permission-registry state. The conftest resets + entity models but NOT the contract registry — without this restore, + get_contract('contacts') stays fail-closed for every later test in the + same process (suite-order dependence). + """ + from app.plugins.builtins.contracts import get_contract_registry + from app.core.permission_registry import get_permission_registry + from app.services.entity_permission_service import ( + ENTITY_MODELS, + ENTITY_PLUGIN_OWNERS, + ) + + perm_reg = get_permission_registry() + saved_active = set(perm_reg._active_plugins) + saved_plugin_perms = {k: list(v) for k, v in perm_reg._plugin_permissions.items()} + saved_field_defs = {k: list(v) for k, v in perm_reg._field_definitions.items()} + + cr = get_contract_registry() + saved_contracts = dict(cr._contracts) + saved_loaded = set(cr._loaded) + saved_unreg = set(cr._unregistered) + saved_db_inactive = set(cr._db_inactive) + + saved_entity_models = dict(ENTITY_MODELS) + saved_owners = dict(ENTITY_PLUGIN_OWNERS) + + yield + + perm_reg._active_plugins.clear() + perm_reg._active_plugins.update(saved_active) + perm_reg._plugin_permissions.clear() + perm_reg._plugin_permissions.update(saved_plugin_perms) + perm_reg._field_definitions.clear() + perm_reg._field_definitions.update(saved_field_defs) + + cr._contracts.clear() + cr._contracts.update(saved_contracts) + cr._loaded.clear() + cr._loaded.update(saved_loaded) + cr._unregistered.clear() + cr._unregistered.update(saved_unreg) + cr._db_inactive.clear() + cr._db_inactive.update(saved_db_inactive) + + ENTITY_MODELS.clear() + ENTITY_MODELS.update(saved_entity_models) + ENTITY_PLUGIN_OWNERS.clear() + ENTITY_PLUGIN_OWNERS.update(saved_owners) + +# ── F1: list_workspaces returns modules ───────────────────────────────────── + + +async def _seed_tenant_and_user(db: AsyncSession) -> dict: + """Seed a tenant and a user, return IDs (test_workspaces.py pattern).""" + from app.core.auth import hash_password + + tenant = Tenant(name="Audit Tenant", slug=f"audit-{uuid.uuid4().hex[:8]}") + db.add(tenant) + await db.flush() + + user = User( + email=f"audit-{uuid.uuid4().hex[:8]}@example.com", + name="Audit User", + password_hash=hash_password("TestPass123!"), + is_active=True, + preferences={}, + ) + db.add(user) + await db.flush() + + db.add(UserTenant(user_id=user.id, tenant_id=tenant.id, is_default=True, role="admin")) + await db.flush() + return {"tenant": tenant, "user": user} + + +@pytest.mark.asyncio +async def test_f1_list_workspaces_returns_modules(db_session: AsyncSession): + """Audit F1: list_workspaces must include configured modules. + + Previously every workspace came back with modules: [] and the + WorkspaceManager editor overwrote existing config as all-hidden. + """ + seed = await _seed_tenant_and_user(db_session) + created = await workspace_service.create_workspace( + db_session, seed["tenant"].id, seed["user"].id, name="AuditWS", + ) + ws_id = uuid.UUID(created["id"]) + await workspace_service.set_workspace_modules( + db_session, + seed["tenant"].id, + ws_id, + [ + {"module_key": "contacts", "is_visible": True, "menu_order": 10, "config": {}}, + {"module_key": "calendar", "is_visible": False, "menu_order": 20, "config": {}}, + ], + ) + + result = await workspace_service.list_workspaces(db_session, seed["tenant"].id) + assert result["total"] == 1 + listed = result["items"][0] + assert listed["name"] == "AuditWS" + # THE regression: modules must be present with their is_visible flags + mods = {m["module_key"]: m["is_visible"] for m in listed["modules"]} + assert mods == {"contacts": True, "calendar": False} + + +# ── F2: active-manifests tenant filtering (signature level) ──────────────── + + +def test_f2_get_active_manifests_accepts_tenant_id(): + """Audit F2: the registry manifest API accepts and honours tenant_id.""" + import inspect + + from app.plugins.registry import PluginRegistry + + sig = inspect.signature(PluginRegistry.get_active_manifests) + assert "tenant_id" in sig.parameters, ( + "get_active_manifests must accept tenant_id so tenant-deactivated " + "plugins are hidden from the UI manifests" + ) + + # Service passes it through + from app.services.plugin_service import PluginService + + sig = inspect.signature(PluginService.get_active_manifests) + assert "tenant_id" in sig.parameters + + +@pytest.mark.asyncio +async def test_f2_manifests_exclude_tenant_disabled_plugins(db_session: AsyncSession): + """Audit F2: a plugin deactivated for the tenant is filtered out.""" + from sqlalchemy import text + + seed = await _seed_tenant_and_user(db_session) + + # tenant_plugin_activation is created by migration 0066 only (no ORM + # model) — test DBs built via create_all lack it, so create it if needed. + await db_session.execute( + text( + "CREATE TABLE IF NOT EXISTS tenant_plugin_activation (" + "id UUID PRIMARY KEY DEFAULT gen_random_uuid(), " + "tenant_id UUID NOT NULL REFERENCES tenants(id) ON DELETE CASCADE, " + "plugin_name VARCHAR(100) NOT NULL, " + "is_active BOOLEAN NOT NULL DEFAULT true, " + "activated_by UUID, " + "created_at TIMESTAMPTZ NOT NULL DEFAULT now(), " + "updated_at TIMESTAMPTZ NOT NULL DEFAULT now())" + ) + ) + await db_session.execute( + text( + "INSERT INTO tenant_plugin_activation (tenant_id, plugin_name, is_active) " + "VALUES (:tid, 'forgejo_error_reporter', false)" + ), + {"tid": str(seed["tenant"].id)}, + ) + await db_session.flush() + + from app.models.plugin import Plugin as PluginModel + from app.plugins.registry import get_registry + + registry = get_registry() + if not registry.list_discovered(): + registry.discover_builtins() + + # Seed a globally-ACTIVE plugin record so the manifest is served at all + # (test DBs have no plugin rows unless installed through the registry). + db_session.add(PluginModel( + name="forgejo_error_reporter", + display_name="Forgejo Error Reporter", + version="1.0.0", + status="active", + installed=True, + active=True, + is_core=False, + )) + await db_session.flush() + + all_manifests = await registry.get_active_manifests(db_session, tenant_id=None) + assert any(m["name"] == "forgejo_error_reporter" for m in all_manifests), ( + "globally active plugin must appear in manifests (test premise)" + ) + + filtered = await registry.get_active_manifests( + db_session, tenant_id=seed["tenant"].id + ) + assert all(m["name"] != "forgejo_error_reporter" for m in filtered), ( + "tenant-deactivated plugin must not appear in the UI manifests" + ) + + +# ── F4: contract registry restart guard ──────────────────────────────────── + + +def test_f4_contract_registry_fails_closed_for_db_inactive(): + """Audit F4: contracts of DB-inactive plugins are not resurrected.""" + from app.plugins.builtins.contracts import get_contract, get_contract_registry + + reg = get_contract_registry() + reg._reset_for_testing() + try: + reg.mark_db_inactive({"kommunikation"}) + # Even after lazy-load attempts the contract must stay unavailable + assert get_contract("kommunikation") is None + finally: + reg._reset_for_testing() + + +def test_f4_contract_registry_reopens_on_activation(): + """Audit F4: activating a plugin clears its inactive markers.""" + from app.plugins.builtins.contracts import get_contract_registry + + reg = get_contract_registry() + reg._reset_for_testing() + try: + reg.mark_db_inactive({"some_plugin"}) + reg._unregistered.add("some_plugin") + reg.mark_plugin_active("some_plugin") + assert "some_plugin" not in reg._db_inactive + assert "some_plugin" not in reg._unregistered + finally: + reg._reset_for_testing() + + +# ── F5: field definitions lifecycle ───────────────────────────────────────── + + +def test_f5_field_definitions_full_lifecycle(): + """Audit F5: register → present, unregister → gone.""" + from app.core.permission_registry import PermissionRegistry + + reg = PermissionRegistry() + reg.initialize() + defs = [{"module": "x", "field": "y", "label": "Y", "sensitivity": "normal"}] + reg.register_field_definitions("audit_plugin", defs) + assert any(d.get("plugin_key", d.get("field")) or True for d in defs) + all_defs = reg.get_all_field_definitions() + assert any(d.get("field") == "y" for d in all_defs) + + reg.unregister_field_definitions("audit_plugin") + all_defs = reg.get_all_field_definitions() + assert not any(d.get("field") == "y" and d.get("module") == "x" for d in all_defs) + + +def test_f5_deactivate_plugin_unregisters_field_definitions(): + """Audit F5: PluginService.deactivate_plugin removes field definitions.""" + import asyncio + from unittest.mock import AsyncMock, MagicMock + + from app.core.permission_registry import get_permission_registry, init_permission_registry + from app.plugins.builtins.contacts.plugin import ContactsPlugin + from app.services.plugin_service import PluginService + + init_permission_registry(set()) + perm_reg = get_permission_registry() + perm_reg._active_plugins.add("contacts") + perm_reg.register_field_definitions("contacts", ContactsPlugin().get_field_definitions()) + assert any( + d.get("module") == "contacts" + for d in perm_reg.get_all_field_definitions() + ) + + service = PluginService() + # deactivate_plugin resolves the record via registry._get_plugin_record — + # a MagicMock plugin record with active=True triggers the cleanup path. + # NOTE: service._registry IS the global singleton — save & restore the + # patched methods so the conftest bootstrap stays intact for later tests + # (order-dependence guard). + mock_record = MagicMock() + mock_record.active = True + mock_record.status = "active" + _orig_get_record = service._registry._get_plugin_record + _orig_deactivate = service._registry.deactivate + _orig_get_plugin = service._registry.get_plugin + service._registry._get_plugin_record = AsyncMock(return_value=mock_record) + service._registry.deactivate = AsyncMock(return_value=mock_record) + service._registry.get_plugin = MagicMock(return_value=ContactsPlugin()) + + try: + asyncio.get_event_loop().run_until_complete( + service.deactivate_plugin(AsyncMock(), "contacts") + ) + finally: + service._registry._get_plugin_record = _orig_get_record + service._registry.deactivate = _orig_deactivate + service._registry.get_plugin = _orig_get_plugin + assert not any( + d.get("module") == "contacts" + for d in perm_reg.get_all_field_definitions() + ), "deactivation must unregister the plugin's field definitions" + + +# ── F9: permission catalog completeness ───────────────────────────────────── + + +def test_f9_all_audit_permission_keys_registered(): + """Audit F9: the 12 previously missing keys are grantable now.""" + from app.core.permission_registry import ( + CORE_PERMISSIONS, + get_permission_registry, + init_permission_registry, + ) + + init_permission_registry(set()) + get_permission_registry().register_plugin_permissions( + "permissions", ["permissions:read", "permissions:admin"] + ) + get_permission_registry().register_plugin_permissions( + "forgejo_error_reporter", ["system:read"] + ) + + known = get_permission_registry().get_all() + keys = {p["key"] for p in known} + + core_expected = [ + "automation:admin", + "bank-accounts:read", + "bank-accounts:write", + "delegations:read", + "delegations:write", + "policies:read", + "policies:write", + "templates:read", + "templates:write", + ] + for key in core_expected: + assert key in {p["key"] for p in CORE_PERMISSIONS}, f"{key} missing in CORE_PERMISSIONS" + + for key in ["permissions:read", "permissions:admin", "system:read"]: + assert key in keys, f"{key} must be grantable" + + assert get_permission_registry().is_valid("delegations:read") + + +# ── F10: contact field definitions are plugin-owned ──────────────────────── + + +def test_f10_contact_field_definitions_plugin_owned(): + """Audit F10: contacts:* field defs come from the plugin manifest.""" + from app.core.permission_registry import CORE_FIELD_DEFINITIONS + from app.plugins.builtins.contacts.plugin import ContactsPlugin + + core_modules = {fd["module"] for fd in CORE_FIELD_DEFINITIONS} + assert "contacts" not in core_modules, "core must not own contacts field definitions" + + plugin_defs = ContactsPlugin().get_field_definitions() + assert len(plugin_defs) >= 30, "contacts plugin should carry its ~39 field definitions" + assert all(fd["module"] == "contacts" for fd in plugin_defs) + # Spot-check sensitive fields are still classified sensitive after the move + by_field = {fd["field"]: fd for fd in plugin_defs} + assert by_field["mobilephone"]["sensitivity"] == "sensitive" + assert by_field["bic"]["sensitivity"] == "sensitive" + + +# ── F11/F12: plugin classification ───────────────────────────────────────── + + +def test_f11_forgejo_error_reporter_not_core(): + """Audit F11: the test/staging-only reporter is deactivatable.""" + from app.plugins.builtins.forgejo_error_reporter.plugin import ForgejoErrorReporterPlugin + + assert ForgejoErrorReporterPlugin().manifest.is_core is False + + +def test_f12_dms_is_platform_core_plugin(): + """Audit F12/ADR-020: the core schema builds on the DMS files table.""" + from app.plugins.builtins.dms.plugin import DmsPlugin + + assert DmsPlugin().manifest.is_core is True + + +# ── F13: worker decoupled from contacts ──────────────────────────────────── + + +def test_f13_core_worker_has_no_contact_import(): + """Audit F13: app/core/worker.py must not import contact models.""" + import inspect + + import app.core.worker as worker + + src = inspect.getsource(worker) + assert "from app.models.contact import" not in src + + +def test_f13_contacts_trash_job_discovered_via_job_modules(): + """Audit F13: contacts plugin owns its trash cleanup job.""" + from app.core.job_registry import get_job + from app.plugins.builtins.contacts.jobs import cleanup_contacts_trash_job + from app.plugins.builtins.contacts.plugin import ContactsPlugin + + modules = ContactsPlugin().get_job_modules() + assert "app.plugins.builtins.contacts.jobs" in modules + + import importlib + + importlib.import_module("app.plugins.builtins.contacts.jobs") + assert get_job("cleanup_contacts_trash") is cleanup_contacts_trash_job + + +# ── F14: single DSAR export path ─────────────────────────────────────────── + + +def test_f14_dsgvo_route_delegates_to_dsar_collector(): + """Audit F14: the legacy export route delegates, it does not reimplement.""" + import inspect + + import app.routes.system_settings as ss + + src = inspect.getsource(ss) + assert "_dsar_collect_user_data" in src, "route must delegate to the DSAR collector" + assert "from app.models.contact import" not in src, ( + "the export route must not know contacts internals" + ) + + +# ── F15: entity permission fallback fails closed ─────────────────────────── + + +def test_f15_entity_read_permission_fails_closed(): + """Audit F15: unmapped entities no longer default to contacts:read.""" + from app.services.entity_permission_service import ( + ENTITY_PLUGIN_OWNERS, + get_entity_read_permission, + ) + + saved = ENTITY_PLUGIN_OWNERS.pop("zzz_audit_unknown", None) + try: + perm = get_entity_read_permission("zzz_audit_unknown") + assert perm != "contacts:read" + assert perm == "__unmapped__:read" + finally: + if saved is not None: + ENTITY_PLUGIN_OWNERS["zzz_audit_unknown"] = saved + + +def test_f15_contact_folder_registered_by_plugin(): + """Audit F15: contact_folder comes from ContactsPlugin, not core.""" + from app.plugins.builtins.contacts.plugin import ContactsPlugin + + models = ContactsPlugin().get_entity_models() + assert "contact_folder" in models + + from app.services.entity_permission_service import get_entity_read_permission + + assert get_entity_read_permission("contact_folder") == "contacts:read" + + +# ── Uninstall lifecycle (audit P1) ───────────────────────────────────────── + + +def test_f3_uninstall_deactivates_via_service_first(): + """Audit P1: uninstall_plugin runs the full service deactivation first.""" + import inspect + + from app.services.plugin_service import PluginService + + src = inspect.getsource(PluginService.uninstall_plugin) + assert "self.deactivate_plugin(" in src, ( + "uninstall must run the full service-level deactivation (permissions, " + "active set, entity models) before registry.uninstall()" + ) diff --git a/tests/test_contacts_entity_registry.py b/tests/test_contacts_entity_registry.py index 0777644..3b0c93e 100644 --- a/tests/test_contacts_entity_registry.py +++ b/tests/test_contacts_entity_registry.py @@ -29,14 +29,28 @@ def test_core_registry_source_has_no_static_contacts_entries(): def test_contacts_plugin_is_single_source_for_its_entities(): - """ContactsPlugin.get_entity_models() defines contact/contacts/company.""" + """ContactsPlugin.get_entity_models() defines contact/contacts/company/contact_folder. + + Audit P2: contact_folder moved from the static core ENTITY_MODELS map to + the plugin (it is contacts-owned domain data) — the plugin is the single + source for ALL its entities. + """ + from app.models.contact_folder import ContactFolder plugin_models = ContactsPlugin().get_entity_models() assert plugin_models == { "contact": Contact, "contacts": Contact, "company": Contact, + "contact_folder": ContactFolder, } + # The core registry must NOT carry contact_folder statically anymore — + # it appears there only through the plugin bootstrap (conftest mirrors + # main.py: every discovered plugin's entities get registered). + from app.services import entity_permission_service as eps + core_static_types = set(eps.ENTITY_MODELS.keys()) - set(plugin_models.keys()) + assert "contact_folder" not in core_static_types + def test_bootstrap_registers_contacts_entities_via_plugin(): """After the real bootstrap path (conftest mirrors main.py lifespan: diff --git a/tests/test_contacts_lifecycle.py b/tests/test_contacts_lifecycle.py index 14785f1..ba723f5 100644 --- a/tests/test_contacts_lifecycle.py +++ b/tests/test_contacts_lifecycle.py @@ -46,7 +46,13 @@ class TestContactsPluginLifecycle: # Before activation: nothing registered reg = get_restore_registry() assert not reg.is_registered("contact") - assert "contact" not in ENTITY_MODELS or True # May be in core models + # Audit P2 (false-green): this used to be + # ``assert "contact" not in ENTITY_MODELS or True`` — always true. + # Entity models are registered by the conftest bootstrap for ALL + # discovered plugins (mirroring main.py), so "contact" IS present + # before on_activate; what must NOT be registered yet is the + # restore config (checked above) and the plugin gate: + assert not get_permission_registry().is_plugin_active("contacts") # Activate import asyncio @@ -136,11 +142,26 @@ class TestContactsPluginLifecycle: assert models["company"] is Contact def test_app_still_starts_without_contacts_special_case(self): - """App creates successfully with Contacts as a plugin, not a Core special case.""" + """App creates successfully with Contacts as a plugin, not a Core special case. + + Audit P2 (false-green / stale assertion): the old ``> 100 routes`` + assertion reflected the PRE-plugin architecture where business + routes were hardcoded in main.py. Since the plugin refactor, + create_app() mounts only CORE routes (~85); plugin routes are + mounted by the registry lifecycle. The real "no special case" + proof is that no contacts route is statically included. + """ from app.main import create_app app = create_app() assert app is not None - assert len(app.routes) > 100 # Should have many routes + # Core routes must exist + assert len(app.routes) > 50 + # Contacts is a plugin: its routes must NOT be hardcoded in the + # core app (they are mounted via registry.activate at startup). + static_paths = {getattr(r, "path", "") for r in app.routes} + assert not any(p.startswith("/api/v1/contacts") for p in static_paths), ( + "contacts routes must not be statically mounted — Contacts is a plugin" + ) def test_hook_isolation_deactivate_one_plugin_keeps_other(self): """Two plugins register on same event; deactivating one keeps the other's handler.""" @@ -180,12 +201,14 @@ class TestContactsPluginLifecycle: # Cleanup reg._reset_for_testing() - def test_full_lifecycle_activate_deactivate_via_service(self): - """Full lifecycle: activate via PluginService → verify registered → deactivate → verify deregistered. + def test_full_lifecycle_activate_deactivate_simulated(self): + """Full lifecycle: simulate the service activation sequence → verify + registered → simulate deactivation → verify deregistered. - This is the E2E lifecycle test (Phase 5.2): tests that activation/deactivation - through the service layer properly registers/deregisters permissions, - entity models, restore config, and history hooks. + Audit P2 (false-green): renamed — this test drives the plugin hooks + and registry calls MANUALLY (mirroring what PluginService does), + it does NOT go through the real PluginService. The true E2E test + through PluginService lives in test_plugin_lifecycle_service.py. """ from app.services.plugin_service import PluginService from app.core.permission_registry import get_permission_registry, init_permission_registry @@ -252,8 +275,11 @@ class TestContactsPluginLifecycle: # Verify everything is deregistered assert not perm_reg.is_plugin_active(plugin_name), "Plugin should be inactive" assert not get_restore_registry().is_registered("contact"), "Restore config should be removed" - assert "contact" not in ENTITY_MODELS or "contact" in {"contact", "contacts", "company"}, \ - "Entity model may still be in core ENTITY_MODELS" + # Audit P2 (false-green): the old assertion + # ``"contact" not in ENTITY_MODELS or "contact" in {...}`` was + # always true. After unregister_entity_model() the type must be + # GONE from the registry (contacts is plugin-owned, not core). + assert "contact" not in ENTITY_MODELS, "contact must be deregistered after plugin deactivation" all_hooks = hook_reg._actions if hasattr(hook_reg, '_actions') else {} assert not any("contact.after_create" in k for k in all_hooks), "History hooks should be removed" diff --git a/tests/test_rbac_comprehensive.py b/tests/test_rbac_comprehensive.py index d66f42c..476aed1 100644 --- a/tests/test_rbac_comprehensive.py +++ b/tests/test_rbac_comprehensive.py @@ -411,12 +411,35 @@ class TestPermissionRegistryUnit: assert mail_defs[0]["field"] == "subject" def test_get_all_field_definitions_includes_core(self): - """get_all_field_definitions() includes core field definitions.""" + """get_all_field_definitions() includes core field definitions. + + Audit P1/P2: contacts:* field definitions moved to the ContactsPlugin + manifest — the core keeps only core-owned fields (users). Plugin + definitions join via register_field_definitions() (full lifecycle: + register/unregister). + """ reg = PermissionRegistry() reg.initialize() all_defs = reg.get_all_field_definitions() + user_defs = [d for d in all_defs if d.get("module") == "users"] + assert len(user_defs) > 0 + # Contacts are plugin-owned now — nothing in the core list contact_defs = [d for d in all_defs if d.get("module") == "contacts"] - assert len(contact_defs) > 0 + assert contact_defs == [] + + # Plugin definitions appear once registered ... + reg.register_field_definitions("contacts", [ + {"module": "contacts", "field": "firstname", "label": "First Name", "sensitivity": "normal"}, + ]) + all_defs = reg.get_all_field_definitions() + contact_defs = [d for d in all_defs if d.get("module") == "contacts"] + assert len(contact_defs) == 1 + # ... and disappear on unregister (audit: contribution fully + # lifecycle-integrated) + reg.unregister_field_definitions("contacts") + all_defs = reg.get_all_field_definitions() + contact_defs = [d for d in all_defs if d.get("module") == "contacts"] + assert contact_defs == [] # ═══════════════════════════════════════════════════════════════