Fix race condition in doesProjectExist

... by enforcing uniqueness as database constraints, instead of trying to do it in the application code. This bug affects DT v4.x as well, but because we can't create partial indexes there (DataNucleus doesn't give us that much control), we can't really fix it. Fixes DependencyTrack/hyades#1101 Signed-off-by: nscuro <[email protected]>
DependencyTrack · Feb 28, 2024 · b77238c · b77238c
1 parent 26ad987
commit b77238c
Show file tree

Hide file tree

Showing 5 changed files with 112 additions and 37 deletions.
diff --git a/src/main/java/org/dependencytrack/resources/v1/ProjectResource.java b/src/main/java/org/dependencytrack/resources/v1/ProjectResource.java
@@ -61,6 +61,8 @@
 import java.util.function.BiConsumer;
 import java.util.function.Function;
 
+import static org.dependencytrack.util.PersistenceUtil.isUniqueConstraintViolation;
+
 /**
  * JAX-RS resources for processing projects.
  *
@@ -248,21 +250,24 @@ public Response createProject(Project jsonProject) {
                 Project parent = qm.getObjectByUuid(Project.class, jsonProject.getParent().getUuid());
                 jsonProject.setParent(parent);
             }
-            if (!qm.doesProjectExist(StringUtils.trimToNull(jsonProject.getName()), StringUtils.trimToNull(jsonProject.getVersion()))) {
-                final Project project;
-                try {
-                    project = qm.createProject(jsonProject, jsonProject.getTags(), true);
-                } catch (IllegalArgumentException e) {
-                    LOGGER.debug(e.getMessage());
-                    return Response.status(Response.Status.CONFLICT).entity("An inactive Parent cannot be selected as parent").build();
+            final Project project;
+            try {
+                project = qm.createProject(jsonProject, jsonProject.getTags(), true);
+            } catch (IllegalArgumentException e) {
+                LOGGER.debug(e.getMessage());
+                return Response.status(Response.Status.CONFLICT).entity("An inactive Parent cannot be selected as parent").build();
+            } catch (RuntimeException e) {
+                if (isUniqueConstraintViolation(e)) {
+                    return Response.status(Response.Status.CONFLICT).entity("A project with the specified name already exists.").build();
                 }
-                Principal principal = getPrincipal();
-                qm.updateNewProjectACL(project, principal);
-                LOGGER.info("Project " + project.toString() + " created by " + super.getPrincipal().getName());
-                return Response.status(Response.Status.CREATED).entity(project).build();
-            } else {
-                return Response.status(Response.Status.CONFLICT).entity("A project with the specified name already exists.").build();
+
+                LOGGER.error("Failed to create project %s".formatted(jsonProject), e);
+                return Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
             }
+            Principal principal = getPrincipal();
+            qm.updateNewProjectACL(project, principal);
+            LOGGER.info("Project " + project.toString() + " created by " + super.getPrincipal().getName());
+            return Response.status(Response.Status.CREATED).entity(project).build();
         }
     }
 
@@ -303,24 +308,25 @@ public Response updateProject(Project jsonProject) {
                     return Response.status(Response.Status.FORBIDDEN).entity("Access to the specified project is forbidden").build();
                 }
                 final String name = StringUtils.trimToNull(jsonProject.getName());
-                final String version = StringUtils.trimToNull(jsonProject.getVersion());
-                final Project tmpProject = qm.getProject(name, version);
-                if (tmpProject == null || (tmpProject.getUuid().equals(project.getUuid()))) {
-                    // Name cannot be empty or null - prevent it
-                    if (name == null) {
-                        jsonProject.setName(project.getName());
-                    }
-                    try {
-                        project = qm.updateProject(jsonProject, true);
-                    } catch (IllegalArgumentException e) {
-                        LOGGER.debug(e.getMessage());
-                        return Response.status(Response.Status.CONFLICT).entity(e.getMessage()).build();
+                // Name cannot be empty or null - prevent it
+                if (name == null) {
+                    jsonProject.setName(project.getName());
+                }
+                try {
+                    project = qm.updateProject(jsonProject, true);
+                } catch (IllegalArgumentException e) {
+                    LOGGER.debug(e.getMessage());
+                    return Response.status(Response.Status.CONFLICT).entity(e.getMessage()).build();
+                } catch (RuntimeException e) {
+                    if (isUniqueConstraintViolation(e)) {
+                        return Response.status(Response.Status.CONFLICT).entity("A project with the specified name and version already exists.").build();
                     }
-                    LOGGER.info("Project " + project.toString() + " updated by " + super.getPrincipal().getName());
-                    return Response.ok(project).build();
-                } else {
-                    return Response.status(Response.Status.CONFLICT).entity("A project with the specified name and version already exists.").build();
+
+                    LOGGER.error("Failed to update project %s".formatted(jsonProject.getUuid()), e);
+                    return Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
                 }
+                LOGGER.info("Project " + project.toString() + " updated by " + super.getPrincipal().getName());
+                return Response.ok(project).build();
             } else {
                 return Response.status(Response.Status.NOT_FOUND).entity("The UUID of the project could not be found.").build();
             }
@@ -369,10 +375,6 @@ public Response patchProject(
                 project = qm.detachWithGroups(project, List.of(FetchGroup.DEFAULT, Project.FetchGroup.PARENT.name()));
                 modified |= setIfDifferent(jsonProject, project, Project::getName, Project::setName);
                 modified |= setIfDifferent(jsonProject, project, Project::getVersion, Project::setVersion);
-                // if either name or version has been changed, verify that this new combination does not already exist
-                if (modified && qm.doesProjectExist(project.getName(), project.getVersion())) {
-                    return Response.status(Response.Status.CONFLICT).entity("A project with the specified name and version already exists.").build();
-                }
                 modified |= setIfDifferent(jsonProject, project, Project::getAuthor, Project::setAuthor);
                 modified |= setIfDifferent(jsonProject, project, Project::getPublisher, Project::setPublisher);
                 modified |= setIfDifferent(jsonProject, project, Project::getGroup, Project::setGroup);
@@ -409,6 +411,13 @@ public Response patchProject(
                     } catch (IllegalArgumentException e) {
                         LOGGER.debug(e.getMessage());
                         return Response.status(Response.Status.CONFLICT).entity(e.getMessage()).build();
+                    } catch (RuntimeException e) {
+                        if (isUniqueConstraintViolation(e)) {
+                            return Response.status(Response.Status.CONFLICT).entity("A project with the specified name and version already exists.").build();
+                        }
+
+                        LOGGER.error("Failed to patch project %s".formatted(uuid), e);
+                        return Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
                     }
                     LOGGER.info("Project " + project.toString() + " updated by " + super.getPrincipal().getName());
                     return Response.ok(project).build();

diff --git a/src/main/java/org/dependencytrack/util/PersistenceUtil.java b/src/main/java/org/dependencytrack/util/PersistenceUtil.java
@@ -85,10 +85,9 @@ public static <T, V> boolean applyIfChanged(final T existingObject, final T newO
     }
 
     public static boolean isUniqueConstraintViolation(final Throwable throwable) {
-        // TODO: DataNucleus doesn't map constraint violation exceptions very well,
-        // so we have to depend on the exception of the underlying JDBC driver to
-        // tell us what happened. We currently only handle PostgreSQL, but we'll have
-        // to do the same for at least H2 and MSSQL.
+        // NB: DataNucleus doesn't map constraint violation exceptions very well,
+        //   so we have to depend on the exception of the underlying JDBC driver to
+        //   tell us what happened.
         return ExceptionUtils.getRootCause(throwable) instanceof final SQLException se
                 && PSQLState.UNIQUE_VIOLATION.getState().equals(se.getSQLState());
     }

diff --git a/src/main/resources/migration/changelog-main.xml b/src/main/resources/migration/changelog-main.xml
@@ -10,5 +10,6 @@
             http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-latest.xsd">
     <!-- Use separate changelogs per release. -->
     <include file="migration/changelog-v5.3.0.xml"/>
+    <include file="migration/changelog-v5.4.0.xml"/>
     <include file="migration/changelog-procedures.xml"/>
 </databaseChangeLog>
diff --git a/src/main/resources/migration/changelog-v5.4.0.xml b/src/main/resources/migration/changelog-v5.4.0.xml
@@ -0,0 +1,25 @@
+<?xml version="1.1" encoding="UTF-8" standalone="no"?>
+<databaseChangeLog
+        objectQuotingStrategy="QUOTE_ALL_OBJECTS"
+        xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+        xmlns:ext="http://www.liquibase.org/xml/ns/dbchangelog-ext"
+        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+        xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog-ext
+            http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd
+            http://www.liquibase.org/xml/ns/dbchangelog
+            http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-latest.xsd">
+    <changeSet id="v5.4.0-1" author="[email protected]">
+        <!--
+            Create two partial unique indexes to cover cases where VERSION is NULL.
+            Per default, NULLs are considered to be unique values.
+            PostgreSQL 15 supports NULLS NOT DISTINCT: https://stackoverflow.com/a/8289253
+            But we are far away from being able to raise the baseline version to 15.
+        -->
+        <sql splitStatements="true">
+            CREATE UNIQUE INDEX "PROJECT_NAME_VERSION_IDX" ON "PROJECT" ("NAME", "VERSION")
+                WHERE "VERSION" IS NOT NULL;
+            CREATE UNIQUE INDEX "PROJECT_NAME_VERSION_NULL_IDX" ON "PROJECT" ("NAME")
+                WHERE "VERSION" IS NULL;
+        </sql>
+    </changeSet>
+</databaseChangeLog>
diff --git a/src/test/java/org/dependencytrack/resources/v1/ProjectResourceTest.java b/src/test/java/org/dependencytrack/resources/v1/ProjectResourceTest.java
@@ -64,6 +64,11 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.UUID;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -389,6 +394,42 @@ public void createProjectDuplicateTest() {
         Assert.assertEquals("A project with the specified name already exists.", body);
     }
 
+    @Test
+    public void createProjectDuplicateRaceConditionTest() throws Exception {
+        final ExecutorService executor = Executors.newFixedThreadPool(10);
+        final var countDownLatch = new CountDownLatch(1);
+
+        final var responses = new ArrayBlockingQueue<Response>(50);
+        for (int i = 0; i < 50; i++) {
+            executor.submit(() -> {
+                try {
+                    countDownLatch.await();
+                } catch (InterruptedException e) {
+                    throw new RuntimeException(e);
+                }
+
+                final Response response = target(V1_PROJECT)
+                        .request()
+                        .header(X_API_KEY, apiKey)
+                        .put(Entity.entity("""
+                                {
+                                  "name": "acme-app",
+                                  "version": "1.0.0"
+                                }
+                                """, MediaType.APPLICATION_JSON));
+                responses.offer(response);
+            });
+        }
+
+        countDownLatch.countDown();
+        executor.shutdown();
+        assertThat(executor.awaitTermination(15, TimeUnit.SECONDS)).isTrue();
+
+        assertThat(responses).hasSize(50);
+        assertThat(responses).satisfiesOnlyOnce(response -> assertThat(response.getStatus()).isEqualTo(201));
+        assertThat(responses.stream().map(Response::getStatus).filter(status -> status != 201)).containsOnly(409);
+    }
+
     @Test
     public void createProjectEmptyTest() {
         Project project = new Project();