Skip to content

Commit 1355a4a

Browse files
greyson-signalcody-signal
greyson-signal
authored and
cody-signal
committed
Fix bug where username may be put in e164 column.
1 parent 97c34b8 commit 1355a4a

6 files changed

Lines changed: 86 additions & 6 deletions

File tree

‎app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchKey.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ sealed class ContactSearchKey {
3434

3535
data class UnknownRecipientKey(val sectionKey: ContactSearchConfiguration.SectionKey, val query: String) : ContactSearchKey() {
3636
override fun requireSelectedContact(): SelectedContact = when (sectionKey) {
37-
ContactSearchConfiguration.SectionKey.USERNAME -> SelectedContact.forPhone(null, query)
37+
ContactSearchConfiguration.SectionKey.USERNAME -> SelectedContact.forUsername(null, query)
3838
ContactSearchConfiguration.SectionKey.PHONE_NUMBER -> SelectedContact.forPhone(null, query)
3939
else -> error("Unexpected section for unknown recipient: $sectionKey")
4040
}

‎app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package org.thoughtcrime.securesms.database.helpers
33
import android.app.Application
44
import android.content.Context
55
import net.zetetic.database.sqlcipher.SQLiteDatabase
6+
import org.signal.core.util.areForeignKeyConstraintsEnabled
67
import org.signal.core.util.logging.Log
78
import org.signal.core.util.withinTransaction
89
import org.thoughtcrime.securesms.database.helpers.migration.SignalDatabaseMigration
@@ -69,6 +70,7 @@ import org.thoughtcrime.securesms.database.helpers.migration.V209_ClearRecipient
6970
import org.thoughtcrime.securesms.database.helpers.migration.V210_FixPniPossibleColumns
7071
import org.thoughtcrime.securesms.database.helpers.migration.V211_ReceiptColumnRenames
7172
import org.thoughtcrime.securesms.database.helpers.migration.V212_RemoveDistributionListUniqueConstraint
73+
import org.thoughtcrime.securesms.database.helpers.migration.V213_FixUsernameInE164Column
7274

7375
/**
7476
* Contains all of the database migrations for [SignalDatabase]. Broken into a separate file for cleanliness.
@@ -77,8 +79,6 @@ object SignalDatabaseMigrations {
7779

7880
val TAG: String = Log.tag(SignalDatabaseMigrations.javaClass)
7981

80-
const val DATABASE_VERSION = 212
81-
8282
private val migrations: List<Pair<Int, SignalDatabaseMigration>> = listOf(
8383
149 to V149_LegacyMigrations,
8484
150 to V150_UrgentMslFlagMigration,
@@ -143,23 +143,34 @@ object SignalDatabaseMigrations {
143143
209 to V209_ClearRecipientPniFromAciColumn,
144144
210 to V210_FixPniPossibleColumns,
145145
211 to V211_ReceiptColumnRenames,
146-
212 to V212_RemoveDistributionListUniqueConstraint
146+
212 to V212_RemoveDistributionListUniqueConstraint,
147+
213 to V213_FixUsernameInE164Column
147148
)
148149

150+
const val DATABASE_VERSION = 213
151+
149152
@JvmStatic
150153
fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int) {
154+
val initialForeignKeyState = db.areForeignKeyConstraintsEnabled()
155+
151156
for (migrationData in migrations) {
152157
val (version, migration) = migrationData
153158

154159
if (oldVersion < version) {
155-
Log.i(TAG, "Running migration for version $version: ${migration.javaClass.simpleName}")
160+
Log.i(TAG, "Running migration for version $version: ${migration.javaClass.simpleName}. Foreign keys: ${migration.enableForeignKeys}")
161+
val startTime = System.currentTimeMillis()
162+
163+
db.setForeignKeyConstraintsEnabled(migration.enableForeignKeys)
156164
db.withinTransaction {
157165
migration.migrate(context, db, oldVersion, newVersion)
158166
db.version = version
159167
}
160-
Log.i(TAG, "Successfully completed migration for version $version.")
168+
169+
Log.i(TAG, "Successfully completed migration for version $version in ${System.currentTimeMillis() - startTime} ms")
161170
}
162171
}
172+
173+
db.setForeignKeyConstraintsEnabled(initialForeignKeyState)
163174
}
164175

165176
@JvmStatic

‎app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/SignalDatabaseMigration.kt‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,5 +7,9 @@ import net.zetetic.database.sqlcipher.SQLiteDatabase
77
* Simple interface for allowing database migrations to live outside of [org.thoughtcrime.securesms.database.helpers.SignalDatabaseMigrations].
88
*/
99
interface SignalDatabaseMigration {
10+
/** True if you want foreign key constraints to be enforced during a migration, otherwise false. Defaults to false. */
11+
val enableForeignKeys: Boolean
12+
get() = false
13+
1014
fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int)
1115
}

‎app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V213_FixUsernameInE164Column.kt‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/*
2+
* Copyright 2023 Signal Messenger, LLC
3+
* SPDX-License-Identifier: AGPL-3.0-only
4+
*/
5+
6+
package org.thoughtcrime.securesms.database.helpers.migration
7+
8+
import android.app.Application
9+
import net.zetetic.database.sqlcipher.SQLiteDatabase
10+
import org.signal.core.util.logging.Log
11+
12+
/**
13+
* There was a bug where adding a member to a group by username could put that username in the e164 column.
14+
* We have to clean it up and move that value to the username column.
15+
*/
16+
object V213_FixUsernameInE164Column : SignalDatabaseMigration {
17+
18+
private val TAG = Log.tag(V213_FixUsernameInE164Column::class.java)
19+
20+
/** We rely on foreign keys to clean up data from bad recipients */
21+
override val enableForeignKeys: Boolean = true
22+
23+
override fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int) {
24+
// In order to avoid unique constraint violations, we run this query once to move over everything that doesn't break any violations...
25+
db.execSQL(
26+
"""
27+
UPDATE
28+
recipient
29+
SET
30+
username = e164,
31+
e164 = NULL
32+
WHERE
33+
e164 GLOB '[a-zA-Z][a-zA-Z0-9][a-zA-Z0-9]*.[0-9][0-9]*'
34+
AND e164 NOT IN (SELECT username FROM recipient WHERE username IS NOT NULL)
35+
"""
36+
)
37+
38+
// ...and again to just clear out any remaining bad data. This should only clear data that would otherwise violate the unique constraint.
39+
db.execSQL(
40+
"""
41+
UPDATE
42+
recipient
43+
SET
44+
e164 = NULL
45+
WHERE
46+
e164 GLOB '[a-zA-Z][a-zA-Z0-9][a-zA-Z0-9]*.[0-9][0-9]*'
47+
"""
48+
)
49+
50+
// Finally, the above queries may have created recipients that have a username but no ACI. These are invalid entries that need to be trimmed.
51+
// Given that usernames are not public, we'll rely on cascading deletes here to clean things up (foreign keys are enabled for this migration).
52+
db.delete("recipient", "username IS NOT NULL AND aci IS NULL", null).let { deleteCount ->
53+
Log.i(TAG, "Deleted $deleteCount username-only recipients.")
54+
}
55+
}
56+
}

