Initial Implementation of Iframe Attribute for Feature Policy (Part 4)

third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp

Index: third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
diff --git a/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp b/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
index daa9d3a40b8bb14cf87cc61fa4d1bd42b6b2862e..204d2210b176fd9299f4ee969325f90710c33b5c 100644
--- a/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
+++ b/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
@@ -12,6 +12,28 @@
 
 namespace blink {
 
+namespace {
+
+void addAllowFeatureToList(bool isFeatureAllowed,
+                           WebFeaturePolicyFeature feature,
+                           Vector<WebParsedFeaturePolicyDeclaration>& list) {
+  if (isFeatureAllowed) {
+    WebParsedFeaturePolicyDeclaration whitelist;
+    whitelist.feature = feature;
+    whitelist.matchesAllOrigins = true;
+    list.push_back(whitelist);
+  }
+}
+
+void overridePolicy(int index,
+                    Vector<WebParsedFeaturePolicyDeclaration>& whitelists,
+                    RefPtr<SecurityOrigin> origin) {
+  whitelists[index].matchesAllOrigins = false;
+  whitelists[index].origins = Vector<WebSecurityOrigin>(1UL, {origin});
+}
+
+}  // namespace
+
 WebFeaturePolicyFeature getWebFeaturePolicyFeature(const String& feature) {
   if (feature == "fullscreen")
     return WebFeaturePolicyFeature::Fullscreen;
@@ -114,9 +136,30 @@ WebParsedFeaturePolicy parseFeaturePolicy(const String& policy,
 // allowpaymentrequest into account when constructing the whitelist.
 WebParsedFeaturePolicy getContainerPolicyFromAllowedFeatures(
     const WebVector<WebFeaturePolicyFeature>& features,
+    bool allowfullscreen,
+    bool allowpayment,
     RefPtr<SecurityOrigin> origin) {
+  DLOG(WARNING) << origin->toString();

[iclelland, 2017/04/11 19:04:21]

Nit: Remove the warning

   Vector<WebParsedFeaturePolicyDeclaration> whitelists;
+  // If allowfullscreen attribute or allowpayment attribute is(are) present,

[iclelland, 2017/04/11 19:04:21]

I think you can get away with just using "are" here

(Or you could re-word it into two different comments: "If the allowfullscreen
attribute is present, enable the feature for all origins", and similarly for
allowpaymentrequest.)

+  // define the policy as enabled for all.
+  addAllowFeatureToList(allowfullscreen, WebFeaturePolicyFeature::Fullscreen,
+                        whitelists);

[iclelland, 2017/04/11 19:04:21]

Could you move the if(isFeatureAllowed) out of |addAllowFeatureToList|, and just
check them here? It would make it clearer that we only do something if the
condition is true.

+  addAllowFeatureToList(allowpayment, WebFeaturePolicyFeature::Payment,
+                        whitelists);
   for (const WebFeaturePolicyFeature feature : features) {
+    // allow policy will override the allow* attribute policy.

[iclelland, 2017/04/11 19:04:21]

This logic is really hard to follow; it took me a while to figure out why it's
hard coding the idea of "first" and "second".

Could we make it clearer by adding the "allow" policies first, and if you see
fullscreen (or paymentrequest) in there, just set a flag. Then afterwards, you
can process the "allowfullscreen" and "allowpaymentrequest" policies, and ignore
them if their flag is set. Would that make sense?

+    bool shouldOverrideFirstPolicy =
+        (feature == WebFeaturePolicyFeature::Fullscreen && allowfullscreen) ||
+        (feature == WebFeaturePolicyFeature::Payment && allowpayment &&
+         !allowfullscreen);
+    bool shouldOverrideSecondPolicy =
+        feature == WebFeaturePolicyFeature::Payment && allowpayment &&
+        allowfullscreen;
+    if (shouldOverrideFirstPolicy || shouldOverrideSecondPolicy) {
+      overridePolicy(shouldOverrideFirstPolicy ? 0 : 1, whitelists, origin);
+      continue;
+    }
     WebParsedFeaturePolicyDeclaration whitelist;
     whitelist.feature = feature;
     whitelist.origins = Vector<WebSecurityOrigin>(1UL, {origin});