Reland: No longer start up profile if there was an error fetching pol…

…icy.

Changed UserCloudPolicyManagerChromeOS to no longer complete profile
initialization if there is an error fetching policy. Instead, we shutdown the
user and force a policy load the next time we try to startup that user.

BUG=532317
TBR=tnagel@chromium.org

Review URL: https://codereview.chromium.org/1735863002

Cr-Commit-Position: refs/heads/master@{#377558}

chrome/browser/chromeos/login/easy_unlock/bootstrap_browsertest.cc

@@ -28,7 +28,7 @@ namespace chromeos {
 namespace {
 
 const char kFakeGaiaId[] = "123456";
-const char kFakeUser[] = "test_user@example.com";
+const char kFakeUser[] = "test_user@consumer.example.com";
 const char kFakeSid[] = "fake-sid";
 const char kFakeLsid[] = "fake-lsid";
 const char kFakeRefreshToken[] = "fake-refresh-token";

chrome/browser/chromeos/login/saml/saml_browsertest.cc

@@ -317,6 +317,8 @@ class SamlTest : public OobeBaseTest {
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     command_line->AppendSwitch(switches::kOobeSkipPostLogin);
+    command_line->AppendSwitch(
+        chromeos::switches::kAllowFailedPolicyFetchForTest);
 
     const GURL gaia_url = gaia_https_forwarder_.GetURLForSSLHost("");
     const GURL saml_idp_url = saml_https_forwarder_.GetURLForSSLHost("SAML");

chrome/browser/chromeos/login/users/avatar/user_image_manager_browsertest.cc

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
@@ -44,6 +45,7 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chromeos/chromeos_paths.h"
+#include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/cryptohome_client.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_session_manager_client.h"
@@ -117,6 +119,16 @@ class UserImageManagerTest : public LoginManagerTest,
     ASSERT_TRUE(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir_));
   }
 
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    LoginManagerTest::SetUpCommandLine(command_line);
+    // These tests create new users and then inject policy after the fact,
+    // to avoid having to set up a mock policy server. UserCloudPolicyManager
+    // will shut down the profile if there's an error loading the initial
+    // policy, so disable this behavior so we can inject policy directly.
+    command_line->AppendSwitch(
+        chromeos::switches::kAllowFailedPolicyFetchForTest);
+  }
+
   void SetUpOnMainThread() override {
     LoginManagerTest::SetUpOnMainThread();
     local_state_ = g_browser_process->local_state();

chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_policy_browsertest.cc

@@ -184,6 +184,10 @@ class WallpaperManagerPolicyTest
     command_line->AppendSwitch(switches::kLoginManager);
     command_line->AppendSwitch(switches::kForceLoginManagerInTests);
 
+    // Allow policy fetches to fail - these tests instead invoke InjectPolicy()
+    // to directly inject and modify policy dynamically.
+    command_line->AppendSwitch(switches::kAllowFailedPolicyFetchForTest);
+
     LoginManagerTest::SetUpCommandLine(command_line);
   }
 

chrome/browser/chromeos/policy/affiliation_test_helper.cc

@@ -133,6 +133,11 @@ void LoginUser(const std::string& user_id) {
 void AppendCommandLineSwitchesForLoginManager(base::CommandLine* command_line) {
   command_line->AppendSwitch(chromeos::switches::kLoginManager);
   command_line->AppendSwitch(chromeos::switches::kForceLoginManagerInTests);
+  // LoginManager tests typically don't stand up a policy test server but
+  // instead inject policies directly through a SessionManagerClient. So allow
+  // policy fetches to fail - this is expected.
+  command_line->AppendSwitch(
+      chromeos::switches::kAllowFailedPolicyFetchForTest);
 }
 
 }  // namespace affiliation_test_helper

chrome/browser/chromeos/policy/blocking_login_browsertest.cc

@@ -81,6 +81,11 @@ struct BlockingLoginTestParam {
   const bool enroll_device;
 };
 
+// TODO(atwilson): This test is completely broken - it originally was built
+// when we made an entirely different set of network calls on startup. As a
+// result it generates random failures in startup network requests, then waits
+// to see if the profile finishes loading which is not at all what it is
+// intended to test. We need to fix this test or remove it (crbug.com/580537).
 class BlockingLoginTest
     : public OobeBaseTest,
       public content::NotificationObserver,
@@ -95,6 +100,9 @@ class BlockingLoginTest
     command_line->AppendSwitchASCII(
         policy::switches::kDeviceManagementUrl,
         embedded_test_server()->GetURL("/device_management").spec());
+
+    command_line->AppendSwitch(
+        chromeos::switches::kAllowFailedPolicyFetchForTest);
   }
 
   void SetUpOnMainThread() override {

chrome/browser/chromeos/policy/login_policy_test_base.h

@@ -34,6 +34,10 @@ class LoginPolicyTestBase : public chromeos::OobeBaseTest {
   virtual void GetMandatoryPoliciesValue(base::DictionaryValue* policy) const;
   virtual void GetRecommendedPoliciesValue(base::DictionaryValue* policy) const;
 
+  UserPolicyTestHelper* user_policy_helper() {
+    return user_policy_helper_.get();
+  }
+
   void SkipToLoginScreen();
   void LogIn(const std::string& user_id, const std::string& password);
 

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc

@@ -9,6 +9,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/command_line.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
@@ -24,13 +25,15 @@
 #include "chrome/browser/chromeos/policy/wildcard_login_checker.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/common/chrome_content_client.h"
+#include "chromeos/chromeos_switches.h"
 #include "components/policy/core/common/cloud/cloud_external_data_manager.h"
 #include "components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
 #include "components/policy/core/common/cloud/system_policy_request_context.h"
 #include "components/policy/core/common/policy_map.h"
 #include "components/policy/core/common/policy_pref_names.h"
 #include "components/policy/core/common/policy_types.h"
+#include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "policy/policy_constants.h"
@@ -99,7 +102,15 @@ UserCloudPolicyManagerChromeOS::UserCloudPolicyManagerChromeOS(
       wait_for_policy_fetch_(wait_for_policy_fetch),
       policy_fetch_timeout_(false, false) {
   time_init_started_ = base::Time::Now();
-  if (wait_for_policy_fetch_ && !initial_policy_fetch_timeout.is_max()) {
+
+  // Caller should pass a non-zero policy_fetch_timeout iff
+  // |wait_for_policy_fetch| is true.
+  DCHECK_NE(wait_for_policy_fetch_, initial_policy_fetch_timeout.is_zero());
+  allow_failed_policy_fetches_ =
+      base::CommandLine::ForCurrentProcess()->HasSwitch(
+          chromeos::switches::kAllowFailedPolicyFetchForTest) ||
+      !initial_policy_fetch_timeout.is_max();
+  if (wait_for_policy_fetch_ && allow_failed_policy_fetches_) {
     policy_fetch_timeout_.Start(
         FROM_HERE,
         initial_policy_fetch_timeout,
@@ -260,7 +271,7 @@ void UserCloudPolicyManagerChromeOS::OnRegistrationStateChanged(
     } else {
       // If the client has switched to not registered, we bail out as this
       // indicates the cloud policy setup flow has been aborted.
-      CancelWaitForPolicyFetch();
+      CancelWaitForPolicyFetch(true);
     }
   }
 }
@@ -272,7 +283,18 @@ void UserCloudPolicyManagerChromeOS::OnClientError(
     UMA_HISTOGRAM_SPARSE_SLOWLY(kUMAInitialFetchClientError,
                                 cloud_policy_client->status());
   }
-  CancelWaitForPolicyFetch();
+  switch (client()->status()) {
+    case DM_STATUS_SUCCESS:
+    case DM_STATUS_SERVICE_MANAGEMENT_NOT_SUPPORTED:
+      // If management is not supported for this user, then a registration
+      // error is to be expected.
+      CancelWaitForPolicyFetch(true);
+      break;
+    default:
+      // Unexpected error fetching policy.
+      CancelWaitForPolicyFetch(false);
+      break;
+  }
 }
 
 void UserCloudPolicyManagerChromeOS::OnComponentCloudPolicyUpdated() {
@@ -354,9 +376,6 @@ void UserCloudPolicyManagerChromeOS::OnOAuth2PolicyTokenFetched(
                        policy_token, std::string(), std::string(),
                        std::string());
   } else {
-    // Failed to get a token, stop waiting and use an empty policy.
-    CancelWaitForPolicyFetch();
-
     UMA_HISTOGRAM_ENUMERATION(kUMAInitialFetchOAuth2Error,
                               error.state(),
                               GoogleServiceAuthError::NUM_STATES);
@@ -366,6 +385,9 @@ void UserCloudPolicyManagerChromeOS::OnOAuth2PolicyTokenFetched(
       UMA_HISTOGRAM_SPARSE_SLOWLY(kUMAInitialFetchOAuth2NetworkError,
                                   -error.network_error());
     }
+    // Failed to get a token, stop waiting if policy is not required for this
+    // user.
+    CancelWaitForPolicyFetch(false);
   }
 
   token_fetcher_.reset();
@@ -378,23 +400,38 @@ void UserCloudPolicyManagerChromeOS::OnInitialPolicyFetchComplete(
                              now - time_client_registered_);
   UMA_HISTOGRAM_MEDIUM_TIMES(kUMAInitialFetchDelayTotal,
                              now - time_init_started_);
-  CancelWaitForPolicyFetch();
+  CancelWaitForPolicyFetch(success);
 }
 
 void UserCloudPolicyManagerChromeOS::OnBlockingFetchTimeout() {
   if (!wait_for_policy_fetch_)
     return;
-  LOG(WARNING) << "Timed out while waiting for the initial policy fetch. "
-               << "The first session will start without policy.";
-  CancelWaitForPolicyFetch();
+  LOG(WARNING) << "Timed out while waiting for the policy fetch. "
+               << "The session will start with the cached policy.";
+  CancelWaitForPolicyFetch(false);
 }
 
-void UserCloudPolicyManagerChromeOS::CancelWaitForPolicyFetch() {
+void UserCloudPolicyManagerChromeOS::CancelWaitForPolicyFetch(bool success) {
   if (!wait_for_policy_fetch_)
     return;
 
-  wait_for_policy_fetch_ = false;
   policy_fetch_timeout_.Stop();
+
+  // If there was an error, and we don't want to allow profile initialization
+  // to go forward after a failed policy fetch, then just return (profile
+  // initialization will not complete).
+  // TODO(atwilson): Add code to retry policy fetching.
+  if (!success && !allow_failed_policy_fetches_) {
+    LOG(ERROR) << "Policy fetch failed for "
+               << user_manager::UserManager::Get()->GetActiveUser()->email()
+               << " - aborting profile initialization";
+    // Need to exit the current user, because we've already started this user's
+    // session.
+    chrome::AttemptUserExit();
+    return;
+  }
+
+  wait_for_policy_fetch_ = false;
   CheckAndPublishPolicy();
   // Now that |wait_for_policy_fetch_| is guaranteed to be false, the scheduler
   // can be started.

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h

@@ -132,8 +132,10 @@ class UserCloudPolicyManagerChromeOS : public CloudPolicyManager,
 
   // Cancels waiting for the policy fetch and flags the
   // ConfigurationPolicyProvider ready (assuming all other initialization tasks
-  // have completed).
-  void CancelWaitForPolicyFetch();
+  // have completed). Pass |true| if policy fetch was successful (either
+  // because policy was successfully fetched, or if DMServer has notified us
+  // that the user is not managed).
+  void CancelWaitForPolicyFetch(bool success);
 
   void StartRefreshSchedulerIfReady();
 
@@ -153,6 +155,11 @@ class UserCloudPolicyManagerChromeOS : public CloudPolicyManager,
   // IsInitializationComplete().
   bool wait_for_policy_fetch_;
 
+  // Whether we should allow policy fetches to fail, or wait forever until they
+  // succeed (typically we won't allow them to fail until we have loaded policy
+  // at least once).
+  bool allow_failed_policy_fetches_;
+
   // A timer that puts a hard limit on the maximum time to wait for the initial
   // policy fetch.
   base::Timer policy_fetch_timeout_;

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc

@@ -7,14 +7,54 @@
 
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/run_loop.h"
 #include "base/values.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/chromeos/login/ui/webui_login_display.h"
 #include "chrome/browser/chromeos/policy/login_policy_test_base.h"
+#include "chrome/browser/chromeos/policy/user_policy_test_helper.h"
 #include "chrome/browser/prefs/session_startup_pref.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "components/user_manager/user.h"
+#include "components/user_manager/user_manager.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/notification_service.h"
 #include "policy/policy_constants.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+// Helper class that counts the number of notifications of the specified
+// type that have been received.
+class CountNotificationObserver : public content::NotificationObserver {
+ public:
+  CountNotificationObserver(int notification_type_to_count,
+                            const content::NotificationSource& source)
+      : notification_count_(0) {
+    registrar_.Add(this, notification_type_to_count, source);
+  }
+
+  // NotificationObserver:
+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) override {
+    // Count the number of notifications seen.
+    notification_count_++;
+  }
+
+  int notification_count() const { return notification_count_; }
+
+ private:
+  int notification_count_;
+  content::NotificationRegistrar registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(CountNotificationObserver);
+};
+
+}  // namespace
 
 namespace policy {
 
@@ -42,6 +82,12 @@ IN_PROC_BROWSER_TEST_F(UserCloudPolicyManagerTest, StartSession) {
   SkipToLoginScreen();
   LogIn(kAccountId, kAccountPassword);
 
+  // User should be marked as having a valid OAuth token.
+  const user_manager::UserManager* const user_manager =
+      user_manager::UserManager::Get();
+  EXPECT_EQ(user_manager::User::OAUTH2_TOKEN_STATUS_VALID,
+            user_manager->GetActiveUser()->oauth_token_status());
+
   // Check that the startup pages specified in policy were opened.
   BrowserList* browser_list = BrowserList::GetInstance();
   EXPECT_EQ(1U, browser_list->size());
@@ -58,4 +104,24 @@ IN_PROC_BROWSER_TEST_F(UserCloudPolicyManagerTest, StartSession) {
   }
 }
 
+IN_PROC_BROWSER_TEST_F(UserCloudPolicyManagerTest, ErrorLoadingPolicy) {
+  // Delete the policy file - this will cause a 500 error on policy requests.
+  user_policy_helper()->DeletePolicyFile();
+  SkipToLoginScreen();
+  CountNotificationObserver observer(
+      chrome::NOTIFICATION_SESSION_STARTED,
+      content::NotificationService::AllSources());
+  GetLoginDisplay()->ShowSigninScreenForCreds(kAccountId, kAccountPassword);
+  base::RunLoop().Run();
+  // Should not receive a SESSION_STARTED notification.
+  ASSERT_EQ(0, observer.notification_count());
+
+  // User should not be marked as having a valid OAuth token. That way, if we
+  // try to load the user in the future, we will attempt to load policy again.
+  const user_manager::UserManager* user_manager =
+      user_manager::UserManager::Get();
+  EXPECT_NE(user_manager::User::OAUTH2_TOKEN_STATUS_VALID,
+            user_manager->GetActiveUser()->oauth_token_status());
+}
+
 }  // namespace policy