From 4a0325ea55a54eb5bc0928101bbf4d2c0a407b8e Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Tue, 30 Jun 2026 16:36:59 -0400 Subject: [PATCH] Fix restoration of PIN during remote restore in regV5. --- .../securesms/backup/v2/BackupRepository.kt | 52 +++++++------ .../InternalBackupPlaygroundViewModel.kt | 2 +- .../ui/restore/RemoteRestoreViewModel.kt | 2 +- .../v2/AppRegistrationStorageController.kt | 14 +++- .../dependencies/DemoStorageController.kt | 4 +- .../registration/RegistrationRepository.kt | 15 +++- .../RemoteBackupRestoreProgress.kt | 15 +++- .../RemoteBackupRestoreViewModel.kt | 1 + .../src/main/protowire/Registration.proto | 77 ++++++++----------- .../RemoteBackupRestoreViewModelTest.kt | 6 +- 10 files changed, 102 insertions(+), 86 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/backup/v2/BackupRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/backup/v2/BackupRepository.kt index e2dc149625..e2d8afa9f9 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/backup/v2/BackupRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/backup/v2/BackupRepository.kt @@ -1250,6 +1250,7 @@ object BackupRepository { return ImportResult.Failure } SignalStore.backup.hasInvalidBackupVersion = false + val selfId: RecipientId var transactionSuccessful = false try { @@ -1321,7 +1322,7 @@ object BackupRepository { SignalStore.backup.mediaRootBackupKey = mediaRootBackupKey // Add back self after clearing data - val selfId: RecipientId = SignalDatabase.recipients.getAndPossiblyMerge(selfData.aci, selfData.pni, selfData.e164, pniVerified = true, changeSelf = true) + selfId = SignalDatabase.recipients.getAndPossiblyMerge(selfData.aci, selfData.pni, selfData.e164, pniVerified = true, changeSelf = true) SignalDatabase.recipients.setProfileKey(selfId, selfData.profileKey) SignalDatabase.recipients.setProfileSharing(selfId, true) @@ -1532,7 +1533,7 @@ object BackupRepository { Log.d(TAG, "[import] Finished! ${eventTimer.stop().summary}") stopwatch.stop(TAG) - return ImportResult.Success(backupTime = header.backupTimeMs) + return ImportResult.Success(backupTime = header.backupTimeMs, selfRecipientId = selfId) } fun listRemoteMediaObjects(limit: Int, cursor: String? = null): NetworkResult { @@ -2310,16 +2311,20 @@ object BackupRepository { forwardSecrecyToken = forwardSecrecyToken, cancellationSignal = cancellationSignal ) - if (result == ImportResult.Failure) { - Log.w(TAG, "[remoteRestore] Failed to import backup") - return RemoteRestoreResult.Failure + + return when (result) { + is ImportResult.Failure -> { + Log.w(TAG, "[remoteRestore] Failed to import backup") + RemoteRestoreResult.Failure + } + + is ImportResult.Success -> { + Log.i(TAG, "[remoteRestore] Restore successful") + BackupMediaRestoreService.resetTimeout() + AppDependencies.jobManager.add(BackupRestoreMediaJob()) + RemoteRestoreResult.Success(result.selfRecipientId) + } } - - BackupMediaRestoreService.resetTimeout() - AppDependencies.jobManager.add(BackupRestoreMediaJob()) - - Log.i(TAG, "[remoteRestore] Restore successful") - return RemoteRestoreResult.Success } suspend fun restoreLinkAndSyncBackup(response: TransferArchiveResponse, ephemeralBackupKey: MessageBackupKey): RemoteRestoreResult { @@ -2392,16 +2397,19 @@ object BackupRepository { cancellationSignal = cancellationSignal ) - if (result == ImportResult.Failure) { - Log.w(TAG, "[restoreLinkAndSyncBackup] Failed to import backup") - return RemoteRestoreResult.Failure + return when (result) { + is ImportResult.Failure -> { + Log.w(TAG, "[restoreLinkAndSyncBackup] Failed to import backup") + RemoteRestoreResult.Failure + } + + is ImportResult.Success -> { + Log.i(TAG, "[restoreLinkAndSyncBackup] Restore successful") + BackupMediaRestoreService.resetTimeout() + AppDependencies.jobManager.add(BackupRestoreMediaJob()) + RemoteRestoreResult.Success(result.selfRecipientId) + } } - - BackupMediaRestoreService.resetTimeout() - AppDependencies.jobManager.add(BackupRestoreMediaJob()) - - Log.i(TAG, "[restoreLinkAndSyncBackup] Restore successful") - return RemoteRestoreResult.Success } private fun buildDebugInfo(): ByteString { @@ -2531,12 +2539,12 @@ data class StagedBackupKeyRotations( ) sealed class ImportResult { - data class Success(val backupTime: Long) : ImportResult() + data class Success(val backupTime: Long, val selfRecipientId: RecipientId) : ImportResult() data object Failure : ImportResult() } sealed interface RemoteRestoreResult { - data object Success : RemoteRestoreResult + data class Success(val selfRecipientId: RecipientId) : RemoteRestoreResult data object NetworkError : RemoteRestoreResult data object Canceled : RemoteRestoreResult data object Failure : RemoteRestoreResult diff --git a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/internal/backup/InternalBackupPlaygroundViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/internal/backup/InternalBackupPlaygroundViewModel.kt index 9011d04009..f593d53634 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/internal/backup/InternalBackupPlaygroundViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/internal/backup/InternalBackupPlaygroundViewModel.kt @@ -336,7 +336,7 @@ class InternalBackupPlaygroundViewModel : ViewModel() { viewModelScope.launch { when (val result = BackupRepository.restoreRemoteBackup()) { - RemoteRestoreResult.Success -> { + is RemoteRestoreResult.Success -> { _state.value = _state.value.copy(statusMessage = "Import complete!") ThreadUtil.runOnMain { afterDbRestoreCallback() } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/registration/ui/restore/RemoteRestoreViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/registration/ui/restore/RemoteRestoreViewModel.kt index 522fcc8742..19025505d9 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/registration/ui/restore/RemoteRestoreViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/registration/ui/restore/RemoteRestoreViewModel.kt @@ -118,7 +118,7 @@ class RemoteRestoreViewModel(isOnlyRestoreOption: Boolean) : ViewModel() { QuickRegistrationRepository.setRestoreMethodForOldDevice(RestoreMethod.REMOTE_BACKUP) when (val result = BackupRepository.restoreRemoteBackup()) { - RemoteRestoreResult.Success -> { + is RemoteRestoreResult.Success -> { Log.i(TAG, "Restore successful", true) SignalStore.registration.restoreDecisionState = RestoreDecisionState.Completed 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 1059b8e6ab..bb51f0c709 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 @@ -30,6 +30,7 @@ import org.signal.core.util.Result import org.signal.core.util.StreamUtil import org.signal.core.util.crypto.AttachmentSecretProvider import org.signal.core.util.logging.Log +import org.signal.libsignal.zkgroup.profiles.ProfileKey import org.signal.registration.PreExistingRegistrationData import org.signal.registration.RestoreDecision import org.signal.registration.StorageController @@ -446,9 +447,14 @@ class AppRegistrationStorageController(private val context: Context) : StorageCo launch(Dispatchers.IO) { try { - when (BackupRepository.restoreRemoteBackup()) { - RemoteRestoreResult.Success -> { - send(RemoteBackupRestoreProgress.Complete) + when (val result = BackupRepository.restoreRemoteBackup()) { + is RemoteRestoreResult.Success -> { + send( + RemoteBackupRestoreProgress.Complete( + restoredSvrPin = SignalStore.svr.pin, + restoredProfileKey = SignalDatabase.recipients.getRecord(result.selfRecipientId).profileKey?.let { ProfileKey(it) } + ) + ) } RemoteRestoreResult.NetworkError -> { send(RemoteBackupRestoreProgress.NetworkError()) @@ -507,7 +513,7 @@ class AppRegistrationStorageController(private val context: Context) : StorageCo launch(Dispatchers.IO) { try { when (val result = BackupRepository.restoreLinkAndSyncBackup(TransferArchiveResponse(cdn = cdn, key = key), MessageBackupKey(ephemeralBackupKeyBytes))) { - RemoteRestoreResult.Success -> send(LinkAndSyncProgress.Complete) + is RemoteRestoreResult.Success -> send(LinkAndSyncProgress.Complete) RemoteRestoreResult.Canceled -> Log.i(TAG, "[restoreLinkAndSyncBackup] Restore canceled.") else -> { Log.w(TAG, "[restoreLinkAndSyncBackup] Link-and-sync restore did not succeed: $result") 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 ebcc64ea28..65b56bab6b 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 @@ -185,7 +185,7 @@ class DemoStorageController(private val context: Context) : StorageController { // PIN data if (data.pin.isNotEmpty()) { RegistrationPreferences.pin = data.pin - RegistrationPreferences.pinAlphanumeric = data.pinIsAlphanumeric + RegistrationPreferences.pinAlphanumeric = data.pin.any { !it.isDigit() } } if (data.temporaryMasterKey.size > 0) { RegistrationPreferences.temporaryMasterKey = MasterKey(data.temporaryMasterKey.toByteArray()) @@ -352,7 +352,7 @@ class DemoStorageController(private val context: Context) : StorageController { emit(RemoteBackupRestoreProgress.Finalizing) delay(250) - emit(RemoteBackupRestoreProgress.Complete) + emit(RemoteBackupRestoreProgress.Complete(restoredSvrPin = null, restoredProfileKey = null)) Log.d(TAG, "Simulated remote restore complete.") }.flowOn(Dispatchers.IO) 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 d6264bd2f0..0688a03e01 100644 --- a/feature/registration/src/main/java/org/signal/registration/RegistrationRepository.kt +++ b/feature/registration/src/main/java/org/signal/registration/RegistrationRepository.kt @@ -175,7 +175,6 @@ class RegistrationRepository(val context: Context, val networkController: Networ if (it is RequestResult.Success) { storageController.updateInProgressRegistrationData { this.pin = pin - this.pinIsAlphanumeric = isAlphanumeric this.temporaryMasterKey = it.result.masterKey.serialize().toByteString() this.registrationLockEnabled = forRegistrationLock this.svrCredentials += SvrCredential(username = svrCredentials.username, password = svrCredentials.password) @@ -454,7 +453,6 @@ class RegistrationRepository(val context: Context, val networkController: Networ backupVersion = provisioningMessage.backupVersion ) pin = provisioningMessage.pin ?: "" - pinIsAlphanumeric = provisioningMessage.pin?.any { !it.isDigit() } == true } val aep = AccountEntropyPool(provisioningMessage.accountEntropyPool) @@ -663,7 +661,6 @@ class RegistrationRepository(val context: Context, val networkController: Networ if (result is RequestResult.Success) { storageController.updateInProgressRegistrationData { this.pin = pin - this.pinIsAlphanumeric = isAlphanumeric result.result?.let { credential -> this.svrCredentials += SvrCredential(username = credential.username, password = credential.password) } @@ -688,12 +685,22 @@ class RegistrationRepository(val context: Context, val networkController: Networ storageController.updateInProgressRegistrationData { this.pinOptedOut = true this.pin = "" - this.pinIsAlphanumeric = false this.registrationLockEnabled = false } storageController.commitRegistrationData() } + /** + * Persist any data in our scratch storage that was restored as part of a remote backup so that we don't accidentally overwrite it + * when we commit it. + */ + suspend fun persistRemoteBackupRestoredState(restoredPin: String?, restoredProfileKey: ProfileKey?) { + storageController.updateInProgressRegistrationData { + pin = restoredPin ?: pin + profileKey = restoredProfileKey?.serialize()?.toByteString() ?: profileKey + } + } + /** * Records the terminal restore decision the user reached (new account, skipped a restore, or successfully restored) * and commits it. The app translates this into its own restore-decision state so the rest of the app knows what diff --git a/feature/registration/src/main/java/org/signal/registration/screens/remotebackuprestore/RemoteBackupRestoreProgress.kt b/feature/registration/src/main/java/org/signal/registration/screens/remotebackuprestore/RemoteBackupRestoreProgress.kt index d7f2461e39..97949b3af4 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/remotebackuprestore/RemoteBackupRestoreProgress.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/remotebackuprestore/RemoteBackupRestoreProgress.kt @@ -5,6 +5,8 @@ package org.signal.registration.screens.remotebackuprestore +import org.signal.libsignal.zkgroup.profiles.ProfileKey + /** * Progress events emitted during a remote backup restore operation. * Each value directly maps to a UI state for the progress dialog. @@ -19,8 +21,17 @@ sealed interface RemoteBackupRestoreProgress { /** Finalizing the restore (post-import cleanup). */ data object Finalizing : RemoteBackupRestoreProgress - /** Restore completed successfully. */ - data object Complete : RemoteBackupRestoreProgress + /** + * Restore completed successfully. + * Provides registration-relevant data that was restored so that it isn't accidentally overridden. + * + * If any of the args are null, we will assume that they were unavailable in the backup, and will defer to + * values generated during registration. + */ + data class Complete( + val restoredSvrPin: String?, + val restoredProfileKey: ProfileKey? + ) : RemoteBackupRestoreProgress /** Restore failed due to a network error (e.g. connection lost during download). */ data class NetworkError(val cause: Throwable? = null) : RemoteBackupRestoreProgress diff --git a/feature/registration/src/main/java/org/signal/registration/screens/remotebackuprestore/RemoteBackupRestoreViewModel.kt b/feature/registration/src/main/java/org/signal/registration/screens/remotebackuprestore/RemoteBackupRestoreViewModel.kt index c223aa3e4a..c7550d242f 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/remotebackuprestore/RemoteBackupRestoreViewModel.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/remotebackuprestore/RemoteBackupRestoreViewModel.kt @@ -120,6 +120,7 @@ class RemoteBackupRestoreViewModel( restoreProgress = null ) parentEventEmitter(RegistrationFlowEvent.UserSuppliedAepVerified(aep)) + repository.persistRemoteBackupRestoredState(progress.restoredSvrPin, progress.restoredProfileKey) repository.setRestoreDecision(RestoreDecision.COMPLETED) repository.finishRegistrationOrCreateProfile(parentEventEmitter) } diff --git a/feature/registration/src/main/protowire/Registration.proto b/feature/registration/src/main/protowire/Registration.proto index 6b87ac9b34..83a8a57558 100644 --- a/feature/registration/src/main/protowire/Registration.proto +++ b/feature/registration/src/main/protowire/Registration.proto @@ -10,53 +10,36 @@ package signal; option java_package = "org.signal.registration.proto"; message RegistrationData { - // Key material (from generateAndStoreKeyMaterial) - bytes aciIdentityKeyPair = 1; - bytes pniIdentityKeyPair = 2; - bytes aciSignedPreKey = 3; - bytes pniSignedPreKey = 4; - bytes aciLastResortKyberPreKey = 5; - bytes pniLastResortKyberPreKey = 6; - int32 aciRegistrationId = 7; - int32 pniRegistrationId = 8; - bytes unidentifiedAccessKey = 9; - string servicePassword = 10; - string accountEntropyPool = 11; + bytes aciIdentityKeyPair = 1; + bytes pniIdentityKeyPair = 2; + bytes aciSignedPreKey = 3; + bytes pniSignedPreKey = 4; + bytes aciLastResortKyberPreKey = 5; + bytes pniLastResortKyberPreKey = 6; + int32 aciRegistrationId = 7; + int32 pniRegistrationId = 8; - // Account identity (from saveNewRegistrationData / getPreExistingRegistrationData) - string e164 = 12; - string aci = 13; - string pni = 14; + string aci = 9; + string pni = 10; + string e164 = 11; + string servicePassword = 12; + string accountEntropyPool = 13; - // PIN data (from saveValidatedPinAndTemporaryMasterKey / saveNewlyCreatedPin) - string pin = 15; - bool pinIsAlphanumeric = 16; - bytes temporaryMasterKey = 17; - bool registrationLockEnabled = 18; + string pin = 14; + bool pinOptedOut = 15; + bool registrationLockEnabled = 16; - // Whether the user chose not to create a PIN during registration. The app is responsible for applying the - // actual opt-out (clearing PIN/registration lock state, refreshing attributes, etc.) when it commits this data. - bool pinOptedOut = 24; + bytes profileKey = 17; + bytes unidentifiedAccessKey = 18; + bytes temporaryMasterKey = 19; - // SVR credentials (from appendSvrCredentials / getRestoredSvrCredentials) - repeated SvrCredential svrCredentials = 19; + repeated SvrCredential svrCredentials = 20; + ProvisioningData provisioningData = 21; + LinkedDeviceData linkedDeviceData = 22; + bool fetchesMessages = 23; - // Provisioning data (from saveProvisioningData) - ProvisioningData provisioningData = 20; - - // Profile key (used to derive unidentified access key) - bytes profileKey = 22; - - // Whether this device fetches messages (true if no FCM token) - bool fetchesMessages = 23; - - // JSON-serialized flow state snapshot (from saveFlowState/restoreFlowState) - string flowStateJson = 21; - - // Registration data provided by the primary+server to register as a linked device - LinkedDeviceData linkedDeviceData = 25; - - // Next: 26 + string flowStateJson = 24; + // Next: 25 } message SvrCredential { @@ -77,12 +60,12 @@ message ProvisioningData { PAID = 2; } - string restoreMethodToken = 1; + string restoreMethodToken = 1; Platform platform = 2; - Tier tier = 3; - int64 backupTimestampMs = 4; - int64 backupSizeBytes = 5; - int64 backupVersion = 6; + Tier tier = 3; + int64 backupTimestampMs = 4; + int64 backupSizeBytes = 5; + int64 backupVersion = 6; } message LinkedDeviceData { diff --git a/feature/registration/src/test/java/org/signal/registration/screens/remotebackuprestore/RemoteBackupRestoreViewModelTest.kt b/feature/registration/src/test/java/org/signal/registration/screens/remotebackuprestore/RemoteBackupRestoreViewModelTest.kt index ab6324ab0b..14703d77d6 100644 --- a/feature/registration/src/test/java/org/signal/registration/screens/remotebackuprestore/RemoteBackupRestoreViewModelTest.kt +++ b/feature/registration/src/test/java/org/signal/registration/screens/remotebackuprestore/RemoteBackupRestoreViewModelTest.kt @@ -349,7 +349,7 @@ class RemoteBackupRestoreViewModelTest { @Test fun `Complete progress emits UserSuppliedAepVerified and hands off to finishRegistrationOrCreateProfile`() = runTest(testDispatcher) { every { mockRepository.restoreRemoteBackup(any()) } returns flowOf( - RemoteBackupRestoreProgress.Complete + RemoteBackupRestoreProgress.Complete(restoredSvrPin = null, restoredProfileKey = null) ) val viewModel = createViewModel() @@ -370,7 +370,7 @@ class RemoteBackupRestoreViewModelTest { @Test fun `Complete progress moves restore state to Restored and clears progress`() = runTest(testDispatcher) { every { mockRepository.restoreRemoteBackup(any()) } returns flowOf( - RemoteBackupRestoreProgress.Complete + RemoteBackupRestoreProgress.Complete(restoredSvrPin = null, restoredProfileKey = null) ) val viewModel = createViewModel() val states = collectStatesOf(viewModel) @@ -454,7 +454,7 @@ class RemoteBackupRestoreViewModelTest { RemoteBackupRestoreProgress.Downloading(bytesDownloaded = 10, totalBytes = 100), RemoteBackupRestoreProgress.Restoring(bytesRead = 60, totalBytes = 100), RemoteBackupRestoreProgress.Finalizing, - RemoteBackupRestoreProgress.Complete + RemoteBackupRestoreProgress.Complete(restoredSvrPin = null, restoredProfileKey = null) ) val viewModel = createViewModel() val states = collectStatesOf(viewModel)