Adds Cacao blacklist check for Server Previews (aka, LitePageRedirect)

Bug: 864640
Change-Id: I28f5ac776473882973ab5faad2d4426a1f91b959
Reviewed-on: https://chromium-review.googlesource.com/1228974Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Doug Arnett <dougarnett@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592492}

components/previews/content/previews_decider_impl.cc

@@ -64,14 +64,14 @@ bool AllowedOnReload(PreviewsType type) {
   return false;
 }
 
-bool IsServerWhitelistedType(PreviewsType type) {
+bool ShouldCheckOptimizationHints(PreviewsType type) {
   switch (type) {
-    // These types check server whitelist, if available.
+    // These types may have server optimization hints.
     case PreviewsType::NOSCRIPT:
     case PreviewsType::RESOURCE_LOADING_HINTS:
-    // TODO(crbug.com/864640): Move to false when bug is fixed.
     case PreviewsType::LITE_PAGE_REDIRECT:
       return true;
+    // These types do not have server optimization hints.
     case PreviewsType::OFFLINE:
     case PreviewsType::LITE_PAGE:
     case PreviewsType::LOFI:
@@ -232,7 +232,9 @@ void PreviewsDeciderImpl::SetIgnorePreviewsBlacklistDecision(bool ignored) {
 
 bool PreviewsDeciderImpl::ShouldAllowPreview(const net::URLRequest& request,
                                              PreviewsType type) const {
-  DCHECK(type == PreviewsType::OFFLINE || type == PreviewsType::NOSCRIPT ||
+  DCHECK(type == PreviewsType::OFFLINE ||
+         type == PreviewsType::LITE_PAGE_REDIRECT ||
+         type == PreviewsType::NOSCRIPT ||
          type == PreviewsType::RESOURCE_LOADING_HINTS);
   // Consumers that need to specify a blacklist or ignore flag should use
   // ShouldAllowPreviewAtECT directly.
@@ -245,7 +247,7 @@ bool PreviewsDeciderImpl::ShouldAllowPreviewAtECT(
     const net::URLRequest& request,
     PreviewsType type,
     net::EffectiveConnectionType effective_connection_type_threshold,
-    const std::vector<std::string>& host_blacklist_from_server,
+    const std::vector<std::string>& host_blacklist_from_finch,
     bool ignore_long_term_black_list_rules) const {
   if (!previews::params::ArePreviewsAllowed()) {
     return false;
@@ -346,9 +348,9 @@ bool PreviewsDeciderImpl::ShouldAllowPreviewAtECT(
   }
   passed_reasons.push_back(PreviewsEligibilityReason::RELOAD_DISALLOWED);
 
-  // Check provided blacklist, if any. This type of blacklist was added for
-  // Finch provided blacklist for Client LoFi.
-  if (base::ContainsValue(host_blacklist_from_server,
+  // Check Finch-provided blacklist, if any. This type of blacklist was added
+  // for Finch provided blacklist for Client LoFi.
+  if (base::ContainsValue(host_blacklist_from_finch,
                           request.url().host_piece())) {
     LogPreviewDecisionMade(
         PreviewsEligibilityReason::HOST_BLACKLISTED_BY_SERVER, request.url(),
@@ -358,10 +360,11 @@ bool PreviewsDeciderImpl::ShouldAllowPreviewAtECT(
   passed_reasons.push_back(
       PreviewsEligibilityReason::HOST_BLACKLISTED_BY_SERVER);
 
-  // Check whitelist from the server, if provided.
-  if (IsServerWhitelistedType(type)) {
+  // Check server whitelist/blacklist, if provided.
+  if (ShouldCheckOptimizationHints(type)) {
     if (params::IsOptimizationHintsEnabled()) {
-      // Optimization hints are configured, so require whitelist match.
+      // Optimization hints are configured, so determine if those hints
+      // allow the optimization type (as of start-of-navigation time anyway).
       PreviewsEligibilityReason status = ShouldAllowPreviewPerOptimizationHints(
           request, type, &passed_reasons);
       if (status != PreviewsEligibilityReason::ALLOWED) {
@@ -378,7 +381,8 @@ bool PreviewsDeciderImpl::ShouldAllowPreviewAtECT(
           page_id);
       return false;
     } else {
-      DCHECK_EQ(PreviewsType::NOSCRIPT, type);
+      DCHECK(type == PreviewsType::LITE_PAGE_REDIRECT ||
+             type == PreviewsType::NOSCRIPT);
       // Since server optimization guidance not configured, allow the preview
       // but with qualified eligibility reason.
       LogPreviewDecisionMade(
@@ -424,8 +428,8 @@ bool PreviewsDeciderImpl::IsURLAllowedForPreview(const net::URLRequest& request,
     }
   }
 
-  // Check whitelist from the server, if provided.
-  if (IsServerWhitelistedType(type)) {
+  // Re-check server optimization hints (if provided) on this commit-time URL.
+  if (ShouldCheckOptimizationHints(type)) {
     if (params::IsOptimizationHintsEnabled()) {
       std::vector<PreviewsEligibilityReason> passed_reasons;
       PreviewsEligibilityReason status =
@@ -447,7 +451,8 @@ PreviewsDeciderImpl::ShouldAllowPreviewPerOptimizationHints(
     const net::URLRequest& request,
     PreviewsType type,
     std::vector<PreviewsEligibilityReason>* passed_reasons) const {
-  DCHECK(type == PreviewsType::NOSCRIPT ||
+  DCHECK(type == PreviewsType::LITE_PAGE_REDIRECT ||
+         type == PreviewsType::NOSCRIPT ||
          type == PreviewsType::RESOURCE_LOADING_HINTS);
 
   // Per-PreviewsType default if no optimization guide data.
@@ -459,6 +464,15 @@ PreviewsDeciderImpl::ShouldAllowPreviewPerOptimizationHints(
     }
   }
 
+  // For LitePageRedirect, ensure it is not blacklisted for this request.
+  if (type == PreviewsType::LITE_PAGE_REDIRECT) {
+    if (previews_opt_guide_->IsBlacklisted(request, type)) {
+      return PreviewsEligibilityReason::HOST_BLACKLISTED_BY_SERVER;
+    }
+    passed_reasons->push_back(
+        PreviewsEligibilityReason::HOST_BLACKLISTED_BY_SERVER);
+  }
+
   // For NoScript, ensure it is whitelisted for this request.
   if (type == PreviewsType::NOSCRIPT) {
     if (!previews_opt_guide_->IsWhitelisted(request, type)) {
@@ -479,7 +493,8 @@ PreviewsDeciderImpl::IsURLAllowedForPreviewByOptmizationHints(
     const net::URLRequest& request,
     PreviewsType type,
     std::vector<PreviewsEligibilityReason>* passed_reasons) const {
-  DCHECK(type == PreviewsType::NOSCRIPT ||
+  DCHECK(type == PreviewsType::LITE_PAGE_REDIRECT ||
+         type == PreviewsType::NOSCRIPT ||
          type == PreviewsType::RESOURCE_LOADING_HINTS);
 
   // For NoScript, if optimization guide is not present, assume that all URLs

components/previews/content/previews_decider_impl.h

@@ -116,7 +116,7 @@ class PreviewsDeciderImpl : public PreviewsDecider,
       const net::URLRequest& request,
       PreviewsType type,
       net::EffectiveConnectionType effective_connection_type_threshold,
-      const std::vector<std::string>& host_blacklist_from_server,
+      const std::vector<std::string>& host_blacklist_from_finch,
       bool ignore_long_term_black_list_rules) const override;
   bool IsURLAllowedForPreview(const net::URLRequest& request,
                               PreviewsType type) const override;

components/previews/content/previews_decider_impl_unittest.cc

@@ -127,8 +127,8 @@ class TestPreviewsBlackList : public PreviewsBlackList {
   PreviewsEligibilityReason status_;
 };
 
-// Stub class of PreviewsOptimizationGuide to control IsWhitelisted outcome
-// when testing PreviewsDeciderImpl.
+// Stub class of PreviewsOptimizationGuide to control IsWhitelisted and
+// IsBlacklisted outcomes when testing PreviewsDeciderImpl.
 class TestPreviewsOptimizationGuide : public PreviewsOptimizationGuide {
  public:
   TestPreviewsOptimizationGuide(
@@ -152,6 +152,16 @@ class TestPreviewsOptimizationGuide : public PreviewsOptimizationGuide {
     }
     return false;
   }
+
+  // PreviewsOptimizationGuide:
+  bool IsBlacklisted(const net::URLRequest& request,
+                     PreviewsType type) const override {
+    EXPECT_TRUE(type == PreviewsType::LITE_PAGE_REDIRECT);
+    if (type == PreviewsType::LITE_PAGE_REDIRECT) {
+      return request.url().host().compare("blacklisted.example.com") == 0;
+    }
+    return false;
+  }
 };
 
 // Stub class of PreviewsUIService to test logging functionalities in
@@ -383,6 +393,7 @@ class PreviewsDeciderImplTest : public testing::Test {
     allowed_types[static_cast<int>(PreviewsType::OFFLINE)] = 0;
     allowed_types[static_cast<int>(PreviewsType::LOFI)] = 0;
     allowed_types[static_cast<int>(PreviewsType::LITE_PAGE)] = 0;
+    allowed_types[static_cast<int>(PreviewsType::LITE_PAGE_REDIRECT)] = 0;
     allowed_types[static_cast<int>(PreviewsType::NOSCRIPT)] = 0;
     allowed_types[static_cast<int>(PreviewsType::RESOURCE_LOADING_HINTS)] = 0;
     ui_service_.reset(new TestPreviewsUIService(
@@ -986,6 +997,69 @@ TEST_F(PreviewsDeciderImplTest, NoScriptCommitTimeWhitelistCheck) {
   }
 }
 
+TEST_F(PreviewsDeciderImplTest,
+       LitePageRedirectAllowedWithoutOptimizationHints) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {features::kPreviews, features::kLitePageServerPreviews},
+      {features::kOptimizationHints});
+  InitializeUIService();
+
+  network_quality_estimator()->set_effective_connection_type(
+      net::EFFECTIVE_CONNECTION_TYPE_2G);
+
+  base::HistogramTester histogram_tester;
+  EXPECT_TRUE(previews_decider_impl()->ShouldAllowPreviewAtECT(
+      *CreateRequest(), PreviewsType::LITE_PAGE_REDIRECT,
+      previews::params::GetECTThresholdForPreview(
+          previews::PreviewsType::LITE_PAGE_REDIRECT),
+      std::vector<std::string>(), false));
+
+  histogram_tester.ExpectUniqueSample(
+      "Previews.EligibilityReason.LitePageRedirect",
+      static_cast<int>(
+          PreviewsEligibilityReason::ALLOWED_WITHOUT_OPTIMIZATION_HINTS),
+      1);
+}
+
+TEST_F(PreviewsDeciderImplTest, LitePageRedirectDisallowedByServerBlacklist) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {features::kPreviews, features::kLitePageServerPreviews,
+       features::kOptimizationHints},
+      {});
+  InitializeUIService();
+
+  network_quality_estimator()->set_effective_connection_type(
+      net::EFFECTIVE_CONNECTION_TYPE_2G);
+
+  base::HistogramTester histogram_tester;
+
+  // First verify preview allowed for non-whitelisted url.
+  EXPECT_TRUE(previews_decider_impl()->ShouldAllowPreviewAtECT(
+      *CreateHttpsRequest(), PreviewsType::LITE_PAGE_REDIRECT,
+      previews::params::GetECTThresholdForPreview(
+          previews::PreviewsType::LITE_PAGE_REDIRECT),
+      std::vector<std::string>(), false));
+
+  histogram_tester.ExpectUniqueSample(
+      "Previews.EligibilityReason.LitePageRedirect",
+      static_cast<int>(PreviewsEligibilityReason::ALLOWED), 1);
+
+  // Now verify no preview for blacklisted url.
+  EXPECT_FALSE(previews_decider_impl()->ShouldAllowPreviewAtECT(
+      *CreateRequestWithURL(GURL("https://blacklisted.example.com")),
+      PreviewsType::LITE_PAGE_REDIRECT,
+      previews::params::GetECTThresholdForPreview(
+          previews::PreviewsType::LITE_PAGE_REDIRECT),
+      std::vector<std::string>(), false));
+
+  histogram_tester.ExpectBucketCount(
+      "Previews.EligibilityReason.LitePageRedirect",
+      static_cast<int>(PreviewsEligibilityReason::HOST_BLACKLISTED_BY_SERVER),
+      1);
+}
+
 TEST_F(PreviewsDeciderImplTest, ResourceLoadingHintsDisallowedByDefault) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitWithFeatures(

components/previews/core/previews_decider.h

@@ -30,7 +30,7 @@ class PreviewsDecider {
       const net::URLRequest& request,
       PreviewsType type,
       net::EffectiveConnectionType effective_connection_type_threshold,
-      const std::vector<std::string>& host_blacklist_from_server,
+      const std::vector<std::string>& host_blacklist_from_finch,
       bool ignore_long_term_black_list_rules) const = 0;
 
   // Same as ShouldAllowPreviewAtECT, but uses the previews default

tools/metrics/histograms/histograms.xml

@@ -128913,6 +128913,7 @@ uploading your change for review.
 <histogram_suffixes name="Previews.Types" separator=".">
   <suffix name="AMPRedirection" label="AMP Redirection previews"/>
   <suffix name="LitePage" label="Lite page previews"/>
+  <suffix name="LitePageRedirect" label="Lite page redirection previews"/>
   <suffix name="LoFi" label="LoFi previews"/>
   <suffix name="NoScript" label="NoScript previews"/>
   <suffix name="Offline" label="Offline previews"/>