From d0a8899daf3f9771bba87a3c1ea527b6c96d8d6b Mon Sep 17 00:00:00 2001 From: Ravi Khadiwala Date: Tue, 13 Sep 2022 17:10:34 -0500 Subject: [PATCH] Change discriminator seperator and default width --- .../configuration/UsernameConfiguration.java | 2 +- .../textsecuregcm/util/UsernameGenerator.java | 4 +-- .../storage/AccountsManagerTest.java | 29 ++++++++-------- ...ccountsManagerUsernameIntegrationTest.java | 2 +- .../textsecuregcm/storage/AccountsTest.java | 24 +++++++------ .../controllers/AccountControllerTest.java | 34 +++++++++---------- .../tests/util/UsernameGeneratorTest.java | 25 +++++++------- 7 files changed, 62 insertions(+), 58 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/UsernameConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/UsernameConfiguration.java index 40c3482a7..623287053 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/UsernameConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/UsernameConfiguration.java @@ -13,7 +13,7 @@ public class UsernameConfiguration { @JsonProperty @Min(1) - private int discriminatorInitialWidth = 4; + private int discriminatorInitialWidth = 2; @JsonProperty @Min(1) 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 d8000715a..283efd80f 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/util/UsernameGenerator.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/util/UsernameGenerator.java @@ -34,7 +34,7 @@ public class UsernameGenerator { * 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 String SEPARATOR = "#"; + public static final String SEPARATOR = "."; private static final Counter USERNAME_NOT_AVAILABLE_COUNTER = Metrics.counter(name(UsernameGenerator.class, "usernameNotAvailable")); private static final DistributionSummary DISCRIMINATOR_ATTEMPT_COUNTER = Metrics.summary(name(UsernameGenerator.class, "discriminatorAttempts")); @@ -112,7 +112,7 @@ public class UsernameGenerator { throw new IllegalArgumentException("Invalid nickname " + nickname); } // zero pad discriminators less than the discriminator initial width - return String.format("%s#%0" + initialWidth + "d", nickname, discriminator); + return String.format("%s%s%0" + initialWidth + "d", nickname, SEPARATOR, discriminator); } public Duration getReservationTtl() { 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 7b840ccb8..8dfacb199 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java @@ -39,6 +39,7 @@ import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.function.BiFunction; import java.util.function.Consumer; +import java.util.regex.Pattern; import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -750,7 +751,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); @@ -760,16 +761,16 @@ class AccountsManagerTest { @Test void testSetReservedHashNameMismatch() { final Account account = AccountsHelper.generateTestAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID(), new ArrayList<>(), new byte[16]); - setReservationHash(account, "pluto#1234"); - when(accounts.usernameAvailable(eq(Optional.of(RESERVATION_TOKEN)), eq("pluto#1234"))).thenReturn(true); + setReservationHash(account, "pluto.1234"); + when(accounts.usernameAvailable(eq(Optional.of(RESERVATION_TOKEN)), eq("pluto.1234"))).thenReturn(true); assertThrows(UsernameReservationNotFoundException.class, - () -> accountsManager.confirmReservedUsername(account, "goofy#1234", RESERVATION_TOKEN)); + () -> accountsManager.confirmReservedUsername(account, "goofy.1234", RESERVATION_TOKEN)); } @Test void testSetReservedHashAciMismatch() { final Account account = AccountsHelper.generateTestAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID(), new ArrayList<>(), new byte[16]); - final String reserved = "toto#1234"; + final String reserved = "toto.1234"; account.setReservedUsernameHash(Accounts.reservedUsernameHash(UUID.randomUUID(), reserved)); when(accounts.usernameAvailable(eq(Optional.of(RESERVATION_TOKEN)), eq(reserved))).thenReturn(true); assertThrows(UsernameReservationNotFoundException.class, @@ -779,7 +780,7 @@ class AccountsManagerTest { @Test void testSetReservedLapsed() { final Account account = AccountsHelper.generateTestAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID(), new ArrayList<>(), new byte[16]); - final String reserved = "porkchop#1234"; + final String reserved = "porkchop.1234"; // name was reserved, but the reservation lapsed and another account took it setReservationHash(account, reserved); when(accounts.usernameAvailable(eq(Optional.of(RESERVATION_TOKEN)), eq(reserved))).thenReturn(false); @@ -790,7 +791,7 @@ class AccountsManagerTest { @Test void testSetReservedRetry() throws UsernameNotAvailableException, UsernameReservationNotFoundException { final Account account = AccountsHelper.generateTestAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID(), new ArrayList<>(), new byte[16]); - final String username = "santaslittlehelper#1234"; + final String username = "santaslittlehelper.1234"; account.setUsername(username); // reserved username already set, should be treated as a replay @@ -802,7 +803,7 @@ class AccountsManagerTest { void testSetUsernameSameUsername() { final Account account = AccountsHelper.generateTestAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID(), new ArrayList<>(), new byte[16]); final String nickname = "test"; - account.setUsername(nickname + "#123"); + account.setUsername(nickname + ".123"); // should be treated as a replayed request assertDoesNotThrow(() -> accountsManager.setUsername(account, nickname, null)); @@ -813,7 +814,7 @@ class AccountsManagerTest { void testSetUsernameReroll() throws UsernameNotAvailableException { final Account account = AccountsHelper.generateTestAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID(), new ArrayList<>(), new byte[16]); final String nickname = "test"; - final String username = nickname + "#ZZZ"; + final String username = nickname + ".ZZZ"; account.setUsername(username); // given the correct old username, should reroll discriminator even if the nick matches @@ -825,7 +826,7 @@ class AccountsManagerTest { void testReserveUsernameReroll() throws UsernameNotAvailableException { final Account account = AccountsHelper.generateTestAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID(), new ArrayList<>(), new byte[16]); final String nickname = "clifford"; - final String username = nickname + "#ZZZ"; + final String username = nickname + ".ZZZ"; account.setUsername(username); // given the correct old username, should reroll discriminator even if the nick matches @@ -838,7 +839,7 @@ class AccountsManagerTest { final Account account = AccountsHelper.generateTestAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID(), new ArrayList<>(), new byte[16]); assertThrows(UsernameReservationNotFoundException.class, - () -> accountsManager.confirmReservedUsername(account, "laika#1234", RESERVATION_TOKEN)); + () -> accountsManager.confirmReservedUsername(account, "laika.1234", RESERVATION_TOKEN)); verify(accounts, never()).confirmUsername(any(), any(), any()); } @@ -849,7 +850,7 @@ class AccountsManagerTest { final String nickname = "test"; ArgumentMatcher isWide = (String username) -> { - String[] spl = username.split(UsernameGenerator.SEPARATOR); + String[] spl = username.split(Pattern.quote(UsernameGenerator.SEPARATOR)); assertEquals(spl.length, 2); int discriminator = Integer.parseInt(spl[1]); // require a 7 digit discriminator @@ -872,8 +873,8 @@ class AccountsManagerTest { void testChangeUsername() throws UsernameNotAvailableException { final Account account = AccountsHelper.generateTestAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID(), new ArrayList<>(), new byte[16]); final String nickname = "test"; - account.setUsername("old#123"); - accountsManager.setUsername(account, nickname, "old#123"); + account.setUsername("old.123"); + accountsManager.setUsername(account, nickname, "old.123"); verify(accounts).setUsername(eq(account), startsWith(nickname)); } 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 c8f7d6257..3e518575f 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java @@ -171,7 +171,7 @@ class AccountsManagerUsernameIntegrationTest { } private static int discriminator(String username) { - return Integer.parseInt(username.split(UsernameGenerator.SEPARATOR)[1]); + return Integer.parseInt(username.substring(username.indexOf(UsernameGenerator.SEPARATOR) + 1)); } @Test 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 b2130f14e..719789a63 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -803,10 +803,11 @@ class AccountsTest { accounts.create(account1); final Account account2 = generateAccount("+18005552222", UUID.randomUUID(), UUID.randomUUID()); accounts.create(account2); + final String username = "snowball.02"; - accounts.reserveUsername(account1, "snowball#0002", Duration.ofDays(2)); + accounts.reserveUsername(account1, username, Duration.ofDays(2)); - Supplier take = () -> accounts.reserveUsername(account2, "snowball#0002", Duration.ofDays(2)); + Supplier take = () -> accounts.reserveUsername(account2, username, Duration.ofDays(2)); for (int i = 0; i <= 2; i++) { when(mockClock.instant()).thenReturn(Instant.EPOCH.plus(Duration.ofDays(i))); @@ -818,12 +819,12 @@ class AccountsTest { final UUID token = take.get(); assertThrows(ContestedOptimisticLockException.class, - () -> accounts.reserveUsername(account1, "snowball#0002", Duration.ofDays(2))); + () -> accounts.reserveUsername(account1, username, Duration.ofDays(2))); assertThrows(ContestedOptimisticLockException.class, - () -> accounts.setUsername(account1, "snowball#0002")); + () -> accounts.setUsername(account1, username)); - accounts.confirmUsername(account2, "snowball#0002", token); - assertThat(accounts.getByUsername("snowball#0002").get().getUuid()).isEqualTo(account2.getUuid()); + accounts.confirmUsername(account2, username, token); + assertThat(accounts.getByUsername(username).get().getUuid()).isEqualTo(account2.getUuid()); } @Test @@ -832,10 +833,11 @@ class AccountsTest { accounts.create(account1); final Account account2 = generateAccount("+18005552222", UUID.randomUUID(), UUID.randomUUID()); accounts.create(account2); + final String username = "simon.123"; - accounts.reserveUsername(account1, "snowball#0002", Duration.ofDays(2)); + accounts.reserveUsername(account1, username, Duration.ofDays(2)); - Runnable take = () -> accounts.setUsername(account2, "snowball#0002"); + Runnable take = () -> accounts.setUsername(account2, username); for (int i = 0; i <= 2; i++) { when(mockClock.instant()).thenReturn(Instant.EPOCH.plus(Duration.ofDays(i))); @@ -847,10 +849,10 @@ class AccountsTest { take.run(); assertThrows(ContestedOptimisticLockException.class, - () -> accounts.reserveUsername(account1, "snowball#0002", Duration.ofDays(2))); + () -> accounts.reserveUsername(account1, username, Duration.ofDays(2))); assertThrows(ContestedOptimisticLockException.class, - () -> accounts.setUsername(account1, "snowball#0002")); - assertThat(accounts.getByUsername("snowball#0002").get().getUuid()).isEqualTo(account2.getUuid()); + () -> accounts.setUsername(account1, username)); + assertThat(accounts.getByUsername(username).get().getUuid()).isEqualTo(account2.getUuid()); } @Test diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java index 67a2fce5c..aaa2d3a73 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java @@ -1728,7 +1728,7 @@ class AccountControllerTest { @Test void testSetUsername() throws UsernameNotAvailableException { Account account = mock(Account.class); - when(account.getUsername()).thenReturn(Optional.of("n00bkiller#1234")); + when(account.getUsername()).thenReturn(Optional.of("n00bkiller.1234")); when(accountsManager.setUsername(any(), eq("n00bkiller"), isNull())) .thenReturn(account); Response response = @@ -1738,13 +1738,13 @@ class AccountControllerTest { .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) .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)); + .thenReturn(new AccountsManager.UsernameReservation(null, "n00bkiller.1234", RESERVATION_TOKEN)); Response response = resources.getJerseyTest() .target("/v1/accounts/username/reserved") @@ -1753,48 +1753,48 @@ class AccountControllerTest { .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)); } @Test void testCommitUsername() throws UsernameNotAvailableException, UsernameReservationNotFoundException { Account account = mock(Account.class); - when(account.getUsername()).thenReturn(Optional.of("n00bkiller#1234")); - when(accountsManager.confirmReservedUsername(any(), eq("n00bkiller#1234"), eq(RESERVATION_TOKEN))).thenReturn(account); + when(account.getUsername()).thenReturn(Optional.of("n00bkiller.1234")); + when(accountsManager.confirmReservedUsername(any(), eq("n00bkiller.1234"), eq(RESERVATION_TOKEN))).thenReturn(account); Response response = resources.getJerseyTest() .target("/v1/accounts/username/confirm") .request() .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) - .put(Entity.json(new ConfirmUsernameRequest("n00bkiller#1234", RESERVATION_TOKEN))); + .put(Entity.json(new ConfirmUsernameRequest("n00bkiller.1234", RESERVATION_TOKEN))); 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 testCommitUnreservedUsername() throws UsernameNotAvailableException, UsernameReservationNotFoundException { - when(accountsManager.confirmReservedUsername(any(), eq("n00bkiller#1234"), eq(RESERVATION_TOKEN))) + when(accountsManager.confirmReservedUsername(any(), eq("n00bkiller.1234"), eq(RESERVATION_TOKEN))) .thenThrow(new UsernameReservationNotFoundException()); Response response = resources.getJerseyTest() .target("/v1/accounts/username/confirm") .request() .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) - .put(Entity.json(new ConfirmUsernameRequest("n00bkiller#1234", RESERVATION_TOKEN))); + .put(Entity.json(new ConfirmUsernameRequest("n00bkiller.1234", RESERVATION_TOKEN))); assertThat(response.getStatus()).isEqualTo(409); } @Test void testCommitLapsedUsername() throws UsernameNotAvailableException, UsernameReservationNotFoundException { - when(accountsManager.confirmReservedUsername(any(), eq("n00bkiller#1234"), eq(RESERVATION_TOKEN))) + when(accountsManager.confirmReservedUsername(any(), eq("n00bkiller.1234"), eq(RESERVATION_TOKEN))) .thenThrow(new UsernameNotAvailableException()); Response response = resources.getJerseyTest() .target("/v1/accounts/username/confirm") .request() .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) - .put(Entity.json(new ConfirmUsernameRequest("n00bkiller#1234", RESERVATION_TOKEN))); + .put(Entity.json(new ConfirmUsernameRequest("n00bkiller.1234", RESERVATION_TOKEN))); assertThat(response.getStatus()).isEqualTo(410); } @@ -2068,9 +2068,9 @@ class AccountControllerTest { final UUID uuid = UUID.randomUUID(); when(account.getUuid()).thenReturn(uuid); - when(accountsManager.getByUsername(eq("n00bkiller#1234"))).thenReturn(Optional.of(account)); + when(accountsManager.getByUsername(eq("n00bkiller.1234"))).thenReturn(Optional.of(account)); Response response = resources.getJerseyTest() - .target("v1/accounts/username/n00bkiller#1234") + .target("v1/accounts/username/n00bkiller.1234") .request() .header("X-Forwarded-For", "127.0.0.1") .get(); @@ -2080,9 +2080,9 @@ class AccountControllerTest { @Test void testLookupUsernameDoesNotExist() { - when(accountsManager.getByUsername(eq("n00bkiller#1234"))).thenReturn(Optional.empty()); + when(accountsManager.getByUsername(eq("n00bkiller.1234"))).thenReturn(Optional.empty()); assertThat(resources.getJerseyTest() - .target("v1/accounts/username/n00bkiller#1234") + .target("v1/accounts/username/n00bkiller.1234") .request() .header("X-Forwarded-For", "127.0.0.1") .get().getStatus()).isEqualTo(404); @@ -2092,7 +2092,7 @@ class AccountControllerTest { void testLookupUsernameRateLimited() throws RateLimitExceededException { doThrow(new RateLimitExceededException(Duration.ofSeconds(13))).when(usernameLookupLimiter).validate("127.0.0.1"); final Response response = resources.getJerseyTest() - .target("/v1/accounts/username/test#123") + .target("/v1/accounts/username/test.123") .request() .header("X-Forwarded-For", "127.0.0.1") .get(); 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 b3b43facf..1332fe435 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 @@ -34,6 +34,7 @@ public class UsernameGeneratorTest { Arguments.of("ab\uD83D\uDC1B", false, "illegal character"), Arguments.of("1test", false, "illegal start"), Arguments.of("test#123", false, "illegal character"), + Arguments.of("test.123", false, "illegal character"), Arguments.of("ab", false, "too short"), Arguments.of("", false, ""), Arguments.of("_123456789_123456789_123456789123", false, "33 characters"), @@ -54,24 +55,24 @@ public class UsernameGeneratorTest { static Stream nonStandardUsernames() { return Stream.of( - Arguments.of("Test#123", false), - Arguments.of("test#-123", false), - Arguments.of("test#0", false), - Arguments.of("test#", false), - Arguments.of("test#1_00", false), + Arguments.of("Test.123", false), + Arguments.of("test.-123", false), + Arguments.of("test.0", false), + Arguments.of("test.", false), + Arguments.of("test.1_00", false), - Arguments.of("test#1", true), - Arguments.of("abc#1234", true) + Arguments.of("test.1", true), + Arguments.of("abc.1234", true) ); } @Test public void zeroPadDiscriminators() { final UsernameGenerator generator = new UsernameGenerator(4, 5, 1, TTL); - assertThat(generator.fromParts("test", 1)).isEqualTo("test#0001"); - assertThat(generator.fromParts("test", 123)).isEqualTo("test#0123"); - assertThat(generator.fromParts("test", 9999)).isEqualTo("test#9999"); - assertThat(generator.fromParts("test", 99999)).isEqualTo("test#99999"); + assertThat(generator.fromParts("test", 1)).isEqualTo("test.0001"); + assertThat(generator.fromParts("test", 123)).isEqualTo("test.0123"); + assertThat(generator.fromParts("test", 9999)).isEqualTo("test.9999"); + assertThat(generator.fromParts("test", 99999)).isEqualTo("test.99999"); } @Test @@ -139,6 +140,6 @@ public class UsernameGeneratorTest { } private static int extractDiscriminator(final String username) { - return Integer.parseInt(username.split(UsernameGenerator.SEPARATOR)[1]); + return Integer.parseInt(username.substring(username.indexOf(UsernameGenerator.SEPARATOR) + 1)); } }