Chromad: Prevent session from starting without policy

chrome/browser/chromeos/policy/active_directory_policy_manager.cc

Index: chrome/browser/chromeos/policy/active_directory_policy_manager.cc
diff --git a/chrome/browser/chromeos/policy/active_directory_policy_manager.cc b/chrome/browser/chromeos/policy/active_directory_policy_manager.cc
index 195140d9e972a2b2efc033267aa4707a4a941935..90776fa27b46c6bc67ded2245de8a5942152e3c4 100644
--- a/chrome/browser/chromeos/policy/active_directory_policy_manager.cc
+++ b/chrome/browser/chromeos/policy/active_directory_policy_manager.cc
@@ -31,17 +31,24 @@ ActiveDirectoryPolicyManager::~ActiveDirectoryPolicyManager() {}
 std::unique_ptr<ActiveDirectoryPolicyManager>
 ActiveDirectoryPolicyManager::CreateForDevicePolicy(
     std::unique_ptr<CloudPolicyStore> store) {
-  return base::WrapUnique(
-      new ActiveDirectoryPolicyManager(EmptyAccountId(), std::move(store)));
+  // Can't use MakeUnique<> because the constructor is private.
+  return base::WrapUnique(new ActiveDirectoryPolicyManager(
+      EmptyAccountId(), false, base::TimeDelta(), base::OnceClosure(),
+      std::move(store)));
 }
 
 // static
 std::unique_ptr<ActiveDirectoryPolicyManager>
 ActiveDirectoryPolicyManager::CreateForUserPolicy(
     const AccountId& account_id,
+    bool wait_for_policy_fetch,
+    base::TimeDelta initial_policy_fetch_timeout,
+    base::OnceClosure exit_session,
     std::unique_ptr<CloudPolicyStore> store) {
-  return base::WrapUnique(
-      new ActiveDirectoryPolicyManager(account_id, std::move(store)));
+  // Can't use MakeUnique<> because the constructor is private.
+  return base::WrapUnique(new ActiveDirectoryPolicyManager(
+      account_id, wait_for_policy_fetch, initial_policy_fetch_timeout,
+      std::move(exit_session), std::move(store)));
 }
 
 void ActiveDirectoryPolicyManager::Init(SchemaRegistry* registry) {
@@ -56,7 +63,7 @@ void ActiveDirectoryPolicyManager::Init(SchemaRegistry* registry) {
   PublishPolicy();
 
   scheduler_ = base::MakeUnique<PolicyScheduler>(
-      base::BindRepeating(&ActiveDirectoryPolicyManager::DoFetch,
+      base::BindRepeating(&ActiveDirectoryPolicyManager::DoPolicyFetch,
                           weak_ptr_factory_.GetWeakPtr()),
       base::BindRepeating(&ActiveDirectoryPolicyManager::OnPolicyFetched,
                           weak_ptr_factory_.GetWeakPtr()),
@@ -70,6 +77,9 @@ void ActiveDirectoryPolicyManager::Shutdown() {
 
 bool ActiveDirectoryPolicyManager::IsInitializationComplete(
     PolicyDomain domain) const {
+  if (waiting_for_initial_policy_fetch_) {
+    return false;
+  }
   if (domain == POLICY_DOMAIN_CHROME) {
     return store_->is_initialized();
   }
@@ -84,6 +94,12 @@ void ActiveDirectoryPolicyManager::OnStoreLoaded(
     CloudPolicyStore* cloud_policy_store) {
   DCHECK_EQ(store_.get(), cloud_policy_store);
   PublishPolicy();
+  if (fetch_ever_completed_) {
+    // Policy is guaranteed to be up to date with the previous fetch result
+    // because OnPolicyFetched() cancels any potentially running Load()
+    // operations.
+    CancelWaitForInitialPolicy(fetch_ever_succeeded_ /* success */);
+  }
 }
 
 void ActiveDirectoryPolicyManager::OnStoreError(
@@ -93,12 +109,40 @@ void ActiveDirectoryPolicyManager::OnStoreError(
   // complete on the ConfigurationPolicyProvider interface. Technically, this is
   // only required on the first load, but doesn't hurt in any case.
   PublishPolicy();
+  if (fetch_ever_completed_) {
+    CancelWaitForInitialPolicy(false /* success */);
+  }
+}
+
+void ActiveDirectoryPolicyManager::ForceTimeoutForTest() {
+  DCHECK(initial_policy_timeout_.IsRunning());
+  // Stop the timer to mimic what happens when a real timer fires, then invoke
+  // the timer callback directly.
+  initial_policy_timeout_.Stop();
+  OnBlockingFetchTimeout();
 }
 
 ActiveDirectoryPolicyManager::ActiveDirectoryPolicyManager(
     const AccountId& account_id,
+    bool wait_for_policy_fetch,
+    base::TimeDelta initial_policy_fetch_timeout,
+    base::OnceClosure exit_session,
     std::unique_ptr<CloudPolicyStore> store)
-    : account_id_(account_id), store_(std::move(store)) {}
+    : account_id_(account_id),
+      waiting_for_initial_policy_fetch_(wait_for_policy_fetch),
+      exit_session_(std::move(exit_session)),
+      store_(std::move(store)) {
+  // Delaying initialization complete is intended for user policy only.
+  DCHECK(!(account_id == EmptyAccountId() && wait_for_policy_fetch));
+  DCHECK_NE(wait_for_policy_fetch, initial_policy_fetch_timeout.is_zero());

[stevenjb, 2017/07/17 16:07:37]

nit: Do we need |wait_for_policy_fetch| as a parameter? I think the code might
actually be more clear with one less parameter and wait_for_policy fetch ==
!initial_policy_fetch_timeout.is_zero().

[Thiemo Nagel, 2017/07/18 13:02:18]

I like that very much. Thanks!

+  initial_policy_fetch_may_fail_ = !initial_policy_fetch_timeout.is_max();
+  if (wait_for_policy_fetch && !initial_policy_fetch_timeout.is_max()) {
+    initial_policy_timeout_.Start(
+        FROM_HERE, initial_policy_fetch_timeout,
+        base::Bind(&ActiveDirectoryPolicyManager::OnBlockingFetchTimeout,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+}
 
 void ActiveDirectoryPolicyManager::PublishPolicy() {
   if (!store_->is_initialized()) {
@@ -117,7 +161,7 @@ void ActiveDirectoryPolicyManager::PublishPolicy() {
   UpdatePolicy(std::move(bundle));
 }
 
-void ActiveDirectoryPolicyManager::DoFetch(
+void ActiveDirectoryPolicyManager::DoPolicyFetch(
     base::OnceCallback<void(bool success)> callback) {
   chromeos::DBusThreadManager* thread_manager =
       chromeos::DBusThreadManager::Get();
@@ -133,12 +177,65 @@ void ActiveDirectoryPolicyManager::DoFetch(
 }
 
 void ActiveDirectoryPolicyManager::OnPolicyFetched(bool success) {
-  if (!success) {
+  fetch_ever_completed_ = true;
+  if (success) {
+    fetch_ever_succeeded_ = true;
+  } else {
     LOG(ERROR) << "Active Directory policy fetch failed.";
+    if (store_->is_initialized()) {
+      CancelWaitForInitialPolicy(false /* success */);
+    }
   }
-  // Load independently of success or failure to keep up to date with whatever
-  // has happened on the authpolicyd / session manager side.
+  // Load independently of success or failure to keep in sync with the state in
+  // session manager. This cancels any potentially running Load() operations
+  // thus it is guaranteed that at the next OnStoreLoaded() invocation the
+  // policy is up-to-date with what was fetched.
   store_->Load();
 }
 
+void ActiveDirectoryPolicyManager::OnBlockingFetchTimeout() {
+  DCHECK(waiting_for_initial_policy_fetch_);
+  LOG(WARNING) << "Timed out while waiting for the policy fetch. "
+               << "The session will start with the cached policy.";
+  CancelWaitForInitialPolicy(false);
+}
+
+void ActiveDirectoryPolicyManager::CancelWaitForInitialPolicy(bool success) {
+  if (!waiting_for_initial_policy_fetch_)
+    return;
+
+  initial_policy_timeout_.Stop();
+
+  // If the conditions to continue profile initialization are not met, the user
+  // session is exited and initialization is not set as completed.
+  // TODO(tnagel): Maybe add code to retry policy fetch?
+  if (!store_->has_policy()) {
+    // If there's no policy at all (not even cached) the user session must not
+    // continue.
+    LOG(ERROR) << "Policy could not be obtained. "
+               << "Aborting profile initialization";
+    // Prevent duplicate exit session calls.
+    if (exit_session_) {
+      std::move(exit_session_).Run();
+    }
+    return;
+  }
+  if (!success && !initial_policy_fetch_may_fail_) {
+    LOG(ERROR) << "Policy fetch failed for the user. "
+               << "Aborting profile initialization";
+    // Prevent duplicate exit session calls.
+    if (exit_session_) {
+      std::move(exit_session_).Run();
+    }
+    return;
+  }
+
+  // Set initialization complete.
+  waiting_for_initial_policy_fetch_ = false;
+
+  // Publish policy (even though it hasn't changed) in order to signal load
+  // complete on the ConfigurationPolicyProvider interface.
+  PublishPolicy();
+}
+
 }  // namespace policy