shill: vpn: Support Service.Name property change.
BUG=chromium:221577
TEST=Unit tests; Tested on device through configure-vpn; Also, tested
on device through corp policy and checked that the service got
renamed.
Change-Id: I936f85573e8d0820e2baa2361f94619e0e655f8f
Reviewed-on: https://gerrit.chromium.org/gerrit/46514
Reviewed-by: Darin Petkov <petkov@chromium.org>
Tested-by: Darin Petkov <petkov@chromium.org>
Commit-Queue: Darin Petkov <petkov@chromium.org>
diff --git a/mock_profile.h b/mock_profile.h
index aeb3277..2ab3087 100644
--- a/mock_profile.h
+++ b/mock_profile.h
@@ -27,6 +27,7 @@
MOCK_METHOD1(LoadService, bool(const ServiceRefPtr &service));
MOCK_METHOD1(ConfigureService, bool(const ServiceRefPtr &service));
MOCK_METHOD1(ConfigureDevice, bool(const DeviceRefPtr &device));
+ MOCK_METHOD2(DeleteEntry, void(const std::string &entry_name, Error *error));
MOCK_METHOD0(GetRpcIdentifier, std::string());
MOCK_METHOD1(GetStoragePath, bool(base::FilePath *filepath));
MOCK_METHOD1(UpdateService, bool(const ServiceRefPtr &service));
diff --git a/service.cc b/service.cc
index cccd70d..7a54bd9 100644
--- a/service.cc
+++ b/service.cc
@@ -220,11 +220,9 @@
NULL);
// flimflam::kModeProperty: Registered in WiFiService
- // Although this is a read-only property, some callers want to blindly
- // set this value to its current value.
HelpRegisterDerivedString(flimflam::kNameProperty,
&Service::GetNameProperty,
- &Service::AssertTrivialSetNameProperty);
+ &Service::SetNameProperty);
// flimflam::kPassphraseProperty: Registered in WiFiService
// flimflam::kPassphraseRequiredProperty: Registered in WiFiService
store_.RegisterInt32(flimflam::kPriorityProperty, &priority_);
@@ -1348,7 +1346,7 @@
return friendly_name_;
}
-void Service::AssertTrivialSetNameProperty(const string &name, Error *error) {
+void Service::SetNameProperty(const string &name, Error *error) {
if (name != friendly_name_) {
Error::PopulateAndLog(error, Error::kInvalidArguments,
base::StringPrintf(
diff --git a/service.h b/service.h
index 3569a2c..beb61f6 100644
--- a/service.h
+++ b/service.h
@@ -328,6 +328,7 @@
// Sets the flimflam::kNameProperty
void SetFriendlyName(const std::string &friendly_name);
void set_friendly_name(const std::string &n) { friendly_name_ = n; }
+ const std::string &friendly_name() const { return friendly_name_; }
const std::string &guid() const { return guid_; }
void set_guid(const std::string &guid) { guid_ = guid; }
@@ -422,8 +423,6 @@
virtual ~Service();
- const std::string &friendly_name() const { return friendly_name_; }
-
// Returns true if a character is allowed to be in a service storage id.
static bool LegalChar(char a) { return isalnum(a) || a == '_'; }
@@ -611,8 +610,10 @@
std::string GetIPConfigRpcIdentifier(Error *error);
+ // The base implementation asserts that |name| matches the current Name
+ // property value.
+ virtual void SetNameProperty(const std::string &name, Error *error);
std::string GetNameProperty(Error *error);
- void AssertTrivialSetNameProperty(const std::string &name, Error *error);
std::string GetProfileRpcId(Error *error);
void SetProfileRpcId(const std::string &profile, Error *error);
diff --git a/vpn_driver.cc b/vpn_driver.cc
index 4705c8a..6143980 100644
--- a/vpn_driver.cc
+++ b/vpn_driver.cc
@@ -188,4 +188,8 @@
StopConnectTimeout();
}
+string VPNDriver::GetHost() const {
+ return args_.LookupString(flimflam::kProviderHostProperty, "");
+}
+
} // namespace shill
diff --git a/vpn_driver.h b/vpn_driver.h
index ea6bb82..f63dc07 100644
--- a/vpn_driver.h
+++ b/vpn_driver.h
@@ -45,6 +45,8 @@
bool save_credentials);
virtual void UnloadCredentials();
+ std::string GetHost() const;
+
KeyValueStore *args() { return &args_; }
protected:
diff --git a/vpn_provider.cc b/vpn_provider.cc
index dbadcbc..be8530e 100644
--- a/vpn_provider.cc
+++ b/vpn_provider.cc
@@ -49,23 +49,28 @@
return NULL;
}
+ string host = args.LookupString(flimflam::kProviderHostProperty, "");
+ if (host.empty()) {
+ Error::PopulateAndLog(
+ error, Error::kNotSupported, "Missing VPN host property.");
+ return NULL;
+ }
+
string storage_id = VPNService::CreateStorageIdentifier(args, error);
if (storage_id.empty()) {
return NULL;
}
- // Find a service in the provider list which matches these parameters.
- VPNServiceRefPtr service = FindService(type, storage_id);
-
- if (service == NULL) {
- // Create a service, using the name and type arguments passed in.
- string name = args.LookupString(flimflam::kProviderNameProperty, "");
- if (name.empty()) {
- name = args.LookupString(flimflam::kNameProperty, "");
- }
- service = CreateService(type, name, storage_id, error);
+ string name = args.LookupString(flimflam::kProviderNameProperty, "");
+ if (name.empty()) {
+ name = args.LookupString(flimflam::kNameProperty, "");
}
+ // Find a service in the provider list which matches these parameters.
+ VPNServiceRefPtr service = FindService(type, name, host);
+ if (service == NULL) {
+ service = CreateService(type, name, storage_id, error);
+ }
return service;
}
@@ -115,7 +120,14 @@
continue;
}
- VPNServiceRefPtr service = FindService(type, *it);
+ string host;
+ if (!storage->GetString(*it, flimflam::kProviderHostProperty, &host)) {
+ LOG(ERROR) << "Group " << *it << " is missing the "
+ << flimflam::kProviderHostProperty << " property.";
+ continue;
+ }
+
+ VPNServiceRefPtr service = FindService(type, name, host);
if (service != NULL) {
// If the service already exists, it does not need to be configured,
// since PushProfile would have already called ConfigureService on it.
@@ -181,17 +193,18 @@
#endif // DISABLE_VPN
}
-VPNServiceRefPtr VPNProvider::FindService(const std::string &type,
- const std::string &storage_id) {
+VPNServiceRefPtr VPNProvider::FindService(const string &type,
+ const string &name,
+ const string &host) {
for (vector<VPNServiceRefPtr>::const_iterator it = services_.begin();
it != services_.end();
++it) {
if (type == (*it)->driver()->GetProviderType() &&
- storage_id == (*it)->GetStorageIdentifier()) {
+ name == (*it)->friendly_name() &&
+ host == (*it)->driver()->GetHost()) {
return *it;
}
}
-
return NULL;
}
diff --git a/vpn_provider.h b/vpn_provider.h
index cadb17a..cf9d95f 100644
--- a/vpn_provider.h
+++ b/vpn_provider.h
@@ -66,9 +66,11 @@
const std::string &storage_id,
Error *error);
- // Find a service of type |type| whose storage identifier is |storage_id|.
+ // Finds a service of type |type| with its Name property set to |name| and its
+ // Provider.Host property set to |host|.
VPNServiceRefPtr FindService(const std::string &type,
- const std::string &storage_id);
+ const std::string &name,
+ const std::string &host);
ControlInterface *control_interface_;
EventDispatcher *dispatcher_;
diff --git a/vpn_provider_unittest.cc b/vpn_provider_unittest.cc
index dd3af22..6775b3b 100644
--- a/vpn_provider_unittest.cc
+++ b/vpn_provider_unittest.cc
@@ -54,6 +54,10 @@
provider_.services_.push_back(service);
}
+ VPNServiceRefPtr GetServiceAt(int idx) {
+ return provider_.services_[idx];
+ }
+
NiceMockControl control_;
MockMetrics metrics_;
MockManager manager_;
@@ -100,6 +104,10 @@
EXPECT_EQ("vpn_10_8_0_1_vpn_name", service0->GetStorageIdentifier());
EXPECT_EQ(kName, GetServiceFriendlyName(service0));
+ // Configure the service to set its properties (including Provider.Host).
+ service0->Configure(args, &e);
+ EXPECT_TRUE(e.IsSuccess());
+
// A second call should return the same service.
VPNServiceRefPtr service1 = provider_.GetService(args, &e);
EXPECT_TRUE(e.IsSuccess());
@@ -173,50 +181,67 @@
scoped_refptr<MockProfile> profile(
new NiceMock<MockProfile>(&control_, &metrics_, &manager_, ""));
NiceMock<MockStore> storage;
- EXPECT_CALL(*profile, GetConstStorage())
- .WillRepeatedly(Return(&storage));
- EXPECT_CALL(storage, GetString(_, _, _))
- .WillRepeatedly(Return(false));
+ EXPECT_CALL(*profile, GetConstStorage()).WillRepeatedly(Return(&storage));
+ EXPECT_CALL(storage, GetString(_, _, _)).WillRepeatedly(Return(false));
std::set<string> groups;
- const string kNonVPNIdentifier("foo_123_456");
+
+ const string kNonVPNIdentifier("foo_1");
groups.insert(kNonVPNIdentifier);
- const string kVPNIdentifier0("vpn_123_456");
- const string kVPNIdentifier1("vpn_789_012");
- groups.insert(kVPNIdentifier0);
- groups.insert(kVPNIdentifier1);
+
+ const string kVPNIdentifierNoProvider("vpn_no_provider");
+ groups.insert(kVPNIdentifierNoProvider);
+
+ const string kVPNIdentifierNoName("vpn_no_name");
+ groups.insert(kVPNIdentifierNoName);
+ const string kOpenVPNProvider(flimflam::kProviderOpenVpn);
+ EXPECT_CALL(storage, GetString(kVPNIdentifierNoName,
+ flimflam::kProviderTypeProperty,
+ _))
+ .WillRepeatedly(DoAll(SetArgumentPointee<2>(kOpenVPNProvider),
+ Return(true)));
+
+ const string kVPNIdentifierNoHost("vpn_no_host");
+ groups.insert(kVPNIdentifierNoHost);
+ EXPECT_CALL(storage, GetString(kVPNIdentifierNoHost,
+ flimflam::kProviderTypeProperty,
+ _))
+ .WillRepeatedly(DoAll(SetArgumentPointee<2>(kOpenVPNProvider),
+ Return(true)));
+ const string kName("name");
+ EXPECT_CALL(storage, GetString(kVPNIdentifierNoHost,
+ flimflam::kNameProperty, _))
+ .WillRepeatedly(DoAll(SetArgumentPointee<2>(kName), Return(true)));
+
+ const string kVPNIdentifierValid("vpn_valid");
+ groups.insert(kVPNIdentifierValid);
+ EXPECT_CALL(storage, GetString(kVPNIdentifierValid,
+ flimflam::kProviderTypeProperty,
+ _))
+ .WillRepeatedly(DoAll(SetArgumentPointee<2>(kOpenVPNProvider),
+ Return(true)));
+ EXPECT_CALL(storage, GetString(kVPNIdentifierValid,
+ flimflam::kNameProperty, _))
+ .WillRepeatedly(DoAll(SetArgumentPointee<2>(kName), Return(true)));
+ const string kHost("1.2.3.4");
+ EXPECT_CALL(storage, GetString(kVPNIdentifierValid,
+ flimflam::kProviderHostProperty, _))
+ .WillRepeatedly(DoAll(SetArgumentPointee<2>(kHost), Return(true)));
+
EXPECT_CALL(storage, GetGroupsWithKey(flimflam::kProviderTypeProperty))
.WillRepeatedly(Return(groups));
- EXPECT_CALL(*profile, ConfigureService(ServiceWithStorageId(kVPNIdentifier1)))
- .WillOnce(Return(true));
-
- const string kName("name");
- EXPECT_CALL(storage, GetString(_, flimflam::kNameProperty, _))
- .WillRepeatedly(DoAll(SetArgumentPointee<2>(kName), Return(true)));
-
- const string kProviderTypeUnknown("unknown");
- EXPECT_CALL(storage, GetString(kVPNIdentifier0,
- flimflam::kProviderTypeProperty,
- _))
- .WillRepeatedly(DoAll(SetArgumentPointee<2>(kProviderTypeUnknown),
- Return(true)));
-
- const string kProviderTypeOpenVPN(flimflam::kProviderOpenVpn);
- EXPECT_CALL(storage, GetString(kVPNIdentifier1,
- flimflam::kProviderTypeProperty,
- _))
- .WillRepeatedly(DoAll(SetArgumentPointee<2>(kProviderTypeOpenVPN),
- Return(true)));
-
EXPECT_CALL(manager_, device_info())
.WillRepeatedly(Return(reinterpret_cast<DeviceInfo *>(NULL)));
-
- EXPECT_CALL(manager_, RegisterService(ServiceWithStorageId(kVPNIdentifier1)))
- .Times(1);
-
+ EXPECT_CALL(manager_,
+ RegisterService(ServiceWithStorageId(kVPNIdentifierValid)));
+ EXPECT_CALL(*profile,
+ ConfigureService(ServiceWithStorageId(kVPNIdentifierValid)))
+ .WillOnce(Return(true));
provider_.CreateServicesFromProfile(profile);
+ GetServiceAt(0)->driver()->args()->SetString(flimflam::kProviderHostProperty,
+ kHost);
// Calling this again should not create any more services (checked by the
// Times(1) above).
provider_.CreateServicesFromProfile(profile);
diff --git a/vpn_service.cc b/vpn_service.cc
index f9a9d30..9d4a94c 100644
--- a/vpn_service.cc
+++ b/vpn_service.cc
@@ -12,6 +12,7 @@
#include "shill/key_value_store.h"
#include "shill/logging.h"
#include "shill/manager.h"
+#include "shill/profile.h"
#include "shill/technology.h"
#include "shill/vpn_driver.h"
#include "shill/vpn_provider.h"
@@ -163,4 +164,32 @@
return true;
}
+void VPNService::SetNameProperty(const string &name, Error *error) {
+ if (name == friendly_name()) {
+ return;
+ }
+ LOG(INFO) << "Renaming service " << unique_name() << ": "
+ << friendly_name() << " -> " << name;
+
+ KeyValueStore *args = driver_->args();
+ if (args->LookupString(flimflam::kProviderNameProperty, "") != "") {
+ args->SetString(flimflam::kProviderNameProperty, name);
+ }
+ args->SetString(flimflam::kNameProperty, name);
+ string new_storage_id = CreateStorageIdentifier(*args, error);
+ if (new_storage_id.empty()) {
+ return;
+ }
+ string old_storage_id = storage_id_;
+ DCHECK_NE(old_storage_id, new_storage_id);
+
+ SetFriendlyName(name);
+
+ // Update the storage identifier before invoking DeleteEntry to prevent it
+ // from unloading this service.
+ storage_id_ = new_storage_id;
+ profile()->DeleteEntry(old_storage_id, NULL);
+ profile()->UpdateService(this);
+}
+
} // namespace shill
diff --git a/vpn_service.h b/vpn_service.h
index 4fc46f2..4160690 100644
--- a/vpn_service.h
+++ b/vpn_service.h
@@ -34,6 +34,7 @@
virtual bool Unload();
virtual void MakeFavorite();
virtual void SetConnection(const ConnectionRefPtr &connection);
+ virtual void SetNameProperty(const std::string &name, Error *error);
virtual void InitDriverPropertyStore();
diff --git a/vpn_service_unittest.cc b/vpn_service_unittest.cc
index 5507ecd..c4d1dba 100644
--- a/vpn_service_unittest.cc
+++ b/vpn_service_unittest.cc
@@ -7,6 +7,7 @@
#include <chromeos/dbus/service_constants.h>
#include <gtest/gtest.h>
+#include "shill/dbus_adaptor.h"
#include "shill/error.h"
#include "shill/nice_mock_control.h"
#include "shill/mock_adaptors.h"
@@ -14,6 +15,7 @@
#include "shill/mock_device_info.h"
#include "shill/mock_manager.h"
#include "shill/mock_metrics.h"
+#include "shill/mock_profile.h"
#include "shill/mock_sockets.h"
#include "shill/mock_store.h"
#include "shill/mock_vpn_driver.h"
@@ -288,4 +290,32 @@
EXPECT_FALSE(reason);
}
+TEST_F(VPNServiceTest, SetNamePropertyTrivial) {
+ DBus::Error error;
+ EXPECT_TRUE(DBusAdaptor::SetProperty(service_->mutable_store(),
+ flimflam::kNameProperty,
+ DBusAdaptor::StringToVariant(
+ service_->friendly_name()),
+ &error));
+}
+
+TEST_F(VPNServiceTest, SetNameProperty) {
+ const string kHost = "1.2.3.4";
+ driver_->args()->SetString(flimflam::kProviderHostProperty, kHost);
+ string kOldId = service_->GetStorageIdentifier();
+ DBus::Error error;
+ const string kName = "New Name";
+ scoped_refptr<MockProfile> profile(
+ new MockProfile(&control_, &metrics_, &manager_));
+ EXPECT_CALL(*profile, DeleteEntry(kOldId, _));
+ EXPECT_CALL(*profile, UpdateService(_));
+ service_->set_profile(profile);
+ EXPECT_TRUE(DBusAdaptor::SetProperty(service_->mutable_store(),
+ flimflam::kNameProperty,
+ DBusAdaptor::StringToVariant(kName),
+ &error));
+ EXPECT_NE(service_->GetStorageIdentifier(), kOldId);
+ EXPECT_EQ(kName, service_->friendly_name());
+}
+
} // namespace shill