Decouple download directory from mullvad-update

7 files changed, 170 insertions, 82 deletions

diff --git a/installer-downloader/src/controller.rs b/installer-downloader/src/controller.rs
index e7b8a6bb40..6dbf9255f3 100644
--- a/installer-downloader/src/controller.rs
+++ b/installer-downloader/src/controller.rs
@@ -10,6 +10,7 @@ use mullvad_update::{
 };
 
 use std::future::Future;
+use std::path::PathBuf;
 use tokio::sync::{mpsc, oneshot};
 
 /// Actions handled by an async worker task in [handle_action_messages].
@@ -19,6 +20,21 @@ enum TaskMessage {
     Cancel,
 }
 
+/// Provide a directory to use for [AppDownloader]
+pub trait DirectoryProvider: 'static {
+    /// Provide a directory to use for [AppDownloader]
+    fn create_download_dir() -> impl Future<Output = anyhow::Result<PathBuf>> + Send;
+}
+
+struct TempDirProvider;
+
+impl DirectoryProvider for TempDirProvider {
+    /// Create a locked-down directory to store downloads in
+    fn create_download_dir() -> impl Future<Output = anyhow::Result<PathBuf>> + Send {
+        mullvad_update::dir::admin_temp_dir()
+    }
+}
+
 /// See the [module-level docs](self).
 pub struct AppController {}
 
@@ -30,8 +46,10 @@ pub fn initialize_controller<T: AppDelegate + 'static>(delegate: &mut T) {
     type Downloader<T> = HttpAppDownloader<UiProgressUpdater<T>>;
     // Version info provider to use
     type VersionInfoProvider = ApiVersionInfoProvider;
+    // Directory provider to use
+    type DirProvider = TempDirProvider;
 
