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 38d6cb473..931630299 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java @@ -11,7 +11,6 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectWriter; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Throwables; -import com.google.i18n.phonenumbers.PhoneNumberUtil; import io.micrometer.core.instrument.Metrics; import io.micrometer.core.instrument.Timer; import java.io.IOException; @@ -424,11 +423,14 @@ public class Accounts { final AttributeValue numberAttr = AttributeValues.fromString(number); final AttributeValue pniAttr = AttributeValues.fromUUID(phoneNumberIdentifier); - writeItems.add(buildDelete(phoneNumberConstraintTableName, ATTR_ACCOUNT_E164, originalNumber)); - writeItems.add(buildConstraintTablePut(phoneNumberConstraintTableName, uuidAttr, ATTR_ACCOUNT_E164, numberAttr)); - writeItems.add(buildDelete(phoneNumberIdentifierConstraintTableName, ATTR_PNI_UUID, originalPni)); - writeItems.add(buildConstraintTablePut(phoneNumberIdentifierConstraintTableName, uuidAttr, ATTR_PNI_UUID, pniAttr)); - writeItems.add(buildRemoveDeletedAccount(phoneNumberIdentifier)); + if (!originalNumber.equals(number)) { + writeItems.add(buildDelete(phoneNumberConstraintTableName, ATTR_ACCOUNT_E164, originalNumber)); + writeItems.add(buildConstraintTablePut(phoneNumberConstraintTableName, uuidAttr, ATTR_ACCOUNT_E164, numberAttr)); + writeItems.add(buildDelete(phoneNumberIdentifierConstraintTableName, ATTR_PNI_UUID, originalPni)); + writeItems.add(buildConstraintTablePut(phoneNumberIdentifierConstraintTableName, uuidAttr, ATTR_PNI_UUID, pniAttr)); + writeItems.add(buildRemoveDeletedAccount(phoneNumberIdentifier)); + } + maybeDisplacedAccountIdentifier.ifPresent(displacedAccountIdentifier -> writeItems.add(buildPutDeletedAccount(displacedAccountIdentifier, originalPni))); 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 1d64ceb4a..c189d07bb 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -663,15 +663,10 @@ public class AccountsManager extends RedisPubSubAdapter implemen final Map pniPqLastResortPreKeys, final Map pniRegistrationIds) throws InterruptedException, MismatchedDevicesException { - final UUID originalPhoneNumberIdentifier = account.getPhoneNumberIdentifier(); final UUID targetPhoneNumberIdentifier = phoneNumberIdentifiers.getPhoneNumberIdentifier(targetNumber).join(); - if (originalPhoneNumberIdentifier.equals(targetPhoneNumberIdentifier)) { - return account; - } - try { - return accountLockManager.withLock(Set.of(account.getPhoneNumberIdentifier(), targetPhoneNumberIdentifier), + return accountLockManager.withLock(new HashSet<>(List.of(account.getPhoneNumberIdentifier(), targetPhoneNumberIdentifier)), () -> changeNumber(account, targetNumber, targetPhoneNumberIdentifier, pniIdentityKey, pniSignedPreKeys, pniPqLastResortPreKeys, pniRegistrationIds), accountLockExecutor); } catch (final Exception e) { if (e instanceof MismatchedDevicesException mismatchedDevicesException) { @@ -699,24 +694,30 @@ public class AccountsManager extends RedisPubSubAdapter implemen redisDelete(account); - // There are three possible states for accounts associated with the target phone number: + // There are four possible states for accounts associated with the target phone number: // - // 1. An account exists with the target PNI; the caller has proved ownership of the number, so delete the + // 1. The authenticated account already has the given phone number. We don't want to delete the account, but do want + // to update keys. + // 2. An account exists with the target PNI; the caller has proved ownership of the number, so delete the // account with the target PNI. This will leave a "deleted account" record for the deleted account mapping // the UUID of the deleted account to the target PNI. We'll then overwrite that so it points to the // original PNI to facilitate switching back and forth between numbers. - // 2. No account with the target PNI exists, but one has recently been deleted. In that case, add a "deleted + // 3. No account with the target PNI exists, but one has recently been deleted. In that case, add a "deleted // account" record that maps the ACI of the recently-deleted account to the now-abandoned original PNI // of the account changing its number (which facilitates ACI consistency in cases that a party is switching // back and forth between numbers). - // 3. No account with the target PNI exists at all, in which case no additional action is needed. + // 4. No account with the target PNI exists at all, in which case no additional action is needed. final Optional recentlyDeletedAci = accounts.findRecentlyDeletedAccountIdentifier(targetPhoneNumberIdentifier); final Optional maybeExistingAccount = getByE164(targetNumber); final Optional maybeDisplacedUuid; if (maybeExistingAccount.isPresent()) { - delete(maybeExistingAccount.get()).join(); - maybeDisplacedUuid = maybeExistingAccount.map(Account::getUuid); + if (maybeExistingAccount.get().getIdentifier(IdentityType.ACI).equals(account.getIdentifier(IdentityType.ACI))) { + maybeDisplacedUuid = Optional.empty(); + } else { + delete(maybeExistingAccount.get()).join(); + maybeDisplacedUuid = maybeExistingAccount.map(Account::getUuid); + } } else { maybeDisplacedUuid = recentlyDeletedAci; } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManager.java index 7fb942c52..366a24e10 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManager.java @@ -10,7 +10,6 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.stream.Collectors; -import javax.annotation.Nullable; import org.apache.commons.lang3.ObjectUtils; import org.signal.libsignal.protocol.IdentityKey; import org.slf4j.Logger; @@ -57,20 +56,6 @@ public class ChangeNumberManager { throw new IllegalArgumentException("PNI identity key, signed pre-keys, device messages, and registration IDs must be all null or all non-null"); } - if (number.equals(account.getNumber())) { - // The client has gotten confused/desynchronized with us about their own phone number, most likely due to losing - // our OK response to an immediately preceding change-number request, and are sending a change they don't realize - // is a no-op change. - // - // We don't need to actually do a number-change operation in our DB, but we *do* need to accept their new key - // material and distribute the sync messages, to be sure all clients agree with us and each other about what their - // keys are. Pretend this change-number request was actually a PNI key distribution request. - if (pniIdentityKey == null) { - return account; - } - return updatePniKeys(account, pniIdentityKey, deviceSignedPreKeys, devicePqLastResortPreKeys, deviceMessages, pniRegistrationIds, senderUserAgent); - } - final Account updatedAccount = accountsManager.changeNumber( account, number, pniIdentityKey, deviceSignedPreKeys, devicePqLastResortPreKeys, pniRegistrationIds); @@ -81,23 +66,6 @@ public class ChangeNumberManager { return updatedAccount; } - public Account updatePniKeys(final Account account, - final IdentityKey pniIdentityKey, - final Map deviceSignedPreKeys, - @Nullable final Map devicePqLastResortPreKeys, - final List deviceMessages, - final Map pniRegistrationIds, - final String senderUserAgent) throws MismatchedDevicesException, MessageTooLargeException { - - // Don't try to be smart about ignoring unnecessary retries. If we make literally no change we will skip the ddb - // write anyway. Linked devices can handle some wasted extra key rotations. - final Account updatedAccount = accountsManager.updatePniKeys( - account, pniIdentityKey, deviceSignedPreKeys, devicePqLastResortPreKeys, pniRegistrationIds); - - sendDeviceMessages(updatedAccount, deviceMessages, senderUserAgent); - return updatedAccount; - } - private void sendDeviceMessages(final Account account, final List deviceMessages, final String senderUserAgent) throws MessageTooLargeException, MismatchedDevicesException { diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java index 7e664cb4e..c20048178 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java @@ -1019,10 +1019,15 @@ class AccountsManagerTest { ); } - @Test - void testChangePhoneNumber() throws InterruptedException, MismatchedDevicesException { - final String originalNumber = "+14152222222"; - final String targetNumber = "+14153333333"; + @ParameterizedTest + @CsvSource({ + "+14152222222,+14153333333", + + // Historically, "change number" behavior was different for "change to existing number," though that's no longer + // the case + "+14152222222,+14152222222" + }) + void testChangePhoneNumber(final String originalNumber, final String targetNumber) throws InterruptedException, MismatchedDevicesException { final UUID uuid = UUID.randomUUID(); final UUID originalPni = UUID.randomUUID(); final ECKeyPair pniIdentityKeyPair = Curve.generateKeyPair(); @@ -1048,33 +1053,17 @@ class AccountsManagerTest { verify(keysManager).buildWriteItemForLastResortKey(phoneNumberIdentifiersByE164.get(targetNumber), Device.PRIMARY_ID, kemLastResortPreKey); } - @Test - void testChangePhoneNumberSameNumber() throws InterruptedException, MismatchedDevicesException { - final String number = "+14152222222"; - final ECKeyPair pniIdentityKeyPair = Curve.generateKeyPair(); - - Account account = AccountsHelper.generateTestAccount(number, UUID.randomUUID(), UUID.randomUUID(), List.of(DevicesHelper.createDevice(Device.PRIMARY_ID)), new byte[UnidentifiedAccessUtil.UNIDENTIFIED_ACCESS_KEY_LENGTH]); - phoneNumberIdentifiersByE164.put(number, account.getPhoneNumberIdentifier()); - account = accountsManager.changeNumber(account, - number, - new IdentityKey(pniIdentityKeyPair.getPublicKey()), - Map.of(Device.PRIMARY_ID, KeysHelper.signedECPreKey(1, pniIdentityKeyPair)), - Map.of(Device.PRIMARY_ID, KeysHelper.signedKEMPreKey(2, pniIdentityKeyPair)), - Map.of(Device.PRIMARY_ID, 1)); - - assertEquals(number, account.getNumber()); - verifyNoInteractions(keysManager); - } - @Test void testChangePhoneNumberDifferentNumberSamePni() throws InterruptedException, MismatchedDevicesException { final String originalNumber = "+22923456789"; // the canonical form of numbers may change over time, so we use PNIs as stable identifiers final String newNumber = "+2290123456789"; final ECKeyPair pniIdentityKeyPair = Curve.generateKeyPair(); + final UUID phoneNumberIdentifier = UUID.randomUUID(); + + Account account = AccountsHelper.generateTestAccount(originalNumber, UUID.randomUUID(), phoneNumberIdentifier, + List.of(DevicesHelper.createDevice(Device.PRIMARY_ID)), new byte[UnidentifiedAccessUtil.UNIDENTIFIED_ACCESS_KEY_LENGTH]); - Account account = AccountsHelper.generateTestAccount(originalNumber, UUID.randomUUID(), UUID.randomUUID(), - new ArrayList<>(), new byte[UnidentifiedAccessUtil.UNIDENTIFIED_ACCESS_KEY_LENGTH]); phoneNumberIdentifiersByE164.put(originalNumber, account.getPhoneNumberIdentifier()); phoneNumberIdentifiersByE164.put(newNumber, account.getPhoneNumberIdentifier()); account = accountsManager.changeNumber(account, @@ -1084,8 +1073,9 @@ class AccountsManagerTest { Map.of(Device.PRIMARY_ID, KeysHelper.signedKEMPreKey(2, pniIdentityKeyPair)), Map.of(Device.PRIMARY_ID, 1)); - assertEquals(originalNumber, account.getNumber()); - verifyNoInteractions(keysManager); + assertEquals(newNumber, account.getNumber()); + assertEquals(phoneNumberIdentifier, account.getIdentifier(IdentityType.PNI)); + verify(accounts, never()).delete(any(), any()); } @Test diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManagerTest.java index 61b9223c1..59e10efa4 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManagerTest.java @@ -5,7 +5,6 @@ package org.whispersystems.textsecuregcm.storage; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyByte; @@ -245,153 +244,6 @@ public class ChangeNumberManagerTest { assertEquals(updatedPhoneNumberIdentifiersByAccount.get(account), UUID.fromString(envelope.getUpdatedPni())); } - @Test - void changeNumberSameNumberSetPrimaryDevicePrekeyAndSendMessages() throws Exception { - final String originalE164 = "+18005551234"; - final UUID aci = UUID.randomUUID(); - final UUID pni = UUID.randomUUID(); - - final Account account = mock(Account.class); - when(account.getNumber()).thenReturn(originalE164); - when(account.getUuid()).thenReturn(aci); - when(account.getPhoneNumberIdentifier()).thenReturn(pni); - - final Device d2 = mock(Device.class); - final byte deviceId2 = 2; - when(d2.getId()).thenReturn(deviceId2); - - when(account.getDevice(deviceId2)).thenReturn(Optional.of(d2)); - when(account.getDevices()).thenReturn(List.of(d2)); - - final ECKeyPair pniIdentityKeyPair = Curve.generateKeyPair(); - final IdentityKey pniIdentityKey = new IdentityKey(pniIdentityKeyPair.getPublicKey()); - final Map prekeys = Map.of(Device.PRIMARY_ID, - KeysHelper.signedECPreKey(1, pniIdentityKeyPair), - deviceId2, KeysHelper.signedECPreKey(2, pniIdentityKeyPair)); - final Map pqPrekeys = Map.of((byte) 3, KeysHelper.signedKEMPreKey(3, pniIdentityKeyPair), - (byte) 4, KeysHelper.signedKEMPreKey(4, pniIdentityKeyPair)); - final Map registrationIds = Map.of(Device.PRIMARY_ID, 17, deviceId2, 19); - - final IncomingMessage msg = mock(IncomingMessage.class); - when(msg.destinationDeviceId()).thenReturn(deviceId2); - when(msg.content()).thenReturn(new byte[]{1}); - - changeNumberManager.changeNumber(account, originalE164, pniIdentityKey, prekeys, pqPrekeys, List.of(msg), registrationIds, null); - - verify(accountsManager).updatePniKeys(account, pniIdentityKey, prekeys, pqPrekeys, registrationIds); - - @SuppressWarnings("unchecked") final ArgumentCaptor> envelopeCaptor = - ArgumentCaptor.forClass(Map.class); - - verify(messageSender).sendMessages(any(), any(), envelopeCaptor.capture(), any(), any(), any()); - - assertEquals(1, envelopeCaptor.getValue().size()); - assertEquals(Set.of(deviceId2), envelopeCaptor.getValue().keySet()); - - final MessageProtos.Envelope envelope = envelopeCaptor.getValue().get(deviceId2); - - assertEquals(aci, UUID.fromString(envelope.getDestinationServiceId())); - assertEquals(aci, UUID.fromString(envelope.getSourceServiceId())); - assertEquals(Device.PRIMARY_ID, envelope.getSourceDevice()); - assertFalse(updatedPhoneNumberIdentifiersByAccount.containsKey(account)); - } - - @Test - void updatePniKeysSetPrimaryDevicePrekeyAndSendMessages() throws Exception { - final UUID aci = UUID.randomUUID(); - final UUID pni = UUID.randomUUID(); - - final Account account = mock(Account.class); - when(account.getUuid()).thenReturn(aci); - when(account.getPhoneNumberIdentifier()).thenReturn(pni); - - final Device d2 = mock(Device.class); - final byte deviceId2 = 2; - when(d2.getId()).thenReturn(deviceId2); - - when(account.getDevice(deviceId2)).thenReturn(Optional.of(d2)); - when(account.getDevices()).thenReturn(List.of(d2)); - - final ECKeyPair pniIdentityKeyPair = Curve.generateKeyPair(); - final IdentityKey pniIdentityKey = new IdentityKey(pniIdentityKeyPair.getPublicKey()); - final Map prekeys = Map.of(Device.PRIMARY_ID, - KeysHelper.signedECPreKey(1, pniIdentityKeyPair), - deviceId2, KeysHelper.signedECPreKey(2, pniIdentityKeyPair)); - final Map registrationIds = Map.of(Device.PRIMARY_ID, 17, deviceId2, 19); - - final IncomingMessage msg = mock(IncomingMessage.class); - when(msg.destinationDeviceId()).thenReturn(deviceId2); - when(msg.content()).thenReturn(new byte[]{1}); - - changeNumberManager.updatePniKeys(account, pniIdentityKey, prekeys, null, List.of(msg), registrationIds, null); - - verify(accountsManager).updatePniKeys(account, pniIdentityKey, prekeys, null, registrationIds); - - @SuppressWarnings("unchecked") final ArgumentCaptor> envelopeCaptor = - ArgumentCaptor.forClass(Map.class); - - verify(messageSender).sendMessages(any(), any(), envelopeCaptor.capture(), any(), any(), any()); - - assertEquals(1, envelopeCaptor.getValue().size()); - assertEquals(Set.of(deviceId2), envelopeCaptor.getValue().keySet()); - - final MessageProtos.Envelope envelope = envelopeCaptor.getValue().get(deviceId2); - - assertEquals(aci, UUID.fromString(envelope.getDestinationServiceId())); - assertEquals(aci, UUID.fromString(envelope.getSourceServiceId())); - assertEquals(Device.PRIMARY_ID, envelope.getSourceDevice()); - assertFalse(updatedPhoneNumberIdentifiersByAccount.containsKey(account)); - } - - @Test - void updatePniKeysSetPrimaryDevicePrekeyPqAndSendMessages() throws Exception { - final UUID aci = UUID.randomUUID(); - final UUID pni = UUID.randomUUID(); - - final Account account = mock(Account.class); - when(account.getUuid()).thenReturn(aci); - when(account.getPhoneNumberIdentifier()).thenReturn(pni); - - final Device d2 = mock(Device.class); - final byte deviceId2 = 2; - when(d2.getId()).thenReturn(deviceId2); - - when(account.getDevice(deviceId2)).thenReturn(Optional.of(d2)); - when(account.getDevices()).thenReturn(List.of(d2)); - - final ECKeyPair pniIdentityKeyPair = Curve.generateKeyPair(); - final IdentityKey pniIdentityKey = new IdentityKey(pniIdentityKeyPair.getPublicKey()); - final Map prekeys = Map.of(Device.PRIMARY_ID, - KeysHelper.signedECPreKey(1, pniIdentityKeyPair), - deviceId2, KeysHelper.signedECPreKey(2, pniIdentityKeyPair)); - final Map pqPrekeys = Map.of((byte) 3, KeysHelper.signedKEMPreKey(3, pniIdentityKeyPair), - (byte) 4, KeysHelper.signedKEMPreKey(4, pniIdentityKeyPair)); - final Map registrationIds = Map.of(Device.PRIMARY_ID, 17, deviceId2, 19); - - final IncomingMessage msg = mock(IncomingMessage.class); - when(msg.destinationDeviceId()).thenReturn(deviceId2); - when(msg.content()).thenReturn(new byte[]{1}); - - changeNumberManager.updatePniKeys(account, pniIdentityKey, prekeys, pqPrekeys, List.of(msg), registrationIds, null); - - verify(accountsManager).updatePniKeys(account, pniIdentityKey, prekeys, pqPrekeys, registrationIds); - - @SuppressWarnings("unchecked") final ArgumentCaptor> envelopeCaptor = - ArgumentCaptor.forClass(Map.class); - - verify(messageSender).sendMessages(any(), any(), envelopeCaptor.capture(), any(), any(), any()); - - assertEquals(1, envelopeCaptor.getValue().size()); - assertEquals(Set.of(deviceId2), envelopeCaptor.getValue().keySet()); - - final MessageProtos.Envelope envelope = envelopeCaptor.getValue().get(deviceId2); - - assertEquals(aci, UUID.fromString(envelope.getDestinationServiceId())); - assertEquals(aci, UUID.fromString(envelope.getSourceServiceId())); - assertEquals(Device.PRIMARY_ID, envelope.getSourceDevice()); - assertFalse(updatedPhoneNumberIdentifiersByAccount.containsKey(account)); - } - @Test void changeNumberMissingData() { final Account account = mock(Account.class);