From 83bb965e07d499b4a4accb22c9795a10c12b07fb Mon Sep 17 00:00:00 2001 From: nscuro Date: Mon, 13 May 2024 22:56:11 +0200 Subject: [PATCH] Fix broken global vuln audit view for MSSQL MSSQL does not support `DISTINCT` for columns of type `TEXT`, which `DESCRIPTION` and `RECOMMENDATION` are. Because the database schema is controlled by DataNucleus, and DataNucleus doesn't allow us to customize column types for specific RDBMSes, changing the respective columns to `VARCHAR(MAX)` is not possible. `DISTINCT` was needed because finding rows are joined with the `PROJECT_ACCESS_TEAMS` table, to support portfolio ACLs. If a user is member of multiple teams, the query would yield a duplicate row for each permitted team the user is a member of. The need for `DISTINCT` is eliminated by converting the ACL check from a `LEFT JOIN` to an `EXISTS` subquery. Fixes #3692 Signed-off-by: nscuro --- .../org/dependencytrack/model/Finding.java | 5 +- .../dependencytrack/model/GroupedFinding.java | 2 - .../FindingsSearchQueryManager.java | 73 ++++++++----------- .../persistence/QueryManager.java | 36 +++++++++ 4 files changed, 68 insertions(+), 48 deletions(-) diff --git a/src/main/java/org/dependencytrack/model/Finding.java b/src/main/java/org/dependencytrack/model/Finding.java index 78910d338..aa174b14f 100644 --- a/src/main/java/org/dependencytrack/model/Finding.java +++ b/src/main/java/org/dependencytrack/model/Finding.java @@ -100,8 +100,7 @@ public class Finding implements Serializable { // language=SQL public static final String QUERY_ALL_FINDINGS = """ - SELECT DISTINCT - "COMPONENT"."UUID" + SELECT "COMPONENT"."UUID" , "COMPONENT"."NAME" , "COMPONENT"."GROUP" , "COMPONENT"."VERSION" @@ -147,8 +146,6 @@ public class Finding implements Serializable { AND "COMPONENT"."PROJECT_ID" = "ANALYSIS"."PROJECT_ID" INNER JOIN "PROJECT" ON "COMPONENT"."PROJECT_ID" = "PROJECT"."ID" - LEFT JOIN "PROJECT_ACCESS_TEAMS" - ON "PROJECT"."ID" = "PROJECT_ACCESS_TEAMS"."PROJECT_ID" """; private final UUID project; diff --git a/src/main/java/org/dependencytrack/model/GroupedFinding.java b/src/main/java/org/dependencytrack/model/GroupedFinding.java index c4a1fb4fa..d25a82106 100644 --- a/src/main/java/org/dependencytrack/model/GroupedFinding.java +++ b/src/main/java/org/dependencytrack/model/GroupedFinding.java @@ -68,8 +68,6 @@ public class GroupedFinding implements Serializable { AND "COMPONENT"."PROJECT_ID" = "ANALYSIS"."PROJECT_ID" INNER JOIN "PROJECT" ON "COMPONENT"."PROJECT_ID" = "PROJECT"."ID" - LEFT JOIN "PROJECT_ACCESS_TEAMS" - ON "PROJECT"."ID" = "PROJECT_ACCESS_TEAMS"."PROJECT_ID" """; private final Map vulnerability = new LinkedHashMap<>(); diff --git a/src/main/java/org/dependencytrack/persistence/FindingsSearchQueryManager.java b/src/main/java/org/dependencytrack/persistence/FindingsSearchQueryManager.java index 3732200cf..06ae79c2d 100644 --- a/src/main/java/org/dependencytrack/persistence/FindingsSearchQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/FindingsSearchQueryManager.java @@ -18,9 +18,6 @@ */ package org.dependencytrack.persistence; -import alpine.model.ApiKey; -import alpine.model.Team; -import alpine.model.UserPrincipal; import alpine.persistence.OrderDirection; import alpine.persistence.PaginatedResult; import alpine.resources.AlpineRequest; @@ -39,7 +36,6 @@ import javax.jdo.PersistenceManager; import javax.jdo.Query; import java.util.ArrayList; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -347,44 +343,37 @@ private void processInputFilter(StringBuilder queryFilter, Map p } private void preprocessACLs(StringBuilder queryFilter, final Map params) { - if (super.principal != null && isEnabled(ConfigPropertyConstants.ACCESS_MANAGEMENT_ACL_ENABLED)) { - final List teams; - if (super.principal instanceof UserPrincipal userPrincipal) { - teams = userPrincipal.getTeams(); - if (super.hasAccessManagementPermission(userPrincipal)) { - return; - } - } else if (super.principal instanceof final ApiKey apiKey) { - teams = apiKey.getTeams(); - if (super.hasAccessManagementPermission(apiKey)) { - return; - } - } else { - teams = Collections.emptyList(); - } - if (teams != null && !teams.isEmpty()) { - final StringBuilder sb = new StringBuilder(); - for (int i = 0, teamsSize = teams.size(); i < teamsSize; i++) { - final Team team = super.getObjectById(Team.class, teams.get(i).getId()); - sb.append(" \"PROJECT_ACCESS_TEAMS\".\"TEAM_ID\" = :team").append(i); - params.put("team" + i, team.getId()); - if (i < teamsSize - 1) { - sb.append(" OR "); - } - } - if (queryFilter != null && !queryFilter.isEmpty()) { - queryFilter.append(" AND (").append(sb).append(")"); - } else if (queryFilter != null) { - queryFilter.append("WHERE (").append(sb).append(")"); - } - } else { - params.put("false", false); - if (queryFilter != null && !queryFilter.isEmpty()) { - queryFilter.append(" AND :false"); - } else if (queryFilter != null) { - queryFilter.append("WHERE :false"); - } - } + if (!isEnabled(ConfigPropertyConstants.ACCESS_MANAGEMENT_ACL_ENABLED) + || hasAccessManagementPermission(this.principal)) { + return; + } + + if (queryFilter.isEmpty()) { + queryFilter.append(" WHERE "); + } else { + queryFilter.append(" AND "); + } + + final var teamIds = new ArrayList<>(getTeamIds(principal)); + if (teamIds.isEmpty()) { + queryFilter.append(":false"); + params.put("false", false); + return; + } + + // NB: Need to work around the fact that the RDBMSes can't agree on how to do member checks. Oh joy! :))) + final var teamIdChecks = new ArrayList(); + for (int i = 0; i < teamIds.size(); i++) { + teamIdChecks.add("\"PROJECT_ACCESS_TEAMS\".\"TEAM_ID\" = :teamId" + i); + params.put("teamId" + i, teamIds.get(i)); } + + queryFilter.append(""" + EXISTS ( + SELECT 1 + FROM "PROJECT_ACCESS_TEAMS" + WHERE "PROJECT_ACCESS_TEAMS"."PROJECT_ID" = "PROJECT"."ID" + AND (%s) + )""".formatted(String.join(" OR ", teamIdChecks))); } } diff --git a/src/main/java/org/dependencytrack/persistence/QueryManager.java b/src/main/java/org/dependencytrack/persistence/QueryManager.java index dec2ff9bd..81b9fe1c8 100644 --- a/src/main/java/org/dependencytrack/persistence/QueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/QueryManager.java @@ -31,6 +31,7 @@ import alpine.resources.AlpineRequest; import com.github.packageurl.PackageURL; import com.google.common.collect.Lists; +import org.apache.commons.lang3.ClassUtils; import org.datanucleus.PropertyNames; import org.datanucleus.api.jdo.JDOQuery; import org.dependencytrack.event.IndexEvent; @@ -92,6 +93,7 @@ import java.time.ZonedDateTime; import java.util.ArrayList; import java.util.Date; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -355,6 +357,30 @@ public QueryManager withL2CacheDisabled() { return this; } + /** + * Get the IDs of the {@link Team}s a given {@link Principal} is a member of. + * + * @return A {@link Set} of {@link Team} IDs + * @since 4.11.1 + */ + protected Set getTeamIds(final Principal principal) { + final var principalTeamIds = new HashSet(); + + if (principal instanceof final UserPrincipal userPrincipal + && userPrincipal.getTeams() != null) { + for (final Team userInTeam : userPrincipal.getTeams()) { + principalTeamIds.add(userInTeam.getId()); + } + } else if (principal instanceof final ApiKey apiKey + && apiKey.getTeams() != null) { + for (final Team userInTeam : apiKey.getTeams()) { + principalTeamIds.add(userInTeam.getId()); + } + } + + return principalTeamIds; + } + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// //// BEGIN WRAPPER METHODS //// //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -1321,6 +1347,16 @@ public void commitSearchIndex(Class clazz) { commitSearchIndex(true, clazz); } + public boolean hasAccessManagementPermission(final Object principal) { + if (principal instanceof final UserPrincipal userPrincipal) { + return hasAccessManagementPermission(userPrincipal); + } else if (principal instanceof final ApiKey apiKey) { + return hasAccessManagementPermission(apiKey); + } + + throw new IllegalArgumentException("Provided principal is of invalid type " + ClassUtils.getName(principal)); + } + public boolean hasAccessManagementPermission(final UserPrincipal userPrincipal) { return getProjectQueryManager().hasAccessManagementPermission(userPrincipal); }