Implemented DeviceStatusCollector::GetDeviceSessionStatus()

chrome/browser/chromeos/policy/device_status_collector.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_status_collector.h"
 
 #include <stdint.h>
 #include <limits>
 #include <sys/statvfs.h>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/process/process.h"
 #include "base/process/process_iterator.h"
 #include "base/process/process_metrics.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/sys_info.h"
 #include "base/task_runner_util.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/chromeos/app_mode/kiosk_app_manager.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
+#include "chrome/browser/chromeos/policy/device_local_account.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/disks/disk_mount_manager.h"
 #include "chromeos/network/device_state.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/system/statistics_provider.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/user_manager/user_manager.h"
 #include "components/user_manager/user_type.h"
 #include "content/public/browser/browser_thread.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension.h"
 #include "policy/proto/device_management_backend.pb.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 using base::Time;
 using base::TimeDelta;
 
 namespace em = enterprise_management;
 
 namespace {
 // How many seconds of inactivity triggers the idle state.
@@ skipping 46 matching lines @@
       info.set_storage_free(static_cast<uint64_t>(stat.f_bavail) *
                             stat.f_frsize);
       result.push_back(info);
     } else {
       LOG(ERROR) << "Unable to get volume status for " << mount_point;
     }
   }
   return result;
 }
 
+bool GetCurrentDeviceLocalAccount(chromeos::CrosSettings* settings,

[bartfab (slow), 2015/02/11 17:28:43]

Nit: The name does not match what the method actually does. Device-local
accounts are kiosk sessions and public sessions. This method handles kiosk
sessions only.

[Andrew T Wilson (Slow), 2015/02/12 15:15:38]

Done.

[Andrew T Wilson (Slow), 2015/02/12 15:15:39]

Changed name to clarify the behavior and added a comment.

+                                  policy::DeviceLocalAccount* account) {
+  if (!user_manager::UserManager::Get()->IsLoggedInAsKioskApp())
+    return false;
+  user_manager::User* user = user_manager::UserManager::Get()->GetActiveUser();

[bartfab (slow), 2015/02/11 17:28:43]

Nit: s/user_manager::User*/const user_manager::User* const/

[Andrew T Wilson (Slow), 2015/02/12 15:15:38]

Done.

+  std::string user_id = user->GetUserID();

[bartfab (slow), 2015/02/11 17:28:43]

Nit: const.

[Andrew T Wilson (Slow), 2015/02/12 15:15:38]

Done.

+  std::vector<policy::DeviceLocalAccount> accounts =

[bartfab (slow), 2015/02/11 17:28:43]

Nit: const.

+      policy::GetDeviceLocalAccounts(settings);

[bartfab (slow), 2015/02/11 17:28:43]

It is possible to remove a device-local account from policy while a session
using that account is running. You will run into a NOTREACHED() if that happens.

Why do you not simply grab the kiosk app ID from the switches::kAppId
command-line switch?

You still need the account ID of course. This is available from the policy
broker, even if the account was removed from policy.

[Andrew T Wilson (Slow), 2015/02/12 15:15:38]

OK, good to know. I've changed the NOTREACHED into a LOG(WARNING) and now we
just won't report on running kiosk sessions that are no longer part of policy.

[Andrew T Wilson (Slow), 2015/02/13 13:59:59]

BTW, not using the policy broker because it's basically impossible to mock so
testing is hugely problematic.

Let me know if for some reason going through the active user to figure out which
device-local account is active is bad. Otherwise I'm going to leave as-is.

[bartfab (slow), 2015/02/16 13:30:33]

The one advantage of going via the broker is that it will continue to work even
if the currently logged-in account has been removed from policy. It makes sense
that you would not care about reporting the status for a deleted account, so no
problem with going the CrosSettings route.

+
+  for (auto device_account : accounts) {

[bartfab (slow), 2015/02/11 17:28:43]

Nit 1: s/device_account/device_local_account/ or s/device_account/account/.
Let's not introduce new terminology ("device account").
Nit 2: s/auto/const auto&/ (no need for a copy where a reference will do)

[Andrew T Wilson (Slow), 2015/02/12 15:15:38]

Done.

+    if (device_account.user_id == user_id) {
+      *account = device_account;
+      return true;
+    }
+  }
+  NOTREACHED() << "Kiosk app user not found in list of local accounts";

[bartfab (slow), 2015/02/11 17:28:43]

Nit: s/local/device-local/

[Andrew T Wilson (Slow), 2015/02/12 15:15:38]

Done.

+  return false;
+}
+
 }  // namespace
 
 namespace policy {
 
 DeviceStatusCollector::DeviceStatusCollector(
     PrefService* local_state,
     chromeos::system::StatisticsProvider* provider,
     const LocationUpdateRequester& location_update_requester,
     const VolumeInfoFetcher& volume_info_fetcher)
     : max_stored_past_activity_days_(kMaxStoredPastActivityDays),
       max_stored_future_activity_days_(kMaxStoredFutureActivityDays),
       local_state_(local_state),
       last_idle_check_(Time()),
       last_reported_day_(0),
       duration_for_last_reported_day_(0),
       geolocation_update_in_progress_(false),
       volume_info_fetcher_(volume_info_fetcher),
       statistics_provider_(provider),
       location_update_requester_(location_update_requester),
       report_version_info_(false),
       report_activity_times_(false),
       report_boot_mode_(false),
       report_location_(false),
       report_network_interfaces_(false),
       report_users_(false),
       report_hardware_status_(false),
+      report_session_status_(false),
       weak_factory_(this) {
   if (volume_info_fetcher_.is_null())
     volume_info_fetcher_ = base::Bind(&GetVolumeInfo);
 
   idle_poll_timer_.Start(FROM_HERE,
                          TimeDelta::FromSeconds(kIdlePollIntervalSeconds),
                          this, &DeviceStatusCollector::CheckIdleState);
   hardware_status_sampling_timer_.Start(
       FROM_HERE,
       TimeDelta::FromSeconds(kHardwareStatusSampleIntervalSeconds),
@@ skipping 14 matching lines @@
   boot_mode_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceBootMode, callback);
   location_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceLocation, callback);
   network_interfaces_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceNetworkInterfaces, callback);
   users_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceUsers, callback);
   hardware_status_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceHardwareStatus, callback);