‎app/src/main/java/org/thoughtcrime/securesms/recipients/Recipient.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import org.thoughtcrime.securesms.service.webrtc.links.CallLinkRoomId;
4848
import org.thoughtcrime.securesms.util.AvatarUtil;
4949
import org.thoughtcrime.securesms.util.FeatureFlags;
50+
import org.thoughtcrime.securesms.util.UsernameUtil;
5051
import org.thoughtcrime.securesms.util.Util;
5152
import org.thoughtcrime.securesms.wallpaper.ChatWallpaper;
5253
import org.whispersystems.signalservice.api.push.ServiceId;
@@ -354,6 +355,8 @@ public class Recipient {
354355
id = db.getOrInsertFromGroupId(GroupId.parseOrThrow(identifier));
355356
} else if (NumberUtil.isValidEmail(identifier)) {
356357
id = db.getOrInsertFromEmail(identifier);
358+
} else if (UsernameUtil.isValidUsernameForSearch(identifier)) {
359+
throw new IllegalArgumentException("Creating a recipient based on username alone is not supported!");
357360
} else {
358361
String e164 = PhoneNumberFormatter.get(context).format(identifier);
359362
id = db.getOrInsertFromE164(e164);

‎core-util/src/main/java/org/signal/core/util/SQLiteDatabaseExtensions.kt‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,12 @@ fun SupportSQLiteDatabase.getForeignKeys(): List<ForeignKeyConstraint> {
5151
.flatten()
5252
}
5353

54+
fun SupportSQLiteDatabase.areForeignKeyConstraintsEnabled(): Boolean {
55+
return this.query("PRAGMA foreign_keys", null).use { cursor ->
56+
cursor.moveToFirst() && cursor.getInt(0) != 0
57+
}
58+
}
59+
5460
fun SupportSQLiteDatabase.getIndexes(): List<Index> {
5561
return this.query("SELECT name, tbl_name FROM sqlite_master WHERE type='index' ORDER BY name ASC").readToList { cursor ->
5662
val indexName = cursor.requireNonNullString("name")

0 commit comments

Comments
 (0)