From baaae6cd9f22b0fdcc0e4315c7992c298e46d122 Mon Sep 17 00:00:00 2001 From: Ravi Khadiwala Date: Tue, 8 Feb 2022 18:30:58 -0600 Subject: [PATCH] Add `@NotNull` to controller args where appropriate Notably, `@Valid` doesn't imply `@NotNull` --- .../controllers/AccountController.java | 15 +++++++------- .../controllers/DeviceController.java | 2 +- .../controllers/DonationController.java | 5 +++-- .../controllers/KeysController.java | 3 ++- .../controllers/MessageController.java | 7 ++++--- .../controllers/ProfileController.java | 3 ++- .../controllers/ProvisioningController.java | 3 ++- .../controllers/RemoteConfigController.java | 3 ++- .../controllers/SubscriptionController.java | 9 ++++++--- .../SubscriptionControllerTest.java | 20 +++++++++++++++++++ 10 files changed, 50 insertions(+), 20 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 a812b630e..2e175f286 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -29,6 +29,7 @@ import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; import javax.servlet.http.HttpServletRequest; import javax.validation.Valid; +import javax.validation.constraints.NotNull; import javax.ws.rs.BadRequestException; import javax.ws.rs.Consumes; import javax.ws.rs.DELETE; @@ -339,7 +340,7 @@ public class AccountController { @HeaderParam("X-Signal-Agent") String signalAgent, @HeaderParam("User-Agent") String userAgent, @QueryParam("transfer") Optional availableForTransfer, - @Valid AccountAttributes accountAttributes) + @NotNull @Valid AccountAttributes accountAttributes) throws RateLimitExceededException, InterruptedException { String number = authorizationHeader.getUsername(); @@ -399,7 +400,7 @@ public class AccountController { @PUT @Path("/number") @Produces(MediaType.APPLICATION_JSON) - public AccountIdentityResponse changeNumber(@Auth final AuthenticatedAccount authenticatedAccount, @Valid final ChangePhoneNumberRequest request) + public AccountIdentityResponse changeNumber(@Auth final AuthenticatedAccount authenticatedAccount, @NotNull @Valid final ChangePhoneNumberRequest request) throws RateLimitExceededException, InterruptedException, ImpossiblePhoneNumberException, NonNormalizedPhoneNumberException { final Account updatedAccount; @@ -457,7 +458,7 @@ public class AccountController { @Consumes(MediaType.APPLICATION_JSON) @ChangesDeviceEnabledState public void setGcmRegistrationId(@Auth DisabledPermittedAuthenticatedAccount disabledPermittedAuth, - @Valid GcmRegistrationId registrationId) { + @NotNull @Valid GcmRegistrationId registrationId) { Account account = disabledPermittedAuth.getAccount(); Device device = disabledPermittedAuth.getAuthenticatedDevice(); @@ -495,7 +496,7 @@ public class AccountController { @Consumes(MediaType.APPLICATION_JSON) @ChangesDeviceEnabledState public void setApnRegistrationId(@Auth DisabledPermittedAuthenticatedAccount disabledPermittedAuth, - @Valid ApnRegistrationId registrationId) { + @NotNull @Valid ApnRegistrationId registrationId) { Account account = disabledPermittedAuth.getAccount(); Device device = disabledPermittedAuth.getAuthenticatedDevice(); @@ -530,7 +531,7 @@ public class AccountController { @PUT @Produces(MediaType.APPLICATION_JSON) @Path("/registration_lock") - public void setRegistrationLock(@Auth AuthenticatedAccount auth, @Valid RegistrationLock accountLock) { + public void setRegistrationLock(@Auth AuthenticatedAccount auth, @NotNull @Valid RegistrationLock accountLock) { AuthenticationCredentials credentials = new AuthenticationCredentials(accountLock.getRegistrationLock()); accounts.update(auth.getAccount(), @@ -547,7 +548,7 @@ public class AccountController { @Timed @PUT @Path("/name/") - public void setName(@Auth DisabledPermittedAuthenticatedAccount disabledPermittedAuth, @Valid DeviceName deviceName) { + public void setName(@Auth DisabledPermittedAuthenticatedAccount disabledPermittedAuth, @NotNull @Valid DeviceName deviceName) { Account account = disabledPermittedAuth.getAccount(); Device device = disabledPermittedAuth.getAuthenticatedDevice(); accounts.updateDevice(account, device.getId(), d -> d.setName(deviceName.getDeviceName())); @@ -567,7 +568,7 @@ public class AccountController { @ChangesDeviceEnabledState public void setAccountAttributes(@Auth DisabledPermittedAuthenticatedAccount disabledPermittedAuth, @HeaderParam("X-Signal-Agent") String userAgent, - @Valid AccountAttributes attributes) { + @NotNull @Valid AccountAttributes attributes) { Account account = disabledPermittedAuth.getAccount(); long deviceId = disabledPermittedAuth.getAuthenticatedDevice().getId(); 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 bf2bdccde..50b08e2d4 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java @@ -224,7 +224,7 @@ public class DeviceController { @Timed @PUT @Path("/capabilities") - public void setCapabiltities(@Auth AuthenticatedAccount auth, @Valid DeviceCapabilities capabilities) { + public void setCapabiltities(@Auth AuthenticatedAccount auth, @NotNull @Valid DeviceCapabilities capabilities) { assert (auth.getAuthenticatedDevice() != null); final long deviceId = auth.getAuthenticatedDevice().getId(); accounts.updateDevice(auth.getAccount(), deviceId, d -> d.setCapabilities(capabilities)); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DonationController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DonationController.java index ee2eecaaf..ca76d79af 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DonationController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DonationController.java @@ -35,6 +35,7 @@ import java.util.concurrent.ForkJoinPool.ManagedBlocker; import java.util.function.Function; import javax.annotation.Nonnull; import javax.validation.Valid; +import javax.validation.constraints.NotNull; import javax.ws.rs.Consumes; import javax.ws.rs.POST; import javax.ws.rs.Path; @@ -124,7 +125,7 @@ public class DonationController { @Produces({MediaType.APPLICATION_JSON, MediaType.TEXT_PLAIN}) public CompletionStage redeemReceipt( @Auth final AuthenticatedAccount auth, - @Valid final RedeemReceiptRequest request) { + @NotNull @Valid final RedeemReceiptRequest request) { return CompletableFuture.supplyAsync(() -> { ReceiptCredentialPresentation receiptCredentialPresentation; try { @@ -192,7 +193,7 @@ public class DonationController { @Path("/authorize-apple-pay") @Consumes(MediaType.APPLICATION_JSON) @Produces(MediaType.APPLICATION_JSON) - public CompletableFuture getApplePayAuthorization(@Auth AuthenticatedAccount auth, @Valid ApplePayAuthorizationRequest request) { + public CompletableFuture getApplePayAuthorization(@Auth AuthenticatedAccount auth, @NotNull @Valid ApplePayAuthorizationRequest request) { if (!supportedCurrencies.contains(request.getCurrency())) { return CompletableFuture.completedFuture(Response.status(422).build()); } 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 e3d7bf41f..7dedebd50 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java @@ -18,6 +18,7 @@ import java.util.Map; import java.util.Optional; import java.util.UUID; import javax.validation.Valid; +import javax.validation.constraints.NotNull; import javax.ws.rs.Consumes; import javax.ws.rs.GET; import javax.ws.rs.HeaderParam; @@ -85,7 +86,7 @@ public class KeysController { @Consumes(MediaType.APPLICATION_JSON) @ChangesDeviceEnabledState public void setKeys(@Auth final DisabledPermittedAuthenticatedAccount disabledPermittedAuth, - @Valid final PreKeyState preKeys, + @NotNull @Valid final PreKeyState preKeys, @QueryParam("identity") final Optional identityType) { Account account = disabledPermittedAuth.getAccount(); Device device = disabledPermittedAuth.getAuthenticatedDevice(); 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 8d0695e23..9e73b7241 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/MessageController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/MessageController.java @@ -38,6 +38,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.Nonnull; import javax.validation.Valid; +import javax.validation.constraints.NotNull; import javax.ws.rs.BadRequestException; import javax.ws.rs.Consumes; import javax.ws.rs.DELETE; @@ -167,7 +168,7 @@ public class MessageController { @HeaderParam("User-Agent") String userAgent, @HeaderParam("X-Forwarded-For") String forwardedFor, @PathParam("destination") UUID destinationUuid, - @Valid IncomingMessageList messages) + @NotNull @Valid IncomingMessageList messages) throws RateLimitExceededException, RateLimitChallengeException { if (source.isEmpty() && accessKey.isEmpty()) { @@ -274,7 +275,7 @@ public class MessageController { @PathParam("destination") UUID destinationUuid, @QueryParam("online") boolean online, @QueryParam("ts") long timestamp, - @Valid IncomingDeviceMessage[] messages) + @NotNull @Valid IncomingDeviceMessage[] messages) throws RateLimitExceededException, RateLimitChallengeException { if (source.isEmpty() && accessKey.isEmpty()) { @@ -374,7 +375,7 @@ public class MessageController { @HeaderParam("X-Forwarded-For") String forwardedFor, @QueryParam("online") boolean online, @QueryParam("ts") long timestamp, - @Valid MultiRecipientMessage multiRecipientMessage) { + @NotNull @Valid MultiRecipientMessage multiRecipientMessage) { Map uuidToAccountMap = Arrays.stream(multiRecipientMessage.getRecipients()) .map(Recipient::getUuid) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java index 6b689a265..ca7683319 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java @@ -23,6 +23,7 @@ import java.util.UUID; import java.util.function.Function; import java.util.stream.Collectors; import javax.validation.Valid; +import javax.validation.constraints.NotNull; import javax.ws.rs.BadRequestException; import javax.ws.rs.Consumes; import javax.ws.rs.DefaultValue; @@ -148,7 +149,7 @@ public class ProfileController { @PUT @Produces(MediaType.APPLICATION_JSON) @Consumes(MediaType.APPLICATION_JSON) - public Response setProfile(@Auth AuthenticatedAccount auth, @Valid CreateProfileRequest request) { + public Response setProfile(@Auth AuthenticatedAccount auth, @NotNull @Valid CreateProfileRequest request) { if (StringUtils.isNotBlank(request.getPaymentAddress())) { final boolean hasDisallowedPrefix = dynamicConfigurationManager.getConfiguration().getPaymentsConfiguration().getDisallowedPrefixes().stream() diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProvisioningController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProvisioningController.java index a7de6788c..6310ded13 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProvisioningController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProvisioningController.java @@ -9,6 +9,7 @@ import com.codahale.metrics.annotation.Timed; import io.dropwizard.auth.Auth; import java.util.Base64; import javax.validation.Valid; +import javax.validation.constraints.NotNull; import javax.ws.rs.Consumes; import javax.ws.rs.PUT; import javax.ws.rs.Path; @@ -41,7 +42,7 @@ public class ProvisioningController { @Produces(MediaType.APPLICATION_JSON) public void sendProvisioningMessage(@Auth AuthenticatedAccount auth, @PathParam("destination") String destinationName, - @Valid ProvisioningMessage message) + @NotNull @Valid ProvisioningMessage message) throws RateLimitExceededException { rateLimiters.getMessagesLimiter().validate(auth.getAccount().getUuid()); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RemoteConfigController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RemoteConfigController.java index 2127cf0d7..cb7e3ed99 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RemoteConfigController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RemoteConfigController.java @@ -19,6 +19,7 @@ import java.util.UUID; import java.util.stream.Collectors; import java.util.stream.Stream; import javax.validation.Valid; +import javax.validation.constraints.NotNull; import javax.ws.rs.Consumes; import javax.ws.rs.DELETE; import javax.ws.rs.GET; @@ -79,7 +80,7 @@ public class RemoteConfigController { @PUT @Consumes(MediaType.APPLICATION_JSON) @Produces(MediaType.APPLICATION_JSON) - public void set(@HeaderParam("Config-Token") String configToken, @Valid RemoteConfig config) { + public void set(@HeaderParam("Config-Token") String configToken, @NotNull @Valid RemoteConfig config) { if (!isAuthorized(configToken)) { throw new WebApplicationException(Response.Status.UNAUTHORIZED); } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/SubscriptionController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/SubscriptionController.java index 62a6722d8..aad74f77b 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/SubscriptionController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/SubscriptionController.java @@ -35,6 +35,7 @@ import javax.crypto.spec.SecretKeySpec; import javax.validation.Valid; import javax.validation.constraints.Min; import javax.validation.constraints.NotEmpty; +import javax.validation.constraints.NotNull; import javax.ws.rs.BadRequestException; import javax.ws.rs.Consumes; import javax.ws.rs.DELETE; @@ -512,7 +513,7 @@ public class SubscriptionController { @Path("/boost/create") @Consumes(MediaType.APPLICATION_JSON) @Produces(MediaType.APPLICATION_JSON) - public CompletableFuture createBoostPaymentIntent(CreateBoostRequest request) { + public CompletableFuture createBoostPaymentIntent(@NotNull @Valid CreateBoostRequest request) { return stripeManager.createPaymentIntent(request.getCurrency(), request.getAmount()) .thenApply(paymentIntent -> Response.ok(new CreateBoostResponse(paymentIntent.getClientSecret())).build()); } @@ -530,10 +531,12 @@ public class SubscriptionController { this.receiptCredentialRequest = receiptCredentialRequest; } + @NotNull public String getPaymentIntentId() { return paymentIntentId; } + @NotNull public byte[] getReceiptCredentialRequest() { return receiptCredentialRequest; } @@ -559,7 +562,7 @@ public class SubscriptionController { @Path("/boost/receipt_credentials") @Consumes(MediaType.APPLICATION_JSON) @Produces(MediaType.APPLICATION_JSON) - public CompletableFuture createBoostReceiptCredentials(CreateBoostReceiptCredentialsRequest request) { + public CompletableFuture createBoostReceiptCredentials(@NotNull @Valid CreateBoostReceiptCredentialsRequest request) { return stripeManager.getPaymentIntent(request.getPaymentIntentId()) .thenCompose(paymentIntent -> { if (paymentIntent == null) { @@ -745,7 +748,7 @@ public class SubscriptionController { public CompletableFuture createSubscriptionReceiptCredentials( @Auth Optional authenticatedAccount, @PathParam("subscriberId") String subscriberId, - @Valid GetReceiptCredentialsRequest request) { + @NotNull @Valid GetReceiptCredentialsRequest request) { RequestData requestData = RequestData.process(authenticatedAccount, subscriberId, clock); return subscriptionManager.get(requestData.subscriberUser, requestData.hmac) .thenApply(this::requireRecordFromGetResult) diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/SubscriptionControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/SubscriptionControllerTest.java index 53fc8770c..a155759bb 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/SubscriptionControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/SubscriptionControllerTest.java @@ -34,6 +34,7 @@ import org.whispersystems.textsecuregcm.configuration.BoostConfiguration; import org.whispersystems.textsecuregcm.configuration.SubscriptionConfiguration; import org.whispersystems.textsecuregcm.configuration.SubscriptionLevelConfiguration; import org.whispersystems.textsecuregcm.configuration.SubscriptionPriceConfiguration; +import org.whispersystems.textsecuregcm.controllers.SubscriptionController.CreateBoostReceiptCredentialsRequest; import org.whispersystems.textsecuregcm.controllers.SubscriptionController.GetLevelsResponse; import org.whispersystems.textsecuregcm.entities.Badge; import org.whispersystems.textsecuregcm.entities.BadgeSvg; @@ -42,6 +43,8 @@ import org.whispersystems.textsecuregcm.storage.SubscriptionManager; import org.whispersystems.textsecuregcm.stripe.StripeManager; import org.whispersystems.textsecuregcm.tests.util.AuthHelper; import org.whispersystems.textsecuregcm.util.SystemMapper; +import javax.ws.rs.client.Entity; +import javax.ws.rs.core.Response; @ExtendWith(DropwizardExtensionsSupport.class) class SubscriptionControllerTest { @@ -74,6 +77,23 @@ class SubscriptionControllerTest { BADGE_TRANSLATOR, LEVEL_TRANSLATOR); } + @Test + void createBoostReceiptInvalid() { + final Response response = RESOURCE_EXTENSION.target("/v1/subscription/boost/receipt_credentials") + .request() + // invalid, request body should have receiptCredentialRequest + .post(Entity.json("{\"paymentIntentId\": \"foo\"}")); + assertThat(response.getStatus()).isEqualTo(422); + } + + @Test + void createBoostReceiptNoRequest() { + final Response response = RESOURCE_EXTENSION.target("/v1/subscription/boost/receipt_credentials") + .request() + .post(Entity.json("")); + assertThat(response.getStatus()).isEqualTo(422); + } + @Test void getLevels() { when(SUBSCRIPTION_CONFIG.getLevels()).thenReturn(Map.of(