serial: Implement persistent permissions on Linux and macOS

This change updates the serial port enumerators for Linux and macOS to
read the serial number (if available) for USB devices and use this to
construct a persistent ID for these devices.

This means that only USB devices will have permissions stored
persistently. Support for remembering other types of devices will be
investigated in the future.

Bug: 908836
Change-Id: I28c53b880e69723447c445d447d677f2799c2ebd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2265357
Auto-Submit: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782158}

chrome/browser/serial/serial_chooser_context.cc

@@ -42,12 +42,19 @@ base::UnguessableToken DecodeToken(base::StringPiece input) {
 }
 
 bool CanStorePersistentEntry(const device::mojom::SerialPortInfo& port) {
+  // If there is no display name then the path name will be used instead. The
+  // path name is not guaranteed to be stable. For example, on Linux the name
+  // "ttyUSB0" is reused for any USB serial device. A name like that would be
+  // confusing to show in settings when the device is disconnected.
+  if (!port.display_name || port.display_name->empty())
+    return false;
+
   return port.persistent_id && !port.persistent_id->empty();
 }
 
 base::Value PortInfoToValue(const device::mojom::SerialPortInfo& port) {
   base::Value value(base::Value::Type::DICTIONARY);
-  if (port.display_name)
+  if (port.display_name && !port.display_name->empty())
     value.SetStringKey(kPortNameKey, *port.display_name);
   else
     value.SetStringKey(kPortNameKey, port.path.LossyDisplayName());

chrome/browser/serial/serial_chooser_context_unittest.cc

@@ -128,6 +128,7 @@ TEST_F(SerialChooserContextTest, GrantAndRevokePersistentPermission) {
 
   auto port = device::mojom::SerialPortInfo::New();
   port->token = base::UnguessableToken::Create();
+  port->display_name = "Persistent Port";
   port->persistent_id = "ABC123";
 
   EXPECT_FALSE(context()->HasPortPermission(origin, origin, *port));
@@ -203,12 +204,52 @@ TEST_F(SerialChooserContextTest, EphemeralPermissionRevokedOnDisconnect) {
   EXPECT_EQ(0u, objects.size());
 }
 
+TEST_F(SerialChooserContextTest, PersistenceRequiresDisplayName) {
+  const auto origin = url::Origin::Create(GURL("https://google.com"));
+
+  auto port = device::mojom::SerialPortInfo::New();
+  port->token = base::UnguessableToken::Create();
+  // port->display_name is left unset.
+  port->persistent_id = "ABC123";
+  port_manager().AddPort(port.Clone());
+
+  context()->GrantPortPermission(origin, origin, *port);
+  EXPECT_TRUE(context()->HasPortPermission(origin, origin, *port));
+
+  EXPECT_CALL(permission_observer(),
+              OnChooserObjectPermissionChanged(
+                  ContentSettingsType::SERIAL_GUARD,
+                  ContentSettingsType::SERIAL_CHOOSER_DATA));
+  EXPECT_CALL(permission_observer(), OnPermissionRevoked(origin, origin));
+
+  // Without a display name a persistent permission cannot be recorded and so
+  // removing the device will revoke permission.
+  port_manager().RemovePort(port->token);
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(port_observer(), OnPortRemoved(testing::_))
+        .WillOnce(
+            testing::Invoke([&](const device::mojom::SerialPortInfo& info) {
+              EXPECT_EQ(port->token, info.token);
+              EXPECT_TRUE(context()->HasPortPermission(origin, origin, info));
+              run_loop.Quit();
+            }));
+    run_loop.Run();
+  }
+  EXPECT_FALSE(context()->HasPortPermission(origin, origin, *port));
+  auto origin_objects = context()->GetGrantedObjects(origin, origin);
+  EXPECT_EQ(0u, origin_objects.size());
+  auto objects = context()->GetAllGrantedObjects();
+  EXPECT_EQ(0u, objects.size());
+}
+
 TEST_F(SerialChooserContextTest, PersistentPermissionNotRevokedOnDisconnect) {
   const auto origin = url::Origin::Create(GURL("https://google.com"));
   const char persistent_id[] = "ABC123";
 
   auto port = device::mojom::SerialPortInfo::New();
   port->token = base::UnguessableToken::Create();
+  port->display_name = "Persistent Port";
   port->persistent_id = persistent_id;
   port_manager().AddPort(port.Clone());
 

chrome/browser/ui/serial/serial_chooser_controller.cc

@@ -67,7 +67,7 @@ base::string16 SerialChooserController::GetOption(size_t index) const {
   // serial port and to differentiate between ports with similar display names.
   base::string16 display_path = port.path.BaseName().LossyDisplayName();
 
-  if (port.display_name) {
+  if (port.display_name && !port.display_name->empty()) {
     return l10n_util::GetStringFUTF16(IDS_SERIAL_PORT_CHOOSER_NAME_WITH_PATH,
                                       base::UTF8ToUTF16(*port.display_name),
                                       display_path);

services/device/serial/serial_device_enumerator_linux.cc

@@ -15,6 +15,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/threading/scoped_blocking_call.h"
 #include "device/udev_linux/udev.h"
 
@@ -166,6 +167,8 @@ void SerialDeviceEnumeratorLinux::CreatePort(ScopedUdevDevicePtr device,
       udev_device_get_property_value(device.get(), "ID_PRODUCT_ID");
   const char* product_name_enc =
       udev_device_get_property_value(device.get(), "ID_MODEL_ENC");
+  const char* serial_number =
+      udev_device_get_property_value(device.get(), "ID_SERIAL_SHORT");
 
   uint32_t int_value;
   if (vendor_id && base::HexStringToUInt(vendor_id, &int_value)) {
@@ -177,7 +180,12 @@ void SerialDeviceEnumeratorLinux::CreatePort(ScopedUdevDevicePtr device,
     info->has_product_id = true;
   }
   if (product_name_enc)
-    info->display_name.emplace(device::UdevDecodeString(product_name_enc));
+    info->display_name = device::UdevDecodeString(product_name_enc);
+
+  if (info->has_vendor_id && info->has_product_id && serial_number) {
+    info->persistent_id = base::StringPrintf("%04X-%04X-%s", info->vendor_id,
+                                             info->product_id, serial_number);
+  }
 
   paths_.insert(std::make_pair(syspath, token));
   AddPort(std::move(info));

services/device/serial/serial_device_enumerator_mac.cc

@@ -171,10 +171,14 @@ void SerialDeviceEnumeratorMac::AddDevices() {
       info->product_id = *product_id;
     }
 
-    base::Optional<std::string> display_name =
+    info->display_name =
         GetStringProperty(device.get(), CFSTR(kUSBProductString));
-    if (display_name) {
-      info->display_name = std::move(*display_name);
+
+    base::Optional<std::string> serial_number =
+        GetStringProperty(device.get(), CFSTR(kUSBSerialNumberString));
+    if (vendor_id && product_id && serial_number) {
+      info->persistent_id = base::StringPrintf(
+          "%04X-%04X-%s", *vendor_id, *product_id, serial_number->c_str());
     }
 
     // Each serial device has two paths associated with it: a "dialin" path

services/device/serial/serial_device_enumerator_win.cc

@@ -277,6 +277,7 @@ void SerialDeviceEnumeratorWin::EnumeratePort(HDEVINFO dev_info,
   info->path = *path;
   info->persistent_id = instance_id;
 
+  // TODO(https://crbug.com/1015074): Read the real USB strings here.
   std::string display_name;
   if (GetDisplayName(*friendly_name, &display_name))
     info->display_name = std::move(display_name);