Reland "Use only SafeSearch filter for EduCoexistence webview."

This is a reland of b545f589
The original cl failed in the MSAN test in
SupervisedUserNavigationThrottleTest::
AllowEDUCoexistenceInnerWebContents
where the inner WebContents is not attached by the time it is expected to.
This only happens in the MSAN test case. The fix simply delays the test
until it is sure that the inner WebContent is attached.

Bug: 1113054

Original change's description:
> Use only SafeSearch filter for EduCoexistence webview.
>
> This cl fixes the bug where the child user is stuck
> in EDU account addition flow when the user's parental
> control setting is 'Only allow certain sites'.
>
> Inorder to prevent this error from happening we will do
> only safe search filter for the EduCoexistence webview
> and not enforce the 'Only allow certain cites' settings
> potentially set by the child's parents.
>
> "https://accounts.google.com" is manually whitelisted. This
> is because there is a race condition where we don't know
> whether the WebContent is an inner WebContent belonging
> to EduCoexistence flow or not by the time throttling happens.
> This happens because navigation to accounts.google.com is
> started before the <webview> WebContents is attached to
> the outer WebContents hosted by the EduCoexistence flow WebUI.
>
> We are making this exception only to the webview that is present in
> the EDUCoexistence flow. The exception is made because the parent is
> present with the child during the flow. Extending this exception
> to every inner webcontents will may allow the child to break parental
> settings.
>
> Bug: 1079510
> Change-Id: I4f3e65e58537a845613b0c49b03f801f0a3caa08
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2316443
> Reviewed-by: Marc Treib <treib@chromium.org>
> Reviewed-by: Aga Wronska <agawronska@chromium.org>
> Commit-Queue: Yilkal Abe <yilkal@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#794661}

Bug: 1079510
Change-Id: Ida69bb2fc63efbdbc3e06365290ddc2aed9790c3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2338856Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Aga Wronska <agawronska@chromium.org>
Commit-Queue: Yilkal Abe <yilkal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796898}

chrome/browser/supervised_user/supervised_user_navigation_observer.cc

@@ -86,7 +86,7 @@ void SupervisedUserNavigationObserver::UpdateMainFrameFilteringStatus(
 }
 
 void SupervisedUserNavigationObserver::DidFinishNavigation(
-      content::NavigationHandle* navigation_handle) {
+    content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->HasCommitted())
     return;
 
@@ -108,13 +108,15 @@ void SupervisedUserNavigationObserver::DidFinishNavigation(
     auto* render_frame_host = web_contents()->GetMainFrame();
     int process_id = render_frame_host->GetProcess()->GetID();
     int routing_id = render_frame_host->GetRoutingID();
-
+    bool skip_manual_parent_filter =
+        url_filter_->ShouldSkipParentManualAllowlistFiltering(web_contents());
     url_filter_->GetFilteringBehaviorForURLWithAsyncChecks(
         web_contents()->GetLastCommittedURL(),
         base::BindOnce(
             &SupervisedUserNavigationObserver::URLFilterCheckCallback,
             weak_ptr_factory_.GetWeakPtr(), navigation_handle->GetURL(),
-            process_id, routing_id));
+            process_id, routing_id),
+        skip_manual_parent_filter);
   }
 }
 
@@ -149,13 +151,15 @@ void SupervisedUserNavigationObserver::OnURLFilterChanged() {
   auto* main_frame = web_contents()->GetMainFrame();
   int main_frame_process_id = main_frame->GetProcess()->GetID();
   int routing_id = main_frame->GetRoutingID();
-
+  bool skip_manual_parent_filter =
+      url_filter_->ShouldSkipParentManualAllowlistFiltering(web_contents());
   url_filter_->GetFilteringBehaviorForURLWithAsyncChecks(
       web_contents()->GetLastCommittedURL(),
       base::BindOnce(&SupervisedUserNavigationObserver::URLFilterCheckCallback,
                      weak_ptr_factory_.GetWeakPtr(),
                      web_contents()->GetLastCommittedURL(),
-                     main_frame_process_id, routing_id));
+                     main_frame_process_id, routing_id),
+      skip_manual_parent_filter);
 
   MaybeUpdateRequestedHosts();
 
