Consolidate avatar deletion logic in ProfilesManager

This commit is contained in:
Chris Eager 2025-05-22 14:50:07 -05:00 committed by Chris Eager
parent c1a66e0418
commit ccb209ad37
7 changed files with 57 additions and 74 deletions

View File

@ -872,7 +872,7 @@ public class WhisperServerService extends Application<WhisperServerConfiguration
.addService(ExternalServiceCredentialsGrpcService.createForAllExternalServices(config, rateLimiters))
.addService(new KeysGrpcService(accountsManager, keysManager, rateLimiters))
.addService(new ProfileGrpcService(clock, accountsManager, profilesManager, dynamicConfigurationManager,
config.getBadges(), asyncCdnS3Client, profileCdnPolicyGenerator, profileCdnPolicySigner, profileBadgeConverter, rateLimiters, zkProfileOperations, config.getCdnConfiguration().bucket()));
config.getBadges(), profileCdnPolicyGenerator, profileCdnPolicySigner, profileBadgeConverter, rateLimiters, zkProfileOperations));
}
};
@ -1100,8 +1100,8 @@ public class WhisperServerService extends Application<WhisperServerConfiguration
Clock.systemUTC()),
new PaymentsController(currencyManager, paymentsCredentialsGenerator),
new ProfileController(clock, rateLimiters, accountsManager, profilesManager, dynamicConfigurationManager,
profileBadgeConverter, config.getBadges(), cdnS3Client, profileCdnPolicyGenerator, profileCdnPolicySigner,
config.getCdnConfiguration().bucket(), zkSecretParams, zkProfileOperations, batchIdentityCheckExecutor),
profileBadgeConverter, config.getBadges(), profileCdnPolicyGenerator, profileCdnPolicySigner,
zkSecretParams, zkProfileOperations, batchIdentityCheckExecutor),
new ProvisioningController(rateLimiters, provisioningManager),
new RegistrationController(accountsManager, phoneVerificationTokenManager, registrationLockVerificationManager,
rateLimiters),

View File

