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 4abf30f2afe845a4e4205ab37a4c1acd28551dad..aeb9af1deb372f3399d6532c9cd5cf69b58c623f 100644
--- a/chrome/browser/managed_mode/managed_mode_navigation_observer.cc
+++ b/chrome/browser/managed_mode/managed_mode_navigation_observer.cc
@@ -11,6 +11,7 @@
 #include "chrome/browser/infobars/infobar_tab_helper.h"
 #include "chrome/browser/managed_mode/managed_mode.h"
 #include "chrome/browser/managed_mode/managed_mode_interstitial.h"
+#include "chrome/browser/managed_mode/managed_mode_resource_throttle.h"
 #include "chrome/browser/managed_mode/managed_mode_url_filter.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -18,17 +19,18 @@
 #include "chrome/common/jstemplate_builder.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/common/frame_navigate_params.h"
 #include "grit/generated_resources.h"
 #include "grit/locale_settings.h"
 #include "ui/base/l10n/l10n_util.h"
 
-namespace {
+using content::BrowserThread;
 
-bool IsInList(const ListValue *list, const std::string& url_to_add) {
-  return list->Find(*Value::CreateStringValue(url_to_add)) != list->end();
-}
+namespace {
 
 class ManagedModeWarningInfobarDelegate : public ConfirmInfoBarDelegate {
  public:
@@ -157,22 +159,26 @@ string16 ManagedModePreviewInfobarDelegate::GetButtonLabel(
     InfoBarButton button) const {
   return l10n_util::GetStringUTF16(
       (button == BUTTON_OK) ? IDS_MANAGED_MODE_PREVIEW_ACCEPT
-                            : IDS_MANAGED_MODE_PREVIEW_CANCEL);
+                            : IDS_MANAGED_MODE_GO_BACK_ACTION);
 }
 
 bool ManagedModePreviewInfobarDelegate::Accept() {
   ManagedModeNavigationObserver* observer =
       ManagedModeNavigationObserver::FromWebContents(
           owner()->GetWebContents());
-  observer->AddURLList();
-  // Clear the pointer as the infobar was closed.
-  observer->PreviewInfobarDismissed();
+  observer->AddSavedURLsToWhitelist();
+  // Notify the navigation observer that the infobar was dismissed.
+  observer->ClearObserverState();
 
   return true;
 }
 
 bool ManagedModePreviewInfobarDelegate::Cancel() {
   // TODO(bauerb): Go back to the last page.
+  ManagedModeNavigationObserver* observer =
+      ManagedModeNavigationObserver::FromWebContents(
+          owner()->GetWebContents());
+  observer->ClearObserverState();
   return false;
 }
 
@@ -189,11 +195,23 @@ void ManagedModePreviewInfobarDelegate::InfoBarDismissed() {
   observer->PreviewInfobarDismissed();
 }
 
+// Taken from shill_manager_client.cc as ListValue returns a const_iterator
+// in its Find method.
+struct ValueEquals {
+  explicit ValueEquals(const Value* first) : first_(first) {}
+  bool operator()(const Value* second) const {
+    return first_->Equals(second);
+  }
+  const Value* first_;
+};
+
 }  // namespace
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(ManagedModeNavigationObserver)
 
-ManagedModeNavigationObserver::~ManagedModeNavigationObserver() {}
+ManagedModeNavigationObserver::~ManagedModeNavigationObserver() {
+  RemoveTemporaryException();
+}
 
 ManagedModeNavigationObserver::ManagedModeNavigationObserver(
     content::WebContents* web_contents)
@@ -201,9 +219,37 @@ ManagedModeNavigationObserver::ManagedModeNavigationObserver(
       url_filter_(ManagedMode::GetURLFilterForUIThread()),
       warn_infobar_delegate_(NULL),
       preview_infobar_delegate_(NULL),
-      after_interstitial_(false),
+      state_(RECORDING_URLS_BEFORE_PREVIEW),
       last_allowed_page_(-1) {}
 
+void ManagedModeNavigationObserver::AddTemporaryException() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(web_contents());
+
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&ManagedModeResourceThrottle::AddTemporaryException,
+                 web_contents()->GetRenderProcessHost()->GetID(),
+                 web_contents()->GetRenderViewHost()->GetRoutingID(),
+                 last_url_pattern_));
+}
+
+void ManagedModeNavigationObserver::RemoveTemporaryException() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // When closing the browser web_contents() may return NULL so guard against
+  // that.
+  if (!web_contents())
+    return;
+
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&ManagedModeResourceThrottle::RemoveTemporaryException,
+                 web_contents()->GetRenderProcessHost()->GetID(),
+                 web_contents()->GetRenderViewHost()->GetRoutingID()));
+}
+
 void ManagedModeNavigationObserver::WarnInfobarDismissed() {
   DCHECK(warn_infobar_delegate_);
   warn_infobar_delegate_ = NULL;
@@ -214,70 +260,74 @@ void ManagedModeNavigationObserver::PreviewInfobarDismissed() {
   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::AddSavedURLsToWhitelist() {
+  if (!url_patterns_.empty())
+    ManagedMode::AddToManualWhitelist(url_patterns_);
+  ListValue whitelist;
+  whitelist.AppendString(last_url_pattern_);

[Bernhard Bauer, 2012/12/03 10:23:37]

Append all the url_patterns_ to |whitelist|, then pass it to
ManagedMode::AddToManualWhitelist()?

[Sergiu, 2012/12/04 14:02:31]

Refactored with GURLs now.

+  ManagedMode::AddToManualWhitelist(whitelist);
 }
 
-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.
-  scoped_ptr<base::ListValue> whitelist(
-      ManagedMode::GetWhitelist()->DeepCopy());
-  std::string url_to_add;
-  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;
-    }
-  }
+void ManagedModeNavigationObserver::AddURLToPatternList(const GURL& url) {
+  DCHECK(state_ != NOT_RECORDING_URLS);
+
+  std::string current_url = url.scheme() + "://." + url.host();
+
+  url_patterns_.AppendIfNotPresent(new StringValue(current_url));
+}
 
