In order to manage device media gallery permissions, use a device id that is...

In order to manage device media gallery permissions, use a device id that is unique and persistent across device attachments. Therefore, extract device uuid from DiskInfo::InitializeFromResponse and use that identifier while dispatching media device attached notification event.

BUG=none
TEST=none

Review URL: https://chromiumcodereview.appspot.com/10830003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@149099 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/disks/disk_mount_manager.cc

@@ -376,6 +376,7 @@ class DiskMountManagerImpl : public DiskMountManager {
                           disk_info.file_path(),
                           disk_info.label(),
                           disk_info.drive_label(),
+                          disk_info.uuid(),
                           FindSystemPathPrefix(disk_info.system_path()),
                           disk_info.device_type(),
                           disk_info.total_size_in_bytes(),
@@ -566,6 +567,7 @@ DiskMountManager::Disk::Disk(const std::string& device_path,
                              const std::string& file_path,
                              const std::string& device_label,
                              const std::string& drive_label,
+                             const std::string& fs_uuid,
                              const std::string& system_path_prefix,
                              DeviceType device_type,
                              uint64 total_size_in_bytes,
@@ -580,6 +582,7 @@ DiskMountManager::Disk::Disk(const std::string& device_path,
       file_path_(file_path),
       device_label_(device_label),
       drive_label_(drive_label),
+      fs_uuid_(fs_uuid),
       system_path_prefix_(system_path_prefix),
       device_type_(device_type),
       total_size_in_bytes_(total_size_in_bytes),

chrome/browser/chromeos/disks/disk_mount_manager.h

@@ -52,6 +52,7 @@ class DiskMountManager {
          const std::string& file_path,
          const std::string& device_label,
          const std::string& drive_label,
+         const std::string& fs_uuid,
          const std::string& system_path_prefix,
          DeviceType device_type,
          uint64 total_size_in_bytes,
@@ -85,6 +86,9 @@ class DiskMountManager {
     // (e.g. "TransMemory")
     const std::string& drive_label() const { return drive_label_; }
 
+    // Returns the file system uuid string.
+    const std::string& fs_uuid() const { return fs_uuid_; }
+
     // Path of the system device this device's block is a part of.
     // (e.g. /sys/devices/pci0000:00/.../8:0:0:0/)
     const std::string& system_path_prefix() const {
@@ -125,6 +129,7 @@ class DiskMountManager {
     std::string file_path_;
     std::string device_label_;
     std::string drive_label_;
+    std::string fs_uuid_;
     std::string system_path_prefix_;
     DeviceType device_type_;
     uint64 total_size_in_bytes_;

chrome/browser/chromeos/disks/mock_disk_mount_manager.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/chromeos/disks/mock_disk_mount_manager.h"
 
 #include "base/message_loop.h"
+#include "base/stl_util.h"
 #include "base/string_util.h"
 #include "content/public/browser/browser_thread.h"
 
@@ -26,6 +27,7 @@ const char* kTestMountPath = "/media/foofoo";
 const char* kTestFilePath = "/this/file/path";
 const char* kTestDeviceLabel = "A label";
 const char* kTestDriveLabel = "Another label";
+const char* kTestUuid = "FFFF-FFFF";
 
 }  // namespace
 
@@ -50,6 +52,8 @@ MockDiskMountManager::MockDiskMountManager() {
 }
 
 MockDiskMountManager::~MockDiskMountManager() {
+  STLDeleteContainerPairSecondPointers(disks_.begin(), disks_.end());
+  disks_.clear();
 }
 
 void MockDiskMountManager::NotifyDeviceInsertEvents() {
@@ -60,6 +64,7 @@ void MockDiskMountManager::NotifyDeviceInsertEvents() {
       std::string(kTestFilePath),
       std::string(),
       std::string(kTestDriveLabel),
+      std::string(kTestUuid),
       std::string(kTestSystemPathPrefix),
       DEVICE_TYPE_USB,
       4294967295U,
@@ -90,6 +95,7 @@ void MockDiskMountManager::NotifyDeviceInsertEvents() {
       std::string(kTestFilePath),
       std::string(kTestDeviceLabel),
       std::string(kTestDriveLabel),
+      std::string(kTestUuid),
       std::string(kTestSystemPathPrefix),
       DEVICE_TYPE_MOBILE,
       1073741824,
@@ -113,6 +119,7 @@ void MockDiskMountManager::NotifyDeviceRemoveEvents() {
       std::string(kTestFilePath),
       std::string(kTestDeviceLabel),
       std::string(kTestDriveLabel),
+      std::string(kTestUuid),
       std::string(kTestSystemPathPrefix),
       DEVICE_TYPE_SD,
       1073741824,
@@ -148,6 +155,37 @@ void MockDiskMountManager::SetupDefaultReplies() {
       .Times(AnyNumber());
 }
 
+void MockDiskMountManager::CreateDiskEntryForMountDevice(
+    const DiskMountManager::MountPointInfo& mount_info,
+    const std::string& device_id) {
+  Disk* disk = new DiskMountManager::Disk(std::string(mount_info.source_path),
+                                          std::string(mount_info.mount_path),
+                                          std::string(),  // system_path
+                                          std::string(),  // file_path
+                                          std::string(),  // device_label
+                                          std::string(),  // drive_label
+                                          device_id,  // fs_uuid
+                                          std::string(),  // system_path_prefix
+                                          DEVICE_TYPE_USB,  // device_type
+                                          1073741824,  // total_size_in_bytes
+                                          false,  // is_parent
+                                          false,  // is_read_only
+                                          true,  // has_media
+                                          false,  // on_boot_device
+                                          false);  // is_hidden
+  disks_.insert(std::pair<std::string, DiskMountManager::Disk*>(
+      std::string(mount_info.source_path), disk));
+}
+
+void MockDiskMountManager::RemoveDiskEntryForMountDevice(
+    const DiskMountManager::MountPointInfo& mount_info) {
+  DiskMountManager::DiskMap::iterator it = disks_.find(mount_info.source_path);
+  if (it != disks_.end()) {
+    delete it->second;
+    disks_.erase(it);
+  }
+}
+
 void MockDiskMountManager::NotifyDiskChanged(DiskMountManagerEventType event,
                                              const DiskMountManager::Disk* disk)
 {

chrome/browser/chromeos/disks/mock_disk_mount_manager.h

@@ -48,6 +48,17 @@ class MockDiskMountManager : public DiskMountManager {
   // Sets up default results for mock methods.
   void SetupDefaultReplies();
 
+  // Creates a fake disk entry for the mounted device. This function is
+  // primarily for MediaDeviceNotificationsTest.
+  void CreateDiskEntryForMountDevice(
+      const DiskMountManager::MountPointInfo& mount_info,
+      const std::string& device_id);
+
+  // Removes the fake disk entry associated with the mounted device. This
+  // function is primarily for MediaDeviceNotificationsTest.
+  void RemoveDiskEntryForMountDevice(
+      const DiskMountManager::MountPointInfo& mount_info);
+
  private:
   // Is used to implement AddObserver.
   void AddObserverInternal(DiskMountManager::Observer* observer);

chrome/browser/chromeos/extensions/file_browser_private_apitest.cc

@@ -28,6 +28,7 @@ struct TestDiskInfo {
   const char* file_path;
   const char* device_label;
   const char* drive_label;
+  const char* fs_uuid;
   const char* system_path_prefix;
   chromeos::DeviceType device_type;
   uint64 size_in_bytes;
@@ -54,6 +55,7 @@ TestDiskInfo kTestDisks[] = {
     "file_path1",
     "device_label1",
     "drive_label1",
+    "FFFF-FFFF",
     "system_path_prefix1",
     chromeos::DEVICE_TYPE_USB,
     1073741824,
@@ -68,6 +70,7 @@ TestDiskInfo kTestDisks[] = {
     "file_path2",
     "device_label2",
     "drive_label2",
+    "0FFF-FFFF",
     "system_path_prefix2",
     chromeos::DEVICE_TYPE_MOBILE,
     47723,
@@ -82,6 +85,7 @@ TestDiskInfo kTestDisks[] = {
     "file_path3",
     "device_label3",
     "drive_label3",
+    "00FF-FFFF",
     "system_path_prefix3",
     chromeos::DEVICE_TYPE_OPTICAL_DISC,
     0,
@@ -190,6 +194,7 @@ class ExtensionFileBrowserPrivateApiTest : public ExtensionApiTest {
                 kTestDisks[disk_info_index].file_path,
                 kTestDisks[disk_info_index].device_label,
                 kTestDisks[disk_info_index].drive_label,
+                kTestDisks[disk_info_index].fs_uuid,
                 kTestDisks[disk_info_index].system_path_prefix,
                 kTestDisks[disk_info_index].device_type,
                 kTestDisks[disk_info_index].size_in_bytes,

chrome/browser/media_gallery/media_device_notifications_chromeos.cc

@@ -8,6 +8,7 @@
 
 #include "base/file_path.h"
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
@@ -16,10 +17,21 @@
 
 namespace chromeos {
 
+namespace {
+
+std::string GetDeviceUuid(const std::string& source_path) {
+  // Get the media device uuid if exists.
+  const disks::DiskMountManager::DiskMap& disks =
+      disks::DiskMountManager::GetInstance()->disks();
+  disks::DiskMountManager::DiskMap::const_iterator it = disks.find(source_path);
+  return it == disks.end() ? std::string() : it->second->fs_uuid();
+}
+
+}  // namespace
+
 using content::BrowserThread;
 
-MediaDeviceNotifications::MediaDeviceNotifications()
-    : current_device_id_(0) {
+MediaDeviceNotifications::MediaDeviceNotifications() {
   DCHECK(disks::DiskMountManager::GetInstance());
   disks::DiskMountManager::GetInstance()->AddObserver(this);
 }
@@ -101,7 +113,16 @@ void MediaDeviceNotifications::AddMountedPathOnUIThread(
     NOTREACHED();
     return;
   }
-  const std::string device_id_str = base::IntToString(current_device_id_++);
+
+  // Get the media device uuid if exists.
+  std::string device_id_str = GetDeviceUuid(mount_info.source_path);
+
+  // Keep track of device uuid, to see how often we receive empty uuid values.
+  UMA_HISTOGRAM_BOOLEAN("MediaDeviceNotification.device_uuid_available",
+                        !device_id_str.empty());
+  if (device_id_str.empty())
+    return;
+
   mount_map_.insert(std::make_pair(mount_info.mount_path, device_id_str));
   base::SystemMonitor::Get()->ProcessMediaDeviceAttached(
       device_id_str,

chrome/browser/media_gallery/media_device_notifications_chromeos.h

@@ -57,11 +57,6 @@ class MediaDeviceNotifications
   void AddMountedPathOnUIThread(
       const disks::DiskMountManager::MountPointInfo& mount_info);
 
-  // The lowest available device id number.
-  // Only accessed on the UI thread.
-  // TODO(thestig) Remove this and use a device UUID instead.
-  int current_device_id_;
-
   // Mapping of relevant mount points and their corresponding mount devices.
   // Only accessed on the UI thread.
   MountMap mount_map_;

chrome/browser/media_gallery/media_device_notifications_chromeos_unittest.cc

@@ -73,7 +73,12 @@ class MediaDeviceNotificationsTest : public testing::Test {
   }
 
   void MountDevice(MountError error_code,
-                   const DiskMountManager::MountPointInfo& mount_info) {
+                   const DiskMountManager::MountPointInfo& mount_info,
+                   const std::string& device_id) {
+    if (error_code == MOUNT_ERROR_NONE) {
+      disk_mount_manager_mock_->CreateDiskEntryForMountDevice(
+        mount_info, device_id);
+    }
     notifications_->MountCompleted(disks::DiskMountManager::MOUNTING,
                                    error_code,
                                    mount_info);
@@ -85,6 +90,10 @@ class MediaDeviceNotificationsTest : public testing::Test {
     notifications_->MountCompleted(disks::DiskMountManager::UNMOUNTING,
                                    error_code,
                                    mount_info);
+    if (error_code == MOUNT_ERROR_NONE) {
+      disk_mount_manager_mock_->RemoveDiskEntryForMountDevice(
+          mount_info);
+    }
     WaitForFileThread();
   }
 
@@ -146,14 +155,14 @@ TEST_F(MediaDeviceNotificationsTest, BasicAttachDetach) {
                                               mount_path1.value(),
                                               MOUNT_TYPE_DEVICE,
                                               disks::MOUNT_CONDITION_NONE);
-  const std::string kDeviceId0 = "0";
+  const std::string kDeviceId0 = "FFFF-FFFF";
   EXPECT_CALL(observer(),
               OnMediaDeviceAttached(kDeviceId0,
                                     ASCIIToUTF16(kDevice1Name),
                                     base::SystemMonitor::TYPE_PATH,
                                     mount_path1.value()))
       .InSequence(mock_sequence);
-  MountDevice(MOUNT_ERROR_NONE, mount_info);
+  MountDevice(MOUNT_ERROR_NONE, mount_info, kDeviceId0);
 
   EXPECT_CALL(observer(), OnMediaDeviceDetached(kDeviceId0))
       .InSequence(mock_sequence);
@@ -165,7 +174,7 @@ TEST_F(MediaDeviceNotificationsTest, BasicAttachDetach) {
                                                mount_path2.value(),
                                                MOUNT_TYPE_DEVICE,
                                                disks::MOUNT_CONDITION_NONE);
-  const std::string kDeviceId1 = "1";
+  const std::string kDeviceId1 = "FFF0-FFF0";
 
   EXPECT_CALL(observer(),
               OnMediaDeviceAttached(kDeviceId1,
@@ -173,7 +182,7 @@ TEST_F(MediaDeviceNotificationsTest, BasicAttachDetach) {
                                     base::SystemMonitor::TYPE_PATH,
                                     mount_path2.value()))
       .InSequence(mock_sequence);
-  MountDevice(MOUNT_ERROR_NONE, mount_info2);
+  MountDevice(MOUNT_ERROR_NONE, mount_info2, kDeviceId1);
 
   EXPECT_CALL(observer(), OnMediaDeviceDetached(kDeviceId1))
       .InSequence(mock_sequence);
@@ -184,19 +193,21 @@ TEST_F(MediaDeviceNotificationsTest, BasicAttachDetach) {
 TEST_F(MediaDeviceNotificationsTest, DCIM) {
   testing::Sequence mock_sequence;
   FilePath mount_path = CreateMountPoint(kMountPointA, false);
+  const std::string kDeviceId = "FFFF-FFFF";
   ASSERT_FALSE(mount_path.empty());
   DiskMountManager::MountPointInfo mount_info(kDevice1,
                                               mount_path.value(),
                                               MOUNT_TYPE_DEVICE,
                                               disks::MOUNT_CONDITION_NONE);
   EXPECT_CALL(observer(), OnMediaDeviceAttached(_, _, _, _)).Times(0);
-  MountDevice(MOUNT_ERROR_NONE, mount_info);
+  MountDevice(MOUNT_ERROR_NONE, mount_info, kDeviceId);
 }
 
 // Non device mounts and mount errors are ignored.
 TEST_F(MediaDeviceNotificationsTest, Ignore) {
   testing::Sequence mock_sequence;
   FilePath mount_path = CreateMountPoint(kMountPointA, true);
+  const std::string kDeviceId = "FFFF-FFFF";
   ASSERT_FALSE(mount_path.empty());
 
   // Mount error.
@@ -205,18 +216,18 @@ TEST_F(MediaDeviceNotificationsTest, Ignore) {
                                               MOUNT_TYPE_DEVICE,
                                               disks::MOUNT_CONDITION_NONE);
   EXPECT_CALL(observer(), OnMediaDeviceAttached(_, _, _, _)).Times(0);
-  MountDevice(MOUNT_ERROR_UNKNOWN, mount_info);
+  MountDevice(MOUNT_ERROR_UNKNOWN, mount_info, kDeviceId);
 
   // Not a device
   mount_info.mount_type = MOUNT_TYPE_ARCHIVE;
   EXPECT_CALL(observer(), OnMediaDeviceAttached(_, _, _, _)).Times(0);
-  MountDevice(MOUNT_ERROR_NONE, mount_info);
+  MountDevice(MOUNT_ERROR_NONE, mount_info, kDeviceId);
 
   // Unsupported file system.
   mount_info.mount_type = MOUNT_TYPE_DEVICE;
   mount_info.mount_condition = disks::MOUNT_CONDITION_UNSUPPORTED_FILESYSTEM;
   EXPECT_CALL(observer(), OnMediaDeviceAttached(_, _, _, _)).Times(0);
-  MountDevice(MOUNT_ERROR_NONE, mount_info);
+  MountDevice(MOUNT_ERROR_NONE, mount_info, kDeviceId);
 }
 
 }  // namespace

chromeos/dbus/cros_disks_client.cc

@@ -546,6 +546,7 @@ void DiskInfo::InitializeFromResponse(dbus::Response* response) {
   MaybePopString(properties[cros_disks::kDeviceFile], &file_path_);
   MaybePopString(properties[cros_disks::kDriveModel], &drive_model_);
   MaybePopString(properties[cros_disks::kIdLabel], &label_);
+  MaybePopString(properties[cros_disks::kIdUuid], &uuid_);
   MaybePopUint64(properties[cros_disks::kDeviceSize], &total_size_in_bytes_);
 
   uint32 media_type_uint32 = 0;

chromeos/dbus/cros_disks_client.h

@@ -110,6 +110,9 @@ class DiskInfo {
   // Returns true if the device should be hidden from the file browser.
   bool is_hidden() const { return is_hidden_; }
 
+  // Returns file system uuid.
+  std::string uuid() const { return uuid_; }
+
  private:
   void InitializeFromResponse(dbus::Response* response);
 
@@ -127,6 +130,7 @@ class DiskInfo {
   uint64 total_size_in_bytes_;
   bool is_read_only_;
   bool is_hidden_;
+  std::string uuid_;
 };
 
 // A class to make the actual DBus calls for cros-disks service.