The me2me host is now configurable from the web UI on Windows.

remoting/host/elevated_controller_win.cc

Index: remoting/host/elevated_controller_win.cc
diff --git a/remoting/host/elevated_controller_win.cc b/remoting/host/elevated_controller_win.cc
index 708bed29f74a240cd0aeb60d4cdd150359838128..aad59ba7ec7e83050ee6174119e970658e63e2b0 100644
--- a/remoting/host/elevated_controller_win.cc
+++ b/remoting/host/elevated_controller_win.cc
@@ -4,11 +4,87 @@
 
 #include "remoting/host/elevated_controller_win.h"
 
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/path_service.h"
+#include "base/utf_string_conversions.h"
+#include "base/values.h"
+#include "base/win/scoped_handle.h"
+#include "remoting/host/branding.h"
+
 // MIDL-generated definitions.
-#include <elevated_controller_i.c>
+#include "elevated_controller_i.c"
+
+namespace {
+
+// TODO(alexeypa): remove this limitation.
+const size_t kMaxConfigFileSize = 0x10000;

[Wez, 2012/03/30 22:11:01]

nit: Specifying the max file size in hex makes it less obvious to the casual
reader what the limit is!

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+
+const char kHostId[] = "host_id";
+const char kXmppLogin[] = "xmpp_login";

[Wez, 2012/03/30 22:11:01]

nit: Add a comment to explain what these constants are for.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+
+// Names of the configuration files.
+const FilePath::CharType kAuthConfig[] = FILE_PATH_LITERAL("auth.json");

[Wez, 2012/03/30 22:11:01]

nit: These should really be kAuthConfig -> kAuthConfigFilename.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+const FilePath::CharType kHostConfig[] = FILE_PATH_LITERAL("host.json");
+
+// TODO(alexeypa): remove the hardcoded undocimented paths and store

[Wez, 2012/03/30 22:11:01]

typos: remove -> Remove, undocimented.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+// the configuration in the registry.
+#ifdef OFFICIAL_BUILD
+const FilePath::CharType kConfigDir[] = FILE_PATH_LITERAL(
+    "config\\systemprofile\\AppData\\Local\\Google\\Chrome Remote Desktop");
+#else
+const FilePath::CharType kConfigDir[] =
+    FILE_PATH_LITERAL("config\\systemprofile\\AppData\\Local\\Chromoting");
+#endif
+
+// Reads and parses a JSON configuration file (up to 64KB in size).

[Wez, 2012/03/30 22:11:01]

nit: The "up to 64KB in size" comment goes with the definition of
kMaxConfigFileSize. If kMaxConfigFileSize only applies to this function,
consider defining it local to the function.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+HRESULT ReadConfig(const FilePath& filename,
+                   scoped_ptr<base::DictionaryValue>* config_out) {
+  // 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 read '" << filename.value() << "'";
+    return HRESULT_FROM_WIN32(error);
+  }
+
+  std::vector<char> buffer(kMaxConfigFileSize);
+  DWORD size = static_cast<DWORD>(buffer.size());
+  if (!::ReadFile(file, &buffer[0], size, &size, NULL)) {
+    DWORD error = GetLastError();
+    LOG_GETLASTERROR(ERROR)
+        << "Failed to read '" << filename.value() << "'";
+    return HRESULT_FROM_WIN32(error);
+  }
 
-using ATL::CComQIPtr;
-using ATL::CComPtr;
+  // Parse JSON data.

[Wez, 2012/03/30 22:11:01]

nit: "Parse the JSON configuration, expecting it to contain a dictionary."

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  std::string file_content(&buffer[0], size);
+  scoped_ptr<base::Value> value(base::JSONReader::Read(file_content, true));
+
+  base::DictionaryValue* dictionary;
+  if (value.get() == NULL || !value->GetAsDictionary(&dictionary)) {
+    LOG(ERROR) << "Failed to read '" << filename.value() << "'.";
+    return E_FAIL;
+  }
+
+  value.release();
+  config_out->reset(dictionary);

[Wez, 2012/03/30 22:11:01]

Gah. Gross.  Can't see a better way to do it, off the top of my head, though,
short of adding TakeAsDictionary() to base::Value. :(

+  return S_OK;
+}
+
+} // namespace
 
 namespace remoting {
 
@@ -22,52 +98,143 @@ HRESULT ElevatedControllerWin::FinalConstruct() {
 void ElevatedControllerWin::FinalRelease() {
 }
 
-STDMETHODIMP ElevatedControllerWin::get_State(DaemonState* state_out) {
-  return E_NOTIMPL;
-}
+STDMETHODIMP ElevatedControllerWin::GetConfig(BSTR* config_out) {
+  FilePath system_profile;
+  PathService::Get(base::DIR_SYSTEM, &system_profile);
+
+  // Build the host configuration.

[Wez, 2012/03/30 22:11:01]

nit: Isn't this reading it, rather than "building" it?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  scoped_ptr<base::DictionaryValue> config;
+  HRESULT hr = ReadConfig(system_profile.Append(kConfigDir).Append(kHostConfig),
+                          &config);
+  if (FAILED(hr)) {
+    return hr;
+  }
 
-STDMETHODIMP ElevatedControllerWin::ReadConfig(BSTR* config_out) {
-  return E_NOTIMPL;
+  // Build the filtered config.

[Wez, 2012/03/30 22:11:01]

Why?  What is the purpose of the filtering?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

This matched Mac implementation.

[Lambros, 2012/03/31 00:48:34]

GetConfig() is documented to return only certain keys, and not to include
private information.  That's the purpose of the filtering, if that helps.

[Wez, 2012/04/01 01:25:27]

Right; my point is that the comment should clarify that e.g. "GetConfig is
defined to return only a sub-set of the configuration." or "Filter the
configuration (see GetConfig definition)."

[alexeypa (please no reviews), 2012/04/02 16:26:39]

Oh, well. I'll try to address this in one of the follow up CL.

+  scoped_ptr<base::DictionaryValue> filtered_config(
+      new base::DictionaryValue());
+
+  std::string value;
+  if (config->GetString(kHostId, &value)) {
+    filtered_config->SetString(kHostId, value);
+  }
+

[Wez, 2012/03/30 22:11:01]

nit: No need for this blank line.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  if (config->GetString(kXmppLogin, &value)) {
+    filtered_config->SetString(kXmppLogin, value);
+  }
+
+  // Convert the filtered config back to string and return it to the caller.

[Wez, 2012/03/30 22:11:01]

typo: ... back to a string ...

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  std::string file_content;
+  base::JSONWriter::Write(filtered_config.get(), &file_content);
+
+  *config_out = ::SysAllocString(UTF8ToUTF16(file_content).c_str());
+  if (config_out == NULL) {
+    return E_OUTOFMEMORY;
+  }
+
+  return S_OK;
 }
 
-STDMETHODIMP ElevatedControllerWin::WriteConfig(BSTR config) {
-  return E_NOTIMPL;
+STDMETHODIMP ElevatedControllerWin::SetConfig(BSTR config) {
+  FilePath system_profile;

[Wez, 2012/03/30 22:11:01]

nit: Comment before these lines e.g. "Determine the config directory path and
create it if necessary".  May want to comment on the security guarantees this
has, in terms of who can read the config.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  PathService::Get(base::DIR_SYSTEM, &system_profile);
+

[Wez, 2012/03/30 22:11:01]

nit: No need for this blank line?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Removed. My eyes hurt now! :-)

+  FilePath config_dir = system_profile.Append(kConfigDir);
+  if (!file_util::CreateDirectory(config_dir)) {
+    return HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED);
+  }
+
+  std::string file_content = UTF16ToUTF8(
+    string16(static_cast<char16*>(config), ::SysStringLen(config)));
+
+  int written = file_util::WriteFile(

[Wez, 2012/03/30 22:11:01]

Should we be doing the
write-temp-file-then-delete-old-file-then-rename-temp-to-be-new-config dance
here?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

We could. However the whole configuration flow is so not transactional that at
this point it does not matter if we fail half way into the process.

+      config_dir.Append(kAuthConfig),
+      file_content.c_str(),
+      file_content.size());
+
+  if (written != static_cast<int>(file_content.size())) {
+    return HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED);
+  }
+
+  // TODO(alexeypa): make it a single file.

[Wez, 2012/03/30 22:11:01]

nit: I think you mean "Store the authentication and host configurations in a
single file"?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  written = file_util::WriteFile(
+      config_dir.Append(kHostConfig),
+      file_content.c_str(),
+      file_content.size());
+
+  if (written != static_cast<int>(file_content.size())) {
+    return HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED);

[Wez, 2012/03/30 22:11:01]

nit: That's not strictly the right error, and in fact access-denied would lead
me to believe that the config file wasn't touch, whereas it will have been
written truncated data?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Nope. I bet something else bad happened. I replaced it E_FAIL which means
exactly that. Broadly. :-)

