shill: VPNDriver: Support string arrays
Add a new flag to the VPNDriver::Property that indicates that
this property is an array of strings instead of a single string.
Add handling in VPNDriver for DBus read/write and load/save for
such a property.
BUG=chromium:249363
TEST=Unit test
Change-Id: Iab11248ecb330d91cb7b12712f325a9313097275
Reviewed-on: https://gerrit.chromium.org/gerrit/58721
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Commit-Queue: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/vpn_driver.cc b/vpn_driver.cc
index ed7de93..780b1c0 100644
--- a/vpn_driver.cc
+++ b/vpn_driver.cc
@@ -4,6 +4,9 @@
#include "shill/vpn_driver.h"
+#include <string>
+#include <vector>
+
#include <base/string_util.h>
#include <chromeos/dbus/service_constants.h>
@@ -16,6 +19,7 @@
#include "shill/store_interface.h"
using std::string;
+using std::vector;
namespace shill {
@@ -42,14 +46,25 @@
continue;
}
const string property = properties_[i].property;
- string value;
- bool loaded = (properties_[i].flags & Property::kCredential) ?
- storage->GetCryptedString(storage_id, property, &value) :
- storage->GetString(storage_id, property, &value);
- if (loaded) {
- args_.SetString(property, value);
+ if (properties_[i].flags & Property::kArray) {
+ CHECK(!(properties_[i].flags & Property::kCredential))
+ << "Property cannot be both an array and a credential";
+ vector<string> value;
+ if (storage->GetStringList(storage_id, property, &value)) {
+ args_.SetStrings(property, value);
+ } else {
+ args_.RemoveStrings(property);
+ }
} else {
- args_.RemoveString(property);
+ string value;
+ bool loaded = (properties_[i].flags & Property::kCredential) ?
+ storage->GetCryptedString(storage_id, property, &value) :
+ storage->GetString(storage_id, property, &value);
+ if (loaded) {
+ args_.SetString(property, value);
+ } else {
+ args_.RemoveString(property);
+ }
}
}
return true;
@@ -65,15 +80,27 @@
}
bool credential = (properties_[i].flags & Property::kCredential);
const string property = properties_[i].property;
- if (!args_.ContainsString(property) || (credential && !save_credentials)) {
- storage->DeleteKey(storage_id, property);
- continue;
- }
- string value = args_.GetString(property);
- if (credential) {
- storage->SetCryptedString(storage_id, property, value);
+ if (properties_[i].flags & Property::kArray) {
+ CHECK(!credential)
+ << "Property cannot be both an array and a credential";
+ if (!args_.ContainsStrings(property)) {
+ storage->DeleteKey(storage_id, property);
+ continue;
+ }
+ Strings value = args_.GetStrings(property);
+ storage->SetStringList(storage_id, property, value);
} else {
- storage->SetString(storage_id, property, value);
+ if (!args_.ContainsString(property) ||
+ (credential && !save_credentials)) {
+ storage->DeleteKey(storage_id, property);
+ continue;
+ }
+ string value = args_.GetString(property);
+ if (credential) {
+ storage->SetCryptedString(storage_id, property, value);
+ } else {
+ storage->SetString(storage_id, property, value);
+ }
}
}
return true;
@@ -92,15 +119,27 @@
void VPNDriver::InitPropertyStore(PropertyStore *store) {
SLOG(VPN, 2) << __func__;
for (size_t i = 0; i < property_count_; i++) {
- store->RegisterDerivedString(
- properties_[i].property,
- StringAccessor(
- new CustomMappedAccessor<VPNDriver, string, size_t>(
- this,
- &VPNDriver::ClearMappedProperty,
- &VPNDriver::GetMappedProperty,
- &VPNDriver::SetMappedProperty,
- i)));
+ if (properties_[i].flags & Property::kArray) {
+ store->RegisterDerivedStrings(
+ properties_[i].property,
+ StringsAccessor(
+ new CustomMappedAccessor<VPNDriver, Strings, size_t>(
+ this,
+ &VPNDriver::ClearMappedStringsProperty,
+ &VPNDriver::GetMappedStringsProperty,
+ &VPNDriver::SetMappedStringsProperty,
+ i)));
+ } else {
+ store->RegisterDerivedString(
+ properties_[i].property,
+ StringAccessor(
+ new CustomMappedAccessor<VPNDriver, string, size_t>(
+ this,
+ &VPNDriver::ClearMappedStringProperty,
+ &VPNDriver::GetMappedStringProperty,
+ &VPNDriver::SetMappedStringProperty,
+ i)));
+ }
}
store->RegisterDerivedKeyValueStore(
@@ -110,7 +149,7 @@
this, &VPNDriver::GetProvider, NULL)));
}
-void VPNDriver::ClearMappedProperty(const size_t &index, Error *error) {
+void VPNDriver::ClearMappedStringProperty(const size_t &index, Error *error) {
CHECK(index < property_count_);
if (args_.ContainsString(properties_[index].property)) {
args_.RemoveString(properties_[index].property);
@@ -119,7 +158,16 @@
}
}
-string VPNDriver::GetMappedProperty(const size_t &index, Error *error) {
+void VPNDriver::ClearMappedStringsProperty(const size_t &index, Error *error) {
+ CHECK(index < property_count_);
+ if (args_.ContainsStrings(properties_[index].property)) {
+ args_.RemoveStrings(properties_[index].property);
+ } else {
+ error->Populate(Error::kNotFound, "Property is not set");
+ }
+}
+
+string VPNDriver::GetMappedStringProperty(const size_t &index, Error *error) {
// 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
@@ -130,7 +178,18 @@
return string();
}
-bool VPNDriver::SetMappedProperty(
+Strings VPNDriver::GetMappedStringsProperty(const size_t &index, Error *error) {
+ // 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 Strings();
+}
+
+bool VPNDriver::SetMappedStringProperty(
const size_t &index, const string &value, Error *error) {
CHECK(index < property_count_);
if (args_.ContainsString(properties_[index].property) &&
@@ -141,6 +200,17 @@
return true;
}
+bool VPNDriver::SetMappedStringsProperty(
+ const size_t &index, const Strings &value, Error *error) {
+ CHECK(index < property_count_);
+ if (args_.ContainsStrings(properties_[index].property) &&
+ args_.GetStrings(properties_[index].property) == value) {
+ return false;
+ }
+ args_.SetStrings(properties_[index].property, value);
+ return true;
+}
+
KeyValueStore VPNDriver::GetProvider(Error *error) {
SLOG(VPN, 2) << __func__;
string provider_prefix = string(flimflam::kProviderProperty) + ".";
@@ -151,15 +221,28 @@
continue;
}
string prop = properties_[i].property;
- if (!args_.ContainsString(prop)) {
- continue;
- }
- string value = args_.GetString(prop);
+
// Chomp off leading "Provider." from properties that have this prefix.
+ string chopped_prop;
if (StartsWithASCII(prop, provider_prefix, false)) {
- prop = prop.substr(provider_prefix.length());
+ chopped_prop = prop.substr(provider_prefix.length());
+ } else {
+ chopped_prop = prop;
}
- provider_properties.SetString(prop, value);
+
+ if (properties_[i].flags & Property::kArray) {
+ if (!args_.ContainsStrings(prop)) {
+ continue;
+ }
+ Strings value = args_.GetStrings(prop);
+ provider_properties.SetStrings(chopped_prop, value);
+ } else {
+ if (!args_.ContainsString(prop)) {
+ continue;
+ }
+ string value = args_.GetString(prop);
+ provider_properties.SetString(chopped_prop, value);
+ }
}
return provider_properties;
diff --git a/vpn_driver.h b/vpn_driver.h
index dfccf8c..b880b19 100644
--- a/vpn_driver.h
+++ b/vpn_driver.h
@@ -55,6 +55,7 @@
kEphemeral = 1 << 0, // Never load or save.
kCredential = 1 << 1, // Save if saving credentials (crypted).
kWriteOnly = 1 << 2, // Never read over RPC.
+ kArray = 1 << 3, // Property is an array of strings.
};
const char *property;
@@ -92,10 +93,15 @@
private:
friend class VPNDriverTest;
- void ClearMappedProperty(const size_t &index, Error *error);
- std::string GetMappedProperty(const size_t &index, Error *error);
- bool SetMappedProperty(
+ void ClearMappedStringProperty(const size_t &index, Error *error);
+ void ClearMappedStringsProperty(const size_t &index, Error *error);
+ std::string GetMappedStringProperty(const size_t &index, Error *error);
+ std::vector<std::string> GetMappedStringsProperty(
+ const size_t &index, Error *error);
+ bool SetMappedStringProperty(
const size_t &index, const std::string &value, Error *error);
+ bool SetMappedStringsProperty(
+ const size_t &index, const std::vector<std::string> &value, Error *error);
base::WeakPtrFactory<VPNDriver> weak_ptr_factory_;
EventDispatcher *dispatcher_;
diff --git a/vpn_driver_unittest.cc b/vpn_driver_unittest.cc
index 2cabe1c..afb78c9 100644
--- a/vpn_driver_unittest.cc
+++ b/vpn_driver_unittest.cc
@@ -23,6 +23,7 @@
#include "shill/property_store.h"
using std::string;
+using std::vector;
using testing::_;
using testing::AnyNumber;
using testing::NiceMock;
@@ -70,7 +71,9 @@
// static
const VPNDriverUnderTest::Property VPNDriverUnderTest::kProperties[] = {
+ { kEapCaCertPemProperty, Property::kArray },
{ kHostProperty, 0 },
+ { kL2tpIpsecCaCertPemProperty, Property::kArray },
{ kOTPProperty, Property::kEphemeral },
{ kPINProperty, Property::kWriteOnly },
{ kPSKProperty, Property::kCredential },
@@ -116,11 +119,19 @@
driver_.args()->SetString(arg, value);
}
+ void SetArgArray(const string &arg, const vector<string> &value) {
+ driver_.args()->SetStrings(arg, value);
+ }
+
KeyValueStore *GetArgs() { return driver_.args(); }
- bool GetProviderProperty(const PropertyStore &store,
- const string &key,
- string *value);
+ bool GetProviderPropertyString(const PropertyStore &store,
+ const string &key,
+ string *value);
+
+ bool GetProviderPropertyStrings(const PropertyStore &store,
+ const string &key,
+ vector<string> *value);
NiceMockControl control_;
NiceMock<MockDeviceInfo> device_info_;
@@ -131,9 +142,9 @@
VPNDriverUnderTest driver_;
};
-bool VPNDriverTest::GetProviderProperty(const PropertyStore &store,
- const string &key,
- string *value) {
+bool VPNDriverTest::GetProviderPropertyString(const PropertyStore &store,
+ const string &key,
+ string *value) {
KeyValueStore provider_properties;
Error error;
EXPECT_TRUE(store.GetKeyValueStoreProperty(
@@ -147,14 +158,39 @@
return true;
}
+bool VPNDriverTest::GetProviderPropertyStrings(const PropertyStore &store,
+ const string &key,
+ vector<string> *value) {
+ KeyValueStore provider_properties;
+ Error error;
+ EXPECT_TRUE(store.GetKeyValueStoreProperty(
+ flimflam::kProviderProperty, &provider_properties, &error));
+ if (!provider_properties.ContainsStrings(key)) {
+ return false;
+ }
+ if (value != NULL) {
+ *value = provider_properties.GetStrings(key);
+ }
+ return true;
+}
+
TEST_F(VPNDriverTest, Load) {
MockStore storage;
GetArgs()->SetString(kHostProperty, "1.2.3.4");
GetArgs()->SetString(kPSKProperty, "1234");
+ GetArgs()->SetStrings(kL2tpIpsecCaCertPemProperty,
+ vector<string>{ "cleared-cert0", "cleared-cert1" });
EXPECT_CALL(storage, GetString(kStorageID, _, _))
.WillRepeatedly(Return(false));
+ EXPECT_CALL(storage, GetStringList(kStorageID, _, _))
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(storage, GetString(_, kEapCaCertPemProperty, _)).Times(0);
EXPECT_CALL(storage, GetString(_, kOTPProperty, _)).Times(0);
EXPECT_CALL(storage, GetCryptedString(_, kOTPProperty, _)).Times(0);
+ EXPECT_CALL(storage, GetStringList(_, kOTPProperty, _)).Times(0);
+ vector<string> kCaCerts{ "cert0", "cert1" };
+ EXPECT_CALL(storage, GetStringList(kStorageID, kEapCaCertPemProperty, _))
+ .WillOnce(DoAll(SetArgumentPointee<2>(kCaCerts), Return(true)));
EXPECT_CALL(storage, GetString(kStorageID, kPortProperty, _))
.WillOnce(DoAll(SetArgumentPointee<2>(string(kPort)), Return(true)));
EXPECT_CALL(storage, GetString(kStorageID, kPINProperty, _))
@@ -164,12 +200,17 @@
EXPECT_CALL(storage, GetCryptedString(kStorageID, kPasswordProperty, _))
.WillOnce(DoAll(SetArgumentPointee<2>(string(kPassword)), Return(true)));
EXPECT_TRUE(driver_.Load(&storage, kStorageID));
+ EXPECT_TRUE(GetArgs()->ContainsStrings(kEapCaCertPemProperty));
+ if (GetArgs()->ContainsStrings(kEapCaCertPemProperty)) {
+ EXPECT_EQ(kCaCerts, GetArgs()->GetStrings(kEapCaCertPemProperty));
+ }
EXPECT_EQ(kPort, GetArgs()->LookupString(kPortProperty, ""));
EXPECT_EQ(kPIN, GetArgs()->LookupString(kPINProperty, ""));
EXPECT_EQ(kPassword, GetArgs()->LookupString(kPasswordProperty, ""));
// Properties missing from the persistent store should be deleted.
EXPECT_FALSE(GetArgs()->ContainsString(kHostProperty));
+ EXPECT_FALSE(GetArgs()->ContainsStrings(kL2tpIpsecCaCertPemProperty));
EXPECT_FALSE(GetArgs()->ContainsString(kPSKProperty));
}
@@ -179,8 +220,13 @@
SetArg(kPortProperty, kPort);
SetArg(kPasswordProperty, kPassword);
SetArg(kOTPProperty, "987654");
+ const vector<string> kCaCerts{ "cert0", "cert1" };
+ SetArgArray(kEapCaCertPemProperty, kCaCerts);
MockStore storage;
EXPECT_CALL(storage,
+ SetStringList(kStorageID, kEapCaCertPemProperty, kCaCerts))
+ .WillOnce(Return(true));
+ EXPECT_CALL(storage,
SetString(kStorageID, flimflam::kProviderTypeProperty, ""))
.WillOnce(Return(true));
EXPECT_CALL(storage, SetString(kStorageID, kPortProperty, kPort))
@@ -192,10 +238,13 @@
.WillOnce(Return(true));
EXPECT_CALL(storage, SetCryptedString(_, kOTPProperty, _)).Times(0);
EXPECT_CALL(storage, SetString(_, kOTPProperty, _)).Times(0);
+ EXPECT_CALL(storage, SetString(_, kEapCaCertPemProperty, _)).Times(0);
+ EXPECT_CALL(storage, DeleteKey(kStorageID, kEapCaCertPemProperty)).Times(0);
EXPECT_CALL(storage, DeleteKey(kStorageID, flimflam::kProviderTypeProperty))
.Times(0);
- EXPECT_CALL(storage, DeleteKey(kStorageID, kPSKProperty)).Times(1);
- EXPECT_CALL(storage, DeleteKey(kStorageID, kHostProperty)).Times(1);
+ EXPECT_CALL(storage, DeleteKey(kStorageID, kL2tpIpsecCaCertPemProperty));
+ EXPECT_CALL(storage, DeleteKey(kStorageID, kPSKProperty));
+ EXPECT_CALL(storage, DeleteKey(kStorageID, kHostProperty));
EXPECT_TRUE(driver_.Save(&storage, kStorageID, true));
}
@@ -206,8 +255,10 @@
EXPECT_CALL(storage, SetString(_, kPasswordProperty, _)).Times(0);
EXPECT_CALL(storage, SetCryptedString(_, kPasswordProperty, _)).Times(0);
EXPECT_CALL(storage, DeleteKey(kStorageID, _)).Times(AnyNumber());
- EXPECT_CALL(storage, DeleteKey(kStorageID, kPasswordProperty)).Times(1);
- EXPECT_CALL(storage, DeleteKey(kStorageID, kPSKProperty)).Times(1);
+ EXPECT_CALL(storage, DeleteKey(kStorageID, kPasswordProperty));
+ EXPECT_CALL(storage, DeleteKey(kStorageID, kPSKProperty));
+ EXPECT_CALL(storage, DeleteKey(kStorageID, kEapCaCertPemProperty));
+ EXPECT_CALL(storage, DeleteKey(kStorageID, kL2tpIpsecCaCertPemProperty));
EXPECT_TRUE(driver_.Save(&storage, kStorageID, false));
}
@@ -233,13 +284,22 @@
EXPECT_FALSE(store.GetStringProperty(kPortProperty, NULL, &error));
EXPECT_EQ(Error::kInvalidArguments, error.type());
}
- EXPECT_FALSE(GetProviderProperty(store, kPortProperty, NULL));
+ {
+ Error error;
+ EXPECT_FALSE(store.GetStringsProperty(kEapCaCertPemProperty, NULL, &error));
+ EXPECT_EQ(Error::kInvalidArguments, error.type());
+ }
+ EXPECT_FALSE(GetProviderPropertyString(store, kPortProperty, NULL));
+ EXPECT_FALSE(GetProviderPropertyStrings(store, kEapCaCertPemProperty, NULL));
const string kProviderType = "boo";
SetArg(kPortProperty, kPort);
SetArg(kPasswordProperty, kPassword);
SetArg(flimflam::kProviderTypeProperty, kProviderType);
SetArg(kHostProperty, "");
+ const vector<string> kCaCerts{ "cert1" };
+ SetArgArray(kEapCaCertPemProperty, kCaCerts);
+ SetArgArray(kL2tpIpsecCaCertPemProperty, vector<string>());
// We should not be able to read a property out of the driver args using the
// key to the args directly.
@@ -248,18 +308,35 @@
EXPECT_FALSE(store.GetStringProperty(kPortProperty, NULL, &error));
EXPECT_EQ(Error::kInvalidArguments, error.type());
}
+ {
+ Error error;
+ EXPECT_FALSE(store.GetStringsProperty(kEapCaCertPemProperty, NULL, &error));
+ EXPECT_EQ(Error::kInvalidArguments, error.type());
+ }
// We should instead be able to find it within the "Provider" stringmap.
{
string value;
- EXPECT_TRUE(GetProviderProperty(store, kPortProperty, &value));
+ EXPECT_TRUE(GetProviderPropertyString(store, kPortProperty, &value));
EXPECT_EQ(kPort, value);
}
+ {
+ vector<string> value;
+ EXPECT_TRUE(GetProviderPropertyStrings(store, kEapCaCertPemProperty,
+ &value));
+ EXPECT_EQ(kCaCerts, value);
+ }
// We should be able to read empty properties from the "Provider" stringmap.
{
string value;
- EXPECT_TRUE(GetProviderProperty(store, kHostProperty, &value));
+ EXPECT_TRUE(GetProviderPropertyString(store, kHostProperty, &value));
+ EXPECT_TRUE(value.empty());
+ }
+ {
+ vector<string> value;
+ EXPECT_TRUE(GetProviderPropertyStrings(store, kL2tpIpsecCaCertPemProperty,
+ &value));
EXPECT_TRUE(value.empty());
}
@@ -267,7 +344,8 @@
// name in the Properties dict with the prefix removed.
{
string value;
- EXPECT_TRUE(GetProviderProperty(store, flimflam::kTypeProperty, &value));
+ EXPECT_TRUE(GetProviderPropertyString(store, flimflam::kTypeProperty,
+ &value));
EXPECT_EQ(kProviderType, value);
}
@@ -276,7 +354,14 @@
Error error;
EXPECT_TRUE(store.ClearProperty(kPortProperty, &error));
EXPECT_TRUE(error.IsSuccess());
- EXPECT_FALSE(GetProviderProperty(store, kPortProperty, NULL));
+ EXPECT_FALSE(GetProviderPropertyString(store, kPortProperty, NULL));
+ }
+ {
+ Error error;
+ EXPECT_TRUE(store.ClearProperty(kEapCaCertPemProperty, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ EXPECT_FALSE(GetProviderPropertyStrings(store, kEapCaCertPemProperty,
+ NULL));
}
// A second attempt to clear this property should return an error.
@@ -285,9 +370,14 @@
EXPECT_FALSE(store.ClearProperty(kPortProperty, &error));
EXPECT_EQ(Error::kNotFound, error.type());
}
+ {
+ Error error;
+ EXPECT_FALSE(store.ClearProperty(kEapCaCertPemProperty, &error));
+ EXPECT_EQ(Error::kNotFound, error.type());
+ }
// Test write only properties.
- EXPECT_FALSE(GetProviderProperty(store, kPINProperty, NULL));
+ EXPECT_FALSE(GetProviderPropertyString(store, kPINProperty, NULL));
// Write properties to the driver args using the PropertyStore interface.
{
@@ -296,6 +386,13 @@
EXPECT_TRUE(store.SetStringProperty(kPINProperty, kValue, &error));
EXPECT_EQ(kValue, GetArgs()->GetString(kPINProperty));
}
+ {
+ const vector<string> kValue{ "some-value" };
+ Error error;
+ EXPECT_TRUE(store.SetStringsProperty(kEapCaCertPemProperty, kValue,
+ &error));
+ EXPECT_EQ(kValue, GetArgs()->GetStrings(kEapCaCertPemProperty));
+ }
}
TEST_F(VPNDriverTest, ConnectTimeout) {