Add tests for FeaturePolicy integration with RenderFrameHost

content/browser/frame_host/render_frame_host_manager_unittest.cc

Index: content/browser/frame_host/render_frame_host_manager_unittest.cc
diff --git a/content/browser/frame_host/render_frame_host_manager_unittest.cc b/content/browser/frame_host/render_frame_host_manager_unittest.cc
index a250dae3376ffa69e62f0918beaf733c5410e248..42c51a8263d94170f3ab7760347a6d072439de92 100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -24,6 +24,7 @@
 #include "content/browser/frame_host/render_frame_proxy_host.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
+#include "content/common/feature_policy/feature_policy.h"
 #include "content/common/frame_messages.h"
 #include "content/common/frame_owner_properties.h"
 #include "content/common/input_messages.h"
@@ -55,6 +56,7 @@
 #include "content/test/test_web_contents.h"
 #include "net/base/load_flags.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/WebKit/public/platform/WebFeaturePolicyFeature.h"
 #include "third_party/WebKit/public/platform/WebInsecureRequestPolicy.h"
 #include "third_party/WebKit/public/web/WebSandboxFlags.h"
 #include "ui/base/page_transition_types.h"
@@ -3167,4 +3169,145 @@ TEST_F(RenderFrameHostManagerTest,
   EXPECT_FALSE(initial_rfh->navigation_handle());
 }
 
+// Integration tests for feature policy setup and querying through a RFH. These
+// tests are not meant to cover every edge case as the FeaturePolicy class
+// itself is tested thoroughly in feature_policy_unittest.cc. Instead they are
+// meant to ensure that integration with RenderFrameHost works correctly.
+class RenderFrameFeaturePolicyTest : public content::RenderViewHostTestHarness {

[alexmos, 2017/05/16 21:40:05]

After looking at this and also consulting with creis@, I think this might be
better in web_contents_impl_unittest.cc.  render_frame_host_manager_unittests.cc
should be for testing proxies, swapping behavior, etc - things that you didn't
need to get involved with here.

Alternatively, given that you have quite a bit of FP-specific setup here, it
might be ok to just put these in a new file, perhaps
frame_host/render_frame_host_feature_policy_unittest.cc?

[raymes, 2017/05/17 05:46:41]

I like the idea of a separate test file :) (giant files scare me in general)

