Improving connection handling in cert management related functionalities, adding UNIQUE key constraints to DM_DEVICE and DM_ENROLMENT tables, improving response handling of certificate management admin API

components/certificate-mgt/org.wso2.carbon.certificate.mgt.cert.admin.api/src/main/java/org/wso2/carbon/certificate/mgt/cert/jaxrs/api/impl/CertificateManagementAdminServiceImpl.java

@@ -3,7 +3,6 @@ package org.wso2.carbon.certificate.mgt.cert.jaxrs.api.impl;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.wso2.carbon.certificate.mgt.cert.jaxrs.api.CertificateManagementAdminService;
-import org.wso2.carbon.certificate.mgt.cert.jaxrs.api.UnexpectedServerErrorException;
 import org.wso2.carbon.certificate.mgt.cert.jaxrs.api.beans.CertificateList;
 import org.wso2.carbon.certificate.mgt.cert.jaxrs.api.beans.EnrollmentCertificate;
 import org.wso2.carbon.certificate.mgt.cert.jaxrs.api.beans.ErrorResponse;
@@ -53,8 +52,8 @@ public class CertificateManagementAdminServiceImpl implements CertificateManagem
         } catch (KeystoreException e) {
             String msg = "Error occurred while converting PEM file to X509Certificate.";
             log.error(msg, e);
-            throw new UnexpectedServerErrorException(
+            return Response.serverError().entity(
-                    new ErrorResponse.ErrorResponseBuilder().setCode(500l).setMessage(msg).build());
+                    new ErrorResponse.ErrorResponseBuilder().setCode(500l).setMessage(msg).build()).build();
         }
     }
 
@@ -79,8 +78,8 @@ public class CertificateManagementAdminServiceImpl implements CertificateManagem
         } catch (CertificateManagementException e) {
             String msg = "Error occurred while converting PEM file to X509Certificate";
             log.error(msg, e);
-            throw new UnexpectedServerErrorException(
+            return Response.serverError().entity(
-                    new ErrorResponse.ErrorResponseBuilder().setCode(500l).setMessage(msg).build());
+                    new ErrorResponse.ErrorResponseBuilder().setCode(500l).setMessage(msg).build()).build();
         }
     }
 
@@ -109,8 +108,8 @@ public class CertificateManagementAdminServiceImpl implements CertificateManagem
         } catch (CertificateManagementException e) {
             String msg = "Error occurred while fetching all certificates.";
             log.error(msg, e);
-            throw new UnexpectedServerErrorException(
+            return Response.serverError().entity(
-                    new ErrorResponse.ErrorResponseBuilder().setCode(500l).setMessage(msg).build());
+                    new ErrorResponse.ErrorResponseBuilder().setMessage(msg).build()).build();
         }
     }
 
@@ -131,8 +130,9 @@ public class CertificateManagementAdminServiceImpl implements CertificateManagem
         } catch (CertificateManagementException e) {
             String msg = "Error occurred while converting PEM file to X509Certificate";
             log.error(msg, e);
-            throw new UnexpectedServerErrorException(
+            return Response.serverError().entity(
-                    new ErrorResponse.ErrorResponseBuilder().setCode(500l).setMessage(msg).build());
+                    new ErrorResponse.ErrorResponseBuilder().setMessage(msg).build()).build();
         }
     }
+
 }

components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/dao/CertificateManagementDAOFactory.java

@@ -38,7 +38,12 @@ public class CertificateManagementDAOFactory {
     private static DataSource dataSource;
     private static String databaseEngine;
     private static final Log log = LogFactory.getLog(CertificateManagementDAOFactory.class);
-    private static ThreadLocal<Connection> currentConnection = new ThreadLocal<Connection>();
+    private static ThreadLocal<Connection> currentConnection = new ThreadLocal<>();
+    private static ThreadLocal<TxState> currentTxState = new ThreadLocal<>();
+
+    private enum TxState {
+        CONNECTION_NOT_BORROWED, CONNECTION_BORROWED, CONNECTION_CLOSED
+    }
 
 
     public static CertificateDAO getCertificateDAO() {
@@ -50,7 +55,7 @@ public class CertificateManagementDAOFactory {
         try {
             databaseEngine = dataSource.getConnection().getMetaData().getDatabaseProductName();
         } catch (SQLException e) {
-            log.error(  "Error occurred while retrieving config.datasource connection", e);
+            log.error("Error occurred while retrieving config.datasource connection", e);
         }
     }
 
@@ -85,9 +90,11 @@ public class CertificateManagementDAOFactory {
                 log.warn("Error occurred while closing the borrowed connection. " +
                         "Transaction has ended pre-maturely", e1);
             }
+            currentTxState.set(TxState.CONNECTION_CLOSED);
             throw new TransactionManagementException("Error occurred while setting auto-commit to false", e);
         }
         currentConnection.set(conn);
+        currentTxState.set(TxState.CONNECTION_BORROWED);
     }
 
     public static void openConnection() throws SQLException {
@@ -97,8 +104,14 @@ public class CertificateManagementDAOFactory {
                     "this particular thread. Therefore, calling 'beginTransaction/openConnection' while another " +
                     "transaction is already active is a sign of improper transaction handling");
         }
