Merge branch 'fix-parse-empty-acc-history'

2 files changed, 56 insertions, 35 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ee11ea0dce..ba30e96c3c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -54,6 +54,7 @@ Line wrap the file at 100 chars.                                              Th
 - Fix resource leak caused by location check.
 - Fix issue where sockets didn't close after disconnecting from WireGuard servers over TCP
   by updating `udp-over-tcp` to 0.2.
+- Parse old account history formats correctly when they are empty.
 
 #### Windows
 - Fix "Open Mullvad VPN" tray context menu item not working after toggling unpinned window setting.
diff --git a/mullvad-daemon/src/migrations/account_history.rs b/mullvad-daemon/src/migrations/account_history.rs
index 5400f04e33..06a8326f04 100644
--- a/mullvad-daemon/src/migrations/account_history.rs
+++ b/mullvad-daemon/src/migrations/account_history.rs
@@ -5,7 +5,7 @@ use serde::Deserialize;
 use std::path::Path;
 use talpid_types::ErrorExt;
 use tokio::{
-    fs,
+    fs::{self, File},
     io::{self, AsyncReadExt, AsyncSeekExt, AsyncWriteExt},
 };
 
@@ -63,29 +63,21 @@ pub async fn migrate_formats(settings_dir: &Path, settings: &mut serde_json::Val
     if is_format_v3(&bytes) {
         return Ok(());
     }
-
-    let token = migrate_formats_inner(&bytes, settings)?;
-
-    file.set_len(0).await.map_err(Error::WriteHistoryError)?;
-    file.seek(io::SeekFrom::Start(0))
-        .await
-        .map_err(Error::WriteHistoryError)?;
-    file.write_all(token.as_bytes())
-        .await
-        .map_err(Error::WriteHistoryError)?;
-    file.sync_all().await.map_err(Error::WriteHistoryError)?;
-
-    Ok(())
+    write_format_v3(file, migrate_formats_inner(&bytes, settings)?).await
 }
 
 fn migrate_formats_inner(
     account_bytes: &[u8],
     settings: &mut serde_json::Value,
-) -> Result<AccountToken> {
-    if let Some((token, wg_data)) = try_format_v2(account_bytes) {
-        settings["wireguard"] = wg_data;
-        Ok(token)
-    } else if let Some(token) = try_format_v1(account_bytes) {
+) -> Result<Option<AccountToken>> {
+    if let Ok(result) = try_format_v2(account_bytes) {
+        if let Some((token, wg_data)) = result {
+            settings["wireguard"] = wg_data;
+            Ok(Some(token))
+        } else {
+            Ok(None)
+        }
+    } else if let Ok(token) = try_format_v1(account_bytes) {
         Ok(token)
     } else {
         Err(Error::ParseHistoryError)
@@ -99,30 +91,42 @@ fn is_format_v3(bytes: &[u8]) -> bool {
     }
 }
 
-fn try_format_v2(bytes: &[u8]) -> Option<(AccountToken, serde_json::Value)> {
+async fn write_format_v3(mut file: File, token: Option<AccountToken>) -> Result<()> {
+    file.set_len(0).await.map_err(Error::WriteHistoryError)?;
+    file.seek(io::SeekFrom::Start(0))
+        .await
+        .map_err(Error::WriteHistoryError)?;
+    if let Some(token) = token {
+        file.write_all(token.as_bytes())
+            .await
+            .map_err(Error::WriteHistoryError)?;
+    }
+    file.sync_all().await.map_err(Error::WriteHistoryError)
+}
+
+fn try_format_v2(bytes: &[u8]) -> Result<Option<(AccountToken, serde_json::Value)>> {
     #[derive(Deserialize, Clone)]
     pub struct AccountEntry {
         pub account: AccountToken,
         pub wireguard: serde_json::Value,
     }
-    serde_json::from_slice(bytes)
-        .ok()
-        .and_then(|entries: Vec<AccountEntry>| {
-            entries
-                .into_iter()
-                .next()
-                .map(|entry| (entry.account, entry.wireguard))
-        })
+    Ok(serde_json::from_slice::<'_, Vec<AccountEntry>>(bytes)
+        .map_err(|_error| Error::ParseHistoryError)?
+        .into_iter()
+        .next()
+        .map(|entry| (entry.account, entry.wireguard)))
 }
 
-fn try_format_v1(bytes: &[u8]) -> Option<AccountToken> {
+fn try_format_v1(bytes: &[u8]) -> Result<Option<AccountToken>> {
     #[derive(Deserialize)]
     struct OldFormat {
         accounts: Vec<AccountToken>,
     }
-    serde_json::from_slice(bytes)
-        .ok()
-        .and_then(|old_format: OldFormat| old_format.accounts.into_iter().next())
+    Ok(serde_json::from_slice::<'_, OldFormat>(bytes)
+        .map_err(|_error| Error::ParseHistoryError)?
+        .accounts
+        .into_iter()
+        .next())
 }
 
 #[cfg(test)]
@@ -134,6 +138,11 @@ mod test {
   "accounts": ["1234", "4567"]
 }
 "#;
+    pub const ACCOUNT_HISTORY_V1_EMPTY: &str = r#"
+{
+  "accounts": []
+}
+"#;
     pub const ACCOUNT_HISTORY_V2: &str = r#"
 [
   {
@@ -159,6 +168,7 @@ mod test {
     }
   }
 ]"#;
+    pub const ACCOUNT_HISTORY_V2_EMPTY: &str = r#"[]"#;
     pub const ACCOUNT_HISTORY_V3: &str = r#"123456"#;
 
     pub const OLD_SETTINGS: &str = r#"
@@ -327,7 +337,7 @@ mod test {
 
     #[test]
     fn test_v2() {
-        assert!(super::try_format_v2(ACCOUNT_HISTORY_V1.as_bytes()).is_none());
+        assert!(super::try_format_v2(ACCOUNT_HISTORY_V1.as_bytes()).is_err());
 
         let mut old_settings = serde_json::from_str(OLD_SETTINGS).unwrap();
         let new_settings: serde_json::Value = serde_json::from_str(NEW_SETTINGS).unwrap();
@@ -337,12 +347,22 @@ mod test {
             super::migrate_formats_inner(ACCOUNT_HISTORY_V2.as_bytes(), &mut old_settings).unwrap();
 
         assert_eq!(&old_settings, &new_settings);
-        assert_eq!(token, "1234");
+        assert_eq!(token, Some("1234".to_string()));
+
+        // Test whether empty histories are handled correctly
+        let mut old_settings = serde_json::from_str(OLD_SETTINGS).unwrap();
+        let token =
+            super::migrate_formats_inner(ACCOUNT_HISTORY_V2_EMPTY.as_bytes(), &mut old_settings)
+                .unwrap();
+        assert_eq!(&old_settings, &old_settings);
+        assert_eq!(token, None);
     }
 
     #[test]
     fn test_v1() {
-        let token = super::try_format_v1(ACCOUNT_HISTORY_V1.as_bytes());
+        let token = super::try_format_v1(ACCOUNT_HISTORY_V1.as_bytes()).unwrap();
         assert_eq!(token, Some("1234".to_string()));
+        let token = super::try_format_v1(ACCOUNT_HISTORY_V1_EMPTY.as_bytes()).unwrap();
+        assert_eq!(token, None);
     }
 }