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 78f01cd60..258e07558 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManager.java @@ -148,9 +148,15 @@ public class RegistrationLockVerificationManager { updatedAccount = account; } - // 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()); + // The client often sends an empty registration lock token on the first request + // and sends an actual token if the server returns a 423 indicating that one is required. + // This logic accounts for that behavior by not deleting the registration recovery password + // if the user verified correctly via registration recovery password and sent an empty token. + // 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.removeForNumber(updatedAccount.getNumber()); + } final List deviceIds = updatedAccount.getDevices().stream().map(Device::getId).toList(); clientPresenceManager.disconnectAllPresences(updatedAccount.getUuid(), deviceIds); 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 5a73005e9..98adeb5ca 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManagerTest.java @@ -84,21 +84,26 @@ class RegistrationLockVerificationManagerTest { @ParameterizedTest @MethodSource - void testErrors(RegistrationLockError error, boolean alreadyLocked) throws Exception { + void testErrors(RegistrationLockError error, + PhoneVerificationRequest.VerificationType verificationType, + @Nullable String clientRegistrationLock, + 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"; - final Pair, Consumer> exceptionType = switch (error) { case MISMATCH -> { - when(existingRegistrationLock.verify(submittedRegistrationLock)).thenReturn(false); + when(existingRegistrationLock.verify(clientRegistrationLock)).thenReturn(false); yield new Pair<>(WebApplicationException.class, e -> { if (e instanceof WebApplicationException wae) { assertEquals(RegistrationLockVerificationManager.FAILURE_HTTP_STATUS, wae.getResponse().getStatus()); - verify(registrationRecoveryPasswordsManager).removeForNumber(account.getNumber()); + if (!verificationType.equals(PhoneVerificationRequest.VerificationType.RECOVERY_PASSWORD) || clientRegistrationLock != null) { + verify(registrationRecoveryPasswordsManager).removeForNumber(account.getNumber()); + } else { + verify(registrationRecoveryPasswordsManager, never()).removeForNumber(account.getNumber()); + } verify(clientPresenceManager).disconnectAllPresences(account.getUuid(), List.of(Device.MASTER_ID)); try { verify(pushNotificationManager).sendAttemptLoginNotification(any(), eq("failedRegistrationLock")); @@ -128,18 +133,20 @@ class RegistrationLockVerificationManagerTest { }; final Exception e = assertThrows(exceptionType.first(), () -> - registrationLockVerificationManager.verifyRegistrationLock(account, submittedRegistrationLock, + registrationLockVerificationManager.verifyRegistrationLock(account, clientRegistrationLock, "Signal-Android/4.68.3", RegistrationLockVerificationManager.Flow.REGISTRATION, - PhoneVerificationRequest.VerificationType.SESSION)); + verificationType)); 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) + Arguments.of(RegistrationLockError.MISMATCH, PhoneVerificationRequest.VerificationType.SESSION, "reglock", true), + Arguments.of(RegistrationLockError.MISMATCH, PhoneVerificationRequest.VerificationType.SESSION, "reglock", false), + Arguments.of(RegistrationLockError.MISMATCH, PhoneVerificationRequest.VerificationType.RECOVERY_PASSWORD, "reglock", false), + Arguments.of(RegistrationLockError.MISMATCH, PhoneVerificationRequest.VerificationType.RECOVERY_PASSWORD, null, false), + Arguments.of(RegistrationLockError.RATE_LIMITED, PhoneVerificationRequest.VerificationType.SESSION, "reglock", false) ); }