[CrOS Cellular] Add ModifyCustomApn in CrosNetworkConfig API
This new API replaces custom APNs entries by ID in the new APN UI revamp.
If an entry is found, it replaces the APN and updates the ONC value
"Cellular.UserAPNList", and the NetworkMetadataStore.
Bug: b:162365553
Test: chromeos_unittests --gtest_filter=CrosNetworkConfigTest.*
Change-Id: Iffd176b6ba7ed0965cdb02436e8142c042c91581
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4020387
Commit-Queue: Emmanuel Arias Soto <eariassoto@google.com>
Reviewed-by: Gordon Seto <gordonseto@google.com>
Reviewed-by: Chad Duffin <chadduffin@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1073273}
diff --git a/chromeos/services/network_config/cros_network_config.cc b/chromeos/services/network_config/cros_network_config.cc
index bc9e5c2..bbb53b8 100644
--- a/chromeos/services/network_config/cros_network_config.cc
+++ b/chromeos/services/network_config/cros_network_config.cc
@@ -3672,7 +3672,7 @@
const base::Value::List* current_apns =
network_metadata_store->GetCustomApnList(network_guid);
- if (!current_apns) {
+ if (!current_apns || current_apns->empty()) {
NET_LOG(ERROR) << "RemoveCustomApn: Called for network: " << network_guid
<< " that does not have any user APNs.";
return;
@@ -3708,6 +3708,83 @@
network_guid));
}
+void CrosNetworkConfig::ModifyCustomApn(const std::string& network_guid,
+ mojom::ApnPropertiesPtr apn) {
+ if (!ash::features::IsApnRevampEnabled()) {
+ receivers_.ReportBadMessage(
+ "ModifyCustomApn: Cannot be called if the APN Revamp feature flag is "
+ "disabled.");
+ return;
+ }
+
+ const NetworkState* network =
+ network_state_handler_->GetNetworkStateFromGuid(network_guid);
+ if (!network || network->profile_path().empty()) {
+ NET_LOG(ERROR) << "ModifyCustomApn: Called with unconfigured network: "
+ << network_guid << ".";
+ return;
+ }
+
+ if (!apn->id.has_value()) {
+ NET_LOG(ERROR)
+ << "ModifyCustomApn: Called with an APN without ID for network: "
+ << network_guid << '.';
+ return;
+ }
+
+ NetworkMetadataStore* network_metadata_store =
+ NetworkHandler::Get()->network_metadata_store();
+ DCHECK(network_metadata_store);
+
+ const base::Value::List* old_custom_apns =
+ network_metadata_store->GetCustomApnList(network_guid);
+ if (!old_custom_apns || old_custom_apns->empty()) {
+ NET_LOG(ERROR) << "ModifyCustomApn: Called for network: " << network_guid
+ << " that does not have any user APNs.";
+ return;
+ }
+
+ base::Value::List new_custom_apns;
+ bool was_value_replaced = false;
+ for (const base::Value& old_apn : *old_custom_apns) {
+ const std::string* old_apn_id =
+ old_apn.GetDict().FindString(::onc::cellular_apn::kId);
+ DCHECK(old_apn_id);
+ if (*apn->id == *old_apn_id) {
+ new_custom_apns.Append(MojoApnToOnc(*apn));
+ was_value_replaced = true;
+ } else {
+ new_custom_apns.Append(old_apn.Clone());
+ }
+ }
+
+ if (!was_value_replaced) {
+ NET_LOG(ERROR) << "ModifyCustomApn: Called for network: " << network_guid
+ << " that does have an user APNs with id: " << *apn->id
+ << '.';
+ return;
+ }
+ NET_LOG(USER) << "ModifyCustomApn: Setting user APNs for: " << network_guid
+ << ": " << new_custom_apns.size();
+
+ network_metadata_store->SetCustomApnList(network_guid,
+ new_custom_apns.Clone());
+ SetPropertiesInternal(
+ network_guid, *network,
+ UserApnListToOnc(network_guid, std::move(new_custom_apns)),
+ base::BindOnce(
+ [](const std::string& guid, bool success,
+ const std::string& message) {
+ if (!success) {
+ NET_LOG(ERROR)
+ << "ModifyCustomApn: Failed to update the user APN "
+ "list in Shill for network: "
+ << guid << ": [" << message << ']';
+ }
+ },
+ network_guid));
+}
+
// static
mojom::TrafficCounterSource CrosNetworkConfig::GetTrafficCounterEnumForTesting(
const std::string& source) {
diff --git a/chromeos/services/network_config/cros_network_config.h b/chromeos/services/network_config/cros_network_config.h
index 5ac222d..bf9d00ce 100644
--- a/chromeos/services/network_config/cros_network_config.h
+++ b/chromeos/services/network_config/cros_network_config.h
@@ -112,6 +112,8 @@
mojom::ApnPropertiesPtr apn) override;
void RemoveCustomApn(const std::string& network_guid,
const std::string& apn_id) override;
+ void ModifyCustomApn(const std::string& network_guid,
+ mojom::ApnPropertiesPtr apn) override;
// static
static mojom::TrafficCounterSource GetTrafficCounterEnumForTesting(
diff --git a/chromeos/services/network_config/cros_network_config_unittest.cc b/chromeos/services/network_config/cros_network_config_unittest.cc
index 49c48b5a..cebbc41 100644
--- a/chromeos/services/network_config/cros_network_config_unittest.cc
+++ b/chromeos/services/network_config/cros_network_config_unittest.cc
@@ -828,6 +828,11 @@
base::RunLoop().RunUntilIdle();
}
+ void ModifyCustomApn(const std::string& guid, mojom::ApnPropertiesPtr apn) {
+ cros_network_config()->ModifyCustomApn(guid, std::move(apn));
+ base::RunLoop().RunUntilIdle();
+ }
+
bool UserApnsInNetworkMetadataStoreMatch(
const std::string& guid,
const std::vector<TestApnData*>& expected_apns) {
@@ -1941,6 +1946,121 @@
}
}
+TEST_F(CrosNetworkConfigTest, ModifyCustomApn) {
+ base::test::ScopedFeatureList scoped_feature_list;
+ scoped_feature_list.InitAndEnableFeature(ash::features::kApnRevamp);
+
+ // Register an observer to capture values sent to Shill
+ TestNetworkConfigurationObserver network_config_observer(
+ network_configuration_handler());
+
+ ASSERT_FALSE(network_metadata_store()->GetCustomApnList(kCellularGuid));
+ // Verify ModifyCustomApn reports an error and return when the
+ // network_metadata_store has a nullptr custom APN list
+ size_t expected_network_config_calls = 0u;
+ TestApnData test_apn1;
+ test_apn1.access_point_name = kCellularTestApn1;
+ test_apn1.name = kCellularTestApnName1;
+ test_apn1.username = kCellularTestApnUsername1;
+ test_apn1.password = kCellularTestApnPassword1;
+ test_apn1.attach = kCellularTestApnAttach1;
+ test_apn1.mojo_apn_types = {mojom::ApnType::kDefault,
+ mojom::ApnType::kAttach};
+ test_apn1.onc_apn_types = {::onc::cellular_apn::kApnTypeDefault,
+ ::onc::cellular_apn::kApnTypeAttach};
+ test_apn1.id = "apn_id_1";
+ ModifyCustomApn(kCellularGuid, test_apn1.AsMojoApn());
+ EXPECT_EQ(expected_network_config_calls,
+ network_config_observer.GetOnConfigurationModifiedCallCount());
+ ASSERT_FALSE(network_metadata_store()->GetCustomApnList(kCellularGuid));
+
+ // Try to replace an APN when the custom APN list is empty, it should do
+ // nothing
+ network_metadata_store()->SetCustomApnList(kCellularGuid,
+ base::Value::List());
+ ModifyCustomApn(kCellularGuid, test_apn1.AsMojoApn());
+ EXPECT_EQ(expected_network_config_calls,
+ network_config_observer.GetOnConfigurationModifiedCallCount());
+ EXPECT_TRUE(UserApnsInNetworkMetadataStoreMatch({}));
+
+ TestApnData test_apn2;
+ test_apn2.access_point_name = kCellularTestApn2;
+ test_apn2.name = kCellularTestApnName2;
+ test_apn2.username = kCellularTestApnUsername2;
+ test_apn2.password = kCellularTestApnPassword2;
+ test_apn2.attach = kCellularTestApnAttach2;
+ test_apn2.mojo_apn_types = {mojom::ApnType::kDefault,
+ mojom::ApnType::kAttach};
+ test_apn2.onc_apn_types = {::onc::cellular_apn::kApnTypeDefault,
+ ::onc::cellular_apn::kApnTypeAttach};
+
+ // Add two custom APNs using the official API
+ {
+ CreateCustomApn(kCellularGuid, test_apn1.AsMojoApn());
+ EXPECT_EQ(++expected_network_config_calls,
+ network_config_observer.GetOnConfigurationModifiedCallCount());
+ CreateCustomApn(kCellularGuid, test_apn2.AsMojoApn());
+ EXPECT_EQ(++expected_network_config_calls,
+ network_config_observer.GetOnConfigurationModifiedCallCount());
+ }
+
+ // Verify that ModifyCustomApn replaces the first custom APN
+ const base::Value::List* custom_apns =
+ network_metadata_store()->GetCustomApnList(kCellularGuid);
+ ASSERT_TRUE(custom_apns);
+ ASSERT_EQ(2u, custom_apns->size());
+ const std::string* first_apn_id =
+ custom_apns->front().GetDict().FindString(::onc::cellular_apn::kId);
+ ASSERT_TRUE(first_apn_id);
+
+ TestApnData test_apn3;
+ test_apn3.access_point_name = kCellularTestApn3;
+ test_apn3.name = kCellularTestApnName3;
+ test_apn3.username = kCellularTestApnUsername3;
+ test_apn3.password = kCellularTestApnPassword3;
+ test_apn3.attach = kCellularTestApnAttach3;
+ test_apn3.mojo_apn_types = {mojom::ApnType::kAttach};
+ test_apn3.onc_apn_types = {::onc::cellular_apn::kApnTypeAttach};
+
+ // Verify that ModifyCustomApn does nothing if the input APN does not have an
+ // ID.
+ ModifyCustomApn(kCellularGuid, test_apn3.AsMojoApn());
+ EXPECT_EQ(expected_network_config_calls,
+ network_config_observer.GetOnConfigurationModifiedCallCount());
+ {
+ std::vector<TestApnData*> expected_apns({&test_apn2, &test_apn1});
+ EXPECT_TRUE(UserApnsInNetworkMetadataStoreMatch(expected_apns));
+ EXPECT_TRUE(
+ UserApnsInCellularConfigMatch(expected_apns, network_config_observer));
+ EXPECT_TRUE(UserApnsInManagedPropertiesMatch(expected_apns));
+ }
+
+ test_apn3.id = *first_apn_id;
+ ModifyCustomApn(kCellularGuid, test_apn3.AsMojoApn());
+ EXPECT_EQ(++expected_network_config_calls,
+ network_config_observer.GetOnConfigurationModifiedCallCount());
+ {
+ std::vector<TestApnData*> expected_apns({&test_apn3, &test_apn1});
+ EXPECT_TRUE(UserApnsInNetworkMetadataStoreMatch(expected_apns));
+ EXPECT_TRUE(
+ UserApnsInCellularConfigMatch(expected_apns, network_config_observer));
+ EXPECT_TRUE(UserApnsInManagedPropertiesMatch(expected_apns));
+ }
+
+ // Try to update an ID not found in the list, API should do nothing
+ test_apn3.id = "invalid_apn_id";
+ ModifyCustomApn(kCellularGuid, test_apn3.AsMojoApn());
+ EXPECT_EQ(expected_network_config_calls,
+ network_config_observer.GetOnConfigurationModifiedCallCount());
+ {
+ std::vector<TestApnData*> expected_apns({&test_apn3, &test_apn1});
+ EXPECT_TRUE(UserApnsInNetworkMetadataStoreMatch(expected_apns));
+ EXPECT_TRUE(
+ UserApnsInCellularConfigMatch(expected_apns, network_config_observer));
+ EXPECT_TRUE(UserApnsInManagedPropertiesMatch(expected_apns));
+ }
+}
+
TEST_F(CrosNetworkConfigTest, ConnectedAPN_ApnRevampEnabled) {
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeature(ash::features::kApnRevamp);
diff --git a/chromeos/services/network_config/public/mojom/cros_network_config.mojom b/chromeos/services/network_config/public/mojom/cros_network_config.mojom
index 8b3acbf0..8cda349 100644
--- a/chromeos/services/network_config/public/mojom/cros_network_config.mojom
+++ b/chromeos/services/network_config/public/mojom/cros_network_config.mojom
@@ -1252,6 +1252,11 @@
// network with ID |network_guid|. Should only be called when the APN
// Revamp feature flag is enabled.
RemoveCustomApn(string network_guid, string apn_id);
+
+ // Finds the APN that matches |apn.id| in |custom_apn_list| for the network
+ // with ID |network_guid|. If found, replaces the content with |apn|.
+ // Should only be called when the APN Revamp feature flag is enabled.
+ ModifyCustomApn(string network_guid, ApnProperties apn);
};
interface CrosNetworkConfigObserver {
