Repository navigation
feat(cli): campus audit command group — API-key auth, keys, traces - #55
Conversation
campus.audit (/audit/v1) authenticates with its own API keys (audit_v1_... Bearer), not OAuth tokens, so this adds the CLI's second credential lane alongside the device-flow machinery: - campus audit login/logout: store/clear an audit key from --api-key, CAMPUS_AUDIT_API_KEY, or a prompt, in the shared credential store, bound to the audit URL it was stored for (endpoint-mismatch fails fast, like OAuth tokens). Login verifies the key against the service first (401 rejects; 403 counts as verified for trace-only keys; unreachable stores unverified) unless --skip-verify. - campus audit keys list/new/get/update/revoke/regenerate: the operator workflow for minting and managing per-student keys (--owner, repeatable --scope, --rate-limit, --expires-at). Plaintext key material prints exactly once, at creation/regeneration (mirroring the server, which keeps only the hash). - campus audit traces list/get/search: read-only trace queries with cursor pagination (--cursor continuation hint), gated on traces:read. URL resolution mirrors campus_python exactly: CAMPUS_AUDIT_URL, then the config file, then ENV/CAMPUS_ENV (development Railway default, staging/production domains). All API-backed commands support --dry-run with the audit preamble (AuditRoot + API-key bearer) and only show kwargs actually passed. Beyond the issue's proposed surface (login, keys list/new/get/revoke, traces list/get/search): logout for symmetry with auth logout, and keys update/regenerate to complete the lifecycle the issue summary describes (PATCH fields; rotation with immediate old-key death). The server's login-journey search filter is not exposed yet — campus-api-python's traces.search() does not carry it.
…t note, polish (#56, #57, #58, #59) Follow-up to the #55 review. - #56: the key status now distinguishes revoked / expired / active. A key with a future expiry is still active (the server counts it as such), so it no longer renders as "expires"; an expiry in the past renders as "expired". An unparsable expiry never counts as expired. - #57: credential-store failures in the audit group are reported and exit 1 instead of escaping as a traceback — the login write, the client key read, and the URL-binding read. - #58: `auth logout` names the still-stored campus.audit key and the command that clears it, so "logged out" does not read as "every stored credential is gone". - #59: no double full stop when the server's message is already a sentence, `--json` on `keys revoke`, `keys list --dry-run` shows only the arguments the caller passed, and the README leads with the env var / prompt for `campus audit login` and describes `owner_id` as a free-form label (the server accepts any string). Tests: 277 passing (18 new), ruff clean.
ngjunsiang
left a comment
There was a problem hiding this comment.
Reviewed the audit command group end to end: ran the full suite (277 passing, ruff clean), drove every command against a stub of the audit service, and cross-checked the client contract against the pinned campus-api-python (rev 89df17e) and the server's own routes and auth flow in nyjc-computing/campus.
The design holds up well — the URL binding, the 401/403 verification split (which matches the server's actual behaviour, not a guess), the once-only plaintext key printing via typer.echo, and the dry-run snippets all check out. The emitted snippets even run verbatim against a live stub.
Fixes pushed as 12b62eb (#56, #57, #58, #59): the key status now distinguishes expired from active, credential-store failures are reported instead of raising a traceback, auth logout names the still-stored audit key, and the small polish items (double full stop, --json on revoke, dry-run kwargs, README wording) are in.
Three items are left as issues, with the reasoning inline below.
| return parsed | ||
|
|
||
|
|
||
| def _key_status(record: dict) -> str: |
There was a problem hiding this comment.
Fixed in 12b62eb (#56). The status label used to come from the presence of expires_at alone, so a live key with a future expiry rendered as Status: expires and an expired key rendered identically — the operator could not tell a live key from a dead one without reading the date. _key_status now compares expires_at against the current time and reports revoked / expired / active, matching the server's APIKey.is_active(). An unparsable expiry is treated as active, not expired.
| description = error_obj | ||
| if description: | ||
| return status, str(description) | ||
| return status, str(e) |
There was a problem hiding this comment.
Open — #60. The str(e) fallback is empty for a campus_python APIError raised without an error_description, which is exactly what happens when the response body is not JSON (a proxy's HTML 502, an empty body, a 404 page from a misrouted deployment). The CLI then prints Failed to list API keys: . and drops the status code it already extracted a few lines up — the login warning shows the same empty parentheses. Falling back to the status (HTTP 502) would make these diagnosable; the same pass could surface the 422 field-level errors array, which the server sends and campus_python parses into APIError.errors but the CLI currently drops, leaving One or more fields are invalid with no indication of which field was rejected.
| keys = result.get("api_keys", []) | ||
|
|
||
| if output_json: | ||
| print_json(keys) |
There was a problem hiding this comment.
Open — #61. This prints just the array of key records, dropping the server's count, while campus audit traces list --json prints the whole server response including the pagination cursor. Two sibling commands, two JSON shapes — a script consuming both has to special-case one. Picking one shape for the group (the server response verbatim, as traces does) would settle it.
| audit_app = typer.Typer(help="Campus Audit service commands (API-key auth)") | ||
|
|
||
| # login/logout attach directly; keys and traces are sub-groups. | ||
| audit_app.command("login")(login_cmd) |
There was a problem hiding this comment.
Open — #62. campus auth status (#45) answers "what can this stored credential do?", and the audit group has no equivalent: a trace-only student key cannot run campus audit keys list (it lacks apikeys:read), so the stored key's state is only visible when a command fails. The audit service has no "who am I" endpoint, so this would have to be a local view — the URL the key is bound to, the URL currently targeted, and whether a key is stored. The information the URL-binding check already uses would be worth surfacing.
… status (#60, #61, #62) Address the three open review issues on #55. - #60: a non-2xx failure with no error_description (proxy HTML 502, empty body, misrouted 404 page) now renders as "HTTP <status>" instead of an empty sentence — the login warning included — so the one breadcrumb an operator can act on actually reaches the message. The server's 422 field-level errors array (parsed by campus_python into APIError.errors) is appended, so a rejected 'keys new' names the offending field instead of a bare "One or more fields are invalid". - #61: 'keys list --json' prints the server's {"api_keys": [...], "count": n} envelope verbatim, matching the traces commands' shape — one shape for the whole group, documented in the README (bodyless actions like revoke report {"id": ..., "revoked": true}). - #62: new 'campus audit status' — a purely offline local view of the stored key: whether one is stored, the URL it is bound to, the URL the CLI currently targets (an invalid ENV/CAMPUS_ENV renders as unknown rather than dying, since that is exactly the state worth diagnosing), and when the key was stored (new audit_key_stored_at in the credential store; keys stored before this report "unknown"). A bound/target mismatch is called out with the same sentence the commands' guard uses (factored into client.mismatch_message). Exit 1 only when no key is stored; a mismatch is informational, mirroring 'campus auth status'. Tests: 293 passing (16 new), ruff clean.
Closes #54.
Closes #56, #57, #58, #59 (review follow-up).
What
campus audit— the CLI's first non-OAuth command group. The auditservice (
/audit/v1) only accepts its own API keys (audit_v1_…Bearer), so this adds a second credential lane next to the device-flow
machinery, per the issue's use case: per-student minted keys for
dev-environment trace access, administered by the operator.
campus audit login/logout— store/clear an audit key from--api-key,CAMPUS_AUDIT_API_KEY, or an interactive prompt, in theshared credential store. Login verifies the key against the service
first (
--skip-verifyopts out): 401 rejects and stores nothing, 403counts as verified (valid key, no
apikeys:read— the trace-onlystudent case), unreachable stores unverified with a warning. Like
OAuth tokens, the key is bound to the URL it was stored for;
commands fail fast on a target mismatch with re-login as the remedy.
campus audit keys list/new/get/update/revoke/regenerate— theoperator lifecycle (
--owner, repeatable--scope,--rate-limit,--expires-at;--allto include revoked/expired). Plaintext keymaterial prints exactly once, at creation/regeneration, via a plain
typer.echoso Rich never wraps the token.campus audit traces list/get/search— read-only queries(
traces:read), cursor-paginated; the continuation prints the exact--cursortoken to re-run with.getprints the full span tree asJSON.
Notes
logout(symmetry withauth logout; local-only — the help says aleaked key is disabled server-side via
keys revoke) andkeys update/regenerate(the issue summary's PATCH/rotationlifecycle; rotation kills the old value immediately and confirms
first).
CAMPUS_AUDIT_URL→ config file
audit_url→ENV/CAMPUS_ENV(development Railwaydefault;
audit.campus.nyjc.dev/.app). No/audit/v1prefix —AuditRoot.url_prefixcarries it.non-2xx responses to
errors.APIError(status + description), rawtransport failures stay
requestsexceptions. 401/403 responses getstanding remedy hints (re-login / widen the key).
--dry-runworks on every API-backed audit command with anaudit-specific preamble (
AuditRoot+ API-key bearer) and shows onlykwargs actually passed, matching the client/vault convention.
(campus#803) — campus-api-python's
traces.search()does not carryit yet; noted in the command help. Web counterpart remains
campus#915.
Tests
pytest),ruffclean (259 before the reviewfollow-up; 18 new tests).
tests/unit/test_audit.py): URL resolutionprecedence, endpoint binding, key input resolution, verification
semantics per status, login/logout end-to-end, trace formatting,
credential round-trip, APIError/HTTPError extraction.
preamble, omit unpassed fields, compile, and never touch the client;
--dry-rundocumented in both new groups' help.Review follow-up (12b62eb)
A review pass drove every command against a stub of the audit service and
cross-checked the client contract against the pinned campus-api-python and
the server's own routes and auth flow. The fixes that came out of it:
key with a future expiry is still active (the server counts it as such),
so it no longer renders as
expires; an expiry in the past renders asexpired.exit 1 instead of escaping as a traceback (the login write, the client
key read, the URL-binding read).
auth logoutshould mention the stored campus.audit key #58 —campus auth logoutnames the still-stored campus.audit keyand the command that clears it, so "logged out" does not read as "every
stored credential is gone".
--jsonon revoke, dry-run kwargs, README wording #59 — polish: no double full stop when the server's message isalready a sentence,
--jsononkeys revoke,keys list --dry-runshows only the arguments the caller passed, and the README leads with
the env var / prompt for
campus audit loginand describesowner_idas a free-form label.
Three findings are left as issues rather than fixed here, with the
reasoning inline on the diff: #60 (audit error messages drop the HTTP
status when the response carries no description), #61 (the
--jsonoutput shapes differ between
keys listandtraces list), and #62(no
campus audit statusequivalent tocampus auth status).Relates to campus#915 (web sign-in counterpart).