From 1355a4a28d6421ee48a97c73bf3b7bd824a09741 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Thu, 30 Nov 2023 09:32:04 -0800 Subject: [PATCH] Fix bug where username may be put in e164 column. --- .../contacts/paged/ContactSearchKey.kt | 2 +- .../helpers/SignalDatabaseMigrations.kt | 21 +++++-- .../migration/SignalDatabaseMigration.kt | 4 ++ .../migration/V213_FixUsernameInE164Column.kt | 56 +++++++++++++++++++ .../securesms/recipients/Recipient.java | 3 + .../core/util/SQLiteDatabaseExtensions.kt | 6 ++ 6 files changed, 86 insertions(+), 6 deletions(-) create mode 100644 app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V213_FixUsernameInE164Column.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchKey.kt b/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchKey.kt index a30dc35739..08d91cd8db 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchKey.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchKey.kt @@ -34,7 +34,7 @@ sealed class ContactSearchKey { data class UnknownRecipientKey(val sectionKey: ContactSearchConfiguration.SectionKey, val query: String) : ContactSearchKey() { override fun requireSelectedContact(): SelectedContact = when (sectionKey) { - ContactSearchConfiguration.SectionKey.USERNAME -> SelectedContact.forPhone(null, query) + ContactSearchConfiguration.SectionKey.USERNAME -> SelectedContact.forUsername(null, query) ContactSearchConfiguration.SectionKey.PHONE_NUMBER -> SelectedContact.forPhone(null, query) else -> error("Unexpected section for unknown recipient: $sectionKey") } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt index 8d103140cb..d4fba06012 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt @@ -3,6 +3,7 @@ package org.thoughtcrime.securesms.database.helpers import android.app.Application import android.content.Context import net.zetetic.database.sqlcipher.SQLiteDatabase +import org.signal.core.util.areForeignKeyConstraintsEnabled import org.signal.core.util.logging.Log import org.signal.core.util.withinTransaction import org.thoughtcrime.securesms.database.helpers.migration.SignalDatabaseMigration @@ -69,6 +70,7 @@ import org.thoughtcrime.securesms.database.helpers.migration.V209_ClearRecipient import org.thoughtcrime.securesms.database.helpers.migration.V210_FixPniPossibleColumns import org.thoughtcrime.securesms.database.helpers.migration.V211_ReceiptColumnRenames import org.thoughtcrime.securesms.database.helpers.migration.V212_RemoveDistributionListUniqueConstraint +import org.thoughtcrime.securesms.database.helpers.migration.V213_FixUsernameInE164Column /** * Contains all of the database migrations for [SignalDatabase]. Broken into a separate file for cleanliness. @@ -77,8 +79,6 @@ object SignalDatabaseMigrations { val TAG: String = Log.tag(SignalDatabaseMigrations.javaClass) - const val DATABASE_VERSION = 212 - private val migrations: List> = listOf( 149 to V149_LegacyMigrations, 150 to V150_UrgentMslFlagMigration, @@ -143,23 +143,34 @@ object SignalDatabaseMigrations { 209 to V209_ClearRecipientPniFromAciColumn, 210 to V210_FixPniPossibleColumns, 211 to V211_ReceiptColumnRenames, - 212 to V212_RemoveDistributionListUniqueConstraint + 212 to V212_RemoveDistributionListUniqueConstraint, + 213 to V213_FixUsernameInE164Column ) + const val DATABASE_VERSION = 213 + @JvmStatic fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { + val initialForeignKeyState = db.areForeignKeyConstraintsEnabled() + for (migrationData in migrations) { val (version, migration) = migrationData if (oldVersion < version) { - Log.i(TAG, "Running migration for version $version: ${migration.javaClass.simpleName}") + Log.i(TAG, "Running migration for version $version: ${migration.javaClass.simpleName}. Foreign keys: ${migration.enableForeignKeys}") + val startTime = System.currentTimeMillis() + + db.setForeignKeyConstraintsEnabled(migration.enableForeignKeys) db.withinTransaction { migration.migrate(context, db, oldVersion, newVersion) db.version = version } - Log.i(TAG, "Successfully completed migration for version $version.") + + Log.i(TAG, "Successfully completed migration for version $version in ${System.currentTimeMillis() - startTime} ms") } } + + db.setForeignKeyConstraintsEnabled(initialForeignKeyState) } @JvmStatic diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/SignalDatabaseMigration.kt b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/SignalDatabaseMigration.kt index d6b23ff370..ef190da45c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/SignalDatabaseMigration.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/SignalDatabaseMigration.kt @@ -7,5 +7,9 @@ import net.zetetic.database.sqlcipher.SQLiteDatabase * Simple interface for allowing database migrations to live outside of [org.thoughtcrime.securesms.database.helpers.SignalDatabaseMigrations]. */ interface SignalDatabaseMigration { + /** True if you want foreign key constraints to be enforced during a migration, otherwise false. Defaults to false. */ + val enableForeignKeys: Boolean + get() = false + fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V213_FixUsernameInE164Column.kt b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V213_FixUsernameInE164Column.kt new file mode 100644 index 0000000000..260f34ea58 --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V213_FixUsernameInE164Column.kt @@ -0,0 +1,56 @@ +/* + * Copyright 2023 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.database.helpers.migration + +import android.app.Application +import net.zetetic.database.sqlcipher.SQLiteDatabase +import org.signal.core.util.logging.Log + +/** + * There was a bug where adding a member to a group by username could put that username in the e164 column. + * We have to clean it up and move that value to the username column. + */ +object V213_FixUsernameInE164Column : SignalDatabaseMigration { + + private val TAG = Log.tag(V213_FixUsernameInE164Column::class.java) + + /** We rely on foreign keys to clean up data from bad recipients */ + override val enableForeignKeys: Boolean = true + + override fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { + // In order to avoid unique constraint violations, we run this query once to move over everything that doesn't break any violations... + db.execSQL( + """ + UPDATE + recipient + SET + username = e164, + e164 = NULL + WHERE + e164 GLOB '[a-zA-Z][a-zA-Z0-9][a-zA-Z0-9]*.[0-9][0-9]*' + AND e164 NOT IN (SELECT username FROM recipient WHERE username IS NOT NULL) + """ + ) + + // ...and again to just clear out any remaining bad data. This should only clear data that would otherwise violate the unique constraint. + db.execSQL( + """ + UPDATE + recipient + SET + e164 = NULL + WHERE + e164 GLOB '[a-zA-Z][a-zA-Z0-9][a-zA-Z0-9]*.[0-9][0-9]*' + """ + ) + + // Finally, the above queries may have created recipients that have a username but no ACI. These are invalid entries that need to be trimmed. + // Given that usernames are not public, we'll rely on cascading deletes here to clean things up (foreign keys are enabled for this migration). + db.delete("recipient", "username IS NOT NULL AND aci IS NULL", null).let { deleteCount -> + Log.i(TAG, "Deleted $deleteCount username-only recipients.") + } + } +} diff --git a/app/src/main/java/org/thoughtcrime/securesms/recipients/Recipient.java b/app/src/main/java/org/thoughtcrime/securesms/recipients/Recipient.java index 9c99400576..c021f3260c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/recipients/Recipient.java +++ b/app/src/main/java/org/thoughtcrime/securesms/recipients/Recipient.java @@ -47,6 +47,7 @@ import org.thoughtcrime.securesms.profiles.ProfileName; import org.thoughtcrime.securesms.service.webrtc.links.CallLinkRoomId; import org.thoughtcrime.securesms.util.AvatarUtil; import org.thoughtcrime.securesms.util.FeatureFlags; +import org.thoughtcrime.securesms.util.UsernameUtil; import org.thoughtcrime.securesms.util.Util; import org.thoughtcrime.securesms.wallpaper.ChatWallpaper; import org.whispersystems.signalservice.api.push.ServiceId; @@ -354,6 +355,8 @@ public class Recipient { id = db.getOrInsertFromGroupId(GroupId.parseOrThrow(identifier)); } else if (NumberUtil.isValidEmail(identifier)) { id = db.getOrInsertFromEmail(identifier); + } else if (UsernameUtil.isValidUsernameForSearch(identifier)) { + throw new IllegalArgumentException("Creating a recipient based on username alone is not supported!"); } else { String e164 = PhoneNumberFormatter.get(context).format(identifier); id = db.getOrInsertFromE164(e164); diff --git a/core-util/src/main/java/org/signal/core/util/SQLiteDatabaseExtensions.kt b/core-util/src/main/java/org/signal/core/util/SQLiteDatabaseExtensions.kt index d65cb3eba2..4d56634a1d 100644 --- a/core-util/src/main/java/org/signal/core/util/SQLiteDatabaseExtensions.kt +++ b/core-util/src/main/java/org/signal/core/util/SQLiteDatabaseExtensions.kt @@ -51,6 +51,12 @@ fun SupportSQLiteDatabase.getForeignKeys(): List { .flatten() } +fun SupportSQLiteDatabase.areForeignKeyConstraintsEnabled(): Boolean { + return this.query("PRAGMA foreign_keys", null).use { cursor -> + cursor.moveToFirst() && cursor.getInt(0) != 0 + } +} + fun SupportSQLiteDatabase.getIndexes(): List { return this.query("SELECT name, tbl_name FROM sqlite_master WHERE type='index' ORDER BY name ASC").readToList { cursor -> val indexName = cursor.requireNonNullString("name")