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.

Author: poljar

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<ContinuationMessageSender>,
     },
+    WaitingForAuth {
+        continuation_sender: Arc<ContinuationMessageSender>,
+    },
     /// We are syncing secrets.
     SyncingSecrets,
     /// The login has successfully finished.
@@ -638,11 +641,15 @@ impl From<qrcode::GrantLoginProgress<QrProgress>> for GrantQrLoginProgress {
                     check_code_string: format!("{check_code:02}"),
                 }
             }
-            GrantLoginProgress::Authenticating { 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<QrCodeData> },
+    QrReady {
+        qr_code: Arc<QrCodeData>,
+    },
     /// 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<CheckCodeSender> },
+    QrScanned {
+        check_code_sender: Arc<CheckCodeSender>,
+    },
     /// 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<ContinuationMessageSender>,
+        continuation_sender: Arc<ContinuationMessageSender>,
+    },
+    WaitingForAuth {
+        continuation_sender: Arc<ContinuationMessageSender>,
     },
     /// We are syncing secrets.
     SyncingSecrets,
@@ -694,11 +708,15 @@ impl From<qrcode::GrantLoginProgress<GeneratedQrProgress>> for GrantGeneratedQrL
             GrantLoginProgress::EstablishingSecureChannel(GeneratedQrProgress::QrScanned(
                 inner,
             )) => Self::QrScanned { check_code_sender: Arc::new(CheckCodeSender { inner }) },
-            GrantLoginProgress::Authenticating { 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,

crates/matrix-sdk/src/authentication/oauth/qrcode/grant.rs

@@ -124,10 +124,8 @@ async fn finish_login_grant<Q>(
         continuation_sender: ContinuationMessageSender(CloneableSender::new(sender)),
     });
 
-    // Here we are waiting for the verification URI to be opened?
-
-    // 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
@@ -152,13 +150,31 @@ async fn finish_login_grant<Q>(
     let message = QrAuthMessage::LoginProtocolAccepted;
     channel.send_json(&message).await?;
 
-    // TODO: Wait here for what exactly, for the auth to succeed.
-
     // 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
@@ -222,13 +238,20 @@ pub enum GrantLoginProgress<Q> {
     /// and/or [`CheckCode`].
     EstablishingSecureChannel(Q),
     /// The secure channel has been confirmed using the [`CheckCode`] and this
-    /// device is waiting for the authorization to complete.
+    /// 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
@@ -982,8 +1005,12 @@ mod test {
                         assert_eq!(verification_uri.as_str(), verification_uri_complete);
                         continuation_sender.confirm().await.expect("should be able to confirm");
                     }
-                    GrantLoginProgress::SyncingSecrets => {
+                    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 { .. });
                     }
                     GrantLoginProgress::Done => {
                         assert_matches!(state, GrantLoginProgress::SyncingSecrets);
@@ -1122,8 +1149,14 @@ mod test {
                         assert_eq!(verification_uri.as_str(), verification_uri_complete);
                         continuation_sender.confirm().await.expect("should be able to confirm");
                     }
-                    GrantLoginProgress::SyncingSecrets => {
+
+                    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 { .. });
                     }
                     GrantLoginProgress::Done => {
                         assert_matches!(state, GrantLoginProgress::SyncingSecrets);
@@ -1266,8 +1299,12 @@ mod test {
                         assert_eq!(verification_uri.as_str(), verification_uri_complete);
                         continuation_sender.confirm().await.expect("should be able to confirm");
                     }
-                    GrantLoginProgress::SyncingSecrets => {
+                    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 { .. });
                     }
                     GrantLoginProgress::Done => {
                         assert_matches!(state, GrantLoginProgress::SyncingSecrets);
@@ -1900,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");
                     }
@@ -2027,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");
                     }
@@ -2570,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");
                     }
@@ -2702,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");
                     }
@@ -2853,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");
                     }
@@ -2986,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");
                     }