From f90e199f305d7b4d3709188a2941c960cc927492 Mon Sep 17 00:00:00 2001 From: Enrico Date: Thu, 30 Nov 2023 12:37:49 +0100 Subject: [PATCH] chore: Validate sort direction and remove unsupported options [TECH-1622] --- .../event/mapper/SortDirection.java | 25 ++-- .../event/webrequest/OrderCriteria.java | 4 +- .../event/webrequest/OrderCriteriaTest.java | 9 +- .../OrderCriteriaParamEditor.java} | 18 +-- .../controller/CrudControllerAdvice.java | 3 + .../webapi/security/config/WebMvcConfig.java | 1 - .../webapi/controller/OrderBindingTest.java | 119 ++++++++++++++++++ 7 files changed, 143 insertions(+), 36 deletions(-) rename dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/{security/config/StringToOrderCriteriaListConverter.java => common/OrderCriteriaParamEditor.java} (76%) create mode 100644 dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/OrderBindingTest.java diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/mapper/SortDirection.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/mapper/SortDirection.java index 4d3a5b260303..66231ac4ce6a 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/mapper/SortDirection.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/mapper/SortDirection.java @@ -34,29 +34,26 @@ @Getter @AllArgsConstructor public enum SortDirection { - ASC("asc", false), - DESC("desc", false), - IASC("iasc", true), - IDESC("idesc", true); - - private static final SortDirection DEFAULT_SORTING_DIRECTION = ASC; + ASC("asc"), + DESC("desc"); private final String value; - private final boolean ignoreCase; - public static SortDirection of(String value) { - return of(value, DEFAULT_SORTING_DIRECTION); - } - - public static SortDirection of(String value, SortDirection defaultSortingDirection) { return Arrays.stream(values()) .filter(sortDirection -> sortDirection.getValue().equalsIgnoreCase(value)) .findFirst() - .orElse(defaultSortingDirection); + .orElseThrow( + () -> + new IllegalArgumentException( + "'" + + value + + "' is not a valid sort direction. Valid values are: " + + Arrays.toString(SortDirection.values()) + + ".")); } public boolean isAscending() { - return this == ASC || this == IASC; + return this == ASC; } } diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java index 74c2aaaaea00..c3d3a686e4c3 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java @@ -68,10 +68,10 @@ private static List toOrderCriterias(String s) { .collect(Collectors.toList()); } - private static OrderCriteria toOrderCriteria(String s1) { + public static OrderCriteria toOrderCriteria(String s1) { String[] props = s1.split(":"); if (props.length == 2) { - return OrderCriteria.of(props[0], SortDirection.of(props[1])); + return OrderCriteria.of(props[0], SortDirection.of(props[1].trim())); } if (props.length == 1) { return OrderCriteria.of(props[0], SortDirection.ASC); diff --git a/dhis-2/dhis-api/src/test/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteriaTest.java b/dhis-2/dhis-api/src/test/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteriaTest.java index 4b4512ff8e0c..a443b4059b94 100644 --- a/dhis-2/dhis-api/src/test/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteriaTest.java +++ b/dhis-2/dhis-api/src/test/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteriaTest.java @@ -30,6 +30,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import java.util.List; import org.hisp.dhis.webapi.controller.event.mapper.SortDirection; @@ -79,13 +80,7 @@ void fromOrderStringDefaultsToAscGivenFieldAndColon() { @Test void fromOrderStringDefaultsSortDirectionToAscGivenAnUnknownSortDirection() { - - List orderCriteria = OrderCriteria.fromOrderString("one:wrong"); - - assertNotNull(orderCriteria); - assertEquals( - 1, orderCriteria.size(), String.format("Expected 1 item, instead got %s", orderCriteria)); - assertEquals(OrderCriteria.of("one", SortDirection.ASC), orderCriteria.get(0)); + assertThrows(IllegalArgumentException.class, () -> OrderCriteria.fromOrderString("one:wrong")); } @Test diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/StringToOrderCriteriaListConverter.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/common/OrderCriteriaParamEditor.java similarity index 76% rename from dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/StringToOrderCriteriaListConverter.java rename to dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/common/OrderCriteriaParamEditor.java index 1f1bccb01dd9..e3eb8cd6bf3f 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/StringToOrderCriteriaListConverter.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/common/OrderCriteriaParamEditor.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-2022, University of Oslo + * Copyright (c) 2004-2023, University of Oslo * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -25,20 +25,14 @@ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -package org.hisp.dhis.webapi.security.config; +package org.hisp.dhis.webapi.common; -import java.util.List; +import java.beans.PropertyEditorSupport; import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria; -import org.springframework.core.convert.converter.Converter; -/** - * String to Order Criterias converter for MVC requests - * - * @author Giuseppe Nespolino - */ -class StringToOrderCriteriaListConverter implements Converter> { +public class OrderCriteriaParamEditor extends PropertyEditorSupport { @Override - public List convert(String source) { - return OrderCriteria.fromOrderString(source); + public void setAsText(String source) { + setValue(OrderCriteria.toOrderCriteria(source)); } } diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/CrudControllerAdvice.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/CrudControllerAdvice.java index a7d5d7a1b5d3..39a15de9fdce 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/CrudControllerAdvice.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/CrudControllerAdvice.java @@ -77,7 +77,9 @@ import org.hisp.dhis.schema.SchemaPathException; import org.hisp.dhis.tracker.imports.TrackerIdSchemeParam; import org.hisp.dhis.util.DateUtils; +import org.hisp.dhis.webapi.common.OrderCriteriaParamEditor; import org.hisp.dhis.webapi.common.UIDParamEditor; +import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria; import org.hisp.dhis.webapi.controller.exception.MetadataImportConflictException; import org.hisp.dhis.webapi.controller.exception.MetadataSyncException; import org.hisp.dhis.webapi.controller.exception.MetadataVersionException; @@ -145,6 +147,7 @@ protected void initBinder(WebDataBinder binder) { IdentifiableProperty.class, new FromTextPropertyEditor(String::toUpperCase)); this.enumClasses.forEach(c -> binder.registerCustomEditor(c, new ConvertEnum(c))); binder.registerCustomEditor(TrackerIdSchemeParam.class, new IdSchemeParamEditor()); + binder.registerCustomEditor(OrderCriteria.class, new OrderCriteriaParamEditor()); binder.registerCustomEditor(UID.class, new UIDParamEditor()); } diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/WebMvcConfig.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/WebMvcConfig.java index 92bd83d8e57e..989d9319b6dc 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/WebMvcConfig.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/WebMvcConfig.java @@ -219,7 +219,6 @@ public void configureMessageConverters(List> converters) @Override protected void addFormatters(FormatterRegistry registry) { - registry.addConverter(new StringToOrderCriteriaListConverter()); registry.addConverter(new FieldPathConverter()); } diff --git a/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/OrderBindingTest.java b/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/OrderBindingTest.java new file mode 100644 index 000000000000..dc671a25c4c8 --- /dev/null +++ b/dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/OrderBindingTest.java @@ -0,0 +1,119 @@ +/* + * Copyright (c) 2004-2022, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * Neither the name of the HISP project nor the names of its contributors may + * be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.webapi.controller; + +import static org.hamcrest.core.StringContains.containsString; +import static org.hisp.dhis.dxf2.webmessage.WebMessageUtils.ok; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; + +import java.util.List; +import org.hisp.dhis.dxf2.webmessage.WebMessage; +import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.stereotype.Controller; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.setup.MockMvcBuilders; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.ResponseBody; + +class OrderBindingTest { + private static final String ENDPOINT = "/ordering"; + + private MockMvc mockMvc; + + @BeforeEach + public void setUp() { + mockMvc = + MockMvcBuilders.standaloneSetup(new OrderingController()) + .setControllerAdvice(new CrudControllerAdvice()) + .build(); + } + + @Test + void shouldReturnDefaultSortDirectionWhenNoSortDirectionIsPassedAsParameter() throws Exception { + mockMvc + .perform(get(ENDPOINT).param("order", "field")) + .andExpect(content().string(containsString("OK"))) + .andExpect(content().string(containsString("field"))) + .andExpect(content().string(containsString("ASC"))); + } + + @Test + void shouldReturnAscSortDirectionWhenAscSortDirectionIsPassedAsParameter() throws Exception { + mockMvc + .perform(get(ENDPOINT).param("order", "field:asc")) + .andExpect(content().string(containsString("OK"))) + .andExpect(content().string(containsString("field"))) + .andExpect(content().string(containsString("ASC"))); + } + + @Test + void shouldReturnDescSortDirectionWhenDescSortDirectionIsPassedAsParameter() throws Exception { + mockMvc + .perform(get(ENDPOINT).param("order", "field:desc")) + .andExpect(content().string(containsString("OK"))) + .andExpect(content().string(containsString("field"))) + .andExpect(content().string(containsString("DESC"))); + } + + @Test + void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameter() throws Exception { + mockMvc + .perform(get(ENDPOINT).param("order", "field:wrong")) + .andExpect(content().string(containsString("Bad Request"))) + .andExpect( + content() + .string( + containsString( + "'wrong' is not a valid sort direction. Valid values are: [ASC, DESC]"))); + } + + @Test + void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameterInAListOfOrders() + throws Exception { + mockMvc + .perform(get(ENDPOINT).param("order", "field1:wrong").param("order", "field2:asc")) + .andExpect(content().string(containsString("Bad Request"))) + .andExpect( + content() + .string( + containsString( + "'wrong' is not a valid sort direction. Valid values are: [ASC, DESC]"))); + } + + @Controller + private static class OrderingController extends CrudControllerAdvice { + @GetMapping(value = ENDPOINT) + public @ResponseBody WebMessage getOrder(@RequestParam List order) { + return ok(order.toString()); + } + } +}