Serialize subscription errors as json

signalapp · Aug 15, 2024 · e4f9f94 · e4f9f94
1 parent 7605462
commit e4f9f94
Show file tree

Hide file tree

Showing 4 changed files with 77 additions and 22 deletions.
diff --git a/...e/src/main/java/org/whispersystems/textsecuregcm/mappers/SubscriptionExceptionMapper.java b/...e/src/main/java/org/whispersystems/textsecuregcm/mappers/SubscriptionExceptionMapper.java
@@ -5,11 +5,9 @@
 
 package org.whispersystems.textsecuregcm.mappers;
 
-import javax.ws.rs.BadRequestException;
-import javax.ws.rs.ClientErrorException;
-import javax.ws.rs.ForbiddenException;
-import javax.ws.rs.InternalServerErrorException;
-import javax.ws.rs.NotFoundException;
+import io.dropwizard.jersey.errors.ErrorMessage;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.ext.ExceptionMapper;
 import org.whispersystems.textsecuregcm.storage.SubscriptionException;
@@ -18,14 +16,24 @@ public class SubscriptionExceptionMapper implements ExceptionMapper<Subscription
 
   @Override
   public Response toResponse(final SubscriptionException exception) {
-    return switch (exception) {
-      case SubscriptionException.NotFound e -> new NotFoundException(e.getMessage(), e.getCause()).getResponse();
-      case SubscriptionException.Forbidden e -> new ForbiddenException(e.getMessage(), e.getCause()).getResponse();
-      case SubscriptionException.InvalidArguments e ->
-          new BadRequestException(e.getMessage(), e.getCause()).getResponse();
-      case SubscriptionException.ProcessorConflict e ->
-          new ClientErrorException("existing processor does not match", Response.Status.CONFLICT).getResponse();
-      default -> new InternalServerErrorException(exception.getMessage(), exception.getCause()).getResponse();
-    };
+    final Response.Status status = (switch (exception) {
+      case SubscriptionException.NotFound e -> Response.Status.NOT_FOUND;
+      case SubscriptionException.Forbidden e -> Response.Status.FORBIDDEN;
+      case SubscriptionException.InvalidArguments e -> Response.Status.BAD_REQUEST;
+      case SubscriptionException.ProcessorConflict e -> Response.Status.CONFLICT;
+      default -> Response.Status.INTERNAL_SERVER_ERROR;
+    });
+
+    // If the SubscriptionException came with suitable error message, include that in the response body. Otherwise,
+    // don't provide any message to the WebApplicationException constructor so the response includes the default
+    // HTTP error message for the status.
+    final WebApplicationException wae = exception.errorDetail()
+        .map(errorMessage -> new WebApplicationException(errorMessage, exception, Response.status(status).build()))
+        .orElseGet(() -> new WebApplicationException(exception, Response.status(status).build()));
+
+    return Response
+        .fromResponse(wae.getResponse())
+        .type(MediaType.APPLICATION_JSON_TYPE)
+        .entity(new ErrorMessage(wae.getResponse().getStatus(), wae.getLocalizedMessage())).build();
   }
 }
diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/SubscriptionException.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/SubscriptionException.java
@@ -4,30 +4,51 @@
  */
 package org.whispersystems.textsecuregcm.storage;
 