-  // 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();
+void ManagedModeNavigationObserver::AddURLAsLastPattern(const GURL& url) {
+  DCHECK(state_ != NOT_RECORDING_URLS);
+
+  // Search for the last |url| to see if present in the |url_patterns_|

[Bernhard Bauer, 2012/12/03 10:23:37]

Nit: "[…] to see if it is present […]"

[Sergiu, 2012/12/04 14:02:31]

Done.

+  // and remove it from there if it is. |url| has to be converted to the
+  // |url_patterns_| format first. This stops us from having both
+  // http://.www.google.com (exact URL from the pattern list) and
+  // www.google.com (hostname from the last URL pattern) in the list.
+  base::StringValue value_to_find(url.scheme() + "://." + url.host());

[Bernhard Bauer, 2012/12/03 10:23:37]

It might be worth it to extract this to a method.

[Sergiu, 2012/12/04 14:02:31]

Refactored, see new version.

+  base::ListValue::iterator it =
+      std::find_if(url_patterns_.begin(), url_patterns_.end(),

[Bernhard Bauer, 2012/12/03 10:23:37]

I think this would be a lot easier if you'd store |url_patterns| as a
std::vector<std::string>; then you don't have to work around the interface of
base::ListValue.

[Sergiu, 2012/12/04 14:02:31]

Done, back to set<GURL> now which I think is better.

+                   ValueEquals(&value_to_find));
+  if (it != url_patterns_.end())
+    url_patterns_.Erase(it, NULL);
+
+  if (url.SchemeIs("https")) {
+    last_url_pattern_ = "https://" + url.host();
   } else {
-    url_to_add = navigated_urls_.back().host();
+    last_url_pattern_ = url.host();
   }
+}
 
