Initate VPN reconnections when the default physical service is Online

The current logic tries to reconnect the VPN when the default physical
service changes, and is in any of the connected states.  This means
that it will try to reconnect even if a portal was detected and shill
is waiting for the user to authenticate to the portal.  This is not
desirable because the device does not have internet access in Portal
state.

Change it so that it waits until the default service is Online before
attempting a reconnection.

Bug: None
BUG=chromium:598781
TEST=`FEATURES=test emerge-link shill`

Change-Id: I97b632783b0fd1ce6a7ca36cb5b03a142263f0b6

7 files changed, 151 insertions, 16 deletions

diff --git a/vpn/third_party_vpn_driver.cc b/vpn/third_party_vpn_driver.cc
index d67369d9..75add13c 100644
--- a/vpn/third_party_vpn_driver.cc
+++ b/vpn/third_party_vpn_driver.cc
@@ -547,6 +547,21 @@ void ThirdPartyVpnDriver::OnConnectionDisconnected() {
   SLOG(this, 2) << __func__;
 }
 
+void ThirdPartyVpnDriver::TriggerReconnect(const ServiceRefPtr& new_service) {
+  StartConnectTimeout(kConnectTimeoutSeconds);
+  SLOG(this, 2) << __func__
+                << " - requesting reconnection via "
+                << new_service->unique_name();
+  if (link_down_) {
+    adaptor_interface_->EmitPlatformMessage(
+        static_cast<uint32_t>(PlatformMessage::kLinkUp));
+    link_down_ = false;
+  } else {
+    adaptor_interface_->EmitPlatformMessage(
+        static_cast<uint32_t>(PlatformMessage::kLinkChanged));
+  }
+}
+
 void ThirdPartyVpnDriver::OnDefaultServiceChanged(
     const ServiceRefPtr& service) {
   if (!service_ || !device_)
@@ -561,31 +576,31 @@ void ThirdPartyVpnDriver::OnDefaultServiceChanged(
   device_->SetServiceState(Service::kStateConfiguring);
   device_->ResetConnection();
 
-  if (!service || !service->IsConnected()) {
-    // No physical connection, so all we can do is wait.
+  if (service && service->state() == Service::kStateOnline) {
+    // The original service is no longer the default, but manager was able
+    // to find another physical service that is already Online.
+    // Ask the vpnProvider to reconnect.
+    TriggerReconnect(service);
+  } else {
+    // The default physical service went away, and nothing else is available
+    // right now.  All we can do is wait.
+    if (link_down_)
+      return;
     StopConnectTimeout();
     SLOG(this, 2) << __func__
                   << " - physical connection lost";
     link_down_ = true;
     adaptor_interface_->EmitPlatformMessage(
         static_cast<uint32_t>(PlatformMessage::kLinkDown));
-  } else {
-    // Ask the vpnProvider to reconnect.
-    StartConnectTimeout(kConnectTimeoutSeconds);
-    SLOG(this, 2) << __func__
-                  << " - requesting reconnection via "
-                  << service->unique_name();
-    if (link_down_) {
-      adaptor_interface_->EmitPlatformMessage(
-          static_cast<uint32_t>(PlatformMessage::kLinkUp));
-      link_down_ = false;
-    } else {
-      adaptor_interface_->EmitPlatformMessage(
-          static_cast<uint32_t>(PlatformMessage::kLinkChanged));
-    }
   }
 }
 
+void ThirdPartyVpnDriver::OnDefaultServiceStateChanged(
+    const ServiceRefPtr& service) {
+  if (link_down_ && service->state() == Service::kStateOnline)
+    TriggerReconnect(service);
+}
+
 void ThirdPartyVpnDriver::OnBeforeSuspend(const ResultCallback& callback) {
   if (service_ && reconnect_supported_) {
     // FIXME: Currently the VPN app receives this message at the same time
diff --git a/vpn/third_party_vpn_driver.h b/vpn/third_party_vpn_driver.h
index 43a48659..d6e9bd82 100644
--- a/vpn/third_party_vpn_driver.h
+++ b/vpn/third_party_vpn_driver.h
@@ -85,6 +85,7 @@ class ThirdPartyVpnDriver : public VPNDriver {
   void Disconnect() override;
   void OnConnectionDisconnected() override;
   void OnDefaultServiceChanged(const ServiceRefPtr& service);
+  void OnDefaultServiceStateChanged(const ServiceRefPtr& service) override;
   bool Load(StoreInterface* storage, const std::string& storage_id) override;
   bool Save(StoreInterface* storage, const std::string& storage_id,
             bool save_credentials) override;
@@ -100,6 +101,8 @@ class ThirdPartyVpnDriver : public VPNDriver {
  private:
   friend class ThirdPartyVpnDriverTest;
   FRIEND_TEST(ThirdPartyVpnDriverTest, ConnectAndDisconnect);
+  FRIEND_TEST(ThirdPartyVpnDriverTest, ReconnectionEvents);
+  FRIEND_TEST(ThirdPartyVpnDriverTest, PowerEvents);
   FRIEND_TEST(ThirdPartyVpnDriverTest, SetParameters);
   FRIEND_TEST(ThirdPartyVpnDriverTest, UpdateConnectionState);
   FRIEND_TEST(ThirdPartyVpnDriverTest, SendPacket);
@@ -193,6 +196,10 @@ class ThirdPartyVpnDriver : public VPNDriver {
   void OnInput(InputData* data);
   void OnInputError(const std::string& error);
 
+  // This function is called when a new default service first comes online,
+  // so the app knows it needs to reconnect to the VPN gateway.
+  void TriggerReconnect(const ServiceRefPtr& service);
+
   static const Property kProperties[];
 
   // This variable keeps track of the active instance. There can be multiple
diff --git a/vpn/third_party_vpn_driver_unittest.cc b/vpn/third_party_vpn_driver_unittest.cc
index 477d13ec..5a529062 100644
--- a/vpn/third_party_vpn_driver_unittest.cc
+++ b/vpn/third_party_vpn_driver_unittest.cc
@@ -16,8 +16,10 @@
 
 #include "shill/vpn/third_party_vpn_driver.h"
 
+#include <base/bind.h>
 #include <gtest/gtest.h>
 
+#include "shill/callbacks.h"
 #include "shill/mock_adaptors.h"
 #include "shill/mock_device_info.h"
 #include "shill/mock_event_dispatcher.h"
@@ -66,6 +68,8 @@ class ThirdPartyVpnDriverTest : public testing::Test {
     driver_->file_io_ = nullptr;
   }
 
+  MOCK_METHOD1(TestCallback, void(const Error& error));
+
  protected:
   static const char kConfigName[];
   static const char kInterfaceName[];
@@ -123,6 +127,105 @@ TEST_F(ThirdPartyVpnDriverTest, ConnectAndDisconnect) {
   EXPECT_EQ(driver_->io_handler_.get(), nullptr);
 }
 
+TEST_F(ThirdPartyVpnDriverTest, ReconnectionEvents) {
+  const std::string interface = kInterfaceName;
+  IOHandler* io_handler = new IOHandler();  // Owned by |driver_|
+  int fd = 1;
+
+  EXPECT_CALL(device_info_, CreateTunnelInterface(_))
+      .WillOnce(DoAll(SetArgumentPointee<0>(interface), Return(true)));
+  Error error;
+  driver_->Connect(service_, &error);
+  EXPECT_TRUE(error.IsSuccess());
+
+  EXPECT_CALL(device_info_, OpenTunnelInterface(interface))
+      .WillOnce(Return(fd));
+  EXPECT_CALL(dispatcher_, CreateInputHandler(fd, _, _))
+      .WillOnce(Return(io_handler));
+  EXPECT_TRUE(driver_->ClaimInterface(interface, kInterfaceIndex));
+
+  driver_->reconnect_supported_ = true;
+
+  // Roam from one Online network to another -> kLinkChanged.
+  scoped_refptr<MockService> mock_service(
+      new NiceMock<MockService>(&control_, &dispatcher_, &metrics_, &manager_));
+  EXPECT_CALL(*adaptor_interface_, EmitPlatformMessage(static_cast<uint32_t>(
+                                       ThirdPartyVpnDriver::kLinkChanged)));
+  EXPECT_CALL(*mock_service, state())
+      .WillOnce(Return(Service::kStateOnline));
+  driver_->OnDefaultServiceChanged(mock_service);
+
+  // Default physical service has no connection -> kLinkDown.
+  EXPECT_CALL(*adaptor_interface_, EmitPlatformMessage(static_cast<uint32_t>(
+                                       ThirdPartyVpnDriver::kLinkDown)));
+  driver_->OnDefaultServiceChanged(nullptr);
+
+  // New default physical service not online yet -> no change.
+  EXPECT_CALL(*adaptor_interface_, EmitPlatformMessage(_))
+      .Times(0);
+  EXPECT_CALL(*mock_service, state())
+      .WillOnce(Return(Service::kStateConnected));
+  driver_->OnDefaultServiceChanged(mock_service);
+
+  EXPECT_CALL(*adaptor_interface_, EmitPlatformMessage(_))
+      .Times(0);
+  EXPECT_CALL(*mock_service, state())
+      .WillOnce(Return(Service::kStatePortal));
+  driver_->OnDefaultServiceStateChanged(mock_service);
+
+  // Default physical service comes Online -> kLinkUp.
+  EXPECT_CALL(*adaptor_interface_, EmitPlatformMessage(static_cast<uint32_t>(
+                                       ThirdPartyVpnDriver::kLinkUp)));
+  EXPECT_CALL(*mock_service, state())
+      .WillOnce(Return(Service::kStateOnline));
+  driver_->OnDefaultServiceStateChanged(mock_service);
+
+  // Default physical service vanishes, but the app doesn't support
+  // reconnecting -> kDisconnected.
+  driver_->reconnect_supported_ = false;
+  EXPECT_CALL(*adaptor_interface_, EmitPlatformMessage(static_cast<uint32_t>(
+                                       ThirdPartyVpnDriver::kDisconnected)));
+  driver_->OnDefaultServiceChanged(nullptr);
+
+  driver_->Disconnect();
+}
+
+TEST_F(ThirdPartyVpnDriverTest, PowerEvents) {
+  const std::string interface = kInterfaceName;
+  IOHandler* io_handler = new IOHandler();  // Owned by |driver_|
+  int fd = 1;
+
+  EXPECT_CALL(device_info_, CreateTunnelInterface(_))
+      .WillOnce(DoAll(SetArgumentPointee<0>(interface), Return(true)));
+  Error error;
+  driver_->Connect(service_, &error);
+  EXPECT_TRUE(error.IsSuccess());
+
+  EXPECT_CALL(device_info_, OpenTunnelInterface(interface))
+      .WillOnce(Return(fd));
+  EXPECT_CALL(dispatcher_, CreateInputHandler(fd, _, _))
+      .WillOnce(Return(io_handler));
+  EXPECT_TRUE(driver_->ClaimInterface(interface, kInterfaceIndex));
+
+  driver_->reconnect_supported_ = true;
+
+  ResultCallback callback =
+      base::Bind(&ThirdPartyVpnDriverTest::TestCallback,
+                 base::Unretained(this));
+  EXPECT_CALL(*adaptor_interface_, EmitPlatformMessage(static_cast<uint32_t>(
+                                       ThirdPartyVpnDriver::kSuspend)));
+  EXPECT_CALL(*this, TestCallback(_));
+  driver_->OnBeforeSuspend(callback);
+
+  EXPECT_CALL(*adaptor_interface_, EmitPlatformMessage(static_cast<uint32_t>(
+                                       ThirdPartyVpnDriver::kResume)));
+  driver_->OnAfterResume();
+
+  EXPECT_CALL(*adaptor_interface_, EmitPlatformMessage(static_cast<uint32_t>(
+                                       ThirdPartyVpnDriver::kDisconnected)));
+  driver_->Disconnect();
+}
+
 TEST_F(ThirdPartyVpnDriverTest, SendPacket) {
   int fd = 1;
   std::string error;
diff --git a/vpn/vpn_driver.cc b/vpn/vpn_driver.cc
index e21c3d4d..dfbc5744 100644
--- a/vpn/vpn_driver.cc
+++ b/vpn/vpn_driver.cc
@@ -306,6 +306,9 @@ void VPNDriver::OnBeforeSuspend(const ResultCallback& callback) {
 void VPNDriver::OnAfterResume() {
 }
 
+void VPNDriver::OnDefaultServiceStateChanged(const ServiceRefPtr& service) {
+}
+
 string VPNDriver::GetHost() const {
   return args_.LookupString(kProviderHostProperty, "");
 }
diff --git a/vpn/vpn_driver.h b/vpn/vpn_driver.h
index 82cc8862..ea72b05e 100644
--- a/vpn/vpn_driver.h
+++ b/vpn/vpn_driver.h
@@ -62,6 +62,7 @@ class VPNDriver {
   // Power management events.
   virtual void OnBeforeSuspend(const ResultCallback& callback);
   virtual void OnAfterResume();
+  virtual void OnDefaultServiceStateChanged(const ServiceRefPtr& service);
 
   std::string GetHost() const;
 
diff --git a/vpn/vpn_service.cc b/vpn/vpn_service.cc
index 47d96290..52f0d7fe 100644
--- a/vpn/vpn_service.cc
+++ b/vpn/vpn_service.cc
@@ -267,4 +267,8 @@ void VPNService::OnAfterResume() {
   Service::OnAfterResume();
 }
 
+void VPNService::OnDefaultServiceStateChanged(const ServiceRefPtr& service) {
+  driver_->OnDefaultServiceStateChanged(service);
+}
+
 }  // namespace shill
diff --git a/vpn/vpn_service.h b/vpn/vpn_service.h
index a6ed7789..4db7ba18 100644
--- a/vpn/vpn_service.h
+++ b/vpn/vpn_service.h
@@ -54,6 +54,8 @@ class VPNService : public Service {
   // Power management events.
   virtual void OnBeforeSuspend(const ResultCallback& callback) override;
   virtual void OnAfterResume() override;
+  virtual void OnDefaultServiceStateChanged(
+      const ServiceRefPtr& service) override;
 
   virtual void InitDriverPropertyStore();