[Native File System] Set user activation when file picker succeeds

Sets a new user gesture when the user successfully picks a file or
directory.

This allows developers to request write permissions immediately
following the user making a choice.

Will not activate if the user canceled.

Bug: 1014171
Change-Id: Ie7847b67316244348bf1021bac0ca058a8521dd3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1872776
Commit-Queue: Olivier Yiptong <oyiptong@chromium.org>
Reviewed-by: Mustaq Ahmed <mustaq@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709704}

third_party/blink/renderer/modules/BUILD.gn

@@ -457,6 +457,7 @@ jumbo_source_set("unit_tests") {
     "//third_party/blink/renderer/core",
     "//third_party/blink/renderer/modules/gamepad:unit_tests",
     "//third_party/blink/renderer/modules/hid:unit_tests",
+    "//third_party/blink/renderer/modules/native_file_system:unit_tests",
     "//third_party/blink/renderer/modules/storage:unit_tests",
     "//third_party/blink/renderer/modules/webtransport:unit_tests",
     "//third_party/blink/renderer/platform",

third_party/blink/renderer/modules/native_file_system/BUILD.gn

@@ -26,3 +26,23 @@ blink_modules_sources("native_file_system") {
     "//third_party/blink/renderer/platform",
   ]
 }
+
+jumbo_source_set("unit_tests") {
+  testonly = true
+  sources = [
+    "window_native_file_system_test.cc",
+  ]
+
+  configs += [
+    "//third_party/blink/renderer:config",
+    "//third_party/blink/renderer:inside_blink",
+    "//third_party/blink/renderer/core:blink_core_pch",
+  ]
+
+  deps = [
+    "//testing/gtest",
+    "//third_party/blink/renderer/modules",
+    "//third_party/blink/renderer/platform",
+    "//third_party/blink/renderer/platform/wtf",
+  ]
+}

third_party/blink/renderer/modules/native_file_system/window_native_file_system.cc

@@ -12,8 +12,10 @@
 #include "third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h"
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/dom/dom_exception.h"
+#include "third_party/blink/renderer/core/dom/user_gesture_indicator.h"
 #include "third_party/blink/renderer/core/fileapi/file_error.h"
 #include "third_party/blink/renderer/core/frame/local_dom_window.h"
+#include "third_party/blink/renderer/core/frame/local_frame.h"
 #include "third_party/blink/renderer/modules/native_file_system/choose_file_system_entries_options.h"
 #include "third_party/blink/renderer/modules/native_file_system/choose_file_system_entries_options_accepts.h"
 #include "third_party/blink/renderer/modules/native_file_system/native_file_system_directory_handle.h"
@@ -75,7 +77,8 @@ ScriptPromise WindowNativeFileSystem::chooseFileSystemEntries(
         MakeGarbageCollected<DOMException>(DOMExceptionCode::kAbortError));
   }
 
