diff --git a/library/src/c_api/mod.rs b/library/src/c_api/mod.rs index 57264c3..815e2a9 100644 --- a/library/src/c_api/mod.rs +++ b/library/src/c_api/mod.rs @@ -281,7 +281,10 @@ pub extern "C" fn shorebird_report_launch_success() { #[cfg(test)] mod test { use super::*; - use crate::network::{testing_set_network_hooks, PatchCheckResponse}; + use crate::{ + network::{testing_set_network_hooks, PatchCheckResponse}, + test_utils::write_fake_apk, + }; use anyhow::Ok; use serial_test::serial; use tempdir::TempDir; @@ -438,18 +441,6 @@ mod test { shorebird_report_launch_failure(); } - fn write_fake_zip(zip_path: &str, libapp_contents: &[u8]) { - use std::io::Write; - let mut zip = zip::ZipWriter::new(std::fs::File::create(zip_path).unwrap()); - let options = zip::write::FileOptions::default() - .compression_method(zip::CompressionMethod::Stored) - .unix_permissions(0o755); - let app_path = crate::android::get_relative_lib_path("libapp.so"); - zip.start_file(app_path.to_str().unwrap(), options).unwrap(); - zip.write_all(libapp_contents).unwrap(); - zip.finish().unwrap(); - } - #[serial] #[test] fn patch_success() { @@ -460,7 +451,7 @@ mod test { let base = "hello world"; let expected_new: &str = "hello tests"; let apk_path = tmp_dir.path().join("base.apk"); - write_fake_zip(apk_path.to_str().unwrap(), base.as_bytes()); + write_fake_apk(apk_path.to_str().unwrap(), base.as_bytes()); let fake_libapp_path = tmp_dir.path().join("lib/arch/ignored.so"); let c_params = parameters(&tmp_dir, fake_libapp_path.to_str().unwrap()); // app_id is required or shorebird_init will fail. @@ -482,6 +473,7 @@ mod test { download_url: "ignored".to_owned(), hash_signature: None, }), + rolled_back_patch_numbers: None, }) }, |_url| { @@ -584,6 +576,7 @@ mod test { download_url: "ignored".to_owned(), hash_signature: None, }), + rolled_back_patch_numbers: None, }) }, |_url| { diff --git a/library/src/cache/patch_manager.rs b/library/src/cache/patch_manager.rs index 510f27a..7cfa351 100644 --- a/library/src/cache/patch_manager.rs +++ b/library/src/cache/patch_manager.rs @@ -110,6 +110,10 @@ pub trait ManagePatches { /// Whether we have failed to boot from the patch with `patch_number`. fn is_known_bad_patch(&self, patch_number: usize) -> bool; + /// Deletes artifacts for the provided patch_number if they exist. + /// If the patch is the next_boot_patch, it is cleared. + fn remove_patch(&mut self, patch_number: usize) -> Result<()>; + /// Resets the patch manager to its initial state, removing all patches. This is /// intended to be used when a new release version is installed. fn reset(&mut self) -> Result<()>; @@ -243,9 +247,13 @@ impl PatchManager { } fn delete_patch_artifacts(&mut self, patch_number: usize) -> Result<()> { - info!("Deleting patch artifacts for patch {}", patch_number); - let patch_dir = self.patch_dir(patch_number); + if !patch_dir.exists() { + debug!("Patch {} not installed, nothing to delete", patch_number); + return Ok(()); + } + + info!("Deleting patch artifacts for patch {}", patch_number); std::fs::remove_dir_all(&patch_dir) .map_err(|e| { @@ -258,7 +266,7 @@ impl PatchManager { /// Deletes artifacts for the provided bad_patch_number and attempts to set the next_boot_patch to the last /// successfully booted patch. If the last successfully booted patch is not bootable or has the same number /// as the patch we're falling back from, we clear it as well. - fn try_fall_back_from_patch(&mut self, bad_patch_number: usize) { + fn try_fall_back_from_patch(&mut self, bad_patch_number: usize) -> Result<()> { // Continue even if we fail to delete the patch artifacts. It's more important to not try to // boot from a bad patch than to delete its artifacts. // No need to log failure – delete_patch_artifacts logs for us. @@ -284,6 +292,8 @@ impl PatchManager { let _ = self.delete_patch_artifacts(last_boot_patch.number); } } + + self.save_patches_state() } /// Deletes all patch artifacts with numbers less than patch_number. @@ -387,10 +397,11 @@ impl ManagePatches for PatchManager { if let Err(e) = self.validate_patch_is_bootable(&next_boot_patch) { error!("Patch {} is not bootable: {}", next_boot_patch.number, e); - self.try_fall_back_from_patch(next_boot_patch.number); - - if let Err(e) = self.save_patches_state() { - error!("Failed to save patches state: {}", e); + if let Err(e) = self.try_fall_back_from_patch(next_boot_patch.number) { + error!( + "Failed to fall back from next_boot_patch {}: {}", + next_boot_patch.number, e + ); } } @@ -440,14 +451,17 @@ impl ManagePatches for PatchManager { fn record_boot_failure_for_patch(&mut self, patch_number: usize) -> Result<()> { self.patches_state.currently_booting_patch = None; self.patches_state.known_bad_patches.insert(patch_number); - self.try_fall_back_from_patch(patch_number); - self.save_patches_state() + self.try_fall_back_from_patch(patch_number) } fn is_known_bad_patch(&self, patch_number: usize) -> bool { self.patches_state.known_bad_patches.contains(&patch_number) } + fn remove_patch(&mut self, patch_number: usize) -> Result<()> { + self.try_fall_back_from_patch(patch_number) + } + fn reset(&mut self) -> Result<()> { self.patches_state = PatchesState::default(); self.save_patches_state()?; @@ -871,7 +885,7 @@ mod fall_back_tests { assert!(manager.patches_state.last_booted_patch.is_none()); assert!(manager.patches_state.next_boot_patch.is_none()); - manager.try_fall_back_from_patch(1); + manager.try_fall_back_from_patch(1)?; assert!(manager.patches_state.last_booted_patch.is_none()); assert!(manager.patches_state.next_boot_patch.is_none()); @@ -892,7 +906,7 @@ mod fall_back_tests { hash: "hash".to_string(), signature: Some("signature".to_owned()), }); - manager.try_fall_back_from_patch(1); + manager.try_fall_back_from_patch(1)?; assert_eq!( manager.patches_state.next_boot_patch, @@ -915,7 +929,7 @@ mod fall_back_tests { // Download and fall back from patch 2 manager.add_patch_for_test(&temp_dir, 2)?; - manager.try_fall_back_from_patch(2); + manager.try_fall_back_from_patch(2)?; assert_eq!(manager.patches_state.last_booted_patch.unwrap().number, 1); assert_eq!(manager.patches_state.next_boot_patch.unwrap().number, 1); @@ -938,7 +952,7 @@ mod fall_back_tests { // Download and fall back from patch 2 manager.add_patch_for_test(&temp_dir, 2)?; - manager.try_fall_back_from_patch(2); + manager.try_fall_back_from_patch(2)?; // Neither patch should exist. assert!(manager.patches_state.last_booted_patch.is_none()); @@ -963,7 +977,7 @@ mod fall_back_tests { manager.record_boot_failure_for_patch(1)?; - manager.try_fall_back_from_patch(1); + manager.try_fall_back_from_patch(1)?; assert!(manager.patches_state.last_booted_patch.is_none()); assert_eq!( @@ -999,7 +1013,7 @@ mod fall_back_tests { let patch_dir = manager.patch_dir(2); std::fs::remove_dir_all(patch_dir)?; - manager.try_fall_back_from_patch(2); + manager.try_fall_back_from_patch(2)?; assert!(manager.patches_state.last_booted_patch.is_none()); assert!(manager.patches_state.next_boot_patch.is_none()); diff --git a/library/src/cache/updater_state.rs b/library/src/cache/updater_state.rs index 0955d71..88a2758 100644 --- a/library/src/cache/updater_state.rs +++ b/library/src/cache/updater_state.rs @@ -199,6 +199,12 @@ impl UpdaterState { .add_patch(patch.number, &patch.path, hash, signature) } + /// Removes the artifacts for patch `patch_number` from disk and updates state to ensure the + /// uninstalled patch is not booted in the future. + pub fn uninstall_patch(&mut self, patch_number: usize) -> Result<()> { + self.patch_manager.remove_patch(patch_number) + } + /// Returns true if we have previously failed to boot from patch `patch_number`. pub fn is_known_bad_patch(&self, patch_number: usize) -> bool { self.patch_manager.is_known_bad_patch(patch_number) diff --git a/library/src/lib.rs b/library/src/lib.rs index 30bb7d9..9b331d2 100644 --- a/library/src/lib.rs +++ b/library/src/lib.rs @@ -19,6 +19,9 @@ mod yaml; #[cfg(any(target_os = "android", test))] mod android; +#[cfg(test)] +mod test_utils; + // Take all public items from the updater namespace and make them public. pub use self::updater::*; diff --git a/library/src/network.rs b/library/src/network.rs index 242778f..0679693 100644 --- a/library/src/network.rs +++ b/library/src/network.rs @@ -190,13 +190,20 @@ pub struct CreatePatchEventRequest { event: PatchEvent, } +/// A response from the server telling us the latest state of patches for this release. #[derive(Debug, Deserialize, Serialize)] pub struct PatchCheckResponse { pub patch_available: bool, #[serde(default)] pub patch: Option, + + /// A list of patch numbers that have been rolled back by app developers. These should be + /// uninstalled from the device and not booted from. + #[serde(default)] + pub rolled_back_patch_numbers: Option>, } +/// Reports a patch event (e.g., install success/failure) to the server. pub fn send_patch_event(event: PatchEvent, config: &UpdateConfig) -> anyhow::Result<()> { let request = CreatePatchEventRequest { event }; @@ -205,6 +212,7 @@ pub fn send_patch_event(event: PatchEvent, config: &UpdateConfig) -> anyhow::Res report_event_fn(url, request) } +/// Downloads the file at `url` to `path`. pub fn download_to_path( network_hooks: &NetworkHooks, url: &str, diff --git a/library/src/test_utils.rs b/library/src/test_utils.rs new file mode 100644 index 0000000..9e79b48 --- /dev/null +++ b/library/src/test_utils.rs @@ -0,0 +1,44 @@ +/// Helper methods for tests. +use std::fs; + +use crate::{ + cache::{PatchInfo, UpdaterState}, + config::with_config, +}; + +/// Writes a fake patch to the patches directory and sets it as the next boot patch. +pub fn install_fake_patch(patch_number: usize) -> anyhow::Result<()> { + with_config(|config| { + let download_dir = std::path::PathBuf::from(&config.download_dir); + let artifact_path = download_dir.join(patch_number.to_string()); + fs::create_dir_all(&download_dir)?; + fs::write(&artifact_path, "hello")?; + + let mut state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); + state.install_patch( + &PatchInfo { + path: artifact_path, + number: patch_number, + }, + "hash", + None, + )?; + state.save() + }) +} + +/// Creates a fake APK at `apk_path` and writes `libapp_contents` to its relative `libapp.so` path. +pub fn write_fake_apk(apk_path: &str, libapp_contents: &[u8]) { + use std::io::Write; + let mut zip = zip::ZipWriter::new(std::fs::File::create(apk_path).unwrap()); + let options = + zip::write::FileOptions::default().compression_method(zip::CompressionMethod::Stored); + let app_path = crate::android::get_relative_lib_path("libapp.so"); + zip.start_file(app_path.to_str().unwrap(), options).unwrap(); + zip.write_all(libapp_contents).unwrap(); + zip.finish().unwrap(); +} diff --git a/library/src/updater.rs b/library/src/updater.rs index 18b1a2a..2f96b80 100644 --- a/library/src/updater.rs +++ b/library/src/updater.rs @@ -103,7 +103,7 @@ impl Display for UpdateError { } } -// `AppConfig` is the rust API. `ResolvedConfig` is the internal storage. +// `AppConfig` is the rust API. // However rusty api would probably used `&str` instead of `String`, // but making `&str` from `CStr*` is a bit of a pain. pub struct AppConfig { @@ -360,6 +360,21 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result { // Check for update. let patch_check_request_fn = &(config.network_hooks.patch_check_request_fn); let response = patch_check_request_fn(&patches_check_url(&config.base_url), request)?; + info!("Patch check response: {:?}", response); + + with_mut_state(|state| { + if let Some(rolled_back_patches) = response.rolled_back_patch_numbers { + if !rolled_back_patches.is_empty() { + for patch_number in rolled_back_patches { + info!("Attempting uninstall of patch {}...", patch_number); + state.uninstall_patch(patch_number)?; + } + } + } + + Ok(()) + })?; + if !response.patch_available { return Ok(UpdateStatus::NoUpdate); } @@ -610,22 +625,23 @@ mod tests { use tempdir::TempDir; use crate::{ - cache::{PatchInfo, UpdaterState}, + cache::UpdaterState, config::{testing_reset_config, with_config}, events::EventType, network::{testing_set_network_hooks, NetworkHooks, PatchCheckResponse}, + test_utils::install_fake_patch, time, with_state, ExternalFileProvider, Patch, }; #[derive(Debug, Clone)] - struct FakeExternalFileProvider {} + pub struct FakeExternalFileProvider {} impl ExternalFileProvider for FakeExternalFileProvider { fn open(&self) -> anyhow::Result> { Ok(Box::new(std::io::Cursor::new(vec![]))) } } - fn init_for_testing(tmp_dir: &TempDir, base_url: Option<&str>) { + pub fn init_for_testing(tmp_dir: &TempDir, base_url: Option<&str>) { testing_reset_config(); let cache_dir = tmp_dir.path().to_str().unwrap().to_string(); let mut yaml = "app_id: 1234".to_string(); @@ -633,12 +649,19 @@ mod tests { yaml += &format!("\nbase_url: {}", url); } + let libapp_path = tmp_dir + .path() + .join("lib/arch/libapp.so") + .to_str() + .unwrap() + .to_string(); + crate::init( crate::AppConfig { app_storage_dir: cache_dir.clone(), code_cache_dir: cache_dir.clone(), release_version: "1.0.0+1".to_string(), - original_libapp_paths: vec!["/dir/lib/arch/libapp.so".to_string()], + original_libapp_paths: vec![libapp_path], }, Box::new(FakeExternalFileProvider {}), &yaml, @@ -646,30 +669,6 @@ mod tests { .unwrap(); } - fn install_fake_patch(patch_number: usize) -> anyhow::Result<()> { - with_config(|config| { - let download_dir = std::path::PathBuf::from(&config.download_dir); - let artifact_path = download_dir.join(patch_number.to_string()); - fs::create_dir_all(&download_dir)?; - fs::write(&artifact_path, "hello")?; - - let mut state = UpdaterState::load_or_new_on_error( - &config.storage_dir, - &config.release_version, - config.patch_public_key.as_deref(), - ); - state.install_patch( - &PatchInfo { - path: artifact_path, - number: patch_number, - }, - "hash", - None, - )?; - state.save() - }) - } - #[serial] #[test] fn subsequent_init_calls_do_not_update_config() { @@ -947,6 +946,7 @@ mod tests { hash: "hash".to_string(), hash_signature: None, }), + rolled_back_patch_numbers: None, }; let check_response_body = serde_json::to_string(&check_response).unwrap(); let _ = server @@ -994,6 +994,7 @@ mod tests { download_url: "download_url".to_string(), hash_signature: None, }), + rolled_back_patch_numbers: None, }; let check_response_body = serde_json::to_string(&check_response).unwrap(); let _ = server @@ -1025,6 +1026,7 @@ mod tests { let check_response = PatchCheckResponse { patch_available: false, patch: None, + rolled_back_patch_numbers: None, }; let check_response_body = serde_json::to_string(&check_response).unwrap(); let _ = server @@ -1100,6 +1102,7 @@ mod tests { return Ok(PatchCheckResponse { patch_available: false, patch: None, + rolled_back_patch_numbers: None, }); } @@ -1132,3 +1135,159 @@ mod tests { // the patch check callback. } } + +#[cfg(test)] +mod rollback_tests { + use anyhow::Result; + use serial_test::serial; + use tempdir::TempDir; + + use crate::{ + network::PatchCheckResponse, + test_utils::{install_fake_patch, write_fake_apk}, + }; + + use super::{ + report_launch_start, report_launch_success, tests::init_for_testing, with_mut_state, Patch, + }; + + #[serial] + #[test] + fn does_not_roll_back_when_rolled_back_patches_is_empty() -> Result<()> { + let mut server = mockito::Server::new(); + let check_response = PatchCheckResponse { + patch_available: false, + patch: None, + rolled_back_patch_numbers: Some(vec![]), + }; + let check_response_body = serde_json::to_string(&check_response).unwrap(); + let _ = server + .mock("POST", "/api/v1/patches/check") + .with_status(200) + .with_body(check_response_body) + .create(); + let tmp_dir = TempDir::new("example").unwrap(); + init_for_testing(&tmp_dir, Some(&server.url())); + + install_fake_patch(1)?; + report_launch_start()?; + report_launch_success()?; + + with_mut_state(|state| { + assert_eq!(state.current_boot_patch().map(|p| p.number), Some(1)); + assert_eq!(state.next_boot_patch().map(|p| p.number), Some(1)); + Ok(()) + })?; + + let update_result = crate::update(); + assert_eq!(update_result.unwrap(), crate::UpdateStatus::NoUpdate); + + Ok(()) + } + + /// If the next_boot_patch is rolled back, the updater should roll back to the release version + /// if no other patches are available on disk. + #[serial] + #[test] + fn rolls_back_from_current_patch_to_release() -> Result<()> { + let mut server = mockito::Server::new(); + let check_response = PatchCheckResponse { + patch_available: false, + patch: None, + rolled_back_patch_numbers: Some(vec![1]), + }; + let check_response_body = serde_json::to_string(&check_response).unwrap(); + let _ = server + .mock("POST", "/api/v1/patches/check") + .with_status(200) + .with_body(check_response_body) + .create(); + let tmp_dir = TempDir::new("example").unwrap(); + init_for_testing(&tmp_dir, Some(&server.url())); + + install_fake_patch(1)?; + report_launch_start()?; + report_launch_success()?; + + with_mut_state(|state| { + assert_eq!(state.next_boot_patch().map(|p| p.number), Some(1)); + Ok(()) + })?; + + crate::update()?; + + with_mut_state(|state| { + assert!(state.next_boot_patch().is_none()); + Ok(()) + })?; + + Ok(()) + } + + /// If an older patch is provided by the patch check response, verify that we uninstall the + /// rolled back patch and install the older patch specified by the patch check response. + #[serial] + #[test] + fn rolls_back_to_previous_patch() -> Result<()> { + let mut server = mockito::Server::new(); + let download_url = format!("{}/patch/1", server.url()); + let check_response = PatchCheckResponse { + patch_available: true, + patch: Some(Patch { + number: 1, + download_url: download_url.to_string(), + // Generated by `string_patch "hello world" "hello tests"` + hash: "bb8f1d041a5cdc259055afe9617136799543e0a7a86f86db82f8c1fadbd8cc45" + .to_string(), + hash_signature: None, + }), + rolled_back_patch_numbers: Some(vec![2]), + }; + let check_response_body = serde_json::to_string(&check_response).unwrap(); + let _ = server + .mock("POST", "/api/v1/patches/check") + .with_status(200) + .with_body(check_response_body) + .create(); + let _ = server + .mock("GET", "/patch/1") + .with_status(200) + .with_body( + // Generated by `string_patch "hello world" "hello tests"` + [ + 40, 181, 47, 253, 0, 128, 177, 0, 0, 223, 177, 0, 0, 0, 16, 0, 0, 6, 0, 0, 0, + 0, 0, 0, 5, 116, 101, 115, 116, 115, 0, + ], + ) + .create(); + let tmp_dir = TempDir::new("example").unwrap(); + init_for_testing(&tmp_dir, Some(&server.url())); + + // Install the base apk to allow the "downloaded" patch 1 to successfully inflate and install. + let base = "hello world"; + let apk_path = tmp_dir.path().join("base.apk"); + write_fake_apk(apk_path.to_str().unwrap(), base.as_bytes()); + + // Install patch 2, pretend we're starting to boot from it, but don't report success or failure + // to ensure we still have patch 1 on disk. + install_fake_patch(2)?; + report_launch_start()?; + report_launch_success()?; + + with_mut_state(|state| { + assert_eq!(state.current_boot_patch().map(|p| p.number), Some(2)); + assert_eq!(state.next_boot_patch().map(|p| p.number), Some(2)); + Ok(()) + })?; + + let update_result = crate::update(); + assert_eq!(update_result.unwrap(), crate::UpdateStatus::UpdateInstalled); + + with_mut_state(|state| { + assert_eq!(state.next_boot_patch().map(|p| p.number), Some(1)); + Ok(()) + })?; + + Ok(()) + } +}