shill: vpn: Properly export provider properties
Provider properties should be exported as a "Provider" dict instead
of at the same toplevel as the rest of the Service properties. This
means that there is a difference between the way these properties
are stored in the keystore and accesed over DBus for writing and how
they are enumerated in "GetProperties".
Also fix VPNProvider::GetService so that all properties are applied,
even the ones that should modify Service level attributes, like GUID
and UIData, by using Service::Configure instead of passing the
KeyValueStore directly into the driver.
BUG=chromium-os:29287
TEST=New unit tests + Manual:
- Test that logging in to an ONC-enabled account creates the service
automatically. Moreover, make sure that the profile data is written
out and materially quite similar to flimflam's profile data.
- Test that logging out and logging back in maintains that data.
- Test that the VPN entry appears in the networks dropdown
Change-Id: I60a807886d17074d3f88b1475dcf42e80c0cec54
Reviewed-on: https://gerrit.chromium.org/gerrit/20046
Commit-Ready: Darin Petkov <petkov@chromium.org>
Reviewed-by: Darin Petkov <petkov@chromium.org>
Tested-by: Darin Petkov <petkov@chromium.org>
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 8362882..18f1f75 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -475,9 +475,7 @@
ProfileRefPtr profile(
new Profile(control_interface(), &manager, id, "", false));
MockStore *storage = new MockStore;
- // Say we don't have |s2| the first time asked, then that we do.
EXPECT_CALL(*storage, ContainsGroup(s2->GetStorageIdentifier()))
- .WillOnce(Return(false))
.WillRepeatedly(Return(true));
EXPECT_CALL(*storage, Flush())
.Times(AnyNumber())
@@ -487,7 +485,7 @@
}
// Create a profile that already has |s2| in it.
ProfileRefPtr profile(new EphemeralProfile(control_interface(), &manager));
- profile->AdoptService(s2);
+ EXPECT_TRUE(profile->AdoptService(s2));
// Now, move the Service |s2| to another profile.
EXPECT_CALL(*s2.get(), Save(_)).WillOnce(Return(true));
diff --git a/mock_openvpn_driver.cc b/mock_openvpn_driver.cc
index aa82f49..93c3335 100644
--- a/mock_openvpn_driver.cc
+++ b/mock_openvpn_driver.cc
@@ -6,8 +6,8 @@
namespace shill {
-MockOpenVPNDriver::MockOpenVPNDriver(const KeyValueStore &args)
- : OpenVPNDriver(NULL, NULL, NULL, NULL, NULL, NULL, args) {}
+MockOpenVPNDriver::MockOpenVPNDriver()
+ : OpenVPNDriver(NULL, NULL, NULL, NULL, NULL, NULL) {}
MockOpenVPNDriver::~MockOpenVPNDriver() {}
diff --git a/mock_openvpn_driver.h b/mock_openvpn_driver.h
index 7213582..08a18ee 100644
--- a/mock_openvpn_driver.h
+++ b/mock_openvpn_driver.h
@@ -13,7 +13,7 @@
class MockOpenVPNDriver : public OpenVPNDriver {
public:
- MockOpenVPNDriver(const KeyValueStore &args);
+ MockOpenVPNDriver();
virtual ~MockOpenVPNDriver();
MOCK_METHOD0(OnReconnecting, void());
diff --git a/openvpn_driver.cc b/openvpn_driver.cc
index 9ea08ae..0cf2dcd 100644
--- a/openvpn_driver.cc
+++ b/openvpn_driver.cc
@@ -112,15 +112,13 @@
Metrics *metrics,
Manager *manager,
DeviceInfo *device_info,
- GLib *glib,
- const KeyValueStore &args)
+ GLib *glib)
: control_(control),
dispatcher_(dispatcher),
metrics_(metrics),
manager_(manager),
device_info_(device_info),
glib_(glib),
- args_(args),
management_server_(new OpenVPNManagementServer(this, glib)),
nss_(NSS::GetInstance()),
pid_(0),
@@ -666,6 +664,12 @@
i)));
}
+ store->RegisterDerivedStringmap(
+ flimflam::kProviderProperty,
+ StringmapAccessor(
+ new CustomAccessor<OpenVPNDriver, Stringmap>(
+ this, &OpenVPNDriver::GetProvider, NULL)));
+
// TODO(pstew): Add the PassphraseRequired and PSKRequired properties.
// crosbug.com/27323
}
@@ -686,18 +690,15 @@
string OpenVPNDriver::GetMappedProperty(const size_t &index,
Error *error) {
- CHECK(index < arraysize(kProperties));
- if (kProperties[index].crypted) {
- error->Populate(Error::kPermissionDenied, "Property is write-only");
- return string();
- }
-
- if (!args_.ContainsString(kProperties[index].property)) {
- error->Populate(Error::kNotFound, "Property is not set");
- return string();
- }
-
- return args_.LookupString(kProperties[index].property, "");
+ // Provider properties are set via SetProperty calls to "Provider.XXX",
+ // however, they are retrieved via a GetProperty call, which returns
+ // all properties in a single "Provider" dict. Therefore, none of
+ // the individual properties in the kProperties are available for
+ // enumeration in GetProperties. Instead, they are retrieved via
+ // GetProvider below.
+ error->Populate(Error::kInvalidArguments,
+ "Provider properties are not read back in this manner");
+ return string();
}
void OpenVPNDriver::SetMappedProperty(const size_t &index,
@@ -707,5 +708,30 @@
args_.SetString(kProperties[index].property, value);
}
+Stringmap OpenVPNDriver::GetProvider(Error *error) {
+ string provider_prefix = string(flimflam::kProviderProperty) + ".";
+ Stringmap provider_properties;
+
+ for (size_t i = 0; i < arraysize(kProperties); i++) {
+ // Never return any encrypted properties.
+ if (kProperties[i].crypted) {
+ continue;
+ }
+
+ string prop = kProperties[i].property;
+
+ // Chomp off leading "Provider." from properties that have this prefix.
+ if (StartsWithASCII(prop, provider_prefix, false)) {
+ prop = prop.substr(provider_prefix.length());
+ }
+
+ if (args_.ContainsString(kProperties[i].property)) {
+ provider_properties[prop] =
+ args_.LookupString(kProperties[i].property, "");
+ }
+ }
+
+ return provider_properties;
+}
} // namespace shill
diff --git a/openvpn_driver.h b/openvpn_driver.h
index d5cedb5..f6cf7ce 100644
--- a/openvpn_driver.h
+++ b/openvpn_driver.h
@@ -41,8 +41,7 @@
Metrics *metrics,
Manager *manager,
DeviceInfo *device_info,
- GLib *glib,
- const KeyValueStore &args);
+ GLib *glib);
virtual ~OpenVPNDriver();
// Inherited from VPNDriver. |Connect| initiates the VPN connection by
@@ -70,6 +69,8 @@
void SetMappedProperty(const size_t &index,
const std::string &value,
Error *error);
+ Stringmap GetProvider(Error *error);
+
KeyValueStore *args() { return &args_; }
diff --git a/openvpn_driver_unittest.cc b/openvpn_driver_unittest.cc
index 05b257f..bacf277 100644
--- a/openvpn_driver_unittest.cc
+++ b/openvpn_driver_unittest.cc
@@ -54,7 +54,7 @@
: device_info_(&control_, NULL, NULL, NULL),
manager_(&control_, &dispatcher_, &metrics_, &glib_),
driver_(new OpenVPNDriver(&control_, &dispatcher_, &metrics_, &manager_,
- &device_info_, &glib_, args_)),
+ &device_info_, &glib_)),
service_(new MockVPNService(&control_, &dispatcher_, &metrics_,
&manager_, driver_)),
device_(new MockVPN(&control_, &dispatcher_, &metrics_, &manager_,
@@ -105,6 +105,10 @@
const string &key,
string *value,
Error *error);
+ bool FindStringmapPropertyInStore(const PropertyStore &store,
+ const string &key,
+ Stringmap *value,
+ Error *error);
// Inherited from RPCTaskDelegate.
virtual void Notify(const string &reason, const map<string, string> &dict);
@@ -115,7 +119,6 @@
MockMetrics metrics_;
MockGLib glib_;
MockManager manager_;
- KeyValueStore args_;
OpenVPNDriver *driver_; // Owned by |service_|.
scoped_refptr<MockVPNService> service_;
scoped_refptr<MockVPN> device_;
@@ -175,6 +178,22 @@
return false;
}
+bool OpenVPNDriverTest::FindStringmapPropertyInStore(const PropertyStore &store,
+ const string &key,
+ Stringmap *value,
+ Error *error) {
+ ReadablePropertyConstIterator<Stringmap> it =
+ store.GetStringmapPropertiesIter();
+ for ( ; !it.AtEnd(); it.Advance()) {
+ if (it.Key() == key) {
+ *value = it.Value(error);
+ return error->IsSuccess();
+ }
+ }
+ error->Populate(Error::kNotFound);
+ return false;
+}
+
TEST_F(OpenVPNDriverTest, Connect) {
EXPECT_CALL(*service_, SetState(Service::kStateConfiguring));
const string interface = kInterfaceName;
@@ -722,18 +741,48 @@
const string kKeyDirection = "1";
const string kPassword0 = "foobar";
const string kPassword1 = "baz";
+ const string kProviderName = "boo";
SetArg(flimflam::kOpenVPNKeyDirectionProperty, kKeyDirection);
SetArg(flimflam::kOpenVPNPasswordProperty, kPassword0);
+ SetArg(flimflam::kProviderNameProperty, kProviderName);
- // Read a property out of the driver args using the PropertyStore interface.
+ // We should not be able to read a property out of the driver args using
+ // the key to the args directly.
{
Error error;
string string_property;
- EXPECT_TRUE(
+ EXPECT_FALSE(
FindStringPropertyInStore(store, flimflam::kOpenVPNKeyDirectionProperty,
&string_property,
&error));
- EXPECT_EQ(kKeyDirection, string_property);
+ EXPECT_EQ(Error::kNotFound, error.type());
+ }
+
+ // We should instead be able to find it within the "Provider" stringmap.
+ {
+ Error error;
+ Stringmap provider_properties;
+ EXPECT_TRUE(
+ FindStringmapPropertyInStore(store, flimflam::kProviderProperty,
+ &provider_properties,
+ &error));
+ EXPECT_TRUE(ContainsKey(provider_properties,
+ flimflam::kOpenVPNKeyDirectionProperty));
+ EXPECT_EQ(kKeyDirection,
+ provider_properties[flimflam::kOpenVPNKeyDirectionProperty]);
+ }
+
+ // Properties that start with the prefix "Provider." should be mapped to
+ // the name in the Properties dict with the prefix removed.
+ {
+ Error error;
+ Stringmap provider_properties;
+ EXPECT_TRUE(
+ FindStringmapPropertyInStore(store, flimflam::kProviderProperty,
+ &provider_properties,
+ &error));
+ EXPECT_TRUE(ContainsKey(provider_properties, flimflam::kNameProperty));
+ EXPECT_EQ(kProviderName, provider_properties[flimflam::kNameProperty]);
}
// If we clear this property, we should no longer be able to find it.
diff --git a/openvpn_management_server_unittest.cc b/openvpn_management_server_unittest.cc
index acab874..09b9240 100644
--- a/openvpn_management_server_unittest.cc
+++ b/openvpn_management_server_unittest.cc
@@ -36,8 +36,7 @@
class OpenVPNManagementServerTest : public testing::Test {
public:
OpenVPNManagementServerTest()
- : driver_(args_),
- server_(&driver_, &glib_) {}
+ : server_(&driver_, &glib_) {}
virtual ~OpenVPNManagementServerTest() {}
@@ -82,7 +81,6 @@
static const int kConnectedSocket;
GLib glib_;
- KeyValueStore args_;
MockOpenVPNDriver driver_;
OpenVPNManagementServer server_;
MockSockets sockets_;
diff --git a/profile.cc b/profile.cc
index f3d7dd2..46d589d 100644
--- a/profile.cc
+++ b/profile.cc
@@ -141,8 +141,9 @@
}
bool Profile::AdoptService(const ServiceRefPtr &service) {
- if (storage_->ContainsGroup(service->GetStorageIdentifier()))
+ if (service->profile() == this) {
return false;
+ }
service->set_profile(this);
return service->Save(storage_.get()) && storage_->Flush();
}
diff --git a/vpn_provider.cc b/vpn_provider.cc
index 5387cc9..9d10689 100644
--- a/vpn_provider.cc
+++ b/vpn_provider.cc
@@ -55,8 +55,19 @@
// Find a service in the provider list which matches these parameters.
VPNServiceRefPtr service = FindService(type, storage_id);
+
if (service == NULL) {
- service = CreateService(type, storage_id, args, error);
+ // 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);
+ }
+
+ if (service != NULL) {
+ // Configure the service using the the rest of the passed-in arguments.
+ service->Configure(args, error);
}
return service;
@@ -84,6 +95,7 @@
}
void VPNProvider::CreateServicesFromProfile(ProfileRefPtr profile) {
+ VLOG(2) << __func__;
const StoreInterface *storage = profile->GetConstStorage();
set<string> groups =
storage->GetGroupsWithKey(flimflam::kProviderTypeProperty);
@@ -99,6 +111,14 @@
continue;
}
+ string name;
+ if (!storage->GetString(*it, flimflam::kProviderNameProperty, &name) &&
+ !storage->GetString(*it, flimflam::kNameProperty, &name)) {
+ LOG(ERROR) << "Group " << *it << " is missing the "
+ << flimflam::kProviderNameProperty << " property.";
+ continue;
+ }
+
VPNServiceRefPtr service = FindService(type, *it);
if (service != NULL) {
// If the service already exists, it does not need to be configured,
@@ -107,11 +127,8 @@
continue;
}
- // Create a service with an empty |args| parameter. We will populate
- // the service with properties using ConfigureService() below.
- KeyValueStore args;
Error error;
- service = CreateService(type, *it, args, &error);
+ service = CreateService(type, name, *it, &error);
if (service == NULL) {
LOG(ERROR) << "Could not create service for " << *it;
@@ -126,14 +143,16 @@
}
VPNServiceRefPtr VPNProvider::CreateService(const string &type,
+ const string &name,
const string &storage_id,
- const KeyValueStore &args,
Error *error) {
+ VLOG(2) << __func__ << " type " << type << " name " << name
+ << " storage id " << storage_id;
scoped_ptr<VPNDriver> driver;
if (type == flimflam::kProviderOpenVpn) {
driver.reset(new OpenVPNDriver(
control_interface_, dispatcher_, metrics_, manager_,
- manager_->device_info(), manager_->glib(), args));
+ manager_->device_info(), manager_->glib()));
} else {
Error::PopulateAndLog(
error, Error::kNotSupported, "Unsupported VPN type: " + type);
@@ -144,7 +163,6 @@
control_interface_, dispatcher_, metrics_, manager_, driver.release());
service->set_storage_id(storage_id);
service->InitDriverPropertyStore();
- string name = args.LookupString(flimflam::kProviderNameProperty, "");
if (!name.empty()) {
service->set_friendly_name(name);
}
diff --git a/vpn_provider.h b/vpn_provider.h
index 2fae9ce..158107c 100644
--- a/vpn_provider.h
+++ b/vpn_provider.h
@@ -55,8 +55,8 @@
// to the newly created service, or popuplates |error| with an the error
// that caused this to fail.
VPNServiceRefPtr CreateService(const std::string &type,
+ const std::string &name,
const std::string &storage_id,
- const KeyValueStore &args,
Error *error);
// Find a service of type |type| whose storage identifier is |storage_id|.
diff --git a/vpn_provider_unittest.cc b/vpn_provider_unittest.cc
index e67072e..62c7f95 100644
--- a/vpn_provider_unittest.cc
+++ b/vpn_provider_unittest.cc
@@ -177,12 +177,17 @@
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,
diff --git a/vpn_service.cc b/vpn_service.cc
index 4d29ed7..27e219c 100644
--- a/vpn_service.cc
+++ b/vpn_service.cc
@@ -58,9 +58,12 @@
}
string name = args.LookupString(flimflam::kProviderNameProperty, "");
if (name.empty()) {
- Error::PopulateAndLog(
- error, Error::kNotSupported, "Missing VPN name.");
- return "";
+ name = args.LookupString(flimflam::kNameProperty, "");
+ if (name.empty()) {
+ Error::PopulateAndLog(
+ error, Error::kNotSupported, "Missing VPN name.");
+ return "";
+ }
}
string id = StringPrintf("vpn_%s_%s", host.c_str(), name.c_str());
replace_if(id.begin(), id.end(), &Service::IllegalChar, '_');