shill: Manager: Merge stored configuration in ConfigureService
In ConfigureService, if there is already stored information available
for a service, merge this stored information into newly created services
instead of overriding them with the defaults. This situation can arise
when configuration is loaded via policy for a service that is not
currently visible. User-specified parameters such as AutoConnect can be
lost in this process otherwise.
BUG=chromium-os:37042
TEST=Modified unit tests + manual: login to ONC-managed account
with WiFi disabled, and ensure configuration is merged (credentials are
available in the service although ONC did not provide them).
Change-Id: I92cbb7d14dc21c4173794ef1eb19b792d97c44ac
Reviewed-on: https://gerrit.chromium.org/gerrit/39398
Commit-Ready: Paul Stewart <pstew@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/manager.cc b/manager.cc
index ca184d8..bacafcd 100644
--- a/manager.cc
+++ b/manager.cc
@@ -1289,7 +1289,14 @@
// instead, when we no longer need to support flimflam. crosbug.com/31523
return ConfigureService(args, error);
}
- return GetServiceInner(args, error);
+
+ ServiceRefPtr service = GetServiceInner(args, error);
+ if (service) {
+ // Configures the service using the rest of the passed-in arguments.
+ service->Configure(args, error);
+ }
+
+ return service;
}
ServiceRefPtr Manager::GetServiceInner(const KeyValueStore &args,
@@ -1299,7 +1306,6 @@
ServiceRefPtr service =
GetServiceWithGUID(args.GetString(flimflam::kGuidProperty), NULL);
if (service) {
- service->Configure(args, error);
return service;
}
}
@@ -1362,6 +1368,27 @@
return NULL;
}
+ // First pull in any stored configuration associated with the service.
+ if (service->profile() == profile) {
+ SLOG(Manager, 2) << __func__ << ": service " << service->friendly_name()
+ << " is already a member of profile "
+ << profile->GetFriendlyName()
+ << " so a load is not necessary.";
+ } else if (profile->LoadService(service)) {
+ SLOG(Manager, 2) << __func__ << ": applied stored information from profile "
+ << profile->GetFriendlyName()
+ << " into service "
+ << service->friendly_name();
+ } else {
+ SLOG(Manager, 2) << __func__ << ": no previous information in profile "
+ << profile->GetFriendlyName()
+ << " exists for service "
+ << service->friendly_name();
+ }
+
+ // Overlay this with the passed-in configuration parameters.
+ service->Configure(args, error);
+
// Overwrite the profile data with the resulting configured service.
if (!profile->UpdateService(service)) {
Error::PopulateAndLog(error, Error::kInternalError,
diff --git a/manager_unittest.cc b/manager_unittest.cc
index a524850..0a6a9c3 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -1419,6 +1419,8 @@
EXPECT_CALL(*profile, UpdateService(_))
.WillOnce(DoAll(SaveArg<0>(&updated_service), Return(true)));
ServiceRefPtr configured_service;
+ EXPECT_CALL(*profile, LoadService(_))
+ .WillOnce(Return(false));
EXPECT_CALL(*profile, ConfigureService(_))
.WillOnce(DoAll(SaveArg<0>(&configured_service), Return(true)));
ServiceRefPtr service = manager()->GetService(args, &e);
@@ -1524,7 +1526,7 @@
EXPECT_TRUE(error.IsSuccess());
}
-// If were configure a service that was already registered and explicitly
+// If we configure a service that was already registered and explicitly
// specify a profile, it should be moved from the profile it was previously
// in to the specified profile if one was requested.
TEST_F(ManagerTest, ConfigureRegisteredServiceWithProfile) {
@@ -1571,6 +1573,8 @@
manager()->RegisterDevice(wifi);
EXPECT_CALL(*wifi, GetService(_, _))
.WillOnce(Return(service));
+ EXPECT_CALL(*profile0, LoadService(ServiceRefPtr(service.get())))
+ .WillOnce(Return(true));
EXPECT_CALL(*profile0, UpdateService(ServiceRefPtr(service.get())))
.WillOnce(Return(true));
EXPECT_CALL(*profile0, AdoptService(ServiceRefPtr(service.get())))
@@ -1587,6 +1591,63 @@
service->set_profile(NULL); // Breaks refcounting loop.
}
+// If we configure a service that is already a member of the specified
+// profile, the Manager should not call LoadService or AdoptService again
+// on this service.
+TEST_F(ManagerTest, ConfigureRegisteredServiceWithSameProfile) {
+ scoped_refptr<MockProfile> profile0(
+ new NiceMock<MockProfile>(control_interface(), manager(), ""));
+
+ const string kProfileName0 = "profile0";
+
+ EXPECT_CALL(*profile0, GetRpcIdentifier())
+ .WillRepeatedly(Return(kProfileName0));
+
+ AdoptProfile(manager(), profile0); // profile0 is now the ActiveProfile.
+
+ const std::vector<uint8_t> ssid;
+ scoped_refptr<MockWiFiService> service(
+ new NiceMock<MockWiFiService>(control_interface(),
+ dispatcher(),
+ metrics(),
+ manager(),
+ mock_wifi_,
+ ssid,
+ "",
+ "",
+ false));
+
+ manager()->RegisterService(service);
+ service->set_profile(profile0);
+
+ // A separate MockWiFi from mock_wifi_ is used in the Manager since using
+ // the same device as that used above causes a refcounting loop.
+ scoped_refptr<MockWiFi> wifi(new NiceMock<MockWiFi>(control_interface(),
+ dispatcher(),
+ metrics(),
+ manager(),
+ "wifi1",
+ "addr5",
+ 5));
+ manager()->RegisterDevice(wifi);
+ EXPECT_CALL(*wifi, GetService(_, _))
+ .WillOnce(Return(service));
+ EXPECT_CALL(*profile0, LoadService(ServiceRefPtr(service.get())))
+ .Times(0);
+ EXPECT_CALL(*profile0, UpdateService(ServiceRefPtr(service.get())))
+ .WillOnce(Return(true));
+ EXPECT_CALL(*profile0, AdoptService(ServiceRefPtr(service.get())))
+ .Times(0);
+
+ KeyValueStore args;
+ args.SetString(flimflam::kTypeProperty, flimflam::kTypeWifi);
+ args.SetString(flimflam::kProfileProperty, kProfileName0);
+ Error error;
+ manager()->ConfigureService(args, &error);
+ EXPECT_TRUE(error.IsSuccess());
+ service->set_profile(NULL); // Breaks refcounting loop.
+}
+
// An unregistered service should remain unregistered, but its contents should
// be saved to the specified profile nonetheless.
TEST_F(ManagerTest, ConfigureUnregisteredServiceWithProfile) {
diff --git a/mock_profile.h b/mock_profile.h
index bbeb2b1..5922dc2 100644
--- a/mock_profile.h
+++ b/mock_profile.h
@@ -23,6 +23,7 @@
MOCK_METHOD1(AdoptService, bool(const ServiceRefPtr &service));
MOCK_METHOD1(AbandonService, bool(const ServiceRefPtr &service));
+ MOCK_METHOD1(LoadService, bool(const ServiceRefPtr &service));
MOCK_METHOD1(ConfigureService, bool(const ServiceRefPtr &service));
MOCK_METHOD1(ConfigureDevice, bool(const DeviceRefPtr &device));
MOCK_METHOD0(GetRpcIdentifier, std::string());
diff --git a/profile.cc b/profile.cc
index f9c568b..e73f784 100644
--- a/profile.cc
+++ b/profile.cc
@@ -165,13 +165,19 @@
return service->Save(storage_.get()) && storage_->Flush();
}
-bool Profile::ConfigureService(const ServiceRefPtr &service) {
+bool Profile::LoadService(const ServiceRefPtr &service) {
if (!ContainsService(service))
return false;
- service->SetProfile(this);
return service->Load(storage_.get());
}
+bool Profile::ConfigureService(const ServiceRefPtr &service) {
+ if (!LoadService(service))
+ return false;
+ service->SetProfile(this);
+ return true;
+}
+
bool Profile::ConfigureDevice(const DeviceRefPtr &device) {
return device->Load(storage_.get());
}
diff --git a/profile.h b/profile.h
index 8096aba..9739489 100644
--- a/profile.h
+++ b/profile.h
@@ -89,8 +89,13 @@
virtual bool UpdateService(const ServiceRefPtr &service);
// Ask |service| if it can configure itself from the profile. If it can,
- // change the service to point at this profile, ask |service| to perform
- // the configuration and return true. If not, return false.
+ // ask |service| to perform the configuration and return true. If not,
+ // return false.
+ virtual bool LoadService(const ServiceRefPtr &service);
+
+ // Perform LoadService() on |service|. If this succeeds, change
+ // the service to point at this profile and return true. If not, return
+ // false.
virtual bool ConfigureService(const ServiceRefPtr &service);
// Allow the device to configure itself from this profile. Returns
diff --git a/vpn_provider.cc b/vpn_provider.cc
index 0bd403f..0fce3df 100644
--- a/vpn_provider.cc
+++ b/vpn_provider.cc
@@ -66,11 +66,6 @@
service = CreateService(type, name, storage_id, error);
}
- if (service != NULL) {
- // Configure the service using the the rest of the passed-in arguments.
- service->Configure(args, error);
- }
-
return service;
}
diff --git a/wifi.cc b/wifi.cc
index a5f522d..92e6ce5 100644
--- a/wifi.cc
+++ b/wifi.cc
@@ -1353,8 +1353,6 @@
// The Service will be registered if/when we find Endpoints for it.
}
- service->Configure(args, error);
-
// TODO(pstew): Schedule a task to forget up all non-hidden services that
// have no endpoints like the one we may have just created. crosbug.com/28224
diff --git a/wifi_unittest.cc b/wifi_unittest.cc
index 676f784..4d8ab0f 100644
--- a/wifi_unittest.cc
+++ b/wifi_unittest.cc
@@ -482,7 +482,11 @@
Error e;
KeyValueStore args_kv;
DBusAdaptor::ArgsToKeyValueStore(args, &args_kv, &e);
- return wifi_->GetService(args_kv, result);
+ WiFiServiceRefPtr service = wifi_->GetService(args_kv, result);
+ if (service) {
+ service->Configure(args_kv, result);
+ }
+ return service;
}
WiFiServiceRefPtr GetServiceWithKeyValues(const KeyValueStore &args,
@@ -1627,7 +1631,7 @@
}
TEST_F(WiFiMainTest, GetServiceWithGUID) {
- // Perform a GetService that also configures properties in the base Service
+ // Perform a GetService and also configure properties in the base Service
// class using Service::Configure().
KeyValueStore args;
args.SetString(flimflam::kTypeProperty, flimflam::kTypeWifi);
@@ -1635,9 +1639,16 @@
args.SetString(flimflam::kSecurityProperty, flimflam::kSecurityNone);
const string kGUID = "aguid"; // Stored as a registered Service property.
args.SetString(flimflam::kGuidProperty, kGUID);
+
Error e;
WiFiServiceRefPtr service = GetServiceWithKeyValues(args, &e);
EXPECT_TRUE(e.IsSuccess());
+ // Assert that before Configure is called on the service, the GUID property
+ // is not set.
+ EXPECT_EQ("", service->guid());
+
+ service->Configure(args, &e);
+ EXPECT_TRUE(e.IsSuccess());
EXPECT_EQ(kGUID, service->guid());
}
diff --git a/wimax_provider.cc b/wimax_provider.cc
index 8fd0b3b..a6f45bf 100644
--- a/wimax_provider.cc
+++ b/wimax_provider.cc
@@ -165,8 +165,6 @@
}
WiMaxServiceRefPtr service = GetUniqueService(id, name);
CHECK(service);
- // Configures the service using the rest of the passed-in arguments.
- service->Configure(args, error);
// Starts the service if there's a matching live network.
StartLiveServices();
return service;
diff --git a/wimax_provider_unittest.cc b/wimax_provider_unittest.cc
index 1029efc..152f238 100644
--- a/wimax_provider_unittest.cc
+++ b/wimax_provider_unittest.cc
@@ -505,6 +505,14 @@
service = provider_.GetService(args, &e);
EXPECT_TRUE(e.IsSuccess());
ASSERT_TRUE(service);
+
+ // GetService should create a service with only identifying parameters set.
+ EXPECT_EQ(kName, service->friendly_name());
+ EXPECT_EQ("", service->eap().identity);
+
+ // After configuring the service, other parameters should be set.
+ service->Configure(args, &e);
+ EXPECT_TRUE(e.IsSuccess());
EXPECT_EQ(kIdentity, service->eap().identity);
}