fixed issue 29157: code review issues for Process Definition Reporting Tool
authorAsier Lostalé <asier.lostale@openbravo.com>
Fri, 06 Mar 2015 08:51:40 +0100
changeset 26135 d8ec9347e127
parent 26134 4ee0aae2bc57
child 26136 95f9c5ce6207
fixed issue 29157: code review issues for Process Definition Reporting Tool

Fixes include:
* Security: prevent traversal attack. BaseReportActionHandler could be invoked
to download any file in the system. Fixed by:
- Now it only accepts file name instead of full path, looking for this file
in the temporary directory.
- Filename is parsed to ensure it is a valid generated jasper file name,
preventing in this manner downloads of any arbitrary file in the temporary
directory.
* ReportSemaphoreHandling changes:
- Modified to make use of standard java.util.concurrent.Semaphore
implementation rather than implementing its own semaphore.
- Property to read maximum number of concurrent executions is read on
initialization instead of when acquiring. This way acquisition is faster.
* When a Jasper report is generated with a virtualizer, it's finally cleaned
up.
* When downloading a report, temporary file is deleted on a finally block to
ensure deletion even on failure.
* Changes in javadoc to fix some typos + prevent undocumented parameters.
* Defensive coding: when generating/downloading a report, don't assume if type
is not pdf then it is xls, but do check all the types and raise an exception
in case of unsupported type.
* UI: in process definition window, don't show Can Add Records flag for process
definitions of type report
modules/org.openbravo.client.application/src-db/database/sourcedata/AD_FIELD.xml
modules/org.openbravo.client.application/src/org/openbravo/client/application/report/BaseReportActionHandler.java
modules/org.openbravo.client.application/src/org/openbravo/client/application/report/ReportSemaphoreHandling.java
modules/org.openbravo.client.application/src/org/openbravo/client/application/report/ReportingUtils.java
modules/org.openbravo.client.application/web/org.openbravo.client.application/js/utilities/ob-utilities-action-def.js
--- a/modules/org.openbravo.client.application/src-db/database/sourcedata/AD_FIELD.xml	Wed Mar 04 17:07:47 2015 +0100
+++ b/modules/org.openbravo.client.application/src-db/database/sourcedata/AD_FIELD.xml	Fri Mar 06 08:51:40 2015 +0100
@@ -5077,6 +5077,7 @@
 <!--F33844ACDC3A40ABAA83E614E659FC8F-->  <AD_COLUMN_ID><![CDATA[8C5CC70F805F41B9BE4999E2A5E70C26]]></AD_COLUMN_ID>
 <!--F33844ACDC3A40ABAA83E614E659FC8F-->  <IGNOREINWAD><![CDATA[N]]></IGNOREINWAD>
 <!--F33844ACDC3A40ABAA83E614E659FC8F-->  <ISDISPLAYED><![CDATA[Y]]></ISDISPLAYED>
+<!--F33844ACDC3A40ABAA83E614E659FC8F-->  <DISPLAYLOGIC><![CDATA[@Uipattern@='OBUIAPP_PickAndExecute']]></DISPLAYLOGIC>
 <!--F33844ACDC3A40ABAA83E614E659FC8F-->  <DISPLAYLENGTH><![CDATA[1]]></DISPLAYLENGTH>
 <!--F33844ACDC3A40ABAA83E614E659FC8F-->  <ISREADONLY><![CDATA[N]]></ISREADONLY>
 <!--F33844ACDC3A40ABAA83E614E659FC8F-->  <SEQNO><![CDATA[160]]></SEQNO>
