From 32602edea947c33719b2ee33457e750b8306c984 Mon Sep 17 00:00:00 2001 From: sarmuru2 Date: Tue, 10 Oct 2023 17:03:33 +0530 Subject: [PATCH 1/5] ISICO-15108: javascript validation done on updating w/d --- .../service/MetadataServiceImpl.java | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java b/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java index 7326ab62df..7d172bb470 100644 --- a/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java +++ b/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java @@ -17,6 +17,9 @@ import java.util.Map; import java.util.Optional; import java.util.TreeSet; +import java.util.stream.Collectors; + +import javax.script.ScriptException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -27,9 +30,11 @@ import com.netflix.conductor.common.metadata.tasks.TaskDef; import com.netflix.conductor.common.metadata.workflow.WorkflowDef; import com.netflix.conductor.common.metadata.workflow.WorkflowDefSummary; +import com.netflix.conductor.common.metadata.workflow.WorkflowTask; import com.netflix.conductor.common.model.BulkResponse; import com.netflix.conductor.core.WorkflowContext; import com.netflix.conductor.core.config.ConductorProperties; +import com.netflix.conductor.core.events.ScriptEvaluator; import com.netflix.conductor.core.exception.NotFoundException; import com.netflix.conductor.dao.EventHandlerDAO; import com.netflix.conductor.dao.MetadataDAO; @@ -116,9 +121,42 @@ public TaskDef getTaskDef(String taskType) { */ public void updateWorkflowDef(WorkflowDef workflowDef) { workflowDef.setUpdateTime(System.currentTimeMillis()); + validateScriptExpression(workflowDef); metadataDAO.updateWorkflowDef(workflowDef); } + /** This function is used to eval condition script before saving the workflow */ + private void validateScriptExpression(WorkflowDef workflowDef) { + List tasks = + workflowDef.getTasks().stream() + .filter( + t -> + t.getType().equalsIgnoreCase("decision") + || t.getType().equalsIgnoreCase("switch")) + .collect(Collectors.toList()); + for (WorkflowTask task : tasks) { + String taskType = task.getType(); + String case0 = task.getCaseExpression(); + + Map map = task.getInputParameters(); + if (task.getType().equalsIgnoreCase("decision") + || task.getType().equalsIgnoreCase("switch")) { + try { + if (task.getCaseExpression() != null) { + Object returnValue = ScriptEvaluator.eval(task.getCaseExpression(), map); + } else if (task.getEvaluatorType().equalsIgnoreCase("javascript")) { + Object returnValue = ScriptEvaluator.eval(task.getExpression(), map); + } + } catch (ScriptException e) { + throw new IllegalArgumentException( + String.format( + "Decision task condition is not well formated: %s", + e.getMessage())); + } + } + } + } + /** * @param workflowDefList Workflow definitions to be updated. */ @@ -169,6 +207,7 @@ public List getWorkflowDefs() { public void registerWorkflowDef(WorkflowDef workflowDef) { workflowDef.setCreateTime(System.currentTimeMillis()); + validateScriptExpression(workflowDef); metadataDAO.createWorkflowDef(workflowDef); } From 33899676d5155a5d8917f4424d340410b5de6b21 Mon Sep 17 00:00:00 2001 From: sarmuru2 Date: Wed, 11 Oct 2023 12:04:14 +0530 Subject: [PATCH 2/5] ISICO-14902: NPE while checking EvaluatorType is fixed --- .../java/com/netflix/conductor/service/MetadataServiceImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java b/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java index 7d172bb470..74ef010e46 100644 --- a/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java +++ b/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java @@ -144,7 +144,7 @@ private void validateScriptExpression(WorkflowDef workflowDef) { try { if (task.getCaseExpression() != null) { Object returnValue = ScriptEvaluator.eval(task.getCaseExpression(), map); - } else if (task.getEvaluatorType().equalsIgnoreCase("javascript")) { + } else if ("javascript".equalsIgnoreCase(task.getEvaluatorType())) { Object returnValue = ScriptEvaluator.eval(task.getExpression(), map); } } catch (ScriptException e) { From 6c90644b985dc56dfb3093ee6cb8d1608a635410 Mon Sep 17 00:00:00 2001 From: sarmuru2 Date: Wed, 11 Oct 2023 12:58:19 +0530 Subject: [PATCH 3/5] ISICO-14902: unused variables removed --- .../com/netflix/conductor/service/MetadataServiceImpl.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java b/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java index 74ef010e46..886f719c8d 100644 --- a/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java +++ b/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java @@ -135,9 +135,6 @@ private void validateScriptExpression(WorkflowDef workflowDef) { || t.getType().equalsIgnoreCase("switch")) .collect(Collectors.toList()); for (WorkflowTask task : tasks) { - String taskType = task.getType(); - String case0 = task.getCaseExpression(); - Map map = task.getInputParameters(); if (task.getType().equalsIgnoreCase("decision") || task.getType().equalsIgnoreCase("switch")) { From 1c80862b2684d65fccb461407cb103b2f5d85233 Mon Sep 17 00:00:00 2001 From: sarmuru2 Date: Fri, 13 Oct 2023 13:01:59 +0530 Subject: [PATCH 4/5] ISICO-15108: javascript validation added in the constraint violation part --- .../service/MetadataServiceImpl.java | 36 ---------- .../WorkflowTaskTypeConstraint.java | 43 ++++++++++++ .../service/MetadataServiceTest.java | 69 +++++++++++++++++-- 3 files changed, 105 insertions(+), 43 deletions(-) diff --git a/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java b/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java index 886f719c8d..7326ab62df 100644 --- a/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java +++ b/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java @@ -17,9 +17,6 @@ import java.util.Map; import java.util.Optional; import java.util.TreeSet; -import java.util.stream.Collectors; - -import javax.script.ScriptException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -30,11 +27,9 @@ import com.netflix.conductor.common.metadata.tasks.TaskDef; import com.netflix.conductor.common.metadata.workflow.WorkflowDef; import com.netflix.conductor.common.metadata.workflow.WorkflowDefSummary; -import com.netflix.conductor.common.metadata.workflow.WorkflowTask; import com.netflix.conductor.common.model.BulkResponse; import com.netflix.conductor.core.WorkflowContext; import com.netflix.conductor.core.config.ConductorProperties; -import com.netflix.conductor.core.events.ScriptEvaluator; import com.netflix.conductor.core.exception.NotFoundException; import com.netflix.conductor.dao.EventHandlerDAO; import com.netflix.conductor.dao.MetadataDAO; @@ -121,39 +116,9 @@ public TaskDef getTaskDef(String taskType) { */ public void updateWorkflowDef(WorkflowDef workflowDef) { workflowDef.setUpdateTime(System.currentTimeMillis()); - validateScriptExpression(workflowDef); metadataDAO.updateWorkflowDef(workflowDef); } - /** This function is used to eval condition script before saving the workflow */ - private void validateScriptExpression(WorkflowDef workflowDef) { - List tasks = - workflowDef.getTasks().stream() - .filter( - t -> - t.getType().equalsIgnoreCase("decision") - || t.getType().equalsIgnoreCase("switch")) - .collect(Collectors.toList()); - for (WorkflowTask task : tasks) { - Map map = task.getInputParameters(); - if (task.getType().equalsIgnoreCase("decision") - || task.getType().equalsIgnoreCase("switch")) { - try { - if (task.getCaseExpression() != null) { - Object returnValue = ScriptEvaluator.eval(task.getCaseExpression(), map); - } else if ("javascript".equalsIgnoreCase(task.getEvaluatorType())) { - Object returnValue = ScriptEvaluator.eval(task.getExpression(), map); - } - } catch (ScriptException e) { - throw new IllegalArgumentException( - String.format( - "Decision task condition is not well formated: %s", - e.getMessage())); - } - } - } - } - /** * @param workflowDefList Workflow definitions to be updated. */ @@ -204,7 +169,6 @@ public List getWorkflowDefs() { public void registerWorkflowDef(WorkflowDef workflowDef) { workflowDef.setCreateTime(System.currentTimeMillis()); - validateScriptExpression(workflowDef); metadataDAO.createWorkflowDef(workflowDef); } diff --git a/core/src/main/java/com/netflix/conductor/validations/WorkflowTaskTypeConstraint.java b/core/src/main/java/com/netflix/conductor/validations/WorkflowTaskTypeConstraint.java index 888dd7a49e..f0eed4f61b 100644 --- a/core/src/main/java/com/netflix/conductor/validations/WorkflowTaskTypeConstraint.java +++ b/core/src/main/java/com/netflix/conductor/validations/WorkflowTaskTypeConstraint.java @@ -18,8 +18,10 @@ import java.lang.annotation.Target; import java.text.ParseException; import java.time.format.DateTimeParseException; +import java.util.Map; import java.util.Optional; +import javax.script.ScriptException; import javax.validation.Constraint; import javax.validation.ConstraintValidator; import javax.validation.ConstraintValidatorContext; @@ -30,6 +32,7 @@ import com.netflix.conductor.common.metadata.tasks.TaskDef; import com.netflix.conductor.common.metadata.tasks.TaskType; import com.netflix.conductor.common.metadata.workflow.WorkflowTask; +import com.netflix.conductor.core.events.ScriptEvaluator; import com.netflix.conductor.core.utils.DateTimeUtils; import static com.netflix.conductor.core.execution.tasks.Terminate.getTerminationStatusParameter; @@ -166,9 +169,34 @@ private boolean isDecisionTaskValid( context.buildConstraintViolationWithTemplate(message).addConstraintViolation(); valid = false; } + + if (workflowTask.getCaseExpression() != null) { + try { + validateScriptExpression( + workflowTask.getCaseExpression(), workflowTask.getInputParameters()); + } catch (Exception ee) { + String message = + String.format( + ee.getMessage() + ", taskType: DECISION taskName %s", + workflowTask.getName()); + context.buildConstraintViolationWithTemplate(message).addConstraintViolation(); + valid = false; + } + } + return valid; } + private void validateScriptExpression( + String expression, Map inputParameters) { + try { + Object returnValue = ScriptEvaluator.eval(expression, inputParameters); + } catch (ScriptException e) { + throw new IllegalArgumentException( + String.format("Expression is not well formatted: %s", e.getMessage())); + } + } + private boolean isSwitchTaskValid( WorkflowTask workflowTask, ConstraintValidatorContext context) { boolean valid = true; @@ -209,6 +237,21 @@ private boolean isSwitchTaskValid( context.buildConstraintViolationWithTemplate(message).addConstraintViolation(); valid = false; } + + if ("javascript".equals(workflowTask.getEvaluatorType()) + && workflowTask.getExpression() != null) { + try { + validateScriptExpression( + workflowTask.getCaseExpression(), workflowTask.getInputParameters()); + } catch (Exception ee) { + String message = + String.format( + ee.getMessage() + ", taskType: SWITCH taskName %s", + workflowTask.getName()); + context.buildConstraintViolationWithTemplate(message).addConstraintViolation(); + valid = false; + } + } return valid; } diff --git a/core/src/test/java/com/netflix/conductor/service/MetadataServiceTest.java b/core/src/test/java/com/netflix/conductor/service/MetadataServiceTest.java index 8ea351ac78..87fd9054e5 100644 --- a/core/src/test/java/com/netflix/conductor/service/MetadataServiceTest.java +++ b/core/src/test/java/com/netflix/conductor/service/MetadataServiceTest.java @@ -12,13 +12,7 @@ */ package com.netflix.conductor.service; -import java.util.ArrayList; -import java.util.Collections; -import java.util.Date; -import java.util.Iterator; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; import javax.validation.ConstraintViolationException; @@ -35,6 +29,7 @@ import com.netflix.conductor.common.metadata.workflow.WorkflowDef; import com.netflix.conductor.common.metadata.workflow.WorkflowDefSummary; import com.netflix.conductor.common.metadata.workflow.WorkflowTask; +import com.netflix.conductor.common.model.BulkResponse; import com.netflix.conductor.core.config.ConductorProperties; import com.netflix.conductor.core.exception.NotFoundException; import com.netflix.conductor.dao.EventHandlerDAO; @@ -282,6 +277,66 @@ public void testUpdateWorkflowDef() { verify(metadataDAO, times(1)).updateWorkflowDef(workflowDef); } + @Test(expected = ConstraintViolationException.class) + public void testUpdateWorkflowDefWithCaseExpression() { + WorkflowDef workflowDef = new WorkflowDef(); + workflowDef.setName("somename"); + workflowDef.setOwnerEmail("sample@test.com"); + List tasks = new ArrayList<>(); + WorkflowTask workflowTask = new WorkflowTask(); + workflowTask.setTaskReferenceName("hello"); + workflowTask.setName("hello"); + workflowTask.setType("DECISION"); + + WorkflowTask caseTask = new WorkflowTask(); + caseTask.setTaskReferenceName("casetrue"); + caseTask.setName("casetrue"); + + List caseTaskList = new ArrayList<>(); + caseTaskList.add(caseTask); + + Map> decisionCases = new HashMap(); + decisionCases.put("true", caseTaskList); + + workflowTask.setDecisionCases(decisionCases); + workflowTask.setCaseExpression("1 >0abcd"); + tasks.add(workflowTask); + workflowDef.setTasks(tasks); + when(metadataDAO.getTaskDef(any())).thenReturn(new TaskDef()); + BulkResponse bulkResponse = + metadataService.updateWorkflowDef(Collections.singletonList(workflowDef)); + } + + @Test(expected = ConstraintViolationException.class) + public void testUpdateWorkflowDefWithJavscriptEvaluator() { + WorkflowDef workflowDef = new WorkflowDef(); + workflowDef.setName("somename"); + workflowDef.setOwnerEmail("sample@test.com"); + List tasks = new ArrayList<>(); + WorkflowTask workflowTask = new WorkflowTask(); + workflowTask.setTaskReferenceName("hello"); + workflowTask.setName("hello"); + workflowTask.setType("SWITCH"); + workflowTask.setEvaluatorType("javascript"); + workflowTask.setExpression("1>abcd"); + WorkflowTask caseTask = new WorkflowTask(); + caseTask.setTaskReferenceName("casetrue"); + caseTask.setName("casetrue"); + + List caseTaskList = new ArrayList<>(); + caseTaskList.add(caseTask); + + Map> decisionCases = new HashMap(); + decisionCases.put("true", caseTaskList); + + workflowTask.setDecisionCases(decisionCases); + tasks.add(workflowTask); + workflowDef.setTasks(tasks); + when(metadataDAO.getTaskDef(any())).thenReturn(new TaskDef()); + BulkResponse bulkResponse = + metadataService.updateWorkflowDef(Collections.singletonList(workflowDef)); + } + @Test(expected = ConstraintViolationException.class) public void testRegisterWorkflowDefNoName() { try { From 6df9a8284242813af3dbe5755102f00176da14f7 Mon Sep 17 00:00:00 2001 From: sarmuru2 Date: Fri, 13 Oct 2023 14:09:07 +0530 Subject: [PATCH 5/5] ISICO-15108: getExpression() is used for switch javascript code --- .../conductor/validations/WorkflowTaskTypeConstraint.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/com/netflix/conductor/validations/WorkflowTaskTypeConstraint.java b/core/src/main/java/com/netflix/conductor/validations/WorkflowTaskTypeConstraint.java index f0eed4f61b..99d0709853 100644 --- a/core/src/main/java/com/netflix/conductor/validations/WorkflowTaskTypeConstraint.java +++ b/core/src/main/java/com/netflix/conductor/validations/WorkflowTaskTypeConstraint.java @@ -242,7 +242,7 @@ private boolean isSwitchTaskValid( && workflowTask.getExpression() != null) { try { validateScriptExpression( - workflowTask.getCaseExpression(), workflowTask.getInputParameters()); + workflowTask.getExpression(), workflowTask.getInputParameters()); } catch (Exception ee) { String message = String.format(