⚠️ Delegate logging decisions to the checks instead of a helper (#4019)

* add func to log single finding at caller specified level. This may not be the final form, we may support want to support passing a map of probe+outcome to level mappings. Signed-off-by: Spencer Schrock <[email protected]> * switch dependency update tool check off LogFindings For now, we only use one probe so logging is simple. Signed-off-by: Spencer Schrock <[email protected]> * switch fuzzing check off LogFindings Signed-off-by: Spencer Schrock <[email protected]> * switch packaging check off LogFindings Signed-off-by: Spencer Schrock <[email protected]> * switch security policy check off LogFindings This changes the logging of an error state, but it's not one we expect to see. Signed-off-by: Spencer Schrock <[email protected]> * switch signed releases off LogFindings Signed-off-by: Spencer Schrock <[email protected]> * switch license check off LogFindings Signed-off-by: Spencer Schrock <[email protected]> * switch vuln check off LogFindings Signed-off-by: Spencer Schrock <[email protected]> * switch maintained check off logfindings and delete it Signed-off-by: Spencer Schrock <[email protected]> * dont log lack of commit or issue activity scdiff caught a lot of new details being generated. So going to try removing them Signed-off-by: Spencer Schrock <[email protected]> --------- Signed-off-by: Spencer Schrock <[email protected]>
ossf · Apr 9, 2024 · a220b48 · a220b48
1 parent 775fc97
commit a220b48
Show file tree

Hide file tree

Showing 12 changed files with 118 additions and 137 deletions.
diff --git a/checker/check_result.go b/checker/check_result.go
@@ -243,23 +243,15 @@ func CreateRuntimeErrorResult(name string, e error) CheckResult {
 	}
 }
 
