PM-23667: Optimize authenticator sync with totp database query (#5508)

This commit is contained in:
David Perez 2025-07-10 14:03:02 -05:00 committed by GitHub
parent 532fcbb40e
commit e193661f5f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 331 additions and 20 deletions

View File

@ -0,0 +1,252 @@
{
"formatVersion": 1,
"database": {
"version": 7,
"identityHash": "4c6ad1f5268d7e8add7407201788aa2e",
"entities": [
{
"tableName": "ciphers",
"createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `user_id` TEXT NOT NULL, `has_totp` INTEGER NOT NULL DEFAULT 1, `cipher_type` TEXT NOT NULL, `cipher_json` TEXT NOT NULL, PRIMARY KEY(`id`))",
"fields": [
{
"fieldPath": "id",
"columnName": "id",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "userId",
"columnName": "user_id",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "hasTotp",
"columnName": "has_totp",
"affinity": "INTEGER",
"notNull": true,
"defaultValue": "1"
},
{
"fieldPath": "cipherType",
"columnName": "cipher_type",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "cipherJson",
"columnName": "cipher_json",
"affinity": "TEXT",
"notNull": true
}
],
"primaryKey": {
"autoGenerate": false,
"columnNames": [
"id"
]
},
"indices": [
{
"name": "index_ciphers_user_id",
"unique": false,
"columnNames": [
"user_id"
],
"orders": [],
"createSql": "CREATE INDEX IF NOT EXISTS `index_ciphers_user_id` ON `${TABLE_NAME}` (`user_id`)"
}
]
},
{
"tableName": "collections",
"createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `user_id` TEXT NOT NULL, `organization_id` TEXT NOT NULL, `should_hide_passwords` INTEGER NOT NULL, `name` TEXT NOT NULL, `external_id` TEXT, `read_only` INTEGER NOT NULL, `manage` INTEGER, PRIMARY KEY(`id`))",
"fields": [
{
"fieldPath": "id",
"columnName": "id",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "userId",
"columnName": "user_id",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "organizationId",
"columnName": "organization_id",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "shouldHidePasswords",
"columnName": "should_hide_passwords",
"affinity": "INTEGER",
"notNull": true
},
{
"fieldPath": "name",
"columnName": "name",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "externalId",
"columnName": "external_id",
"affinity": "TEXT"
},
{
"fieldPath": "isReadOnly",
"columnName": "read_only",
"affinity": "INTEGER",
"notNull": true
},
{
"fieldPath": "canManage",
"columnName": "manage",
"affinity": "INTEGER"
}
],
"primaryKey": {
"autoGenerate": false,
"columnNames": [
"id"
]
},
"indices": [
{
"name": "index_collections_user_id",
"unique": false,
"columnNames": [
"user_id"
],
"orders": [],
"createSql": "CREATE INDEX IF NOT EXISTS `index_collections_user_id` ON `${TABLE_NAME}` (`user_id`)"
}
]
},
{
"tableName": "domains",
"createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`user_id` TEXT NOT NULL, `domains_json` TEXT, PRIMARY KEY(`user_id`))",
"fields": [
{
"fieldPath": "userId",
"columnName": "user_id",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "domainsJson",
"columnName": "domains_json",
"affinity": "TEXT"
}
],
"primaryKey": {
"autoGenerate": false,
"columnNames": [
"user_id"
]
}
},
{
"tableName": "folders",
"createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `user_id` TEXT NOT NULL, `name` TEXT, `revision_date` INTEGER NOT NULL, PRIMARY KEY(`id`))",
"fields": [
{
"fieldPath": "id",
"columnName": "id",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "userId",
"columnName": "user_id",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "name",
"columnName": "name",
"affinity": "TEXT"
},
{
"fieldPath": "revisionDate",
"columnName": "revision_date",
"affinity": "INTEGER",
"notNull": true
}
],
"primaryKey": {
"autoGenerate": false,
"columnNames": [
"id"
]
},
"indices": [
{
"name": "index_folders_user_id",
"unique": false,
"columnNames": [
"user_id"
],
"orders": [],
"createSql": "CREATE INDEX IF NOT EXISTS `index_folders_user_id` ON `${TABLE_NAME}` (`user_id`)"
}
]
},
{
"tableName": "sends",
"createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `user_id` TEXT NOT NULL, `send_type` TEXT NOT NULL, `send_json` TEXT NOT NULL, PRIMARY KEY(`id`))",
"fields": [
{
"fieldPath": "id",
"columnName": "id",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "userId",
"columnName": "user_id",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "sendType",
"columnName": "send_type",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "sendJson",
"columnName": "send_json",
"affinity": "TEXT",
"notNull": true
}
],
"primaryKey": {
"autoGenerate": false,
"columnNames": [
"id"
]
},
"indices": [
{
"name": "index_sends_user_id",
"unique": false,
"columnNames": [
"user_id"
],
"orders": [],
"createSql": "CREATE INDEX IF NOT EXISTS `index_sends_user_id` ON `${TABLE_NAME}` (`user_id`)"
}
]
}
],
"setupQueries": [
"CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)",
"INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, '4c6ad1f5268d7e8add7407201788aa2e')"
]
}
}

View File

@ -95,10 +95,9 @@ class AuthenticatorBridgeRepositoryImpl(
// Vault is unlocked, query vault disk source for totp logins:
val totpUris = vaultDiskSource
.getCiphersFlow(userId)
.first()
// Filter out any ciphers without a totp item and also deleted ciphers
.filter { it.login?.totp != null && it.deletedDate == null }
.getTotpCiphers(userId = userId)
// Filter out any deleted ciphers.
.filter { it.deletedDate == null }
.mapNotNull {
val decryptedCipher = vaultSdkSource
.decryptCipher(

View File

@ -24,6 +24,11 @@ interface VaultDiskSource {
*/
suspend fun getCiphers(userId: String): List<SyncResponseJson.Cipher>
/**
* Retrieves all ciphers from the data source for a given [userId] that contain TOTP codes.
*/
suspend fun getTotpCiphers(userId: String): List<SyncResponseJson.Cipher>
/**
* Retrieves a cipher from the data source for a given [userId] and [cipherId].
*/

View File

@ -52,6 +52,7 @@ class VaultDiskSourceImpl(
CipherEntity(
id = cipher.id,
userId = userId,
hasTotp = cipher.login?.totp != null,
cipherType = json.encodeToString(cipher.type),
cipherJson = json.encodeToString(cipher),
),
@ -96,6 +97,26 @@ class VaultDiskSourceImpl(
}
}
override suspend fun getTotpCiphers(userId: String): List<SyncResponseJson.Cipher> {
val entities = ciphersDao.getAllTotpCiphers(userId = userId)
return withContext(context = dispatcherManager.default) {
entities
.map { entity ->
async {
json.decodeFromStringWithErrorCallback<SyncResponseJson.Cipher>(
string = entity.cipherJson,
) { Timber.e(it, "Failed to deserialize TOTP Cipher in Vault") }
}
}
.awaitAll()
.filter {
// A safety-check since after the DB migration, we will temporarily think
// all ciphers contain a totp code
it.login?.totp != null
}
}
}
override suspend fun getCipher(
userId: String,
cipherId: String,
@ -249,6 +270,7 @@ class VaultDiskSourceImpl(
CipherEntity(
id = cipher.id,
userId = userId,
hasTotp = cipher.login?.totp != null,
cipherType = json.encodeToString(cipher.type),
cipherJson = json.encodeToString(cipher),
)

View File

@ -37,6 +37,14 @@ interface CiphersDao {
userId: String,
): List<CipherEntity>
/**
* Retrieves all ciphers from the database for a given [userId].
*/
@Query("SELECT * FROM ciphers WHERE user_id = :userId AND has_totp = 1")
suspend fun getAllTotpCiphers(
userId: String,
): List<CipherEntity>
/**
* Retrieves a cipher from the database for a given [userId] and [cipherId].
*/

View File

@ -1,5 +1,6 @@
package com.x8bit.bitwarden.data.vault.datasource.disk.database
import androidx.room.AutoMigration
import androidx.room.Database
import androidx.room.RoomDatabase
import androidx.room.TypeConverters
@ -26,8 +27,11 @@ import com.x8bit.bitwarden.data.vault.datasource.disk.entity.SendEntity
FolderEntity::class,
SendEntity::class,
],
version = 6,
version = 7,
exportSchema = true,
autoMigrations = [
AutoMigration(from = 6, to = 7),
],
)
@TypeConverters(ZonedDateTimeTypeConverter::class)
abstract class VaultDatabase : RoomDatabase() {

View File

@ -16,6 +16,11 @@ data class CipherEntity(
@ColumnInfo(name = "user_id", index = true)
val userId: String,
// Default to true for initial migration.
// Subsequent syncs will populate with correct values for optimizations.
@ColumnInfo(name = "has_totp", defaultValue = "1")
val hasTotp: Boolean,
@ColumnInfo(name = "cipher_type")
val cipherType: String,

View File

@ -27,7 +27,6 @@ import io.mockk.unmockkStatic
import io.mockk.verify
import kotlinx.coroutines.async
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.test.runTest
import org.junit.jupiter.api.AfterEach
import org.junit.jupiter.api.Assertions.assertEquals
@ -98,8 +97,8 @@ class AuthenticatorBridgeRepositoryTest {
// Add some ciphers to vaultDiskSource for each user,
// and setup mock decryption for them:
every { vaultDiskSource.getCiphersFlow(USER_1_ID) } returns flowOf(USER_1_CIPHERS)
every { vaultDiskSource.getCiphersFlow(USER_2_ID) } returns flowOf(USER_2_CIPHERS)
coEvery { vaultDiskSource.getTotpCiphers(USER_1_ID) } returns USER_1_CIPHERS
coEvery { vaultDiskSource.getTotpCiphers(USER_2_ID) } returns USER_2_CIPHERS
mockkStatic(SyncResponseJson.Cipher::toEncryptedSdkCipher)
every {
USER_1_TOTP_CIPHER.toEncryptedSdkCipher()
@ -137,8 +136,8 @@ class AuthenticatorBridgeRepositoryTest {
)
verify { authRepository.userStateFlow }
verify { vaultRepository.vaultUnlockDataStateFlow }
verify { vaultDiskSource.getCiphersFlow(USER_1_ID) }
verify { vaultDiskSource.getCiphersFlow(USER_2_ID) }
coVerify { vaultDiskSource.getTotpCiphers(USER_1_ID) }
coVerify { vaultDiskSource.getTotpCiphers(USER_2_ID) }
verify { vaultRepository.isVaultUnlocked(USER_1_ID) }
verify { vaultRepository.isVaultUnlocked(USER_2_ID) }
coVerify {
@ -186,7 +185,7 @@ class AuthenticatorBridgeRepositoryTest {
}
verify { vaultRepository.vaultUnlockDataStateFlow }
verify { vaultRepository.lockVault(USER_2_ID, isUserInitiated = false) }
verify { vaultDiskSource.getCiphersFlow(USER_2_ID) }
coVerify { vaultDiskSource.getTotpCiphers(USER_2_ID) }
coVerify { vaultSdkSource.decryptCipher(USER_2_ID, USER_2_ENCRYPTED_SDK_TOTP_CIPHER) }
}
@ -206,7 +205,7 @@ class AuthenticatorBridgeRepositoryTest {
sharedAccounts,
)
verify { vaultRepository.vaultUnlockDataStateFlow }
verify { vaultDiskSource.getCiphersFlow(USER_1_ID) }
coVerify { vaultDiskSource.getTotpCiphers(USER_1_ID) }
verify { vaultRepository.isVaultUnlocked(USER_1_ID) }
coVerify { vaultSdkSource.decryptCipher(USER_1_ID, USER_1_ENCRYPTED_SDK_TOTP_CIPHER) }
verify { authRepository.userStateFlow }
@ -226,7 +225,7 @@ class AuthenticatorBridgeRepositoryTest {
}
verify { vaultRepository.vaultUnlockDataStateFlow }
verify { vaultRepository.lockVault(USER_2_ID, isUserInitiated = false) }
verify { vaultDiskSource.getCiphersFlow(USER_2_ID) }
coVerify { vaultDiskSource.getTotpCiphers(USER_2_ID) }
coVerify { vaultSdkSource.decryptCipher(USER_2_ID, USER_2_ENCRYPTED_SDK_TOTP_CIPHER) }
}
@ -261,7 +260,7 @@ class AuthenticatorBridgeRepositoryTest {
}
verify { vaultRepository.vaultUnlockDataStateFlow }
verify { vaultRepository.lockVault(USER_2_ID, isUserInitiated = false) }
verify { vaultDiskSource.getCiphersFlow(USER_2_ID) }
coVerify { vaultDiskSource.getTotpCiphers(USER_2_ID) }
coVerify { vaultSdkSource.decryptCipher(USER_2_ID, USER_2_ENCRYPTED_SDK_TOTP_CIPHER) }
}
@ -282,10 +281,8 @@ class AuthenticatorBridgeRepositoryTest {
}
// None of these calls should happen until after user 1's vault state is not UNLOCKING:
verify(exactly = 0) {
vaultRepository.isVaultUnlocked(userId = USER_1_ID)
vaultDiskSource.getCiphersFlow(USER_1_ID)
}
verify(exactly = 0) { vaultRepository.isVaultUnlocked(userId = USER_1_ID) }
coVerify(exactly = 0) { vaultDiskSource.getTotpCiphers(USER_1_ID) }
// Then move out of UNLOCKING state, and things should proceed as normal:
vaultUnlockStateFlow.value = listOf(
@ -296,8 +293,8 @@ class AuthenticatorBridgeRepositoryTest {
deferred.await()
verify { authRepository.userStateFlow }
verify { vaultDiskSource.getCiphersFlow(USER_1_ID) }
verify { vaultDiskSource.getCiphersFlow(USER_2_ID) }
coVerify { vaultDiskSource.getTotpCiphers(USER_1_ID) }
coVerify { vaultDiskSource.getTotpCiphers(USER_2_ID) }
verify { vaultRepository.isVaultUnlocked(USER_1_ID) }
verify { vaultRepository.isVaultUnlocked(USER_2_ID) }
verify { vaultRepository.vaultUnlockDataStateFlow }

View File

@ -105,6 +105,21 @@ class VaultDiskSourceTest {
assertEquals(ciphers, result2)
}
@Test
fun `getTotpCiphers should return all CiphersDao totp ciphers`() = runTest {
val cipherEntities = listOf(
CIPHER_ENTITY,
CIPHER_ENTITY.copy(id = "otherCipherId", hasTotp = false),
)
val ciphers = listOf(CIPHER_1)
val result1 = vaultDiskSource.getTotpCiphers(USER_ID)
assertEquals(emptyList<SyncResponseJson.Cipher>(), result1)
ciphersDao.insertCiphers(cipherEntities)
val result2 = vaultDiskSource.getTotpCiphers(USER_ID)
assertEquals(ciphers, result2)
}
@Test
fun `getCipher should return CiphersDao cipher`() = runTest {
val cipherEntities = listOf(CIPHER_ENTITY)
@ -450,6 +465,7 @@ private const val CIPHER_JSON = """
private val CIPHER_ENTITY = CipherEntity(
id = "mockId-1",
userId = USER_ID,
hasTotp = true,
cipherType = "1",
cipherJson = CIPHER_JSON,
)

View File

@ -41,6 +41,9 @@ class FakeCiphersDao : CiphersDao {
override suspend fun getAllCiphers(userId: String): List<CipherEntity> =
storedCiphers.filter { it.userId == userId }
override suspend fun getAllTotpCiphers(userId: String): List<CipherEntity> =
storedCiphers.filter { it.userId == userId && it.hasTotp }
override suspend fun getCipher(userId: String, cipherId: String): CipherEntity? =
storedCiphers.find { it.userId == userId && it.id == cipherId }