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..fc1f40020a4d6a3e080b96a47ccd5d2f7af2ba84 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) {
@@ -70,10 +77,13 @@ 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();
   }
-  return true;
+  return false;

[emaxx, 2017/07/12 20:18:19]

Not sure I understand this change: now the component policies are always
reported as "uninitialized", while previously they were always "initialized" -
is that an expected change?

[Thiemo Nagel, 2017/07/14 12:18:55]

Since we don't support component policy for Chromad (yet), I thought returning
false would make more sense. But maybe it's a bad idea?

[Thiemo Nagel, 2017/07/14 12:21:29]

I've changed it back.

 }
 
 void ActiveDirectoryPolicyManager::RefreshPolicies() {
@@ -84,6 +94,9 @@ void ActiveDirectoryPolicyManager::OnStoreLoaded(
     CloudPolicyStore* cloud_policy_store) {
   DCHECK_EQ(store_.get(), cloud_policy_store);
   PublishPolicy();
+  if (fetch_ever_completed_) {
+    CancelWaitForPolicy(fetch_ever_succeeded_ /* success */);
+  }
 }
 
 void ActiveDirectoryPolicyManager::OnStoreError(
@@ -93,12 +106,39 @@ 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_) {
+    CancelWaitForPolicy(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)) {
+  DCHECK(!(account_id == EmptyAccountId() && wait_for_policy_fetch));
+  DCHECK_NE(wait_for_policy_fetch, initial_policy_fetch_timeout.is_zero());
+  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,
+                   base::Unretained(this)));

[emaxx, 2017/07/12 20:18:19]

nit: Ideally we'd have this documented why Unretained is safe here (although I
know that this was copied from the existing class which doesn't have this).

[Thiemo Nagel, 2017/07/14 12:18:55]

Good point. It used to be safe because the timer was the last member of the
class but that isn't true anymore... :-O  Probably better to use a weak pointer.

+  }
+}
 
 void ActiveDirectoryPolicyManager::PublishPolicy() {
   if (!store_->is_initialized()) {
@@ -133,12 +173,60 @@ 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()) {
+      CancelWaitForPolicy(false /* success */);
+    }
   }
   // Load independently of success or failure to keep up to date with whatever
   // has happened on the authpolicyd / session manager side.
   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.";
+  CancelWaitForPolicy(false);
+}
+
+void ActiveDirectoryPolicyManager::CancelWaitForPolicy(bool success) {
+  if (!waiting_for_initial_policy_fetch_)
+    return;
+
+  initial_policy_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(tnagel): Maybe add code to retry policy fetching?
+  if (!store_->has_policy()) {

[emaxx, 2017/07/12 20:18:20]

I'm quite confused about complexity of the state machine in this class.
However, I'm not ready to give any specific advice here.

But what about this case - isn't there a race condition:
1. Init(). (This calls into store_->Load().)
2. OnPolicyFetched(true). (This calls into store_->Load() again.)
3. OnStoreLoaded(), which is called as the result of *first* store_->Load().
(This calls into CancelWaitForPolicy().)
On step #3, if store_ is unlucky to not have loaded the policy fetch result in
time, the condition at this line will lead to the user being signed out.
Or am I missing something?

In any case, I'd be glad if some simpler way to handle all those state
transitions could be found...

[Thiemo Nagel, 2017/07/14 12:18:55]

After a lot of offline discussion: It's hard to simplify the logic without
sacrificing performance (i.e. de-parallelizing the code).

store_->Load() cancels the previous load operation, thus there is no race
condition.

+    // If there's no policy at all (not even cached) we can't continue.
+    LOG(ERROR) << "Policy could not be obtained. "
+               << "Aborting profile initialization";
+    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";
+    if (exit_session_) {
+      std::move(exit_session_).Run();
+    }
+    return;
+  }
+
+  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