From 6bcc47d3ec891ce5db40506690372dbdecd89d88 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Tue, 8 Dec 2020 11:08:46 +0100 Subject: [PATCH 01/11] Stop cleaning the plugin directory by default, add an opt-in flag for doing so --- .../tools/pluginmanager/cli/CliOptions.java | 10 +++++ .../jenkins/tools/pluginmanager/cli/Main.java | 6 --- .../tools/pluginmanager/config/Config.java | 14 ++++++ .../pluginmanager/impl/PluginManager.java | 45 ++++++++++++------- .../impl/PluginManagerIntegrationTest.java | 27 +++++++++++ .../pluginmanager/impl/PluginManagerTest.java | 2 +- 6 files changed, 82 insertions(+), 22 deletions(-) diff --git a/plugin-management-cli/src/main/java/io/jenkins/tools/pluginmanager/cli/CliOptions.java b/plugin-management-cli/src/main/java/io/jenkins/tools/pluginmanager/cli/CliOptions.java index 76df514d..db204c12 100644 --- a/plugin-management-cli/src/main/java/io/jenkins/tools/pluginmanager/cli/CliOptions.java +++ b/plugin-management-cli/src/main/java/io/jenkins/tools/pluginmanager/cli/CliOptions.java @@ -39,6 +39,11 @@ class CliOptions { handler = FileOptionHandler.class) private File pluginDir; + @Option(name = "--clean-download-directory", + usage = "If sets, cleans the plugin download directory before plugin installation. " + + "Otherwise the tool performs plugin download and reports compatibility issues, if any.") + private boolean cleanPluginDir; + @Option(name = "--plugins", aliases = {"-p"}, usage = "List of plugins to install, separated by a space", handler = StringArrayOptionHandler.class) private String[] plugins = new String[0]; @@ -144,6 +149,7 @@ Config setup() { return Config.builder() .withPlugins(getPlugins()) .withPluginDir(getPluginDir()) + .withCleanPluginsDir(isCleanPluginDir()) .withJenkinsUc(getUpdateCenter()) .withJenkinsUcExperimental(getExperimentalUpdateCenter()) .withJenkinsIncrementalsRepoMirror(getIncrementalsMirror()) @@ -214,6 +220,10 @@ private File getPluginDir() { return new File(Settings.DEFAULT_PLUGIN_DIR_LOCATION); } + public boolean isCleanPluginDir() { + return cleanPluginDir; + } + @CheckForNull private VersionNumber getJenkinsVersion() { if (jenkinsVersion != null) { diff --git a/plugin-management-cli/src/main/java/io/jenkins/tools/pluginmanager/cli/Main.java b/plugin-management-cli/src/main/java/io/jenkins/tools/pluginmanager/cli/Main.java index c9edf997..2df54d18 100644 --- a/plugin-management-cli/src/main/java/io/jenkins/tools/pluginmanager/cli/Main.java +++ b/plugin-management-cli/src/main/java/io/jenkins/tools/pluginmanager/cli/Main.java @@ -67,12 +67,6 @@ public static void main(String[] args) throws IOException { return; } - if (cfg.isShowPluginsToBeDownloaded()) { - System.out.println("The --list flag is currently unsafe and is temporarily disabled, " + - "see https://github.com/jenkinsci/plugin-installation-manager-tool/issues/173"); - return; - } - pm.start(); } catch (Exception e) { if (options.isVerbose()) { diff --git a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/config/Config.java b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/config/Config.java index 6b717fa6..925cadbf 100644 --- a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/config/Config.java +++ b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/config/Config.java @@ -20,6 +20,7 @@ */ public class Config { private File pluginDir; + private boolean cleanPluginDir; private boolean showWarnings; private boolean showAllWarnings; private boolean showAvailableUpdates; @@ -49,6 +50,7 @@ public class Config { private Config( File pluginDir, + boolean cleanPluginDir, boolean showWarnings, boolean showAllWarnings, boolean showAvailableUpdates, @@ -67,6 +69,7 @@ private Config( boolean skipFailedPlugins, OutputFormat outputFormat) { this.pluginDir = pluginDir; + this.cleanPluginDir = cleanPluginDir; this.showWarnings = showWarnings; this.showAllWarnings = showAllWarnings; this.showAvailableUpdates = showAvailableUpdates; @@ -90,6 +93,10 @@ public File getPluginDir() { return pluginDir; } + public boolean isCleanPluginDir() { + return cleanPluginDir; + } + public boolean isShowWarnings() { return showWarnings; } @@ -165,6 +172,7 @@ public static Builder builder() { public static class Builder { private File pluginDir; + private boolean cleanPluginDir; private boolean showWarnings; private boolean showAllWarnings; private boolean showAvailableUpdates; @@ -191,6 +199,11 @@ public Builder withPluginDir(File pluginDir) { return this; } + public Builder withCleanPluginsDir(boolean cleanPluginDir) { + this.cleanPluginDir = cleanPluginDir; + return this; + } + public Builder withShowWarnings(boolean showWarnings) { this.showWarnings = showWarnings; return this; @@ -284,6 +297,7 @@ public Builder withOutputFormat(OutputFormat outputFormat) { public Config build() { return new Config( pluginDir, + cleanPluginDir, showWarnings, showAllWarnings, showAvailableUpdates, diff --git a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java index 804dc30a..54cd2ea6 100644 --- a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java +++ b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java @@ -68,7 +68,10 @@ public class PluginManager { private static final VersionNumber LATEST = new VersionNumber("latest"); private List failedPlugins; - private File refDir; + /** + * Directory where the plugins will be downloaded + */ + private File pluginDir; private String jenkinsUcLatest; private @CheckForNull VersionNumber jenkinsVersion; private @CheckForNull File jenkinsWarFile; @@ -96,7 +99,7 @@ public class PluginManager { @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "we want the user to be able to specify a path") public PluginManager(Config cfg) { this.cfg = cfg; - refDir = cfg.getPluginDir(); + pluginDir = cfg.getPluginDir(); jenkinsVersion = cfg.getJenkinsVersion(); final String warArg = cfg.getJenkinsWar(); jenkinsWarFile = warArg != null ? new File(warArg) : null; @@ -129,19 +132,20 @@ public void start() { * @since TODO */ public void start(boolean downloadUc) { - if (refDir.exists()) { + if (cfg.isCleanPluginDir() && pluginDir.exists()) { try { - File[] toBeDeleted = refDir.listFiles(); + logVerbose("Cleaning up the target plugin directory: " + pluginDir); + File[] toBeDeleted = pluginDir.listFiles(); if (toBeDeleted != null) { for (File deletableFile : toBeDeleted) { FileUtils.forceDelete(deletableFile); } } } catch (IOException e) { - throw new UncheckedIOException("Unable to delete: " + refDir.getAbsolutePath(), e); + throw new UncheckedIOException("Unable to delete: " + pluginDir.getAbsolutePath(), e); } } - createRefDir(); + createPluginDir(cfg.isCleanPluginDir()); if (useLatestSpecified && useLatestAll) { throw new PluginDependencyStrategyException("Only one plugin dependency version strategy can be selected " + @@ -171,9 +175,19 @@ public void start(boolean downloadUc) { System.out.println("Done"); } - void createRefDir() { + void createPluginDir(boolean failIfExists) { + if (pluginDir.exists()) { + if (failIfExists) { + throw new DirectoryCreationException("The plugin directory already exists: " + pluginDir); + } else { + if (!pluginDir.isDirectory()) { + throw new DirectoryCreationException("The plugin directory path is not a directory: " + pluginDir); + } + return; + } + } try { - Files.createDirectories(refDir.toPath()); + Files.createDirectories(pluginDir.toPath()); } catch (IOException e) { throw new DirectoryCreationException("Unable to create plugin directory", e); } @@ -449,7 +463,8 @@ public void checkVersionCompatibility(VersionNumber jenkinsVersion, List } /** - * Downloads a list of plugins + * Downloads a list of plugins. + * Plugins will be downloaded to a temporary directory, and then copied over to the final destination. * * @param plugins list of plugins to download */ @@ -909,11 +924,11 @@ private Map resolveRecursiveDependencies(Plugin plugin, @CheckFo * resolved after the plugin is downloaded. * * @param plugin to download - * @param location location to download plugin to. If location is set to null, will download to the plugin folder + * @param location location to download plugin to. If location is set to {@code null}, will download to the plugin folder * otherwise will download to the temporary location specified. * @return boolean signifying if plugin was successful */ - public boolean downloadPlugin(Plugin plugin, File location) { + public boolean downloadPlugin(Plugin plugin, @CheckForNull File location) { String pluginName = plugin.getName(); VersionNumber pluginVersion = plugin.getVersion(); // location will be populated if downloading a plugin to a temp file to determine dependencies @@ -995,7 +1010,7 @@ public String getPluginDownloadUrl(Plugin plugin) { * be null * @return true if download is successful, false otherwise */ - public boolean downloadToFile(String urlString, Plugin plugin, File fileLocation) { + public boolean downloadToFile(String urlString, Plugin plugin, @CheckForNull File fileLocation) { return downloadToFile(urlString, plugin, fileLocation, DEFAULT_MAX_RETRIES); } @@ -1011,10 +1026,10 @@ public boolean downloadToFile(String urlString, Plugin plugin, File fileLocation * @return true if download is successful, false otherwise */ @SuppressFBWarnings({"RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE", "PATH_TRAVERSAL_IN"}) - public boolean downloadToFile(String urlString, Plugin plugin, File fileLocation, int maxRetries) { + public boolean downloadToFile(String urlString, Plugin plugin, @CheckForNull File fileLocation, int maxRetries) { File pluginFile; if (fileLocation == null) { - pluginFile = new File(refDir, plugin.getArchiveFileName()); + pluginFile = new File(pluginDir, plugin.getArchiveFileName()); System.out.println("\nDownloading plugin " + plugin.getName() + " from url: " + urlString); } else { pluginFile = fileLocation; @@ -1209,7 +1224,7 @@ public Map installedPlugins() { FileFilter fileFilter = new WildcardFileFilter("*.jpi"); // Only lists files in same directory, does not list files recursively - File[] files = refDir.listFiles(fileFilter); + File[] files = pluginDir.listFiles(fileFilter); if (files != null) { for (File file : files) { diff --git a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java index 7fa61aac..01d25fcd 100644 --- a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java +++ b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java @@ -17,6 +17,7 @@ import org.json.JSONObject; import org.junit.BeforeClass; import org.junit.ClassRule; +import org.junit.Ignore; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -191,4 +192,30 @@ public void allowsLatestTopLevelDependency() throws IOException { pluginManager.findPluginsAndDependencies(requestedPlugins); assertThatNoException(); } + + //TODO: Enable as auto-test once it can run without massive traffic overhead + @Test + @Ignore + public void verifyDownloads() throws Exception { + + // First cycle, empty dir + List requestedPlugins_1 = new ArrayList<>(Arrays.asList( + new Plugin("workflow-job", "2.39", null, null) + )); + PluginManager pluginManager = initPluginManager( + configBuilder -> configBuilder.withPlugins(requestedPlugins_1)); + pluginManager.start(); + + // Second cycle + List requestedPlugins_2 = new ArrayList<>(Arrays.asList( + new Plugin("workflow-job", "2.40", null, null), + new Plugin("pipeline-utility-steps", "2.6.1", null, null) + )); + PluginManager pluginManager2 = initPluginManager( + configBuilder -> configBuilder.withPlugins(requestedPlugins_2)); + pluginManager2.start(); + + + Plugin + } } diff --git a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerTest.java b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerTest.java index 78b913b3..4d54ade7 100644 --- a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerTest.java +++ b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerTest.java @@ -78,7 +78,7 @@ public void startTest() { PluginManager pluginManagerSpy = spy(pluginManager); - doNothing().when(pluginManagerSpy).createRefDir(); + doNothing().when(pluginManagerSpy).createPluginDir(true); VersionNumber versionNumber = new VersionNumber("2.182"); doReturn(versionNumber).when(pluginManagerSpy).getJenkinsVersionFromWar(); doNothing().when(pluginManagerSpy).getUCJson(versionNumber); From d6bdb3e0be07b27f441bf9b5806923eeb69f5971 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Tue, 8 Dec 2020 11:18:42 +0100 Subject: [PATCH 02/11] Ensure that the plugins get actually downloaded --- .../pluginmanager/impl/PluginManagerIntegrationTest.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java index 01d25fcd..1f0e012a 100644 --- a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java +++ b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java @@ -203,7 +203,7 @@ public void verifyDownloads() throws Exception { new Plugin("workflow-job", "2.39", null, null) )); PluginManager pluginManager = initPluginManager( - configBuilder -> configBuilder.withPlugins(requestedPlugins_1)); + configBuilder -> configBuilder.withPlugins(requestedPlugins_1).withDoDownload(true)); pluginManager.start(); // Second cycle @@ -214,8 +214,5 @@ public void verifyDownloads() throws Exception { PluginManager pluginManager2 = initPluginManager( configBuilder -> configBuilder.withPlugins(requestedPlugins_2)); pluginManager2.start(); - - - Plugin } } From d06b926281e1def506fa276b52926f8004658cb0 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Tue, 8 Dec 2020 13:38:52 +0100 Subject: [PATCH 03/11] Update plugin download logic to use a temporary directory and to handle skipFailed correctly --- .../pluginmanager/impl/PluginManager.java | 101 ++++++++++-------- .../pluginmanager/util/ManifestTools.java | 73 +++++++++++++ .../impl/PluginManagerIntegrationTest.java | 30 ++++-- 3 files changed, 155 insertions(+), 49 deletions(-) create mode 100644 plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/util/ManifestTools.java diff --git a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java index 54cd2ea6..54e24d2c 100644 --- a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java +++ b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java @@ -16,22 +16,9 @@ import java.net.URISyntaxException; import java.net.URL; import java.nio.charset.StandardCharsets; -import java.nio.file.FileSystem; -import java.nio.file.FileSystems; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.PathMatcher; -import java.nio.file.Paths; +import java.nio.file.*; import java.security.MessageDigest; -import java.util.ArrayList; -import java.util.Base64; -import java.util.Collections; -import java.util.Deque; -import java.util.HashMap; -import java.util.Iterator; -import java.util.LinkedList; -import java.util.List; -import java.util.Map; +import java.util.*; import java.util.concurrent.ExecutionException; import java.util.concurrent.ForkJoinPool; import java.util.jar.Attributes; @@ -41,6 +28,8 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.CheckForNull; + +import io.jenkins.tools.pluginmanager.util.ManifestTools; import org.apache.commons.codec.digest.DigestUtils; import org.apache.commons.io.FileUtils; import org.apache.commons.io.FilenameUtils; @@ -220,6 +209,8 @@ public List findPluginsToDownload(Map requestedPlugins) installedPluginVersions.get(pluginName).getVersion(); } if (installedVersion == null) { + logVerbose(String.format( + "Will install new plugin %s %s", pluginName, plugin.getVersion())); pluginsToDownload.add(plugin); } else if (installedVersion.isOlderThan(plugin.getVersion())) { logVerbose(String.format( @@ -469,10 +460,18 @@ public void checkVersionCompatibility(VersionNumber jenkinsVersion, List * @param plugins list of plugins to download */ public void downloadPlugins(List plugins) { + final File downloadsTmpDir; + try { + downloadsTmpDir = Files.createTempDirectory("plugin-installation-manager-downloads").toFile(); + } catch (IOException ex) { + throw new DownloadPluginException("Cannot create a temporary directory for downloads", ex); + } + + // Download to a temporary dir ForkJoinPool ioThreadPool = new ForkJoinPool(64); try { ioThreadPool.submit(() -> plugins.parallelStream().forEach(plugin -> { - boolean successfulDownload = downloadPlugin(plugin, null); + boolean successfulDownload = downloadPlugin(plugin, new File(downloadsTmpDir, plugin.getArchiveFileName())); if (skipFailedPlugins) { System.out.println( "SKIP: Unable to download " + plugin.getName()); @@ -489,6 +488,35 @@ public void downloadPlugins(List plugins) { e.printStackTrace(); } } + + // Filter out failed plugins + final List failedPlugins = getFailedPlugins(); + if (!skipFailedPlugins && failedPlugins.size() > 0) { + throw new DownloadPluginException("Some plugin downloads failed: " + + failedPlugins.stream().map(Plugin::getName).collect(Collectors.joining(","))); + } + Set failedPluginNames = new HashSet<>(failedPlugins.size()); + failedPlugins.forEach(plugin -> failedPluginNames.add(plugin.getName())); + + // Copy files over to the destination directory + for (Plugin plugin : plugins) { + String archiveName = plugin.getArchiveFileName(); + File downloadedPlugin = new File(downloadsTmpDir, archiveName); + try { + if (failedPluginNames.contains(plugin.getName())) { + System.out.println("Will skip the failed plugin download: " + plugin.getName()); + Files.deleteIfExists(downloadedPlugin.toPath()); + } + // We do not double-check overrides here, because findPluginsToDownload() has already done it + Files.copy(downloadedPlugin.toPath(), new File(pluginDir, archiveName).toPath(), StandardCopyOption.REPLACE_EXISTING); + } catch (IOException ex) { + if (skipFailedPlugins) { + System.out.println("SKIP: Unable to copy " + plugin.getName() + " to the plugin directory"); + } else { + throw new DownloadPluginException("Unable to copy " + plugin.getName() + " to the plugin directory", ex); + } + } + } } /** @@ -720,6 +748,7 @@ public VersionNumber getLatestPluginVersion(String pluginName) { * @return list of dependencies that were parsed from the plugin's manifest file */ public List resolveDependenciesFromManifest(Plugin plugin) { + // TODO(oleg_nenashev): refactor to use ManifestTools. This logic not only resolves dependencies, but also modifies the plugin's metadata List dependentPlugins = new ArrayList<>(); try { File tempFile = Files.createTempFile(FilenameUtils.getName(plugin.getName()), ".jpi").toFile(); @@ -733,13 +762,13 @@ public List resolveDependenciesFromManifest(Plugin plugin) { if (plugin.getVersion().toString().equals("latest") || plugin.getVersion().toString().equals("experimental")) { - String version = getAttributeFromManifest(tempFile, "Plugin-Version"); + String version = ManifestTools.getAttributeFromManifest(tempFile, "Plugin-Version"); if (!StringUtils.isEmpty(version)) { plugin.setVersion(new VersionNumber(version)); } } - String dependencyString = getAttributeFromManifest(tempFile, "Plugin-Dependencies"); + String dependencyString = ManifestTools.getAttributeFromManifest(tempFile, "Plugin-Dependencies"); //not all plugin Manifests contain the Plugin-Dependencies field if (StringUtils.isEmpty(dependencyString)) { @@ -771,7 +800,7 @@ public List resolveDependenciesFromManifest(Plugin plugin) { .map(p -> p.getName() + " " + p.getVersion()) .collect(Collectors.joining("\n"))); - plugin.setJenkinsVersion(getAttributeFromManifest(tempFile, "Jenkins-Version")); + plugin.setJenkinsVersion(ManifestTools.getAttributeFromManifest(tempFile, "Jenkins-Version")); Files.delete(tempFile.toPath()); return dependentPlugins; } catch (IOException e) { @@ -1141,28 +1170,6 @@ private byte[] calculateChecksum(File pluginFile) { } } - /** - * Given a jar file and a key to retrieve from the jar's MANIFEST.MF file, confirms that the file is a jar returns - * the value matching the key - * - * @param file jar file to get manifest from - * @param key key matching value to retrieve - * @return value matching the key in the jar file - */ - public String getAttributeFromManifest(File file, String key) { - try (JarFile jarFile = new JarFile(file)) { - Manifest manifest = jarFile.getManifest(); - Attributes attributes = manifest.getMainAttributes(); - return attributes.getValue(key); - } catch (IOException e) { - System.out.println("Unable to open " + file); - if (key.equals("Plugin-Dependencies")) { - throw new DownloadPluginException("Unable to determine plugin dependencies", e); - } - } - return null; - } - /** * Gets Jenkins version using one of the available methods. * @return Jenkins version or {@code null} if it cannot be determined @@ -1190,7 +1197,7 @@ public VersionNumber getJenkinsVersionFromWar() { System.out.println("Unable to get Jenkins version from the WAR file: WAR file path is not defined."); return null; } - String version = getAttributeFromManifest(jenkinsWarFile, "Jenkins-Version"); + String version = ManifestTools.getAttributeFromManifest(jenkinsWarFile, "Jenkins-Version"); if (StringUtils.isEmpty(version)) { System.out.println("Unable to get Jenkins version from the WAR file " + jenkinsWarFile.getPath()); return null; @@ -1206,7 +1213,7 @@ public VersionNumber getJenkinsVersionFromWar() { * @return plugin version */ public String getPluginVersion(File file) { - String version = getAttributeFromManifest(file, "Plugin-Version"); + String version = ManifestTools.getAttributeFromManifest(file, "Plugin-Version"); if (StringUtils.isEmpty(version)) { System.out.println("Unable to get plugin version from " + file); return ""; @@ -1214,6 +1221,14 @@ public String getPluginVersion(File file) { return version; } + /** + * @deprecated Use {@link ManifestTools#getAttributeFromManifest(File, String)} + */ + @Deprecated + public String getAttributeFromManifest(File file, String key) { + return ManifestTools.getAttributeFromManifest(file, key); + } + /** * Finds all the plugins and their versions currently in the plugin directory specified in the Config class * diff --git a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/util/ManifestTools.java b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/util/ManifestTools.java new file mode 100644 index 00000000..b5fa84a8 --- /dev/null +++ b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/util/ManifestTools.java @@ -0,0 +1,73 @@ +package io.jenkins.tools.pluginmanager.util; + +import hudson.util.VersionNumber; +import io.jenkins.tools.pluginmanager.impl.DownloadPluginException; +import io.jenkins.tools.pluginmanager.impl.Plugin; +import org.apache.commons.lang3.StringUtils; + +import java.io.File; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.jar.Attributes; +import java.util.jar.JarFile; +import java.util.jar.Manifest; + +public class ManifestTools { + + public static Plugin readPluginFromFile(File file) throws IOException { + Plugin plugin = new Plugin(file.getName(), "undefined", null, null); + List dependentPlugins = new ArrayList<>(); + + // TODO: refactor code so that we read the manifest only once + String version = getAttributeFromManifest(file, "Plugin-Version"); + if (!StringUtils.isEmpty(version)) { + plugin.setVersion(new VersionNumber(version)); + } + plugin.setJenkinsVersion(getAttributeFromManifest(file, "Jenkins-Version")); + + + String dependencyString = getAttributeFromManifest(file, "Plugin-Dependencies"); + if (StringUtils.isEmpty(dependencyString)) { + // not all plugin Manifests contain the Plugin-Dependencies field + return plugin; + } + + String[] dependencies = dependencyString.split(","); + for (String dependency : dependencies) { + if (!dependency.contains("resolution:=optional")) { + String[] pluginInfo = dependency.split(":"); + String pluginName = pluginInfo[0]; + String pluginVersion = pluginInfo[1]; + Plugin dependentPlugin = new Plugin(pluginName, pluginVersion, null, null); + dependentPlugins.add(dependentPlugin); + dependentPlugin.setParent(plugin); + } + } + + return plugin; + } + + /** + * Given a jar file and a key to retrieve from the jar's MANIFEST.MF file, confirms that the file is a jar returns + * the value matching the key + * + * @param file jar file to get manifest from + * @param key key matching value to retrieve + * @return value matching the key in the jar file + */ + public static String getAttributeFromManifest(File file, String key) { + try (JarFile jarFile = new JarFile(file)) { + Manifest manifest = jarFile.getManifest(); + Attributes attributes = manifest.getMainAttributes(); + return attributes.getValue(key); + } catch (IOException e) { + System.out.println("Unable to open " + file); + if (key.equals("Plugin-Dependencies")) { + throw new DownloadPluginException("Unable to determine plugin dependencies", e); + } + } + return null; + } + +} diff --git a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java index 1f0e012a..5de3f4e2 100644 --- a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java +++ b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java @@ -13,11 +13,13 @@ import java.util.Map; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; + +import io.jenkins.tools.pluginmanager.util.ManifestTools; import org.apache.commons.io.IOUtils; +import org.hamcrest.CoreMatchers; import org.json.JSONObject; import org.junit.BeforeClass; import org.junit.ClassRule; -import org.junit.Ignore; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -26,6 +28,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatNoException; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.Assert.assertTrue; /** * Tests for {@link PluginManager} which operate with real data. @@ -199,20 +202,35 @@ public void allowsLatestTopLevelDependency() throws IOException { public void verifyDownloads() throws Exception { // First cycle, empty dir + Plugin initialWorkflowJob = new Plugin("workflow-job", "2.39", null, null); List requestedPlugins_1 = new ArrayList<>(Arrays.asList( - new Plugin("workflow-job", "2.39", null, null) + initialWorkflowJob )); PluginManager pluginManager = initPluginManager( configBuilder -> configBuilder.withPlugins(requestedPlugins_1).withDoDownload(true)); pluginManager.start(); + assertPluginInstalled(initialWorkflowJob); - // Second cycle + // Second cycle, with plugin update and new plugin installation + Plugin workflowJob = new Plugin("workflow-job", "2.40", null, null); + Plugin utilitySteps = new Plugin("pipeline-utility-steps", "2.6.1", null, null); List requestedPlugins_2 = new ArrayList<>(Arrays.asList( - new Plugin("workflow-job", "2.40", null, null), - new Plugin("pipeline-utility-steps", "2.6.1", null, null) + workflowJob, utilitySteps )); PluginManager pluginManager2 = initPluginManager( - configBuilder -> configBuilder.withPlugins(requestedPlugins_2)); + configBuilder -> configBuilder.withPlugins(requestedPlugins_2).withDoDownload(true)); pluginManager2.start(); + + // Ensure that the plugins are actually in place + assertPluginInstalled(workflowJob); + assertPluginInstalled(utilitySteps); + } + + public void assertPluginInstalled(Plugin plugin) throws IOException, AssertionError { + File pluginArchive = new File(pluginsDir, plugin.getArchiveFileName()); + + assertTrue("Plugin is not installed: " + plugin, pluginArchive.exists()); + Plugin installed = ManifestTools.readPluginFromFile(pluginArchive); + assertThat(installed.getVersion()).isEqualByComparingTo(plugin.getVersion()); } } From 7643ab2efabf45f8a73a042660192eb8b3ccdaa5 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Tue, 8 Dec 2020 13:41:24 +0100 Subject: [PATCH 04/11] Move plugin files, do not just copy --- .../java/io/jenkins/tools/pluginmanager/impl/PluginManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java index 54e24d2c..74318a59 100644 --- a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java +++ b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java @@ -508,7 +508,7 @@ public void downloadPlugins(List plugins) { Files.deleteIfExists(downloadedPlugin.toPath()); } // We do not double-check overrides here, because findPluginsToDownload() has already done it - Files.copy(downloadedPlugin.toPath(), new File(pluginDir, archiveName).toPath(), StandardCopyOption.REPLACE_EXISTING); + Files.move(downloadedPlugin.toPath(), new File(pluginDir, archiveName).toPath(), StandardCopyOption.REPLACE_EXISTING); } catch (IOException ex) { if (skipFailedPlugins) { System.out.println("SKIP: Unable to copy " + plugin.getName() + " to the plugin directory"); From add4faa23d71dc0ff01780e4f05a07478a1098d0 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Tue, 8 Dec 2020 13:42:22 +0100 Subject: [PATCH 05/11] Apply suggestions from code review --- .../io/jenkins/tools/pluginmanager/impl/PluginManager.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java index 74318a59..d608f237 100644 --- a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java +++ b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java @@ -511,9 +511,9 @@ public void downloadPlugins(List plugins) { Files.move(downloadedPlugin.toPath(), new File(pluginDir, archiveName).toPath(), StandardCopyOption.REPLACE_EXISTING); } catch (IOException ex) { if (skipFailedPlugins) { - System.out.println("SKIP: Unable to copy " + plugin.getName() + " to the plugin directory"); + System.out.println("SKIP: Unable to move" + plugin.getName() + " to the plugin directory"); } else { - throw new DownloadPluginException("Unable to copy " + plugin.getName() + " to the plugin directory", ex); + throw new DownloadPluginException("Unable to move" + plugin.getName() + " to the plugin directory", ex); } } } From 921ec69e10366c1d290d9d67a7b587355b9d9c36 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Tue, 8 Dec 2020 13:50:44 +0100 Subject: [PATCH 06/11] Fix test imports --- .../tools/pluginmanager/impl/PluginManagerIntegrationTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java index 5de3f4e2..a8339861 100644 --- a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java +++ b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java @@ -16,10 +16,10 @@ import io.jenkins.tools.pluginmanager.util.ManifestTools; import org.apache.commons.io.IOUtils; -import org.hamcrest.CoreMatchers; import org.json.JSONObject; import org.junit.BeforeClass; import org.junit.ClassRule; +import org.junit.Ignore; import org.junit.Test; import org.junit.rules.TemporaryFolder; From 18272cab40a0e72d3e0d69a86c118680b8a9ed64 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Tue, 8 Dec 2020 14:05:00 +0100 Subject: [PATCH 07/11] Revert to the old method make Mockito happy --- .../tools/pluginmanager/impl/PluginManager.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java index d608f237..218b7efa 100644 --- a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java +++ b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java @@ -762,13 +762,13 @@ public List resolveDependenciesFromManifest(Plugin plugin) { if (plugin.getVersion().toString().equals("latest") || plugin.getVersion().toString().equals("experimental")) { - String version = ManifestTools.getAttributeFromManifest(tempFile, "Plugin-Version"); + String version = getAttributeFromManifest(tempFile, "Plugin-Version"); if (!StringUtils.isEmpty(version)) { plugin.setVersion(new VersionNumber(version)); } } - String dependencyString = ManifestTools.getAttributeFromManifest(tempFile, "Plugin-Dependencies"); + String dependencyString = getAttributeFromManifest(tempFile, "Plugin-Dependencies"); //not all plugin Manifests contain the Plugin-Dependencies field if (StringUtils.isEmpty(dependencyString)) { @@ -800,7 +800,7 @@ public List resolveDependenciesFromManifest(Plugin plugin) { .map(p -> p.getName() + " " + p.getVersion()) .collect(Collectors.joining("\n"))); - plugin.setJenkinsVersion(ManifestTools.getAttributeFromManifest(tempFile, "Jenkins-Version")); + plugin.setJenkinsVersion(getAttributeFromManifest(tempFile, "Jenkins-Version")); Files.delete(tempFile.toPath()); return dependentPlugins; } catch (IOException e) { @@ -1197,7 +1197,7 @@ public VersionNumber getJenkinsVersionFromWar() { System.out.println("Unable to get Jenkins version from the WAR file: WAR file path is not defined."); return null; } - String version = ManifestTools.getAttributeFromManifest(jenkinsWarFile, "Jenkins-Version"); + String version = getAttributeFromManifest(jenkinsWarFile, "Jenkins-Version"); if (StringUtils.isEmpty(version)) { System.out.println("Unable to get Jenkins version from the WAR file " + jenkinsWarFile.getPath()); return null; @@ -1213,7 +1213,7 @@ public VersionNumber getJenkinsVersionFromWar() { * @return plugin version */ public String getPluginVersion(File file) { - String version = ManifestTools.getAttributeFromManifest(file, "Plugin-Version"); + String version = getAttributeFromManifest(file, "Plugin-Version"); if (StringUtils.isEmpty(version)) { System.out.println("Unable to get plugin version from " + file); return ""; @@ -1226,7 +1226,7 @@ public String getPluginVersion(File file) { */ @Deprecated public String getAttributeFromManifest(File file, String key) { - return ManifestTools.getAttributeFromManifest(file, key); + return getAttributeFromManifest(file, key); } /** From 4c9cdd365853d364e3aa45bd3221833900ca6028 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Tue, 8 Dec 2020 14:30:13 +0100 Subject: [PATCH 08/11] Update Plugin Installation Manager, add smoke test for downloads and updates --- .../pluginmanager/impl/PluginManager.java | 9 ++--- .../impl/PluginManagerIntegrationTest.java | 33 +++++++++++++++++-- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java index 218b7efa..fa53f7ae 100644 --- a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java +++ b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java @@ -493,7 +493,8 @@ public void downloadPlugins(List plugins) { final List failedPlugins = getFailedPlugins(); if (!skipFailedPlugins && failedPlugins.size() > 0) { throw new DownloadPluginException("Some plugin downloads failed: " + - failedPlugins.stream().map(Plugin::getName).collect(Collectors.joining(","))); + failedPlugins.stream().map(Plugin::getName).collect(Collectors.joining(",")) + + ". See " + downloadsTmpDir.getAbsolutePath() + " for the temporary download directory"); } Set failedPluginNames = new HashSet<>(failedPlugins.size()); failedPlugins.forEach(plugin -> failedPluginNames.add(plugin.getName())); @@ -504,8 +505,8 @@ public void downloadPlugins(List plugins) { File downloadedPlugin = new File(downloadsTmpDir, archiveName); try { if (failedPluginNames.contains(plugin.getName())) { - System.out.println("Will skip the failed plugin download: " + plugin.getName()); - Files.deleteIfExists(downloadedPlugin.toPath()); + System.out.println("Will skip the failed plugin download: " + plugin.getName() + + ". See " + downloadedPlugin.getAbsolutePath() + " for the downloaded file"); } // We do not double-check overrides here, because findPluginsToDownload() has already done it Files.move(downloadedPlugin.toPath(), new File(pluginDir, archiveName).toPath(), StandardCopyOption.REPLACE_EXISTING); @@ -1226,7 +1227,7 @@ public String getPluginVersion(File file) { */ @Deprecated public String getAttributeFromManifest(File file, String key) { - return getAttributeFromManifest(file, key); + return ManifestTools.getAttributeFromManifest(file, key); } /** diff --git a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java index a8339861..240e78d4 100644 --- a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java +++ b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java @@ -196,10 +196,39 @@ public void allowsLatestTopLevelDependency() throws IOException { assertThatNoException(); } - //TODO: Enable as auto-test once it can run without massive traffic overhead + + @Test + public void verifyDownloads_smoke() throws Exception { + + // First cycle, empty dir + Plugin initialTrileadAPI = new Plugin("trilead-api", "1.0.12", null, null); + List requestedPlugins_1 = new ArrayList<>(Arrays.asList( + initialTrileadAPI + )); + PluginManager pluginManager = initPluginManager( + configBuilder -> configBuilder.withPlugins(requestedPlugins_1).withDoDownload(true)); + pluginManager.start(); + assertPluginInstalled(initialTrileadAPI); + + // Second cycle, with plugin update and new plugin installation + Plugin trileadAPI = new Plugin("trilead-api", "1.0.13", null, null); + Plugin snakeYamlAPI = new Plugin("snakeyaml-api", "1.27.0", null, null); + List requestedPlugins_2 = new ArrayList<>(Arrays.asList( + trileadAPI, snakeYamlAPI + )); + PluginManager pluginManager2 = initPluginManager( + configBuilder -> configBuilder.withPlugins(requestedPlugins_2).withDoDownload(true)); + pluginManager2.start(); + + // Ensure that the plugins are actually in place + assertPluginInstalled(trileadAPI); + assertPluginInstalled(snakeYamlAPI); + } + + //TODO: Enable as auto-test once it can run without big traffic overhead (15 plugin downloads) @Test @Ignore - public void verifyDownloads() throws Exception { + public void verifyDownloads_withDependencies() throws Exception { // First cycle, empty dir Plugin initialWorkflowJob = new Plugin("workflow-job", "2.39", null, null); From 9fe873516865b31af0ca7a27c420feee475c528f Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Tue, 8 Dec 2020 15:09:13 +0100 Subject: [PATCH 09/11] Mockito test is replaced bz the integration test --- .../pluginmanager/impl/PluginManager.java | 4 ++-- .../pluginmanager/impl/PluginManagerTest.java | 20 ------------------- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java index fa53f7ae..a6c650a0 100644 --- a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java +++ b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java @@ -512,9 +512,9 @@ public void downloadPlugins(List plugins) { Files.move(downloadedPlugin.toPath(), new File(pluginDir, archiveName).toPath(), StandardCopyOption.REPLACE_EXISTING); } catch (IOException ex) { if (skipFailedPlugins) { - System.out.println("SKIP: Unable to move" + plugin.getName() + " to the plugin directory"); + System.out.println("SKIP: Unable to move " + plugin.getName() + " to the plugin directory"); } else { - throw new DownloadPluginException("Unable to move" + plugin.getName() + " to the plugin directory", ex); + throw new DownloadPluginException("Unable to move " + plugin.getName() + " to the plugin directory", ex); } } } diff --git a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerTest.java b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerTest.java index 4d54ade7..f768db54 100644 --- a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerTest.java +++ b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerTest.java @@ -496,26 +496,6 @@ public void checkVersionCompatibilityPassTest() { pm.checkVersionCompatibility(new VersionNumber("2.121.2"), Arrays.asList(plugin1, plugin2)); } - @Test - public void downloadPluginsSuccessfulTest() { - Config config = Config.builder() - .withJenkinsWar(Settings.DEFAULT_WAR) - .withPluginDir(new File(folder.getRoot(), "plugins")) - .build(); - - PluginManager pluginManager = new PluginManager(config); - PluginManager pluginManagerSpy = spy(pluginManager); - - doReturn(true).when(pluginManagerSpy).downloadPlugin(any(Plugin.class), nullable(File.class)); - - List plugins = singletonList( - new Plugin("plugin", "1.0", null, null)); - - pluginManagerSpy.downloadPlugins(plugins); - - assertThat(pluginManagerSpy.getFailedPlugins()).isEmpty(); - } - @Test public void downloadPluginsUnsuccessfulTest() { Config config = Config.builder() From 13e520061cca5b606fbebb907696cdc27f7a69c7 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Wed, 9 Dec 2020 09:16:21 +0100 Subject: [PATCH 10/11] Fix Checkstyle --- .../pluginmanager/impl/PluginManager.java | 33 +++++++++++++++---- .../pluginmanager/util/ManifestTools.java | 7 ++-- .../impl/PluginManagerIntegrationTest.java | 3 +- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java index a6c650a0..b2a42b62 100644 --- a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java +++ b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java @@ -4,6 +4,7 @@ import hudson.util.VersionNumber; import io.jenkins.tools.pluginmanager.config.Config; import io.jenkins.tools.pluginmanager.config.Settings; +import io.jenkins.tools.pluginmanager.util.ManifestTools; import java.io.File; import java.io.FileFilter; import java.io.FileInputStream; @@ -16,20 +17,32 @@ import java.net.URISyntaxException; import java.net.URL; import java.nio.charset.StandardCharsets; -import java.nio.file.*; +import java.nio.file.FileSystem; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.PathMatcher; +import java.nio.file.Paths; +import java.nio.file.StandardCopyOption; import java.security.MessageDigest; -import java.util.*; +import java.util.ArrayList; +import java.util.Base64; +import java.util.Collections; +import java.util.Deque; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Iterator; +import java.util.LinkedList; +import java.util.List; +import java.util.Map; +import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.ForkJoinPool; -import java.util.jar.Attributes; import java.util.jar.JarFile; -import java.util.jar.Manifest; import java.util.regex.Matcher; import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.CheckForNull; - -import io.jenkins.tools.pluginmanager.util.ManifestTools; import org.apache.commons.codec.digest.DigestUtils; import org.apache.commons.io.FileUtils; import org.apache.commons.io.FilenameUtils; @@ -459,6 +472,7 @@ public void checkVersionCompatibility(VersionNumber jenkinsVersion, List * * @param plugins list of plugins to download */ + @SuppressFBWarnings("PATH_TRAVERSAL_IN") public void downloadPlugins(List plugins) { final File downloadsTmpDir; try { @@ -471,7 +485,7 @@ public void downloadPlugins(List plugins) { ForkJoinPool ioThreadPool = new ForkJoinPool(64); try { ioThreadPool.submit(() -> plugins.parallelStream().forEach(plugin -> { - boolean successfulDownload = downloadPlugin(plugin, new File(downloadsTmpDir, plugin.getArchiveFileName())); + boolean successfulDownload = downloadPlugin(plugin, getPluginArchive(downloadsTmpDir, plugin)); if (skipFailedPlugins) { System.out.println( "SKIP: Unable to download " + plugin.getName()); @@ -520,6 +534,11 @@ public void downloadPlugins(List plugins) { } } + @SuppressFBWarnings("PATH_TRAVERSAL_IN") + private File getPluginArchive(File pluginDir, Plugin plugin) { + return new File(pluginDir, plugin.getArchiveFileName()); + } + /** * Given a list of plugins, finds the recursive set of all dependent plugins. If multiple plugins rely on different * versions of the same plugin, the higher version required will replace the lower version dependency diff --git a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/util/ManifestTools.java b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/util/ManifestTools.java index b5fa84a8..57b8c5f7 100644 --- a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/util/ManifestTools.java +++ b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/util/ManifestTools.java @@ -3,8 +3,6 @@ import hudson.util.VersionNumber; import io.jenkins.tools.pluginmanager.impl.DownloadPluginException; import io.jenkins.tools.pluginmanager.impl.Plugin; -import org.apache.commons.lang3.StringUtils; - import java.io.File; import java.io.IOException; import java.util.ArrayList; @@ -12,12 +10,13 @@ import java.util.jar.Attributes; import java.util.jar.JarFile; import java.util.jar.Manifest; +import org.apache.commons.lang3.StringUtils; public class ManifestTools { public static Plugin readPluginFromFile(File file) throws IOException { Plugin plugin = new Plugin(file.getName(), "undefined", null, null); - List dependentPlugins = new ArrayList<>(); + // TODO: refactor code so that we read the manifest only once String version = getAttributeFromManifest(file, "Plugin-Version"); @@ -34,6 +33,7 @@ public static Plugin readPluginFromFile(File file) throws IOException { } String[] dependencies = dependencyString.split(","); + List dependentPlugins = new ArrayList<>(); for (String dependency : dependencies) { if (!dependency.contains("resolution:=optional")) { String[] pluginInfo = dependency.split(":"); @@ -44,6 +44,7 @@ public static Plugin readPluginFromFile(File file) throws IOException { dependentPlugin.setParent(plugin); } } + plugin.setDependencies(dependentPlugins); return plugin; } diff --git a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java index 240e78d4..9b0b505c 100644 --- a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java +++ b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java @@ -1,6 +1,7 @@ package io.jenkins.tools.pluginmanager.impl; import io.jenkins.tools.pluginmanager.config.Config; +import io.jenkins.tools.pluginmanager.util.ManifestTools; import java.io.File; import java.io.FileInputStream; import java.io.IOException; @@ -13,8 +14,6 @@ import java.util.Map; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; - -import io.jenkins.tools.pluginmanager.util.ManifestTools; import org.apache.commons.io.IOUtils; import org.json.JSONObject; import org.junit.BeforeClass; From cbcdbba7ae1aeb3c722a5fe45673dd10bcdeac85 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Wed, 9 Dec 2020 10:51:07 +0100 Subject: [PATCH 11/11] Apply suggestions from code review Co-authored-by: Joseph Petersen --- .../java/io/jenkins/tools/pluginmanager/impl/PluginManager.java | 2 +- .../tools/pluginmanager/impl/PluginManagerIntegrationTest.java | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java index b2a42b62..15cb5e91 100644 --- a/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java +++ b/plugin-management-library/src/main/java/io/jenkins/tools/pluginmanager/impl/PluginManager.java @@ -134,7 +134,7 @@ public void start() { * @since TODO */ public void start(boolean downloadUc) { - if (cfg.isCleanPluginDir() && pluginDir.exists()) { + if (cfg.isCleanPluginDir() && pluginDir.exists()) { try { logVerbose("Cleaning up the target plugin directory: " + pluginDir); File[] toBeDeleted = pluginDir.listFiles(); diff --git a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java index 9b0b505c..bbc4ef99 100644 --- a/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java +++ b/plugin-management-library/src/test/java/io/jenkins/tools/pluginmanager/impl/PluginManagerIntegrationTest.java @@ -195,7 +195,6 @@ public void allowsLatestTopLevelDependency() throws IOException { assertThatNoException(); } - @Test public void verifyDownloads_smoke() throws Exception {