Initial RemoteCommandService

components/policy/core/common/remote_commands/remote_commands_service.cc

Index: components/policy/core/common/remote_commands/remote_commands_service.cc
diff --git a/components/policy/core/common/remote_commands/remote_commands_service.cc b/components/policy/core/common/remote_commands/remote_commands_service.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e1571ca82cad06d2b05dc2401f11ea95fa9781dd
--- /dev/null
+++ b/components/policy/core/common/remote_commands/remote_commands_service.cc
@@ -0,0 +1,122 @@
+// Copyright 2015 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 "components/policy/core/common/remote_commands/remote_commands_service.h"
+
+#include <algorithm>
+#include <string>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/logging.h"
+#include "base/time/time.h"
+#include "components/policy/core/common/cloud/cloud_policy_client.h"
+#include "components/policy/core/common/remote_commands/remote_commands_factory.h"
+
+namespace policy {
+
+namespace em = enterprise_management;
+
+RemoteCommandsService::RemoteCommandsService(
+    scoped_ptr<RemoteCommandsFactory> factory,
+    CloudPolicyClient* client)
+    : factory_(factory.Pass()), client_(client), weak_factory_(this) {
+  queue_.AddObserver(this);
+}
+
+RemoteCommandsService::~RemoteCommandsService() {
+  queue_.RemoveObserver(this);
+}
+
+bool RemoteCommandsService::FetchRemoteCommands() {
+  if (command_fetch_in_progress_) {
+    has_unhanded_fetch_request_ = true;
+    return false;
+  }
+
+  command_fetch_in_progress_ = true;
+
+  std::vector<em::RemoteCommandResult> previous_results;
+  unsent_results_.swap(previous_results);
+
+  client_->FetchRemoteCommands(
+      last_executed_command_id_, previous_results,

[bartfab (slow), 2015/02/18 11:12:53]

s/last_executed_command_id_/last_fetched_command_id_/

Ideally, there should be a test that verifies we do not confuse the two here.

[binjin, 2015/02/18 15:37:50]

As discussed offline, I think it's better to acknowledge server with last
executed id think it will recover better. Also add set based container (instead
of queue) and some comments regarding this.

+      base::Bind(&RemoteCommandsService::OnRemoteCommandsFetched,
+                 weak_factory_.GetWeakPtr()));
+  return true;
+}
+
+void RemoteCommandsService::AddCommand(
+    const enterprise_management::RemoteCommand& command) {
+  if (!command.has_type() || !command.has_unique_id()) {
+    LOG(WARNING) << "Invalid remote command from server.";
+    return;
+  }
+
+  last_fetched_command_id_ = command.unique_id();
+
+  scoped_ptr<RemoteCommandJob> job = factory_->BuildJobForType(command.type());
+
+  if (!job || !job->Init(command)) {
+    em::RemoteCommandResult ignored_result;
+    ignored_result.set_result(
+        em::RemoteCommandResult_ResultType_RESULT_IGNORED);
+    ignored_result.set_unique_id(command.unique_id());
+    unsent_results_.push_back(ignored_result);
+    return;
+  }
+
+  queue_.AddJob(job.Pass());
+}
+
+void RemoteCommandsService::OnJobStarted(RemoteCommandJob* command) {
+  last_executed_command_id_ = command->unique_id();

[bartfab (slow), 2015/02/18 11:12:53]

You are currently not using |last_executed_command_id_| for anything. Once you
add persistence, you should probably use the |last_executed_command_id_| you
read back from persistent storage to initialized |last_fetched_command_id_|.

I see why we would want to persist this information. I see no need to actually
keep it in a member.

[binjin, 2015/02/18 15:37:50]

See above comments, |last_executed_command_id_| will be used for fetching
requests. Deleted |last_fetched_command_id_| instead since we compare and ignore
commands with existing IDs now.

[bartfab (slow), 2015/02/23 13:13:56]

We should think about the error scenario that a command's execution causes a
crash. If we do not add some special-case handling for this, a command that
triggers a bug can get Chrome into a nasty crash loop (until the command
expires). Or maybe just looping until the command expires is good enough.

[binjin, 2015/02/24 05:29:49]

I think think can be avoided if the server add limits on the number of times a
single commands can be fetched. It's reasonable measure although not being neat.

For client side code, since the commands needs to crash RemoteCommandService to
actually started loops, I think I would rather not acknowledge this commands
just after OnJobStarted(), and wait until the results is available.

+  // TODO(binjin): Evaluate the security risks and attempts to sync
+  // |last_executed_command_id_| to some persistent source, so that we can
+  // reload it later without relying solely on server to keep our last
+  // acknowledged command id.
+}
+
+void RemoteCommandsService::OnJobFinished(RemoteCommandJob* command) {
+  em::RemoteCommandResult result;
+  result.set_unique_id(command->unique_id());
+  result.set_timestamp((command->execution_started_time() -
+                        base::Time::UnixEpoch()).InMilliseconds());
+
+  if (command->status() == RemoteCommandJob::SUCCEEDED) {
+    result.set_result(em::RemoteCommandResult_ResultType_RESULT_SUCCESS);
+    scoped_ptr<std::string> result_payload = command->GetResultPayload();
+    if (result_payload)
+      result.set_payload(*result_payload);
+  } else if (command->status() == RemoteCommandJob::EXPIRED ||
+             command->status() == RemoteCommandJob::INVALID) {
+    result.set_result(em::RemoteCommandResult_ResultType_RESULT_IGNORED);
+  } else {
+    result.set_result(em::RemoteCommandResult_ResultType_RESULT_FAILURE);
+  }
+
+  unsent_results_.push_back(result);
+
+  FetchRemoteCommands();
+}
+
+void RemoteCommandsService::OnRemoteCommandsFetched(
+    const std::vector<enterprise_management::RemoteCommand>& commands) {
+  DCHECK(command_fetch_in_progress_);
+  command_fetch_in_progress_ = false;
+
+  for (const auto& command : commands)
+    AddCommand(command);
+
+  // Start another fetch request job immediately if there is there are unsent
+  // command results or unhandled fetch requests.
+  if (!unsent_results_.empty() || has_unhanded_fetch_request_) {
+    has_unhanded_fetch_request_ = false;
+    FetchRemoteCommands();
+  } else {
+    has_unhanded_fetch_request_ = false;
+  }
+}
+
+}  // namespace policy