diff --git a/bindings/matrix-sdk-ffi/changelog.d/6711.changed.md b/bindings/matrix-sdk-ffi/changelog.d/6711.changed.md new file mode 100644 index 00000000000..21c2bd8262e --- /dev/null +++ b/bindings/matrix-sdk-ffi/changelog.d/6711.changed.md @@ -0,0 +1,9 @@ +[**breaking**] `GrantQrLoginProgress::WaitingForAuth` and +`GrantGeneratedQrLoginProgress::WaitingForAuth` now have a +`continuation_sender: ContinuationMessageSender` field. Applications must call +`continuation_sender.confirm()` once the verification URI has been opened in +the browser and the application is ready to proceed, or +`continuation_sender.cancel()` to abort; previously it proceeded automatically +as soon as this state was reached. This lets applications that suspend or +navigate away while the verification URI is open to resume the process +explicitly. diff --git a/bindings/matrix-sdk-ffi/src/qr_code.rs b/bindings/matrix-sdk-ffi/src/qr_code.rs index 96ffc4cef10..fecd19f8fe9 100644 --- a/bindings/matrix-sdk-ffi/src/qr_code.rs +++ b/bindings/matrix-sdk-ffi/src/qr_code.rs @@ -17,8 +17,9 @@ use std::sync::Arc; use matrix_sdk::authentication::oauth::{ OAuth, qrcode::{ - self, CheckCodeSender as SdkCheckCodeSender, CheckCodeSenderError, - DeviceCodeErrorResponseType, GeneratedQrProgress, LoginFailureReason, QrProgress, + self, CheckCodeSender as SdkCheckCodeSender, + ContinuationMessageSender as SdkContinuationMessageSender, DeviceCodeErrorResponseType, + GeneratedQrProgress, LoginFailureReason, QrProgress, SenderError, }, }; use matrix_sdk_base::crypto::types::qr_login::{self, QrCodeIntent}; @@ -390,11 +391,11 @@ impl From for HumanQrLoginError { } } -impl From for HumanQrLoginError { - fn from(value: CheckCodeSenderError) -> Self { +impl From for HumanQrLoginError { + fn from(value: SenderError) -> Self { match value { - CheckCodeSenderError::AlreadySent => HumanQrLoginError::CheckCodeAlreadySent, - CheckCodeSenderError::CannotSend => HumanQrLoginError::CheckCodeCannotBeSent, + SenderError::AlreadySent => HumanQrLoginError::CheckCodeAlreadySent, + SenderError::CannotSend => HumanQrLoginError::CheckCodeCannotBeSent, } } } @@ -614,6 +615,12 @@ pub enum GrantQrLoginProgress { WaitingForAuth { /// A URI to open in a (secure) system browser to verify the new login. verification_uri: String, + /// A sender to confirm that the authorization using the verification + /// URI has been started in the browser and that the application is + /// ready to proceed. This allows applications that suspend or navigate + /// away while the verification URI is open to resume the process + /// explicitly. + continuation_sender: Arc, }, /// We are syncing secrets. SyncingSecrets, @@ -640,8 +647,11 @@ impl From> for GrantQrLoginProgress { check_code_string: format!("{check_code:02}"), } } - GrantLoginProgress::WaitingForAuth { verification_uri } => { - Self::WaitingForAuth { verification_uri: verification_uri.into() } + GrantLoginProgress::WaitingForAuth { verification_uri, continuation_sender } => { + Self::WaitingForAuth { + verification_uri: verification_uri.into(), + continuation_sender: Arc::new(continuation_sender.into()), + } } GrantLoginProgress::SyncingSecrets => Self::SyncingSecrets, GrantLoginProgress::Done => Self::Done, @@ -668,6 +678,12 @@ pub enum GrantGeneratedQrLoginProgress { WaitingForAuth { /// A URI to open in a (secure) system browser to verify the new login. verification_uri: String, + /// A sender to confirm that the authorization using the verification + /// URI has been started in the browser and that the application is + /// ready to proceed. This allows applications that suspend or navigate + /// away while the verification URI is open to resume the process + /// explicitly. + continuation_sender: Arc, }, /// We are syncing secrets. SyncingSecrets, @@ -692,8 +708,11 @@ impl From> for GrantGeneratedQrL GrantLoginProgress::EstablishingSecureChannel(GeneratedQrProgress::QrScanned( inner, )) => Self::QrScanned { check_code_sender: Arc::new(CheckCodeSender { inner }) }, - GrantLoginProgress::WaitingForAuth { verification_uri } => { - Self::WaitingForAuth { verification_uri: verification_uri.into() } + GrantLoginProgress::WaitingForAuth { verification_uri, continuation_sender } => { + Self::WaitingForAuth { + verification_uri: verification_uri.into(), + continuation_sender: Arc::new(continuation_sender.into()), + } } GrantLoginProgress::SyncingSecrets => Self::SyncingSecrets, GrantLoginProgress::Done => Self::Done, @@ -701,9 +720,9 @@ impl From> for GrantGeneratedQrL } } -#[derive(Debug, uniffi::Object)] /// Used to pass back the [`CheckCode`] entered by the user to verify that the /// secure channel is indeed secure. +#[derive(Debug, uniffi::Object)] pub struct CheckCodeSender { inner: SdkCheckCodeSender, } @@ -721,3 +740,30 @@ impl CheckCodeSender { self.inner.send(code).await.map_err(HumanQrLoginError::from) } } + +/// Struct used to let the QR code granting logic know that it can continue with +/// the process since applications might suspend things while the verification +/// URI is open. +#[derive(Debug, Clone, uniffi::Object)] +pub struct ContinuationMessageSender { + inner: SdkContinuationMessageSender, +} + +#[matrix_sdk_ffi_macros::export] +impl ContinuationMessageSender { + /// Confirm the continuation of the login granting process. + pub async fn confirm(&self) -> Result<(), HumanQrLoginError> { + self.inner.confirm().await.map_err(HumanQrLoginError::from) + } + + /// Cancel the login granting process. + pub async fn cancel(&self) -> Result<(), HumanQrLoginError> { + self.inner.cancel().await.map_err(HumanQrLoginError::from) + } +} + +impl From for ContinuationMessageSender { + fn from(value: SdkContinuationMessageSender) -> Self { + Self { inner: value } + } +} diff --git a/crates/matrix-sdk/changelog.d/6711.changed.md b/crates/matrix-sdk/changelog.d/6711.changed.md new file mode 100644 index 00000000000..5987413ef60 --- /dev/null +++ b/crates/matrix-sdk/changelog.d/6711.changed.md @@ -0,0 +1,11 @@ +[**breaking**] `GrantLoginProgress::WaitingForAuth` now has a +`continuation_sender: ContinuationMessageSender` field. Applications must call +`continuation_sender.confirm()` once the verification URI has been opened in +the browser and the application is ready to proceed, or +`continuation_sender.cancel()` to abort; previously it proceeded automatically +as soon as this state was reached. This lets applications that suspend or +navigate away while the verification URI is open to resume the process +explicitly. + +[**breaking**] `qrcode::CheckCodeSenderError` has been renamed to +`qrcode::SenderError`, since it is now shared with `ContinuationMessageSender`. diff --git a/crates/matrix-sdk/src/authentication/oauth/mod.rs b/crates/matrix-sdk/src/authentication/oauth/mod.rs index fa23d684c08..9a4e5a53453 100644 --- a/crates/matrix-sdk/src/authentication/oauth/mod.rs +++ b/crates/matrix-sdk/src/authentication/oauth/mod.rs @@ -1619,8 +1619,12 @@ impl<'a> GrantLoginWithQrCodeBuilder<'a> { /// GrantLoginProgress::EstablishingSecureChannel(QrProgress { check_code }) => { /// println!("Please enter the checkcode on your other device: {:?}", check_code); /// } - /// GrantLoginProgress::WaitingForAuth { verification_uri } => { - /// println!("Please open {verification_uri} to confirm the new login") + /// GrantLoginProgress::WaitingForAuth { verification_uri, continuation_sender } => { + /// println!("Please open {verification_uri} to confirm the new login"); + /// + /// // Once the new login has been confirmed in the browser, we can let the + /// // client continue with the process. + /// continuation_sender.confirm().await?; /// }, /// GrantLoginProgress::Done => break, /// } @@ -1696,8 +1700,12 @@ impl<'a> GrantLoginWithQrCodeBuilder<'a> { /// let check_code = s.trim().parse::()?; /// checkcode_sender.send(check_code).await?; /// } - /// GrantLoginProgress::WaitingForAuth { verification_uri } => { - /// println!("Please open {verification_uri} to confirm the new login") + /// GrantLoginProgress::WaitingForAuth { verification_uri, continuation_sender } => { + /// println!("Please open {verification_uri} to confirm the new login"); + /// + /// // Once the new login has been confirmed in the browser, we can let the + /// // client continue with the process. + /// continuation_sender.confirm().await?; /// }, /// GrantLoginProgress::Done => break, /// } diff --git a/crates/matrix-sdk/src/authentication/oauth/qrcode/grant.rs b/crates/matrix-sdk/src/authentication/oauth/qrcode/grant.rs index b2aced751a2..f02b74c21d1 100644 --- a/crates/matrix-sdk/src/authentication/oauth/qrcode/grant.rs +++ b/crates/matrix-sdk/src/authentication/oauth/qrcode/grant.rs @@ -36,8 +36,9 @@ use super::{ use crate::{ Client, authentication::oauth::qrcode::{ - CheckCodeSender, GeneratedQrProgress, LoginFailureReason, QRCodeGrantLoginError, - QrProgress, SecureChannelError, + CheckCodeSender, CloneableSender, ContinuationMessage, ContinuationMessageSender, + GeneratedQrProgress, LoginFailureReason, QRCodeGrantLoginError, QrProgress, + SecureChannelError, }, }; @@ -116,8 +117,29 @@ async fn finish_login_grant( .as_str(), ) .map_err(|e| QRCodeGrantLoginError::Unknown(e.to_string()))?; - state.set(GrantLoginProgress::WaitingForAuth { verification_uri }); + let (sender, receiver) = tokio::sync::oneshot::channel(); + state.set(GrantLoginProgress::WaitingForAuth { + verification_uri, + continuation_sender: ContinuationMessageSender(CloneableSender::new(sender)), + }); + + // We wait for this device to confirm that the authorization using the + // verification URI has succeeded. + match receiver.await { + Ok(ContinuationMessage::Confirm) => {} + Ok(ContinuationMessage::Cancel) | Err(_) => { + channel + .send_json(QrAuthMessage::LoginFailure { + reason: LoginFailureReason::UserCancelled, + homeserver: None, + }) + .await?; + return Err(QRCodeGrantLoginError::LoginFailure { + reason: LoginFailureReason::UserCancelled, + }); + } + } // We send the new device the m.login.protocol_accepted message to let it know // that the consent process is in progress. // -- MSC4108 OAuth 2.0 login step 4 continued @@ -196,6 +218,12 @@ pub enum GrantLoginProgress { WaitingForAuth { /// A URI to open in a (secure) system browser to verify the new login. verification_uri: Url, + /// A sender to confirm that the authorization using the verification + /// URI has been started in the browser and that the application is + /// ready to proceed. This allows applications that suspend or navigate + /// away while the verification URI is open to resume the process + /// explicitly. + continuation_sender: ContinuationMessageSender, }, /// The new device has been granted access and this device is sending the /// secrets to it. @@ -875,7 +903,10 @@ mod test { .await .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { verification_uri } => { + GrantLoginProgress::WaitingForAuth { + verification_uri, + continuation_sender, + } => { assert_matches!( state, GrantLoginProgress::EstablishingSecureChannel( @@ -883,6 +914,7 @@ mod test { ) ); assert_eq!(verification_uri.as_str(), verification_uri_complete); + continuation_sender.confirm().await.expect("should be able to confirm"); } GrantLoginProgress::SyncingSecrets => { assert_matches!(state, GrantLoginProgress::WaitingForAuth { .. }); @@ -1003,12 +1035,16 @@ mod test { .send(check_code.to_digit()) .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { verification_uri } => { + GrantLoginProgress::WaitingForAuth { + verification_uri, + continuation_sender, + } => { assert_matches!( state, GrantLoginProgress::EstablishingSecureChannel(QrProgress { .. }) ); assert_eq!(verification_uri.as_str(), verification_uri_complete); + continuation_sender.confirm().await.expect("should be able to confirm"); } GrantLoginProgress::SyncingSecrets => { assert_matches!(state, GrantLoginProgress::WaitingForAuth { .. }); @@ -1132,12 +1168,16 @@ mod test { .send(check_code.to_digit()) .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { verification_uri } => { + GrantLoginProgress::WaitingForAuth { + verification_uri, + continuation_sender, + } => { assert_matches!( state, GrantLoginProgress::EstablishingSecureChannel(QrProgress { .. }) ); assert_eq!(verification_uri.as_str(), verification_uri_complete); + continuation_sender.confirm().await.expect("should be able to confirm"); } GrantLoginProgress::SyncingSecrets => { assert_matches!(state, GrantLoginProgress::WaitingForAuth { .. }); @@ -1755,7 +1795,10 @@ mod test { .await .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { verification_uri } => { + GrantLoginProgress::WaitingForAuth { + verification_uri, + continuation_sender, + } => { assert_matches!( state, GrantLoginProgress::EstablishingSecureChannel( @@ -1763,6 +1806,7 @@ mod test { ) ); assert_eq!(verification_uri.as_str(), verification_uri_complete); + continuation_sender.confirm().await.expect("should be able to confirm"); } _ => { panic!("Alice should abort the process"); @@ -1879,12 +1923,16 @@ mod test { .send(check_code.to_digit()) .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { verification_uri } => { + GrantLoginProgress::WaitingForAuth { + verification_uri, + continuation_sender, + } => { assert_matches!( state, GrantLoginProgress::EstablishingSecureChannel(QrProgress { .. }) ); assert_eq!(verification_uri.as_str(), verification_uri_complete); + continuation_sender.confirm().await.expect("should be able to confirm"); } _ => { panic!("Alice should abort the process"); @@ -2405,7 +2453,10 @@ mod test { .await .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { verification_uri } => { + GrantLoginProgress::WaitingForAuth { + verification_uri, + continuation_sender, + } => { assert_matches!( state, GrantLoginProgress::EstablishingSecureChannel( @@ -2413,6 +2464,7 @@ mod test { ) ); assert_eq!(verification_uri.as_str(), verification_uri_complete); + continuation_sender.confirm().await.expect("should be able to confirm"); } _ => { panic!("Alice should abort the process"); @@ -2534,12 +2586,16 @@ mod test { .send(check_code.to_digit()) .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { verification_uri } => { + GrantLoginProgress::WaitingForAuth { + verification_uri, + continuation_sender, + } => { assert_matches!( state, GrantLoginProgress::EstablishingSecureChannel(QrProgress { .. }) ); assert_eq!(verification_uri.as_str(), verification_uri_complete); + continuation_sender.confirm().await.expect("should be able to confirm"); } _ => { panic!("Alice should abort the process"); @@ -2678,7 +2734,10 @@ mod test { .await .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { verification_uri } => { + GrantLoginProgress::WaitingForAuth { + verification_uri, + continuation_sender, + } => { assert_matches!( state, GrantLoginProgress::EstablishingSecureChannel( @@ -2686,6 +2745,7 @@ mod test { ) ); assert_eq!(verification_uri.as_str(), verification_uri_complete); + continuation_sender.confirm().await.expect("should be able to confirm"); } _ => { panic!("Alice should abort the process"); @@ -2808,12 +2868,16 @@ mod test { .send(check_code.to_digit()) .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { verification_uri } => { + GrantLoginProgress::WaitingForAuth { + verification_uri, + continuation_sender, + } => { assert_matches!( state, GrantLoginProgress::EstablishingSecureChannel(QrProgress { .. }) ); assert_eq!(verification_uri.as_str(), verification_uri_complete); + continuation_sender.confirm().await.expect("should be able to confirm"); } _ => { panic!("Alice should abort the process"); diff --git a/crates/matrix-sdk/src/authentication/oauth/qrcode/mod.rs b/crates/matrix-sdk/src/authentication/oauth/qrcode/mod.rs index c1af524b1e9..61427ff5a99 100644 --- a/crates/matrix-sdk/src/authentication/oauth/qrcode/mod.rs +++ b/crates/matrix-sdk/src/authentication/oauth/qrcode/mod.rs @@ -329,7 +329,7 @@ pub struct QrProgress { /// /// We have established the secure channel, but we need to let the /// other device know about the [`QrCodeData`] so they can connect to the -/// channel and let us know about the checkcode so we can verify that the +/// channel and let us know about the check code so we can verify that the /// channel is indeed secure. #[derive(Clone, Debug)] pub enum GeneratedQrProgress { @@ -337,45 +337,81 @@ pub enum GeneratedQrProgress { /// device to scan it. QrReady(QrCodeData), /// The QR code has been scanned by the other device and this device is - /// waiting for the user to put in the checkcode displayed on the + /// waiting for the user to put in the check code displayed on the /// other device. QrScanned(CheckCodeSender), } -/// Used to pass back the checkcode entered by the user to verify that the -/// secure channel is indeed secure. -#[derive(Clone, Debug)] -pub struct CheckCodeSender { - inner: Arc>>>, -} +/// A oneshot sender used to send the check code back to the device that +/// generated the QR code. +pub type CheckCodeSender = CloneableSender; impl CheckCodeSender { - pub(crate) fn new(tx: tokio::sync::oneshot::Sender) -> Self { - Self { inner: Arc::new(Mutex::new(Some(tx))) } - } - - /// Send the checkcode. + /// Send the check code. /// /// Calling this method more than once will result in an error. /// /// # Arguments /// /// * `check_code` - The check code in digits representation. - pub async fn send(&self, check_code: u8) -> Result<(), CheckCodeSenderError> { + pub async fn send(&self, check_code: u8) -> Result<(), SenderError> { + self.send_impl(check_code).await + } +} + +/// The internal message of the [`ContinuationMessageSender`] to either continue +/// the login granting process or to cancel it. +#[derive(Clone, Copy, Debug)] +pub(crate) enum ContinuationMessage { + Confirm, + Cancel, +} + +/// Struct used to let the QR code granting logic know that it can continue with +/// the process since applications might suspend things while the verification +/// URI is open. +#[derive(Clone, Debug)] +pub struct ContinuationMessageSender(CloneableSender); + +impl ContinuationMessageSender { + /// Confirm the continuation of the login granting process. + pub async fn confirm(&self) -> Result<(), SenderError> { + self.0.send_impl(ContinuationMessage::Confirm).await + } + + /// Cancel the login granting process. + pub async fn cancel(&self) -> Result<(), SenderError> { + self.0.send_impl(ContinuationMessage::Cancel).await + } +} + +/// A oneshot sender we are able to clone so we can put it into a +/// [`SharedObservable`](eyeball::SharedObservable). +#[derive(Clone, Debug)] +pub struct CloneableSender { + inner: Arc>>>, +} + +impl CloneableSender { + pub(crate) fn new(tx: tokio::sync::oneshot::Sender) -> Self { + Self { inner: Arc::new(Mutex::new(Some(tx))) } + } + + async fn send_impl(&self, message: T) -> Result<(), SenderError> { match self.inner.lock().await.take() { - Some(tx) => tx.send(check_code).map_err(|_| CheckCodeSenderError::CannotSend), - None => Err(CheckCodeSenderError::AlreadySent), + Some(tx) => tx.send(message).map_err(|_| SenderError::CannotSend), + None => Err(SenderError::AlreadySent), } } } -/// Possible errors when calling [`CheckCodeSender::send`]. +/// Possible errors when calling [`CloneableSender::send`]. #[derive(Debug, thiserror::Error)] -pub enum CheckCodeSenderError { - /// The check code has already been sent. - #[error("check code already sent.")] +pub enum SenderError { + /// The message has already been sent. + #[error("message already sent.")] AlreadySent, - /// The check code cannot be sent. - #[error("check code cannot be sent.")] + /// The message cannot be sent. + #[error("message cannot be sent.")] CannotSend, }