From 9f9c323fd57d5e612aeec9d669a23ae6787db8fa Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Wed, 29 Nov 2023 09:01:55 +0100 Subject: [PATCH] Add unit tests for `PersistentTargetState` This includes refactoring reading of the state cache into a higher-order function. --- mullvad-daemon/src/target_state.rs | 149 ++++++++++++++++++++++++----- 1 file changed, 123 insertions(+), 26 deletions(-) diff --git a/mullvad-daemon/src/target_state.rs b/mullvad-daemon/src/target_state.rs index cc3094b9863c..cdac30c2b520 100644 --- a/mullvad-daemon/src/target_state.rs +++ b/mullvad-daemon/src/target_state.rs @@ -1,5 +1,6 @@ use mullvad_types::states::TargetState; use std::{ + future::Future, ops::Deref, path::{Path, PathBuf}, }; @@ -21,48 +22,76 @@ impl PersistentTargetState { /// Initialize using the current target state (if there is one) pub async fn new(cache_dir: &Path) -> Self { let cache_path = cache_dir.join(TARGET_START_STATE_FILE); - let mut update_cache = false; - let state = match fs::read_to_string(&cache_path).await { + let TargetStateInner { + state, + update_cache, + } = Self::read_target_state(&cache_path, fs::read_to_string).await; + let state = PersistentTargetState { + state, + cache_path, + locked: false, + }; + if update_cache { + state.save().await; + } + state + } + + /// Construct a [`TargetState`] from cache. + /// + /// `read_cache` allows the caller to decide how to read from a cache of + /// [`TargetState`]. + /// + /// This function will always succeed, even in the presence of IO + /// operations. Errors are handled gracefully by defaulting to safe target + /// states if necessary. + async fn read_target_state(cache: &Path, read_cache: F) -> TargetStateInner + where + F: FnOnce(PathBuf) -> R, + R: Future>, + { + match read_cache(cache.to_path_buf()).await { Ok(content) => serde_json::from_str(&content) .map(|state| { log::info!( - "Loaded cached target state \"{}\" from {}", + "Loaded cached target state {} from {}", state, - cache_path.display() + cache.display() ); - state + TargetStateInner { + state, + update_cache: false, + } }) .unwrap_or_else(|error| { log::error!( "{}", error.display_chain_with_msg("Failed to parse cached target tunnel state") ); - update_cache = true; - TargetState::Secured + TargetStateInner { + state: TargetState::Secured, + update_cache: true, + } }), + + Err(error) if error.kind() == io::ErrorKind::NotFound => { + log::debug!("No cached target state to load"); + TargetStateInner { + state: DEFAULT_TARGET_STATE, + update_cache: false, + } + } Err(error) => { - if error.kind() == io::ErrorKind::NotFound { - log::debug!("No cached target state to load"); - DEFAULT_TARGET_STATE - } else { - log::error!( - "{}", - error.display_chain_with_msg("Failed to read cached target tunnel state") - ); - update_cache = true; - TargetState::Secured + log::error!( + "{}", + error.display_chain_with_msg("Failed to read cached target tunnel state") + ); + TargetStateInner { + state: TargetState::Secured, + update_cache: true, } } - }; - let state = PersistentTargetState { - state, - cache_path, - locked: false, - }; - if update_cache { - state.save().await; } - state } /// Override the current target state, if there is one @@ -153,3 +182,71 @@ impl Deref for PersistentTargetState { &self.state } } + +/// The result of calling `read_target_state`. +struct TargetStateInner { + state: TargetState, + /// In some circumstances, the target state cache should be updated on disk + /// upon initialization a [`PersistentTargetState`]. This is signaled to the + /// constructor of [`PersistentTargetState`] by setting this value to + /// `true`. + update_cache: bool, +} + +impl Deref for TargetStateInner { + type Target = TargetState; + + fn deref(&self) -> &Self::Target { + &self.state + } +} + +#[cfg(test)] +mod test { + use super::*; + + static DUMMY_CACHE_DIR: &str = "target-state-test"; + + /// If no target state cache exist, the default target state is used. This + /// is the most basic check. + #[tokio::test] + async fn test_target_state_initialization_empty() { + // A completely blank slate. No target state cache file has been created yet. + let target_state = + PersistentTargetState::read_target_state(Path::new(DUMMY_CACHE_DIR), |_| async { + Err(io::ErrorKind::NotFound.into()) + }) + .await; + assert_eq!(*target_state, DEFAULT_TARGET_STATE); + } + + /// If a target state cache exist with some target state, the state can be + /// read-back successfully. + #[tokio::test] + async fn test_target_state_initialization_existing() { + for cached_state in [TargetState::Secured, TargetState::Unsecured] { + // Create a target state cache file before initializing `PersistentTargetState`. + let target_state = + PersistentTargetState::read_target_state(Path::new(DUMMY_CACHE_DIR), |_| async { + Ok(serde_json::to_string(&cached_state).unwrap()) + }) + .await; + assert_eq!(*target_state, cached_state); + } + } + + /// The state can not be read-back successfully if the state file has become + /// corrupt. In such cases, initializing a [`PersistentTargetState`] should + /// yield a "better safe than sorry"-target state of `Secured`. + #[tokio::test] + async fn test_target_corrupt_state_cache() { + // Intentionally corrupt the target state cache. + let target_state = + PersistentTargetState::read_target_state(Path::new(DUMMY_CACHE_DIR), |_| async { + Ok("Not a valid target state".to_string()) + }) + .await; + // Reading back a corrupt target state cache should yield `TargetState::Secured`. + assert_eq!(*target_state, TargetState::Secured); + } +}