+import java.util.Optional;
+import javax.annotation.Nullable;
+
 public class SubscriptionException extends Exception {
-  public SubscriptionException(String message, Exception cause) {
-    super(message, cause);
+
+  private @Nullable String errorDetail;
+
+  public SubscriptionException(Exception cause) {
+    this(cause, null);
+  }
+
+  SubscriptionException(Exception cause, String errorDetail) {
+    super(cause);
+    this.errorDetail = errorDetail;
+  }
+
+  /**
+   * @return An error message suitable to include in a client response
+   */
+  public Optional<String> errorDetail() {
+    return Optional.ofNullable(errorDetail);
   }
 
   public static class NotFound extends SubscriptionException {
+
     public NotFound() {
-      super(null, null);
+      super(null);
     }
 
     public NotFound(Exception cause) {
-      super(null, cause);
+      super(cause);
     }
   }
 
   public static class Forbidden extends SubscriptionException {
+
     public Forbidden(final String message) {
-      super(message, null);
+      super(null, message);
     }
   }
 
   public static class InvalidArguments extends SubscriptionException {
+
     public InvalidArguments(final String message, final Exception cause) {
-      super(message, cause);
+      super(cause, message);
     }
   }
 
@@ -45,7 +66,7 @@ public PaymentRequiresAction() {
 
   public static class ProcessorConflict extends SubscriptionException {
     public ProcessorConflict(final String message) {
-      super(message, null);
+      super(null, message);
     }
   }
 }
diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/SubscriptionManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/SubscriptionManager.java
@@ -263,7 +263,7 @@ public <T extends SubscriptionPaymentProcessor, R> CompletableFuture<R> addPayme
           final String customerId = updatedRecord.getProcessorCustomer()
               .filter(pc -> pc.processor().equals(subscriptionPaymentProcessor.getProvider()))
               .orElseThrow(() ->
-                  ExceptionUtils.wrap(new SubscriptionException("record should not be missing customer", null)))
+                  ExceptionUtils.wrap(new SubscriptionException(null, "record should not be missing customer")))
               .customerId();
           return paymentSetupFunction.apply(subscriptionPaymentProcessor, customerId);
         });

diff --git a/...rc/test/java/org/whispersystems/textsecuregcm/controllers/SubscriptionControllerTest.java b/...rc/test/java/org/whispersystems/textsecuregcm/controllers/SubscriptionControllerTest.java
@@ -457,8 +457,34 @@ Subscriptions.KEY_ACCESSED_AT, n(Instant.now().getEpochSecond())
               String.format("/v1/subscription/%s/level/%s/%s/%s", subscriberId, level, currency, idempotencyKey))
           .request()
           .put(Entity.json(""));
+      assertThat(response.getStatus()).isEqualTo(409);
+      assertThat(response.readEntity(Map.class)).containsOnlyKeys("code", "message");
+    }
+
+    @Test
+    void wrongProcessor() {
+      final byte[] subscriberUserAndKey = new byte[32];
+      Arrays.fill(subscriberUserAndKey, (byte) 1);
+      subscriberId = Base64.getEncoder().encodeToString(subscriberUserAndKey);
+
+      final ProcessorCustomer processorCustomer = new ProcessorCustomer("testCustomerId", PaymentProvider.BRAINTREE);
+      final Map<String, AttributeValue> dynamoItem = Map.of(Subscriptions.KEY_PASSWORD, b(new byte[16]),
+          Subscriptions.KEY_CREATED_AT, n(Instant.now().getEpochSecond()),
+          Subscriptions.KEY_ACCESSED_AT, n(Instant.now().getEpochSecond()),
+          Subscriptions.KEY_PROCESSOR_ID_CUSTOMER_ID, b(processorCustomer.toDynamoBytes())
+      );
+      final Subscriptions.Record record = Subscriptions.Record.from(
+          Arrays.copyOfRange(subscriberUserAndKey, 0, 16), dynamoItem);
+      when(SUBSCRIPTIONS.get(eq(Arrays.copyOfRange(subscriberUserAndKey, 0, 16)), any()))
+          .thenReturn(CompletableFuture.completedFuture(Subscriptions.GetResult.found(record)));
+
+      final Response response = RESOURCE_EXTENSION
+          .target(String.format("/v1/subscription/%s/create_payment_method", subscriberId))
+          .request()
+          .post(Entity.json(""));
 
       assertThat(response.getStatus()).isEqualTo(409);
+      assertThat(response.readEntity(Map.class)).containsOnlyKeys("code", "message");
     }
 
     @Test