+ protected:
+  static constexpr const char* kOrigin1 = "https://google.com";
+  static constexpr const char* kOrigin2 = "https://maps.google.com";
+  static constexpr const char* kOrigin3 = "https://example.com";
+  static constexpr const char* kOrigin4 = "https://test.com";
+
+  static const blink::WebFeaturePolicyFeature kDefaultEnabledFeature =
+      blink::WebFeaturePolicyFeature::kDocumentWrite;
+  static const blink::WebFeaturePolicyFeature kDefaultSelfFeature =
+      blink::WebFeaturePolicyFeature::kGeolocation;
+
+  RenderFrameHost* GetMainRFH(const char* origin) {
+    RenderFrameHost* result = web_contents()->GetMainFrame();
+    RenderFrameHostTester::For(result)->InitializeRenderFrameIfNeeded();
+    RenderFrameHostTester::For(result)->SimulateNavigationCommit(GURL(origin));
+    return result;
+  }
+
+  RenderFrameHost* AddChildRFH(RenderFrameHost* parent, const char* origin) {
+    RenderFrameHost* result =
+        RenderFrameHostTester::For(parent)->AppendChild("");
+    RenderFrameHostTester::For(result)->InitializeRenderFrameIfNeeded();
+    RenderFrameHostTester::For(result)->SimulateNavigationCommit(GURL(origin));
+    return result;
+  }
+
+  void SetHeaderPolicy(RenderFrameHost* rfh,
+                       blink::WebFeaturePolicyFeature feature,
+                       const std::vector<std::string>& origins) {
+    static_cast<TestRenderFrameHost*>(rfh)->OnDidSetFeaturePolicyHeader(
+        CreateFPHeader(feature, origins));
+  }
+
+  void SetContainerPolicy(RenderFrameHost* parent,
+                          RenderFrameHost* child,
+                          blink::WebFeaturePolicyFeature feature,
+                          const std::vector<std::string>& origins) {
+    static_cast<TestRenderFrameHost*>(parent)->OnDidChangeFramePolicy(
+        child->GetRoutingID(), blink::WebSandboxFlags(),
+        CreateFPHeader(feature, origins));
+  }
+
+ private:
+  ParsedFeaturePolicyHeader CreateFPHeader(
+      blink::WebFeaturePolicyFeature feature,
+      const std::vector<std::string>& origins) {
+    ParsedFeaturePolicyHeader result(1);
+    result[0].feature = feature;
+    result[0].matches_all_origins = false;
+    for (const std::string& origin : origins)
+      result[0].origins.push_back(url::Origin(GURL(origin)));
+    return result;
+  }
+};
+
+TEST_F(RenderFrameFeaturePolicyTest, DefaultPolicy) {

[alexmos, 2017/05/16 21:40:04]

I'd include Host in the name, i.e. RenderFrameHostFeaturePolicyTest

[raymes, 2017/05/17 05:46:41]

Oops, done.

+  RenderFrameHost* parent = GetMainRFH(kOrigin1);
+  RenderFrameHost* child = AddChildRFH(parent, kOrigin2);
+
+  EXPECT_TRUE(parent->IsFeatureEnabled(kDefaultEnabledFeature));
+  EXPECT_TRUE(parent->IsFeatureEnabled(kDefaultSelfFeature));
+  EXPECT_TRUE(child->IsFeatureEnabled(kDefaultEnabledFeature));
+  EXPECT_FALSE(child->IsFeatureEnabled(kDefaultSelfFeature));
+}
+
+TEST_F(RenderFrameFeaturePolicyTest, HeaderPolicy) {
+  RenderFrameHost* parent = GetMainRFH(kOrigin1);
+
+  // Enable the feature for the child in the parent frame.
+  SetHeaderPolicy(parent, kDefaultSelfFeature, {kOrigin1, kOrigin2});
+
+  // Create the child.
+  RenderFrameHost* child = AddChildRFH(parent, kOrigin2);
+
+  EXPECT_TRUE(parent->IsFeatureEnabled(kDefaultSelfFeature));
+  EXPECT_TRUE(child->IsFeatureEnabled(kDefaultSelfFeature));
+
+  // Set an empty whitelist in the child to test that the policies combine
+  // correctly.
+  SetHeaderPolicy(child, kDefaultSelfFeature, {});
+
+  EXPECT_TRUE(parent->IsFeatureEnabled(kDefaultSelfFeature));
+  EXPECT_FALSE(child->IsFeatureEnabled(kDefaultSelfFeature));
+
+  // Re-enable the feature in the child.
+  SetHeaderPolicy(child, kDefaultSelfFeature, {kOrigin2});
+  EXPECT_TRUE(child->IsFeatureEnabled(kDefaultSelfFeature));
+
+  // Navigate the child. Check that the feature is disabled.
+  RenderFrameHostTester::For(child)->SimulateNavigationCommit(GURL(kOrigin3));
+  EXPECT_FALSE(child->IsFeatureEnabled(kDefaultSelfFeature));
+}
+
+TEST_F(RenderFrameFeaturePolicyTest, ContainerPolicy) {
+  RenderFrameHost* parent = GetMainRFH(kOrigin1);
+  RenderFrameHost* child = AddChildRFH(parent, kOrigin2);
+
+  // Set a container policy on origin 3 to give it the feature. It should not
+  // be enabled because container policy will only take effect after navigation.
+  SetContainerPolicy(parent, child, kDefaultSelfFeature, {kOrigin2, kOrigin3});
+  EXPECT_FALSE(child->IsFeatureEnabled(kDefaultSelfFeature));
+
+  // Navigate the child so that the container policy takes effect.
+  RenderFrameHostTester::For(child)->SimulateNavigationCommit(GURL(kOrigin3));
+  EXPECT_TRUE(child->IsFeatureEnabled(kDefaultSelfFeature));
+
+  // Navigate the child again, the feature should not be enabled.
+  RenderFrameHostTester::For(child)->SimulateNavigationCommit(GURL(kOrigin4));
+  EXPECT_FALSE(child->IsFeatureEnabled(kDefaultSelfFeature));
+}
+
+TEST_F(RenderFrameFeaturePolicyTest, HeaderAndContainerPolicy) {
+  RenderFrameHost* parent = GetMainRFH(kOrigin1);
+
+  // Set a header policy and container policy. Check that they both take effect.
+  SetHeaderPolicy(parent, kDefaultSelfFeature, {kOrigin1, kOrigin2});
+
+  RenderFrameHost* child = AddChildRFH(parent, kOrigin2);
+  SetContainerPolicy(parent, child, kDefaultSelfFeature, {kOrigin3});
+
+  // The feature should be enabled in kOrigin2, kOrigin3 but not kOrigin4.
+  EXPECT_TRUE(child->IsFeatureEnabled(kDefaultSelfFeature));
+  RenderFrameHostTester::For(child)->SimulateNavigationCommit(GURL(kOrigin3));
+  EXPECT_TRUE(child->IsFeatureEnabled(kDefaultSelfFeature));
+  RenderFrameHostTester::For(child)->SimulateNavigationCommit(GURL(kOrigin4));
+  EXPECT_FALSE(child->IsFeatureEnabled(kDefaultSelfFeature));
+
+  // Change the header policy to turn off the feature. It should be disabled in
+  // all children.
+  SetHeaderPolicy(parent, kDefaultSelfFeature, {});

[alexmos, 2017/05/16 21:40:05]

Is this testing a scenario that's actually possible?  If the header policy for
the parent frame changes, doesn't that imply that the parent has committed a new
document?  And in that case, the old child frames should go away, so |child|
wouldn't normally exist for the checks below.

[raymes, 2017/05/17 05:46:41]

It's true - I was cheating :) I changed it to "RefreshPageAndSetHeaderPolicy". A
few questions though:
1) I added a DCHECK in RenderFrameHostImpl to ensure a header isn't set twice
(to make what I was trying to do throw an error). Does that seem like it holds
to you? 
2) I had to navigate to a different URL in order to actually have the navigation
stick to simulate a refresh. Is there a way around that?
3) It doesn't seem like the child RenderFrameHost gets destroyed when its parent
navigates. It's not problematic for this test, but just want to make sure that
I'm not doing something wrong.