@ -96,8 +96,6 @@ import org.whispersystems.textsecuregcm.util.ProfileHelper;
import org.whispersystems.textsecuregcm.util.Util;
import org.whispersystems.websocket.auth.Mutable;
import org.whispersystems.websocket.auth.ReadOnly;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.DeleteObjectRequest;
@SuppressWarnings("OptionalUsedAsFieldOrParameterType")
@Path("/v1/profile")
@ -116,9 +114,6 @@ public class ProfileController {
private final ServerSecretParams serverSecretParams;
private final ServerZkProfileOperations zkProfileOperations;
private final S3Client s3client;
private final String bucket;
private final Executor batchIdentityCheckExecutor;
private static final String EXPIRING_PROFILE_KEY_CREDENTIAL_TYPE = "expiringProfileKey";
@ -134,10 +129,8 @@ public class ProfileController {
DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager,
ProfileBadgeConverter profileBadgeConverter,
BadgesConfiguration badgesConfiguration,
S3Client s3client,
PostPolicyGenerator policyGenerator,
PolicySigner policySigner,
String bucket,
ServerSecretParams serverSecretParams,
ServerZkProfileOperations zkProfileOperations,
Executor batchIdentityCheckExecutor) {
@ -151,8 +144,6 @@ public class ProfileController {
BadgeConfiguration::getId, Function.identity()));
this.serverSecretParams = serverSecretParams;
this.zkProfileOperations = zkProfileOperations;
this.bucket = bucket;
this.s3client = s3client;
this.policyGenerator = policyGenerator;
this.policySigner = policySigner;
this.batchIdentityCheckExecutor = Preconditions.checkNotNull(batchIdentityCheckExecutor);
@ -200,10 +191,7 @@ public class ProfileController {
request.commitment().serialize()));
if (request.getAvatarChange() != CreateProfileRequest.AvatarChange.UNCHANGED) {
currentAvatar.ifPresent(s -> s3client.deleteObject(DeleteObjectRequest.builder()
.bucket(bucket)
.key(s)
.build()));
currentAvatar.ifPresent(s -> profilesManager.deleteAvatar(s).join());
}
accountsManager.update(auth.getAccount(), a -> {

View File

@ -50,8 +50,6 @@ import org.whispersystems.textsecuregcm.util.Pair;
import org.whispersystems.textsecuregcm.util.ProfileHelper;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
import software.amazon.awssdk.services.s3.S3AsyncClient;
import software.amazon.awssdk.services.s3.model.DeleteObjectRequest;
public class ProfileGrpcService extends ReactorProfileGrpc.ProfileImplBase {
@ -60,13 +58,11 @@ public class ProfileGrpcService extends ReactorProfileGrpc.ProfileImplBase {
private final ProfilesManager profilesManager;
private final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager;
private final Map<String, BadgeConfiguration> badgeConfigurationMap;
private final S3AsyncClient asyncS3client;
private final PostPolicyGenerator policyGenerator;
private final PolicySigner policySigner;
private final ProfileBadgeConverter profileBadgeConverter;
private final RateLimiters rateLimiters;
private final ServerZkProfileOperations zkProfileOperations;
private final String bucket;
private record AvatarData(Optional<String> currentAvatar,
Optional<String> finalAvatar,
@ -78,26 +74,22 @@ public class ProfileGrpcService extends ReactorProfileGrpc.ProfileImplBase {
final ProfilesManager profilesManager,
final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager,
final BadgesConfiguration badgesConfiguration,
final S3AsyncClient asyncS3client,
final PostPolicyGenerator policyGenerator,
final PolicySigner policySigner,
final ProfileBadgeConverter profileBadgeConverter,
final RateLimiters rateLimiters,
final ServerZkProfileOperations zkProfileOperations,
final String bucket) {
final ServerZkProfileOperations zkProfileOperations) {
this.clock = clock;
this.accountsManager = accountsManager;
this.profilesManager = profilesManager;
this.dynamicConfigurationManager = dynamicConfigurationManager;
this.badgeConfigurationMap = badgesConfiguration.getBadges().stream().collect(Collectors.toMap(
BadgeConfiguration::getId, Function.identity()));
this.asyncS3client = asyncS3client;
this.policyGenerator = policyGenerator;
this.policySigner = policySigner;
this.profileBadgeConverter = profileBadgeConverter;
this.rateLimiters = rateLimiters;
this.zkProfileOperations = zkProfileOperations;
this.bucket = bucket;
}
@Override
@ -157,10 +149,7 @@ public class ProfileGrpcService extends ReactorProfileGrpc.ProfileImplBase {
})));
if (request.getAvatarChange() != AvatarChange.AVATAR_CHANGE_UNCHANGED && avatarData.currentAvatar().isPresent()) {
updates.add(Mono.fromFuture(() -> asyncS3client.deleteObject(DeleteObjectRequest.builder()
.bucket(bucket)
.key(avatarData.currentAvatar().get())
.build())));
updates.add(Mono.fromFuture(() -> profilesManager.deleteAvatar(avatarData.currentAvatar.get())));
}
return profileSetMono.thenMany(Flux.merge(updates)).then(Mono.just(avatarData));
})

View File

