Add support for Active Directory to PreSigninPolicyFetcher

Support Active Directory policy in PreSigninPolicyFetcher:
Don't require a policy verification key for Active Directory.
Active Directory provided policy is not signed with any
verification key.

BUG=772372
TEST=unit_tests --gtest_filter=PreSigninPolicyFetcherTest*

Change-Id: I671713f890d1de0444f3fd462de061d8f18e373c
Reviewed-on: https://chromium-review.googlesource.com/707240
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510023}

chrome/browser/chromeos/login/existing_user_controller.cc

@@ -286,6 +286,14 @@ bool IsArcEnabledFromPolicy(
   return false;
 }
 
+// Returns true if the device is enrolled to an Active Directory domain
+// according to InstallAttributes (proxied through BrowserPolicyConnector).
+bool IsActiveDirectoryManaged() {
+  return g_browser_process->platform_part()
+      ->browser_policy_connector_chromeos()
+      ->IsActiveDirectoryManaged();
+}
+
 }  // namespace
 
 // static
@@ -548,9 +556,7 @@ void ExistingUserController::PerformLogin(
     login_performer_.reset(nullptr);
     login_performer_.reset(new ChromeLoginPerformer(this));
   }
-  policy::BrowserPolicyConnectorChromeOS* connector =
-      g_browser_process->platform_part()->browser_policy_connector_chromeos();
-  if (connector->IsActiveDirectoryManaged() &&
+  if (IsActiveDirectoryManaged() &&
       user_context.GetUserType() != user_manager::USER_TYPE_ACTIVE_DIRECTORY) {
     PerformLoginFinishedActions(false /* don't start auto login timer */);
     ShowError(IDS_LOGIN_ERROR_GOOGLE_ACCOUNT_NOT_ALLOWED,
@@ -1035,7 +1041,8 @@ void ExistingUserController::OnOldEncryptionDetected(
   pre_signin_policy_fetcher_ = base::MakeUnique<policy::PreSigninPolicyFetcher>(
       DBusThreadManager::Get()->GetCryptohomeClient(),
       DBusThreadManager::Get()->GetSessionManagerClient(),
-      std::move(cloud_policy_client), user_context.GetAccountId(),
+      std::move(cloud_policy_client), IsActiveDirectoryManaged(),
+      user_context.GetAccountId(),
       cryptohome::KeyDefinition(user_context.GetKey()->GetSecret(),
                                 std::string(), cryptohome::PRIV_DEFAULT));
   pre_signin_policy_fetcher_->FetchPolicy(

chrome/browser/chromeos/policy/pre_signin_policy_fetcher.cc

@@ -43,15 +43,20 @@ PreSigninPolicyFetcher::PreSigninPolicyFetcher(
     chromeos::CryptohomeClient* cryptohome_client,
     chromeos::SessionManagerClient* session_manager_client,
     std::unique_ptr<CloudPolicyClient> cloud_policy_client,
+    bool is_active_directory_managed,
     const AccountId& account_id,
     const cryptohome::KeyDefinition& auth_key)
     : cryptohome_client_(cryptohome_client),
       session_manager_client_(session_manager_client),
       cloud_policy_client_(std::move(cloud_policy_client)),
+      is_active_directory_managed_(is_active_directory_managed),
       account_id_(account_id),
       auth_key_(auth_key),
       task_runner_(base::CreateSequencedTaskRunnerWithTraits(kTaskTraits)),
-      weak_ptr_factory_(this) {}
+      weak_ptr_factory_(this) {
+  DCHECK(account_id_.GetAccountType() != AccountType::ACTIVE_DIRECTORY ||
+         is_active_directory_managed_);
+}
 
 PreSigninPolicyFetcher ::~PreSigninPolicyFetcher() {}
 
@@ -97,8 +102,10 @@ void PreSigninPolicyFetcher::OnMountTemporaryUserHome(
 void PreSigninPolicyFetcher::OnCachedPolicyRetrieved(
     const std::string& policy_blob,
     RetrievePolicyResponseType retrieve_policy_response) {
-  // We only need the cached policy key if there was policy.
-  if (!policy_blob.empty()) {
+  // We only need the cached policy key if there was policy and if the device is
+  // not joined to Active Directory (policy blobs from Active Directory servers
+  // are not signed).
+  if (!policy_blob.empty() && !is_active_directory_managed_) {
     base::FilePath policy_key_dir;
     CHECK(PathService::Get(chromeos::DIR_USER_POLICY_KEYS, &policy_key_dir));
     cached_policy_key_loader_ = base::MakeUnique<CachedPolicyKeyLoaderChromeOS>(
@@ -108,7 +115,8 @@ void PreSigninPolicyFetcher::OnCachedPolicyRetrieved(
         weak_ptr_factory_.GetWeakPtr(), policy_blob, retrieve_policy_response));
   } else {
     // Skip and pretend we've loaded policy key. We won't need it anyway,
-    // because there is no policy to validate.
+    // because there is no policy to validate or because it's not signed (Active
+    // Directory).
     OnPolicyKeyLoaded(policy_blob, retrieve_policy_response);
   }
 }
@@ -151,8 +159,10 @@ void PreSigninPolicyFetcher::OnUnmountTemporaryUserHome(
     return;
   }
 
-  // Before validating, check that we have a cached policy key.
-  if (cached_policy_key_loader_->cached_policy_key().empty()) {
+  // Before validating, check that we have a cached policy key. This does not
+  // apply to Active Directory joined devices (cached policy is unsigned there).
+  if (!is_active_directory_managed_ &&
+      cached_policy_key_loader_->cached_policy_key().empty()) {
     LOG(ERROR) << "No cached policy key loaded.";
     NotifyCallback(PolicyFetchResult::ERROR, nullptr);
     return;
@@ -175,7 +185,7 @@ void PreSigninPolicyFetcher::OnCachedPolicyValidated(
   policy_data_ = std::move(validator->policy_data());
   policy_payload_ = std::move(validator->payload());
 
-  if (account_id_.GetAccountType() == AccountType::ACTIVE_DIRECTORY) {
+  if (is_active_directory_managed_) {
     // For AD, we don't support fresh policy fetch at the moment. Simply exit
     // with cached policy.
     NotifyCallback(PolicyFetchResult::SUCCESS, std::move(policy_payload_));
@@ -284,7 +294,7 @@ PreSigninPolicyFetcher::CreateValidatorForCachedPolicy(
   validator->ValidatePolicyType(dm_protocol::kChromeUserPolicyType);
   validator->ValidatePayload();
 
-  if (account_id_.GetAccountType() != AccountType::ACTIVE_DIRECTORY) {
+  if (!is_active_directory_managed_) {
     // Also validate the user e-mail and the signature (except for authpolicy).
     validator->ValidateUsername(account_id_.GetUserEmail(), true);
     validator->ValidateSignature(
@@ -307,7 +317,7 @@ PreSigninPolicyFetcher::CreateValidatorForFetchedPolicy(
       CloudPolicyValidatorBase::DEVICE_ID_REQUIRED);
   validator->ValidatePayload();
 
-  if (account_id_.GetAccountType() != AccountType::ACTIVE_DIRECTORY) {
+  if (!is_active_directory_managed_) {
     // Also validate the signature.
     const std::string domain = gaia::ExtractDomainName(
         gaia::CanonicalizeEmail(account_id_.GetUserEmail()));

chrome/browser/chromeos/policy/pre_signin_policy_fetcher.h

@@ -71,6 +71,7 @@ class PreSigninPolicyFetcher : public CloudPolicyClient::Observer {
   PreSigninPolicyFetcher(chromeos::CryptohomeClient* cryptohome_client,
                          chromeos::SessionManagerClient* session_manager_client,
                          std::unique_ptr<CloudPolicyClient> cloud_policy_client,
+                         bool is_active_directory_managed,
                          const AccountId& account_id,
                          const cryptohome::KeyDefinition& auth_key);
   ~PreSigninPolicyFetcher() override;
@@ -133,6 +134,7 @@ class PreSigninPolicyFetcher : public CloudPolicyClient::Observer {
   chromeos::CryptohomeClient* const cryptohome_client_;
   chromeos::SessionManagerClient* const session_manager_client_;
   const std::unique_ptr<CloudPolicyClient> cloud_policy_client_;
+  const bool is_active_directory_managed_;
   const AccountId account_id_;
   const cryptohome::KeyDefinition auth_key_;
   scoped_refptr<base::SequencedTaskRunner> task_runner_;

chrome/browser/chromeos/policy/pre_signin_policy_fetcher_unittest.cc

@@ -47,9 +47,9 @@ namespace {
 const char kCachedHomepage[] = "http://cached.test";
 const char kFreshHomepage[] = "http://fresh.test";
 
-class PreSigninPolicyFetcherTest : public testing::Test {
+class PreSigninPolicyFetcherTestBase : public testing::Test {
  protected:
-  PreSigninPolicyFetcherTest() = default;
+  PreSigninPolicyFetcherTestBase() = default;
 
   void SetUp() override {
     // Setup mock HomedirMethods - this is used by PreSigninPolicyFetcher to
@@ -74,7 +74,8 @@ class PreSigninPolicyFetcherTest : public testing::Test {
     cloud_policy_client_ = cloud_policy_client.get();
     pre_signin_policy_fetcher_ = base::MakeUnique<PreSigninPolicyFetcher>(
         &cryptohome_client_, &session_manager_client_,
-        std::move(cloud_policy_client), account_id_, cryptohome_key_);
+        std::move(cloud_policy_client), IsActiveDirectoryManaged(),
+        GetAccountId(), cryptohome_key_);
     cached_policy_.payload().mutable_homepagelocation()->set_value(
         kCachedHomepage);
     cached_policy_.Build();
@@ -90,6 +91,17 @@ class PreSigninPolicyFetcherTest : public testing::Test {
     mock_homedir_methods_ = nullptr;
   }
 
+  // Returns true for Active Directory test, false otherwise.
+  virtual bool IsActiveDirectoryManaged() const = 0;
+
+  // Returns the AccountId to be used during the test. This will differ between
+  // regular gaia user and AD user.
+  virtual const AccountId& GetAccountId() const = 0;
+
+  cryptohome::Identification GetCryptohomeIdentification() const {
+    return cryptohome::Identification(GetAccountId());
+  }
+
   void StoreUserPolicyKey(const std::string& public_key) {
     ASSERT_TRUE(base::CreateDirectory(user_policy_key_file().DirName()));
     ASSERT_EQ(static_cast<int>(public_key.size()),
@@ -103,7 +115,8 @@ class PreSigninPolicyFetcherTest : public testing::Test {
 
   base::FilePath user_policy_key_file() const {
     const std::string sanitized_username =
-        chromeos::CryptohomeClient::GetStubSanitizedUsername(cryptohome_id_);
+        chromeos::CryptohomeClient::GetStubSanitizedUsername(
+            GetCryptohomeIdentification());
     return user_policy_keys_dir()
         .AppendASCII(sanitized_username)
         .AppendASCII("policy.pub");
@@ -113,7 +126,7 @@ class PreSigninPolicyFetcherTest : public testing::Test {
   // the passed |mount_error|.
   void ExpectTemporaryCryptohomeMount(cryptohome::MountError mount_error) {
     EXPECT_CALL(*mock_homedir_methods_,
-                MountEx(cryptohome::Identification(account_id_),
+                MountEx(GetCryptohomeIdentification(),
                         cryptohome::Authorization(cryptohome_key_), _, _))
         .WillOnce(WithArgs<2, 3>(Invoke(
             [mount_error](const cryptohome::MountRequest& mount_request,
@@ -175,7 +188,7 @@ class PreSigninPolicyFetcherTest : public testing::Test {
 
   void ExecuteFetchPolicy() {
     pre_signin_policy_fetcher_->FetchPolicy(
-        base::Bind(&PreSigninPolicyFetcherTest::OnPolicyRetrieved,
+        base::Bind(&PreSigninPolicyFetcherTestBase::OnPolicyRetrieved,
                    base::Unretained(this)));
     scoped_task_environment_.RunUntilIdle();
   }
@@ -187,10 +200,6 @@ class PreSigninPolicyFetcherTest : public testing::Test {
   chromeos::FakeSessionManagerClient session_manager_client_;
   UserPolicyBuilder cached_policy_;
   UserPolicyBuilder fresh_policy_;
-  const AccountId account_id_ =
-      AccountId::FromUserEmail(PolicyBuilder::kFakeUsername);
-  const cryptohome::Identification cryptohome_id_ =
-      cryptohome::Identification(account_id_);
   const cryptohome::KeyDefinition cryptohome_key_ =
       cryptohome::KeyDefinition("secret",
                                 std::string() /* label */,
@@ -208,7 +217,19 @@ class PreSigninPolicyFetcherTest : public testing::Test {
 
   bool expecting_fresh_policy_fetch_ = false;
 
-  DISALLOW_COPY_AND_ASSIGN(PreSigninPolicyFetcherTest);
+  DISALLOW_COPY_AND_ASSIGN(PreSigninPolicyFetcherTestBase);
+};
+
+// Tests for PreSigninPolicyFetcher with a regular gaia account.
+class PreSigninPolicyFetcherTest : public PreSigninPolicyFetcherTestBase {
+ protected:
+  bool IsActiveDirectoryManaged() const override { return false; }
+
+  const AccountId& GetAccountId() const override { return account_id_; }
+
+ private:
+  const AccountId account_id_ =
+      AccountId::FromUserEmail(PolicyBuilder::kFakeUsername);
 };
 
 // Test that we successfully determine that the user has no policy (unmanaged
@@ -218,8 +239,8 @@ TEST_F(PreSigninPolicyFetcherTest, NoPolicy) {
   ExpectTemporaryCryptohomeMount();
   // session_manager's RetrievePolicy* methods signal that there is no policy by
   // passing an empty string as policy blob.
-  session_manager_client_.set_user_policy_without_session(cryptohome_id_,
-                                                          std::string());
+  session_manager_client_.set_user_policy_without_session(
+      GetCryptohomeIdentification(), std::string());
 
   ExpectNoFreshPolicyFetchOnClient();
   ExecuteFetchPolicy();
@@ -257,7 +278,7 @@ TEST_F(PreSigninPolicyFetcherTest, CachedPolicyFailsToValidate) {
 
   ExpectTemporaryCryptohomeMount();
   session_manager_client_.set_user_policy_without_session(
-      cryptohome_id_, cached_policy_.GetBlob());
+      GetCryptohomeIdentification(), cached_policy_.GetBlob());
 
   ExpectNoFreshPolicyFetchOnClient();
   ExecuteFetchPolicy();
@@ -277,7 +298,7 @@ TEST_F(PreSigninPolicyFetcherTest, CachedPolicyFailsToValidate) {
 TEST_F(PreSigninPolicyFetcherTest, NoCachedPolicyKeyAccessible) {
   ExpectTemporaryCryptohomeMount();
   session_manager_client_.set_user_policy_without_session(
-      cryptohome_id_, cached_policy_.GetBlob());
+      GetCryptohomeIdentification(), cached_policy_.GetBlob());
 
   ExpectNoFreshPolicyFetchOnClient();
   ExecuteFetchPolicy();
@@ -299,7 +320,7 @@ TEST_F(PreSigninPolicyFetcherTest, FreshPolicyFetchFails) {
 
   ExpectTemporaryCryptohomeMount();
   session_manager_client_.set_user_policy_without_session(
-      cryptohome_id_, cached_policy_.GetBlob());
+      GetCryptohomeIdentification(), cached_policy_.GetBlob());
 
   ExpectFreshPolicyFetchOnClient(PolicyBuilder::kFakeToken,
                                  PolicyBuilder::kFakeDeviceId);
@@ -327,7 +348,7 @@ TEST_F(PreSigninPolicyFetcherTest, FreshPolicyFetchTimeout) {
 
   ExpectTemporaryCryptohomeMount();
   session_manager_client_.set_user_policy_without_session(
-      cryptohome_id_, cached_policy_.GetBlob());
+      GetCryptohomeIdentification(), cached_policy_.GetBlob());
 
   ExpectFreshPolicyFetchOnClient(PolicyBuilder::kFakeToken,
                                  PolicyBuilder::kFakeDeviceId);
@@ -356,7 +377,7 @@ TEST_F(PreSigninPolicyFetcherTest, FreshPolicyFetchFailsToValidate) {
 
   ExpectTemporaryCryptohomeMount();
   session_manager_client_.set_user_policy_without_session(
-      cryptohome_id_, cached_policy_.GetBlob());
+      GetCryptohomeIdentification(), cached_policy_.GetBlob());
 
   ExpectFreshPolicyFetchOnClient(PolicyBuilder::kFakeToken,
                                  PolicyBuilder::kFakeDeviceId);
@@ -391,7 +412,7 @@ TEST_F(PreSigninPolicyFetcherTest, FreshPolicyFetchSuccess) {
 
   ExpectTemporaryCryptohomeMount();
   session_manager_client_.set_user_policy_without_session(
-      cryptohome_id_, cached_policy_.GetBlob());
+      GetCryptohomeIdentification(), cached_policy_.GetBlob());
 
   ExpectFreshPolicyFetchOnClient(PolicyBuilder::kFakeToken,
                                  PolicyBuilder::kFakeDeviceId);
@@ -415,6 +436,37 @@ TEST_F(PreSigninPolicyFetcherTest, FreshPolicyFetchSuccess) {
             obtained_policy_payload_->homepagelocation().value());
 }
 
-}  // namespace
+// Tests for PreSigninPolicyFetcher with an Active Directory account.
+class PreSigninPolicyFetcherTestAD : public PreSigninPolicyFetcherTestBase {
+ protected:
+  bool IsActiveDirectoryManaged() const override { return true; }
+
+  const AccountId& GetAccountId() const override { return account_id_; }
 
+ private:
+  const AccountId account_id_ =
+      AccountId::AdFromUserEmailObjGuid(PolicyBuilder::kFakeUsername, "guid");
+};
+
+// For Active Directory, we only have unsigned cached policy. There is no policy
+// key and no fresh policy fetch is attempted currently.
+TEST_F(PreSigninPolicyFetcherTestAD, UnsignedCachedPolicyForActiveDirectory) {
+  ExpectTemporaryCryptohomeMount();
+  session_manager_client_.set_user_policy_without_session(
+      GetCryptohomeIdentification(), cached_policy_.GetBlob());
+
+  ExpectNoFreshPolicyFetchOnClient();
+  ExecuteFetchPolicy();
+
+  VerifyExpectationsOnClient();
+
+  EXPECT_TRUE(policy_retrieved_called_);
+  EXPECT_EQ(PreSigninPolicyFetcher::PolicyFetchResult::SUCCESS,
+            obtained_policy_fetch_result_);
+  EXPECT_TRUE(obtained_policy_payload_);
+  EXPECT_EQ(kCachedHomepage,
+            obtained_policy_payload_->homepagelocation().value());
+}
+
+}  // namespace
 }  // namespace policy