From 350682b83ae4db18344d7b15c883f25a5779ee3d Mon Sep 17 00:00:00 2001 From: Katherine Yen Date: Mon, 17 Apr 2023 10:30:36 -0700 Subject: [PATCH] Lock account and send notification when someone passes phone verification but fails reglock --- .../textsecuregcm/WhisperServerService.java | 2 +- .../RegistrationLockVerificationManager.java | 40 ++++++++++--- .../textsecuregcm/push/APNSender.java | 11 ++++ .../textsecuregcm/push/FcmSender.java | 1 + .../textsecuregcm/push/PushNotification.java | 6 +- .../push/PushNotificationManager.java | 16 +++++ .../RegistrationRecoveryPasswordsManager.java | 6 +- ...gistrationLockVerificationManagerTest.java | 58 +++++++++++++++++-- .../controllers/AccountControllerTest.java | 3 +- .../push/PushNotificationManagerTest.java | 33 +++++++++++ .../tests/util/AccountsHelper.java | 1 + 11 files changed, 160 insertions(+), 17 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 175790ca9..3d096e2aa 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -552,7 +552,7 @@ public class WhisperServerService extends Application deviceIds = updatedAccount.getDevices().stream().map(Device::getId).toList(); + // This will often be a no-op, since the recovery password is deleted when there's a verified session. + // However, this covers the case where a user re-registers with SMS bypass and then forgets their PIN. + registrationRecoveryPasswordsManager.removeForNumber(updatedAccount.getNumber()); + + final List deviceIds = updatedAccount.getDevices().stream().map(Device::getId).toList(); clientPresenceManager.disconnectAllPresences(updatedAccount.getUuid(), deviceIds); - */ - final ExternalServiceCredentials existingBackupCredentials = - backupServiceCredentialGenerator.generateForUuid(account.getUuid()); + + try { + // Send a push notification that prompts the client to attempt login and fail due to locked credentials + pushNotificationManager.sendAttemptLoginNotification(updatedAccount, "failedRegistrationLock"); + } catch (final NotPushRegisteredException e) { + Metrics.counter(CHALLENGED_DEVICE_NOT_PUSH_REGISTERED_COUNTER_NAME).increment(); + } throw new WebApplicationException(Response.status(FAILURE_HTTP_STATUS) .entity(new RegistrationLockFailure(existingRegistrationLock.getTimeRemaining().toMillis(), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/push/APNSender.java b/service/src/main/java/org/whispersystems/textsecuregcm/push/APNSender.java index 8eae39f2a..78c1bee76 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/push/APNSender.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/push/APNSender.java @@ -96,6 +96,17 @@ public class APNSender implements Managed, PushNotificationSender { } } + case ATTEMPT_LOGIN_NOTIFICATION_HIGH_PRIORITY -> new SimpleApnsPayloadBuilder() + .setMutableContent(true) + .setLocalizedAlertMessage("APN_Message") + .addCustomProperty("attemptLoginContext", notification.data()) + .build(); + + case ATTEMPT_LOGIN_NOTIFICATION_LOW_PRIORITY -> new SimpleApnsPayloadBuilder() + .setContentAvailable(true) + .addCustomProperty("attemptLoginContext", notification.data()) + .build(); + case CHALLENGE -> new SimpleApnsPayloadBuilder() .setSound("default") .setLocalizedAlertMessage("APN_Message") diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/push/FcmSender.java b/service/src/main/java/org/whispersystems/textsecuregcm/push/FcmSender.java index f13b72b28..5946067d3 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/push/FcmSender.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/push/FcmSender.java @@ -89,6 +89,7 @@ public class FcmSender implements PushNotificationSender { final String key = switch (pushNotification.notificationType()) { case NOTIFICATION -> "notification"; + case ATTEMPT_LOGIN_NOTIFICATION_HIGH_PRIORITY, ATTEMPT_LOGIN_NOTIFICATION_LOW_PRIORITY -> "attemptLoginContext"; case CHALLENGE -> "challenge"; case RATE_LIMIT_CHALLENGE -> "rateLimitChallenge"; }; diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/push/PushNotification.java b/service/src/main/java/org/whispersystems/textsecuregcm/push/PushNotification.java index 0b2297ffa..37cbda79e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/push/PushNotification.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/push/PushNotification.java @@ -18,7 +18,11 @@ public record PushNotification(String deviceToken, boolean urgent) { public enum NotificationType { - NOTIFICATION, CHALLENGE, RATE_LIMIT_CHALLENGE + NOTIFICATION, + ATTEMPT_LOGIN_NOTIFICATION_HIGH_PRIORITY, + @Deprecated ATTEMPT_LOGIN_NOTIFICATION_LOW_PRIORITY, // Temporary support for iOS clients; can be removed after 2023-06-12 + CHALLENGE, + RATE_LIMIT_CHALLENGE } public enum TokenType { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/push/PushNotificationManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/push/PushNotificationManager.java index c6c4d4816..df270395a 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/push/PushNotificationManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/push/PushNotificationManager.java @@ -78,6 +78,22 @@ public class PushNotificationManager { PushNotification.NotificationType.RATE_LIMIT_CHALLENGE, challengeToken, destination, device, true)); } + public void sendAttemptLoginNotification(final Account destination, final String context) throws NotPushRegisteredException { + final Device device = destination.getDevice(Device.MASTER_ID).orElseThrow(NotPushRegisteredException::new); + final Pair tokenAndType = getToken(device); + + sendNotification(new PushNotification(tokenAndType.first(), tokenAndType.second(), + PushNotification.NotificationType.ATTEMPT_LOGIN_NOTIFICATION_HIGH_PRIORITY, + context, destination, device, true)); + + // This is a workaround for older iOS clients who need a low priority push to trigger the logout notification + if (tokenAndType.second() == PushNotification.TokenType.APN) { + sendNotification(new PushNotification(tokenAndType.first(), tokenAndType.second(), + PushNotification.NotificationType.ATTEMPT_LOGIN_NOTIFICATION_LOW_PRIORITY, + context, destination, device, false)); + } + } + public void handleMessagesRetrieved(final Account account, final Device device, final String userAgent) { RedisOperation.unchecked(() -> pushLatencyManager.recordQueueRead(account.getUuid(), device.getId(), userAgent)); apnPushNotificationScheduler.cancelScheduledNotifications(account, device).whenComplete(logErrors()); 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 ccc657f46..40c988219 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryPasswordsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/RegistrationRecoveryPasswordsManager.java @@ -14,6 +14,7 @@ import java.util.concurrent.CompletableFuture; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.whispersystems.textsecuregcm.auth.SaltedTokenHash; +import software.amazon.awssdk.services.dynamodb.model.ResourceNotFoundException; public class RegistrationRecoveryPasswordsManager { @@ -53,7 +54,10 @@ public class RegistrationRecoveryPasswordsManager { // there is no action to be taken on its completion return registrationRecoveryPasswords.removeEntry(number) .whenComplete((ignored, error) -> { - if (error != null) { + 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) { 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 e4f5528f1..5a73005e9 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManagerTest.java @@ -11,10 +11,15 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.util.List; import java.util.UUID; import java.util.function.Consumer; import java.util.stream.Stream; @@ -23,15 +28,19 @@ import javax.ws.rs.WebApplicationException; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; -import org.junit.jupiter.params.provider.EnumSource; import org.junit.jupiter.params.provider.MethodSource; import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; import org.whispersystems.textsecuregcm.entities.PhoneVerificationRequest; import org.whispersystems.textsecuregcm.limits.RateLimiter; import org.whispersystems.textsecuregcm.limits.RateLimiters; import org.whispersystems.textsecuregcm.push.ClientPresenceManager; +import org.whispersystems.textsecuregcm.push.NotPushRegisteredException; +import org.whispersystems.textsecuregcm.push.PushNotificationManager; import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountsManager; +import org.whispersystems.textsecuregcm.storage.Device; +import org.whispersystems.textsecuregcm.storage.RegistrationRecoveryPasswordsManager; +import org.whispersystems.textsecuregcm.tests.util.AccountsHelper; import org.whispersystems.textsecuregcm.util.Pair; class RegistrationLockVerificationManagerTest { @@ -40,9 +49,12 @@ class RegistrationLockVerificationManagerTest { private final ClientPresenceManager clientPresenceManager = mock(ClientPresenceManager.class); private final ExternalServiceCredentialsGenerator backupServiceCredentialsGeneraor = mock( ExternalServiceCredentialsGenerator.class); + private final RegistrationRecoveryPasswordsManager registrationRecoveryPasswordsManager = mock( + RegistrationRecoveryPasswordsManager.class); + private static PushNotificationManager pushNotificationManager = mock(PushNotificationManager.class); private final RateLimiters rateLimiters = mock(RateLimiters.class); private final RegistrationLockVerificationManager registrationLockVerificationManager = new RegistrationLockVerificationManager( - accountsManager, clientPresenceManager, backupServiceCredentialsGeneraor, rateLimiters); + accountsManager, clientPresenceManager, backupServiceCredentialsGeneraor, registrationRecoveryPasswordsManager, pushNotificationManager, rateLimiters); private final RateLimiter pinLimiter = mock(RateLimiter.class); @@ -51,22 +63,32 @@ class RegistrationLockVerificationManagerTest { @BeforeEach void setUp() { + clearInvocations(pushNotificationManager); when(rateLimiters.getPinLimiter()).thenReturn(pinLimiter); when(backupServiceCredentialsGeneraor.generateForUuid(any())) .thenReturn(mock(ExternalServiceCredentials.class)); + final Device device = mock(Device.class); + when(device.getId()).thenReturn(Device.MASTER_ID); + + AccountsHelper.setupMockUpdate(accountsManager); + account = mock(Account.class); when(account.getUuid()).thenReturn(UUID.randomUUID()); when(account.getNumber()).thenReturn("+18005551212"); + when(account.getDevices()).thenReturn(List.of(device)); + existingRegistrationLock = mock(StoredRegistrationLock.class); when(account.getRegistrationLock()).thenReturn(existingRegistrationLock); } @ParameterizedTest - @EnumSource - void testErrors(RegistrationLockError error) throws Exception { + @MethodSource + void testErrors(RegistrationLockError error, boolean alreadyLocked) throws Exception { when(existingRegistrationLock.getStatus()).thenReturn(StoredRegistrationLock.Status.REQUIRED); + when(account.hasLockedCredentials()).thenReturn(alreadyLocked); + doThrow(new NotPushRegisteredException()).when(pushNotificationManager).sendAttemptLoginNotification(any(), any()); final String submittedRegistrationLock = "reglock"; @@ -76,6 +98,16 @@ class RegistrationLockVerificationManagerTest { yield new Pair<>(WebApplicationException.class, e -> { if (e instanceof WebApplicationException wae) { assertEquals(RegistrationLockVerificationManager.FAILURE_HTTP_STATUS, wae.getResponse().getStatus()); + verify(registrationRecoveryPasswordsManager).removeForNumber(account.getNumber()); + verify(clientPresenceManager).disconnectAllPresences(account.getUuid(), List.of(Device.MASTER_ID)); + try { + verify(pushNotificationManager).sendAttemptLoginNotification(any(), eq("failedRegistrationLock")); + } catch (NotPushRegisteredException npre) {} + if (alreadyLocked) { + verify(account, never()).lockAuthTokenHash(); + } else { + verify(account).lockAuthTokenHash(); + } } else { fail("Exception was not of expected type"); } @@ -85,6 +117,12 @@ class RegistrationLockVerificationManagerTest { when(existingRegistrationLock.verify(any())).thenReturn(true); doThrow(RateLimitExceededException.class).when(pinLimiter).validate(anyString()); yield new Pair<>(RateLimitExceededException.class, ignored -> { + verify(account, never()).lockAuthTokenHash(); + try { + verify(pushNotificationManager, never()).sendAttemptLoginNotification(any(), eq("failedRegistrationLock")); + } catch (NotPushRegisteredException npre) {} + verify(registrationRecoveryPasswordsManager, never()).removeForNumber(account.getNumber()); + verify(clientPresenceManager, never()).disconnectAllPresences(account.getUuid(), List.of(Device.MASTER_ID)); }); } }; @@ -97,6 +135,14 @@ class RegistrationLockVerificationManagerTest { exceptionType.second().accept(e); } + static Stream testErrors() { + return Stream.of( + Arguments.of(RegistrationLockError.MISMATCH, true), + Arguments.of(RegistrationLockError.MISMATCH, false), + Arguments.of(RegistrationLockError.RATE_LIMITED, false) + ); + } + @ParameterizedTest @MethodSource void testSuccess(final StoredRegistrationLock.Status status, @Nullable final String submittedRegistrationLock) { @@ -109,6 +155,10 @@ class RegistrationLockVerificationManagerTest { () -> registrationLockVerificationManager.verifyRegistrationLock(account, submittedRegistrationLock, "Signal-Android/4.68.3", RegistrationLockVerificationManager.Flow.REGISTRATION, PhoneVerificationRequest.VerificationType.SESSION)); + + verify(account, never()).lockAuthTokenHash(); + verify(registrationRecoveryPasswordsManager, never()).removeForNumber(account.getNumber()); + verify(clientPresenceManager, never()).disconnectAllPresences(account.getUuid(), List.of(Device.MASTER_ID)); } static Stream testSuccess() { 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 302797bcf..d2d517f14 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java @@ -202,7 +202,8 @@ class AccountControllerTest { BACKUP_CFG); private static final RegistrationLockVerificationManager registrationLockVerificationManager = new RegistrationLockVerificationManager( - accountsManager, clientPresenceManager, backupCredentialsGenerator, rateLimiters); + accountsManager, clientPresenceManager, backupCredentialsGenerator, registrationRecoveryPasswordsManager, + pushNotificationManager, rateLimiters); private static final RegistrationCaptchaManager registrationCaptchaManager = new RegistrationCaptchaManager( captchaChecker, rateLimiters, Map.of(TEST_NUMBER, 123456), dynamicConfigurationManager); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java index ae489d1fc..40f6e2c8b 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java @@ -9,6 +9,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; 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; @@ -137,6 +138,38 @@ class PushNotificationManagerTest { verify(apnSender).sendNotification(new PushNotification(deviceToken, PushNotification.TokenType.APN, PushNotification.NotificationType.RATE_LIMIT_CHALLENGE, challengeToken, account, device, true)); } + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void sendAttemptLoginNotification(final boolean isApn) throws NotPushRegisteredException { + final Account account = mock(Account.class); + final Device device = mock(Device.class); + + final String deviceToken = "token"; + + when(device.getId()).thenReturn(Device.MASTER_ID); + if (isApn) { + when(device.getApnId()).thenReturn(deviceToken); + when(apnSender.sendNotification(any())) + .thenReturn(CompletableFuture.completedFuture(new SendPushNotificationResult(true, null, false))); + } else { + when(device.getGcmId()).thenReturn(deviceToken); + when(fcmSender.sendNotification(any())) + .thenReturn(CompletableFuture.completedFuture(new SendPushNotificationResult(true, null, false))); + } + when(account.getDevice(Device.MASTER_ID)).thenReturn(Optional.of(device)); + + pushNotificationManager.sendAttemptLoginNotification(account, "someContext"); + + if (isApn){ + verify(apnSender).sendNotification(new PushNotification(deviceToken, PushNotification.TokenType.APN, + PushNotification.NotificationType.ATTEMPT_LOGIN_NOTIFICATION_HIGH_PRIORITY, "someContext", account, device, true)); + verify(apnPushNotificationScheduler).scheduleBackgroundNotification(account, device); + } else { + verify(fcmSender, times(1)).sendNotification(new PushNotification(deviceToken, PushNotification.TokenType.FCM, + PushNotification.NotificationType.ATTEMPT_LOGIN_NOTIFICATION_HIGH_PRIORITY, "someContext", account, device, true)); + } + } + @ParameterizedTest @ValueSource(booleans = {true, false}) void testSendNotificationFcm(final boolean urgent) { diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/AccountsHelper.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/AccountsHelper.java index 4151059d1..972e1f9f4 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/AccountsHelper.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/util/AccountsHelper.java @@ -132,6 +132,7 @@ public class AccountsHelper { case "getIdentityKey" -> when(updatedAccount.getIdentityKey()).thenAnswer(stubbing); case "getBadges" -> when(updatedAccount.getBadges()).thenAnswer(stubbing); case "getLastSeen" -> when(updatedAccount.getLastSeen()).thenAnswer(stubbing); + case "hasLockedCredentials" -> when(updatedAccount.hasLockedCredentials()).thenAnswer(stubbing); default -> throw new IllegalArgumentException("unsupported method: Account#" + stubbing.getInvocation().getMethod().getName()); } }