diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java index 43f984307..cd8774005 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java @@ -115,9 +115,6 @@ public class DeviceController { private static final String WAIT_FOR_TRANSFER_ARCHIVE_TIMER_NAME = MetricsUtil.name(DeviceController.class, "waitForTransferArchiveDuration"); - private static final String RECORD_TRANSFER_ARCHIVE_UPLOADED_COUNTER_NAME = MetricsUtil.name(DeviceController.class, "recordTransferArchiveUploaded"); - private static final String HAS_REGISTRATION_ID_TAG_NAME = "hasRegistrationId"; - @VisibleForTesting static final int MIN_TOKEN_IDENTIFIER_LENGTH = 32; @@ -537,14 +534,7 @@ public class DeviceController { @ApiResponse(responseCode = "422", description = "The request object could not be parsed or was otherwise invalid") @ApiResponse(responseCode = "429", description = "Rate-limited; try again after the prescribed delay") public CompletionStage recordTransferArchiveUploaded(@Auth final AuthenticatedDevice authenticatedDevice, - @NotNull @Valid final TransferArchiveUploadedRequest transferArchiveUploadedRequest, - @HeaderParam(HttpHeaders.USER_AGENT) @Nullable String userAgent) { - Metrics.counter(RECORD_TRANSFER_ARCHIVE_UPLOADED_COUNTER_NAME, Tags.of( - UserAgentTagUtil.getPlatformTag(userAgent), - io.micrometer.core.instrument.Tag.of( - HAS_REGISTRATION_ID_TAG_NAME, - String.valueOf(transferArchiveUploadedRequest.destinationDeviceRegistrationId().isPresent())) - )).increment(); + @NotNull @Valid final TransferArchiveUploadedRequest transferArchiveUploadedRequest) { return rateLimiters.getUploadTransferArchiveLimiter() .validateAsync(authenticatedDevice.accountIdentifier()) .thenCompose(ignored -> accounts.getByAccountIdentifierAsync(authenticatedDevice.accountIdentifier())) @@ -554,7 +544,6 @@ public class DeviceController { return accounts.recordTransferArchiveUpload(account, transferArchiveUploadedRequest.destinationDeviceId(), - transferArchiveUploadedRequest.destinationDeviceCreated().map(Instant::ofEpochMilli), transferArchiveUploadedRequest.destinationDeviceRegistrationId(), transferArchiveUploadedRequest.transferArchive()); }); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/entities/DeviceInfo.java b/service/src/main/java/org/whispersystems/textsecuregcm/entities/DeviceInfo.java index 24e7bf3b9..b640340a4 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/entities/DeviceInfo.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/entities/DeviceInfo.java @@ -21,13 +21,6 @@ public record DeviceInfo(long id, long lastSeen, - @Deprecated - @Schema(description = """ - The time in milliseconds since epoch when the device was linked. - Deprecated in favor of `createdAtCiphertext`. - """, deprecated = true) - long created, - @Schema(description = "The registration ID of the given device.") int registrationId, @@ -40,7 +33,7 @@ public record DeviceInfo(long id, byte[] createdAtCiphertext) { public static DeviceInfo forDevice(final Device device) { - return new DeviceInfo(device.getId(), device.getName(), device.getLastSeen(), device.getCreated(), device.getRegistrationId( + return new DeviceInfo(device.getId(), device.getName(), device.getLastSeen(), device.getRegistrationId( IdentityType.ACI), device.getCreatedAtCiphertext()); } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/entities/TransferArchiveUploadedRequest.java b/service/src/main/java/org/whispersystems/textsecuregcm/entities/TransferArchiveUploadedRequest.java index 2176f1c25..64adfb054 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/entities/TransferArchiveUploadedRequest.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/entities/TransferArchiveUploadedRequest.java @@ -22,15 +22,8 @@ public record TransferArchiveUploadedRequest( @Schema(description = "The ID of the device for which the transfer archive has been prepared") byte destinationDeviceId, - @Schema(description = """ - The timestamp, in milliseconds since the epoch, at which the destination device was created. - Deprecated in favor of `destinationDeviceRegistrationId`. - """, deprecated = true) - @Deprecated - Optional<@Positive Long> destinationDeviceCreated, - @Schema(description = "The registration ID of the destination device") - Optional<@Min(0) @Max(Device.MAX_REGISTRATION_ID) Integer> destinationDeviceRegistrationId, + @Min(0) @Max(Device.MAX_REGISTRATION_ID) int destinationDeviceRegistrationId, @NotNull @Valid @@ -39,9 +32,4 @@ public record TransferArchiveUploadedRequest( the upload has failed and the destination device should stop waiting """, oneOf = {RemoteAttachment.class, RemoteAttachmentError.class}) TransferArchiveResult transferArchive) { - @AssertTrue - @Schema(hidden = true) - public boolean isExactlyOneDisambiguatorProvided() { - return destinationDeviceCreated.isPresent() ^ destinationDeviceRegistrationId.isPresent(); - } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java index 198545531..37259d462 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -37,7 +37,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Base64; import java.util.Collection; -import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -51,7 +50,6 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiConsumer; import java.util.function.Consumer; @@ -115,7 +113,6 @@ public class AccountsManager extends RedisPubSubAdapter implemen private static final String DELETE_COUNTER_NAME = name(AccountsManager.class, "deleteCounter"); private static final String COUNTRY_CODE_TAG_NAME = "country"; private static final String DELETION_REASON_TAG_NAME = "reason"; - private static final String TIMESTAMP_BASED_TRANSFER_ARCHIVE_KEY_COUNTER_NAME = name(AccountsManager.class, "timestampRedisKeyCounter"); private static final String REGISTRATION_ID_BASED_TRANSFER_ARCHIVE_KEY_COUNTER_NAME = name(AccountsManager.class,"registrationIdRedisKeyCounter"); private static final String RETRY_NAME = ResilienceUtil.name(AccountsManager.class); @@ -205,14 +202,8 @@ public class AccountsManager extends RedisPubSubAdapter implemen } } - private interface DeviceIdentifier {} - - private record TimestampDeviceIdentifier(UUID accountIdentifier, byte deviceId, Instant deviceCreationTimestamp) - implements DeviceIdentifier { - } - - private record RegistrationIdDeviceIdentifier(UUID accountIdentifier, byte deviceId, - int registrationId) implements DeviceIdentifier { + private record DeviceIdentifier(UUID accountIdentifier, byte deviceId, + int registrationId) { } public AccountsManager(final Accounts accounts, @@ -1469,21 +1460,14 @@ public class AccountsManager extends RedisPubSubAdapter implemen linkDeviceTokenIdentifier, getLinkedDeviceKey(linkDeviceTokenIdentifier), timeout, this::handleDeviceAdded); return deviceAdded.thenCompose(maybeDeviceInfo -> maybeDeviceInfo.map(deviceInfo -> { - // The device finished linking, we now want to make sure the client has fetched messages that could - // have come in before the device's mailbox was set up. + // The device finished linking, we now want to make sure the primary client has fetched messages that could + // have come in before the linked device's mailbox was set up. This avoids a race where the linked device + // misses out on messages that were sent before its mailbox was set up but received by the primary *after* + // creating its backup for the linked device. - // A worst case estimate of the wall clock time at which the linked device was added to the account record - Instant deviceLinked = Instant.ofEpochMilli(deviceInfo.created()).plus(MAX_SERVER_CLOCK_DRIFT); - - Instant now = clock.instant(); - - // We know at `now` the device finished linking, so if we waited for all the messages before now it would be - // sufficient. However, now might be much later that the device was linked, so we don't want to force - // the client to wait for messages that are past our worst case estimate of when the device was linked - Instant messageEpoch = Collections.min(List.of(deviceLinked, now)); - - // We assume that any message with a timestamp after the messageEpoch made it into the linked device's queues - return waitForPreLinkMessagesToBeFetched(accountIdentifier, linkingDevice, deviceInfo, messageEpoch, deadline); + // We know the device finished linking at the current time, so waiting for all messages + // before now is sufficient. + return waitForPreLinkMessagesToBeFetched(accountIdentifier, linkingDevice, deviceInfo, clock.instant(), deadline); }) .orElseGet(() -> CompletableFuture.completedFuture(maybeDeviceInfo))); } @@ -1547,59 +1531,24 @@ public class AccountsManager extends RedisPubSubAdapter implemen } public CompletableFuture> waitForTransferArchive(final Account account, final Device device, final Duration timeout) { - final DeviceIdentifier timestampDeviceIdentifier = new TimestampDeviceIdentifier(account.getIdentifier(IdentityType.ACI), device.getId(), Instant.ofEpochMilli(device.getCreated())); - final String timestampTransferArchiveKey = getTimestampTransferArchiveKey(account.getIdentifier(IdentityType.ACI), device.getId(), Instant.ofEpochMilli(device.getCreated())); - - final DeviceIdentifier registrationIdDeviceIdentifier = new RegistrationIdDeviceIdentifier(account.getIdentifier(IdentityType.ACI), device.getId(), device.getRegistrationId(IdentityType.ACI)); + final DeviceIdentifier deviceIdentifier = new DeviceIdentifier(account.getIdentifier(IdentityType.ACI), device.getId(), device.getRegistrationId(IdentityType.ACI)); final String registrationIdTransferArchiveKey = getRegistrationIdTransferArchiveKey(account.getIdentifier(IdentityType.ACI), device.getId(), device.getRegistrationId(IdentityType.ACI)); - final CompletableFuture> timestampFuture = waitForPubSubKey(waitForTransferArchiveFuturesByDeviceIdentifier, - timestampDeviceIdentifier, - timestampTransferArchiveKey, - timeout, - this::handleTransferArchiveAdded); - - final CompletableFuture> registrationIdFuture = waitForPubSubKey(waitForTransferArchiveFuturesByDeviceIdentifier, - registrationIdDeviceIdentifier, + return waitForPubSubKey(waitForTransferArchiveFuturesByDeviceIdentifier, + deviceIdentifier, registrationIdTransferArchiveKey, timeout, this::handleTransferArchiveAdded); - return firstSuccessfulTransferArchiveFuture(List.of(timestampFuture, registrationIdFuture)); - } - - @VisibleForTesting - static CompletableFuture> firstSuccessfulTransferArchiveFuture( - final List>> futures) { - final CompletableFuture> result = new CompletableFuture<>(); - final AtomicInteger remaining = new AtomicInteger(futures.size()); - - for (CompletableFuture> future : futures) { - future.whenComplete((value, _) -> { - if (value.isPresent()) { - result.complete(value); - } else if (remaining.decrementAndGet() == 0) { - result.complete(Optional.empty()); - } - }); - } - - return result; } public CompletableFuture recordTransferArchiveUpload(final Account account, final byte destinationDeviceId, - @SuppressWarnings("OptionalUsedAsFieldOrParameterType") final Optional destinationDeviceCreationTimestamp, - @SuppressWarnings("OptionalUsedAsFieldOrParameterType") final Optional maybeRegistrationId, + final int registrationId, final TransferArchiveResult transferArchiveResult) { try { final String transferArchiveJson = SystemMapper.jsonMapper().writeValueAsString(transferArchiveResult); - final String key = destinationDeviceCreationTimestamp - .map(timestamp -> getTimestampTransferArchiveKey(account.getIdentifier(IdentityType.ACI), destinationDeviceId, timestamp)) - .orElseGet(() -> maybeRegistrationId - .map(registrationId -> getRegistrationIdTransferArchiveKey(account.getIdentifier(IdentityType.ACI), destinationDeviceId, registrationId)) - // We validate the request object so this should never happen - .orElseThrow(() -> new AssertionError("No creation timestamp or registration ID provided"))); + final String key = getRegistrationIdTransferArchiveKey(account.getIdentifier(IdentityType.ACI), destinationDeviceId, registrationId); return ResilienceUtil.getGeneralRedisRetry(RETRY_NAME) .executeCompletionStage(retryExecutor, () -> pubSubRedisClient.withConnection(connection -> connection.async() @@ -1622,16 +1571,6 @@ public class AccountsManager extends RedisPubSubAdapter implemen } } - private static String getTimestampTransferArchiveKey(final UUID accountIdentifier, - final byte destinationDeviceId, - final Instant destinationDeviceCreationTimestamp) { - Metrics.counter(TIMESTAMP_BASED_TRANSFER_ARCHIVE_KEY_COUNTER_NAME).increment(); - - return TRANSFER_ARCHIVE_PREFIX + accountIdentifier.toString() + - ":" + destinationDeviceId + - ":" + destinationDeviceCreationTimestamp.toEpochMilli(); - } - private static String getRegistrationIdTransferArchiveKey(final UUID accountIdentifier, final byte destinationDeviceId, final int registrationId) { @@ -1734,8 +1673,9 @@ public class AccountsManager extends RedisPubSubAdapter implemen final String[] deviceIdentifierComponents = channel.substring(TRANSFER_ARCHIVE_KEYSPACE_PATTERN.length() - 1).split(":", 4); - if (deviceIdentifierComponents.length != 3 && deviceIdentifierComponents.length != 4) { - logger.error("Could not parse device identifier; unexpected component count"); + if (deviceIdentifierComponents.length != 4) { + logger.error("Could not parse device identifier; unexpected component count: {}", + deviceIdentifierComponents.length); return; } @@ -1745,24 +1685,16 @@ public class AccountsManager extends RedisPubSubAdapter implemen final UUID accountIdentifier = UUID.fromString(deviceIdentifierComponents[0]); final byte deviceId = Byte.parseByte(deviceIdentifierComponents[1]); - if (deviceIdentifierComponents.length == 3) { - // Parse the old transfer archive Redis key format - final Instant deviceCreationTimestamp = Instant.ofEpochMilli(Long.parseLong(deviceIdentifierComponents[2])); - - deviceIdentifier = new TimestampDeviceIdentifier(accountIdentifier, deviceId, deviceCreationTimestamp); - transferArchiveKey = getTimestampTransferArchiveKey(accountIdentifier, deviceId, deviceCreationTimestamp); - } else { - final String maybeRegistrationIdPattern = deviceIdentifierComponents[2]; - if (!maybeRegistrationIdPattern.equals(TRANSFER_ARCHIVE_REGISTRATION_ID_PATTERN)) { - throw new IllegalArgumentException("Could not parse Redis key with pattern " + maybeRegistrationIdPattern); - } - final int registrationId = Integer.parseInt(deviceIdentifierComponents[3]); - if (!RegistrationIdValidator.validRegistrationId(registrationId)) { - throw new IllegalArgumentException("Invalid registration ID: " + registrationId); - } - deviceIdentifier = new RegistrationIdDeviceIdentifier(accountIdentifier, deviceId, registrationId); - transferArchiveKey = getRegistrationIdTransferArchiveKey(accountIdentifier, deviceId, registrationId); + final String registrationIdPattern = deviceIdentifierComponents[2]; + if (!registrationIdPattern.equals(TRANSFER_ARCHIVE_REGISTRATION_ID_PATTERN)) { + throw new IllegalArgumentException("Could not parse Redis key with pattern " + registrationIdPattern); } + final int registrationId = Integer.parseInt(deviceIdentifierComponents[3]); + if (!RegistrationIdValidator.validRegistrationId(registrationId)) { + throw new IllegalArgumentException("Invalid registration ID: " + registrationId); + } + deviceIdentifier = new DeviceIdentifier(accountIdentifier, deviceId, registrationId); + transferArchiveKey = getRegistrationIdTransferArchiveKey(accountIdentifier, deviceId, registrationId); Optional.ofNullable(waitForTransferArchiveFuturesByDeviceIdentifier.remove(deviceIdentifier)) .ifPresent(future -> pubSubRedisClient.withConnection(connection -> connection.async().get(transferArchiveKey)) 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 835bb9b3f..b1ddbd866 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/DeviceControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/DeviceControllerTest.java @@ -10,6 +10,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyByte; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.anyString; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.eq; @@ -196,7 +197,6 @@ class DeviceControllerTest { final Device refreshedDevice = mock(Device.class); when(refreshedDevice.getId()).thenReturn(deviceId); when(refreshedDevice.getName()).thenReturn(deviceName); - when(refreshedDevice.getCreated()).thenReturn(deviceCreated); when(refreshedDevice.getLastSeen()).thenReturn(deviceLastSeen); when(refreshedDevice.getRegistrationId(IdentityType.ACI)).thenReturn(registrationId); when(refreshedDevice.getCreatedAtCiphertext()).thenReturn(createdAtCiphertext); @@ -215,7 +215,6 @@ class DeviceControllerTest { assertEquals(1, deviceInfoList.devices().size()); assertEquals(deviceId, deviceInfoList.devices().getFirst().id()); assertArrayEquals(deviceName, deviceInfoList.devices().getFirst().name()); - assertEquals(deviceCreated, deviceInfoList.devices().getFirst().created()); assertEquals(deviceLastSeen, deviceInfoList.devices().getFirst().lastSeen()); assertEquals(registrationId, deviceInfoList.devices().getFirst().registrationId()); assertArrayEquals(createdAtCiphertext, deviceInfoList.devices().getFirst().createdAtCiphertext()); @@ -962,7 +961,6 @@ class DeviceControllerTest { final DeviceInfo deviceInfo = new DeviceInfo(Device.PRIMARY_ID, "Device name ciphertext".getBytes(StandardCharsets.UTF_8), System.currentTimeMillis(), - System.currentTimeMillis(), 1, "timestamp ciphertext".getBytes(StandardCharsets.UTF_8)); @@ -985,7 +983,6 @@ class DeviceControllerTest { final DeviceInfo retrievedDeviceInfo = response.readEntity(DeviceInfo.class); assertEquals(deviceInfo.id(), retrievedDeviceInfo.id()); assertArrayEquals(deviceInfo.name(), retrievedDeviceInfo.name()); - assertEquals(deviceInfo.created(), retrievedDeviceInfo.created()); assertEquals(deviceInfo.lastSeen(), retrievedDeviceInfo.lastSeen()); assertEquals(deviceInfo.registrationId(), retrievedDeviceInfo.registrationId()); assertArrayEquals(deviceInfo.createdAtCiphertext(), retrievedDeviceInfo.createdAtCiphertext()); @@ -1083,60 +1080,53 @@ class DeviceControllerTest { } } - @ParameterizedTest - @MethodSource - @SuppressWarnings("OptionalUsedAsFieldOrParameterType") - void recordTransferArchiveUploaded(final Optional deviceCreated, final Optional registrationId) { + @Test + void recordTransferArchiveUploaded() { final byte deviceId = Device.PRIMARY_ID + 1; + final int registrationId = 123; final RemoteAttachment transferArchive = new RemoteAttachment(3, Base64.getUrlEncoder().encodeToString("test".getBytes(StandardCharsets.UTF_8))); when(rateLimiter.validateAsync(AuthHelper.VALID_UUID)).thenReturn(CompletableFuture.completedFuture(null)); - when(accountsManager.recordTransferArchiveUpload(account, deviceId, deviceCreated, registrationId, transferArchive)) + when(accountsManager.recordTransferArchiveUpload(account, deviceId, registrationId, transferArchive)) .thenReturn(CompletableFuture.completedFuture(null)); try (final Response response = resources.getJerseyTest() .target("/v1/devices/transfer_archive") .request() .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) - .put(Entity.entity(new TransferArchiveUploadedRequest(deviceId, deviceCreated.map(Instant::toEpochMilli), registrationId, transferArchive), + .put(Entity.entity(new TransferArchiveUploadedRequest(deviceId, registrationId, transferArchive), MediaType.APPLICATION_JSON_TYPE))) { assertEquals(204, response.getStatus()); verify(accountsManager) - .recordTransferArchiveUpload(account, deviceId, deviceCreated, registrationId, transferArchive); + .recordTransferArchiveUpload(account, deviceId, registrationId, transferArchive); } } - private static List recordTransferArchiveUploaded() { - return List.of( - Arguments.of(Optional.empty(), Optional.of(123)), - Arguments.of(Optional.of(Instant.now().truncatedTo(ChronoUnit.MILLIS)), Optional.empty()) - ); - } - @Test void recordTransferArchiveFailed() { final byte deviceId = Device.PRIMARY_ID + 1; + final int registrationId = 123; final Instant deviceCreated = Instant.now().truncatedTo(ChronoUnit.MILLIS); final RemoteAttachmentError transferFailure = new RemoteAttachmentError(RemoteAttachmentError.ErrorType.CONTINUE_WITHOUT_UPLOAD); when(rateLimiter.validateAsync(AuthHelper.VALID_UUID)).thenReturn(CompletableFuture.completedFuture(null)); - when(accountsManager.recordTransferArchiveUpload(account, deviceId, Optional.of(deviceCreated), Optional.empty(), transferFailure)) + when(accountsManager.recordTransferArchiveUpload(account, deviceId, registrationId, transferFailure)) .thenReturn(CompletableFuture.completedFuture(null)); try (final Response response = resources.getJerseyTest() .target("/v1/devices/transfer_archive") .request() .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) - .put(Entity.entity(new TransferArchiveUploadedRequest(deviceId, Optional.of(deviceCreated.toEpochMilli()), Optional.empty(), transferFailure), + .put(Entity.entity(new TransferArchiveUploadedRequest(deviceId, registrationId, transferFailure), MediaType.APPLICATION_JSON_TYPE))) { assertEquals(204, response.getStatus()); verify(accountsManager) - .recordTransferArchiveUpload(account, deviceId, Optional.of(deviceCreated), Optional.empty(), transferFailure); + .recordTransferArchiveUpload(account, deviceId, registrationId, transferFailure); } } @@ -1154,7 +1144,7 @@ class DeviceControllerTest { assertEquals(422, response.getStatus()); verify(accountsManager, never()) - .recordTransferArchiveUpload(any(), anyByte(), any(), any(), any()); + .recordTransferArchiveUpload(any(), anyByte(), anyInt(), any()); } } @@ -1164,22 +1154,16 @@ class DeviceControllerTest { new RemoteAttachment(3, Base64.getUrlEncoder().encodeToString("archive".getBytes(StandardCharsets.UTF_8))); return List.of( - Arguments.argumentSet("Invalid device ID", new TransferArchiveUploadedRequest((byte) -1, Optional.of(System.currentTimeMillis()), Optional.empty(), validTransferArchive)), - Arguments.argumentSet("Invalid \"created at\" timestamp", - new TransferArchiveUploadedRequest(Device.PRIMARY_ID, Optional.of((long) -1), Optional.empty(), validTransferArchive)), + Arguments.argumentSet("Invalid device ID", new TransferArchiveUploadedRequest((byte) -1, 1, validTransferArchive)), Arguments.argumentSet("Invalid registration ID - negative", - new TransferArchiveUploadedRequest(Device.PRIMARY_ID, Optional.empty(), Optional.of(-1), validTransferArchive)), + new TransferArchiveUploadedRequest(Device.PRIMARY_ID, -1, validTransferArchive)), Arguments.argumentSet("Invalid registration ID - too large", - new TransferArchiveUploadedRequest(Device.PRIMARY_ID, Optional.empty(), Optional.of(0x4000), validTransferArchive)), - Arguments.argumentSet("Exactly one of \"created at\" timestamp and registration ID must be present - neither provided", - new TransferArchiveUploadedRequest(Device.PRIMARY_ID, Optional.empty(), Optional.empty(), validTransferArchive)), - Arguments.argumentSet("Exactly one of \"created at\" timestamp and registration ID must be present - both provided", - new TransferArchiveUploadedRequest(Device.PRIMARY_ID, Optional.of(System.currentTimeMillis()), Optional.of(123), validTransferArchive)), + new TransferArchiveUploadedRequest(Device.PRIMARY_ID, 0x4000, validTransferArchive)), Arguments.argumentSet("Missing CDN number", - new TransferArchiveUploadedRequest(Device.PRIMARY_ID, Optional.of(System.currentTimeMillis()), Optional.empty(), + new TransferArchiveUploadedRequest(Device.PRIMARY_ID, 1, new RemoteAttachment(null, Base64.getUrlEncoder().encodeToString("archive".getBytes(StandardCharsets.UTF_8))))), Arguments.argumentSet("Bad attachment key", - new TransferArchiveUploadedRequest(Device.PRIMARY_ID, Optional.of(System.currentTimeMillis()), Optional.empty(), + new TransferArchiveUploadedRequest(Device.PRIMARY_ID, 1, new RemoteAttachment(3, "This is not a valid base64 string"))) ); } @@ -1193,14 +1177,14 @@ class DeviceControllerTest { .target("/v1/devices/transfer_archive") .request() .header("Authorization", AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) - .put(Entity.entity(new TransferArchiveUploadedRequest(Device.PRIMARY_ID, Optional.of(System.currentTimeMillis()), Optional.empty(), + .put(Entity.entity(new TransferArchiveUploadedRequest(Device.PRIMARY_ID, 1, new RemoteAttachment(3, Base64.getUrlEncoder().encodeToString("test".getBytes(StandardCharsets.UTF_8)))), MediaType.APPLICATION_JSON_TYPE))) { assertEquals(429, response.getStatus()); verify(accountsManager, never()) - .recordTransferArchiveUpload(any(), anyByte(), any(), any(), any()); + .recordTransferArchiveUpload(any(), anyByte(), anyInt(), any()); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerDeviceTransferIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerDeviceTransferIntegrationTest.java index 479ef458c..367a98d6c 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerDeviceTransferIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerDeviceTransferIntegrationTest.java @@ -29,7 +29,6 @@ import org.whispersystems.textsecuregcm.util.TestRandomUtil; import java.nio.charset.StandardCharsets; import java.time.Clock; import java.time.Duration; -import java.time.Instant; import java.util.Base64; import java.util.List; import java.util.Optional; @@ -91,13 +90,10 @@ public class AccountsManagerDeviceTransferIntegrationTest { accountsManager.stop(); } - @ParameterizedTest - @MethodSource - @SuppressWarnings("OptionalUsedAsFieldOrParameterType") - void waitForTransferArchive( - final Optional recordUploadDeviceCreated, - final Optional recordUploadRegistrationId) { + @Test + void waitForTransferArchive() { final UUID accountIdentifier = UUID.randomUUID(); + final int registrationId = 123; final byte deviceId = Device.PRIMARY_ID; final RemoteAttachment transferArchive = @@ -105,8 +101,7 @@ public class AccountsManagerDeviceTransferIntegrationTest { final Device device = mock(Device.class); when(device.getId()).thenReturn(deviceId); - when(device.getCreated()).thenReturn(recordUploadDeviceCreated.orElse(System.currentTimeMillis())); - when(device.getRegistrationId(IdentityType.ACI)).thenReturn(recordUploadRegistrationId.orElse(1)); + when(device.getRegistrationId(IdentityType.ACI)).thenReturn(registrationId); final Account account = mock(Account.class); when(account.getIdentifier(IdentityType.ACI)).thenReturn(accountIdentifier); @@ -119,106 +114,62 @@ public class AccountsManagerDeviceTransferIntegrationTest { assertEquals(Optional.empty(), displacedFuture.join()); - accountsManager.recordTransferArchiveUpload(account, deviceId, recordUploadDeviceCreated.map(Instant::ofEpochMilli), recordUploadRegistrationId, transferArchive).join(); + accountsManager.recordTransferArchiveUpload(account, deviceId, registrationId, transferArchive).join(); assertEquals(Optional.of(transferArchive), activeFuture.join()); } - private static List waitForTransferArchive() { - final long deviceCreated = System.currentTimeMillis(); - final int registrationId = 123; - - return List.of( - Arguments.of(Optional.empty(), Optional.of(registrationId)), - Arguments.of(Optional.of(deviceCreated), Optional.empty()) - ); - } - - @ParameterizedTest - @MethodSource - @SuppressWarnings("OptionalUsedAsFieldOrParameterType") - void waitForTransferArchiveAlreadyAdded( - final Optional recordUploadDeviceCreated, - final Optional recordUploadRegistrationId) { + @Test + void waitForTransferArchiveAlreadyAdded() { final UUID accountIdentifier = UUID.randomUUID(); final byte deviceId = Device.PRIMARY_ID; - + final int registrationId = 123; final RemoteAttachment transferArchive = new RemoteAttachment(3, Base64.getUrlEncoder().encodeToString("transfer-archive".getBytes(StandardCharsets.UTF_8))); final Device device = mock(Device.class); when(device.getId()).thenReturn(deviceId); - when(device.getCreated()).thenReturn(recordUploadDeviceCreated.orElse(System.currentTimeMillis())); - when(device.getRegistrationId(IdentityType.ACI)).thenReturn(recordUploadRegistrationId.orElse(1)); + when(device.getRegistrationId(IdentityType.ACI)).thenReturn(registrationId); final Account account = mock(Account.class); when(account.getIdentifier(IdentityType.ACI)).thenReturn(accountIdentifier); - accountsManager.recordTransferArchiveUpload(account, deviceId, recordUploadDeviceCreated.map(Instant::ofEpochMilli), recordUploadRegistrationId, transferArchive).join(); + accountsManager.recordTransferArchiveUpload(account, deviceId, registrationId, transferArchive).join(); assertEquals(Optional.of(transferArchive), accountsManager.waitForTransferArchive(account, device, Duration.ofSeconds(5)).join()); } - private static List waitForTransferArchiveAlreadyAdded() { - final long deviceCreated = System.currentTimeMillis(); - final int registrationId = 123; - - return List.of( - Arguments.of(Optional.empty(), Optional.of(registrationId)), - Arguments.of(Optional.of(deviceCreated), Optional.empty()) - ); - } - - @ParameterizedTest - @MethodSource - @SuppressWarnings("OptionalUsedAsFieldOrParameterType") - void waitForErrorTransferArchive( - final Optional recordUploadDeviceCreated, - final Optional recordUploadRegistrationId) { + @Test + void waitForErrorTransferArchive() { final UUID accountIdentifier = UUID.randomUUID(); final byte deviceId = Device.PRIMARY_ID; + final int registrationId = 123; final RemoteAttachmentError transferArchiveError = new RemoteAttachmentError(RemoteAttachmentError.ErrorType.CONTINUE_WITHOUT_UPLOAD); final Device device = mock(Device.class); when(device.getId()).thenReturn(deviceId); - when(device.getCreated()).thenReturn(recordUploadDeviceCreated.orElse(System.currentTimeMillis())); - when(device.getRegistrationId(IdentityType.ACI)).thenReturn(recordUploadRegistrationId.orElse(1)); + when(device.getRegistrationId(IdentityType.ACI)).thenReturn(registrationId); final Account account = mock(Account.class); when(account.getIdentifier(IdentityType.ACI)).thenReturn(accountIdentifier); - accountsManager.recordTransferArchiveUpload(account, deviceId, recordUploadDeviceCreated.map(Instant::ofEpochMilli), - recordUploadRegistrationId, transferArchiveError).join(); + accountsManager.recordTransferArchiveUpload(account, deviceId, + registrationId, transferArchiveError).join(); assertEquals(Optional.of(transferArchiveError), accountsManager.waitForTransferArchive(account, device, Duration.ofSeconds(5)).join()); } - private static List waitForErrorTransferArchive() { - final long deviceCreated = System.currentTimeMillis(); - final int registrationId = 123; - - return List.of( - Arguments.of(Optional.empty(), Optional.of(registrationId)), - Arguments.of(Optional.of(deviceCreated), Optional.empty()) - ); - } - - @ParameterizedTest - @MethodSource - @SuppressWarnings("OptionalUsedAsFieldOrParameterType") - void waitForTransferArchiveTimeout( - final Optional recordUploadDeviceCreated, - final Optional recordUploadRegistrationId) { + @Test + void waitForTransferArchiveTimeout() { final UUID accountIdentifier = UUID.randomUUID(); final Device device = mock(Device.class); - when(device.getCreated()).thenReturn(recordUploadDeviceCreated.orElse(System.currentTimeMillis())); - when(device.getRegistrationId(IdentityType.ACI)).thenReturn(recordUploadRegistrationId.orElse(1)); + when(device.getRegistrationId(IdentityType.ACI)).thenReturn(123); final Account account = mock(Account.class); when(account.getIdentifier(IdentityType.ACI)).thenReturn(accountIdentifier); @@ -227,16 +178,6 @@ public class AccountsManagerDeviceTransferIntegrationTest { accountsManager.waitForTransferArchive(account, device, Duration.ofMillis(1)).join()); } - private static List waitForTransferArchiveTimeout() { - final long deviceCreated = System.currentTimeMillis(); - final int registrationId = 123; - - return List.of( - Arguments.of(Optional.empty(), Optional.of(registrationId)), - Arguments.of(Optional.of(deviceCreated), Optional.empty()) - ); - } - @Test void waitForRestoreAccountRequest() { final String token = RandomStringUtils.secure().nextAlphanumeric(16); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java index 5a59cbce2..87468b61d 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java @@ -1497,61 +1497,6 @@ class AccountsManagerTest { } } - @Test - void testFirstSuccessfulTransferArchiveCompletableFutureOneTimeout() { - // First future times out, second one completes successfully - final RemoteAttachment transferArchive = new RemoteAttachment(3, Base64.getUrlEncoder().encodeToString("test".getBytes(StandardCharsets.UTF_8))); - - final CompletableFuture> timeoutFuture = new CompletableFuture<>(); - timeoutFuture.completeOnTimeout(Optional.empty(), 50, TimeUnit.MILLISECONDS); - - final CompletableFuture> successfulFuture = new CompletableFuture<>(); - - final CompletableFuture> result = - AccountsManager.firstSuccessfulTransferArchiveFuture(List.of(timeoutFuture, successfulFuture)); - - CompletableFuture.delayedExecutor(100, TimeUnit.MILLISECONDS) - .execute(() -> successfulFuture.complete(Optional.of(transferArchive))); - - final Optional maybeTransferArchive = result.join(); - assertTrue(maybeTransferArchive.isPresent()); - assertEquals(transferArchive, maybeTransferArchive.get()); - } - - @Test - void testFirstSuccessfulTransferArchiveCompletableFutureBothTimeout() { - // Both futures time out - final CompletableFuture> firstTimeoutFuture = new CompletableFuture<>(); - firstTimeoutFuture.completeOnTimeout(Optional.empty(), 10, TimeUnit.MILLISECONDS); - - final CompletableFuture> secondTimeoutFuture = new CompletableFuture<>(); - secondTimeoutFuture.completeOnTimeout(Optional.empty(), 10, TimeUnit.MILLISECONDS); - - final CompletableFuture> result = - AccountsManager.firstSuccessfulTransferArchiveFuture(List.of(firstTimeoutFuture, secondTimeoutFuture)); - - assertTrue(result.join().isEmpty()); - } - - @Test - void testFirstSuccessfulTransferArchiveCompletableFuture() { - // First future completes successfully, second one times out - final RemoteAttachment transferArchive = new RemoteAttachment(3, Base64.getUrlEncoder().encodeToString("test".getBytes(StandardCharsets.UTF_8))); - - final CompletableFuture> successfulFuture = new CompletableFuture<>(); - - final CompletableFuture> timeoutFuture = new CompletableFuture<>(); - timeoutFuture.completeOnTimeout(Optional.empty(), 50, TimeUnit.MILLISECONDS); - - final CompletableFuture> result = - AccountsManager.firstSuccessfulTransferArchiveFuture(List.of(successfulFuture, timeoutFuture)); - successfulFuture.complete(Optional.of(transferArchive)); - - final Optional maybeTransferArchive = result.join(); - assertTrue(maybeTransferArchive.isPresent()); - assertEquals(transferArchive, maybeTransferArchive.get()); - } - private static List validateCompleteDeviceList() { final byte deviceId = Device.PRIMARY_ID; final byte extraDeviceId = deviceId + 1; diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java index 078a35a5f..07be14e1c 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AddRemoveDeviceIntegrationTest.java @@ -472,7 +472,6 @@ public class AddRemoveDeviceIntegrationTest { final DeviceInfo deviceInfo = maybeDeviceInfo.get(); assertEquals(updatedAccountAndDevice.second().getId(), deviceInfo.id()); - assertEquals(updatedAccountAndDevice.second().getCreated(), deviceInfo.created()); assertEquals(updatedAccountAndDevice.second().getRegistrationId(IdentityType.ACI), deviceInfo.registrationId()); assertNotNull(deviceInfo.createdAtCiphertext()); } @@ -521,7 +520,6 @@ public class AddRemoveDeviceIntegrationTest { final DeviceInfo deviceInfo = maybeDeviceInfo.get(); assertEquals(updatedAccountAndDevice.second().getId(), deviceInfo.id()); - assertEquals(updatedAccountAndDevice.second().getCreated(), deviceInfo.created()); assertEquals(updatedAccountAndDevice.second().getRegistrationId(IdentityType.ACI), deviceInfo.registrationId()); assertNotNull(deviceInfo.createdAtCiphertext()); } @@ -546,13 +544,12 @@ public class AddRemoveDeviceIntegrationTest { @ParameterizedTest @CsvSource({ - "10_000,1000,,false", // no pending messages - "10_000,1000,1000,true", // pending message at device creation - "10_000,1000,5999,true", // pending message right before device creation + fudge factor - "10_000,1000,6000,false", // pending message at device creation + fudge factor - "3000,5000,4000,false", // pending message after current time but before device creation + "10_000,,false", // no pending messages + "10_000,9999,true", // pending message right before now + "10_000,10_000,false", // pending message at now + "10_000,10_001,false", // pending message after now }) - void waitForMessageFetch(long currentTime, long deviceCreation, Long oldestMessage, boolean shouldWait) + void waitForMessageFetch(long currentTime, Long oldestMessage, boolean shouldWait) throws InterruptedException { final String number = PhoneNumberUtil.getInstance().format( PhoneNumberUtil.getInstance().getExampleNumber("US"), @@ -564,7 +561,6 @@ public class AddRemoveDeviceIntegrationTest { final String linkDeviceToken = accountsManager.generateLinkDeviceToken(UUID.randomUUID()); final String linkDeviceTokenIdentifier = AccountsManager.getLinkDeviceTokenIdentifier(linkDeviceToken); - clock.pin(Instant.ofEpochMilli(deviceCreation)); final Pair updatedAccountAndDevice = accountsManager.addDevice(account, new DeviceSpec( "device-name".getBytes(StandardCharsets.UTF_8), @@ -583,8 +579,6 @@ public class AddRemoveDeviceIntegrationTest { linkDeviceToken) .join(); - assertEquals(updatedAccountAndDevice.second().getCreated(), deviceCreation); - when(messagesManager.getEarliestUndeliveredTimestampForDevice(account.getUuid(), account.getPrimaryDevice())) .thenReturn(CompletableFuture.completedFuture(Optional.ofNullable(oldestMessage).map(Instant::ofEpochMilli)));