From 0968bb3b4b06f587b47ddbde839eaee209dacd42 Mon Sep 17 00:00:00 2001 From: michael Date: Mon, 27 Apr 2026 06:15:51 -0700 Subject: [PATCH] docs: expand Phase 2 (storage layer) to step granularity --- docs/plans/2026-04-27-phase-2-storage.md | 1618 ++++++++++++++++++++++ 1 file changed, 1618 insertions(+) create mode 100644 docs/plans/2026-04-27-phase-2-storage.md diff --git a/docs/plans/2026-04-27-phase-2-storage.md b/docs/plans/2026-04-27-phase-2-storage.md new file mode 100644 index 0000000..ee121cf --- /dev/null +++ b/docs/plans/2026-04-27-phase-2-storage.md @@ -0,0 +1,1618 @@ +# Phase 2: Daemon Storage Layer — Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Replace `AppState`'s in-memory `HashMap` fixtures with real on-disk I/O — TOML policy directory with last-key-wins merge, JSON pending flags, `pam_u2f`-format credential files, sqlite user-enrollment cache. Daemon watches the policy dir via inotify and emits a `PolicyChanged` D-Bus signal on change. + +**Architecture:** Pure parser/merger logic lives in `common/src/policy.rs` (CLI also calls it without spawning the daemon). Daemon wraps it in `daemon/src/storage/` modules that own the I/O paths and the inotify watcher. `AppState` keeps its public surface (`list_credentials`, `set_policy`, etc. — what `dbus.rs` calls) but delegates internally to a `Storage` aggregate. Tests use `tempfile::tempdir()` fixtures end-to-end; nothing is mocked. + +**Tech Stack:** Rust 2021, `toml = "0.8"` (already in workspace deps), `tokio::fs` for async file I/O, `notify = "6"` for cross-platform fsnotify (uses inotify on Linux), `rusqlite = "0.31"` with `bundled` feature for the user-enrollment cache. + +**Reference:** +- Design doc: [2026-04-26-authforge-design.md](2026-04-26-authforge-design.md) — § Policy file format, § Lockout safety simulation, Flow diagrams. +- Master plan (Phase 2 section): [2026-04-26-authforge-implementation.md](2026-04-26-authforge-implementation.md). +- Phase 1 (state/dbus contract): [2026-04-26-phase-1-dbus.md](2026-04-26-phase-1-dbus.md). + +--- + +## Conventions + +- One logical change per commit. Conventional prefixes (`feat:`, `test:`, `chore:`, `refactor:`). +- TDD where mechanical: failing test → run to confirm fail → minimal impl → run to confirm pass → commit. +- Always commit with `--no-gpg-sign` (smart card not present in dev env; persistent project memory). +- After each task: `cargo fmt --all && cargo clippy --workspace --all-targets -- -D warnings && cargo test -p authforge-{common,daemon}`. Fix before commit. +- `gui` is excluded from `default-members`; workspace tests run only common + daemon + cli. +- Tests use `tempfile::tempdir()` to write into ephemeral dirs — never touch `/etc` or `/var` from a test. +- All new files in `daemon/src/storage/` are `pub(crate)`. Only `AppState` is the public surface to `dbus.rs`. + +--- + +## Parallelism Note + +Tasks 2.9 (pending), 2.10–2.12 (credentials), and 2.13–2.14 (userdb) operate on independent file trees and are good candidates for parallel subagents (one per file group, all in the same worktree — they don't share files). Tasks 2.6–2.8 (policy storage + inotify watcher) must run after 2.3–2.5 land in `common`. The acceptance gate (2.17) must run last. + +--- + +## Task 2.1: Add storage dependencies + +**Files:** +- Modify: `Cargo.toml` (workspace root) +- Modify: `common/Cargo.toml` +- Modify: `daemon/Cargo.toml` + +**Step 1:** Add to root `Cargo.toml` `[workspace.dependencies]`: + +```toml +notify = "6" +rusqlite = { version = "0.31", features = ["bundled"] } +``` + +`bundled` ships SQLite as C source so we don't depend on libsqlite3-dev at build time. + +**Step 2:** `common/Cargo.toml` — no changes needed yet (toml is already there). + +**Step 3:** `daemon/Cargo.toml` — add to `[dependencies]`: + +```toml +notify = { workspace = true } +rusqlite = { workspace = true } +toml = { workspace = true } +``` + +`tokio` is already in deps with `features = ["full"]`, which includes `fs` and `sync::watch`. + +**Step 4:** Verify the workspace compiles: + +```bash +cargo check -p authforge-common -p authforge-daemon +``` + +Expected: clean (the new deps are unused but pulled into the lockfile). + +**Step 5:** Commit. + +```bash +git add Cargo.toml Cargo.lock daemon/Cargo.toml +git commit --no-gpg-sign -m "chore: add notify + rusqlite for Phase 2 storage layer" +``` + +--- + +## Task 2.2: Move Policy types from `types.rs` to `policy.rs` + +**Files:** +- Modify: `common/src/policy.rs` +- Modify: `common/src/types.rs` + +**Why:** `common/src/policy.rs` has been a stub since Phase 0; per the master plan, the Policy types and parsing logic live in `policy.rs`. `types.rs` keeps the cross-cutting D-Bus wire types (`Mode`, `Method`, `Credential`, `PendingFlag`, etc.). Splitting now keeps Phase 2's parser additions tightly scoped. + +**Step 1:** Move these item definitions from `common/src/types.rs` to `common/src/policy.rs`: +- `StorageBackend` +- `StackPolicy` +- `Storage` +- `Firstrun` +- `Policy` +- The two policy-related tests (`policy_default_is_all_disabled`, `policy_serde_via_json`). + +`policy.rs` top of file: + +```rust +//! Policy types and (Phase 2) parsing/merging logic. Wire-typed for D-Bus. + +use crate::types::{Method, Mode}; +use serde::{Deserialize, Serialize}; +use std::collections::BTreeMap; +``` + +**Step 2:** Re-export from `policy.rs` if any external consumer expected `types::Policy`: + +`common/src/lib.rs`: + +```rust +pub mod policy; +pub mod types; + +// Convenience re-exports — Phase 1 clients imported these from `types`. Keep +// the path stable to avoid churn in daemon/cli. +pub use policy::{Firstrun, Policy, StackPolicy, Storage, StorageBackend}; +``` + +**Step 3:** Update daemon imports — `dbus.rs` imports `Policy`, `PolicyApplyResult`, etc. from `authforge_common::types`. Switch `Policy`-related ones to `authforge_common::policy::*` (or use the re-exports). `state.rs` likewise. + +**Step 4:** Run: + +```bash +cargo test -p authforge-common -p authforge-daemon +``` + +Expected: 7 common + 14 daemon tests pass; no test changes needed. + +**Step 5:** Commit. + +```bash +git add common/src/policy.rs common/src/types.rs common/src/lib.rs daemon/src/dbus.rs daemon/src/state.rs +git commit --no-gpg-sign -m "refactor(common): move Policy types from types.rs into policy.rs" +``` + +--- + +## Task 2.3: `Policy::load_from_dir` — single-file case + +**Files:** +- Modify: `common/src/policy.rs` + +**Step 1:** Add the failing test: + +```rust +#[cfg(test)] +mod parse_tests { + use super::*; + use crate::types::Method; + use tempfile::tempdir; + + fn write(dir: &std::path::Path, name: &str, body: &str) { + std::fs::write(dir.join(name), body).unwrap(); + } + + #[test] + fn load_single_file() { + let d = tempdir().unwrap(); + write( + d.path(), + "00-base.conf", + r#" + [stacks.sudo] + mode = "required" + methods = ["fido2"] + "#, + ); + let p = Policy::load_from_dir(d.path()).unwrap(); + assert_eq!(p.stacks.len(), 1); + let sudo = p.stacks.get("sudo").unwrap(); + assert_eq!(sudo.mode, Mode::Required); + assert_eq!(sudo.methods, vec![Method::Fido2]); + } +} +``` + +`tempfile` is in `[workspace.dependencies]` already; add it as a dev-dep on `common` if not yet there: + +```toml +[dev-dependencies] +tempfile = { workspace = true } +``` + +**Step 2:** Run, confirm fail (`Policy::load_from_dir` undefined). + +**Step 3:** Implement. Append to `policy.rs`: + +```rust +use thiserror::Error; + +#[derive(Debug, Error)] +pub enum PolicyError { + #[error("reading policy directory: {0}")] + Io(#[from] std::io::Error), + #[error("parsing policy file {path}: {source}")] + Toml { path: String, source: toml::de::Error }, +} + +impl Policy { + /// Load and merge all `*.conf` files in `dir`, lex-ascending. Last value + /// for any key wins. Missing dir → returns Self::default(). + pub fn load_from_dir(dir: &std::path::Path) -> Result { + if !dir.exists() { + return Ok(Self::default()); + } + let mut entries: Vec<_> = std::fs::read_dir(dir)? + .filter_map(Result::ok) + .filter(|e| { + e.path() + .extension() + .and_then(|s| s.to_str()) + .map(|s| s.eq_ignore_ascii_case("conf")) + .unwrap_or(false) + }) + .collect(); + entries.sort_by_key(|e| e.file_name()); + + let mut acc = Self::default(); + for e in entries { + let body = std::fs::read_to_string(e.path())?; + let next: Policy = toml::from_str(&body).map_err(|source| PolicyError::Toml { + path: e.path().display().to_string(), + source, + })?; + acc.merge(next); + } + Ok(acc) + } + + fn merge(&mut self, other: Policy) { + for (k, v) in other.stacks { + self.stacks.insert(k, v); + } + if other.storage != Storage::default() { + self.storage = other.storage; + } + if other.firstrun != Firstrun::default() { + self.firstrun = other.firstrun; + } + } +} +``` + +**Step 4:** Run, confirm PASS. + +**Step 5:** Commit. + +```bash +git add common/Cargo.toml common/src/policy.rs +git commit --no-gpg-sign -m "feat(common): Policy::load_from_dir parses TOML policy files" +``` + +--- + +## Task 2.4: `Policy::load_from_dir` — multi-file last-wins merge + +**Files:** +- Modify: `common/src/policy.rs` + +**Step 1:** Add three more tests that prove the merge contract: + +```rust + #[test] + fn last_file_wins_on_overlap() { + let d = tempdir().unwrap(); + write(d.path(), "00-base.conf", r#"[stacks.sudo] + mode = "optional" + methods = ["fido2"]"#); + write(d.path(), "90-fleet.conf", r#"[stacks.sudo] + mode = "required" + methods = ["fido2","totp"]"#); + let p = Policy::load_from_dir(d.path()).unwrap(); + let sudo = p.stacks.get("sudo").unwrap(); + assert_eq!(sudo.mode, Mode::Required); + assert_eq!(sudo.methods.len(), 2); + } + + #[test] + fn lex_order_not_filesystem_order() { + // Reverse-create to defeat filesystem creation-order ordering. + let d = tempdir().unwrap(); + write(d.path(), "99-late.conf", r#"[stacks.sudo] + mode = "disabled" + methods = []"#); + write(d.path(), "10-early.conf", r#"[stacks.sudo] + mode = "required" + methods = ["fido2"]"#); + let p = Policy::load_from_dir(d.path()).unwrap(); + // 99-late wins because lex > 10-early. + assert_eq!(p.stacks.get("sudo").unwrap().mode, Mode::Disabled); + } + + #[test] + fn ignores_non_conf_files() { + let d = tempdir().unwrap(); + write(d.path(), "garbage.txt", "this is not toml"); + write(d.path(), "00-base.conf", r#"[stacks.sudo] + mode = "required" + methods = ["fido2"]"#); + let p = Policy::load_from_dir(d.path()).unwrap(); + assert_eq!(p.stacks.get("sudo").unwrap().mode, Mode::Required); + } + + #[test] + fn missing_dir_yields_default() { + let d = tempdir().unwrap(); + let bogus = d.path().join("nope"); + assert_eq!(Policy::load_from_dir(&bogus).unwrap(), Policy::default()); + } +``` + +**Step 2:** Run. The merge logic from Task 2.3 should already pass `last_file_wins_on_overlap`, `lex_order_not_filesystem_order`, and `ignores_non_conf_files`. `missing_dir_yields_default` passes via the early `if !dir.exists()` branch. + +If any fail, fix `merge()`/`load_from_dir`. + +**Step 3:** Commit. + +```bash +git add common/src/policy.rs +git commit --no-gpg-sign -m "test(common): cover last-wins, lex order, non-conf, missing-dir cases" +``` + +--- + +## Task 2.5: `Policy::save_local` — write `50-local.conf`, preserve siblings + +**Files:** +- Modify: `common/src/policy.rs` + +**Why:** GUI/CLI edits should land in `50-local.conf` so a fleet-managed `90-fleet.conf` still wins (lex). The save path writes only the local file; sibling files are untouched. + +**Step 1:** Failing test: + +```rust + #[test] + fn save_local_preserves_sibling_files() { + let d = tempdir().unwrap(); + write(d.path(), "90-fleet.conf", r#"[stacks.sudo] + mode = "required" + methods = ["fido2"]"#); + + let mut local = Policy::default(); + local.stacks.insert( + "gdm-password".to_string(), + StackPolicy { mode: Mode::Optional, methods: vec![Method::Fido2] }, + ); + local.save_local(d.path()).unwrap(); + + // Sibling untouched. + let fleet_body = std::fs::read_to_string(d.path().join("90-fleet.conf")).unwrap(); + assert!(fleet_body.contains("required")); + + // 50-local.conf created. + assert!(d.path().join("50-local.conf").exists()); + + // Reload merges both. + let merged = Policy::load_from_dir(d.path()).unwrap(); + assert_eq!(merged.stacks.get("sudo").unwrap().mode, Mode::Required); // from 90-fleet + assert_eq!(merged.stacks.get("gdm-password").unwrap().mode, Mode::Optional); // from 50-local + } +``` + +**Step 2:** Run, confirm fail. + +**Step 3:** Implement on `impl Policy`: + +```rust + /// Serialize `self` as TOML and write to `/50-local.conf`. Other files + /// in `dir` are not read, written, or removed. + pub fn save_local(&self, dir: &std::path::Path) -> Result<(), PolicyError> { + std::fs::create_dir_all(dir)?; + let body = toml::to_string_pretty(self).map_err(|e| PolicyError::Toml { + path: dir.join("50-local.conf").display().to_string(), + source: toml::de::Error::custom(e.to_string()), + })?; + std::fs::write(dir.join("50-local.conf"), body)?; + Ok(()) + } +``` + +`toml::de::Error::custom` doesn't exist — use a fresh enum variant instead: + +```rust +#[derive(Debug, Error)] +pub enum PolicyError { + #[error("reading policy directory: {0}")] + Io(#[from] std::io::Error), + #[error("parsing policy file {path}: {source}")] + Toml { path: String, source: toml::de::Error }, + #[error("serializing policy: {0}")] + Serialize(#[from] toml::ser::Error), +} +``` + +Then `save_local` becomes: + +```rust + pub fn save_local(&self, dir: &std::path::Path) -> Result<(), PolicyError> { + std::fs::create_dir_all(dir)?; + let body = toml::to_string_pretty(self)?; + std::fs::write(dir.join("50-local.conf"), body)?; + Ok(()) + } +``` + +**Step 4:** Run; PASS. + +**Step 5:** Commit. + +```bash +git add common/src/policy.rs +git commit --no-gpg-sign -m "feat(common): Policy::save_local writes 50-local.conf preserving siblings" +``` + +--- + +## Task 2.6: `daemon/src/storage/policy.rs` — wraps the parser with config + +**Files:** +- Create: `daemon/src/storage/mod.rs` +- Create: `daemon/src/storage/policy.rs` +- Modify: `daemon/src/main.rs` (add `mod storage;`) + +**Step 1:** `daemon/src/storage/mod.rs`: + +```rust +pub(crate) mod policy; +``` + +**Step 2:** `daemon/src/storage/policy.rs`: + +```rust +use authforge_common::policy::{Policy, PolicyError}; +use std::path::PathBuf; + +pub(crate) struct PolicyStore { + dir: PathBuf, +} + +impl PolicyStore { + pub fn new(dir: PathBuf) -> Self { + Self { dir } + } + + pub fn load(&self) -> Result { + Policy::load_from_dir(&self.dir) + } + + pub fn save(&self, p: &Policy) -> Result<(), PolicyError> { + p.save_local(&self.dir) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use authforge_common::types::Method; + use authforge_common::policy::StackPolicy; + use authforge_common::types::Mode; + use tempfile::tempdir; + + #[test] + fn save_then_load_roundtrip() { + let d = tempdir().unwrap(); + let store = PolicyStore::new(d.path().to_path_buf()); + + let mut p = Policy::default(); + p.stacks.insert( + "sudo".to_string(), + StackPolicy { mode: Mode::Required, methods: vec![Method::Fido2] }, + ); + store.save(&p).unwrap(); + + let back = store.load().unwrap(); + assert_eq!(back, p); + } +} +``` + +**Step 3:** Add `mod storage;` to `daemon/src/main.rs`. + +**Step 4:** Run `cargo test -p authforge-daemon storage`. Expected: 1 PASS. + +**Step 5:** Commit. + +```bash +git add daemon/src/storage/mod.rs daemon/src/storage/policy.rs daemon/src/main.rs +git commit --no-gpg-sign -m "feat(daemon): storage::policy wraps Policy::load_from_dir + save_local" +``` + +--- + +## Task 2.7: `PolicyChanged` D-Bus signal + +**Files:** +- Modify: `daemon/src/dbus.rs` + +**Step 1:** Add a `signal` method to the `#[zbus::interface]` impl. zbus 4 emits signals via methods marked `#[zbus(signal)]`: + +```rust + #[zbus(signal)] + pub async fn policy_changed( + signal_emitter: &zbus::object_server::SignalEmitter<'_>, + ) -> zbus::Result<()>; +``` + +(Must be declared inside the `#[interface]` impl. zbus generates the dispatcher.) + +**Step 2:** Add an integration test that subscribes to the signal on the p2p connection: + +```rust + #[tokio::test] + async fn policy_changed_signal_fires_on_emit() { + use futures_util::stream::StreamExt; + let (server, client, _state) = p2p_pair().await; + + let proxy = proxy(&client).await; + let mut stream = proxy.receive_signal("PolicyChanged").await.unwrap(); + + // Emit from the server side. + let iface_ref: zbus::object_server::InterfaceRef = + server.object_server().interface("/io/dangerousthings/AuthForge").await.unwrap(); + AuthForge::policy_changed(iface_ref.signal_emitter()).await.unwrap(); + + let _msg = tokio::time::timeout(std::time::Duration::from_secs(2), stream.next()) + .await + .expect("signal not received within 2s") + .expect("stream closed before signal"); + } +``` + +`futures-util` isn't in deps; add to daemon `[dev-dependencies]`: + +```toml +futures-util = "0.3" +``` + +(Add `futures-util = "0.3"` to workspace deps so `daemon`'s dev-dep can `{ workspace = true }`.) + +**Step 3:** Run. Expected: PASS. + +**Step 4:** Commit. + +```bash +git add Cargo.toml daemon/Cargo.toml daemon/src/dbus.rs +git commit --no-gpg-sign -m "feat(daemon): emit PolicyChanged D-Bus signal" +``` + +--- + +## Task 2.8: inotify watcher → emits `PolicyChanged` + +**Files:** +- Modify: `daemon/src/storage/policy.rs` +- Modify: `daemon/src/main.rs` + +**Step 1:** Add a `watch()` method to `PolicyStore` that returns a `tokio::sync::watch::Receiver<()>` ticking on every fs change in the policy dir: + +```rust +use notify::{RecommendedWatcher, RecursiveMode, Watcher}; + +impl PolicyStore { + /// Start watching `self.dir` for any change. The returned receiver yields + /// `()` each time something changes. Caller is responsible for keeping the + /// returned `RecommendedWatcher` alive — drop it to stop watching. + pub fn watch(&self) -> notify::Result<(RecommendedWatcher, tokio::sync::watch::Receiver<()>)> { + let (tx, rx) = tokio::sync::watch::channel(()); + let mut watcher = notify::recommended_watcher(move |res: notify::Result| { + if res.is_ok() { + let _ = tx.send(()); + } + })?; + // Safe to call even if the dir doesn't yet exist — we created it via save_local. + std::fs::create_dir_all(&self.dir).ok(); + watcher.watch(&self.dir, RecursiveMode::NonRecursive)?; + Ok((watcher, rx)) + } +} +``` + +**Step 2:** Add a unit test that creates a tempdir, starts watching, touches a file, asserts a tick arrives: + +```rust + #[tokio::test] + async fn watcher_ticks_on_file_change() { + let d = tempdir().unwrap(); + let store = PolicyStore::new(d.path().to_path_buf()); + let (_watcher, mut rx) = store.watch().unwrap(); + + std::fs::write(d.path().join("00-base.conf"), "[stacks.sudo]\nmode = \"required\"\nmethods = [\"fido2\"]\n").unwrap(); + + tokio::time::timeout(std::time::Duration::from_secs(2), rx.changed()) + .await + .expect("no change within 2s") + .unwrap(); + } +``` + +**Step 3:** Wire `main.rs` to spawn a task that calls `dbus::AuthForge::policy_changed(...)` whenever `rx.changed()` fires: + +```rust + // After object_server().at(...).await? line: + let policy_dir = std::env::var_os("AUTHFORGE_POLICY_DIR") + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from("/etc/authforge/policy.d")); + let store = storage::policy::PolicyStore::new(policy_dir); + let (watcher, mut rx) = store.watch()?; + let conn_for_signal = conn.clone(); + tokio::spawn(async move { + while rx.changed().await.is_ok() { + let iface_ref = match conn_for_signal + .object_server() + .interface::<_, dbus::AuthForge>(OBJECT_PATH) + .await + { + Ok(r) => r, + Err(e) => { tracing::warn!(error=%e, "no AuthForge interface; skipping signal"); continue; } + }; + if let Err(e) = dbus::AuthForge::policy_changed(iface_ref.signal_emitter()).await { + tracing::warn!(error=%e, "PolicyChanged emit failed"); + } + } + }); + // Keep watcher alive for the daemon's lifetime. + std::mem::forget(watcher); +``` + +`std::mem::forget(watcher)` is acceptable here — the daemon runs until SIGTERM and `notify::RecommendedWatcher` releases its inotify fd in its Drop. We deliberately leak it for lifetime simplicity. (Phase 5 lockout simulator may revisit.) + +**Step 4:** Run `cargo test -p authforge-daemon`. Expected: all prior tests + the new watcher test pass. + +**Step 5:** Commit. + +```bash +git add daemon/src/storage/policy.rs daemon/src/main.rs +git commit --no-gpg-sign -m "feat(daemon): inotify-based policy watcher emitting PolicyChanged" +``` + +--- + +## Task 2.9: `daemon/src/storage/pending.rs` — JSON pending flag CRUD + +**Files:** +- Create: `daemon/src/storage/pending.rs` +- Modify: `daemon/src/storage/mod.rs` + +**Why:** `/var/lib/authforge/pending/` is a small JSON file with `PendingFlag` content. PAM module (Phase 6) reads this; daemon writes it. + +**Step 1:** Add `pub(crate) mod pending;` to `daemon/src/storage/mod.rs`. + +**Step 2:** Failing tests + impl in one file: + +```rust +use authforge_common::types::PendingFlag; +use std::path::{Path, PathBuf}; +use thiserror::Error; + +#[derive(Debug, Error)] +pub(crate) enum PendingError { + #[error("io: {0}")] + Io(#[from] std::io::Error), + #[error("invalid username: {0}")] + InvalidUser(String), + #[error("json: {0}")] + Json(#[from] serde_json::Error), +} + +pub(crate) struct PendingStore { + dir: PathBuf, +} + +impl PendingStore { + pub fn new(dir: PathBuf) -> Self { + Self { dir } + } + + fn user_path(&self, user: &str) -> Result { + // Reject anything that could escape the dir or hit a special filename. + if user.is_empty() + || user.contains('/') + || user.contains('\0') + || user == "." + || user == ".." + { + return Err(PendingError::InvalidUser(user.to_string())); + } + Ok(self.dir.join(user)) + } + + pub fn set(&self, user: &str, flag: &PendingFlag) -> Result<(), PendingError> { + std::fs::create_dir_all(&self.dir)?; + let path = self.user_path(user)?; + let body = serde_json::to_vec_pretty(flag)?; + std::fs::write(path, body)?; + Ok(()) + } + + pub fn clear(&self, user: &str) -> Result { + let path = self.user_path(user)?; + match std::fs::remove_file(path) { + Ok(()) => Ok(true), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(e) => Err(PendingError::Io(e)), + } + } + + pub fn get(&self, user: &str) -> Result, PendingError> { + let path = self.user_path(user)?; + match std::fs::read(&path) { + Ok(b) => Ok(Some(serde_json::from_slice(&b)?)), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(e) => Err(PendingError::Io(e)), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use authforge_common::types::Method; + use tempfile::tempdir; + + fn flag() -> PendingFlag { + PendingFlag { + required_methods: vec![Method::Fido2], + created_unix: 1, + deadline_unix: 0, + re_enroll: false, + } + } + + #[test] + fn set_get_clear_roundtrip() { + let d = tempdir().unwrap(); + let s = PendingStore::new(d.path().to_path_buf()); + assert!(s.get("alice").unwrap().is_none()); + s.set("alice", &flag()).unwrap(); + assert_eq!(s.get("alice").unwrap().unwrap(), flag()); + assert!(s.clear("alice").unwrap()); + assert!(s.get("alice").unwrap().is_none()); + assert!(!s.clear("alice").unwrap()); + } + + #[test] + fn rejects_path_traversal() { + let d = tempdir().unwrap(); + let s = PendingStore::new(d.path().to_path_buf()); + for evil in ["../etc", "a/b", "", ".", "..", "x\0y"] { + assert!(s.set(evil, &flag()).is_err()); + assert!(s.get(evil).is_err()); + assert!(s.clear(evil).is_err()); + } + } +} +``` + +**Step 3:** Run; PASS. + +**Step 4:** Commit. + +```bash +git add daemon/src/storage/mod.rs daemon/src/storage/pending.rs +git commit --no-gpg-sign -m "feat(daemon): storage::pending JSON read/write/clear with path-traversal guards" +``` + +--- + +## Task 2.10: `daemon/src/storage/credentials.rs` — pam_u2f line parser + +**Files:** +- Create: `daemon/src/storage/credentials.rs` +- Modify: `daemon/src/storage/mod.rs` + +**Why:** `pam_u2f` reads either `~/.config/Yubico/u2f_keys` (per-user) or a central `authfile` like `/etc/u2f_mappings`. Format per pam_u2f source: each line is `username:credential[:credential...]`, where `credential` is `keyHandle,publicKey,coseType,attributes` (commas, hex-encoded). For Phase 2 we treat each colon-separated chunk after the username as an opaque blob (`pam_u2f` is the source of truth on internal format); we only need split, append, and remove-by-credId where credId = the first comma-separated field. + +**Step 1:** `pub(crate) mod credentials;` in `storage/mod.rs`. + +**Step 2:** Failing test + impl: + +```rust +use thiserror::Error; + +#[derive(Debug, Error)] +pub(crate) enum CredsError { + #[error("io: {0}")] + Io(#[from] std::io::Error), + #[error("malformed line for {user}: {reason}")] + Malformed { user: String, reason: &'static str }, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct CredEntry { + pub user: String, + pub creds: Vec, // each = keyHandle,publicKey,coseType,attributes +} + +impl CredEntry { + pub fn from_line(line: &str) -> Result { + let mut it = line.splitn(2, ':'); + let user = it.next().ok_or(CredsError::Malformed { + user: String::new(), + reason: "empty", + })?; + if user.is_empty() { + return Err(CredsError::Malformed { user: String::new(), reason: "empty user" }); + } + let rest = it.next().unwrap_or(""); + let creds: Vec = if rest.is_empty() { + vec![] + } else { + rest.split(':').map(|s| s.to_string()).collect() + }; + Ok(Self { user: user.to_string(), creds }) + } + + pub fn to_line(&self) -> String { + let mut s = String::with_capacity(self.user.len() + 64); + s.push_str(&self.user); + for c in &self.creds { + s.push(':'); + s.push_str(c); + } + s + } + + /// Returns the credential ID (the first comma-separated field) of `cred`. + pub fn cred_id(cred: &str) -> &str { + cred.split(',').next().unwrap_or("") + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_user_with_two_credentials() { + let line = "alice:khA,pkA,es256,present:khB,pkB,es256,present"; + let e = CredEntry::from_line(line).unwrap(); + assert_eq!(e.user, "alice"); + assert_eq!(e.creds.len(), 2); + assert_eq!(CredEntry::cred_id(&e.creds[0]), "khA"); + } + + #[test] + fn parse_user_no_credentials() { + let e = CredEntry::from_line("bob:").unwrap(); + assert_eq!(e.user, "bob"); + assert!(e.creds.is_empty()); + } + + #[test] + fn line_roundtrips() { + let line = "alice:khA,pkA,es256,present:khB,pkB,es256,present"; + let e = CredEntry::from_line(line).unwrap(); + assert_eq!(e.to_line(), line); + } + + #[test] + fn rejects_empty_user() { + assert!(CredEntry::from_line(":khA,pkA,es256,present").is_err()); + } +} +``` + +**Step 3:** Run; PASS. + +**Step 4:** Commit. + +```bash +git add daemon/src/storage/mod.rs daemon/src/storage/credentials.rs +git commit --no-gpg-sign -m "feat(daemon): storage::credentials parses pam_u2f line format" +``` + +--- + +## Task 2.11: `CredentialsStore` — append, remove-by-credId, persist + +**Files:** +- Modify: `daemon/src/storage/credentials.rs` + +**Step 1:** Failing tests: + +```rust + #[test] + fn add_then_remove_credential_idempotent() { + let d = tempdir().unwrap(); + let path = d.path().join("u2f_keys"); + let store = CredentialsStore::new(path.clone()); + + store.add("alice", "kh1,pk1,es256,present").unwrap(); + store.add("alice", "kh2,pk2,es256,present").unwrap(); + // Idempotent: adding kh1 again should not duplicate. + store.add("alice", "kh1,pk1-renewed,es256,present").unwrap(); + + let creds = store.list("alice").unwrap(); + assert_eq!(creds.len(), 2); + assert!(creds.iter().any(|c| CredEntry::cred_id(c) == "kh1")); + assert!(creds.iter().any(|c| CredEntry::cred_id(c) == "kh2")); + + assert!(store.remove("alice", "kh1").unwrap()); + let creds = store.list("alice").unwrap(); + assert_eq!(creds.len(), 1); + assert_eq!(CredEntry::cred_id(&creds[0]), "kh2"); + + // Removing a non-existent credId returns false. + assert!(!store.remove("alice", "kh1").unwrap()); + } + + #[test] + fn list_for_unknown_user_is_empty() { + let d = tempdir().unwrap(); + let store = CredentialsStore::new(d.path().join("u2f_keys")); + assert!(store.list("noone").unwrap().is_empty()); + } +``` + +(`tempfile::tempdir` already in scope.) + +**Step 2:** Run, confirm fail. + +**Step 3:** Implement on top of existing module: + +```rust +pub(crate) struct CredentialsStore { + path: std::path::PathBuf, +} + +impl CredentialsStore { + pub fn new(path: std::path::PathBuf) -> Self { + Self { path } + } + + fn read_all(&self) -> Result, CredsError> { + match std::fs::read_to_string(&self.path) { + Ok(s) => s + .lines() + .filter(|l| !l.trim().is_empty()) + .map(CredEntry::from_line) + .collect(), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(vec![]), + Err(e) => Err(e.into()), + } + } + + fn write_all(&self, entries: &[CredEntry]) -> Result<(), CredsError> { + if let Some(parent) = self.path.parent() { + std::fs::create_dir_all(parent)?; + } + let body = entries + .iter() + .map(CredEntry::to_line) + .collect::>() + .join("\n"); + std::fs::write(&self.path, body)?; + Ok(()) + } + + pub fn list(&self, user: &str) -> Result, CredsError> { + Ok(self + .read_all()? + .into_iter() + .find(|e| e.user == user) + .map(|e| e.creds) + .unwrap_or_default()) + } + + pub fn add(&self, user: &str, cred: &str) -> Result<(), CredsError> { + let new_id = CredEntry::cred_id(cred); + let mut entries = self.read_all()?; + match entries.iter_mut().find(|e| e.user == user) { + Some(entry) => { + if entry.creds.iter().any(|c| CredEntry::cred_id(c) == new_id) { + // Already present; treat as idempotent. (Phase 3 may decide + // to refresh the public key on re-enroll; not Phase 2's job.) + return Ok(()); + } + entry.creds.push(cred.to_string()); + } + None => entries.push(CredEntry { user: user.to_string(), creds: vec![cred.to_string()] }), + } + self.write_all(&entries) + } + + pub fn remove(&self, user: &str, cred_id: &str) -> Result { + let mut entries = self.read_all()?; + let mut changed = false; + if let Some(entry) = entries.iter_mut().find(|e| e.user == user) { + let before = entry.creds.len(); + entry.creds.retain(|c| CredEntry::cred_id(c) != cred_id); + changed = entry.creds.len() != before; + } + if changed { + self.write_all(&entries)?; + } + Ok(changed) + } +} +``` + +**Step 4:** Run; PASS. + +**Step 5:** Commit. + +```bash +git add daemon/src/storage/credentials.rs +git commit --no-gpg-sign -m "feat(daemon): CredentialsStore add/remove/list against pam_u2f file" +``` + +--- + +## Task 2.12: Per-user vs central credential resolution + +**Files:** +- Modify: `daemon/src/storage/credentials.rs` + +**Why:** Per `Storage.backend`: `PerUser` writes to `~/.config/Yubico/u2f_keys`; `Central` writes all users into `Storage.central_path`. The right choice is dictated by current Policy; daemon resolves the path before calling `CredentialsStore`. + +**Step 1:** Failing test: + +```rust + #[test] + fn per_user_path_resolution() { + // Without an actual user account, we just verify the path computation + // pivots on the configured backend. + let central = std::path::PathBuf::from("/etc/u2f_mappings"); + let resolver = CredsPathResolver::new(authforge_common::policy::Storage { + backend: authforge_common::policy::StorageBackend::Central, + central_path: central.display().to_string(), + }); + assert_eq!(resolver.path_for("alice").unwrap(), central); + + let resolver = CredsPathResolver::new(authforge_common::policy::Storage { + backend: authforge_common::policy::StorageBackend::PerUser, + central_path: String::new(), + }); + let p = resolver.path_for("alice").unwrap(); + // We don't have alice in /etc/passwd in CI, so accept either a real + // home-dir-derived path or the documented fallback. + assert!(p.ends_with(".config/Yubico/u2f_keys")); + } +``` + +**Step 2:** Implement: + +```rust +use authforge_common::policy::{Storage, StorageBackend}; + +pub(crate) struct CredsPathResolver { + storage: Storage, +} + +impl CredsPathResolver { + pub fn new(storage: Storage) -> Self { + Self { storage } + } + + pub fn path_for(&self, user: &str) -> Result { + match self.storage.backend { + StorageBackend::Central => Ok(std::path::PathBuf::from(&self.storage.central_path)), + StorageBackend::PerUser => { + // Lookup home dir via getpwnam. Tests against a CI machine without + // the user fall back to /home//.config/Yubico/u2f_keys — + // pam_u2f does the real lookup at auth time, so the daemon's + // chosen path only matters for *writes* on enrollment. + let home = nix::unistd::User::from_name(user) + .map_err(|e| CredsError::Io(std::io::Error::other(e)))? + .map(|u| u.dir) + .unwrap_or_else(|| std::path::PathBuf::from(format!("/home/{user}"))); + Ok(home.join(".config/Yubico/u2f_keys")) + } + } + } +} +``` + +`nix::unistd::User::from_name` is in the `user` feature — already enabled in workspace deps. + +**Step 3:** Run; PASS. + +**Step 4:** Commit. + +```bash +git add daemon/src/storage/credentials.rs +git commit --no-gpg-sign -m "feat(daemon): CredsPathResolver picks per-user vs central path" +``` + +--- + +## Task 2.13: `daemon/src/storage/userdb.rs` — sqlite open + schema + +**Files:** +- Create: `daemon/src/storage/userdb.rs` +- Modify: `daemon/src/storage/mod.rs` + +**Why:** Lockout simulation (Phase 5) needs a fast lookup of "every user with any enrolled method." Walking `/home/*/.config/Yubico/u2f_keys` at policy-apply time is O(users); a sqlite cache keeps it O(1). The cache is authoritative for *the simulator's input*, not for actual auth — `pam_u2f` reads the file directly at login. + +**Step 1:** Add `pub(crate) mod userdb;` to `storage/mod.rs`. + +**Step 2:** Create `userdb.rs`: + +```rust +use authforge_common::types::Method; +use rusqlite::{params, Connection}; +use std::path::Path; +use thiserror::Error; + +#[derive(Debug, Error)] +pub(crate) enum UserDbError { + #[error("sqlite: {0}")] + Sqlite(#[from] rusqlite::Error), +} + +pub(crate) struct UserDb { + conn: Connection, +} + +impl UserDb { + pub fn open(path: &Path) -> Result { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).ok(); + } + let conn = Connection::open(path)?; + Self::init(&conn)?; + Ok(Self { conn }) + } + + pub fn open_in_memory() -> Result { + let conn = Connection::open_in_memory()?; + Self::init(&conn)?; + Ok(Self { conn }) + } + + fn init(conn: &Connection) -> Result<(), UserDbError> { + conn.execute_batch( + r#" + CREATE TABLE IF NOT EXISTS users ( + username TEXT PRIMARY KEY, + has_fido2 INTEGER NOT NULL DEFAULT 0, + has_totp INTEGER NOT NULL DEFAULT 0, + last_seen INTEGER NOT NULL DEFAULT 0 + ) WITHOUT ROWID; + "#, + )?; + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn open_in_memory_creates_schema() { + let db = UserDb::open_in_memory().unwrap(); + let row: i64 = db + .conn + .query_row( + "SELECT count(*) FROM sqlite_master WHERE type='table' AND name='users'", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(row, 1); + } +} +``` + +**Step 3:** Run; PASS. + +**Step 4:** Commit. + +```bash +git add daemon/src/storage/mod.rs daemon/src/storage/userdb.rs +git commit --no-gpg-sign -m "feat(daemon): storage::userdb opens sqlite + initializes schema" +``` + +--- + +## Task 2.14: `UserDb` — record / query / list + +**Files:** +- Modify: `daemon/src/storage/userdb.rs` + +**Step 1:** Failing test: + +```rust + #[test] + fn record_then_list_users_with_method() { + let db = UserDb::open_in_memory().unwrap(); + db.record_enrollment("alice", Method::Fido2).unwrap(); + db.record_enrollment("bob", Method::Totp).unwrap(); + db.record_enrollment("alice", Method::Totp).unwrap(); + + let mut fido2 = db.users_with(Method::Fido2).unwrap(); + fido2.sort(); + assert_eq!(fido2, vec!["alice".to_string()]); + + let mut totp = db.users_with(Method::Totp).unwrap(); + totp.sort(); + assert_eq!(totp, vec!["alice".to_string(), "bob".to_string()]); + + assert!(db.has_any("alice").unwrap()); + assert!(!db.has_any("nobody").unwrap()); + + // Drop alice's fido2; still has totp, still in users table. + db.drop_enrollment("alice", Method::Fido2).unwrap(); + assert!(db.users_with(Method::Fido2).unwrap().is_empty()); + assert!(db.has_any("alice").unwrap()); + } +``` + +**Step 2:** Implement on `impl UserDb`: + +```rust + pub fn record_enrollment(&self, user: &str, method: Method) -> Result<(), UserDbError> { + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs() as i64) + .unwrap_or(0); + let col = method_col(method); + let sql = format!( + "INSERT INTO users (username, {col}, last_seen) VALUES (?1, 1, ?2) + ON CONFLICT(username) DO UPDATE SET {col} = 1, last_seen = ?2" + ); + self.conn.execute(&sql, params![user, now])?; + Ok(()) + } + + pub fn drop_enrollment(&self, user: &str, method: Method) -> Result<(), UserDbError> { + let col = method_col(method); + let sql = format!("UPDATE users SET {col} = 0 WHERE username = ?1"); + self.conn.execute(&sql, params![user])?; + Ok(()) + } + + pub fn users_with(&self, method: Method) -> Result, UserDbError> { + let col = method_col(method); + let sql = format!("SELECT username FROM users WHERE {col} = 1"); + let mut stmt = self.conn.prepare(&sql)?; + let rows = stmt.query_map([], |r| r.get::<_, String>(0))?; + Ok(rows.filter_map(Result::ok).collect()) + } + + pub fn has_any(&self, user: &str) -> Result { + let row: Option = self + .conn + .query_row( + "SELECT 1 FROM users WHERE username = ?1 AND (has_fido2 = 1 OR has_totp = 1)", + params![user], + |r| r.get(0), + ) + .ok(); + Ok(row.is_some()) + } +} + +fn method_col(m: Method) -> &'static str { + match m { + Method::Fido2 => "has_fido2", + Method::Totp => "has_totp", + } +} +``` + +**Step 3:** Run; PASS. + +**Step 4:** Commit. + +```bash +git add daemon/src/storage/userdb.rs +git commit --no-gpg-sign -m "feat(daemon): UserDb record/drop/users_with/has_any" +``` + +--- + +## Task 2.15: Refactor `AppState` to delegate to storage modules + +**Files:** +- Modify: `daemon/src/state.rs` +- Modify: `daemon/src/main.rs` + +**Step 1:** Replace `state.rs` with a façade. New shape: + +```rust +use crate::storage::{ + credentials::{CredEntry, CredentialsStore, CredsPathResolver, CredsError}, + pending::{PendingError, PendingStore}, + policy::PolicyStore, + userdb::{UserDb, UserDbError}, +}; +use authforge_common::policy::Policy; +use authforge_common::types::{Credential, Method, PendingFlag, Transport}; +use std::path::PathBuf; +use thiserror::Error; +use tokio::sync::Mutex; + +#[derive(Debug, Error)] +pub enum StateError { + #[error(transparent)] + Policy(#[from] authforge_common::policy::PolicyError), + #[error(transparent)] + Pending(#[from] PendingError), + #[error(transparent)] + Creds(#[from] CredsError), + #[error(transparent)] + UserDb(#[from] UserDbError), +} + +pub struct StorageConfig { + pub policy_dir: PathBuf, + pub pending_dir: PathBuf, + pub userdb_path: PathBuf, +} + +impl StorageConfig { + pub fn from_env_or_defaults() -> Self { + let env = |k: &str, d: &str| -> PathBuf { + std::env::var_os(k).map(PathBuf::from).unwrap_or_else(|| PathBuf::from(d)) + }; + Self { + policy_dir: env("AUTHFORGE_POLICY_DIR", "/etc/authforge/policy.d"), + pending_dir: env("AUTHFORGE_PENDING_DIR", "/var/lib/authforge/pending"), + userdb_path: env("AUTHFORGE_USERDB", "/var/lib/authforge/users.db"), + } + } +} + +pub struct AppState { + policy: PolicyStore, + pending: PendingStore, + userdb: Mutex, + cfg: StorageConfig, +} + +impl AppState { + pub fn open(cfg: StorageConfig) -> Result { + let policy = PolicyStore::new(cfg.policy_dir.clone()); + let pending = PendingStore::new(cfg.pending_dir.clone()); + let userdb = Mutex::new(UserDb::open(&cfg.userdb_path)?); + Ok(Self { policy, pending, userdb, cfg }) + } + + fn creds_for_current_policy(&self) -> Result { + let pol = self.policy.load()?; + Ok(CredsPathResolver::new(pol.storage)) + } + + pub async fn list_credentials(&self, user: &str) -> Vec { + // Phase 2 returns opaque cred IDs; richer Credential metadata (nickname, + // transport) lands when Phase 3 records them at enrollment time. For + // now, surface the keyHandle as both id and nickname so the GUI shows + // *something* and Phase 3 can swap the source. + let resolver = match self.creds_for_current_policy() { + Ok(r) => r, + Err(_) => return vec![], + }; + let path = match resolver.path_for(user) { + Ok(p) => p, + Err(_) => return vec![], + }; + let store = CredentialsStore::new(path); + let lines = store.list(user).unwrap_or_default(); + lines + .iter() + .map(|c| { + let id = CredEntry::cred_id(c).to_string(); + Credential { + id: id.clone(), + nickname: id, + method: Method::Fido2, + transport: Transport::Unknown, + created_unix: 0, + } + }) + .collect() + } + + pub async fn add_credential(&self, user: &str, c: Credential) -> Result<(), StateError> { + let resolver = self.creds_for_current_policy()?; + let path = resolver.path_for(user)?; + let store = CredentialsStore::new(path); + // Phase 2 stub: record id-only (Phase 3 fills the full pam_u2f line). + store.add(user, &c.id)?; + let mut db = self.userdb.lock().await; + db.record_enrollment(user, c.method)?; + Ok(()) + } + + pub async fn remove_credential(&self, user: &str, cred_id: &str) -> Result { + let resolver = self.creds_for_current_policy()?; + let path = resolver.path_for(user)?; + let store = CredentialsStore::new(path); + let removed = store.remove(user, cred_id)?; + // If user has no remaining creds, drop the fido2 flag. (Phase 5 simulator + // reads has_any/users_with, so keep the cache honest.) + if removed && store.list(user)?.is_empty() { + self.userdb.lock().await.drop_enrollment(user, Method::Fido2)?; + } + Ok(removed) + } + + pub async fn get_policy(&self) -> Result { + Ok(self.policy.load()?) + } + + pub async fn set_policy(&self, p: Policy) -> Result<(), StateError> { + self.policy.save(&p)?; + Ok(()) + } + + pub async fn set_pending(&self, user: &str, f: PendingFlag) -> Result<(), StateError> { + self.pending.set(user, &f)?; + Ok(()) + } + + pub async fn clear_pending(&self, user: &str) -> Result { + Ok(self.pending.clear(user)?) + } + + #[cfg(test)] + pub async fn has_pending(&self, user: &str) -> Result { + Ok(self.pending.get(user)?.is_some()) + } +} +``` + +**Step 2:** Drop the Phase 1 `with_fixtures()` constructor — fixtures now come from a tempdir in tests. Update `state.rs`'s tests to use `tempdir + StorageConfig`: + +```rust +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir; + + fn open_in(d: &std::path::Path) -> AppState { + AppState::open(StorageConfig { + policy_dir: d.join("policy.d"), + pending_dir: d.join("pending"), + userdb_path: d.join("users.db"), + }) + .unwrap() + } + + #[tokio::test] + async fn empty_state_has_nothing() { + let d = tempdir().unwrap(); + let s = open_in(d.path()); + assert!(s.list_credentials("alice").await.is_empty()); + assert!(!s.has_pending("alice").await.unwrap()); + assert!(s.get_policy().await.unwrap().stacks.is_empty()); + } + + #[tokio::test] + async fn pending_set_clear_roundtrip() { + let d = tempdir().unwrap(); + let s = open_in(d.path()); + let f = PendingFlag { + required_methods: vec![Method::Fido2], + created_unix: 1, + deadline_unix: 0, + re_enroll: false, + }; + s.set_pending("alice", f).await.unwrap(); + assert!(s.has_pending("alice").await.unwrap()); + assert!(s.clear_pending("alice").await.unwrap()); + assert!(!s.has_pending("alice").await.unwrap()); + } +} +``` + +**Step 3:** Update `daemon/src/main.rs`: + +```rust +let state = Arc::new(state::AppState::open(state::StorageConfig::from_env_or_defaults())?); +``` + +(replacing the old `with_fixtures()` call.) + +**Step 4:** Run `cargo test -p authforge-daemon`. + +Expected: state tests + dbus tests still green. The dbus p2p tests previously assumed alice had a fixture cred — UPDATE those tests in Task 2.16 to use a tempdir-backed AppState that gets seeded explicitly. + +This task may show the dbus tests failing (red); fix in 2.16. It's acceptable to commit a temporarily-red dbus test suite *only if 2.16 lands immediately after* — pragmatically, finish 2.15 + 2.16 before pushing. + +**Step 5:** Commit (with the understood pending-fix on dbus tests in 2.16). + +```bash +git add daemon/src/state.rs daemon/src/main.rs +git commit --no-gpg-sign -m "refactor(daemon): AppState delegates to storage modules (fixtures dropped)" +``` + +--- + +## Task 2.16: Update dbus tests to use file-backed `AppState` + +**Files:** +- Modify: `daemon/src/dbus.rs` + +**Step 1:** The `p2p_pair()` helper currently constructs `AppState::with_fixtures()`. Switch to: + +```rust + async fn p2p_pair() -> (Connection, Connection, Arc, tempfile::TempDir) { + let tmp = tempfile::tempdir().unwrap(); + let state = Arc::new( + AppState::open(crate::state::StorageConfig { + policy_dir: tmp.path().join("policy.d"), + pending_dir: tmp.path().join("pending"), + userdb_path: tmp.path().join("users.db"), + }) + .unwrap(), + ); + // ... rest of the function unchanged. + (server, client, state, tmp) + } +``` + +The `tempfile::TempDir` must be returned — dropping it at function end would delete the directory before the test runs. + +`tempfile` is already a daemon dev-dep. + +**Step 2:** Tests that previously relied on alice's fixture credential (`list_credentials_returns_fixture_for_alice`, `remove_own_drops_credential`) need to seed first: + +```rust + #[tokio::test] + async fn list_credentials_after_enroll() { + let (_srv, client, state, _tmp) = p2p_pair().await; + // Seed via the same EnrollOwn path the GUI would use. + let p = proxy(&client).await; + let _: Credential = p.call("EnrollOwn", &("alice", "Yellow")).await.unwrap(); + let creds: Vec = p.call("ListCredentials", &("alice",)).await.unwrap(); + assert_eq!(creds.len(), 1); + assert_eq!(creds[0].method, Method::Fido2); + } +``` + +Replace `list_credentials_returns_fixture_for_alice` (delete) and rename test if cleaner. `remove_own_drops_credential` likewise — first call EnrollOwn, capture the returned Credential, then RemoveOwn against `cred.id`. + +**Step 3:** Update the dbus interface to match the new error-returning storage. `dbus.rs`'s methods convert `StateError` to `zbus::fdo::Error::Failed(msg)`: + +```rust + async fn enroll_own(&self, user: String, nickname: String) -> zbus::fdo::Result { + self.authz("io.dangerousthings.AuthForge.enroll-own").await?; + let now = Self::now(); + let cred = Credential { + id: format!("stub-{user}-{now}"), + nickname, + method: Method::Fido2, + transport: Transport::Usb, + created_unix: now, + }; + self.state + .add_credential(&user, cred.clone()) + .await + .map_err(|e| zbus::fdo::Error::Failed(e.to_string()))?; + Ok(cred) + } +``` + +Apply the same pattern to remove_own, set_policy, set_pending_flag, clear_pending_flag, get_policy. + +**Step 4:** Run `cargo test -p authforge-daemon`. Expected: all tests green. + +**Step 5:** Run `cargo fmt && cargo clippy --workspace --all-targets -- -D warnings`. Fix any issues. + +**Step 6:** Commit. + +```bash +git add daemon/src/dbus.rs +git commit --no-gpg-sign -m "test(daemon): dbus tests use tempdir-backed AppState; thread storage errors" +``` + +--- + +## Task 2.17: Phase 2 acceptance gate + +Verify before declaring Phase 2 complete: + +- [ ] `cargo build --workspace --release` succeeds (excludes gui as before). +- [ ] `cargo test -p authforge-common` — all common tests including 4 policy parse tests pass. +- [ ] `cargo test -p authforge-daemon` — all storage + state + dbus tests pass. +- [ ] `cargo clippy --workspace --all-targets -- -D warnings` clean. +- [ ] Manual: drop a `00-base.conf` into `/etc/authforge/policy.d/`, observe daemon emits `PolicyChanged` (`busctl monitor io.dangerousthings.AuthForge`). +- [ ] `debuild -us -uc -b` still produces 5 debs (no packaging regression — Phase 2 doesn't touch `debian/`). + +Tag the milestone: + +```bash +git tag -a v0.2.0-storage -m "Phase 2: storage layer (policy.d, pending, credentials, userdb)" +``` + +--- + +## Risks / known unknowns + +| Risk | Mitigation | +|---|---| +| `notify` crate's debouncing — single file write may emit multiple events. | We send `()` ticks via a `watch` channel, which collapses bursts. The signal listener is fine with one or many ticks per change. Reconsider if the GUI shows flicker. | +| `pam_u2f` line format may change between minor versions. | We split on `:` and pass colons-and-commas through opaquely except for the credId field. If pam_u2f extends the comma-fields, we still round-trip the line losslessly. | +| `rusqlite` `bundled` adds ~500 KB to the binary. | Acceptable. Avoids dynamic libsqlite3 pinning across Ubuntu LTS versions. | +| Path traversal in `pending_dir + user`. | Explicit `InvalidUser` rejection for `.`, `..`, `/`, `\0`, empty in `PendingStore::user_path`. Test asserts. | +| Per-user u2f file path needs the user's home — fails for users without `getpwnam` entry (e.g., enrollment in CI). | Fall back to `/home//.config/Yubico/u2f_keys`. pam_u2f does the real lookup at auth, so the fallback only matters for write-time on a fresh enrollment, where the user does exist. | +| `std::mem::forget(watcher)` in main.rs is a deliberate leak. | Documented; daemon lifetime = process lifetime. Phase 5 may revisit if tests need teardown. | + +--- + +## Execution Handoff + +Plan complete and saved to `docs/plans/2026-04-27-phase-2-storage.md`. Phase 2 is broken into 17 bite-sized tasks. Independent file groups (pending / credentials / userdb in tasks 2.9–2.14) can be parallelized via subagents in the same worktree. + +Two execution options: + +**1. Subagent-Driven (this session)** — fresh subagent per task with two-stage review, parallelize 2.9 / 2.10–2.12 / 2.13–2.14 across 3 agents. + +**2. Parallel Session (separate)** — open a new session in a worktree using `superpowers:executing-plans`. + +Which approach?