diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java index 21155d323..39a7f7d64 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java @@ -474,9 +474,12 @@ public class Accounts extends AbstractDynamoDbStore { ATTR_TTL, AttributeValues.fromLong(expirationTime), ATTR_CONFIRMED, AttributeValues.fromBool(false), ATTR_RECLAIMABLE, AttributeValues.fromBool(false))) - .conditionExpression("attribute_not_exists(#username_hash) OR (#ttl < :now)") - .expressionAttributeNames(Map.of("#username_hash", ATTR_USERNAME_HASH, "#ttl", ATTR_TTL)) - .expressionAttributeValues(Map.of(":now", AttributeValues.fromLong(clock.instant().getEpochSecond()))) + .conditionExpression("attribute_not_exists(#username_hash) OR #ttl < :now OR (#aci = :aci AND #confirmed = :confirmed)") + .expressionAttributeNames(Map.of("#username_hash", ATTR_USERNAME_HASH, "#ttl", ATTR_TTL, "#aci", KEY_ACCOUNT_UUID, "#confirmed", ATTR_CONFIRMED)) + .expressionAttributeValues(Map.of( + ":now", AttributeValues.fromLong(clock.instant().getEpochSecond()), + ":aci", AttributeValues.fromUUID(uuid), + ":confirmed", AttributeValues.fromBool(false))) .returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD) .build()) .build()); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java index 4adb16617..f1280d6d9 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -910,12 +910,7 @@ class AccountsTest { final UUID newHandle = account.getUsernameLinkHandle(); assertThat(accounts.getByUsernameHash(USERNAME_HASH_1).join()).isEmpty(); - assertThat(DYNAMO_DB_EXTENSION.getDynamoDbClient() - .getItem(GetItemRequest.builder() - .tableName(Tables.USERNAMES.tableName()) - .key(Map.of(Accounts.ATTR_USERNAME_HASH, AttributeValues.fromByteArray(USERNAME_HASH_1))) - .build()) - .item()).isEmpty(); + assertThat(getUsernameConstraintTableItem(USERNAME_HASH_1)).isEmpty(); assertThat(accounts.getByUsernameLinkHandle(oldHandle).join()).isEmpty(); { @@ -1045,17 +1040,69 @@ class AccountsTest { assertArrayEquals(account1.getUsernameHash().orElseThrow(), USERNAME_HASH_1); assertThat(accounts.getByUsernameHash(USERNAME_HASH_1).join().get().getUuid()).isEqualTo(account1.getUuid()); - final Map usernameConstraintRecord = DYNAMO_DB_EXTENSION.getDynamoDbClient() - .getItem(GetItemRequest.builder() - .tableName(Tables.USERNAMES.tableName()) - .key(Map.of(Accounts.ATTR_USERNAME_HASH, AttributeValues.fromByteArray(USERNAME_HASH_1))) - .build()) - .item(); + final Map usernameConstraintRecord = getUsernameConstraintTableItem(USERNAME_HASH_1); assertThat(usernameConstraintRecord).containsKey(Accounts.ATTR_USERNAME_HASH); assertThat(usernameConstraintRecord).doesNotContainKey(Accounts.ATTR_TTL); } + @Test + void switchBetweenReservedUsernameHashes() { + final Account account = generateAccount("+18005551111", UUID.randomUUID(), UUID.randomUUID()); + createAccount(account); + + accounts.reserveUsernameHash(account, USERNAME_HASH_1, Duration.ofDays(1)).join(); + assertArrayEquals(account.getReservedUsernameHash().orElseThrow(), USERNAME_HASH_1); + assertThat(account.getUsernameHash()).isEmpty(); + + accounts.reserveUsernameHash(account, USERNAME_HASH_2, Duration.ofDays(1)).join(); + assertArrayEquals(account.getReservedUsernameHash().orElseThrow(), USERNAME_HASH_2); + assertThat(account.getUsernameHash()).isEmpty(); + + final Map usernameConstraintRecord1 = getUsernameConstraintTableItem(USERNAME_HASH_1); + final Map usernameConstraintRecord2 = getUsernameConstraintTableItem(USERNAME_HASH_2); + assertThat(usernameConstraintRecord1).containsKey(Accounts.ATTR_USERNAME_HASH); + assertThat(usernameConstraintRecord2).containsKey(Accounts.ATTR_USERNAME_HASH); + assertThat(usernameConstraintRecord1).containsKey(Accounts.ATTR_TTL); + assertThat(usernameConstraintRecord2).containsKey(Accounts.ATTR_TTL); + + clock.pin(Instant.EPOCH.plus(Duration.ofMinutes(1))); + + accounts.reserveUsernameHash(account, USERNAME_HASH_1, Duration.ofDays(1)).join(); + assertArrayEquals(account.getReservedUsernameHash().orElseThrow(), USERNAME_HASH_1); + assertThat(account.getUsernameHash()).isEmpty(); + + final Map newUsernameConstraintRecord1 = getUsernameConstraintTableItem(USERNAME_HASH_1); + assertThat(newUsernameConstraintRecord1).containsKey(Accounts.ATTR_USERNAME_HASH); + assertThat(newUsernameConstraintRecord1).containsKey(Accounts.ATTR_TTL); + assertThat(usernameConstraintRecord1.get(Accounts.ATTR_TTL)) + .isNotEqualTo(newUsernameConstraintRecord1.get(Accounts.ATTR_TTL)); + } + + @Test + void reserveOwnConfirmedUsername() { + final Account account = generateAccount("+18005551111", UUID.randomUUID(), UUID.randomUUID()); + createAccount(account); + + accounts.reserveUsernameHash(account, USERNAME_HASH_1, Duration.ofDays(1)).join(); + assertArrayEquals(account.getReservedUsernameHash().orElseThrow(), USERNAME_HASH_1); + assertThat(account.getUsernameHash()).isEmpty(); + assertThat(getUsernameConstraintTableItem(USERNAME_HASH_1)).containsKey(Accounts.ATTR_TTL); + + + accounts.confirmUsernameHash(account, USERNAME_HASH_1, ENCRYPTED_USERNAME_1).join(); + assertThat(account.getReservedUsernameHash()).isEmpty(); + assertArrayEquals(account.getUsernameHash().orElseThrow(), USERNAME_HASH_1); + assertThat(getUsernameConstraintTableItem(USERNAME_HASH_1)).doesNotContainKey(Accounts.ATTR_TTL); + + CompletableFutureTestUtil.assertFailsWithCause(ContestedOptimisticLockException.class, + accounts.reserveUsernameHash(account, USERNAME_HASH_1, Duration.ofDays(1))); + assertThat(account.getReservedUsernameHash()).isEmpty(); + assertArrayEquals(account.getUsernameHash().orElseThrow(), USERNAME_HASH_1); + assertThat(getUsernameConstraintTableItem(USERNAME_HASH_1)).containsKey(Accounts.ATTR_USERNAME_HASH); + assertThat(getUsernameConstraintTableItem(USERNAME_HASH_1)).doesNotContainKey(Accounts.ATTR_TTL); + } + @Test void testUsernameHashAvailable() { final Account account1 = generateAccount("+18005551111", UUID.randomUUID(), UUID.randomUUID()); @@ -1119,17 +1166,6 @@ class AccountsTest { assertThat(accounts.getByUsernameHash(USERNAME_HASH_1).join().get().getUuid()).isEqualTo(account2.getUuid()); } - @Test - void testRetryReserveUsernameHash() { - final Account account = generateAccount("+18005551111", UUID.randomUUID(), UUID.randomUUID()); - createAccount(account); - accounts.reserveUsernameHash(account, USERNAME_HASH_1, Duration.ofDays(2)).join(); - - CompletableFutureTestUtil.assertFailsWithCause(ContestedOptimisticLockException.class, - accounts.reserveUsernameHash(account, USERNAME_HASH_1, Duration.ofDays(2)), - "Shouldn't be able to re-reserve same username hash (would extend ttl)"); - } - @Test void testReserveConfirmUsernameHashVersionConflict() { final Account account = generateAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID()); @@ -1300,6 +1336,15 @@ class AccountsTest { return get.item(); } + private Map getUsernameConstraintTableItem(final byte[] usernameHash) { + return DYNAMO_DB_EXTENSION.getDynamoDbClient() + .getItem(GetItemRequest.builder() + .tableName(Tables.USERNAMES.tableName()) + .key(Map.of(Accounts.ATTR_USERNAME_HASH, AttributeValues.fromByteArray(usernameHash))) + .build()) + .item(); + } + private void verifyStoredState(String number, UUID uuid, UUID pni, byte[] usernameHash, Account expecting, boolean canonicallyDiscoverable) { final DynamoDbClient db = DYNAMO_DB_EXTENSION.getDynamoDbClient();