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 6565355
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 33 deletions.
52 changes: 28 additions & 24 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,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;
Expand All @@ -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;
Expand All @@ -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) {

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

Expand All @@ -197,55 +200,56 @@ 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(IdPManagementConstants.WarningMessage.WARN_STALE_IDP_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(IdPManagementConstants.WarningMessage.WARN_STALE_IDP_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(IdPManagementConstants.WarningMessage.WARN_STALE_IDP_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(IdPManagementConstants.WarningMessage.WARN_STALE_IDP_ACTION,
identityProvider.getIdentityProviderName()));
}
}

Expand Down
8 changes: 8 additions & 0 deletions ...so2.carbon.idp.mgt/src/main/java/org/wso2/carbon/idp/mgt/util/IdPManagementConstants.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
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 6565355

Please sign in to comment.