+  }
+
+  return S_OK;
 }
 
 STDMETHODIMP ElevatedControllerWin::StartDaemon() {
-  return E_NOTIMPL;
-}
+  ScopedScHandle service;
+  HRESULT hr = OpenService(&service);
+  if (FAILED(hr)) {
+    return hr;
+  }
 
-STDMETHODIMP ElevatedControllerWin::StopDaemon() {
-  return E_NOTIMPL;
+  if (!StartService(service, 0, NULL)) {
+    DWORD error = GetLastError();
+    if (error != ERROR_SERVICE_ALREADY_RUNNING) {
+      LOG_GETLASTERROR(ERROR)
+          << "Failed to start the '" << kWindowsServiceName << "'service";
+
+      return HRESULT_FROM_WIN32(error);
+    }
+  }
+
+  return S_OK;
 }
 
-HRESULT ElevatedControllerWin::FireOnStateChange(DaemonState state) {
-  CComPtr<IConnectionPoint> connection_point;
-  FindConnectionPoint(__uuidof(IDaemonEvents), &connection_point);
-  if (!connection_point) {
-    return S_OK;
+STDMETHODIMP ElevatedControllerWin::StopDaemon() {
+  ScopedScHandle service;
+  HRESULT hr = OpenService(&service);
+  if (FAILED(hr)) {
+    return hr;

[Wez, 2012/03/30 22:11:01]

Do you actually want to return potentially arbitrary return-codes from the
service control manager from these methods? It might be cleaner to continue to
log the error with LOG_GETLASTERROR() as you do below but return something
better defined to the caller.

I assume you're converting Win32->HRESULT to avoid GetLastError() getting
trampled during logging.  One slight oddity of this arrangement is that APIs
returning an invalid value but leaving GetLastError() set to zero will be
treated as having succeeded.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

I don't think so. Something better defined is "operation failed"/"succeeded". OS
codes deliver this message and provide a possibility to display a more
meaningful error message to the user (potentially).
No. COM methods should return HRESULT error by convention.
GetLastError() is purely Win32 concept. It usually does not apply to
intra-application interfaces. It also typically applies only to functions
returned a boolean instead of error code.

[Wez, 2012/04/01 01:25:27]

The word "here" was missing from my sentence; I thought it might be cleaner to
do the conversion later, except for the GetLastError() trampling issue.
 
Yes; my point is that an API could return FALSE (i.e. error) but GetLastError()
be ERROR_SUCCESS, in which case this code will behave as if the call had
succeeded.  Of course APIs shouldn't fail in that way...

[alexeypa (please no reviews), 2012/04/02 16:26:39]

That is not the only reason. I don't think that using GetLastError() for
anything that is not Win32 API is a good idea. There should be a single source
of the error status to avoid issues like the one you are describing below.
Well, this can definitely happen in practice but it happens so rare that it can
be fixed on case-by-case basic.

   }
 
-  CComPtr<IEnumConnections> connections;
-  if (FAILED(connection_point->EnumConnections(&connections))) {
-    return S_OK;
+  SERVICE_STATUS status;
+  if (!ControlService(service, SERVICE_CONTROL_STOP, &status)) {
+    DWORD error = GetLastError();
+    if (error != ERROR_SERVICE_NOT_ACTIVE) {
+      LOG_GETLASTERROR(ERROR)
+          << "Failed to stop the '" << kWindowsServiceName << "'service";
+      return HRESULT_FROM_WIN32(error);
+    }
   }
 
-  CONNECTDATA connect_data;
-  while (connections->Next(1, &connect_data, NULL) == S_OK) {
-    if (connect_data.pUnk != NULL) {
-      CComQIPtr<IDaemonEvents, &__uuidof(IDaemonEvents)> sink(
-          connect_data.pUnk);
+  return S_OK;
+}
+
+HRESULT ElevatedControllerWin::OpenService(ScopedScHandle* service_out) {
+  DWORD error;
 
-      if (sink != NULL) {
-        sink->OnStateChange(state);
-      }
+  ScopedScHandle scmanager(
+      ::OpenSCManagerW(NULL, SERVICES_ACTIVE_DATABASE,
+                       SC_MANAGER_CONNECT | SC_MANAGER_ENUMERATE_SERVICE));
+  if (!scmanager.IsValid()) {
+    error = GetLastError();
+    LOG_GETLASTERROR(ERROR)
+        << "Failed to connect to the service control manager";
 
-      connect_data.pUnk->Release();
-    }
+    return HRESULT_FROM_WIN32(error);
+  }
+
+  ScopedScHandle service(
+      ::OpenServiceA(scmanager, kWindowsServiceName,

[Wez, 2012/03/30 22:11:01]

nit: Why are we using OpenServiceA but OpenSCManagerW?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+                     SERVICE_QUERY_STATUS | SERVICE_START | SERVICE_STOP));
+  if (!service.IsValid()) {
+    error = GetLastError();
+    LOG_GETLASTERROR(ERROR)
+        << "Failed to open to the '" << kWindowsServiceName << "' service";
+
+    return HRESULT_FROM_WIN32(error);
   }
 
+  service_out->Set(service.Take());
   return S_OK;
 }