[FS] Two minor permission changes.

1) Adds a UserAction for drag&drop so read access can be auto-granted
for directories when dropping. Without this websites would have to
explicitly request access after dropping a directory.

2) Adds a CanRequestReadPermission method that lets us block file
dialogs if read permission would be denied anyway. Currently read access
can't be denied yet, but enterprise policy might change that, so we
should at least deal with it properly.

Bug: 1053363, 1080811
Change-Id: I2a03c6d1c7d804de2eadc84a7aa13a0515d3d5df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2353193Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Auto-Submit: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797715}

chrome/browser/native_file_system/chrome_native_file_system_permission_context.cc

@@ -336,6 +336,12 @@ ChromeNativeFileSystemPermissionContext::GetReadGuardContentSetting(
       /*provider_id=*/std::string());
 }
 
+bool ChromeNativeFileSystemPermissionContext::CanObtainReadPermission(
+    const url::Origin& origin) {
+  return GetReadGuardContentSetting(origin) == CONTENT_SETTING_ASK ||
+         GetReadGuardContentSetting(origin) == CONTENT_SETTING_ALLOW;
+}
+
 bool ChromeNativeFileSystemPermissionContext::CanObtainWritePermission(
     const url::Origin& origin) {
   return GetWriteGuardContentSetting(origin) == CONTENT_SETTING_ASK ||

chrome/browser/native_file_system/chrome_native_file_system_permission_context.h

@@ -56,6 +56,7 @@ class ChromeNativeFileSystemPermissionContext
       std::unique_ptr<content::NativeFileSystemWriteItem> item,
       content::GlobalFrameRoutingId frame_id,
       base::OnceCallback<void(AfterWriteCheckResult)> callback) override;
+  bool CanObtainReadPermission(const url::Origin& origin) override;
   bool CanObtainWritePermission(const url::Origin& origin) override;
 
   ContentSetting GetReadGuardContentSetting(const url::Origin& origin);

chrome/browser/native_file_system/origin_scoped_native_file_system_permission_context.cc

@@ -331,12 +331,20 @@ OriginScopedNativeFileSystemPermissionContext::GetReadPermissionGrant(
       existing_grant->SetStatus(PermissionStatus::GRANTED);
       break;
     case CONTENT_SETTING_ASK:
-      // Files automatically get read access when picked by the user,
-      // directories need to first be confirmed.
-      if (user_action != UserAction::kLoadFromStorage &&
-          handle_type == HandleType::kFile) {
-        existing_grant->SetStatus(PermissionStatus::GRANTED);
-        ScheduleUsageIconUpdate();
+      switch (user_action) {
+        case UserAction::kOpen:
+        case UserAction::kSave:
+          // Open and Save dialog only grant read access for individual files.
+          if (handle_type == HandleType::kDirectory)
+            break;
+          FALLTHROUGH;
+        case UserAction::kDragAndDrop:
+          // Drag&drop grants read access for all handles.
+          existing_grant->SetStatus(PermissionStatus::GRANTED);
+          ScheduleUsageIconUpdate();
+          break;
+        case UserAction::kLoadFromStorage:
+          break;
       }
       break;
     case CONTENT_SETTING_BLOCK:
@@ -391,9 +399,16 @@ OriginScopedNativeFileSystemPermissionContext::GetWritePermissionGrant(
       existing_grant->SetStatus(PermissionStatus::GRANTED);
       break;
     case CONTENT_SETTING_ASK:
-      if (user_action == UserAction::kSave) {
-        existing_grant->SetStatus(PermissionStatus::GRANTED);
-        ScheduleUsageIconUpdate();
+      switch (user_action) {
+        case UserAction::kSave:
+          // Only automatically grant write access for save dialogs.
+          existing_grant->SetStatus(PermissionStatus::GRANTED);
+          ScheduleUsageIconUpdate();
+          break;
+        case UserAction::kOpen:
+        case UserAction::kDragAndDrop:
+        case UserAction::kLoadFromStorage:
+          break;
       }
       break;
     case CONTENT_SETTING_BLOCK:

content/browser/native_file_system/file_system_chooser_browsertest.cc

@@ -179,6 +179,34 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest, FullscreenOpenFile) {
   EXPECT_FALSE(IsFullscreen());
 }
 
+IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
+                       OpenFile_BlockedPermission) {
+  const base::FilePath test_file = CreateTestFile("Save File");
+  SelectFileDialogParams dialog_params;
+  ui::SelectFileDialog::SetFactory(
+      new FakeSelectFileDialogFactory({test_file}, &dialog_params));
+
+  testing::StrictMock<MockNativeFileSystemPermissionContext> permission_context;
+  static_cast<NativeFileSystemManagerImpl*>(
+      BrowserContext::GetStoragePartition(
+          shell()->web_contents()->GetBrowserContext(),
+          shell()->web_contents()->GetSiteInstance())
+          ->GetNativeFileSystemEntryFactory())
+      ->SetPermissionContextForTesting(&permission_context);
+
+  EXPECT_CALL(permission_context,
+              CanObtainReadPermission(url::Origin::Create(
+                  embedded_test_server()->GetURL("/title1.html"))))
+      .WillOnce(testing::Return(false));
+
+  ASSERT_TRUE(
+      NavigateToURL(shell(), embedded_test_server()->GetURL("/title1.html")));
+  auto result = EvalJs(shell(), "self.showOpenFilePicker()");
+  EXPECT_TRUE(result.error.find("not allowed") != std::string::npos)
+      << result.error;
+  EXPECT_EQ(ui::SelectFileDialog::SELECT_NONE, dialog_params.type);
+}
+
 IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest, SaveFile_NonExistingFile) {
   const std::string file_contents = "file contents to write";
   const base::FilePath test_file = CreateTestFile("");
@@ -253,6 +281,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
           ->GetNativeFileSystemEntryFactory())
       ->SetPermissionContextForTesting(&permission_context);
 
+  EXPECT_CALL(permission_context,
+              CanObtainReadPermission(url::Origin::Create(
+                  embedded_test_server()->GetURL("/title1.html"))))
+      .WillOnce(testing::Return(true));
   EXPECT_CALL(permission_context,
               CanObtainWritePermission(url::Origin::Create(
                   embedded_test_server()->GetURL("/title1.html"))))
@@ -355,6 +387,34 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest, FullscreenOpenDirectory) {
   EXPECT_FALSE(IsFullscreen());
 }
 
+IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
+                       OpenDirectory_BlockedPermission) {
+  base::FilePath test_dir = CreateTestDir();
+  SelectFileDialogParams dialog_params;
+  ui::SelectFileDialog::SetFactory(
+      new FakeSelectFileDialogFactory({test_dir}, &dialog_params));
+
+  testing::StrictMock<MockNativeFileSystemPermissionContext> permission_context;
+  static_cast<NativeFileSystemManagerImpl*>(
+      BrowserContext::GetStoragePartition(
+          shell()->web_contents()->GetBrowserContext(),
+          shell()->web_contents()->GetSiteInstance())
+          ->GetNativeFileSystemEntryFactory())
+      ->SetPermissionContextForTesting(&permission_context);
+
+  EXPECT_CALL(permission_context,
+              CanObtainReadPermission(url::Origin::Create(
+                  embedded_test_server()->GetURL("/title1.html"))))
+      .WillOnce(testing::Return(false));
+
+  ASSERT_TRUE(
+      NavigateToURL(shell(), embedded_test_server()->GetURL("/title1.html")));
+  auto result = EvalJs(shell(), "self.showDirectoryPicker()");
+  EXPECT_TRUE(result.error.find("not allowed") != std::string::npos)
+      << result.error;
+  EXPECT_EQ(ui::SelectFileDialog::SELECT_NONE, dialog_params.type);
+}
+
 IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest, OpenDirectory_DenyAccess) {
   base::FilePath test_dir = CreateTestDir();
   SelectFileDialogParams dialog_params;
@@ -374,6 +434,11 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest, OpenDirectory_DenyAccess) {
   auto write_grant = base::MakeRefCounted<FixedNativeFileSystemPermissionGrant>(
       PermissionStatus::ASK);
 
+  EXPECT_CALL(permission_context,
+              CanObtainReadPermission(url::Origin::Create(
+                  embedded_test_server()->GetURL("/title1.html"))))
+      .WillOnce(testing::Return(true));
+
   EXPECT_CALL(permission_context,
               ConfirmSensitiveDirectoryAccess_(
                   testing::_, testing::_, testing::_, testing::_, testing::_))
@@ -436,6 +501,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
                   testing::_, testing::_, testing::_, testing::_, testing::_))
       .WillOnce(RunOnceCallback<4>(SensitiveDirectoryResult::kAbort));
 
