From 90c27f69693b68e09b302fdddada68dfa02f3a6c Mon Sep 17 00:00:00 2001 From: Chris Eager Date: Fri, 8 May 2026 16:29:16 -0500 Subject: [PATCH] Add post-registration change number waiting period --- .../WhisperServerConfiguration.java | 9 ++ .../textsecuregcm/WhisperServerService.java | 3 +- .../ChangeNumberConfiguration.java | 11 +++ .../controllers/AccountControllerV2.java | 2 +- .../storage/ChangeNumberManager.java | 33 +++++-- .../storage/ChangeNumberManagerTest.java | 96 ++++++++++++++++++- 6 files changed, 142 insertions(+), 12 deletions(-) create mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/configuration/ChangeNumberConfiguration.java diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java index 76e4dcd89..7f0dc2d0f 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java @@ -22,6 +22,7 @@ import org.whispersystems.textsecuregcm.configuration.BraintreeConfiguration; import org.whispersystems.textsecuregcm.configuration.CallQualitySurveyConfiguration; import org.whispersystems.textsecuregcm.configuration.Cdn3StorageManagerConfiguration; import org.whispersystems.textsecuregcm.configuration.CdnConfiguration; +import org.whispersystems.textsecuregcm.configuration.ChangeNumberConfiguration; import org.whispersystems.textsecuregcm.configuration.CircuitBreakerConfiguration; import org.whispersystems.textsecuregcm.configuration.ClientReleaseConfiguration; import org.whispersystems.textsecuregcm.configuration.DefaultAwsCredentialsFactory; @@ -355,6 +356,11 @@ public class WhisperServerConfiguration extends Configuration { @JsonProperty private CallQualitySurveyConfiguration callQualitySurvey; + @Valid + @NotNull + @JsonProperty + private ChangeNumberConfiguration changeNumber = new ChangeNumberConfiguration(Duration.ofHours(1)); + public TlsKeyStoreConfiguration getTlsKeyStoreConfiguration() { return tlsKeyStore; } @@ -591,4 +597,7 @@ public class WhisperServerConfiguration extends Configuration { return hlrLookup; } + public ChangeNumberConfiguration getChangeNumber() { + return changeNumber; + } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 8db4c8d03..2c6b2654d 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -1096,7 +1096,8 @@ public class WhisperServerService extends Application commonControllers = Lists.newArrayList( new AccountController(accountsManager, rateLimiters, registrationRecoveryPasswordsManager, diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/ChangeNumberConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/ChangeNumberConfiguration.java new file mode 100644 index 000000000..eb1ff2f54 --- /dev/null +++ b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/ChangeNumberConfiguration.java @@ -0,0 +1,11 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.whispersystems.textsecuregcm.configuration; + +import java.time.Duration; + +public record ChangeNumberConfiguration(Duration postRegistrationWaitingPeriod) { +} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java index 99b702207..d43734083 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java @@ -76,7 +76,7 @@ public class AccountControllerV2 { @ApiResponse(responseCode = "423", content = @Content(schema = @Schema(implementation = RegistrationLockFailure.class))) @ApiResponse(responseCode = "429", description = "Too many attempts", headers = @Header( name = "Retry-After", - description = "If present, an positive integer indicating the number of seconds before a subsequent attempt could succeed")) + description = "If present, a positive integer indicating the number of seconds before a subsequent attempt could succeed")) public AccountIdentityResponse changeNumber(@Auth final AuthenticatedDevice authenticatedDevice, @NotNull @Valid final ChangeNumberRequest request, @HeaderParam(HttpHeaders.USER_AGENT) final String userAgentString, diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManager.java index 9f9e28b1b..b9a15cd28 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManager.java @@ -4,17 +4,22 @@ */ package org.whispersystems.textsecuregcm.storage; -import java.time.Clock; -import java.util.List; -import java.util.Map; -import java.util.Optional; -import java.util.UUID; -import java.util.stream.Collectors; +import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name; + import com.google.common.net.HttpHeaders; import io.micrometer.core.instrument.Metrics; import io.micrometer.core.instrument.Tag; import io.micrometer.core.instrument.Tags; import jakarta.ws.rs.container.ContainerRequestContext; +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.UUID; +import java.util.stream.Collectors; +import javax.annotation.Nullable; import org.signal.libsignal.protocol.IdentityKey; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -36,9 +41,6 @@ import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil; import org.whispersystems.textsecuregcm.push.MessageSender; import org.whispersystems.textsecuregcm.push.MessageTooLargeException; import org.whispersystems.textsecuregcm.util.Pair; -import javax.annotation.Nullable; - -import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name; public class ChangeNumberManager { @@ -46,6 +48,7 @@ public class ChangeNumberManager { private final AccountsManager accountsManager; private final PhoneVerificationTokenManager phoneVerificationTokenManager; private final RegistrationLockVerificationManager registrationLockVerificationManager; + private final Duration postRegistrationWaitingPeriod; private final RateLimiters rateLimiters; private final Clock clock; @@ -54,6 +57,8 @@ public class ChangeNumberManager { // Note that this counter name references another class deliberately in the interest of metric continuity private static final String CHANGE_NUMBER_COUNTER_NAME = name(AccountControllerV2.class, "changeNumber"); private static final String VERIFICATION_TYPE_TAG_NAME = "verification"; + private static final String POST_REGISTRATION_WAITING_PERIOD_NOT_MET_COUNTER_NAME = name(ChangeNumberManager.class, + "postRegistrationWaitingPeriodNotMet"); public ChangeNumberManager( final MessageSender messageSender, @@ -61,6 +66,7 @@ public class ChangeNumberManager { final PhoneVerificationTokenManager phoneVerificationTokenManager, final RegistrationLockVerificationManager registrationLockVerificationManager, final RateLimiters rateLimiters, + final Duration postRegistrationWaitingPeriod, final Clock clock) { this.messageSender = messageSender; @@ -68,6 +74,7 @@ public class ChangeNumberManager { this.phoneVerificationTokenManager = phoneVerificationTokenManager; this.registrationLockVerificationManager = registrationLockVerificationManager; this.rateLimiters = rateLimiters; + this.postRegistrationWaitingPeriod = postRegistrationWaitingPeriod; this.clock = clock; } @@ -93,6 +100,14 @@ public class ChangeNumberManager { // Only verify and check reglock if there's a data change to be made... if (!account.getNumber().equals(number)) { + + final Instant registration = Instant.ofEpochMilli(account.getPrimaryDevice().getCreated()); + final Duration waitingPeriodRemaining = Duration.between(clock.instant().minus(postRegistrationWaitingPeriod), registration); + if (waitingPeriodRemaining.isPositive()) { + Metrics.counter(POST_REGISTRATION_WAITING_PERIOD_NOT_MET_COUNTER_NAME).increment(); + throw new RateLimitExceededException(waitingPeriodRemaining); + } + rateLimiters.getRegistrationLimiter().validate(number); final PhoneVerificationRequest.VerificationType verificationType = diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManagerTest.java index 4e70bb09d..d0ccabc4c 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManagerTest.java @@ -4,6 +4,7 @@ */ package org.whispersystems.textsecuregcm.storage; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyByte; @@ -22,6 +23,8 @@ import jakarta.ws.rs.WebApplicationException; import jakarta.ws.rs.container.ContainerRequestContext; import java.time.Duration; import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -30,6 +33,10 @@ import java.util.Optional; import java.util.UUID; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.mockito.stubbing.Answer; import org.signal.libsignal.protocol.IdentityKey; import org.signal.libsignal.protocol.ecc.ECKeyPair; @@ -67,6 +74,14 @@ public class ChangeNumberManagerTest { private static final TestClock CLOCK = TestClock.pinned(Instant.now()); + private static final Duration POST_REGISTRATION_WAITING_PERIOD = Duration.ofHours(2); + private static final Device DEFAULT_PRIMARY_DEVICE; + static { + DEFAULT_PRIMARY_DEVICE = new Device(); + DEFAULT_PRIMARY_DEVICE.setId((byte) 1); + DEFAULT_PRIMARY_DEVICE.setCreated(CLOCK.instant().minus(POST_REGISTRATION_WAITING_PERIOD).minusSeconds(1).toEpochMilli()); + } + @BeforeEach void setUp() throws Exception { messageSender = mock(MessageSender.class); @@ -87,7 +102,8 @@ public class ChangeNumberManagerTest { when(rateLimiters.getRegistrationLimiter()).thenReturn(rateLimiter); changeNumberManager = new ChangeNumberManager(messageSender, accountsManager, - phoneVerificationTokenManager, registrationLockVerificationManager, rateLimiters, CLOCK); + phoneVerificationTokenManager, registrationLockVerificationManager, rateLimiters, + POST_REGISTRATION_WAITING_PERIOD, CLOCK); updatedPhoneNumberIdentifiersByAccount = new HashMap<>(); @@ -100,6 +116,7 @@ public class ChangeNumberManagerTest { final UUID uuid = account.getIdentifier(IdentityType.ACI); final List devices = account.getDevices(); + final Device primaryDevice = account.getPrimaryDevice(); final UUID updatedPni = UUID.randomUUID(); updatedPhoneNumberIdentifiersByAccount.put(account, updatedPni); @@ -113,6 +130,7 @@ public class ChangeNumberManagerTest { when(updatedAccount.getNumber()).thenReturn(number); when(updatedAccount.getDevices()).thenReturn(devices); when(updatedAccount.getDevice(anyByte())).thenReturn(Optional.empty()); + when(updatedAccount.getPrimaryDevice()).thenReturn(primaryDevice); account.getDevices().forEach(device -> when(updatedAccount.getDevice(device.getId())).thenReturn(Optional.of(device))); @@ -145,6 +163,7 @@ public class ChangeNumberManagerTest { when(account.getIdentifier(IdentityType.ACI)).thenReturn(accountIdentifier); when(account.isIdentifiedBy(any())).thenReturn(false); when(account.isIdentifiedBy(new AciServiceIdentifier(accountIdentifier))).thenReturn(true); + when(account.getPrimaryDevice()).thenReturn(DEFAULT_PRIMARY_DEVICE); when(accountsManager.getAccountsForChangeNumber(eq(accountIdentifier), any())) .thenReturn(new Pair<>(account, Optional.empty())); @@ -187,6 +206,7 @@ public class ChangeNumberManagerTest { when(account.getDevice(primaryDeviceId)).thenReturn(Optional.of(primaryDevice)); when(account.getDevice(linkedDeviceId)).thenReturn(Optional.of(linkedDevice)); when(account.getDevices()).thenReturn(List.of(primaryDevice, linkedDevice)); + when(account.getPrimaryDevice()).thenReturn(DEFAULT_PRIMARY_DEVICE); final ECKeyPair pniIdentityKeyPair = ECKeyPair.generate(); final IdentityKey pniIdentityKey = new IdentityKey(pniIdentityKeyPair.getPublicKey()); @@ -269,6 +289,7 @@ public class ChangeNumberManagerTest { when(account.getIdentifier(IdentityType.ACI)).thenReturn(accountIdentifier); when(account.isIdentifiedBy(any())).thenReturn(false); when(account.isIdentifiedBy(new AciServiceIdentifier(accountIdentifier))).thenReturn(true); + when(account.getPrimaryDevice()).thenReturn(DEFAULT_PRIMARY_DEVICE); when(accountsManager.getAccountsForChangeNumber(eq(accountIdentifier), any())) .thenReturn(new Pair<>(account, Optional.empty())); @@ -308,6 +329,7 @@ public class ChangeNumberManagerTest { when(account.getIdentifier(IdentityType.ACI)).thenReturn(accountIdentifier); when(account.isIdentifiedBy(any())).thenReturn(false); when(account.isIdentifiedBy(new AciServiceIdentifier(accountIdentifier))).thenReturn(true); + when(account.getPrimaryDevice()).thenReturn(DEFAULT_PRIMARY_DEVICE); when(accountsManager.getAccountsForChangeNumber(eq(accountIdentifier), any())) .thenReturn(new Pair<>(account, Optional.empty())); @@ -356,6 +378,7 @@ public class ChangeNumberManagerTest { when(account.getIdentifier(IdentityType.ACI)).thenReturn(accountIdentifier); when(account.isIdentifiedBy(any())).thenReturn(false); when(account.isIdentifiedBy(new AciServiceIdentifier(accountIdentifier))).thenReturn(true); + when(account.getPrimaryDevice()).thenReturn(DEFAULT_PRIMARY_DEVICE); final Account existingAccount = mock(Account.class); when(existingAccount.getNumber()).thenReturn(targetNumber); @@ -381,4 +404,75 @@ public class ChangeNumberManagerTest { verify(accountsManager, never()).changeNumber(any(), any(), any(), any(), any(), any()); verify(messageSender, never()).sendMessages(any(), any(), any(), any(), any(), any()); } + + @ParameterizedTest + @MethodSource + void testRecentRegistration(final boolean expectRateLimited, final boolean sameNumber, final Instant registrationInstant) throws Throwable { + + final String originalNumber = PhoneNumberUtil.getInstance().format( + PhoneNumberUtil.getInstance().getExampleNumber("DE"), PhoneNumberUtil.PhoneNumberFormat.E164); + + final String targetNumber = sameNumber + ? originalNumber + : PhoneNumberUtil.getInstance().format( + PhoneNumberUtil.getInstance().getExampleNumber("US"), PhoneNumberUtil.PhoneNumberFormat.E164); + + final ECKeyPair pniIdentityKeyPair = ECKeyPair.generate(); + final IdentityKey pniIdentityKey = new IdentityKey(ECKeyPair.generate().getPublicKey()); + + final Map ecSignedPreKeys = + Map.of(Device.PRIMARY_ID, KeysHelper.signedECPreKey(1, pniIdentityKeyPair)); + + final Map kemLastResortPreKeys = + Map.of(Device.PRIMARY_ID, KeysHelper.signedKEMPreKey(2, pniIdentityKeyPair)); + + final UUID accountIdentifier = UUID.randomUUID(); + + final Account account = mock(Account.class); + when(account.getNumber()).thenReturn(originalNumber); + when(account.getIdentifier(IdentityType.ACI)).thenReturn(accountIdentifier); + when(account.isIdentifiedBy(any())).thenReturn(false); + when(account.isIdentifiedBy(new AciServiceIdentifier(accountIdentifier))).thenReturn(true); + + final Device primaryDevice = mock(Device.class); + when(account.getPrimaryDevice()).thenReturn(primaryDevice); + when(primaryDevice.getCreated()).thenReturn(registrationInstant.toEpochMilli()); + + when(accountsManager.getAccountsForChangeNumber(eq(accountIdentifier), any())) + .thenReturn(new Pair<>(account, Optional.empty())); + + final Executable changeNumberOperation = () -> changeNumberManager.changeNumber(accountIdentifier, + null, + null, + null, + targetNumber, + pniIdentityKey, + ecSignedPreKeys, + kemLastResortPreKeys, + Collections.emptyList(), + Collections.emptyMap(), + mock(ContainerRequestContext.class)); + if (expectRateLimited) { + final RateLimitExceededException e = assertThrows(RateLimitExceededException.class, changeNumberOperation); + + assertEquals(Duration.between(CLOCK.instant().minus(POST_REGISTRATION_WAITING_PERIOD), registrationInstant), e.getRetryDuration().orElseThrow()); + } else { + changeNumberOperation.execute(); + verify(accountsManager).changeNumber(accountIdentifier, targetNumber, pniIdentityKey, ecSignedPreKeys, kemLastResortPreKeys, Collections.emptyMap()); + } + } + + static Collection testRecentRegistration() { + // truncate to millis because that is the resolution for device.created + final Instant tooRecent = CLOCK.instant().minus(POST_REGISTRATION_WAITING_PERIOD).plusSeconds(1) + .truncatedTo(ChronoUnit.MILLIS); + final Instant outsideWaitingPeriod = CLOCK.instant().minus(POST_REGISTRATION_WAITING_PERIOD).minusSeconds(1) + .truncatedTo(ChronoUnit.MILLIS); + return List.of( + // expect exception, same number, registration instant + Arguments.argumentSet("waiting period elapsed", false, false, outsideWaitingPeriod), + Arguments.argumentSet("waiting period not elapsed", true, false, tooRecent), + Arguments.argumentSet("waiting period not elapsed; same number", false, true, tooRecent) + ); + } }