From a97f0156aa3682d7826588fa19fcebc4882a5818 Mon Sep 17 00:00:00 2001
From: Gabi <gabrielalejandro7@gmail.com>
Date: Thu, 5 Sep 2024 14:35:43 -0700
Subject: [PATCH] fix(android): prevent onLost from triggering a session
 disconnect when the VPN is re-established just after (#6605)

On Android when when we receive the `onLost` callback for the VPN we
disconnect the tunnel, since that's the only way to find when the user
has disconnected the VPN from outside Firezone, otherwise this would
kill network connection when it's this happens.

But `onLost` was also called when we call `establish` on the tunnel to
recreate it and there's no obvious way to distinguish between both
cases.

So we previously just stopped monitoring `onLost` while we were
executing `establish`. The problem with this approach was that sometimes
`onLost` might have a delayed since `establish` is called.

Therefore, to fix this, we changed the `onLost` call to delay the
execution of `session.disconnect` for 2 second, if within this grace
period we get a `linkPropertiesChanged` for our interface, which is
always called after the network is re-created, we cancel the execution,
otherwise we go ahead an disconnect the session.
---
 .../android/tunnel/DisconnectMonitor.kt       | 24 ++++++++++++++++++-
 .../firezone/android/tunnel/TunnelService.kt  |  6 ++---
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/DisconnectMonitor.kt b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/DisconnectMonitor.kt
index cd31fd696..2c7b77cb2 100644
--- a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/DisconnectMonitor.kt
+++ b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/DisconnectMonitor.kt
@@ -2,6 +2,9 @@
 import android.net.ConnectivityManager
 import android.net.LinkProperties
 import android.net.Network
+import android.os.Handler
+import android.os.Looper
+import android.util.Log
 import dev.firezone.android.tunnel.TunnelService
 
 // None of the TunnelService lifecycle callbacks are called when a user disconnects the VPN
@@ -10,12 +13,17 @@ import dev.firezone.android.tunnel.TunnelService
 class DisconnectMonitor(private val tunnelService: TunnelService) : ConnectivityManager.NetworkCallback() {
     private var vpnNetwork: Network? = null
 
+    // This handler is used to stop connlib when the VPN fd is lost
+    private var sessionStopperHandle: Handler = Handler(Looper.getMainLooper())
+
     // Android doesn't provide a good way to associate a network with a VPN service, so we
     // have to use the IP addresses of the tunnel to determine if the network is our VPN.
     override fun onLinkPropertiesChanged(
         network: Network,
         linkProperties: LinkProperties,
     ) {
+        Log.d("DisconnectMonitor", "properties: $linkProperties")
+
         super.onLinkPropertiesChanged(network, linkProperties)
 
         if (tunnelService.tunnelIpv4Address.isNullOrBlank() || tunnelService.tunnelIpv6Address.isNullOrBlank()) {
@@ -26,6 +34,9 @@ class DisconnectMonitor(private val tunnelService: TunnelService) : Connectivity
         val ipv6Found = linkProperties.linkAddresses.find { it.address.hostAddress == tunnelService.tunnelIpv6Address }
 
         if (ipv4Found != null && ipv6Found != null) {
+            // When we get onLinkPropertiesChanged it means the interface is available again and we
+            // should abort stopping the session
+            sessionStopperHandle.removeCallbacksAndMessages(null)
             // Matched both IPv4 and IPv6 addresses, this is our VPN network
             vpnNetwork = network
         }
@@ -33,7 +44,18 @@ class DisconnectMonitor(private val tunnelService: TunnelService) : Connectivity
 
     override fun onLost(network: Network) {
         if (network == vpnNetwork) {
-            tunnelService.disconnect()
+            Log.d("DisconnectMonitor", "Scheduling a session disconnect for $network")
+
+            // We delay the execution of disconnecting the tunnel when the network is lost since
+            // when the tunnel is rebuild we get an onLost just like with disabling the VPN and we
+            // can't distinguish between those save for getting an onLinkProperties changed later
+            sessionStopperHandle.postDelayed(
+                {
+                    Log.d("DisconnectMonitor", "Disconnect tunnel service due to network lost $network")
+                    tunnelService.disconnect()
+                },
+                2000,
+            )
         }
 
         super.onLost(network)
diff --git a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt
index 7b9bb498b..d9fb51334 100644
--- a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt
+++ b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt
@@ -136,6 +136,7 @@ class TunnelService : VpnService() {
             // service.
             override fun onDisconnect(error: String): Boolean {
                 stopNetworkMonitoring()
+                stopDisconnectMonitoring()
 
                 // Clear any user tokens and actorNames
                 repo.clearToken()
@@ -164,8 +165,6 @@ class TunnelService : VpnService() {
             }
         }
 
-        stopDisconnectMonitoring()
-
         Builder().apply {
             if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {
                 setMetered(false) // Inherit the metered status from the underlying networks.
@@ -203,8 +202,6 @@ class TunnelService : VpnService() {
                 ConnlibSession.setTun(it, fd)
             }
         }
-
-        startDisconnectMonitoring()
     }
 
     private val restrictionsFilter = IntentFilter(Intent.ACTION_APPLICATION_RESTRICTIONS_CHANGED)
@@ -333,6 +330,7 @@ class TunnelService : VpnService() {
                 )
 
             startNetworkMonitoring()
+            startDisconnectMonitoring()
         }
     }