Change SubscribePermissionStatusChange to use a RFH to query permissions

Currently updates to permission change subscriptions in PermissionManager won't use the RenderFrameHost to determine the permission value. This results in incorrect permission updates being sent in some cases. This changes updates to query GetPermissionStatusForFrame when possible. This is only not possible when the request is from a worker in which case we just use the worker's origin. Tbr: slan@chromium.org, asanka@chromium.org Bug: 802945 Change-Id: Ia69f7de8f166000661b5560a2f430b3787872b75 Reviewed-on: https://chromium-review.googlesource.com/979735 Commit-Queue: Raymes Khoury <raymes@chromium.org> Reviewed-by: Sami Kyöstilä <skyostil@chromium.org> Reviewed-by: Timothy Loh <timloh@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#581843}
marcosholgado · Aug 9, 2018 · 3ef4f6e · 3ef4f6e
1 parent f2e5d42
commit 3ef4f6e
Show file tree

Hide file tree

Showing 20 changed files with 126 additions and 56 deletions.
diff --git a/android_webview/browser/aw_permission_manager.cc b/android_webview/browser/aw_permission_manager.cc
@@ -448,8 +448,8 @@ PermissionStatus AwPermissionManager::GetPermissionStatusForFrame(
 
 int AwPermissionManager::SubscribePermissionStatusChange(
     PermissionType permission,
+    content::RenderFrameHost* render_frame_host,
     const GURL& requesting_origin,
-    const GURL& embedding_origin,
     const base::Callback<void(PermissionStatus)>& callback) {
   return content::PermissionController::kNoPendingOperation;
 }

diff --git a/android_webview/browser/aw_permission_manager.h b/android_webview/browser/aw_permission_manager.h
@@ -52,8 +52,8 @@ class AwPermissionManager : public content::PermissionControllerDelegate {
       const GURL& requesting_origin) override;
   int SubscribePermissionStatusChange(
       content::PermissionType permission,
+      content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin,
-      const GURL& embedding_origin,
       const base::Callback<void(blink::mojom::PermissionStatus)>& callback)
       override;
   void UnsubscribePermissionStatusChange(int subscription_id) override;

diff --git a/chrome/browser/permissions/permission_manager.cc b/chrome/browser/permissions/permission_manager.cc
@@ -253,7 +253,8 @@ class PermissionManager::PermissionResponseCallback {
 struct PermissionManager::Subscription {
   ContentSettingsType permission;
   GURL requesting_origin;
-  GURL embedding_origin;
+  int render_frame_id = -1;
+  int render_process_id = -1;
   base::Callback<void(ContentSetting)> callback;
   ContentSetting current_value;
 };
@@ -537,35 +538,50 @@ PermissionStatus PermissionManager::GetPermissionStatusForFrame(
     GURL embedding_origin = web_contents->GetLastCommittedURL().GetOrigin();
     result = context->UpdatePermissionStatusWithDeviceStatus(
         result, GetCanonicalOrigin(requesting_origin, embedding_origin),
-        content::WebContents::FromRenderFrameHost(render_frame_host)
-            ->GetLastCommittedURL()
-            .GetOrigin());
+        embedding_origin);
   }
 
   return ContentSettingToPermissionStatus(result.content_setting);
 }
 
 int PermissionManager::SubscribePermissionStatusChange(
     PermissionType permission,
+    content::RenderFrameHost* render_frame_host,
     const GURL& requesting_origin,
-    const GURL& embedding_origin,
     const base::Callback<void(PermissionStatus)>& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (subscriptions_.IsEmpty())
     HostContentSettingsMapFactory::GetForProfile(profile_)->AddObserver(this);
 
   ContentSettingsType content_type = PermissionTypeToContentSetting(permission);
   auto subscription = std::make_unique<Subscription>();
+
+  // The RFH may be null if the request is for a worker.
+  GURL embedding_origin;
+  if (render_frame_host) {
+    content::WebContents* web_contents =
+        content::WebContents::FromRenderFrameHost(render_frame_host);
+    embedding_origin = web_contents->GetLastCommittedURL().GetOrigin();
+    subscription->render_frame_id = render_frame_host->GetRoutingID();
+    subscription->render_process_id = render_frame_host->GetProcess()->GetID();
+    subscription->current_value =
+        GetPermissionStatusForFrame(content_type, render_frame_host,
+                                    requesting_origin)
+            .content_setting;
+  } else {
+    embedding_origin = requesting_origin;
+    subscription->render_frame_id = -1;
+    subscription->render_process_id = -1;
+    subscription->current_value =
+        GetPermissionStatus(content_type, requesting_origin, requesting_origin)
+            .content_setting;
+  }
+
   subscription->permission = content_type;
   subscription->requesting_origin =
       GetCanonicalOrigin(requesting_origin, embedding_origin);
-  subscription->embedding_origin = embedding_origin;
   subscription->callback = base::Bind(&SubscriptionCallbackWrapper, callback);
 
-  subscription->current_value =
-      GetPermissionStatus(content_type, requesting_origin, embedding_origin)
-          .content_setting;
-
   return subscriptions_.Add(std::move(subscription));
 }
 
