diff --git a/alembic/versions/0060_rls_contacts_secure.py b/alembic/versions/0060_rls_contacts_secure.py new file mode 100644 index 0000000..dfc39b9 --- /dev/null +++ b/alembic/versions/0060_rls_contacts_secure.py @@ -0,0 +1,202 @@ +"""Fix RLS policies on contacts — add tenant_id isolation. + +Revision ID: 0060 +Revises: 0059 +Create Date: 2026-07-29 + +This migration drops the insecure contact RLS policies (created in 0052) +and recreates them with proper tenant_id isolation. + +Problems fixed: +1. contacts_tenant_owned_visible had USING (owner_id IS NULL) without tenant_id check +2. contacts_admin_visible had no tenant_id check +3. contacts_owner_visible had no tenant_id check +4. All policies used FOR ALL instead of separate SELECT/INSERT/UPDATE/DELETE +5. No WITH CHECK on write operations +""" + +from alembic import op + +revision = "0060" +down_revision = "0059" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # Drop all existing contact policies + op.execute("DROP POLICY IF EXISTS contacts_admin_visible ON contacts") + op.execute("DROP POLICY IF EXISTS contacts_owner_visible ON contacts") + op.execute("DROP POLICY IF EXISTS contacts_tenant_owned_visible ON contacts") + op.execute("DROP POLICY IF EXISTS contacts_shared_visible ON contacts") + op.execute("DROP POLICY IF EXISTS tenant_isolation ON contacts") + + # ── Restrive policy: Tenant isolation (always enforced) ── + # This is the base policy that ALL other permissive policies are ANDed with + op.execute(""" + CREATE POLICY contacts_tenant_isolation ON contacts + FOR ALL + USING (tenant_id = current_setting('app.current_tenant_id', true)::uuid) + WITH CHECK (tenant_id = current_setting('app.current_tenant_id', true)::uuid) + """) + + # ── Permissive policies for SELECT (visibility) ── + + # System admin sees everything (within tenant) + op.execute(""" + CREATE POLICY contacts_admin_select ON contacts + FOR SELECT + USING ( + current_setting('app.is_system_admin', true) = 'true' + AND tenant_id = current_setting('app.current_tenant_id', true)::uuid + ) + """) + + # Owner sees own rows (within tenant) + op.execute(""" + CREATE POLICY contacts_owner_select ON contacts + FOR SELECT + USING ( + owner_id::text = current_setting('app.current_user_id', true) + AND tenant_id = current_setting('app.current_tenant_id', true)::uuid + ) + """) + + # Tenant-owned (owner_id IS NULL) visible to all in tenant + op.execute(""" + CREATE POLICY contacts_tenant_owned_select ON contacts + FOR SELECT + USING ( + owner_id IS NULL + AND tenant_id = current_setting('app.current_tenant_id', true)::uuid + ) + """) + + # Shared via entity_permissions (within tenant) + op.execute(""" + CREATE POLICY contacts_shared_select ON contacts + FOR SELECT + USING ( + EXISTS ( + SELECT 1 FROM entity_permissions ep + WHERE ep.entity_type = 'contact' + AND ep.entity_id = contacts.id + AND ep.tenant_id = contacts.tenant_id + AND ep.permission_level != 'none' + AND ( + ep.expires_at IS NULL OR ep.expires_at > NOW() + ) + AND ( + (ep.principal_type = 'user' + AND ep.principal_id::text = current_setting('app.current_user_id', true)) + OR + (ep.principal_type = 'group' + AND ep.principal_id::text = ANY( + string_to_array(current_setting('app.current_user_groups', true), ',') + )) + OR + (ep.principal_type = 'role' + AND ep.principal_id IN ( + SELECT ut.role_id FROM user_tenants ut + WHERE ut.user_id::text = current_setting('app.current_user_id', true) + AND ut.tenant_id = contacts.tenant_id + )) + ) + ) + AND tenant_id = current_setting('app.current_tenant_id', true)::uuid + ) + """) + + # ── Permissive policies for INSERT ── + op.execute(""" + CREATE POLICY contacts_insert_policy ON contacts + FOR INSERT + WITH CHECK ( + tenant_id = current_setting('app.current_tenant_id', true)::uuid + AND ( + current_setting('app.is_system_admin', true) = 'true' + OR owner_id::text = current_setting('app.current_user_id', true) + OR owner_id IS NULL + ) + ) + """) + + # ── Permissive policies for UPDATE ── + op.execute(""" + CREATE POLICY contacts_update_policy ON contacts + FOR UPDATE + USING ( + tenant_id = current_setting('app.current_tenant_id', true)::uuid + AND ( + current_setting('app.is_system_admin', true) = 'true' + OR owner_id::text = current_setting('app.current_user_id', true) + OR owner_id IS NULL + OR EXISTS ( + SELECT 1 FROM entity_permissions ep + WHERE ep.entity_type = 'contact' + AND ep.entity_id = contacts.id + AND ep.tenant_id = contacts.tenant_id + AND ep.permission_level IN ('write', 'admin', 'delete') + AND ( + ep.expires_at IS NULL OR ep.expires_at > NOW() + ) + AND ( + (ep.principal_type = 'user' + AND ep.principal_id::text = current_setting('app.current_user_id', true)) + OR + (ep.principal_type = 'group' + AND ep.principal_id::text = ANY( + string_to_array(current_setting('app.current_user_groups', true), ',') + )) + ) + ) + ) + ) + WITH CHECK ( + tenant_id = current_setting('app.current_tenant_id', true)::uuid + ) + """) + + # ── Permissive policies for DELETE ── + op.execute(""" + CREATE POLICY contacts_delete_policy ON contacts + FOR DELETE + USING ( + tenant_id = current_setting('app.current_tenant_id', true)::uuid + AND ( + current_setting('app.is_system_admin', true) = 'true' + OR owner_id::text = current_setting('app.current_user_id', true) + OR EXISTS ( + SELECT 1 FROM entity_permissions ep + WHERE ep.entity_type = 'contact' + AND ep.entity_id = contacts.id + AND ep.tenant_id = contacts.tenant_id + AND ep.permission_level IN ('admin', 'delete') + AND ( + ep.expires_at IS NULL OR ep.expires_at > NOW() + ) + AND ( + (ep.principal_type = 'user' + AND ep.principal_id::text = current_setting('app.current_user_id', true)) + OR + (ep.principal_type = 'group' + AND ep.principal_id::text = ANY( + string_to_array(current_setting('app.current_user_groups', true), ',') + )) + ) + ) + ) + ) + """) + + +def downgrade() -> None: + # Drop the new secure policies + op.execute("DROP POLICY IF EXISTS contacts_tenant_isolation ON contacts") + op.execute("DROP POLICY IF EXISTS contacts_admin_select ON contacts") + op.execute("DROP POLICY IF EXISTS contacts_owner_select ON contacts") + op.execute("DROP POLICY IF EXISTS contacts_tenant_owned_select ON contacts") + op.execute("DROP POLICY IF EXISTS contacts_shared_select ON contacts") + op.execute("DROP POLICY IF EXISTS contacts_insert_policy ON contacts") + op.execute("DROP POLICY IF EXISTS contacts_update_policy ON contacts") + op.execute("DROP POLICY IF EXISTS contacts_delete_policy ON contacts") diff --git a/alembic/versions/0061_db_roles_secure.py b/alembic/versions/0061_db_roles_secure.py new file mode 100644 index 0000000..fce76d5 --- /dev/null +++ b/alembic/versions/0061_db_roles_secure.py @@ -0,0 +1,67 @@ +"""Fix DB roles — add default privileges and grants for all tables. + +Revision ID: 0061 +Revises: 0060 +Create Date: 2026-07-29 + +Problems fixed: +1. crm_runtime role has no grants on tables created after migration 0044 +2. No ALTER DEFAULT PRIVILEGES for future tables +3. Auth tables (users, tenants, user_tenants, user_groups) need SELECT grants +4. New permission/guest/policy tables need grants +""" + +from alembic import op + +revision = "0061" +down_revision = "0060" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # Grant privileges on all existing tables to crm_runtime + op.execute("GRANT SELECT, INSERT, UPDATE, DELETE ON ALL TABLES IN SCHEMA public TO crm_runtime") + + # Grant USAGE on sequences + op.execute("GRANT USAGE, SELECT ON ALL SEQUENCES IN SCHEMA public TO crm_runtime") + + # Default privileges for future tables created by migration owner + op.execute("ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO crm_runtime") + op.execute("ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT USAGE, SELECT ON SEQUENCES TO crm_runtime") + + # Ensure RLS is enabled on all tenant tables that have tenant_id + # (covers tables created after migration 0044 that missed RLS) + tenant_tables = [ + "entity_permissions", + "entity_policies", + "permission_templates", + "permission_delegations", + "guest_users", + "guest_invitations", + "contact_folder_permissions", + ] + for table in tenant_tables: + op.execute(f"ALTER TABLE {table} ENABLE ROW LEVEL SECURITY") + # Create tenant isolation policy if not exists + op.execute(f""" + DO $$ + BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_policy + WHERE polname = '{table}_tenant_isolation' + AND polrelid = '{table}'::regclass + ) THEN + CREATE POLICY {table}_tenant_isolation ON {table} + FOR ALL + USING (tenant_id = current_setting('app.current_tenant_id', true)::uuid) + WITH CHECK (tenant_id = current_setting('app.current_tenant_id', true)::uuid); + END IF; + END $$; + """) + + +def downgrade() -> None: + # Revoke default privileges + op.execute("ALTER DEFAULT PRIVILEGES IN SCHEMA public REVOKE SELECT, INSERT, UPDATE, DELETE ON TABLES FROM crm_runtime") + op.execute("ALTER DEFAULT PRIVILEGES IN SCHEMA public REVOKE USAGE, SELECT ON SEQUENCES FROM crm_runtime") diff --git a/app/core/db/__init__.py b/app/core/db/__init__.py index e98bd67..1967a19 100644 --- a/app/core/db/__init__.py +++ b/app/core/db/__init__.py @@ -99,10 +99,19 @@ async def get_db() -> AsyncGenerator[AsyncSession, None]: async def set_tenant_context(session: AsyncSession, tenant_id: uuid.UUID | str) -> None: - """Set PostgreSQL session variable for RLS tenant context.""" + """Set PostgreSQL session variable for RLS tenant context. + + Sets both app.current_tenant_id (new standard) and app.tenant_id + (legacy, used by migration 0044 policies) for backward compatibility. + """ + tid = str(tenant_id) await session.execute( text("SELECT set_config('app.current_tenant_id', :tid, true)"), - {"tid": str(tenant_id)}, + {"tid": tid}, + ) + await session.execute( + text("SELECT set_config('app.tenant_id', :tid, true)"), + {"tid": tid}, ) diff --git a/app/core/permission_registry.py b/app/core/permission_registry.py index faf322b..fd82fdf 100644 --- a/app/core/permission_registry.py +++ b/app/core/permission_registry.py @@ -128,17 +128,31 @@ class PermissionRegistry: self._core_field_definitions: list[dict[str, str]] = list(CORE_FIELD_DEFINITIONS) def initialize(self, active_plugin_names: set[str] | None = None) -> None: - """Build the registry from core permissions and active plugin manifests.""" + """Build the registry from core permissions and active plugin manifests. + + Preserves already-registered plugin permissions (fixes P1.3 bug where + initialize() would wipe plugin permissions registered before startup). + """ + # Preserve existing plugin permissions + existing_plugin_perms = self._plugin_permissions.copy() + + # Reset only core permissions, keep plugin permissions self._permissions = {} - self._plugin_permissions = {} self._active_plugins = active_plugin_names or set() # Register core permissions for perm in CORE_PERMISSIONS: self._permissions[perm["key"]] = perm + # Re-apply plugin permissions that were registered before initialize() + for plugin_name, perms in existing_plugin_perms.items(): + self._plugin_permissions[plugin_name] = perms + for entry in perms: + self._permissions[entry["key"]] = entry + self._initialized = True - logger.info("Permission registry initialized with %d core permissions", len(CORE_PERMISSIONS)) + logger.info("Permission registry initialized with %d core permissions, %d plugin permissions", + len(CORE_PERMISSIONS), len(existing_plugin_perms)) def register_plugin_permissions(self, plugin_name: str, permissions: list[str]) -> None: """Register permissions from a plugin manifest.""" diff --git a/app/deps.py b/app/deps.py index 1cddb71..2ea937b 100644 --- a/app/deps.py +++ b/app/deps.py @@ -127,6 +127,20 @@ async def get_current_user( is_admin = session_data.get("is_system_admin", False) await set_user_context(db, user_id, group_ids, is_admin) + # Check membership status (P1.7: suspended membership should not be usable) + from app.models.user import UserTenant + membership_q = await db.execute( + select(UserTenant.status) + .where(UserTenant.user_id == user_id) + .where(UserTenant.tenant_id == tenant_id) + ) + membership_status = membership_q.scalar_one_or_none() + if membership_status is not None and membership_status != "active": + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail={"detail": f"Mitgliedschaft ist {membership_status}, Zugriff verweigert", "code": "membership_suspended"}, + ) + # Load resolved permissions from cache (or DB on miss) from app.core.permissions import get_cached_permissions diff --git a/app/plugins/builtins/report_generator/pdf_generator.py b/app/plugins/builtins/report_generator/pdf_generator.py index df7478c..cb32db3 100644 --- a/app/plugins/builtins/report_generator/pdf_generator.py +++ b/app/plugins/builtins/report_generator/pdf_generator.py @@ -8,7 +8,8 @@ from datetime import datetime, timezone from pathlib import Path from typing import Any -from jinja2 import Environment, FileSystemLoader, select_autoescape +from jinja2 import Environment, FileSystemLoader, select_autoescape, StrictUndefined +from jinja2.sandbox import SandboxedEnvironment # ─── Constants ────────────────────────────────────────────────────────────── @@ -63,14 +64,17 @@ PRESET_META: list[dict[str, Any]] = [ # ─── Jinja2 Environment ───────────────────────────────────────────────────── -def _get_env() -> Environment: - """Create a Jinja2 environment with file system loader for templates dir.""" - return Environment( +def _get_env() -> SandboxedEnvironment: + """Create a sandboxed Jinja2 environment with file system loader for templates dir.""" + env = SandboxedEnvironment( loader=FileSystemLoader(str(TEMPLATES_DIR)), autoescape=select_autoescape(["html", "htm", "j2", "xml"]), trim_blocks=True, lstrip_blocks=True, + undefined=StrictUndefined, ) + env.globals.clear() + return env # ─── Public API ───────────────────────────────────────────────────────────── @@ -111,11 +115,13 @@ def render_template_string(template_content: str, data: dict[str, Any]) -> str: Returns: Rendered HTML string """ - env = Environment( + env = SandboxedEnvironment( autoescape=select_autoescape(["html", "htm", "xml"]), trim_blocks=True, lstrip_blocks=True, + undefined=StrictUndefined, ) + env.globals.clear() template = env.from_string(template_content) if "generated_at" not in data: data["generated_at"] = datetime.now(timezone.utc).strftime( diff --git a/app/plugins/builtins/report_generator/routes.py b/app/plugins/builtins/report_generator/routes.py index 3b86708..cb4fff2 100644 --- a/app/plugins/builtins/report_generator/routes.py +++ b/app/plugins/builtins/report_generator/routes.py @@ -77,10 +77,12 @@ def _report_to_response(r: ReportInstance) -> dict: def _render_jinja2(template_content: str, data: dict) -> str: - """Render a Jinja2 template string with the given data.""" - from jinja2 import Environment, StrictUndefined + """Render a Jinja2 template string with the given data (sandboxed).""" + from jinja2 import StrictUndefined + from jinja2.sandbox import SandboxedEnvironment - env = Environment(autoescape=False, undefined=StrictUndefined) + env = SandboxedEnvironment(autoescape=True, undefined=StrictUndefined) + env.globals.clear() template = env.from_string(template_content) return template.render(**data) diff --git a/app/services/attachment_service.py b/app/services/attachment_service.py index f6a8d0f..392d202 100644 --- a/app/services/attachment_service.py +++ b/app/services/attachment_service.py @@ -47,8 +47,18 @@ async def save_attachment( filename: str, file_content: bytes, mime_type: str, + is_system_admin: bool = False, ) -> dict[str, Any]: """Save a file to storage and create an Attachment record.""" + # Check access on parent entity + if not is_system_admin: + from app.core.visibility import check_single_entity_access + has_access = await check_single_entity_access( + db, entity_type, entity_id, user_id, tenant_id, "write", is_system_admin + ) + if not has_access: + raise PermissionError(f"No write access to {entity_type} {entity_id}") + # Generate unique filename and relative storage path unique_filename = _generate_unique_filename(filename) file_path = f"{entity_type}/{entity_id}/{unique_filename}" diff --git a/app/services/entity_permission_service.py b/app/services/entity_permission_service.py index b83374e..54e1042 100644 --- a/app/services/entity_permission_service.py +++ b/app/services/entity_permission_service.py @@ -27,9 +27,70 @@ from app.models.entity_permission import EntityPermission from app.models.group import Group, UserGroup from app.models.role import Role from app.models.user import User, UserTenant +from app.models.contact import Contact +from app.models.address import Address +from app.models.attachment import Attachment +from app.models.bank_account import BankAccount +from app.models.workflow import Workflow +from app.models.sequence import Sequence +from app.models.saved_filter import SavedFilter +from app.models.saved_view import SavedView +from app.models.webhook import Webhook +from app.models.notification import Notification +from app.models.custom_field_definition import CustomFieldDefinition +from app.models.contact_folder import ContactFolder logger = logging.getLogger(__name__) +# ── Entity Model Registry ──────────────────────────────────────────────────── +# Maps entity_type string to SQLAlchemy model class. +# This replaces insecure text(f"SELECT ... FROM {entity_type}s") queries +# with safe SQLAlchemy model-based queries (prevents SQL injection). +ENTITY_MODELS: dict[str, type] = { + "contact": Contact, + "address": Address, + "attachment": Attachment, + "bank_account": BankAccount, + "workflow": Workflow, + "sequence": Sequence, + "saved_filter": SavedFilter, + "saved_view": SavedView, + "webhook": Webhook, + "notification": Notification, + "custom_field_definition": CustomFieldDefinition, + "contact_folder": ContactFolder, +} + +# Try to add plugin models if available +try: + from app.plugins.builtins.dms.models import DmsFile + ENTITY_MODELS["dms_file"] = DmsFile +except ImportError: + pass +try: + from app.plugins.builtins.calendar.models import CalendarEvent + ENTITY_MODELS["calendar_event"] = CalendarEvent +except ImportError: + pass +try: + from app.plugins.builtins.tasks.models import Task + ENTITY_MODELS["task"] = Task +except ImportError: + pass +try: + from app.plugins.builtins.mail.models import Mailbox + ENTITY_MODELS["mailbox"] = Mailbox +except ImportError: + pass + + +def _get_entity_model(entity_type: str) -> type: + """Get SQLAlchemy model class for entity_type, or raise ValueError.""" + model = ENTITY_MODELS.get(entity_type) + if model is None: + raise ValueError(f"Unknown entity type: {entity_type}") + return model + # Permission hierarchy: higher = more access _PERM_RANK = {"none": 0, "read": 1, "write": 2, "admin": 3, "delete": 4, "owner": 5} @@ -350,11 +411,10 @@ async def get_effective_access( if user_q.scalar(): return "delete" - # Check ownership — load the entity's owner_id - # We use raw SQL to avoid importing every model + # Check ownership — load the entity's owner_id via SQLAlchemy model (safe from SQL injection) + model = _get_entity_model(entity_type) owner_q = await db.execute( - text(f"SELECT owner_id FROM {entity_type}s WHERE id = :eid AND tenant_id = :tid"), - {"eid": entity_id, "tid": tenant_id}, + select(model.owner_id).where(model.id == entity_id).where(model.tenant_id == tenant_id) ) owner_row = owner_q.first() if not owner_row: @@ -460,10 +520,11 @@ async def get_visible_ids( ) if user_q.scalar(): # Return all entity IDs - all_q = await db.execute( - text(f"SELECT id FROM {entity_type}s WHERE tenant_id = :tid AND deleted_at IS NULL"), - {"tid": tenant_id}, - ) + model = _get_entity_model(entity_type) + admin_q = select(model.id).where(model.tenant_id == tenant_id) + if hasattr(model, 'deleted_at'): + admin_q = admin_q.where(model.deleted_at.is_(None)) + all_q = await db.execute(admin_q) all_ids = {row[0] for row in all_q} return all_ids, {eid: "delete" for eid in all_ids} @@ -483,10 +544,11 @@ async def get_visible_ids( role_id = role_q.scalar_one_or_none() # 1. Owned entities - owned_q = await db.execute( - text(f"SELECT id FROM {entity_type}s WHERE tenant_id = :tid AND owner_id = :uid AND deleted_at IS NULL"), - {"tid": tenant_id, "uid": user_id}, - ) + model = _get_entity_model(entity_type) + owned_q_builder = select(model.id).where(model.tenant_id == tenant_id).where(model.owner_id == user_id) + if hasattr(model, 'deleted_at'): + owned_q_builder = owned_q_builder.where(model.deleted_at.is_(None)) + owned_q = await db.execute(owned_q_builder) visible: set[uuid.UUID] = set() access_map: dict[uuid.UUID, str] = {} for (eid,) in owned_q: @@ -494,10 +556,10 @@ async def get_visible_ids( access_map[eid] = "owner" # 2. Tenant-owned entities (owner_id IS NULL) - tenant_owned_q = await db.execute( - text(f"SELECT id FROM {entity_type}s WHERE tenant_id = :tid AND owner_id IS NULL AND deleted_at IS NULL"), - {"tid": tenant_id}, - ) + tenant_q_builder = select(model.id).where(model.tenant_id == tenant_id).where(model.owner_id.is_(None)) + if hasattr(model, 'deleted_at'): + tenant_q_builder = tenant_q_builder.where(model.deleted_at.is_(None)) + tenant_owned_q = await db.execute(tenant_q_builder) for (eid,) in tenant_owned_q: if eid not in visible: visible.add(eid) @@ -591,16 +653,17 @@ async def batch_get_effective_access( result: dict[uuid.UUID, str] = {} - # 1. Check ownership - owner_q = await db.execute( - text(f"SELECT id, owner_id FROM {entity_type}s WHERE id = ANY(:ids) AND tenant_id = :tid AND deleted_at IS NULL"), - {"ids": [str(eid) for eid in entity_ids], "tid": tenant_id}, - ) + # 1. Check ownership via SQLAlchemy model (safe from SQL injection) + model = _get_entity_model(entity_type) + batch_q = select(model.id, model.owner_id).where(model.id.in_(entity_ids)).where(model.tenant_id == tenant_id) + if hasattr(model, 'deleted_at'): + batch_q = batch_q.where(model.deleted_at.is_(None)) + owner_q = await db.execute(batch_q) for eid, owner_id in owner_q: if owner_id == user_id: - result[uuid.UUID(str(eid))] = "owner" + result[eid] = "owner" elif owner_id is None: - result[uuid.UUID(str(eid))] = "read" + result[eid] = "read" # 2. Check permissions now = datetime.now(UTC) diff --git a/tests/test_abac.py b/tests/test_abac.py index 45e5060..e289952 100644 --- a/tests/test_abac.py +++ b/tests/test_abac.py @@ -421,4 +421,3 @@ class TestABACPolicyService: db_session, tenant_id, policy["id"], enabled=False ) assert updated["enabled"] is False -} diff --git a/tests/test_entity_permissions.py b/tests/test_entity_permissions.py index eacda88..6b18ebd 100644 --- a/tests/test_entity_permissions.py +++ b/tests/test_entity_permissions.py @@ -403,4 +403,3 @@ class TestEntityPermissions: db_session, tenant_id, viewer_id, "contact", contact_id ) assert access == "none", "Permission should be gone after cleanup" -} diff --git a/tests/test_permission_performance.py b/tests/test_permission_performance.py index a7aa6f4..bd8a2e8 100644 --- a/tests/test_permission_performance.py +++ b/tests/test_permission_performance.py @@ -286,4 +286,3 @@ class TestPermissionPerformance: assert elapsed < 3.0, f"get_visible_ids with mixed ownership took {elapsed:.3f}s (expected <3.0s)" # Viewer should see: 200 shared + 300 tenant-owned = 500 assert len(visible) >= 500, f"Expected >=500 visible, got {len(visible)}" -}