Disallow removing policy network configuration

This CL changes the behaviour of
ManagedNetworkConfigurationHandler::RemoveConfiguration to respect
network configurations done by policy by preventing removing them.

Bug: 470104
Change-Id: Ica30268a979c27084a58fd86745e613de10fce51
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410865Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Omar Morsi <omorsi@google.com>
Commit-Queue: Omar Morsi <omorsi@google.com>
Cr-Commit-Position: refs/heads/master@{#808398}

chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc

@@ -8,6 +8,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/memory/ptr_util.h"
+#include "base/optional.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
@@ -185,8 +186,8 @@ class VpnProviderApiTest : public extensions::ExtensionApiTest {
 
   void TriggerInternalRemove() {
     NetworkHandler::Get()->network_configuration_handler()->RemoveConfiguration(
-        GetSingleServicePath(), base::DoNothing(),
-        base::Bind(DoNothingFailureCallback));
+        GetSingleServicePath(), /*remove_confirmer=*/base::nullopt,
+        base::DoNothing(), base::Bind(DoNothingFailureCallback));
   }
 
  protected:

chromeos/network/managed_network_configuration_handler.h

@@ -159,6 +159,18 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) ManagedNetworkConfigurationHandler {
       const std::string& profile_path,
       ::onc::ONCSource* onc_source) const = 0;
 
+  // Returns true if the network with |guid| is configured by device or user
+  // policy for profile |profile_path|.
+  virtual bool IsNetworkConfiguredByPolicy(
+      const std::string& guid,
+      const std::string& profile_path) const = 0;
+
+  // Returns true if the configuration of the network with |guid| is not
+  // managed by policy for profile with |profile_path| and thus can be removed.
+  virtual bool CanRemoveNetworkConfig(
+      const std::string& guid,
+      const std::string& profile_path) const = 0;
+
   // Return true if the AllowOnlyPolicyNetworksToConnect policy is enabled.
   virtual bool AllowOnlyPolicyNetworksToConnect() const = 0;
 

chromeos/network/managed_network_configuration_handler_impl.cc

@@ -458,7 +458,11 @@ void ManagedNetworkConfigurationHandlerImpl::RemoveConfiguration(
     base::OnceClosure callback,
     network_handler::ErrorCallback error_callback) const {
   network_configuration_handler_->RemoveConfiguration(
-      service_path, std::move(callback), std::move(error_callback));
+      service_path,
+      base::BindRepeating(
+          &ManagedNetworkConfigurationHandlerImpl::CanRemoveNetworkConfig,
+          base::Unretained(this)),
+      std::move(callback), std::move(error_callback));
 }
 
 void ManagedNetworkConfigurationHandlerImpl::
@@ -777,6 +781,19 @@ ManagedNetworkConfigurationHandlerImpl::FindPolicyByGuidAndProfile(
   return policy;
 }
 
+bool ManagedNetworkConfigurationHandlerImpl::IsNetworkConfiguredByPolicy(
+    const std::string& guid,
+    const std::string& profile_path) const {
+  ::onc::ONCSource onc_source = ::onc::ONC_SOURCE_UNKNOWN;
+  return FindPolicyByGUID(guid, profile_path, &onc_source) != nullptr;
+}
+
+bool ManagedNetworkConfigurationHandlerImpl::CanRemoveNetworkConfig(
+    const std::string& guid,
+    const std::string& profile_path) const {
+  return !IsNetworkConfiguredByPolicy(guid, profile_path);
+}
+
 bool ManagedNetworkConfigurationHandlerImpl::AllowOnlyPolicyNetworksToConnect()
     const {
   const base::DictionaryValue* global_network_config =

chromeos/network/managed_network_configuration_handler_impl.h

@@ -95,6 +95,13 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) ManagedNetworkConfigurationHandlerImpl
       const std::string& profile_path,
       ::onc::ONCSource* onc_source) const override;
 
+  bool IsNetworkConfiguredByPolicy(
+      const std::string& guid,
+      const std::string& profile_path) const override;
+
+  bool CanRemoveNetworkConfig(const std::string& guid,
+                              const std::string& profile_path) const override;
+
   bool AllowOnlyPolicyNetworksToConnect() const override;
   bool AllowOnlyPolicyNetworksToConnectIfAvailable() const override;
   bool AllowOnlyPolicyNetworksToAutoconnect() const override;

