FontAccess: Require User Activation only when showing permission prompt

Following feedback from partners, requiring UA each time
navigator.fonts.query() is invoked is cumbersome for UX.

This change ensures that User Activation is checked before showing a
permission prompt, but isn't checked if the permission status is
either granted or denied.

Bug: 1043306
Change-Id: I37a848feaf7c02e881bb4a63f53594a3636a9f96
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2388982
Commit-Queue: Olivier Yiptong <oyiptong@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803729}

content/browser/font_access/font_access_manager_impl.cc

@@ -43,9 +43,6 @@ void FontAccessManagerImpl::RequestPermission(
   const BindingContext& context = receivers_.current_context();
   RenderFrameHost* rfh = RenderFrameHost::FromID(context.frame_id);
 
-  // Double checking: renderer processes should already have checked for user
-  // activation before the RPC has been made. It is not an error, because it is
-  // possible that user activation has lapsed before reaching here.
   if (!rfh->HasTransientUserActivation()) {
     std::move(callback).Run(blink::mojom::PermissionStatus::DENIED);
     return;
@@ -74,9 +71,17 @@ void FontAccessManagerImpl::EnumerateLocalFonts(
   const BindingContext& context = receivers_.current_context();
   RenderFrameHost* rfh = RenderFrameHost::FromID(context.frame_id);
 
-  // Double checking: renderer processes should already have checked for user
-  // activation before the RPC has been made. It is not an error, because it is
-  // possible that user activation has lapsed before reaching here.
+  auto status = PermissionControllerImpl::FromBrowserContext(
+                    rfh->GetProcess()->GetBrowserContext())
+                    ->GetPermissionStatusForFrame(PermissionType::FONT_ACCESS,
+                                                  rfh, context.origin.GetURL());
+
+  if (status != blink::mojom::PermissionStatus::ASK) {
+    // Permission has been requested before.
+    DidRequestPermission(std::move(callback), std::move(status));
+    return;
+  }
+
   if (!rfh->HasTransientUserActivation()) {
     std::move(callback).Run(
         blink::mojom::FontEnumerationStatus::kPermissionDenied,

content/browser/font_access/font_access_manager_impl_unittest.cc

@@ -48,13 +48,26 @@ class TestPermissionManager : public MockPermissionManager {
     return 0;
   }
 
+  blink::mojom::PermissionStatus GetPermissionStatusForFrame(
+      PermissionType permission,
+      RenderFrameHost* render_frame_host,
+      const GURL& requesting_origin) override {
+    return permission_status_for_frame_;
+  }
+
   void SetRequestCallback(
       base::RepeatingCallback<void(PermissionCallback)> request_callback) {
     request_callback_ = std::move(request_callback);
   }
 
+  void SetPermissionStatusForFrame(blink::mojom::PermissionStatus status) {
+    permission_status_for_frame_ = status;
+  }
+
  private:
   base::RepeatingCallback<void(PermissionCallback)> request_callback_;
+  blink::mojom::PermissionStatus permission_status_for_frame_ =
+      blink::mojom::PermissionStatus::ASK;
 };
 
 }  // namespace
@@ -106,6 +119,8 @@ class FontAccessManagerImplTest : public RenderViewHostImplTestHarness {
         base::BindRepeating([](PermissionCallback callback) {
           std::move(callback).Run(blink::mojom::PermissionStatus::GRANTED);
         }));
+    test_permission_manager()->SetPermissionStatusForFrame(
+        blink::mojom::PermissionStatus::GRANTED);
   }
 
   void AutoDenyPermission() {
@@ -113,6 +128,26 @@ class FontAccessManagerImplTest : public RenderViewHostImplTestHarness {
         base::BindRepeating([](PermissionCallback callback) {
           std::move(callback).Run(blink::mojom::PermissionStatus::DENIED);
         }));
+    test_permission_manager()->SetPermissionStatusForFrame(
+        blink::mojom::PermissionStatus::DENIED);
+  }
+
+  void AskGrantPermission() {
+    test_permission_manager()->SetPermissionStatusForFrame(
+        blink::mojom::PermissionStatus::ASK);
+    test_permission_manager()->SetRequestCallback(
+        base::BindRepeating([](PermissionCallback callback) {
+          std::move(callback).Run(blink::mojom::PermissionStatus::GRANTED);
+        }));
+  }
+
+  void AskDenyPermission() {
+    test_permission_manager()->SetPermissionStatusForFrame(
+        blink::mojom::PermissionStatus::ASK);
+    test_permission_manager()->SetRequestCallback(
+        base::BindRepeating([](PermissionCallback callback) {
+          std::move(callback).Run(blink::mojom::PermissionStatus::DENIED);
+        }));
   }
 
   void SimulateUserActivation() {
@@ -136,7 +171,7 @@ class FontAccessManagerImplTest : public RenderViewHostImplTestHarness {
 #if defined(OS_MAC)
 TEST_F(FontAccessManagerImplTest, NoUserActivationPermissionDenied) {
   ASSERT_TRUE(manager_remote_.is_bound() && manager_remote_.is_connected());
-  AutoGrantPermission();
+  AskGrantPermission();
 
   base::RunLoop loop;
   bool permission_requested = false;
@@ -154,7 +189,7 @@ TEST_F(FontAccessManagerImplTest, NoUserActivationPermissionDenied) {
 
 TEST_F(FontAccessManagerImplTest, UserActivationPermissionManagerTriggered) {
   ASSERT_TRUE(manager_remote_.is_bound() && manager_remote_.is_connected());
-  AutoGrantPermission();
+  AskGrantPermission();
   SimulateUserActivation();
 
   base::RunLoop loop;
@@ -193,10 +228,9 @@ void ValidateFontEnumerationBasic(FontEnumerationStatus status,
 
 }  // namespace
 
-TEST_F(FontAccessManagerImplTest, ValidateEnumerationBasic) {
+TEST_F(FontAccessManagerImplTest, PreviouslyGrantedValidateEnumerationBasic) {
   ASSERT_TRUE(manager_remote_.is_bound() && manager_remote_.is_connected());
   AutoGrantPermission();
-  SimulateUserActivation();
 
   base::RunLoop run_loop;
   manager_remote_->EnumerateLocalFonts(
@@ -210,9 +244,25 @@ TEST_F(FontAccessManagerImplTest, ValidateEnumerationBasic) {
   run_loop.Run();
 }
 
+TEST_F(FontAccessManagerImplTest, UserActivationRequiredBeforeGrant) {
+  ASSERT_TRUE(manager_remote_.is_bound() && manager_remote_.is_connected());
+  AskGrantPermission();
+  SimulateUserActivation();
+
+  base::RunLoop run_loop;
+  manager_remote_->EnumerateLocalFonts(
+      base::BindLambdaForTesting([&](FontEnumerationStatus status,
+                                     base::ReadOnlySharedMemoryRegion region) {
+        EXPECT_EQ(status, FontEnumerationStatus::kOk)
+            << "Font Enumeration was successful.";
+        run_loop.Quit();
+      }));
+  run_loop.Run();
+}
+
 TEST_F(FontAccessManagerImplTest, EnumerationPermissionDeniedIfNoActivation) {
   ASSERT_TRUE(manager_remote_.is_bound() && manager_remote_.is_connected());
-  AutoGrantPermission();
+  AskGrantPermission();
 
   base::RunLoop run_loop;
   manager_remote_->EnumerateLocalFonts(
@@ -225,9 +275,9 @@ TEST_F(FontAccessManagerImplTest, EnumerationPermissionDeniedIfNoActivation) {
   run_loop.Run();
 }
 
-TEST_F(FontAccessManagerImplTest, PermissionDeniedErrors) {
+TEST_F(FontAccessManagerImplTest, PermissionDeniedOnAskErrors) {
   ASSERT_TRUE(manager_remote_.is_bound() && manager_remote_.is_connected());
-  AutoDenyPermission();
+  AskDenyPermission();
   SimulateUserActivation();
 
   base::RunLoop run_loop;
@@ -240,6 +290,21 @@ TEST_F(FontAccessManagerImplTest, PermissionDeniedErrors) {
       }));
   run_loop.Run();
 }
+
+TEST_F(FontAccessManagerImplTest, PermissionPreviouslyDeniedErrors) {
+  ASSERT_TRUE(manager_remote_.is_bound() && manager_remote_.is_connected());
+  AutoDenyPermission();
+
+  base::RunLoop run_loop;
+  manager_remote_->EnumerateLocalFonts(
+      base::BindLambdaForTesting([&](FontEnumerationStatus status,
+                                     base::ReadOnlySharedMemoryRegion region) {
+        EXPECT_EQ(status, FontEnumerationStatus::kPermissionDenied)
+            << "Permission was denied.";
+        run_loop.Quit();
+      }));
+  run_loop.Run();
+}
 #endif
 
 }  // namespace content

third_party/blink/renderer/modules/font_access/font_manager.cc

@@ -26,7 +26,6 @@ void ReturnDataFunction(const v8::FunctionCallbackInfo<v8::Value>& info) {
 
 ScriptValue FontManager::query(ScriptState* script_state,
                                ExceptionState& exception_state) {
-  ValidateRequest(ExecutionContext::From(script_state), exception_state);
   if (exception_state.HadException())
     return ScriptValue();
 
@@ -51,15 +50,4 @@ void FontManager::Trace(blink::Visitor* visitor) const {
   ScriptWrappable::Trace(visitor);
 }
 
-void FontManager::ValidateRequest(ExecutionContext* context,
-                                  ExceptionState& exception_state) {
-  DCHECK(context);
-  auto* frame = To<LocalDOMWindow>(context)->GetFrame();
-
-  if (!LocalFrame::HasTransientUserActivation(frame)) {
-    exception_state.ThrowSecurityError(
-        "Must be handling a user gesture to enumerate local fonts.");
-  }
-}
-
 }  // namespace blink

third_party/blink/renderer/modules/font_access/font_manager.h

@@ -14,7 +14,6 @@ namespace blink {
 
 class ScriptState;
 class ScriptValue;
-class ExecutionContext;
 
 class FontManager final : public ScriptWrappable {
   DEFINE_WRAPPERTYPEINFO();
@@ -25,9 +24,6 @@ class FontManager final : public ScriptWrappable {
 
   DISALLOW_COPY_AND_ASSIGN(FontManager);
   void Trace(blink::Visitor*) const override;
-
- private:
-  void ValidateRequest(ExecutionContext*, ExceptionState&);
 };
 
 }  // namespace blink

third_party/blink/web_tests/external/wpt/font-access/resources/window-tests-enumeration.js

 'use strict';
 
 promise_test(async t => {
-  assert_throws_dom('SecurityError', () => {
-    navigator.fonts.query();
-  });
-}, 'query(): fails if there is no user activation');
+  const iterator = navigator.fonts.query();
+
+  await promise_rejects_dom(t, 'NotAllowedError', (async () => {
+                              for await (const f of iterator) {
+                              }
+                            })());
+}, 'iteration fails if there is no user activation');
 
 font_access_test(async t => {
   const iterator = navigator.fonts.query();