Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor ActionManagementDAO to handle action type specific properties #6119

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Refactor ActionManagementDAO to handle action type specific properties #6119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0756224
Add PreUpdatePasswordAction class
ashanthamara Nov 7, 2024
c207614
Merge remote-tracking branch 'upstream/master' into action-properties
ashanthamara Nov 8, 2024
a3a6111
Improve action-mgt to handle action related properties
ashanthamara Nov 10, 2024
891c883
Improve unit tests for DAO
ashanthamara Nov 10, 2024
9245f16
Merge remote-tracking branch 'upstream/master' into action-properties
ashanthamara Nov 10, 2024
1f987bb
lower coverage levels
ashanthamara Nov 10, 2024
4dca5c1
Refactor unit tests
ashanthamara Nov 10, 2024
1296c3a
Rename `IDN_ACTION_ENDPOINT` table name to `IDN_ACTION_PROPERTIES`
ashanthamara Nov 10, 2024
ab1bb18
Add jdbcTemplate.withTransaction for delete-secret and update-secret …
ashanthamara Nov 10, 2024
f4b2f28
Fix sonarcloud suggestions
ashanthamara Nov 10, 2024
374b341
Move errors defined to rest-api layer to service layer
ashanthamara Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions components/action-mgt/org.wso2.carbon.identity.action.management/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@
<groupId>org.wso2.carbon.identity.framework</groupId>
<artifactId>org.wso2.carbon.identity.secret.mgt.core</artifactId>
</dependency>
<dependency>
<groupId>org.wso2.carbon.identity.framework</groupId>
<artifactId>org.wso2.carbon.identity.certificate.management</artifactId>
</dependency>
<dependency>
<groupId>org.json.wso2</groupId>
<artifactId>json</artifactId>
Expand All @@ -54,6 +58,7 @@
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.wso2.carbon.identity.framework</groupId>
Expand Down Expand Up @@ -178,14 +183,14 @@
<!-- Temporarily decreasing this.
Will be improved upon in a follow-up effort.
Related issue: https://github.com/wso2/product-is/issues/21368 -->
<minimum>0.70</minimum>
<minimum>0.77</minimum>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Target here should be 0.8

</limit>
<limit implementation="org.jacoco.report.check.Limit">
<counter>COMPLEXITY</counter>
<value>COVEREDRATIO</value>
<!-- Temp decreasing this. There is an ongoing effort to improve
the code complexity coverage. -->
<minimum>0.60</minimum>
<minimum>0.68</minimum>
Copy link
Member

@malithie malithie Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this to 0.6

</limit>
</limits>
</rule>
Expand Down
14 changes: 0 additions & 14 deletions ...ent/src/main/java/org/wso2/carbon/identity/action/management/ActionManagementService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

import org.wso2.carbon.identity.action.management.exception.ActionMgtException;
import org.wso2.carbon.identity.action.management.model.Action;
import org.wso2.carbon.identity.action.management.model.Authentication;

import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -115,17 +114,4 @@ Action updateAction(String actionType, String actionId, Action action, String te
* @throws ActionMgtException If an error occurs while retrieving the Action of a given Action ID.
*/
Action getActionByActionId(String actionType, String actionId, String tenantDomain) throws ActionMgtException;

/**
* Update the authentication of the action endpoint.
*
* @param actionType Action Type.
* @param actionId Action ID.
* @param authentication Authentication Information to be updated.
* @param tenantDomain Tenant domain.
* @return Action response after update.
* @throws ActionMgtException If an error occurs while updating action endpoint authentication information.
*/
Action updateActionEndpointAuthentication(String actionType, String actionId, Authentication authentication,
String tenantDomain) throws ActionMgtException;
}
79 changes: 1 addition & 78 deletions ...src/main/java/org/wso2/carbon/identity/action/management/ActionManagementServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import org.wso2.carbon.identity.action.management.exception.ActionMgtException;
import org.wso2.carbon.identity.action.management.model.Action;
import org.wso2.carbon.identity.action.management.model.Authentication;
import org.wso2.carbon.identity.action.management.model.EndpointConfig;
import org.wso2.carbon.identity.action.management.util.ActionManagementAuditLogger;
import org.wso2.carbon.identity.action.management.util.ActionManagementUtil;
import org.wso2.carbon.identity.action.management.util.ActionValidator;
Expand Down Expand Up @@ -237,32 +236,6 @@ public Action getActionByActionId(String actionType, String actionId, String ten
IdentityTenantUtil.getTenantId(tenantDomain));
}

