fix: avoid penalyzing non-admin for dismissStaleReview

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>
ossf · Dec 6, 2023 · 6858790 · 6858790
1 parent d63e71b
commit 6858790
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 24 deletions.
diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go
@@ -341,24 +341,19 @@ func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailL
 	// Only log information if the branch is protected.
 	log := branch.Protected != nil && *branch.Protected
 
-	if branch.BranchProtectionRule.RequiredPullRequestReviews == nil {
-		// If the repo doesn't require PRs, we still have to discount score for not dismissing stale reviews.
+	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
+		branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil {
+		// Note: we don't increase max possible score for non-admin viewers.
 		max++
-		warn(dl, log, "stale review dismissal disabled on branch '%s'", *branch.Name)
-	} else {
-		if branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews == nil {
-			// The case of a non-admin run on a repo using the old Branch Protection
-			debug(dl, log, "unable to retrieve review dismissal on branch '%s'", *branch.Name)
-		} else {
-			max++
-			switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews {
-			case true:
-				info(dl, log, "stale review dismissal enabled on branch '%s'", *branch.Name)
-				score++
-			case false:
-				warn(dl, log, "stale review dismissal disabled on branch '%s'", *branch.Name)
-			}
+		switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews {
+		case true:
+			info(dl, log, "stale review dismissal enabled on branch '%s'", *branch.Name)
+			score++
+		case false:
+			warn(dl, log, "stale review dismissal disabled on branch '%s'", *branch.Name)
 		}
+	} else {
+		debug(dl, log, "unable to retrieve review dismissal on branch '%s'", *branch.Name)
 	}
 
 	// nil typically means we do not have access to the value.

diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go
@@ -86,9 +86,9 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         0,
-				NumberOfWarn:  4,
+				NumberOfWarn:  3,
 				NumberOfInfo:  0,
-				NumberOfDebug: 3,
+				NumberOfDebug: 4,
 			},
 			branch: &clients.BranchRef{
 				Name:      &branchVal,
@@ -162,9 +162,9 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         3,
-				NumberOfWarn:  7,
+				NumberOfWarn:  6,
 				NumberOfInfo:  2,
-				NumberOfDebug: 0,
+				NumberOfDebug: 1,
 			},
 			branch: &clients.BranchRef{
 				Name:      &branchVal,
@@ -189,9 +189,9 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         4, // Should be 4.2 if we allow decimal puctuation
-				NumberOfWarn:  3,
+				NumberOfWarn:  2,
 				NumberOfInfo:  6,
-				NumberOfDebug: 0,
+				NumberOfDebug: 1,
 			},
 			branch: &clients.BranchRef{
 				Name:      &branchVal,
@@ -278,9 +278,9 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         3,
-				NumberOfWarn:  4,
+				NumberOfWarn:  3,
 				NumberOfInfo:  2,
-				NumberOfDebug: 3,
+				NumberOfDebug: 4,
 			},
 			branch: &clients.BranchRef{
 				Name:      &branchVal,