Skip to content

Commit 30fce5b

Browse files
committed
fix: reject invalid order parameter values DHIS2-16935
1 parent 6139218 commit 30fce5b

File tree

9 files changed

+440
-137
lines changed

9 files changed

+440
-137
lines changed

dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/mapper/SortDirection.java

+8-5
Original file line numberDiff line numberDiff line change
@@ -46,14 +46,17 @@ public enum SortDirection {
4646
private final boolean ignoreCase;
4747

4848
public static SortDirection of(String value) {
49-
return of(value, DEFAULT_SORTING_DIRECTION);
50-
}
51-
52-
public static SortDirection of(String value, SortDirection defaultSortingDirection) {
5349
return Arrays.stream(values())
5450
.filter(sortDirection -> sortDirection.getValue().equalsIgnoreCase(value))
5551
.findFirst()
56-
.orElse(defaultSortingDirection);
52+
.orElseThrow(
53+
() ->
54+
new IllegalArgumentException(
55+
"'"
56+
+ value
57+
+ "' is not a valid sort direction. Valid values are: "
58+
+ Arrays.toString(SortDirection.values())
59+
+ "."));
5760
}
5861

5962
public boolean isAscending() {

dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java

+28-10
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,9 @@
4646
@Value
4747
@AllArgsConstructor(staticName = "of")
4848
public class OrderCriteria {
49-
private final String field;
49+
String field;
5050

51-
private final SortDirection direction;
51+
SortDirection direction;
5252

5353
public OrderParam toOrderParam() {
5454
return new OrderParam(field, direction);
@@ -64,18 +64,36 @@ public static List<OrderCriteria> fromOrderString(String source) {
6464

6565
private static List<OrderCriteria> toOrderCriterias(String s) {
6666
return Arrays.stream(s.split(","))
67-
.map(OrderCriteria::toOrderCriteria)
67+
.filter(StringUtils::isNotBlank)
68+
.map(OrderCriteria::valueOf)
6869
.collect(Collectors.toList());
6970
}
7071

71-
private static OrderCriteria toOrderCriteria(String s1) {
72-
String[] props = s1.split(":");
73-
if (props.length == 2) {
74-
return OrderCriteria.of(props[0], SortDirection.of(props[1]));
72+
/**
73+
* Create an {@link OrderCriteria} from a string in the format of "field:direction". Valid
74+
* directions are defined by {@link SortDirection}.
75+
*
76+
* @throws IllegalArgumentException if the input is not in the correct format.
77+
*/
78+
public static OrderCriteria valueOf(String input) {
79+
String[] props = input.split(":");
80+
if (props.length > 2) {
81+
throw new IllegalArgumentException(
82+
"Invalid order property: '"
83+
+ input
84+
+ "'. Valid formats are 'field:direction' or 'field'. Valid directions are 'asc' or 'desc'. Direction defaults to 'asc'.");
7585
}
76-
if (props.length == 1) {
77-
return OrderCriteria.of(props[0], SortDirection.ASC);
86+
87+
String field = props[0].trim();
88+
if (StringUtils.isEmpty(field)) {
89+
throw new IllegalArgumentException(
90+
"Missing field name in order property: '"
91+
+ input
92+
+ "'. Valid formats are 'field:direction' or 'field'. Valid directions are 'asc' or 'desc'. Direction defaults to 'asc'.");
7893
}
79-
return null;
94+
95+
SortDirection direction =
96+
props.length == 1 ? SortDirection.ASC : SortDirection.of(props[1].trim());
97+
return OrderCriteria.of(field, direction);
8098
}
8199
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
/*
2+
* Copyright (c) 2004-2023, University of Oslo
3+
* All rights reserved.
4+
*
5+
* Redistribution and use in source and binary forms, with or without
6+
* modification, are permitted provided that the following conditions are met:
7+
* Redistributions of source code must retain the above copyright notice, this
8+
* list of conditions and the following disclaimer.
9+
*
10+
* Redistributions in binary form must reproduce the above copyright notice,
11+
* this list of conditions and the following disclaimer in the documentation
12+
* and/or other materials provided with the distribution.
13+
* Neither the name of the HISP project nor the names of its contributors may
14+
* be used to endorse or promote products derived from this software without
15+
* specific prior written permission.
16+
*
17+
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
18+
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
19+
* WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
20+
* DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
21+
* ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
22+
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
23+
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
24+
* ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
25+
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
26+
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
27+
*/
28+
package org.hisp.dhis.webapi.controller.event.webrequest;
29+
30+
import static org.junit.jupiter.api.Assertions.assertEquals;
31+
import static org.junit.jupiter.api.Assertions.assertNotNull;
32+
import static org.junit.jupiter.api.Assertions.assertThrows;
33+
34+
import java.util.List;
35+
import org.hisp.dhis.webapi.controller.event.mapper.SortDirection;
36+
import org.junit.jupiter.api.Test;
37+
import org.junit.jupiter.params.ParameterizedTest;
38+
import org.junit.jupiter.params.provider.ValueSource;
39+
40+
class OrderCriteriaTest {
41+
42+
@Test
43+
void fromOrderString() {
44+
List<OrderCriteria> order = OrderCriteria.fromOrderString("one:desc,, two:asc ,three ");
45+
46+
assertEquals(
47+
List.of(
48+
OrderCriteria.of("one", SortDirection.DESC),
49+
OrderCriteria.of("two", SortDirection.ASC),
50+
OrderCriteria.of("three", SortDirection.ASC)),
51+
order);
52+
}
53+
54+
@ValueSource(strings = {"one:desc", "one:Desc", "one:DESC"})
55+
@ParameterizedTest
56+
void fromOrderStringSortDirectionParsingIsCaseInsensitive(String source) {
57+
List<OrderCriteria> order = OrderCriteria.fromOrderString(source);
58+
59+
assertEquals(List.of(OrderCriteria.of("one", SortDirection.DESC)), order);
60+
}
61+
62+
@Test
63+
void fromOrderStringDefaultsToAscGivenFieldAndColon() {
64+
List<OrderCriteria> order = OrderCriteria.fromOrderString("one:");
65+
66+
assertEquals(List.of(OrderCriteria.of("one", SortDirection.ASC)), order);
67+
}
68+
69+
@Test
70+
void failGivenAnUnknownSortDirection() {
71+
assertThrows(IllegalArgumentException.class, () -> OrderCriteria.fromOrderString("one:wrong"));
72+
}
73+
74+
@Test
75+
void fromOrderStringReturnsEmptyListGivenEmptyOrder() {
76+
List<OrderCriteria> orderCriteria = OrderCriteria.fromOrderString(" ");
77+
78+
assertNotNull(orderCriteria);
79+
assertEquals(
80+
0, orderCriteria.size(), String.format("Expected 0 items, instead got %s", orderCriteria));
81+
}
82+
83+
@Test
84+
void failGivenMoreThanTwoColons() {
85+
assertThrows(
86+
IllegalArgumentException.class, () -> OrderCriteria.fromOrderString("one:desc:wrong"));
87+
}
88+
89+
@Test
90+
void failGivenEmptyField() {
91+
assertThrows(IllegalArgumentException.class, () -> OrderCriteria.valueOf(" :desc"));
92+
}
93+
}
Original file line numberDiff line numberDiff line change
@@ -1,75 +1,83 @@
11
{
2-
"id": "ctZIHTIIlYS",
3-
"name": "TA Tracker program filter",
4-
"program": {
5-
"id": "f1AyMswryyQ"
2+
"id": "ctZIHTIIlYS",
3+
"name": "TA Tracker program filter",
4+
"program": {
5+
"id": "f1AyMswryyQ"
6+
},
7+
"sharing": {
8+
"public": "rw------",
9+
"userGroups": {
10+
"OPVIvvXzNTw": {
11+
"access": "rw------",
12+
"id": "OPVIvvXzNTw"
13+
}
14+
}
15+
},
16+
"entityQueryCriteria": {
17+
"enrollmentStatus": "COMPLETED",
18+
"followUp": true,
19+
"ouMode": "ACCESSIBLE",
20+
"displayColumnOrder": [
21+
"eventDate",
22+
"dueDate",
23+
"program",
24+
"invalid"
25+
],
26+
"assignedUserMode": "ANY",
27+
"programStage": "a3kGcGDCuk6",
28+
"trackedEntityType": "Q9GufDoplCL",
29+
"lastUpdatedDate": {
30+
"period": "TODAY",
31+
"startBuffer": -5,
32+
"endBuffer": 5,
33+
"type": "RELATIVE"
634
},
7-
"sharing": {
8-
"public": "rw------",
9-
"userGroups": {
10-
"OPVIvvXzNTw": {
11-
"access": "rw------",
12-
"id": "OPVIvvXzNTw"
13-
}
14-
}
35+
"enrollmentCreatedDate": {
36+
"period": "TODAY",
37+
"startBuffer": -5,
38+
"endBuffer": 5,
39+
"type": "RELATIVE"
40+
},
41+
"enrollmentIncidentDate": {
42+
"endDate": "2019-03-20T00:00:00.000",
43+
"type": "ABSOLUTE",
44+
"startDate": "2014-05-01T00:00:00.000"
1545
},
16-
"entityQueryCriteria": {
17-
"enrollmentStatus": "COMPLETED",
18-
"followUp": true,
19-
"ouMode": "ACCESSIBLE",
20-
"displayColumnOrder": [ "eventDate", "dueDate", "program", "invalid"],
21-
"assignedUserMode": "ANY",
22-
"programStage":"a3kGcGDCuk6",
23-
"trackedEntityType":"Q9GufDoplCL",
24-
"lastUpdatedDate": {
25-
"period": "TODAY",
26-
"startBuffer": -5,
27-
"endBuffer": 5,
28-
"type": "RELATIVE"
29-
},
30-
"enrollmentCreatedDate": {
31-
"period": "TODAY",
32-
"startBuffer": -5,
33-
"endBuffer": 5,
34-
"type": "RELATIVE"
35-
},
36-
"enrollmentIncidentDate": {
37-
"endDate": "2019-03-20T00:00:00.000",
38-
"type": "ABSOLUTE",
39-
"startDate": "2014-05-01T00:00:00.000"
40-
},
41-
"trackedEntityInstances": ["a3kGcGDCuk7", "a3kGcGDCuk8"],
42-
"order": "createdAt:desc;orgUnit:asc",
43-
"attributeValueFilters": [
44-
{
45-
"attribute": "dIVt4l5vIOa",
46-
"sw": "a",
47-
"ew": "e"
48-
},
49-
{
50-
"attribute": "kZeSYCgaHTk",
51-
"like": "abc"
52-
},
53-
{
54-
"attribute": "x5yfLot5VCM",
55-
"dateFilter": {
56-
"startDate": "2014-05-01T00:00:00.000",
57-
"endDate": "2019-03-20T00:00:00.000",
58-
"type": "ABSOLUTE"
59-
}
60-
},
61-
{
62-
"attribute": "ypGAwVRNtVY",
63-
"le": "20",
64-
"ge": "10"
65-
},
66-
{
67-
"attribute": "aIga5mPOFOJ",
68-
"in": [
69-
"MALE",
70-
"FEMALE"
71-
]
72-
}
46+
"trackedEntityInstances": [
47+
"a3kGcGDCuk7",
48+
"a3kGcGDCuk8"
49+
],
50+
"order": "createdAt:desc",
51+
"attributeValueFilters": [
52+
{
53+
"attribute": "dIVt4l5vIOa",
54+
"sw": "a",
55+
"ew": "e"
56+
},
57+
{
58+
"attribute": "kZeSYCgaHTk",
59+
"like": "abc"
60+
},
61+
{
62+
"attribute": "x5yfLot5VCM",
63+
"dateFilter": {
64+
"startDate": "2014-05-01T00:00:00.000",
65+
"endDate": "2019-03-20T00:00:00.000",
66+
"type": "ABSOLUTE"
67+
}
68+
},
69+
{
70+
"attribute": "ypGAwVRNtVY",
71+
"le": "20",
72+
"ge": "10"
73+
},
74+
{
75+
"attribute": "aIga5mPOFOJ",
76+
"in": [
77+
"MALE",
78+
"FEMALE"
7379
]
74-
}
75-
}
80+
}
81+
]
82+
}
83+
}

dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/MvcTestConfig.java

-2
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,6 @@
6060
import org.hisp.dhis.webapi.mvc.messageconverter.StreamingJsonRootMessageConverter;
6161
import org.hisp.dhis.webapi.mvc.messageconverter.XmlMessageConverter;
6262
import org.hisp.dhis.webapi.mvc.messageconverter.XmlPathMappingJackson2XmlHttpMessageConverter;
63-
import org.hisp.dhis.webapi.security.config.StringToOrderCriteriaListConverter;
6463
import org.hisp.dhis.webapi.view.CustomPathExtensionContentNegotiationStrategy;
6564
import org.mockito.Mockito;
6665
import org.springframework.beans.factory.annotation.Autowired;
@@ -273,7 +272,6 @@ public void configureMessageConverters(List<HttpMessageConverter<?>> converters)
273272

274273
@Override
275274
public void addFormatters(FormatterRegistry registry) {
276-
registry.addConverter(new StringToOrderCriteriaListConverter());
277275
registry.addConverter(new FieldPathConverter());
278276
}
279277

0 commit comments

Comments
 (0)