Refresh policies from DM server periodically

chrome/browser/policy/device_management_policy_provider.cc

Index: chrome/browser/policy/device_management_policy_provider.cc
diff --git a/chrome/browser/policy/device_management_policy_provider.cc b/chrome/browser/policy/device_management_policy_provider.cc
index fbf36ce952650e50e923ec11880733bfbb4a6286..502897fab0217713d69a9a7f0ba9366e11bb291d 100644
--- a/chrome/browser/policy/device_management_policy_provider.cc
+++ b/chrome/browser/policy/device_management_policy_provider.cc
@@ -7,13 +7,12 @@
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
+#include "base/rand_util.h"
 #include "base/task.h"
-#include "base/time.h"
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/policy/device_management_backend.h"
 #include "chrome/browser/policy/device_management_backend_impl.h"
 #include "chrome/browser/policy/device_management_policy_cache.h"
-#include "chrome/browser/policy/device_token_fetcher.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/notification_service.h"
@@ -23,7 +22,12 @@ namespace {
 
 const char kChromePolicyScope[] = "cros/device";
 const char kChromeDevicePolicySettingKey[] = "chrome-policy";
-const int64 kPolicyRefreshRateInMinutes = 3 * 60;  // 3 hours
+const int64 kPolicyRefreshRateInMilliseconds = 3 * 60 * 60 * 1000;  // 3 hours
+const int64 kPolicyRefreshMaxEarlierInMilliseconds = 20 * 60 * 1000;  // 20 mins
+// These are the base values for delays before retrying after an error. They
+// will be doubled each time they are used.
+const int64 kPolicyRefreshErrorDelayInMilliseconds = 3 * 1000;  // 3 seconds
+const int64 kDeviceTokenRefreshErrorDelayInMilliseconds = 3 * 1000;
 
 }  // namespace
 
@@ -53,6 +57,22 @@ class DeviceManagementPolicyProvider::InitializeAfterIOThreadExistsTask
   base::WeakPtr<DeviceManagementPolicyProvider> provider_;
 };
 
