From e200548e35e7d8a1da1d6bb4f01e59580ae5ed27 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Mon, 28 Mar 2022 15:45:43 -0400 Subject: [PATCH] Introduce an account change validator --- .../storage/AccountChangeValidator.java | 56 +++++++++++++ .../storage/AccountsManager.java | 75 ++++++++--------- .../storage/AccountChangeValidatorTest.java | 83 +++++++++++++++++++ 3 files changed, 173 insertions(+), 41 deletions(-) create mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountChangeValidator.java create mode 100644 service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountChangeValidatorTest.java diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountChangeValidator.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountChangeValidator.java new file mode 100644 index 000000000..2db069324 --- /dev/null +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountChangeValidator.java @@ -0,0 +1,56 @@ +/* + * Copyright 2013-2022 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.whispersystems.textsecuregcm.storage; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +class AccountChangeValidator { + + private final boolean allowNumberChange; + private final boolean allowUsernameChange; + + static final AccountChangeValidator GENERAL_CHANGE_VALIDATOR = new AccountChangeValidator(false, false); + static final AccountChangeValidator NUMBER_CHANGE_VALIDATOR = new AccountChangeValidator(true, false); + static final AccountChangeValidator USERNAME_CHANGE_VALIDATOR = new AccountChangeValidator(false, true); + + private static final Logger logger = LoggerFactory.getLogger(AccountChangeValidator.class); + + AccountChangeValidator(final boolean allowNumberChange, + final boolean allowUsernameChange) { + + this.allowNumberChange = allowNumberChange; + this.allowUsernameChange = allowUsernameChange; + } + + public void validateChange(final Account originalAccount, final Account updatedAccount) { + if (!allowNumberChange) { + assert updatedAccount.getNumber().equals(originalAccount.getNumber()); + + if (!updatedAccount.getNumber().equals(originalAccount.getNumber())) { + logger.error("Account number changed via \"normal\" update; numbers must be changed via changeNumber method", + new RuntimeException()); + } + + assert updatedAccount.getPhoneNumberIdentifier().equals(originalAccount.getPhoneNumberIdentifier()); + + if (!updatedAccount.getPhoneNumberIdentifier().equals(originalAccount.getPhoneNumberIdentifier())) { + logger.error( + "Phone number identifier changed via \"normal\" update; PNIs must be changed via changeNumber method", + new RuntimeException()); + } + } + + if (!allowUsernameChange) { + assert updatedAccount.getUsername().equals(originalAccount.getUsername()); + + if (!updatedAccount.getUsername().equals(originalAccount.getUsername())) { + logger.error("Username changed via \"normal\" update; usernames must be changed via setUsername method", + new RuntimeException()); + } + } + } +} 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 8205fa013..ecd58bca0 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -80,9 +80,10 @@ public class AccountsManager { private final SecureStorageClient secureStorageClient; private final SecureBackupClient secureBackupClient; private final ClientPresenceManager clientPresenceManager; - private final ObjectMapper mapper; private final Clock clock; + private static final ObjectMapper mapper = SystemMapper.getMapper(); + // An account that's used at least daily will get reset in the cache at least once per day when its "last seen" // timestamp updates; expiring entries after two days will help clear out "zombie" cache entries that are read // frequently (e.g. the account is in an active group and receives messages frequently), but aren't actively used by @@ -133,7 +134,6 @@ public class AccountsManager { this.secureBackupClient = secureBackupClient; this.clientPresenceManager = clientPresenceManager; this.reservedUsernames = reservedUsernames; - this.mapper = SystemMapper.getMapper(); this.clock = Objects.requireNonNull(clock); } @@ -254,7 +254,8 @@ public class AccountsManager { account, a -> true, a -> accounts.changeNumber(a, number, phoneNumberIdentifier), - () -> accounts.getByAccountIdentifier(uuid).orElseThrow()); + () -> accounts.getByAccountIdentifier(uuid).orElseThrow(), + AccountChangeValidator.NUMBER_CHANGE_VALIDATOR); } catch (UsernameNotAvailableException e) { // This should never happen when changing numbers throw new RuntimeException(e); @@ -289,7 +290,8 @@ public class AccountsManager { account, a -> true, a -> accounts.setUsername(a, canonicalUsername), - () -> accounts.getByAccountIdentifier(account.getUuid()).orElseThrow()); + () -> accounts.getByAccountIdentifier(account.getUuid()).orElseThrow(), + AccountChangeValidator.USERNAME_CHANGE_VALIDATOR); } public Account clearUsername(final Account account) { @@ -300,7 +302,8 @@ public class AccountsManager { account, a -> true, accounts::clearUsername, - () -> accounts.getByAccountIdentifier(account.getUuid()).orElseThrow()); + () -> accounts.getByAccountIdentifier(account.getUuid()).orElseThrow(), + AccountChangeValidator.USERNAME_CHANGE_VALIDATOR); } catch (UsernameNotAvailableException e) { // This should never happen throw new RuntimeException(e); @@ -364,35 +367,12 @@ public class AccountsManager { redisDelete(account); final UUID uuid = account.getUuid(); - final String originalNumber = account.getNumber(); - final UUID originalPhoneNumberIdentifier = account.getPhoneNumberIdentifier(); - final Optional originalUsername = account.getUsername(); updatedAccount = updateWithRetries(account, updater, accounts::update, - () -> accounts.getByAccountIdentifier(uuid).orElseThrow()); - - assert updatedAccount.getNumber().equals(originalNumber); - - if (!updatedAccount.getNumber().equals(originalNumber)) { - logger.error("Account number changed via \"normal\" update; numbers must be changed via changeNumber method", - new RuntimeException()); - } - - assert updatedAccount.getPhoneNumberIdentifier().equals(originalPhoneNumberIdentifier); - - if (!updatedAccount.getPhoneNumberIdentifier().equals(originalPhoneNumberIdentifier)) { - logger.error("Phone number identifier changed via \"normal\" update; PNIs must be changed via changeNumber method", - new RuntimeException()); - } - - assert updatedAccount.getUsername().equals(originalUsername); - - if (!updatedAccount.getUsername().equals(originalUsername)) { - logger.error("Username changed via \"normal\" update; usernames must be changed via setUsername method", - new RuntimeException()); - } + () -> accounts.getByAccountIdentifier(uuid).orElseThrow(), + AccountChangeValidator.GENERAL_CHANGE_VALIDATOR); redisSet(updatedAccount); } @@ -406,8 +386,13 @@ public class AccountsManager { return updatedAccount; } - private Account updateWithRetries(Account account, Function updater, AccountPersister persister, - Supplier retriever) throws UsernameNotAvailableException { + private Account updateWithRetries(Account account, + final Function updater, + final AccountPersister persister, + final Supplier retriever, + final AccountChangeValidator changeValidator) throws UsernameNotAvailableException { + + Account originalAccount = cloneAccount(account); if (!updater.apply(account)) { return account; @@ -421,21 +406,17 @@ public class AccountsManager { try { persister.persistAccount(account); - final Account updatedAccount; - try { - updatedAccount = mapper.readValue(mapper.writeValueAsBytes(account), Account.class); - updatedAccount.setUuid(account.getUuid()); - } catch (final IOException e) { - // this should really, truly, never happen - throw new IllegalArgumentException(e); - } - + final Account updatedAccount = cloneAccount(account); account.markStale(); + changeValidator.validateChange(originalAccount, updatedAccount); + return updatedAccount; } catch (final ContestedOptimisticLockException e) { tries++; + account = retriever.get(); + originalAccount = cloneAccount(account); if (!updater.apply(account)) { return account; @@ -446,6 +427,18 @@ public class AccountsManager { throw new OptimisticLockRetryLimitExceededException(); } + private static Account cloneAccount(final Account account) { + try { + final Account clone = mapper.readValue(mapper.writeValueAsBytes(account), Account.class); + clone.setUuid(account.getUuid()); + + return clone; + } catch (final IOException e) { + // this should really, truly, never happen + throw new IllegalArgumentException(e); + } + } + public Account updateDevice(Account account, long deviceId, Consumer deviceUpdater) { try { return update(account, a -> { diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountChangeValidatorTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountChangeValidatorTest.java new file mode 100644 index 000000000..39ef6e24e --- /dev/null +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountChangeValidatorTest.java @@ -0,0 +1,83 @@ +/* + * Copyright 2013-2022 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.whispersystems.textsecuregcm.storage; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.Optional; +import java.util.UUID; +import java.util.stream.Stream; +import org.junit.jupiter.api.function.Executable; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +class AccountChangeValidatorTest { + + private static final String ORIGINAL_NUMBER = "+18005551234"; + private static final String CHANGED_NUMBER = "+18005559876"; + + private static final UUID ORIGINAL_PNI = UUID.randomUUID(); + private static final UUID CHANGED_PNI = UUID.randomUUID(); + + private static final String ORIGINAL_USERNAME = "bruce_wayne"; + private static final String CHANGED_USERNAME = "batman"; + + @ParameterizedTest + @MethodSource + void validateChange(final Account originalAccount, + final Account updatedAccount, + final AccountChangeValidator changeValidator, + final boolean expectChangeAllowed) { + + final Executable applyChange = () -> changeValidator.validateChange(originalAccount, updatedAccount); + + if (expectChangeAllowed) { + assertDoesNotThrow(applyChange); + } else { + assertThrows(AssertionError.class, applyChange); + } + } + + private static Stream validateChange() { + final Account originalAccount = mock(Account.class); + when(originalAccount.getNumber()).thenReturn(ORIGINAL_NUMBER); + when(originalAccount.getPhoneNumberIdentifier()).thenReturn(ORIGINAL_PNI); + when(originalAccount.getUsername()).thenReturn(Optional.of(ORIGINAL_USERNAME)); + + final Account unchangedAccount = mock(Account.class); + when(unchangedAccount.getNumber()).thenReturn(ORIGINAL_NUMBER); + when(unchangedAccount.getPhoneNumberIdentifier()).thenReturn(ORIGINAL_PNI); + when(unchangedAccount.getUsername()).thenReturn(Optional.of(ORIGINAL_USERNAME)); + + final Account changedNumberAccount = mock(Account.class); + when(changedNumberAccount.getNumber()).thenReturn(CHANGED_NUMBER); + when(changedNumberAccount.getPhoneNumberIdentifier()).thenReturn(CHANGED_PNI); + when(changedNumberAccount.getUsername()).thenReturn(Optional.of(ORIGINAL_USERNAME)); + + final Account changedUsernameAccount = mock(Account.class); + when(changedUsernameAccount.getNumber()).thenReturn(ORIGINAL_NUMBER); + when(changedUsernameAccount.getPhoneNumberIdentifier()).thenReturn(ORIGINAL_PNI); + when(changedUsernameAccount.getUsername()).thenReturn(Optional.of(CHANGED_USERNAME)); + + return Stream.of( + Arguments.of(originalAccount, unchangedAccount, AccountChangeValidator.GENERAL_CHANGE_VALIDATOR, true), + Arguments.of(originalAccount, unchangedAccount, AccountChangeValidator.NUMBER_CHANGE_VALIDATOR, true), + Arguments.of(originalAccount, unchangedAccount, AccountChangeValidator.USERNAME_CHANGE_VALIDATOR, true), + + Arguments.of(originalAccount, changedNumberAccount, AccountChangeValidator.GENERAL_CHANGE_VALIDATOR, false), + Arguments.of(originalAccount, changedNumberAccount, AccountChangeValidator.NUMBER_CHANGE_VALIDATOR, true), + Arguments.of(originalAccount, changedNumberAccount, AccountChangeValidator.USERNAME_CHANGE_VALIDATOR, false), + + Arguments.of(originalAccount, changedUsernameAccount, AccountChangeValidator.GENERAL_CHANGE_VALIDATOR, false), + Arguments.of(originalAccount, changedUsernameAccount, AccountChangeValidator.NUMBER_CHANGE_VALIDATOR, false), + Arguments.of(originalAccount, changedUsernameAccount, AccountChangeValidator.USERNAME_CHANGE_VALIDATOR, true) + ); + } +}