From 94061ac65997ee5a1cfb752decbfe2ae45268a36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Wed, 8 Apr 2026 16:32:27 +0200 Subject: [PATCH] feat(qr-code): Add yet another state to the login granting process This separates the step of opening the verification URI in a browser and closing the browser. --- bindings/matrix-sdk-ffi/src/qr_code.rs | 48 +++++--- .../src/authentication/oauth/qrcode/grant.rs | 103 ++++++++++++++---- 2 files changed, 117 insertions(+), 34 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/qr_code.rs b/bindings/matrix-sdk-ffi/src/qr_code.rs index 34aac4c43..ecb924364 100644 --- a/bindings/matrix-sdk-ffi/src/qr_code.rs +++ b/bindings/matrix-sdk-ffi/src/qr_code.rs @@ -610,11 +610,14 @@ pub enum GrantQrLoginProgress { }, /// The secure channel has been confirmed using the [`CheckCode`] and this /// device is waiting for the authorization to complete. - WaitingForAuth { + OpeningVerificationUri { /// A URI to open in a (secure) system browser to verify the new login. verification_uri: String, continuation_sender: Arc, }, + WaitingForAuth { + continuation_sender: Arc, + }, /// We are syncing secrets. SyncingSecrets, /// The login has successfully finished. @@ -638,11 +641,15 @@ impl From> for GrantQrLoginProgress { check_code_string: format!("{check_code:02}"), } } - GrantLoginProgress::WaitingForAuth { verification_uri, continuation_sender } => { - Self::WaitingForAuth { - verification_uri: verification_uri.into(), - continuation_sender: Arc::new(continuation_sender.into()), - } + GrantLoginProgress::OpeningVerificationUri { + verification_uri, + continuation_sender, + } => Self::OpeningVerificationUri { + verification_uri: verification_uri.into(), + continuation_sender: Arc::new(continuation_sender.into()), + }, + GrantLoginProgress::WaitingForAuth { continuation_sender } => { + Self::WaitingForAuth { continuation_sender: Arc::new(continuation_sender.into()) } } GrantLoginProgress::SyncingSecrets => Self::SyncingSecrets, GrantLoginProgress::Done => Self::Done, @@ -659,17 +666,24 @@ pub enum GrantGeneratedQrLoginProgress { Starting, /// We have established the secure channel and now need to display the /// QR code so that the existing device can scan it. - QrReady { qr_code: Arc }, + QrReady { + qr_code: Arc, + }, /// The existing device has scanned the QR code and is displaying the /// checkcode. We now need to ask the user to enter the checkcode so that /// we can verify that the channel is indeed secure. - QrScanned { check_code_sender: Arc }, + QrScanned { + check_code_sender: Arc, + }, /// The secure channel has been confirmed using the [`CheckCode`] and this /// device is waiting for the authorization to complete. - WaitingForAuth { + OpeningVerificationUri { /// A URI to open in a (secure) system browser to verify the new login. verification_uri: String, - continuation_message_sender: Arc, + continuation_sender: Arc, + }, + WaitingForAuth { + continuation_sender: Arc, }, /// We are syncing secrets. SyncingSecrets, @@ -694,11 +708,15 @@ impl From> for GrantGeneratedQrL GrantLoginProgress::EstablishingSecureChannel(GeneratedQrProgress::QrScanned( inner, )) => Self::QrScanned { check_code_sender: Arc::new(CheckCodeSender { inner }) }, - GrantLoginProgress::WaitingForAuth { verification_uri, continuation_sender } => { - Self::WaitingForAuth { - verification_uri: verification_uri.into(), - continuation_message_sender: Arc::new(continuation_sender.into()), - } + GrantLoginProgress::OpeningVerificationUri { + verification_uri, + continuation_sender, + } => Self::OpeningVerificationUri { + verification_uri: verification_uri.into(), + continuation_sender: Arc::new(continuation_sender.into()), + }, + GrantLoginProgress::WaitingForAuth { continuation_sender } => { + Self::WaitingForAuth { continuation_sender: Arc::new(continuation_sender.into()) } } GrantLoginProgress::SyncingSecrets => Self::SyncingSecrets, GrantLoginProgress::Done => Self::Done, diff --git a/crates/matrix-sdk/src/authentication/oauth/qrcode/grant.rs b/crates/matrix-sdk/src/authentication/oauth/qrcode/grant.rs index 95298658b..1991e3288 100644 --- a/crates/matrix-sdk/src/authentication/oauth/qrcode/grant.rs +++ b/crates/matrix-sdk/src/authentication/oauth/qrcode/grant.rs @@ -119,13 +119,13 @@ async fn finish_login_grant( .map_err(|e| QRCodeGrantLoginError::Unknown(e.to_string()))?; let (sender, receiver) = tokio::sync::oneshot::channel(); - state.set(GrantLoginProgress::WaitingForAuth { + state.set(GrantLoginProgress::OpeningVerificationUri { verification_uri, continuation_sender: ContinuationMessageSender(CloneableSender::new(sender)), }); - // We wait for this device to finish up the login in the browser, only then do - // we continue. + // We wait for this device to open the verification URI, this gives the device + // the chance to tell us that it couldn't have opened the verification URI. match receiver.await { Ok(ContinuationMessage::Confirm) => {} // If the user sends an explicit cancellation or the sender is dropped without sending we @@ -153,8 +153,28 @@ async fn finish_login_grant( // The new device displays the user code it received from the authorization // server and starts polling for an access token. In parallel, the user // consents to the new login in the browser on this device, while verifying - // the user code displayed on the other device. -- MSC4108 OAuth 2.0 login - // steps 5 & 6 + // the user code displayed on the other device. -- MSC4108 OAuth 2.0 login steps + // 5 & 6 + + let (sender, receiver) = tokio::sync::oneshot::channel(); + state.set(GrantLoginProgress::WaitingForAuth { + 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::UnableToOpenVerificationUri, + homeserver: None, + }) + .await?; + return Ok(()); + } + } // We wait for the new device to send us the m.login.success or m.login.failure // message @@ -218,13 +238,20 @@ pub enum GrantLoginProgress { /// and/or [`CheckCode`]. EstablishingSecureChannel(Q), /// The secure channel has been confirmed using the [`CheckCode`] and this - /// device is waiting for the authorization to complete. - WaitingForAuth { + /// device is waiting for verification URI to be opened. + OpeningVerificationUri { /// A URI to open in a (secure) system browser to verify the new login. verification_uri: Url, - /// A sender to confirm that the login has been veriried in the system - /// browser or to cancel because the verification URI couldn't - /// be opened. + /// A sender to confirm that the verification URI has been successfully + /// opened, or cancel if the URI could not have been opened. + continuation_sender: ContinuationMessageSender, + }, + /// The verification URI has been opened and the device is waiting for the + /// authorization to complete. + WaitingForAuth { + /// A sender to confirm that the authorization using the verification + /// URI has been completed. We can now wait on the other side to confirm + /// the receival of the access token. continuation_sender: ContinuationMessageSender, }, /// The new device has been granted access and this device is sending the @@ -965,7 +992,7 @@ mod test { .await .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { + GrantLoginProgress::OpeningVerificationUri { verification_uri, continuation_sender, } => { @@ -978,6 +1005,10 @@ mod test { assert_eq!(verification_uri.as_str(), verification_uri_complete); continuation_sender.confirm().await.expect("should be able to confirm"); } + GrantLoginProgress::WaitingForAuth { continuation_sender } => { + assert_matches!(state, GrantLoginProgress::OpeningVerificationUri { .. }); + continuation_sender.confirm().await.expect("should be able to confirm"); + } GrantLoginProgress::SyncingSecrets => { assert_matches!(state, GrantLoginProgress::WaitingForAuth { .. }); } @@ -1107,7 +1138,7 @@ mod test { .send(*check_code) .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { + GrantLoginProgress::OpeningVerificationUri { verification_uri, continuation_sender, } => { @@ -1118,6 +1149,12 @@ mod test { assert_eq!(verification_uri.as_str(), verification_uri_complete); continuation_sender.confirm().await.expect("should be able to confirm"); } + + GrantLoginProgress::WaitingForAuth { continuation_sender } => { + assert_matches!(state, GrantLoginProgress::OpeningVerificationUri { .. }); + continuation_sender.confirm().await.expect("should be able to confirm"); + } + GrantLoginProgress::SyncingSecrets => { assert_matches!(state, GrantLoginProgress::WaitingForAuth { .. }); } @@ -1251,7 +1288,7 @@ mod test { .send(*check_code) .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { + GrantLoginProgress::OpeningVerificationUri { verification_uri, continuation_sender, } => { @@ -1262,6 +1299,10 @@ mod test { assert_eq!(verification_uri.as_str(), verification_uri_complete); continuation_sender.confirm().await.expect("should be able to confirm"); } + GrantLoginProgress::WaitingForAuth { continuation_sender } => { + assert_matches!(state, GrantLoginProgress::OpeningVerificationUri { .. }); + continuation_sender.confirm().await.expect("should be able to confirm"); + } GrantLoginProgress::SyncingSecrets => { assert_matches!(state, GrantLoginProgress::WaitingForAuth { .. }); } @@ -1883,7 +1924,7 @@ mod test { .await .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { + GrantLoginProgress::OpeningVerificationUri { verification_uri, continuation_sender, } => { @@ -1896,6 +1937,10 @@ mod test { assert_eq!(verification_uri.as_str(), verification_uri_complete); continuation_sender.confirm().await.expect("should be able to confirm"); } + GrantLoginProgress::WaitingForAuth { continuation_sender } => { + assert_matches!(state, GrantLoginProgress::OpeningVerificationUri { .. }); + continuation_sender.confirm().await.expect("should be able to confirm"); + } _ => { panic!("Alice should abort the process"); } @@ -2012,7 +2057,7 @@ mod test { .send(*check_code) .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { + GrantLoginProgress::OpeningVerificationUri { verification_uri, continuation_sender, } => { @@ -2023,6 +2068,10 @@ mod test { assert_eq!(verification_uri.as_str(), verification_uri_complete); continuation_sender.confirm().await.expect("should be able to confirm"); } + GrantLoginProgress::WaitingForAuth { continuation_sender } => { + assert_matches!(state, GrantLoginProgress::OpeningVerificationUri { .. }); + continuation_sender.confirm().await.expect("should be able to confirm"); + } _ => { panic!("Alice should abort the process"); } @@ -2553,7 +2602,7 @@ mod test { .await .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { + GrantLoginProgress::OpeningVerificationUri { verification_uri, continuation_sender, } => { @@ -2566,6 +2615,10 @@ mod test { assert_eq!(verification_uri.as_str(), verification_uri_complete); continuation_sender.confirm().await.expect("should be able to confirm"); } + GrantLoginProgress::WaitingForAuth { continuation_sender } => { + assert_matches!(state, GrantLoginProgress::OpeningVerificationUri { .. }); + continuation_sender.confirm().await.expect("should be able to confirm"); + } _ => { panic!("Alice should abort the process"); } @@ -2687,7 +2740,7 @@ mod test { .send(*check_code) .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { + GrantLoginProgress::OpeningVerificationUri { verification_uri, continuation_sender, } => { @@ -2698,6 +2751,10 @@ mod test { assert_eq!(verification_uri.as_str(), verification_uri_complete); continuation_sender.confirm().await.expect("should be able to confirm"); } + GrantLoginProgress::WaitingForAuth { continuation_sender } => { + assert_matches!(state, GrantLoginProgress::OpeningVerificationUri { .. }); + continuation_sender.confirm().await.expect("should be able to confirm"); + } _ => { panic!("Alice should abort the process"); } @@ -2836,7 +2893,7 @@ mod test { .await .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { + GrantLoginProgress::OpeningVerificationUri { verification_uri, continuation_sender, } => { @@ -2849,6 +2906,10 @@ mod test { assert_eq!(verification_uri.as_str(), verification_uri_complete); continuation_sender.confirm().await.expect("should be able to confirm"); } + GrantLoginProgress::WaitingForAuth { continuation_sender } => { + assert_matches!(state, GrantLoginProgress::OpeningVerificationUri { .. }); + continuation_sender.confirm().await.expect("should be able to confirm"); + } _ => { panic!("Alice should abort the process"); } @@ -2971,7 +3032,7 @@ mod test { .send(*check_code) .expect("Alice should be able to forward the checkcode"); } - GrantLoginProgress::WaitingForAuth { + GrantLoginProgress::OpeningVerificationUri { verification_uri, continuation_sender, } => { @@ -2982,6 +3043,10 @@ mod test { assert_eq!(verification_uri.as_str(), verification_uri_complete); continuation_sender.confirm().await.expect("should be able to confirm"); } + GrantLoginProgress::WaitingForAuth { continuation_sender } => { + assert_matches!(state, GrantLoginProgress::OpeningVerificationUri { .. }); + continuation_sender.confirm().await.expect("should be able to confirm"); + } _ => { panic!("Alice should abort the process"); }