From 6893a0abc049f5b10f712046e998af4b41ff8d2c Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Mon, 6 Jul 2026 15:30:20 -0400 Subject: [PATCH] Fix lifecycle of regV5 scratch storage. --- .../v2/AppRegistrationStorageController.kt | 12 ++----- .../dependencies/DemoStorageController.kt | 3 +- .../registration/RegistrationRepository.kt | 16 +++++++++ .../registration/RegistrationViewModel.kt | 4 +-- .../VerificationCodeScreen.kt | 6 ++++ .../VerificationCodeScreenEvents.kt | 6 ++++ .../VerificationCodeViewModel.kt | 24 +++++++++++++ .../src/main/protowire/Registration.proto | 6 +++- .../registration/RegistrationViewModelTest.kt | 8 ++--- .../VerificationCodeViewModelTest.kt | 35 +++++++++++++++++++ 10 files changed, 102 insertions(+), 18 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/registration/v2/AppRegistrationStorageController.kt b/app/src/main/java/org/thoughtcrime/securesms/registration/v2/AppRegistrationStorageController.kt index 7d923de9c0..08bdd57262 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/registration/v2/AppRegistrationStorageController.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/registration/v2/AppRegistrationStorageController.kt @@ -67,7 +67,6 @@ import org.whispersystems.signalservice.api.link.TransferArchiveResponse import java.io.File import java.io.IOException import java.time.LocalDateTime -import kotlin.time.Duration.Companion.minutes /** * Implementation of [StorageController] that bridges to the app's existing storage infrastructure. @@ -77,7 +76,6 @@ class AppRegistrationStorageController(private val context: Context) : StorageCo companion object { private val TAG = Log.tag(AppRegistrationStorageController::class) private const val TEMP_PROTO_FILENAME = "registration-in-progress.proto" - private val TEMP_PROTO_TIMEOUT = 15.minutes private val MODERN_BACKUP_PATTERN = Regex("^signal-backup-(\\d{4})-(\\d{2})-(\\d{2})-(\\d{2})-(\\d{2})-(\\d{2})$") private val LEGACY_BACKUP_PATTERN = Regex("^signal-(\\d{4})-(\\d{2})-(\\d{2})-(\\d{2})-(\\d{2})-(\\d{2})\\.backup$") } @@ -155,13 +153,6 @@ class AppRegistrationStorageController(private val context: Context) : StorageCo override suspend fun readInProgressRegistrationData(): RegistrationData = withContext(Dispatchers.IO) { val file = File(context.cacheDir, TEMP_PROTO_FILENAME) if (file.exists()) { - val age = System.currentTimeMillis() - file.lastModified() - if (age > TEMP_PROTO_TIMEOUT.inWholeMilliseconds) { - Log.w(TAG, "In-progress registration data is stale (${age}ms old), discarding.") - file.delete() - return@withContext RegistrationData() - } - try { RegistrationData.ADAPTER.decode(file.readBytes()) } catch (e: Exception) { @@ -537,7 +528,8 @@ class AppRegistrationStorageController(private val context: Context) : StorageCo } private suspend fun writeRegistrationData(data: RegistrationData) = withContext(Dispatchers.IO) { + val stamped = data.newBuilder().lastUpdatedMillis(System.currentTimeMillis()).build() val file = File(context.cacheDir, TEMP_PROTO_FILENAME) - file.writeBytes(RegistrationData.ADAPTER.encode(data)) + file.writeBytes(RegistrationData.ADAPTER.encode(stamped)) } } diff --git a/demo/registration/src/main/java/org/signal/registration/sample/dependencies/DemoStorageController.kt b/demo/registration/src/main/java/org/signal/registration/sample/dependencies/DemoStorageController.kt index 65b56bab6b..22af077aa3 100644 --- a/demo/registration/src/main/java/org/signal/registration/sample/dependencies/DemoStorageController.kt +++ b/demo/registration/src/main/java/org/signal/registration/sample/dependencies/DemoStorageController.kt @@ -440,7 +440,8 @@ class DemoStorageController(private val context: Context) : StorageController { } private suspend fun writeRegistrationData(data: RegistrationData) = withContext(Dispatchers.IO) { + val stamped = data.newBuilder().lastUpdatedMillis(System.currentTimeMillis()).build() val file = File(context.filesDir, TEMP_PROTO_FILENAME) - file.writeBytes(RegistrationData.ADAPTER.encode(data)) + file.writeBytes(RegistrationData.ADAPTER.encode(stamped)) } } diff --git a/feature/registration/src/main/java/org/signal/registration/RegistrationRepository.kt b/feature/registration/src/main/java/org/signal/registration/RegistrationRepository.kt index f28072412d..d3edcb70e2 100644 --- a/feature/registration/src/main/java/org/signal/registration/RegistrationRepository.kt +++ b/feature/registration/src/main/java/org/signal/registration/RegistrationRepository.kt @@ -798,6 +798,22 @@ class RegistrationRepository(val context: Context, val networkController: Networ storageController.clearLocalDataAndRestart() } + /** + * Deletes all in-progress registration data. Called once registration is fully complete, so the scratch data is + * never reused by a later flow. + */ + suspend fun clearInProgressRegistrationData() = withContext(Dispatchers.IO) { + storageController.clearAllData() + } + + /** + * The time the in-progress registration data was last written, as epoch milliseconds, or null if nothing has been + * written yet. Read from the in-progress data's `lastUpdatedMillis`, which is stamped on every write. + */ + suspend fun getInProgressRegistrationDataLastUpdated(): Long? = withContext(Dispatchers.IO) { + storageController.readInProgressRegistrationData().lastUpdatedMillis.takeIf { it > 0 } + } + /** * Clears any persisted flow state JSON from the in-progress registration data. */ diff --git a/feature/registration/src/main/java/org/signal/registration/RegistrationViewModel.kt b/feature/registration/src/main/java/org/signal/registration/RegistrationViewModel.kt index 834405a090..2de673203f 100644 --- a/feature/registration/src/main/java/org/signal/registration/RegistrationViewModel.kt +++ b/feature/registration/src/main/java/org/signal/registration/RegistrationViewModel.kt @@ -181,7 +181,7 @@ class RegistrationViewModel(private val repository: RegistrationRepository, save is RegistrationFlowEvent.ResetState -> repository.clearFlowState() is RegistrationFlowEvent.NavigateToScreen -> { if (event.route is RegistrationRoute.FullyComplete) { - repository.clearFlowState() + repository.clearInProgressRegistrationData() } else { repository.saveFlowState(_state.value) } @@ -195,7 +195,7 @@ class RegistrationViewModel(private val repository: RegistrationRepository, save is RegistrationFlowEvent.RestoreMethodTokenReceived, is RegistrationFlowEvent.UserSuppliedAepSubmitted, is RegistrationFlowEvent.UserSuppliedAepVerified -> repository.saveFlowState(_state.value) - is RegistrationFlowEvent.RegistrationComplete -> repository.clearFlowState() + is RegistrationFlowEvent.RegistrationComplete -> repository.clearInProgressRegistrationData() // No need to persist anything new, fields accounted for in proto already is RegistrationFlowEvent.Registered, diff --git a/feature/registration/src/main/java/org/signal/registration/screens/verificationcode/VerificationCodeScreen.kt b/feature/registration/src/main/java/org/signal/registration/screens/verificationcode/VerificationCodeScreen.kt index 2ab3dd40f2..f16fbe4f8c 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/verificationcode/VerificationCodeScreen.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/verificationcode/VerificationCodeScreen.kt @@ -50,6 +50,8 @@ import androidx.compose.ui.res.stringResource import androidx.compose.ui.text.input.KeyboardType import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.unit.dp +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.compose.LifecycleEventEffect import kotlinx.coroutines.delay import org.signal.core.ui.compose.AllDevicePreviews import org.signal.core.ui.compose.Previews @@ -146,6 +148,10 @@ fun VerificationCodeScreen( focusRequesters[0].requestFocus() } + LifecycleEventEffect(Lifecycle.Event.ON_RESUME) { + onEvent(VerificationCodeScreenEvents.Foregrounded) + } + Scaffold( snackbarHost = { SnackbarHost(snackbarHostState) }, modifier = modifier diff --git a/feature/registration/src/main/java/org/signal/registration/screens/verificationcode/VerificationCodeScreenEvents.kt b/feature/registration/src/main/java/org/signal/registration/screens/verificationcode/VerificationCodeScreenEvents.kt index 4316ed9625..d5189da4ca 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/verificationcode/VerificationCodeScreenEvents.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/verificationcode/VerificationCodeScreenEvents.kt @@ -35,4 +35,10 @@ sealed class VerificationCodeScreenEvents { * Event to update countdown timers. Should be triggered periodically (e.g., every second). */ data object CountdownTick : VerificationCodeScreenEvents() + + /** + * The screen returned to the foreground. Used to check whether the in-progress registration data (and thus the + * verification session) has grown too stale to keep waiting on, in which case we restart the flow. + */ + data object Foregrounded : VerificationCodeScreenEvents() } diff --git a/feature/registration/src/main/java/org/signal/registration/screens/verificationcode/VerificationCodeViewModel.kt b/feature/registration/src/main/java/org/signal/registration/screens/verificationcode/VerificationCodeViewModel.kt index 11751545dd..9869ecabf1 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/verificationcode/VerificationCodeViewModel.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/verificationcode/VerificationCodeViewModel.kt @@ -41,6 +41,7 @@ import org.signal.registration.screens.util.navigateTo import org.signal.registration.screens.verificationcode.VerificationCodeState.OneTimeEvent import kotlin.time.Duration import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.minutes import kotlin.time.Duration.Companion.seconds class VerificationCodeViewModel( @@ -54,6 +55,12 @@ class VerificationCodeViewModel( companion object { private val TAG = Log.tag(VerificationCodeViewModel::class) + /** + * How old the in-progress registration data can be before we assume the verification session has expired and + * restart the flow. Checked whenever the screen is foregrounded. + */ + private val IN_PROGRESS_DATA_TIMEOUT = 15.minutes + /** * Cold [Flow] of verification codes automatically retrieved from incoming SMS messages via the Play Services SMS * retriever. Registers a [BroadcastReceiver] for [SmsRetriever.SMS_RETRIEVED_ACTION] while collected, and @@ -123,10 +130,27 @@ class VerificationCodeViewModel( is VerificationCodeScreenEvents.HavingTrouble -> throw NotImplementedError("having trouble flow") // TODO [registration] - Having trouble flow is VerificationCodeScreenEvents.ConsumeInnerOneTimeEvent -> state.copy(oneTimeEvent = null) is VerificationCodeScreenEvents.CountdownTick -> applyCountdownTick(state) + is VerificationCodeScreenEvents.Foregrounded -> applyForegrounded(state) } stateEmitter(result) } + /** + * If the in-progress registration data has grown older than [IN_PROGRESS_DATA_TIMEOUT], the verification session has + * likely expired server-side. Rather than let the user enter a code only to fail, restart the flow from the beginning. + */ + private suspend fun applyForegrounded(state: VerificationCodeState): VerificationCodeState { + val lastUpdated = repository.getInProgressRegistrationDataLastUpdated() ?: return state + val age = (clock() - lastUpdated).milliseconds + + if (age >= IN_PROGRESS_DATA_TIMEOUT) { + Log.w(TAG, "[Foregrounded] In-progress registration data is stale (${age.inWholeMilliseconds}ms old). Restarting the flow.") + parentEventEmitter(RegistrationFlowEvent.ResetState) + } + + return state + } + @VisibleForTesting fun applyParentState(state: VerificationCodeState, parentState: RegistrationFlowState): VerificationCodeState { if (parentState.sessionMetadata == null || parentState.sessionE164 == null) { diff --git a/feature/registration/src/main/protowire/Registration.proto b/feature/registration/src/main/protowire/Registration.proto index 83a8a57558..6b27b42ff9 100644 --- a/feature/registration/src/main/protowire/Registration.proto +++ b/feature/registration/src/main/protowire/Registration.proto @@ -39,7 +39,11 @@ message RegistrationData { bool fetchesMessages = 23; string flowStateJson = 24; - // Next: 25 + + // Epoch milliseconds of the last write to this data. Used to detect data that has grown too stale to trust (e.g. an + // expired verification session). Stamped on every write. + int64 lastUpdatedMillis = 25; + // Next: 26 } message SvrCredential { diff --git a/feature/registration/src/test/java/org/signal/registration/RegistrationViewModelTest.kt b/feature/registration/src/test/java/org/signal/registration/RegistrationViewModelTest.kt index 199f851b99..ec78dcb82e 100644 --- a/feature/registration/src/test/java/org/signal/registration/RegistrationViewModelTest.kt +++ b/feature/registration/src/test/java/org/signal/registration/RegistrationViewModelTest.kt @@ -201,7 +201,7 @@ class RegistrationViewModelTest { } @Test - fun `onEvent NavigateToScreen FullyComplete clears flow state`() = runTest(testDispatcher) { + fun `onEvent NavigateToScreen FullyComplete clears in-progress registration data`() = runTest(testDispatcher) { coEvery { mockRepository.restoreFlowState() } returns null coEvery { mockRepository.getPreExistingRegistrationData() } returns null @@ -211,7 +211,7 @@ class RegistrationViewModelTest { viewModel.onEvent(RegistrationFlowEvent.NavigateToScreen(RegistrationRoute.FullyComplete)) advanceUntilIdle() - coVerify { mockRepository.clearFlowState() } + coVerify { mockRepository.clearInProgressRegistrationData() } } @Test @@ -257,7 +257,7 @@ class RegistrationViewModelTest { } @Test - fun `onEvent RegistrationComplete commits final data and clears flow state`() = runTest(testDispatcher) { + fun `onEvent RegistrationComplete commits final data and clears in-progress registration data`() = runTest(testDispatcher) { coEvery { mockRepository.restoreFlowState() } returns null coEvery { mockRepository.getPreExistingRegistrationData() } returns null @@ -268,7 +268,7 @@ class RegistrationViewModelTest { advanceUntilIdle() coVerify { mockRepository.commitFinalRegistrationData() } - coVerify { mockRepository.clearFlowState() } + coVerify { mockRepository.clearInProgressRegistrationData() } } // ==================== applyEvent Tests (Navigation & State Reducers) ==================== diff --git a/feature/registration/src/test/java/org/signal/registration/screens/verificationcode/VerificationCodeViewModelTest.kt b/feature/registration/src/test/java/org/signal/registration/screens/verificationcode/VerificationCodeViewModelTest.kt index 0bbba8db8d..9c56b94d21 100644 --- a/feature/registration/src/test/java/org/signal/registration/screens/verificationcode/VerificationCodeViewModelTest.kt +++ b/feature/registration/src/test/java/org/signal/registration/screens/verificationcode/VerificationCodeViewModelTest.kt @@ -37,6 +37,7 @@ import org.signal.registration.RegistrationFlowEvent import org.signal.registration.RegistrationFlowState import org.signal.registration.RegistrationRepository import org.signal.registration.RegistrationRoute +import kotlin.time.Duration.Companion.minutes import kotlin.time.Duration.Companion.seconds @OptIn(ExperimentalCoroutinesApi::class) @@ -869,6 +870,40 @@ class VerificationCodeViewModelTest { assertThat(emittedStates.last().oneTimeEvent).isEqualTo(VerificationCodeState.OneTimeEvent.UnableToSendSms) } + // ==================== applyEvent: Foregrounded Tests ==================== + + @Test + fun `Foregrounded emits ResetState when in-progress data is older than the timeout`() = runTest { + val now = 100.minutes.inWholeMilliseconds + coEvery { mockRepository.getInProgressRegistrationDataLastUpdated() } returns now - 16.minutes.inWholeMilliseconds + + val vm = VerificationCodeViewModel(mockRepository, parentState, parentEventEmitter, clock = { now }) + vm.applyEvent(VerificationCodeState(), VerificationCodeScreenEvents.Foregrounded, stateEmitter) + + assertThat(emittedEvents).hasSize(1) + assertThat(emittedEvents.first()).isEqualTo(RegistrationFlowEvent.ResetState) + } + + @Test + fun `Foregrounded does not emit ResetState when in-progress data is within the timeout`() = runTest { + val now = 100.minutes.inWholeMilliseconds + coEvery { mockRepository.getInProgressRegistrationDataLastUpdated() } returns now - 14.minutes.inWholeMilliseconds + + val vm = VerificationCodeViewModel(mockRepository, parentState, parentEventEmitter, clock = { now }) + vm.applyEvent(VerificationCodeState(), VerificationCodeScreenEvents.Foregrounded, stateEmitter) + + assertThat(emittedEvents).hasSize(0) + } + + @Test + fun `Foregrounded does not emit ResetState when there is no in-progress data`() = runTest { + coEvery { mockRepository.getInProgressRegistrationDataLastUpdated() } returns null + + viewModel.applyEvent(VerificationCodeState(), VerificationCodeScreenEvents.Foregrounded, stateEmitter) + + assertThat(emittedEvents).hasSize(0) + } + // ==================== Helper Functions ==================== private fun createSessionMetadata(