diff --git a/app/src/androidTest/java/org/thoughtcrime/securesms/database/RecipientDatabaseTest_processPnpTuple.kt b/app/src/androidTest/java/org/thoughtcrime/securesms/database/RecipientDatabaseTest_processPnpTuple.kt index d1f5c98d3..100567a92 100644 --- a/app/src/androidTest/java/org/thoughtcrime/securesms/database/RecipientDatabaseTest_processPnpTuple.kt +++ b/app/src/androidTest/java/org/thoughtcrime/securesms/database/RecipientDatabaseTest_processPnpTuple.kt @@ -12,16 +12,22 @@ import org.signal.core.util.requireLong import org.signal.core.util.requireString import org.signal.core.util.select import org.thoughtcrime.securesms.keyvalue.SignalStore +import org.thoughtcrime.securesms.recipients.Recipient import org.thoughtcrime.securesms.recipients.RecipientId +import org.thoughtcrime.securesms.sms.IncomingEncryptedMessage +import org.thoughtcrime.securesms.sms.IncomingTextMessage import org.whispersystems.signalservice.api.push.ACI import org.whispersystems.signalservice.api.push.PNI import org.whispersystems.signalservice.api.push.ServiceId +import java.util.Optional import java.util.UUID @RunWith(AndroidJUnit4::class) class RecipientDatabaseTest_processPnpTuple { private lateinit var recipientDatabase: RecipientDatabase + private lateinit var smsDatabase: SmsDatabase + private lateinit var threadDatabase: ThreadDatabase private val localAci = ACI.from(UUID.randomUUID()) private val localPni = PNI.from(UUID.randomUUID()) @@ -29,6 +35,8 @@ class RecipientDatabaseTest_processPnpTuple { @Before fun setup() { recipientDatabase = SignalDatabase.recipients + smsDatabase = SignalDatabase.sms + threadDatabase = SignalDatabase.threads ensureDbEmpty() @@ -180,11 +188,12 @@ class RecipientDatabaseTest_processPnpTuple { fun onlyPniMatches_noExistingPniSession_changeNumber() { // This test, I could go either way. We decide to change the E164 on the existing row rather than create a new one. // But it's an "unstable E164->PNI mapping" case, which we don't expect, so as long as there's a user-visible impact that should be fine. - // TODO Verify change number test { - given(E164_B, PNI_A, null) + given(E164_B, PNI_A, null, createThread = true) process(E164_A, PNI_A, ACI_A) expect(E164_A, PNI_A, ACI_A) + + expectChangeNumberEvent() } } @@ -192,21 +201,23 @@ class RecipientDatabaseTest_processPnpTuple { fun pniAndAciMatches_changeNumber() { // This test, I could go either way. We decide to change the E164 on the existing row rather than create a new one. // But it's an "unstable E164->PNI mapping" case, which we don't expect, so as long as there's a user-visible impact that should be fine. - // TODO Verify change number test { - given(E164_B, PNI_A, ACI_A) + given(E164_B, PNI_A, ACI_A, createThread = true) process(E164_A, PNI_A, ACI_A) expect(E164_A, PNI_A, ACI_A) + + expectChangeNumberEvent() } } @Test fun onlyAciMatches_changeNumber() { - // TODO Verify change number test { - given(E164_B, null, ACI_A) + given(E164_B, null, ACI_A, createThread = true) process(E164_A, PNI_A, ACI_A) expect(E164_A, PNI_A, ACI_A) + + expectChangeNumberEvent() } } @@ -318,29 +329,31 @@ class RecipientDatabaseTest_processPnpTuple { @Test fun merge_e164AndPni_e164AndPniAndAci_changeNumber() { - // TODO Verify change number test { given(E164_A, PNI_A, null) - given(E164_B, PNI_B, ACI_A) + given(E164_B, PNI_B, ACI_A, createThread = true) process(E164_A, PNI_A, ACI_A) expectDeleted() expect(E164_A, PNI_A, ACI_A) + + expectChangeNumberEvent() } } @Test fun merge_e164AndPni_e164Aci_changeNumber() { - // TODO Verify change number test { given(E164_A, PNI_A, null) - given(E164_B, null, ACI_A) + given(E164_B, null, ACI_A, createThread = true) process(E164_A, PNI_A, ACI_A) expectDeleted() expect(E164_A, PNI_A, ACI_A) + + expectChangeNumberEvent() } } @@ -402,15 +415,23 @@ class RecipientDatabaseTest_processPnpTuple { private inner class TestCase { private val generatedIds: LinkedHashSet = LinkedHashSet() private var expectCount = 0 + private lateinit var outputRecipientId: RecipientId - fun given(e164: String?, pni: PNI?, aci: ACI?) { - generatedIds += insert(e164, pni, aci) + fun given(e164: String?, pni: PNI?, aci: ACI?, createThread: Boolean = false) { + val id = insert(e164, pni, aci) + generatedIds += id + if (createThread) { + // Create a thread and throw a dummy message in it so it doesn't get automatically deleted + threadDatabase.getOrCreateThreadIdFor(Recipient.resolved(id)) + smsDatabase.insertMessageInbox(IncomingEncryptedMessage(IncomingTextMessage(id, 1, 0, 0, 0, "", Optional.empty(), 0, false, ""), "")) + } } fun process(e164: String?, pni: PNI?, aci: ACI?) { SignalDatabase.rawDatabase.beginTransaction() try { - generatedIds += recipientDatabase.processPnpTuple(e164, pni, aci, pniVerified = false).finalId + outputRecipientId = recipientDatabase.processPnpTuple(e164, pni, aci, pniVerified = false).finalId + generatedIds += outputRecipientId SignalDatabase.rawDatabase.setTransactionSuccessful() } finally { SignalDatabase.rawDatabase.endTransaction() @@ -435,6 +456,10 @@ class RecipientDatabaseTest_processPnpTuple { fun expectDeleted(id: RecipientId) { assertNull(get(id)) } + + fun expectChangeNumberEvent() { + assertEquals(1, smsDatabase.getChangeNumberMessageCount(outputRecipientId)) + } } private data class IdRecord( diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/MessageDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/MessageDatabase.java index 16b2256ed..15ed6f43b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/MessageDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/MessageDatabase.java @@ -178,7 +178,7 @@ public abstract class MessageDatabase extends Database implements MmsSmsColumns, public abstract long insertMessageOutbox(@NonNull OutgoingMediaMessage message, long threadId, boolean forceSms, int defaultReceiptStatus, @Nullable SmsDatabase.InsertListener insertListener) throws MmsException; public abstract void insertProfileNameChangeMessages(@NonNull Recipient recipient, @NonNull String newProfileName, @NonNull String previousProfileName); public abstract void insertGroupV1MigrationEvents(@NonNull RecipientId recipientId, long threadId, @NonNull GroupMigrationMembershipChange membershipChange); - public abstract void insertNumberChangeMessages(@NonNull Recipient recipient); + public abstract void insertNumberChangeMessages(@NonNull RecipientId recipientId); public abstract void insertBoostRequestMessage(@NonNull RecipientId recipientId, long threadId); public abstract void insertThreadMergeEvent(@NonNull RecipientId recipientId, long threadId, @NonNull ThreadMergeEvent event); public abstract void insertSmsExportMessage(@NonNull RecipientId recipientId, long threadId); diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.java index c0f5f458c..1e3891b80 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.java @@ -572,7 +572,7 @@ public class MmsDatabase extends MessageDatabase { } @Override - public void insertNumberChangeMessages(@NonNull Recipient recipient) { + public void insertNumberChangeMessages(@NonNull RecipientId recipientId) { throw new UnsupportedOperationException(); } 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 4ecea1bbd..421a448ed 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.kt @@ -2445,8 +2445,11 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : Log.w(TAG, "Session switchover events aren't implemented yet!") } is PnpOperation.ChangeNumberInsert -> { - // TODO [pnp] - Log.w(TAG, "Change number inserts aren't implemented yet!") + if (changeSet.id is PnpIdResolver.PnpNoopId) { + SignalDatabase.sms.insertNumberChangeMessages(changeSet.id.recipientId) + } else { + throw IllegalStateException("There's a change number event on a newly-inserted recipient?") + } } } } @@ -2649,7 +2652,7 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : ) } - if (record.e164 != null && updatedNumber) { + if (record.e164 != null && updatedNumber && notSelf(e164, pni, aci) && !record.isBlocked) { operations += PnpOperation.ChangeNumberInsert( recipientId = commonId, oldE164 = record.e164, @@ -2762,7 +2765,7 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : secondaryId = data.byPniSid ) - if (data.aciSidRecord.e164 != null && data.aciSidRecord.e164 != e164) { + if (data.aciSidRecord.e164 != null && data.aciSidRecord.e164 != e164 && notSelf(e164, pni, aci) && !data.aciSidRecord.isBlocked) { operations += PnpOperation.ChangeNumberInsert( recipientId = data.byAciSid, oldE164 = data.aciSidRecord.e164, @@ -2788,7 +2791,7 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : e164 = e164 ) - if (data.aciSidRecord.e164 != null) { + if (data.aciSidRecord.e164 != null && notSelf(e164, pni, aci) && !data.aciSidRecord.isBlocked) { operations += PnpOperation.ChangeNumberInsert( recipientId = data.byAciSid, oldE164 = data.aciSidRecord.e164, @@ -2829,7 +2832,7 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : secondaryId = data.byE164 ) - if (data.aciSidRecord.e164 != null && data.aciSidRecord.e164 != e164) { + if (data.aciSidRecord.e164 != null && data.aciSidRecord.e164 != e164 && notSelf(e164, pni, aci) && !data.aciSidRecord.isBlocked) { operations += PnpOperation.ChangeNumberInsert( recipientId = data.byAciSid, oldE164 = data.aciSidRecord.e164, @@ -2854,7 +2857,7 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : secondaryId = data.byE164 ) - if (data.aciSidRecord.e164 != null && data.aciSidRecord.e164 != e164) { + if (data.aciSidRecord.e164 != null && data.aciSidRecord.e164 != e164 && notSelf(e164, pni, aci) && !data.aciSidRecord.isBlocked) { operations += PnpOperation.ChangeNumberInsert( recipientId = data.byAciSid, oldE164 = data.aciSidRecord.e164, @@ -2872,7 +2875,7 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) : e164 = e164 ) - if (data.aciSidRecord.e164 != null && data.aciSidRecord.e164 != e164) { + if (data.aciSidRecord.e164 != null && data.aciSidRecord.e164 != e164 && notSelf(e164, pni, aci) && !data.aciSidRecord.isBlocked) { operations += PnpOperation.ChangeNumberInsert( recipientId = data.byAciSid, oldE164 = data.aciSidRecord.e164, diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/SmsDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/SmsDatabase.java index 1b954585d..5799ba87e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/SmsDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/SmsDatabase.java @@ -88,6 +88,7 @@ import java.util.Optional; import java.util.Set; import java.util.UUID; +import static org.thoughtcrime.securesms.database.MmsSmsColumns.Types.CHANGE_NUMBER_TYPE; import static org.thoughtcrime.securesms.database.MmsSmsColumns.Types.GROUP_V2_LEAVE_BITS; /** @@ -1070,16 +1071,16 @@ public class SmsDatabase extends MessageDatabase { } @Override - public void insertNumberChangeMessages(@NonNull Recipient recipient) { + public void insertNumberChangeMessages(@NonNull RecipientId recipientId) { ThreadDatabase threadDatabase = SignalDatabase.threads(); - List groupRecords = SignalDatabase.groups().getGroupsContainingMember(recipient.getId(), false); + List groupRecords = SignalDatabase.groups().getGroupsContainingMember(recipientId, false); List threadIdsToUpdate = new LinkedList<>(); SQLiteDatabase db = databaseHelper.getSignalWritableDatabase(); db.beginTransaction(); try { - threadIdsToUpdate.add(threadDatabase.getThreadIdFor(recipient.getId())); + threadIdsToUpdate.add(threadDatabase.getThreadIdFor(recipientId)); for (GroupDatabase.GroupRecord groupRecord : groupRecords) { if (groupRecord.isActive()) { threadIdsToUpdate.add(threadDatabase.getThreadIdFor(groupRecord.getRecipientId())); @@ -1090,7 +1091,7 @@ public class SmsDatabase extends MessageDatabase { .filter(Objects::nonNull) .forEach(threadId -> { ContentValues values = new ContentValues(); - values.put(RECIPIENT_ID, recipient.getId().serialize()); + values.put(RECIPIENT_ID, recipientId.serialize()); values.put(ADDRESS_DEVICE_ID, 1); values.put(DATE_RECEIVED, System.currentTimeMillis()); values.put(DATE_SENT, System.currentTimeMillis()); @@ -2018,6 +2019,26 @@ public class SmsDatabase extends MessageDatabase { } } + /** + * The number of change number messages in the thread. + * Currently only used for tests. + */ + @VisibleForTesting + int getChangeNumberMessageCount(@NonNull RecipientId recipientId) { + try (Cursor cursor = SQLiteDatabaseExtensionsKt + .select(getReadableDatabase(), "COUNT(*)") + .from(TABLE_NAME) + .where(RECIPIENT_ID + " = ? AND " + TYPE + " = ?", recipientId, CHANGE_NUMBER_TYPE) + .run()) + { + if (cursor.moveToFirst()) { + return cursor.getInt(0); + } else { + return 0; + } + } + } + @VisibleForTesting Optional collapseJoinRequestEventsIfPossible(long threadId, IncomingGroupUpdateMessage message) { InsertResult result = null; diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/RecipientChangedNumberJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/RecipientChangedNumberJob.kt index 35811f99c..84c062725 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/RecipientChangedNumberJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/RecipientChangedNumberJob.kt @@ -32,7 +32,7 @@ class RecipientChangedNumberJob(parameters: Parameters, private val recipientId: if (!recipient.isBlocked && !recipient.isGroup && !recipient.isSelf) { Log.i(TAG, "Writing a number change event.") - SignalDatabase.sms.insertNumberChangeMessages(recipient) + SignalDatabase.sms.insertNumberChangeMessages(recipient.id) } else { Log.i(TAG, "Number changed but not relevant. blocked: ${recipient.isBlocked} isGroup: ${recipient.isGroup} isSelf: ${recipient.isSelf}") }