Added system log uploader.

chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc

Index: chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
diff --git a/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc b/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
index f7c2aa57c6fed7300141ecb1fef3244764852733..a8c14fe0ffe9914c58d89b35028984a6b83af9cf 100644
--- a/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
+++ b/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
@@ -8,6 +8,7 @@
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
+#include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
@@ -24,6 +25,10 @@
 #include "chrome/browser/chromeos/policy/remote_commands/device_commands_factory_chromeos.h"
 #include "chrome/browser/chromeos/policy/server_backed_state_keys_broker.h"
 #include "chrome/browser/chromeos/policy/status_uploader.h"
+#include "chrome/browser/chromeos/policy/system_log_uploader.h"
+#include "chrome/browser/chromeos/policy/upload_job_impl.h"
+#include "chrome/browser/chromeos/settings/device_oauth2_token_service.h"
+#include "chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/chromeos_constants.h"
 #include "chromeos/chromeos_switches.h"
@@ -102,6 +107,116 @@ bool ForcedReEnrollmentEnabled() {
          chromeos::AutoEnrollmentController::MODE_FORCED_RE_ENROLLMENT;
 }
 
+// The file names of the system logs to upload.
+// Note: do not add anything to this list without checking for PII in the file.
+const char* const kSystemLogFileNames[] = {"/var/log/bios_info.txt",
+                                           "/var/log/chrome/chrome",
+                                           "/var/log/eventlog.txt",
+                                           "/var/log/messages",
+                                           "/var/log/net.log",
+                                           "/var/log/platform_info.txt",
+                                           "/var/log/ui/ui.LATEST",
+                                           "/var/log/update_engine.log"};
+
+// There is an implementation of the |SystemLogUploader::Delegate|,
+// that is used to create an upload job and load system logs from the disk.
+class SystemLogDelegate : public policy::SystemLogUploader::Delegate {
+ public:
+  SystemLogDelegate();
+  ~SystemLogDelegate() override;
+
+  // SystemLogUploader::Delegate:
+  void LoadSystemLogs(const LogUploadCallback& upload_callback) override;
+
+  scoped_ptr<policy::UploadJob> CreateUploadJob(
+      const GURL& upload_url,
+      policy::UploadJob::Delegate* delegate) override;
+
+ private:
+  // FileReader - helper class that thread safely reads files from the disk.
+  class FileReader : public base::RefCountedThreadSafe<FileReader> {

[Andrew T Wilson (Slow), 2015/07/31 12:07:13]

Since this class doesn't have any actual data members, do you need a class here?
Can you just define static StartRead and Read functions? That's always
preferable to having a class that has to span threads.

[Polina Bondarenko, 2015/07/31 13:52:03]

Done, removed StartRead function, moved its functionality to LoadSystemLogs.

+   public:
+    FileReader() {}
+    void StartRead(const LogUploadCallback& upload_callback,

[Andrew T Wilson (Slow), 2015/07/31 12:07:13]

Document what this does/what should be passed.

[Polina Bondarenko, 2015/07/31 13:52:03]

Removed this function, for LoadSystemLogs function it is documented.

+                   base::CancelableTaskTracker* tracker);
+
+   private:
+    friend class base::RefCountedThreadSafe<FileReader>;
+    ~FileReader() {}
+
+    // Reads the system log files as binary files, stores the files as pairs
+    // (file name, data) in the external structure to pass it to the
+    // |upload_callback|. Called on the file thread (non-blocking).
+    void Read(SystemLogDelegate::SystemLogs* system_logs);
+
+    DISALLOW_COPY_AND_ASSIGN(FileReader);
+  };
+
+  // Used in reading log files.
+  scoped_refptr<FileReader> file_reader_;
+  base::CancelableTaskTracker tracker_;
+
+  DISALLOW_COPY_AND_ASSIGN(SystemLogDelegate);
+};
+
+SystemLogDelegate::SystemLogDelegate() : file_reader_(new FileReader) {
+}
+
+SystemLogDelegate::~SystemLogDelegate() {
+}
+
+void SystemLogDelegate::LoadSystemLogs(
+    const LogUploadCallback& upload_callback) {
+  file_reader_->StartRead(upload_callback, &tracker_);
+}
+
+scoped_ptr<policy::UploadJob> SystemLogDelegate::CreateUploadJob(

[Andrew T Wilson (Slow), 2015/07/31 12:07:13]

So, it's weird to me that this code lives here in
DeviceCloudPolicyManagerChromeOS - why should this class be responsible for
creating upload jobs, reading log files, etc? Why doesn't this all live off in
SystemLogUploader?

Note that for DeviceStatusCollector, we pass in a bunch of null pointers for the
various delegates/callbacks so it knows to use the default implementation. This
allows us to still inject implementations for tests, but
DeviceCloudPolicyManager doesn't need to know these details. Think we should do
that in this case as well.

[Polina Bondarenko, 2015/07/31 13:52:03]

Done, mpved to system_log_uploader.cc

+    const GURL& upload_url,
+    policy::UploadJob::Delegate* delegate) {
+  chromeos::DeviceOAuth2TokenService* device_oauth2_token_service =
+      chromeos::DeviceOAuth2TokenServiceFactory::Get();
+
+  scoped_refptr<net::URLRequestContextGetter> system_request_context =
+      g_browser_process->system_request_context();
+  std::string robot_account_id =
+      device_oauth2_token_service->GetRobotAccountId();
+  return scoped_ptr<policy::UploadJob>(new policy::UploadJobImpl(
+      upload_url, robot_account_id, device_oauth2_token_service,
+      system_request_context, delegate,
+      make_scoped_ptr(new policy::UploadJobImpl::RandomMimeBoundaryGenerator)));
+}
+
+void SystemLogDelegate::FileReader::StartRead(
+    const SystemLogDelegate::LogUploadCallback& upload_callback,
+    base::CancelableTaskTracker* tracker) {
+  // Owned by reply callback posted below.
+  SystemLogs* system_logs = new SystemLogs();
+
+  // Run Read() in the thread that interacts with the file
+  // system and return to the current thread.
+  tracker->PostTaskAndReply(
+      content::BrowserThread::GetMessageLoopProxyForThread(
+          content::BrowserThread::FILE).get(),
+      FROM_HERE, base::Bind(&FileReader::Read, this, system_logs),

[Andrew T Wilson (Slow), 2015/07/31 12:07:13]

I don't understand why we need a FileReader class. Can we just use static
functions?

[Polina Bondarenko, 2015/07/31 13:52:03]

Done.

+      base::Bind(upload_callback, base::Owned(system_logs)));
+}
+
+void SystemLogDelegate::FileReader::Read(SystemLogs* system_logs) {
+  // Must be called on the file thread.
+  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
+
+  for (auto const file_path : kSystemLogFileNames) {
+    if (!base::PathExists(base::FilePath(file_path)))
+      continue;
+    system_logs->push_back(std::make_pair(file_path, std::string()));
+    if (!base::ReadFileToString(base::FilePath(file_path),

[Andrew T Wilson (Slow), 2015/07/31 12:07:13]

We also want to add something that scans for common PII (email address, IP
address) and strips it. can you log a bug for this and add a TODO() to the
appropriate place in the code?

[Polina Bondarenko, 2015/07/31 13:52:03]

Done.

+                                &(system_logs->back().second))) {
+      LOG(ERROR) << "Failed to read the system log file from the disk "
+                 << file_path << std::endl;
+    }
+  }
+}
+
 }  // namespace
 
 DeviceCloudPolicyManagerChromeOS::DeviceCloudPolicyManagerChromeOS(
@@ -253,6 +368,8 @@ void DeviceCloudPolicyManagerChromeOS::StartConnection(
   // the monitoring settings and only perform monitoring if it is active.
   if (install_attributes->IsEnterpriseDevice()) {
     CreateStatusUploader();
+    syslog_uploader_.reset(new SystemLogUploader(
+        make_scoped_ptr(new SystemLogDelegate()), task_runner_));
     heartbeat_scheduler_.reset(
         new HeartbeatScheduler(g_browser_process->gcm_driver(),
                                install_attributes->GetDomain(),
@@ -277,6 +394,7 @@ void DeviceCloudPolicyManagerChromeOS::Unregister(
 
 void DeviceCloudPolicyManagerChromeOS::Disconnect() {
   status_uploader_.reset();
+  syslog_uploader_.reset();
   heartbeat_scheduler_.reset();
   core()->Disconnect();