chromeos/network/mock_managed_network_configuration_handler.h

@@ -56,11 +56,11 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) MockManagedNetworkConfigurationHandler
                     const base::ListValue& network_configs_onc,
                     const base::DictionaryValue& global_network_config));
   MOCK_CONST_METHOD0(IsAnyPolicyApplicationRunning, bool());
-  MOCK_CONST_METHOD3(FindPolicyByGUID,
-                     const base::DictionaryValue*(
-                         const std::string userhash,
-                         const std::string& guid,
-                         ::onc::ONCSource* onc_source));
+  MOCK_CONST_METHOD3(
+      FindPolicyByGUID,
+      const base::DictionaryValue*(const std::string userhash,
+                                   const std::string& guid,
+                                   ::onc::ONCSource* onc_source));
   MOCK_CONST_METHOD1(GetNetworkConfigsFromPolicy,
                      const GuidToPolicyMap*(const std::string& userhash));
   MOCK_CONST_METHOD1(GetGlobalConfigFromPolicy,
@@ -70,6 +70,12 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) MockManagedNetworkConfigurationHandler
       const base::DictionaryValue*(const std::string& guid,
                                    const std::string& profile_path,
                                    ::onc::ONCSource* onc_source));
+  MOCK_CONST_METHOD2(IsNetworkConfiguredByPolicy,
+                     bool(const std::string& guid,
+                          const std::string& profile_path));
+  MOCK_CONST_METHOD2(CanRemoveNetworkConfig,
+                     bool(const std::string& guid,
+                          const std::string& profile_path));
   MOCK_CONST_METHOD0(AllowOnlyPolicyNetworksToConnect, bool());
   MOCK_CONST_METHOD0(AllowOnlyPolicyNetworksToConnectIfAvailable, bool());
   MOCK_CONST_METHOD0(AllowOnlyPolicyNetworksToAutoconnect, bool());

chromeos/network/network_configuration_handler.cc

@@ -17,6 +17,7 @@
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/optional.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
@@ -63,9 +64,8 @@ void SetNetworkProfileErrorCallback(
       std::move(error_callback), dbus_error_name, dbus_error_message);
 }
 
