From a790dc4403ff8dc10ff0f97b766fdd42eeb81cfd Mon Sep 17 00:00:00 2001 From: Cody Henthorne Date: Wed, 19 Aug 2026 13:24:43 -0400 Subject: [PATCH] Ensure successful storage sync of profile key. --- .../securesms/jobs/RotateProfileKeyJob.java | 11 ++- .../securesms/jobs/StorageForcePushJob.kt | 1 + .../securesms/jobs/StorageSyncJob.kt | 2 + .../securesms/keyvalue/AccountValues.kt | 4 + .../data/RegistrationRepository.kt | 1 + .../v2/AppRegistrationStorageController.kt | 1 + .../storage/AccountRecordProcessor.kt | 7 +- .../securesms/storage/StorageSyncHelper.kt | 13 +++ .../securesms/jobs/RotateProfileKeyJobTest.kt | 64 ++++++++++++++ .../securesms/jobs/StorageSyncJobTest.kt | 88 +++++++++++++++++++ .../testutil/FakeStorageServiceRule.kt | 4 + 11 files changed, 193 insertions(+), 3 deletions(-) create mode 100644 app/src/test/java/org/thoughtcrime/securesms/jobs/RotateProfileKeyJobTest.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/RotateProfileKeyJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/RotateProfileKeyJob.java index 99e2182448..a0ccb3acf6 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/RotateProfileKeyJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/RotateProfileKeyJob.java @@ -3,15 +3,18 @@ package org.thoughtcrime.securesms.jobs; import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import org.signal.core.util.logging.Log; import org.signal.libsignal.zkgroup.profiles.ProfileKey; import org.thoughtcrime.securesms.crypto.ProfileKeyUtil; import org.thoughtcrime.securesms.database.SignalDatabase; -import org.thoughtcrime.securesms.jobmanager.JsonJobData; import org.thoughtcrime.securesms.jobmanager.Job; +import org.thoughtcrime.securesms.keyvalue.SignalStore; import org.thoughtcrime.securesms.recipients.Recipient; public class RotateProfileKeyJob extends BaseJob { + private static final String TAG = Log.tag(RotateProfileKeyJob.class); + public static String KEY = "RotateProfileKeyJob"; public RotateProfileKeyJob() { @@ -37,9 +40,15 @@ public class RotateProfileKeyJob extends BaseJob { @Override public void onRun() { + if (SignalStore.account().isLinkedDevice()) { + Log.i(TAG, "Linked device, skipping."); + return; + } + ProfileKey newProfileKey = ProfileKeyUtil.createNew(); Recipient self = Recipient.self(); + SignalStore.account().setNotSyncedRotatedSelfProfileKey(newProfileKey.serialize()); SignalDatabase.recipients().setProfileKey(self.getId(), newProfileKey); } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageForcePushJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageForcePushJob.kt index 831d389d2d..053f5feac9 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageForcePushJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageForcePushJob.kt @@ -167,6 +167,7 @@ class StorageForcePushJob private constructor(parameters: Parameters) : BaseJob( Log.i(TAG, "Force push succeeded. Updating local manifest version to: $newVersion") SignalStore.storageService.manifest = manifest SignalStore.svr.masterKeyForInitialDataRestore = null + StorageSyncHelper.clearRotatedProfileKeyIfSynced(inserts) SignalDatabase.recipients.applyStorageIdUpdates(newContactStorageIds) SignalDatabase.recipients.applyStorageIdUpdates(Collections.singletonMap(Recipient.self().id, accountRecord.id)) SignalDatabase.chatFolders.applyStorageIdUpdates(newChatFolderStorageIds) 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 18ed411b20..1b4c376f64 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.kt @@ -502,6 +502,8 @@ class StorageSyncJob private constructor(parameters: Parameters, private var loc SignalStore.storageService.manifest = remoteWriteOperation.manifest SignalStore.svr.masterKeyForInitialDataRestore = null + StorageSyncHelper.clearRotatedProfileKeyIfSynced(remoteWriteOperation.inserts) + stopwatch.split("remote-write") needsMultiDeviceSync = true diff --git a/app/src/main/java/org/thoughtcrime/securesms/keyvalue/AccountValues.kt b/app/src/main/java/org/thoughtcrime/securesms/keyvalue/AccountValues.kt index 6d58178b8d..bf52db5494 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/keyvalue/AccountValues.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/keyvalue/AccountValues.kt @@ -87,6 +87,7 @@ class AccountValues internal constructor(store: KeyValueStore, context: Context) private const val KEY_HAS_LINKED_DEVICES = "account.has_linked_devices" private const val KEY_HAS_INACTIVE_PRIMARY_DEVICE_ALERT = "account.has_inactive_primary_device_alert" + private const val KEY_NOT_SYNCED_ROTATED_SELF_PROFILE_KEY = "account.not_synced_rotated_self_profile_key" private const val KEY_VERIFICATION_CODE_REQUESTED_AT = "account.verification_code_requested_at" @@ -608,6 +609,9 @@ class AccountValues internal constructor(store: KeyValueStore, context: Context) @get:JvmName("isMultiDevice") var isMultiDevice by booleanValue(KEY_HAS_LINKED_DEVICES, false) + /** Our own profile key that is still pending being written to storage service. */ + var notSyncedRotatedSelfProfileKey: ByteArray? by nullableBlobValue(KEY_NOT_SYNCED_ROTATED_SELF_PROFILE_KEY, null) + /** Server has indicated a verification code was requested for the account at this timestamp (ms since epoch) */ private val verificationCodeRequestedAtMsValue = longValue(KEY_VERIFICATION_CODE_REQUESTED_AT, 0) var verificationCodeRequestedAtMs: Long by verificationCodeRequestedAtMsValue diff --git a/app/src/main/java/org/thoughtcrime/securesms/registration/data/RegistrationRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/registration/data/RegistrationRepository.kt index 38bc8cac57..b402ad55ae 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/registration/data/RegistrationRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/registration/data/RegistrationRepository.kt @@ -213,6 +213,7 @@ object RegistrationRepository { recipientTable.markRegisteredOrThrow(selfId, aci) recipientTable.linkIdsForSelf(aci, pni, data.e164) recipientTable.setProfileKey(selfId, ProfileKey(data.profileKey.toByteArray())) + SignalStore.account.notSyncedRotatedSelfProfileKey = null AppDependencies.recipientCache.clearSelf() 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 467d4c1cc2..8c2d544a20 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 @@ -792,6 +792,7 @@ class AppRegistrationStorageController(private val context: Context) : StorageCo recipientTable.markRegisteredOrThrow(selfId, aci) recipientTable.linkIdsForSelf(aci, pni, e164) recipientTable.setProfileKey(selfId, profileKey) + SignalStore.account.notSyncedRotatedSelfProfileKey = null AppDependencies.recipientCache.clearSelf() diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/AccountRecordProcessor.kt b/app/src/main/java/org/thoughtcrime/securesms/storage/AccountRecordProcessor.kt index bba162ae51..03bb9204c8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/AccountRecordProcessor.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/AccountRecordProcessor.kt @@ -108,11 +108,14 @@ class AccountRecordProcessor( val unknownFields = remote.serializedUnknowns + val unpublishedRotation = SignalStore.account.notSyncedRotatedSelfProfileKey + val keepLocalProfileKey = unpublishedRotation != null && local.proto.profileKey.toByteArray().contentEquals(unpublishedRotation) + val merged = SignalAccountRecord.newBuilder(unknownFields).apply { givenName = mergedGivenName familyName = mergedFamilyName - avatarUrlPath = remote.proto.avatarUrlPath.nullIfEmpty() ?: local.proto.avatarUrlPath - profileKey = remote.proto.profileKey.nullIfEmpty() ?: local.proto.profileKey + avatarUrlPath = if (keepLocalProfileKey) local.proto.avatarUrlPath else remote.proto.avatarUrlPath.nullIfEmpty() ?: local.proto.avatarUrlPath + profileKey = if (keepLocalProfileKey) local.proto.profileKey else remote.proto.profileKey.nullIfEmpty() ?: local.proto.profileKey noteToSelfArchived = remote.proto.noteToSelfArchived noteToSelfMarkedUnread = remote.proto.noteToSelfMarkedUnread readReceipts = remote.proto.readReceipts diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.kt b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.kt index e241f44600..2fdbed2cdc 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.kt @@ -115,6 +115,19 @@ object StorageSyncHelper { return update.old.proto.profileKey != update.new.proto.profileKey } + /** + * Iff successfully written records carried the expected rotation, clears the content. + */ + @JvmStatic + fun clearRotatedProfileKeyIfSynced(written: List) { + val rotated = SignalStore.account.notSyncedRotatedSelfProfileKey ?: return + + if (written.any { it.proto.account?.profileKey?.toByteArray().contentEquals(rotated) }) { + Log.i(TAG, "Published our rotated profile key.") + SignalStore.account.notSyncedRotatedSelfProfileKey = null + } + } + @JvmStatic fun buildAccountRecord(context: Context, self: Recipient): SignalStorageRecord { var self = self diff --git a/app/src/test/java/org/thoughtcrime/securesms/jobs/RotateProfileKeyJobTest.kt b/app/src/test/java/org/thoughtcrime/securesms/jobs/RotateProfileKeyJobTest.kt new file mode 100644 index 0000000000..0f8922f1c8 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/jobs/RotateProfileKeyJobTest.kt @@ -0,0 +1,64 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.jobs + +import android.app.Application +import io.mockk.every +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import org.thoughtcrime.securesms.database.SignalDatabase +import org.thoughtcrime.securesms.testutil.RecipientTestRule + +@RunWith(RobolectricTestRunner::class) +@Config(manifest = Config.NONE, application = Application::class) +class RotateProfileKeyJobTest { + + @get:Rule + val recipients = RecipientTestRule() + + private var unpublishedRotation: ByteArray? = null + + @Before + fun setUp() { + every { recipients.signalStore.account.notSyncedRotatedSelfProfileKey } answers { unpublishedRotation } + every { recipients.signalStore.account.notSyncedRotatedSelfProfileKey = any() } answers { unpublishedRotation = firstArg() } + } + + @Test + fun `given I am the primary, when I run, then I rotate my profile key and record it as unpublished`() { + val before = selfProfileKey() + + RotateProfileKeyJob().run() + + assertFalse(selfProfileKey().contentEquals(before)) + assertArrayEquals(selfProfileKey(), unpublishedRotation) + } + + @Test + fun `given I am a linked device, when I run, then I leave my profile key alone`() { + every { recipients.signalStore.account.isLinkedDevice } returns true + every { recipients.signalStore.account.isPrimaryDevice } returns false + + val before = selfProfileKey() + + RotateProfileKeyJob().run() + + assertArrayEquals(before, selfProfileKey()) + assertNull(unpublishedRotation) + } + + private fun selfProfileKey(): ByteArray { + return SignalDatabase.recipients.getRecord(recipients.self).profileKey.also { assertNotNull(it) }!! + } +} 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 293d414652..24c18fc5ce 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/jobs/StorageSyncJobTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/jobs/StorageSyncJobTest.kt @@ -34,11 +34,13 @@ 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.signal.libsignal.zkgroup.profiles.ProfileKey import org.thoughtcrime.securesms.database.IssueReporter import org.thoughtcrime.securesms.database.RecipientTable import org.thoughtcrime.securesms.database.SignalDatabase import org.thoughtcrime.securesms.database.model.IssuePriority import org.thoughtcrime.securesms.database.model.StickerPackId +import org.thoughtcrime.securesms.dependencies.AppDependencies import org.thoughtcrime.securesms.groups.GroupId import org.thoughtcrime.securesms.jobmanager.Job import org.thoughtcrime.securesms.jobs.StorageSyncJobTest.Companion.BASE_MANIFEST_VERSION @@ -446,6 +448,76 @@ class StorageSyncJobTest { } } + @Test + fun `given I just rotated my profile key, when a newer remote manifest still has the old one, then I keep mine`() { + val rotated = rotateSelfProfileKey() + + bumpRemoteManifestWithoutTouchingAccountRecord() + + assertTrue(runJob(StorageSyncJob.forRemoteChange()).isSuccess) + assertArrayEquals(rotated.serialize(), SignalDatabase.recipients.getRecord(recipients.self).profileKey) + } + + @Test + fun `given I just rotated my profile key, when a newer remote manifest still has the old one, then I publish mine`() { + val rotated = rotateSelfProfileKey() + + bumpRemoteManifestWithoutTouchingAccountRecord() + + assertTrue(runJob(StorageSyncJob.forRemoteChange()).isSuccess) + + val accounts = remoteStorage.records.mapNotNull { it.proto.account } + assertEquals(1, accounts.size) + assertArrayEquals(rotated.serialize(), accounts[0].profileKey.toByteArray()) + assertNull(recipients.signalStore.account.notSyncedRotatedSelfProfileKey) + } + + @Test + fun `given no rotation of mine is pending, when remote has a different profile key, then I take theirs`() { + val remoteKey = SignalDatabase.recipients.getRecord(recipients.self).profileKey + + // Registration always leaves us a locally generated key, so this must still defer to remote. + SignalDatabase.recipients.setProfileKey(recipients.self, ProfileKey(Util.getSecretBytes(32))) + Recipient.self().live().refresh() + + bumpRemoteManifestWithoutTouchingAccountRecord() + + assertTrue(runJob(StorageSyncJob.forRemoteChange()).isSuccess) + assertArrayEquals(remoteKey, SignalDatabase.recipients.getRecord(recipients.self).profileKey) + } + + @Test + fun `given a recorded rotation that no longer matches my profile key, when remote has a different one, then I take theirs`() { + recipients.signalStore.account.notSyncedRotatedSelfProfileKey = Util.getSecretBytes(32) + + val remoteKey = SignalDatabase.recipients.getRecord(recipients.self).profileKey + SignalDatabase.recipients.setProfileKey(recipients.self, ProfileKey(Util.getSecretBytes(32))) + Recipient.self().live().refresh() + + bumpRemoteManifestWithoutTouchingAccountRecord() + + assertTrue(runJob(StorageSyncJob.forRemoteChange()).isSuccess) + assertArrayEquals(remoteKey, SignalDatabase.recipients.getRecord(recipients.self).profileKey) + } + + @Test + fun `given I just rotated my profile key, when their account record has an avatar path, then I do not fetch it`() { + SignalDatabase.recipients.setProfileAvatar(recipients.self, "avatar-path-for-the-old-key") + Recipient.self().live().refresh() + check(runJob(StorageSyncJob.forLocalChange()).isSuccess) + check(remoteStorage.records.mapNotNull { it.proto.account }.single().avatarUrlPath == "avatar-path-for-the-old-key") + + SignalDatabase.recipients.setProfileAvatar(recipients.self, null) + rotateSelfProfileKey() + + bumpRemoteManifestWithoutTouchingAccountRecord() + + val jobManager = AppDependencies.jobManager + + assertTrue(runJob(StorageSyncJob.forRemoteChange()).isSuccess) + verify(exactly = 0) { jobManager.add(ofType(RetrieveProfileAvatarJob::class)) } + } + /** * Writes the same contact, with the other device deleting it again after each, until a run gets throttled. Returns * that run, leaving [FakeStorageServiceRule.writeCount] counting only it. Driven by observation rather than a fixed @@ -581,4 +653,20 @@ class StorageSyncJobTest { job.setContext(ApplicationProvider.getApplicationContext()) return job.run() } + + /** Rotates our own profile key the way [RotateProfileKeyJob] does, and returns the new key. */ + private fun rotateSelfProfileKey(): ProfileKey { + val rotated = ProfileKey(Util.getSecretBytes(32)) + + recipients.signalStore.account.notSyncedRotatedSelfProfileKey = rotated.serialize() + SignalDatabase.recipients.setProfileKey(recipients.self, rotated) + Recipient.self().live().refresh() + + return rotated + } + + /** Mimics another device writing an unrelated record, which bumps the manifest but leaves our account record stale. */ + private fun bumpRemoteManifestWithoutTouchingAccountRecord() { + remoteStorage.addRemoteRecords(listOf(contactRecord(ACI.from(UUID.randomUUID()), ProfileName.fromParts("Remote", "Contact")))) + } } diff --git a/app/src/test/java/org/thoughtcrime/securesms/testutil/FakeStorageServiceRule.kt b/app/src/test/java/org/thoughtcrime/securesms/testutil/FakeStorageServiceRule.kt index 3391c1cbfe..42e3504c5a 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/testutil/FakeStorageServiceRule.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/testutil/FakeStorageServiceRule.kt @@ -110,6 +110,7 @@ class FakeStorageServiceRule(val storageKey: StorageKey = StorageKey(Util.getSec fun stubDefaults(store: MockSignalStoreRule) { var localManifest = SignalStorageManifest.EMPTY var syncLoopState = StorageSyncLoopState() + var notSyncedRotatedSelfProfileKey: ByteArray? = null every { store.storageService.manifest } answers { localManifest } every { store.storageService.manifest = any() } answers { localManifest = firstArg() } @@ -122,6 +123,9 @@ class FakeStorageServiceRule(val storageKey: StorageKey = StorageKey(Util.getSec every { store.svr.hasPin() } returns true every { store.svr.hasOptedOut() } returns false + every { store.account.notSyncedRotatedSelfProfileKey } answers { notSyncedRotatedSelfProfileKey } + every { store.account.notSyncedRotatedSelfProfileKey = any() } answers { notSyncedRotatedSelfProfileKey = firstArg() } + every { store.account.isRegistered } returns true every { store.account.isPrimaryDevice } returns true every { store.account.isLinkedDevice } returns false