From bd57c1c7e7631ee92c2f4ab6cc4406f3ed99d3f5 Mon Sep 17 00:00:00 2001 From: Katherine Date: Fri, 13 Sep 2024 10:57:09 -0400 Subject: [PATCH] Introduce configurable way to ignore SVR errors in the account deletion flow --- .../textsecuregcm/WhisperServerService.java | 2 +- .../dynamic/DynamicConfiguration.java | 9 +++++ .../SecureValueRecovery2Client.java | 2 +- .../SecureValueRecoveryException.java | 8 +++- .../storage/AccountsManager.java | 19 ++++++++- .../workers/CommandDependencies.java | 2 +- ...ccountCreationDeletionIntegrationTest.java | 3 +- ...ntsManagerChangeNumberIntegrationTest.java | 3 +- ...ConcurrentModificationIntegrationTest.java | 3 +- .../storage/AccountsManagerTest.java | 39 +++++++++++++++++-- ...ccountsManagerUsernameIntegrationTest.java | 3 +- .../AddRemoveDeviceIntegrationTest.java | 3 +- 12 files changed, 81 insertions(+), 15 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 5fcf25655..27516266b 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -650,7 +650,7 @@ public class WhisperServerService extends Application svrStatusCodesToIgnoreForAccountDeletion = Collections.emptyList(); + public Optional getExperimentEnrollmentConfiguration( final String experimentName) { return Optional.ofNullable(experiments.get(experimentName)); @@ -129,4 +134,8 @@ public class DynamicConfiguration { return messagesConfiguration; } + public List getSvrStatusCodesToIgnoreForAccountDeletion() { + return svrStatusCodesToIgnoreForAccountDeletion; + } + } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/securevaluerecovery/SecureValueRecovery2Client.java b/service/src/main/java/org/whispersystems/textsecuregcm/securevaluerecovery/SecureValueRecovery2Client.java index 465958be5..21ce5ecd7 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/securevaluerecovery/SecureValueRecovery2Client.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/securevaluerecovery/SecureValueRecovery2Client.java @@ -72,7 +72,7 @@ public class SecureValueRecovery2Client { return null; } - throw new SecureValueRecoveryException("Failed to delete backup: " + response.statusCode()); + throw new SecureValueRecoveryException("Failed to delete backup", String.valueOf(response.statusCode())); }); } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/securevaluerecovery/SecureValueRecoveryException.java b/service/src/main/java/org/whispersystems/textsecuregcm/securevaluerecovery/SecureValueRecoveryException.java index e6a3a9b70..aeb6152a1 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/securevaluerecovery/SecureValueRecoveryException.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/securevaluerecovery/SecureValueRecoveryException.java @@ -6,8 +6,14 @@ package org.whispersystems.textsecuregcm.securevaluerecovery; public class SecureValueRecoveryException extends RuntimeException { + private final String statusCode; - public SecureValueRecoveryException(final String message) { + public SecureValueRecoveryException(final String message, final String statusCode) { super(message); + this.statusCode = statusCode; + } + + public String getStatusCode() { + return statusCode; } } 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 43cad4bd2..13d524320 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -33,6 +33,7 @@ import java.util.Optional; import java.util.Queue; import java.util.UUID; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.concurrent.CompletionStage; import java.util.concurrent.Executor; import java.util.concurrent.atomic.AtomicReference; @@ -48,6 +49,7 @@ import org.signal.libsignal.protocol.IdentityKey; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.whispersystems.textsecuregcm.auth.SaltedTokenHash; +import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; import org.whispersystems.textsecuregcm.controllers.MismatchedDevicesException; import org.whispersystems.textsecuregcm.entities.AccountAttributes; import org.whispersystems.textsecuregcm.entities.ECSignedPreKey; @@ -60,6 +62,7 @@ import org.whispersystems.textsecuregcm.redis.FaultTolerantRedisCluster; import org.whispersystems.textsecuregcm.redis.RedisOperation; import org.whispersystems.textsecuregcm.securestorage.SecureStorageClient; import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecovery2Client; +import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecoveryException; import org.whispersystems.textsecuregcm.util.DestinationDeviceValidator; import org.whispersystems.textsecuregcm.util.ExceptionUtils; import org.whispersystems.textsecuregcm.util.Pair; @@ -107,6 +110,7 @@ public class AccountsManager { private final Executor accountLockExecutor; private final Executor clientPresenceExecutor; private final Clock clock; + private final DynamicConfigurationManager dynamicConfigurationManager; private static final ObjectWriter ACCOUNT_REDIS_JSON_WRITER = SystemMapper.jsonMapper() .writer(SystemMapper.excludingField(Account.class, List.of("uuid"))); @@ -147,7 +151,8 @@ public class AccountsManager { final ClientPublicKeysManager clientPublicKeysManager, final Executor accountLockExecutor, final Executor clientPresenceExecutor, - final Clock clock) { + final Clock clock, + final DynamicConfigurationManager dynamicConfigurationManager) { this.accounts = accounts; this.phoneNumberIdentifiers = phoneNumberIdentifiers; this.cacheCluster = cacheCluster; @@ -163,6 +168,7 @@ public class AccountsManager { this.accountLockExecutor = accountLockExecutor; this.clientPresenceExecutor = clientPresenceExecutor; this.clock = requireNonNull(clock); + this.dynamicConfigurationManager = dynamicConfigurationManager; } public Account create(final String number, @@ -982,10 +988,19 @@ public class AccountsManager { account.getIdentifier(IdentityType.ACI), device.getId()))) .toList(); + CompletableFuture deleteBackupFuture = secureValueRecovery2Client.deleteBackups(account.getUuid()) + .exceptionally(exception -> { + final List svrStatusCodesToIgnore = dynamicConfigurationManager.getConfiguration().getSvrStatusCodesToIgnoreForAccountDeletion(); + if (exception instanceof SecureValueRecoveryException e && svrStatusCodesToIgnore.contains(e.getStatusCode())) { + logger.warn("Failed to delete backup for account: " + account.getUuid(), exception); + return null; + } + throw new CompletionException(exception); + }); return CompletableFuture.allOf( secureStorageClient.deleteStoredData(account.getUuid()), - secureValueRecovery2Client.deleteBackups(account.getUuid()), + deleteBackupFuture, keysManager.deleteSingleUsePreKeys(account.getUuid()), keysManager.deleteSingleUsePreKeys(account.getPhoneNumberIdentifier()), messagesManager.clear(account.getUuid()), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java index 71e113d28..f7dfcba61 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/CommandDependencies.java @@ -222,7 +222,7 @@ record CommandDependencies( accountLockManager, keys, messagesManager, profilesManager, secureStorageClient, secureValueRecovery2Client, clientPresenceManager, registrationRecoveryPasswordsManager, clientPublicKeysManager, accountLockExecutor, clientPresenceExecutor, - clock); + clock, dynamicConfigurationManager); RateLimiters rateLimiters = RateLimiters.createAndValidate(configuration.getLimitsConfiguration(), dynamicConfigurationManager, rateLimitersCluster); final BackupsDb backupsDb = diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountCreationDeletionIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountCreationDeletionIntegrationTest.java index 6a55fc109..f63807b88 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountCreationDeletionIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountCreationDeletionIntegrationTest.java @@ -152,7 +152,8 @@ public class AccountCreationDeletionIntegrationTest { clientPublicKeysManager, accountLockExecutor, clientPresenceExecutor, - CLOCK); + CLOCK, + dynamicConfigurationManager); } @AfterEach 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 e9b81dc41..8447d00e5 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerChangeNumberIntegrationTest.java @@ -147,7 +147,8 @@ class AccountsManagerChangeNumberIntegrationTest { clientPublicKeysManager, accountLockExecutor, clientPresenceExecutor, - mock(Clock.class)); + mock(Clock.class), + dynamicConfigurationManager); } } 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 7216d298d..f9419a4f2 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java @@ -134,7 +134,8 @@ class AccountsManagerConcurrentModificationIntegrationTest { mock(ClientPublicKeysManager.class), mock(Executor.class), mock(Executor.class), - mock(Clock.class) + mock(Clock.class), + dynamicConfigurationManager ); } } 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 3e0dae3b9..af2407a24 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java @@ -48,6 +48,7 @@ import java.util.Objects; import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.concurrent.Executor; import java.util.function.Consumer; import java.util.function.Supplier; @@ -76,6 +77,7 @@ import org.whispersystems.textsecuregcm.identity.PniServiceIdentifier; import org.whispersystems.textsecuregcm.push.ClientPresenceManager; import org.whispersystems.textsecuregcm.securestorage.SecureStorageClient; import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecovery2Client; +import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecoveryException; import org.whispersystems.textsecuregcm.storage.AccountsManager.UsernameReservation; import org.whispersystems.textsecuregcm.storage.Device.DeviceCapabilities; import org.whispersystems.textsecuregcm.tests.util.AccountsHelper; @@ -113,6 +115,8 @@ class AccountsManagerTest { private RedisAdvancedClusterAsyncCommands asyncCommands; private TestClock clock; private AccountsManager accountsManager; + private SecureValueRecovery2Client svr2Client; + private DynamicConfiguration dynamicConfiguration; private static final Answer ACCOUNT_UPDATE_ANSWER = (answer) -> { // it is implicit in the update() contract is that a successful call will @@ -139,6 +143,7 @@ class AccountsManagerTest { profilesManager = mock(ProfilesManager.class); clientPresenceManager = mock(ClientPresenceManager.class); clientPublicKeysManager = mock(ClientPublicKeysManager.class); + dynamicConfiguration = mock(DynamicConfiguration.class); final Executor clientPresenceExecutor = mock(Executor.class); @@ -175,7 +180,7 @@ class AccountsManagerTest { final SecureStorageClient storageClient = mock(SecureStorageClient.class); when(storageClient.deleteStoredData(any())).thenReturn(CompletableFuture.completedFuture(null)); - final SecureValueRecovery2Client svr2Client = mock(SecureValueRecovery2Client.class); + svr2Client = mock(SecureValueRecovery2Client.class); when(svr2Client.deleteBackups(any())).thenReturn(CompletableFuture.completedFuture(null)); final PhoneNumberIdentifiers phoneNumberIdentifiers = mock(PhoneNumberIdentifiers.class); @@ -189,9 +194,8 @@ class AccountsManagerTest { @SuppressWarnings("unchecked") final DynamicConfigurationManager dynamicConfigurationManager = mock(DynamicConfigurationManager.class); - final DynamicConfiguration dynamicConfiguration = mock(DynamicConfiguration.class); - when(dynamicConfigurationManager.getConfiguration()).thenReturn(dynamicConfiguration); + when(dynamicConfiguration.getSvrStatusCodesToIgnoreForAccountDeletion()).thenReturn(Collections.emptyList()); final AccountLockManager accountLockManager = mock(AccountLockManager.class); @@ -218,6 +222,7 @@ class AccountsManagerTest { clock = TestClock.now(); + accountsManager = new AccountsManager( accounts, phoneNumberIdentifiers, @@ -236,7 +241,33 @@ class AccountsManagerTest { clientPublicKeysManager, mock(Executor.class), clientPresenceExecutor, - clock); + clock, + dynamicConfigurationManager); + } + + @ParameterizedTest + @MethodSource + void testDeleteWithSvrErrorStatusCodes(final String statusCode, final boolean expectError) throws InterruptedException { + when(svr2Client.deleteBackups(any())).thenReturn( + CompletableFuture.failedFuture(new SecureValueRecoveryException("Failed to delete backup", statusCode))); + when(dynamicConfiguration.getSvrStatusCodesToIgnoreForAccountDeletion()).thenReturn(List.of("500")); + + final AccountAttributes attributes = new AccountAttributes(false, 1, 2, null, null, true, null); + + final Account createdAccount = createAccount("+18005550123", attributes); + + if (expectError) { + assertThrows(CompletionException.class, () -> accountsManager.delete(createdAccount, AccountsManager.DeletionReason.USER_REQUEST).toCompletableFuture().join()); + } else { + assertDoesNotThrow(() -> accountsManager.delete(createdAccount, AccountsManager.DeletionReason.USER_REQUEST).toCompletableFuture().join()); + } + } + + private static Stream testDeleteWithSvrErrorStatusCodes() { + return Stream.of( + Arguments.of("500", false), + Arguments.of("429", true) + ); } @Test diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java index 176781d4d..a3f341c84 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java @@ -146,7 +146,8 @@ class AccountsManagerUsernameIntegrationTest { mock(ClientPublicKeysManager.class), Executors.newSingleThreadExecutor(), Executors.newSingleThreadExecutor(), - mock(Clock.class)); + mock(Clock.class), + dynamicConfigurationManager); } @Test diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java index 6db9be434..e11b477e7 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java @@ -140,7 +140,8 @@ public class AddRemoveDeviceIntegrationTest { clientPublicKeysManager, accountLockExecutor, clientPresenceExecutor, - CLOCK); + CLOCK, + dynamicConfigurationManager); } @AfterEach