Fix managed mode allow/block flow

This patch fixes most of the problems with the allow/block flow:
 - removes got_user_gesture
 - uses the redirect list for URLs instead of recording them every time
 - better identifies when the user has clicked on a link and when the page
   was redirected.

R=bauerb@chromium.org
BUG=168772
TEST=browser_tests --gtest_filter=ManagedModeBlockModeTest.*


Review URL: https://chromiumcodereview.appspot.com/13533017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@192948 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/managed_mode/managed_mode_navigation_observer.cc

@@ -263,10 +263,10 @@ ManagedModeNavigationObserver::ManagedModeNavigationObserver(
     : WebContentsObserver(web_contents),
       warn_infobar_delegate_(NULL),
       preview_infobar_delegate_(NULL),
-      got_user_gesture_(false),
       state_(RECORDING_URLS_BEFORE_PREVIEW),
       is_elevated_(false),
-      last_allowed_page_(-1) {
+      last_allowed_page_(-1),
+      finished_redirects_(false) {
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
   managed_user_service_ = ManagedUserServiceFactory::GetForProfile(profile);
@@ -285,21 +285,7 @@ void ManagedModeNavigationObserver::AddTemporaryException() {
       base::Bind(&ManagedModeResourceThrottle::AddTemporaryException,
                  web_contents()->GetRenderProcessHost()->GetID(),
                  web_contents()->GetRenderViewHost()->GetRoutingID(),
-                 last_url_,
-                 got_user_gesture_));
-}
-
-void ManagedModeNavigationObserver::UpdateExceptionNavigationStatus() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(web_contents());
-
-  BrowserThread::PostTask(
-      BrowserThread::IO,
-      FROM_HERE,
-      base::Bind(&ManagedModeResourceThrottle::UpdateExceptionNavigationStatus,
-                 web_contents()->GetRenderProcessHost()->GetID(),
-                 web_contents()->GetRenderViewHost()->GetRoutingID(),
-                 got_user_gesture_));
+                 last_url_));
 }
 
 void ManagedModeNavigationObserver::RemoveTemporaryException() {
@@ -408,10 +394,17 @@ void ManagedModeNavigationObserver::NavigateToPendingEntry(
   // the user types a new URL. So do the main work in
   // ProvisionalChangeToMainFrameUrl and only check that the user didn't go
   // back to a blocked site here.
-  if (web_contents()->GetController().GetCurrentEntryIndex() <
-      last_allowed_page_) {
+  ManagedModeURLFilter::FilteringBehavior behavior =
+      url_filter_->GetFilteringBehaviorForURL(url);
+  int current_index = web_contents()->GetController().GetCurrentEntryIndex();
+  // First check that the user didn't go back to a blocked page, then check
+  // that the navigation is allowed.
+  if (last_allowed_page_ < 0 || current_index <= last_allowed_page_ ||
+      (behavior == ManagedModeURLFilter::BLOCK &&
+       !CanTemporarilyNavigateHost(url))) {
     ClearObserverState();
   }
+  finished_redirects_ = false;
 }
 
 void ManagedModeNavigationObserver::DidNavigateMainFrame(
@@ -419,30 +412,48 @@ void ManagedModeNavigationObserver::DidNavigateMainFrame(
     const content::FrameNavigateParams& params) {
   if (!ShouldStayElevatedForURL(params.url))
     is_elevated_ = false;
-
+  DVLOG(1) << "DidNavigateMainFrame " << params.url.spec();
+
+  // The page already loaded so we are not redirecting anymore, so the
+  // next call to ProvisionalChangeToMainFrameURL will probably be after a
+  // click.
+  // TODO(sergiu): One case where I think this fails is if we get a client-side
+  // redirect to a different domain as that will not have a temporary
+  // exception. See about that in the future.
+  finished_redirects_ = true;
   content::RecordAction(UserMetricsAction("ManagedMode_MainFrameNavigation"));
 
   ManagedModeURLFilter::FilteringBehavior behavior =
       url_filter_->GetFilteringBehaviorForURL(params.url);
 
+  if (behavior == ManagedModeURLFilter::ALLOW) {
+    // Update the last allowed page so that we can go back to it.
+    last_allowed_page_ = web_contents()->GetController().GetCurrentEntryIndex();
+  }
+
   UMA_HISTOGRAM_ENUMERATION("ManagedMode.FilteringBehavior",
                             behavior,
                             ManagedModeURLFilter::HISTOGRAM_BOUNDING_VALUE);
 
-  // The page can be redirected to a different domain, record those URLs as
-  // well.
-  if (behavior == ManagedModeURLFilter::BLOCK &&
-      !CanTemporarilyNavigateHost(params.url))
-    AddURLToPatternList(params.url);
+  // Record all the URLs this navigation went through.
+  if ((behavior == ManagedModeURLFilter::ALLOW &&
+       state_ == RECORDING_URLS_AFTER_PREVIEW) ||
+      !CanTemporarilyNavigateHost(params.url)) {
+    for (std::vector<GURL>::const_iterator it = params.redirects.begin();
+         it != params.redirects.end(); ++it) {
+      DVLOG(1) << "Adding: " << it->spec();
+      AddURLToPatternList(*it);
+    }
+  }
 
   if (behavior == ManagedModeURLFilter::ALLOW &&
       state_ == RECORDING_URLS_AFTER_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.
-    // Update the |last_url_| since it was not added to the list before.
-    last_url_ = params.url;
     AddSavedURLsToWhitelistAndClearState();
+    // This page is now allowed so save the index as well.
+    last_allowed_page_ = web_contents()->GetController().GetCurrentEntryIndex();
     SimpleAlertInfoBarDelegate::Create(
         InfoBarService::FromWebContents(web_contents()),
         NULL,
@@ -456,10 +467,6 @@ void ManagedModeNavigationObserver::DidNavigateMainFrame(
   if (state_ == RECORDING_URLS_AFTER_PREVIEW) {
     AddTemporaryException();
   }
-
-  // The navigation is complete, unless there is a redirect. So set the
-  // new navigation to false to detect user interaction.
-  got_user_gesture_ = false;
 }
 
 void ManagedModeNavigationObserver::ProvisionalChangeToMainFrameUrl(
@@ -471,21 +478,15 @@ void ManagedModeNavigationObserver::ProvisionalChangeToMainFrameUrl(
   // This function is the last one to be called before the resource throttle
   // shows the interstitial if the URL must be blocked.
   DVLOG(1) << "ProvisionalChangeToMainFrameURL " << url.spec();
+
   ManagedModeURLFilter::FilteringBehavior behavior =
       url_filter_->GetFilteringBehaviorForURL(url);
-
   if (behavior != ManagedModeURLFilter::BLOCK)
     return;
 
-  if (state_ == RECORDING_URLS_AFTER_PREVIEW && got_user_gesture_ &&
+  if (finished_redirects_ && state_ == RECORDING_URLS_AFTER_PREVIEW &&
       !CanTemporarilyNavigateHost(url))
     ClearObserverState();
-
-  if (behavior == ManagedModeURLFilter::BLOCK &&
-      !CanTemporarilyNavigateHost(url))
-    AddURLToPatternList(url);
-
-  got_user_gesture_ = false;
 }
 
 void ManagedModeNavigationObserver::DidCommitProvisionalLoadForFrame(
@@ -523,14 +524,4 @@ void ManagedModeNavigationObserver::DidCommitProvisionalLoadForFrame(
           InfoBarService::FromWebContents(web_contents()));
     }
   }
-
-  if (behavior == ManagedModeURLFilter::ALLOW)
-    last_allowed_page_ = web_contents()->GetController().GetCurrentEntryIndex();
-}
-
-void ManagedModeNavigationObserver::DidGetUserGesture() {
-  got_user_gesture_ = true;
-  // Update the exception status so that the resource throttle knows that
-  // there was a manual navigation.
-  UpdateExceptionNavigationStatus();
 }

chrome/browser/managed_mode/managed_mode_navigation_observer.h

@@ -69,8 +69,6 @@ class ManagedModeNavigationObserver
   // an interstitial for this RenderView. This allows the user to navigate
   // around on the website after clicking preview.
   void AddTemporaryException();
-  // Updates the ResourceThrottle with the latest user navigation status.
-  void UpdateExceptionNavigationStatus();
   void RemoveTemporaryException();
 
   void AddURLToPatternList(const GURL& url);
@@ -102,7 +100,6 @@ class ManagedModeNavigationObserver
       const GURL& url,
       content::PageTransition transition_type,
       content::RenderViewHost* render_view_host) OVERRIDE;
-  virtual void DidGetUserGesture() OVERRIDE;
 
   // Returns whether the user would stay in elevated state if he visits this
   // URL.
@@ -118,12 +115,6 @@ class ManagedModeNavigationObserver
   InfoBarDelegate* warn_infobar_delegate_;
   InfoBarDelegate* preview_infobar_delegate_;
 
-  // Whether we received a user gesture.
-  // The goal is to allow automatic redirects (in order not to break the flow
-  // or show too many interstitials) while not allowing the user to navigate
-  // to blocked pages. We consider a redirect to be automatic if we did
-  // not get a user gesture.
-  bool got_user_gesture_;
   ObserverState state_;
   std::set<GURL> navigated_urls_;
   GURL last_url_;
@@ -134,6 +125,16 @@ class ManagedModeNavigationObserver
 
   int last_allowed_page_;
 
+  // There are two starting points for a new navigation:
+  // 1. NavigateToPendingEntry when the omnibox is used to navigate to a URL or
+  //    the user goes back or forward.
+  // 2. ProvisionalChangeToMainFrameURL when the user clicks on a link.
+  // The main problem is that ProvisionalChangeToMainFrameURL is called for
+  // redirects as well and we need a way to distinguish between the two
+  // scenarios. |finished_redirects_| helps us do that by tracking the cases
+  // when the user did not click on a URL.
+  bool finished_redirects_;
+
   DISALLOW_COPY_AND_ASSIGN(ManagedModeNavigationObserver);
 };
 

chrome/browser/managed_mode/managed_mode_resource_throttle.cc

@@ -29,19 +29,11 @@ struct WebContentsId {
   int render_view_id;
 };
 
-struct TemporaryExceptionData {
-  // Hostname for which the temporary exception was added.
-  std::string host;
-  // Whether the user initiated a new navigation or not.
-  bool new_navigation;
-};
-
 // This map contains <render_process_host_id_, render_view_id> pairs mapped
-// to |TemporaryExceptionData| which identifies individual tabs. If a
-// |TemporaryExceptionData| is present for a specific pair then the user
-// clicked preview, is navigating around and has not clicked one of the options
-// on the infobar.
-typedef std::map<WebContentsId, TemporaryExceptionData> PreviewMap;
+// to a host string which identifies individual tabs. If a host is present for
+// a specific pair then the user clicked preview, is navigating around and has
+// not clicked one of the options on the infobar.
+typedef std::map<WebContentsId, std::string> PreviewMap;
 base::LazyInstance<PreviewMap> g_in_preview_mode = LAZY_INSTANCE_INITIALIZER;
 
 }
@@ -66,27 +58,9 @@ ManagedModeResourceThrottle::~ManagedModeResourceThrottle() {}
 void ManagedModeResourceThrottle::AddTemporaryException(
     int render_process_host_id,
     int render_view_id,
-    const GURL& url,
-    bool new_navigation) {
-  TemporaryExceptionData data;
-  data.host = url.host();
-  data.new_navigation = new_navigation;
-  WebContentsId web_contents_id(render_process_host_id, render_view_id);
-  g_in_preview_mode.Get()[web_contents_id] = data;
-}
-
-// static
-void ManagedModeResourceThrottle::UpdateExceptionNavigationStatus(
-    int render_process_host_id,
-    int render_view_id,
-    bool new_navigation) {
-  PreviewMap* preview_map = g_in_preview_mode.Pointer();
+    const GURL& url) {
   WebContentsId web_contents_id(render_process_host_id, render_view_id);
-  PreviewMap::iterator it = preview_map->find(web_contents_id);
-  if (it == preview_map->end())
-    return;
-
-  it->second.new_navigation = new_navigation;
+  g_in_preview_mode.Get()[web_contents_id] = url.host();
 }
 
 // static
@@ -119,8 +93,7 @@ void ManagedModeResourceThrottle::ShowInterstitialIfNeeded(bool is_redirect,
 
   WebContentsId web_contents_id(render_process_host_id_, render_view_id_);
   PreviewMap::iterator it = preview_map->find(web_contents_id);
-  if (it != preview_map->end() &&
-      (!it->second.new_navigation || url.host() == it->second.host)) {
+  if (it != preview_map->end() && url.host() == it->second) {
     temporarily_allowed_ = true;
     RemoveTemporaryException(render_process_host_id_, render_view_id_);
     return;

chrome/browser/managed_mode/managed_mode_resource_throttle.h

@@ -32,11 +32,7 @@ class ManagedModeResourceThrottle : public content::ResourceThrottle {
   // details on the preview map.
   static void AddTemporaryException(int render_process_host_id,
                                     int render_view_id,
-                                    const GURL& url,
-                                    bool new_navigation);
-  static void UpdateExceptionNavigationStatus(int render_process_host_id,
-                                              int render_view_id,
-                                              bool new_navigation);
+                                    const GURL& url);
   static void RemoveTemporaryException(int render_process_host_id,
                                        int render_view_id);