-  if (!window.GetFrame() || window.GetFrame()->IsCrossOriginSubframe()) {
+  LocalFrame* local_frame = window.GetFrame();
+  if (!local_frame || local_frame->IsCrossOriginSubframe()) {
     return ScriptPromise::RejectWithDOMException(
         script_state,
         MakeGarbageCollected<DOMException>(
@@ -83,7 +86,7 @@ ScriptPromise WindowNativeFileSystem::chooseFileSystemEntries(
             "Cross origin sub frames aren't allowed to show a file picker."));
   }
 
-  if (!LocalFrame::HasTransientUserActivation(window.GetFrame())) {
+  if (!LocalFrame::HasTransientUserActivation(local_frame)) {
     return ScriptPromise::RejectWithDOMException(
         script_state,
         MakeGarbageCollected<DOMException>(
@@ -119,6 +122,7 @@ ScriptPromise WindowNativeFileSystem::chooseFileSystemEntries(
           [](ScriptPromiseResolver* resolver,
              mojo::Remote<mojom::blink::NativeFileSystemManager>,
              const ChooseFileSystemEntriesOptions* options,
+             LocalFrame* local_frame,
              mojom::blink::NativeFileSystemErrorPtr file_operation_result,
              Vector<mojom::blink::NativeFileSystemEntryPtr> entries) {
             ExecutionContext* context = resolver->GetExecutionContext();
@@ -130,6 +134,17 @@ ScriptPromise WindowNativeFileSystem::chooseFileSystemEntries(
                                                *file_operation_result);
               return;
             }
+
+            // While it would be better to not trust the renderer process,
+            // we're doing this here to avoid potential mojo message pipe
+            // ordering problems, where the frame activation state
+            // reconciliation messages would compete with concurrent Native File
+            // System messages to the browser.
+            // TODO(https://crbug.com/1017270): Remove this after spec change,
+            // or when activation moves to browser.
+            LocalFrame::NotifyUserActivation(local_frame,
+                                             UserGestureToken::kNewGesture);
+
             if (options->multiple()) {
               HeapVector<Member<NativeFileSystemHandle>> results;
               results.ReserveInitialCapacity(entries.size());
@@ -144,8 +159,8 @@ ScriptPromise WindowNativeFileSystem::chooseFileSystemEntries(
                   std::move(entries[0]), context));
             }
           },
-          WrapPersistent(resolver), std::move(manager),
-          WrapPersistent(options)));
+          WrapPersistent(resolver), std::move(manager), WrapPersistent(options),
+          WrapPersistent(local_frame)));
   return resolver_result;
 }
 