-  // Use the local whitelist to see if this last URL is already there.
-  if (!IsInList(ManagedMode::GetWhitelist().get(), url_to_add)) {
-    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.
+void ManagedModeNavigationObserver::SetStateToRecordingAfterPreview() {
+  state_ = RECORDING_URLS_AFTER_PREVIEW;
+}
+
+void ManagedModeNavigationObserver::ClearObserverState() {
+  if (state_ == NOT_RECORDING_URLS && preview_infobar_delegate_) {
     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));
+    infobar_tab_helper->RemoveInfoBar(preview_infobar_delegate_);
+    preview_infobar_delegate_ = NULL;
   }
-
-  ManagedMode::SetWhitelist(whitelist.get());
+  url_patterns_.Clear();
+  last_url_pattern_ = "";
+  state_ = RECORDING_URLS_BEFORE_PREVIEW;
+  RemoveTemporaryException();
 }
 
 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
-  // the list of URLs needs to be cleared.
-  navigated_urls_.clear();
-  after_interstitial_ = false;
+
+  // This method gets called first when a user navigates to a (new) URL.
+  // This means that the data related to the list of URLs needs to be cleared
+  // in certain circumstances.
+  if (web_contents()->GetController().GetCurrentEntryIndex() <
+      last_allowed_page_ ||

[Bernhard Bauer, 2012/12/03 10:23:37]

Nit: Indent this four more spaces?

[Sergiu, 2012/12/04 14:02:31]

Done.

+      last_url_pattern_ != url.host()) {

[Bernhard Bauer, 2012/12/03 10:23:37]

If url is HTTPS, this check won't be correct, I think.

[Sergiu, 2012/12/04 14:02:31]

Nice catch, done.

+    ClearObserverState();
+  }
 }
 
 void ManagedModeNavigationObserver::DidNavigateMainFrame(
@@ -288,14 +338,31 @@ void ManagedModeNavigationObserver::DidNavigateMainFrame(
   ManagedModeURLFilter::FilteringBehavior behavior =
       url_filter_->GetFilteringBehaviorForURL(params.url);
 
-  if (behavior != ManagedModeURLFilter::ALLOW)
-    AddNavigatedURL(params.url);
+  // If the user just saw an interstitial this is the final URL so it is
+  // recorded. Checking for filtering behavior here isn't useful because
+  // although this specific URL can be allowed the hostname will be added which
+  // is more general. The hostname will be checked later when it is
+  // added to the actual whitelist to see if it is already present.
+  if (behavior == ManagedModeURLFilter::BLOCK && state_ != NOT_RECORDING_URLS)
+    AddURLAsLastPattern(params.url);
 
-  if (behavior == ManagedModeURLFilter::ALLOW && after_interstitial_) {
+  if (behavior == ManagedModeURLFilter::ALLOW &&
+      state_ != RECORDING_URLS_BEFORE_PREVIEW) {
     // 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();
+    AddSavedURLsToWhitelist();

[Bernhard Bauer, 2012/12/03 10:23:37]

Should we also reset the state here (or do that in AddSavedURLsToWhitelist())?

[Sergiu, 2012/12/04 14:02:31]

Done, clearing the state in the AddSavedTo[...]

+    InfoBarTabHelper* infobar_tab_helper =
+        InfoBarTabHelper::FromWebContents(web_contents());
+    infobar_tab_helper->AddInfoBar(new SimpleAlertInfoBarDelegate(
+        infobar_tab_helper,
+        NULL,
+        l10n_util::GetStringUTF16(IDS_MANAGED_MODE_ALREADY_ADDED_MESSAGE),
+        true));

[Bernhard Bauer, 2012/12/03 10:23:37]

Nit: I think doing an early-return in this case would be nicer, because this is
sort of a special case.

[Sergiu, 2012/12/04 14:02:31]

Done.

+  } else if (state_ == RECORDING_URLS_AFTER_PREVIEW) {
+    // Only add an exception when the final page is not allowed.

[Bernhard Bauer, 2012/12/03 10:23:37]

OK, I can infer that behavior != ALLOW if I think about it, but it would be nice
to explain that in a bit more detail.

[Sergiu, 2012/12/04 14:02:31]

Done.

+    state_ = NOT_RECORDING_URLS;
+    AddTemporaryException();
   }
 }
 
@@ -317,14 +384,16 @@ 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.
+  // This function is the last one to be called before the resource throttle
+  // shows the interstitial if the URL must be blocked.
   ManagedModeURLFilter::FilteringBehavior behavior =
       url_filter_->GetFilteringBehaviorForURL(url);
-  if (behavior == ManagedModeURLFilter::BLOCK)
-    after_interstitial_ = true;
-  if (behavior != ManagedModeURLFilter::ALLOW)
-    AddNavigatedURL(url);
+
+  if (state_ == NOT_RECORDING_URLS && last_url_pattern_ != url.host())
+    ClearObserverState();
+
+  if (behavior == ManagedModeURLFilter::BLOCK && state_ != NOT_RECORDING_URLS)
+    AddURLToPatternList(url);
 }
 
 void ManagedModeNavigationObserver::DidCommitProvisionalLoadForFrame(
@@ -340,7 +409,6 @@ void ManagedModeNavigationObserver::DidCommitProvisionalLoadForFrame(
   ManagedModeURLFilter::FilteringBehavior behavior =
       url_filter_->GetFilteringBehaviorForURL(url);
 
-  DLOG(ERROR) << "Current behavior: " << behavior;
   if (behavior == ManagedModeURLFilter::WARN) {
     if (!warn_infobar_delegate_) {
       InfoBarTabHelper* infobar_tab_helper =
@@ -355,12 +423,12 @@ void ManagedModeNavigationObserver::DidCommitProvisionalLoadForFrame(
       InfoBarTabHelper* infobar_tab_helper =
           InfoBarTabHelper::FromWebContents(web_contents());
       infobar_tab_helper->RemoveInfoBar(warn_infobar_delegate_);
-      warn_infobar_delegate_= NULL;
+      warn_infobar_delegate_ = NULL;
     }
-    last_allowed_page_ = web_contents()->GetController().GetCurrentEntryIndex();
   }
 
-  if (behavior == ManagedModeURLFilter::BLOCK) {
+  if (state_ != RECORDING_URLS_BEFORE_PREVIEW &&
+      behavior == ManagedModeURLFilter::BLOCK) {

[Bernhard Bauer, 2012/12/03 10:23:37]

Hm, we committed a load for a site that should be blocked. I think we should
DCHECK that we're in RECORDING_URLS_AFTER_PREVIEW state.

[Sergiu, 2012/12/04 14:02:31]

Actually checking that we're in AFTER_PREVIEW should be enough so I changed it
to that.

[Bernhard Bauer, 2012/12/04 14:25:00]

What I meant was that if we commit a navigation for a URL that should be
blocked, it can only happen if we are in the RECORDING_URLS_AFTER_PREVIEW state,
so we don't need to check that in production code, but we can make it a DCHECK
instead to verify this assumption.

[Sergiu, 2012/12/04 15:45:46]

Done.

     if (!preview_infobar_delegate_) {
       InfoBarTabHelper* infobar_tab_helper =
           InfoBarTabHelper::FromWebContents(web_contents());
@@ -368,12 +436,9 @@ void ManagedModeNavigationObserver::DidCommitProvisionalLoadForFrame(
           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;
-    }
+  }
+
+  if (behavior == ManagedModeURLFilter::ALLOW) {
+    last_allowed_page_ = web_contents()->GetController().GetCurrentEntryIndex();
   }
 }