Introduce configurable way to ignore SVR errors in the account deletion flow

This commit is contained in:
Katherine 2024-09-13 10:57:09 -04:00 committed by GitHub
parent f4b94a7a89
commit bd57c1c7e7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 81 additions and 15 deletions

View File

@ -650,7 +650,7 @@ public class WhisperServerService extends Application<WhisperServerConfiguration
secureStorageClient, secureValueRecovery2Client, secureStorageClient, secureValueRecovery2Client,
clientPresenceManager, clientPresenceManager,
registrationRecoveryPasswordsManager, clientPublicKeysManager, accountLockExecutor, clientPresenceExecutor, registrationRecoveryPasswordsManager, clientPublicKeysManager, accountLockExecutor, clientPresenceExecutor,
clock); clock, dynamicConfigurationManager);
RemoteConfigsManager remoteConfigsManager = new RemoteConfigsManager(remoteConfigs); RemoteConfigsManager remoteConfigsManager = new RemoteConfigsManager(remoteConfigs);
APNSender apnSender = new APNSender(apnSenderExecutor, config.getApnConfiguration()); APNSender apnSender = new APNSender(apnSenderExecutor, config.getApnConfiguration());
FcmSender fcmSender = new FcmSender(fcmSenderExecutor, config.getFcmConfiguration().credentials().value()); FcmSender fcmSender = new FcmSender(fcmSenderExecutor, config.getFcmConfiguration().credentials().value());

View File

@ -8,6 +8,7 @@ package org.whispersystems.textsecuregcm.configuration.dynamic;
import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonProperty;
import java.util.Collections; import java.util.Collections;
import java.util.HashMap; import java.util.HashMap;
import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.Optional; import java.util.Optional;
import javax.validation.Valid; import javax.validation.Valid;
@ -71,6 +72,10 @@ public class DynamicConfiguration {
@Valid @Valid
DynamicMessagesConfiguration messagesConfiguration = new DynamicMessagesConfiguration(); DynamicMessagesConfiguration messagesConfiguration = new DynamicMessagesConfiguration();
@JsonProperty
@Valid
List<String> svrStatusCodesToIgnoreForAccountDeletion = Collections.emptyList();
public Optional<DynamicExperimentEnrollmentConfiguration> getExperimentEnrollmentConfiguration( public Optional<DynamicExperimentEnrollmentConfiguration> getExperimentEnrollmentConfiguration(
final String experimentName) { final String experimentName) {
return Optional.ofNullable(experiments.get(experimentName)); return Optional.ofNullable(experiments.get(experimentName));
@ -129,4 +134,8 @@ public class DynamicConfiguration {
return messagesConfiguration; return messagesConfiguration;
} }
public List<String> getSvrStatusCodesToIgnoreForAccountDeletion() {
return svrStatusCodesToIgnoreForAccountDeletion;
}
} }

View File

@ -72,7 +72,7 @@ public class SecureValueRecovery2Client {
return null; return null;
} }
throw new SecureValueRecoveryException("Failed to delete backup: " + response.statusCode()); throw new SecureValueRecoveryException("Failed to delete backup", String.valueOf(response.statusCode()));
}); });
} }

View File

@ -6,8 +6,14 @@
package org.whispersystems.textsecuregcm.securevaluerecovery; package org.whispersystems.textsecuregcm.securevaluerecovery;
public class SecureValueRecoveryException extends RuntimeException { public class SecureValueRecoveryException extends RuntimeException {
private final String statusCode;
public SecureValueRecoveryException(final String message) { public SecureValueRecoveryException(final String message, final String statusCode) {
super(message); super(message);
this.statusCode = statusCode;
}
public String getStatusCode() {
return statusCode;
} }
} }

View File