[alexmos, 2017/05/17 22:48:49]

Will that work across multiple documents in the same frame?  (I.e., same-site
renderer-initiated navigations that reuse the RFH?)  I guess we should be
resetting the header as part of commit, and then
FrameHostMsg_DidSetFeaturePolicyHeader would come in right afterward, so it
should work.
Are you referring to the about:blank navigation that precedes the real one? 
Might be some weirdness with the unit test setup -- e.g., it might be thinking
that we haven't changed the document, due to this url comparison:
https://cs.chromium.org/chromium/src/content/test/test_render_frame_host.cc?l...

You might be able to simulate the commit some other way to avoid this.  The
comment on SimulateNavigationCommit actually says it's deprecated in favor of
NavigationSimulator, so you might want to give that a try.  (example:
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe...)
That might just be a problem with unit tests.  For cross-process navigations,
the child FrameTreeNodes are removed by ResetForNewProcess(), and for
same-process navigations, I think they'd be detached by FrameHostMsg_Detach
messages as part of unloading the previous document, which probably aren't
sent/simulated in unit tests.

+  RenderFrameHostTester::For(child)->SimulateNavigationCommit(GURL(kOrigin2));
+  EXPECT_FALSE(child->IsFeatureEnabled(kDefaultSelfFeature));
+  RenderFrameHostTester::For(child)->SimulateNavigationCommit(GURL(kOrigin3));
+  EXPECT_FALSE(child->IsFeatureEnabled(kDefaultSelfFeature));
+}
+
 }  // namespace content