mirror of
https://github.com/signalapp/Signal-Server
synced 2026-10-06 02:44:56 +01:00
Add a dynamic configuration flag to fail open in the event of upstream captcha errors
This commit is contained in:
1 parent
a15b70476d
commit
2ee739dbe1
4 files changed
+77
-16
No files matched your search
@@ -1068,7 +1068,7 @@ public class WhisperServerService extends Application<WhisperServerConfiguration
|
||||
final HttpClient shortCodeRetrieverHttpClient = HttpClient.newBuilder().version(HttpClient.Version.HTTP_2)
|
||||
.connectTimeout(Duration.ofSeconds(10)).build();
|
||||
final ShortCodeExpander shortCodeRetriever = new ShortCodeExpander(shortCodeRetrieverHttpClient, config.getShortCodeRetrieverConfiguration().baseUrl());
|
||||
final CaptchaChecker captchaChecker = new CaptchaChecker(shortCodeRetriever, captchaClientSupplier);
|
||||
final CaptchaChecker captchaChecker = new CaptchaChecker(shortCodeRetriever, captchaClientSupplier, dynamicConfigurationManager);
|
||||
|
||||
final RegistrationCaptchaManager registrationCaptchaManager = new RegistrationCaptchaManager(captchaChecker);
|
||||
|
||||
|
||||
@@ -9,7 +9,6 @@ import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name;
|
||||
|
||||
import com.google.common.annotations.VisibleForTesting;
|
||||
import io.micrometer.core.instrument.Metrics;
|
||||
import jakarta.ws.rs.BadRequestException;
|
||||
import java.io.IOException;
|
||||
import java.util.Locale;
|
||||
import java.util.Optional;
|
||||
@@ -19,6 +18,8 @@ import java.util.function.Function;
|
||||
import javax.annotation.Nullable;
|
||||
import org.slf4j.Logger;
|
||||
import org.slf4j.LoggerFactory;
|
||||
import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration;
|
||||
import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager;
|
||||
|
||||
public class CaptchaChecker {
|
||||
private static final Logger logger = LoggerFactory.getLogger(CaptchaChecker.class);
|
||||
@@ -33,12 +34,15 @@ public class CaptchaChecker {
|
||||
|
||||
private final ShortCodeExpander shortCodeExpander;
|
||||
private final Function<String, CaptchaClient> captchaClientSupplier;
|
||||
private final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager;
|
||||
|
||||
public CaptchaChecker(
|
||||
final ShortCodeExpander shortCodeRetriever,
|
||||
final Function<String, CaptchaClient> captchaClientSupplier) {
|
||||
final Function<String, CaptchaClient> captchaClientSupplier,
|
||||
final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager) {
|
||||
this.shortCodeExpander = shortCodeRetriever;
|
||||
this.captchaClientSupplier = captchaClientSupplier;
|
||||
this.dynamicConfigurationManager = dynamicConfigurationManager;
|
||||
}
|
||||
|
||||
|
||||
@@ -105,12 +109,21 @@ public class CaptchaChecker {
|
||||
throw new InvalidCaptchaArgumentException("invalid captcha site-key");
|
||||
}
|
||||
|
||||
final AssessmentResult result = client.verify(maybeAci, siteKey, parsedAction, token, ip, userAgent);
|
||||
Metrics.counter(ASSESSMENTS_COUNTER_NAME,
|
||||
"action", action,
|
||||
"score", result.getScoreString(),
|
||||
"provider", provider)
|
||||
.increment();
|
||||
return result;
|
||||
try {
|
||||
final AssessmentResult result = client.verify(maybeAci, siteKey, parsedAction, token, ip, userAgent);
|
||||
Metrics.counter(ASSESSMENTS_COUNTER_NAME,
|
||||
"action", action,
|
||||
"score", result.getScoreString(),
|
||||
"provider", provider)
|
||||
.increment();
|
||||
return result;
|
||||
} catch (final IOException | RuntimeException e) {
|
||||
if (dynamicConfigurationManager.getConfiguration().getCaptchaConfiguration().isFailOpen()) {
|
||||
logger.warn("Failed to verify captcha; failing open", e);
|
||||
return AssessmentResult.alwaysValid();
|
||||
}
|
||||
|
||||
throw e;
|
||||
}
|
||||
}
|
||||
}
|
||||
+10
@@ -35,6 +35,9 @@ public class DynamicCaptchaConfiguration {
|
||||
@NotNull
|
||||
private Map<Action, BigDecimal> scoreFloorByAction = Collections.emptyMap();
|
||||
|
||||
@JsonProperty
|
||||
private boolean failOpen = false;
|
||||
|
||||
public BigDecimal getScoreFloor() {
|
||||
return scoreFloor;
|
||||
}
|
||||
@@ -66,4 +69,11 @@ public class DynamicCaptchaConfiguration {
|
||||
this.hCaptchaSiteKeys = hCaptchaSiteKeys;
|
||||
}
|
||||
|
||||
public boolean isFailOpen() {
|
||||
return failOpen;
|
||||
}
|
||||
|
||||
public void setFailOpen(final boolean failOpen) {
|
||||
this.failOpen = failOpen;
|
||||
}
|
||||
}
|
||||
+44
-6
@@ -6,6 +6,7 @@
|
||||
package org.whispersystems.textsecuregcm.captcha;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
|
||||
import static org.junit.jupiter.api.Assertions.assertThrows;
|
||||
import static org.mockito.ArgumentMatchers.any;
|
||||
import static org.mockito.ArgumentMatchers.eq;
|
||||
@@ -15,7 +16,6 @@ import static org.mockito.Mockito.verify;
|
||||
import static org.mockito.Mockito.when;
|
||||
import static org.whispersystems.textsecuregcm.captcha.CaptchaChecker.SEPARATOR;
|
||||
|
||||
import jakarta.ws.rs.BadRequestException;
|
||||
import java.io.IOException;
|
||||
import java.util.Collections;
|
||||
import java.util.Map;
|
||||
@@ -23,9 +23,14 @@ import java.util.Optional;
|
||||
import java.util.UUID;
|
||||
import java.util.stream.Stream;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.junit.jupiter.api.function.Executable;
|
||||
import org.junit.jupiter.params.ParameterizedTest;
|
||||
import org.junit.jupiter.params.provider.Arguments;
|
||||
import org.junit.jupiter.params.provider.MethodSource;
|
||||
import org.junit.jupiter.params.provider.ValueSource;
|
||||
import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicCaptchaConfiguration;
|
||||
import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration;
|
||||
import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager;
|
||||
|
||||
public class CaptchaCheckerTest {
|
||||
|
||||
@@ -72,6 +77,19 @@ public class CaptchaCheckerTest {
|
||||
return captchaClient;
|
||||
}
|
||||
|
||||
private static DynamicConfigurationManager<DynamicConfiguration> mockDynamicConfigurationManager(final boolean failOpen) {
|
||||
final DynamicCaptchaConfiguration dynamicCaptchaConfiguration = mock(DynamicCaptchaConfiguration.class);
|
||||
final DynamicConfiguration dynamicConfiguration = mock(DynamicConfiguration.class);
|
||||
|
||||
@SuppressWarnings("unchecked") final DynamicConfigurationManager<DynamicConfiguration> dynamicConfigurationManager =
|
||||
mock(DynamicConfigurationManager.class);
|
||||
|
||||
when(dynamicConfigurationManager.getConfiguration()).thenReturn(dynamicConfiguration);
|
||||
when(dynamicConfiguration.getCaptchaConfiguration()).thenReturn(dynamicCaptchaConfiguration);
|
||||
when(dynamicCaptchaConfiguration.isFailOpen()).thenReturn(failOpen);
|
||||
|
||||
return dynamicConfigurationManager;
|
||||
}
|
||||
|
||||
@ParameterizedTest
|
||||
@MethodSource
|
||||
@@ -81,7 +99,7 @@ public class CaptchaCheckerTest {
|
||||
final String siteKey,
|
||||
final Action expectedAction) throws IOException, InvalidCaptchaArgumentException {
|
||||
final CaptchaClient captchaClient = mockClient(PREFIX);
|
||||
new CaptchaChecker(null, PREFIX -> captchaClient).verify(Optional.empty(), expectedAction, input, null, USER_AGENT);
|
||||
new CaptchaChecker(null, _ -> captchaClient, mockDynamicConfigurationManager(false)).verify(Optional.empty(), expectedAction, input, null, USER_AGENT);
|
||||
verify(captchaClient, times(1)).verify(any(), eq(siteKey), eq(expectedAction), eq(expectedToken), any(), eq(USER_AGENT));
|
||||
}
|
||||
|
||||
@@ -110,10 +128,10 @@ public class CaptchaCheckerTest {
|
||||
final CaptchaClient b = mockClient(PREFIX_B);
|
||||
final Map<String, CaptchaClient> captchaClientMap = Map.of(PREFIX_A, a, PREFIX_B, b);
|
||||
|
||||
new CaptchaChecker(null, captchaClientMap::get).verify(Optional.of(ACI), Action.CHALLENGE, ainput, null, USER_AGENT);
|
||||
new CaptchaChecker(null, captchaClientMap::get, mockDynamicConfigurationManager(false)).verify(Optional.of(ACI), Action.CHALLENGE, ainput, null, USER_AGENT);
|
||||
verify(a, times(1)).verify(any(), any(), any(), any(), any(), any());
|
||||
|
||||
new CaptchaChecker(null, captchaClientMap::get).verify(Optional.of(ACI), Action.CHALLENGE, binput, null, USER_AGENT);
|
||||
new CaptchaChecker(null, captchaClientMap::get, mockDynamicConfigurationManager(false)).verify(Optional.of(ACI), Action.CHALLENGE, binput, null, USER_AGENT);
|
||||
verify(b, times(1)).verify(any(), any(), any(), any(), any(), any());
|
||||
}
|
||||
|
||||
@@ -135,7 +153,7 @@ public class CaptchaCheckerTest {
|
||||
public void badArgs(final String input) throws IOException {
|
||||
final CaptchaClient cc = mockClient(PREFIX);
|
||||
assertThrows(InvalidCaptchaArgumentException.class,
|
||||
() -> new CaptchaChecker(null, prefix -> PREFIX.equals(prefix) ? cc : null).verify(Optional.of(ACI), Action.CHALLENGE, input, null, USER_AGENT));
|
||||
() -> new CaptchaChecker(null, prefix -> PREFIX.equals(prefix) ? cc : null, mockDynamicConfigurationManager(false)).verify(Optional.of(ACI), Action.CHALLENGE, input, null, USER_AGENT));
|
||||
|
||||
}
|
||||
|
||||
@@ -145,8 +163,28 @@ public class CaptchaCheckerTest {
|
||||
final ShortCodeExpander retriever = mock(ShortCodeExpander.class);
|
||||
when(retriever.retrieve("abc")).thenReturn(Optional.of(TOKEN));
|
||||
final String input = String.join(SEPARATOR, PREFIX + "-short", REG_SITE_KEY, "registration", "abc");
|
||||
new CaptchaChecker(retriever, ignored -> captchaClient).verify(Optional.of(ACI), Action.REGISTRATION, input, null, USER_AGENT);
|
||||
new CaptchaChecker(retriever, _ -> captchaClient, mockDynamicConfigurationManager(false)).verify(Optional.of(ACI), Action.REGISTRATION, input, null, USER_AGENT);
|
||||
verify(captchaClient, times(1)).verify(any(), eq(REG_SITE_KEY), eq(Action.REGISTRATION), eq(TOKEN), any(), any());
|
||||
|
||||
}
|
||||
|
||||
@ParameterizedTest
|
||||
@ValueSource(booleans = {true, false})
|
||||
public void failOpen(final boolean failOpen) throws IOException {
|
||||
final CaptchaClient captchaClient = mockClient(PREFIX);
|
||||
final String input = String.join(SEPARATOR, PREFIX, CHALLENGE_SITE_KEY, "challenge", TOKEN);
|
||||
|
||||
final Exception exception = new IOException();
|
||||
|
||||
when(captchaClient.verify(any(), any(), any(), any(), any(), any())).thenThrow(exception);
|
||||
|
||||
final Executable executable =
|
||||
() -> new CaptchaChecker(null, _ -> captchaClient, mockDynamicConfigurationManager(failOpen)).verify(Optional.of(ACI), Action.CHALLENGE, input, null, USER_AGENT);
|
||||
|
||||
if (failOpen) {
|
||||
assertDoesNotThrow(executable);
|
||||
} else {
|
||||
assertThrows(exception.getClass(), executable);
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user