webauthn: make caBLE complete callback a method of the |Platform|.

Rather than passing a separate callback into each of the methods that
create a Transaction, have the completed signal be a method on the
|Platform|.

BUG=1002262

Change-Id: Ie63e5ace91177127efa9f84ef49c4412139f13ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2500276
Commit-Queue: Adam Langley <agl@chromium.org>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#822764}

chrome/android/features/cablev2_authenticator/native/cablev2_authenticator_android.cc

@@ -137,6 +137,15 @@ GlobalData& GetGlobalData() {
   return *global_data;
 }
 
+void ResetGlobalData() {
+  GlobalData& global_data = GetGlobalData();
+  global_data.current_transaction.reset();
+  global_data.pending_make_credential_callback.reset();
+  global_data.pending_get_assertion_callback.reset();
+  global_data.usb_callback.reset();
+  global_data.last_event.reset();
+}
+
 // AndroidBLEAdvert wraps a Java |BLEAdvert| object so that
 // |authenticator::Platform| can hold it.
 class AndroidBLEAdvert
@@ -208,6 +217,13 @@ class AndroidPlatform : public device::cablev2::authenticator::Platform {
         ToJavaArrayOfByteArray(env_, params->allowed_cred_ids));
   }
 
+  void OnCompleted(bool success) override {
+    Java_CableAuthenticator_onComplete(env_, cable_authenticator_);
+    // ResetGlobalData will delete the |Transaction|, which will delete this
+    // object. Thus nothing else can be done after this call.
+    ResetGlobalData();
+  }
+
   std::unique_ptr<BLEAdvert> SendBLEAdvert(
       base::span<const uint8_t, device::cablev2::kAdvertSize> payload)
       override {
@@ -224,23 +240,6 @@ class AndroidPlatform : public device::cablev2::authenticator::Platform {
   SEQUENCE_CHECKER(sequence_checker_);
 };
 
-void ResetGlobalData() {
-  GlobalData& global_data = GetGlobalData();
-  global_data.current_transaction.reset();
-  global_data.pending_make_credential_callback.reset();
-  global_data.pending_get_assertion_callback.reset();
-  global_data.usb_callback.reset();
-  global_data.last_event.reset();
-}
-
-// TransactionComplete is called by |authenticator::Platform| whenever a
-// transaction has completed.
-void TransactionComplete(JNIEnv* env,
-                         ScopedJavaGlobalRef<jobject> cable_authenticator) {
-  ResetGlobalData();
-  Java_CableAuthenticator_onComplete(env, cable_authenticator);
-}
-
 // USBTransport wraps the Java |USBHandler| object so that
 // |authenticator::Platform| can use it as a transport.
 class USBTransport : public device::cablev2::authenticator::Transport {
@@ -355,9 +354,7 @@ static void JNI_CableAuthenticator_StartUSB(
       device::cablev2::authenticator::TransactWithPlaintextTransport(
           std::make_unique<AndroidPlatform>(env, cable_authenticator),
           std::unique_ptr<device::cablev2::authenticator::Transport>(
-              transport.release()),
-          base::BindOnce(&TransactionComplete, env,
-                         ScopedJavaGlobalRef<jobject>(cable_authenticator)));
+              transport.release()));
 }
 
 static jboolean JNI_CableAuthenticator_StartQR(
@@ -380,9 +377,7 @@ static jboolean JNI_CableAuthenticator_StartQR(
           std::make_unique<AndroidPlatform>(env, cable_authenticator),
           global_data.network_context, global_data.root_secret,
           ConvertJavaStringToUTF8(authenticator_name), decoded_qr->secret,
-          decoded_qr->peer_identity, global_data.registration->contact_id(),
-          base::BindOnce(&TransactionComplete, env,
-                         ScopedJavaGlobalRef<jobject>(cable_authenticator)));
+          decoded_qr->peer_identity, global_data.registration->contact_id());
 
   return true;
 }
@@ -400,9 +395,7 @@ static void JNI_CableAuthenticator_StartFCM(
           std::make_unique<AndroidPlatform>(env, cable_authenticator),
           global_data.network_context, global_data.root_secret,
           event.routing_id, event.tunnel_id, event.pairing_id,
-          event.client_nonce,
-          base::BindOnce(&TransactionComplete, env,
-                         ScopedJavaGlobalRef<jobject>(cable_authenticator)));
+          event.client_nonce);
 }
 
 static void JNI_CableAuthenticator_Stop(JNIEnv* env) {

content/browser/webauth/authenticator_impl_unittest.cc

@@ -6116,7 +6116,7 @@ TEST_F(CableV2AuthenticatorImplTest, QRBasedWithNoPairing) {
                                                           &virtual_device_),
           network_context_.get(), root_secret_, "Test Authenticator",
           zero_qr_secret_, peer_identity_x962_,
-          /*contact_id=*/base::nullopt, base::DoNothing());
+          /*contact_id=*/base::nullopt);
 
   EXPECT_EQ(AuthenticatorMakeCredential().status, AuthenticatorStatus::SUCCESS);
   EXPECT_EQ(pairings_.size(), 0u);
