From 26f5ffdde34ec2941f6c6c1f07be9ee3380d2569 Mon Sep 17 00:00:00 2001 From: Katherine Yen Date: Tue, 13 Dec 2022 07:59:37 -0800 Subject: [PATCH] Enable case-sensitive usernames --- .../textsecuregcm/storage/Accounts.java | 20 +++++----- .../storage/AccountsManager.java | 7 ++-- .../textsecuregcm/util/UsernameGenerator.java | 11 +++--- .../util/UsernameNormalizer.java | 10 +++++ .../controllers/AccountControllerTest.java | 16 ++++---- .../storage/AccountsManagerTest.java | 3 +- .../textsecuregcm/storage/AccountsTest.java | 39 ++++++++++++------- .../tests/util/UsernameGeneratorTest.java | 6 +-- .../tests/util/UsernameNormalizerTest.java | 17 ++++++++ 9 files changed, 83 insertions(+), 46 deletions(-) create mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/util/UsernameNormalizer.java create mode 100644 service/src/test/java/org/whispersystems/textsecuregcm/tests/util/UsernameNormalizerTest.java 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 d4f3d8fa2..b862957fa 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java @@ -39,6 +39,7 @@ import org.whispersystems.textsecuregcm.util.AttributeValues; import org.whispersystems.textsecuregcm.util.ExceptionUtils; import org.whispersystems.textsecuregcm.util.SystemMapper; import org.whispersystems.textsecuregcm.util.UUIDUtil; +import org.whispersystems.textsecuregcm.util.UsernameNormalizer; import software.amazon.awssdk.services.dynamodb.DynamoDbAsyncClient; import software.amazon.awssdk.services.dynamodb.DynamoDbClient; import software.amazon.awssdk.services.dynamodb.model.AttributeValue; @@ -304,7 +305,6 @@ public class Accounts extends AbstractDynamoDbStore { final String reservedUsername, final Duration ttl) { final long startNanos = System.nanoTime(); - // if there is an existing old reservation it will be cleaned up via ttl final Optional maybeOriginalReservation = account.getReservedUsernameHash(); account.setReservedUsernameHash(reservedUsernameHash(account.getUuid(), reservedUsername)); @@ -322,7 +322,7 @@ public class Accounts extends AbstractDynamoDbStore { .tableName(usernamesConstraintTableName) .item(Map.of( KEY_ACCOUNT_UUID, AttributeValues.fromUUID(reservationToken), - ATTR_USERNAME, AttributeValues.fromString(reservedUsername), + ATTR_USERNAME, AttributeValues.fromString(UsernameNormalizer.normalize(reservedUsername)), ATTR_TTL, AttributeValues.fromLong(expirationTime))) .conditionExpression("attribute_not_exists(#username) OR (#ttl < :now)") .expressionAttributeNames(Map.of("#username", ATTR_USERNAME, "#ttl", ATTR_TTL)) @@ -411,12 +411,13 @@ public class Accounts extends AbstractDynamoDbStore { final List writeItems = new ArrayList<>(); // add the username to the constraint table, wiping out the ttl if we had already reserved the name + // Persist the normalized username in the usernamesConstraint table and the original username in the accounts table writeItems.add(TransactWriteItem.builder() .put(Put.builder() .tableName(usernamesConstraintTableName) .item(Map.of( KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()), - ATTR_USERNAME, AttributeValues.fromString(username))) + ATTR_USERNAME, AttributeValues.fromString(UsernameNormalizer.normalize(username)))) // it's not in the constraint table OR it's expired OR it was reserved by us .conditionExpression("attribute_not_exists(#username) OR #ttl < :now OR #aci = :reservation ") .expressionAttributeNames(Map.of("#username", ATTR_USERNAME, "#ttl", ATTR_TTL, "#aci", KEY_ACCOUNT_UUID)) @@ -446,7 +447,7 @@ public class Accounts extends AbstractDynamoDbStore { .build()); maybeOriginalUsername.ifPresent(originalUsername -> writeItems.add( - buildDelete(usernamesConstraintTableName, ATTR_USERNAME, originalUsername))); + buildDelete(usernamesConstraintTableName, ATTR_USERNAME, UsernameNormalizer.normalize(originalUsername)))); final TransactWriteItemsRequest request = TransactWriteItemsRequest.builder() .transactItems(writeItems) @@ -499,7 +500,7 @@ public class Accounts extends AbstractDynamoDbStore { .build()) .build()); - writeItems.add(buildDelete(usernamesConstraintTableName, ATTR_USERNAME, username)); + writeItems.add(buildDelete(usernamesConstraintTableName, ATTR_USERNAME, UsernameNormalizer.normalize(username))); final TransactWriteItemsRequest request = TransactWriteItemsRequest.builder() .transactItems(writeItems) @@ -606,7 +607,7 @@ public class Accounts extends AbstractDynamoDbStore { public boolean usernameAvailable(final Optional reservationToken, final String username) { final Optional> usernameItem = itemByKey( - usernamesConstraintTableName, ATTR_USERNAME, AttributeValues.fromString(username)); + usernamesConstraintTableName, ATTR_USERNAME, AttributeValues.fromString(UsernameNormalizer.normalize(username))); if (usernameItem.isEmpty()) { // username is free @@ -643,7 +644,7 @@ public class Accounts extends AbstractDynamoDbStore { GET_BY_USERNAME_TIMER, usernamesConstraintTableName, ATTR_USERNAME, - AttributeValues.fromString(username), + AttributeValues.fromString(UsernameNormalizer.normalize(username)), item -> !item.containsKey(ATTR_TTL) // ignore items with a ttl (reservations) ); } @@ -665,11 +666,10 @@ public class Accounts extends AbstractDynamoDbStore { )); account.getUsername().ifPresent(username -> transactWriteItems.add( - buildDelete(usernamesConstraintTableName, ATTR_USERNAME, username))); + buildDelete(usernamesConstraintTableName, ATTR_USERNAME, UsernameNormalizer.normalize(username)))); final TransactWriteItemsRequest request = TransactWriteItemsRequest.builder() .transactItems(transactWriteItems).build(); - db().transactWriteItems(request); })); } @@ -852,7 +852,7 @@ public class Accounts extends AbstractDynamoDbStore { throw new AssertionError(e); } final ByteBuffer byteBuffer = ByteBuffer.allocate(32 + 1); - sha256.update(reservedUsername.getBytes(StandardCharsets.UTF_8)); + sha256.update(UsernameNormalizer.normalize(reservedUsername).getBytes(StandardCharsets.UTF_8)); sha256.update(UUIDUtil.toBytes(accountId)); byteBuffer.put(RESERVED_USERNAME_HASH_VERSION); byteBuffer.put(sha256.digest()); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java index 2bbc1868d..cc88f3fc5 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -51,6 +51,7 @@ import org.whispersystems.textsecuregcm.util.Constants; import org.whispersystems.textsecuregcm.util.DestinationDeviceValidator; import org.whispersystems.textsecuregcm.util.SystemMapper; import org.whispersystems.textsecuregcm.util.UsernameGenerator; +import org.whispersystems.textsecuregcm.util.UsernameNormalizer; import org.whispersystems.textsecuregcm.util.Util; public class AccountsManager { @@ -391,7 +392,7 @@ public class AccountsManager { return account; } - final byte[] newHash = Accounts.reservedUsernameHash(account.getUuid(), reservedUsername); + final byte[] newHash = Accounts.reservedUsernameHash(account.getUuid(), UsernameNormalizer.normalize(reservedUsername)); if (!account.getReservedUsernameHash().map(oldHash -> Arrays.equals(oldHash, newHash)).orElse(false)) { // no such reservation existed, either there was no previous call to reserveUsername // or the reservation changed @@ -720,8 +721,8 @@ public class AccountsManager { clientPresenceManager.disconnectPresence(account.getUuid(), device.getId()))); } - private String getUsernameAccountMapKey(String key) { - return "UAccountMap::" + key; + private String getUsernameAccountMapKey(String username) { + return "UAccountMap::" + UsernameNormalizer.normalize(username); } private String getAccountMapKey(String key) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/util/UsernameGenerator.java b/service/src/main/java/org/whispersystems/textsecuregcm/util/UsernameGenerator.java index 283efd80f..0a1ff3168 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/util/UsernameGenerator.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/util/UsernameGenerator.java @@ -22,18 +22,17 @@ import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name; public class UsernameGenerator { /** - * Nicknames are + * Nicknames * - *
  • lowercase
    • *
  • do not start with a number
    • - *
  • alphanumeric or underscores only
    • - *
  • minimum length 3
    • - *
  • maximum length 32
    • + *
  • are alphanumeric or underscores only
    • + *
  • have minimum length 3
    • + *
  • have maximum length 32
    • *     * * Usernames typically consist of a nickname and an integer discriminator */ - public static final Pattern NICKNAME_PATTERN = Pattern.compile("^[_a-z][_a-z0-9]{2,31}$"); + public static final Pattern NICKNAME_PATTERN = Pattern.compile("^[_a-zA-Z][_a-zA-Z0-9]{2,31}$"); public static final String SEPARATOR = "."; private static final Counter USERNAME_NOT_AVAILABLE_COUNTER = Metrics.counter(name(UsernameGenerator.class, "usernameNotAvailable")); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/util/UsernameNormalizer.java b/service/src/main/java/org/whispersystems/textsecuregcm/util/UsernameNormalizer.java new file mode 100644 index 000000000..1d0ecaf6c --- /dev/null +++ b/service/src/main/java/org/whispersystems/textsecuregcm/util/UsernameNormalizer.java @@ -0,0 +1,10 @@ +package org.whispersystems.textsecuregcm.util; + +import java.util.Locale; + +public final class UsernameNormalizer { + private UsernameNormalizer() {} + public static String normalize(final String username) { + return username.toLowerCase(Locale.ROOT); + } +} diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java index 93aeebd90..c5f0911b6 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java @@ -1496,32 +1496,32 @@ class AccountControllerTest { @Test void testSetUsername() throws UsernameNotAvailableException { Account account = mock(Account.class); - when(account.getUsername()).thenReturn(Optional.of("n00bkiller.1234")); - when(accountsManager.setUsername(any(), eq("n00bkiller"), isNull())) + when(account.getUsername()).thenReturn(Optional.of("N00bkilleR.1234")); + when(accountsManager.setUsername(any(), eq("N00bkilleR"), isNull())) .thenReturn(account); Response response = resources.getJerseyTest() .target("/v1/accounts/username") .request() .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) - .put(Entity.json(new UsernameRequest("n00bkiller", null))); + .put(Entity.json(new UsernameRequest("N00bkilleR", null))); assertThat(response.getStatus()).isEqualTo(200); - assertThat(response.readEntity(UsernameResponse.class).username()).isEqualTo("n00bkiller.1234"); + assertThat(response.readEntity(UsernameResponse.class).username()).isEqualTo("N00bkilleR.1234"); } @Test void testReserveUsername() throws UsernameNotAvailableException { - when(accountsManager.reserveUsername(any(), eq("n00bkiller"))) - .thenReturn(new AccountsManager.UsernameReservation(null, "n00bkiller.1234", RESERVATION_TOKEN)); + when(accountsManager.reserveUsername(any(), eq("N00bkilleR"))) + .thenReturn(new AccountsManager.UsernameReservation(null, "N00bkilleR.1234", RESERVATION_TOKEN)); Response response = resources.getJerseyTest() .target("/v1/accounts/username/reserved") .request() .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) - .put(Entity.json(new ReserveUsernameRequest("n00bkiller"))); + .put(Entity.json(new ReserveUsernameRequest("N00bkilleR"))); assertThat(response.getStatus()).isEqualTo(200); assertThat(response.readEntity(ReserveUsernameResponse.class)) - .satisfies(r -> r.username().equals("n00bkiller.1234")) + .satisfies(r -> r.username().equals("N00bkilleR.1234")) .satisfies(r -> r.reservationToken().equals(RESERVATION_TOKEN)); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java index c7f463b60..5c4a10403 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java @@ -63,6 +63,7 @@ import org.whispersystems.textsecuregcm.storage.Device.DeviceCapabilities; import org.whispersystems.textsecuregcm.tests.util.AccountsHelper; import org.whispersystems.textsecuregcm.tests.util.RedisClusterHelper; import org.whispersystems.textsecuregcm.util.UsernameGenerator; +import org.whispersystems.textsecuregcm.util.UsernameNormalizer; class AccountsManagerTest { @@ -751,7 +752,7 @@ class AccountsManagerTest { @Test void testSetReservedUsername() throws UsernameNotAvailableException, UsernameReservationNotFoundException { final Account account = AccountsHelper.generateTestAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID(), new ArrayList<>(), new byte[16]); - final String reserved = "scooby.1234"; + final String reserved = "sCoObY.1234"; setReservationHash(account, reserved); when(accounts.usernameAvailable(eq(Optional.of(RESERVATION_TOKEN)), eq(reserved))).thenReturn(true); accountsManager.confirmReservedUsername(account, reserved, RESERVATION_TOKEN); 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 01bc3474d..773174c84 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -43,6 +43,7 @@ import org.whispersystems.textsecuregcm.util.TestClock; import software.amazon.awssdk.services.dynamodb.DynamoDbAsyncClient; import software.amazon.awssdk.services.dynamodb.DynamoDbClient; import software.amazon.awssdk.services.dynamodb.model.AttributeDefinition; +import software.amazon.awssdk.services.dynamodb.model.AttributeValue; import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException; import software.amazon.awssdk.services.dynamodb.model.CreateTableRequest; import software.amazon.awssdk.services.dynamodb.model.GetItemRequest; @@ -617,7 +618,7 @@ class AccountsTest { final Account account = generateAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID()); accounts.create(account); - final String username = "test"; + final String username = "TeST"; assertThat(accounts.getByUsername(username)).isEmpty(); @@ -638,6 +639,12 @@ class AccountsTest { accounts.setUsername(account, secondUsername); assertThat(accounts.getByUsername(username)).isEmpty(); + assertThat(dynamoDbExtension.getDynamoDbClient() + .getItem(GetItemRequest.builder() + .tableName(USERNAME_CONSTRAINT_TABLE_NAME) + .key(Map.of(Accounts.ATTR_USERNAME, AttributeValues.fromString("test"))) + .build()) + .item()).isEmpty(); { final Optional maybeAccount = accounts.getByUsername(secondUsername); @@ -688,7 +695,7 @@ class AccountsTest { final Account account = generateAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID()); accounts.create(account); - final String username = "test"; + final String username = "TeST"; accounts.setUsername(account, username); assertThat(accounts.getByUsername(username)).isPresent(); @@ -731,29 +738,31 @@ class AccountsTest { final Account account2 = generateAccount("+18005552222", UUID.randomUUID(), UUID.randomUUID()); accounts.create(account2); - final UUID token = accounts.reserveUsername(account1, "garfield", Duration.ofDays(1)); - assertThat(account1.getReservedUsernameHash()).get().isEqualTo(Accounts.reservedUsernameHash(account1.getUuid(), "garfield")); + final UUID token = accounts.reserveUsername(account1, "GarfielD", Duration.ofDays(1)); + assertThat(account1.getReservedUsernameHash()).get().isEqualTo(Accounts.reservedUsernameHash(account1.getUuid(), "GarfielD")); assertThat(account1.getUsername()).isEmpty(); - // account 2 shouldn't be able to reserve the username + // account 2 shouldn't be able to reserve the username if it's the same when normalized assertThrows(ContestedOptimisticLockException.class, - () -> accounts.reserveUsername(account2, "garfield", Duration.ofDays(1))); + () -> accounts.reserveUsername(account2, "gARFIELd", Duration.ofDays(1))); assertThrows(ContestedOptimisticLockException.class, - () -> accounts.confirmUsername(account2, "garfield", UUID.randomUUID())); - assertThat(accounts.getByUsername("garfield")).isEmpty(); + () -> accounts.confirmUsername(account2, "gARFIELd", UUID.randomUUID())); + assertThat(accounts.getByUsername("gARFIELd")).isEmpty(); - accounts.confirmUsername(account1, "garfield", token); + accounts.confirmUsername(account1, "GarfielD", token); assertThat(account1.getReservedUsernameHash()).isEmpty(); - assertThat(account1.getUsername()).get().isEqualTo("garfield"); - assertThat(accounts.getByUsername("garfield").get().getUuid()).isEqualTo(account1.getUuid()); + assertThat(account1.getUsername()).get().isEqualTo("GarfielD"); + assertThat(accounts.getByUsername("GarfielD").get().getUuid()).isEqualTo(account1.getUuid()); - assertThat(dynamoDbExtension.getDynamoDbClient() + final Map usernameConstraintRecord = dynamoDbExtension.getDynamoDbClient() .getItem(GetItemRequest.builder() .tableName(USERNAME_CONSTRAINT_TABLE_NAME) .key(Map.of(Accounts.ATTR_USERNAME, AttributeValues.fromString("garfield"))) .build()) - .item()) - .doesNotContainKey(Accounts.ATTR_TTL); + .item(); + + assertThat(usernameConstraintRecord).containsKey(Accounts.ATTR_USERNAME); + assertThat(usernameConstraintRecord).doesNotContainKey(Accounts.ATTR_TTL); } @Test @@ -761,7 +770,7 @@ class AccountsTest { final Account account1 = generateAccount("+18005551111", UUID.randomUUID(), UUID.randomUUID()); accounts.create(account1); - final String username = "unsinkablesam"; + final String username = "UnSinkaBlesam"; final UUID token = accounts.reserveUsername(account1, username, Duration.ofDays(1)); assertThat(accounts.usernameAvailable(username)).isFalse(); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/UsernameGeneratorTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/UsernameGeneratorTest.java index 1332fe435..d494d76ed 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/UsernameGeneratorTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/UsernameGeneratorTest.java @@ -28,8 +28,8 @@ public class UsernameGeneratorTest { static Stream nicknameValidation() { return Stream.of( - Arguments.of("Test", false, "upper case"), - Arguments.of("tesT", false, "upper case"), + Arguments.of("Test", true, "upper case"), + Arguments.of("tesT", true, "upper case"), Arguments.of("te-st", false, "illegal character"), Arguments.of("ab\uD83D\uDC1B", false, "illegal character"), Arguments.of("1test", false, "illegal start"), @@ -55,7 +55,7 @@ public class UsernameGeneratorTest { static Stream nonStandardUsernames() { return Stream.of( - Arguments.of("Test.123", false), + Arguments.of("Test.123", true), Arguments.of("test.-123", false), Arguments.of("test.0", false), Arguments.of("test.", false), diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/UsernameNormalizerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/UsernameNormalizerTest.java new file mode 100644 index 000000000..432b318fe --- /dev/null +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/UsernameNormalizerTest.java @@ -0,0 +1,17 @@ +package org.whispersystems.textsecuregcm.tests.util; + +import org.junit.jupiter.api.Test; +import org.whispersystems.textsecuregcm.util.UsernameNormalizer; + +import static org.assertj.core.api.Assertions.assertThat; + +public class UsernameNormalizerTest { + + @Test + public void usernameNormalization() { + assertThat(UsernameNormalizer.normalize("TeST")).isEqualTo("test"); + assertThat(UsernameNormalizer.normalize("TeST_")).isEqualTo("test_"); + assertThat(UsernameNormalizer.normalize("TeST_.123")).isEqualTo("test_.123"); + } + +}