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

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 ac47f84af6f59cbb5b57379bf46c0b457dafed1c..0cca5e7f8bd8c002a2b4a598c9097bf8cd96fcc9 100644
--- a/remoting/host/elevated_controller_win.cc
+++ b/remoting/host/elevated_controller_win.cc
@@ -17,6 +17,7 @@
 #include "base/values.h"
 #include "base/win/scoped_handle.h"
 #include "remoting/host/branding.h"
+#include "remoting/host/daemon_controller_common_win.h"
 #include "remoting/host/elevated_controller_resource.h"
 #include "remoting/host/verify_config_window_win.h"
 
@@ -33,20 +34,20 @@ const FilePath::CharType kTempFileExtension[] = FILE_PATH_LITERAL("json~");
 const char16 kConfigFileSecurityDescriptor[] =
     TO_L_STRING("O:BAG:BAD:(A;;GA;;;SY)(A;;GA;;;BA)");
 
-// The maximum size of the configuration file. "1MB ought to be enough" for any
-// reasonable configuration we will ever need. 1MB is low enough to make
-// the probability of out of memory situation fairly low. OOM is still possible
-// and we will crash if it occurs.
-const size_t kMaxConfigFileSize = 1024 * 1024;
+const char16 kUnprivilegedConfigFileSecurityDescriptor[] =
+    TO_L_STRING("O:BAG:BAD:(A;;GA;;;SY)(A;;GA;;;BA)(A;;GR;;;AU)");
 
-// ReadConfig() filters the configuration file stripping all variables except of
-// the following two.
+// Configuration keys.
 const char kHostId[] = "host_id";
 const char kXmppLogin[] = "xmpp_login";
+const char kHostSecretHash[] = "host_secret_hash";
 
 // The configuration keys that cannot be specified in UpdateConfig().
 const char* const kReadonlyKeys[] = { kHostId, kXmppLogin };
 
+// The configuration keys whose values may be read by GetConfig().
+const char* const kUnprivilegedConfigKeys[] = { kHostId, kXmppLogin };

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

kReadonlyKeys and kUnprivilegedConfigKeys strangely match. It does not seem to
be a coincidence.  

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

Yes, but is it clear that the two sets will always be equal, even if we later
add new keys?

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

As per our discussion these lists are not the same. Both are while lists. Please
file a bug so we will not forget to make sure it is correctly documented and
documented in the code.

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

crbug.com/124825

