fix(api): return error when a workflow contains default payload on no…

…n root node (#3688) * fix(api): return error when a workflow contains default payload on non root node Signed-off-by: Benjamin Coenen <benjamin.coenen@corp.ovh.com> * fix(api): return error when a workflow contains default payload on non root node Signed-off-by: Benjamin Coenen <benjamin.coenen@corp.ovh.com> * fix(api): return error when a workflow contains default payload on non root node Signed-off-by: Benjamin Coenen <benjamin.coenen@corp.ovh.com>
ovh · Dec 5, 2018 · 184072c · 184072c
1 parent fe8ee44
commit 184072c
Show file tree

Hide file tree

Showing 7 changed files with 116 additions and 19 deletions.
diff --git a/engine/api/workflow/dao_node.go b/engine/api/workflow/dao_node.go
@@ -92,6 +92,10 @@ func insertNode(db gorp.SqlExecutor, store cache.Store, w *sdk.Workflow, n *sdk.
 		n.Context.DefaultPipelineParameters = defaultPipParams
 	}
 
+	if n.Name != w.WorkflowData.Node.Name && n.Context.DefaultPayload != nil {
+		return sdk.ErrInvalidNodeDefaultPayload
+	}
+
 	if n.ID == 0 {
 		//Insert new node
 		dbwn := Node(*n)

diff --git a/engine/api/workflow_test.go b/engine/api/workflow_test.go
@@ -376,6 +376,63 @@ func Test_putWorkflowHandler(t *testing.T) {
 
 	assert.NotEmpty(t, payload["git.branch"], "git.branch should not be empty")
 }
+func Test_postWorkflowHandlerWithError(t *testing.T) {
+	// Init database
+	api, db, router, end := newTestAPI(t)
+	defer end()
+
+	// Init user
+	u, pass := assets.InsertAdminUser(api.mustDB())
+	// Init project
+	key := sdk.RandomString(10)
+	proj := assets.InsertTestProject(t, db, api.Cache, key, key, u)
+
+	// Init pipeline
+	pip := sdk.Pipeline{
+		Name:      "pipeline1",
+		ProjectID: proj.ID,
+		Type:      sdk.BuildPipeline,
+	}
+	test.NoError(t, pipeline.InsertPipeline(api.mustDB(), api.Cache, proj, &pip, nil))
+
+	//Prepare request
+	vars := map[string]string{
+		"permProjectKey": proj.Key,
+	}
+	uri := router.GetRoute("POST", api.postWorkflowHandler, vars)
+	test.NotEmpty(t, uri)
+
+	var workflow = &sdk.Workflow{
+		Name:        "Name",
+		Description: "Description",
+		WorkflowData: &sdk.WorkflowData{
+			Node: sdk.Node{
+				Type: sdk.NodeTypePipeline,
+				Context: &sdk.NodeContext{
+					PipelineID: pip.ID,
+				},
+				Triggers: []sdk.NodeTrigger{{
+					ChildNode: sdk.Node{
+						Type: sdk.NodeTypePipeline,
+						Context: &sdk.NodeContext{
+							PipelineID: pip.ID,
+							DefaultPayload: map[string]interface{}{
+								"test": "content",
+							},
+						},
+					},
+				}},
+			},
+		},
+	}
+
+	req := assets.NewAuthentifiedRequest(t, u, pass, "POST", uri, &workflow)
+
+	//Do the request
+	w := httptest.NewRecorder()
+	router.Mux.ServeHTTP(w, req)
+	assert.Equal(t, 400, w.Code)
+}
 
 func Test_postWorkflowRollbackHandler(t *testing.T) {
 	// Init database

diff --git a/sdk/error.go b/sdk/error.go
@@ -167,6 +167,7 @@ var (
 	ErrInvalidGroupAdmin                      = Error{ID: 150, Status: http.StatusForbidden}
 	ErrWorkflowNotGenerated                   = Error{ID: 151, Status: http.StatusForbidden}
 	ErrAlreadyLatestTemplate                  = Error{ID: 152, Status: http.StatusForbidden}
+	ErrInvalidNodeDefaultPayload              = Error{ID: 153, Status: http.StatusBadRequest}
 )
 
 var errorsAmericanEnglish = map[int]string{
@@ -316,6 +317,7 @@ var errorsAmericanEnglish = map[int]string{
 	ErrInvalidData.ID:                            "Cannot validate given data",
 	ErrInvalidGroupAdmin.ID:                      "User is not a group's admin",
 	ErrWorkflowNotGenerated.ID:                   "Workflow was not generated by a template",
+	ErrInvalidNodeDefaultPayload.ID:              "Workflow node which isn't a root node cannot have a default payload",
 }
 
 var errorsFrench = map[int]string{
@@ -465,6 +467,7 @@ var errorsFrench = map[int]string{
 	ErrInvalidData.ID:                            "Impossible de valider les données",
 	ErrInvalidGroupAdmin.ID:                      "L'utilisateur n'est pas administrateur du groupe",
 	ErrWorkflowNotGenerated.ID:                   "Le workflow n'a pas été généré par un template",
+	ErrInvalidNodeDefaultPayload.ID:              "Le workflow est incorrect. Un payload par défaut ne peut pas être sur un pipeline autre que le premier du workflow",
 }
 
 var errorsLanguages = []map[int]string{

diff --git a/sdk/exportentities/workflow.go b/sdk/exportentities/workflow.go
@@ -529,6 +529,9 @@ func (e *NodeEntry) getNode(name string, w *sdk.Workflow) (*sdk.Node, error) {
 	}
 
 	if len(e.Payload) > 0 {
+		if len(e.DependsOn) > 0 {
+			return nil, fmt.Errorf("Default payload cannot be set on another node than the first one")
+		}
 		node.Context.DefaultPayload = e.Payload
 	}
 

diff --git a/sdk/exportentities/workflow_test.go b/sdk/exportentities/workflow_test.go
@@ -314,6 +314,26 @@ func TestWorkflow_GetWorkflow(t *testing.T) {
 			},
 		},
 		// root(pipeline-root) -> child(pipeline-child)
+		{
+			name: "Complexe workflow without joins with a default payload on a non root node should raise an error",
+			fields: fields{
+				Workflow: map[string]NodeEntry{
+					"root": NodeEntry{
+						PipelineName: "pipeline-root",
+					},
+					"child": NodeEntry{
+						PipelineName: "pipeline-child",
+						DependsOn:    []string{"root"},
+						Payload: map[string]interface{}{
+							"test": "content",
+						},
+						OneAtATime: &True,
+					},
+				},
+			},
+			wantErr: true,
+		},
+		// root(pipeline-root) -> child(pipeline-child)
 		{
 			name: "Complexe workflow unordered without joins should not raise an error",
 			fields: fields{
@@ -679,6 +699,11 @@ func TestWorkflow_GetWorkflow(t *testing.T) {
 				return
 			}
 
+			if tt.wantErr {
+				assert.Error(t, err)
+				return
+			}
+
 			got.HookModels = nil
 			got.OutGoingHookModels = nil
 			got.Applications = nil

diff --git a/ui/src/app/app.component.ts b/ui/src/app/app.component.ts
@@ -140,14 +140,17 @@ export class AppComponent  implements OnInit {
         }
         let authKey: string;
         let authValue: string;
+        let user = this._authStore.getUser();
         // ADD user AUTH
         let sessionToken = this._authStore.getSessionToken();
         if (sessionToken) {
             authKey = this._authStore.localStorageSessionKey;
             authValue = sessionToken;
-        } else {
+        } else if (user) {
             authKey = 'Authorization';
-            authValue = 'Basic ' + this._authStore.getUser().token;
+            authValue = 'Basic ' + user.token;
+        } else {
+            return;
         }
 
         if (window['SharedWorker']) {

diff --git a/ui/src/app/shared/workflow/modal/trigger/workflow.trigger.component.ts b/ui/src/app/shared/workflow/modal/trigger/workflow.trigger.component.ts
@@ -1,15 +1,12 @@
-import {Component, EventEmitter, Input, Output, ViewChild} from '@angular/core';
-import {cloneDeep} from 'lodash';
-import {ModalTemplate, SuiModalService, TemplateModalConfig} from 'ng2-semantic-ui';
-import {ActiveModal} from 'ng2-semantic-ui/dist';
-import {PipelineStatus} from '../../../../model/pipeline.model';
-import {Project} from '../../../../model/project.model';
-import {
-    WNode, WNodeTrigger,
-    Workflow, WorkflowNodeCondition, WorkflowNodeConditions
-} from '../../../../model/workflow.model';
-import {WorkflowNodeOutGoingHookFormComponent} from '../../node/outgoinghook-form/outgoinghook.form.component';
-import {WorkflowNodeAddWizardComponent} from '../../node/wizard/node.wizard.component';
+import { Component, EventEmitter, Input, Output, ViewChild } from '@angular/core';
+import { cloneDeep } from 'lodash';
+import { ModalTemplate, SuiModalService, TemplateModalConfig } from 'ng2-semantic-ui';
+import { ActiveModal } from 'ng2-semantic-ui/dist';
+import { PipelineStatus } from '../../../../model/pipeline.model';
+import { Project } from '../../../../model/project.model';
+import { WNode, WNodeTrigger, Workflow, WorkflowNodeCondition, WorkflowNodeConditions } from '../../../../model/workflow.model';
+import { WorkflowNodeOutGoingHookFormComponent } from '../../node/outgoinghook-form/outgoinghook.form.component';
+import { WorkflowNodeAddWizardComponent } from '../../node/wizard/node.wizard.component';
 
 @Component({
     selector: 'app-workflow-trigger',
@@ -75,20 +72,25 @@ export class WorkflowTriggerComponent {
 
         let clonedWorkflow = cloneDeep(this.workflow);
         if (this.source && !this.isParent) {
-            let n = Workflow.getNodeByID(this.source.id, clonedWorkflow);
-            if (!n.triggers) {
-                n.triggers = new Array<WNodeTrigger>();
+            let sourceNode = Workflow.getNodeByID(this.source.id, clonedWorkflow);
+            if (!sourceNode.triggers) {
+                sourceNode.triggers = new Array<WNodeTrigger>();
             }
             let newTrigger = new WNodeTrigger();
-            newTrigger.parent_node_name = n.ref;
+            let previousDefaultPayload = this.destNode.context.default_payload;
+            this.destNode.context.default_payload = null;
+            newTrigger.parent_node_name = sourceNode.ref;
             newTrigger.child_node = this.destNode;
-            n.triggers.push(newTrigger);
+            sourceNode.context.default_payload = previousDefaultPayload;
+            sourceNode.triggers.push(newTrigger);
             this.triggerEvent.emit(clonedWorkflow);
         } else if (this.isParent) {
             this.destNode.triggers = new Array<WNodeTrigger>();
             let newTrigger = new WNodeTrigger();
             newTrigger.child_node = clonedWorkflow.workflow_data.node;
             this.destNode.triggers.push(newTrigger);
+            this.destNode.context.default_payload = newTrigger.child_node.context.default_payload;
+            newTrigger.child_node.context.default_payload = null;
             this.destNode.hooks = cloneDeep(clonedWorkflow.workflow_data.node.hooks);
             clonedWorkflow.workflow_data.node.hooks = [];
             clonedWorkflow.workflow_data.node = this.destNode;