Implemented DeviceStatusCollector::GetDeviceSessionStatus()

chrome/browser/chromeos/policy/device_status_collector.cc

Index: chrome/browser/chromeos/policy/device_status_collector.cc
diff --git a/chrome/browser/chromeos/policy/device_status_collector.cc b/chrome/browser/chromeos/policy/device_status_collector.cc
index d3bd1140d17b926159ba8143a7363a301ea579af..c15926027898b878d0cd41a6c9bb570b8352140b 100644
--- a/chrome/browser/chromeos/policy/device_status_collector.cc
+++ b/chrome/browser/chromeos/policy/device_status_collector.cc
@@ -25,7 +25,10 @@
 #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"
@@ -40,6 +43,8 @@
 #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"
 
@@ -106,6 +111,25 @@ std::vector<em::VolumeInfo> GetVolumeInfo(
   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 {
@@ -132,6 +156,7 @@ DeviceStatusCollector::DeviceStatusCollector(
       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);
@@ -166,6 +191,8 @@ DeviceStatusCollector::DeviceStatusCollector(
       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
@@ -237,32 +264,38 @@ void DeviceStatusCollector::UpdateReportingSettings() {
 
   // 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(
@@ -382,9 +415,16 @@ void DeviceStatusCollector::IdleStateCallback(ui::IdleState state) {
 }
 
 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() {
@@ -693,9 +733,8 @@ bool DeviceStatusCollector::GetDeviceStatus(
   if (report_network_interfaces_)
     GetNetworkInterfaces(status);
 
-  if (report_users_) {
+  if (report_users_)
     GetUsers(status);
-  }
 
   if (report_hardware_status_)
     GetHardwareStatus(status);
@@ -711,7 +750,40 @@ bool DeviceStatusCollector::GetDeviceStatus(
 
 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() {