fix(api): check if workflow exists found before permission (#5084)

richardlt · web-flow · commit b289f6b96a95 · 2020-03-27T08:13:52.000+01:00
diff --git a/cli/cdsctl/workflow_delete.go b/cli/cdsctl/workflow_delete.go
@@ -19,7 +19,7 @@ var workflowDeleteCmd = cli.Command{
 
 func workflowDeleteRun(v cli.Values) error {
 	err := client.WorkflowDelete(v.GetString(_ProjectKey), v.GetString(_WorkflowName))
-	if err != nil && v.GetBool("force") && sdk.ErrorIs(err, sdk.ErrWorkflowNotFound) {
+	if err != nil && v.GetBool("force") && sdk.ErrorIs(err, sdk.ErrNotFound) {
 		fmt.Println(err.Error())
 		os.Exit(0)
 	}
diff --git a/engine/api/pipeline_test.go b/engine/api/pipeline_test.go
@@ -10,10 +10,9 @@ import (
 	"testing"
 	"time"
 
-	"github.com/ovh/cds/engine/api/application"
-
 	"github.com/stretchr/testify/assert"
 
+	"github.com/ovh/cds/engine/api/application"
 	"github.com/ovh/cds/engine/api/pipeline"
 	"github.com/ovh/cds/engine/api/repositoriesmanager"
 	"github.com/ovh/cds/engine/api/services"
@@ -104,6 +103,7 @@ func TestUpdateAsCodePipelineHandler(t *testing.T) {
 			"secret": "bar",
 		},
 	}))
+	wkf := assets.InsertTestWorkflow(t, db, api.Cache, proj, sdk.RandomString(10))
 
 	pip := sdk.Pipeline{
 		Name:           sdk.RandomString(10),
@@ -154,7 +154,7 @@ func TestUpdateAsCodePipelineHandler(t *testing.T) {
 		// Get operation
 		uriGET := api.Router.GetRoute("GET", api.getWorkflowAsCodeHandler, map[string]string{
 			"key":              proj.Key,
-			"permWorkflowName": pip.Name,
+			"permWorkflowName": wkf.Name,
 			"uuid":             myOpe.UUID,
 		})
 		reqGET, err := http.NewRequest("GET", uriGET, nil)
@@ -176,5 +176,4 @@ func TestUpdateAsCodePipelineHandler(t *testing.T) {
 		assert.Equal(t, "myURL", myOpeGet.Setup.Push.PRLink)
 		break
 	}
-
 }
diff --git a/engine/api/router_middleware_auth_permission.go b/engine/api/router_middleware_auth_permission.go
@@ -153,13 +153,21 @@ func (api *API) checkWorkflowPermissions(ctx context.Context, workflowName strin
 		projectKey, has = routeVars["key"]
 	}
 	if !has {
-		return sdk.WrapError(sdk.ErrForbidden, "not authorized for workflow %s, missing project key value", workflowName)
+		return sdk.WithStack(sdk.ErrNotFound)
 	}
 
 	if workflowName == "" {
 		return sdk.WrapError(sdk.ErrWrongRequest, "invalid given workflow name")
 	}
 
+	exists, err := workflow.Exists(api.mustDB(), projectKey, workflowName)
+	if err != nil {
+		return err
+	}
+	if !exists {
+		return sdk.WithStack(sdk.ErrNotFound)
+	}
+
 	perms, err := permission.LoadWorkflowMaxLevelPermission(ctx, api.mustDB(), projectKey, []string{workflowName}, getAPIConsumer(ctx).GetGroupIDs())
 	if err != nil {
 		return sdk.NewError(sdk.ErrForbidden, err)
diff --git a/engine/api/templates.go b/engine/api/templates.go
@@ -690,7 +690,7 @@ func (api *API) getTemplateInstanceHandler() service.Handler {
 
 		wf, err := workflow.Load(ctx, api.mustDB(), api.Cache, *proj, workflowName, workflow.LoadOptions{})
 		if err != nil {
-			if sdk.ErrorIs(err, sdk.ErrWorkflowNotFound) {
+			if sdk.ErrorIs(err, sdk.ErrNotFound) {
 				return sdk.NewErrorFrom(sdk.ErrNotFound, "cannot load workflow %s", workflowName)
 			}
 			return sdk.WithStack(err)
diff --git a/engine/api/workflow.go b/engine/api/workflow.go
@@ -577,7 +577,7 @@ func (api *API) deleteWorkflowHandler() service.Handler {
 			return sdk.WrapError(errW, "Cannot check Workflow %s", key)
 		}
 		if !b {
-			return sdk.WithStack(sdk.ErrWorkflowNotFound)
+			return sdk.WithStack(sdk.ErrNotFound)
 		}
 
 		tx, errT := api.mustDB().Begin()
@@ -713,7 +713,7 @@ func (api *API) getWorkflowNotificationsConditionsHandler() service.Handler {
 
 		wr, errr := workflow.LoadLastRun(api.mustDB(), key, name, workflow.LoadRunOptions{})
 		if errr != nil {
-			if !sdk.ErrorIs(errr, sdk.ErrWorkflowNotFound) {
+			if !sdk.ErrorIs(errr, sdk.ErrNotFound) {
 				return sdk.WrapError(errr, "getWorkflowTriggerConditionHandler> Unable to load last run workflow")
 			}
 		}
diff --git a/engine/api/workflow/dao.go b/engine/api/workflow/dao.go
@@ -263,7 +263,7 @@ func LoadAll(db gorp.SqlExecutor, projectKey string) (sdk.Workflows, error) {
 
 	if _, err := db.Select(&dbRes, query, projectKey); err != nil {
 		if err == sql.ErrNoRows {
-			return nil, sdk.ErrWorkflowNotFound
+			return nil, sdk.WithStack(sdk.ErrNotFound)
 		}
 		return nil, sdk.WrapError(err, "Unable to load workflows project %s", projectKey)
 	}
@@ -511,7 +511,7 @@ func load(ctx context.Context, db gorp.SqlExecutor, proj sdk.Project, opts LoadO
 	next()
 	if err != nil {
 		if err == sql.ErrNoRows {
-			return nil, sdk.ErrWorkflowNotFound
+			return nil, sdk.WithStack(sdk.ErrNotFound)
 		}
 		return nil, sdk.WrapError(err, "Unable to load workflow")
 	}
diff --git a/engine/api/workflow/dao_run.go b/engine/api/workflow/dao_run.go
@@ -439,13 +439,13 @@ func loadRun(db gorp.SqlExecutor, loadOpts LoadRunOptions, query string, args ..
 	runDB := &Run{}
 	if err := db.SelectOne(runDB, query, args...); err != nil {
 		if err == sql.ErrNoRows {
-			return nil, sdk.ErrWorkflowNotFound
+			return nil, sdk.WithStack(sdk.ErrNotFound)
 		}
 		return nil, sdk.WrapError(err, "Unable to load workflow run. query:%s args:%v", query, args)
 	}
 	wr := sdk.WorkflowRun(*runDB)
 	if !loadOpts.WithDeleted && wr.ToDelete {
-		return nil, sdk.WithStack(sdk.ErrWorkflowNotFound)
+		return nil, sdk.WithStack(sdk.ErrNotFound)
 	}
 
 	tags, errT := loadTagsByRunID(db, wr.ID)
diff --git a/engine/api/workflow_run_test.go b/engine/api/workflow_run_test.go
@@ -2522,14 +2522,16 @@ func Test_deleteWorkflowRunHandler(t *testing.T) {
 func Test_postWorkflowRunHandlerBadResyncOptions(t *testing.T) {
 	api, db, router, end := newTestAPI(t)
 	defer end()
-	u, pass := assets.InsertAdminUser(t, api.mustDB())
+
 	key := sdk.RandomString(10)
 	proj := assets.InsertTestProject(t, db, api.Cache, key, key)
+	w := assets.InsertTestWorkflow(t, db, api.Cache, proj, sdk.RandomString(10))
+	u, pass := assets.InsertLambdaUser(t, api.mustDB(), &proj.ProjectGroups[0].Group)
 
 	//Prepare request
 	vars := map[string]string{
 		"key":              proj.Key,
-		"permWorkflowName": "foo",
+		"permWorkflowName": w.Name,
 	}
 	uri := router.GetRoute("POST", api.postWorkflowRunHandler, vars)
 	test.NotEmpty(t, uri)
diff --git a/engine/api/workflow_trigger.go b/engine/api/workflow_trigger.go
@@ -39,7 +39,7 @@ func (api *API) getWorkflowTriggerConditionHandler() service.Handler {
 
 		wr, err := workflow.LoadLastRun(api.mustDB(), key, name, workflow.LoadRunOptions{})
 		if err != nil {
-			if !sdk.ErrorIs(err, sdk.ErrWorkflowNotFound) {
+			if !sdk.ErrorIs(err, sdk.ErrNotFound) {
 				return sdk.WrapError(err, "unable to load last run workflow")
 			}
 		}
diff --git a/sdk/error.go b/sdk/error.go
@@ -105,7 +105,6 @@ var (
 	ErrJobAlreadyBooked                              = Error{ID: 89, Status: http.StatusConflict}
 	ErrPipelineBuildNotFound                         = Error{ID: 90, Status: http.StatusNotFound}
 	ErrAlreadyTaken                                  = Error{ID: 91, Status: http.StatusGone}
-	ErrWorkflowNotFound                              = Error{ID: 92, Status: http.StatusNotFound}
 	ErrWorkflowNodeNotFound                          = Error{ID: 93, Status: http.StatusNotFound}
 	ErrWorkflowInvalidRoot                           = Error{ID: 94, Status: http.StatusBadRequest}
 	ErrWorkflowNodeRef                               = Error{ID: 95, Status: http.StatusBadRequest}
@@ -296,7 +295,6 @@ var errorsAmericanEnglish = map[int]string{
 	ErrJobAlreadyBooked.ID:                              "Job already booked",
 	ErrPipelineBuildNotFound.ID:                         "Pipeline build not found",
 	ErrAlreadyTaken.ID:                                  "This job is already taken by another worker",
-	ErrWorkflowNotFound.ID:                              "Workflow not found",
 	ErrWorkflowNodeNotFound.ID:                          "Workflow node not found",
 	ErrWorkflowInvalidRoot.ID:                           "Invalid workflow root",
 	ErrWorkflowNodeRef.ID:                               "Invalid workflow node reference",
@@ -480,7 +478,6 @@ var errorsFrench = map[int]string{
 	ErrJobAlreadyBooked.ID:                              "Le job est déjà réservé",
 	ErrPipelineBuildNotFound.ID:                         "Le pipeline build n'a pu être trouvé",
 	ErrAlreadyTaken.ID:                                  "Ce job est déjà en cours de traitement par un autre worker",
-	ErrWorkflowNotFound.ID:                              "Workflow introuvable",
 	ErrWorkflowNodeNotFound.ID:                          "Noeud de Workflow introuvable",
 	ErrWorkflowInvalidRoot.ID:                           "Racine de Workflow invalide",
 	ErrWorkflowNodeRef.ID:                               "Référence de noeud de workflow invalide",

Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ var workflowDeleteCmd = cli.Command{`
`19`	`19`
`20`	`20`	`func workflowDeleteRun(v cli.Values) error {`
`21`	`21`	`err := client.WorkflowDelete(v.GetString(_ProjectKey), v.GetString(_WorkflowName))`
`22`		`- if err != nil && v.GetBool("force") && sdk.ErrorIs(err, sdk.ErrWorkflowNotFound) {`
	`22`	`+ if err != nil && v.GetBool("force") && sdk.ErrorIs(err, sdk.ErrNotFound) {`
`23`	`23`	`fmt.Println(err.Error())`
`24`	`24`	`os.Exit(0)`
`25`	`25`	`}`
Original file line number	Diff line number	Diff line change
`@@ -690,7 +690,7 @@ func (api *API) getTemplateInstanceHandler() service.Handler {`
`690`	`690`
`691`	`691`	`wf, err := workflow.Load(ctx, api.mustDB(), api.Cache, *proj, workflowName, workflow.LoadOptions{})`
`692`	`692`	`if err != nil {`
`693`		`- if sdk.ErrorIs(err, sdk.ErrWorkflowNotFound) {`
	`693`	`+ if sdk.ErrorIs(err, sdk.ErrNotFound) {`
`694`	`694`	`return sdk.NewErrorFrom(sdk.ErrNotFound, "cannot load workflow %s", workflowName)`
`695`	`695`	`}`
`696`	`696`	`return sdk.WithStack(err)`
Original file line number	Diff line number	Diff line change
`@@ -577,7 +577,7 @@ func (api *API) deleteWorkflowHandler() service.Handler {`
`577`	`577`	`return sdk.WrapError(errW, "Cannot check Workflow %s", key)`
`578`	`578`	`}`
`579`	`579`	`if !b {`
`580`		`- return sdk.WithStack(sdk.ErrWorkflowNotFound)`
	`580`	`+ return sdk.WithStack(sdk.ErrNotFound)`
`581`	`581`	`}`
`582`	`582`
`583`	`583`	`tx, errT := api.mustDB().Begin()`
`@@ -713,7 +713,7 @@ func (api *API) getWorkflowNotificationsConditionsHandler() service.Handler {`
`713`	`713`
`714`	`714`	`wr, errr := workflow.LoadLastRun(api.mustDB(), key, name, workflow.LoadRunOptions{})`
`715`	`715`	`if errr != nil {`
`716`		`- if !sdk.ErrorIs(errr, sdk.ErrWorkflowNotFound) {`
	`716`	`+ if !sdk.ErrorIs(errr, sdk.ErrNotFound) {`
`717`	`717`	`return sdk.WrapError(errr, "getWorkflowTriggerConditionHandler> Unable to load last run workflow")`
`718`	`718`	`}`
`719`	`719`	`}`
Original file line number	Diff line number	Diff line change
`@@ -263,7 +263,7 @@ func LoadAll(db gorp.SqlExecutor, projectKey string) (sdk.Workflows, error) {`
`263`	`263`
`264`	`264`	`if _, err := db.Select(&dbRes, query, projectKey); err != nil {`
`265`	`265`	`if err == sql.ErrNoRows {`
`266`		`- return nil, sdk.ErrWorkflowNotFound`
	`266`	`+ return nil, sdk.WithStack(sdk.ErrNotFound)`
`267`	`267`	`}`
`268`	`268`	`return nil, sdk.WrapError(err, "Unable to load workflows project %s", projectKey)`
`269`	`269`	`}`
`@@ -511,7 +511,7 @@ func load(ctx context.Context, db gorp.SqlExecutor, proj sdk.Project, opts LoadO`
`511`	`511`	`next()`
`512`	`512`	`if err != nil {`
`513`	`513`	`if err == sql.ErrNoRows {`
`514`		`- return nil, sdk.ErrWorkflowNotFound`
	`514`	`+ return nil, sdk.WithStack(sdk.ErrNotFound)`
`515`	`515`	`}`
`516`	`516`	`return nil, sdk.WrapError(err, "Unable to load workflow")`
`517`	`517`	`}`
Original file line number	Diff line number	Diff line change
`@@ -439,13 +439,13 @@ func loadRun(db gorp.SqlExecutor, loadOpts LoadRunOptions, query string, args ..`
`439`	`439`	`runDB := &Run{}`
`440`	`440`	`if err := db.SelectOne(runDB, query, args...); err != nil {`
`441`	`441`	`if err == sql.ErrNoRows {`
`442`		`- return nil, sdk.ErrWorkflowNotFound`
	`442`	`+ return nil, sdk.WithStack(sdk.ErrNotFound)`
`443`	`443`	`}`
`444`	`444`	`return nil, sdk.WrapError(err, "Unable to load workflow run. query:%s args:%v", query, args)`
`445`	`445`	`}`
`446`	`446`	`wr := sdk.WorkflowRun(*runDB)`
`447`	`447`	`if !loadOpts.WithDeleted && wr.ToDelete {`
`448`		`- return nil, sdk.WithStack(sdk.ErrWorkflowNotFound)`
	`448`	`+ return nil, sdk.WithStack(sdk.ErrNotFound)`
`449`	`449`	`}`
`450`	`450`
`451`	`451`	`tags, errT := loadTagsByRunID(db, wr.ID)`
Original file line number	Diff line number	Diff line change
`@@ -39,7 +39,7 @@ func (api *API) getWorkflowTriggerConditionHandler() service.Handler {`
`39`	`39`
`40`	`40`	`wr, err := workflow.LoadLastRun(api.mustDB(), key, name, workflow.LoadRunOptions{})`
`41`	`41`	`if err != nil {`
`42`		`- if !sdk.ErrorIs(err, sdk.ErrWorkflowNotFound) {`
	`42`	`+ if !sdk.ErrorIs(err, sdk.ErrNotFound) {`
`43`	`43`	`return sdk.WrapError(err, "unable to load last run workflow")`
`44`	`44`	`}`
`45`	`45`	`}`