+
 // Reads and parses the configuration file up to |kMaxConfigFileSize| in
 // size.
 HRESULT ReadConfig(const FilePath& filename,

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

Is ReadConfig used now, when GetCOnfig is implemented in the plugin?

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

No, but there's no need to use this CL to remove it.

I've added a TODO.

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

Please include this into the COM interfaces cleanup bug and assign it to me.

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

crbug.com/124937

@@ -69,8 +70,8 @@ HRESULT ReadConfig(const FilePath& filename,
     return HRESULT_FROM_WIN32(error);
   }
 
-  scoped_array<char> buffer(new char[kMaxConfigFileSize]);
-  DWORD size = kMaxConfigFileSize;
+  scoped_array<char> buffer(new char[remoting::kMaxConfigFileSize]);
+  DWORD size = remoting::kMaxConfigFileSize;
   if (!::ReadFile(file, &buffer[0], size, &size, NULL)) {
     DWORD error = GetLastError();
     LOG_GETLASTERROR(ERROR)
@@ -94,44 +95,18 @@ HRESULT ReadConfig(const FilePath& filename,
   return S_OK;
 }
 
-// Writes the configuration file up to |kMaxConfigFileSize| in size.
-HRESULT WriteConfig(const FilePath& filename,
-                    const char* content,
-                    size_t length) {
-  if (length > kMaxConfigFileSize) {
-      return E_FAIL;
-  }
-
-  // Extract the configuration data that the user will verify.
-  scoped_ptr<base::Value> config_value(base::JSONReader::Read(content));
-  if (!config_value.get()) {
-    return E_FAIL;
-  }
-  base::DictionaryValue* config_dict = NULL;
-  if (!config_value->GetAsDictionary(&config_dict)) {
-    return E_FAIL;
-  }
-  std::string email, host_id, host_secret_hash;
-  if (!config_dict->GetString("xmpp_login", &email) ||
-      !config_dict->GetString("host_id", &host_id) ||
-      !config_dict->GetString("host_secret_hash", &host_secret_hash)) {
-    return E_FAIL;
-  }
-
-  // Ask the user to verify the configuration.
-  remoting::VerifyConfigWindowWin verify_win(email, host_id, host_secret_hash);
-  if (!verify_win.Run()) {
-    return E_FAIL;
-  }
-
+HRESULT WriteConfigFile(bool privileged, const char* content, size_t length) {
   // Create a security descriptor for the configuration file.
   SECURITY_ATTRIBUTES security_attributes;
   security_attributes.nLength = sizeof(security_attributes);
   security_attributes.bInheritHandle = FALSE;
 
   ULONG security_descriptor_length = 0;
+  const char16* descriptor = privileged ?

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

I somehow feel that passing ACL and filename to this function is better than
passing a flag.

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

Done.

+                             kConfigFileSecurityDescriptor :
+                             kUnprivilegedConfigFileSecurityDescriptor;
   if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(
-           kConfigFileSecurityDescriptor,
+           descriptor,
            SDDL_REVISION_1,
            reinterpret_cast<PSECURITY_DESCRIPTOR*>(
                &security_attributes.lpSecurityDescriptor),
@@ -143,6 +118,10 @@ HRESULT WriteConfig(const FilePath& filename,
   }
 
   // Create a temporary file and write configuration to it.
+  const FilePath::CharType* basename = privileged ?
+                                       kConfigFileName :
+                                       remoting::kUnprivilegedConfigFileName;
+  FilePath filename = remoting::GetConfigDir().Append(basename);
   FilePath tempname = filename.ReplaceExtension(kTempFileExtension);
   {
     base::win::ScopedHandle file(
@@ -185,6 +164,62 @@ HRESULT WriteConfig(const FilePath& filename,
   return S_OK;
 }
 
+// Writes the configuration file up to |kMaxConfigFileSize| in size.
+HRESULT WriteConfig(const char* content, size_t length) {
+  if (length > remoting::kMaxConfigFileSize) {
+      return E_FAIL;
+  }
+
+  // Extract the configuration data that the user will verify.
+  scoped_ptr<base::Value> config_value(base::JSONReader::Read(content));
+  if (!config_value.get()) {
+    return E_FAIL;
+  }
+  base::DictionaryValue* config_dict = NULL;
+  if (!config_value->GetAsDictionary(&config_dict)) {
+    return E_FAIL;
+  }
+  std::string email, host_id, host_secret_hash;
+  if (!config_dict->GetString(kXmppLogin, &email) ||
+      !config_dict->GetString(kHostId, &host_id) ||
+      !config_dict->GetString(kHostSecretHash, &host_secret_hash)) {
+    return E_FAIL;
+  }
+
+  // Ask the user to verify the configuration.
+  remoting::VerifyConfigWindowWin verify_win(email, host_id, host_secret_hash);
+  if (!verify_win.Run()) {
+    return E_FAIL;
+  }
+
+  // Write the full configuration file.
+  HRESULT hr = WriteConfigFile(true, content, length);
+  if (FAILED(hr)) {
+    return hr;
+  }
+
+  // Extract the unprivileged fields from the configuration.
+  base::DictionaryValue unprivileged_config_dict;
+  for (int i = 0; i < arraysize(kUnprivilegedConfigKeys); ++i) {
+    const char* key = kUnprivilegedConfigKeys[i];
+    string16 value;
+    if (config_dict->GetString(key, &value)) {
+      unprivileged_config_dict.SetString(key, value);
+    }
+  }
+  std::string unprivileged_config_str;
+  base::JSONWriter::Write(&unprivileged_config_dict, &unprivileged_config_str);
+
+  // Write the unprivileged configuration file.
+  hr = WriteConfigFile(false,
+                       unprivileged_config_str.data(),
+                       unprivileged_config_str.size());
+  if (FAILED(hr)) {
+    return hr;
+  }
+
+  return S_OK;
+}
 
 } // namespace
 
@@ -244,9 +279,7 @@ STDMETHODIMP ElevatedControllerWin::SetConfig(BSTR config) {
   std::string file_content = UTF16ToUTF8(
     string16(static_cast<char16*>(config), ::SysStringLen(config)));
 
-  return WriteConfig(config_dir.Append(kConfigFileName),
-                     file_content.c_str(),
-                     file_content.size());
+  return WriteConfig(file_content.c_str(), file_content.size());
 }
 
 STDMETHODIMP ElevatedControllerWin::StartDaemon() {
@@ -359,9 +392,7 @@ STDMETHODIMP ElevatedControllerWin::UpdateConfig(BSTR config) {
   // Write the updated config.
   std::string config_updated_str;
   base::JSONWriter::Write(config_old.get(), &config_updated_str);
-  return WriteConfig(config_dir.Append(kConfigFileName),
-                     config_updated_str.c_str(),
-                     config_updated_str.size());
+  return WriteConfig(config_updated_str.c_str(), config_updated_str.size());
 }
 
 HRESULT ElevatedControllerWin::OpenService(ScopedScHandle* service_out) {