Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,7 @@
hs_err_pid*

# Ignore everything in this directory
target
target

# Maven versions plugin backup files
*.versionsBackup

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

revert!

Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ public enum ActionType {
PRE_UPDATE_PASSWORD,
PRE_UPDATE_PROFILE,
AUTHENTICATION,
PRE_ISSUE_ID_TOKEN;
PRE_ISSUE_ID_TOKEN,
FLOW_EXTENSION;

public String getDisplayName() {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,18 @@

package org.wso2.carbon.identity.action.execution.api.model;

import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.databind.JsonSerializer;
import com.fasterxml.jackson.databind.SerializerProvider;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

/**
* This class models the User.
Expand All @@ -30,9 +39,11 @@ public class User {

private final String id;
private final List<UserClaim> claims = new ArrayList<>();
private final Map<String, char[]> userCredentials = new HashMap<>();
private final List<String> groups = new ArrayList<>();
private final List<String> roles = new ArrayList<>();
private Organization organization;
private UserStore userStoreDomain;
/**
* Represents the user id when the user is shared across sub-organizations.
* This field differs from the regular user id ({@link #id}) in scenarios where a user is accessed in the context
Expand Down Expand Up @@ -65,15 +76,18 @@ public User(Builder builder) {

this.id = builder.id;
this.claims.addAll(builder.claims);
this.userCredentials.putAll(builder.userCredentials);
this.groups.addAll(builder.groups);
Comment on lines +79 to 80

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Defensively copy char[] credential values to avoid mutable secret leakage.

Collections.unmodifiableMap(...) protects map structure only; the contained char[] values are still mutable and currently shared across builder/instance/caller boundaries.

🔧 Proposed fix
@@
-        this.userCredentials.putAll(builder.userCredentials);
+        builder.userCredentials.forEach((k, v) ->
+                this.userCredentials.put(k, v == null ? null : v.clone()));
@@
     public Map<String, char[]> getUserCredentials() {
-
-        return Collections.unmodifiableMap(userCredentials);
+        Map<String, char[]> copy = new HashMap<>();
+        userCredentials.forEach((k, v) -> copy.put(k, v == null ? null : v.clone()));
+        return Collections.unmodifiableMap(copy);
     }
@@
         public Builder userCredentials(Map<String, char[]> userCredentials) {
-
-            this.userCredentials.putAll(userCredentials);
+            if (userCredentials != null) {
+                userCredentials.forEach((k, v) ->
+                        this.userCredentials.put(k, v == null ? null : v.clone()));
+            }
             return this;
         }

Also applies to: 101-104, 224-227

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/api/model/User.java`
around lines 79 - 80, The User class copies references to mutable credential
char[] values (via this.userCredentials.putAll(builder.userCredentials)) which
allows secret leakage; update all places where userCredentials is copied or
exposed (e.g., the User constructor, any builder.build path, and getters that
return the map) to defensively clone each char[] value: iterate the source map
(builder.userCredentials or internal map), create a copy of each char[] (new
char[length] + System.arraycopy/Arrays.copyOf) and store the clones in the
target map, and likewise when returning credentials from getters return a new
map with cloned char[] values (or immutable wrappers) so no internal char[] is
shared. Ensure you apply this cloning consistently in the User constructor,
builder->User transfer, and any methods referenced around userCredentials and
related fields (e.g., groups handling locations noted in the review).

this.roles.addAll(builder.roles);
this.organization = builder.organization;
this.sharedUserId = builder.sharedUserId;
this.userType = builder.userType;
this.federatedIdP = builder.federatedIdP;
this.accessingOrganization = builder.accessingOrganization;
this.userStoreDomain = builder.userStoreDomain;
}

@JsonInclude(JsonInclude.Include.NON_NULL)
public String getId() {

return id;
Expand All @@ -84,6 +98,11 @@ public List<UserClaim> getClaims() {
return Collections.unmodifiableList(claims);
}

public Map<String, char[]> getUserCredentials() {

return Collections.unmodifiableMap(userCredentials);
}

public List<String> getGroups() {

return Collections.unmodifiableList(groups);
Expand All @@ -99,6 +118,13 @@ public Organization getOrganization() {
return organization;
}

@JsonInclude(JsonInclude.Include.NON_NULL)
Comment thread
KD23243 marked this conversation as resolved.
@JsonSerialize(using = UserStoreNameSerializer.class)
public UserStore getUserStoreDomain() {
Comment thread
KD23243 marked this conversation as resolved.

return userStoreDomain;
}

public String getSharedUserId() {

return sharedUserId;
Expand Down Expand Up @@ -126,9 +152,11 @@ public static class Builder {

private final String id;
private final List<UserClaim> claims = new ArrayList<>();
private final Map<String, char[]> userCredentials = new HashMap<>();
private final List<String> groups = new ArrayList<>();
private final List<String> roles = new ArrayList<>();
private Organization organization;
private UserStore userStoreDomain;
private String sharedUserId;
private String userType;
private String federatedIdP;
Expand Down Expand Up @@ -163,6 +191,12 @@ public Builder organization(Organization organization) {
return this;
}

public Builder userStoreDomain(UserStore userStoreDomain) {

this.userStoreDomain = userStoreDomain;
return this;
}

public Builder sharedUserId(String sharedUserId) {

this.sharedUserId = sharedUserId;
Expand All @@ -187,9 +221,24 @@ public Builder accessingOrganization(Organization accessingOrganization) {
return this;
}

public Builder userCredentials(Map<String, char[]> userCredentials) {

this.userCredentials.putAll(userCredentials);
return this;
}

public User build() {

return new User(this);
}
}

private static class UserStoreNameSerializer extends JsonSerializer<UserStore> {

@Override
public void serialize(UserStore value, JsonGenerator gen, SerializerProvider serializers) throws IOException {

gen.writeString(value.getName() != null ? value.getName() : "");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Setting an empty string might be problematic for the other action types, as this will start adding this userstoredomain attribute to the user object with an empty value.

Since this userStoreDomain is specific for the Inflow extension, I suggest using an extended User class in the inflow extensions component.

}
}
Comment on lines +236 to +243

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need this

}
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,12 @@ public ActionExecutionStatus execute(ActionType actionType, String actionId,
throw new ActionExecutionException("Action Id cannot be blank.");
}

try {
Action action = getActionByActionId(actionType, actionId, tenantDomain);
return execute(action, flowContext, tenantDomain);
} catch (ActionExecutionRuntimeException e) {
LOG.debug("Skip executing action for action type: " + actionType.name(), e);
// Skip executing actions when no action available is considered as action execution being successful.
Action action = getActionByActionId(actionType, actionId, tenantDomain);
if (action == null) {
LOG.debug("No action found for action Id: " + actionId + ". Skipping action execution.");
return new SuccessStatus.Builder().setResponseContext(flowContext.getContextData()).build();
Comment on lines +151 to 154

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Guard the new concatenated DEBUG log.

The new debug statement does string concatenation without a debug guard.

Proposed fix
         Action action = getActionByActionId(actionType, actionId, tenantDomain);
         if (action == null) {
-            LOG.debug("No action found for action Id: " + actionId + ". Skipping action execution.");
+            if (LOG.isDebugEnabled()) {
+                LOG.debug("No action found for action Id: " + actionId + ". Skipping action execution.");
+            }
             return new SuccessStatus.Builder().setResponseContext(flowContext.getContextData()).build();
         }

As per coding guidelines, "DEBUG: guard all non-trivial debug logs with if (log.isDebugEnabled())."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Action action = getActionByActionId(actionType, actionId, tenantDomain);
if (action == null) {
LOG.debug("No action found for action Id: " + actionId + ". Skipping action execution.");
return new SuccessStatus.Builder().setResponseContext(flowContext.getContextData()).build();
Action action = getActionByActionId(actionType, actionId, tenantDomain);
if (action == null) {
if (LOG.isDebugEnabled()) {
LOG.debug("No action found for action Id: " + actionId + ". Skipping action execution.");
}
return new SuccessStatus.Builder().setResponseContext(flowContext.getContextData()).build();
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/internal/service/impl/ActionExecutorServiceImpl.java`
around lines 151 - 154, The new LOG.debug call in the block after calling
getActionByActionId(actionType, actionId, tenantDomain) performs string
concatenation unguarded; wrap that debug log in a conditional using
LOG.isDebugEnabled() to avoid unnecessary work when debug logging is off (i.e.,
check if (LOG.isDebugEnabled()) before calling LOG.debug("No action found for
action Id: " + actionId + ". Skipping action execution.")), keeping the rest of
the method flow (returning new
SuccessStatus.Builder().setResponseContext(flowContext.getContextData()).build())
unchanged.

}
return execute(action, flowContext, tenantDomain);
Comment on lines +151 to +156

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ashanthamara for feedback

}

private ActionExecutionStatus<?> execute(Action action, FlowContext flowContext, String tenantDomain)
Expand Down Expand Up @@ -197,8 +195,9 @@ private Action getActionByActionId(ActionType actionType, String actionId, Strin
throws ActionExecutionException {

try {
return ActionExecutionServiceComponentHolder.getInstance().getActionManagementService().getActionByActionId(
Action.ActionTypes.valueOf(actionType.name()).getPathParam(), actionId, tenantDomain);
return ActionExecutionServiceComponentHolder.getInstance().getActionManagementService()
.getActionByActionId(Action.ActionTypes.valueOf(actionType.name()).getPathParam(), actionId,
tenantDomain);
Comment thread
ThaminduR marked this conversation as resolved.
Outdated
} catch (ActionMgtException e) {
throw new ActionExecutionException("Error occurred while retrieving action by action Id.", e);
}
Comment thread
KD23243 marked this conversation as resolved.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ public boolean isExecutionForActionTypeEnabled(ActionType actionType) {
return isActionTypeEnabled(ActionTypeConfig.PRE_UPDATE_PROFILE.getActionTypeEnableProperty());
case PRE_ISSUE_ID_TOKEN:
return isActionTypeEnabled(ActionTypeConfig.PRE_ISSUE_ID_TOKEN.getActionTypeEnableProperty());
case FLOW_EXTENSION:
return isActionTypeEnabled(ActionTypeConfig.FLOW_EXTENSION.getActionTypeEnableProperty());
default:
return false;
}
Expand Down Expand Up @@ -333,6 +335,8 @@ public String getRetiredUpToVersion(ActionType actionType) {
return getVersion(ActionTypeConfig.PRE_UPDATE_PROFILE.getRetiredUpToVersionProperty());
case PRE_ISSUE_ID_TOKEN:
return getVersion(ActionTypeConfig.PRE_ISSUE_ID_TOKEN.getRetiredUpToVersionProperty());
case FLOW_EXTENSION:
return getVersion(ActionTypeConfig.FLOW_EXTENSION.getRetiredUpToVersionProperty());
default:
return null;
}
Expand Down Expand Up @@ -417,7 +421,13 @@ private enum ActionTypeConfig {
"Actions.Types.PreIssueIdToken.ActionRequest.ExcludedParameters.Parameter",
"Actions.Types.PreIssueIdToken.ActionRequest.AllowedHeaders.Header",
"Actions.Types.PreIssueIdToken.ActionRequest.AllowedParameters.Parameter",
"Actions.Types.PreIssueIdToken.Version.RetiredUpTo");
"Actions.Types.PreIssueIdToken.Version.RetiredUpTo"),
FLOW_EXTENSION("Actions.Types.InFlowExtension.Enable",
"Actions.Types.InFlowExtension.ActionRequest.ExcludedHeaders.Header",
"Actions.Types.InFlowExtension.ActionRequest.ExcludedParameters.Parameter",
"Actions.Types.InFlowExtension.ActionRequest.AllowedHeaders.Header",
"Actions.Types.InFlowExtension.ActionRequest.AllowedParameters.Parameter",
"Actions.Types.InFlowExtension.Version.RetiredUpTo");

private final String actionTypeEnableProperty;
private final String excludedHeadersProperty;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,13 @@ public enum ErrorMessage {
"An unsupported rule has been provided for action version %s."),
ERROR_MAXIMUM_ATTRIBUTES_LIMIT_EXCEEDED("60011", "Maximum attributes limit exceeded.",
"The number of configured attributes: %s exceeds the maximum allowed limit: %s"),
ERROR_INVALID_ATTRIBUTES("60012", "Invalid attribute provided.",
"%s"),
ERROR_INVALID_ATTRIBUTES("60012", "Invalid attribute provided.", "%s"),
ERROR_EMPTY_ATTRIBUTE_VALUE("60013", "Invalid attribute provided.",
"Each attribute must be a non-empty string."),
ERROR_UNSUPPORTED_ATTRIBUTE("60014", "Unsupported attribute provided.",
"The attribute %s is not supported to be shared with the extension."),
ERROR_ACTION_NAME_ALREADY_EXISTS("60015", "Action name already exists.",
"An action with the name '%s' already exists for the given action type."),

// Server errors.
ERROR_WHILE_ADDING_ACTION("65001", "Error while adding Action.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,13 @@ public enum ActionTypes {
"PRE_ISSUE_ID_TOKEN",
"Pre Issue ID Token",
"Configure an extension point for modifying ID token via a custom service.",
Category.PRE_POST);
Category.PRE_POST),
FLOW_EXTENSION(
"inFlowExtension",
"FLOW_EXTENSION",
"In-Flow Extension",
"Configure an extension point within any flow via a custom service.",
Category.IN_FLOW_EXTENSION);

private final String pathParam;
private final String actionType;
Expand Down Expand Up @@ -131,7 +137,8 @@ public static ActionTypes[] filterByCategory(Category category) {
*/
public enum Category {
PRE_POST,
IN_FLOW
IN_FLOW,
IN_FLOW_EXTENSION
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,4 +128,5 @@ Action updateAction(String actionType, String actionId, Action action, String te
*/
Action updateActionEndpointAuthentication(String actionType, String actionId, Authentication authentication,
String tenantDomain) throws ActionMgtException;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

revert

}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@

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

import java.util.List;

/**
* This interface to the validate action in the Action management service layer.
Expand All @@ -42,6 +45,22 @@ public interface ActionValidator {
void doPreAddActionValidations(Action.ActionTypes actionType, String actionVersion, Action action)
throws ActionMgtException;

/**
* Perform pre validations on action model when creating an action, including tenant-scoped checks.
* Overload that receives the pre-fetched list of existing actions for validations such as name uniqueness.
* Default delegates to {@link #doPreAddActionValidations(Action.ActionTypes, String, Action)} for
* backward compatibility with existing implementations.
*
* @param action Action creation model.
* @param existingActionsOfType Existing actions of the same type in the tenant.
* @throws ActionMgtException if action model is invalid.
*/
default void doPreAddActionValidations(Action.ActionTypes actionType, String actionVersion, Action action,
List<ActionDTO> existingActionsOfType) throws ActionMgtException {

doPreAddActionValidations(actionType, actionVersion, action);
}

/**
* Perform pre validations on action model when updating an existing action.
* This is specifically used during HTTP PATCH operation and only validate non-null and non-empty fields.
Expand All @@ -51,4 +70,22 @@ void doPreAddActionValidations(Action.ActionTypes actionType, String actionVersi
*/
void doPreUpdateActionValidations(Action.ActionTypes actionType, String actionVersion, Action action)
throws ActionMgtException;

/**
* Perform pre validations on action model when updating an existing action, including tenant-scoped checks.
* Overload that receives the pre-fetched list of existing actions for validations such as name uniqueness.
* Default delegates to {@link #doPreUpdateActionValidations(Action.ActionTypes, String, Action)} for
* backward compatibility.
*
* @param action Action update model.
* @param excludeId Action ID to exclude from uniqueness check.
* @param existingActionsOfType Existing actions of the same type in the tenant.
* @throws ActionMgtException if action model is invalid.
*/
default void doPreUpdateActionValidations(Action.ActionTypes actionType, String actionVersion, Action action,
String excludeId, List<ActionDTO> existingActionsOfType)
throws ActionMgtException {

doPreUpdateActionValidations(actionType, actionVersion, action);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.wso2.carbon.identity.action.management.api.exception.ActionMgtException;
import org.wso2.carbon.identity.action.management.api.model.Action;
import org.wso2.carbon.identity.action.management.api.model.Action.ActionTypes;
import org.wso2.carbon.identity.action.management.api.model.ActionDTO;
import org.wso2.carbon.identity.action.management.api.model.Authentication;
import org.wso2.carbon.identity.action.management.api.service.ActionValidator;
import org.wso2.carbon.identity.action.management.internal.component.ActionMgtServiceComponentHolder;
Expand Down Expand Up @@ -93,6 +94,16 @@ public void doPreAddActionValidations(Action.ActionTypes actionType, String acti
isRulesApplicableForActionVersion(actionVersion, action);
}

@Override
public void doPreAddActionValidations(Action.ActionTypes actionType, String actionVersion, Action action,
List<ActionDTO> existingActionsOfType) throws ActionMgtException {
Comment thread
KD23243 marked this conversation as resolved.
Outdated

doPreAddActionValidations(actionType, actionVersion, action);
if (ActionTypes.FLOW_EXTENSION.equals(actionType)) {
validateActionNameUniqueness(action.getName(), null, existingActionsOfType);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Uniqueness validation is scoped only to FLOW_EXTENSION, not all action types.

The new checks are gated by ActionTypes.FLOW_EXTENSION, so duplicate-name prevention is skipped for other action types even when existingActionsOfType is available. That conflicts with the stated behavior of enforcing uniqueness within each action type at create/update.

💡 Suggested adjustment
-        if (ActionTypes.FLOW_EXTENSION.equals(actionType)) {
+        if (existingActionsOfType != null) {
             validateActionNameUniqueness(action.getName(), null, existingActionsOfType);
         }
...
-        if (action.getName() != null && ActionTypes.FLOW_EXTENSION.equals(actionType)) {
+        if (action.getName() != null && existingActionsOfType != null) {
             validateActionNameUniqueness(action.getName(), excludeId, existingActionsOfType);
         }

Also applies to: 142-144

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/impl/DefaultActionValidator.java`
around lines 102 - 104, The uniqueness check is incorrectly limited to
ActionTypes.FLOW_EXTENSION; update DefaultActionValidator so
validateActionNameUniqueness(action.getName(), null, existingActionsOfType) is
invoked for any action type when existingActionsOfType is present (i.e., remove
or replace the ActionTypes.FLOW_EXTENSION guard with a condition that checks
existingActionsOfType != null or always call the method to enforce per-type
uniqueness). Apply the same change for the other occurrence (the block around
the second use at lines 142-144) so duplicate-name prevention runs for all
action types within the same action type scope.

}

/**
* Perform pre validations on action model when updating an existing action.
* This is specifically used during HTTP PATCH operation and only validate non-null and non-empty fields.
Expand Down Expand Up @@ -122,6 +133,17 @@ public void doPreUpdateActionValidations(Action.ActionTypes actionType, String a
isRulesApplicableForActionVersion(actionVersion, action);
}

@Override
public void doPreUpdateActionValidations(Action.ActionTypes actionType, String actionVersion, Action action,
String excludeId, List<ActionDTO> existingActionsOfType)
throws ActionMgtException {

doPreUpdateActionValidations(actionType, actionVersion, action);
if (action.getName() != null && ActionTypes.FLOW_EXTENSION.equals(actionType)) {
validateActionNameUniqueness(action.getName(), excludeId, existingActionsOfType);
}
}

/**
* Perform pre validations on endpoint authentication model.
*
Expand Down Expand Up @@ -249,6 +271,27 @@ private void validateAllowedParameters(List<String> allowedParametersInAction) t
}
}

/**
* Validate that the action name is unique within the given list of existing actions.
*
* @param name Action name to validate.
* @param excludeId Action ID to exclude (for update). Null for creation.
* @param existing Existing actions of the same type in the tenant.
* @throws ActionMgtClientException If a duplicate name is found.
*/
public void validateActionNameUniqueness(String name, String excludeId, List<ActionDTO> existing)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

make this private

throws ActionMgtClientException {

boolean duplicateExists = existing.stream()
.filter(dto -> excludeId == null || !excludeId.equals(dto.getId()))
.anyMatch(dto -> name.equalsIgnoreCase(dto.getName()));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

if (duplicateExists) {
throw ActionManagementExceptionHandler.handleClientException(
ErrorMessage.ERROR_ACTION_NAME_ALREADY_EXISTS, name);
Comment thread
KD23243 marked this conversation as resolved.
Outdated
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add WARN log before throwing duplicate name exception.

Per coding guidelines, error handling boundaries (exception throws) should include log statements. This helps trace validation failures.

📝 Suggested log addition
     if (duplicateExists) {
+        LOG.warn("Action name already exists: " + name);
         throw ActionManagementExceptionHandler.handleClientException(
                 ErrorMessage.ERROR_ACTION_NAME_ALREADY_EXISTS, name);
     }

As per coding guidelines: "Suggest log statements at error handling boundaries (catch blocks, error returns)."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/impl/DefaultActionValidator.java`
around lines 289 - 292, Add a WARN log immediately before throwing the
duplicate-name client exception in DefaultActionValidator: when duplicateExists
is true, log a warning via the class logger that includes the action name
(variable name) and context, then call
ActionManagementExceptionHandler.handleClientException(ErrorMessage.ERROR_ACTION_NAME_ALREADY_EXISTS,
name) as before; ensure the logger used matches the class logger instance (e.g.,
log or LOGGER) so the warning appears in logs prior to the exception being
thrown.

}

/**
* Validate whether required fields exist.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ public static ActionDTOModelResolver getActionDTOModelResolver(Action.ActionType
return actionDTOModelResolvers.get(Action.ActionTypes.PRE_UPDATE_PASSWORD);
case PRE_ISSUE_ACCESS_TOKEN:
return actionDTOModelResolvers.get(Action.ActionTypes.PRE_ISSUE_ACCESS_TOKEN);
case FLOW_EXTENSION:
return actionDTOModelResolvers.get(Action.ActionTypes.FLOW_EXTENSION);
default:
return null;
}
Expand Down
Loading
Loading