+class DeviceManagementPolicyProvider::RefreshTask : public Task {
+ public:
+  explicit RefreshTask(base::WeakPtr<DeviceManagementPolicyProvider> provider)
+      : provider_(provider) {}
+
+  // Task implementation:
+  virtual void Run() {
+    DeviceManagementPolicyProvider* provider = provider_.get();
+    if (provider)
+      provider->RefreshTaskExecute();
+  }
+
+ private:
+  base::WeakPtr<DeviceManagementPolicyProvider> provider_;
+};
+
 DeviceManagementPolicyProvider::DeviceManagementPolicyProvider(
     const ConfigurationPolicyProvider::PolicyDefinitionList* policy_list,
     DeviceManagementBackend* backend,
@@ -62,7 +82,12 @@ DeviceManagementPolicyProvider::DeviceManagementPolicyProvider(
       backend_(backend),
       token_service_(token_service),
       storage_dir_(GetOrCreateDeviceManagementDir(storage_dir)),
-      policy_request_pending_(false) {
+      policy_request_pending_(false),
+      refresh_task_pending_(false),
+      policy_refresh_rate_ms_(kPolicyRefreshRateInMilliseconds),
+      policy_refresh_max_earlier_ms_(kPolicyRefreshMaxEarlierInMilliseconds),
+      policy_refresh_error_delay_ms_(kPolicyRefreshErrorDelayInMilliseconds),
+      token_fetch_error_delay_ms_(kDeviceTokenRefreshErrorDelayInMilliseconds) {
   Initialize();
 }
 
@@ -75,34 +100,42 @@ bool DeviceManagementPolicyProvider::Provide(
   return true;
 }
 
-void DeviceManagementPolicyProvider::Observe(
-    NotificationType type,
-    const NotificationSource& source,
-    const NotificationDetails& details) {
-  if (type == NotificationType::DEVICE_TOKEN_AVAILABLE) {
-    if (token_fetcher_.get() == Source<DeviceTokenFetcher>(source).ptr() &&
-        !policy_request_pending_ &&
-        IsPolicyStale()) {
-      SendPolicyRequest();
-    }
-  } else {
-    NOTREACHED();
-  }
-}
-
 void DeviceManagementPolicyProvider::HandlePolicyResponse(
     const em::DevicePolicyResponse& response) {
-  cache_->SetPolicy(response);
-  NotifyStoreOfPolicyChange();
+  if (cache_->SetPolicy(response))
+    NotifyStoreOfPolicyChange();
   policy_request_pending_ = false;
+  // Reset the error delay since policy fetching succeeded this time.
+  policy_refresh_error_delay_ms_ = kPolicyRefreshErrorDelayInMilliseconds;
+  ScheduleRefreshTask(GetRefreshTaskDelay(
+      policy_refresh_rate_ms_, policy_refresh_max_earlier_ms_));
 }
 
 void DeviceManagementPolicyProvider::OnError(
     DeviceManagementBackend::ErrorCode code) {
-  LOG(WARNING) << "could not provide policy from the device manager (error = "
+  LOG(WARNING) << "Could not provide policy from the device manager (error = "
                << code << ")";

[danno, 2010/11/22 13:47:50]

you may want to add a "retrying in x millis ...." to the log

[Jakob Kummerow (corp), 2010/11/22 16:56:08]

Done.

   policy_request_pending_ = false;
-  // TODO(danno): do something sensible in the error case, perhaps retry later?
+  policy_refresh_error_delay_ms_ *= 2;
+  if (policy_refresh_rate_ms_ &&
+      policy_refresh_rate_ms_ < policy_refresh_error_delay_ms_) {
+    policy_refresh_error_delay_ms_ = policy_refresh_rate_ms_;
+  }
+  ScheduleRefreshTask(policy_refresh_error_delay_ms_);
+}
+
+void DeviceManagementPolicyProvider::OnTokenSuccess() {
+  if (policy_request_pending_)
+    return;
+  SendPolicyRequest();
+}
+
+void DeviceManagementPolicyProvider::OnTokenError() {
+  LOG(WARNING) << "Could not retrieve device token.";
+  token_fetch_error_delay_ms_ *= 2;
+  if (token_fetch_error_delay_ms_ > policy_refresh_rate_ms_)
+    token_fetch_error_delay_ms_ = policy_refresh_rate_ms_;
+  ScheduleRefreshTask(token_fetch_error_delay_ms_);

[danno, 2010/11/22 13:47:50]

Perhaps a second, token-specific task to address my concern below?

[Jakob Kummerow (corp), 2010/11/22 16:56:08]

See my answer below.

 }
 
 void DeviceManagementPolicyProvider::Shutdown() {
@@ -120,10 +153,6 @@ DeviceManagementBackend* DeviceManagementPolicyProvider::GetBackend() {
 }
 
 void DeviceManagementPolicyProvider::Initialize() {
-  registrar_.Add(this,
-                 NotificationType::DEVICE_TOKEN_AVAILABLE,
-                 NotificationService::AllSources());
-
   const FilePath policy_path = storage_dir_.Append(
       FILE_PATH_LITERAL("Policy"));
   cache_.reset(new DeviceManagementPolicyCache(policy_path));
@@ -141,12 +170,14 @@ void DeviceManagementPolicyProvider::InitializeAfterIOThreadExists() {
   if (token_service_) {
     token_fetcher_ =
         new DeviceTokenFetcher(GetBackend(), token_service_, token_path);
+    token_fetcher_->AddObserver(this);

[danno, 2010/11/22 13:47:50]

What ensures that the observer gets removed? You probably need a registrar
object or the like.

[Jakob Kummerow (corp), 2010/11/22 16:56:08]

Done. 
Added a registrar mechanism. 

     token_fetcher_->StartFetching();
   }
 }
 
 void DeviceManagementPolicyProvider::SendPolicyRequest() {
   if (!policy_request_pending_) {
+    policy_request_pending_ = true;
     em::DevicePolicyRequest policy_request;
     policy_request.set_policy_scope(kChromePolicyScope);
     em::DevicePolicySettingRequest* setting =
@@ -155,18 +186,48 @@ void DeviceManagementPolicyProvider::SendPolicyRequest() {
     GetBackend()->ProcessPolicyRequest(token_fetcher_->GetDeviceToken(),
                                        policy_request,
                                        this);
-    policy_request_pending_ = true;
+  } else {
+    LOG(WARNING) << "Not sending another PolicyRequest.";
+  }
+}
+
+void DeviceManagementPolicyProvider::RefreshTaskExecute() {

[danno, 2010/11/22 13:47:50]

Combining both the token request and the policy request into one seems to not
appropriately split responsibilities, and in any case the name becomes
misleading. Refresh==get token if I don't have one and refresh policy. I think I
would like this better if this routine was a little less of a big hammer that
both faults in the token and sends policy requests, e.g. I see only the token
fetching code as an appropriate response to OnTokenError(), not policy fetching
code.

[Jakob Kummerow (corp), 2010/11/22 16:56:08]

I don't see what's wrong with a task that assesses the overall situation and
then does whatever needs to be done. 
Also, splitting this into two separate tasks would mean to perform the same
functionality with almost twice as much code. 

Discussed offline: will keep it as it is. 

+  refresh_task_pending_ = false;

[danno, 2010/11/22 13:47:50]

DCHECK(refresh_task_pending_) before?

[Jakob Kummerow (corp), 2010/11/22 16:56:08]

Done.

+  // If there is no valid device token, the token_fetcher_ apparently failed,
+  // so it must be restarted.
+  if (!token_fetcher_->IsTokenValid()) {
+    if (token_fetcher_->IsTokenPending()) {
+      NOTREACHED();
+      return;
+    }
+    token_fetcher_->Restart();
+    return;
   }
+  // If there is a device token, just refresh policies.
+  SendPolicyRequest();
 }
 
-bool DeviceManagementPolicyProvider::IsPolicyStale() const {
-  base::Time now(base::Time::NowFromSystemTime());
-  base::Time last_policy_refresh_time =
-      cache_->last_policy_refresh_time();
-  base::Time policy_expiration_time =
-      last_policy_refresh_time + base::TimeDelta::FromMinutes(
-          kPolicyRefreshRateInMinutes);
-  return (now > policy_expiration_time);
+void DeviceManagementPolicyProvider::ScheduleRefreshTask(
+    int64 delay_in_milliseconds) {
+  // This check is simply a safeguard, the situation currently cannot happen.
+  if (refresh_task_pending_) {
+    NOTREACHED();
+    return;
+  }
+  refresh_task_pending_ = true;
+  BrowserThread::PostDelayedTask(BrowserThread::UI, FROM_HERE,
+                                 new RefreshTask(AsWeakPtr()),
+                                 delay_in_milliseconds);
+}
+
+// static
+int64 DeviceManagementPolicyProvider::GetRefreshTaskDelay(
+    int64 policy_refresh_rate_ms,
+    int64 policy_refresh_max_random_earlier_ms) {
+  if (policy_refresh_max_random_earlier_ms)
+    policy_refresh_rate_ms -=
+        base::RandGenerator(policy_refresh_max_random_earlier_ms);
+  return policy_refresh_rate_ms;
 }
 
 // static