Skip to content

Commit

Permalink
Fix broken global vuln audit view for MSSQL
Browse files Browse the repository at this point in the history 
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 DependencyTrack#3692

Signed-off-by: nscuro <nscuro@protonmail.com>
  • Loading branch information
@nscuro @MM-msr
nscuro authored and MM-msr committed Jun 18, 2024
1 parent 466c566 commit 83bb965
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 48 deletions.
5 changes: 1 addition & 4 deletions src/main/java/org/dependencytrack/model/Finding.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 0 additions & 2 deletions src/main/java/org/dependencytrack/model/GroupedFinding.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, Object> vulnerability = new LinkedHashMap<>();
Expand Down
73 changes: 31 additions & 42 deletions src/main/java/org/dependencytrack/persistence/FindingsSearchQueryManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -347,44 +343,37 @@ private void processInputFilter(StringBuilder queryFilter, Map<String, Object> p
}

private void preprocessACLs(StringBuilder queryFilter, final Map<String, Object> params) {
if (super.principal != null && isEnabled(ConfigPropertyConstants.ACCESS_MANAGEMENT_ACL_ENABLED)) {
final List<Team> 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<String>();
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)));
}
}
36 changes: 36 additions & 0 deletions src/main/java/org/dependencytrack/persistence/QueryManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<Long> getTeamIds(final Principal principal) {
final var principalTeamIds = new HashSet<Long>();

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 ////
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -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);
}
Expand Down

0 comments on commit 83bb965

Please sign in to comment.