+  session_status_subscription_ = cros_settings_->AddSettingsObserver(
+      chromeos::kReportDeviceSessionStatus, callback);
 
   // The last known location is persisted in local state. This makes location
   // information available immediately upon startup and avoids the need to
   // reacquire the location on every user session change or browser crash.
   content::Geoposition position;
   std::string timestamp_str;
   int64 timestamp;
   const base::DictionaryValue* location =
       local_state_->GetDictionary(prefs::kDeviceLocation);
   if (location->GetDouble(kLatitude, &position.latitude) &&
@@ skipping 51 matching lines @@
   // back when they are available.
   if (chromeos::CrosSettingsProvider::TRUSTED !=
       cros_settings_->PrepareTrustedValues(
       base::Bind(&DeviceStatusCollector::UpdateReportingSettings,
                  weak_factory_.GetWeakPtr()))) {
     return;
   }
 
   // All reporting settings default to 'enabled'.
   if (!cros_settings_->GetBoolean(
-      chromeos::kReportDeviceVersionInfo, &report_version_info_)) {
+          chromeos::kReportDeviceVersionInfo, &report_version_info_)) {
     report_version_info_ = true;
   }
   if (!cros_settings_->GetBoolean(
-      chromeos::kReportDeviceActivityTimes, &report_activity_times_)) {
+          chromeos::kReportDeviceActivityTimes, &report_activity_times_)) {
     report_activity_times_ = true;
   }
   if (!cros_settings_->GetBoolean(
-      chromeos::kReportDeviceBootMode, &report_boot_mode_)) {
+          chromeos::kReportDeviceBootMode, &report_boot_mode_)) {
     report_boot_mode_ = true;
   }
   if (!cros_settings_->GetBoolean(
-      chromeos::kReportDeviceNetworkInterfaces, &report_network_interfaces_)) {
+          chromeos::kReportDeviceNetworkInterfaces,
+          &report_network_interfaces_)) {
     report_network_interfaces_ = true;
   }
   if (!cros_settings_->GetBoolean(
-      chromeos::kReportDeviceUsers, &report_users_)) {
+          chromeos::kReportDeviceUsers, &report_users_)) {
     report_users_ = true;
   }
 
   const bool already_reporting_hardware_status = report_hardware_status_;
   if (!cros_settings_->GetBoolean(
-      chromeos::kReportDeviceHardwareStatus, &report_hardware_status_)) {
+          chromeos::kReportDeviceHardwareStatus, &report_hardware_status_)) {
     report_hardware_status_ = true;
   }
 
