android: stop tailscaled when VPN has been revoked (#480)

-add new Ipn UI state 'Stopping' to handle the case where the VPN is no longer active and a request to stop Tailscale has been issued (but is not complete yet) and use for optimistic UI
-when VPN has been revoked, stop tailscaled and set the state to Stopping
-this fixes the race condition where when we tell tailscaled to stop, stopping races against the netmap state updating as a result of the VPN being revoked
-add isActive state and use instead of isPrepared for UI showing whether we are connected - we were previously using isPrepared as a proxy for connection, but sometimes the VPN has been prepared but is not active (eg when VPN permissions have been given and VPN has been connected previously, but has been revoked)
-refactor network callbacks into its own class for readability

Fixes tailscale/tailscale#12850

Signed-off-by: kari-ts <[email protected]>

Author: kari-ts

android/src/main/java/com/tailscale/ipn/App.kt

@@ -12,10 +12,6 @@ import android.content.Intent
 import android.content.SharedPreferences
 import android.content.pm.PackageManager
 import android.net.ConnectivityManager
-import android.net.LinkProperties
-import android.net.Network
-import android.net.NetworkCapabilities
-import android.net.NetworkRequest
 import android.os.Build
 import android.os.Environment
 import android.util.Log
@@ -45,7 +41,6 @@ import kotlinx.serialization.json.Json
 import libtailscale.Libtailscale
 import java.io.File
 import java.io.IOException
-import java.net.InetAddress
 import java.net.NetworkInterface
 import java.security.GeneralSecurityException
 import java.util.Locale
@@ -56,11 +51,6 @@ class App : UninitializedApp(), libtailscale.AppContext, ViewModelStoreOwner {
   companion object {
     private const val FILE_CHANNEL_ID = "tailscale-files"
     private const val TAG = "App"
-    private val networkConnectivityRequest =
-        NetworkRequest.Builder()
-            .addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)
-            .addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN)
-            .build()
     private lateinit var appInstance: App
 
     /**
@@ -81,9 +71,6 @@ class App : UninitializedApp(), libtailscale.AppContext, ViewModelStoreOwner {
   override val viewModelStore: ViewModelStore
     get() = appViewModelStore
 
-  lateinit var vpnViewModel: VpnViewModel
-    private set
-
   private val appViewModelStore: ViewModelStore by lazy { ViewModelStore() }
 
   var healthNotifier: HealthNotifier? = null
@@ -147,7 +134,8 @@ class App : UninitializedApp(), libtailscale.AppContext, ViewModelStoreOwner {
     Notifier.start(applicationScope)
     healthNotifier = HealthNotifier(Notifier.health, applicationScope)
     connectivityManager = this.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager
-    setAndRegisterNetworkCallbacks()
+    NetworkChangeCallback.monitorDnsChanges(connectivityManager, dns)
+    initViewModels()
     applicationScope.launch {
       Notifier.state.collect { state ->
         val ableToStartVPN = state > Ipn.State.NeedsMachineAuth
@@ -161,14 +149,13 @@ class App : UninitializedApp(), libtailscale.AppContext, ViewModelStoreOwner {
         QuickToggleService.setVPNRunning(vpnRunning)
       }
     }
-    initViewModels()
   }
 
   private fun initViewModels() {
     vpnViewModel = ViewModelProvider(this, VpnViewModelFactory(this)).get(VpnViewModel::class.java)
   }
 
-  fun setWantRunning(wantRunning: Boolean) {
+  fun setWantRunning(wantRunning: Boolean, onSuccess: (() -> Unit)? = null) {
     val callback: (Result<Ipn.Prefs>) -> Unit = { result ->
       result.fold(
           onSuccess = {},
@@ -180,41 +167,6 @@ class App : UninitializedApp(), libtailscale.AppContext, ViewModelStoreOwner {
         .editPrefs(Ipn.MaskedPrefs().apply { WantRunning = wantRunning }, callback)
   }
 
-  // requestNetwork attempts to find the best network that matches the passed NetworkRequest. It is
-  // possible that this might return an unusuable network, eg a captive portal.
-  private fun setAndRegisterNetworkCallbacks() {
-    connectivityManager.requestNetwork(
-        networkConnectivityRequest,
-        object : ConnectivityManager.NetworkCallback() {
-          override fun onAvailable(network: Network) {
-            super.onAvailable(network)
-
-            val sb = StringBuilder()
-            val linkProperties: LinkProperties? = connectivityManager.getLinkProperties(network)
-            val dnsList: MutableList<InetAddress> = linkProperties?.dnsServers ?: mutableListOf()
-            for (ip in dnsList) {
-              sb.append(ip.hostAddress).append(" ")
-            }
-            val searchDomains: String? = linkProperties?.domains
-            if (searchDomains != null) {
-              sb.append("\n")
-              sb.append(searchDomains)
-            }
-
-            if (dns.updateDNSFromNetwork(sb.toString())) {
-              Libtailscale.onDNSConfigChanged(linkProperties?.interfaceName)
-            }
-          }
-
-          override fun onLost(network: Network) {
-            super.onLost(network)
-            if (dns.updateDNSFromNetwork("")) {
-              Libtailscale.onDNSConfigChanged("")
-            }
-          }
-        })
-  }
-
   // encryptToPref a byte array of data using the Jetpack Security
   // library and writes it to a global encrypted preference store.
   @Throws(IOException::class, GeneralSecurityException::class)
@@ -389,6 +341,8 @@ open class UninitializedApp : Application() {
     private lateinit var appInstance: UninitializedApp
     lateinit var notificationManager: NotificationManagerCompat
 
+    lateinit var vpnViewModel: VpnViewModel
+
     @JvmStatic
     fun get(): UninitializedApp {
       return appInstance
@@ -550,6 +504,10 @@ open class UninitializedApp : Application() {
     return builtInDisallowedPackageNames + userDisallowed
   }
 
+  fun getAppScopedViewModel(): VpnViewModel {
+    return vpnViewModel
+  }
+
   val builtInDisallowedPackageNames: List<String> =
       listOf(
           // RCS/Jibe https://github.com/tailscale/tailscale/issues/2322

android/src/main/java/com/tailscale/ipn/IPNService.kt

@@ -10,42 +10,49 @@ import android.os.Build
 import android.system.OsConstants
 import android.util.Log
 import com.tailscale.ipn.mdm.MDMSettings
+import com.tailscale.ipn.ui.model.Ipn
+import com.tailscale.ipn.ui.notifier.Notifier
 import libtailscale.Libtailscale
 import java.util.UUID
 
 open class IPNService : VpnService(), libtailscale.IPNService {
   private val TAG = "IPNService"
   private val randomID: String = UUID.randomUUID().toString()
+  private lateinit var app: App
 
   override fun id(): String {
     return randomID
   }
 
+  override fun updateVpnStatus(status: Boolean) {
+    app.getAppScopedViewModel().setVpnActive(status)
+  }
+
   override fun onCreate() {
     super.onCreate()
     // grab app to make sure it initializes
-    App.get()
+    app = App.get()
   }
 
   override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int =
       when (intent?.action) {
         ACTION_STOP_VPN -> {
-          App.get().setWantRunning(false)
+          app.setWantRunning(false)
           close()
           START_NOT_STICKY
         }
         ACTION_START_VPN -> {
           showForegroundNotification()
-          App.get().setWantRunning(true)
+          app.setWantRunning(true)
           Libtailscale.requestVPN(this)
           START_STICKY
         }
         "android.net.VpnService" -> {
           // This means we were started by Android due to Always On VPN.
           // We show a non-foreground notification because we weren't
           // started as a foreground service.
-          App.get().notifyStatus(true)
-          App.get().setWantRunning(true)
+          app.notifyStatus(true)
+          app.setWantRunning(true)
           Libtailscale.requestVPN(this)
           START_STICKY
         }
@@ -64,6 +71,8 @@ open class IPNService : VpnService(), libtailscale.IPNService {
       }
 
   override fun close() {
+    app.setWantRunning(false) { updateVpnStatus(false) }
+    Notifier.setState(Ipn.State.Stopping)
     stopForeground(STOP_FOREGROUND_REMOVE)
     Libtailscale.serviceDisconnect(this)
   }
@@ -78,6 +87,10 @@ open class IPNService : VpnService(), libtailscale.IPNService {
     super.onRevoke()
   }
 
+  private fun setVpnPrepared(isPrepared: Boolean) {
+    app.getAppScopedViewModel().setVpnPrepared(isPrepared)
+  }
+
   private fun showForegroundNotification() {
     try {
       startForeground(

android/src/main/java/com/tailscale/ipn/MainActivity.kt

@@ -89,7 +89,7 @@ class MainActivity : ComponentActivity() {
   private lateinit var vpnPermissionLauncher: ActivityResultLauncher<Intent>
   private val viewModel: MainViewModel by lazy {
     val app = App.get()
-    vpnViewModel = app.vpnViewModel
+    vpnViewModel = app.getAppScopedViewModel()
     ViewModelProvider(this, MainViewModelFactory(vpnViewModel)).get(MainViewModel::class.java)
   }
   private lateinit var vpnViewModel: VpnViewModel
@@ -137,7 +137,7 @@ class MainActivity : ComponentActivity() {
               showOtherVPNConflictDialog()
             } else {
               Log.d("VpnPermission", "Permission was denied by the user")
-              viewModel.setVpnPrepared(false)
+              vpnViewModel.setVpnPrepared(false)
             }
           }
         }

android/src/main/java/com/tailscale/ipn/NetworkChangeCallback.kt

@@ -0,0 +1,58 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+package com.tailscale.ipn
+
+import android.content.Context
+import android.net.ConnectivityManager
+import android.net.LinkProperties
+import android.net.Network
+import android.net.NetworkCapabilities
+import android.net.NetworkRequest
+import android.util.Log
+import libtailscale.Libtailscale
+import java.net.InetAddress
+import java.net.NetworkInterface
+
+object NetworkChangeCallback {
+
+  // requestNetwork attempts to find the best network that matches the passed NetworkRequest. It is
+  // possible that this might return an unusuable network, eg a captive portal.
+  fun monitorDnsChanges(connectivityManager: ConnectivityManager, dns: DnsConfig) {
+    val networkConnectivityRequest =
+        NetworkRequest.Builder()
+            .addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)
+            .addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN)
+            .build()
+
+    connectivityManager.registerNetworkCallback(
+        networkConnectivityRequest,
+        object : ConnectivityManager.NetworkCallback() {
+          override fun onAvailable(network: Network) {
+            super.onAvailable(network)
+
+            val sb = StringBuilder()
+            val linkProperties: LinkProperties? = connectivityManager.getLinkProperties(network)
+            val dnsList: MutableList<InetAddress> = linkProperties?.dnsServers ?: mutableListOf()
+            for (ip in dnsList) {
+              sb.append(ip.hostAddress).append(" ")
+            }
+            val searchDomains: String? = linkProperties?.domains
+            if (searchDomains != null) {
+              sb.append("\n")
+              sb.append(searchDomains)
+            }
+
+            if (dns.updateDNSFromNetwork(sb.toString())) {
+              Libtailscale.onDNSConfigChanged(linkProperties?.interfaceName)
+            }
+          }
+
+          override fun onLost(network: Network) {
+            super.onLost(network)
+            if (dns.updateDNSFromNetwork("")) {
+              Libtailscale.onDNSConfigChanged("")
+            }
+          }
+        })
+  }
+}

android/src/main/java/com/tailscale/ipn/ui/model/Ipn.kt

@@ -18,7 +18,11 @@ class Ipn {
     NeedsMachineAuth(3),
     Stopped(4),
     Starting(5),
-    Running(6);
+    Running(6),
+    // Stopping represents a state where a request to stop Tailscale has been issue but has not
+    // completed. This state allows UI to optimistically reflect a stopped state, and to fallback if
+    // necessary.
+    Stopping(7);
 
     companion object {
       fun fromInt(value: Int): State {

android/src/main/java/com/tailscale/ipn/ui/notifier/Notifier.kt

@@ -33,7 +33,8 @@ object Notifier {
   private val decoder = Json { ignoreUnknownKeys = true }
 
   // General IPN Bus State
-  val state: StateFlow<Ipn.State> = MutableStateFlow(Ipn.State.NoState)
+  private val _state = MutableStateFlow(Ipn.State.NoState)
+  val state: StateFlow<Ipn.State> = _state
   val netmap: StateFlow<Netmap.NetworkMap?> = MutableStateFlow(null)
   val prefs: StateFlow<Ipn.Prefs?> = MutableStateFlow(null)
   val engineStatus: StateFlow<Ipn.EngineStatus?> = MutableStateFlow(null)
@@ -68,22 +69,22 @@ object Notifier {
               NotifyWatchOpt.Prefs.value or
               NotifyWatchOpt.InitialState.value or
                   NotifyWatchOpt.InitialHealthState.value
-      manager =
-          app.watchNotifications(mask.toLong()) { notification ->
-            val notify = decoder.decodeFromStream<Notify>(notification.inputStream())
-            notify.State?.let { state.set(Ipn.State.fromInt(it)) }
-            notify.NetMap?.let(netmap::set)
-            notify.Prefs?.let(prefs::set)
-            notify.Engine?.let(engineStatus::set)
-            notify.TailFSShares?.let(tailFSShares::set)
-            notify.BrowseToURL?.let(browseToURL::set)
-            notify.LoginFinished?.let { loginFinished.set(it.property) }
-            notify.Version?.let(version::set)
-            notify.OutgoingFiles?.let(outgoingFiles::set)
-            notify.FilesWaiting?.let(filesWaiting::set)
-            notify.IncomingFiles?.let(incomingFiles::set)
-            notify.Health?.let(health::set)
-          }
+                  manager =
+                  app.watchNotifications(mask.toLong()) { notification ->
+                      val notify = decoder.decodeFromStream<Notify>(notification.inputStream())
+                      notify.State?.let { state.set(Ipn.State.fromInt(it)) }
+                      notify.NetMap?.let(netmap::set)
+                      notify.Prefs?.let(prefs::set)
+                      notify.Engine?.let(engineStatus::set)
+                      notify.TailFSShares?.let(tailFSShares::set)
+                      notify.BrowseToURL?.let(browseToURL::set)
+                      notify.LoginFinished?.let { loginFinished.set(it.property) }
+                      notify.Version?.let(version::set)
+                      notify.OutgoingFiles?.let(outgoingFiles::set)
+                      notify.FilesWaiting?.let(filesWaiting::set)
+                      notify.IncomingFiles?.let(incomingFiles::set)
+                      notify.Health?.let(health::set)
+                  }
     }
   }
 
@@ -107,4 +108,8 @@ object Notifier {
     InitialOutgoingFiles(64),
     InitialHealthState(128),
   }
+
+  fun setState(newState: Ipn.State) {
+    _state.value = newState
+}
 }

android/src/main/java/com/tailscale/ipn/ui/view/MainView.kt

@@ -232,6 +232,9 @@ fun MainView(
                 ConnectView(
                     state,
                     isPrepared,
+                    // If Tailscale is stopping, don't automatically restart; wait for user to take
+                    // action (eg, if the user connected to another VPN).
+                    state != Ipn.State.Stopping,
                     user,
                     { viewModel.toggleVpn() },
                     { viewModel.login() },
@@ -407,6 +410,7 @@ fun StartingView() {
 fun ConnectView(
     state: Ipn.State,
     isPrepared: Boolean,
+    shouldStartAutomatically: Boolean,
     user: IpnLocal.LoginProfile?,
     connectAction: () -> Unit,
     loginAction: () -> Unit,
@@ -415,7 +419,7 @@ fun ConnectView(
     showVPNPermissionLauncherIfUnauthorized: () -> Unit
 ) {
   LaunchedEffect(isPrepared) {
-    if (!isPrepared) {
+    if (!isPrepared && shouldStartAutomatically) {
       showVPNPermissionLauncherIfUnauthorized()
     }
   }

android/src/main/java/com/tailscale/ipn/ui/viewModel/IpnViewModel.kt

@@ -134,11 +134,6 @@ open class IpnViewModel : ViewModel() {
   }
 
   // VPN Control
-
-  fun setVpnPrepared(prepared: Boolean) {
-    _vpnPrepared.value = prepared
-  }
-
   fun startVPN() {
     UninitializedApp.get().startVPN()
   }