@@ -599,18 +615,37 @@ void PermissionManager::OnContentSettingChanged(
     if (subscription->permission != content_type)
       continue;
 
+    // The RFH may be null if the request is for a worker.
+    content::RenderFrameHost* rfh = content::RenderFrameHost::FromID(
+        subscription->render_process_id, subscription->render_frame_id);
+    GURL embedding_origin;
+    if (rfh) {
+      content::WebContents* web_contents =
+          content::WebContents::FromRenderFrameHost(rfh);
+      embedding_origin = web_contents->GetLastCommittedURL().GetOrigin();
+    } else {
+      embedding_origin = subscription->requesting_origin;
+    }
+
     if (primary_pattern.IsValid() &&
         !primary_pattern.Matches(subscription->requesting_origin))
       continue;
     if (secondary_pattern.IsValid() &&
-        !secondary_pattern.Matches(subscription->embedding_origin))
+        !secondary_pattern.Matches(embedding_origin))
       continue;
 
-    ContentSetting new_value =
-        GetPermissionStatus(subscription->permission,
-                            subscription->requesting_origin,
-                            subscription->embedding_origin)
-            .content_setting;
+    ContentSetting new_value;
+    if (rfh) {
+      new_value = GetPermissionStatusForFrame(subscription->permission, rfh,
+                                              subscription->requesting_origin)
+                      .content_setting;
+    } else {
+      new_value = GetPermissionStatus(subscription->permission,
+                                      subscription->requesting_origin,
+                                      subscription->requesting_origin)
+                      .content_setting;
+    }
+
     if (subscription->current_value == new_value)
       continue;
 

diff --git a/chrome/browser/permissions/permission_manager.h b/chrome/browser/permissions/permission_manager.h
@@ -108,8 +108,8 @@ class PermissionManager : public KeyedService,
       const GURL& requesting_origin) override;
   int SubscribePermissionStatusChange(
       content::PermissionType permission,
+      content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin,
-      const GURL& embedding_origin,
       const base::Callback<void(blink::mojom::PermissionStatus)>& callback)
       override;
   void UnsubscribePermissionStatusChange(int subscription_id) override;

diff --git a/chrome/browser/permissions/permission_manager_unittest.cc b/chrome/browser/permissions/permission_manager_unittest.cc
@@ -140,6 +140,7 @@ class PermissionManagerTest : public ChromeRenderViewHostTestHarness {
         true /* has_android_location_permission */,
         true /* is_system_location_setting_enabled */);
 #endif
