Introduce lock screen app manager

chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc

+// Copyright 2017 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/lock_screen_apps/app_manager_impl.h"
+
+#include <memory>
+#include <utility>
+#include <vector>
+
+#include "apps/launcher.h"
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/strings/string16.h"
+#include "chrome/browser/chromeos/note_taking_helper.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/common/pref_names.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/browser/install_flag.h"
+#include "extensions/common/api/app_runtime.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_set.h"
+
+namespace lock_screen_apps {
+
+AppManagerImpl::AppManagerImpl() : extensions_observer_(this) {}
+
+AppManagerImpl::~AppManagerImpl() = default;
+
+void AppManagerImpl::Initialize(Profile* primary_profile,
+                                Profile* lock_screen_profile) {
+  DCHECK_EQ(State::kNotInitialized, state_);
+  DCHECK(primary_profile);
+  DCHECK(lock_screen_profile);
+  DCHECK_NE(primary_profile, lock_screen_profile);

[Devlin, 2017/06/05 16:09:35]

Is there any defined relation between primary and lock screen profile?  e.g.,
would calling lockscreen_profile->GetOriginalProfile() return primary profile?

[tbarzic, 2017/06/06 01:54:55]

Not really. GetOriginalProfile would return the sign-in profile that is OTR
(which is the case for sign-in profile).
Though, we don't really need OTR sign-in profile here - all usages will get
redirected to the original profile (either using
ExtensionSystem/ExtensionRegistry service getters, or explicitly in
LaunchNoteTaking) - I think I can just pass in the orignial profile as
lock_screen_profile in |Initialize|.

+
+  CHECK(!chromeos::ProfileHelper::Get()->GetUserByProfile(lock_screen_profile))
+      << "Lock screen profile should not be associated with any users.";
+
+  primary_profile_ = primary_profile;
+  lock_screen_profile_ = lock_screen_profile;
+  state_ = State::kInactive;
+
+  pref_change_registrar_.Init(primary_profile->GetPrefs());
+  pref_change_registrar_.Add(
+      prefs::kNoteTakingAppId,
+      base::Bind(&AppManagerImpl::OnNoteTakingExtensionChanged,
+                 base::Unretained(this)));
+  pref_change_registrar_.Add(
+      prefs::kNoteTakingAppEnabledOnLockScreen,
+      base::Bind(&AppManagerImpl::OnNoteTakingExtensionChanged,
+                 base::Unretained(this)));
+}
+
+void AppManagerImpl::Start(const base::Closure& note_taking_changed_callback) {
+  DCHECK_NE(State::kNotInitialized, state_);
+
+  note_taking_changed_callback_ = note_taking_changed_callback;
+  extensions_observer_.Add(
+      extensions::ExtensionRegistry::Get(primary_profile_));
+
+  if (state_ == State::kActive)
+    return;
+
+  lock_screen_app_id_.clear();
+  std::string app_id = FindLockScreenNoteTakingApp();
+  if (!app_id.empty() && AddAppToLockScreenProfile(app_id))
+    lock_screen_app_id_ = app_id;
+
+  state_ = State::kActive;
+}
+
+void AppManagerImpl::Stop() {
+  DCHECK_NE(State::kNotInitialized, state_);
+
+  note_taking_changed_callback_.Reset();
+  extensions_observer_.RemoveAll();

[Devlin, 2017/06/05 16:09:35]

Is the ExtensionRegistry guaranteed to outlive this object?

[tbarzic, 2017/06/06 01:54:55]

yes, this is owned by StateController, which is destroyed before profiles (in
ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun before "shared"
PostMainMessageLoopRun, which starts process tear down, part of which is
destroying profiles).

+
+  if (state_ == State::kInactive)
+    return;
+
+  UnloadLockApp(lock_screen_app_id_);
+  lock_screen_app_id_.clear();
+  state_ = State::kInactive;
+}
+
+bool AppManagerImpl::IsNoteTakingAppAvailable() const {
+  return state_ == State::kActive && !lock_screen_app_id_.empty();
+}
+
+std::string AppManagerImpl::GetNoteTakingAppId() const {
+  if (!IsNoteTakingAppAvailable())
+    return std::string();
+  return lock_screen_app_id_;
+}
+
+bool AppManagerImpl::LaunchNoteTaking() {
+  if (!IsNoteTakingAppAvailable())
+    return false;
+
+  Profile* target_profile = lock_screen_profile_->GetOriginalProfile();

[Devlin, 2017/06/05 16:09:35]

can we somewhere explain target, lockscreen, and primary profile distinctions?

[tbarzic, 2017/06/06 01:54:55]

Documentation for AppManager::Initialize has some explanations for lock screen
and primary profile.

I removed target profile (which was just non-OTR lock_screen_profile)

+  const extensions::ExtensionRegistry* extension_registry =
+      extensions::ExtensionRegistry::Get(target_profile);
+  const extensions::Extension* app = extension_registry->GetExtensionById(
+      lock_screen_app_id_, extensions::ExtensionRegistry::ENABLED);
+  if (!app)
+    return false;
+
+  auto action_data =
+      base::MakeUnique<extensions::api::app_runtime::ActionData>();
+  action_data->action_type =
+      extensions::api::app_runtime::ActionType::ACTION_TYPE_NEW_NOTE;
+  apps::LaunchPlatformAppWithAction(target_profile, app, std::move(action_data),
+                                    base::FilePath());
+  return true;
+}
+
+void AppManagerImpl::OnExtensionLoaded(content::BrowserContext* browser_context,
+                                       const extensions::Extension* extension) {
+  if (extension->id() ==
+      primary_profile_->GetPrefs()->GetString(prefs::kNoteTakingAppId)) {
+    OnNoteTakingExtensionChanged();
+  }
+}
+
+void AppManagerImpl::OnExtensionUnloaded(
+    content::BrowserContext* browser_context,
+    const extensions::Extension* extension,
+    extensions::UnloadedExtensionReason reason) {
+  if (extension->id() == lock_screen_app_id_)
+    OnNoteTakingExtensionChanged();
+}
+
+void AppManagerImpl::OnNoteTakingExtensionChanged() {
+  if (state_ != State::kActive)
+    return;
+  std::string app_id = FindLockScreenNoteTakingApp();
+  if (app_id == lock_screen_app_id_)
+    return;
+
+  UnloadLockApp(lock_screen_app_id_);
+  lock_screen_app_id_.clear();
+
+  if (AddAppToLockScreenProfile(app_id))
+    lock_screen_app_id_ = app_id;
+
+  if (!note_taking_changed_callback_.is_null())
+    note_taking_changed_callback_.Run();
+}
+
+std::string AppManagerImpl::FindLockScreenNoteTakingApp() const {
+  // Note that lock screen does not currently support Android apps, so
+  // it's enough to only check the state of the preferred Chrome app.
+  std::unique_ptr<chromeos::NoteTakingAppInfo> note_taking_app =
+      chromeos::NoteTakingHelper::Get()->GetPreferredChromeAppInfo(
+          primary_profile_);
+
+  if (!note_taking_app ||
+      note_taking_app->lock_screen_support !=
+          chromeos::NoteTakingLockScreenSupport::kSelected) {
+    return std::string();
+  }
+
+  return note_taking_app->app_id;
+}
+
+bool AppManagerImpl::AddAppToLockScreenProfile(const std::string& app_id) {
+  extensions::ExtensionRegistry* primary_registry =
+      extensions::ExtensionRegistry::Get(primary_profile_);
+  const extensions::Extension* app =
+      primary_registry->enabled_extensions().GetByID(app_id);
+  if (!app)
+    return false;
+
+  ExtensionService* lock_screen_service =
+      extensions::ExtensionSystem::Get(lock_screen_profile_)
+          ->extension_service();
+
+  // If the app is already installed, remove it.
+  extensions::ExtensionRegistry* lock_screen_registry =
+      extensions::ExtensionRegistry::Get(lock_screen_profile_);
+  if (lock_screen_registry->GetExtensionById(
+          app_id, extensions::ExtensionRegistry::EVERYTHING)) {
+    ExtensionService::UninstallExtensionHelper(

[Devlin, 2017/06/05 16:09:35]

nit: prefer just using ExtensionService::UninstallExtension().

[tbarzic, 2017/06/06 01:54:55]

Done.

+        lock_screen_service, app_id,
+        extensions::UNINSTALL_REASON_INTERNAL_MANAGEMENT);
+  }
+
+  // TODO(tbarzic): Instead of loading the app from user profile path, consider
+  // copying and installing the app to the target profile.

[Devlin, 2017/06/05 16:09:35]

Yeah, this strikes me as very strange.  What happens when one of these profiles
tries to write to the extension directory?  Does the lock screen profile read
access to all the files under the user profile?

[tbarzic, 2017/06/06 01:54:55]

Yeah, I have not yet completely figured out the best approach here :/

Lock screen profile should be able to load resources from user profile location
(provided that the user cryptohome remains decrypted; this might change in
future, but I don't see it happening soon) - after all the resources reading is
done by the same process.

As writing to the extension directory goes, that should generally happen on
extension installation/update, right?
I'd expect that an extension directory remains constant while the extension is
loaded, and that the resource changes become visible to the app after reload -
the similar thing would happen in lock screen profile (when user profile updates
an extension) - loading/unloading of the user profile app should cause the app
load/unload in the lock screen profile.

And lock screen profile should not really write to paths under the user profile
(we probably should also ensure that the extension updater is not started for
lock screen profile, so there are no unexpected updates - though, looking at
code, these would seem to end up in the lock screen profile).

[Devlin, 2017/06/09 18:18:40]

Out of curiosity, is there a reason to not write the extension to the lockscreen
profile, thus avoiding this issue and the uninstall/delete file problem?

[tbarzic, 2017/06/09 19:01:45]

One reason is time - I do plan to look more closely into possibility of copying
extension to the lock screen profile, but I think getting the lock screen APIs
working and possibly unbounding lock screen profile from sign-in profile have
somewhat higher priority at this moment.

One potential concern is that we'd end up copying data from (encrypted) user
profile to (unencrypted) lock screen profile, though I expect this being
relevant mainly for unpacked extensions; then there's the question of if the
memory cost of duplicating the extension dir and time cost of copying data is
justified; handling app updates (instead of just reloading the app in lock
screen profile, we'd have to reload and copy app directory)..
I don't think there's anything huge, but it's not that clear-cut to me that
introducing a new installer (from a location within another profile) would be
beneficial.

[Devlin, 2017/06/12 18:31:45]

I talked with kerrnel@ about this, and we both agreed that it makes us a little
uncomfortable from a security standpoint to have the lockscreen profile require
read access to the user's profile.  I think we really should strive to have the
lockscreen profile version separate from the other, which also alleviates any
concerns around the lockscreen profile interfering with the user's profile.

[rkc, 2017/06/13 21:18:17]

We've had this discussion in many different forms now. This code runs in a
process that *has* access to the user profile. We are depending on the context
keying of code to prevent access to the logged in user's profile from the
lockscreen handling code.

That being said, making one explicit call to read data from the user's profile,
does not equate to "giving the lock screen access to the user profile". Again,
the lockscreen code *already* has that access.

I do want us to take any and all measures we can to ensure that we mitigate any
possible security issues - I just don't see removing this call does that.

We need to load the app. The app is in the user profile. Whether we have code
that copies the app out first, or code that directly loads the app - we *will*
be reading this one particular piece of data from the user profile.

I would argue that given that this call is a well defined call in very mature
code, it would be safer to use it than to introduce new code to do the copy.

+  std::string error;
+  scoped_refptr<extensions::Extension> lock_profile_app =
+      extensions::Extension::Create(app->path(), app->location(),
+                                    *app->manifest()->value()->CreateDeepCopy(),
+                                    app->creation_flags(), app->id(), &error);
+
+  if (!lock_profile_app)
+    return false;
+
+  lock_screen_service->OnExtensionInstalled(
+      lock_profile_app.get(), syncer::StringOrdinal(),
+      extensions::kInstallFlagInstallImmediately);
+  lock_screen_service->EnableExtension(lock_profile_app->id());
+  return true;
+}
+
+void AppManagerImpl::UnloadLockApp(const std::string& app_id) {

[Devlin, 2017/06/05 16:09:35]

s/Lock/LockScreen?

[tbarzic, 2017/06/06 01:54:55]

Done.

+  if (app_id.empty())
+    return;
+
+  extensions::ExtensionRegistry* lock_screen_registry =
+      extensions::ExtensionRegistry::Get(lock_screen_profile_);
+  if (!lock_screen_registry->GetExtensionById(
+          app_id, extensions::ExtensionRegistry::EVERYTHING)) {
+    return;
+  }
+
+  ExtensionService::UninstallExtensionHelper(

[Devlin, 2017/06/05 16:09:34]

This is pretty strange in combination with using the different path above.  We
delete the extension files when we uninstall the extension.  By virtue of
implementation details, I think this happens to work, but we really shouldn't be
relying on that.

[Devlin, 2017/06/05 16:09:34]

Do we unload it, or uninstall it?  The two are not synonymous in the extension
system.

[tbarzic, 2017/06/06 01:54:55]

Closer to uninstall (though, the initial version of the cl had unloading - I
changed that so the app data is cleared between session locks).
Yeah, this happens to work due to implementation in chrome_assets_manager - I do
plan to make this check more explicit there when I introduce designated lock
screen app profile (provided the way the app is added to lock screen profile
does not change - otherwise this would become a moot point).

Or maybe I can just add an option to ExtensionService::UninstallExtension to
skip deleting extension files.

+      extensions::ExtensionSystem::Get(lock_screen_profile_)
+          ->extension_service(),
+      app_id, extensions::UNINSTALL_REASON_INTERNAL_MANAGEMENT);
+}
+
+}  // namespace lock_screen_apps