Make the policy fetch for first time login blocking

The CL makes policy fetching for first time login blocking for all users, except the ones that are known to be non-enterprise users.

BUG=334584

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

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

chrome/browser/chromeos/login/login_utils_browsertest.cc

@@ -528,49 +528,15 @@ class LoginUtilsTest : public testing::Test,
   DISALLOW_COPY_AND_ASSIGN(LoginUtilsTest);
 };
 
+struct LoginUtilsBlockingLoginTestParam {
+  const int steps;
+  const char* username;
+  const bool enroll_device;
+};
+
 class LoginUtilsBlockingLoginTest
     : public LoginUtilsTest,
-      public testing::WithParamInterface<int> {};
-
-TEST_F(LoginUtilsTest, NormalLoginDoesntBlock) {
-  UserManager* user_manager = UserManager::Get();
-  EXPECT_FALSE(user_manager->IsUserLoggedIn());
-  EXPECT_FALSE(connector_->IsEnterpriseManaged());
-  EXPECT_FALSE(prepared_profile_);
-  EXPECT_EQ(policy::USER_AFFILIATION_NONE,
-            connector_->GetUserAffiliation(kUsername));
-
-  // The profile will be created without waiting for a policy response.
-  PrepareProfile(kUsername);
-
-  EXPECT_TRUE(prepared_profile_);
-  ASSERT_TRUE(user_manager->IsUserLoggedIn());
-  EXPECT_EQ(kUsername, user_manager->GetLoggedInUser()->email());
-}
-
-TEST_F(LoginUtilsTest, EnterpriseLoginDoesntBlockForNormalUser) {
-  UserManager* user_manager = UserManager::Get();
-  EXPECT_FALSE(user_manager->IsUserLoggedIn());
-  EXPECT_FALSE(connector_->IsEnterpriseManaged());
-  EXPECT_FALSE(prepared_profile_);
-
-  // Enroll the device.
-  EnrollDevice(kUsername);
-
-  EXPECT_FALSE(user_manager->IsUserLoggedIn());
-  EXPECT_TRUE(connector_->IsEnterpriseManaged());
-  EXPECT_EQ(kDomain, connector_->GetEnterpriseDomain());
-  EXPECT_FALSE(prepared_profile_);
-  EXPECT_EQ(policy::USER_AFFILIATION_NONE,
-            connector_->GetUserAffiliation(kUsernameOtherDomain));
-
-  // Login with a non-enterprise user shouldn't block.
-  PrepareProfile(kUsernameOtherDomain);
-
-  EXPECT_TRUE(prepared_profile_);
-  ASSERT_TRUE(user_manager->IsUserLoggedIn());
-  EXPECT_EQ(kUsernameOtherDomain, user_manager->GetLoggedInUser()->email());
-}
+      public testing::WithParamInterface<LoginUtilsBlockingLoginTestParam> {};
 
 #if defined(ENABLE_RLZ)
 TEST_F(LoginUtilsTest, RlzInitialized) {
@@ -598,25 +564,26 @@ TEST_F(LoginUtilsTest, RlzInitialized) {
 }
 #endif
 
-TEST_P(LoginUtilsBlockingLoginTest, EnterpriseLoginBlocksForEnterpriseUser) {
+TEST_P(LoginUtilsBlockingLoginTest, LoginBlocksForUser) {
   UserManager* user_manager = UserManager::Get();
   EXPECT_FALSE(user_manager->IsUserLoggedIn());
   EXPECT_FALSE(connector_->IsEnterpriseManaged());
   EXPECT_FALSE(prepared_profile_);
 
-  // Enroll the device.
-  EnrollDevice(kUsername);
+  if (GetParam().enroll_device) {
+    // Enroll the device.
+    EnrollDevice(kUsername);
 
-  EXPECT_FALSE(user_manager->IsUserLoggedIn());
-  EXPECT_TRUE(connector_->IsEnterpriseManaged());
-  EXPECT_EQ(kDomain, connector_->GetEnterpriseDomain());
-  EXPECT_FALSE(prepared_profile_);
-  EXPECT_EQ(policy::USER_AFFILIATION_MANAGED,
-            connector_->GetUserAffiliation(kUsername));
-  EXPECT_FALSE(user_manager->IsKnownUser(kUsername));
+    EXPECT_FALSE(user_manager->IsUserLoggedIn());
+    EXPECT_TRUE(connector_->IsEnterpriseManaged());
+    EXPECT_EQ(kDomain, connector_->GetEnterpriseDomain());
+    EXPECT_FALSE(prepared_profile_);
+    EXPECT_EQ(policy::USER_AFFILIATION_MANAGED,
+              connector_->GetUserAffiliation(kUsername));
+    EXPECT_FALSE(user_manager->IsKnownUser(kUsername));
+  }
 
-  // Login with a user of the enterprise domain waits for policy.
-  PrepareProfile(kUsername);
+  PrepareProfile(GetParam().username);
 
   EXPECT_FALSE(prepared_profile_);
   ASSERT_TRUE(user_manager->IsUserLoggedIn());
@@ -628,7 +595,7 @@ TEST_P(LoginUtilsBlockingLoginTest, EnterpriseLoginBlocksForEnterpriseUser) {
   // |steps| is the test parameter, and is the number of successful fetches.
   // The first incomplete fetch will fail. In any case, the profile creation
   // should resume.
-  int steps = GetParam();
+  int steps = GetParam().steps;
 
   // The next expected fetcher ID. This is used to make it fail.
   int next_expected_fetcher_id = 0;
@@ -703,10 +670,29 @@ TEST_P(LoginUtilsBlockingLoginTest, EnterpriseLoginBlocksForEnterpriseUser) {
   EXPECT_TRUE(prepared_profile_);
 }
 
-INSTANTIATE_TEST_CASE_P(
-    LoginUtilsBlockingLoginTestInstance,
-    LoginUtilsBlockingLoginTest,
-    testing::Values(0, 1, 2, 3, 4, 5));
+const LoginUtilsBlockingLoginTestParam kBlockinLoginTestCases[] = {
+    {0, kUsername, true},
+    {1, kUsername, true},
+    {2, kUsername, true},
+    {3, kUsername, true},
+    {4, kUsername, true},
+    {5, kUsername, true},
+    {0, kUsername, false},
+    {1, kUsername, false},
+    {2, kUsername, false},
+    {3, kUsername, false},
+    {4, kUsername, false},
+    {5, kUsername, false},
+    {0, kUsernameOtherDomain, true},
+    {1, kUsernameOtherDomain, true},
+    {2, kUsernameOtherDomain, true},
+    {3, kUsernameOtherDomain, true},
+    {4, kUsernameOtherDomain, true},
+    {5, kUsernameOtherDomain, true}};
+
+INSTANTIATE_TEST_CASE_P(LoginUtilsBlockingLoginTestInstance,
+                        LoginUtilsBlockingLoginTest,
+                        testing::ValuesIn(kBlockinLoginTestCases));
 
 }  // namespace
 

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc

@@ -91,7 +91,7 @@ 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_) {
+  if (wait_for_policy_fetch_ && !initial_policy_fetch_timeout.is_max()) {
     policy_fetch_timeout_.Start(
         FROM_HERE,
         initial_policy_fetch_timeout,

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc

+// Copyright 2014 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 <string>
+
+#include "base/command_line.h"
+#include "base/compiler_specific.h"
+#include "base/file_util.h"
+#include "base/files/file_path.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/stringprintf.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/chromeos/login/test/oobe_base_test.h"
+#include "chrome/browser/chromeos/login/wizard_controller.h"
+#include "chrome/browser/policy/test/local_policy_test_server.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 "chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h"
+#include "components/policy/core/common/cloud/cloud_policy_constants.h"
+#include "components/policy/core/common/policy_switches.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/test/test_utils.h"
+#include "google_apis/gaia/fake_gaia.h"
+#include "google_apis/gaia/gaia_constants.h"
+#include "google_apis/gaia/gaia_urls.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace policy {
+
+namespace {
+
+const char kAccountId[] = "dla1@example.com";
+const char kAccountPassword[] = "letmein";
+const char* kStartupURLs[] = {"chrome://policy", "chrome://about"};
+const char kTestAuthCode[] = "fake-auth-code";
+const char kTestGaiaUberToken[] = "fake-uber-token";
+const char kTestAuthLoginAccessToken[] = "fake-access-token";
+const char kTestRefreshToken[] = "fake-refresh-token";
+const char kTestAuthSIDCookie[] = "fake-auth-SID-cookie";
+const char kTestAuthLSIDCookie[] = "fake-auth-LSID-cookie";
+const char kTestSessionSIDCookie[] = "fake-session-SID-cookie";
+const char kTestSessionLSIDCookie[] = "fake-session-LSID-cookie";
+const char kTestUserinfoToken[] = "fake-userinfo-token";
+
+}  // namespace
+
+class UserCloudPolicyManagerTest : public chromeos::OobeBaseTest {
+ protected:
+  UserCloudPolicyManagerTest() {
+    set_open_about_blank_on_browser_launch(false);
+  }
+
+  virtual ~UserCloudPolicyManagerTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    SetServerPolicy();
+
+    test_server_.reset(new LocalPolicyTestServer(policy_file_path()));
+    ASSERT_TRUE(test_server_->Start());
+
+    OobeBaseTest::SetUp();
+  }
+
+  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
+    command_line->AppendSwitchASCII(policy::switches::kDeviceManagementUrl,
+                                    test_server_->GetServiceURL().spec());
+    OobeBaseTest::SetUpCommandLine(command_line);
+  }
+
+  virtual void SetUpOnMainThread() OVERRIDE {
+    SetMergeSessionParams(kAccountId);
+    SetupGaiaServerWithAccessTokens();
+    OobeBaseTest::SetUpOnMainThread();
+  }
+
+  void SetupGaiaServerWithAccessTokens() {
+    FakeGaia::AccessTokenInfo token_info;
+    token_info.token = kTestUserinfoToken;
+    token_info.scopes.insert(GaiaConstants::kDeviceManagementServiceOAuth);
+    token_info.scopes.insert(GaiaConstants::kOAuthWrapBridgeUserInfoScope);
+    token_info.audience = GaiaUrls::GetInstance()->oauth2_chrome_client_id();
+    token_info.email = kAccountId;
+    fake_gaia_->IssueOAuthToken(kTestRefreshToken, token_info);
+  }
+
+  void SetMergeSessionParams(const std::string& email) {
+    FakeGaia::MergeSessionParams params;
+    params.auth_sid_cookie = kTestAuthSIDCookie;
+    params.auth_lsid_cookie = kTestAuthLSIDCookie;
+    params.auth_code = kTestAuthCode;
+    params.refresh_token = kTestRefreshToken;
+    params.access_token = kTestAuthLoginAccessToken;
+    params.gaia_uber_token = kTestGaiaUberToken;
+    params.session_sid_cookie = kTestSessionSIDCookie;
+    params.session_lsid_cookie = kTestSessionLSIDCookie;
+    params.email = email;
+    fake_gaia_->SetMergeSessionParams(params);
+  }
+
+  void SkipToLoginScreen() {
+    chromeos::WizardController::SkipPostLoginScreensForTesting();
+    chromeos::WizardController* wizard_controller =
+        chromeos::WizardController::default_controller();
+    ASSERT_TRUE(wizard_controller);
+    wizard_controller->SkipToLoginForTesting(chromeos::LoginScreenContext());
+
+    content::WindowedNotificationObserver(
+        chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE,
+        content::NotificationService::AllSources()).Wait();
+  }
+
+  void LogIn(const std::string& user_id, const std::string& password) {
+    GetLoginDisplay()->ShowSigninScreenForCreds(user_id, password);
+
+    content::WindowedNotificationObserver(
+        chrome::NOTIFICATION_SESSION_STARTED,
+        content::NotificationService::AllSources()).Wait();
+  }
+
+  void SetServerPolicy() {
+    const char kPolicy[] =
+        "{"
+        "  \"%s\": {"
+        "    \"mandatory\": {"
+        "      \"RestoreOnStartup\": 4,"
+        "      \"RestoreOnStartupURLs\": ["
+        "        \"chrome://policy\","
+        "        \"chrome://about\""
+        "      ]"
+        "    },"
+        "    \"recommended\": {}"
+        "  },"
+        "  \"managed_users\": [ \"*\" ],"
+        "  \"policy_user\": \"%s\","
+        "  \"current_key_index\": 0"
+        "}";
+
+    const std::string policy = base::StringPrintf(
+        kPolicy, dm_protocol::kChromeUserPolicyType, kAccountId);
+
+    const int bytes_written =
+        base::WriteFile(policy_file_path(), policy.data(), policy.size());
+    ASSERT_EQ(static_cast<int>(policy.size()), bytes_written);
+  }
+
+  base::FilePath policy_file_path() const {
+    return temp_dir_.path().AppendASCII("policy.json");
+  }
+
+  scoped_ptr<LocalPolicyTestServer> test_server_;
+
+  base::ScopedTempDir temp_dir_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(UserCloudPolicyManagerTest);
+};
+
+IN_PROC_BROWSER_TEST_F(UserCloudPolicyManagerTest, StartSession) {
+  SkipToLoginScreen();
+
+  LogIn(kAccountId, kAccountPassword);
+
+  // Check that the startup pages specified in policy were opened.
+  BrowserList* browser_list =
+      BrowserList::GetInstance(chrome::HOST_DESKTOP_TYPE_ASH);
+  EXPECT_EQ(1U, browser_list->size());
+  Browser* browser = browser_list->get(0);
+  ASSERT_TRUE(browser);
+
+  TabStripModel* tabs = browser->tab_strip_model();
+  ASSERT_TRUE(tabs);
+  const int expected_tab_count = static_cast<int>(arraysize(kStartupURLs));
+  EXPECT_EQ(expected_tab_count, tabs->count());
+  for (int i = 0; i < expected_tab_count && i < tabs->count(); ++i) {
+    EXPECT_EQ(GURL(kStartupURLs[i]),
+              tabs->GetWebContentsAt(i)->GetVisibleURL());
+  }
+}
+
+}  // namespace policy

chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc

@@ -17,6 +17,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/login/login_utils.h"
 #include "chrome/browser/chromeos/login/users/user.h"
+#include "chrome/browser/chromeos/login/users/user_manager.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/policy/user_cloud_external_data_manager.h"
 #include "chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h"
@@ -29,6 +30,7 @@
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/policy/core/browser/browser_policy_connector.h"
 #include "components/policy/core/common/cloud/cloud_external_data_manager.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
 #include "components/user_manager/user_type.h"
@@ -133,6 +135,7 @@ scoped_ptr<UserCloudPolicyManagerChromeOS>
   // policy registration.
   // USER_TYPE_PUBLIC_ACCOUNT gets its policy from the
   // DeviceLocalAccountPolicyService.
+  // Non-managed domains will be skipped by the below check
   const std::string& username = user->email();
   if (user->GetType() != user_manager::USER_TYPE_REGULAR ||
       BrowserPolicyConnector::IsNonEnterpriseUser(username)) {
@@ -142,10 +145,17 @@ scoped_ptr<UserCloudPolicyManagerChromeOS>
   policy::BrowserPolicyConnectorChromeOS* connector =
       g_browser_process->platform_part()->browser_policy_connector_chromeos();
   UserAffiliation affiliation = connector->GetUserAffiliation(username);
-  const bool is_managed_user = affiliation == USER_AFFILIATION_MANAGED;
+  const bool is_affiliated_user = affiliation == USER_AFFILIATION_MANAGED;
   const bool is_browser_restart =
       command_line->HasSwitch(chromeos::switches::kLoginUser);
-  const bool wait_for_initial_policy = is_managed_user && !is_browser_restart;
+  const bool wait_for_initial_policy =
+      !is_browser_restart &&
+      (chromeos::UserManager::Get()->IsCurrentUserNew() || is_affiliated_user);
+
+  const base::TimeDelta initial_policy_fetch_timeout =
+      chromeos::UserManager::Get()->IsCurrentUserNew()
+          ? base::TimeDelta::Max()
+          : base::TimeDelta::FromSeconds(kInitialPolicyFetchTimeoutSeconds);
 
   DeviceManagementService* device_management_service =
       connector->device_management_service();
@@ -195,7 +205,7 @@ scoped_ptr<UserCloudPolicyManagerChromeOS>
           external_data_manager.Pass(),
           component_policy_cache_dir,
           wait_for_initial_policy,
-          base::TimeDelta::FromSeconds(kInitialPolicyFetchTimeoutSeconds),
+          initial_policy_fetch_timeout,
           base::MessageLoopProxy::current(),
           file_task_runner,
           io_task_runner));

chrome/chrome_tests.gypi

@@ -1025,6 +1025,7 @@
         'browser/chromeos/policy/policy_cert_verifier_browsertest.cc',
         'browser/chromeos/policy/power_policy_browsertest.cc',
         'browser/chromeos/policy/user_cloud_external_data_manager_browsertest.cc',
+        'browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc',
         'browser/chromeos/policy/variations_service_policy_browsertest.cc',
         'browser/chromeos/power/peripheral_battery_observer_browsertest.cc',
         'browser/chromeos/preferences_browsertest.cc',

chrome/test/base/in_process_browser_test.cc

@@ -112,6 +112,7 @@ void SingleDesktopTestObserver::OnBrowserAdded(Browser* browser) {
 InProcessBrowserTest::InProcessBrowserTest()
     : browser_(NULL),
       exit_when_last_browser_closes_(true),
+      open_about_blank_on_browser_launch_(true),
       multi_desktop_test_(false)
 #if defined(OS_MACOSX)
       , autorelease_pool_(NULL)
@@ -248,7 +249,7 @@ void InProcessBrowserTest::PrepareTestCommandLine(CommandLine* command_line) {
   if (exit_when_last_browser_closes_)
     command_line->AppendSwitch(switches::kDisableZeroBrowsersOpenForTests);
 
-  if (command_line->GetArgs().empty())
+  if (open_about_blank_on_browser_launch_ && command_line->GetArgs().empty())
     command_line->AppendArg(url::kAboutBlankURL);
 }
 

chrome/test/base/in_process_browser_test.h

@@ -171,6 +171,10 @@ class InProcessBrowserTest : public content::BrowserTestBase {
     exit_when_last_browser_closes_ = value;
   }
 
+  void set_open_about_blank_on_browser_launch(bool value) {
+    open_about_blank_on_browser_launch_ = value;
+  }
+
   // This must be called before RunTestOnMainThreadLoop() to have any effect.
   void set_multi_desktop_test(bool multi_desktop_test) {
     multi_desktop_test_ = multi_desktop_test;
@@ -198,6 +202,9 @@ class InProcessBrowserTest : public content::BrowserTestBase {
   // True if we should exit the tests after the last browser instance closes.
   bool exit_when_last_browser_closes_;
 
+  // True if the about:blank tab should be opened when the browser is launched.
+  bool open_about_blank_on_browser_launch_;
+
   // True if this is a multi-desktop test (in which case this browser test will
   // not ensure that Browsers are only created on the tested desktop).
   bool multi_desktop_test_;