webauthn: make GetDisplayName return a std::string.

This "display" name is never actually displayed and never contains non-ASCII. Drop the use of string16 and clarify the different between it and |GetId|. Change-Id: I0abf916d9dc21dac7f8d3e50a303ee649df05647 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2575039 Commit-Queue: Martin Kreichgauer <martinkr@google.com> Auto-Submit: Adam Langley <agl@chromium.org> Reviewed-by: Martin Kreichgauer <martinkr@google.com> Cr-Commit-Position: refs/heads/master@{#834867}
Denger-Network · Dec 8, 2020 · 96470d8 · 96470d8
1 parent 140fa08
commit 96470d8
Show file tree

Hide file tree

Showing 24 changed files with 44 additions and 54 deletions.
diff --git a/chrome/browser/webauthn/authenticator_reference.cc b/chrome/browser/webauthn/authenticator_reference.cc
@@ -6,10 +6,8 @@
 
 AuthenticatorReference::AuthenticatorReference(
     base::StringPiece authenticator_id,
-    base::StringPiece16 authenticator_display_name,
     device::FidoTransportProtocol transport)
     : authenticator_id(authenticator_id),
-      authenticator_display_name(authenticator_display_name),
       transport(transport) {}
 
 AuthenticatorReference::AuthenticatorReference(AuthenticatorReference&& data) =

diff --git a/chrome/browser/webauthn/authenticator_reference.h b/chrome/browser/webauthn/authenticator_reference.h
@@ -17,14 +17,12 @@
 // from the user via the UI.
 struct AuthenticatorReference {
   AuthenticatorReference(base::StringPiece device_id,
-                         base::StringPiece16 authenticator_display_name,
                          device::FidoTransportProtocol transport);
   AuthenticatorReference(AuthenticatorReference&& data);
   AuthenticatorReference& operator=(AuthenticatorReference&& other);
   ~AuthenticatorReference();
 
   std::string authenticator_id;
-  base::string16 authenticator_display_name;
   device::FidoTransportProtocol transport;
   bool dispatched = false;
 

diff --git a/chrome/browser/webauthn/authenticator_request_dialog_model.cc b/chrome/browser/webauthn/authenticator_request_dialog_model.cc
@@ -475,8 +475,7 @@ void AuthenticatorRequestDialogModel::AddAuthenticator(
   }
 
   AuthenticatorReference authenticator_reference(
-      authenticator.GetId(), authenticator.GetDisplayName(),
-      *authenticator.AuthenticatorTransport());
+      authenticator.GetId(), *authenticator.AuthenticatorTransport());
 
   ephemeral_state_.saved_authenticators_.AddAuthenticator(
       std::move(authenticator_reference));

diff --git a/chrome/browser/webauthn/authenticator_request_dialog_model_unittest.cc b/chrome/browser/webauthn/authenticator_request_dialog_model_unittest.cc
@@ -517,7 +517,6 @@ TEST_F(AuthenticatorRequestDialogModelTest,
       &num_called));
   model.saved_authenticators().AddAuthenticator(
       AuthenticatorReference("authenticator" /* authenticator_id */,
-                             base::string16() /* authenticator_display_name */,
                              AuthenticatorTransport::kInternal));
 
   model.StartFlow(std::move(transports_info), base::nullopt);

