Feature - In-Flow Extension#7946
Open
ThejithaR wants to merge 41 commits into
Open
Conversation
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds FLOW_EXTENSIONS action type and User fields; integrates flow-extension configs and validation; introduces a new flow-extensions module (executor, request/response, JWE/path utilities, DTO resolver, action converter, metadata service, OSGi DS component); updates flow engine models/handling; and adds comprehensive tests and build/module changes. ChangesIn-Flow Extensions Feature
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+314
to
+316
| */ | ||
| private Set<String> getValidClaimUris(String tenantDomain) { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 18
Suggested change
| */ | |
| private Set<String> getValidClaimUris(String tenantDomain) { | |
| try { | |
| ClaimMetadataManagementService claimService = getClaimMetadataManagementService(); | |
| if (claimService != null) { | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("Loading local claims for tenant: " + cacheKey); | |
| } |
Comment on lines
+79
to
+80
| public static String encrypt(String plaintext, String certificatePEM) throws ActionExecutionException { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 19
Suggested change
| public static String encrypt(String plaintext, String certificatePEM) throws ActionExecutionException { | |
| X509Certificate certificate = parsePEMCertificate(certificatePEM); | |
| RSAPublicKey publicKey = (RSAPublicKey) certificate.getPublicKey(); | |
| log.debug("Encrypting data for In-Flow Extension action using RSA-OAEP-256 and A256GCM"); |
| if (MultitenantConstants.SUPER_TENANT_DOMAIN_NAME.equals(tenantDomain)) { | ||
| privateKey = keyStoreManager.getDefaultPrivateKey(); | ||
| } else { | ||
| String tenantKeyStoreName = tenantDomain.trim().replace(".", "-") + ".jks"; |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 20
Suggested change
| String tenantKeyStoreName = tenantDomain.trim().replace(".", "-") + ".jks"; | |
| PRIVATE_KEYS.put(cacheKey, privateKey); | |
| log.debug("Successfully retrieved and cached private key for tenant: " + tenantDomain); |
Comment on lines
+72
to
+85
| * @return A two-element array: {@code [cleanPath, annotation]}. | ||
| * If no annotation is found, annotation element is {@code null}. | ||
| */ | ||
| public static String[] stripAnnotation(String rawPath) { | ||
|
|
||
| if (rawPath == null) { | ||
| return new String[]{null, null}; | ||
| } | ||
|
|
||
| Matcher matcher = ANNOTATION_PATTERN.matcher(rawPath); | ||
| if (matcher.find()) { | ||
| String cleanPath = rawPath.substring(0, matcher.start()); | ||
| String annotation = matcher.group(1); | ||
| return new String[]{cleanPath, annotation}; |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 21
Suggested change
| * @return A two-element array: {@code [cleanPath, annotation]}. | |
| * If no annotation is found, annotation element is {@code null}. | |
| */ | |
| public static String[] stripAnnotation(String rawPath) { | |
| if (rawPath == null) { | |
| return new String[]{null, null}; | |
| } | |
| Matcher matcher = ANNOTATION_PATTERN.matcher(rawPath); | |
| if (matcher.find()) { | |
| String cleanPath = rawPath.substring(0, matcher.start()); | |
| String annotation = matcher.group(1); | |
| return new String[]{cleanPath, annotation}; | |
| public static String[] stripAnnotation(String rawPath) { | |
| if (rawPath == null) { | |
| return new String[]{null, null}; | |
| } | |
| Matcher matcher = ANNOTATION_PATTERN.matcher(rawPath); | |
| if (matcher.find()) { | |
| String cleanPath = rawPath.substring(0, matcher.start()); | |
| String annotation = matcher.group(1); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Stripped annotation from path. Clean path: " + cleanPath + ", Annotation: " + annotation); | |
| } | |
| return new String[]{cleanPath, annotation}; | |
| } | |
| return new String[]{rawPath, null}; | |
| } |
Comment on lines
+283
to
+298
| * decryption, or when an external service serialises a complex object/array before encrypting it. | ||
| * | ||
| * <p>If the value is not a String, does not start with {@code [} or {@code {}, or cannot be | ||
| * parsed as valid JSON, the original value is returned unchanged.</p> | ||
| * | ||
| * @param value The value to inspect. | ||
| * @return The parsed JSON structure (List or Map), or the original value if not applicable. | ||
| */ | ||
| private static Object tryParseJsonString(Object value) { | ||
|
|
||
| if (!(value instanceof String)) { | ||
| return value; | ||
| } | ||
| String str = ((String) value).trim(); | ||
| if (!str.startsWith("[") && !str.startsWith("{")) { | ||
| return value; |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 22
Suggested change
| * decryption, or when an external service serialises a complex object/array before encrypting it. | |
| * | |
| * <p>If the value is not a String, does not start with {@code [} or {@code {}, or cannot be | |
| * parsed as valid JSON, the original value is returned unchanged.</p> | |
| * | |
| * @param value The value to inspect. | |
| * @return The parsed JSON structure (List or Map), or the original value if not applicable. | |
| */ | |
| private static Object tryParseJsonString(Object value) { | |
| if (!(value instanceof String)) { | |
| return value; | |
| } | |
| String str = ((String) value).trim(); | |
| if (!str.startsWith("[") && !str.startsWith("{")) { | |
| return value; | |
| private static Object tryParseJsonString(Object value) { | |
| if (!(value instanceof String)) { | |
| return value; | |
| } | |
| String str = ((String) value).trim(); | |
| if (!str.startsWith("[") && !str.startsWith("{")) { | |
| return value; | |
| } | |
| try { | |
| Object parsed = OBJECT_MAPPER.readValue(str, Object.class); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Successfully parsed JSON string value"); | |
| } | |
| return parsed; | |
| } catch (IOException ignored) { | |
| log.warn("Failed to parse value as JSON, returning original value"); | |
| return value; | |
| } | |
| } |
Comment on lines
+75
to
+77
| private static final String MODIFY_FIELD = "modify"; | ||
|
|
||
| @Override |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 23
Suggested change
| private static final String MODIFY_FIELD = "modify"; | |
| @Override | |
| public void onFlowUpdate(String flowType, GraphConfig graphConfig, int tenantId) | |
| throws FlowMgtFrameworkException { | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("Processing flow update for flowType: " + flowType + ", tenantId: " + tenantId); | |
| } |
Comment on lines
+164
to
+165
| + " in flow type " + flowType + " and stripped from executor metadata."); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 24
Suggested change
| + " in flow type " + flowType + " and stripped from executor metadata."); | |
| } | |
| } catch (ActionMgtException e) { | |
| LOG.error("Error updating access config override for action: " + actionId + " in flow type: " + flowType); | |
| throw new FlowMgtServerException("FLOW-60010", |
Comment on lines
+64
to
+68
| */ | ||
| @Override | ||
| public ActionDTO buildActionDTO(Action action) { | ||
|
|
||
| if (!(action instanceof InFlowExtensionAction)) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 25
Suggested change
| */ | |
| @Override | |
| public ActionDTO buildActionDTO(Action action) { | |
| if (!(action instanceof InFlowExtensionAction)) { | |
| @Override | |
| public ActionDTO buildActionDTO(Action action) { | |
| log.info("Converting InFlowExtensionAction to ActionDTO for action: " + action.getName()); | |
| if (!(action instanceof InFlowExtensionAction)) { |
Comment on lines
+89
to
+93
|
|
||
| @Override | ||
| public ActionDTO resolveForAddOperation(ActionDTO actionDTO, String tenantDomain) | ||
| throws ActionDTOModelResolverException { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 27
Suggested change
| @Override | |
| public ActionDTO resolveForAddOperation(ActionDTO actionDTO, String tenantDomain) | |
| throws ActionDTOModelResolverException { | |
| @Override | |
| public ActionDTO resolveForAddOperation(ActionDTO actionDTO, String tenantDomain) | |
| throws ActionDTOModelResolverException { | |
| LOG.info("Resolving ActionDTO for add operation. Action ID: " + actionDTO.getId()); | |
| Map<String, ActionProperty> properties = new HashMap<>(); |
* Diagnostic Logging: Sanitized external error messages in `ActionExecutionDiagnosticLogger` to prevent potential PII or sensitive data leaks. * Action Management: - Enforced consistent action name uniqueness validation across both create and update operations in `ActionManagementServiceImpl`. - Added null guards to `isActionNameAvailable` methods to prevent `NullPointerException`s. * Response Processing: claim validation has removed - fix irrelevant * DTO Model Resolver: - no need to fix path overlaps since all paths will be terminal. - Refactored certificate deletion to utilize `extractCertificateId()` rather than `.toString()`, ensuring the correct UUID is passed and preventing orphaned certificates. * Access Configuration: no need to match area prefix since all paths will be terminal.
…ow execution engine - fallback kept permissive until discussion
KD23243
force-pushed
the
feature/in-flow-extensions
branch
3 times, most recently
from
May 21, 2026 07:18
e47721d to
b1b784c
Compare
coderabbitai
Bot
requested review from
RushanNanayakkara,
ThaminduR,
ashanthamara and
piraveena
May 23, 2026 11:38
![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: 11
🧹 Nitpick comments (7)
components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/model/AccessConfig.java (1)
63-64: ⚡ Quick winSnapshot
expose/modifylists before making them unmodifiable.Current wrapping is shallow on the same list instances; external mutations can still change this config after construction.
Proposed fix
- this.expose = expose != null ? Collections.unmodifiableList(expose) : null; - this.modify = modify != null ? Collections.unmodifiableList(modify) : null; + this.expose = expose != null ? List.copyOf(expose) : null; + this.modify = modify != null ? List.copyOf(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 63 - 64, The constructor in AccessConfig currently wraps the incoming expose and modify lists with Collections.unmodifiableList(expose) which is a shallow wrapper around the same list instance; to prevent external mutation make defensive copies first (e.g., new ArrayList<>(expose)) and then wrap those copies with Collections.unmodifiableList, doing the same for modify and preserving null handling; update the lines that set this.expose and this.modify in the AccessConfig constructor to copy-then-wrap so the internal state cannot be altered by callers.components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/model/Action.java (1)
178-179: ⚡ Quick winAvoid storing externally mutable
attributeslist references.
attributesis assigned by reference in multiple paths, so caller-side list mutations can alter an already builtAction.Proposed fix
@@ public Action(ActionResponseBuilder actionResponseBuilder) { @@ - this.attributes = actionResponseBuilder.attributes; + this.attributes = actionResponseBuilder.attributes == null ? null : List.copyOf(actionResponseBuilder.attributes); } @@ public Action(ActionRequestBuilder actionRequestBuilder) { @@ - this.attributes = actionRequestBuilder.attributes; + this.attributes = actionRequestBuilder.attributes == null ? null : List.copyOf(actionRequestBuilder.attributes); } @@ public ActionResponseBuilder attributes(List<String> attributes) { - - this.attributes = attributes; + this.attributes = attributes == null ? null : List.copyOf(attributes); return this; } @@ public ActionRequestBuilder attributes(List<String> attributes) { @@ if (attributes == null || attributes.isEmpty()) { - this.attributes = attributes; + this.attributes = attributes == null ? null : List.copyOf(attributes); return this; }Also applies to: 188-189, 326-330, 382-384
🤖 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/model/Action.java` around lines 178 - 179, The Action class currently assigns the attributes list by reference (e.g., this.attributes = actionResponseBuilder.attributes), which allows external mutation; change all assignments (including where you set from actionResponseBuilder and any constructors or builder methods referenced) to defensive copies (e.g., this.attributes = attributes == null ? null : new ArrayList<>(attributes)) and ensure the getter returns an unmodifiable view or a copy (Collections.unmodifiableList(attributes) or new ArrayList<>(attributes)); also apply the same defensive-copy pattern to any setter/withAttributes methods so external callers cannot mutate internal state via the attributes list.components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/service/impl/DefaultActionValidator.java (1)
434-447: ⚡ Quick winAdd logs at handled claim-metadata failure boundaries.
This path handles two failure scenarios but emits no log before rethrowing, which weakens production traceability during attribute validation failures.
As per coding guidelines: "Suggest log statements at error handling boundaries (catch blocks, error returns)."♻️ Suggested logging update
try { if (claimMetadataManagementService == null) { + LOG.error("Claim metadata service is unavailable for action attribute validation."); throw ActionManagementExceptionHandler.handleServerException( ErrorMessage.ERROR_WHILE_RETRIEVING_CLAIM_METADATA, new IllegalStateException("Claim metadata management service is not available.")); } List<LocalClaim> localClaims = claimMetadataManagementService.getLocalClaims(tenantDomain); @@ } catch (ClaimMetadataException e) { + LOG.error("Failed to retrieve claim metadata for action attribute validation. Tenant: " + tenantDomain); throw ActionManagementExceptionHandler.handleServerException( ErrorMessage.ERROR_WHILE_RETRIEVING_CLAIM_METADATA, e); }🤖 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 434 - 447, The code path in DefaultActionValidator that calls claimMetadataManagementService.getLocalClaims lacks logs on failure; add error logs before rethrowing in both the null-service branch and the catch for ClaimMetadataException. Specifically, log an error with context (including tenantDomain and the exception) when claimMetadataManagementService is null prior to throwing the ActionManagementExceptionHandler error for ErrorMessage.ERROR_WHILE_RETRIEVING_CLAIM_METADATA, and log the caught ClaimMetadataException (with tenantDomain) inside the catch before rethrowing via ActionManagementExceptionHandler. Use the class logger (e.g., log or logger) so the messages point to DefaultActionValidator, reference claimMetadataManagementService, getLocalClaims, ClaimMetadataException and ErrorMessage.ERROR_WHILE_RETRIEVING_CLAIM_METADATA.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeMetadata.java (1)
42-43: ⚡ Quick winMake
contextTreetruly immutable with a defensive copy.At Line 42,
Collections.unmodifiableList(contextTree)still reflects later mutations to the original list reference. Copy first, then wrap.Proposed fix
- this.contextTree = contextTree != null ? Collections.unmodifiableList(contextTree) + this.contextTree = contextTree != null ? Collections.unmodifiableList(List.copyOf(contextTree)) : Collections.emptyList();🤖 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/InFlowExtensionContextTreeMetadata.java` around lines 42 - 43, The constructor in InFlowExtensionContextTreeMetadata currently assigns contextTree using Collections.unmodifiableList(contextTree), which still reflects mutations to the original list; make a defensive copy first and then wrap it to ensure true immutability by replacing that assignment with Collections.unmodifiableList(new ArrayList<>(contextTree)) (or Collections.emptyList() when null) so the field contextTree is backed by an independent, unmodifiable copy.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/internal/InFlowExtensionServiceComponent.java (1)
75-75: ⚡ Quick winUse INFO for activation/deactivation lifecycle transitions.
At Line 75 and Line 84, these are major component state transitions and should be logged at INFO instead of DEBUG.
As per coding guidelines, “Use INFO for major actions/significant state transitions.”
Also applies to: 84-84
🤖 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` at line 75, Replace DEBUG-level lifecycle logs with INFO in the InFlowExtensionServiceComponent class: change the LOG.debug call that logs "In-Flow Extension service successfully activated." and the corresponding LOG.debug for deactivation at the same class to LOG.info so activation/deactivation transitions are recorded as INFO-level events; locate these calls in the InFlowExtensionServiceComponent class (activation/deactivation methods) and update the logging method accordingly.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeNode.java (1)
50-57: ⚡ Quick winDefensively copy builder lists before wrapping unmodifiable.
At Line 50-57, both list fields can still change if the builder-provided lists are mutated elsewhere.
Proposed fix
- this.allowedOperations = b.allowedOperations != null - ? Collections.unmodifiableList(b.allowedOperations) : Collections.emptyList(); + this.allowedOperations = b.allowedOperations != null + ? Collections.unmodifiableList(List.copyOf(b.allowedOperations)) : Collections.emptyList(); ... - this.children = b.children != null - ? Collections.unmodifiableList(b.children) : Collections.emptyList(); + this.children = b.children != null + ? Collections.unmodifiableList(List.copyOf(b.children)) : Collections.emptyList();🤖 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 currently wraps builder lists directly with Collections.unmodifiableList allowing external mutation via the original builder lists; defensively copy the builder-provided lists for allowedOperations and children before wrapping them. In the InFlowExtensionContextTreeNode constructor (references: allowedOperations, children and builder variable b), replace Collections.unmodifiableList(b.allowedOperations) and Collections.unmodifiableList(b.children) with Collections.unmodifiableList(new ArrayList<>(b.allowedOperations)) and Collections.unmodifiableList(new ArrayList<>(b.children)) (handling null as you do now) so the node holds an immutable copy independent of the builder’s lists.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/management/InFlowExtensionActionDTOModelResolver.java (1)
428-431: ⚡ Quick winAdd logs at certificate-operation catch boundaries.
These catch blocks wrap and rethrow, but they do not emit logs for add/get/update/delete certificate failures. Add concise error logs (without sensitive payloads) before rethrowing.
As per coding guidelines, “Suggest log statements at error handling boundaries (catch blocks, error returns).”
Also applies to: 452-455, 476-479, 490-493, 519-522
🤖 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 428 - 431, The catch blocks that handle CertificateMgtException in InFlowExtensionActionDTOModelResolver currently rethrow without logging; add a concise error log (without sensitive payloads) immediately before each throw—include the operation type (add/get/update/delete) and the action id (actionDTO.getId()) and the exception in the logger call (e.g., logger.error("Error <operation> certificate for action: {}", actionDTO.getId(), e)); update the catch at the shown block and the similar catch blocks handling CertificateMgtException (the ones around lines handling add/get/update/delete) so every rethrow is preceded by a non-sensitive error log.
🤖 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`:
- Line 79: The code currently copies the map reference via putAll but still
shares mutable char[] values; change all places that populate or expose
userCredentials (the builder assignment that does
this.userCredentials.putAll(builder.userCredentials), the builder's
setter/constructor code at the other indicated spots, and any getUserCredentials
method) to defensively copy each char[] (e.g., Arrays.copyOf(value,
value.length)) when storing into the internal map and likewise copy each char[]
when returning values or building the unmodifiable map so callers never get a
direct reference to the original mutable arrays; update the builder to copy
incoming arrays into its map and ensure the User constructor copies
builder.userCredentials entries into its internal map using per-entry
Arrays.copyOf before wrapping with Collections.unmodifiableMap.
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 200-205: The code currently throws ActionExecutionRuntimeException
when getActionByActionId returns null, but that runtime exception is swallowed
upstream causing missing actionId to be treated as success; instead create and
throw a specific checked exception (e.g., ActionNotFoundException) from
ActionExecutorServiceImpl when action == null (replace the
ActionExecutionRuntimeException throw), update the method signature(s) that call
getActionByActionId/executeAction to declare the new checked exception, and
propagate the exception up so callers cannot silently swallow it (adjust callers
to catch or rethrow as appropriate) — reference symbols:
ActionExecutorServiceImpl, getActionByActionId, ActionExecutionRuntimeException,
and the new ActionNotFoundException to implement and propagate.
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`:
- Around line 210-221: The code in TaskExecutionNode treats a blank executor
userId as present and logs a username (PII); update checks to treat
blank/whitespace-only values from response.getUserId() as missing (e.g., use a
blank-safe check like StringUtils.isBlank(response.getUserId()) or trim+isEmpty)
so resolveUserIdFromUserStore(user, context.getTenantDomain()) runs when userId
is blank, and change any warning/error log statements that currently include
user.getUsername() or other PII to a non-PII message (e.g., "failed to resolve
user id for executor" or include only a safe identifier), applying the same
blank-safe check and logging change to the other block referenced (around lines
229-246) where response.getUserId()/username are handled.
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 107-113: The setClaims(Map<String, String> claims) method replaces
the internal claims map but does not update the cached username field, causing
getUsername() to return stale data; after clearing and updating this.claims (in
setClaims) update the cached username (the username field) from the incoming
claims (using the same claim key that getUsername() reads) and set it to null if
the key is absent so the username cache stays in sync with claims.
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 163-168: buildEvent() and other callers unconditionally call
applyOrganization(), which emits organization id/name/handle/depth from
thread-local state even when the action didn't opt into them; gate emission
behind the explicit exposure settings. Modify the call sites (e.g., in
buildEvent() where applyOrganization(eventBuilder) is invoked and the other
location that also calls applyOrganization) to only call applyOrganization when
the provided expose parameter authorizes organization fields, or change
applyOrganization(...) itself to check the expose/AccessConfig before adding org
id/name/handle/depth; ensure the organization node is represented in the
metadata/expose tree and that applyOrganization reads that flag (or an
equivalent expose.isOrganizationAllowed() check) so org fields are not sent
unless explicitly exposed.
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 575-577: The debug logging currently prints raw external-text
variables (errorMessage, errorDescription, failureReason, failureDescription) in
InFlowExtensionResponseProcessor; replace those raw values with either a safe
status/action identifier or a redacted placeholder before logging (e.g., log
extension response type/status and use "[REDACTED]" or a sanitized summary such
as string length or hash for the external fields). Update the LOG.debug calls
that reference errorMessage/errorDescription and
failureReason/failureDescription to avoid emitting verbatim external content and
only include non-sensitive identifiers or redacted summaries.
- Around line 131-137: The loop in InFlowExtensionResponseProcessor that
iterates over operations (calls decryptOperationValueIfNeeded and
processOperation) must guard against null entries: before calling
decryptOperationValueIfNeeded(operation, accessConfig, tenantDomain) or using
operation in any way, check if operation == null and either skip it (continue)
or fail fast by logging and throwing an appropriate validation/BadRequest
exception so malformed extension responses containing null elements do not cause
NPEs; apply the same null-entry check and handling to the second loop referenced
(around the processOperation usage at the other spot, lines 480-485) to ensure
consistent contract validation.
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 85-95: The code in JWEEncryptionUtil currently casts
certificate.getPublicKey() to RSAPublicKey without validating the key type,
which can cause an uncaught ClassCastException; update the encrypt logic after
parsePEMCertificate(certificatePEM) to verify the certificate.getPublicKey() is
an instance of RSAPublicKey before casting, and if not, throw an
ActionExecutionException with a clear message (so the method's error path is
honored); keep the existing JOSEException handling for jweObject.encrypt(...)
but ensure the new type-check prevents non-RSA certs from bypassing that error
handling.
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 76-78: The catch block in InFlowExtensionServiceComponent (in the
activate method) currently swallows all Throwables by only logging them; change
the catch (Throwable e) handler to log the error using LOG.error(...) and then
re-throw the exception (either rethrow e or wrap it in a RuntimeException) so DS
marks activation as failed after service-registration errors instead of leaving
the component partially active.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeBuilder.java`:
- Around line 153-163: The tree currently advertises a selectable "username"
node (InFlowExtensionContextTreeNode with key "username" and path
"/user/username") but the runtime (InFlowExtensionRequestBuilder.buildUser(...))
does not populate username, so selecting it yields empty data; fix by either
removing the username node from InFlowExtensionContextTreeBuilder (delete the
builder block that creates key "username") or update
InFlowExtensionRequestBuilder.buildUser(...) to include and forward the user's
username into the outbound user payload (ensure the field name matches
"/user/username") so the metadata matches runtime behavior.
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-46: The constructor of FlowContextHandoverConfig currently
stores unmodifiable views of caller-provided sets and then derives/stores policy
flags, which allows external mutation of the original sets to desync flags; fix
this by defensively copying the incoming sets (e.g., new
HashSet<>(includedAttributes) and new HashSet<>(includedUserAttributes)) before
wrapping them with Collections.unmodifiableSet and before computing/storing
fullUserPassthrough or any derived flags in the class, and apply the same
defensive-copy pattern to the other block that sets
includedAttributes/includedUserAttributes/fullUserPassthrough (the second
occurrence handling those fields) so flags are always derived from an immutable
snapshot.
---
Nitpick comments:
In
`@components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/api/model/Action.java`:
- Around line 178-179: The Action class currently assigns the attributes list by
reference (e.g., this.attributes = actionResponseBuilder.attributes), which
allows external mutation; change all assignments (including where you set from
actionResponseBuilder and any constructors or builder methods referenced) to
defensive copies (e.g., this.attributes = attributes == null ? null : new
ArrayList<>(attributes)) and ensure the getter returns an unmodifiable view or a
copy (Collections.unmodifiableList(attributes) or new ArrayList<>(attributes));
also apply the same defensive-copy pattern to any setter/withAttributes methods
so external callers cannot mutate internal state via the attributes list.
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 434-447: The code path in DefaultActionValidator that calls
claimMetadataManagementService.getLocalClaims lacks logs on failure; add error
logs before rethrowing in both the null-service branch and the catch for
ClaimMetadataException. Specifically, log an error with context (including
tenantDomain and the exception) when claimMetadataManagementService is null
prior to throwing the ActionManagementExceptionHandler error for
ErrorMessage.ERROR_WHILE_RETRIEVING_CLAIM_METADATA, and log the caught
ClaimMetadataException (with tenantDomain) inside the catch before rethrowing
via ActionManagementExceptionHandler. Use the class logger (e.g., log or logger)
so the messages point to DefaultActionValidator, reference
claimMetadataManagementService, getLocalClaims, ClaimMetadataException and
ErrorMessage.ERROR_WHILE_RETRIEVING_CLAIM_METADATA.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/internal/InFlowExtensionServiceComponent.java`:
- Line 75: Replace DEBUG-level lifecycle logs with INFO in the
InFlowExtensionServiceComponent class: change the LOG.debug call that logs
"In-Flow Extension service successfully activated." and the corresponding
LOG.debug for deactivation at the same class to LOG.info so
activation/deactivation transitions are recorded as INFO-level events; locate
these calls in the InFlowExtensionServiceComponent class
(activation/deactivation methods) and update the logging method accordingly.
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 428-431: The catch blocks that handle CertificateMgtException in
InFlowExtensionActionDTOModelResolver currently rethrow without logging; add a
concise error log (without sensitive payloads) immediately before each
throw—include the operation type (add/get/update/delete) and the action id
(actionDTO.getId()) and the exception in the logger call (e.g.,
logger.error("Error <operation> certificate for action: {}", actionDTO.getId(),
e)); update the catch at the shown block and the similar catch blocks handling
CertificateMgtException (the ones around lines handling add/get/update/delete)
so every rethrow is preceded by a non-sensitive error log.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/metadata/InFlowExtensionContextTreeMetadata.java`:
- Around line 42-43: The constructor in InFlowExtensionContextTreeMetadata
currently assigns contextTree using Collections.unmodifiableList(contextTree),
which still reflects mutations to the original list; make a defensive copy first
and then wrap it to ensure true immutability by replacing that assignment with
Collections.unmodifiableList(new ArrayList<>(contextTree)) (or
Collections.emptyList() when null) so the field contextTree is backed by an
independent, unmodifiable copy.
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 currently wraps builder lists directly with
Collections.unmodifiableList allowing external mutation via the original builder
lists; defensively copy the builder-provided lists for allowedOperations and
children before wrapping them. In the InFlowExtensionContextTreeNode constructor
(references: allowedOperations, children and builder variable b), replace
Collections.unmodifiableList(b.allowedOperations) and
Collections.unmodifiableList(b.children) with Collections.unmodifiableList(new
ArrayList<>(b.allowedOperations)) and Collections.unmodifiableList(new
ArrayList<>(b.children)) (handling null as you do now) so the node holds an
immutable copy independent of the builder’s lists.
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 63-64: The constructor in AccessConfig currently wraps the
incoming expose and modify lists with Collections.unmodifiableList(expose) which
is a shallow wrapper around the same list instance; to prevent external mutation
make defensive copies first (e.g., new ArrayList<>(expose)) and then wrap those
copies with Collections.unmodifiableList, doing the same for modify and
preserving null handling; update the lines that set this.expose and this.modify
in the AccessConfig constructor to copy-then-wrap so the internal state cannot
be altered by callers.
🪄 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: 3c70e17f-17a1-46dc-9aac-53f33c7973cd
📒 Files selected for processing (66)
.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.xml
💤 Files with no reviewable changes (4)
- components/flow-orchestration-framework/pom.xml
- components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/resources/testng.xml
- components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/resources/repository/conf/carbon.xml
- components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/test/java/org/wso2/carbon/identity/flow/extensions/util/InFlowExtensionPathUtilTest.java
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- components/entitlement/pom.xml
- 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/main/java/org/wso2/carbon/identity/flow/extensions/InFlowExtensionConstants.java
- components/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/test/java/org/wso2/carbon/identity/flow/execution/engine/validation/InputValidationServiceTest.java
|
|
||
| this.id = builder.id; | ||
| this.claims.addAll(builder.claims); | ||
| this.userCredentials.putAll(builder.userCredentials); |
Contributor
There was a problem hiding this comment.
Defensively copy credential arrays instead of sharing references.
userCredentials is wrapped as unmodifiable, but each char[] value is still mutable and currently shared by reference. That allows external mutation after build/get, which is risky for credential data integrity and secrecy.
Proposed fix
@@
public User(Builder builder) {
@@
- this.userCredentials.putAll(builder.userCredentials);
+ builder.userCredentials.forEach((k, v) ->
+ this.userCredentials.put(k, v != null ? v.clone() : null));
@@
public Map<String, char[]> getUserCredentials() {
-
- return Collections.unmodifiableMap(userCredentials);
+ Map<String, char[]> safeCopy = new HashMap<>();
+ userCredentials.forEach((k, v) -> safeCopy.put(k, v != null ? v.clone() : null));
+ return Collections.unmodifiableMap(safeCopy);
}
@@
public Builder userCredentials(Map<String, char[]> userCredentials) {
-
- this.userCredentials.putAll(userCredentials);
+ if (userCredentials == null) {
+ return this;
+ }
+ userCredentials.forEach((k, v) -> this.userCredentials.put(k, v != null ? v.clone() : null));
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`
at line 79, The code currently copies the map reference via putAll but still
shares mutable char[] values; change all places that populate or expose
userCredentials (the builder assignment that does
this.userCredentials.putAll(builder.userCredentials), the builder's
setter/constructor code at the other indicated spots, and any getUserCredentials
method) to defensively copy each char[] (e.g., Arrays.copyOf(value,
value.length)) when storing into the internal map and likewise copy each char[]
when returning values or building the unmodifiable map so callers never get a
direct reference to the original mutable arrays; update the builder to copy
incoming arrays into its map and ensure the User constructor copies
builder.userCredentials entries into its internal map using per-entry
Arrays.copyOf before wrapping with Collections.unmodifiableMap.
Comment on lines
+200
to
+205
| Action action = ActionExecutionServiceComponentHolder.getInstance().getActionManagementService() | ||
| .getActionByActionId(Action.ActionTypes.valueOf(actionType.name()).getPathParam(), actionId, | ||
| tenantDomain); | ||
| if (action == null) { | ||
| throw new ActionExecutionRuntimeException("No action found for action Id: " + actionId); | ||
| } |
Contributor
There was a problem hiding this comment.
Do not convert “action not found” into a silently skipped success path.
Throwing ActionExecutionRuntimeException here is swallowed upstream (Line 154), so an explicit actionId miss is treated as successful execution instead of a failure.
Suggested fix
- if (action == null) {
- throw new ActionExecutionRuntimeException("No action found for action Id: " + actionId);
- }
+ if (action == null) {
+ throw new ActionExecutionException("No action found for action Id: " + actionId);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Action action = ActionExecutionServiceComponentHolder.getInstance().getActionManagementService() | |
| .getActionByActionId(Action.ActionTypes.valueOf(actionType.name()).getPathParam(), actionId, | |
| tenantDomain); | |
| if (action == null) { | |
| throw new ActionExecutionRuntimeException("No action found for action Id: " + actionId); | |
| } | |
| Action action = ActionExecutionServiceComponentHolder.getInstance().getActionManagementService() | |
| .getActionByActionId(Action.ActionTypes.valueOf(actionType.name()).getPathParam(), actionId, | |
| tenantDomain); | |
| if (action == null) { | |
| throw new ActionExecutionException("No action found for action Id: " + actionId); | |
| } |
🤖 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 - 205, The code currently throws
ActionExecutionRuntimeException when getActionByActionId returns null, but that
runtime exception is swallowed upstream causing missing actionId to be treated
as success; instead create and throw a specific checked exception (e.g.,
ActionNotFoundException) from ActionExecutorServiceImpl when action == null
(replace the ActionExecutionRuntimeException throw), update the method
signature(s) that call getActionByActionId/executeAction to declare the new
checked exception, and propagate the exception up so callers cannot silently
swallow it (adjust callers to catch or rethrow as appropriate) — reference
symbols: ActionExecutorServiceImpl, getActionByActionId,
ActionExecutionRuntimeException, and the new ActionNotFoundException to
implement and propagate.
Comment on lines
+210
to
+221
| if (response.getUserId() != null) { | ||
| user.setUserId(response.getUserId()); | ||
| } | ||
| if (response.getUpdatedUserClaims() != null) { | ||
| response.getUpdatedUserClaims().forEach((key, value) -> user.addClaim(key, String.valueOf(value))); | ||
| } | ||
| if (response.getUserCredentials() != null) { | ||
| user.getUserCredentials().putAll(response.getUserCredentials()); | ||
| } | ||
| if (user.getUserId() == null) { | ||
| resolveUserIdFromUserStore(user, context.getTenantDomain()); | ||
| } |
Contributor
There was a problem hiding this comment.
Treat blank executor userId as missing and remove username from warning logs.
Blank response.getUserId() currently prevents fallback resolution, and the catch log exposes a username. Use blank-safe checks and a non-PII log message.
🔧 Proposed fix
FlowUser user = context.getFlowUser();
- if (response.getUserId() != null) {
+ if (StringUtils.isNotBlank(response.getUserId())) {
user.setUserId(response.getUserId());
}
@@
- if (user.getUserId() == null) {
+ if (StringUtils.isBlank(user.getUserId())) {
resolveUserIdFromUserStore(user, context.getTenantDomain());
}
@@
- } catch (Exception e) {
- LOG.warn("Failed to resolve userId for user '" + username + "' from user store.", e);
+ } catch (Exception e) {
+ LOG.warn("Failed to resolve userId from user store.", e);
}As per coding guidelines, "Never log sensitive data: secrets/tokens/passwords/PII; avoid logging entire objects—log only safe identifiers/fields."
Also applies to: 229-246
🤖 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.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/graph/TaskExecutionNode.java`
around lines 210 - 221, The code in TaskExecutionNode treats a blank executor
userId as present and logs a username (PII); update checks to treat
blank/whitespace-only values from response.getUserId() as missing (e.g., use a
blank-safe check like StringUtils.isBlank(response.getUserId()) or trim+isEmpty)
so resolveUserIdFromUserStore(user, context.getTenantDomain()) runs when userId
is blank, and change any warning/error log statements that currently include
user.getUsername() or other PII to a non-PII message (e.g., "failed to resolve
user id for executor" or include only a safe identifier), applying the same
blank-safe check and logging change to the other block referenced (around lines
229-246) where response.getUserId()/username are handled.
Comment on lines
+107
to
+113
| public void setClaims(Map<String, String> claims) { | ||
|
|
||
| this.claims.clear(); | ||
| if (claims != null) { | ||
| this.claims.putAll(claims); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Keep username cache in sync when replacing claims.
setClaims(...) replaces the claims map but leaves the cached username unchanged, so getUsername() can return stale data.
🔧 Proposed fix
public void setClaims(Map<String, String> claims) {
this.claims.clear();
if (claims != null) {
this.claims.putAll(claims);
}
+ this.username = this.claims.get(USERNAME_CLAIM_URI);
}🤖 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.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/model/FlowUser.java`
around lines 107 - 113, The setClaims(Map<String, String> claims) method
replaces the internal claims map but does not update the cached username field,
causing getUsername() to return stale data; after clearing and updating
this.claims (in setClaims) update the cached username (the username field) from
the incoming claims (using the same claim key that getUsername() reads) and set
it to null if the key is absent so the username cache stays in sync with claims.
Comment on lines
+575
to
+577
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug("Processing error response from In-Flow Extension. Error: " + errorMessage + | ||
| ", Description: " + errorDescription); |
Contributor
There was a problem hiding this comment.
Don't log raw error/failure text returned by the extension.
errorMessage / errorDescription and failureReason / failureDescription come from the external service. Logging them verbatim lets arbitrary remote content land in auth logs, including PII or secrets echoed by the extension. Log only the status/action type, or redact before logging.
Suggested change
if (LOG.isDebugEnabled()) {
- LOG.debug("Processing error response from In-Flow Extension. Error: " + errorMessage +
- ", Description: " + errorDescription);
+ LOG.debug("Processing error response from In-Flow Extension.");
}
@@
if (LOG.isDebugEnabled()) {
- LOG.debug("Processing failure response from In-Flow Extension. Reason: " + failureReason +
- ", Description: " + failureDescription);
+ LOG.debug("Processing failure response from In-Flow Extension.");
}As per coding guidelines, "Never log sensitive data: secrets/tokens/passwords/PII; avoid logging entire objects—log only safe identifiers/fields."
Also applies to: 591-593
🤖 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 575 - 577, The debug logging currently prints raw external-text
variables (errorMessage, errorDescription, failureReason, failureDescription) in
InFlowExtensionResponseProcessor; replace those raw values with either a safe
status/action identifier or a redacted placeholder before logging (e.g., log
extension response type/status and use "[REDACTED]" or a sanitized summary such
as string length or hash for the external fields). Update the LOG.debug calls
that reference errorMessage/errorDescription and
failureReason/failureDescription to avoid emitting verbatim external content and
only include non-sensitive identifiers or redacted summaries.
Comment on lines
+85
to
+95
| try { | ||
| X509Certificate certificate = parsePEMCertificate(certificatePEM); | ||
| RSAPublicKey publicKey = (RSAPublicKey) certificate.getPublicKey(); | ||
|
|
||
| JWEHeader header = new JWEHeader.Builder(JWEAlgorithm.RSA_OAEP_256, EncryptionMethod.A256GCM) | ||
| .contentType("application/json") | ||
| .build(); | ||
|
|
||
| JWEObject jweObject = new JWEObject(header, new Payload(plaintext)); | ||
| jweObject.encrypt(new RSAEncrypter(publicKey)); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/JWEEncryptionUtil.java"
echo "=== File exists? ==="
ls -l "$FILE"
echo "=== Methods/usage of ActionExecutionException in file ==="
rg -n "ActionExecutionException" "$FILE" || true
echo "=== encrypt(...) method context ==="
# Print around likely area; also find encrypt signature
rg -n "encrypt\\s*\\(" "$FILE"
ENCRYPT_LINE="$(rg -n "encrypt\\s*\\(" "$FILE" | head -n1 | cut -d: -f1 || true)"
if [ -n "${ENCRYPT_LINE:-}" ]; then
START=$((ENCRYPT_LINE-40))
END=$((ENCRYPT_LINE+80))
sed -n "${START},${END}p" "$FILE" | nl -ba | sed 's/^/ /'
fi
echo "=== parsePEMCertificate(...) context ==="
rg -n "parsePEMCertificate\\s*\\(" "$FILE"
PARSE_LINE="$(rg -n "parsePEMCertificate\\s*\\(" "$FILE" | head -n1 | cut -d: -f1 || true)"
if [ -n "${PARSE_LINE:-}" ]; then
START=$((PARSE_LINE-40))
END=$((PARSE_LINE+120))
sed -n "${START},${END}p" "$FILE" | nl -ba | sed 's/^/ /'
fi
echo "=== Determine whether ClassCastException is caught in encrypt(...) ==="
# Heuristic: show all catch blocks in file
rg -n "catch\\s*\\(" "$FILE"Repository: wso2/carbon-identity-framework
Length of output: 2046
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/JWEEncryptionUtil.java"
FILE2="components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/JWEEncryptionUtil.java"
# Use the correct path (in case of typo)
if [ -f "$FILE2" ]; then
FILE="$FILE2"
fi
echo "=== encrypt(...) method body (approx 70-120) ==="
sed -n '70,120p' "$FILE" | awk '{printf "%6d %s\n", NR+69, $0}'
echo
echo "=== parsePEMCertificate(...) method body (approx 190-260) ==="
sed -n '190,260p' "$FILE" | awk '{printf "%6d %s\n", NR+189, $0}'
echo
echo "=== catch(...) blocks in file ==="
rg -n "catch\\s*\\(" "$FILE" || trueRepository: wso2/carbon-identity-framework
Length of output: 6333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extensions/src/main/java/org/wso2/carbon/identity/flow/extensions/executor/JWEEncryptionUtil.java"
echo "=== Top of file (imports) ==="
sed -n '1,60p' "$FILE" | nl -ba 2>/dev/null || sed -n '1,60p' "$FILE"
echo
echo "=== Current import for PublicKey (if any) ==="
rg -n "import\\s+java\\.security\\.PublicKey\\b" "$FILE" || trueRepository: wso2/carbon-identity-framework
Length of output: 2658
Validate certificate public key type before casting to RSAPublicKey
encrypt(...) only catches JOSEException, but it blindly casts certificate.getPublicKey() to RSAPublicKey. Since parsePEMCertificate(...) only checks validity (expiry/not-yet-valid/decoding) and does not restrict key algorithm, non-RSA certs will throw an uncaught ClassCastException and bypass the method’s declared ActionExecutionException error path.
Suggested change
+import java.security.PublicKey;
@@
try {
X509Certificate certificate = parsePEMCertificate(certificatePEM);
- RSAPublicKey publicKey = (RSAPublicKey) certificate.getPublicKey();
+ PublicKey publicKey = certificate.getPublicKey();
+ if (!(publicKey instanceof RSAPublicKey)) {
+ throw new ActionExecutionException(
+ "Outbound encryption certificate must contain an RSA public key.");
+ }
+ RSAPublicKey rsaPublicKey = (RSAPublicKey) publicKey;
@@
- jweObject.encrypt(new RSAEncrypter(publicKey));
+ jweObject.encrypt(new RSAEncrypter(rsaPublicKey));🤖 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 85 - 95, The code in JWEEncryptionUtil currently casts
certificate.getPublicKey() to RSAPublicKey without validating the key type,
which can cause an uncaught ClassCastException; update the encrypt logic after
parsePEMCertificate(certificatePEM) to verify the certificate.getPublicKey() is
an instance of RSAPublicKey before casting, and if not, throw an
ActionExecutionException with a clear message (so the method's error path is
honored); keep the existing JOSEException handling for jweObject.encrypt(...)
but ensure the new type-check prevents non-RSA certs from bypassing that error
handling.
Comment on lines
+76
to
+78
| } catch (Throwable e) { | ||
| LOG.error("Error while initiating In-Flow Extension service", e); | ||
| } |
Contributor
There was a problem hiding this comment.
Do not swallow activation failure after service-registration errors.
At Line 76-78, catching Throwable and only logging can leave the component active but incompletely registered. Re-throw after logging so DS marks activation as failed.
Proposed fix
- } catch (Throwable e) {
- LOG.error("Error while initiating In-Flow Extension service", e);
+ } catch (Throwable e) {
+ LOG.error("Error while initiating In-Flow Extension service", e);
+ throw new IllegalStateException("In-Flow Extension service activation failed.", e);
}📝 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
| } catch (Throwable e) { | |
| LOG.error("Error while initiating In-Flow Extension service", e); | |
| } | |
| } catch (Throwable e) { | |
| LOG.error("Error while initiating In-Flow Extension service", e); | |
| throw new IllegalStateException("In-Flow Extension service activation failed.", e); | |
| } |
🤖 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 76 - 78, The catch block in InFlowExtensionServiceComponent (in the
activate method) currently swallows all Throwables by only logging them; change
the catch (Throwable e) handler to log the error using LOG.error(...) and then
re-throw the exception (either rethrow e or wrap it in a RuntimeException) so DS
marks activation as failed after service-registration errors instead of leaving
the component partially active.
Comment on lines
+153
to
+163
| if (fullPassthrough || userAttrs.contains("username")) { | ||
| children.add(InFlowExtensionContextTreeNode.builder() | ||
| .key("username") | ||
| .title("Username") | ||
| .path("/user/username") | ||
| .dataType(DATA_TYPE_STRING) | ||
| .nodeType(NODE_LEAF) | ||
| .allowedOperations(Collections.singletonList(OP_EXPOSE)) | ||
| .replaceable(false) | ||
| .build()); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't advertise /user/username until the runtime payload supports it.
This node is selectable in metadata, but InFlowExtensionRequestBuilder.buildUser(...) only forwards userId, claims, credentials, and userStoreDomain. Choosing username in the composer will therefore serialize nothing at runtime. Either remove this node for now or wire username through the outbound user model in the same PR.
🤖 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/InFlowExtensionContextTreeBuilder.java`
around lines 153 - 163, The tree currently advertises a selectable "username"
node (InFlowExtensionContextTreeNode with key "username" and path
"/user/username") but the runtime (InFlowExtensionRequestBuilder.buildUser(...))
does not populate username, so selecting it yields empty data; fix by either
removing the username node from InFlowExtensionContextTreeBuilder (delete the
builder block that creates key "username") or update
InFlowExtensionRequestBuilder.buildUser(...) to include and forward the user's
username into the outbound user payload (ensure the field name matches
"/user/username") so the metadata matches runtime behavior.
Comment on lines
+44
to
+46
| this.includedAttributes = Collections.unmodifiableSet(includedAttributes); | ||
| this.includedUserAttributes = Collections.unmodifiableSet(includedUserAttributes); | ||
| this.fullUserPassthrough = fullUserPassthrough; |
Contributor
There was a problem hiding this comment.
Snapshot attribute sets before deriving/storing policy flags.
This class currently keeps references to caller-provided sets. Later external mutation can desync includedAttributes from fullUserPassthrough, causing inconsistent handover behavior.
Proposed fix
@@
import java.util.Collections;
+import java.util.HashSet;
import java.util.Set;
@@
private FlowContextHandoverConfig(Set<String> includedAttributes,
Set<String> includedUserAttributes,
boolean fullUserPassthrough) {
- this.includedAttributes = Collections.unmodifiableSet(includedAttributes);
- this.includedUserAttributes = Collections.unmodifiableSet(includedUserAttributes);
+ this.includedAttributes = Collections.unmodifiableSet(new HashSet<>(includedAttributes));
+ this.includedUserAttributes = Collections.unmodifiableSet(new HashSet<>(includedUserAttributes));
this.fullUserPassthrough = fullUserPassthrough;
}
@@
- Set<String> resolvedAttrs = (attrs != null) ? attrs : Collections.emptySet();
- Set<String> resolvedUserAttrs = (userAttrs != null) ? userAttrs : Collections.emptySet();
+ Set<String> resolvedAttrs = (attrs != null) ? new HashSet<>(attrs) : Collections.emptySet();
+ Set<String> resolvedUserAttrs = (userAttrs != null) ? new HashSet<>(userAttrs) : Collections.emptySet();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 - 46, The constructor of FlowContextHandoverConfig currently
stores unmodifiable views of caller-provided sets and then derives/stores policy
flags, which allows external mutation of the original sets to desync flags; fix
this by defensively copying the incoming sets (e.g., new
HashSet<>(includedAttributes) and new HashSet<>(includedUserAttributes)) before
wrapping them with Collections.unmodifiableSet and before computing/storing
fullUserPassthrough or any derived flags in the class, and apply the same
defensive-copy pattern to the other block that sets
includedAttributes/includedUserAttributes/fullUserPassthrough (the second
occurrence handling those fields) so flags are always derived from an immutable
snapshot.
|
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
4 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 encryptionApproach
InFlowExtensionActionextendsActionwith anAccessConfig(expose/modify context paths with encryption flags) and anEncryptionconfiguration (certificate for JWE)User 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