Componentize SafeBrowsingBlockingPage for WebView use

components/safe_browsing/base_safe_browsing_blocking_page.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Implementation of the BaseSafeBrowsingBlockingPage class.
+
+#include "components/safe_browsing/base_safe_browsing_blocking_page.h"
+
+#include "base/bind.h"
+#include "base/lazy_instance.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/time/time.h"
+#include "components/safe_browsing_db/safe_browsing_prefs.h"
+#include "components/security_interstitials/content/security_interstitial_controller_client.h"
+#include "components/security_interstitials/core/metrics_helper.h"
+#include "content/public/browser/interstitial_page.h"
+#include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/user_metrics.h"
+#include "content/public/browser/web_contents.h"
+
+using base::UserMetricsAction;
+using content::InterstitialPage;
+using content::WebContents;
+using security_interstitials::SafeBrowsingErrorUI;
+using security_interstitials::SecurityInterstitialControllerClient;
+
+namespace safe_browsing {
+
+namespace {
+
+base::LazyInstance<BaseSafeBrowsingBlockingPage::UnsafeResourceMap>
+    g_unsafe_resource_map = LAZY_INSTANCE_INITIALIZER;

[meacer, 2017/01/10 21:18:21]

You can make this leaky so that it's not destructed during shutdown.

base::LazyInstance<BaseSafeBrowsingBlockingPage::UnsafeResourceMap>::Leaky
    g_unsafe_resource_map = LAZY_INSTANCE_INITIALIZER;

[Jialiu Lin, 2017/01/11 00:50:06]

Good to know. Thanks!

+
+}  // namespace
+
+BaseSafeBrowsingBlockingPage::BaseSafeBrowsingBlockingPage(
+    BaseSafeBrowsingUIManager* ui_manager,
+    WebContents* web_contents,
+    const GURL& main_frame_url,
+    const UnsafeResourceList& unsafe_resources,
+    std::unique_ptr<SecurityInterstitialControllerClient> controller_client,
+    SafeBrowsingErrorUI::SBErrorDisplayOptions* display_options)
+    : SecurityInterstitialPage(web_contents,
+                               unsafe_resources[0].url,

[meacer, 2017/01/10 21:18:20]

nit: While you are at it, I wonder if it makes sense to refactor unsafe
resources code so that it doesn't necessarily rely on the first item.

E.g. something like:

struct UnsafeResourceList {
  GURL page_url;
  vector<UnsafeResource> resources;
}

I don't know if this is more desirable, or whether it makes sense at all, just
throwing the idea around.

[Jialiu Lin, 2017/01/11 00:50:06]

Good thinking. Let's come back to this idea later, since it is kind of
orthogonal to this refactoring.

+                               std::move(controller_client)),
+      ui_manager_(ui_manager),
+      main_frame_url_(main_frame_url),

[meacer, 2017/01/10 21:18:21]

Actually, it looks like we have two URLs here: unsafe_resources[0].url and
main_frame_url. Are they always the same? Can we use only one of them?

Looks like SecurityInterstitialPage isn't documented well, so it's hard to tell
what the |url| parameter in  SecurityInterstitialPage constructor refers to, but
it seems to be the main page URL (based on what content::InterstitialPage says).

[Jialiu Lin, 2017/01/11 00:50:06]

No, they can be different things. For example, if the SB blocking page is
triggered
by a bad sub-resources (e.g. an iframe, or a bad scrpt), the
unsafe_resources[0].url
is the url of this sub-resource, and the main_frame_url is the url of the top
level
page.

I added some comments to the SecurityInterstitialPage constructor to make it
clear
that it could be sub-resource URL.

[meacer, 2017/01/11 01:33:30]

Sounds good, thanks for updating SecurityInterstitialPage.

+      unsafe_resources_(unsafe_resources),
+      proceeded_(false) {
+  if (display_options) {
+    sb_error_ui_ = base::MakeUnique<SafeBrowsingErrorUI>(

[vakh (use Gerrit instead), 2017/01/10 20:44:26]

To avoid repetition, re-write as:

sb_error_ui_ = base::MakeUnique<SafeBrowsingErrorUI>(
    unsafe_resources_[0].url, main_frame_url_,
    GetInterstitialReason(unsafe_resources_),
    display_options ? *display_options :
CreateDefaultDisplayOptions(unsafe_resources),
    ui_manager_->app_locale(), base::Time::NowFromSystemTime(),
    controller());

[Jialiu Lin, 2017/01/11 00:50:05]

Done. Thanks!

+        unsafe_resources_[0].url, main_frame_url_,

[meacer, 2017/01/10 21:18:20]

Same here, would be nice to use only one of these.

[Jialiu Lin, 2017/01/11 00:50:06]

Similar to the above, they could be different URLs.

[meacer, 2017/01/11 01:33:30]

Acknowledged.

+        GetInterstitialReason(unsafe_resources_), *display_options,
+        ui_manager_->app_locale(), base::Time::NowFromSystemTime(),
+        controller());
+  } else {
+    sb_error_ui_ = base::MakeUnique<SafeBrowsingErrorUI>(
+        unsafe_resources_[0].url, main_frame_url_,
+        GetInterstitialReason(unsafe_resources_),
+        CreateDefaultDisplayOptions(unsafe_resources),
+        ui_manager_->app_locale(), base::Time::NowFromSystemTime(),
+        controller());

[meacer, 2017/01/10 21:18:21]

It could be slightly cleaner to create display_options if it's null, and then
call SafeBrowsingErrorUI constructor once.

Or a better approach is to pass |display_options| as const ref and let the
caller handle a missing display_options parameter. I think I like that better
since |display_options| is not an output parameter (the pointer implies that).
And then you can push the construction of sb_error_ui to the initializer list:

So something like:

BaseSafeBrowsingBlockingPage::BaseSafeBrowsingBlockingPage(
    BaseSafeBrowsingUIManager* ui_manager,
    ...
    const SBErrorDisplayOptions& display_options)
  : SecurityInterstitialPage(...),
    ui_manager_(ui_manager),
    main_frame_url_(main_frame_url),
    unsafe_resources_(unsafe_resources),
    sb_error_ui_(base::MakeUnique<SafeBrowsingErrorUI>(...),
    proceeded_(false) {
 ....
}

[Jialiu Lin, 2017/01/11 00:50:06]

Done.

+  }
+
+  if (!IsMainPageLoadBlocked(unsafe_resources)) {
+    navigation_entry_index_to_remove_ =
+        web_contents->GetController().GetLastCommittedEntryIndex();
+  } else {
+    navigation_entry_index_to_remove_ = -1;

[meacer, 2017/01/10 21:18:21]

nit: I wonder if navigation_entry_index_to_remove_ could be const int so that
it's also initialized in the parameter list? That way the constructor body would
be empty which I think makes for a slightly easier to read code (all parameters
are set once and never modified).

It looks like navigation_entry_index_to_remove_ is set to -1 in OnDontProceed
but I wonder if that's needed? Once we DontProceed, we the blocking page should
be destructed, right?

[Jialiu Lin, 2017/01/11 00:50:06]

You're right. Done.

+  }
+}
+
+BaseSafeBrowsingBlockingPage::~BaseSafeBrowsingBlockingPage() {}
+
+// static
+BaseSafeBrowsingBlockingPage* BaseSafeBrowsingBlockingPage::CreateBlockingPage(
+    BaseSafeBrowsingUIManager* ui_manager,
+    WebContents* web_contents,
+    const GURL& main_frame_url,
+    const UnsafeResource& unsafe_resource) {
+  std::vector<UnsafeResource> resources;

[vakh (use Gerrit instead), 2017/01/10 20:44:26]

This should work and avoid the explicit call to push_back:

std::vector<UnsafeResource> resources {resource};

[Jialiu Lin, 2017/01/11 00:50:05]

Done.

+  resources.push_back(unsafe_resource);

[meacer, 2017/01/10 21:18:20]

nit:

const std::vector<UnsafeResource> resources{unsafe_resource};

You could also inline this below.

[Jialiu Lin, 2017/01/11 00:50:06]

Done.

+  auto display_options = CreateDefaultDisplayOptions(resources);

[meacer, 2017/01/10 21:18:20]

If you decide to change display_options as a const ref, you can also inline
this. Otherwise it'll need to be changed to const auto& display_options = ...

[Jialiu Lin, 2017/01/11 00:50:06]

Changed CreateDefaultDisplayOptions(..) to return a SBErrorDisplayOptions*, so
it can be called inline.

+  return new BaseSafeBrowsingBlockingPage(
+      ui_manager, web_contents, main_frame_url, resources,
+      CreateControllerClient(
+          web_contents, resources, ui_manager->history_service(web_contents),
+          ui_manager->app_locale(), ui_manager->default_safe_page()),
+      &display_options);
+}
+
+// static
+void BaseSafeBrowsingBlockingPage::ShowBlockingPage(
+    BaseSafeBrowsingUIManager* ui_manager,
+    const UnsafeResource& unsafe_resource) {
+  DVLOG(1) << __func__ << " " << unsafe_resource.url.spec();

[vakh (use Gerrit instead), 2017/01/10 20:44:26]

Remove DVLOG?

[Jialiu Lin, 2017/01/11 00:50:05]

Done.

+  WebContents* web_contents = unsafe_resource.web_contents_getter.Run();
+
+  if (!InterstitialPage::GetInterstitialPage(web_contents) ||
+      !unsafe_resource.is_subresource) {
+    // There is no interstitial currently showing in that tab, or we are about
+    // to display a new one for the main frame. If there is already an
+    // interstitial, showing the new one will automatically hide the old one.
+    content::NavigationEntry* entry =
+        unsafe_resource.GetNavigationEntryForResource();
+    BaseSafeBrowsingBlockingPage* blocking_page =
+        CreateBlockingPage(ui_manager, web_contents,
+                           entry ? entry->GetURL() : GURL(), unsafe_resource);
+    blocking_page->Show();
+    return;
+  }
+
+  // This is an interstitial for a page's resource, let's queue it.
+  UnsafeResourceMap* unsafe_resource_map = GetUnsafeResourcesMap();
+  (*unsafe_resource_map)[web_contents].push_back(unsafe_resource);
+}
+
+// static
+bool BaseSafeBrowsingBlockingPage::IsMainPageLoadBlocked(
+    const UnsafeResourceList& unsafe_resources) {
+  // If there is more than one unsafe resource, the main page load must not be
+  // blocked. Otherwise, check if the one resource is.
+  return unsafe_resources.size() == 1 &&
+         unsafe_resources[0].IsMainPageLoadBlocked();
+}
+
+void BaseSafeBrowsingBlockingPage::OnProceed() {
+  proceeded_ = true;
+
+  ui_manager_->OnBlockingPageDone(unsafe_resources_, true, web_contents(),

[vakh (use Gerrit instead), 2017/01/10 20:44:26]

Add: /* proceed */

[Jialiu Lin, 2017/01/11 00:50:06]

Done.

+                                  main_frame_url_);
+}
+
+void BaseSafeBrowsingBlockingPage::OnDontProceed() {
+  // We could have already called Proceed(), in which case we must not notify
+  // the SafeBrowsingUIManager again, as the client has been deleted.
+  if (proceeded_)
+    return;
+
+  if (!sb_error_ui_->is_proceed_anyway_disabled()) {
+    controller()->metrics_helper()->RecordUserDecision(
+        security_interstitials::MetricsHelper::DONT_PROCEED);
+  }
+
+  // Send the malware details, if we opted to.
+  FinishThreatDetails(0, false /* did_proceed */,
+                      controller()->metrics_helper()->NumVisits());  // No delay
+
+  ui_manager_->OnBlockingPageDone(unsafe_resources_, false, web_contents(),

[vakh (use Gerrit instead), 2017/01/10 20:44:26]

Add: /* proceed */

[Jialiu Lin, 2017/01/11 00:50:05]

Done.

+                                  main_frame_url_);
+
+  // The user does not want to proceed, clear the queued unsafe resources
+  // notifications we received while the interstitial was showing.
+  UnsafeResourceMap* unsafe_resource_map = GetUnsafeResourcesMap();
+  UnsafeResourceMap::iterator iter = unsafe_resource_map->find(web_contents());
+  if (iter != unsafe_resource_map->end() && !iter->second.empty()) {
+    ui_manager_->OnBlockingPageDone(iter->second, false, web_contents(),
+                                    main_frame_url_);
+    unsafe_resource_map->erase(iter);
+  }
+
+  // We don't remove the navigation entry if the tab is being destroyed as this
+  // would trigger a navigation that would cause trouble as the render view host
+  // for the tab has by then already been destroyed.  We also don't delete the
+  // current entry if it has been committed again, which is possible on a page
+  // that had a subresource warning.
+  int last_committed_index =
+      web_contents()->GetController().GetLastCommittedEntryIndex();
+  if (navigation_entry_index_to_remove_ != -1 &&
+      navigation_entry_index_to_remove_ != last_committed_index &&
+      !web_contents()->IsBeingDestroyed()) {
+    CHECK(web_contents()->GetController().RemoveEntryAtIndex(
+        navigation_entry_index_to_remove_));
+    navigation_entry_index_to_remove_ = -1;

[meacer, 2017/01/10 21:18:21]

nit: As I mentioned above, I wonder if this is necessary? It may very well be, I
just don't know about the code that well to tell :)

[Jialiu Lin, 2017/01/11 00:50:05]

You're right. the final assignment of navigation_entry_index_to_remove_  is not
necessary. 

+  }
+}
+
+void BaseSafeBrowsingBlockingPage::CommandReceived(
+    const std::string& page_cmd) {
+  if (page_cmd == "\"pageLoadComplete\"") {
+    // content::WaitForRenderFrameReady sends this message when the page
+    // load completes. Ignore it.
+    return;
+  }
+
+  int command = 0;
+  bool retval = base::StringToInt(page_cmd, &command);
+  DCHECK(retval) << page_cmd;
+
+  sb_error_ui_->HandleCommand(
+      static_cast<security_interstitials::SecurityInterstitialCommands>(
+          command));
+}
+
+bool BaseSafeBrowsingBlockingPage::ShouldCreateNewNavigation() const {
+  return sb_error_ui_->is_main_frame_load_blocked();
+}
+
+void BaseSafeBrowsingBlockingPage::PopulateInterstitialStrings(
+    base::DictionaryValue* load_time_data) {
+  sb_error_ui_->PopulateStringsForHTML(load_time_data);
+}
+
+void BaseSafeBrowsingBlockingPage::FinishThreatDetails(int64_t delay_ms,
+                                                       bool did_proceed,
+                                                       int num_visits) {}
+
+// static
+BaseSafeBrowsingBlockingPage::UnsafeResourceMap*
+BaseSafeBrowsingBlockingPage::GetUnsafeResourcesMap() {
+  return g_unsafe_resource_map.Pointer();
+}
+
+
+// static
+std::string BaseSafeBrowsingBlockingPage::GetMetricPrefix(
+    const UnsafeResourceList& unsafe_resources,
+    SafeBrowsingErrorUI::SBInterstitialReason interstitial_reason) {
+  bool primary_subresource = unsafe_resources[0].is_subresource;
+  switch (interstitial_reason) {
+    case SafeBrowsingErrorUI::SB_REASON_MALWARE:
+      return primary_subresource ? "malware_subresource" : "malware";
+    case SafeBrowsingErrorUI::SB_REASON_HARMFUL:
+      return primary_subresource ? "harmful_subresource" : "harmful";
+    case SafeBrowsingErrorUI::SB_REASON_PHISHING:
+      ThreatPatternType threat_pattern_type =
+          unsafe_resources[0].threat_metadata.threat_pattern_type;
+      if (threat_pattern_type == ThreatPatternType::PHISHING ||
+          threat_pattern_type == ThreatPatternType::NONE)
+        return primary_subresource ? "phishing_subresource" : "phishing";
+      else if (threat_pattern_type == ThreatPatternType::SOCIAL_ENGINEERING_ADS)
+        return primary_subresource ? "social_engineering_ads_subresource"
+                                   : "social_engineering_ads";
+      else if (threat_pattern_type ==
+               ThreatPatternType::SOCIAL_ENGINEERING_LANDING)
+        return primary_subresource ? "social_engineering_landing_subresource"
+                                   : "social_engineering_landing";
+  }
+  NOTREACHED();
+  return "unkown_metric_prefix";
+}
+
+// We populate a parallel set of metrics to differentiate some threat sources.
+// static
+std::string BaseSafeBrowsingBlockingPage::GetExtraMetricsSuffix(
+    const UnsafeResourceList& unsafe_resources) {
+  switch (unsafe_resources[0].threat_source) {
+    case safe_browsing::ThreatSource::DATA_SAVER:
+      return "from_data_saver";
+    case safe_browsing::ThreatSource::REMOTE:
+    case safe_browsing::ThreatSource::LOCAL_PVER3:
+      // REMOTE and LOCAL_PVER3 can be distinguished in the logs
+      // by platform type: Remote is mobile, local_pver3 is desktop.
+      return "from_device";
+    case safe_browsing::ThreatSource::LOCAL_PVER4:
+      return "from_device_v4";
+    case safe_browsing::ThreatSource::CLIENT_SIDE_DETECTION:
+      return "from_client_side_detection";
+    case safe_browsing::ThreatSource::UNKNOWN:
+      break;
+  }
+  NOTREACHED();
+  return std::string();
+}
+
+// static
+SafeBrowsingErrorUI::SBInterstitialReason
+BaseSafeBrowsingBlockingPage::GetInterstitialReason(
+    const UnsafeResourceList& unsafe_resources) {
+  bool malware = false;
+  bool harmful = false;
+  bool phishing = false;
+  for (UnsafeResourceList::const_iterator iter = unsafe_resources.begin();
+       iter != unsafe_resources.end(); ++iter) {
+    const BaseSafeBrowsingUIManager::UnsafeResource& resource = *iter;
+    safe_browsing::SBThreatType threat_type = resource.threat_type;
+    if (threat_type == SB_THREAT_TYPE_URL_MALWARE ||
+        threat_type == SB_THREAT_TYPE_CLIENT_SIDE_MALWARE_URL) {
+      malware = true;

[vakh (use Gerrit instead), 2017/01/10 20:44:26]

You could shortcut the method here with: return
SafeBrowsingErrorUI::SB_REASON_MALWARE;

[Jialiu Lin, 2017/01/11 00:50:06]

You're right. I just copied it down without taking a closer look. Thanks!

+    } else if (threat_type == SB_THREAT_TYPE_URL_UNWANTED) {
+      harmful = true;
+    } else {
+      DCHECK(threat_type == SB_THREAT_TYPE_URL_PHISHING ||
+             threat_type == SB_THREAT_TYPE_CLIENT_SIDE_PHISHING_URL);
+      phishing = true;
+    }
+  }
+  DCHECK(phishing || malware || harmful);
+  if (malware)
+    return SafeBrowsingErrorUI::SB_REASON_MALWARE;
+  else if (harmful)
+    return SafeBrowsingErrorUI::SB_REASON_HARMFUL;
+  return SafeBrowsingErrorUI::SB_REASON_PHISHING;
+}
+
+// static
+std::unique_ptr<SecurityInterstitialControllerClient>
+BaseSafeBrowsingBlockingPage::CreateControllerClient(
+    content::WebContents* web_contents,
+    const UnsafeResourceList& unsafe_resources,
+    history::HistoryService* history_service,
+    const std::string& app_locale,
+    const GURL& default_safe_page) {
+  SafeBrowsingErrorUI::SBInterstitialReason interstitial_reason =
+      GetInterstitialReason(unsafe_resources);
+  GURL request_url(unsafe_resources[0].url);

[vakh (use Gerrit instead), 2017/01/10 20:44:26]

request_url is only being used in one place so we should not create that
temporary, just use unsafe_resources[0].url directly.

[Jialiu Lin, 2017/01/11 00:50:06]

Done.

+  security_interstitials::MetricsHelper::ReportDetails reporting_info;
+  reporting_info.metric_prefix =
+      GetMetricPrefix(unsafe_resources, interstitial_reason);
+  reporting_info.extra_suffix = GetExtraMetricsSuffix(unsafe_resources);
+
+  std::unique_ptr<security_interstitials::MetricsHelper> metrics_helper =
+      base::MakeUnique<security_interstitials::MetricsHelper>(
+          request_url, reporting_info, history_service);
+
+  return base::MakeUnique<SecurityInterstitialControllerClient>(
+      web_contents, std::move(metrics_helper), nullptr, app_locale,
+      default_safe_page);
+}
+
+// static
+const SafeBrowsingErrorUI::SBErrorDisplayOptions
+BaseSafeBrowsingBlockingPage::CreateDefaultDisplayOptions(
+    const UnsafeResourceList& unsafe_resources) {
+  SafeBrowsingErrorUI::SBErrorDisplayOptions display_options(
+      IsMainPageLoadBlocked(unsafe_resources),
+      true,    // kSafeBrowsingExtendedReportingOptInAllowed
+      false,   // is_off_the_record
+      false,   // is_extended_reporting
+      false,   // is_scout
+      false);  // kSafeBrowsingProceedAnywayDisabled
+  return display_options;

[vakh (use Gerrit instead), 2017/01/10 20:44:26]

directly return?

[Jialiu Lin, 2017/01/11 00:50:05]

Sure. Done.

+}
+
+}  // namespace safe_browsing