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

[JENKINS-66661] Fix fallback behavior of Fork PR trust criteria #711

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Dohbedoh
Copy link
Contributor

@Dohbedoh Dohbedoh commented Jun 28, 2023

Description

JENKINS-66661: The "Collaborators" and "From users with Admin or Write permission" trust strategies of the PR from Fork traits are breaking Indexing / event mechanism / Jenkinsfile retrieval in cases where the request fails with 401 | 403 | 404.
Instead of failing the entire scan, the trust check should return that the revision is not trusted.

Submitter checklist

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Automated tests have been added to exercise the changes
  • Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

  • Run the changes and verify that the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

Documentation changes

  • Link to jenkins.io PR, or an explanation for why no doc changes are needed

Users/aliases to notify

@Dohbedoh Dohbedoh changed the title [JENKINS-66661] Fix Fork PR trust criteria fallback behavior [JENKINS-66661] Fix fallback behavior of Fork PR trust criteria Jun 28, 2023
@Dohbedoh
Copy link
Contributor Author

ping @jenkinsci/github-branch-source-plugin-developers

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

Personally I am against this change as is.

If the repository collaborators can not be obtained then the job should fail fast not hide the fact in a log message that no one is looking at.

People have setup the job with the expectation that it would test their Jenkinsfile changes in a PR -

If they want everything to be treated as non trusted then they should actually set the trust to "no one".

This would lead to PRs from maintainers passing when they should fail and then a potentially broken master branch.

I would go so far as to say the previous fix in this area was also incorrect.

if a user is wanting to be able to trust some users (not all and not none) then we should obey this and if we can not should not silently ignore the fact and continue (no one looks at logs unless something breaks!)

IN other words the correct "fix" for me would be to catch the exception and throw a more detailed one saying your credentials "do not have access to query for collaborators - you can either change your scan credential or change the trust model to NO-ONE"

@jglick
Copy link
Member

jglick commented Jun 28, 2023

(also see jenkinsci/scm-api-plugin#180)

@Dohbedoh
Copy link
Contributor Author

Dohbedoh commented Jul 5, 2023

Adjusted the behavior to the following:

  • on PR retrieval (org scan / branch indexing / PR event) the PR is skipped if the trust check fails with an exception
  • on PR revision retrieval (get the Jenkinsfile when building a PR) the retrieval fails if the trust check fails with an exception

@Dohbedoh Dohbedoh requested a review from jtnord July 10, 2023 23:18
@jtnord jtnord requested a review from a team October 13, 2023 09:57
@Dohbedoh
Copy link
Contributor Author

pinging maintainers @dwnusbaum @jeromepochat @rsandell

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.

3 participants