shill: vpn: Use Mapped custom accessor for properties
VPN services simply ask their drivers to populate their PropertyStore.
VPN drivers use CustomMapped accessors to make their KeyValueStore
available.
BUG=chromium-os:28303,chromium-os:28223
TEST=New unit tests
Change-Id: I05a4f2c09ddd03b40b947274fd38572da5d6dbdc
Reviewed-on: https://gerrit.chromium.org/gerrit/18989
Commit-Ready: Paul Stewart <pstew@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/mock_vpn_driver.h b/mock_vpn_driver.h
index 58b398e..45b2e0c 100644
--- a/mock_vpn_driver.h
+++ b/mock_vpn_driver.h
@@ -24,6 +24,7 @@
const std::string &storage_id));
MOCK_METHOD2(Save, bool(StoreInterface *storage,
const std::string &storage_id));
+ MOCK_METHOD1(InitPropertyStore, void(PropertyStore *store));
private:
DISALLOW_COPY_AND_ASSIGN(MockVPNDriver);
diff --git a/mock_vpn_service.h b/mock_vpn_service.h
index a6b5640..4e6b2b1 100644
--- a/mock_vpn_service.h
+++ b/mock_vpn_service.h
@@ -21,6 +21,7 @@
virtual ~MockVPNService();
MOCK_METHOD1(SetState, void(ConnectState state));
+ MOCK_METHOD0(InitDriverPropertyStore, void());
private:
DISALLOW_COPY_AND_ASSIGN(MockVPNService);
diff --git a/openvpn_driver.cc b/openvpn_driver.cc
index f1574bd..a5d36ee 100644
--- a/openvpn_driver.cc
+++ b/openvpn_driver.cc
@@ -16,6 +16,7 @@
#include "shill/dhcp_config.h"
#include "shill/error.h"
#include "shill/manager.h"
+#include "shill/property_accessor.h"
#include "shill/rpc_task.h"
#include "shill/store_interface.h"
#include "shill/vpn.h"
@@ -48,10 +49,14 @@
const char kOpenVPNVerbProperty[] = "OpenVPN.Verb";
const char kVPNMTUProperty[] = "VPN.MTU";
-const struct {
- const char *property;
- bool crypted;
-} kProperties[] = {
+} // namespace
+
+// static
+const char OpenVPNDriver::kOpenVPNPath[] = "/usr/sbin/openvpn";
+// static
+const char OpenVPNDriver::kOpenVPNScript[] = SCRIPTDIR "/openvpn-script";
+// static
+const OpenVPNDriver::Property OpenVPNDriver::kProperties[] = {
{ flimflam::kOpenVPNAuthNoCacheProperty, false },
{ flimflam::kOpenVPNAuthProperty, false },
{ flimflam::kOpenVPNAuthRetryProperty, false },
@@ -90,14 +95,7 @@
{ kOpenVPNTLSAuthProperty, false },
{ kOpenVPNVerbProperty, false },
{ kVPNMTUProperty, false },
- { NULL, false },
};
-} // namespace
-
-// static
-const char OpenVPNDriver::kOpenVPNPath[] = "/usr/sbin/openvpn";
-// static
-const char OpenVPNDriver::kOpenVPNScript[] = SCRIPTDIR "/openvpn-script";
OpenVPNDriver::OpenVPNDriver(ControlInterface *control,
EventDispatcher *dispatcher,
@@ -536,7 +534,7 @@
}
bool OpenVPNDriver::Load(StoreInterface *storage, const string &storage_id) {
- for (int i = 0; kProperties[i].property; i++) {
+ for (size_t i = 0; i < arraysize(kProperties); i++) {
const string property = kProperties[i].property;
string value;
bool loaded = kProperties[i].crypted ?
@@ -552,7 +550,7 @@
}
bool OpenVPNDriver::Save(StoreInterface *storage, const string &storage_id) {
- for (int i = 0; kProperties[i].property; i++) {
+ for (size_t i = 0; i < arraysize(kProperties); i++) {
const string property = kProperties[i].property;
const string value = args_.LookupString(property, "");
if (value.empty()) {
@@ -566,4 +564,55 @@
return true;
}
+void OpenVPNDriver::InitPropertyStore(PropertyStore *store) {
+ for (size_t i = 0; i < arraysize(kProperties); i++) {
+ store->RegisterDerivedString(
+ kProperties[i].property,
+ StringAccessor(
+ new CustomMappedAccessor<OpenVPNDriver, string, size_t>(
+ this,
+ &OpenVPNDriver::ClearMappedProperty,
+ &OpenVPNDriver::GetMappedProperty,
+ &OpenVPNDriver::SetMappedProperty,
+ i)));
+ }
+
+ // TODO(pstew): Add the PassphraseRequired and PSKRequired properties.
+ // crosbug.com/27323
+}
+
+void OpenVPNDriver::ClearMappedProperty(const size_t &index,
+ Error *error) {
+ CHECK(index < arraysize(kProperties));
+ if (args_.ContainsString(kProperties[index].property)) {
+ args_.RemoveString(kProperties[index].property);
+ } else {
+ error->Populate(Error::kNotFound, "Property is not set");
+ }
+}
+
+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, "");
+}
+
+void OpenVPNDriver::SetMappedProperty(const size_t &index,
+ const string &value,
+ Error *error) {
+ CHECK(index < arraysize(kProperties));
+ args_.SetString(kProperties[index].property, value);
+}
+
+
} // namespace shill
diff --git a/openvpn_driver.h b/openvpn_driver.h
index 374258b..91a9ff6 100644
--- a/openvpn_driver.h
+++ b/openvpn_driver.h
@@ -55,6 +55,13 @@
virtual bool Load(StoreInterface *storage, const std::string &storage_id);
virtual bool Save(StoreInterface *storage, const std::string &storage_id);
+ virtual void InitPropertyStore(PropertyStore *store);
+ void ClearMappedProperty(const size_t &index, Error *error);
+ std::string GetMappedProperty(const size_t &index, Error *error);
+ void SetMappedProperty(const size_t &index,
+ const std::string &value,
+ Error *error);
+
private:
friend class OpenVPNDriverTest;
FRIEND_TEST(OpenVPNDriverTest, AppendFlag);
@@ -79,8 +86,14 @@
FRIEND_TEST(OpenVPNDriverTest, SpawnOpenVPN);
FRIEND_TEST(OpenVPNDriverTest, VerifyPaths);
+ struct Property {
+ const char *property;
+ bool crypted;
+ };
+
static const char kOpenVPNPath[];
static const char kOpenVPNScript[];
+ static const Property kProperties[];
// The map is a sorted container that allows us to iterate through the options
// in order.
diff --git a/openvpn_driver_unittest.cc b/openvpn_driver_unittest.cc
index 016fb04..a4c531e 100644
--- a/openvpn_driver_unittest.cc
+++ b/openvpn_driver_unittest.cc
@@ -12,6 +12,7 @@
#include <chromeos/dbus/service_constants.h>
#include <gtest/gtest.h>
+#include "shill/dbus_adaptor.h"
#include "shill/error.h"
#include "shill/ipconfig.h"
#include "shill/mock_adaptors.h"
@@ -93,6 +94,11 @@
void ExpectInFlags(const vector<string> &options, const string &flag,
const string &value);
+ bool FindStringPropertyInStore(const PropertyStore &store,
+ const string &key,
+ string *value,
+ Error *error);
+
// Inherited from RPCTaskDelegate.
virtual void Notify(const string &reason, const map<string, string> &dict);
@@ -142,6 +148,21 @@
EXPECT_EQ(value, *it);
}
+bool OpenVPNDriverTest::FindStringPropertyInStore(const PropertyStore &store,
+ const string &key,
+ string *value,
+ Error *error) {
+ ReadablePropertyConstIterator<std::string> it =
+ store.GetStringPropertiesIter();
+ 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));
@@ -568,4 +589,92 @@
EXPECT_TRUE(driver_->Save(&storage, kStorageID));
}
+TEST_F(OpenVPNDriverTest, InitPropertyStore) {
+ // Figure out if the store is actually hooked up to the driver argument
+ // KeyValueStore.
+ PropertyStore store;
+ driver_->InitPropertyStore(&store);
+
+ // An un-set property in OpenVPN should not be readable.
+ {
+ Error error;
+ string string_property;
+ EXPECT_FALSE(
+ FindStringPropertyInStore(store, flimflam::kOpenVPNKeyDirectionProperty,
+ &string_property,
+ &error));
+ EXPECT_EQ(Error::kNotFound, error.type());
+ }
+
+ const string kKeyDirection = "1";
+ const string kPassword0 = "foobar";
+ const string kPassword1 = "baz";
+ args_.SetString(flimflam::kOpenVPNKeyDirectionProperty, kKeyDirection);
+ args_.SetString(flimflam::kOpenVPNPasswordProperty, kPassword0);
+ SetArgs();
+
+ // Read a property out of the driver args using the PropertyStore interface.
+ {
+ Error error;
+ string string_property;
+ EXPECT_TRUE(
+ FindStringPropertyInStore(store, flimflam::kOpenVPNKeyDirectionProperty,
+ &string_property,
+ &error));
+ EXPECT_EQ(kKeyDirection, string_property);
+ }
+
+ // If we clear this property, we should no longer be able to find it.
+ {
+ Error error;
+ EXPECT_TRUE(store.ClearProperty(flimflam::kOpenVPNKeyDirectionProperty,
+ &error));
+ EXPECT_TRUE(error.IsSuccess());
+ }
+
+ {
+ Error error;
+ string string_property;
+ EXPECT_FALSE(
+ FindStringPropertyInStore(store, flimflam::kOpenVPNKeyDirectionProperty,
+ &string_property,
+ &error));
+ EXPECT_EQ(Error::kNotFound, error.type());
+ }
+
+ // A second attempt to clear this property should return an error.
+ {
+ Error error;
+ EXPECT_FALSE(store.ClearProperty(flimflam::kOpenVPNKeyDirectionProperty,
+ &error));
+ EXPECT_EQ(Error::kNotFound, error.type());
+ }
+
+ // This one should be write-only.
+ {
+ Error error;
+ string string_property;
+ EXPECT_FALSE(
+ FindStringPropertyInStore(store, flimflam::kOpenVPNPasswordProperty,
+ &string_property,
+ &error));
+ // We get NotFound here instead of PermissionDenied here due to the
+ // implementation of ReadablePropertyConstIterator: it shields us from
+ // store members for which Value() would have returned an error.
+ EXPECT_EQ(Error::kNotFound, error.type());
+ }
+
+ // Write a property to the driver args using the PropertyStore interface.
+ {
+ Error error;
+ EXPECT_TRUE(store.SetStringProperty(flimflam::kOpenVPNPasswordProperty,
+ kPassword1,
+ &error));
+ }
+
+ KeyValueStore *driver_args = GetArgs();
+ EXPECT_EQ(kPassword1,
+ driver_args->GetString(flimflam::kOpenVPNPasswordProperty));
+}
+
} // namespace shill
diff --git a/property_accessor.h b/property_accessor.h
index 9005192..74071fc 100644
--- a/property_accessor.h
+++ b/property_accessor.h
@@ -227,7 +227,7 @@
CustomMappedAccessor(C *target,
void(C::*clearer)(const A &argument, Error *error),
T(C::*getter)(const A &argument, Error *error),
- void(C::*setter)(const T &value, const A &argument,
+ void(C::*setter)(const A &argument, const T &value,
Error *error),
const A &argument)
: target_(target),
@@ -249,7 +249,7 @@
}
void Set(const T &value, Error *error) {
if (setter_) {
- (target_->*setter_)(value, argument_, error);
+ (target_->*setter_)(argument_, value, error);
} else {
error->Populate(Error::kInvalidArguments, "Property is read-only");
}
@@ -259,7 +259,7 @@
C *const target_;
void(C::*const clearer_)(const A &argument, Error *error);
T(C::*const getter_)(const A &argument, Error *error);
- void(C::*const setter_)(const T &value, const A &argument, Error *error);
+ void(C::*const setter_)(const A &argument, const T &value, Error *error);
A argument_;
DISALLOW_COPY_AND_ASSIGN(CustomMappedAccessor);
};
diff --git a/property_accessor_unittest.cc b/property_accessor_unittest.cc
index b52fe19..062f4f1 100644
--- a/property_accessor_unittest.cc
+++ b/property_accessor_unittest.cc
@@ -388,7 +388,7 @@
EXPECT_TRUE(ContainsKey(value_, key));
return value_[key];
}
- void Set(const string &value, const string &key, Error */*error*/) {
+ void Set(const string &key, const string &value, Error */*error*/) {
value_[key] = value;
}
diff --git a/vpn_driver.h b/vpn_driver.h
index 1f26fe6..bf5013d 100644
--- a/vpn_driver.h
+++ b/vpn_driver.h
@@ -14,6 +14,7 @@
namespace shill {
class Error;
+class PropertyStore;
class StoreInterface;
class VPNDriver {
@@ -26,6 +27,7 @@
virtual void Disconnect() = 0;
virtual bool Load(StoreInterface *storage, const std::string &storage_id) = 0;
virtual bool Save(StoreInterface *storage, const std::string &storage_id) = 0;
+ virtual void InitPropertyStore(PropertyStore *store) = 0;
};
} // namespace shill
diff --git a/vpn_provider.cc b/vpn_provider.cc
index 8ac7b38..b9daf1a 100644
--- a/vpn_provider.cc
+++ b/vpn_provider.cc
@@ -61,6 +61,7 @@
VPNServiceRefPtr service = new VPNService(
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_service.cc b/vpn_service.cc
index e2583fe..7e99962 100644
--- a/vpn_service.cc
+++ b/vpn_service.cc
@@ -80,4 +80,8 @@
driver_->Save(storage, GetStorageIdentifier());
}
+void VPNService::InitDriverPropertyStore() {
+ driver_->InitPropertyStore(mutable_store());
+}
+
} // namespace shill
diff --git a/vpn_service.h b/vpn_service.h
index 203357f..2b45b61 100644
--- a/vpn_service.h
+++ b/vpn_service.h
@@ -31,6 +31,8 @@
virtual bool Load(StoreInterface *storage);
virtual bool Save(StoreInterface *storage);
+ virtual void InitDriverPropertyStore();
+
VPNDriver *driver() const { return driver_.get(); }
static std::string CreateStorageIdentifier(const KeyValueStore &args,
diff --git a/vpn_service_unittest.cc b/vpn_service_unittest.cc
index 9e603f1..4cdebd6 100644
--- a/vpn_service_unittest.cc
+++ b/vpn_service_unittest.cc
@@ -104,4 +104,9 @@
EXPECT_TRUE(service_->Save(&storage));
}
+TEST_F(VPNServiceTest, InitPropertyStore) {
+ EXPECT_CALL(*driver_, InitPropertyStore(service_->mutable_store()));
+ service_->InitDriverPropertyStore();
+}
+
} // namespace shill