diff --git a/device/fido/cable/fido_cable_device.cc b/device/fido/cable/fido_cable_device.cc
@@ -62,7 +62,7 @@ FidoCableDevice::~FidoCableDevice() = default;
 
 // static
 std::string FidoCableDevice::GetIdForAddress(const std::string& address) {
-  return "ble:" + address;
+  return "ble-" + address;
 }
 
 std::string FidoCableDevice::GetAddress() {
@@ -120,14 +120,6 @@ std::string FidoCableDevice::GetId() const {
   return GetIdForAddress(connection_->address());
 }
 
-base::string16 FidoCableDevice::GetDisplayName() const {
-  auto* device = connection_->GetBleDevice();
-  if (!device)
-    return base::string16();
-
-  return device->GetNameForDisplay();
-}
-
 FidoTransportProtocol FidoCableDevice::DeviceTransport() const {
   return FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy;
 }

diff --git a/device/fido/cable/fido_cable_device.h b/device/fido/cable/fido_cable_device.h
@@ -53,7 +53,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoCableDevice : public FidoDevice {
   // FidoDevice:
   void Cancel(CancelToken token) override;
   std::string GetId() const override;
-  base::string16 GetDisplayName() const override;
   FidoTransportProtocol DeviceTransport() const override;
   CancelToken DeviceTransact(std::vector<uint8_t> command,
                              DeviceCallback callback) override;

diff --git a/device/fido/cable/fido_cable_device_unittest.cc b/device/fido/cable/fido_cable_device_unittest.cc
@@ -181,11 +181,11 @@ TEST_F(FidoCableDeviceTest, ConnectionFailureTest) {
 
 TEST_F(FidoCableDeviceTest, StaticGetIdTest) {
   std::string address = BluetoothTestBase::kTestDeviceAddress1;
-  EXPECT_EQ("ble:" + address, FidoCableDevice::GetIdForAddress(address));
+  EXPECT_EQ("ble-" + address, FidoCableDevice::GetIdForAddress(address));
 }
 
 TEST_F(FidoCableDeviceTest, GetIdTest) {
-  EXPECT_EQ(std::string("ble:") + BluetoothTestBase::kTestDeviceAddress1,
+  EXPECT_EQ(std::string("ble-") + BluetoothTestBase::kTestDeviceAddress1,
             device()->GetId());
 }
 

diff --git a/device/fido/cros/authenticator.cc b/device/fido/cros/authenticator.cc
@@ -33,10 +33,6 @@ std::string ChromeOSAuthenticator::GetId() const {
   return "ChromeOSAuthenticator";
 }
 
-base::string16 ChromeOSAuthenticator::GetDisplayName() const {
-  return base::string16(base::ASCIIToUTF16("ChromeOS Authenticator"));
-}
-
 namespace {
 
 // DBus timeout for method calls that doesn't involve user interaction.

diff --git a/device/fido/cros/authenticator.h b/device/fido/cros/authenticator.h
@@ -50,7 +50,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) ChromeOSAuthenticator
   void GetNextAssertion(GetAssertionCallback callback) override {}
   void Cancel() override;
   std::string GetId() const override;
-  base::string16 GetDisplayName() const override;
   const base::Optional<AuthenticatorSupportedOptions>& Options() const override;
 
   base::Optional<FidoTransportProtocol> AuthenticatorTransport() const override;

diff --git a/device/fido/fido_authenticator.cc b/device/fido/fido_authenticator.cc
@@ -171,6 +171,10 @@ void FidoAuthenticator::Reset(ResetCallback callback) {
                           base::nullopt);
 }
 
+std::string FidoAuthenticator::GetDisplayName() const {
+  return GetId();
+}
+
 ProtocolVersion FidoAuthenticator::SupportedProtocol() const {
   return ProtocolVersion::kUnknown;
 }

diff --git a/device/fido/fido_authenticator.h b/device/fido/fido_authenticator.h
@@ -236,8 +236,14 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoAuthenticator {
   // stored resident keys and any configured PIN.
   virtual void Reset(ResetCallback callback);
   virtual void Cancel() = 0;
+  // GetId returns a unique string representing this device. This string should
+  // be distinct from all other devices concurrently discovered.
   virtual std::string GetId() const = 0;
-  virtual base::string16 GetDisplayName() const = 0;
+  // GetDisplayName returns a string identifying a device to a human, which
+  // might not be unique. For example, |GetDisplayName| could return the VID:PID
+  // of a HID device, but |GetId| could not because two devices can share the
+  // same VID:PID. It defaults to returning the value of |GetId|.
+  virtual std::string GetDisplayName() const;
   virtual ProtocolVersion SupportedProtocol() const;
   virtual bool SupportsCredProtectExtension() const;
   virtual bool SupportsHMACSecretExtension() const;

diff --git a/device/fido/fido_device.cc b/device/fido/fido_device.cc
@@ -21,9 +21,8 @@ void FidoDevice::TryWink(base::OnceClosure callback) {
   std::move(callback).Run();
 }
 
-base::string16 FidoDevice::GetDisplayName() const {
-  const auto id = GetId();
-  return base::string16(id.begin(), id.end());
+std::string FidoDevice::GetDisplayName() const {
+  return GetId();
 }
 
 bool FidoDevice::IsInPairingMode() const {

diff --git a/device/fido/fido_device.h b/device/fido/fido_device.h
@@ -79,8 +79,14 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDevice {
   // cancel may be unsuccessful and that the request may complete normally.
   // It is safe to attempt to cancel an operation that has already completed.
   virtual void Cancel(CancelToken token) = 0;
+  // GetId returns a unique string representing this device. This string should
+  // be distinct from all other devices concurrently discovered.
   virtual std::string GetId() const = 0;
-  virtual base::string16 GetDisplayName() const;
+  // GetDisplayName returns a string identifying a device to a human, which
+  // might not be unique. For example, |GetDisplayName| could return the VID:PID
+  // of a HID device, but |GetId| could not because two devices can share the
+  // same VID:PID. It defaults to returning the value of |GetId|.
+  virtual std::string GetDisplayName() const;
   virtual FidoTransportProtocol DeviceTransport() const = 0;
 
   // These must only be called on Bluetooth devices.

diff --git a/device/fido/fido_device_authenticator.cc b/device/fido/fido_device_authenticator.cc
@@ -951,7 +951,7 @@ std::string FidoDeviceAuthenticator::GetId() const {
   return device_->GetId();
 }
 
-base::string16 FidoDeviceAuthenticator::GetDisplayName() const {
+std::string FidoDeviceAuthenticator::GetDisplayName() const {
   return device_->GetDisplayName();
 }
 

diff --git a/device/fido/fido_device_authenticator.h b/device/fido/fido_device_authenticator.h
@@ -115,7 +115,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
   void Reset(ResetCallback callback) override;
   void Cancel() override;
   std::string GetId() const override;
-  base::string16 GetDisplayName() const override;
+  std::string GetDisplayName() const override;
   ProtocolVersion SupportedProtocol() const override;
   bool SupportsHMACSecretExtension() const override;
   bool SupportsEnterpriseAttestation() const override;

diff --git a/device/fido/fido_request_handler_unittest.cc b/device/fido/fido_request_handler_unittest.cc
@@ -442,7 +442,7 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleFailureResponses) {
       test_data::kTestAuthenticatorGetInfoResponse);
   EXPECT_CALL(*device0, GetId()).WillRepeatedly(testing::Return("device0"));
   EXPECT_CALL(*device0, GetDisplayName())
-      .WillRepeatedly(testing::Return(base::string16()));
+      .WillRepeatedly(testing::Return(std::string()));
   device0->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
                                        CreateFakeDeviceProcesssingError());
 
@@ -454,7 +454,7 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleFailureResponses) {
       test_data::kTestAuthenticatorGetInfoResponse);
   EXPECT_CALL(*device1, GetId()).WillRepeatedly(testing::Return("device1"));
   EXPECT_CALL(*device1, GetDisplayName())
-      .WillRepeatedly(testing::Return(base::string16()));
+      .WillRepeatedly(testing::Return(std::string()));
   device1->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
                                        CreateFakeUserPresenceVerifiedError(),
                                        base::TimeDelta::FromMicroseconds(1));
@@ -467,7 +467,7 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleFailureResponses) {
       test_data::kTestAuthenticatorGetInfoResponse);
   EXPECT_CALL(*device2, GetId()).WillRepeatedly(testing::Return("device2"));
   EXPECT_CALL(*device2, GetDisplayName())
-      .WillRepeatedly(testing::Return(base::string16()));
+      .WillRepeatedly(testing::Return(std::string()));
   device2->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
                                        CreateFakeDeviceProcesssingError(),
                                        base::TimeDelta::FromMicroseconds(10));

diff --git a/device/fido/hid/fido_hid_device.cc b/device/fido/hid/fido_hid_device.cc
@@ -551,14 +551,6 @@ void FidoHidDevice::WriteCancel() {
                            base::BindOnce(WriteCancelComplete, connection_));
 }
 
-std::string FidoHidDevice::GetId() const {
-  return GetIdForDevice(*device_info_);
-}
-
-FidoTransportProtocol FidoHidDevice::DeviceTransport() const {
-  return FidoTransportProtocol::kUsbHumanInterfaceDevice;
-}
-
 // VidPidToString returns the device's vendor and product IDs as formatted by
 // the lsusb utility.
 static std::string VidPidToString(const mojom::HidDeviceInfoPtr& device_info) {
@@ -574,6 +566,18 @@ static std::string VidPidToString(const mojom::HidDeviceInfoPtr& device_info) {
                             base::HexEncode(&product_id, 2));
 }
 
+std::string FidoHidDevice::GetDisplayName() const {
+  return "usb-" + VidPidToString(device_info_);
+}
+
+std::string FidoHidDevice::GetId() const {
+  return GetIdForDevice(*device_info_);
+}
+
+FidoTransportProtocol FidoHidDevice::DeviceTransport() const {
+  return FidoTransportProtocol::kUsbHumanInterfaceDevice;
+}
+
 void FidoHidDevice::DiscoverSupportedProtocolAndDeviceInfo(
     base::OnceClosure done) {
   // The following devices cannot handle GetInfo messages.

diff --git a/device/fido/hid/fido_hid_device.h b/device/fido/hid/fido_hid_device.h
@@ -44,6 +44,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoHidDevice final : public FidoDevice {
                              DeviceCallback callback) final;
   void TryWink(base::OnceClosure callback) final;
   void Cancel(CancelToken token) final;
+  std::string GetDisplayName() const final;
   std::string GetId() const final;
   FidoTransportProtocol DeviceTransport() const final;
   void DiscoverSupportedProtocolAndDeviceInfo(base::OnceClosure done) override;

diff --git a/device/fido/mac/authenticator.h b/device/fido/mac/authenticator.h
@@ -59,7 +59,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdAuthenticator
   void GetNextAssertion(GetAssertionCallback callback) override;
   void Cancel() override;
   std::string GetId() const override;
-  base::string16 GetDisplayName() const override;
   const base::Optional<AuthenticatorSupportedOptions>& Options() const override;
   base::Optional<FidoTransportProtocol> AuthenticatorTransport() const override;
   bool IsInPairingMode() const override;

diff --git a/device/fido/mac/authenticator.mm b/device/fido/mac/authenticator.mm
@@ -128,10 +128,6 @@ new TouchIdAuthenticator(std::move(config.keychain_access_group),
   return "TouchIdAuthenticator";
 }
 
-base::string16 TouchIdAuthenticator::GetDisplayName() const {
-  return base::string16();
-}
-
 base::Optional<FidoTransportProtocol>
 TouchIdAuthenticator::AuthenticatorTransport() const {
   return FidoTransportProtocol::kInternal;

diff --git a/device/fido/mock_fido_device.cc b/device/fido/mock_fido_device.cc
@@ -201,7 +201,7 @@ void MockFidoDevice::SetDeviceTransport(
 
 void MockFidoDevice::StubGetDisplayName() {
   EXPECT_CALL(*this, GetDisplayName())
-      .WillRepeatedly(testing::Return(base::string16()));
+      .WillRepeatedly(testing::Return(std::string()));
 }
 
 }  // namespace device
diff --git a/device/fido/mock_fido_device.h b/device/fido/mock_fido_device.h
@@ -77,7 +77,7 @@ class MockFidoDevice : public ::testing::StrictMock<FidoDevice> {
                              DeviceCallback cb) override;
   MOCK_METHOD1(Cancel, void(FidoDevice::CancelToken));
   MOCK_CONST_METHOD0(GetId, std::string(void));
-  MOCK_CONST_METHOD0(GetDisplayName, base::string16(void));
+  MOCK_CONST_METHOD0(GetDisplayName, std::string(void));
   FidoTransportProtocol DeviceTransport() const override;
   base::WeakPtr<FidoDevice> GetWeakPtr() override;
 

diff --git a/device/fido/win/authenticator.cc b/device/fido/win/authenticator.cc
@@ -159,10 +159,6 @@ std::string WinWebAuthnApiAuthenticator::GetId() const {
   return "WinWebAuthnApiAuthenticator";
 }
 
-base::string16 WinWebAuthnApiAuthenticator::GetDisplayName() const {
-  return base::UTF8ToUTF16(GetId());
-}
-
 bool WinWebAuthnApiAuthenticator::IsInPairingMode() const {
   return false;
 }

diff --git a/device/fido/win/authenticator.h b/device/fido/win/authenticator.h
@@ -57,7 +57,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) WinWebAuthnApiAuthenticator
   void GetTouch(base::OnceClosure callback) override;
   void Cancel() override;
   std::string GetId() const override;
-  base::string16 GetDisplayName() const override;
   bool IsInPairingMode() const override;
   bool IsPaired() const override;
   bool RequiresBlePairingPin() const override;