@@ -287,13 +291,15 @@ void SupervisedUserNavigationObserver::FilterRenderFrame(
     return;
 
   const GURL& last_committed_url = render_frame_host->GetLastCommittedURL();
-
+  bool skip_manual_parent_filter =
+      url_filter_->ShouldSkipParentManualAllowlistFiltering(web_contents());
   url_filter_->GetFilteringBehaviorForURLWithAsyncChecks(
       web_contents()->GetLastCommittedURL(),
       base::BindOnce(&SupervisedUserNavigationObserver::URLFilterCheckCallback,
                      weak_ptr_factory_.GetWeakPtr(), last_committed_url,
                      render_frame_host->GetProcess()->GetID(),
-                     render_frame_host->GetRoutingID()));
+                     render_frame_host->GetRoutingID()),
+      skip_manual_parent_filter);
 }
 
 void SupervisedUserNavigationObserver::GoBack() {

chrome/browser/supervised_user/supervised_user_navigation_throttle.cc

@@ -158,9 +158,15 @@ SupervisedUserNavigationThrottle::CheckURL() {
   deferred_ = false;
   DCHECK_EQ(SupervisedUserURLFilter::INVALID, behavior_);
   GURL url = navigation_handle()->GetURL();
+
+  bool skip_manual_parent_filter =
+      url_filter_->ShouldSkipParentManualAllowlistFiltering(
+          navigation_handle()->GetWebContents()->GetOutermostWebContents());
   bool got_result = url_filter_->GetFilteringBehaviorForURLWithAsyncChecks(
-      url, base::BindOnce(&SupervisedUserNavigationThrottle::OnCheckDone,
-                          weak_ptr_factory_.GetWeakPtr(), url));
+      url,
+      base::BindOnce(&SupervisedUserNavigationThrottle::OnCheckDone,
+                     weak_ptr_factory_.GetWeakPtr(), url),
+      skip_manual_parent_filter);
   DCHECK_EQ(got_result, behavior_ != SupervisedUserURLFilter::INVALID);
   // If we got a "not blocked" result synchronously, don't defer.
   deferred_ = !got_result || (behavior_ == SupervisedUserURLFilter::BLOCK);

chrome/browser/supervised_user/supervised_user_navigation_throttle_browsertest.cc

@@ -26,7 +26,9 @@
 #include "chrome/browser/supervised_user/supervised_user_url_filter.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/webui_url_constants.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
@@ -83,6 +85,40 @@ void RenderFrameTracker::FrameDeleted(content::RenderFrameHost* host) {
   render_frame_hosts_.erase(host->GetFrameTreeNodeId());
 }
 
+class InnerWebContentsAttachedWaiter : public content::WebContentsObserver {
+ public:
+  explicit InnerWebContentsAttachedWaiter(content::WebContents* contents)
+      : content::WebContentsObserver(contents) {}
+  InnerWebContentsAttachedWaiter(const InnerWebContentsAttachedWaiter&) =
+      delete;
+  InnerWebContentsAttachedWaiter& operator=(
+      const InnerWebContentsAttachedWaiter&) = delete;
+  ~InnerWebContentsAttachedWaiter() override = default;
+
+  // content::WebContentsObserver:
+  void InnerWebContentsAttached(content::WebContents* inner_web_contents,
+                                content::RenderFrameHost* render_frame_host,
+                                bool is_full_page) override;
+
+  void WaitForInnerWebContentsAttached();
+
+ private:
+  base::RunLoop run_loop_{base::RunLoop::Type::kNestableTasksAllowed};
+};
+
+void InnerWebContentsAttachedWaiter::InnerWebContentsAttached(
+    content::WebContents* inner_web_contents,
+    content::RenderFrameHost* render_frame_host,
+    bool is_full_page) {
+  run_loop_.Quit();
+}
+
+void InnerWebContentsAttachedWaiter::WaitForInnerWebContentsAttached() {
+  if (web_contents()->GetInnerWebContents().size() > 0u)
+    return;
+  run_loop_.Run();
+}
+
 }  // namespace
 
 class SupervisedUserNavigationThrottleTest