third_party/blink/renderer/modules/native_file_system/window_native_file_system_test.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/modules/native_file_system/window_native_file_system.h"
+
+#include "mojo/public/cpp/bindings/pending_receiver.h"
+#include "mojo/public/cpp/bindings/receiver_set.h"
+#include "services/service_manager/public/cpp/interface_provider.h"
+#include "third_party/blink/public/common/browser_interface_broker_proxy.h"
+#include "third_party/blink/public/mojom/native_file_system/native_file_system_directory_handle.mojom-blink.h"
+#include "third_party/blink/public/mojom/native_file_system/native_file_system_error.mojom-blink.h"
+#include "third_party/blink/public/mojom/native_file_system/native_file_system_file_handle.mojom-blink.h"
+#include "third_party/blink/public/mojom/native_file_system/native_file_system_manager.mojom-blink.h"
+#include "third_party/blink/renderer/bindings/core/v8/script_controller.h"
+#include "third_party/blink/renderer/core/dom/user_gesture_indicator.h"
+#include "third_party/blink/renderer/core/frame/local_frame.h"
+#include "third_party/blink/renderer/core/frame/settings.h"
+#include "third_party/blink/renderer/core/html/forms/html_button_element.h"
+#include "third_party/blink/renderer/core/html/html_element.h"
+#include "third_party/blink/renderer/core/testing/page_test_base.h"
+#include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
+
+namespace blink {
+
+class MockNativeFileSystemManager
+    : public mojom::blink::NativeFileSystemManager {
+ public:
+  MockNativeFileSystemManager(service_manager::InterfaceProvider* provider,
+                              base::OnceClosure reached_callback)
+      : reached_callback_(std::move(reached_callback)), provider_(provider) {
+    service_manager::InterfaceProvider::TestApi(provider_).SetBinderForName(
+        mojom::blink::NativeFileSystemManager::Name_,
+        WTF::BindRepeating(
+            &MockNativeFileSystemManager::BindNativeFileSystemManager,
+            WTF::Unretained(this)));
+  }
+  MockNativeFileSystemManager(service_manager::InterfaceProvider* provider)
+      : provider_(provider) {
+    service_manager::InterfaceProvider::TestApi(provider_).SetBinderForName(
+        mojom::blink::NativeFileSystemManager::Name_,
+        WTF::BindRepeating(
+            &MockNativeFileSystemManager::BindNativeFileSystemManager,
+            WTF::Unretained(this)));
+  }
+  ~MockNativeFileSystemManager() override {
+    service_manager::InterfaceProvider::TestApi(provider_).SetBinderForName(
+        mojom::blink::NativeFileSystemManager::Name_, {});
+  }
+
+  using ChooseEntriesResponseCallback =
+      base::OnceCallback<void(ChooseEntriesCallback callback)>;
+
+  void SetQuitClosure(base::OnceClosure reached_callback) {
+    reached_callback_ = std::move(reached_callback);
+  }
+
+  // Unused for these tests.
+  void GetSandboxedFileSystem(
+      GetSandboxedFileSystemCallback callback) override {}
+
+  void ChooseEntries(
+      mojom::ChooseFileSystemEntryType type,
+      WTF::Vector<mojom::blink::ChooseFileSystemEntryAcceptsOptionPtr> accepts,
+      bool include_accepts_all,
+      ChooseEntriesCallback callback) override {
+    if (choose_entries_response_callback_) {
+      std::move(choose_entries_response_callback_).Run(std::move(callback));
+    }
+
+    if (reached_callback_)
+      std::move(reached_callback_).Run();
+  }
+
+  void SetChooseEntriesResponse(ChooseEntriesResponseCallback callback) {
+    choose_entries_response_callback_ = std::move(callback);
+  }
+
+  void GetFileHandleFromToken(
+      mojo::PendingRemote<mojom::blink::NativeFileSystemTransferToken>,
+      mojo::PendingReceiver<mojom::blink::NativeFileSystemFileHandle>)
+      override {}
+
+  void GetDirectoryHandleFromToken(
+      mojo::PendingRemote<mojom::blink::NativeFileSystemTransferToken>,
+      mojo::PendingReceiver<mojom::blink::NativeFileSystemDirectoryHandle>)
+      override {}
+
+ private:
+  void BindNativeFileSystemManager(mojo::ScopedMessagePipeHandle handle) {
+    receivers_.Add(this,
+                   mojo::PendingReceiver<mojom::blink::NativeFileSystemManager>(
+                       std::move(handle)));
+  }
+
+  base::OnceClosure reached_callback_;
+  ChooseEntriesResponseCallback choose_entries_response_callback_;
+  mojo::ReceiverSet<mojom::blink::NativeFileSystemManager> receivers_;
+  service_manager::InterfaceProvider* provider_;
+};
+
+class WindowNativeFileSystemTest : public PageTestBase {
+ public:
+  void SetUp() override {
+    PageTestBase::SetUp();
+    Navigate("http://localhost");
+    GetDocument().GetSettings()->SetScriptEnabled(true);
+  }
+
+  void Navigate(const String& destinationUrl) {
+    const KURL& url = KURL(NullURL(), destinationUrl);
+    auto navigation_params =
+        WebNavigationParams::CreateWithHTMLBuffer(SharedBuffer::Create(), url);
+    GetDocument().GetFrame()->Loader().CommitNavigation(
+        std::move(navigation_params), /*extra_data=*/nullptr);
+    blink::test::RunPendingTasks();
+    ASSERT_EQ(url.GetString(), GetDocument().Url().GetString());
+  }
+};
+
+TEST_F(WindowNativeFileSystemTest, UserActivationRequiredOtherwiseDenied) {
+  LocalFrame* frame = &GetFrame();
+  EXPECT_FALSE(frame->HasBeenActivated());
+
+  MockNativeFileSystemManager manager(&frame->GetInterfaceProvider());
+  manager.SetChooseEntriesResponse(WTF::Bind(
+      [](MockNativeFileSystemManager::ChooseEntriesCallback callback) {
+        FAIL();
+      }));
+  GetFrame().GetScriptController().ExecuteScriptInMainWorld(
+      "window.chooseFileSystemEntries({type: 'openFile'});");
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(frame->HasBeenActivated());
+}
+
+TEST_F(WindowNativeFileSystemTest, UserActivationChooseEntriesSuccessful) {
+  LocalFrame* frame = &GetFrame();
+  EXPECT_FALSE(frame->HasBeenActivated());
+
+  LocalFrame::NotifyUserActivation(frame, UserGestureToken::kNewGesture);
+  EXPECT_TRUE(frame->HasBeenActivated());
+
+  base::RunLoop manager_run_loop;
+  MockNativeFileSystemManager manager(&frame->GetInterfaceProvider(),
+                                      manager_run_loop.QuitClosure());
+  manager.SetChooseEntriesResponse(WTF::Bind(
+      [](MockNativeFileSystemManager::ChooseEntriesCallback callback) {
+        auto error = mojom::blink::NativeFileSystemError::New();
+        error->status = mojom::blink::NativeFileSystemStatus::kOk;
+        error->message = "";
+
+        mojo::PendingRemote<mojom::blink::NativeFileSystemFileHandle>
+            pending_remote;
+        ignore_result(pending_remote.InitWithNewPipeAndPassReceiver());
+        auto handle = mojom::blink::NativeFileSystemHandle::NewFile(
+            std::move(pending_remote));
+        auto entry = mojom::blink::NativeFileSystemEntry::New(std::move(handle),
+                                                              "foo.txt");
+        Vector<mojom::blink::NativeFileSystemEntryPtr> entries;
+        entries.push_back(std::move(entry));
+
+        std::move(callback).Run(std::move(error), std::move(entries));
+      }));
+  GetFrame().GetScriptController().ExecuteScriptInMainWorld(
+      "window.chooseFileSystemEntries({type: 'openFile'});");
+  manager_run_loop.Run();
+
+  // Mock Manager finished sending data over the mojo pipe.
+  // Clearing the user activation.
+  frame->ClearActivation();
+  EXPECT_FALSE(frame->HasBeenActivated());
+
+  // Let blink-side receiver process the response and set the user activation
+  // again.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(frame->HasBeenActivated());
+}
+
+TEST_F(WindowNativeFileSystemTest, UserActivationChooseEntriesErrors) {
+  LocalFrame* frame = &GetFrame();
+  EXPECT_FALSE(frame->HasBeenActivated());
+
+  using mojom::blink::NativeFileSystemStatus;
+
+  NativeFileSystemStatus statuses[] = {
+      NativeFileSystemStatus::kPermissionDenied,
+      NativeFileSystemStatus::kInvalidState,
+      NativeFileSystemStatus::kInvalidArgument,
+      NativeFileSystemStatus::kOperationFailed,
+      // kOperationAborted is when the user cancels the file selection.
+      NativeFileSystemStatus::kOperationAborted,
+  };
+  MockNativeFileSystemManager manager(&frame->GetInterfaceProvider());
+
+  for (const NativeFileSystemStatus& status : statuses) {
+    LocalFrame::NotifyUserActivation(frame, UserGestureToken::kNewGesture);
+    EXPECT_TRUE(frame->HasBeenActivated());
+
+    base::RunLoop manager_run_loop;
+    manager.SetQuitClosure(manager_run_loop.QuitClosure());
+    manager.SetChooseEntriesResponse(WTF::Bind(
+        [](mojom::blink::NativeFileSystemStatus status,
+           MockNativeFileSystemManager::ChooseEntriesCallback callback) {
+          auto error = mojom::blink::NativeFileSystemError::New();
+          error->status = status;
+          error->message = "";
+          Vector<mojom::blink::NativeFileSystemEntryPtr> entries;
+
+          std::move(callback).Run(std::move(error), std::move(entries));
+        },
+        status));
+    GetFrame().GetScriptController().ExecuteScriptInMainWorld(
+        "window.chooseFileSystemEntries({type: 'openFile'});");
+    manager_run_loop.Run();
+
+    // Mock Manager finished sending data over the mojo pipe.
+    // Clearing the user activation.
+    frame->ClearActivation();
+    EXPECT_FALSE(frame->HasBeenActivated());
+
+    base::RunLoop().RunUntilIdle();
+    EXPECT_FALSE(frame->HasBeenActivated());
+  }
+}
+
+}  // namespace blink