+    NavigateAndCommit(url());
   }
 
   void TearDown() override {
@@ -226,15 +227,15 @@ TEST_F(PermissionManagerTest, SubscriptionDestroyedCleanlyWithoutUnsubscribe) {
   // Test that the PermissionManager shuts down cleanly with subscriptions that
   // haven't been removed, crbug.com/720071.
   GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
-      PermissionType::GEOLOCATION, url(), url(),
+      PermissionType::GEOLOCATION, main_rfh(), url(),
       base::Bind(&PermissionManagerTest::OnPermissionChange,
                  base::Unretained(this)));
 }
 
 TEST_F(PermissionManagerTest, SameTypeChangeNotifies) {
   int subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
-          PermissionType::GEOLOCATION, url(), url(),
+          PermissionType::GEOLOCATION, main_rfh(), url(),
           base::Bind(&PermissionManagerTest::OnPermissionChange,
                      base::Unretained(this)));
 
@@ -252,7 +253,7 @@ TEST_F(PermissionManagerTest, SameTypeChangeNotifies) {
 TEST_F(PermissionManagerTest, DifferentTypeChangeDoesNotNotify) {
   int subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
-          PermissionType::GEOLOCATION, url(), url(),
+          PermissionType::GEOLOCATION, main_rfh(), url(),
           base::Bind(&PermissionManagerTest::OnPermissionChange,
                      base::Unretained(this)));
 
@@ -269,7 +270,7 @@ TEST_F(PermissionManagerTest, DifferentTypeChangeDoesNotNotify) {
 TEST_F(PermissionManagerTest, ChangeAfterUnsubscribeDoesNotNotify) {
   int subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
-          PermissionType::GEOLOCATION, url(), url(),
+          PermissionType::GEOLOCATION, main_rfh(), url(),
           base::Bind(&PermissionManagerTest::OnPermissionChange,
                      base::Unretained(this)));
 
@@ -286,7 +287,7 @@ TEST_F(PermissionManagerTest, ChangeAfterUnsubscribeDoesNotNotify) {
 TEST_F(PermissionManagerTest, DifferentPrimaryUrlDoesNotNotify) {
   int subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
-          PermissionType::GEOLOCATION, url(), url(),
+          PermissionType::GEOLOCATION, main_rfh(), url(),
           base::Bind(&PermissionManagerTest::OnPermissionChange,
                      base::Unretained(this)));
 
@@ -303,7 +304,7 @@ TEST_F(PermissionManagerTest, DifferentPrimaryUrlDoesNotNotify) {
 TEST_F(PermissionManagerTest, DifferentSecondaryUrlDoesNotNotify) {
   int subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
-          PermissionType::GEOLOCATION, url(), url(),
+          PermissionType::GEOLOCATION, main_rfh(), url(),
           base::Bind(&PermissionManagerTest::OnPermissionChange,
                      base::Unretained(this)));
 
@@ -320,7 +321,7 @@ TEST_F(PermissionManagerTest, DifferentSecondaryUrlDoesNotNotify) {
 TEST_F(PermissionManagerTest, WildCardPatternNotifies) {
   int subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
-          PermissionType::GEOLOCATION, url(), url(),
+          PermissionType::GEOLOCATION, main_rfh(), url(),
           base::Bind(&PermissionManagerTest::OnPermissionChange,
                      base::Unretained(this)));
 
@@ -341,7 +342,7 @@ TEST_F(PermissionManagerTest, ClearSettingsNotifies) {
 
   int subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
-          PermissionType::GEOLOCATION, url(), url(),
+          PermissionType::GEOLOCATION, main_rfh(), url(),
           base::Bind(&PermissionManagerTest::OnPermissionChange,
                      base::Unretained(this)));
 
@@ -358,7 +359,7 @@ TEST_F(PermissionManagerTest, ClearSettingsNotifies) {
 TEST_F(PermissionManagerTest, NewValueCorrectlyPassed) {
   int subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
-          PermissionType::GEOLOCATION, url(), url(),
+          PermissionType::GEOLOCATION, main_rfh(), url(),
           base::Bind(&PermissionManagerTest::OnPermissionChange,
                      base::Unretained(this)));
 
@@ -380,7 +381,7 @@ TEST_F(PermissionManagerTest, ChangeWithoutPermissionChangeDoesNotNotify) {
 
   int subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
-          PermissionType::GEOLOCATION, url(), url(),
+          PermissionType::GEOLOCATION, main_rfh(), url(),
           base::Bind(&PermissionManagerTest::OnPermissionChange,
                      base::Unretained(this)));
 
@@ -401,7 +402,38 @@ TEST_F(PermissionManagerTest, ChangesBackAndForth) {
 
   int subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
-          PermissionType::GEOLOCATION, url(), url(),
+          PermissionType::GEOLOCATION, main_rfh(), url(),
+          base::Bind(&PermissionManagerTest::OnPermissionChange,
+                     base::Unretained(this)));
+
+  GetHostContentSettingsMap()->SetContentSettingDefaultScope(
+      url(), url(), CONTENT_SETTINGS_TYPE_GEOLOCATION, std::string(),
+      CONTENT_SETTING_ALLOW);
+
+  EXPECT_TRUE(callback_called());
+  EXPECT_EQ(PermissionStatus::GRANTED, callback_result());
+
+  Reset();
+
+  GetHostContentSettingsMap()->SetContentSettingDefaultScope(
+      url(), url(), CONTENT_SETTINGS_TYPE_GEOLOCATION, std::string(),
+      CONTENT_SETTING_ASK);
+
+  EXPECT_TRUE(callback_called());
+  EXPECT_EQ(PermissionStatus::ASK, callback_result());
+
+  GetPermissionControllerDelegate()->UnsubscribePermissionStatusChange(
+      subscription_id);
+}
+
+TEST_F(PermissionManagerTest, ChangesBackAndForthWorker) {
+  GetHostContentSettingsMap()->SetContentSettingDefaultScope(
+      url(), url(), CONTENT_SETTINGS_TYPE_GEOLOCATION, std::string(),
+      CONTENT_SETTING_ASK);
+
+  int subscription_id =
+      GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
+          PermissionType::GEOLOCATION, nullptr, url(),
           base::Bind(&PermissionManagerTest::OnPermissionChange,
                      base::Unretained(this)));
 
@@ -428,7 +460,7 @@ TEST_F(PermissionManagerTest, ChangesBackAndForth) {
 TEST_F(PermissionManagerTest, SubscribeMIDIPermission) {
   int subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
-          PermissionType::MIDI, url(), url(),
+          PermissionType::MIDI, main_rfh(), url(),
           base::Bind(&PermissionManagerTest::OnPermissionChange,
                      base::Unretained(this)));
 

diff --git a/chromecast/browser/cast_permission_manager.cc b/chromecast/browser/cast_permission_manager.cc
@@ -65,8 +65,8 @@ CastPermissionManager::GetPermissionStatusForFrame(
 
 int CastPermissionManager::SubscribePermissionStatusChange(
     content::PermissionType permission,
+    content::RenderFrameHost* render_frame_host,
     const GURL& requesting_origin,
-    const GURL& embedding_origin,
     const base::Callback<void(blink::mojom::PermissionStatus)>& callback) {
   return content::PermissionController::kNoPendingOperation;
 }

diff --git a/chromecast/browser/cast_permission_manager.h b/chromecast/browser/cast_permission_manager.h
@@ -46,8 +46,8 @@ class CastPermissionManager : public content::PermissionControllerDelegate {
       const GURL& requesting_origin) override;
   int SubscribePermissionStatusChange(
       content::PermissionType permission,
+      content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin,
-      const GURL& embedding_origin,
       const base::Callback<void(blink::mojom::PermissionStatus)>& callback)
       override;
   void UnsubscribePermissionStatusChange(int subscription_id) override;

diff --git a/components/domain_reliability/service_unittest.cc b/components/domain_reliability/service_unittest.cc
@@ -102,8 +102,8 @@ class TestPermissionManager : public content::PermissionControllerDelegate {
 
   int SubscribePermissionStatusChange(
       content::PermissionType permission,
+      content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin,
-      const GURL& embedding_origin,
       const base::Callback<void(blink::mojom::PermissionStatus)>& callback)
       override {
     NOTIMPLEMENTED();

diff --git a/content/browser/permissions/permission_controller_impl.cc b/content/browser/permissions/permission_controller_impl.cc
@@ -101,15 +101,15 @@ void PermissionControllerImpl::ResetPermission(PermissionType permission,
 
 int PermissionControllerImpl::SubscribePermissionStatusChange(
     PermissionType permission,
+    RenderFrameHost* render_frame_host,
     const GURL& requesting_origin,
-    const GURL& embedding_origin,
     const base::Callback<void(blink::mojom::PermissionStatus)>& callback) {
   PermissionControllerDelegate* delegate =
       browser_context_->GetPermissionControllerDelegate();
   if (!delegate)
     return kNoPendingOperation;
   return delegate->SubscribePermissionStatusChange(
-      permission, requesting_origin, embedding_origin, callback);
+      permission, render_frame_host, requesting_origin, callback);
 }
 
 void PermissionControllerImpl::UnsubscribePermissionStatusChange(

diff --git a/content/browser/permissions/permission_controller_impl.h b/content/browser/permissions/permission_controller_impl.h
@@ -55,8 +55,8 @@ class CONTENT_EXPORT PermissionControllerImpl : public PermissionController {
 
   int SubscribePermissionStatusChange(
       PermissionType permission,
+      RenderFrameHost* render_frame_host,
       const GURL& requesting_origin,
-      const GURL& embedding_origin,
       const base::Callback<void(blink::mojom::PermissionStatus)>& callback);
 
   void UnsubscribePermissionStatusChange(int subscription_id);

diff --git a/content/browser/permissions/permission_service_context.cc b/content/browser/permissions/permission_service_context.cc
@@ -97,15 +97,10 @@ void PermissionServiceContext::CreateSubscription(
   auto subscription =
       std::make_unique<PermissionSubscription>(this, std::move(observer));
   GURL requesting_origin(origin.Serialize());
-  GURL embedding_origin = GetEmbeddingOrigin();
   int subscription_id =
       PermissionControllerImpl::FromBrowserContext(browser_context)
           ->SubscribePermissionStatusChange(
-              permission_type, requesting_origin,
-              // If the embedding_origin is empty, we'll use the |origin|
-              // instead.
-              embedding_origin.is_empty() ? requesting_origin
-                                          : embedding_origin,
+              permission_type, render_frame_host_, requesting_origin,
               base::Bind(&PermissionSubscription::OnPermissionStatusChanged,
                          base::Unretained(subscription.get())));
   subscription->set_id(subscription_id);

diff --git a/content/public/browser/permission_controller_delegate.h b/content/public/browser/permission_controller_delegate.h
@@ -77,13 +77,13 @@ class CONTENT_EXPORT PermissionControllerDelegate {
                                const GURL& embedding_origin) = 0;
 
   // Runs the given |callback| whenever the |permission| associated with the
-  // pair { requesting_origin, embedding_origin } changes.
-  // Returns the subscription_id to be used to unsubscribe. Can be
-  // kNoPendingOperation if the subscribe was not successful.
+  // given RenderFrameHost changes. A nullptr should be passed if the request
+  // is from a worker. Returns the subscription_id to be used to unsubscribe.
+  // Can be kNoPendingOperation if the subscribe was not successful.
   virtual int SubscribePermissionStatusChange(
-      PermissionType permission,
+      content::PermissionType permission,
+      content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin,
-      const GURL& embedding_origin,
       const base::Callback<void(blink::mojom::PermissionStatus)>& callback) = 0;
 
   // Unregisters from permission status change notifications.

diff --git a/content/public/test/mock_permission_manager.cc b/content/public/test/mock_permission_manager.cc
@@ -34,8 +34,8 @@ int MockPermissionManager::RequestPermissions(
 
 int MockPermissionManager::SubscribePermissionStatusChange(
     PermissionType permission,
+    RenderFrameHost* render_frame_host,
     const GURL& requesting_origin,
-    const GURL& embedding_origin,
     const base::Callback<void(blink::mojom::PermissionStatus)>& callback) {
   // Return a fake subscription_id.
   return 0;