From 2e867841619efc67e8d221b7d15833520d56f3be Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Commaille?= <zecakeh@tedomum.fr>
Date: Mon, 27 Nov 2023 13:10:22 +0100
Subject: [PATCH] secret: Serialize secret as JSON

MessagePack creates issues with Secret Service providers that expect a
valid string.
We don't really care about saving a few bytes when storing secrets.
---
 src/secret.rs | 80 ++++++++++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 42 deletions(-)

diff --git a/src/secret.rs b/src/secret.rs
index 1c7a6759f..88f757f5b 100644
--- a/src/secret.rs
+++ b/src/secret.rs
@@ -13,14 +13,14 @@ use ruma::{DeviceId, OwnedDeviceId, OwnedUserId, UserId};
 use serde::{Deserialize, Serialize};
 use serde_json::error::Error as JsonError;
 use thiserror::Error;
-use tracing::{debug, error, warn};
+use tracing::{debug, error, info};
 use url::Url;
 
 use crate::{
     application::AppProfile, gettext_f, prelude::*, spawn_tokio, utils::matrix, APP_ID, PROFILE,
 };
 
-pub const CURRENT_VERSION: u8 = 4;
+pub const CURRENT_VERSION: u8 = 5;
 const SCHEMA_ATTRIBUTE: &str = "xdg:schema";
 
 static DATA_PATH: Lazy<PathBuf> = Lazy::new(|| {
@@ -275,21 +275,21 @@ impl StoredSession {
         };
         let secret = match item.secret().await {
             Ok(secret) => {
-                if version == 0 {
-                    match Secret::from_utf8(&secret) {
+                if version <= 4 {
+                    match rmp_serde::from_slice::<Secret>(&secret) {
                         Ok(secret) => secret,
                         Err(error) => {
-                            error!("Could not parse secret in stored session: {error:?}");
+                            error!("Could not parse secret in stored session: {error}");
                             return Err(SecretError::Invalid(gettext(
                                 "Malformed secret in stored session",
                             )));
                         }
                     }
                 } else {
-                    match rmp_serde::from_slice::<Secret>(&secret) {
+                    match serde_json::from_slice(&secret) {
                         Ok(secret) => secret,
                         Err(error) => {
-                            error!("Could not parse secret in stored session: {error}");
+                            error!("Could not parse secret in stored session: {error:?}");
                             return Err(SecretError::Invalid(gettext(
                                 "Malformed secret in stored session",
                             )));
@@ -407,7 +407,7 @@ impl StoredSession {
 
         let attrs = self.attributes();
         let attributes = attrs.iter().map(|(k, v)| (*k, v.as_ref())).collect();
-        let secret = rmp_serde::to_vec_named(&self.secret).unwrap();
+        let secret = serde_json::to_string(&self.secret).unwrap();
 
         keyring
             .create_item(
@@ -478,34 +478,30 @@ impl StoredSession {
         Ok(())
     }
 
-    /// Migrate this session to version 4.
-    ///
-    /// This implies moving the database under Fractal's directory.
-    pub async fn migrate_to_v4(&mut self, item: Item) {
-        warn!(
-            "Session {} with version {} found for user {}, migrating to version 4…",
-            self.id(),
-            self.version,
-            self.user_id,
-        );
+    /// Migrate this session to the current version.
+    pub async fn apply_migrations(&mut self, item: Item) {
+        if self.version < 4 {
+            info!("Migrating to version 4…");
 
-        let target_path = DATA_PATH.join(self.id());
+            let target_path = DATA_PATH.join(self.id());
 
-        if self.path != target_path {
-            debug!("Moving database to: {}", target_path.to_string_lossy());
+            if self.path != target_path {
+                debug!("Moving database to: {}", target_path.to_string_lossy());
 
-            if let Err(error) = fs::create_dir_all(&target_path) {
-                error!("Failed to create new directory: {error}");
-            }
+                if let Err(error) = fs::create_dir_all(&target_path) {
+                    error!("Failed to create new directory: {error}");
+                }
 
-            if let Err(error) = fs::rename(&self.path, &target_path) {
-                error!("Failed to move database: {error}");
-            }
+                if let Err(error) = fs::rename(&self.path, &target_path) {
+                    error!("Failed to move database: {error}");
+                }
 
-            self.path = target_path;
+                self.path = target_path;
+            }
         }
 
-        self.version = 4;
+        info!("Migrating to version 5…");
+        self.version = 5;
 
         let clone = self.clone();
         spawn_tokio!(async move {
@@ -548,14 +544,6 @@ pub struct Secret {
     pub passphrase: String,
 }
 
-impl Secret {
-    /// Converts a vector of bytes to a `Secret`.
-    pub fn from_utf8(slice: &[u8]) -> Result<Self, FromUtf8SecretError> {
-        let s = String::from_utf8(slice.to_owned())?;
-        Ok(serde_json::from_str(&s)?)
-    }
-}
-
 /// Retrieves all sessions stored to the `SecretService`
 pub async fn restore_sessions() -> Result<Vec<StoredSession>, SecretError> {
     let keyring = Keyring::new().await?;
@@ -575,15 +563,23 @@ pub async fn restore_sessions() -> Result<Vec<StoredSession>, SecretError> {
             Ok(session) => sessions.push(session),
             Err(SecretError::OldVersion { item, mut session }) => {
                 if session.version == 0 {
-                    warn!(
-                        "Found old session for {} with sled store, removing…",
+                    info!(
+                        "Found old session for user {} with sled store, removing…",
                         session.user_id
                     );
                     session.delete(Some(item), true).await;
-                } else if session.version < 4 {
-                    session.migrate_to_v4(item).await;
-                    sessions.push(session);
+                    continue;
                 }
+
+                info!(
+                    "Found session {} for user {} with old version {}, applying migrations…",
+                    session.id(),
+                    session.user_id,
+                    session.version,
+                );
+                session.apply_migrations(item).await;
+
+                sessions.push(session);
             }
             Err(SecretError::WrongProfile) => {}
             Err(error) => {
-- 
GitLab