Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ feature: branch protection without admin token #823

Merged
merged 8 commits into from
Aug 12, 2021

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Aug 9, 2021

Signed-off-by: Asra Ali asraa@google.com

  • Please check if the PR fulfills these requirements
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Enables a score for branch protection without an admin token for the repository. Only an "is protected" boolean is available via the Branches API. In this case, a score of 1 with a warning is added to the final score (with a detailed reason). Enabling branch protection without using any settings gives a score of 1.

  • What is the current behavior? (You can also link to an open issue here)
    Not found for non-admin token usage

  • What is the new behavior (if this is a feature change)?
    Score of 1 with warning for protected branches

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

  • Other information:

Signed-off-by: Asra Ali <asraa@google.com>
@asraa asraa force-pushed the branch-protection-non-admin branch from 3bfedb5 to e283518 Compare August 9, 2021 18:37
@laurentsimon laurentsimon self-requested a review August 10, 2021 00:13
@@ -113,7 +114,12 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, dl
// The branch is protected. Check the protection.
score, err := getProtectionAndCheck(ctx, r, dl, ownerStr, repoStr, b)
if err != nil {
return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we differentiate true errors and errors that lead to an unknown result?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we know this branch is protected, so calling getProtectionAndCheck will only return an error if we can't get the detailed protection information.

Redirects however: they did close my issue on getting the redirected branch when you get protection, but I will test this out in case I'm missing a way to use it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. The assumption is that https://github.com/ossf/scorecard/blob/main/checks/branch_protection.go#L174 does not trigger a new request that could timeout or return a GitHub's internal error. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an explicit check to make sure it's a 404 Not Found and not a timeout, i think you're right.

We should have already resolved the branch name by matching it against known branch names, so we can't run into a problem that the branch isn't found because it doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg, thanks for double checking!

Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
@naveensrinivasan naveensrinivasan enabled auto-merge (squash) August 12, 2021 15:06
@naveensrinivasan naveensrinivasan merged commit cc312f2 into ossf:main Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants