From cacea886b08438c2059d2d2d5279f562fda10f16 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Wed, 11 Sep 2024 16:44:29 -0300 Subject: [PATCH] Simplify transport init and token handling The token used to be very important, because it was our restore set folder name. Now it is just a number in a snapshot, so things get a bit simpler. --- .../com/stevesoltys/seedvault/PluginTest.kt | 6 +- .../seedvault/e2e/LargeTestBase.kt | 2 +- .../java/com/stevesoltys/seedvault/App.kt | 18 ------ .../seedvault/metadata/MetadataManager.kt | 55 +----------------- .../seedvault/metadata/MetadataModule.kt | 2 +- .../restore/AppDataRestoreManager.kt | 6 -- .../seedvault/settings/SettingsManager.kt | 45 +++++++-------- .../transport/backup/BackupCoordinator.kt | 28 +--------- .../transport/restore/RestoreCoordinator.kt | 7 +-- .../seedvault/metadata/MetadataManagerTest.kt | 56 ------------------- .../transport/backup/BackupCoordinatorTest.kt | 56 ------------------- .../restore/RestoreCoordinatorTest.kt | 2 +- 12 files changed, 32 insertions(+), 251 deletions(-) diff --git a/app/src/androidTest/java/com/stevesoltys/seedvault/PluginTest.kt b/app/src/androidTest/java/com/stevesoltys/seedvault/PluginTest.kt index 40b8be89..bd92deeb 100644 --- a/app/src/androidTest/java/com/stevesoltys/seedvault/PluginTest.kt +++ b/app/src/androidTest/java/com/stevesoltys/seedvault/PluginTest.kt @@ -95,7 +95,7 @@ class PluginTest : KoinComponent { assertEquals(0, backend.getAvailableBackupFileHandles().toList().size) // prepare returned tokens requested when initializing device - every { mockedSettingsManager.getToken() } returnsMany listOf(token, token + 1, token + 1) + every { mockedSettingsManager.token } returnsMany listOf(token, token + 1, token + 1) // write metadata (needed for backup to be recognized) backend.save(LegacyAppBackupFile.Metadata(token)) @@ -117,7 +117,7 @@ class PluginTest : KoinComponent { @Test fun testMetadataWriteRead() = runBlocking(Dispatchers.IO) { - every { mockedSettingsManager.getToken() } returns token + every { mockedSettingsManager.token } returns token // write metadata val metadata = getRandomByteArray() @@ -201,7 +201,7 @@ class PluginTest : KoinComponent { } private fun initStorage(token: Long) = runBlocking { - every { mockedSettingsManager.getToken() } returns token + every { mockedSettingsManager.token } returns token } } diff --git a/app/src/androidTest/java/com/stevesoltys/seedvault/e2e/LargeTestBase.kt b/app/src/androidTest/java/com/stevesoltys/seedvault/e2e/LargeTestBase.kt index ea648ddc..809834e9 100644 --- a/app/src/androidTest/java/com/stevesoltys/seedvault/e2e/LargeTestBase.kt +++ b/app/src/androidTest/java/com/stevesoltys/seedvault/e2e/LargeTestBase.kt @@ -85,7 +85,7 @@ internal interface LargeTestBase : KoinComponent { fun resetApplicationState() { backupManager.setAutoRestore(false) - settingsManager.setNewToken(null) + settingsManager.token = null val sharedPreferences = permitDiskReads { PreferenceManager.getDefaultSharedPreferences(targetContext) diff --git a/app/src/main/java/com/stevesoltys/seedvault/App.kt b/app/src/main/java/com/stevesoltys/seedvault/App.kt index afc5bc74..8112fdeb 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/App.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/App.kt @@ -125,9 +125,6 @@ open class App : Application() { .build() ) } - permitDiskReads { - migrateTokenFromMetadataToSettingsManager() - } if (!isTest) migrateToOwnScheduling() } @@ -153,25 +150,10 @@ open class App : Application() { ) private val settingsManager: SettingsManager by inject() - private val metadataManager: MetadataManager by inject() private val backupManager: IBackupManager by inject() private val backendManager: BackendManager by inject() private val backupStateManager: BackupStateManager by inject() - /** - * The responsibility for the current token was moved to the [SettingsManager] - * in the end of 2020. - * This method migrates the token for existing installs and can be removed - * after sufficient time has passed. - */ - private fun migrateTokenFromMetadataToSettingsManager() { - @Suppress("DEPRECATION") - val token = metadataManager.getBackupToken() - if (token != 0L && settingsManager.getToken() == null) { - settingsManager.setNewToken(token) - } - } - /** * Disables the framework scheduling in favor of our own. * Introduced in the first half of 2024 and can be removed after a suitable migration period. 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 ca5dcdfb..c3e66416 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataManager.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataManager.kt @@ -8,9 +8,6 @@ package com.stevesoltys.seedvault.metadata import android.content.Context import android.content.Context.MODE_PRIVATE import android.content.pm.PackageInfo -import android.content.pm.PackageManager.PERMISSION_GRANTED -import android.os.Build -import android.os.UserManager import android.util.Log import androidx.annotation.VisibleForTesting import androidx.annotation.WorkerThread @@ -18,8 +15,6 @@ import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData import androidx.lifecycle.distinctUntilChanged import com.stevesoltys.seedvault.Clock -import com.stevesoltys.seedvault.crypto.Crypto -import com.stevesoltys.seedvault.encodeBase64 import com.stevesoltys.seedvault.header.VERSION import com.stevesoltys.seedvault.metadata.PackageState.APK_AND_DATA import com.stevesoltys.seedvault.settings.SettingsManager @@ -39,14 +34,13 @@ internal const val METADATA_SALT_SIZE = 32 internal class MetadataManager( private val context: Context, private val clock: Clock, - private val crypto: Crypto, private val metadataWriter: MetadataWriter, private val metadataReader: MetadataReader, private val packageService: PackageService, private val settingsManager: SettingsManager, ) { - private val uninitializedMetadata = BackupMetadata(token = 0L, salt = "") + private val uninitializedMetadata = BackupMetadata(token = -42L, salt = "foo bar") private var metadata: BackupMetadata = uninitializedMetadata get() { if (field == uninitializedMetadata) { @@ -65,37 +59,10 @@ internal class MetadataManager( return field } - val backupSize: Long get() = metadata.size - private val launchableSystemApps by lazy { packageService.launchableSystemApps.map { it.activityInfo.packageName }.toSet() } - /** - * Call this when initializing a new device. - * - * 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) { - val salt = crypto.getRandomBytes(METADATA_SALT_SIZE).encodeBase64() - modifyCachedMetadata { - val userName = getUserName() - metadata = BackupMetadata( - token = token, - salt = salt, - deviceName = if (userName == null) { - "${Build.MANUFACTURER} ${Build.MODEL}" - } else { - "${Build.MANUFACTURER} ${Build.MODEL} - $userName" - }, - ) - } - } - /** * Call this after a package's APK has been backed up successfully. * @@ -253,18 +220,6 @@ internal class MetadataManager( mLastBackupTime.postValue(metadata.time) // TODO only do after snapshot was written } - /** - * Returns the current backup token. - * - * If the token is 0L, it is not yet initialized and must not be used for anything. - */ - @Synchronized - @Deprecated( - "Responsibility for current token moved to SettingsManager", - ReplaceWith("settingsManager.getToken()") - ) - fun getBackupToken(): Long = metadata.token - /** * Returns the last backup time in unix epoch milli seconds. * @@ -317,12 +272,4 @@ internal class MetadataManager( } } - private fun getUserName(): String? { - val perm = "android.permission.QUERY_USERS" - return if (context.checkSelfPermission(perm) == PERMISSION_GRANTED) { - val userManager = context.getSystemService(UserManager::class.java) ?: return null - userManager.userName - } else null - } - } diff --git a/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataModule.kt b/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataModule.kt index b5eaee76..b0a10173 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataModule.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataModule.kt @@ -9,7 +9,7 @@ import org.koin.android.ext.koin.androidContext import org.koin.dsl.module val metadataModule = module { - single { MetadataManager(androidContext(), get(), get(), get(), get(), get(), get()) } + single { MetadataManager(androidContext(), get(), get(), get(), get(), get()) } single { MetadataWriterImpl(get()) } single { MetadataReaderImpl(get()) } } diff --git a/app/src/main/java/com/stevesoltys/seedvault/restore/AppDataRestoreManager.kt b/app/src/main/java/com/stevesoltys/seedvault/restore/AppDataRestoreManager.kt index d47bee2d..33c8333f 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/restore/AppDataRestoreManager.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/restore/AppDataRestoreManager.kt @@ -85,12 +85,6 @@ internal class AppDataRestoreManager( Log.d(TAG, "Starting new restore session to restore backup $token") - // if we had no token before (i.e. restore from setup wizard), - // use the token of the current restore set from now on - if (settingsManager.getToken() == null) { - settingsManager.setNewToken(token) - } - // start a new restore session val session = try { getOrStartSession() diff --git a/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsManager.kt b/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsManager.kt index baac5c8d..edcba386 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsManager.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsManager.kt @@ -13,7 +13,6 @@ import androidx.annotation.UiThread import androidx.preference.PreferenceManager import com.stevesoltys.seedvault.backend.webdav.WebDavHandler.Companion.createWebDavProperties import com.stevesoltys.seedvault.permitDiskReads -import com.stevesoltys.seedvault.transport.backup.BackupCoordinator import org.calyxos.seedvault.core.backends.Backend import org.calyxos.seedvault.core.backends.saf.SafBackend import org.calyxos.seedvault.core.backends.saf.SafProperties @@ -63,9 +62,6 @@ class SettingsManager(private val context: Context) { PreferenceManager.getDefaultSharedPreferences(context) } - @Volatile - private var token: Long? = null - fun registerOnSharedPreferenceChangeListener(listener: OnSharedPreferenceChangeListener) { prefs.registerOnSharedPreferenceChangeListener(listener) } @@ -83,29 +79,26 @@ class SettingsManager(private val context: Context) { ConcurrentSkipListSet(prefs.getStringSet(PREF_KEY_BACKUP_APP_BLACKLIST, emptySet())) } - fun getToken(): Long? = token ?: run { - val value = prefs.getLong(PREF_KEY_TOKEN, 0L) - if (value == 0L) null else value - } - - /** - * Sets a new RestoreSet token. - * Should only be called by the [BackupCoordinator] - * to ensure that related work is performed after moving to a new token. - */ - fun setNewToken(newToken: Long?) { - if (newToken == null) { - prefs.edit() - .remove(PREF_KEY_TOKEN) - .apply() - } else { - prefs.edit() - .putLong(PREF_KEY_TOKEN, newToken) - .apply() + @Volatile + var token: Long? = null + set(newToken) { + if (newToken == null) { + prefs.edit() + .remove(PREF_KEY_TOKEN) + .apply() + } else { + prefs.edit() + .putLong(PREF_KEY_TOKEN, newToken) + .apply() + } + field = newToken + } + // we may be able to get this from latest snapshot, + // but that is not always readily available + get() = field ?: run { + val value = prefs.getLong(PREF_KEY_TOKEN, 0L) + if (value == 0L) null else value } - - token = newToken - } internal val storagePluginType: StoragePluginType? get() { 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 b5a6d5e6..e8df2afa 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 @@ -15,7 +15,6 @@ import android.app.backup.BackupTransport.TRANSPORT_NOT_INITIALIZED import android.app.backup.BackupTransport.TRANSPORT_OK import android.app.backup.BackupTransport.TRANSPORT_PACKAGE_REJECTED import android.app.backup.BackupTransport.TRANSPORT_QUOTA_EXCEEDED -import android.app.backup.RestoreSet import android.content.Context import android.content.pm.PackageInfo import android.os.ParcelFileDescriptor @@ -84,22 +83,6 @@ internal class BackupCoordinator( // Transport initialization and quota // - /** - * Starts a new [RestoreSet] with a new token (the current unix epoch in milliseconds). - * Call this at least once before calling [initializeDevice] - * which must be called after this method to properly initialize the backup transport. - * - * @return the token of the new [RestoreSet]. - */ - @Throws(IOException::class) - private suspend fun startNewRestoreSet() { - val token = clock.time() - Log.i(TAG, "Starting new RestoreSet with token $token...") - settingsManager.setNewToken(token) - Log.d(TAG, "Resetting backup metadata...") - metadataManager.onDeviceInitialization(token) - } - /** * Initialize the storage for this device, erasing all stored data. * The transport may send the request immediately, or may buffer it. @@ -118,20 +101,15 @@ internal class BackupCoordinator( * @return One of [TRANSPORT_OK] (OK so far) or * [TRANSPORT_ERROR] (to retry following network error or other failure). */ - suspend fun initializeDevice(): Int = try { + fun initializeDevice(): Int { // we don't respect the intended system behavior here by always starting a new [RestoreSet] // instead of simply deleting the current one - startNewRestoreSet() Log.i(TAG, "Initialize Device!") + // [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: Exception) { - Log.e(TAG, "Error initializing device", e) - // Show error notification if we needed init or were ready for backups - if (metadataManager.requiresInit || backendManager.canDoBackupNow()) nm.onBackupError() - TRANSPORT_ERROR + return TRANSPORT_OK } fun isAppEligibleForBackup( diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinator.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinator.kt index 5de3d7d1..a6b29bff 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinator.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinator.kt @@ -162,10 +162,9 @@ internal class RestoreCoordinator( * or 0 if there is no backup set available corresponding to the current device state. */ fun getCurrentRestoreSet(): Long { - Log.d(TAG, "getCurrentRestoreSet() = ") // TODO where to store current token? - return (settingsManager.getToken() ?: 0L).apply { - Log.i(TAG, "Got current restore set token: $this") - } + val token = settingsManager.token ?: 0L + Log.d(TAG, "getCurrentRestoreSet() = $token") + return token } /** 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 0ebeb6a1..2a1526d5 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt @@ -14,7 +14,6 @@ import android.content.pm.ApplicationInfo.FLAG_SYSTEM import android.content.pm.PackageInfo import android.content.pm.PackageManager import android.content.pm.ResolveInfo -import android.os.UserManager import androidx.test.ext.junit.runners.AndroidJUnit4 import com.stevesoltys.seedvault.Clock import com.stevesoltys.seedvault.TestApp @@ -72,7 +71,6 @@ class MetadataManagerTest { private val manager = MetadataManager( context = context, clock = clock, - crypto = crypto, metadataWriter = metadataWriter, metadataReader = metadataReader, packageService = packageService, @@ -106,59 +104,6 @@ class MetadataManagerTest { stopKoin() } - @Test - fun `test onDeviceInitialization() without user permission`() { - every { clock.time() } returns time - every { crypto.getRandomBytes(METADATA_SALT_SIZE) } returns saltBytes - expectReadFromCache() - expectModifyMetadata(initialMetadata) - - every { - context.checkSelfPermission("android.permission.QUERY_USERS") - } returns PackageManager.PERMISSION_DENIED - - manager.onDeviceInitialization(token) - - assertEquals(token, manager.getBackupToken()) - assertEquals(0L, manager.getLastBackupTime()) - - verify { - cacheInputStream.close() - cacheOutputStream.close() - } - } - - @Test - fun `test onDeviceInitialization() with user permission`() { - val userManager: UserManager = mockk() - val userName = getRandomString() - val newMetadata = initialMetadata.copy( - deviceName = initialMetadata.deviceName + " - $userName", - ) - - every { clock.time() } returns time - every { crypto.getRandomBytes(METADATA_SALT_SIZE) } returns saltBytes - expectReadFromCache() - expectModifyMetadata(newMetadata) - - every { - context.checkSelfPermission("android.permission.QUERY_USERS") - } returns PackageManager.PERMISSION_GRANTED - every { context.getSystemService(UserManager::class.java) } returns userManager - every { userManager.userName } returns userName - - manager.onDeviceInitialization(token) - - assertEquals(token, manager.getBackupToken()) - assertEquals(0L, manager.getLastBackupTime()) - - verify { - cacheInputStream.close() - cacheOutputStream.close() - userManager.userName - } - } - @Test fun `test onApkBackedUp() with no prior package metadata`() { val packageMetadata = PackageMetadata( @@ -503,7 +448,6 @@ class MetadataManagerTest { expectReadFromCache() assertEquals(initialMetadata.time, manager.getLastBackupTime()) - assertEquals(initialMetadata.token, manager.getBackupToken()) verify { cacheInputStream.close() } } 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 a0b53a04..35bf022d 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 @@ -5,7 +5,6 @@ package com.stevesoltys.seedvault.transport.backup -import android.app.backup.BackupTransport.TRANSPORT_ERROR import android.app.backup.BackupTransport.TRANSPORT_NOT_INITIALIZED import android.app.backup.BackupTransport.TRANSPORT_OK import android.app.backup.BackupTransport.TRANSPORT_PACKAGE_REJECTED @@ -15,7 +14,6 @@ import android.net.Uri import android.os.ParcelFileDescriptor import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER import com.stevesoltys.seedvault.backend.BackendManager -import com.stevesoltys.seedvault.coAssertThrows import com.stevesoltys.seedvault.getRandomString import com.stevesoltys.seedvault.metadata.BackupType import com.stevesoltys.seedvault.metadata.PackageMetadata @@ -78,7 +76,6 @@ internal class BackupCoordinatorTest : BackupTest() { @Test fun `device initialization succeeds and delegates to plugin`() = runBlocking { - expectStartNewRestoreSet() every { kv.hasState } returns false every { full.hasState } returns false @@ -86,52 +83,6 @@ internal class BackupCoordinatorTest : BackupTest() { assertEquals(TRANSPORT_OK, backup.finishBackup()) } - private fun expectStartNewRestoreSet() { - every { clock.time() } returns token - every { settingsManager.setNewToken(token) } just Runs - every { metadataManager.onDeviceInitialization(token) } just Runs - } - - @Test - fun `error notification when device initialization fails`() = runBlocking { - val maybeTrue = Random.nextBoolean() - - every { clock.time() } returns token - every { settingsManager.setNewToken(token) } just Runs - every { metadataManager.onDeviceInitialization(token) } throws IOException() - every { metadataManager.requiresInit } returns maybeTrue - every { backendManager.canDoBackupNow() } returns !maybeTrue - every { notificationManager.onBackupError() } just Runs - - assertEquals(TRANSPORT_ERROR, backup.initializeDevice()) - - // finish will only be called when TRANSPORT_OK is returned, so it should throw - every { kv.hasState } returns false - every { full.hasState } returns false - coAssertThrows(IllegalStateException::class.java) { - backup.finishBackup() - } - } - - @Test - fun `no error notification when device initialization fails when no backup possible`() = - runBlocking { - every { clock.time() } returns token - every { settingsManager.setNewToken(token) } just Runs - every { metadataManager.onDeviceInitialization(token) } throws IOException() - every { metadataManager.requiresInit } returns false - every { backendManager.canDoBackupNow() } returns false - - assertEquals(TRANSPORT_ERROR, backup.initializeDevice()) - - // finish will only be called when TRANSPORT_OK is returned, so it should throw - every { kv.hasState } returns false - every { full.hasState } returns false - coAssertThrows(IllegalStateException::class.java) { - backup.finishBackup() - } - } - @Test fun `performIncrementalBackup of @pm@ causes re-init when legacy format`() = runBlocking { val packageInfo = PackageInfo().apply { packageName = MAGIC_PACKAGE_MANAGER } @@ -139,11 +90,6 @@ internal class BackupCoordinatorTest : BackupTest() { every { backendManager.canDoBackupNow() } returns true every { metadataManager.requiresInit } returns true - // start new restore set - every { clock.time() } returns token + 1 - every { settingsManager.setNewToken(token + 1) } just Runs - every { metadataManager.onDeviceInitialization(token + 1) } just Runs - every { data.close() } just Runs // returns TRANSPORT_NOT_INITIALIZED to re-init next time @@ -203,7 +149,6 @@ internal class BackupCoordinatorTest : BackupTest() { every { kv.currentPackageInfo } returns packageInfo coEvery { kv.finishBackup() } throws IOException() - every { settingsManager.getToken() } returns token every { metadataManager.onPackageBackupError( packageInfo, @@ -342,7 +287,6 @@ internal class BackupCoordinatorTest : BackupTest() { private fun expectApkBackupAndMetadataWrite() { coEvery { apkBackup.backupApkIfNecessary(packageInfo) } just Runs - every { settingsManager.getToken() } returns token every { metadataManager.onApkBackedUp(any(), packageMetadata) } just Runs } diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinatorTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinatorTest.kt index e0d1d3a6..a7a5a091 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinatorTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/transport/restore/RestoreCoordinatorTest.kt @@ -135,7 +135,7 @@ internal class RestoreCoordinatorTest : TransportTest() { @Test fun `getCurrentRestoreSet() delegates to plugin`() { - every { settingsManager.getToken() } returns token + every { settingsManager.token } returns token assertEquals(token, restore.getCurrentRestoreSet()) }