serve: expose auth register-projects over REST (POST /auth/register-projects) - #677
Merged
Conversation
padak
force-pushed
the
feat/537-serve-auth-register-projects
branch
from
August 23, 2026 23:05
f3edda8 to
2a5761b
Compare
Adds the first permission-policy enforcement seam on the `kbagent serve` REST surface, plus the AuthService wiring the /auth/* router will need. - ServiceRegistry gains `auth: AuthService` and a `permission_engine` field. create_app builds the engine from the persisted `config.permissions` policy when none is passed (a corrupted config degrades to "no policy" instead of refusing to start), and `serve_command` now forwards the CLI's own engine so the process-global --deny-writes / --deny-destructive flags reach the routes too. - New `require_permission(operation)` FastAPI dependency factory; PermissionDeniedError maps centrally to HTTP 403 with the same PERMISSION_DENIED error code the CLI prints. - OPERATION_REGISTRY gains the serve-only `auth.projects` (read). It has no CLI leaf command, so SERVE_ONLY_OPERATIONS exempts it from the dead-key check in scripts/check_command_sync.py. - tests/test_server_permissions.py covers the default/explicit engine precedence, the 403 envelope, and the serve -> create_app plumbing.
Expose the read/audit half of the auth session picker over the REST surface: GET /auth/projects, POST /auth/register-projects, and GET /auth/status, each gated by require_permission and delegating to AuthService. login/login-password/logout stay CLI-only (documented in the router's module docstring) since they mint or destroy live browser sessions rather than read an existing one.
…nforcement Task 3 of issue #537: document the read/audit auth endpoints added in Tasks 1-2 (GET /auth/projects, POST /auth/register-projects, GET /auth/status), their permission ops, and the deliberate login/login-password/logout REST gaps, replacing the now-stale "auth has no REST router" claim in docs/web-server.md. Also records that /auth/* is the first (and so far only) router enforcing the permissions policy, and documents the SERVE_ONLY_OPERATIONS registry-key exemption in CONTRIBUTING.md's command-sync gate section. All version-gated text uses the (since vNEXT) placeholder per the no-version-bump-in-feature-PRs convention.
GET /auth/status never raises SESSION_EXPIRED/SESSION_NOT_FOUND -- AuthService.status() explicitly catches both cases and reports session health (status: missing/expired/ degraded/refreshed/live) in the response body with HTTP 200, because it is the probe callers use to detect a dead session and must not itself fail that way. The 401 behavior only applies to GET /auth/projects and POST /auth/register-projects, which go through _introspect_accessible_projects / provider.introspect() and let those error codes propagate. Corrects the previous docs/web-server.md passage that lumped all three endpoints together.
…onfig dir Fix wave over the whole #537 branch, addressing the review findings: - create_app now builds the permission engine from the persisted policy of the config dir it actually SERVES plus the session flags, instead of taking the CLI callback's pre-built engine. `kbagent --config-dir A serve --config-dir B` serves B, so B's policy is the one routes are checked against; only the --deny-writes / --deny-destructive flags travel from the invocation. `apply_firewall_flags` moves to permissions.py so the CLI and create_app share one composition (re-exported from cli.py for existing callers). `permission_engine=` stays as an explicit override for embedders and tests. - The engine now lives on app.state, not on ServiceRegistry, and require_permission reads it from request.app.state and fails CLOSED when it is absent. Overriding get_registry in a test can no longer silently disable enforcement; the hand-set registry.permission_engine workaround is gone. - Docs (docs/web-server.md, CLAUDE.md, gotchas.md) now lead with the reachable enforcement forms. `kbagent --deny-writes serve` cannot start the server at all -- `serve` is admin-class and cli:write spans admin -- so a persisted narrow policy in the served config dir is the recipe, verified live. - Scope the /auth/status doc claim: 200 for a missing or expired session, but an unresolvable stack (4xx) and an unexpected auth-service failure (502) are still errors. - check_command_sync.find_drift takes serve-only keys as its own parameter and subtracts them only in the dead-key check, so a future `auth projects` CLI leaf cannot be misreported as missing from OPERATION_REGISTRY. - Tests: divergent-config-dir regression, deny-writes-blocks-serve, end-to-end persisted-policy enforcement on /auth/*, session_unsupported_features surviving the REST boundary, and a dict annotation that unblocks `ty`.
padak
force-pushed
the
feat/537-serve-auth-register-projects
branch
from
August 23, 2026 23:20
2a5761b to
66216db
Compare
This was referenced Aug 23, 2026
Merged
This was referenced Aug 23, 2026
padak
added a commit
that referenced
this pull request
Aug 24, 2026
…ns comments (#681) * docs(permissions): fix a leftover vNEXT and the false serve-enforcement claim Two corrections in OPERATION_REGISTRY's comments, both fallout of #677: - `auth.projects` still carried `(since vNEXT)` after the 0.90.1 release. The release-time gate (scripts/check_version_gates.py) only scans markdown surfaces -- CLAUDE.md, docs/*.md, plugins/**/*.md -- so a placeholder in src/ resolves nowhere and ships silently. Pinned to 0.90.1, the release that actually carries the endpoint. - The `http.*` block claimed "the serve's own routes enforce their own permissions on top". Issue #655 flagged that as load-bearing and false: it was true for no route at all. Since 0.90.1 it is true for /auth/* and nothing else, so the comment now states that scope instead of a blanket claim that misleads the risk classification directly beneath it. * serve: honor the root-level --config-dir (closes #679) `serve` is the only subcommand carrying a `--config-dir` of its own, and it forwarded only that one into `create_app`. The root-level `kbagent --config-dir A serve` was ignored entirely, with no warning: the server exposed whatever the fallback chain resolved (usually the global config) instead of the directory the caller named. Since 0.90.1, when the `/auth/*` routes began enforcing the persisted `permissions` policy, that also meant a deny policy stored beside the caller's projects was silently not the one in force -- verified live: `POST /auth/register-projects` answered 401 (the other directory's session state) where the named directory's policy says 403. Precedence is now most-specific-wins, the same rule `kbagent repl` applies when forwarding root flags into a subcommand: `serve --config-dir X` beats a root `kbagent --config-dir Y`, which beats KBAGENT_CONFIG_DIR / the .kbagent walk-up / global. Passing both is not an error. Only an explicit `cli-flag` source propagates -- for env-var/local/global, `create_app` resolves the identical directory itself, and forwarding those would pin a resolution made at a different moment. Tests cover all four flag combinations plus the env-var non-propagation, and one end-to-end case proving a deny policy in the root-flag directory reaches the REST surface. Docs updated on docs/web-server.md, CLAUDE.md, gotchas.md, commands-reference.md and the `kbagent context` text; the new behavior is gated `(since vNEXT)` for the release PR to resolve. * tests: correct the mechanism named in the /auth/status control comment The comment justifying the `!= 403` assertion said "there is no session in this fixture dir", implying a SESSION_NOT_FOUND / 401 path. Verified: the response is HTTP 400 CONFIG_ERROR -- the fixture dir has no default project and the request carries no `?stack=`, so `AuthService.status()` -> `_resolve_stack_url()` raises ConfigError before any session lookup happens. Comment rewritten to name that mechanism and to lead with the point it is there to make: `/auth/status` is gated by a different registry key (`auth.status`), which the deny policy does not name, so a non-403 proves the denial is narrow rather than a blanket failure of the whole REST surface. The assertion is unchanged -- it was correct. * ci(version-gates): scan src/**/*.py, so a placeholder cannot ship again The previous commit rewrote a `(since vNEXT)` that survived the 0.90.1 release, but not the reason it survived: check_version_gates.py resolved its file list from SCANNED_GLOBS, which listed markdown plus exactly one hand-picked Python file (commands/context.py). permissions.py was never read, so the placeholder was invisible to the very check built to catch it -- and the next feature PR writing a gate in a Python comment would have shipped the same defect. Fixing only the symptom leaves the hole. A version gate in a Python comment is agent-facing documentation exactly like a markdown one, so src/**/*.py is now scanned as a whole (it subsumes the context.py entry). Verified against the 0.90.1 tree: the widened scan reports permissions.py:29, the marker that shipped. Its first run on the current tree also surfaced a real, pre-existing defect -- `(since v0.26.1)` on the project members & invitations block. 0.26.1 never existed: it is not a CHANGELOG key, `git tag -l "v0.26*"` lists only v0.26.0, and `gh release view v0.26.1` reports no release. The feature shipped in 0.29.0 (CHANGELOG 0.29.0 describes the seven `project` member/invitation commands; the commit that added this very block is c7d7078 "feat(0.29.0): ... + member lifecycle", drafted as 0.26.1 and renumbered -- the docs were rewritten at release, the Python comment was not). Corrected to v0.29.0. scripts/*.py is deliberately left out and the constant now says why: this module has to NAME the placeholder it hunts (its --release usage line, the VNEXT_TOKEN constant), so scanning it self-flags forever. INLINE_CODE_RE also learns double-backtick spans. Python docstrings here use RST convention, so the first ``(since vNEXT)`` written in one would otherwise be a false positive. Measured: markdown residue is unchanged at 7, i.e. no live gate is lost to over-stripping. The double-backtick alternative comes first because alternation is left-biased -- the single-backtick pattern would match the empty span between the two opening backticks and leave the token exposed. Gate counts: 67 files / 476 markers -> 295 files / 529 markers. * docs(permissions): stop the http.* comment contradicting its own registry The block opened with "GET = read, mutating verbs = write" and then classified `http.delete` as destructive three lines below it. DELETE is a mutating verb, so a reader auditing the four keys against the stated rule hits a contradiction on the strictest one -- and this comment is the security rationale those keys are audited against. State the mapping that the registry actually implements, in the taxonomy declared at the top of the file (read = no side effects, write = creates/modifies, destructive = deletes): GET = read, POST/PATCH = write, DELETE = destructive. The extra rung is load-bearing, not cosmetic: apply_firewall_flags adds `cli:destructive`, which matches only the destructive class, so classifying http.delete as `write` would let a --deny-destructive session delete through the REST boundary. * fix(version-gates): pin UTF-8 when reading and writing scanned files The Windows job failed on the new test with UnicodeEncodeError: cp1252 is the platform default there, and the fixture mirrors a real repo line whose section rule is box-drawing characters. Reproduced locally -- write_text(body, encoding='cp1252') raises exactly the CI error, and the same bytes read back as cp1252 turn '--' rules into mojibake. The read side matters beyond the test. Widening SCANNED_GLOBS to src/**/*.py put 73 non-ASCII-carrying source files in front of a read_text() that never named an encoding, so on Windows the scan would regex over mangled text -- and since INLINE_CODE_RE keys on backticks, mojibake can move or destroy the very characters that separate a live placeholder from prose quoting one. A gate that classifies differently per OS is worse than no gate. Both read_text() calls in the script and every read/write in its tests now name UTF-8 explicitly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #537.
Exposes the non-interactive part of the
authgroup overkbagent serve, exactly per the issue's proposed scope, and introduces the first permission-policy enforcement seam on the serve path (AC #5).What's in
New REST endpoints (
server/routers/auth.py):GET /auth/projects?stack=auth.projects(read, new registry entry)POST /auth/register-projects— body{stack?, all?, project_ids?, aliases?}auth register-projects --all/--project-id/--aliasauth.register-projects(write)GET /auth/status?stack=auth statusauth.status(read)selectionsparameter (the picker's input) is not reachable over REST.login,login-passwordandlogoutdeliberately get no endpoints (browser required / password over REST / machine-wide revocation), per the issue's out-of-scope list; the router docstring records the rationale.kbc-session://sentinel — pinned by a test that serializes real result dataclasses and greps the wire bytes.SESSION_NOT_FOUND/SESSION_EXPIREDanswer 401 via the existing central mapping; theexists/skippedalias-protection statuses pass through unchanged;session_unsupported_featuressurvives the REST boundary (test-pinned).Permission seam (first serve router to enforce the policy):
create_appnow composes aPermissionEnginefrom the persisted policy of the config dir it actually serves, plusdeny_writes/deny_destructiveflags passed from the CLI (apply_firewall_flagsis now shared betweencli.pyand the server instead of being CLI-only). An explicitly passedpermission_engine=remains an embedder override.app.stateand therequire_permissiondependency fails closed — a test overridingget_registrycannot accidentally disable enforcement./auth/*routes are guarded so far; the other routers are unchanged (documented in gotchas.md so nobody assumes a deny policy firewalls the whole REST surface).SERVE_ONLY_OPERATIONSset inpermissions.py: registry keys with no CLI leaf command (currentlyauth.projects), subtracted only incheck_command_sync.py's dead-key check. Documented in CONTRIBUTING.md.Docs:
docs/web-server.md(endpoint section + accurate status-code semantics), CLAUDE.md, gotchas.md — all version-gated text uses the(since vNEXT)placeholder per the release process; no version bump, no changelog entry.Notes for review
Two things the issue's AC #5 didn't anticipate, surfaced during implementation (both verified live):
kbagent --deny-writes servenever starts —serveis an admin-class operation and--deny-writesdeniescli:write, which spans admin, so the CLI callback blocks the command itself. The docs therefore lead with the reachable forms (a persisted narrow policy such aspermissions set --mode allow --deny auth.register-projects, ormode=denywithservein the allow list) and state the--deny-writesinteraction explicitly. A test pins--deny-writes serve→EXIT_PERMISSION_DENIED.servehas its own--config-dir, distinct from the root flag. The engine is now composed against the served dir, so its persisted policy always applies (regression test for the divergent-dirs case). Pre-existing and untouched: the root-levelkbagent --config-dir X servedoes not propagateXinto serve's own resolution — filed separately as serve: root-level --config-dir is silently ignored (kbagent --config-dir X serve serves a different dir) #679 with a verified reproduction.One deliberate semantic to be aware of:
GET /auth/statusreports a missing/expired session as HTTP 200 with the health in the body'sstatusfield (live/refreshed/degraded/expired/missing) — it is the probe you call to find out, so it must not itself fail; clients that want to branch on the HTTP status must call/auth/projectsor/auth/register-projects(those 401).Relation to #655
#655 ("serve: PermissionEngine firewall is not enforced on REST routes") asks for exactly the mechanism this PR builds — option 1 in that issue: build the engine in
create_app()from the same config dir and enforce it via a FastAPI dependency. This PR implements it, but wires it to/auth/*only, so #655 stays open for the remaining ~30 routers; gotchas.md says so explicitly, so nobody reads a deny policy as a whole-surface firewall.The last commit also corrects the
http.*comment inpermissions.pythat #655 flagged as load-bearing and false — it claimed serve routes enforce their own permissions on top, which was true for no route before this PR and is true for/auth/*after it.Testing
tests/test_server_auth.py(kwarg parity,allalias, 401/4xx/403 mapping, token-free wire check, no-login-routes check),tests/test_server_permissions.py(seam: persisted policy, flag composition, fail-closed, served-dir-wins,--deny-writes serveblocked end-to-end).make checkgreen end-to-end: 6069 passed, 12 skipped;tyclean for all touched files;check_command_sync(264 commands),check_sentinel_guards,check_version_gates,loc-checkall OK.