diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 2155d6f12..c21867fc7 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -449,8 +449,7 @@ public class WhisperServerService extends Application storeKemOneTimePreKeys(final UUID identifier, final byte deviceId, final List preKeys) { - final boolean enrolledInPagedKeys = experimentEnrollmentManager.isEnrolled(identifier, PAGED_KEYS_EXPERIMENT_NAME); - final CompletableFuture deleteOtherKeys = enrolledInPagedKeys - ? pqPreKeys.delete(identifier, deviceId) - : pagedPqPreKeys.delete(identifier, deviceId); - return deleteOtherKeys.thenCompose(ignored -> enrolledInPagedKeys - ? pagedPqPreKeys.store(identifier, deviceId, preKeys) - : pqPreKeys.store(identifier, deviceId, preKeys)); + // Unconditionally delete keys in old format keystore, then write to the pagedPqPreKeys store + return pqPreKeys.delete(identifier, deviceId) + .thenCompose(_ -> pagedPqPreKeys.store(identifier, deviceId, preKeys)); } @@ -133,14 +123,13 @@ public class KeysManager { @VisibleForTesting CompletableFuture> takePQ(final UUID identifier, final byte deviceId) { - final boolean enrolledInPagedKeys = experimentEnrollmentManager.isEnrolled(identifier, PAGED_KEYS_EXPERIMENT_NAME); - return tagTakePQ(pagedPqPreKeys.take(identifier, deviceId), PQSource.PAGE, enrolledInPagedKeys) + return tagTakePQ(pagedPqPreKeys.take(identifier, deviceId), PQSource.PAGE) .thenCompose(maybeSingleUsePreKey -> maybeSingleUsePreKey .map(ignored -> CompletableFuture.completedFuture(maybeSingleUsePreKey)) - .orElseGet(() -> tagTakePQ(pqPreKeys.take(identifier, deviceId), PQSource.ROW, enrolledInPagedKeys))) + .orElseGet(() -> tagTakePQ(pqPreKeys.take(identifier, deviceId), PQSource.ROW))) .thenCompose(maybeSingleUsePreKey -> maybeSingleUsePreKey .map(singleUsePreKey -> CompletableFuture.completedFuture(maybeSingleUsePreKey)) - .orElseGet(() -> tagTakePQ(pqLastResortKeys.find(identifier, deviceId), PQSource.LAST_RESORT, enrolledInPagedKeys))); + .orElseGet(() -> tagTakePQ(pqLastResortKeys.find(identifier, deviceId), PQSource.LAST_RESORT))); } private enum PQSource { @@ -148,7 +137,7 @@ public class KeysManager { ROW, LAST_RESORT } - private CompletableFuture> tagTakePQ(CompletableFuture> prekey, final PQSource source, final boolean enrolledInPagedKeys) { + private CompletableFuture> tagTakePQ(CompletableFuture> prekey, final PQSource source) { return prekey.thenApply(maybeSingleUsePreKey -> { final Optional maybeSourceTag = maybeSingleUsePreKey // If we found a PK, use this source tag @@ -156,10 +145,7 @@ public class KeysManager { // If we didn't and this is our last resort, we didn't find a PK .or(() -> source == PQSource.LAST_RESORT ? Optional.of("absent") : Optional.empty()); maybeSourceTag.ifPresent(sourceTag -> { - Metrics.counter(TAKE_PQ_NAME, - "source", sourceTag, - "enrolled", Boolean.toString(enrolledInPagedKeys)) - .increment(); + Metrics.counter(TAKE_PQ_NAME, "source", sourceTag).increment(); }); return maybeSingleUsePreKey; }); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java index ab20c8605..7561e03a4 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java @@ -231,8 +231,7 @@ record CommandDependencies( new RepeatedUseECSignedPreKeyStore(dynamoDbAsyncClient, configuration.getDynamoDbTables().getEcSignedPreKeys().getTableName()), new RepeatedUseKEMSignedPreKeyStore(dynamoDbAsyncClient, - configuration.getDynamoDbTables().getKemLastResortKeys().getTableName()), - experimentEnrollmentManager); + configuration.getDynamoDbTables().getKemLastResortKeys().getTableName())); MessagesDynamoDb messagesDynamoDb = new MessagesDynamoDb(dynamoDbClient, dynamoDbAsyncClient, configuration.getDynamoDbTables().getMessages().getTableName(), configuration.getDynamoDbTables().getMessages().getExpiration(), diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountCreationDeletionIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountCreationDeletionIntegrationTest.java index e9b452cd9..02c163da5 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountCreationDeletionIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountCreationDeletionIntegrationTest.java @@ -109,8 +109,7 @@ public class AccountCreationDeletionIntegrationTest { new RepeatedUseECSignedPreKeyStore(dynamoDbAsyncClient, DynamoDbExtensionSchema.Tables.REPEATED_USE_EC_SIGNED_PRE_KEYS.tableName()), new RepeatedUseKEMSignedPreKeyStore(dynamoDbAsyncClient, - DynamoDbExtensionSchema.Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName()), - mock(ExperimentEnrollmentManager.class)); + DynamoDbExtensionSchema.Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName())); final ClientPublicKeys clientPublicKeys = new ClientPublicKeys(DYNAMO_DB_EXTENSION.getDynamoDbAsyncClient(), DynamoDbExtensionSchema.Tables.CLIENT_PUBLIC_KEYS.tableName()); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java index ab667e7e7..ce7f8e56e 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java @@ -37,7 +37,6 @@ import org.whispersystems.textsecuregcm.controllers.MismatchedDevicesException; import org.whispersystems.textsecuregcm.entities.AccountAttributes; import org.whispersystems.textsecuregcm.entities.ECSignedPreKey; import org.whispersystems.textsecuregcm.entities.KEMSignedPreKey; -import org.whispersystems.textsecuregcm.experiment.ExperimentEnrollmentManager; import org.whispersystems.textsecuregcm.identity.IdentityType; import org.whispersystems.textsecuregcm.redis.FaultTolerantRedisClient; import org.whispersystems.textsecuregcm.redis.RedisClusterExtension; @@ -99,8 +98,7 @@ class AccountsManagerChangeNumberIntegrationTest { new RepeatedUseECSignedPreKeyStore(dynamoDbAsyncClient, DynamoDbExtensionSchema.Tables.REPEATED_USE_EC_SIGNED_PRE_KEYS.tableName()), new RepeatedUseKEMSignedPreKeyStore(dynamoDbAsyncClient, - DynamoDbExtensionSchema.Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName()), - mock(ExperimentEnrollmentManager.class)); + DynamoDbExtensionSchema.Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName())); final ClientPublicKeys clientPublicKeys = new ClientPublicKeys(DYNAMO_DB_EXTENSION.getDynamoDbAsyncClient(), DynamoDbExtensionSchema.Tables.CLIENT_PUBLIC_KEYS.tableName()); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java index c408462b6..b972a8bda 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java @@ -39,7 +39,6 @@ import org.junit.jupiter.api.extension.RegisterExtension; import org.mockito.Mockito; import org.whispersystems.textsecuregcm.auth.DisconnectionRequestManager; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; -import org.whispersystems.textsecuregcm.experiment.ExperimentEnrollmentManager; import org.whispersystems.textsecuregcm.redis.FaultTolerantRedisClient; import org.whispersystems.textsecuregcm.redis.RedisClusterExtension; import org.whispersystems.textsecuregcm.securestorage.SecureStorageClient; @@ -112,8 +111,7 @@ class AccountsManagerUsernameIntegrationTest { new RepeatedUseECSignedPreKeyStore(dynamoDbAsyncClient, DynamoDbExtensionSchema.Tables.REPEATED_USE_EC_SIGNED_PRE_KEYS.tableName()), new RepeatedUseKEMSignedPreKeyStore(dynamoDbAsyncClient, - DynamoDbExtensionSchema.Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName()), - mock(ExperimentEnrollmentManager.class)); + DynamoDbExtensionSchema.Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName())); accounts = Mockito.spy(new Accounts( Clock.systemUTC(), diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java index 4b403ab67..7e42a0161 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java @@ -37,7 +37,6 @@ import org.signal.libsignal.protocol.ecc.ECKeyPair; import org.whispersystems.textsecuregcm.auth.DisconnectionRequestManager; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; import org.whispersystems.textsecuregcm.entities.DeviceInfo; -import org.whispersystems.textsecuregcm.experiment.ExperimentEnrollmentManager; import org.whispersystems.textsecuregcm.identity.IdentityType; import org.whispersystems.textsecuregcm.redis.RedisClusterExtension; import org.whispersystems.textsecuregcm.redis.RedisServerExtension; @@ -107,8 +106,7 @@ public class AddRemoveDeviceIntegrationTest { new RepeatedUseECSignedPreKeyStore(dynamoDbAsyncClient, DynamoDbExtensionSchema.Tables.REPEATED_USE_EC_SIGNED_PRE_KEYS.tableName()), new RepeatedUseKEMSignedPreKeyStore(dynamoDbAsyncClient, - DynamoDbExtensionSchema.Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName()), - mock(ExperimentEnrollmentManager.class)); + DynamoDbExtensionSchema.Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName())); final ClientPublicKeys clientPublicKeys = new ClientPublicKeys(DYNAMO_DB_EXTENSION.getDynamoDbAsyncClient(), DynamoDbExtensionSchema.Tables.CLIENT_PUBLIC_KEYS.tableName()); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/KeysManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/KeysManagerTest.java index 33879b155..83b31e668 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/KeysManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/KeysManagerTest.java @@ -8,9 +8,6 @@ package org.whispersystems.textsecuregcm.storage; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; import java.util.List; import java.util.Optional; @@ -20,12 +17,10 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.EnumSource; -import org.junit.jupiter.params.provider.ValueSource; import org.signal.libsignal.protocol.ecc.ECKeyPair; import org.whispersystems.textsecuregcm.entities.ECPreKey; import org.whispersystems.textsecuregcm.entities.ECSignedPreKey; import org.whispersystems.textsecuregcm.entities.KEMSignedPreKey; -import org.whispersystems.textsecuregcm.experiment.ExperimentEnrollmentManager; import org.whispersystems.textsecuregcm.identity.AciServiceIdentifier; import org.whispersystems.textsecuregcm.storage.DynamoDbExtensionSchema.Tables; import org.whispersystems.textsecuregcm.tests.util.KeysHelper; @@ -34,7 +29,6 @@ import software.amazon.awssdk.services.dynamodb.DynamoDbAsyncClient; class KeysManagerTest { private KeysManager keysManager; - private ExperimentEnrollmentManager experimentEnrollmentManager; private SingleUseKEMPreKeyStore singleUseKEMPreKeyStore; private PagedSingleUseKEMPreKeyStore pagedSingleUseKEMPreKeyStore; @@ -56,7 +50,6 @@ class KeysManagerTest { @BeforeEach void setup() { final DynamoDbAsyncClient dynamoDbAsyncClient = DYNAMO_DB_EXTENSION.getDynamoDbAsyncClient(); - experimentEnrollmentManager = mock(ExperimentEnrollmentManager.class); singleUseKEMPreKeyStore = new SingleUseKEMPreKeyStore(dynamoDbAsyncClient, Tables.PQ_KEYS.tableName()); pagedSingleUseKEMPreKeyStore = new PagedSingleUseKEMPreKeyStore(dynamoDbAsyncClient, S3_EXTENSION.getS3Client(), @@ -68,8 +61,7 @@ class KeysManagerTest { singleUseKEMPreKeyStore, pagedSingleUseKEMPreKeyStore, new RepeatedUseECSignedPreKeyStore(dynamoDbAsyncClient, Tables.REPEATED_USE_EC_SIGNED_PRE_KEYS.tableName()), - new RepeatedUseKEMSignedPreKeyStore(dynamoDbAsyncClient, Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName()), - experimentEnrollmentManager); + new RepeatedUseKEMSignedPreKeyStore(dynamoDbAsyncClient, Tables.REPEATED_USE_KEM_SIGNED_PRE_KEYS.tableName())); } @Test @@ -85,55 +77,38 @@ class KeysManagerTest { "Repeatedly storing same key should have no effect"); } - @ParameterizedTest - @ValueSource(booleans = {true, false}) - void storeKemOneTimePreKeysClearsOld(boolean inPagedExperiment) { + @Test + void storeKemOneTimePreKeysClearsOld() { final List oldPreKeys = List.of(generateTestKEMSignedPreKey(1)); - // Leave a key in the 'other' key store - (inPagedExperiment - ? singleUseKEMPreKeyStore.store(ACCOUNT_UUID, DEVICE_ID, oldPreKeys) - : pagedSingleUseKEMPreKeyStore.store(ACCOUNT_UUID, DEVICE_ID, oldPreKeys)) - .join(); - - when(experimentEnrollmentManager.isEnrolled(ACCOUNT_UUID, KeysManager.PAGED_KEYS_EXPERIMENT_NAME)) - .thenReturn(inPagedExperiment); - + // Leave a key in the 'old' key store + singleUseKEMPreKeyStore.store(ACCOUNT_UUID, DEVICE_ID, oldPreKeys).join(); final List newPreKeys = List.of(generateTestKEMSignedPreKey(2)); keysManager.storeKemOneTimePreKeys(ACCOUNT_UUID, DEVICE_ID, newPreKeys).join(); - final int expectedPagedKeyCount = inPagedExperiment ? 1 : 0; - final int expectedUnpagedKeyCount = 1 - expectedPagedKeyCount; assertEquals(1, keysManager.getPqCount(ACCOUNT_UUID, DEVICE_ID).join()); - assertEquals(expectedPagedKeyCount, pagedSingleUseKEMPreKeyStore.getCount(ACCOUNT_UUID, DEVICE_ID).join()); - assertEquals(expectedUnpagedKeyCount, singleUseKEMPreKeyStore.getCount(ACCOUNT_UUID, DEVICE_ID).join()); + assertEquals(1, pagedSingleUseKEMPreKeyStore.getCount(ACCOUNT_UUID, DEVICE_ID).join()); + assertEquals(0, singleUseKEMPreKeyStore.getCount(ACCOUNT_UUID, DEVICE_ID).join()); final KEMSignedPreKey key = keysManager.takePQ(ACCOUNT_UUID, DEVICE_ID).join().orElseThrow(); assertEquals(2, key.keyId()); } - @ParameterizedTest - @ValueSource(booleans = {true, false}) - void storeKemOneTimePreKeys(boolean inPagedExperiment) { + @Test + void storeKemOneTimePreKeys() { assertEquals(0, keysManager.getPqCount(ACCOUNT_UUID, DEVICE_ID).join(), "Initial pre-key count for an account should be zero"); - when(experimentEnrollmentManager.isEnrolled(ACCOUNT_UUID, KeysManager.PAGED_KEYS_EXPERIMENT_NAME)) - .thenReturn(inPagedExperiment); - - final int expectedPagedKeyCount = inPagedExperiment ? 1 : 0; - final int expectedUnpagedKeyCount = 1 - expectedPagedKeyCount; + keysManager.storeKemOneTimePreKeys(ACCOUNT_UUID, DEVICE_ID, List.of(generateTestKEMSignedPreKey(1))).join(); + assertEquals(1, keysManager.getPqCount(ACCOUNT_UUID, DEVICE_ID).join()); + assertEquals(1, pagedSingleUseKEMPreKeyStore.getCount(ACCOUNT_UUID, DEVICE_ID).join()); + assertEquals(0, singleUseKEMPreKeyStore.getCount(ACCOUNT_UUID, DEVICE_ID).join()); keysManager.storeKemOneTimePreKeys(ACCOUNT_UUID, DEVICE_ID, List.of(generateTestKEMSignedPreKey(1))).join(); assertEquals(1, keysManager.getPqCount(ACCOUNT_UUID, DEVICE_ID).join()); - assertEquals(expectedPagedKeyCount, pagedSingleUseKEMPreKeyStore.getCount(ACCOUNT_UUID, DEVICE_ID).join()); - assertEquals(expectedUnpagedKeyCount, singleUseKEMPreKeyStore.getCount(ACCOUNT_UUID, DEVICE_ID).join()); - - keysManager.storeKemOneTimePreKeys(ACCOUNT_UUID, DEVICE_ID, List.of(generateTestKEMSignedPreKey(1))).join(); - assertEquals(1, keysManager.getPqCount(ACCOUNT_UUID, DEVICE_ID).join()); - assertEquals(expectedPagedKeyCount, pagedSingleUseKEMPreKeyStore.getCount(ACCOUNT_UUID, DEVICE_ID).join()); - assertEquals(expectedUnpagedKeyCount, singleUseKEMPreKeyStore.getCount(ACCOUNT_UUID, DEVICE_ID).join()); + assertEquals(1, pagedSingleUseKEMPreKeyStore.getCount(ACCOUNT_UUID, DEVICE_ID).join()); + assertEquals(0, singleUseKEMPreKeyStore.getCount(ACCOUNT_UUID, DEVICE_ID).join()); } @@ -196,14 +171,10 @@ class KeysManagerTest { assertEquals(0, keysManager.getPqCount(ACCOUNT_UUID, DEVICE_ID).join()); } - @ParameterizedTest - @ValueSource(booleans = {true, false}) - void testDeleteSingleUsePreKeysByAccount(final boolean inPagedExperiment) { + @Test + void testDeleteSingleUsePreKeysByAccount() { int keyId = 1; - when(experimentEnrollmentManager.isEnrolled(ACCOUNT_UUID, KeysManager.PAGED_KEYS_EXPERIMENT_NAME)) - .thenReturn(inPagedExperiment); - for (byte deviceId : new byte[] {DEVICE_ID, DEVICE_ID + 1}) { keysManager.storeEcOneTimePreKeys(ACCOUNT_UUID, deviceId, List.of(generateTestPreKey(keyId++))).join(); keysManager.storeKemOneTimePreKeys(ACCOUNT_UUID, deviceId, List.of(generateTestKEMSignedPreKey(keyId++))).join(); @@ -228,14 +199,10 @@ class KeysManagerTest { } } - @ParameterizedTest - @ValueSource(booleans = {true, false}) - void testDeleteSingleUsePreKeysByAccountAndDevice(final boolean inPagedExperiment) { + @Test + void testDeleteSingleUsePreKeysByAccountAndDevice() { int keyId = 1; - when(experimentEnrollmentManager.isEnrolled(ACCOUNT_UUID, KeysManager.PAGED_KEYS_EXPERIMENT_NAME)) - .thenReturn(inPagedExperiment); - for (byte deviceId : new byte[] {DEVICE_ID, DEVICE_ID + 1}) { keysManager.storeEcOneTimePreKeys(ACCOUNT_UUID, deviceId, List.of(generateTestPreKey(keyId++))).join(); keysManager.storeKemOneTimePreKeys(ACCOUNT_UUID, deviceId, List.of(generateTestKEMSignedPreKey(keyId++))).join();