Support manual (white|black)list, previewing and allowing after interstitial

chrome/browser/managed_mode/managed_mode_navigation_observer.cc

Index: chrome/browser/managed_mode/managed_mode_navigation_observer.cc
diff --git a/chrome/browser/managed_mode/managed_mode_navigation_observer.cc b/chrome/browser/managed_mode/managed_mode_navigation_observer.cc
index b9a49b26d5cd83c6f2511f2e03547d34926fba1d..1d21899893f614dfe4c6287b412989af1f84b215 100644
--- a/chrome/browser/managed_mode/managed_mode_navigation_observer.cc
+++ b/chrome/browser/managed_mode/managed_mode_navigation_observer.cc
@@ -4,7 +4,9 @@
 
 #include "chrome/browser/managed_mode/managed_mode_navigation_observer.h"
 
+#include "base/bind.h"
 #include "base/i18n/rtl.h"
+#include "base/string_number_conversions.h"
 #include "chrome/browser/api/infobars/simple_alert_infobar_delegate.h"
 #include "chrome/browser/infobars/infobar_tab_helper.h"
 #include "chrome/browser/managed_mode/managed_mode.h"
@@ -24,6 +26,10 @@
 
 namespace {
 
+bool IsInList(const ListValue *list, const std::string& url_to_add) {
+  return list->Find(*Value::CreateStringValue(url_to_add)) != list->end();

[Bernhard Bauer, 2012/11/16 15:04:46]

This leaks a StringValue. You could just iterate over the list and check the
entries, no?

[Sergiu, 2012/11/26 14:48:08]

Moved to managed_mode.cc and declared a StringValue there, that should be ok,
right?

+}
+
 class ManagedModeWarningInfobarDelegate : public ConfirmInfoBarDelegate {
  public:
   explicit ManagedModeWarningInfobarDelegate(
@@ -96,7 +102,80 @@ bool ManagedModeWarningInfobarDelegate::ShouldExpire(
 void ManagedModeWarningInfobarDelegate::InfoBarDismissed() {
   ManagedModeNavigationObserver* observer =
       ManagedModeNavigationObserver::FromWebContents(owner()->GetWebContents());
-  observer->InfobarDismissed();
+  observer->WarnInfobarDismissed();
+}
+
+class ManagedModePreviewInfobarDelegate : public ConfirmInfoBarDelegate {
+ public:
+  explicit ManagedModePreviewInfobarDelegate(
+      InfoBarTabHelper* infobar_tab_helper);
+
+ private:
+  virtual ~ManagedModePreviewInfobarDelegate();
+
+  // ConfirmInfoBarDelegate overrides:
+  virtual string16 GetMessageText() const OVERRIDE;
+  virtual int GetButtons() const OVERRIDE;
+  virtual string16 GetButtonLabel(InfoBarButton button) const OVERRIDE;
+  virtual bool Accept() OVERRIDE;
+  virtual bool Cancel() OVERRIDE;
+
+  // InfoBarDelegate override:
+  virtual bool ShouldExpire(
+      const content::LoadCommittedDetails& details) const OVERRIDE;
+  virtual void InfoBarDismissed() OVERRIDE;
+
+  DISALLOW_COPY_AND_ASSIGN(ManagedModePreviewInfobarDelegate);
+};
+
+ManagedModePreviewInfobarDelegate::ManagedModePreviewInfobarDelegate(
+    InfoBarTabHelper* infobar_tab_helper)
+    : ConfirmInfoBarDelegate(infobar_tab_helper) {}
+
+ManagedModePreviewInfobarDelegate::~ManagedModePreviewInfobarDelegate() {}
+
+string16 ManagedModePreviewInfobarDelegate::GetMessageText() const {
+  return l10n_util::GetStringUTF16(IDS_MANAGED_MODE_PREVIEW_MESSAGE);
+}
+
+int ManagedModePreviewInfobarDelegate::GetButtons() const {
+  return BUTTON_OK | BUTTON_CANCEL;
+}
+
+string16 ManagedModePreviewInfobarDelegate::GetButtonLabel(
+    InfoBarButton button) const {
+  return l10n_util::GetStringUTF16(
+      (button == BUTTON_OK) ? IDS_MANAGED_MODE_PREVIEW_ACCEPT
+                            : IDS_MANAGED_MODE_PREVIEW_CANCEL);
+}
+
+bool ManagedModePreviewInfobarDelegate::Accept() {
+  ManagedModeNavigationObserver* observer =
+      ManagedModeNavigationObserver::FromWebContents(
+          owner()->GetWebContents());
+  observer->AddURLList();
+  // Clear the pointer as the infobar was closed.

[Bernhard Bauer, 2012/11/16 15:04:46]

The ManagedModePreviewInfobarDelegate doesn't *really* know what the
ManagedModeNavigationObserver does, it just notifies it that it's being
dismissed.

+  observer->PreviewInfobarDismissed();
+
+  return true;
+}
+
+bool ManagedModePreviewInfobarDelegate::Cancel() {
+  // TODO(bauerb): Go back to the last page.
+  return false;
+}
+
+bool ManagedModePreviewInfobarDelegate::ShouldExpire(
+    const content::LoadCommittedDetails& details) const {
+  // ManagedModeNavigationObserver removes us below.
+  return false;
+}
+
+void ManagedModePreviewInfobarDelegate::InfoBarDismissed() {
+  ManagedModeNavigationObserver* observer =
+      ManagedModeNavigationObserver::FromWebContents(
+          owner()->GetWebContents());
+  observer->PreviewInfobarDismissed();
 }
 
 }  // namespace
@@ -109,23 +188,103 @@ ManagedModeNavigationObserver::ManagedModeNavigationObserver(
     content::WebContents* web_contents)
     : WebContentsObserver(web_contents),
       url_filter_(ManagedMode::GetURLFilterForUIThread()),
-      infobar_delegate_(NULL) {}
+      warn_infobar_delegate_(NULL),
+      preview_infobar_delegate_(NULL),
+      after_interstitial_(false) {}
+
+void ManagedModeNavigationObserver::WarnInfobarDismissed() {
+  DCHECK(warn_infobar_delegate_);
+  warn_infobar_delegate_ = NULL;
+}
+
+void ManagedModeNavigationObserver::PreviewInfobarDismissed() {
+  DCHECK(preview_infobar_delegate_);
+  preview_infobar_delegate_ = NULL;
+}
+
+void ManagedModeNavigationObserver::AddNavigatedURL(const GURL& url) {
+  if (std::find(navigated_urls_.begin(), navigated_urls_.end(), url) !=
+      navigated_urls_.end()) {
+    navigated_urls_.push_back(url);
+  }
+}
+
+void ManagedModeNavigationObserver::AddURLList() {
+  // Get a copy of the whitelist since it can't be edited in place.
+  // |whitelist| is the preference list while AddStringToManualWhitelist adds
+  // the navigated urls to the URL filter.

[Bernhard Bauer, 2012/11/16 15:04:46]

Why don't we do that stuff with one method? Where the whitelist/blacklist are
persisted should be an implementation detail of ManagedMode.

[Sergiu, 2012/11/26 14:48:08]

Moved in ManagedMode.

+  scoped_ptr<base::ListValue> whitelist(
+      ManagedMode::GetWhitelist()->DeepCopy());
+  std::string url_to_add;

[Bernhard Bauer, 2012/11/16 15:04:46]

Move this down where it's initialized? Also, it's not a URL, it's a URL pattern
(which doesn't parse as a URL, which is why I'm nitpicking about this).

+  int added_url_count = 0;
+
+  for (std::vector<GURL>::const_iterator it = navigated_urls_.begin();
+       it+1 != navigated_urls_.end(); ++it) {
+    url_to_add = it->spec();
+    if (!IsInList(ManagedMode::GetWhitelist().get(), url_to_add)) {
+      DLOG(ERROR) << "Adding (exact):" << url_to_add;
+      ManagedMode::AddStringToManualWhitelist(url_to_add);
+      whitelist->Append(Value::CreateStringValue(url_to_add));
+      ++added_url_count;
+    }
+  }
+
+  // If the URL uses https add the protocol as well instead of just the
+  // hostname.
+  if (navigated_urls_.back().SchemeIs("https")) {
+    url_to_add = navigated_urls_.back().GetOrigin().spec();
+  } else {
+    url_to_add = navigated_urls_.back().host();
+  }
 
-void ManagedModeNavigationObserver::InfobarDismissed() {
-  DCHECK(infobar_delegate_);
-  infobar_delegate_ = NULL;
+  // Use the local whitelist to see if this last URL is already there.
+  if (!IsInList(ManagedMode::GetWhitelist().get(), url_to_add)) {

[Bernhard Bauer, 2012/11/16 15:04:46]

Argh! Please don't ever do this. You create a scoped_ptr as a temporary, which
means that it will be destroyed. In this case you're safe because that will
happen after this statement has executed, but this is a pure recipe for
use-after-free. 

Do we even need to check the manual whitelist as stored in prefs? Couldn't we
just ask the URL filter?

[Sergiu, 2012/11/26 14:48:08]

Got it, refactored and as we discussed a bit, I check via the pref one because
that has the exact entries while the url filter only knows the final effect.

+    DLOG(ERROR) << "Adding (hostname): " << url_to_add;
+    ManagedMode::AddStringToManualWhitelist(url_to_add);
+    whitelist->Append(Value::CreateStringValue(url_to_add));
+    ++added_url_count;
+  } else {
+    // Tell the user that the site was already present in the whitelist.
+    InfoBarTabHelper* infobar_tab_helper =
+        InfoBarTabHelper::FromWebContents(web_contents());
+    infobar_tab_helper->AddInfoBar(new SimpleAlertInfoBarDelegate(
+        infobar_tab_helper,
+        NULL,
+        l10n_util::GetStringFUTF16(IDS_MANAGED_MODE_ALREADY_ADDED_MESSAGE,
+                                   base::IntToString16(added_url_count)),
+        true));
+  }
+
+  ManagedMode::SetWhitelist(whitelist.get());
 }
 
 void ManagedModeNavigationObserver::NavigateToPendingEntry(
     const GURL& url,
     content::NavigationController::ReloadType reload_type) {
   DLOG(ERROR) << "NavigateToPendingEntry: " << url;
+  // This means that a new navigation was instantiated and the data related to

[Bernhard Bauer, 2012/11/16 15:04:46]

What do you mean by "a new navigation was instantiated"? I think a navigation
"happens", but even that is oversimplifying it (navigation is Complicated, as
evidenced by the multitude of events here). When is this method called? When we
navigate somewhere completely different?

[Sergiu, 2012/11/26 14:48:08]

Gave an example in the .h.

+  // the list of URLs needs to be cleared.
+  navigated_urls_.clear();
+  after_interstitial_ = false;
 }
 
 void ManagedModeNavigationObserver::DidNavigateMainFrame(
     const content::LoadCommittedDetails& details,
     const content::FrameNavigateParams& params) {
   DLOG(ERROR) << "DidNavigateMainFrame: " << params.url;
+
+  ManagedModeURLFilter::FilteringBehavior behavior =
+      url_filter_->GetFilteringBehaviorForURL(params.url);
+
+  if (behavior != ManagedModeURLFilter::ALLOW)
+    AddNavigatedURL(params.url);
+
+  if (behavior == ManagedModeURLFilter::ALLOW && after_interstitial_) {
+    // The initial page that triggered the interstitial was blocked but the
+    // final page is already in the whitelist so add the series of URLs
+    // which lead to the final page to the whitelist as well.
+    AddURLList();
+  }
 }
 
 void ManagedModeNavigationObserver::DidStartProvisionalLoadForFrame(
@@ -146,6 +305,14 @@ void ManagedModeNavigationObserver::ProvisionalChangeToMainFrameUrl(
     const GURL& opener_url,
     content::RenderViewHost* render_view_host) {
   DLOG(ERROR) << "ProvisionalChangeToMainFrameUrl: " << url;
+  // Mark the fact that an interstitial will be triggered here if the URL
+  // must be blocked.

[Bernhard Bauer, 2012/11/16 15:04:46]

I'm not sure if I understand the interplay between the different navigation
events here. Can you explain what happens in what order?

[Sergiu, 2012/11/26 14:48:08]

Rephrased here and provided an example in the .h

+  ManagedModeURLFilter::FilteringBehavior behavior =
+      url_filter_->GetFilteringBehaviorForURL(url);
+  if (behavior == ManagedModeURLFilter::BLOCK)
+    after_interstitial_ = true;
+  if (behavior != ManagedModeURLFilter::ALLOW)
+    AddNavigatedURL(url);
 }
 
 void ManagedModeNavigationObserver::DidCommitProvisionalLoadForFrame(
@@ -158,26 +325,41 @@ void ManagedModeNavigationObserver::DidCommitProvisionalLoadForFrame(
     return;
 
   DLOG(ERROR) << "DidCommitProvisionalLoadForFrame: " << url;
-
   ManagedModeURLFilter::FilteringBehavior behavior =
       url_filter_->GetFilteringBehaviorForURL(url);
+
+  DLOG(ERROR) << "Current behavior: " << behavior;
   if (behavior == ManagedModeURLFilter::WARN) {
-    if (!infobar_delegate_) {
+    if (!warn_infobar_delegate_) {
       InfoBarTabHelper* infobar_tab_helper =
           InfoBarTabHelper::FromWebContents(web_contents());
-      infobar_delegate_ =
+      warn_infobar_delegate_ =
           new ManagedModeWarningInfobarDelegate(infobar_tab_helper);
-      infobar_tab_helper->AddInfoBar(infobar_delegate_);
+      infobar_tab_helper->AddInfoBar(warn_infobar_delegate_);
     }
   } else {
-    if (infobar_delegate_) {
+    if (warn_infobar_delegate_) {
       InfoBarTabHelper* infobar_tab_helper =
           InfoBarTabHelper::FromWebContents(web_contents());
-      infobar_tab_helper->RemoveInfoBar(infobar_delegate_);
-      infobar_delegate_= NULL;
+      infobar_tab_helper->RemoveInfoBar(warn_infobar_delegate_);
+      warn_infobar_delegate_= NULL;
     }
   }
 
-  // if (behavior == ManagedModeURLFilter::BLOCK)
-  //   new ManagedModeInterstitial(web_contents(), url);
+  if (behavior == ManagedModeURLFilter::BLOCK) {
+    if (!preview_infobar_delegate_) {
+      InfoBarTabHelper* infobar_tab_helper =
+          InfoBarTabHelper::FromWebContents(web_contents());
+      preview_infobar_delegate_ =
+          new ManagedModePreviewInfobarDelegate(infobar_tab_helper);
+      infobar_tab_helper->AddInfoBar(preview_infobar_delegate_);
+    }
+  } else {
+    if (preview_infobar_delegate_) {
+      InfoBarTabHelper* infobar_tab_helper =
+          InfoBarTabHelper::FromWebContents(web_contents());
+      infobar_tab_helper->RemoveInfoBar(preview_infobar_delegate_);
+      preview_infobar_delegate_= NULL;
+    }
+  }
 }