App lock: cover the app instead of destroying it, and let debug builds lock at all

app/src/main/AndroidManifest.xml

@@ -66,7 +66,7 @@
     <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" /> <!-- Only used on Android API 29 and lower -->
     <uses-permission android:name="android.permission.WRITE_SYNC_SETTINGS" />
     <uses-permission android:name="com.google.android.c2dm.permission.RECEIVE" />
-    <uses-permission android:name="network.loki.messenger.ACCESS_SESSION_SECRETS" />
+    <uses-permission android:name="network.loki.messenger.ACCESS_SESSION_SECRETS${authority_postfix}" />
 
     <queries>
         <intent>

app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt

@@ -46,6 +46,8 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() {
         private const val STATE_WELCOME_SCREEN    = 3
         private const val STATE_DATABASE_MIGRATE  = 4 // This is different from STATE_UPGRADE_DATABASE as it is used to migrate database in a whole rather than the internal db schema upgrades
 
+        private const val KEY_HAS_STARTED = "has_started"
+
         private fun getStateName(state: Int): String {
             return when (state) {
                 STATE_NORMAL           -> "STATE_NORMAL"
@@ -80,6 +82,8 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() {
     }
 
     private var clearKeyReceiver: BroadcastReceiver? = null
+    private var hasStarted = false
+    private var wasRecreated = false
 
     @Inject
     lateinit var loginStateRepository: LoginStateRepository
@@ -95,9 +99,12 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() {
 
         super.onCreate(savedInstanceState)
 
-        val locked = KeyCachingService.isLocked(this) && isScreenLockEnabled(this) &&
-                loginStateRepository.peekLoginState() != null
-        routeApplicationState(locked)
+        // Captured once, before routing: routeApplicationState is a coroutine, so reading hasStarted
+        // from inside it would race with onStart setting it and misread a cold start as a recreation.
+        wasRecreated = (savedInstanceState != null)
+        hasStarted = savedInstanceState?.getBoolean(KEY_HAS_STARTED) == true
+
+        routeApplicationState(isScreenLocked())
 
         if (!isFinishing) {
             initializeClearKeyReceiver()
@@ -107,6 +114,45 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() {
 
     protected open fun onCreate(savedInstanceState: Bundle?, ready: Boolean) {}
 
+    override fun onSaveInstanceState(outState: Bundle) {
+        super.onSaveInstanceState(outState)
+        outState.putBoolean(KEY_HAS_STARTED, hasStarted)
+    }
+
+    override fun onStart() {
+        super.onStart()
+
+        // Only re-locks an activity that is coming BACK to the foreground; a first start while locked
+        // is already handled by onCreate's routing. The distinction matters because the lock can be
+        // cleared while this activity is merely stopped, which happens on any excursion to another app
+        // - a file picker, the camera, the share sheet. Covering it here rather than destroying it is
+        // what lets such an excursion return to the state it left, including a pending
+        // onActivityResult.
+        val isReturningToForeground = hasStarted
+        hasStarted = true
+
+        if (isReturningToForeground && isScreenLocked()) presentScreenLock()
+    }
+
+    private fun isScreenLocked(): Boolean =
+        KeyCachingService.isLocked(this) && isScreenLockEnabled(this) &&
+                loginStateRepository.peekLoginState() != null
+
+    // Puts the lock screen in front of this activity WITHOUT finishing it, so the task and any pending
+    // activity result survive. No "next_intent" is supplied because there is nothing to route back to
+    // - this activity is still here, and ScreenLockActivity finishes itself on success to reveal it.
+    //
+    // ScreenLockActivity is singleInstancePerTask, so it occupies its own task ABOVE this one rather
+    // than stacking in it: concurrent calls from several live activities reuse the one instance, and
+    // dismissing it without authenticating reveals this activity again - which is why it has to send
+    // the user to the launcher instead (see ScreenLockActivity.leaveAppLocked).
+    private fun presentScreenLock() {
+        startActivity(
+            Intent(this, ScreenLockActivity::class.java)
+                .putExtra(ScreenLockActivity.EXTRA_PRESENTED_OVER, true)
+        )
+    }
+
     override fun onPause() {
         Log.i(TAG, "onPause()")
         super.onPause()
@@ -120,8 +166,15 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() {
 
     fun onMasterSecretCleared() {
         Log.i(TAG, "onMasterSecretCleared()")
-        if (appVisibilityManager.isAppVisible.value) routeApplicationState(true)
-        else finish()
+
+        // KeyCachingService.onDestroy broadcasts this whether or not app lock is enabled, so the same
+        // isScreenLocked() check onStart uses is needed here too - without it the broadcast can raise a
+        // lock screen that never prompts for anything, because ScreenLockActivity only starts an
+        // authentication when the setting is on.
+        //
+        // Nothing to cover while backgrounded either, and finishing would discard whatever the user was
+        // in the middle of - onStart presents the lock when they come back instead.
+        if (appVisibilityManager.isAppVisible.value && isScreenLocked()) presentScreenLock()
     }
 
     protected fun <T : Fragment?> initFragment(@IdRes target: Int, fragment: T): T? {
@@ -165,7 +218,10 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() {
         Log.i(TAG, "routeApplicationState() - ${getStateName(state)}")
 
         return when (state) {
-            STATE_SCREEN_LOCKED    -> getScreenUnlockIntent() // Note: This is a suspend function
+            // Routing finishes this activity, which on a recreation would discard a pending activity
+            // result - a picker excursion survives a rotation only if the activity waiting on it does.
+            // A recreated instance is covered by onStart instead, via the restored hasStarted.
+            STATE_SCREEN_LOCKED    -> if (wasRecreated) null else getScreenUnlockIntent() // Note: This is a suspend function
             STATE_UPGRADE_DATABASE -> getUpgradeDatabaseIntent()
             STATE_WELCOME_SCREEN   -> getWelcomeIntent()
             STATE_DATABASE_MIGRATE -> getRoutedIntent(DatabaseMigrationStateActivity::class.java, getConversationListIntent())

app/src/main/java/org/thoughtcrime/securesms/ScreenLockActivity.kt

@@ -30,6 +30,7 @@ import android.view.animation.TranslateAnimation
 import android.widget.ImageView
 import android.widget.TextView
 import android.widget.Toast
+import androidx.activity.OnBackPressedCallback
 import androidx.biometric.BiometricPrompt
 import androidx.biometric.BiometricManager
 import androidx.core.content.ContextCompat
@@ -47,6 +48,17 @@ import org.thoughtcrime.securesms.service.KeyCachingService
 import org.thoughtcrime.securesms.service.KeyCachingService.KeySetBinder
 
 class ScreenLockActivity : BaseActionBarActivity() {
+    companion object {
+        /**
+         * Marks a lock that was presented over a still-live activity, and which therefore carries no
+         * [EXTRA_NEXT_INTENT]. Without it there is no way to tell that case apart from a routed unlock
+         * whose destination has gone missing, and the second one has to stay reportable.
+         */
+        const val EXTRA_PRESENTED_OVER: String = "presented_over"
+
+        private const val EXTRA_NEXT_INTENT: String = "next_intent"
+    }
+
     private val TAG: String = ScreenLockActivity::class.java.simpleName
 
     private lateinit var fingerprintPrompt: ImageView
@@ -74,6 +86,14 @@ class ScreenLockActivity : BaseActionBarActivity() {
         setContentView(R.layout.screen_lock_activity)
         initializeResources()
 
+        // Back must leave the app, not just dismiss the lock: the activity underneath is still alive and
+        // would re-present it immediately.
+        onBackPressedDispatcher.addCallback(this, object : OnBackPressedCallback(true) {
+            override fun handleOnBackPressed() {
+                leaveAppLocked()
+            }
+        })
+
         // Start and bind to the KeyCachingService instance.
         val bindIntent = Intent(this, KeyCachingService::class.java)
         startService(bindIntent)
@@ -101,27 +121,27 @@ class ScreenLockActivity : BaseActionBarActivity() {
                     BiometricPrompt.ERROR_NEGATIVE_BUTTON,
                     BiometricPrompt.ERROR_USER_CANCELED -> {
                         onAuthenticationFailed()
-                        finish()
+                        leaveAppLocked()
                     }
 
                     // User made 5 incorrect biometric login attempts so they get a timeout
                     // Note: The SYSTEM provides the localised error "Too many attempts. Try again later.".
                     BiometricPrompt.ERROR_LOCKOUT -> {
                         Toast.makeText(context, errString, Toast.LENGTH_SHORT).show()
-                        finish()
+                        leaveAppLocked()
                     }
 
                     // User made a large number of incorrect biometric login attempts and Android disabled
                     // the fingerprint sensor until they lock the device then log back in via non-biometric means.
                     // Note: The SYSTEM provides the localised error "Too many attempts. Fingerprint sensor disabled."
                     BiometricPrompt.ERROR_LOCKOUT_PERMANENT -> {
                         Toast.makeText(context, errString, Toast.LENGTH_SHORT).show()
-                        finish()
+                        leaveAppLocked()
                     }
 
                     else -> {
                         Log.w(TAG, "Unhandled authentication error: $errorCode $errString")
-                        finish()
+                        leaveAppLocked()
                     }
                 }
             }
@@ -219,7 +239,11 @@ class ScreenLockActivity : BaseActionBarActivity() {
 
     override fun onNewIntent(intent: Intent) {
         super.onNewIntent(intent)
-        setIntent(intent)
+
+        // A lock presented over a live activity carries no "next_intent". Adopting such an intent would
+        // discard the destination a routed unlock still has to reach - the activity that routed here has
+        // already finished itself, so nothing else remembers where to go.
+        if (intent.hasExtra(EXTRA_NEXT_INTENT)) setIntent(intent)
     }
 
     public override fun onActivityResult(requestCode: Int, resultcode: Int, data: Intent?) {
@@ -276,17 +300,40 @@ class ScreenLockActivity : BaseActionBarActivity() {
             ?.start()
     }
 
+    // Declining the unlock has to leave the app, not merely finish this activity. The activity being
+    // locked is still alive (ScreenLockActionBarActivity.presentScreenLock) and finishing alone reveals
+    // it, whereupon its onStart presents the lock straight back - a loop with no way out, which during a
+    // biometric lockout spins with no user input at all and flashes the covered content each time.
+    //
+    // Backgrounding this task is not enough either: launchMode is singleInstancePerTask, so this lives
+    // in its OWN task and the app's task sits beneath it. Going to the launcher is what backgrounds
+    // both. Returning to the app afterwards presents the lock again, which is correct.
+    private fun leaveAppLocked() {
+        startActivity(
+            Intent(Intent.ACTION_MAIN)
+                .addCategory(Intent.CATEGORY_HOME)
+                .setFlags(Intent.FLAG_ACTIVITY_NEW_TASK)
+        )
+        finish()
+    }
+
     private fun handleAuthenticated() {
         authenticated = true
         keyCachingService?.setMasterSecret(Any())
 
         // The 'nextIntent' will take us to the MainActivity if this is a standard unlock, or it will
         // take us to the ShareActivity if this is an external share.
-        val nextIntent = intent.getParcelableExtra<Intent?>("next_intent")
-        if (nextIntent == null) {
-            Log.w(TAG, "Got a null nextIntent - cannot proceed.")
-        } else {
-            startActivity(nextIntent)
+        val nextIntent = intent.getParcelableExtra<Intent?>(EXTRA_NEXT_INTENT)
+
+        when {
+            nextIntent != null -> startActivity(nextIntent)
+
+            // Presented over a still-live activity, so there is nothing to route to and finishing
+            // reveals it again (see ScreenLockActionBarActivity.presentScreenLock).
+            intent.getBooleanExtra(EXTRA_PRESENTED_OVER, false) -> {}
+
+            // Routed here, but the destination is gone: unlocking will land on nothing.
+            else -> Log.w(TAG, "Got a null nextIntent - cannot proceed.")
         }
 
         finish()

app/src/main/java/org/thoughtcrime/securesms/service/KeyCachingService.java

@@ -220,10 +220,29 @@ private static void startTimeoutIfAppropriate(@NonNull Context context) {
       if (!TextSecurePreferences.isPasswordDisabled(context)) timeoutMillis = TimeUnit.MINUTES.toMillis(passphraseTimeoutMinutes);
       else                                                    timeoutMillis = TimeUnit.SECONDS.toMillis(screenLockTimeoutSeconds);
 
+      PendingIntent expirationIntent = buildExpirationPendingIntent(context);
+
+      // With no timeout there is nothing to schedule, and scheduling anyway is what left the lock
+      // unreliable: AlarmManager batches non-wakeup alarms, so an alarm for "now" was still arriving
+      // seconds later, and onAppForegrounded cancels it on the way back in. A quick trip to another app
+      // and straight back therefore never locked at all. Deliver it directly instead - the service is
+      // necessarily foregrounded here, because we only get this far with a secret set.
+      if (timeoutMillis <= 0) {
+        Log.i(TAG, "No timeout, expiring immediately");
+
+        try {
+          expirationIntent.send();
+        } catch (PendingIntent.CanceledException e) {
+          Log.w(TAG, "Immediate expiry was cancelled, falling back to an alarm", e);
+          ServiceUtil.getAlarmManager(context).set(AlarmManager.ELAPSED_REALTIME, SystemClock.elapsedRealtime(), expirationIntent);
+        }
+
+        return;
+      }
+
       Log.i(TAG, "Starting timeout: " + timeoutMillis);
 
-      AlarmManager  alarmManager     = ServiceUtil.getAlarmManager(context);
-      PendingIntent expirationIntent = buildExpirationPendingIntent(context);
+      AlarmManager alarmManager = ServiceUtil.getAlarmManager(context);
 
       alarmManager.cancel(expirationIntent);
       alarmManager.set(AlarmManager.ELAPSED_REALTIME, SystemClock.elapsedRealtime() + timeoutMillis, expirationIntent);