@@ -233,6 +269,42 @@ IN_PROC_BROWSER_TEST_F(SupervisedUserNavigationThrottleTest,
   EXPECT_TRUE(loaded2);
 }
 
+IN_PROC_BROWSER_TEST_F(SupervisedUserNavigationThrottleTest,
+                       AllowEDUCoexistenceInnerWebContents) {
+  BlockHost(kExampleHost2);
+  GURL manually_blocked_url = embedded_test_server()->GetURL(
+      kExampleHost2, "/supervised_user/with_iframes.html");
+
+  ui_test_utils::NavigateToURL(browser(),
+                               GURL(chrome::kChromeUIEDUCoexistenceLoginURL));
+  // Get the top level WebContents.
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_EQ(contents->GetURL(), GURL(chrome::kChromeUIEDUCoexistenceLoginURL));
+
+  InnerWebContentsAttachedWaiter web_contents_attached_waiter(
+      browser()->tab_strip_model()->GetActiveWebContents());
+  web_contents_attached_waiter.WaitForInnerWebContentsAttached();
+
+  // Get the inner WebContents.
+  std::vector<content::WebContents*> inner_web_contents =
+      contents->GetInnerWebContents();
+
+  // There is only one inner web content in EDUCoexistence flow.
+  EXPECT_EQ(inner_web_contents.size(), 1u);
+
+  content::WebContents* webview_element = inner_web_contents[0];
+  NavigationFinishedWaiter waiter(webview_element, manually_blocked_url);
+  webview_element->GetController().LoadURLWithParams(
+      NavigationController::LoadURLParams(manually_blocked_url));
+  waiter.Wait();
+
+  // Make sure that there is no error page in the inner web content.
+  EXPECT_NE(
+      webview_element->GetController().GetLastCommittedEntry()->GetPageType(),
+      content::PAGE_TYPE_ERROR);
+}
+
 class SupervisedUserIframeFilterTest
     : public SupervisedUserNavigationThrottleTest {
  protected:

chrome/browser/supervised_user/supervised_user_service.cc

@@ -583,6 +583,7 @@ void SupervisedUserService::OnDefaultFilteringBehaviorChanged() {
   SupervisedUserURLFilter::FilteringBehavior behavior =
       SupervisedUserURLFilter::BehaviorFromInt(behavior_value);
   url_filter_.SetDefaultFilteringBehavior(behavior);
+  UpdateAsyncUrlChecker();
 
   for (SupervisedUserServiceObserver& observer : observer_list_)
     observer.OnURLFilterChanged();
@@ -603,8 +604,19 @@ void SupervisedUserService::OnSafeSitesSettingChanged() {
     // Do nothing - we'll check the setting again when the load finishes.
   }
 
+  UpdateAsyncUrlChecker();
+}
+
+void SupervisedUserService::UpdateAsyncUrlChecker() {
+  int behavior_value = profile_->GetPrefs()->GetInteger(
+      prefs::kDefaultSupervisedUserFilteringBehavior);
+  SupervisedUserURLFilter::FilteringBehavior behavior =
+      SupervisedUserURLFilter::BehaviorFromInt(behavior_value);
+
   bool use_online_check =
-      supervised_users::IsSafeSitesOnlineCheckEnabled(profile_);
+      supervised_users::IsSafeSitesOnlineCheckEnabled(profile_) ||
+      behavior == SupervisedUserURLFilter::FilteringBehavior::BLOCK;
+
   if (use_online_check != url_filter_.HasAsyncURLChecker()) {
     if (use_online_check) {
       url_filter_.InitAsyncURLChecker(

chrome/browser/supervised_user/supervised_user_service.h

@@ -333,6 +333,8 @@ class SupervisedUserService : public KeyedService,
 
   void OnSafeSitesSettingChanged();
 
+  void UpdateAsyncUrlChecker();
+
   void OnSiteListsChanged(
       const std::vector<scoped_refptr<SupervisedUserSiteList>>& site_lists);
 

chrome/browser/supervised_user/supervised_user_url_filter.cc

@@ -18,6 +18,7 @@
 #include "base/macros.h"
 #include "base/no_destructor.h"
 #include "base/stl_util.h"
+#include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -28,11 +29,13 @@
 #include "chrome/browser/supervised_user/kids_management_url_checker_client.h"
 #include "chrome/browser/supervised_user/supervised_user_blacklist.h"
 #include "chrome/common/chrome_features.h"
+#include "chrome/common/webui_url_constants.h"
 #include "components/policy/core/browser/url_util.h"
 #include "components/url_formatter/url_formatter.h"
 #include "components/url_matcher/url_matcher.h"
 #include "components/variations/service/variations_service.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents.h"
 #include "extensions/buildflags/buildflags.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
@@ -108,6 +111,9 @@ const char kFamiliesUrl[] = "http://families.google.com/";
 const char kPlayStoreHost[] = "play.google.com";
 const char kPlayTermsPath[] = "/about/play-terms";
 
+// accounts.google.com used for login:
+const char kAccountsGoogleUrl[] = "https://accounts.google.com";
+
 // This class encapsulates all the state that is required during construction of
 // a new SupervisedUserURLFilter::Contents.
 class FilterBuilder {
@@ -224,6 +230,18 @@ SupervisedUserURLFilter::~SupervisedUserURLFilter() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
+// static
+bool SupervisedUserURLFilter::ShouldSkipParentManualAllowlistFiltering(
+    content::WebContents* contents) {
+  // Note that |contents| can be an inner WebContents. Get the outer most
+  // WebContents and check if it belongs to the EDUCoexistence login flow.
+  content::WebContents* outer_most_content =
+      contents->GetOutermostWebContents();
+
+  return outer_most_content->GetURL() ==
+         GURL(chrome::kChromeUIEDUCoexistenceLoginURL);
+}
+
 // static
 SupervisedUserURLFilter::FilteringBehavior
 SupervisedUserURLFilter::BehaviorFromInt(int behavior_value) {
@@ -344,10 +362,12 @@ SupervisedUserURLFilter::GetFilteringBehaviorForURL(
   }
 #endif
 
-  // Allow navigations to whitelisted origins (currently families.google.com).
+  // Allow navigations to whitelisted origins (currently families.google.com and
+  // accounts.google.com).
   static const base::NoDestructor<base::flat_set<GURL>> kWhitelistedOrigins(
       base::flat_set<GURL>({GURL(kFamiliesUrl).GetOrigin(),
-                            GURL(kFamiliesSecureUrl).GetOrigin()}));
+                            GURL(kFamiliesSecureUrl).GetOrigin(),
+                            GURL(kAccountsGoogleUrl).GetOrigin()}));
   if (base::Contains(*kWhitelistedOrigins, effective_url.GetOrigin()))
     return ALLOW;
 
@@ -445,17 +465,27 @@ SupervisedUserURLFilter::GetManualFilteringBehaviorForURL(
 
 bool SupervisedUserURLFilter::GetFilteringBehaviorForURLWithAsyncChecks(
     const GURL& url,
-    FilteringBehaviorCallback callback) const {
-  supervised_user_error_page::FilteringBehaviorReason reason =
-      supervised_user_error_page::DEFAULT;
-  FilteringBehavior behavior = GetFilteringBehaviorForURL(url, false, &reason);
-  // Any non-default reason trumps the async checker.
-  // Also, if we're blocking anyway, then there's no need to check it.
-  if (reason != supervised_user_error_page::DEFAULT || behavior == BLOCK ||
-      !async_url_checker_) {
-    std::move(callback).Run(behavior, reason, false);
-    for (Observer& observer : observers_)
-      observer.OnURLChecked(url, behavior, reason, false);
+    FilteringBehaviorCallback callback,
+    bool skip_manual_parent_filter) const {
+  if (!skip_manual_parent_filter) {
+    supervised_user_error_page::FilteringBehaviorReason reason =
+        supervised_user_error_page::DEFAULT;
+    FilteringBehavior behavior =
+        GetFilteringBehaviorForURL(url, false, &reason);
+    // Any non-default reason trumps the async checker.
+    // Also, if we're blocking anyway, then there's no need to check it.
+    if (reason != supervised_user_error_page::DEFAULT || behavior == BLOCK ||
+        !async_url_checker_) {
+      std::move(callback).Run(behavior, reason, false);
+      for (Observer& observer : observers_)
+        observer.OnURLChecked(url, behavior, reason, false);
+      return true;
+    }
+  }
+
+  if (!async_url_checker_) {
+    std::move(callback).Run(FilteringBehavior::ALLOW,
+                            supervised_user_error_page::DEFAULT, false);
     return true;
   }
 
@@ -611,11 +641,8 @@ void SupervisedUserURLFilter::CheckCallback(
     const GURL& url,
     safe_search_api::Classification classification,
     bool uncertain) const {
-  DCHECK(default_behavior_ != BLOCK);
-
   FilteringBehavior behavior =
       GetBehaviorFromSafeSearchClassification(classification);
-
   std::move(callback).Run(behavior, supervised_user_error_page::ASYNC_CHECKER,
                           uncertain);
   for (Observer& observer : observers_) {

chrome/browser/supervised_user/supervised_user_url_filter.h

@@ -27,6 +27,10 @@ namespace base {
 class TaskRunner;
 }
 
+namespace content {
+class WebContents;
+}  // namespace content
+
 namespace network {
 class SharedURLLoaderFactory;
 }  // namespace network
@@ -75,6 +79,11 @@ class SupervisedUserURLFilter {
   SupervisedUserURLFilter();
   ~SupervisedUserURLFilter();
 
+  // Returns true if the parental allowlist/blocklist should be skipped in
+  // |contents|. SafeSearch filtering is still applied to |contents|.
+  static bool ShouldSkipParentManualAllowlistFiltering(
+      content::WebContents* contents);
+
   static FilteringBehavior BehaviorFromInt(int behavior_value);
 
   static bool ReasonIsAutomatic(
@@ -115,10 +124,13 @@ class SupervisedUserURLFilter {
   // Like |GetFilteringBehaviorForURL|, but also includes asynchronous checks
   // against a remote service. If the result is already determined by the
   // synchronous checks, then |callback| will be called synchronously.
-  // Returns true if |callback| was called synchronously.
+  // Returns true if |callback| was called synchronously. If
+  // |skip_manual_parent_filter| is set to true, it only uses the asynchronous
+  // safe search checks.
   bool GetFilteringBehaviorForURLWithAsyncChecks(
       const GURL& url,
-      FilteringBehaviorCallback callback) const;
+      FilteringBehaviorCallback callback,
+      bool skip_manual_parent_filter = false) const;
 
   // Gets all the whitelists that the url is part of. Returns id->name of each
   // whitelist.

chrome/browser/supervised_user/supervised_user_url_filter_unittest.cc

@@ -73,10 +73,11 @@ class SupervisedUserURLFilterTest : public ::testing::Test,
   void ExpectURLCheckMatches(
       const std::string& url,
       SupervisedUserURLFilter::FilteringBehavior expected_behavior,
-      supervised_user_error_page::FilteringBehaviorReason expected_reason) {
+      supervised_user_error_page::FilteringBehaviorReason expected_reason,
+      bool skip_manual_parent_filter = false) {
     bool called_synchronously =
-        filter_.GetFilteringBehaviorForURLWithAsyncChecks(GURL(url),
-                                                          base::DoNothing());
+        filter_.GetFilteringBehaviorForURLWithAsyncChecks(
+            GURL(url), base::DoNothing(), skip_manual_parent_filter);
     ASSERT_TRUE(called_synchronously);
 
     EXPECT_EQ(behavior_, expected_behavior);

chrome/browser/ui/webui/signin/inline_login_dialog_chromeos.cc

@@ -86,11 +86,7 @@ GURL GetInlineLoginUrl(const std::string& email,
       source == InlineLoginDialogChromeOS::Source::kArc) {
     return GURL(chrome::kChromeUIAccountManagerErrorURL);
   }
-  DCHECK_EQ(std::string(chrome::kChromeUIChromeSigninURL).back(), '/');
-  // chrome://chrome-signin/edu
-  const std::string kEduAccountLoginURL =
-      std::string(chrome::kChromeUIChromeSigninURL) + "edu";
-  return GetUrlWithEmailParam(kEduAccountLoginURL, email);
+  return GetUrlWithEmailParam(chrome::kChromeUIEDUCoexistenceLoginURL, email);
 }
 
 }  // namespace

chrome/browser/ui/webui/supervised_user_internals_message_handler.cc

@@ -28,6 +28,7 @@
 #include "components/url_formatter/url_fixer.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/storage_partition.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 
 using content::BrowserThread;
@@ -181,12 +182,22 @@ void SupervisedUserInternalsMessageHandler::HandleTryURL(
     return;
 
   SupervisedUserURLFilter* filter = GetSupervisedUserService()->GetURLFilter();
+  content::WebContents* web_contents =
+      web_ui() ? web_ui()->GetWebContents() : nullptr;
+  bool skip_manual_parent_filter = false;
+  if (web_contents) {
+    skip_manual_parent_filter =
+        filter->ShouldSkipParentManualAllowlistFiltering(
+            web_contents->GetOutermostWebContents());
+  }
+
   std::map<std::string, base::string16> allowlists =
       filter->GetMatchingWhitelistTitles(url);
   filter->GetFilteringBehaviorForURLWithAsyncChecks(
       url,
       base::BindOnce(&SupervisedUserInternalsMessageHandler::OnTryURLResult,
-                     weak_factory_.GetWeakPtr(), allowlists));
+                     weak_factory_.GetWeakPtr(), allowlists),
+      skip_manual_parent_filter);
 }
 
 void SupervisedUserInternalsMessageHandler::SendBasicInfo() {

chrome/common/webui_url_constants.cc

@@ -68,6 +68,7 @@ const char kChromeUIDownloadInternalsHost[] = "download-internals";
 const char kChromeUIDownloadsHost[] = "downloads";
 const char kChromeUIDownloadsURL[] = "chrome://downloads/";
 const char kChromeUIDriveInternalsHost[] = "drive-internals";
+const char kChromeUIEDUCoexistenceLoginURL[] = "chrome://chrome-signin/edu";
 const char kChromeUIExtensionIconHost[] = "extension-icon";
 const char kChromeUIExtensionIconURL[] = "chrome://extension-icon/";
 const char kChromeUIExtensionsHost[] = "extensions";

chrome/common/webui_url_constants.h

@@ -71,6 +71,7 @@ extern const char kChromeUIDownloadInternalsHost[];
 extern const char kChromeUIDownloadsHost[];
 extern const char kChromeUIDownloadsURL[];
 extern const char kChromeUIDriveInternalsHost[];
+extern const char kChromeUIEDUCoexistenceLoginURL[];
 extern const char kChromeUIExtensionIconHost[];
 extern const char kChromeUIExtensionIconURL[];
 extern const char kChromeUIExtensionsHost[];