fix: export cve adds resource check and project validation (#17265)

1. Add resource permission check for API handler
2. Validate export cve params project
3. Optimize friendly human message when execution status is error

Signed-off-by: chlins <chenyuzh@vmware.com>

api/v2.0/swagger.yaml

@@ -9228,10 +9228,6 @@ definitions:
         type: string
         x-omitempty: false
         description: The status text
-      job_name:
-        type: string
-        x-omitempty: false
-        description: The name of the job as specified by the user
       user_name:
         type: string
         x-omitempty: false

src/common/rbac/const.go

@@ -76,4 +76,5 @@ const (
 	ResourceScanAll            = Resource("scan-all")
 	ResourceSystemVolumes      = Resource("system-volumes")
 	ResourcePurgeAuditLog      = Resource("purge-audit")
+	ResourceExportCVE          = Resource("export-cve")
 )

src/common/rbac/project/rbac_role.go

@@ -124,6 +124,10 @@ var (
 			{Resource: rbac.ResourcePreatPolicy, Action: rbac.ActionUpdate},
 			{Resource: rbac.ResourcePreatPolicy, Action: rbac.ActionDelete},
 			{Resource: rbac.ResourcePreatPolicy, Action: rbac.ActionList},
+
+			{Resource: rbac.ResourceExportCVE, Action: rbac.ActionCreate},
+			{Resource: rbac.ResourceExportCVE, Action: rbac.ActionRead},
+			{Resource: rbac.ResourceExportCVE, Action: rbac.ActionList},
 		},
 
 		"maintainer": {
@@ -208,6 +212,10 @@ var (
 
 			{Resource: rbac.ResourceArtifactLabel, Action: rbac.ActionCreate},
 			{Resource: rbac.ResourceArtifactLabel, Action: rbac.ActionDelete},
+
+			{Resource: rbac.ResourceExportCVE, Action: rbac.ActionCreate},
+			{Resource: rbac.ResourceExportCVE, Action: rbac.ActionRead},
+			{Resource: rbac.ResourceExportCVE, Action: rbac.ActionList},
 		},
 
 		"developer": {
@@ -270,6 +278,10 @@ var (
 
 			{Resource: rbac.ResourceArtifactLabel, Action: rbac.ActionCreate},
 			{Resource: rbac.ResourceArtifactLabel, Action: rbac.ActionDelete},
+
+			{Resource: rbac.ResourceExportCVE, Action: rbac.ActionCreate},
+			{Resource: rbac.ResourceExportCVE, Action: rbac.ActionRead},
+			{Resource: rbac.ResourceExportCVE, Action: rbac.ActionList},
 		},
 
 		"guest": {

src/controller/scandataexport/execution.go

@@ -114,6 +114,7 @@ func (c *controller) Start(ctx context.Context, request export.Request) (executi
 	logger := log.GetLogger(ctx)
 	vendorID := int64(ctx.Value(export.CsvJobVendorIDKey).(int))
 	extraAttrs := make(map[string]interface{})
+	extraAttrs[export.ProjectIDsAttribute] = request.Projects
 	extraAttrs[export.JobNameAttribute] = request.JobName
 	extraAttrs[export.UserNameAttribute] = request.UserName
 	id, err := c.execMgr.Create(ctx, job.ScanDataExport, vendorID, task.ExecutionTriggerManual, extraAttrs)
@@ -170,6 +171,11 @@ func (c *controller) convertToExportExecStatus(ctx context.Context, exec *task.E
 		StartTime:     exec.StartTime,
 		EndTime:       exec.EndTime,
 	}
+	if pids, ok := exec.ExtraAttrs[export.ProjectIDsAttribute]; ok {
+		for _, pid := range pids.([]interface{}) {
+			execStatus.ProjectIDs = append(execStatus.ProjectIDs, int64(pid.(float64)))
+		}
+	}
 	if digest, ok := exec.ExtraAttrs[export.DigestKey]; ok {
 		execStatus.ExportDataDigest = digest.(string)
 	}

src/controller/scandataexport/execution_test.go

@@ -241,6 +241,7 @@ func (suite *ScanDataExportExecutionTestSuite) TestStart() {
 	{
 		// get execution succeeds
 		attrs := make(map[string]interface{})
+		attrs[export.ProjectIDsAttribute] = []int64{1}
 		attrs[export.JobNameAttribute] = "test-job"
 		attrs[export.UserNameAttribute] = "test-user"
 		suite.execMgr.On("Create", mock.Anything, mock.Anything, mock.Anything, mock.Anything, attrs).Return(int64(10), nil)
@@ -248,6 +249,7 @@ func (suite *ScanDataExportExecutionTestSuite) TestStart() {
 		ctx := context.Background()
 		ctx = context.WithValue(ctx, export.CsvJobVendorIDKey, int(-1))
 		criteria := export.Request{}
+		criteria.Projects = []int64{1}
 		criteria.UserName = "test-user"
 		criteria.JobName = "test-job"
 		executionId, err := suite.ctl.Start(ctx, criteria)
@@ -303,13 +305,14 @@ func (suite *ScanDataExportExecutionTestSuite) TestStartWithTaskManagerError() {
 		ctx := context.Background()
 		ctx = context.WithValue(ctx, export.CsvJobVendorIDKey, int(-1))
 		attrs := make(map[string]interface{})
+		attrs[export.ProjectIDsAttribute] = []int64{1}
 		attrs[export.JobNameAttribute] = "test-job"
 		attrs[export.UserNameAttribute] = "test-user"
 		suite.execMgr.On("Create", mock.Anything, mock.Anything, mock.Anything, mock.Anything, attrs).Return(int64(10), nil)
 		suite.taskMgr.On("Create", mock.Anything, mock.Anything, mock.Anything).Return(int64(-1), errors.New("Test Error"))
 		mock.OnAnything(suite.execMgr, "StopAndWait").Return(nil)
 		mock.OnAnything(suite.execMgr, "MarkError").Return(nil)
-		_, err := suite.ctl.Start(ctx, export.Request{JobName: "test-job", UserName: "test-user"})
+		_, err := suite.ctl.Start(ctx, export.Request{JobName: "test-job", UserName: "test-user", Projects: []int64{1}})
 		suite.Error(err)
 	}
 }

src/pkg/scan/export/constants.go

@@ -4,13 +4,14 @@ package export
 type CsvJobVendorID string
 
 const (
-	JobNameAttribute   = "job_name"
-	UserNameAttribute  = "user_name"
-	ScanDataExportDir  = "/var/scandata_exports"
-	QueryPageSize      = 100000
-	ArtifactGroupSize  = 10000
-	DigestKey          = "artifact_digest"
-	CreateTimestampKey = "create_ts"
-	Vendor             = "SCAN_DATA_EXPORT"
-	CsvJobVendorIDKey  = CsvJobVendorID("vendorId")
+	ProjectIDsAttribute = "project_ids"
+	JobNameAttribute    = "job_name"
+	UserNameAttribute   = "user_name"
+	ScanDataExportDir   = "/var/scandata_exports"
+	QueryPageSize       = 100000
+	ArtifactGroupSize   = 10000
+	DigestKey           = "artifact_digest"
+	CreateTimestampKey  = "create_ts"
+	Vendor              = "SCAN_DATA_EXPORT"
+	CsvJobVendorIDKey   = CsvJobVendorID("vendorId")
 )

src/pkg/scan/export/model.go

@@ -78,6 +78,8 @@ type Execution struct {
 	ID int64
 	// UserID triggering the execution
 	UserID int64
+	// ProjectIDs contains projects ids
+	ProjectIDs []int64
 	// Status provides the status of the execution
 	Status string
 	// StatusMessage contains the human-readable status message for the execution

src/server/v2.0/handler/scanexport.go

@@ -15,7 +15,7 @@ import (
 
 	"github.com/goharbor/harbor/src/common/rbac"
 	"github.com/goharbor/harbor/src/controller/scandataexport"
-	"github.com/goharbor/harbor/src/jobservice/logger"
+	"github.com/goharbor/harbor/src/jobservice/job"
 	"github.com/goharbor/harbor/src/lib/log"
 	"github.com/goharbor/harbor/src/lib/orm"
 	"github.com/goharbor/harbor/src/pkg/scan/export"
@@ -46,7 +46,19 @@ func (se *scanDataExportAPI) Prepare(ctx context.Context, operation string, para
 }
 
 func (se *scanDataExportAPI) ExportScanData(ctx context.Context, params operation.ExportScanDataParams) middleware.Responder {
-	if err := se.RequireAuthenticated(ctx); err != nil {
+	// validate project id, currently we only support single project
+	criteria := params.Criteria
+	if criteria == nil {
+		err := errors.New(errors.Errorf("criteria is invalid: %v", criteria)).WithCode(errors.BadRequestCode)
+		return se.SendError(ctx, err)
+	}
+
+	if len(criteria.Projects) != 1 {
+		err := errors.New(errors.Errorf("only support export single project, invalid value: %v", criteria.Projects)).WithCode(errors.BadRequestCode)
+		return se.SendError(ctx, err)
+	}
+
+	if err := se.RequireProjectAccess(ctx, criteria.Projects[0], rbac.ActionCreate, rbac.ResourceExportCVE); err != nil {
 		return se.SendError(ctx, err)
 	}
 
@@ -57,17 +69,6 @@ func (se *scanDataExportAPI) ExportScanData(ctx context.Context, params operatio
 		return operation.NewExportScanDataBadRequest().WithPayload(errors)
 	}
 
-	// loop through the list of projects and validate that scan privilege and create privilege
-	// is available for all projects
-	// TODO : Should we just ignore projects that do not have the required level of access?
-
-	projects := params.Criteria.Projects
-	for _, project := range projects {
-		if err := se.RequireProjectAccess(ctx, project, rbac.ActionCreate, rbac.ResourceScan); err != nil {
-			return se.SendError(ctx, err)
-		}
-	}
-
 	scanDataExportJob := new(models.ScanDataExportJob)
 
 	secContext, err := se.GetSecurityContext(ctx)
@@ -104,12 +105,17 @@ func (se *scanDataExportAPI) ExportScanData(ctx context.Context, params operatio
 }
 
 func (se *scanDataExportAPI) GetScanDataExportExecution(ctx context.Context, params operation.GetScanDataExportExecutionParams) middleware.Responder {
-	err := se.RequireAuthenticated(ctx)
+	if err := se.RequireAuthenticated(ctx); err != nil {
+		return se.SendError(ctx, err)
+	}
+
+	execution, err := se.scanDataExportCtl.GetExecution(ctx, params.ExecutionID)
 	if err != nil {
 		return se.SendError(ctx, err)
 	}
-	execution, err := se.scanDataExportCtl.GetExecution(ctx, params.ExecutionID)
-	if err != nil {
+
+	// check the permission by project ids in execution
+	if err = se.requireProjectsAccess(ctx, execution.ProjectIDs, rbac.ActionRead, rbac.ResourceExportCVE); err != nil {
 		return se.SendError(ctx, err)
 	}
 
@@ -134,7 +140,6 @@ func (se *scanDataExportAPI) GetScanDataExportExecution(ctx context.Context, par
 		StatusText:  execution.StatusMessage,
 		Trigger:     execution.Trigger,
 		UserID:      execution.UserID,
-		JobName:     execution.JobName,
 		UserName:    execution.UserName,
 		FilePresent: execution.FilePresent,
 	}
@@ -143,12 +148,11 @@ func (se *scanDataExportAPI) GetScanDataExportExecution(ctx context.Context, par
 }
 
 func (se *scanDataExportAPI) DownloadScanData(ctx context.Context, params operation.DownloadScanDataParams) middleware.Responder {
-	err := se.RequireAuthenticated(ctx)
-	if err != nil {
+	if err := se.RequireAuthenticated(ctx); err != nil {
 		return se.SendError(ctx, err)
 	}
-	execution, err := se.scanDataExportCtl.GetExecution(ctx, params.ExecutionID)
 
+	execution, err := se.scanDataExportCtl.GetExecution(ctx, params.ExecutionID)
 	if err != nil {
 		if notFound := orm.AsNotFoundError(err, "execution with id: %d not found", params.ExecutionID); notFound != nil {
 			return middleware.ResponderFunc(func(writer http.ResponseWriter, producer runtime.Producer) {
@@ -158,6 +162,11 @@ func (se *scanDataExportAPI) DownloadScanData(ctx context.Context, params operat
 		return se.SendError(ctx, err)
 	}
 
+	// check the permission by project ids in execution
+	if err = se.requireProjectsAccess(ctx, execution.ProjectIDs, rbac.ActionRead, rbac.ResourceExportCVE); err != nil {
+		return se.SendError(ctx, err)
+	}
+
 	// check if the CSV artifact for the execution exists
 	if !execution.FilePresent {
 		return middleware.ResponderFunc(func(writer http.ResponseWriter, producer runtime.Producer) {
@@ -182,7 +191,7 @@ func (se *scanDataExportAPI) DownloadScanData(ctx context.Context, params operat
 	if err != nil {
 		return se.SendError(ctx, err)
 	}
-	logger.Infof("reading data from file : %s", repositoryName)
+	log.Infof("reading data from file : %s", repositoryName)
 
 	return middleware.ResponderFunc(func(writer http.ResponseWriter, producer runtime.Producer) {
 		defer se.cleanUpArtifact(ctx, repositoryName, execution.ExportDataDigest, params.ExecutionID, file)
@@ -191,27 +200,29 @@ func (se *scanDataExportAPI) DownloadScanData(ctx context.Context, params operat
 		writer.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=%q", fmt.Sprintf("%s.csv", repositoryName)))
 		nbytes, err := io.Copy(writer, file)
 		if err != nil {
-			logger.Errorf("Encountered error while copying data: %v", err)
+			log.Errorf("Encountered error while copying data: %v", err)
 		} else {
-			logger.Debugf("Copied %v bytes from file to client", nbytes)
+			log.Debugf("Copied %v bytes from file to client", nbytes)
 		}
 	})
 }
 
 func (se *scanDataExportAPI) GetScanDataExportExecutionList(ctx context.Context, params operation.GetScanDataExportExecutionListParams) middleware.Responder {
-	err := se.RequireAuthenticated(ctx)
-	if err != nil {
+	if err := se.RequireAuthenticated(ctx); err != nil {
 		return se.SendError(ctx, err)
 	}
-	secContext, err := se.GetSecurityContext(ctx)
 
+	secContext, err := se.GetSecurityContext(ctx)
 	if err != nil {
 		return se.SendError(ctx, err)
 	}
+
 	executions, err := se.scanDataExportCtl.ListExecutions(ctx, secContext.GetUsername())
 	if err != nil {
 		return se.SendError(ctx, err)
 	}
+	// projectSet store the unrepeated project ids
+	projectSet := make(map[int64]struct{})
 	execs := make([]*models.ScanDataExportExecution, 0)
 	for _, execution := range executions {
 		sdeExec := &models.ScanDataExportExecution{
@@ -223,11 +234,30 @@ func (se *scanDataExportAPI) GetScanDataExportExecutionList(ctx context.Context,
 			Trigger:     execution.Trigger,
 			UserID:      execution.UserID,
 			UserName:    execution.UserName,
-			JobName:     execution.JobName,
 			FilePresent: execution.FilePresent,
 		}
+		// add human friendly message when status is error
+		if sdeExec.Status == job.ErrorStatus.String() {
+			sdeExec.StatusText = "Please contact the system administrator to check the logs of jobservice."
+		}
+		// store project ids
+		for _, pid := range execution.ProjectIDs {
+			projectSet[pid] = struct{}{}
+		}
+
 		execs = append(execs, sdeExec)
 	}
+
+	// convert projectSet to pids
+	var pids []int64
+	for pid := range projectSet {
+		pids = append(pids, pid)
+	}
+	// check the permission by project ids in execution
+	if err = se.requireProjectsAccess(ctx, pids, rbac.ActionRead, rbac.ResourceExportCVE); err != nil {
+		return se.SendError(ctx, err)
+	}
+
 	sdeExecList := models.ScanDataExportExecutionList{Items: execs}
 	return operation.NewGetScanDataExportExecutionListOK().WithPayload(&sdeExecList)
 }
@@ -247,7 +277,7 @@ func (se *scanDataExportAPI) convertToCriteria(requestCriteria *models.ScanDataE
 
 func (se *scanDataExportAPI) cleanUpArtifact(ctx context.Context, repositoryName, digest string, execID int64, file io.ReadCloser) {
 	file.Close()
-	logger.Infof("Deleting report artifact : %v:%v", repositoryName, digest)
+	log.Infof("Deleting report artifact : %v:%v", repositoryName, digest)
 
 	// the entire delete operation is executed within a transaction to ensure that any failures
 	// during the blob creation or tracking record creation result in a rollback of the transaction
@@ -270,3 +300,15 @@ func (se *scanDataExportAPI) cleanUpArtifact(ctx context.Context, repositoryName
 		log.Errorf("Error deleting system artifact record for %s/%s/%s: %v", vendor, repositoryName, digest, err)
 	}
 }
+
+func (se *scanDataExportAPI) requireProjectsAccess(ctx context.Context, pids []int64, action rbac.Action, subresource ...rbac.Resource) error {
+	// check project permission one by one, return error if any project cannot
+	// access permission.
+	for _, pid := range pids {
+		if err := se.RequireProjectAccess(ctx, pid, action, subresource...); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}

src/server/v2.0/handler/scanexport_test.go

@@ -75,6 +75,7 @@ func (suite *ScanExportTestSuite) TestAuthorization() {
 			{http.MethodGet, "/export/cve/download/100", nil, nil},
 		}
 
+		suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(false).Times(3)
 		suite.Security.On("IsAuthenticated").Return(false).Times(3)
 		for _, req := range reqs {
 
@@ -134,6 +135,7 @@ func (suite *ScanExportTestSuite) TestExportScanData() {
 
 	// user authenticated but incorrect/unsupported header sent across
 	{
+		suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Once()
 		suite.Security.On("IsAuthenticated").Return(true).Once()
 		url := "/export/cve"
 
@@ -157,6 +159,32 @@ func (suite *ScanExportTestSuite) TestExportScanData() {
 		suite.Equal(nil, err)
 	}
 
+	// should return 400 if project id number is not one
+	{
+		suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Once()
+		suite.Security.On("IsAuthenticated").Return(true).Once()
+		url := "/export/cve"
+
+		criteria := models.ScanDataExportRequest{
+			CVEIds:       "CVE-123",
+			Labels:       []int64{100},
+			Projects:     []int64{200, 300},
+			Repositories: "test-repo",
+			Tags:         "{test-tag1, test-tag2}",
+		}
+
+		data, err := json.Marshal(criteria)
+		buffer := bytes.NewBuffer(data)
+
+		headers := make(map[string]string)
+		headers["X-Scan-Data-Type"] = v1.MimeTypeGenericVulnerabilityReport
+
+		mock.OnAnything(suite.scanExportCtl, "Start").Return(int64(100), nil).Once()
+		res, err := suite.DoReq(http.MethodPost, url, buffer, headers)
+		suite.Equal(400, res.StatusCode)
+		suite.Equal(nil, err)
+	}
+
 }
 
 func (suite *ScanExportTestSuite) TestExportScanDataGetUserIdError() {
@@ -279,7 +307,6 @@ func (suite *ScanExportTestSuite) TestGetScanDataExportExecution() {
 	respData := models.ScanDataExportExecution{}
 	json.NewDecoder(res.Body).Decode(&respData)
 	suite.Equal("test-user", respData.UserName)
-	suite.Equal("test-job", respData.JobName)
 	suite.Equal(false, respData.FilePresent)
 
 }