/**
* Update endpoint authentication of a given action.
*
* @param actionType Action type.
* @param actionId Action ID.
* @param authentication Authentication Information to be updated.
* @param tenantDomain Tenant domain.
* @return Updated action.
* @throws ActionMgtException if an error occurred while updating endpoint authentication information.
*/
@Override
public Action updateActionEndpointAuthentication(String actionType, String actionId, Authentication authentication,
String tenantDomain) throws ActionMgtException {

String resolvedActionType = getActionTypeFromPath(actionType);
Action existingAction = checkIfActionExists(resolvedActionType, actionId, tenantDomain);
doEndpointAuthenticationValidation(authentication);
if (existingAction.getEndpoint().getAuthentication().getType().equals(authentication.getType())) {
// Only need to update the properties since the authentication type is same.
return updateEndpointAuthenticationProperties(resolvedActionType, actionId, authentication, tenantDomain);
} else {
// Need to update the authentication type and properties.
return updateEndpoint(resolvedActionType, actionId, existingAction, authentication, tenantDomain);
}
}

/**
* Get Action Type from path.
*
Expand Down Expand Up @@ -317,61 +290,11 @@ private Action checkIfActionExists(String actionType, String actionId, String te
return action;
}

/**
* Update the authentication type and properties of the action endpoint.
*
* @param actionType Action Type.
* @param actionId Action Id.
* @param existingAction Existing Action Information.
* @param authentication Authentication Information to be updated.
* @param tenantDomain Tenant Domain.
* @return Action response after update.
* @throws ActionMgtException If an error occurs while updating action endpoint authentication.
*/
private Action updateEndpoint(String actionType, String actionId, Action existingAction,
Authentication authentication, String tenantDomain)
throws ActionMgtException {

if (LOG.isDebugEnabled()) {
LOG.debug(String.format("Updating endpoint authentication of Action Type: %s " +
"and Action ID: %s to Authentication Type: %s", actionType, actionId,
authentication.getType().name()));
}
EndpointConfig endpoint = new EndpointConfig.EndpointConfigBuilder()
.uri(existingAction.getEndpoint().getUri())
.authentication(authentication).build();
return CACHE_BACKED_DAO.updateActionEndpoint(actionType, actionId, endpoint,
existingAction.getEndpoint().getAuthentication(), IdentityTenantUtil.getTenantId(tenantDomain));
}

/**
* Update the authentication properties of the action endpoint.
*
* @param actionType Action Type.
* @param actionId Action Id.
* @param authentication Authentication Information to be updated.
* @param tenantDomain Tenant domain.
* @return Action response after update.
* @throws ActionMgtException If an error occurs while updating action endpoint authentication properties.
*/
private Action updateEndpointAuthenticationProperties(String actionType, String actionId,
Authentication authentication, String tenantDomain)
throws ActionMgtException {

if (LOG.isDebugEnabled()) {
LOG.debug(String.format("Updating endpoint authentication properties of Action Type: %s " +
"Action ID: %s and Authentication Type: %s", actionType, actionId,
authentication.getType().name()));
}
return CACHE_BACKED_DAO.updateActionEndpointAuthProperties(actionType, actionId, authentication,
IdentityTenantUtil.getTenantId(tenantDomain));
}

