From 434b4f5074a1133dae8ff8d8934c8eba3de0d75c Mon Sep 17 00:00:00 2001
From: Marc Goujon <netguino@gmail.com>
Date: Sat, 30 Jan 2021 20:32:10 -0800
Subject: [PATCH] Fixed bug in allowed_workflows logic (#1377)

---
 server/events/yaml/valid/global_cfg.go      |  11 +-
 server/events/yaml/valid/global_cfg_test.go | 120 +++++++++++++++++++-
 2 files changed, 125 insertions(+), 6 deletions(-)

diff --git a/server/events/yaml/valid/global_cfg.go b/server/events/yaml/valid/global_cfg.go
index b019556b5..b7305e741 100644
--- a/server/events/yaml/valid/global_cfg.go
+++ b/server/events/yaml/valid/global_cfg.go
@@ -292,12 +292,15 @@ func (g GlobalCfg) ValidateRepoCfg(rCfg RepoCfg, repoID string) error {
 		// default is always allowed
 		if p.WorkflowName != nil && len(allowedWorkflows) != 0 {
 			name := *p.WorkflowName
-			if !sliceContainsF(allowedWorkflows, name) || !allowCustomWorkflows {
-				return fmt.Errorf("workflow '%s' is not allowed for this repo", name)
+			if allowCustomWorkflows {
+				// If we allow CustomWorkflows we need to check that workflow name is defined inside repo and not global.
+				if mapContainsF(rCfg.Workflows, name) {
+					break
+				}
 			}
 
-			if allowCustomWorkflows {
-				break
+			if !sliceContainsF(allowedWorkflows, name) {
+				return fmt.Errorf("workflow '%s' is not allowed for this repo", name)
 			}
 		}
 	}
diff --git a/server/events/yaml/valid/global_cfg_test.go b/server/events/yaml/valid/global_cfg_test.go
index 61f39bdd6..040481604 100644
--- a/server/events/yaml/valid/global_cfg_test.go
+++ b/server/events/yaml/valid/global_cfg_test.go
@@ -133,7 +133,7 @@ func TestGlobalCfg_ValidateRepoCfg(t *testing.T) {
 		repoID string
 		expErr string
 	}{
-		"repo uses workflow that is defined but not allowed": {
+		"repo uses workflow that is defined server side but not allowed (with custom workflows)": {
 			gCfg: valid.GlobalCfg{
 				Repos: []valid.Repo{
 					valid.NewGlobalCfg(true, false, false).Repos[0],
@@ -161,7 +161,123 @@ func TestGlobalCfg_ValidateRepoCfg(t *testing.T) {
 			repoID: "github.com/owner/repo",
 			expErr: "workflow 'forbidden' is not allowed for this repo",
 		},
-		"repo uses workflow that is defined AND allowed": {
+		"repo uses workflow that is defined server side but not allowed (without custom workflows)": {
+			gCfg: valid.GlobalCfg{
+				Repos: []valid.Repo{
+					valid.NewGlobalCfg(true, false, false).Repos[0],
+					{
+						ID:                   "github.com/owner/repo",
+						AllowCustomWorkflows: Bool(false),
+						AllowedOverrides:     []string{"workflow"},
+						AllowedWorkflows:     []string{"allowed"},
+					},
+				},
+				Workflows: map[string]valid.Workflow{
+					"allowed":   {},
+					"forbidden": {},
+				},
+			},
+			rCfg: valid.RepoCfg{
+				Projects: []valid.Project{
+					{
+						Dir:          ".",
+						Workspace:    "default",
+						WorkflowName: String("forbidden"),
+					},
+				},
+			},
+			repoID: "github.com/owner/repo",
+			expErr: "workflow 'forbidden' is not allowed for this repo",
+		},
+		"repo uses workflow that is defined in both places with same name (without custom workflows)": {
+			gCfg: valid.GlobalCfg{
+				Repos: []valid.Repo{
+					valid.NewGlobalCfg(true, false, false).Repos[0],
+					{
+						ID:                   "github.com/owner/repo",
+						AllowCustomWorkflows: Bool(false),
+						AllowedOverrides:     []string{"workflow"},
+						AllowedWorkflows:     []string{"duplicated"},
+					},
+				},
+				Workflows: map[string]valid.Workflow{
+					"duplicated": {},
+				},
+			},
+			rCfg: valid.RepoCfg{
+				Projects: []valid.Project{
+					{
+						Dir:          ".",
+						Workspace:    "default",
+						WorkflowName: String("duplicated"),
+					},
+				},
+				Workflows: map[string]valid.Workflow{
+					"duplicated": {},
+				},
+			},
+			repoID: "github.com/owner/repo",
+			expErr: "repo config not allowed to define custom workflows: server-side config needs 'allow_custom_workflows: true'",
+		},
+		"repo uses workflow that is defined repo side, but not allowed (with custom workflows)": {
+			gCfg: valid.GlobalCfg{
+				Repos: []valid.Repo{
+					valid.NewGlobalCfg(true, false, false).Repos[0],
+					{
+						ID:                   "github.com/owner/repo",
+						AllowCustomWorkflows: Bool(true),
+						AllowedOverrides:     []string{"workflow"},
+						AllowedWorkflows:     []string{"none"},
+					},
+				},
+				Workflows: map[string]valid.Workflow{
+					"forbidden": {},
+				},
+			},
+			rCfg: valid.RepoCfg{
+				Projects: []valid.Project{
+					{
+						Dir:          ".",
+						Workspace:    "default",
+						WorkflowName: String("repodefined"),
+					},
+				},
+				Workflows: map[string]valid.Workflow{
+					"repodefined": {},
+				},
+			},
+			repoID: "github.com/owner/repo",
+			expErr: "",
+		},
+		"repo uses workflow that is defined server side and allowed (without custom workflows)": {
+			gCfg: valid.GlobalCfg{
+				Repos: []valid.Repo{
+					valid.NewGlobalCfg(true, false, false).Repos[0],
+					{
+						ID:                   "github.com/owner/repo",
+						AllowCustomWorkflows: Bool(false),
+						AllowedOverrides:     []string{"workflow"},
+						AllowedWorkflows:     []string{"allowed"},
+					},
+				},
+				Workflows: map[string]valid.Workflow{
+					"allowed":   {},
+					"forbidden": {},
+				},
+			},
+			rCfg: valid.RepoCfg{
+				Projects: []valid.Project{
+					{
+						Dir:          ".",
+						Workspace:    "default",
+						WorkflowName: String("allowed"),
+					},
+				},
+			},
+			repoID: "github.com/owner/repo",
+			expErr: "",
+		},
+		"repo uses workflow that is defined server side and allowed (with custom workflows)": {
 			gCfg: valid.GlobalCfg{
 				Repos: []valid.Repo{
 					valid.NewGlobalCfg(true, false, false).Repos[0],