✨ Add support for RequiresLastPushReview in Branch Protection for Git…

…Hub (ossf#2492) * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> * update Signed-off-by: laurentsimon <[email protected]> Signed-off-by: laurentsimon <[email protected]>
raghavkaul · Feb 9, 2023 · fd19013 · fd19013
1 parent 4a4b3c9
commit fd19013
Show file tree

Hide file tree

Showing 8 changed files with 150 additions and 114 deletions.
diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go
@@ -135,7 +135,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         2,
-				NumberOfWarn:  6,
+				NumberOfWarn:  7,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
 			},
@@ -159,10 +159,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
 						},
-						EnforceAdmins:        &falseVal,
-						RequireLinearHistory: &falseVal,
-						AllowForcePushes:     &falseVal,
-						AllowDeletions:       &falseVal,
+						EnforceAdmins:           &falseVal,
+						RequireLinearHistory:    &falseVal,
+						RequireLastPushApproval: &falseVal,
+						AllowForcePushes:        &falseVal,
+						AllowDeletions:          &falseVal,
 					},
 				},
 			},
@@ -173,8 +174,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         2,
-				NumberOfWarn:  8,
-				NumberOfInfo:  9,
+				NumberOfWarn:  9,
+				NumberOfInfo:  10,
 				NumberOfDebug: 0,
 			},
 			defaultBranch: main,
@@ -193,10 +194,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
 						},
-						EnforceAdmins:        &trueVal,
-						RequireLinearHistory: &trueVal,
-						AllowForcePushes:     &falseVal,
-						AllowDeletions:       &falseVal,
+						EnforceAdmins:           &trueVal,
+						RequireLastPushApproval: &trueVal,
+						RequireLinearHistory:    &trueVal,
+						AllowForcePushes:        &falseVal,
+						AllowDeletions:          &falseVal,
 					},
 				},
 				{
@@ -213,10 +215,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
 						},
-						EnforceAdmins:        &falseVal,
-						RequireLinearHistory: &falseVal,
-						AllowForcePushes:     &falseVal,
-						AllowDeletions:       &falseVal,
+						EnforceAdmins:           &falseVal,
+						RequireLastPushApproval: &falseVal,
+						RequireLinearHistory:    &falseVal,
+						AllowForcePushes:        &falseVal,
+						AllowDeletions:          &falseVal,
 					},
 				},
 			},
@@ -228,7 +231,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				Error:         nil,
 				Score:         8,
 				NumberOfWarn:  4,
-				NumberOfInfo:  14,
+				NumberOfInfo:  16,
 				NumberOfDebug: 0,
 			},
 			defaultBranch: main,
@@ -247,10 +250,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
 						},
-						EnforceAdmins:        &trueVal,
-						RequireLinearHistory: &trueVal,
-						AllowForcePushes:     &falseVal,
-						AllowDeletions:       &falseVal,
+						EnforceAdmins:           &trueVal,
+						RequireLastPushApproval: &trueVal,
+						RequireLinearHistory:    &trueVal,
+						AllowForcePushes:        &falseVal,
+						AllowDeletions:          &falseVal,
 					},
 				},
 				{
@@ -267,10 +271,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
 						},
-						EnforceAdmins:        &trueVal,
-						RequireLinearHistory: &trueVal,
-						AllowForcePushes:     &falseVal,
-						AllowDeletions:       &falseVal,
+						EnforceAdmins:           &trueVal,
+						RequireLastPushApproval: &trueVal,
+						RequireLinearHistory:    &trueVal,
+						AllowForcePushes:        &falseVal,
+						AllowDeletions:          &falseVal,
 					},
 				},
 			},
@@ -281,7 +286,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         2,
-				NumberOfWarn:  6,
+				NumberOfWarn:  7,
 				NumberOfInfo:  2,
 				NumberOfDebug: 0,
 			},
@@ -302,10 +307,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
 						},
-						EnforceAdmins:        &falseVal,
-						RequireLinearHistory: &falseVal,
-						AllowForcePushes:     &falseVal,
-						AllowDeletions:       &falseVal,
+						EnforceAdmins:           &falseVal,
+						RequireLastPushApproval: &falseVal,
+						RequireLinearHistory:    &falseVal,
+						AllowForcePushes:        &falseVal,
+						AllowDeletions:          &falseVal,
 					},
 				}, {
 					Name:      &rel1,
@@ -354,7 +360,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				Score:         0,
 				NumberOfWarn:  4,
 				NumberOfInfo:  0,
-				NumberOfDebug: 6,
+				NumberOfDebug: 8,
 			},
 			nonadmin:      true,
 			defaultBranch: main,

diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go
@@ -373,18 +373,31 @@ func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (
 	// Only log information if the branch is protected.
 	log := branch.Protected != nil && *branch.Protected
 
-	if branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge != nil {
+	// Process UpToDateBeforeMerge value.
+	if branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge == nil {
+		debug(dl, log, "unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", *branch.Name)
+	} else {
 		// Note: `This setting will not take effect unless at least one status check is enabled`.
 		max++
-		switch *branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge {
-		case true:
+		if *branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge {
 			info(dl, log, "status checks require up-to-date branches for '%s'", *branch.Name)
 			score++
-		default:
+		} else {
 			warn(dl, log, "status checks do not require up-to-date branches for '%s'", *branch.Name)
 		}
+	}
+
+	// Process RequireLastPushApproval value.
+	if branch.BranchProtectionRule.RequireLastPushApproval == nil {
+		debug(dl, log, "unable to retrieve whether 'last push approval' is required to merge on branch '%s'", *branch.Name)
 	} else {
-		debug(dl, log, "unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", *branch.Name)
+		max++
+		if *branch.BranchProtectionRule.RequireLastPushApproval {
+			info(dl, log, "'last push approval' enabled on branch '%s'", *branch.Name)
+			score++
+		} else {
+			warn(dl, log, "'last push approval' disabled on branch '%s'", *branch.Name)
+		}
 	}
 
 	return score, max
@@ -401,10 +414,10 @@ func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailL
 		max++
 		switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews {
 		case true:
-			info(dl, log, "Stale review dismissal enabled on branch '%s'", *branch.Name)
+			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)
+			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)