- updated to comply with Testlab REST API changes

- try to remove unneeded dependency to TAP when the feature is not really used or needed - fix SECURITY-854 - fix SECURITY-847
jenkinsci · Jul 6, 2018 · 59d43d5 · 59d43d5
1 parent d34b15b
commit 59d43d5
Show file tree

Hide file tree

Showing 18 changed files with 1,569 additions and 128 deletions.
diff --git a/src/main/java/fi/meliora/testlab/ext/crest/CrestEndpointFactory.java b/src/main/java/fi/meliora/testlab/ext/crest/CrestEndpointFactory.java
@@ -27,10 +27,12 @@ private CrestEndpointFactory() {
     /**
      * Constructs a new CRest endpoint, caches it and returns it for use.
      *
-     * @param url
-     * @param username
-     * @param password
-     * @return
+     * @param url url
+     * @param username user name
+     * @param password password
+     * @param endpointClass endpoint class
+     * @param <T> type
+     * @return endpoint
      */
     public <T>T getEndpoint(String url, String username, String password, Class<T> endpointClass) {
         T endpoint;
@@ -50,15 +52,15 @@ public <T>T getEndpoint(String url, String username, String password, Class<T> e
      * Returns an endpoint to Testlab.
      *
      * If onpremiseUrl is set it used as a base url. It not, this methods peeks for
-     * TESTLAB_<companyid in upper case> system environment variable for testlab api address.
+     * TESTLAB_&lt;companyid in upper case&gt; system environment variable for testlab api address.
      * If none is set a default of https://companyid.melioratestlab.com/api is used.
      *
-     * @param companyId
-     * @parma onpremiseUrl
-     * @param apiKey
-     * @param endpointClass
-     * @param <T>
-     * @return
+     * @param companyId company id
+     * @param onpremiseUrl onpremise url
+     * @param apiKey api key
+     * @param endpointClass endpoint class
+     * @param <T> type
+     * @return endpoint
      */
     public <T>T getTestlabEndpoint(String companyId, String onpremiseUrl, String apiKey, Class<T> endpointClass) {
         String url;

diff --git a/src/main/java/fi/meliora/testlab/ext/crest/ErrorHandler.java b/src/main/java/fi/meliora/testlab/ext/crest/ErrorHandler.java
@@ -1,7 +1,6 @@
 package fi.meliora.testlab.ext.crest;
 
-import fi.meliora.testlab.ext.crest.exception.NotFoundException;
-import fi.meliora.testlab.ext.crest.exception.TestlabAPIException;
+import fi.meliora.testlab.ext.crest.exception.*;
 import org.codegist.crest.io.Request;
 import org.codegist.crest.io.RequestException;
 import org.codegist.crest.io.Response;
@@ -12,7 +11,7 @@
 import java.util.Scanner;
 
 /**
- * Our Testlab api call error handler.
+ * Our Testlab API call error handler.
  *
  * @author Marko Kanala
  */
@@ -22,32 +21,12 @@ public class ErrorHandler implements org.codegist.crest.handler.ErrorHandler {
     @Override
     public <T> T handle(Request request, Exception e) throws Exception {
         if(e instanceof RequestException) {
-            // if we have a http response from testlab just log the error
             RequestException re = (RequestException)e;
             Response testlabResponse = re.getResponse();
 
             if(testlabResponse != null) {
-                // read response
-
-                String responseData = null;
-                // try to read error
-                InputStream is = null;
-                try {
-                    is = testlabResponse.asStream();
-                    if(is != null) {
-                        String charset = testlabResponse.getCharset() != null ? testlabResponse.getCharset().name() : "UTF-8";
-                        Scanner s = new Scanner(is, charset).useDelimiter("\\A");
-                        if(s.hasNext())
-                            responseData = s.next();
-                    }
-                    if(responseData != null && responseData.length() > 400)
-                        responseData = responseData.substring(0, 400) + "...";
-                } catch (Exception ee) {
-                    // ...
-                } finally {
-                    if(is != null)
-                        try { is.close(); } catch (Exception eee) {}
-                }
+                // read response, if any
+                String responseData = getResponseIfAny(testlabResponse);
 
                 if(log.isErrorEnabled())
                     log.error(
@@ -64,7 +43,16 @@ public <T> T handle(Request request, Exception e) throws Exception {
                 if(statusCode == javax.ws.rs.core.Response.Status.NOT_FOUND.getStatusCode()) {
                     // call returned not found status
                     throw new NotFoundException(responseData);
+                } else if(statusCode == javax.ws.rs.core.Response.Status.CONFLICT.getStatusCode()) {
+                    throw new ConflictException(responseData);
+                } else if(statusCode == javax.ws.rs.core.Response.Status.SERVICE_UNAVAILABLE.getStatusCode()) {
+                    throw new ServiceUnavailableException(responseData);
+                } else if(statusCode == javax.ws.rs.core.Response.Status.UNAUTHORIZED.getStatusCode()) {
+                    throw new UnauthorizedException(responseData);
+                } else if(statusCode == javax.ws.rs.core.Response.Status.BAD_REQUEST.getStatusCode()) {
+                    throw new ValidationException(responseData);
                 }
+
                 if(responseData != null && responseData.length() > 0) {
                     throw new TestlabAPIException(responseData);
                 }
@@ -80,4 +68,27 @@ public <T> T handle(Request request, Exception e) throws Exception {
         throw e;
     }
 
+    protected String getResponseIfAny(Response response) {
+        // read response
+        String responseData = null;
+        InputStream is = null;
+        try {
+            is = response.asStream();
+            if(is != null) {
+                String charset = response.getCharset() != null ? response.getCharset().name() : "UTF-8";
+                Scanner s = new Scanner(is, charset).useDelimiter("\\A");
+                if(s.hasNext())
+                    responseData = s.next();
+            }
+            if(responseData != null && responseData.length() > 400)
+                responseData = responseData.substring(0, 400) + "...";
+        } catch (Exception e) {
+            // could not read response, just ignore
+        } finally {
+            if(is != null)
+                try { is.close(); } catch (Exception ee) {}
+        }
+        return responseData;
+    }
+
 }
diff --git a/src/main/java/fi/meliora/testlab/ext/crest/exception/ConflictException.java b/src/main/java/fi/meliora/testlab/ext/crest/exception/ConflictException.java
@@ -0,0 +1,14 @@
+package fi.meliora.testlab.ext.crest.exception;
+
+/**
+ * Thrown if rest endpoint sends 409 CONFLICT response.
+ *
+ * @author Marko Kanala
+ */
+public class ConflictException extends TestlabAPIException {
+
+    public ConflictException(Object responseData) {
+        super(responseData);
+    }
+
+}
diff --git a/src/main/java/fi/meliora/testlab/ext/crest/exception/ServiceUnavailableException.java b/src/main/java/fi/meliora/testlab/ext/crest/exception/ServiceUnavailableException.java
@@ -0,0 +1,15 @@
+package fi.meliora.testlab.ext.crest.exception;
+
+/**
+ * Thrown if rest endpoint sends 503 SERVICE UNAVAILABLE response.
+ *
+ * @author Marko Kanala
+ */
+public class ServiceUnavailableException extends TestlabAPIException {
+
+    public ServiceUnavailableException(Object responseData) {
+        super(responseData);
+    }
+
+}
+
diff --git a/src/main/java/fi/meliora/testlab/ext/crest/exception/UnauthorizedException.java b/src/main/java/fi/meliora/testlab/ext/crest/exception/UnauthorizedException.java
@@ -0,0 +1,15 @@
+package fi.meliora.testlab.ext.crest.exception;
+
+/**
+ * Thrown if rest endpoint sends 401 UNAUTHORIZED response.
+ *
+ * @author Marko Kanala
+ */
+public class UnauthorizedException extends TestlabAPIException {
+
+    public UnauthorizedException(Object responseData) {
+        super(responseData);
+    }
+
+}
+
diff --git a/src/main/java/fi/meliora/testlab/ext/crest/exception/ValidationException.java b/src/main/java/fi/meliora/testlab/ext/crest/exception/ValidationException.java
@@ -0,0 +1,15 @@
+package fi.meliora.testlab.ext.crest.exception;
+
+/**
+ * Thrown if rest endpoint sends 400 BAD_REQUEST response.
+ *
+ * @author Marko Kanala
+ */
+public class ValidationException extends TestlabAPIException {
+
+    public ValidationException(Object responseData) {
+        super(responseData);
+    }
+
+}
+
diff --git a/src/main/java/fi/meliora/testlab/ext/jenkins/Sender.java b/src/main/java/fi/meliora/testlab/ext/jenkins/Sender.java
@@ -21,8 +21,6 @@
 import org.apache.commons.lang.StringUtils;
 import org.apache.tools.ant.DirectoryScanner;
 import org.apache.tools.ant.types.FileSet;
-import org.tap4j.plugin.TapTestResultAction;
-import org.tap4j.plugin.model.TapTestResultResult;
 
 import java.io.File;
 import java.io.IOException;
@@ -60,35 +58,6 @@ public class Sender {
 
     /**
      * Does the actual sending of results to Testlab. Called from appropriate Jenkins extension point.
-     *
-     * @param workspace
-     * @param companyId
-     * @param usingonpremise
-     * @param onpremiseurl
-     * @param apiKey
-     * @param projectKey
-     * @param milestone
-     * @param testRunTitle
-     * @param comment
-     * @param testTargetTitle
-     * @param testEnvironmentTitle
-     * @param tags
-     * @param parameters
-     * @param addIssues
-     * @param mergeAsSingleIssue
-     * @param reopenExisting
-     * @param assignToUser
-     * @param publishTap
-     * @param tapTestsAsSteps,
-     * @param tapFileNameInIdentifier,
-     * @param tapTestNumberInIdentifier
-     * @param importTestCases
-     * @param importTestCasesRootCategory
-     * @param testCaseMappingField
-     * @param publishRobot
-     * @param robotOutput
-     * @param robotCatenateParentKeywords
-     * @param build
      */
     public static void sendResults(final FilePath workspace, String companyId, boolean usingonpremise, String onpremiseurl, String apiKey, String projectKey, String milestone,
                                    String testRunTitle, String comment, String testTargetTitle, String testEnvironmentTitle, String tags,
@@ -117,7 +86,7 @@ public static void sendResults(final FilePath workspace, String companyId, boole
         for(Action a : build.getAllActions()) {
             if(log.isLoggable(Level.FINE))
                 log.fine("Action: " + a);
-            if (hasTAPSupport && a instanceof TapTestResultAction) {
+            if (hasTAPSupport && a instanceof org.tap4j.plugin.TapTestResultAction) {
                 ras.add(a);
             } else if (a instanceof AbstractTestResultAction) {
                 ras.add(a);
@@ -187,7 +156,7 @@ public static void sendResults(final FilePath workspace, String companyId, boole
 
             for(Object ra : ras) {
                 Object resultObject = null;
-                if(ra instanceof TapTestResultAction) {
+                if(hasTAPSupport && ra instanceof org.tap4j.plugin.TapTestResultAction) {
                     try {
                         // due to 2.1 change in tap plugin, try to keep compatibility to tap plugin < 2.1
                         Method m = ra.getClass().getMethod("getResult");
@@ -294,7 +263,7 @@ else if(cr.isSkipped())
                 org.tap4j.plugin.model.TapStreamResult tsr = (org.tap4j.plugin.model.TapStreamResult)result;
 
                 for(TestResult tr : tsr.getChildren()) {
-                    TapTestResultResult r = (TapTestResultResult)tr;
+                    org.tap4j.plugin.model.TapTestResultResult r = (org.tap4j.plugin.model.TapTestResultResult)tr;
 
                     // see https://testanything.org/tap-specification.html
 

diff --git a/src/main/java/fi/meliora/testlab/ext/jenkins/TestlabNotifier.java b/src/main/java/fi/meliora/testlab/ext/jenkins/TestlabNotifier.java
@@ -4,6 +4,7 @@
 import hudson.model.*;
 import hudson.tasks.*;
 import hudson.util.PluginServletFilter;
+import hudson.util.Secret;
 import net.sf.json.JSONObject;
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.StaplerRequest;
@@ -137,9 +138,9 @@ public String getCompanyId() {
     }
 
     // job specific apikey of target testlab, optional
-    private String apiKey;
+    private Secret apiKey;
 
-    public String getApiKey() {
+    public Secret getApiKey() {
         return apiKey;
     }
 
@@ -298,7 +299,6 @@ public TestlabNotifier(String projectKey, String testRunTitle, String comment, S
      * When {@link hudson.tasks.Publisher} behaves this way, note that they can no longer
      * change the build status anymore.
      *
-     * @author Kohsuke Kawaguchi
      * @since 1.153
      */
     @Override
@@ -329,7 +329,7 @@ public BuildStepMonitor getRequiredMonitorService() {
      * @param build
      * @param launcher
      * @param listener
-     * @return
+     * @return boolean
      * @throws InterruptedException
      * @throws IOException
      */
@@ -342,7 +342,12 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
         log.fine("perform(): " + this + ", descriptor: " + d);
 
         // get job specific settings if any and fallback to global configuration
-        String runApiKey = isBlank(apiKey) ? d.getApiKey() : apiKey;
+        Secret secretKey = apiKey;
+        if(secretKey == null || "".equals(secretKey.getPlainText())) {
+            // prefer key from global settings if the job has none
+            secretKey = d.getApiKey();
+        }
+        String runApiKey = secretKey.getPlainText();
         String runTestCaseMappingField = isBlank(testCaseMappingField) ? d.getTestCaseMappingField() : testCaseMappingField;
 
         Usingonpremise uop = advancedSettings != null && advancedSettings.getUsingonpremise() != null
@@ -503,7 +508,7 @@ public static final class DescriptorImpl extends BuildStepDescriptor<Publisher>
         // company id of the testlab which to publish to
         private String companyId;
         // testlab api key
-        private String apiKey;
+        private Secret apiKey;
         // custom field name to map the test ids against with
         private String testCaseMappingField;
         // if set, on-premise variant of Testlab is used and Testlab URL should be set and honored
@@ -551,7 +556,7 @@ public boolean isApplicable(Class type) {
         public boolean configure(StaplerRequest staplerRequest, JSONObject json) throws Descriptor.FormException {
             // persist configuration
             companyId = json.getString("companyId");
-            apiKey = json.getString("apiKey");
+            apiKey = Secret.fromString(json.getString("apiKey"));
             testCaseMappingField = json.getString("testCaseMappingField");
 
             JSONObject uop = json.getJSONObject("usingonpremise");
@@ -596,7 +601,7 @@ public String getCompanyId() {
             return companyId;
         }
 
-        public String getApiKey() {
+        public Secret getApiKey() {
             return apiKey;
         }
 
@@ -640,9 +645,9 @@ public String getCompanyId() {
         }
 
         // job specific apikey of target testlab, optional
-        private String apiKey;
+        private Secret apiKey;
 
-        public String getApiKey() {
+        public Secret getApiKey() {
             return apiKey;
         }
 
@@ -661,7 +666,7 @@ public Usingonpremise getUsingonpremise() {
         }
 
         @DataBoundConstructor
-        public AdvancedSettings(String companyId, String apiKey, String testCaseMappingField, Usingonpremise usingonpremise) {
+        public AdvancedSettings(String companyId, Secret apiKey, String testCaseMappingField, Usingonpremise usingonpremise) {
             this.companyId = companyId;
             this.apiKey = apiKey;
             this.testCaseMappingField = testCaseMappingField;

diff --git a/src/main/java/fi/meliora/testlab/ext/jenkins/VariableReplacer.java b/src/main/java/fi/meliora/testlab/ext/jenkins/VariableReplacer.java
@@ -18,7 +18,7 @@ public class VariableReplacer {
     /**
      * Creates a new variable replacer by combining key-value maps provided.
      *
-     * @param vars
+     * @param vars variables
      */
     @SuppressWarnings("unchecked")
     public VariableReplacer(Map<String, String>... vars) {
@@ -38,8 +38,8 @@ public VariableReplacer(Map<String, String>... vars) {
      * Replaces all tags in format ${BUILD_NUMBER} with matching value from envVars.
      * If variable is missing, tag is left as it is.
      *
-     * @param src
-     * @return
+     * @param src source String
+     * @return String with variables replaced
      */
     public String replace(String src) {
         if(vars == null || src == null || src.length() == 0)