Added system log uploader.

chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc

 // Copyright (c) 2012 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 "chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h"
 
 #include "base/bind.h"
 #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"
 #include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/attestation/attestation_policy_observer.h"
 #include "chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h"
 #include "chrome/browser/chromeos/login/startup_utils.h"
 #include "chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.h"
 #include "chrome/browser/chromeos/policy/device_status_collector.h"
 #include "chrome/browser/chromeos/policy/enterprise_install_attributes.h"
 #include "chrome/browser/chromeos/policy/heartbeat_scheduler.h"
 #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"
 #include "chromeos/system/statistics_provider.h"
 #include "components/policy/core/common/cloud/cloud_policy_core.h"
 #include "components/policy/core/common/cloud/cloud_policy_service.h"
 #include "components/policy/core/common/cloud/cloud_policy_store.h"
 #include "components/policy/core/common/remote_commands/remote_commands_factory.h"
 #include "content/public/browser/browser_thread.h"
 #include "crypto/sha2.h"
@@ skipping 58 matching lines @@
 
   return value;
 }
 
 // Checks whether forced re-enrollment is enabled.
 bool ForcedReEnrollmentEnabled() {
   return chromeos::AutoEnrollmentController::GetMode() ==
          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(
     scoped_ptr<DeviceCloudPolicyStoreChromeOS> store,
     const scoped_refptr<base::SequencedTaskRunner>& task_runner,
     ServerBackedStateKeysBroker* state_keys_broker)
     : CloudPolicyManager(
           dm_protocol::kChromeDevicePolicyType,
           std::string(),
           store.get(),
@@ skipping 131 matching lines @@
   attestation_policy_observer_.reset(
       new chromeos::attestation::AttestationPolicyObserver(client()));
 
   // Enable device reporting and status monitoring for enterprise enrolled
   // devices. We want to create these objects for enrolled devices, even if
   // monitoring is currently inactive, in case monitoring is turned back on in
   // a future policy fetch - the classes themselves track the current state of
   // 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(),
                                install_attributes->GetDeviceId(),
                                task_runner_));
   }
 
   NotifyConnected();
 }
 
 void DeviceCloudPolicyManagerChromeOS::Unregister(
     const UnregisterCallback& callback) {
   if (!service()) {
     LOG(ERROR) << "Tried to unregister but DeviceCloudPolicyManagerChromeOS is "
                << "not connected.";
     callback.Run(false);
     return;
   }
 
   service()->Unregister(callback);
 }
 
 void DeviceCloudPolicyManagerChromeOS::Disconnect() {
   status_uploader_.reset();
+  syslog_uploader_.reset();
   heartbeat_scheduler_.reset();
   core()->Disconnect();
 
   NotifyDisconnected();
 }
 
 void DeviceCloudPolicyManagerChromeOS::OnStateKeysUpdated() {
   if (client() && ForcedReEnrollmentEnabled())
     client()->SetStateKeysToUpload(state_keys_broker_->state_keys());
 }
@@ skipping 45 matching lines @@
       client(),
       make_scoped_ptr(new DeviceStatusCollector(
           local_state_, chromeos::system::StatisticsProvider::GetInstance(),
           DeviceStatusCollector::LocationUpdateRequester(),
           DeviceStatusCollector::VolumeInfoFetcher(),
           DeviceStatusCollector::CPUStatisticsFetcher())),
       task_runner_));
 }
 
 }  // namespace policy