From 0d58c41b9c136189b8c1ef898c4a56c30f1e9f75 Mon Sep 17 00:00:00 2001 From: malithie Date: Wed, 8 Jan 2025 13:52:25 +0530 Subject: [PATCH] Fix updating action adding updating or removing rule. --- .../dao/impl/ActionManagementDAOFacade.java | 87 +++++++++++++------ .../dao/impl/ActionManagementDAOImpl.java | 18 ++-- .../action/management/model/ActionRule.java | 10 +++ .../dao/ActionManagementDAOFacadeTest.java | 21 ++++- .../ActionManagementServiceImplTest.java | 2 +- 5 files changed, 102 insertions(+), 36 deletions(-) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/dao/impl/ActionManagementDAOFacade.java b/components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/dao/impl/ActionManagementDAOFacade.java index 21ca6361a734..a39c89eea433 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/dao/impl/ActionManagementDAOFacade.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/dao/impl/ActionManagementDAOFacade.java @@ -87,8 +87,8 @@ public void addAction(ActionDTO actionDTO, Integer tenantId) throws ActionMgtExc // Since exceptions thrown are wrapped with TransactionException, extracting the actual cause. handleActionPropertyResolverClientException(e.getCause()); LOG.debug("Error while creating the Action of Action Type: " + actionDTO.getType().getDisplayName() + - " in Tenant Domain: " + IdentityTenantUtil.getTenantDomain(tenantId) + - ". Rolling back created action information, authentication secrets and action properties."); + " in Tenant Domain: " + IdentityTenantUtil.getTenantDomain(tenantId) + + ". Rolling back created action information, authentication secrets and action properties."); throw ActionManagementExceptionHandler.handleServerException(ErrorMessage.ERROR_WHILE_ADDING_ACTION, e); } } @@ -289,8 +289,8 @@ private void deleteAuthenticationSecrets(ActionDTO deletingActionDTO) throws Act * Add the encrypted authentication secrets and replace the input authentication properties in the ActionDTOBuilder * object. * - * @param actionDTOBuilder ActionDTOBuilder object. - * @param encryptedProperties List of encrypted AuthProperty objects. + * @param actionDTOBuilder ActionDTOBuilder object. + * @param encryptedProperties List of encrypted AuthProperty objects. */ private void addEncryptedAuthSecretsToBuilder(ActionDTOBuilder actionDTOBuilder, List encryptedProperties) { @@ -299,17 +299,17 @@ private void addEncryptedAuthSecretsToBuilder(ActionDTOBuilder actionDTOBuilder, .collect(Collectors.toMap(AuthProperty::getName, AuthProperty::getValue)); actionDTOBuilder.endpoint(new EndpointConfig.EndpointConfigBuilder() - .uri(actionDTOBuilder.getEndpoint().getUri()) - .authentication(new Authentication.AuthenticationBuilder() - .type(actionDTOBuilder.getEndpoint().getAuthentication().getType()) - .properties(encryptedPropertyMap) - .build()) - .build()); + .uri(actionDTOBuilder.getEndpoint().getUri()) + .authentication(new Authentication.AuthenticationBuilder() + .type(actionDTOBuilder.getEndpoint().getAuthentication().getType()) + .properties(encryptedPropertyMap) + .build()) + .build()); } private void addActionRule(ActionDTOBuilder actionDTOBuilder, String tenantDomain) throws ActionMgtException { - if (actionDTOBuilder.getActionRule() == null) { + if (actionDTOBuilder.getActionRule() == null || actionDTOBuilder.getActionRule().getRule() == null) { return; } @@ -341,25 +341,56 @@ private void loadActionRule(ActionDTOBuilder actionDTOBuilder, String tenantDoma } private void updateActionRule(ActionDTOBuilder updatingActionDTOBuilder, ActionDTO existingActionDTO, - String tenantDomain) - throws ActionMgtException { - - if (existingActionDTO.getActionRule() == null && updatingActionDTOBuilder.getActionRule() != null) { - // This means a new action rule is added when updating the action. + String tenantDomain) throws ActionMgtException { + + /* + When updating an action, the action rule can be added, removed or updated. + When action rule is added, the Rule object is added to the ActionRule of the ActionDTO. + When action rule is removed, the Rule object is set as null in the ActionRule of the ActionDTO. + This happens as the API accepts the removal of the rule in an action update via an empty rule JSON object. + e.g., rule: {}. If rule is not present in the payload that means rule is not updated. + When action rule is updated, the Rule object is updated in the ActionRule of the ActionDTO. + */ + if (isAddingNewActionRule(updatingActionDTOBuilder, existingActionDTO)) { addActionRule(updatingActionDTOBuilder, tenantDomain); - } else if (existingActionDTO.getActionRule() != null && updatingActionDTOBuilder.getActionRule() == null) { - // This means the existing action rule is removed when updating the action. + } else if (isRemovingExistingActionRule(updatingActionDTOBuilder, existingActionDTO)) { deleteActionRule(existingActionDTO, tenantDomain); - } else if (existingActionDTO.getActionRule() != null && updatingActionDTOBuilder.getActionRule() != - null) { // This means the existing action rule is updated when updating the action. - try { - updatingActionDTOBuilder.getActionRule().getRule().setId(existingActionDTO.getActionRule().getId()); - ActionMgtServiceComponentHolder.getInstance() - .getRuleManagementService() - .updateRule(updatingActionDTOBuilder.getActionRule().getRule(), tenantDomain); - } catch (RuleManagementException e) { - throw new ActionMgtServerException("Error while updating the Rule associated with the Action.", e); - } + } else if (isUpdatingExistingActionRule(updatingActionDTOBuilder, existingActionDTO)) { + updateExistingActionRule(updatingActionDTOBuilder, existingActionDTO, tenantDomain); + } + } + + private boolean isAddingNewActionRule(ActionDTOBuilder updatingActionDTOBuilder, ActionDTO existingActionDTO) + throws ActionMgtException { + + return existingActionDTO.getActionRule() == null && updatingActionDTOBuilder.getActionRule() != null && + updatingActionDTOBuilder.getActionRule().getRule() != null; + } + + private boolean isRemovingExistingActionRule(ActionDTOBuilder updatingActionDTOBuilder, + ActionDTO existingActionDTO) throws ActionMgtException { + + return existingActionDTO.getActionRule() != null && updatingActionDTOBuilder.getActionRule() != null && + updatingActionDTOBuilder.getActionRule().getRule() == null; + } + + private boolean isUpdatingExistingActionRule(ActionDTOBuilder updatingActionDTOBuilder, + ActionDTO existingActionDTO) throws ActionMgtException { + + return existingActionDTO.getActionRule() != null && updatingActionDTOBuilder.getActionRule() != null && + updatingActionDTOBuilder.getActionRule().getRule() != null; + } + + private void updateExistingActionRule(ActionDTOBuilder updatingActionDTOBuilder, ActionDTO existingActionDTO, + String tenantDomain) throws ActionMgtException { + + try { + updatingActionDTOBuilder.getActionRule().getRule().setId(existingActionDTO.getActionRule().getId()); + ActionMgtServiceComponentHolder.getInstance() + .getRuleManagementService() + .updateRule(updatingActionDTOBuilder.getActionRule().getRule(), tenantDomain); + } catch (RuleManagementException e) { + throw new ActionMgtServerException("Error while updating the Rule associated with the Action.", e); } } diff --git a/components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/dao/impl/ActionManagementDAOImpl.java b/components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/dao/impl/ActionManagementDAOImpl.java index 8026085a1e9b..61aa9800ada0 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/dao/impl/ActionManagementDAOImpl.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/dao/impl/ActionManagementDAOImpl.java @@ -481,12 +481,18 @@ private void addRuleReference(ActionDTO actionDTO, Integer tenantId) throws Acti private void updateRuleReference(ActionDTO updatingActionDTO, ActionDTO existingActionDTO, Integer tenantId) throws ActionMgtServerException { - if (existingActionDTO.getActionRule() == null && updatingActionDTO.getActionRule() != null) { - // This means a new action rule is added when updating the action. Add the rule reference. - addRuleReference(updatingActionDTO, tenantId); - } else if (existingActionDTO.getActionRule() != null && updatingActionDTO.getActionRule() == null) { - // This means the existing action rule is removed when updating the action. Remove the rule reference. - deleteRuleReference(updatingActionDTO, tenantId); + try { + if (existingActionDTO.getActionRule() == null && updatingActionDTO.getActionRule() != null && + updatingActionDTO.getActionRule().getRule() != null) { + // This means a new action rule is added when updating the action. Add the rule reference. + addRuleReference(updatingActionDTO, tenantId); + } else if (existingActionDTO.getActionRule() != null && updatingActionDTO.getActionRule() != null && + updatingActionDTO.getActionRule().getRule() == null) { + // This means the existing action rule is removed when updating the action. Remove the rule reference. + deleteRuleReference(updatingActionDTO, tenantId); + } + } catch (ActionMgtException e) { + throw new ActionMgtServerException("Error while updating the reference for the Rule in Action.", e); } } diff --git a/components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/model/ActionRule.java b/components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/model/ActionRule.java index e84902cc178e..6f463a8567ce 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/model/ActionRule.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/model/ActionRule.java @@ -71,6 +71,16 @@ action status (active/inactive), returning state of the prior action in the chai private Rule getRuleFromRuleManagementService() throws ActionMgtServerException { + if (StringUtils.isBlank(id) || StringUtils.isBlank(tenantDomain)) { + /* + This could happen if the ActionRule is created without the Rule object. + That means the rule value is explicitly set to null. + In such cases, loading the Rule from the Rule Management Service is not expected. + This scenario is used to let the user remove a Rule from an Action via the Action update API. + */ + return null; + } + try { return ActionMgtServiceComponentHolder.getInstance() .getRuleManagementService() diff --git a/components/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/dao/ActionManagementDAOFacadeTest.java b/components/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/dao/ActionManagementDAOFacadeTest.java index a579156155b9..fc569f20ff12 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/dao/ActionManagementDAOFacadeTest.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/dao/ActionManagementDAOFacadeTest.java @@ -456,7 +456,8 @@ public void testFailureInUpdateActionUpdatingRuleAtRuleManagementException() thr @Test(priority = 18) public void testUpdateActionRemovingRule() throws Exception { - ActionDTO updatingActionDTOWithoutRule = createActionDTOForPasswordUpdateAction(); + // In order to remove the rule from ActionRule create an ActionRule with a null Rule reference. + ActionDTO updatingActionDTOWithoutRule = createActionDTOForPasswordUpdateActionWithRule(null); mockActionPropertyResolver(testActionPropertyResolver); daoFacade.updateAction(updatingActionDTOWithoutRule, actionDTORetrieved, TENANT_ID); @@ -507,6 +508,24 @@ public void testUpdateActionAddingRule() throws Exception { } @Test(priority = 21) + public void testUpdateActionWithRuleWithoutUpdatingRule() throws Exception { + + Rule rule = mockRule(); + mockRuleManagementService(rule); + ActionDTO updatingActionDTO = createActionDTOForPasswordUpdateAction(); + ActionDTO expectedActionDTO = createActionDTOForPasswordUpdateActionWithRule(rule); + mockActionPropertyResolver(testActionPropertyResolver); + + daoFacade.updateAction(updatingActionDTO, actionDTORetrieved, TENANT_ID); + + actionDTORetrieved = daoFacade.getActionByActionId(PRE_UPDATE_PASSWORD_TYPE, PRE_UPDATE_PASSWORD_ACTION_ID, + TENANT_ID); + + verifyActionDTO(actionDTORetrieved, expectedActionDTO); + verifyActionDTORule(actionDTORetrieved, expectedActionDTO); + } + + @Test(priority = 22) public void testDeleteActionWithRule() throws ActionMgtException { mockActionPropertyResolver(testActionPropertyResolver); diff --git a/components/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/service/ActionManagementServiceImplTest.java b/components/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/service/ActionManagementServiceImplTest.java index 5fcc6fc3b1ef..d6f9bfe0e7e5 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/service/ActionManagementServiceImplTest.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/service/ActionManagementServiceImplTest.java @@ -395,7 +395,7 @@ public void testGetActionsWithRulesByActionType() throws ActionMgtException { } @Test(priority = 17) - public void testUpdateActionWithRule() throws ActionMgtException, SecretManagementException { + public void testUpdateActionUpdatingRule() throws ActionMgtException, SecretManagementException { Action updatingAction = TestUtil.buildMockActionWithRule( TEST_ACTION_NAME_UPDATED,