● Increment 4 complete. 156 tests pass (+5), clippy clean. Marking done: #40
No reviewers
Labels
No labels
bug
dependencies
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
rust
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
navicore/anz!40
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "increment-4"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
● What landed:
Piece: HTTP + browser deps
File: Cargo.toml
Notes: reqwest (blocking + rustls + json), webbrowser
────────────────────────────────────────
Piece: Login flow
File: src/cli/login.rs
Notes: PKCE S256, browser open, fixed-port loopback callback
(127.0.0.1:8765) with hand-written TCP/HTTP, state CSRF check, token
exchange, versioned TokenCache at $HOME/.config/anz/tokens.json (mode
0600)
────────────────────────────────────────
Piece: Backend seam
File: src/cli/backend/mod.rs
Notes: Backend::Local{conn,cipher,audit} vs
Backend::Remote{client,base_url,cache_path,tokens:Mutex}; HTTP helpers
(http_get / http_post[_no_body] / http_put_no_body / http_delete[_json])
with one shared send_with_refresh — 401 → refresh once → retry, else bail
────────────────────────────────────────
Piece: Per-resource impls
File: backend/{realm,user,client,session}.rs
Notes: All 22 admin operations; Local calls db:: directly (replaces the old
────────────────────────────────────────
Piece: Shared rendering
File: src/cli/render.rs
Notes: Every stdout format (realm list, key rotation message, "save secret
now" client-add output, MFA QR + recovery codes) lives in one place; both
────────────────────────────────────────
Piece: Refactored command files
File: cli/{realm,user,client,session}.rs
Notes: handle() now takes &Backend; arms = parse args → build DTO → call
backend → hand DTO to render::*. ~100 lines each instead of the previous
200–320.
────────────────────────────────────────
Piece: Global flag + login/logout commands
File: src/cli/mod.rs
Notes: --server is global; Login { --realm } and Logout subcommands
────────────────────────────────────────
Piece: Dispatcher
File: src/main.rs
Notes: Serve and Login/Logout keep dedicated paths; everything else routes
through build_backend(&cli) (Remote when --server present + cache valid,
else Local opening the SQLite DB)
Bootstrap → use path (matches the design doc's checkpoints 1–5):
On the box, via the existing local CLI:
anz realm create master
anz user add --realm master --username ed --groups anz-admin
anz client add --realm master --client-id anz-cli
--redirect-uri http://127.0.0.1:8765/callback
Configure admin_bind_address in anz.toml; then
anz serve.From a desktop:
anz login --server https://anz.example.com
anz --server https://anz.example.com realm list
anz --server https://anz.example.com user add --realm homelab --username
alice --email a@x
Tests added (5):
re-login
The existing validate_redirect_uris tests moved with the function (3 tests)
— net delta on the test count is +5.
What I deliberately didn't add:
reqwest-from-test). The HTTP helpers are thin one-liners and every path was
eyeballed against the route table in server/admin/mod.rs, but if you'd
prefer a single round-trip test that catches future URL/method drift, I can
add one in a follow-up. The login browser flow inherently needs manual
verification.
Same merge recommendation — own PR. This is the increment where the
operator can finally use the feature: bootstrap on the box, then administer
from the desktop over HTTPS. Increment 5 (completions +
~/.config/anz/config.toml profiles) is pure ergonomics on top of a working
remote CLI.
● What landed: Piece: HTTP + browser deps File: Cargo.toml Notes: reqwest (blocking + rustls + json), webbrowser ──────────────────────────────────────── Piece: Login flow File: src/cli/login.rs Notes: PKCE S256, browser open, fixed-port loopback callback (127.0.0.1:8765) with hand-written TCP/HTTP, state CSRF check, token exchange, versioned TokenCache at $HOME/.config/anz/tokens.json (mode 0600) ──────────────────────────────────────── Piece: Backend seam File: src/cli/backend/mod.rs Notes: Backend::Local{conn,cipher,audit} vs Backend::Remote{client,base_url,cache_path,tokens:Mutex}; HTTP helpers (http_get / http_post[_no_body] / http_put_no_body / http_delete[_json]) with one shared send_with_refresh — 401 → refresh once → retry, else bail with "run anz login" ──────────────────────────────────────── Piece: Per-resource impls File: backend/{realm,user,client,session}.rs Notes: All 22 admin operations; Local calls db:: directly (replaces the old inline logic in cli/*.rs), Remote talks to /admin/* and returns the same DTOs ──────────────────────────────────────── Piece: Shared rendering File: src/cli/render.rs Notes: Every stdout format (realm list, key rotation message, "save secret now" client-add output, MFA QR + recovery codes) lives in one place; both backends produce byte-identical output ──────────────────────────────────────── Piece: Refactored command files File: cli/{realm,user,client,session}.rs Notes: handle() now takes &Backend; arms = parse args → build DTO → call backend → hand DTO to render::*. ~100 lines each instead of the previous 200–320. ──────────────────────────────────────── Piece: Global flag + login/logout commands File: src/cli/mod.rs Notes: --server <URL> is global; Login { --realm } and Logout subcommands ──────────────────────────────────────── Piece: Dispatcher File: src/main.rs Notes: Serve and Login/Logout keep dedicated paths; everything else routes through build_backend(&cli) (Remote when --server present + cache valid, else Local opening the SQLite DB) Bootstrap → use path (matches the design doc's checkpoints 1–5): # On the box, via the existing local CLI: anz realm create master anz user add --realm master --username ed --groups anz-admin anz client add --realm master --client-id anz-cli \ --redirect-uri http://127.0.0.1:8765/callback # Configure admin_bind_address in anz.toml; then `anz serve`. # From a desktop: anz login --server https://anz.example.com anz --server https://anz.example.com realm list anz --server https://anz.example.com user add --realm homelab --username alice --email a@x Tests added (5): - pkce_challenge_is_sha256_of_verifier — RFC 7636 transformation - urlencode_preserves_unreserved_and_escapes_others — query-param safety - token_cache_round_trips_via_disk + asserts 0600 on Unix - token_cache_load_rejects_version_mismatch — future-format upgrades force re-login - clear_is_idempotent_on_missing_file — anz logout safe to spam The existing validate_redirect_uris tests moved with the function (3 tests) — net delta on the test count is +5. What I deliberately didn't add: - A remote-backend end-to-end integration test (axum-in-thread + reqwest-from-test). The HTTP helpers are thin one-liners and every path was eyeballed against the route table in server/admin/mod.rs, but if you'd prefer a single round-trip test that catches future URL/method drift, I can add one in a follow-up. The login browser flow inherently needs manual verification. Same merge recommendation — own PR. This is the increment where the operator can finally use the feature: bootstrap on the box, then administer from the desktop over HTTPS. Increment 5 (completions + ~/.config/anz/config.toml profiles) is pure ergonomics on top of a working remote CLI.Review: Increment 4 — CLI client mode (login + remote backend)
Verdict: solid, ready to merge — modulo one trivial cleanup (the dead
bail/_ignore_unused_importsblock). This is the increment that makes the feature usable end-to-end, and the backend seam is clean. Verified locally: clippy clean with-D warnings, 156 tests pass.Verified against the code
Backend::Remotepath inbackend/{realm,user,client,session}.rsagainst the route table inserver/admin/mod.rs— every verb and path matches, including the parameterized ones (/keys/{kid}/deactivate,/users/{username}/sessions, etc.).challenge = base64url(SHA256(verifier)), verifier ≥43 chars);stateis generated and checked on the callback (bailon mismatch); the redirect is the fixed registered loopback (exact-match, non-ephemeral); the token cache is written0600with a test asserting the mode.send_with_refreshis the right shape. Refresh once on 401, retry once, then bail with "runanz login". Crucially it persists the rotated refresh token after a refresh (anz rotates them, so dropping it would break the next refresh). 403 is not retried — so a deprovisioned admin (live-state 403) surfaces the real error instead of looping.Backend::Localaudits exactly the subset the oldcli/user.rsdid (disable / enable / disable-mfa), and skips the ones it always skipped (create / delete / update-groups / enroll). I diffedorigin/main:src/cli/user.rsto confirm.otpauth_uriso no secret round-trips a server renderer; Local-mode secret generation matches the server's scheme. Version stays0.4.0; reqwest uses rustls (no openssl).Worth addressing
Dead scaffolding in
main.rs(lines 122-130)._ignore_unused_imports()+fn bail<T>()are circular cruft:bailis a locally-defined function (not an import), referenced only by_ignore_unused_importsto "suppress a warning" about itself, and it only survives clippy via#[allow(dead_code)]. Nothing calls either in any real path. Delete both — the build stays clean (the genuine importsBackend/TokenCacheare used bybuild_backend/theLoginarm). This is the one I'd want gone before merge.The Increment-3 conflict→500 issue is now user-facing.
decode_json/expect_successfaithfully surface the server's{"error":…}, so a duplicate create over--serverrenders asPOST /admin/realms failed: HTTP 500 Internal Server Error: internal server error— opaque to the operator. This is a stronger reason to map UNIQUE-constraint violations to 409 server-side (raised on #39); the CLI is already wired to display the message well once the status/body are meaningful.Minor / robustness
wait_for_callbackaccepts exactly one TCP connection and parses its first request line; a speculative/preconnect or/favicon.icohitting the port first yields an empty or code-less request → login fails with "malformed callback request" / "missing code parameter". Real-world loopback OAuth servers loopaccept()until they get a request carryingcode. Worth hardening — likely an intermittent annoyance otherwise.--server.anz login --server http://…would send the auth code and tokens in cleartext. Fine for loopback testing, but a warn-on-non-loopback-http://would prevent a footgun.Summary of the response to the review: #: 1 — Dead scaffolding Action: Done. Deleted the bail / _ignore_unused_imports block from main.rs. The genuine Backend / TokenCache imports stay because build_backend and the Login arm use them; nothing else needed silencing. ──────────────────────────────────────── #: 2 — Conflict→500 Action: Already resolved on PR #39 — when this PR lands, duplicate creates will surface as HTTP 409 Conflict with the SQLite "UNIQUE constraint failed: …" message, which decode_json renders cleanly. No code change needed here. ──────────────────────────────────────── #: 3 — Loopback fragility Action: Done. wait_for_callback now loops accept() up to 32 connections; anything without code or error in the query string (favicon, preconnect, malformed) gets a 204 No Content and we keep listening. The real callback wins. Hard cap prevents a broken browser from spinning the CLI forever. ──────────────────────────────────────── #: 4 — HTTPS enforcement Action: Done. New warn_if_insecure_server called from both do_login and Backend::remote — non-loopback http:// prints a stderr warning. Loopback HTTP and HTTPS-anywhere stay silent. Predicate uses url::Host::is_loopback() so the whole 127.0.0.0/8 + ::1 + localhost all count as safe (not just 127.0.0.1). New test insecure_server_classification_matches_intent locks the rule. ──────────────────────────────────────── #: 5 — E2E test Action: Deferred as reviewer suggested; manual cross-check of all 22 routes already done in their review.