From d068366a61337f37775a58cfc515bdb6530ff086 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Sun, 24 May 2026 18:31:23 -0700 Subject: [PATCH] rbac: builtin-role override editor + tighten under-enforced perm gates (#585) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(rbac): editable builtin role permissions via overlay layer Adds a ``role_permission_overrides`` table that stores per-(role_id, permission) grant/revoke deltas, applied on top of the immutable ``roles.permissions`` baseline at permission-load time. Builtin roles (``builtin-admin/operator/viewer``) become customizable through the admin Roles UI without losing the "reset to default" guarantee — every override is auditable and reversible. Motivating case: ``model.skills.write`` is deliberately default-ungranted on every role so operators must consciously opt in before a coordinator session can mutate the skill catalog. Until now there was no UX path to do that opt-in — the only options were dropping into SQL or running a fresh migration. The overrides editor closes that gap. Backend - Migration 057 + storage methods on both sqlite + postgresql backends - ``get_user_permissions`` merges baseline ∪ grants − revokes for builtin rows; custom rows pass through unchanged - ``GET /v1/api/admin/roles/{id}/effective`` for inspect - ``PUT /v1/api/admin/roles/{id}/overrides`` for write — admin.roles gated, audited, validates against ``_VALID_PERMISSIONS``, refuses non-builtin targets, strips no-op grants/revokes before persisting - Lockout guard: cannot revoke ``admin.roles`` if doing so would leave zero users with the permission (returns 409) - ``coordinator.trust.send`` added to ``_VALID_PERMISSIONS`` — was seeded into builtin-admin by migration 042 but never registered with the validator, so the very first round-trip through the editor 400'd on it. Drift-detection test guards future migrations from recreating the same gap Frontend - Roles tab redesign: chevron + permission-count chip replace the "..." truncation; expand-on-click drawer groups perms by namespace with baseline / grant (green +) / revoke (red −) chip variants - Edit modal opens for builtin rows ("Customize Built-in Role" title); toggles show baseline-default vs override state; submit diffs against the rendered toggle universe (not raw baseline) so future taxonomy drift can't silently strip unknown perms - "Modified +N/-N" pill on rows with active overrides; "Reset to default" drawer action clears the override set - ``_PERMISSION_SECTIONS`` brought up to date with all currently-seeded perms (admin.coordinator, admin.cluster.inspect, admin.models, admin.nodes, admin.prompt_policies, conversation.modify, coordinator.trust.send were missing) Tests - 7 storage tests covering set/list/clear/effective + overlay merge into ``get_user_permissions`` for both builtin and custom roles - 11 endpoint tests covering effective/overrides happy paths, validation, lockout guard, builtin-only restriction, no-op normalization, list enrichment * feat(rbac): enforce workstreams.{create,close} + tools.approve gates These three permissions were declared in ``_VALID_PERMISSIONS``, seeded into ``builtin-operator``'s baseline by migration 008/017, surfaced in the admin Roles UI as toggles, and documented in ``bootstrap.py`` as the operator role's capabilities — and never enforced anywhere. The audit that ran out of the overlay PR found zero ``require_permission`` sites for any of them; any authenticated user could create workstreams, close any workstream, or approve any pending tool regardless of role. Behaviour change for callers without the perms: - ``POST /v1/api/workstreams/new`` (node + console proxy variants) now 403 without ``workstreams.create`` - ``POST /v1/api/workstreams/{ws_id}/close`` (and ``/route/`` proxy) now 403 without ``workstreams.close`` - ``POST /v1/api/workstreams/{ws_id}/approve`` (and ``/route/`` proxy) now 403 without ``tools.approve`` The OR-fallback to ``admin.coordinator`` keeps coord sessions spawning interactive children unblocked without needing operator-style perms. Service-scoped inter-cluster calls bypass via the existing ``allow_service_bypass`` path on the new ``require_any_permission`` helper. Builtin admin and operator both already carry these perms; viewer correctly loses workstream create/close/approve (it already couldn't do those in spirit). Implementation - ``require_any_permission`` (core/auth.py) — OR-semantics variant of ``require_permission`` with per-conditional comments documenting the security policy at the choke point. 403 body names every accepted perm so operators get an actionable remediation - ``make_{create,close,approve}_handler`` (core/session_routes.py) accept ``fallback_permissions: tuple[str, ...]`` — checked only when ``cfg.permission_gate is None`` (interactive case). Coord's ``permission_gate=_require_admin_coordinator`` continues to take precedence on the coord-config side - Console-side ``create_workstream`` and ``route_create`` inline the same OR check before proxying — fail fast on a forbidden request without burning a cluster round-trip - ``route_proxy`` adds a verb-scoped gate on ``approve`` and ``close`` only; ``send``/``cancel``/``dequeue``/``command``/``plan`` remain authenticated-only (pre-existing, out of scope for this audit) Tests - New ``TestPermissionGatesOnLifecycle`` (4 tests) in test_server_authz pinning 403-without-perm + non-403-with-perm at the node lift sites - New ``TestRouteProxyPermissionGates`` (5 tests) in test_console_routing_proxy covering 403 paths, OR fallback via ``admin.coordinator``, and that ``send`` remains ungated - ``_make_jwt`` helpers in test_server_authz, test_close_reason_ persistence, test_server_attachments_on_create updated to embed operator-shaped perms by default so existing tests continue to exercise the post-gate logic rather than 403'ing on the new check Docs - ``bootstrap.py`` operator role line corrected to list every perm it actually carries (was missing ``tools.approve`` and ``conversation.modify``) * fix(rbac): close lockout + escalation gaps in role-overrides editor Three issues surfaced by /review of the overlay layer and gate uplift — all in the RBAC/auth surface, treated as zero-days. **F-1: lockout guard misses the grant-removal path.** PUT-replace semantics on ``set_role_overrides`` mean an existing grant of ``admin.roles`` (added via override to e.g. builtin-operator) is silently dropped when the new payload omits it. The previous guard short-circuited on ``"admin.roles" not in revokes`` and never noticed. Concrete cluster-bricking scenario: grant admin.roles to operator via override, unassign builtin-admin, click "Reset to default" on operator → all users lose admin.roles, recoverable only via SQL. The rewritten guard simulates the post-PUT effective set on the target role directly: if ``(baseline | new_grants) - new_revokes`` lacks admin.roles AND nobody holds it via another role, refuse the change. The "via another role" question is answered by one bulk query rather than the prior O(users × roles) round-trip loop. **F-3: lockout check blocked the event loop on moderate deployments.** The prior check called ``storage.list_user_roles`` per user and ``storage.effective_role_permissions`` per (user, role) pair — synchronous SQL inside an async handler. 200 users × 5 roles = 1000 connection cycles long enough to trip reverse-proxy timeouts on a permission revoke. Replaced with ``storage.users_with_permission(perm, *, exclude_role_id)`` — one join over ``user_roles ⋈ roles`` plus one IN fetch on overrides for the builtin role ids in the result, folded in-process. Two queries total, independent of cluster size. The whole check now runs under ``asyncio.to_thread`` so even the bulk read doesn't stall the loop. **F-2 reframed: admin_assign_role's subset check ignored the overlay.** The check at lines 6321-6328 reads ``target_role.get("permissions", "")`` (baseline column) when computing the perms it requires the caller to hold. After this branch, an admin.roles holder can grant e.g. ``model.skills.write`` to builtin-operator via override; an admin.users holder (who happens to NOT hold that perm) could then assign operator to a new user, silently escalating the assignee. The existing two-person-rule by perm split (admin.roles for catalog edits, admin.users for assignments) only holds if the assignment-time check considers the overlay. Switched ``target_perms`` to ``storage.effective_role_permissions(role_id)["effective"]``. Note: this PR retains the existing model where admin.roles is the catalog-edit superuser (admin_create_role, admin_update_role, and now admin_role_overrides all skip the caller-holds-grants check). The two-person rule against escalation lives at the assignment gate, which this fix reinforces. **F-7: delete_role left orphaned override rows.** No FK on ``role_permission_overrides.role_id`` (migration 057 omitted FKs to match the rest of the governance schema). Added explicit cleanup in both sqlite + postgresql ``delete_role`` implementations so a re-seeded role_id (deterministic for builtins on schema reseed) can't silently inherit stale overrides from the prior occupant. Tests - storage: ``test_users_with_permission_bulk`` exercises the new bulk helper including ``exclude_role_id`` and overlay folding - storage: ``test_delete_role_cleans_up_overrides`` pins the F-7 fix - endpoint: ``test_overrides_lockout_guard_blocks_grant_removal`` is the F-1 reproduction — operator-overlay grants admin.roles, builtin- admin has it removed, attempting to reset operator's overrides 409s - endpoint: ``test_assign_role_blocks_escalation_via_overlay_grant`` pins the F-2 reframed fix — overlay-poisoned operator can't be assigned by a caller missing the overlay perms * refactor(rbac): cleanup batch from /review (#584) Five non-security findings folded into one commit so the security batch stays focused. All consistent with the existing intent of ``feat/builtin-role-overrides``. **F-4: presence check on ``_effectivePerms``.** ``governance.js`` was guarding on ``Array.isArray(role.effective) && role.effective.length > 0``, falling through to splitting ``role.permissions`` (the baseline) when the array was empty. For a builtin role whose overrides legitimately revoke every baseline perm, that path silently rendered the baseline chips with no override indicators — the inspector lied about what the role can do. ``_enrich_role`` always sets ``effective: []``, so presence is the right sentinel. **F-5: JS-side drift detector.** Commit 1 added a Python-side test asserting ``_VALID_PERMISSIONS`` covers every baseline perm; the mirror invariant on the frontend went uncaught. A new perm added to ``_VALID_PERMISSIONS`` without a matching entry in ``_PERMISSION_SECTIONS`` becomes silently un-customizable through the admin UI (the only documented grant/revoke path). Test parses the JS const out via regex and asserts set-equality both directions — detects "missing in UI" and "extra in UI" so the toggle catalog and validator can't fork. **F-6: bulk enrich for ``admin_list_roles``.** Was ``1 + 2*builtin_count + 1*custom_count`` SELECTs per admin-tab open; collapsed to one ``IN``-filtered query via new ``storage.effective_role_permissions_bulk(role_ids)``. Implemented on both sqlite + postgresql backends following the existing ``effective_role_permissions`` shape. **F-8: rename ``fallback_permissions`` → ``accepted_permissions``.** The lift body uses ``if cfg.permission_gate / elif accepted_permissions`` — mutually exclusive — so when ``permission_gate`` is None this IS the primary gate, not a fallback to anything. The "fallback" name suggested a tier-2-after-tier-1 semantic that didn't exist. Renamed across ``make_{approve,close,create}_handler`` factories, the three call sites in ``turnstone/server.py``, and the docstrings. **F-9: positive lift-level tests for ``admin.coordinator``-only.** ``TestPermissionGatesOnLifecycle`` previously had a single positive test for ``workstreams.create`` alone, plus negative-403 tests for each verb without perms. The OR-fallback to ``admin.coordinator`` (which keeps coord sessions spawning interactive children unblocked) had no positive coverage at the lift code path — only at the proxy, which exercises a different verb-dict gate. Added three tests (create / close / approve) that pass ``admin.coordinator`` alone and assert non-403, so a future tightening of the accepted_permissions tuple can't silently regress coord-driven child workstreams. Out of scope: nit perf-4 (event-delegation refactor on ``_renderGovRoles``). ``setSafeHtml`` rebuild is the existing pattern across every admin tab; rewriting one tab's render path on this branch would be drive-by inconsistent with the surrounding codebase. Filed as a separate concern if the Roles tab grows past the scale where it bites. * fix(rbac-ui): aria-expanded + row-click on Roles drawer (#585) Two Copilot review findings on governance.js: - Expand button was missing aria-expanded — screen readers couldn't announce drawer state. Now reflects the row's expanded flag. - Comment said "row + chevron both work" but only the chevron was wired. Added data-expand-role to the row element too so the existing handler loop (querySelectorAll on the attribute) picks up both — clicking anywhere in the role row toggles the drawer. Edit/Delete handlers already stopPropagation so they aren't triggered by the row-level click. * fix(migrations): rebase role_permission_overrides to 058 PR #560 mitigation #1 landed 057_output_assessments_llm_judge.py on main in parallel; my migration claimed the same number, forking alembic's head and breaking postgres. Renumbered to 058 and re-pointed down_revision at 057 so the chain stays linear. No behaviour change — same DDL. Full sweep clean (6730 passed). * fix(migrations): update 058 revision strings to match filename Previous commit (ea86aefc) renamed 057_role_permission_overrides.py to 058_* but the in-file revision = "057" / down_revision = "056" strings stayed — leftover from when the file shipped as 057. Tests pass because alembic walks the chain by revision string, and the strings now correctly read revision = "058" / down_revision = "057" to make the chain linear with main's 057_output_assessments_llm_judge. Caught locally before re-running CI; my prior `git mv` + content edit landed as a staged rename + unstaged modification on the previous push. --- tests/test_close_reason_persistence.py | 5 +- tests/test_console_routing_proxy.py | 83 ++++ tests/test_governance_endpoints.py | 332 ++++++++++++++++ tests/test_governance_storage.py | 106 +++++ tests/test_server_attachments_on_create.py | 5 + tests/test_server_authz.py | 130 +++++- turnstone/api/console_schemas.py | 19 + turnstone/api/console_spec.py | 19 + turnstone/bootstrap.py | 2 +- turnstone/console/server.py | 241 +++++++++++- turnstone/console/static/governance.js | 371 ++++++++++++++++-- turnstone/console/static/style.css | 157 +++++++- turnstone/core/auth.py | 75 ++++ turnstone/core/session_routes.py | 33 +- turnstone/core/storage/_postgresql.py | 223 ++++++++++- turnstone/core/storage/_protocol.py | 72 +++- turnstone/core/storage/_schema.py | 13 + turnstone/core/storage/_sqlite.py | 223 ++++++++++- turnstone/core/storage/_utils.py | 7 + .../versions/058_role_permission_overrides.py | 61 +++ turnstone/server.py | 20 +- 21 files changed, 2129 insertions(+), 68 deletions(-) create mode 100644 turnstone/core/storage/migrations/versions/058_role_permission_overrides.py diff --git a/tests/test_close_reason_persistence.py b/tests/test_close_reason_persistence.py index cd431e0a..27f8cae4 100644 --- a/tests/test_close_reason_persistence.py +++ b/tests/test_close_reason_persistence.py @@ -23,9 +23,12 @@ _JWT_SECRET = "test-jwt-secret-minimum-32-chars!" def _full_hdr() -> dict[str, str]: + # ``workstreams.close`` is now a real gate on the close handler + # (was a vestigial perm, see PR adding 057_role_permission_overrides); + # tests that drive close need it embedded in the JWT. return { "Authorization": ( - f"Bearer {create_jwt('u1', frozenset({'read', 'write', 'approve'}), 'test', _JWT_SECRET, audience=JWT_AUD_SERVER)}" + f"Bearer {create_jwt('u1', frozenset({'read', 'write', 'approve'}), 'test', _JWT_SECRET, audience=JWT_AUD_SERVER, permissions=frozenset({'workstreams.close'}))}" ) } diff --git a/tests/test_console_routing_proxy.py b/tests/test_console_routing_proxy.py index 0a5d9f92..b6b21691 100644 --- a/tests/test_console_routing_proxy.py +++ b/tests/test_console_routing_proxy.py @@ -354,6 +354,89 @@ class TestRouteProxy: assert resp.status_code == 200 +class TestRouteProxyPermissionGates: + """``route_proxy`` was pre-existing infra that forwarded blindly — + any authenticated caller could send/approve/cancel/close. PR + adding 057_role_permission_overrides added verb-scoped gates on + approve + close (the verbs that had vestigial perms in + ``_VALID_PERMISSIONS`` with no enforcement site). These tests + pin the new shape and the OR fallback to ``admin.coordinator``.""" + + @pytest.fixture() + def client(self): + router = _make_mock_router() + app = _make_app(router=router) + _wire_proxy(app, _make_proxy_post(json_data={"status": "ok"})) + client = TestClient(app, raise_server_exceptions=False) + yield client + client.close() + + @staticmethod + def _hdr(*, perms: frozenset[str] = frozenset()) -> dict[str, str]: + # Plain user — no service scope, so the bypass doesn't kick in; + # just the perms passed by the test. + from turnstone.core.auth import JWT_AUD_CONSOLE, create_jwt + + return { + "Authorization": ( + "Bearer " + + create_jwt( + user_id="test-user", + scopes=frozenset({"read", "write", "approve"}), + source="test", + secret=_TEST_JWT_SECRET, + audience=JWT_AUD_CONSOLE, + permissions=perms, + ) + ) + } + + def test_approve_without_perm_returns_403(self, client): + resp = client.post( + "/v1/api/route/workstreams/abc123/approve", + json={"approved": True}, + headers=self._hdr(), + ) + assert resp.status_code == 403 + assert "tools.approve" in resp.json()["error"] + + def test_close_without_perm_returns_403(self, client): + resp = client.post( + "/v1/api/route/workstreams/abc123/close", + json={}, + headers=self._hdr(), + ) + assert resp.status_code == 403 + assert "workstreams.close" in resp.json()["error"] + + def test_approve_with_tools_approve_passes(self, client): + resp = client.post( + "/v1/api/route/workstreams/abc123/approve", + json={"approved": True}, + headers=self._hdr(perms=frozenset({"tools.approve"})), + ) + assert resp.status_code == 200 + + def test_close_with_admin_coordinator_passes(self, client): + # The OR fallback: coord sessions can drive close on + # interactive children without holding workstreams.close. + resp = client.post( + "/v1/api/route/workstreams/abc123/close", + json={}, + headers=self._hdr(perms=frozenset({"admin.coordinator"})), + ) + assert resp.status_code == 200 + + def test_send_remains_authenticated_only(self, client): + # send/cancel/dequeue/command/plan are unchanged — no new gate. + resp = client.post( + "/v1/api/route/workstreams/abc123/send", + json={"message": "hi"}, + headers=self._hdr(), + ) + assert resp.status_code == 200 + + # --------------------------------------------------------------------------- # Tests — route_lookup # --------------------------------------------------------------------------- diff --git a/tests/test_governance_endpoints.py b/tests/test_governance_endpoints.py index 9f6a84cb..61bf9330 100644 --- a/tests/test_governance_endpoints.py +++ b/tests/test_governance_endpoints.py @@ -28,6 +28,8 @@ from turnstone.console.server import ( admin_list_policies, admin_list_roles, admin_list_user_roles, + admin_role_effective, + admin_role_overrides, admin_unassign_role, admin_update_org, admin_update_policy, @@ -100,6 +102,12 @@ def client(storage): Route("/api/admin/roles", admin_create_role, methods=["POST"]), Route("/api/admin/roles/{role_id}", admin_update_role, methods=["PUT"]), Route("/api/admin/roles/{role_id}", admin_delete_role, methods=["DELETE"]), + Route("/api/admin/roles/{role_id}/effective", admin_role_effective), + Route( + "/api/admin/roles/{role_id}/overrides", + admin_role_overrides, + methods=["PUT"], + ), # Users Route( "/api/admin/users/{user_id}", @@ -217,6 +225,94 @@ class TestRoles: assert resp.status_code == 200, resp.json() assert "model.skills.write" in resp.json()["permissions"] + def test_permission_sections_js_covers_valid_permissions(self): + """F-5: ``_PERMISSION_SECTIONS`` in governance.js mirrors + ``_VALID_PERMISSIONS`` in console/server.py. A new perm added + to the Python validator without a matching JS toggle becomes + silently un-customizable through the admin Roles UI — the only + documented path for granting/revoking perms on a builtin. + Catches the same shape that surfaced ``coordinator.trust.send`` + missing from the validator during manual verification of the + overlay editor (a similar drift, in the opposite direction).""" + import re + from pathlib import Path + + from turnstone.console.server import _VALID_PERMISSIONS + + src = Path("turnstone/console/static/governance.js").read_text() + # _PERMISSION_SECTIONS is a `const X = [...]` containing nested + # `permissions: ["a", "b", ...]` arrays. Pull every quoted + # string out of every permissions: [...] block; we don't need + # a full JS parser to enumerate the perm names. + m = re.search( + r"const _PERMISSION_SECTIONS\s*=\s*\[(.*?)\];", + src, + re.DOTALL, + ) + assert m, "could not locate _PERMISSION_SECTIONS in governance.js" + body = m.group(1) + in_ui = set(re.findall(r'"([a-z][a-z._]*)"', body)) + # Exclude the section labels themselves (they're sentence-case + # like "Scopes", "Admin"; the regex above already excludes them + # by anchoring on lowercase, but be explicit about intent). + missing_in_ui = sorted(_VALID_PERMISSIONS - in_ui) + extra_in_ui = sorted(in_ui - _VALID_PERMISSIONS) + assert not missing_in_ui, ( + f"perms in _VALID_PERMISSIONS but not _PERMISSION_SECTIONS " + f"(silently un-customizable in admin UI): {missing_in_ui}" + ) + assert not extra_in_ui, ( + f"perms in _PERMISSION_SECTIONS but not _VALID_PERMISSIONS " + f"(toggle would 400 on save): {extra_in_ui}" + ) + + def test_valid_permissions_covers_all_seeded_builtin_perms(self): + """Every permission migration 008/011/014/015/029/032/033/035/040/042 + adds to a builtin role must be in ``_VALID_PERMISSIONS`` — otherwise + the overrides editor cannot round-trip the baseline (a perm dropped + from the toggle universe gets stripped to satisfy the validator, + producing a silent capability loss). Caught by the manual + verification run of feat/builtin-role-overrides: + ``coordinator.trust.send`` was in the baseline but not the + validator, so the very first Save through the overrides editor + 400'd.""" + from turnstone.console.server import _VALID_PERMISSIONS + + # Mirror the union the bootstrap migrations write into the baseline + # ``permissions`` column for builtin-admin. Keep this in sync with + # 017_catchup_admin_permissions.py and every subsequent migration + # that touches builtin-admin. + seeded = { + "read", + "write", + "approve", + "admin.users", + "admin.roles", + "admin.orgs", + "admin.policies", + "admin.prompt_policies", + "admin.skills", + "admin.audit", + "admin.usage", + "admin.schedules", + "admin.watches", + "admin.judge", + "admin.memories", + "admin.settings", + "admin.mcp", + "admin.models", + "admin.nodes", + "admin.coordinator", + "admin.cluster.inspect", + "tools.approve", + "workstreams.create", + "workstreams.close", + "conversation.modify", + "coordinator.trust.send", + } + missing = sorted(seeded - _VALID_PERMISSIONS) + assert not missing, f"perms in baseline but not _VALID_PERMISSIONS: {missing}" + def test_create_role_rejects_unknown_permission(self, client): """Unknown permission strings are rejected — guards the validator against typos in the constant list and would-be capability inflation @@ -311,6 +407,242 @@ class TestRoles: assert "builtin" in resp.json()["error"].lower() +# --------------------------------------------------------------------------- +# Tests — Role permission overrides (builtin customization) +# --------------------------------------------------------------------------- + + +def _seed_builtin_admin(storage: Any, perms: str = "read,write,admin.roles") -> None: + storage.create_role( + role_id="builtin-admin", + name="admin", + display_name="Admin", + permissions=perms, + builtin=True, + ) + storage.assign_role("test-admin", "builtin-admin") + + +class TestRoleOverrides: + def test_effective_returns_baseline_when_no_overrides(self, client, storage): + _seed_builtin_admin(storage, "read,admin.roles") + resp = client.get("/v1/api/admin/roles/builtin-admin/effective") + assert resp.status_code == 200 + body = resp.json() + assert body["baseline"] == ["admin.roles", "read"] + assert body["grants"] == [] + assert body["revokes"] == [] + assert body["effective"] == ["admin.roles", "read"] + + def test_effective_404_unknown_role(self, client): + resp = client.get("/v1/api/admin/roles/nope/effective") + assert resp.status_code == 404 + + def test_overrides_grant_skills_write(self, client, storage): + # The motivating case: model.skills.write is default-ungranted, + # operator opts in via the overrides endpoint. + _seed_builtin_admin(storage, "read,write,admin.roles") + resp = client.put( + "/v1/api/admin/roles/builtin-admin/overrides", + json={"grant": ["model.skills.write"], "revoke": []}, + ) + assert resp.status_code == 200, resp.json() + body = resp.json() + assert "model.skills.write" in body["effective"] + assert body["grants"] == ["model.skills.write"] + + def test_overrides_replace_semantics(self, client, storage): + _seed_builtin_admin(storage, "read,write,admin.roles") + client.put( + "/v1/api/admin/roles/builtin-admin/overrides", + json={"grant": ["model.skills.write"], "revoke": []}, + ) + # PUT replaces — the prior grant should be gone after sending an + # empty body, leaving only the new revoke (which IS in baseline). + resp = client.put( + "/v1/api/admin/roles/builtin-admin/overrides", + json={"grant": [], "revoke": ["write"]}, + ) + assert resp.status_code == 200 + body = resp.json() + assert body["grants"] == [] + assert body["revokes"] == ["write"] + assert "model.skills.write" not in body["effective"] + + def test_overrides_invalid_permission_rejected(self, client, storage): + _seed_builtin_admin(storage) + resp = client.put( + "/v1/api/admin/roles/builtin-admin/overrides", + json={"grant": ["totally.fake.perm"], "revoke": []}, + ) + assert resp.status_code == 400 + assert "invalid" in resp.json()["error"].lower() + + def test_overrides_disjoint_grant_revoke_rejected(self, client, storage): + _seed_builtin_admin(storage) + resp = client.put( + "/v1/api/admin/roles/builtin-admin/overrides", + json={"grant": ["approve"], "revoke": ["approve"]}, + ) + assert resp.status_code == 400 + + def test_overrides_non_builtin_rejected(self, client, storage): + storage.create_role( + role_id="custom-1", + name="custom", + display_name="Custom", + permissions="read", + builtin=False, + ) + resp = client.put( + "/v1/api/admin/roles/custom-1/overrides", + json={"grant": ["write"], "revoke": []}, + ) + assert resp.status_code == 400 + assert "builtin" in resp.json()["error"].lower() + + def test_overrides_no_op_grant_and_revoke_normalize(self, client, storage): + # A grant of a perm already in baseline AND a revoke of a perm not + # in baseline both have zero behavioural effect; the endpoint + # strips them rather than persisting redundant rows. + _seed_builtin_admin(storage, "read,write,admin.roles") + resp = client.put( + "/v1/api/admin/roles/builtin-admin/overrides", + json={ + "grant": ["read", "model.skills.write"], + "revoke": ["tools.approve"], + }, + ) + assert resp.status_code == 200 + body = resp.json() + # Only the meaningful delta survived. + assert body["grants"] == ["model.skills.write"] + assert body["revokes"] == [] + + def test_overrides_lockout_guard_blocks_last_admin_revoke(self, client, storage): + _seed_builtin_admin(storage, "read,admin.roles") + resp = client.put( + "/v1/api/admin/roles/builtin-admin/overrides", + json={"grant": [], "revoke": ["admin.roles"]}, + ) + assert resp.status_code == 409 + assert "admin.roles" in resp.json()["error"] + # Verify the override was NOT applied — the user must still be admin. + assert "admin.roles" in storage.get_user_permissions("test-admin") + + def test_overrides_lockout_guard_permits_revoke_when_other_admin_exists(self, client, storage): + _seed_builtin_admin(storage, "read,admin.roles") + # Second role on a different user that also carries admin.roles — + # revoking from builtin-admin no longer locks the deployment out. + storage.create_role( + role_id="custom-admin", + name="custom-admin", + display_name="Custom Admin", + permissions="read,admin.roles", + builtin=False, + ) + storage.assign_role("user-1", "custom-admin") + resp = client.put( + "/v1/api/admin/roles/builtin-admin/overrides", + json={"grant": [], "revoke": ["admin.roles"]}, + ) + assert resp.status_code == 200 + + def test_list_roles_includes_overlay_fields(self, client, storage): + _seed_builtin_admin(storage, "read,admin.roles") + client.put( + "/v1/api/admin/roles/builtin-admin/overrides", + json={"grant": ["model.skills.write"], "revoke": []}, + ) + resp = client.get("/v1/api/admin/roles") + roles = resp.json()["roles"] + # Find builtin-admin in the listing + row = next(r for r in roles if r["role_id"] == "builtin-admin") + assert row["grants"] == ["model.skills.write"] + assert row["revokes"] == [] + assert "model.skills.write" in row["effective"] + + def test_overrides_lockout_guard_blocks_grant_removal(self, client, storage): + # F-1: PUT-replace semantics mean an existing grant of admin.roles + # on a role whose baseline lacks it is silently dropped when the + # new payload omits it. Old guard only fired on explicit revokes + # and missed this path entirely — concrete cluster-bricking scenario. + # Setup: only builtin-operator users hold admin.roles, via overlay grant. + storage.create_role( + role_id="builtin-operator", + name="operator", + display_name="Operator", + permissions="read,write", # baseline lacks admin.roles + builtin=True, + ) + # Grant admin.roles to operator via overlay, then unassign builtin-admin + # from the test user so operator is the only path to admin.roles. + storage.set_role_overrides("builtin-operator", {"admin.roles"}, set()) + storage.assign_role("test-admin", "builtin-operator") + # The test-admin user keeps builtin-admin assigned by _seed_builtin_admin + # which would normally hold admin.roles — but we seed without it so the + # only source is the overlay on builtin-operator. + if storage.get_role("builtin-admin") is None: + storage.create_role( + role_id="builtin-admin", + name="admin", + display_name="Admin", + permissions="read,write", # baseline lacks admin.roles + builtin=True, + ) + storage.assign_role("test-admin", "builtin-admin") + # Sanity: admin.roles only reachable via operator's overlay + assert "admin.roles" in storage.get_user_permissions("test-admin") + # The lockout-triggering call: Reset operator's overrides (drops + # the admin.roles grant). Old guard short-circuited because + # revoke=[] doesn't contain "admin.roles"; new guard simulates + # the post-PUT effective set on the target role. + resp = client.put( + "/v1/api/admin/roles/builtin-operator/overrides", + json={"grant": [], "revoke": []}, + ) + assert resp.status_code == 409, resp.json() + assert "admin.roles" in resp.json()["error"] + # Override was NOT applied — admin.roles still reachable. + assert "admin.roles" in storage.get_user_permissions("test-admin") + + def test_assign_role_blocks_escalation_via_overlay_grant(self, storage, client): + # F-2 reframed. Simulates the attack path where a previous + # admin.roles holder injected an overlay grant on a builtin + # role, then a separate admin.users holder (who does NOT hold + # the granted perm) tries to assign that role to a new user. + # Without this fix the assign-time subset check would read the + # baseline column and miss the overlay, silently escalating + # the assignee. + # + # Operator's baseline is unchanged production default + # ("read,write" — no model.skills.write). The overlay grant + # below is the simulated attack step, not the system default. + _seed_builtin_admin(storage, "read,write,admin.roles,admin.users") + storage.create_role( + role_id="builtin-operator", + name="operator", + display_name="Operator", + permissions="read,write", # production default + builtin=True, + ) + storage.set_role_overrides( + "builtin-operator", {"model.skills.write"}, set() + ) # simulated prior poisoning by an admin.roles holder + + # The harness AuthResult holds admin.roles + admin.users + many + # admin.* perms but NOT model.skills.write. Assigning operator + # — whose POST-OVERLAY effective set in this test scenario + # contains model.skills.write — must 403, because the assignee + # would otherwise gain a perm the assigner doesn't hold. + resp = client.post( + "/v1/api/admin/users/user-1/roles", + json={"role_id": "builtin-operator"}, + ) + assert resp.status_code == 403 + assert "permissions you do not hold" in resp.json()["error"] + + # --------------------------------------------------------------------------- # Tests — Role assignments # --------------------------------------------------------------------------- diff --git a/tests/test_governance_storage.py b/tests/test_governance_storage.py index cb33d60b..654d5f60 100644 --- a/tests/test_governance_storage.py +++ b/tests/test_governance_storage.py @@ -152,6 +152,112 @@ class TestRoleCRUD: assert db.get_user_permissions("u1") == set() +# --------------------------------------------------------------------------- +# Role permission overrides (builtin-role customization layer) +# --------------------------------------------------------------------------- + + +class TestRolePermissionOverrides: + def test_overrides_empty_by_default(self, db): + db.create_role("r1", "admin", "Admin", "read,write", builtin=True, org_id="") + assert db.list_role_overrides("r1") == [] + eff = db.effective_role_permissions("r1") + assert eff["baseline"] == ["read", "write"] + assert eff["grants"] == [] + assert eff["revokes"] == [] + assert eff["effective"] == ["read", "write"] + + def test_set_role_overrides_grant_and_revoke(self, db): + db.create_role("r1", "admin", "Admin", "read,write", builtin=True, org_id="") + db.set_role_overrides("r1", {"approve"}, {"write"}, created_by="u-admin") + eff = db.effective_role_permissions("r1") + assert eff["baseline"] == ["read", "write"] + assert eff["grants"] == ["approve"] + assert eff["revokes"] == ["write"] + assert eff["effective"] == ["approve", "read"] + + def test_set_role_overrides_replaces_prior_state(self, db): + db.create_role("r1", "admin", "Admin", "read,write", builtin=True, org_id="") + db.set_role_overrides("r1", {"approve"}, set()) + db.set_role_overrides("r1", set(), {"write"}) + rows = db.list_role_overrides("r1") + # Prior grant is gone; only the new revoke remains. + assert len(rows) == 1 + assert rows[0]["permission"] == "write" + assert rows[0]["action"] == "revoke" + + def test_set_role_overrides_disjoint_required(self, db): + db.create_role("r1", "admin", "Admin", "read", builtin=True, org_id="") + with pytest.raises(ValueError): + db.set_role_overrides("r1", {"write"}, {"write"}) + + def test_clear_role_overrides(self, db): + db.create_role("r1", "admin", "Admin", "read", builtin=True, org_id="") + db.set_role_overrides("r1", {"approve"}, set()) + assert len(db.list_role_overrides("r1")) == 1 + db.clear_role_overrides("r1") + assert db.list_role_overrides("r1") == [] + + def test_get_user_permissions_applies_overlay_to_builtin(self, db): + db.create_role("r1", "admin", "Admin", "read,write", builtin=True, org_id="") + db.create_user("u1", "alice", "Alice", "$2b$hash") + db.assign_role("u1", "r1") + # Before overrides: baseline only + assert db.get_user_permissions("u1") == {"read", "write"} + # After overrides: grants in, revokes out + db.set_role_overrides("r1", {"approve", "model.skills.write"}, {"write"}) + assert db.get_user_permissions("u1") == {"read", "approve", "model.skills.write"} + + def test_get_user_permissions_ignores_overlay_on_custom_role(self, db): + # Overrides only apply to builtin rows. A custom role with stray + # override rows (defensive case — should never happen via the API) + # must NOT have them applied. + db.create_role("r1", "custom", "Custom", "read", builtin=False, org_id="") + db.create_user("u1", "alice", "Alice", "$2b$hash") + db.assign_role("u1", "r1") + db.set_role_overrides("r1", {"approve"}, {"read"}) + # Effective perms come from the role row only — overlay is dropped. + assert db.get_user_permissions("u1") == {"read"} + + def test_users_with_permission_bulk(self, db): + # Two roles, three users; only users whose EFFECTIVE perm set + # includes the queried perm appear. Drives the lockout-guard + # rewrite in admin_role_overrides — one bulk SELECT replaces + # the prior per-user/per-role loop. + db.create_role("r-adm", "adm", "Adm", "admin.roles,read", builtin=True, org_id="") + db.create_role("r-op", "op", "Op", "read,write", builtin=True, org_id="") + db.create_user("u1", "alice", "Alice", "$2b$hash") + db.create_user("u2", "bob", "Bob", "$2b$hash") + db.create_user("u3", "cara", "Cara", "$2b$hash") + db.assign_role("u1", "r-adm") + db.assign_role("u2", "r-op") + db.assign_role("u3", "r-op") + # Baseline state + assert db.users_with_permission("admin.roles") == {"u1"} + # Overlay-grant admin.roles to r-op → u2 + u3 now hold it too + db.set_role_overrides("r-op", {"admin.roles"}, set()) + assert db.users_with_permission("admin.roles") == {"u1", "u2", "u3"} + # exclude_role_id = r-adm → u1 drops; u2/u3 still hold via r-op + assert db.users_with_permission("admin.roles", exclude_role_id="r-adm") == { + "u2", + "u3", + } + # Overlay-revoke admin.roles from r-adm → u1 no longer holds via that role + db.set_role_overrides("r-adm", set(), {"admin.roles"}) + assert db.users_with_permission("admin.roles") == {"u2", "u3"} + + def test_delete_role_cleans_up_overrides(self, db): + # F-7: no FK on role_permission_overrides. Storage layer must + # clean up by hand so a re-seeded role_id (deterministic for + # builtins on schema reseed) doesn't silently inherit stale + # overrides from the prior occupant. + db.create_role("r1", "custom", "Custom", "read", builtin=False, org_id="") + db.set_role_overrides("r1", {"approve"}, set()) + assert len(db.list_role_overrides("r1")) == 1 + assert db.delete_role("r1") is True + assert db.list_role_overrides("r1") == [] + + # --------------------------------------------------------------------------- # Organizations # --------------------------------------------------------------------------- diff --git a/tests/test_server_attachments_on_create.py b/tests/test_server_attachments_on_create.py index 2b73636a..18e0b66f 100644 --- a/tests/test_server_attachments_on_create.py +++ b/tests/test_server_attachments_on_create.py @@ -36,6 +36,11 @@ def _make_jwt(user_id: str) -> str: source="test", secret=_TEST_JWT_SECRET, audience=JWT_AUD_SERVER, + # ``workstreams.create`` is now a real gate on POST /workstreams/new + # — see PR adding 057_role_permission_overrides. Embed the perm so + # the multipart-create flow under test stays exercising the create + # path and not the new 403. + permissions=frozenset({"workstreams.create"}), ) diff --git a/tests/test_server_authz.py b/tests/test_server_authz.py index ac54bc5f..4bc03328 100644 --- a/tests/test_server_authz.py +++ b/tests/test_server_authz.py @@ -21,7 +21,21 @@ from starlette.testclient import TestClient _TEST_JWT_SECRET = "test-jwt-secret-minimum-32-chars!" -def _make_jwt(user_id: str, *, scopes: frozenset[str] | None = None) -> str: +# Default permission set for test JWTs. Mirrors what builtin-operator +# carries: enough perms to exercise create/close/approve gates without +# turning every existing test into a re-authorization round. Tests +# negating these gates pass ``permissions=frozenset()`` explicitly. +_DEFAULT_TEST_PERMS = frozenset( + {"workstreams.create", "workstreams.close", "tools.approve", "conversation.modify"} +) + + +def _make_jwt( + user_id: str, + *, + scopes: frozenset[str] | None = None, + permissions: frozenset[str] | None = None, +) -> str: from turnstone.core.auth import JWT_AUD_SERVER, create_jwt return create_jwt( @@ -30,11 +44,17 @@ def _make_jwt(user_id: str, *, scopes: frozenset[str] | None = None) -> str: source="test", secret=_TEST_JWT_SECRET, audience=JWT_AUD_SERVER, + permissions=_DEFAULT_TEST_PERMS if permissions is None else permissions, ) -def _auth(user: str, *, scopes: frozenset[str] | None = None) -> dict[str, str]: - return {"Authorization": f"Bearer {_make_jwt(user, scopes=scopes)}"} +def _auth( + user: str, + *, + scopes: frozenset[str] | None = None, + permissions: frozenset[str] | None = None, +) -> dict[str, str]: + return {"Authorization": f"Bearer {_make_jwt(user, scopes=scopes, permissions=permissions)}"} # --------------------------------------------------------------------------- @@ -411,6 +431,110 @@ class TestCrossTenantClose: assert resp.status_code == 404 +class TestPermissionGatesOnLifecycle: + """Gates that previously didn't exist — ``workstreams.create``, + ``workstreams.close``, ``tools.approve`` were declared, seeded into + builtin-operator, surfaced in the admin Roles UI, and never wired + to a single ``require_permission`` site. PR added the gates; these + tests confirm a JWT without each perm gets 403.""" + + def test_create_without_perm_returns_403(self, app_client): + client, _mgr = app_client + resp = client.post( + "/v1/api/workstreams/new", + json={"name": "no-perm"}, + headers=_auth("user-1", permissions=frozenset()), + ) + assert resp.status_code == 403 + assert "workstreams.create" in resp.json()["error"] + + def test_close_without_perm_returns_403(self, app_client): + from turnstone.core.storage import get_storage + + client, _mgr = app_client + storage = get_storage() + assert storage is not None + _register_ws(storage, "ws-1", "user-1") + resp = client.post( + "/v1/api/workstreams/ws-1/close", + json={}, + headers=_auth("user-1", permissions=frozenset()), + ) + assert resp.status_code == 403 + assert "workstreams.close" in resp.json()["error"] + + def test_approve_without_perm_returns_403(self, app_client): + from turnstone.core.storage import get_storage + + client, _mgr = app_client + storage = get_storage() + assert storage is not None + _register_ws(storage, "ws-1", "user-1") + resp = client.post( + "/v1/api/workstreams/ws-1/approve", + json={"approved": True}, + headers=_auth("user-1", permissions=frozenset()), + ) + assert resp.status_code == 403 + assert "tools.approve" in resp.json()["error"] + + def test_create_with_perm_passes_gate(self, app_client): + # Sanity: same call WITH the perm reaches the post-gate logic + # (whatever its outcome — a successful create or a non-403 + # validation/state error is fine; only the gate behaviour is + # under test here). + client, _mgr = app_client + resp = client.post( + "/v1/api/workstreams/new", + json={"name": "with-perm"}, + headers=_auth("user-1", permissions=frozenset({"workstreams.create"})), + ) + assert resp.status_code != 403, resp.json() + + # Positive coverage for the admin.coordinator OR-fallback on each + # of the three lifted verbs. Without these, a future refactor + # that dropped admin.coordinator from the accepted_permissions + # tuple would regress coord-session children silently — the proxy + # tests only exercise the route_proxy verb dict, not the lift. + + def test_create_with_admin_coordinator_passes_gate(self, app_client): + client, _mgr = app_client + resp = client.post( + "/v1/api/workstreams/new", + json={"name": "coord-child"}, + headers=_auth("user-1", permissions=frozenset({"admin.coordinator"})), + ) + assert resp.status_code != 403, resp.json() + + def test_close_with_admin_coordinator_passes_gate(self, app_client): + from turnstone.core.storage import get_storage + + client, _mgr = app_client + storage = get_storage() + assert storage is not None + _register_ws(storage, "ws-1", "user-1") + resp = client.post( + "/v1/api/workstreams/ws-1/close", + json={}, + headers=_auth("user-1", permissions=frozenset({"admin.coordinator"})), + ) + assert resp.status_code != 403, resp.json() + + def test_approve_with_admin_coordinator_passes_gate(self, app_client): + from turnstone.core.storage import get_storage + + client, _mgr = app_client + storage = get_storage() + assert storage is not None + _register_ws(storage, "ws-1", "user-1") + resp = client.post( + "/v1/api/workstreams/ws-1/approve", + json={"approved": True}, + headers=_auth("user-1", permissions=frozenset({"admin.coordinator"})), + ) + assert resp.status_code != 403, resp.json() + + class TestCrossTenantTitle: def test_refresh_title_requires_live_session(self, app_client): # Trusted-team model: scope-level auth is the gate; any caller diff --git a/turnstone/api/console_schemas.py b/turnstone/api/console_schemas.py index 19431a3b..68617ab7 100644 --- a/turnstone/api/console_schemas.py +++ b/turnstone/api/console_schemas.py @@ -189,6 +189,13 @@ class RoleInfo(BaseModel): org_id: str created: str updated: str + # Overlay fields (populated by list/get endpoints for builtin roles; + # ``effective`` always reflects the post-overlay set, ``grants``/ + # ``revokes`` are the user-applied deltas — both empty for custom roles + # since overrides apply only to builtins). + effective: list[str] = [] + grants: list[str] = [] + revokes: list[str] = [] class CreateRoleRequest(BaseModel): @@ -206,6 +213,18 @@ class ListRolesResponse(BaseModel): roles: list[RoleInfo] +class RoleOverridesRequest(BaseModel): + grant: list[str] = [] + revoke: list[str] = [] + + +class RoleEffectiveResponse(BaseModel): + baseline: list[str] + grants: list[str] + revokes: list[str] + effective: list[str] + + class AssignRoleRequest(BaseModel): role_id: str diff --git a/turnstone/api/console_spec.py b/turnstone/api/console_spec.py index f4e74bbb..582e82e6 100644 --- a/turnstone/api/console_spec.py +++ b/turnstone/api/console_spec.py @@ -81,7 +81,9 @@ from turnstone.api.console_schemas import ( ParseSkillResponse, RegistryInstallRequest, RegistrySearchResponse, + RoleEffectiveResponse, RoleInfo, + RoleOverridesRequest, RouteCreateResponse, RouteResponse, SetNodeMetadataValueRequest, @@ -449,6 +451,23 @@ CONSOLE_ENDPOINTS: list[EndpointSpec] = [ error_codes=[400, 404], tags=["Admin"], ), + EndpointSpec( + "/v1/api/admin/roles/{role_id}/effective", + "GET", + "Get effective permissions for a role (baseline + overrides)", + response_model=RoleEffectiveResponse, + error_codes=[404], + tags=["Admin"], + ), + EndpointSpec( + "/v1/api/admin/roles/{role_id}/overrides", + "PUT", + "Replace the grant/revoke override set for a builtin role", + request_model=RoleOverridesRequest, + response_model=RoleEffectiveResponse, + error_codes=[400, 404, 409], + tags=["Admin"], + ), EndpointSpec( "/v1/api/admin/users/{user_id}/roles", "GET", diff --git a/turnstone/bootstrap.py b/turnstone/bootstrap.py index 132a3dcb..cdbc4aba 100644 --- a/turnstone/bootstrap.py +++ b/turnstone/bootstrap.py @@ -138,7 +138,7 @@ model and behavioral settings after deployment through the admin panel. ## Built-in Roles - **Admin** (`builtin-admin`): Full access — read, write, approve, all admin.* permissions -- **Operator** (`builtin-operator`): read, write, workstreams.create, workstreams.close +- **Operator** (`builtin-operator`): create / close workstreams, approve tools, modify conversations (read, write, workstreams.create, workstreams.close, tools.approve, conversation.modify) - **Viewer** (`builtin-viewer`): read only ## Tool Policies diff --git a/turnstone/console/server.py b/turnstone/console/server.py index 5d90ecc8..ebdb110a 100644 --- a/turnstone/console/server.py +++ b/turnstone/console/server.py @@ -1761,8 +1761,19 @@ async def create_workstream(request: Request) -> JSONResponse: - ``node_id`` omitted or ``"auto"`` → console picks the node with most headroom - ``node_id`` set to ``"pool"`` → console picks any available node """ + from turnstone.core.auth import require_any_permission from turnstone.core.web_helpers import read_json_or_400 + # Gate on workstreams.create OR admin.coordinator before proxying — + # keeps the 403 attributed at the console (audit clarity) and avoids + # a cluster round-trip on a forbidden request. The node-side lift + # gates again as defense in depth. See ``interactive_endpoint_config`` + # in ``turnstone/server.py`` for the OR rationale (coord sessions + # spawning interactive children). + err = require_any_permission(request, ("workstreams.create", "admin.coordinator")) + if err is not None: + return err + body = await read_json_or_400(request) if isinstance(body, JSONResponse): return body @@ -1892,7 +1903,17 @@ async def route_create(request: Request) -> Response: console can hash to the owning node before the multipart body lands — we do not parse the body just to peek at the metadata. """ + from turnstone.core.auth import require_any_permission + t0 = time.monotonic() + # Fail fast on forbidden requests — the upstream node's lift gates + # too (see ``make_create_handler`` in session_routes.py). + # ``admin.coordinator`` is accepted so coord sessions can spawn + # interactive children via the route proxy without holding + # ``workstreams.create``. + err = require_any_permission(request, ("workstreams.create", "admin.coordinator")) + if err is not None: + return _record_route(request, "create", 403, t0, err) router: ConsoleRouter | None = request.app.state.router ring_ready = router is not None and router.is_ready() if not ring_ready: @@ -2267,12 +2288,32 @@ async def route_proxy(request: Request) -> Response: legacies still in scope). ``verb`` drives the audit action lookup; DELETE on ``/send`` is treated as dequeue for audit attribution. """ + from turnstone.core.auth import require_any_permission + t0 = time.monotonic() # Extract verb name from URL tail: /v1/api/route/.../send -> "send". # DELETE on /send is the dequeue path — audit attribution diverges. verb = request.url.path.rsplit("/", 1)[-1] if verb == "send" and request.method == "DELETE": verb = "dequeue" + + # Verb-scoped permission gate. Redundant with the node-side lift's + # check (the upstream server gates again), but failing fast at the + # proxy avoids a cluster round-trip on a forbidden request and keeps + # the 403 attributed to the proxy in audit logs. Only the two verbs + # whose perms exist; other verbs (send/cancel/dequeue/command/plan) + # remain authenticated-only and pre-existing — leaving them alone + # rather than expanding scope. ``admin.coordinator`` accepted as + # an alternative on each so coord sessions driving interactive + # children pass through without the operator-style perms. + _verb_perms: dict[str, tuple[str, ...]] = { + "approve": ("tools.approve", "admin.coordinator"), + "close": ("workstreams.close", "admin.coordinator"), + } + if verb in _verb_perms: + err = require_any_permission(request, _verb_perms[verb]) + if err is not None: + return _record_route(request, verb, 403, t0, err) router: ConsoleRouter | None = request.app.state.router ring_ready = router is not None and router.is_ready() if not ring_ready: @@ -5904,6 +5945,11 @@ _VALID_PERMISSIONS = frozenset( # explicitly before their coordinator sessions can mutate the # catalog. "model.skills.write", + # Coordinator out-of-band send capability — granted to + # ``builtin-admin`` by migration 042 but previously absent + # from this validator, which made it impossible to add to a + # custom role or restore via the overrides editor. + "coordinator.trust.send", "tools.approve", "workstreams.create", "workstreams.close", @@ -5912,8 +5958,28 @@ _VALID_PERMISSIONS = frozenset( ) +def _enrich_role(row: dict[str, Any], eff: dict[str, list[str]]) -> dict[str, Any]: + """Add overlay fields (effective / grants / revokes) to a role dict. + + For builtin rows, ``effective`` is the post-overlay set; for custom + rows it's just the parsed ``permissions`` column with empty deltas. + Keeps a single round-trip shape so the admin UI can render chips + + "modified" indicators without per-row fetches. + + Caller supplies the prefetched ``eff`` dict from + :meth:`effective_role_permissions_bulk` so admin_list_roles needs + one storage round-trip total instead of 1 + 2*builtin_count. + """ + return { + **row, + "effective": eff["effective"], + "grants": eff["grants"] if row.get("builtin") else [], + "revokes": eff["revokes"] if row.get("builtin") else [], + } + + async def admin_list_roles(request: Request) -> JSONResponse: - """GET /v1/api/admin/roles — list all roles.""" + """GET /v1/api/admin/roles — list all roles with overlay info.""" from turnstone.core.auth import require_permission from turnstone.core.web_helpers import require_storage_or_503 @@ -5923,7 +5989,18 @@ async def admin_list_roles(request: Request) -> JSONResponse: err = require_permission(request, "admin.roles") if err: return err - return JSONResponse({"roles": storage.list_roles()}) + rows = storage.list_roles() + eff_map = storage.effective_role_permissions_bulk([r["role_id"] for r in rows]) + return JSONResponse( + { + "roles": [ + _enrich_role( + r, eff_map.get(r["role_id"], {"effective": [], "grants": [], "revokes": []}) + ) + for r in rows + ] + } + ) async def admin_create_role(request: Request) -> JSONResponse: @@ -6074,6 +6151,148 @@ async def admin_delete_role(request: Request) -> JSONResponse: return JSONResponse({"status": "ok"}) +async def admin_role_effective(request: Request) -> JSONResponse: + """GET /v1/api/admin/roles/{role_id}/effective — baseline + overrides.""" + from turnstone.core.auth import require_permission + from turnstone.core.web_helpers import require_storage_or_503 + + storage, err = require_storage_or_503(request) + if err: + return err + err = require_permission(request, "admin.roles") + if err: + return err + + role_id = request.path_params["role_id"] + if storage.get_role(role_id) is None: + return JSONResponse({"error": "Role not found"}, status_code=404) + return JSONResponse(storage.effective_role_permissions(role_id)) + + +def _check_admin_lockout( + storage: Any, + role_id: str, + grants: set[str], + revokes: set[str], +) -> JSONResponse | None: + """Refuse override changes that would leave nobody with admin.roles. + + ``admin.roles`` is the only permission whose loss is self-locking — + without it, no user can reach the Roles tab to undo the change. Other + revoked permissions (``model.skills.write``, ``admin.skills``, etc.) + can always be restored by an admin, so they don't get this guard. + + PUT-replace semantics on ``set_role_overrides`` mean the lockout + surface isn't just "did the new payload revoke admin.roles" — it's + also "did the new payload omit a previously-granted admin.roles + override." Either path lands at the same effective state, so the + check computes the post-PUT effective set on the target role and + falls through to a bulk users_with_permission query for any user + who retains the perm via another role. + + Two queries total (one ``get_role`` for the target's baseline, one + join over ``user_roles ⋈ roles`` plus IN-fetch on overrides for the + builtin role ids), regardless of cluster user/role count. Caller + is expected to wrap this in ``asyncio.to_thread`` since both + SQLite and asyncpg-via-sync-wrapper open new connections. + """ + role = storage.get_role(role_id) + if role is None: + return None # caller already validated existence; defensive no-op + baseline = {p.strip() for p in (role.get("permissions") or "").split(",") if p.strip()} + # Simulate the proposed PUT on the target role. If admin.roles + # survives there, every user assigned to the target keeps it; we're + # done. + target_effective = (baseline | grants) - revokes + if "admin.roles" in target_effective: + return None + # admin.roles is leaving the target role. Only need a single user + # who still holds it through some OTHER role to keep the cluster + # recoverable. ``exclude_role_id`` makes that one SQL question + # instead of N+M round-trips. + if storage.users_with_permission("admin.roles", exclude_role_id=role_id): + return None + return JSONResponse( + {"error": "Refusing change: would leave no user with admin.roles"}, + status_code=409, + ) + + +async def admin_role_overrides(request: Request) -> JSONResponse: + """PUT /v1/api/admin/roles/{role_id}/overrides — replace grant/revoke set.""" + from turnstone.core.audit import record_audit + from turnstone.core.auth import require_permission + from turnstone.core.web_helpers import read_json_or_400, require_storage_or_503 + + storage, err = require_storage_or_503(request) + if err: + return err + err = require_permission(request, "admin.roles") + if err: + return err + + role_id = request.path_params["role_id"] + existing = storage.get_role(role_id) + if existing is None: + return JSONResponse({"error": "Role not found"}, status_code=404) + if not existing.get("builtin"): + return JSONResponse( + {"error": "Overrides apply only to builtin roles; edit custom roles directly"}, + status_code=400, + ) + + body = await read_json_or_400(request) + if isinstance(body, JSONResponse): + return body + + grant_list = body.get("grant", []) or [] + revoke_list = body.get("revoke", []) or [] + if not isinstance(grant_list, list) or not isinstance(revoke_list, list): + return JSONResponse({"error": "grant and revoke must be arrays"}, status_code=400) + grants = {str(p) for p in grant_list} + revokes = {str(p) for p in revoke_list} + + invalid = sorted((grants | revokes) - _VALID_PERMISSIONS) + if invalid: + return JSONResponse( + {"error": f"Invalid permissions: {', '.join(invalid)}"}, + status_code=400, + ) + if grants & revokes: + return JSONResponse( + {"error": "A permission cannot appear in both grant and revoke"}, + status_code=400, + ) + + # Strip no-ops: grants already in baseline and revokes not in baseline both + # have zero effect on the merged set. Storing them wastes rows and clutters + # the audit detail without changing behavior. + baseline = {p.strip() for p in (existing.get("permissions") or "").split(",") if p.strip()} + grants = grants - baseline + revokes = revokes & baseline + + # Block in a worker thread — even bulk-querying the lockout state + # touches sync DB connections (SQLite open, asyncpg sync wrapper) + # and should never run inline on the asyncio event loop. + lockout = await asyncio.to_thread(_check_admin_lockout, storage, role_id, grants, revokes) + if lockout is not None: + return lockout + + audit_uid, ip = _audit_context(request) + storage.set_role_overrides(role_id, grants, revokes, created_by=audit_uid) + record_audit( + storage, + audit_uid, + "role.overrides.set", + "role", + role_id, + {"grants": sorted(grants), "revokes": sorted(revokes)}, + ip, + ) + + return JSONResponse(storage.effective_role_permissions(role_id)) + + async def admin_list_user_roles(request: Request) -> JSONResponse: """GET /v1/api/admin/users/{user_id}/roles — list roles assigned to a user.""" from turnstone.core.auth import require_permission @@ -6129,10 +6348,14 @@ async def admin_assign_role(request: Request) -> JSONResponse: if auth_result and auth_result.user_id == user_id: return JSONResponse({"error": "Cannot modify own role assignments"}, status_code=403) - # Ensure caller holds all permissions present in the target role - target_perms = set( - p.strip() for p in target_role.get("permissions", "").split(",") if p.strip() - ) + # Ensure caller holds all permissions present in the target role. + # Read the EFFECTIVE perm set (post-overlay) rather than the raw + # ``permissions`` baseline column — builtin roles can carry an + # override layer added via PUT ``/v1/api/admin/roles/{id}/overrides``, + # and skipping the overlay here would let an admin.roles holder + # silently bypass the subset gate by granting a perm to e.g. + # builtin-operator before assigning that role to a new user. + target_perms = set(storage.effective_role_permissions(role_id)["effective"]) if ( auth_result and auth_result.permissions @@ -12507,6 +12730,12 @@ def create_app( Route("/api/admin/roles", admin_create_role, methods=["POST"]), Route("/api/admin/roles/{role_id}", admin_update_role, methods=["PUT"]), Route("/api/admin/roles/{role_id}", admin_delete_role, methods=["DELETE"]), + Route("/api/admin/roles/{role_id}/effective", admin_role_effective), + Route( + "/api/admin/roles/{role_id}/overrides", + admin_role_overrides, + methods=["PUT"], + ), Route("/api/admin/users/{user_id}/roles", admin_list_user_roles), Route( "/api/admin/users/{user_id}/roles", diff --git a/turnstone/console/static/governance.js b/turnstone/console/static/governance.js index 7d016967..530c27d4 100644 --- a/turnstone/console/static/governance.js +++ b/turnstone/console/static/governance.js @@ -58,6 +58,100 @@ function loadGovRoles() { }); } +// Set of role_ids whose drawer is open. Persists across re-renders so a +// reload (e.g. after edit) doesn't collapse the inspector the user was +// looking at. +const _govRoleExpanded = new Set(); + +function _effectivePerms(role) { + // Server's _enrich_role always sets ``effective`` (empty array when + // the role legitimately has no perms — e.g. a builtin whose overrides + // revoke every baseline entry). Presence is the right sentinel, NOT + // length: a length check would silently fall through to the baseline + // column for a fully-revoked role, lying to the inspector about what + // the role can do. Only the raw-column fallback is for callers that + // hit older /v1/api/admin/roles payloads (e.g. mocked tests). + if (Array.isArray(role.effective)) return role.effective; + return (role.permissions || "") + .split(",") + .map(function (p) { + return p.trim(); + }) + .filter(Boolean); +} + +function _renderRoleDrawer(role) { + const effective = _effectivePerms(role); + const effectiveSet = {}; + for (let i = 0; i < effective.length; i++) effectiveSet[effective[i]] = true; + const baseline = role.builtin + ? (role.permissions || "") + .split(",") + .map(function (p) { + return p.trim(); + }) + .filter(Boolean) + : effective; + const baselineSet = {}; + for (let i = 0; i < baseline.length; i++) baselineSet[baseline[i]] = true; + const grants = role.grants || []; + const revokes = role.revokes || []; + const grantSet = {}; + for (let i = 0; i < grants.length; i++) grantSet[grants[i]] = true; + const revokeSet = {}; + for (let i = 0; i < revokes.length; i++) revokeSet[revokes[i]] = true; + + let body = ""; + for (let s = 0; s < _PERMISSION_SECTIONS.length; s++) { + const section = _PERMISSION_SECTIONS[s]; + let chips = ""; + let sectionHasAny = false; + for (let i = 0; i < section.permissions.length; i++) { + const p = section.permissions[i]; + const inBase = !!baselineSet[p]; + const inEff = !!effectiveSet[p]; + const isGrant = !!grantSet[p]; + const isRevoke = !!revokeSet[p]; + if (!inEff && !isRevoke && !isGrant) continue; // hide perms not relevant to this role + sectionHasAny = true; + let cls = "perm-inspect-chip"; + if (isGrant) cls += " is-grant"; + else if (isRevoke) cls += " is-revoke"; + else if (inBase) cls += " is-baseline"; + let suffix = ""; + if (isGrant) suffix = ' +'; + else if (isRevoke) suffix = ' −'; + chips += + '' + escapeHtml(p) + suffix + ""; + } + if (!sectionHasAny) continue; + body += + '
' + + '
' + + escapeHtml(section.label) + + "
" + + '
' + + chips + + "
"; + } + const drawerActions = + role.builtin && (grants.length || revokes.length) + ? '' + : ""; + return ( + '
' + + body + + (drawerActions + ? '
' + drawerActions + "
" + : "") + + "
" + ); +} + function _renderGovRoles(items) { const el = document.getElementById("admin-roles-table"); if (!items.length) { @@ -67,57 +161,105 @@ function _renderGovRoles(items) { let html = ""; for (let i = 0; i < items.length; i++) { const r = items[i]; - // Render permissions as badges - const perms = (r.permissions || "").split(","); - let badges = ""; - for (let j = 0; j < perms.length; j++) { - const p = perms[j].trim(); - if (!p) continue; - let cls = "scope-badge"; - if (p === "approve" || p.indexOf("admin.") === 0) cls += " scope-approve"; - else if (p === "write" || p.indexOf("workstreams.") === 0) - cls += " scope-write"; - badges += '' + escapeHtml(p) + ""; + const effective = _effectivePerms(r); + const grants = r.grants || []; + const revokes = r.revokes || []; + const modified = grants.length > 0 || revokes.length > 0; + const expanded = _govRoleExpanded.has(r.role_id); + + let pills = ""; + if (r.builtin) + pills += 'builtin'; + if (modified) { + const deltaLabel = + "modified " + + (grants.length ? "+" + grants.length : "") + + (grants.length && revokes.length ? " / " : "") + + (revokes.length ? "−" + revokes.length : ""); + pills += + '' + + escapeHtml(deltaLabel) + + ""; } - const typeLabel = r.builtin - ? 'builtin' - : ""; - const actions = r.builtin - ? "" - : '' + + + const countChip = + '' + + effective.length + + (effective.length === 1 ? " permission" : " permissions") + + ""; + + // Builtin rows always get Edit (lands in the overrides editor). + // Custom rows get Edit + Delete (existing behavior). + let actions = + ''; + if (!r.builtin) { + actions += ''; + } + + const chevron = expanded ? "▾" : "▸"; html += - '
' + + '
' + '' + + '" + escapeHtml(r.display_name) + " " + - typeLabel + + pills + "" + '' + - badges + + countChip + "" + '' + actions + "
"; + if (expanded) html += _renderRoleDrawer(r); } setSafeHtml(el, html); + + // Bind expand toggle (row + chevron both work) + const expandBtns = el.querySelectorAll("[data-expand-role]"); + for (let k = 0; k < expandBtns.length; k++) { + expandBtns[k].addEventListener("click", function (ev) { + ev.stopPropagation(); + const rid = this.getAttribute("data-expand-role"); + if (_govRoleExpanded.has(rid)) _govRoleExpanded.delete(rid); + else _govRoleExpanded.add(rid); + _renderGovRoles(_govRoles); + }); + } // Bind edit const editBtns = el.querySelectorAll("[data-edit-role]"); for (let k = 0; k < editBtns.length; k++) { - editBtns[k].addEventListener("click", function () { + editBtns[k].addEventListener("click", function (ev) { + ev.stopPropagation(); showEditRoleModal(this.getAttribute("data-edit-role")); }); } // Bind delete const delBtns = el.querySelectorAll("[data-delete-role]"); for (let k = 0; k < delBtns.length; k++) { - delBtns[k].addEventListener("click", function () { + delBtns[k].addEventListener("click", function (ev) { + ev.stopPropagation(); const rid = this.getAttribute("data-delete-role"); const rname = this.getAttribute("data-role-name"); showConfirmModal( @@ -143,6 +285,37 @@ function _renderGovRoles(items) { ); }); } + // Bind reset-to-default (drawer action; builtin + overridden only) + const resetBtns = el.querySelectorAll("[data-reset-role]"); + for (let k = 0; k < resetBtns.length; k++) { + resetBtns[k].addEventListener("click", function (ev) { + ev.stopPropagation(); + const rid = this.getAttribute("data-reset-role"); + showConfirmModal( + "Reset overrides", + "Drop all overrides on this builtin role? Effective permissions return to the shipped baseline.", + "Reset", + function () { + authFetch("/v1/api/admin/roles/" + rid + "/overrides", { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ grant: [], revoke: [] }), + }) + .then(function (r) { + if (!r.ok) throw new Error(); + return r.json(); + }) + .then(function () { + showToast("Overrides cleared"); + loadGovRoles(); + }) + .catch(function () { + showToast("Failed to reset overrides"); + }); + }, + ); + }); + } } // Permission inventory grouped by namespace so the role modal can @@ -165,6 +338,7 @@ const _PERMISSION_SECTIONS = [ "admin.roles", "admin.orgs", "admin.policies", + "admin.prompt_policies", "admin.skills", "admin.audit", "admin.usage", @@ -174,11 +348,24 @@ const _PERMISSION_SECTIONS = [ "admin.memories", "admin.settings", "admin.mcp", + "admin.models", + "admin.nodes", + "admin.coordinator", + "admin.cluster.inspect", ], }, { label: "Workstreams & Tools", - permissions: ["workstreams.create", "workstreams.close", "tools.approve"], + permissions: [ + "workstreams.create", + "workstreams.close", + "conversation.modify", + "tools.approve", + ], + }, + { + label: "Coordinator", + permissions: ["coordinator.trust.send"], }, { label: "Model", @@ -196,7 +383,7 @@ const _ALL_PERMISSIONS = (function () { return flat; })(); -function _buildPermCheckboxes(prefix, selected) { +function _buildPermCheckboxes(prefix, selected, baseline) { // Emits the toggle-switch component used elsewhere in the admin // modals so each permission reads as a deliberate on/off rather // than a generic checkbox. Sections are wrapped in a @@ -206,6 +393,18 @@ function _buildPermCheckboxes(prefix, selected) { // The underlying ```` // shape is preserved so ``_collectPermCheckboxes`` still picks // them up regardless of section. + // + // ``baseline`` is optional — when provided (builtin-role edits) each + // toggle gets a small visual mark showing whether the perm is on by + // default and whether the current state is an override (added or + // removed). ``submitEditRole`` diffs the toggle state against the + // baseline to produce the {grant, revoke} payload for /overrides. + const baselineSet = {}; + if (baseline) + for (let i = 0; i < baseline.length; i++) baselineSet[baseline[i]] = true; + const selectedSet = {}; + if (selected) + for (let i = 0; i < selected.length; i++) selectedSet[selected[i]] = true; let html = ""; for (let s = 0; s < _PERMISSION_SECTIONS.length; s++) { const section = _PERMISSION_SECTIONS[s]; @@ -217,9 +416,28 @@ function _buildPermCheckboxes(prefix, selected) { '
'; for (let i = 0; i < section.permissions.length; i++) { const p = section.permissions[i]; - const checked = selected && selected.indexOf(p) >= 0 ? " checked" : ""; + const isChecked = !!selectedSet[p]; + const inBase = !!baselineSet[p]; + const checked = isChecked ? " checked" : ""; + let extraCls = ""; + let badge = ""; + if (baseline) { + if (inBase && !isChecked) { + extraCls = " is-revoke"; + badge = '−'; + } else if (!inBase && isChecked) { + extraCls = " is-grant"; + badge = '+'; + } else if (inBase) { + extraCls = " is-baseline"; + badge = + '•'; + } + } html += - '
"; @@ -326,11 +545,41 @@ function showEditRoleModal(roleId) { const ov = document.getElementById("edit-role-overlay"); ov.style.display = "flex"; document.getElementById("er-id").value = roleId; - document.getElementById("er-name").value = role.display_name; - const selected = (role.permissions || "").split(","); + // Builtin rows expose the name field read-only — only display_name and + // permissions are mutable on customs; for builtins, only permissions + // (via the override layer). The display_name input is kept visible on + // builtin rows but disabled to reduce surprise. + const nameInput = document.getElementById("er-name"); + nameInput.value = role.display_name; + nameInput.disabled = !!role.builtin; + + const titleEl = document.getElementById("edit-role-title"); + if (titleEl) + titleEl.textContent = role.builtin + ? "Customize Built-in Role" + : "Edit Role"; + + const baseline = role.builtin + ? (role.permissions || "") + .split(",") + .map(function (p) { + return p.trim(); + }) + .filter(Boolean) + : null; + const selected = _effectivePerms(role); + + // Persist the baseline + builtin flag on the form so submit can diff + // without re-walking _govRoles (which could have been refreshed mid-edit). + const form = document.getElementById("edit-role-box"); + if (form) { + form.dataset.builtin = role.builtin ? "1" : "0"; + form.dataset.baseline = baseline ? baseline.join(",") : ""; + } + setSafeHtml( document.getElementById("er-perms-container"), - _buildPermCheckboxes("er", selected), + _buildPermCheckboxes("er", selected, baseline), ); document.getElementById("edit-role-error").classList.remove("is-visible"); _erTrapHandler = _installTrap("edit-role-overlay", "edit-role-box"); @@ -348,13 +597,61 @@ function hideEditRoleModal() { function submitEditRole() { const roleId = document.getElementById("er-id").value; const dname = document.getElementById("er-name").value.trim(); - const perms = _collectPermCheckboxes("er"); + const form = document.getElementById("edit-role-box"); + const isBuiltin = form && form.dataset.builtin === "1"; document.getElementById("er-submit").disabled = true; - authFetch("/v1/api/admin/roles/" + roleId, { - method: "PUT", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ display_name: dname, permissions: perms }), - }) + + let fetchOpts; + let url; + if (isBuiltin) { + // Diff against baseline → {grant, revoke}. Display name is immutable. + // + // Crucial safety property: the diff universe is the set of permissions + // we actually RENDERED as toggles, not the full baseline. If the + // permission taxonomy in `_PERMISSION_SECTIONS` ever falls behind a + // new server-side perm (e.g. migration adds it to builtin-admin + // before this file ships the toggle), naïve baseline-vs-selected + // diffing would treat every unrendered perm as "user wants this + // revoked" and silently strip it. Limiting the universe to rendered + // toggles makes the editor a NO-OP for unknown perms — they pass + // through untouched. + const renderedNodes = document.querySelectorAll('input[name="er-perm"]'); + const renderedSet = {}; + for (let i = 0; i < renderedNodes.length; i++) + renderedSet[renderedNodes[i].value] = true; + const baseline = (form.dataset.baseline || "").split(",").filter(Boolean); + const baselineSet = {}; + for (let i = 0; i < baseline.length; i++) baselineSet[baseline[i]] = true; + const selectedStr = _collectPermCheckboxes("er"); + const selected = selectedStr.split(",").filter(Boolean); + const selectedSet = {}; + for (let i = 0; i < selected.length; i++) selectedSet[selected[i]] = true; + const grant = []; + const revoke = []; + for (const p in selectedSet) { + if (!baselineSet[p]) grant.push(p); + } + for (const p in baselineSet) { + // Only revoke perms the user could actually see — unrendered ones + // are out of scope for this edit and must round-trip unchanged. + if (renderedSet[p] && !selectedSet[p]) revoke.push(p); + } + url = "/v1/api/admin/roles/" + roleId + "/overrides"; + fetchOpts = { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ grant: grant, revoke: revoke }), + }; + } else { + const perms = _collectPermCheckboxes("er"); + url = "/v1/api/admin/roles/" + roleId; + fetchOpts = { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ display_name: dname, permissions: perms }), + }; + } + authFetch(url, fetchOpts) .then(function (r) { if (!r.ok) return r.json().then(function (d) { diff --git a/turnstone/console/static/style.css b/turnstone/console/static/style.css index 7fd4098e..c7528e9e 100644 --- a/turnstone/console/static/style.css +++ b/turnstone/console/static/style.css @@ -2622,7 +2622,162 @@ textarea.skill-content-area { ========================================================================== */ #admin-roles .admin-colheaders, #admin-roles .admin-row { - grid-template-columns: 160px 1fr 110px; + grid-template-columns: 240px 1fr 140px; +} + +/* Row-level chevron — opens the inspect drawer. Square-bracketed + ascii triangle keeps with the "instrument panel" typographic + palette already used by the rest of the admin UX (no SVG icons). */ +.role-expand-btn { + display: inline-block; + background: transparent; + border: 1px solid transparent; + color: var(--fg-dim); + font-family: var(--font-ui); + font-size: 10px; + line-height: 1; + padding: 1px 4px; + margin-right: 6px; + cursor: pointer; + border-radius: 2px; +} +.role-expand-btn:hover { + color: var(--accent); + border-color: var(--border); +} +.role-expand-btn:focus-visible { + outline: 2px solid var(--accent); + outline-offset: 1px; +} + +.admin-role-row[data-expanded="true"] .role-expand-btn { + color: var(--accent); +} + +/* Collapsed-row permission summary — replaces the chip stack that + used to overflow with a "…" the moment a role accrued more than a + handful of permissions. Drawer carries the real inventory. */ +.perm-count-chip { + display: inline-block; + font-family: var(--font-ui); + font-size: 11px; + color: var(--fg-dim); + letter-spacing: 0.02em; +} + +/* Inspect drawer — slot directly under its row, full-width. No + sub-grid (the parent table is a column of rows, not a grid), so a + plain block container with bordered padding is sufficient. */ +.admin-role-drawer { + padding: 12px 16px 14px 24px; + /* Pull contrast from the page background, not from --bg-highlight, + so the drawer reads as a recessed surface in both themes. Light + theme: a slight darken via rgba black layered over the page bg + instead of --bg-highlight (which is barely distinguishable from + --bg in the light palette). */ + background: rgba(0, 0, 0, 0.04); + border-left: 2px solid var(--accent); + border-bottom: 1px solid var(--border); + margin-bottom: 2px; +} +.role-drawer-section { + margin-top: 6px; +} +.role-drawer-section:first-child { + margin-top: 0; +} +.role-drawer-section-label { + font-family: var(--font-ui); + font-size: 9px; + font-weight: 600; + text-transform: uppercase; + letter-spacing: 0.08em; + color: var(--fg-dim); + margin-bottom: 4px; +} +.role-drawer-chips { + display: flex; + flex-wrap: wrap; + gap: 4px; +} +.role-drawer-actions { + margin-top: 12px; + padding-top: 10px; + border-top: 1px solid var(--border); + display: flex; + gap: 8px; +} + +/* Drawer chips: baseline = subdued, grant = green plus, revoke = + red strike-through. The trailing "+" / "−" sigil ('.perm-delta-mark') + gives the same signal at a glance for screen-reader pass-through + and high-contrast users where the colour alone isn't enough. */ +.perm-inspect-chip { + display: inline-flex; + align-items: center; + gap: 3px; + font-family: var(--font-ui); + font-size: 10px; + font-weight: 600; + text-transform: uppercase; + letter-spacing: 0.06em; + padding: 2px 7px; + border-radius: 2px; + /* Solid page bg so chips sit on top of the recessed drawer with a + visible step; --fg (not --fg-dim) so the label is comfortably + readable in both themes. */ + background: var(--bg); + color: var(--fg); + border: 1px solid var(--border); +} +.perm-inspect-chip.is-baseline { + /* Baseline = "shipped default." Slightly dimmed so the override + variants pop, but still high-contrast enough to read. */ + color: var(--fg-dim); +} +.perm-inspect-chip.is-grant { + /* Bumped from 0.06 → 0.16 alpha so the green wash actually reads + as a state. Border step too. */ + color: var(--green); + border-color: rgba(52, 211, 153, 0.55); + background: rgba(52, 211, 153, 0.16); +} +.perm-inspect-chip.is-revoke { + color: var(--red); + border-color: rgba(248, 113, 113, 0.55); + background: rgba(248, 113, 113, 0.14); + text-decoration: line-through; +} +.perm-delta-mark { + font-weight: 700; + font-size: 10px; +} + +/* Toggle annotations inside the edit modal — same baseline/grant/revoke + palette as the drawer, but lighter so the toggle remains the primary + affordance. The dot/plus/minus mark trails the label. */ +.perm-baseline-mark { + display: inline-block; + margin-left: 6px; + font-size: 10px; + font-weight: 700; + color: var(--fg-dim); +} +.perm-baseline-mark.is-default { + color: var(--fg-dim); + opacity: 0.6; +} +.perm-toggle.is-grant .perm-baseline-mark { + color: var(--green); +} +.perm-toggle.is-revoke .perm-baseline-mark { + color: var(--red); +} +.perm-toggle.is-revoke .toggle-label { + color: var(--red); +} +.perm-toggle.is-grant .toggle-label { + color: var(--green); } /* ========================================================================== diff --git a/turnstone/core/auth.py b/turnstone/core/auth.py index ae3035ca..2dcf59a3 100644 --- a/turnstone/core/auth.py +++ b/turnstone/core/auth.py @@ -206,6 +206,81 @@ def require_permission( ) +def require_any_permission( + request: Request, + permissions: tuple[str, ...], + *, + allow_service_bypass: bool = True, +) -> JSONResponse | None: + """OR-semantics variant of :func:`require_permission`. + + Returns ``None`` if the caller holds at least one of ``permissions``; + otherwise a 403 naming the full set so operators know which roles + would satisfy the gate. Used where multiple roles legitimately + reach the same endpoint — e.g. workstream-create accepts both + ``workstreams.create`` (operator) and ``admin.coordinator`` (coord + sessions spawning interactive children). ``permissions`` is + required and must be non-empty; the empty tuple is almost certainly + a programmer error and would produce an always-403 gate. + + This function is the OR-equivalent of the security choke point in + :func:`require_permission` — every branch is intentional and the + per-branch comments below should stay accurate as the policy + evolves. If a future change adds or reorders a branch, the comment + must move with it; a stale comment on a security gate is worse than + no comment. + """ + from starlette.responses import JSONResponse + + # Defensive: an empty tuple here means a caller mis-wired the gate + # and would silently 403 every request — fail loud at import-adjacent + # time so the breakage shows up in tests, not under load. + if not permissions: + raise ValueError("require_any_permission needs at least one permission") + + # Pull the AuthResult attached by the auth middleware. Using + # ``getattr`` twice survives both "no state attribute" (Starlette + # request not yet wrapped) and "state present but auth_result + # unset" (middleware skipped the request) — both manifest as the + # same "no identity" outcome and route to 401, never silently + # admitting the call. + auth_result: AuthResult | None = getattr(getattr(request, "state", None), "auth_result", None) + if auth_result is None: + # Distinguishing 401 from 403 here matters: 401 tells the client + # "we don't know who you are, retry with credentials" while a + # 403 would suggest the identity is known but lacks the perm, + # which would mislead operators chasing an auth bug. + return JSONResponse({"error": "Unauthorized"}, status_code=401) + + # Service-scope bypass: inter-cluster calls (collector → node, + # console → upstream) carry a service token and must not be blocked + # by per-user permission grants — the service scope itself is the + # cluster-side trust boundary. Callers that protect a capability- + # escalation gate (e.g. ``coordinator.trust.send``) pass + # ``allow_service_bypass=False`` to opt out. + if allow_service_bypass and auth_result.has_scope("service"): + return None + + # OR-semantics happy path: any single permission in the set is + # enough. ``any()`` short-circuits so the linear scan is cheap + # even on a long permissions tuple. Note: this is a pure set + # membership check against the AuthResult — DB role lookups + # already happened at middleware time, so the gate stays in-process. + if any(auth_result.has_permission(p) for p in permissions): + return None + + # Final fallthrough: identity present, not a service, no matching + # perm. Listing every accepted perm in the error body gives the + # operator an actionable remediation — "grant one of {workstreams.create, + # admin.coordinator}" — instead of guessing which role would + # satisfy a generic 403. + perm_list = ", ".join(f"'{p}'" for p in permissions) + return JSONResponse( + {"error": f"Forbidden: missing one of {perm_list} permissions"}, + status_code=403, + ) + + # --------------------------------------------------------------------------- # Path classification # --------------------------------------------------------------------------- diff --git a/turnstone/core/session_routes.py b/turnstone/core/session_routes.py index 68820579..527353d8 100644 --- a/turnstone/core/session_routes.py +++ b/turnstone/core/session_routes.py @@ -695,7 +695,11 @@ def register_coord_verbs( # --------------------------------------------------------------------------- -def make_approve_handler(cfg: SessionEndpointConfig) -> Handler: +def make_approve_handler( + cfg: SessionEndpointConfig, + *, + accepted_permissions: tuple[str, ...] = (), +) -> Handler: """Lifted body for ``POST {prefix}/{ws_id}/approve``. Resolves a pending tool approval on the workstream's UI. Both @@ -704,7 +708,17 @@ def make_approve_handler(cfg: SessionEndpointConfig) -> Handler: differences are auth scope, manager lookup, and the ``__budget_override__`` filter (interactive-only — coord workstreams don't have the budget-override pseudo-tool). + + ``accepted_permissions`` is OR-checked via :func:`require_any_permission` + only when ``cfg.permission_gate`` is ``None`` — i.e. for the + interactive kind, where it IS the primary gate (not a fallback to + something else). Coord's ``permission_gate`` already takes + precedence so admin-coordinator users don't also need + ``tools.approve`` to act on their own coord workstreams. Pass + ``admin.coordinator`` alongside ``tools.approve`` for endpoints + reachable by coord sessions spawning interactive children. """ + from turnstone.core.auth import require_any_permission from turnstone.core.web_helpers import read_json_or_400 async def approve(request: Request) -> Response: @@ -714,6 +728,10 @@ def make_approve_handler(cfg: SessionEndpointConfig) -> Handler: err = cfg.permission_gate(request) if err is not None: return err + elif accepted_permissions: + err = require_any_permission(request, accepted_permissions) + if err is not None: + return err mgr_opt, err503 = cfg.manager_lookup(request) if err503 is not None: return err503 @@ -826,6 +844,7 @@ def make_close_handler( *, audit_emit: CloseAuditEmitter | None = None, supports_close_reason: bool = False, + accepted_permissions: tuple[str, ...] = (), ) -> Handler: """Lifted body for ``POST {prefix}/{ws_id}/close``. @@ -870,10 +889,16 @@ def make_close_handler( async def close(request: Request) -> Response: import asyncio + from turnstone.core.auth import require_any_permission + if cfg.permission_gate is not None: err = cfg.permission_gate(request) if err is not None: return err + elif accepted_permissions: + err = require_any_permission(request, accepted_permissions) + if err is not None: + return err mgr_opt, err503 = cfg.manager_lookup(request) if err503 is not None: return err503 @@ -1726,6 +1751,7 @@ def make_create_handler( cfg: SessionEndpointConfig, *, audit_emit: CreateAuditEmitter | None = None, + accepted_permissions: tuple[str, ...] = (), ) -> Handler: """Lifted body for ``POST {prefix}/new`` — workstream creation. @@ -1861,6 +1887,7 @@ def make_create_handler( IMAGE_SIZE_CAP, validate_and_save_uploaded_files, ) + from turnstone.core.auth import require_any_permission from turnstone.core.web_helpers import ( read_json_or_400, read_multipart_create_or_400, @@ -1870,6 +1897,10 @@ def make_create_handler( err = cfg.permission_gate(request) if err is not None: return err + elif accepted_permissions: + err = require_any_permission(request, accepted_permissions) + if err is not None: + return err mgr_opt, err503 = cfg.manager_lookup(request) if err503 is not None: return err503 diff --git a/turnstone/core/storage/_postgresql.py b/turnstone/core/storage/_postgresql.py index 7159f63b..c864a85f 100644 --- a/turnstone/core/storage/_postgresql.py +++ b/turnstone/core/storage/_postgresql.py @@ -45,6 +45,7 @@ from turnstone.core.storage._schema import ( output_assessments, output_guard_patterns, prompt_templates, + role_permission_overrides, roles, scheduled_task_runs, scheduled_tasks, @@ -121,6 +122,9 @@ from turnstone.core.storage._utils import sanitize_text from turnstone.core.storage._utils import ( scan_skill_content as _scan_skill_content, ) +from turnstone.core.storage._utils import ( + split_perms as _split_perms, +) from turnstone.core.workstream import BULK_CLOSE_STATE_VALUES, WorkstreamKind log = get_logger(__name__) @@ -2267,6 +2271,16 @@ class PostgreSQLBackend: def delete_role(self, role_id: str) -> bool: with self._conn() as conn: conn.execute(sa.delete(user_roles).where(user_roles.c.role_id == role_id)) + # No FK on role_permission_overrides (migration 057 omitted + # to match the rest of the governance schema), so clean up + # by hand. Orphan rows would otherwise apply silently if + # a role_id were ever reused — deterministic for builtins + # on schema reseed. + conn.execute( + sa.delete(role_permission_overrides).where( + role_permission_overrides.c.role_id == role_id + ) + ) result = conn.execute(sa.delete(roles).where(roles.c.role_id == role_id)) conn.commit() return result.rowcount > 0 @@ -2385,20 +2399,213 @@ class PostgreSQLBackend: def get_user_permissions(self, user_id: str) -> set[str]: with self._conn() as conn: - rows = conn.execute( - sa.select(roles.c.permissions) + role_rows = conn.execute( + sa.select(roles.c.role_id, roles.c.permissions, roles.c.builtin) .select_from(user_roles.join(roles, user_roles.c.role_id == roles.c.role_id)) .where(user_roles.c.user_id == user_id) ).fetchall() + if not role_rows: + return set() + builtin_role_ids = [r[0] for r in role_rows if r[2]] + grants: dict[str, set[str]] = {} + revokes: dict[str, set[str]] = {} + if builtin_role_ids: + ov_rows = conn.execute( + sa.select( + role_permission_overrides.c.role_id, + role_permission_overrides.c.permission, + role_permission_overrides.c.action, + ).where(role_permission_overrides.c.role_id.in_(builtin_role_ids)) + ).fetchall() + for rid, perm, action in ov_rows: + if action == "grant": + grants.setdefault(rid, set()).add(perm) + elif action == "revoke": + revokes.setdefault(rid, set()).add(perm) perms: set[str] = set() - for r in rows: - if r[0]: - for p in r[0].split(","): - p = p.strip() - if p: - perms.add(p) + for rid, perms_str, builtin in role_rows: + role_perms = _split_perms(perms_str) + if builtin: + role_perms = (role_perms | grants.get(rid, set())) - revokes.get(rid, set()) + perms |= role_perms return perms + def users_with_permission( + self, + permission: str, + *, + exclude_role_id: str | None = None, + ) -> set[str]: + with self._conn() as conn: + q = sa.select( + user_roles.c.user_id, + user_roles.c.role_id, + roles.c.permissions, + roles.c.builtin, + ).select_from(user_roles.join(roles, user_roles.c.role_id == roles.c.role_id)) + if exclude_role_id: + q = q.where(user_roles.c.role_id != exclude_role_id) + rows = conn.execute(q).fetchall() + if not rows: + return set() + builtin_role_ids = {r[1] for r in rows if r[3]} + grants: dict[str, set[str]] = {} + revokes: dict[str, set[str]] = {} + if builtin_role_ids: + ov_rows = conn.execute( + sa.select( + role_permission_overrides.c.role_id, + role_permission_overrides.c.permission, + role_permission_overrides.c.action, + ).where(role_permission_overrides.c.role_id.in_(builtin_role_ids)) + ).fetchall() + for rid, perm, action in ov_rows: + if action == "grant": + grants.setdefault(rid, set()).add(perm) + elif action == "revoke": + revokes.setdefault(rid, set()).add(perm) + holders: set[str] = set() + for user_id, role_id, perms_str, builtin in rows: + eff = _split_perms(perms_str) + if builtin: + eff = (eff | grants.get(role_id, set())) - revokes.get(role_id, set()) + if permission in eff: + holders.add(user_id) + return holders + + def list_role_overrides(self, role_id: str) -> list[dict[str, str]]: + with self._conn() as conn: + rows = conn.execute( + sa.select(role_permission_overrides) + .where(role_permission_overrides.c.role_id == role_id) + .order_by( + role_permission_overrides.c.action, + role_permission_overrides.c.permission, + ) + ).fetchall() + return [dict(r._mapping) for r in rows] + + def set_role_overrides( + self, + role_id: str, + grants: set[str], + revokes: set[str], + created_by: str = "", + ) -> None: + if grants & revokes: + raise ValueError("grants and revokes must be disjoint") + now = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S") + with self._conn() as conn: + conn.execute( + sa.delete(role_permission_overrides).where( + role_permission_overrides.c.role_id == role_id + ) + ) + rows = [ + { + "role_id": role_id, + "permission": p, + "action": "grant", + "created": now, + "created_by": created_by, + } + for p in sorted(grants) + ] + [ + { + "role_id": role_id, + "permission": p, + "action": "revoke", + "created": now, + "created_by": created_by, + } + for p in sorted(revokes) + ] + if rows: + conn.execute(sa.insert(role_permission_overrides), rows) + conn.commit() + + def clear_role_overrides(self, role_id: str) -> None: + with self._conn() as conn: + conn.execute( + sa.delete(role_permission_overrides).where( + role_permission_overrides.c.role_id == role_id + ) + ) + conn.commit() + + def effective_role_permissions(self, role_id: str) -> dict[str, list[str]]: + with self._conn() as conn: + role_row = conn.execute( + sa.select(roles.c.permissions, roles.c.builtin).where(roles.c.role_id == role_id) + ).fetchone() + if role_row is None: + return {"baseline": [], "grants": [], "revokes": [], "effective": []} + baseline = _split_perms(role_row[0]) + grants: set[str] = set() + revokes: set[str] = set() + if role_row[1]: + ov_rows = conn.execute( + sa.select( + role_permission_overrides.c.permission, + role_permission_overrides.c.action, + ).where(role_permission_overrides.c.role_id == role_id) + ).fetchall() + for perm, action in ov_rows: + if action == "grant": + grants.add(perm) + elif action == "revoke": + revokes.add(perm) + effective = (baseline | grants) - revokes + return { + "baseline": sorted(baseline), + "grants": sorted(grants), + "revokes": sorted(revokes), + "effective": sorted(effective), + } + + def effective_role_permissions_bulk( + self, role_ids: list[str] + ) -> dict[str, dict[str, list[str]]]: + if not role_ids: + return {} + with self._conn() as conn: + role_rows = conn.execute( + sa.select(roles.c.role_id, roles.c.permissions, roles.c.builtin).where( + roles.c.role_id.in_(role_ids) + ) + ).fetchall() + if not role_rows: + return {} + builtin_role_ids = [r[0] for r in role_rows if r[2]] + grants: dict[str, set[str]] = {} + revokes: dict[str, set[str]] = {} + if builtin_role_ids: + ov_rows = conn.execute( + sa.select( + role_permission_overrides.c.role_id, + role_permission_overrides.c.permission, + role_permission_overrides.c.action, + ).where(role_permission_overrides.c.role_id.in_(builtin_role_ids)) + ).fetchall() + for rid, perm, action in ov_rows: + if action == "grant": + grants.setdefault(rid, set()).add(perm) + elif action == "revoke": + revokes.setdefault(rid, set()).add(perm) + out: dict[str, dict[str, list[str]]] = {} + for rid, perms_str, builtin in role_rows: + baseline = _split_perms(perms_str) + role_grants = grants.get(rid, set()) if builtin else set() + role_revokes = revokes.get(rid, set()) if builtin else set() + effective = (baseline | role_grants) - role_revokes + out[rid] = { + "baseline": sorted(baseline), + "grants": sorted(role_grants), + "revokes": sorted(role_revokes), + "effective": sorted(effective), + } + return out + # -- Organizations --------------------------------------------------------- def create_org(self, org_id: str, name: str, display_name: str, settings: str = "{}") -> None: diff --git a/turnstone/core/storage/_protocol.py b/turnstone/core/storage/_protocol.py index 01bbd162..6f075672 100644 --- a/turnstone/core/storage/_protocol.py +++ b/turnstone/core/storage/_protocol.py @@ -1202,7 +1202,77 @@ class StorageBackend(Protocol): ... def get_user_permissions(self, user_id: str) -> set[str]: - """Return the union of all permissions from the user's assigned roles.""" + """Return the union of all permissions from the user's assigned roles. + + For builtin roles, applies any rows in ``role_permission_overrides`` + on top of ``roles.permissions`` as ``baseline ∪ grants − revokes``. + """ + ... + + def users_with_permission( + self, + permission: str, + *, + exclude_role_id: str | None = None, + ) -> set[str]: + """Return ``user_id``s whose effective perms include ``permission``. + + Walks every ``(user, assigned_role)`` pair in two bulk queries + (one over ``user_roles ⋈ roles``, one over + ``role_permission_overrides`` for the builtin role ids in the + first query's result) instead of N round-trips, then folds the + overlay in-process. ``exclude_role_id``, when set, ignores any + contribution from that role — used by the lockout guard to + answer "would anyone still hold ``admin.roles`` via SOME OTHER + role if we modified this one?" without first having to apply + the proposed override. + """ + ... + + def list_role_overrides(self, role_id: str) -> list[dict[str, str]]: + """Return override rows for ``role_id`` (action in {'grant','revoke'}).""" + ... + + def set_role_overrides( + self, + role_id: str, + grants: set[str], + revokes: set[str], + created_by: str = "", + ) -> None: + """Transactionally replace the override set for ``role_id``. + + Deletes any existing rows for the role and inserts one row per + (permission, action) in ``grants`` / ``revokes``. Empty inputs + clear all overrides (equivalent to ``clear_role_overrides``). + ``grants`` and ``revokes`` MUST be disjoint — the caller is + responsible for ensuring no permission appears in both. + """ + ... + + def clear_role_overrides(self, role_id: str) -> None: + """Delete every override row for ``role_id`` (reset-to-default).""" + ... + + def effective_role_permissions(self, role_id: str) -> dict[str, list[str]]: + """Return ``{'baseline': [...], 'grants': [...], 'revokes': [...], + 'effective': [...]}`` for a single role, with overrides applied. + Each list is sorted for stable rendering. + """ + ... + + def effective_role_permissions_bulk( + self, role_ids: list[str] + ) -> dict[str, dict[str, list[str]]]: + """Bulk variant of :meth:`effective_role_permissions`. + + Returns ``{role_id: {baseline, grants, revokes, effective}}`` + for every role_id in ``role_ids``. Issues at most two queries + regardless of list size (one over ``roles``, one IN-filter over + ``role_permission_overrides``). Missing role_ids are omitted + from the result rather than mapped to an empty dict — caller + can detect absence directly. + """ ... # -- Organizations --------------------------------------------------------- diff --git a/turnstone/core/storage/_schema.py b/turnstone/core/storage/_schema.py index a3999bff..4f9fa0e1 100644 --- a/turnstone/core/storage/_schema.py +++ b/turnstone/core/storage/_schema.py @@ -395,6 +395,19 @@ user_roles = sa.Table( sa.Index("idx_user_roles_role_id", user_roles.c.role_id) +role_permission_overrides = sa.Table( + "role_permission_overrides", + metadata, + sa.Column("role_id", sa.Text, nullable=False), + sa.Column("permission", sa.Text, nullable=False), + sa.Column("action", sa.Text, nullable=False), # 'grant' | 'revoke' + sa.Column("created", sa.Text, nullable=False), + sa.Column("created_by", sa.Text, nullable=False, server_default=""), + sa.PrimaryKeyConstraint("role_id", "permission"), +) + +sa.Index("idx_role_permission_overrides_role", role_permission_overrides.c.role_id) + tool_policies = sa.Table( "tool_policies", metadata, diff --git a/turnstone/core/storage/_sqlite.py b/turnstone/core/storage/_sqlite.py index 32818670..59e8550c 100644 --- a/turnstone/core/storage/_sqlite.py +++ b/turnstone/core/storage/_sqlite.py @@ -45,6 +45,7 @@ from turnstone.core.storage._schema import ( output_assessments, output_guard_patterns, prompt_templates, + role_permission_overrides, roles, scheduled_task_runs, scheduled_tasks, @@ -121,6 +122,9 @@ from turnstone.core.storage._utils import sanitize_text from turnstone.core.storage._utils import ( scan_skill_content as _scan_skill_content, ) +from turnstone.core.storage._utils import ( + split_perms as _split_perms, +) from turnstone.core.workstream import BULK_CLOSE_STATE_VALUES, WorkstreamKind log = get_logger(__name__) @@ -2417,6 +2421,16 @@ class SQLiteBackend: def delete_role(self, role_id: str) -> bool: with self._conn() as conn: conn.execute(sa.delete(user_roles).where(user_roles.c.role_id == role_id)) + # No FK on role_permission_overrides (migration 057 omitted + # to match the rest of the governance schema), so clean up + # by hand. Orphan rows would otherwise apply silently if + # a role_id were ever reused — deterministic for builtins + # on schema reseed. + conn.execute( + sa.delete(role_permission_overrides).where( + role_permission_overrides.c.role_id == role_id + ) + ) result = conn.execute(sa.delete(roles).where(roles.c.role_id == role_id)) conn.commit() return result.rowcount > 0 @@ -2547,20 +2561,213 @@ class SQLiteBackend: def get_user_permissions(self, user_id: str) -> set[str]: with self._conn() as conn: - rows = conn.execute( - sa.select(roles.c.permissions) + role_rows = conn.execute( + sa.select(roles.c.role_id, roles.c.permissions, roles.c.builtin) .select_from(user_roles.join(roles, user_roles.c.role_id == roles.c.role_id)) .where(user_roles.c.user_id == user_id) ).fetchall() + if not role_rows: + return set() + builtin_role_ids = [r[0] for r in role_rows if r[2]] + grants: dict[str, set[str]] = {} + revokes: dict[str, set[str]] = {} + if builtin_role_ids: + ov_rows = conn.execute( + sa.select( + role_permission_overrides.c.role_id, + role_permission_overrides.c.permission, + role_permission_overrides.c.action, + ).where(role_permission_overrides.c.role_id.in_(builtin_role_ids)) + ).fetchall() + for rid, perm, action in ov_rows: + if action == "grant": + grants.setdefault(rid, set()).add(perm) + elif action == "revoke": + revokes.setdefault(rid, set()).add(perm) perms: set[str] = set() - for r in rows: - if r[0]: - for p in r[0].split(","): - p = p.strip() - if p: - perms.add(p) + for rid, perms_str, builtin in role_rows: + role_perms = _split_perms(perms_str) + if builtin: + role_perms = (role_perms | grants.get(rid, set())) - revokes.get(rid, set()) + perms |= role_perms return perms + def users_with_permission( + self, + permission: str, + *, + exclude_role_id: str | None = None, + ) -> set[str]: + with self._conn() as conn: + q = sa.select( + user_roles.c.user_id, + user_roles.c.role_id, + roles.c.permissions, + roles.c.builtin, + ).select_from(user_roles.join(roles, user_roles.c.role_id == roles.c.role_id)) + if exclude_role_id: + q = q.where(user_roles.c.role_id != exclude_role_id) + rows = conn.execute(q).fetchall() + if not rows: + return set() + builtin_role_ids = {r[1] for r in rows if r[3]} + grants: dict[str, set[str]] = {} + revokes: dict[str, set[str]] = {} + if builtin_role_ids: + ov_rows = conn.execute( + sa.select( + role_permission_overrides.c.role_id, + role_permission_overrides.c.permission, + role_permission_overrides.c.action, + ).where(role_permission_overrides.c.role_id.in_(builtin_role_ids)) + ).fetchall() + for rid, perm, action in ov_rows: + if action == "grant": + grants.setdefault(rid, set()).add(perm) + elif action == "revoke": + revokes.setdefault(rid, set()).add(perm) + holders: set[str] = set() + for user_id, role_id, perms_str, builtin in rows: + eff = _split_perms(perms_str) + if builtin: + eff = (eff | grants.get(role_id, set())) - revokes.get(role_id, set()) + if permission in eff: + holders.add(user_id) + return holders + + def list_role_overrides(self, role_id: str) -> list[dict[str, str]]: + with self._conn() as conn: + rows = conn.execute( + sa.select(role_permission_overrides) + .where(role_permission_overrides.c.role_id == role_id) + .order_by( + role_permission_overrides.c.action, + role_permission_overrides.c.permission, + ) + ).fetchall() + return [dict(r._mapping) for r in rows] + + def set_role_overrides( + self, + role_id: str, + grants: set[str], + revokes: set[str], + created_by: str = "", + ) -> None: + if grants & revokes: + raise ValueError("grants and revokes must be disjoint") + now = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S") + with self._conn() as conn: + conn.execute( + sa.delete(role_permission_overrides).where( + role_permission_overrides.c.role_id == role_id + ) + ) + rows = [ + { + "role_id": role_id, + "permission": p, + "action": "grant", + "created": now, + "created_by": created_by, + } + for p in sorted(grants) + ] + [ + { + "role_id": role_id, + "permission": p, + "action": "revoke", + "created": now, + "created_by": created_by, + } + for p in sorted(revokes) + ] + if rows: + conn.execute(sa.insert(role_permission_overrides), rows) + conn.commit() + + def clear_role_overrides(self, role_id: str) -> None: + with self._conn() as conn: + conn.execute( + sa.delete(role_permission_overrides).where( + role_permission_overrides.c.role_id == role_id + ) + ) + conn.commit() + + def effective_role_permissions(self, role_id: str) -> dict[str, list[str]]: + with self._conn() as conn: + role_row = conn.execute( + sa.select(roles.c.permissions, roles.c.builtin).where(roles.c.role_id == role_id) + ).fetchone() + if role_row is None: + return {"baseline": [], "grants": [], "revokes": [], "effective": []} + baseline = _split_perms(role_row[0]) + grants: set[str] = set() + revokes: set[str] = set() + if role_row[1]: + ov_rows = conn.execute( + sa.select( + role_permission_overrides.c.permission, + role_permission_overrides.c.action, + ).where(role_permission_overrides.c.role_id == role_id) + ).fetchall() + for perm, action in ov_rows: + if action == "grant": + grants.add(perm) + elif action == "revoke": + revokes.add(perm) + effective = (baseline | grants) - revokes + return { + "baseline": sorted(baseline), + "grants": sorted(grants), + "revokes": sorted(revokes), + "effective": sorted(effective), + } + + def effective_role_permissions_bulk( + self, role_ids: list[str] + ) -> dict[str, dict[str, list[str]]]: + if not role_ids: + return {} + with self._conn() as conn: + role_rows = conn.execute( + sa.select(roles.c.role_id, roles.c.permissions, roles.c.builtin).where( + roles.c.role_id.in_(role_ids) + ) + ).fetchall() + if not role_rows: + return {} + builtin_role_ids = [r[0] for r in role_rows if r[2]] + grants: dict[str, set[str]] = {} + revokes: dict[str, set[str]] = {} + if builtin_role_ids: + ov_rows = conn.execute( + sa.select( + role_permission_overrides.c.role_id, + role_permission_overrides.c.permission, + role_permission_overrides.c.action, + ).where(role_permission_overrides.c.role_id.in_(builtin_role_ids)) + ).fetchall() + for rid, perm, action in ov_rows: + if action == "grant": + grants.setdefault(rid, set()).add(perm) + elif action == "revoke": + revokes.setdefault(rid, set()).add(perm) + out: dict[str, dict[str, list[str]]] = {} + for rid, perms_str, builtin in role_rows: + baseline = _split_perms(perms_str) + role_grants = grants.get(rid, set()) if builtin else set() + role_revokes = revokes.get(rid, set()) if builtin else set() + effective = (baseline | role_grants) - role_revokes + out[rid] = { + "baseline": sorted(baseline), + "grants": sorted(role_grants), + "revokes": sorted(role_revokes), + "effective": sorted(effective), + } + return out + # -- Organizations --------------------------------------------------------- def create_org(self, org_id: str, name: str, display_name: str, settings: str = "{}") -> None: diff --git a/turnstone/core/storage/_utils.py b/turnstone/core/storage/_utils.py index 2608bc55..a7bf8e6b 100644 --- a/turnstone/core/storage/_utils.py +++ b/turnstone/core/storage/_utils.py @@ -143,6 +143,13 @@ def row_to_dict(row: Any, *bool_fields: str) -> dict[str, Any]: return d +def split_perms(value: str | None) -> set[str]: + """Split the comma-separated ``roles.permissions`` column into a set.""" + if not value: + return set() + return {p.strip() for p in value.split(",") if p.strip()} + + # --------------------------------------------------------------------------- # Field allowlists for governance update methods # --------------------------------------------------------------------------- diff --git a/turnstone/core/storage/migrations/versions/058_role_permission_overrides.py b/turnstone/core/storage/migrations/versions/058_role_permission_overrides.py new file mode 100644 index 00000000..08edfc32 --- /dev/null +++ b/turnstone/core/storage/migrations/versions/058_role_permission_overrides.py @@ -0,0 +1,61 @@ +"""Add ``role_permission_overrides`` for editing builtin role permissions. + +Builtin roles (``builtin-admin``, ``builtin-operator``, ``builtin-viewer``) +are seeded by migration 008 and treated as immutable — the ``roles.permissions`` +column on those rows is the *baseline* that subsequent feature migrations +extend (most recently ``040_coord_cluster_admin_perms`` and +``042_coord_trust_send_perm``). Some permissions are deliberately +default-ungranted — ``model.skills.write`` is the motivating case: it gates +the ``skills(action=create|update|...)`` in-process tool path and an +operator should consciously opt themselves in before a coordinator session +can mutate the skill catalog. Until now there was no UX to grant such a +permission without dropping into SQL. + +This table stores per-(role_id, permission) grant/revoke deltas. The +effective set for a role is computed at permission-load time as +``baseline ∪ {action=grant} − {action=revoke}``; ``roles.permissions`` +stays as today (still the baseline on builtin rows, still the full set on +custom rows where overrides do not apply). + +Composite PK ``(role_id, permission)`` collapses repeat toggles for the +same permission onto one row. No FK to ``roles`` — matches the rest of +the governance schema (migration 008 does not declare FKs either) and +keeps the postgres dialect aligned with sqlite. + +Revision ID: 058 +Revises: 057 +Create Date: 2026-05-24 +""" + +import sqlalchemy as sa +from alembic import op + +revision = "058" +down_revision = "057" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_table( + "role_permission_overrides", + sa.Column("role_id", sa.Text, nullable=False), + sa.Column("permission", sa.Text, nullable=False), + sa.Column("action", sa.Text, nullable=False), + sa.Column("created", sa.Text, nullable=False), + sa.Column("created_by", sa.Text, nullable=False, server_default=""), + sa.PrimaryKeyConstraint("role_id", "permission"), + ) + op.create_index( + "idx_role_permission_overrides_role", + "role_permission_overrides", + ["role_id"], + ) + + +def downgrade() -> None: + op.drop_index( + "idx_role_permission_overrides_role", + table_name="role_permission_overrides", + ) + op.drop_table("role_permission_overrides") diff --git a/turnstone/server.py b/turnstone/server.py index d6315fd8..9a274abe 100644 --- a/turnstone/server.py +++ b/turnstone/server.py @@ -4078,11 +4078,28 @@ def create_app( # both saved AND loaded is a normal display state. saved_loaded_lookup=None, ) - approve_handler = make_approve_handler(interactive_endpoint_config) + # ``accepted_permissions`` gates the lifted body on any one of the + # named perms when ``cfg.permission_gate`` is ``None`` (interactive + # case) — for the interactive kind it IS the primary gate, not a + # fallback. Coord's ``permission_gate`` (admin.coordinator) takes + # precedence on the coord-config side; here we accept ``admin. + # coordinator`` as a parallel allow so a coord session spawning an + # interactive child workstream isn't blocked by the operator-style + # perm requirement. Was a pre-existing security smell: the + # ``workstreams.create`` / ``workstreams.close`` / ``tools.approve`` + # perms were declared and seeded into builtin-operator's baseline + # but never wired to a gate — any authenticated user could hit + # these endpoints regardless of role. See PR adding 057_role_ + # permission_overrides for the audit that surfaced this. + approve_handler = make_approve_handler( + interactive_endpoint_config, + accepted_permissions=("tools.approve", "admin.coordinator"), + ) close_handler = make_close_handler( interactive_endpoint_config, audit_emit=_audit_close_workstream, supports_close_reason=True, + accepted_permissions=("workstreams.close", "admin.coordinator"), ) cancel_handler = make_cancel_handler(interactive_endpoint_config) open_handler = make_open_handler( @@ -4096,6 +4113,7 @@ def create_app( create_handler = make_create_handler( interactive_endpoint_config, audit_emit=_audit_workstream_created, + accepted_permissions=("workstreams.create", "admin.coordinator"), ) list_handler = make_list_handler(interactive_endpoint_config) saved_handler = make_saved_handler(interactive_endpoint_config)