diff --git a/CLAUDE.md b/CLAUDE.md index af707b04..548b6341 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -467,7 +467,7 @@ kbagent auth register-projects [--stack URL|alias] [--all] [--project-id ID ...] # bearer token, and revoking the session is a host-operator action, not a remote one. `/auth/*` # is also the FIRST router to enforce the `permissions` policy: every route declares # `Depends(require_permission(...))`, so a denial answers HTTP 403 `PERMISSION_DENIED` over REST -# exactly as on the CLI. The other ~30 routers do not check the engine yet. +# exactly as on the CLI. Every other route is checked too since #655 (see `permissions` below). # The policy comes from the config dir `serve` RESOLVES (its own `--config-dir`, then -- since # 0.91.0, #679 -- an explicit root `kbagent --config-dir`, then KBAGENT_CONFIG_DIR, then the # local/global chain) plus the session flags of the invocation. On 0.90.1 the root-level @@ -479,7 +479,7 @@ kbagent auth register-projects [--stack URL|alias] [--all] [--project-id ID ...] # the allow list. `kbagent --deny-writes serve` does NOT work: `serve` is admin-class and # `--deny-writes` appends `cli:write`, which spans write+destructive+admin, so the CLI callback # blocks the `serve` command itself (exit 6) and no server ever starts. `--deny-destructive` -# starts the server but no `/auth/*` operation is destructive, so it affects nothing here. +# starts the server and blocks every destructive route (none of them under `/auth/*`). # See docs/web-server.md. kbagent project create --url URL [--project ALIAS] [--name NAME] [--backend snowflake|bigquery] [--sync-backend-init] @@ -813,6 +813,25 @@ kbagent token refresh --project NAME --token-id ID [--yes] # is persisted. `permissions show` / `kbagent doctor` also generalized: they now flag ANY # persisted pattern matching zero known operations, not only the retired tool:* namespace. # PermissionEngine itself stays lenient at evaluation time -- only `permissions set` is strict. +# (since vNEXT, #655) The policy also firewalls the WHOLE `kbagent serve` REST surface: every +# route is classified in server/route_permissions.py and checked by one app-level dependency +# against the SERVED config dir's policy -> HTTP 403 error_code PERMISSION_DENIED, the same +# code the CLI exits on. On 0.98.0 and older only /auth/* (0.90.1+) and /merge-requests/* +# (0.94.0+) were checked and every other route executed unchecked. Routes that declare their +# own `require_permission(...)` (those two routers) are skipped by the table. An unclassified +# route is REFUSED, not exempted (a test keeps the table +# and the live app in sync both ways); bootstrap paths (/health/ping, /health/auth-info, +# /ui-config, /docs, /redoc, /openapi.json, SPA shell) are never checked. A path param +# that collapses CLI leaves is checked like the CLI: POST/PUT/DELETE +# /semantic-layer/items/{kind}[/{name}] check the parent key (`semantic-layer.add`) AND +# the leaf `semantic-layer.add.` (route_permissions.LEAF_PATH_PARAMS). +# A `--scope organization` semantic-layer write is destructive-class over REST too: +# the handlers of scope PUT, models, items/{kind} (per-kind leaf key), import, promote +# and build check ` --scope organization` (FLAG_ESCALATIONS) for a typed OR an +# inherited organization scope, like the CLI. +# `GET /permissions/show` (serve, since vNEXT) reports the EFFECTIVE policy (persisted block +# merged with the daemon's --deny-* flags); read-only by design -- no REST route can widen +# the policy that constrains it, so `permissions set|reset` stay terminal actions on the host. kbagent permissions list [--category read|write|destructive|admin] kbagent permissions show kbagent permissions set --mode allow|deny [--allow PATTERN ...] [--deny PATTERN ...] diff --git a/docs/web-server-endpoints.md b/docs/web-server-endpoints.md index 0a9b0d16..2cd3e525 100644 --- a/docs/web-server-endpoints.md +++ b/docs/web-server-endpoints.md @@ -9,7 +9,7 @@ auth, and the concepts behind these routes live in [`web-server.md`](web-server.md); a running server serves the same spec interactively at `/docs` (Swagger) and `/openapi.json`. -**256 operations** across **221 paths** and **31 routers**. +**257 operations** across **222 paths** and **32 routers**. Paths are shown as the server registers them. Reaching them through the Node BFF (or single-process `--ui` mode) prefixes every path with `/api`. @@ -490,6 +490,14 @@ Developer Portal app discovery -- list a vendor's apps, get one app's full entry ## System +### `permissions` (1 operation) + +Read the session firewall policy this server enforces on every route (issue #655). Read-only: `permissions set|reset` stay terminal actions on the host, so a bearer token can never widen the policy that constrains it. Mirrors `kbagent permissions show`. + +| Method | Path | Summary | +|---|---|---| +| `GET` | `/permissions/show` | Show the active permission policy | + ### `health` (6 operations) Liveness ping, auth-info bootstrap, version, changelog, and doctor checks. `/health/ping` is the only public endpoint -- everything else requires Bearer auth. diff --git a/docs/web-server.md b/docs/web-server.md index 9c3e83bf..35acff6d 100644 --- a/docs/web-server.md +++ b/docs/web-server.md @@ -100,14 +100,13 @@ the web UI's chat. `agents` mirrors `kbagent agent *` (both sides read the same `agents.json`); what is serve-only there is the cron loop, so a scheduled task fires only while the server runs. `auth` mirrors only the read/audit half of `kbagent auth` *(since v0.90.1)* — `login` / -`login-password` / `logout` deliberately have no endpoint — and it is so far -the only router that enforces the permission policy; see "`/auth/*` — three -read/audit endpoints, three deliberate gaps" below. +`login-password` / `logout` deliberately have no endpoint. Going the other way, several CLI surfaces are deliberately CLI-only: `sync` -(filesystem-local by design), `permissions`, and `init`. The mirrors still -considered missing are tracked in #657, and the fact that `permissions` -constrains only `/auth/*` and not the other ~30 routers is #655. +(filesystem-local by design), `init`, and the *write* half of `permissions` +(`set` / `reset`). The mirrors still considered missing are tracked in #657. +`GET /permissions/show` *(since vNEXT)* is the read half — see "The session +firewall applies to every route" below. Auto-generated OpenAPI spec at `/openapi.json`, Swagger UI at `/docs`. @@ -485,23 +484,25 @@ None of the three response shapes (`ProjectCandidatesResult`, `RegisterProjectsResult`, `AuthStatusResult`) ever carries a token value, including the `kbc-session://` sentinel. -`/auth/*` is also the **first router to enforce the permission policy**: every -route above declares `Depends(require_permission(...))`, so a denied operation -answers **HTTP 403** with `error_code: PERMISSION_DENIED` — the same code the -CLI exits on. The other ~30 routers do not check the engine yet; see the -gotchas entry on this before assuming a deny policy firewalls the whole REST -surface. +`/auth/*` was the **first router to enforce the permission policy** (0.90.1): +every route above declares `Depends(require_permission(...))` at its own call +site. Since vNEXT that is no longer special — *every* route on the server is +enforced (see "The session firewall applies to every route" below) — and the +three inline declarations survive only as the per-route override form. The policy in force is the **persisted `permissions` block of the config dir `serve` resolves**, plus whichever session flags the `kbagent` invocation -carried. Two consequences worth knowing before you reach for a flag: +carried. The server reads that block once, when it starts. After +`permissions set` or `permissions reset`, restart `kbagent serve`: until then +the running server keeps the old policy. Two consequences worth knowing before +you reach for a flag: - **`kbagent --deny-writes serve` never starts the server.** `serve` is classified `admin`, and `--deny-writes` appends `cli:write`, which spans write + destructive + admin — so the CLI callback blocks the `serve` command itself (exit code 6, `Operation 'serve' is blocked by the active permission - policy`). `--deny-destructive` does start the server, but no `/auth/*` - operation is destructive, so it changes nothing here. + policy`). `--deny-destructive` does start the server, and since vNEXT it + blocks every destructive route (no `/auth/*` operation is destructive). - **Use a persisted policy instead.** Run, on the host, in a real terminal (`permissions set` requires a typed confirmation code — there is no `--yes`): @@ -573,6 +574,84 @@ for a human), then use `POST /auth/register-projects` — or `auth register-projects` on the CLI — to register the resulting session's projects for `serve` to use. +### The session firewall applies to every route + +*(since vNEXT — issue #655)* + +`PermissionEngine` used to be built only in the Typer callback, so a persisted +`permissions set --mode deny` policy — and both `--deny-writes` and +`--deny-destructive` — protected the CLI process and nothing else. `kbagent +serve` exposed every route, `DELETE /storage/buckets` included, behind a single +all-or-nothing bearer token. 0.90.1 closed that for `/auth/*` and 0.94.0 for +`/merge-requests/*`; vNEXT closes it for the whole surface. + +**How it works.** One app-level dependency (`server/route_permissions.py`) runs +on every request, looks the matched route up by `(method, path template)` in +`ROUTE_OPERATIONS`, and calls the same `PermissionEngine.check_or_raise` the CLI +uses. A denial answers **HTTP 403** with `error_code: PERMISSION_DENIED` and the +same message the CLI prints, so a client can branch on one value across both +surfaces. The body is the error envelope every other `serve` error uses +(`{"status": "error", "error": {"code", "message"}}`). A route that declares its +own `Depends(require_permission(...))` (`/auth/*`, `/merge-requests/*`) is +checked by that dependency and skipped by the table. + +Three properties worth knowing: + +- **A route with no classification is refused, not exempted.** Failing open + would make the newest, least-reviewed part of the surface the one outside the + firewall. A test (`tests/test_server_route_permissions.py`) asserts the table + matches the live app in both directions, so the refusal path should never be + reached in a released build — if you hit it, a route was added without a table + entry. +- **No policy configured changes nothing.** With a clean `config.json` and no + session flags, `is_allowed` returns True for everything and the surface + behaves exactly as it did before. +- **Bootstrap paths are never checked** (`UNGUARDED_PATHS`: `/health/ping`, + `/health/auth-info`, `/ui-config`, `/docs`, `/redoc`, `/openapi.json`, and the + SPA shell). A locked-down server must still be able to say who it is; + otherwise a client cannot tell a policy refusal from a dead process. + +**Per-kind leaf keys.** `POST /semantic-layer/items/{kind}`, `PUT +/semantic-layer/items/{kind}/{name}` and `DELETE +/semantic-layer/items/{kind}/{name}` cover all five kinds in one route each. Like +the CLI, the server checks two keys for them: the parent (`semantic-layer.add`) +and the leaf built from the `{kind}` path parameter (`semantic-layer.add.metric`). +A policy that names only a leaf key, or a leaf glob like `semantic-layer.add.*`, +is enforced over REST the same as on the CLI. A `mode: deny` policy must allow +both keys, as on the CLI. + +**`--scope organization` writes are destructive-class over REST too.** The table +sees the route, not the body, so it checks only the non-flag keys (`write`). An +organization scope is one-way and makes an item visible to every project in the +organization, so `FLAG_ESCALATIONS` classifies it `destructive`, and the +handlers add that check: `PUT /semantic-layer/scope/{context_id}` with +`"scope": "organization"` checks `semantic-layer.scope.set --scope +organization`; `POST /semantic-layer/models`, `POST /semantic-layer/items/{kind}` +(per kind: `semantic-layer.add. --scope organization`), `POST +/semantic-layer/import` and `POST /semantic-layer/promote` check their own +escalated key when the body asks for `organization`. As on the CLI, an omitted +`scope` that would INHERIT `organization` from the target model (`items`, +`import`, `promote`, and `build` with a `model`) gets the same check; the model +lookup runs only when a policy is active. Under `--deny-destructive` these calls +answer 403 `PERMISSION_DENIED`; the same calls at `project` or `targeted` scope +are allowed. + +**Discovering the policy.** `GET /permissions/show` returns the *effective* +policy the server enforces — the persisted block already merged with the +`--deny-*` flags the daemon was launched with, which a REST caller can neither +see nor change: + +```json +{"active": true, "policy": {"mode": "allow", "allow": [], "deny": ["cli:destructive"]}} +``` + +It stays reachable under any policy (`permissions.*` operations are always +allowed, by the same anti-lockout rule the CLI has) but still requires the +bearer token. There is deliberately **no write counterpart**: letting a bearer +token widen the policy that constrains it would make the firewall +self-defeating, so `permissions set` / `reset` stay terminal actions on the +host. + ### Manage tokens are per-request Operations that need a Keboola Manage API token (`org setup`, diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index 88075317..7b936aa7 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -107,6 +107,7 @@ All seven commands authenticate via `KBC_MANAGE_API_TOKEN` (Manage API), not the ## Permissions (session firewall commands) The `permissions` subcommands persist a write/destructive policy to config.json (the `--deny-*` flags above are the one-shot form). The engine guards against agent mistakes; it is not a sandbox. +Since vNEXT (#655) the persisted policy also firewalls the whole `kbagent serve` REST surface -- every route is checked against the SERVED config dir's policy and a denial answers HTTP 403 `PERMISSION_DENIED`. On 0.98.0 and older only `/auth/*` (0.90.1+) and `/merge-requests/*` (0.94.0+) were checked. A `--scope organization` semantic-layer write (typed or inherited from the target model) is destructive-class over REST too, as on the CLI. `POST/PUT/DELETE /semantic-layer/items/{kind}` check the parent key and the per-kind leaf key (`semantic-layer.add.metric`), as on the CLI. `GET /permissions/show` (serve-only, since vNEXT) reports the EFFECTIVE policy; there is deliberately no REST route that CHANGES it. - `permissions list [--category read|write|destructive|admin]` -- list all operations with their risk category and current allowed/denied status - `permissions show` -- show the current active permission policy. *(since 0.91.0)* also flags ANY persisted pattern matching zero known operations, not only the retired `tool:*` namespace (same generalization applies to `kbagent doctor`) -- a typo like `cli:reed` or `stroage.upload-table` is surfaced too, with a generic "check for typos" hint instead of the MCP-migration one - `permissions set --mode allow|deny [--allow PATTERN ...] [--deny PATTERN ...]` -- set the permission policy (firewall rules); patterns like `cli:read`, `cli:write`, `cli:destructive`. `tool:*` patterns are INERT since v0.85.0 (the MCP passthrough is gone) -- they load but match nothing, so a `--mode deny` policy whose only allowance was `tool:read` now denies everything. *(since 0.91.0, issue #688)* Every pattern is validated BEFORE the interactive confirmation: it must be a `cli:*` category, an exact operation name, or a glob matching at least one known operation, or the call fails fast with `VALIDATION_ERROR` (exit 2, `error.details.invalid_patterns` in `--json`) and nothing is written @@ -493,7 +494,7 @@ CLI parity for the `/agents` REST surface. Reads/writes `/agents.jso - `doctor` -- health checks (no `--fix` since v0.85.0 -- it only installed the MCP server). Includes a `sync_secrets` check (since 0.55.0): when run inside a sync working tree (`.keboola/manifest.json`), warns if any in-sync config holds plaintext `#`-secrets (#378); `skip` outside a sync tree. Also an `mcp_tool_tasks` check: FAILs when `agents.json` holds tasks using the removed `mcp_tool` action, listing each task in `details.tasks[]` -- map the tool to its native command via `docs/mcp-migration.md` and recreate the task as `--type cli_command` - `version` -- show the kbagent version and check for kbagent updates (kbagent only since v0.85.0); on a standalone binary it advertises the native channel's upgrade command instead of `kbagent update` (v0.79.0+) - `context` -- full usage instructions for AI agents -- `serve [--host HOST] [--port PORT] [--ui] [--ui-dist PATH] [--reload] [--log-level LVL] [--cors-origin ORIGIN] [--config-dir DIR] [--no-banner]` -- launch the FastAPI HTTP server backing the web UI. `--ui` mounts the built SPA at `/` and authenticates the browser with an HttpOnly `kbagent_session` cookie (nothing is injected into `index.html`). `--no-banner` *(since 0.90.0)* suppresses the UI's "What's new" popup -- a curated per-version highlights modal shown once per version, dismissal persisted in localStorage `kbagent.whatsnew.seen`. The SPA reads the switch from `GET /ui-config` -> `{"banner": bool}`; it gates only the UNSOLICITED popup, so the command palette's "What's new" action still opens it on request. `--config-dir` picks the directory the server exposes (projects AND the `permissions` policy the `/auth/*` routes enforce); *(since 0.91.0)* an explicit root-level `kbagent --config-dir Y serve` is honored too, with the serve-level flag winning when both are given -- on 0.90.1 and older the root-level one was silently ignored, so pass it to `serve` itself there. Requires the optional `server` extra +- `serve [--host HOST] [--port PORT] [--ui] [--ui-dist PATH] [--reload] [--log-level LVL] [--cors-origin ORIGIN] [--config-dir DIR] [--no-banner]` -- launch the FastAPI HTTP server backing the web UI. `--ui` mounts the built SPA at `/` and authenticates the browser with an HttpOnly `kbagent_session` cookie (nothing is injected into `index.html`). `--no-banner` *(since 0.90.0)* suppresses the UI's "What's new" popup -- a curated per-version highlights modal shown once per version, dismissal persisted in localStorage `kbagent.whatsnew.seen`. The SPA reads the switch from `GET /ui-config` -> `{"banner": bool}`; it gates only the UNSOLICITED popup, so the command palette's "What's new" action still opens it on request. `--config-dir` picks the directory the server exposes (projects AND the `permissions` policy its routes enforce -- every route since vNEXT, only `/auth/*` and `/merge-requests/*` on 0.98.0 and older); *(since 0.91.0)* an explicit root-level `kbagent --config-dir Y serve` is honored too, with the serve-level flag winning when both are given -- on 0.90.1 and older the root-level one was silently ignored, so pass it to `serve` itself there. Requires the optional `server` extra ## Global Flags | Flag | Description | diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index c9149ba7..dedf8675 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -5304,24 +5304,25 @@ though single-project `job list` looked correctly time-ordered. already server-sorted); the fix only changes behavior once 2+ projects are queried together. -## `kbagent serve` permission enforcement is `/auth/*`-only so far (since v0.90.1) +## `kbagent serve` permission enforcement (since v0.90.1) `create_app` builds a `PermissionEngine` from the persisted `permissions` policy of **the config dir `serve` resolves** (its own `--config-dir`, then -- since 0.91.0 -- an explicit root-level `kbagent --config-dir`, then `KBAGENT_CONFIG_DIR`, then the local/global chain), and `kbagent serve` -forwards only the session FLAGS of the invocation on top. But of the ~30 -routers, only the three `/auth/*` routes (`server/routers/auth.py`) declare -`Depends(require_permission(...))` -- see `docs/web-server.md` for the endpoint -shapes. +forwards only the session FLAGS of the invocation on top. The three `/auth/*` +routes (`server/routers/auth.py`) and the `/merge-requests/*` routes declare +`Depends(require_permission(...))` themselves; since vNEXT every other route is +checked through one central table (below) -- see `docs/web-server.md` for the +endpoint shapes. - **`kbagent --deny-writes serve` cannot start the server** -- do not recommend it. `serve` is classified `admin` in `permissions.py`, and `--deny-writes` appends `cli:write`, which spans write+destructive+admin, so the CLI callback blocks the `serve` command itself: `Error: Operation 'serve' is blocked by the active permission policy.`, exit code 6, no - uvicorn. `--deny-destructive` does start the server, but no `/auth/*` - operation is destructive, so it changes nothing on this router. + uvicorn. `--deny-destructive` does start the server, and since vNEXT it + blocks every destructive route (no `/auth/*` operation is destructive). - **The reachable recipe is a persisted policy in the SERVED config dir.** Verified live: @@ -5337,20 +5338,49 @@ shapes. A `--mode deny` policy works too, but then `serve` (and the reads you want to keep) must be in its allow list, or the server will not start either. -- **A deny policy does NOT firewall the whole REST surface.** - `permissions set --mode deny --deny cli:write` blocks `POST - /auth/register-projects` (HTTP 403, `error_code: PERMISSION_DENIED`) but - does nothing to `POST /storage/tables/{project}` (create a table) or any - other write route on any other router -- those still execute unchecked. -- `GET /auth/projects` backs a registry key (`auth.projects`) with no CLI leaf - command -- `auth register-projects`'s interactive picker is its terminal - equivalent. It is exempted from `scripts/check_command_sync.py`'s dead-key - check via `SERVE_ONLY_OPERATIONS` in `permissions.py`; see CONTRIBUTING.md's - command-sync gate section for the general rule when adding another - serve-only operation. -- Treat this as a gap being closed incrementally, not the design end state: - today a deny policy gates only `/auth/*`, not the ~30 other routers a - session token can otherwise reach. +- **A deny policy firewalls the WHOLE REST surface (since vNEXT, #655).** + Every route is classified in `server/route_permissions.py` and checked by + one app-level dependency against the served config dir's policy, so + `permissions set --mode deny --deny cli:write` now blocks `POST + /storage/tables/{project}` exactly like it blocks `POST + /auth/register-projects` -- HTTP 403, `error_code: PERMISSION_DENIED`, same + code the CLI exits on. **On 0.98.0 and older only `/auth/*` (0.90.1+) and + `/merge-requests/*` (0.94.0+) were checked**; + on those versions every other write/destructive route executed unchecked, so + do not rely on a deny policy to contain a `serve` you did not upgrade. + Bootstrap paths (`/health/ping`, `/health/auth-info`, `/ui-config`, `/docs`, + `/redoc`, `/openapi.json`, SPA shell) are never checked, by design. + A `--scope organization` write is destructive-class over REST too, as on the + CLI: `PUT /semantic-layer/scope/{context_id}` with `"scope": "organization"`, + and `models` / `items/{kind}` / `import` / `promote` asking for (or + inheriting from the target model) `organization` scope, plus `build` into an + organization-scope model, answer 403 under `--deny-destructive`. +- **`GET /permissions/show` (since vNEXT)** reports the EFFECTIVE policy -- + the persisted block already merged with the `--deny-*` flags the daemon was + launched with. Read-only: there is no REST way to change the policy, so an + agent cannot widen the firewall that constrains it. Reachable under any + policy (`permissions.*` is always allowed), but still needs the bearer token. +- **`semantic-layer` item routes check the per-kind leaf key, like the CLI.** + `POST /semantic-layer/items/{kind}` and `PUT` / `DELETE + /semantic-layer/items/{kind}/{name}` check the parent key + (`semantic-layer.add`) and the leaf key built from `{kind}` + (`semantic-layer.add.metric`). A deny of a leaf key or of a leaf glob like + `semantic-layer.add.*` applies over REST too. A `mode: deny` policy must + allow both keys. +- Four registry keys back a serve-only surface with no CLI leaf command: + `auth.projects` (the terminal equivalent is `auth register-projects`'s + interactive picker), `merge-request.by-branch` (the CLI resolves it behind an + omitted `--merge-request-id`) plus `ai.chat` and `workspace.sql-improve` (since vNEXT -- + the dashboard's Local AI tile and the SQL editor's "improve this query" + helper; both are `write`, because both spawn a local CLI process on the host + exactly like `agent prompt-improve`). All four are exempted from + `scripts/check_command_sync.py`'s dead-key check via `SERVE_ONLY_OPERATIONS` + in `permissions.py`; see CONTRIBUTING.md's command-sync gate section before + adding another. +- A route with no entry in `ROUTE_OPERATIONS` is **refused** (403), not + exempted. A released build cannot reach that path -- a test asserts the + table matches the live app in both directions -- so seeing it means a route + was added without classifying it. ## `serve` honors the root-level `--config-dir` diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index 17fbca62..264739e7 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -2272,7 +2272,8 @@ normal chain (KBAGENT_CONFIG_DIR, .kbagent walk-up, global). Passing both is not an error; the serve-level flag wins. That directory decides which projects the REST surface exposes AND which persisted `permissions` - policy the /auth/* routes enforce. NOTE for older installs: up to 0.90.1 + policy its routes enforce (every route since vNEXT; only /auth/* and + /merge-requests/* on 0.98.0 and older). NOTE for older installs: up to 0.90.1 `serve` ignored the root-level flag entirely, silently serving a different directory -- there, always pass --config-dir to `serve` itself. @@ -2328,7 +2329,10 @@ List all operations with risk categories and current allowed/denied status. kbagent permissions show - Show current active permission policy. + Show current active permission policy. The persisted policy also applies + to every route of `kbagent serve` (denial = HTTP 403 PERMISSION_DENIED), + against the config dir the SERVER resolved; `GET /permissions/show` over + serve reports that effective policy. No REST route can change it. kbagent permissions set --mode allow|deny [--allow PATTERN ...] [--deny PATTERN ...] Set firewall-style permission policy. Patterns: exact (branch.delete), diff --git a/src/keboola_agent_cli/permissions.py b/src/keboola_agent_cli/permissions.py index 5feab1c5..a4404ce8 100644 --- a/src/keboola_agent_cli/permissions.py +++ b/src/keboola_agent_cli/permissions.py @@ -372,16 +372,24 @@ "semantic-layer.scope.set": "write", "semantic-layer.scope.request-create": "write", "semantic-layer.scope.request-delete": "write", + # Serve-only AI helpers (since vNEXT). Neither touches Keboola, but both + # spawn a local `claude` / `codex` / `gemini` process on the host, exactly + # like `agent prompt-improve` -- classified `write` for the same reason, so + # one --deny-writes keeps an agent from spawning subprocesses through any + # of the three. Exempted from the dead-key check via SERVE_ONLY_OPERATIONS. + "ai.chat": "write", + "workspace.sql-improve": "write", # Raw HTTP client against `kbagent serve` (used by AI subprocesses). # Categorised by the underlying HTTP method under the taxonomy at the top # of this registry: GET = read, POST/PATCH = write (they create/modify), # DELETE = destructive (it deletes). DELETE is deliberately a rung above # the other mutating verbs -- lumping it in as `write` would let a # `--deny-destructive` session delete through the REST boundary. - # These keys are the ONLY firewall an `http.*` call meets for - # most routes: on the serve side only `/auth/*` re-checks the policy - # (since 0.90.1) -- every other router is still unguarded, so a deny - # policy does not survive the REST boundary there (issue #655). + # Since vNEXT (issue #655) the serve's own routes DO enforce the served + # config dir's policy on top, so a denied operation is refused at both + # ends; before that, only `/auth/*` (since 0.90.1) and `/merge-requests/*` + # re-checked the policy and every other router was unguarded, so these + # keys were the only firewall an `http.*` call met there. "http.get": "read", "http.post": "write", "http.patch": "write", @@ -476,7 +484,17 @@ # they have no CLI leaf command, so the command-sync gate would otherwise report # them as dead keys -- `scripts/check_command_sync.py` subtracts this set before # its "key matching no live command" check. -SERVE_ONLY_OPERATIONS: frozenset[str] = frozenset({"auth.projects", "merge-request.by-branch"}) +SERVE_ONLY_OPERATIONS: frozenset[str] = frozenset( + { + "auth.projects", + "merge-request.by-branch", + # Both back a web-UI affordance with no terminal equivalent: the + # dashboard's Local AI chat tile and the workspace SQL editor's + # "improve this query" helper. + "ai.chat", + "workspace.sql-improve", + } +) # The operation namespace that disappeared with the MCP passthrough, and the @@ -650,6 +668,20 @@ def active(self) -> bool: """Whether a permission policy is configured.""" return self._policy is not None + @property + def policy(self) -> PermissionPolicy | None: + """The EFFECTIVE policy this engine evaluates, or None when unrestricted. + + On the serve side the engine is built by ``create_app`` from the served + directory's persisted policy already merged with ``--deny-writes`` / + ``--deny-destructive`` (:func:`apply_firewall_flags`), so this is what + ``GET /permissions/show`` must report: a caller needs the rules that + actually apply, not the persisted half of them. + + Read-only by design -- a policy is chosen once, at engine construction. + """ + return self._policy + def is_allowed(self, operation: str) -> bool: """Check if an operation is allowed by the active policy. diff --git a/src/keboola_agent_cli/server/_serve_command_map.py b/src/keboola_agent_cli/server/_serve_command_map.py index 89e2871d..34ef218f 100644 --- a/src/keboola_agent_cli/server/_serve_command_map.py +++ b/src/keboola_agent_cli/server/_serve_command_map.py @@ -120,6 +120,7 @@ ): "merge-request diff", ("GET", "/notifications"): "notification list", ("GET", "/notifications/{project}/{subscription_id}"): "notification detail", + ("GET", "/permissions/show"): "permissions show", ("GET", "/projects"): "project list", ("GET", "/projects/current"): "project current", ("GET", "/projects/status"): "project status", diff --git a/src/keboola_agent_cli/server/app.py b/src/keboola_agent_cli/server/app.py index 07354ac0..6b358d24 100644 --- a/src/keboola_agent_cli/server/app.py +++ b/src/keboola_agent_cli/server/app.py @@ -42,6 +42,7 @@ install_registry, translate_project_refs, ) +from .route_permissions import enforce_route_permission from .routers import ( agents, ai_chat, @@ -75,6 +76,9 @@ transformation, workspaces, ) +from .routers import ( + permissions as permissions_router, +) logger = logging.getLogger(__name__) @@ -380,6 +384,17 @@ ), }, # ---- System ---- + { + "name": "permissions", + "description": ( + "**System.** " + "Read the session firewall policy this server enforces on every " + "route (issue #655). Read-only: `permissions set|reset` stay " + "terminal actions on the host, so a bearer token can never widen " + "the policy that constrains it. " + "Mirrors `kbagent permissions show`." + ), + }, { "name": "health", "description": ( @@ -849,8 +864,17 @@ async def _lifespan(app_: FastAPI): app = FastAPI( lifespan=_lifespan, # type: ignore[arg-type] + # The session firewall, applied to EVERY route the app declares + # (issue #655). App-level rather than per-router so a new router + # cannot be added outside it; the route -> operation classification + # lives in `route_permissions.ROUTE_OPERATIONS`, and an unclassified + # route is refused rather than silently exempted. Listed first, so a + # denied request is refused before any other dependency runs. # A project ID in `{project}` / `?project=` becomes its alias (CLI-22). - dependencies=[Depends(translate_project_refs("project"))], + dependencies=[ + Depends(enforce_route_permission), + Depends(translate_project_refs("project")), + ], title="kbagent serve", description=APP_DESCRIPTION, version=__version__, @@ -965,6 +989,7 @@ async def _generic_handler(_request, exc: Exception): return _format_error(str(exc) or repr(exc), ErrorCode.INTERNAL_ERROR, http_status=500) app.include_router(health.router) + app.include_router(permissions_router.router) app.include_router(auth.router) app.include_router(projects.router) app.include_router(members.router) diff --git a/src/keboola_agent_cli/server/dependencies.py b/src/keboola_agent_cli/server/dependencies.py index c276a134..12d19372 100644 --- a/src/keboola_agent_cli/server/dependencies.py +++ b/src/keboola_agent_cli/server/dependencies.py @@ -228,6 +228,14 @@ def get_permission_engine(request: Request) -> PermissionEngine: return engine # type: ignore[no-any-return] +# Attribute stamped on the callable :func:`require_permission` returns, so the +# app-level dependency in ``route_permissions`` can recognise a route that +# already guards itself and skip its own (table-driven) lookup. A plain +# ``hasattr`` check on some ad-hoc name would be guesswork; a named constant +# makes the handshake between the two modules greppable. +PERMISSION_DEPENDENCY_MARKER = "kbagent_permission_operation" + + def require_permission(operation: str) -> Callable[[PermissionEngine], None]: """Build a FastAPI dependency enforcing the permission policy for ``operation``. @@ -246,6 +254,13 @@ def require_permission(operation: str) -> Callable[[PermissionEngine], None]: The engine comes from :func:`get_permission_engine` (app state), never from the registry, so overriding ``get_registry`` in a test cannot disable the check. + + Since vNEXT (issue #655) this is the per-route OVERRIDE form, not the + default one: every route is classified centrally in + ``server/route_permissions.py`` and checked by one app-level dependency. + Use this when a route needs an operation the table cannot express -- and + when you do, the app-level dependency stands down for that route + (:data:`PERMISSION_DEPENDENCY_MARKER` is how it recognises one). """ def _check_permission( @@ -253,6 +268,7 @@ def _check_permission( ) -> None: engine.check_or_raise(operation) + setattr(_check_permission, PERMISSION_DEPENDENCY_MARKER, operation) return _check_permission diff --git a/src/keboola_agent_cli/server/route_permissions.py b/src/keboola_agent_cli/server/route_permissions.py new file mode 100644 index 00000000..8a12e3f4 --- /dev/null +++ b/src/keboola_agent_cli/server/route_permissions.py @@ -0,0 +1,532 @@ +"""Route-to-operation map that puts the REST surface behind the session firewall. + +Issue #655: ``PermissionEngine`` used to be built only in the Typer callback, +so a persisted ``permissions set --mode deny`` policy -- and both +``--deny-writes`` / ``--deny-destructive`` -- protected the CLI process and +nothing else. ``kbagent serve`` exposed every route, ``DELETE +/storage/buckets`` included, behind one all-or-nothing bearer token. + +#677 built the enforcement machinery (an engine on ``app.state``, a +``PermissionDeniedError`` -> HTTP 403 handler, and the +:func:`~keboola_agent_cli.server.dependencies.require_permission` dependency) +but wired it to three ``/auth/*`` routes only. This module supplies the +missing half: coverage. + +Why one central table instead of 231 per-route declarations +----------------------------------------------------------- +FastAPI puts the matched route object into ``request.scope["route"]`` *before* +dependencies run, so a single app-level dependency can look the request up by +``(method, route.path)``. That buys two things a scattered declaration cannot: + +1. **One auditable screen.** A reviewer answering "what can a caller still do + under ``--deny-destructive``?" reads one file, not thirty routers. +2. **Fail-closed by construction.** A route with no entry is *denied*, not + silently allowed -- the failure mode of a forgotten annotation is a loud + 403, never an open door. ``tests/test_server_route_permissions.py`` keeps + that path unreachable in practice by asserting the table covers the live + app exactly, in both directions. + +The per-route form still works and still wins: a route declaring +``Depends(require_permission(...))`` inline is enforced by that dependency and +skipped here (see :func:`resolve_route_operation`). That is what keeps the +``/auth/*`` routes from #677 -- and test-only probe routes registered after +``create_app`` -- working unchanged. + +Leaf keys from a path parameter +------------------------------- +``POST /semantic-layer/items/{kind}`` covers ``metric``/``dataset``/... in one +route. The CLI checks two keys for ``semantic-layer add metric``: the parent +``semantic-layer.add`` and the leaf ``semantic-layer.add.metric``. The table +maps the route to the parent key, and :data:`LEAF_PATH_PARAMS` names the path +parameter that the dependency appends to it for the second check. So a policy +that names only a leaf key, or a leaf glob like ``semantic-layer.add.*``, is +enforced over REST the same as on the CLI. + +Flag escalations +---------------- +The table sees the route, not the body, so it never checks a flag key. The +semantic-layer writes that can land at ``organization`` scope (``FLAG_ESCALATIONS`` +makes that destructive-class) add the escalated check in their handlers, with +the per-kind leaf key for ``POST /semantic-layer/items/{kind}``. +""" + +from __future__ import annotations + +from fastapi import Depends, Request + +from ..permissions import OPERATION_REGISTRY, PermissionEngine +from .dependencies import get_permission_engine + +# Paths served without a permission check. +# +# All of them are bootstrap surface: they carry no project data and no side +# effects, and denying them would leave a caller unable to discover *why* it +# is being denied. ``/health/ping``, ``/health/auth-info``, ``/docs``, +# ``/redoc`` and ``/openapi.json`` are already unauthenticated +# (``server/auth.py``'s ``PUBLIC_PATHS``); ``/ui-config`` and the SPA shell +# routes still require the bearer token, they just carry no policy decision. +# +# FastAPI's own ``/docs``, ``/redoc`` and ``/openapi.json`` are plain Starlette +# routes registered inside ``FastAPI.__init__``, so the app-level dependency +# never attaches to them at all. They are listed anyway: the exemption should +# read as a decision, not as an accident of registration order. +# Only reads of the paths below are exempt (see :func:`is_unguarded`). +_UNGUARDED_METHODS: frozenset[str] = frozenset({"GET", "HEAD"}) + +UNGUARDED_PATHS: frozenset[str] = frozenset( + { + "/health/ping", + "/health/auth-info", + "/ui-config", + "/openapi.json", + "/docs", + "/docs/oauth2-redirect", + "/redoc", + # `kbagent serve --ui` SPA shell (see server/__init__.py::_install_ui). + "/", + "/index.html", + } +) + +# (HTTP method, route path template) -> OPERATION_REGISTRY key. +# +# The path is ``APIRoute.path`` verbatim -- including ``:path`` converters -- +# so an entry can be copy-pasted from, and grepped against, the router +# decorator it mirrors. +ROUTE_OPERATIONS: dict[tuple[str, str], str] = { + # ── health / meta ──────────────────────────────────────────────── + ("GET", "/version"): "version", + ("GET", "/changelog"): "changelog", + ("GET", "/doctor"): "doctor", + ("GET", "/permissions/show"): "permissions.show", + # ── projects ───────────────────────────────────────────────────── + ("GET", "/projects"): "project.list", + ("POST", "/projects"): "project.add", + # Serve-only bulk form of `project remove`; same blast radius, so the + # same admin-class key rather than a weaker one of its own. + ("POST", "/projects/bulk-delete"): "project.remove", + ("DELETE", "/projects/{alias}"): "project.remove", + ("PATCH", "/projects/{alias}"): "project.edit", + ("GET", "/projects/status"): "project.status", + ("GET", "/projects/current"): "project.current", + ("POST", "/projects/use/{alias}"): "project.use", + ("GET", "/projects/{alias}/info"): "project.info", + ("GET", "/projects/{alias}/description"): "project.description-get", + ("PUT", "/projects/{alias}/description"): "project.description-set", + # ── members ────────────────────────────────────────────────────── + ("GET", "/members/{project}"): "project.member-list", + ("GET", "/members/{project}/invitations"): "project.invitation-list", + ("POST", "/members/{project}/invite"): "project.invite", + ("POST", "/members/{project}/invitations/cancel"): "project.invitation-cancel", + ("POST", "/members/{project}/remove"): "project.member-remove", + ("POST", "/members/{project}/set-role"): "project.member-set-role", + # ── feature flags (super-admin manage token) ───────────────────── + ("GET", "/feature/{project}/list"): "feature.list", + ("GET", "/feature/{project}/project-show"): "feature.project-show", + ("POST", "/feature/{project}/project-add"): "feature.project-add", + ("POST", "/feature/{project}/project-remove"): "feature.project-remove", + ("GET", "/feature/{project}/user-show"): "feature.user-show", + ("POST", "/feature/{project}/user-add"): "feature.user-add", + ("POST", "/feature/{project}/user-remove"): "feature.user-remove", + # ── billing ────────────────────────────────────────────────────── + ("GET", "/billing/credits"): "billing.credits", + # ── configurations ─────────────────────────────────────────────── + ("GET", "/configs"): "config.list", + ("GET", "/configs/search"): "config.search", + ("GET", "/configs/examples/{component_id}"): "config.examples", + ("GET", "/configs/trash/{project}"): "config.trash-list", + ("GET", "/configs/{project}/{component_id}/{config_id}"): "config.detail", + ("PATCH", "/configs/{project}/{component_id}/{config_id}"): "config.update", + ("DELETE", "/configs/{project}/{component_id}/{config_id}"): "config.delete", + ("POST", "/configs/{project}/{component_id}/{config_id}/restore"): "config.restore", + ("POST", "/configs/{project}/{component_id}"): "config.new", + ("POST", "/configs/{project}/{component_id}/{config_id}/clone"): "config.clone", + ( + "POST", + "/configs/{project}/{component_id}/{config_id}/set-default-bucket", + ): "config.set-default-bucket", + ("POST", "/configs/{project}/{component_id}/{config_id}/rename"): "config.rename", + ("GET", "/configs/{project}/{component_id}/{config_id}/metadata"): "config.metadata-list", + ("GET", "/configs/{project}/{component_id}/{config_id}/metadata/{key}"): "config.get-metadata", + ("PUT", "/configs/{project}/{component_id}/{config_id}/metadata/{key}"): "config.set-metadata", + ( + "DELETE", + "/configs/{project}/{component_id}/{config_id}/metadata/{metadata_id}", + ): "config.delete-metadata", + ("POST", "/configs/{project}/{component_id}/{config_id}/folder"): "config.set-folder", + ("POST", "/configs/{project}/{component_id}/{config_id}/rows"): "config.row-create", + ("PATCH", "/configs/{project}/{component_id}/{config_id}/rows/{row_id}"): "config.row-update", + ("DELETE", "/configs/{project}/{component_id}/{config_id}/rows/{row_id}"): "config.row-delete", + ("GET", "/configs/{project}/{component_id}/{config_id}/oauth-url"): "config.oauth-url", + ("GET", "/configs/{project}/{component_id}/{config_id}/state"): "config.state-get", + ("PUT", "/configs/{project}/{component_id}/{config_id}/state"): "config.state-set", + ("GET", "/configs/{project}/{component_id}/{config_id}/variables"): "config.variables-get", + ("PUT", "/configs/{project}/{component_id}/{config_id}/variables"): "config.variables-set", + ( + "DELETE", + "/configs/{project}/{component_id}/{config_id}/variables", + ): "config.variables-clear", + # ── components ─────────────────────────────────────────────────── + ("GET", "/components"): "component.list", + ("GET", "/components/{component_id}"): "component.detail", + # Scaffolding writes a new configuration (`config new --push`). + ("POST", "/components/{component_id}/scaffold"): "config.new", + ("POST", "/components/{component_id}/actions/{action}"): "component.sync-action", + # ── storage: buckets ───────────────────────────────────────────── + ("GET", "/storage/buckets"): "storage.buckets", + ("GET", "/storage/buckets/{project}/{bucket_id:path}"): "storage.bucket-detail", + ("POST", "/storage/buckets/{project}"): "storage.create-bucket", + ("DELETE", "/storage/buckets/{project}"): "storage.delete-bucket", + ("POST", "/storage/buckets/{project}/{bucket_id:path}/describe"): "storage.describe-bucket", + # ── storage: tables ────────────────────────────────────────────── + ("GET", "/storage/tables"): "storage.tables", + ("GET", "/storage/table-detail/{project}/{table_id:path}"): "storage.table-detail", + # Both read table DATA; `download-table` is the CLI operation that does + # the same thing, and the preview is just a row-capped variant of it. + ("GET", "/storage/table-preview/{project}/{table_id:path}"): "storage.download-table", + ("GET", "/storage/table-download/{project}/{table_id:path}"): "storage.download-table", + ("POST", "/storage/tables/{project}"): "storage.create-table", + ("POST", "/storage/tables/{project}/upload"): "storage.upload-table", + ("DELETE", "/storage/tables/{project}"): "storage.delete-table", + ("POST", "/storage/tables/{project}/truncate"): "storage.truncate-table", + ("POST", "/storage/tables/{project}/{table_id:path}/swap"): "storage.swap-tables", + ("POST", "/storage/tables/{project}/{table_id:path}/pull"): "storage.clone-table", + ("POST", "/storage/tables/{project}/{table_id:path}/describe"): "storage.describe-table", + # ── storage: columns ───────────────────────────────────────────── + ("POST", "/storage/columns/{project}/{table_id:path}"): "storage.add-column", + ("DELETE", "/storage/columns/{project}/{table_id:path}"): "storage.delete-column", + ("POST", "/storage/columns/{project}/{table_id:path}/describe"): "storage.describe-column", + ("POST", "/storage/columns/{project}/describe-migrate"): "storage.describe-migrate", + # ── storage: snapshots ─────────────────────────────────────────── + ("POST", "/storage/tables/{project}/{table_id:path}/snapshots"): "storage.snapshot-create", + ("GET", "/storage/snapshots/{project}/{table_id:path}"): "storage.snapshots", + ("GET", "/storage/snapshot-detail/{project}/{snapshot_id}"): "storage.snapshot-detail", + ("DELETE", "/storage/snapshots/{project}"): "storage.snapshot-delete", + ("POST", "/storage/table-from-snapshot/{project}"): "storage.table-from-snapshot", + # ── storage: files ─────────────────────────────────────────────── + ("GET", "/storage/files"): "storage.files", + ("POST", "/storage/files/upload"): "storage.file-upload", + ("GET", "/storage/files/{project}/{file_id}"): "storage.file-detail", + ("GET", "/storage/files/{project}/{file_id}/download"): "storage.file-download", + ("DELETE", "/storage/files/{project}"): "storage.file-delete", + ("POST", "/storage/files/{project}/{file_id}/tag"): "storage.file-tag", + ("POST", "/storage/files/{project}/load-to-table"): "storage.load-file", + # ── data streams ───────────────────────────────────────────────── + ("GET", "/stream/{project}/list"): "stream.list", + ("GET", "/stream/{project}/detail"): "stream.detail", + ("POST", "/stream/{project}/create-source"): "stream.create-source", + ("POST", "/stream/{project}/delete"): "stream.delete", + # ── scoped storage tokens ──────────────────────────────────────── + ("GET", "/token/list"): "token.list", + ("GET", "/token/{project}/list"): "token.list", + ("POST", "/token/{project}/create"): "token.create", + ("POST", "/token/{project}/delete"): "token.delete", + ("POST", "/token/{project}/refresh"): "token.refresh", + # ── jobs ───────────────────────────────────────────────────────── + ("GET", "/jobs"): "job.list", + ("GET", "/jobs/{project}/{job_id}"): "job.detail", + # SSE tail of one job -- the streaming form of `job detail`. + ("GET", "/jobs/{project}/{job_id}/stream"): "job.detail", + ("POST", "/jobs/{project}/run"): "job.run", + ("POST", "/jobs/{project}/terminate"): "job.terminate", + # ── branches ───────────────────────────────────────────────────── + ("GET", "/branches"): "branch.list", + ("POST", "/branches/{project}"): "branch.create", + ("POST", "/branches/{project}/use"): "branch.use", + ("POST", "/branches/{project}/reset"): "branch.reset", + ("DELETE", "/branches/{project}/{branch_id}"): "branch.delete", + # `branch merge` is itself only a URL producer (the merge happens in the + # web UI), so the GET mirrors it exactly -- including its `write` class. + ("GET", "/branches/{project}/merge-url"): "branch.merge", + ("GET", "/branches/{project}/metadata"): "branch.metadata-list", + ("GET", "/branches/{project}/metadata/{key}"): "branch.metadata-get", + ("PUT", "/branches/{project}/metadata/{key}"): "branch.metadata-set", + ("DELETE", "/branches/{project}/metadata/{metadata_id}"): "branch.metadata-delete", + # ── workspaces ─────────────────────────────────────────────────── + ("GET", "/workspaces"): "workspace.list", + ("POST", "/workspaces/{project}"): "workspace.create", + ("GET", "/workspaces/{project}/{workspace_id}"): "workspace.detail", + ("DELETE", "/workspaces/{project}/{workspace_id}"): "workspace.delete", + ("POST", "/workspaces/{project}/{workspace_id}/password"): "workspace.password", + ("POST", "/workspaces/{project}/{workspace_id}/load"): "workspace.load", + ("POST", "/workspaces/{project}/{workspace_id}/query"): "workspace.query", + ("POST", "/workspaces/{project}/from-transformation"): "workspace.from-transformation", + ("POST", "/workspaces/gc"): "workspace.gc", + ("POST", "/workspaces/sql/improve/stream"): "workspace.sql-improve", + # ── flows ──────────────────────────────────────────────────────── + ("GET", "/flows"): "flow.list", + ("GET", "/flows/examples"): "flow.examples", + ("POST", "/flows/validate"): "flow.validate", + ("GET", "/flows/{project}/schema"): "flow.schema", + ("GET", "/flows/{project}/{config_id}"): "flow.detail", + ("POST", "/flows/{project}"): "flow.new", + ("PATCH", "/flows/{project}/{config_id}"): "flow.update", + ("DELETE", "/flows/{project}/{config_id}"): "flow.delete", + ("GET", "/flows/{project}/{config_id}/schedules"): "schedule.list", + ("GET", "/flows/{project}/{config_id}/triggers"): "flow.triggers", + ("POST", "/flows/{project}/{config_id}/schedule"): "flow.schedule", + ("DELETE", "/flows/{project}/{config_id}/schedule"): "flow.schedule-remove", + # ── schedules / notifications ──────────────────────────────────── + ("GET", "/schedules"): "schedule.list", + ("GET", "/schedules/{project}/{schedule_id}"): "schedule.detail", + ("GET", "/schedules/find/query"): "schedule.find", + ("GET", "/notifications"): "notification.list", + ("GET", "/notifications/{project}/{subscription_id}"): "notification.detail", + ("POST", "/notifications/{project}"): "notification.create", + ("DELETE", "/notifications/{project}/{subscription_id}"): "notification.delete", + # Create-then-delete (the API has no update), `write` like the CLI command. + ( + "POST", + "/notifications/{project}/{subscription_id}/replace-recipient", + ): "notification.replace-recipient", + # ── lineage (all read-only; `build` is the only cache writer) ───── + ("POST", "/lineage/build"): "lineage.build", + ("GET", "/lineage/info"): "lineage.info", + ("POST", "/lineage/show"): "lineage.show", + ("GET", "/lineage/edges"): "lineage.show", + ("GET", "/lineage/browser"): "lineage.show", + ("GET", "/lineage/data"): "lineage.show", + ("GET", "/lineage/walk"): "lineage.show", + ("GET", "/lineage/mermaid"): "lineage.show", + # ── sharing ────────────────────────────────────────────────────── + ("GET", "/sharing"): "sharing.list", + ("GET", "/sharing/edges"): "sharing.edges", + ("POST", "/sharing/{project}/share"): "sharing.share", + ("POST", "/sharing/{project}/unshare/{bucket_id:path}"): "sharing.unshare", + ("POST", "/sharing/{project}/link"): "sharing.link", + ("POST", "/sharing/{project}/unlink/{bucket_id:path}"): "sharing.unlink", + # ── data apps ──────────────────────────────────────────────────── + ("GET", "/data-apps"): "data-app.list", + ("GET", "/data-apps/{project}/{app_id}"): "data-app.detail", + ("POST", "/data-apps/{project}"): "data-app.create", + ("POST", "/data-apps/{project}/{app_id}/deploy"): "data-app.deploy", + ("POST", "/data-apps/{project}/{app_id}/start"): "data-app.start", + ("POST", "/data-apps/{project}/{app_id}/stop"): "data-app.stop", + ("DELETE", "/data-apps/{project}/{app_id}"): "data-app.delete", + ("GET", "/data-apps/{project}/{app_id}/password"): "data-app.password", + ("GET", "/data-apps/{project}/{app_id}/logs"): "data-app.logs", + ("GET", "/data-apps/{project}/{app_id}/runs"): "data-app.runs", + ("GET", "/data-apps/{project}/{app_id}/secrets"): "data-app.secrets-list", + ("GET", "/data-apps/{project}/{app_id}/secrets/{key:path}"): "data-app.secrets-get", + ("PUT", "/data-apps/{project}/{app_id}/secrets"): "data-app.secrets-set", + ("POST", "/data-apps/{project}/{app_id}/secrets/remove"): "data-app.secrets-remove", + ("POST", "/data-apps/validate-repo"): "data-app.validate-repo", + ("GET", "/data-apps/{project}/{app_id}/git-repo"): "data-app.git-repo", + ("GET", "/data-apps/{project}/{app_id}/git-repo/credentials"): "data-app.git-credentials", + ( + "POST", + "/data-apps/{project}/{app_id}/git-repo/credentials", + ): "data-app.git-credentials-create", + # ── developer portal ───────────────────────────────────────────── + ("GET", "/dev-portal/apps"): "dev-portal.list", + ("GET", "/dev-portal/apps/{app}"): "dev-portal.get", + # ── Kai / local AI ─────────────────────────────────────────────── + ("GET", "/kai/ping"): "kai.ping", + ("GET", "/kai/preflight"): "kai.preflight", + ("POST", "/kai/ask"): "kai.ask", + ("POST", "/kai/chat"): "kai.chat", + ("GET", "/kai/history"): "kai.history", + ("GET", "/kai/chat/{chat_id}"): "kai.chat-detail", + ("POST", "/ai/chat/stream"): "ai.chat", + # ── encrypt / search / docs ────────────────────────────────────── + ("POST", "/encrypt/values"): "encrypt.values", + ("GET", "/search"): "search", + ("POST", "/documentation/query"): "docs.query", + # ── semantic layer ─────────────────────────────────────────────── + ("GET", "/semantic-layer/models"): "semantic-layer.model.list", + ("POST", "/semantic-layer/models"): "semantic-layer.model.create", + ("DELETE", "/semantic-layer/models/{model}"): "semantic-layer.model.delete", + ("GET", "/semantic-layer/show"): "semantic-layer.show", + ("GET", "/semantic-layer/validate"): "semantic-layer.validate", + ("GET", "/semantic-layer/search-context"): "semantic-layer.search-context", + ("GET", "/semantic-layer/get-context"): "semantic-layer.get-context", + ("GET", "/semantic-layer/schema"): "semantic-layer.schema", + ("GET", "/semantic-layer/export"): "semantic-layer.export", + ("POST", "/semantic-layer/diff"): "semantic-layer.diff", + # Parent keys -- the leaf `.` is checked too, see LEAF_PATH_PARAMS. + ("POST", "/semantic-layer/items/{kind}"): "semantic-layer.add", + ("PUT", "/semantic-layer/items/{kind}/{name}"): "semantic-layer.edit", + ("DELETE", "/semantic-layer/items/{kind}/{name}"): "semantic-layer.remove", + ("POST", "/semantic-layer/import"): "semantic-layer.import", + ("POST", "/semantic-layer/promote"): "semantic-layer.promote", + ("POST", "/semantic-layer/build"): "semantic-layer.build", + ("POST", "/semantic-layer/token/encrypt"): "semantic-layer.token", + ("GET", "/semantic-layer/reference-data"): "semantic-layer.reference-data.list", + ("GET", "/semantic-layer/reference-data/{record_id}"): "semantic-layer.reference-data.get", + ("PUT", "/semantic-layer/reference-data"): "semantic-layer.reference-data.set", + ( + "DELETE", + "/semantic-layer/reference-data/{record_id}", + ): "semantic-layer.reference-data.delete", + # Base keys only. An `organization` scope write escalates to the + # destructive-class ` --scope organization` inside the handler (the + # scope sits in the body, which this table never sees) -- see + # `_gate_organization_scope` in server/routers/semantic_layer.py. + ("GET", "/semantic-layer/scope/elevation-requests"): "semantic-layer.scope.request-list", + ("GET", "/semantic-layer/scope/{context_id}"): "semantic-layer.scope.get", + ("PUT", "/semantic-layer/scope/{context_id}"): "semantic-layer.scope.set", + ("POST", "/semantic-layer/scope/{context_id}/target-projects"): "semantic-layer.scope.add", + ("DELETE", "/semantic-layer/scope/{context_id}/target-projects"): "semantic-layer.scope.remove", + ( + "PUT", + "/semantic-layer/scope/{context_id}/elevation-request", + ): "semantic-layer.scope.request-create", + ( + "DELETE", + "/semantic-layer/scope/{context_id}/elevation-request", + ): "semantic-layer.scope.request-delete", + # ── transformations ────────────────────────────────────────────── + ("POST", "/transformations/{project}"): "transformation.create", + ("GET", "/transformations/{project}/{config_id}"): "transformation.show", + ("PATCH", "/transformations/{project}/{config_id}"): "transformation.edit", + # ── organization ───────────────────────────────────────────────── + ("POST", "/org/setup"): "org.setup", + # `--refresh` is a flag on the same command, not a command of its own. + ("POST", "/org/refresh"): "org.setup", + # ── scheduled agent tasks ──────────────────────────────────────── + ("GET", "/agents"): "agent.list", + ("POST", "/agents"): "agent.create", + ("GET", "/agents/cron/preview"): "agent.cron-preview", + ("POST", "/agents/test"): "agent.test", + ("POST", "/agents/test/stream"): "agent.test", + ("POST", "/agents/prompt/improve"): "agent.prompt-improve", + ("POST", "/agents/prompt/improve/stream"): "agent.prompt-improve", + ("GET", "/agents/{task_id}"): "agent.show", + ("PATCH", "/agents/{task_id}"): "agent.update", + ("DELETE", "/agents/{task_id}"): "agent.delete", + ("POST", "/agents/{task_id}/run"): "agent.run", + ("POST", "/agents/{task_id}/run/stream"): "agent.run", + ("GET", "/agents/{task_id}/runs"): "agent.runs", + ("GET", "/agents/{task_id}/runs/{run_id}"): "agent.run-detail", + ("GET", "/agents/{task_id}/runs/{run_id}/events"): "agent.run-events", +} + + +# Routes that collapse several CLI leaf commands into one path parameter. +# After the table key passes, the dependency also checks +# ``.`` -- the leaf key the CLI's +# ``check_cli_permission`` composes from the subcommand name. The value comes +# from the request, but it can only add a check: the parent key is checked +# first in every case. +LEAF_PATH_PARAMS: dict[tuple[str, str], str] = { + ("POST", "/semantic-layer/items/{kind}"): "kind", + ("PUT", "/semantic-layer/items/{kind}/{name}"): "kind", + ("DELETE", "/semantic-layer/items/{kind}/{name}"): "kind", +} + + +def resolve_route_operation(method: str, path: str) -> str | None: + """Return the operation key guarding ``method path``, or None when exempt. + + ``path`` is the route TEMPLATE (``APIRoute.path``), never the concrete + request URL -- looking policy up by a concrete URL would make the decision + depend on user-supplied identifiers. + """ + if is_unguarded(method, path): + return None + return ROUTE_OPERATIONS.get((method.upper(), path)) + + +def is_unguarded(method: str, path: str) -> bool: + """Whether ``method path`` is bootstrap surface that no policy checks. + + Only a read (GET or HEAD) of an :data:`UNGUARDED_PATHS` entry is exempt, so + a future write route on one of those paths still needs a table entry. + """ + return method.upper() in _UNGUARDED_METHODS and path in UNGUARDED_PATHS + + +def enforce_route_permission( + request: Request, + engine: PermissionEngine = Depends(get_permission_engine), +) -> None: + """App-level dependency: check the active policy for the matched route. + + Registered once on the ``FastAPI`` instance, so it runs for every route the + app declares -- there is no per-router opt-in to forget. FastAPI resolves + the route before dependencies run, so ``request.scope["route"]`` is the + matched :class:`~fastapi.routing.APIRoute` and its ``.path`` is the + template the table is keyed on. + + Three outcomes: + + * exempt path (:data:`UNGUARDED_PATHS`) -> allowed, no policy consulted; + * mapped route -> ``engine.check_or_raise(operation)``, i.e. HTTP 403 with + ``error_code: PERMISSION_DENIED`` when the policy says no, plus the leaf + key for a route in :data:`LEAF_PATH_PARAMS`; + * unmapped route -> **denied**. See :func:`_deny_unmapped`. + + With no policy configured (the default) ``is_allowed`` returns True for + everything, so a mapped route behaves exactly as it did before this + dependency existed. Only an unmapped route changes behavior -- and the + completeness test makes that state unreachable in a released build. + """ + route = request.scope.get("route") + path = getattr(route, "path", None) + if path is None: + # No matched APIRoute (a Mount, or a 404 that never resolved). Nothing + # to authorize; Starlette will answer for it. + return + + if is_unguarded(request.method, path): + return + + key = (request.method.upper(), path) + operation = ROUTE_OPERATIONS.get(key) + if operation is None: + if _declares_inline_permission(route): + # The route carries its own `require_permission(...)`, which has + # already run (route dependencies are resolved after app-level + # ones). Checking again here would need an operation the table + # does not have; the inline guard is the authority. + return + _deny_unmapped(request.method.upper(), path) + return + + engine.check_or_raise(operation) + leaf_param = LEAF_PATH_PARAMS.get(key) + if leaf_param is not None: + engine.check_or_raise(f"{operation}.{request.path_params[leaf_param]}") + + +def _declares_inline_permission(route: object) -> bool: + """Whether ``route`` declares a ``require_permission(...)`` dependency itself.""" + from .dependencies import PERMISSION_DEPENDENCY_MARKER + + for dependant in getattr(route, "dependencies", ()) or (): + call = getattr(dependant, "dependency", None) + if getattr(call, PERMISSION_DEPENDENCY_MARKER, None) is not None: + return True + return False + + +def _deny_unmapped(method: str, path: str) -> None: + """Refuse a route no entry in :data:`ROUTE_OPERATIONS` classifies. + + Failing open here would mean a route added without a table entry is exempt + from every policy -- silently, and exactly for the newest and least + reviewed part of the surface. Failing closed makes the omission a loud + 403 the first time anyone calls the route, and + ``tests/test_server_route_permissions.py`` turns it into a failing test + long before that. + """ + from ..errors import PermissionDeniedError + + raise PermissionDeniedError( + f"{method} {path}", + message=( + f"{method} {path} has no permission classification, so its risk cannot be " + "evaluated and it is refused. Add an entry to ROUTE_OPERATIONS in " + "server/route_permissions.py (or list the path in UNGUARDED_PATHS)." + ), + ) + + +def unknown_operations() -> list[str]: + """Table values that are not :data:`OPERATION_REGISTRY` keys. + + A typo'd operation would be classified ``write`` by the engine's own + fail-closed default and never match an exact-name policy pattern -- wrong + in a way nothing else notices. The completeness test asserts this is empty. + """ + return sorted({op for op in ROUTE_OPERATIONS.values() if op not in OPERATION_REGISTRY}) diff --git a/src/keboola_agent_cli/server/routers/permissions.py b/src/keboola_agent_cli/server/routers/permissions.py new file mode 100644 index 00000000..f0af6eee --- /dev/null +++ b/src/keboola_agent_cli/server/routers/permissions.py @@ -0,0 +1,63 @@ +"""Read-only discovery of the firewall policy the REST surface enforces. + +Issue #655 asked for this alongside enforcement, and the two belong together: +once a route can answer 403 PERMISSION_DENIED, a client needs a way to learn +the rules *before* it starts making calls it is not allowed to make. Without +it the only discovery channel is trial and error against destructive routes. + +There is deliberately no write counterpart. ``permissions set`` / ``reset`` +edit ``config.json`` on the host, and letting a bearer token widen the very +policy that constrains it would make the firewall self-defeating -- a policy +change stays a terminal action on the machine running the server. +""" + +from __future__ import annotations + +from typing import Any + +from fastapi import APIRouter, Depends + +from ...permissions import ( + INERT_PATTERN_HINT, + INERT_SINCE_VERSION, + PermissionEngine, + find_inert_patterns, +) +from ..dependencies import get_permission_engine + +router = APIRouter(prefix="/permissions", tags=["permissions"]) + + +@router.get("/show", summary="Show the active permission policy") +def show(engine: PermissionEngine = Depends(get_permission_engine)) -> dict[str, Any]: + """Report the policy every route on this server is checked against. + + Mirrors `kbagent permissions show --json`, with one deliberate difference: + the CLI reports the persisted policy and the session ``--deny-*`` flags as + two separate layers, while this returns the single EFFECTIVE policy the + server enforces -- ``create_app`` merges the two at startup and a REST + caller can neither see nor change the flags the daemon was launched with. + + ``active`` is False when no policy is configured at all; ``policy`` is then + null and every operation is allowed. + """ + policy = engine.policy + payload: dict[str, Any] = { + "active": policy is not None, + "policy": ( + None + if policy is None + else {"mode": policy.mode, "allow": list(policy.allow), "deny": list(policy.deny)} + ), + } + + # Additive keys, present only when the policy carries rules that can no + # longer match anything -- same wording the CLI and `doctor` use, so a + # client cannot learn a third phrasing of one problem. + inert = find_inert_patterns(policy) + if inert: + payload["inert_patterns"] = inert + payload["inert_since_version"] = INERT_SINCE_VERSION + payload["inert_hint"] = INERT_PATTERN_HINT + + return payload diff --git a/src/keboola_agent_cli/server/routers/semantic_layer.py b/src/keboola_agent_cli/server/routers/semantic_layer.py index 9ee9e95d..cb34c372 100644 --- a/src/keboola_agent_cli/server/routers/semantic_layer.py +++ b/src/keboola_agent_cli/server/routers/semantic_layer.py @@ -26,8 +26,9 @@ from pydantic import BaseModel, Field, model_validator from ...errors import ErrorCode +from ...permissions import PermissionEngine from ...services.semantic_layer_service import SCHEMA_TYPE_ALIAS -from ..dependencies import ServiceRegistry, get_registry +from ..dependencies import ServiceRegistry, get_permission_engine, get_registry router = APIRouter(prefix="/semantic-layer", tags=["semantic-layer"]) @@ -232,6 +233,32 @@ class RefDataSet(BaseModel): description: str | None = None +def _gate_organization_scope( + engine: PermissionEngine, + operation: str, + scope: str | None, + *, + inherit_from: tuple[ServiceRegistry, str, str | None] | None = None, +) -> None: + """Check `` --scope organization`` when the write lands at organization scope. + + The REST form of the CLI's ``resolve_scope_targets`` / + ``gate_inherited_organization_scope``. The route table checks only the base + key (``write``); an ``organization`` scope is one-way and widens visibility + to the whole organization, so ``FLAG_ESCALATIONS`` makes it destructive-class. + + ``inherit_from=(registry, alias, model)`` marks a write whose items take the + model's scope when ``scope`` is omitted. As on the CLI, the model lookup runs + only when a policy is active, and an inherited ``organization`` gets the same + check as a typed one. + """ + if scope is None and inherit_from is not None and engine.active: + registry, alias, model = inherit_from + scope = registry.semantic_layer.child_scope(alias=alias, model_name_or_uuid=model) + if scope == "organization": + engine.check_or_raise(f"{operation} --scope organization") + + # ── Routes (14 declarations, in the order from the plan) ──────────── @@ -246,9 +273,12 @@ def list_models( @router.post("/models", summary="Create a semantic-layer model") def create_model( - body: ModelCreate, registry: ServiceRegistry = Depends(get_registry) + body: ModelCreate, + registry: ServiceRegistry = Depends(get_registry), + engine: PermissionEngine = Depends(get_permission_engine), ) -> dict[str, Any]: """Create a semantic-layer model (POST /repository/semantic-model).""" + _gate_organization_scope(engine, "semantic-layer.model.create", body.scope) return registry.semantic_layer.create_model( alias=body.project, name=body.name, @@ -427,6 +457,7 @@ def add_item( kind: ItemKind, body: dict[str, Any], registry: ServiceRegistry = Depends(get_registry), + engine: PermissionEngine = Depends(get_permission_engine), ) -> dict[str, Any]: """Add an entity to a model. ``kind`` selects the entity type. @@ -438,6 +469,12 @@ def add_item( svc = registry.semantic_layer if kind == "metric": m = AddMetric.model_validate(body) + _gate_organization_scope( + engine, + "semantic-layer.add.metric", + m.scope, + inherit_from=(registry, m.project, m.model), + ) return svc.add_metric( alias=m.project, model_name_or_uuid=m.model, @@ -452,6 +489,12 @@ def add_item( ) if kind == "dataset": d = AddDataset.model_validate(body) + _gate_organization_scope( + engine, + "semantic-layer.add.dataset", + d.scope, + inherit_from=(registry, d.project, d.model), + ) return svc.add_dataset( alias=d.project, model_name_or_uuid=d.model, @@ -467,6 +510,12 @@ def add_item( ) if kind == "relationship": r = AddRelationship.model_validate(body) + _gate_organization_scope( + engine, + "semantic-layer.add.relationship", + r.scope, + inherit_from=(registry, r.project, r.model), + ) return svc.add_relationship( alias=r.project, model_name_or_uuid=r.model, @@ -480,6 +529,12 @@ def add_item( ) if kind == "constraint": c = AddConstraint.model_validate(body) + _gate_organization_scope( + engine, + "semantic-layer.add.constraint", + c.scope, + inherit_from=(registry, c.project, c.model), + ) return svc.add_constraint( alias=c.project, model_name_or_uuid=c.model, @@ -493,6 +548,12 @@ def add_item( ) if kind == "glossary": g = AddGlossary.model_validate(body) + _gate_organization_scope( + engine, + "semantic-layer.add.glossary", + g.scope, + inherit_from=(registry, g.project, g.model), + ) return svc.add_glossary( alias=g.project, model_name_or_uuid=g.model, @@ -610,9 +671,17 @@ def remove_item( @router.post("/import", summary="Import a snapshot into a project") def import_snapshot( - body: ImportRequest, registry: ServiceRegistry = Depends(get_registry) + body: ImportRequest, + registry: ServiceRegistry = Depends(get_registry), + engine: PermissionEngine = Depends(get_permission_engine), ) -> dict[str, Any]: """Replay an inline snapshot into a project. Default: skip on conflict.""" + _gate_organization_scope( + engine, + "semantic-layer.import", + body.scope, + inherit_from=(registry, body.project, body.model), + ) return registry.semantic_layer.import_snapshot_from_dict( body.project, snapshot=body.snapshot, @@ -627,9 +696,17 @@ def import_snapshot( @router.post("/promote", summary="Promote a model between projects") def promote( - body: PromoteRequest, registry: ServiceRegistry = Depends(get_registry) + body: PromoteRequest, + registry: ServiceRegistry = Depends(get_registry), + engine: PermissionEngine = Depends(get_permission_engine), ) -> dict[str, Any]: """Promote a model from one project to another (additive + overwrite).""" + _gate_organization_scope( + engine, + "semantic-layer.promote", + body.scope, + inherit_from=(registry, body.to_project, body.to_model), + ) return registry.semantic_layer.promote_model( from_project=body.from_project, to_project=body.to_project, @@ -643,8 +720,17 @@ def promote( @router.post("/build", summary="Build a model from tables") -def build(body: BuildRequest, registry: ServiceRegistry = Depends(get_registry)) -> dict[str, Any]: +def build( + body: BuildRequest, + registry: ServiceRegistry = Depends(get_registry), + engine: PermissionEngine = Depends(get_permission_engine), +) -> dict[str, Any]: """Heuristic greenfield builder — synthesize a model from a list of tables.""" + if body.model is not None: + # No `scope` field: only building INTO an existing model can land at organization. + _gate_organization_scope( + engine, "semantic-layer.build", None, inherit_from=(registry, body.project, body.model) + ) return registry.semantic_layer.build_model( alias=body.project, table_ids=body.tables, @@ -715,8 +801,9 @@ def delete_reference_data( # ── scope (visibility, target-project grants, org-elevation requests) ── # Mirrors `kbagent semantic-layer scope *`. A write to `organization` scope is -# irreversible; like every router outside /auth, these do not consult the -# permission engine -- the bearer token of `serve` is the gate. +# irreversible: the route table checks the base key, and `scope_set` escalates +# an elevation to the destructive-class `semantic-layer.scope.set --scope +# organization`, as the CLI does. ScopeKind = Literal["model", "dataset", "metric", "relationship", "constraint", "glossary"] @@ -767,8 +854,10 @@ def scope_set( context_id: str, body: ScopeSet, registry: ServiceRegistry = Depends(get_registry), + engine: PermissionEngine = Depends(get_permission_engine), ) -> dict[str, Any]: """Elevate to organization (irreversible), or replace/clear the target projects.""" + _gate_organization_scope(engine, "semantic-layer.scope.set", body.scope) return registry.semantic_layer.scope_set( alias=body.project, kind=body.type, diff --git a/tests/test_serve_telemetry.py b/tests/test_serve_telemetry.py index b4d31702..6a70271a 100644 --- a/tests/test_serve_telemetry.py +++ b/tests/test_serve_telemetry.py @@ -10,11 +10,13 @@ from pathlib import Path import pytest +from fastapi import Depends from fastapi.testclient import TestClient from keboola_agent_cli import telemetry from keboola_agent_cli.server._serve_command_map import SERVE_COMMAND_MAP from keboola_agent_cli.server.app import create_app +from keboola_agent_cli.server.dependencies import require_permission def test_command_map_matches_every_route_exactly(tmp_config_dir: Path) -> None: @@ -98,7 +100,15 @@ def fake_send( def _boom(project: str) -> None: raise RuntimeError("boom") - app.add_api_route("/telemetry-test-boom/{project}", _boom, methods=["POST"], name="boom_route") + # A route without a permission classification is refused with 403 before it + # runs (issue #655), so the test route declares its own guard. + app.add_api_route( + "/telemetry-test-boom/{project}", + _boom, + methods=["POST"], + name="boom_route", + dependencies=[Depends(require_permission("job.run"))], + ) with TestClient(app, raise_server_exceptions=False) as client: response = client.post("/telemetry-test-boom/prod", headers={"Authorization": "Bearer tok"}) diff --git a/tests/test_server_route_permissions.py b/tests/test_server_route_permissions.py new file mode 100644 index 00000000..95652bc7 --- /dev/null +++ b/tests/test_server_route_permissions.py @@ -0,0 +1,828 @@ +"""Tests for the app-wide route firewall on ``kbagent serve`` (issue #655). + +``tests/test_server_permissions.py`` pins the SEAM #677 built -- the engine on +``app.state``, ``require_permission``, and the 403 envelope. This file pins the +COVERAGE #655 asked for: every route on the app is classified, the +classification is checked on every request, and an unclassified route is +refused rather than silently exempted. + +The completeness tests are the load-bearing ones. They are what makes the +runtime fail-closed branch unreachable in a released build: a route added +without a table entry fails here long before anyone meets its 403. +""" + +from __future__ import annotations + +import importlib.util +import re +from pathlib import Path +from typing import Any, ClassVar + +import pytest + +if importlib.util.find_spec("fastapi") is None: # pragma: no cover + pytest.skip( + "FastAPI not installed; run `uv pip install -e '.[server]'`", allow_module_level=True + ) + +from fastapi.testclient import TestClient + +from keboola_agent_cli.config_store import ConfigStore +from keboola_agent_cli.errors import PermissionDeniedError +from keboola_agent_cli.models import AppConfig, PermissionPolicy, ProjectConfig +from keboola_agent_cli.permissions import OPERATION_REGISTRY, SERVE_ONLY_OPERATIONS +from keboola_agent_cli.server import create_app +from keboola_agent_cli.server.dependencies import PERMISSION_DEPENDENCY_MARKER +from keboola_agent_cli.server.route_permissions import ( + LEAF_PATH_PARAMS, + ROUTE_OPERATIONS, + UNGUARDED_PATHS, + is_unguarded, + resolve_route_operation, + unknown_operations, +) + +TOKEN = "route-perm-token" +AUTH = {"Authorization": f"Bearer {TOKEN}"} + + +def _persist_policy(config_dir: Path, policy: PermissionPolicy) -> None: + store = ConfigStore(config_dir=config_dir) + config = store.load() + config.permissions = policy + store.save(config) + + +def _app(tmp_path: Path, **kwargs: Any) -> Any: + return create_app(config_dir=str(tmp_path), auth_token=TOKEN, **kwargs) + + +def _client(tmp_path: Path, **kwargs: Any) -> TestClient: + return TestClient(_app(tmp_path, **kwargs)) + + +def _iter_api_routes(app: Any) -> list[Any]: + """Every real ``APIRoute`` on ``app``, expanding lazy router includes. + + FastAPI 0.137 stopped flattening ``include_router`` eagerly: ``app.routes`` + holds one ``_IncludedRouter`` proxy per include (35 of them here) instead of + the 236 routes they stand for, and nothing materialises them -- not + ``app.openapi()``, not ``TestClient`` startup. Request handling is + unaffected (``request.scope["route"]`` is still the real ``APIRoute``, with + its full path template), but a test that walks ``app.routes`` naively would + audit four routes, find nothing wrong, and pass. A coverage test that can + pass by seeing nothing is worse than no coverage test, hence the recursion. + """ + stack, seen, routes = list(app.routes), set(), [] + while stack: + route = stack.pop() + if id(route) in seen: + continue + seen.add(id(route)) + inner = getattr(route, "original_router", None) + if inner is not None: + stack.extend(inner.routes) + continue + if getattr(route, "methods", None): + routes.append(route) + return routes + + +def _live_routes(app: Any) -> set[tuple[str, str]]: + """Every (method, path template) the app answers, minus HEAD/OPTIONS.""" + return { + (method, route.path) + for route in _iter_api_routes(app) + for method in route.methods + if method not in ("HEAD", "OPTIONS") + } + + +def _declares_inline_guard(app: Any, method: str, path: str) -> bool: + for route in _iter_api_routes(app): + if route.path != path or method not in route.methods: + continue + for dependant in getattr(route, "dependencies", ()) or (): + call = getattr(dependant, "dependency", None) + if getattr(call, PERMISSION_DEPENDENCY_MARKER, None) is not None: + return True + return False + + +def _inline_guard_operations(app: Any) -> dict[tuple[str, str], str]: + """(method, path) -> operation of every route that declares ``require_permission``.""" + guarded: dict[tuple[str, str], str] = {} + for route in _iter_api_routes(app): + for dependant in getattr(route, "dependencies", ()) or (): + operation = getattr( + getattr(dependant, "dependency", None), PERMISSION_DEPENDENCY_MARKER, None + ) + if operation is None: + continue + for method in route.methods - {"HEAD", "OPTIONS"}: + guarded[(method, route.path)] = operation + return guarded + + +def _openapi_operations(app: Any) -> set[tuple[str, str]]: + """Every (method, path) in the app's OpenAPI spec, with ``{name:path}`` put back. + + The spec drops path converters (``{table_id}``), ``APIRoute.path`` keeps them + (``{table_id:path}``). The spec is built by FastAPI itself, not by + :func:`_iter_api_routes`, so it is an independent list of the routes. + """ + converters = { + re.sub(r"\{(\w+):[^}]+\}", r"{\1}", route.path): route.path + for route in _iter_api_routes(app) + } + return { + (method.upper(), converters.get(path, path)) + for path, item in app.openapi()["paths"].items() + for method in item + if method.upper() not in ("HEAD", "OPTIONS") + } + + +def test_unguarded_exemption_covers_reads_only() -> None: + """A write route on a bootstrap path is not exempt; it needs a table entry.""" + for path in UNGUARDED_PATHS: + assert is_unguarded("GET", path) + assert is_unguarded("head", path) + assert not is_unguarded("POST", path) + assert not is_unguarded("PUT", path) + assert not is_unguarded("DELETE", path) + + +class TestTableCoversTheLiveApp: + """The three-way partition: mapped, exempt, or inline-guarded. No fourth case.""" + + def test_every_route_is_classified(self, tmp_path: Path) -> None: + app = _app(tmp_path) + unclassified = sorted( + f"{method} {path}" + for method, path in _live_routes(app) + if (method, path) not in ROUTE_OPERATIONS + and not is_unguarded(method, path) + and not _declares_inline_guard(app, method, path) + ) + assert unclassified == [], ( + "These routes have no permission classification and would be REFUSED at " + "runtime. Add an entry to ROUTE_OPERATIONS in server/route_permissions.py, " + "or list the path in UNGUARDED_PATHS if it is bootstrap surface: " + f"{unclassified}" + ) + + def test_no_stale_entries(self, tmp_path: Path) -> None: + """A table entry for a route that no longer exists is dead weight. + + Worse than dead: it reads as coverage during a security review while + classifying nothing at all. + """ + live = _live_routes(_app(tmp_path)) + stale = sorted( + f"{method} {path}" for method, path in ROUTE_OPERATIONS if (method, path) not in live + ) + assert stale == [], f"ROUTE_OPERATIONS entries matching no live route: {stale}" + + def test_every_operation_is_a_registry_key(self) -> None: + """A typo'd operation silently defaults to 'write' and matches no exact pattern.""" + assert unknown_operations() == [] + + def test_route_walk_sees_every_openapi_operation(self, tmp_path: Path) -> None: + """The completeness checks above audit only the routes the walk finds. + + A walk that misses a router (a new FastAPI version, a new kind of + include) would let an unmapped route pass them. The OpenAPI spec is an + independent list of the same routes, so it must be a subset of the walk. + """ + app = _app(tmp_path) + missed = sorted( + f"{method} {path}" for method, path in _openapi_operations(app) - _live_routes(app) + ) + assert missed == [], f"_iter_api_routes does not see these routes: {missed}" + + def test_inline_guards_name_registry_operations(self, tmp_path: Path) -> None: + """An inline ``require_permission`` with a typo is as silent as a table typo.""" + unknown = sorted( + f"{method} {path} -> {operation}" + for (method, path), operation in _inline_guard_operations(_app(tmp_path)).items() + if operation not in OPERATION_REGISTRY + ) + assert unknown == [], f"Inline guards naming no OPERATION_REGISTRY key: {unknown}" + + def test_auth_routes_are_covered_by_their_inline_guards(self, tmp_path: Path) -> None: + """#677's three routes stay enforced without a table entry.""" + app = _app(tmp_path) + for method, path in [ + ("GET", "/auth/projects"), + ("GET", "/auth/status"), + ("POST", "/auth/register-projects"), + ]: + assert (method, path) not in ROUTE_OPERATIONS + assert _declares_inline_guard(app, method, path), f"{method} {path} lost its guard" + + +class TestResolveRouteOperation: + def test_exempt_path_resolves_to_none(self) -> None: + assert resolve_route_operation("GET", "/health/ping") is None + + def test_mapped_route_resolves(self) -> None: + assert resolve_route_operation( + "DELETE", "/configs/{project}/{component_id}/{config_id}" + ) == ("config.delete") + + def test_method_is_part_of_the_key(self) -> None: + """Same path, different verb, different risk -- the classic mapping bug.""" + path = "/configs/{project}/{component_id}/{config_id}" + assert resolve_route_operation("GET", path) == "config.detail" + assert resolve_route_operation("DELETE", path) == "config.delete" + + def test_lowercase_method_is_accepted(self) -> None: + assert resolve_route_operation("get", "/projects") == "project.list" + + def test_unmapped_route_resolves_to_none(self) -> None: + assert resolve_route_operation("GET", "/nope/not/a/route") is None + + +class TestEnforcementOnRealRoutes: + """The behaviour #655 reported missing, on routes it named.""" + + def test_destructive_route_is_denied(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["cli:destructive"])) + response = _client(tmp_path).request( + "DELETE", "/storage/buckets/demo", headers=AUTH, json={} + ) + assert response.status_code == 403 + body = response.json() + assert body["error"]["code"] == "PERMISSION_DENIED" + assert "storage.delete-bucket" in body["error"]["message"] + + def test_read_route_still_allowed_under_a_destructive_deny(self, tmp_path: Path) -> None: + """The firewall must stay surgical -- denying deletes cannot break listing.""" + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["cli:destructive"])) + response = _client(tmp_path).get("/projects", headers=AUTH) + assert response.status_code == 200 + + def test_deny_writes_flag_blocks_a_write_route(self, tmp_path: Path) -> None: + client = _client(tmp_path, deny_writes=True) + response = client.post("/jobs/demo/run", headers=AUTH, json={}) + assert response.status_code == 403 + assert response.json()["error"]["code"] == "PERMISSION_DENIED" + + def test_deny_writes_flag_leaves_reads_alone(self, tmp_path: Path) -> None: + client = _client(tmp_path, deny_writes=True) + assert client.get("/version", headers=AUTH).status_code == 200 + + def test_token_delete_is_denied_by_an_exact_pattern(self, tmp_path: Path) -> None: + """Exact-name patterns work over REST, not just category ones.""" + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["token.delete"])) + client = _client(tmp_path) + assert client.post("/token/demo/delete", headers=AUTH, json={}).status_code == 403 + + def test_default_install_denies_nothing(self, tmp_path: Path) -> None: + """No policy configured -> the surface behaves exactly as before #655.""" + client = _client(tmp_path) + assert client.get("/projects", headers=AUTH).status_code == 200 + assert client.get("/version", headers=AUTH).status_code == 200 + + def test_mode_deny_still_serves_bootstrap_paths(self, tmp_path: Path) -> None: + """A locked-down server must still be able to say who it is. + + Denying `/health/ping` would leave a client unable to distinguish a + policy refusal from a dead process. + """ + _persist_policy(tmp_path, PermissionPolicy(mode="deny", allow=[])) + client = _client(tmp_path) + assert client.get("/health/ping").status_code == 200 + assert client.get("/health/auth-info", headers=AUTH).status_code == 200 + + def test_mode_deny_blocks_an_unlisted_read(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="deny", allow=["project.list"])) + client = _client(tmp_path) + assert client.get("/projects", headers=AUTH).status_code == 200 + assert client.get("/storage/buckets", headers=AUTH).status_code == 403 + + +class TestUnmappedRouteFailsClosed: + def test_route_added_without_a_table_entry_is_refused(self, tmp_path: Path) -> None: + app = _app(tmp_path) + + @app.get("/_unclassified") + def _unclassified() -> dict[str, str]: # pragma: no cover - never reached + return {"status": "ok"} + + response = TestClient(app).get("/_unclassified", headers=AUTH) + assert response.status_code == 403 + assert response.json()["error"]["code"] == "PERMISSION_DENIED" + assert "ROUTE_OPERATIONS" in response.json()["error"]["message"] + + def test_inline_guarded_route_without_a_table_entry_is_allowed(self, tmp_path: Path) -> None: + """The per-route override still works -- that is what keeps /auth/* alive.""" + from fastapi import Depends + + from keboola_agent_cli.server.dependencies import require_permission + + app = _app(tmp_path) + + @app.get("/_inline", dependencies=[Depends(require_permission("config.list"))]) + def _inline() -> dict[str, str]: + return {"status": "ok"} + + client = TestClient(app) + assert client.get("/_inline", headers=AUTH).status_code == 200 + + def test_inline_guarded_route_is_still_subject_to_its_own_policy(self, tmp_path: Path) -> None: + from fastapi import Depends + + from keboola_agent_cli.server.dependencies import require_permission + + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["config.list"])) + app = _app(tmp_path) + + @app.get("/_inline", dependencies=[Depends(require_permission("config.list"))]) + def _inline() -> dict[str, str]: # pragma: no cover - never reached + return {"status": "ok"} + + assert TestClient(app).get("/_inline", headers=AUTH).status_code == 403 + + +class TestRoutesAddedAfterTheTable: + """Routes added to `main` after the table was first written are enforced too.""" + + @pytest.mark.parametrize( + ("method", "path", "operation"), + [ + ("GET", "/flows/demo/1/triggers", "flow.triggers"), + ("POST", "/notifications/demo", "notification.create"), + ("DELETE", "/notifications/demo/1", "notification.delete"), + ("POST", "/notifications/demo/1/replace-recipient", "notification.replace-recipient"), + ], + ) + def test_exact_operation_deny_blocks_the_route( + self, tmp_path: Path, method: str, path: str, operation: str + ) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=[operation])) + response = _client(tmp_path).request(method, path, headers=AUTH, json={}) + assert response.status_code == 403 + assert response.json()["error"]["message"] == PermissionDeniedError(operation).message + + +# Minimal valid body per semantic-layer write that can land at organization +# scope: (method, path, body without `scope`, escalated operation, inherits). +_SL = "/semantic-layer" +_ORG_SCOPE_WRITES = [ + ("POST", f"{_SL}/models", {"project": "demo", "name": "m"}, "model.create", False), + ( + "POST", + f"{_SL}/items/metric", + {"project": "demo", "name": "n", "sql": "1", "dataset": "d"}, + "add.metric", + True, + ), + ( + "POST", + f"{_SL}/items/dataset", + {"project": "demo", "name": "n", "table_id": "t"}, + "add.dataset", + True, + ), + ( + "POST", + f"{_SL}/items/relationship", + {"project": "demo", "name": "n", "from": "a", "to": "b", "on": "x"}, + "add.relationship", + True, + ), + ( + "POST", + f"{_SL}/items/constraint", + {"project": "demo", "name": "n", "constraint_type": "c", "rule": "r", "metrics": ["m"]}, + "add.constraint", + True, + ), + ( + "POST", + f"{_SL}/items/glossary", + {"project": "demo", "term": "t", "definition": "d"}, + "add.glossary", + True, + ), + ("POST", f"{_SL}/import", {"project": "demo", "snapshot": {}}, "import", True), + ("POST", f"{_SL}/promote", {"from_project": "a", "to_project": "demo"}, "promote", True), +] +_ORG_SCOPE_IDS = [f"{method} {path}" for method, path, *_ in _ORG_SCOPE_WRITES] + + +class TestOrganizationScopeEscalation: + """`--scope organization` is destructive-class over REST too (FLAG_ESCALATIONS). + + The route table checks only the base `write` key, so without the in-handler + check a REST caller under `--deny-destructive` could still make an + irreversible organization elevation the CLI refuses. + """ + + @staticmethod + def _client( + tmp_path: Path, inherited: str = "project", **kwargs: Any + ) -> tuple[TestClient, Any]: + from unittest.mock import MagicMock + + from keboola_agent_cli.server.dependencies import ServiceRegistry, get_registry + + sl = MagicMock() + sl.child_scope.return_value = inherited + for method in ( + "create_model", + "add_metric", + "add_dataset", + "add_relationship", + "add_constraint", + "add_glossary", + "import_snapshot_from_dict", + "promote_model", + "build_model", + "scope_set", + ): + # A bare MagicMock return value is not JSON-serializable. + getattr(sl, method).return_value = {} + registry = ServiceRegistry.__new__(ServiceRegistry) + registry.semantic_layer = sl + app = _app(tmp_path, **kwargs) + app.dependency_overrides[get_registry] = lambda: registry + return TestClient(app), sl + + @staticmethod + def _assert_denied(response: Any, operation: str) -> None: + assert response.status_code == 403, response.text + assert response.json()["error"]["code"] == "PERMISSION_DENIED" + assert f"{operation} --scope organization" in response.json()["error"]["message"] + + def test_scope_set_organization_is_denied(self, tmp_path: Path) -> None: + client, sl = self._client(tmp_path, deny_destructive=True) + item = {"project": "demo", "type": "dataset"} + response = client.put( + f"{_SL}/scope/d1", headers=AUTH, json={**item, "scope": "organization"} + ) + self._assert_denied(response, "semantic-layer.scope.set") + sl.scope_set.assert_not_called() + targets = {**item, "target_projects": ["a"]} + assert client.put(f"{_SL}/scope/d1", headers=AUTH, json=targets).status_code == 200 + + @pytest.mark.parametrize( + ("method", "path", "body", "op", "inherits"), _ORG_SCOPE_WRITES, ids=_ORG_SCOPE_IDS + ) + def test_typed_organization_scope_is_denied( + self, tmp_path: Path, method: str, path: str, body: dict[str, Any], op: str, inherits: bool + ) -> None: + client, _ = self._client(tmp_path, deny_destructive=True) + org = {**body, "scope": "organization"} + self._assert_denied( + client.request(method, path, headers=AUTH, json=org), f"semantic-layer.{op}" + ) + assert ( + client.request( + method, path, headers=AUTH, json={**body, "scope": "project"} + ).status_code + == 200 + ) + + @pytest.mark.parametrize( + ("method", "path", "body", "op", "inherits"), _ORG_SCOPE_WRITES, ids=_ORG_SCOPE_IDS + ) + def test_inherited_organization_scope_is_denied( + self, tmp_path: Path, method: str, path: str, body: dict[str, Any], op: str, inherits: bool + ) -> None: + client, sl = self._client(tmp_path, inherited="organization", deny_destructive=True) + response = client.request(method, path, headers=AUTH, json=body) + if inherits: + self._assert_denied(response, f"semantic-layer.{op}") + # The scope is looked up on the project the items land in (`to_project` for promote). + sl.child_scope.assert_called_once_with(alias="demo", model_name_or_uuid=None) + else: # a new model has no model to inherit from: it is created project-scoped + assert response.status_code == 200 + sl.child_scope.return_value = "project" + assert client.request(method, path, headers=AUTH, json=body).status_code == 200 + + def test_build_into_an_organization_model_is_denied(self, tmp_path: Path) -> None: + body = {"project": "demo", "tables": ["in.c-a.t"]} + client, sl = self._client(tmp_path, inherited="organization", deny_destructive=True) + self._assert_denied( + client.post(f"{_SL}/build", headers=AUTH, json={**body, "model": "m"}), + "semantic-layer.build", + ) + assert client.post(f"{_SL}/build", headers=AUTH, json=body).status_code == 200 + sl.child_scope.assert_called_once() # only the `model` call looked the scope up + + def test_leaf_escalation_pattern_is_per_kind(self, tmp_path: Path) -> None: + policy = PermissionPolicy( + mode="allow", deny=["semantic-layer.add.metric --scope organization"] + ) + _persist_policy(tmp_path, policy) + client, _ = self._client(tmp_path) + metric = { + "project": "demo", + "name": "n", + "sql": "1", + "dataset": "d", + "scope": "organization", + } + glossary = {"project": "demo", "term": "t", "definition": "d", "scope": "organization"} + self._assert_denied( + client.post(f"{_SL}/items/metric", headers=AUTH, json=metric), + "semantic-layer.add.metric", + ) + assert client.post(f"{_SL}/items/glossary", headers=AUTH, json=glossary).status_code == 200 + + def test_no_policy_skips_the_model_lookup(self, tmp_path: Path) -> None: + client, sl = self._client(tmp_path, inherited="organization") + body = {"project": "demo", "term": "t", "definition": "d"} + assert client.post(f"{_SL}/items/glossary", headers=AUTH, json=body).status_code == 200 + sl.child_scope.assert_not_called() + + +_METRIC = {"project": "demo", "name": "n", "sql": "1", "dataset": "d"} +_GLOSSARY = {"project": "demo", "term": "t", "definition": "d"} + + +class TestLeafKeysFromPathParam: + """`/semantic-layer/items/{kind}` checks the per-kind leaf key, like the CLI. + + The CLI checks `semantic-layer.add` and `semantic-layer.add.`. A leaf + pattern does not match the parent key (fnmatch), so without the second + check a policy that works on the CLI would not apply over REST. + """ + + @staticmethod + def _client(tmp_path: Path, policy: PermissionPolicy) -> tuple[TestClient, Any]: + _persist_policy(tmp_path, policy) + client, sl = TestOrganizationScopeEscalation._client(tmp_path) + for method in ("edit_metric", "edit_glossary", "remove_item"): + getattr(sl, method).return_value = {} + return client, sl + + @staticmethod + def _assert_denied(response: Any, operation: str) -> None: + assert response.status_code == 403, response.text + assert response.json()["error"]["code"] == "PERMISSION_DENIED" + assert operation in response.json()["error"]["message"] + + def test_exact_leaf_deny_blocks_only_that_kind(self, tmp_path: Path) -> None: + policy = PermissionPolicy(mode="allow", deny=["semantic-layer.add.metric"]) + client, sl = self._client(tmp_path, policy) + self._assert_denied( + client.post(f"{_SL}/items/metric", headers=AUTH, json=_METRIC), + "semantic-layer.add.metric", + ) + sl.add_metric.assert_not_called() + assert client.post(f"{_SL}/items/glossary", headers=AUTH, json=_GLOSSARY).status_code == 200 + + def test_leaf_glob_and_exact_leaves_block_add_edit_and_remove(self, tmp_path: Path) -> None: + policy = PermissionPolicy( + mode="allow", + deny=[ + "semantic-layer.add.*", + "semantic-layer.edit.metric", + "semantic-layer.remove.metric", + ], + ) + client, sl = self._client(tmp_path, policy) + edit = {"project": "demo", "new_description": "x"} + self._assert_denied( + client.post(f"{_SL}/items/glossary", headers=AUTH, json=_GLOSSARY), + "semantic-layer.add.glossary", + ) + self._assert_denied( + client.put(f"{_SL}/items/metric/x", headers=AUTH, json=edit), + "semantic-layer.edit.metric", + ) + self._assert_denied( + client.delete(f"{_SL}/items/metric/x", headers=AUTH, params={"project": "demo"}), + "semantic-layer.remove.metric", + ) + sl.add_glossary.assert_not_called() + sl.edit_metric.assert_not_called() + sl.remove_item.assert_not_called() + # Other kinds of edit and remove stay allowed. + assert client.put(f"{_SL}/items/glossary/t", headers=AUTH, json=edit).status_code == 200 + response = client.delete( + f"{_SL}/items/glossary/t", headers=AUTH, params={"project": "demo"} + ) + assert response.status_code == 200 + + def test_deny_mode_needs_both_parent_and_leaf(self, tmp_path: Path) -> None: + """As on the CLI: allowing the parent key alone does not allow a kind.""" + parent_only = PermissionPolicy(mode="deny", allow=["semantic-layer.add"]) + client, _ = self._client(tmp_path, parent_only) + self._assert_denied( + client.post(f"{_SL}/items/metric", headers=AUTH, json=_METRIC), + "semantic-layer.add.metric", + ) + both = PermissionPolicy( + mode="deny", allow=["semantic-layer.add", "semantic-layer.add.metric"] + ) + client, _ = self._client(tmp_path, both) + assert client.post(f"{_SL}/items/metric", headers=AUTH, json=_METRIC).status_code == 200 + + def test_every_composed_leaf_is_a_registry_key(self) -> None: + """A leaf key missing from the registry would never match an exact-name pattern.""" + from typing import get_args + + from keboola_agent_cli.server.routers.semantic_layer import ItemKind + + for route, param in LEAF_PATH_PARAMS.items(): + assert param == "kind" + parent = ROUTE_OPERATIONS[route] + for kind in get_args(ItemKind): + assert f"{parent}.{kind}" in OPERATION_REGISTRY, (route, kind) + + +class TestDenialEnvelope: + """A firewall denial looks like every other `kbagent serve` error.""" + + @staticmethod + def _shape(body: dict[str, Any]) -> tuple[list[str], str, list[str]]: + return sorted(body), body["status"], sorted(body["error"]) + + def test_table_inline_and_unmapped_denials_share_one_envelope(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["cli:destructive"])) + app = _app(tmp_path) + + @app.get("/_unclassified") + def _unclassified() -> dict[str, str]: # pragma: no cover - never reached + return {"status": "ok"} + + client = TestClient(app) + table = client.request("DELETE", "/storage/buckets/demo", headers=AUTH, json={}) + inline = client.post("/merge-requests/demo/1/merge", headers=AUTH) + unmapped = client.get("/_unclassified", headers=AUTH) + not_found = client.get("/no-such-route", headers=AUTH) + + for response in (table, inline, unmapped): + assert response.status_code == 403 + assert response.json()["error"]["code"] == "PERMISSION_DENIED" + # Same keys as any other error the server returns, here a plain 404. + assert not_found.status_code == 404 + expected = self._shape(not_found.json()) + assert expected == (["error", "status"], "error", ["code", "message"]) + for response in (table, inline, unmapped): + assert self._shape(response.json()) == expected + + def test_table_denial_carries_the_cli_message(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["cli:destructive"])) + response = _client(tmp_path).request( + "DELETE", "/storage/buckets/demo", headers=AUTH, json={} + ) + expected = PermissionDeniedError("storage.delete-bucket").message + assert response.json()["error"]["message"] == expected + + def test_unmapped_denial_message_is_not_wrapped_as_an_operation_name( + self, tmp_path: Path + ) -> None: + app = _app(tmp_path) + + @app.get("/_unclassified") + def _unclassified() -> dict[str, str]: # pragma: no cover - never reached + return {"status": "ok"} + + message = TestClient(app).get("/_unclassified", headers=AUTH).json()["error"]["message"] + assert message.startswith("GET /_unclassified has no permission classification") + assert "is blocked by the active permission policy" not in message + + def test_denial_comes_before_the_project_id_translation(self, tmp_path: Path) -> None: + """A denied route answers 403 even when its `{project}` would fail to resolve. + + Project ID 77 is registered under two aliases on two stacks, so + `translate_project_refs` refuses it with 400 CONFIG_ERROR. The firewall + runs first, so the caller learns about the policy, and the translation + does no work for a request that is refused anyway. + """ + token = "901-55555-fakeTestTokenDoNotUseXXXXXXXX" + projects = { + "a": ProjectConfig( + stack_url="https://connection.keboola.com", token=token, project_id=77 + ), + "b": ProjectConfig( + stack_url="https://connection.eu-central-1.keboola.com", token=token, project_id=77 + ), + } + ConfigStore(config_dir=tmp_path).save( + AppConfig(projects=projects, permissions=PermissionPolicy(deny=["cli:destructive"])) + ) + client = _client(tmp_path) + denied = client.request("DELETE", "/storage/buckets/77", headers=AUTH, json={}) + assert denied.status_code == 403 + assert denied.json()["error"]["code"] == "PERMISSION_DENIED" + # Control: the same ID on an allowed route does reach the translation. + allowed = client.get("/storage/buckets?project=77", headers=AUTH) + assert allowed.status_code == 400 + assert allowed.json()["error"]["code"] == "CONFIG_ERROR" + + +class TestPermissionsShowEndpoint: + def test_reports_no_policy_when_clean(self, tmp_path: Path) -> None: + body = _client(tmp_path).get("/permissions/show", headers=AUTH).json() + assert body == {"active": False, "policy": None} + + def test_reports_the_persisted_policy(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="deny", allow=["cli:read"], deny=[])) + body = _client(tmp_path).get("/permissions/show", headers=AUTH).json() + assert body["active"] is True + assert body["policy"] == {"mode": "deny", "allow": ["cli:read"], "deny": []} + + def test_reports_the_effective_policy_including_session_flags(self, tmp_path: Path) -> None: + """The merged view is the point: a REST caller cannot see the daemon's flags.""" + body = ( + _client(tmp_path, deny_destructive=True).get("/permissions/show", headers=AUTH).json() + ) + assert body["active"] is True + assert body["policy"]["deny"] == ["cli:destructive"] + + def test_surfaces_inert_patterns(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["tool:write"])) + body = _client(tmp_path).get("/permissions/show", headers=AUTH).json() + assert body["inert_patterns"] == ["tool:write"] + assert body["inert_since_version"] == "0.85.0" + + def test_is_reachable_under_a_total_deny(self, tmp_path: Path) -> None: + """Discovery must survive the policy it describes, or it is useless.""" + _persist_policy(tmp_path, PermissionPolicy(mode="deny", allow=[])) + response = _client(tmp_path).get("/permissions/show", headers=AUTH) + assert response.status_code == 200 + assert response.json()["active"] is True + + def test_still_requires_the_bearer_token(self, tmp_path: Path) -> None: + assert _client(tmp_path).get("/permissions/show").status_code == 401 + + +class TestServeOnlyOperationsAreRegistered: + """New serve-only keys must be real registry entries AND declared serve-only.""" + + @pytest.mark.parametrize("operation", ["ai.chat", "workspace.sql-improve"]) + def test_operation_is_registered_and_declared_serve_only(self, operation: str) -> None: + assert operation in OPERATION_REGISTRY + assert operation in SERVE_ONLY_OPERATIONS + + +class TestVerbAndRiskClassAgree: + """Cheap structural checks that catch the mapping mistakes worth catching. + + Neither is a law of nature -- HTTP verbs and risk classes are different + vocabularies -- so both carry an explicit allowlist. The point is that a + disagreement has to be *chosen*: a new DELETE quietly classified ``read`` + would be a firewall hole nothing else notices. + """ + + # POST because the request needs a request BODY, not because it mutates. + _NON_MUTATING_POSTS: ClassVar[set[str]] = { + "POST /data-apps/validate-repo", + "POST /documentation/query", + "POST /flows/validate", + "POST /kai/ask", + "POST /lineage/build", + "POST /lineage/show", + "POST /semantic-layer/diff", + "POST /workspaces/{project}/{workspace_id}/password", + } + # `branch merge` only ever produces a URL (the merge happens in the web + # UI), so mirroring its `write` class on a GET is CLI parity, not a slip. + _NON_READ_GETS: ClassVar[set[str]] = {"GET /branches/{project}/merge-url"} + # DELETE of a grant or a pending request, not of data: it narrows an item's + # visibility or withdraws an elevation request, and the CLI classifies both + # `write`. Mirroring that class is CLI parity. + _NON_DESTRUCTIVE_DELETES: ClassVar[set[str]] = { + "DELETE /semantic-layer/scope/{context_id}/target-projects", + "DELETE /semantic-layer/scope/{context_id}/elevation-request", + } + + def test_every_delete_route_is_destructive_or_admin(self) -> None: + offenders = sorted( + f"{method} {path}" + for (method, path), op in ROUTE_OPERATIONS.items() + if method == "DELETE" and OPERATION_REGISTRY[op] not in ("destructive", "admin") + ) + assert set(offenders) <= self._NON_DESTRUCTIVE_DELETES, ( + "A DELETE route classified below `destructive` slips through " + f"--deny-destructive: {sorted(set(offenders) - self._NON_DESTRUCTIVE_DELETES)}" + ) + + def test_mutating_verbs_are_not_classified_read(self) -> None: + offenders = sorted( + f"{method} {path}" + for (method, path), op in ROUTE_OPERATIONS.items() + if method in ("POST", "PUT", "PATCH") and OPERATION_REGISTRY[op] == "read" + ) + assert set(offenders) <= self._NON_MUTATING_POSTS, ( + "New mutating route classified `read` -- it would survive " + f"--deny-writes: {sorted(set(offenders) - self._NON_MUTATING_POSTS)}" + ) + + def test_get_routes_are_reads(self) -> None: + offenders = sorted( + f"{method} {path}" + for (method, path), op in ROUTE_OPERATIONS.items() + if method == "GET" and OPERATION_REGISTRY[op] != "read" + ) + assert set(offenders) <= self._NON_READ_GETS, ( + "A GET classified above `read` is blocked by --deny-writes; make sure " + f"that is intended: {sorted(set(offenders) - self._NON_READ_GETS)}" + ) diff --git a/tests/test_server_smoke.py b/tests/test_server_smoke.py index 16452ba4..ef40c69d 100644 --- a/tests/test_server_smoke.py +++ b/tests/test_server_smoke.py @@ -205,13 +205,14 @@ def test_semantic_layer_add_dataset_forwards_fqn(fqn: str | None) -> None: """POST /items/dataset forwards the optional ``fqn`` override to the service.""" from unittest.mock import MagicMock + from keboola_agent_cli.permissions import PermissionEngine from keboola_agent_cli.server.routers.semantic_layer import add_item registry = MagicMock() body = {"project": "prod", "name": "fact_x", "table_id": "out.c-gold.FACT_X"} if fqn is not None: body["fqn"] = fqn - add_item("dataset", body, registry=registry) + add_item("dataset", body, registry=registry, engine=PermissionEngine(None)) assert registry.semantic_layer.add_dataset.call_args.kwargs["fqn"] == fqn