@@ -6140,7 +6140,7 @@ TEST_F(CableV2AuthenticatorImplTest, PairingBased) {
                                                           &virtual_device_),
           network_context_.get(), root_secret_, "Test Authenticator",
           zero_qr_secret_, peer_identity_x962_,
-          /*contact_id=*/std::vector<uint8_t>({1, 2, 3}), base::DoNothing());
+          /*contact_id=*/std::vector<uint8_t>({1, 2, 3}));
 
   EXPECT_EQ(AuthenticatorMakeCredential().status, AuthenticatorStatus::SUCCESS);
   EXPECT_EQ(pairings_.size(), 1u);
@@ -6170,7 +6170,7 @@ TEST_F(CableV2AuthenticatorImplTest, PairingBased) {
             device::cablev2::authenticator::NewMockPlatform(discovery_ptr,
                                                             &virtual_device_),
             network_context_.get(), root_secret_, routing_id, tunnel_id,
-            pairing_id, client_nonce, base::DoNothing());
+            pairing_id, client_nonce);
       });
 
   AuthenticatorEnvironmentImpl::GetInstance()

device/fido/cable/v2_authenticator.cc

@@ -459,11 +459,8 @@ class TunnelTransport : public Transport {
 class CTAP2Processor : public Transaction {
  public:
   CTAP2Processor(std::unique_ptr<Transport> transport,
-                 std::unique_ptr<Platform> platform,
-                 Transaction::CompleteCallback complete_callback)
-      : transport_(std::move(transport)),
-        platform_(std::move(platform)),
-        complete_callback_(std::move(complete_callback)) {
+                 std::unique_ptr<Platform> platform)
+      : transport_(std::move(transport)), platform_(std::move(platform)) {
     transport_->StartReading(
         base::BindRepeating(&CTAP2Processor::OnData, base::Unretained(this)));
   }
@@ -474,7 +471,7 @@ class CTAP2Processor : public Transaction {
 
     if (!msg) {
       FIDO_LOG(ERROR) << "Closing transaction due to transport EOF";
-      std::move(complete_callback_).Run();
+      platform_->OnCompleted(transaction_done_);
       return;
     }
 
@@ -482,7 +479,7 @@ class CTAP2Processor : public Transaction {
     if (!response) {
       // Fatal error.
       // TODO: need to signal this to the UI.
-      std::move(complete_callback_).Run();
+      platform_->OnCompleted(false);
       return;
     }
 
@@ -692,6 +689,7 @@ class CTAP2Processor : public Transaction {
                       response_payload->end());
     }
 
+    transaction_done_ = true;
     transport_->Write(std::move(response));
   }
 
@@ -729,12 +727,13 @@ class CTAP2Processor : public Transaction {
                       response_payload->end());
     }
 
+    transaction_done_ = true;
     transport_->Write(std::move(response));
   }
 
+  bool transaction_done_ = false;
   const std::unique_ptr<Transport> transport_;
   const std::unique_ptr<Platform> platform_;
-  Transaction::CompleteCallback complete_callback_;
   SEQUENCE_CHECKER(sequence_checker_);
   base::WeakPtrFactory<CTAP2Processor> weak_factory_{this};
 };
