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](crates/manager-core/migrations/0026_app_users.sql) | ✅ |
| Argon2id password hashing (reuse existing dep, no bcrypt/PBKDF2) | [auth.rs:41-45](crates/manager-core/src/auth.rs#L41-L45) — `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](crates/manager-core/src/users_service.rs) | ✅ 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](crates/manager-core/src/auth.rs#L81-L94) + [0027_app_user_sessions.sql:23](crates/manager-core/migrations/0027_app_user_sessions.sql#L23) | ✅ `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](crates/manager-core/src/users_service.rs) | ✅ `expires_at = min(now + session_ttl, absolute_expires_at)` |
| Revocation explicit (logout, admin revoke-sessions, password reset) | [users_service.rs:560-566,718-720](crates/manager-core/src/users_service.rs) + `revoke_for_user` cascade in `complete_password_reset` | ✅ |
| Weekly GC sweeps for the four new token tables | [gc.rs](crates/manager-core/src/gc.rs) (new module) + wired in [picloud/src/lib.rs](crates/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](crates/manager-core/src/users_service.rs#L486-L512) | ✅ 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](crates/manager-core/src/auth.rs#L35-L36) | ✅ 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](crates/manager-core/src/users_service.rs#L638-L695) | ✅ 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](crates/manager-core/src/users_service.rs#L818-L826) | ✅ Documented |
| `complete_password_reset` revokes all sessions for the user | [users_service.rs:718-720](crates/manager-core/src/users_service.rs#L718-L720) | ✅ Per brief |
| Roles: string-tagged only; no registry/hierarchy/permission matrix | [0031_app_user_roles.sql](crates/manager-core/migrations/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](crates/manager-core/src/authz.rs) | ✅ No new `Scope` variant added |
| Admin HTTP surface: 10 endpoints under `/api/v1/admin/apps/{id}/{users,invitations}` | [users_admin_api.rs](crates/manager-core/src/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/](dashboard/src/routes/apps/[slug]/users/+page.svelte) + 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](crates/manager-core/migrations/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](crates/manager-core/migrations/0033_topics_auth_mode_session.sql) + [realtime_authority.rs:113-134](crates/manager-core/src/realtime_authority.rs#L113-L134) | ✅ 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](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**.