diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml
index 3d3ce25040..2a6f7bb80b 100644
--- a/app/src/main/AndroidManifest.xml
+++ b/app/src/main/AndroidManifest.xml
@@ -66,7 +66,7 @@
-
+
diff --git a/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt b/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt
index 9952eb3359..4aae429d06 100644
--- a/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt
+++ b/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 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())
diff --git a/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActivity.kt b/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActivity.kt
index 771bdfe9dc..9194752d0d 100644
--- a/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActivity.kt
+++ b/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,14 +121,14 @@ 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
@@ -116,12 +136,12 @@ class ScreenLockActivity : BaseActionBarActivity() {
// 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("next_intent")
- if (nextIntent == null) {
- Log.w(TAG, "Got a null nextIntent - cannot proceed.")
- } else {
- startActivity(nextIntent)
+ val nextIntent = intent.getParcelableExtra(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()
diff --git a/app/src/main/java/org/thoughtcrime/securesms/service/KeyCachingService.java b/app/src/main/java/org/thoughtcrime/securesms/service/KeyCachingService.java
index 434bfa1b8b..0fd27f29ae 100644
--- a/app/src/main/java/org/thoughtcrime/securesms/service/KeyCachingService.java
+++ b/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);