diff --git a/CLAUDE.md b/CLAUDE.md index 97d94b41..5a07fbc5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -392,6 +392,25 @@ kbagent auth register-projects [--stack URL|alias] [--all] [--project-id ID ...] # applies retroactively to `auth login --register-projects` (now suffixes on an alias collision # instead of silently skipping the second project). See docs/programmatic-auth-login-plan.md # section 4.5 for the full design. +# `auth` over `kbagent serve` (since vNEXT): `register-projects` / `status` / the project-candidate +# listing get a `server/routers/auth.py` counterpart -- `POST /auth/register-projects`, +# `GET /auth/status`, `GET /auth/projects` (the interactive picker's data source; no CLI leaf +# command of its own). `login` / `login-password` / `logout` deliberately have NO endpoint -- a +# browser login only completes on the host, a password grant must never sit behind the serve +# 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. +# The policy comes from the config dir `serve` RESOLVES (its own `--config-dir`, then +# KBAGENT_CONFIG_DIR, then the local/global chain) -- not from the root callback's `--config-dir` +# -- plus the session flags of the invocation. Reachable ways to enforce: a persisted narrow +# policy (`kbagent permissions set --mode allow --deny auth.register-projects`, needs a real +# terminal for the confirmation code), or `--mode deny` with `serve` (and the reads you want) in +# 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. +# See docs/web-server.md. kbagent project add --project NAME --url URL --token TOKEN kbagent project list diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 06940919..59297861 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -824,9 +824,13 @@ live Typer command tree as the single source of truth and fails if any command is missing from `permissions.py` `OPERATION_REGISTRY`, `CLAUDE.md` `## All CLI Commands`, `commands/context.py` `AGENT_CONTEXT`, or `commands-reference.md`. It also flags dead `OPERATION_REGISTRY` keys (renamed / -removed commands). This is the deterministic half of the "Plugin synchronization -map" -- the judgement half (is a behaviour change worth a new gotcha? is the -`(since vX.Y.Z)` tag right?) is left to `/kbagent:review`. +removed commands). A registry key that intentionally has no CLI leaf command -- +e.g. a `kbagent serve`-only REST operation like `auth.projects` -- must be +added to `SERVE_ONLY_OPERATIONS` in `permissions.py`, which the script +subtracts before that dead-key check; otherwise it fails CI as if the command +had been renamed or removed. This is the deterministic half of the "Plugin +synchronization map" -- the judgement half (is a behaviour change worth a new +gotcha? is the `(since vX.Y.Z)` tag right?) is left to `/kbagent:review`. ### `.github/workflows/e2e.yml` -- nightly + on-demand (NOT per-PR) diff --git a/docs/web-server-endpoints.md b/docs/web-server-endpoints.md index d7e7cd5d..cceeb3ec 100644 --- a/docs/web-server-endpoints.md +++ b/docs/web-server-endpoints.md @@ -9,13 +9,23 @@ 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`. -**228 operations** across **199 paths** and **29 routers**. +**231 operations** across **202 paths** and **30 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`. ## Project Management +### `auth` (3 operations) + +Read/audit the current browser-login session and register its accessible projects as local aliases. `login` / `login-password` / `logout` have no endpoint here -- see `server/routers/auth.py`. Mirrors `kbagent auth status|register-projects` (partially). + +| Method | Path | Summary | +|---|---|---| +| `GET` | `/auth/projects` | List the session's registerable project candidates | +| `POST` | `/auth/register-projects` | Register accessible projects as local aliases | +| `GET` | `/auth/status` | Session health for a stack | + ### `projects` (11 operations) Register, list, edit, and remove Keboola project aliases. Mirrors `kbagent project add|list|remove|edit|status|use|current|info`. diff --git a/docs/web-server.md b/docs/web-server.md index 173e25c9..5f62dab3 100644 --- a/docs/web-server.md +++ b/docs/web-server.md @@ -86,7 +86,7 @@ The routers group into the categories declared in | Category | Routers | |---|---| -| Project Management | `projects` `members` `org` `feature` `billing` `token` | +| Project Management | `auth` `projects` `members` `org` `feature` `billing` `token` | | Configurations | `configs` `components` `transformations` `encrypt` | | Data | `storage` `stream` `search` `sharing` | | Execution | `jobs` `flows` `schedules` `notifications` `data-apps` `workspaces` | @@ -98,13 +98,16 @@ The routers group into the categories declared in `ai-chat` is the one router with no CLI counterpart — it exists to stream 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. +scheduled task fires only while the server runs. `auth` mirrors only the +read/audit half of `kbagent auth` *(since vNEXT)* — `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. -Going the other way, several CLI surfaces are deliberately CLI-only: -`auth` (see "The `auth` command group has no REST router" below), `sync` +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` -does not constrain serve at all is #655. +constrains only `/auth/*` and not the other ~30 routers is #655. Auto-generated OpenAPI spec at `/openapi.json`, Swagger UI at `/docs`. @@ -462,19 +465,112 @@ a session-backed project from the web UI at all: For a project you would rather not expose this way, register it with a static Storage token (`kbagent project add --token`) — that path has neither property. -### The `auth` command group has no REST router — including `login-password` - -`kbagent auth login` / `login-password` / `status` / `logout` / -`register-projects` have no `server/routers/auth.py` counterpart; this is a -whole-group skip (CONTRIBUTING.md's 1:1 CLI/REST convention), not a per-command -gap. It is a deliberate omission for `login-password` specifically: exposing a -password grant over `serve` would let whoever holds `KBAGENT_SERVE_TOKEN` -submit arbitrary account credentials through this process, which is a strictly -worse blast radius than the existing "serve token borrows a session identity" -tradeoff above — that one requires a session to already exist; this one would -let a caller mint one. Sign in via the CLI directly (`kbagent auth -login-password`, or `auth login` for a human), then register the resulting -session's projects for `serve` to use. +### `/auth/*` — three read/audit endpoints, three deliberate gaps *(since vNEXT)* + +`kbagent auth` now has a `server/routers/auth.py` counterpart, but it mirrors +only the read/audit half of the CLI group: + +| Endpoint | CLI equivalent | Permission op | +|---|---|---| +| `GET /auth/projects?stack=` | the interactive picker inside `auth register-projects` (no CLI leaf command of its own) | `auth.projects` (read) | +| `POST /auth/register-projects` | `auth register-projects --all` / `--project-id ID ...` | `auth.register-projects` (write) | +| `GET /auth/status?stack=` | `auth status` | `auth.status` (read) | + +`POST /auth/register-projects` takes a body of `{stack?, all?, project_ids?, +aliases?}` (`all` is the wire alias for the service's `select_all`; `aliases` +maps a numeric project id to an alias override, coerced from the JSON body's +string keys) and returns the same `registered` / `exists` / `skipped` +per-project statuses the CLI prints — an existing alias is never overwritten. +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. + +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: + +- **`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. +- **Use a persisted policy instead.** Run, on the host, in a real terminal + (`permissions set` requires a typed confirmation code — there is no `--yes`): + + ```bash + kbagent --config-dir /path/to/cfg permissions set \ + --mode allow --deny auth.register-projects + kbagent serve --config-dir /path/to/cfg --port 8001 + ``` + + `POST /auth/register-projects` then answers 403 `PERMISSION_DENIED` while + `GET /auth/projects` and `GET /auth/status` stay reachable. A `--mode deny` + policy works too, but its allow list must then include `serve` (and the reads + you want to keep), or the server will not start for the same reason as above. + + Pass `--config-dir` **to `serve`**: the server resolves its own config dir + (`--config-dir` on the `serve` command, then `KBAGENT_CONFIG_DIR`, then the + local/global chain), so a root-level `kbagent --config-dir ... serve` sets the + directory for the CLI invocation, not for the served process. + +A missing or expired session reaches `GET /auth/projects` and `POST +/auth/register-projects` as a **thrown error**, both funnelled through +`AuthService._introspect_accessible_projects`: no stored session raises +`SESSION_NOT_FOUND`, a stored session whose refresh fails raises +`SESSION_EXPIRED` (via `provider.introspect()`) — both answer **HTTP 401**, +same as every other session-project failure documented above. `GET +/auth/status` is the deliberate exception: it is the probe you call *to find +out* whether a session is dead, so it must not itself fail that way. +`AuthService.status()` catches both cases and always answers **HTTP 200**, +reporting session health in the response body's `status` field instead — +`"missing"` (no stored session), `"expired"` (refresh failed), +`"degraded"` (the auth service was unreachable; locally stored data is shown), +`"refreshed"` (introspection rotated the access token), or `"live"`. + +Scope that exactly: **for a missing or expired session `/auth/status` answers +200 and reports health in `status`; an unresolvable stack (4xx) or an +unexpected auth-service failure (502) still surface as errors.** The stack must +resolve before any session is looked at — with no `?stack=` and no default +project to fall back on, `AuthService.status()` raises `ConfigError` and the +route answers 4xx — and any `KeboolaApiError` that is neither +`SESSION_EXPIRED` nor a network code is re-raised rather than swallowed, so it +reaches the central handler (502, or 401 for a session-credential code such as +`SESSION_NOT_FOUND`). So a client detecting a dead session by HTTP status alone +must call `/auth/projects` or `/auth/register-projects`; on `/auth/status` a +200 is the normal answer for a dead session and the caller must read `status` +from the body. + +Registering a project through `POST /auth/register-projects` writes the same +`kbc-session://` sentinel `auth login --register-projects` would — +so whoever holds `KBAGENT_SERVE_TOKEN` can grow the set of session-backed +projects this server exposes, still acting as the signed-in user for all of +them, per "Session-registered projects" above. + +`login` / `login-password` / `logout` deliberately have **no** endpoint: + +- `auth login` opens a browser (or prints a device-flow code) on the host and + only completes there — a REST caller has no way to sit in that loop. +- `auth login-password` takes a plaintext password (and, for MFA accounts, a + TOTP seed) meant to flow from a CI secrets store into one `kbagent` CLI + invocation, never as a REST request body sitting behind this server's own + bearer token. +- `auth logout` revokes the live session backing every session-registered + project reachable through this very server. Destroying that session is a + deliberate host-operator action taken at the CLI, not something a REST + client holding the serve bearer token should be able to trigger remotely. + +Sign in via the CLI directly (`kbagent auth login-password`, or `auth login` +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. ### Manage tokens are per-request diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 6a66fbc6..41c432b6 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -4471,3 +4471,52 @@ though single-project `job list` looked correctly time-ordered. - Single-project `job list` is unaffected in practice (its one page was already server-sorted); the fix only changes behavior once 2+ projects are queried together. + +## `kbagent serve` permission enforcement is `/auth/*`-only so far (since vNEXT) + +`create_app` builds a `PermissionEngine` from the persisted `permissions` +policy of **the config dir `serve` resolves** (its own `--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. + +- **`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. +- **The reachable recipe is a persisted policy in the SERVED config dir.** + Verified live: + + ```bash + # in a real terminal: `permissions set` demands a typed confirmation code + kbagent --config-dir /path/to/cfg permissions set \ + --mode allow --deny auth.register-projects + kbagent serve --config-dir /path/to/cfg --port 8001 + # POST /auth/register-projects -> 403 {"error":{"code":"PERMISSION_DENIED"}} + # GET /auth/status -> 200 + # GET /auth/projects -> 200 (401 SESSION_NOT_FOUND when no session) + ``` + + 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. + Pass `--config-dir` to `serve` itself: the root-level `kbagent --config-dir + ... serve` sets the dir for the CLI invocation, not for the server process. +- **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. diff --git a/scripts/check_command_sync.py b/scripts/check_command_sync.py index 4b88b55c..787e0c7d 100644 --- a/scripts/check_command_sync.py +++ b/scripts/check_command_sync.py @@ -52,7 +52,7 @@ from keboola_agent_cli.cli import app from keboola_agent_cli.commands.context import AGENT_CONTEXT from keboola_agent_cli.commands.repl import _is_group -from keboola_agent_cli.permissions import OPERATION_REGISTRY +from keboola_agent_cli.permissions import OPERATION_REGISTRY, SERVE_ONLY_OPERATIONS REPO_ROOT = Path(__file__).resolve().parent.parent CLAUDE_MD = REPO_ROOT / "CLAUDE.md" @@ -108,11 +108,20 @@ def find_drift( groups: list[CommandPath], *, registry_keys: set[str], + serve_only_keys: frozenset[str] = frozenset(), claude_text: str, context_text: str, reference_text: str, ) -> list[str]: - """Return a human-readable block per drifted surface (empty list == clean).""" + """Return a human-readable block per drifted surface (empty list == clean). + + ``serve_only_keys`` are registry entries with no CLI leaf command by design + (they guard `kbagent serve` routes). They are exempt from the DEAD-key check + only. Subtracting them from ``registry_keys`` at the call site instead would + also feed the MISSING-key check, so the day a CLI leaf command is added for + one of them (e.g. `auth projects`), the gate would report it as missing from + OPERATION_REGISTRY while the key sat right there. + """ leaf_keys = {".".join(p) for p in leaves} all_keys = leaf_keys | {".".join(p) for p in groups} two_segment = {" ".join(p[:2]) for p in leaves} @@ -129,7 +138,7 @@ def find_drift( "fail-closed default 'write' hides their true risk category):\n" + entries ) - dead_registry = sorted(registry_keys - all_keys) + dead_registry = sorted(registry_keys - all_keys - serve_only_keys) if dead_registry: keys = "\n".join(f" {k}" for k in dead_registry) problems.append( @@ -172,6 +181,11 @@ def main() -> int: leaves, groups, registry_keys=set(OPERATION_REGISTRY), + # Serve-only operations have no CLI leaf command by design (they are + # enforced on `kbagent serve` routes), so they are not dead keys. They + # stay in `registry_keys` so a future CLI leaf command with the same + # name still counts as categorised. + serve_only_keys=SERVE_ONLY_OPERATIONS, claude_text=CLAUDE_MD.read_text(encoding="utf-8"), context_text=AGENT_CONTEXT, reference_text=COMMANDS_REFERENCE_MD.read_text(encoding="utf-8"), diff --git a/src/keboola_agent_cli/cli.py b/src/keboola_agent_cli/cli.py index d383e45c..ce1e3171 100644 --- a/src/keboola_agent_cli/cli.py +++ b/src/keboola_agent_cli/cli.py @@ -46,9 +46,12 @@ from .config_store import ConfigStore, resolve_config_dir from .constants import EXIT_PERMISSION_DENIED from .errors import ErrorCode, PermissionDeniedError -from .models import PermissionPolicy from .output import OutputFormatter, force_utf8_when_redirected -from .permissions import PermissionEngine + +# `apply_firewall_flags` lives in permissions.py so `server/app.py` composes the +# very same policy for the REST surface; re-exported here because callers (and +# tests) have imported it from `cli` since 0.22.0. +from .permissions import PermissionEngine, apply_firewall_flags from .services.agent_service import AgentService from .services.auth_service import AuthService from .services.billing_service import BillingService @@ -155,57 +158,6 @@ app.add_typer(dev_portal_app, name="dev-portal", rich_help_panel=_DEV) -def apply_firewall_flags( - persisted: PermissionPolicy | None, - *, - deny_writes: bool, - deny_destructive: bool, -) -> PermissionPolicy | None: - """Merge --deny-writes / --deny-destructive into the active policy for this invocation. - - Session-only: does NOT touch config.json. If neither flag is set, the - persisted policy is returned unchanged (possibly None). - - Merge semantics: - - A fresh session policy synthesized from the flags uses mode='allow' - so everything is allowed unless matched by the deny list. - - When a persisted policy already exists, the flag-implied deny patterns - are appended to its deny list (dedup); the mode is preserved. This is - strictly additive -- adding a flag never relaxes the persisted policy. - """ - if not deny_writes and not deny_destructive: - return persisted - - extra_deny: list[str] = [] - if deny_writes: - # The cli:write pattern intentionally spans write+destructive+admin - # (see permissions._matches_pattern). Wide net: --deny-writes blocks - # anything that mutates state. - extra_deny.append("cli:write") - if deny_destructive: - # cli:destructive narrowly matches only ops categorized 'destructive' - # (data destruction). Admin and pure-write are left allowed by design: - # the two flags exist precisely so callers can opt into the narrower - # block without forfeiting writes (e.g. allow create-bucket, block - # delete-bucket). - extra_deny.append("cli:destructive") - - if persisted is None: - return PermissionPolicy(mode="allow", allow=[], deny=extra_deny) - - # Preserve persisted mode, allow list; extend deny list without duplicates. - merged_deny = list(persisted.deny) - for pattern in extra_deny: - if pattern not in merged_deny: - merged_deny.append(pattern) - - return PermissionPolicy( - mode=persisted.mode, - allow=list(persisted.allow), - deny=merged_deny, - ) - - def _version_callback(value: bool) -> None: """Print version and exit -- standard `--version` flag for CLI tools.""" if value: diff --git a/src/keboola_agent_cli/commands/serve.py b/src/keboola_agent_cli/commands/serve.py index 607d2604..5be59778 100644 --- a/src/keboola_agent_cli/commands/serve.py +++ b/src/keboola_agent_cli/commands/serve.py @@ -134,6 +134,7 @@ def _write_banner(text: str) -> None: def serve_command( + ctx: typer.Context, host: str = typer.Option( "127.0.0.1", "--host", @@ -281,6 +282,16 @@ def serve_command( # Inverted at the boundary: the CLI flag is opt-OUT ("--no-banner"), # the app-level switch is a plain positive ("is the banner allowed"). ui_banner=not no_banner, + # Carry the process-global session firewall flags into the REST + # surface; without them the flags would guard the CLI while every route + # on the same process stayed wide open. Only the FLAGS travel -- the + # persisted policy is loaded by create_app from the config dir it + # actually serves, because `--config-dir` here may point somewhere else + # than the root callback's own `--config-dir`, and the served + # directory's policy is the one that must apply. `ctx.obj` is None when + # the callback never ran (direct invocation in tests). + deny_writes=bool((ctx.obj or {}).get("deny_writes")), + deny_destructive=bool((ctx.obj or {}).get("deny_destructive")), ) if resolved_ui_dist: diff --git a/src/keboola_agent_cli/permissions.py b/src/keboola_agent_cli/permissions.py index 0c08e0cc..844cb901 100644 --- a/src/keboola_agent_cli/permissions.py +++ b/src/keboola_agent_cli/permissions.py @@ -26,6 +26,12 @@ # tokens, never a real credential) -- same risk class as login/logout, # not the "admin" class `project add` uses for a pasted static token. "auth.register-projects": "write", + # Serve-only (since vNEXT): `GET /auth/projects` lists the session's + # registerable project candidates. It has no CLI leaf command -- the + # terminal equivalent is the interactive picker inside + # `auth register-projects` -- so it is exempted from the dead-key check + # in scripts/check_command_sync.py via SERVE_ONLY_OPERATIONS below. + "auth.projects": "read", # Project management "project.add": "admin", "project.list": "read", @@ -382,6 +388,13 @@ "auth.logout --remove-projects": "admin", } +# Operations that exist ONLY on the `kbagent serve` REST surface. They are real +# OPERATION_REGISTRY entries (a policy must be able to name and deny them), but +# 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"}) + # The operation namespace that disappeared with the MCP passthrough, and the # version that removed it. A pattern aimed at it can no longer match anything. @@ -393,6 +406,62 @@ INERT_PATTERN_HINT = "Rewrite the intent with cli:* categories -- see docs/mcp-migration.md." +def apply_firewall_flags( + persisted: PermissionPolicy | None, + *, + deny_writes: bool, + deny_destructive: bool, +) -> PermissionPolicy | None: + """Merge --deny-writes / --deny-destructive into the active policy for this invocation. + + Session-only: does NOT touch config.json. If neither flag is set, the + persisted policy is returned unchanged (possibly None). + + Merge semantics: + - A fresh session policy synthesized from the flags uses mode='allow' + so everything is allowed unless matched by the deny list. + - When a persisted policy already exists, the flag-implied deny patterns + are appended to its deny list (dedup); the mode is preserved. This is + strictly additive -- adding a flag never relaxes the persisted policy. + + Lives here rather than in ``cli.py`` because it has two callers that must + not drift: the CLI callback (``cli.py``, which re-exports this name) and + ``server.app.create_app``, which composes the same policy for the REST + surface out of the config dir it actually serves. + """ + if not deny_writes and not deny_destructive: + return persisted + + extra_deny: list[str] = [] + if deny_writes: + # The cli:write pattern intentionally spans write+destructive+admin + # (see _matches_pattern below). Wide net: --deny-writes blocks + # anything that mutates state. + extra_deny.append("cli:write") + if deny_destructive: + # cli:destructive narrowly matches only ops categorized 'destructive' + # (data destruction). Admin and pure-write are left allowed by design: + # the two flags exist precisely so callers can opt into the narrower + # block without forfeiting writes (e.g. allow create-bucket, block + # delete-bucket). + extra_deny.append("cli:destructive") + + if persisted is None: + return PermissionPolicy(mode="allow", allow=[], deny=extra_deny) + + # Preserve persisted mode, allow list; extend deny list without duplicates. + merged_deny = list(persisted.deny) + for pattern in extra_deny: + if pattern not in merged_deny: + merged_deny.append(pattern) + + return PermissionPolicy( + mode=persisted.mode, + allow=list(persisted.allow), + deny=merged_deny, + ) + + def find_inert_patterns(policy: PermissionPolicy | None) -> list[str]: """Patterns in a persisted policy that can no longer match any operation. diff --git a/src/keboola_agent_cli/server/app.py b/src/keboola_agent_cli/server/app.py index 0c44f594..0396ff1e 100644 --- a/src/keboola_agent_cli/server/app.py +++ b/src/keboola_agent_cli/server/app.py @@ -27,13 +27,15 @@ from .. import __version__ from ..config_store import ConfigStore, resolve_config_dir -from ..errors import ConfigError, ErrorCode, KeboolaApiError +from ..errors import ConfigError, ErrorCode, KeboolaApiError, PermissionDeniedError +from ..permissions import PermissionEngine, apply_firewall_flags from .agents_store import AgentStore from .auth import PUBLIC_PATHS, AuthSettings, install_auth -from .dependencies import ServiceRegistry, install_registry +from .dependencies import ServiceRegistry, install_permission_engine, install_registry from .routers import ( agents, ai_chat, + auth, billing, branches, components, @@ -79,6 +81,16 @@ # a section to the end of the sidebar with no description. OPENAPI_TAGS: list[dict[str, str]] = [ # ---- Project Management ---- + { + "name": "auth", + "description": ( + "**Project Management.** " + "Read/audit the current browser-login session and register its " + "accessible projects as local aliases. `login` / `login-password` " + "/ `logout` have no endpoint here -- see `server/routers/auth.py`. " + "Mirrors `kbagent auth status|register-projects` (partially)." + ), + }, { "name": "projects", "description": ( @@ -545,6 +557,40 @@ def _resolve_cors_origins(cors_origins: list[str] | None) -> list[str]: return origins +def _default_permission_engine( + config_store: ConfigStore, + *, + deny_writes: bool = False, + deny_destructive: bool = False, +) -> PermissionEngine: + """Build the REST surface's permission engine for the config dir being served. + + Mirrors the CLI's own bootstrap (``cli.py``): the ``permissions`` block of + config.json is the policy, the session flags are merged on top through the + shared :func:`~keboola_agent_cli.permissions.apply_firewall_flags`, and an + unreadable/corrupted config degrades to "no policy" rather than refusing to + start -- a broken config file must not take the server down. + + The policy deliberately comes from ``config_store`` -- the store + ``create_app`` resolved -- and never from the CLI callback's own store. + ``kbagent --config-dir A serve --config-dir B`` serves B, so B's persisted + policy is the one that must apply; only the two session flags travel from + the CLI invocation, because they are a property of the invocation rather + than of a directory. + """ + try: + persisted_policy = config_store.load().permissions + except Exception: + persisted_policy = None + return PermissionEngine( + apply_firewall_flags( + persisted_policy, + deny_writes=deny_writes, + deny_destructive=deny_destructive, + ) + ) + + def create_app( *, config_dir: str | None = None, @@ -553,6 +599,9 @@ def create_app( serve_url: str | None = None, ui_dist: str | None = None, ui_banner: bool = True, + deny_writes: bool = False, + deny_destructive: bool = False, + permission_engine: PermissionEngine | None = None, ) -> FastAPI: """Build and configure the FastAPI application. @@ -584,6 +633,19 @@ def create_app( ui_banner: Whether the web UI may show its unsolicited "What's new" popup. Surfaced to the SPA over ``GET /ui-config`` rather than injected into the page -- see that endpoint's docstring. + deny_writes: Apply the ``--deny-writes`` session flag to the engine + built for the resolved config dir. ``kbagent serve`` forwards the + CLI invocation's own flag here (not a pre-built engine) so the + persisted policy that applies is always the SERVED directory's -- + ``kbagent --config-dir A serve --config-dir B`` serves B, and B's + policy is the one a route is checked against. + deny_destructive: Same, for ``--deny-destructive``. + permission_engine: Explicit override for embedders and tests. When + given it wins outright: the persisted policy of the resolved config + dir and both ``deny_*`` flags are ignored, and this engine is what + routes declaring ``Depends(require_permission(...))`` are checked + against. Leave it None (the ``kbagent serve`` path) to get the + served directory's policy plus the flags above. Returns: Configured FastAPI app ready for uvicorn. @@ -654,6 +716,22 @@ async def _lifespan(app_: FastAPI): ) install_registry(app, registry) + # The engine lives on app.state, NOT on the registry: server tests routinely + # override `get_registry` with a hand-built mock, and an engine reachable + # only through the registry would be silently dropped by every such test -- + # enforcement that disappears under a test override is enforcement nobody + # can trust. `require_permission` reads it from `request.app.state` and + # fails closed when it is absent, so the attribute must always be set here. + install_permission_engine( + app, + permission_engine + or _default_permission_engine( + config_store, + deny_writes=deny_writes, + deny_destructive=deny_destructive, + ), + ) + app.state.agent_store = AgentStore(resolved_dir) from .run_broadcaster import install_broadcaster @@ -682,6 +760,14 @@ async def _api_error_handler(_request, exc: KeboolaApiError): return _format_error(msg, code, http_status=404) return _format_error(msg, code, http_status=502) + @app.exception_handler(PermissionDeniedError) + async def _permission_denied_handler(_request, exc: PermissionDeniedError): + # 403, not 401: the caller authenticated fine (the bearer token was + # accepted) -- the operation itself is what the active policy blocks. + # Same `PERMISSION_DENIED` code the CLI prints for the same denial, so + # a caller can branch on one value across both surfaces. + return _format_error(exc.message, ErrorCode.PERMISSION_DENIED, http_status=403) + @app.exception_handler(StarletteHTTPException) async def _starlette_handler(_request, exc: StarletteHTTPException): return _format_error( @@ -694,6 +780,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(auth.router) app.include_router(projects.router) app.include_router(members.router) app.include_router(feature.router) diff --git a/src/keboola_agent_cli/server/dependencies.py b/src/keboola_agent_cli/server/dependencies.py index 5bfe7e2b..9fee92ee 100644 --- a/src/keboola_agent_cli/server/dependencies.py +++ b/src/keboola_agent_cli/server/dependencies.py @@ -8,13 +8,17 @@ from __future__ import annotations +from collections.abc import Callable from dataclasses import dataclass, field from typing import TYPE_CHECKING -from fastapi import FastAPI, Request +from fastapi import Depends, FastAPI, Request from ..config_store import ConfigStore from ..dev_portal_client import DeveloperPortalClient +from ..errors import PermissionDeniedError +from ..permissions import PermissionEngine +from ..services.auth_service import AuthService from ..services.billing_service import BillingService from ..services.branch_service import BranchService from ..services.component_service import ComponentService @@ -130,6 +134,7 @@ class ServiceRegistry: docs: DocsService = field(init=False) transformation: TransformationService = field(init=False) billing: BillingService = field(init=False) + auth: AuthService = field(init=False) def __post_init__(self) -> None: cs = self.config_store @@ -173,6 +178,11 @@ def __post_init__(self) -> None: self.docs = DocsService(config_store=cs) self.transformation = TransformationService(config_store=cs) self.billing = BillingService(config_store=cs) + # AuthService's browser-facing seams (client factory, browser opener, + # sleep) all have safe defaults and are never reached by the read-only + # session/project methods the REST surface exposes -- a browser login + # only ever completes on the host, never for a REST caller. + self.auth = AuthService(config_store=cs) def install_registry(app: FastAPI, registry: ServiceRegistry) -> None: @@ -185,6 +195,66 @@ def get_registry(request: Request) -> ServiceRegistry: return request.app.state.registry # type: ignore[no-any-return] +def install_permission_engine(app: FastAPI, engine: PermissionEngine) -> None: + """Attach the REST surface's session firewall to the FastAPI app state. + + Called exactly once by ``create_app``. The engine deliberately does NOT + live on :class:`ServiceRegistry`: server tests routinely replace the + registry via ``dependency_overrides[get_registry]``, and an engine reachable + only through the registry would vanish with it -- enforcement that a test + override can silently switch off is not enforcement. ``app.state`` also + survives a second registry-construction site being added later. + """ + app.state.permission_engine = engine + + +def get_permission_engine(request: Request) -> PermissionEngine: + """Return the app's permission engine, failing CLOSED when it is missing. + + ``create_app`` always calls :func:`install_permission_engine`, so an absent + attribute means the app was assembled some other way. Treating that as "no + policy" would turn an assembly bug into a silently open firewall, so it + raises instead -- an app that cannot say whether an operation is permitted + must not perform it. "No policy configured" is expressed by an engine + wrapping a ``None`` policy, exactly as on the CLI side. + """ + engine = getattr(request.app.state, "permission_engine", None) + if engine is None: + raise PermissionDeniedError( + "Permission engine unavailable: this app was not built by create_app(), " + "so no policy can be evaluated. Refusing the operation." + ) + return engine # type: ignore[no-any-return] + + +def require_permission(operation: str) -> Callable[[PermissionEngine], None]: + """Build a FastAPI dependency enforcing the permission policy for ``operation``. + + ``operation`` is an :data:`~keboola_agent_cli.permissions.OPERATION_REGISTRY` + key (``"auth.register-projects"``, ``"config.delete"``, ...). Use it as a + route dependency:: + + @router.post("/auth/register-projects", + dependencies=[Depends(require_permission("auth.register-projects"))]) + + A denial raises :class:`~keboola_agent_cli.errors.PermissionDeniedError`, + which ``server/app.py`` maps centrally to HTTP 403 with + ``error_code: PERMISSION_DENIED`` -- the same code and message the CLI + prints, so a caller can branch on one value across both surfaces. + + 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. + """ + + def _check_permission( + engine: PermissionEngine = Depends(get_permission_engine), + ) -> None: + engine.check_or_raise(operation) + + return _check_permission + + def get_manage_token(request: Request) -> str | None: """Return the per-request manage token from the X-Manage-Token header. diff --git a/src/keboola_agent_cli/server/routers/auth.py b/src/keboola_agent_cli/server/routers/auth.py new file mode 100644 index 00000000..efed21ec --- /dev/null +++ b/src/keboola_agent_cli/server/routers/auth.py @@ -0,0 +1,109 @@ +"""Programmatic-auth session endpoints -- read/audit + local-alias registration only. + +Mirrors the read/audit half of the `kbagent auth` command group: +`GET /auth/projects` (the interactive picker's data source), `POST +/auth/register-projects`, and `GET /auth/status`. Every operation acts on a +session already established via a browser login on the host -- none of them +can create or destroy that session, and none of them ever return a token +value (`AuthStatusResult` / `ProjectCandidatesResult` / `RegisterProjectsResult` +are token-free by construction, see `services/auth_service.py`). + +`login` / `login-password` / `logout` deliberately have NO endpoints here: + +- `auth login` opens a browser (or a device-flow code) on the host machine + and only completes there -- a REST caller has no way to sit in that loop, + and "a browser login only completes on the host" is exactly the property + ``ServiceRegistry`` documents about session projects served over `serve`. +- `auth login-password` takes a plaintext password (and, for MFA accounts, + a TOTP seed) as input. That credential is meant to flow from a CI secrets + store straight into one `kbagent` CLI invocation, never as a REST request + body sitting behind this server's own bearer token. +- `auth logout` revokes the live session backing every session-registered + project reachable through this very server. Destroying that session is a + deliberate host-operator action taken at the CLI, not something a REST + client holding the serve bearer token should be able to trigger remotely. +""" + +from __future__ import annotations + +from dataclasses import asdict +from typing import Any + +from fastapi import APIRouter, Depends +from pydantic import BaseModel, ConfigDict, Field + +from ..dependencies import ServiceRegistry, get_registry, require_permission + +router = APIRouter(prefix="/auth", tags=["auth"]) + + +class RegisterProjectsBody(BaseModel): + stack: str | None = None + select_all: bool = Field(default=False, alias="all") + project_ids: list[int] | None = None + aliases: dict[int, str] | None = None # id -> alias override + + model_config = ConfigDict(populate_by_name=True) + + +@router.get( + "/projects", + summary="List the session's registerable project candidates", + dependencies=[Depends(require_permission("auth.projects"))], +) +def list_project_candidates( + stack: str | None = None, + registry: ServiceRegistry = Depends(get_registry), +) -> dict[str, Any]: + """Projects the current session for ``stack`` can access, with a + collision-free suggested alias each. No CLI leaf command -- the terminal + equivalent is the interactive picker inside `kbagent auth + register-projects`. Read-only: never writes `config.json`. + """ + return asdict(registry.auth.list_project_candidates(stack=stack)) + + +@router.post( + "/register-projects", + summary="Register accessible projects as local aliases", + dependencies=[Depends(require_permission("auth.register-projects"))], +) +def register_projects( + body: RegisterProjectsBody, + registry: ServiceRegistry = Depends(get_registry), +) -> dict[str, Any]: + """Register accessible projects as `kbagent` aliases under session-sentinel + tokens. Mirrors `kbagent auth register-projects --all` / `--project-id`. + + Exactly one of `all` (aliased to `select_all`) or `project_ids` selects + the batch -- the service raises `ConfigError` for zero or both, which + propagates to the central error handler unchanged. `aliases` overrides + the suggested alias per project id; keys arrive as `int` (coerced from + the JSON body's string keys). The interactive picker's own `selections` + parameter is intentionally unreachable from this body -- it is a CLI-only + concept with no REST representation. + """ + return asdict( + registry.auth.register_projects( + stack=body.stack, + select_all=body.select_all, + project_ids=body.project_ids, + alias_overrides=body.aliases, + ) + ) + + +@router.get( + "/status", + summary="Session health for a stack", + dependencies=[Depends(require_permission("auth.status"))], +) +def auth_status( + stack: str | None = None, + registry: ServiceRegistry = Depends(get_registry), +) -> dict[str, Any]: + """Report whether the stored session for `stack` is live, refreshed, + degraded (offline), expired, or missing -- without mutating it. Mirrors + `kbagent auth status`. + """ + return asdict(registry.auth.status(stack=stack)) diff --git a/tests/test_check_command_sync.py b/tests/test_check_command_sync.py index 2ba9a7ba..158622e5 100644 --- a/tests/test_check_command_sync.py +++ b/tests/test_check_command_sync.py @@ -111,6 +111,39 @@ def test_find_drift_detects_missing_documentation() -> None: assert "commands-reference.md" in blob +def test_serve_only_key_is_exempt_from_the_dead_key_check() -> None: + surfaces = _clean_surfaces() + surfaces["registry_keys"] = {"config", "config.list", "auth.projects"} + problems = _mod.find_drift( + [("config", "list")], + [("config",)], + serve_only_keys=frozenset({"auth.projects"}), + **surfaces, + ) + assert problems == [] + + +def test_serve_only_key_still_counts_as_categorised_once_a_cli_leaf_exists() -> None: + """The exemption must not leak into the MISSING-registry check. + + If `auth projects` ever becomes a real CLI leaf, its registry key is + already there -- reporting it as missing would send the author to add a + duplicate entry. + """ + surfaces = _clean_surfaces() + surfaces["registry_keys"] = {"config", "config.list", "auth", "auth.projects"} + surfaces["claude_text"] = "kbagent config list --project NAME\nkbagent auth projects" + surfaces["context_text"] = "config list\nauth projects" + surfaces["reference_text"] = "config list\nauth projects" + problems = _mod.find_drift( + [("config", "list"), ("auth", "projects")], + [("config",), ("auth",)], + serve_only_keys=frozenset({"auth.projects"}), + **surfaces, + ) + assert problems == [] + + def test_claude_is_full_leaf_while_reference_is_two_segment() -> None: """A 3-level leaf: CLAUDE.md needs the full path; context/reference the 2-seg prefix.""" leaves = [("grp", "add", "metric")] diff --git a/tests/test_server_auth.py b/tests/test_server_auth.py new file mode 100644 index 00000000..7ca44cef --- /dev/null +++ b/tests/test_server_auth.py @@ -0,0 +1,457 @@ +"""Tests for the `/auth/*` REST router (Task 2 of issue #537). + +Covers three things: + +1. Router -> service kwarg parity (the pattern from + ``tests/test_server_router_calls.py``): each endpoint must call the + corresponding ``AuthService`` method with the exact keyword arguments the + brief specifies -- never a positional call, never a renamed kwarg, and + never the CLI-only ``selections`` parameter. +2. Central error translation: a ``KeboolaApiError`` carrying + ``SESSION_NOT_FOUND`` must answer HTTP 401 (via ``_SESSION_CREDENTIAL_CODES`` + in ``server/app.py``), and a ``ConfigError`` (the service's own guard + against zero/two selectors) must answer a 4xx, never a 500. +3. The permission seam from Task 1: ``POST /auth/register-projects`` is a + write operation and must be blocked by a deny-writes policy while the two + GET endpoints stay open, and no route exists at all for + ``/auth/login`` / ``/auth/login-password`` / ``/auth/logout``. +""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock + +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.errors import ConfigError, ErrorCode, KeboolaApiError +from keboola_agent_cli.permissions import PermissionEngine +from keboola_agent_cli.server import create_app +from keboola_agent_cli.server.dependencies import ServiceRegistry, get_registry +from keboola_agent_cli.services._auth_registration import SESSION_UNSUPPORTED_FEATURES +from keboola_agent_cli.services.auth_service import ( + AuthStatusResult, + ProjectCandidate, + ProjectCandidatesResult, + RegisteredProject, + RegisterProjectsResult, +) + +AUTH = {"Authorization": "Bearer test-token"} +STACK = "https://connection.keboola.com" + + +def _mock_registry(**services: Any) -> ServiceRegistry: + """Bare ServiceRegistry with only the given service mocks attached. + + Matches the established pattern in test_server_router_calls.py. The + permission engine is deliberately NOT here: it lives on ``app.state``, so + overriding ``get_registry`` with this stub leaves enforcement intact. + """ + registry = ServiceRegistry.__new__(ServiceRegistry) + for name, mock in services.items(): + setattr(registry, name, mock) + return registry + + +def _make_app_with_registry(tmp_path: Path, registry: ServiceRegistry) -> Any: + app = create_app(config_dir=str(tmp_path), auth_token="test-token") + app.dependency_overrides[get_registry] = lambda: registry + return app + + +def _status_result(**overrides: Any) -> AuthStatusResult: + base: dict[str, Any] = dict( + status="live", + stack_url=STACK, + session_id="sess-1", + user_email="user@example.com", + user_name="Test User", + access_expires_at="2026-08-24T00:00:00+00:00", + refresh_expires_at="2026-09-23T00:00:00+00:00", + accessible_projects=[{"id": 123, "name": "Prod", "role": "admin"}], + orphaned_session_ids=[], + detail="", + ) + base.update(overrides) + return AuthStatusResult(**base) + + +def _candidates_result(**overrides: Any) -> ProjectCandidatesResult: + base: dict[str, Any] = dict( + stack_url=STACK, + candidates=[ + ProjectCandidate( + project_id=123, + project_name="Prod", + role="admin", + default_alias="prod-123", + existing_alias="", + registered=False, + ) + ], + ) + base.update(overrides) + return ProjectCandidatesResult(**base) + + +def _register_result(**overrides: Any) -> RegisterProjectsResult: + base: dict[str, Any] = dict( + status="ok", + stack_url=STACK, + registered_projects=[ + RegisteredProject( + alias="prod-123", + project_id=123, + project_name="Prod", + status="registered", + ) + ], + warnings=[], + ) + base.update(overrides) + return RegisterProjectsResult(**base) + + +# --------------------------------------------------------------------------- +# 1. kwarg parity +# --------------------------------------------------------------------------- + + +class TestKwargParity: + def test_list_project_candidates_passes_stack_kwarg(self, tmp_path: Path) -> None: + auth_svc = MagicMock() + auth_svc.list_project_candidates.return_value = _candidates_result() + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.get("/auth/projects", headers=AUTH, params={"stack": STACK}) + + assert resp.status_code == 200, resp.text + auth_svc.list_project_candidates.assert_called_once_with(stack=STACK) + + def test_list_project_candidates_defaults_stack_to_none(self, tmp_path: Path) -> None: + auth_svc = MagicMock() + auth_svc.list_project_candidates.return_value = _candidates_result() + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.get("/auth/projects", headers=AUTH) + + assert resp.status_code == 200, resp.text + auth_svc.list_project_candidates.assert_called_once_with(stack=None) + + def test_status_passes_stack_kwarg(self, tmp_path: Path) -> None: + auth_svc = MagicMock() + auth_svc.status.return_value = _status_result() + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.get("/auth/status", headers=AUTH, params={"stack": STACK}) + + assert resp.status_code == 200, resp.text + auth_svc.status.assert_called_once_with(stack=STACK) + + def test_register_projects_passes_exact_kwargs(self, tmp_path: Path) -> None: + auth_svc = MagicMock() + auth_svc.register_projects.return_value = _register_result() + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.post( + "/auth/register-projects", + headers=AUTH, + json={"stack": STACK, "project_ids": [123]}, + ) + + assert resp.status_code == 200, resp.text + kwargs = auth_svc.register_projects.call_args.kwargs + assert kwargs == { + "stack": STACK, + "select_all": False, + "project_ids": [123], + "alias_overrides": None, + } + # The CLI picker's own parameter must never be reachable from REST. + assert "selections" not in kwargs + + +# --------------------------------------------------------------------------- +# 2. request-body shape: `all` alias, project_ids + aliases int coercion +# --------------------------------------------------------------------------- + + +class TestRegisterProjectsBody: + def test_all_alias_sets_select_all(self, tmp_path: Path) -> None: + auth_svc = MagicMock() + auth_svc.register_projects.return_value = _register_result() + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.post("/auth/register-projects", headers=AUTH, json={"all": True}) + + assert resp.status_code == 200, resp.text + kwargs = auth_svc.register_projects.call_args.kwargs + assert kwargs["select_all"] is True + assert kwargs["project_ids"] is None + + def test_select_all_field_name_also_works(self, tmp_path: Path) -> None: + # populate_by_name=True: the Python field name is accepted too, not + # only the "all" alias. + auth_svc = MagicMock() + auth_svc.register_projects.return_value = _register_result() + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.post("/auth/register-projects", headers=AUTH, json={"select_all": True}) + + assert resp.status_code == 200, resp.text + assert auth_svc.register_projects.call_args.kwargs["select_all"] is True + + def test_project_ids_and_aliases_pass_through_with_int_keys(self, tmp_path: Path) -> None: + auth_svc = MagicMock() + auth_svc.register_projects.return_value = _register_result() + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.post( + "/auth/register-projects", + headers=AUTH, + json={ + "project_ids": [123, 456], + "aliases": {"123": "prod", "456": "stage"}, + }, + ) + + assert resp.status_code == 200, resp.text + kwargs = auth_svc.register_projects.call_args.kwargs + assert kwargs["project_ids"] == [123, 456] + assert kwargs["alias_overrides"] == {123: "prod", 456: "stage"} + # JSON object keys are always strings on the wire; pydantic must have + # coerced them to int per the `dict[int, str]` annotation. + assert all(isinstance(k, int) for k in kwargs["alias_overrides"]) + + +# --------------------------------------------------------------------------- +# 3. error translation +# --------------------------------------------------------------------------- + + +class TestErrorTranslation: + def test_session_not_found_answers_401_with_error_code(self, tmp_path: Path) -> None: + auth_svc = MagicMock() + auth_svc.status.side_effect = KeboolaApiError( + "no session", error_code=ErrorCode.SESSION_NOT_FOUND + ) + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.get("/auth/status", headers=AUTH, params={"stack": STACK}) + + assert resp.status_code == 401, resp.text + body = resp.json() + assert body["error"]["code"] == "SESSION_NOT_FOUND" + + def test_config_error_from_bad_selector_combo_is_4xx_not_500(self, tmp_path: Path) -> None: + auth_svc = MagicMock() + auth_svc.register_projects.side_effect = ConfigError( + "Selection modes are mutually exclusive; got select_all + project_ids." + ) + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.post( + "/auth/register-projects", + headers=AUTH, + json={"all": True, "project_ids": [1]}, + ) + + assert 400 <= resp.status_code < 500, resp.text + assert resp.json()["error"]["code"] == "CONFIG_ERROR" + + +# --------------------------------------------------------------------------- +# 4. permission enforcement +# --------------------------------------------------------------------------- + + +class TestPermissionEnforcement: + def _registry(self) -> ServiceRegistry: + auth_svc = MagicMock() + auth_svc.list_project_candidates.return_value = _candidates_result() + auth_svc.status.return_value = _status_result() + auth_svc.register_projects.return_value = _register_result() + return _mock_registry(auth=auth_svc) + + def _deny_writes_app(self, tmp_path: Path) -> Any: + """App whose session policy denies writes, with the mock registry bound. + + The engine is passed explicitly (the embedder/test escape hatch) and + lives on ``app.state``, so the ``get_registry`` override below cannot + take enforcement with it. + """ + from keboola_agent_cli.permissions import apply_firewall_flags + + engine = PermissionEngine( + apply_firewall_flags(None, deny_writes=True, deny_destructive=False) + ) + app = create_app( + config_dir=str(tmp_path), auth_token="test-token", permission_engine=engine + ) + app.dependency_overrides[get_registry] = lambda: self._registry() + return app + + def test_post_register_projects_is_denied(self, tmp_path: Path) -> None: + with TestClient(self._deny_writes_app(tmp_path)) as client: + resp = client.post("/auth/register-projects", headers=AUTH, json={"all": True}) + + assert resp.status_code == 403, resp.text + assert resp.json()["error"]["code"] == "PERMISSION_DENIED" + + def test_get_projects_and_status_pass_through(self, tmp_path: Path) -> None: + with TestClient(self._deny_writes_app(tmp_path)) as client: + assert client.get("/auth/projects", headers=AUTH).status_code == 200 + assert client.get("/auth/status", headers=AUTH).status_code == 200 + + def test_persisted_policy_in_the_served_dir_blocks_only_the_write(self, tmp_path: Path) -> None: + """The reachable enforcement recipe, end to end. + + `kbagent --deny-writes serve` cannot start (`serve` is admin-class and + `cli:write` spans admin), so a narrow persisted policy in the SERVED + config dir is what an operator actually uses. No engine is passed here + -- `create_app` must load this policy from disk itself. + """ + from keboola_agent_cli.config_store import ConfigStore + from keboola_agent_cli.models import PermissionPolicy + + store = ConfigStore(config_dir=tmp_path) + config = store.load() + config.permissions = PermissionPolicy(mode="allow", deny=["auth.register-projects"]) + store.save(config) + + app = create_app(config_dir=str(tmp_path), auth_token="test-token") + app.dependency_overrides[get_registry] = lambda: self._registry() + + with TestClient(app) as client: + denied = client.post("/auth/register-projects", headers=AUTH, json={"all": True}) + assert denied.status_code == 403, denied.text + assert denied.json()["error"]["code"] == "PERMISSION_DENIED" + assert client.get("/auth/projects", headers=AUTH).status_code == 200 + assert client.get("/auth/status", headers=AUTH).status_code == 200 + + +# --------------------------------------------------------------------------- +# 5. token-free serialization +# --------------------------------------------------------------------------- + + +class TestTokenFreeSerialization: + def test_register_projects_response_carries_no_token_material(self, tmp_path: Path) -> None: + auth_svc = MagicMock() + auth_svc.register_projects.return_value = _register_result( + registered_projects=[ + RegisteredProject( + alias="prod-123", + project_id=123, + project_name="Prod", + status="registered", + ), + RegisteredProject( + alias="stage-456", + project_id=456, + project_name="Stage", + status="exists", + note="Already registered.", + ), + ], + warnings=["Alias 'foo' already points at a different project; not overwritten."], + ) + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.post("/auth/register-projects", headers=AUTH, json={"all": True}) + + assert resp.status_code == 200, resp.text + for needle in ("kbc-session://", "kbc_at_", "kbc_rt_"): + assert needle not in resp.text + + def test_session_unsupported_features_survives_the_rest_boundary(self, tmp_path: Path) -> None: + """`session_unsupported_features` is the caller's authoritative list. + + It rides `RegisterProjectsResult` via a default factory, so a REST + caller that just registered session projects learns which surfaces will + fail on them WITHOUT re-deriving the list by hand. `asdict` must carry + it through unchanged -- dropping it would silently push every caller + back to hard-coding a copy. + """ + auth_svc = MagicMock() + auth_svc.register_projects.return_value = _register_result() + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.post("/auth/register-projects", headers=AUTH, json={"all": True}) + + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["session_unsupported_features"] == list(SESSION_UNSUPPORTED_FEATURES) + assert body["session_unsupported_features"], "the list must not be empty" + + def test_projects_response_carries_no_token_material(self, tmp_path: Path) -> None: + auth_svc = MagicMock() + auth_svc.list_project_candidates.return_value = _candidates_result( + candidates=[ + ProjectCandidate( + project_id=123, + project_name="Prod", + role="admin", + default_alias="prod-123", + existing_alias="prod-123", + registered=True, + ), + ProjectCandidate( + project_id=456, + project_name="Stage", + role="share", + default_alias="stage-456", + existing_alias="", + registered=False, + ), + ] + ) + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.get("/auth/projects", headers=AUTH) + + assert resp.status_code == 200, resp.text + for needle in ("kbc-session://", "kbc_at_", "kbc_rt_"): + assert needle not in resp.text + + +# --------------------------------------------------------------------------- +# 6. login / login-password / logout have no route +# --------------------------------------------------------------------------- + + +class TestNoCredentialMintingRoutes: + @pytest.mark.parametrize( + "path", + ["/auth/login", "/auth/login-password", "/auth/logout"], + ) + def test_route_does_not_exist(self, tmp_path: Path, path: str) -> None: + auth_svc = MagicMock() + app = _make_app_with_registry(tmp_path, _mock_registry(auth=auth_svc)) + + with TestClient(app) as client: + resp = client.post(path, headers=AUTH, json={}) + + assert resp.status_code == 404, resp.text diff --git a/tests/test_server_permissions.py b/tests/test_server_permissions.py new file mode 100644 index 00000000..c458924f --- /dev/null +++ b/tests/test_server_permissions.py @@ -0,0 +1,323 @@ +"""Tests for the permission seam on the ``kbagent serve`` REST surface. + +The CLI has always had a session firewall (`--deny-writes` / +`--deny-destructive` plus the persisted `config.permissions` policy). Until +now it stopped at the terminal: every route on the same process was wide +open. These tests pin the seam that closes that gap: + +* ``app.state.permission_engine`` -- built by ``create_app`` from the persisted + policy of the config dir it SERVES, plus the ``--deny-writes`` / + ``--deny-destructive`` flags ``kbagent serve`` forwards. +* ``require_permission(operation)`` -- the FastAPI dependency routes declare. +* ``PermissionDeniedError`` -> HTTP 403 with ``error_code: PERMISSION_DENIED``, + the same code the CLI prints for the same denial. + +Task 2 builds the ``/auth/*`` router on top of this; the probe routes below +exercise the seam without depending on any particular route existing. +""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path +from typing import Any + +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 import Depends +from fastapi.testclient import TestClient + +from keboola_agent_cli.config_store import ConfigStore +from keboola_agent_cli.constants import EXIT_PERMISSION_DENIED +from keboola_agent_cli.models import PermissionPolicy +from keboola_agent_cli.permissions import ( + OPERATION_REGISTRY, + SERVE_ONLY_OPERATIONS, + PermissionEngine, +) +from keboola_agent_cli.server import create_app +from keboola_agent_cli.server.dependencies import ( + ServiceRegistry, + get_registry, + require_permission, +) +from keboola_agent_cli.services.auth_service import AuthService + +TOKEN = "perm-test-token" +AUTH = {"Authorization": f"Bearer {TOKEN}"} + +# One read and one destructive probe, so a policy targeting a category can be +# observed to block one while letting the other through. +READ_PROBE = "/_probe/read" +DESTRUCTIVE_PROBE = "/_probe/destructive" + + +def _install_probes(app: Any) -> None: + """Register two test-only routes guarded by ``require_permission``.""" + + @app.get(READ_PROBE, dependencies=[Depends(require_permission("config.list"))]) + def _read_probe() -> dict[str, str]: + return {"status": "ok", "operation": "config.list"} + + @app.get(DESTRUCTIVE_PROBE, dependencies=[Depends(require_permission("config.delete"))]) + def _destructive_probe() -> dict[str, str]: + return {"status": "ok", "operation": "config.delete"} + + +def _persist_policy(config_dir: Path, policy: PermissionPolicy) -> None: + """Write ``policy`` into config.json of ``config_dir`` the way the CLI does.""" + store = ConfigStore(config_dir=config_dir) + config = store.load() + config.permissions = policy + store.save(config) + + +def _client(tmp_path: Path, **create_app_kwargs: Any) -> TestClient: + app = create_app(config_dir=str(tmp_path), auth_token=TOKEN, **create_app_kwargs) + _install_probes(app) + return TestClient(app) + + +class TestDefaultEngineFromPersistedPolicy: + """``create_app`` with no engine reads ``config.permissions`` from disk.""" + + def test_denied_operation_answers_403(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["cli:destructive"])) + resp = _client(tmp_path).get(DESTRUCTIVE_PROBE, headers=AUTH) + assert resp.status_code == 403, resp.text + + def test_allowed_operation_passes_through(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["cli:destructive"])) + resp = _client(tmp_path).get(READ_PROBE, headers=AUTH) + assert resp.status_code == 200, resp.text + assert resp.json()["operation"] == "config.list" + + def test_no_config_file_means_no_policy(self, tmp_path: Path) -> None: + # Nothing persisted at all -- every operation passes, exactly like + # `PermissionEngine(None)` on the CLI side. + client = _client(tmp_path / "empty") + assert client.get(READ_PROBE, headers=AUTH).status_code == 200 + assert client.get(DESTRUCTIVE_PROBE, headers=AUTH).status_code == 200 + + def test_corrupted_config_degrades_to_no_policy(self, tmp_path: Path) -> None: + # A broken config file must not take the server down; it degrades to + # "no policy" the same way the CLI bootstrap does. + tmp_path.mkdir(parents=True, exist_ok=True) + (tmp_path / "config.json").write_text("{ not json", encoding="utf-8") + assert _client(tmp_path).get(DESTRUCTIVE_PROBE, headers=AUTH).status_code == 200 + + +class TestExplicitEngineWins: + """An engine passed to ``create_app`` replaces the persisted-policy default. + + This is how ``kbagent serve`` carries the process-global ``--deny-writes`` + / ``--deny-destructive`` flags into the REST surface. + """ + + def _session_engine(self) -> PermissionEngine: + from keboola_agent_cli.cli import apply_firewall_flags + + policy = apply_firewall_flags(None, deny_writes=True, deny_destructive=False) + return PermissionEngine(policy) + + def test_session_deny_writes_blocks_destructive_probe(self, tmp_path: Path) -> None: + # Persisted policy would allow everything; the session engine does not. + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=[])) + client = _client(tmp_path, permission_engine=self._session_engine()) + assert client.get(DESTRUCTIVE_PROBE, headers=AUTH).status_code == 403 + + def test_persisted_policy_is_not_consulted(self, tmp_path: Path) -> None: + # Persisted policy denies the READ probe; the explicit engine (which + # only denies writes) wins, so the read passes. + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["config.list"])) + client = _client(tmp_path, permission_engine=self._session_engine()) + assert client.get(READ_PROBE, headers=AUTH).status_code == 200 + + +class TestErrorEnvelope: + """A denial renders the kbagent error envelope, not a FastAPI detail body.""" + + def test_403_body_carries_permission_denied_code(self, tmp_path: Path) -> None: + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["cli:destructive"])) + resp = _client(tmp_path).get(DESTRUCTIVE_PROBE, headers=AUTH) + assert resp.status_code == 403 + body = resp.json() + assert body["status"] == "error" + assert body["error"]["code"] == "PERMISSION_DENIED" + # The message names the operation, so a caller knows what to re-run + # with a different policy. + assert "config.delete" in body["error"]["message"] + + def test_denial_is_403_not_401(self, tmp_path: Path) -> None: + # The bearer token was accepted; only the operation is blocked. A 401 + # would send callers chasing their credentials instead of the policy. + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["cli:destructive"])) + client = _client(tmp_path) + assert client.get(DESTRUCTIVE_PROBE).status_code == 401 # no bearer at all + assert client.get(DESTRUCTIVE_PROBE, headers=AUTH).status_code == 403 + + +class TestRequirePermissionDependency: + """Behaviour of the dependency itself, independent of ``create_app``.""" + + def test_registry_override_cannot_disable_enforcement(self, tmp_path: Path) -> None: + # Server tests routinely override `get_registry` with a registry built + # via `__new__` (no `__init__`). The engine lives on app.state, not on + # the registry, precisely so such an override cannot silently switch + # the firewall off. + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["cli:destructive"])) + app = create_app(config_dir=str(tmp_path), auth_token=TOKEN) + _install_probes(app) + bare = ServiceRegistry.__new__(ServiceRegistry) + app.dependency_overrides[get_registry] = lambda: bare + client = TestClient(app) + assert client.get(DESTRUCTIVE_PROBE, headers=AUTH).status_code == 403 + assert client.get(READ_PROBE, headers=AUTH).status_code == 200 + + def test_app_without_an_engine_fails_closed(self, tmp_path: Path) -> None: + # An app assembled some other way than create_app has no engine, so it + # cannot say whether an operation is permitted -- it must refuse rather + # than treat the missing attribute as "no policy". + from fastapi import FastAPI + + from keboola_agent_cli.server.auth import AuthSettings, install_auth + + app = FastAPI() + install_auth(app, AuthSettings(token=TOKEN)) + _install_probes(app) + resp = TestClient(app, raise_server_exceptions=False).get(READ_PROBE, headers=AUTH) + assert resp.status_code != 200 + + +class TestRegistryWiring: + """The registry exposes AuthService; the engine lives on app.state.""" + + def test_auth_service_is_registered(self, tmp_path: Path) -> None: + app = create_app(config_dir=str(tmp_path), auth_token=TOKEN) + assert isinstance(app.state.registry.auth, AuthService) + + def test_explicit_engine_is_stored_on_app_state(self, tmp_path: Path) -> None: + engine = PermissionEngine(PermissionPolicy(mode="allow", deny=["cli:destructive"])) + app = create_app(config_dir=str(tmp_path), auth_token=TOKEN, permission_engine=engine) + assert app.state.permission_engine is engine + # Not on the registry: one source of truth, and a registry override in + # a test must not be able to drop it. + assert not hasattr(app.state.registry, "permission_engine") + + +class TestServedConfigDirPolicyWins: + """`create_app` reads the policy of the dir it SERVES, not the caller's. + + `kbagent --config-dir A serve --config-dir B` serves B. Before this, the + CLI callback's pre-built engine (policy of A) was handed to `create_app` + while every service read B -- so B's persisted deny policy was silently + ignored whenever the two dirs diverged. + """ + + def test_served_dir_policy_applies_when_dirs_diverge(self, tmp_path: Path) -> None: + caller_dir = tmp_path / "caller" + served_dir = tmp_path / "served" + # The caller's own dir allows everything; only the SERVED dir denies. + _persist_policy(caller_dir, PermissionPolicy(mode="allow", deny=[])) + _persist_policy(served_dir, PermissionPolicy(mode="allow", deny=["cli:destructive"])) + + client = _client(served_dir) + assert client.get(DESTRUCTIVE_PROBE, headers=AUTH).status_code == 403 + assert client.get(READ_PROBE, headers=AUTH).status_code == 200 + + def test_session_flags_merge_onto_the_served_dir_policy(self, tmp_path: Path) -> None: + # The flags are a property of the invocation, so they travel; the + # persisted policy is the served dir's. Both must end up in force. + _persist_policy(tmp_path, PermissionPolicy(mode="allow", deny=["config.list"])) + client = _client(tmp_path, deny_destructive=True) + assert client.get(READ_PROBE, headers=AUTH).status_code == 403 # persisted + assert client.get(DESTRUCTIVE_PROBE, headers=AUTH).status_code == 403 # flag + + +class TestServeCommandCarriesTheFlags: + """`kbagent --deny-destructive serve` must reach the REST surface.""" + + def _invoke_serve( + self, + monkeypatch: pytest.MonkeyPatch, + argv: list[str], + ) -> tuple[Any, dict[str, Any]]: + import uvicorn + from typer.testing import CliRunner + + from keboola_agent_cli import server as server_pkg + from keboola_agent_cli.cli import app as cli_app + + monkeypatch.setenv("KBAGENT_AUTO_UPDATE", "false") + captured: dict[str, Any] = {} + + def _fake_create_app(**kwargs: Any) -> object: + captured.update(kwargs) + return object() + + def _fake_run(*_args: Any, **_kwargs: Any) -> None: + return None + + monkeypatch.setattr(server_pkg, "create_app", _fake_create_app) + monkeypatch.setattr(uvicorn, "run", _fake_run) + return CliRunner().invoke(cli_app, argv), captured + + def test_deny_destructive_flag_is_forwarded( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + result, captured = self._invoke_serve( + monkeypatch, ["--config-dir", str(tmp_path), "--deny-destructive", "serve"] + ) + assert result.exit_code == 0, result.output + assert captured["deny_destructive"] is True + assert captured["deny_writes"] is False + # The CLI must NOT hand over a pre-built engine any more -- that engine + # carried the caller's config dir, not the served one. + assert captured.get("permission_engine") is None + + def test_no_flags_forwards_false(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + result, captured = self._invoke_serve(monkeypatch, ["--config-dir", str(tmp_path), "serve"]) + assert result.exit_code == 0, result.output + assert captured["deny_writes"] is False + assert captured["deny_destructive"] is False + + +class TestDenyWritesBlocksTheServeCommandItself: + """`kbagent --deny-writes serve` never starts the server. + + `serve` is classified `admin` in OPERATION_REGISTRY and `--deny-writes` + appends `cli:write`, which spans write+destructive+admin -- so the CLI + callback denies the `serve` command before uvicorn is ever reached. Every + doc surface that recommends enforcement over REST must therefore recommend + a persisted policy (or `--deny-destructive`), never `--deny-writes`. + """ + + def test_deny_writes_serve_exits_permission_denied( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + result, captured = TestServeCommandCarriesTheFlags()._invoke_serve( + monkeypatch, ["--config-dir", str(tmp_path), "--deny-writes", "serve"] + ) + assert result.exit_code == EXIT_PERMISSION_DENIED + # create_app was never called: the command itself was blocked. + assert captured == {} + + +class TestOperationRegistryEntry: + """`auth.projects` is the serve-only read endpoint Task 2 will expose.""" + + def test_auth_projects_is_a_read_operation(self) -> None: + assert OPERATION_REGISTRY["auth.projects"] == "read" + + def test_auth_projects_is_exempt_from_the_command_sync_gate(self) -> None: + # It has no CLI leaf command, so the dead-key check in + # scripts/check_command_sync.py must skip it. + assert "auth.projects" in SERVE_ONLY_OPERATIONS + + def test_serve_only_operations_are_all_registered(self) -> None: + assert set(OPERATION_REGISTRY) >= SERVE_ONLY_OPERATIONS