From 22e1bdd00845f637925c258fbdd73c1cefc96896 Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Thu, 7 Jan 2021 19:30:09 -0500 Subject: [PATCH] Use better null-handling around registrar certificates (#922) * Use better null-handling around registrar certificates Now with Optional it's always very clear whether they do or do not have values. isNullOrEmpty() shouldn't be necessary anymore (indeed it wasn't necessary prior to this either, as the relevant setters in the Registrar builder already coerced empty strings to null). And also the cert hash is a required HTTP header, so it will error out in the Dagger component if null or empty long before getting to any other code. * Merge branch 'master' into optional-get-certs --- .../google/registry/flows/TlsCredentials.java | 47 ++++++++++--------- .../registry/model/registrar/Registrar.java | 20 ++++---- .../registry/request/RequestParameters.java | 2 +- .../ValidateLoginCredentialsCommand.java | 4 +- .../registrar/RegistrarSettingsAction.java | 5 +- .../registry/flows/EppLoginTlsTest.java | 5 +- .../google/registry/flows/FlowRunnerTest.java | 3 +- .../registry/flows/TlsCredentialsTest.java | 44 ++++++++++++----- .../flows/session/LoginFlowViaTlsTest.java | 8 ++-- .../registry/model/OteAccountBuilderTest.java | 4 +- .../model/registrar/RegistrarTest.java | 20 ++++---- .../tools/CreateRegistrarCommandTest.java | 12 ++--- .../registry/tools/SetupOteCommandTest.java | 3 +- .../tools/UpdateRegistrarCommandTest.java | 37 +++++++-------- .../RegistrarSettingsActionTest.java | 4 +- .../registrar/SecuritySettingsTest.java | 21 +++++---- 16 files changed, 139 insertions(+), 100 deletions(-) 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"); }