[Chromoting] Let the Windows daemon controller read the unprivileged part of the config without ele…

remoting/host/plugin/daemon_controller_win.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 "remoting/host/plugin/daemon_controller.h"
 
 #include <objbase.h>
+#include <sddl.h>

[alexeypa (please no reviews), 2012/04/23 17:35:10]

I don't think you need to include sddl.h here

[simonmorris, 2012/04/23 19:39:49]

Done.

 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/string16.h"
 #include "base/stringize_macros.h"
 #include "base/threading/thread.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "base/win/scoped_bstr.h"
 #include "base/win/scoped_comptr.h"
 #include "remoting/base/scoped_sc_handle_win.h"
 #include "remoting/host/branding.h"
+#include "remoting/host/daemon_controller_common_win.h"

[alexeypa (please no reviews), 2012/04/23 17:35:10]

Is it a new file? I don't see it in the CL.

[simonmorris, 2012/04/23 19:39:49]

Done.

 #include "remoting/host/plugin/daemon_installer_win.h"
 
 // MIDL-generated declarations and definitions.
 #include "remoting/host/elevated_controller.h"
 
 using base::win::ScopedBstr;
 using base::win::ScopedComPtr;
 
 namespace remoting {
 
@@ skipping 61 matching lines @@
   // The functions that actually do the work. They should be called in
   // the context of |worker_thread_|;
   void DoGetConfig(const GetConfigCallback& callback);
   void DoInstallAsNeededAndStart(scoped_ptr<base::DictionaryValue> config,
                                  const CompletionCallback& done_callback);
   void DoSetConfigAndStart(scoped_ptr<base::DictionaryValue> config,
                            const CompletionCallback& done_callback);
   void DoUpdateConfig(scoped_ptr<base::DictionaryValue> config,
                       const CompletionCallback& done_callback);
   void DoStop(const CompletionCallback& done_callback);
+  base::DictionaryValue* ReadConfig();
 
   // The worker thread used for servicing long running operations.
   ComThread worker_thread_;
 
   scoped_ptr<DaemonInstallerWin> installer_;
 
   DISALLOW_COPY_AND_ASSIGN(DaemonControllerWin);
 };
 
 ComThread::ComThread(const char* name) : base::Thread(name), control_(NULL) {
@@ skipping 185 matching lines @@
     return error;
   }
 
   service_out->Set(service.Take());
   return ERROR_SUCCESS;
 }
 
 void DaemonControllerWin::DoGetConfig(const GetConfigCallback& callback) {
   DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
 
-  IDaemonControl* control = NULL;
-  HRESULT hr = worker_thread_.ActivateElevatedController(&control);
-  if (FAILED(hr)) {
-    callback.Run(scoped_ptr<base::DictionaryValue>());
-    return;
-  }
-
-  // Get the host configuration.
-  ScopedBstr host_config;
-  hr = control->GetConfig(host_config.Receive());
-  if (FAILED(hr)) {
-    callback.Run(scoped_ptr<base::DictionaryValue>());
-    return;
-  }
-
-  string16 file_content(static_cast<BSTR>(host_config), host_config.Length());
-
-  // Parse the string into a dictionary.
-  scoped_ptr<base::Value> config(
-      base::JSONReader::Read(UTF16ToUTF8(file_content),
-          base::JSON_ALLOW_TRAILING_COMMAS));
-
-  base::DictionaryValue* dictionary;
-  if (config.get() == NULL || !config->GetAsDictionary(&dictionary)) {
-    callback.Run(scoped_ptr<base::DictionaryValue>());
-    return;
-  }
-
-  config.release();
-  callback.Run(scoped_ptr<base::DictionaryValue>(dictionary));
+  callback.Run(scoped_ptr<base::DictionaryValue>(ReadConfig()));

[alexeypa (please no reviews), 2012/04/23 17:35:10]

Why is a separate private method? DoXxx methods are the worker methods and
ReadConfig's code naturally belongs to DoGetConfig.

[simonmorris, 2012/04/23 19:39:49]

Only because ReadConfig forms a pair with WriteConfig. I don't mind merging
ReadConfig into DoGetConfig, if we're sure that WriteConfig doesn't belong in
this file.

[alexeypa (please no reviews), 2012/04/24 16:54:56]

WriteConfig belongs to the privileged code so I guess you can move the code back
to DoGetConfig

[simonmorris, 2012/04/25 00:33:32]

Done.

 }
 
 void DaemonControllerWin::DoInstallAsNeededAndStart(
     scoped_ptr<base::DictionaryValue> config,
     const CompletionCallback& done_callback) {
   DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
 
   IDaemonControl* control = NULL;
   HRESULT hr = worker_thread_.ActivateElevatedController(&control);
 
   // Just configure and start the Daemon Controller if it is installed already.
   if (SUCCEEDED(hr)) {
     DoSetConfigAndStart(config.Pass(), done_callback);
     return;
   }
 
-  // Otherwise, install it if it's COM registration entry is missing.
+  // Otherwise, install it if its COM registration entry is missing.
   if (hr == CO_E_CLASSSTRING) {
     scoped_ptr<DaemonInstallerWin> installer = DaemonInstallerWin::Create(
         base::Bind(&DaemonControllerWin::OnInstallationComplete,
                    base::Unretained(this),
                    base::Passed(&config),
                    done_callback));
     if (installer.get()) {
       DCHECK(!installer_.get());
       installer_ = installer.Pass();
       installer_->Install();
@@ skipping 73 matching lines @@
   HRESULT hr = worker_thread_.ActivateElevatedController(&control);
   if (FAILED(hr)) {
     done_callback.Run(HResultToAsyncResult(hr));
     return;
   }
 
   hr = control->StopDaemon();
   done_callback.Run(HResultToAsyncResult(hr));
 }
 
+// Reads and parses the configuration file up to |kMaxConfigFileSize| in
+// size. On error, it logs a message and returns NULL.
+base::DictionaryValue* DaemonControllerWin::ReadConfig() {
+  // Get the name of the configuration file.
+  FilePath dir = remoting::GetConfigDir();
+  FilePath filename = dir.Append(kUnprivilegedConfigFileName);

[alexeypa (please no reviews), 2012/04/23 17:35:10]

This is a leaking abstraction. The COM component is supposed to hide the details
like the name of configuration file and so on. Let's talk offline to figure out
a better packaging/abstraction for this code.

[alexeypa (please no reviews), 2012/04/24 16:54:56]

We have decided to file a bug to tracking fixing it. We will fix it in the
future if it cause enough pain.

[simonmorris, 2012/04/25 00:33:32]

crbug.com/124937

+
+  // Read raw data from the configuration file.
+  base::win::ScopedHandle file(
+      CreateFileW(filename.value().c_str(),
+                  GENERIC_READ,
+                  FILE_SHARE_READ | FILE_SHARE_WRITE,
+                  NULL,
+                  OPEN_EXISTING,
+                  FILE_FLAG_SEQUENTIAL_SCAN,
+                  NULL));
+
+  if (!file.IsValid()) {
+    DWORD error = GetLastError();
+    LOG_GETLASTERROR(ERROR)
+        << "Failed to open '" << filename.value() << "'";
+    return NULL;
+  }
+
+  scoped_array<char> buffer(new char[kMaxConfigFileSize]);
+  DWORD size = kMaxConfigFileSize;
+  if (!::ReadFile(file, &buffer[0], size, &size, NULL)) {
+    DWORD error = GetLastError();
+    LOG_GETLASTERROR(ERROR)
+        << "Failed to read '" << filename.value() << "'";
+    return NULL;
+  }
+
+  // Parse the JSON configuration, expecting it to contain a dictionary.
+  std::string file_content(buffer.get(), size);
+  scoped_ptr<base::Value> value(
+      base::JSONReader::Read(file_content, base::JSON_ALLOW_TRAILING_COMMAS));
+
+  base::DictionaryValue* dictionary = NULL;
+  if (value.get() == NULL || !value->GetAsDictionary(&dictionary)) {
+    LOG(ERROR) << "Failed to read '" << filename.value() << "'";
+    return NULL;
+  }
+  // Release value, because dictionary points to the same object.
+  value.release();
+  return dictionary;
+}
+
 }  // namespace
 
 scoped_ptr<DaemonController> remoting::DaemonController::Create() {
   return scoped_ptr<DaemonController>(new DaemonControllerWin());
 }
 
 }  // namespace remoting