From 795307754ff0d370d3316b6bbe81940d930fed42 Mon Sep 17 00:00:00 2001 From: Agent Zero Date: Sun, 23 Aug 2026 18:35:50 +0200 Subject: [PATCH] fix(arch-008,arch-009): canonical 2-segment permission schema enforced; fix dead role wildcard patterns --- .../0141_fix_role_permission_patterns.py | 84 +++++++++++++++++++ app/plugins/manifest.py | 21 ++++- app/routes/delegations.py | 10 +-- app/routes/permission_templates.py | 10 +-- app/routes/policies.py | 8 +- docs/permissions.md | 21 +++++ tests/test_arch_block_a.py | 68 +++++++++++++++ 7 files changed, 207 insertions(+), 15 deletions(-) create mode 100644 alembic/versions/0141_fix_role_permission_patterns.py diff --git a/alembic/versions/0141_fix_role_permission_patterns.py b/alembic/versions/0141_fix_role_permission_patterns.py new file mode 100644 index 0000000..179b2eb --- /dev/null +++ b/alembic/versions/0141_fix_role_permission_patterns.py @@ -0,0 +1,84 @@ +'''Fix role permission wildcard patterns to canonical 2-segment schema + +Revision ID: 0141 +Revises: 0140 +Create Date: 2026-08-23 + +Migration 0019 seeded default roles with 3-segment permission patterns +(core:*:read etc.). The runtime matcher (_matches_permission) compares +segment counts strictly, so those patterns could never match any +2-segment requirement - editor/viewer roles were silently dead. + +Canonical schema is module:action (2 segments, * wildcards allowed). +core:*:X means all modules with action X, so it converts to *:X. +''' + +from alembic import op + +# revision identifiers, used by Alembic. +revision = '0141' +down_revision = '0140' +branch_labels = None +depends_on = None + +# Rebuild the permissions JSONB object, rewriting every key that starts +# with the dead 'core:' prefix to its 2-segment equivalent ('*:X'). +_UPGRADE_SQL = ''' +UPDATE roles +SET permissions = sub.new_perms, + permission_version = permission_version + 1 +FROM ( + SELECT + r.id AS role_id, + jsonb_object_agg( + CASE WHEN k LIKE 'core:%' + THEN '*:' || split_part(k, ':', 3) + ELSE k END, + v + ) AS new_perms + FROM roles r, + jsonb_each(r.permissions) AS e(k, v) + GROUP BY r.id +) AS sub +WHERE roles.id = sub.role_id + AND EXISTS ( + SELECT 1 FROM jsonb_object_keys(roles.permissions) k + WHERE k LIKE 'core:%' + ) +''' + +# Reverse: map '*:X' back to 'core:*:X' only for keys that came from the +# original seeding pattern. Roles that legitimately use '*:X' without a +# matching 'core:*:X' history are left untouched (best-effort downgrade). +_DOWNGRADE_SQL = ''' +UPDATE roles +SET permissions = sub.new_perms, + permission_version = permission_version + 1 +FROM ( + SELECT + r.id AS role_id, + jsonb_object_agg( + CASE WHEN k = '*:' || split_part(k, ':', 2) + AND k <> '*:*' + THEN 'core:*:' || split_part(k, ':', 2) + ELSE k END, + v + ) AS new_perms + FROM roles r, + jsonb_each(r.permissions) AS e(k, v) + GROUP BY r.id +) AS sub +WHERE roles.id = sub.role_id + AND EXISTS ( + SELECT 1 FROM jsonb_object_keys(roles.permissions) k + WHERE k = '*:' || split_part(k, ':', 2) AND k <> '*:*' + ) +''' + + +def upgrade() -> None: + op.execute(_UPGRADE_SQL) + + +def downgrade() -> None: + op.execute(_DOWNGRADE_SQL) diff --git a/app/plugins/manifest.py b/app/plugins/manifest.py index 8c97e19..0661f61 100644 --- a/app/plugins/manifest.py +++ b/app/plugins/manifest.py @@ -191,6 +191,25 @@ class PluginManifest(BaseModel): permissions: list[str] = Field( default_factory=list, description="Required permissions for this plugin" ) + + @field_validator("permissions") + @classmethod + def _validate_permission_format(cls, v: list[str]) -> list[str]: + """Enforce the canonical 2-segment permission schema (ARCH-008/009). + + Canonical form is ``module:action`` with ``*`` wildcards allowed in + either segment (e.g. ``contacts:read``, ``contacts:*``, ``*:read``, + ``*:*``). 3-segment names like ``core:contacts:read`` never match + the runtime matcher and are rejected at manifest load time. + """ + for perm in v: + parts = perm.split(":") + if len(parts) != 2 or not all(parts): + raise ValueError( + f"Invalid permission {perm!r}: use 2-segment 'module:action' " + f"(wildcards '*' allowed), e.g. 'contacts:read' or '*:*'" + ) + return v is_core: bool = Field( default=False, description="Whether this is a core plugin that cannot be deactivated" ) @@ -423,7 +442,7 @@ MANIFEST_SCHEMA_DOC = ManifestSchemaResponse( routes=[], events=["contact.created"], migrations=["0001_initial.sql"], - permissions=["contacts.read"], + permissions=["contacts:read"], menu_items=[ FrontendMenuItem( label_key="nav.examplePlugin", diff --git a/app/routes/delegations.py b/app/routes/delegations.py index 4a49309..5f718d0 100644 --- a/app/routes/delegations.py +++ b/app/routes/delegations.py @@ -19,7 +19,7 @@ router = APIRouter(prefix="/api/v1/delegations", tags=["delegations"]) async def list_delegations( direction: str = Query("all", pattern="^(from|to|all)$"), db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:delegations:read")), + current_user: dict = Depends(require_permission("delegations:read")), ): """List delegations for the current user.""" tenant_id = uuid.UUID(current_user["tenant_id"]) @@ -32,7 +32,7 @@ async def list_delegations( async def create_delegation( body: DelegationCreate, db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:delegations:write")), + current_user: dict = Depends(require_permission("delegations:write")), ): """Create a new permission delegation.""" tenant_id = uuid.UUID(current_user["tenant_id"]) @@ -56,7 +56,7 @@ async def update_delegation( delegation_id: str, body: DelegationUpdate, db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:delegations:write")), + current_user: dict = Depends(require_permission("delegations:write")), ): """Update an existing delegation.""" tenant_id = uuid.UUID(current_user["tenant_id"]) @@ -78,7 +78,7 @@ async def update_delegation( async def delete_delegation( delegation_id: str, db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:delegations:write")), + current_user: dict = Depends(require_permission("delegations:write")), ): """Delete a delegation.""" tenant_id = uuid.UUID(current_user["tenant_id"]) @@ -91,7 +91,7 @@ async def delete_delegation( @router.get("/active") async def check_active_delegation( db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:delegations:read")), + current_user: dict = Depends(require_permission("delegations:read")), ): """Check if the current user has any active delegations.""" tenant_id = uuid.UUID(current_user["tenant_id"]) diff --git a/app/routes/permission_templates.py b/app/routes/permission_templates.py index 948bf7a..30d6a0b 100644 --- a/app/routes/permission_templates.py +++ b/app/routes/permission_templates.py @@ -23,7 +23,7 @@ router = APIRouter(prefix="/api/v1/permission-templates", tags=["permission-temp async def list_templates( entity_type: str | None = None, db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:templates:read")), + current_user: dict = Depends(require_permission("templates:read")), ): """List all permission templates for the current tenant.""" tenant_id = uuid.UUID(current_user["tenant_id"]) @@ -35,7 +35,7 @@ async def list_templates( async def create_template( body: PermissionTemplateCreate, db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:templates:write")), + current_user: dict = Depends(require_permission("templates:write")), ): """Create a new permission template.""" tenant_id = uuid.UUID(current_user["tenant_id"]) @@ -58,7 +58,7 @@ async def update_template( template_id: str, body: PermissionTemplateUpdate, db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:templates:write")), + current_user: dict = Depends(require_permission("templates:write")), ): """Update an existing permission template.""" tenant_id = uuid.UUID(current_user["tenant_id"]) @@ -81,7 +81,7 @@ async def update_template( async def delete_template( template_id: str, db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:templates:write")), + current_user: dict = Depends(require_permission("templates:write")), ): """Delete a permission template.""" tenant_id = uuid.UUID(current_user["tenant_id"]) @@ -95,7 +95,7 @@ async def delete_template( async def apply_template( body: PermissionTemplateApply, db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:templates:write")), + current_user: dict = Depends(require_permission("templates:write")), ): """Apply a permission template to an entity, creating entity_permissions.""" tenant_id = uuid.UUID(current_user["tenant_id"]) diff --git a/app/routes/policies.py b/app/routes/policies.py index 2e8229a..94c3758 100644 --- a/app/routes/policies.py +++ b/app/routes/policies.py @@ -19,7 +19,7 @@ router = APIRouter(prefix="/api/v1/policies", tags=["policies"]) async def list_policies( entity_type: str, db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:policies:read")), + current_user: dict = Depends(require_permission("policies:read")), ): """List all ABAC policies for a given entity type.""" tenant_id = uuid.UUID(current_user["tenant_id"]) @@ -34,7 +34,7 @@ async def list_policies( async def create_policy( body: PolicyCreate, db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:policies:write")), + current_user: dict = Depends(require_permission("policies:write")), ): """Create a new ABAC policy.""" tenant_id = uuid.UUID(current_user["tenant_id"]) @@ -59,7 +59,7 @@ async def update_policy( policy_id: str, body: PolicyUpdate, db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:policies:write")), + current_user: dict = Depends(require_permission("policies:write")), ): """Update an existing ABAC policy.""" tenant_id = uuid.UUID(current_user["tenant_id"]) @@ -85,7 +85,7 @@ async def update_policy( async def delete_policy( policy_id: str, db: AsyncSession = Depends(get_db), - current_user: dict = Depends(require_permission("permissions:policies:write")), + current_user: dict = Depends(require_permission("policies:write")), ): """Delete an ABAC policy.""" tenant_id = uuid.UUID(current_user["tenant_id"]) diff --git a/docs/permissions.md b/docs/permissions.md index a112487..5d4e0dd 100644 --- a/docs/permissions.md +++ b/docs/permissions.md @@ -39,6 +39,27 @@ When checking access to an entity, the system resolves in this order (highest wi | `delete` | 4 | Admin + transfer ownership | | `owner` | 5 | Full control (automatic for owner) | +### Permission Name Schema (canonical) + +All module-level permission strings follow the strict **2-segment schema** +`module:action`, with `*` wildcards allowed in either segment: + +| Pattern | Meaning | +|---------|---------| +| `contacts:read` | Exact: read contacts | +| `contacts:*` | All actions on contacts | +| `*:read` | Read on all modules | +| `*:*` | Everything (superadmin) | + +The runtime matcher (`app/core/permissions.py::_matches_permission`) compares +segment counts strictly — a 3-segment grant like `core:contacts:read` can never +match any 2-segment requirement and is therefore **invalid**. The plugin manifest +validator (`app/plugins/manifest.py`) rejects such patterns at load time. + +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. + --- ## 2. Data Model diff --git a/tests/test_arch_block_a.py b/tests/test_arch_block_a.py index e8f720c..315dff7 100644 --- a/tests/test_arch_block_a.py +++ b/tests/test_arch_block_a.py @@ -129,3 +129,71 @@ class TestGetFileMetadataAsync: meta = get_file_metadata('nonexistent/sync.txt') assert meta == {'size': None, 'modified': None, 'exists': False} + + +# --- ARCH-008/009: 2-segment permission canonical schema --- + + +import re as _re + +from app.core.permissions import _matches_permission + +_THREE_SEGMENT = _re.compile(r'^[a-z_]+:[a-z_]+:[a-z_*]+$') + + +class TestMatchesPermissionCanonical: + def test_two_segment_wildcards(self): + assert _matches_permission('*:*', 'contacts:read') is True + assert _matches_permission('contacts:*', 'contacts:read') is True + assert _matches_permission('*:read', 'contacts:read') is True + assert _matches_permission('contacts:read', 'contacts:read') is True + + def test_three_segment_grant_never_matches(self): + assert _matches_permission('core:*:read', 'contacts:read') is False + + def test_segment_count_mismatch(self): + assert _matches_permission('contacts:read', 'contacts:read:extra') is False + + +class TestRouteLiteralsTwoSegment: + def test_no_three_segment_literals_in_routes(self): + import pathlib + + routes_dir = pathlib.Path('/a0/usr/projects/leocrm/app/routes') + offenders = [] + for py_file in sorted(routes_dir.glob('*.py')): + text = py_file.read_text() + for m in _re.finditer(r'require_permission\(([^)]*)\)', text): + arg = m.group(1).strip() + if not arg: + continue + value = arg.strip('\'"') + if ':' in value and value.count(':') != 1: + offenders.append(f'{py_file.name}: {value}') + assert offenders == [], f'3-segment permission literals found: {offenders}' + + +class TestManifestPermissionValidator: + def test_valid_permissions_accepted(self): + from app.plugins.manifest import PluginManifest + + manifest = PluginManifest( + name='x', + version='1.0.0', + display_name='X', + permissions=['contacts:read', '*:*'], + ) + assert manifest.permissions == ['contacts:read', '*:*'] + + def test_three_segment_rejected(self): + from pydantic import ValidationError + + from app.plugins.manifest import PluginManifest + + with pytest.raises(ValidationError): + PluginManifest( + name='x', + version='1.0.0', + display_name='X', + permissions=['core:contacts:read'], + )