@ -33,6 +33,7 @@ import java.util.Optional;
import java.util.Queue; import java.util.Queue;
import java.util.UUID; import java.util.UUID;
import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.CompletionStage; import java.util.concurrent.CompletionStage;
import java.util.concurrent.Executor; import java.util.concurrent.Executor;
import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.atomic.AtomicReference;
@ -48,6 +49,7 @@ import org.signal.libsignal.protocol.IdentityKey;
import org.slf4j.Logger; import org.slf4j.Logger;
import org.slf4j.LoggerFactory; import org.slf4j.LoggerFactory;
import org.whispersystems.textsecuregcm.auth.SaltedTokenHash; import org.whispersystems.textsecuregcm.auth.SaltedTokenHash;
import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration;
import org.whispersystems.textsecuregcm.controllers.MismatchedDevicesException; import org.whispersystems.textsecuregcm.controllers.MismatchedDevicesException;
import org.whispersystems.textsecuregcm.entities.AccountAttributes; import org.whispersystems.textsecuregcm.entities.AccountAttributes;
import org.whispersystems.textsecuregcm.entities.ECSignedPreKey; 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.redis.RedisOperation;
import org.whispersystems.textsecuregcm.securestorage.SecureStorageClient; import org.whispersystems.textsecuregcm.securestorage.SecureStorageClient;
import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecovery2Client; import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecovery2Client;
import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecoveryException;
import org.whispersystems.textsecuregcm.util.DestinationDeviceValidator; import org.whispersystems.textsecuregcm.util.DestinationDeviceValidator;
import org.whispersystems.textsecuregcm.util.ExceptionUtils; import org.whispersystems.textsecuregcm.util.ExceptionUtils;
import org.whispersystems.textsecuregcm.util.Pair; import org.whispersystems.textsecuregcm.util.Pair;
@ -107,6 +110,7 @@ public class AccountsManager {
private final Executor accountLockExecutor; private final Executor accountLockExecutor;
private final Executor clientPresenceExecutor; private final Executor clientPresenceExecutor;
private final Clock clock; private final Clock clock;
private final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager;
private static final ObjectWriter ACCOUNT_REDIS_JSON_WRITER = SystemMapper.jsonMapper() private static final ObjectWriter ACCOUNT_REDIS_JSON_WRITER = SystemMapper.jsonMapper()
.writer(SystemMapper.excludingField(Account.class, List.of("uuid"))); .writer(SystemMapper.excludingField(Account.class, List.of("uuid")));
@ -147,7 +151,8 @@ public class AccountsManager {
final ClientPublicKeysManager clientPublicKeysManager, final ClientPublicKeysManager clientPublicKeysManager,
final Executor accountLockExecutor, final Executor accountLockExecutor,
final Executor clientPresenceExecutor, final Executor clientPresenceExecutor,
final Clock clock) { final Clock clock,
final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager) {
this.accounts = accounts; this.accounts = accounts;
this.phoneNumberIdentifiers = phoneNumberIdentifiers; this.phoneNumberIdentifiers = phoneNumberIdentifiers;
this.cacheCluster = cacheCluster; this.cacheCluster = cacheCluster;
@ -163,6 +168,7 @@ public class AccountsManager {
this.accountLockExecutor = accountLockExecutor; this.accountLockExecutor = accountLockExecutor;
this.clientPresenceExecutor = clientPresenceExecutor; this.clientPresenceExecutor = clientPresenceExecutor;
this.clock = requireNonNull(clock); this.clock = requireNonNull(clock);
this.dynamicConfigurationManager = dynamicConfigurationManager;
} }
public Account create(final String number, public Account create(final String number,
@ -982,10 +988,19 @@ public class AccountsManager {
account.getIdentifier(IdentityType.ACI), account.getIdentifier(IdentityType.ACI),
device.getId()))) device.getId())))
.toList(); .toList();
CompletableFuture<Void> deleteBackupFuture = secureValueRecovery2Client.deleteBackups(account.getUuid())
.exceptionally(exception -> {
final List<String> 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( return CompletableFuture.allOf(
secureStorageClient.deleteStoredData(account.getUuid()), secureStorageClient.deleteStoredData(account.getUuid()),
secureValueRecovery2Client.deleteBackups(account.getUuid()), deleteBackupFuture,
keysManager.deleteSingleUsePreKeys(account.getUuid()), keysManager.deleteSingleUsePreKeys(account.getUuid()),
keysManager.deleteSingleUsePreKeys(account.getPhoneNumberIdentifier()), keysManager.deleteSingleUsePreKeys(account.getPhoneNumberIdentifier()),
messagesManager.clear(account.getUuid()), messagesManager.clear(account.getUuid()),

View File

@ -222,7 +222,7 @@ record CommandDependencies(
accountLockManager, keys, messagesManager, profilesManager, accountLockManager, keys, messagesManager, profilesManager,
secureStorageClient, secureValueRecovery2Client, clientPresenceManager, secureStorageClient, secureValueRecovery2Client, clientPresenceManager,
registrationRecoveryPasswordsManager, clientPublicKeysManager, accountLockExecutor, clientPresenceExecutor, registrationRecoveryPasswordsManager, clientPublicKeysManager, accountLockExecutor, clientPresenceExecutor,
clock); clock, dynamicConfigurationManager);
RateLimiters rateLimiters = RateLimiters.createAndValidate(configuration.getLimitsConfiguration(), RateLimiters rateLimiters = RateLimiters.createAndValidate(configuration.getLimitsConfiguration(),
dynamicConfigurationManager, rateLimitersCluster); dynamicConfigurationManager, rateLimitersCluster);
final BackupsDb backupsDb = final BackupsDb backupsDb =

View File

@ -152,7 +152,8 @@ public class AccountCreationDeletionIntegrationTest {
clientPublicKeysManager, clientPublicKeysManager,
accountLockExecutor, accountLockExecutor,
clientPresenceExecutor, clientPresenceExecutor,
CLOCK); CLOCK,
dynamicConfigurationManager);
} }
@AfterEach @AfterEach