-    AppController::initialize::<_, Downloader<T>, VersionInfoProvider>(delegate)
+    AppController::initialize::<_, Downloader<T>, VersionInfoProvider, DirProvider>(delegate)
 }
 
 impl AppController {
@@ -39,11 +57,12 @@ impl AppController {
     ///
     /// Providing the downloader and version info fetcher as type arguments, they're decoupled from
     /// the logic of [AppController], allowing them to be mocked.
-    pub fn initialize<D, A, V>(delegate: &mut D)
+    pub fn initialize<D, A, V, DirProvider>(delegate: &mut D)
     where
         D: AppDelegate + 'static,
         V: VersionInfoProvider + 'static,
         A: From<UiAppDownloaderParameters<D>> + AppDownloader + 'static,
+        DirProvider: DirectoryProvider,
     {
         delegate.hide_download_progress();
         delegate.show_download_button();
@@ -52,7 +71,10 @@ impl AppController {
         delegate.hide_beta_text();
 
         let (task_tx, task_rx) = mpsc::channel(1);
-        tokio::spawn(handle_action_messages::<D, A>(delegate.queue(), task_rx));
+        tokio::spawn(handle_action_messages::<D, A, DirProvider>(
+            delegate.queue(),
+            task_rx,
+        ));
         delegate.set_status_text(resource::FETCH_VERSION_DESC);
         tokio::spawn(fetch_app_version_info::<D, V>(delegate, task_tx.clone()));
         Self::register_user_action_callbacks(delegate, task_tx);
@@ -105,10 +127,13 @@ where
 
 /// Async worker that handles actions such as initiating a download, cancelling it, and updating
 /// labels.
-async fn handle_action_messages<D, A>(queue: D::Queue, mut rx: mpsc::Receiver<TaskMessage>)
-where
+async fn handle_action_messages<D, A, DirProvider>(
+    queue: D::Queue,
+    mut rx: mpsc::Receiver<TaskMessage>,
+) where
     D: AppDelegate + 'static,
     A: From<UiAppDownloaderParameters<D>> + AppDownloader + 'static,
+    DirProvider: DirectoryProvider,
 {
     let mut version_info = None;
     let mut active_download = None;
@@ -135,6 +160,18 @@ where
                     continue;
                 };
 
+                // Create temporary dir
+                let download_dir = match DirProvider::create_download_dir().await {
+                    Ok(dir) => dir,
+                    Err(_err) => {
+                        queue.queue_main(move |self_| {
+                            self_.set_status_text("Failed to create download directory");
+                        });
+                        continue;
+                    }
+                };
+
+                // Begin download
                 let (tx, rx) = oneshot::channel();
                 queue.queue_main(move |self_| {
                     // TODO: Select appropriate URLs
@@ -158,6 +195,7 @@ where
                         app_size,
                         app_progress: UiProgressUpdater::new(self_.queue()),
                         app_sha256,
+                        cache_dir: download_dir,
                     });
 
                     let ui_downloader = UiAppDownloader::new(self_, downloader);
diff --git a/installer-downloader/src/ui_downloader.rs b/installer-downloader/src/ui_downloader.rs
index 7ae37df3e5..437320a537 100644
--- a/installer-downloader/src/ui_downloader.rs
+++ b/installer-downloader/src/ui_downloader.rs
@@ -37,19 +37,6 @@ impl<Delegate: AppDelegate, Downloader: AppDownloader + Send + 'static>
 impl<Delegate: AppDelegate, Downloader: AppDownloader + Send + 'static> AppDownloader
     for UiAppDownloader<Delegate, Downloader>
 {
-    async fn create_cache_dir(&mut self) -> Result<(), app::DownloadError> {
-        match self.downloader.create_cache_dir().await {
-            Ok(()) => Ok(()),
-            Err(err) => {
-                self.queue.queue_main(move |self_| {
-                    self_.set_download_text("ERROR: Failed to create cache directory.");
-                });
-
-                Err(err)
-            }
-        }
-    }
-
     async fn download_executable(&mut self) -> Result<(), app::DownloadError> {
         match self.downloader.download_executable().await {
             Ok(()) => {
diff --git a/installer-downloader/tests/controller.rs b/installer-downloader/tests/controller.rs
index 460e1c168a..2076b44a65 100644
--- a/installer-downloader/tests/controller.rs
+++ b/installer-downloader/tests/controller.rs
@@ -5,12 +5,13 @@
 //! changes to, and update, snapshots are by running `cargo insta review`.
 
 use insta::assert_yaml_snapshot;
-use installer_downloader::controller::AppController;
+use installer_downloader::controller::{AppController, DirectoryProvider};
 use installer_downloader::delegate::{AppDelegate, AppDelegateQueue};
 use installer_downloader::ui_downloader::UiAppDownloaderParameters;
 use mullvad_update::api::{Version, VersionInfo, VersionInfoProvider, VersionParameters};
 use mullvad_update::app::{AppDownloader, DownloadError};
 use mullvad_update::fetch::ProgressUpdater;
+use std::path::{Path, PathBuf};
 use std::sync::{Arc, LazyLock, Mutex};
 use std::time::Duration;
 use std::vec::Vec;
@@ -35,6 +36,18 @@ impl VersionInfoProvider for FakeVersionInfoProvider {
     }
 }
 
+pub struct FakeDirectoryProvider<const SUCCEED: bool> {}
+
+impl<const SUCCEEDED: bool> DirectoryProvider for FakeDirectoryProvider<SUCCEEDED> {
+    async fn create_download_dir() -> anyhow::Result<PathBuf> {
+        if SUCCEEDED {
+            Ok(Path::new("/tmp/fake").to_owned())
+        } else {
+            anyhow::bail!("Failed to create directory");
+        }
+    }
+}
+
 /// Downloader for which all steps immediately succeed
 pub type FakeAppDownloaderHappyPath = FakeAppDownloader<true, true, true>;
 
@@ -70,10 +83,6 @@ pub struct FakeAppDownloader<
 impl<const EXE_SUCCEED: bool, const VERIFY_SUCCEED: bool, const LAUNCH_SUCCEED: bool> AppDownloader
     for FakeAppDownloader<EXE_SUCCEED, VERIFY_SUCCEED, LAUNCH_SUCCEED>
 {
-    async fn create_cache_dir(&mut self) -> Result<(), DownloadError> {
-        Ok(())
-    }
-
     async fn download_executable(&mut self) -> Result<(), DownloadError> {
         self.params.app_progress.set_url(&self.params.app_url);
         self.params.app_progress.set_progress(0.);
@@ -274,9 +283,12 @@ impl AppDelegate for FakeAppDelegate {
 #[tokio::test(start_paused = true)]
 async fn test_fetch_version() {
     let mut delegate = FakeAppDelegate::default();
-    AppController::initialize::<_, FakeAppDownloaderHappyPath, FakeVersionInfoProvider>(
-        &mut delegate,
-    );
+    AppController::initialize::<
+        _,
+        FakeAppDownloaderHappyPath,
+        FakeVersionInfoProvider,
+        FakeDirectoryProvider<true>,
+    >(&mut delegate);
 
     // The app should start out by fetching the current app version
     assert_yaml_snapshot!(delegate.state);
@@ -296,9 +308,12 @@ async fn test_fetch_version() {
 #[tokio::test(start_paused = true)]
 async fn test_download() {
     let mut delegate = FakeAppDelegate::default();
-    AppController::initialize::<_, FakeAppDownloaderHappyPath, FakeVersionInfoProvider>(
-        &mut delegate,
-    );
+    AppController::initialize::<
+        _,
+        FakeAppDownloaderHappyPath,
+        FakeVersionInfoProvider,
+        FakeDirectoryProvider<true>,
+    >(&mut delegate);
 
     // Wait for the version info
     tokio::time::sleep(Duration::from_secs(1)).await;
@@ -340,9 +355,51 @@ async fn test_download() {
 #[tokio::test(start_paused = true)]
 async fn test_failed_verification() {
     let mut delegate = FakeAppDelegate::default();
-    AppController::initialize::<_, FakeAppDownloaderVerifyFail, FakeVersionInfoProvider>(
-        &mut delegate,
-    );
+    AppController::initialize::<
+        _,
+        FakeAppDownloaderVerifyFail,
+        FakeVersionInfoProvider,
+        FakeDirectoryProvider<true>,
+    >(&mut delegate);
+
+    // Wait for the version info
+    tokio::time::sleep(Duration::from_secs(1)).await;
+
+    let queue = delegate.queue.clone();
+    queue.run_callbacks(&mut delegate);
+
+    // Initiate download
+    let cb = delegate
+        .download_callback
+        .take()
+        .expect("no download callback registered");
+    cb();
+
+    tokio::time::sleep(Duration::from_secs(1)).await;
+
+    // Wait for queued actions to complete
+    let queue = delegate.queue.clone();
+    queue.run_callbacks(&mut delegate);
+
+    tokio::time::sleep(Duration::from_secs(1)).await;
+
+    let queue = delegate.queue.clone();
+    queue.run_callbacks(&mut delegate);
+
+    // Verification failed
+    assert_yaml_snapshot!(delegate.state);
+}
+
+/// Test failing to create the download directory
+#[tokio::test(start_paused = true)]
+async fn test_failed_directory_creation() {
+    let mut delegate = FakeAppDelegate::default();
+    AppController::initialize::<
+        _,
+        FakeAppDownloaderHappyPath,
+        FakeVersionInfoProvider,
+        FakeDirectoryProvider<false>,
+    >(&mut delegate);
 
     // Wait for the version info
     tokio::time::sleep(Duration::from_secs(1)).await;
diff --git a/installer-downloader/tests/snapshots/controller__failed_directory_creation.snap b/installer-downloader/tests/snapshots/controller__failed_directory_creation.snap
new file mode 100644
index 0000000000..9cb6d6fcb1
--- /dev/null
+++ b/installer-downloader/tests/snapshots/controller__failed_directory_creation.snap
@@ -0,0 +1,27 @@
+---
+source: installer-downloader/tests/controller.rs
+expression: delegate.state
+snapshot_kind: text
+---
+status_text: Failed to create download directory
+download_text: ""
+download_button_visible: true
+cancel_button_visible: false
+cancel_button_enabled: false
+download_button_enabled: true
+download_progress: 0
+download_progress_visible: false
+beta_text_visible: false
+quit: false
+call_log:
+  - hide_download_progress
+  - show_download_button
+  - disable_download_button
+  - hide_cancel_button
+  - hide_beta_text
+  - "set_status_text: Loading version details..."
+  - on_download
+  - on_cancel
+  - "set_status_text: Version: 2025.1"
+  - enable_download_button
+  - "set_status_text: Failed to create download directory"
diff --git a/mullvad-update/src/app.rs b/mullvad-update/src/app.rs
index 6d1f6b8198..f543c15955 100644
--- a/mullvad-update/src/app.rs
+++ b/mullvad-update/src/app.rs
@@ -9,8 +9,6 @@ use crate::{
     verify::{AppVerifier, Sha256Verifier},
 };
 
-const INSTALLER_STARTUP_TIMEOUT: Duration = Duration::from_millis(500);
-
 #[derive(Debug)]
 pub enum DownloadError {
     CreateDir(anyhow::Error),
@@ -29,14 +27,14 @@ pub struct AppDownloaderParameters<AppProgress> {
     pub app_size: usize,
     pub app_progress: AppProgress,
     pub app_sha256: [u8; 32],
+    /// Directory to store the installer in.
+    /// Ensure that this has proper permissions set.
+    pub cache_dir: PathBuf,
 }
 
 /// See the [module-level documentation](self).
 #[async_trait::async_trait]
 pub trait AppDownloader: Send {
-    /// Create download directory.
-    async fn create_cache_dir(&mut self) -> Result<(), DownloadError>;
-
     /// Download the app binary.
     async fn download_executable(&mut self) -> Result<(), DownloadError>;
 
@@ -47,9 +45,11 @@ pub trait AppDownloader: Send {
     async fn install(&mut self) -> Result<(), DownloadError>;
 }
 
+/// How long to wait for the installer to exit before returning
+const INSTALLER_STARTUP_TIMEOUT: Duration = Duration::from_millis(500);
+
 /// Download the app and signature, and verify the app's signature
 pub async fn install_and_upgrade(mut downloader: impl AppDownloader) -> Result<(), DownloadError> {
-    downloader.create_cache_dir().await?;
     downloader.download_executable().await?;
     downloader.verify().await?;
     downloader.install().await
@@ -58,12 +58,11 @@ pub async fn install_and_upgrade(mut downloader: impl AppDownloader) -> Result<(
 #[derive(Clone)]
 pub struct HttpAppDownloader<AppProgress> {
     params: AppDownloaderParameters<AppProgress>,
-    cache_dir: Option<PathBuf>,
 }
 
 impl<AppProgress> HttpAppDownloader<AppProgress> {
     pub fn new(params: AppDownloaderParameters<AppProgress>) -> Self {
-        Self { params, cache_dir: None }
+        Self { params }
     }
 }
 
@@ -77,14 +76,8 @@ impl<AppProgress: ProgressUpdater> From<AppDownloaderParameters<AppProgress>>
 
 #[async_trait::async_trait]
 impl<AppProgress: ProgressUpdater> AppDownloader for HttpAppDownloader<AppProgress> {
-    async fn create_cache_dir(&mut self) -> Result<(), DownloadError> {
-        let dir = crate::dir::update_directory().await.map_err(DownloadError::CreateDir)?;
-        self.cache_dir = Some(dir);
-        Ok(())
-    }
-
     async fn download_executable(&mut self) -> Result<(), DownloadError> {
-        let bin_path = self.bin_path().expect("Performed after 'create_cache_dir'");
+        let bin_path = self.bin_path();
         fetch::get_to_file(
             bin_path,
             &self.params.app_url,
@@ -96,7 +89,7 @@ impl<AppProgress: ProgressUpdater> AppDownloader for HttpAppDownloader<AppProgre
     }
 
     async fn verify(&mut self) -> Result<(), DownloadError> {
-        let bin_path = self.bin_path().expect("Performed after 'create_cache_dir'");
+        let bin_path = self.bin_path();
         let hash = self.hash_sha256();
 
         match Sha256Verifier::verify(&bin_path, *hash)
@@ -115,10 +108,9 @@ impl<AppProgress: ProgressUpdater> AppDownloader for HttpAppDownloader<AppProgre
     }
 
     async fn install(&mut self) -> Result<(), DownloadError> {
-        let bin_path = self.bin_path().expect("Performed after 'create_cache_dir'");
+        let bin_path = self.bin_path();
 
         // Launch process
-        // TODO: move to launch.rs?
         let mut cmd = Command::new(bin_path);
         let mut child = cmd.spawn().map_err(DownloadError::Launch)?;
 
@@ -129,22 +121,26 @@ impl<AppProgress: ProgressUpdater> AppDownloader for HttpAppDownloader<AppProgre
             // No timeout: Incredibly quick but successful (or wrong exit code, probably)
             Ok(Ok(status)) if status.success() => Ok(()),
             // Installer failed
-            Ok(Ok(status)) => Err(DownloadError::InstallFailed(anyhow::anyhow!("Install failed with status: {status}"))),
+            Ok(Ok(status)) => Err(DownloadError::InstallFailed(anyhow::anyhow!(
+                "Install failed with status: {status}"
+            ))),
             // Installer failed
-            Ok(Err(err)) => Err(DownloadError::InstallFailed(anyhow::anyhow!("Install failed : {err}"))),
+            Ok(Err(err)) => Err(DownloadError::InstallFailed(anyhow::anyhow!(
+                "Install failed: {err}"
+            ))),
         }
     }
 }
 
 impl<AppProgress> HttpAppDownloader<AppProgress> {
-    fn bin_path(&self) -> Option<PathBuf> {
+    fn bin_path(&self) -> PathBuf {
         #[cfg(windows)]
-        let bin_filename = format!("{}.exe", self.params.app_version);
+        let bin_filename = format!("mullvad-{}.exe", self.params.app_version);
 
         #[cfg(unix)]
         let bin_filename = self.params.app_version.to_string();
 
-        self.cache_dir.as_ref().map(|dir| dir.join(bin_filename))
+        self.params.cache_dir.join(bin_filename)
     }
 
     fn hash_sha256(&self) -> &[u8; 32] {
diff --git a/mullvad-update/src/dir.rs b/mullvad-update/src/dir.rs
index a7e7b81e11..cf3a85eb24 100644
--- a/mullvad-update/src/dir.rs
+++ b/mullvad-update/src/dir.rs
@@ -1,26 +1,22 @@
-//! This provides a secure directory suitable for storing updates, with admin-only write access.
+//! This provides a secure temp directory suitable for storing updates, with admin-only write access.
 
 use std::path::PathBuf;
-use tokio::fs;
 
 use anyhow::Context;
 
-/// Name of subdirectory in the cache directory
-const CACHE_DIRNAME: &str = "updates";
+/// Name of subdirectory in the temp directory
+const CACHE_DIRNAME: &str = "mullvad-updates";
 
-/// This returns a directory suitable for storing updates. Only admins have write access.
+/// This returns a directory suitable for storing updates, where only admins have write access.
 ///
 /// This function is a bit racey, as the directory is created before being restricted.
 /// This is acceptable as long as the checksum of each file is verified before being used.
-pub async fn update_directory() -> anyhow::Result<PathBuf> {
-    let dir = tokio::task::spawn_blocking(|| mullvad_paths::cache_dir())
-        .await
-        .unwrap()?
-        .join(CACHE_DIRNAME);
+pub async fn admin_temp_dir() -> anyhow::Result<PathBuf> {
+    let temp_dir = std::env::temp_dir().join(CACHE_DIRNAME);
 
     #[cfg(windows)]
     {
-        let dir_clone = dir.clone();
+        let dir_clone = temp_dir.clone();
         tokio::task::spawn_blocking(move || {
             mullvad_paths::windows::create_privileged_directory(&dir_clone)
         })
@@ -34,24 +30,13 @@ pub async fn update_directory() -> anyhow::Result<PathBuf> {
         use std::{fs::Permissions, os::unix::fs::PermissionsExt};
         use tokio::fs;
 
-        fs::create_dir_all(&dir)
+        fs::create_dir_all(&temp_dir)
             .await
             .context("Failed to create cache directory")?;
-        fs::set_permissions(&dir, Permissions::from_mode(0o700))
+        fs::set_permissions(&temp_dir, Permissions::from_mode(0o700))
             .await
             .context("Failed to set cache directory permissions")?;
     }
 
-    Ok(dir)
-}
-
-/// Remove all files from the update directory
-pub async fn cleanup_update_directory() -> anyhow::Result<()> {
-
-    let dir = update_directory().await?;
-
-    // It's fine to remove the directory in its entirety, since `update_directory` recreates it.
-    fs::remove_dir_all(dir).await?;
-
-    Ok(())
+    Ok(temp_dir)
 }
diff --git a/mullvad-update/src/fetch.rs b/mullvad-update/src/fetch.rs
index 9b13fd9ed1..8ba821a5f4 100644
--- a/mullvad-update/src/fetch.rs
+++ b/mullvad-update/src/fetch.rs
@@ -23,8 +23,6 @@ pub trait ProgressUpdater: Send + 'static {
     fn set_url(&mut self, url: &str);
 }
 
-// TODO: save file to protected dir so it cannot be tampered with after verification
-
 /// This describes how to handle files that do not match an expected size
 #[derive(Debug, Clone, Copy)]
 pub enum SizeHint {