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 a3894272c..80343891e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2021 Signal Messenger, LLC + * Copyright 2013-2022 Signal Messenger, LLC * SPDX-License-Identifier: AGPL-3.0-only */ package org.whispersystems.textsecuregcm.storage; @@ -228,7 +228,7 @@ public class AccountsManager { final AtomicReference updatedAccount = new AtomicReference<>(); - deletedAccountsManager.lockAndPut(account.getNumber(), number, () -> { + deletedAccountsManager.lockAndPut(account.getNumber(), number, (originalAci, deletedAci) -> { redisDelete(account); final Optional maybeExistingAccount = getByE164(number); @@ -239,7 +239,7 @@ public class AccountsManager { directoryQueue.deleteAccount(maybeExistingAccount.get()); displacedUuid = maybeExistingAccount.map(Account::getUuid); } else { - displacedUuid = Optional.empty(); + displacedUuid = deletedAci; } final UUID uuid = account.getUuid(); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsManager.java index 1d1b54ee4..5373246bf 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/DeletedAccountsManager.java @@ -19,6 +19,7 @@ import java.util.Optional; import java.util.Set; import java.util.UUID; import java.util.concurrent.TimeUnit; +import java.util.function.BiFunction; import java.util.function.Consumer; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -72,9 +73,9 @@ public class DeletedAccountsManager { * @throws InterruptedException if interrupted while waiting to acquire a lock on the given phone number */ public void lockAndTake(final String e164, final Consumer> consumer) throws InterruptedException { - withLock(List.of(e164), () -> { + withLock(List.of(e164), acis -> { try { - consumer.accept(deletedAccounts.findUuid(e164)); + consumer.accept(acis.get(0)); deletedAccounts.remove(e164); } catch (final Exception e) { log.warn("Consumer threw an exception while holding lock on a deleted account record", e); @@ -95,7 +96,7 @@ public class DeletedAccountsManager { * @throws InterruptedException if interrupted while waiting to acquire a lock on the given phone number */ public void lockAndPut(final String e164, final Supplier supplier) throws InterruptedException { - withLock(List.of(e164), () -> { + withLock(List.of(e164), ignored -> { try { deletedAccounts.put(supplier.get(), e164, true); } catch (final Exception e) { @@ -111,18 +112,18 @@ public class DeletedAccountsManager { * recently-deleted e164-to-UUID mappings. * * @param original the phone number of an existing account to lock and with which to perform an action - * @param target the phone number of an account that may or may not exist with which to perform an action - * @param supplier the action to take on the given phone numbers; the action may delete the account identified by the - * target number, in which case it must return the UUID of that account - * + * @param target the phone number of an account that may or may not exist with which to perform an action + * @param function the action to take on the given phone numbers and ACIs, if known; the action may delete the account + * identified by the target number, in which case it must return the ACI of that account * @throws InterruptedException if interrupted while waiting to acquire a lock on the given phone numbers */ - public void lockAndPut(final String original, final String target, final Supplier> supplier) + public void lockAndPut(final String original, final String target, + final BiFunction, Optional, Optional> function) throws InterruptedException { - withLock(List.of(original, target), () -> { + withLock(List.of(original, target), acis -> { try { - supplier.get().ifPresent(uuid -> deletedAccounts.put(uuid, original, true)); + function.apply(acis.get(0), acis.get(1)).ifPresent(aci -> deletedAccounts.put(aci, original, true)); } catch (final Exception e) { log.warn("Supplier threw an exception while holding lock on a deleted account record", e); throw new RuntimeException(e); @@ -130,17 +131,20 @@ public class DeletedAccountsManager { }); } - private void withLock(final List e164s, final Runnable task) throws InterruptedException { + private void withLock(final List e164s, final Consumer>> task) + throws InterruptedException { final List lockItems = new ArrayList<>(e164s.size()); try { + final List> previouslyDeletedUuids = new ArrayList<>(e164s.size()); for (final String e164 : e164s) { lockItems.add(lockClient.acquireLock(AcquireLockOptions.builder(e164) .withAcquireReleasedLocksConsistently(true) .build())); + previouslyDeletedUuids.add(deletedAccounts.findUuid(e164)); } - task.run(); + task.accept(previouslyDeletedUuids); } finally { for (final LockItem lockItem : lockItems) { lockClient.releaseLock(ReleaseLockOptions.builder(lockItem) 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 8c6c04cb3..3a4fd5827 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java @@ -269,6 +269,13 @@ class AccountsManagerChangeNumberIntegrationTest { assertEquals(Optional.of(existingAccountUuid), deletedAccounts.findUuid(originalNumber)); assertEquals(Optional.empty(), deletedAccounts.findUuid(secondNumber)); + + accountsManager.changeNumber(accountsManager.getByAccountIdentifier(originalUuid).orElseThrow(), originalNumber); + + final Account existingAccount2 = accountsManager.create(secondNumber, "password", null, new AccountAttributes(), + new ArrayList<>()); + + assertEquals(existingAccountUuid, existingAccount2.getUuid()); } @Test 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 0596f7fd4..072cd4e59 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java @@ -35,8 +35,8 @@ import java.util.Map; import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; +import java.util.function.BiFunction; import java.util.function.Consumer; -import java.util.function.Supplier; import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -640,7 +640,7 @@ class AccountsManagerTest { @Test void testChangePhoneNumber() throws InterruptedException { - doAnswer(invocation -> invocation.getArgument(2, Supplier.class).get()) + doAnswer(invocation -> invocation.getArgument(2, BiFunction.class).apply(Optional.empty(), Optional.empty())) .when(deletedAccountsManager).lockAndPut(anyString(), anyString(), any()); final String originalNumber = "+14152222222"; @@ -669,7 +669,7 @@ class AccountsManagerTest { @Test void testChangePhoneNumberExistingAccount() throws InterruptedException { - doAnswer(invocation -> invocation.getArgument(2, Supplier.class).get()) + doAnswer(invocation -> invocation.getArgument(2, BiFunction.class).apply(Optional.empty(), Optional.empty())) .when(deletedAccountsManager).lockAndPut(anyString(), anyString(), any()); final String originalNumber = "+14152222222";