View File

@ -147,7 +147,8 @@ class AccountsManagerChangeNumberIntegrationTest {
clientPublicKeysManager, clientPublicKeysManager,
accountLockExecutor, accountLockExecutor,
clientPresenceExecutor, clientPresenceExecutor,
mock(Clock.class)); mock(Clock.class),
dynamicConfigurationManager);
} }
} }

View File

@ -134,7 +134,8 @@ class AccountsManagerConcurrentModificationIntegrationTest {
mock(ClientPublicKeysManager.class), mock(ClientPublicKeysManager.class),
mock(Executor.class), mock(Executor.class),
mock(Executor.class), mock(Executor.class),
mock(Clock.class) mock(Clock.class),
dynamicConfigurationManager
); );
} }
} }

View File

@ -48,6 +48,7 @@ import java.util.Objects;
import java.util.Optional; import java.util.Optional;
import java.util.UUID; import java.util.UUID;
import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.Executor; import java.util.concurrent.Executor;
import java.util.function.Consumer; import java.util.function.Consumer;
import java.util.function.Supplier; 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.push.ClientPresenceManager;
import org.whispersystems.textsecuregcm.securestorage.SecureStorageClient; import org.whispersystems.textsecuregcm.securestorage.SecureStorageClient;
import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecovery2Client; import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecovery2Client;
import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecoveryException;
import org.whispersystems.textsecuregcm.storage.AccountsManager.UsernameReservation; import org.whispersystems.textsecuregcm.storage.AccountsManager.UsernameReservation;
import org.whispersystems.textsecuregcm.storage.Device.DeviceCapabilities; import org.whispersystems.textsecuregcm.storage.Device.DeviceCapabilities;
import org.whispersystems.textsecuregcm.tests.util.AccountsHelper; import org.whispersystems.textsecuregcm.tests.util.AccountsHelper;
@ -113,6 +115,8 @@ class AccountsManagerTest {
private RedisAdvancedClusterAsyncCommands<String, String> asyncCommands; private RedisAdvancedClusterAsyncCommands<String, String> asyncCommands;
private TestClock clock; private TestClock clock;
private AccountsManager accountsManager; private AccountsManager accountsManager;
private SecureValueRecovery2Client svr2Client;
private DynamicConfiguration dynamicConfiguration;
private static final Answer<?> ACCOUNT_UPDATE_ANSWER = (answer) -> { private static final Answer<?> ACCOUNT_UPDATE_ANSWER = (answer) -> {
// it is implicit in the update() contract is that a successful call will // it is implicit in the update() contract is that a successful call will
@ -139,6 +143,7 @@ class AccountsManagerTest {
profilesManager = mock(ProfilesManager.class); profilesManager = mock(ProfilesManager.class);
clientPresenceManager = mock(ClientPresenceManager.class); clientPresenceManager = mock(ClientPresenceManager.class);
clientPublicKeysManager = mock(ClientPublicKeysManager.class); clientPublicKeysManager = mock(ClientPublicKeysManager.class);
dynamicConfiguration = mock(DynamicConfiguration.class);
final Executor clientPresenceExecutor = mock(Executor.class); final Executor clientPresenceExecutor = mock(Executor.class);
@ -175,7 +180,7 @@ class AccountsManagerTest {
final SecureStorageClient storageClient = mock(SecureStorageClient.class); final SecureStorageClient storageClient = mock(SecureStorageClient.class);
when(storageClient.deleteStoredData(any())).thenReturn(CompletableFuture.completedFuture(null)); 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)); when(svr2Client.deleteBackups(any())).thenReturn(CompletableFuture.completedFuture(null));
final PhoneNumberIdentifiers phoneNumberIdentifiers = mock(PhoneNumberIdentifiers.class); final PhoneNumberIdentifiers phoneNumberIdentifiers = mock(PhoneNumberIdentifiers.class);
@ -189,9 +194,8 @@ class AccountsManagerTest {
@SuppressWarnings("unchecked") final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager = @SuppressWarnings("unchecked") final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager =
mock(DynamicConfigurationManager.class); mock(DynamicConfigurationManager.class);
final DynamicConfiguration dynamicConfiguration = mock(DynamicConfiguration.class);
when(dynamicConfigurationManager.getConfiguration()).thenReturn(dynamicConfiguration); when(dynamicConfigurationManager.getConfiguration()).thenReturn(dynamicConfiguration);
when(dynamicConfiguration.getSvrStatusCodesToIgnoreForAccountDeletion()).thenReturn(Collections.emptyList());
final AccountLockManager accountLockManager = mock(AccountLockManager.class); final AccountLockManager accountLockManager = mock(AccountLockManager.class);
@ -218,6 +222,7 @@ class AccountsManagerTest {
clock = TestClock.now(); clock = TestClock.now();
accountsManager = new AccountsManager( accountsManager = new AccountsManager(
accounts, accounts,
phoneNumberIdentifiers, phoneNumberIdentifiers,
@ -236,7 +241,33 @@ class AccountsManagerTest {
clientPublicKeysManager, clientPublicKeysManager,
mock(Executor.class), mock(Executor.class),
clientPresenceExecutor, 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<Arguments> testDeleteWithSvrErrorStatusCodes() {
return Stream.of(
Arguments.of("500", false),
Arguments.of("429", true)
);
} }
@Test @Test

View File

@ -146,7 +146,8 @@ class AccountsManagerUsernameIntegrationTest {
mock(ClientPublicKeysManager.class), mock(ClientPublicKeysManager.class),
Executors.newSingleThreadExecutor(), Executors.newSingleThreadExecutor(),
Executors.newSingleThreadExecutor(), Executors.newSingleThreadExecutor(),
mock(Clock.class)); mock(Clock.class),
dynamicConfigurationManager);
} }
@Test @Test

View File

@ -140,7 +140,8 @@ public class AddRemoveDeviceIntegrationTest {
clientPublicKeysManager, clientPublicKeysManager,
accountLockExecutor, accountLockExecutor,
clientPresenceExecutor, clientPresenceExecutor,
CLOCK); CLOCK,
dynamicConfigurationManager);
} }
@AfterEach @AfterEach