⚠️ Warn and not fail if job perms are content:write

[eddie-knight]

Following discussion in #2338, it has been determined that the permissions check should no longer deduct when a job is set to write. This code is a minimalist change, and will require additional polish/cleanup of obsolete logic later.

Signed-off-by: Eddie Knight [email protected]

What kind of change does this PR introduce?

Changes how scoring is done for token permissions

• PR title follows the guidelines defined in our pull request documentation

What is the current behavior?

The Token-Permissions check is opinionated regarding job-level permissions.

What is the new behavior (if this is a feature change)?**

The Token-Permissions check now will only issue a warning regarding job-level permissions.

• Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #2338

Special notes for your reviewer

I wasn't able to get the Token-Permissions check to report anything lower than 10/10 when running locally, even for repos that are less than 10 when checked via the API. As such, my validation checks were limited. I suggest a regular contributor or maintainer validates these changes before merging.

Does this PR introduce a user-facing change?

Job Level Permissions will no longer be evaluated on the Scorecard, but warnings will still be issued.

[spencerschrock]

If we want to warn on write permissions, but not affect scoring if defined at the run level, this might be better addressed in the scoring code. Curious what others think. Various unit tests (make unit-test) check the number of debug, vs info, vs warn messages. Regardless of approach, some of the unit-tests will need changed as the expected scores will change.

scorecard/checks/evaluation/permissions.go

Line 191 in 36d6a34

func calculateScore(result map[string]permissions) int {

The various calls to permissionIsPresent could be replaced with permissionIsPresentInTopLevel for example.

[eddie-knight]

Thanks @spencerschrock, I think it's a good idea to shift the change into the calculate function instead.

[github-actions]

Integration tests success for
[3e127da]
(https://github.com/ossf/scorecard/actions/runs/3253012126)

[codecov]

Codecov Report

Merging #2355 (72d9e4e) into main (78c7e83) will not change coverage.
The diff coverage is 0.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2355   +/-   ##
=======================================
  Coverage   40.55%   40.55%           
=======================================
  Files         112      112           
  Lines        8822     8822           
=======================================
  Hits         3578     3578           
  Misses       4984     4984           
  Partials      260      260           

[naveensrinivasan]

Thanks! I'll let @spencerschrock 👀

[spencerschrock]

LGTM overall. May be worth adding packages as that's another permission which results in a -10 deduction if not on the allowlist.

There's also an allowlist for security-events, but I imagine that's much less commonly used and only a -1 deduction.

[github-actions]

Integration tests success for
[72d9e4e]
(https://github.com/ossf/scorecard/actions/runs/3267248107)

[eddie-knight]

Thanks @spencerschrock! I raised packages as a suggestion for discussion on #2338 so we can make a PR for that as well if there is agreement