Instrument registration recovery password modifications

2026-04-19 03:28:05 +01:00 · 2026-01-28 17:37:50 -05:00
parent 2a7e99e9f0
commit e6116044f8
12 changed files with 127 additions and 50 deletions
--- 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<Byte> deviceIds = updatedAccount.getDevices().stream().map(Device::getId).toList();
--- 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
--- 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<ExternalServiceCredentialsSelector.CredentialInfo> credentials = ExternalServiceCredentialsSelector.check(
        request.tokens(),
        backupServiceCredentialGenerator,
-        MAX_AGE_SECONDS);
+        MAX_AGE.getSeconds());

    // the username associated with the provided number
    final Optional<String> matchingUsername = accountsManager
--- 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<Account> 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);
  }
--- 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<String, String> 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<String, String> 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<String, String> 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<String, String> 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<String, String> 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<String, String> implemen
                        }))
                        .thenApply(ignored -> maybeAccountFromAccounts))
                    .orElseGet(() -> CompletableFuture.completedFuture(maybeAccountFromAccounts)))))
-        .whenComplete((ignored, throwable) -> sample.stop(overallTimer));
+        .whenComplete((_, _) -> sample.stop(overallTimer));
  }

  private Optional<Account> redisGetBySecondaryKey(final String secondaryKey, final Timer timer) {
@@ -1373,7 +1378,7 @@ public class AccountsManager extends RedisPubSubAdapter<String, String> 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<String, String> implemen
    final CompletableFuture<Optional<T>> 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<Optional<T>> displacedFuture = futureMap.put(mapKey, future);
--- 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<Void> 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<Boolean> 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<Void> 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<Boolean> 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
--- 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<Boolean> 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<Void> store(final UUID phoneNumberIdentifier, final byte[] password) {
+  public CompletableFuture<Boolean> 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<Void> remove(final UUID phoneNumberIdentifier) {
+  public CompletableFuture<Boolean> 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);
          }
        });