diff --git a/components/idp-mgt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/dao/IdPManagementFacade.java b/components/idp-mgt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/dao/IdPManagementFacade.java index 8d342de4299d..1269670d01bd 100644 --- a/components/idp-mgt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/dao/IdPManagementFacade.java +++ b/components/idp-mgt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/dao/IdPManagementFacade.java @@ -19,6 +19,8 @@ package org.wso2.carbon.idp.mgt.dao; import org.apache.commons.lang.StringUtils; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.wso2.carbon.identity.application.common.model.*; import org.wso2.carbon.identity.base.AuthenticatorPropertyConstants; import org.wso2.carbon.identity.core.model.ExpressionNode; @@ -27,10 +29,10 @@ import org.wso2.carbon.idp.mgt.IdentityProviderManagementException; import org.wso2.carbon.idp.mgt.IdentityProviderManagementServerException; import org.wso2.carbon.idp.mgt.model.ConnectedAppsResult; +import org.wso2.carbon.idp.mgt.util.IdPManagementConstants; import org.wso2.carbon.idp.mgt.util.UserDefinedAuthenticatorEndpointConfigManager; import java.sql.Connection; -import java.sql.SQLException; import java.util.List; import java.util.Map; import java.util.Set; @@ -40,6 +42,7 @@ public class IdPManagementFacade { private final IdPManagementDAO dao; private final UserDefinedAuthenticatorEndpointConfigManager endpointConfigurationManager = new UserDefinedAuthenticatorEndpointConfigManager(); + private static final Log LOG = LogFactory.getLog(IdPManagementFacade.class); public IdPManagementFacade(IdPManagementDAO dao) { @@ -183,12 +186,12 @@ public void deleteIdP(String idPName, int tenantId, String tenantDomain) throws IdentityProviderManagementException { IdentityProvider identityProvider = getIdPByName(null, idPName, tenantId, tenantDomain); - deleteEndpointConfig(identityProvider, tenantDomain); + dao.deleteIdP(idPName, tenantId, tenantDomain); try { - dao.deleteIdP(idPName, tenantId, tenantDomain); + deleteEndpointConfig(identityProvider, tenantDomain); } catch (IdentityProviderManagementException e) { - addEndpointConfig(identityProvider, tenantDomain); - throw e; + // Error will not be thrown, since the IDP is already deleted. But there will be stale action. + LOG.warn(String.format(IdPManagementConstants.WarningMessage.WARN_STALE_IDP_ACTION, idPName)); } } @@ -197,16 +200,14 @@ public void deleteIdPs(int tenantId) throws IdentityProviderManagementException // TODO: Replace loops with batch operations once issue:https://github.com/wso2/product-is/issues/21783 is done. List idpList = getIdPs(null, tenantId, IdentityTenantUtil.getTenantDomain(tenantId)); - for (IdentityProvider idp : idpList) { - deleteEndpointConfig(idp, IdentityTenantUtil.getTenantDomain(tenantId)); - } + dao.deleteIdPs(tenantId); try { - dao.deleteIdPs(tenantId); - } catch (IdentityProviderManagementException e) { for (IdentityProvider idp : idpList) { - addEndpointConfig(idp, IdentityTenantUtil.getTenantDomain(tenantId)); + deleteEndpointConfig(idp, IdentityTenantUtil.getTenantDomain(tenantId)); } - throw e; + } catch (IdentityProviderManagementException e) { + // Error will not be thrown, since the IDPs is already deleted. But there will be stale actions. + LOG.warn(IdPManagementConstants.WarningMessage.WARN_STALE_IDP_ACTIONS); } } @@ -214,12 +215,14 @@ public void deleteIdPByResourceId(String resourceId, int tenantId, String tenant throws IdentityProviderManagementException { IdentityProvider identityProvider = getIDPbyResourceId(null, resourceId, tenantId, tenantDomain); - deleteEndpointConfig(identityProvider, tenantDomain); + dao.deleteIdPByResourceId(resourceId, tenantId, tenantDomain); try { - dao.deleteIdPByResourceId(identityProvider.getResourceId(), tenantId, tenantDomain); + + deleteEndpointConfig(identityProvider, tenantDomain); } catch (IdentityProviderManagementException e) { - addEndpointConfig(identityProvider, tenantDomain); - throw e; + // Error will not be thrown, since the IDP is already deleted. But there will be stale action. + LOG.warn(String.format(IdPManagementConstants.WarningMessage.WARN_STALE_IDP_ACTION, + identityProvider.getIdentityProviderName())); } } @@ -227,12 +230,12 @@ public void forceDeleteIdP(String idPName, int tenantId, String tenantDomain) throws IdentityProviderManagementException { IdentityProvider identityProvider = getIdPByName(null, idPName, tenantId, tenantDomain); - deleteEndpointConfig(identityProvider, tenantDomain); + dao.forceDeleteIdP(idPName, tenantId, tenantDomain); try { - dao.forceDeleteIdP(idPName, tenantId, tenantDomain); + deleteEndpointConfig(identityProvider, tenantDomain); } catch (IdentityProviderManagementException e) { - addEndpointConfig(identityProvider, tenantDomain); - throw e; + // Error will not be thrown, since the IDP is already deleted. But there will be stale action. + LOG.warn(String.format(IdPManagementConstants.WarningMessage.WARN_STALE_IDP_ACTION, idPName)); } } @@ -240,12 +243,13 @@ public void forceDeleteIdPByResourceId(String resourceId, int tenantId, String t throws IdentityProviderManagementException { IdentityProvider identityProvider = getIDPbyResourceId(null, resourceId, tenantId, tenantDomain); - deleteEndpointConfig(identityProvider, tenantDomain); + dao.forceDeleteIdPByResourceId(resourceId, tenantId, tenantDomain); try { - dao.forceDeleteIdPByResourceId(resourceId, tenantId, tenantDomain); + deleteEndpointConfig(identityProvider, tenantDomain); } catch (IdentityProviderManagementException e) { - addEndpointConfig(identityProvider, tenantDomain); - throw e; + // Error will not be thrown, since the IDP is already deleted. But there will be stale action. + LOG.warn(String.format(IdPManagementConstants.WarningMessage.WARN_STALE_IDP_ACTION, + identityProvider.getIdentityProviderName())); } } diff --git a/components/idp-mgt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/util/IdPManagementConstants.java b/components/idp-mgt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/util/IdPManagementConstants.java index 06dd1ad5a73d..9debd70dcce6 100644 --- a/components/idp-mgt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/util/IdPManagementConstants.java +++ b/components/idp-mgt/org.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/util/IdPManagementConstants.java @@ -604,6 +604,14 @@ public static class SQLQueries { "SELECT * FROM IDP_AUTHENTICATOR WHERE TENANT_ID = ? AND DEFINED_BY = 'USER'"; } + public static class WarningMessage { + + public static final String WARN_STALE_IDP_ACTION = "The %s Identity Provider was deleted, but an error " + + "occurred while deleting its associated action."; + public static final String WARN_STALE_IDP_ACTIONS = "The Identity Providers was deleted, but an error " + + "occurred while deleting their associated actions."; + } + public enum ErrorMessage { // Client Errors. diff --git a/components/idp-mgt/org.wso2.carbon.idp.mgt/src/test/java/org/wso2/carbon/idp/mgt/IdentityProviderManagementServiceTest.java b/components/idp-mgt/org.wso2.carbon.idp.mgt/src/test/java/org/wso2/carbon/idp/mgt/IdentityProviderManagementServiceTest.java index 143c0b95204c..489890e8cf5e 100644 --- a/components/idp-mgt/org.wso2.carbon.idp.mgt/src/test/java/org/wso2/carbon/idp/mgt/IdentityProviderManagementServiceTest.java +++ b/components/idp-mgt/org.wso2.carbon.idp.mgt/src/test/java/org/wso2/carbon/idp/mgt/IdentityProviderManagementServiceTest.java @@ -629,9 +629,8 @@ public void testDeleteIdPActionException() throws Exception { when(actionManagementServiceForException.getActionByActionId(anyString(), any(), any())).thenReturn(action); IdpMgtServiceComponentHolder.getInstance().setActionManagementService(actionManagementServiceForException); - assertThrows(IdentityProviderManagementException.class, () -> - identityProviderManagementService.deleteIdP(userDefinedIdP.getIdentityProviderName())); - Assert.assertNotNull(identityProviderManagementService.getIdPByName(userDefinedIdP + identityProviderManagementService.deleteIdP(userDefinedIdP.getIdentityProviderName()); + Assert.assertNull(identityProviderManagementService.getIdPByName(userDefinedIdP .getIdentityProviderName())); } @@ -1226,9 +1225,9 @@ public void testDeleteIdPDAOException() throws Exception { assertThrows(IdentityProviderManagementException.class, () -> identityProviderManagementService.deleteIdP(userDefinedIdP.getIdentityProviderName())); - /* check ActionManagementService actionManagementService.deleteAction() is called. Two time, when creating idp - and rollback when idp deletion. */ - verify(actionManagementService, times(2)).addAction(anyString(), any(), anyString()); + /* check ActionManagementService actionManagementService.deleteAction() is called only once when creating the + user defined federated authenticator. */ + verify(actionManagementService, times(1)).addAction(anyString(), any(), anyString()); } private void addTestIdps() throws IdentityProviderManagementException { diff --git a/components/idp-mgt/org.wso2.carbon.idp.mgt/src/test/java/org/wso2/carbon/idp/mgt/dao/CacheBackedIdPMgtDAOTest.java b/components/idp-mgt/org.wso2.carbon.idp.mgt/src/test/java/org/wso2/carbon/idp/mgt/dao/CacheBackedIdPMgtDAOTest.java index a80704e4c4e0..faab4ecc3f28 100644 --- a/components/idp-mgt/org.wso2.carbon.idp.mgt/src/test/java/org/wso2/carbon/idp/mgt/dao/CacheBackedIdPMgtDAOTest.java +++ b/components/idp-mgt/org.wso2.carbon.idp.mgt/src/test/java/org/wso2/carbon/idp/mgt/dao/CacheBackedIdPMgtDAOTest.java @@ -1545,7 +1545,7 @@ public void testDeleteIdPsDAOException() throws Exception { // Deleting multiple IDPs on a tenant. assertThrows(IdentityProviderManagementException.class, () -> cacheBackedIdPMgtDAOForException.deleteIdPs(SAMPLE_TENANT_ID1)); - verify(actionManagementService, times(1)).addAction(anyString(), any(), anyString()); + verify(actionManagementService, times(0)).addAction(anyString(), any(), anyString()); } } @@ -1630,7 +1630,7 @@ public void testForceDeleteIdPDAOException() throws Exception { cacheBackedIdPMgtDAOForException.forceDeleteIdP( userDefinedIdP.getIdentityProviderName(), SAMPLE_TENANT_ID1, TENANT_DOMAIN)); - verify(actionManagementService, times(2)).addAction(anyString(), any(), anyString()); + verify(actionManagementService, times(0)).addAction(anyString(), any(), anyString()); } } @@ -1654,7 +1654,7 @@ public void testDeleteIdPDAOException() throws Exception { cacheBackedIdPMgtDAOForException.deleteIdP( userDefinedIdP.getIdentityProviderName(), SAMPLE_TENANT_ID1, TENANT_DOMAIN)); - verify(actionManagementService, times(1)).addAction(anyString(), any(), anyString()); + verify(actionManagementService, times(0)).addAction(anyString(), any(), anyString()); } }