From 5d0f71e02c31379e96fcf7bb5005a5c52e3ceddc Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Thu, 3 Jul 2025 14:09:54 -0400 Subject: [PATCH] Fix duplicate numbers, remove shortcodes entirely. --- .../contacts/sync/ContactDiscovery.kt | 5 +- .../securesms/database/RecipientTable.kt | 6 +- .../jobmanager/impl/RegisteredConstraint.kt | 47 ++++ .../securesms/jobs/E164FormattingJob.kt | 47 ++++ .../securesms/jobs/JobManagerFactories.java | 8 +- .../securesms/keyvalue/AccountValues.kt | 4 + .../migrations/ApplicationMigrations.java | 7 +- .../migrations/E164FormattingMigrationJob.kt | 228 +++++++++--------- .../securesms/restore/RestoreRepository.kt | 3 + .../storage/ContactRecordProcessor.kt | 2 +- .../securesms/util/SignalE164Util.kt | 25 ++ .../storage/ContactRecordProcessorTest.kt | 1 + .../securesms/util/SignalE164UtilTest.kt | 62 +++++ .../java/org/signal/core/util/E164Util.kt | 6 + .../java/org/signal/core/util/E164UtilTest.kt | 3 + 15 files changed, 332 insertions(+), 122 deletions(-) create mode 100644 app/src/main/java/org/thoughtcrime/securesms/jobmanager/impl/RegisteredConstraint.kt create mode 100644 app/src/main/java/org/thoughtcrime/securesms/jobs/E164FormattingJob.kt create mode 100644 app/src/test/java/org/thoughtcrime/securesms/util/SignalE164UtilTest.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/contacts/sync/ContactDiscovery.kt b/app/src/main/java/org/thoughtcrime/securesms/contacts/sync/ContactDiscovery.kt index a50460555c..501e15c197 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/contacts/sync/ContactDiscovery.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/contacts/sync/ContactDiscovery.kt @@ -7,7 +7,6 @@ import androidx.annotation.WorkerThread import org.signal.contacts.SystemContactsRepository import org.signal.contacts.SystemContactsRepository.ContactIterator import org.signal.contacts.SystemContactsRepository.ContactPhoneDetails -import org.signal.core.util.E164Util import org.signal.core.util.Stopwatch import org.signal.core.util.StringUtil import org.signal.core.util.logging.Log @@ -25,6 +24,7 @@ import org.thoughtcrime.securesms.recipients.Recipient import org.thoughtcrime.securesms.recipients.RecipientId import org.thoughtcrime.securesms.registration.util.RegistrationUtil import org.thoughtcrime.securesms.storage.StorageSyncHelper +import org.thoughtcrime.securesms.util.SignalE164Util import org.thoughtcrime.securesms.util.TextSecurePreferences import org.thoughtcrime.securesms.util.Util import org.whispersystems.signalservice.api.push.ServiceId @@ -142,8 +142,7 @@ object ContactDiscovery { } private fun phoneNumberFormatter(): (String) -> String? { - val formatter = E164Util.createFormatterForE164(SignalStore.account.e164!!) - return { formatter.formatAsE164(it) } + return { SignalE164Util.formatNonShortCodeAsE164(it) } } private fun refreshRecipients( 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 8348d9ae78..4b349206f4 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt @@ -2374,7 +2374,7 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da val record = getRecord(recipientId) val pni = record.pni - val e164 = record.e164?.takeIf { SignalE164Util.isPotentialE164(it) } + val e164 = record.e164?.takeIf { SignalE164Util.isPotentialNonShortCodeE164(it) } if (pni == null && e164 == null) { return @@ -4114,7 +4114,7 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da check(e164 != null || pni != null || aci != null) { "Must provide some sort of identifier!" } val values = contentValuesOf( - E164 to e164, + E164 to e164.nullIfBlank()?.let { SignalE164Util.formatAsE164(it) }, ACI_COLUMN to aci?.toString(), PNI_COLUMN to pni?.toString(), PNI_SIGNATURE_VERIFIED to pniVerified.toInt(), @@ -4139,7 +4139,7 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da put(ACI_COLUMN, contact.proto.signalAci?.toString()) put(PNI_COLUMN, contact.proto.signalPni?.toString()) - put(E164, contact.proto.e164.nullIfBlank()) + put(E164, contact.proto.e164.nullIfBlank()?.let { SignalE164Util.formatAsE164(it) }) put(PROFILE_GIVEN_NAME, profileName.givenName) put(PROFILE_FAMILY_NAME, profileName.familyName) put(PROFILE_JOINED_NAME, profileName.toString()) diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/impl/RegisteredConstraint.kt b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/impl/RegisteredConstraint.kt new file mode 100644 index 0000000000..11c0f7f5a7 --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/impl/RegisteredConstraint.kt @@ -0,0 +1,47 @@ +/* + * Copyright 2025 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.jobmanager.impl + +import android.app.job.JobInfo +import org.thoughtcrime.securesms.jobmanager.Constraint +import org.thoughtcrime.securesms.jobmanager.ConstraintObserver +import org.thoughtcrime.securesms.keyvalue.SignalStore + +/** + * A constraint that is met so long as the current user is registered. + */ +object RegisteredConstraint : Constraint { + + const val KEY = "RegisteredConstraint" + + override fun isMet(): Boolean { + return SignalStore.account.isRegistered && SignalStore.account.aci != null + } + + override fun getFactoryKey(): String = KEY + + override fun applyToJobInfo(jobInfoBuilder: JobInfo.Builder) = Unit + + object Observer : ConstraintObserver { + val listeners: MutableSet = mutableSetOf() + + override fun register(notifier: ConstraintObserver.Notifier) { + listeners += notifier + } + + fun notifyListeners() { + for (listener in listeners) { + listener.onConstraintMet(KEY) + } + } + } + + class Factory : Constraint.Factory { + override fun create(): RegisteredConstraint { + return RegisteredConstraint + } + } +} diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/E164FormattingJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/E164FormattingJob.kt new file mode 100644 index 0000000000..c5affd0596 --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/E164FormattingJob.kt @@ -0,0 +1,47 @@ +/* + * Copyright 2025 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.jobs + +import org.thoughtcrime.securesms.jobmanager.Job +import org.thoughtcrime.securesms.jobmanager.impl.RegisteredConstraint +import org.thoughtcrime.securesms.migrations.E164FormattingMigrationJob + +/** + * A job that performs the same duties as [E164FormattingMigrationJob], but outside the scope of an app migration. + * This exists to be run after a backup restore, which might introduce more bad data. + */ +class E164FormattingJob private constructor(params: Parameters) : Job(params) { + + companion object { + const val KEY = "E164FormattingJob" + } + + constructor() : this( + Parameters.Builder() + .setQueue("E164FormattingJob") + .setLifespan(Parameters.IMMORTAL) + .setMaxInstancesForFactory(1) + .addConstraint(RegisteredConstraint.KEY) + .build() + ) + + override fun serialize(): ByteArray? = null + + override fun getFactoryKey(): String = KEY + + override fun run(): Result { + E164FormattingMigrationJob.fixE164Formatting() + return Result.success() + } + + override fun onFailure() = Unit + + class Factory : Job.Factory { + override fun create(params: Parameters, data: ByteArray?): E164FormattingJob { + return E164FormattingJob(params) + } + } +} diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java index 34187b2d51..16bdee445f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java @@ -26,6 +26,7 @@ import org.thoughtcrime.securesms.jobmanager.impl.NetworkOrCellServiceConstraint import org.thoughtcrime.securesms.jobmanager.impl.NoRemoteArchiveGarbageCollectionPendingConstraint; import org.thoughtcrime.securesms.jobmanager.impl.NotInCallConstraint; import org.thoughtcrime.securesms.jobmanager.impl.NotInCallConstraintObserver; +import org.thoughtcrime.securesms.jobmanager.impl.RegisteredConstraint; import org.thoughtcrime.securesms.jobmanager.impl.RestoreAttachmentConstraint; import org.thoughtcrime.securesms.jobmanager.impl.RestoreAttachmentConstraintObserver; import org.thoughtcrime.securesms.jobmanager.impl.SqlCipherMigrationConstraint; @@ -152,6 +153,7 @@ public final class JobManagerFactories { put(DeviceNameChangeJob.KEY, new DeviceNameChangeJob.Factory()); put(DirectoryRefreshJob.KEY, new DirectoryRefreshJob.Factory()); put(DownloadLatestEmojiDataJob.KEY, new DownloadLatestEmojiDataJob.Factory()); + put(E164FormattingJob.KEY, new E164FormattingJob.Factory()); put(EmojiSearchIndexDownloadJob.KEY, new EmojiSearchIndexDownloadJob.Factory()); put(FcmRefreshJob.KEY, new FcmRefreshJob.Factory()); put(FetchRemoteMegaphoneImageJob.KEY, new FetchRemoteMegaphoneImageJob.Factory()); @@ -411,9 +413,10 @@ public final class JobManagerFactories { put(NetworkOrCellServiceConstraint.KEY, new NetworkOrCellServiceConstraint.Factory(application)); put(NetworkOrCellServiceConstraint.LEGACY_KEY, new NetworkOrCellServiceConstraint.Factory(application)); put(NotInCallConstraint.KEY, new NotInCallConstraint.Factory()); + put(RegisteredConstraint.KEY, new RegisteredConstraint.Factory()); + put(RestoreAttachmentConstraint.KEY, new RestoreAttachmentConstraint.Factory(application)); put(SqlCipherMigrationConstraint.KEY, new SqlCipherMigrationConstraint.Factory(application)); put(WifiConstraint.KEY, new WifiConstraint.Factory(application)); - put(RestoreAttachmentConstraint.KEY, new RestoreAttachmentConstraint.Factory(application)); }}; } @@ -427,7 +430,8 @@ public final class JobManagerFactories { ChangeNumberConstraintObserver.INSTANCE, DataRestoreConstraintObserver.INSTANCE, RestoreAttachmentConstraintObserver.INSTANCE, - NoRemoteArchiveGarbageCollectionPendingConstraint.Observer.INSTANCE); + NoRemoteArchiveGarbageCollectionPendingConstraint.Observer.INSTANCE, + RegisteredConstraint.Observer.INSTANCE); } public static List getJobMigrations(@NonNull Application application) { 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 8bae6862e8..e990a01719 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/keyvalue/AccountValues.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/keyvalue/AccountValues.kt @@ -17,6 +17,7 @@ import org.thoughtcrime.securesms.crypto.ProfileKeyUtil import org.thoughtcrime.securesms.crypto.storage.PreKeyMetadataStore import org.thoughtcrime.securesms.database.SignalDatabase import org.thoughtcrime.securesms.dependencies.AppDependencies +import org.thoughtcrime.securesms.jobmanager.impl.RegisteredConstraint import org.thoughtcrime.securesms.jobs.PreKeysSyncJob import org.thoughtcrime.securesms.recipients.Recipient import org.thoughtcrime.securesms.service.KeyCachingService @@ -176,6 +177,7 @@ class AccountValues internal constructor(store: KeyValueStore, context: Context) fun setAci(aci: ACI) { putString(KEY_ACI, aci.toString()) + RegisteredConstraint.Observer.notifyListeners() } /** The local user's [PNI]. */ @@ -441,6 +443,8 @@ class AccountValues internal constructor(store: KeyValueStore, context: Context) } else if (!registered) { registeredAtTimestamp = -1 } + + RegisteredConstraint.Observer.notifyListeners() } /** diff --git a/app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java b/app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java index d5e1879801..3e40f1fb47 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java +++ b/app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java @@ -182,9 +182,10 @@ public class ApplicationMigrations { static final int SVR2_ENCLAVE_UPDATE_3 = 138; static final int DUPLICATE_E164_FIX_3 = 139; static final int E164_FORMATTING_2 = 140; + static final int E164_FORMATTING_3 = 141; } - public static final int CURRENT_VERSION = 140; + public static final int CURRENT_VERSION = 141; /** * This *must* be called after the {@link JobManager} has been instantiated, but *before* the call @@ -839,6 +840,10 @@ public class ApplicationMigrations { jobs.put(Version.E164_FORMATTING_2, new E164FormattingMigrationJob()); } + if (lastSeenVersion < Version.E164_FORMATTING_3) { + jobs.put(Version.E164_FORMATTING_3, new E164FormattingMigrationJob()); + } + return jobs; } diff --git a/app/src/main/java/org/thoughtcrime/securesms/migrations/E164FormattingMigrationJob.kt b/app/src/main/java/org/thoughtcrime/securesms/migrations/E164FormattingMigrationJob.kt index da1aa7384b..cb468dab46 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migrations/E164FormattingMigrationJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/migrations/E164FormattingMigrationJob.kt @@ -37,136 +37,140 @@ internal class E164FormattingMigrationJob( parameters: Parameters = Parameters.Builder().build() ) : MigrationJob(parameters) { - companion object { - val TAG = Log.tag(E164FormattingMigrationJob::class.java) - const val KEY = "E164FormattingMigrationJob" - } - override fun getFactoryKey(): String = KEY override fun isUiBlocking(): Boolean = false override fun performMigration() { - val stopwatch = Stopwatch("format-e164") - - val e164sByRecipientId = SignalDatabase.recipients.getAllE164sByRecipientId() - val invalidE164s: MutableList = mutableListOf() - stopwatch.split("fetch") - - for ((id, e164) in e164sByRecipientId) { - val formattedE164: String? = SignalE164Util.formatAsE164(e164) - if (formattedE164 == null) { - invalidE164s.add(id) - continue - } - - if (formattedE164 == e164) { - continue - } - - try { - Log.w(TAG, "Formatted e164 did not match saved e164. Attempting to update...") - SignalDatabase.recipients.setE164(id, formattedE164) - Log.w(TAG, "Successfully updated without a conflict.") - } catch (e: SQLiteException) { - Log.w(TAG, "Hit a SQLiteException, likely a conflict. Looking to merge.", true) - - val existing: Optional = SignalDatabase.recipients.getByE164(formattedE164) - if (existing.isPresent) { - Log.w(TAG, "Merging ${existing.get()} and $id", true) - SignalDatabase.rawDatabase.withinTransaction { - SignalDatabase.recipients.mergeForMigration(existing.get(), id) - } - Log.w(TAG, "Successfully merged ${existing.get()} and $id", true) - } else { - Log.w(TAG, "Unable to set E164, and it's not a conflict? Crashing.", e) - throw e - } - } - } - stopwatch.split("format") - - if (invalidE164s.isNotEmpty()) { - Log.w(TAG, "There were ${invalidE164s.size} invalid numbers found that could not be converted to proper e164's at all. Attempting to remove them.", true) - val failureCount = attemptToGetRidOfE164(invalidE164s) - if (failureCount > 0) { - Log.w(TAG, "Failed to remove $failureCount of the ${invalidE164s.size} invalid numbers.", true) - } else { - Log.w(TAG, "Successfully removed all ${invalidE164s.size} invalid numbers.", true) - } - } else { - Log.w(TAG, "No invalid E164's found. All could be formatted as e164.") - } - stopwatch.split("invalid") - - stopwatch.stop(TAG) + fixE164Formatting() } override fun shouldRetry(e: Exception): Boolean = false - private fun attemptToGetRidOfE164(entries: List): Int { - var failureCount = 0 + companion object { + val TAG = Log.tag(E164FormattingMigrationJob::class.java) + const val KEY = "E164FormattingMigrationJob" - for (id in entries) { - if (SignalDatabase.recipients.removeE164IfAnotherIdentifierIsPresent(id)) { - Log.run { w(TAG, "[$id] Successfully removed an invalid e164 on a recipient that has other identifiers.", true) } - continue - } - Log.w(TAG, "[$id] Unable to remove an invalid e164 on a recipient because it has no other identifiers. Attempting to remove the recipient entirely.", true) + fun fixE164Formatting() { + val stopwatch = Stopwatch("format-e164") - if (SignalDatabase.recipients.deleteRecipientIfItHasNoMessages(id)) { - Log.w(TAG, "[$id] Successfully deleted a recipient with an invalid e164 because it had no messages.", true) - continue + val e164sByRecipientId = SignalDatabase.recipients.getAllE164sByRecipientId() + val invalidE164s: MutableList = mutableListOf() + stopwatch.split("fetch") + + for ((id, e164) in e164sByRecipientId) { + val formattedE164: String? = SignalE164Util.formatNonShortCodeAsE164(e164) + if (formattedE164 == null) { + invalidE164s.add(id) + continue + } + + if (formattedE164 == e164) { + continue + } + + try { + Log.w(TAG, "Formatted e164 did not match saved e164. Attempting to update...") + SignalDatabase.recipients.setE164(id, formattedE164) + Log.w(TAG, "Successfully updated without a conflict.") + } catch (e: SQLiteException) { + Log.w(TAG, "Hit a SQLiteException, likely a conflict. Looking to merge.", true) + + val existing: Optional = SignalDatabase.recipients.getByE164(formattedE164) + if (existing.isPresent) { + Log.w(TAG, "Merging ${existing.get()} and $id", true) + SignalDatabase.rawDatabase.withinTransaction { + SignalDatabase.recipients.mergeForMigration(existing.get(), id) + } + Log.w(TAG, "Successfully merged ${existing.get()} and $id", true) + } else { + Log.w(TAG, "Unable to set E164, and it's not a conflict? Crashing.", e) + throw e + } + } } - Log.w(TAG, "[$id] Unable to delete a recipient with an invalid e164 because it had messages.", true) - failureCount++ + stopwatch.split("format") + + if (invalidE164s.isNotEmpty()) { + Log.w(TAG, "There were ${invalidE164s.size} invalid numbers found that could not be converted to proper e164's at all. Attempting to remove them.", true) + val failureCount = attemptToGetRidOfE164(invalidE164s) + if (failureCount > 0) { + Log.w(TAG, "Failed to remove $failureCount of the ${invalidE164s.size} invalid numbers.", true) + } else { + Log.w(TAG, "Successfully removed all ${invalidE164s.size} invalid numbers.", true) + } + } else { + Log.w(TAG, "No invalid E164's found. All could be formatted as e164.") + } + stopwatch.split("invalid") + + stopwatch.stop(TAG) } - return failureCount - } + private fun attemptToGetRidOfE164(entries: List): Int { + var failureCount = 0 - private fun RecipientTable.removeE164IfAnotherIdentifierIsPresent(recipientId: RecipientId): Boolean { - return readableDatabase - .update(TABLE_NAME) - .values(E164 to null) - .where("$ID = ? AND ($ACI_COLUMN NOT NULL OR $PNI_COLUMN NOT NULL)", recipientId) - .run() > 0 - } + for (id in entries) { + if (SignalDatabase.recipients.removeE164IfAnotherIdentifierIsPresent(id)) { + Log.run { w(TAG, "[$id] Successfully removed an invalid e164 on a recipient that has other identifiers.", true) } + continue + } + Log.w(TAG, "[$id] Unable to remove an invalid e164 on a recipient because it has no other identifiers. Attempting to remove the recipient entirely.", true) - private fun RecipientTable.deleteRecipientIfItHasNoMessages(recipientId: RecipientId): Boolean { - return readableDatabase - .delete(TABLE_NAME) - .where( - """ - $ID = ? AND - $ID NOT IN ( - SELECT ${MessageTable.TO_RECIPIENT_ID} FROM ${MessageTable.TABLE_NAME} - UNION - SELECT ${MessageTable.FROM_RECIPIENT_ID} FROM ${MessageTable.TABLE_NAME} - ) - """, - recipientId - ) - .run() > 0 - } - - private fun RecipientTable.getAllE164sByRecipientId(): Map { - return readableDatabase - .select(ID, E164) - .from(TABLE_NAME) - .where("$E164 NOT NULL") - .run() - .readToMap { - RecipientId.from(it.requireLong(ID)) to it.requireNonNullString(E164) + if (SignalDatabase.recipients.deleteRecipientIfItHasNoMessages(id)) { + Log.w(TAG, "[$id] Successfully deleted a recipient with an invalid e164 because it had no messages.", true) + continue + } + Log.w(TAG, "[$id] Unable to delete a recipient with an invalid e164 because it had messages.", true) + failureCount++ } - } - private fun RecipientTable.setE164(id: RecipientId, e164: String) { - writableDatabase - .update(TABLE_NAME) - .values(E164 to e164) - .where("$ID = ?", id) - .run() + return failureCount + } + + private fun RecipientTable.removeE164IfAnotherIdentifierIsPresent(recipientId: RecipientId): Boolean { + return readableDatabase + .update(TABLE_NAME) + .values(E164 to null) + .where("$ID = ? AND ($ACI_COLUMN NOT NULL OR $PNI_COLUMN NOT NULL)", recipientId) + .run() > 0 + } + + private fun RecipientTable.deleteRecipientIfItHasNoMessages(recipientId: RecipientId): Boolean { + return readableDatabase + .delete(TABLE_NAME) + .where( + """ + $ID = ? AND + $ID NOT IN ( + SELECT ${MessageTable.TO_RECIPIENT_ID} FROM ${MessageTable.TABLE_NAME} + UNION + SELECT ${MessageTable.FROM_RECIPIENT_ID} FROM ${MessageTable.TABLE_NAME} + ) + """, + recipientId + ) + .run() > 0 + } + + private fun RecipientTable.getAllE164sByRecipientId(): Map { + return readableDatabase + .select(ID, E164) + .from(TABLE_NAME) + .where("$E164 NOT NULL") + .run() + .readToMap { + RecipientId.from(it.requireLong(ID)) to it.requireNonNullString(E164) + } + } + + private fun RecipientTable.setE164(id: RecipientId, e164: String) { + writableDatabase + .update(TABLE_NAME) + .values(E164 to e164) + .where("$ID = ?", id) + .run() + } } class Factory : Job.Factory { diff --git a/app/src/main/java/org/thoughtcrime/securesms/restore/RestoreRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/restore/RestoreRepository.kt index cb4e3d40a7..fa12e0c575 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/restore/RestoreRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/restore/RestoreRepository.kt @@ -15,7 +15,9 @@ import org.thoughtcrime.securesms.backup.BackupPassphrase import org.thoughtcrime.securesms.backup.FullBackupImporter import org.thoughtcrime.securesms.crypto.AttachmentSecretProvider import org.thoughtcrime.securesms.database.SignalDatabase +import org.thoughtcrime.securesms.dependencies.AppDependencies import org.thoughtcrime.securesms.jobmanager.impl.DataRestoreConstraint +import org.thoughtcrime.securesms.jobs.E164FormattingJob import org.thoughtcrime.securesms.keyvalue.SignalStore import org.thoughtcrime.securesms.notifications.NotificationChannels import org.thoughtcrime.securesms.service.LocalBackupListener @@ -68,6 +70,7 @@ object RestoreRepository { SignalDatabase.runPostBackupRestoreTasks(database) NotificationChannels.getInstance().restoreContactNotificationChannels() + AppDependencies.jobManager.add(E164FormattingJob()) if (BackupUtil.canUserAccessBackupDirectory(context)) { LocalBackupListener.setNextBackupTimeToIntervalFromNow(context) diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.kt b/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.kt index 95f43f57fc..383ae41f90 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.kt @@ -41,7 +41,7 @@ class ContactRecordProcessor( companion object { private val TAG = Log.tag(ContactRecordProcessor::class.java) - private val E164_PATTERN: Pattern = Pattern.compile("^\\+[1-9]\\d{0,18}$") + private val E164_PATTERN: Pattern = Pattern.compile("^\\+[1-9]\\d{6,18}$") private fun isValidE164(value: String): Boolean { return E164_PATTERN.matcher(value).matches() diff --git a/app/src/main/java/org/thoughtcrime/securesms/util/SignalE164Util.kt b/app/src/main/java/org/thoughtcrime/securesms/util/SignalE164Util.kt index acfb828df1..2c584ef305 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/util/SignalE164Util.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/util/SignalE164Util.kt @@ -53,6 +53,23 @@ object SignalE164Util { return getFormatter().formatAsE164(input) } + /** + * Formats the number as an E164, or null if the number cannot be reasonably interpreted as a phone number, or if + * the number is a shortcode (<= 6 digits, excluding leading '+' and zeroes). + * + * This does not check if the number is *valid* for a given region. Instead, it's very lenient and just + * does it's best to interpret the input string as a number that could be put into the E164 format. + * + * Note that shortcodes will not have leading '+' signs. + * + * In other words, if this method returns null, you likely do not have anything that could be considered + * a phone number. + */ + @JvmStatic + fun formatNonShortCodeAsE164(input: String): String? { + return getFormatter().formatAsE164(input)?.takeIf { input.trimStart('+', '0').length > 6 } + } + /** * Returns true if the input string can be considered an E164. Specifically, it returns true if we could figure out how to format it as an E164. */ @@ -61,6 +78,14 @@ object SignalE164Util { return formatAsE164(input) != null } + /** + * Performs the same check as [isPotentialE164], with the additional validation to check if there are more than 6 digits in the number. + * When counting digits, leading zeroes and '+' will be ignored. + */ + fun isPotentialNonShortCodeE164(input: String): Boolean { + return formatNonShortCodeAsE164(input) != null + } + private fun getFormatter(): E164Util.Formatter { val localNumber = SignalStore.account.e164 ?: return defaultFormatter val formatter = cachedFormatters[localNumber] diff --git a/app/src/test/java/org/thoughtcrime/securesms/storage/ContactRecordProcessorTest.kt b/app/src/test/java/org/thoughtcrime/securesms/storage/ContactRecordProcessorTest.kt index 463ac955ec..3a146ff616 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/storage/ContactRecordProcessorTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/storage/ContactRecordProcessorTest.kt @@ -41,6 +41,7 @@ class ContactRecordProcessorTest { fun setup() { mockkObject(SignalStore) every { SignalStore.account.isPrimaryDevice } returns true + every { SignalStore.account.e164 } returns "+11234567890" recipientTable = mockk(relaxed = true) } diff --git a/app/src/test/java/org/thoughtcrime/securesms/util/SignalE164UtilTest.kt b/app/src/test/java/org/thoughtcrime/securesms/util/SignalE164UtilTest.kt new file mode 100644 index 0000000000..0840153156 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/util/SignalE164UtilTest.kt @@ -0,0 +1,62 @@ +/* + * Copyright 2025 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.util + +import assertk.assertThat +import assertk.assertions.isFalse +import assertk.assertions.isTrue +import io.mockk.every +import io.mockk.mockkObject +import org.junit.Before +import org.junit.Test +import org.thoughtcrime.securesms.keyvalue.SignalStore + +class SignalE164UtilTest { + + @Before + fun setup() { + mockkObject(SignalStore) + every { SignalStore.account.e164 } returns "+11234567890" + } + + @Test + fun `isPotentialNonShortCodeE164 - valid`() { + assertThat(SignalE164Util.isPotentialNonShortCodeE164("+1234567890")).isTrue() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("1234567")).isTrue() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("1234568")).isTrue() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("12345679")).isTrue() + } + + @Test + fun `isPotentialNonShortCodeE164 - invalid, no leading characters`() { + assertThat(SignalE164Util.isPotentialNonShortCodeE164("1")).isFalse() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("12")).isFalse() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("123")).isFalse() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("12345")).isFalse() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("123456")).isFalse() + } + + @Test + fun `isPotentialNonShortCodeE164 - invalid, leading plus sign`() { + assertThat(SignalE164Util.isPotentialNonShortCodeE164("+123456")).isFalse() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("++123456")).isFalse() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("+++123456")).isFalse() + } + + @Test + fun `isPotentialNonShortCodeE164 - invalid, leading zeros`() { + assertThat(SignalE164Util.isPotentialNonShortCodeE164("0123456")).isFalse() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("00123456")).isFalse() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("000123456")).isFalse() + } + + @Test + fun `isPotentialNonShortCodeE164 - invalid, mix of leading characters`() { + assertThat(SignalE164Util.isPotentialNonShortCodeE164("+0123456")).isFalse() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("0+0123456")).isFalse() + assertThat(SignalE164Util.isPotentialNonShortCodeE164("+0+123456")).isFalse() + } +} diff --git a/core-util-jvm/src/main/java/org/signal/core/util/E164Util.kt b/core-util-jvm/src/main/java/org/signal/core/util/E164Util.kt index e10ce5c6f6..61834a20a6 100644 --- a/core-util-jvm/src/main/java/org/signal/core/util/E164Util.kt +++ b/core-util-jvm/src/main/java/org/signal/core/util/E164Util.kt @@ -33,6 +33,8 @@ object E164Util { /** A set of country codes representing countries where we'd like to use the (555) 555-5555 number format for pretty printing. */ private val NATIONAL_FORMAT_COUNTRY_CODES = setOf(COUNTRY_CODE_US_INT, COUNTRY_CODE_UK_INT) + private val INVALID_CHARACTERS_REGEX = "[a-zA-Z]".toRegex() + /** * Creates a formatter based on the provided local number. This is largely an improvement in performance/convenience * over parsing out the various number attributes themselves and caching them manually. @@ -253,6 +255,10 @@ object E164Util { * a phone number. */ fun formatAsE164(input: String): String? { + if (INVALID_CHARACTERS_REGEX.containsMatchIn(input)) { + return null + } + val formatted = formatAsE164WithRegionCode( localNumber = localNumber, localAreaCode = localAreaCode, diff --git a/core-util-jvm/src/test/java/org/signal/core/util/E164UtilTest.kt b/core-util-jvm/src/test/java/org/signal/core/util/E164UtilTest.kt index c45fd92539..9d6a356a55 100644 --- a/core-util-jvm/src/test/java/org/signal/core/util/E164UtilTest.kt +++ b/core-util-jvm/src/test/java/org/signal/core/util/E164UtilTest.kt @@ -107,6 +107,7 @@ class E164UtilTest { Assert.assertEquals("40404", formatter.formatAsE164("+40404")) Assert.assertEquals("404040", formatter.formatAsE164("+404040")) Assert.assertEquals("404040", formatter.formatAsE164("404040")) + Assert.assertEquals("49173", formatter.formatAsE164("+49173")) Assert.assertEquals("7726", formatter.formatAsE164("+7726")) Assert.assertEquals("69987", formatter.formatAsE164("+69987")) Assert.assertEquals("40404", formatter.formatAsE164("40404")) @@ -127,6 +128,7 @@ class E164UtilTest { Assert.assertEquals("988", formatter.formatAsE164("988")) Assert.assertEquals("999", formatter.formatAsE164("999")) Assert.assertEquals("118", formatter.formatAsE164("118")) + Assert.assertEquals("20202", formatter.formatAsE164("020202")) Assert.assertEquals("+119990001", formatter.formatAsE164("19990001")) formatter = E164Util.createFormatterForE164("+61 2 9876 5432") @@ -145,6 +147,7 @@ class E164UtilTest { Assert.assertEquals(null, formatter.formatAsE164("55")) Assert.assertEquals(null, formatter.formatAsE164("0")) Assert.assertEquals(null, formatter.formatAsE164("000")) + Assert.assertEquals(null, formatter.formatAsE164("+1555ABC4567")) } @Test