android: fix connect VPN permissions (#398)

-show VPN connection permissions after intro screen -make toggle state and main view take VPN preparedness into consideration Fixes tailscale/tailscale#12148 Signed-off-by: kari-ts <kari@tailscale.com>
6 months ago · 12ad295706
parent d842ccde22
commit 12ad295706
7 changed files with 123 additions and 117 deletions
--- a/android/src/main/java/com/tailscale/ipn/App.kt
+++ b/android/src/main/java/com/tailscale/ipn/App.kt
@ -3,9 +3,7 @@
 package com.tailscale.ipn

 import android.Manifest
-import android.app.Activity
 import android.app.Application
-import android.app.Fragment
 import android.app.Notification
 import android.app.NotificationChannel
 import android.app.PendingIntent
@ -18,7 +16,6 @@ import android.net.LinkProperties
 import android.net.Network
 import android.net.NetworkCapabilities
 import android.net.NetworkRequest
-import android.net.VpnService
 import android.os.Build
 import android.os.Environment
 import android.util.Log
@ -51,7 +48,6 @@ class App : UninitializedApp(), libtailscale.AppContext {
  val applicationScope = CoroutineScope(SupervisorJob() + Dispatchers.Default)

  companion object {
-    private const val PEER_TAG = "peer"
    private const val FILE_CHANNEL_ID = "tailscale-files"
    private const val TAG = "App"
    private val networkConnectivityRequest =
@ -61,12 +57,6 @@ class App : UninitializedApp(), libtailscale.AppContext {
            .build()
    private lateinit var appInstance: App

-    @JvmStatic
-    fun startActivityForResult(act: Activity, intent: Intent?, request: Int) {
-      val f: Fragment = act.fragmentManager.findFragmentByTag(PEER_TAG)
-      f.startActivityForResult(intent, request)
-    }
-
    /**
     * Initializes the app (if necessary) and returns the singleton app instance. Always use this
     * function to obtain an App reference to make sure the app initializes.
@ -246,20 +236,6 @@ class App : UninitializedApp(), libtailscale.AppContext {
    return packageManager.hasSystemFeature("android.hardware.type.pc")
  }

-  fun prepareVPN(act: Activity, reqCode: Int) {
-    // We do this with UI in case it's our first time starting the VPN.
-    act.runOnUiThread {
-      val prepareIntent = VpnService.prepare(this)
-      if (prepareIntent == null) {
-        // No intent here means that we already have permission to be a VPN.
-        startVPN()
-      } else {
-        // An intent here means that we need to prompt for permission to be a VPN.
-        startActivityForResult(act, prepareIntent, reqCode)
-      }
-    }
-  }
-
  override fun getInterfacesAsString(): String {
    val interfaces: ArrayList<NetworkInterface> =
        java.util.Collections.list(NetworkInterface.getNetworkInterfaces())
--- a/android/src/main/java/com/tailscale/ipn/MainActivity.kt
+++ b/android/src/main/java/com/tailscale/ipn/MainActivity.kt
@ -19,6 +19,7 @@ import androidx.activity.ComponentActivity
 import androidx.activity.compose.setContent
 import androidx.activity.result.ActivityResultLauncher
 import androidx.activity.result.contract.ActivityResultContract
+import androidx.activity.viewModels
 import androidx.browser.customtabs.CustomTabsIntent
 import androidx.compose.animation.core.tween
 import androidx.compose.animation.fadeIn
@ -38,7 +39,6 @@ import androidx.navigation.compose.NavHost
 import androidx.navigation.compose.composable
 import androidx.navigation.compose.rememberNavController
 import androidx.navigation.navArgument
-import com.tailscale.ipn.Peer.RequestCodes
 import com.tailscale.ipn.mdm.MDMSettings
 import com.tailscale.ipn.ui.model.Ipn
 import com.tailscale.ipn.ui.notifier.Notifier
@ -68,6 +68,7 @@ import com.tailscale.ipn.ui.view.TailnetLockSetupView
 import com.tailscale.ipn.ui.view.UserSwitcherNav
 import com.tailscale.ipn.ui.view.UserSwitcherView
 import com.tailscale.ipn.ui.viewModel.ExitNodePickerNav
+import com.tailscale.ipn.ui.viewModel.MainViewModel
 import com.tailscale.ipn.ui.viewModel.SettingsNav
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.cancel
@ -78,6 +79,8 @@ import kotlinx.coroutines.launch
 class MainActivity : ComponentActivity() {
  private lateinit var requestVpnPermission: ActivityResultLauncher<Unit>
  private lateinit var navController: NavHostController
+  private lateinit var vpnPermissionLauncher: ActivityResultLauncher<Intent>
+  private val viewModel: MainViewModel by viewModels()

  companion object {
    private const val TAG = "Main Activity"
@ -109,6 +112,18 @@ class MainActivity : ComponentActivity() {

    installSplashScreen()

+    vpnPermissionLauncher =
+        registerForActivityResult(VpnPermissionContract()) { granted ->
+          if (granted) {
+            Log.d("VpnPermission", "VPN permission granted")
+            viewModel.setVpnPrepared(true)
+          } else {
+            Log.d("VpnPermission", "VPN permission denied")
+            viewModel.setVpnPrepared(false)
+          }
+        }
+        viewModel.setVpnPermissionLauncher(vpnPermissionLauncher)
+
    setContent {
      AppTheme {
        navController = rememberNavController()
@ -176,7 +191,7 @@ class MainActivity : ComponentActivity() {
                          onNavigateToAuthKey = { navController.navigate("loginWithAuthKey") })

                  composable("main", enterTransition = { fadeIn(animationSpec = tween(150)) }) {
-                    MainView(loginAtUrl = ::login, navigation = mainViewNav)
+                    MainView(loginAtUrl = ::login, navigation = mainViewNav, viewModel = viewModel)
                  }
                  composable("settings") { SettingsView(settingsNav) }
                  composable("exitNodes") { ExitNodePicker(exitNodePickerNav) }
@ -231,13 +246,6 @@ class MainActivity : ComponentActivity() {
        }
      }
    }
-    lifecycleScope.launch {
-      Notifier.state.collect { state ->
-        if (state > Ipn.State.Stopped) {
-          App.get().prepareVPN(this@MainActivity, RequestCodes.requestPrepareVPN)
-        }
-      }
-    }
  }

  init {
@ -322,10 +330,6 @@ class MainActivity : ComponentActivity() {

  override fun onStart() {
    super.onStart()
-
-    // (jonathan) TODO: Requesting VPN permissions onStart is a bit aggressive.  This should
-    // be done when the user initiall starts the VPN
-    requestVpnPermission()
  }

  override fun onStop() {
@ -335,18 +339,6 @@ class MainActivity : ComponentActivity() {
    lifecycleScope.launch(Dispatchers.IO) { MDMSettings.update(App.get(), restrictionsManager) }
  }

-  private fun requestVpnPermission() {
-    val vpnIntent = VpnService.prepare(this)
-    if (vpnIntent != null) {
-      val contract = VpnPermissionContract()
-      requestVpnPermission =
-          registerForActivityResult(contract) { granted ->
-            Log.i("VPN", "VPN permission ${if (granted) "granted" else "denied"}")
-          }
-      requestVpnPermission.launch(Unit)
-    }
-  }
-
  private fun openApplicationSettings() {
    val intent =
        Intent(Settings.ACTION_APP_NOTIFICATION_SETTINGS).apply {
@ -368,9 +360,9 @@ class MainActivity : ComponentActivity() {
  }
 }

-class VpnPermissionContract : ActivityResultContract<Unit, Boolean>() {
-  override fun createIntent(context: Context, input: Unit): Intent {
-    return VpnService.prepare(context) ?: Intent()
+class VpnPermissionContract : ActivityResultContract<Intent, Boolean>() {
+  override fun createIntent(context: Context, input: Intent): Intent {
+    return input
  }

  override fun parseResult(resultCode: Int, intent: Intent?): Boolean {
--- a/android/src/main/java/com/tailscale/ipn/Peer.java
+++ b/android/src/main/java/com/tailscale/ipn/Peer.java
@ -1,28 +0,0 @@
-// Copyright (c) Tailscale Inc & AUTHORS
-// SPDX-License-Identifier: BSD-3-Clause
-
-package com.tailscale.ipn;
-
-import android.app.Fragment;
-import android.content.Intent;
-
-public class Peer extends Fragment {
-
-    private static int resultOK = -1;
-
-    public class RequestCodes {
-        public static final int requestPrepareVPN = 1001;
-    }
-
-    @Override
-    public void onActivityResult(int requestCode, int resultCode, Intent data) {
-        if (requestCode == RequestCodes.requestPrepareVPN) {
-            if (resultCode == resultOK) {
-                UninitializedApp.get().startVPN();
-            } else {
-                App.get().setWantRunning(false);
-                // notify VPN revoked
-            }
-        }
-    }
-}
--- a/android/src/main/java/com/tailscale/ipn/ui/view/MainView.kt
+++ b/android/src/main/java/com/tailscale/ipn/ui/view/MainView.kt
@ -46,6 +46,7 @@ import androidx.compose.runtime.getValue
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
 import androidx.compose.runtime.setValue
+import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.draw.alpha
@ -88,6 +89,7 @@ import com.tailscale.ipn.ui.util.PeerSet
 import com.tailscale.ipn.ui.util.flag
 import com.tailscale.ipn.ui.util.itemsWithDividers
 import com.tailscale.ipn.ui.viewModel.MainViewModel
+import android.util.Log

 // Navigation actions for the MainView
 data class MainViewNavigation(
@ -101,14 +103,17 @@ data class MainViewNavigation(
 fun MainView(
    loginAtUrl: (String) -> Unit,
    navigation: MainViewNavigation,
-    viewModel: MainViewModel = viewModel()
+    viewModel: MainViewModel
 ) {
-  val isOn = viewModel.vpnToggleState.collectAsState()
+  
  LoadingIndicator.Wrap {
    Scaffold(contentWindowInsets = WindowInsets.Companion.statusBars) { paddingInsets ->
      Column(
          modifier = Modifier.fillMaxWidth().padding(paddingInsets),
          verticalArrangement = Arrangement.Center) {
+            // Assume VPN has been prepared. Whether or not it has been prepared cannot be known until permission has been granted to prepare the VPN.
+            val isPrepared by viewModel.vpnPrepared.collectAsState(initial=true)
+            val isOn by viewModel.vpnToggleState.collectAsState(initial = false)
            val state by viewModel.ipnState.collectAsState(initial = Ipn.State.NoState)
            val user by viewModel.loggedInUser.collectAsState(initial = null)
            val stateVal by viewModel.stateRes.collectAsState(initial = R.string.placeholder)
@ -128,7 +133,7 @@ fun MainView(
                        }
                      },
                      enabled = !disableToggle,
-                      checked = isOn.value)
+                      checked = isOn)
                },
                headlineContent = {
                  user?.NetworkProfile?.DomainName?.let { domain ->
@ -185,11 +190,13 @@ fun MainView(
              else -> {
                ConnectView(
                    state,
+                    isPrepared,
                    user,
                    { viewModel.toggleVpn() },
                    { viewModel.login() },
                    loginAtUrl,
-                    netmap?.SelfNode)
+                    netmap?.SelfNode, 
+                    {viewModel.showVPNPermissionLauncherIfUnauthorized()})
              }
            }
          }
@ -300,12 +307,19 @@ fun StartingView() {
@Composable
 fun ConnectView(
    state: Ipn.State,
+    isPrepared: Boolean,
    user: IpnLocal.LoginProfile?,
    connectAction: () -> Unit,
    loginAction: () -> Unit,
    loginAtUrlAction: (String) -> Unit,
-    selfNode: Tailcfg.Node?
+    selfNode: Tailcfg.Node?,
+    showVPNPermissionLauncherIfUnauthorized: () -> Unit
 ) {
+  LaunchedEffect(isPrepared) {
+    if (!isPrepared) {
+      showVPNPermissionLauncherIfUnauthorized()
+    }
+}
  Row(horizontalArrangement = Arrangement.Center, modifier = Modifier.fillMaxWidth()) {
    Column(horizontalAlignment = Alignment.CenterHorizontally, modifier = Modifier.fillMaxWidth()) {
      Column(
@ -313,7 +327,24 @@ fun ConnectView(
          verticalArrangement = Arrangement.spacedBy(8.dp, alignment = Alignment.CenterVertically),
          horizontalAlignment = Alignment.CenterHorizontally,
      ) {
-        if (state == Ipn.State.NeedsMachineAuth) {
+        if (!isPrepared) { 
+          TailscaleLogoView(modifier = Modifier.size(50.dp))
+          Spacer(modifier = Modifier.size(1.dp))
+          Text(
+              text = stringResource(id = R.string.welcome_to_tailscale),
+              style = MaterialTheme.typography.titleMedium,
+              textAlign = TextAlign.Center)
+          Text(
+              stringResource(R.string.give_permissions),
+              style = MaterialTheme.typography.titleSmall,
+              textAlign = TextAlign.Center)
+          Spacer(modifier = Modifier.size(1.dp))
+          PrimaryActionButton(onClick = connectAction) {
+            Text(
+                text = stringResource(id = R.string.connect),
+                fontSize = MaterialTheme.typography.titleMedium.fontSize)
+          }
+        } else if (state == Ipn.State.NeedsMachineAuth) {
          Icon(
              modifier = Modifier.size(40.dp),
              imageVector = Icons.Outlined.Lock,
--- a/android/src/main/java/com/tailscale/ipn/ui/viewModel/IpnViewModel.kt
+++ b/android/src/main/java/com/tailscale/ipn/ui/viewModel/IpnViewModel.kt
@ -59,19 +59,11 @@ open class IpnViewModel : ViewModel() {
  }

  // VPN Control
-
-  fun toggleVpn() {
-    when (Notifier.state.value) {
-      Ipn.State.Running -> stopVPN()
-      else -> startVPN()
-    }
-  }
-
  fun startVPN() {
    UninitializedApp.get().startVPN()
  }

-  private fun stopVPN() {
+ fun stopVPN() {
    UninitializedApp.get().stopVPN()
  }

--- a/android/src/main/java/com/tailscale/ipn/ui/viewModel/MainViewModel.kt
+++ b/android/src/main/java/com/tailscale/ipn/ui/viewModel/MainViewModel.kt
@ -3,10 +3,15 @@

 package com.tailscale.ipn.ui.viewModel

+import android.content.Intent
+import android.net.VpnService
 import android.util.Log
+import androidx.activity.result.ActivityResultLauncher
 import androidx.lifecycle.viewModelScope
+import com.tailscale.ipn.App
 import com.tailscale.ipn.R
 import com.tailscale.ipn.mdm.MDMSettings
+import com.tailscale.ipn.ui.model.Ipn
 import com.tailscale.ipn.ui.model.Ipn.State
 import com.tailscale.ipn.ui.notifier.Notifier
 import com.tailscale.ipn.ui.util.PeerCategorizer
@ -16,17 +21,23 @@ import com.tailscale.ipn.ui.util.set
 import com.tailscale.ipn.App
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
+import kotlinx.coroutines.flow.combine
 import kotlinx.coroutines.launch
 import java.time.Duration

 class MainViewModel : IpnViewModel() {

  // The user readable state of the system
-  val stateRes: StateFlow<Int> = MutableStateFlow(State.NoState.userStringRes(null))
+  val stateRes: StateFlow<Int> = MutableStateFlow(userStringRes(State.NoState, State.NoState, true))

  // The expected state of the VPN toggle
  private val _vpnToggleState = MutableStateFlow(false)
-  val vpnToggleState: StateFlow<Boolean> = _vpnToggleState  
+  val vpnToggleState: StateFlow<Boolean> = _vpnToggleState
+
+  // Whether or not the VPN has been prepared
+  private val _vpnPrepared = MutableStateFlow(false)
+  val vpnPrepared: StateFlow<Boolean> = _vpnPrepared
+  private var vpnPermissionLauncher: ActivityResultLauncher<Intent>? = null

  // The list of peers
  val peers: StateFlow<List<PeerSet>> = MutableStateFlow(emptyList<PeerSet>())
@ -49,16 +60,20 @@ class MainViewModel : IpnViewModel() {

    viewModelScope.launch {
      var previousState: State? = null
-      Notifier.state.collect { currentState ->
-          val userString = currentState.userStringRes(previousState)
-          stateRes.set(userString)
-          _vpnToggleState.value = when {
-              currentState == State.Running || currentState == State.Starting -> true
-              previousState == State.NoState && currentState == State.Starting -> true
-              else -> false
+  
+      combine(Notifier.state, vpnPrepared) { state, prepared -> state to prepared }
+          .collect { (currentState, prepared) ->
+              stateRes.set(userStringRes(currentState, previousState, prepared))
+  
+              val isOn = when {
+                  currentState == State.Running || currentState == State.Starting -> true
+                  previousState == State.NoState && currentState == State.Starting -> true
+                  else -> false
+              }
+  
+              _vpnToggleState.value = isOn
+              previousState = currentState
          }
-          previousState = currentState
-      }
  }

    viewModelScope.launch {
@ -91,22 +106,48 @@ class MainViewModel : IpnViewModel() {
    }
  }

+  fun showVPNPermissionLauncherIfUnauthorized() {
+    val vpnIntent = VpnService.prepare(App.get())
+    if (vpnIntent != null) {
+      vpnPermissionLauncher?.launch(vpnIntent)
+    }
+  }
+
+  fun toggleVpn() {
+    val state = Notifier.state.value
+    val isPrepared = vpnPrepared.value
+
+    when {
+        !isPrepared -> showVPNPermissionLauncherIfUnauthorized()
+        state == Ipn.State.Running -> stopVPN()
+        else -> startVPN()
+    }
+}
+
  fun searchPeers(searchTerm: String) {
    this.searchTerm.set(searchTerm)
  }
+
+  fun setVpnPermissionLauncher(launcher: ActivityResultLauncher<Intent>) {
+    vpnPermissionLauncher = launcher
+  }
+
+  fun setVpnPrepared(prepared: Boolean) {
+    _vpnPrepared.value = prepared
+  }
 }

-private fun State?.userStringRes(previousState: State?): Int {
-  val resId = when {
-      previousState == State.NoState && this == State.Starting -> R.string.starting
-      this == State.NoState -> R.string.placeholder
-      this == State.InUseOtherUser -> R.string.placeholder
-      this == State.NeedsLogin -> R.string.please_login
-      this == State.NeedsMachineAuth -> R.string.needs_machine_auth
-      this == State.Stopped -> R.string.stopped
-      this == State.Starting -> R.string.starting
-      this == State.Running -> R.string.connected
+private fun userStringRes(currentState: State?, previousState: State?, vpnPrepared: Boolean): Int {
+  return when {
+      previousState == State.NoState && currentState == State.Starting -> R.string.starting
+      currentState == State.NoState -> R.string.placeholder
+      currentState == State.InUseOtherUser -> R.string.placeholder
+      currentState == State.NeedsLogin -> if (vpnPrepared) R.string.please_login else R.string.connect_to_vpn
+      currentState == State.NeedsMachineAuth -> R.string.needs_machine_auth
+      currentState == State.Stopped -> R.string.stopped
+      currentState == State.Starting -> R.string.starting
+      currentState == State.Running -> R.string.connected
      else -> R.string.placeholder
  }
  return resId
-}
+}
--- a/android/src/main/res/values/strings.xml
+++ b/android/src/main/res/values/strings.xml
@ -58,6 +58,7 @@
    <string name="connect_to_tailnet_suffix">" tailnet."</string>
    <string name="welcome_to_tailscale">Welcome to Tailscale</string>
    <string name="login_to_join_your_tailnet">Log in to join your tailnet and connect your devices.</string>
+    <string name="give_permissions">Set up a VPN connection so you can start using Tailscale.</string>
    <string name="keyExpiryExplainer">Reauthenticate to remain connected to Tailscale.</string>
    <string name="deviceKeyNeverExpires">Device key does not expire</string>
    <string name="deviceKeyExpires">Device key expires %s</string>
@ -87,6 +88,7 @@
    <string name="placeholder">--</string>
    <string name="please_login">Login required</string>
    <string name="stopped">Not connected</string>
+    <string name="connect_to_vpn">Connect to VPN</string>

    <!-- Time conversion templates -->
    <string name="expired">expired</string>