From c4e5cc13296363b0dea6619e0d038d45b8d21f95 Mon Sep 17 00:00:00 2001 From: Tom Reed Date: Thu, 24 Oct 2024 09:50:10 +0100 Subject: [PATCH] Po 749 747 exception paths (#609) * merge conflicts * exception pathways for put and patch * unused import --- ...DraftAccountControllerIntegrationTest.java | 160 +++++++++++++++++- .../controllers/DraftAccountController.java | 8 +- .../interceptor/AcceptHeaderInterceptor.java | 2 +- .../service/opal/DraftAccountService.java | 5 +- .../updateDraftAccountRequest.json | 7 +- .../AcceptHeaderInterceptorTest.java | 17 -- 6 files changed, 172 insertions(+), 27 deletions(-) diff --git a/src/integrationTest/java/uk/gov/hmcts/opal/controllers/DraftAccountControllerIntegrationTest.java b/src/integrationTest/java/uk/gov/hmcts/opal/controllers/DraftAccountControllerIntegrationTest.java index bb06d91d..eb066b3d 100644 --- a/src/integrationTest/java/uk/gov/hmcts/opal/controllers/DraftAccountControllerIntegrationTest.java +++ b/src/integrationTest/java/uk/gov/hmcts/opal/controllers/DraftAccountControllerIntegrationTest.java @@ -3,6 +3,9 @@ import com.fasterxml.jackson.databind.ObjectMapper; import jakarta.persistence.QueryTimeoutException; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.mockito.stubbing.OngoingStubbing; import org.postgresql.util.PSQLException; import org.postgresql.util.PSQLState; @@ -11,6 +14,7 @@ import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; import org.springframework.boot.test.mock.mockito.MockBean; import org.springframework.boot.test.mock.mockito.SpyBean; +import org.springframework.http.HttpMethod; import org.springframework.http.MediaType; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.ContextConfiguration; @@ -21,6 +25,7 @@ import uk.gov.hmcts.opal.authentication.service.AccessTokenService; import uk.gov.hmcts.opal.authorisation.model.Permissions; import uk.gov.hmcts.opal.authorisation.model.UserState; +import uk.gov.hmcts.opal.config.WebConfig; import uk.gov.hmcts.opal.controllers.advice.GlobalExceptionHandler; import uk.gov.hmcts.opal.dto.AddDraftAccountRequestDto; import uk.gov.hmcts.opal.dto.ReplaceDraftAccountRequestDto; @@ -38,6 +43,7 @@ import java.time.LocalDate; import java.time.LocalDateTime; import java.util.logging.Logger; +import java.util.stream.Stream; import static java.util.Collections.singletonList; import static org.hamcrest.Matchers.containsString; @@ -46,6 +52,10 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.springframework.http.HttpMethod.GET; +import static org.springframework.http.HttpMethod.PATCH; +import static org.springframework.http.HttpMethod.POST; +import static org.springframework.http.HttpMethod.PUT; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.patch; @@ -60,7 +70,7 @@ @WebMvcTest -@ContextConfiguration(classes = {DraftAccountController.class, GlobalExceptionHandler.class}) +@ContextConfiguration(classes = {DraftAccountController.class, GlobalExceptionHandler.class, WebConfig.class}) @ActiveProfiles({"integration"}) class DraftAccountControllerIntegrationTest { private static final Logger logger = Logger.getLogger(DraftAccountControllerIntegrationTest.class.getSimpleName()); @@ -637,7 +647,7 @@ void shouldReturn503WhenDownstreamServiceIsUnavailable(MockHttpServletRequestBui }""")); } - private String validCreateRequestBody() { + private static String validCreateRequestBody() { return """ { "account": { @@ -662,7 +672,7 @@ private String validCreateRequestBody() { }"""; } - private String invalidCreateRequestBody() { + private static String invalidCreateRequestBody() { return """ { "invalid_field": "This field shouldn't be here", @@ -683,7 +693,7 @@ private String invalidCreateRequestBody() { }"""; } - private String validUpdateRequestBody() { + private static String validUpdateRequestBody() { return """ { "account_status": "PENDING", @@ -706,4 +716,146 @@ private DraftAccountEntity toEntity(AddDraftAccountRequestDto dto, LocalDateTim .build(); } + //CEP 1 CEP1 - Invalid Request Payload (400) + @ParameterizedTest + @MethodSource("endpointsWithInvalidBodiesProvider") + void methodsShouldReturn400WhenRequestPayloadIsInvalid(HttpMethod method, String fullPath, String requestBody) + throws Exception { + MockHttpServletRequestBuilder requestBuilder = switch (method.name()) { + case "GET" -> get(fullPath); + case "POST" -> post(fullPath); + case "PUT" -> put(fullPath); + case "PATCH" -> patch(fullPath); + default -> throw new IllegalArgumentException("Unsupported HTTP method: " + method); + }; + + when(userStateService.checkForAuthorisedUser(any())).thenReturn(allPermissionsUser()); + + mockMvc.perform(requestBuilder + .header("Authorization", "Bearer some_value") + .header("Accept", "application/json") + .contentType(MediaType.APPLICATION_JSON) + .content(requestBody)) + .andExpect(status().isBadRequest()); + } + + private static Stream endpointsWithInvalidBodiesProvider() { + return Stream.of(Arguments.of(POST, "/draft-accounts",invalidCreateRequestBody()), + Arguments.of(PUT, "/draft-accounts/1",invalidCreateRequestBody()), + Arguments.of(PATCH, "/draft-accounts/1",invalidCreateRequestBody()) + ); + } + + //CEP2 - Invalid or No Access Token (401) - Security Context required - test elsewhere + + //CEP3 - Not Authorised to perform the requested action (403) + @ParameterizedTest + @MethodSource("testCasesRequiringAuthorizationProvider") + void methodsShouldReturn403WhenUserLacksPermission(HttpMethod method, String fullPath, String requestBody) + throws Exception { + MockHttpServletRequestBuilder requestBuilder = switch (method.name()) { + case "GET" -> get(fullPath); + case "POST" -> post(fullPath); + case "PUT" -> put(fullPath); + case "PATCH" -> patch(fullPath); + default -> throw new IllegalArgumentException("Unsupported HTTP method: " + method); + }; + + when(userStateService.checkForAuthorisedUser(any())).thenReturn(noPermissionsUser()); + + mockMvc.perform(requestBuilder + .header("Authorization", "Bearer some_value") + .contentType(MediaType.APPLICATION_JSON) + .content(requestBody)) + .andExpect(status().isForbidden()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(jsonPath("$.error").value("Forbidden")); + } + + private static Stream testCasesRequiringAuthorizationProvider() { + return Stream.of( + Arguments.of(POST, "/draft-accounts", validCreateRequestBody()), + Arguments.of(PUT, "/draft-accounts/1", validCreateRequestBody()), + Arguments.of(PATCH, "/draft-accounts/1", validUpdateRequestBody()), + Arguments.of(GET, "/draft-accounts", "") // GET endpoints with empty body + ); + } + + //CEP4 - Resource Not Found (404) - applies to GET PUT PATCH & DELETE + @ParameterizedTest + @MethodSource("testCasesForResourceNotFoundProvider") + void methodsShouldReturn404WhenResourceNotFound(HttpMethod method, String fullPath, String requestBody) + throws Exception { + // Set up a non-existent ID + long nonExistentId = 999L; + + MockHttpServletRequestBuilder requestBuilder = switch (method.name()) { + case "GET" -> get(fullPath); + case "PUT" -> put(fullPath); + case "PATCH" -> patch(fullPath); + case "DELETE" -> delete(fullPath); + default -> throw new IllegalArgumentException("Unsupported HTTP method: " + method); + }; + + // Mock the service behavior + when(userStateService.checkForAuthorisedUser(any())).thenReturn(allPermissionsUser()); + + // For GET return null + when(draftAccountService.getDraftAccount(nonExistentId)).thenReturn(null); + + // For PUT, throw EntityNotFoundException + when(draftAccountService.replaceDraftAccount(eq(nonExistentId), any(ReplaceDraftAccountRequestDto.class))) + .thenThrow(new jakarta.persistence.EntityNotFoundException("Draft Account not found with id: " + + nonExistentId)); + // For PATCH, throw EntityNotFoundException + when(draftAccountService.updateDraftAccount(eq(nonExistentId), any(UpdateDraftAccountRequestDto.class))) + .thenThrow(new jakarta.persistence.EntityNotFoundException("Draft Account not found with id: " + + nonExistentId)); + mockMvc.perform(requestBuilder + .header("Authorization", "Bearer some_value") + .contentType(MediaType.APPLICATION_JSON) + .content(requestBody)) + .andExpect(status().isNotFound()); + } + + private static Stream testCasesForResourceNotFoundProvider() { + return Stream.of( + Arguments.of(GET, "/draft-accounts/999", ""), + Arguments.of(PUT, "/draft-accounts/999", validCreateRequestBody()), + Arguments.of(PATCH, "/draft-accounts/999", validUpdateRequestBody()) + ); + } + + //CEP5 - Unsupported Content Type for Response (406) + @ParameterizedTest + @MethodSource("testCasesWithValidBodiesProvider") + void methodsShouldReturn406WhenAcceptHeaderIsNotSupported(HttpMethod method, String fullPath, String requestBody) + throws Exception { + MockHttpServletRequestBuilder requestBuilder = switch (method.name()) { + case "GET" -> get(fullPath); + case "POST" -> post(fullPath); + case "PUT" -> put(fullPath); + case "PATCH" -> patch(fullPath); + default -> throw new IllegalArgumentException("Unsupported HTTP method: " + method); + }; + + when(userStateService.checkForAuthorisedUser(any())).thenReturn(allPermissionsUser()); + + mockMvc.perform(requestBuilder + .header("Authorization", "Bearer some_value") + .header("Accept", "application/xml") + .contentType(MediaType.APPLICATION_JSON) + .content(requestBody)) + .andExpect(status().isNotAcceptable()); + } + + private static Stream testCasesWithValidBodiesProvider() { + return Stream.of(Arguments.of(POST, "/draft-accounts",validCreateRequestBody()), + Arguments.of(PUT, "/draft-accounts/1","{}"), + Arguments.of(PATCH, "/draft-accounts/1","{}"), + Arguments.of(GET, "/draft-accounts/1","{}") + ); + } + + } diff --git a/src/main/java/uk/gov/hmcts/opal/controllers/DraftAccountController.java b/src/main/java/uk/gov/hmcts/opal/controllers/DraftAccountController.java index 7a92f6ae..373eb127 100644 --- a/src/main/java/uk/gov/hmcts/opal/controllers/DraftAccountController.java +++ b/src/main/java/uk/gov/hmcts/opal/controllers/DraftAccountController.java @@ -72,6 +72,7 @@ public DraftAccountController(UserStateService userStateService, DraftAccountSer } @GetMapping(value = "/{draftAccountId}") + @CheckAcceptHeader @Operation(summary = "Returns the Draft Account for the given draftAccountId.") public ResponseEntity getDraftAccountById( @PathVariable Long draftAccountId, @@ -86,6 +87,7 @@ public ResponseEntity getDraftAccountById( } @GetMapping() + @CheckAcceptHeader @Operation(summary = "Returns a collection of draft accounts summaries for the given user.") public ResponseEntity getDraftAccountSummaries( @RequestParam(value = "business_unit") Optional> optionalBusinessUnitIds, @@ -176,18 +178,19 @@ public ResponseEntity deleteDraftAccountById( @PutMapping(value = "/{draftAccountId}", consumes = MediaType.APPLICATION_JSON_VALUE) @Operation(summary = "Replaces an existing Draft Account Entity in the DB with data in request body") + @CheckAcceptHeader public ResponseEntity replaceDraftAccount( @PathVariable Long draftAccountId, @RequestBody ReplaceDraftAccountRequestDto dto, @RequestHeader(value = "Authorization", required = false) String authHeaderValue) { UserState userState = userStateService.checkForAuthorisedUser(authHeaderValue); + jsonSchemaValidationService.validateOrError(dto.toJson(), REPLACE_DRAFT_ACCOUNT_REQUEST_JSON); if (userState.hasBusinessUnitUserWithPermission(dto.getBusinessUnitId(), Permissions.CREATE_MANAGE_DRAFT_ACCOUNTS)) { log.info(":PUT:replaceDraftAccount: replacing draft account entity with ID: {} and data: {}", draftAccountId, dto); - jsonSchemaValidationService.validateOrError(dto.toJson(), REPLACE_DRAFT_ACCOUNT_REQUEST_JSON); DraftAccountEntity replacedEntity = draftAccountService.replaceDraftAccount(draftAccountId, dto); @@ -199,12 +202,14 @@ public ResponseEntity replaceDraftAccount( @PatchMapping(value = "/{draftAccountId}", consumes = MediaType.APPLICATION_JSON_VALUE) @Operation(summary = "Updates an existing Draft Account Entity in the DB with data in request body") + @CheckAcceptHeader public ResponseEntity updateDraftAccount( @PathVariable Long draftAccountId, @RequestBody UpdateDraftAccountRequestDto dto, @RequestHeader(value = "Authorization", required = false) String authHeaderValue) { UserState userState = userStateService.checkForAuthorisedUser(authHeaderValue); + jsonSchemaValidationService.validateOrError(dto.toJson(), UPDATE_DRAFT_ACCOUNT_REQUEST_JSON); if (userState.hasBusinessUnitUserWithPermission(dto.getBusinessUnitId(), Permissions.CREATE_MANAGE_DRAFT_ACCOUNTS)) { @@ -213,7 +218,6 @@ public ResponseEntity updateDraftAccount( draftAccountId, dto ); - jsonSchemaValidationService.validateOrError(dto.toJson(), UPDATE_DRAFT_ACCOUNT_REQUEST_JSON); DraftAccountEntity updatedEntity = draftAccountService.updateDraftAccount(draftAccountId, dto); diff --git a/src/main/java/uk/gov/hmcts/opal/interceptor/AcceptHeaderInterceptor.java b/src/main/java/uk/gov/hmcts/opal/interceptor/AcceptHeaderInterceptor.java index e4377d5d..b0ad4e57 100644 --- a/src/main/java/uk/gov/hmcts/opal/interceptor/AcceptHeaderInterceptor.java +++ b/src/main/java/uk/gov/hmcts/opal/interceptor/AcceptHeaderInterceptor.java @@ -25,6 +25,6 @@ public boolean preHandle(HttpServletRequest request, HttpServletResponse respons } private boolean isAcceptableMediaType(String acceptHeader) { - return acceptHeader != null && (acceptHeader.contains("application/json") || acceptHeader.contains("*/*")); + return acceptHeader == null || (acceptHeader.contains("application/json") || acceptHeader.contains("*/*")); } } diff --git a/src/main/java/uk/gov/hmcts/opal/service/opal/DraftAccountService.java b/src/main/java/uk/gov/hmcts/opal/service/opal/DraftAccountService.java index b893e385..61542ed0 100644 --- a/src/main/java/uk/gov/hmcts/opal/service/opal/DraftAccountService.java +++ b/src/main/java/uk/gov/hmcts/opal/service/opal/DraftAccountService.java @@ -4,6 +4,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ObjectNode; +import jakarta.persistence.EntityNotFoundException; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Qualifier; @@ -100,7 +101,7 @@ public DraftAccountEntity submitDraftAccount(AddDraftAccountRequestDto dto) { public DraftAccountEntity replaceDraftAccount(Long draftAccountId, ReplaceDraftAccountRequestDto dto) { DraftAccountEntity existingAccount = draftAccountRepository.findById(draftAccountId) - .orElseThrow(() -> new RuntimeException("Draft Account not found with id: " + draftAccountId)); + .orElseThrow(() -> new EntityNotFoundException("Draft Account not found with id: " + draftAccountId)); BusinessUnitEntity businessUnit = businessUnitRepository.findById(dto.getBusinessUnitId()) .orElseThrow(() -> new RuntimeException("Business Unit not found with id: " + dto.getBusinessUnitId())); @@ -133,7 +134,7 @@ public DraftAccountEntity replaceDraftAccount(Long draftAccountId, ReplaceDraftA public DraftAccountEntity updateDraftAccount(Long draftAccountId, UpdateDraftAccountRequestDto dto) { DraftAccountEntity existingAccount = draftAccountRepository.findById(draftAccountId) - .orElseThrow(() -> new RuntimeException("Draft Account not found with id: " + draftAccountId)); + .orElseThrow(() -> new EntityNotFoundException("Draft Account not found with id: " + draftAccountId)); if (!(existingAccount.getBusinessUnit().getBusinessUnitId().equals(dto.getBusinessUnitId()))) { log.info("DTO BU does not match entity for draft account with ID: {}", draftAccountId); diff --git a/src/main/resources/jsonSchemas/updateDraftAccountRequest.json b/src/main/resources/jsonSchemas/updateDraftAccountRequest.json index 32bc142a..79fb3223 100644 --- a/src/main/resources/jsonSchemas/updateDraftAccountRequest.json +++ b/src/main/resources/jsonSchemas/updateDraftAccountRequest.json @@ -20,5 +20,10 @@ "type": "object", "description": "Status changes to the Draft Account in chronological order (JSON Array) - System generated (UI)" } - } + }, + "required": [ + "business_unit_id", + "validated_by", + "account_status" + ] } diff --git a/src/test/java/uk/gov/hmcts/opal/interceptor/AcceptHeaderInterceptorTest.java b/src/test/java/uk/gov/hmcts/opal/interceptor/AcceptHeaderInterceptorTest.java index d0610465..edee9c33 100644 --- a/src/test/java/uk/gov/hmcts/opal/interceptor/AcceptHeaderInterceptorTest.java +++ b/src/test/java/uk/gov/hmcts/opal/interceptor/AcceptHeaderInterceptorTest.java @@ -78,23 +78,6 @@ void preHandle_WithCheckAcceptHeaderAnnotationAndInvalidAcceptHeader_ShouldRetur assertTrue(stringWriter.toString().contains("\"message\":\"The requested media type is not supported\"")); } - @Test - void preHandle_WithCheckAcceptHeaderAnnotationAndNullAcceptHeader_ShouldReturnFalse() throws Exception { - when(handlerMethod.hasMethodAnnotation(CheckAcceptHeader.class)).thenReturn(true); - when(request.getHeader("Accept")).thenReturn(null); - - StringWriter stringWriter = new StringWriter(); - PrintWriter writer = new PrintWriter(stringWriter); - when(response.getWriter()).thenReturn(writer); - - boolean result = interceptor.preHandle(request, response, handlerMethod); - - assertFalse(result); - verify(response).setStatus(HttpServletResponse.SC_NOT_ACCEPTABLE); - writer.flush(); - assertTrue(stringWriter.toString().contains("\"error\":\"Not Acceptable\"")); - assertTrue(stringWriter.toString().contains("\"message\":\"The requested media type is not supported\"")); - } @Test void preHandle_WithoutCheckAcceptHeaderAnnotation_ShouldReturnTrue() throws Exception {