From 101cd40f36541587b6da7a524f26db124826d683 Mon Sep 17 00:00:00 2001 From: rusirijayodaillesinghe Date: Thu, 30 May 2024 10:15:31 +0530 Subject: [PATCH] Add unit tests, modify existing tests and bug fixes identified at manual tests --- .../security/APIAuthenticationHandler.java | 1 - .../authenticator/MutualSSLAuthenticator.java | 19 ++++++++------ .../CertificateManagerImplTest.java | 19 ++++++++------ .../impl/dao/test/CertificateMgtDaoTest.java | 25 ++++++++++++++++--- .../src/test/resources/dbscripts/h2.sql | 1 + .../src/main/resources/sql/h2.sql | 1 + .../src/main/resources/sql/mysql.sql | 1 + .../src/main/resources/sql/mysql_cluster.sql | 1 + .../src/main/resources/sql/h2.sql | 1 + 9 files changed, 51 insertions(+), 18 deletions(-) diff --git a/components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/APIAuthenticationHandler.java b/components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/APIAuthenticationHandler.java index 95b5ba332b05..4676a5b6b075 100644 --- a/components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/APIAuthenticationHandler.java +++ b/components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/APIAuthenticationHandler.java @@ -544,7 +544,6 @@ private void handleNoAuthentication(MessageContext messageContext){ authContext.setStopOnQuotaReach(true); //Requests are throttled by the ApiKey that is set here. In an unauthenticated scenario, we will use the client's IP address for throttling. authContext.setApiKey(clientIP); - //TODO: verify the key type authContext.setKeyType(APIConstants.API_KEY_TYPE_PRODUCTION); //This name is hardcoded as anonymous because there is no associated user token authContext.setUsername(APIConstants.END_USER_ANONYMOUS); diff --git a/components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/authenticator/MutualSSLAuthenticator.java b/components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/authenticator/MutualSSLAuthenticator.java index f4a0fcd9e6c3..c6a05fb6b946 100644 --- a/components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/authenticator/MutualSSLAuthenticator.java +++ b/components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/authenticator/MutualSSLAuthenticator.java @@ -71,10 +71,10 @@ public class MutualSSLAuthenticator implements Authenticator { public MutualSSLAuthenticator(String apiLevelPolicy, boolean isMandatory, String certificateDetails) { this.apiLevelPolicy = apiLevelPolicy; certificates = new HashMap<>(); - HashMap certificateData = new HashMap<>(); if (StringUtils.isNotEmpty(certificateDetails)) { String[] certificateParts = certificateDetails.substring(1, certificateDetails.length() - 1).split(","); for (String certificatePart : certificateParts) { + HashMap certificateData = new HashMap<>(); int tierDivisionIndex = certificatePart.lastIndexOf("="); if (tierDivisionIndex > 0) { String uniqueIdentifier = certificatePart.substring(0, tierDivisionIndex).trim(); @@ -181,6 +181,12 @@ private void setAuthContext(MessageContext messageContext, Certificate certifica String subjectDN = x509Certificate.getSubjectDN().getName(); String uniqueIdentifier = (x509Certificate.getSerialNumber() + "_" + x509Certificate.getSubjectDN()).replaceAll(",", "#").replaceAll("\"", "'").trim(); + /* Since there can be previously deleted certificates persisted in the trust store that matches with the + certificate object but not in the certificates list for the particular API. + */ + if (certificates.get(uniqueIdentifier) == null ) { + handleCertificateNotAssociatedToAPIFailure(messageContext); + } String tier = certificates.get(uniqueIdentifier).get(APIConstants.CLIENT_CERTIFICATE_TIER); String keyType = certificates.get(uniqueIdentifier).get(APIConstants.CLIENT_CERTIFICATE_KEY_TYPE); if (StringUtils.isEmpty(tier)) { @@ -236,13 +242,10 @@ private void setAuthContext(MessageContext messageContext, Certificate[] certifi } if (StringUtils.isEmpty(tier) || StringUtils.isEmpty(keyType)) { subjectDNIdentifiers = getUniqueIdentifierFromCompleteCertificateChain(x509Certificates, subjectDNIdentifiers); - } - if (StringUtils.isEmpty(tier)) { tier = getTierFromCompleteCertificateChain(subjectDNIdentifiers); - } - if (StringUtils.isEmpty(keyType)) { keyType = getKeyTypeFromCompleteCertificateChain(subjectDNIdentifiers); } + if (StringUtils.isEmpty(tier) || StringUtils.isEmpty(keyType)) { handleCertificateNotAssociatedToAPIFailure(messageContext); } @@ -292,7 +295,8 @@ private String getTierFromCompleteCertificateChain(List uniqueIdentifier String tier = null; for (String uniqueIdentifier : uniqueIdentifiers) { - tier = certificates.get(uniqueIdentifier).get(APIConstants.CLIENT_CERTIFICATE_TIER); + tier = certificates.get(uniqueIdentifier) == null ? null : + certificates.get(uniqueIdentifier).get(APIConstants.CLIENT_CERTIFICATE_TIER); if (StringUtils.isNotEmpty(tier)) { break; } @@ -311,7 +315,8 @@ private String getKeyTypeFromCompleteCertificateChain(List uniqueIdentif String keyType = null; for (String uniqueIdentifier : uniqueIdentifiers) { - keyType = certificates.get(uniqueIdentifier).get(APIConstants.CLIENT_CERTIFICATE_KEY_TYPE); + keyType = certificates.get(uniqueIdentifier) == null ? null : + certificates.get(uniqueIdentifier).get(APIConstants.CLIENT_CERTIFICATE_KEY_TYPE); if (StringUtils.isNotEmpty(keyType)) { break; } diff --git a/components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/certificatemgt/CertificateManagerImplTest.java b/components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/certificatemgt/CertificateManagerImplTest.java index 5c7ef946fed1..0e344573486d 100644 --- a/components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/certificatemgt/CertificateManagerImplTest.java +++ b/components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/certificatemgt/CertificateManagerImplTest.java @@ -30,6 +30,7 @@ import org.wso2.carbon.apimgt.api.dto.CertificateInformationDTO; import org.wso2.carbon.apimgt.api.dto.CertificateMetadataDTO; import org.wso2.carbon.apimgt.api.dto.ClientCertificateDTO; +import org.wso2.carbon.apimgt.impl.APIConstants; import org.wso2.carbon.apimgt.impl.TestUtils; import org.wso2.carbon.apimgt.impl.certificatemgt.exceptions.CertificateAliasExistsException; import org.wso2.carbon.apimgt.impl.certificatemgt.exceptions.CertificateManagementException; @@ -421,7 +422,8 @@ public void testAddClientCertificate() throws CertificateManagementException { .stub(PowerMockito.method(CertificateMgtUtils.class, "validateCertificate")) .toReturn(ResponseCode.ALIAS_EXISTS_IN_TRUST_STORE); ResponseCode responseCode = certificateManager - .addClientCertificate(null, BASE64_ENCODED_CERT, ALIAS, null, MultitenantConstants.SUPER_TENANT_ID, "org1"); + .addClientCertificate(null, BASE64_ENCODED_CERT, ALIAS, null, + APIConstants.API_KEY_TYPE_PRODUCTION, MultitenantConstants.SUPER_TENANT_ID, "org1"); Assert.assertEquals("Response code was wrong while trying add a client certificate with an existing alias", ResponseCode.ALIAS_EXISTS_IN_TRUST_STORE.getResponseCode(), responseCode.getResponseCode()); PowerMockito @@ -429,15 +431,17 @@ public void testAddClientCertificate() throws CertificateManagementException { .toReturn(ResponseCode.SUCCESS); Mockito.when(certificateMgtDAO.checkWhetherAliasExist(ALIAS, TENANT_ID)).thenReturn(true); responseCode = certificateManager - .addClientCertificate(null, BASE64_ENCODED_CERT, ALIAS, null, MultitenantConstants.SUPER_TENANT_ID, "org1"); + .addClientCertificate(null, BASE64_ENCODED_CERT, ALIAS, null, + APIConstants.API_KEY_TYPE_PRODUCTION, MultitenantConstants.SUPER_TENANT_ID, "org1"); Assert.assertEquals("Response code was wrong while trying add a client certificate with an existing alias", ResponseCode.ALIAS_EXISTS_IN_TRUST_STORE.getResponseCode(), responseCode.getResponseCode()); Mockito.when(certificateMgtDAO.checkWhetherAliasExist(ALIAS, TENANT_ID)).thenReturn(false); Mockito.when(certificateMgtDAO .addClientCertificate(Mockito.anyString(), Mockito.any(), Mockito.anyString(), Mockito.anyString(), - Mockito.anyInt(), Mockito.anyString())).thenReturn(true); + Mockito.anyString(), Mockito.anyInt(), Mockito.anyString())).thenReturn(true); responseCode = certificateManager - .addClientCertificate(null, BASE64_ENCODED_CERT, ALIAS, null, MultitenantConstants.SUPER_TENANT_ID, "org1"); + .addClientCertificate(null, BASE64_ENCODED_CERT, ALIAS, null, + APIConstants.API_KEY_TYPE_PRODUCTION, MultitenantConstants.SUPER_TENANT_ID, "org1"); Assert.assertEquals("Response code was wrong while trying add a client certificate", ResponseCode.SUCCESS.getResponseCode(), responseCode.getResponseCode()); } @@ -452,8 +456,8 @@ public void testUpdateClientCertificate() throws APIManagementException { .stub(PowerMockito.method(CertificateMgtUtils.class, "validateCertificate")) .toReturn(ResponseCode.CERTIFICATE_EXPIRED); ResponseCode responseCode = certificateManager - .updateClientCertificate(BASE64_ENCODED_CERT, ALIAS, null, MultitenantConstants.SUPER_TENANT_ID, - organization); + .updateClientCertificate(BASE64_ENCODED_CERT, ALIAS, null, + APIConstants.API_KEY_TYPE_PRODUCTION, MultitenantConstants.SUPER_TENANT_ID, organization); Assert.assertEquals("Response code was wrong while trying add a expired client certificate", ResponseCode.CERTIFICATE_EXPIRED.getResponseCode(), responseCode.getResponseCode()); PowerMockito @@ -461,7 +465,8 @@ public void testUpdateClientCertificate() throws APIManagementException { .toReturn(ResponseCode.SUCCESS); PowerMockito.stub(PowerMockito.method(CertificateMgtDAO.class, "updateClientCertificate")).toReturn(false); responseCode = certificateManager - .updateClientCertificate(BASE64_ENCODED_CERT, ALIAS, null, MultitenantConstants.SUPER_TENANT_ID, + .updateClientCertificate(BASE64_ENCODED_CERT, ALIAS, null, APIConstants.API_KEY_TYPE_PRODUCTION, + MultitenantConstants.SUPER_TENANT_ID, organization); Assert.assertEquals("Response code was wrong, for a failure in update", ResponseCode.INTERNAL_SERVER_ERROR.getResponseCode(), responseCode.getResponseCode()); diff --git a/components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/dao/test/CertificateMgtDaoTest.java b/components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/dao/test/CertificateMgtDaoTest.java index 786e9c43adec..9855e4bceeeb 100644 --- a/components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/dao/test/CertificateMgtDaoTest.java +++ b/components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/dao/test/CertificateMgtDaoTest.java @@ -31,6 +31,7 @@ import org.wso2.carbon.apimgt.api.dto.CertificateMetadataDTO; import org.wso2.carbon.apimgt.api.dto.ClientCertificateDTO; import org.wso2.carbon.apimgt.api.model.APIIdentifier; +import org.wso2.carbon.apimgt.impl.APIConstants; import org.wso2.carbon.apimgt.impl.APIManagerConfiguration; import org.wso2.carbon.apimgt.impl.APIManagerConfigurationServiceImpl; import org.wso2.carbon.apimgt.impl.certificatemgt.exceptions.CertificateAliasExistsException; @@ -208,7 +209,21 @@ public void testAddClientCertificate() throws CertificateManagementException { @Test public void testUpdateClientCertificateOfNonExistingAlias() throws CertificateManagementException { Assert.assertFalse("Update of client certificate for a non existing alias succeeded", - certificateMgtDAO.updateClientCertificate(certificate, "test1", "test", TENANT_ID, "org1")); + certificateMgtDAO.updateClientCertificate(certificate, "test1", "test", + APIConstants.API_KEY_TYPE_PRODUCTION, TENANT_ID, "org1")); + } + + /** + * This method tests the behaviour of updateClientCertificate method when trying to update keyType of an + * existing client certificate entry. + * + * @throws CertificateManagementException Certificate Management Exception. + */ + @Test + public void testUpdateKeyTypeOfExistingAlias() throws CertificateManagementException { + Assert.assertFalse("Update of key type for an existing client certificate entry succeeded", + certificateMgtDAO.updateClientCertificate(certificate, "test1", "test", + APIConstants.API_KEY_TYPE_SANDBOX, TENANT_ID, "org1")); } /** @@ -221,7 +236,8 @@ public void testUpdateClientCertificateOfExistingAlias() throws CertificateManag try { addClientCertificate(); Assert.assertTrue("Update of client certificate for an existing alias failed", - certificateMgtDAO.updateClientCertificate(null, "test", "test", TENANT_ID, "org1")); + certificateMgtDAO.updateClientCertificate(null, "test", "test", + APIConstants.API_KEY_TYPE_PRODUCTION, TENANT_ID, "org1")); } finally { deleteClientCertificate(); } @@ -293,6 +309,8 @@ public void testGetClientCertificates() throws CertificateManagementException { clientCertificateDTOS = certificateMgtDAO.getClientCertificates(TENANT_ID, "test", apiIdentifier, organization); Assert.assertEquals("The client certificate DTO list that matches the search criteria is not returned", 1, clientCertificateDTOS.size()); + Assert.assertNotNull("The key type of a client certificate cannot be null", + clientCertificateDTOS.get(0).getKeyType()); clientCertificateDTOS = certificateMgtDAO.getClientCertificates(TENANT_ID, null, apiIdentifier, organization); Assert.assertEquals("The client certificate DTO list that matches the search criteria is not returned", 1, clientCertificateDTOS.size()); @@ -324,7 +342,8 @@ public void testGetDeletedClientCertificates() throws CertificateManagementExcep * @throws CertificateManagementException Certificate Management Exception. */ private boolean addClientCertificate() throws CertificateManagementException { - return certificateMgtDAO.addClientCertificate(certificate, apiIdentifier, "test", "Gold", TENANT_ID, "org1"); + return certificateMgtDAO.addClientCertificate(certificate, apiIdentifier, "test", "Gold", + APIConstants.API_KEY_TYPE_PRODUCTION, TENANT_ID, "org1"); } /** diff --git a/components/apimgt/org.wso2.carbon.apimgt.keymgt/src/test/resources/dbscripts/h2.sql b/components/apimgt/org.wso2.carbon.apimgt.keymgt/src/test/resources/dbscripts/h2.sql index 1c6cf07d5aae..6ba0ae41c324 100644 --- a/components/apimgt/org.wso2.carbon.apimgt.keymgt/src/test/resources/dbscripts/h2.sql +++ b/components/apimgt/org.wso2.carbon.apimgt.keymgt/src/test/resources/dbscripts/h2.sql @@ -1715,6 +1715,7 @@ CREATE TABLE IF NOT EXISTS `AM_API_CLIENT_CERTIFICATE` ( `CERTIFICATE` BLOB NOT NULL, `REMOVED` BOOLEAN NOT NULL DEFAULT 0, `TIER_NAME` VARCHAR (512), + `KEY_TYPE` VARCHAR(20) NOT NULL DEFAULT 'PRODUCTION', FOREIGN KEY (API_ID) REFERENCES AM_API (API_ID) ON DELETE CASCADE ON UPDATE CASCADE, PRIMARY KEY (`ALIAS`,`TENANT_ID`, `REMOVED`) ); diff --git a/features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql b/features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql index ec4d4d9cb886..98402e27a516 100644 --- a/features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql +++ b/features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql @@ -2014,6 +2014,7 @@ CREATE TABLE IF NOT EXISTS AM_API_CLIENT_CERTIFICATE ( CERTIFICATE BLOB NOT NULL, REMOVED BOOLEAN NOT NULL DEFAULT 0, TIER_NAME VARCHAR (512), + KEY_TYPE VARCHAR(20) NOT NULL DEFAULT 'PRODUCTION', REVISION_UUID VARCHAR(255) NOT NULL DEFAULT 'Current API', FOREIGN KEY (API_ID) REFERENCES AM_API (API_ID) ON DELETE CASCADE ON UPDATE CASCADE, PRIMARY KEY (ALIAS,TENANT_ID, REMOVED, REVISION_UUID) diff --git a/features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql b/features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql index 9638f78a38e5..931dd31cacbf 100644 --- a/features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql +++ b/features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql @@ -1961,6 +1961,7 @@ CREATE TABLE IF NOT EXISTS `AM_API_CLIENT_CERTIFICATE` ( `CERTIFICATE` BLOB NOT NULL, `REMOVED` BOOLEAN NOT NULL DEFAULT 0, `TIER_NAME` VARCHAR (512), + `KEY_TYPE` VARCHAR(20) NOT NULL DEFAULT 'PRODUCTION', `REVISION_UUID` VARCHAR(255) NOT NULL DEFAULT 'Current API', FOREIGN KEY (API_ID) REFERENCES AM_API (API_ID) ON DELETE CASCADE ON UPDATE CASCADE, PRIMARY KEY (`ALIAS`, `TENANT_ID`, `REMOVED`, `REVISION_UUID`) diff --git a/features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sql b/features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sql index 64f9a447abe1..fcf364b58b4c 100644 --- a/features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sql +++ b/features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sql @@ -2141,6 +2141,7 @@ CREATE TABLE IF NOT EXISTS `AM_API_CLIENT_CERTIFICATE` ( `CERTIFICATE` BLOB NOT NULL, `REMOVED` BOOLEAN NOT NULL DEFAULT 0, `TIER_NAME` VARCHAR (512), + `KEY_TYPE` VARCHAR(20) NOT NULL DEFAULT 'PRODUCTION', `REVISION_UUID` VARCHAR(255) NOT NULL DEFAULT 'Current API', FOREIGN KEY (API_ID) REFERENCES AM_API (API_ID), PRIMARY KEY (`ALIAS`, `TENANT_ID`, `REMOVED`, `REVISION_UUID`) diff --git a/features/apimgt/org.wso2.carbon.apimgt.keymanager.feature/src/main/resources/sql/h2.sql b/features/apimgt/org.wso2.carbon.apimgt.keymanager.feature/src/main/resources/sql/h2.sql index ec96d9913a98..ea536e316628 100644 --- a/features/apimgt/org.wso2.carbon.apimgt.keymanager.feature/src/main/resources/sql/h2.sql +++ b/features/apimgt/org.wso2.carbon.apimgt.keymanager.feature/src/main/resources/sql/h2.sql @@ -1701,6 +1701,7 @@ CREATE TABLE IF NOT EXISTS `AM_API_CLIENT_CERTIFICATE` ( `CERTIFICATE` BLOB NOT NULL, `REMOVED` BOOLEAN NOT NULL DEFAULT 0, `TIER_NAME` VARCHAR (512), + `KEY_TYPE` VARCHAR(20) NOT NULL DEFAULT 'PRODUCTION', FOREIGN KEY (API_ID) REFERENCES AM_API (API_ID) ON DELETE CASCADE ON UPDATE CASCADE, PRIMARY KEY (`ALIAS`,`TENANT_ID`, `REMOVED`) );