diff --git a/core/src/main/java/google/registry/flows/TlsCredentials.java b/core/src/main/java/google/registry/flows/TlsCredentials.java index 1b2990d29..59c6ee181 100644 --- a/core/src/main/java/google/registry/flows/TlsCredentials.java +++ b/core/src/main/java/google/registry/flows/TlsCredentials.java @@ -15,9 +15,7 @@ package google.registry.flows; import static com.google.common.base.MoreObjects.toStringHelper; -import static com.google.common.base.Strings.isNullOrEmpty; import static google.registry.request.RequestParameters.extractOptionalHeader; -import static google.registry.request.RequestParameters.extractRequiredHeader; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; @@ -56,17 +54,17 @@ public class TlsCredentials implements TransportCredentials { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final boolean requireSslCertificates; - private final String clientCertificateHash; - private final InetAddress clientInetAddr; + private final Optional clientCertificateHash; + private final Optional clientInetAddr; @Inject public TlsCredentials( @Config("requireSslCertificates") boolean requireSslCertificates, - @Header("X-SSL-Certificate") String clientCertificateHash, + @Header("X-SSL-Certificate") Optional clientCertificateHash, @Header("X-Forwarded-For") Optional clientAddress) { this.requireSslCertificates = requireSslCertificates; this.clientCertificateHash = clientCertificateHash; - this.clientInetAddr = clientAddress.isPresent() ? parseInetAddress(clientAddress.get()) : null; + this.clientInetAddr = clientAddress.map(TlsCredentials::parseInetAddress); } static InetAddress parseInetAddress(String asciiAddr) { @@ -97,10 +95,14 @@ public class TlsCredentials implements TransportCredentials { registrar.getClientId()); return; } - for (CidrAddressBlock cidrAddressBlock : ipAddressAllowList) { - if (cidrAddressBlock.contains(clientInetAddr)) { - // IP address is in allow list; return early. - return; + // In the rare unexpected case that the client inet address wasn't passed along at all, then + // by default deny access. + if (clientInetAddr.isPresent()) { + for (CidrAddressBlock cidrAddressBlock : ipAddressAllowList) { + if (cidrAddressBlock.contains(clientInetAddr.get())) { + // IP address is in allow list; return early. + return; + } } } logger.atInfo().log( @@ -118,8 +120,8 @@ public class TlsCredentials implements TransportCredentials { */ @VisibleForTesting void validateCertificate(Registrar registrar) throws AuthenticationErrorException { - if (isNullOrEmpty(registrar.getClientCertificateHash()) - && isNullOrEmpty(registrar.getFailoverClientCertificateHash())) { + if (!registrar.getClientCertificateHash().isPresent() + && !registrar.getFailoverClientCertificateHash().isPresent()) { if (requireSslCertificates) { throw new RegistrarCertificateNotConfiguredException(); } else { @@ -128,7 +130,7 @@ public class TlsCredentials implements TransportCredentials { return; } } - if (isNullOrEmpty(clientCertificateHash)) { + if (!clientCertificateHash.isPresent()) { logger.atInfo().log("Request did not include X-SSL-Certificate"); throw new MissingRegistrarCertificateException(); } @@ -154,21 +156,21 @@ public class TlsCredentials implements TransportCredentials { @Override public String toString() { return toStringHelper(getClass()) - .add("clientCertificateHash", clientCertificateHash) - .add("clientAddress", clientInetAddr) + .add("clientCertificateHash", clientCertificateHash.orElse(null)) + .add("clientAddress", clientInetAddr.orElse(null)) .toString(); } /** Registrar certificate does not match stored certificate. */ public static class BadRegistrarCertificateException extends AuthenticationErrorException { - public BadRegistrarCertificateException() { + BadRegistrarCertificateException() { super("Registrar certificate does not match stored certificate"); } } /** Registrar certificate not present. */ public static class MissingRegistrarCertificateException extends AuthenticationErrorException { - public MissingRegistrarCertificateException() { + MissingRegistrarCertificateException() { super("Registrar certificate not present"); } } @@ -176,14 +178,14 @@ public class TlsCredentials implements TransportCredentials { /** Registrar certificate is not configured. */ public static class RegistrarCertificateNotConfiguredException extends AuthenticationErrorException { - public RegistrarCertificateNotConfiguredException() { + RegistrarCertificateNotConfiguredException() { super("Registrar certificate is not configured"); } } /** Registrar IP address is not in stored allow list. */ public static class BadRegistrarIpAddressException extends AuthenticationErrorException { - public BadRegistrarIpAddressException() { + BadRegistrarIpAddressException() { super("Registrar IP address is not in stored allow list"); } } @@ -191,10 +193,13 @@ public class TlsCredentials implements TransportCredentials { /** Dagger module for the EPP TLS endpoint. */ @Module public static final class EppTlsModule { + @Provides @Header("X-SSL-Certificate") - static String provideClientCertificateHash(HttpServletRequest req) { - return extractRequiredHeader(req, "X-SSL-Certificate"); + static Optional provideClientCertificateHash(HttpServletRequest req) { + // Note: This header is actually required, we just want to handle its absence explicitly + // by throwing an EPP exception rather than a generic Bad Request exception. + return extractOptionalHeader(req, "X-SSL-Certificate"); } @Provides diff --git a/core/src/main/java/google/registry/model/registrar/Registrar.java b/core/src/main/java/google/registry/model/registrar/Registrar.java index 17ee58b9f..c5a08892b 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/core/src/main/java/google/registry/model/registrar/Registrar.java @@ -537,20 +537,24 @@ public class Registrar extends ImmutableObject return LIVE_STATES.contains(state) && PUBLICLY_VISIBLE_TYPES.contains(type); } - public String getClientCertificate() { - return clientCertificate; + /** Returns the client certificate string if it has been set, or empty otherwise. */ + public Optional getClientCertificate() { + return Optional.ofNullable(clientCertificate); } - public String getClientCertificateHash() { - return clientCertificateHash; + /** Returns the client certificate hash if it has been set, or empty otherwise. */ + public Optional getClientCertificateHash() { + return Optional.ofNullable(clientCertificateHash); } - public String getFailoverClientCertificate() { - return failoverClientCertificate; + /** Returns the failover client certificate string if it has been set, or empty otherwise. */ + public Optional getFailoverClientCertificate() { + return Optional.ofNullable(failoverClientCertificate); } - public String getFailoverClientCertificateHash() { - return failoverClientCertificateHash; + /** Returns the failover client certificate hash if it has been set, or empty otherwise. */ + public Optional getFailoverClientCertificateHash() { + return Optional.ofNullable(failoverClientCertificateHash); } public ImmutableList getIpAddressAllowList() { diff --git a/core/src/main/java/google/registry/request/RequestParameters.java b/core/src/main/java/google/registry/request/RequestParameters.java index 698f7e030..a5f2365e0 100644 --- a/core/src/main/java/google/registry/request/RequestParameters.java +++ b/core/src/main/java/google/registry/request/RequestParameters.java @@ -310,7 +310,7 @@ public final class RequestParameters { * @param name case insensitive header name */ public static Optional extractOptionalHeader(HttpServletRequest req, String name) { - return Optional.ofNullable(req.getHeader(name)); + return Optional.ofNullable(emptyToNull(req.getHeader(name))); } private RequestParameters() {} diff --git a/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java b/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java index 8e53a5fcc..1f12adf2c 100644 --- a/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java +++ b/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java @@ -61,6 +61,7 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi { description = "Hash of the client certificate.") private String clientCertificateHash; + @Nullable @Parameter( names = {"-i", "--ip_address"}, description = "Client ip address to pretend to use") @@ -78,7 +79,8 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi { Registrar registrar = checkArgumentPresent( Registrar.loadByClientId(clientId), "Registrar %s not found", clientId); - new TlsCredentials(true, clientCertificateHash, Optional.of(clientIpAddress)) + new TlsCredentials( + true, Optional.ofNullable(clientCertificateHash), Optional.ofNullable(clientIpAddress)) .validate(registrar, password); checkState( registrar.isLive(), "Registrar %s has non-live state: %s", clientId, registrar.getState()); diff --git a/core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index b999fdca4..f5fe3c2f0 100644 --- a/core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java @@ -334,8 +334,9 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA * Returns true if the registrar should accept the new certificate. Returns false if the * certificate is already the one stored for the registrar. */ - private boolean validateCertificate(String existingCertificate, String certificateString) { - if ((existingCertificate == null) || !existingCertificate.equals(certificateString)) { + private boolean validateCertificate( + Optional existingCertificate, String certificateString) { + if (!existingCertificate.isPresent() || !existingCertificate.get().equals(certificateString)) { certificateChecker.validateCertificate(certificateString); return true; } diff --git a/core/src/test/java/google/registry/flows/EppLoginTlsTest.java b/core/src/test/java/google/registry/flows/EppLoginTlsTest.java index 9e724ed39..8112852bb 100644 --- a/core/src/test/java/google/registry/flows/EppLoginTlsTest.java +++ b/core/src/test/java/google/registry/flows/EppLoginTlsTest.java @@ -36,7 +36,8 @@ class EppLoginTlsTest extends EppTestCase { void setClientCertificateHash(String clientCertificateHash) { setTransportCredentials( - new TlsCredentials(true, clientCertificateHash, Optional.of("192.168.1.100:54321"))); + new TlsCredentials( + true, Optional.ofNullable(clientCertificateHash), Optional.of("192.168.1.100:54321"))); } @BeforeEach @@ -107,7 +108,7 @@ class EppLoginTlsTest extends EppTestCase { @Test void testGfeDidntProvideClientCertificate_failsMissingCertificate2200() throws Exception { - setClientCertificateHash(""); + setClientCertificateHash(null); assertThatLogin("NewRegistrar", "foo-BAR2") .hasResponse( "response_error.xml", diff --git a/core/src/test/java/google/registry/flows/FlowRunnerTest.java b/core/src/test/java/google/registry/flows/FlowRunnerTest.java index caf7775b8..e1f298a4b 100644 --- a/core/src/test/java/google/registry/flows/FlowRunnerTest.java +++ b/core/src/test/java/google/registry/flows/FlowRunnerTest.java @@ -137,7 +137,8 @@ class FlowRunnerTest { @Test void testRun_loggingStatement_tlsCredentials() throws Exception { - flowRunner.credentials = new TlsCredentials(true, "abc123def", Optional.of("127.0.0.1")); + flowRunner.credentials = + new TlsCredentials(true, Optional.of("abc123def"), Optional.of("127.0.0.1")); flowRunner.run(eppMetricBuilder); assertThat(Splitter.on("\n\t").split(findFirstLogMessageByPrefix(handler, "EPP Command\n\t"))) .contains("TlsCredentials{clientCertificateHash=abc123def, clientAddress=/127.0.0.1}"); diff --git a/core/src/test/java/google/registry/flows/TlsCredentialsTest.java b/core/src/test/java/google/registry/flows/TlsCredentialsTest.java index 42518d525..093e9dd09 100644 --- a/core/src/test/java/google/registry/flows/TlsCredentialsTest.java +++ b/core/src/test/java/google/registry/flows/TlsCredentialsTest.java @@ -14,7 +14,8 @@ package google.registry.flows; -import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; +import static google.registry.testing.CertificateSamples.SAMPLE_CERT; import static google.registry.testing.DatabaseHelper.loadRegistrar; import static google.registry.testing.DatabaseHelper.persistResource; import static org.joda.time.DateTimeZone.UTC; @@ -22,9 +23,12 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import com.google.common.collect.ImmutableSet; +import google.registry.flows.TlsCredentials.BadRegistrarIpAddressException; +import google.registry.flows.TlsCredentials.MissingRegistrarCertificateException; import google.registry.model.registrar.Registrar; -import google.registry.request.HttpException.BadRequestException; import google.registry.testing.AppEngineExtension; +import google.registry.util.CidrAddressBlock; import java.util.Optional; import javax.servlet.http.HttpServletRequest; import org.joda.time.DateTime; @@ -42,22 +46,40 @@ final class TlsCredentialsTest { void testProvideClientCertificateHash() { HttpServletRequest req = mock(HttpServletRequest.class); when(req.getHeader("X-SSL-Certificate")).thenReturn("data"); - assertThat(TlsCredentials.EppTlsModule.provideClientCertificateHash(req)).isEqualTo("data"); + assertThat(TlsCredentials.EppTlsModule.provideClientCertificateHash(req)).hasValue("data"); } @Test - void testProvideClientCertificateHash_missing() { - HttpServletRequest req = mock(HttpServletRequest.class); - BadRequestException thrown = - assertThrows( - BadRequestException.class, - () -> TlsCredentials.EppTlsModule.provideClientCertificateHash(req)); - assertThat(thrown).hasMessageThat().contains("Missing header: X-SSL-Certificate"); + void testClientCertificateHash_missing() { + TlsCredentials tls = new TlsCredentials(true, Optional.empty(), Optional.of("192.168.1.1")); + persistResource( + loadRegistrar("TheRegistrar") + .asBuilder() + .setClientCertificate(SAMPLE_CERT, DateTime.now(UTC)) + .build()); + assertThrows( + MissingRegistrarCertificateException.class, + () -> tls.validateCertificate(Registrar.loadByClientId("TheRegistrar").get())); + } + + @Test + void test_missingIpAddress_doesntAllowAccess() { + TlsCredentials tls = new TlsCredentials(false, Optional.of("certHash"), Optional.empty()); + persistResource( + loadRegistrar("TheRegistrar") + .asBuilder() + .setClientCertificate(SAMPLE_CERT, DateTime.now(UTC)) + .setIpAddressAllowList(ImmutableSet.of(CidrAddressBlock.create("3.5.8.13"))) + .build()); + assertThrows( + BadRegistrarIpAddressException.class, + () -> tls.validate(Registrar.loadByClientId("TheRegistrar").get(), "password")); } @Test void test_validateCertificate_canBeConfiguredToBypassCertHashes() throws Exception { - TlsCredentials tls = new TlsCredentials(false, "certHash", Optional.of("192.168.1.1")); + TlsCredentials tls = + new TlsCredentials(false, Optional.of("certHash"), Optional.of("192.168.1.1")); persistResource( loadRegistrar("TheRegistrar") .asBuilder() diff --git a/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java b/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java index 0e4ef5888..a4406d199 100644 --- a/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java +++ b/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java @@ -33,8 +33,10 @@ import org.junit.jupiter.api.Test; /** Unit tests for {@link LoginFlow} when accessed via a TLS transport. */ public class LoginFlowViaTlsTest extends LoginFlowTestCase { - private static final String GOOD_CERT = CertificateSamples.SAMPLE_CERT_HASH; - private static final String BAD_CERT = CertificateSamples.SAMPLE_CERT2_HASH; + private static final Optional GOOD_CERT = + Optional.of(CertificateSamples.SAMPLE_CERT_HASH); + private static final Optional BAD_CERT = + Optional.of(CertificateSamples.SAMPLE_CERT2_HASH); private static final Optional GOOD_IP = Optional.of("192.168.1.1"); private static final Optional BAD_IP = Optional.of("1.1.1.1"); private static final Optional GOOD_IPV6 = Optional.of("2001:db8::1"); @@ -97,7 +99,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { @Test void testFailure_missingClientCertificateHash() { persistResource(getRegistrarBuilder().build()); - credentials = new TlsCredentials(true, null, GOOD_IP); + credentials = new TlsCredentials(true, Optional.empty(), GOOD_IP); doFailingTest("login_valid.xml", MissingRegistrarCertificateException.class); } diff --git a/core/src/test/java/google/registry/model/OteAccountBuilderTest.java b/core/src/test/java/google/registry/model/OteAccountBuilderTest.java index 218126bf6..a5473775f 100644 --- a/core/src/test/java/google/registry/model/OteAccountBuilderTest.java +++ b/core/src/test/java/google/registry/model/OteAccountBuilderTest.java @@ -163,9 +163,9 @@ public final class OteAccountBuilderTest { .buildAndPersist(); assertThat(Registrar.loadByClientId("myclientid-3").get().getClientCertificateHash()) - .isEqualTo(SAMPLE_CERT_HASH); + .hasValue(SAMPLE_CERT_HASH); assertThat(Registrar.loadByClientId("myclientid-3").get().getClientCertificate()) - .isEqualTo(SAMPLE_CERT); + .hasValue(SAMPLE_CERT); } @Test diff --git a/core/src/test/java/google/registry/model/registrar/RegistrarTest.java b/core/src/test/java/google/registry/model/registrar/RegistrarTest.java index 0a13ac89d..c4082a0e1 100644 --- a/core/src/test/java/google/registry/model/registrar/RegistrarTest.java +++ b/core/src/test/java/google/registry/model/registrar/RegistrarTest.java @@ -190,18 +190,18 @@ class RegistrarTest extends EntityTestCase { fakeClock.advanceOneMilli(); registrar = registrar.asBuilder().setClientCertificate(SAMPLE_CERT, fakeClock.nowUtc()).build(); assertThat(registrar.getLastCertificateUpdateTime()).isEqualTo(fakeClock.nowUtc()); - assertThat(registrar.getClientCertificate()).isEqualTo(SAMPLE_CERT); - assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT_HASH); + assertThat(registrar.getClientCertificate()).hasValue(SAMPLE_CERT); + assertThat(registrar.getClientCertificateHash()).hasValue(SAMPLE_CERT_HASH); } @TestOfyAndSql void testDeleteCertificateHash_alsoDeletesHash() { - assertThat(registrar.getClientCertificateHash()).isNotNull(); + assertThat(registrar.getClientCertificateHash()).isPresent(); fakeClock.advanceOneMilli(); registrar = registrar.asBuilder().setClientCertificate(null, fakeClock.nowUtc()).build(); assertThat(registrar.getLastCertificateUpdateTime()).isEqualTo(fakeClock.nowUtc()); - assertThat(registrar.getClientCertificate()).isNull(); - assertThat(registrar.getClientCertificateHash()).isNull(); + assertThat(registrar.getClientCertificate()).isEmpty(); + assertThat(registrar.getClientCertificateHash()).isEmpty(); } @TestOfyAndSql @@ -213,21 +213,21 @@ class RegistrarTest extends EntityTestCase { .setFailoverClientCertificate(SAMPLE_CERT2, fakeClock.nowUtc()) .build(); assertThat(registrar.getLastCertificateUpdateTime()).isEqualTo(fakeClock.nowUtc()); - assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT2); - assertThat(registrar.getFailoverClientCertificateHash()).isEqualTo(SAMPLE_CERT2_HASH); + assertThat(registrar.getFailoverClientCertificate()).hasValue(SAMPLE_CERT2); + assertThat(registrar.getFailoverClientCertificateHash()).hasValue(SAMPLE_CERT2_HASH); } @TestOfyAndSql void testDeleteFailoverCertificateHash_alsoDeletesHash() { registrar = registrar.asBuilder().setFailoverClientCertificate(SAMPLE_CERT, fakeClock.nowUtc()).build(); - assertThat(registrar.getFailoverClientCertificateHash()).isNotNull(); + assertThat(registrar.getFailoverClientCertificateHash()).isPresent(); fakeClock.advanceOneMilli(); registrar = registrar.asBuilder().setFailoverClientCertificate(null, fakeClock.nowUtc()).build(); assertThat(registrar.getLastCertificateUpdateTime()).isEqualTo(fakeClock.nowUtc()); - assertThat(registrar.getFailoverClientCertificate()).isNull(); - assertThat(registrar.getFailoverClientCertificateHash()).isNull(); + assertThat(registrar.getFailoverClientCertificate()).isEmpty(); + assertThat(registrar.getFailoverClientCertificateHash()).isEmpty(); } @TestOfyAndSql diff --git a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java index fa8f65e6a..afd095333 100644 --- a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java @@ -95,7 +95,7 @@ class CreateRegistrarCommandTest extends CommandTestCase assertThat(registrar.getState()).isEqualTo(Registrar.State.ACTIVE); assertThat(registrar.getAllowedTlds()).isEmpty(); assertThat(registrar.getIpAddressAllowList()).isEmpty(); - assertThat(registrar.getClientCertificateHash()).isNull(); + assertThat(registrar.getClientCertificateHash()).isEmpty(); assertThat(registrar.getPhonePasscode()).isEqualTo("01234"); assertThat(registrar.getCreationTime()).isIn(Range.closed(before, after)); assertThat(registrar.getLastUpdateTime()).isEqualTo(registrar.getCreationTime()); @@ -383,7 +383,7 @@ class CreateRegistrarCommandTest extends CommandTestCase Optional registrar = Registrar.loadByClientId("clientz"); assertThat(registrar).isPresent(); - assertThat(registrar.get().getClientCertificateHash()).isEqualTo(SAMPLE_CERT3_HASH); + assertThat(registrar.get().getClientCertificateHash()).hasValue(SAMPLE_CERT3_HASH); } @Test @@ -467,10 +467,10 @@ class CreateRegistrarCommandTest extends CommandTestCase Optional registrarOptional = Registrar.loadByClientId("clientz"); assertThat(registrarOptional).isPresent(); Registrar registrar = registrarOptional.get(); - assertThat(registrar.getClientCertificate()).isNull(); - assertThat(registrar.getClientCertificateHash()).isNull(); - assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT3); - assertThat(registrar.getFailoverClientCertificateHash()).isEqualTo(SAMPLE_CERT3_HASH); + assertThat(registrar.getClientCertificate()).isEmpty(); + assertThat(registrar.getClientCertificateHash()).isEmpty(); + assertThat(registrar.getFailoverClientCertificate()).hasValue(SAMPLE_CERT3); + assertThat(registrar.getFailoverClientCertificateHash()).hasValue(SAMPLE_CERT3_HASH); } @Test diff --git a/core/src/test/java/google/registry/tools/SetupOteCommandTest.java b/core/src/test/java/google/registry/tools/SetupOteCommandTest.java index 09630fa37..8b4ea0e1d 100644 --- a/core/src/test/java/google/registry/tools/SetupOteCommandTest.java +++ b/core/src/test/java/google/registry/tools/SetupOteCommandTest.java @@ -15,6 +15,7 @@ package google.registry.tools; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; import static google.registry.model.registrar.Registrar.State.ACTIVE; import static google.registry.model.registry.Registry.TldState.GENERAL_AVAILABILITY; import static google.registry.model.registry.Registry.TldState.START_DATE_SUNRISE; @@ -105,7 +106,7 @@ class SetupOteCommandTest extends CommandTestCase { assertThat(registrar.getState()).isEqualTo(ACTIVE); assertThat(registrar.verifyPassword(password)).isTrue(); assertThat(registrar.getIpAddressAllowList()).isEqualTo(ipAllowList); - assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT_HASH); + assertThat(registrar.getClientCertificateHash()).hasValue(SAMPLE_CERT_HASH); } private void verifyRegistrarContactCreation(String registrarName, String email) { diff --git a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java index c2ff1fb0e..43fee5cef 100644 --- a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java @@ -14,7 +14,6 @@ package google.registry.tools; -import static com.google.common.base.Strings.isNullOrEmpty; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; @@ -250,22 +249,22 @@ class UpdateRegistrarCommandTest extends CommandTestCase void testSuccess_certFile() throws Exception { fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); Registrar registrar = loadRegistrar("NewRegistrar"); - assertThat(registrar.getClientCertificate()).isNull(); - assertThat(registrar.getClientCertificateHash()).isNull(); + assertThat(registrar.getClientCertificate()).isEmpty(); + assertThat(registrar.getClientCertificateHash()).isEmpty(); runCommand("--cert_file=" + getCertFilename(SAMPLE_CERT3), "--force", "NewRegistrar"); registrar = loadRegistrar("NewRegistrar"); // NB: Hash was computed manually using 'openssl x509 -fingerprint -sha256 -in ...' and then // converting the result from a hex string to non-padded base64 encoded string. - assertThat(registrar.getClientCertificate()).isEqualTo(SAMPLE_CERT3); - assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT3_HASH); + assertThat(registrar.getClientCertificate()).hasValue(SAMPLE_CERT3); + assertThat(registrar.getClientCertificateHash()).hasValue(SAMPLE_CERT3_HASH); } @Test void testFail_certFileWithViolation() throws Exception { fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); Registrar registrar = loadRegistrar("NewRegistrar"); - assertThat(registrar.getClientCertificate()).isNull(); - assertThat(registrar.getClientCertificateHash()).isNull(); + assertThat(registrar.getClientCertificate()).isEmpty(); + assertThat(registrar.getClientCertificateHash()).isEmpty(); IllegalArgumentException thrown = assertThrows( IllegalArgumentException.class, @@ -274,15 +273,15 @@ class UpdateRegistrarCommandTest extends CommandTestCase .isEqualTo( "Certificate validity period is too long; it must be less than or equal to 398" + " days."); - assertThat(registrar.getClientCertificate()).isNull(); + assertThat(registrar.getClientCertificate()).isEmpty(); } @Test void testFail_certFileWithMultipleViolations() throws Exception { fakeClock.setTo(DateTime.parse("2055-10-01T00:00:00Z")); Registrar registrar = loadRegistrar("NewRegistrar"); - assertThat(registrar.getClientCertificate()).isNull(); - assertThat(registrar.getClientCertificateHash()).isNull(); + assertThat(registrar.getClientCertificate()).isEmpty(); + assertThat(registrar.getClientCertificateHash()).isEmpty(); IllegalArgumentException thrown = assertThrows( IllegalArgumentException.class, @@ -291,14 +290,14 @@ class UpdateRegistrarCommandTest extends CommandTestCase .isEqualTo( "Certificate is expired.\nCertificate validity period is too long; it must be less" + " than or equal to 398 days."); - assertThat(registrar.getClientCertificate()).isNull(); + assertThat(registrar.getClientCertificate()).isEmpty(); } @Test void testFail_failoverCertFileWithViolation() throws Exception { fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); Registrar registrar = loadRegistrar("NewRegistrar"); - assertThat(registrar.getFailoverClientCertificate()).isNull(); + assertThat(registrar.getFailoverClientCertificate()).isEmpty(); IllegalArgumentException thrown = assertThrows( IllegalArgumentException.class, @@ -308,14 +307,14 @@ class UpdateRegistrarCommandTest extends CommandTestCase .isEqualTo( "Certificate validity period is too long; it must be less than or equal to 398" + " days."); - assertThat(registrar.getFailoverClientCertificate()).isNull(); + assertThat(registrar.getFailoverClientCertificate()).isEmpty(); } @Test void testFail_failoverCertFileWithMultipleViolations() throws Exception { fakeClock.setTo(DateTime.parse("2055-10-01T00:00:00Z")); Registrar registrar = loadRegistrar("NewRegistrar"); - assertThat(registrar.getFailoverClientCertificate()).isNull(); + assertThat(registrar.getFailoverClientCertificate()).isEmpty(); IllegalArgumentException thrown = assertThrows( IllegalArgumentException.class, @@ -325,17 +324,17 @@ class UpdateRegistrarCommandTest extends CommandTestCase .isEqualTo( "Certificate is expired.\nCertificate validity period is too long; it must be less" + " than or equal to 398 days."); - assertThat(registrar.getFailoverClientCertificate()).isNull(); + assertThat(registrar.getFailoverClientCertificate()).isEmpty(); } @Test void testSuccess_failoverCertFile() throws Exception { fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); Registrar registrar = loadRegistrar("NewRegistrar"); - assertThat(registrar.getFailoverClientCertificate()).isNull(); + assertThat(registrar.getFailoverClientCertificate()).isEmpty(); runCommand("--failover_cert_file=" + getCertFilename(SAMPLE_CERT3), "--force", "NewRegistrar"); registrar = loadRegistrar("NewRegistrar"); - assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT3); + assertThat(registrar.getFailoverClientCertificate()).hasValue(SAMPLE_CERT3); } @Test @@ -345,9 +344,9 @@ class UpdateRegistrarCommandTest extends CommandTestCase .asBuilder() .setClientCertificate(SAMPLE_CERT, DateTime.now(UTC)) .build()); - assertThat(isNullOrEmpty(loadRegistrar("NewRegistrar").getClientCertificate())).isFalse(); + assertThat(loadRegistrar("NewRegistrar").getClientCertificate()).isPresent(); runCommand("--cert_file=/dev/null", "--force", "NewRegistrar"); - assertThat(loadRegistrar("NewRegistrar").getClientCertificate()).isNull(); + assertThat(loadRegistrar("NewRegistrar").getClientCertificate()).isEmpty(); } @Test diff --git a/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java b/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java index c54046c68..ac2e728ea 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java +++ b/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java @@ -371,7 +371,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase { clock.setTo(DateTime.parse("2020-11-02T00:00:00Z")); doTestUpdate( Role.OWNER, - Registrar::getClientCertificate, + r -> r.getClientCertificate().orElse(null), CertificateSamples.SAMPLE_CERT3, (builder, s) -> builder.setClientCertificate(s, clock.nowUtc())); } @@ -431,7 +431,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase { clock.setTo(DateTime.parse("2020-11-02T00:00:00Z")); doTestUpdate( Role.OWNER, - Registrar::getFailoverClientCertificate, + r -> r.getFailoverClientCertificate().orElse(null), CertificateSamples.SAMPLE_CERT3, (builder, s) -> builder.setFailoverClientCertificate(s, clock.nowUtc())); } diff --git a/core/src/test/java/google/registry/ui/server/registrar/SecuritySettingsTest.java b/core/src/test/java/google/registry/ui/server/registrar/SecuritySettingsTest.java index 97a1357dc..53b9cdcfd 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/SecuritySettingsTest.java +++ b/core/src/test/java/google/registry/ui/server/registrar/SecuritySettingsTest.java @@ -15,6 +15,7 @@ package google.registry.ui.server.registrar; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; import static google.registry.testing.CertificateSamples.SAMPLE_CERT; import static google.registry.testing.CertificateSamples.SAMPLE_CERT2; import static google.registry.testing.CertificateSamples.SAMPLE_CERT2_HASH; @@ -121,10 +122,10 @@ class SecuritySettingsTest extends RegistrarSettingsActionTestCase { "op", "update", "id", CLIENT_ID, "args", jsonMap)); assertThat(response).containsEntry("status", "SUCCESS"); Registrar registrar = loadRegistrar(CLIENT_ID); - assertThat(registrar.getClientCertificate()).isEqualTo(SAMPLE_CERT3); - assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT3_HASH); - assertThat(registrar.getFailoverClientCertificate()).isNull(); - assertThat(registrar.getFailoverClientCertificateHash()).isNull(); + assertThat(registrar.getClientCertificate()).hasValue(SAMPLE_CERT3); + assertThat(registrar.getClientCertificateHash()).hasValue(SAMPLE_CERT3_HASH); + assertThat(registrar.getFailoverClientCertificate()).isEmpty(); + assertThat(registrar.getFailoverClientCertificateHash()).isEmpty(); assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); verifyNotificationEmailsSent(); } @@ -138,8 +139,8 @@ class SecuritySettingsTest extends RegistrarSettingsActionTestCase { "op", "update", "id", CLIENT_ID, "args", jsonMap)); assertThat(response).containsEntry("status", "SUCCESS"); Registrar registrar = loadRegistrar(CLIENT_ID); - assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT3); - assertThat(registrar.getFailoverClientCertificateHash()).isEqualTo(SAMPLE_CERT3_HASH); + assertThat(registrar.getFailoverClientCertificate()).hasValue(SAMPLE_CERT3); + assertThat(registrar.getFailoverClientCertificateHash()).hasValue(SAMPLE_CERT3_HASH); assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); verifyNotificationEmailsSent(); } @@ -160,10 +161,10 @@ class SecuritySettingsTest extends RegistrarSettingsActionTestCase { "op", "update", "id", CLIENT_ID, "args", jsonMap)); assertThat(response).containsEntry("status", "SUCCESS"); Registrar registrar = loadRegistrar(CLIENT_ID); - assertThat(registrar.getClientCertificate()).isEqualTo(SAMPLE_CERT); - assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT_HASH); - assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT2); - assertThat(registrar.getFailoverClientCertificateHash()).isEqualTo(SAMPLE_CERT2_HASH); + assertThat(registrar.getClientCertificate()).hasValue(SAMPLE_CERT); + assertThat(registrar.getClientCertificateHash()).hasValue(SAMPLE_CERT_HASH); + assertThat(registrar.getFailoverClientCertificate()).hasValue(SAMPLE_CERT2); + assertThat(registrar.getFailoverClientCertificateHash()).hasValue(SAMPLE_CERT2_HASH); assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); }