diff --git a/integration-tests/src/main/java/org/signal/integration/IntegrationTools.java b/integration-tests/src/main/java/org/signal/integration/IntegrationTools.java index b3b30b5ec..c7409eb64 100644 --- a/integration-tests/src/main/java/org/signal/integration/IntegrationTools.java +++ b/integration-tests/src/main/java/org/signal/integration/IntegrationTools.java @@ -18,6 +18,7 @@ import org.whispersystems.textsecuregcm.storage.RegistrationRecoveryPasswords; import org.whispersystems.textsecuregcm.storage.RegistrationRecoveryPasswordsManager; import org.whispersystems.textsecuregcm.storage.VerificationSessionManager; import org.whispersystems.textsecuregcm.storage.VerificationSessions; +import org.whispersystems.textsecuregcm.util.Util; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; import software.amazon.awssdk.services.dynamodb.DynamoDbAsyncClient; @@ -62,7 +63,8 @@ public class IntegrationTools { public CompletableFuture populateRecoveryPassword(final String phoneNumber, final byte[] password) { return phoneNumberIdentifiers .getPhoneNumberIdentifier(phoneNumber) - .thenCompose(pni -> registrationRecoveryPasswordsManager.store(pni, password)); + .thenCompose(pni -> registrationRecoveryPasswordsManager.store(pni, password)) + .thenRun(Util.NOOP); } public CompletableFuture> peekVerificationSessionPushChallenge(final String sessionId) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManager.java index ab0d8f9a4..f70fd97b6 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManager.java @@ -153,7 +153,7 @@ public class RegistrationLockVerificationManager { // This allows users to re-register via registration recovery password // instead of always being forced to fall back to SMS verification. if (!phoneVerificationType.equals(PhoneVerificationRequest.VerificationType.RECOVERY_PASSWORD) || clientRegistrationLock != null) { - registrationRecoveryPasswordsManager.remove(updatedAccount.getIdentifier(IdentityType.PNI)); + registrationRecoveryPasswordsManager.remove(updatedAccount.getIdentifier(IdentityType.PNI)).join(); } final List deviceIds = updatedAccount.getDevices().stream().map(Device::getId).toList(); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java index dd05ef4ab..2a1005467 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -5,6 +5,9 @@ package org.whispersystems.textsecuregcm.controllers; import io.dropwizard.auth.Auth; +import io.micrometer.core.instrument.Metrics; +import io.micrometer.core.instrument.Tag; +import io.micrometer.core.instrument.Tags; import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.Parameter; import io.swagger.v3.oas.annotations.media.Schema; @@ -56,6 +59,7 @@ import org.whispersystems.textsecuregcm.identity.IdentityType; import org.whispersystems.textsecuregcm.identity.ServiceIdentifier; import org.whispersystems.textsecuregcm.limits.RateLimitedByIp; import org.whispersystems.textsecuregcm.limits.RateLimiters; +import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil; import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.Device; @@ -67,6 +71,8 @@ import org.whispersystems.textsecuregcm.util.HeaderUtils; import org.whispersystems.textsecuregcm.util.UsernameHashZkProofVerifier; import org.whispersystems.textsecuregcm.util.Util; +import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name; + @SuppressWarnings("OptionalUsedAsFieldOrParameterType") @Path("/v1/accounts") @io.swagger.v3.oas.annotations.tags.Tag(name = "Account") @@ -75,6 +81,10 @@ public class AccountController { public static final int USERNAME_HASH_LENGTH = 32; public static final int MAXIMUM_USERNAME_CIPHERTEXT_LENGTH = 128; + private static final String RECOVERY_PASSWORD_SET_COUNTER_NAME = + name(AccountController.class, "recoveryPasswordSet"); + + private final AccountsManager accounts; private final RateLimiters rateLimiters; private final RegistrationRecoveryPasswordsManager registrationRecoveryPasswordsManager; @@ -262,8 +272,15 @@ public class AccountController { }); // if registration recovery password was sent to us, store it (or refresh its expiration) - attributes.recoveryPassword().ifPresent(registrationRecoveryPassword -> - registrationRecoveryPasswordsManager.store(updatedAccount.getIdentifier(IdentityType.PNI), registrationRecoveryPassword)); + attributes.recoveryPassword().ifPresent(registrationRecoveryPassword -> { + final boolean rrpCreated = registrationRecoveryPasswordsManager + .store(updatedAccount.getIdentifier(IdentityType.PNI), registrationRecoveryPassword) + .join(); + Metrics.counter(RECOVERY_PASSWORD_SET_COUNTER_NAME, Tags.of( + UserAgentTagUtil.getPlatformTag(userAgent), + Tag.of("outcome", rrpCreated ? "created" : "updated"))) + .increment(); + }); } @GET diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/SecureValueRecovery2Controller.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/SecureValueRecovery2Controller.java index 4bca684f6..6700fd98d 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/SecureValueRecovery2Controller.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/SecureValueRecovery2Controller.java @@ -20,6 +20,7 @@ import jakarta.ws.rs.Path; import jakarta.ws.rs.Produces; import jakarta.ws.rs.core.MediaType; import java.time.Clock; +import java.time.Duration; import java.util.List; import java.util.Optional; import java.util.concurrent.TimeUnit; @@ -41,7 +42,7 @@ import org.whispersystems.textsecuregcm.storage.AccountsManager; @Schema(description = "Note: /v2/backup is deprecated. Use /v2/svr instead.") public class SecureValueRecovery2Controller { - private static final long MAX_AGE_SECONDS = TimeUnit.DAYS.toSeconds(30); + public static final Duration MAX_AGE = Duration.ofDays(30); public static ExternalServiceCredentialsGenerator credentialsGenerator(final SecureValueRecoveryConfiguration cfg) { return credentialsGenerator(cfg, Clock.systemUTC()); @@ -105,7 +106,7 @@ public class SecureValueRecovery2Controller { final List credentials = ExternalServiceCredentialsSelector.check( request.tokens(), backupServiceCredentialGenerator, - MAX_AGE_SECONDS); + MAX_AGE.getSeconds()); // the username associated with the provided number final Optional matchingUsername = accountsManager diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VerificationController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VerificationController.java index 35917a53d..1e5817fc2 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VerificationController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/VerificationController.java @@ -50,16 +50,19 @@ import java.security.MessageDigest; import java.security.SecureRandom; import java.time.Clock; import java.time.Duration; +import java.time.Instant; import java.util.ArrayList; import java.util.Base64; import java.util.Collections; import java.util.HexFormat; import java.util.List; +import java.util.Locale; import java.util.Objects; import java.util.Optional; import java.util.concurrent.CancellationException; import java.util.concurrent.CompletionException; import java.util.concurrent.TimeUnit; +import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.Strings; import org.apache.http.HttpStatus; import org.slf4j.Logger; @@ -76,6 +79,7 @@ import org.whispersystems.textsecuregcm.entities.VerificationSessionResponse; import org.whispersystems.textsecuregcm.filters.RemoteAddressFilter; import org.whispersystems.textsecuregcm.limits.RateLimiters; import org.whispersystems.textsecuregcm.mappers.RegistrationServiceSenderExceptionMapper; +import org.whispersystems.textsecuregcm.metrics.DevicePlatformUtil; import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil; import org.whispersystems.textsecuregcm.push.PushNotification; import org.whispersystems.textsecuregcm.push.PushNotificationManager; @@ -89,7 +93,9 @@ import org.whispersystems.textsecuregcm.registration.TransportNotAllowedExceptio import org.whispersystems.textsecuregcm.registration.VerificationSession; import org.whispersystems.textsecuregcm.spam.RegistrationFraudChecker; import org.whispersystems.textsecuregcm.spam.RegistrationFraudChecker.VerificationCheck; +import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountsManager; +import org.whispersystems.textsecuregcm.storage.Device; import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; import org.whispersystems.textsecuregcm.storage.PhoneNumberIdentifiers; import org.whispersystems.textsecuregcm.storage.RegistrationRecoveryPasswordsManager; @@ -101,6 +107,7 @@ import org.whispersystems.textsecuregcm.util.ExceptionUtils; import org.whispersystems.textsecuregcm.util.ObsoletePhoneNumberFormatException; import org.whispersystems.textsecuregcm.util.Pair; import org.whispersystems.textsecuregcm.util.Util; +import org.whispersystems.textsecuregcm.util.ua.ClientPlatform; @Path("/v1/verification") @io.swagger.v3.oas.annotations.tags.Tag(name = "Verification") @@ -123,6 +130,10 @@ public class VerificationController { private static final String VERIFICATION_TRANSPORT_TAG_NAME = "transport"; private static final String VERIFIED_COUNTER_NAME = name(VerificationController.class, "verified"); private static final String SUCCESS_TAG_NAME = "success"; + private static final String RECOVERY_PASSWORD_REMOVED_TAG_NAME = "recoveryPasswordRemoved"; + private static final String REREGISTRATION_TAG_NAME = "reregistration"; + private static final String EXISTING_ACCOUNT_PLATFORM = "existingAccountPlatform"; + private static final String EXISTING_ACCOUNT_RECENTLY_SEEN_TAG_NAME = "existingAccountRecentlySeen"; private final RegistrationServiceClient registrationServiceClient; private final VerificationSessionManager verificationSessionManager; @@ -770,16 +781,43 @@ public class VerificationController { } } + boolean existingRRP = false; if (resultSession.verified()) { - registrationRecoveryPasswordsManager.remove(phoneNumberIdentifiers.getPhoneNumberIdentifier(registrationServiceSession.number()).join()); + existingRRP = registrationRecoveryPasswordsManager.remove(phoneNumberIdentifiers.getPhoneNumberIdentifier(registrationServiceSession.number()).join()).join(); } - Metrics.counter(VERIFIED_COUNTER_NAME, Tags.of( - UserAgentTagUtil.getPlatformTag(userAgent), - Tag.of(COUNTRY_CODE_TAG_NAME, Util.getCountryCode(registrationServiceSession.number())), - Tag.of(REGION_CODE_TAG_NAME, Util.getRegion(registrationServiceSession.number())), - Tag.of(SUCCESS_TAG_NAME, Boolean.toString(resultSession.verified())))) - .increment(); + Optional maybeExistingAccount; + try { + maybeExistingAccount = accountsManager.getByE164(registrationServiceSession.number()); + } catch (RuntimeException e) { + // Only for metrics, so it's ok if we fail to lookup the account + maybeExistingAccount = Optional.empty(); + } + + Tags tags = Tags.of( + UserAgentTagUtil.getPlatformTag(userAgent), + Tag.of(COUNTRY_CODE_TAG_NAME, Util.getCountryCode(registrationServiceSession.number())), + Tag.of(REGION_CODE_TAG_NAME, Util.getRegion(registrationServiceSession.number())), + Tag.of(SUCCESS_TAG_NAME, Boolean.toString(resultSession.verified())), + Tag.of(RECOVERY_PASSWORD_REMOVED_TAG_NAME, Boolean.toString(existingRRP)), + Tag.of(REREGISTRATION_TAG_NAME, Boolean.toString(maybeExistingAccount.isPresent()))); + + if (maybeExistingAccount.isPresent()) { + final Account existingAccount = maybeExistingAccount.get(); + final Duration timeSinceLastSeen = + Duration.between(Instant.ofEpochMilli(existingAccount.getLastSeen()), clock.instant()); + // Clients may reuse recent SVR2 credentials to authenticate via recovery password instead of SMS verification. + // If this is a re-registering account, check if the client could possibly have an unexpired SVR2 credential. + final boolean recentlySeen = timeSinceLastSeen.compareTo(SecureValueRecovery2Controller.MAX_AGE) < 0; + final String existingPlatform = DevicePlatformUtil + .getDevicePlatform(existingAccount.getPrimaryDevice()) + .map(ClientPlatform::name).orElse("unknown"); + tags = tags + .and(EXISTING_ACCOUNT_PLATFORM, existingPlatform) + .and(EXISTING_ACCOUNT_RECENTLY_SEEN_TAG_NAME, Boolean.toString(recentlySeen)); + } + + Metrics.counter(VERIFIED_COUNTER_NAME, tags).increment(); return buildResponse(resultSession, verificationSession); } 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 2363cec00..9294f3116 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -109,11 +109,12 @@ public class AccountsManager extends RedisPubSubAdapter implemen private static final Timer redisUuidGetTimer = Metrics.timer(name(AccountsManager.class, "redisUuidGet")); private static final Timer redisDeleteTimer = Metrics.timer(name(AccountsManager.class, "redisDelete")); - private static final String CREATE_COUNTER_NAME = name(AccountsManager.class, "createCounter"); - private static final String DELETE_COUNTER_NAME = name(AccountsManager.class, "deleteCounter"); - private static final String COUNTRY_CODE_TAG_NAME = "country"; - private static final String DELETION_REASON_TAG_NAME = "reason"; - private static final String REGISTRATION_ID_BASED_TRANSFER_ARCHIVE_KEY_COUNTER_NAME = name(AccountsManager.class,"registrationIdRedisKeyCounter"); + private static final String CREATE_COUNTER_NAME = name(AccountsManager.class, "createCounter"); + private static final String DELETE_COUNTER_NAME = name(AccountsManager.class, "deleteCounter"); + private static final String COUNTRY_CODE_TAG_NAME = "country"; + private static final String DELETION_REASON_TAG_NAME = "reason"; + private static final String REGISTRATION_ID_BASED_TRANSFER_ARCHIVE_KEY_COUNTER_NAME = + name(AccountsManager.class, "registrationIdRedisKeyCounter"); private static final String RETRY_NAME = ResilienceUtil.name(AccountsManager.class); @@ -172,7 +173,6 @@ public class AccountsManager extends RedisPubSubAdapter implemen .writer(SystemMapper.excludingField(Account.class, List.of("uuid"))); private static final Duration MESSAGE_POLL_INTERVAL = Duration.ofSeconds(1); - private static final Duration MAX_SERVER_CLOCK_DRIFT = Duration.ofSeconds(5); // An account that's used at least daily will get reset in the cache at least once per day when its "last seen" // timestamp updates; expiring entries after two days will help clear out "zombie" cache entries that are read @@ -421,23 +421,28 @@ public class AccountsManager extends RedisPubSubAdapter implemen redisSet(account); + final boolean rrpCreated = accountAttributes.recoveryPassword().map(registrationRecoveryPassword -> + registrationRecoveryPasswordsManager + .store(account.getIdentifier(IdentityType.PNI), registrationRecoveryPassword) + .join()) + .orElse(false); + + Tags tags = Tags.of(UserAgentTagUtil.getPlatformTag(userAgent), Tag.of("type", accountCreationType), Tag.of("hasPushToken", String.valueOf( primaryDeviceSpec.apnRegistrationId().isPresent() || primaryDeviceSpec.gcmRegistrationId() .isPresent())), - Tag.of("pushTokenType", pushTokenType)); + Tag.of("pushTokenType", pushTokenType), + Tag.of("hasRecoveryPassword", String.valueOf(accountAttributes.recoveryPassword().isPresent()))); if (StringUtils.isNotBlank(previousPushTokenType)) { tags = tags.and(Tag.of("previousPushTokenType", previousPushTokenType)); } - + if (accountAttributes.recoveryPassword().isPresent()) { + tags = tags.and(Tag.of("recoveryPasswordOutcome", rrpCreated ? "created" : "updated")); + } Metrics.counter(CREATE_COUNTER_NAME, tags).increment(); - - accountAttributes.recoveryPassword().ifPresent(registrationRecoveryPassword -> - registrationRecoveryPasswordsManager.store(account.getIdentifier(IdentityType.PNI), - registrationRecoveryPassword)); - return account; } @@ -1028,7 +1033,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen MAX_UPDATE_ATTEMPTS); }) .thenCompose(updatedAccount -> redisSetAsync(updatedAccount).thenApply(ignored -> updatedAccount)) - .whenComplete((ignored, throwable) -> timerSample.stop(updateTimer)); + .whenComplete((_, _) -> timerSample.stop(updateTimer)); } private Account updateWithRetries(Account account, @@ -1329,7 +1334,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen return resolveFromRedis.get() .thenCompose(maybeAccountFromRedis -> maybeAccountFromRedis - .map(accountFromRedis -> CompletableFuture.completedFuture(maybeAccountFromRedis)) + .map(_ -> CompletableFuture.completedFuture(maybeAccountFromRedis)) .orElseGet(() -> resolveFromAccounts.get() .thenCompose(maybeAccountFromAccounts -> maybeAccountFromAccounts .map(account -> redisSetAsync(account) @@ -1339,7 +1344,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen })) .thenApply(ignored -> maybeAccountFromAccounts)) .orElseGet(() -> CompletableFuture.completedFuture(maybeAccountFromAccounts))))) - .whenComplete((ignored, throwable) -> sample.stop(overallTimer)); + .whenComplete((_, _) -> sample.stop(overallTimer)); } private Optional redisGetBySecondaryKey(final String secondaryKey, final Timer timer) { @@ -1373,7 +1378,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen logger.warn("Failed to retrieve account from Redis", throwable); return Optional.empty(); }) - .whenComplete((ignored, throwable) -> sample.stop(timer)) + .whenComplete((_, _) -> sample.stop(timer)) .toCompletableFuture(); } @@ -1640,7 +1645,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen final CompletableFuture> future = new CompletableFuture<>(); future.completeOnTimeout(Optional.empty(), TimeUnit.MILLISECONDS.convert(timeout), TimeUnit.MILLISECONDS) - .whenComplete((maybeBackup, throwable) -> futureMap.remove(mapKey, future)); + .whenComplete((_, _) -> futureMap.remove(mapKey, future)); { final CompletableFuture> displacedFuture = futureMap.put(mapKey, future); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryPasswords.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryPasswords.java index 2c536ba7d..45fa0a6f2 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryPasswords.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryPasswords.java @@ -22,6 +22,7 @@ import software.amazon.awssdk.services.dynamodb.model.AttributeValue; import software.amazon.awssdk.services.dynamodb.model.DeleteItemRequest; import software.amazon.awssdk.services.dynamodb.model.GetItemRequest; import software.amazon.awssdk.services.dynamodb.model.PutItemRequest; +import software.amazon.awssdk.services.dynamodb.model.ReturnValue; public class RegistrationRecoveryPasswords { @@ -62,26 +63,36 @@ public class RegistrationRecoveryPasswords { .map(RegistrationRecoveryPasswords::saltedTokenHashFromItem)); } - public CompletableFuture addOrReplace(final UUID phoneNumberIdentifier, final SaltedTokenHash data) { + /// Add a PNI -> RRP mapping, or replace the current one if it already exists + /// + /// @param phoneNumberIdentifier The PNI to associate the salted RRP with + /// @param data The salted registration recovery password + /// @return true if a new mapping was added, false if an existing mapping was updated + public CompletableFuture addOrReplace(final UUID phoneNumberIdentifier, final SaltedTokenHash data) { final long expirationSeconds = expirationSeconds(); return asyncClient.putItem(PutItemRequest.builder() .tableName(tableName) + .returnValues(ReturnValue.ALL_OLD) .item(Map.of( KEY_PNI, AttributeValues.fromString(phoneNumberIdentifier.toString()), ATTR_EXP, AttributeValues.fromLong(expirationSeconds), ATTR_SALT, AttributeValues.fromString(data.salt()), ATTR_HASH, AttributeValues.fromString(data.hash()))) .build()) - .thenRun(Util.NOOP); + .thenApply(response -> response.attributes() == null || response.attributes().isEmpty()); } - public CompletableFuture removeEntry(final UUID phoneNumberIdentifier) { + /// Remove the entry associated with the provided PNI + /// + /// @return true if an entry was removed, false if no entry existed + public CompletableFuture removeEntry(final UUID phoneNumberIdentifier) { return asyncClient.deleteItem(DeleteItemRequest.builder() .tableName(tableName) + .returnValues(ReturnValue.ALL_OLD) .key(Map.of(KEY_PNI, AttributeValues.fromString(phoneNumberIdentifier.toString()))) .build()) - .thenRun(Util.NOOP); + .thenApply(response -> response.attributes() != null && !response.attributes().isEmpty()); } @VisibleForTesting diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryPasswordsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryPasswordsManager.java index 09e73a917..713095f15 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryPasswordsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryPasswordsManager.java @@ -30,7 +30,7 @@ public class RegistrationRecoveryPasswordsManager { public CompletableFuture verify(final UUID phoneNumberIdentifier, final byte[] password) { return registrationRecoveryPasswords.lookup(phoneNumberIdentifier) .thenApply(maybeHash -> maybeHash.filter(hash -> hash.verify(bytesToString(password)))) - .whenComplete((result, error) -> { + .whenComplete((_, error) -> { if (error != null) { logger.warn("Failed to lookup Registration Recovery Password", error); } @@ -38,25 +38,22 @@ public class RegistrationRecoveryPasswordsManager { .thenApply(Optional::isPresent); } - public CompletableFuture store(final UUID phoneNumberIdentifier, final byte[] password) { + public CompletableFuture store(final UUID phoneNumberIdentifier, final byte[] password) { final String token = bytesToString(password); final SaltedTokenHash tokenHash = SaltedTokenHash.generateFor(token); return registrationRecoveryPasswords.addOrReplace(phoneNumberIdentifier, tokenHash) - .whenComplete((result, error) -> { + .whenComplete((_, error) -> { if (error != null) { logger.warn("Failed to store Registration Recovery Password", error); } }); } - public CompletableFuture remove(final UUID phoneNumberIdentifier) { + public CompletableFuture remove(final UUID phoneNumberIdentifier) { return registrationRecoveryPasswords.removeEntry(phoneNumberIdentifier) - .whenComplete((ignored, error) -> { - if (error instanceof ResourceNotFoundException) { - // These will naturally happen if a recovery password is already deleted. Since we can remove - // the recovery password through many flows, we avoid creating log messages for these exceptions - } else if (error != null) { + .whenComplete((_, error) -> { + if (error != null) { logger.warn("Failed to remove Registration Recovery Password", error); } }); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManagerTest.java index 3171e43ba..cdf16b814 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManagerTest.java @@ -22,6 +22,7 @@ import static org.mockito.Mockito.when; import jakarta.ws.rs.WebApplicationException; import java.util.List; import java.util.UUID; +import java.util.concurrent.CompletableFuture; import java.util.function.Consumer; import java.util.stream.Stream; import javax.annotation.Nullable; @@ -94,6 +95,8 @@ class RegistrationLockVerificationManagerTest { when(account.hasLockedCredentials()).thenReturn(alreadyLocked); doThrow(new NotPushRegisteredException()).when(pushNotificationManager).sendAttemptLoginNotification(any(), any()); + when(registrationRecoveryPasswordsManager.remove(any())).thenReturn(CompletableFuture.completedFuture(true)); + final Pair, Consumer> exceptionType = switch (error) { case MISMATCH -> { when(existingRegistrationLock.verify(clientRegistrationLock)).thenReturn(false); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java index 5c15f054f..5a702a068 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java @@ -802,6 +802,8 @@ class AccountControllerTest { void testAccountsAttributesUpdateRecoveryPassword() { final byte[] recoveryPassword = TestRandomUtil.nextBytes(32); + when(registrationRecoveryPasswordsManager.store(any(), any())) + .thenReturn(CompletableFuture.completedFuture(true)); try (final Response response = resources.getJerseyTest() .target("/v1/accounts/attributes/") .request() diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/VerificationControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/VerificationControllerTest.java index e49be1980..9a02754ef 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/VerificationControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/VerificationControllerTest.java @@ -1394,6 +1394,8 @@ class VerificationControllerTest { .thenReturn(CompletableFuture.completedFuture( Optional.of(new VerificationSession(null, null, Collections.emptyList(), Collections.emptyList(), null, null, true, clock.millis(), clock.millis(), registrationServiceSession.expiration())))); + when(registrationRecoveryPasswordsManager.remove(any())) + .thenReturn(CompletableFuture.completedFuture(true)); final RegistrationServiceSession verifiedSession = new RegistrationServiceSession(SESSION_ID, NUMBER, true, null, null, 0L, diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryTest.java index c8c5433eb..92ab3b1bf 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryTest.java @@ -5,7 +5,6 @@ package org.whispersystems.textsecuregcm.storage; -import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -61,7 +60,7 @@ public class RegistrationRecoveryTest { @Test public void testLookupAfterWrite() throws Exception { - registrationRecoveryPasswords.addOrReplace(PNI, ORIGINAL_HASH).get(); + assertTrue(registrationRecoveryPasswords.addOrReplace(PNI, ORIGINAL_HASH).get()); final long initialExp = fetchTimestamp(PNI); final long expectedExpiration = CLOCK.instant().getEpochSecond() + EXPIRATION.getSeconds(); assertEquals(expectedExpiration, initialExp); @@ -90,8 +89,8 @@ public class RegistrationRecoveryTest { @Test public void testReplace() throws Exception { - registrationRecoveryPasswords.addOrReplace(PNI, ORIGINAL_HASH).get(); - registrationRecoveryPasswords.addOrReplace(PNI, ANOTHER_HASH).get(); + assertTrue(registrationRecoveryPasswords.addOrReplace(PNI, ORIGINAL_HASH).get()); + assertFalse(registrationRecoveryPasswords.addOrReplace(PNI, ANOTHER_HASH).get()); final Optional saltedTokenHashByPni = registrationRecoveryPasswords.lookup(PNI).get(); assertTrue(saltedTokenHashByPni.isPresent()); @@ -101,12 +100,12 @@ public class RegistrationRecoveryTest { @Test public void testRemove() throws Exception { - assertDoesNotThrow(() -> registrationRecoveryPasswords.removeEntry(PNI).join()); + assertFalse(registrationRecoveryPasswords.removeEntry(PNI).join()); registrationRecoveryPasswords.addOrReplace(PNI, ORIGINAL_HASH).get(); assertTrue(registrationRecoveryPasswords.lookup(PNI).get().isPresent()); - registrationRecoveryPasswords.removeEntry(PNI).get(); + assertTrue(registrationRecoveryPasswords.removeEntry(PNI).get()); assertTrue(registrationRecoveryPasswords.lookup(PNI).get().isEmpty()); }