Skip to content

android: replace broadcast intent with service intent #650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+45 −56
Open

android: replace broadcast intent with service intent #650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 18 additions & 49 deletions android/src/main/java/com/tailscale/ipn/App.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import android.app.Application
import android.app.Notification
import android.app.NotificationChannel
import android.app.PendingIntent
import android.content.BroadcastReceiver
import android.content.Context
import android.content.Intent
import android.content.IntentFilter
Expand Down Expand Up @@ -37,11 +36,6 @@ import com.tailscale.ipn.ui.viewModel.VpnViewModel
import com.tailscale.ipn.ui.viewModel.VpnViewModelFactory
import com.tailscale.ipn.util.FeatureFlags
import com.tailscale.ipn.util.TSLog
import java.io.File
import java.io.IOException
import java.net.NetworkInterface
import java.security.GeneralSecurityException
import java.util.Locale
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.SupervisorJob
Expand All @@ -53,6 +47,11 @@ import kotlinx.coroutines.launch
import kotlinx.serialization.encodeToString
import kotlinx.serialization.json.Json
import libtailscale.Libtailscale
import java.io.File
import java.io.IOException
import java.net.NetworkInterface
import java.security.GeneralSecurityException
import java.util.Locale

class App : UninitializedApp(), libtailscale.AppContext, ViewModelStoreOwner {
val applicationScope = CoroutineScope(SupervisorJob() + Dispatchers.Default)
Expand Down Expand Up @@ -470,25 +469,15 @@ open class UninitializedApp : Application() {
}

fun restartVPN() {
// Register a receiver to listen for the completion of stopVPN
val stopReceiver =
object : BroadcastReceiver() {
override fun onReceive(context: Context?, intent: Intent?) {
// Ensure stop intent is complete
if (intent?.action == IPNService.ACTION_STOP_VPN) {
// Unregister receiver after receiving the broadcast
context?.unregisterReceiver(this)
// Now start the VPN
startVPN()
}
}
}

// Register the receiver before stopping VPN
val intentFilter = IntentFilter(IPNService.ACTION_STOP_VPN)
this.registerReceiver(stopReceiver, intentFilter, Context.RECEIVER_NOT_EXPORTED)

stopVPN()
val intent =
Intent(this, IPNService::class.java).apply { action = IPNService.ACTION_RESTART_VPN }
try {
startService(intent)
} catch (illegalStateException: IllegalStateException) {
TSLog.e(TAG, "restartVPN hit IllegalStateException in startService(): $illegalStateException")
} catch (e: Exception) {
TSLog.e(TAG, "restartVPN hit exception in startService(): $e")
}
Comment on lines +474 to +480
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In android/src/main/java/com/tailscale/ipn/App.kt's restartVPN method, consider adding a more specific catch for SecurityException which can occur when the VPN service start is disallowed by the user. This would help distinguish between security-related failures and other runtime exceptions, allowing for more targeted error handling.

}

fun createNotificationChannel(id: String, name: String, description: String, importance: Int) {
Expand Down Expand Up @@ -569,33 +558,13 @@ open class UninitializedApp : Application() {
return builder.build()
}

fun addUserDisallowedPackageName(packageName: String) {
if (packageName.isEmpty()) {
TSLog.e(TAG, "addUserDisallowedPackageName called with empty packageName")
return
}

getUnencryptedPrefs()
.edit()
.putStringSet(
DISALLOWED_APPS_KEY, disallowedPackageNames().toMutableSet().union(setOf(packageName)))
.apply()

this.restartVPN()
}

fun removeUserDisallowedPackageName(packageName: String) {
if (packageName.isEmpty()) {
TSLog.e(TAG, "removeUserDisallowedPackageName called with empty packageName")
fun updateUserDisallowedPackageNames(packageNames: List<String>) {
if (packageNames.any { it.isEmpty() }) {
TSLog.e(TAG, "updateUserDisallowedPackageNames called with empty packageName(s)")
return
}

getUnencryptedPrefs()
.edit()
.putStringSet(
DISALLOWED_APPS_KEY,
disallowedPackageNames().toMutableSet().subtract(setOf(packageName)))
.apply()
getUnencryptedPrefs().edit().putStringSet(DISALLOWED_APPS_KEY, packageNames.toSet()).apply()

this.restartVPN()
}
Expand Down
11 changes: 9 additions & 2 deletions android/src/main/java/com/tailscale/ipn/IPNService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ import com.tailscale.ipn.mdm.MDMSettings
import com.tailscale.ipn.ui.model.Ipn
import com.tailscale.ipn.ui.notifier.Notifier
import com.tailscale.ipn.util.TSLog
import java.util.UUID
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.launch
import libtailscale.Libtailscale
import java.util.UUID

open class IPNService : VpnService(), libtailscale.IPNService {
private val TAG = "IPNService"
Expand Down Expand Up @@ -46,6 +46,13 @@ open class IPNService : VpnService(), libtailscale.IPNService {
close()
START_NOT_STICKY
}
ACTION_RESTART_VPN -> {
app.setWantRunning(false){
close()
app.startVPN()
}
START_NOT_STICKY
}
Comment on lines +49 to +55
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In android/src/main/java/com/tailscale/ipn/IPNService.kt, the ACTION_RESTART_VPN handling first calls app.setWantRunning(false) followed by close() which no longer contains app.setWantRunning(false). Should we be concerned about this state management change or potential duplicate calls?

ACTION_START_VPN -> {
scope.launch { showForegroundNotification() }
app.setWantRunning(true)
Expand Down Expand Up @@ -82,7 +89,6 @@ open class IPNService : VpnService(), libtailscale.IPNService {
}

override fun close() {
app.setWantRunning(false) {}
Notifier.setState(Ipn.State.Stopping)
disconnectVPN()
Libtailscale.serviceDisconnect(this)
Comment on lines 91 to 94
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IPNService.kt close() method no longer calls app.setWantRunning(false), while the ACTION_RESTART_VPN handler does this explicitly. Consider abstracting this state management logic to prevent potential inconsistencies in future modifications.

Expand Down Expand Up @@ -180,5 +186,6 @@ open class IPNService : VpnService(), libtailscale.IPNService {
companion object {
const val ACTION_START_VPN = "com.tailscale.ipn.START_VPN"
const val ACTION_STOP_VPN = "com.tailscale.ipn.STOP_VPN"
const val ACTION_RESTART_VPN = "com.tailscale.ipn.RESTART_VPN"
}
}
23 changes: 18 additions & 5 deletions android/src/main/java/com/tailscale/ipn/ui/viewModel/SplitTunnelAppPickerViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,18 @@
package com.tailscale.ipn.ui.viewModel

import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.tailscale.ipn.App
import com.tailscale.ipn.mdm.MDMSettings
import com.tailscale.ipn.mdm.SettingState
import com.tailscale.ipn.ui.util.InstalledApp
import com.tailscale.ipn.ui.util.InstalledAppsManager
import com.tailscale.ipn.ui.util.set
import kotlinx.coroutines.Job
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.launch

class SplitTunnelAppPickerViewModel : ViewModel() {
val installedAppsManager = InstalledAppsManager(packageManager = App.get().packageManager)
Expand All @@ -20,6 +24,8 @@ class SplitTunnelAppPickerViewModel : ViewModel() {
val mdmExcludedPackages: StateFlow<SettingState<String?>> = MDMSettings.excludedPackages.flow
val mdmIncludedPackages: StateFlow<SettingState<String?>> = MDMSettings.includedPackages.flow

private var saveJob: Job? = null

init {
installedApps.set(installedAppsManager.fetchInstalledApps())
excludedPackageNames.set(
Expand All @@ -30,15 +36,22 @@ class SplitTunnelAppPickerViewModel : ViewModel() {
}

fun exclude(packageName: String) {
if (excludedPackageNames.value.contains(packageName)) {
return
}
if (excludedPackageNames.value.contains(packageName)) return
excludedPackageNames.set(excludedPackageNames.value + packageName)
App.get().addUserDisallowedPackageName(packageName)
debounceSave()
}

fun unexclude(packageName: String) {
excludedPackageNames.set(excludedPackageNames.value - packageName)
App.get().removeUserDisallowedPackageName(packageName)
debounceSave()
}

private fun debounceSave() {
saveJob?.cancel()
saveJob =
viewModelScope.launch {
delay(500) // Wait to batch multiple rapid updates
App.get().updateUserDisallowedPackageNames(excludedPackageNames.value)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In android/src/main/java/com/tailscale/ipn/ui/viewModel/SplitTunnelAppPickerViewModel.kt, the pendingChanges set is added but only used for tracking add/remove operations. Since the actual updates are based on excludedPackageNames.value, can you clarify the purpose of this set? It appears unused in the final update operation.

}
}