Update remoting::HostConfig to use non-deprecated JSON methods

Bug: NO_BUG
Change-Id: I9eb09f684d8051c7a38af801a1ddfbf81061c40d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3213389
Reviewed-by: Yuwei Huang <[email protected]>
Commit-Queue: Joe Downing <[email protected]>
Cr-Commit-Position: refs/heads/main@{#929825}

remoting/host/daemon_process_win.cc

@@ -51,6 +51,7 @@
 #include "remoting/host/win/security_descriptor.h"
 #include "remoting/host/win/unprivileged_process_delegate.h"
 #include "remoting/host/win/worker_process_launcher.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 using base::win::ScopedHandle;
 
@@ -287,16 +288,14 @@ void DaemonProcessWin::SendHostConfigToNetworkProcess(
     LOG_IF(ERROR, !remoting_host_control_.is_connected())
         << "IPC channel not connected. HostConfig message will be dropped.";
 
-    std::unique_ptr<base::DictionaryValue> config(
-        HostConfigFromJson(serialized_config));
-    if (!config) {
+    absl::optional<base::Value> config(HostConfigFromJson(serialized_config));
+    if (!config.has_value()) {
       LOG(ERROR) << "Invalid host config, shutting down.";
       OnPermanentError(kInvalidHostConfigurationExitCode);
       return;
     }
 
-    remoting_host_control_->ApplyHostConfig(
-        base::Value::FromUniquePtrValue(std::move(config)));
+    remoting_host_control_->ApplyHostConfig(std::move(config.value()));
   }
 }
 

remoting/host/host_config.cc

@@ -28,37 +28,40 @@ const char kEnableVp9ConfigPath[] = "enable_vp9";
 const char kEnableH264ConfigPath[] = "enable_h264";
 const char kFrameRecorderBufferKbConfigPath[] = "frame-recorder-buffer-kb";
 
-std::unique_ptr<base::DictionaryValue> HostConfigFromJson(
-    const std::string& json) {
-  std::unique_ptr<base::Value> value =
-      base::JSONReader::ReadDeprecated(json, base::JSON_ALLOW_TRAILING_COMMAS);
-  if (!value || !value->is_dict()) {
-    LOG(WARNING) << "Failed to parse host config from JSON";
-    return nullptr;
+absl::optional<base::Value> HostConfigFromJson(const std::string& json) {
+  absl::optional<base::Value> value =
+      base::JSONReader::Read(json, base::JSON_ALLOW_TRAILING_COMMAS);
+  if (!value.has_value()) {
+    LOG(ERROR) << "Failed to parse host config from JSON";
+    return absl::nullopt;
   }
 
-  return base::WrapUnique(static_cast<base::DictionaryValue*>(value.release()));
+  if (!value->is_dict()) {
+    LOG(ERROR) << "Parsed host config returned was not a dictionary";
+    return absl::nullopt;
+  }
+
+  return value;
 }
 
-std::string HostConfigToJson(const base::DictionaryValue& host_config) {
+std::string HostConfigToJson(const base::Value& host_config) {
   std::string data;
   base::JSONWriter::Write(host_config, &data);
   return data;
 }
 
-std::unique_ptr<base::DictionaryValue> HostConfigFromJsonFile(
+absl::optional<base::Value> HostConfigFromJsonFile(
     const base::FilePath& config_file) {
-  // TODO(sergeyu): Implement better error handling here.
   std::string serialized;
   if (!base::ReadFileToString(config_file, &serialized)) {
-    LOG(WARNING) << "Failed to read " << config_file.value();
-    return nullptr;
+    LOG(ERROR) << "Failed to read " << config_file.value();
+    return absl::nullopt;
   }
 
   return HostConfigFromJson(serialized);
 }
 
-bool HostConfigToJsonFile(const base::DictionaryValue& host_config,
+bool HostConfigToJsonFile(const base::Value& host_config,
                           const base::FilePath& config_file) {
   std::string serialized = HostConfigToJson(host_config);
   return base::ImportantFileWriter::WriteFileAtomically(config_file,

remoting/host/host_config.h

@@ -8,9 +8,10 @@
 #include <memory>
 #include <string>
 
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 namespace base {
-class DictionaryValue;
+class Value;
 class FilePath;
 }  // namespace base
 
@@ -28,7 +29,7 @@ extern const char kHostOwnerConfigPath[];
 extern const char kHostOwnerEmailConfigPath[];
 // Login used to authenticate signaling.
 extern const char kXmppLoginConfigPath[];
-// OAuth refresh token used to fetch an access token for the XMPP network.
+// OAuth refresh token used to fetch an access token for calling network APIs.
 extern const char kOAuthRefreshTokenConfigPath[];
 // Unique identifier of the host used to register the host in directory.
 // Normally a random UUID.
@@ -48,15 +49,14 @@ extern const char kEnableH264ConfigPath[];
 // Number of Kibibytes of frame data to allow each client to record.
 extern const char kFrameRecorderBufferKbConfigPath[];
 
-// Helpers for serializing/deserializing Host configuration dictonaries.
-std::unique_ptr<base::DictionaryValue> HostConfigFromJson(
-    const std::string& serialized);
-std::string HostConfigToJson(const base::DictionaryValue& host_config);
+// Helpers for serializing/deserializing Host configuration dictionaries.
+absl::optional<base::Value> HostConfigFromJson(const std::string& serialized);
+std::string HostConfigToJson(const base::Value& host_config);
 
 // Helpers for loading/saving host configurations from/to files.
-std::unique_ptr<base::DictionaryValue> HostConfigFromJsonFile(
+absl::optional<base::Value> HostConfigFromJsonFile(
     const base::FilePath& config_file);
-bool HostConfigToJsonFile(const base::DictionaryValue& host_config,
+bool HostConfigToJsonFile(const base::Value& host_config,
                           const base::FilePath& config_file);
 
 }  // namespace remoting

remoting/host/host_config_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/values.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 namespace remoting {
 
@@ -53,54 +54,50 @@ TEST_F(HostConfigTest, Read) {
   ASSERT_TRUE(test_dir_.CreateUniqueTempDir());
   base::FilePath test_file = test_dir_.GetPath().AppendASCII("read.json");
   WriteTestFile(test_file);
-  std::unique_ptr<base::DictionaryValue> target(
-      HostConfigFromJsonFile(test_file));
-  ASSERT_TRUE(target);
-
-  std::string value;
-  EXPECT_TRUE(target->GetString(kXmppLoginConfigPath, &value));
-  EXPECT_EQ("[email protected]", value);
-  EXPECT_TRUE(target->GetString(kOAuthRefreshTokenConfigPath, &value));
-  EXPECT_EQ("TEST_REFRESH_TOKEN", value);
-  EXPECT_TRUE(target->GetString(kHostIdConfigPath, &value));
-  EXPECT_EQ("TEST_HOST_ID", value);
-  EXPECT_TRUE(target->GetString(kHostNameConfigPath, &value));
-  EXPECT_EQ("TEST_MACHINE_NAME", value);
-  EXPECT_TRUE(target->GetString(kPrivateKeyConfigPath, &value));
-  EXPECT_EQ("TEST_PRIVATE_KEY", value);
-
-  EXPECT_FALSE(target->GetString("non_existent_value", &value));
+  absl::optional<base::Value> target(HostConfigFromJsonFile(test_file));
+  ASSERT_TRUE(target.has_value());
+
+  std::string* value = target->FindStringKey(kXmppLoginConfigPath);
+  EXPECT_EQ("[email protected]", *value);
+  value = target->FindStringKey(kOAuthRefreshTokenConfigPath);
+  EXPECT_EQ("TEST_REFRESH_TOKEN", *value);
+  value = target->FindStringKey(kHostIdConfigPath);
+  EXPECT_EQ("TEST_HOST_ID", *value);
+  value = target->FindStringKey(kHostNameConfigPath);
+  EXPECT_EQ("TEST_MACHINE_NAME", *value);
+  value = target->FindStringKey(kPrivateKeyConfigPath);
+  EXPECT_EQ("TEST_PRIVATE_KEY", *value);
+
+  value = target->FindStringKey("non_existent_value");
+  EXPECT_EQ(nullptr, value);
 }
 
 TEST_F(HostConfigTest, Write) {
   ASSERT_TRUE(test_dir_.CreateUniqueTempDir());
 
   base::FilePath test_file = test_dir_.GetPath().AppendASCII("write.json");
   WriteTestFile(test_file);
-  std::unique_ptr<base::DictionaryValue> target(
-      HostConfigFromJsonFile(test_file));
-  ASSERT_TRUE(target);
+  absl::optional<base::Value> target(HostConfigFromJsonFile(test_file));
+  ASSERT_TRUE(target.has_value());
 
   std::string new_refresh_token_value = "NEW_REFRESH_TOKEN";
-  target->SetString(kOAuthRefreshTokenConfigPath, new_refresh_token_value);
+  target->SetStringKey(kOAuthRefreshTokenConfigPath, new_refresh_token_value);
   ASSERT_TRUE(HostConfigToJsonFile(*target, test_file));
 
   // Now read the file again and check that the value has been written.
-  std::unique_ptr<base::DictionaryValue> reader(
-      HostConfigFromJsonFile(test_file));
+  absl::optional<base::Value> reader(HostConfigFromJsonFile(test_file));
   ASSERT_TRUE(reader);
 
-  std::string value;
-  EXPECT_TRUE(reader->GetString(kXmppLoginConfigPath, &value));
-  EXPECT_EQ("[email protected]", value);
-  EXPECT_TRUE(reader->GetString(kOAuthRefreshTokenConfigPath, &value));
-  EXPECT_EQ(new_refresh_token_value, value);
-  EXPECT_TRUE(reader->GetString(kHostIdConfigPath, &value));
-  EXPECT_EQ("TEST_HOST_ID", value);
-  EXPECT_TRUE(reader->GetString(kHostNameConfigPath, &value));
-  EXPECT_EQ("TEST_MACHINE_NAME", value);
-  EXPECT_TRUE(reader->GetString(kPrivateKeyConfigPath, &value));
-  EXPECT_EQ("TEST_PRIVATE_KEY", value);
+  std::string* value = target->FindStringKey(kXmppLoginConfigPath);
+  EXPECT_EQ("[email protected]", *value);
+  value = target->FindStringKey(kOAuthRefreshTokenConfigPath);
+  EXPECT_EQ(new_refresh_token_value, *value);
+  value = target->FindStringKey(kHostIdConfigPath);
+  EXPECT_EQ("TEST_HOST_ID", *value);
+  value = target->FindStringKey(kHostNameConfigPath);
+  EXPECT_EQ("TEST_MACHINE_NAME", *value);
+  value = target->FindStringKey(kPrivateKeyConfigPath);
+  EXPECT_EQ("TEST_PRIVATE_KEY", *value);
 }
 
 }  // namespace remoting

remoting/host/remoting_me2me_host.cc

@@ -104,6 +104,7 @@
 #include "remoting/signaling/ftl_signal_strategy.h"
 #include "remoting/signaling/remoting_log_to_server.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 #include "third_party/webrtc/api/scoped_refptr.h"
 
 #if defined(OS_POSIX)
@@ -609,15 +610,14 @@ bool HostProcess::InitWithCommandLine(const base::CommandLine* cmd_line) {
 void HostProcess::OnConfigUpdated(const std::string& serialized_config) {
   HOST_LOG << "Parsing new host configuration.";
 
-  std::unique_ptr<base::DictionaryValue> config(
-      HostConfigFromJson(serialized_config));
-  if (!config) {
+  absl::optional<base::Value> config(HostConfigFromJson(serialized_config));
+  if (!config.has_value()) {
     LOG(ERROR) << "Invalid configuration.";
     ShutdownHost(kInvalidHostConfigurationExitCode);
     return;
   }
 
-  OnConfigParsed(base::Value::FromUniquePtrValue(std::move(config)));
+  OnConfigParsed(std::move(config.value()));
 }
 
 void HostProcess::OnConfigParsed(base::Value config) {

remoting/host/setup/daemon_controller_delegate_linux.cc

@@ -27,6 +27,7 @@
 #include "remoting/base/file_path_util_linux.h"
 #include "remoting/host/host_config.h"
 #include "remoting/host/usage_stats_consent.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 namespace remoting {
 
@@ -146,19 +147,22 @@ DaemonController::State DaemonControllerDelegateLinux::GetState() {
 
 std::unique_ptr<base::DictionaryValue>
 DaemonControllerDelegateLinux::GetConfig() {
-  std::unique_ptr<base::DictionaryValue> config(
+  absl::optional<base::Value> host_config(
       HostConfigFromJsonFile(GetConfigPath()));
-  if (!config)
+  if (!host_config.has_value())
     return nullptr;
 
-  std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue());
-  std::string value;
-  if (config->GetString(kHostIdConfigPath, &value)) {
-    result->SetString(kHostIdConfigPath, value);
+  std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue);
+  std::string* value = host_config->FindStringKey(kHostIdConfigPath);
+  if (value) {
+    result->SetString(kHostIdConfigPath, *value);
   }
-  if (config->GetString(kXmppLoginConfigPath, &value)) {
-    result->SetString(kXmppLoginConfigPath, value);
+
+  value = host_config->FindStringKey(kXmppLoginConfigPath);
+  if (value) {
+    result->SetString(kXmppLoginConfigPath, *value);
   }
+
   return result;
 }
 
@@ -213,11 +217,17 @@ void DaemonControllerDelegateLinux::SetConfigAndStart(
 void DaemonControllerDelegateLinux::UpdateConfig(
     std::unique_ptr<base::DictionaryValue> config,
     DaemonController::CompletionCallback done) {
-  std::unique_ptr<base::DictionaryValue> new_config(
+  absl::optional<base::Value> new_config(
       HostConfigFromJsonFile(GetConfigPath()));
-  if (new_config)
-    new_config->MergeDictionary(config.get());
-  if (!new_config || !HostConfigToJsonFile(*new_config, GetConfigPath())) {
+  if (!new_config.has_value()) {
+    LOG(ERROR) << "Failed to read existing config file.";
+    std::move(done).Run(DaemonController::RESULT_FAILED);
+    return;
+  }
+
+  new_config->MergeDictionary(config.get());
+
+  if (!HostConfigToJsonFile(new_config.value(), GetConfigPath())) {
     LOG(ERROR) << "Failed to update config file.";
     std::move(done).Run(DaemonController::RESULT_FAILED);
     return;

remoting/host/setup/daemon_controller_delegate_mac.mm

@@ -29,6 +29,7 @@
 #include "remoting/host/mac/permission_checker.h"
 #include "remoting/host/mac/permission_wizard.h"
 #include "remoting/host/resources.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/l10n/l10n_util_mac.h"
 
@@ -234,17 +235,21 @@ void ElevateAndStopHost(DaemonController::CompletionCallback done) {
 std::unique_ptr<base::DictionaryValue>
 DaemonControllerDelegateMac::GetConfig() {
   base::FilePath config_path(kHostConfigFilePath);
-  std::unique_ptr<base::DictionaryValue> host_config(
-      HostConfigFromJsonFile(config_path));
-  if (!host_config)
+  absl::optional<base::Value> host_config(HostConfigFromJsonFile(config_path));
+  if (!host_config.has_value())
     return nullptr;
 
   std::unique_ptr<base::DictionaryValue> config(new base::DictionaryValue);
-  std::string value;
-  if (host_config->GetString(kHostIdConfigPath, &value))
-    config->SetString(kHostIdConfigPath, value);
-  if (host_config->GetString(kXmppLoginConfigPath, &value))
-    config->SetString(kXmppLoginConfigPath, value);
+  std::string* value = host_config->FindStringKey(kHostIdConfigPath);
+  if (value) {
+    config->SetString(kHostIdConfigPath, *value);
+  }
+
+  value = host_config->FindStringKey(kXmppLoginConfigPath);
+  if (value) {
+    config->SetString(kXmppLoginConfigPath, *value);
+  }
+
   return config;
 }
 
@@ -271,15 +276,16 @@ void ElevateAndStopHost(DaemonController::CompletionCallback done) {
     std::unique_ptr<base::DictionaryValue> config,
     DaemonController::CompletionCallback done) {
   base::FilePath config_file_path(kHostConfigFilePath);
-  std::unique_ptr<base::DictionaryValue> host_config(
+  absl::optional<base::Value> host_config(
       HostConfigFromJsonFile(config_file_path));
-  if (!host_config) {
+  if (!host_config.has_value()) {
     std::move(done).Run(DaemonController::RESULT_FAILED);
     return;
   }
 
   host_config->MergeDictionary(config.get());
-  ElevateAndSetConfig(*host_config, std::move(done));
+  ElevateAndSetConfig(base::Value::AsDictionaryValue(host_config.value()),
+                      std::move(done));
 }
 
 void DaemonControllerDelegateMac::Stop(
@@ -296,10 +302,14 @@ void ElevateAndStopHost(DaemonController::CompletionCallback done) {
   consent.set_by_policy = false;
 
   base::FilePath config_file_path(kHostConfigFilePath);
-  std::unique_ptr<base::DictionaryValue> host_config(
+  absl::optional<base::Value> host_config(
       HostConfigFromJsonFile(config_file_path));
-  if (host_config) {
-    host_config->GetBoolean(kUsageStatsConsentConfigPath, &consent.allowed);
+  if (host_config.has_value()) {
+    absl::optional<bool> host_config_value =
+        host_config->FindBoolKey(kUsageStatsConsentConfigPath);
+    if (host_config_value.has_value()) {
+      consent.allowed = host_config_value.value();
+    }
   }
 
   return consent;