From 7764185c5725c39c6be4e2c471b1e0466721624f Mon Sep 17 00:00:00 2001 From: Jonathan Klabunde Tomer <125505367+jkt-signal@users.noreply.github.com> Date: Tue, 14 Nov 2023 21:57:25 -0800 Subject: [PATCH] return explicit Response rather than Void from async controllers with no expected body content --- .../textsecuregcm/controllers/AccountController.java | 8 ++++---- .../textsecuregcm/controllers/KeysController.java | 10 ++++++---- .../textsecuregcm/controllers/MessageController.java | 5 +++-- .../org/whispersystems/textsecuregcm/util/Util.java | 10 ++++++++++ .../controllers/AccountControllerTest.java | 7 ++++++- .../textsecuregcm/controllers/KeysControllerTest.java | 5 +++-- .../controllers/MessageControllerTest.java | 7 ++++--- .../textsecuregcm/util/CompletableFutureTestUtil.java | 6 ++++++ 8 files changed, 42 insertions(+), 16 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java index 25c893e84..f20af9f30 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -274,9 +274,9 @@ private AccountIdentityResponse buildAccountIdentityResponse(AccountAndAuthentic ) @ApiResponse(responseCode = "204", description = "Username successfully deleted.", useReturnTypeSchema = true) @ApiResponse(responseCode = "401", description = "Account authentication check failed.") - public CompletableFuture deleteUsernameHash(@Auth final AuthenticatedAccount auth) { + public CompletableFuture deleteUsernameHash(@Auth final AuthenticatedAccount auth) { return accounts.clearUsernameHash(auth.getAccount()) - .thenRun(Util.NOOP); + .thenApply(Util.ASYNC_EMPTY_RESPONSE); } @PUT @@ -518,8 +518,8 @@ public Response accountExists( @DELETE @Path("/me") - public CompletableFuture deleteAccount(@Auth DisabledPermittedAuthenticatedAccount auth) throws InterruptedException { - return accounts.delete(auth.getAccount(), AccountsManager.DeletionReason.USER_REQUEST); + public CompletableFuture deleteAccount(@Auth DisabledPermittedAuthenticatedAccount auth) throws InterruptedException { + return accounts.delete(auth.getAccount(), AccountsManager.DeletionReason.USER_REQUEST).thenApply(Util.ASYNC_EMPTY_RESPONSE); } private void clearUsernameLink(final Account account) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java index 7de1cb35e..4d09208d1 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java @@ -121,7 +121,7 @@ public CompletableFuture getStatus(@Auth final AuthenticatedAccount @ApiResponse(responseCode = "401", description = "Account authentication check failed.") @ApiResponse(responseCode = "403", description = "Attempt to change identity key from a non-primary device.") @ApiResponse(responseCode = "422", description = "Invalid request format.") - public CompletableFuture setKeys(@Auth final DisabledPermittedAuthenticatedAccount disabledPermittedAuth, + public CompletableFuture setKeys(@Auth final DisabledPermittedAuthenticatedAccount disabledPermittedAuth, @RequestBody @NotNull @Valid final PreKeyState preKeys, @Parameter(allowEmptyValue=true) @@ -189,7 +189,8 @@ public CompletableFuture setKeys(@Auth final DisabledPermittedAuthenticate } return keys.store(getIdentifier(account, identityType), device.getId(), - preKeys.getPreKeys(), preKeys.getPqPreKeys(), preKeys.getSignedPreKey(), preKeys.getPqLastResortPreKey()); + preKeys.getPreKeys(), preKeys.getPqPreKeys(), preKeys.getSignedPreKey(), preKeys.getPqLastResortPreKey()) + .thenApply(Util.ASYNC_EMPTY_RESPONSE); } @GET @@ -299,7 +300,7 @@ public PreKeyResponse getDeviceKeys(@Auth Optional auth, @ApiResponse(responseCode = "200", description = "Indicates that new prekey was successfully stored.") @ApiResponse(responseCode = "401", description = "Account authentication check failed.") @ApiResponse(responseCode = "422", description = "Invalid request format.") - public CompletableFuture setSignedKey(@Auth final AuthenticatedAccount auth, + public CompletableFuture setSignedKey(@Auth final AuthenticatedAccount auth, @Valid final ECSignedPreKey signedPreKey, @QueryParam("identity") final Optional identityType) { @@ -314,7 +315,8 @@ public CompletableFuture setSignedKey(@Auth final AuthenticatedAccount aut }); return keys.storeEcSignedPreKeys(getIdentifier(auth.getAccount(), identityType), - Map.of(device.getId(), signedPreKey)); + Map.of(device.getId(), signedPreKey)) + .thenApply(Util.ASYNC_EMPTY_RESPONSE); } private static boolean usePhoneNumberIdentity(final Optional identityType) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/MessageController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/MessageController.java index ae9f8cd22..659ce2921 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/MessageController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/MessageController.java @@ -581,7 +581,7 @@ private static long estimateMessageListSizeBytes(final OutgoingMessageEntityList @Timed @DELETE @Path("/uuid/{uuid}") - public CompletableFuture removePendingMessage(@Auth AuthenticatedAccount auth, @PathParam("uuid") UUID uuid) { + public CompletableFuture removePendingMessage(@Auth AuthenticatedAccount auth, @PathParam("uuid") UUID uuid) { return messagesManager.delete( auth.getAccount().getUuid(), auth.getAuthenticatedDevice().getId(), @@ -603,7 +603,8 @@ public CompletableFuture removePendingMessage(@Auth AuthenticatedAccount a } } }); - }); + }) + .thenApply(Util.ASYNC_EMPTY_RESPONSE); } @Timed diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/util/Util.java b/service/src/main/java/org/whispersystems/textsecuregcm/util/Util.java index 6f9e8d912..19ebbc3d3 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/util/Util.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/util/Util.java @@ -16,8 +16,12 @@ import java.util.Locale.LanguageRange; import java.util.Optional; import java.util.concurrent.TimeUnit; +import java.util.function.Function; import java.util.regex.Matcher; import java.util.regex.Pattern; + +import javax.ws.rs.core.Response; + import org.apache.commons.lang3.StringUtils; public class Util { @@ -28,6 +32,12 @@ public class Util { public static final Runnable NOOP = () -> {}; + // Use `CompletableFuture#thenApply(ASYNC_EMPTY_RESPONSE) to convert futures to + // CompletableFuture instead of using NOOP to convert them to CompletableFuture + // for jersey controllers; https://github.com/eclipse-ee4j/jersey/issues/3901 causes controllers + // returning Void futures to behave differently than synchronous controllers returning void + public static final Function ASYNC_EMPTY_RESPONSE = ignored -> Response.noContent().build(); + /** * Checks that the given number is a valid, E164-normalized phone number. * 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 d39db373c..4d1194b20 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java @@ -38,6 +38,7 @@ import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; import java.util.stream.Stream; import javax.ws.rs.client.Entity; import javax.ws.rs.client.Invocation; @@ -90,6 +91,7 @@ import org.whispersystems.textsecuregcm.storage.UsernameReservationNotFoundException; import org.whispersystems.textsecuregcm.tests.util.AccountsHelper; import org.whispersystems.textsecuregcm.tests.util.AuthHelper; +import org.whispersystems.textsecuregcm.util.CompletableFutureTestUtil; import org.whispersystems.textsecuregcm.util.MockUtils; import org.whispersystems.textsecuregcm.util.SystemMapper; import org.whispersystems.textsecuregcm.util.UsernameHashZkProofVerifier; @@ -737,7 +739,7 @@ void testCommitUsernameHashWithInvalidProof() throws BaseUsernameException { @Test void testDeleteUsername() { when(accountsManager.clearUsernameHash(any())) - .thenAnswer(invocation -> CompletableFuture.completedFuture(invocation.getArgument(0))); + .thenAnswer(invocation -> CompletableFutureTestUtil.almostCompletedFuture(invocation.getArgument(0))); Response response = resources.getJerseyTest() @@ -746,6 +748,7 @@ void testDeleteUsername() { .header(HttpHeaders.AUTHORIZATION, AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) .delete(); + assertThat(response.readEntity(String.class)).isEqualTo(""); assertThat(response.getStatus()).isEqualTo(204); verify(accountsManager).clearUsernameHash(AuthHelper.VALID_ACCOUNT); } @@ -828,6 +831,8 @@ void testSetAccountAttributesBadUnidentifiedKeyLength() { @Test void testDeleteAccount() { + when(accountsManager.delete(any(), any())).thenReturn(CompletableFutureTestUtil.almostCompletedFuture(null)); + Response response = resources.getJerseyTest() .target("/v1/accounts/me") diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/KeysControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/KeysControllerTest.java index bb9e38989..fad2aa397 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/KeysControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/KeysControllerTest.java @@ -74,6 +74,7 @@ import org.whispersystems.textsecuregcm.tests.util.AuthHelper; import org.whispersystems.textsecuregcm.tests.util.KeysHelper; import org.whispersystems.textsecuregcm.util.ByteArrayAdapter; +import org.whispersystems.textsecuregcm.util.CompletableFutureTestUtil; @ExtendWith(DropwizardExtensionsSupport.class) class KeysControllerTest { @@ -235,9 +236,9 @@ void setup() { when(rateLimiters.getPreKeysLimiter()).thenReturn(rateLimiter); - when(KEYS.store(any(), anyByte(), any(), any(), any(), any())).thenReturn(CompletableFuture.completedFuture(null)); + when(KEYS.store(any(), anyByte(), any(), any(), any(), any())).thenReturn(CompletableFutureTestUtil.almostCompletedFuture(null)); when(KEYS.getEcSignedPreKey(any(), anyByte())).thenReturn(CompletableFuture.completedFuture(Optional.empty())); - when(KEYS.storeEcSignedPreKeys(any(), any())).thenReturn(CompletableFuture.completedFuture(null)); + when(KEYS.storeEcSignedPreKeys(any(), any())).thenReturn(CompletableFutureTestUtil.almostCompletedFuture(null)); when(KEYS.takeEC(EXISTS_UUID, sampleDeviceId)).thenReturn( CompletableFuture.completedFuture(Optional.of(SAMPLE_KEY))); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java index 4884cbe9b..d7db49867 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java @@ -121,6 +121,7 @@ import org.whispersystems.textsecuregcm.tests.util.AccountsHelper; import org.whispersystems.textsecuregcm.tests.util.AuthHelper; import org.whispersystems.textsecuregcm.tests.util.KeysHelper; +import org.whispersystems.textsecuregcm.util.CompletableFutureTestUtil; import org.whispersystems.textsecuregcm.util.Pair; import org.whispersystems.textsecuregcm.util.SystemMapper; import org.whispersystems.textsecuregcm.util.UUIDUtil; @@ -610,19 +611,19 @@ void testDeleteMessages() { UUID uuid1 = UUID.randomUUID(); when(messagesManager.delete(AuthHelper.VALID_UUID, (byte) 1, uuid1, null)) .thenReturn( - CompletableFuture.completedFuture(Optional.of(generateEnvelope(uuid1, Envelope.Type.CIPHERTEXT_VALUE, + CompletableFutureTestUtil.almostCompletedFuture(Optional.of(generateEnvelope(uuid1, Envelope.Type.CIPHERTEXT_VALUE, timestamp, sourceUuid, (byte) 1, AuthHelper.VALID_UUID, null, "hi".getBytes(), 0)))); UUID uuid2 = UUID.randomUUID(); when(messagesManager.delete(AuthHelper.VALID_UUID, (byte) 1, uuid2, null)) .thenReturn( - CompletableFuture.completedFuture(Optional.of(generateEnvelope( + CompletableFutureTestUtil.almostCompletedFuture(Optional.of(generateEnvelope( uuid2, Envelope.Type.SERVER_DELIVERY_RECEIPT_VALUE, System.currentTimeMillis(), sourceUuid, (byte) 1, AuthHelper.VALID_UUID, null, null, 0)))); UUID uuid3 = UUID.randomUUID(); when(messagesManager.delete(AuthHelper.VALID_UUID, (byte) 1, uuid3, null)) - .thenReturn(CompletableFuture.completedFuture(Optional.empty())); + .thenReturn(CompletableFutureTestUtil.almostCompletedFuture(Optional.empty())); UUID uuid4 = UUID.randomUUID(); when(messagesManager.delete(AuthHelper.VALID_UUID, (byte) 1, uuid4, null)) diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/util/CompletableFutureTestUtil.java b/service/src/test/java/org/whispersystems/textsecuregcm/util/CompletableFutureTestUtil.java index 6024cbda8..6656bba36 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/util/CompletableFutureTestUtil.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/util/CompletableFutureTestUtil.java @@ -10,6 +10,7 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; +import java.util.concurrent.TimeUnit; public class CompletableFutureTestUtil { @@ -24,4 +25,9 @@ public static void assertFailsWithCause(final Class exp final CompletionException completionException = assertThrows(CompletionException.class, completableFuture::join, message); assertTrue(ExceptionUtils.unwrap(completionException).getClass().isAssignableFrom(expectedCause), message); } + + public static CompletableFuture almostCompletedFuture(T result) { + return new CompletableFuture().completeOnTimeout(result, 5, TimeUnit.MILLISECONDS); + } + }