From 9a3a6f9d5fae1c8258b40fee20755df7f8b899ab Mon Sep 17 00:00:00 2001 From: codaMW Date: Fri, 4 Sep 2026 15:52:55 +0200 Subject: [PATCH] feat(#141): persist backup_confirmed in the Rust identity record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves the backup-confirmed flag out of Dart SharedPreferences into the Rust identity record (Principle I), rebuilt cleanly on current main. Rust: - backup_confirmed on IdentityInfo (#[serde(default)] — legacy blobs load false). - get/set/reset with persist-then-commit (#217 discipline): save before committing in memory, so a failed save never reports a confirmed backup that didn't reach disk. - set requires durable storage only when confirming (true): a native memory-only session (initDb failed) must not report a non-durable confirm as success, or Dart burns the one-time migration marker against a write that evaporates on restart, permanently losing the flag (review). Reset (false) is fail-safe and does not require durability, so new-identity re-arm still works with no store. - restore_backup_confirmed guards on the public key so a blob from a different mnemonic can't leak its state. Dart: - BackupCompletedNotifier: native one-time migration (marker-guarded, coalesced load), native via the Rust bridge, web via SharedPreferences. - Web read falls back to the legacy dismissed key so a pre-migration confirmed web install isn't flipped to unconfirmed (review). - Both confirm sites do the Rust write before the permanent local dismissal. Tests: restore helper + serde default; persist-then-commit, the durable-storage gate, the no-store refusal and reset pinned in the identity_lock lifecycle test; Dart notifier (migration, coalesced load, native + web, dismissed-key fallback) and both screens. cargo test --lib green; clippy --locked clean; flutter analyze + account tests green. --- .../providers/backup_reminder_provider.dart | 158 +++++++-- .../account/screens/account_screen.dart | 6 +- .../account/screens/backup_ritual_screen.dart | 6 +- rust/src/api/identity.rs | 225 +++++++++++-- rust/src/api/types.rs | 7 + rust/src/db/sqlite.rs | 2 + .../backup_reminder_provider_test.dart | 311 +++++++++++++----- .../account/backup_ritual_screen_test.dart | 19 +- 8 files changed, 611 insertions(+), 123 deletions(-) diff --git a/lib/features/account/providers/backup_reminder_provider.dart b/lib/features/account/providers/backup_reminder_provider.dart index 59631982..5a957ed0 100644 --- a/lib/features/account/providers/backup_reminder_provider.dart +++ b/lib/features/account/providers/backup_reminder_provider.dart @@ -1,6 +1,9 @@ +import 'package:flutter/foundation.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:shared_preferences/shared_preferences.dart'; +import 'package:mostro/src/rust/api/identity.dart' as identity_api; + const kBackupReminderDismissedKey = 'backupReminderDismissed'; const kBackupReminderActiveKey = 'backupReminderActive'; @@ -19,8 +22,8 @@ const kBackupSnoozedUntilKey = 'backupSnoozedUntilMillis'; /// Dismissed permanently after `confirmBackupComplete()` is called. final backupReminderProvider = StateNotifierProvider( - (ref) => BackupReminderNotifier(), -); + (ref) => BackupReminderNotifier(), + ); /// Whether the user has ever completed a backup of the current identity. /// @@ -28,8 +31,8 @@ final backupReminderProvider = /// identity is generated or imported. final backupCompletedProvider = StateNotifierProvider( - (ref) => BackupCompletedNotifier(), -); + (ref) => BackupCompletedNotifier(), + ); class BackupReminderNotifier extends StateNotifier { /// When [initialValue] is provided the notifier starts with the correct @@ -112,7 +115,25 @@ class BackupReminderNotifier extends StateNotifier { } class BackupCompletedNotifier extends StateNotifier { - BackupCompletedNotifier({bool? initialValue}) : super(initialValue ?? false) { + /// The three bridge calls are injectable so the notifier is testable without + /// a live Rust runtime; they default to the real identity-bridge functions + /// (issue #141). + BackupCompletedNotifier({ + bool? initialValue, + Future Function()? getConfirmed, + Future Function(bool confirmed)? setConfirmed, + Future Function()? resetConfirmed, + // Test seam: force the web (SharedPreferences-authoritative) path off-web. + // Defaults to the real platform flag. + bool? isWebOverride, + }) : _getConfirmed = getConfirmed ?? identity_api.getBackupConfirmed, + _setConfirmed = + setConfirmed ?? + ((confirmed) => + identity_api.setBackupConfirmed(confirmed: confirmed)), + _resetConfirmed = resetConfirmed ?? identity_api.resetBackupConfirmation, + _isWeb = isWebOverride ?? kIsWeb, + super(initialValue ?? false) { if (initialValue == null) { load(); } else { @@ -120,33 +141,128 @@ class BackupCompletedNotifier extends StateNotifier { } } + final Future Function() _getConfirmed; + final Future Function(bool confirmed) _setConfirmed; + final Future Function() _resetConfirmed; + final bool _isWeb; + + // Web has no durable Rust identity store until #233, but SharedPreferences + // (backed by localStorage) IS durable there. So on web the backup-confirmed + // flag is read/written/cleared directly in kBackupCompletedKey, and the Rust + // bridge is used only on native. This keeps a confirmed backup surviving a + // page reload on web, instead of resetting to the session-only Rust default. + // (#141 review — CodeRabbit) + Future _readConfirmed() async { + if (_isWeb) { + final prefs = await SharedPreferences.getInstance(); + // Legacy web installs (before kBackupCompletedKey existed) only have the + // dismissed flag; fall back to it so a user who confirmed pre-migration + // isn't flipped to unconfirmed (#141 review). Matches main's non-web read + // this PR otherwise replaced. + return prefs.getBool(kBackupCompletedKey) ?? + prefs.getBool(kBackupReminderDismissedKey) ?? + false; + } + return _getConfirmed(); + } + + Future _writeConfirmed(bool confirmed) async { + if (_isWeb) { + final prefs = await SharedPreferences.getInstance(); + await prefs.setBool(kBackupCompletedKey, confirmed); + return; + } + await _setConfirmed(confirmed); + } + + Future _clearConfirmed() async { + if (_isWeb) { + final prefs = await SharedPreferences.getInstance(); + await prefs.setBool(kBackupCompletedKey, false); + return; + } + await _resetConfirmed(); + } + bool _loaded = false; - Future load() async { - if (_loaded) return; - final prefs = await SharedPreferences.getInstance(); - // Legacy installs only have the dismissed flag, which was set exclusively - // by the explicit "I have written down my secret words" confirmation — - // treat it as a completed backup. - state = prefs.getBool(kBackupCompletedKey) ?? - prefs.getBool(kBackupReminderDismissedKey) ?? - false; - _loaded = true; + /// Marks that the one-time SharedPreferences -> Rust migration has run, so + /// the legacy key is only ever read once (issue #141). + static const _kMigratedKey = 'backupCompletedMigratedToRust'; + + // Coalesce concurrent load()s: the constructor fires load() un-awaited, and a + // caller (or test) may await load() before it finishes. Without sharing the + // in-flight future, both could pass the _loaded check, run the one-time + // migration, and call _setConfirmed twice. Cleared on completion so a failed + // load (which leaves _loaded false) can be retried. (#141 review — CodeRabbit) + Future? _loading; + + Future load() { + if (_loaded) return Future.value(); + return _loading ??= _load().whenComplete(() => _loading = null); + } + + Future _load() async { + // The backup-confirmed flag now lives in the Rust identity record. On the + // first run after upgrading, copy the legacy SharedPreferences value into + // Rust once, then read from Rust exclusively. + try { + // #141 review: skip the migration entirely on web. There, initDb is + // never called (main.dart guards it with !kIsWeb), so set_backup_confirmed + // has no store and returns Ok WITHOUT persisting. Running the migration + // would burn the durable _kMigratedKey (localStorage) against that + // non-durable write, consuming the legacy SharedPreferences value and + // re-arming the reminder on every reload. Until IndexedDB save_identity + // lands (#233), the legacy SharedPreferences flag stays authoritative on + // web, so we neither migrate nor mark it migrated. + if (!_isWeb) { + final prefs = await SharedPreferences.getInstance(); + final migrated = prefs.getBool(_kMigratedKey) ?? false; + if (!migrated) { + // Legacy installs only have the dismissed flag, which was set + // exclusively by the explicit "I have written down my secret words" + // confirmation — treat it as a completed backup. + final legacy = + prefs.getBool(kBackupCompletedKey) ?? + prefs.getBool(kBackupReminderDismissedKey) ?? + false; + if (legacy) { + // Best-effort: if no identity is loaded yet, the bridge throws and + // we simply leave Rust at its default (false); the reminder stays + // armed, which is safe. The marker is only set once the copy + // sticks — and on native the write is always durable here. + await _setConfirmed(true); + } + await prefs.setBool(_kMigratedKey, true); + } + } + state = await _readConfirmed(); + // Only mark loaded once the read succeeded. If the bridge was not + // ready (no identity yet), leaving _loaded false lets the next load() + // retry instead of pinning the UI to `false` for the whole session. + _loaded = true; + } catch (e) { + // Rust unavailable (e.g. no identity yet, or tests without the bridge): + // fall back to unconfirmed so the reminder stays armed, and let a later + // load() retry (we deliberately do NOT set _loaded here). + debugPrint('[backup] load() failed, reminder stays armed: $e'); + state = false; + } } - /// Persist that the current identity has been backed up. + /// Persist that the current identity has been backed up (Rust identity + /// record, #141). Future markCompleted() async { await load(); - final prefs = await SharedPreferences.getInstance(); - await prefs.setBool(kBackupCompletedKey, true); + await _writeConfirmed(true); state = true; } - /// Clear the backed-up flag (new identity generated or imported). + /// Clear the backed-up flag (new identity generated or imported). The Rust + /// side is also reset in `create_identity`; this keeps the UI in sync (#141). Future reset() async { await load(); - final prefs = await SharedPreferences.getInstance(); - await prefs.setBool(kBackupCompletedKey, false); + await _clearConfirmed(); state = false; } } diff --git a/lib/features/account/screens/account_screen.dart b/lib/features/account/screens/account_screen.dart index f5c5b2b6..df124487 100644 --- a/lib/features/account/screens/account_screen.dart +++ b/lib/features/account/screens/account_screen.dart @@ -106,8 +106,12 @@ class _AccountScreenState extends ConsumerState { Future _confirmBackup() async { final l10n = AppLocalizations.of(context); try { - await ref.read(backupReminderProvider.notifier).confirmBackupComplete(); + // Authoritative Rust write first; dismiss the reminder locally only once + // it succeeds. If markCompleted() throws, the catch below fires before the + // permanent local dismissal, so the reminder stays armed and consistent + // with backup_confirmed=false (#141 review). await ref.read(backupCompletedProvider.notifier).markCompleted(); + await ref.read(backupReminderProvider.notifier).confirmBackupComplete(); if (mounted) setState(() => _showBackupCheckbox = false); } catch (e) { debugPrint('[account] _confirmBackup error: $e'); diff --git a/lib/features/account/screens/backup_ritual_screen.dart b/lib/features/account/screens/backup_ritual_screen.dart index 5d983799..7e39b9c0 100644 --- a/lib/features/account/screens/backup_ritual_screen.dart +++ b/lib/features/account/screens/backup_ritual_screen.dart @@ -213,8 +213,12 @@ class _BackupRitualScreenState extends ConsumerState { if (!_allCorrect || _confirming) return; setState(() => _confirming = true); try { - await ref.read(backupReminderProvider.notifier).confirmBackupComplete(); + // Authoritative Rust write first; dismiss the reminder locally only once + // it succeeds. If markCompleted() throws, the catch below fires before the + // permanent local dismissal, so the reminder stays armed and consistent + // with backup_confirmed=false (#141 review). await ref.read(backupCompletedProvider.notifier).markCompleted(); + await ref.read(backupReminderProvider.notifier).confirmBackupComplete(); if (mounted) setState(() => _step = 2); } catch (e) { debugPrint('[backup-ritual] confirm error: $e'); diff --git a/rust/src/api/identity.rs b/rust/src/api/identity.rs index 54ecdaaf..f2f21822 100644 --- a/rust/src/api/identity.rs +++ b/rust/src/api/identity.rs @@ -141,6 +141,9 @@ pub async fn create_identity() -> Result { privacy_mode: false, trade_key_index: 0, created_at: now, + // A freshly generated mnemonic is by definition not backed up yet, so + // the reminder starts armed (#141). + backup_confirmed: false, }; *guard = Some(IdentityState { @@ -211,6 +214,10 @@ pub async fn load_identity_from_mnemonic( privacy_mode, trade_key_index, created_at, + // Restore the persisted flag for THIS mnemonic; guarded on the public + // key inside restore_backup_confirmed so a blob from another mnemonic + // can't leak its state (#141). + backup_confirmed: restore_backup_confirmed(stored.as_ref(), &public_key), }; let mut guard = identity_lock().write().await; @@ -254,9 +261,88 @@ pub async fn import_from_mnemonic(words: Vec, recover: bool) -> Result, public_key: &str) -> bool { + match stored { + Some(info) if info.public_key == public_key => info.backup_confirmed, + _ => false, + } +} + +/// Read the current identity's backup-confirmed flag. `false` when no identity +/// is loaded (an unconfirmed backup keeps the reminder armed), so this never +/// fails on a missing identity (#141). +pub async fn get_backup_confirmed() -> Result { + let guard = identity_lock().read().await; + Ok(guard + .as_ref() + .map(|s| s.identity_info.backup_confirmed) + .unwrap_or(false)) +} + +/// Set the backup-confirmed flag and persist it to the identity record. +/// +/// Persist-then-commit (the `trade_key_index` discipline, #217): build the +/// updated record, save it, and only then commit in memory — so a save failure +/// never reports a confirmed backup that didn't reach disk. +/// +/// On native this REQUIRES durable storage (`require_durable_storage`), exactly +/// as `derive_trade_key` does: a memory-only session (initDb failed) must not +/// silently succeed, because Dart would then burn the one-time migration marker +/// against a write that evaporates on restart, permanently losing the confirmed +/// flag (#141 / review). Web has no store by design until #233 and is exempt — +/// `require_durable_storage` is a no-op there. +pub async fn set_backup_confirmed(confirmed: bool) -> Result<()> { + set_backup_confirmed_with(crate::db::app_db::db(), confirmed).await +} + +/// [`set_backup_confirmed`] against an explicit store, so the persist-then-commit +/// path is testable with an injected failing store without touching the global +/// singleton (mirrors [`derive_trade_key_with`]). +async fn set_backup_confirmed_with(db: Option<&S>, confirmed: bool) -> Result<()> { + let mut guard = identity_lock().write().await; + let state = guard.as_mut().ok_or_else(|| anyhow!("NoIdentity"))?; + if state.identity_info.backup_confirmed == confirmed { + return Ok(()); + } + // Require durable storage only when *confirming* (true). This is reached + // only on native (the Dart notifier routes web through SharedPreferences), + // so db == None means a native session whose initDb failed. Reporting Ok for + // a confirm then would let Dart burn the one-time migration marker against a + // write that evaporates on restart, permanently losing the flag (#141 review). + // + // Setting false (reset / re-arm the reminder) is the fail-safe direction — a + // non-durable write converges to the same unconfirmed state on the next + // restore — so it must NOT require durability, or the new-identity re-arm + // would fail on a memory-only native session. + if confirmed { + require_durable_storage(db)?; + } + let mut updated = state.identity_info.clone(); + updated.backup_confirmed = confirmed; + if let Some(db) = db { + db.save_identity(&updated).await.map_err(|e| { + anyhow!("StorageError: failed to persist backup_confirmed={confirmed}: {e}") + })?; + } + state.identity_info = updated; + Ok(()) +} + +/// Clear the backup-confirmed flag, re-arming the reminder. Used when a new +/// identity is generated (#141). A no-op when no identity is loaded. +pub async fn reset_backup_confirmation() -> Result<()> { + if get_identity().await?.is_none() { + return Ok(()); + } + set_backup_confirmed(false).await +} + pub async fn import_from_nsec(nsec: String) -> Result { - let keys = - Keys::parse(&nsec).map_err(|e| anyhow!("InvalidKey: {e}"))?; + let keys = Keys::parse(&nsec).map_err(|e| anyhow!("InvalidKey: {e}"))?; let public_key = keys.public_key().to_hex(); let now = unix_now(); @@ -266,6 +352,9 @@ pub async fn import_from_nsec(nsec: String) -> Result { privacy_mode: false, trade_key_index: 0, created_at: now, + // Importing recovery words is not the in-app verification ritual, so an + // imported identity stays unconfirmed (#141). + backup_confirmed: false, }; let mut guard = identity_lock().write().await; @@ -528,7 +617,10 @@ pub async fn get_trade_key(index: u32) -> Result { } if index > state.identity_info.trade_key_index { - bail!("InvalidIndex: {index} exceeds current trade_key_index {}", state.identity_info.trade_key_index); + bail!( + "InvalidIndex: {index} exceeds current trade_key_index {}", + state.identity_info.trade_key_index + ); } let trade_keys = key_ops::derive_trade_key(&state.mnemonic_words, index)?; @@ -602,11 +694,7 @@ pub async fn export_encrypted_backup(passphrase: String) -> Result { /// means re-deriving already-consumed keys, which the daemon rejects with /// `InvalidTradeIndex`. A stored identity with a different public key is /// ignored: its counter belongs to another mnemonic. -fn reconcile_trade_key_index( - passed: u32, - stored: Option<&IdentityInfo>, - public_key: &str, -) -> u32 { +fn reconcile_trade_key_index(passed: u32, stored: Option<&IdentityInfo>, public_key: &str) -> u32 { match stored { Some(info) if info.public_key == public_key => passed.max(info.trade_key_index), _ => passed, @@ -683,8 +771,8 @@ mod tests { /// A throwaway SQLite store, named per test so parallel runs never collide. async fn temp_store(tag: &str) -> crate::db::sqlite::SqliteStorage { - let path = std::env::temp_dir() - .join(format!("mostro_identity_{tag}_{}.db", std::process::id())); + let path = + std::env::temp_dir().join(format!("mostro_identity_{tag}_{}.db", std::process::id())); let _ = std::fs::remove_file(&path); crate::db::sqlite::SqliteStorage::open(path.to_str().unwrap()) .await @@ -698,6 +786,7 @@ mod tests { privacy_mode: false, trade_key_index, created_at: 1, + backup_confirmed: false, } } @@ -713,6 +802,34 @@ mod tests { /// resync rollback path with an injected failure. The seam under test only /// calls `save_identity`, so every other method is `unimplemented!()` — /// reaching one would be a test bug, not silent success. + // ── backup_confirmed restore (#141) ─────────────────────────────────────── + #[test] + fn restore_reads_the_persisted_backup_flag_for_the_same_identity() { + let mut stored = stored_identity("abc", 4); + stored.backup_confirmed = true; + assert!(restore_backup_confirmed(Some(&stored), "abc")); + } + #[test] + fn restore_defaults_to_unconfirmed_when_nothing_is_persisted() { + assert!(!restore_backup_confirmed(None, "abc")); + } + #[test] + fn restore_ignores_a_backup_flag_from_another_identity() { + // A leftover blob from a previous mnemonic must not mark the new + // identity as backed up (#141 cross-identity guard). + let mut stored = stored_identity("other-pubkey", 0); + stored.backup_confirmed = true; + assert!(!restore_backup_confirmed(Some(&stored), "abc")); + } + #[test] + fn an_identity_persisted_before_the_field_deserializes_as_unconfirmed() { + // Serde default: an identity blob written before backup_confirmed + // existed must load as `false` (reminder armed), not error. + let legacy = r#"{"public_key":"abc","display_name":null,"privacy_mode":false,"trade_key_index":3,"created_at":1}"#; + let info: IdentityInfo = serde_json::from_str(legacy).unwrap(); + assert!(!info.backup_confirmed); + } + struct FailingStore; impl Storage for FailingStore { @@ -788,9 +905,7 @@ mod tests { ) -> Result<()> { unimplemented!() } - async fn list_queued_messages( - &self, - ) -> Result> { + async fn list_queued_messages(&self) -> Result> { unimplemented!() } async fn update_queued_message_status( @@ -903,7 +1018,10 @@ mod tests { // Already in sync, and a counter belonging to another mnemonic: no // publication, so Dart never rewrites a value it already holds. assert_eq!(reconcile_and_publish_to(&tx, 22, Some(&stored), "abc"), 22); - assert_eq!(reconcile_and_publish_to(&tx, 30, Some(&stored), "other"), 30); + assert_eq!( + reconcile_and_publish_to(&tx, 30, Some(&stored), "other"), + 30 + ); assert!( stream.rx.try_recv().is_err(), "nothing further should have been published" @@ -969,23 +1087,80 @@ mod tests { let current = get_identity().await.unwrap().unwrap(); assert_eq!(current.trade_key_index, 22); + // ── backup_confirmed: persist-then-commit + durable-storage gate (#141) ── + // Folded into this one identity_lock test on purpose: a separate + // #[tokio::test] touching the singleton would race it. + assert!(!current.backup_confirmed); + // B1 (#141 review): a confirm with NO durable store must be refused, not + // silently succeed — otherwise the Dart migration marker is burned + // against a write that evaporates on restart, permanently losing the flag. + assert!( + set_backup_confirmed_with(None::<&crate::db::sqlite::SqliteStorage>, true) + .await + .is_err(), + "a confirm with no durable store must fail, not silently succeed", + ); + assert!( + !get_identity().await.unwrap().unwrap().backup_confirmed, + "the refused confirm must not flip the in-memory flag", + ); + // A failing store errors with the StorageError marker and leaves the flag + // unchanged — the persist happens before the commit (pins the ordering). + let backup_err = set_backup_confirmed_with(Some(&FailingStore), true) + .await + .unwrap_err() + .to_string(); + assert!( + backup_err.contains("StorageError:"), + "unexpected error: {backup_err}" + ); + assert!( + !get_identity().await.unwrap().unwrap().backup_confirmed, + "a failed persist must not flip the in-memory backup flag", + ); + // Retry against the working store writes: the short-circuit was not poisoned. + set_backup_confirmed_with(Some(&db), true).await.unwrap(); + assert!( + get_identity().await.unwrap().unwrap().backup_confirmed, + "retry against a working store must persist the flag", + ); + assert!( + db.get_identity().await.unwrap().unwrap().backup_confirmed, + "the working store must hold the confirmed flag durably", + ); + // reset_backup_confirmation re-arms the reminder (delegates to set(false)). + reset_backup_confirmation().await.unwrap(); + assert!( + !get_identity().await.unwrap().unwrap().backup_confirmed, + "reset must clear the in-memory flag", + ); + // #217 resync — asserted here (not a separate #[tokio::test]) so it // shares the single identity_lock lifecycle and can't race it. Uses the // `_with` core so publications land on this test's private channel. // Never lowers: a floor below current is a no-op — no write, no publish. - ensure_trade_key_index_at_least_with(Some(&db), &tx, 10).await.unwrap(); + ensure_trade_key_index_at_least_with(Some(&db), &tx, 10) + .await + .unwrap(); assert_eq!(get_identity().await.unwrap().unwrap().trade_key_index, 22); assert!( published.rx.try_recv().is_err(), "a no-op resync must not publish", ); // Raises to the recovered max, persists, and publishes to the mirror. - ensure_trade_key_index_at_least_with(Some(&db), &tx, 50).await.unwrap(); + ensure_trade_key_index_at_least_with(Some(&db), &tx, 50) + .await + .unwrap(); assert_eq!(get_identity().await.unwrap().unwrap().trade_key_index, 50); assert_eq!(published.next().await.unwrap(), 50); - assert_eq!(db.get_identity().await.unwrap().unwrap().trade_key_index, 50); + assert_eq!( + db.get_identity().await.unwrap().unwrap().trade_key_index, + 50 + ); // Idempotent: the same floor again changes nothing and publishes nothing. - ensure_trade_key_index_at_least_with(Some(&db), &tx, 50).await.unwrap(); + ensure_trade_key_index_at_least_with(Some(&db), &tx, 50) + .await + .unwrap(); assert_eq!(get_identity().await.unwrap().unwrap().trade_key_index, 50); assert!( published.rx.try_recv().is_err(), @@ -995,11 +1170,10 @@ mod tests { // counter advanced. Counter is 50 here. Ask for a higher floor (60) // against a failing store: the call errors and the counter stays 50. let (fx, _frx) = private_channel(); - let rollback_err = - ensure_trade_key_index_at_least_with(Some(&FailingStore), &fx, 60) - .await - .unwrap_err() - .to_string(); + let rollback_err = ensure_trade_key_index_at_least_with(Some(&FailingStore), &fx, 60) + .await + .unwrap_err() + .to_string(); assert!( rollback_err.contains("StorageError:"), "unexpected error: {rollback_err}" @@ -1020,7 +1194,10 @@ mod tests { "retry against a working store must raise and persist", ); assert_eq!(published.next().await.unwrap(), 60); - assert_eq!(db.get_identity().await.unwrap().unwrap().trade_key_index, 60); + assert_eq!( + db.get_identity().await.unwrap().unwrap().trade_key_index, + 60 + ); // Regression (the bug #217 fixes): the next derived key is FRESH — // index 61, past every recovered trade — not a reused recovered index. diff --git a/rust/src/api/types.rs b/rust/src/api/types.rs index 985aeeb9..b6869705 100644 --- a/rust/src/api/types.rs +++ b/rust/src/api/types.rs @@ -361,6 +361,13 @@ pub struct IdentityInfo { pub privacy_mode: bool, pub trade_key_index: u32, pub created_at: i64, + /// Whether the user has confirmed a backup of the current identity's secret + /// words (issue #141 — migrated out of Dart SharedPreferences into the Rust + /// identity record per Principle I). `#[serde(default)]` so identities + /// persisted before this field deserialize as `false` — an unconfirmed + /// backup, which correctly keeps the reminder armed. + #[serde(default)] + pub backup_confirmed: bool, } /// Deterministic pseudonymous identity derived from a public key. diff --git a/rust/src/db/sqlite.rs b/rust/src/db/sqlite.rs index b7f8ba6b..d1963c9c 100644 --- a/rust/src/db/sqlite.rs +++ b/rust/src/db/sqlite.rs @@ -1419,6 +1419,7 @@ mod tests { privacy_mode: false, trade_key_index: 21, created_at: 1_700_000_000, + backup_confirmed: false, }; storage.save_identity(&identity).await.unwrap(); let loaded = storage.get_identity().await.unwrap().unwrap(); @@ -1447,6 +1448,7 @@ mod tests { privacy_mode: false, trade_key_index: 7, created_at: 1_700_000_000, + backup_confirmed: false, }; storage.save_identity(&identity).await.unwrap(); storage.save_trade_key("order-1", 5).await.unwrap(); diff --git a/test/features/account/backup_reminder_provider_test.dart b/test/features/account/backup_reminder_provider_test.dart index ffdaeced..f3f73ac4 100644 --- a/test/features/account/backup_reminder_provider_test.dart +++ b/test/features/account/backup_reminder_provider_test.dart @@ -61,59 +61,65 @@ void main() { expect(notifier.state, isTrue); }); - test('showBackupReminder(): arms the badge and clears prior state', - () async { - SharedPreferences.setMockInitialValues({ - kBackupReminderDismissedKey: true, - kBackupCompletedKey: true, - kBackupSnoozedUntilKey: _inFuture(const Duration(days: 1)), - }); - - final notifier = BackupReminderNotifier(); - await notifier.showBackupReminder(); - - expect(notifier.state, isTrue); - final prefs = await _prefs(); - expect(prefs.getBool(kBackupReminderActiveKey), isTrue); - expect(prefs.getBool(kBackupReminderDismissedKey), isFalse); - expect(prefs.getBool(kBackupCompletedKey), isFalse); - expect(prefs.getInt(kBackupSnoozedUntilKey), isNull); - }); - - test('snoozeUntilTomorrow(): hides badge and persists a future snooze', - () async { - SharedPreferences.setMockInitialValues({ - kBackupReminderActiveKey: true, - kBackupReminderDismissedKey: false, - }); - - final notifier = BackupReminderNotifier(); - await notifier.snoozeUntilTomorrow(); - - expect(notifier.state, isFalse); - final prefs = await _prefs(); - final until = prefs.getInt(kBackupSnoozedUntilKey); - expect(until, isNotNull); - expect(until, greaterThan(DateTime.now().millisecondsSinceEpoch)); - }); - - test('confirmBackupComplete(): permanently dismisses the reminder', - () async { - SharedPreferences.setMockInitialValues({ - kBackupReminderActiveKey: true, - kBackupReminderDismissedKey: false, - kBackupSnoozedUntilKey: _inFuture(const Duration(days: 1)), - }); - - final notifier = BackupReminderNotifier(); - await notifier.confirmBackupComplete(); - - expect(notifier.state, isFalse); - final prefs = await _prefs(); - expect(prefs.getBool(kBackupReminderDismissedKey), isTrue); - expect(prefs.getBool(kBackupCompletedKey), isTrue); - expect(prefs.getInt(kBackupSnoozedUntilKey), isNull); - }); + test( + 'showBackupReminder(): arms the badge and clears prior state', + () async { + SharedPreferences.setMockInitialValues({ + kBackupReminderDismissedKey: true, + kBackupCompletedKey: true, + kBackupSnoozedUntilKey: _inFuture(const Duration(days: 1)), + }); + + final notifier = BackupReminderNotifier(); + await notifier.showBackupReminder(); + + expect(notifier.state, isTrue); + final prefs = await _prefs(); + expect(prefs.getBool(kBackupReminderActiveKey), isTrue); + expect(prefs.getBool(kBackupReminderDismissedKey), isFalse); + expect(prefs.getBool(kBackupCompletedKey), isFalse); + expect(prefs.getInt(kBackupSnoozedUntilKey), isNull); + }, + ); + + test( + 'snoozeUntilTomorrow(): hides badge and persists a future snooze', + () async { + SharedPreferences.setMockInitialValues({ + kBackupReminderActiveKey: true, + kBackupReminderDismissedKey: false, + }); + + final notifier = BackupReminderNotifier(); + await notifier.snoozeUntilTomorrow(); + + expect(notifier.state, isFalse); + final prefs = await _prefs(); + final until = prefs.getInt(kBackupSnoozedUntilKey); + expect(until, isNotNull); + expect(until, greaterThan(DateTime.now().millisecondsSinceEpoch)); + }, + ); + + test( + 'confirmBackupComplete(): permanently dismisses the reminder', + () async { + SharedPreferences.setMockInitialValues({ + kBackupReminderActiveKey: true, + kBackupReminderDismissedKey: false, + kBackupSnoozedUntilKey: _inFuture(const Duration(days: 1)), + }); + + final notifier = BackupReminderNotifier(); + await notifier.confirmBackupComplete(); + + expect(notifier.state, isFalse); + final prefs = await _prefs(); + expect(prefs.getBool(kBackupReminderDismissedKey), isTrue); + expect(prefs.getBool(kBackupCompletedKey), isTrue); + expect(prefs.getInt(kBackupSnoozedUntilKey), isNull); + }, + ); test('initialValue with a live snooze is reconciled to off', () async { SharedPreferences.setMockInitialValues({ @@ -128,47 +134,204 @@ void main() { }); }); - group('BackupCompletedNotifier', () { - test('load(): reads the explicit completed flag', () async { - SharedPreferences.setMockInitialValues({kBackupCompletedKey: true}); + group('BackupCompletedNotifier (backed by the Rust bridge, #141)', () { + // A fake identity-bridge backing store so the notifier is exercised without + // a live Rust runtime. + BackupCompletedNotifier makeNotifier({required bool initial}) { + var confirmed = initial; + return BackupCompletedNotifier( + getConfirmed: () async => confirmed, + setConfirmed: (v) async => confirmed = v, + resetConfirmed: () async => confirmed = false, + isWebOverride: false, + ); + } + + test('load(): reads the confirmed flag from the bridge', () async { + SharedPreferences.setMockInitialValues({ + 'backupCompletedMigratedToRust': true, + }); - final notifier = BackupCompletedNotifier(); + final notifier = makeNotifier(initial: true); await notifier.load(); expect(notifier.state, isTrue); }); - test('load(): legacy installs fall back to the dismissed flag', () async { + test( + 'load(): migrates a legacy SharedPreferences flag into the bridge once', + () async { + // Legacy install: completed flag set, no migration marker. load() copies + // it into the bridge, marks the migration done, then reads the bridge. + SharedPreferences.setMockInitialValues({kBackupCompletedKey: true}); + + var confirmed = false; + final notifier = BackupCompletedNotifier( + getConfirmed: () async => confirmed, + setConfirmed: (v) async => confirmed = v, + resetConfirmed: () async => confirmed = false, + isWebOverride: false, + ); + await notifier.load(); + + expect(confirmed, isTrue, reason: 'legacy flag copied into the bridge'); + expect(notifier.state, isTrue); + final prefs = await _prefs(); + expect(prefs.getBool('backupCompletedMigratedToRust'), isTrue); + }, + ); + + test( + 'load(): concurrent calls run the migration write exactly once', + () async { + // The constructor fires load() un-awaited; a caller may await load() + // before it finishes. Both must share one in-flight future so the + // one-time migration calls the bridge exactly once. (#141 review) + SharedPreferences.setMockInitialValues({kBackupCompletedKey: true}); + var setCount = 0; + final notifier = BackupCompletedNotifier( + getConfirmed: () async => setCount > 0, + setConfirmed: (v) async => setCount++, + resetConfirmed: () async {}, + isWebOverride: false, + ); + // Two overlapping loads (plus the un-awaited one from the constructor). + await Future.wait([notifier.load(), notifier.load()]); + expect( + setCount, + 1, + reason: 'migration must write through the bridge exactly once', + ); + expect(notifier.state, isTrue); + }, + ); + + test('markCompleted() writes true through the bridge', () async { SharedPreferences.setMockInitialValues({ - kBackupReminderDismissedKey: true, + 'backupCompletedMigratedToRust': true, }); - final notifier = BackupCompletedNotifier(); - await notifier.load(); + // Track the fake bridge's backing value so we assert the write reached it, + // not only that notifier.state flipped. + var confirmed = false; + final notifier = BackupCompletedNotifier( + getConfirmed: () async => confirmed, + setConfirmed: (v) async => confirmed = v, + resetConfirmed: () async => confirmed = false, + isWebOverride: false, + ); + await notifier.markCompleted(); + expect( + confirmed, + isTrue, + reason: 'markCompleted must write to the bridge', + ); expect(notifier.state, isTrue); }); - test('markCompleted() persists and flips state on', () async { - SharedPreferences.setMockInitialValues({}); + test('reset() clears the flag through the bridge', () async { + SharedPreferences.setMockInitialValues({ + 'backupCompletedMigratedToRust': true, + }); - final notifier = BackupCompletedNotifier(); - await notifier.markCompleted(); + var confirmed = true; + final notifier = BackupCompletedNotifier( + getConfirmed: () async => confirmed, + setConfirmed: (v) async => confirmed = v, + resetConfirmed: () async => confirmed = false, + isWebOverride: false, + ); + await notifier.reset(); - expect(notifier.state, isTrue); - final prefs = await _prefs(); - expect(prefs.getBool(kBackupCompletedKey), isTrue); + expect(confirmed, isFalse, reason: 'reset must clear the bridge value'); + expect(notifier.state, isFalse); }); + }); - test('reset() clears the backed-up flag', () async { + group('BackupCompletedNotifier — web (SharedPreferences authoritative, #233)', () { + // On web the Rust identity store is a stub (#233), so SharedPreferences + // (localStorage) is the durable source for the backup-confirmed flag. + // These force the web path via isWebOverride and assert the flag round-trips + // through SharedPreferences across a simulated reload. (#141 review) + test( + 'web: markCompleted persists to SharedPreferences and survives reload', + () async { + SharedPreferences.setMockInitialValues({}); + final n1 = BackupCompletedNotifier(isWebOverride: true); + await n1.load(); + expect( + n1.state, + isFalse, + reason: 'fresh web install starts unconfirmed', + ); + await n1.markCompleted(); + expect(n1.state, isTrue); + final prefs = await _prefs(); + expect( + prefs.getBool(kBackupCompletedKey), + isTrue, + reason: 'web write must reach SharedPreferences', + ); + // Simulate a page reload: a brand-new notifier reads the durable value. + final n2 = BackupCompletedNotifier(isWebOverride: true); + await n2.load(); + expect( + n2.state, + isTrue, + reason: 'a confirmed backup must survive a web reload', + ); + }, + ); + + test('web: reset clears the SharedPreferences flag', () async { SharedPreferences.setMockInitialValues({kBackupCompletedKey: true}); + final n = BackupCompletedNotifier(isWebOverride: true); + await n.load(); + expect(n.state, isTrue); + await n.reset(); + expect(n.state, isFalse); + final prefs = await _prefs(); + expect( + prefs.getBool(kBackupCompletedKey), + isFalse, + reason: 'web reset must clear the durable flag', + ); + }); - final notifier = BackupCompletedNotifier(); - await notifier.reset(); - - expect(notifier.state, isFalse); + test('web: load does not run the native migration', () async { + // A legacy completed flag with no migration marker: on web we must NOT + // consume it via migration; SharedPreferences stays authoritative and the + // migration marker is never written. + SharedPreferences.setMockInitialValues({kBackupCompletedKey: true}); + final n = BackupCompletedNotifier(isWebOverride: true); + await n.load(); + expect(n.state, isTrue, reason: 'web reads the flag directly'); final prefs = await _prefs(); - expect(prefs.getBool(kBackupCompletedKey), isFalse); + expect( + prefs.getBool('backupCompletedMigratedToRust'), + isNull, + reason: 'web must not set the migration marker', + ); }); + test( + 'web: read falls back to the legacy dismissed flag (#141 B2)', + () async { + // A web install that confirmed before kBackupCompletedKey existed has + // only kBackupReminderDismissedKey set. The web read must honor it, not + // flip the user to unconfirmed — the regression this fixes. + SharedPreferences.setMockInitialValues({ + kBackupReminderDismissedKey: true, + // kBackupCompletedKey deliberately absent. + }); + final n = BackupCompletedNotifier(isWebOverride: true); + await n.load(); + expect( + n.state, + isTrue, + reason: 'a legacy confirmed web install must not be flipped to false', + ); + }, + ); }); } diff --git a/test/features/account/backup_ritual_screen_test.dart b/test/features/account/backup_ritual_screen_test.dart index 4d93b80f..dbc5bd7e 100644 --- a/test/features/account/backup_ritual_screen_test.dart +++ b/test/features/account/backup_ritual_screen_test.dart @@ -3,6 +3,7 @@ import 'package:flutter_localizations/flutter_localizations.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:shared_preferences/shared_preferences.dart'; +import 'package:mostro/features/account/providers/backup_reminder_provider.dart'; import 'package:mostro/features/account/screens/backup_ritual_screen.dart'; import 'package:mostro/l10n/app_localizations.dart'; @@ -18,8 +19,22 @@ Future _pumpRitual(WidgetTester tester) async { addTearDown(tester.view.resetDevicePixelRatio); await tester.pumpWidget( - const ProviderScope( - child: MaterialApp( + ProviderScope( + overrides: [ + // The backup-completed flag persists through the Rust identity bridge + // (#141), which is unavailable under flutter_test — back it with an + // in-memory fake so tapping "confirm" doesn't hit the real bridge. + backupCompletedProvider.overrideWith((ref) { + var confirmed = false; + return BackupCompletedNotifier( + initialValue: false, + getConfirmed: () async => confirmed, + setConfirmed: (v) async => confirmed = v, + resetConfirmed: () async => confirmed = false, + ); + }), + ], + child: const MaterialApp( locale: Locale('en'), localizationsDelegates: [ AppLocalizations.delegate,