From 5e03a39f419b4df126ad71bdbb9eda89d7a4620e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Commaille?= Date: Sat, 18 Nov 2023 13:36:52 +0100 Subject: [PATCH] secret: Handle per-session errors and migrations Per-session errors should never happen in practice, it should be fine to just log them. Migrations already swallow any errors. --- po/POTFILES.in | 1 - src/error_page.rs | 81 +++++----------------------- src/error_page.ui | 94 +++++---------------------------- src/login/mod.rs | 10 +--- src/secret.rs | 131 ++++++++++++++++++++++++---------------------- src/window.rs | 41 ++++----------- 6 files changed, 105 insertions(+), 253 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 62c119d5..199889f9 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -14,7 +14,6 @@ src/components/loading_row.ui src/components/location_viewer.rs src/components/media_content_viewer.rs src/contrib/qr_code_scanner/mod.ui -src/error_page.rs src/error_page.ui src/greeter.rs src/greeter.ui diff --git a/src/error_page.rs b/src/error_page.rs index 575ab855..096b83a7 100644 --- a/src/error_page.rs +++ b/src/error_page.rs @@ -1,27 +1,19 @@ -use adw::subclass::prelude::BinImpl; -use gettextrs::gettext; -use gtk::{self, glib, glib::clone, prelude::*, subclass::prelude::*, CompositeTemplate}; -use tracing::error; - -use crate::{spawn, toast, Window}; +use adw::subclass::prelude::*; +use gtk::{self, glib, CompositeTemplate}; pub enum ErrorSubpage { - SecretErrorSession, - SecretErrorOther, + SecretError, } -impl AsRef for ErrorSubpage { - fn as_ref(&self) -> &str { +impl ErrorSubpage { + fn as_str(&self) -> &str { match self { - Self::SecretErrorSession => "secret-error-session", - Self::SecretErrorOther => "secret-error-other", + Self::SecretError => "secret-error", } } } mod imp { - use std::cell::RefCell; - use glib::subclass::InitializingObject; use super::*; @@ -30,10 +22,9 @@ mod imp { #[template(resource = "/org/gnome/Fractal/ui/error_page.ui")] pub struct ErrorPage { #[template_child] - pub page: TemplateChild, + pub secret_error_page: TemplateChild, #[template_child] pub stack: TemplateChild, - pub secret_item: RefCell>, } #[glib::object_subclass] @@ -45,15 +36,6 @@ mod imp { fn class_init(klass: &mut Self::Class) { Self::bind_template(klass); klass.set_accessible_role(gtk::AccessibleRole::Group); - klass.install_action( - "error-page.remove-secret-error-session", - None, - |obj, _, _| { - spawn!(clone!(@weak obj => async move { - obj.remove_secret_error_session().await; - })); - }, - ); } fn instance_init(obj: &InitializingObject) { @@ -61,19 +43,13 @@ mod imp { } } - impl ObjectImpl for ErrorPage { - fn constructed(&self) { - self.obj() - .action_set_enabled("error-page.remove-secret-error-session", false); - } - } - + impl ObjectImpl for ErrorPage {} impl WidgetImpl for ErrorPage {} - impl BinImpl for ErrorPage {} } glib::wrapper! { + /// A view displaying an error. pub struct ErrorPage(ObjectSubclass) @extends gtk::Widget, adw::Bin, @implements gtk::Accessible; } @@ -83,41 +59,10 @@ impl ErrorPage { glib::Object::new() } - pub fn display_secret_error(&self, message: &str, item: Option) { + pub fn display_secret_error(&self, message: &str) { let imp = self.imp(); - self.action_set_enabled("error-page.remove-secret-error-session", item.is_some()); - imp.page.set_description(Some(message)); - - let error_subpage = if item.is_some() { - ErrorSubpage::SecretErrorSession - } else { - ErrorSubpage::SecretErrorOther - }; - - imp.stack.set_visible_child_name(error_subpage.as_ref()); - imp.secret_item.replace(item); - } - - async fn remove_secret_error_session(&self) { - let Some(item) = self.imp().secret_item.take() else { - return; - }; - - match item.delete().await { - Ok(_) => { - self.action_set_enabled("error-page.remove-secret-error-session", false); - if let Some(window) = self.root().and_downcast_ref::() { - toast!(self, gettext("Session removed successfully.")); - window.restore_sessions().await; - } - } - Err(error) => { - error!("Could not remove session from secret storage: {error}"); - toast!( - self, - gettext("Could not remove session from secret storage") - ); - } - } + imp.secret_error_page.set_description(Some(message)); + imp.stack + .set_visible_child_name(ErrorSubpage::SecretError.as_str()); } } diff --git a/src/error_page.ui b/src/error_page.ui index 77fcca88..57f85774 100644 --- a/src/error_page.ui +++ b/src/error_page.ui @@ -13,88 +13,18 @@ - - Secret Service Error - key-symbolic - true + - - - + + secret-error + + + Secret Service Error + key-symbolic + true - - secret-error-session - - - vertical - 24 - - - - true - word-char - 0.0 - It seems like one of the Fractal sessions stored in the Secret Service is corrupted. If you know how to fix it you should do so. - - - - - - true - word-char - 0.0 - Alternatively, we could disconnect this session for you. This means you will have to login again and you will lose access to your encrypted messages, unless you have a session open in another client or you have already backed up your encryption keys. - - - - - - center - 6 - - - warning-symbolic - - - - - - true - word-char - 0.0 - Clicking this button might disconnect more than one session! - - - - - - - - center - true - Disconnect Session - error-page.remove-secret-error-session - - - - - - - - - secret-error-other - + + vertical 24 @@ -188,11 +118,11 @@ - + - + diff --git a/src/login/mod.rs b/src/login/mod.rs index 7441cfe3..a0991870 100644 --- a/src/login/mod.rs +++ b/src/login/mod.rs @@ -507,14 +507,8 @@ impl Login { let handle = spawn_tokio!(async move { session_info.store().await }); if let Err(error) = handle.await.unwrap() { - error!("Couldn't store session: {error}"); - - let (message, item) = error.into_parts(); - self.parent_window().switch_to_error_page( - &format!("{}\n\n{}", gettext("Unable to store session"), message), - item, - ); - return; + error!("Could not store session: {error}"); + toast!(self, gettext("Unable to store session")); } session.connect_ready(clone!(@weak self as obj => move |_| { diff --git a/src/secret.rs b/src/secret.rs index ef7b88a2..ec4aa8f0 100644 --- a/src/secret.rs +++ b/src/secret.rs @@ -47,9 +47,12 @@ pub enum SecretError { #[error("Session found with old version")] OldVersion { item: Item, session: StoredSession }, - /// A corrupted session was found. - #[error("{error}")] - CorruptSession { error: String, item: Item }, + /// An invalid session was found. + /// + /// This should only happen if for some reason we get an item from a + /// different application. + #[error("Invalid session: {0}")] + Invalid(String), /// An error occurred interacting with the secret service. #[error(transparent)] @@ -60,22 +63,18 @@ pub enum SecretError { WrongProfile, } -impl SecretError { - /// Split `self` between its message and its optional `Item`. - pub fn into_parts(self) -> (String, Option) { +impl UserFacingError for SecretError { + fn to_user_facing(self) -> String { match self { - SecretError::UnsupportedVersion { version, item, .. } => ( - gettext_f( - // Translators: Do NOT translate the content between '{' and '}', this is a - // variable name. - "Found stored session with unsupported version {version_nb}", - &[("version_nb", &version.to_string())], - ), - Some(item), + SecretError::UnsupportedVersion { version, .. } => gettext_f( + // Translators: Do NOT translate the content between '{' and '}', this is a + // variable name. + "Found stored session with unsupported version {version_nb}", + &[("version_nb", &version.to_string())], ), - SecretError::CorruptSession { error, item } => (error, Some(item)), - SecretError::Oo7(error) => (error.to_user_facing(), None), - error => (error.to_string(), None), + SecretError::Invalid(error) => error, + SecretError::Oo7(error) => error.to_user_facing(), + error => error.to_string(), } } } @@ -195,10 +194,9 @@ impl StoredSession { Ok(version) => version, Err(error) => { error!("Could not parse 'version' attribute in stored session: {error}"); - return Err(SecretError::CorruptSession { - error: gettext("Malformed version in stored session"), - item, - }); + return Err(SecretError::Invalid(gettext( + "Malformed version in stored session", + ))); } }, None => 0, @@ -219,10 +217,9 @@ impl StoredSession { // It's an error if the version is at least 2 but there is no profile. // Versions older than 2 will be migrated. None if version >= 2 => { - return Err(SecretError::CorruptSession { - error: gettext("Could not find profile in stored session"), - item, - }); + return Err(SecretError::Invalid(gettext( + "Could not find profile in stored session", + ))); } // No issue for other cases. _ => {} @@ -233,17 +230,15 @@ impl StoredSession { Ok(homeserver) => homeserver, Err(error) => { error!("Could not parse 'homeserver' attribute in stored session: {error}"); - return Err(SecretError::CorruptSession { - error: gettext("Malformed homeserver in stored session"), - item, - }); + return Err(SecretError::Invalid(gettext( + "Malformed homeserver in stored session", + ))); } }, None => { - return Err(SecretError::CorruptSession { - error: gettext("Could not find homeserver in stored session"), - item, - }); + return Err(SecretError::Invalid(gettext( + "Could not find homeserver in stored session", + ))); } }; let user_id = match attr.get("user") { @@ -251,35 +246,31 @@ impl StoredSession { Ok(user_id) => user_id, Err(error) => { error!("Could not parse 'user' attribute in stored session: {error}"); - return Err(SecretError::CorruptSession { - error: gettext("Malformed user ID in stored session"), - item, - }); + return Err(SecretError::Invalid(gettext( + "Malformed user ID in stored session", + ))); } }, None => { - return Err(SecretError::CorruptSession { - error: gettext("Could not find user ID in stored session"), - item, - }); + return Err(SecretError::Invalid(gettext( + "Could not find user ID in stored session", + ))); } }; let device_id = match attr.get("device-id") { Some(string) => <&DeviceId>::from(string.as_str()).to_owned(), None => { - return Err(SecretError::CorruptSession { - error: gettext("Could not find device ID in stored session"), - item, - }); + return Err(SecretError::Invalid(gettext( + "Could not find device ID in stored session", + ))); } }; let path = match attr.get("db-path") { Some(string) => PathBuf::from(string), None => { - return Err(SecretError::CorruptSession { - error: gettext("Could not find database path in stored session"), - item, - }); + return Err(SecretError::Invalid(gettext( + "Could not find database path in stored session", + ))); } }; let secret = match item.secret().await { @@ -289,10 +280,9 @@ impl StoredSession { Ok(secret) => secret, Err(error) => { error!("Could not parse secret in stored session: {error:?}"); - return Err(SecretError::CorruptSession { - error: gettext("Malformed secret in stored session"), - item, - }); + return Err(SecretError::Invalid(gettext( + "Malformed secret in stored session", + ))); } } } else { @@ -300,20 +290,18 @@ impl StoredSession { Ok(secret) => secret, Err(error) => { error!("Could not parse secret in stored session: {error}"); - return Err(SecretError::CorruptSession { - error: gettext("Malformed secret in stored session"), - item, - }); + return Err(SecretError::Invalid(gettext( + "Malformed secret in stored session", + ))); } } } } Err(error) => { error!("Could not get secret in stored session: {error}"); - return Err(SecretError::CorruptSession { - error: gettext("Could not get secret in stored session"), - item, - }); + return Err(SecretError::Invalid(gettext( + "Could not get secret in stored session", + ))); } }; @@ -493,7 +481,7 @@ impl StoredSession { /// 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) { + pub async fn migrate_to_v4(&mut self, item: Item) { warn!( "Session {} with version {} found for user {}, migrating to version 4…", self.id(), @@ -519,12 +507,13 @@ impl StoredSession { self.version = 4; + let clone = self.clone(); spawn_tokio!(async move { if let Err(error) = item.delete().await { error!("Failed to remove outdated session: {error}"); } - if let Err(error) = self.store().await { + if let Err(error) = clone.store().await { error!("Failed to store updated session: {error}"); } }) @@ -584,8 +573,22 @@ pub async fn restore_sessions() -> Result, SecretError> { match StoredSession::try_from_secret_item(item).await { Ok(session) => sessions.push(session), + Err(SecretError::OldVersion { item, mut session }) => { + if session.version == 0 { + warn!( + "Found old session for {} 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); + } + } Err(SecretError::WrongProfile) => {} - Err(error) => return Err(error), + Err(error) => { + error!("Failed to restore previous session: {error}"); + } } } diff --git a/src/window.rs b/src/window.rs index 223d23b5..58b8e0a0 100644 --- a/src/window.rs +++ b/src/window.rs @@ -13,7 +13,7 @@ use crate::{ greeter::Greeter, login::Login, prelude::*, - secret::{self, SecretError, StoredSession}, + secret::{self, StoredSession}, session::{ model::{Session, SessionState}, view::{AccountSettings, SessionView}, @@ -349,34 +349,15 @@ impl Window { } } } - Err(error) => match error { - SecretError::OldVersion { item, session } => { - if session.version == 0 { - warn!("Found old session with sled store, removing…"); - session.delete(Some(item), true).await - } else if session.version < 4 { - session.migrate_to_v4(item).await - } + Err(error) => { + error!("Failed to restore previous sessions: {error}"); - // Restart. - spawn!(clone!(@weak self as obj => async move { - obj.restore_sessions().await; - })); - } - _ => { - error!("Failed to restore previous sessions: {error}"); - - let (message, item) = error.into_parts(); - self.switch_to_error_page( - &format!( - "{}\n\n{}", - gettext("Failed to restore previous sessions"), - message, - ), - item, - ); - } - }, + self.switch_to_error_page(&format!( + "{}\n\n{}", + gettext("Failed to restore previous sessions"), + error.to_user_facing(), + )); + } } } @@ -481,9 +462,9 @@ impl Window { imp.main_stack.set_visible_child(&*imp.greeter); } - pub fn switch_to_error_page(&self, message: &str, item: Option) { + pub fn switch_to_error_page(&self, message: &str) { let imp = self.imp(); - imp.error_page.display_secret_error(message, item); + imp.error_page.display_secret_error(message); imp.main_stack.set_visible_child(&*imp.error_page); }