diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/backup/BackupManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/backup/BackupManager.java index 0b40d43dd..3d61f339f 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/backup/BackupManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/backup/BackupManager.java @@ -59,6 +59,8 @@ public class BackupManager { static final String MESSAGE_BACKUP_NAME = "messageBackup"; + private static final int DEFAULT_LIST_MEDIA_LIMIT = 1000; + private static final String ZK_AUTHN_COUNTER_NAME = MetricsUtil.name(BackupManager.class, "authentication"); private static final String ZK_AUTHZ_FAILURE_COUNTER_NAME = MetricsUtil.name(BackupManager.class, "authorizationFailure"); @@ -458,17 +460,19 @@ public class BackupManager { * * @param backupUser An already ZK authenticated backup user * @param cursor A cursor returned by a previous call that can be used to resume listing - * @param limit The maximum number of list results to return + * @param limit The maximum number of list results to return, or empty to use + * {@link #DEFAULT_LIST_MEDIA_LIMIT} * @return A {@link ListMediaResult} * @throws BackupPermissionException if the credential does not have the correct level */ public ListMediaResult list( final AuthenticatedBackupUser backupUser, final Optional cursor, - final int limit) throws BackupPermissionException { + final Optional limit) throws BackupPermissionException { checkBackupLevel(backupUser, BackupLevel.FREE); - final RemoteStorageManager.ListResult result = - remoteStorageManager.list(cdnMediaDirectory(backupUser), cursor, limit).toCompletableFuture().join(); + final RemoteStorageManager.ListResult result = remoteStorageManager + .list(cdnMediaDirectory(backupUser), cursor, limit.orElse(DEFAULT_LIST_MEDIA_LIMIT)) + .toCompletableFuture().join(); return new ListMediaResult(result .objects() .stream() diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ArchiveController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ArchiveController.java index 04b0e7bf2..cbd624f6a 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ArchiveController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ArchiveController.java @@ -975,7 +975,7 @@ public class ArchiveController { final AuthenticatedBackupUser backupUser = backupManager.authenticateBackupUser(presentation.presentation, signature.signature, userAgent); final BackupManager.ListMediaResult listResult = - backupManager.list(backupUser, cursor, limit.orElse(1000)); + backupManager.list(backupUser, cursor, limit); return new ListResponse(listResult.media() .stream().map(entry -> new StoredMediaObject(entry.cdn(), entry.key(), entry.length())) .toList(), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/grpc/BackupsAnonymousGrpcService.java b/service/src/main/java/org/whispersystems/textsecuregcm/grpc/BackupsAnonymousGrpcService.java index 7578c8a2e..6c5a9d9b4 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/grpc/BackupsAnonymousGrpcService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/grpc/BackupsAnonymousGrpcService.java @@ -265,7 +265,7 @@ public class BackupsAnonymousGrpcService extends SimpleBackupsAnonymousGrpc.Back final BackupManager.ListMediaResult listResult = backupManager.list( backupUser, request.hasCursor() ? Optional.of(request.getCursor()) : Optional.empty(), - request.getLimit()); + request.hasLimit() ? Optional.of(request.getLimit()) : Optional.empty()); final ListMediaResponse.ListResult.Builder builder = ListMediaResponse.ListResult.newBuilder(); for (BackupManager.StorageDescriptorWithLength sd : listResult.media()) { builder.addPage(ListMediaResponse.ListEntry.newBuilder() diff --git a/service/src/main/proto/org/signal/chat/backups.proto b/service/src/main/proto/org/signal/chat/backups.proto index 7448d42b1..92b75064c 100644 --- a/service/src/main/proto/org/signal/chat/backups.proto +++ b/service/src/main/proto/org/signal/chat/backups.proto @@ -473,9 +473,11 @@ message ListMediaRequest { // A cursor returned by a previous call to ListMedia, absent on the first call optional string cursor = 2; - // If provided, the maximum number of entries to return in a page - uint32 limit = 3 [(require.range) = {min: 1, max: 10000}]; + // If provided, the maximum number of entries to return in a page. If absent, + // a server-chosen default is used. + optional uint32 limit = 3 [(require.range) = {min: 1, max: 10000}]; } + message ListMediaResponse { message ListEntry { // The backup cdn where this media object is stored diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupManagerTest.java index be320fa6d..de40d4e39 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupManagerTest.java @@ -667,7 +667,7 @@ public class BackupManagerTest { Optional.of("newCursor") ))); - final BackupManager.ListMediaResult result = backupManager.list(backupUser, cursor, 17); + final BackupManager.ListMediaResult result = backupManager.list(backupUser, cursor, Optional.of(17)); assertThat(result.media()).hasSize(1); assertThat(result.media().getFirst().cdn()).isEqualTo(13); assertThat(result.media().getFirst().key()).isEqualTo( diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ArchiveControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ArchiveControllerTest.java index 4d183aad6..a957dd0b1 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ArchiveControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ArchiveControllerTest.java @@ -562,7 +562,7 @@ public class ArchiveControllerTest { final Optional expectedCursor = cursorProvided ? Optional.of("myCursor") : Optional.empty(); final Optional returnedCursor = cursorReturned ? Optional.of("newCursor") : Optional.empty(); - when(backupManager.list(any(), eq(expectedCursor), eq(17))) + when(backupManager.list(any(), eq(expectedCursor), eq(Optional.of(17)))) .thenReturn(new BackupManager.ListMediaResult( List.of(new BackupManager.StorageDescriptorWithLength(1, mediaId, 100)), returnedCursor diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/grpc/BackupsAnonymousGrpcServiceTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/grpc/BackupsAnonymousGrpcServiceTest.java index 6272b2265..c7f3fd005 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/grpc/BackupsAnonymousGrpcServiceTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/grpc/BackupsAnonymousGrpcServiceTest.java @@ -11,6 +11,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.google.protobuf.ByteString; @@ -290,7 +291,7 @@ class BackupsAnonymousGrpcServiceTest extends final int limit = 17; - when(backupManager.list(any(), eq(expectedCursor), eq(limit))) + when(backupManager.list(any(), eq(expectedCursor), eq(Optional.of(limit)))) .thenReturn(new BackupManager.ListMediaResult( List.of(new BackupManager.StorageDescriptorWithLength(1, mediaId, 100)), returnedCursor)); @@ -463,6 +464,21 @@ class BackupsAnonymousGrpcServiceTest extends .build())); } + @Test + void listMediaUnsetLimit() throws BackupException { + when(backupManager.list(any(), any(), eq(Optional.empty()))) + .thenReturn(new BackupManager.ListMediaResult(List.of(), Optional.empty())); + + // should be able to list without setting a limit + final ListMediaResponse response = unauthenticatedServiceStub().listMedia( + ListMediaRequest.newBuilder() + .setSignedPresentation(signedPresentation(presentation)) + .build()); + + assertThat(response.hasListResult()).isTrue(); + verify(backupManager).list(any(), any(), eq(Optional.empty())); + } + private static AuthenticatedBackupUser backupUser(final byte[] backupId, final BackupCredentialType credentialType, final BackupLevel backupLevel) {