@ -5,6 +5,8 @@
package org.whispersystems.textsecuregcm.storage;
import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.annotations.VisibleForTesting;
@ -15,6 +17,7 @@ import java.util.UUID;
import java.util.concurrent.CompletableFuture;
import java.util.function.Function;
import javax.annotation.Nullable;
import io.micrometer.core.instrument.Metrics;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.whispersystems.textsecuregcm.redis.FaultTolerantRedisClusterClient;
@ -36,8 +39,7 @@ public class ProfilesManager {
private final String bucket;
private final ObjectMapper mapper;
private static final CompletableFuture<?>[] EMPTY_FUTURE_ARRAY = new CompletableFuture[0];
private static final String DELETE_AVATAR_COUNTER_NAME = name(ProfilesManager.class, "deleteAvatar");
public ProfilesManager(final Profiles profiles, final FaultTolerantRedisClusterClient cacheCluster, final S3AsyncClient s3Client,
final String bucket) {
@ -63,19 +65,35 @@ public class ProfilesManager {
final CompletableFuture<Void> profilesAndAvatars = Mono.fromFuture(profiles.deleteAll(uuid))
.flatMapIterable(Function.identity())
.flatMap(avatar ->
Mono.fromFuture(s3Client.deleteObject(DeleteObjectRequest.builder()
.bucket(bucket)
.key(avatar)
.build()))
Mono.fromFuture(deleteAvatar(avatar))
// this is best-effort
.retry(3)
.onErrorComplete()
.then()
).then().toFuture();
.onErrorComplete())
.then().toFuture();
return CompletableFuture.allOf(redisDelete(uuid), profilesAndAvatars);
}
public CompletableFuture<Void> deleteAvatar(String avatar) {
return s3Client.deleteObject(DeleteObjectRequest.builder()
.bucket(bucket)
.key(avatar)
.build())
.handle((ignored, throwable) -> {
final String outcome;
if (throwable != null) {
logger.warn("Error deleting avatar", throwable);
outcome = "error";
} else {
outcome = "success";
}
Metrics.counter(DELETE_AVATAR_COUNTER_NAME, "outcome", outcome).increment();
return null;
})
.thenRun(Util.NOOP);
}
public Optional<VersionedProfile> get(UUID uuid, String version) {
Optional<VersionedProfile> profile = redisGet(uuid, version);

View File

@ -8,6 +8,7 @@ package org.whispersystems.textsecuregcm.controllers;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatNoException;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.refEq;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.clearInvocations;
@ -112,8 +113,6 @@ import org.whispersystems.textsecuregcm.util.SystemMapper;
import org.whispersystems.textsecuregcm.util.TestClock;
import org.whispersystems.textsecuregcm.util.TestRandomUtil;
import org.whispersystems.textsecuregcm.util.Util;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.DeleteObjectRequest;
@ExtendWith(DropwizardExtensionsSupport.class)
class ProfileControllerTest {
@ -125,7 +124,6 @@ class ProfileControllerTest {
private static final RateLimiter rateLimiter = mock(RateLimiter.class);
private static final RateLimiter usernameRateLimiter = mock(RateLimiter.class);
private static final S3Client s3client = mock(S3Client.class);
private static final PostPolicyGenerator postPolicyGenerator = new PostPolicyGenerator("us-west-1", "profile-bucket",
"accessKey");
private static final PolicySigner policySigner = new PolicySigner("accessSecret", "us-west-1");
@ -169,10 +167,8 @@ class ProfileControllerTest {
new BadgeConfiguration("TEST2", "testing", List.of("l", "m", "h", "x", "xx", "xxx"), "SVG", List.of(new BadgeSvg("sl", "sd"), new BadgeSvg("ml", "md"), new BadgeSvg("ll", "ld"))),
new BadgeConfiguration("TEST3", "testing", List.of("l", "m", "h", "x", "xx", "xxx"), "SVG", List.of(new BadgeSvg("sl", "sd"), new BadgeSvg("ml", "md"), new BadgeSvg("ll", "ld")))
), List.of("TEST1"), Map.of(1L, "TEST1", 2L, "TEST2", 3L, "TEST3")),
s3client,
postPolicyGenerator,
policySigner,
"profilesBucket",
serverSecretParams,
zkProfileOperations,
Executors.newSingleThreadExecutor()))
@ -180,7 +176,7 @@ class ProfileControllerTest {
@BeforeEach
void setup() {
reset(s3client);
reset(profilesManager);
clock.pin(Instant.ofEpochSecond(42));
AccountsHelper.setupMockUpdate(accountsManager);
@ -234,6 +230,8 @@ class ProfileControllerTest {
when(profilesManager.get(eq(AuthHelper.VALID_UUID_TWO), eq(versionHex("validversion")))).thenReturn(Optional.of(new VersionedProfile(
versionHex("validversion"), name, "profiles/validavatar", emoji, about, null, phoneNumberSharing, "validcommitment".getBytes())));
when(profilesManager.deleteAvatar(anyString())).thenReturn(CompletableFuture.completedFuture(null));
clearInvocations(rateLimiter);
clearInvocations(accountsManager);
clearInvocations(usernameRateLimiter);
@ -474,7 +472,7 @@ class ProfileControllerTest {
verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID), eq(versionHex("someversion")));
verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID), profileArgumentCaptor.capture());
verifyNoMoreInteractions(s3client);
verifyNoMoreInteractions(profilesManager);
assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize());
assertThat(profileArgumentCaptor.getValue().avatar()).isEqualTo(uploadAttributes.getKey());
@ -523,7 +521,7 @@ class ProfileControllerTest {
verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(versionHex("anotherversion")));
verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture());
verifyNoMoreInteractions(s3client);
verifyNoMoreInteractions(profilesManager);
assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize());
assertThat(profileArgumentCaptor.getValue().avatar()).isNull();
@ -552,7 +550,7 @@ class ProfileControllerTest {
verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(versionHex("validversion")));
verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture());
verify(s3client, times(1)).deleteObject(eq(DeleteObjectRequest.builder().bucket("profilesBucket").key("profiles/validavatar").build()));
verify(profilesManager, times(1)).deleteAvatar("profiles/validavatar");
assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize());
assertThat(profileArgumentCaptor.getValue().avatar()).startsWith("profiles/");
@ -581,7 +579,7 @@ class ProfileControllerTest {
verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(versionHex("validversion")));
verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture());
verify(s3client, times(1)).deleteObject(eq(DeleteObjectRequest.builder().bucket("profilesBucket").key("profiles/validavatar").build()));
verify(profilesManager, times(1)).deleteAvatar(eq("profiles/validavatar"));
assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize());
assertThat(profileArgumentCaptor.getValue().avatar()).isNull();
@ -611,7 +609,7 @@ class ProfileControllerTest {
verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(versionHex("validversion")));
verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture());
verify(s3client, never()).deleteObject(any(DeleteObjectRequest.class));
verify(profilesManager, never()).deleteAvatar(anyString());
assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize());
assertThat(profileArgumentCaptor.getValue().avatar()).isEqualTo("profiles/validavatar");
@ -639,7 +637,7 @@ class ProfileControllerTest {
verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(versionHex("validversion")));
verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture());
verify(s3client, times(1)).deleteObject(eq(DeleteObjectRequest.builder().bucket("profilesBucket").key("profiles/validavatar").build()));
verify(profilesManager, times(1)).deleteAvatar(eq("profiles/validavatar"));
assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize());
assertThat(profileArgumentCaptor.getValue().avatar()).isNull();
@ -669,7 +667,7 @@ class ProfileControllerTest {
verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID), eq(version));
verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID), profileArgumentCaptor.capture());
verify(s3client, never()).deleteObject(any(DeleteObjectRequest.class));
verify(profilesManager, never()).deleteAvatar(anyString());
assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize());
assertThat(profileArgumentCaptor.getValue().avatar()).isNull();
@ -700,7 +698,7 @@ class ProfileControllerTest {
verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(version));
verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture());
verify(s3client, times(1)).deleteObject(eq(DeleteObjectRequest.builder().bucket("profilesBucket").key("profiles/validavatar").build()));
verify(profilesManager, times(1)).deleteAvatar("profiles/validavatar");
assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize());
assertThat(profileArgumentCaptor.getValue().avatar()).startsWith("profiles/");
@ -738,7 +736,7 @@ class ProfileControllerTest {
verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(version));
verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture());
verifyNoMoreInteractions(s3client);
verifyNoMoreInteractions(profilesManager);
final VersionedProfile profile = profileArgumentCaptor.getValue();
assertThat(profile.commitment()).isEqualTo(commitment.serialize());
@ -778,7 +776,7 @@ class ProfileControllerTest {
verify(profilesManager).get(eq(AuthHelper.VALID_UUID_TWO), eq(version));
verify(profilesManager).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture());
verifyNoMoreInteractions(s3client);
verifyNoMoreInteractions(profilesManager);
final VersionedProfile profile = profileArgumentCaptor.getValue();
assertThat(profile.commitment()).isEqualTo(commitment.serialize());
@ -859,7 +857,7 @@ class ProfileControllerTest {
verify(profilesManager).get(eq(AuthHelper.VALID_UUID_TWO), eq(version));
verify(profilesManager).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture());
verifyNoMoreInteractions(s3client);
verifyNoMoreInteractions(profilesManager);
final VersionedProfile profile = profileArgumentCaptor.getValue();
assertThat(profile.commitment()).isEqualTo(commitment.serialize());
@ -902,7 +900,7 @@ class ProfileControllerTest {
verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(version));
verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture());
verifyNoMoreInteractions(s3client);
verifyNoMoreInteractions(profilesManager);
assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize());
assertThat(profileArgumentCaptor.getValue().avatar()).isNull();

