Merge pull request #1093 from DependencyTrack/issue-1713

Prevent duplicates in `PROJECT_ACCESS_TEAMS` table

Author: nscuro

src/main/java/org/dependencytrack/model/Project.java

@@ -35,14 +35,14 @@
 import com.github.packageurl.MalformedPackageURLException;
 import com.github.packageurl.PackageURL;
 import io.swagger.v3.oas.annotations.media.Schema;
-import jakarta.validation.constraints.NotBlank;
-import jakarta.validation.constraints.NotNull;
-import jakarta.validation.constraints.Pattern;
-import jakarta.validation.constraints.Size;
 import org.dependencytrack.persistence.converter.OrganizationalContactsJsonConverter;
 import org.dependencytrack.persistence.converter.OrganizationalEntityJsonConverter;
 import org.dependencytrack.resources.v1.serializers.CustomPackageURLSerializer;
 
+import jakarta.validation.constraints.NotBlank;
+import jakarta.validation.constraints.NotNull;
+import jakarta.validation.constraints.Pattern;
+import jakarta.validation.constraints.Size;
 import javax.jdo.annotations.Column;
 import javax.jdo.annotations.Convert;
 import javax.jdo.annotations.Element;
@@ -61,10 +61,11 @@
 import javax.jdo.annotations.Unique;
 import java.io.IOException;
 import java.io.Serializable;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Date;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.UUID;
 
 /**
@@ -298,10 +299,9 @@ public enum FetchGroup {
     private Date inactiveSince;
 
     @Persistent(table = "PROJECT_ACCESS_TEAMS")
-    @Join(column = "PROJECT_ID")
+    @Join(column = "PROJECT_ID", primaryKey = "PROJECT_ACCESS_TEAMS_PK")
     @Element(column = "TEAM_ID")
-    @Order(extensions = @Extension(vendorName = "datanucleus", key = "list-ordering", value = "name ASC"))
-    private List<Team> accessTeams;
+    private Set<Team> accessTeams;
 
     @Persistent(defaultFetchGroup = "true")
     @Column(name = "EXTERNAL_REFERENCES")
@@ -570,20 +570,28 @@ public void setVersions(List<ProjectVersion> versions) {
     }
 
     @JsonIgnore
-    public List<Team> getAccessTeams() {
+    public Set<Team> getAccessTeams() {
         return accessTeams;
     }
 
     @JsonSetter
-    public void setAccessTeams(List<Team> accessTeams) {
+    public void setAccessTeams(Set<Team> accessTeams) {
         this.accessTeams = accessTeams;
     }
 
-    public void addAccessTeam(Team accessTeam) {
+    public boolean addAccessTeam(Team accessTeam) {
         if (this.accessTeams == null) {
-            this.accessTeams = new ArrayList<>();
+            this.accessTeams = new HashSet<>();
         }
-        this.accessTeams.add(accessTeam);
+        return this.accessTeams.add(accessTeam);
+    }
+
+    public boolean removeAccessTeam(Team accessTeam) {
+        if (this.accessTeams == null) {
+            return false;
+        }
+
+        return this.accessTeams.remove(accessTeam);
     }
 
     public ProjectMetadata getMetadata() {

src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java

@@ -803,9 +803,9 @@ public Project clone(
             }
 
             if (includeACL) {
-                List<Team> accessTeams = source.getAccessTeams();
+                Set<Team> accessTeams = source.getAccessTeams();
                 if (!CollectionUtils.isEmpty(accessTeams)) {
-                    project.setAccessTeams(new ArrayList<>(accessTeams));
+                    project.setAccessTeams(new HashSet<>(accessTeams));
                 }
             }
 

src/main/java/org/dependencytrack/resources/v1/AccessControlResource.java

@@ -34,7 +34,16 @@
 import io.swagger.v3.oas.annotations.security.SecurityRequirement;
 import io.swagger.v3.oas.annotations.security.SecurityRequirements;
 import io.swagger.v3.oas.annotations.tags.Tag;
+import org.dependencytrack.auth.Permissions;
+import org.dependencytrack.model.Project;
+import org.dependencytrack.model.validation.ValidUuid;
+import org.dependencytrack.persistence.QueryManager;
+import org.dependencytrack.resources.v1.openapi.PaginatedApi;
+import org.dependencytrack.resources.v1.problems.ProblemDetails;
+import org.dependencytrack.resources.v1.vo.AclMappingRequest;
+
 import jakarta.validation.Validator;
+import jakarta.ws.rs.ClientErrorException;
 import jakarta.ws.rs.DELETE;
 import jakarta.ws.rs.GET;
 import jakarta.ws.rs.PUT;
@@ -44,15 +53,7 @@
 import jakarta.ws.rs.QueryParam;
 import jakarta.ws.rs.core.MediaType;
 import jakarta.ws.rs.core.Response;
-import org.dependencytrack.auth.Permissions;
-import org.dependencytrack.model.Project;
-import org.dependencytrack.model.validation.ValidUuid;
-import org.dependencytrack.persistence.QueryManager;
-import org.dependencytrack.resources.v1.openapi.PaginatedApi;
-import org.dependencytrack.resources.v1.vo.AclMappingRequest;
-
-import java.util.ArrayList;
-import java.util.List;
+import java.util.NoSuchElementException;
 
 /**
  * JAX-RS resources for processing LDAP group mapping requests.
@@ -114,10 +115,19 @@ public Response retrieveProjects(@Parameter(description = "The UUID of the team
             description = "<p>Requires permission <strong>ACCESS_MANAGEMENT</strong></p>"
     )
     @ApiResponses(value = {
-            @ApiResponse(responseCode = "200", description = "Mapping created successfully", content = @Content(schema = @Schema(implementation = AclMappingRequest.class))),
+            @ApiResponse(
+                    responseCode = "200",
+                    description = "Mapping created successfully",
+                    content = @Content(schema = @Schema(implementation = AclMappingRequest.class))),
             @ApiResponse(responseCode = "401", description = "Unauthorized"),
-            @ApiResponse(responseCode = "404", description = "The UUID of the team or project could not be found"),
-            @ApiResponse(responseCode = "409", description = "A mapping with the same team and project already exists")
+            @ApiResponse(
+                    responseCode = "404",
+                    description = "Team or project could not be found",
+                    content = @Content(schema = @Schema(implementation = ProblemDetails.class), mediaType = ProblemDetails.MEDIA_TYPE_JSON)),
+            @ApiResponse(
+                    responseCode = "409",
+                    description = "A mapping with the same team and project already exists",
+                    content = @Content(schema = @Schema(implementation = ProblemDetails.class), mediaType = ProblemDetails.MEDIA_TYPE_JSON))
     })
     @PermissionRequired({Permissions.Constants.ACCESS_MANAGEMENT, Permissions.Constants.ACCESS_MANAGEMENT_CREATE})
     public Response addMapping(AclMappingRequest request) {
@@ -126,21 +136,31 @@ public Response addMapping(AclMappingRequest request) {
                 validator.validateProperty(request, "team"),
                 validator.validateProperty(request, "project")
         );
-        try (QueryManager qm = new QueryManager()) {
-            final Team team = qm.getObjectByUuid(Team.class, request.getTeam());
-            final Project project = qm.getObjectByUuid(Project.class, request.getProject());
-            if (team != null && project != null) {
-                for (final Team t : project.getAccessTeams()) {
-                    if (t.getUuid() == team.getUuid()) {
-                        return Response.status(Response.Status.CONFLICT).entity("A mapping with the same team and project already exists.").build();
-                    }
+        try (final var qm = new QueryManager()) {
+            qm.runInTransaction(() -> {
+                final Team team = qm.getObjectByUuid(Team.class, request.getTeam());
+                if (team == null) {
+                    throw new NoSuchElementException("Team could not be found");
                 }
-                project.addAccessTeam(team);
-                qm.persist(project);
-                return Response.ok().build();
-            } else {
-                return Response.status(Response.Status.NOT_FOUND).entity("The UUID of the team could not be found.").build();
-            }
+
+                final Project project = qm.getObjectByUuid(Project.class, request.getProject());
+                if (project == null) {
+                    throw new NoSuchElementException("Project could not be found");
+                }
+
+                // TODO: The conflict error is legacy behavior, but wouldn't it make more
+                //  sense to return a 304 - Not Modified instead?
+                final boolean added = project.addAccessTeam(team);
+                if (!added) {
+                    final var problemDetails = new ProblemDetails();
+                    problemDetails.setStatus(409);
+                    problemDetails.setTitle("Conflict");
+                    problemDetails.setDetail("A mapping with the same team and project already exists");
+                    throw new ClientErrorException(problemDetails.toResponse());
+                }
+            });
+
+            return Response.ok().build();
         }
     }
 
@@ -154,30 +174,34 @@ public Response addMapping(AclMappingRequest request) {
     @ApiResponses(value = {
             @ApiResponse(responseCode = "200", description = "Mapping removed successfully"),
             @ApiResponse(responseCode = "401", description = "Unauthorized"),
-            @ApiResponse(responseCode = "404", description = "The UUID of the team or project could not be found"),
+            @ApiResponse(
+                    responseCode = "404",
+                    description = "Team or project could not be found",
+                    content = @Content(schema = @Schema(implementation = ProblemDetails.class), mediaType = ProblemDetails.MEDIA_TYPE_JSON))
     })
     @PermissionRequired({Permissions.Constants.ACCESS_MANAGEMENT, Permissions.Constants.ACCESS_MANAGEMENT_DELETE})
     public Response deleteMapping(
             @Parameter(description = "The UUID of the team to delete the mapping for", schema = @Schema(type = "string", format = "uuid"), required = true)
             @PathParam("teamUuid") @ValidUuid String teamUuid,
             @Parameter(description = "The UUID of the project to delete the mapping for", schema = @Schema(type = "string", format = "uuid"), required = true)
             @PathParam("projectUuid") @ValidUuid String projectUuid) {
-        try (QueryManager qm = new QueryManager()) {
-            final Team team = qm.getObjectByUuid(Team.class, teamUuid);
-            final Project project = qm.getObjectByUuid(Project.class, projectUuid);
-            if (team != null && project != null) {
-                final List<Team> teams = new ArrayList<>();
-                for (final Team t : project.getAccessTeams()) {
-                    if (t.getUuid() != team.getUuid()) {
-                        teams.add(t);
-                    }
+        try (final var qm = new QueryManager()) {
+            qm.runInTransaction(() -> {
+                final Team team = qm.getObjectByUuid(Team.class, teamUuid);
+                if (team == null) {
+                    throw new NoSuchElementException("Team could not be found");
                 }
-                project.setAccessTeams(teams);
-                qm.persist(project);
-                return Response.ok().build();
-            } else {
-                return Response.status(Response.Status.NOT_FOUND).entity("The UUID of the team or project could not be found.").build();
-            }
+
+                final Project project = qm.getObjectByUuid(Project.class, projectUuid);
+                if (project == null) {
+                    throw new NoSuchElementException("Project could not be found");
+                }
+
+                project.removeAccessTeam(team);
+            });
         }
+
+        return Response.ok().build();
     }
+
 }

src/main/java/org/dependencytrack/resources/v1/ProjectResource.java

@@ -476,8 +476,8 @@ public Response createProject(final Project jsonProject) {
 
                 Principal principal = getPrincipal();
 
-                final List<Team> chosenTeams = requireNonNullElseGet(
-                        jsonProject.getAccessTeams(), Collections::emptyList);
+                final Set<Team> chosenTeams = requireNonNullElseGet(
+                        jsonProject.getAccessTeams(), Collections::emptySet);
                 jsonProject.setAccessTeams(null);
 
                 for (final Team chosenTeam : chosenTeams) {
@@ -486,8 +486,8 @@ public Response createProject(final Project jsonProject) {
                                 .status(Response.Status.BAD_REQUEST)
                                 .entity("""
                                         accessTeams must either specify a UUID or a name,\
-                                        but the team at index %d has neither.\
-                                        """.formatted(chosenTeams.indexOf(chosenTeam)))
+                                        but the team %s has neither.\
+                                        """.formatted(chosenTeam))
                                 .build());
                     }
                 }
@@ -555,7 +555,6 @@ public Response createProject(final Project jsonProject) {
                     LOGGER.error("Failed to create project %s".formatted(jsonProject), e);
                     throw new ServerErrorException(Response.Status.INTERNAL_SERVER_ERROR);
                 }
-                qm.updateNewProjectACL(project, principal);
                 return project;
             });
 

src/main/resources/migration/changelog-v5.6.0.xml

@@ -674,4 +674,32 @@
             <column name="COMPONENT_ID"/>
         </createIndex>
     </changeSet>
+
+    <changeSet id="v5.6.0-14" author="nscuro">
+        <dropIndex tableName="PROJECT_ACCESS_TEAMS" indexName="PROJECT_ACCESS_TEAMS_PROJECT_ID_IDX"/>
+        <dropIndex tableName="PROJECT_ACCESS_TEAMS" indexName="PROJECT_ACCESS_TEAMS_TEAM_ID_IDX"/>
+        <sql>
+            WITH
+            cte_duplicate AS (
+               SELECT "PROJECT_ID"
+                    , "TEAM_ID"
+                 FROM "PROJECT_ACCESS_TEAMS"
+                GROUP BY "PROJECT_ID"
+                       , "TEAM_ID"
+               HAVING COUNT(*) > 1
+            ),
+            cte_deleted_duplicate AS (
+              DELETE FROM "PROJECT_ACCESS_TEAMS"
+               USING cte_duplicate
+               WHERE cte_duplicate."PROJECT_ID" = "PROJECT_ACCESS_TEAMS"."PROJECT_ID"
+                 AND cte_duplicate."TEAM_ID" = "PROJECT_ACCESS_TEAMS"."TEAM_ID"
+            )
+            INSERT INTO "PROJECT_ACCESS_TEAMS" ("PROJECT_ID", "TEAM_ID")
+            SELECT "PROJECT_ID", "TEAM_ID" FROM cte_duplicate
+        </sql>
+        <addPrimaryKey
+                tableName="PROJECT_ACCESS_TEAMS"
+                columnNames="PROJECT_ID, TEAM_ID"
+                constraintName="PROJECT_ACCESS_TEAMS_PK"/>
+    </changeSet>
 </databaseChangeLog>

src/test/java/org/dependencytrack/ResourceTest.java

@@ -48,6 +48,7 @@
 
 public abstract class ResourceTest {
 
+    protected final String V1_ACL = "/v1/acl";
     protected final String V1_ANALYSIS = "/v1/analysis";
     protected final String V1_BADGE = "/v1/badge";
     protected final String V1_BOM = "/v1/bom";

src/test/java/org/dependencytrack/persistence/datanucleus/method/ProjectIsAccessibleByMethodTest.java

@@ -28,6 +28,7 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
@@ -47,7 +48,7 @@ public void shouldEvaluateToTrueWhenProjectIsAccessible() {
 
         final var project = new Project();
         project.setName("acme-app");
-        project.setAccessTeams(List.of(teamA, teamB));
+        project.setAccessTeams(Set.of(teamA, teamB));
         qm.persist(project);
 
         final Query<Project> query = qm.getPersistenceManager().newQuery(Project.class);
@@ -67,7 +68,7 @@ public void shouldEvaluateToTrueWhenProjectParentIsAccessible() {
 
         final var parentProject = new Project();
         parentProject.setName("acme-app-parent");
-        parentProject.setAccessTeams(List.of(team));
+        parentProject.setAccessTeams(Set.of(team));
         qm.persist(parentProject);
 
         final var project = new Project();
@@ -92,7 +93,7 @@ public void shouldEvaluateToTrueWhenProjectGrandParentIsAccessible() {
 
         final var grandParentProject = new Project();
         grandParentProject.setName("acme-app-grand-parent");
-        grandParentProject.setAccessTeams(List.of(team));
+        grandParentProject.setAccessTeams(Set.of(team));
         qm.persist(grandParentProject);
 
         final var parentProject = new Project();
@@ -126,7 +127,7 @@ public void shouldEvaluateToFalseWhenProjectIsNotAccessible() {
 
         final var project = new Project();
         project.setName("acme-app");
-        project.setAccessTeams(List.of(teamA));
+        project.setAccessTeams(Set.of(teamA));
         qm.persist(project);
 
         final Query<Project> query = qm.getPersistenceManager().newQuery(Project.class);
@@ -151,7 +152,7 @@ public void shouldEvaluateToFalseWhenOnlyChildProjectIsAccessible() {
         final var project = new Project();
         project.setParent(parentProject);
         project.setName("acme-app");
-        project.setAccessTeams(List.of(team));
+        project.setAccessTeams(Set.of(team));
         qm.persist(project);
 
         final Query<Project> query = qm.getPersistenceManager().newQuery(Project.class);
@@ -171,7 +172,7 @@ public void shouldBeAllowedOnProjectMembersOfNonProjectObjects() {
 
         final var project = new Project();
         project.setName("acme-app");
-        project.setAccessTeams(List.of(team));
+        project.setAccessTeams(Set.of(team));
         qm.persist(project);
 
         final var component = new Component();