code review - simplifies state management, adds some tests

Author: timbastin

internal/core/assetversion/asset_version_service.go

@@ -183,7 +183,7 @@ func (s *service) HandleScanResult(asset models.Asset, assetVersion *models.Asse
 			Vulnerability: models.Vulnerability{
 				AssetVersionName: assetVersion.Name,
 				AssetID:          asset.ID,
-				ScannerIDs:       scannerID + " ",
+				ScannerIDs:       scannerID,
 			},
 			CVEID:                 utils.Ptr(v.CVEID),
 			ComponentPurl:         utils.Ptr(v.Purl),
@@ -209,7 +209,6 @@ func (s *service) HandleScanResult(asset models.Asset, assetVersion *models.Asse
 	devguardScanner := "github.com/l3montree-dev/devguard/cmd/devguard-scanner" + "/"
 
 	switch scannerID {
-
 	case devguardScanner + "sca":
 		assetVersion.LastScaScan = utils.Ptr(time.Now())
 	case devguardScanner + "container-scanning":
@@ -219,12 +218,43 @@ func (s *service) HandleScanResult(asset models.Asset, assetVersion *models.Asse
 	return amountOpened, amountClosed, amountExisting, nil
 }
 
+func diffScanResults(currentScanner string, foundVulnerabilities []models.DependencyVuln, existingDependencyVulns []models.DependencyVuln) ([]models.DependencyVuln, []models.DependencyVuln, []models.DependencyVuln, []models.DependencyVuln) {
+	comparison := utils.CompareSlices(existingDependencyVulns, foundVulnerabilities, func(dependencyVuln models.DependencyVuln) string {
+		return dependencyVuln.CalculateHash()
+	})
+
+	foundByScannerAndNotExisting := comparison.OnlyInB //We want to create new vulnerabilities for these
+	foundByScannerAndExisting := comparison.InBoth     //We have to check if it was already found by this scanner or only by other scanners
+	notFoundByScannerAndExisting := comparison.OnlyInA //We have to update all vulnerabilities which were previously found by this scanner and now aren't
+
+	var detectedByOtherScanner []models.DependencyVuln
+	var notDetectedByScannerAnymore []models.DependencyVuln
+
+	var fixedVulns []models.DependencyVuln //We should collect all vulnerabilities we want to fix so we can do it all at once
+
+	// Now we work on the vulnerabilities found in both sets -> has the vulnerability this scanner id already in his scanner_ids
+	for i := range foundByScannerAndExisting {
+		if !strings.Contains(foundByScannerAndExisting[i].ScannerIDs, currentScanner) {
+			detectedByOtherScanner = append(detectedByOtherScanner, foundByScannerAndExisting[i])
+		}
+	}
+
+	// Last we have to change the already existing vulnerabilities which were not found this time
+	for i := range notFoundByScannerAndExisting {
+		if strings.TrimSpace(notFoundByScannerAndExisting[i].ScannerIDs) == currentScanner {
+			fixedVulns = append(fixedVulns, notFoundByScannerAndExisting[i])
+		} else if strings.Contains(notFoundByScannerAndExisting[i].ScannerIDs, currentScanner) {
+			notDetectedByScannerAnymore = append(notDetectedByScannerAnymore, notFoundByScannerAndExisting[i])
+		}
+	}
+
+	return foundByScannerAndNotExisting, fixedVulns, detectedByOtherScanner, notDetectedByScannerAnymore
+}
+
 func (s *service) handleScanResult(userID string, scannerID string, assetVersion *models.AssetVersion, dependencyVulns []models.DependencyVuln, doRiskManagement bool, asset models.Asset) (int, int, []models.DependencyVuln, error) {
 	// get all existing dependencyVulns from the database - this is the old state
-
 	//number := rand.IntN(len(dependencyVulns))
 	//dependencyVulns = dependencyVulns[:0]
-	scannerID = scannerID + " "
 	existingDependencyVulns, err := s.dependencyVulnRepository.ListByAssetAndAssetVersion(assetVersion.Name, assetVersion.AssetID)
 	if err != nil {
 		slog.Error("could not get existing dependencyVulns", "err", err)
@@ -236,84 +266,36 @@ func (s *service) handleScanResult(userID string, scannerID string, assetVersion
 		return dependencyVuln.State != models.VulnStateFixed
 	})
 
-	comparison := utils.CompareSlices(existingDependencyVulns, dependencyVulns, func(dependencyVuln models.DependencyVuln) string {
-		return dependencyVuln.CalculateHash()
-	})
-
-	foundByScannerAndNotExisting := comparison.OnlyInB //We want to create new vulnerabilities for these
-	foundByScannerAndExisting := comparison.InBoth     //We have to check if it was already found by this scanner or only by other scanners
-	notFoundByScannerAndExisting := comparison.OnlyInA //We have to update all vulnerabilities which were previously found by this scanner and now aren't
+	newDetectedVulns, fixedVulns, firstTimeDetectedByCurrentScanner, notDetectedByCurrentScannerAnymore := diffScanResults(scannerID, dependencyVulns, existingDependencyVulns)
+	amountFixed := len(utils.Filter(fixedVulns, func(dependencyVuln models.DependencyVuln) bool {
+		return dependencyVuln.State == models.VulnStateOpen
+	}))
 
-	var vulnerabilitiesToFix []models.DependencyVuln    //We should collect all vulnerabilities we want to fix so we can do it all at once
-	var vulnerabilitiesToUpdate []models.DependencyVuln //We should do the same
-	// get a transaction
 	if err := s.dependencyVulnRepository.Transaction(func(tx core.DB) error {
 		// We can create the newly found one without checking anything
-		if err := s.dependencyVulnService.UserDetectedDependencyVulns(tx, userID, foundByScannerAndNotExisting, *assetVersion, asset, true); err != nil {
+		if err := s.dependencyVulnService.UserDetectedDependencyVulns(tx, userID, newDetectedVulns, *assetVersion, asset, true); err != nil {
 			return err // this will cancel the transaction
 		}
 
-		// Now we work on the vulnerabilities found in both sets -> has the vulnerability this scanner id already in his scanner_ids
-		for i := range foundByScannerAndExisting {
-			if !strings.Contains(foundByScannerAndExisting[i].ScannerIDs, scannerID) {
-				foundByScannerAndExisting[i].ScannerIDs = foundByScannerAndExisting[i].ScannerIDs + " " + scannerID
-				vulnerabilitiesToUpdate = append(vulnerabilitiesToUpdate, foundByScannerAndExisting[i])
-			}
-		}
-		err = s.dependencyVulnRepository.SaveBatch(tx, foundByScannerAndExisting)
-		if err != nil {
-			slog.Error("error when trying to update vulnerabilities")
-			return err
-		}
-
-		err = s.dependencyVulnService.MakeAddedScannerEvent(tx, vulnerabilitiesToUpdate, userID)
+		err = s.dependencyVulnService.UserDetectedDependencyVulnWithAnotherScanner(tx, firstTimeDetectedByCurrentScanner, userID, scannerID)
 		if err != nil {
 			slog.Error("error when trying to add events for adding scanner to vulnerability")
 			return err
 		}
 
-		vulnerabilitiesToUpdate = nil
-
-		//Last we have to change the already existing vulnerabilities which were not found this time
-		for i := range notFoundByScannerAndExisting {
-			if notFoundByScannerAndExisting[i].ScannerIDs == scannerID {
-				notFoundByScannerAndExisting[i].ScannerIDs = ""
-				vulnerabilitiesToFix = append(vulnerabilitiesToFix, notFoundByScannerAndExisting[i])
-			} else if strings.Contains(notFoundByScannerAndExisting[i].ScannerIDs, scannerID) {
-				removeScannerFromVulnerability(&notFoundByScannerAndExisting[i], scannerID)
-				vulnerabilitiesToUpdate = append(vulnerabilitiesToUpdate, notFoundByScannerAndExisting[i])
-			}
-		}
-
-		err = s.dependencyVulnRepository.SaveBatch(tx, vulnerabilitiesToUpdate)
-		if err != nil {
-			slog.Error("error when trying to update vulnerabilities")
-			return err
-		}
-
-		err := s.dependencyVulnService.MakeRemoveScannerEvent(tx, vulnerabilitiesToUpdate, userID)
+		err := s.dependencyVulnService.UserDidNotDetectDependencyVulnWithScannerAnymore(tx, notDetectedByCurrentScannerAnymore, userID, scannerID)
 		if err != nil {
 			slog.Error("error when trying to add events for removing scanner from vulnerability")
 			return err
 		}
 
-		return s.dependencyVulnService.UserFixedDependencyVulns(tx, userID, vulnerabilitiesToFix, *assetVersion, asset, true)
+		return s.dependencyVulnService.UserFixedDependencyVulns(tx, userID, fixedVulns, *assetVersion, asset, true)
 	}); err != nil {
 		slog.Error("could not save dependencyVulns", "err", err)
 		return 0, 0, []models.DependencyVuln{}, err
 	}
 
-	// the amount we actually fixed, is the amount that was open before
-	vulnerabilitiesToFix = utils.Filter(vulnerabilitiesToFix, func(dependencyVuln models.DependencyVuln) bool {
-		return dependencyVuln.State == models.VulnStateOpen
-	})
-	return len(foundByScannerAndNotExisting /* maybe also return vulns newly found by this scanner*/), len(vulnerabilitiesToFix), append(foundByScannerAndNotExisting, comparison.InBoth...), nil
-}
-
-// pass by reference to edit the actual vulnerability and not a copy
-func removeScannerFromVulnerability(vulnerability *models.DependencyVuln, scannerID string) {
-
-	vulnerability.ScannerIDs = strings.Replace(vulnerability.ScannerIDs, scannerID, "", 1)
+	return len(newDetectedVulns) + len(firstTimeDetectedByCurrentScanner), amountFixed, append(newDetectedVulns, firstTimeDetectedByCurrentScanner...), nil
 }
 
 func recursiveBuildBomRefMap(component cdx.Component) map[string]cdx.Component {
@@ -348,7 +330,6 @@ func buildBomRefMap(bom normalize.SBOM) map[string]cdx.Component {
 
 func (s *service) UpdateSBOM(assetVersion models.AssetVersion, scannerID string, sbom normalize.SBOM) error {
 	// load the asset components
-
 	assetComponents, err := s.componentRepository.LoadComponents(nil, assetVersion.Name, assetVersion.AssetID, "")
 	if err != nil {
 		return errors.Wrap(err, "could not load asset components")
@@ -385,7 +366,7 @@ func (s *service) UpdateSBOM(assetVersion models.AssetVersion, scannerID string,
 			dependencies = append(dependencies,
 				models.ComponentDependency{
 					ComponentPurl:  nil, // direct dependency - therefore set it to nil
-					ScannerIDs:     scannerID + " ",
+					ScannerIDs:     scannerID,
 					DependencyPurl: componentPackageUrl,
 				},
 			)
@@ -411,7 +392,7 @@ func (s *service) UpdateSBOM(assetVersion models.AssetVersion, scannerID string,
 			dependencies = append(dependencies,
 				models.ComponentDependency{
 					ComponentPurl:  utils.EmptyThenNil(compPackageUrl),
-					ScannerIDs:     scannerID + " ",
+					ScannerIDs:     scannerID,
 					DependencyPurl: depPurlOrName,
 				},
 			)

internal/core/assetversion/asset_version_service_test.go

@@ -0,0 +1,83 @@
+package assetversion
+
+import (
+	"testing"
+
+	"github.com/l3montree-dev/devguard/internal/database/models"
+	"github.com/l3montree-dev/devguard/internal/utils"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDiffScanResults(t *testing.T) {
+
+	t.Run("should correctly identify a vulnerability which now gets found by another scanner", func(t *testing.T) {
+		currentScanner := "new-scanner"
+
+		foundVulnerabilities := []models.DependencyVuln{
+			{CVEID: utils.Ptr("CVE-1234")},
+		}
+
+		existingDependencyVulns := []models.DependencyVuln{
+			{CVEID: utils.Ptr("CVE-1234"), Vulnerability: models.Vulnerability{ScannerIDs: "scanner-1"}},
+		}
+
+		foundByScannerAndNotExisting, fixedVulns, detectedByCurrentScanner, notDetectedByCurrentScannerAnymore := diffScanResults(currentScanner, foundVulnerabilities, existingDependencyVulns)
+
+		assert.Empty(t, foundByScannerAndNotExisting)
+		assert.Empty(t, fixedVulns)
+		assert.Empty(t, notDetectedByCurrentScannerAnymore)
+		assert.Equal(t, 1, len(detectedByCurrentScanner))
+	})
+
+	t.Run("should correctly identify a vulnerability which now is fixed, since it was not found by the scanner anymore", func(t *testing.T) {
+		currentScanner := "new-scanner"
+
+		foundVulnerabilities := []models.DependencyVuln{}
+
+		existingDependencyVulns := []models.DependencyVuln{
+			{CVEID: utils.Ptr("CVE-1234"), Vulnerability: models.Vulnerability{ScannerIDs: currentScanner}},
+		}
+
+		foundByScannerAndNotExisting, fixedVulns, detectedByCurrentScanner, notDetectedByCurrentScannerAnymore := diffScanResults(currentScanner, foundVulnerabilities, existingDependencyVulns)
+
+		assert.Empty(t, foundByScannerAndNotExisting)
+		assert.Equal(t, 1, len(fixedVulns))
+		assert.Empty(t, detectedByCurrentScanner)
+		assert.Empty(t, notDetectedByCurrentScannerAnymore)
+	})
+
+	t.Run("should correctly identify a vulnerability which is not detected by the current scanner anymore", func(t *testing.T) {
+		currentScanner := "new-scanner"
+
+		foundVulnerabilities := []models.DependencyVuln{}
+
+		existingDependencyVulns := []models.DependencyVuln{
+			{CVEID: utils.Ptr("CVE-1234"), Vulnerability: models.Vulnerability{ScannerIDs: currentScanner + " scanner-1"}},
+		}
+
+		foundByScannerAndNotExisting, fixedVulns, detectedByCurrentScanner, notDetectedByCurrentScannerAnymore := diffScanResults(currentScanner, foundVulnerabilities, existingDependencyVulns)
+
+		assert.Empty(t, foundByScannerAndNotExisting)
+		assert.Empty(t, fixedVulns)
+		assert.Empty(t, detectedByCurrentScanner)
+		assert.Equal(t, 1, len(notDetectedByCurrentScannerAnymore))
+	})
+
+	t.Run("should identify new vulnerabilities", func(t *testing.T) {
+		currentScanner := "new-scanner"
+
+		foundVulnerabilities := []models.DependencyVuln{
+			{CVEID: utils.Ptr("CVE-1234")},
+			{CVEID: utils.Ptr("CVE-5678")},
+		}
+
+		existingDependencyVulns := []models.DependencyVuln{}
+
+		foundByScannerAndNotExisting, fixedVulns, detectedByCurrentScanner, notDetectedByCurrentScannerAnymore := diffScanResults(currentScanner, foundVulnerabilities, existingDependencyVulns)
+
+		assert.Equal(t, 2, len(foundByScannerAndNotExisting))
+		assert.Empty(t, fixedVulns)
+		assert.Empty(t, detectedByCurrentScanner)
+		assert.Empty(t, notDetectedByCurrentScannerAnymore)
+	})
+}

internal/core/common_interfaces.go

@@ -183,8 +183,8 @@ type DependencyVulnService interface {
 	RecalculateRawRiskAssessment(tx DB, responsible string, dependencyVulns []models.DependencyVuln, justification string, asset models.Asset) error
 	UserFixedDependencyVulns(tx DB, userID string, dependencyVulns []models.DependencyVuln, assetVersion models.AssetVersion, asset models.Asset, doRiskManagement bool) error
 	UserDetectedDependencyVulns(tx DB, userID string, dependencyVulns []models.DependencyVuln, assetVersion models.AssetVersion, asset models.Asset, doRiskManagement bool) error
-	MakeAddedScannerEvent(tx DB, vulnerabilities []models.DependencyVuln, userID string) error
-	MakeRemoveScannerEvent(tx DB, vulnerabilities []models.DependencyVuln, userID string) error
+	UserDetectedDependencyVulnWithAnotherScanner(tx DB, vulnerabilities []models.DependencyVuln, userID string, scannerID string) error
+	UserDidNotDetectDependencyVulnWithScannerAnymore(tx DB, vulnerabilities []models.DependencyVuln, userID string, scannerID string) error
 	UpdateDependencyVulnState(tx DB, assetID uuid.UUID, userID string, dependencyVuln *models.DependencyVuln, statusType string, justification string, assetVersionName string) (models.VulnEvent, error)
 	CreateIssuesForVulns(asset models.Asset, vulnList []models.DependencyVuln) error
 	ShouldCreateIssue(assetVersion models.AssetVersion) bool

internal/core/component/component_dto.go

@@ -14,7 +14,7 @@ type componentDTO struct {
 	Dependency     models.Component `json:"dependency"`
 	DependencyPurl string           `json:"dependencyPurl"` // will be nil, for direct dependencies
 	AssetID        uuid.UUID        `json:"assetVersionId"`
-	ScannerIDs     string           `json:"scanner"` // the id of the scanner
+	ScannerIDs     string           `json:"scannerIds"` // the id of the scanner
 }
 
 func toDTO(m models.ComponentDependency) componentDTO {

internal/core/dependency_vuln/dependency_vuln_controller.go

@@ -226,7 +226,6 @@ func (c dependencyVulnHttpController) Read(ctx core.Context) error {
 }
 
 func (c dependencyVulnHttpController) CreateEvent(ctx core.Context) error {
-
 	asset := core.GetAsset(ctx)
 	assetVersion := core.GetAssetVersion(ctx)
 	thirdPartyIntegration := core.GetThirdPartyIntegration(ctx)

internal/core/dependency_vuln/dependency_vuln_service.go

@@ -153,26 +153,46 @@ func (s *service) RecalculateAllRawRiskAssessments() error {
 
 }
 
-func (s *service) MakeAddedScannerEvent(tx core.DB, vulnerabilities []models.DependencyVuln, userID string) error {
+func (s *service) UserDetectedDependencyVulnWithAnotherScanner(tx core.DB, vulnerabilities []models.DependencyVuln, userID string, scannerID string) error {
+	if len(vulnerabilities) == 0 {
+		return nil
+	}
+
+	// create a new VulnEvent for each fixed dependencyVuln
 	events := make([]models.VulnEvent, len(vulnerabilities))
+
 	for i := range vulnerabilities {
-		ev := models.NewAddedScannerEvent(vulnerabilities[i].CalculateHash(), userID)
+		ev := models.NewAddedScannerEvent(vulnerabilities[i].CalculateHash(), userID, scannerID)
 		ev.Apply(&vulnerabilities[i])
 		events[i] = ev
 	}
+
+	err := s.dependencyVulnRepository.SaveBatch(tx, vulnerabilities)
+	if err != nil {
+		return err
+	}
+
 	return s.vulnEventRepository.SaveBatch(tx, events)
 
 }
 
-func (s *service) MakeRemoveScannerEvent(tx core.DB, vulnerabilities []models.DependencyVuln, userID string) error {
+func (s *service) UserDidNotDetectDependencyVulnWithScannerAnymore(tx core.DB, vulnerabilities []models.DependencyVuln, userID string, scannerID string) error {
+	if len(vulnerabilities) == 0 {
+		return nil
+	}
+
 	events := make([]models.VulnEvent, len(vulnerabilities))
 	for i := range vulnerabilities {
-		ev := models.NewRemovedScannerEvent(vulnerabilities[i].CalculateHash(), userID)
+		ev := models.NewRemovedScannerEvent(vulnerabilities[i].CalculateHash(), userID, scannerID)
 		ev.Apply(&vulnerabilities[i])
 		events[i] = ev
 	}
+	err := s.dependencyVulnRepository.SaveBatch(tx, vulnerabilities)
+	if err != nil {
+		return err
+	}
+	// save the events
 	return s.vulnEventRepository.SaveBatch(tx, events)
-
 }
 
 func (s *service) RecalculateRawRiskAssessment(tx core.DB, userID string, dependencyVulns []models.DependencyVuln, justification string, asset models.Asset) error {
@@ -281,7 +301,6 @@ func (s *service) UpdateDependencyVulnState(tx core.DB, assetID uuid.UUID, userI
 
 func (s *service) updateDependencyVulnState(tx core.DB, userID string, dependencyVuln *models.DependencyVuln, statusType string, justification string) (models.VulnEvent, error) {
 	var ev models.VulnEvent
-	fmt.Printf("Called 'ApplyAndSave' with vuln: %s", *dependencyVuln.CVEID)
 	switch models.VulnEventType(statusType) {
 	case models.EventTypeAccepted:
 		ev = models.NewAcceptedEvent(dependencyVuln.CalculateHash(), userID, justification)
@@ -292,7 +311,6 @@ func (s *service) updateDependencyVulnState(tx core.DB, userID string, dependenc
 	case models.EventTypeComment:
 		ev = models.NewCommentEvent(dependencyVuln.CalculateHash(), userID, justification)
 	}
-	//Found by toher scanner
 
 	err := s.dependencyVulnRepository.ApplyAndSave(tx, dependencyVuln, &ev)
 	return ev, err

internal/core/risk/risk_explaination.go

@@ -222,7 +222,7 @@ type Explanation struct {
 	cveDescription string
 
 	affectedComponentName string
-	scannerID             string
+	scannerIDs            string
 	fixedVersion          *string
 }
 
@@ -232,7 +232,7 @@ func (e Explanation) Markdown(baseUrl, orgSlug, projectSlug, assetSlug, assetVer
 	str.WriteString(e.cveDescription)
 	str.WriteString("\n")
 	str.WriteString("### Affected component \n")
-	str.WriteString(fmt.Sprintf("The vulnerability is in `%s`, detected by the `%s` scan.\n", e.affectedComponentName, e.scannerID))
+	str.WriteString(fmt.Sprintf("The vulnerability is in `%s`, detected by the `%s` scan.\n", e.affectedComponentName, e.scannerIDs))
 	str.WriteString("### Recommended fix\n")
 	if e.fixedVersion != nil {
 		str.WriteString(fmt.Sprintf("Upgrade to version %s or later.\n", *e.fixedVersion))
@@ -300,7 +300,7 @@ func Explain(dependencyVuln models.DependencyVuln, asset models.Asset, vector st
 		cveDescription: dependencyVuln.CVE.Description,
 
 		affectedComponentName: utils.SafeDereference(dependencyVuln.ComponentPurl),
-		scannerID:             dependencyVuln.ScannerIDs,
+		scannerIDs:            dependencyVuln.ScannerIDs,
 		fixedVersion:          dependencyVuln.ComponentFixedVersion,
 	}
 }

internal/database/models/dependency_vuln_model.go

@@ -67,7 +67,7 @@ func (m DependencyVuln) TableName() string {
 }
 
 func (m *DependencyVuln) CalculateHash() string {
-	hash := utils.HashString(fmt.Sprintf("%s/%s/%s/%s", *m.CVEID, *m.ComponentPurl, m.AssetVersionName, m.AssetID))
+	hash := utils.HashString(fmt.Sprintf("%s/%s/%s/%s", utils.OrDefault(m.CVEID, ""), utils.OrDefault(m.ComponentPurl, ""), m.AssetVersionName, m.AssetID))
 	return hash
 }