From b2e993b0c1f5534660ba953052d208dd8e36b39a Mon Sep 17 00:00:00 2001 From: gnacho Date: Sun, 23 Aug 2026 22:09:23 +0200 Subject: [PATCH] feat(credentials): cache account passwords in memory (closes #178) One Secret Service session negotiation per account per process instead of one per credential lookup (the pattern desktop clients like Iotas use): every get_for_account used to open a fresh DH session, and a sync run did several. The cache is written through on set, evicted on delete, and invalidated when a run ends in AuthFailed so the next lookup re-reads the keyring after the user signs in again. --- src/nextcloud/credentials.rs | 135 ++++++++++++++++++++++++++++++++++- src/nextcloud/sync_engine.rs | 46 +++++++++++- 2 files changed, 177 insertions(+), 4 deletions(-) diff --git a/src/nextcloud/credentials.rs b/src/nextcloud/credentials.rs index 76e92ab..4deb371 100644 --- a/src/nextcloud/credentials.rs +++ b/src/nextcloud/credentials.rs @@ -11,8 +11,18 @@ //! //! Uses `secret_service::blocking` (feature `rt-tokio-crypto-rust`, DH //! encrypted session). Blocking calls must not run on the async UI loop. +//! +//! Issue #178: resolved passwords are cached in process memory, so each +//! account costs one Secret Service session negotiation per process instead +//! of one per sync run (the desktop reference clients, e.g. Iotas, do the +//! same). The cache is written through on [`CredentialsStore::set`], evicted +//! on [`CredentialsStore::delete`], and invalidated when a sync run ends in +//! authentication failure so the next lookup re-reads the keyring. Secrets +//! therefore live in process memory for the whole session; a revoked +//! password is noticed on the next 401, not before. use std::collections::HashMap; +use std::sync::{Mutex, MutexGuard, OnceLock}; use secret_service::blocking::SecretService; use secret_service::EncryptionType; @@ -61,6 +71,16 @@ const ATTR_USERNAME: &str = "username"; /// Secret content type used for stored passwords. const CONTENT_TYPE: &str = "text/plain"; +/// Process-local password cache, keyed by account id (issue #178). +static PASSWORD_CACHE: OnceLock>> = OnceLock::new(); + +fn cache_lock() -> MutexGuard<'static, HashMap> { + PASSWORD_CACHE + .get_or_init(|| Mutex::new(HashMap::new())) + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) +} + /// Prefer the `login` collection (unlocked automatically by the desktop /// session) and fall back to the default collection (issue #58). /// @@ -92,6 +112,7 @@ impl CredentialsStore { true, CONTENT_TYPE, )?; + cache_lock().insert(account_id.to_string(), password.to_string()); Ok(()) } @@ -124,18 +145,23 @@ impl CredentialsStore { /// Read the password for an account, falling back to the legacy entry. /// - /// Tries the `account_id` item first; when absent, searches the legacy + /// Serves the in-memory cache first (issue #178); on a miss, tries the + /// `account_id` item first; when absent, searches the legacy /// Python `nextsync` attributes (`{server, username}`). A legacy hit is /// adopted: the secret is re-stored under `account_id` so later lookups /// hit the fast path, while the legacy item itself is left in place for /// the Python app. Adoption failure is not fatal — the password is still - /// returned. + /// returned. Successful keyring resolutions populate the cache. pub fn get_for_account( account_id: &str, server: &str, login: &str, ) -> Result, CredentialError> { + if let Some(cached) = cache_lock().get(account_id) { + return Ok(Some(cached.clone())); + } if let Some(password) = Self::get(account_id)? { + cache_lock().insert(account_id.to_string(), password.clone()); return Ok(Some(password)); } let service = SecretService::connect(EncryptionType::Dh)?; @@ -154,11 +180,22 @@ impl CredentialsStore { let secret = item.get_secret()?; let password = String::from_utf8_lossy(&secret).into_owned(); let _ = Self::set(account_id, &password); + cache_lock().insert(account_id.to_string(), password.clone()); Ok(Some(password)) } + /// Drop the cached password for an account (issue #178). + /// + /// Called when a sync run proves the credential wrong (authentication + /// failure), so the next lookup re-reads the keyring instead of + /// replaying the stale secret. + pub fn invalidate(account_id: &str) { + cache_lock().remove(account_id); + } + /// Delete the stored password for an account, if any. pub fn delete(account_id: &str) -> Result<(), CredentialError> { + cache_lock().remove(account_id); let service = SecretService::connect(EncryptionType::Dh)?; let result = service.search_items(HashMap::from([(ATTR_ACCOUNT_ID, account_id)]))?; // Only the unlocked items are reachable; items in a locked collection @@ -170,6 +207,16 @@ impl CredentialsStore { } } +#[cfg(test)] +pub(crate) fn seed_cache_for_tests(account_id: &str, password: &str) { + cache_lock().insert(account_id.to_string(), password.to_string()); +} + +#[cfg(test)] +pub(crate) fn cached_for_tests(account_id: &str) -> Option { + cache_lock().get(account_id).cloned() +} + #[cfg(test)] mod tests { use super::*; @@ -308,4 +355,88 @@ mod tests { assert!(message.contains("UTF-8")); assert!(!message.contains("0xFF")); } + + const TEST_CACHE_ACCOUNT: &str = + "7e3a9c1f5b28d4e6a0f2c8b1d3e5a7c9b0d2e4f6a8c0b2d4e6f8a0c2e4b6d8a0c2"; + // Distinct id per cache test: the cache is process-global and tests run + // in parallel threads. + const TEST_CACHE_ACCOUNT_WT: &str = + "1a2b3c4d5e6f708192a3b4c5d6e7f80192a3b4c5d6e7f8091a2b3c4d5e6f7a8b9"; + + /// Issue #178: a seeded cache entry is served without touching the + /// Secret Service, and `invalidate` forces the next lookup back to the + /// keyring. + #[test] + fn cache_serves_until_invalidated() { + CredentialsStore::invalidate(TEST_CACHE_ACCOUNT); + seed_cache_for_tests(TEST_CACHE_ACCOUNT, TEST_PASSWORD); + + // Served from memory: works even though nothing is stored under + // this account in any keyring. + let resolved = CredentialsStore::get_for_account( + TEST_CACHE_ACCOUNT, + "https://cache-unit-test.example.net", + "cache-unit-test@example.net", + ) + .expect("cached lookup should succeed"); + assert_eq!(resolved.as_deref(), Some(TEST_PASSWORD)); + + CredentialsStore::invalidate(TEST_CACHE_ACCOUNT); + assert!(cached_for_tests(TEST_CACHE_ACCOUNT).is_none()); + // After invalidation the cache no longer answers: the result now + // depends on the real keyring (Ok(None) when available, an error + // when there is no session bus at all). + let after = CredentialsStore::get_for_account( + TEST_CACHE_ACCOUNT, + "https://cache-unit-test.example.net", + "cache-unit-test@example.net", + ); + assert!(after.map(|password| password.is_none()).unwrap_or(true)); + } + + /// Issue #178: `set` writes through to the cache and `delete` evicts it. + #[test] + fn set_writes_through_and_delete_evicts() { + match SecretService::connect(EncryptionType::Dh) { + Ok(_) => {} + Err(_) => { + eprintln!("no Secret Service session bus available; skipping"); + return; + } + } + struct CacheCleanup; + impl Drop for CacheCleanup { + fn drop(&mut self) { + let _ = CredentialsStore::delete(TEST_CACHE_ACCOUNT_WT); + } + } + let _guard = CacheCleanup; + + let _ = CredentialsStore::delete(TEST_CACHE_ACCOUNT_WT); + CredentialsStore::set(TEST_CACHE_ACCOUNT_WT, TEST_PASSWORD).expect("set should succeed"); + assert_eq!( + cached_for_tests(TEST_CACHE_ACCOUNT_WT).as_deref(), + Some(TEST_PASSWORD) + ); + + // Remove the keyring item directly, bypassing the store: the cache + // must keep serving the password regardless. + let service = SecretService::connect(EncryptionType::Dh).expect("connect"); + let result = service + .search_items(HashMap::from([(ATTR_ACCOUNT_ID, TEST_CACHE_ACCOUNT_WT)])) + .expect("search"); + for item in result.unlocked { + item.delete().expect("direct delete"); + } + let resolved = CredentialsStore::get_for_account( + TEST_CACHE_ACCOUNT_WT, + "https://cache-unit-test.example.net", + "cache-unit-test@example.net", + ) + .expect("cached lookup should succeed"); + assert_eq!(resolved.as_deref(), Some(TEST_PASSWORD)); + + CredentialsStore::delete(TEST_CACHE_ACCOUNT_WT).expect("delete should succeed"); + assert!(cached_for_tests(TEST_CACHE_ACCOUNT_WT).is_none()); + } } diff --git a/src/nextcloud/sync_engine.rs b/src/nextcloud/sync_engine.rs index dddf278..8352efc 100644 --- a/src/nextcloud/sync_engine.rs +++ b/src/nextcloud/sync_engine.rs @@ -248,6 +248,7 @@ impl SyncEngine { impl SyncRunner for SyncEngine { fn start(&mut self, _reasons: &[Trigger], on_finished: Box) { + let account_id = self.account.id.clone(); let account = self.account.clone(); let folder = self.folder.clone(); let network = self.network.clone(); @@ -274,8 +275,22 @@ impl SyncRunner for SyncEngine { Err(_) => EngineRun::Direct(SyncOutcome::Failed), }; match run { - EngineRun::Result(result) => on_finished(result.outcome()), - EngineRun::Direct(outcome) => on_finished(outcome), + EngineRun::Result(result) => { + let outcome = result.outcome(); + // Issue #178: a proven-dead password must leave the + // in-memory cache so the next lookup re-reads the + // keyring (e.g. after the user signs in again). + if matches!(outcome, SyncOutcome::AuthFailed) { + CredentialsStore::invalidate(&account_id); + } + on_finished(outcome); + } + EngineRun::Direct(outcome) => { + if matches!(outcome, SyncOutcome::AuthFailed) { + CredentialsStore::invalidate(&account_id); + } + on_finished(outcome); + } } }); } @@ -941,6 +956,33 @@ mod tests { assert_eq!(outcome, SyncOutcome::AuthFailed); } + /// Issue #178: when a run proves the password dead, the cached copy must + /// be dropped so the next lookup re-reads the keyring instead of + /// replaying the stale secret (e.g. after the user signs in again). + #[test] + fn auth_failed_outcome_invalidates_the_cached_password() { + let account = account(); + crate::nextcloud::credentials::seed_cache_for_tests(&account.id, "stale-secret"); + let (progress_tx, _progress_rx) = async_channel::unbounded(); + let engine = SyncEngine::new( + account, + folder(), + NetworkConfig::default(), + None, + Some(PathBuf::from("/bin/false")), + progress_tx, + ) + .with_credentials(Arc::new(FakeCredentials(CredentialLookup::Found( + "stale-secret".to_string(), + )))) + .with_remote_ensurer(Arc::new(|_account, _folder, _password| { + Err(ApiError::AuthRejected) + })); + let (outcome, _) = run_engine(engine, &async_channel::unbounded().1); + assert_eq!(outcome, SyncOutcome::AuthFailed); + assert!(crate::nextcloud::credentials::cached_for_tests("test-account").is_none()); + } + /// Issue #162: a transport failure (the server itself does not answer) /// must short-circuit to NetworkError without spawning nextcloudcmd. The /// fake binary is /bin/false, so if the engine spawned it the run would