Skip to content

Commit 0e4b8e6

Browse files
fboulnoisfboulnois
authored
[DT-824] Return 400 for invalid byte sequence (#2451)
1 parent 5ec63c2 commit 0e4b8e6

File tree

5 files changed

+120
-13
lines changed

5 files changed

+120
-13
lines changed

‎src/main/java/org/broadinstitute/consent/http/resources/Resource.java

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,12 @@
1515
import java.io.InputStream;
1616
import java.sql.SQLException;
1717
import java.sql.SQLSyntaxErrorException;
18-
import java.util.AbstractMap;
1918
import java.util.HashMap;
2019
import java.util.List;
2120
import java.util.Map;
2221
import java.util.Objects;
2322
import org.apache.commons.io.IOUtils;
23+
import org.apache.commons.lang3.tuple.ImmutablePair;
2424
import org.broadinstitute.consent.http.enumeration.UserRoles;
2525
import org.broadinstitute.consent.http.exceptions.ConsentConflictException;
2626
import org.broadinstitute.consent.http.exceptions.UnknownIdentifierException;
@@ -57,9 +57,14 @@ abstract public class Resource implements ConsentLogger {
5757
public static final String ITDIRECTOR = "ITDirector";
5858

5959
// NOTE: implement more Postgres vendor codes as we encounter them
60-
private static final Map<String, Integer> vendorCodeStatusMap = Map.ofEntries(
61-
new AbstractMap.SimpleEntry<>(PSQLState.UNIQUE_VIOLATION.getState(),
62-
Response.Status.CONFLICT.getStatusCode())
60+
private static final Map<String, ImmutablePair<Integer, String>> vendorCodeStatusMap = Map.ofEntries(
61+
Map.entry(PSQLState.UNKNOWN_STATE.getState(),
62+
ImmutablePair.of(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
63+
"Database error")),
64+
Map.entry(PSQLState.UNIQUE_VIOLATION.getState(),
65+
ImmutablePair.of(Response.Status.CONFLICT.getStatusCode(), "Database conflict")),
66+
Map.entry("22021",
67+
ImmutablePair.of(Response.Status.BAD_REQUEST.getStatusCode(), "Invalid byte sequence"))
6368
);
6469

6570
protected Response createExceptionResponse(Exception e) {
@@ -179,12 +184,13 @@ private static Response errorLoggedExceptionHandler(Exception e, Error error) {
179184
}
180185

181186
//Helper method to process generic JDBI Postgres exceptions for responses
182-
private static Response unableToExecuteExceptionHandler(Exception e) {
187+
protected static Response unableToExecuteExceptionHandler(Exception e) {
183188
//default status definition
184189
LoggerFactory.getLogger(Resource.class.getName()).error(e.getMessage());
185190
// static makes using the interface less flexible
186191
Sentry.captureEvent(new SentryEvent(e));
187-
Integer status = Response.Status.INTERNAL_SERVER_ERROR.getStatusCode();
192+
193+
var status = vendorCodeStatusMap.get(PSQLState.UNKNOWN_STATE.getState());
188194

189195
try {
190196
if (e.getCause() instanceof PSQLException) {
@@ -197,9 +203,12 @@ private static Response unableToExecuteExceptionHandler(Exception e) {
197203
//no need to handle, default status already assigned
198204
}
199205

200-
return Response.status(status)
206+
int statusCode = status.getLeft();
207+
String message = status.getRight();
208+
209+
return Response.status(statusCode)
201210
.type(MediaType.APPLICATION_JSON)
202-
.entity(new Error("Database Error", status))
211+
.entity(new Error(message, statusCode))
203212
.build();
204213
}
205214

‎src/test/java/org/broadinstitute/consent/http/db/DAOTestHelper.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@
1212
import java.util.Random;
1313
import java.util.UUID;
1414
import org.apache.commons.lang3.RandomStringUtils;
15-
import org.broadinstitute.consent.http.ConsentApplication;
1615
import org.broadinstitute.consent.http.AbstractTestHelper;
16+
import org.broadinstitute.consent.http.ConsentApplication;
1717
import org.broadinstitute.consent.http.configurations.ConsentConfiguration;
1818
import org.broadinstitute.consent.http.enumeration.OrganizationType;
1919
import org.broadinstitute.consent.http.enumeration.UserFields;
@@ -33,6 +33,7 @@
3333
import org.junit.jupiter.api.AfterEach;
3434
import org.junit.jupiter.api.BeforeAll;
3535
import org.testcontainers.containers.PostgreSQLContainer;
36+
import org.testcontainers.containers.wait.strategy.Wait;
3637

3738
public class DAOTestHelper extends AbstractTestHelper {
3839

@@ -74,7 +75,8 @@ public class DAOTestHelper extends AbstractTestHelper {
7475
public static void startUp() throws Exception {
7576
// Start the database
7677
postgresContainer = new PostgreSQLContainer<>(POSTGRES_IMAGE).
77-
withCommand("postgres -c max_connections=" + maxConnections);
78+
withCommand("postgres -c max_connections=" + maxConnections).
79+
waitingFor(Wait.forListeningPorts());
7880
postgresContainer.start();
7981
ConfigOverride driverOverride = ConfigOverride.config("database.driverClass",
8082
postgresContainer.getDriverClassName());

‎src/test/java/org/broadinstitute/consent/http/db/UserDAOTest.java

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package org.broadinstitute.consent.http.db;
22

3+
import static org.junit.Assert.assertThrows;
34
import static org.junit.jupiter.api.Assertions.assertEquals;
45
import static org.junit.jupiter.api.Assertions.assertFalse;
56
import static org.junit.jupiter.api.Assertions.assertNotEquals;
@@ -30,6 +31,7 @@
3031
import org.broadinstitute.consent.http.models.LibraryCard;
3132
import org.broadinstitute.consent.http.models.User;
3233
import org.broadinstitute.consent.http.models.UserRole;
34+
import org.jdbi.v3.core.statement.UnableToExecuteStatementException;
3335
import org.junit.jupiter.api.Test;
3436
import org.junit.jupiter.api.extension.ExtendWith;
3537
import org.mockito.junit.jupiter.MockitoExtension;
@@ -206,7 +208,8 @@ void testDeleteUserById() {
206208
@Test
207209
void testFindUsersWithLCsAndInstitution() {
208210
User user = createUserWithInstitution();
209-
int dacId = dacDAO.createDac(RandomStringUtils.randomAlphabetic(5), RandomStringUtils.randomAlphabetic(5), new Date());
211+
int dacId = dacDAO.createDac(RandomStringUtils.randomAlphabetic(5),
212+
RandomStringUtils.randomAlphabetic(5), new Date());
210213
Instant now = Instant.now();
211214
int daaId = daaDAO.createDaa(user.getUserId(), now, user.getUserId(), now, dacId);
212215
int lcId1 = libraryCardDAO.insertLibraryCard(user.getUserId(), user.getInstitutionId(), "asdf",
@@ -270,6 +273,14 @@ void testUpdateDisplayName() {
270273
assertEquals(newName, u1.getDisplayName());
271274
}
272275

276+
@Test
277+
void testUpdateDisplayNameInvalidChars() {
278+
User researcher = createUserWithRole(UserRoles.RESEARCHER.getRoleId());
279+
String newName = "invalid\0name";
280+
assertThrows(UnableToExecuteStatementException.class,
281+
() -> userDAO.updateDisplayName(researcher.getUserId(), newName));
282+
}
283+
273284
@Test
274285
void testFindUserByEmailAndRoleId() {
275286
User chair = createUserWithRole(UserRoles.CHAIRPERSON.getRoleId());
@@ -341,7 +352,8 @@ void testGetSOsByInstitution() {
341352

342353
@Test
343354
void testGetUsersFromInstitutionWithCards() {
344-
int dacId = dacDAO.createDac(RandomStringUtils.randomAlphabetic(5), RandomStringUtils.randomAlphabetic(5), new Date());
355+
int dacId = dacDAO.createDac(RandomStringUtils.randomAlphabetic(5),
356+
RandomStringUtils.randomAlphabetic(5), new Date());
345357
Instant now = Instant.now();
346358
LibraryCard card = createLibraryCard();
347359
int daaId = daaDAO.createDaa(card.getUserId(), now, card.getUserId(), now, dacId);
@@ -360,7 +372,8 @@ void testGetUsersFromInstitutionWithCards() {
360372

361373
@Test
362374
void testGetUsersWithCardsByDaaId() {
363-
int dacId = dacDAO.createDac(RandomStringUtils.randomAlphabetic(5), RandomStringUtils.randomAlphabetic(5), new Date());
375+
int dacId = dacDAO.createDac(RandomStringUtils.randomAlphabetic(5),
376+
RandomStringUtils.randomAlphabetic(5), new Date());
364377
Instant now = Instant.now();
365378
LibraryCard card = createLibraryCard();
366379
int daaId = daaDAO.createDaa(card.getUserId(), now, card.getUserId(), now, dacId);

‎src/test/java/org/broadinstitute/consent/http/resources/ResourceTest.java

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,72 @@
11
package org.broadinstitute.consent.http.resources;
22

3+
import static org.hamcrest.MatcherAssert.assertThat;
4+
import static org.hamcrest.Matchers.is;
35
import static org.junit.jupiter.api.Assertions.assertThrows;
46
import static org.junit.jupiter.api.Assertions.fail;
57
import static org.mockito.Mockito.mock;
68
import static org.mockito.Mockito.when;
79

10+
import org.broadinstitute.consent.http.models.Error;
811
import org.glassfish.jersey.media.multipart.FormDataContentDisposition;
12+
import org.jdbi.v3.core.statement.StatementContext;
13+
import org.jdbi.v3.core.statement.StatementExceptions;
14+
import org.jdbi.v3.core.statement.UnableToExecuteStatementException;
915
import org.junit.jupiter.api.Test;
1016
import org.junit.jupiter.api.extension.ExtendWith;
1117
import org.mockito.Mockito;
1218
import org.mockito.junit.jupiter.MockitoExtension;
1319
import org.owasp.fileio.FileValidator;
20+
import org.postgresql.util.PSQLException;
21+
import org.postgresql.util.PSQLState;
1422

1523
@ExtendWith(MockitoExtension.class)
1624
class ResourceTest {
1725

26+
@Test
27+
void testUnableToExecuteExceptionGeneric() {
28+
var error = new UnableToExecuteStatementException("generic error");
29+
var result = Resource.unableToExecuteExceptionHandler(error);
30+
var entity = (Error) result.getEntity();
31+
assertThat(result.getStatus(), is(500));
32+
assertThat(entity.message(), is("Database error"));
33+
}
34+
35+
@Test
36+
void testUnableToExecuteExceptionDatabaseConflict() {
37+
PSQLException psqlException = new PSQLException(
38+
"duplicate key value violates unique constraint", PSQLState.UNIQUE_VIOLATION);
39+
StatementContext ctx = mock(StatementContext.class);
40+
StatementExceptions exceptions = mock(StatementExceptions.class);
41+
when(ctx.getConfig(StatementExceptions.class)).thenReturn(exceptions);
42+
UnableToExecuteStatementException exception = new UnableToExecuteStatementException(
43+
"Failed to execute statement", psqlException, ctx);
44+
45+
var result = Resource.unableToExecuteExceptionHandler(exception);
46+
var entity = (Error) result.getEntity();
47+
assertThat(result.getStatus(), is(409));
48+
assertThat(entity.message(), is("Database conflict"));
49+
}
50+
51+
@Test
52+
void testUnableToExecuteExceptionInvalidByteSequence() {
53+
PSQLState psqlState = mock(PSQLState.class);
54+
// PSQLState is missing the enum constant 22021 for invalid byte sequence but returns it so we mock it
55+
when(psqlState.getState()).thenReturn("22021");
56+
PSQLException psqlException = new PSQLException(
57+
"invalid byte sequence for encoding \"UTF8\": 0x00", psqlState);
58+
StatementContext ctx = mock(StatementContext.class);
59+
StatementExceptions exceptions = mock(StatementExceptions.class);
60+
when(ctx.getConfig(StatementExceptions.class)).thenReturn(exceptions);
61+
UnableToExecuteStatementException exception = new UnableToExecuteStatementException(
62+
"Failed to execute statement", psqlException, ctx);
63+
64+
var result = Resource.unableToExecuteExceptionHandler(exception);
65+
var entity = (Error) result.getEntity();
66+
assertThat(result.getStatus(), is(400));
67+
assertThat(entity.message(), is("Invalid byte sequence"));
68+
}
69+
1870
@Test
1971
void testValidateFileDetails() {
2072
Long maxSize = new FileValidator().getMaxFileUploadSize();

‎src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import static org.mockito.ArgumentMatchers.anyString;
1010
import static org.mockito.Mockito.doNothing;
1111
import static org.mockito.Mockito.doThrow;
12+
import static org.mockito.Mockito.mock;
1213
import static org.mockito.Mockito.when;
1314

1415
import com.google.api.client.http.HttpStatusCodes;
@@ -47,10 +48,15 @@
4748
import org.broadinstitute.consent.http.service.UserService;
4849
import org.broadinstitute.consent.http.service.sam.SamService;
4950
import org.broadinstitute.consent.http.util.gson.GsonUtil;
51+
import org.jdbi.v3.core.statement.StatementContext;
52+
import org.jdbi.v3.core.statement.StatementExceptions;
53+
import org.jdbi.v3.core.statement.UnableToExecuteStatementException;
5054
import org.junit.jupiter.api.Test;
5155
import org.junit.jupiter.api.extension.ExtendWith;
5256
import org.mockito.Mock;
5357
import org.mockito.junit.jupiter.MockitoExtension;
58+
import org.postgresql.util.PSQLException;
59+
import org.postgresql.util.PSQLState;
5460

5561
@ExtendWith(MockitoExtension.class)
5662
class UserResourceTest {
@@ -456,6 +462,31 @@ void testUpdateSelf() {
456462
assertEquals(HttpStatusCodes.STATUS_CODE_OK, response.getStatus());
457463
}
458464

465+
@Test
466+
void testUpdateSelfInvalidName() {
467+
PSQLState psqlState = mock(PSQLState.class);
468+
// PSQLState is missing the enum constant 22021 for invalid byte sequence but returns it so we mock it
469+
when(psqlState.getState()).thenReturn("22021");
470+
PSQLException psqlException = new PSQLException(
471+
"invalid byte sequence for encoding \"UTF8\": 0x00", psqlState);
472+
StatementContext ctx = mock(StatementContext.class);
473+
StatementExceptions exceptions = mock(StatementExceptions.class);
474+
when(ctx.getConfig(StatementExceptions.class)).thenReturn(exceptions);
475+
UnableToExecuteStatementException exception = new UnableToExecuteStatementException(
476+
"Failed to execute statement", psqlException, ctx);
477+
478+
User user = createUserWithRole();
479+
String invalidName = "invalid\0name";
480+
UserUpdateFields userUpdateFields = new UserUpdateFields();
481+
userUpdateFields.setDisplayName(invalidName);
482+
when(userService.findUserByEmail(any())).thenReturn(user);
483+
when(userService.updateUserFieldsById(any(), any())).thenThrow(exception);
484+
initResource();
485+
486+
Response response = userResource.updateSelf(authUser, uriInfo, gson.toJson(userUpdateFields));
487+
assertEquals(HttpStatusCodes.STATUS_CODE_BAD_REQUEST, response.getStatus());
488+
}
489+
459490
@Test
460491
void testUpdateSelfRolesNotAdmin() {
461492
User user = createUserWithRole();

0 commit comments

Comments
 (0)