+  EXPECT_CALL(permission_context,
+              CanObtainReadPermission(url::Origin::Create(
+                  embedded_test_server()->GetURL("/title1.html"))))
+      .WillOnce(testing::Return(true));
   EXPECT_CALL(permission_context,
               CanObtainWritePermission(url::Origin::Create(
                   embedded_test_server()->GetURL("/title1.html"))))
@@ -483,6 +552,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
                   testing::_, testing::_, testing::_, testing::_, testing::_))
       .WillOnce(RunOnceCallback<4>(SensitiveDirectoryResult::kAbort));
 
+  EXPECT_CALL(permission_context,
+              CanObtainReadPermission(url::Origin::Create(
+                  embedded_test_server()->GetURL("/title1.html"))))
+      .WillOnce(testing::Return(true));
   EXPECT_CALL(permission_context,
               CanObtainWritePermission(url::Origin::Create(
                   embedded_test_server()->GetURL("/title1.html"))))

content/browser/native_file_system/mock_native_file_system_permission_context.h

@@ -53,6 +53,7 @@ class MockNativeFileSystemPermissionContext
                     GlobalFrameRoutingId frame_id,
                     base::OnceCallback<void(AfterWriteCheckResult)>& callback));
 
+  MOCK_METHOD1(CanObtainReadPermission, bool(const url::Origin& origin));
   MOCK_METHOD1(CanObtainWritePermission, bool(const url::Origin& origin));
 };
 

content/browser/native_file_system/native_file_system_manager_impl.cc

@@ -246,16 +246,17 @@ void NativeFileSystemManagerImpl::ChooseEntries(
     return;
   }
 
-  // When site setting is block, it's better not to show file chooser for save.
-  if (type == blink::mojom::ChooseFileSystemEntryType::kSaveFile &&
-      permission_context_ &&
-      !permission_context_->CanObtainWritePermission(context.origin)) {
-    std::move(callback).Run(
-        native_file_system_error::FromStatus(
-            NativeFileSystemStatus::kPermissionDenied),
-        std::vector<blink::mojom::NativeFileSystemEntryPtr>());
-
-    return;
+  if (permission_context_) {
+    // When site setting is block, it's better not to show file chooser.
+    if (!permission_context_->CanObtainReadPermission(context.origin) ||
+        (type == blink::mojom::ChooseFileSystemEntryType::kSaveFile &&
+         !permission_context_->CanObtainWritePermission(context.origin))) {
+      std::move(callback).Run(
+          native_file_system_error::FromStatus(
+              NativeFileSystemStatus::kPermissionDenied),
+          std::vector<blink::mojom::NativeFileSystemEntryPtr>());
+      return;
+    }
   }
 
   RenderFrameHost* rfh = RenderFrameHost::FromID(context.frame_id);

content/public/browser/native_file_system_permission_context.h

@@ -24,8 +24,9 @@ class NativeFileSystemPermissionContext {
   // use to automatically grant write access to the path.
   enum class UserAction {
     // The path for which a permission grant is requested was the result of a
-    // "open" dialog, and as such the grant should probably not start out as
-    // granted.
+    // "open" dialog. As such, only read access to files should be automatically
+    // granted, but read access to directories as well as write access to files
+    // or directories should not be granted without needing to request it.
     kOpen,
     // The path for which a permission grant is requested was the result of a
     // "save" dialog, and as such it could make sense to return a grant that
@@ -35,6 +36,10 @@ class NativeFileSystemPermissionContext {
     // loading a handle from storage. As such the grant should not start out
     // as granted, even for read access.
     kLoadFromStorage,
+    // The path for which a permission grant is requested was the result of a
+    // drag&drop operation. Read access should start out granted, but write
+    // access will require a prompt.
+    kDragAndDrop,
   };
 
   // This enum helps distinguish between file or directory Native File System
@@ -87,6 +92,11 @@ class NativeFileSystemPermissionContext {
       GlobalFrameRoutingId frame_id,
       base::OnceCallback<void(AfterWriteCheckResult)> callback) = 0;
 
+  // Returns whether the give |origin| already allows read permission, or it is
+  // possible to request one. This is used to block file dialogs from being
+  // shown if permission won't be granted anyway.
+  virtual bool CanObtainReadPermission(const url::Origin& origin) = 0;
+
   // Returns whether the give |origin| already allows write permission, or it is
   // possible to request one. This is used to block save file dialogs from being
   // shown if there is no need to ask for it.