Watch for filtering pref changes in ManagedModeInterstitial and unblock requests if they become…

chrome/browser/managed_mode/managed_mode_interstitial.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/managed_mode/managed_mode_interstitial.h"
 
 #include "base/i18n/rtl.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/managed_mode/managed_user_service.h"
 #include "chrome/browser/managed_mode/managed_user_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/interstitial_page.h"
 #include "content/public/browser/web_contents.h"
-#include "content/public/browser/web_contents_delegate.h"
 #include "content/public/browser/web_ui.h"
 #include "grit/browser_resources.h"
 #include "grit/generated_resources.h"
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/webui/jstemplate_builder.h"
 #include "ui/webui/web_ui_util.h"
 
 using content::BrowserThread;
 
 ManagedModeInterstitial::ManagedModeInterstitial(
     content::WebContents* web_contents,
     const GURL& url,
     const base::Callback<void(bool)>& callback)
     : web_contents_(web_contents),
+      interstitial_page_(NULL),
       url_(url),
-      weak_ptr_factory_(this),
       callback_(callback) {
+  if (ShouldProceed()) {
+    // It can happen that the site was only allowed very recently and the URL
+    // filter on the IO thread had not been updated yet. Proceed with the
+    // request without showing the interstitial

[Pam (message me for reviews), 2013/09/04 09:56:59]

missing .

[Bernhard Bauer, 2013/09/04 11:48:35]

Done.

+    DispatchContinueRequest(true);
+    delete this;
+    return;
+  }
+
+  // TODO(bauerb): Extract an observer callback on ManagedUserService for this.
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
-  languages_ = profile->GetPrefs()->GetString(prefs::kAcceptLanguages);
+  PrefService* prefs = profile->GetPrefs();
+  pref_change_registrar_.Init(prefs);
+  pref_change_registrar_.Add(
+      prefs::kDefaultManagedModeFilteringBehavior,
+      base::Bind(&ManagedModeInterstitial::OnFilteringPrefsChanged,
+                 base::Unretained(this)));
+  pref_change_registrar_.Add(
+      prefs::kManagedModeManualHosts,
+      base::Bind(&ManagedModeInterstitial::OnFilteringPrefsChanged,
+                 base::Unretained(this)));
+  pref_change_registrar_.Add(
+      prefs::kManagedModeManualURLs,
+      base::Bind(&ManagedModeInterstitial::OnFilteringPrefsChanged,
+                 base::Unretained(this)));
 
+  languages_ = prefs->GetString(prefs::kAcceptLanguages);
   interstitial_page_ =
       content::InterstitialPage::Create(web_contents, true, url_, this);
   interstitial_page_->Show();
 }
 
 ManagedModeInterstitial::~ManagedModeInterstitial() {}
 
 std::string ManagedModeInterstitial::GetHTMLContents() {
   DictionaryValue strings;
   strings.SetString("blockPageTitle",
@@ skipping 64 matching lines @@
         ManagedUserServiceFactory::GetForProfile(profile);
     managed_user_service->AddAccessRequest(url_);
     DVLOG(1) << "Sent access request for " << url_.spec();
 
     return;
   }
 
   NOTREACHED();
 }
 
-void ManagedModeInterstitial::OnProceed() { NOTREACHED(); }
+void ManagedModeInterstitial::OnProceed() {
+  DCHECK(ShouldProceed());

[Pam (message me for reviews), 2013/09/04 09:56:59]

Should we just crash? I could imagine some situations where showing a blocked
page would be worse than crashing.

[Bernhard Bauer, 2013/09/04 11:48:35]

Well, this is really one of these "this cannot happen" situations. I feel like
purposely crashing would be trying to handle it, which you're not supposed to
do. 

[Pam (message me for reviews), 2013/09/05 10:13:52]

Keeping going somehow even though something unexpected has happened is
"handling" it. Crashing would be "not handling". I'm proposing changing the
DCHECK to a CHECK, on the grounds that showing a blocked site is kinda-sorta a
"security" problem.

[Bernhard Bauer, 2013/09/05 12:49:10]

What I meant by handling was that a CHECK results in production code that checks
this condition.
Fair enough. I changed it to a CHECK, and added a comment.

+  DispatchContinueRequest(true);
+}
 
 void ManagedModeInterstitial::OnDontProceed() {
   DispatchContinueRequest(false);
 }
 
+bool ManagedModeInterstitial::ShouldProceed() {
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents_->GetBrowserContext());
+  ManagedUserService* managed_user_service =
+      ManagedUserServiceFactory::GetForProfile(profile);
+  ManagedModeURLFilter* url_filter =
+      managed_user_service->GetURLFilterForUIThread();
+  return url_filter->GetFilteringBehaviorForURL(url_) !=
+         ManagedModeURLFilter::BLOCK;
+}
+
+void ManagedModeInterstitial::OnFilteringPrefsChanged() {
+  if (ShouldProceed())
+    interstitial_page_->Proceed();
+}
+
 void ManagedModeInterstitial::DispatchContinueRequest(bool continue_request) {
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE, base::Bind(callback_, continue_request));
 }