diff --git a/app/build.gradle b/app/build.gradle index ed5cecfb82..f32c908a21 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -190,6 +190,16 @@ android { execution 'ANDROIDX_TEST_ORCHESTRATOR' } + sourceSets { + test { + java.srcDirs += "$projectDir/src/testShared" + } + + androidTest { + java.srcDirs += "$projectDir/src/testShared" + } + } + compileOptions { coreLibraryDesugaringEnabled true sourceCompatibility JAVA_VERSION diff --git a/app/src/androidTest/java/org/thoughtcrime/securesms/database/RecipientDatabaseTest.kt b/app/src/androidTest/java/org/thoughtcrime/securesms/database/RecipientDatabaseTest.kt index 82a19a3afe..d16c828bca 100644 --- a/app/src/androidTest/java/org/thoughtcrime/securesms/database/RecipientDatabaseTest.kt +++ b/app/src/androidTest/java/org/thoughtcrime/securesms/database/RecipientDatabaseTest.kt @@ -8,6 +8,11 @@ import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import org.thoughtcrime.securesms.keyvalue.AccountValues +import org.thoughtcrime.securesms.keyvalue.KeyValueDataSet +import org.thoughtcrime.securesms.keyvalue.KeyValueStore +import org.thoughtcrime.securesms.keyvalue.MockKeyValuePersistentStorage +import org.thoughtcrime.securesms.keyvalue.SignalStore import org.thoughtcrime.securesms.recipients.Recipient import org.thoughtcrime.securesms.recipients.RecipientId import org.whispersystems.libsignal.util.guava.Optional @@ -214,6 +219,29 @@ class RecipientDatabaseTest { assertEquals(E164_A, existingRecipient.requireE164()) } + /** We never want to remove the e164 of our own contact entry. So basically treat this as a low-trust case, and leave the e164 alone. */ + @Test + fun getAndPossiblyMerge_e164MapsToExistingUserButAciDoesNot_e164BelongsToLocalUser_highTrust() { + val dataSet = KeyValueDataSet().apply { + putString(AccountValues.KEY_E164, E164_A) + putString(AccountValues.KEY_ACI, ACI_A.toString()) + } + SignalStore.inject(KeyValueStore(MockKeyValuePersistentStorage.withDataSet(dataSet))) + + val existingId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_A, true) + + val retrievedId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_B, E164_A, true) + assertNotEquals(existingId, retrievedId) + + val retrievedRecipient = Recipient.resolved(retrievedId) + assertEquals(ACI_B, retrievedRecipient.requireAci()) + assertFalse(retrievedRecipient.hasE164()) + + val existingRecipient = Recipient.resolved(existingId) + assertEquals(ACI_A, existingRecipient.requireAci()) + assertEquals(E164_A, existingRecipient.requireE164()) + } + // ============================================================== // If both the ACI and E164 map to an existing user // ============================================================== @@ -324,6 +352,30 @@ class RecipientDatabaseTest { assertEquals(retrievedId, recipientWithId2.id) } + /** We never want to remove the e164 of our own contact entry. So basically treat this as a low-trust case, and leave the e164 alone. */ + @Test + fun getAndPossiblyMerge_bothAciAndE164MapToExistingUser_e164BelongsToLocalUser() { + val dataSet = KeyValueDataSet().apply { + putString(AccountValues.KEY_E164, E164_A) + putString(AccountValues.KEY_ACI, ACI_B.toString()) + } + SignalStore.inject(KeyValueStore(MockKeyValuePersistentStorage.withDataSet(dataSet))) + + val existingId1: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_B, E164_A, true) + val existingId2: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, null, true) + + val retrievedId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_A, true) + assertEquals(existingId2, retrievedId) + + val retrievedRecipient = Recipient.resolved(retrievedId) + assertEquals(ACI_A, retrievedRecipient.requireAci()) + assertFalse(retrievedRecipient.hasE164()) + + val recipientWithId1 = Recipient.resolved(existingId1) + assertEquals(ACI_B, recipientWithId1.requireAci()) + assertEquals(E164_A, recipientWithId1.requireE164()) + } + // ============================================================== // Misc // ============================================================== @@ -378,7 +430,7 @@ class RecipientDatabaseTest { val ACI_A = ACI.from(UUID.fromString("3436efbe-5a76-47fa-a98a-7e72c948a82e")) val ACI_B = ACI.from(UUID.fromString("8de7f691-0b60-4a68-9cd9-ed2f8453f9ed")) - val E164_A = "+12221234567" - val E164_B = "+13331234567" + const val E164_A = "+12221234567" + const val E164_B = "+13331234567" } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.kt b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.kt index 6dbff382aa..f5bd8d0c75 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.kt @@ -390,7 +390,7 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : } fun getAndPossiblyMerge(aci: ACI?, e164: String?, highTrust: Boolean, changeSelf: Boolean): RecipientId { - require(!(aci == null && e164 == null)) { "Must provide a UUID or E164!" } + require(!(aci == null && e164 == null)) { "Must provide an ACI or E164!" } var recipientNeedingRefresh: RecipientId? = null var remapped: Pair? = null @@ -417,8 +417,8 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : if (aci != null) { val e164Record: RecipientRecord = getRecord(byE164.get()) if (e164Record.aci != null) { - if (highTrust) { - Log.w(TAG, "Found out about an ACI ($aci) for a known E164 user (${byE164.get()}), but that user already has an ACI (${e164Record.aci}). Likely a case of re-registration. High-trust, so stripping the E164 from the existing account and assigning it to a new entry", true) + if (highTrust && e164Record.aci != SignalStore.account().aci) { + Log.w(TAG, "Found out about an ACI ($aci) for a known E164 user (${byE164.get()}), but that user already has an ACI (${e164Record.aci}). Likely a case of re-registration. High-trust, so stripping the E164 ($e164) from the existing account and assigning it to a new entry.", true) removePhoneNumber(byE164.get(), db) recipientNeedingRefresh = byE164.get() @@ -428,7 +428,11 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : val id = db.insert(TABLE_NAME, null, insertValues) finalId = RecipientId.from(id) } else { - Log.w(TAG, "Found out about an ACI ($aci) for a known E164 user (${byE164.get()}), but that user already has an ACI (${e164Record.aci}). Likely a case of re-registration. Low-trust, so making a new user for the UUID.", true) + if (e164Record.aci == SignalStore.account().aci) { + Log.w(TAG, "Found out about an ACI ($aci) for a known E164 user (${byE164.get()}), but that user is us! Likely a case where we changed our number, but the old owner is sending sealed sender messages. Keeping the E164 ($e164) for ourselves and making a new user for the ACI.", true) + } else { + Log.w(TAG, "Found out about an ACI ($aci) for a known E164 user (${byE164.get()}), but that user already has an ACI (${e164Record.aci}). Likely a case of re-registration. Low-trust, so making a new user for the ACI.", true) + } val id = db.insert(TABLE_NAME, null, buildContentValuesForNewUser(null, aci)) finalId = RecipientId.from(id) } @@ -463,7 +467,7 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : } } } else { - Log.i(TAG, "Found out about an E164 (%s) for a known ACI user (%s). Low-trust, so doing nothing.", true) + Log.i(TAG, "Found out about an E164 ($e164) for a known ACI user (${byAci.get()}). Low-trust, so doing nothing.", true) finalId = byAci.get() } } else { @@ -476,8 +480,8 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : Log.w(TAG, "Hit a conflict between ${byE164.get()} (E164 of $e164) and ${byAci.get()} (ACI $aci). They map to different recipients.", Throwable(), true) val e164Record: RecipientRecord = getRecord(byE164.get()) if (e164Record.aci != null) { - if (highTrust) { - Log.w(TAG, "The E164 contact has a different ACI. Likely a case of re-registration. High-trust, so stripping the E164 from the existing account and assigning it to the ACI entry.", true) + if (highTrust && e164Record.aci != SignalStore.account().aci) { + Log.w(TAG, "The E164 contact (${byE164.get()}) has a different ACI ($aci). Likely a case of re-registration. High-trust, so stripping the E164 ($e164) from the existing account and assigning it to the ACI entry.", true) removePhoneNumber(byE164.get(), db) recipientNeedingRefresh = byE164.get() @@ -489,7 +493,11 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : recipientChangedNumber = finalId } } else { - Log.w(TAG, "The E164 contact has a different ACI. Likely a case of re-registration. Low-trust, so doing nothing.", true) + if (e164Record.aci == SignalStore.account().aci) { + Log.w(TAG, "The E164 contact (${byE164.get()}) has a different ACI ($aci), but the E164 contact is us! Likely a case where we changed our number, but the old owner is sending sealed sender messages. Keeping the E164 ($e164) for ourselves.", true) + } else { + Log.w(TAG, "The E164 contact (${byE164.get()}) has a different ACI ($aci). Likely a case of re-registration. Low-trust, so doing nothing.", true) + } finalId = byAci.get() } } else { 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 5b1e6df4fc..c3d174c6ef 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/keyvalue/AccountValues.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/keyvalue/AccountValues.kt @@ -16,8 +16,6 @@ internal class AccountValues internal constructor(store: KeyValueStore) : Signal companion object { private val TAG = Log.tag(AccountValues::class.java) - private const val KEY_ACI = "account.aci" - private const val KEY_PNI = "account.pni" private const val KEY_SERVICE_PASSWORD = "account.service_password" private const val KEY_IS_REGISTERED = "account.is_registered" private const val KEY_REGISTRATION_ID = "account.registration_id" @@ -28,6 +26,10 @@ internal class AccountValues internal constructor(store: KeyValueStore) : Signal @VisibleForTesting const val KEY_E164 = "account.e164" + @VisibleForTesting + const val KEY_ACI = "account.aci" + @VisibleForTesting + const val KEY_PNI = "account.pni" } init { diff --git a/app/src/test/java/org/thoughtcrime/securesms/keyvalue/MockKeyValuePersistentStorage.java b/app/src/testShared/org/thoughtcrime/securesms/keyvalue/MockKeyValuePersistentStorage.java similarity index 100% rename from app/src/test/java/org/thoughtcrime/securesms/keyvalue/MockKeyValuePersistentStorage.java rename to app/src/testShared/org/thoughtcrime/securesms/keyvalue/MockKeyValuePersistentStorage.java