[mac] App shims

apps/app_shim/app_shim_host.cc

+// Copyright 2013 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 "apps/app_shim/app_shim_host.h"
+
+#include "apps/app_shim/app_shim_messages.h"
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/extension_host.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system.h"
+#include "chrome/browser/extensions/shell_window_registry.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/extensions/application_launch.h"
+#include "chrome/common/extensions/extension_constants.h"
+#include "ipc/ipc_channel_proxy.h"
+
+AppShimHost::AppShimHost()
+    : channel_(NULL), profile_(NULL) {
+}
+
+AppShimHost::~AppShimHost() {
+  DCHECK(CalledOnValidThread());
+  if (channel_)
+    delete channel_;

[koz (OOO until 15th September), 2013/03/14 23:51:05]

scoped_ptr<> to hold channel_?

[jeremya, 2013/03/15 02:24:14]

Done.

+}
+
+void AppShimHost::ServeChannel(const IPC::ChannelHandle& handle) {
+  DCHECK(CalledOnValidThread());
+  channel_ = new IPC::ChannelProxy(handle, IPC::Channel::MODE_SERVER, this,

[koz (OOO until 15th September), 2013/03/14 23:51:05]

Is this method intended to be called multiple times? DCHECK(!channel_)
otherwise.

[jeremya, 2013/03/15 02:24:14]

Nope. Done.

+      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));
+}
+
+bool AppShimHost::OnMessageReceived(const IPC::Message& message) {
+  DCHECK(CalledOnValidThread());
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(AppShimHost, message)
+    IPC_MESSAGE_HANDLER(AppShimHostMsg_LaunchApp, OnLaunchApp)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+
+  return handled;
+}
+
+bool AppShimHost::Send(IPC::Message* message) {
+  DCHECK(channel_);
+  return channel_->Send(message);
+}
+
+void AppShimHost::OnLaunchApp(std::string profile_dir, std::string app_id) {
+  DCHECK(CalledOnValidThread());
+  bool success = LaunchAppImpl(profile_dir, app_id);
+  Send(new AppShimMsg_LaunchApp_Done(success));

[palmer, 2013/03/14 19:05:04]

Please pardon my ignorance: How is this new object's lifetime managed?

[jeremya, 2013/03/14 23:33:01]

From ipc/ipc_sender.h:
  // Sends the given IPC message.  The implementor takes ownership of the
  // given Message regardless of whether or not this method succeeds.  This
  // is done to make this method easier to use.  Returns true on success and
  // false otherwise.
  virtual bool Send(Message* msg) = 0;

i.e. channel_->Send() deletes it.

+}
+
+bool AppShimHost::LaunchAppImpl(const std::string& profile_dir,
+                                const std::string& app_id) {
+  DCHECK(CalledOnValidThread());
+  if (profile_) {
+    // Only one app launch message per channel.
+    return false;
+  }
+  if (!extensions::Extension::IdIsValid(app_id)) {
+    LOG(ERROR) << "Bad app ID from app shim launch message.";
+    return false;
+  }
+  Profile* profile = FetchProfileForDirectory(profile_dir);
+  if (!profile)
+    return false;
+
+  if (!LaunchApp(profile, app_id))
+    return false;
+
+  profile_ = profile;
+  app_id_ = app_id;
+
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
+                 content::Source<Profile>(profile_));
+  return true;
+}
+
+Profile* AppShimHost::FetchProfileForDirectory(const std::string& profile_dir) {
+  ProfileManager* profileManager = g_browser_process->profile_manager();
+  base::FilePath path(profile_dir);

[palmer, 2013/03/14 19:05:04]

Can we therefore have |pfile_dir| originate as a FilePath? Hmm, I suppose since
we are doing our own IPC mechanism we'd have to serialize and de-serialize it
ourselves.. so yeah perhaps doing it as a string is best?

[jeremya, 2013/03/14 23:33:01]

Not sure if base::FilePaths can be serialized over IPC... but I'm also not sure
what advantage doing that would have over passing a string.

+  path = profileManager->user_data_dir().Append(path);
+  ProfileInfoCache& cache = profileManager->GetProfileInfoCache();
+  if (cache.GetIndexOfProfileWithPath(path) == std::string::npos) {

[palmer, 2013/03/14 19:05:04]

So this line is the true security check? Is it worth adding a comment saying so?

[jeremya, 2013/03/14 23:33:01]

Done.

+    LOG(ERROR) << "Requested directory is not a known profile.";
+    return NULL;
+  }
+  Profile* profile = profileManager->GetProfile(path);

[palmer, 2013/03/14 19:05:04]

Is this kind of the same thing, effectively, as the GetIndexOfProfileWithPath
check? Or perhaps this is a better check — we should check that the client named
a profile that (a) exists as a pathname naming a directory on the filesystem,
and (b) that the directory is owned by the right user, and (c) that it really is
a Chrome profile and not /etc or something.

[jeremya, 2013/03/14 23:33:01]

The ProfileInfoCache stores info about the profiles that Chrome knows about, and
I'm pretty sure it can only be updated by either editing the local prefs
manually (in which case we're hosed security-wise anyway) or by the normal ways
of adding a profile (i.e. from the settings page). So it already ensures all
those things.

+  if (!profile) {
+    LOG(ERROR) << "Couldn't get profile for directory '" << profile_dir << "'.";
+    return NULL;
+  }
+  return profile;
+}
+
+bool AppShimHost::LaunchApp(Profile* profile, const std::string& app_id) {
+  extensions::ExtensionSystem* extension_system =
+      extensions::ExtensionSystem::Get(profile);
+  ExtensionServiceInterface* extension_service =
+      extension_system->extension_service();
+  const extensions::Extension* extension =
+      extension_service->GetExtensionById(
+          app_id, false);
+  if (!extension) {
+    LOG(ERROR) << "Attempted to launch nonexistent app with id '"
+               << app_id << "'.";
+    return false;
+  }
+  // TODO(jeremya): Handle the case that launching the app fails. Probably we
+  // need to watch for 'app successfully launched' or at least 'background page
+  // exists/was created' and time out with failure if we don't see that sign of
+  // life within a certain window.
+  chrome::AppLaunchParams params(profile,
+                                 extension,
+                                 extension_misc::LAUNCH_NONE,
+                                 NEW_WINDOW);
+  chrome::OpenApplication(params);
+  return true;
+}
+
+void AppShimHost::Observe(int type,
+                          const content::NotificationSource& source,
+                          const content::NotificationDetails& details) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(content::Source<Profile>(source).ptr() == profile_);
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED: {
+      extensions::ExtensionHost* extension_host =
+          content::Details<extensions::ExtensionHost>(details).ptr();
+      if (app_id_ == extension_host->extension_id())
+        Close();
+      break;
+    }
+    default:
+      NOTREACHED() << "Unexpected notification sent.";
+      break;
+  }
+}
+
+void AppShimHost::Close() {
+  DCHECK(CalledOnValidThread());
+  delete this;
+}