[Native File System] Don't allow "save file" dialogs when site setting is block

Currently a save dialog will return in a read-only file handle
if the site is configured to not be allowed to prompt for
write access. Modify the logic before triggering
the file chooser to reject this attempt

Bug: 990988
Change-Id: I0a63a64ce4605fed6c83c67df60027b09a9b3329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1753986
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#693813}

chrome/browser/native_file_system/chrome_native_file_system_permission_context.cc

@@ -412,12 +412,10 @@ operator<(const Key& rhs) const {
 ChromeNativeFileSystemPermissionContext::WritePermissionGrantImpl::
     WritePermissionGrantImpl(
         scoped_refptr<ChromeNativeFileSystemPermissionContext> context,
-        ContentSettingsType content_settings_guard_type,
         const url::Origin& origin,
         const Key& key,
         bool is_directory)
     : context_(std::move(context)),
-      write_guard_content_setting_type_(content_settings_guard_type),
       origin_(origin),
       key_(key),
       is_directory_(is_directory) {}
@@ -463,13 +461,7 @@ void ChromeNativeFileSystemPermissionContext::WritePermissionGrantImpl::
 
 bool ChromeNativeFileSystemPermissionContext::WritePermissionGrantImpl::
     CanRequestPermission() {
-  HostContentSettingsMap* content_settings = context_->content_settings();
-  ContentSetting content_setting = content_settings->GetContentSetting(
-      origin_.GetURL(), origin_.GetURL(), write_guard_content_setting_type_,
-      /*provider_id=*/std::string());
-  DCHECK(content_setting == CONTENT_SETTING_ASK ||
-         content_setting == CONTENT_SETTING_BLOCK);
-  return content_setting == CONTENT_SETTING_ASK;
+  return context_->CanRequestWritePermission(origin_);
 }
 
 void ChromeNativeFileSystemPermissionContext::WritePermissionGrantImpl::
@@ -570,8 +562,14 @@ ChromeNativeFileSystemPermissionContext::GetReadPermissionGrant(
 }
 
 bool ChromeNativeFileSystemPermissionContext::CanRequestWritePermission(
-    WritePermissionGrantImpl* grant) {
-  return grant->CanRequestPermission();
+    const url::Origin& origin) {
+  ContentSetting content_setting = content_settings()->GetContentSetting(
+      origin.GetURL(), origin.GetURL(),
+      CONTENT_SETTINGS_TYPE_NATIVE_FILE_SYSTEM_WRITE_GUARD,
+      /*provider_id=*/std::string());
+  DCHECK(content_setting == CONTENT_SETTING_ASK ||
+         content_setting == CONTENT_SETTING_BLOCK);
+  return content_setting == CONTENT_SETTING_ASK;
 }
 
 scoped_refptr<content::NativeFileSystemPermissionGrant>
@@ -605,8 +603,7 @@ ChromeNativeFileSystemPermissionContext::GetWritePermissionGrant(
   // status, and store a reference to it in |origin_state| by assigning
   // |existing_grant|.
   auto result = base::MakeRefCounted<WritePermissionGrantImpl>(
-      this, CONTENT_SETTINGS_TYPE_NATIVE_FILE_SYSTEM_WRITE_GUARD, origin,
-      grant_key, is_directory);
+      this, origin, grant_key, is_directory);
   if (result->CanRequestPermission()) {
     if (user_action == UserAction::kSave) {
       result->SetStatus(WritePermissionGrantImpl::PermissionStatus::GRANTED);

chrome/browser/native_file_system/chrome_native_file_system_permission_context.h

@@ -66,7 +66,6 @@ class ChromeNativeFileSystemPermissionContext
 
     WritePermissionGrantImpl(
         scoped_refptr<ChromeNativeFileSystemPermissionContext> context,
-        ContentSettingsType content_settings_guard_type,
         const url::Origin& origin,
         const Key& key,
         bool is_directory);
@@ -115,7 +114,6 @@ class ChromeNativeFileSystemPermissionContext
     SEQUENCE_CHECKER(sequence_checker_);
 
     scoped_refptr<ChromeNativeFileSystemPermissionContext> const context_;
-    const ContentSettingsType write_guard_content_setting_type_;
     const url::Origin origin_;
     const Key key_;
     const bool is_directory_;
@@ -135,7 +133,7 @@ class ChromeNativeFileSystemPermissionContext
                          int process_id,
                          int frame_id) override;
 
-  bool CanRequestWritePermission(WritePermissionGrantImpl* grant);
+  bool CanRequestWritePermission(const url::Origin& origin) override;
   scoped_refptr<content::NativeFileSystemPermissionGrant>
   GetWritePermissionGrant(const url::Origin& origin,
                           const base::FilePath& path,

chrome/browser/native_file_system/chrome_native_file_system_permission_context_unittest.cc

@@ -106,7 +106,7 @@ class ChromeNativeFileSystemPermissionContextTest : public testing::Test {
     auto* grant = static_cast<
         ChromeNativeFileSystemPermissionContext::WritePermissionGrantImpl*>(
         actual_grant);
-    EXPECT_EQ(expected, permission_context_->CanRequestWritePermission(grant));
+    EXPECT_EQ(expected, grant->CanRequestPermission());
   }
 
  protected:
@@ -573,4 +573,19 @@ TEST_F(ChromeNativeFileSystemPermissionContextTest,
   EXPECT_EQ(PermissionStatus::DENIED, grant2->GetStatus());
 }
 
+TEST_F(ChromeNativeFileSystemPermissionContextTest,
+       CanRequestWritePermission_Allowed) {
+  bool expected = permission_context()->CanRequestWritePermission(kTestOrigin);
+  EXPECT_EQ(true, expected);
+}
+
+TEST_F(ChromeNativeFileSystemPermissionContextTest,
+       CanRequestWritePermission_ContentSettingsBlock) {
+  SetDefaultContentSettingValue(
+      CONTENT_SETTINGS_TYPE_NATIVE_FILE_SYSTEM_WRITE_GUARD,
+      CONTENT_SETTING_BLOCK);
+  bool expected = permission_context()->CanRequestWritePermission(kTestOrigin);
+  EXPECT_EQ(false, expected);
+}
+
 #endif  // !defined(OS_ANDROID)

content/browser/native_file_system/file_system_chooser_browsertest.cc

@@ -174,6 +174,35 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
                    "return await file.text(); })()"));
 }
 
+IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
+                       SaveFile_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,
+              CanRequestWritePermission(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.chooseFileSystemEntries({type: 'saveFile'})");
+  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, OpenMultipleFiles) {
   const base::FilePath test_file1 = CreateTestFile("file1");
   const base::FilePath test_file2 = CreateTestFile("file2");
@@ -267,6 +296,11 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
                                       testing::_, testing::_, testing::_))
       .WillOnce(RunOnceCallback<5>(SensitiveDirectoryResult::kAbort));
 
+  EXPECT_CALL(permission_context,
+              CanRequestWritePermission(url::Origin::Create(
+                  embedded_test_server()->GetURL("/title1.html"))))
+      .WillOnce(testing::Return(true));
+
   ASSERT_TRUE(
       NavigateToURL(shell(), embedded_test_server()->GetURL("/title1.html")));
   auto result =
@@ -310,6 +344,11 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
                                       testing::_, testing::_, testing::_))
       .WillOnce(RunOnceCallback<5>(SensitiveDirectoryResult::kAbort));
 
+  EXPECT_CALL(permission_context,
+              CanRequestWritePermission(url::Origin::Create(
+                  embedded_test_server()->GetURL("/title1.html"))))
+      .WillOnce(testing::Return(true));
+
   ASSERT_TRUE(
       NavigateToURL(shell(), embedded_test_server()->GetURL("/title1.html")));
   auto result =

content/browser/native_file_system/mock_native_file_system_permission_context.h

@@ -72,6 +72,8 @@ class MockNativeFileSystemPermissionContext
                     int process_id,
                     int frame_id,
                     base::OnceCallback<void(SafeBrowsingResult)>& callback));
+
+  MOCK_METHOD1(CanRequestWritePermission, bool(const url::Origin& origin));
 };
 
 }  // namespace content

content/browser/native_file_system/native_file_system_manager_impl.cc

@@ -204,6 +204,18 @@ 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_->CanRequestWritePermission(context.origin)) {
+    std::move(callback).Run(
+        native_file_system_error::FromStatus(
+            NativeFileSystemStatus::kPermissionDenied),
+        std::vector<blink::mojom::NativeFileSystemEntryPtr>());
+
+    return;
+  }
+
   FileSystemChooser::Options options(type, std::move(accepts),
                                      include_accepts_all);
   base::PostTask(

content/public/browser/native_file_system_permission_context.h

@@ -103,6 +103,11 @@ class NativeFileSystemPermissionContext {
       int frame_id,
       base::OnceCallback<void(SafeBrowsingResult)> callback) = 0;
 
+  // Returns whether the given |origin| is allowed to ask for write access.
+  // This is used to block save file dialogs from being shown
+  // if an origin isn't allowed to ask for write access.
+  virtual bool CanRequestWritePermission(const url::Origin& origin) = 0;
+
  protected:
   virtual ~NativeFileSystemPermissionContext() = default;
 };