shill: vpn: Auto-connect at most one service at a time, only when online.
This patch suppresses auto-connect for non-primary technologies (such
as VPN) while the system is offline. Also, don't auto-connect a VPN
service if there's already an active (connecting or connected) VPN
service.
Cleanup Manager's online/offline API a bit.
BUG=chromium-os:38229
TEST=unit tests
Change-Id: Iedd3879cf45c8b509a956415c7de5e5ba1af4652
Reviewed-on: https://gerrit.chromium.org/gerrit/42226
Tested-by: Darin Petkov <petkov@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: Darin Petkov <petkov@chromium.org>
diff --git a/manager.cc b/manager.cc
index 6dc1991..98a22c5 100644
--- a/manager.cc
+++ b/manager.cc
@@ -1194,13 +1194,13 @@
}
}
-string Manager::CalculateState(Error */*error*/) {
+bool Manager::IsOnline() const {
// |services_| is sorted such that connected services are first.
- if (!services_.empty() &&
- services_.front()->IsConnected()) {
- return flimflam::kStateOnline;
- }
- return flimflam::kStateOffline;
+ return !services_.empty() && services_.front()->IsConnected();
+}
+
+string Manager::CalculateState(Error */*error*/) {
+ return IsOnline() ? flimflam::kStateOnline : flimflam::kStateOffline;
}
vector<string> Manager::AvailableTechnologies(Error */*error*/) {
diff --git a/manager.h b/manager.h
index f0b43af..d4989bd 100644
--- a/manager.h
+++ b/manager.h
@@ -200,6 +200,9 @@
// Return whether |storage| is for the default profile.
virtual bool IsDefaultProfile(const StoreInterface *storage) const;
+ // Returns true if at least one connection exists, and false if there's no
+ // connected service.
+ virtual bool IsOnline() const;
std::string CalculateState(Error *error);
virtual int GetPortalCheckInterval() const {
@@ -282,8 +285,6 @@
FRIEND_TEST(CellularTest, ConnectAddsTerminationAction);
FRIEND_TEST(CellularTest, LinkEventWontDestroyService);
FRIEND_TEST(ManagerTest, AvailableTechnologies);
- FRIEND_TEST(ManagerTest, CalculateStateOffline);
- FRIEND_TEST(ManagerTest, CalculateStateOnline);
FRIEND_TEST(ManagerTest, ConnectedTechnologies);
FRIEND_TEST(ManagerTest, DefaultTechnology);
FRIEND_TEST(ManagerTest, DeviceRegistrationAndStart);
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 3b0461e..74c0983 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -120,6 +120,10 @@
}
virtual ~ManagerTest() {}
+ void SetMetrics(Metrics *metrics) {
+ manager()->set_metrics(metrics);
+ }
+
bool IsDeviceRegistered(const DeviceRefPtr &device,
Technology::Identifier tech) {
vector<DeviceRefPtr> devices;
@@ -1901,7 +1905,7 @@
TEST_F(ManagerTest, SortServicesWithConnection) {
MockMetrics mock_metrics;
- manager()->set_metrics(&mock_metrics);
+ SetMetrics(&mock_metrics);
scoped_refptr<MockService> mock_service0(
new NiceMock<MockService>(control_interface(),
@@ -1980,7 +1984,7 @@
EXPECT_TRUE(manager()->default_service_callbacks_.empty());
MockMetrics mock_metrics;
- manager()->set_metrics(&mock_metrics);
+ SetMetrics(&mock_metrics);
scoped_refptr<MockService> mock_service(
new NiceMock<MockService>(
@@ -2620,8 +2624,11 @@
TEST_F(ManagerTest, CalculateStateOffline) {
+ EXPECT_FALSE(manager()->IsOnline());
+ EXPECT_EQ("offline", manager()->CalculateState(NULL));
+
MockMetrics mock_metrics;
- manager()->set_metrics(&mock_metrics);
+ SetMetrics(&mock_metrics);
EXPECT_CALL(mock_metrics, NotifyDefaultServiceChanged(_))
.Times(AnyNumber());
scoped_refptr<MockService> mock_service0(
@@ -2644,6 +2651,7 @@
manager()->RegisterService(mock_service0);
manager()->RegisterService(mock_service1);
+ EXPECT_FALSE(manager()->IsOnline());
EXPECT_EQ("offline", manager()->CalculateState(NULL));
manager()->DeregisterService(mock_service0);
@@ -2652,7 +2660,7 @@
TEST_F(ManagerTest, CalculateStateOnline) {
MockMetrics mock_metrics;
- manager()->set_metrics(&mock_metrics);
+ SetMetrics(&mock_metrics);
EXPECT_CALL(mock_metrics, NotifyDefaultServiceChanged(_))
.Times(AnyNumber());
scoped_refptr<MockService> mock_service0(
@@ -2680,6 +2688,7 @@
manager()->RegisterService(mock_service1);
CompleteServiceSort();
+ EXPECT_TRUE(manager()->IsOnline());
EXPECT_EQ("online", manager()->CalculateState(NULL));
manager()->DeregisterService(mock_service0);
diff --git a/mock_manager.h b/mock_manager.h
index ce21c3f..8b1ef7e 100644
--- a/mock_manager.h
+++ b/mock_manager.h
@@ -43,6 +43,7 @@
bool (const ProfileRefPtr &profile,
const std::string &entry_name));
MOCK_CONST_METHOD0(GetDefaultService, ServiceRefPtr());
+ MOCK_CONST_METHOD0(IsOnline, bool());
MOCK_METHOD0(UpdateEnabledTechnologies, void());
MOCK_METHOD1(LoadDeviceFromProfiles, void(const DeviceRefPtr &device));
MOCK_METHOD1(IsPortalDetectionEnabled, bool(Technology::Identifier tech));
diff --git a/mock_vpn_provider.h b/mock_vpn_provider.h
index c914e5b..a8d9872 100644
--- a/mock_vpn_provider.h
+++ b/mock_vpn_provider.h
@@ -21,6 +21,7 @@
MOCK_METHOD0(Stop, void());
MOCK_METHOD2(OnDeviceInfoAvailable, bool(const std::string &link_name,
int interface_index));
+ MOCK_CONST_METHOD0(HasActiveService, bool());
DISALLOW_COPY_AND_ASSIGN(MockVPNProvider);
};
diff --git a/openvpn_driver.cc b/openvpn_driver.cc
index 2e2651d..ca8aea4 100644
--- a/openvpn_driver.cc
+++ b/openvpn_driver.cc
@@ -692,8 +692,7 @@
// establish connection as soon as it's started. Otherwise, hold the client
// until an underlying service connects and OnDefaultServiceChanged is
// invoked.
- ServiceRefPtr service = manager()->GetDefaultService();
- if (service && service->IsConnected()) {
+ if (manager()->IsOnline()) {
management_server_->ReleaseHold();
}
return true;
diff --git a/openvpn_driver.h b/openvpn_driver.h
index ac97459..2fcc2cd 100644
--- a/openvpn_driver.h
+++ b/openvpn_driver.h
@@ -89,7 +89,6 @@
FRIEND_TEST(OpenVPNDriverTest, InitClientAuthOptions);
FRIEND_TEST(OpenVPNDriverTest, InitEnvironment);
FRIEND_TEST(OpenVPNDriverTest, InitLoggingOptions);
- FRIEND_TEST(OpenVPNDriverTest, InitManagementChannelOptions);
FRIEND_TEST(OpenVPNDriverTest, InitOptions);
FRIEND_TEST(OpenVPNDriverTest, InitOptionsHostWithPort);
FRIEND_TEST(OpenVPNDriverTest, InitOptionsNoHost);
diff --git a/openvpn_driver_unittest.cc b/openvpn_driver_unittest.cc
index 8e84d5e..8ad04c1 100644
--- a/openvpn_driver_unittest.cc
+++ b/openvpn_driver_unittest.cc
@@ -114,6 +114,14 @@
return device_->selected_service();
}
+ bool InitManagementChannelOptions(vector<string> *options, Error *error) {
+ return driver_->InitManagementChannelOptions(options, error);
+ }
+
+ Sockets *GetSockets() {
+ return &driver_->sockets_;
+ }
+
// Used to assert that a flag appears in the options.
void ExpectInFlags(const vector<string> &options, const string &flag,
const string &value);
@@ -475,8 +483,7 @@
driver_->rpc_task_.reset(new RPCTask(&control_, this));
driver_->tunnel_interface_ = kInterfaceName;
EXPECT_CALL(*management_server_, Start(_, _, _)).WillOnce(Return(true));
- ServiceRefPtr null_service;
- EXPECT_CALL(manager_, GetDefaultService()).WillOnce(Return(null_service));
+ EXPECT_CALL(manager_, IsOnline()).WillOnce(Return(false));
Error error;
vector<string> options;
@@ -505,8 +512,7 @@
driver_->rpc_task_.reset(new RPCTask(&control_, this));
driver_->tunnel_interface_ = kInterfaceName;
EXPECT_CALL(*management_server_, Start(_, _, _)).WillOnce(Return(true));
- ServiceRefPtr null_service;
- EXPECT_CALL(manager_, GetDefaultService()).WillOnce(Return(null_service));
+ EXPECT_CALL(manager_, IsOnline()).WillOnce(Return(false));
Error error;
vector<string> options;
@@ -626,43 +632,35 @@
ExpectInFlags(options, "--pkcs11-providers", kProvider);
}
-TEST_F(OpenVPNDriverTest, InitManagementChannelOptions) {
+TEST_F(OpenVPNDriverTest, InitManagementChannelOptionsServerFail) {
vector<string> options;
+ EXPECT_CALL(*management_server_, Start(&dispatcher_, GetSockets(), &options))
+ .WillOnce(Return(false));
Error error;
-
- EXPECT_CALL(*management_server_,
- Start(&dispatcher_, &driver_->sockets_, &options))
- .Times(4)
- .WillOnce(Return(false))
- .WillRepeatedly(Return(true));
-
- // Management server fails to start.
- EXPECT_FALSE(driver_->InitManagementChannelOptions(&options, &error));
+ EXPECT_FALSE(InitManagementChannelOptions(&options, &error));
EXPECT_EQ(Error::kInternalError, error.type());
EXPECT_EQ("Unable to setup management channel.", error.message());
+}
- // Start with a connected default service.
- scoped_refptr<MockService> mock_service(
- new MockService(&control_, &dispatcher_, &metrics_, &manager_));
- EXPECT_CALL(manager_, GetDefaultService()).WillOnce(Return(mock_service));
- EXPECT_CALL(*mock_service, IsConnected()).WillOnce(Return(true));
+TEST_F(OpenVPNDriverTest, InitManagementChannelOptionsOnline) {
+ vector<string> options;
+ EXPECT_CALL(*management_server_, Start(&dispatcher_, GetSockets(), &options))
+ .WillOnce(Return(true));
+ EXPECT_CALL(manager_, IsOnline()).WillOnce(Return(true));
EXPECT_CALL(*management_server_, ReleaseHold());
- error.Reset();
- EXPECT_TRUE(driver_->InitManagementChannelOptions(&options, &error));
+ Error error;
+ EXPECT_TRUE(InitManagementChannelOptions(&options, &error));
EXPECT_TRUE(error.IsSuccess());
+}
- // Start with a disconnected default service.
- EXPECT_CALL(manager_, GetDefaultService()).WillOnce(Return(mock_service));
- EXPECT_CALL(*mock_service, IsConnected()).WillOnce(Return(false));
+TEST_F(OpenVPNDriverTest, InitManagementChannelOptionsOffline) {
+ vector<string> options;
+ EXPECT_CALL(*management_server_, Start(&dispatcher_, GetSockets(), &options))
+ .WillOnce(Return(true));
+ EXPECT_CALL(manager_, IsOnline()).WillOnce(Return(false));
EXPECT_CALL(*management_server_, ReleaseHold()).Times(0);
- EXPECT_TRUE(driver_->InitManagementChannelOptions(&options, &error));
- EXPECT_TRUE(error.IsSuccess());
-
- // Start with no default service.
- ServiceRefPtr null_service;
- EXPECT_CALL(manager_, GetDefaultService()).WillOnce(Return(null_service));
- EXPECT_CALL(*management_server_, ReleaseHold()).Times(0);
- EXPECT_TRUE(driver_->InitManagementChannelOptions(&options, &error));
+ Error error;
+ EXPECT_TRUE(InitManagementChannelOptions(&options, &error));
EXPECT_TRUE(error.IsSuccess());
}
@@ -737,8 +735,7 @@
static const char kHost[] = "192.168.2.254";
SetArg(flimflam::kProviderHostProperty, kHost);
EXPECT_CALL(*management_server_, Start(_, _, _)).WillOnce(Return(true));
- ServiceRefPtr null_service;
- EXPECT_CALL(manager_, GetDefaultService()).WillOnce(Return(null_service));
+ EXPECT_CALL(manager_, IsOnline()).WillOnce(Return(false));
EXPECT_CALL(glib_, SpawnAsyncWithPipesCWD(_, _, _, _, _, _, _, _, _, _))
.WillOnce(Return(true));
EXPECT_CALL(glib_, ChildWatchAdd(_, _, _)).WillOnce(Return(1));
@@ -817,10 +814,7 @@
EXPECT_CALL(*management_server_, Start(_, _, _))
.Times(2)
.WillRepeatedly(Return(true));
- ServiceRefPtr null_service;
- EXPECT_CALL(manager_, GetDefaultService())
- .Times(2)
- .WillRepeatedly(Return(null_service));
+ EXPECT_CALL(manager_, IsOnline()).Times(2).WillRepeatedly(Return(false));
const int kPID = 234678;
EXPECT_CALL(glib_,
diff --git a/service.cc b/service.cc
index ad03cdc..0863c07 100644
--- a/service.cc
+++ b/service.cc
@@ -44,6 +44,7 @@
const char Service::kAutoConnConnecting[] = "connecting";
const char Service::kAutoConnExplicitDisconnect[] = "explicitly disconnected";
const char Service::kAutoConnNotConnectable[] = "not connectable";
+const char Service::kAutoConnOffline[] = "offline";
const char Service::kAutoConnThrottled[] = "throttled";
const size_t Service::kEAPMaxCertificationElements = 10;
@@ -1082,6 +1083,12 @@
return false;
}
+ if (!Technology::IsPrimaryConnectivityTechnology(technology_) &&
+ !manager_->IsOnline()) {
+ *reason = kAutoConnOffline;
+ return false;
+ }
+
return true;
}
diff --git a/service.h b/service.h
index aba1399..eb72a29 100644
--- a/service.h
+++ b/service.h
@@ -572,6 +572,7 @@
static const char kAutoConnConnecting[];
static const char kAutoConnExplicitDisconnect[];
static const char kAutoConnNotConnectable[];
+ static const char kAutoConnOffline[];
static const char kAutoConnThrottled[];
static const size_t kEAPMaxCertificationElements;
diff --git a/service_unittest.cc b/service_unittest.cc
index fe57996..6bbc033 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -541,8 +541,17 @@
}
TEST_F(ServiceTest, IsAutoConnectable) {
- const char *reason;
+ const char *reason = NULL;
service_->set_connectable(true);
+
+ // Services with non-primary connectivity technologies should not auto-connect
+ // when the system is offline.
+ EXPECT_EQ(Technology::kUnknown, service_->technology());
+ EXPECT_CALL(mock_manager_, IsOnline()).WillOnce(Return(false));
+ EXPECT_FALSE(service_->IsAutoConnectable(&reason));
+ EXPECT_STREQ(Service::kAutoConnOffline, reason);
+
+ service_->technology_ = Technology::kEthernet;
EXPECT_TRUE(service_->IsAutoConnectable(&reason));
// We should not auto-connect to a Service that a user has
@@ -607,6 +616,7 @@
TEST_F(AllMockServiceTest, AutoConnectWithFailures) {
const char *reason;
service_->set_connectable(true);
+ service_->technology_ = Technology::kEthernet;
EXPECT_TRUE(service_->IsAutoConnectable(&reason));
// The very first AutoConnect() doesn't trigger any throttling.
diff --git a/vpn_provider.cc b/vpn_provider.cc
index 0fce3df..754b1a1 100644
--- a/vpn_provider.cc
+++ b/vpn_provider.cc
@@ -186,4 +186,14 @@
return NULL;
}
+bool VPNProvider::HasActiveService() const {
+ for (vector<VPNServiceRefPtr>::const_iterator it = services_.begin();
+ it != services_.end(); ++it) {
+ if ((*it)->IsConnecting() || (*it)->IsConnected()) {
+ return true;
+ }
+ }
+ return false;
+}
+
} // namespace shill
diff --git a/vpn_provider.h b/vpn_provider.h
index 610a31b..cadb17a 100644
--- a/vpn_provider.h
+++ b/vpn_provider.h
@@ -47,7 +47,11 @@
void CreateServicesFromProfile(ProfileRefPtr profile);
+ // Returns true if any of the managed VPN services is connecting or connected.
+ virtual bool HasActiveService() const;
+
private:
+ friend class VPNProviderTest;
FRIEND_TEST(VPNProviderTest, CreateService);
FRIEND_TEST(VPNProviderTest, OnDeviceInfoAvailable);
FRIEND_TEST(VPNProviderTest, RemoveService);
diff --git a/vpn_provider_unittest.cc b/vpn_provider_unittest.cc
index 5a66c76..61777be 100644
--- a/vpn_provider_unittest.cc
+++ b/vpn_provider_unittest.cc
@@ -44,6 +44,15 @@
return service->friendly_name();
}
+ void SetConnectState(const ServiceRefPtr &service,
+ Service::ConnectState state) {
+ service->state_ = state;
+ }
+
+ void AddService(const VPNServiceRefPtr &service) {
+ provider_.services_.push_back(service);
+ }
+
NiceMockControl control_;
MockMetrics metrics_;
MockManager manager_;
@@ -242,4 +251,26 @@
EXPECT_EQ(Error::kNotSupported, error.type());
}
+TEST_F(VPNProviderTest, HasActiveService) {
+ EXPECT_FALSE(provider_.HasActiveService());
+
+ scoped_refptr<MockVPNService> service0(
+ new MockVPNService(&control_, NULL, &metrics_, NULL, NULL));
+ scoped_refptr<MockVPNService> service1(
+ new MockVPNService(&control_, NULL, &metrics_, NULL, NULL));
+ scoped_refptr<MockVPNService> service2(
+ new MockVPNService(&control_, NULL, &metrics_, NULL, NULL));
+
+ AddService(service0);
+ AddService(service1);
+ AddService(service2);
+ EXPECT_FALSE(provider_.HasActiveService());
+
+ SetConnectState(service1, Service::kStateAssociating);
+ EXPECT_TRUE(provider_.HasActiveService());
+
+ SetConnectState(service1, Service::kStateOnline);
+ EXPECT_TRUE(provider_.HasActiveService());
+}
+
} // namespace shill
diff --git a/vpn_service.cc b/vpn_service.cc
index a7991e3..e5cdc0c 100644
--- a/vpn_service.cc
+++ b/vpn_service.cc
@@ -24,6 +24,7 @@
namespace shill {
const char VPNService::kAutoConnNeverConnected[] = "never connected";
+const char VPNService::kAutoConnVPNAlreadyActive[] = "vpn already active";
VPNService::VPNService(ControlInterface *control,
EventDispatcher *dispatcher,
@@ -153,6 +154,11 @@
*reason = kAutoConnNeverConnected;
return false;
}
+ // Don't auto-connect a VPN service if another VPN service is already active.
+ if (manager()->vpn_provider()->HasActiveService()) {
+ *reason = kAutoConnVPNAlreadyActive;
+ return false;
+ }
return true;
}
diff --git a/vpn_service.h b/vpn_service.h
index 4a6bc9b..4fc46f2 100644
--- a/vpn_service.h
+++ b/vpn_service.h
@@ -48,11 +48,12 @@
virtual bool IsAutoConnectable(const char **reason) const;
private:
+ friend class VPNServiceTest;
FRIEND_TEST(VPNServiceTest, GetDeviceRpcId);
- FRIEND_TEST(VPNServiceTest, IsAutoConnectable);
FRIEND_TEST(VPNServiceTest, SetConnection);
static const char kAutoConnNeverConnected[];
+ static const char kAutoConnVPNAlreadyActive[];
virtual std::string GetDeviceRpcId(Error *error);
diff --git a/vpn_service_unittest.cc b/vpn_service_unittest.cc
index a7076d6..f464c6c 100644
--- a/vpn_service_unittest.cc
+++ b/vpn_service_unittest.cc
@@ -67,6 +67,22 @@
service_->connectable_ = connectable;
}
+ const char *GetAutoConnOffline() {
+ return Service::kAutoConnOffline;
+ }
+
+ const char *GetAutoConnNeverConnected() {
+ return VPNService::kAutoConnNeverConnected;
+ }
+
+ const char *GetAutoConnVPNAlreadyActive() {
+ return VPNService::kAutoConnVPNAlreadyActive;
+ }
+
+ bool IsAutoConnectable(const char **reason) const {
+ return service_->IsAutoConnectable(reason);
+ }
+
std::string interface_name_;
std::string ipconfig_rpc_identifier_;
MockVPNDriver *driver_; // Owned by |service_|.
@@ -219,21 +235,51 @@
connection_->OnLowerDisconnect();
}
-TEST_F(VPNServiceTest, IsAutoConnectable) {
+TEST_F(VPNServiceTest, IsAutoConnectableOffline) {
+ EXPECT_TRUE(service_->connectable());
+ const char *reason = NULL;
+ EXPECT_CALL(manager_, IsOnline()).WillOnce(Return(false));
+ EXPECT_FALSE(IsAutoConnectable(&reason));
+ EXPECT_STREQ(GetAutoConnOffline(), reason);
+}
+
+TEST_F(VPNServiceTest, IsAutoConnectableNeverConnected) {
EXPECT_TRUE(service_->connectable());
EXPECT_FALSE(service_->has_ever_connected());
-
const char *reason = NULL;
- EXPECT_FALSE(service_->IsAutoConnectable(&reason));
- EXPECT_STREQ(VPNService::kAutoConnNeverConnected, reason);
+ EXPECT_CALL(manager_, IsOnline()).WillOnce(Return(true));
+ EXPECT_FALSE(IsAutoConnectable(&reason));
+ EXPECT_STREQ(GetAutoConnNeverConnected(), reason);
+}
+TEST_F(VPNServiceTest, IsAutoConnectableVPNAlreadyActive) {
+ EXPECT_TRUE(service_->connectable());
SetHasEverConnected(true);
- reason = NULL;
- EXPECT_TRUE(service_->IsAutoConnectable(&reason));
- EXPECT_FALSE(reason);
+ EXPECT_CALL(manager_, IsOnline()).WillOnce(Return(true));
+ MockVPNProvider provider;
+ EXPECT_CALL(manager_, vpn_provider()).WillOnce(Return(&provider));
+ EXPECT_CALL(provider, HasActiveService()).WillOnce(Return(true));
+ const char *reason = NULL;
+ EXPECT_FALSE(IsAutoConnectable(&reason));
+ EXPECT_STREQ(GetAutoConnVPNAlreadyActive(), reason);
+}
+TEST_F(VPNServiceTest, IsAutoConnectableNotConnectable) {
+ const char *reason = NULL;
SetConnectable(false);
- EXPECT_FALSE(service_->IsAutoConnectable(&reason));
+ EXPECT_FALSE(IsAutoConnectable(&reason));
+}
+
+TEST_F(VPNServiceTest, IsAutoConnectable) {
+ EXPECT_TRUE(service_->connectable());
+ SetHasEverConnected(true);
+ EXPECT_CALL(manager_, IsOnline()).WillOnce(Return(true));
+ MockVPNProvider provider;
+ EXPECT_CALL(manager_, vpn_provider()).WillOnce(Return(&provider));
+ EXPECT_CALL(provider, HasActiveService()).WillOnce(Return(false));
+ const char *reason = NULL;
+ EXPECT_TRUE(IsAutoConnectable(&reason));
+ EXPECT_FALSE(reason);
}
} // namespace shill