--- a/modules/org.openbravo.client.application/src/org/openbravo/client/application/report/BaseReportActionHandler.java	Wed Mar 04 17:07:47 2015 +0100
+++ b/modules/org.openbravo.client.application/src/org/openbravo/client/application/report/BaseReportActionHandler.java	Fri Mar 06 08:51:40 2015 +0100
@@ -68,13 +68,18 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+/**
+ * Action Handler used as base for jasper reports generated from process defition. This handler can
+ * be extended to customize its behavior.
+ *
+ */
 public class BaseReportActionHandler extends BaseProcessActionHandler {
   private static final Logger log = LoggerFactory.getLogger(BaseReportActionHandler.class);
   private static final String JASPER_PARAM_PROCESS = "jasper_process";
 
   /**
-   * execute() method overridden to add the logic to download the report file stored in the temporal
-   * folder.
+   * execute() method overridden to add the logic to download the report file stored in the
+   * temporary folder.
    */
   @Override
   public void execute() {
@@ -144,49 +149,58 @@
   }
 
   /**
-   * Downloads the file with the report result. The file is stored in a temporal folder with a
+   * Downloads the file with the report result. The file is stored in a temporary folder with a
    * generated name. It is renamed and download as an attachment of the response. Once it is
    * finished the file is removed from the server.
-   * 
-   * @param request
-   * @param parameters
-   *          Map with the needed parameters to download the report.
-   * @throws IOException
    */
   private void doDownload(HttpServletRequest request, Map<String, Object> parameters)
       throws IOException {
     final String strFileName = (String) parameters.get("fileName");
-    final String strFilePath = (String) parameters.get("filePath");
+    final String tmpFileName = (String) parameters.get("tmpfileName");
     ExportType expType = null;
-    if (strFileName.endsWith("pdf")) {
+
+    if (strFileName.endsWith("." + ExportType.PDF.getExtension())) {
       expType = ExportType.PDF;
+    } else if (strFileName.endsWith("." + ExportType.XLS.getExtension())) {
+      expType = ExportType.XLS;
     } else {
-      expType = ExportType.XLS;
+      throw new IllegalArgumentException("Trying to download report file with unsupported type "
+          + strFileName);
     }
 
-    FileUtility fileUtil = new FileUtility();
-    final File file = new File(strFilePath);
-    fileUtil = new FileUtility(file.getParent(), file.getName(), false, true);
-
-    final HttpServletResponse response = RequestContext.get().getResponse();
-
-    response.setHeader("Content-Type", expType.getContentType());
-    response.setContentType(expType.getContentType());
-    response.setCharacterEncoding("UTF-8");
-    // TODO: Compatibility code with IE8. To be reviewed when its support is stopped.
-    String userAgent = request.getHeader("user-agent");
-    if (userAgent.contains("MSIE")) {
-      response.setHeader("Content-Disposition",
-          "attachment; filename=\"" + URLEncoder.encode(strFileName, "utf-8") + "\"");
-    } else {
-      response.setHeader("Content-Disposition",
-          "attachment; filename=\"" + MimeUtility.encodeWord(strFileName, "utf-8", "Q") + "\"");
+    if (!expType.isValidTemporaryFileName(tmpFileName)) {
+      throw new IllegalArgumentException("Trying to download report with invalid name "
+          + strFileName);
     }
 
-    fileUtil.dumpFile(response.getOutputStream());
-    response.getOutputStream().flush();
-    response.getOutputStream().close();
-    file.delete();
+    final String tmpDirectory = ReportingUtils.getTempFolder();
+    final File file = new File(tmpDirectory, tmpFileName);
+    FileUtility fileUtil = new FileUtility(tmpDirectory, tmpFileName, false, true);
+    try {
+      final HttpServletResponse response = RequestContext.get().getResponse();
+
+      response.setHeader("Content-Type", expType.getContentType());
+      response.setContentType(expType.getContentType());
+      response.setCharacterEncoding("UTF-8");
+      // TODO: Compatibility code with IE8. To be reviewed when its support is stopped.
+      // see issue #29109
+      String userAgent = request.getHeader("user-agent");
+      if (userAgent.contains("MSIE")) {
+        response.setHeader("Content-Disposition",
+            "attachment; filename=\"" + URLEncoder.encode(strFileName, "utf-8") + "\"");
+      } else {
+        response.setHeader("Content-Disposition",
+            "attachment; filename=\"" + MimeUtility.encodeWord(strFileName, "utf-8", "Q") + "\"");
+      }
+
+      fileUtil.dumpFile(response.getOutputStream());
+      response.getOutputStream().flush();
+      response.getOutputStream().close();
+    } finally {
+      if (file.exists()) {
+        file.delete();
+      }
+    }
   }
 
   /**
@@ -201,7 +215,6 @@
    *          JSONObject with the values set in the filter parameters.
    * @param action
    *          String with the output type of the report.
-   * @throws JSONException
    * @throws OBException
    *           Exception thrown when a validation fails.
    */
@@ -225,6 +238,9 @@
       }
     case PDF:
       strJRPath = report.getPDFTemplate();
+      break;
+    default:
+      throw new OBException(OBMessageUtils.messageBD("OBUIAPP_UnsupportedAction"));
     }
     if (StringUtils.isEmpty(strJRPath)) {
       throw new OBException(OBMessageUtils.messageBD("OBUIAPP_NoJRTemplateFound"));
@@ -236,7 +252,7 @@
     loadReportParams(jrParams, report, jrTemplatePath, jsonContent);
     log.debug("Report: {}. Start export JR process.", report.getId());
     long t1 = System.currentTimeMillis();
-    String strFilePath = doJRExport(jrTemplatePath, expType, jrParams, strTmpFileName);
+    doJRExport(jrTemplatePath, expType, jrParams, strTmpFileName);
     log.debug("Report: {}. Finish export JR process. Elapsed time: {}", report.getId(),
         System.currentTimeMillis() - t1);
 
@@ -245,7 +261,7 @@
     params.put("reportId", parameters.get("reportId"));
     params.put("actionHandler", this.getClass().getName());
     recordInfo.put("processParameters", params);
-    recordInfo.put("filePath", strFilePath);
+    recordInfo.put("tmpfileName", strTmpFileName);
     recordInfo.put("fileName", strFileName);
 
     final JSONObject reportAction = new JSONObject();
@@ -280,7 +296,6 @@
    *          the Report Definition.
    * @param params
    *          JSONObject with the values set in the filter parameters.
-   * @throws JSONException
    */
   private void loadFilterParams(HashMap<String, Object> jrParams, ReportDefinition report,
       JSONObject params) throws JSONException {
@@ -429,6 +444,7 @@
    * @param process
    *          the Process Definition of the Report
    * @param jsonContent
+   *          values set in the filter parameters
    * @param parameters
    *          the current Parameter Map that it is send to the Jasper Report.
    */
@@ -436,13 +452,13 @@
       Map<String, Object> parameters) {
   }
 
-  private static String doJRExport(String jrTemplatePath, ExportType expType,
+  private static void doJRExport(String jrTemplatePath, ExportType expType,
       Map<String, Object> parameters, String strFileName) {
-    ReportSemaphoreHandling.acquire();
+    ReportSemaphoreHandling.getInstance().acquire();
     try {
-      return ReportingUtils.exportJR(jrTemplatePath, expType, parameters, strFileName);
+      ReportingUtils.exportJR(jrTemplatePath, expType, parameters, strFileName);
     } finally {
-      ReportSemaphoreHandling.release();
+      ReportSemaphoreHandling.getInstance().release();
     }
   }
 }
--- a/modules/org.openbravo.client.application/src/org/openbravo/client/application/report/ReportSemaphoreHandling.java	Wed Mar 04 17:07:47 2015 +0100
+++ b/modules/org.openbravo.client.application/src/org/openbravo/client/application/report/ReportSemaphoreHandling.java	Fri Mar 06 08:51:40 2015 +0100
@@ -18,6 +18,8 @@
  */
 package org.openbravo.client.application.report;
 
+import java.util.concurrent.Semaphore;
+
 import org.openbravo.base.exception.OBException;
 import org.openbravo.base.provider.OBSingleton;
 import org.openbravo.erpCommon.businessUtility.Preferences;
@@ -34,16 +36,18 @@
  * This semaphore handler can/should be used by heavy resource intensive reporting processes.To
  * prevent too many to run at the same time.
  * 
+ * Implementation is based on {@link java.util.concurrent.Semaphore}.
+ * 
  * The {@link #acquire()} and {@link #release()} methods should be called using a try finally block:
  * 
  * <pre>
  * <code>
  * // call acquire before the try:
- * ReportSemaphoreHandling.acquire();
+ * ReportSemaphoreHandling.getInstance().acquire();
  * try {
  * 
  * } finally {
- *   ReportSemaphoreHandling.release();
+ *   ReportSemaphoreHandling.getInstance().release();
  * }
  * </code>
  * </pre>
@@ -52,19 +56,21 @@
  */
 public class ReportSemaphoreHandling implements OBSingleton {
   private static int DEFAULT_MAX_THREADS = 3;
-  private static int maxThreads = 3;
-  private static int threadCounter = 0;
+  private static int maxThreads;
+
   private static final Logger log = LoggerFactory.getLogger(ReportSemaphoreHandling.class);
 
-  /**
-   * Increments the threadCounter by one unit. The Max value of the counter can be parameterized by
-   * setting the OBUIAPP_MaxReportThreads System preference. If no preference is found or it is
-   * wrongly configured DEFAULT_MAX_THREADS default value is used.
-   * 
-   * @throws OBException
-   *           When the threadCounter is in its max value.
-   */
-  public static synchronized void acquire() throws OBException {
+  private static ReportSemaphoreHandling instance;
+  private Semaphore semaphore;
+
+  public static synchronized ReportSemaphoreHandling getInstance() {
+    if (instance == null) {
+      instance = new ReportSemaphoreHandling();
+    }
+    return instance;
+  }
+
+  private ReportSemaphoreHandling() {
     String value = "";
     try {
       // strNull needed to avoid ambiguous definition of getPreferenceValue() method. That can
@@ -85,24 +91,29 @@
       log.warn("The value of OBUIAPP_MaxReportThreads property is not a valid number {}.", value, e);
       maxThreads = DEFAULT_MAX_THREADS;
     }
-    if (threadCounter == maxThreads) {
+
+    semaphore = new Semaphore(maxThreads);
+  }
+
+  /**
+   * Increments the threadCounter by one unit. The Max value of the counter can be parameterized by
+   * setting the OBUIAPP_MaxReportThreads System preference. If no preference is found or it is
+   * wrongly configured DEFAULT_MAX_THREADS default value is used.
+   * 
+   * @throws OBException
+   *           When the threadCounter is in its max value.
+   */
+  public void acquire() throws OBException {
+    boolean acquired = semaphore.tryAcquire();
+
+    if (!acquired) {
       log.error("All available threads ({}) occupied.", maxThreads);
       throw new OBException(OBMessageUtils.messageBD("OBUIAPP_ReportProcessOccupied"));
     }
-    threadCounter++;
   }
 
-  /**
-   * Decreases the threadCounter by one unit.
-   * 
-   * @throws OBException
-   *           In case the current value is zero an Exception is thrown. This can only happen by a
-   *           wrong implementation of the semaphore.
-   */
-  public static synchronized void release() throws OBException {
-    if (threadCounter == 0) {
-      throw new OBException("Illegal thread counter, decreasing below zero");
-    }
-    threadCounter--;
+  /** Decreases the threadCounter by one unit. */
+  public void release() {
+    semaphore.release();
   }
 }
--- a/modules/org.openbravo.client.application/src/org/openbravo/client/application/report/ReportingUtils.java	Wed Mar 04 17:07:47 2015 +0100
+++ b/modules/org.openbravo.client.application/src/org/openbravo/client/application/report/ReportingUtils.java	Fri Mar 06 08:51:40 2015 +0100
@@ -18,6 +18,7 @@
  */
 package org.openbravo.client.application.report;
 
+import java.io.File;
 import java.text.DecimalFormat;
 import java.text.DecimalFormatSymbols;
 import java.util.HashMap;
@@ -51,6 +52,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+/** Utilities to generate jasper reports */
 public class ReportingUtils {
   public static final String JASPER_PARAM_OBCONTEXT = "jasper_obContext";
   public static final String JASPER_PARAM_HBSESSION = "jasper_hbSession";
@@ -67,13 +69,14 @@
    *          The parameters to be sent to Jasper Report.
    * @param strFileName
    *          The name to be used on the generated file.
-   * @return a String with the complete path of the generated file.
    * @throws OBException
    *           In case there is any error generating the file an exception is thrown with the error
    *           message.
    */
-  public static String exportJR(String jasperFilePath, ExportType expType,
+  public static void exportJR(String jasperFilePath, ExportType expType,
       Map<String, Object> parameters, String strFileName) throws OBException {
+
+    JRSwapFileVirtualizer virtualizer = null;
     try {
       parameters.put(JASPER_PARAM_HBSESSION, OBDal.getInstance().getSession());
       parameters.put(JASPER_PARAM_OBCONTEXT, OBContext.getOBContext());
@@ -125,7 +128,6 @@
       }
       parameters.put("Readable_Organizations", strOrgs);
 
-      JRSwapFileVirtualizer virtualizer = null;
       // if no custom virtualizer is requested use a default one
       if (!parameters.containsKey(JRParameter.REPORT_VIRTUALIZER)) {
         // virtualizer is essentially using a tmp-file to avoid huge memory consumption by jasper
@@ -159,14 +161,11 @@
       } else {
         jasperPrint = JasperFillManager.fillReport(jasperFilePath, parameters);
       }
-      String target = getTempFolder();
-      if (!target.endsWith("/")) {
-        target += "/";
-      }
-      target += strFileName;
+      File target = new File(getTempFolder(), strFileName);
+
       switch (expType) {
       case PDF:
-        JasperExportManager.exportReportToPdfFile(jasperPrint, target);
+        JasperExportManager.exportReportToPdfFile(jasperPrint, target.getAbsolutePath());
         break;
       case XLS:
         JExcelApiExporter exporter = new JExcelApiExporter();
@@ -181,11 +180,14 @@
         exporter.exportReport();
         break;
       }
-
-      return target;
     } catch (JRException e) {
       log.error("Error generating Jasper Report: " + jasperFilePath, e);
       throw new OBException(e.getMessage(), e);
+    } finally {
+      // remove virtualizer tmp files if we created them
+      if (virtualizer != null) {
+        virtualizer.cleanup();
+      }
     }
   }
 
@@ -213,11 +215,13 @@
    * is generated.
    */
   public enum ExportType {
+    @SuppressWarnings("serial")
     PDF("pdf", "103", new HashMap<String, Object>() {
       {
         put("IS_IGNORE_PAGINATION", false);
       }
     }), //
+    @SuppressWarnings("serial")
     XLS("xls", "101", new HashMap<String, Object>() {
       {
         put("IS_IGNORE_PAGINATION", true);
@@ -257,9 +261,23 @@
         throw new OBException(OBMessageUtils.messageBD("OBUIAPP_UnsupportedAction"));
       }
     }
+
+    /** Checks if temporary file name is a valid one: has extension and the name is a uuid */
+    public boolean isValidTemporaryFileName(String tmpFileName) {
+      if (!tmpFileName.endsWith("." + getExtension())) {
+        // file name should end with the extension
+        return false;
+      }
+      final String tmpFileNameWithoutExtension = tmpFileName.substring(0, tmpFileName.length()
+          - getExtension().length() - 1);
+
+      // temp file must be a valid uuid
+      return tmpFileNameWithoutExtension.matches("[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}");
+    }
   }
 
-  private static String getTempFolder() {
+  /** Returns temporary directory to save generated reports */
+  public static String getTempFolder() {
     final String tmpFolder = System.getProperty("java.io.tmpdir");
 
     return tmpFolder;
--- a/modules/org.openbravo.client.application/web/org.openbravo.client.application/js/utilities/ob-utilities-action-def.js	Wed Mar 04 17:07:47 2015 +0100
+++ b/modules/org.openbravo.client.application/web/org.openbravo.client.application/js/utilities/ob-utilities-action-def.js	Fri Mar 06 08:51:40 2015 +0100
@@ -150,7 +150,7 @@
 //changed to DOWNLOAD so the BaseReportActionHanlder execute the logic to download the report.
 //Parameters:
 //* {{{processParameters}}}: The process parameters is an object that includes the action handler implementing the download, the report id that it is being executed and the process definition id.
-//* {{{filePath}}}: The complete path of the temporary file.
+//* {{{tmpfileName}}}: Name of the temporary file.
 //* {{{fileName}}}: The name to be used in the file to download.
 OB.Utilities.Action.set('OBUIAPP_downloadReport', function (paramObj) {
   var processParameters = paramObj.processParameters,
@@ -158,7 +158,7 @@
   params._action = processParameters.actionHandler;
   params.reportId = processParameters.reportId;
   params.processId = processParameters.processId;
-  params.filePath = paramObj.filePath;
+  params.tmpfileName = paramObj.tmpfileName;
   params.fileName = paramObj.fileName;
   params.mode = 'DOWNLOAD';
   OB.Utilities.postThroughHiddenForm(OB.Application.contextUrl + 'org.openbravo.client.kernel', params);