MirrorMirror
/
Signal-Server
mirror of https://github.com/signalapp/Signal-Server.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Introduce an account change validator

This commit is contained in:
Jon Chambers 2022-03-28 15:45:43 -04:00 committed by Jon Chambers
parent fdf7b69996
commit e200548e35
3 changed files with 173 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

56
service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountChangeValidator.java Normal file
View File

@ -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());
}
}
}
}

75
service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java
View File

@ -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<String> 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<Account, Boolean> updater, AccountPersister persister,
Supplier<Account> retriever) throws UsernameNotAvailableException {
private Account updateWithRetries(Account account,
final Function<Account, Boolean> updater,
final AccountPersister persister,
final Supplier<Account> 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<Device> deviceUpdater) {
try {
return update(account, a -> {

83
service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountChangeValidatorTest.java Normal file
View File

@ -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<Arguments> 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)
);
}
}