Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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_EXTENSIONS;

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 @@ -18,6 +18,7 @@

package org.wso2.carbon.identity.action.execution.internal.component;

import org.wso2.carbon.identity.action.execution.api.service.ActionExecutorService;
import org.wso2.carbon.identity.action.management.api.service.ActionManagementService;
import org.wso2.carbon.identity.rule.evaluation.api.service.RuleEvaluationService;
import org.wso2.carbon.identity.secret.mgt.core.SecretManager;
Expand All @@ -36,6 +37,7 @@ public class ActionExecutionServiceComponentHolder {
private SecretManager secretManager;
private SecretResolveManager secretResolveManager;
private RealmService realmService;
private ActionExecutorService actionExecutorService;

private ActionExecutionServiceComponentHolder() {

Expand Down Expand Up @@ -115,4 +117,24 @@ public void setRealmService(RealmService realmService) {

this.realmService = realmService;
}

/**
* Get the ActionExecutorService instance.
*
* @return ActionExecutorService instance.
*/
public ActionExecutorService getActionExecutorService() {

return actionExecutorService;
}

/**
* Set the ActionExecutorService instance.
*
* @param actionExecutorService ActionExecutorService instance.
*/
public void setActionExecutorService(ActionExecutorService actionExecutorService) {

this.actionExecutorService = actionExecutorService;
}
}

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 @@ -197,8 +197,13 @@ private Action getActionByActionId(ActionType actionType, String actionId, Strin
throws ActionExecutionException {

try {
return ActionExecutionServiceComponentHolder.getInstance().getActionManagementService().getActionByActionId(
Action.ActionTypes.valueOf(actionType.name()).getPathParam(), actionId, tenantDomain);
Action action = ActionExecutionServiceComponentHolder.getInstance().getActionManagementService()
.getActionByActionId(Action.ActionTypes.valueOf(actionType.name()).getPathParam(), actionId,
tenantDomain);
Comment thread
ThaminduR marked this conversation as resolved.
Outdated
if (action == null) {
throw new ActionExecutionRuntimeException("No action found for action Id: " + actionId);
}
return action;
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.

I think better not to do this change here. Null check should be handled in the consuming service level.

If you are changing this you have to refactor all of the usages of this method. Specially in Custom Authenticator invocation. Better not to touch that at this moment

} 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_EXTENSIONS:
return isActionTypeEnabled(ActionTypeConfig.FLOW_EXTENSIONS.getActionTypeEnableProperty());
Comment thread
KD23243 marked this conversation as resolved.
Outdated
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_EXTENSIONS:
return getVersion(ActionTypeConfig.FLOW_EXTENSIONS.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_EXTENSIONS("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 @@ -44,7 +44,13 @@ public enum ErrorMessage {
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"),
"The provided %s attribute is not available in the system."),
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."),

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.

Remove this unintended diff.

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_EXTENSIONS(
"inFlowExtension",
"FLOW_EXTENSIONS",
"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);
}
}
Loading
Loading