diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt index 0b0b2f4c27..fe343a84d3 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt @@ -1141,25 +1141,30 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da /** * Removes storageIds from unregistered recipients who were unregistered more than [RemoteConfig.messageQueueTime] ago. + * + * Never touches self: our own storageId backs the ACCOUNT record, so it always needs to be present. If self ever ends up with a stale + * [UNREGISTERED_TIMESTAMP], clearing it here would leave us regenerating our storageId on every single storage sync. + * * @return The number of rows affected. */ fun removeStorageIdsFromOldUnregisteredRecipients(now: Long): Int { return writableDatabase .update(TABLE_NAME) .values(STORAGE_SERVICE_ID to null) - .where("$STORAGE_SERVICE_ID NOT NULL AND $UNREGISTERED_TIMESTAMP > 0 AND $UNREGISTERED_TIMESTAMP < ?", now - RemoteConfig.messageQueueTime) + .where("$STORAGE_SERVICE_ID NOT NULL AND $ID != ${Recipient.self().id.toLong()} AND $UNREGISTERED_TIMESTAMP > 0 AND $UNREGISTERED_TIMESTAMP < ?", now - RemoteConfig.messageQueueTime) .run() } /** - * Removes storageIds from unregistered contacts that have storageIds in the provided collection. + * Removes storageIds from unregistered contacts that have storageIds in the provided collection. Never touches self, for the reasons + * described in [removeStorageIdsFromOldUnregisteredRecipients]. * @return The number of updated rows. */ fun removeStorageIdsFromLocalOnlyUnregisteredRecipients(storageIds: Collection): Int { val values = contentValuesOf(STORAGE_SERVICE_ID to null) var updated = 0 - SqlUtil.buildCollectionQuery(STORAGE_SERVICE_ID, storageIds.map { Base64.encodeWithPadding(it.raw) }, "$UNREGISTERED_TIMESTAMP > 0 AND") + SqlUtil.buildCollectionQuery(STORAGE_SERVICE_ID, storageIds.map { Base64.encodeWithPadding(it.raw) }, "$ID != ${Recipient.self().id.toLong()} AND $UNREGISTERED_TIMESTAMP > 0 AND") .forEach { updated += writableDatabase.update(TABLE_NAME, values, it.where, it.whereArgs) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.kt index 30293df29b..e23a8288be 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.kt @@ -396,8 +396,6 @@ class StorageSyncJob private constructor(parameters: Parameters, private var loc stopwatch.split("known-unknowns") val remoteWriteOperation: WriteOperationResult = db.withinTransaction { - self = freshSelf() - val removedUnregistered = SignalDatabase.recipients.removeStorageIdsFromOldUnregisteredRecipients(System.currentTimeMillis()) val removedDeletedFolders = SignalDatabase.chatFolders.removeStorageIdsFromOldDeletedFolders(System.currentTimeMillis()) val removedDeletedProfiles = SignalDatabase.notificationProfiles.removeStorageIdsFromOldDeletedProfiles(System.currentTimeMillis()) @@ -406,6 +404,14 @@ class StorageSyncJob private constructor(parameters: Parameters, private var loc Log.i(TAG, "Removed $removedUnregistered unregistered, $removedDeletedFolders folders, $removedDeletedProfiles notification profiles, $removedDeletedPacks sticker packs from storage service that have been deleted for longer than ${RemoteConfig.messageQueueTime.milliseconds.inWholeDays} days.") } + self = freshSelf() + + if (self.storageId == null) { + Log.w(TAG, "No storageId for self. Generating.") + SignalDatabase.recipients.updateStorageId(self.id, StorageSyncHelper.generateKey()) + self = freshSelf() + } + var localStorageIds = getAllLocalStorageIds(self) var idDifference = StorageSyncHelper.findIdDifference(remoteManifest.storageIds, localStorageIds) diff --git a/app/src/test/java/org/thoughtcrime/securesms/jobs/StorageSyncJobTest.kt b/app/src/test/java/org/thoughtcrime/securesms/jobs/StorageSyncJobTest.kt index 70b069f536..b041d782e2 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/jobs/StorageSyncJobTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/jobs/StorageSyncJobTest.kt @@ -13,6 +13,7 @@ import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Rule @@ -25,17 +26,21 @@ import org.signal.core.models.ServiceId.ACI import org.signal.core.util.Hex import org.signal.core.util.Util import org.signal.core.util.logging.Log +import org.signal.core.util.update import org.signal.core.util.withinTransaction +import org.thoughtcrime.securesms.database.RecipientTable import org.thoughtcrime.securesms.database.SignalDatabase import org.thoughtcrime.securesms.database.model.StickerPackId import org.thoughtcrime.securesms.groups.GroupId import org.thoughtcrime.securesms.jobmanager.Job import org.thoughtcrime.securesms.profiles.ProfileName import org.thoughtcrime.securesms.recipients.Recipient +import org.thoughtcrime.securesms.recipients.RecipientId import org.thoughtcrime.securesms.storage.StorageSyncHelper import org.thoughtcrime.securesms.testutil.FakeStorageServiceRule import org.thoughtcrime.securesms.testutil.RecipientTestRule import org.thoughtcrime.securesms.testutil.SystemOutLogger +import org.thoughtcrime.securesms.util.RemoteConfig import org.whispersystems.signalservice.api.storage.SignalStorageRecord import org.whispersystems.signalservice.api.storage.StorageId import org.whispersystems.signalservice.internal.storage.protos.ContactRecord @@ -293,6 +298,79 @@ class StorageSyncJobTest { assertFalse(SignalDatabase.unknownStorageIds.allUnknownIds.contains(strandedId)) } + @Test + fun `given self was unregistered long ago, when I run, then I keep our storage id`() { + markUnregisteredLongAgo(recipients.self) + val selfStorageId = storageIdOf(recipients.self)!! + + val result = runJob(StorageSyncJob.forLocalChange()) + + assertTrue(result.isSuccess) + assertArrayEquals(selfStorageId, storageIdOf(recipients.self)) + assertTrue(remoteStorage.manifest!!.storageIds.contains(StorageId.forAccount(selfStorageId))) + } + + @Test + fun `given self was unregistered long ago, when I run again, then I do not rotate our storage id`() { + markUnregisteredLongAgo(recipients.self) + check(runJob(StorageSyncJob.forLocalChange()).isSuccess) + + val selfStorageId = storageIdOf(recipients.self)!! + remoteStorage.resetCounters() + + val result = runJob(StorageSyncJob.forLocalChange()) + + assertTrue(result.isSuccess) + assertArrayEquals(selfStorageId, storageIdOf(recipients.self)) + assertEquals(0, remoteStorage.writeCount) + } + + @Test + fun `given self was unregistered long ago and our storage id is local-only, when I run, then I keep our storage id`() { + markUnregisteredLongAgo(recipients.self) + val selfStorageId = storageIdOf(recipients.self)!! + + remoteStorage.setRemoteState(remoteStorage.records.filter { it.proto.account == null }, version = remoteStorage.manifest!!.version + 1) + + val result = runJob(StorageSyncJob.forRemoteChange()) + + assertTrue(result.isSuccess) + assertArrayEquals(selfStorageId, storageIdOf(recipients.self)) + assertEquals(1, remoteStorage.records.count { it.proto.account != null }) + assertTrue(remoteStorage.manifest!!.storageIds.contains(StorageId.forAccount(selfStorageId))) + } + + @Test + fun `given self has no storage id, when I run, then I generate one and write our account record`() { + clearStorageId(recipients.self) + + val result = runJob(StorageSyncJob.forLocalChange()) + + assertTrue(result.isSuccess) + + val selfStorageId = storageIdOf(recipients.self) + assertNotNull(selfStorageId) + assertEquals(1, remoteStorage.records.count { it.proto.account != null }) + assertTrue(remoteStorage.manifest!!.storageIds.contains(StorageId.forAccount(selfStorageId!!))) + } + + @Test + fun `given a contact was unregistered long ago, when I run, then I remove their storage id`() { + val contact = recipients.createRecipient("Local Contact") + SignalDatabase.recipients.rotateStorageId(contact) + check(runJob(StorageSyncJob.forLocalChange()).isSuccess) + check(remoteStorage.records.count { it.proto.contact != null } == 1) + + markUnregisteredLongAgo(contact) + remoteStorage.resetCounters() + + val result = runJob(StorageSyncJob.forLocalChange()) + + assertTrue(result.isSuccess) + assertNull(storageIdOf(contact)) + assertEquals(0, remoteStorage.records.count { it.proto.contact != null }) + } + /** * Gets us to a steady state: remote holds our account record at version 1, then a sync pushes up everything else * the fresh database came with (the default chat folder), leaving both sides at [BASE_MANIFEST_VERSION]. @@ -315,6 +393,33 @@ class StorageSyncJobTest { return recipients.signalStore.storageService.manifest.version } + private fun storageIdOf(id: RecipientId): ByteArray? { + return SignalDatabase.recipients.getRecord(id).storageId + } + + private fun clearStorageId(id: RecipientId) { + SignalDatabase.writableDatabase + .update(RecipientTable.TABLE_NAME) + .values(RecipientTable.STORAGE_SERVICE_ID to null) + .where("${RecipientTable.ID} = ?", id.toLong()) + .run() + + Recipient.live(id).refresh() + } + + /** Marks [id] unregistered further back than [RemoteConfig.messageQueueTime], making its storageId eligible for cleanup. */ + private fun markUnregisteredLongAgo(id: RecipientId) { + SignalDatabase.recipients.markUnregistered(id) + + SignalDatabase.writableDatabase + .update(RecipientTable.TABLE_NAME) + .values(RecipientTable.UNREGISTERED_TIMESTAMP to System.currentTimeMillis() - RemoteConfig.messageQueueTime - 1) + .where("${RecipientTable.ID} = ?", id.toLong()) + .run() + + Recipient.live(id).refresh() + } + private fun contactRecord(aci: ACI, profileName: ProfileName): SignalStorageRecord { return SignalStorageRecord( id = StorageId.forContact(Util.getSecretBytes(16)),