Account for a registration session with only failed attempts

signalapp · Jul 15, 2024 · e91e950 · e91e950
1 parent 2896d8f
commit e91e950
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 4 deletions.
diff --git a/src/main/java/org/signal/registration/RegistrationService.java b/src/main/java/org/signal/registration/RegistrationService.java
@@ -421,8 +421,10 @@ Instant getSessionExpiration(final RegistrationSession session) {
           .map(nextAction -> nextAction.plus(SESSION_TTL_AFTER_LAST_ACTION))
           .ifPresent(candidateExpirations::add);
 
+      // If a session never has a successful registration attempt and exhausts all SMS and voice ratelimits,
+      // fall back to the expiration set at session creation time
       expiration = candidateExpirations.stream().max(Comparator.naturalOrder())
-          .orElseThrow(() -> new IllegalStateException("An unverified session must have an allowed next action or at least one registration attempt"));
+              .orElse(Instant.ofEpochMilli(session.getExpirationEpochMillis()));
     } else {
       // The session must have been verified as a result of the last check
       expiration = Instant.ofEpochMilli(session.getLastCheckCodeAttemptEpochMillis()).plus(SESSION_TTL_AFTER_LAST_ACTION);
@@ -460,7 +462,10 @@ NextActionTimes getNextActionTimes(final RegistrationSession session) {
       // Callers may not request codes via phone call until they've attempted an SMS
       final boolean hasAttemptedSms = session.getRegistrationAttemptsList().stream().anyMatch(attempt ->
           attempt.getMessageTransport() == org.signal.registration.rpc.MessageTransport.MESSAGE_TRANSPORT_SMS) ||
-          session.getRejectedTransportsList().contains(org.signal.registration.rpc.MessageTransport.MESSAGE_TRANSPORT_SMS);
+          session.getRejectedTransportsList().contains(org.signal.registration.rpc.MessageTransport.MESSAGE_TRANSPORT_SMS) ||
+              session.getFailedAttemptsList().stream().anyMatch(attempt ->
+                  attempt.getMessageTransport() == org.signal.registration.rpc.MessageTransport.MESSAGE_TRANSPORT_SMS
+                      && attempt.getFailedSendReason() != FailedSendReason.FAILED_SEND_REASON_SUSPECTED_FRAUD);
 
       if (hasAttemptedSms) {
         nextVoiceCall = sendVoiceVerificationCodeRateLimiter.getTimeOfNextAction(session).join();

diff --git a/src/test/java/org/signal/registration/RegistrationServiceTest.java b/src/test/java/org/signal/registration/RegistrationServiceTest.java
@@ -50,6 +50,7 @@
 import org.signal.registration.sender.SenderRejectedTransportException;
 import org.signal.registration.sender.SenderSelectionStrategy;
 import org.signal.registration.sender.VerificationCodeSender;
+import org.signal.registration.session.FailedSendAttempt;
 import org.signal.registration.session.FailedSendReason;
 import org.signal.registration.session.MemorySessionRepository;
 import org.signal.registration.session.RegistrationAttempt;
@@ -649,6 +650,26 @@ private static Stream<Arguments> getNextActionTimes() {
             false, true, true,
             false, true, false),
 
+        // Unverified session with one failed, non-fraud SMS attempt
+        Arguments.of(getBaseSessionBuilder()
+                .addFailedAttempts(FailedSendAttempt.newBuilder()
+                    .setMessageTransport(org.signal.registration.rpc.MessageTransport.MESSAGE_TRANSPORT_SMS)
+                    .setFailedSendReason(FailedSendReason.FAILED_SEND_REASON_REJECTED)
+                    .build())
+                .build(),
+            true, true, true,
+            true, true, false),
+
+        // Unverified session with one failed, fraud SMS attempt
+        Arguments.of(getBaseSessionBuilder()
+                .addFailedAttempts(FailedSendAttempt.newBuilder()
+                    .setMessageTransport(org.signal.registration.rpc.MessageTransport.MESSAGE_TRANSPORT_SMS)
+                    .setFailedSendReason(FailedSendReason.FAILED_SEND_REASON_SUSPECTED_FRAUD)
+                    .build())
+                .build(),
+            true, true, true,
+            true, false, false),
+
         // Unverified session with voice calls exhausted
         Arguments.of(getBaseSessionBuilder()
                 .addRegistrationAttempts(RegistrationAttempt.newBuilder()
@@ -749,6 +770,7 @@ void getSessionExpiration(final Instant sessionCreation,
 
     final RegistrationSession.Builder sessionBuilder = RegistrationSession.newBuilder()
         .setCreatedEpochMillis(sessionCreation.toEpochMilli())
+        .setExpirationEpochMillis(sessionCreation.plus(RegistrationService.SESSION_TTL_AFTER_LAST_ACTION).toEpochMilli())
         .setLastCheckCodeAttemptEpochMillis(lastCodeCheck.toEpochMilli());
 
     if (verified) {
@@ -790,7 +812,7 @@ private static Stream<Arguments> getSessionExpiration() {
             List.of(CURRENT_TIME.plus(RegistrationService.SESSION_TTL_AFTER_LAST_ACTION).plus(Duration.ofMinutes(3))),
             CURRENT_TIME.plus(RegistrationService.SESSION_TTL_AFTER_LAST_ACTION).plus(Duration.ofMinutes(3))),
 
-        // Verification code never checked; two recent registration attempts
+        // Verification code never checked; two recent successful registration attempts
         Arguments.of(CURRENT_TIME.minusSeconds(600),
             false,
             Instant.ofEpochMilli(0),
@@ -807,7 +829,17 @@ private static Stream<Arguments> getSessionExpiration() {
             Instant.ofEpochMilli(0),
             Optional.of(CURRENT_TIME.minus(RegistrationService.SESSION_TTL_AFTER_LAST_ACTION.dividedBy(2))),
             List.of(),
-            CURRENT_TIME.plus(RegistrationService.SESSION_TTL_AFTER_LAST_ACTION.dividedBy(2)))
+            CURRENT_TIME.plus(RegistrationService.SESSION_TTL_AFTER_LAST_ACTION.dividedBy(2))),
+
+        // No successful registration attempts, not allowed to request another SMS
+        Arguments.of(
+            CURRENT_TIME,
+            false,
+            Instant.ofEpochMilli(0),
+            Optional.empty(),
+            List.of(),
+            CURRENT_TIME.plus(RegistrationService.SESSION_TTL_AFTER_LAST_ACTION)
+        )
     );
   }
 }