[FIXED JENKINS-10315] Only open SFTP channel if have source files

Still try to open SFTP channel in Test Connection, but try to give helpful message if it fails
jenkinsci · Jul 17, 2011 · ee71481 · ee71481
1 parent b340dda
commit ee71481
Show file tree

Hide file tree

Showing 9 changed files with 104 additions and 12 deletions.
diff --git a/pom.xml b/pom.xml
@@ -69,7 +69,7 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins</groupId>
             <artifactId>publish-over</artifactId>
-            <version>0.10-SNAPSHOT</version>
+            <version>0.11-SNAPSHOT</version>
         </dependency>
         <dependency>
             <groupId>junit</groupId>

diff --git a/src/main/java/jenkins/plugins/publish_over_ssh/BapSshClient.java b/src/main/java/jenkins/plugins/publish_over_ssh/BapSshClient.java
@@ -71,6 +71,10 @@ public void setSftp(final ChannelSftp sftp) {
         this.sftp = sftp;
     }
 
+    public Session getSession() {
+        return session;
+    }
+
     public void beginTransfers(final BapSshTransfer transfer) {
         if (disableExec) {
             if (!transfer.hasConfiguredSourceFiles())

diff --git a/src/main/java/jenkins/plugins/publish_over_ssh/BapSshHostConfiguration.java b/src/main/java/jenkins/plugins/publish_over_ssh/BapSshHostConfiguration.java
@@ -34,6 +34,7 @@
 import hudson.model.Hudson;
 import jenkins.plugins.publish_over.BPBuildInfo;
 import jenkins.plugins.publish_over.BPHostConfiguration;
+import jenkins.plugins.publish_over.BapPublisher;
 import jenkins.plugins.publish_over.BapPublisherException;
 import jenkins.plugins.publish_over_ssh.descriptor.BapSshHostConfigurationDescriptor;
 import org.apache.commons.lang.builder.EqualsBuilder;
@@ -103,8 +104,17 @@ private BapSshKeyInfo getEffectiveKeyInfo() {
         return overrideKey ? keyInfo : getCommonConfig();
     }
 
+    @Override
+    public BapSshClient createClient(final BPBuildInfo buildInfo, final BapPublisher publisher) {
+        return createClient(buildInfo, ((BapSshPublisher) publisher).isSftpRequired());
+    }
+
     @Override
     public BapSshClient createClient(final BPBuildInfo buildInfo) {
+        return createClient(buildInfo, true);
+    }
+
+    public BapSshClient createClient(final BPBuildInfo buildInfo, final boolean connectSftp) {
         final JSch ssh = createJSch();
         final Session session = createSession(buildInfo, ssh);
         final BapSshClient bapClient = new BapSshClient(buildInfo, session, isEffectiveDisableExec());
@@ -119,11 +129,7 @@ public BapSshClient createClient(final BPBuildInfo buildInfo) {
             }
             session.setConfig(sessionProperties);
             connect(buildInfo, session);
-            final ChannelSftp sftp = openSftpChannel(buildInfo, session);
-            bapClient.setSftp(sftp);
-            connectSftpChannel(buildInfo, sftp);
-            changeToRootDirectory(bapClient);
-            setRootDirectoryInClient(bapClient, sftp);
+            if (connectSftp) setupSftp(buildInfo, bapClient);
             return bapClient;
         } catch (IOException ioe) {
             bapClient.disconnectQuietly();
@@ -134,6 +140,14 @@ public BapSshClient createClient(final BPBuildInfo buildInfo) {
         }
     }
 
+    private void setupSftp(final BPBuildInfo buildInfo, final BapSshClient bapClient) throws IOException {
+        final ChannelSftp sftp = openSftpChannel(buildInfo, bapClient.getSession());
+        bapClient.setSftp(sftp);
+        connectSftpChannel(buildInfo, sftp);
+        changeToRootDirectory(bapClient);
+        setRootDirectoryInClient(bapClient, sftp);
+    }
+
     private void setKey(final BPBuildInfo buildInfo, final JSch ssh, final BapSshKeyInfo keyInfo) {
         try {
             ssh.addIdentity("TheKey", keyInfo.getEffectiveKey(buildInfo), null, BapSshUtil.toBytes(keyInfo.getPassphrase()));
@@ -172,7 +186,7 @@ private void connectSftpChannel(final BPBuildInfo buildInfo, final ChannelSftp c
         } catch (JSchException jse) {
             final String message = Messages.exception_sftp_connect(jse.getLocalizedMessage());
             LOG.warn(message, jse);
-            throw new BapPublisherException(message); // NOPMD - it's in the log!
+            throw new BapSshSftpSetupException(message); // NOPMD - it's in the log!
         }
         buildInfo.printIfVerbose(Messages.console_sftp_connected());
     }
@@ -185,7 +199,7 @@ private ChannelSftp openSftpChannel(final BPBuildInfo buildInfo, final Session s
         } catch (JSchException jse) {
             final String message = Messages.exception_sftp_open(jse.getLocalizedMessage());
             LOG.warn(message, jse);
-            throw new BapPublisherException(message); // NOPMD - it's in the log!
+            throw new BapSshSftpSetupException(message); // NOPMD - it's in the log!
         }
         buildInfo.printIfVerbose(Messages.console_sftp_opened());
         return sftp;

diff --git a/src/main/java/jenkins/plugins/publish_over_ssh/BapSshPublisher.java b/src/main/java/jenkins/plugins/publish_over_ssh/BapSshPublisher.java
@@ -51,6 +51,13 @@ public BapSshPublisher(final String configName, final boolean verbose, final Arr
         super(configName, verbose, transfers, useWorkspaceInPromotion, usePromotionTimestamp, sshRetry, sshLabel);
     }
 
+    public final boolean isSftpRequired() {
+        for (BapSshTransfer transfer : getTransfers()) {
+            if (transfer.hasConfiguredSourceFiles()) return true;
+        }
+        return false;
+    }
+
     public BapSshRetry getSshRetry() {
         return (BapSshRetry) super.getRetry();
     }

diff --git a/src/main/java/jenkins/plugins/publish_over_ssh/BapSshSftpSetupException.java b/src/main/java/jenkins/plugins/publish_over_ssh/BapSshSftpSetupException.java
@@ -0,0 +1,39 @@
+/*
+ * The MIT License
+ *
+ * Copyright (C) 2010-2011 by Anthony Robinson
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package jenkins.plugins.publish_over_ssh;
+
+import jenkins.plugins.publish_over.BapPublisherException;
+
+public class BapSshSftpSetupException extends BapPublisherException {
+
+    public BapSshSftpSetupException(final Throwable throwable) {
+        super(throwable);
+    }
+
+    public BapSshSftpSetupException(final String message) {
+        super(message);
+    }
+
+}
diff --git a/...ain/java/jenkins/plugins/publish_over_ssh/descriptor/BapSshPublisherPluginDescriptor.java b/...ain/java/jenkins/plugins/publish_over_ssh/descriptor/BapSshPublisherPluginDescriptor.java
@@ -40,6 +40,7 @@
 import jenkins.plugins.publish_over_ssh.BapSshCommonConfiguration;
 import jenkins.plugins.publish_over_ssh.BapSshHostConfiguration;
 import jenkins.plugins.publish_over_ssh.BapSshPublisherPlugin;
+import jenkins.plugins.publish_over_ssh.BapSshSftpSetupException;
 import jenkins.plugins.publish_over_ssh.Messages;
 import net.sf.json.JSONObject;
 import org.kohsuke.stapler.StaplerRequest;
@@ -123,14 +124,20 @@ public FormValidation doTestConnection(final StaplerRequest request, final Stapl
         try {
             hostConfig.createClient(buildInfo).disconnect();
             return FormValidation.ok(Messages.descriptor_testConnection_ok());
+        } catch (BapSshSftpSetupException sse) {
+            return connectionError(Messages.descriptor_testConnection_sftpError(), sse);
         } catch (Exception e) {
-            return FormValidation.errorWithMarkup("<p>"
-                    + Messages.descriptor_testConnection_error() + "</p><p><pre>"
-                    + Util.escape(e.getClass().getCanonicalName() + ": " + e.getLocalizedMessage())
-                    + "</pre></p>");
+            return connectionError(Messages.descriptor_testConnection_error(), e);
         }
     }
 
+    private FormValidation connectionError(final String description, final Exception exception) {
+        return FormValidation.errorWithMarkup("<p>"
+                + description + "</p><p><pre>"
+                + Util.escape(exception.getClass().getCanonicalName() + ": " + exception.getLocalizedMessage())
+                + "</pre></p>");
+    }
+
     private BPBuildInfo createDummyBuildInfo() {
         return new BPBuildInfo(
             TaskListener.NULL,

diff --git a/src/main/resources/jenkins/plugins/publish_over_ssh/Messages.properties b/src/main/resources/jenkins/plugins/publish_over_ssh/Messages.properties
@@ -29,6 +29,7 @@ promotion.descriptor.displayName=Send build artifacts over SSH
 descriptor.displayName=Send build artifacts over SSH
 descriptor.testConnection.ok=Success
 descriptor.testConnection.error=Failed to connect or change directory
+descriptor.testConnection.sftpError=Connected, but failed to setup SFTP - check the SSH server. Exec commands should work, but transferring files will fail
 descriptor.sourceOrExec=Either Source files, Exec command or both must be supplied
 descriptor.sourceFiles.check.configNotFound=Could not find the SSH Server configuration named [{0}]\
   - check the System Configuration and then reload this configuration page

diff --git a/src/main/resources/jenkins/plugins/publish_over_ssh/Messages_no_BV.properties b/src/main/resources/jenkins/plugins/publish_over_ssh/Messages_no_BV.properties
@@ -29,6 +29,7 @@ promotion.descriptor.displayName=S*n* b*i*d a*t*f*c*s o*e* S*H
 descriptor.displayName=S*n* b*i*d a*t*f*c*s o*e* S*H
 descriptor.testConnection.ok=S*c*e*s
 descriptor.testConnection.error=F*i*e* t* c*n*e*t o* c*a*g* d*r*c*o*y
+descriptor.testConnection.sftpError=F*i*e* t* s*t*p S*T* - d*e* t*e s*r*e* s*p*o*t i*? B*a*, b*a*, b*a*
 descriptor.sourceOrExec=E*t*e* S*u*c* f*l*s, E*e* c*m*a*d o* b*t* m*s* b* s*p*l*e*
 descriptor.sourceFiles.check.configNotFound=C*u*d n*t f*n* t*e S*H S*r*e* c*n*i*u*a*i*n n*m*d [{0}]\
   - c*e*k t*e S*s*e* C*n*i*u*a*i*n a*d t*e* r*l*a* t*i* c*n*i*u*a*i*n p*g*

diff --git a/src/test/java/jenkins/plugins/publish_over_ssh/BapHostConfigurationTest.java b/src/test/java/jenkins/plugins/publish_over_ssh/BapHostConfigurationTest.java
@@ -45,6 +45,8 @@
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Properties;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -282,6 +284,23 @@ private BapSshClient assertCreateClientWithDefaultKey(final boolean disableExec)
         assertTrue(client.isDisableExec());
     }
 
+    @Test public void testDontConnectSftpIfNoSourceFilesInAnyTransfers() throws Exception {
+        final BapSshCommonConfiguration defaultKeyInfo = new BapSshCommonConfiguration(TEST_PASSPHRASE, null, null, false);
+        hostConfig = createWithDefaultKeyInfo(mockJSch, defaultKeyInfo);
+        final BapSshTransfer transfer1 = new BapSshTransfer("", "", "", "", false, false, "ls -la", 10000);
+        final BapSshTransfer transfer2 = new BapSshTransfer("", "", "", "", false, false, "pwd", 10000);
+        final ArrayList<BapSshTransfer> transfers = new ArrayList<BapSshTransfer>();
+        transfers.addAll(Arrays.asList(transfer1, transfer2));
+        final BapSshPublisher publisher = new BapSshPublisher(hostConfig.getName(), false, transfers, false, false, null, null);
+        expect(mockJSch.getSession(hostConfig.getUsername(), hostConfig.getHostname(), hostConfig.getPort())).andReturn(mockSession);
+        mockSession.setPassword(defaultKeyInfo.getPassphrase());
+        mockSession.setConfig((Properties) anyObject());
+        mockSession.connect(hostConfig.getTimeout());
+        mockControl.replay();
+        hostConfig.createClient(buildInfo, publisher);
+        mockControl.verify();
+    }    
+
     private void assertCreateClientThrowsException(final Exception messageToInclude) throws Exception {
         assertCreateClientThrowsException(messageToInclude.getLocalizedMessage());
     }