-void ManagerSetPropertiesErrorCallback(
-    const std::string& dbus_error_name,
-    const std::string& dbus_error_message) {
+void ManagerSetPropertiesErrorCallback(const std::string& dbus_error_name,
+                                       const std::string& dbus_error_message) {
   network_handler::ShillErrorCallbackFunction(
       "ShillManagerClient.SetProperties Failed", std::string(),
       base::NullCallback(), dbus_error_name, dbus_error_message);
@@ -104,11 +104,13 @@ class NetworkConfigurationHandler::ProfileEntryDeleter {
   ProfileEntryDeleter(NetworkConfigurationHandler* handler,
                       const std::string& service_path,
                       const std::string& guid,
+                      base::Optional<RemoveConfirmer> remove_confirmer,
                       base::OnceClosure callback,
                       network_handler::ErrorCallback error_callback)
       : owner_(handler),
         service_path_(service_path),
         guid_(guid),
+        remove_confirmer_(std::move(remove_confirmer)),
         callback_(std::move(callback)),
         error_callback_(std::move(error_callback)) {}
 
@@ -160,6 +162,11 @@ class NetworkConfigurationHandler::ProfileEntryDeleter {
         continue;
       }
 
+      if (remove_confirmer_.has_value() &&
+          !remove_confirmer_->Run(guid_, profile_path)) {
+        continue;
+      }
+
       NET_LOG(DEBUG) << "Delete Profile Entry: " << profile_path << ": "
                      << entry_path;
       profile_delete_entries_[profile_path] = entry_path;
@@ -221,6 +228,7 @@ class NetworkConfigurationHandler::ProfileEntryDeleter {
   // value is the profile path of the profile in question.
   std::string restrict_to_profile_path_;
   std::string guid_;
+  base::Optional<RemoveConfirmer> remove_confirmer_;
   base::OnceClosure callback_;
   network_handler::ErrorCallback error_callback_;
 
@@ -381,9 +389,11 @@ void NetworkConfigurationHandler::CreateShillConfiguration(
 
 void NetworkConfigurationHandler::RemoveConfiguration(
     const std::string& service_path,
+    base::Optional<RemoveConfirmer> remove_confirmer,
     base::OnceClosure callback,
     network_handler::ErrorCallback error_callback) {
-  RemoveConfigurationFromProfile(service_path, "", std::move(callback),
+  RemoveConfigurationFromProfile(service_path, "", std::move(remove_confirmer),
+                                 std::move(callback),
                                  std::move(error_callback));
 }
 
@@ -400,6 +410,7 @@ void NetworkConfigurationHandler::RemoveConfigurationFromCurrentProfile(
     return;
   }
   RemoveConfigurationFromProfile(service_path, network_state->profile_path(),
+                                 /*remove_confirmer=*/base::nullopt,
                                  std::move(callback),
                                  std::move(error_callback));
 }
@@ -407,6 +418,7 @@ void NetworkConfigurationHandler::RemoveConfigurationFromCurrentProfile(
 void NetworkConfigurationHandler::RemoveConfigurationFromProfile(
     const std::string& service_path,
     const std::string& profile_path,
+    base::Optional<RemoveConfirmer> remove_confirmer,
     base::OnceClosure callback,
     network_handler::ErrorCallback error_callback) {
   // Service.Remove is not reliable. Instead, request the profile entries
@@ -428,7 +440,8 @@ void NetworkConfigurationHandler::RemoveConfigurationFromProfile(
   for (auto& observer : observers_)
     observer.OnBeforeConfigurationRemoved(service_path, guid);
   ProfileEntryDeleter* deleter = new ProfileEntryDeleter(
-      this, service_path, guid, std::move(callback), std::move(error_callback));
+      this, service_path, guid, std::move(remove_confirmer),
+      std::move(callback), std::move(error_callback));
   if (!profile_path.empty())
     deleter->RestrictToProfilePath(profile_path);
   profile_entry_deleters_[service_path] = base::WrapUnique(deleter);

chromeos/network/network_configuration_handler.h

@@ -98,8 +98,15 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkConfigurationHandler
                                 network_handler::ServiceResultCallback callback,
                                 network_handler::ErrorCallback error_callback);
 
-  // Removes the network |service_path| from any profiles that include it.
+  using RemoveConfirmer =
+      base::RepeatingCallback<bool(const std::string& guid,
+                                   const std::string& profile_path)>;
+  // Removes the network |service_path| from any profiles that include it. If
+  // |remove_confirmer| is provided, it will be used to confirm the remove
+  // operation and only entries that evaluate to true by applying the confirmer
+  // will be removed.
   void RemoveConfiguration(const std::string& service_path,
+                           base::Optional<RemoveConfirmer> remove_confirmer,
                            base::OnceClosure callback,
                            network_handler::ErrorCallback error_callback);
 
@@ -197,10 +204,13 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkConfigurationHandler
 
   // Removes network configuration for |service_path| from the profile specified
   // by |profile_path|. If |profile_path| is not set, the network is removed
-  // from all the profiles that include it.
+  // from all the profiles that include it. If |remove_confirmer| is provided,
+  // it will be used to confirm the remove operation and only entries that
+  // evaluate to true by applying the confirmer will be removed.
   void RemoveConfigurationFromProfile(
       const std::string& service_path,
       const std::string& profile_path,
+      base::Optional<RemoveConfirmer> remove_confirmer,
       base::OnceClosure callback,
       network_handler::ErrorCallback error_callback);
 

chromeos/network/network_configuration_handler_unittest.cc

@@ -520,7 +520,7 @@ TEST_F(NetworkConfigurationHandlerTest, RemoveConfiguration) {
 
   TestCallback test_callback;
   network_configuration_handler_->RemoveConfiguration(
-      "/service/2",
+      "/service/2", /*remove_confirmer=*/base::nullopt,
       base::BindOnce(&TestCallback::Run, base::Unretained(&test_callback)),
       base::BindOnce(&ErrorCallback));
 
@@ -563,6 +563,47 @@ TEST_F(NetworkConfigurationHandlerTest, RemoveConfigurationFromCurrentProfile) {
   EXPECT_EQ(1u, profiles.size());
 }
 
+TEST_F(NetworkConfigurationHandlerTest, RemoveConfigurationWithPredicate) {
+  ASSERT_NO_FATAL_FAILURE(SetUpRemovableConfiguration());
+
+  // Don't remove profile 1 entries.
+  NetworkConfigurationHandler::RemoveConfirmer remove_confirmer =
+      base::BindRepeating(
+          [](const std::string& guid, const std::string& profile_path) {
+            return profile_path != "profile1";
+          });
+
+  TestCallback test_callback1;
+  network_configuration_handler_->RemoveConfiguration(
+      "/service/1", remove_confirmer,
+      base::BindOnce(&TestCallback::Run, base::Unretained(&test_callback1)),
+      base::BindOnce(&ErrorCallback));
+
+  TestCallback test_callback2;
+  network_configuration_handler_->RemoveConfiguration(
+      "/service/2", remove_confirmer,
+      base::BindOnce(&TestCallback::Run, base::Unretained(&test_callback2)),
+      base::BindOnce(&ErrorCallback));
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, test_callback1.run_count());
+  EXPECT_EQ(1, test_callback2.run_count());
+
+  std::vector<std::string> profilesForService1, profilesForService2;
+  GetShillProfileClient()->GetProfilePathsContainingService(
+      "/service/1", &profilesForService1);
+  GetShillProfileClient()->GetProfilePathsContainingService(
+      "/service/2", &profilesForService2);
+
+  EXPECT_EQ(1u, profilesForService1.size());
+  EXPECT_EQ("profile1", profilesForService1[0]);
+  profilesForService1.clear();
+
+  EXPECT_EQ(1u, profilesForService2.size());
+  EXPECT_EQ("profile1", profilesForService2[0]);
+  profilesForService2.clear();
+}
+
 TEST_F(NetworkConfigurationHandlerTest,
        RemoveNonExistentConfigurationFromCurrentProfile) {
   ASSERT_NO_FATAL_FAILURE(SetUpRemovableConfiguration());
@@ -712,7 +753,8 @@ TEST_F(NetworkConfigurationHandlerTest, NetworkConfigurationObserver_Removed) {
           service_path));
 
   network_configuration_handler_->RemoveConfiguration(
-      service_path, base::DoNothing(), base::BindOnce(&ErrorCallback));
+      service_path, /*remove_confirmer=*/base::nullopt, base::DoNothing(),
+      base::BindOnce(&ErrorCallback));
   base::RunLoop().RunUntilIdle();
 
   EXPECT_TRUE(

chromeos/network/network_metadata_store_unittest.cc

@@ -5,6 +5,7 @@
 #include <memory>
 
 #include "base/bind_helpers.h"
+#include "base/optional.h"
 #include "base/test/task_environment.h"
 #include "chromeos/constants/chromeos_pref_names.h"
 #include "chromeos/dbus/shill/shill_clients.h"
@@ -331,7 +332,8 @@ TEST_F(NetworkMetadataStoreTest, ConfigurationRemoved) {
   ASSERT_TRUE(metadata_store()->GetIsConfiguredBySync(kGuid));
 
   network_configuration_handler()->RemoveConfiguration(
-      service_path, base::DoNothing(), base::DoNothing());
+      service_path, /*remove_confirmer=*/base::nullopt, base::DoNothing(),
+      base::DoNothing());
   base::RunLoop().RunUntilIdle();
 
   ASSERT_TRUE(metadata_store()->GetLastConnectedTimestamp(kGuid).is_zero());

extensions/browser/api/vpn_provider/vpn_service.cc

@@ -15,6 +15,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/optional.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ -100,11 +101,9 @@ VpnService::VpnConfiguration::VpnConfiguration(
       configuration_name_(configuration_name),
       key_(key),
       object_path_(shill::kObjectPathBase + key_),
-      vpn_service_(vpn_service) {
-}
+      vpn_service_(vpn_service) {}
 
-VpnService::VpnConfiguration::~VpnConfiguration() {
-}
+VpnService::VpnConfiguration::~VpnConfiguration() {}
 
 void VpnService::VpnConfiguration::OnPacketReceived(
     const std::vector<char>& data) {
@@ -427,6 +426,7 @@ void VpnService::DestroyConfiguration(const std::string& extension_id,
 
   network_configuration_handler_->RemoveConfiguration(
       service_path,
+      /*remove_confirmer=*/base::nullopt,
       base::Bind(&VpnService::OnRemoveConfigurationSuccess,
                  weak_factory_.GetWeakPtr(), success),
       base::Bind(&VpnService::OnRemoveConfigurationFailure,