fix(api): resolution edition wasn't validating all the fields (including step state) causing internal issues (#250)

Signed-off-by: Romain Beuque <556072+rbeuque74@users.noreply.github.com>

Author: rbeuque74

api/handler/resolution.go

@@ -255,6 +255,30 @@ func UpdateResolution(c *gin.Context, in *updateResolutionIn) error {
 
 	if in.Steps != nil {
 		r.Steps = in.Steps
+
+		tt, err := tasktemplate.LoadFromID(dbp, t.TemplateID)
+		if err != nil {
+			_ = dbp.Rollback()
+			return err
+		}
+
+		// valid and normalize steps
+		for name, st := range r.Steps {
+			if err := st.ValidAndNormalize(name, tt.BaseConfigurations, r.Steps); err != nil {
+				return errors.NewNotValid(err, fmt.Sprintf("invalid step %s", name))
+			}
+
+			valid, err := st.CheckIfValidState()
+			if err != nil {
+				_ = dbp.Rollback()
+				return err
+			}
+
+			if !valid {
+				_ = dbp.Rollback()
+				return errors.NewBadRequest(nil, fmt.Sprintf("step %q: invalid state provided: %q is not allowed", name, st.State))
+			}
+		}
 	}
 
 	if in.ResolverInputs != nil {
@@ -743,6 +767,17 @@ func UpdateResolutionStep(c *gin.Context, in *updateResolutionStepIn) error {
 		return err
 	}
 
+	valid, err := r.Steps[in.StepName].CheckIfValidState()
+	if err != nil {
+		_ = dbp.Rollback()
+		return err
+	}
+
+	if !valid {
+		_ = dbp.Rollback()
+		return errors.NewBadRequest(nil, fmt.Sprintf("invalid state provided: %q is not allowed", in.State))
+	}
+
 	logrus.WithFields(logrus.Fields{"resolution_id": r.PublicID}).Debugf("Handler UpdateResolutionStep: manual update of resolution %s step %s", r.PublicID, in.StepName)
 
 	if err := r.Update(dbp); err != nil {

engine/step/step.go

@@ -561,7 +561,7 @@ func (st *Step) ValidAndNormalize(name string, baseConfigs map[string]json.RawMe
 	}
 	preHook, err := st.GetPreHook()
 	if err != nil {
-		return errors.NewNotValid(err, "Invalid  prehook action")
+		return errors.NewNotValid(err, "Invalid prehook action")
 	}
 	if preHook != nil {
 		ph, err := validExecutor(baseConfigs, *preHook, nil)
@@ -573,52 +573,6 @@ func (st *Step) ValidAndNormalize(name string, baseConfigs map[string]json.RawMe
 		}
 	}
 
-	// check that we don't set restricted field from the template
-	if st.State != "" {
-		return errors.NewNotValid(nil, "step state must not be set")
-	}
-
-	if st.ChildrenSteps != nil {
-		return errors.NewNotValid(nil, "step children_steps must not be set")
-	}
-
-	if st.ChildrenStepMap != nil {
-		return errors.NewNotValid(nil, "step children_steps_map must not be set")
-	}
-
-	if st.Output != nil {
-		return errors.NewNotValid(nil, "step output must not be set")
-	}
-
-	if st.Metadata != nil {
-		return errors.NewNotValid(nil, "step metadatas must not be set")
-	}
-
-	if st.Tags != nil {
-		return errors.NewNotValid(nil, "step tags must not be set")
-	}
-
-	if st.Children != nil {
-		return errors.NewNotValid(nil, "step children must not be set")
-	}
-
-	if st.Error != "" {
-		return errors.NewNotValid(nil, "step error must not be set")
-	}
-
-	if st.TryCount != 0 {
-		return errors.NewNotValid(nil, "step try_count must not be set")
-	}
-
-	t := time.Time{}
-	if st.LastRun != t {
-		return errors.NewNotValid(nil, "step last_time must not be set")
-	}
-
-	if st.Item != nil {
-		return errors.NewNotValid(nil, "step item must not be set")
-	}
-
 	if st.ForEachStrategy != "" && st.ForEach == "" {
 		return errors.NewNotValid(nil, "step foreach_strategy can't be set without foreach")
 	}
@@ -711,6 +665,58 @@ func (st *Step) ValidAndNormalize(name string, baseConfigs map[string]json.RawMe
 	return nil
 }
 
+// ValidAndNormalizeNewStep will validate that a given step doesn't have extragenous fields defined
+// when coming from a task_template.
+func (st *Step) ValidAndNormalizeNewStep() error {
+	// check that we don't set restricted field from the template
+	if st.State != "" {
+		return errors.NewNotValid(nil, "step state must not be set")
+	}
+
+	if st.ChildrenSteps != nil {
+		return errors.NewNotValid(nil, "step children_steps must not be set")
+	}
+
+	if st.ChildrenStepMap != nil {
+		return errors.NewNotValid(nil, "step children_steps_map must not be set")
+	}
+
+	if st.Output != nil {
+		return errors.NewNotValid(nil, "step output must not be set")
+	}
+
+	if st.Metadata != nil {
+		return errors.NewNotValid(nil, "step metadatas must not be set")
+	}
+
+	if st.Tags != nil {
+		return errors.NewNotValid(nil, "step tags must not be set")
+	}
+
+	if st.Children != nil {
+		return errors.NewNotValid(nil, "step children must not be set")
+	}
+
+	if st.Error != "" {
+		return errors.NewNotValid(nil, "step error must not be set")
+	}
+
+	if st.TryCount != 0 {
+		return errors.NewNotValid(nil, "step try_count must not be set")
+	}
+
+	t := time.Time{}
+	if st.LastRun != t {
+		return errors.NewNotValid(nil, "step last_time must not be set")
+	}
+
+	if st.Item != nil {
+		return errors.NewNotValid(nil, "step item must not be set")
+	}
+
+	return nil
+}
+
 // IsRunnable asserts that Step is in a runnable state
 func (st *Step) IsRunnable() bool {
 	return utils.ListContainsString(runnableStates, st.State)

models/tasktemplate/template.go

@@ -345,6 +345,10 @@ func (tt *TaskTemplate) Valid() (err error) {
 		if err := st.ValidAndNormalize(name, tt.BaseConfigurations, tt.Steps); err != nil {
 			return errors.NewNotValid(err, fmt.Sprintf("Invalid step %s", name))
 		}
+
+		if err := st.ValidAndNormalizeNewStep(); err != nil {
+			return errors.NewNotValid(err, fmt.Sprintf("Invalid step %s", name))
+		}
 	}
 
 	// MarshalIndent as it's easier to read line by line