From 625637b888ad5fb9e5774de565dbedc3d0408ba0 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Thu, 3 Aug 2023 18:12:15 -0400 Subject: [PATCH] Stop checking for stored verification codes when linking devices --- service/config/sample.yml | 2 - .../textsecuregcm/WhisperServerService.java | 13 +-- .../configuration/DynamoDbTables.java | 9 -- .../controllers/DeviceController.java | 28 ++---- .../controllers/DeviceControllerTest.java | 90 +------------------ 5 files changed, 13 insertions(+), 129 deletions(-) diff --git a/service/config/sample.yml b/service/config/sample.yml index b8fc52a6b..d7906c676 100644 --- a/service/config/sample.yml +++ b/service/config/sample.yml @@ -112,8 +112,6 @@ dynamoDbTables: messages: tableName: Example_Messages expiration: P30D # Duration of time until rows expire - pendingDevices: - tableName: Example_PendingDevices phoneNumberIdentifiers: tableName: Example_PhoneNumberIdentifiers profiles: diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 69434b0d2..836320f02 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -115,14 +115,14 @@ import org.whispersystems.textsecuregcm.controllers.VerificationController; import org.whispersystems.textsecuregcm.currency.CoinMarketCapClient; import org.whispersystems.textsecuregcm.currency.CurrencyConversionManager; import org.whispersystems.textsecuregcm.currency.FixerClient; -import org.whispersystems.textsecuregcm.grpc.GrpcServerManagedWrapper; -import org.whispersystems.textsecuregcm.grpc.UserAgentInterceptor; import org.whispersystems.textsecuregcm.experiment.ExperimentEnrollmentManager; import org.whispersystems.textsecuregcm.filters.RemoteDeprecationFilter; import org.whispersystems.textsecuregcm.filters.RequestStatisticsFilter; import org.whispersystems.textsecuregcm.filters.TimestampResponseFilter; -import org.whispersystems.textsecuregcm.grpc.KeysGrpcService; +import org.whispersystems.textsecuregcm.grpc.GrpcServerManagedWrapper; import org.whispersystems.textsecuregcm.grpc.KeysAnonymousGrpcService; +import org.whispersystems.textsecuregcm.grpc.KeysGrpcService; +import org.whispersystems.textsecuregcm.grpc.UserAgentInterceptor; import org.whispersystems.textsecuregcm.limits.CardinalityEstimator; import org.whispersystems.textsecuregcm.limits.PushChallengeManager; import org.whispersystems.textsecuregcm.limits.RateLimitChallengeManager; @@ -189,9 +189,7 @@ import org.whispersystems.textsecuregcm.storage.RemoteConfigs; import org.whispersystems.textsecuregcm.storage.RemoteConfigsManager; import org.whispersystems.textsecuregcm.storage.ReportMessageDynamoDb; import org.whispersystems.textsecuregcm.storage.ReportMessageManager; -import org.whispersystems.textsecuregcm.storage.StoredVerificationCodeManager; import org.whispersystems.textsecuregcm.storage.SubscriptionManager; -import org.whispersystems.textsecuregcm.storage.VerificationCodeStore; import org.whispersystems.textsecuregcm.storage.VerificationSessionManager; import org.whispersystems.textsecuregcm.storage.VerificationSessions; import org.whispersystems.textsecuregcm.subscriptions.BraintreeManager; @@ -352,8 +350,6 @@ public class WhisperServerService extends Application maxDeviceConfiguration, final Clock clock) { - this.pendingDevices = pendingDevices; this.verificationTokenKey = new SecretKeySpec(linkDeviceSecret, VERIFICATION_TOKEN_ALGORITHM); this.accounts = accounts; this.messages = messages; @@ -202,8 +198,7 @@ public class DeviceController { @Context ContainerRequest containerRequest) throws RateLimitExceededException, DeviceLimitExceededException { - final Pair accountAndDevice = createDevice(authorizationHeader.getUsername(), - authorizationHeader.getPassword(), + final Pair accountAndDevice = createDevice(authorizationHeader.getPassword(), verificationCode, accountAttributes, containerRequest, @@ -237,8 +232,7 @@ public class DeviceController { @Context ContainerRequest containerRequest) throws RateLimitExceededException, DeviceLimitExceededException { - final Pair accountAndDevice = createDevice(authorizationHeader.getUsername(), - authorizationHeader.getPassword(), + final Pair accountAndDevice = createDevice(authorizationHeader.getPassword(), linkDeviceRequest.verificationCode(), linkDeviceRequest.accountAttributes(), containerRequest, @@ -362,28 +356,20 @@ public class DeviceController { return isDowngrade; } - private Pair createDevice(final String phoneNumber, - final String password, + private Pair createDevice(final String password, final String verificationCode, final AccountAttributes accountAttributes, final ContainerRequest containerRequest, final Optional maybeDeviceActivationRequest) throws RateLimitExceededException, DeviceLimitExceededException { - rateLimiters.getVerifyDeviceLimiter().validate(phoneNumber); - final Optional maybeAciFromToken = checkVerificationToken(verificationCode); final Account account = maybeAciFromToken.flatMap(accounts::getByAccountIdentifier) - .or(() -> { - final boolean verificationCodeValid = pendingDevices.getCodeForNumber(phoneNumber) - .map(storedVerificationCode -> storedVerificationCode.isValid(verificationCode)) - .orElse(false); - - return verificationCodeValid ? accounts.getByE164(phoneNumber) : Optional.empty(); - }) .orElseThrow(ForbiddenException::new); + rateLimiters.getVerifyDeviceLimiter().validate(account.getUuid()); + maybeDeviceActivationRequest.ifPresent(deviceActivationRequest -> { assert deviceActivationRequest.aciSignedPreKey().isPresent(); assert deviceActivationRequest.pniSignedPreKey().isPresent(); @@ -468,8 +454,6 @@ public class DeviceController { a.addDevice(device); }); - pendingDevices.remove(phoneNumber); - if (maybeAciFromToken.isPresent()) { usedTokenCluster.useCluster(connection -> connection.sync().set(getUsedTokenKey(verificationCode), "", new SetArgs().ex(TOKEN_EXPIRATION_DURATION))); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/DeviceControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/DeviceControllerTest.java index d71c6a6d0..43eaf680d 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/DeviceControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/DeviceControllerTest.java @@ -25,7 +25,6 @@ import org.signal.libsignal.protocol.ecc.Curve; import org.signal.libsignal.protocol.ecc.ECKeyPair; import org.whispersystems.textsecuregcm.auth.AuthenticatedAccount; import org.whispersystems.textsecuregcm.auth.DisabledPermittedAuthenticatedAccount; -import org.whispersystems.textsecuregcm.auth.StoredVerificationCode; import org.whispersystems.textsecuregcm.auth.WebsocketRefreshApplicationEventListener; import org.whispersystems.textsecuregcm.entities.*; import org.whispersystems.textsecuregcm.limits.RateLimiter; @@ -34,6 +33,8 @@ import org.whispersystems.textsecuregcm.mappers.DeviceLimitExceededExceptionMapp import org.whispersystems.textsecuregcm.push.ClientPresenceManager; import org.whispersystems.textsecuregcm.storage.*; import org.whispersystems.textsecuregcm.storage.Device.DeviceCapabilities; +import org.whispersystems.textsecuregcm.storage.KeysManager; +import org.whispersystems.textsecuregcm.storage.MessagesManager; import org.whispersystems.textsecuregcm.tests.util.AccountsHelper; import org.whispersystems.textsecuregcm.tests.util.AuthHelper; import org.whispersystems.textsecuregcm.tests.util.KeysHelper; @@ -61,7 +62,6 @@ import static org.mockito.Mockito.*; @ExtendWith(DropwizardExtensionsSupport.class) class DeviceControllerTest { - private static StoredVerificationCodeManager pendingDevicesManager = mock(StoredVerificationCodeManager.class); private static AccountsManager accountsManager = mock(AccountsManager.class); private static MessagesManager messagesManager = mock(MessagesManager.class); private static KeysManager keysManager = mock(KeysManager.class); @@ -75,7 +75,7 @@ class DeviceControllerTest { private static Map deviceConfiguration = new HashMap<>(); private static TestClock testClock = TestClock.now(); - private static DeviceController deviceController = new DeviceController(pendingDevicesManager, + private static DeviceController deviceController = new DeviceController( generateLinkDeviceSecret(), accountsManager, messagesManager, @@ -125,7 +125,6 @@ class DeviceControllerTest { when(account.isGiftBadgesSupported()).thenReturn(true); when(account.isPaymentActivationSupported()).thenReturn(false); - when(pendingDevicesManager.getCodeForNumber(any())).thenReturn(Optional.empty()); when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID)).thenReturn(Optional.of(account)); when(accountsManager.getByE164(AuthHelper.VALID_NUMBER)).thenReturn(Optional.of(account)); when(accountsManager.getByE164(AuthHelper.VALID_NUMBER_TWO)).thenReturn(Optional.of(maxedAccount)); @@ -139,7 +138,6 @@ class DeviceControllerTest { @AfterEach void teardown() { reset( - pendingDevicesManager, accountsManager, messagesManager, keysManager, @@ -179,36 +177,10 @@ class DeviceControllerTest { assertThat(response.getDeviceId()).isEqualTo(42L); - verify(pendingDevicesManager).remove(AuthHelper.VALID_NUMBER); verify(messagesManager).clear(eq(AuthHelper.VALID_UUID), eq(42L)); verify(commands).set(anyString(), anyString(), any()); } - @Test - void validDeviceRegisterTestStoredCode() { - final Device existingDevice = mock(Device.class); - when(existingDevice.getId()).thenReturn(Device.MASTER_ID); - when(account.getDevices()).thenReturn(List.of(existingDevice)); - - final String storedCode = "5678901"; - - when(pendingDevicesManager.getCodeForNumber(AuthHelper.VALID_NUMBER)).thenReturn( - Optional.of(new StoredVerificationCode(storedCode, System.currentTimeMillis(), null, null))); - - final DeviceResponse response = resources.getJerseyTest() - .target("/v1/devices/" + storedCode) - .request() - .header("Authorization", AuthHelper.getProvisioningAuthHeader(AuthHelper.VALID_NUMBER, "password1")) - .put(Entity.entity(new AccountAttributes(false, 1234, null, - null, true, null), - MediaType.APPLICATION_JSON_TYPE), - DeviceResponse.class); - - assertThat(response.getDeviceId()).isEqualTo(42L); - - verify(commands, never()).set(anyString(), anyString(), any()); - } - @Test void validDeviceRegisterTestSignedTokenUsed() { when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID)).thenReturn(Optional.of(account)); @@ -339,7 +311,6 @@ class DeviceControllerTest { expectedGcmToken.ifPresentOrElse(expectedToken -> assertEquals(expectedToken, device.getGcmId()), () -> assertNull(device.getGcmId())); - verify(pendingDevicesManager).remove(AuthHelper.VALID_NUMBER); verify(messagesManager).clear(eq(AuthHelper.VALID_UUID), eq(42L)); verify(keysManager).storeEcSignedPreKeys(AuthHelper.VALID_UUID, Map.of(response.getDeviceId(), aciSignedPreKey.get())); verify(keysManager).storeEcSignedPreKeys(AuthHelper.VALID_PNI, Map.of(response.getDeviceId(), pniSignedPreKey.get())); @@ -349,61 +320,6 @@ class DeviceControllerTest { } - @ParameterizedTest - @MethodSource("linkDeviceAtomic") - @SuppressWarnings("OptionalUsedAsFieldOrParameterType") - void linkDeviceAtomicWithStoredCode(final boolean fetchesMessages, - final Optional apnRegistrationId, - final Optional gcmRegistrationId, - final Optional expectedApnsToken, - final Optional expectedApnsVoipToken, - final Optional expectedGcmToken) { - - when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID)).thenReturn(Optional.of(account)); - - final Device existingDevice = mock(Device.class); - when(existingDevice.getId()).thenReturn(Device.MASTER_ID); - when(AuthHelper.VALID_ACCOUNT.getDevices()).thenReturn(List.of(existingDevice)); - - final Optional aciSignedPreKey; - final Optional pniSignedPreKey; - final Optional aciPqLastResortPreKey; - final Optional pniPqLastResortPreKey; - - final ECKeyPair aciIdentityKeyPair = Curve.generateKeyPair(); - final ECKeyPair pniIdentityKeyPair = Curve.generateKeyPair(); - - aciSignedPreKey = Optional.of(KeysHelper.signedECPreKey(1, aciIdentityKeyPair)); - pniSignedPreKey = Optional.of(KeysHelper.signedECPreKey(2, pniIdentityKeyPair)); - aciPqLastResortPreKey = Optional.of(KeysHelper.signedKEMPreKey(3, aciIdentityKeyPair)); - pniPqLastResortPreKey = Optional.of(KeysHelper.signedKEMPreKey(4, pniIdentityKeyPair)); - - when(account.getIdentityKey()).thenReturn(new IdentityKey(aciIdentityKeyPair.getPublicKey())); - when(account.getPhoneNumberIdentityKey()).thenReturn(new IdentityKey(pniIdentityKeyPair.getPublicKey())); - - when(keysManager.storeEcSignedPreKeys(any(), any())).thenReturn(CompletableFuture.completedFuture(null)); - when(keysManager.storePqLastResort(any(), any())).thenReturn(CompletableFuture.completedFuture(null)); - - final String storedCode = "5678901"; - - when(pendingDevicesManager.getCodeForNumber(AuthHelper.VALID_NUMBER)).thenReturn( - Optional.of(new StoredVerificationCode(storedCode, System.currentTimeMillis(), null, null))); - - final LinkDeviceRequest request = new LinkDeviceRequest(storedCode, - new AccountAttributes(fetchesMessages, 1234, null, null, true, null), - new DeviceActivationRequest(aciSignedPreKey, pniSignedPreKey, aciPqLastResortPreKey, pniPqLastResortPreKey, apnRegistrationId, gcmRegistrationId)); - - final DeviceResponse response = resources.getJerseyTest() - .target("/v1/devices/link") - .request() - .header("Authorization", AuthHelper.getProvisioningAuthHeader(AuthHelper.VALID_NUMBER, "password1")) - .put(Entity.entity(request, MediaType.APPLICATION_JSON_TYPE), DeviceResponse.class); - - assertThat(response.getDeviceId()).isEqualTo(42L); - - verify(commands, never()).set(anyString(), anyString(), any()); - } - private static Stream linkDeviceAtomic() { final String apnsToken = "apns-token"; final String apnsVoipToken = "apns-voip-token";