diff --git a/build.gradle b/build.gradle index 9624475872..cab5adfd80 100644 --- a/build.gradle +++ b/build.gradle @@ -398,6 +398,7 @@ dependencies { implementation group: 'org.springdoc', name: 'springdoc-openapi-starter-webmvc-ui', version: '2.8.17' implementation group: 'uk.gov.service.notify', name: 'notifications-java-client', version: '6.0.0-RELEASE' implementation group: 'org.modelmapper', name: 'modelmapper', version: '3.2.6' + implementation group: 'com.github.ben-manes.caffeine', name: 'caffeine', version: '3.2.2' implementation group: 'com.github.hmcts.java-logging', name: 'logging', version: '8.0.0' implementation group: 'com.github.hmcts', name: 'core-case-data-store-client', version: '5.3.0' diff --git a/src/integrationTest/java/uk/gov/hmcts/reform/pcs/config/AbstractPostgresContainerIT.java b/src/integrationTest/java/uk/gov/hmcts/reform/pcs/config/AbstractPostgresContainerIT.java index 13efe14e00..f6ffb1efdd 100644 --- a/src/integrationTest/java/uk/gov/hmcts/reform/pcs/config/AbstractPostgresContainerIT.java +++ b/src/integrationTest/java/uk/gov/hmcts/reform/pcs/config/AbstractPostgresContainerIT.java @@ -1,11 +1,15 @@ package uk.gov.hmcts.reform.pcs.config; +import com.github.benmanes.caffeine.cache.Cache; import lombok.extern.slf4j.Slf4j; +import org.junit.jupiter.api.BeforeEach; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.test.context.DynamicPropertyRegistry; import org.springframework.test.context.DynamicPropertySource; import org.testcontainers.containers.PostgreSQLContainer; import org.testcontainers.junit.jupiter.Container; import org.testcontainers.junit.jupiter.Testcontainers; +import uk.gov.hmcts.reform.pcs.idam.UserInfo; @Slf4j @Testcontainers @@ -14,6 +18,19 @@ public abstract class AbstractPostgresContainerIT { @Container static PostgreSQLContainer postgres = new PostgreSQLContainer<>("postgres:16-alpine"); + // The Spring context (and so this singleton cache) is shared across test classes, + // but @MockitoBean mocks are reset per test. Without this, a token validated by an + // earlier test is served from the cache and never hits the freshly-reset IDAM mock. + @Autowired(required = false) + private Cache idamUserInfoCache; + + @BeforeEach + void invalidateIdamUserInfoCache() { + if (idamUserInfoCache != null) { + idamUserInfoCache.invalidateAll(); + } + } + @DynamicPropertySource static void configure(DynamicPropertyRegistry registry) { registry.add("spring.flyway.locations", () -> "classpath:db/migration"); diff --git a/src/main/java/uk/gov/hmcts/reform/pcs/ccd/PCSCaseView.java b/src/main/java/uk/gov/hmcts/reform/pcs/ccd/PCSCaseView.java index 8b0e6086bf..8a7ffdac9e 100644 --- a/src/main/java/uk/gov/hmcts/reform/pcs/ccd/PCSCaseView.java +++ b/src/main/java/uk/gov/hmcts/reform/pcs/ccd/PCSCaseView.java @@ -101,22 +101,14 @@ public PCSCase getCase(CaseViewRequest request) { State state = request.state(); SubmittedCase submittedCase = getSubmittedCase(caseReference); PCSCase pcsCase = submittedCase.pcsCase(); - boolean hasUnsubmittedCaseData = caseHasUnsubmittedData(caseReference, state); + Optional draftCaseData = getDraftCaseData(caseReference, state); - if (hasUnsubmittedCaseData) { - draftCaseDataService - .getUnsubmittedCaseData(caseReference, resumePossessionClaim) - .ifPresentOrElse( - draft -> { - caseTabView.setDraftCaseTabFields(pcsCase, draft); - }, - () -> caseTabView.setCaseTabFields(pcsCase) - ); - } else { - caseTabView.setCaseTabFields(pcsCase); - } + draftCaseData.ifPresentOrElse( + draft -> caseTabView.setDraftCaseTabFields(pcsCase, draft), + () -> caseTabView.setCaseTabFields(pcsCase) + ); - setMarkdownFields(pcsCase, hasUnsubmittedCaseData); + setMarkdownFields(pcsCase, draftCaseData.isPresent()); enforcementOrderMediator.handleEnforcementRequirements(submittedCase.pcsCaseEntity(), pcsCase); caseFieldsView.setCaseFields(pcsCase); @@ -129,12 +121,12 @@ public PCSCase getCase(CaseViewRequest request) { return pcsCase; } - private boolean caseHasUnsubmittedData(long caseReference, State state) { + private Optional getDraftCaseData(long caseReference, State state) { if (State.AWAITING_SUBMISSION_TO_HMCTS == state) { - return draftCaseDataService.hasUnsubmittedCaseData(caseReference, resumePossessionClaim); + return draftCaseDataService.getUnsubmittedCaseData(caseReference, resumePossessionClaim); } - return false; + return Optional.empty(); } private SubmittedCase getSubmittedCase(long caseReference) { diff --git a/src/main/java/uk/gov/hmcts/reform/pcs/ccd/repository/PcsCaseRepository.java b/src/main/java/uk/gov/hmcts/reform/pcs/ccd/repository/PcsCaseRepository.java index adf2ea847c..9f2c74ebcc 100644 --- a/src/main/java/uk/gov/hmcts/reform/pcs/ccd/repository/PcsCaseRepository.java +++ b/src/main/java/uk/gov/hmcts/reform/pcs/ccd/repository/PcsCaseRepository.java @@ -9,7 +9,7 @@ public interface PcsCaseRepository extends JpaRepository { - @EntityGraph(attributePaths = {"propertyAddress", "parties", "parties.address"}) + @EntityGraph(attributePaths = {"propertyAddress", "parties", "parties.address", "tenancyLicence"}) Optional findByCaseReference(long caseReference); } diff --git a/src/main/java/uk/gov/hmcts/reform/pcs/config/IdamUserInfoCacheConfiguration.java b/src/main/java/uk/gov/hmcts/reform/pcs/config/IdamUserInfoCacheConfiguration.java new file mode 100644 index 0000000000..e84ca5be32 --- /dev/null +++ b/src/main/java/uk/gov/hmcts/reform/pcs/config/IdamUserInfoCacheConfiguration.java @@ -0,0 +1,31 @@ +package uk.gov.hmcts.reform.pcs.config; + +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import uk.gov.hmcts.reform.pcs.idam.UserInfo; + +import java.time.Duration; + +@Configuration +public class IdamUserInfoCacheConfiguration { + + /** + * Cache of IDAM /o/userinfo responses, keyed by a hash of the bearer token. + * A token maps to the same user for its whole lifetime, so entries can never be + * wrong — the TTL only bounds how long a revoked token would still be accepted. + */ + @Bean + public Cache idamUserInfoCache( + @Value("${idam.userinfo-cache.ttl-seconds:120}") long ttlSeconds, + @Value("${idam.userinfo-cache.max-size:1000}") long maxSize) { + + return Caffeine.newBuilder() + .expireAfterWrite(Duration.ofSeconds(ttlSeconds)) + .maximumSize(maxSize) + .build(); + } + +} diff --git a/src/main/java/uk/gov/hmcts/reform/pcs/idam/IdamAuthenticator.java b/src/main/java/uk/gov/hmcts/reform/pcs/idam/IdamAuthenticator.java index 2e76599b40..cc8284ce60 100644 --- a/src/main/java/uk/gov/hmcts/reform/pcs/idam/IdamAuthenticator.java +++ b/src/main/java/uk/gov/hmcts/reform/pcs/idam/IdamAuthenticator.java @@ -1,5 +1,6 @@ package uk.gov.hmcts.reform.pcs.idam; +import com.github.benmanes.caffeine.cache.Cache; import feign.FeignException; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -8,6 +9,11 @@ import uk.gov.hmcts.reform.pcs.exception.InvalidAuthTokenException; import uk.gov.hmcts.reform.pcs.security.IdamTokenProvider; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.HexFormat; + @Service @RequiredArgsConstructor @Slf4j @@ -15,6 +21,7 @@ public class IdamAuthenticator { private static final String BEARER_PREFIX = IdamTokenProvider.BEARER_PREFIX; private final IdamUserInfoApi idamUserInfoApi; + private final Cache idamUserInfoCache; public User validateAuthToken(String authorisation) { if (authorisation == null || authorisation.isBlank()) { @@ -43,10 +50,31 @@ public User validateAuthToken(String authorisation) { public User retrieveUser(String authorisation) { final String bearerToken = getBearerToken(authorisation); - final UserInfo userDetails = idamUserInfoApi.getUserInfo(bearerToken); + + if (bearerToken == null || bearerToken.isBlank()) { + return new User(bearerToken, idamUserInfoApi.getUserInfo(bearerToken)); + } + + // A token always resolves to the same user, so the response is safe to cache for + // the TTL. Failures propagate out of the loader uncached, so a rejected token is + // re-validated on its next use. + final UserInfo userDetails = idamUserInfoCache.get( + cacheKey(bearerToken), + key -> idamUserInfoApi.getUserInfo(bearerToken) + ); return new User(bearerToken, userDetails); } + private String cacheKey(String bearerToken) { + try { + MessageDigest digest = MessageDigest.getInstance("SHA-256"); + byte[] hash = digest.digest(bearerToken.getBytes(StandardCharsets.UTF_8)); + return HexFormat.of().formatHex(hash); + } catch (NoSuchAlgorithmException ex) { + throw new IllegalStateException("SHA-256 not available", ex); + } + } + private String getBearerToken(String token) { if (token == null || token.isBlank()) { return token; diff --git a/src/main/resources/application.yaml b/src/main/resources/application.yaml index f7ea09ef9b..19f7e4e76f 100644 --- a/src/main/resources/application.yaml +++ b/src/main/resources/application.yaml @@ -110,6 +110,11 @@ flyway: idam: api: url: ${IDAM_API_URL:http://localhost:5062} + userinfo-cache: + # How long a validated token is trusted without re-asking IDAM. This bounds the + # window in which a revoked (but unexpired) token would still be accepted. + ttl-seconds: ${IDAM_USERINFO_CACHE_TTL_SECONDS:120} + max-size: ${IDAM_USERINFO_CACHE_MAX_SIZE:1000} s2s-auth: url: ${IDAM_S2S_AUTH_URL:http://localhost:4502} totp_secret: ${PCS_API_S2S_SECRET:AAAAAAAAAAAAAAAA} diff --git a/src/test/java/uk/gov/hmcts/reform/pcs/ccd/PCSCaseViewTest.java b/src/test/java/uk/gov/hmcts/reform/pcs/ccd/PCSCaseViewTest.java index 85803ca9a9..549bbc9c58 100644 --- a/src/test/java/uk/gov/hmcts/reform/pcs/ccd/PCSCaseViewTest.java +++ b/src/test/java/uk/gov/hmcts/reform/pcs/ccd/PCSCaseViewTest.java @@ -335,8 +335,6 @@ void shouldSetCaseFields() { void shouldSetDraftCaseTabFieldsWhenUnsubmittedCaseDataExists() { // Given PCSCase draftCaseData = PCSCase.builder().build(); - when(draftCaseDataService.hasUnsubmittedCaseData(CASE_REFERENCE, resumePossessionClaim)) - .thenReturn(true); when(draftCaseDataService.getUnsubmittedCaseData(CASE_REFERENCE, resumePossessionClaim)) .thenReturn(Optional.of(draftCaseData)); @@ -344,7 +342,6 @@ void shouldSetDraftCaseTabFieldsWhenUnsubmittedCaseDataExists() { PCSCase pcsCase = underTest.getCase(request(CASE_REFERENCE, State.AWAITING_SUBMISSION_TO_HMCTS)); // Then - verify(draftCaseDataService).hasUnsubmittedCaseData(CASE_REFERENCE, resumePossessionClaim); verify(draftCaseDataService).getUnsubmittedCaseData(CASE_REFERENCE, resumePossessionClaim); verify(caseTabView).setDraftCaseTabFields(pcsCase, draftCaseData); verify(caseTabView, never()).setCaseTabFields(any(PCSCase.class)); @@ -352,10 +349,8 @@ void shouldSetDraftCaseTabFieldsWhenUnsubmittedCaseDataExists() { } @Test - void shouldSetSubmittedCaseTabFieldsWhenUnsubmittedCaseDataIsExpectedButDraftIsMissing() { + void shouldSetSubmittedCaseTabFieldsWhenNoDraftExists() { // Given - when(draftCaseDataService.hasUnsubmittedCaseData(CASE_REFERENCE, resumePossessionClaim)) - .thenReturn(true); when(draftCaseDataService.getUnsubmittedCaseData(CASE_REFERENCE, resumePossessionClaim)) .thenReturn(Optional.empty()); @@ -363,23 +358,10 @@ void shouldSetSubmittedCaseTabFieldsWhenUnsubmittedCaseDataIsExpectedButDraftIsM PCSCase pcsCase = underTest.getCase(request(CASE_REFERENCE, State.AWAITING_SUBMISSION_TO_HMCTS)); // Then - verify(draftCaseDataService).hasUnsubmittedCaseData(CASE_REFERENCE, resumePossessionClaim); verify(draftCaseDataService).getUnsubmittedCaseData(CASE_REFERENCE, resumePossessionClaim); verify(caseTabView, never()).setDraftCaseTabFields(any(PCSCase.class), any(PCSCase.class)); verify(caseTabView).setCaseTabFields(pcsCase); - assertThat(pcsCase.getNextStepsMarkdown()).contains("Resume claim"); - } - - @Test - void shouldNotFetchUnsubmittedCaseDataWhenNoUnsubmittedCaseDataExists() { - // When - underTest.getCase(request(CASE_REFERENCE, State.AWAITING_SUBMISSION_TO_HMCTS)); - - // Then - verify(draftCaseDataService).hasUnsubmittedCaseData(CASE_REFERENCE, resumePossessionClaim); - verify(draftCaseDataService, never()).getUnsubmittedCaseData(any(Long.class), any()); - verify(caseTabView, never()).setDraftCaseTabFields(any(PCSCase.class), any(PCSCase.class)); - verify(caseTabView).setCaseTabFields(any(PCSCase.class)); + assertThat(pcsCase.getNextStepsMarkdown()).contains("Provide more details about your claim"); } @Test diff --git a/src/test/java/uk/gov/hmcts/reform/pcs/idam/IdamAuthenticatorTest.java b/src/test/java/uk/gov/hmcts/reform/pcs/idam/IdamAuthenticatorTest.java index 20a59d328f..ea6c9eafd3 100644 --- a/src/test/java/uk/gov/hmcts/reform/pcs/idam/IdamAuthenticatorTest.java +++ b/src/test/java/uk/gov/hmcts/reform/pcs/idam/IdamAuthenticatorTest.java @@ -1,13 +1,15 @@ package uk.gov.hmcts.reform.pcs.idam; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; import feign.FeignException; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.NullAndEmptySource; import org.junit.jupiter.params.provider.ValueSource; -import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import uk.gov.hmcts.reform.pcs.exception.IdamException; @@ -16,6 +18,7 @@ import static org.assertj.core.api.AssertionsForClassTypes.assertThat; import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -28,9 +31,16 @@ class IdamAuthenticatorTest { @Mock private IdamUserInfoApi idamUserInfoApi; - @InjectMocks + private Cache userInfoCache; + private IdamAuthenticator underTest; + @BeforeEach + void setUp() { + userInfoCache = Caffeine.newBuilder().maximumSize(100).build(); + underTest = new IdamAuthenticator(idamUserInfoApi, userInfoCache); + } + @ParameterizedTest @NullAndEmptySource @DisplayName("Should throw InvalidAuthTokenException when token is null or blank") @@ -152,4 +162,53 @@ void shouldHandleNullTokenInRetrieveUser() { assertThat(user).isNotNull(); assertThat(user.getAuthToken()).isNull(); } + + @Test + @DisplayName("Should call IDAM only once for repeated validations of the same token") + void shouldCacheUserInfoForRepeatedValidationsOfSameToken() { + String token = BEARER_PREFIX + "valid-token"; + UserInfo userInfo = mock(UserInfo.class); + when(idamUserInfoApi.getUserInfo(token)).thenReturn(userInfo); + + User firstUser = underTest.validateAuthToken(token); + User secondUser = underTest.validateAuthToken(token); + + assertThat(firstUser.getUserDetails()).isEqualTo(userInfo); + assertThat(secondUser.getUserDetails()).isEqualTo(userInfo); + verify(idamUserInfoApi, times(1)).getUserInfo(token); + } + + @Test + @DisplayName("Should call IDAM separately for different tokens") + void shouldNotShareCacheEntriesBetweenDifferentTokens() { + String tokenA = BEARER_PREFIX + "token-a"; + String tokenB = BEARER_PREFIX + "token-b"; + when(idamUserInfoApi.getUserInfo(tokenA)).thenReturn(mock(UserInfo.class)); + when(idamUserInfoApi.getUserInfo(tokenB)).thenReturn(mock(UserInfo.class)); + + underTest.validateAuthToken(tokenA); + underTest.validateAuthToken(tokenB); + + verify(idamUserInfoApi).getUserInfo(tokenA); + verify(idamUserInfoApi).getUserInfo(tokenB); + } + + @Test + @DisplayName("Should not cache failures - a rejected token is re-validated on next use") + void shouldNotCacheFailedValidations() { + String token = BEARER_PREFIX + "flaky-token"; + FeignException.Unauthorized unauthorizedException = mock(FeignException.Unauthorized.class); + UserInfo userInfo = mock(UserInfo.class); + when(idamUserInfoApi.getUserInfo(token)) + .thenThrow(unauthorizedException) + .thenReturn(userInfo); + + assertThatThrownBy(() -> underTest.validateAuthToken(token)) + .isInstanceOf(InvalidAuthTokenException.class); + + User user = underTest.validateAuthToken(token); + + assertThat(user.getUserDetails()).isEqualTo(userInfo); + verify(idamUserInfoApi, times(2)).getUserInfo(token); + } }