-// LogFindings logs the list of findings.
-func LogFindings(findings []finding.Finding, dl DetailLogger) {
-	for i := range findings {
-		f := &findings[i]
-		switch f.Outcome {
-		case finding.OutcomeFalse:
-			dl.Warn(&LogMessage{
-				Finding: f,
-			})
-		case finding.OutcomeTrue:
-			dl.Info(&LogMessage{
-				Finding: f,
-			})
-		default:
-			dl.Debug(&LogMessage{
-				Finding: f,
-			})
-		}
+// LogFinding logs the given finding at the given level.
+func LogFinding(dl DetailLogger, f *finding.Finding, level DetailType) {
+	lm := LogMessage{Finding: f}
+	switch level {
+	case DetailDebug:
+		dl.Debug(&lm)
+	case DetailInfo:
+		dl.Info(&lm)
+	case DetailWarn:
+		dl.Warn(&lm)
 	}
 }
diff --git a/checks/evaluation/dependency_update_tool.go b/checks/evaluation/dependency_update_tool.go
@@ -34,16 +34,24 @@ func DependencyUpdateTool(name string,
 		return checker.CreateRuntimeErrorResult(name, e)
 	}
 
+	var usesTool bool
 	for i := range findings {
 		f := &findings[i]
-		if f.Outcome == finding.OutcomeTrue {
-			// Log all findings except the false ones.
-			checker.LogFindings(nonFalseFindings(findings), dl)
-			return checker.CreateMaxScoreResult(name, "update tool detected")
+		var logLevel checker.DetailType
+		switch f.Outcome {
+		case finding.OutcomeFalse:
+			logLevel = checker.DetailWarn
+		case finding.OutcomeTrue:
+			usesTool = true
+			logLevel = checker.DetailInfo
+		default:
+			logLevel = checker.DetailDebug
 		}
+		checker.LogFinding(dl, f, logLevel)
 	}
 
-	// Log all findings.
-	checker.LogFindings(findings, dl)
+	if usesTool {
+		return checker.CreateMaxScoreResult(name, "update tool detected")
+	}
 	return checker.CreateMinScoreResult(name, "no update tool detected")
 }
diff --git a/checks/evaluation/finding.go b/checks/evaluation/finding.go
diff --git a/checks/evaluation/fuzzing.go b/checks/evaluation/fuzzing.go
@@ -36,16 +36,25 @@ func Fuzzing(name string,
 		return checker.CreateRuntimeErrorResult(name, e)
 	}
 
+	var fuzzerDetected bool
 	// Compute the score.
 	for i := range findings {
 		f := &findings[i]
-		if f.Outcome == finding.OutcomeTrue {
-			// Log all findings except the false ones.
-			checker.LogFindings(nonFalseFindings(findings), dl)
-			return checker.CreateMaxScoreResult(name, "project is fuzzed")
+		var logLevel checker.DetailType
+		switch f.Outcome {
+		case finding.OutcomeFalse:
+			logLevel = checker.DetailWarn
+		case finding.OutcomeTrue:
+			fuzzerDetected = true
+			logLevel = checker.DetailInfo
+		default:
+			logLevel = checker.DetailDebug
 		}
+		checker.LogFinding(dl, f, logLevel)
+	}
+
+	if fuzzerDetected {
+		return checker.CreateMaxScoreResult(name, "project is fuzzed")
 	}
-	// Log all findings.
-	checker.LogFindings(findings, dl)
 	return checker.CreateMinScoreResult(name, "project is not fuzzed")
 }
diff --git a/checks/evaluation/license.go b/checks/evaluation/license.go
@@ -27,7 +27,6 @@ func License(name string,
 	findings []finding.Finding,
 	dl checker.DetailLogger,
 ) checker.CheckResult {
-	// We have 3 unique probes, each should have a finding.
 	expectedProbes := []string{
 		hasLicenseFile.Probe,
 		hasFSFOrOSIApprovedLicense.Probe,
@@ -41,9 +40,12 @@ func License(name string,
 	// Compute the score.
 	score := 0
 	m := make(map[string]bool)
+	var logLevel checker.DetailType
 	for i := range findings {
 		f := &findings[i]
-		if f.Outcome == finding.OutcomeTrue {
+		switch f.Outcome {
+		case finding.OutcomeTrue:
+			logLevel = checker.DetailInfo
 			switch f.Probe {
 			case hasFSFOrOSIApprovedLicense.Probe:
 				score += scoreProbeOnce(f.Probe, m, 1)
@@ -53,10 +55,13 @@ func License(name string,
 				e := sce.WithMessage(sce.ErrScorecardInternal, "unknown probe results")
 				return checker.CreateRuntimeErrorResult(name, e)
 			}
+		case finding.OutcomeFalse:
+			logLevel = checker.DetailWarn
+		default:
+			logLevel = checker.DetailDebug
 		}
+		checker.LogFinding(dl, f, logLevel)
 	}
-	checker.LogFindings(findings, dl)
-
 	_, defined := m[hasLicenseFile.Probe]
 	if !defined {
 		if score > 0 {

diff --git a/checks/evaluation/maintained.go b/checks/evaluation/maintained.go
@@ -50,21 +50,15 @@ func Maintained(name string,
 		return checker.CreateRuntimeErrorResult(name, e)
 	}
 
-	if projectIsArchived(findings) {
-		checker.LogFindings(falseFindings(findings), dl)
-		return checker.CreateMinScoreResult(name, "project is archived")
-	}
-
-	if projectWasCreatedInLast90Days(findings) {
-		checker.LogFindings(falseFindings(findings), dl)
-		return checker.CreateMinScoreResult(name, "project was created in last 90 days. please review its contents carefully")
-	}
+	var archived, recentlyCreated bool
 
 	var commitsWithinThreshold, numberOfIssuesUpdatedWithinThreshold int
 	var err error
 	for i := range findings {
 		f := &findings[i]
-		if f.Outcome == finding.OutcomeTrue {
+		// currently this works but when we switch notArchived for #3654 this will need to also check the probe
+		switch f.Outcome {
+		case finding.OutcomeTrue:
 			switch f.Probe {
 			case issueActivityByProjectMember.Probe:
 				numberOfIssuesUpdatedWithinThreshold, err = strconv.Atoi(f.Values[issueActivityByProjectMember.NumIssuesKey])
@@ -77,31 +71,32 @@ func Maintained(name string,
 					return checker.CreateRuntimeErrorResult(name, sce.WithMessage(sce.ErrScorecardInternal, err.Error()))
 				}
 			}
+		case finding.OutcomeFalse:
+			switch f.Probe {
+			case notArchived.Probe:
+				archived = true
+			case notCreatedRecently.Probe:
+				recentlyCreated = true
+			// informational probes dont need logged, but they do factor into the score
+			case hasRecentCommits.Probe, issueActivityByProjectMember.Probe:
+				continue
+			}
+			checker.LogFinding(dl, f, checker.DetailWarn)
+		default:
+			checker.LogFinding(dl, f, checker.DetailDebug)
 		}
 	}
 
+	if archived {
+		return checker.CreateMinScoreResult(name, "project is archived")
+	}
+
+	if recentlyCreated {
+		return checker.CreateMinScoreResult(name, "project was created in last 90 days. please review its contents carefully")
+	}
+
 	return checker.CreateProportionalScoreResult(name, fmt.Sprintf(
 		"%d commit(s) and %d issue activity found in the last %d days",
 		commitsWithinThreshold, numberOfIssuesUpdatedWithinThreshold, lookBackDays),
 		commitsWithinThreshold+numberOfIssuesUpdatedWithinThreshold, activityPerWeek*lookBackDays/daysInOneWeek)
 }
-
-func projectIsArchived(findings []finding.Finding) bool {
-	for i := range findings {
-		f := &findings[i]
-		if f.Outcome == finding.OutcomeFalse && f.Probe == notArchived.Probe {
-			return true
-		}
-	}
-	return false
-}
-
-func projectWasCreatedInLast90Days(findings []finding.Finding) bool {
-	for i := range findings {
-		f := &findings[i]
-		if f.Outcome == finding.OutcomeFalse && f.Probe == notCreatedRecently.Probe {
-			return true
-		}
-	}
-	return false
-}
diff --git a/checks/evaluation/maintained_test.go b/checks/evaluation/maintained_test.go
@@ -121,7 +121,7 @@ func TestMaintained(t *testing.T) {
 			},
 			result: scut.TestReturn{
 				Score:        0,
-				NumberOfWarn: 3,
+				NumberOfWarn: 1,
 			},
 		},
 	}

diff --git a/checks/evaluation/packaging.go b/checks/evaluation/packaging.go
@@ -40,21 +40,22 @@ func Packaging(name string,
 	// we return max score if the outcome is true and lowest score if
 	// the outcome is false.
 	maxScore := false
-	for _, f := range findings {
-		f := f
-		if f.Outcome == finding.OutcomeTrue {
+	for i := range findings {
+		f := &findings[i]
+		var logLevel checker.DetailType
+		switch f.Outcome {
+		case finding.OutcomeFalse:
+			logLevel = checker.DetailWarn
+		case finding.OutcomeTrue:
 			maxScore = true
-			// Log all findings except the false ones.
-			dl.Info(&checker.LogMessage{
-				Finding: &f,
-			})
+			logLevel = checker.DetailInfo
+		default:
+			logLevel = checker.DetailDebug
 		}
+		checker.LogFinding(dl, f, logLevel)
 	}
 	if maxScore {
 		return checker.CreateMaxScoreResult(name, "packaging workflow detected")
 	}
-
-	checker.LogFindings(falseFindings(findings), dl)
-	return checker.CreateInconclusiveResult(name,
-		"packaging workflow not detected")
+	return checker.CreateInconclusiveResult(name, "packaging workflow not detected")
 }
diff --git a/checks/evaluation/security_policy.go b/checks/evaluation/security_policy.go
@@ -40,9 +40,13 @@ func SecurityPolicy(name string, findings []finding.Finding, dl checker.DetailLo
 
 	score := 0
 	m := make(map[string]bool)
+	var logLevel checker.DetailType
 	for i := range findings {
 		f := &findings[i]
-		if f.Outcome == finding.OutcomeTrue {
+		// all of the security policy probes are good things if true and bad if false
+		switch f.Outcome {
+		case finding.OutcomeTrue:
+			logLevel = checker.DetailInfo
 			switch f.Probe {
 			case securityPolicyContainsVulnerabilityDisclosure.Probe:
 				score += scoreProbeOnce(f.Probe, m, 1)
@@ -56,26 +60,21 @@ func SecurityPolicy(name string, findings []finding.Finding, dl checker.DetailLo
 				e := sce.WithMessage(sce.ErrScorecardInternal, "unknown probe results")
 				return checker.CreateRuntimeErrorResult(name, e)
 			}
+		case finding.OutcomeFalse:
+			logLevel = checker.DetailWarn
+		default:
+			logLevel = checker.DetailDebug
 		}
+		checker.LogFinding(dl, f, logLevel)
 	}
 	_, defined := m[securityPolicyPresent.Probe]
 	if !defined {
 		if score > 0 {
 			e := sce.WithMessage(sce.ErrScorecardInternal, "score calculation problem")
 			return checker.CreateRuntimeErrorResult(name, e)
 		}
-
-		// Log all findings.
-		checker.LogFindings(findings, dl)
 		return checker.CreateMinScoreResult(name, "security policy file not detected")
 	}
-
-	// Log all findings.
-	// NOTE: if the score is checker.MaxResultScore, then all findings are true.
-	// If the score is less than checker.MaxResultScore, some findings are false,
-	// so we log both true and false findings.
-	checker.LogFindings(findings, dl)
-
 	return checker.CreateResultWithScore(name, "security policy file detected", score)
 }
 

diff --git a/checks/evaluation/security_policy_test.go b/checks/evaluation/security_policy_test.go
@@ -127,8 +127,10 @@ func TestSecurityPolicy(t *testing.T) {
 				},
 			},
 			result: scut.TestReturn{
-				Score: checker.InconclusiveResultScore,
-				Error: sce.ErrScorecardInternal,
+				Score:        checker.InconclusiveResultScore,
+				Error:        sce.ErrScorecardInternal,
+				NumberOfWarn: 1,
+				NumberOfInfo: 3,
 			},
 		},
 		{

diff --git a/checks/evaluation/signed_releases.go b/checks/evaluation/signed_releases.go
@@ -73,8 +73,8 @@ func SignedReleases(name string,
 	totalTrue := 0
 	releaseMap := make(map[string]int)
 	uniqueReleaseTags := make([]string, 0)
-	checker.LogFindings(findings, dl)
 
+	var logLevel checker.DetailType
 	for i := range findings {
 		f := &findings[i]
 
@@ -86,9 +86,10 @@ func SignedReleases(name string,
 			uniqueReleaseTags = append(uniqueReleaseTags, releaseName)
 		}
 
-		if f.Outcome == finding.OutcomeTrue {
+		switch f.Outcome {
+		case finding.OutcomeTrue:
+			logLevel = checker.DetailInfo
 			totalTrue++
-
 			switch f.Probe {
 			case releasesAreSigned.Probe:
 				if _, ok := releaseMap[releaseName]; !ok {
@@ -97,7 +98,12 @@ func SignedReleases(name string,
 			case releasesHaveProvenance.Probe:
 				releaseMap[releaseName] = 10
 			}
+		case finding.OutcomeFalse:
+			logLevel = checker.DetailWarn
+		default:
+			logLevel = checker.DetailDebug
 		}
+		checker.LogFinding(dl, f, logLevel)
 	}
 
 	if totalTrue == 0 {