Skip to content

Commit

Permalink
retry hCaptcha errors
Browse files Browse the repository at this point in the history
Co-authored-by: Jon Chambers <[email protected]>
  • Loading branch information
ravi-signal and jon-signal authored Sep 14, 2023
1 parent b594986 commit 0fa8276
Show file tree
Hide file tree
Showing 6 changed files with 140 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,8 @@ public void run(WhisperServerConfiguration config, Environment environment) thro
.scheduledExecutorService(name(getClass(), "secureValueRecoveryServiceRetry-%d")).threads(1).build();
ScheduledExecutorService storageServiceRetryExecutor = environment.lifecycle()
.scheduledExecutorService(name(getClass(), "storageServiceRetry-%d")).threads(1).build();
ScheduledExecutorService hcaptchaRetryExecutor = environment.lifecycle()
.scheduledExecutorService(name(getClass(), "hCaptchaRetry-%d")).threads(1).build();

Scheduler messageDeliveryScheduler = Schedulers.fromExecutorService(
ExecutorServiceMetrics.monitor(Metrics.globalRegistry,
Expand Down Expand Up @@ -569,9 +571,11 @@ public void run(WhisperServerConfiguration config, Environment environment) thro
config.getRecaptchaConfiguration().projectPath(),
config.getRecaptchaConfiguration().credentialConfigurationJson(),
dynamicConfigurationManager);
HttpClient hcaptchaHttpClient = HttpClient.newBuilder().version(HttpClient.Version.HTTP_2)
.connectTimeout(Duration.ofSeconds(10)).build();
HCaptchaClient hCaptchaClient = new HCaptchaClient(config.getHCaptchaConfiguration().apiKey().value(), hcaptchaHttpClient,
HCaptchaClient hCaptchaClient = new HCaptchaClient(
config.getHCaptchaConfiguration().getApiKey().value(),
hcaptchaRetryExecutor,
config.getHCaptchaConfiguration().getCircuitBreaker(),
config.getHCaptchaConfiguration().getRetry(),
dynamicConfigurationManager);
HttpClient shortCodeRetrieverHttpClient = HttpClient.newBuilder().version(HttpClient.Version.HTTP_2)
.connectTimeout(Duration.ofSeconds(10)).build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name;

import com.google.common.annotations.VisibleForTesting;
import io.micrometer.core.instrument.Metrics;
import java.io.IOException;
import java.math.BigDecimal;
Expand All @@ -16,15 +17,25 @@
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.util.Collections;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.CompletionException;
import java.util.concurrent.Executor;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import javax.ws.rs.core.Response;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.whispersystems.textsecuregcm.configuration.CircuitBreakerConfiguration;
import org.whispersystems.textsecuregcm.configuration.RetryConfiguration;
import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicCaptchaConfiguration;
import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration;
import org.whispersystems.textsecuregcm.http.FaultTolerantHttpClient;
import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager;
import org.whispersystems.textsecuregcm.util.ExceptionUtils;
import org.whispersystems.textsecuregcm.util.SystemMapper;

public class HCaptchaClient implements CaptchaClient {
Expand All @@ -34,18 +45,38 @@ public class HCaptchaClient implements CaptchaClient {
private static final String ASSESSMENT_REASON_COUNTER_NAME = name(HCaptchaClient.class, "assessmentReason");
private static final String INVALID_REASON_COUNTER_NAME = name(HCaptchaClient.class, "invalidReason");
private final String apiKey;
private final HttpClient client;
private final FaultTolerantHttpClient client;
private final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager;

public HCaptchaClient(
final String apiKey,
final HttpClient client,
@VisibleForTesting
HCaptchaClient(final String apiKey,
final FaultTolerantHttpClient faultTolerantHttpClient,
final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager) {
this.apiKey = apiKey;
this.client = client;
this.client = faultTolerantHttpClient;
this.dynamicConfigurationManager = dynamicConfigurationManager;
}

public HCaptchaClient(
final String apiKey,
final ScheduledExecutorService retryExecutor,
final CircuitBreakerConfiguration circuitBreakerConfiguration,
final RetryConfiguration retryConfiguration,
final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager) {
this(apiKey,
FaultTolerantHttpClient.newBuilder()
.withName("hcaptcha")
.withCircuitBreaker(circuitBreakerConfiguration)
.withExecutor(Executors.newCachedThreadPool())
.withRetryExecutor(retryExecutor)
.withRetry(retryConfiguration)
.withRetryOnException(ex -> ex instanceof IOException)
.withConnectTimeout(Duration.ofSeconds(10))
.withVersion(HttpClient.Version.HTTP_2)
.build(),
dynamicConfigurationManager);
}

@Override
public String scheme() {
return PREFIX;
Expand Down Expand Up @@ -82,11 +113,12 @@ public AssessmentResult verify(
.POST(HttpRequest.BodyPublishers.ofString(body))
.build();

HttpResponse<String> response;
final HttpResponse<String> response;
try {
response = this.client.send(request, HttpResponse.BodyHandlers.ofString());
} catch (InterruptedException e) {
throw new IOException(e);
response = this.client.sendAsync(request, HttpResponse.BodyHandlers.ofString()).join();
} catch (CompletionException e) {
logger.warn("failed to make http request to hCaptcha: {}", e.getMessage());
throw new IOException(ExceptionUtils.unwrap(e));
}

if (response.statusCode() != Response.Status.OK.getStatusCode()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,35 @@

package org.whispersystems.textsecuregcm.configuration;

import com.fasterxml.jackson.annotation.JsonProperty;
import javax.validation.constraints.NotNull;
import org.whispersystems.textsecuregcm.configuration.secrets.SecretString;

public record HCaptchaConfiguration(@NotNull SecretString apiKey) {
public class HCaptchaConfiguration {

@JsonProperty
@NotNull
SecretString apiKey;

@JsonProperty
@NotNull
CircuitBreakerConfiguration circuitBreaker = new CircuitBreakerConfiguration();

@JsonProperty
@NotNull
RetryConfiguration retry = new RetryConfiguration();


public SecretString getApiKey() {
return apiKey;
}

public CircuitBreakerConfiguration getCircuitBreaker() {
return circuitBreaker;
}

public RetryConfiguration getRetry() {
return retry;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package org.whispersystems.textsecuregcm.http;

import com.google.common.annotations.VisibleForTesting;
import io.github.resilience4j.circuitbreaker.CircuitBreaker;
import io.github.resilience4j.retry.Retry;
import io.github.resilience4j.retry.RetryConfig;
Expand All @@ -18,12 +19,14 @@
import java.util.concurrent.CompletionStage;
import java.util.concurrent.Executor;
import java.util.concurrent.ScheduledExecutorService;
import java.util.function.Predicate;
import java.util.function.Supplier;
import org.glassfish.jersey.SslConfigurator;
import org.whispersystems.textsecuregcm.configuration.CircuitBreakerConfiguration;
import org.whispersystems.textsecuregcm.configuration.RetryConfiguration;
import org.whispersystems.textsecuregcm.util.CertificateUtil;
import org.whispersystems.textsecuregcm.util.CircuitBreakerUtil;
import org.whispersystems.textsecuregcm.util.ExceptionUtils;

public class FaultTolerantHttpClient {

Expand All @@ -40,9 +43,10 @@ public static Builder newBuilder() {
return new Builder();
}

private FaultTolerantHttpClient(String name, HttpClient httpClient, ScheduledExecutorService retryExecutor,
@VisibleForTesting
FaultTolerantHttpClient(String name, HttpClient httpClient, ScheduledExecutorService retryExecutor,
Duration defaultRequestTimeout, RetryConfiguration retryConfiguration,
CircuitBreakerConfiguration circuitBreakerConfiguration) {
final Predicate<Throwable> retryOnException, CircuitBreakerConfiguration circuitBreakerConfiguration) {

this.httpClient = httpClient;
this.retryExecutor = retryExecutor;
Expand All @@ -55,9 +59,12 @@ private FaultTolerantHttpClient(String name, HttpClient httpClient, ScheduledExe
if (this.retryExecutor == null) {
throw new IllegalArgumentException("retryExecutor must be specified with retryConfiguration");
}
RetryConfig retryConfig = retryConfiguration.<HttpResponse>toRetryConfigBuilder()
.retryOnResult(o -> o.statusCode() >= 500).build();
this.retry = Retry.of(name + "-retry", retryConfig);
final RetryConfig.Builder<HttpResponse> retryConfig = retryConfiguration.<HttpResponse>toRetryConfigBuilder()
.retryOnResult(o -> o.statusCode() >= 500);
if (retryOnException != null) {
retryConfig.retryOnException(retryOnException);
}
this.retry = Retry.of(name + "-retry", retryConfig.build());
CircuitBreakerUtil.registerMetrics(retry, FaultTolerantHttpClient.class);
} else {
this.retry = null;
Expand Down Expand Up @@ -101,6 +108,7 @@ public static class Builder {
private KeyStore trustStore;
private String securityProtocol = SECURITY_PROTOCOL_TLS_1_2;
private RetryConfiguration retryConfiguration;
private Predicate<Throwable> retryOnException;
private CircuitBreakerConfiguration circuitBreakerConfiguration;

private Builder() {
Expand Down Expand Up @@ -161,6 +169,11 @@ public Builder withTrustedServerCertificates(final String... certificatePem) thr
return this;
}

public Builder withRetryOnException(final Predicate<Throwable> predicate) {
this.retryOnException = throwable -> predicate.test(ExceptionUtils.unwrap(throwable));
return this;
}

public FaultTolerantHttpClient build() {
if (this.circuitBreakerConfiguration == null || this.name == null || this.executor == null) {
throw new IllegalArgumentException("Must specify circuit breaker config, name, and executor");
Expand All @@ -181,7 +194,7 @@ public FaultTolerantHttpClient build() {
builder.sslContext(sslConfigurator.createSSLContext());

return new FaultTolerantHttpClient(name, builder.build(), retryExecutor, requestTimeout, retryConfiguration,
circuitBreakerConfiguration);
retryOnException, circuitBreakerConfiguration);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@
import java.util.Collections;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.stream.Stream;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicCaptchaConfiguration;
import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration;
import org.whispersystems.textsecuregcm.http.FaultTolerantHttpClient;
import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager;

public class HCaptchaClientTest {
Expand All @@ -44,7 +46,7 @@ static Stream<Arguments> captchaProcessed() {
public void captchaProcessed(final boolean success, final float score, final boolean expectedResult)
throws IOException, InterruptedException {

final HttpClient client = mockResponder(200, String.format("""
final FaultTolerantHttpClient client = mockResponder(200, String.format("""
{
"success": %b,
"score": %f,
Expand All @@ -64,14 +66,14 @@ public void captchaProcessed(final boolean success, final float score, final boo

@Test
public void errorResponse() throws IOException, InterruptedException {
final HttpClient httpClient = mockResponder(503, "");
final FaultTolerantHttpClient httpClient = mockResponder(503, "");
final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5));
assertThrows(IOException.class, () -> client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null));
}

@Test
public void invalidScore() throws IOException, InterruptedException {
final HttpClient httpClient = mockResponder(200, """
final FaultTolerantHttpClient httpClient = mockResponder(200, """
{"success" : true, "score": 1.1}
""");
final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5));
Expand All @@ -80,7 +82,7 @@ public void invalidScore() throws IOException, InterruptedException {

@Test
public void badBody() throws IOException, InterruptedException {
final HttpClient httpClient = mockResponder(200, """
final FaultTolerantHttpClient httpClient = mockResponder(200, """
{"success" : true,
""");
final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5));
Expand All @@ -102,15 +104,14 @@ public void badSiteKey() throws IOException {
}
}

private static HttpClient mockResponder(final int statusCode, final String jsonBody)
throws IOException, InterruptedException {
HttpClient httpClient = mock(HttpClient.class);
private static FaultTolerantHttpClient mockResponder(final int statusCode, final String jsonBody) {
FaultTolerantHttpClient httpClient = mock(FaultTolerantHttpClient.class);
@SuppressWarnings("unchecked") final HttpResponse<Object> httpResponse = mock(HttpResponse.class);

when(httpResponse.body()).thenReturn(jsonBody);
when(httpResponse.statusCode()).thenReturn(statusCode);

when(httpClient.send(any(), any())).thenReturn(httpResponse);
when(httpClient.sendAsync(any(), any())).thenReturn(CompletableFuture.completedFuture(httpResponse));
return httpClient;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@
import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import com.github.tomakehurst.wiremock.junit5.WireMockExtension;
import io.github.resilience4j.circuitbreaker.CallNotPermittedException;
Expand All @@ -19,6 +24,8 @@
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.time.Duration;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
Expand Down Expand Up @@ -114,6 +121,35 @@ void testRetryGet() {
wireMock.verify(3, getRequestedFor(urlEqualTo("/failure")));
}

@Test
void testRetryGetOnException() {
final HttpClient mockHttpClient = mock(HttpClient.class);
final FaultTolerantHttpClient client = new FaultTolerantHttpClient(
"test",
mockHttpClient,
retryExecutor,
Duration.ofSeconds(1),
new RetryConfiguration(),
throwable -> throwable instanceof IOException,
new CircuitBreakerConfiguration());

when(mockHttpClient.sendAsync(any(), any()))
.thenReturn(CompletableFuture.failedFuture(new IOException("test exception")));

HttpRequest request = HttpRequest.newBuilder()
.uri(URI.create("http://localhost:1234/failure"))
.GET()
.build();

try {
client.sendAsync(request, HttpResponse.BodyHandlers.ofString()).join();
throw new AssertionError("Should have failed!");
} catch (CompletionException e) {
assertThat(e.getCause()).isInstanceOf(IOException.class);
}
verify(mockHttpClient, times(3)).sendAsync(any(), any());
}

@Test
void testNetworkFailureCircuitBreaker() throws InterruptedException {
CircuitBreakerConfiguration circuitBreakerConfiguration = new CircuitBreakerConfiguration();
Expand Down

0 comments on commit 0fa8276

Please sign in to comment.