View File

@ -17,9 +17,9 @@ import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.ArgumentMatchers.refEq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.when;
import static org.whispersystems.textsecuregcm.grpc.GrpcTestUtils.assertRateLimitExceeded;
import static org.whispersystems.textsecuregcm.grpc.GrpcTestUtils.assertStatusException;
@ -110,13 +110,9 @@ import org.whispersystems.textsecuregcm.util.MockUtils;
import org.whispersystems.textsecuregcm.util.TestRandomUtil;
import org.whispersystems.textsecuregcm.util.UUIDUtil;
import reactor.core.publisher.Mono;
import software.amazon.awssdk.services.s3.S3AsyncClient;
import software.amazon.awssdk.services.s3.model.DeleteObjectRequest;
public class ProfileGrpcServiceTest extends SimpleBaseGrpcTest<ProfileGrpcService, ProfileGrpc.ProfileBlockingStub> {
private static final String S3_BUCKET = "profileBucket";
private static final String VERSION = "someVersion";
private static final byte[] VALID_NAME = new byte[81];
@ -130,9 +126,6 @@ public class ProfileGrpcServiceTest extends SimpleBaseGrpcTest<ProfileGrpcServic
@Mock
private DynamicPaymentsConfiguration dynamicPaymentsConfiguration;
@Mock
private S3AsyncClient asyncS3client;
@Mock
private VersionedProfile profile;
@ -203,7 +196,7 @@ public class ProfileGrpcServiceTest extends SimpleBaseGrpcTest<ProfileGrpcServic
when(dynamicConfiguration.getPaymentsConfiguration()).thenReturn(dynamicPaymentsConfiguration);
when(dynamicPaymentsConfiguration.getDisallowedPrefixes()).thenReturn(Collections.emptyList());
when(asyncS3client.deleteObject(any(DeleteObjectRequest.class))).thenReturn(CompletableFuture.completedFuture(null));
when(profilesManager.deleteAvatar(anyString())).thenReturn(CompletableFuture.completedFuture(null));
clock = Clock.fixed(Instant.ofEpochSecond(42), ZoneId.of("Etc/UTC"));
@ -213,13 +206,11 @@ public class ProfileGrpcServiceTest extends SimpleBaseGrpcTest<ProfileGrpcServic
profilesManager,
dynamicConfigurationManager,
badgesConfiguration,
asyncS3client,
policyGenerator,
policySigner,
profileBadgeConverter,
rateLimiters,
serverZkProfileOperations,
S3_BUCKET
serverZkProfileOperations
);
}
@ -289,12 +280,9 @@ public class ProfileGrpcServiceTest extends SimpleBaseGrpcTest<ProfileGrpcServic
}
if (expectDeleteS3Object) {
verify(asyncS3client).deleteObject(DeleteObjectRequest.builder()
.bucket(S3_BUCKET)
.key(currentAvatar)
.build());
verify(profilesManager).deleteAvatar(currentAvatar);
} else {
verifyNoInteractions(asyncS3client);
verify(profilesManager, never()).deleteAvatar(anyString());
}
}

View File

@ -249,7 +249,9 @@ public class ProfilesManagerTest {
final String avatarTwo = "avatar2";
when(profiles.deleteAll(uuid)).thenReturn(CompletableFuture.completedFuture(List.of(avatarOne, avatarTwo)));
when(asyncCommands.del(ProfilesManager.getCacheKey(uuid))).thenReturn(MockRedisFuture.completedFuture(null));
when(s3Client.deleteObject(any(DeleteObjectRequest.class))).thenReturn(CompletableFuture.completedFuture(null));
when(s3Client.deleteObject(any(DeleteObjectRequest.class)))
.thenReturn(CompletableFuture.completedFuture(null))
.thenReturn(CompletableFuture.failedFuture(new RuntimeException("some error")));
profilesManager.deleteAll(uuid).join();