From 5adfab9f8a21841a55828c2c8a2830a0cbc2f386 Mon Sep 17 00:00:00 2001 From: prabathabey Date: Tue, 26 Jul 2016 23:37:42 +0530 Subject: [PATCH] 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 --- ...CertificateManagementAdminServiceImpl.java | 18 ++++---- .../dao/CertificateManagementDAOFactory.java | 37 +++++++++++++--- .../dao/impl/GenericCertificateDAOImpl.java | 26 ++++++------ .../mgt/core/impl/CertificateGenerator.java | 4 ++ .../core/impl/CertificateGeneratorTests.java | 42 +++++++++++++++++++ .../mgt/core/impl/KeyGeneratorTests.java | 25 +++++++++++ .../src/test/resources/testng.xml | 2 + .../impl/DeviceInformationManagerImpl.java | 2 +- .../src/main/resources/dbscripts/cdm/h2.sql | 8 ++-- 9 files changed, 132 insertions(+), 32 deletions(-) create mode 100644 components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/java/org/wso2/carbon/certificate/mgt/core/impl/CertificateGeneratorTests.java create mode 100644 components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/java/org/wso2/carbon/certificate/mgt/core/impl/KeyGeneratorTests.java diff --git a/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 b/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 index 6d98253f83..d9d074c70b 100644 --- a/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 +++ b/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( - new ErrorResponse.ErrorResponseBuilder().setCode(500l).setMessage(msg).build()); + return Response.serverError().entity( + 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( - new ErrorResponse.ErrorResponseBuilder().setCode(500l).setMessage(msg).build()); + return Response.serverError().entity( + 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( - new ErrorResponse.ErrorResponseBuilder().setCode(500l).setMessage(msg).build()); + return Response.serverError().entity( + 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( - new ErrorResponse.ErrorResponseBuilder().setCode(500l).setMessage(msg).build()); + return Response.serverError().entity( + new ErrorResponse.ErrorResponseBuilder().setMessage(msg).build()).build(); } } + } diff --git a/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/dao/CertificateManagementDAOFactory.java b/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/dao/CertificateManagementDAOFactory.java index 02345c127b..b87a4af218 100644 --- a/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/dao/CertificateManagementDAOFactory.java +++ b/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/dao/CertificateManagementDAOFactory.java @@ -38,11 +38,16 @@ public class CertificateManagementDAOFactory { private static DataSource dataSource; private static String databaseEngine; private static final Log log = LogFactory.getLog(CertificateManagementDAOFactory.class); - private static ThreadLocal currentConnection = new ThreadLocal(); + private static ThreadLocal currentConnection = new ThreadLocal<>(); + private static ThreadLocal currentTxState = new ThreadLocal<>(); + + private enum TxState { + CONNECTION_NOT_BORROWED, CONNECTION_BORROWED, CONNECTION_CLOSED + } public static CertificateDAO getCertificateDAO() { - return new GenericCertificateDAOImpl(); + return new GenericCertificateDAOImpl(); } public static void init(DataSourceConfig config) { @@ -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"); } - conn = dataSource.getConnection(); + 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(); } @@ -170,14 +195,14 @@ public class CertificateManagementDAOFactory { if (config == null) { throw new RuntimeException( "Device Management Repository data source configuration " + "is null and " + - "thus, is not initialized" + "thus, is not initialized" ); } JNDILookupDefinition jndiConfig = config.getJndiLookupDefinition(); if (jndiConfig != null) { if (log.isDebugEnabled()) { log.debug("Initializing Device Management Repository data source using the JNDI " + - "Lookup Definition"); + "Lookup Definition"); } List jndiPropertyList = jndiConfig.getJndiProperties(); diff --git a/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/dao/impl/GenericCertificateDAOImpl.java b/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/dao/impl/GenericCertificateDAOImpl.java index 9d1129d067..13543d66b6 100644 --- a/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/dao/impl/GenericCertificateDAOImpl.java +++ b/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")); @@ -193,11 +194,11 @@ public class GenericCertificateDAOImpl implements CertificateDAO { paginationResult.setData(certificates); paginationResult.setRecordsTotal(resultCount); } catch (SQLException e) { - String errorMsg = "SQL error occurred while retrieving the certificates."; + String errorMsg = "SQL error occurred while retrieving the certificates."; 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")); @@ -227,11 +228,11 @@ public class GenericCertificateDAOImpl implements CertificateDAO { certificates.add(certificateResponse); } } catch (SQLException e) { - String errorMsg = "SQL error occurred while retrieving the certificates."; + String errorMsg = "SQL error occurred while retrieving the certificates."; log.error(errorMsg, e); throw new CertificateManagementDAOException(errorMsg, e); } finally { - OperationManagementDAOUtil.cleanupResources(stmt, resultSet); + CertificateManagementDAOUtil.cleanupResources(stmt, resultSet); } return certificates; } @@ -246,17 +247,16 @@ public class GenericCertificateDAOImpl implements CertificateDAO { conn = this.getConnection(); String query = "DELETE FROM DM_DEVICE_CERTIFICATE WHERE SERIAL_NUMBER = ?" + - " AND TENANT_ID = ? "; + " AND TENANT_ID = ? "; stmt = conn.prepareStatement(query); stmt.setString(1, serialNumber); stmt.setInt(2, tenantId); return stmt.executeUpdate() > 0; } catch (SQLException e) { - String errorMsg = - "Unable to get the read the certificate with serial" + serialNumber; - log.error(errorMsg, e); - throw new CertificateManagementDAOException(errorMsg, e); + String msg = "Unable to get the read the certificate with serial" + serialNumber; + log.error(msg, e); + throw new CertificateManagementDAOException(msg, e); } finally { CertificateManagementDAOUtil.cleanupResources(stmt, resultSet); } diff --git a/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/impl/CertificateGenerator.java b/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/impl/CertificateGenerator.java index e4332c599f..92bf568dba 100755 --- a/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/main/java/org/wso2/carbon/certificate/mgt/core/impl/CertificateGenerator.java +++ b/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; diff --git a/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/java/org/wso2/carbon/certificate/mgt/core/impl/CertificateGeneratorTests.java b/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/java/org/wso2/carbon/certificate/mgt/core/impl/CertificateGeneratorTests.java new file mode 100644 index 0000000000..b7e8283ce4 --- /dev/null +++ b/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(); + } + } + +} diff --git a/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/java/org/wso2/carbon/certificate/mgt/core/impl/KeyGeneratorTests.java b/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/java/org/wso2/carbon/certificate/mgt/core/impl/KeyGeneratorTests.java new file mode 100644 index 0000000000..c23043d38f --- /dev/null +++ b/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 { + + + +} diff --git a/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/resources/testng.xml b/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/resources/testng.xml index 8d91ced59f..d05fefdb8a 100644 --- a/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/resources/testng.xml +++ b/components/certificate-mgt/org.wso2.carbon.certificate.mgt.core/src/test/resources/testng.xml @@ -5,6 +5,8 @@ + + \ No newline at end of file diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/device/details/mgt/impl/DeviceInformationManagerImpl.java b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/device/details/mgt/impl/DeviceInformationManagerImpl.java index 62fcd3bf82..6e0cc1cf9d 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/device/details/mgt/impl/DeviceInformationManagerImpl.java +++ b/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 { diff --git a/features/device-mgt/org.wso2.carbon.device.mgt.server.feature/src/main/resources/dbscripts/cdm/h2.sql b/features/device-mgt/org.wso2.carbon.device.mgt.server.feature/src/main/resources/dbscripts/cdm/h2.sql index e9894b7997..f3f3735e50 100644 --- a/features/device-mgt/org.wso2.carbon.device.mgt.server.feature/src/main/resources/dbscripts/cdm/h2.sql +++ b/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 ) - REFERENCES DM_DEVICE_TYPE (ID) ON DELETE NO ACTION ON UPDATE NO ACTION + 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, + 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 (