Feature - In-Flow Extension#8108
Open
KD23243 wants to merge 17 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds FLOW_EXTENSION action type, config, and management logic. Introduces a new flow-extension module (executor, request builder, response processor, crypto, models, metadata). Integrates with the flow engine (prompt handling, user id, errors). Updates build/poms, identity config templates, and comprehensive tests/resources. ChangesIn-flow extensions end-to-end
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
- Introduced InFlowExtensionExecutor to handle execution of In-Flow Extension actions during flow execution. - Created InFlowExtensionRequestBuilder to build action execution requests from FlowContext. - Implemented InFlowExtensionResponseProcessor to process responses from In-Flow Extension actions, handling context updates for properties, user claims, and user inputs. - Added OperationExecutionResult class to encapsulate the result of operation executions. - Updated ActionExecutorConfig to include configuration for In-Flow Extension actions. - Enhanced Action model to support In-Flow Extension action type. - Modified ActionManagementConfig to manage versions and properties for In-Flow Extension actions.
KD23243
force-pushed
the
feature/in-flow-extensions-master
branch
from
5f288fb to
88786f8
Compare
KD23243
force-pushed
the
feature/in-flow-extensions-master
branch
from
88786f8 to
1913bbb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (2)
components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/internal/service/impl/ActionExecutorServiceImpl.java (2)
207-209:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd error logging in exception handler.
The catch block handles ActionMgtException but doesn't log the error before wrapping and rethrowing. Per coding guidelines, error handling boundaries (catch blocks) should include log statements.
Suggested logging addition
} catch (ActionMgtException e) { + log.error("Error retrieving action for actionId: " + actionId + " of type: " + + actionType.name() + " in tenant: " + tenantDomain, e); throw new ActionExecutionException("Error occurred while retrieving action by action Id.", e); }As per coding guidelines, error handling boundaries should have log statements. Based on learnings, logging the exception object as the second argument is acceptable in this repository.
🤖 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 207 - 209, In the catch block inside ActionExecutorServiceImpl where ActionMgtException is caught and rethrown as an ActionExecutionException, add an error log statement before throwing; use the class logger (e.g., log or logger) to call log.error("Error occurred while retrieving action by action Id.", e) so the original exception is recorded, then throw the new ActionExecutionException as currently implemented.
200-206:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd error logging when action is not found.
The method retrieves an action via a DB call but doesn't log when the action is null before throwing an exception. Per coding guidelines, error handling boundaries should have log statements.
Suggested logging addition
Action action = ActionExecutionServiceComponentHolder.getInstance().getActionManagementService() .getActionByActionId(Action.ActionTypes.valueOf(actionType.name()).getPathParam(), actionId, tenantDomain); if (action == null) { + log.error("No action found for action Id: " + actionId + " in tenant: " + tenantDomain); throw new ActionExecutionRuntimeException("No action found for action Id: " + actionId); }As per coding guidelines, functions performing significant operations (DB calls) and error handling boundaries should include log statements.
🤖 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 200 - 206, The null-action branch in ActionExecutorServiceImpl (after calling ActionExecutionServiceComponentHolder.getInstance().getActionManagementService().getActionByActionId(...)) throws an ActionExecutionRuntimeException without logging; add an error log statement using the class logger in ActionExecutorServiceImpl right before throwing (include actionId, actionType.name() and tenantDomain in the message and/or as structured fields) so failures from getActionByActionId are recorded for diagnostics.
🧹 Nitpick comments (8)
components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/JWEEncryptionUtil.java (1)
97-100: ⚡ Quick winAdd failure logs at crypto/key error boundaries.
Handled failures in
encrypt,decrypt,getPrivateKey, andparsePEMCertificatecurrently throw without logging. Add conciseERROR/WARNlogs (without plaintext/certificate content) before rethrowing to improve operational diagnosis.As per coding guidelines, “Add logs around major executions, meaningful decision points, and handled exceptional branches.”
Also applies to: 123-134, 169-172, 223-235
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/JWEEncryptionUtil.java` around lines 97 - 100, Add concise ERROR/WARN log statements at the crypto/key error boundaries before rethrowing in the methods encrypt, decrypt, getPrivateKey, and parsePEMCertificate: in each catch block (e.g., the JOSEException catch that currently throws ActionExecutionException) call the class logger (e.g., log.error or log.warn) with a short contextual message like "Failed to JWE-encrypt data" or "Failed to parse PEM certificate" and include the exception (e) as the throwable argument, but do NOT log any plaintext, keys, or certificate contents; then rethrow the same ActionExecutionException (or original exception) as currently done. Ensure each method uses an appropriate log level (ERROR for failures, WARN for recoverable issues) and avoids sensitive data in messages.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/pom.xml (1)
223-236: 💤 Low valueCheckstyle and FindBugs are disabled for this new module.
Static analysis tools are skipped. Consider enabling these checks before merging to maintain code quality consistency across the project.
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/pom.xml` around lines 223 - 236, The pom currently disables static analysis by setting <skip>true</skip> for the maven-checkstyle-plugin and findbugs-maven-plugin; remove or set those <skip> entries to false (or delete the <skip> element) for the plugins named maven-checkstyle-plugin and findbugs-maven-plugin so checkstyle and findbugs run for this module, and ensure any required plugin configuration (ruleset/checkstyle.xml or findbugs excludes) is present or inherited from the parent POM.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionExecutor.java (1)
75-142: 💤 Low valueAdd INFO log for In-Flow Extension action execution.
The
executemethod performs a significant operation (invoking external services). Per logging guidelines, major actions should be logged at INFO level for production visibility.Suggested log addition
public ExecutorResponse execute(FlowExecutionContext context) throws FlowEngineException { String actionId = getMetadataValue(context, InFlowExtensionConstants.ACTION_ID_METADATA_KEY); if (actionId == null || actionId.isEmpty()) { triggerDiagnosticFailure(null, "In-Flow Extension action execution failed: action ID is not configured."); return buildErrorResponse("Extension is not configured.", "The In-Flow Extension action is missing required configuration. " + "Contact your administrator."); } + LOG.info("Executing In-Flow Extension action: " + actionId); + if (LOG.isDebugEnabled()) {As per coding guidelines: "use INFO for major actions/state transitions" from LOG_ENHANCEMENT_GUIDELINES.md.
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionExecutor.java` around lines 75 - 142, Add an INFO-level log in the InFlowExtensionExecutor.execute method to record the major action start; specifically, after validating actionId and before calling actionExecutorService.execute (or right after obtaining actionExecutorService), log an INFO message including actionId, context.getFlowType(), and context.getTenantDomain() so production systems have visibility into the In-Flow Extension invocation; update the logging block that currently uses LOG.debug (or add a new LOG.info call near it) in the execute method to include those identifiers.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/PathTypeAnnotationUtil.java (1)
128-134: 💤 Low valueEdge case: malformed annotation
{[}could cause unexpected behavior.When
annotation.startsWith("["), the code assumesannotation.endsWith("]")as well. A malformed annotation like{[}would produceinner = ""(empty string after substring), which is handled gracefully. However,{[abc}would produceinner = "abc"without the closing bracket check.Consider adding validation for balanced brackets if strict parsing is required.
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/PathTypeAnnotationUtil.java` around lines 128 - 134, The current block in PathTypeAnnotationUtil that checks annotation.startsWith("[") assumes a trailing ']'—add a balanced-bracket validation before taking substring: check annotation.endsWith("]") and only compute inner = annotation.substring(1, annotation.length()-1) when both start and end brackets are present; if the closing bracket is missing (malformed like "{[abc}"), skip the complex-array branch (do not call tryParseJsonString(value) for that case) and fall through to the non-array handling or return a safe default/error. Keep references to the existing symbols: the annotation variable, the startsWith/endsWith checks, and tryParseJsonString so the change is localized and avoids parsing malformed annotations.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionRequestBuilder.java (3)
79-108: 💤 Low valueAdd INFO log for the In-Flow Extension request build action.
Per logging guidelines, major actions should be logged at INFO level. The
buildActionExecutionRequestmethod is a significant operation that constructs requests for external service invocation.Suggested log addition
public ActionExecutionRequest buildActionExecutionRequest(FlowContext flowContext, ActionExecutionRequestContext actionExecutionContext) throws ActionExecutionRequestBuilderException { FlowExecutionContext execCtx = getFlowExecutionContextOrThrow(flowContext); + if (LOG.isInfoEnabled()) { + LOG.info("Building In-Flow Extension request for flowType: " + execCtx.getFlowType()); + } ResolvedActionConfig resolvedActionConfig = resolveActionConfig(actionExecutionContext, execCtx.getFlowType());Based on learnings: "use INFO for major actions/state transitions" from LOG_ENHANCEMENT_GUIDELINES.md.
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionRequestBuilder.java` around lines 79 - 108, Add an INFO-level log in InFlowExtensionRequestBuilder.buildActionExecutionRequest to record the major operation of building an in-flow extension request; log should be emitted once the request details are known (e.g., after computing execCtx, exposeResolution/effectiveExposePaths, modifyPaths and allowedOperations or after building the final event/request payload) and include key context like execCtx.getFlowType(), resolvedActionConfig identifier (if available), exposeResolution.getEffectiveExposePaths(), and modifyPaths so callers can trace request creation; use the class logger (e.g., the existing logger instance in InFlowExtensionRequestBuilder) and a single concise message indicating the request was built with those contextual fields.
627-637: 💤 Low valueCredential
char[]toStringconversion leaves plaintext in memory.The credential is converted to a
String(line 628) before the originalchar[]is zeroed (line 629). TheStringobject cannot be cleared from memory and remains until garbage collected. This is a known limitation when encryption requiresStringinput.This is acceptable if the JWE encryption API requires
String, but be aware the plaintext persists in memory longer than intended.🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionRequestBuilder.java` around lines 627 - 637, The code converts credValue (char[]) into a String (plaintext) before zeroing the char[] which leaves the plaintext resident in the immutable String; to fix, avoid creating a String at all by updating the call site to use a char[]-based API (prefer an overload of toEncryptedOrPlainCredentialChars that accepts char[] or add one) and pass credValue directly, zero the char[] immediately after use (java.util.Arrays.fill(credValue, '\0')), and only then put the result into filteredCredentials; if an API that requires String cannot be changed, document this limitation clearly and minimize exposure by restricting the String scope and nulling references as soon as possible while preferring a char[]-based encryption helper.
793-799: 💤 Low valueCatching generic
Exceptionis overly broad.The catch block captures all exceptions from
JWEEncryptionUtil.encrypt. Consider catching the specific exceptions thatencryptcan throw to avoid masking unexpected runtime errors.🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionRequestBuilder.java` around lines 793 - 799, The catch of generic Exception around JWEEncryptionUtil.encrypt in InFlowExtensionRequestBuilder should be narrowed: update the try-catch to catch only the specific checked exceptions declared by JWEEncryptionUtil.encrypt (for example the library's encryption/JOSE or crypto checked exceptions) and rethrow them wrapped in ActionExecutionRequestBuilderException (preserving the original exception as the cause); do not swallow RuntimeException or Error — allow those to propagate. Locate the call to JWEEncryptionUtil.encrypt and replace the catch(Exception e) with catches for the concrete exception types the encrypt method declares.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.java (1)
406-523: ⚡ Quick winAdd operational logs around certificate lifecycle calls and failure boundaries.
These methods perform external certificate-service operations (add/get/update/delete), but there are no logs at invocation/failure points. Add concise INFO/WARN logs (without sensitive payloads) so production troubleshooting has traceability.
As per coding guidelines: “Flag functions or methods that perform significant operations (DB calls, API calls, auth, payments) but lack log statements” and “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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.java` around lines 406 - 523, Add concise non-sensitive logging around certificate service calls in the certificate lifecycle methods: insert INFO logs before calling certificateManagementService in handleCertificateAdd (log action id and that add is starting with certName), handleCertificateGet (log action id and certId lookup), handleCertificateUpdate (log action id, whether adding/updating/deleting and the existing/new-state decisions), and handleCertificateDelete (log action id and certId deletion); also add WARN/ERROR logs in each catch block (in handleCertificateAdd, handleCertificateGet, handleCertificateUpdate, handleCertificateDelete) including the action id and exception message/stack trace but not the certificate PEM/content or full certificate object, and ensure when you store/forward the certificate ID via properties.put(CERTIFICATE, ...) you only log the ID if needed and never log sensitive certificate content.
🤖 Prompt for all review comments with 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.
Inline comments:
In
`@components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/api/model/User.java`:
- Around line 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).
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 line 285-287: The uniqueness check in DefaultActionValidator uses
existing.stream() and dto.getName() without null checks, which can cause NPEs;
update the uniqueness validation to defensively handle nulls by first guarding
against a null existing collection (treat as empty) and then in the stream
filter/map ensure dto != null and dto.getName() is null-safe (e.g., compare name
with dto.getName() using a null-safe equalsIgnoreCase or skip null names) while
keeping the excludeId logic intact so duplicateExists is computed without
throwing.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/graph/TaskExecutionNode.java`:
- Line 245: Replace the raw username in the WARN log inside TaskExecutionNode
(the LOG.warn call that currently references the username variable) to avoid
logging PII; update the message to use a non-PII identifier (e.g., a generic
message like "Failed to resolve userId from user store" or a previously-recorded
correlation id/trace id) or a hashed/masked form of username, and ensure the
exception e is still logged for debugging; do not include the plain username
string in the log.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/model/FlowUser.java`:
- Around line 126-138: The class FlowUser contains a duplicate method
addClaims(Map<String, String>) causing a compile error; remove the redundant
declaration so only one addClaims implementation remains (or if behavior should
differ, rename one method and adjust its implementation accordingly). Locate the
methods around setClaims(Map<String, String>) and addClaims(Map<String, String>)
in the FlowUser class and delete or rename the duplicate to ensure a single,
correct addClaims method.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionResponseProcessor.java`:
- Around line 233-239: After extracting propertyName with
extractNameFromPath(operation.getPath(),
InFlowExtensionConstants.PROPERTIES_PATH_PREFIX), enforce that the path is flat
by rejecting any propertyName that contains a slash or is otherwise malformed;
update the conditional that currently checks (propertyName == null ||
propertyName.isEmpty()) to also return an OperationExecutionResult with
Status.FAILURE (using the same OperationExecutionResult constructor) when
propertyName.contains("/") (or starts/ends with "/" as appropriate) so nested
paths like "/properties/a/b" are rejected.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/internal/InFlowExtensionDataHolder.java`:
- Around line 36-39: The mutable OSGi service fields in
InFlowExtensionDataHolder (e.g., actionExecutorService, actionManagementService,
certificateManagementService, claimMetadataManagementService and the other
service fields around lines 50-88) are published without visibility guarantees;
change their storage to thread-safe holders (mark each field volatile or wrap
them in AtomicReference) and update corresponding bind/unbind setters and
getters to write/read the volatile/AtomicReference so callers always see the
latest value (for AtomicReference use set/get; for volatile use simple
assignment/return). Ensure all service fields in the class follow the same
pattern so bind/unbind and readers have proper cross-thread visibility.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/internal/InFlowExtensionServiceComponent.java`:
- Around line 59-78: The activation currently swallows all exceptions in the
try-catch around the registerService calls in InFlowExtensionServiceComponent
(the bundleContext.registerService(...) calls and the catch (Throwable e)
block); instead, capture each ServiceRegistration returned by
bundleContext.registerService for Executor, ActionExecutionRequestBuilder,
ActionExecutionResponseProcessor, ActionConverter, and ActionDTOModelResolver,
and if any exception occurs, unregister any successfully-registered services
(call unregister on each non-null ServiceRegistration) and then rethrow the
exception (wrap in a RuntimeException or rethrow the caught Throwable) so the
component activation fails rather than leaving partial registrations.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionConverter.java`:
- Around line 123-133: In InFlowExtensionActionConverter, guard against null
AccessConfig values when iterating the overrides map: inside the for-loop over
overrides.entrySet() check if overrideConfig (the entry.getValue()) is null and
skip (or log) before calling overrideConfig.getExpose() or
overrideConfig.getModify(); only construct and put ActionProperty instances for
ACCESS_CONFIG_EXPOSE_PREFIX + flowType and ACCESS_CONFIG_MODIFY_PREFIX +
flowType when overrideConfig is non-null and the respective getters return
non-null.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeNode.java`:
- Around line 50-57: The constructor is exposing node state to external mutation
because it wraps the builder's lists directly; change the assignments for
allowedOperations and children in InFlowExtensionContextTreeNode to copy the
builder lists before wrapping (e.g., create a new ArrayList from
b.allowedOperations / b.children and then Collections.unmodifiableList(...) ) so
the node holds an immutable snapshot, and apply the same fix to the other
similar places (the other constructors/assignments for
allowedOperations/children around the other occurrences referenced).
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/AccessConfig.java`:
- Around line 61-64: The AccessConfig constructor currently assigns unmodifiable
wrappers over the incoming lists (expose/modify) which still reference the
original mutable lists; change it to defensively copy inputs before wrapping:
for both expose and modify in the AccessConfig(List<ContextPath> expose,
List<ContextPath> modify) constructor, if the parameter is non-null create a new
ArrayList<>(expose) / new ArrayList<>(modify) and then wrap that copy with
Collections.unmodifiableList(...) (leave null as null) so the
AccessConfig.expose and AccessConfig.modify fields cannot be affected by later
external mutations.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/FlowContextHandoverConfig.java`:
- Around line 44-45: The class currently sets includedAttributes and
includedUserAttributes to Collections.unmodifiableSet(...) which only wraps the
original sets; change the of(...) factory/constructor in
FlowContextHandoverConfig to first defensively copy inputs (e.g., new
HashSet<>(attrs)) and then wrap the copy with Collections.unmodifiableSet(), and
do the same for includedUserAttributes; likewise apply the same defensive-copy +
unmodifiable wrap for the other set fields referenced around lines 60-64 (e.g.,
excludedAttributes, excludedUserAttributes) and handle null inputs by using an
empty set so the internal immutable fields cannot be affected by external
mutations.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/resources/testng.xml`:
- Around line 36-41: The testng suite "in-flow-extension-model-tests" is missing
the InFlowExtensionPathUtilTest; update the <test
name="in-flow-extension-model-tests"> block to include the class entry for
org.wso2.carbon.identity.flow.extensions.util.InFlowExtensionPathUtilTest (add a
<class
name="org.wso2.carbon.identity.flow.extensions.util.InFlowExtensionPathUtilTest"/>
alongside AccessConfigTest, OperationExecutionResultTest, and
InFlowExtensionEventTest) so the changed test class is executed during module
test runs.
---
Duplicate comments:
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 line 207-209: In the catch block inside ActionExecutorServiceImpl where
ActionMgtException is caught and rethrown as an ActionExecutionException, add an
error log statement before throwing; use the class logger (e.g., log or logger)
to call log.error("Error occurred while retrieving action by action Id.", e) so
the original exception is recorded, then throw the new ActionExecutionException
as currently implemented.
- Around line 200-206: The null-action branch in ActionExecutorServiceImpl
(after calling
ActionExecutionServiceComponentHolder.getInstance().getActionManagementService().getActionByActionId(...))
throws an ActionExecutionRuntimeException without logging; add an error log
statement using the class logger in ActionExecutorServiceImpl right before
throwing (include actionId, actionType.name() and tenantDomain in the message
and/or as structured fields) so failures from getActionByActionId are recorded
for diagnostics.
---
Nitpick comments:
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/pom.xml`:
- Around line 223-236: The pom currently disables static analysis by setting
<skip>true</skip> for the maven-checkstyle-plugin and findbugs-maven-plugin;
remove or set those <skip> entries to false (or delete the <skip> element) for
the plugins named maven-checkstyle-plugin and findbugs-maven-plugin so
checkstyle and findbugs run for this module, and ensure any required plugin
configuration (ruleset/checkstyle.xml or findbugs excludes) is present or
inherited from the parent POM.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionExecutor.java`:
- Around line 75-142: Add an INFO-level log in the
InFlowExtensionExecutor.execute method to record the major action start;
specifically, after validating actionId and before calling
actionExecutorService.execute (or right after obtaining actionExecutorService),
log an INFO message including actionId, context.getFlowType(), and
context.getTenantDomain() so production systems have visibility into the In-Flow
Extension invocation; update the logging block that currently uses LOG.debug (or
add a new LOG.info call near it) in the execute method to include those
identifiers.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionRequestBuilder.java`:
- Around line 79-108: Add an INFO-level log in
InFlowExtensionRequestBuilder.buildActionExecutionRequest to record the major
operation of building an in-flow extension request; log should be emitted once
the request details are known (e.g., after computing execCtx,
exposeResolution/effectiveExposePaths, modifyPaths and allowedOperations or
after building the final event/request payload) and include key context like
execCtx.getFlowType(), resolvedActionConfig identifier (if available),
exposeResolution.getEffectiveExposePaths(), and modifyPaths so callers can trace
request creation; use the class logger (e.g., the existing logger instance in
InFlowExtensionRequestBuilder) and a single concise message indicating the
request was built with those contextual fields.
- Around line 627-637: The code converts credValue (char[]) into a String
(plaintext) before zeroing the char[] which leaves the plaintext resident in the
immutable String; to fix, avoid creating a String at all by updating the call
site to use a char[]-based API (prefer an overload of
toEncryptedOrPlainCredentialChars that accepts char[] or add one) and pass
credValue directly, zero the char[] immediately after use
(java.util.Arrays.fill(credValue, '\0')), and only then put the result into
filteredCredentials; if an API that requires String cannot be changed, document
this limitation clearly and minimize exposure by restricting the String scope
and nulling references as soon as possible while preferring a char[]-based
encryption helper.
- Around line 793-799: The catch of generic Exception around
JWEEncryptionUtil.encrypt in InFlowExtensionRequestBuilder should be narrowed:
update the try-catch to catch only the specific checked exceptions declared by
JWEEncryptionUtil.encrypt (for example the library's encryption/JOSE or crypto
checked exceptions) and rethrow them wrapped in
ActionExecutionRequestBuilderException (preserving the original exception as the
cause); do not swallow RuntimeException or Error — allow those to propagate.
Locate the call to JWEEncryptionUtil.encrypt and replace the catch(Exception e)
with catches for the concrete exception types the encrypt method declares.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/JWEEncryptionUtil.java`:
- Around line 97-100: Add concise ERROR/WARN log statements at the crypto/key
error boundaries before rethrowing in the methods encrypt, decrypt,
getPrivateKey, and parsePEMCertificate: in each catch block (e.g., the
JOSEException catch that currently throws ActionExecutionException) call the
class logger (e.g., log.error or log.warn) with a short contextual message like
"Failed to JWE-encrypt data" or "Failed to parse PEM certificate" and include
the exception (e) as the throwable argument, but do NOT log any plaintext, keys,
or certificate contents; then rethrow the same ActionExecutionException (or
original exception) as currently done. Ensure each method uses an appropriate
log level (ERROR for failures, WARN for recoverable issues) and avoids sensitive
data in messages.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/PathTypeAnnotationUtil.java`:
- Around line 128-134: The current block in PathTypeAnnotationUtil that checks
annotation.startsWith("[") assumes a trailing ']'—add a balanced-bracket
validation before taking substring: check annotation.endsWith("]") and only
compute inner = annotation.substring(1, annotation.length()-1) when both start
and end brackets are present; if the closing bracket is missing (malformed like
"{[abc}"), skip the complex-array branch (do not call tryParseJsonString(value)
for that case) and fall through to the non-array handling or return a safe
default/error. Keep references to the existing symbols: the annotation variable,
the startsWith/endsWith checks, and tryParseJsonString so the change is
localized and avoids parsing malformed annotations.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.java`:
- Around line 406-523: Add concise non-sensitive logging around certificate
service calls in the certificate lifecycle methods: insert INFO logs before
calling certificateManagementService in handleCertificateAdd (log action id and
that add is starting with certName), handleCertificateGet (log action id and
certId lookup), handleCertificateUpdate (log action id, whether
adding/updating/deleting and the existing/new-state decisions), and
handleCertificateDelete (log action id and certId deletion); also add WARN/ERROR
logs in each catch block (in handleCertificateAdd, handleCertificateGet,
handleCertificateUpdate, handleCertificateDelete) including the action id and
exception message/stack trace but not the certificate PEM/content or full
certificate object, and ensure when you store/forward the certificate ID via
properties.put(CERTIFICATE, ...) you only log the ID if needed and never log
sensitive certificate content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 453bc94e-d02d-4b69-9f99-2c6b89ffae87
📒 Files selected for processing (72)
.gitignorecomponents/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/api/model/ActionType.javacomponents/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/api/model/User.javacomponents/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/internal/component/ActionExecutionServiceComponentHolder.javacomponents/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/internal/service/impl/ActionExecutorServiceImpl.javacomponents/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/internal/util/ActionExecutorConfig.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/constant/ErrorMessage.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/model/Action.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/ActionManagementService.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/ActionValidator.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/impl/DefaultActionValidator.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/dao/impl/ActionDTOModelResolverFactory.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/service/impl/ActionConverterFactory.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/service/impl/ActionManagementServiceImpl.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/util/ActionManagementConfig.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/model/ActionTypesTest.javacomponents/entitlement/pom.xmlcomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/pom.xmlcomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/Constants.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/core/FlowExecutionEngine.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/graph/TaskExecutionNode.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/model/ExecutorResponse.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/model/FlowUser.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/test/java/org/wso2/carbon/identity/flow/execution/engine/validation/InputValidationServiceTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/test/resources/testng.xmlcomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/pom.xmlcomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/InFlowExtensionConstants.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionExecutor.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionRequestBuilder.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionResponseProcessor.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/JWEEncryptionUtil.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/PathTypeAnnotationUtil.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/internal/InFlowExtensionDataHolder.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/internal/InFlowExtensionServiceComponent.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionConverter.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeBuilder.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeMetadata.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeNode.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeService.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/AccessConfig.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/ContextPath.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/Encryption.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/FlowContextHandoverConfig.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/InFlowExtensionAction.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/InFlowExtensionEvent.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/InFlowExtensionFlow.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/InFlowExtensionRequest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/InFlowUser.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/OperationExecutionResult.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/util/InFlowExtensionContextFilterUtil.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/util/InFlowExtensionPathUtil.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/InFlowExtensionTestUtils.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionExecutorTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionRequestBuilderTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionResponseProcessorTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/executor/PathTypeAnnotationUtilTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/metadata/FlowContextHandoverConfigTestHelper.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeBuilderTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/model/AccessConfigTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/model/InFlowExtensionEventTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/model/OperationExecutionResultTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/util/InFlowExtensionPathUtilTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/resources/repository/conf/carbon.xmlcomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/resources/testng.xmlcomponents/flow-orchestration-framework/pom.xmlfeatures/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions.server.feature/pom.xmlfeatures/flow-orchestration-framework/org.wso2.carbon.identity.flow.orchestration.framework.feature/pom.xmlfeatures/flow-orchestration-framework/pom.xmlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.jsonpom.xml
Comment on lines
+79
to
80
| this.userCredentials.putAll(builder.userCredentials); | ||
| this.groups.addAll(builder.groups); |
Contributor
There was a problem hiding this comment.
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).
Comment on lines
+233
to
+239
| String propertyName = extractNameFromPath(operation.getPath(), | ||
| InFlowExtensionConstants.PROPERTIES_PATH_PREFIX); | ||
|
|
||
| if (propertyName == null || propertyName.isEmpty()) { | ||
| return new OperationExecutionResult(operation, OperationExecutionResult.Status.FAILURE, | ||
| "Invalid property path. Property name is required."); | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce flat /properties/<name> paths explicitly.
The code accepts nested property paths (for example, /properties/a/b) and stores key a/b, even though this handler documents flat paths only. Add a slash check after extraction.
Suggested fix
if (propertyName == null || propertyName.isEmpty()) {
return new OperationExecutionResult(operation, OperationExecutionResult.Status.FAILURE,
"Invalid property path. Property name is required.");
}
+ if (propertyName.contains("/")) {
+ return new OperationExecutionResult(operation, OperationExecutionResult.Status.FAILURE,
+ "Invalid property path. Only flat property paths are supported.");
+ }🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionResponseProcessor.java`
around lines 233 - 239, After extracting propertyName with
extractNameFromPath(operation.getPath(),
InFlowExtensionConstants.PROPERTIES_PATH_PREFIX), enforce that the path is flat
by rejecting any propertyName that contains a slash or is otherwise malformed;
update the conditional that currently checks (propertyName == null ||
propertyName.isEmpty()) to also return an OperationExecutionResult with
Status.FAILURE (using the same OperationExecutionResult constructor) when
propertyName.contains("/") (or starts/ends with "/" as appropriate) so nested
paths like "/properties/a/b" are rejected.
Comment on lines
+50
to
+57
| this.allowedOperations = b.allowedOperations != null | ||
| ? Collections.unmodifiableList(b.allowedOperations) : Collections.emptyList(); | ||
| this.readOnly = b.readOnly; | ||
| this.replaceable = b.replaceable; | ||
| this.dynamicEntryAllowed = b.dynamicEntryAllowed; | ||
| this.dynamicEntryType = b.dynamicEntryType; | ||
| this.children = b.children != null | ||
| ? Collections.unmodifiableList(b.children) : Collections.emptyList(); |
Contributor
There was a problem hiding this comment.
Copy lists before wrapping to keep node instances immutable.
Current wrapping is shallow over shared references. Mutating the original allowedOperations/children lists after build mutates the node state.
Proposed fix
- this.allowedOperations = b.allowedOperations != null
- ? Collections.unmodifiableList(b.allowedOperations) : Collections.emptyList();
+ this.allowedOperations = b.allowedOperations != null
+ ? Collections.unmodifiableList(new java.util.ArrayList<>(b.allowedOperations))
+ : Collections.emptyList();
...
- this.children = b.children != null
- ? Collections.unmodifiableList(b.children) : Collections.emptyList();
+ this.children = b.children != null
+ ? Collections.unmodifiableList(new java.util.ArrayList<>(b.children))
+ : Collections.emptyList();Also applies to: 164-167, 194-197
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeNode.java`
around lines 50 - 57, The constructor is exposing node state to external
mutation because it wraps the builder's lists directly; change the assignments
for allowedOperations and children in InFlowExtensionContextTreeNode to copy the
builder lists before wrapping (e.g., create a new ArrayList from
b.allowedOperations / b.children and then Collections.unmodifiableList(...) ) so
the node holds an immutable snapshot, and apply the same fix to the other
similar places (the other constructors/assignments for
allowedOperations/children around the other occurrences referenced).
Comment on lines
+61
to
+64
| public AccessConfig(List<ContextPath> expose, List<ContextPath> modify) { | ||
|
|
||
| this.expose = expose != null ? Collections.unmodifiableList(expose) : null; | ||
| this.modify = modify != null ? Collections.unmodifiableList(modify) : null; |
Contributor
There was a problem hiding this comment.
Defensively copy constructor inputs to preserve config immutability.
At Line 63 and Line 64, Collections.unmodifiableList(expose/modify) wraps the same incoming list instances. If callers mutate those original lists later, AccessConfig changes unexpectedly at runtime.
Proposed fix
+import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;
@@
- this.expose = expose != null ? Collections.unmodifiableList(expose) : null;
- this.modify = modify != null ? Collections.unmodifiableList(modify) : null;
+ this.expose = expose != null ? Collections.unmodifiableList(new ArrayList<>(expose)) : null;
+ this.modify = modify != null ? Collections.unmodifiableList(new ArrayList<>(modify)) : null;📝 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
| public AccessConfig(List<ContextPath> expose, List<ContextPath> modify) { | |
| this.expose = expose != null ? Collections.unmodifiableList(expose) : null; | |
| this.modify = modify != null ? Collections.unmodifiableList(modify) : null; | |
| public AccessConfig(List<ContextPath> expose, List<ContextPath> modify) { | |
| this.expose = expose != null ? Collections.unmodifiableList(new ArrayList<>(expose)) : null; | |
| this.modify = modify != null ? Collections.unmodifiableList(new ArrayList<>(modify)) : null; |
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/AccessConfig.java`
around lines 61 - 64, The AccessConfig constructor currently assigns
unmodifiable wrappers over the incoming lists (expose/modify) which still
reference the original mutable lists; change it to defensively copy inputs
before wrapping: for both expose and modify in the
AccessConfig(List<ContextPath> expose, List<ContextPath> modify) constructor, if
the parameter is non-null create a new ArrayList<>(expose) / new
ArrayList<>(modify) and then wrap that copy with
Collections.unmodifiableList(...) (leave null as null) so the
AccessConfig.expose and AccessConfig.modify fields cannot be affected by later
external mutations.
Comment on lines
+44
to
+45
| this.includedAttributes = Collections.unmodifiableSet(includedAttributes); | ||
| this.includedUserAttributes = Collections.unmodifiableSet(includedUserAttributes); |
Contributor
There was a problem hiding this comment.
Defensive-copy input sets to preserve immutability.
Collections.unmodifiableSet(includedAttributes) and Collections.unmodifiableSet(includedUserAttributes) only wrap references. If caller mutates attrs/userAttrs after of(...), this config changes unexpectedly.
Proposed fix
- this.includedAttributes = Collections.unmodifiableSet(includedAttributes);
- this.includedUserAttributes = Collections.unmodifiableSet(includedUserAttributes);
+ this.includedAttributes = Collections.unmodifiableSet(new java.util.HashSet<>(includedAttributes));
+ this.includedUserAttributes = Collections.unmodifiableSet(new java.util.HashSet<>(includedUserAttributes));Also applies to: 60-64
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/FlowContextHandoverConfig.java`
around lines 44 - 45, The class currently sets includedAttributes and
includedUserAttributes to Collections.unmodifiableSet(...) which only wraps the
original sets; change the of(...) factory/constructor in
FlowContextHandoverConfig to first defensively copy inputs (e.g., new
HashSet<>(attrs)) and then wrap the copy with Collections.unmodifiableSet(), and
do the same for includedUserAttributes; likewise apply the same defensive-copy +
unmodifiable wrap for the other set fields referenced around lines 60-64 (e.g.,
excludedAttributes, excludedUserAttributes) and handle null inputs by using an
empty set so the internal immutable fields cannot be affected by external
mutations.
Comment on lines
+36
to
+41
| <test name="in-flow-extension-model-tests"> | ||
| <classes> | ||
| <class name="org.wso2.carbon.identity.flow.extensions.model.AccessConfigTest"/> | ||
| <class name="org.wso2.carbon.identity.flow.extensions.model.OperationExecutionResultTest"/> | ||
| <class name="org.wso2.carbon.identity.flow.extensions.model.InFlowExtensionEventTest"/> | ||
| </classes> |
Contributor
There was a problem hiding this comment.
Add InFlowExtensionPathUtilTest to this suite.
At Line 37, this suite omits org.wso2.carbon.identity.flow.extensions.util.InFlowExtensionPathUtilTest, so that changed test class is likely not executed in this module test run.
Suggested update
<test name="in-flow-extension-model-tests">
<classes>
<class name="org.wso2.carbon.identity.flow.extensions.model.AccessConfigTest"/>
<class name="org.wso2.carbon.identity.flow.extensions.model.OperationExecutionResultTest"/>
<class name="org.wso2.carbon.identity.flow.extensions.model.InFlowExtensionEventTest"/>
+ <class name="org.wso2.carbon.identity.flow.extensions.util.InFlowExtensionPathUtilTest"/>
</classes>
</test>📝 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
| <test name="in-flow-extension-model-tests"> | |
| <classes> | |
| <class name="org.wso2.carbon.identity.flow.extensions.model.AccessConfigTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extensions.model.OperationExecutionResultTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extensions.model.InFlowExtensionEventTest"/> | |
| </classes> | |
| <test name="in-flow-extension-model-tests"> | |
| <classes> | |
| <class name="org.wso2.carbon.identity.flow.extensions.model.AccessConfigTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extensions.model.OperationExecutionResultTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extensions.model.InFlowExtensionEventTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extensions.util.InFlowExtensionPathUtilTest"/> | |
| </classes> |
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/resources/testng.xml`
around lines 36 - 41, The testng suite "in-flow-extension-model-tests" is
missing the InFlowExtensionPathUtilTest; update the <test
name="in-flow-extension-model-tests"> block to include the class entry for
org.wso2.carbon.identity.flow.extensions.util.InFlowExtensionPathUtilTest (add a
<class
name="org.wso2.carbon.identity.flow.extensions.util.InFlowExtensionPathUtilTest"/>
alongside AccessConfigTest, OperationExecutionResultTest, and
InFlowExtensionEventTest) so the changed test class is executed during module
test runs.
1913bbb to
c3fe3b0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/PathTypeAnnotationUtil.java (1)
128-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard malformed array annotations before slicing inner content.
substring(1, annotation.length() - 1)is executed when annotation onlystartsWith("["). Malformed values (e.g.,"[","[String") can throw at runtime.Proposed fix
@@ - if (annotation.startsWith("[")) { + if (annotation.startsWith("[") && annotation.endsWith("]") && annotation.length() > 1) { String inner = annotation.substring(1, annotation.length() - 1); @@ return singleList; } + if (annotation.startsWith("[")) { + // Malformed array annotation; fall back to string coercion. + return String.valueOf(value); + } @@ - boolean isArray = annotation.startsWith("["); + boolean isArray = annotation.startsWith("["); + if (isArray && (!annotation.endsWith("]") || annotation.length() <= 1)) { + return false; + } String inner = isArray ? annotation.substring(1, annotation.length() - 1) : annotation;Also applies to: 225-226
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/PathTypeAnnotationUtil.java` around lines 128 - 129, The code in PathTypeAnnotationUtil that slices array-style annotations using annotation.substring(1, annotation.length() - 1) assumes the annotation both starts with '[' and is well-formed; guard this by validating annotation.length() >= 2 and annotation.endsWith("]") (or otherwise detect the closing bracket) before calling substring, and handle malformed cases (e.g., return a safe default or throw a clear exception). Apply the same validation/fix at the other occurrence in the class (the second substring call currently around the 225-226 logic) so neither path will throw on inputs like "[" or "[String".components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.java (2)
111-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate/serialize everything before touching certificate storage.
The add/update flows perform certificate writes before all override properties are validated. If a later
validateExpose(...)or serialization step fails, the resolver aborts after mutating certificate state, which can leave orphaned or partially updated certificate records even though the action change is rejected.Also applies to: 235-239
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.java` around lines 111 - 121, The certificate is being persisted (handleCertificateAdd) before validating/serializing override properties, risking orphaned certs if validation/serialize fails; change the flow in InFlowExtensionActionDTOModelResolver to first run all validation/serialization steps (including resolveAddOverrideProperties and any validateExpose/serialization logic referenced in the add/update flows) and only after those succeed call handleCertificateAdd to persist certificates; apply the same reordering/fix for the corresponding update path (the similar block around lines 235-239) so certificate storage occurs strictly after successful validation/serialization.
385-391:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not reject slash-terminated container paths here.
This blanket trailing-slash check blocks area-style paths like
/input/and/flow/, which the request-builder path model in this PR already uses to express area exposure. That makes valid configs impossible to persist through add/update.🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.java` around lines 385 - 391, The trailing-slash validation in InFlowExtensionActionDTOModelResolver is incorrectly rejecting valid container/area paths (the path variable) — remove or relax the path.endsWith("/") check so slash-terminated paths like "/input/" or "/flow/" are accepted; specifically, delete the throw that fires when path.endsWith("/") (or change it to allow a single trailing slash) and ensure any downstream normalization/usage of path handles trailing slashes consistently after this change.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/JWEEncryptionUtil.java (1)
208-219:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
parsePEMCertificateto handle raw PEM (remove the outer Base64 decode)
parsePEMCertificate(String base64EncodedPem)Base64-decodes the entire input beforeCertificateFactory.generateCertificate(...), but the upstreamcertificate.getCertificateContent()is validated/stored as PEM (includes-----BEGIN/END CERTIFICATE-----) and forwarded unchanged intoencrypt(...). This mismatch makes valid certificates fail to parse.Proposed fix
public static X509Certificate parsePEMCertificate(String base64EncodedPem) throws ActionExecutionException { if (base64EncodedPem == null || base64EncodedPem.trim().isEmpty()) { throw new ActionExecutionException("Certificate string is null or empty."); } try { - byte[] decodedPemBytes = java.util.Base64.getDecoder().decode(base64EncodedPem.trim()); - CertificateFactory factory = CertificateFactory.getInstance("X.509"); X509Certificate certificate = - (X509Certificate) factory.generateCertificate(new ByteArrayInputStream(decodedPemBytes)); + (X509Certificate) factory.generateCertificate( + new ByteArrayInputStream(base64EncodedPem.trim().getBytes(StandardCharsets.UTF_8))); certificate.checkValidity(); return certificate; ...🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/JWEEncryptionUtil.java` around lines 208 - 219, The parsePEMCertificate method is incorrectly Base64-decoding an already-PEM-formatted string; change parsePEMCertificate(String base64EncodedPem) to detect raw PEM vs base64 DER: if the trimmed input starts with "-----BEGIN" (or contains PEM headers) convert the string to bytes using UTF-8 and pass that stream to CertificateFactory.generateCertificate, otherwise Base64-decode the input and pass the decoded bytes; keep the existing null/empty check and exception behavior and update references to parsePEMCertificate and any callers like certificate.getCertificateContent() / encrypt(...) to continue passing the PEM unchanged when appropriate.
♻️ Duplicate comments (4)
components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeNode.java (1)
50-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCopy lists before wrapping to prevent external mutation.
Wrapping
b.allowedOperationsandb.childrendirectly without copying leaves the node vulnerable to mutation if the caller retains a reference to the builder's lists.🛡️ Suggested fix
- this.allowedOperations = b.allowedOperations != null - ? Collections.unmodifiableList(b.allowedOperations) : Collections.emptyList(); + this.allowedOperations = b.allowedOperations != null + ? Collections.unmodifiableList(new java.util.ArrayList<>(b.allowedOperations)) + : Collections.emptyList(); this.readOnly = b.readOnly; this.replaceable = b.replaceable; this.dynamicEntryAllowed = b.dynamicEntryAllowed; this.dynamicEntryType = b.dynamicEntryType; - this.children = b.children != null - ? Collections.unmodifiableList(b.children) : Collections.emptyList(); + this.children = b.children != null + ? Collections.unmodifiableList(new java.util.ArrayList<>(b.children)) + : Collections.emptyList();Also applies to: 56-57
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeNode.java` around lines 50 - 51, The constructor of InFlowExtensionContextTreeNode wraps builder lists directly, which allows external mutation; for both allowedOperations and children (from builder variable b) create defensive copies first (e.g., new ArrayList<>(b.allowedOperations) and new ArrayList<>(b.children)) and then wrap those copies with Collections.unmodifiableList (or Collections.emptyList() when null/empty) so the node cannot be mutated via retained builder references.components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/impl/DefaultActionValidator.java (2)
285-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle nulls defensively in uniqueness validation.
Line 285 assumes
existingis non-null and Line 287 assumesdto.getName()is non-null. If either is null, this throws NPE and returns a server error instead of a controlled validation error.🛡️ Suggested fix
public void validateActionNameUniqueness(String name, String excludeId, List<ActionDTO> existing) throws ActionMgtClientException { + if (existing == null || existing.isEmpty()) { + return; + } + boolean duplicateExists = existing.stream() + .filter(dto -> dto != null && dto.getName() != null) .filter(dto -> excludeId == null || !excludeId.equals(dto.getId())) .anyMatch(dto -> name.equalsIgnoreCase(dto.getName())); if (duplicateExists) { throw ActionManagementExceptionHandler.handleClientException( ErrorMessage.ERROR_ACTION_NAME_ALREADY_EXISTS, name); } }🤖 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 285 - 287, The uniqueness check in DefaultActionValidator assumes existing and dto.getName() (and name) are non-null which can cause NPEs; fix by guarding against nulls: if existing is null treat it as empty (no duplicate), and in the stream/filter ensure dto != null and compare ids using Objects.equals(dto.getId(), excludeId) (or the inverse) to handle null excludeId safely, and compare names with a null-safe equality (e.g., if (name == null) check dto.getName() == null else name.equalsIgnoreCase(dto.getName())). Update the boolean duplicateExists computation to use these null checks around existing, dto, dto.getId(), dto.getName(), and name.
97-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd DEBUG log at validation entry point.
This overload validates uniqueness for FLOW_EXTENSIONS actions before DB insert. Per coding guidelines, major method executions should log their invocation.
📝 Suggested log addition
`@Override` public void doPreAddActionValidations(Action.ActionTypes actionType, String actionVersion, Action action, List<ActionDTO> existingActionsOfType) throws ActionMgtException { + if (LOG.isDebugEnabled()) { + LOG.debug("Performing pre-add validations for action type: " + actionType + " with name: " + + action.getName()); + } doPreAddActionValidations(actionType, actionVersion, action);As per coding guidelines: "Add logs only around major method executions/decision points/handled exceptional scenarios."
🤖 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 97 - 105, Add a DEBUG log at the start of the overloaded doPreAddActionValidations(Action.ActionTypes, String, Action, List<ActionDTO>) to record invocation and key params; specifically log actionType, actionVersion and action.getName() before calling the existing doPreAddActionValidations(...) and before the FLOW_EXTENSIONS branch so that when ActionTypes.FLOW_EXTENSIONS.equals(actionType) and validateActionNameUniqueness(...) runs you have an entry point trace; use the existing class logger and follow project logging style and level.components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/util/ActionManagementConfig.java (1)
99-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd DEBUG log for FLOW_EXTENSIONS version resolution.
Per coding guidelines, major method executions (configuration resolution) should include diagnostics. For consistency with other complex operations, add a DEBUG log.
📝 Suggested log addition
case FLOW_EXTENSIONS: + if (LOG.isDebugEnabled()) { + LOG.debug("Resolving latest version for FLOW_EXTENSIONS action type"); + } return getVersion( ActionTypeConfig.FLOW_EXTENSIONS.getLatestVersionProperty(), actionType);As per coding guidelines: "Add logs only around major method executions/decision points/handled exceptional scenarios."
🤖 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/internal/util/ActionManagementConfig.java` around lines 99 - 101, In the switch branch handling ActionTypeConfig.FLOW_EXTENSIONS inside ActionManagementConfig (where it currently returns getVersion(ActionTypeConfig.FLOW_EXTENSIONS.getLatestVersionProperty(), actionType)), add a DEBUG log statement before calling getVersion that logs resolution start and the actionType and property being used (e.g., the value from ActionTypeConfig.FLOW_EXTENSIONS.getLatestVersionProperty()); ensure the logger used by ActionManagementConfig (existing class logger) is used and that the log is guarded by isDebugEnabled() if applicable.
🧹 Nitpick comments (4)
components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/internal/InFlowExtensionServiceComponent.java (1)
75-85: ⚡ Quick winUse INFO for service lifecycle state transitions.
Activation/deactivation success logs are major component state transitions and should be INFO-level.
As per coding guidelines: "Use INFO for major actions/state transitions; avoid repetitive/trivial logs."Suggested logging-level update
- LOG.debug("In-Flow Extension service successfully activated."); + LOG.info("In-Flow Extension service successfully activated."); @@ - LOG.debug("In-Flow Extension service successfully deactivated."); + LOG.info("In-Flow Extension service successfully deactivated.");🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/internal/InFlowExtensionServiceComponent.java` around lines 75 - 85, The activation/deactivation messages in InFlowExtensionServiceComponent use LOG.debug but should be INFO-level for major lifecycle transitions; change the LOG.debug calls in the activate(ComponentContext) method ("In-Flow Extension service successfully activated.") and in the deactivate(ComponentContext) method ("In-Flow Extension service successfully deactivated.") to LOG.info while leaving the LOG.error in the catch block unchanged.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.java (1)
406-522: ⚡ Quick winAdd logs around handled certificate lifecycle failures.
These methods perform significant certificate-management operations and convert handled exceptions into resolver exceptions without any local log context. A concise WARN/ERROR log with safe identifiers such as the action ID would make storage/update/delete failures much easier to diagnose.
As per coding guidelines, "Flag functions or methods that perform significant operations (DB calls, API calls, auth, payments) but lack log statements, and suggest appropriate logs if possible."
🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.java` around lines 406 - 522, These certificate lifecycle handlers (handleCertificateAdd, handleCertificateGet, handleCertificateUpdate, handleCertificateDelete) swallow service errors into ActionDTOModelResolverException without any local log context; add concise logs at each catch (and for the explicit removal branch failure) that include a safe identifier like the action ID and the exception message/stacktrace before rethrowing. Specifically, in each catch block use the class logger (e.g., log or LOGGER consistent with the class) to emit an ERROR (or WARN where appropriate) with a message such as "Failed to <add|get|update|delete> certificate for action: {actionId}" and include the caught exception, then rethrow the existing ActionDTOModelResolverException as currently done. Ensure you update handleCertificateUpdate's delete/update/add catch blocks and handleCertificateGet/handleCertificateAdd/handleCertificateDelete catch blocks accordingly.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionResponseProcessor.java (2)
370-376: 💤 Low valueConsider enforcing flat credential paths.
Same pattern as property handling:
extractNameFromPathaccepts nested paths like/user/credentials/a/b, storing keya/b. If only flat credential keys are valid, add a slash check after extraction for consistency with the property path contract.Suggested fix
if (credentialKey == null || credentialKey.isEmpty()) { return new OperationExecutionResult(operation, OperationExecutionResult.Status.FAILURE, "Invalid credential path. Credential key is required."); } + if (credentialKey.contains("/")) { + return new OperationExecutionResult(operation, OperationExecutionResult.Status.FAILURE, + "Invalid credential path. Only flat credential paths are supported."); + }🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionResponseProcessor.java` around lines 370 - 376, The code in InFlowExtensionResponseProcessor uses extractNameFromPath with USER_CREDENTIALS_PATH_PREFIX but does not reject nested paths; after extracting credentialKey, add a validation that credentialKey does not contain '/' (same approach used for property paths) and return an OperationExecutionResult with Status.FAILURE and a clear message (e.g., "Invalid credential path. Credential key must be flat.") if it does; update the block handling credentialKey (around extractNameFromPath and the subsequent null/empty check) to include this slash check to enforce flat credential keys.
425-441: 💤 Low valueConsider reusing
extractClaimUriFromPathto reduce duplication.The claim URI extraction logic is duplicated between
normalizeToInternalPathandextractClaimUriFromPath. A minor consolidation would improve maintainability.Suggested refactor
private static String normalizeToInternalPath(String externalPath) { - if (externalPath != null - && externalPath.startsWith(InFlowExtensionConstants.USER_CLAIMS_SELECTOR_PREFIX) - && externalPath.endsWith(InFlowExtensionConstants.USER_CLAIMS_SELECTOR_SUFFIX)) { - String claimUri = externalPath.substring( - InFlowExtensionConstants.USER_CLAIMS_SELECTOR_PREFIX.length(), - externalPath.length() - InFlowExtensionConstants.USER_CLAIMS_SELECTOR_SUFFIX.length()); + String claimUri = extractClaimUriFromSelectorPath(externalPath); + if (claimUri != null) { return InFlowExtensionConstants.USER_CLAIMS_PATH_PREFIX + claimUri; } return externalPath; } + + // Rename extractClaimUriFromPath to extractClaimUriFromSelectorPath and make it static + // to be usable from both instance and static contexts.🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionResponseProcessor.java` around lines 425 - 441, normalizeToInternalPath duplicates claim-uri extraction logic found in extractClaimUriFromPath; change normalizeToInternalPath to call extractClaimUriFromPath to get the claimUri and then prepend InFlowExtensionConstants.USER_CLAIMS_PATH_PREFIX, while preserving the existing null/startsWith/endsWith checks and returning the original externalPath when not applicable. Specifically, inside normalizeToInternalPath use extractClaimUriFromPath(externalPath) to obtain the URI, if non-null return InFlowExtensionConstants.USER_CLAIMS_PATH_PREFIX + claimUri, otherwise return externalPath; keep references to InFlowExtensionConstants.USER_CLAIMS_SELECTOR_PREFIX, USER_CLAIMS_SELECTOR_SUFFIX and USER_CLAIMS_PATH_PREFIX to locate the code.
🤖 Prompt for all review comments with 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.
Inline comments:
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 line 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.
- Around line 136-145: Add a DEBUG log at the start of the overloaded
doPreUpdateActionValidations(Action.ActionTypes actionType, String
actionVersion, Action action, String excludeId, List<ActionDTO>
existingActionsOfType) method to record invocation and key parameters
(actionType, actionVersion, excludeId and action.getName()) before any
validation logic runs; place the log as the first statement in that method
(before the call to doPreUpdateActionValidations(actionType, actionVersion,
action)) using the class logger (same logger used elsewhere in
DefaultActionValidator) and include concise context text like "Entering
pre-update validation".
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionRequestBuilder.java`:
- Around line 627-631: The code in InFlowExtensionRequestBuilder currently zeros
the original credValue (Arrays.fill(credValue, '\0')), mutating the
FlowExecutionContext; instead make and use a temporary copy: create a temp
char[] via Arrays.copyOf(credValue, credValue.length), build plaintext from the
temp copy (new String(temp)), zero only that temp copy (Arrays.fill(temp,'\0')),
then call toEncryptedOrPlainCredentialChars(plaintext, exposePath, accessConfig,
certificatePEM) and put the result into filteredCredentials; ensure credValue in
the original context is never modified.
---
Outside diff comments:
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/JWEEncryptionUtil.java`:
- Around line 208-219: The parsePEMCertificate method is incorrectly
Base64-decoding an already-PEM-formatted string; change
parsePEMCertificate(String base64EncodedPem) to detect raw PEM vs base64 DER: if
the trimmed input starts with "-----BEGIN" (or contains PEM headers) convert the
string to bytes using UTF-8 and pass that stream to
CertificateFactory.generateCertificate, otherwise Base64-decode the input and
pass the decoded bytes; keep the existing null/empty check and exception
behavior and update references to parsePEMCertificate and any callers like
certificate.getCertificateContent() / encrypt(...) to continue passing the PEM
unchanged when appropriate.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/PathTypeAnnotationUtil.java`:
- Around line 128-129: The code in PathTypeAnnotationUtil that slices
array-style annotations using annotation.substring(1, annotation.length() - 1)
assumes the annotation both starts with '[' and is well-formed; guard this by
validating annotation.length() >= 2 and annotation.endsWith("]") (or otherwise
detect the closing bracket) before calling substring, and handle malformed cases
(e.g., return a safe default or throw a clear exception). Apply the same
validation/fix at the other occurrence in the class (the second substring call
currently around the 225-226 logic) so neither path will throw on inputs like
"[" or "[String".
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.java`:
- Around line 111-121: The certificate is being persisted (handleCertificateAdd)
before validating/serializing override properties, risking orphaned certs if
validation/serialize fails; change the flow in
InFlowExtensionActionDTOModelResolver to first run all validation/serialization
steps (including resolveAddOverrideProperties and any
validateExpose/serialization logic referenced in the add/update flows) and only
after those succeed call handleCertificateAdd to persist certificates; apply the
same reordering/fix for the corresponding update path (the similar block around
lines 235-239) so certificate storage occurs strictly after successful
validation/serialization.
- Around line 385-391: The trailing-slash validation in
InFlowExtensionActionDTOModelResolver is incorrectly rejecting valid
container/area paths (the path variable) — remove or relax the
path.endsWith("/") check so slash-terminated paths like "/input/" or "/flow/"
are accepted; specifically, delete the throw that fires when path.endsWith("/")
(or change it to allow a single trailing slash) and ensure any downstream
normalization/usage of path handles trailing slashes consistently after this
change.
---
Duplicate comments:
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 line 285-287: The uniqueness check in DefaultActionValidator assumes
existing and dto.getName() (and name) are non-null which can cause NPEs; fix by
guarding against nulls: if existing is null treat it as empty (no duplicate),
and in the stream/filter ensure dto != null and compare ids using
Objects.equals(dto.getId(), excludeId) (or the inverse) to handle null excludeId
safely, and compare names with a null-safe equality (e.g., if (name == null)
check dto.getName() == null else name.equalsIgnoreCase(dto.getName())). Update
the boolean duplicateExists computation to use these null checks around
existing, dto, dto.getId(), dto.getName(), and name.
- Around line 97-105: Add a DEBUG log at the start of the overloaded
doPreAddActionValidations(Action.ActionTypes, String, Action, List<ActionDTO>)
to record invocation and key params; specifically log actionType, actionVersion
and action.getName() before calling the existing doPreAddActionValidations(...)
and before the FLOW_EXTENSIONS branch so that when
ActionTypes.FLOW_EXTENSIONS.equals(actionType) and
validateActionNameUniqueness(...) runs you have an entry point trace; use the
existing class logger and follow project logging style and level.
In
`@components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/util/ActionManagementConfig.java`:
- Around line 99-101: In the switch branch handling
ActionTypeConfig.FLOW_EXTENSIONS inside ActionManagementConfig (where it
currently returns
getVersion(ActionTypeConfig.FLOW_EXTENSIONS.getLatestVersionProperty(),
actionType)), add a DEBUG log statement before calling getVersion that logs
resolution start and the actionType and property being used (e.g., the value
from ActionTypeConfig.FLOW_EXTENSIONS.getLatestVersionProperty()); ensure the
logger used by ActionManagementConfig (existing class logger) is used and that
the log is guarded by isDebugEnabled() if applicable.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeNode.java`:
- Around line 50-51: The constructor of InFlowExtensionContextTreeNode wraps
builder lists directly, which allows external mutation; for both
allowedOperations and children (from builder variable b) create defensive copies
first (e.g., new ArrayList<>(b.allowedOperations) and new
ArrayList<>(b.children)) and then wrap those copies with
Collections.unmodifiableList (or Collections.emptyList() when null/empty) so the
node cannot be mutated via retained builder references.
---
Nitpick comments:
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionResponseProcessor.java`:
- Around line 370-376: The code in InFlowExtensionResponseProcessor uses
extractNameFromPath with USER_CREDENTIALS_PATH_PREFIX but does not reject nested
paths; after extracting credentialKey, add a validation that credentialKey does
not contain '/' (same approach used for property paths) and return an
OperationExecutionResult with Status.FAILURE and a clear message (e.g., "Invalid
credential path. Credential key must be flat.") if it does; update the block
handling credentialKey (around extractNameFromPath and the subsequent null/empty
check) to include this slash check to enforce flat credential keys.
- Around line 425-441: normalizeToInternalPath duplicates claim-uri extraction
logic found in extractClaimUriFromPath; change normalizeToInternalPath to call
extractClaimUriFromPath to get the claimUri and then prepend
InFlowExtensionConstants.USER_CLAIMS_PATH_PREFIX, while preserving the existing
null/startsWith/endsWith checks and returning the original externalPath when not
applicable. Specifically, inside normalizeToInternalPath use
extractClaimUriFromPath(externalPath) to obtain the URI, if non-null return
InFlowExtensionConstants.USER_CLAIMS_PATH_PREFIX + claimUri, otherwise return
externalPath; keep references to
InFlowExtensionConstants.USER_CLAIMS_SELECTOR_PREFIX,
USER_CLAIMS_SELECTOR_SUFFIX and USER_CLAIMS_PATH_PREFIX to locate the code.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/internal/InFlowExtensionServiceComponent.java`:
- Around line 75-85: The activation/deactivation messages in
InFlowExtensionServiceComponent use LOG.debug but should be INFO-level for major
lifecycle transitions; change the LOG.debug calls in the
activate(ComponentContext) method ("In-Flow Extension service successfully
activated.") and in the deactivate(ComponentContext) method ("In-Flow Extension
service successfully deactivated.") to LOG.info while leaving the LOG.error in
the catch block unchanged.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.java`:
- Around line 406-522: These certificate lifecycle handlers
(handleCertificateAdd, handleCertificateGet, handleCertificateUpdate,
handleCertificateDelete) swallow service errors into
ActionDTOModelResolverException without any local log context; add concise logs
at each catch (and for the explicit removal branch failure) that include a safe
identifier like the action ID and the exception message/stacktrace before
rethrowing. Specifically, in each catch block use the class logger (e.g., log or
LOGGER consistent with the class) to emit an ERROR (or WARN where appropriate)
with a message such as "Failed to <add|get|update|delete> certificate for
action: {actionId}" and include the caught exception, then rethrow the existing
ActionDTOModelResolverException as currently done. Ensure you update
handleCertificateUpdate's delete/update/add catch blocks and
handleCertificateGet/handleCertificateAdd/handleCertificateDelete catch blocks
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f748978d-c491-4359-83ab-70dba53a4334
📒 Files selected for processing (64)
.gitignorecomponents/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/api/model/ActionType.javacomponents/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/api/model/User.javacomponents/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/internal/service/impl/ActionExecutorServiceImpl.javacomponents/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/internal/util/ActionExecutorConfig.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/model/Action.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/ActionManagementService.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/ActionValidator.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/impl/DefaultActionValidator.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/dao/impl/ActionDTOModelResolverFactory.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/service/impl/ActionManagementServiceImpl.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/util/ActionManagementConfig.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/model/ActionTypesTest.javacomponents/entitlement/pom.xmlcomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/pom.xmlcomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/Constants.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/graph/TaskExecutionNode.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/model/ExecutorResponse.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/model/FlowUser.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/pom.xmlcomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/InFlowExtensionConstants.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionExecutor.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionRequestBuilder.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionResponseProcessor.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/JWEEncryptionUtil.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/PathTypeAnnotationUtil.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/internal/InFlowExtensionDataHolder.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/internal/InFlowExtensionServiceComponent.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionConverter.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeBuilder.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeMetadata.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeNode.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeService.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/AccessConfig.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/ContextPath.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/Encryption.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/FlowContextHandoverConfig.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/InFlowExtensionAction.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/InFlowExtensionEvent.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/InFlowExtensionFlow.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/InFlowExtensionRequest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/InFlowUser.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/OperationExecutionResult.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/util/InFlowExtensionContextFilterUtil.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/util/InFlowExtensionPathUtil.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/InFlowExtensionTestUtils.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionExecutorTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionRequestBuilderTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionResponseProcessorTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/executor/PathTypeAnnotationUtilTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/metadata/FlowContextHandoverConfigTestHelper.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeBuilderTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/model/AccessConfigTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/model/InFlowExtensionEventTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/model/OperationExecutionResultTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/util/InFlowExtensionPathUtilTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/resources/repository/conf/carbon.xmlcomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/resources/testng.xmlcomponents/flow-orchestration-framework/pom.xmlfeatures/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions.server.feature/pom.xmlfeatures/flow-orchestration-framework/org.wso2.carbon.identity.flow.orchestration.framework.feature/pom.xmlfeatures/flow-orchestration-framework/pom.xmlpom.xml
💤 Files with no reviewable changes (2)
- components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/ActionManagementService.java
- components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/resources/repository/conf/carbon.xml
✅ Files skipped from review due to trivial changes (5)
- features/flow-orchestration-framework/pom.xml
- components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/internal/service/impl/ActionExecutorServiceImpl.java
- components/entitlement/pom.xml
- components/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/model/ActionTypesTest.java
- components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/resources/testng.xml
Comment on lines
+136
to
+145
| @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_EXTENSIONS.equals(actionType)) { | ||
| validateActionNameUniqueness(action.getName(), excludeId, existingActionsOfType); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add DEBUG log at validation entry point.
This overload validates uniqueness for FLOW_EXTENSIONS actions before DB update. Per coding guidelines, major method executions should log their invocation.
📝 Suggested log addition
`@Override`
public void doPreUpdateActionValidations(Action.ActionTypes actionType, String actionVersion, Action action,
String excludeId, List<ActionDTO> existingActionsOfType)
throws ActionMgtException {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Performing pre-update validations for action type: " + actionType + " with name: " +
+ action.getName() + ", excludeId: " + excludeId);
+ }
doPreUpdateActionValidations(actionType, actionVersion, action);As per coding guidelines: "Add logs only around major method executions/decision points/handled exceptional scenarios."
🤖 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 136 - 145, Add a DEBUG log at the start of the overloaded
doPreUpdateActionValidations(Action.ActionTypes actionType, String
actionVersion, Action action, String excludeId, List<ActionDTO>
existingActionsOfType) method to record invocation and key parameters
(actionType, actionVersion, excludeId and action.getName()) before any
validation logic runs; place the log as the first statement in that method
(before the call to doPreUpdateActionValidations(actionType, actionVersion,
action)) using the class logger (same logger used elsewhere in
DefaultActionValidator) and include concise context text like "Entering
pre-update validation".
Comment on lines
+289
to
+292
| if (duplicateExists) { | ||
| throw ActionManagementExceptionHandler.handleClientException( | ||
| ErrorMessage.ERROR_ACTION_NAME_ALREADY_EXISTS, name); | ||
| } |
Contributor
There was a problem hiding this comment.
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.
Comment on lines
+627
to
+631
| if (credValue != null) { | ||
| String plaintext = new String(credValue); | ||
| java.util.Arrays.fill(credValue, '\0'); | ||
| filteredCredentials.put(credKey, | ||
| toEncryptedOrPlainCredentialChars(plaintext, exposePath, accessConfig, certificatePEM)); |
Contributor
There was a problem hiding this comment.
Do not zero the credential array from the source context.
Arrays.fill(credValue, '\0') mutates the FlowExecutionContext while merely building the outbound request. If that char[] is shared with the live flow state, the credential is erased before the rest of the flow can use it. Clear only a temporary copy.
Proposed fix
char[] credValue = credentials != null ? credentials.get(credKey) : null;
if (credValue != null) {
- String plaintext = new String(credValue);
- java.util.Arrays.fill(credValue, '\0');
- filteredCredentials.put(credKey,
- toEncryptedOrPlainCredentialChars(plaintext, exposePath, accessConfig, certificatePEM));
+ char[] credentialCopy = java.util.Arrays.copyOf(credValue, credValue.length);
+ try {
+ String plaintext = new String(credentialCopy);
+ filteredCredentials.put(credKey,
+ toEncryptedOrPlainCredentialChars(
+ plaintext, exposePath, accessConfig, certificatePEM));
+ } finally {
+ java.util.Arrays.fill(credentialCopy, '\0');
+ }
} else {
filteredCredentials.put(credKey, new char[0]);
}🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/InFlowExtensionRequestBuilder.java`
around lines 627 - 631, The code in InFlowExtensionRequestBuilder currently
zeros the original credValue (Arrays.fill(credValue, '\0')), mutating the
FlowExecutionContext; instead make and use a temporary copy: create a temp
char[] via Arrays.copyOf(credValue, credValue.length), build plaintext from the
temp copy (new String(temp)), zero only that temp copy (Arrays.fill(temp,'\0')),
then call toEncryptedOrPlainCredentialChars(plaintext, exposePath, accessConfig,
certificatePEM) and put the result into filteredCredentials; ensure credValue in
the original context is never modified.
Comment on lines
120
to
140
|
|
||
| /** | ||
| * 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; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Why do we need this ?
ThaminduR
reviewed
May 24, 2026
|
|
||
| } | ||
|
|
||
| // ---- FlowContext pipeline keys ---- |
Contributor
There was a problem hiding this comment.
fix the comments with the proper format
ThaminduR
reviewed
May 24, 2026
Comment on lines
+54
to
+73
| // ---- Context path prefixes ---- | ||
| public static final String USER_PREFIX = "/user/"; | ||
| public static final String USER_ID_PATH = "/user/id"; | ||
| public static final String USER_STORE_DOMAIN_PATH = "/user/userStoreDomain"; | ||
| public static final String USER_CLAIMS_PATH_PREFIX = "/user/claims/"; | ||
| public static final String USER_CLAIMS_SELECTOR_PREFIX = "/user/claims[uri="; | ||
| public static final String USER_CLAIMS_SELECTOR_SUFFIX = "]"; | ||
| public static final String USER_CREDENTIALS_PATH_PREFIX = "/user/credentials/"; | ||
| public static final String PROPERTIES_PATH_PREFIX = "/properties/"; | ||
| public static final String FLOW_PREFIX = "/flow/"; | ||
| public static final String FLOW_TENANT_PATH = "/flow/tenantDomain"; | ||
| public static final String FLOW_APP_ID_PATH = "/flow/applicationId"; | ||
| public static final String FLOW_TYPE_PATH = "/flow/flowType"; | ||
| public static final String FLOW_CALLBACK_URL_PATH = "/flow/callbackUrl"; | ||
| public static final String FLOW_PORTAL_URL_PATH = "/flow/portalUrl"; | ||
| public static final String ORGANIZATION_PREFIX = "/organization/"; | ||
| public static final String ORGANIZATION_ID_PATH = "/organization/id"; | ||
| public static final String ORGANIZATION_NAME_PATH = "/organization/name"; | ||
| public static final String ORGANIZATION_HANDLE_PATH = "/organization/orgHandle"; | ||
| public static final String ORGANIZATION_DEPTH_PATH = "/organization/depth"; |
ThaminduR
reviewed
May 24, 2026
Comment on lines
+122
to
+133
| /** | ||
| * Compile-time default handover policy constants. | ||
| * | ||
| * <p>These constants define which {@code FlowExecutionContext} and {@code FlowUser} | ||
| * fields are handed to the action framework during in-flow extension execution. | ||
| * {@code "properties"} is intentionally excluded from {@link #INCLUDED_ATTRIBUTES}: | ||
| * it is always modifiable via the executor response path (context tree always exposes | ||
| * it with MODIFY ops), but must not be forwarded to external services by default.</p> | ||
| * | ||
| * <p>When the toml-based dynamic config PR is merged, these constants serve as the | ||
| * documented defaults for {@code identity.xml.j2}.</p> | ||
| */ |
Contributor
There was a problem hiding this comment.
Reduce long comments. Only add comments if code itself isn't self explanatory.
ThaminduR
reviewed
May 24, 2026
| target | ||
|
|
||
| # Maven versions plugin backup files | ||
| *.versionsBackup No newline at end of file |
ThaminduR
reviewed
May 24, 2026
| /** | ||
| * Path-matching utilities for In-Flow Extension access control. | ||
| */ | ||
| public final class InFlowExtensionPathUtil { |
Contributor
There was a problem hiding this comment.
Is there a requirement for a separate utils class for these method?
IMO one generic util class should suffice for flow extensions.
Contributor
Author
There was a problem hiding this comment.
Will move this to the AccessConfig and keep the other one generic
ThaminduR
reviewed
May 24, 2026
| * {@code userCredentials} field receives a per-entry {@code char[]} clone so that the | ||
| * request builder's post-extraction wipe zeroes the copy, not the source.</p> | ||
| */ | ||
| public final class InFlowExtensionContextFilterUtil { |
Contributor
There was a problem hiding this comment.
Avoid multiple different utils classes, if possible
ThaminduR
reviewed
May 24, 2026
Contributor
There was a problem hiding this comment.
What's the purpose of this class
ThaminduR
reviewed
May 24, 2026
Contributor
There was a problem hiding this comment.
Do we have a usage for this?
isn't this similar to FlowExecutionContext?
ThaminduR
reviewed
May 24, 2026
Contributor
There was a problem hiding this comment.
Let's simplify the current implementation by having the current default policy in FlowExtensionContextFilterUtil itself.
Contributor
There was a problem hiding this comment.
IMO since this is governed at code level right now, we do not need to use this class
ThaminduR
reviewed
May 24, 2026
Contributor
There was a problem hiding this comment.
This seems to be just a wrapper for Certificate. We should be able to directly use the certificate instead
ThaminduR
reviewed
May 24, 2026
Comment on lines
+39
to
+51
| private static final String FLOW_REGISTRATION = "REGISTRATION"; | ||
| private static final String FLOW_INVITED_USER_REGISTRATION = "INVITED_USER_REGISTRATION"; | ||
| private static final String FLOW_PASSWORD_RECOVERY = "PASSWORD_RECOVERY"; | ||
|
|
||
| // Node-type sentinels matching the tree component's NodeType enum on the Console side. | ||
| private static final String NODE_OBJECT = "OBJECT"; | ||
| private static final String NODE_LEAF = "LEAF"; | ||
| private static final String NODE_MAP = "MAP"; | ||
| private static final String NODE_COMPLEX_MAP = "COMPLEX_MAP"; | ||
|
|
||
| private static final String OP_EXPOSE = "EXPOSE"; | ||
| private static final String OP_MODIFY = "MODIFY"; | ||
| private static final String DATA_TYPE_STRING = "String"; |
Contributor
There was a problem hiding this comment.
Move this to an inner class in the component's constant class
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/executor/InFlowExtensionExecutorTest.java (1)
219-220: ⚡ Quick winUse shared constants for failureType assertion to avoid brittle string coupling.
Prefer asserting with
InFlowExtensionConstants.FAILURE_TYPE_KEYandInFlowExtensionConstants.FLOW_EXTENSION_FAILURE_TYPEinstead of hardcoded literals.Proposed test hardening
- assertEquals(response.getAdditionalInfo().get("failureType"), "FLOW_EXTENSION_FAILURE"); + assertEquals(response.getAdditionalInfo().get(InFlowExtensionConstants.FAILURE_TYPE_KEY), + InFlowExtensionConstants.FLOW_EXTENSION_FAILURE_TYPE);🤖 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/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/executor/InFlowExtensionExecutorTest.java` around lines 219 - 220, The test is asserting failureType using hardcoded strings; replace those literals with the shared constants InFlowExtensionConstants.FAILURE_TYPE_KEY and InFlowExtensionConstants.FLOW_EXTENSION_FAILURE_TYPE in InFlowExtensionExecutorTest (where response.getAdditionalInfo() is asserted) so the assertions use the constants instead of "failureType" and "FLOW_EXTENSION_FAILURE".components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/service/impl/ActionManagementServiceImpl.java (1)
490-498: ⚡ Quick winAdd a warning log before rejecting duplicate FLOW_EXTENSION names.
The duplicate-name rejection branch throws a client exception without any local log, even though this is a significant DB-backed decision point. Add a concise
WARNwith safe identifiers (e.g., action type and tenant id, without sensitive payloads).As per coding guidelines, "Add logs primarily around significant control-flow branches, decision points, and handled exceptional scenarios."Suggested patch
boolean duplicateExists = existingActions.stream() .filter(dto -> excludeId == null || !excludeId.equals(dto.getId())) .anyMatch(dto -> name.equalsIgnoreCase(dto.getName())); if (duplicateExists) { + LOG.warn("Duplicate FLOW_EXTENSION action name detected for tenantId: " + tenantId + + ". Rejecting action create/update request."); throw ActionManagementExceptionHandler.handleClientException( ErrorMessage.ERROR_ACTION_NAME_ALREADY_EXISTS, name); }🤖 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/internal/service/impl/ActionManagementServiceImpl.java` around lines 490 - 498, Before throwing the client exception in ActionManagementServiceImpl where duplicateExists is detected, add a concise WARN log that records the safe identifiers (action type and tenant id) to indicate a duplicate FLOW_EXTENSION name was rejected; locate the duplicate-check block that uses existingActions, duplicateExists, DAO_FACADE and ErrorMessage.ERROR_ACTION_NAME_ALREADY_EXISTS and call the class logger (e.g., log.warn) with a short message including actionType.getActionType() and tenantId prior to invoking ActionManagementExceptionHandler.handleClientException.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In
`@components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/service/impl/ActionManagementServiceImpl.java`:
- Around line 490-498: Before throwing the client exception in
ActionManagementServiceImpl where duplicateExists is detected, add a concise
WARN log that records the safe identifiers (action type and tenant id) to
indicate a duplicate FLOW_EXTENSION name was rejected; locate the
duplicate-check block that uses existingActions, duplicateExists, DAO_FACADE and
ErrorMessage.ERROR_ACTION_NAME_ALREADY_EXISTS and call the class logger (e.g.,
log.warn) with a short message including actionType.getActionType() and tenantId
prior to invoking ActionManagementExceptionHandler.handleClientException.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/executor/InFlowExtensionExecutorTest.java`:
- Around line 219-220: The test is asserting failureType using hardcoded
strings; replace those literals with the shared constants
InFlowExtensionConstants.FAILURE_TYPE_KEY and
InFlowExtensionConstants.FLOW_EXTENSION_FAILURE_TYPE in
InFlowExtensionExecutorTest (where response.getAdditionalInfo() is asserted) so
the assertions use the constants instead of "failureType" and
"FLOW_EXTENSION_FAILURE".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 66410271-b513-417f-be45-591834884d89
📒 Files selected for processing (12)
components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/internal/service/impl/ActionExecutorServiceImpl.javacomponents/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/internal/util/ActionExecutorConfig.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/model/Action.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/impl/DefaultActionValidator.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/service/impl/ActionManagementServiceImpl.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/util/ActionManagementConfig.javacomponents/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/model/ActionTypesTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/InFlowExtensionConstants.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/executor/InFlowExtensionExecutor.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/executor/InFlowExtensionExecutorTest.javafeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json
💤 Files with no reviewable changes (1)
- components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/impl/DefaultActionValidator.java
✅ Files skipped from review due to trivial changes (1)
- components/action-mgt/org.wso2.carbon.identity.action.management/src/test/java/org/wso2/carbon/identity/action/management/model/ActionTypesTest.java
|
ThaminduR
reviewed
May 25, 2026
| char[] credValue = credentials != null ? credentials.get(credKey) : null; | ||
|
|
||
| if (credValue != null) { | ||
| String plaintext = new String(credValue); |
Contributor
There was a problem hiding this comment.
Wiping the cloned array is fine, but copying the secret into an immutable String defeats the purpose since it cannot be cleared. Keep credentials as char[] end to end. The same applies to the String.valueOf(...).toCharArray() path in the response processor.
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Resolves wso2/product-is#27771
Introduce In-Flow Extension as a new action type that enables invoking external services during authentication flow execution. Sensitive flow context data is JWE-encrypted before being sent to the external endpoint.
Goals
IN_FLOW_EXTENSIONaction type under theEXTENSIONcategoryAccessConfigto control what context data is exposed to and modifiable by the external service, with per-path encryptionActionValidatorinterface, introduceInFlowExtensionPathUtil, addInFlowUserandInFlowExtensionFlowmodelsApproach
InFlowExtensionActionextendsActionwith anAccessConfig(expose/modify context paths with encryption flags) and anEncryptionconfiguration (certificate for JWE)InFlowExtensionPathUtil) ensures context path configurations don't conflict and supports bidirectional parent-child matchingActionValidatorinterface introduced to decouple validation logic from the service implementationUser stories
N/A
Release note
N/A
Documentation
N/A
Training
N/A
Certification
N/A
Marketing
N/A
Automation tests
Security checks
Samples
N/A
Related PRs
Migrations (if applicable)
N/A
Test environment
JDK 21, macOS
Learning
N/A