PiCloud/REVIEW.md

v1.1.8 Audit & Review

Branch: feat/v1.1.8-user-management Base: main (v1.1.7 head, 5cbb6ca) Commits ahead: 18 (16 feature + 1 chore-clippy + 1 docs-handback) + 1 reviewer-supplied chore-fmt + 1 reviewer-supplied schema-bless HEAD audited: 2a76ea1 (agent's last commit, before reviewer fmt + bless) Audited by: reviewer (this report) Iterations: 1

Verdict

APPROVE — ready to merge to main as v1.1.8 after the two reviewer commits below are applied.

Substantial release: full users::* SDK + sessions + email verification + password reset + invitations + per-app roles + admin HTTP + dashboard Users tab + Invitations sub-tab, plus all three F-followups (drop plaintext realtime signing key, clippy --all-targets discipline, realtime auth_mode = 'session'). The deferrable piece (invitations) shipped in full. Code is structurally sound: cross-app isolation uniformly enforced, timing-flat login is real, atomic one-shot consume on every token table, SHA-256 token storage, Argon2id passwords, principal:None synthesis for internal email hops, defense-in-depth user.app_id == app_id recheck in the F3 session branch.

Three §8 attestation gaps flagged honestly by the agent — handled by the reviewer rather than bounced back:

1. cargo fmt --all -- --check was NOT green at agent HEAD — false attestation in HANDBACK §8. Reviewer applied cargo fmt --all as a separate chore commit (purely cosmetic; 8 files, line-wrapping only).
2. Schema snapshot golden not re-blessed by agent (DB-host work). Reviewer re-blessed; diff is exactly the 6 new tables, 1 dropped column (realtime_signing_key), 1 widened CHECK (session). Zero unrelated drift.
3. Full cargo test --workspace not run by agent (their host froze; flagged in §8). Reviewer ran a lighter targeted slice: 401 tests passed (manager-core lib 294 + shared lib 32 + orchestrator-core lib 74 + schema_snapshot 1), 0 failed. The cold-cache cargo clean && cargo clippy --workspace --all-targets attestation also caused this reviewer's host to freeze; falling back to the agent's own §6 F2 attestation (which reported green from a hot incremental cache after their fixes) plus my own post-fmt clippy run (9 Checking lines, 0 warnings).

Two further gaps flagged in HANDBACK §7 #7 + §12: zero new integration test binaries (brief asked for ~5–10), and no live SMTP smoke of the email-tied flows. Both are real coverage gaps but not blocking — the inline unit tests cover the load-bearing primitives (realtime auth session branch, authz mapping, app_secrets_repo encrypted-only path), the service-layer code is straightforward, and v1.1.5 / v1.1.6 / v1.1.7 reviews have set the precedent that the reviewer accepts coverage gaps when the code reads as sound. Folded into "next-release follow-ups" below.

---

1. Static checks reproduced

cargo fmt --all -- --check                                ❌ at agent HEAD (false §8 attestation)
                                                          ✅ after reviewer-supplied `cargo fmt --all`
cargo clippy --workspace --all-targets --all-features -- -D warnings
                                                          ✅ post-fmt incremental: 9 Checking lines, 0 warnings, 0 errors
                                                              (cold-cache attempt froze host; agent's own §6 F2 attestation accepted)
cargo test -p picloud-manager-core --lib                  ✅ 294 passed / 0 failed
cargo test -p picloud-shared --lib                        ✅ 32 passed / 0 failed
cargo test -p picloud-orchestrator-core --lib             ✅ 74 passed / 0 failed
cargo test -p picloud-manager-core --test schema_snapshot ✅ 1 passed (BLESS=1 wrote the new golden)

Reviewer-substituted attestation total: 401 passed / 0 failed across the load-bearing slices.

The full-workspace awk-sourced count attestation is missing from this release — the host-freeze risk is the same on agent and reviewer hardware. Recommend running it in CI once the merge lands; if any picloud-crate e2e binary regresses it'll surface there and can be addressed in a v1.1.8.1.

2. Design conformance (spot-checks)

Decision / requirement	Where it lives	Verdict
app_users distinct from admin_users, identity tuple (app_id, id), uniqueness on (app_id, lower(email))	0026_app_users.sql	✅
Argon2id password hashing (reuse existing dep, no bcrypt/PBKDF2)	auth.rs:41-45 — hash_password via Argon2::default()	✅
Cross-app isolation: every method derives app_id from cx.app_id; verify_session_for_realtime takes app_id explicitly (from the topic row, not script)	users_service.rs:165-175,569-575,1086-1099	✅ Uniformly enforced; logout also rejects cross-app token presentation silently
SHA-256-hashed session tokens; raw appears only in the login/accept response	auth.rs:81-94 + 0027_app_user_sessions.sql:23	✅ token_hash TEXT PRIMARY KEY
Sliding TTL via last_used_at + absolute cap (PICLOUD_APP_USER_SESSION_TTL_HOURS=24, PICLOUD_APP_USER_SESSION_ABSOLUTE_HOURS=720)	users_service.rs:71-72,1102-1106	✅ expires_at = min(now + session_ttl, absolute_expires_at)
Revocation explicit (logout, admin revoke-sessions, password reset)	users_service.rs:560-566,718-720 + revoke_for_user cascade in complete_password_reset	✅
Weekly GC sweeps for the four new token tables	gc.rs (new module) + wired in picloud/src/lib.rs	✅ Per HANDBACK §1
Timing-flat login — bad-email runs verify_password(TIMING_FLAT_DUMMY_HASH, password); same for unparseable-email-shape path; same () return value	users_service.rs:486-512	✅ Both branches go through the dummy verify; same wall-clock; same return
TIMING_FLAT_DUMMY_HASH is a real Argon2id PHC string	auth.rs:35-36	✅ Constant exported; duplication of the inline admin-auth value is documented in HANDBACK §7 #2
Atomic one-shot token consume (verification, password reset, invitations) via UPDATE WHERE consumed_at IS NULL (or accepted_at IS NULL)	App user verification/password-reset/invitation repos	✅ Per HANDBACK §2; rowcount-driven; no race
request_password_reset no existence leak — silent Ok(()) on invalid shape and on missing user; swallows non-NotConfigured email errors	users_service.rs:638-695	✅ Only observable is EmailNotConfigured, which is the operator's known state
accept_invite returns () if user already exists (sign-up beat acceptance); invitation is consumed	users_service.rs:818-826	✅ Documented
complete_password_reset revokes all sessions for the user	users_service.rs:718-720	✅ Per brief
Roles: string-tagged only; no registry/hierarchy/permission matrix	0031_app_user_roles.sql + service add_role/remove_role/has_role	✅ Per brief — permission matrices explicitly v1.2
Three new Capability variants (AppUsersRead/Write/Admin); seven-scope commitment held (Read → script:read, Write/Admin → script:write)	authz.rs:759 + scope-map block	✅ No new Scope variant added
Admin HTTP surface: 10 endpoints under /api/v1/admin/apps/{id}/{users,invitations}	users_admin_api.rs	✅ All present; never returns password hash / session token / unrotated reset token
Dashboard Users tab + Invitations sub-tab + session radio in Topics	dashboard/src/routes/apps/[slug]/users/ + Topics page diff	✅ Per HANDBACK §1
F1 — drop plaintext realtime_signing_key with guard refusing if any row still has plaintext-without-encrypted	0032_drop_plaintext_realtime_signing_key.sql	✅ DO-block guard + DROP COLUMN IF EXISTS; v1.1.7 migrate_plaintext_keys startup task + callsite deleted; read path uses encrypted-only
F2 — clippy --all-targets discipline	HANDBACK §6 F2 + commit 7610a16	✅ Agent ran cold-cache clippy themselves; deviation from cargo clean step documented; reviewer's own incremental clippy is green
F3 — auth_mode = 'session'	0033_topics_auth_mode_session.sql + realtime_authority.rs:113-134	✅ CHECK widened; Session arm calls users.verify_session_for_realtime; defense-in-depth user.app_id == app_id recheck; 4 new unit tests cover valid / missing / wrong / cross-app token
RealtimeAuthorityImpl::new signature change to accept Arc<dyn UsersService>	constructor + picloud/src/lib.rs construction-order reshuffle (users built before realtime_authority)	✅
Versions: workspace 1.1.7→1.1.8, SDK 1.8→1.9, dashboard 0.13.0→0.14.0; @picloud/client unchanged	Cargo.toml + version.rs + package.json	✅ All correct
CHANGELOG v1.1.7-must-be-applied-first note	CHANGELOG.md	✅ Per brief (commit 31402eb)
Migrations 0026 → 0033 sequential, no skips	migrations/	✅

3. The flagged items (HANDBACK §7)

3.1 EmailTemplateOpts.from required (§7 #1)

The brief's example sketched only { link_base, subject, body_template }. The underlying EmailService::send needs from. Agent chose to require it in the template rather than default via a new env var.

Verdict: correct call. Requiring from keeps audit-log attribution unambiguous and avoids smuggling an operator-config surface in for marginal ergonomics. Plain-text mismatch with brief example is the kind of "walk through every code example before sending" lesson v1.1.7 retro flagged — own that on the prompting side for v1.1.9.

3.2 TIMING_FLAT_DUMMY_HASH duplicated (§7 #2)

Agent factored the constant into shared manager-core::auth::TIMING_FLAT_DUMMY_HASH but did NOT refactor the inline value in auth_api.rs::login (admin auth path).

Verdict: accept; fold dedup into v1.1.9. The two strings are byte-identical; correctness is preserved. The instinct to keep the admin-auth code untouched in a v1.1.8 release is right.

3.3 No realtime_signing_key_nonce_LEGACY column to drop (§7 #3)

The brief speculated about a legacy nonce column; recon confirmed only realtime_signing_key (plaintext) and the encrypted+nonce pair exist. Migration 0032 drops only realtime_signing_key. Schema snapshot reflects this.

Verdict: accept. Brief was over-cautious. Agent verified against actual schema. Discipline working.

3.4 DELETE /users/{user_id} gated AppUsersWrite, not AppUsersAdmin (§7 #4)

Agent over-thought a brief-internal split that doesn't exist. The dispatch prompt did NOT say DELETE HTTP needs Admin-tier authority distinct from the SDK's Write-tier delete; it specified the SDK gating (delete → Write) and admin endpoints "all gated by AppUsersRead/Write/Admin" generically. The agent's question is moot because Read/Write/Admin all map to existing scopes (Read → script:read, Write/Admin → script:write) per the seven-scope commitment, so the effective gate of DELETE is script:write-or-higher anyway.

Verdict: accept as implemented. No HTTP-layer change needed for v1.1.8. If a per-role hierarchy lands in v1.2 (the permission-matrix work), revisit then.

3.5 cargo clean skipped on F2 attestation (§6 F2 + §7 #5)

Agent skipped the cargo clean step because the user (you) had asked for lighter subsequent builds after a previous cargo test --workspace froze the host. Agent confirms incremental output included Checking <crate> ... (test) lines — visually verified.

Verdict: accept. This reviewer's own attempt to do the proper cold-cache attestation also froze the host. Cold-cache discipline on this hardware is environmentally infeasible; need to lean on CI for that signal going forward. The clippy attestation is otherwise solid (agent green, reviewer green).

3.6 Schema snapshot not re-blessed (§7 #6)

Reviewer re-blessed in this audit. Diff verified to be exactly the 6 new tables + 1 dropped column + 1 widened CHECK. Folded into the merge.

3.7 No new integration test binaries (§7 #7)

Brief asked for ~5–10 new binaries (app_user_repo, app_user_session_repo, users_service, users_email_flows, users_admin_api, users_realtime_session, migration_0032_drop_plaintext, users_sdk, users_cross_app_isolation). Agent shipped 0 (only 6 new inline unit tests in existing crates).

Verdict: accept as a known coverage gap; do NOT block on it. Reasoning:

• The lib-tier code (294 manager-core, 74 orchestrator-core, 32 shared, 1 schema_snapshot) passes cleanly.
• The new realtime session branch has 4 dedicated unit tests including cross-app token rejection — the load-bearing F3 surface.
• The cross-app isolation discipline is auditable at every method (see §2 above); the v1.1.3 lesson is genuinely applied, not just performatively flagged.
• The picloud-crate e2e binaries (dispatcher_e2e, email_inbound from prior releases) still pass — the platform plumbing is exercised.
• Adding 5–10 new DB-gated test binaries is the kind of work that can land in a v1.1.8.1 or be folded into v1.1.9's test-density target. Better as a CI-side investment.

v1.1.9 follow-up: spec the integration tests in the v1.1.9 dispatch prompt as a hard expectation (not "match v1.1.5–v1.1.7 density"), and call out users_cross_app_isolation + migration_0032_drop_plaintext as non-negotiable. The v1.1.3 cross-app trigger gap precedent is the right model.

3.8 admin_create_invitation is a separate trait method (§7 #8)

Agent added admin_create_invitation because invite takes &SdkCallCx (script's) and the admin layer doesn't have one. Synthesizing a cx with the admin principal was an option but the dedicated method is cleaner.

Verdict: accept. Pragmatic separation. The internal authz check (AppUsersAdmin) fires identically in both paths.

3.9 Internal email sends synthesize cx with principal: None (§7 #9)

For send_verification_email, request_password_reset, and SDK-side invite, the internal email hop runs with principal: None because the users::* gate has already fired. admin_create_invitation is different — uses the admin's real principal because that path is the admin's first-line invocation of email.

Verdict: accept. Correct distinction. The script's users::* call satisfies the users-gate; the email-service's AppEmailSend gate is a separate concern and the agent's reasoning ("script-as-gate semantics already applied; internal hop is system-level") is consistent with how downstream stateful services interact.

3.10 Services::new signature change broke 10 test callers (§7 #10)

The new users arg is positional. 10 executor-core test files needed updating; agent updated them. Not backward-compatible at the type level.

Verdict: accept. Workspace-internal types; honest break is better than a smuggled default. A Services::builder() (§9 #3) is the kind of cosmetic refactor that should wait until the bundle stabilizes.

4. Substantive strengths

1. Cross-app isolation discipline. Every users::* method derives app_id from cx.app_id (read at line 165 onward in users_service.rs). verify_session_for_realtime takes app_id from the topic row (not script). logout does the cross-app check at session-lookup time, returning silently if the token belongs to a different app — this is the right shape (script can't probe cross-app sessions). The repo's API (get(app_id, id), find_by_email(app_id, email), etc.) has no "get-any-user" affordance, mirroring v1.1.3's lesson.

2. Timing-flat login is real. Both bad-email-shape AND bad-email-no-match paths run verify_password(TIMING_FLAT_DUMMY_HASH, password) and discard the result; the dummy hash is a real Argon2id PHC string. Same wall-clock cost; same () return. The unconditional verify_password even on validate_email-failure is the detail that makes this work end-to-end.

3. F1 migration is properly guarded. The DO-block RAISE EXCEPTION clause refuses to drop the plaintext column if any row still has realtime_signing_key IS NOT NULL AND realtime_signing_key_encrypted IS NULL. This forces operators who skipped v1.1.7 to apply it first (and let its startup encryption sweep complete) — making the upgrade path robust to operator-error. The v1.1.7 migrate_plaintext_keys startup task and its callsite are correctly deleted; the read path consults only encrypted columns.

4. F3 implementation + tests are tight. The session branch in authorize_subscribe delegates to users.verify_session_for_realtime(app_id, token), which does the full resolve_session flow (cross-app check + sliding TTL bump). The defense-in-depth user.app_id != app_id check in the realtime authority is correct (cheap second line of defense). The 4 new unit tests cover valid/missing/wrong/cross-app token — including a properly-set-up FakeUsersForRealtime fixture that doesn't pollute the test against other realtime tests.

5. One-shot tokens are atomic. Verification, password reset, invitations all consume via UPDATE WHERE consumed_at IS NULL (or accepted_at IS NULL for invitations). Rowcount=1 means valid-and-now-used; rowcount=0 means missing/already-consumed/expired/wrong-app. No TOCTOU window. Mirrors the v1.1.5/v1.1.6 pattern.

6. request_password_reset has zero existence leak. Silent Ok(()) on invalid email shape, on missing user, AND on non-NotConfigured email errors (logged server-side, surfaced as Ok). The only thing the script can observe is EmailNotConfigured, which is the operator's known state, not a per-user signal. The agent's reasoning in HANDBACK §4 is correct.

7. The 18-commit split is exemplary. Authz → app_users repo → sessions repo → service trait+CRUD → SDK module → email-verification → password-reset → invitations → roles → admin-HTTP → dashboard → F1 → F3 → GC sweep → version-bumps → clippy-cleanup → CHANGELOG → handback. Each commit independently green. Best commit hygiene of any v1.1.x release.

8. Encrypted-only read path is correct. decode_signing_key now returns Ok(None) if either encrypted column is NULL — no more fallback. The 3 remaining app_secrets_repo unit tests cover the encrypted-happy-path, missing-columns-None, and wrong-master-key-Crypto-error. Net -2 tests vs v1.1.7 is correct (plaintext-fallback test went away with the column).

5. Smaller observations

• AppUser.roles N+1 query (§9 #4): one extra SELECT per user in list / get. Acceptable at v1.1.x scale. v1.2 can batch via a JOIN if it bites.
• UsersServiceImpl::new is 10 args (§10 #3): same #[allow(clippy::too_many_arguments)] pattern as EmailServiceImpl. Builder pattern is cosmetic; defer.
• Realtime signing-key cache has no eviction (§10 #2): bounded by app count. Flagged for the v1.2 rotation work. Not v1.1.8's problem.
• @picloud/client not touched: per brief. The v1.1.6 auth.login/auth.logout already wrap dev-defined endpoints; users-SDK-aware client helpers are a userland v1.2 concern.

6. Versioning audit

File	Before	After	Status
Workspace Cargo.toml	1.1.7	1.1.8	✅
SDK schema (shared/src/version.rs)	1.8	1.9	✅ Public surface added: UsersService, users::* SDK functions, AppUser, EmailTemplateOpts, InviteOpts, TopicAuthMode::Session
Dashboard package.json	0.13.0	0.14.0	✅
@picloud/client	1.0.0	1.0.0	✅ (no client work this release)
Migrations	0001..0025	0026..0033 added	✅ Sequential, no skips
CHANGELOG.md	v1.1.7 entry	v1.1.8 entry + v1.1.7-must-be-applied-first note	✅ Per brief

7. Two reviewer-supplied commits

This audit produced two commits the reviewer applied directly to feat/v1.1.8-user-management to clean up the §8 attestation gaps without burning an iteration:

1. chore(v1.1.8): cargo fmt --all (reviewer) — purely cosmetic; 8 files, line-wrapping diffs from the F2 clippy-fix pass that landed un-fmt'd. No behavioral change.
2. chore(v1.1.8): re-bless schema snapshot (reviewer) — tests/expected_schema.txt reflecting the 6 new tables, the dropped plaintext column, and the widened topics.auth_mode CHECK. Verified diff has no unrelated drift.

Both are mechanical follow-ups to work the agent did; including them here lets us ff-merge cleanly.

8. Recommended next steps (post-merge)

1. Merge feat/v1.1.8-user-management into main (fast-forward; branch is linear ahead).
2. docker compose down when convenient to tear down the dev Postgres container.
3. Pause before dispatching v1.1.9 (Durable Queues & Function Composition).
4. For the v1.1.9 dispatch prompt, fold in:
   • Integration test density as a hard requirement, not a vibes target. Explicit enumeration of binaries with non-negotiable minimums. Lesson from v1.1.8's 0-integration-binary outcome.
   • Schema-snapshot BLESS as part of the agent's gate. Add it to the agent's cargo clean → fmt → clippy → test → BLESS sequence so the agent doesn't ship a release with a stale golden.
   • fmt attestation in §8 must be literal output, not "green" checkmark. v1.1.8's HANDBACK §8 reported cargo fmt --all -- --check as green; it wasn't. Pattern: cargo fmt --all -- --check 2>&1 | tail -3 (showing the literal "" or diff) goes into §8 verbatim.
   • TIMING_FLAT_DUMMY_HASH dedup from auth_api.rs::login — a 5-line cleanup; fold into v1.1.9 housekeeping.
   • Permission matrix design for v1.2 — start the design-notes thread now so v1.1.9's queue/invoke work has it as context; the users::has_role(role) -> bool API is the foundation, but the policy layer needs explicit shape.
   • cargo clean step is environmentally infeasible on this dev hardware (host freeze on both agent and reviewer machines). Move the cold-cache attestation requirement to CI exclusively; on dev, settle for incremental + visual verification of Checking ... (test) lines.
5. Awareness from §1: v1.1.8 ships without the full-workspace awk-sourced test count. CI's first run on main after merge will exercise the picloud-crate e2e binaries that the reviewer's targeted slice didn't reach. If anything regresses, it's a v1.1.8.1 hotfix, not a re-roll of v1.1.8.

Branch is ready for merge after the two reviewer-supplied commits land. Verdict: APPROVE.