/**
* Perform pre validations on action model when creating an action.
*
* @param action Action create model.
* @throws ActionMgtException if action model is invalid.
* @throws ActionMgtClientException if action model is invalid.
*/
private void doPreAddActionValidations(Action action) throws ActionMgtClientException {

Expand Down
50 changes: 37 additions & 13 deletions ...src/main/java/org/wso2/carbon/identity/action/management/constant/ActionMgtConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@
*/
public class ActionMgtConstants {

public static final String URI_ATTRIBUTE = "uri";
public static final String AUTHN_TYPE_ATTRIBUTE = "authnType";
public static final String URI_PROPERTY = "uri";
public static final String AUTHN_TYPE_PROPERTY = "authnType";
public static final String PASSWORD_SHARING_FORMAT_PROPERTY = "passwordSharingFormat";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constant should not come to the actions core. It's specific to a particular extension

public static final String CERTIFICATE_ID_PROPERTY = "certificateId";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

public static final String IDN_SECRET_TYPE_ACTION_SECRETS = "ACTION_API_ENDPOINT_AUTH_SECRETS";

public static final String ACTION_NAME_FIELD = "Action name";
Expand Down Expand Up @@ -53,19 +55,30 @@ public enum ErrorMessages {
"%s is empty."),
ERROR_INVALID_ACTION_REQUEST_FIELD("60005", "Invalid request.",
"%s is invalid."),
ERROR_INVALID_ACTION_CERTIFICATE("60006", "Invalid request.", "Provided certificate is invalid."),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is specific to a particular extension, we should be able to take these validations out from the core


// Client errors thrown at REST API layer.
ERROR_INVALID_ACTION_ENDPOINT_AUTHENTICATION_PROPERTIES("60007", "Unable to perform the operation.",
"Required authentication properties are not provided or invalid."),
ERROR_INVALID_ACTION_ENDPOINT_AUTH_TYPE("60008", "Invalid Authentication Type for Action Endpoint.",
"Invalid authentication type used for path parameter."),
ERROR_EMPTY_ACTION_ENDPOINT_AUTHENTICATION_PROPERTIES("60009", "Unable to perform the operation.",
"Authentication property values cannot be empty."),
ERROR_NO_ACTION_FOUND_ON_GIVEN_ACTION_TYPE_AND_ID("60010", "Action is not found.",
"No action is found for given action id and action type"),

// Server errors.
ERROR_WHILE_ADDING_ACTION("65001", "Error while adding Action.",
"Error while persisting Action in the system."),
ERROR_WHILE_ADDING_ENDPOINT_PROPERTIES("65002", "Error while adding Endpoint properties",
"Error while persisting Action Endpoint properties in the system."),
ERROR_WHILE_RETRIEVING_ACTION_ENDPOINT_PROPERTIES("65003",
"Error while retrieving Action Endpoint properties",
"Error while retrieving Action Endpoint properties from the system."),
ERROR_WHILE_ADDING_ACTION_PROPERTIES("65002", "Error while adding Action properties",
"Error while persisting Action properties in the system."),
ERROR_WHILE_RETRIEVING_ACTION_PROPERTIES("65003",
"Error while retrieving Action properties",
"Error while retrieving Action properties from the system."),
ERROR_WHILE_RETRIEVING_ACTIONS_BY_ACTION_TYPE("65004",
"Error while retrieving Actions by Action Type",
"Error while retrieving Actions by Action Type from the system."),
ERROR_WHILE_UPDATING_ENDPOINT_PROPERTIES("65005",
ERROR_WHILE_UPDATING_ACTION_PROPERTIES("65005",
"Error while updating Action Endpoint properties",
"Error while updating Action Endpoint properties in the system."),
ERROR_WHILE_UPDATING_ACTION("65006", "Error while updating Action.",
Expand All @@ -84,11 +97,22 @@ public enum ErrorMessages {
ERROR_WHILE_DECRYPTING_ACTION_ENDPOINT_AUTH_PROPERTIES("65012",
"Error while decrypting Action Endpoint Authentication properties",
"Error while decrypting Action Endpoint Authentication properties in the system."),
ERROR_NO_AUTHENTICATION_TYPE("65013",
"Error while retrieving Action Endpoint Authentication configurations",
"Authentication type is not defined for the Action Endpoint."),
ERROR_WHILE_UPDATING_ACTION_BASIC_INFO("65014", "Error while updating basic Action information",
"Error while updating basic Action information in the system.");
ERROR_WHILE_UPDATING_ACTION_BASIC_INFO("65013", "Error while updating basic Action information",
"Error while updating basic Action information in the system."),
ERROR_WHILE_BUILDING_ACTION_RESPONSE("65014", "Error while building Action response.",
"Error while building Action response object."),
ERROR_WHILE_ADDING_ACTION_CERTIFICATE("65015", "Error while adding action certificate.",
"Error while persisting certificate in the system."),
ERROR_WHILE_RETRIEVING_ACTION_CERTIFICATE("65016", "Error while retrieving action certificate.",
"Error while retrieving certificate from the system."),
ERROR_WHILE_UPDATING_ACTION_CERTIFICATE("65017", "Error while updating action certificate.",
"Error while updating certificate in the system."),
ERROR_WHILE_DELETING_ACTION_CERTIFICATE("65018", "Error while deleting action certificate.",
"Error while deleting certificate from the system."),

// Server errors thrown at REST API layer.
ERROR_NOT_IMPLEMENTED_ACTION_TYPE("650015", "Unable to perform the operation.",
"The requested action type is not currently supported by the server.");

private final String code;
private final String message;
Expand Down
9 changes: 3 additions & 6 deletions .../main/java/org/wso2/carbon/identity/action/management/constant/ActionMgtSQLConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,28 +55,25 @@ public static class Query {

public static final String ADD_ACTION_TO_ACTION_TYPE = "INSERT INTO IDN_ACTION (UUID, TYPE, NAME, " +
"DESCRIPTION, STATUS, TENANT_ID) VALUES (:UUID;, :TYPE;, :NAME;, :DESCRIPTION;, :STATUS;, :TENANT_ID;)";
public static final String ADD_ACTION_ENDPOINT_PROPERTIES = "INSERT INTO IDN_ACTION_ENDPOINT (ACTION_UUID, " +
public static final String ADD_ACTION_ENDPOINT_PROPERTIES = "INSERT INTO IDN_ACTION_PROPERTIES (ACTION_UUID, " +
"PROPERTY_NAME, PROPERTY_VALUE, TENANT_ID) VALUES (:ACTION_UUID;, :PROPERTY_NAME;, :PROPERTY_VALUE;, " +
":TENANT_ID;)";
public static final String GET_ACTION_BASIC_INFO_BY_ID = "SELECT TYPE, NAME, DESCRIPTION, STATUS FROM " +
"IDN_ACTION WHERE TYPE = :TYPE; AND UUID = :UUID; AND TENANT_ID = :TENANT_ID;";
public static final String GET_ACTION_ENDPOINT_INFO_BY_ID = "SELECT PROPERTY_NAME, PROPERTY_VALUE FROM " +
"IDN_ACTION_ENDPOINT WHERE ACTION_UUID = :ACTION_UUID; AND TENANT_ID = :TENANT_ID;";
"IDN_ACTION_PROPERTIES WHERE ACTION_UUID = :ACTION_UUID; AND TENANT_ID = :TENANT_ID;";
public static final String GET_ACTIONS_BASIC_INFO_BY_ACTION_TYPE = "SELECT UUID, TYPE, NAME, DESCRIPTION," +
" STATUS FROM IDN_ACTION WHERE TYPE = :TYPE; AND TENANT_ID = :TENANT_ID;";
public static final String UPDATE_ACTION_BASIC_INFO = "UPDATE IDN_ACTION SET NAME = :NAME;, DESCRIPTION = " +
":DESCRIPTION; WHERE UUID = :UUID; AND TYPE = :TYPE; AND TENANT_ID = :TENANT_ID;";
public static final String DELETE_ACTION_ENDPOINT_PROPERTIES = "DELETE FROM IDN_ACTION_ENDPOINT WHERE " +
public static final String DELETE_ACTION_ENDPOINT_PROPERTIES = "DELETE FROM IDN_ACTION_PROPERTIES WHERE " +
"ACTION_UUID = :ACTION_UUID; AND TENANT_ID = :TENANT_ID;";
public static final String DELETE_ACTION = "DELETE FROM IDN_ACTION WHERE UUID = :UUID; AND TYPE = :TYPE;" +
" AND TENANT_ID = :TENANT_ID;";
public static final String CHANGE_ACTION_STATUS = "UPDATE IDN_ACTION SET STATUS = :STATUS; WHERE UUID = " +
":UUID; AND TYPE = :TYPE; AND TENANT_ID = :TENANT_ID;";
public static final String GET_ACTIONS_COUNT_PER_ACTION_TYPE = "SELECT TYPE, COUNT(UUID) AS COUNT" +
" FROM IDN_ACTION WHERE TENANT_ID = :TENANT_ID; GROUP BY TYPE";
public static final String UPDATE_ACTION_ENDPOINT_PROPERTIES = "UPDATE IDN_ACTION_ENDPOINT SET " +
"PROPERTY_VALUE = :PROPERTY_VALUE; WHERE ACTION_UUID = :ACTION_UUID; AND " +
"TENANT_ID = :TENANT_ID; AND PROPERTY_NAME = :PROPERTY_NAME;";

private Query() {

Expand Down
28 changes: 0 additions & 28 deletions ...ent/src/main/java/org/wso2/carbon/identity/action/management/dao/ActionManagementDAO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@

import org.wso2.carbon.identity.action.management.exception.ActionMgtException;
import org.wso2.carbon.identity.action.management.model.Action;
import org.wso2.carbon.identity.action.management.model.Authentication;
import org.wso2.carbon.identity.action.management.model.EndpointConfig;

import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -118,30 +116,4 @@ Action updateAction(String actionType, String actionId, Action updatingAction, A
* @throws ActionMgtException If an error occurs while retrieving the Action of a given Action ID.
*/
Action getActionByActionId(String actionType, String actionId, Integer tenantId) throws ActionMgtException;

/**
* Update the endpoint authentication properties of an {@link Action} by given Action ID.
*
* @param actionId Action ID.
* @param authentication Authentication information to be updated.
* @param tenantId Tenant Id.
* @return Updated <code>Action</code>.
* @throws ActionMgtException If an error occurs while updating the Action endpoint authentication properties.
*/
Action updateActionEndpointAuthProperties(String actionType, String actionId, Authentication authentication,
int tenantId) throws ActionMgtException;

/**
* Update the endpoint authentication properties of an {@link Action} by given Action ID.
*
* @param actionType Action Type.
* @param actionId Action ID.
* @param endpoint Endpoint information to be updated.
* @param currentAuthentication Current Action endpoint authentication information.
* @param tenantId Tenant Id.
* @return Updated <code>Action</code>.
* @throws ActionMgtException If an error occurs while updating the Action endpoint.
*/
Action updateActionEndpoint(String actionType, String actionId, EndpointConfig endpoint,
Authentication currentAuthentication, int tenantId) throws ActionMgtException;
}
Loading
Loading