From 0e4b8e66e5abb3d6c49d7e54c93040676c3ed51c Mon Sep 17 00:00:00 2001 From: fboulnois Date: Mon, 27 Jan 2025 12:45:09 -0500 Subject: [PATCH] [DT-824] Return 400 for invalid byte sequence (#2451) --- .../consent/http/resources/Resource.java | 25 ++++++--- .../consent/http/db/DAOTestHelper.java | 6 ++- .../consent/http/db/UserDAOTest.java | 19 +++++-- .../consent/http/resources/ResourceTest.java | 52 +++++++++++++++++++ .../http/resources/UserResourceTest.java | 31 +++++++++++ 5 files changed, 120 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/broadinstitute/consent/http/resources/Resource.java b/src/main/java/org/broadinstitute/consent/http/resources/Resource.java index a61998c007..3d70c2caae 100644 --- a/src/main/java/org/broadinstitute/consent/http/resources/Resource.java +++ b/src/main/java/org/broadinstitute/consent/http/resources/Resource.java @@ -15,12 +15,12 @@ import java.io.InputStream; import java.sql.SQLException; import java.sql.SQLSyntaxErrorException; -import java.util.AbstractMap; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; import org.apache.commons.io.IOUtils; +import org.apache.commons.lang3.tuple.ImmutablePair; import org.broadinstitute.consent.http.enumeration.UserRoles; import org.broadinstitute.consent.http.exceptions.ConsentConflictException; import org.broadinstitute.consent.http.exceptions.UnknownIdentifierException; @@ -57,9 +57,14 @@ abstract public class Resource implements ConsentLogger { public static final String ITDIRECTOR = "ITDirector"; // NOTE: implement more Postgres vendor codes as we encounter them - private static final Map vendorCodeStatusMap = Map.ofEntries( - new AbstractMap.SimpleEntry<>(PSQLState.UNIQUE_VIOLATION.getState(), - Response.Status.CONFLICT.getStatusCode()) + private static final Map> vendorCodeStatusMap = Map.ofEntries( + Map.entry(PSQLState.UNKNOWN_STATE.getState(), + ImmutablePair.of(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), + "Database error")), + Map.entry(PSQLState.UNIQUE_VIOLATION.getState(), + ImmutablePair.of(Response.Status.CONFLICT.getStatusCode(), "Database conflict")), + Map.entry("22021", + ImmutablePair.of(Response.Status.BAD_REQUEST.getStatusCode(), "Invalid byte sequence")) ); protected Response createExceptionResponse(Exception e) { @@ -179,12 +184,13 @@ private static Response errorLoggedExceptionHandler(Exception e, Error error) { } //Helper method to process generic JDBI Postgres exceptions for responses - private static Response unableToExecuteExceptionHandler(Exception e) { + protected static Response unableToExecuteExceptionHandler(Exception e) { //default status definition LoggerFactory.getLogger(Resource.class.getName()).error(e.getMessage()); // static makes using the interface less flexible Sentry.captureEvent(new SentryEvent(e)); - Integer status = Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(); + + var status = vendorCodeStatusMap.get(PSQLState.UNKNOWN_STATE.getState()); try { if (e.getCause() instanceof PSQLException) { @@ -197,9 +203,12 @@ private static Response unableToExecuteExceptionHandler(Exception e) { //no need to handle, default status already assigned } - return Response.status(status) + int statusCode = status.getLeft(); + String message = status.getRight(); + + return Response.status(statusCode) .type(MediaType.APPLICATION_JSON) - .entity(new Error("Database Error", status)) + .entity(new Error(message, statusCode)) .build(); } diff --git a/src/test/java/org/broadinstitute/consent/http/db/DAOTestHelper.java b/src/test/java/org/broadinstitute/consent/http/db/DAOTestHelper.java index dc8d3bf283..48d01da076 100644 --- a/src/test/java/org/broadinstitute/consent/http/db/DAOTestHelper.java +++ b/src/test/java/org/broadinstitute/consent/http/db/DAOTestHelper.java @@ -12,8 +12,8 @@ import java.util.Random; import java.util.UUID; import org.apache.commons.lang3.RandomStringUtils; -import org.broadinstitute.consent.http.ConsentApplication; import org.broadinstitute.consent.http.AbstractTestHelper; +import org.broadinstitute.consent.http.ConsentApplication; import org.broadinstitute.consent.http.configurations.ConsentConfiguration; import org.broadinstitute.consent.http.enumeration.OrganizationType; import org.broadinstitute.consent.http.enumeration.UserFields; @@ -33,6 +33,7 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.testcontainers.containers.PostgreSQLContainer; +import org.testcontainers.containers.wait.strategy.Wait; public class DAOTestHelper extends AbstractTestHelper { @@ -74,7 +75,8 @@ public class DAOTestHelper extends AbstractTestHelper { public static void startUp() throws Exception { // Start the database postgresContainer = new PostgreSQLContainer<>(POSTGRES_IMAGE). - withCommand("postgres -c max_connections=" + maxConnections); + withCommand("postgres -c max_connections=" + maxConnections). + waitingFor(Wait.forListeningPorts()); postgresContainer.start(); ConfigOverride driverOverride = ConfigOverride.config("database.driverClass", postgresContainer.getDriverClassName()); diff --git a/src/test/java/org/broadinstitute/consent/http/db/UserDAOTest.java b/src/test/java/org/broadinstitute/consent/http/db/UserDAOTest.java index e622404693..c8f5575009 100644 --- a/src/test/java/org/broadinstitute/consent/http/db/UserDAOTest.java +++ b/src/test/java/org/broadinstitute/consent/http/db/UserDAOTest.java @@ -1,5 +1,6 @@ package org.broadinstitute.consent.http.db; +import static org.junit.Assert.assertThrows; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -30,6 +31,7 @@ import org.broadinstitute.consent.http.models.LibraryCard; import org.broadinstitute.consent.http.models.User; import org.broadinstitute.consent.http.models.UserRole; +import org.jdbi.v3.core.statement.UnableToExecuteStatementException; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.junit.jupiter.MockitoExtension; @@ -206,7 +208,8 @@ void testDeleteUserById() { @Test void testFindUsersWithLCsAndInstitution() { User user = createUserWithInstitution(); - int dacId = dacDAO.createDac(RandomStringUtils.randomAlphabetic(5), RandomStringUtils.randomAlphabetic(5), new Date()); + int dacId = dacDAO.createDac(RandomStringUtils.randomAlphabetic(5), + RandomStringUtils.randomAlphabetic(5), new Date()); Instant now = Instant.now(); int daaId = daaDAO.createDaa(user.getUserId(), now, user.getUserId(), now, dacId); int lcId1 = libraryCardDAO.insertLibraryCard(user.getUserId(), user.getInstitutionId(), "asdf", @@ -270,6 +273,14 @@ void testUpdateDisplayName() { assertEquals(newName, u1.getDisplayName()); } + @Test + void testUpdateDisplayNameInvalidChars() { + User researcher = createUserWithRole(UserRoles.RESEARCHER.getRoleId()); + String newName = "invalid\0name"; + assertThrows(UnableToExecuteStatementException.class, + () -> userDAO.updateDisplayName(researcher.getUserId(), newName)); + } + @Test void testFindUserByEmailAndRoleId() { User chair = createUserWithRole(UserRoles.CHAIRPERSON.getRoleId()); @@ -341,7 +352,8 @@ void testGetSOsByInstitution() { @Test void testGetUsersFromInstitutionWithCards() { - int dacId = dacDAO.createDac(RandomStringUtils.randomAlphabetic(5), RandomStringUtils.randomAlphabetic(5), new Date()); + int dacId = dacDAO.createDac(RandomStringUtils.randomAlphabetic(5), + RandomStringUtils.randomAlphabetic(5), new Date()); Instant now = Instant.now(); LibraryCard card = createLibraryCard(); int daaId = daaDAO.createDaa(card.getUserId(), now, card.getUserId(), now, dacId); @@ -360,7 +372,8 @@ void testGetUsersFromInstitutionWithCards() { @Test void testGetUsersWithCardsByDaaId() { - int dacId = dacDAO.createDac(RandomStringUtils.randomAlphabetic(5), RandomStringUtils.randomAlphabetic(5), new Date()); + int dacId = dacDAO.createDac(RandomStringUtils.randomAlphabetic(5), + RandomStringUtils.randomAlphabetic(5), new Date()); Instant now = Instant.now(); LibraryCard card = createLibraryCard(); int daaId = daaDAO.createDaa(card.getUserId(), now, card.getUserId(), now, dacId); diff --git a/src/test/java/org/broadinstitute/consent/http/resources/ResourceTest.java b/src/test/java/org/broadinstitute/consent/http/resources/ResourceTest.java index 29de70af59..fd344eb25e 100644 --- a/src/test/java/org/broadinstitute/consent/http/resources/ResourceTest.java +++ b/src/test/java/org/broadinstitute/consent/http/resources/ResourceTest.java @@ -1,20 +1,72 @@ package org.broadinstitute.consent.http.resources; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import org.broadinstitute.consent.http.models.Error; import org.glassfish.jersey.media.multipart.FormDataContentDisposition; +import org.jdbi.v3.core.statement.StatementContext; +import org.jdbi.v3.core.statement.StatementExceptions; +import org.jdbi.v3.core.statement.UnableToExecuteStatementException; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; import org.owasp.fileio.FileValidator; +import org.postgresql.util.PSQLException; +import org.postgresql.util.PSQLState; @ExtendWith(MockitoExtension.class) class ResourceTest { + @Test + void testUnableToExecuteExceptionGeneric() { + var error = new UnableToExecuteStatementException("generic error"); + var result = Resource.unableToExecuteExceptionHandler(error); + var entity = (Error) result.getEntity(); + assertThat(result.getStatus(), is(500)); + assertThat(entity.message(), is("Database error")); + } + + @Test + void testUnableToExecuteExceptionDatabaseConflict() { + PSQLException psqlException = new PSQLException( + "duplicate key value violates unique constraint", PSQLState.UNIQUE_VIOLATION); + StatementContext ctx = mock(StatementContext.class); + StatementExceptions exceptions = mock(StatementExceptions.class); + when(ctx.getConfig(StatementExceptions.class)).thenReturn(exceptions); + UnableToExecuteStatementException exception = new UnableToExecuteStatementException( + "Failed to execute statement", psqlException, ctx); + + var result = Resource.unableToExecuteExceptionHandler(exception); + var entity = (Error) result.getEntity(); + assertThat(result.getStatus(), is(409)); + assertThat(entity.message(), is("Database conflict")); + } + + @Test + void testUnableToExecuteExceptionInvalidByteSequence() { + PSQLState psqlState = mock(PSQLState.class); + // PSQLState is missing the enum constant 22021 for invalid byte sequence but returns it so we mock it + when(psqlState.getState()).thenReturn("22021"); + PSQLException psqlException = new PSQLException( + "invalid byte sequence for encoding \"UTF8\": 0x00", psqlState); + StatementContext ctx = mock(StatementContext.class); + StatementExceptions exceptions = mock(StatementExceptions.class); + when(ctx.getConfig(StatementExceptions.class)).thenReturn(exceptions); + UnableToExecuteStatementException exception = new UnableToExecuteStatementException( + "Failed to execute statement", psqlException, ctx); + + var result = Resource.unableToExecuteExceptionHandler(exception); + var entity = (Error) result.getEntity(); + assertThat(result.getStatus(), is(400)); + assertThat(entity.message(), is("Invalid byte sequence")); + } + @Test void testValidateFileDetails() { Long maxSize = new FileValidator().getMaxFileUploadSize(); diff --git a/src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java b/src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java index 3f3cf1a0ff..50ccad233e 100644 --- a/src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java +++ b/src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java @@ -9,6 +9,7 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import com.google.api.client.http.HttpStatusCodes; @@ -47,10 +48,15 @@ import org.broadinstitute.consent.http.service.UserService; import org.broadinstitute.consent.http.service.sam.SamService; import org.broadinstitute.consent.http.util.gson.GsonUtil; +import org.jdbi.v3.core.statement.StatementContext; +import org.jdbi.v3.core.statement.StatementExceptions; +import org.jdbi.v3.core.statement.UnableToExecuteStatementException; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.postgresql.util.PSQLException; +import org.postgresql.util.PSQLState; @ExtendWith(MockitoExtension.class) class UserResourceTest { @@ -456,6 +462,31 @@ void testUpdateSelf() { assertEquals(HttpStatusCodes.STATUS_CODE_OK, response.getStatus()); } + @Test + void testUpdateSelfInvalidName() { + PSQLState psqlState = mock(PSQLState.class); + // PSQLState is missing the enum constant 22021 for invalid byte sequence but returns it so we mock it + when(psqlState.getState()).thenReturn("22021"); + PSQLException psqlException = new PSQLException( + "invalid byte sequence for encoding \"UTF8\": 0x00", psqlState); + StatementContext ctx = mock(StatementContext.class); + StatementExceptions exceptions = mock(StatementExceptions.class); + when(ctx.getConfig(StatementExceptions.class)).thenReturn(exceptions); + UnableToExecuteStatementException exception = new UnableToExecuteStatementException( + "Failed to execute statement", psqlException, ctx); + + User user = createUserWithRole(); + String invalidName = "invalid\0name"; + UserUpdateFields userUpdateFields = new UserUpdateFields(); + userUpdateFields.setDisplayName(invalidName); + when(userService.findUserByEmail(any())).thenReturn(user); + when(userService.updateUserFieldsById(any(), any())).thenThrow(exception); + initResource(); + + Response response = userResource.updateSelf(authUser, uriInfo, gson.toJson(userUpdateFields)); + assertEquals(HttpStatusCodes.STATUS_CODE_BAD_REQUEST, response.getStatus()); + } + @Test void testUpdateSelfRolesNotAdmin() { User user = createUserWithRole();