From 0be494a2111a75ea80e18bf8fe27000cdcb9163d Mon Sep 17 00:00:00 2001 From: Phillip Whittlesea Date: Mon, 12 Jun 2017 01:48:33 +0100 Subject: [PATCH] Issue #730: Remove redundant method from Settings Pull file loading from loadSuppressionData() to make it easier to read Add test assertion to happy case Ant task test --- .../taskdefs/DependencyCheckTaskTest.java | 2 + .../analyzer/AbstractSuppressionAnalyzer.java | 116 ++++++++++-------- .../owasp/dependencycheck/utils/Settings.java | 14 +-- .../dependencycheck/utils/SettingsTest.java | 15 --- 4 files changed, 67 insertions(+), 80 deletions(-) diff --git a/dependency-check-ant/src/test/java/org/owasp/dependencycheck/taskdefs/DependencyCheckTaskTest.java b/dependency-check-ant/src/test/java/org/owasp/dependencycheck/taskdefs/DependencyCheckTaskTest.java index dfb4c86e5..e1c41c9e8 100644 --- a/dependency-check-ant/src/test/java/org/owasp/dependencycheck/taskdefs/DependencyCheckTaskTest.java +++ b/dependency-check-ant/src/test/java/org/owasp/dependencycheck/taskdefs/DependencyCheckTaskTest.java @@ -127,6 +127,8 @@ public class DependencyCheckTaskTest { buildFileRule.executeTarget(antTaskName); // THEN the ant task executed without error + final File report = new File("target/dependency-check-report.html"); + assertTrue("Expected the DependencyCheck report to be generated", report.exists()); } /** diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java index f2f35a837..b8f41da5c 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java @@ -99,13 +99,12 @@ public abstract class AbstractSuppressionAnalyzer extends AbstractAnalyzer { } /** - * Loads the suppression rules file. + * Loads all the suppression rules files configured in the {@link Settings} singleton. * * @throws SuppressionParseException thrown if the XML cannot be parsed. */ private void loadSuppressionData() throws SuppressionParseException { final SuppressionParser parser = new SuppressionParser(); - File file = null; try { final InputStream in = this.getClass().getClassLoader().getResourceAsStream("dependencycheck-base-suppression.xml"); rules = parser.parseSuppressionRules(in); @@ -117,67 +116,80 @@ public abstract class AbstractSuppressionAnalyzer extends AbstractAnalyzer { return; } + // Load all the suppression file paths for (final String suppressionFilePath : suppressionFilePaths) { - LOGGER.debug("Loading suppression rules from '{}'", suppressionFilePath); + loadSuppressionFile(parser, suppressionFilePath); + } + LOGGER.debug("{} suppression rules were loaded.", rules.size()); + } - boolean deleteTempFile = false; - try { - final Pattern uriRx = Pattern.compile("^(https?|file)\\:.*", Pattern.CASE_INSENSITIVE); - if (uriRx.matcher(suppressionFilePath).matches()) { - deleteTempFile = true; - file = FileUtils.getTempFile("suppression", "xml"); - final URL url = new URL(suppressionFilePath); - try { - Downloader.fetchFile(url, file, false); - } catch (DownloadFailedException ex) { - Downloader.fetchFile(url, file, true); - } - } else { - file = new File(suppressionFilePath); + /** + * Load a single suppression rules file from the path provided using the parser provided. + * + * @param parser the parser to use for loading the file. + * @param suppressionFilePath the path to load. + * @throws SuppressionParseException thrown if the suppression file cannot be loaded and parsed. + */ + private void loadSuppressionFile(final SuppressionParser parser, final String suppressionFilePath) throws SuppressionParseException { + LOGGER.debug("Loading suppression rules from '{}'", suppressionFilePath); - if (!file.exists()) { - try (InputStream suppressionsFromClasspath = this.getClass().getClassLoader().getResourceAsStream(suppressionFilePath)) { - if (suppressionsFromClasspath != null) { - deleteTempFile = true; - file = FileUtils.getTempFile("suppression", "xml"); - try { - org.apache.commons.io.FileUtils.copyInputStreamToFile(suppressionsFromClasspath, file); - } catch (IOException ex) { - throwSuppressionParseException("Unable to locate suppressions file in classpath", ex); - } + File file = null; + boolean deleteTempFile = false; + try { + final Pattern uriRx = Pattern.compile("^(https?|file)\\:.*", Pattern.CASE_INSENSITIVE); + if (uriRx.matcher(suppressionFilePath).matches()) { + deleteTempFile = true; + file = FileUtils.getTempFile("suppression", "xml"); + final URL url = new URL(suppressionFilePath); + try { + Downloader.fetchFile(url, file, false); + } catch (DownloadFailedException ex) { + Downloader.fetchFile(url, file, true); + } + } else { + file = new File(suppressionFilePath); + + if (!file.exists()) { + try (InputStream suppressionsFromClasspath = this.getClass().getClassLoader().getResourceAsStream(suppressionFilePath)) { + if (suppressionsFromClasspath != null) { + deleteTempFile = true; + file = FileUtils.getTempFile("suppression", "xml"); + try { + org.apache.commons.io.FileUtils.copyInputStreamToFile(suppressionsFromClasspath, file); + } catch (IOException ex) { + throwSuppressionParseException("Unable to locate suppressions file in classpath", ex); } } } } - if (file != null) { - if (!file.exists()) { - final String msg = String.format("Suppression file '%s' does not exists", file.getPath()); - LOGGER.warn(msg); - throw new SuppressionParseException(msg); - } - try { - rules.addAll(parser.parseSuppressionRules(file)); - } catch (SuppressionParseException ex) { - LOGGER.warn("Unable to parse suppression xml file '{}'", file.getPath()); - LOGGER.warn(ex.getMessage()); - throw ex; - } + } + if (file != null) { + if (!file.exists()) { + final String msg = String.format("Suppression file '%s' does not exists", file.getPath()); + LOGGER.warn(msg); + throw new SuppressionParseException(msg); } - } catch (DownloadFailedException ex) { - throwSuppressionParseException("Unable to fetch the configured suppression file", ex); - } catch (MalformedURLException ex) { - throwSuppressionParseException("Configured suppression file has an invalid URL", ex); - } catch (SuppressionParseException ex) { - throw ex; - } catch (IOException ex) { - throwSuppressionParseException("Unable to create temp file for suppressions", ex); - } finally { - if (deleteTempFile && file != null) { - FileUtils.delete(file); + try { + rules.addAll(parser.parseSuppressionRules(file)); + } catch (SuppressionParseException ex) { + LOGGER.warn("Unable to parse suppression xml file '{}'", file.getPath()); + LOGGER.warn(ex.getMessage()); + throw ex; } } + } catch (DownloadFailedException ex) { + throwSuppressionParseException("Unable to fetch the configured suppression file", ex); + } catch (MalformedURLException ex) { + throwSuppressionParseException("Configured suppression file has an invalid URL", ex); + } catch (SuppressionParseException ex) { + throw ex; + } catch (IOException ex) { + throwSuppressionParseException("Unable to create temp file for suppressions", ex); + } finally { + if (deleteTempFile && file != null) { + FileUtils.delete(file); + } } - LOGGER.debug("{} suppression rules were loaded.", rules.size()); } /** diff --git a/dependency-check-utils/src/main/java/org/owasp/dependencycheck/utils/Settings.java b/dependency-check-utils/src/main/java/org/owasp/dependencycheck/utils/Settings.java index 32aa3147b..a24971df0 100644 --- a/dependency-check-utils/src/main/java/org/owasp/dependencycheck/utils/Settings.java +++ b/dependency-check-utils/src/main/java/org/owasp/dependencycheck/utils/Settings.java @@ -564,18 +564,6 @@ public final class Settings { LOGGER.debug("Setting: {}='{}'", key, value); } - /** - * Sets a property value from an array. - *

