Skip to content

Commit

Permalink
Improve idp deletion error scenario.
Browse files Browse the repository at this point in the history
  • Loading branch information
@Thisara-Welmilla
Thisara-Welmilla committed Dec 4, 2024
1 parent 78c31ab commit 1b2d38d
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 32 deletions.
54 changes: 31 additions & 23 deletions ...rg.wso2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/dao/IdPManagementFacade.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,16 @@
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;
import org.wso2.carbon.identity.core.util.IdentityTenantUtil;
import org.wso2.carbon.idp.mgt.IdentityProviderManagementClientException;
import org.wso2.carbon.idp.mgt.IdentityProviderManagementException;
import org.wso2.carbon.idp.mgt.IdentityProviderManagementServerException;
import org.wso2.carbon.idp.mgt.IdentityProviderManager;
import org.wso2.carbon.idp.mgt.model.ConnectedAppsResult;
import org.wso2.carbon.idp.mgt.util.UserDefinedAuthenticatorEndpointConfigManager;

Expand All @@ -40,6 +43,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) {

Expand Down Expand Up @@ -183,12 +187,13 @@ 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("The %s Identity Provider was deleted, but an error occurred while deleting its " +
"associated action.", idPName));
}
}

Expand All @@ -197,55 +202,58 @@ 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<IdentityProvider> 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("The Identity Providers was deleted, but an error occurred while deleting their " +
"associated actions.");
}
}

public void deleteIdPByResourceId(String resourceId, int tenantId, String tenantDomain)
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("The %s Identity Provider was deleted, but an error occurred while deleting its " +
"associated action.", identityProvider.getIdentityProviderName()));
}
}

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("The %s Identity Provider was deleted, but an error occurred while deleting its " +
"associated action.", idPName));
}
}

public void forceDeleteIdPByResourceId(String resourceId, int tenantId, String tenantDomain)
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("The %s Identity Provider was deleted, but an error occurred while deleting its " +
"associated action.", identityProvider.getIdentityProviderName()));
}
}

Expand Down
11 changes: 5 additions & 6 deletions ....idp.mgt/src/test/java/org/wso2/carbon/idp/mgt/IdentityProviderManagementServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}

Expand Down Expand Up @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions ...o2.carbon.idp.mgt/src/test/java/org/wso2/carbon/idp/mgt/dao/CacheBackedIdPMgtDAOTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}

Expand Down Expand Up @@ -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());
}
}

Expand All @@ -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());
}
}

Expand Down

0 comments on commit 1b2d38d

Please sign in to comment.