+  if (!cros_settings_->GetBoolean(
+          chromeos::kReportDeviceSessionStatus, &report_session_status_)) {
+    report_session_status_ = true;
+  }
+
   // Device location reporting is disabled by default because it is
   // not launched yet.
   if (!cros_settings_->GetBoolean(
       chromeos::kReportDeviceLocation, &report_location_)) {
     report_location_ = false;
   }
 
   if (report_location_) {
     ScheduleGeolocationUpdateRequest();
   } else {
@@ skipping 99 matching lines @@
     } else {
       AddActivePeriod(last_idle_check_, now);
     }
 
     PruneStoredActivityPeriods(now);
   }
   last_idle_check_ = now;
 }
 
 bool DeviceStatusCollector::IsAutoLaunchedKioskSession() {
-  // TODO(atwilson): Determine if the currently active session is an
-  // autolaunched kiosk session (http://crbug.com/452968).
-  return false;
+  DeviceLocalAccount account(DeviceLocalAccount::TYPE_KIOSK_APP);

[bartfab (slow), 2015/02/11 17:28:43]

I think this implementation can be improved. The line above makes it look like
passing TYPE_KIOSK_APP here is significant in some way while in reality, you
just want a dummy DeviceLocalAccount you can write to.

How about having GetCurrentDeviceLocalAccount() return a scoped_ptr instead?

By doing it this way, you avoid the need to introduce a new DeviceLocalAccount
constructor and prevent the TYPE_KIOSK_APP above from looking significant.

[Andrew T Wilson (Slow), 2015/02/12 15:15:39]

Done.

+  // If no kiosk app is currently running, exit.
+  if (!GetCurrentDeviceLocalAccount(cros_settings_, &account))
+    return false;
+  chromeos::KioskAppManager::App current_app;
+  if (!chromeos::KioskAppManager::Get()->GetApp(account.kiosk_app_id,
+                                                &current_app)) {
+    return false;

[bartfab (slow), 2015/02/11 17:28:43]

Is this not a NOTREACHED() case?

[Andrew T Wilson (Slow), 2015/02/12 15:15:38]

Done.

+  }
+  return current_app.was_auto_launched_with_zero_delay;
 }
 
 void DeviceStatusCollector::SampleHardwareStatus() {
   // If hardware reporting has been disabled, do nothing here.
   if (!report_hardware_status_)
     return;
 
   // Create list of mounted disk volumes to query status.
   std::vector<std::string> mount_points;
   for (const auto& mount_info :
@@ skipping 288 matching lines @@
 
   if (report_boot_mode_)
     GetBootMode(status);
 
   if (report_location_)
     GetLocation(status);
 
   if (report_network_interfaces_)
     GetNetworkInterfaces(status);
 
-  if (report_users_) {
+  if (report_users_)
     GetUsers(status);
-  }
 
   if (report_hardware_status_)
     GetHardwareStatus(status);
 
   return (report_activity_times_ ||
           report_version_info_ ||
           report_boot_mode_ ||
           report_location_ ||
           report_network_interfaces_ ||
           report_users_ ||
           report_hardware_status_);
 }
 
 bool DeviceStatusCollector::GetDeviceSessionStatus(
     em::SessionStatusReportRequest* status) {
-  return false;
+  // Only generate session status reports if session status reporting is
+  // enabled, and we are in an auto-launched kiosk session.
+  if (!report_session_status_ || !IsAutoLaunchedKioskSession())
+    return false;
+
+  // Get the account ID associated with this user.
+  DeviceLocalAccount account(DeviceLocalAccount::TYPE_KIOSK_APP);
+  if (!GetCurrentDeviceLocalAccount(cros_settings_, &account))

[bartfab (slow), 2015/02/11 17:28:43]

You are doing all this fetching, parsing and searching of the device-local
account list twice. It would be much nicer to hold on to the kiosk app ID you
get the first time instead.

[Andrew T Wilson (Slow), 2015/02/12 15:15:38]

Done. This cascaded into a yak shave that impacted the test code, but probably
in a good way.

+    return false;
+  status->set_device_local_account_id(account.account_id);
+  em::AppStatus* app_status = status->add_installed_apps();
+  app_status->set_app_id(account.kiosk_app_id);
+
+  // Look up the app and get the version.
+  std::string app_version = GetAppVersion(account.kiosk_app_id);

[bartfab (slow), 2015/02/11 17:28:43]

Nit: const.

[Andrew T Wilson (Slow), 2015/02/12 15:15:39]

Done.

+  if (app_version.empty()) {
+    DLOG(ERROR) << "Unable to get version for extension: "
+                << account.kiosk_app_id;
+  } else {
+    app_status->set_extension_version(app_version);
+  }
+  return true;
+}
+
+std::string DeviceStatusCollector::GetAppVersion(std::string kiosk_app_id) {
+  Profile* profile = chromeos::ProfileHelper::Get()->GetProfileByUser(
+      user_manager::UserManager::Get()->GetActiveUser());
+  extensions::ExtensionRegistry* registry =
+      extensions::ExtensionRegistry::Get(profile);
+  const extensions::Extension* extension = registry->GetExtensionById(
+      kiosk_app_id, extensions::ExtensionRegistry::EVERYTHING);
+  if (!extension)
+    return std::string();
+  return extension->VersionString();
 }
 
 void DeviceStatusCollector::OnSubmittedSuccessfully() {
   TrimStoredActivityPeriods(last_reported_day_, duration_for_last_reported_day_,
                             std::numeric_limits<int64>::max());
 }
 
 void DeviceStatusCollector::OnOSVersion(const std::string& version) {
   os_version_ = version;
 }
@@ skipping 60 matching lines @@
   ScheduleGeolocationUpdateRequest();
 }
 
 void DeviceStatusCollector::ReceiveVolumeInfo(
     const std::vector<em::VolumeInfo>& info) {
   if (report_hardware_status_)
     volume_info_ = info;
 }
 
 }  // namespace policy