- * Note: each value of the array will be joined by the delimiter {@link Settings#ARRAY_SEP}. - * - * @param key the key for the property - * @param value the value for the property - */ - static void setArray(String key, String[] value) { - setString(key, StringUtils.join(value, ARRAY_SEP)); - } - /** * Sets a property value only if the value is not null. * @@ -608,7 +596,7 @@ public final class Settings { */ public static void setArrayIfNotEmpty(String key, String[] value) { if (null != value && value.length > 0) { - setArray(key, value); + setString(key, StringUtils.join(value, ARRAY_SEP)); } } diff --git a/dependency-check-utils/src/test/java/org/owasp/dependencycheck/utils/SettingsTest.java b/dependency-check-utils/src/test/java/org/owasp/dependencycheck/utils/SettingsTest.java index 881be4138..b91333f3e 100644 --- a/dependency-check-utils/src/test/java/org/owasp/dependencycheck/utils/SettingsTest.java +++ b/dependency-check-utils/src/test/java/org/owasp/dependencycheck/utils/SettingsTest.java @@ -336,19 +336,4 @@ public class SettingsTest extends BaseTest { assertThat("Expected the property to be set", Settings.getString("key"), is("value1")); } - /** - * Assert {@link Settings#setArray(String, String[])} with multiple values sets a delimited string. - */ - @Test - public void testSetArraySetsADelimitedString() { - // GIVEN an array with values - final String[] array = { "value1", "value2" }; - - // WHEN setting the array - Settings.setArray("key", array); - - // THEN the property is set - assertThat("Expected the property to be set", Settings.getString("key"), is("value1,value2")); - } - }