From 64462e9fd3c95db0ea052c97106394b6eaa08058 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Fri, 28 Aug 2026 23:25:45 +0200 Subject: [PATCH] Test the folder sign-in instead of trusting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The whole of a folder library's sign-in lived inside a Slint callback, which cannot run without a display server — so the one path that decides whether a mistyped folder becomes a *stored* account had no test at all. That failure is quiet and lasting: an account for a directory that is not there skips the launch screen on the next start and reads as a library that has lost its photographs. `open_folder_library` is that logic, lifted out whole. Four tests: it stores an account with no credential and reaches the keyring for nothing, a typo is refused before anything is written, the messages read as instructions because they go straight to the screen's error line, and a file is not a library. --- docs/traceability.md | 6 +- ui/dr-ui/src/launch_ui.rs | 157 ++++++++++++++++++++++++++++---------- 2 files changed, 121 insertions(+), 42 deletions(-) diff --git a/docs/traceability.md b/docs/traceability.md index 73d80dd..2346a68 100644 --- a/docs/traceability.md +++ b/docs/traceability.md @@ -9,8 +9,8 @@ Denominators are parsed from [`requirements.md`](requirements.md) at run time, n | Metric | Value | |---|---| -| Source files scanned | 282 | -| TRACES tags found | 822 | +| Source files scanned | 283 | +| TRACES tags found | 823 | | Requirements defined | 178 | | Requirements covered | 107 | | **Coverage** | **60.1%** (107/178) | @@ -88,7 +88,7 @@ _None._ | FR-NC-1 | [`core/dr-sync-nextcloud/src/auth.rs:132`](../core/dr-sync-nextcloud/src/auth.rs#L132), [`core/dr-sync-nextcloud/src/auth.rs:44`](../core/dr-sync-nextcloud/src/auth.rs#L44), [`core/dr-sync-nextcloud/src/provider.rs:1`](../core/dr-sync-nextcloud/src/provider.rs#L1), [`core/dr-sync/src/account.rs:355`](../core/dr-sync/src/account.rs#L355), [`ui/dr-ui/src/launch.rs:277`](../ui/dr-ui/src/launch.rs#L277), [`ui/dr-ui/src/launch.rs:61`](../ui/dr-ui/src/launch.rs#L61), [`ui/dr-ui/src/launch_ui.rs:417`](../ui/dr-ui/src/launch_ui.rs#L417) | | FR-NC-10 | [`core/dr-sync/src/account.rs:220`](../core/dr-sync/src/account.rs#L220), [`ui/dr-ui/src/export.rs:1`](../ui/dr-ui/src/export.rs#L1), [`ui/dr-ui/src/lib.rs:424`](../ui/dr-ui/src/lib.rs#L424), [`ui/dr-ui/src/library.rs:1040`](../ui/dr-ui/src/library.rs#L1040), [`ui/dr-ui/src/library.rs:1677`](../ui/dr-ui/src/library.rs#L1677), [`ui/dr-ui/src/library.rs:543`](../ui/dr-ui/src/library.rs#L543), [`ui/dr-ui/src/library.rs:844`](../ui/dr-ui/src/library.rs#L844), [`ui/dr-ui/src/library_ui.rs:1605`](../ui/dr-ui/src/library_ui.rs#L1605), [`ui/dr-ui/src/library_ui.rs:3358`](../ui/dr-ui/src/library_ui.rs#L3358), [`ui/dr-ui/src/library_ui.rs:504`](../ui/dr-ui/src/library_ui.rs#L504), [`ui/dr-ui/src/sidecar_cache.rs:1`](../ui/dr-ui/src/sidecar_cache.rs#L1) | | FR-NC-12 | [`core/dr-sync-folder/src/lib.rs:1`](../core/dr-sync-folder/src/lib.rs#L1), [`core/dr-sync-nextcloud/src/lib.rs:39`](../core/dr-sync-nextcloud/src/lib.rs#L39), [`core/dr-sync-nextcloud/src/lib.rs:909`](../core/dr-sync-nextcloud/src/lib.rs#L909), [`core/dr-sync-nextcloud/src/provider.rs:1`](../core/dr-sync-nextcloud/src/provider.rs#L1), [`core/dr-sync/src/account.rs:1`](../core/dr-sync/src/account.rs#L1), [`core/dr-sync/src/account.rs:81`](../core/dr-sync/src/account.rs#L81), [`core/dr-sync/src/lib.rs:166`](../core/dr-sync/src/lib.rs#L166), [`core/dr-sync/src/lib.rs:49`](../core/dr-sync/src/lib.rs#L49), [`core/dr-sync/src/provider.rs:106`](../core/dr-sync/src/provider.rs#L106), [`core/dr-sync/src/provider.rs:1`](../core/dr-sync/src/provider.rs#L1), [`core/dr-sync/src/provider.rs:53`](../core/dr-sync/src/provider.rs#L53), [`core/dr-sync/src/reachability.rs:1`](../core/dr-sync/src/reachability.rs#L1), [`ui/dr-ui/src/remote.rs:1`](../ui/dr-ui/src/remote.rs#L1) | -| FR-NC-13 | [`core/dr-sync-folder/src/lib.rs:143`](../core/dr-sync-folder/src/lib.rs#L143), [`core/dr-sync-folder/src/lib.rs:1`](../core/dr-sync-folder/src/lib.rs#L1), [`core/dr-sync-folder/src/lib.rs:65`](../core/dr-sync-folder/src/lib.rs#L65), [`core/dr-sync/src/provider.rs:106`](../core/dr-sync/src/provider.rs#L106), [`ui/dr-ui/src/remote.rs:1`](../ui/dr-ui/src/remote.rs#L1) | +| FR-NC-13 | [`core/dr-sync-folder/src/lib.rs:143`](../core/dr-sync-folder/src/lib.rs#L143), [`core/dr-sync-folder/src/lib.rs:1`](../core/dr-sync-folder/src/lib.rs#L1), [`core/dr-sync-folder/src/lib.rs:65`](../core/dr-sync-folder/src/lib.rs#L65), [`core/dr-sync/src/provider.rs:106`](../core/dr-sync/src/provider.rs#L106), [`ui/dr-ui/src/launch_ui.rs:316`](../ui/dr-ui/src/launch_ui.rs#L316), [`ui/dr-ui/src/remote.rs:1`](../ui/dr-ui/src/remote.rs#L1) | | FR-NC-2 | [`core/dr-sync/src/account.rs:1`](../core/dr-sync/src/account.rs#L1), [`core/dr-sync/src/account.rs:298`](../core/dr-sync/src/account.rs#L298), [`core/dr-sync/src/account.rs:355`](../core/dr-sync/src/account.rs#L355), [`core/dr-sync/src/account.rs:59`](../core/dr-sync/src/account.rs#L59), [`platform/dr-plat/src/secrets.rs:82`](../platform/dr-plat/src/secrets.rs#L82) | | FR-NC-3 | [`core/dr-decode/src/locate.rs:1`](../core/dr-decode/src/locate.rs#L1), [`core/dr-decode/src/preview.rs:148`](../core/dr-decode/src/preview.rs#L148), [`core/dr-sync/src/capability.rs:41`](../core/dr-sync/src/capability.rs#L41), [`core/dr-thumbs/src/lib.rs:1`](../core/dr-thumbs/src/lib.rs#L1), [`ui/dr-ui/src/library.rs:1`](../ui/dr-ui/src/library.rs#L1), [`ui/dr-ui/src/library.rs:2702`](../ui/dr-ui/src/library.rs#L2702), [`ui/dr-ui/src/library.rs:3172`](../ui/dr-ui/src/library.rs#L3172), [`ui/dr-ui/src/library_ui.rs:1`](../ui/dr-ui/src/library_ui.rs#L1), [`ui/dr-ui/src/library_ui.rs:3610`](../ui/dr-ui/src/library_ui.rs#L3610), [`ui/dr-ui/src/library_ui.rs:4575`](../ui/dr-ui/src/library_ui.rs#L4575), [`ui/dr-ui/ui/app.slint:319`](../ui/dr-ui/ui/app.slint#L319), [`ui/dr-ui/ui/settings.slint:372`](../ui/dr-ui/ui/settings.slint#L372), [`ui/dr-ui/ui/settings.slint:72`](../ui/dr-ui/ui/settings.slint#L72) | | FR-NC-4 | [`core/dr-sync-folder/src/lib.rs:143`](../core/dr-sync-folder/src/lib.rs#L143), [`core/dr-sync-nextcloud/src/propfind.rs:100`](../core/dr-sync-nextcloud/src/propfind.rs#L100), [`core/dr-sync-nextcloud/src/propfind.rs:51`](../core/dr-sync-nextcloud/src/propfind.rs#L51), [`core/dr-sync/src/capability.rs:6`](../core/dr-sync/src/capability.rs#L6), [`core/dr-sync/src/lib.rs:166`](../core/dr-sync/src/lib.rs#L166), [`core/dr-sync/src/scan.rs:93`](../core/dr-sync/src/scan.rs#L93), [`ui/dr-ui/src/launch.rs:61`](../ui/dr-ui/src/launch.rs#L61) | diff --git a/ui/dr-ui/src/launch_ui.rs b/ui/dr-ui/src/launch_ui.rs index 9b6c9cf..8ef0dd0 100644 --- a/ui/dr-ui/src/launch_ui.rs +++ b/ui/dr-ui/src/launch_ui.rs @@ -155,47 +155,13 @@ where let ctl = controller.clone(); window.on_launch_use_folder(move |path| { let Some(w) = weak.upgrade() else { return }; - - let provider = match crate::remote::registry().get(dr_sync_folder::BACKEND_ID) { - Some(p) => p.clone(), - None => { - ctl.model - .borrow_mut() - .fail("this build has no folder support"); - render(&w, &ctl); - return; + match open_folder_library(&ctl.store, &path) { + Ok(account) => { + log::info!("using folder library at {}", account.endpoint); + ctl.model.borrow_mut().signed_in(account); } - }; - - // The connector checks the directory before an account is written - // for it. A typo stored here would skip the launch screen next - // start and surface as a library that finds nothing. - let endpoint = match provider.normalise_endpoint(&path) { - Ok(e) => e, - Err(e) => { - ctl.model.borrow_mut().fail(e); - render(&w, &ctl); - return; - } - }; - - let account = match provider.account_for(&endpoint) { - Ok(a) => a, - Err(e) => { - ctl.model.borrow_mut().fail(e.to_string()); - render(&w, &ctl); - return; - } - }; - - // `None`: there is no credential, and asking the keyring for one - // would fail on a machine with no secrets daemon — where a folder - // library is exactly the thing that should still work. - if let Err(e) = ctl.store.save(&account, None) { - log::warn!("persisting account: {e}"); + Err(e) => ctl.model.borrow_mut().fail(e), } - log::info!("using folder library at {}", account.endpoint); - ctl.model.borrow_mut().signed_in(account); render(&w, &ctl); }); } @@ -347,6 +313,40 @@ where render(window, &controller); } +/// TRACES: FR-NC-13 +/// Establish a folder library, returning the account to sign in as. +/// +/// The whole of a [`SignIn::EndpointOnly`](dr_sync::SignIn) sign-in: check the +/// endpoint, build the account, persist it. Split out of the callback rather +/// than written inline because a Slint callback cannot be tested without a +/// display server, and this is the path that decides whether a mistyped folder +/// becomes a stored account — the failure that would then skip the launch +/// screen on the next start and surface as a library that finds nothing. +/// +/// The error is a string because it goes straight to the screen's error line; +/// the connector wrote it to say what to fix. +fn open_folder_library(store: &AccountStore, path: &str) -> Result { + let provider = crate::remote::registry() + .get(dr_sync_folder::BACKEND_ID) + .ok_or("this build has no folder support")?; + + // The connector checks the directory before an account is written for it. + let endpoint = provider.normalise_endpoint(path)?; + let account = provider.account_for(&endpoint).map_err(|e| e.to_string())?; + + // `None`: there is no credential, and asking the keyring for one would + // fail on a machine with no secrets daemon — where a folder library is + // exactly the thing that should still work. + // + // A failure to persist is reported, not fatal: the library opens for this + // session and the user is asked again next launch, which is a great deal + // better than refusing to open a folder that is plainly there. + if let Err(e) = store.save(&account, None) { + log::warn!("persisting account: {e}"); + } + Ok(account) +} + /// Run Login Flow v2 without blocking the UI thread. /// /// Slint's event loop is single-threaded, so the network work happens on a @@ -901,3 +901,82 @@ fn android_open_url(url: &str) -> Result<(), String> { }) .map_err(|e: jni::errors::Error| e.to_string()) } + +#[cfg(test)] +mod tests { + use super::*; + use dr_plat::EphemeralSecretStore; + + fn store_in(dir: &std::path::Path) -> AccountStore { + AccountStore::open_at( + dir.join("sessions.json"), + Box::new(EphemeralSecretStore::new()), + ) + } + + fn tmpdir(name: &str) -> std::path::PathBuf { + let d = std::env::temp_dir().join(format!("dr-launch-folder-{name}")); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(&d).unwrap(); + d + } + + #[test] + fn opening_a_folder_stores_an_account_with_no_credential() { + // The whole sign-in, end to end through the registry: no browser, no + // keyring, no waiting state. + let dir = tmpdir("ok"); + let library = dir.join("Photos"); + std::fs::create_dir_all(&library).unwrap(); + let store = store_in(&dir); + + let account = open_folder_library(&store, &library.to_string_lossy()).unwrap(); + assert_eq!(account.backend, dr_sync_folder::BACKEND_ID); + assert!(account.login.is_empty(), "a folder has nobody to name"); + + // And it survives, so the next launch skips the screen. + let reloaded = store.current().expect("persisted"); + assert_eq!(reloaded.endpoint, account.endpoint); + + // With nothing in the keyring — the machine may have no secrets daemon + // at all, which is precisely when a folder library matters. + assert!(store.connection(&reloaded, false).unwrap().secret.is_none()); + } + + #[test] + fn a_mistyped_folder_is_refused_rather_than_stored() { + // The failure this guards: a stored account for a folder that is not + // there skips the launch screen next start and reads as a library + // that has lost its photographs. + let dir = tmpdir("typo"); + let store = store_in(&dir); + + let err = open_folder_library(&store, &dir.join("Pictrues").to_string_lossy()).unwrap_err(); + assert!(err.contains("No folder"), "{err}"); + assert!(store.current().is_none(), "nothing may be persisted"); + } + + #[test] + fn the_error_says_what_to_fix() { + // It goes straight to the screen's error line, so it has to read as + // instruction rather than as a type name. + let dir = tmpdir("messages"); + let store = store_in(&dir); + + for (input, want) in [("", "Choose"), ("Pictures", "full path")] { + let err = open_folder_library(&store, input).unwrap_err(); + assert!(err.contains(want), "{input:?} gave {err:?}"); + } + } + + #[test] + fn a_file_is_not_a_library() { + let dir = tmpdir("file"); + let f = dir.join("a.CR2"); + std::fs::write(&f, b"raw").unwrap(); + let store = store_in(&dir); + + let err = open_folder_library(&store, &f.to_string_lossy()).unwrap_err(); + assert!(err.contains("not a folder"), "{err}"); + } +}