Clean up Safari importing

Starting in macOS 10.14, access to the Safari bookmarks file was
restricted. Chromium didn't notice, and would proceed anyway,
because it assumed that if the file was there, it could be read.
That's not true in the general case anyway, so fix it.

History import from Safari broke many years ago when they moved
to a database for their storage from using a plist. No one realized
that it broke, and since it's access restricted from 10.14 on,
there's no point in fixing it, so this CL removes it.

Bug: 850225
Change-Id: I984ba22a3bcce60ed39cc4fb45331d38f6a6fa94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2404529Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806257}

base/files/file_util.h

@@ -161,6 +161,9 @@ BASE_EXPORT bool CopyDirectoryExcl(const FilePath& from_path,
 // false otherwise.
 BASE_EXPORT bool PathExists(const FilePath& path);
 
+// Returns true if the given path is readable by the user, false otherwise.
+BASE_EXPORT bool PathIsReadable(const FilePath& path);
+
 // Returns true if the given path is writable by the user, false otherwise.
 BASE_EXPORT bool PathIsWritable(const FilePath& path);
 

base/files/file_util_posix.cc

@@ -470,6 +470,13 @@ bool PathExists(const FilePath& path) {
   return access(path.value().c_str(), F_OK) == 0;
 }
 
+#if !defined(OS_NACL_NONSFI)
+bool PathIsReadable(const FilePath& path) {
+  ScopedBlockingCall scoped_blocking_call(FROM_HERE, BlockingType::MAY_BLOCK);
+  return access(path.value().c_str(), R_OK) == 0;
+}
+#endif  // !defined(OS_NACL_NONSFI)
+
 #if !defined(OS_NACL_NONSFI)
 bool PathIsWritable(const FilePath& path) {
   ScopedBlockingCall scoped_blocking_call(FROM_HERE, BlockingType::MAY_BLOCK);

base/files/file_util_unittest.cc

@@ -915,6 +915,7 @@ TEST_F(FileUtilTest, ChangeFilePermissionsAndRead) {
   FilePath file_name =
       temp_dir_.GetPath().Append(FPL("Test Readable File.txt"));
   EXPECT_FALSE(PathExists(file_name));
+  EXPECT_FALSE(PathIsReadable(file_name));
 
   static constexpr char kData[] = "hello";
   static constexpr int kDataSize = sizeof(kData) - 1;
@@ -928,11 +929,13 @@ TEST_F(FileUtilTest, ChangeFilePermissionsAndRead) {
   int32_t mode = 0;
   EXPECT_TRUE(GetPosixFilePermissions(file_name, &mode));
   EXPECT_TRUE(mode & FILE_PERMISSION_READ_BY_USER);
+  EXPECT_TRUE(PathIsReadable(file_name));
 
   // Get rid of the read permission.
   EXPECT_TRUE(SetPosixFilePermissions(file_name, 0u));
   EXPECT_TRUE(GetPosixFilePermissions(file_name, &mode));
   EXPECT_FALSE(mode & FILE_PERMISSION_READ_BY_USER);
+  EXPECT_FALSE(PathIsReadable(file_name));
   // Make sure the file can't be read.
   EXPECT_EQ(-1, ReadFile(file_name, buffer, kDataSize));
 
@@ -940,6 +943,7 @@ TEST_F(FileUtilTest, ChangeFilePermissionsAndRead) {
   EXPECT_TRUE(SetPosixFilePermissions(file_name, FILE_PERMISSION_READ_BY_USER));
   EXPECT_TRUE(GetPosixFilePermissions(file_name, &mode));
   EXPECT_TRUE(mode & FILE_PERMISSION_READ_BY_USER);
+  EXPECT_TRUE(PathIsReadable(file_name));
   // Make sure the file can be read.
   EXPECT_EQ(kDataSize, ReadFile(file_name, buffer, kDataSize));
 

base/files/file_util_win.cc

@@ -458,17 +458,39 @@ bool PathExists(const FilePath& path) {
   return (GetFileAttributes(path.value().c_str()) != INVALID_FILE_ATTRIBUTES);
 }
 
-bool PathIsWritable(const FilePath& path) {
+namespace {
+
+bool PathHasAccess(const FilePath& path,
+                   DWORD dir_desired_access,
+                   DWORD file_desired_access) {
   ScopedBlockingCall scoped_blocking_call(FROM_HERE, BlockingType::MAY_BLOCK);
-  HANDLE dir =
-      CreateFile(path.value().c_str(), FILE_ADD_FILE, kFileShareAll, NULL,
-                 OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
 
-  if (dir == INVALID_HANDLE_VALUE)
+  const wchar_t* const path_str = path.value().c_str();
+  DWORD fileattr = GetFileAttributes(path_str);
+  if (fileattr == INVALID_FILE_ATTRIBUTES)
     return false;
 
-  CloseHandle(dir);
-  return true;
+  bool is_directory = fileattr & FILE_ATTRIBUTE_DIRECTORY;
+  DWORD desired_access =
+      is_directory ? dir_desired_access : file_desired_access;
+  DWORD flags_and_attrs =
+      is_directory ? FILE_FLAG_BACKUP_SEMANTICS : FILE_ATTRIBUTE_NORMAL;
+
+  win::ScopedHandle file(CreateFile(path_str, desired_access, kFileShareAll,
+                                    nullptr, OPEN_EXISTING, flags_and_attrs,
+                                    nullptr));
+
+  return file.IsValid();
+}
+
+}  // namespace
+
+bool PathIsReadable(const FilePath& path) {
+  return PathHasAccess(path, FILE_LIST_DIRECTORY, GENERIC_READ);
+}
+
+bool PathIsWritable(const FilePath& path) {
+  return PathHasAccess(path, FILE_ADD_FILE, GENERIC_WRITE);
 }
 
 bool DirectoryExists(const FilePath& path) {

chrome/common/importer/safari_importer_utils.mm

@@ -13,17 +13,21 @@ bool SafariImporterCanImport(const base::FilePath& library_dir,
   DCHECK(services_supported);
   *services_supported = importer::NONE;
 
-  // Import features are toggled by the following:
-  // bookmarks import: existence of ~/Library/Safari/Bookmarks.plist file.
-  // history import: existence of ~/Library/Safari/History.plist file.
+  // Only support the importing of bookmarks from Safari, if there is access to
+  // the bookmarks storage file.
+
+  // As for history import, this code used to support that, dependent on the
+  // existence of the "History.plist" file. Long before macOS 10.10, Safari
+  // switched to a database for the history file, and no one noticed that the
+  // history importing broke. Given the file access restrictions in macOS 10.14
+  // (https://crbug.com/850225) it's probably not worth fixing it, and so it was
+  // removed.
+
   base::FilePath safari_dir = library_dir.Append("Safari");
   base::FilePath bookmarks_path = safari_dir.Append("Bookmarks.plist");
-  base::FilePath history_path = safari_dir.Append("History.plist");
 
-  if (base::PathExists(bookmarks_path))
+  if (base::PathIsReadable(bookmarks_path))
     *services_supported |= importer::FAVORITES;
-  if (base::PathExists(history_path))
-    *services_supported |= importer::HISTORY;
 
   return *services_supported != importer::NONE;
 }

chrome/utility/importer/importer_creator.cc

@@ -16,8 +16,6 @@
 #endif
 
 #if defined(OS_MAC)
-#include <CoreFoundation/CoreFoundation.h>
-
 #include "base/mac/foundation_util.h"
 #include "chrome/utility/importer/safari_importer.h"
 #endif

chrome/utility/importer/safari_importer.h

@@ -58,11 +58,9 @@ class SafariImporter : public Importer {
   // Multiple URLs can share the same favicon; this is a map
   // of URLs -> IconIDs that we load as a temporary step before
   // actually loading the icons.
-  typedef std::map<int64_t, std::set<GURL>> FaviconMap;
+  using FaviconMap = std::map<int64_t, std::set<GURL>>;
 
   void ImportBookmarks();
-  void ImportPasswords();
-  void ImportHistory();
 
   // Parse Safari's stored bookmarks.
   void ParseBookmarks(const base::string16& toolbar_name,
@@ -80,13 +78,6 @@ class SafariImporter : public Importer {
       const base::string16& toolbar_name,
       std::vector<ImportedBookmarkEntry>* out_bookmarks);
 
-  // Converts history time stored by Safari as a double serialized as a string,
-  // to seconds-since-UNIX-Ephoch-format used by Chrome.
-  double HistoryTimeToEpochTime(NSString* history_time);
-
-  // Parses Safari's history and loads it into the input array.
-  void ParseHistoryItems(std::vector<ImporterURLRow>* history_items);
-
   // Opens the favicon database file.
   bool OpenDatabase(sql::Database* db);
 

chrome/utility/importer/safari_importer.mm

@@ -25,22 +25,6 @@
 #include "sql/statement.h"
 #include "url/gurl.h"
 
-namespace {
-
-// A function like this is used by other importers in order to filter out
-// URLS we don't want to import.
-// For now it's pretty basic, but I've split it out so it's easy to slot
-// in necessary logic for filtering URLS, should we need it.
-bool CanImportSafariURL(const GURL& url) {
-  // The URL is not valid.
-  if (!url.is_valid())
-    return false;
-
-  return true;
-}
-
-}  // namespace
-
 SafariImporter::SafariImporter(const base::FilePath& library_dir)
     : library_dir_(library_dir) {
 }
@@ -58,21 +42,12 @@ void SafariImporter::StartImport(const importer::SourceProfile& source_profile,
   // In keeping with import on other platforms (and for other browsers), we
   // don't import the home page (since it may lead to a useless homepage); see
   // crbug.com/25603.
-  if ((items & importer::HISTORY) && !cancelled()) {
-    bridge_->NotifyItemStarted(importer::HISTORY);
-    ImportHistory();
-    bridge_->NotifyItemEnded(importer::HISTORY);
-  }
   if ((items & importer::FAVORITES) && !cancelled()) {
     bridge_->NotifyItemStarted(importer::FAVORITES);
     ImportBookmarks();
     bridge_->NotifyItemEnded(importer::FAVORITES);
   }
-  if ((items & importer::PASSWORDS) && !cancelled()) {
-    bridge_->NotifyItemStarted(importer::PASSWORDS);
-    ImportPasswords();
-    bridge_->NotifyItemEnded(importer::PASSWORDS);
-  }
+
   bridge_->NotifyEnded();
 }
 
@@ -105,7 +80,7 @@ void SafariImporter::ImportBookmarks() {
 }
 
 bool SafariImporter::OpenDatabase(sql::Database* db) {
-  // Construct ~/Library/Safari/WebIcons.db path.
+  // Construct ~/Library/Safari/WebpageIcons.db path.
   NSString* library_dir = [NSString
       stringWithUTF8String:library_dir_.value().c_str()];
   NSString* safari_dir = [library_dir
@@ -289,93 +264,3 @@ void SafariImporter::ParseBookmarks(
   RecursiveReadBookmarksFolder(bookmarks_dict, parent_path_elements, false,
                                toolbar_name, bookmarks);
 }
-
-void SafariImporter::ImportPasswords() {
-  // Safari stores it's passwords in the Keychain, same as us so we don't need
-  // to import them.
-  // Note: that we don't automatically pick them up, there is some logic around
-  // the user needing to explicitly input their username in a page and blurring
-  // the field before we pick it up, but the details of that are beyond the
-  // scope of this comment.
-}
-
-void SafariImporter::ImportHistory() {
-  std::vector<ImporterURLRow> rows;
-  ParseHistoryItems(&rows);
-
-  if (!rows.empty() && !cancelled()) {
-    bridge_->SetHistoryItems(rows, importer::VISIT_SOURCE_SAFARI_IMPORTED);
-  }
-}
-
-double SafariImporter::HistoryTimeToEpochTime(NSString* history_time) {
-  DCHECK(history_time);
-  // Add Difference between Unix epoch and CFAbsoluteTime epoch in seconds.
-  // Unix epoch is 1970-01-01 00:00:00.0 UTC,
-  // CF epoch is 2001-01-01 00:00:00.0 UTC.
-  return CFStringGetDoubleValue(base::mac::NSToCFCast(history_time)) +
-      kCFAbsoluteTimeIntervalSince1970;
-}
-
-void SafariImporter::ParseHistoryItems(
-    std::vector<ImporterURLRow>* history_items) {
-  DCHECK(history_items);
-
-  // Construct ~/Library/Safari/History.plist path
-  NSString* library_dir = [NSString
-      stringWithUTF8String:library_dir_.value().c_str()];
-  NSString* safari_dir = [library_dir
-      stringByAppendingPathComponent:@"Safari"];
-  NSString* history_plist = [safari_dir
-      stringByAppendingPathComponent:@"History.plist"];
-
-  // Load the plist file.
-  NSDictionary* history_dict = [NSDictionary
-      dictionaryWithContentsOfFile:history_plist];
-  if (!history_dict)
-    return;
-
-  NSArray* safari_history_items = [history_dict
-      objectForKey:@"WebHistoryDates"];
-
-  for (NSDictionary* history_item in safari_history_items) {
-    NSString* url_ns = [history_item objectForKey:@""];
-    if (!url_ns)
-      continue;
-
-    GURL url(base::SysNSStringToUTF8(url_ns));
-
-    if (!CanImportSafariURL(url))
-      continue;
-
-    ImporterURLRow row(url);
-    NSString* title_ns = [history_item objectForKey:@"title"];
-
-    // Sometimes items don't have a title, in which case we just substitue
-    // the url.
-    if (!title_ns)
-      title_ns = url_ns;
-
-    row.title = base::SysNSStringToUTF16(title_ns);
-    int visit_count = [[history_item objectForKey:@"visitCount"]
-                          intValue];
-    row.visit_count = visit_count;
-    // Include imported URLs in autocompletion - don't hide them.
-    row.hidden = 0;
-    // Item was never typed before in the omnibox.
-    row.typed_count = 0;
-
-    NSString* last_visit_str = [history_item objectForKey:@"lastVisitedDate"];
-    // The last visit time should always be in the history item, but if not
-    /// just continue without this item.
-    DCHECK(last_visit_str);
-    if (!last_visit_str)
-      continue;
-
-    // Convert Safari's last visit time to Unix Epoch time.
-    double seconds_since_unix_epoch = HistoryTimeToEpochTime(last_visit_str);
-    row.last_visit = base::Time::FromDoubleT(seconds_since_unix_epoch);
-
-    history_items->push_back(row);
-  }
-}

chrome/utility/importer/safari_importer_unittest.mm

@@ -54,36 +54,6 @@ class SafariImporterTest : public PlatformTest {
   }
 };
 
-TEST_F(SafariImporterTest, HistoryImport) {
-  scoped_refptr<SafariImporter> importer(GetSafariImporter());
-
-  std::vector<ImporterURLRow> history_items;
-  importer->ParseHistoryItems(&history_items);
-
-  // Should be 2 history items.
-  ASSERT_EQ(history_items.size(), 2U);
-
-  ImporterURLRow& it1 = history_items[0];
-  EXPECT_EQ(it1.url, GURL("http://www.firsthistoryitem.com/"));
-  EXPECT_EQ(it1.title, base::UTF8ToUTF16("First History Item Title"));
-  EXPECT_EQ(it1.visit_count, 1);
-  EXPECT_EQ(it1.hidden, 0);
-  EXPECT_EQ(it1.typed_count, 0);
-  EXPECT_EQ(it1.last_visit.ToDoubleT(),
-      importer->HistoryTimeToEpochTime(@"270598264.4"));
-
-  ImporterURLRow& it2 = history_items[1];
-  std::string second_item_title("http://www.secondhistoryitem.com/");
-  EXPECT_EQ(it2.url, GURL(second_item_title));
-  // The second item lacks a title so we expect the URL to be substituted.
-  EXPECT_EQ(base::UTF16ToUTF8(it2.title), second_item_title.c_str());
-  EXPECT_EQ(it2.visit_count, 55);
-  EXPECT_EQ(it2.hidden, 0);
-  EXPECT_EQ(it2.typed_count, 0);
-  EXPECT_EQ(it2.last_visit.ToDoubleT(),
-      importer->HistoryTimeToEpochTime(@"270598231.4"));
-}
-
 TEST_F(SafariImporterTest, BookmarkImport) {
   // Expected results
   const struct {
@@ -264,11 +234,7 @@ TEST_F(SafariImporterTest, CanImport) {
   uint16_t items = importer::NONE;
   EXPECT_TRUE(SafariImporterCanImport(
       GetTestSafariLibraryPath("default"), &items));
-  EXPECT_EQ(items, importer::HISTORY | importer::FAVORITES);
-  EXPECT_EQ(items & importer::COOKIES, importer::NONE);
-  EXPECT_EQ(items & importer::PASSWORDS, importer::NONE);
-  EXPECT_EQ(items & importer::SEARCH_ENGINES, importer::NONE);
-  EXPECT_EQ(items & importer::HOME_PAGE, importer::NONE);
+  EXPECT_EQ(items, importer::FAVORITES);
 
   // Check that we don't import anything from a bogus library directory.
   base::ScopedTempDir fake_library_dir;