From 086e3dc5c0e092259f89316a02e35c035df224d2 Mon Sep 17 00:00:00 2001 From: Hilmar Falkenberg Date: Wed, 27 Apr 2022 11:31:08 +0200 Subject: [PATCH] fix NPE issue with http HEAD (#1809) * feign can't handle empty/null reponse body :-( * move DB truncation into the same transaction as the insert, otherwise we might run into inconsistency! * RetentionPolicyDccRevocationTest * add DccRevocationClientDelegatorTest --- .../service/DccRevocationListService.java | 6 +-- .../service/DccRevocationListServiceTest.java | 6 --- .../Eclipse/spring-boot-dcc_revocation.launch | 2 +- ...dDccRevocationFeignHttpClientProvider.java | 5 +- .../dcc/DccRevocationClientDelegator.java | 43 +++++++++++++++ .../dcc/ProdDccRevocationClient.java | 5 +- .../dcc/TestDccRevocationClient.java | 2 +- .../dcc/decode/DccRevocationListDecoder.java | 5 ++ .../distribution/runner/RetentionPolicy.java | 2 +- .../dcc/DccRevocationClientDelegatorTest.java | 53 +++++++++++++++++++ .../dcc/DccRevocationClientUnitTest.java | 2 +- .../distribution/dcc/DccRevocationListIT.java | 2 +- ... => RetentionPolicyDccRevocationTest.java} | 25 +++++++-- 13 files changed, 133 insertions(+), 25 deletions(-) create mode 100644 services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/DccRevocationClientDelegator.java create mode 100644 services/distribution/src/test/java/app/coronawarn/server/services/distribution/dcc/DccRevocationClientDelegatorTest.java rename services/distribution/src/test/java/app/coronawarn/server/services/distribution/runner/{RetentionPolicyTestRevocation.java => RetentionPolicyDccRevocationTest.java} (76%) diff --git a/common/persistence/src/main/java/app/coronawarn/server/common/persistence/service/DccRevocationListService.java b/common/persistence/src/main/java/app/coronawarn/server/common/persistence/service/DccRevocationListService.java index 0aae58f895..8892e2956d 100644 --- a/common/persistence/src/main/java/app/coronawarn/server/common/persistence/service/DccRevocationListService.java +++ b/common/persistence/src/main/java/app/coronawarn/server/common/persistence/service/DccRevocationListService.java @@ -60,6 +60,8 @@ public Collection getRevocationListEntries() { @Timed @Transactional public void store(final Collection revocationEntries) { + logger.info("Truncate Revocation list..."); + repository.truncate(); logger.info("Saving Revocation list entries..."); for (final RevocationEntry entry : revocationEntries) { repository.saveDoNothingOnConflict(entry.getKid(), entry.getType(), entry.getHash()); @@ -79,8 +81,4 @@ public void store(final RevocationEtag etag) { } etagRepository.save(etag.getPath(), etag.getEtag()); } - - public void truncate() { - repository.truncate(); - } } diff --git a/common/persistence/src/test/java/app/coronawarn/server/common/persistence/service/DccRevocationListServiceTest.java b/common/persistence/src/test/java/app/coronawarn/server/common/persistence/service/DccRevocationListServiceTest.java index 18b1ebcf28..d51c9670ef 100644 --- a/common/persistence/src/test/java/app/coronawarn/server/common/persistence/service/DccRevocationListServiceTest.java +++ b/common/persistence/src/test/java/app/coronawarn/server/common/persistence/service/DccRevocationListServiceTest.java @@ -9,7 +9,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.data.jdbc.DataJdbcTest; @@ -20,11 +19,6 @@ class DccRevocationListServiceTest { @Autowired private DccRevocationListService service; - @AfterEach - public void tearDown() { - service.truncate(); - } - @Test void testStore() { Collection some = new ArrayList<>(); diff --git a/runconfigs/Eclipse/spring-boot-dcc_revocation.launch b/runconfigs/Eclipse/spring-boot-dcc_revocation.launch index 0a6e5898b8..e4d3086fcc 100644 --- a/runconfigs/Eclipse/spring-boot-dcc_revocation.launch +++ b/runconfigs/Eclipse/spring-boot-dcc_revocation.launch @@ -31,7 +31,7 @@ - + diff --git a/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/CloudDccRevocationFeignHttpClientProvider.java b/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/CloudDccRevocationFeignHttpClientProvider.java index 1c4ba016f6..52310496ac 100644 --- a/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/CloudDccRevocationFeignHttpClientProvider.java +++ b/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/CloudDccRevocationFeignHttpClientProvider.java @@ -60,8 +60,7 @@ public CloudDccRevocationFeignHttpClientProvider(DistributionServiceConfig confi @Override @Bean public Client createDccRevocationFeignClient() { - return new ApacheHttpClient( - dccHttpClientFactory().createBuilder().build()); + return new DccRevocationClientDelegator(new ApacheHttpClient(dccHttpClientFactory().createBuilder().build())); } /** @@ -73,7 +72,7 @@ private SSLContext getSslContext(File trustStorePath, String trustStorePass) { logger.info("Instantiating DCC Revocation client - SSL context with truststore: {}", trustStorePath.getName()); try { return SSLContextBuilder.create().loadTrustMaterial(trustStorePath, - emptyCharrArrayIfNull(trustStorePass)) + emptyCharrArrayIfNull(trustStorePass)) .build(); } catch (Exception e) { logger.error("Problem on creating DCC Revocation client - SSL context with truststore: " diff --git a/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/DccRevocationClientDelegator.java b/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/DccRevocationClientDelegator.java new file mode 100644 index 0000000000..5d0fa91538 --- /dev/null +++ b/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/DccRevocationClientDelegator.java @@ -0,0 +1,43 @@ +package app.coronawarn.server.services.distribution.dcc; + +import feign.Client; +import feign.Request; +import feign.Request.Options; +import feign.Response; +import feign.Response.Body; +import feign.httpclient.ApacheHttpClient; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class DccRevocationClientDelegator implements Client { + + private static final Logger logger = LoggerFactory.getLogger(DccRevocationClientDelegator.class); + + private final ApacheHttpClient apacheHttpClient; + + public DccRevocationClientDelegator(final ApacheHttpClient apacheHttpClient) { + this.apacheHttpClient = apacheHttpClient; + } + + @Override + public Response execute(final Request request, final Options options) throws IOException { + final Response response = apacheHttpClient.execute(request, options); + + // in case of http HEAD the response is NULL! + final Body body = response.body(); + if (body != null) { + return response; + } else { + logger.info("response body is null for '{}'", request); + } + + return Response.builder() + .status(response.status()) + .reason(response.reason()) + .headers(response.headers()) + .request(response.request()) + .body("", StandardCharsets.UTF_8).build(); + } +} diff --git a/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/ProdDccRevocationClient.java b/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/ProdDccRevocationClient.java index 684c131641..2ddcb6e9ab 100644 --- a/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/ProdDccRevocationClient.java +++ b/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/ProdDccRevocationClient.java @@ -52,6 +52,7 @@ public String getETag() throws FetchDccListException { etag = getETag(dccRevocationFeignClient.head()); return etag; } catch (final Exception e) { + logger.error(e.getMessage(), e); throw new FetchDccListException("http-HEAD for DCC Revocation List failed", e); } } @@ -64,8 +65,8 @@ public String getETag() throws FetchDccListException { * @throws NullPointerException if there is no ETag in the {@link ResponseEntity#getHeaders()}. */ public static String getETag(final ResponseEntity response) { - final String string = response.getHeaders().getETag().replaceAll("\"", ""); - logger.info("got DCC Revocation List ETag: {}", string); + final String string = response.getHeaders().getETag(); + logger.info("got DCC Revocation List ETag: '{}'", string); return string; } } diff --git a/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/TestDccRevocationClient.java b/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/TestDccRevocationClient.java index 51d8bc0291..ba22f15608 100644 --- a/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/TestDccRevocationClient.java +++ b/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/TestDccRevocationClient.java @@ -41,6 +41,6 @@ public Optional> getDccRevocationList() throws FetchDccLis @Override public String getETag() throws FetchDccListException { - return "62620c23-2953"; + return "62620c23-test"; } } diff --git a/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/decode/DccRevocationListDecoder.java b/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/decode/DccRevocationListDecoder.java index 2f454c088c..5e1906da47 100644 --- a/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/decode/DccRevocationListDecoder.java +++ b/services/distribution/src/main/java/app/coronawarn/server/services/distribution/dcc/decode/DccRevocationListDecoder.java @@ -15,11 +15,15 @@ import java.util.Arrays; import java.util.List; import java.util.Map; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.stereotype.Component; @Component public class DccRevocationListDecoder { + private static final Logger logger = LoggerFactory.getLogger(DccRevocationListDecoder.class); + private DistributionServiceConfig distributionServiceConfig; public DccRevocationListDecoder(DistributionServiceConfig distributionServiceConfig) { @@ -58,6 +62,7 @@ public List decode(byte[] data) throws DccRevocationListDecodeE revocationEntries.add(new RevocationEntry(kid, type, hash))); }); } catch (Exception e) { + logger.error(e.getMessage(), e); throw new DccRevocationListDecodeException("DCC revocation list NOT decoded.", e); } return revocationEntries; diff --git a/services/distribution/src/main/java/app/coronawarn/server/services/distribution/runner/RetentionPolicy.java b/services/distribution/src/main/java/app/coronawarn/server/services/distribution/runner/RetentionPolicy.java index c5ce993d84..4f8cc42164 100644 --- a/services/distribution/src/main/java/app/coronawarn/server/services/distribution/runner/RetentionPolicy.java +++ b/services/distribution/src/main/java/app/coronawarn/server/services/distribution/runner/RetentionPolicy.java @@ -87,9 +87,9 @@ public void run(ApplicationArguments args) { if (dccRevocationListService.etagExists(dccRevocationClient.getETag())) { logger.info("DCC Revocation - ETag didn't change, nothing to do, shutting down."); SpringApplication.exit(applicationContext); + System.exit(0); return; } - dccRevocationListService.truncate(); s3RetentionPolicy.deleteDccRevocationDir(); } else { diagnosisKeyService.applyRetentionPolicy(retentionDays); diff --git a/services/distribution/src/test/java/app/coronawarn/server/services/distribution/dcc/DccRevocationClientDelegatorTest.java b/services/distribution/src/test/java/app/coronawarn/server/services/distribution/dcc/DccRevocationClientDelegatorTest.java new file mode 100644 index 0000000000..77e2cb47d5 --- /dev/null +++ b/services/distribution/src/test/java/app/coronawarn/server/services/distribution/dcc/DccRevocationClientDelegatorTest.java @@ -0,0 +1,53 @@ +package app.coronawarn.server.services.distribution.dcc; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import feign.Request; +import feign.Request.Body; +import feign.Request.HttpMethod; +import feign.Request.Options; +import feign.RequestTemplate; +import feign.Response; +import feign.httpclient.ApacheHttpClient; +import java.util.Collections; +import org.junit.jupiter.api.Test; + +class DccRevocationClientDelegatorTest { + + @Test + void nullBodyShouldBeTurnedIntoEmptyString() throws Exception { + final ApacheHttpClient client = mock(ApacheHttpClient.class); + final Request request = Request.create(HttpMethod.GET, "http://localhost", Collections.emptyMap(), Body.empty(), + (RequestTemplate) null); + final Response mockResponse = Response.builder().request(request).body((Response.Body) null).build(); + when(client.execute(any(), any())).thenReturn(mockResponse); + final DccRevocationClientDelegator fixture = new DccRevocationClientDelegator(client); + assertNull(mockResponse.body()); + final Response response = fixture.execute(request, new Options()); + assertNotNull(response.body()); + } + + @Test + void responseIsNotChangedIfBodyIsNotNull() throws Exception { + final ApacheHttpClient client = mock(ApacheHttpClient.class); + final Request request = Request.create(HttpMethod.GET, "http://localhost", Collections.emptyMap(), Body.empty(), + (RequestTemplate) null); + final Response mockResponse = Response.builder().request(request).body("foo".getBytes()).build(); + when(client.execute(any(), any())).thenReturn(mockResponse); + final DccRevocationClientDelegator fixture = new DccRevocationClientDelegator(client); + assertNotNull(mockResponse.body()); + final Response response = fixture.execute(request, new Options()); + assertEquals(mockResponse, response); + } + + @Test + void testDccRevocationClientDelegator() { + final DccRevocationClientDelegator fixture = new DccRevocationClientDelegator(new ApacheHttpClient()); + assertNotNull(fixture); + } +} diff --git a/services/distribution/src/test/java/app/coronawarn/server/services/distribution/dcc/DccRevocationClientUnitTest.java b/services/distribution/src/test/java/app/coronawarn/server/services/distribution/dcc/DccRevocationClientUnitTest.java index 6a1d845623..989d3f92f2 100644 --- a/services/distribution/src/test/java/app/coronawarn/server/services/distribution/dcc/DccRevocationClientUnitTest.java +++ b/services/distribution/src/test/java/app/coronawarn/server/services/distribution/dcc/DccRevocationClientUnitTest.java @@ -48,7 +48,7 @@ void coverTestDccRevocationClient2() throws Exception { @Test void coverTestDccRevocationClient3() throws Exception { - assertEquals("62620c23-2953", testDccRevocationClient.getETag()); + assertEquals("62620c23-test", testDccRevocationClient.getETag()); } @Test diff --git a/services/distribution/src/test/java/app/coronawarn/server/services/distribution/dcc/DccRevocationListIT.java b/services/distribution/src/test/java/app/coronawarn/server/services/distribution/dcc/DccRevocationListIT.java index 285efdae8d..527bf78e70 100644 --- a/services/distribution/src/test/java/app/coronawarn/server/services/distribution/dcc/DccRevocationListIT.java +++ b/services/distribution/src/test/java/app/coronawarn/server/services/distribution/dcc/DccRevocationListIT.java @@ -44,6 +44,6 @@ void should_fetch_dcc_revocation_list() throws FetchDccListException { void should_fetch_etag() throws FetchDccListException { final String etag = dccRevocationClient.getETag(); assertThat(etag).isNotBlank(); - assertThat(etag).doesNotContain("\""); + assertThat(etag).doesNotContain("test"); } } diff --git a/services/distribution/src/test/java/app/coronawarn/server/services/distribution/runner/RetentionPolicyTestRevocation.java b/services/distribution/src/test/java/app/coronawarn/server/services/distribution/runner/RetentionPolicyDccRevocationTest.java similarity index 76% rename from services/distribution/src/test/java/app/coronawarn/server/services/distribution/runner/RetentionPolicyTestRevocation.java rename to services/distribution/src/test/java/app/coronawarn/server/services/distribution/runner/RetentionPolicyDccRevocationTest.java index 4e692be00b..bf982286c3 100644 --- a/services/distribution/src/test/java/app/coronawarn/server/services/distribution/runner/RetentionPolicyTestRevocation.java +++ b/services/distribution/src/test/java/app/coronawarn/server/services/distribution/runner/RetentionPolicyDccRevocationTest.java @@ -1,30 +1,33 @@ package app.coronawarn.server.services.distribution.runner; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; - import app.coronawarn.server.common.persistence.service.DccRevocationListService; import app.coronawarn.server.common.persistence.service.DiagnosisKeyService; import app.coronawarn.server.common.persistence.service.StatisticsDownloadService; import app.coronawarn.server.common.persistence.service.TraceTimeIntervalWarningService; import app.coronawarn.server.services.distribution.config.DistributionServiceConfig; import app.coronawarn.server.services.distribution.dcc.DccRevocationClient; +import app.coronawarn.server.services.distribution.dcc.FetchDccListException; import app.coronawarn.server.services.distribution.objectstore.S3RetentionPolicy; +import org.assertj.core.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.SpringApplication; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.boot.test.context.ConfigDataApplicationContextInitializer; import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.context.ApplicationContext; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit.jupiter.SpringExtension; +import static org.mockito.Mockito.*; + @EnableConfigurationProperties(value = DistributionServiceConfig.class) @ExtendWith(SpringExtension.class) @ContextConfiguration(classes = { RetentionPolicy.class }, initializers = ConfigDataApplicationContextInitializer.class) @ActiveProfiles("revocation") -class RetentionPolicyTestRevocation { +class RetentionPolicyDccRevocationTest { @MockBean DiagnosisKeyService diagnosisKeyService; @@ -50,6 +53,9 @@ class RetentionPolicyTestRevocation { @MockBean DccRevocationClient dccRevocationClient; + @MockBean + ApplicationContext applicationContext; + @Test void shouldCallDatabaseAndS3RetentionRunner() { retentionPolicy.run(null); @@ -60,7 +66,16 @@ void shouldCallDatabaseAndS3RetentionRunner() { verify(traceTimeIntervalWarningService, times(0)) .applyRetentionPolicy(distributionServiceConfig.getRetentionDays()); - verify(dccRevocationListService, times(1)).truncate(); + verify(s3RetentionPolicy, times(1)).deleteDccRevocationDir(); + } + + @Test + void shouldCheckForEtag() throws FetchDccListException { + retentionPolicy.run(null); + + when(dccRevocationListService.etagExists(dccRevocationClient.getETag())).thenReturn(true); + Assertions.assertThat(retentionPolicy.isDccRevocation()).isTrue(); + Assertions.assertThat(SpringApplication.exit(applicationContext)); verify(s3RetentionPolicy, times(1)).deleteDccRevocationDir(); } }