@@ -833,10 +832,9 @@ Transaction::~Transaction() = default;
 
 std::unique_ptr<Transaction> TransactWithPlaintextTransport(
     std::unique_ptr<Platform> platform,
-    std::unique_ptr<Transport> transport,
-    Transaction::CompleteCallback complete_callback) {
-  return std::make_unique<CTAP2Processor>(
-      std::move(transport), std::move(platform), std::move(complete_callback));
+    std::unique_ptr<Transport> transport) {
+  return std::make_unique<CTAP2Processor>(std::move(transport),
+                                          std::move(platform));
 }
 
 std::unique_ptr<Transaction> TransactFromQRCode(
@@ -846,8 +844,7 @@ std::unique_ptr<Transaction> TransactFromQRCode(
     const std::string& authenticator_name,
     base::span<const uint8_t, 16> qr_secret,
     base::span<const uint8_t, kP256X962Length> peer_identity,
-    base::Optional<std::vector<uint8_t>> contact_id,
-    Transaction::CompleteCallback complete_callback) {
+    base::Optional<std::vector<uint8_t>> contact_id) {
   auto generate_pairing_data = PairingDataGenerator::GetClosure(
       root_secret, authenticator_name, contact_id);
 
@@ -856,7 +853,7 @@ std::unique_ptr<Transaction> TransactFromQRCode(
       std::make_unique<TunnelTransport>(platform_ptr, network_context,
                                         qr_secret, peer_identity,
                                         std::move(generate_pairing_data)),
-      std::move(platform), std::move(complete_callback));
+      std::move(platform));
 }
 
 std::unique_ptr<Transaction> TransactFromFCM(
@@ -866,8 +863,7 @@ std::unique_ptr<Transaction> TransactFromFCM(
     std::array<uint8_t, kRoutingIdSize> routing_id,
     base::span<const uint8_t, kTunnelIdSize> tunnel_id,
     base::span<const uint8_t> pairing_id,
-    base::span<const uint8_t, kClientNonceSize> client_nonce,
-    Transaction::CompleteCallback complete_callback) {
+    base::span<const uint8_t, kClientNonceSize> client_nonce) {
   std::array<uint8_t, 32> paired_secret;
   paired_secret = Derive<EXTENT(paired_secret)>(
       root_secret, pairing_id, DerivedValueType::kPairedSecret);
@@ -877,7 +873,7 @@ std::unique_ptr<Transaction> TransactFromFCM(
       std::make_unique<TunnelTransport>(platform_ptr, network_context,
                                         paired_secret, client_nonce, routing_id,
                                         tunnel_id, IdentityKey(root_secret)),
-      std::move(platform), std::move(complete_callback));
+      std::move(platform));
 }
 
 }  // namespace authenticator

device/fido/cable/v2_authenticator.h

@@ -82,6 +82,11 @@ class Platform {
 
   virtual void GetAssertion(std::unique_ptr<GetAssertionParams> params) = 0;
 
+  // OnCompleted is called when the transaction has completed. Note that calling
+  // this may result in the |Transaction| that owns this |Platform| being
+  // deleted.
+  virtual void OnCompleted(bool success) = 0;
+
   virtual std::unique_ptr<BLEAdvert> SendBLEAdvert(
       base::span<const uint8_t, kAdvertSize> payload) = 0;
 };
@@ -112,8 +117,7 @@ class Transaction {
 // caBLEv2 transaction.
 std::unique_ptr<Transaction> TransactWithPlaintextTransport(
     std::unique_ptr<Platform> platform,
-    std::unique_ptr<Transport> transport,
-    Transaction::CompleteCallback complete_callback);
+    std::unique_ptr<Transport> transport);
 
 // TransactFromQRCode starts a network-based transaction based on the decoded
 // contents of a QR code.
@@ -125,8 +129,7 @@ std::unique_ptr<Transaction> TransactFromQRCode(
     // TODO: name this constant.
     base::span<const uint8_t, 16> qr_secret,
     base::span<const uint8_t, kP256X962Length> peer_identity,
-    base::Optional<std::vector<uint8_t>> contact_id,
-    Transaction::CompleteCallback complete_callback);
+    base::Optional<std::vector<uint8_t>> contact_id);
 
 // TransactFromQRCode starts a network-based transaction based on the decoded
 // contents of a cloud message.
@@ -137,8 +140,7 @@ std::unique_ptr<Transaction> TransactFromFCM(
     std::array<uint8_t, kRoutingIdSize> routing_id,
     base::span<const uint8_t, kTunnelIdSize> tunnel_id,
     base::span<const uint8_t> pairing_id,
-    base::span<const uint8_t, kClientNonceSize> client_nonce,
-    Transaction::CompleteCallback complete_callback);
+    base::span<const uint8_t, kClientNonceSize> client_nonce);
 
 }  // namespace authenticator
 }  // namespace cablev2

device/fido/cable/v2_test_util.cc

@@ -378,6 +378,8 @@ class TestPlatform : public authenticator::Platform {
     NOTREACHED();
   }
 
+  void OnCompleted(bool success) override {}
+
   std::unique_ptr<authenticator::Platform::BLEAdvert> SendBLEAdvert(
       base::span<const uint8_t, kAdvertSize> payload) override {
     base::SequencedTaskRunnerHandle::Get()->PostTask(