+        try {
             conn = dataSource.getConnection();
+        } catch (SQLException e) {
+            currentTxState.set(TxState.CONNECTION_NOT_BORROWED);
+            throw e;
+        }
         currentConnection.set(conn);
+        currentTxState.set(TxState.CONNECTION_BORROWED);
     }
 
     public static Connection getConnection() throws SQLException {
@@ -144,6 +157,17 @@ public class CertificateManagementDAOFactory {
     }
 
     public static void closeConnection() {
+        TxState txState = currentTxState.get();
+        if (TxState.CONNECTION_NOT_BORROWED == txState) {
+            if (log.isDebugEnabled()) {
+                log.debug("No successful connection appears to have been borrowed to perform the underlying " +
+                        "transaction even though the 'openConnection' method has been called. Therefore, " +
+                        "'closeConnection' method is returning silently");
+            }
+            currentTxState.remove();
+            return;
+        }
+
         Connection conn = currentConnection.get();
         if (conn == null) {
             throw new IllegalTransactionStateException("No connection is associated with the current transaction. " +
@@ -156,6 +180,7 @@ public class CertificateManagementDAOFactory {
             log.warn("Error occurred while close the connection", e);
         }
         currentConnection.remove();
+        currentTxState.remove();
     }
 
 

components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/dao/impl/GenericCertificateDAOImpl.java

@@ -45,6 +45,7 @@ import java.util.ArrayList;
 import java.util.List;
 
 public class GenericCertificateDAOImpl implements CertificateDAO {
+
     private static final Log log = LogFactory.getLog(GenericCertificateDAOImpl.class);
 
     @Override
@@ -103,7 +104,7 @@ public class GenericCertificateDAOImpl implements CertificateDAO {
 
             if (resultSet.next()) {
                 certificateResponse = new CertificateResponse();
-                byte [] certificateBytes = resultSet.getBytes("CERTIFICATE");
+                byte[] certificateBytes = resultSet.getBytes("CERTIFICATE");
                 certificateResponse.setCertificate(certificateBytes);
                 certificateResponse.setSerialNumber(resultSet.getString("SERIAL_NUMBER"));
                 certificateResponse.setTenantId(resultSet.getInt("TENANT_ID"));
@@ -142,7 +143,7 @@ public class GenericCertificateDAOImpl implements CertificateDAO {
 
             while (resultSet.next()) {
                 certificateResponse = new CertificateResponse();
-                byte [] certificateBytes = resultSet.getBytes("CERTIFICATE");
+                byte[] certificateBytes = resultSet.getBytes("CERTIFICATE");
                 certificateResponse.setSerialNumber(resultSet.getString("SERIAL_NUMBER"));
                 certificateResponse.setTenantId(resultSet.getInt("TENANT_ID"));
                 certificateResponse.setUsername(resultSet.getString("USERNAME"));
@@ -181,7 +182,7 @@ public class GenericCertificateDAOImpl implements CertificateDAO {
             int resultCount = 0;
             while (resultSet.next()) {
                 certificateResponse = new CertificateResponse();
-                byte [] certificateBytes = resultSet.getBytes("CERTIFICATE");
+                byte[] certificateBytes = resultSet.getBytes("CERTIFICATE");
                 certificateResponse.setSerialNumber(resultSet.getString("SERIAL_NUMBER"));
                 certificateResponse.setTenantId(resultSet.getInt("TENANT_ID"));
                 certificateResponse.setUsername(resultSet.getString("USERNAME"));
@@ -197,7 +198,7 @@ public class GenericCertificateDAOImpl implements CertificateDAO {
             log.error(errorMsg, e);
             throw new CertificateManagementDAOException(errorMsg, e);
         } finally {
-            OperationManagementDAOUtil.cleanupResources(stmt, resultSet);
+            CertificateManagementDAOUtil.cleanupResources(stmt, resultSet);
         }
         return paginationResult;
     }
@@ -219,7 +220,7 @@ public class GenericCertificateDAOImpl implements CertificateDAO {
 
             while (resultSet.next()) {
                 certificateResponse = new CertificateResponse();
-                byte [] certificateBytes = resultSet.getBytes("CERTIFICATE");
+                byte[] certificateBytes = resultSet.getBytes("CERTIFICATE");
                 certificateResponse.setSerialNumber(resultSet.getString("SERIAL_NUMBER"));
                 certificateResponse.setTenantId(resultSet.getInt("TENANT_ID"));
                 certificateResponse.setUsername(resultSet.getString("USERNAME"));
@@ -231,7 +232,7 @@ public class GenericCertificateDAOImpl implements CertificateDAO {
             log.error(errorMsg, e);
             throw new CertificateManagementDAOException(errorMsg, e);
         } finally {
-            OperationManagementDAOUtil.cleanupResources(stmt, resultSet);
+            CertificateManagementDAOUtil.cleanupResources(stmt, resultSet);
         }
         return certificates;
     }
@@ -253,10 +254,9 @@ public class GenericCertificateDAOImpl implements CertificateDAO {
 
             return stmt.executeUpdate() > 0;
         } catch (SQLException e) {
-            String errorMsg =
+            String msg = "Unable to get the read the certificate with serial" + serialNumber;
-                    "Unable to get the read the certificate with serial" + serialNumber;
+            log.error(msg, e);
-            log.error(errorMsg, e);
+            throw new CertificateManagementDAOException(msg, e);
-            throw new CertificateManagementDAOException(errorMsg, e);
         } finally {
             CertificateManagementDAOUtil.cleanupResources(stmt, resultSet);
         }

components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/impl/CertificateGenerator.java

@@ -308,6 +308,10 @@ public class CertificateGenerator {
     }
 
     public CertificateResponse verifyPEMSignature(X509Certificate requestCertificate) throws KeystoreException {
+        if (requestCertificate == null) {
+            throw new IllegalArgumentException("Certificate of which the signature needs to be validated cannot " +
+                    "be null");
+        }
         KeyStoreReader keyStoreReader = new KeyStoreReader();
         CertificateResponse lookUpCertificate;
 

components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/java/org/wso2/carbon/certificate/mgt/core/impl/CertificateGeneratorTests.java

@@ -0,0 +1,42 @@
+/*
+ *   Copyright (c) 2016, WSO2 Inc. (http://www.wso2.org) All Rights Reserved.
+ *
+ *   WSO2 Inc. licenses this file to you under the Apache License,
+ *   Version 2.0 (the "License"); you may not use this file except
+ *   in compliance with the License.
+ *   You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *   Unless required by applicable law or agreed to in writing,
+ *   software distributed under the License is distributed on an
+ *   "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *   KIND, either express or implied.  See the License for the
+ *   specific language governing permissions and limitations
+ *   under the License.
+ *
+ */
+package org.wso2.carbon.certificate.mgt.core.impl;
+
+import junit.framework.Assert;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.testng.annotations.Test;
+import org.wso2.carbon.certificate.mgt.core.exception.KeystoreException;
+
+public class CertificateGeneratorTests {
+
+    private static final Log log = LogFactory.getLog(CertificateGeneratorTests.class);
+
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    public void testVerifyNullPEMSignature() {
+        CertificateGenerator certGenerator = new CertificateGenerator();
+        try {
+            certGenerator.verifyPEMSignature(null);
+        } catch (KeystoreException e) {
+            log.error("Error occurred while verifying PEM signature", e);
+            Assert.fail();
+        }
+    }
+
+}

components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/java/org/wso2/carbon/certificate/mgt/core/impl/KeyGeneratorTests.java

@@ -0,0 +1,25 @@
+/*
+ *   Copyright (c) 2016, WSO2 Inc. (http://www.wso2.org) All Rights Reserved.
+ *
+ *   WSO2 Inc. licenses this file to you under the Apache License,
+ *   Version 2.0 (the "License"); you may not use this file except
+ *   in compliance with the License.
+ *   You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *   Unless required by applicable law or agreed to in writing,
+ *   software distributed under the License is distributed on an
+ *   "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *   KIND, either express or implied.  See the License for the
+ *   specific language governing permissions and limitations
+ *   under the License.
+ *
+ */
+package org.wso2.carbon.certificate.mgt.core.impl;
+
+public class KeyGeneratorTests {
+
+
+
+}

components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/resources/testng.xml

@@ -5,6 +5,8 @@
         <classes>
             <class name="org.wso2.carbon.certificate.mgt.core.impl.CertificateGeneratorTestSuite"/>
             <class name="org.wso2.carbon.certificate.mgt.core.util.CommonUtil"/>
+            <class name="org.wso2.carbon.certificate.mgt.core.impl.CertificateGeneratorTests"/>
+            <class name="org.wso2.carbon.certificate.mgt.core.impl.KeyGeneratorTests"/>
         </classes>
     </test>
 </suite>

components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/device/details/mgt/impl/DeviceInformationManagerImpl.java

@@ -162,7 +162,6 @@ public class DeviceInformationManagerImpl implements DeviceInformationManager {
             deviceDetailsDAO.addDeviceLocation(deviceLocation);
             DeviceManagementDAOFactory.commitTransaction();
         } catch (TransactionManagementException e) {
-            DeviceManagementDAOFactory.rollbackTransaction();
             throw new DeviceDetailsMgtException("Transactional error occurred while adding the device location " +
                     "information.", e);
         } catch (DeviceDetailsMgtDAOException e) {
@@ -172,6 +171,7 @@ public class DeviceInformationManagerImpl implements DeviceInformationManager {
             DeviceManagementDAOFactory.rollbackTransaction();
             throw new DeviceDetailsMgtException("Error occurred while getting the device information.", e);
         } catch (DeviceManagementDAOException e) {
+            DeviceManagementDAOFactory.rollbackTransaction();
             throw new DeviceDetailsMgtException("Error occurred while updating the last updated timestamp of " +
                     "the device", e);
         } finally {

features/device-mgt/org.wso2.carbon.device.mgt.server.feature/src/main/resources/dbscripts/cdm/h2.sql

@@ -26,8 +26,9 @@ CREATE TABLE IF NOT EXISTS DM_DEVICE (
      LAST_UPDATED_TIMESTAMP TIMESTAMP NOT NULL,
      TENANT_ID INTEGER DEFAULT 0,
      PRIMARY KEY (ID),
-     CONSTRAINT fk_DM_DEVICE_DM_DEVICE_TYPE2 FOREIGN KEY (DEVICE_TYPE_ID )
+     CONSTRAINT fk_DM_DEVICE_DM_DEVICE_TYPE2 FOREIGN KEY (DEVICE_TYPE_ID)
-     REFERENCES DM_DEVICE_TYPE (ID) ON DELETE NO ACTION ON UPDATE NO ACTION
+     REFERENCES DM_DEVICE_TYPE (ID) ON DELETE NO ACTION ON UPDATE NO ACTION,
+     CONSTRAINT uk_DM_DEVICE UNIQUE (NAME, DEVICE_TYPE_ID, DEVICE_IDENTIFICATION, TENANT_ID)
 );
 
 CREATE TABLE IF NOT EXISTS DM_DEVICE_GROUP_MAP (
@@ -96,7 +97,8 @@ CREATE TABLE IF NOT EXISTS DM_ENROLMENT (
     TENANT_ID INT NOT NULL,
     PRIMARY KEY (ID),
     CONSTRAINT fk_dm_device_enrolment FOREIGN KEY (DEVICE_ID) REFERENCES
-    DM_DEVICE (ID) ON DELETE NO ACTION ON UPDATE NO ACTION
+    DM_DEVICE (ID) ON DELETE NO ACTION ON UPDATE NO ACTION,
+    CONSTRAINT uk_dm_device_enrolment UNIQUE (DEVICE_ID, OWNER, OWNERSHIP, TENANT_ID)
 );
 
 CREATE TABLE IF NOT EXISTS DM_ENROLMENT_OP_MAPPING (