Improvements to Vulnerability Data Export functionality. (#17161)

Closes:
* https://github.com/goharbor/harbor/issues/17152
* https://github.com/goharbor/harbor/issues/17153
Better error handling in case of task executions not found in the system

Signed-off-by: prahaladdarkin <prahaladd@vmware.com>

api/v2.0/swagger.yaml

@@ -5662,7 +5662,6 @@ paths:
       operationId: getScanDataExportExecutionList
       parameters:
         - $ref: '#/parameters/requestId'
-        - $ref: '#/parameters/userName'
       responses:
         '200':
           description: Success

src/controller/scandataexport/execution.go

@@ -188,6 +188,8 @@ func (c *controller) isCsvArtifactPresent(ctx context.Context, execID int64, dig
 	repositoryName := fmt.Sprintf("scandata_export_%v", execID)
 	exists, err := c.sysArtifactMgr.Exists(ctx, strings.ToLower(export.Vendor), repositoryName, digest)
 	if err != nil {
+		logger.Errorf("failed to check existence of csv artifact for vendor: %s repository: %s digest: %s",
+			strings.ToLower(export.Vendor), repositoryName, digest)
 		exists = false
 	}
 	return exists

src/pkg/scan/export/filter_processor.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"github.com/goharbor/harbor/src/pkg"
 
-	"github.com/goharbor/harbor/src/common"
 	commonmodels "github.com/goharbor/harbor/src/common/models"
 	"github.com/goharbor/harbor/src/common/security/local"
 	"github.com/goharbor/harbor/src/common/utils"
@@ -137,6 +136,7 @@ func (dfp *DefaultFilterProcessor) getProjectQueryFilter(user *commonmodels.User
 		logger.Infof("User %v is sys admin. Selecting all projects for export.", user.Username)
 		return q.New(q.KeyWords{})
 	}
+
 	logger.Infof("User %v is not sys admin. Selecting projects with admin roles for export.", user.Username)
-	return q.New(q.KeyWords{"member": &models.MemberQuery{UserID: user.UserID, Role: common.RoleProjectAdmin}})
+	return q.New(q.KeyWords{"member": &models.MemberQuery{UserID: user.UserID, GroupIDs: user.GroupIDs}})
 }

src/pkg/scan/export/filter_processor_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	commonmodels "github.com/goharbor/harbor/src/common/models"
+	project3 "github.com/goharbor/harbor/src/controller/project"
 	"github.com/goharbor/harbor/src/lib/q"
 	"github.com/goharbor/harbor/src/pkg/project/models"
 	"github.com/goharbor/harbor/src/pkg/repository/model"
@@ -15,6 +16,7 @@ import (
 	"github.com/goharbor/harbor/src/testing/pkg/user"
 	testifymock "github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/suite"
+	"reflect"
 	"testing"
 	"time"
 )
@@ -69,6 +71,22 @@ func (suite *FilterProcessorTestSuite) TestProcessProjectFilter() {
 		suite.NoError(err)
 	}
 
+	// filtered project with group ids
+	{
+		groupIDs := []int{4, 5}
+		suite.usrMgr.On("GetByName", mock.Anything, "test-user").Return(&commonmodels.User{UserID: 1, GroupIDs: groupIDs}, nil).Once()
+		suite.projectMgr.On("List", mock.Anything, mock.Anything).Return([]*models.Project{project1, project2}, nil).Once()
+		projectIds, err := suite.filterProcessor.ProcessProjectFilter(context.TODO(), "test-user", []int64{1})
+		suite.Equal(1, len(projectIds))
+		suite.Equal(int64(1), projectIds[0])
+		suite.NoError(err)
+		memberQueryMatcher := testifymock.MatchedBy(func(query *q.Query) bool {
+			memberQuery := query.Keywords["member"].(*project3.MemberQuery)
+			return len(memberQuery.GroupIDs) == 2 && reflect.DeepEqual(memberQuery.GroupIDs, groupIDs) && memberQuery.Role == 0
+		})
+		suite.projectMgr.AssertCalled(suite.T(), "List", mock.Anything, memberQueryMatcher)
+	}
+
 	// project listing for admin user
 	{
 		suite.usrMgr.On("GetByName", mock.Anything, "test-user").Return(&commonmodels.User{UserID: 1, SysAdminFlag: true}, nil).Once()

src/portal/src/app/shared/components/operation/operation.component.ts

@@ -270,9 +270,7 @@ export class OperationComponent implements OnInit, OnDestroy {
     refreshExportJobs() {
         if (this.session.getCurrentUser()) {
             this.scanDataExportService
-                .getScanDataExportExecutionList({
+                .getScanDataExportExecutionList()
-                    userName: this.session?.getCurrentUser()?.username,
-                })
                 .subscribe(res => {
                     if (res?.items) {
                         this.exportJobs = [];

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

@@ -131,8 +131,21 @@ func (se *scanDataExportAPI) DownloadScanData(ctx context.Context, params operat
 		return se.SendError(ctx, err)
 	}
 	execution, err := se.scanDataExportCtl.GetExecution(ctx, params.ExecutionID)
+
 	if err != nil {
-		se.SendError(ctx, err)
+		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) {
+				writer.WriteHeader(http.StatusNotFound)
+			})
+		}
+		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) {
+			writer.WriteHeader(http.StatusNotFound)
+		})
 	}
 
 	// check if the execution being downloaded is owned by the current user
@@ -143,7 +156,7 @@ func (se *scanDataExportAPI) DownloadScanData(ctx context.Context, params operat
 
 	if secContext.GetUsername() != execution.UserName {
 		return middleware.ResponderFunc(func(writer http.ResponseWriter, producer runtime.Producer) {
-			writer.WriteHeader(http.StatusUnauthorized)
+			writer.WriteHeader(http.StatusForbidden)
 		})
 	}
 
@@ -173,7 +186,12 @@ func (se *scanDataExportAPI) GetScanDataExportExecutionList(ctx context.Context,
 	if err != nil {
 		return se.SendError(ctx, err)
 	}
-	executions, err := se.scanDataExportCtl.ListExecutions(ctx, params.UserName)
+	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)
 	}

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

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	beegoorm "github.com/beego/beego/orm"
 	commonmodels "github.com/goharbor/harbor/src/common/models"
 	"github.com/goharbor/harbor/src/pkg/scan/export"
 	v1 "github.com/goharbor/harbor/src/pkg/scan/rest/v1"
@@ -299,6 +300,7 @@ func (suite *ScanExportTestSuite) TestDownloadScanData() {
 		EndTime:          endTime,
 		ExportDataDigest: "datadigest",
 		UserName:         "test-user",
+		FilePresent:      true,
 	}
 	mock.OnAnything(suite.scanExportCtl, "GetExecution").Return(execution, nil)
 	mock.OnAnything(suite.scanExportCtl, "DeleteExecution").Return(nil)
@@ -340,6 +342,7 @@ func (suite *ScanExportTestSuite) TestDownloadScanDataUserNotOwnerofExport() {
 		EndTime:          endTime,
 		ExportDataDigest: "datadigest",
 		UserName:         "test-user",
+		FilePresent:      true,
 	}
 	mock.OnAnything(suite.scanExportCtl, "GetExecution").Return(execution, nil)
 	mock.OnAnything(suite.scanExportCtl, "DeleteExecution").Return(nil)
@@ -353,12 +356,92 @@ func (suite *ScanExportTestSuite) TestDownloadScanDataUserNotOwnerofExport() {
 	mock.OnAnything(suite.sysArtifactMgr, "Delete").Return(nil)
 
 	res, err := suite.DoReq(http.MethodGet, url, nil)
-	suite.Equal(http.StatusUnauthorized, res.StatusCode)
+	suite.Equal(http.StatusForbidden, res.StatusCode)
+	suite.Equal(nil, err)
+}
+
+func (suite *ScanExportTestSuite) TestDownloadScanDataNoCsvFilePresent() {
+	suite.Security.On("GetUsername").Return("test-user1")
+	suite.Security.On("IsAuthenticated").Return(true).Once()
+	suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Times(1)
+	url := "/export/cve/download/100"
+	endTime := time.Now()
+	startTime := endTime.Add(-10 * time.Minute)
+
+	execution := &export.Execution{
+		ID:               int64(100),
+		UserID:           int64(3),
+		Status:           "Success",
+		StatusMessage:    "",
+		Trigger:          "MANUAL",
+		StartTime:        startTime,
+		EndTime:          endTime,
+		ExportDataDigest: "datadigest",
+		UserName:         "test-user",
+		FilePresent:      false,
+	}
+	mock.OnAnything(suite.scanExportCtl, "GetExecution").Return(execution, nil)
+	mock.OnAnything(suite.scanExportCtl, "DeleteExecution").Return(nil)
+
+	// all BLOB related operations succeed
+	mock.OnAnything(suite.sysArtifactMgr, "Create").Return(int64(1), nil)
+
+	sampleData := "test,hello,world"
+	data := io.NopCloser(strings.NewReader(sampleData))
+	mock.OnAnything(suite.sysArtifactMgr, "Read").Return(data, nil)
+	mock.OnAnything(suite.sysArtifactMgr, "Delete").Return(nil)
+
+	res, err := suite.DoReq(http.MethodGet, url, nil)
+	suite.Equal(http.StatusNotFound, res.StatusCode)
+	suite.Equal(nil, err)
+}
+
+func (suite *ScanExportTestSuite) TestDownloadScanDataExecutionNotPresent() {
+	suite.Security.On("GetUsername").Return("test-user1")
+	suite.Security.On("IsAuthenticated").Return(true).Once()
+	suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Times(1)
+	url := "/export/cve/download/100"
+
+	mock.OnAnything(suite.scanExportCtl, "GetExecution").Return(nil, beegoorm.ErrNoRows)
+	mock.OnAnything(suite.scanExportCtl, "DeleteExecution").Return(nil)
+
+	// all BLOB related operations succeed
+	mock.OnAnything(suite.sysArtifactMgr, "Create").Return(int64(1), nil)
+
+	sampleData := "test,hello,world"
+	data := io.NopCloser(strings.NewReader(sampleData))
+	mock.OnAnything(suite.sysArtifactMgr, "Read").Return(data, nil)
+	mock.OnAnything(suite.sysArtifactMgr, "Delete").Return(nil)
+
+	res, err := suite.DoReq(http.MethodGet, url, nil)
+	suite.Equal(http.StatusNotFound, res.StatusCode)
+	suite.Equal(nil, err)
+}
+
+func (suite *ScanExportTestSuite) TestDownloadScanDataExecutionError() {
+	suite.Security.On("GetUsername").Return("test-user1")
+	suite.Security.On("IsAuthenticated").Return(true).Once()
+	suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Times(1)
+	url := "/export/cve/download/100"
+
+	mock.OnAnything(suite.scanExportCtl, "GetExecution").Return(nil, errors.New("test error"))
+	mock.OnAnything(suite.scanExportCtl, "DeleteExecution").Return(nil)
+
+	// all BLOB related operations succeed
+	mock.OnAnything(suite.sysArtifactMgr, "Create").Return(int64(1), nil)
+
+	sampleData := "test,hello,world"
+	data := io.NopCloser(strings.NewReader(sampleData))
+	mock.OnAnything(suite.sysArtifactMgr, "Read").Return(data, nil)
+	mock.OnAnything(suite.sysArtifactMgr, "Delete").Return(nil)
+
+	res, err := suite.DoReq(http.MethodGet, url, nil)
+	suite.Equal(http.StatusInternalServerError, res.StatusCode)
 	suite.Equal(nil, err)
 }
 
 func (suite *ScanExportTestSuite) TestGetScanDataExportExecutionList() {
-
+	suite.Security.On("GetUsername").Return("test-user")
 	suite.Security.On("IsAuthenticated").Return(true).Once()
 	suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Once()
 	url, err := url2.Parse("/export/cve/executions")