Feature - Flow Extension Executor (stacked on #8109)#8112
Open
KD23243 wants to merge 20 commits into
Open
Conversation
- 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.
- Rename component org.wso2.carbon.identity.flow.extensions → org.wso2.carbon.identity.flow.extension - Rename feature org.wso2.carbon.identity.flow.extensions.server.feature → org.wso2.carbon.identity.flow.extension.server.feature - Update all Java package declarations and imports accordingly - Change action type constant FLOW_EXTENSIONS → FLOW_EXTENSION
Introduces a new FLOW_EXTENSION action type and the org.wso2.carbon.identity.flow.extension module enabling custom extension points invoked mid-flow. Wires the action executor and configuration paths, adds the DTO model resolver and converter, the AccessConfig (expose/modify paths with encryption metadata), path type annotation utilities, and action name uniqueness validation for the new type.
Merges the FLOW_EXTENSION action type work from PR wso2#8109. Conflicts on 6 overlapping files (ActionExecutorServiceImpl, ErrorMessage, ActionManagementServiceImpl, flow.extension/pom.xml, PathTypeAnnotationUtil, AccessConfig) resolved in favor of wso2#8109 since it is the further-along reference and stays the single source of truth for those changes.
- Delete 4 duplicates (InFlowExtensionConstants, InFlowExtensionAction, InFlowExtensionActionConverter, InFlowExtensionActionDTOModelResolver) now provided by wso2#8109. - Extend FlowExtensionAction with Encryption field, flowTypeOverrides map, and resolveAccessConfig(flowType) needed by the executor. - Add ACCESS_CONFIG_EXPOSE_PREFIX / MODIFY_PREFIX / OVERRIDE_KEY_SEPARATOR to FlowExtensionConstants.ActionManagement. - Port encryption serialization and per-flow-type override handling into FlowExtensionActionConverter and FlowExtensionActionDTOModelResolver. - Rename InFlow* to FlowExtension* across executor, request builder, response processor, internal, metadata, model, util, and tests. - Migrate path-prefix constant references to FlowExtensionConstants.FlowContextPaths.
Contributor
📝 WalkthroughWalkthroughAdds a new Flow Extension action type and a full flow-extension module with executor, request builder, response processor, models, utilities, OSGi wiring, and tests. Updates action/management enums/configs, engine constants/behavior, build modules, and server feature configs to enable and package the extension. ChangesFlow Extension integration across action, engine, and new module
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
coderabbitai
Bot
requested review from
RushanNanayakkara,
ThaminduR and
piraveena
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+238
to
+242
| @Override | ||
| public void serialize(UserStore value, JsonGenerator gen, SerializerProvider serializers) throws IOException { | ||
|
|
||
| gen.writeString(value.getName() != null ? value.getName() : ""); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| @Override | |
| public void serialize(UserStore value, JsonGenerator gen, SerializerProvider serializers) throws IOException { | |
| gen.writeString(value.getName() != null ? value.getName() : ""); | |
| } | |
| @Override | |
| public void serialize(UserStore value, JsonGenerator gen, SerializerProvider serializers) throws IOException { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Serializing UserStore domain for user."); | |
| } | |
| gen.writeString(value.getName() != null ? value.getName() : ""); | |
| } |
Comment on lines
154
to
+160
| } catch (ActionExecutionRuntimeException e) { | ||
| LOG.debug("Skip executing action for action type: " + actionType.name(), e); | ||
| // Skip executing actions when no action available is considered as action execution being successful. | ||
| Action.ActionTypes.Category category = Action.ActionTypes.valueOf(actionType.toString()).getCategory(); | ||
| if (Action.ActionTypes.Category.FLOW_EXTENSION.equals(category)) { | ||
| throw new ActionExecutionException( | ||
| "Failed to execute flow extension action with id: " + actionId |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| } catch (ActionExecutionRuntimeException e) { | |
| LOG.debug("Skip executing action for action type: " + actionType.name(), e); | |
| // Skip executing actions when no action available is considered as action execution being successful. | |
| Action.ActionTypes.Category category = Action.ActionTypes.valueOf(actionType.toString()).getCategory(); | |
| if (Action.ActionTypes.Category.FLOW_EXTENSION.equals(category)) { | |
| throw new ActionExecutionException( | |
| "Failed to execute flow extension action with id: " + actionId | |
| } catch (ActionExecutionRuntimeException e) { | |
| LOG.debug("Skip executing action for action type: " + actionType.name(), e); | |
| // Skip executing actions when no action available is considered as action execution being successful. | |
| Action.ActionTypes.Category category = Action.ActionTypes.valueOf(actionType.toString()).getCategory(); | |
| if (Action.ActionTypes.Category.FLOW_EXTENSION.equals(category)) { | |
| LOG.error("Failed to execute flow extension action with id: " + actionId + " for action type: " + actionType.name() + ". Error: " + e.getMessage()); | |
| throw new ActionExecutionException( |
Comment on lines
89
to
92
| return isActionTypeEnabled(ActionTypeConfig.PRE_ISSUE_ID_TOKEN.getActionTypeEnableProperty()); | ||
| case FLOW_EXTENSION: | ||
| return isActionTypeEnabled(ActionTypeConfig.FLOW_EXTENSION.getActionTypeEnableProperty()); | ||
| default: |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| return isActionTypeEnabled(ActionTypeConfig.PRE_ISSUE_ID_TOKEN.getActionTypeEnableProperty()); | |
| case FLOW_EXTENSION: | |
| return isActionTypeEnabled(ActionTypeConfig.FLOW_EXTENSION.getActionTypeEnableProperty()); | |
| default: | |
| public boolean isExecutionForActionTypeEnabled(ActionType actionType) { | |
| switch (actionType) { | |
| case PRE_ISSUE_ACCESS_TOKEN: | |
| return isActionTypeEnabled(ActionTypeConfig.PRE_ISSUE_ACCESS_TOKEN.getActionTypeEnableProperty()); | |
| case PRE_UPDATE_PASSWORD: | |
| return isActionTypeEnabled(ActionTypeConfig.PRE_UPDATE_PASSWORD.getActionTypeEnableProperty()); | |
| case PRE_UPDATE_PROFILE: | |
| return isActionTypeEnabled(ActionTypeConfig.PRE_UPDATE_PROFILE.getActionTypeEnableProperty()); | |
| case PRE_ISSUE_ID_TOKEN: | |
| return isActionTypeEnabled(ActionTypeConfig.PRE_ISSUE_ID_TOKEN.getActionTypeEnableProperty()); | |
| case FLOW_EXTENSION: | |
| if (log.isDebugEnabled()) { | |
| log.debug("Checking if FLOW_EXTENSION action type is enabled"); | |
| } | |
| return isActionTypeEnabled(ActionTypeConfig.FLOW_EXTENSION.getActionTypeEnableProperty()); |
Comment on lines
40
to
43
| public static ActionConverter getActionConverter(Action.ActionTypes actionType) { | ||
|
|
||
| switch (actionType) { | ||
| case PRE_UPDATE_PROFILE: | ||
| return actionConverters.get(Action.ActionTypes.PRE_UPDATE_PROFILE); | ||
| case PRE_UPDATE_PASSWORD: | ||
| return actionConverters.get(Action.ActionTypes.PRE_UPDATE_PASSWORD); | ||
| case PRE_ISSUE_ACCESS_TOKEN: | ||
| default: | ||
| return null; | ||
| } | ||
| return actionConverters.get(actionType); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| public static ActionConverter getActionConverter(Action.ActionTypes actionType) { | |
| switch (actionType) { | |
| case PRE_UPDATE_PROFILE: | |
| return actionConverters.get(Action.ActionTypes.PRE_UPDATE_PROFILE); | |
| case PRE_UPDATE_PASSWORD: | |
| return actionConverters.get(Action.ActionTypes.PRE_UPDATE_PASSWORD); | |
| case PRE_ISSUE_ACCESS_TOKEN: | |
| default: | |
| return null; | |
| } | |
| return actionConverters.get(actionType); | |
| } | |
| public static ActionConverter getActionConverter(Action.ActionTypes actionType) { | |
| ActionConverter converter = actionConverters.get(actionType); | |
| if (converter == null) { | |
| log.debug("No ActionConverter found for action type: " + actionType); | |
| } | |
| return converter; |
Comment on lines
77
to
79
| Action.ActionTypes castedActionType = Action.ActionTypes.valueOf(resolvedActionType); | ||
| ActionValidatorFactory.getActionValidator(castedActionType).doPreAddActionValidations( | ||
| castedActionType, ActionManagementConfig.getInstance().getLatestVersion(castedActionType), action); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| Action.ActionTypes castedActionType = Action.ActionTypes.valueOf(resolvedActionType); | |
| ActionValidatorFactory.getActionValidator(castedActionType).doPreAddActionValidations( | |
| castedActionType, ActionManagementConfig.getInstance().getLatestVersion(castedActionType), action); | |
| Action.ActionTypes castedActionType = Action.ActionTypes.valueOf(resolvedActionType); | |
| log.info("Adding new action with type: " + castedActionType + " for tenant: " + tenantDomain); | |
| ActionValidatorFactory.getActionValidator(castedActionType).doPreAddActionValidations( |
Comment on lines
+500
to
+502
| throw ActionManagementExceptionHandler.handleClientException( | ||
| ErrorMessage.ERROR_ACTION_NAME_ALREADY_EXISTS, name, actionType.getActionType()); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| throw ActionManagementExceptionHandler.handleClientException( | |
| ErrorMessage.ERROR_ACTION_NAME_ALREADY_EXISTS, name, actionType.getActionType()); | |
| } | |
| throw ActionManagementExceptionHandler.handleClientException( | |
| ErrorMessage.ERROR_ACTION_NAME_ALREADY_EXISTS, name, actionType.getActionType()); | |
| log.error("Action name already exists: " + name + " for action type: " + actionType.getActionType()); | |
| } |
Comment on lines
96
to
+101
| case PRE_ISSUE_ID_TOKEN: | ||
| return getVersion(ActionTypeConfig.PRE_ISSUE_ID_TOKEN.getLatestVersionProperty(), actionType); | ||
| return getVersion( | ||
| ActionTypeConfig.PRE_ISSUE_ID_TOKEN.getLatestVersionProperty(), actionType); | ||
| case FLOW_EXTENSION: | ||
| return getVersion( | ||
| ActionTypeConfig.FLOW_EXTENSION.getLatestVersionProperty(), actionType); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| case PRE_ISSUE_ID_TOKEN: | |
| return getVersion(ActionTypeConfig.PRE_ISSUE_ID_TOKEN.getLatestVersionProperty(), actionType); | |
| return getVersion( | |
| ActionTypeConfig.PRE_ISSUE_ID_TOKEN.getLatestVersionProperty(), actionType); | |
| case FLOW_EXTENSION: | |
| return getVersion( | |
| ActionTypeConfig.FLOW_EXTENSION.getLatestVersionProperty(), actionType); | |
| return getVersion( | |
| ActionTypeConfig.PRE_ISSUE_ID_TOKEN.getLatestVersionProperty(), actionType); | |
| case FLOW_EXTENSION: | |
| log.debug("Retrieving latest version for FLOW_EXTENSION action type"); | |
| return getVersion( | |
| ActionTypeConfig.FLOW_EXTENSION.getLatestVersionProperty(), actionType); |
Comment on lines
102
to
103
| default: | ||
| throw new ActionMgtServerException("Unsupported action type: " + actionType); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
Suggested change
| default: | |
| throw new ActionMgtServerException("Unsupported action type: " + actionType); | |
| default: | |
| log.error("Unsupported action type encountered: " + actionType); | |
| throw new ActionMgtServerException("Unsupported action type: " + actionType); |
Comment on lines
+145
to
147
| .additionalInfo(response.getAdditionalInfo()) | ||
| .error(response.getErrorMessage()) | ||
| .build(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 9
Suggested change
| .additionalInfo(response.getAdditionalInfo()) | |
| .error(response.getErrorMessage()) | |
| .build(); | |
| .additionalInfo(response.getAdditionalInfo()) | |
| .error(response.getErrorMessage()) | |
| .build(); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Building user input required response with additional info present: " + | |
| (response.getAdditionalInfo() != null)); | |
| } |
Comment on lines
+88
to
+90
| LOG.debug("Executing Flow Extension action. actionId: " + actionId | ||
| + ", flowType: " + context.getFlowType() | ||
| + ", tenant: " + context.getTenantDomain()); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 10
Suggested change
| LOG.debug("Executing Flow Extension action. actionId: " + actionId | |
| + ", flowType: " + context.getFlowType() | |
| + ", tenant: " + context.getTenantDomain()); | |
| } | |
| LOG.info("Starting Flow Extension action execution. actionId: " + actionId); | |
| ActionExecutorService actionExecutorService = getActionExecutorService(); |
Comment on lines
+114
to
+117
| .add(FlowExtensionConstants.FLOW_EXECUTION_CONTEXT_KEY, filteredContext); | ||
|
|
||
| ActionExecutionStatus<?> executionStatus = actionExecutorService.execute( | ||
| ActionType.FLOW_EXTENSION, actionId, flowContext, context.getTenantDomain()); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 11
Suggested change
| .add(FlowExtensionConstants.FLOW_EXECUTION_CONTEXT_KEY, filteredContext); | |
| ActionExecutionStatus<?> executionStatus = actionExecutorService.execute( | |
| ActionType.FLOW_EXTENSION, actionId, flowContext, context.getTenantDomain()); | |
| ExecutorResponse executionResponse = mapExecutionStatus(executionStatus, flowContext, context, actionId); | |
| if (ExecutorStatus.STATUS_COMPLETE.equals(executionResponse.getResult())) { | |
| LOG.info("Flow Extension action executed successfully. actionId: " + actionId); | |
| } else if (ExecutorStatus.STATUS_RETRY.equals(executionResponse.getResult())) { | |
| LOG.warn("Flow Extension action execution failed with retry status. actionId: " + actionId); | |
| } else if (ExecutorStatus.STATUS_ERROR.equals(executionResponse.getResult())) { | |
| LOG.error("Flow Extension action execution failed with error. actionId: " + actionId + ", errorCode: " + executionResponse.getErrorCode()); | |
| } | |
| // On success, extract pending context updates collected by the response processor |
Comment on lines
+78
to
+82
|
|
||
| @Override | ||
| public ActionExecutionRequest buildActionExecutionRequest(FlowContext flowContext, | ||
| ActionExecutionRequestContext actionExecutionContext) | ||
| throws ActionExecutionRequestBuilderException { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 12
Suggested change
| @Override | |
| public ActionExecutionRequest buildActionExecutionRequest(FlowContext flowContext, | |
| ActionExecutionRequestContext actionExecutionContext) | |
| throws ActionExecutionRequestBuilderException { | |
| public ActionExecutionRequest buildActionExecutionRequest(FlowContext flowContext, | |
| ActionExecutionRequestContext actionExecutionContext) | |
| throws ActionExecutionRequestBuilderException { | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("Building ActionExecutionRequest for FlowExtension action type."); | |
| } |
Comment on lines
+502
to
+506
| private void applyApplication(FlowExtensionEvent.Builder eventBuilder, FlowExecutionContext context, | ||
| List<String> expose) { | ||
|
|
||
| if (!isLeafExposed(FlowExtensionConstants.FlowContextPaths.FLOW_APP_ID_PATH, expose)) { | ||
| return; |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 13
Suggested change
| private void applyApplication(FlowExtensionEvent.Builder eventBuilder, FlowExecutionContext context, | |
| List<String> expose) { | |
| if (!isLeafExposed(FlowExtensionConstants.FlowContextPaths.FLOW_APP_ID_PATH, expose)) { | |
| return; | |
| private void applyFlowProperties(FlowExtensionEvent.Builder eventBuilder, FlowExecutionContext context, | |
| List<String> expose, AccessConfig accessConfig, | |
| String certificatePEM) throws ActionExecutionRequestBuilderException { | |
| if (!isAreaExposed(FlowExtensionConstants.FlowContextPaths.PROPERTIES_PATH_PREFIX, expose)) { | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("Flow properties are not exposed in the configuration."); | |
| } |
Comment on lines
+94
to
+98
| return ActionType.FLOW_EXTENSION; | ||
| } | ||
|
|
||
| @Override | ||
| @SuppressWarnings("unchecked") |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 14
Suggested change
| return ActionType.FLOW_EXTENSION; | |
| } | |
| @Override | |
| @SuppressWarnings("unchecked") | |
| @Override | |
| @SuppressWarnings("unchecked") | |
| public ActionExecutionStatus<Success> processSuccessResponse(FlowContext flowContext, | |
| ActionExecutionResponseContext<ActionInvocationSuccessResponse> responseContext) | |
| throws ActionExecutionResponseProcessorException { | |
| if (LOG.isInfoEnabled()) { | |
| LOG.info("Processing success response for In-Flow Extension action."); | |
| } |
Comment on lines
+340
to
+343
| LOG.debug("ClaimMetadataManagementService is unavailable. Skipping claim existence check for: " | ||
| + claimUri); | ||
| } | ||
| return null; |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 15
Suggested change
| LOG.debug("ClaimMetadataManagementService is unavailable. Skipping claim existence check for: " | |
| + claimUri); | |
| } | |
| return null; | |
| } catch (ClaimMetadataException e) { | |
| LOG.error("Failed to look up claim URI '" + claimUri + "' in tenant '" + tenantDomain + "'.", e); | |
| return "Failed to verify claim existence due to system error."; | |
| } |
Comment on lines
+82
to
+84
| */ | ||
| public static String encrypt(String plaintext, String certificatePEM) throws ActionExecutionException { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 16
Suggested change
| */ | |
| public static String encrypt(String plaintext, String certificatePEM) throws ActionExecutionException { | |
| try { | |
| X509Certificate certificate = parsePEMCertificate(certificatePEM); | |
| RSAPublicKey publicKey = (RSAPublicKey) certificate.getPublicKey(); | |
| LOG.info("Encrypting data for In-Flow Extension action."); |
Comment on lines
+94
to
+95
| jweObject.encrypt(new RSAEncrypter(publicKey)); | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 17
Suggested change
| jweObject.encrypt(new RSAEncrypter(publicKey)); | |
| } catch (JOSEException e) { | |
| LOG.error("Failed to JWE-encrypt data for In-Flow Extension action."); | |
| throw new ActionExecutionException( |
Comment on lines
+67
to
+79
| */ | ||
| @SuppressWarnings("unchecked") | ||
| public static Object coerceValue(String path, Object value, Map<String, String> pathTypeAnnotations) { | ||
|
|
||
| if (value == null) { | ||
| return null; | ||
| } | ||
| String annotation = pathTypeAnnotations.get(path); | ||
| if (annotation == null) { | ||
| return String.valueOf(value); | ||
| } | ||
| if (annotation.startsWith("[")) { | ||
| String inner = annotation.substring(1, annotation.length() - 1); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 18
Suggested change
| */ | |
| @SuppressWarnings("unchecked") | |
| public static Object coerceValue(String path, Object value, Map<String, String> pathTypeAnnotations) { | |
| if (value == null) { | |
| return null; | |
| } | |
| String annotation = pathTypeAnnotations.get(path); | |
| if (annotation == null) { | |
| return String.valueOf(value); | |
| } | |
| if (annotation.startsWith("[")) { | |
| String inner = annotation.substring(1, annotation.length() - 1); | |
| @SuppressWarnings("unchecked") | |
| public static Object coerceValue(String path, Object value, Map<String, String> pathTypeAnnotations) { | |
| if (value == null) { | |
| return null; | |
| } | |
| String annotation = pathTypeAnnotations.get(path); | |
| if (annotation == null) { | |
| return String.valueOf(value); | |
| } | |
| if (log.isDebugEnabled()) { | |
| log.debug("Coercing value for path: " + path + " with annotation: " + annotation); | |
| } |
Comment on lines
+177
to
+190
| * Handles serialized JSON arriving from external services (e.g. after JWE decryption). | ||
| */ | ||
| 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 { | ||
| return OBJECT_MAPPER.readValue(str, Object.class); | ||
| } catch (IOException ignored) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 19
Suggested change
| * Handles serialized JSON arriving from external services (e.g. after JWE decryption). | |
| */ | |
| 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 { | |
| return OBJECT_MAPPER.readValue(str, Object.class); | |
| } catch (IOException ignored) { | |
| 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 { | |
| return OBJECT_MAPPER.readValue(str, Object.class); | |
| } catch (IOException ignored) { | |
| log.warn("Failed to parse JSON string for value starting with '" + str.charAt(0) + "'"); | |
| return value; | |
| } |
Comment on lines
+55
to
+58
|
|
||
| @Activate | ||
| protected void activate(ComponentContext context) { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 20
Suggested change
| @Activate | |
| protected void activate(ComponentContext context) { | |
| @Activate | |
| protected void activate(ComponentContext context) { | |
| LOG.info("Activating In-Flow Extension service component."); | |
| try { |
Comment on lines
+74
to
+77
|
|
||
| LOG.debug("In-Flow Extension service successfully activated."); | ||
| } catch (Throwable e) { | ||
| LOG.error("Error while initiating In-Flow Extension service", e); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 21
Suggested change
| LOG.debug("In-Flow Extension service successfully activated."); | |
| } catch (Throwable e) { | |
| LOG.error("Error while initiating In-Flow Extension service", e); | |
| LOG.debug("In-Flow Extension service successfully activated."); | |
| } catch (Throwable e) { | |
| LOG.error("Error while initiating In-Flow Extension service: " + e.getMessage()); | |
| throw new RuntimeException("Failed to activate In-Flow Extension service", e); | |
| } |
Comment on lines
+65
to
+69
| */ | ||
| @Override | ||
| public ActionDTO buildActionDTO(Action action) { | ||
|
|
||
| if (!(action instanceof FlowExtensionAction flowExtensionAction)) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 22
Suggested change
| */ | |
| @Override | |
| public ActionDTO buildActionDTO(Action action) { | |
| if (!(action instanceof FlowExtensionAction flowExtensionAction)) { | |
| @Override | |
| public ActionDTO buildActionDTO(Action action) { | |
| if (!(action instanceof FlowExtensionAction flowExtensionAction)) { | |
| log.debug("Action is not of type FlowExtensionAction, returning basic ActionDTO"); | |
| return new ActionDTO.Builder(action).build(); | |
| } | |
| if (log.isDebugEnabled()) { | |
| log.debug("Converting FlowExtensionAction to ActionDTO for action: " + flowExtensionAction.getName()); | |
| } |
Comment on lines
+140
to
+143
| * | ||
| * @param actionDTO The ActionDTO to convert. | ||
| * @return FlowExtensionAction with access config and overrides populated. | ||
| */ |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 23
Suggested change
| * | |
| * @param actionDTO The ActionDTO to convert. | |
| * @return FlowExtensionAction with access config and overrides populated. | |
| */ | |
| @Override | |
| public Action buildAction(ActionDTO actionDTO) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Converting ActionDTO to FlowExtensionAction for action id: " + actionDTO.getId()); | |
| } | |
| // Default access config. |
Comment on lines
+74
to
+79
| } | ||
|
|
||
| @Override | ||
| public ActionDTO resolveForAddOperation(ActionDTO actionDTO, String tenantDomain) | ||
| throws ActionDTOModelResolverException { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 24
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 flow extension action for add operation. Action ID: " + actionDTO.getId()); | |
| Map<String, ActionProperty> properties = new HashMap<>(); |
Comment on lines
+371
to
+375
| .name(certName) | ||
| .certificateContent(certificatePEM) | ||
| .build(), | ||
| tenantDomain); | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 25
Suggested change
| .name(certName) | |
| .certificateContent(certificatePEM) | |
| .build(), | |
| tenantDomain); | |
| } catch (CertificateMgtException e) { | |
| log.error("Failed to add certificate for action ID: " + actionDTO.getId() + ". Error: " + e.getMessage()); | |
| throw new ActionDTOModelResolverException("Error storing certificate for action: " | |
| + actionDTO.getId(), e); | |
| } |
Comment on lines
+68
to
+71
| * @return a fully populated metadata DTO. | ||
| */ | ||
| public FlowExtensionContextTreeMetadata build(String flowType) { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 26
Suggested change
| * @return a fully populated metadata DTO. | |
| */ | |
| public FlowExtensionContextTreeMetadata build(String flowType) { | |
| public FlowExtensionContextTreeMetadata build(String flowType) { | |
| log.info("Building flow extension context tree metadata for flowType: {}", flowType != null ? flowType : "default"); | |
| Set<String> attrs = handoverConfig.getIncludedAttributes(); |
Comment on lines
+129
to
+133
| * by the task execution node. EXPOSE is added only when the attribute is configured.</li> | ||
| * </ul> | ||
| * | ||
| * <p>{@code userAttrs == null} signals full-passthrough (show and expose everything). | ||
| */ |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 27
Suggested change
| * by the task execution node. EXPOSE is added only when the attribute is configured.</li> | |
| * </ul> | |
| * | |
| * <p>{@code userAttrs == null} signals full-passthrough (show and expose everything). | |
| */ | |
| private FlowExtensionContextTreeNode buildUserNode(Set<String> userAttrs) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Building user node with {} user attributes", userAttrs != null ? userAttrs.size() : "full passthrough"); | |
| } | |
| // userAttrs == null → full passthrough set by the caller (build()). | |
| boolean fullPassthrough = (userAttrs == null); |
Comment on lines
+36
to
+46
| public FlowExtensionContextTreeMetadata(String flowType, | ||
| List<FlowExtensionContextTreeNode> contextTree, | ||
| boolean redirectionEnabled, | ||
| boolean allowReadOnlyClaimsModification) { | ||
|
|
||
| this.flowType = flowType; | ||
| this.contextTree = contextTree != null ? Collections.unmodifiableList(contextTree) | ||
| : Collections.emptyList(); | ||
| this.redirectionEnabled = redirectionEnabled; | ||
| this.allowReadOnlyClaimsModification = allowReadOnlyClaimsModification; | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 28
Suggested change
| public FlowExtensionContextTreeMetadata(String flowType, | |
| List<FlowExtensionContextTreeNode> contextTree, | |
| boolean redirectionEnabled, | |
| boolean allowReadOnlyClaimsModification) { | |
| this.flowType = flowType; | |
| this.contextTree = contextTree != null ? Collections.unmodifiableList(contextTree) | |
| : Collections.emptyList(); | |
| this.redirectionEnabled = redirectionEnabled; | |
| this.allowReadOnlyClaimsModification = allowReadOnlyClaimsModification; | |
| } | |
| public FlowExtensionContextTreeMetadata(String flowType, | |
| List<FlowExtensionContextTreeNode> contextTree, | |
| boolean redirectionEnabled, | |
| boolean allowReadOnlyClaimsModification) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Creating FlowExtensionContextTreeMetadata for flowType: " + flowType + | |
| ", contextTreeSize: " + (contextTree != null ? contextTree.size() : 0) + | |
| ", redirectionEnabled: " + redirectionEnabled + | |
| ", allowReadOnlyClaimsModification: " + allowReadOnlyClaimsModification); | |
| } | |
| this.flowType = flowType; | |
| this.contextTree = contextTree != null ? Collections.unmodifiableList(contextTree) | |
| : Collections.emptyList(); | |
| this.redirectionEnabled = redirectionEnabled; | |
| this.allowReadOnlyClaimsModification = allowReadOnlyClaimsModification; | |
| } |
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.
|
![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: 7
🧹 Nitpick comments (7)
components/action-mgt/org.wso2.carbon.identity.action.management/src/main/java/org/wso2/carbon/identity/action/management/internal/service/impl/ActionManagementServiceImpl.java (1)
484-501: ⚡ Quick winAdd observability logs around the new DB-backed uniqueness validation path.
This helper performs a tenant/type query (Line 494) and a handled duplicate-decision path (Line 499) without logs, which makes production diagnostics harder.
Suggested logging additions
private void validateActionNameUniqueness(String name, String excludeId, ActionTypes actionType, int tenantId) throws ActionMgtException { if (!ActionTypes.FLOW_EXTENSION.equals(actionType)) { return; } + if (LOG.isDebugEnabled()) { + LOG.debug(String.format("Validating FLOW_EXTENSION action name uniqueness. tenantId: %d, excludeId: %s", + tenantId, excludeId)); + } if (StringUtils.isBlank(name)) { throw ActionManagementExceptionHandler.handleClientException( ErrorMessage.ERROR_ACTION_NAME_BLANK); } List<ActionDTO> existingActions = DAO_FACADE.getActionsByActionType(actionType.getActionType(), tenantId); @@ if (duplicateExists) { + LOG.warn(String.format("Duplicate FLOW_EXTENSION action name detected for tenantId: %d", tenantId)); throw ActionManagementExceptionHandler.handleClientException( ErrorMessage.ERROR_ACTION_NAME_ALREADY_EXISTS, name, actionType.getActionType()); } }As per coding guidelines: add logs around major method executions, key decision points, and handled failure paths.
🤖 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 484 - 501, The validateActionNameUniqueness method lacks observability: add structured debug/info logs at entry (log method name, actionType, tenantId and whether excludeId is present), after the DAO_FACADE.getActionsByActionType call (log number of actions returned and actionType), at the duplicate decision point (log value of duplicateExists and the compared name) and on the handled failure path before throwing via ActionManagementExceptionHandler.handleClientException so the exception path is recorded; use the existing logger instance and avoid logging sensitive payloads (log excludeId presence, not full user data).components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/metadata/FlowExtensionContextTreeService.java (1)
51-55: ⚡ Quick winAdd a lightweight invocation log for this service entry point.
buildContextTreeis a public API path; a guarded DEBUG log here improves traceability for flow-type-specific metadata builds.💡 Proposed fix
+import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.wso2.carbon.identity.flow.extension.model.FlowContextHandoverConfig; @@ public final class FlowExtensionContextTreeService { + private static final Log LOG = LogFactory.getLog(FlowExtensionContextTreeService.class); private static final FlowExtensionContextTreeService INSTANCE = new FlowExtensionContextTreeService(); @@ public FlowExtensionContextTreeMetadata buildContextTree(String flowType) { + if (LOG.isDebugEnabled()) { + LOG.debug("Building Flow Extension context tree for flow type: " + flowType); + } return new FlowExtensionContextTreeBuilder( FlowContextHandoverConfig.defaultPolicy()).build(flowType); } }As per coding guidelines: "Flag entry/exit points of critical services that don't log their invocation" and "DEBUG only when needed for troubleshooting and always guard with if (log.isDebugEnabled())".
🤖 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/main/java/org/wso2/carbon/identity/flow/extension/metadata/FlowExtensionContextTreeService.java` around lines 51 - 55, Add a guarded DEBUG entry log to the public service method buildContextTree in FlowExtensionContextTreeService: inside buildContextTree, before creating FlowExtensionContextTreeBuilder/ calling build(flowType), check if (log.isDebugEnabled()) and log a lightweight message including the method name and the flowType value to mark invocation (e.g., "Entering buildContextTree for flowType={}" style); keep the message concise and ensure you use the existing logger instance in the class.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/executor/JWEEncryptionUtil.java (1)
66-68: ⚖️ Poor tradeoffConsider bounded cache for multi-tenant scalability.
The
PRIVATE_KEYScache grows unboundedly as entries for new tenants are added but never evicted (expired entries remain in the map until accessed). In large multi-tenant deployments, this could cause gradual memory growth. Consider using a bounded cache (e.g., Guava'sCachewithexpireAfterWriteandmaximumSize) or periodically purging expired entries.🤖 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/main/java/org/wso2/carbon/identity/flow/extension/executor/JWEEncryptionUtil.java` around lines 66 - 68, The PRIVATE_KEYS map currently stores CachedKey entries indefinitely (even after PRIVATE_KEY_CACHE_TTL_MS), causing unbounded growth across tenants; replace the ConcurrentHashMap-based cache with a bounded, time-expiring cache (e.g., com.google.common.cache.Cache) configured with expireAfterWrite(PRIVATE_KEY_CACHE_TTL_MS) and a sensible maximumSize, or implement a periodic cleanup task that iterates PRIVATE_KEYS and removes entries whose CachedKey.timestamp + PRIVATE_KEY_CACHE_TTL_MS < now; update all usages that read/write PRIVATE_KEYS to use the new cache API and keep the CachedKey type for stored values.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/executor/FlowExtensionResponseProcessor.java (1)
632-644: 💤 Low valueMinor: Redundant ObjectMapper configuration.
Both
setSerializationInclusioncalls configure the same mapper. The second call tosetSerializationInclusion(JsonInclude.Include.NON_EMPTY)effectively overwrites the first one. If you intend to exclude both null and empty values, only the second line is needed (sinceNON_EMPTYimpliesNON_NULL).Suggested fix
if (LOG.isDebugEnabled()) { ObjectMapper mapper = new ObjectMapper(); - mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL); mapper.setSerializationInclusion(JsonInclude.Include.NON_EMPTY); try {🤖 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/main/java/org/wso2/carbon/identity/flow/extension/executor/FlowExtensionResponseProcessor.java` around lines 632 - 644, The ObjectMapper configuration in FlowExtensionResponseProcessor is redundant: remove the first call to mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL) and keep only mapper.setSerializationInclusion(JsonInclude.Include.NON_EMPTY) so null and empty values are excluded; locate the ObjectMapper instance created in the debug block (variable name mapper) and drop the overwrite call before serializing results for LOG.debug.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/internal/FlowExtensionServiceComponent.java (1)
75-75: ⚡ Quick winUse INFO for activate/deactivate success logs.
Service lifecycle transitions are major state changes and should be logged at INFO, not DEBUG.
As per coding guidelines, “Use INFO for major actions/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.extension/src/main/java/org/wso2/carbon/identity/flow/extension/internal/FlowExtensionServiceComponent.java` at line 75, Replace the DEBUG-level lifecycle logs in FlowExtensionServiceComponent with INFO: change the LOG.debug(...) calls in the activate and deactivate methods (the two occurrences currently logging "In-Flow Extension service successfully activated." and its deactivate counterpart) to LOG.info(...) and keep the existing messages unchanged so major service state transitions are logged at INFO level.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/management/FlowExtensionActionDTOModelResolver.java (1)
357-458: ⚡ Quick winAdd non-sensitive logs around certificate lifecycle operations.
These methods perform significant external operations (add/get/update/delete certificate) but currently have no observability logs for major decision paths (add vs update vs explicit removal vs carry-forward).
As per coding guidelines, “Flag functions or methods that perform significant operations (DB calls, API calls, auth, payments) but lack log statements,” and “Use INFO for major actions/state transitions.”
🤖 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/main/java/org/wso2/carbon/identity/flow/extension/management/FlowExtensionActionDTOModelResolver.java` around lines 357 - 458, Add INFO-level logs to the certificate lifecycle methods to improve observability: in handleCertificateAdd log when starting an add (include action id and tenantDomain) and on success/failure; in handleCertificateGet log before attempting the get and on success/failure; in handleCertificateUpdate log the chosen branch (explicit removal, update existing, add new, or carry-forward) with action ids and tenantDomain and log success/failure for delete/update operations; in handleCertificateDelete log before deleting and on success/failure. Use the existing logger instance (e.g., log or processLogger) and include the unique symbols actionDTO.getId()/updatingActionDTO.getId()/deletingActionDTO.getId(), certificateManagementService method names, and tenantDomain in the messages.components/flow-orchestration-framework/org.wso2.carbon.identity.flow.execution.engine/src/main/java/org/wso2/carbon/identity/flow/execution/engine/graph/TaskExecutionNode.java (1)
139-147: ⚡ Quick winAdd a retry-path log for observability in
STATUS_RETRY.This is a handled non-success path. A safe WARN/INFO log (without PII) will make retry loops diagnosable.
🔎 Suggested logging addition
case STATUS_RETRY: + if (LOG.isWarnEnabled()) { + LOG.warn("Executor returned STATUS_RETRY for flow type: " + flowType + + ", error code: " + response.getErrorCode()); + } return new NodeResponse.Builder() .status(STATUS_INCOMPLETE) .type(VIEW) .requiredData(response.getRequiredData()) .optionalData(response.getOptionalData()) .additionalInfo(response.getAdditionalInfo()) .error(response.getErrorMessage()) .build();As per coding guidelines
Add logs around major method executions, key decision points, handled failure paths, and exceptional-but-handled 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/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 139 - 147, Add a non-PII observability log in the STATUS_RETRY branch of TaskExecutionNode to record that a retry path was taken; locate the switch/case handling STATUS_RETRY in TaskExecutionNode and before returning the NodeResponse.Builder(...) emit a safe-level log (INFO or WARN) that includes contextual identifiers such as the node id or task name (use existing fields or getters on TaskExecutionNode) and a short retry reason (e.g., response.getErrorMessage() or response.getAdditionalInfo() truncated/cleansed) so operators can trace retry loops without logging sensitive data.
🤖 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 User class currently copies Builder.userCredentials via putAll
and wraps with Collections.unmodifiableMap but leaves the stored char[] mutable
and Builder.userCredentials(...) can NPE on null; fix by defensively copying the
map and each char[] value when setting Builder.userCredentials and when
constructing User (instead of plain putAll/unmodifiableMap), e.g., create a new
HashMap, for each entry copy the key and clone the char[] (or create a new
char[] and System.arraycopy) before putting it into the map, handle null input
in Builder.userCredentials by treating null as an empty map (or throwing a clear
IllegalArgumentException), and apply the same defensive-copy pattern in the
other affected spots (the builder setter and any other places around lines
referenced) so the internal userCredentials map and its char[] values are
immutable from external mutation.
In
`@components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/internal/util/ActionExecutorConfig.java`:
- Around line 425-430: ActionTypeConfig.FLOW_EXTENSION defines allowed/excluded
header/parameter config keys but the reader methods
(getExcludedHeadersInActionRequestForActionType,
getExcludedParamsInActionRequestForActionType, getAllowedHeadersForActionType,
getAllowedParamsForActionType) only handle PRE_ISSUE_ACCESS_TOKEN, so
FLOW_EXTENSION filters are ignored; update each of those reader methods to
include a branch/case for ActionTypeConfig.FLOW_EXTENSION that reads the
corresponding keys from ActionExecutorConfig.FLOW_EXTENSION (e.g., the
AllowedHeaders.Header, AllowedParameters.Parameter, ExcludedHeaders.Header,
ExcludedParameters.Parameter entries) and returns those values just like the
PRE_ISSUE_ACCESS_TOKEN branch does.
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-493: The method in ActionManagementServiceImpl is rejecting
blank/missing name unconditionally (throwing
ErrorMessage.ERROR_ACTION_NAME_BLANK) which breaks PATCH semantics; change the
logic so blank or null name is only rejected for create/full-update flows, not
for PATCH/partial updates—i.e., guard the current blank-name check and the
uniqueness validation (the check invoked around the symbol referenced at line
~166) so they run only when the request explicitly supplies a non-blank name (or
when the operation is not a PATCH), and skip both validations when the name is
omitted in a PATCH. Ensure you reference the same method in
ActionManagementServiceImpl where the name and uniqueness checks are performed
and only validate uniqueness when name != null/blank.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/internal/FlowExtensionServiceComponent.java`:
- Around line 56-79: The activate() method in FlowExtensionServiceComponent
registers several OSGi services via bundleContext.registerService(...) but does
not keep ServiceRegistration handles and does not unregister them on
deactivate(), and it swallows activation exceptions; update
FlowExtensionServiceComponent to store each ServiceRegistration (for Executor,
ActionExecutionRequestBuilder, ActionExecutionResponseProcessor,
ActionConverter, ActionDTOModelResolver) as instance fields, call unregister()
on each non-null registration in deactivate(), and in activate() ensure that if
an exception occurs you unregister any registrations already created and rethrow
a runtime/activation exception so activation fails cleanly; also change
LOG.error("Error while initiating In-Flow Extension service", e) to
LOG.error("Error while initiating In-Flow Extension service: " + e.getMessage())
(log only the message, not the exception object).
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/metadata/FlowExtensionContextTreeMetadata.java`:
- Around line 42-43: FlowExtensionContextTreeMetadata currently assigns
this.contextTree = Collections.unmodifiableList(contextTree) which keeps a
reference to the caller's list; change the constructor so it defensively copies
the input before wrapping it: if contextTree != null create a new
ArrayList<>(contextTree) and then wrap that copy with
Collections.unmodifiableList(...) (otherwise use Collections.emptyList()),
ensuring the field contextTree cannot be affected by subsequent external
mutations.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/model/FlowContextHandoverConfig.java`:
- Around line 44-45: The constructor of FlowContextHandoverConfig currently
wraps caller-provided sets directly into Collections.unmodifiableSet (fields
includedAttributes and includedUserAttributes), which allows external mutation
if the original sets are later changed; fix this by making defensive copies
first (e.g., new HashSet<>(includedAttributes) and new
HashSet<>(includedUserAttributes)) and then wrap those copies with
Collections.unmodifiableSet before assigning to the includedAttributes and
includedUserAttributes fields so the object truly holds an immutable snapshot.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/resources/testng.xml`:
- Around line 23-29: The test suite (testng.xml) currently lists executor tests
but omits the newly added test class
org.wso2.carbon.identity.flow.extension.util.FlowExtensionPathUtilTest, so that
test is silently skipped; update the <classes> block in testng.xml by adding an
entry for FlowExtensionPathUtilTest (class name
"org.wso2.carbon.identity.flow.extension.util.FlowExtensionPathUtilTest")
alongside the existing class entries so the test suite includes and executes
this new test.
---
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 484-501: The validateActionNameUniqueness method lacks
observability: add structured debug/info logs at entry (log method name,
actionType, tenantId and whether excludeId is present), after the
DAO_FACADE.getActionsByActionType call (log number of actions returned and
actionType), at the duplicate decision point (log value of duplicateExists and
the compared name) and on the handled failure path before throwing via
ActionManagementExceptionHandler.handleClientException so the exception path is
recorded; use the existing logger instance and avoid logging sensitive payloads
(log excludeId presence, not full user data).
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 139-147: Add a non-PII observability log in the STATUS_RETRY
branch of TaskExecutionNode to record that a retry path was taken; locate the
switch/case handling STATUS_RETRY in TaskExecutionNode and before returning the
NodeResponse.Builder(...) emit a safe-level log (INFO or WARN) that includes
contextual identifiers such as the node id or task name (use existing fields or
getters on TaskExecutionNode) and a short retry reason (e.g.,
response.getErrorMessage() or response.getAdditionalInfo() truncated/cleansed)
so operators can trace retry loops without logging sensitive data.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/executor/FlowExtensionResponseProcessor.java`:
- Around line 632-644: The ObjectMapper configuration in
FlowExtensionResponseProcessor is redundant: remove the first call to
mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL) and keep only
mapper.setSerializationInclusion(JsonInclude.Include.NON_EMPTY) so null and
empty values are excluded; locate the ObjectMapper instance created in the debug
block (variable name mapper) and drop the overwrite call before serializing
results for LOG.debug.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/executor/JWEEncryptionUtil.java`:
- Around line 66-68: The PRIVATE_KEYS map currently stores CachedKey entries
indefinitely (even after PRIVATE_KEY_CACHE_TTL_MS), causing unbounded growth
across tenants; replace the ConcurrentHashMap-based cache with a bounded,
time-expiring cache (e.g., com.google.common.cache.Cache) configured with
expireAfterWrite(PRIVATE_KEY_CACHE_TTL_MS) and a sensible maximumSize, or
implement a periodic cleanup task that iterates PRIVATE_KEYS and removes entries
whose CachedKey.timestamp + PRIVATE_KEY_CACHE_TTL_MS < now; update all usages
that read/write PRIVATE_KEYS to use the new cache API and keep the CachedKey
type for stored values.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/internal/FlowExtensionServiceComponent.java`:
- Line 75: Replace the DEBUG-level lifecycle logs in
FlowExtensionServiceComponent with INFO: change the LOG.debug(...) calls in the
activate and deactivate methods (the two occurrences currently logging "In-Flow
Extension service successfully activated." and its deactivate counterpart) to
LOG.info(...) and keep the existing messages unchanged so major service state
transitions are logged at INFO level.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/management/FlowExtensionActionDTOModelResolver.java`:
- Around line 357-458: Add INFO-level logs to the certificate lifecycle methods
to improve observability: in handleCertificateAdd log when starting an add
(include action id and tenantDomain) and on success/failure; in
handleCertificateGet log before attempting the get and on success/failure; in
handleCertificateUpdate log the chosen branch (explicit removal, update
existing, add new, or carry-forward) with action ids and tenantDomain and log
success/failure for delete/update operations; in handleCertificateDelete log
before deleting and on success/failure. Use the existing logger instance (e.g.,
log or processLogger) and include the unique symbols
actionDTO.getId()/updatingActionDTO.getId()/deletingActionDTO.getId(),
certificateManagementService method names, and tenantDomain in the messages.
In
`@components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/metadata/FlowExtensionContextTreeService.java`:
- Around line 51-55: Add a guarded DEBUG entry log to the public service method
buildContextTree in FlowExtensionContextTreeService: inside buildContextTree,
before creating FlowExtensionContextTreeBuilder/ calling build(flowType), check
if (log.isDebugEnabled()) and log a lightweight message including the method
name and the flowType value to mark invocation (e.g., "Entering buildContextTree
for flowType={}" style); keep the message concise and ensure you use the
existing logger instance in the class.
🪄 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: 88d90dce-eebf-4d92-a3fd-53ac3fc8ecbe
📒 Files selected for processing (63)
.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/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/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/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/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.extension/pom.xmlcomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/FlowExtensionConstants.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/executor/FlowExtensionExecutor.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/executor/FlowExtensionRequestBuilder.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/executor/FlowExtensionResponseProcessor.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/executor/JWEEncryptionUtil.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/executor/PathTypeAnnotationUtil.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/internal/FlowExtensionDataHolder.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/internal/FlowExtensionServiceComponent.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/management/FlowExtensionActionConverter.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/management/FlowExtensionActionDTOModelResolver.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/metadata/FlowExtensionContextTreeBuilder.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/metadata/FlowExtensionContextTreeMetadata.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/metadata/FlowExtensionContextTreeNode.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/metadata/FlowExtensionContextTreeService.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/model/AccessConfig.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/model/ContextPath.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/model/Encryption.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/model/FlowContextHandoverConfig.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/model/FlowExtensionAction.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/model/FlowExtensionEvent.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/model/FlowExtensionFlow.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/model/FlowExtensionRequest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/model/OperationExecutionResult.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/util/FlowExtensionContextFilterUtil.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/util/FlowExtensionPathUtil.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/FlowExtensionTestUtils.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/executor/FlowExtensionExecutorTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/executor/FlowExtensionRequestBuilderTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/executor/FlowExtensionResponseProcessorTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/executor/PathTypeAnnotationUtilTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/metadata/FlowContextHandoverConfigTestHelper.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/metadata/FlowExtensionContextTreeBuilderTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/model/AccessConfigTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/model/FlowExtensionEventTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/model/OperationExecutionResultTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/java/org/wso2/carbon/identity/flow/extension/util/FlowExtensionPathUtilTest.javacomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/resources/repository/conf/carbon.xmlcomponents/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/test/resources/testng.xmlcomponents/flow-orchestration-framework/pom.xmlfeatures/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension.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
|
|
||
| this.id = builder.id; | ||
| this.claims.addAll(builder.claims); | ||
| this.userCredentials.putAll(builder.userCredentials); |
Contributor
There was a problem hiding this comment.
Defensively copy userCredentials to prevent mutable credential leakage.
Current putAll/unmodifiableMap handling is shallow. External code can still mutate the stored char[] values, and Builder.userCredentials(...) can NPE on null input.
🔧 Suggested fix
+import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@
- this.userCredentials.putAll(builder.userCredentials);
+ builder.userCredentials.forEach((key, value) ->
+ this.userCredentials.put(key, value != null ? Arrays.copyOf(value, value.length) : null));
@@
public Map<String, char[]> getUserCredentials() {
- return Collections.unmodifiableMap(userCredentials);
+ Map<String, char[]> copy = new HashMap<>();
+ userCredentials.forEach((key, value) ->
+ copy.put(key, value != null ? Arrays.copyOf(value, value.length) : null));
+ return Collections.unmodifiableMap(copy);
}
@@
public Builder userCredentials(Map<String, char[]> userCredentials) {
- this.userCredentials.putAll(userCredentials);
+ if (userCredentials == null) {
+ return this;
+ }
+ userCredentials.forEach((key, value) ->
+ this.userCredentials.put(key, value != null ? Arrays.copyOf(value, value.length) : 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 User class currently copies Builder.userCredentials via putAll
and wraps with Collections.unmodifiableMap but leaves the stored char[] mutable
and Builder.userCredentials(...) can NPE on null; fix by defensively copying the
map and each char[] value when setting Builder.userCredentials and when
constructing User (instead of plain putAll/unmodifiableMap), e.g., create a new
HashMap, for each entry copy the key and clone the char[] (or create a new
char[] and System.arraycopy) before putting it into the map, handle null input
in Builder.userCredentials by treating null as an empty map (or throwing a clear
IllegalArgumentException), and apply the same defensive-copy pattern in the
other affected spots (the builder setter and any other places around lines
referenced) so the internal userCredentials map and its char[] values are
immutable from external mutation.
Comment on lines
+425
to
+430
| FLOW_EXTENSION("Actions.Types.FlowExtension.Enable", | ||
| "Actions.Types.FlowExtension.ActionRequest.ExcludedHeaders.Header", | ||
| "Actions.Types.FlowExtension.ActionRequest.ExcludedParameters.Parameter", | ||
| "Actions.Types.FlowExtension.ActionRequest.AllowedHeaders.Header", | ||
| "Actions.Types.FlowExtension.ActionRequest.AllowedParameters.Parameter", | ||
| "Actions.Types.FlowExtension.Version.RetiredUpTo"); |
Contributor
There was a problem hiding this comment.
FLOW_EXTENSION request-filter config keys are defined but not applied.
ActionTypeConfig.FLOW_EXTENSION adds allowed/excluded header/parameter keys, but the readers (getExcludedHeadersInActionRequestForActionType, getExcludedParamsInActionRequestForActionType, getAllowedHeadersForActionType, getAllowedParamsForActionType) still only handle PRE_ISSUE_ACCESS_TOKEN, so FLOW_EXTENSION-specific filters never take effect.
🔧 Suggested fix
@@ public Set<String> getExcludedHeadersInActionRequestForActionType(ActionType actionType) {
switch (actionType) {
case PRE_ISSUE_ACCESS_TOKEN:
excludedHeadersPropertyValue = getPropertyValues(
ActionTypeConfig.PRE_ISSUE_ACCESS_TOKEN.getExcludedHeadersProperty());
break;
+ case FLOW_EXTENSION:
+ excludedHeadersPropertyValue = getPropertyValues(
+ ActionTypeConfig.FLOW_EXTENSION.getExcludedHeadersProperty());
+ break;
default:
break;
}
@@ public Set<String> getExcludedParamsInActionRequestForActionType(ActionType actionType) {
switch (actionType) {
case PRE_ISSUE_ACCESS_TOKEN:
excludedParamsPropertyValue = getPropertyValues(
ActionTypeConfig.PRE_ISSUE_ACCESS_TOKEN.getExcludedParamsProperty());
break;
+ case FLOW_EXTENSION:
+ excludedParamsPropertyValue = getPropertyValues(
+ ActionTypeConfig.FLOW_EXTENSION.getExcludedParamsProperty());
+ break;
default:
break;
}
@@ public Set<String> getAllowedHeadersForActionType(ActionType actionType) {
switch (actionType) {
case PRE_ISSUE_ACCESS_TOKEN:
allowedPropertyKey = ActionTypeConfig.PRE_ISSUE_ACCESS_TOKEN.getAllowedHeaderProperty();
break;
+ case FLOW_EXTENSION:
+ allowedPropertyKey = ActionTypeConfig.FLOW_EXTENSION.getAllowedHeaderProperty();
+ break;
default:
break;
}
@@ public Set<String> getAllowedParamsForActionType(ActionType actionType) {
switch (actionType) {
case PRE_ISSUE_ACCESS_TOKEN:
allowedPropertyKey = ActionTypeConfig.PRE_ISSUE_ACCESS_TOKEN.getAllowedParamsProperty();
break;
+ case FLOW_EXTENSION:
+ allowedPropertyKey = ActionTypeConfig.FLOW_EXTENSION.getAllowedParamsProperty();
+ break;
default:
break;
}🤖 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/util/ActionExecutorConfig.java`
around lines 425 - 430, ActionTypeConfig.FLOW_EXTENSION defines allowed/excluded
header/parameter config keys but the reader methods
(getExcludedHeadersInActionRequestForActionType,
getExcludedParamsInActionRequestForActionType, getAllowedHeadersForActionType,
getAllowedParamsForActionType) only handle PRE_ISSUE_ACCESS_TOKEN, so
FLOW_EXTENSION filters are ignored; update each of those reader methods to
include a branch/case for ActionTypeConfig.FLOW_EXTENSION that reads the
corresponding keys from ActionExecutorConfig.FLOW_EXTENSION (e.g., the
AllowedHeaders.Header, AllowedParameters.Parameter, ExcludedHeaders.Header,
ExcludedParameters.Parameter entries) and returns those values just like the
PRE_ISSUE_ACCESS_TOKEN branch does.
Comment on lines
+490
to
+493
| if (StringUtils.isBlank(name)) { | ||
| throw ActionManagementExceptionHandler.handleClientException( | ||
| ErrorMessage.ERROR_ACTION_NAME_BLANK); | ||
| } |
Contributor
There was a problem hiding this comment.
PATCH updates are incorrectly rejected when name is omitted for FLOW_EXTENSION.
Line 166 always invokes uniqueness validation, and Line 490 rejects blank names. That breaks the method’s PATCH contract (null/empty fields should be ignored) and can fail partial updates that do not include name.
Proposed fix
- if (StringUtils.isBlank(name)) {
- throw ActionManagementExceptionHandler.handleClientException(
- ErrorMessage.ERROR_ACTION_NAME_BLANK);
- }
+ if (StringUtils.isBlank(name)) {
+ // PATCH update path: omitted/blank fields are ignored.
+ if (excludeId != null) {
+ return;
+ }
+ throw ActionManagementExceptionHandler.handleClientException(
+ ErrorMessage.ERROR_ACTION_NAME_BLANK);
+ }🤖 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 - 493, The method in ActionManagementServiceImpl is rejecting
blank/missing name unconditionally (throwing
ErrorMessage.ERROR_ACTION_NAME_BLANK) which breaks PATCH semantics; change the
logic so blank or null name is only rejected for create/full-update flows, not
for PATCH/partial updates—i.e., guard the current blank-name check and the
uniqueness validation (the check invoked around the symbol referenced at line
~166) so they run only when the request explicitly supplies a non-blank name (or
when the operation is not a PATCH), and skip both validations when the name is
omitted in a PATCH. Ensure you reference the same method in
ActionManagementServiceImpl where the name and uniqueness checks are performed
and only validate uniqueness when name != null/blank.
Comment on lines
+56
to
+79
| @Activate | ||
| protected void activate(ComponentContext context) { | ||
|
|
||
| try { | ||
| BundleContext bundleContext = context.getBundleContext(); | ||
|
|
||
| bundleContext.registerService(Executor.class.getName(), new FlowExtensionExecutor(), null); | ||
| bundleContext.registerService(ActionExecutionRequestBuilder.class.getName(), | ||
| new FlowExtensionRequestBuilder(), null); | ||
| bundleContext.registerService(ActionExecutionResponseProcessor.class.getName(), | ||
| new FlowExtensionResponseProcessor(), null); | ||
|
|
||
| bundleContext.registerService(ActionConverter.class.getName(), | ||
| new FlowExtensionActionConverter(), null); | ||
| bundleContext.registerService(ActionDTOModelResolver.class.getName(), | ||
| new FlowExtensionActionDTOModelResolver( | ||
| FlowExtensionDataHolder.getInstance().getCertificateManagementService()), | ||
| null); | ||
|
|
||
| LOG.debug("In-Flow Extension service successfully activated."); | ||
| } catch (Throwable e) { | ||
| LOG.error("Error while initiating In-Flow Extension service", e); | ||
| } | ||
| } |
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.extension/src/main/java/org/wso2/carbon/identity/flow/extension/internal/FlowExtensionServiceComponent.java"
# Show around activate/deactivate and service registrations (keep output small)
rg -n "void activate\\(|`@Activate`|void deactivate\\(|`@Deactivate`|registerService\\(" "$FILE"
echo "----"
# Print likely activate/deactivate blocks with line numbers
nl -ba "$FILE" | sed -n '1,200p'Repository: wso2/carbon-identity-framework
Length of output: 754
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/internal/FlowExtensionServiceComponent.java"
# Show exact activate/deactivate blocks
sed -n '45,120p' "$FILE"
# Also sanity-check whether ServiceRegistration is used anywhere in the file
rg -n "ServiceRegistration|unregister\\(" "$FILE" || trueRepository: wso2/carbon-identity-framework
Length of output: 3248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/internal/FlowExtensionServiceComponent.java"
# Print remainder of file
sed -n '120,260p' "$FILE"
# Search for any ServiceRegistration/unregister usage in the file
rg -n "ServiceRegistration|unregister\\(" "$FILE" || trueRepository: wso2/carbon-identity-framework
Length of output: 2639
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/internal/FlowExtensionServiceComponent.java"
sed -n '1,60p' "$FILE"Repository: wso2/carbon-identity-framework
Length of output: 2996
Unregister OSGi services and fail activation cleanly in FlowExtensionServiceComponent
activate()registers multiple services viabundleContext.registerService(...)but doesn’t retainServiceRegistrationhandles, anddeactivate()neverunregister()s them—stale services can remain across lifecycle.- Activation exceptions are swallowed (only
LOG.error(...)in the catch), so partial registrations may persist; cleanup should happen and activation should fail so DS doesn’t continue with a broken component. LOG.error(..., e)logs the exception/stack trace; ERROR logs should log only the error message (no exception object) per the logging guidelines.
🤖 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/main/java/org/wso2/carbon/identity/flow/extension/internal/FlowExtensionServiceComponent.java`
around lines 56 - 79, The activate() method in FlowExtensionServiceComponent
registers several OSGi services via bundleContext.registerService(...) but does
not keep ServiceRegistration handles and does not unregister them on
deactivate(), and it swallows activation exceptions; update
FlowExtensionServiceComponent to store each ServiceRegistration (for Executor,
ActionExecutionRequestBuilder, ActionExecutionResponseProcessor,
ActionConverter, ActionDTOModelResolver) as instance fields, call unregister()
on each non-null registration in deactivate(), and in activate() ensure that if
an exception occurs you unregister any registrations already created and rethrow
a runtime/activation exception so activation fails cleanly; also change
LOG.error("Error while initiating In-Flow Extension service", e) to
LOG.error("Error while initiating In-Flow Extension service: " + e.getMessage())
(log only the message, not the exception object).
Comment on lines
+42
to
+43
| this.contextTree = contextTree != null ? Collections.unmodifiableList(contextTree) | ||
| : Collections.emptyList(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current constructor behavior uses an unmodifiable view without a defensive copy.
rg -n "unmodifiableList\\(contextTree\\)" \
components/flow-orchestration-framework/org.wso2.carbon.identity.flow.extension/src/main/java/org/wso2/carbon/identity/flow/extension/metadata/FlowExtensionContextTreeMetadata.javaRepository: wso2/carbon-identity-framework
Length of output: 168
Defensively copy contextTree before wrapping it as unmodifiable.
FlowExtensionContextTreeMetadata currently stores contextTree via Collections.unmodifiableList(contextTree), which keeps the caller’s list reference—mutations to the passed-in list can still change this instance.
💡 Proposed fix
+import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
@@
- this.contextTree = contextTree != null ? Collections.unmodifiableList(contextTree)
+ this.contextTree = contextTree != null ? Collections.unmodifiableList(new ArrayList<>(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.extension/src/main/java/org/wso2/carbon/identity/flow/extension/metadata/FlowExtensionContextTreeMetadata.java`
around lines 42 - 43, FlowExtensionContextTreeMetadata currently assigns
this.contextTree = Collections.unmodifiableList(contextTree) which keeps a
reference to the caller's list; change the constructor so it defensively copies
the input before wrapping it: if contextTree != null create a new
ArrayList<>(contextTree) and then wrap that copy with
Collections.unmodifiableList(...) (otherwise use Collections.emptyList()),
ensuring the field contextTree cannot be affected by subsequent 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 copies are missing, so this “immutable snapshot” can still be mutated externally.
Line 44 and Line 45 wrap caller-provided sets directly. If the caller mutates those sets later, this object’s state changes unexpectedly.
Proposed fix
import java.util.Collections;
+import java.util.HashSet;
import java.util.Set;
@@
this.includedAttributes = Collections.unmodifiableSet(includedAttributes);
this.includedUserAttributes = Collections.unmodifiableSet(includedUserAttributes);
this.fullUserPassthrough = 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;🤖 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/main/java/org/wso2/carbon/identity/flow/extension/model/FlowContextHandoverConfig.java`
around lines 44 - 45, The constructor of FlowContextHandoverConfig currently
wraps caller-provided sets directly into Collections.unmodifiableSet (fields
includedAttributes and includedUserAttributes), which allows external mutation
if the original sets are later changed; fix this by making defensive copies
first (e.g., new HashSet<>(includedAttributes) and new
HashSet<>(includedUserAttributes)) and then wrap those copies with
Collections.unmodifiableSet before assigning to the includedAttributes and
includedUserAttributes fields so the object truly holds an immutable snapshot.
Comment on lines
+23
to
+29
| <classes> | ||
| <class name="org.wso2.carbon.identity.flow.extension.executor.HierarchicalPrefixMatcherTest"/> | ||
| <class name="org.wso2.carbon.identity.flow.extension.executor.PathTypeAnnotationUtilTest"/> | ||
| <class name="org.wso2.carbon.identity.flow.extension.executor.FlowExtensionRequestBuilderTest"/> | ||
| <class name="org.wso2.carbon.identity.flow.extension.executor.FlowExtensionResponseProcessorTest"/> | ||
| <class name="org.wso2.carbon.identity.flow.extension.executor.FlowExtensionExecutorTest"/> | ||
| </classes> |
Contributor
There was a problem hiding this comment.
Register FlowExtensionPathUtilTest in this suite to avoid silently skipping it.
This suite is used as the execution source, but the PR also introduces org.wso2.carbon.identity.flow.extension.util.FlowExtensionPathUtilTest, which is not listed here. That test will not run.
✅ Suggested update
<classes>
<class name="org.wso2.carbon.identity.flow.extension.executor.HierarchicalPrefixMatcherTest"/>
<class name="org.wso2.carbon.identity.flow.extension.executor.PathTypeAnnotationUtilTest"/>
+ <class name="org.wso2.carbon.identity.flow.extension.util.FlowExtensionPathUtilTest"/>
<class name="org.wso2.carbon.identity.flow.extension.executor.FlowExtensionRequestBuilderTest"/>
<class name="org.wso2.carbon.identity.flow.extension.executor.FlowExtensionResponseProcessorTest"/>
<class name="org.wso2.carbon.identity.flow.extension.executor.FlowExtensionExecutorTest"/>
</classes>📝 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
| <classes> | |
| <class name="org.wso2.carbon.identity.flow.extension.executor.HierarchicalPrefixMatcherTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extension.executor.PathTypeAnnotationUtilTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extension.executor.FlowExtensionRequestBuilderTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extension.executor.FlowExtensionResponseProcessorTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extension.executor.FlowExtensionExecutorTest"/> | |
| </classes> | |
| <classes> | |
| <class name="org.wso2.carbon.identity.flow.extension.executor.HierarchicalPrefixMatcherTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extension.executor.PathTypeAnnotationUtilTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extension.util.FlowExtensionPathUtilTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extension.executor.FlowExtensionRequestBuilderTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extension.executor.FlowExtensionResponseProcessorTest"/> | |
| <class name="org.wso2.carbon.identity.flow.extension.executor.FlowExtensionExecutorTest"/> | |
| </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.extension/src/test/resources/testng.xml`
around lines 23 - 29, The test suite (testng.xml) currently lists executor tests
but omits the newly added test class
org.wso2.carbon.identity.flow.extension.util.FlowExtensionPathUtilTest, so that
test is silently skipped; update the <classes> block in testng.xml by adding an
entry for FlowExtensionPathUtilTest (class name
"org.wso2.carbon.identity.flow.extension.util.FlowExtensionPathUtilTest")
alongside the existing class entries so the test suite includes and executes
this new test.
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
3 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.
Proposed changes in this pull request
This PR introduces the executor / request / response / management layer for the Flow Extension action type. It is layered on top of #8109, which contributes the base `FLOW_EXTENSION` action type and basic action-management wiring.
Once #8109 merges to master, this PR's diff will naturally shrink to the contents listed below (the executor stack and the action-management extensions that depend on it).
What this PR adds beyond #8109
Related PRs
Resolves wso2/product-is#27771.
When should this PR be merged
After #8109 is merged. The current diff includes #8109's commits because the branch is rebased on top of #8109's branch — once #8109 lands in master, GitHub will collapse those commits from this PR's view automatically.
Follow up actions
Developer Checklist (Mandatory)
Checklist (for reviewing)
General
Functionality
Code
Tests
Security