From dd2de0440ebf241cc9e584779e7ce424baac58d3 Mon Sep 17 00:00:00 2001 From: Jamil Date: Thu, 4 Jul 2024 22:06:02 -0700 Subject: [PATCH] refactor(android): Remove excessive debug logging from kotlin codebase (#5748) - Why: This prevents logging potentially sensitive data to Logcat or Firebase. For critical codepaths we rely on a non-null `!!` check anyhow, which will be reported with a crash to Firebase. Now that we have some confidence the app is reliable, I think we can reflect that confidence in our code. - This moves the `loadLibrary` call to the app start, which will surface issues immediately when launching the app and not when trying to connect. This also makes connect very slightly faster. - Finally, `BootShutdownReceiver` is removed since it was essentially a no-op. There are a few ways we can connect on boot, but this isn't a good approach since it would ignore Android's Always-on VPN setting. --- .../android/app/src/main/AndroidManifest.xml | 9 ----- .../dev/firezone/android/core/FirezoneApp.kt | 4 ++ .../notifications/CustomUriNotification.kt | 3 -- .../customuri/ui/CustomUriViewModel.kt | 8 +--- .../session/backend/BootShutdownReceiver.kt | 24 ------------ .../features/session/ui/SessionActivity.kt | 5 --- .../firezone/android/tunnel/NetworkMonitor.kt | 3 -- .../firezone/android/tunnel/TunnelService.kt | 37 ------------------- .../tunnel/TunnelStatusNotification.kt | 3 -- 9 files changed, 5 insertions(+), 91 deletions(-) delete mode 100644 kotlin/android/app/src/main/java/dev/firezone/android/features/session/backend/BootShutdownReceiver.kt diff --git a/kotlin/android/app/src/main/AndroidManifest.xml b/kotlin/android/app/src/main/AndroidManifest.xml index 62db0cb43..adb1fbcef 100644 --- a/kotlin/android/app/src/main/AndroidManifest.xml +++ b/kotlin/android/app/src/main/AndroidManifest.xml @@ -79,15 +79,6 @@ - - - - - - - { intent.data?.getQueryParameter(QUERY_ACTOR_NAME)?.let { actorName -> - Log.d(TAG, "Found actor name: $actorName") repo.saveActorName(actorName).collect() } intent.data?.getQueryParameter(QUERY_CLIENT_STATE)?.let { state -> - if (repo.validateState(state).firstOrNull() == true) { - Log.d(TAG, "Valid state parameter. Continuing to save state...") - } else { + if (repo.validateState(state).firstOrNull() != true) { error("Invalid state parameter $state") } } intent.data?.getQueryParameter(QUERY_CLIENT_AUTH_FRAGMENT)?.let { fragment -> if (fragment.isNotBlank()) { - Log.d(TAG, "Found valid auth fragment in response") - // Save token, then clear nonce and state since we don't // need to keep them around anymore repo.saveToken(fragment).collect() @@ -66,7 +61,6 @@ internal class CustomUriViewModel if (accumulatedErrors.isNotEmpty()) { actionMutableLiveData.postValue(ViewAction.AuthFlowError(accumulatedErrors)) } else { - Log.d(TAG, "Auth flow complete") actionMutableLiveData.postValue(ViewAction.AuthFlowComplete) } } diff --git a/kotlin/android/app/src/main/java/dev/firezone/android/features/session/backend/BootShutdownReceiver.kt b/kotlin/android/app/src/main/java/dev/firezone/android/features/session/backend/BootShutdownReceiver.kt deleted file mode 100644 index 35cba036d..000000000 --- a/kotlin/android/app/src/main/java/dev/firezone/android/features/session/backend/BootShutdownReceiver.kt +++ /dev/null @@ -1,24 +0,0 @@ -/* Licensed under Apache 2.0 (C) 2024 Firezone, Inc. */ -package dev.firezone.android.features.session.backend - -import android.content.BroadcastReceiver -import android.content.Context -import android.content.Intent -import android.util.Log - -internal class BootShutdownReceiver : BroadcastReceiver() { - override fun onReceive( - context: Context, - intent: Intent, - ) { - if (Intent.ACTION_BOOT_COMPLETED == intent.action) { - Log.d("BootShutdownReceiver", "Boot completed. Attempting to connect.") - // TODO: Retrieve the session manager from the application context. - // sessionManager.connect() - } else if (Intent.ACTION_SHUTDOWN == intent.action) { - Log.d("BootShutdownReceiver", "Shutting down. Attempting to disconnect.") - // TODO: Retrieve the session manager from the application context. - // sessionManager.disconnect() - } - } -} diff --git a/kotlin/android/app/src/main/java/dev/firezone/android/features/session/ui/SessionActivity.kt b/kotlin/android/app/src/main/java/dev/firezone/android/features/session/ui/SessionActivity.kt index 86eef3e24..000f60a72 100644 --- a/kotlin/android/app/src/main/java/dev/firezone/android/features/session/ui/SessionActivity.kt +++ b/kotlin/android/app/src/main/java/dev/firezone/android/features/session/ui/SessionActivity.kt @@ -7,7 +7,6 @@ import android.content.Intent import android.content.ServiceConnection import android.os.Bundle import android.os.IBinder -import android.util.Log import androidx.activity.viewModels import androidx.appcompat.app.AppCompatActivity import androidx.recyclerview.widget.DividerItemDecoration @@ -30,7 +29,6 @@ internal class SessionActivity : AppCompatActivity() { name: ComponentName?, service: IBinder?, ) { - Log.d(TAG, "onServiceConnected") val binder = service as TunnelService.LocalBinder tunnelService = binder.getService() serviceBound = true @@ -39,7 +37,6 @@ internal class SessionActivity : AppCompatActivity() { } override fun onServiceDisconnected(name: ComponentName?) { - Log.d(TAG, "onServiceDisconnected") serviceBound = false } } @@ -69,14 +66,12 @@ internal class SessionActivity : AppCompatActivity() { private fun setupViews() { binding.btSignOut.setOnClickListener { - Log.d(TAG, "Sign out button clicked") viewModel.clearToken() viewModel.clearActorName() tunnelService?.disconnect() } binding.btSettings.setOnClickListener { - Log.d(TAG, "Settings button clicked") val intent = Intent(this, SettingsActivity::class.java) intent.putExtra("isUserSignedIn", true) startActivity(intent) diff --git a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/NetworkMonitor.kt b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/NetworkMonitor.kt index f5094fecf..6be7e20e6 100644 --- a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/NetworkMonitor.kt +++ b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/NetworkMonitor.kt @@ -2,7 +2,6 @@ import android.net.ConnectivityManager import android.net.LinkProperties import android.net.Network -import android.util.Log import com.google.gson.Gson import dev.firezone.android.tunnel.ConnlibSession import dev.firezone.android.tunnel.TunnelService @@ -17,8 +16,6 @@ class NetworkMonitor(private val tunnelService: TunnelService) : ConnectivityMan network: Network, linkProperties: LinkProperties, ) { - Log.d("NetworkMonitor", "OnLinkPropertiesChanged: $network: $linkProperties") - // Acquire mutex lock if (tunnelService.lock.tryLock()) { if (tunnelService.tunnelState != TunnelService.Companion.State.UP) { 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 e2b201d19..ec24f32b4 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 @@ -15,7 +15,6 @@ import android.os.Binder import android.os.Build import android.os.Bundle import android.os.IBinder -import android.util.Log import androidx.lifecycle.MutableLiveData import com.google.firebase.crashlytics.ktx.crashlytics import com.google.firebase.ktx.Firebase @@ -90,8 +89,6 @@ class TunnelService : VpnService() { private val callback: ConnlibCallback = object : ConnlibCallback { override fun onUpdateResources(resourceListJSON: String) { - Log.d(TAG, "onUpdateResources: $resourceListJSON") - Firebase.crashlytics.log("onUpdateResources: $resourceListJSON") moshi.adapter>().fromJson(resourceListJSON)?.let { tunnelResources = it } @@ -102,9 +99,6 @@ class TunnelService : VpnService() { addressIPv6: String, dnsAddresses: String, ): Int { - Log.d(TAG, "onSetInterfaceConfig: $addressIPv4, $addressIPv6, $dnsAddresses") - Firebase.crashlytics.log("onSetInterfaceConfig: $addressIPv4, $addressIPv6, $dnsAddresses") - // init tunnel config tunnelDnsAddresses = moshi.adapter>().fromJson(dnsAddresses)!! tunnelIpv4Address = addressIPv4 @@ -118,8 +112,6 @@ class TunnelService : VpnService() { routes4JSON: String, routes6JSON: String, ): Int { - Log.d(TAG, "onUpdateRoutes: $routes4JSON, $routes6JSON") - Firebase.crashlytics.log("onUpdateRoutes: $routes4JSON, $routes6JSON") val routes4 = moshi.adapter>().fromJson(routes4JSON)!! val routes6 = moshi.adapter>().fromJson(routes6JSON)!! @@ -133,9 +125,6 @@ class TunnelService : VpnService() { // Unexpected disconnect, most likely a 401. Clear the token and initiate a stop of the // service. override fun onDisconnect(error: String): Boolean { - Log.d(TAG, "onDisconnect: $error") - Firebase.crashlytics.log("onDisconnect: $error") - stopNetworkMonitoring() // Clear any user tokens and actorNames @@ -162,7 +151,6 @@ class TunnelService : VpnService() { context: Context, intent: Intent, ) { - Log.d(TAG, "onReceive") val restrictionsManager = context.getSystemService(Context.RESTRICTIONS_SERVICE) as android.content.RestrictionsManager val newAppRestrictions = restrictionsManager.applicationRestrictions val changed = MANAGED_CONFIGURATIONS.any { newAppRestrictions.getString(it) != appRestrictions.getString(it) } @@ -186,7 +174,6 @@ class TunnelService : VpnService() { flags: Int, startId: Int, ): Int { - Log.d(TAG, "onStartCommand") if (intent?.getBooleanExtra("startedByUser", false) == true) { startedByUser = true } @@ -196,26 +183,21 @@ class TunnelService : VpnService() { override fun onCreate() { super.onCreate() - Log.d(TAG, "onCreate") registerReceiver(restrictionsReceiver, restrictionsFilter) } override fun onDestroy() { - Log.d(TAG, "onDestroy") unregisterReceiver(restrictionsReceiver) super.onDestroy() } override fun onRevoke() { - Log.d(TAG, "onRevoke") disconnect() super.onRevoke() } // Call this to stop the tunnel and shutdown the service, leaving the token intact. fun disconnect() { - Log.d(TAG, "disconnect") - // Acquire mutex lock lock.lock() @@ -232,8 +214,6 @@ class TunnelService : VpnService() { } private fun shutdown() { - Log.d(TAG, "shutdown") - connlibSessionPtr = null stopSelf() tunnelState = State.DOWN @@ -246,7 +226,6 @@ class TunnelService : VpnService() { if (!token.isNullOrBlank()) { tunnelState = State.CONNECTING updateStatusNotification(TunnelStatusNotification.Connecting) - System.loadLibrary("connlib") connlibSessionPtr = ConnlibSession.connect( @@ -315,8 +294,6 @@ class TunnelService : VpnService() { repo.saveDeviceIdSync(newDeviceId) newDeviceId } - Log.d(TAG, "Device ID: $deviceId") - return deviceId } @@ -329,36 +306,24 @@ class TunnelService : VpnService() { private fun buildVpnService(): Int { Builder().apply { - Firebase.crashlytics.log("Building VPN service") // Allow traffic to bypass the VPN interface when Always-on VPN is enabled. allowBypass() if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { - Firebase.crashlytics.log("Setting transport info") setMetered(false) // Inherit the metered status from the underlying networks. } - Firebase.crashlytics.log("Setting underlying networks") setUnderlyingNetworks(null) // Use all available networks. - Log.d(TAG, "Routes: $tunnelRoutes") - Firebase.crashlytics.log("Routes: $tunnelRoutes") tunnelRoutes.forEach { addRoute(it.address, it.prefix) } - Log.d(TAG, "DNS Servers: $tunnelDnsAddresses") - Firebase.crashlytics.log("DNS Servers: $tunnelDnsAddresses") tunnelDnsAddresses.forEach { dns -> addDnsServer(dns) } - Log.d(TAG, "IPv4 Address: $tunnelIpv4Address") - Firebase.crashlytics.log("IPv4 Address: $tunnelIpv4Address") addAddress(tunnelIpv4Address!!, 32) - - Log.d(TAG, "IPv6 Address: $tunnelIpv6Address") - Firebase.crashlytics.log("IPv6 Address: $tunnelIpv6Address") addAddress(tunnelIpv6Address!!, 128) updateAllowedDisallowedApplications("allowedApplications", ::addAllowedApplication) @@ -379,7 +344,6 @@ class TunnelService : VpnService() { allowOrDisallow: (String) -> Unit, ) { val applications = appRestrictions.getString(key) - Log.d(TAG, "$key: $applications") Firebase.crashlytics.log("$key: $applications") applications?.let { if (it.isNotBlank()) { @@ -433,7 +397,6 @@ class TunnelService : VpnService() { } fun start(context: Context) { - Log.d(TAG, "Starting TunnelService") val intent = Intent(context, TunnelService::class.java) intent.putExtra("startedByUser", true) context.startService(intent) diff --git a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelStatusNotification.kt b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelStatusNotification.kt index d41571bd7..6292f8081 100644 --- a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelStatusNotification.kt +++ b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelStatusNotification.kt @@ -8,7 +8,6 @@ import android.app.PendingIntent import android.content.Context import android.content.Context.NOTIFICATION_SERVICE import android.content.Intent -import android.util.Log import androidx.core.app.NotificationCompat import dev.firezone.android.R import dev.firezone.android.core.presentation.MainActivity @@ -25,7 +24,6 @@ object TunnelStatusNotification { context: Context, status: StatusType, ): NotificationCompat.Builder { - Log.d(TAG, "update") val manager = context.getSystemService(NOTIFICATION_SERVICE) as NotificationManager val chan = @@ -44,7 +42,6 @@ object TunnelStatusNotification { } private fun configIntent(context: Context): PendingIntent { - Log.d(TAG, "configIntent") val intent = Intent(context, MainActivity::class.java) intent.flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK return PendingIntent.getActivity(