From 28fcaed18beb8dd7aa87e018dbbb0db2adbb3ad1 Mon Sep 17 00:00:00 2001 From: Richard LT Date: Tue, 9 Mar 2021 18:28:38 +0100 Subject: [PATCH] fix(cdn,api): check workflow access with id, use workflow name from run (#5753) --- engine/Makefile | 2 +- engine/api/api_routes.go | 2 +- engine/api/workflow/dao.go | 15 +++++++ engine/api/workflow_run_log.go | 40 ++++++++++++++----- engine/cdn/auth.go | 9 +++-- engine/cdn/auth_test.go | 2 +- engine/cdn/item_handler_test.go | 4 +- engine/cdn/item_logs_handler_test.go | 4 +- sdk/cdsclient/client_workflow.go | 4 +- sdk/cdsclient/interface.go | 2 +- .../mock_cdsclient/interface_mock.go | 16 ++++---- 11 files changed, 67 insertions(+), 33 deletions(-) diff --git a/engine/Makefile b/engine/Makefile index 6829cbd61f..8fff88fa8a 100644 --- a/engine/Makefile +++ b/engine/Makefile @@ -119,7 +119,7 @@ test-db-stop-docker: test-db-start-docker: $(TEST_DB_START_DOCKER) - @sleep 5 + @sleep 10 $(MAKE) test-api-db-create $(MAKE) test-cdn-db-create diff --git a/engine/api/api_routes.go b/engine/api/api_routes.go index 9a1ee29308..ff0bbd3560 100644 --- a/engine/api/api_routes.go +++ b/engine/api/api_routes.go @@ -281,7 +281,7 @@ func (api *API) InitRouter() { r.Handle("/project/{key}/workflows/{permWorkflowName}/nodes/{nodeRunID}/job/{runJobID}/service/{serviceName}/log", Scope(sdk.AuthConsumerScopeRun), r.GET(api.getWorkflowNodeRunJobServiceLogHandler)) r.Handle("/project/{key}/workflows/{permWorkflowName}/nodes/{nodeRunID}/job/{runJobID}/links", Scope(sdk.AuthConsumerScopeRun), r.GET(api.getWorkflowNodeRunJobStepLinksHandler)) r.Handle("/project/{key}/workflows/{permWorkflowName}/nodes/{nodeRunID}/job/{runJobID}/step/{stepOrder}/link", Scope(sdk.AuthConsumerScopeRun), r.GET(api.getWorkflowNodeRunJobStepLinkHandler)) - r.Handle("/project/{key}/workflows/{workflowName}/type/{type}/access", ScopeNone(), r.GET(api.getWorkflowAccessHandler)) + r.Handle("/project/{key}/workflows/{workflowID}/type/{type}/access", ScopeNone(), r.GET(api.getWorkflowAccessHandler)) r.Handle("/project/{key}/workflows/{permWorkflowName}/nodes/{nodeRunID}/job/{runJobID}/step/{stepOrder}/log", Scope(sdk.AuthConsumerScopeRun), r.GET(api.getWorkflowNodeRunJobStepLogHandler)) r.Handle("/project/{key}/workflows/{permWorkflowName}/node/{nodeID}/triggers/condition", Scope(sdk.AuthConsumerScopeRun), r.GET(api.getWorkflowTriggerConditionHandler)) r.Handle("/project/{key}/workflows/{permWorkflowName}/hook/triggers/condition", Scope(sdk.AuthConsumerScopeRun), r.GET(api.getWorkflowTriggerHookConditionHandler)) diff --git a/engine/api/workflow/dao.go b/engine/api/workflow/dao.go index ca145366a6..871e143d58 100644 --- a/engine/api/workflow/dao.go +++ b/engine/api/workflow/dao.go @@ -79,6 +79,21 @@ func Exists(db gorp.SqlExecutor, key string, name string) (bool, error) { return count > 0, nil } +// ExistsID checks if a workflow exists for given ID and project +func ExistsID(db gorp.SqlExecutor, key string, id int64) (bool, error) { + query := ` + select count(1) + from workflow + join project on project.id = workflow.project_id + where project.projectkey = $1 + and workflow.id = $2` + count, err := db.SelectInt(query, key, id) + if err != nil { + return false, sdk.WithStack(err) + } + return count > 0, nil +} + func LoadByRepo(ctx context.Context, db gorp.SqlExecutor, proj sdk.Project, repo string, opts LoadOptions) (*sdk.Workflow, error) { ctx, end := telemetry.Span(ctx, "workflow.Load") defer end() diff --git a/engine/api/workflow_run_log.go b/engine/api/workflow_run_log.go index 66d5b207ec..c7a39c206e 100644 --- a/engine/api/workflow_run_log.go +++ b/engine/api/workflow_run_log.go @@ -154,12 +154,19 @@ func (api *API) getWorkflowNodeRunJobStepLinksHandler() service.Handler { return sdk.NewErrorFrom(sdk.ErrNotFound, "cannot find run job for id %d", runJobID) } + jobRun, err := workflow.LoadRunByID(api.mustDB(), nodeRun.WorkflowRunID, workflow.LoadRunOptions{ + DisableDetailledNodeRun: true, + }) + if err != nil { + return err + } + refs := make([]sdk.CDNLogAPIRef, 0) apiRef := sdk.CDNLogAPIRef{ ProjectKey: projectKey, - WorkflowName: workflowName, - WorkflowID: nodeRun.WorkflowID, - RunID: nodeRun.WorkflowRunID, + WorkflowName: jobRun.Workflow.Name, + WorkflowID: jobRun.WorkflowID, + RunID: jobRun.ID, NodeRunName: nodeRun.WorkflowNodeName, NodeRunID: nodeRun.ID, NodeRunJobName: runJob.Job.Action.Name, @@ -243,11 +250,18 @@ func (api *API) getWorkflowNodeRunJobLogLinkHandler(ctx context.Context, w http. return sdk.NewErrorFrom(sdk.ErrNotFound, "cannot find run job for id %d", runJobID) } + jobRun, err := workflow.LoadRunByID(api.mustDB(), nodeRun.WorkflowRunID, workflow.LoadRunOptions{ + DisableDetailledNodeRun: true, + }) + if err != nil { + return err + } + apiRef := sdk.CDNLogAPIRef{ ProjectKey: projectKey, - WorkflowName: workflowName, - WorkflowID: nodeRun.WorkflowID, - RunID: nodeRun.WorkflowRunID, + WorkflowName: jobRun.Workflow.Name, + WorkflowID: jobRun.WorkflowID, + RunID: jobRun.ID, NodeRunName: nodeRun.WorkflowNodeName, NodeRunID: nodeRun.ID, NodeRunJobName: runJob.Job.Action.Name, @@ -334,9 +348,12 @@ func (api *API) getWorkflowAccessHandler() service.Handler { return sdk.WrapError(sdk.ErrForbidden, "missing session id header") } - workflowName := vars["workflowName"] + workflowID, err := requestVarInt(r, "workflowID") + if err != nil { + return err + } - exists, err := workflow.Exists(api.mustDB(), projectKey, workflowName) + exists, err := workflow.ExistsID(api.mustDB(), projectKey, workflowID) if err != nil { return err } @@ -359,13 +376,14 @@ func (api *API) getWorkflowAccessHandler() service.Handler { maintainerOrAdmin := consumer.Maintainer() || consumer.Admin() - perms, err := permission.LoadWorkflowMaxLevelPermission(ctx, api.mustDB(), projectKey, []string{workflowName}, consumer.GetGroupIDs()) + perms, err := permission.LoadWorkflowMaxLevelPermissionByWorkflowIDs(ctx, api.mustDB(), []int64{workflowID}, consumer.GetGroupIDs()) if err != nil { return sdk.NewErrorWithStack(err, sdk.ErrUnauthorized) } - maxLevelPermission := perms.Level(workflowName) + workflowIDString := strconv.FormatInt(workflowID, 10) + maxLevelPermission := perms.Level(workflowIDString) if maxLevelPermission < sdk.PermissionRead && !maintainerOrAdmin { - return sdk.WrapError(sdk.ErrUnauthorized, "not authorized for workflow %s/%s", projectKey, workflowName) + return sdk.WrapError(sdk.ErrUnauthorized, "not authorized for workflow %s/%s", projectKey, workflowIDString) } return service.WriteJSON(w, nil, http.StatusOK) diff --git a/engine/cdn/auth.go b/engine/cdn/auth.go index 39bf93b719..596f897948 100644 --- a/engine/cdn/auth.go +++ b/engine/cdn/auth.go @@ -89,16 +89,17 @@ func (s *Service) itemAccessCheck(ctx context.Context, item sdk.CDNItem) error { return nil } - var projectKey, workflowName string + var projectKey string + var workflowID int64 switch item.Type { case sdk.CDNTypeItemStepLog, sdk.CDNTypeItemServiceLog: logRef, _ := item.GetCDNLogApiRef() projectKey = logRef.ProjectKey - workflowName = logRef.WorkflowName + workflowID = logRef.WorkflowID case sdk.CDNTypeItemRunResult: artRef, _ := item.GetCDNRunResultApiRef() projectKey = artRef.ProjectKey - workflowName = artRef.WorkflowName + workflowID = artRef.WorkflowID case sdk.CDNTypeItemWorkerCache: artRef, _ := item.GetCDNWorkerCacheApiRef() projectKey = artRef.ProjectKey @@ -108,7 +109,7 @@ func (s *Service) itemAccessCheck(ctx context.Context, item sdk.CDNItem) error { switch item.Type { case sdk.CDNTypeItemStepLog, sdk.CDNTypeItemServiceLog, sdk.CDNTypeItemRunResult: - if err := s.Client.WorkflowAccess(ctx, projectKey, workflowName, sessionID, item.Type); err != nil { + if err := s.Client.WorkflowAccess(ctx, projectKey, workflowID, sessionID, item.Type); err != nil { return sdk.NewErrorWithStack(err, sdk.ErrNotFound) } case sdk.CDNTypeItemWorkerCache: diff --git a/engine/cdn/auth_test.go b/engine/cdn/auth_test.go index 1d643bfff2..da139bee4b 100644 --- a/engine/cdn/auth_test.go +++ b/engine/cdn/auth_test.go @@ -45,7 +45,7 @@ func Test_itemAccessMiddleware(t *testing.T) { sessionID := sdk.UUID() mockClient.EXPECT(). WorkflowAccess(gomock.Any(), gomock.Any(), gomock.Any(), sessionID, gomock.Any()). - DoAndReturn(func(ctx context.Context, projectKey, workflowName, sessionID string, itemItem sdk.CDNItemType) error { + DoAndReturn(func(ctx context.Context, projectKey string, workflowName int64, sessionID string, itemItem sdk.CDNItemType) error { return nil }). Times(1) diff --git a/engine/cdn/item_handler_test.go b/engine/cdn/item_handler_test.go index 85c4ffb0ca..7bb8a83068 100644 --- a/engine/cdn/item_handler_test.go +++ b/engine/cdn/item_handler_test.go @@ -111,7 +111,7 @@ func TestGetItemLogsDownloadHandler(t *testing.T) { s, db := newTestService(t) s.Client = cdsclient.New(cdsclient.Config{Host: "http://lolcat.api", InsecureSkipVerifyTLS: false}) gock.InterceptClient(s.Client.(cdsclient.Raw).HTTPClient()) - gock.New("http://lolcat.api").Get("/project/" + projectKey + "/workflows/MyWorkflow/type/step-log/access").Reply(http.StatusOK).JSON(nil) + gock.New("http://lolcat.api").Get("/project/" + projectKey + "/workflows/1/type/step-log/access").Reply(http.StatusOK).JSON(nil) ctx, cancel := context.WithCancel(context.TODO()) t.Cleanup(cancel) @@ -213,7 +213,7 @@ func TestGetItemArtifactDownloadHandler(t *testing.T) { s.Client = cdsclient.New(cdsclient.Config{Host: "http://lolcat.api", InsecureSkipVerifyTLS: false}) gock.InterceptClient(s.Client.(cdsclient.Raw).HTTPClient()) t.Cleanup(gock.OffAll) - gock.New("http://lolcat.api").Get("/project/" + projectKey + "/workflows/WfName/type/run-result/access").Reply(http.StatusOK).JSON(nil) + gock.New("http://lolcat.api").Get("/project/" + projectKey + "/workflows/1/type/run-result/access").Reply(http.StatusOK).JSON(nil) gock.New("http://lolcat.api").Post("/project/" + projectKey + "/workflows/WfName/runs/0/results/check").Reply(http.StatusNoContent) gock.New("http://lolcat.api").Post("/project/" + projectKey + "/workflows/WfName/runs/0/results").Reply(http.StatusNoContent) diff --git a/engine/cdn/item_logs_handler_test.go b/engine/cdn/item_logs_handler_test.go index 5a419db980..8903456ed3 100644 --- a/engine/cdn/item_logs_handler_test.go +++ b/engine/cdn/item_logs_handler_test.go @@ -202,7 +202,7 @@ func TestGetItemLogsLinesHandler(t *testing.T) { s.Client = cdsclient.New(cdsclient.Config{Host: "http://lolcat.api", InsecureSkipVerifyTLS: false}) gock.InterceptClient(s.Client.(cdsclient.Raw).HTTPClient()) t.Cleanup(gock.Off) - gock.New("http://lolcat.api").Get("/project/" + projectKey + "/workflows/MyWorkflow/type/step-log/access").Reply(http.StatusOK).JSON(nil) + gock.New("http://lolcat.api").Get("/project/" + projectKey + "/workflows/1/type/step-log/access").Reply(http.StatusOK).JSON(nil) ctx, cancel := context.WithCancel(context.TODO()) t.Cleanup(cancel) @@ -296,7 +296,7 @@ func TestGetItemLogsStreamHandler(t *testing.T) { s.Client = cdsclient.New(cdsclient.Config{Host: "http://lolcat.api", InsecureSkipVerifyTLS: false}) gock.InterceptClient(s.Client.(cdsclient.Raw).HTTPClient()) t.Cleanup(gock.Off) - gock.New("http://lolcat.api").Get("/project/" + projectKey + "/workflows/MyWorkflow/type/step-log/access").Reply(http.StatusOK).JSON(nil) + gock.New("http://lolcat.api").Get("/project/" + projectKey + "/workflows/1/type/step-log/access").Reply(http.StatusOK).JSON(nil) ctx, cancel := context.WithCancel(context.TODO()) t.Cleanup(cancel) diff --git a/sdk/cdsclient/client_workflow.go b/sdk/cdsclient/client_workflow.go index 02889f6f5b..7e680bfefc 100644 --- a/sdk/cdsclient/client_workflow.go +++ b/sdk/cdsclient/client_workflow.go @@ -249,8 +249,8 @@ func (c *client) WorkflowNodeRunJobServiceLink(ctx context.Context, projectKey s return &a, nil } -func (c *client) WorkflowAccess(ctx context.Context, projectKey, workflowName, sessionID string, itemType sdk.CDNItemType) error { - url := fmt.Sprintf("/project/%s/workflows/%s/type/%s/access", projectKey, workflowName, itemType) +func (c *client) WorkflowAccess(ctx context.Context, projectKey string, workflowID int64, sessionID string, itemType sdk.CDNItemType) error { + url := fmt.Sprintf("/project/%s/workflows/%d/type/%s/access", projectKey, workflowID, itemType) if _, err := c.GetJSON(ctx, url, nil, SetHeader(sdk.CDSSessionID, sessionID)); err != nil { return err } diff --git a/sdk/cdsclient/interface.go b/sdk/cdsclient/interface.go index 5b5f3a6257..bf117aa1cd 100644 --- a/sdk/cdsclient/interface.go +++ b/sdk/cdsclient/interface.go @@ -351,7 +351,7 @@ type WorkflowClient interface { WorkflowNodeRunJobStepLog(ctx context.Context, projectKey string, workflowName string, nodeRunID, job int64, step int64) (*sdk.BuildState, error) WorkflowNodeRunJobServiceLink(ctx context.Context, projectKey string, workflowName string, nodeRunID, job int64, serviceName string) (*sdk.CDNLogLink, error) WorkflowNodeRunJobServiceLog(ctx context.Context, projectKey string, workflowName string, nodeRunID, job int64, serviceName string) (*sdk.ServiceLog, error) - WorkflowAccess(ctx context.Context, projectKey, workflowName, sessionID string, itemType sdk.CDNItemType) error + WorkflowAccess(ctx context.Context, projectKey string, workflowID int64, sessionID string, itemType sdk.CDNItemType) error WorkflowLogDownload(ctx context.Context, link sdk.CDNLogLink) ([]byte, error) WorkflowNodeRunRelease(projectKey string, workflowName string, runNumber int64, nodeRunID int64, release sdk.WorkflowNodeRunRelease) error WorkflowAllHooksList() ([]sdk.NodeHook, error) diff --git a/sdk/cdsclient/mock_cdsclient/interface_mock.go b/sdk/cdsclient/mock_cdsclient/interface_mock.go index 78503cd8d8..a179991e28 100644 --- a/sdk/cdsclient/mock_cdsclient/interface_mock.go +++ b/sdk/cdsclient/mock_cdsclient/interface_mock.go @@ -4351,17 +4351,17 @@ func (mr *MockWorkflowClientMockRecorder) WorkflowNodeRunJobServiceLog(ctx, proj } // WorkflowAccess mocks base method -func (m *MockWorkflowClient) WorkflowAccess(ctx context.Context, projectKey, workflowName, sessionID string, itemType sdk.CDNItemType) error { +func (m *MockWorkflowClient) WorkflowAccess(ctx context.Context, projectKey string, workflowID int64, sessionID string, itemType sdk.CDNItemType) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "WorkflowAccess", ctx, projectKey, workflowName, sessionID, itemType) + ret := m.ctrl.Call(m, "WorkflowAccess", ctx, projectKey, workflowID, sessionID, itemType) ret0, _ := ret[0].(error) return ret0 } // WorkflowAccess indicates an expected call of WorkflowAccess -func (mr *MockWorkflowClientMockRecorder) WorkflowAccess(ctx, projectKey, workflowName, sessionID, itemType interface{}) *gomock.Call { +func (mr *MockWorkflowClientMockRecorder) WorkflowAccess(ctx, projectKey, workflowID, sessionID, itemType interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WorkflowAccess", reflect.TypeOf((*MockWorkflowClient)(nil).WorkflowAccess), ctx, projectKey, workflowName, sessionID, itemType) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WorkflowAccess", reflect.TypeOf((*MockWorkflowClient)(nil).WorkflowAccess), ctx, projectKey, workflowID, sessionID, itemType) } // WorkflowLogDownload mocks base method @@ -8414,17 +8414,17 @@ func (mr *MockInterfaceMockRecorder) WorkflowNodeRunJobServiceLog(ctx, projectKe } // WorkflowAccess mocks base method -func (m *MockInterface) WorkflowAccess(ctx context.Context, projectKey, workflowName, sessionID string, itemType sdk.CDNItemType) error { +func (m *MockInterface) WorkflowAccess(ctx context.Context, projectKey string, workflowID int64, sessionID string, itemType sdk.CDNItemType) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "WorkflowAccess", ctx, projectKey, workflowName, sessionID, itemType) + ret := m.ctrl.Call(m, "WorkflowAccess", ctx, projectKey, workflowID, sessionID, itemType) ret0, _ := ret[0].(error) return ret0 } // WorkflowAccess indicates an expected call of WorkflowAccess -func (mr *MockInterfaceMockRecorder) WorkflowAccess(ctx, projectKey, workflowName, sessionID, itemType interface{}) *gomock.Call { +func (mr *MockInterfaceMockRecorder) WorkflowAccess(ctx, projectKey, workflowID, sessionID, itemType interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WorkflowAccess", reflect.TypeOf((*MockInterface)(nil).WorkflowAccess), ctx, projectKey, workflowName, sessionID, itemType) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WorkflowAccess", reflect.TypeOf((*MockInterface)(nil).WorkflowAccess), ctx, projectKey, workflowID, sessionID, itemType) } // WorkflowLogDownload mocks base method