diff --git a/abusive-message-filter b/abusive-message-filter index 56398ec89..d20873c7d 160000 --- a/abusive-message-filter +++ b/abusive-message-filter @@ -1 +1 @@ -Subproject commit 56398ec89d0b719fb57bebaefac02dae85b3abb2 +Subproject commit d20873c7d78eb0a33cb27d103ba6ee6807b09a88 diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java index 17cc07b3f..ac43b4a55 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java @@ -171,6 +171,11 @@ public class WhisperServerConfiguration extends Configuration { @JsonProperty private AccountsDynamoDbConfiguration accountsDynamoDb; + @Valid + @NotNull + @JsonProperty + private DynamoDbConfiguration phoneNumberIdentifiersDynamoDb; + @Valid @NotNull @JsonProperty @@ -436,6 +441,10 @@ public class WhisperServerConfiguration extends Configuration { return accountsDynamoDb; } + public DynamoDbConfiguration getPhoneNumberIdentifiersDynamoDbConfiguration() { + return phoneNumberIdentifiersDynamoDb; + } + public DeletedAccountsDynamoDbConfiguration getDeletedAccountsDynamoDbConfiguration() { return deletedAccountsDynamoDb; } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 29427ccf6..997fcbd8b 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -186,6 +186,7 @@ import org.whispersystems.textsecuregcm.storage.MessagesCache; import org.whispersystems.textsecuregcm.storage.MessagesDynamoDb; import org.whispersystems.textsecuregcm.storage.MessagesManager; import org.whispersystems.textsecuregcm.storage.NonNormalizedAccountCrawlerListener; +import org.whispersystems.textsecuregcm.storage.PhoneNumberIdentifiers; import org.whispersystems.textsecuregcm.storage.Profiles; import org.whispersystems.textsecuregcm.storage.ProfilesManager; import org.whispersystems.textsecuregcm.storage.PubSubManager; @@ -330,6 +331,10 @@ public class WhisperServerService extends Application initialDevicesEnabled = (Map) requestEvent.getContainerRequest().getProperty(DEVICES_ENABLED); - return accountsManager.get((UUID) requestEvent.getContainerRequest().getProperty(ACCOUNT_UUID)).map(account -> { + return accountsManager.getByAccountIdentifier((UUID) requestEvent.getContainerRequest().getProperty(ACCOUNT_UUID)).map(account -> { final Set deviceIdsToDisplace; final Map currentDevicesEnabled = buildDevicesEnabledMap(account); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticator.java b/service/src/main/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticator.java index 9bcb64c04..f50511bf6 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticator.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticator.java @@ -78,7 +78,7 @@ public class BaseAccountAuthenticator { deviceId = identifierAndDeviceId.second(); } - Optional account = accountsManager.get(accountUuid); + Optional account = accountsManager.getByAccountIdentifier(accountUuid); if (account.isEmpty()) { failureReason = "noSuchAccount"; diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/AccountsDynamoDbConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/AccountsDynamoDbConfiguration.java index 26804156a..11fe7f596 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/AccountsDynamoDbConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/AccountsDynamoDbConfiguration.java @@ -8,6 +8,9 @@ public class AccountsDynamoDbConfiguration extends DynamoDbConfiguration { @NotNull private String phoneNumberTableName; + @NotNull + private String phoneNumberIdentifierTableName; + private int scanPageSize = 100; @JsonProperty @@ -15,6 +18,11 @@ public class AccountsDynamoDbConfiguration extends DynamoDbConfiguration { return phoneNumberTableName; } + @JsonProperty + public String getPhoneNumberIdentifierTableName() { + return phoneNumberIdentifierTableName; + } + @JsonProperty public int getScanPageSize() { return scanPageSize; diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java index 8dc69ab40..845d30a72 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -353,7 +353,7 @@ public class AccountController { storedVerificationCode.flatMap(StoredVerificationCode::getTwilioVerificationSid) .ifPresent(smsSender::reportVerificationSucceeded); - Optional existingAccount = accounts.get(number); + Optional existingAccount = accounts.getByE164(number); if (existingAccount.isPresent()) { verifyRegistrationLock(existingAccount.get(), accountAttributes.getRegistrationLock()); @@ -412,7 +412,7 @@ public class AccountController { storedVerificationCode.flatMap(StoredVerificationCode::getTwilioVerificationSid) .ifPresent(smsSender::reportVerificationSucceeded); - final Optional existingAccount = accounts.get(request.getNumber()); + final Optional existingAccount = accounts.getByE164(request.getNumber()); if (existingAccount.isPresent()) { verifyRegistrationLock(existingAccount.get(), request.getRegistrationLock()); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java index 8ea1041f5..9afc57593 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java @@ -168,7 +168,7 @@ public class DeviceController { throw new WebApplicationException(Response.status(403).build()); } - Optional account = accounts.get(number); + Optional account = accounts.getByE164(number); if (!account.isPresent()) { throw new WebApplicationException(Response.status(403).build()); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DonationController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DonationController.java index 8dc4f7d5c..ee2eecaaf 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DonationController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DonationController.java @@ -159,7 +159,7 @@ public class DonationController { @Override public boolean block() { - final Optional optionalAccount = accountsManager.get(auth.getAccount().getUuid()); + final Optional optionalAccount = accountsManager.getByAccountIdentifier(auth.getAccount().getUuid()); optionalAccount.ifPresent(account -> { accountsManager.update(account, a -> { a.addBadge(clock, new AccountBadge(badgeId, receiptExpiration, request.isVisible())); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java index 93db57cca..965cdcaee 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java @@ -129,7 +129,7 @@ public class KeysController { final Optional account = auth.map(AuthenticatedAccount::getAccount); - Optional target = accounts.get(targetUuid); + Optional target = accounts.getByAccountIdentifier(targetUuid); OptionalAccess.verify(account, accessKey, target, deviceId); assert (target.isPresent()); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/MessageController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/MessageController.java index 9881a7da6..6fb8249f6 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/MessageController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/MessageController.java @@ -215,7 +215,7 @@ public class MessageController { Optional destination; if (!isSyncMessage) { - destination = accountsManager.get(destinationUuid); + destination = accountsManager.getByAccountIdentifier(destinationUuid); } else { destination = source.map(AuthenticatedAccount::getAccount); } @@ -311,7 +311,7 @@ public class MessageController { .map(Recipient::getUuid) .distinct() .collect(Collectors.toUnmodifiableMap(Function.identity(), uuid -> { - Optional account = accountsManager.get(uuid); + Optional account = accountsManager.getByAccountIdentifier(uuid); if (account.isEmpty()) { throw new WebApplicationException(Status.NOT_FOUND); } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java index e0767700c..ef2471266 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java @@ -251,7 +251,7 @@ public class ProfileController { isSelf = uuid.equals(authedUuid); } - Optional accountProfile = accountsManager.get(uuid); + Optional accountProfile = accountsManager.getByAccountIdentifier(uuid); OptionalAccess.verify(requestAccount, accessKey, accountProfile); assert(accountProfile.isPresent()); @@ -316,7 +316,7 @@ public class ProfileController { final boolean isSelf = auth.getAccount().getUuid().equals(uuid.get()); - Optional accountProfile = accountsManager.get(uuid.get()); + Optional accountProfile = accountsManager.getByAccountIdentifier(uuid.get()); if (accountProfile.isEmpty()) { throw new WebApplicationException(Response.status(Response.Status.NOT_FOUND).build()); @@ -398,7 +398,7 @@ public class ProfileController { isSelf = authedUuid.equals(identifier); } - Optional accountProfile = accountsManager.get(identifier); + Optional accountProfile = accountsManager.getByAccountIdentifier(identifier); OptionalAccess.verify(auth.map(AuthenticatedAccount::getAccount), accessKey, accountProfile); Optional username = usernamesManager.get(accountProfile.get().getUuid()); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/push/APNSender.java b/service/src/main/java/org/whispersystems/textsecuregcm/push/APNSender.java index 4114ccc2b..9e1a4dd76 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/push/APNSender.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/push/APNSender.java @@ -125,7 +125,7 @@ public class APNSender implements Managed { private void handleUnregisteredUser(String registrationId, UUID uuid, long deviceId) { // logger.info("Got APN Unregistered: " + number + "," + deviceId); - Optional account = accountsManager.get(uuid); + Optional account = accountsManager.getByAccountIdentifier(uuid); if (account.isEmpty()) { logger.info("No account found: {}", uuid); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/push/ApnFallbackManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/push/ApnFallbackManager.java index 46cde7a11..55ba4065f 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/push/ApnFallbackManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/push/ApnFallbackManager.java @@ -99,7 +99,7 @@ public class ApnFallbackManager implements Managed { final Optional maybeAccount = separated.map(Pair::first) .map(UUID::fromString) - .flatMap(accountsManager::get); + .flatMap(accountsManager::getByAccountIdentifier); final Optional maybeDevice = separated.map(Pair::second) .flatMap(deviceId -> maybeAccount.flatMap(account -> account.getDevice(deviceId))); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/push/GCMSender.java b/service/src/main/java/org/whispersystems/textsecuregcm/push/GCMSender.java index f00a18923..6757d6a50 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/push/GCMSender.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/push/GCMSender.java @@ -139,7 +139,7 @@ public class GCMSender { } private Optional getAccountForEvent(GcmMessage message) { - Optional account = message.getUuid().flatMap(accountsManager::get); + Optional account = message.getUuid().flatMap(accountsManager::getByAccountIdentifier); if (account.isPresent()) { Optional device = account.get().getDevice(message.getDeviceId()); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/push/ReceiptSender.java b/service/src/main/java/org/whispersystems/textsecuregcm/push/ReceiptSender.java index c3f839bbf..350eddb39 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/push/ReceiptSender.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/push/ReceiptSender.java @@ -33,7 +33,7 @@ public class ReceiptSender { return; } - final Account destinationAccount = accountManager.get(destinationUuid) + final Account destinationAccount = accountManager.getByAccountIdentifier(destinationUuid) .orElseThrow(() -> new NoSuchUserException(destinationUuid)); final Envelope.Builder message = Envelope.newBuilder() diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Account.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Account.java index 64dba40a6..b767f37a9 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Account.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Account.java @@ -23,6 +23,7 @@ import org.whispersystems.textsecuregcm.auth.AuthenticationCredentials; import org.whispersystems.textsecuregcm.auth.StoredRegistrationLock; import org.whispersystems.textsecuregcm.entities.AccountAttributes; import org.whispersystems.textsecuregcm.util.Util; +import javax.annotation.Nullable; public class Account { @@ -32,6 +33,11 @@ public class Account { @JsonIgnore private UUID uuid; + // Nullable only until initial migration is complete + @JsonProperty("pni") + @Nullable + private UUID phoneNumberIdentifier; + @JsonProperty private String number; @@ -80,9 +86,10 @@ public class Account { public Account() {} @VisibleForTesting - public Account(String number, UUID uuid, Set devices, byte[] unidentifiedAccessKey) { + public Account(String number, UUID uuid, final UUID phoneNumberIdentifier, Set devices, byte[] unidentifiedAccessKey) { this.number = number; this.uuid = uuid; + this.phoneNumberIdentifier = phoneNumberIdentifier; this.devices = devices; this.unidentifiedAccessKey = unidentifiedAccessKey; } @@ -98,16 +105,11 @@ public class Account { this.uuid = uuid; } - public void setNumber(String number) { + // Optional only until initial migration is complete + public Optional getPhoneNumberIdentifier() { requireNotStale(); - this.number = number; - } - - public void setCanonicallyDiscoverable(boolean canonicallyDiscoverable) { - requireNotStale(); - - this.canonicallyDiscoverable = canonicallyDiscoverable; + return Optional.ofNullable(phoneNumberIdentifier); } public String getNumber() { @@ -116,6 +118,13 @@ public class Account { return number; } + public void setNumber(String number, UUID phoneNumberIdentifier) { + requireNotStale(); + + this.number = number; + this.phoneNumberIdentifier = phoneNumberIdentifier; + } + public void addDevice(Device device) { requireNotStale(); @@ -247,6 +256,12 @@ public class Account { return canonicallyDiscoverable; } + public void setCanonicallyDiscoverable(boolean canonicallyDiscoverable) { + requireNotStale(); + + this.canonicallyDiscoverable = canonicallyDiscoverable; + } + public Optional getRelay() { requireNotStale(); 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 3b5389f7e..8d5d8ded4 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java @@ -13,23 +13,24 @@ import io.micrometer.core.instrument.Timer; import java.io.IOException; import java.nio.ByteBuffer; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.UUID; import java.util.stream.Collectors; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.whispersystems.textsecuregcm.util.AttributeValues; import org.whispersystems.textsecuregcm.util.SystemMapper; import org.whispersystems.textsecuregcm.util.UUIDUtil; import software.amazon.awssdk.services.dynamodb.DynamoDbClient; import software.amazon.awssdk.services.dynamodb.model.AttributeValue; import software.amazon.awssdk.services.dynamodb.model.CancellationReason; -import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException; import software.amazon.awssdk.services.dynamodb.model.Delete; import software.amazon.awssdk.services.dynamodb.model.GetItemRequest; import software.amazon.awssdk.services.dynamodb.model.GetItemResponse; import software.amazon.awssdk.services.dynamodb.model.Put; -import software.amazon.awssdk.services.dynamodb.model.ReturnValue; import software.amazon.awssdk.services.dynamodb.model.ReturnValuesOnConditionCheckFailure; import software.amazon.awssdk.services.dynamodb.model.ScanRequest; import software.amazon.awssdk.services.dynamodb.model.TransactWriteItem; @@ -37,13 +38,13 @@ import software.amazon.awssdk.services.dynamodb.model.TransactWriteItemsRequest; import software.amazon.awssdk.services.dynamodb.model.TransactionCanceledException; import software.amazon.awssdk.services.dynamodb.model.TransactionConflictException; import software.amazon.awssdk.services.dynamodb.model.Update; -import software.amazon.awssdk.services.dynamodb.model.UpdateItemRequest; -import software.amazon.awssdk.services.dynamodb.model.UpdateItemResponse; public class Accounts extends AbstractDynamoDbStore { // uuid, primary key static final String KEY_ACCOUNT_UUID = "U"; + // uuid, attribute on account table, primary key for PNI table + static final String ATTR_PNI_UUID = "PNI"; // phone number static final String ATTR_ACCOUNT_E164 = "P"; // account, serialized to JSON @@ -55,7 +56,8 @@ public class Accounts extends AbstractDynamoDbStore { private final DynamoDbClient client; - private final String phoneNumbersTableName; + private final String phoneNumberConstraintTableName; + private final String phoneNumberIdentifierConstraintTableName; private final String accountsTableName; private final int scanPageSize; @@ -64,19 +66,22 @@ public class Accounts extends AbstractDynamoDbStore { private static final Timer CHANGE_NUMBER_TIMER = Metrics.timer(name(Accounts.class, "changeNumber")); private static final Timer UPDATE_TIMER = Metrics.timer(name(Accounts.class, "update")); private static final Timer GET_BY_NUMBER_TIMER = Metrics.timer(name(Accounts.class, "getByNumber")); + private static final Timer GET_BY_PNI_TIMER = Metrics.timer(name(Accounts.class, "getByPni")); private static final Timer GET_BY_UUID_TIMER = Metrics.timer(name(Accounts.class, "getByUuid")); private static final Timer GET_ALL_FROM_START_TIMER = Metrics.timer(name(Accounts.class, "getAllFrom")); private static final Timer GET_ALL_FROM_OFFSET_TIMER = Metrics.timer(name(Accounts.class, "getAllFromOffset")); private static final Timer DELETE_TIMER = Metrics.timer(name(Accounts.class, "delete")); + private static final Logger log = LoggerFactory.getLogger(Accounts.class); - public Accounts(DynamoDbClient client, String accountsTableName, String phoneNumbersTableName, - final int scanPageSize) { + public Accounts(DynamoDbClient client, String accountsTableName, String phoneNumberConstraintTableName, + String phoneNumberIdentifierConstraintTableName, final int scanPageSize) { super(client); this.client = client; - this.phoneNumbersTableName = phoneNumbersTableName; + this.phoneNumberConstraintTableName = phoneNumberConstraintTableName; + this.phoneNumberIdentifierConstraintTableName = phoneNumberIdentifierConstraintTableName; this.accountsTableName = accountsTableName; this.scanPageSize = scanPageSize; } @@ -85,35 +90,98 @@ public class Accounts extends AbstractDynamoDbStore { return CREATE_TIMER.record(() -> { try { - TransactWriteItem phoneNumberConstraintPut = buildPutWriteItemForPhoneNumberConstraint(account, account.getUuid()); - TransactWriteItem accountPut = buildPutWriteItemForAccount(account, account.getUuid(), Put.builder() - .conditionExpression("attribute_not_exists(#number) OR #number = :number") - .expressionAttributeNames(Map.of("#number", ATTR_ACCOUNT_E164)) - .expressionAttributeValues(Map.of(":number", AttributeValues.fromString(account.getNumber())))); + TransactWriteItem phoneNumberConstraintPut = TransactWriteItem.builder() + .put( + Put.builder() + .tableName(phoneNumberConstraintTableName) + .item(Map.of( + ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()), + KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) + .conditionExpression( + "attribute_not_exists(#number) OR (attribute_exists(#number) AND #uuid = :uuid)") + .expressionAttributeNames( + Map.of("#uuid", KEY_ACCOUNT_UUID, + "#number", ATTR_ACCOUNT_E164)) + .expressionAttributeValues( + Map.of(":uuid", AttributeValues.fromUUID(account.getUuid()))) + .returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD) + .build()) + .build(); + + assert account.getPhoneNumberIdentifier().isPresent(); + + if (account.getPhoneNumberIdentifier().isEmpty()) { + log.error("Account {} is missing a phone number identifier", account.getUuid()); + } + + TransactWriteItem phoneNumberIdentifierConstraintPut = TransactWriteItem.builder() + .put( + Put.builder() + .tableName(phoneNumberIdentifierConstraintTableName) + .item(Map.of( + ATTR_PNI_UUID, AttributeValues.fromUUID(account.getPhoneNumberIdentifier().get()), + KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) + .conditionExpression( + "attribute_not_exists(#pni) OR (attribute_exists(#pni) AND #uuid = :uuid)") + .expressionAttributeNames( + Map.of("#uuid", KEY_ACCOUNT_UUID, + "#pni", ATTR_PNI_UUID)) + .expressionAttributeValues( + Map.of(":uuid", AttributeValues.fromUUID(account.getUuid()))) + .returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD) + .build()) + .build(); + + final Map item = new HashMap<>(Map.of( + KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()), + ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()), + ATTR_ACCOUNT_DATA, AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)), + ATTR_VERSION, AttributeValues.fromInt(account.getVersion()), + ATTR_CANONICALLY_DISCOVERABLE, AttributeValues.fromBool(account.shouldBeVisibleInDirectory()))); + + account.getPhoneNumberIdentifier().ifPresent(pni -> item.put(ATTR_PNI_UUID, AttributeValues.fromUUID(pni))); + + TransactWriteItem accountPut = TransactWriteItem.builder() + .put(Put.builder() + .conditionExpression("attribute_not_exists(#number) OR #number = :number") + .expressionAttributeNames(Map.of("#number", ATTR_ACCOUNT_E164)) + .expressionAttributeValues(Map.of(":number", AttributeValues.fromString(account.getNumber()))) + .tableName(accountsTableName) + .item(item) + .build()) + .build(); final TransactWriteItemsRequest request = TransactWriteItemsRequest.builder() - .transactItems(phoneNumberConstraintPut, accountPut) + .transactItems(phoneNumberConstraintPut, phoneNumberIdentifierConstraintPut, accountPut) .build(); try { client.transactWriteItems(request); } catch (TransactionCanceledException e) { - final CancellationReason accountCancellationReason = e.cancellationReasons().get(1); + final CancellationReason accountCancellationReason = e.cancellationReasons().get(2); if ("ConditionalCheckFailed".equals(accountCancellationReason.code())) { - throw new IllegalArgumentException("uuid present with different phone number"); + throw new IllegalArgumentException("account identifier present with different phone number"); } final CancellationReason phoneNumberConstraintCancellationReason = e.cancellationReasons().get(0); + final CancellationReason phoneNumberIdentifierConstraintCancellationReason = e.cancellationReasons().get(1); - if ("ConditionalCheckFailed".equals(phoneNumberConstraintCancellationReason.code())) { + if ("ConditionalCheckFailed".equals(phoneNumberConstraintCancellationReason.code()) || + "ConditionalCheckFailed".equals(phoneNumberIdentifierConstraintCancellationReason.code())) { - ByteBuffer actualAccountUuid = phoneNumberConstraintCancellationReason.item().get(KEY_ACCOUNT_UUID).b().asByteBuffer(); + // In theory, both reasons should trip in tandem and either should give us the information we need. Even so, + // we'll be cautious here and make sure we're choosing a condition check that really failed. + final CancellationReason reason = "ConditionalCheckFailed".equals(phoneNumberConstraintCancellationReason.code()) ? + phoneNumberConstraintCancellationReason : phoneNumberIdentifierConstraintCancellationReason; + + ByteBuffer actualAccountUuid = reason.item().get(KEY_ACCOUNT_UUID).b().asByteBuffer(); account.setUuid(UUIDUtil.fromByteBuffer(actualAccountUuid)); - final int version = get(account.getUuid()).get().getVersion(); - account.setVersion(version); + final Account existingAccount = getByAccountIdentifier(account.getUuid()).orElseThrow(); + account.setNumber(existingAccount.getNumber(), existingAccount.getPhoneNumberIdentifier().orElse(account.getPhoneNumberIdentifier().orElseThrow())); + account.setVersion(existingAccount.getVersion()); update(account); @@ -125,7 +193,7 @@ public class Accounts extends AbstractDynamoDbStore { throw new ContestedOptimisticLockException(); } - // this shouldn’t happen + // this shouldn't happen throw new RuntimeException("could not create account: " + extractCancellationReasonCodes(e)); } } catch (JsonProcessingException e) { @@ -136,44 +204,11 @@ public class Accounts extends AbstractDynamoDbStore { }); } - private TransactWriteItem buildPutWriteItemForAccount(Account account, UUID uuid, Put.Builder putBuilder) throws JsonProcessingException { - return TransactWriteItem.builder() - .put(putBuilder - .tableName(accountsTableName) - .item(Map.of( - KEY_ACCOUNT_UUID, AttributeValues.fromUUID(uuid), - ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()), - ATTR_ACCOUNT_DATA, AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)), - ATTR_VERSION, AttributeValues.fromInt(account.getVersion()), - ATTR_CANONICALLY_DISCOVERABLE, AttributeValues.fromBool(account.shouldBeVisibleInDirectory()))) - .build()) - .build(); - } - - private TransactWriteItem buildPutWriteItemForPhoneNumberConstraint(Account account, UUID uuid) { - return TransactWriteItem.builder() - .put( - Put.builder() - .tableName(phoneNumbersTableName) - .item(Map.of( - ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()), - KEY_ACCOUNT_UUID, AttributeValues.fromUUID(uuid))) - .conditionExpression( - "attribute_not_exists(#number) OR (attribute_exists(#number) AND #uuid = :uuid)") - .expressionAttributeNames( - Map.of("#uuid", KEY_ACCOUNT_UUID, - "#number", ATTR_ACCOUNT_E164)) - .expressionAttributeValues( - Map.of(":uuid", AttributeValues.fromUUID(uuid))) - .returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD) - .build()) - .build(); - } - /** * Changes the phone number for the given account. The given account's number should be its current, pre-change - * number. If this method succeeds, the account's number will be changed to the new number. If the update fails for - * any reason, the account's number will be unchanged. + * number. If this method succeeds, the account's number will be changed to the new number and its phone number + * identifier will be changed to the given phone number identifier. If the update fails for any reason, the account's + * number and PNI will be unchanged. *

* This method expects that any accounts with conflicting numbers will have been removed by the time this method is * called. This method may fail with an unspecified {@link RuntimeException} if another account with the same number @@ -182,26 +217,28 @@ public class Accounts extends AbstractDynamoDbStore { * @param account the account for which to change the phone number * @param number the new phone number */ - public void changeNumber(final Account account, final String number) { + public void changeNumber(final Account account, final String number, final UUID phoneNumberIdentifier) { CHANGE_NUMBER_TIMER.record(() -> { final String originalNumber = account.getNumber(); + final Optional originalPni = account.getPhoneNumberIdentifier(); + boolean succeeded = false; - account.setNumber(number); + account.setNumber(number, phoneNumberIdentifier); try { final List writeItems = new ArrayList<>(); writeItems.add(TransactWriteItem.builder() .delete(Delete.builder() - .tableName(phoneNumbersTableName) + .tableName(phoneNumberConstraintTableName) .key(Map.of(ATTR_ACCOUNT_E164, AttributeValues.fromString(originalNumber))) .build()) .build()); writeItems.add(TransactWriteItem.builder() .put(Put.builder() - .tableName(phoneNumbersTableName) + .tableName(phoneNumberConstraintTableName) .item(Map.of( KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()), ATTR_ACCOUNT_E164, AttributeValues.fromString(number))) @@ -211,20 +248,41 @@ public class Accounts extends AbstractDynamoDbStore { .build()) .build()); + originalPni.ifPresent(pni -> writeItems.add(TransactWriteItem.builder() + .delete(Delete.builder() + .tableName(phoneNumberIdentifierConstraintTableName) + .key(Map.of(ATTR_PNI_UUID, AttributeValues.fromUUID(pni))) + .build()) + .build())); + + writeItems.add(TransactWriteItem.builder() + .put(Put.builder() + .tableName(phoneNumberIdentifierConstraintTableName) + .item(Map.of( + KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()), + ATTR_PNI_UUID, AttributeValues.fromUUID(phoneNumberIdentifier))) + .conditionExpression("attribute_not_exists(#pni)") + .expressionAttributeNames(Map.of("#pni", ATTR_PNI_UUID)) + .returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD) + .build()) + .build()); + writeItems.add( TransactWriteItem.builder() .update(Update.builder() .tableName(accountsTableName) .key(Map.of(KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) - .updateExpression("SET #data = :data, #number = :number, #cds = :cds ADD #version :version_increment") + .updateExpression("SET #data = :data, #number = :number, #pni = :pni, #cds = :cds ADD #version :version_increment") .conditionExpression("attribute_exists(#number) AND #version = :version") .expressionAttributeNames(Map.of("#number", ATTR_ACCOUNT_E164, "#data", ATTR_ACCOUNT_DATA, "#cds", ATTR_CANONICALLY_DISCOVERABLE, + "#pni", ATTR_PNI_UUID, "#version", ATTR_VERSION)) .expressionAttributeValues(Map.of( ":data", AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)), ":number", AttributeValues.fromString(number), + ":pni", AttributeValues.fromUUID(phoneNumberIdentifier), ":cds", AttributeValues.fromBool(account.shouldBeVisibleInDirectory()), ":version", AttributeValues.fromInt(account.getVersion()), ":version_increment", AttributeValues.fromInt(1))) @@ -243,7 +301,7 @@ public class Accounts extends AbstractDynamoDbStore { throw new IllegalArgumentException(e); } finally { if (!succeeded) { - account.setNumber(originalNumber); + account.setNumber(originalNumber, originalPni.orElse(null)); } } }); @@ -251,57 +309,121 @@ public class Accounts extends AbstractDynamoDbStore { public void update(Account account) throws ContestedOptimisticLockException { UPDATE_TIMER.record(() -> { - UpdateItemRequest updateItemRequest; - try { - updateItemRequest = UpdateItemRequest.builder() - .tableName(accountsTableName) - .key(Map.of(KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) - .updateExpression("SET #data = :data, #cds = :cds ADD #version :version_increment") - .conditionExpression("attribute_exists(#number) AND #version = :version") - .expressionAttributeNames(Map.of("#number", ATTR_ACCOUNT_E164, - "#data", ATTR_ACCOUNT_DATA, - "#cds", ATTR_CANONICALLY_DISCOVERABLE, - "#version", ATTR_VERSION)) - .expressionAttributeValues(Map.of( - ":data", AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)), - ":cds", AttributeValues.fromBool(account.shouldBeVisibleInDirectory()), - ":version", AttributeValues.fromInt(account.getVersion()), - ":version_increment", AttributeValues.fromInt(1))) - .returnValues(ReturnValue.UPDATED_NEW) - .build(); + final List transactWriteItems = new ArrayList<>(2); + try { + final TransactWriteItem updateAccountWriteItem; + + if (account.getPhoneNumberIdentifier().isPresent()) { + updateAccountWriteItem = TransactWriteItem.builder() + .update(Update.builder() + .tableName(accountsTableName) + .key(Map.of(KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) + .updateExpression("SET #data = :data, #cds = :cds, #pni = :pni ADD #version :version_increment") + .conditionExpression("attribute_exists(#number) AND #version = :version") + .expressionAttributeNames(Map.of("#number", ATTR_ACCOUNT_E164, + "#data", ATTR_ACCOUNT_DATA, + "#cds", ATTR_CANONICALLY_DISCOVERABLE, + "#version", ATTR_VERSION, + "#pni", ATTR_PNI_UUID)) + .expressionAttributeValues(Map.of( + ":data", AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)), + ":cds", AttributeValues.fromBool(account.shouldBeVisibleInDirectory()), + ":version", AttributeValues.fromInt(account.getVersion()), + ":version_increment", AttributeValues.fromInt(1), + ":pni", AttributeValues.fromUUID(account.getPhoneNumberIdentifier().get()))) + .build()) + .build(); + } else { + updateAccountWriteItem = TransactWriteItem.builder() + .update(Update.builder() + .tableName(accountsTableName) + .key(Map.of(KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) + .updateExpression("SET #data = :data, #cds = :cds ADD #version :version_increment") + .conditionExpression("attribute_exists(#number) AND #version = :version") + .expressionAttributeNames(Map.of("#number", ATTR_ACCOUNT_E164, + "#data", ATTR_ACCOUNT_DATA, + "#cds", ATTR_CANONICALLY_DISCOVERABLE, + "#version", ATTR_VERSION)) + .expressionAttributeValues(Map.of( + ":data", AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)), + ":cds", AttributeValues.fromBool(account.shouldBeVisibleInDirectory()), + ":version", AttributeValues.fromInt(account.getVersion()), + ":version_increment", AttributeValues.fromInt(1))) + .build()) + .build(); + } + + transactWriteItems.add(updateAccountWriteItem); + + // TODO Remove after initial migration to phone number identifiers + account.getPhoneNumberIdentifier().ifPresent(phoneNumberIdentifier -> transactWriteItems.add( + TransactWriteItem.builder() + .put(Put.builder() + .tableName(phoneNumberIdentifierConstraintTableName) + .item(Map.of( + ATTR_PNI_UUID, AttributeValues.fromUUID(account.getPhoneNumberIdentifier().get()), + KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) + .conditionExpression("attribute_not_exists(#pni) OR (attribute_exists(#pni) AND #uuid = :uuid)") + .expressionAttributeNames(Map.of("#uuid", KEY_ACCOUNT_UUID, "#pni", ATTR_PNI_UUID)) + .expressionAttributeValues( + Map.of(":uuid", AttributeValues.fromUUID(account.getUuid()))) + .returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD) + .build()) + .build())); } catch (JsonProcessingException e) { throw new IllegalArgumentException(e); } try { - UpdateItemResponse response = client.updateItem(updateItemRequest); + client.transactWriteItems(TransactWriteItemsRequest.builder() + .transactItems(transactWriteItems) + .build()); - account.setVersion(AttributeValues.getInt(response.attributes(), "V", account.getVersion() + 1)); + account.setVersion(account.getVersion() + 1); } catch (final TransactionConflictException e) { throw new ContestedOptimisticLockException(); - } catch (final ConditionalCheckFailedException e) { + } catch (final TransactionCanceledException e) { - // the exception doesn’t give details about which condition failed, - // but we can infer it was an optimistic locking failure if the UUID is known - throw get(account.getUuid()).isPresent() ? new ContestedOptimisticLockException() : e; + if ("ConditionalCheckFailed".equals(e.cancellationReasons().get(1).code())) { + log.error("Conflicting phone number mapping exists for account {}, PNI {}", account.getUuid(), account.getPhoneNumberIdentifier()); + throw e; + } + + // We can infer an optimistic locking failure if the UUID is known + throw getByAccountIdentifier(account.getUuid()).isPresent() ? new ContestedOptimisticLockException() : e; } }); } - public Optional get(String number) { + public Optional getByE164(String number) { return GET_BY_NUMBER_TIMER.record(() -> { final GetItemResponse response = client.getItem(GetItemRequest.builder() - .tableName(phoneNumbersTableName) + .tableName(phoneNumberConstraintTableName) .key(Map.of(ATTR_ACCOUNT_E164, AttributeValues.fromString(number))) .build()); return Optional.ofNullable(response.item()) .map(item -> item.get(KEY_ACCOUNT_UUID)) - .map(uuid -> accountByUuid(uuid)) + .map(this::accountByUuid) + .map(Accounts::fromItem); + }); + } + + public Optional getByPhoneNumberIdentifier(final UUID phoneNumberIdentifier) { + return GET_BY_PNI_TIMER.record(() -> { + + final GetItemResponse response = client.getItem(GetItemRequest.builder() + .tableName(phoneNumberIdentifierConstraintTableName) + .key(Map.of(ATTR_PNI_UUID, AttributeValues.fromUUID(phoneNumberIdentifier))) + .build()); + + return Optional.ofNullable(response.item()) + .map(item -> item.get(KEY_ACCOUNT_UUID)) + .map(this::accountByUuid) .map(Accounts::fromItem); }); } @@ -315,7 +437,7 @@ public class Accounts extends AbstractDynamoDbStore { return r.item().isEmpty() ? null : r.item(); } - public Optional get(UUID uuid) { + public Optional getByAccountIdentifier(UUID uuid) { return GET_BY_UUID_TIMER.record(() -> Optional.ofNullable(accountByUuid(AttributeValues.fromUUID(uuid))) .map(Accounts::fromItem)); @@ -324,13 +446,11 @@ public class Accounts extends AbstractDynamoDbStore { public void delete(UUID uuid) { DELETE_TIMER.record(() -> { - Optional maybeAccount = get(uuid); - - maybeAccount.ifPresent(account -> { + getByAccountIdentifier(uuid).ifPresent(account -> { TransactWriteItem phoneNumberDelete = TransactWriteItem.builder() .delete(Delete.builder() - .tableName(phoneNumbersTableName) + .tableName(phoneNumberConstraintTableName) .key(Map.of(ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()))) .build()) .build(); @@ -342,8 +462,17 @@ public class Accounts extends AbstractDynamoDbStore { .build()) .build(); + final List transactWriteItems = new ArrayList<>(List.of(phoneNumberDelete, accountDelete)); + + account.getPhoneNumberIdentifier().ifPresent(pni -> transactWriteItems.add(TransactWriteItem.builder() + .delete(Delete.builder() + .tableName(phoneNumberIdentifierConstraintTableName) + .key(Map.of(ATTR_PNI_UUID, AttributeValues.fromUUID(pni))) + .build()) + .build())); + TransactWriteItemsRequest request = TransactWriteItemsRequest.builder() - .transactItems(phoneNumberDelete, accountDelete).build(); + .transactItems(transactWriteItems).build(); client.transactWriteItems(request); }); @@ -393,7 +522,7 @@ public class Accounts extends AbstractDynamoDbStore { } try { Account account = SystemMapper.getMapper().readValue(item.get(ATTR_ACCOUNT_DATA).b().asByteArray(), Account.class); - account.setNumber(item.get(ATTR_ACCOUNT_E164).s()); + account.setNumber(item.get(ATTR_ACCOUNT_E164).s(), AttributeValues.getUUID(item, ATTR_PNI_UUID, null)); account.setUuid(UUIDUtil.fromByteBuffer(item.get(KEY_ACCOUNT_UUID).b().asByteBuffer())); account.setVersion(Integer.parseInt(item.get(ATTR_VERSION).n())); account.setCanonicallyDiscoverable(Optional.ofNullable(item.get(ATTR_CANONICALLY_DISCOVERABLE)).map(av -> av.bool()).orElse(false)); 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 430bd062b..db7cc42ed 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -52,6 +52,7 @@ public class AccountsManager { private static final Timer redisSetTimer = metricRegistry.timer(name(AccountsManager.class, "redisSet" )); private static final Timer redisNumberGetTimer = metricRegistry.timer(name(AccountsManager.class, "redisNumberGet")); + private static final Timer redisPniGetTimer = metricRegistry.timer(name(AccountsManager.class, "redisPniGet")); private static final Timer redisUuidGetTimer = metricRegistry.timer(name(AccountsManager.class, "redisUuidGet" )); private static final Timer redisDeleteTimer = metricRegistry.timer(name(AccountsManager.class, "redisDelete" )); @@ -63,18 +64,19 @@ public class AccountsManager { private final Logger logger = LoggerFactory.getLogger(AccountsManager.class); private final Accounts accounts; + private final PhoneNumberIdentifiers phoneNumberIdentifiers; private final FaultTolerantRedisCluster cacheCluster; private final DeletedAccountsManager deletedAccountsManager; - private final DirectoryQueue directoryQueue; - private final KeysDynamoDb keysDynamoDb; + private final DirectoryQueue directoryQueue; + private final KeysDynamoDb keysDynamoDb; private final MessagesManager messagesManager; private final UsernamesManager usernamesManager; - private final ProfilesManager profilesManager; + private final ProfilesManager profilesManager; private final StoredVerificationCodeManager pendingAccounts; - private final SecureStorageClient secureStorageClient; - private final SecureBackupClient secureBackupClient; + private final SecureStorageClient secureStorageClient; + private final SecureBackupClient secureBackupClient; private final ClientPresenceManager clientPresenceManager; - private final ObjectMapper mapper; + private final ObjectMapper mapper; private final Clock clock; public enum DeletionReason { @@ -89,10 +91,13 @@ public class AccountsManager { } } - public AccountsManager(Accounts accounts, FaultTolerantRedisCluster cacheCluster, + public AccountsManager(final Accounts accounts, + final PhoneNumberIdentifiers phoneNumberIdentifiers, + final FaultTolerantRedisCluster cacheCluster, final DeletedAccountsManager deletedAccountsManager, final DirectoryQueue directoryQueue, - final KeysDynamoDb keysDynamoDb, final MessagesManager messagesManager, + final KeysDynamoDb keysDynamoDb, + final MessagesManager messagesManager, final UsernamesManager usernamesManager, final ProfilesManager profilesManager, final StoredVerificationCodeManager pendingAccounts, @@ -101,6 +106,7 @@ public class AccountsManager { final ClientPresenceManager clientPresenceManager, final Clock clock) { this.accounts = accounts; + this.phoneNumberIdentifiers = phoneNumberIdentifiers; this.cacheCluster = cacheCluster; this.deletedAccountsManager = deletedAccountsManager; this.directoryQueue = directoryQueue; @@ -137,7 +143,7 @@ public class AccountsManager { device.setLastSeen(Util.todayInMillis()); device.setUserAgent(signalAgent); - account.setNumber(number); + account.setNumber(number, phoneNumberIdentifiers.getPhoneNumberIdentifier(number)); account.setUuid(maybeRecentlyDeletedUuid.orElseGet(UUID::randomUUID)); account.addDevice(device); account.setRegistrationLockFromAttributes(accountAttributes); @@ -148,7 +154,7 @@ public class AccountsManager { final UUID originalUuid = account.getUuid(); - boolean freshUser = dynamoCreate(account); + boolean freshUser = accounts.create(account); // create() sometimes updates the UUID, if there was a number conflict. // for metrics, we want secondary to run with the same original UUID @@ -210,7 +216,7 @@ public class AccountsManager { deletedAccountsManager.lockAndPut(account.getNumber(), number, () -> { redisDelete(account); - final Optional maybeExistingAccount = get(number); + final Optional maybeExistingAccount = getByE164(number); final Optional displacedUuid; if (maybeExistingAccount.isPresent()) { @@ -221,12 +227,13 @@ public class AccountsManager { } final UUID uuid = account.getUuid(); + final UUID phoneNumberIdentifier = phoneNumberIdentifiers.getPhoneNumberIdentifier(number); final Account numberChangedAccount = updateWithRetries( account, a -> true, - a -> dynamoChangeNumber(a, number), - () -> dynamoGet(uuid).orElseThrow()); + a -> accounts.changeNumber(a, number, phoneNumberIdentifier), + () -> accounts.getByAccountIdentifier(uuid).orElseThrow()); updatedAccount.set(numberChangedAccount); directoryQueue.changePhoneNumber(numberChangedAccount, originalNumber, number); @@ -286,7 +293,10 @@ public class AccountsManager { final UUID uuid = account.getUuid(); final String originalNumber = account.getNumber(); - updatedAccount = updateWithRetries(account, updater, this::dynamoUpdate, () -> dynamoGet(uuid).get()); + updatedAccount = updateWithRetries(account, + updater, + accounts::update, + () -> accounts.getByAccountIdentifier(uuid).orElseThrow()); assert updatedAccount.getNumber().equals(originalNumber); @@ -355,12 +365,12 @@ public class AccountsManager { }); } - public Optional get(String number) { + public Optional getByE164(String number) { try (Timer.Context ignored = getByNumberTimer.time()) { - Optional account = redisGet(number); + Optional account = redisGetByE164(number); if (account.isEmpty()) { - account = dynamoGet(number); + account = accounts.getByE164(number); account.ifPresent(this::redisSet); } @@ -368,12 +378,25 @@ public class AccountsManager { } } - public Optional get(UUID uuid) { - try (Timer.Context ignored = getByUuidTimer.time()) { - Optional account = redisGet(uuid); + public Optional getByPhoneNumberIdentifier(UUID pni) { + try (Timer.Context ignored = getByNumberTimer.time()) { + Optional account = redisGetByPhoneNumberIdentifier(pni); if (account.isEmpty()) { - account = dynamoGet(uuid); + account = accounts.getByPhoneNumberIdentifier(pni); + account.ifPresent(this::redisSet); + } + + return account; + } + } + + public Optional getByAccountIdentifier(UUID uuid) { + try (Timer.Context ignored = getByUuidTimer.time()) { + Optional account = redisGetByAccountIdentifier(uuid); + + if (account.isEmpty()) { + account = accounts.getByAccountIdentifier(uuid); account.ifPresent(this::redisSet); } @@ -417,19 +440,24 @@ public class AccountsManager { keysDynamoDb.delete(account.getUuid()); messagesManager.clear(account.getUuid()); + account.getPhoneNumberIdentifier().ifPresent(pni -> { + keysDynamoDb.delete(pni); + messagesManager.clear(pni); + }); + deleteStorageServiceDataFuture.join(); deleteBackupServiceDataFuture.join(); redisDelete(account); - dynamoDelete(account); + accounts.delete(account.getUuid()); RedisOperation.unchecked(() -> account.getDevices().forEach(device -> clientPresenceManager.displacePresence(account.getUuid(), device.getId()))); } - private String getAccountMapKey(String number) { - return "AccountMap::" + number; + private String getAccountMapKey(String key) { + return "AccountMap::" + key; } private String getAccountEntityKey(UUID uuid) { @@ -443,6 +471,9 @@ public class AccountsManager { cacheCluster.useCluster(connection -> { final RedisAdvancedClusterCommands commands = connection.sync(); + account.getPhoneNumberIdentifier().ifPresent(pni -> + commands.set(getAccountMapKey(pni.toString()), account.getUuid().toString())); + commands.set(getAccountMapKey(account.getNumber()), account.getUuid().toString()); commands.set(getAccountEntityKey(account.getUuid()), accountJson); }); @@ -451,11 +482,19 @@ public class AccountsManager { } } - private Optional redisGet(String number) { - try (Timer.Context ignored = redisNumberGetTimer.time()) { - final String uuid = cacheCluster.withCluster(connection -> connection.sync().get(getAccountMapKey(number))); + private Optional redisGetByPhoneNumberIdentifier(UUID uuid) { + return redisGetBySecondaryKey(uuid.toString(), redisPniGetTimer); + } - if (uuid != null) return redisGet(UUID.fromString(uuid)); + private Optional redisGetByE164(String e164) { + return redisGetBySecondaryKey(e164, redisNumberGetTimer); + } + + private Optional redisGetBySecondaryKey(String secondaryKey, Timer timer) { + try (Timer.Context ignored = timer.time()) { + final String uuid = cacheCluster.withCluster(connection -> connection.sync().get(getAccountMapKey(secondaryKey))); + + if (uuid != null) return redisGetByAccountIdentifier(UUID.fromString(uuid)); else return Optional.empty(); } catch (IllegalArgumentException e) { logger.warn("Deserialization error", e); @@ -466,7 +505,7 @@ public class AccountsManager { } } - private Optional redisGet(UUID uuid) { + private Optional redisGetByAccountIdentifier(UUID uuid) { try (Timer.Context ignored = redisUuidGetTimer.time()) { final String json = cacheCluster.withCluster(connection -> connection.sync().get(getAccountEntityKey(uuid))); @@ -489,32 +528,11 @@ public class AccountsManager { private void redisDelete(final Account account) { try (final Timer.Context ignored = redisDeleteTimer.time()) { - cacheCluster.useCluster(connection -> connection.sync() - .del(getAccountMapKey(account.getNumber()), getAccountEntityKey(account.getUuid()))); + cacheCluster.useCluster(connection -> { + connection.sync().del(getAccountMapKey(account.getNumber()), getAccountEntityKey(account.getUuid())); + + account.getPhoneNumberIdentifier().ifPresent(pni -> connection.sync().del(getAccountMapKey(pni.toString()))); + }); } } - - private Optional dynamoGet(String number) { - return accounts.get(number); - } - - private Optional dynamoGet(UUID uuid) { - return accounts.get(uuid); - } - - private boolean dynamoCreate(Account account) { - return accounts.create(account); - } - - private void dynamoUpdate(Account account) { - accounts.update(account); - } - - private void dynamoDelete(final Account account) { - accounts.delete(account.getUuid()); - } - - private void dynamoChangeNumber(final Account account, final String number) { - accounts.changeNumber(account, number); - } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/ContactDiscoveryWriter.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/ContactDiscoveryWriter.java index b79350fa1..3f5c1b57a 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/ContactDiscoveryWriter.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/ContactDiscoveryWriter.java @@ -43,7 +43,7 @@ public class ContactDiscoveryWriter extends AccountDatabaseCrawlerListener { // It’s less than ideal, but crawler listeners currently must not call update() // with the accounts from the chunk, because updates cause the account instance to become stale. Instead, they // must get a new copy, which they are free to update. - accounts.get(account.getUuid()).ifPresent(a -> accounts.update(a, NOOP_UPDATER)); + accounts.getByAccountIdentifier(account.getUuid()).ifPresent(a -> accounts.update(a, NOOP_UPDATER)); } } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/MessagePersister.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/MessagePersister.java index ef874d5cc..9f69feac7 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/MessagePersister.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/MessagePersister.java @@ -146,7 +146,7 @@ public class MessagePersister implements Managed { @VisibleForTesting void persistQueue(final UUID accountUuid, final long deviceId) { - final Optional maybeAccount = accountsManager.get(accountUuid); + final Optional maybeAccount = accountsManager.getByAccountIdentifier(accountUuid); if (maybeAccount.isEmpty()) { logger.error("No account record found for account {}", accountUuid); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/NonNormalizedAccountCrawlerListener.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/NonNormalizedAccountCrawlerListener.java index 62fa75940..bc69e9904 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/NonNormalizedAccountCrawlerListener.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/NonNormalizedAccountCrawlerListener.java @@ -62,7 +62,7 @@ public class NonNormalizedAccountCrawlerListener extends AccountDatabaseCrawlerL workingNonNormalizedNumbers++; try { - final Optional maybeConflictingAccount = accountsManager.get(getNormalizedNumber(account)); + final Optional maybeConflictingAccount = accountsManager.getByE164(getNormalizedNumber(account)); if (maybeConflictingAccount.isPresent()) { workingConflictingNumbers++; diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiers.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiers.java new file mode 100644 index 000000000..d1bb78d1d --- /dev/null +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiers.java @@ -0,0 +1,85 @@ +/* + * Copyright 2013-2021 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.whispersystems.textsecuregcm.storage; + +import com.google.common.annotations.VisibleForTesting; +import io.micrometer.core.instrument.Metrics; +import io.micrometer.core.instrument.Timer; +import org.whispersystems.textsecuregcm.util.AttributeValues; +import software.amazon.awssdk.services.dynamodb.DynamoDbClient; +import software.amazon.awssdk.services.dynamodb.model.GetItemRequest; +import software.amazon.awssdk.services.dynamodb.model.GetItemResponse; +import software.amazon.awssdk.services.dynamodb.model.ReturnValue; +import software.amazon.awssdk.services.dynamodb.model.UpdateItemRequest; +import software.amazon.awssdk.services.dynamodb.model.UpdateItemResponse; +import java.util.Map; +import java.util.UUID; + +import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name; + +/** + * Manages a global, persistent mapping of phone numbers to phone number identifiers regardless of whether those + * numbers/identifiers are actually associated with an account. + */ +public class PhoneNumberIdentifiers { + + private final DynamoDbClient dynamoDbClient; + private final String tableName; + + @VisibleForTesting + static final String KEY_E164 = "P"; + private static final String ATTR_PHONE_NUMBER_IDENTIFIER = "PNI"; + + private static final Timer GET_PNI_TIMER = Metrics.timer(name(PhoneNumberIdentifiers.class, "get")); + private static final Timer SET_PNI_TIMER = Metrics.timer(name(PhoneNumberIdentifiers.class, "set")); + + public PhoneNumberIdentifiers(final DynamoDbClient dynamoDbClient, final String tableName) { + this.dynamoDbClient = dynamoDbClient; + this.tableName = tableName; + } + + /** + * Returns the phone number identifier (PNI) associated with the given phone number. + * + * @param phoneNumber the phone number for which to retrieve a phone number identifier + * @return the phone number identifier associated with the given phone number + */ + public UUID getPhoneNumberIdentifier(final String phoneNumber) { + final GetItemResponse response = GET_PNI_TIMER.record(() -> dynamoDbClient.getItem(GetItemRequest.builder() + .tableName(tableName) + .key(Map.of(KEY_E164, AttributeValues.fromString(phoneNumber))) + .projectionExpression(ATTR_PHONE_NUMBER_IDENTIFIER) + .build())); + + final UUID phoneNumberIdentifier; + + if (response.hasItem()) { + phoneNumberIdentifier = AttributeValues.getUUID(response.item(), ATTR_PHONE_NUMBER_IDENTIFIER, null); + } else { + phoneNumberIdentifier = generatePhoneNumberIdentifierIfNotExists(phoneNumber); + } + + if (phoneNumberIdentifier == null) { + throw new RuntimeException("Could not retrieve phone number identifier from stored item"); + } + + return phoneNumberIdentifier; + } + + @VisibleForTesting + UUID generatePhoneNumberIdentifierIfNotExists(final String phoneNumber) { + final UpdateItemResponse response = SET_PNI_TIMER.record(() -> dynamoDbClient.updateItem(UpdateItemRequest.builder() + .tableName(tableName) + .key(Map.of(KEY_E164, AttributeValues.fromString(phoneNumber))) + .updateExpression("SET #pni = if_not_exists(#pni, :pni)") + .expressionAttributeNames(Map.of("#pni", ATTR_PHONE_NUMBER_IDENTIFIER)) + .expressionAttributeValues(Map.of(":pni", AttributeValues.fromUUID(UUID.randomUUID()))) + .returnValues(ReturnValue.ALL_NEW) + .build())); + + return AttributeValues.getUUID(response.attributes(), ATTR_PHONE_NUMBER_IDENTIFIER, null); + } +} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/PushFeedbackProcessor.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/PushFeedbackProcessor.java index 0d2b9d042..e31a11c5e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/PushFeedbackProcessor.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/PushFeedbackProcessor.java @@ -58,7 +58,7 @@ public class PushFeedbackProcessor extends AccountDatabaseCrawlerListener { if (update) { // fetch a new version, since the chunk is shared and implicitly read-only - accountsManager.get(account.getUuid()).ifPresent(accountToUpdate -> { + accountsManager.getByAccountIdentifier(account.getUuid()).ifPresent(accountToUpdate -> { accountsManager.update(accountToUpdate, a -> { for (Device device : a.getDevices()) { if (deviceNeedsUpdate(device)) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/RefreshingAccountAndDeviceSupplier.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/RefreshingAccountAndDeviceSupplier.java index b4a40aaae..1c12e1177 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/RefreshingAccountAndDeviceSupplier.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/RefreshingAccountAndDeviceSupplier.java @@ -24,7 +24,7 @@ public class RefreshingAccountAndDeviceSupplier implements Supplier get() { if (account.isStale()) { - account = accountsManager.get(account.getUuid()) + account = accountsManager.getByAccountIdentifier(account.getUuid()) .orElseThrow(() -> new RuntimeException("Could not find account")); device = account.getDevice(device.getId()) .orElseThrow(() -> new RefreshingAccountAndDeviceNotFoundException("Could not find device")); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/websocket/DeadLetterHandler.java b/service/src/main/java/org/whispersystems/textsecuregcm/websocket/DeadLetterHandler.java index 982d40a39..e67c96fbf 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/websocket/DeadLetterHandler.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/websocket/DeadLetterHandler.java @@ -47,7 +47,7 @@ public class DeadLetterHandler implements DispatchChannel { switch (pubSubMessage.getType().getNumber()) { case PubSubMessage.Type.DELIVER_VALUE: Envelope message = Envelope.parseFrom(pubSubMessage.getContent()); - Optional maybeAccount = accountsManager.get(address.getNumber()); + Optional maybeAccount = accountsManager.getByE164(address.getNumber()); if (maybeAccount.isPresent()) { messagesManager.insert(maybeAccount.get().getUuid(), address.getDeviceId(), message); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java index 3d660240a..cedefac7b 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java @@ -48,6 +48,7 @@ import org.whispersystems.textsecuregcm.storage.KeysDynamoDb; import org.whispersystems.textsecuregcm.storage.MessagesCache; import org.whispersystems.textsecuregcm.storage.MessagesDynamoDb; import org.whispersystems.textsecuregcm.storage.MessagesManager; +import org.whispersystems.textsecuregcm.storage.PhoneNumberIdentifiers; import org.whispersystems.textsecuregcm.storage.Profiles; import org.whispersystems.textsecuregcm.storage.ProfilesManager; import org.whispersystems.textsecuregcm.storage.ReportMessageDynamoDb; @@ -112,6 +113,9 @@ public class DeleteUserCommand extends EnvironmentCommand account = accountsManager.get(user); + Optional account = accountsManager.getByE164(user); if (account.isPresent()) { accountsManager.delete(account.get(), DeletionReason.ADMIN_DELETED); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/SetUserDiscoverabilityCommand.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/SetUserDiscoverabilityCommand.java index 179e205f6..8dc56615e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/SetUserDiscoverabilityCommand.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/SetUserDiscoverabilityCommand.java @@ -46,6 +46,7 @@ import org.whispersystems.textsecuregcm.storage.KeysDynamoDb; import org.whispersystems.textsecuregcm.storage.MessagesCache; import org.whispersystems.textsecuregcm.storage.MessagesDynamoDb; import org.whispersystems.textsecuregcm.storage.MessagesManager; +import org.whispersystems.textsecuregcm.storage.PhoneNumberIdentifiers; import org.whispersystems.textsecuregcm.storage.Profiles; import org.whispersystems.textsecuregcm.storage.ProfilesManager; import org.whispersystems.textsecuregcm.storage.ReportMessageDynamoDb; @@ -114,6 +115,9 @@ public class SetUserDiscoverabilityCommand extends EnvironmentCommand maybeAccount; try { - maybeAccount = accountsManager.get(UUID.fromString(namespace.getString("user"))); + maybeAccount = accountsManager.getByAccountIdentifier(UUID.fromString(namespace.getString("user"))); } catch (final IllegalArgumentException e) { - maybeAccount = accountsManager.get(namespace.getString("user")); + maybeAccount = accountsManager.getByE164(namespace.getString("user")); } maybeAccount.ifPresentOrElse(account -> { diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/auth/AuthEnablementRefreshRequirementProviderTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/auth/AuthEnablementRefreshRequirementProviderTest.java index 4c0847ed4..378989180 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/auth/AuthEnablementRefreshRequirementProviderTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/auth/AuthEnablementRefreshRequirementProviderTest.java @@ -128,7 +128,7 @@ class AuthEnablementRefreshRequirementProviderTest { account.addDevice(authenticatedDevice); LongStream.range(2, 4).forEach(deviceId -> account.addDevice(DevicesHelper.createDevice(deviceId))); - when(accountsManager.get(uuid)).thenReturn(Optional.of(account)); + when(accountsManager.getByAccountIdentifier(uuid)).thenReturn(Optional.of(account)); account.getDevices() .forEach(device -> when(clientPresenceManager.isPresent(uuid, device.getId())).thenReturn(true)); @@ -310,7 +310,7 @@ class AuthEnablementRefreshRequirementProviderTest { assertEquals(200, response.getStatus()); - verify(accountsManager, never()).get(any(UUID.class)); + verify(accountsManager, never()).getByAccountIdentifier(any(UUID.class)); } @Nested diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticatorTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticatorTest.java index c6f570647..f3cb36ed4 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticatorTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticatorTest.java @@ -31,9 +31,6 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; -import org.whispersystems.textsecuregcm.auth.AuthenticatedAccount; -import org.whispersystems.textsecuregcm.auth.AuthenticationCredentials; -import org.whispersystems.textsecuregcm.auth.BaseAccountAuthenticator; import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.Device; @@ -62,11 +59,14 @@ class BaseAccountAuthenticatorTest { clock = mock(Clock.class); baseAccountAuthenticator = new BaseAccountAuthenticator(accountsManager, clock); - acct1 = new Account("+14088675309", AuthHelper.getRandomUUID(random), Set.of(new Device(1, null, null, null, + acct1 = new Account("+14088675309", AuthHelper.getRandomUUID(random), UUID.randomUUID(), + Set.of(new Device(1, null, null, null, null, null, null, false, 0, null, yesterday, 0, null, 0, null)), null); - acct2 = new Account("+14098675309", AuthHelper.getRandomUUID(random), Set.of(new Device(1, null, null, null, + acct2 = new Account("+14098675309", AuthHelper.getRandomUUID(random), UUID.randomUUID(), + Set.of(new Device(1, null, null, null, null, null, null, false, 0, null, yesterday, 0, null, 0, null)), null); - oldAccount = new Account("+14108675309", AuthHelper.getRandomUUID(random), Set.of(new Device(1, null, null, null, + oldAccount = new Account("+14108675309", AuthHelper.getRandomUUID(random), UUID.randomUUID(), + Set.of(new Device(1, null, null, null, null, null, null, false, 0, null, oldTime, 0, null, 0, null)), null); AccountsHelper.setupMockUpdate(accountsManager); @@ -156,7 +156,7 @@ class BaseAccountAuthenticatorTest { final AuthenticationCredentials credentials = mock(AuthenticationCredentials.class); when(clock.instant()).thenReturn(Instant.now()); - when(accountsManager.get(uuid)).thenReturn(Optional.of(account)); + when(accountsManager.getByAccountIdentifier(uuid)).thenReturn(Optional.of(account)); when(account.getUuid()).thenReturn(uuid); when(account.getDevice(deviceId)).thenReturn(Optional.of(device)); when(account.isEnabled()).thenReturn(true); @@ -184,7 +184,7 @@ class BaseAccountAuthenticatorTest { final AuthenticationCredentials credentials = mock(AuthenticationCredentials.class); when(clock.instant()).thenReturn(Instant.now()); - when(accountsManager.get(uuid)).thenReturn(Optional.of(account)); + when(accountsManager.getByAccountIdentifier(uuid)).thenReturn(Optional.of(account)); when(account.getUuid()).thenReturn(uuid); when(account.getDevice(deviceId)).thenReturn(Optional.of(device)); when(account.isEnabled()).thenReturn(true); @@ -213,7 +213,7 @@ class BaseAccountAuthenticatorTest { final AuthenticationCredentials credentials = mock(AuthenticationCredentials.class); when(clock.instant()).thenReturn(Instant.now()); - when(accountsManager.get(uuid)).thenReturn(Optional.of(account)); + when(accountsManager.getByAccountIdentifier(uuid)).thenReturn(Optional.of(account)); when(account.getUuid()).thenReturn(uuid); when(account.getDevice(deviceId)).thenReturn(Optional.of(device)); when(account.isEnabled()).thenReturn(false); @@ -251,7 +251,7 @@ class BaseAccountAuthenticatorTest { final AuthenticationCredentials credentials = mock(AuthenticationCredentials.class); when(clock.instant()).thenReturn(Instant.now()); - when(accountsManager.get(uuid)).thenReturn(Optional.of(account)); + when(accountsManager.getByAccountIdentifier(uuid)).thenReturn(Optional.of(account)); when(account.getUuid()).thenReturn(uuid); when(account.getDevice(deviceId)).thenReturn(Optional.of(device)); when(account.isEnabled()).thenReturn(true); @@ -278,7 +278,7 @@ class BaseAccountAuthenticatorTest { final AuthenticationCredentials credentials = mock(AuthenticationCredentials.class); when(clock.instant()).thenReturn(Instant.now()); - when(accountsManager.get(uuid)).thenReturn(Optional.of(account)); + when(accountsManager.getByAccountIdentifier(uuid)).thenReturn(Optional.of(account)); when(account.getUuid()).thenReturn(uuid); when(account.getDevice(deviceId)).thenReturn(Optional.of(device)); when(account.isEnabled()).thenReturn(true); @@ -303,7 +303,7 @@ class BaseAccountAuthenticatorTest { () -> baseAccountAuthenticator.authenticate(new BasicCredentials(username, "password"), true)); assertThat(maybeAuthenticatedAccount).isEmpty(); - verify(accountsManager, never()).get(any(UUID.class)); + verify(accountsManager, never()).getByAccountIdentifier(any(UUID.class)); } private static Stream testAuthenticateMalformedCredentials() { diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/push/ApnFallbackManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/push/ApnFallbackManagerTest.java index 1c2e6f385..357b749f2 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/push/ApnFallbackManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/push/ApnFallbackManagerTest.java @@ -57,8 +57,8 @@ class ApnFallbackManagerTest { when(account.getDevice(DEVICE_ID)).thenReturn(Optional.of(device)); final AccountsManager accountsManager = mock(AccountsManager.class); - when(accountsManager.get(ACCOUNT_NUMBER)).thenReturn(Optional.of(account)); - when(accountsManager.get(ACCOUNT_UUID)).thenReturn(Optional.of(account)); + when(accountsManager.getByE164(ACCOUNT_NUMBER)).thenReturn(Optional.of(account)); + when(accountsManager.getByAccountIdentifier(ACCOUNT_UUID)).thenReturn(Optional.of(account)); apnSender = mock(APNSender.class); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java index 9e14e1087..ee1373fa5 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java @@ -6,6 +6,7 @@ package org.whispersystems.textsecuregcm.storage; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; @@ -49,6 +50,8 @@ class AccountsManagerChangeNumberIntegrationTest { private static final String ACCOUNTS_TABLE_NAME = "accounts_test"; private static final String NUMBERS_TABLE_NAME = "numbers_test"; + private static final String PNI_ASSIGNMENT_TABLE_NAME = "pni_assignment_test"; + private static final String PNI_TABLE_NAME = "pni_test"; private static final String NEEDS_RECONCILIATION_INDEX_NAME = "needs_reconciliation_test"; private static final String DELETED_ACCOUNTS_LOCK_TABLE_NAME = "deleted_accounts_lock_test"; private static final int SCAN_PAGE_SIZE = 1; @@ -92,6 +95,16 @@ class AccountsManagerChangeNumberIntegrationTest { .attributeType(ScalarAttributeType.S).build()) .build(); + @RegisterExtension + static DynamoDbExtension PNI_DYNAMO_EXTENSION = DynamoDbExtension.builder() + .tableName(PNI_TABLE_NAME) + .hashKey(PhoneNumberIdentifiers.KEY_E164) + .attributeDefinition(AttributeDefinition.builder() + .attributeName(PhoneNumberIdentifiers.KEY_E164) + .attributeType(ScalarAttributeType.S) + .build()) + .build(); + @RegisterExtension static RedisClusterExtension CACHE_CLUSTER_EXTENSION = RedisClusterExtension.builder().build(); @@ -120,14 +133,33 @@ class AccountsManagerChangeNumberIntegrationTest { ACCOUNTS_DYNAMO_EXTENSION.getDynamoDbClient().createTable(createNumbersTableRequest); } + { + CreateTableRequest createPhoneNumberIdentifierTableRequest = CreateTableRequest.builder() + .tableName(PNI_ASSIGNMENT_TABLE_NAME) + .keySchema(KeySchemaElement.builder() + .attributeName(Accounts.ATTR_PNI_UUID) + .keyType(KeyType.HASH) + .build()) + .attributeDefinitions(AttributeDefinition.builder() + .attributeName(Accounts.ATTR_PNI_UUID) + .attributeType(ScalarAttributeType.B) + .build()) + .provisionedThroughput(DynamoDbExtension.DEFAULT_PROVISIONED_THROUGHPUT) + .build(); + + ACCOUNTS_DYNAMO_EXTENSION.getDynamoDbClient().createTable(createPhoneNumberIdentifierTableRequest); + } + final Accounts accounts = new Accounts( ACCOUNTS_DYNAMO_EXTENSION.getDynamoDbClient(), ACCOUNTS_DYNAMO_EXTENSION.getTableName(), NUMBERS_TABLE_NAME, + PNI_ASSIGNMENT_TABLE_NAME, SCAN_PAGE_SIZE); { - final DynamicConfigurationManager dynamicConfigurationManager = mock(DynamicConfigurationManager.class); + @SuppressWarnings("unchecked") final DynamicConfigurationManager dynamicConfigurationManager = + mock(DynamicConfigurationManager.class); DynamicConfiguration dynamicConfiguration = new DynamicConfiguration(); when(dynamicConfigurationManager.getConfiguration()).thenReturn(dynamicConfiguration); @@ -148,8 +180,12 @@ class AccountsManagerChangeNumberIntegrationTest { clientPresenceManager = mock(ClientPresenceManager.class); + final PhoneNumberIdentifiers phoneNumberIdentifiers = + new PhoneNumberIdentifiers(PNI_DYNAMO_EXTENSION.getDynamoDbClient(), PNI_TABLE_NAME); + accountsManager = new AccountsManager( accounts, + phoneNumberIdentifiers, CACHE_CLUSTER_EXTENSION.getRedisCluster(), deletedAccountsManager, mock(DirectoryQueue.class), @@ -172,15 +208,17 @@ class AccountsManagerChangeNumberIntegrationTest { final Account account = accountsManager.create(originalNumber, "password", null, new AccountAttributes(), new ArrayList<>()); final UUID originalUuid = account.getUuid(); + final UUID originalPni = account.getPhoneNumberIdentifier().orElseThrow(); accountsManager.changeNumber(account, secondNumber); - assertTrue(accountsManager.get(originalNumber).isEmpty()); + assertTrue(accountsManager.getByE164(originalNumber).isEmpty()); - assertTrue(accountsManager.get(secondNumber).isPresent()); - assertEquals(Optional.of(originalUuid), accountsManager.get(secondNumber).map(Account::getUuid)); + assertTrue(accountsManager.getByE164(secondNumber).isPresent()); + assertEquals(originalUuid, accountsManager.getByE164(secondNumber).map(Account::getUuid).orElseThrow()); + assertNotEquals(originalPni, accountsManager.getByE164(secondNumber).flatMap(Account::getPhoneNumberIdentifier).orElseThrow()); - assertEquals(secondNumber, accountsManager.get(originalUuid).map(Account::getNumber).orElseThrow()); + assertEquals(secondNumber, accountsManager.getByAccountIdentifier(originalUuid).map(Account::getNumber).orElseThrow()); assertEquals(Optional.empty(), deletedAccounts.findUuid(originalNumber)); assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); @@ -193,16 +231,18 @@ class AccountsManagerChangeNumberIntegrationTest { Account account = accountsManager.create(originalNumber, "password", null, new AccountAttributes(), new ArrayList<>()); final UUID originalUuid = account.getUuid(); + final UUID originalPni = account.getPhoneNumberIdentifier().orElseThrow(); account = accountsManager.changeNumber(account, secondNumber); accountsManager.changeNumber(account, originalNumber); - assertTrue(accountsManager.get(originalNumber).isPresent()); - assertEquals(Optional.of(originalUuid), accountsManager.get(originalNumber).map(Account::getUuid)); + assertTrue(accountsManager.getByE164(originalNumber).isPresent()); + assertEquals(originalUuid, accountsManager.getByE164(originalNumber).map(Account::getUuid).orElseThrow()); + assertEquals(originalPni, accountsManager.getByE164(originalNumber).flatMap(Account::getPhoneNumberIdentifier).orElseThrow()); - assertTrue(accountsManager.get(secondNumber).isEmpty()); + assertTrue(accountsManager.getByE164(secondNumber).isEmpty()); - assertEquals(originalNumber, accountsManager.get(originalUuid).map(Account::getNumber).orElseThrow()); + assertEquals(originalNumber, accountsManager.getByAccountIdentifier(originalUuid).map(Account::getNumber).orElseThrow()); assertEquals(Optional.empty(), deletedAccounts.findUuid(originalNumber)); assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); @@ -223,12 +263,12 @@ class AccountsManagerChangeNumberIntegrationTest { accountsManager.changeNumber(account, secondNumber); - assertTrue(accountsManager.get(originalNumber).isEmpty()); + assertTrue(accountsManager.getByE164(originalNumber).isEmpty()); - assertTrue(accountsManager.get(secondNumber).isPresent()); - assertEquals(Optional.of(originalUuid), accountsManager.get(secondNumber).map(Account::getUuid)); + assertTrue(accountsManager.getByE164(secondNumber).isPresent()); + assertEquals(Optional.of(originalUuid), accountsManager.getByE164(secondNumber).map(Account::getUuid)); - assertEquals(secondNumber, accountsManager.get(originalUuid).map(Account::getNumber).orElseThrow()); + assertEquals(secondNumber, accountsManager.getByAccountIdentifier(originalUuid).map(Account::getNumber).orElseThrow()); verify(clientPresenceManager).displacePresence(existingAccountUuid, Device.MASTER_ID); @@ -243,22 +283,26 @@ class AccountsManagerChangeNumberIntegrationTest { final Account account = accountsManager.create(originalNumber, "password", null, new AccountAttributes(), new ArrayList<>()); final UUID originalUuid = account.getUuid(); + final UUID originalPni = account.getPhoneNumberIdentifier().orElseThrow(); final Account existingAccount = accountsManager.create(secondNumber, "password", null, new AccountAttributes(), new ArrayList<>()); final UUID existingAccountUuid = existingAccount.getUuid(); - accountsManager.changeNumber(account, secondNumber); + final Account changedNumberAccount = accountsManager.changeNumber(account, secondNumber); + final UUID secondPni = changedNumberAccount.getPhoneNumberIdentifier().orElseThrow(); final Account reRegisteredAccount = accountsManager.create(originalNumber, "password", null, new AccountAttributes(), new ArrayList<>()); assertEquals(existingAccountUuid, reRegisteredAccount.getUuid()); + assertEquals(originalPni, reRegisteredAccount.getPhoneNumberIdentifier().orElseThrow()); assertEquals(Optional.empty(), deletedAccounts.findUuid(originalNumber)); assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); - accountsManager.changeNumber(reRegisteredAccount, secondNumber); + final Account changedNumberReRegisteredAccount = accountsManager.changeNumber(reRegisteredAccount, secondNumber); assertEquals(Optional.of(originalUuid), deletedAccounts.findUuid(originalNumber)); assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + assertEquals(secondPni, changedNumberReRegisteredAccount.getPhoneNumberIdentifier().orElseThrow()); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java index a653f11ce..509bd5eeb 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java @@ -15,6 +15,7 @@ import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import io.lettuce.core.cluster.api.sync.RedisAdvancedClusterCommands; import java.io.IOException; @@ -35,6 +36,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.mockito.ArgumentCaptor; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; import org.whispersystems.textsecuregcm.auth.AuthenticationCredentials; import org.whispersystems.textsecuregcm.entities.AccountAttributes; import org.whispersystems.textsecuregcm.entities.SignedPreKey; @@ -55,6 +58,7 @@ class AccountsManagerConcurrentModificationIntegrationTest { private static final String ACCOUNTS_TABLE_NAME = "accounts_test"; private static final String NUMBERS_TABLE_NAME = "numbers_test"; + private static final String PNI_TABLE_NAME = "pni_test"; private static final int SCAN_PAGE_SIZE = 1; @@ -96,10 +100,28 @@ class AccountsManagerConcurrentModificationIntegrationTest { dynamoDbExtension.getDynamoDbClient().createTable(createNumbersTableRequest); } + { + CreateTableRequest createPhoneNumberIdentifierTableRequest = CreateTableRequest.builder() + .tableName(PNI_TABLE_NAME) + .keySchema(KeySchemaElement.builder() + .attributeName(Accounts.ATTR_PNI_UUID) + .keyType(KeyType.HASH) + .build()) + .attributeDefinitions(AttributeDefinition.builder() + .attributeName(Accounts.ATTR_PNI_UUID) + .attributeType(ScalarAttributeType.B) + .build()) + .provisionedThroughput(DynamoDbExtension.DEFAULT_PROVISIONED_THROUGHPUT) + .build(); + + dynamoDbExtension.getDynamoDbClient().createTable(createPhoneNumberIdentifierTableRequest); + } + accounts = new Accounts( dynamoDbExtension.getDynamoDbClient(), dynamoDbExtension.getTableName(), NUMBERS_TABLE_NAME, + PNI_TABLE_NAME, SCAN_PAGE_SIZE); { @@ -114,8 +136,13 @@ class AccountsManagerConcurrentModificationIntegrationTest { return null; }).when(deletedAccountsManager).lockAndTake(anyString(), any()); + final PhoneNumberIdentifiers phoneNumberIdentifiers = mock(PhoneNumberIdentifiers.class); + when(phoneNumberIdentifiers.getPhoneNumberIdentifier(anyString())) + .thenAnswer((Answer) invocation -> UUID.randomUUID()); + accountsManager = new AccountsManager( accounts, + phoneNumberIdentifiers, RedisClusterHelper.buildMockRedisCluster(commands), deletedAccountsManager, mock(DirectoryQueue.class), @@ -185,8 +212,8 @@ class AccountsManagerConcurrentModificationIntegrationTest { modifyDevice(uuid, Device.MASTER_ID, device -> device.setName("deviceName")) ).join(); - final Account managerAccount = accountsManager.get(uuid).orElseThrow(); - final Account dynamoAccount = accounts.get(uuid).orElseThrow(); + final Account managerAccount = accountsManager.getByAccountIdentifier(uuid).orElseThrow(); + final Account dynamoAccount = accounts.getByAccountIdentifier(uuid).orElseThrow(); final Account redisAccount = getLastAccountFromRedisMock(commands); @@ -225,7 +252,7 @@ class AccountsManagerConcurrentModificationIntegrationTest { private CompletableFuture modifyAccount(final UUID uuid, final Consumer accountMutation) { return CompletableFuture.runAsync(() -> { - final Account account = accountsManager.get(uuid).orElseThrow(); + final Account account = accountsManager.getByAccountIdentifier(uuid).orElseThrow(); accountsManager.update(account, accountMutation); }, mutationExecutor); } @@ -233,7 +260,7 @@ class AccountsManagerConcurrentModificationIntegrationTest { private CompletableFuture modifyDevice(final UUID uuid, final long deviceId, final Consumer deviceMutation) { return CompletableFuture.runAsync(() -> { - final Account account = accountsManager.get(uuid).orElseThrow(); + final Account account = accountsManager.getByAccountIdentifier(uuid).orElseThrow(); accountsManager.updateDevice(account, deviceId, deviceMutation); }, mutationExecutor); } 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 7073e6df6..837d3844e 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -12,12 +12,14 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.uuid.UUIDComparator; import io.github.resilience4j.circuitbreaker.CallNotPermittedException; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -25,11 +27,6 @@ import java.util.Optional; import java.util.Random; import java.util.Set; import java.util.UUID; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.LinkedBlockingDeque; -import java.util.concurrent.ThreadPoolExecutor; -import java.util.concurrent.TimeUnit; -import org.assertj.core.api.AssertionsForClassTypes; import org.jdbi.v3.core.transaction.TransactionException; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; @@ -38,15 +35,20 @@ import org.junit.jupiter.api.extension.RegisterExtension; import org.whispersystems.textsecuregcm.configuration.CircuitBreakerConfiguration; import org.whispersystems.textsecuregcm.entities.SignedPreKey; import org.whispersystems.textsecuregcm.util.AttributeValues; +import org.whispersystems.textsecuregcm.util.SystemMapper; import software.amazon.awssdk.services.dynamodb.DynamoDbClient; import software.amazon.awssdk.services.dynamodb.model.AttributeDefinition; -import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException; +import software.amazon.awssdk.services.dynamodb.model.AttributeValue; import software.amazon.awssdk.services.dynamodb.model.CreateTableRequest; import software.amazon.awssdk.services.dynamodb.model.GetItemRequest; import software.amazon.awssdk.services.dynamodb.model.GetItemResponse; import software.amazon.awssdk.services.dynamodb.model.KeySchemaElement; import software.amazon.awssdk.services.dynamodb.model.KeyType; +import software.amazon.awssdk.services.dynamodb.model.Put; +import software.amazon.awssdk.services.dynamodb.model.PutItemRequest; +import software.amazon.awssdk.services.dynamodb.model.ReturnValuesOnConditionCheckFailure; import software.amazon.awssdk.services.dynamodb.model.ScalarAttributeType; +import software.amazon.awssdk.services.dynamodb.model.TransactWriteItem; import software.amazon.awssdk.services.dynamodb.model.TransactWriteItemsRequest; import software.amazon.awssdk.services.dynamodb.model.TransactionCanceledException; import software.amazon.awssdk.services.dynamodb.model.TransactionConflictException; @@ -55,7 +57,8 @@ import software.amazon.awssdk.services.dynamodb.model.UpdateItemRequest; class AccountsTest { private static final String ACCOUNTS_TABLE_NAME = "accounts_test"; - private static final String NUMBERS_TABLE_NAME = "numbers_test"; + private static final String NUMBER_CONSTRAINT_TABLE_NAME = "numbers_test"; + private static final String PNI_CONSTRAINT_TABLE_NAME = "pni_test"; private static final int SCAN_PAGE_SIZE = 1; @@ -74,7 +77,7 @@ class AccountsTest { @BeforeEach void setupAccountsDao() { CreateTableRequest createNumbersTableRequest = CreateTableRequest.builder() - .tableName(NUMBERS_TABLE_NAME) + .tableName(NUMBER_CONSTRAINT_TABLE_NAME) .keySchema(KeySchemaElement.builder() .attributeName(Accounts.ATTR_ACCOUNT_E164) .keyType(KeyType.HASH) @@ -88,27 +91,48 @@ class AccountsTest { dynamoDbExtension.getDynamoDbClient().createTable(createNumbersTableRequest); + CreateTableRequest createPhoneNumberIdentifierTableRequest = CreateTableRequest.builder() + .tableName(PNI_CONSTRAINT_TABLE_NAME) + .keySchema(KeySchemaElement.builder() + .attributeName(Accounts.ATTR_PNI_UUID) + .keyType(KeyType.HASH) + .build()) + .attributeDefinitions(AttributeDefinition.builder() + .attributeName(Accounts.ATTR_PNI_UUID) + .attributeType(ScalarAttributeType.B) + .build()) + .provisionedThroughput(DynamoDbExtension.DEFAULT_PROVISIONED_THROUGHPUT) + .build(); + + dynamoDbExtension.getDynamoDbClient().createTable(createPhoneNumberIdentifierTableRequest); + this.accounts = new Accounts( dynamoDbExtension.getDynamoDbClient(), dynamoDbExtension.getTableName(), - NUMBERS_TABLE_NAME, + NUMBER_CONSTRAINT_TABLE_NAME, + PNI_CONSTRAINT_TABLE_NAME, SCAN_PAGE_SIZE); } @Test void testStore() { - Device device = generateDevice (1 ); - Account account = generateAccount("+14151112222", UUID.randomUUID(), Collections.singleton(device)); + Device device = generateDevice(1); + Account account = generateAccount("+14151112222", UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(device)); boolean freshUser = accounts.create(account); assertThat(freshUser).isTrue(); - verifyStoredState("+14151112222", account.getUuid(), account, true); + verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), account, true); + + assertPhoneNumberConstraintExists("+14151112222", account.getUuid()); + assertPhoneNumberIdentifierConstraintExists(account.getPhoneNumberIdentifier().orElseThrow(), account.getUuid()); freshUser = accounts.create(account); assertThat(freshUser).isTrue(); - verifyStoredState("+14151112222", account.getUuid(), account, true); + verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), account, true); + assertPhoneNumberConstraintExists("+14151112222", account.getUuid()); + assertPhoneNumberIdentifierConstraintExists(account.getPhoneNumberIdentifier().orElseThrow(), account.getUuid()); } @Test @@ -117,11 +141,14 @@ class AccountsTest { devices.add(generateDevice(1)); devices.add(generateDevice(2)); - Account account = generateAccount("+14151112222", UUID.randomUUID(), devices); + Account account = generateAccount("+14151112222", UUID.randomUUID(), UUID.randomUUID(), devices); accounts.create(account); - verifyStoredState("+14151112222", account.getUuid(), account, true); + verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), account, true); + + assertPhoneNumberConstraintExists("+14151112222", account.getUuid()); + assertPhoneNumberIdentifierConstraintExists(account.getPhoneNumberIdentifier().orElseThrow(), account.getUuid()); } @Test @@ -131,46 +158,123 @@ class AccountsTest { devicesFirst.add(generateDevice(2)); UUID uuidFirst = UUID.randomUUID(); - Account accountFirst = generateAccount("+14151112222", uuidFirst, devicesFirst); + UUID pniFirst = UUID.randomUUID(); + Account accountFirst = generateAccount("+14151112222", uuidFirst, pniFirst, devicesFirst); Set devicesSecond = new HashSet<>(); devicesSecond.add(generateDevice(1)); devicesSecond.add(generateDevice(2)); UUID uuidSecond = UUID.randomUUID(); - Account accountSecond = generateAccount("+14152221111", uuidSecond, devicesSecond); + UUID pniSecond = UUID.randomUUID(); + Account accountSecond = generateAccount("+14152221111", uuidSecond, pniSecond, devicesSecond); accounts.create(accountFirst); accounts.create(accountSecond); - Optional retrievedFirst = accounts.get("+14151112222"); - Optional retrievedSecond = accounts.get("+14152221111"); + Optional retrievedFirst = accounts.getByE164("+14151112222"); + Optional retrievedSecond = accounts.getByE164("+14152221111"); assertThat(retrievedFirst.isPresent()).isTrue(); assertThat(retrievedSecond.isPresent()).isTrue(); - verifyStoredState("+14151112222", uuidFirst, retrievedFirst.get(), accountFirst); - verifyStoredState("+14152221111", uuidSecond, retrievedSecond.get(), accountSecond); + verifyStoredState("+14151112222", uuidFirst, pniFirst, retrievedFirst.get(), accountFirst); + verifyStoredState("+14152221111", uuidSecond, pniSecond, retrievedSecond.get(), accountSecond); - retrievedFirst = accounts.get(uuidFirst); - retrievedSecond = accounts.get(uuidSecond); + retrievedFirst = accounts.getByAccountIdentifier(uuidFirst); + retrievedSecond = accounts.getByAccountIdentifier(uuidSecond); assertThat(retrievedFirst.isPresent()).isTrue(); assertThat(retrievedSecond.isPresent()).isTrue(); - verifyStoredState("+14151112222", uuidFirst, retrievedFirst.get(), accountFirst); - verifyStoredState("+14152221111", uuidSecond, retrievedSecond.get(), accountSecond); + verifyStoredState("+14151112222", uuidFirst, pniFirst, retrievedFirst.get(), accountFirst); + verifyStoredState("+14152221111", uuidSecond, pniSecond, retrievedSecond.get(), accountSecond); + + retrievedFirst = accounts.getByPhoneNumberIdentifier(pniFirst); + retrievedSecond = accounts.getByPhoneNumberIdentifier(pniSecond); + + assertThat(retrievedFirst.isPresent()).isTrue(); + assertThat(retrievedSecond.isPresent()).isTrue(); + + verifyStoredState("+14151112222", uuidFirst, pniFirst, retrievedFirst.get(), accountFirst); + verifyStoredState("+14152221111", uuidSecond, pniSecond, retrievedSecond.get(), accountSecond); + } + + @Test + void testRetrieveNoPni() throws JsonProcessingException { + final Set devices = new HashSet<>(); + devices.add(generateDevice(1)); + devices.add(generateDevice(2)); + + final UUID uuid = UUID.randomUUID(); + + final Account account = generateAccount("+14151112222", uuid, null, devices); + + // Accounts#create enforces that newly-created accounts have a PNI, so we need to make a bit of an end-run around it + // to simulate an existing account with no PNI. + { + final TransactWriteItem phoneNumberConstraintPut = TransactWriteItem.builder() + .put( + Put.builder() + .tableName(NUMBER_CONSTRAINT_TABLE_NAME) + .item(Map.of( + Accounts.ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()), + Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) + .conditionExpression( + "attribute_not_exists(#number) OR (attribute_exists(#number) AND #uuid = :uuid)") + .expressionAttributeNames( + Map.of("#uuid", Accounts.KEY_ACCOUNT_UUID, + "#number", Accounts.ATTR_ACCOUNT_E164)) + .expressionAttributeValues( + Map.of(":uuid", AttributeValues.fromUUID(account.getUuid()))) + .returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD) + .build()) + .build(); + + final TransactWriteItem accountPut = TransactWriteItem.builder() + .put(Put.builder() + .tableName(ACCOUNTS_TABLE_NAME) + .conditionExpression("attribute_not_exists(#number) OR #number = :number") + .expressionAttributeNames(Map.of("#number", Accounts.ATTR_ACCOUNT_E164)) + .expressionAttributeValues(Map.of(":number", AttributeValues.fromString(account.getNumber()))) + .item(Map.of( + Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(uuid), + Accounts.ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()), + Accounts.ATTR_ACCOUNT_DATA, AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)), + Accounts.ATTR_VERSION, AttributeValues.fromInt(account.getVersion()), + Accounts.ATTR_CANONICALLY_DISCOVERABLE, AttributeValues.fromBool(account.shouldBeVisibleInDirectory()))) + .build()) + .build(); + + dynamoDbExtension.getDynamoDbClient().transactWriteItems(TransactWriteItemsRequest.builder() + .transactItems(phoneNumberConstraintPut, accountPut) + .build()); + } + + Optional retrieved = accounts.getByE164("+14151112222"); + + assertThat(retrieved.isPresent()).isTrue(); + verifyStoredState("+14151112222", uuid, null, retrieved.get(), account); + + retrieved = accounts.getByAccountIdentifier(uuid); + + assertThat(retrieved.isPresent()).isTrue(); + verifyStoredState("+14151112222", uuid, null, retrieved.get(), account); } @Test void testOverwrite() { - Device device = generateDevice (1 ); - UUID firstUuid = UUID.randomUUID(); - Account account = generateAccount("+14151112222", firstUuid, Collections.singleton(device)); + Device device = generateDevice(1); + UUID firstUuid = UUID.randomUUID(); + UUID firstPni = UUID.randomUUID(); + Account account = generateAccount("+14151112222", firstUuid, firstPni, Collections.singleton(device)); accounts.create(account); - verifyStoredState("+14151112222", account.getUuid(), account, true); + verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), account, true); + + assertPhoneNumberConstraintExists("+14151112222", firstUuid); + assertPhoneNumberIdentifierConstraintExists(firstPni, firstUuid); account.setProfileName("name"); @@ -179,14 +283,17 @@ class AccountsTest { UUID secondUuid = UUID.randomUUID(); device = generateDevice(1); - account = generateAccount("+14151112222", secondUuid, Collections.singleton(device)); + account = generateAccount("+14151112222", secondUuid, UUID.randomUUID(), Collections.singleton(device)); final boolean freshUser = accounts.create(account); assertThat(freshUser).isFalse(); - verifyStoredState("+14151112222", firstUuid, account, true); + verifyStoredState("+14151112222", firstUuid, firstPni, account, true); + + assertPhoneNumberConstraintExists("+14151112222", firstUuid); + assertPhoneNumberIdentifierConstraintExists(firstPni, firstUuid); device = generateDevice(1); - Account invalidAccount = generateAccount("+14151113333", firstUuid, Collections.singleton(device)); + Account invalidAccount = generateAccount("+14151113333", firstUuid, UUID.randomUUID(), Collections.singleton(device)); assertThatThrownBy(() -> accounts.create(invalidAccount)); } @@ -194,28 +301,34 @@ class AccountsTest { @Test void testUpdate() { Device device = generateDevice (1 ); - Account account = generateAccount("+14151112222", UUID.randomUUID(), Collections.singleton(device)); + Account account = generateAccount("+14151112222", UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(device)); accounts.create(account); + assertPhoneNumberConstraintExists("+14151112222", account.getUuid()); + assertPhoneNumberIdentifierConstraintExists(account.getPhoneNumberIdentifier().orElseThrow(), account.getUuid()); + device.setName("foobar"); accounts.update(account); - Optional retrieved = accounts.get("+14151112222"); + assertPhoneNumberConstraintExists("+14151112222", account.getUuid()); + assertPhoneNumberIdentifierConstraintExists(account.getPhoneNumberIdentifier().orElseThrow(), account.getUuid()); + + Optional retrieved = accounts.getByE164("+14151112222"); assertThat(retrieved.isPresent()).isTrue(); - verifyStoredState("+14151112222", account.getUuid(), retrieved.get(), account); + verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), retrieved.get(), account); - retrieved = accounts.get(account.getUuid()); + retrieved = accounts.getByAccountIdentifier(account.getUuid()); assertThat(retrieved.isPresent()).isTrue(); - verifyStoredState("+14151112222", account.getUuid(), account, true); + verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), account, true); device = generateDevice(1); - Account unknownAccount = generateAccount("+14151113333", UUID.randomUUID(), Collections.singleton(device)); + Account unknownAccount = generateAccount("+14151113333", UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(device)); - assertThatThrownBy(() -> accounts.update(unknownAccount)).isInstanceOfAny(ConditionalCheckFailedException.class); + assertThatThrownBy(() -> accounts.update(unknownAccount)).isInstanceOfAny(TransactionCanceledException.class); account.setProfileName("name"); @@ -223,7 +336,7 @@ class AccountsTest { assertThat(account.getVersion()).isEqualTo(2); - verifyStoredState("+14151112222", account.getUuid(), account, true); + verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), account, true); account.setVersion(1); @@ -234,7 +347,7 @@ class AccountsTest { accounts.update(account); - verifyStoredState("+14151112222", account.getUuid(), account, true); + verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), account, true); } @Test @@ -242,13 +355,13 @@ class AccountsTest { final DynamoDbClient dynamoDbClient = mock(DynamoDbClient.class); accounts = new Accounts(dynamoDbClient, - dynamoDbExtension.getTableName(), NUMBERS_TABLE_NAME, SCAN_PAGE_SIZE); + dynamoDbExtension.getTableName(), NUMBER_CONSTRAINT_TABLE_NAME, PNI_CONSTRAINT_TABLE_NAME, SCAN_PAGE_SIZE); - when(dynamoDbClient.updateItem(any(UpdateItemRequest.class))) + when(dynamoDbClient.transactWriteItems(any(TransactWriteItemsRequest.class))) .thenThrow(TransactionConflictException.class); Device device = generateDevice(1); - Account account = generateAccount("+14151112222", UUID.randomUUID(), Collections.singleton(device)); + Account account = generateAccount("+14151112222", UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(device)); assertThatThrownBy(() -> accounts.update(account)).isInstanceOfAny(ContestedOptimisticLockException.class); } @@ -258,7 +371,7 @@ class AccountsTest { List users = new ArrayList<>(); for (int i = 1; i <= 100; i++) { - Account account = generateAccount("+1" + String.format("%03d", i), UUID.randomUUID()); + Account account = generateAccount("+1" + String.format("%03d", i), UUID.randomUUID(), UUID.randomUUID()); users.add(account); accounts.create(account); } @@ -276,7 +389,7 @@ class AccountsTest { .findAny() .orElseThrow(); - verifyStoredState(expectedAccount.getNumber(), expectedAccount.getUuid(), retrievedAccount, expectedAccount); + verifyStoredState(expectedAccount.getNumber(), expectedAccount.getUuid(), expectedAccount.getPhoneNumberIdentifier().orElseThrow(), retrievedAccount, expectedAccount); users.remove(expectedAccount); } @@ -293,7 +406,7 @@ class AccountsTest { .findAny() .orElseThrow(); - verifyStoredState(expectedAccount.getNumber(), expectedAccount.getUuid(), retrievedAccount, expectedAccount); + verifyStoredState(expectedAccount.getNumber(), expectedAccount.getUuid(), expectedAccount.getPhoneNumberIdentifier().orElseThrow(), retrievedAccount, expectedAccount); users.remove(expectedAccount); } @@ -306,48 +419,59 @@ class AccountsTest { void testDelete() { final Device deletedDevice = generateDevice(1); final Account deletedAccount = generateAccount("+14151112222", UUID.randomUUID(), - Collections.singleton(deletedDevice)); + UUID.randomUUID(), Collections.singleton(deletedDevice)); final Device retainedDevice = generateDevice(1); final Account retainedAccount = generateAccount("+14151112345", UUID.randomUUID(), - Collections.singleton(retainedDevice)); + UUID.randomUUID(), Collections.singleton(retainedDevice)); accounts.create(deletedAccount); accounts.create(retainedAccount); - assertThat(accounts.get(deletedAccount.getUuid())).isPresent(); - assertThat(accounts.get(retainedAccount.getUuid())).isPresent(); + assertPhoneNumberConstraintExists("+14151112222", deletedAccount.getUuid()); + assertPhoneNumberIdentifierConstraintExists(deletedAccount.getPhoneNumberIdentifier().orElseThrow(), deletedAccount.getUuid()); + assertPhoneNumberConstraintExists("+14151112345", retainedAccount.getUuid()); + assertPhoneNumberIdentifierConstraintExists(retainedAccount.getPhoneNumberIdentifier().orElseThrow(), retainedAccount.getUuid()); + + assertThat(accounts.getByAccountIdentifier(deletedAccount.getUuid())).isPresent(); + assertThat(accounts.getByAccountIdentifier(retainedAccount.getUuid())).isPresent(); accounts.delete(deletedAccount.getUuid()); - assertThat(accounts.get(deletedAccount.getUuid())).isNotPresent(); + assertThat(accounts.getByAccountIdentifier(deletedAccount.getUuid())).isNotPresent(); - verifyStoredState(retainedAccount.getNumber(), retainedAccount.getUuid(), - accounts.get(retainedAccount.getUuid()).get(), retainedAccount); + assertPhoneNumberConstraintDoesNotExist(deletedAccount.getNumber()); + assertPhoneNumberIdentifierConstraintDoesNotExist(deletedAccount.getPhoneNumberIdentifier().orElseThrow()); + + verifyStoredState(retainedAccount.getNumber(), retainedAccount.getUuid(), retainedAccount.getPhoneNumberIdentifier().orElseThrow(), + accounts.getByAccountIdentifier(retainedAccount.getUuid()).get(), retainedAccount); { final Account recreatedAccount = generateAccount(deletedAccount.getNumber(), UUID.randomUUID(), - Collections.singleton(generateDevice(1))); + UUID.randomUUID(), Collections.singleton(generateDevice(1))); final boolean freshUser = accounts.create(recreatedAccount); assertThat(freshUser).isTrue(); - assertThat(accounts.get(recreatedAccount.getUuid())).isPresent(); - verifyStoredState(recreatedAccount.getNumber(), recreatedAccount.getUuid(), - accounts.get(recreatedAccount.getUuid()).get(), recreatedAccount); + assertThat(accounts.getByAccountIdentifier(recreatedAccount.getUuid())).isPresent(); + verifyStoredState(recreatedAccount.getNumber(), recreatedAccount.getUuid(), recreatedAccount.getPhoneNumberIdentifier().orElseThrow(), + accounts.getByAccountIdentifier(recreatedAccount.getUuid()).get(), recreatedAccount); + + assertPhoneNumberConstraintExists(recreatedAccount.getNumber(), recreatedAccount.getUuid()); + assertPhoneNumberIdentifierConstraintExists(recreatedAccount.getPhoneNumberIdentifier().orElseThrow(), recreatedAccount.getUuid()); } } @Test void testMissing() { Device device = generateDevice (1 ); - Account account = generateAccount("+14151112222", UUID.randomUUID(), Collections.singleton(device)); + Account account = generateAccount("+14151112222", UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(device)); accounts.create(account); - Optional retrieved = accounts.get("+11111111"); + Optional retrieved = accounts.getByE164("+11111111"); assertThat(retrieved.isPresent()).isFalse(); - retrieved = accounts.get(UUID.randomUUID()); + retrieved = accounts.getByAccountIdentifier(UUID.randomUUID()); assertThat(retrieved.isPresent()).isFalse(); } @@ -369,8 +493,9 @@ class AccountsTest { when(client.updateItem(any(UpdateItemRequest.class))) .thenThrow(RuntimeException.class); - Accounts accounts = new Accounts(client, ACCOUNTS_TABLE_NAME, NUMBERS_TABLE_NAME, SCAN_PAGE_SIZE); - Account account = generateAccount("+14151112222", UUID.randomUUID()); + Accounts accounts = new Accounts(client, ACCOUNTS_TABLE_NAME, NUMBER_CONSTRAINT_TABLE_NAME, + PNI_CONSTRAINT_TABLE_NAME, SCAN_PAGE_SIZE); + Account account = generateAccount("+14151112222", UUID.randomUUID(), UUID.randomUUID()); try { accounts.update(account); @@ -406,17 +531,16 @@ class AccountsTest { @Test void testCanonicallyDiscoverableSet() { Device device = generateDevice(1); - UUID uuid = UUID.randomUUID(); - Account account = generateAccount("+14151112222", uuid, Collections.singleton(device)); + Account account = generateAccount("+14151112222", UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(device)); account.setDiscoverableByPhoneNumber(false); accounts.create(account); - verifyStoredState("+14151112222", account.getUuid(), account, false); + verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), account, false); account.setDiscoverableByPhoneNumber(true); accounts.update(account); - verifyStoredState("+14151112222", account.getUuid(), account, true); + verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), account, true); account.setDiscoverableByPhoneNumber(false); accounts.update(account); - verifyStoredState("+14151112222", account.getUuid(), account, false); + verifyStoredState("+14151112222", account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), account, false); } @Test @@ -424,27 +548,45 @@ class AccountsTest { final String originalNumber = "+14151112222"; final String targetNumber = "+14151113333"; + final UUID originalPni = UUID.randomUUID(); + final UUID targetPni = UUID.randomUUID(); + final Device device = generateDevice(1); - final Account account = generateAccount(originalNumber, UUID.randomUUID(), Collections.singleton(device)); + final Account account = generateAccount(originalNumber, UUID.randomUUID(), originalPni, Collections.singleton(device)); accounts.create(account); + assertThat(accounts.getByPhoneNumberIdentifier(originalPni)).isPresent(); + + assertPhoneNumberConstraintExists(originalNumber, account.getUuid()); + assertPhoneNumberIdentifierConstraintExists(originalPni, account.getUuid()); + { - final Optional retrieved = accounts.get(originalNumber); + final Optional retrieved = accounts.getByE164(originalNumber); assertThat(retrieved).isPresent(); - verifyStoredState(originalNumber, account.getUuid(), retrieved.get(), account); + verifyStoredState(originalNumber, account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), retrieved.get(), account); } - accounts.changeNumber(account, targetNumber); + accounts.changeNumber(account, targetNumber, targetPni); - assertThat(accounts.get(originalNumber)).isEmpty(); + assertThat(accounts.getByE164(originalNumber)).isEmpty(); + assertThat(accounts.getByAccountIdentifier(originalPni)).isEmpty(); + + assertPhoneNumberConstraintDoesNotExist(originalNumber); + assertPhoneNumberIdentifierConstraintDoesNotExist(originalPni); + assertPhoneNumberConstraintExists(targetNumber, account.getUuid()); + assertPhoneNumberIdentifierConstraintExists(targetPni, account.getUuid()); { - final Optional retrieved = accounts.get(targetNumber); + final Optional retrieved = accounts.getByE164(targetNumber); assertThat(retrieved).isPresent(); - verifyStoredState(targetNumber, account.getUuid(), retrieved.get(), account); + verifyStoredState(targetNumber, account.getUuid(), account.getPhoneNumberIdentifier().orElseThrow(), retrieved.get(), account); + + assertThat(retrieved.get().getPhoneNumberIdentifier()).isPresent(); + assertThat(retrieved.get().getPhoneNumberIdentifier().get()).isEqualTo(targetPni); + assertThat(accounts.getByPhoneNumberIdentifier(targetPni)).isPresent(); } } @@ -453,16 +595,267 @@ class AccountsTest { final String originalNumber = "+14151112222"; final String targetNumber = "+14151113333"; + final UUID originalPni = UUID.randomUUID(); + final UUID targetPni = UUID.randomUUID(); + final Device existingDevice = generateDevice(1); - final Account existingAccount = generateAccount(targetNumber, UUID.randomUUID(), Collections.singleton(existingDevice)); + final Account existingAccount = generateAccount(targetNumber, UUID.randomUUID(), targetPni, Collections.singleton(existingDevice)); final Device device = generateDevice(1); - final Account account = generateAccount(originalNumber, UUID.randomUUID(), Collections.singleton(device)); + final Account account = generateAccount(originalNumber, UUID.randomUUID(), originalPni, Collections.singleton(device)); accounts.create(account); accounts.create(existingAccount); - assertThrows(TransactionCanceledException.class, () -> accounts.changeNumber(account, targetNumber)); + assertThrows(TransactionCanceledException.class, () -> accounts.changeNumber(account, targetNumber, targetPni)); + + assertPhoneNumberConstraintExists(originalNumber, account.getUuid()); + assertPhoneNumberIdentifierConstraintExists(originalPni, account.getUuid()); + assertPhoneNumberConstraintExists(targetNumber, existingAccount.getUuid()); + assertPhoneNumberIdentifierConstraintExists(targetPni, existingAccount.getUuid()); + } + + @Test + public void testChangeNumberPhoneNumberIdentifierConflict() { + final String originalNumber = "+14151112222"; + final String targetNumber = "+14151113333"; + + final Device device = generateDevice(1); + final Account account = generateAccount(originalNumber, UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(device)); + + accounts.create(account); + + final UUID existingAccountIdentifier = UUID.randomUUID(); + final UUID existingPhoneNumberIdentifier = UUID.randomUUID(); + + // Artificially inject a conflicting PNI entry + dynamoDbExtension.getDynamoDbClient().putItem(PutItemRequest.builder() + .tableName(PNI_CONSTRAINT_TABLE_NAME) + .item(Map.of( + Accounts.ATTR_PNI_UUID, AttributeValues.fromUUID(existingPhoneNumberIdentifier), + Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(existingAccountIdentifier))) + .conditionExpression( + "attribute_not_exists(#pni) OR (attribute_exists(#pni) AND #uuid = :uuid)") + .expressionAttributeNames( + Map.of("#uuid", Accounts.KEY_ACCOUNT_UUID, + "#pni", Accounts.ATTR_PNI_UUID)) + .expressionAttributeValues( + Map.of(":uuid", AttributeValues.fromUUID(existingAccountIdentifier))) + .build()); + + assertThrows(TransactionCanceledException.class, () -> accounts.changeNumber(account, targetNumber, existingPhoneNumberIdentifier)); + } + + @Test + // TODO Remove or adapt after initial PNI migration + void testReregistrationFromAccountWithoutPhoneNumberIdentifier() throws JsonProcessingException { + final String number = "+18005551234"; + final UUID originalUuid = UUID.randomUUID(); + + // Artificially inject Dynamo items for a legacy account without an assigned PNI + { + final Account account = generateAccount(number, originalUuid, null); + + final TransactWriteItem phoneNumberConstraintPut = TransactWriteItem.builder() + .put( + Put.builder() + .tableName(NUMBER_CONSTRAINT_TABLE_NAME) + .item(Map.of( + Accounts.ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()), + Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) + .conditionExpression( + "attribute_not_exists(#number) OR (attribute_exists(#number) AND #uuid = :uuid)") + .expressionAttributeNames( + Map.of("#uuid", Accounts.KEY_ACCOUNT_UUID, + "#number", Accounts.ATTR_ACCOUNT_E164)) + .expressionAttributeValues( + Map.of(":uuid", AttributeValues.fromUUID(account.getUuid()))) + .returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD) + .build()) + .build(); + + final Map item = new HashMap<>(Map.of( + Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()), + Accounts.ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()), + Accounts.ATTR_ACCOUNT_DATA, + AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)), + Accounts.ATTR_VERSION, AttributeValues.fromInt(account.getVersion()), + Accounts.ATTR_CANONICALLY_DISCOVERABLE, AttributeValues.fromBool(account.shouldBeVisibleInDirectory()))); + + final TransactWriteItem accountPut = TransactWriteItem.builder() + .put(Put.builder() + .conditionExpression("attribute_not_exists(#number) OR #number = :number") + .expressionAttributeNames(Map.of("#number", Accounts.ATTR_ACCOUNT_E164)) + .expressionAttributeValues(Map.of(":number", AttributeValues.fromString(account.getNumber()))) + .tableName(ACCOUNTS_TABLE_NAME) + .item(item) + .build()) + .build(); + + dynamoDbExtension.getDynamoDbClient().transactWriteItems(TransactWriteItemsRequest.builder() + .transactItems(phoneNumberConstraintPut, accountPut) + .build()); + } + + final Account reregisteredAccount = generateAccount(number, UUID.randomUUID(), UUID.randomUUID()); + accounts.create(reregisteredAccount); + + assertPhoneNumberConstraintExists(number, originalUuid); + assertPhoneNumberIdentifierConstraintExists(reregisteredAccount.getPhoneNumberIdentifier().orElseThrow(), originalUuid); + } + + @Test + // TODO Remove or adapt after initial PNI migration + void testUpdateAccountAddingPniWithoutPhoneNumberIdentifier() throws JsonProcessingException { + final String number = "+18005551234"; + final UUID uuid = UUID.randomUUID(); + + // Artificially inject Dynamo items for a legacy account without an assigned PNI + { + final Account account = generateAccount(number, uuid, null); + + final TransactWriteItem phoneNumberConstraintPut = TransactWriteItem.builder() + .put( + Put.builder() + .tableName(NUMBER_CONSTRAINT_TABLE_NAME) + .item(Map.of( + Accounts.ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()), + Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) + .conditionExpression( + "attribute_not_exists(#number) OR (attribute_exists(#number) AND #uuid = :uuid)") + .expressionAttributeNames( + Map.of("#uuid", Accounts.KEY_ACCOUNT_UUID, + "#number", Accounts.ATTR_ACCOUNT_E164)) + .expressionAttributeValues( + Map.of(":uuid", AttributeValues.fromUUID(account.getUuid()))) + .returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD) + .build()) + .build(); + + final Map item = new HashMap<>(Map.of( + Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()), + Accounts.ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()), + Accounts.ATTR_ACCOUNT_DATA, + AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)), + Accounts.ATTR_VERSION, AttributeValues.fromInt(account.getVersion()), + Accounts.ATTR_CANONICALLY_DISCOVERABLE, AttributeValues.fromBool(account.shouldBeVisibleInDirectory()))); + + final TransactWriteItem accountPut = TransactWriteItem.builder() + .put(Put.builder() + .conditionExpression("attribute_not_exists(#number) OR #number = :number") + .expressionAttributeNames(Map.of("#number", Accounts.ATTR_ACCOUNT_E164)) + .expressionAttributeValues(Map.of(":number", AttributeValues.fromString(account.getNumber()))) + .tableName(ACCOUNTS_TABLE_NAME) + .item(item) + .build()) + .build(); + + dynamoDbExtension.getDynamoDbClient().transactWriteItems(TransactWriteItemsRequest.builder() + .transactItems(phoneNumberConstraintPut, accountPut) + .build()); + } + + assertThat(accounts.getByAccountIdentifier(uuid)).hasValueSatisfying(account -> { + assertThat(account.getUuid()).isEqualTo(uuid); + assertThat(account.getNumber()).isEqualTo(number); + assertThat(account.getPhoneNumberIdentifier()).isEmpty(); + }); + + final UUID phoneNumberIdentifier = UUID.randomUUID(); + + { + final Account accountToUpdate = accounts.getByAccountIdentifier(uuid).orElseThrow(); + accountToUpdate.setNumber(number, phoneNumberIdentifier); + + assertThat(accountToUpdate.getPhoneNumberIdentifier()).hasValueSatisfying(pni -> + assertThat(pni).isEqualTo(phoneNumberIdentifier)); + + accounts.update(accountToUpdate); + + assertThat(accountToUpdate.getPhoneNumberIdentifier()).hasValueSatisfying(pni -> + assertThat(pni).isEqualTo(phoneNumberIdentifier)); + } + + assertThat(accounts.getByAccountIdentifier(uuid)).hasValueSatisfying(account -> { + assertThat(account.getUuid()).isEqualTo(uuid); + assertThat(account.getNumber()).isEqualTo(number); + assertThat(account.getPhoneNumberIdentifier()).hasValueSatisfying(pni -> + assertThat(pni).isEqualTo(phoneNumberIdentifier)); + }); + } + + @Test + // TODO Remove or adapt after initial PNI migration + void testUpdateAccountWithoutPhoneNumberIdentifier() throws JsonProcessingException { + final String number = "+18005551234"; + final UUID uuid = UUID.randomUUID(); + + // Artificially inject Dynamo items for a legacy account without an assigned PNI + { + final Account account = generateAccount(number, uuid, null); + + final TransactWriteItem phoneNumberConstraintPut = TransactWriteItem.builder() + .put( + Put.builder() + .tableName(NUMBER_CONSTRAINT_TABLE_NAME) + .item(Map.of( + Accounts.ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()), + Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) + .conditionExpression( + "attribute_not_exists(#number) OR (attribute_exists(#number) AND #uuid = :uuid)") + .expressionAttributeNames( + Map.of("#uuid", Accounts.KEY_ACCOUNT_UUID, + "#number", Accounts.ATTR_ACCOUNT_E164)) + .expressionAttributeValues( + Map.of(":uuid", AttributeValues.fromUUID(account.getUuid()))) + .returnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD) + .build()) + .build(); + + final Map item = new HashMap<>(Map.of( + Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()), + Accounts.ATTR_ACCOUNT_E164, AttributeValues.fromString(account.getNumber()), + Accounts.ATTR_ACCOUNT_DATA, + AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)), + Accounts.ATTR_VERSION, AttributeValues.fromInt(account.getVersion()), + Accounts.ATTR_CANONICALLY_DISCOVERABLE, AttributeValues.fromBool(account.shouldBeVisibleInDirectory()))); + + final TransactWriteItem accountPut = TransactWriteItem.builder() + .put(Put.builder() + .conditionExpression("attribute_not_exists(#number) OR #number = :number") + .expressionAttributeNames(Map.of("#number", Accounts.ATTR_ACCOUNT_E164)) + .expressionAttributeValues(Map.of(":number", AttributeValues.fromString(account.getNumber()))) + .tableName(ACCOUNTS_TABLE_NAME) + .item(item) + .build()) + .build(); + + dynamoDbExtension.getDynamoDbClient().transactWriteItems(TransactWriteItemsRequest.builder() + .transactItems(phoneNumberConstraintPut, accountPut) + .build()); + } + + assertThat(accounts.getByAccountIdentifier(uuid)).hasValueSatisfying(account -> { + assertThat(account.getUuid()).isEqualTo(uuid); + assertThat(account.getNumber()).isEqualTo(number); + assertThat(account.getPhoneNumberIdentifier()).isEmpty(); + }); + + final String updatedName = "An updated name!"; + + { + final Account accountToUpdate = accounts.getByAccountIdentifier(uuid).orElseThrow(); + accountToUpdate.setProfileName(updatedName); + + accounts.update(accountToUpdate); + } + + assertThat(accounts.getByAccountIdentifier(uuid)).hasValueSatisfying(account -> { + assertThat(account.getUuid()).isEqualTo(uuid); + assertThat(account.getNumber()).isEqualTo(number); + assertThat(account.getPhoneNumberIdentifier()).isEmpty(); + assertThat(account.getProfileName()).isEqualTo(updatedName); + }); } private Device generateDevice(long id) { @@ -473,20 +866,62 @@ class AccountsTest { random.nextBoolean(), random.nextBoolean(), random.nextBoolean())); } - private Account generateAccount(String number, UUID uuid) { + private Account generateAccount(String number, UUID uuid, final UUID pni) { Device device = generateDevice(1); - return generateAccount(number, uuid, Collections.singleton(device)); + return generateAccount(number, uuid, pni, Collections.singleton(device)); } - private Account generateAccount(String number, UUID uuid, Set devices) { + private Account generateAccount(String number, UUID uuid, final UUID pni, Set devices) { byte[] unidentifiedAccessKey = new byte[16]; Random random = new Random(System.currentTimeMillis()); Arrays.fill(unidentifiedAccessKey, (byte)random.nextInt(255)); - return new Account(number, uuid, devices, unidentifiedAccessKey); + return new Account(number, uuid, pni, devices, unidentifiedAccessKey); } - private void verifyStoredState(String number, UUID uuid, Account expecting, boolean canonicallyDiscoverable) { + private void assertPhoneNumberConstraintExists(final String number, final UUID uuid) { + final GetItemResponse numberConstraintResponse = dynamoDbExtension.getDynamoDbClient().getItem( + GetItemRequest.builder() + .tableName(NUMBER_CONSTRAINT_TABLE_NAME) + .key(Map.of(Accounts.ATTR_ACCOUNT_E164, AttributeValues.fromString(number))) + .build()); + + assertThat(numberConstraintResponse.hasItem()).isTrue(); + assertThat(AttributeValues.getUUID(numberConstraintResponse.item(), Accounts.KEY_ACCOUNT_UUID, null)).isEqualTo(uuid); + } + + private void assertPhoneNumberConstraintDoesNotExist(final String number) { + final GetItemResponse numberConstraintResponse = dynamoDbExtension.getDynamoDbClient().getItem( + GetItemRequest.builder() + .tableName(NUMBER_CONSTRAINT_TABLE_NAME) + .key(Map.of(Accounts.ATTR_ACCOUNT_E164, AttributeValues.fromString(number))) + .build()); + + assertThat(numberConstraintResponse.hasItem()).isFalse(); + } + + private void assertPhoneNumberIdentifierConstraintExists(final UUID phoneNumberIdentifier, final UUID uuid) { + final GetItemResponse pniConstraintResponse = dynamoDbExtension.getDynamoDbClient().getItem( + GetItemRequest.builder() + .tableName(PNI_CONSTRAINT_TABLE_NAME) + .key(Map.of(Accounts.ATTR_PNI_UUID, AttributeValues.fromUUID(phoneNumberIdentifier))) + .build()); + + assertThat(pniConstraintResponse.hasItem()).isTrue(); + assertThat(AttributeValues.getUUID(pniConstraintResponse.item(), Accounts.KEY_ACCOUNT_UUID, null)).isEqualTo(uuid); + } + + private void assertPhoneNumberIdentifierConstraintDoesNotExist(final UUID phoneNumberIdentifier) { + final GetItemResponse pniConstraintResponse = dynamoDbExtension.getDynamoDbClient().getItem( + GetItemRequest.builder() + .tableName(PNI_CONSTRAINT_TABLE_NAME) + .key(Map.of(Accounts.ATTR_PNI_UUID, AttributeValues.fromUUID(phoneNumberIdentifier))) + .build()); + + assertThat(pniConstraintResponse.hasItem()).isFalse(); + } + + private void verifyStoredState(String number, UUID uuid, UUID pni, Account expecting, boolean canonicallyDiscoverable) { final DynamoDbClient db = dynamoDbExtension.getDynamoDbClient(); final GetItemResponse get = db.getItem(GetItemRequest.builder() @@ -506,14 +941,15 @@ class AccountsTest { !canonicallyDiscoverable)).isEqualTo(canonicallyDiscoverable); Account result = Accounts.fromItem(get.item()); - verifyStoredState(number, uuid, result, expecting); + verifyStoredState(number, uuid, pni, result, expecting); } else { throw new AssertionError("No data"); } } - private void verifyStoredState(String number, UUID uuid, Account result, Account expecting) { + private void verifyStoredState(String number, UUID uuid, UUID pni, Account result, Account expecting) { assertThat(result.getNumber()).isEqualTo(number); + assertThat(result.getPhoneNumberIdentifier()).isEqualTo(Optional.ofNullable(pni)); assertThat(result.getLastSeen()).isEqualTo(expecting.getLastSeen()); assertThat(result.getUuid()).isEqualTo(uuid); assertThat(result.getVersion()).isEqualTo(expecting.getVersion()); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/ContactDiscoveryWriterTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/ContactDiscoveryWriterTest.java index a726e3b86..5fd24dfc0 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/ContactDiscoveryWriterTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/ContactDiscoveryWriterTest.java @@ -30,7 +30,7 @@ public class ContactDiscoveryWriterTest { when(acct.getUuid()).thenReturn(uuid); when(acct.isCanonicallyDiscoverable()).thenReturn(canonicallyDiscoverable); when(acct.shouldBeVisibleInDirectory()).thenReturn(shouldBeVisible); - when(mgr.get(uuid)).thenReturn(Optional.of(acct)); + when(mgr.getByAccountIdentifier(uuid)).thenReturn(Optional.of(acct)); ContactDiscoveryWriter writer = new ContactDiscoveryWriter(mgr); writer.onCrawlChunk(Optional.empty(), List.of(acct)); verify(mgr, times(updateCalled ? 1 : 0)).update(acct, ContactDiscoveryWriter.NOOP_UPDATER); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/MessagePersisterIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/MessagePersisterIntegrationTest.java index d343075ef..420db7e25 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/MessagePersisterIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/MessagePersisterIntegrationTest.java @@ -82,7 +82,7 @@ class MessagePersisterIntegrationTest { when(account.getNumber()).thenReturn("+18005551234"); when(account.getUuid()).thenReturn(accountUuid); - when(accountsManager.get(accountUuid)).thenReturn(Optional.of(account)); + when(accountsManager.getByAccountIdentifier(accountUuid)).thenReturn(Optional.of(account)); when(dynamicConfigurationManager.getConfiguration()).thenReturn(new DynamicConfiguration()); messagesCache.start(); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/MessagePersisterTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/MessagePersisterTest.java index 2884a2072..77721ee76 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/MessagePersisterTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/MessagePersisterTest.java @@ -63,7 +63,7 @@ public class MessagePersisterTest extends AbstractRedisClusterTest { final Account account = mock(Account.class); - when(accountsManager.get(DESTINATION_ACCOUNT_UUID)).thenReturn(Optional.of(account)); + when(accountsManager.getByAccountIdentifier(DESTINATION_ACCOUNT_UUID)).thenReturn(Optional.of(account)); when(account.getNumber()).thenReturn(DESTINATION_ACCOUNT_NUMBER); when(dynamicConfigurationManager.getConfiguration()).thenReturn(new DynamicConfiguration()); @@ -98,7 +98,7 @@ public class MessagePersisterTest extends AbstractRedisClusterTest { public void testPersistNextQueuesNoQueues() { messagePersister.persistNextQueues(Instant.now()); - verify(accountsManager, never()).get(any(UUID.class)); + verify(accountsManager, never()).getByAccountIdentifier(any(UUID.class)); } @Test @@ -147,7 +147,7 @@ public class MessagePersisterTest extends AbstractRedisClusterTest { final Account account = mock(Account.class); - when(accountsManager.get(accountUuid)).thenReturn(Optional.of(account)); + when(accountsManager.getByAccountIdentifier(accountUuid)).thenReturn(Optional.of(account)); when(account.getNumber()).thenReturn(accountNumber); insertMessages(accountUuid, deviceId, messagesPerQueue, now); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiersTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiersTest.java new file mode 100644 index 000000000..d7caca1b0 --- /dev/null +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/PhoneNumberIdentifiersTest.java @@ -0,0 +1,58 @@ +/* + * Copyright 2013-2021 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.whispersystems.textsecuregcm.storage; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; + +import java.util.UUID; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import software.amazon.awssdk.services.dynamodb.model.AttributeDefinition; +import software.amazon.awssdk.services.dynamodb.model.ScalarAttributeType; + +class PhoneNumberIdentifiersTest { + + private static final String PNI_TABLE_NAME = "pni_test"; + + @RegisterExtension + static DynamoDbExtension DYNAMO_DB_EXTENSION = DynamoDbExtension.builder() + .tableName(PNI_TABLE_NAME) + .hashKey(PhoneNumberIdentifiers.KEY_E164) + .attributeDefinition(AttributeDefinition.builder() + .attributeName(PhoneNumberIdentifiers.KEY_E164) + .attributeType(ScalarAttributeType.S) + .build()) + .build(); + + private PhoneNumberIdentifiers phoneNumberIdentifiers; + + @BeforeEach + void setUp() { + phoneNumberIdentifiers = new PhoneNumberIdentifiers(DYNAMO_DB_EXTENSION.getDynamoDbClient(), PNI_TABLE_NAME); + } + + @Test + void getPhoneNumberIdentifier() { + final String number = "+18005551234"; + final String differentNumber = "+18005556789"; + + final UUID firstPni = phoneNumberIdentifiers.getPhoneNumberIdentifier(number); + final UUID secondPni = phoneNumberIdentifiers.getPhoneNumberIdentifier(number); + + assertEquals(firstPni, secondPni); + assertNotEquals(firstPni, phoneNumberIdentifiers.getPhoneNumberIdentifier(differentNumber)); + } + + @Test + void generatePhoneNumberIdentifierIfNotExists() { + final String number = "+18005551234"; + + assertEquals(phoneNumberIdentifiers.generatePhoneNumberIdentifierIfNotExists(number), + phoneNumberIdentifiers.generatePhoneNumberIdentifierIfNotExists(number)); + } +} diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/RefreshingAccountAndDeviceSupplierTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/RefreshingAccountAndDeviceSupplierTest.java index 1ef30dcef..dacbf13c3 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/RefreshingAccountAndDeviceSupplierTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/RefreshingAccountAndDeviceSupplierTest.java @@ -34,7 +34,7 @@ class RefreshingAccountAndDeviceSupplierTest { when(initialDevice.getId()).thenReturn(deviceId); when(initialAccount.getDevice(deviceId)).thenReturn(Optional.of(initialDevice)); - when(accountsManager.get(any(UUID.class))).thenAnswer(answer -> { + when(accountsManager.getByAccountIdentifier(any(UUID.class))).thenAnswer(answer -> { final Account account = mock(Account.class); final Device device = mock(Device.class); 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 7f71bed3f..3fd37cb3b 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 @@ -224,14 +224,14 @@ class AccountControllerTest { when(pendingAccountsManager.getCodeForNumber(SENDER_HAS_STORAGE)).thenReturn(Optional.of(new StoredVerificationCode("666666", System.currentTimeMillis(), null, null))); when(pendingAccountsManager.getCodeForNumber(SENDER_TRANSFER)).thenReturn(Optional.of(new StoredVerificationCode("1234", System.currentTimeMillis(), null, null))); - when(accountsManager.get(eq(SENDER_PIN))).thenReturn(Optional.of(senderPinAccount)); - when(accountsManager.get(eq(SENDER_REG_LOCK))).thenReturn(Optional.of(senderRegLockAccount)); - when(accountsManager.get(eq(SENDER_OVER_PIN))).thenReturn(Optional.of(senderPinAccount)); - when(accountsManager.get(eq(SENDER))).thenReturn(Optional.empty()); - when(accountsManager.get(eq(SENDER_OLD))).thenReturn(Optional.empty()); - when(accountsManager.get(eq(SENDER_PREAUTH))).thenReturn(Optional.empty()); - when(accountsManager.get(eq(SENDER_HAS_STORAGE))).thenReturn(Optional.of(senderHasStorage)); - when(accountsManager.get(eq(SENDER_TRANSFER))).thenReturn(Optional.of(senderTransfer)); + when(accountsManager.getByE164(eq(SENDER_PIN))).thenReturn(Optional.of(senderPinAccount)); + when(accountsManager.getByE164(eq(SENDER_REG_LOCK))).thenReturn(Optional.of(senderRegLockAccount)); + when(accountsManager.getByE164(eq(SENDER_OVER_PIN))).thenReturn(Optional.of(senderPinAccount)); + when(accountsManager.getByE164(eq(SENDER))).thenReturn(Optional.empty()); + when(accountsManager.getByE164(eq(SENDER_OLD))).thenReturn(Optional.empty()); + when(accountsManager.getByE164(eq(SENDER_PREAUTH))).thenReturn(Optional.empty()); + when(accountsManager.getByE164(eq(SENDER_HAS_STORAGE))).thenReturn(Optional.of(senderHasStorage)); + when(accountsManager.getByE164(eq(SENDER_TRANSFER))).thenReturn(Optional.of(senderTransfer)); when(accountsManager.create(any(), any(), any(), any(), any())).thenAnswer((Answer) invocation -> { final Account account = mock(Account.class); @@ -1338,7 +1338,7 @@ class AccountControllerTest { when(existingAccount.getUuid()).thenReturn(UUID.randomUUID()); when(existingAccount.getRegistrationLock()).thenReturn(existingRegistrationLock); - when(accountsManager.get(number)).thenReturn(Optional.of(existingAccount)); + when(accountsManager.getByE164(number)).thenReturn(Optional.of(existingAccount)); final Response response = resources.getJerseyTest() @@ -1368,7 +1368,7 @@ class AccountControllerTest { when(existingAccount.getUuid()).thenReturn(UUID.randomUUID()); when(existingAccount.getRegistrationLock()).thenReturn(existingRegistrationLock); - when(accountsManager.get(number)).thenReturn(Optional.of(existingAccount)); + when(accountsManager.getByE164(number)).thenReturn(Optional.of(existingAccount)); final Response response = resources.getJerseyTest() @@ -1400,7 +1400,7 @@ class AccountControllerTest { when(existingAccount.getUuid()).thenReturn(UUID.randomUUID()); when(existingAccount.getRegistrationLock()).thenReturn(existingRegistrationLock); - when(accountsManager.get(number)).thenReturn(Optional.of(existingAccount)); + when(accountsManager.getByE164(number)).thenReturn(Optional.of(existingAccount)); final Response response = resources.getJerseyTest() @@ -1432,7 +1432,7 @@ class AccountControllerTest { when(existingAccount.getUuid()).thenReturn(UUID.randomUUID()); when(existingAccount.getRegistrationLock()).thenReturn(existingRegistrationLock); - when(accountsManager.get(number)).thenReturn(Optional.of(existingAccount)); + when(accountsManager.getByE164(number)).thenReturn(Optional.of(existingAccount)); final Response response = resources.getJerseyTest() diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/DeviceControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/DeviceControllerTest.java index 1008d1e17..e352646c2 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/DeviceControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/DeviceControllerTest.java @@ -7,7 +7,6 @@ package org.whispersystems.textsecuregcm.tests.controllers; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.mock; @@ -132,8 +131,8 @@ class DeviceControllerTest { when(pendingDevicesManager.getCodeForNumber(AuthHelper.VALID_NUMBER)).thenReturn( Optional.of(new StoredVerificationCode("5678901", System.currentTimeMillis(), null, null))); when(pendingDevicesManager.getCodeForNumber(AuthHelper.VALID_NUMBER_TWO)).thenReturn(Optional.empty()); - when(accountsManager.get(AuthHelper.VALID_NUMBER)).thenReturn(Optional.of(account)); - when(accountsManager.get(AuthHelper.VALID_NUMBER_TWO)).thenReturn(Optional.of(maxedAccount)); + when(accountsManager.getByE164(AuthHelper.VALID_NUMBER)).thenReturn(Optional.of(account)); + when(accountsManager.getByE164(AuthHelper.VALID_NUMBER_TWO)).thenReturn(Optional.of(maxedAccount)); AccountsHelper.setupMockUpdate(accountsManager); } @@ -156,7 +155,7 @@ class DeviceControllerTest { @Test void validDeviceRegisterTest() { - when(accountsManager.get(AuthHelper.VALID_UUID)).thenReturn(Optional.of(AuthHelper.VALID_ACCOUNT)); + when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID)).thenReturn(Optional.of(AuthHelper.VALID_ACCOUNT)); final Device existingDevice = mock(Device.class); when(existingDevice.getId()).thenReturn(Device.MASTER_ID); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/DonationControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/DonationControllerTest.java index d41c7bd91..bd136b37b 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/DonationControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/DonationControllerTest.java @@ -210,7 +210,7 @@ class DonationControllerTest { when(receiptCredentialPresentation.getReceiptExpirationTime()).thenReturn(receiptExpiration); when(redeemedReceiptsManager.put(same(receiptSerial), eq(receiptExpiration), eq(receiptLevel), eq(AuthHelper.VALID_UUID))).thenReturn( CompletableFuture.completedFuture(Boolean.TRUE)); - when(accountsManager.get(eq(AuthHelper.VALID_UUID))).thenReturn(Optional.of(AuthHelper.VALID_ACCOUNT)); + when(accountsManager.getByAccountIdentifier(eq(AuthHelper.VALID_UUID))).thenReturn(Optional.of(AuthHelper.VALID_ACCOUNT)); RedeemReceiptRequest request = new RedeemReceiptRequest(presentation, true, true); Response response = resources.getJerseyTest() diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/KeysControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/KeysControllerTest.java index 2e59784a8..ee90018c6 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/KeysControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/KeysControllerTest.java @@ -153,11 +153,11 @@ class KeysControllerTest { when(existsAccount.getNumber()).thenReturn(EXISTS_NUMBER); when(existsAccount.getUnidentifiedAccessKey()).thenReturn(Optional.of("1337".getBytes())); - when(accounts.get(EXISTS_NUMBER)).thenReturn(Optional.of(existsAccount)); - when(accounts.get(EXISTS_UUID)).thenReturn(Optional.of(existsAccount)); + when(accounts.getByE164(EXISTS_NUMBER)).thenReturn(Optional.of(existsAccount)); + when(accounts.getByAccountIdentifier(EXISTS_UUID)).thenReturn(Optional.of(existsAccount)); - when(accounts.get(NOT_EXISTS_NUMBER)).thenReturn(Optional.empty()); - when(accounts.get(NOT_EXISTS_UUID)).thenReturn(Optional.empty()); + when(accounts.getByE164(NOT_EXISTS_NUMBER)).thenReturn(Optional.empty()); + when(accounts.getByAccountIdentifier(NOT_EXISTS_UUID)).thenReturn(Optional.empty()); when(rateLimiters.getPreKeysLimiter()).thenReturn(rateLimiter); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/MessageControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/MessageControllerTest.java index f004fcf6b..a0b66eccd 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/MessageControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/MessageControllerTest.java @@ -160,13 +160,15 @@ class MessageControllerTest { false, false, false))); }}; - Account singleDeviceAccount = new Account(SINGLE_DEVICE_RECIPIENT, SINGLE_DEVICE_UUID, singleDeviceList, "1234".getBytes()); - Account multiDeviceAccount = new Account(MULTI_DEVICE_RECIPIENT, MULTI_DEVICE_UUID, multiDeviceList, "1234".getBytes()); - internationalAccount = new Account(INTERNATIONAL_RECIPIENT, INTERNATIONAL_UUID, singleDeviceList, "1234".getBytes()); + Account singleDeviceAccount = new Account(SINGLE_DEVICE_RECIPIENT, SINGLE_DEVICE_UUID, UUID.randomUUID(), + singleDeviceList, "1234".getBytes()); + Account multiDeviceAccount = new Account(MULTI_DEVICE_RECIPIENT, MULTI_DEVICE_UUID, UUID.randomUUID(), + multiDeviceList, "1234".getBytes()); + internationalAccount = new Account(INTERNATIONAL_RECIPIENT, INTERNATIONAL_UUID, UUID.randomUUID(), singleDeviceList, "1234".getBytes()); - when(accountsManager.get(eq(SINGLE_DEVICE_UUID))).thenReturn(Optional.of(singleDeviceAccount)); - when(accountsManager.get(eq(MULTI_DEVICE_UUID))).thenReturn(Optional.of(multiDeviceAccount)); - when(accountsManager.get(INTERNATIONAL_UUID)).thenReturn(Optional.of(internationalAccount)); + when(accountsManager.getByAccountIdentifier(eq(SINGLE_DEVICE_UUID))).thenReturn(Optional.of(singleDeviceAccount)); + when(accountsManager.getByAccountIdentifier(eq(MULTI_DEVICE_UUID))).thenReturn(Optional.of(multiDeviceAccount)); + when(accountsManager.getByAccountIdentifier(INTERNATIONAL_UUID)).thenReturn(Optional.of(internationalAccount)); when(rateLimiters.getMessagesLimiter()).thenReturn(rateLimiter); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/ProfileControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/ProfileControllerTest.java index 07f070bb9..9df319dc3 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/ProfileControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/ProfileControllerTest.java @@ -169,13 +169,13 @@ class ProfileControllerTest { when(capabilitiesAccount.isAnnouncementGroupSupported()).thenReturn(true); when(capabilitiesAccount.isChangeNumberSupported()).thenReturn(true); - when(accountsManager.get(AuthHelper.VALID_NUMBER_TWO)).thenReturn(Optional.of(profileAccount)); - when(accountsManager.get(AuthHelper.VALID_UUID_TWO)).thenReturn(Optional.of(profileAccount)); + when(accountsManager.getByE164(AuthHelper.VALID_NUMBER_TWO)).thenReturn(Optional.of(profileAccount)); + when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID_TWO)).thenReturn(Optional.of(profileAccount)); when(usernamesManager.get(AuthHelper.VALID_UUID_TWO)).thenReturn(Optional.of("n00bkiller")); when(usernamesManager.get("n00bkiller")).thenReturn(Optional.of(AuthHelper.VALID_UUID_TWO)); - when(accountsManager.get(AuthHelper.VALID_NUMBER)).thenReturn(Optional.of(capabilitiesAccount)); - when(accountsManager.get(AuthHelper.VALID_UUID)).thenReturn(Optional.of(capabilitiesAccount)); + when(accountsManager.getByE164(AuthHelper.VALID_NUMBER)).thenReturn(Optional.of(capabilitiesAccount)); + when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID)).thenReturn(Optional.of(capabilitiesAccount)); when(profilesManager.get(eq(AuthHelper.VALID_UUID), eq("someversion"))).thenReturn(Optional.empty()); when(profilesManager.get(eq(AuthHelper.VALID_UUID_TWO), eq("validversion"))).thenReturn(Optional.of(new VersionedProfile( @@ -208,7 +208,7 @@ class ProfileControllerTest { assertThat(profile.getBadges()).hasSize(1).element(0).has(new Condition<>( badge -> "Test Badge".equals(badge.getName()), "has badge with expected name")); - verify(accountsManager).get(AuthHelper.VALID_UUID_TWO); + verify(accountsManager).getByAccountIdentifier(AuthHelper.VALID_UUID_TWO); verify(usernamesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO)); verify(rateLimiter, times(1)).validate(AuthHelper.VALID_UUID); } @@ -229,7 +229,7 @@ class ProfileControllerTest { assertThat(profile.getBadges()).hasSize(1).element(0).has(new Condition<>( badge -> "Test Badge".equals(badge.getName()), "has badge with expected name")); - verify(accountsManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO)); + verify(accountsManager, times(1)).getByAccountIdentifier(eq(AuthHelper.VALID_UUID_TWO)); verify(usernamesManager, times(1)).get(eq("n00bkiller")); verify(usernameRateLimiter, times(1)).validate(eq(AuthHelper.VALID_UUID)); } @@ -591,7 +591,7 @@ class ProfileControllerTest { assertThat(profile.getBadges()).hasSize(1).element(0).has(new Condition<>( badge -> "Test Badge".equals(badge.getName()), "has badge with expected name")); - verify(accountsManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO)); + verify(accountsManager, times(1)).getByAccountIdentifier(eq(AuthHelper.VALID_UUID_TWO)); verify(usernamesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO)); verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq("validversion")); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/push/APNSenderTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/push/APNSenderTest.java index 7f0f11af8..7cf2faaba 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/push/APNSenderTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/push/APNSenderTest.java @@ -51,7 +51,7 @@ public class APNSenderTest { public void setup() { when(destinationAccount.getDevice(1)).thenReturn(Optional.of(destinationDevice)); when(destinationDevice.getApnId()).thenReturn(DESTINATION_APN_ID); - when(accountsManager.get(DESTINATION_UUID)).thenReturn(Optional.of(destinationAccount)); + when(accountsManager.getByAccountIdentifier(DESTINATION_UUID)).thenReturn(Optional.of(destinationAccount)); } @Test @@ -158,7 +158,7 @@ public class APNSenderTest { assertThat(apnResult.getStatus()).isEqualTo(ApnResult.Status.NO_SUCH_USER); verifyNoMoreInteractions(apnsClient); - verify(accountsManager, times(1)).get(eq(DESTINATION_UUID)); + verify(accountsManager, times(1)).getByAccountIdentifier(eq(DESTINATION_UUID)); verify(destinationAccount, times(1)).getDevice(1); verify(destinationDevice, times(1)).getApnId(); verify(destinationDevice, times(1)).getPushTimestamp(); @@ -261,7 +261,7 @@ public class APNSenderTest { assertThat(apnResult.getStatus()).isEqualTo(ApnResult.Status.NO_SUCH_USER); verifyNoMoreInteractions(apnsClient); - verify(accountsManager, times(1)).get(eq(DESTINATION_UUID)); + verify(accountsManager, times(1)).getByAccountIdentifier(eq(DESTINATION_UUID)); verify(destinationAccount, times(1)).getDevice(1); verify(destinationDevice, times(1)).getApnId(); verify(destinationDevice, times(1)).getPushTimestamp(); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/push/GCMSenderTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/push/GCMSenderTest.java index 3df15c015..718722bb9 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/push/GCMSenderTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/push/GCMSenderTest.java @@ -72,7 +72,7 @@ public class GCMSenderTest { AccountsHelper.setupMockUpdate(accountsManager); when(destinationAccount.getDevice(1)).thenReturn(Optional.of(destinationDevice)); - when(accountsManager.get(destinationUuid)).thenReturn(Optional.of(destinationAccount)); + when(accountsManager.getByAccountIdentifier(destinationUuid)).thenReturn(Optional.of(destinationAccount)); when(destinationDevice.getGcmId()).thenReturn(gcmId); when(invalidResult.isInvalidRegistrationId()).thenReturn(true); @@ -90,7 +90,7 @@ public class GCMSenderTest { gcmSender.sendMessage(message); verify(sender, times(1)).send(any(Message.class)); - verify(accountsManager, times(1)).get(eq(destinationUuid)); + verify(accountsManager, times(1)).getByAccountIdentifier(eq(destinationUuid)); verify(accountsManager, times(1)).updateDevice(eq(destinationAccount), eq(1L), any()); verify(destinationDevice, times(1)).setUninstalledFeedbackTimestamp(eq(Util.todayInMillis())); } @@ -110,7 +110,7 @@ public class GCMSenderTest { Device destinationDevice = mock(Device.class ); when(destinationAccount.getDevice(1)).thenReturn(Optional.of(destinationDevice)); - when(accountsManager.get(destinationUuid)).thenReturn(Optional.of(destinationAccount)); + when(accountsManager.getByAccountIdentifier(destinationUuid)).thenReturn(Optional.of(destinationAccount)); when(destinationDevice.getGcmId()).thenReturn(gcmId); AccountsHelper.setupMockUpdate(accountsManager); @@ -131,7 +131,7 @@ public class GCMSenderTest { gcmSender.sendMessage(message); verify(sender, times(1)).send(any(Message.class)); - verify(accountsManager, times(1)).get(eq(destinationUuid)); + verify(accountsManager, times(1)).getByAccountIdentifier(eq(destinationUuid)); verify(accountsManager, times(1)).updateDevice(eq(destinationAccount), eq(1L), any()); verify(destinationDevice, times(1)).setGcmId(eq(canonicalId)); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountTest.java index 1e15efd7a..a1ad5f6c9 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountTest.java @@ -158,26 +158,30 @@ class AccountTest { when(disabledMasterDevice.getId()).thenReturn(1L); when(disabledLinkedDevice.getId()).thenReturn(2L); - assertTrue( new Account("+14151234567", UUID.randomUUID(), Set.of(enabledMasterDevice), new byte[0]).isEnabled()); - assertTrue( new Account("+14151234567", UUID.randomUUID(), Set.of(enabledMasterDevice, enabledLinkedDevice), new byte[0]).isEnabled()); - assertTrue( new Account("+14151234567", UUID.randomUUID(), Set.of(enabledMasterDevice, disabledLinkedDevice), new byte[0]).isEnabled()); - assertFalse(new Account("+14151234567", UUID.randomUUID(), Set.of(disabledMasterDevice), new byte[0]).isEnabled()); - assertFalse(new Account("+14151234567", UUID.randomUUID(), Set.of(disabledMasterDevice, enabledLinkedDevice), new byte[0]).isEnabled()); - assertFalse(new Account("+14151234567", UUID.randomUUID(), Set.of(disabledMasterDevice, disabledLinkedDevice), new byte[0]).isEnabled()); + assertTrue( new Account("+14151234567", UUID.randomUUID(), UUID.randomUUID(), Set.of(enabledMasterDevice), new byte[0]).isEnabled()); + assertTrue( new Account("+14151234567", UUID.randomUUID(), UUID.randomUUID(), + Set.of(enabledMasterDevice, enabledLinkedDevice), new byte[0]).isEnabled()); + assertTrue( new Account("+14151234567", UUID.randomUUID(), UUID.randomUUID(), + Set.of(enabledMasterDevice, disabledLinkedDevice), new byte[0]).isEnabled()); + assertFalse(new Account("+14151234567", UUID.randomUUID(), UUID.randomUUID(), Set.of(disabledMasterDevice), new byte[0]).isEnabled()); + assertFalse(new Account("+14151234567", UUID.randomUUID(), UUID.randomUUID(), + Set.of(disabledMasterDevice, enabledLinkedDevice), new byte[0]).isEnabled()); + assertFalse(new Account("+14151234567", UUID.randomUUID(), UUID.randomUUID(), + Set.of(disabledMasterDevice, disabledLinkedDevice), new byte[0]).isEnabled()); } @Test void testCapabilities() { - Account uuidCapable = new Account("+14152222222", UUID.randomUUID(), new HashSet() {{ + Account uuidCapable = new Account("+14152222222", UUID.randomUUID(), UUID.randomUUID(), new HashSet() {{ add(gv2CapableDevice); }}, "1234".getBytes()); - Account uuidIncapable = new Account("+14152222222", UUID.randomUUID(), new HashSet() {{ + Account uuidIncapable = new Account("+14152222222", UUID.randomUUID(), UUID.randomUUID(), new HashSet() {{ add(gv2CapableDevice); add(gv2IncapableDevice); }}, "1234".getBytes()); - Account uuidCapableWithExpiredIncapable = new Account("+14152222222", UUID.randomUUID(), new HashSet() {{ + Account uuidCapableWithExpiredIncapable = new Account("+14152222222", UUID.randomUUID(), UUID.randomUUID(), new HashSet() {{ add(gv2CapableDevice); add(gv2IncapableExpiredDevice); }}, "1234".getBytes()); @@ -213,20 +217,20 @@ class AccountTest { { final Account transferableMasterAccount = - new Account("+14152222222", UUID.randomUUID(), Collections.singleton(transferCapableMasterDevice), "1234".getBytes()); + new Account("+14152222222", UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(transferCapableMasterDevice), "1234".getBytes()); assertTrue(transferableMasterAccount.isTransferSupported()); } { final Account nonTransferableMasterAccount = - new Account("+14152222222", UUID.randomUUID(), Collections.singleton(nonTransferCapableMasterDevice), "1234".getBytes()); + new Account("+14152222222", UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(nonTransferCapableMasterDevice), "1234".getBytes()); assertFalse(nonTransferableMasterAccount.isTransferSupported()); } { - final Account transferableLinkedAccount = new Account("+14152222222", UUID.randomUUID(), new HashSet<>() {{ + final Account transferableLinkedAccount = new Account("+14152222222", UUID.randomUUID(), UUID.randomUUID(), new HashSet<>() {{ add(nonTransferCapableMasterDevice); add(transferCapableLinkedDevice); }}, "1234".getBytes()); @@ -237,7 +241,7 @@ class AccountTest { @Test void testDiscoverableByPhoneNumber() { - final Account account = new Account("+14152222222", UUID.randomUUID(), Collections.singleton(recentMasterDevice), + final Account account = new Account("+14152222222", UUID.randomUUID(), UUID.randomUUID(), Collections.singleton(recentMasterDevice), "1234".getBytes()); assertTrue(account.isDiscoverableByPhoneNumber(), @@ -252,66 +256,70 @@ class AccountTest { @Test void isGroupsV2Supported() { - assertTrue(new Account("+18005551234", UUID.randomUUID(), Set.of(gv2CapableDevice), + assertTrue(new Account("+18005551234", UUID.randomUUID(), UUID.randomUUID(), Set.of(gv2CapableDevice), "1234".getBytes(StandardCharsets.UTF_8)).isGroupsV2Supported()); - assertTrue(new Account("+18005551234", UUID.randomUUID(), Set.of(gv2CapableDevice, gv2IncapableExpiredDevice), + assertTrue(new Account("+18005551234", UUID.randomUUID(), UUID.randomUUID(), + Set.of(gv2CapableDevice, gv2IncapableExpiredDevice), "1234".getBytes(StandardCharsets.UTF_8)).isGroupsV2Supported()); - assertFalse(new Account("+18005551234", UUID.randomUUID(), Set.of(gv2CapableDevice, gv2IncapableDevice), + assertFalse(new Account("+18005551234", UUID.randomUUID(), UUID.randomUUID(), + Set.of(gv2CapableDevice, gv2IncapableDevice), "1234".getBytes(StandardCharsets.UTF_8)).isGroupsV2Supported()); } @Test void isGv1MigrationSupported() { - assertTrue(new Account("+18005551234", UUID.randomUUID(), Set.of(gv1MigrationCapableDevice), + assertTrue(new Account("+18005551234", UUID.randomUUID(), UUID.randomUUID(), Set.of(gv1MigrationCapableDevice), "1234".getBytes(StandardCharsets.UTF_8)).isGv1MigrationSupported()); assertFalse( - new Account("+18005551234", UUID.randomUUID(), Set.of(gv1MigrationCapableDevice, gv1MigrationIncapableDevice), + new Account("+18005551234", UUID.randomUUID(), UUID.randomUUID(), + Set.of(gv1MigrationCapableDevice, gv1MigrationIncapableDevice), "1234".getBytes(StandardCharsets.UTF_8)).isGv1MigrationSupported()); assertTrue(new Account("+18005551234", UUID.randomUUID(), - Set.of(gv1MigrationCapableDevice, gv1MigrationIncapableExpiredDevice), "1234".getBytes(StandardCharsets.UTF_8)) + UUID.randomUUID(), Set.of(gv1MigrationCapableDevice, gv1MigrationIncapableExpiredDevice), "1234".getBytes(StandardCharsets.UTF_8)) .isGv1MigrationSupported()); } @Test void isSenderKeySupported() { - assertThat(new Account("+18005551234", UUID.randomUUID(), Set.of(senderKeyCapableDevice), + assertThat(new Account("+18005551234", UUID.randomUUID(), UUID.randomUUID(), Set.of(senderKeyCapableDevice), "1234".getBytes(StandardCharsets.UTF_8)).isSenderKeySupported()).isTrue(); - assertThat(new Account("+18005551234", UUID.randomUUID(), Set.of(senderKeyCapableDevice, senderKeyIncapableDevice), + assertThat(new Account("+18005551234", UUID.randomUUID(), UUID.randomUUID(), + Set.of(senderKeyCapableDevice, senderKeyIncapableDevice), "1234".getBytes(StandardCharsets.UTF_8)).isSenderKeySupported()).isFalse(); assertThat(new Account("+18005551234", UUID.randomUUID(), - Set.of(senderKeyCapableDevice, senderKeyIncapableExpiredDevice), + UUID.randomUUID(), Set.of(senderKeyCapableDevice, senderKeyIncapableExpiredDevice), "1234".getBytes(StandardCharsets.UTF_8)).isSenderKeySupported()).isTrue(); } @Test void isAnnouncementGroupSupported() { assertThat(new Account("+18005551234", UUID.randomUUID(), - Set.of(announcementGroupCapableDevice), + UUID.randomUUID(), Set.of(announcementGroupCapableDevice), "1234".getBytes(StandardCharsets.UTF_8)).isAnnouncementGroupSupported()).isTrue(); assertThat(new Account("+18005551234", UUID.randomUUID(), - Set.of(announcementGroupCapableDevice, announcementGroupIncapableDevice), + UUID.randomUUID(), Set.of(announcementGroupCapableDevice, announcementGroupIncapableDevice), "1234".getBytes(StandardCharsets.UTF_8)).isAnnouncementGroupSupported()).isFalse(); assertThat(new Account("+18005551234", UUID.randomUUID(), - Set.of(announcementGroupCapableDevice, announcementGroupIncapableExpiredDevice), + UUID.randomUUID(), Set.of(announcementGroupCapableDevice, announcementGroupIncapableExpiredDevice), "1234".getBytes(StandardCharsets.UTF_8)).isAnnouncementGroupSupported()).isTrue(); } @Test void isChangeNumberSupported() { assertThat(new Account("+18005551234", UUID.randomUUID(), - Set.of(changeNumberCapableDevice), + UUID.randomUUID(), Set.of(changeNumberCapableDevice), "1234".getBytes(StandardCharsets.UTF_8)).isChangeNumberSupported()).isTrue(); assertThat(new Account("+18005551234", UUID.randomUUID(), - Set.of(changeNumberCapableDevice, changeNumberIncapableDevice), + UUID.randomUUID(), Set.of(changeNumberCapableDevice, changeNumberIncapableDevice), "1234".getBytes(StandardCharsets.UTF_8)).isChangeNumberSupported()).isFalse(); assertThat(new Account("+18005551234", UUID.randomUUID(), - Set.of(changeNumberCapableDevice, changeNumberIncapableExpiredDevice), + UUID.randomUUID(), Set.of(changeNumberCapableDevice, changeNumberIncapableExpiredDevice), "1234".getBytes(StandardCharsets.UTF_8)).isChangeNumberSupported()).isTrue(); } @Test void stale() { - final Account account = new Account("+14151234567", UUID.randomUUID(), Collections.emptySet(), new byte[0]); + final Account account = new Account("+14151234567", UUID.randomUUID(), UUID.randomUUID(), Collections.emptySet(), new byte[0]); assertDoesNotThrow(account::getNumber); @@ -327,7 +335,7 @@ class AccountTest { final Set devices = new HashSet<>(); devices.add(createDevice(Device.MASTER_ID)); - final Account account = new Account("+14151234567", UUID.randomUUID(), devices, new byte[0]); + final Account account = new Account("+14151234567", UUID.randomUUID(), UUID.randomUUID(), devices, new byte[0]); assertThat(account.getNextDeviceId()).isEqualTo(2L); @@ -348,7 +356,7 @@ class AccountTest { @Test void addAndRemoveBadges() { - final Account account = new Account("+14151234567", UUID.randomUUID(), Set.of(createDevice(Device.MASTER_ID)), new byte[0]); + final Account account = new Account("+14151234567", UUID.randomUUID(), UUID.randomUUID(), Set.of(createDevice(Device.MASTER_ID)), new byte[0]); final Clock clock = mock(Clock.class); when(clock.instant()).thenReturn(Instant.ofEpochSecond(40)); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java index b981466fc..7a9bca8f3 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java @@ -27,7 +27,9 @@ import io.lettuce.core.RedisException; import io.lettuce.core.cluster.api.sync.RedisAdvancedClusterCommands; import java.time.Clock; import java.util.ArrayList; +import java.util.HashMap; import java.util.HashSet; +import java.util.Map; import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; @@ -56,6 +58,7 @@ import org.whispersystems.textsecuregcm.storage.Device; import org.whispersystems.textsecuregcm.storage.Device.DeviceCapabilities; import org.whispersystems.textsecuregcm.storage.KeysDynamoDb; import org.whispersystems.textsecuregcm.storage.MessagesManager; +import org.whispersystems.textsecuregcm.storage.PhoneNumberIdentifiers; import org.whispersystems.textsecuregcm.storage.ProfilesManager; import org.whispersystems.textsecuregcm.storage.StoredVerificationCodeManager; import org.whispersystems.textsecuregcm.storage.UsernamesManager; @@ -96,11 +99,12 @@ class AccountsManagerTest { doAnswer((Answer) invocation -> { final Account account = invocation.getArgument(0, Account.class); final String number = invocation.getArgument(1, String.class); + final UUID phoneNumberIdentifier = invocation.getArgument(2, UUID.class); - account.setNumber(number); + account.setNumber(number, phoneNumberIdentifier); return null; - }).when(accounts).changeNumber(any(), anyString()); + }).when(accounts).changeNumber(any(), anyString(), any()); doAnswer(invocation -> { //noinspection unchecked @@ -114,8 +118,17 @@ class AccountsManagerTest { final SecureBackupClient backupClient = mock(SecureBackupClient.class); when(backupClient.deleteBackups(any())).thenReturn(CompletableFuture.completedFuture(null)); + final PhoneNumberIdentifiers phoneNumberIdentifiers = mock(PhoneNumberIdentifiers.class); + final Map phoneNumberIdentifiersByE164 = new HashMap<>(); + + when(phoneNumberIdentifiers.getPhoneNumberIdentifier(anyString())).thenAnswer((Answer) invocation -> { + final String number = invocation.getArgument(0, String.class); + return phoneNumberIdentifiersByE164.computeIfAbsent(number, n -> UUID.randomUUID()); + }); + accountsManager = new AccountsManager( accounts, + phoneNumberIdentifiers, RedisClusterHelper.buildMockRedisCluster(commands), deletedAccountsManager, directoryQueue, @@ -135,13 +148,14 @@ class AccountsManagerTest { UUID uuid = UUID.randomUUID(); when(commands.get(eq("AccountMap::+14152222222"))).thenReturn(uuid.toString()); - when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\"}"); + when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\", \"pni\": \"de24dc73-fbd8-41be-a7d5-764c70d9da7e\"}"); - Optional account = accountsManager.get("+14152222222"); + Optional account = accountsManager.getByE164("+14152222222"); assertTrue(account.isPresent()); assertEquals(account.get().getNumber(), "+14152222222"); assertEquals(account.get().getProfileName(), "test"); + assertEquals(Optional.of(UUID.fromString("de24dc73-fbd8-41be-a7d5-764c70d9da7e")), account.get().getPhoneNumberIdentifier()); verify(commands, times(1)).get(eq("AccountMap::+14152222222")); verify(commands, times(1)).get(eq("Account3::" + uuid)); @@ -154,14 +168,15 @@ class AccountsManagerTest { void testGetAccountByUuidInCache() { UUID uuid = UUID.randomUUID(); - when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\"}"); + when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\", \"pni\": \"de24dc73-fbd8-41be-a7d5-764c70d9da7e\"}"); - Optional account = accountsManager.get(uuid); + Optional account = accountsManager.getByAccountIdentifier(uuid); assertTrue(account.isPresent()); assertEquals(account.get().getNumber(), "+14152222222"); assertEquals(account.get().getUuid(), uuid); assertEquals(account.get().getProfileName(), "test"); + assertEquals(Optional.of(UUID.fromString("de24dc73-fbd8-41be-a7d5-764c70d9da7e")), account.get().getPhoneNumberIdentifier()); verify(commands, times(1)).get(eq("Account3::" + uuid)); verifyNoMoreInteractions(commands); @@ -169,110 +184,189 @@ class AccountsManagerTest { verifyNoInteractions(accounts); } + @Test + void testGetByPniInCache() { + UUID uuid = UUID.randomUUID(); + UUID pni = UUID.randomUUID(); + + when(commands.get(eq("AccountMap::" + pni))).thenReturn(uuid.toString()); + when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\", \"pni\": \"de24dc73-fbd8-41be-a7d5-764c70d9da7e\"}"); + + Optional account = accountsManager.getByPhoneNumberIdentifier(pni); + + assertTrue(account.isPresent()); + assertEquals(account.get().getNumber(), "+14152222222"); + assertEquals(account.get().getProfileName(), "test"); + assertEquals(Optional.of(UUID.fromString("de24dc73-fbd8-41be-a7d5-764c70d9da7e")), account.get().getPhoneNumberIdentifier()); + + verify(commands).get(eq("AccountMap::" + pni)); + verify(commands).get(eq("Account3::" + uuid)); + verifyNoMoreInteractions(commands); + + verifyNoInteractions(accounts); + } @Test void testGetAccountByNumberNotInCache() { UUID uuid = UUID.randomUUID(); - Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); + UUID pni = UUID.randomUUID(); + Account account = new Account("+14152222222", uuid, pni, new HashSet<>(), new byte[16]); when(commands.get(eq("AccountMap::+14152222222"))).thenReturn(null); - when(accounts.get(eq("+14152222222"))).thenReturn(Optional.of(account)); + when(accounts.getByE164(eq("+14152222222"))).thenReturn(Optional.of(account)); - Optional retrieved = accountsManager.get("+14152222222"); + Optional retrieved = accountsManager.getByE164("+14152222222"); assertTrue(retrieved.isPresent()); assertSame(retrieved.get(), account); verify(commands, times(1)).get(eq("AccountMap::+14152222222")); verify(commands, times(1)).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); + verify(commands, times(1)).set(eq("AccountMap::" + pni), eq(uuid.toString())); verify(commands, times(1)).set(eq("Account3::" + uuid), anyString()); verifyNoMoreInteractions(commands); - verify(accounts, times(1)).get(eq("+14152222222")); + verify(accounts, times(1)).getByE164(eq("+14152222222")); verifyNoMoreInteractions(accounts); } @Test void testGetAccountByUuidNotInCache() { UUID uuid = UUID.randomUUID(); - Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); + UUID pni = UUID.randomUUID(); + Account account = new Account("+14152222222", uuid, pni, new HashSet<>(), new byte[16]); when(commands.get(eq("Account3::" + uuid))).thenReturn(null); - when(accounts.get(eq(uuid))).thenReturn(Optional.of(account)); + when(accounts.getByAccountIdentifier(eq(uuid))).thenReturn(Optional.of(account)); - Optional retrieved = accountsManager.get(uuid); + Optional retrieved = accountsManager.getByAccountIdentifier(uuid); assertTrue(retrieved.isPresent()); assertSame(retrieved.get(), account); verify(commands, times(1)).get(eq("Account3::" + uuid)); verify(commands, times(1)).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); + verify(commands, times(1)).set(eq("AccountMap::" + pni), eq(uuid.toString())); verify(commands, times(1)).set(eq("Account3::" + uuid), anyString()); verifyNoMoreInteractions(commands); - verify(accounts, times(1)).get(eq(uuid)); + verify(accounts, times(1)).getByAccountIdentifier(eq(uuid)); + verifyNoMoreInteractions(accounts); + } + + @Test + void testGetAccountByPniNotInCache() { + UUID uuid = UUID.randomUUID(); + UUID pni = UUID.randomUUID(); + + Account account = new Account("+14152222222", uuid, pni, new HashSet<>(), new byte[16]); + + when(commands.get(eq("AccountMap::" + pni))).thenReturn(null); + when(accounts.getByPhoneNumberIdentifier(pni)).thenReturn(Optional.of(account)); + + Optional retrieved = accountsManager.getByPhoneNumberIdentifier(pni); + + assertTrue(retrieved.isPresent()); + assertSame(retrieved.get(), account); + + verify(commands).get(eq("AccountMap::" + pni)); + verify(commands).set(eq("AccountMap::" + pni), eq(uuid.toString())); + verify(commands).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); + verify(commands).set(eq("Account3::" + uuid), anyString()); + verifyNoMoreInteractions(commands); + + verify(accounts).getByPhoneNumberIdentifier(pni); verifyNoMoreInteractions(accounts); } @Test void testGetAccountByNumberBrokenCache() { UUID uuid = UUID.randomUUID(); - Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); + UUID pni = UUID.randomUUID(); + Account account = new Account("+14152222222", uuid, pni, new HashSet<>(), new byte[16]); when(commands.get(eq("AccountMap::+14152222222"))).thenThrow(new RedisException("Connection lost!")); - when(accounts.get(eq("+14152222222"))).thenReturn(Optional.of(account)); + when(accounts.getByE164(eq("+14152222222"))).thenReturn(Optional.of(account)); - Optional retrieved = accountsManager.get("+14152222222"); + Optional retrieved = accountsManager.getByE164("+14152222222"); assertTrue(retrieved.isPresent()); assertSame(retrieved.get(), account); verify(commands, times(1)).get(eq("AccountMap::+14152222222")); verify(commands, times(1)).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); + verify(commands, times(1)).set(eq("AccountMap::" + pni), eq(uuid.toString())); verify(commands, times(1)).set(eq("Account3::" + uuid), anyString()); verifyNoMoreInteractions(commands); - verify(accounts, times(1)).get(eq("+14152222222")); + verify(accounts, times(1)).getByE164(eq("+14152222222")); verifyNoMoreInteractions(accounts); } @Test void testGetAccountByUuidBrokenCache() { UUID uuid = UUID.randomUUID(); - Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); + UUID pni = UUID.randomUUID(); + Account account = new Account("+14152222222", uuid, pni, new HashSet<>(), new byte[16]); when(commands.get(eq("Account3::" + uuid))).thenThrow(new RedisException("Connection lost!")); - when(accounts.get(eq(uuid))).thenReturn(Optional.of(account)); + when(accounts.getByAccountIdentifier(eq(uuid))).thenReturn(Optional.of(account)); - Optional retrieved = accountsManager.get(uuid); + Optional retrieved = accountsManager.getByAccountIdentifier(uuid); assertTrue(retrieved.isPresent()); assertSame(retrieved.get(), account); verify(commands, times(1)).get(eq("Account3::" + uuid)); verify(commands, times(1)).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); + verify(commands, times(1)).set(eq("AccountMap::" + pni), eq(uuid.toString())); verify(commands, times(1)).set(eq("Account3::" + uuid), anyString()); verifyNoMoreInteractions(commands); - verify(accounts, times(1)).get(eq(uuid)); + verify(accounts, times(1)).getByAccountIdentifier(eq(uuid)); + verifyNoMoreInteractions(accounts); + } + + @Test + void testGetAccountByPniBrokenCache() { + UUID uuid = UUID.randomUUID(); + UUID pni = UUID.randomUUID(); + + Account account = new Account("+14152222222", uuid, pni, new HashSet<>(), new byte[16]); + + when(commands.get(eq("AccountMap::" + pni))).thenThrow(new RedisException("OH NO")); + when(accounts.getByPhoneNumberIdentifier(pni)).thenReturn(Optional.of(account)); + + Optional retrieved = accountsManager.getByPhoneNumberIdentifier(pni); + + assertTrue(retrieved.isPresent()); + assertSame(retrieved.get(), account); + + verify(commands).get(eq("AccountMap::" + pni)); + verify(commands).set(eq("AccountMap::" + pni), eq(uuid.toString())); + verify(commands).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); + verify(commands).set(eq("Account3::" + uuid), anyString()); + verifyNoMoreInteractions(commands); + + verify(accounts).getByPhoneNumberIdentifier(pni); verifyNoMoreInteractions(accounts); } @Test void testUpdate_optimisticLockingFailure() { UUID uuid = UUID.randomUUID(); - Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); + Account account = new Account("+14152222222", uuid, UUID.randomUUID(), new HashSet<>(), new byte[16]); when(commands.get(eq("Account3::" + uuid))).thenReturn(null); - when(accounts.get(uuid)).thenReturn( - Optional.of(new Account("+14152222222", uuid, new HashSet<>(), new byte[16]))); + when(accounts.getByAccountIdentifier(uuid)).thenReturn( + Optional.of(new Account("+14152222222", uuid, UUID.randomUUID(), new HashSet<>(), new byte[16]))); doThrow(ContestedOptimisticLockException.class) .doAnswer(ACCOUNT_UPDATE_ANSWER) .when(accounts).update(any()); - when(accounts.get(uuid)).thenReturn( - Optional.of(new Account("+14152222222", uuid, new HashSet<>(), new byte[16]))); + when(accounts.getByAccountIdentifier(uuid)).thenReturn( + Optional.of(new Account("+14152222222", uuid, UUID.randomUUID(), new HashSet<>(), new byte[16]))); doThrow(ContestedOptimisticLockException.class) .doAnswer(ACCOUNT_UPDATE_ANSWER) .when(accounts).update(any()); @@ -282,7 +376,7 @@ class AccountsManagerTest { assertEquals(1, account.getVersion()); assertEquals("name", account.getProfileName()); - verify(accounts, times(1)).get(uuid); + verify(accounts, times(1)).getByAccountIdentifier(uuid); verify(accounts, times(2)).update(any()); verifyNoMoreInteractions(accounts); } @@ -290,10 +384,10 @@ class AccountsManagerTest { @Test void testUpdate_dynamoOptimisticLockingFailureDuringCreate() { UUID uuid = UUID.randomUUID(); - Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); + Account account = new Account("+14152222222", uuid, UUID.randomUUID(), new HashSet<>(), new byte[16]); when(commands.get(eq("Account3::" + uuid))).thenReturn(null); - when(accounts.get(uuid)).thenReturn(Optional.empty()) + when(accounts.getByAccountIdentifier(uuid)).thenReturn(Optional.empty()) .thenReturn(Optional.of(account)); when(accounts.create(any())).thenThrow(ContestedOptimisticLockException.class); @@ -307,10 +401,10 @@ class AccountsManagerTest { @Test void testUpdateDevice() { final UUID uuid = UUID.randomUUID(); - Account account = new Account("+14152222222", uuid, new HashSet<>(), new byte[16]); + Account account = new Account("+14152222222", uuid, UUID.randomUUID(), new HashSet<>(), new byte[16]); - when(accounts.get(uuid)).thenReturn( - Optional.of(new Account("+14152222222", uuid, new HashSet<>(), new byte[16]))); + when(accounts.getByAccountIdentifier(uuid)).thenReturn( + Optional.of(new Account("+14152222222", uuid, UUID.randomUUID(), new HashSet<>(), new byte[16]))); assertTrue(account.getDevices().isEmpty()); @@ -422,7 +516,7 @@ class AccountsManagerTest { @MethodSource void testUpdateDirectoryQueue(final boolean visibleBeforeUpdate, final boolean visibleAfterUpdate, final boolean expectRefresh) { - final Account account = new Account("+14152222222", UUID.randomUUID(), new HashSet<>(), new byte[16]); + final Account account = new Account("+14152222222", UUID.randomUUID(), UUID.randomUUID(), new HashSet<>(), new byte[16]); // this sets up the appropriate result for Account#shouldBeVisibleInDirectory final Device device = new Device(Device.MASTER_ID, "device", "token", "salt", null, null, null, true, 1, @@ -449,7 +543,7 @@ class AccountsManagerTest { @ParameterizedTest @MethodSource void testUpdateDeviceLastSeen(final boolean expectUpdate, final long initialLastSeen, final long updatedLastSeen) { - final Account account = new Account("+14152222222", UUID.randomUUID(), new HashSet<>(), new byte[16]); + final Account account = new Account("+14152222222", UUID.randomUUID(), UUID.randomUUID(), new HashSet<>(), new byte[16]); final Device device = new Device(Device.MASTER_ID, "device", "token", "salt", null, null, null, true, 1, new SignedPreKey(1, "key", "sig"), initialLastSeen, 0, "OWT", 0, new DeviceCapabilities()); @@ -479,7 +573,7 @@ class AccountsManagerTest { final String targetNumber = "+14153333333"; final UUID uuid = UUID.randomUUID(); - Account account = new Account(originalNumber, uuid, new HashSet<>(), new byte[16]); + Account account = new Account(originalNumber, uuid, UUID.randomUUID(), new HashSet<>(), new byte[16]); account = accountsManager.changeNumber(account, targetNumber); assertEquals(targetNumber, account.getNumber()); @@ -491,7 +585,7 @@ class AccountsManagerTest { void testChangePhoneNumberSameNumber() throws InterruptedException { final String number = "+14152222222"; - Account account = new Account(number, UUID.randomUUID(), new HashSet<>(), new byte[16]); + Account account = new Account(number, UUID.randomUUID(), UUID.randomUUID(), new HashSet<>(), new byte[16]); account = accountsManager.changeNumber(account, number); assertEquals(number, account.getNumber()); @@ -509,10 +603,10 @@ class AccountsManagerTest { final UUID existingAccountUuid = UUID.randomUUID(); final UUID uuid = UUID.randomUUID(); - final Account existingAccount = new Account(targetNumber, existingAccountUuid, new HashSet<>(), new byte[16]); - when(accounts.get(targetNumber)).thenReturn(Optional.of(existingAccount)); + final Account existingAccount = new Account(targetNumber, existingAccountUuid, UUID.randomUUID(), new HashSet<>(), new byte[16]); + when(accounts.getByE164(targetNumber)).thenReturn(Optional.of(existingAccount)); - Account account = new Account(originalNumber, uuid, new HashSet<>(), new byte[16]); + Account account = new Account(originalNumber, uuid, UUID.randomUUID(), new HashSet<>(), new byte[16]); account = accountsManager.changeNumber(account, targetNumber); assertEquals(targetNumber, account.getNumber()); @@ -527,8 +621,8 @@ class AccountsManagerTest { final String targetNumber = "+14153333333"; final UUID uuid = UUID.randomUUID(); - final Account account = new Account(originalNumber, uuid, new HashSet<>(), new byte[16]); + final Account account = new Account(originalNumber, uuid, UUID.randomUUID(), new HashSet<>(), new byte[16]); - assertThrows(AssertionError.class, () -> accountsManager.update(account, a -> a.setNumber(targetNumber))); + assertThrows(AssertionError.class, () -> accountsManager.update(account, a -> a.setNumber(targetNumber, UUID.randomUUID()))); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/AccountsHelper.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/AccountsHelper.java index 7b09b3ffe..b10950511 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/AccountsHelper.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/AccountsHelper.java @@ -62,7 +62,7 @@ public class AccountsHelper { } public static void setupMockGet(final AccountsManager mockAccountsManager, final Set mockAccounts) { - when(mockAccountsManager.get(any(UUID.class))).thenAnswer(answer -> { + when(mockAccountsManager.getByAccountIdentifier(any(UUID.class))).thenAnswer(answer -> { final UUID uuid = answer.getArgument(0, UUID.class); @@ -176,7 +176,7 @@ public class AccountsHelper { } else { final ObjectMapper mapper = SystemMapper.getMapper(); updatedAccount = mapper.readValue(mapper.writeValueAsBytes(account), Account.class); - updatedAccount.setNumber(account.getNumber()); + updatedAccount.setNumber(account.getNumber(), account.getPhoneNumberIdentifier().orElse(null)); account.markStale(); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/AuthHelper.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/AuthHelper.java index 7643ab460..3cdd4ccff 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/AuthHelper.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/AuthHelper.java @@ -137,17 +137,17 @@ public class AuthHelper { reset(ACCOUNTS_MANAGER); - when(ACCOUNTS_MANAGER.get(VALID_NUMBER)).thenReturn(Optional.of(VALID_ACCOUNT)); - when(ACCOUNTS_MANAGER.get(VALID_UUID)).thenReturn(Optional.of(VALID_ACCOUNT)); + when(ACCOUNTS_MANAGER.getByE164(VALID_NUMBER)).thenReturn(Optional.of(VALID_ACCOUNT)); + when(ACCOUNTS_MANAGER.getByAccountIdentifier(VALID_UUID)).thenReturn(Optional.of(VALID_ACCOUNT)); - when(ACCOUNTS_MANAGER.get(VALID_NUMBER_TWO)).thenReturn(Optional.of(VALID_ACCOUNT_TWO)); - when(ACCOUNTS_MANAGER.get(VALID_UUID_TWO)).thenReturn(Optional.of(VALID_ACCOUNT_TWO)); + when(ACCOUNTS_MANAGER.getByE164(VALID_NUMBER_TWO)).thenReturn(Optional.of(VALID_ACCOUNT_TWO)); + when(ACCOUNTS_MANAGER.getByAccountIdentifier(VALID_UUID_TWO)).thenReturn(Optional.of(VALID_ACCOUNT_TWO)); - when(ACCOUNTS_MANAGER.get(DISABLED_NUMBER)).thenReturn(Optional.of(DISABLED_ACCOUNT)); - when(ACCOUNTS_MANAGER.get(DISABLED_UUID)).thenReturn(Optional.of(DISABLED_ACCOUNT)); + when(ACCOUNTS_MANAGER.getByE164(DISABLED_NUMBER)).thenReturn(Optional.of(DISABLED_ACCOUNT)); + when(ACCOUNTS_MANAGER.getByAccountIdentifier(DISABLED_UUID)).thenReturn(Optional.of(DISABLED_ACCOUNT)); - when(ACCOUNTS_MANAGER.get(UNDISCOVERABLE_NUMBER)).thenReturn(Optional.of(UNDISCOVERABLE_ACCOUNT)); - when(ACCOUNTS_MANAGER.get(UNDISCOVERABLE_UUID)).thenReturn(Optional.of(UNDISCOVERABLE_ACCOUNT)); + when(ACCOUNTS_MANAGER.getByE164(UNDISCOVERABLE_NUMBER)).thenReturn(Optional.of(UNDISCOVERABLE_ACCOUNT)); + when(ACCOUNTS_MANAGER.getByAccountIdentifier(UNDISCOVERABLE_UUID)).thenReturn(Optional.of(UNDISCOVERABLE_ACCOUNT)); AccountsHelper.setupMockUpdateForAuthHelper(ACCOUNTS_MANAGER); @@ -220,8 +220,8 @@ public class AuthHelper { when(account.getUuid()).thenReturn(uuid); when(account.getRelay()).thenReturn(Optional.empty()); when(account.isEnabled()).thenReturn(true); - when(accountsManager.get(number)).thenReturn(Optional.of(account)); - when(accountsManager.get(uuid)).thenReturn(Optional.of(account)); + when(accountsManager.getByE164(number)).thenReturn(Optional.of(account)); + when(accountsManager.getByAccountIdentifier(uuid)).thenReturn(Optional.of(account)); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/websocket/WebSocketConnectionTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/websocket/WebSocketConnectionTest.java index b3294937a..739595748 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/websocket/WebSocketConnectionTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/websocket/WebSocketConnectionTest.java @@ -172,8 +172,8 @@ public class WebSocketConnectionTest { Account sender1 = mock(Account.class); when(sender1.getDevices()).thenReturn(sender1devices); - when(accountsManager.get("sender1")).thenReturn(Optional.of(sender1)); - when(accountsManager.get("sender2")).thenReturn(Optional.empty()); + when(accountsManager.getByE164("sender1")).thenReturn(Optional.of(sender1)); + when(accountsManager.getByE164("sender2")).thenReturn(Optional.empty()); String userAgent = "user-agent"; @@ -327,8 +327,8 @@ public class WebSocketConnectionTest { Account sender1 = mock(Account.class); when(sender1.getDevices()).thenReturn(sender1devices); - when(accountsManager.get("sender1")).thenReturn(Optional.of(sender1)); - when(accountsManager.get("sender2")).thenReturn(Optional.empty()); + when(accountsManager.getByE164("sender1")).thenReturn(Optional.of(sender1)); + when(accountsManager.getByE164("sender2")).thenReturn(Optional.empty()); String userAgent = "user-agent"; @@ -700,8 +700,8 @@ public class WebSocketConnectionTest { Account sender1 = mock(Account.class); when(sender1.getDevices()).thenReturn(sender1devices); - when(accountsManager.get("sender1")).thenReturn(Optional.of(sender1)); - when(accountsManager.get("sender2")).thenReturn(Optional.empty()); + when(accountsManager.getByE164("sender1")).thenReturn(Optional.of(sender1)); + when(accountsManager.getByE164("sender2")).thenReturn(Optional.empty()); String userAgent = "Signal-Desktop/1.2.3"; @@ -776,8 +776,8 @@ public class WebSocketConnectionTest { Account sender1 = mock(Account.class); when(sender1.getDevices()).thenReturn(sender1devices); - when(accountsManager.get("sender1")).thenReturn(Optional.of(sender1)); - when(accountsManager.get("sender2")).thenReturn(Optional.empty()); + when(accountsManager.getByE164("sender1")).thenReturn(Optional.of(sender1)); + when(accountsManager.getByE164("sender2")).thenReturn(Optional.empty()); String userAgent = "Signal-Android/4.68.3";