From 9570c95a03501d6cad99f8678e55aa568e3882d9 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 26 Feb 2024 16:07:16 -0300 Subject: [PATCH] WIP: Request @pm@ backup after initialization to avoid a 2nd restore set being used. This also changes the initialization behavior to only create the restore set folder and upload the metadata only when we actually need to. This way, double inits are not creating new restore sets on the backup destination. --- .../seedvault/KoinInstrumentationTestApp.kt | 2 +- .../java/com/stevesoltys/seedvault/App.kt | 2 +- .../seedvault/metadata/MetadataManager.kt | 9 ++- .../saf/DocumentsProviderStoragePlugin.kt | 9 --- .../transport/backup/BackupCoordinator.kt | 15 ++-- .../transport/backup/BackupInitializer.kt | 73 +++++++++++++++++++ .../transport/backup/BackupModule.kt | 1 + .../ui/recoverycode/RecoveryCodeViewModel.kt | 14 ++-- .../ui/storage/BackupStorageViewModel.kt | 60 +++++---------- .../seedvault/metadata/MetadataManagerTest.kt | 2 +- .../transport/backup/BackupCoordinatorTest.kt | 7 +- 11 files changed, 115 insertions(+), 79 deletions(-) create mode 100644 app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupInitializer.kt diff --git a/app/src/androidTest/java/com/stevesoltys/seedvault/KoinInstrumentationTestApp.kt b/app/src/androidTest/java/com/stevesoltys/seedvault/KoinInstrumentationTestApp.kt index 6a2e56013..c00438f20 100644 --- a/app/src/androidTest/java/com/stevesoltys/seedvault/KoinInstrumentationTestApp.kt +++ b/app/src/androidTest/java/com/stevesoltys/seedvault/KoinInstrumentationTestApp.kt @@ -47,7 +47,7 @@ class KoinInstrumentationTestApp : App() { viewModel { currentBackupStorageViewModel = - spyk(BackupStorageViewModel(context, get(), get(), get())) + spyk(BackupStorageViewModel(context, get(), get(), get(), get())) currentBackupStorageViewModel!! } diff --git a/app/src/main/java/com/stevesoltys/seedvault/App.kt b/app/src/main/java/com/stevesoltys/seedvault/App.kt index a28ac628c..9391a801e 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/App.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/App.kt @@ -59,7 +59,7 @@ open class App : Application() { viewModel { SettingsViewModel(this@App, get(), get(), get(), get(), get(), get()) } viewModel { RecoveryCodeViewModel(this@App, get(), get(), get(), get(), get(), get()) } - viewModel { BackupStorageViewModel(this@App, get(), get(), get()) } + viewModel { BackupStorageViewModel(this@App, get(), get(), get(), get()) } viewModel { RestoreStorageViewModel(this@App, get(), get()) } viewModel { RestoreViewModel(this@App, get(), get(), get(), get(), get(), get()) } viewModel { FileSelectionViewModel(this@App, get()) } diff --git a/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataManager.kt b/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataManager.kt index 5c3d1a7a6..bd5b4f381 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataManager.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataManager.kt @@ -59,14 +59,15 @@ internal class MetadataManager( /** * Call this when initializing a new device. * - * Existing [BackupMetadata] will be cleared, use the given new token, - * and written encrypted to the given [OutputStream] as well as the internal cache. + * Existing [BackupMetadata] will be cleared + * and new metadata with the given [token] will be written to the internal cache + * with a fresh salt. */ @Synchronized @Throws(IOException::class) - fun onDeviceInitialization(token: Long, metadataOutputStream: OutputStream) { + fun onDeviceInitialization(token: Long) { val salt = crypto.getRandomBytes(METADATA_SALT_SIZE).encodeBase64() - modifyMetadata(metadataOutputStream) { + modifyCachedMetadata { metadata = BackupMetadata(token = token, salt = salt) } } diff --git a/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderStoragePlugin.kt b/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderStoragePlugin.kt index 0dbc6c50e..e8e02baa5 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderStoragePlugin.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderStoragePlugin.kt @@ -35,22 +35,13 @@ internal class DocumentsProviderStoragePlugin( override suspend fun startNewRestoreSet(token: Long) { // reset current storage storage.reset(token) - - // get or create root backup dir - storage.rootBackupDir ?: throw IOException() } @Throws(IOException::class) override suspend fun initializeDevice() { - // wipe existing data - storage.getSetDir()?.deleteContents(context) - // reset storage without new token, so folders get recreated // otherwise stale DocumentFiles will hang around storage.reset(null) - - // create backup folders - storage.currentSetDir ?: throw IOException() } @Throws(IOException::class) diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt index e9275bf7e..4f77f4adc 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt @@ -86,12 +86,13 @@ internal class BackupCoordinator( * @return the token of the new [RestoreSet]. */ @Throws(IOException::class) - private suspend fun startNewRestoreSet(): Long { + private suspend fun startNewRestoreSet() { val token = clock.time() Log.i(TAG, "Starting new RestoreSet with token $token...") settingsManager.setNewToken(token) plugin.startNewRestoreSet(token) - return token + Log.d(TAG, "Resetting backup metadata...") + metadataManager.onDeviceInitialization(token) } /** @@ -115,18 +116,15 @@ internal class BackupCoordinator( suspend fun initializeDevice(): Int = try { // we don't respect the intended system behavior here by always starting a new [RestoreSet] // instead of simply deleting the current one - val token = startNewRestoreSet() + startNewRestoreSet() Log.i(TAG, "Initialize Device!") plugin.initializeDevice() - Log.d(TAG, "Resetting backup metadata for token $token...") - plugin.getMetadataOutputStream(token).use { - metadataManager.onDeviceInitialization(token, it) - } + startNewRestoreSet() // [finishBackup] will only be called when we return [TRANSPORT_OK] here // so we remember that we initialized successfully state.calledInitialize = true TRANSPORT_OK - } catch (e: IOException) { + } catch (e: Exception) { Log.e(TAG, "Error initializing device", e) // Show error notification if we needed init or were ready for backups if (metadataManager.requiresInit || settingsManager.canDoBackupNow()) nm.onBackupError() @@ -222,6 +220,7 @@ internal class BackupCoordinator( state.cancelReason = UNKNOWN_ERROR if (metadataManager.requiresInit) { Log.w(TAG, "Metadata requires re-init!") + // TODO test if this still works // start a new restore set to upgrade from legacy format // by starting a clean backup with all files using the new version try { diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupInitializer.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupInitializer.kt new file mode 100644 index 000000000..9d83d6185 --- /dev/null +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupInitializer.kt @@ -0,0 +1,73 @@ +/* + * SPDX-FileCopyrightText: 2024 The Calyx Institute + * SPDX-License-Identifier: Apache-2.0 + */ + +package com.stevesoltys.seedvault.transport.backup + +import android.app.backup.BackupProgress +import android.app.backup.IBackupManager +import android.app.backup.IBackupObserver +import android.os.UserHandle +import android.util.Log +import androidx.annotation.WorkerThread +import com.stevesoltys.seedvault.BackupMonitor +import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER +import com.stevesoltys.seedvault.transport.TRANSPORT_ID + +class BackupInitializer( + private val backupManager: IBackupManager, +) { + + companion object { + private val TAG = BackupInitializer::class.simpleName + } + + fun initialize(onError: () -> Unit, onSuccess: () -> Unit) { + val observer = BackupObserver("Initialization", onError) { + // After successful initialization, we request a @pm@ backup right away, + // because if this finds empty state, it asks us to do another initialization. + // And then we end up with yet another restore set token. + // Since we want the final token as soon as possible, we need to get it here. + Log.d(TAG, "Requesting initial $MAGIC_PACKAGE_MANAGER backup...") + backupManager.requestBackup( + arrayOf(MAGIC_PACKAGE_MANAGER), + BackupObserver("Initial backup of @pm@", onError, onSuccess), + BackupMonitor(), + 0, + ) + } + backupManager.initializeTransportsForUser( + UserHandle.myUserId(), + arrayOf(TRANSPORT_ID), + observer, + ) + } + + @WorkerThread + private inner class BackupObserver( + private val operation: String, + private val onError: () -> Unit, + private val onSuccess: () -> Unit, + ) : IBackupObserver.Stub() { + override fun onUpdate(currentBackupPackage: String, backupProgress: BackupProgress) { + // noop + } + + override fun onResult(target: String, status: Int) { + // noop + } + + override fun backupFinished(status: Int) { + if (Log.isLoggable(TAG, Log.INFO)) { + Log.i(TAG, "$operation finished. Status: $status") + } + if (status == 0) { + onSuccess() + } else { + onError() + } + } + } + +} diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupModule.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupModule.kt index 3ed9caedd..6bb6f6e2c 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupModule.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupModule.kt @@ -4,6 +4,7 @@ import org.koin.android.ext.koin.androidContext import org.koin.dsl.module val backupModule = module { + single { BackupInitializer(get()) } single { InputFactory() } single { PackageService( diff --git a/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt b/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt index ba126ab1a..274187c3b 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt @@ -1,7 +1,6 @@ package com.stevesoltys.seedvault.ui.recoverycode import android.app.backup.IBackupManager -import android.os.UserHandle import android.util.Log import androidx.lifecycle.AndroidViewModel import cash.z.ecc.android.bip39.Mnemonics @@ -12,8 +11,7 @@ import cash.z.ecc.android.bip39.toSeed import com.stevesoltys.seedvault.App import com.stevesoltys.seedvault.crypto.Crypto import com.stevesoltys.seedvault.crypto.KeyManager -import com.stevesoltys.seedvault.transport.TRANSPORT_ID -import com.stevesoltys.seedvault.transport.backup.BackupCoordinator +import com.stevesoltys.seedvault.transport.backup.BackupInitializer import com.stevesoltys.seedvault.ui.LiveEvent import com.stevesoltys.seedvault.ui.MutableLiveEvent import com.stevesoltys.seedvault.ui.notification.BackupNotificationManager @@ -32,7 +30,7 @@ internal class RecoveryCodeViewModel( private val crypto: Crypto, private val keyManager: KeyManager, private val backupManager: IBackupManager, - private val backupCoordinator: BackupCoordinator, + private val backupInitializer: BackupInitializer, private val notificationManager: BackupNotificationManager, private val storageBackup: StorageBackup, ) : AndroidViewModel(app) { @@ -109,11 +107,9 @@ internal class RecoveryCodeViewModel( storageBackup.clearCache() try { // initialize the new location - if (backupManager.isBackupEnabled) backupManager.initializeTransportsForUser( - UserHandle.myUserId(), - arrayOf(TRANSPORT_ID), - null - ) + if (backupManager.isBackupEnabled) backupInitializer.initialize({ }) { + // no-op + } } catch (e: IOException) { Log.e(TAG, "Error starting new RestoreSet", e) } diff --git a/app/src/main/java/com/stevesoltys/seedvault/ui/storage/BackupStorageViewModel.kt b/app/src/main/java/com/stevesoltys/seedvault/ui/storage/BackupStorageViewModel.kt index 7a3a06a6f..bf6c729bb 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/ui/storage/BackupStorageViewModel.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/ui/storage/BackupStorageViewModel.kt @@ -1,20 +1,16 @@ package com.stevesoltys.seedvault.ui.storage import android.app.Application -import android.app.backup.BackupProgress import android.app.backup.IBackupManager -import android.app.backup.IBackupObserver import android.app.job.JobInfo import android.net.Uri -import android.os.UserHandle import android.util.Log -import androidx.annotation.WorkerThread import androidx.lifecycle.viewModelScope import androidx.work.ExistingPeriodicWorkPolicy.CANCEL_AND_REENQUEUE import com.stevesoltys.seedvault.R import com.stevesoltys.seedvault.settings.SettingsManager import com.stevesoltys.seedvault.storage.StorageBackupJobService -import com.stevesoltys.seedvault.transport.TRANSPORT_ID +import com.stevesoltys.seedvault.transport.backup.BackupInitializer import com.stevesoltys.seedvault.worker.AppBackupWorker import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch @@ -28,6 +24,7 @@ private val TAG = BackupStorageViewModel::class.java.simpleName internal class BackupStorageViewModel( private val app: Application, private val backupManager: IBackupManager, + private val backupInitializer: BackupInitializer, private val storageBackup: StorageBackup, settingsManager: SettingsManager, ) : StorageViewModel(app, settingsManager) { @@ -52,13 +49,23 @@ internal class BackupStorageViewModel( storageBackup.clearCache() try { // initialize the new location (if backups are enabled) - if (backupManager.isBackupEnabled) backupManager.initializeTransportsForUser( - UserHandle.myUserId(), - arrayOf(TRANSPORT_ID), - // if storage is on USB and this is not SetupWizard, do a backup right away - InitializationObserver(isUsb && !isSetupWizard) - ) else { - InitializationObserver(false).backupFinished(0) + if (backupManager.isBackupEnabled) { + val onError = { + Log.e(TAG, "Error starting new RestoreSet") + onInitializationError() + } + backupInitializer.initialize(onError) { + val requestBackup = isUsb && !isSetupWizard + if (requestBackup) { + Log.i(TAG, "Requesting a backup now, because we use USB storage") + AppBackupWorker.scheduleNow(app, reschedule = false) + } + // notify the UI that the location has been set + mLocationChecked.postEvent(LocationResult()) + } + } else { + // notify the UI that the location has been set + mLocationChecked.postEvent(LocationResult()) } } catch (e: IOException) { Log.e(TAG, "Error starting new RestoreSet", e) @@ -90,35 +97,6 @@ internal class BackupStorageViewModel( BackupJobService.cancelJob(app) } - @WorkerThread - private inner class InitializationObserver(val requestBackup: Boolean) : - IBackupObserver.Stub() { - override fun onUpdate(currentBackupPackage: String, backupProgress: BackupProgress) { - // noop - } - - override fun onResult(target: String, status: Int) { - // noop - } - - override fun backupFinished(status: Int) { - if (Log.isLoggable(TAG, Log.INFO)) { - Log.i(TAG, "Initialization finished. Status: $status") - } - if (status == 0) { - // notify the UI that the location has been set - mLocationChecked.postEvent(LocationResult()) - if (requestBackup) { - val isUsb = settingsManager.getStorage()?.isUsb ?: false - AppBackupWorker.scheduleNow(app, reschedule = !isUsb) - } - } else { - // notify the UI that the location was invalid - onInitializationError() - } - } - } - private fun onInitializationError() { val errorMsg = app.getString(R.string.storage_check_fragment_backup_error) mLocationChecked.postEvent(LocationResult(errorMsg)) diff --git a/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt b/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt index f3dbb0167..62300e76d 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt @@ -100,7 +100,7 @@ class MetadataManagerTest { expectReadFromCache() expectModifyMetadata(initialMetadata) - manager.onDeviceInitialization(token, storageOutputStream) + manager.onDeviceInitialization(token) assertEquals(token, manager.getBackupToken()) assertEquals(0L, manager.getLastBackupTime()) diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt index 2fed54abe..598a6ff43 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt @@ -69,22 +69,18 @@ internal class BackupCoordinatorTest : BackupTest() { fun `device initialization succeeds and delegates to plugin`() = runBlocking { expectStartNewRestoreSet() coEvery { plugin.initializeDevice() } just Runs - coEvery { plugin.getOutputStream(token, FILE_BACKUP_METADATA) } returns metadataOutputStream - every { metadataManager.onDeviceInitialization(token, metadataOutputStream) } just Runs every { kv.hasState() } returns false every { full.hasState() } returns false - every { metadataOutputStream.close() } just Runs assertEquals(TRANSPORT_OK, backup.initializeDevice()) assertEquals(TRANSPORT_OK, backup.finishBackup()) - - verify { metadataOutputStream.close() } } private suspend fun expectStartNewRestoreSet() { every { clock.time() } returns token every { settingsManager.setNewToken(token) } just Runs coEvery { plugin.startNewRestoreSet(token) } just Runs + every { metadataManager.onDeviceInitialization(token) } just Runs } @Test @@ -136,6 +132,7 @@ internal class BackupCoordinatorTest : BackupTest() { every { clock.time() } returns token + 1 every { settingsManager.setNewToken(token + 1) } just Runs coEvery { plugin.startNewRestoreSet(token + 1) } just Runs + every { metadataManager.onDeviceInitialization(token + 1) } just Runs every { data.close() } just Runs