From ba4abcb9eb3894c809d077f40fef629d1584d5d2 Mon Sep 17 00:00:00 2001 From: Olivier Lamy Date: Mon, 24 Aug 2026 21:41:28 +1000 Subject: [PATCH 1/4] Add an external contributor approval mechanism Signed-off-by: Olivier Lamy --- pom.xml | 4 + .../ExternalApprovalHelper.java | 131 ++++++ .../ForkPullRequestDiscoveryTrait.java | 207 ++++++++++ .../PendingApprovalAction.java | 378 ++++++++++++++++++ .../TrustExternalApproval/config.jelly | 14 + .../TrustExternalApproval/config.properties | 3 + .../github_branch_source/Messages.properties | 5 + .../PendingApprovalAction/index.jelly | 58 +++ .../PendingApprovalAction/summary.jelly | 14 + .../ForkPullRequestDiscoveryTrait2Test.java | 2 + .../ForkPullRequestDiscoveryTraitTest.java | 103 +++++ .../PendingApprovalActionTest.java | 107 +++++ 12 files changed, 1026 insertions(+) create mode 100644 src/main/java/org/jenkinsci/plugins/github_branch_source/ExternalApprovalHelper.java create mode 100644 src/main/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction.java create mode 100644 src/main/resources/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait/TrustExternalApproval/config.jelly create mode 100644 src/main/resources/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait/TrustExternalApproval/config.properties create mode 100644 src/main/resources/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction/index.jelly create mode 100644 src/main/resources/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction/summary.jelly create mode 100644 src/test/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalActionTest.java diff --git a/pom.xml b/pom.xml index 3b71f1702..4d4c90b2b 100644 --- a/pom.xml +++ b/pom.xml @@ -79,6 +79,10 @@ io.jenkins.plugins okhttp-api + + org.jenkins-ci.plugins + branch-api + org.jenkins-ci.plugins credentials diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/ExternalApprovalHelper.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/ExternalApprovalHelper.java new file mode 100644 index 000000000..17e65fdd0 --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/ExternalApprovalHelper.java @@ -0,0 +1,131 @@ +/* + * The MIT License + * + * Copyright 2026 Olivier Lamy + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package org.jenkinsci.plugins.github_branch_source; + +import edu.umd.cs.findbugs.annotations.CheckForNull; +import hudson.model.Job; +import jenkins.branch.Branch; +import jenkins.branch.BranchProjectFactory; +import jenkins.branch.BranchSource; +import jenkins.branch.MultiBranchProject; +import jenkins.scm.api.SCMHead; +import jenkins.scm.api.SCMHeadOrigin; +import jenkins.scm.api.SCMRevision; +import jenkins.scm.api.SCMSource; +import jenkins.scm.api.trait.SCMSourceTrait; + +/** + * Utility class to determine if a branch job requires external approval. + */ +final class ExternalApprovalHelper { + + private ExternalApprovalHelper() {} + + /** + * Checks if the given job is a fork pull request in a MultiBranchProject configured + * with {@link ForkPullRequestDiscoveryTrait.TrustExternalApproval}. + * + * @param job the job to check + * @return approval info if external approval is required, {@code null} otherwise + */ + @CheckForNull + @SuppressWarnings({"rawtypes", "unchecked"}) + static ExternalApprovalInfo getApprovalInfo(Job job) { + if (!(job.getParent() instanceof MultiBranchProject)) { + return null; + } + MultiBranchProject mp = (MultiBranchProject) job.getParent(); + BranchProjectFactory factory = mp.getProjectFactory(); + if (!factory.isProject(job)) { + return null; + } + Branch branch = factory.getBranch(job); + SCMHead head = branch.getHead(); + if (!(head instanceof PullRequestSCMHead)) { + return null; + } + PullRequestSCMHead prHead = (PullRequestSCMHead) head; + if (prHead.getOrigin().equals(SCMHeadOrigin.DEFAULT)) { + return null; + } + ForkPullRequestDiscoveryTrait.TrustExternalApproval trustPolicy = findTrustPolicy(mp); + if (trustPolicy == null) { + return null; + } + String currentPullHash = getCurrentPullHash(factory, job); + return new ExternalApprovalInfo( + prHead.getNumber(), + prHead.getSourceOwner(), + currentPullHash, + trustPolicy.isRequireApprovalForNewCommits()); + } + + @CheckForNull + @SuppressWarnings("rawtypes") + private static ForkPullRequestDiscoveryTrait.TrustExternalApproval findTrustPolicy(MultiBranchProject mp) { + for (Object src : mp.getSources()) { + if (src instanceof BranchSource) { + SCMSource source = ((BranchSource) src).getSource(); + if (source instanceof GitHubSCMSource) { + for (SCMSourceTrait trait : ((GitHubSCMSource) source).getTraits()) { + if (trait instanceof ForkPullRequestDiscoveryTrait) { + Object trust = ((ForkPullRequestDiscoveryTrait) trait).getTrust(); + if (trust instanceof ForkPullRequestDiscoveryTrait.TrustExternalApproval) { + return (ForkPullRequestDiscoveryTrait.TrustExternalApproval) trust; + } + } + } + } + } + } + return null; + } + + @CheckForNull + @SuppressWarnings({"rawtypes", "unchecked"}) + private static String getCurrentPullHash(BranchProjectFactory factory, Job job) { + SCMRevision revision = factory.getRevision(job); + if (revision instanceof PullRequestSCMRevision) { + return ((PullRequestSCMRevision) revision).getPullHash(); + } + return null; + } +} + +/** + * Holds information about a fork PR that requires external approval. + */ +class ExternalApprovalInfo { + final int prNumber; + final String prAuthor; + final String currentPullHash; + final boolean requireApprovalForNewCommits; + + ExternalApprovalInfo(int prNumber, String prAuthor, String currentPullHash, boolean requireApprovalForNewCommits) { + this.prNumber = prNumber; + this.prAuthor = prAuthor; + this.currentPullHash = currentPullHash; + this.requireApprovalForNewCommits = requireApprovalForNewCommits; + } +} diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java index 495380b21..d72691445 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java @@ -23,10 +23,14 @@ */ package org.jenkinsci.plugins.github_branch_source; +import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Extension; +import hudson.util.FormValidation; import hudson.util.ListBoxModel; import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; import java.util.EnumSet; import java.util.List; import java.util.Set; @@ -46,8 +50,12 @@ import org.jenkinsci.Symbol; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; +import org.kohsuke.github.GHLabel; import org.kohsuke.github.GHPermissionType; +import org.kohsuke.github.GHPullRequest; import org.kohsuke.stapler.DataBoundConstructor; +import org.kohsuke.stapler.DataBoundSetter; +import org.kohsuke.stapler.QueryParameter; /** * A {@link Discovery} trait for GitHub that will discover pull requests from forks of the @@ -385,4 +393,203 @@ public boolean isApplicableToOrigin(@NonNull Class orig } } } + + /** + * An {@link SCMHeadAuthority} that requires external approval before fork pull requests can + * build. Jobs are created as disabled with a pending approval marker. An administrator must + * approve via the UI or API before the job will run. + */ + public static class TrustExternalApproval extends GitHubForkTrustPolicy { + private boolean requireApprovalForNewCommits; + + @CheckForNull + private List autoApprovalLabels; + + @CheckForNull + private List autoApprovalUsers; + + /** Constructor. */ + @DataBoundConstructor + public TrustExternalApproval() {} + + /** + * Returns whether a new approval is required when new commits are pushed to the PR. + * + * @return {@code true} if approval is required for each new commit. + */ + public boolean isRequireApprovalForNewCommits() { + return requireApprovalForNewCommits; + } + + /** + * Sets whether a new approval is required when new commits are pushed to the PR. + * + * @param requireApprovalForNewCommits {@code true} to require re-approval on new commits. + */ + @DataBoundSetter + public void setRequireApprovalForNewCommits(boolean requireApprovalForNewCommits) { + this.requireApprovalForNewCommits = requireApprovalForNewCommits; + } + + /** + * Returns the list of PR labels that trigger automatic approval. + * + * @return the list of label names, or {@code null} if not configured. + */ + @CheckForNull + public List getAutoApprovalLabels() { + return autoApprovalLabels; + } + + /** + * Returns the auto-approval labels as a comma-separated string for form binding. + * + * @return comma-separated label names, or {@code null} if not configured. + */ + @CheckForNull + public String getAutoApprovalLabelsString() { + return autoApprovalLabels == null ? null : String.join(", ", autoApprovalLabels); + } + + /** + * Sets the list of PR labels from a comma-separated string (Stapler form binding). + * + * @param autoApprovalLabels comma-separated label names. + */ + @DataBoundSetter + public void setAutoApprovalLabels(@CheckForNull String autoApprovalLabels) { + this.autoApprovalLabels = parseCommaSeparated(autoApprovalLabels); + } + + /** + * Sets the list of PR labels that trigger automatic approval. + * + * @param autoApprovalLabels the label names to auto-approve. + */ + public void setAutoApprovalLabelsList(@CheckForNull List autoApprovalLabels) { + if (autoApprovalLabels == null || autoApprovalLabels.isEmpty()) { + this.autoApprovalLabels = null; + } else { + this.autoApprovalLabels = Collections.unmodifiableList(new ArrayList<>(autoApprovalLabels)); + } + } + + /** + * Returns the list of GitHub user logins that are automatically trusted. + * + * @return the list of user logins, or {@code null} if not configured. + */ + @CheckForNull + public List getAutoApprovalUsers() { + return autoApprovalUsers; + } + + /** + * Returns the auto-approval users as a comma-separated string for form binding. + * + * @return comma-separated user logins, or {@code null} if not configured. + */ + @CheckForNull + public String getAutoApprovalUsersString() { + return autoApprovalUsers == null ? null : String.join(", ", autoApprovalUsers); + } + + /** + * Sets the list of GitHub user logins from a comma-separated string (Stapler form + * binding). + * + * @param autoApprovalUsers comma-separated GitHub login names. + */ + @DataBoundSetter + public void setAutoApprovalUsers(@CheckForNull String autoApprovalUsers) { + this.autoApprovalUsers = parseCommaSeparated(autoApprovalUsers); + } + + /** + * Sets the list of GitHub user logins that are automatically trusted. + * + * @param autoApprovalUsers the GitHub login names to auto-approve. + */ + public void setAutoApprovalUsersList(@CheckForNull List autoApprovalUsers) { + if (autoApprovalUsers == null || autoApprovalUsers.isEmpty()) { + this.autoApprovalUsers = null; + } else { + this.autoApprovalUsers = Collections.unmodifiableList(new ArrayList<>(autoApprovalUsers)); + } + } + + @CheckForNull + private static List parseCommaSeparated(@CheckForNull String value) { + if (value == null || value.isBlank()) { + return null; + } + List result = new ArrayList<>(); + for (String entry : value.split(",")) { + String trimmed = entry.trim(); + if (!trimmed.isEmpty()) { + result.add(trimmed); + } + } + return result.isEmpty() ? null : Collections.unmodifiableList(result); + } + + /** {@inheritDoc} */ + @Override + protected boolean checkTrusted(@NonNull GitHubSCMSourceRequest request, @NonNull PullRequestSCMHead head) + throws IOException, InterruptedException { + if (autoApprovalUsers != null && autoApprovalUsers.contains(head.getSourceOwner())) { + return true; + } + if (autoApprovalLabels != null && !autoApprovalLabels.isEmpty()) { + for (GHPullRequest pr : request.getPullRequests()) { + if (pr.getNumber() != head.getNumber()) { + continue; + } + for (GHLabel label : pr.getLabels()) { + if (autoApprovalLabels.contains(label.getName())) { + return true; + } + } + break; + } + } + return false; + } + + /** Our descriptor. */ + @Symbol("gitHubTrustExternalApproval") + @Extension + public static class DescriptorImpl extends SCMHeadAuthorityDescriptor { + + /** {@inheritDoc} */ + @Override + public String getDisplayName() { + return Messages.ForkPullRequestDiscoveryTrait_externalApprovalDisplayName(); + } + + /** {@inheritDoc} */ + @Override + public boolean isApplicableToOrigin(@NonNull Class originClass) { + return SCMHeadOrigin.Fork.class.isAssignableFrom(originClass); + } + + @Restricted(NoExternalUse.class) + @SuppressWarnings("unused") // stapler + public FormValidation doCheckAutoApprovalUsers(@QueryParameter String value) { + if (value == null || value.isBlank()) { + return FormValidation.ok(); + } + for (String entry : value.split(",")) { + String trimmed = entry.trim(); + if (trimmed.isEmpty()) { + continue; + } + if (trimmed.contains(" ")) { + return FormValidation.warning("GitHub logins should not contain spaces: '" + trimmed + "'"); + } + } + return FormValidation.ok(); + } + } + } } diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction.java new file mode 100644 index 000000000..51ac59ae0 --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction.java @@ -0,0 +1,378 @@ +/* + * The MIT License + * + * Copyright 2026 Olivier Lamy + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package org.jenkinsci.plugins.github_branch_source; + +import edu.umd.cs.findbugs.annotations.NonNull; +import edu.umd.cs.findbugs.annotations.Nullable; +import hudson.Extension; +import hudson.XmlFile; +import hudson.model.Action; +import hudson.model.Cause; +import hudson.model.CauseAction; +import hudson.model.Item; +import hudson.model.Job; +import hudson.model.Queue; +import hudson.model.queue.ScheduleResult; +import java.io.File; +import java.io.IOException; +import java.io.Serializable; +import java.util.Collection; +import java.util.Collections; +import java.util.logging.Level; +import java.util.logging.Logger; +import jenkins.model.Jenkins; +import jenkins.model.TransientActionFactory; +import org.kohsuke.stapler.HttpRedirect; +import org.kohsuke.stapler.HttpResponse; +import org.kohsuke.stapler.StaplerRequest2; +import org.kohsuke.stapler.interceptor.RequirePOST; + +/** + * Action displayed on branch jobs that require external approval before building. + * Provides UI elements and API endpoints to approve or reject fork pull requests. + */ +public class PendingApprovalAction implements Action { + + private static final Logger LOGGER = Logger.getLogger(PendingApprovalAction.class.getName()); + + private final transient Job owner; + private final ApprovalState state; + private final int prNumber; + private final String prAuthor; + private final String currentPullHash; + private final boolean requireApprovalForNewCommits; + + PendingApprovalAction( + Job owner, + ApprovalState state, + int prNumber, + String prAuthor, + String currentPullHash, + boolean requireApprovalForNewCommits) { + this.owner = owner; + this.state = state; + this.prNumber = prNumber; + this.prAuthor = prAuthor; + this.currentPullHash = currentPullHash; + this.requireApprovalForNewCommits = requireApprovalForNewCommits; + } + + @Override + public String getIconFileName() { + if (state == ApprovalState.PENDING) { + return "symbol-warning plugin-ionicons-api"; + } + return null; + } + + @Override + public String getDisplayName() { + if (state == ApprovalState.PENDING) { + return Messages.PendingApprovalAction_displayName(); + } + return Messages.PendingApprovalAction_approved(); + } + + @Override + public String getUrlName() { + return "pendingApproval"; + } + + public ApprovalState getState() { + return state; + } + + public int getPrNumber() { + return prNumber; + } + + public String getPrAuthor() { + return prAuthor; + } + + public String getCurrentPullHash() { + return currentPullHash; + } + + public boolean isRequireApprovalForNewCommits() { + return requireApprovalForNewCommits; + } + + public Job getOwner() { + return owner; + } + + /** + * Approves the pull request, enabling the job and scheduling a build. + * + * @param req the stapler request + * @return redirect to the parent job + */ + @RequirePOST + public HttpResponse doApprove(StaplerRequest2 req) { + owner.checkPermission(Item.CONFIGURE); + try { + String approvedBy = Jenkins.get().getAuthentication2().getName(); + ApprovalData data = ApprovalData.load(owner); + data.state = ApprovalState.APPROVED; + data.approvedBy = approvedBy; + data.approvedAt = System.currentTimeMillis(); + data.approvedPullHash = currentPullHash; + data.save(owner); + enableAndBuild(); + LOGGER.log(Level.INFO, "PR #{0} in {1} approved by {2}", new Object[] { + prNumber, owner.getFullName(), approvedBy + }); + } catch (IOException e) { + LOGGER.log(Level.WARNING, "Failed to approve PR #" + prNumber, e); + } + return new HttpRedirect(".."); + } + + /** + * Rejects the pull request approval. + * + * @param req the stapler request + * @return redirect to the parent job + */ + @RequirePOST + public HttpResponse doReject(StaplerRequest2 req) { + owner.checkPermission(Item.CONFIGURE); + try { + ApprovalData data = ApprovalData.load(owner); + data.state = ApprovalState.PENDING; + data.approvedBy = null; + data.approvedAt = 0; + data.approvedPullHash = null; + data.save(owner); + disableJob(); + LOGGER.log(Level.INFO, "PR #{0} in {1} rejected by {2}", new Object[] { + prNumber, + owner.getFullName(), + Jenkins.get().getAuthentication2().getName() + }); + } catch (IOException e) { + LOGGER.log(Level.WARNING, "Failed to reject PR #" + prNumber, e); + } + return new HttpRedirect(".."); + } + + private void enableAndBuild() throws IOException { + if (isJobDisabled()) { + makeDisabled(false); + } + if (owner instanceof Queue.Task) { + ScheduleResult result = Jenkins.get() + .getQueue() + .schedule2((Queue.Task) owner, 0, new CauseAction(new ExternalApprovalCause())); + if (result.isRefused()) { + LOGGER.log(Level.WARNING, "Failed to schedule build for {0}", owner.getFullName()); + } + } + } + + private void disableJob() throws IOException { + if (!isJobDisabled()) { + makeDisabled(true); + } + } + + private boolean isJobDisabled() { + try { + java.lang.reflect.Method m = owner.getClass().getMethod("isDisabled"); + return (boolean) m.invoke(owner); + } catch (ReflectiveOperationException e) { + return false; + } + } + + @SuppressWarnings("unchecked") + private void makeDisabled(boolean disabled) throws IOException { + if (owner instanceof hudson.model.AbstractProject) { + ((hudson.model.AbstractProject) owner).makeDisabled(disabled); + } else { + try { + java.lang.reflect.Method m = owner.getClass().getMethod("setDisabled", boolean.class); + m.invoke(owner, disabled); + } catch (ReflectiveOperationException e) { + LOGGER.log(Level.WARNING, "Cannot change disabled state of " + owner.getFullName(), e); + } + } + } + + /** Approval state of an external pull request. */ + public enum ApprovalState { + PENDING, + APPROVED + } + + /** Cause indicating a build was triggered by external approval. */ + public static class ExternalApprovalCause extends Cause { + @Override + public String getShortDescription() { + return "External approval granted"; + } + } + + /** Persistent approval data stored in the job directory. */ + static class ApprovalData implements Serializable { + private static final long serialVersionUID = 1L; + + ApprovalState state = ApprovalState.PENDING; + + @Nullable + String approvedBy; + + long approvedAt; + + @Nullable + String approvedPullHash; + + static XmlFile getConfigFile(Job job) { + return new XmlFile(new File(job.getRootDir(), "pending-approval.xml")); + } + + static ApprovalData load(Job job) { + XmlFile file = getConfigFile(job); + if (file.exists()) { + try { + return (ApprovalData) file.read(); + } catch (IOException e) { + LOGGER.log(Level.WARNING, "Failed to load approval data for " + job.getFullName(), e); + } + } + return new ApprovalData(); + } + + void save(Job job) throws IOException { + getConfigFile(job).write(this); + } + + static boolean exists(Job job) { + return getConfigFile(job).exists(); + } + + static void delete(Job job) { + XmlFile file = getConfigFile(job); + if (file.exists()) { + if (!file.getFile().delete()) { + LOGGER.log(Level.WARNING, "Failed to delete approval data for {0}", job.getFullName()); + } + } + } + } + + /** + * Contributes {@link PendingApprovalAction} to branch jobs that require external approval. + */ + @Extension + public static class ActionFactory extends TransientActionFactory { + + @Override + public Class type() { + return Job.class; + } + + @NonNull + @Override + public Collection createFor(@NonNull Job target) { + ExternalApprovalInfo info = ExternalApprovalHelper.getApprovalInfo(target); + if (info == null) { + return Collections.emptyList(); + } + ApprovalData data = ApprovalData.load(target); + if (!ApprovalData.exists(target)) { + data.state = ApprovalState.PENDING; + try { + data.save(target); + } catch (IOException e) { + LOGGER.log(Level.WARNING, "Failed to initialize approval data for " + target.getFullName(), e); + } + } + if (info.requireApprovalForNewCommits + && data.state == ApprovalState.APPROVED + && data.approvedPullHash != null + && !data.approvedPullHash.equals(info.currentPullHash)) { + data.state = ApprovalState.PENDING; + data.approvedBy = null; + data.approvedAt = 0; + data.approvedPullHash = null; + try { + data.save(target); + } catch (IOException e) { + LOGGER.log(Level.WARNING, "Failed to reset approval data for " + target.getFullName(), e); + } + } + return Collections.singletonList(new PendingApprovalAction( + target, + data.state, + info.prNumber, + info.prAuthor, + info.currentPullHash, + info.requireApprovalForNewCommits)); + } + } + + /** + * Blocks builds of jobs that are pending external approval. + */ + @Extension + public static class QueueDecisionHandler extends Queue.QueueDecisionHandler { + + @Override + public boolean shouldSchedule(Queue.Task task, java.util.List actions) { + if (!(task instanceof Job)) { + return true; + } + Job job = (Job) task; + ExternalApprovalInfo info = ExternalApprovalHelper.getApprovalInfo(job); + if (info == null) { + return true; + } + ApprovalData data = ApprovalData.load(job); + if (data.state == ApprovalState.PENDING) { + PendingApprovalAction helper = + new PendingApprovalAction(job, data.state, info.prNumber, info.prAuthor, null, false); + if (!helper.isJobDisabled()) { + try { + helper.makeDisabled(true); + } catch (IOException e) { + LOGGER.log(Level.WARNING, "Failed to disable " + job.getFullName(), e); + } + } + return false; + } + for (Action action : actions) { + if (action instanceof CauseAction) { + for (Cause cause : ((CauseAction) action).getCauses()) { + if (cause instanceof ExternalApprovalCause) { + return true; + } + } + } + } + return true; + } + } +} diff --git a/src/main/resources/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait/TrustExternalApproval/config.jelly b/src/main/resources/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait/TrustExternalApproval/config.jelly new file mode 100644 index 000000000..ce635d576 --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait/TrustExternalApproval/config.jelly @@ -0,0 +1,14 @@ + + + + ${%blurb} + + + + + + + + + + diff --git a/src/main/resources/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait/TrustExternalApproval/config.properties b/src/main/resources/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait/TrustExternalApproval/config.properties new file mode 100644 index 000000000..a7f605be1 --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait/TrustExternalApproval/config.properties @@ -0,0 +1,3 @@ +blurb=Fork pull requests will be created as disabled jobs requiring explicit approval before building. +Auto-approval\ labels=Auto-approval labels (comma-separated). PRs with any of these GitHub labels will be automatically trusted. +Auto-approval\ users=Auto-approval users (comma-separated GitHub logins). PRs authored by any of these users will be automatically trusted. diff --git a/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties b/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties index cd647028e..1a8fd0a86 100644 --- a/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties +++ b/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties @@ -10,6 +10,11 @@ ForkPullRequestDiscoveryTrait.headAndMerge=Both the current pull request revisio ForkPullRequestDiscoveryTrait.headOnly=The current pull request revision ForkPullRequestDiscoveryTrait.mergeOnly=Merging the pull request with the current target branch revision ForkPullRequestDiscoveryTrait.nobodyDisplayName=Nobody +ForkPullRequestDiscoveryTrait.externalApprovalDisplayName=External approval required + +PendingApprovalAction.displayName=Pending Approval +PendingApprovalAction.approved=Approved +PendingApprovalAction.rejected=Rejected GitHubBranchFilter.DisplayName=GitHub Branch Jobs Only GitHubBuildStatusNotification.CommitStatus.Good=This commit looks good diff --git a/src/main/resources/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction/index.jelly b/src/main/resources/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction/index.jelly new file mode 100644 index 000000000..397e93fa1 --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction/index.jelly @@ -0,0 +1,58 @@ + + + + + +

External Approval Required

+ + + + + + + + + + + + + + + + + + + + + + + + + +
Pull Request#${it.prNumber}
Author${it.prAuthor}
Status + + + Pending Approval + + + Approved + + +
Commit${it.currentPullHash}
ModeApproval required for each new commit
+ +
+ + + +
+ + +
+ + + +
+ + + + diff --git a/src/main/resources/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction/summary.jelly b/src/main/resources/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction/summary.jelly new file mode 100644 index 000000000..3d700b952 --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction/summary.jelly @@ -0,0 +1,14 @@ + + + + +
+ This fork pull request requires external approval before it can build. +
+ PR #${it.prNumber} from ${it.prAuthor} + + (commit ${it.currentPullHash.substring(0, 7)}) + +
+ + diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait2Test.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait2Test.java index c5c3b49c5..06ae099ec 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait2Test.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait2Test.java @@ -52,6 +52,7 @@ public void configRoundtrip() throws Exception { assertRoundTrip(p, new ForkPullRequestDiscoveryTrait.TrustEveryone(), false); assertRoundTrip(p, new ForkPullRequestDiscoveryTrait.TrustContributors(), false); assertRoundTrip(p, new ForkPullRequestDiscoveryTrait.TrustPermission(), false); + assertRoundTrip(p, new ForkPullRequestDiscoveryTrait.TrustExternalApproval(), false); } @Test @@ -62,6 +63,7 @@ public void configRoundtripWithRawUrl() throws Exception { assertRoundTrip(p, new ForkPullRequestDiscoveryTrait.TrustEveryone(), true); assertRoundTrip(p, new ForkPullRequestDiscoveryTrait.TrustContributors(), true); assertRoundTrip(p, new ForkPullRequestDiscoveryTrait.TrustPermission(), true); + assertRoundTrip(p, new ForkPullRequestDiscoveryTrait.TrustExternalApproval(), true); } private void assertRoundTrip( diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTraitTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTraitTest.java index dbf93e8ee..24b8c8c1d 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTraitTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTraitTest.java @@ -100,4 +100,107 @@ public void given__nonDefaultTrust__when__appliedToContext__then__authoritiesCor assertThat(ctx.forkPRStrategies(), Matchers.is(EnumSet.allOf(ChangeRequestCheckoutStrategy.class))); assertThat(ctx.authorities(), hasItem(instanceOf(ForkPullRequestDiscoveryTrait.TrustEveryone.class))); } + + @Test + public void given__externalApproval__when__appliedToContext__then__authoritiesCorrect() throws Exception { + GitHubSCMSourceContext ctx = new GitHubSCMSourceContext(null, SCMHeadObserver.none()); + assumeThat(ctx.wantBranches(), is(false)); + assumeThat(ctx.wantPRs(), is(false)); + assumeThat(ctx.prefilters(), is(Collections.emptyList())); + assumeThat(ctx.filters(), is(Collections.emptyList())); + assumeThat( + ctx.authorities(), not(hasItem(instanceOf(ForkPullRequestDiscoveryTrait.TrustExternalApproval.class)))); + ForkPullRequestDiscoveryTrait instance = new ForkPullRequestDiscoveryTrait( + EnumSet.allOf(ChangeRequestCheckoutStrategy.class), + new ForkPullRequestDiscoveryTrait.TrustExternalApproval()); + instance.decorateContext(ctx); + assertThat(ctx.wantBranches(), is(false)); + assertThat(ctx.wantPRs(), is(true)); + assertThat(ctx.prefilters(), is(Collections.emptyList())); + assertThat(ctx.filters(), is(Collections.emptyList())); + assertThat(ctx.forkPRStrategies(), Matchers.is(EnumSet.allOf(ChangeRequestCheckoutStrategy.class))); + assertThat(ctx.authorities(), hasItem(instanceOf(ForkPullRequestDiscoveryTrait.TrustExternalApproval.class))); + } + + @Test + public void given__externalApproval__when__checkTrusted__then__returnsFalse() throws Exception { + ForkPullRequestDiscoveryTrait.TrustExternalApproval trust = + new ForkPullRequestDiscoveryTrait.TrustExternalApproval(); + assertThat(trust.isRequireApprovalForNewCommits(), is(false)); + trust.setRequireApprovalForNewCommits(true); + assertThat(trust.isRequireApprovalForNewCommits(), is(true)); + } + + @Test + public void given__externalApproval__when__autoApprovalUsers__then__configuredCorrectly() throws Exception { + ForkPullRequestDiscoveryTrait.TrustExternalApproval trust = + new ForkPullRequestDiscoveryTrait.TrustExternalApproval(); + assertThat(trust.getAutoApprovalUsers(), org.hamcrest.Matchers.nullValue()); + + trust.setAutoApprovalUsersList(java.util.Arrays.asList("user1", "user2")); + assertThat(trust.getAutoApprovalUsers(), is(java.util.Arrays.asList("user1", "user2"))); + assertThat(trust.getAutoApprovalUsersString(), is("user1, user2")); + + trust.setAutoApprovalUsersList(java.util.Collections.emptyList()); + assertThat(trust.getAutoApprovalUsers(), org.hamcrest.Matchers.nullValue()); + + trust.setAutoApprovalUsersList(null); + assertThat(trust.getAutoApprovalUsers(), org.hamcrest.Matchers.nullValue()); + } + + @Test + public void given__externalApproval__when__autoApprovalUsersString__then__parsedCorrectly() throws Exception { + ForkPullRequestDiscoveryTrait.TrustExternalApproval trust = + new ForkPullRequestDiscoveryTrait.TrustExternalApproval(); + + trust.setAutoApprovalUsers("user1, user2, user3"); + assertThat(trust.getAutoApprovalUsers(), is(java.util.Arrays.asList("user1", "user2", "user3"))); + + trust.setAutoApprovalUsers(" user1 , user2 "); + assertThat(trust.getAutoApprovalUsers(), is(java.util.Arrays.asList("user1", "user2"))); + + trust.setAutoApprovalUsers(""); + assertThat(trust.getAutoApprovalUsers(), org.hamcrest.Matchers.nullValue()); + + trust.setAutoApprovalUsers((String) null); + assertThat(trust.getAutoApprovalUsers(), org.hamcrest.Matchers.nullValue()); + } + + @Test + public void given__externalApproval__when__autoApprovalLabels__then__configuredCorrectly() throws Exception { + ForkPullRequestDiscoveryTrait.TrustExternalApproval trust = + new ForkPullRequestDiscoveryTrait.TrustExternalApproval(); + assertThat(trust.getAutoApprovalLabels(), org.hamcrest.Matchers.nullValue()); + + trust.setAutoApprovalLabelsList(java.util.Arrays.asList("safe-to-build", "approved")); + assertThat(trust.getAutoApprovalLabels(), is(java.util.Arrays.asList("safe-to-build", "approved"))); + assertThat(trust.getAutoApprovalLabelsString(), is("safe-to-build, approved")); + + trust.setAutoApprovalLabelsList(java.util.Collections.emptyList()); + assertThat(trust.getAutoApprovalLabels(), org.hamcrest.Matchers.nullValue()); + + trust.setAutoApprovalLabels("label1, label2"); + assertThat(trust.getAutoApprovalLabels(), is(java.util.Arrays.asList("label1", "label2"))); + + trust.setAutoApprovalLabels((String) null); + assertThat(trust.getAutoApprovalLabels(), org.hamcrest.Matchers.nullValue()); + } + + @Test + public void xstreamExternalApproval() throws Exception { + ForkPullRequestDiscoveryTrait.TrustExternalApproval trust = + new ForkPullRequestDiscoveryTrait.TrustExternalApproval(); + trust.setRequireApprovalForNewCommits(true); + trust.setAutoApprovalLabelsList(java.util.Arrays.asList("safe-to-build", "ci-approved")); + trust.setAutoApprovalUsersList(java.util.Arrays.asList("octocat", "dependabot")); + String xml = new XStream2().toXML(new ForkPullRequestDiscoveryTrait(3, trust)); + assertThat(xml, org.hamcrest.Matchers.containsString("TrustExternalApproval")); + assertThat(xml, org.hamcrest.Matchers.containsString("requireApprovalForNewCommits")); + assertThat(xml, org.hamcrest.Matchers.containsString("autoApprovalLabels")); + assertThat(xml, org.hamcrest.Matchers.containsString("safe-to-build")); + assertThat(xml, org.hamcrest.Matchers.containsString("ci-approved")); + assertThat(xml, org.hamcrest.Matchers.containsString("autoApprovalUsers")); + assertThat(xml, org.hamcrest.Matchers.containsString("octocat")); + assertThat(xml, org.hamcrest.Matchers.containsString("dependabot")); + } } diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalActionTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalActionTest.java new file mode 100644 index 000000000..7a0b5184a --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalActionTest.java @@ -0,0 +1,107 @@ +/* + * The MIT License + * + * Copyright 2026 Olivier Lamy + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package org.jenkinsci.plugins.github_branch_source; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; + +import hudson.model.FreeStyleProject; +import java.io.File; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; + +public class PendingApprovalActionTest { + + @Rule + public JenkinsRule r = new JenkinsRule(); + + @Test + public void approvalDataPersistence() throws Exception { + FreeStyleProject job = r.createFreeStyleProject("test-pr-job"); + + assertThat(PendingApprovalAction.ApprovalData.exists(job), is(false)); + + PendingApprovalAction.ApprovalData data = new PendingApprovalAction.ApprovalData(); + data.state = PendingApprovalAction.ApprovalState.PENDING; + data.save(job); + + assertThat(PendingApprovalAction.ApprovalData.exists(job), is(true)); + assertThat(new File(job.getRootDir(), "pending-approval.xml").exists(), is(true)); + + PendingApprovalAction.ApprovalData loaded = PendingApprovalAction.ApprovalData.load(job); + assertThat(loaded.state, is(PendingApprovalAction.ApprovalState.PENDING)); + assertThat(loaded.approvedBy, nullValue()); + + loaded.state = PendingApprovalAction.ApprovalState.APPROVED; + loaded.approvedBy = "admin"; + loaded.approvedAt = 1234567890L; + loaded.approvedPullHash = "abc123"; + loaded.save(job); + + PendingApprovalAction.ApprovalData reloaded = PendingApprovalAction.ApprovalData.load(job); + assertThat(reloaded.state, is(PendingApprovalAction.ApprovalState.APPROVED)); + assertThat(reloaded.approvedBy, is("admin")); + assertThat(reloaded.approvedAt, is(1234567890L)); + assertThat(reloaded.approvedPullHash, is("abc123")); + + PendingApprovalAction.ApprovalData.delete(job); + assertThat(PendingApprovalAction.ApprovalData.exists(job), is(false)); + } + + @Test + public void actionProperties() { + PendingApprovalAction pending = new PendingApprovalAction( + null, PendingApprovalAction.ApprovalState.PENDING, 42, "fork-author", "deadbeef", false); + assertThat(pending.getState(), is(PendingApprovalAction.ApprovalState.PENDING)); + assertThat(pending.getPrNumber(), is(42)); + assertThat(pending.getPrAuthor(), is("fork-author")); + assertThat(pending.getCurrentPullHash(), is("deadbeef")); + assertThat(pending.isRequireApprovalForNewCommits(), is(false)); + assertThat(pending.getDisplayName(), is(Messages.PendingApprovalAction_displayName())); + assertThat(pending.getIconFileName(), notNullValue()); + assertThat(pending.getUrlName(), is("pendingApproval")); + + PendingApprovalAction approved = new PendingApprovalAction( + null, PendingApprovalAction.ApprovalState.APPROVED, 42, "fork-author", "deadbeef", false); + assertThat(approved.getState(), is(PendingApprovalAction.ApprovalState.APPROVED)); + assertThat(approved.getDisplayName(), is(Messages.PendingApprovalAction_approved())); + assertThat(approved.getIconFileName(), nullValue()); + } + + @Test + public void externalApprovalCause() { + PendingApprovalAction.ExternalApprovalCause cause = new PendingApprovalAction.ExternalApprovalCause(); + assertThat(cause.getShortDescription(), is("External approval granted")); + } + + @Test + public void queueDecisionHandlerAllowsNonMultiBranchJobs() throws Exception { + FreeStyleProject job = r.createFreeStyleProject("regular-job"); + PendingApprovalAction.QueueDecisionHandler handler = new PendingApprovalAction.QueueDecisionHandler(); + assertThat(handler.shouldSchedule(job, java.util.Collections.emptyList()), is(true)); + } +} From 1709f11fe886796a74e7296ac935791cc1074364 Mon Sep 17 00:00:00 2001 From: Olivier Lamy Date: Tue, 25 Aug 2026 08:35:23 +1000 Subject: [PATCH 2/4] Fix approval bypass and auto-approval, add permission check Signed-off-by: Olivier Lamy --- .../ExternalApprovalHelper.java | 143 +++++++++++++++--- .../ForkPullRequestDiscoveryTrait.java | 28 +++- .../PendingApprovalAction.java | 113 +++++++++----- .../PendingApprovalAction/summary.jelly | 2 +- 4 files changed, 220 insertions(+), 66 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/ExternalApprovalHelper.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/ExternalApprovalHelper.java index 17e65fdd0..1bedb7c5f 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/ExternalApprovalHelper.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/ExternalApprovalHelper.java @@ -23,8 +23,14 @@ */ package org.jenkinsci.plugins.github_branch_source; +import com.cloudbees.plugins.credentials.common.StandardCredentials; import edu.umd.cs.findbugs.annotations.CheckForNull; +import hudson.model.Item; import hudson.model.Job; +import java.io.IOException; +import java.util.List; +import java.util.logging.Level; +import java.util.logging.Logger; import jenkins.branch.Branch; import jenkins.branch.BranchProjectFactory; import jenkins.branch.BranchSource; @@ -34,20 +40,27 @@ import jenkins.scm.api.SCMRevision; import jenkins.scm.api.SCMSource; import jenkins.scm.api.trait.SCMSourceTrait; +import org.kohsuke.github.GHLabel; +import org.kohsuke.github.GHPullRequest; +import org.kohsuke.github.GHRepository; +import org.kohsuke.github.GitHub; /** - * Utility class to determine if a branch job requires external approval. + * Helpers for working out whether a branch job needs external approval before it can build, and + * whether a pull request can be approved automatically. */ final class ExternalApprovalHelper { + private static final Logger LOGGER = Logger.getLogger(ExternalApprovalHelper.class.getName()); + private ExternalApprovalHelper() {} /** - * Checks if the given job is a fork pull request in a MultiBranchProject configured - * with {@link ForkPullRequestDiscoveryTrait.TrustExternalApproval}. + * Returns the approval details when {@code job} is a fork pull request in a multibranch project + * that uses the {@link ForkPullRequestDiscoveryTrait.TrustExternalApproval} policy. * * @param job the job to check - * @return approval info if external approval is required, {@code null} otherwise + * @return the approval info, or {@code null} when external approval doesn't apply to this job */ @CheckForNull @SuppressWarnings({"rawtypes", "unchecked"}) @@ -69,7 +82,11 @@ static ExternalApprovalInfo getApprovalInfo(Job job) { if (prHead.getOrigin().equals(SCMHeadOrigin.DEFAULT)) { return null; } - ForkPullRequestDiscoveryTrait.TrustExternalApproval trustPolicy = findTrustPolicy(mp); + GitHubSCMSource source = findSourceWithExternalApproval(mp); + if (source == null) { + return null; + } + ForkPullRequestDiscoveryTrait.TrustExternalApproval trustPolicy = getTrustPolicy(source); if (trustPolicy == null) { return null; } @@ -78,24 +95,35 @@ static ExternalApprovalInfo getApprovalInfo(Job job) { prHead.getNumber(), prHead.getSourceOwner(), currentPullHash, - trustPolicy.isRequireApprovalForNewCommits()); + trustPolicy.isRequireApprovalForNewCommits(), + trustPolicy.getAutoApprovalUsers(), + trustPolicy.getAutoApprovalLabels(), + source, + mp); } + /** Finds the project's {@link GitHubSCMSource} that uses the external-approval policy, if any. */ @CheckForNull @SuppressWarnings("rawtypes") - private static ForkPullRequestDiscoveryTrait.TrustExternalApproval findTrustPolicy(MultiBranchProject mp) { + private static GitHubSCMSource findSourceWithExternalApproval(MultiBranchProject mp) { for (Object src : mp.getSources()) { if (src instanceof BranchSource) { SCMSource source = ((BranchSource) src).getSource(); - if (source instanceof GitHubSCMSource) { - for (SCMSourceTrait trait : ((GitHubSCMSource) source).getTraits()) { - if (trait instanceof ForkPullRequestDiscoveryTrait) { - Object trust = ((ForkPullRequestDiscoveryTrait) trait).getTrust(); - if (trust instanceof ForkPullRequestDiscoveryTrait.TrustExternalApproval) { - return (ForkPullRequestDiscoveryTrait.TrustExternalApproval) trust; - } - } - } + if (source instanceof GitHubSCMSource && getTrustPolicy((GitHubSCMSource) source) != null) { + return (GitHubSCMSource) source; + } + } + } + return null; + } + + @CheckForNull + private static ForkPullRequestDiscoveryTrait.TrustExternalApproval getTrustPolicy(GitHubSCMSource source) { + for (SCMSourceTrait trait : source.getTraits()) { + if (trait instanceof ForkPullRequestDiscoveryTrait) { + Object trust = ((ForkPullRequestDiscoveryTrait) trait).getTrust(); + if (trust instanceof ForkPullRequestDiscoveryTrait.TrustExternalApproval) { + return (ForkPullRequestDiscoveryTrait.TrustExternalApproval) trust; } } } @@ -111,10 +139,65 @@ private static String getCurrentPullHash(BranchProjectFactory factory, Job } return null; } + + /** + * Returns {@code true} when the PR author is on the auto-approval user list. This is just a + * list check with no GitHub call, so it's safe to use from the scheduler. + */ + static boolean isAutoApprovedUser(ExternalApprovalInfo info) { + if (info.autoApprovalUsers == null || info.prAuthor == null) { + return false; + } + for (String user : info.autoApprovalUsers) { + // GitHub logins are case-insensitive. + if (user.equalsIgnoreCase(info.prAuthor)) { + return true; + } + } + return false; + } + + /** + * Decides whether a PR can be approved automatically, either because its author is on the user + * list or because it carries one of the auto-approval labels. Checking labels costs one GitHub + * call, so only call this when first creating the approval record, never from the scheduler. + * + * @param info the approval info + * @return {@code true} if the PR should be auto-approved + */ + static boolean evaluateAutoApproval(ExternalApprovalInfo info) { + if (isAutoApprovedUser(info)) { + return true; + } + if (info.autoApprovalLabels == null || info.autoApprovalLabels.isEmpty() || info.source == null) { + return false; + } + GitHubSCMSource src = info.source; + StandardCredentials credentials = Connector.lookupScanCredentials( + info.context, src.getApiUri(), src.getCredentialsId(), src.getRepoOwner()); + GitHub github = null; + try { + github = Connector.connect(src.getApiUri(), credentials); + GHRepository repo = github.getRepository(src.getRepoOwner() + "/" + src.getRepository()); + GHPullRequest pr = repo.getPullRequest(info.prNumber); + for (GHLabel label : pr.getLabels()) { + if (info.autoApprovalLabels.contains(label.getName())) { + return true; + } + } + } catch (IOException e) { + LOGGER.log(Level.WARNING, "Failed to check auto-approval labels for PR #" + info.prNumber, e); + } finally { + if (github != null) { + Connector.release(github); + } + } + return false; + } } /** - * Holds information about a fork PR that requires external approval. + * A snapshot of a fork PR plus everything needed to decide its approval. */ class ExternalApprovalInfo { final int prNumber; @@ -122,10 +205,34 @@ class ExternalApprovalInfo { final String currentPullHash; final boolean requireApprovalForNewCommits; - ExternalApprovalInfo(int prNumber, String prAuthor, String currentPullHash, boolean requireApprovalForNewCommits) { + @CheckForNull + final List autoApprovalUsers; + + @CheckForNull + final List autoApprovalLabels; + + @CheckForNull + final GitHubSCMSource source; + + @CheckForNull + final Item context; + + ExternalApprovalInfo( + int prNumber, + String prAuthor, + String currentPullHash, + boolean requireApprovalForNewCommits, + @CheckForNull List autoApprovalUsers, + @CheckForNull List autoApprovalLabels, + @CheckForNull GitHubSCMSource source, + @CheckForNull Item context) { this.prNumber = prNumber; this.prAuthor = prAuthor; this.currentPullHash = currentPullHash; this.requireApprovalForNewCommits = requireApprovalForNewCommits; + this.autoApprovalUsers = autoApprovalUsers; + this.autoApprovalLabels = autoApprovalLabels; + this.source = source; + this.context = context; } } diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java index d72691445..2d6d119ad 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java @@ -26,6 +26,7 @@ import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Extension; +import hudson.model.Item; import hudson.util.FormValidation; import hudson.util.ListBoxModel; import java.io.IOException; @@ -34,6 +35,7 @@ import java.util.EnumSet; import java.util.List; import java.util.Set; +import jenkins.model.Jenkins; import jenkins.scm.api.SCMHeadCategory; import jenkins.scm.api.SCMHeadOrigin; import jenkins.scm.api.SCMRevision; @@ -53,6 +55,7 @@ import org.kohsuke.github.GHLabel; import org.kohsuke.github.GHPermissionType; import org.kohsuke.github.GHPullRequest; +import org.kohsuke.stapler.AncestorInPath; import org.kohsuke.stapler.DataBoundConstructor; import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.QueryParameter; @@ -395,9 +398,10 @@ public boolean isApplicableToOrigin(@NonNull Class orig } /** - * An {@link SCMHeadAuthority} that requires external approval before fork pull requests can - * build. Jobs are created as disabled with a pending approval marker. An administrator must - * approve via the UI or API before the job will run. + * An {@link SCMHeadAuthority} that holds fork pull requests back until someone approves them. + * A new fork PR job starts disabled with a pending-approval marker, and an administrator has to + * approve it from the UI or API before it will build. PRs from trusted users or carrying a + * trusted label can be approved automatically. */ public static class TrustExternalApproval extends GitHubForkTrustPolicy { private boolean requireApprovalForNewCommits; @@ -537,8 +541,13 @@ private static List parseCommaSeparated(@CheckForNull String value) { @Override protected boolean checkTrusted(@NonNull GitHubSCMSourceRequest request, @NonNull PullRequestSCMHead head) throws IOException, InterruptedException { - if (autoApprovalUsers != null && autoApprovalUsers.contains(head.getSourceOwner())) { - return true; + if (autoApprovalUsers != null) { + for (String user : autoApprovalUsers) { + // GitHub logins are case-insensitive. + if (user.equalsIgnoreCase(head.getSourceOwner())) { + return true; + } + } } if (autoApprovalLabels != null && !autoApprovalLabels.isEmpty()) { for (GHPullRequest pr : request.getPullRequests()) { @@ -575,7 +584,14 @@ public boolean isApplicableToOrigin(@NonNull Class orig @Restricted(NoExternalUse.class) @SuppressWarnings("unused") // stapler - public FormValidation doCheckAutoApprovalUsers(@QueryParameter String value) { + public FormValidation doCheckAutoApprovalUsers( + @CheckForNull @AncestorInPath Item context, @QueryParameter String value) { + // Only let users who can configure the job (or admins, outside a job) run the check. + if (context == null) { + Jenkins.get().checkPermission(Jenkins.ADMINISTER); + } else { + context.checkPermission(Item.CONFIGURE); + } if (value == null || value.isBlank()) { return FormValidation.ok(); } diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction.java index 51ac59ae0..2b51ae839 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction.java @@ -49,13 +49,16 @@ import org.kohsuke.stapler.interceptor.RequirePOST; /** - * Action displayed on branch jobs that require external approval before building. - * Provides UI elements and API endpoints to approve or reject fork pull requests. + * Shown on a branch job while its fork pull request waits for external approval. Gives an + * administrator the buttons and endpoints to approve or reject the build. */ public class PendingApprovalAction implements Action { private static final Logger LOGGER = Logger.getLogger(PendingApprovalAction.class.getName()); + /** Marker stored as the approver when a PR was approved automatically. */ + private static final String AUTO_APPROVAL = "auto-approval"; + private final transient Job owner; private final ApprovalState state; private final int prNumber; @@ -227,7 +230,7 @@ public enum ApprovalState { APPROVED } - /** Cause indicating a build was triggered by external approval. */ + /** Marks a build as having been triggered by an external approval. */ public static class ExternalApprovalCause extends Cause { @Override public String getShortDescription() { @@ -235,7 +238,7 @@ public String getShortDescription() { } } - /** Persistent approval data stored in the job directory. */ + /** The approval state, saved alongside the job in its directory. */ static class ApprovalData implements Serializable { private static final long serialVersionUID = 1L; @@ -284,7 +287,7 @@ static void delete(Job job) { } /** - * Contributes {@link PendingApprovalAction} to branch jobs that require external approval. + * Attaches a {@link PendingApprovalAction} to any branch job that needs external approval. */ @Extension public static class ActionFactory extends TransientActionFactory { @@ -301,29 +304,7 @@ public Collection createFor(@NonNull Job target) { if (info == null) { return Collections.emptyList(); } - ApprovalData data = ApprovalData.load(target); - if (!ApprovalData.exists(target)) { - data.state = ApprovalState.PENDING; - try { - data.save(target); - } catch (IOException e) { - LOGGER.log(Level.WARNING, "Failed to initialize approval data for " + target.getFullName(), e); - } - } - if (info.requireApprovalForNewCommits - && data.state == ApprovalState.APPROVED - && data.approvedPullHash != null - && !data.approvedPullHash.equals(info.currentPullHash)) { - data.state = ApprovalState.PENDING; - data.approvedBy = null; - data.approvedAt = 0; - data.approvedPullHash = null; - try { - data.save(target); - } catch (IOException e) { - LOGGER.log(Level.WARNING, "Failed to reset approval data for " + target.getFullName(), e); - } - } + ApprovalData data = resolveApprovalData(target, info); return Collections.singletonList(new PendingApprovalAction( target, data.state, @@ -335,7 +316,56 @@ public Collection createFor(@NonNull Job target) { } /** - * Blocks builds of jobs that are pending external approval. + * Loads the approval record, initializing it on first use and re-evaluating it when new commits + * arrive. This is the single writer of the approval state and may make one GitHub API call (to + * check auto-approval labels), so it must not be called from the scheduler. + * + * @param job the branch job + * @param info the approval info + * @return the resolved approval data (persisted if it changed) + */ + private static ApprovalData resolveApprovalData(Job job, ExternalApprovalInfo info) { + ApprovalData data = ApprovalData.load(job); + boolean changed = false; + if (!ApprovalData.exists(job)) { + // First time we see this PR: auto-approve it if it matches the configured users/labels. + if (ExternalApprovalHelper.evaluateAutoApproval(info)) { + data.state = ApprovalState.APPROVED; + data.approvedBy = AUTO_APPROVAL; + data.approvedPullHash = info.currentPullHash; + } else { + data.state = ApprovalState.PENDING; + } + changed = true; + } else if (info.requireApprovalForNewCommits + && data.state == ApprovalState.APPROVED + && data.approvedPullHash != null + && !data.approvedPullHash.equals(info.currentPullHash)) { + // A new commit was pushed after approval: keep it approved only if still auto-approved, + // otherwise require a fresh approval. + if (ExternalApprovalHelper.evaluateAutoApproval(info)) { + data.approvedBy = AUTO_APPROVAL; + data.approvedPullHash = info.currentPullHash; + } else { + data.state = ApprovalState.PENDING; + data.approvedBy = null; + data.approvedAt = 0; + data.approvedPullHash = null; + } + changed = true; + } + if (changed) { + try { + data.save(job); + } catch (IOException e) { + LOGGER.log(Level.WARNING, "Failed to persist approval data for " + job.getFullName(), e); + } + } + return data; + } + + /** + * Holds a job back from building while it is still waiting for external approval. */ @Extension public static class QueueDecisionHandler extends Queue.QueueDecisionHandler { @@ -351,9 +381,19 @@ public boolean shouldSchedule(Queue.Task task, java.util.List actions) { return true; } ApprovalData data = ApprovalData.load(job); - if (data.state == ApprovalState.PENDING) { - PendingApprovalAction helper = - new PendingApprovalAction(job, data.state, info.prNumber, info.prAuthor, null, false); + boolean approved = data.state == ApprovalState.APPROVED; + // A new commit pushed after approval invalidates it, unless the author is an auto-trusted + // user (checked cheaply here so we never hit the GitHub API under the queue lock). + if (approved + && info.requireApprovalForNewCommits + && data.approvedPullHash != null + && !data.approvedPullHash.equals(info.currentPullHash) + && !ExternalApprovalHelper.isAutoApprovedUser(info)) { + approved = false; + } + if (!approved) { + PendingApprovalAction helper = new PendingApprovalAction( + job, ApprovalState.PENDING, info.prNumber, info.prAuthor, null, false); if (!helper.isJobDisabled()) { try { helper.makeDisabled(true); @@ -363,15 +403,6 @@ public boolean shouldSchedule(Queue.Task task, java.util.List actions) { } return false; } - for (Action action : actions) { - if (action instanceof CauseAction) { - for (Cause cause : ((CauseAction) action).getCauses()) { - if (cause instanceof ExternalApprovalCause) { - return true; - } - } - } - } return true; } } diff --git a/src/main/resources/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction/summary.jelly b/src/main/resources/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction/summary.jelly index 3d700b952..4371c1b52 100644 --- a/src/main/resources/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction/summary.jelly +++ b/src/main/resources/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction/summary.jelly @@ -7,7 +7,7 @@
PR #${it.prNumber} from ${it.prAuthor} - (commit ${it.currentPullHash.substring(0, 7)}) + (commit ${it.currentPullHash.length() > 7 ? it.currentPullHash.substring(0, 7) : it.currentPullHash}) From 4a55d5448d7fbb45cab0ee9620d70b45f744fec3 Mon Sep 17 00:00:00 2001 From: Olivier Lamy Date: Thu, 27 Aug 2026 19:44:03 +1000 Subject: [PATCH 3/4] Address review feedback Signed-off-by: Olivier Lamy --- .../ExternalApprovalHelper.java | 48 ++-- .../ForkPullRequestDiscoveryTrait.java | 69 +---- .../PendingApprovalAction.java | 252 +++++++++--------- .../TrustExternalApproval/config.jelly | 2 +- ...llRequestDiscoveryTraitDescriptorTest.java | 81 ++++++ .../GitHubSCMNavigatorTest.java | 14 +- .../PendingApprovalActionTest.java | 7 +- 7 files changed, 261 insertions(+), 212 deletions(-) create mode 100644 src/test/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTraitDescriptorTest.java diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/ExternalApprovalHelper.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/ExternalApprovalHelper.java index 1bedb7c5f..59724801e 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/ExternalApprovalHelper.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/ExternalApprovalHelper.java @@ -46,8 +46,8 @@ import org.kohsuke.github.GitHub; /** - * Helpers for working out whether a branch job needs external approval before it can build, and - * whether a pull request can be approved automatically. + * Works out whether a branch job needs external approval before it can build, and whether its pull + * request can be approved without asking anyone. */ final class ExternalApprovalHelper { @@ -56,11 +56,9 @@ final class ExternalApprovalHelper { private ExternalApprovalHelper() {} /** - * Returns the approval details when {@code job} is a fork pull request in a multibranch project - * that uses the {@link ForkPullRequestDiscoveryTrait.TrustExternalApproval} policy. - * - * @param job the job to check - * @return the approval info, or {@code null} when external approval doesn't apply to this job + * Returns the approval details for a fork pull request job in a multibranch project using the + * {@link ForkPullRequestDiscoveryTrait.TrustExternalApproval} policy, or {@code null} when + * external approval doesn't apply to this job. */ @CheckForNull @SuppressWarnings({"rawtypes", "unchecked"}) @@ -130,20 +128,25 @@ private static ForkPullRequestDiscoveryTrait.TrustExternalApproval getTrustPolic return null; } + /** + * Returns the pull request head as branch indexing last saw it. It has to be the last seen + * revision and not the last built one: branch-api records the built revision only after a build + * has been scheduled, so that one still holds the previous commit while we decide about the new + * one. + */ @CheckForNull @SuppressWarnings({"rawtypes", "unchecked"}) private static String getCurrentPullHash(BranchProjectFactory factory, Job job) { - SCMRevision revision = factory.getRevision(job); - if (revision instanceof PullRequestSCMRevision) { - return ((PullRequestSCMRevision) revision).getPullHash(); - } - return null; + String pullHash = pullHashOf(factory.getLastSeenRevision(job)); + return pullHash != null ? pullHash : pullHashOf(factory.getRevision(job)); } - /** - * Returns {@code true} when the PR author is on the auto-approval user list. This is just a - * list check with no GitHub call, so it's safe to use from the scheduler. - */ + @CheckForNull + private static String pullHashOf(@CheckForNull SCMRevision revision) { + return revision instanceof PullRequestSCMRevision ? ((PullRequestSCMRevision) revision).getPullHash() : null; + } + + /** Returns {@code true} when the PR author is on the auto-approval list. Just a list lookup. */ static boolean isAutoApprovedUser(ExternalApprovalInfo info) { if (info.autoApprovalUsers == null || info.prAuthor == null) { return false; @@ -158,12 +161,9 @@ static boolean isAutoApprovedUser(ExternalApprovalInfo info) { } /** - * Decides whether a PR can be approved automatically, either because its author is on the user - * list or because it carries one of the auto-approval labels. Checking labels costs one GitHub - * call, so only call this when first creating the approval record, never from the scheduler. - * - * @param info the approval info - * @return {@code true} if the PR should be auto-approved + * Decides whether a pull request can be approved without asking anyone, because its author is on + * the user list or it carries one of the auto-approval labels. The label check costs a GitHub + * call, so only ask when first recording the approval or when the commit has moved on. */ static boolean evaluateAutoApproval(ExternalApprovalInfo info) { if (isAutoApprovedUser(info)) { @@ -196,9 +196,7 @@ static boolean evaluateAutoApproval(ExternalApprovalInfo info) { } } -/** - * A snapshot of a fork PR plus everything needed to decide its approval. - */ +/** A snapshot of a fork pull request, with everything needed to decide its approval. */ class ExternalApprovalInfo { final int prNumber; final String prAuthor; diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java index 2d6d119ad..8cb0aa5c9 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait.java @@ -59,6 +59,7 @@ import org.kohsuke.stapler.DataBoundConstructor; import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.QueryParameter; +import org.kohsuke.stapler.verb.POST; /** * A {@link Discovery} trait for GitHub that will discover pull requests from forks of the @@ -399,9 +400,8 @@ public boolean isApplicableToOrigin(@NonNull Class orig /** * An {@link SCMHeadAuthority} that holds fork pull requests back until someone approves them. - * A new fork PR job starts disabled with a pending-approval marker, and an administrator has to - * approve it from the UI or API before it will build. PRs from trusted users or carrying a - * trusted label can be approved automatically. + * The job starts out disabled and only builds once an administrator has approved it. Pull + * requests from a trusted login, or carrying a trusted label, are approved for you. */ public static class TrustExternalApproval extends GitHubForkTrustPolicy { private boolean requireApprovalForNewCommits; @@ -416,60 +416,35 @@ public static class TrustExternalApproval extends GitHubForkTrustPolicy { @DataBoundConstructor public TrustExternalApproval() {} - /** - * Returns whether a new approval is required when new commits are pushed to the PR. - * - * @return {@code true} if approval is required for each new commit. - */ + /** Whether every new commit on the pull request has to be approved again. */ public boolean isRequireApprovalForNewCommits() { return requireApprovalForNewCommits; } - /** - * Sets whether a new approval is required when new commits are pushed to the PR. - * - * @param requireApprovalForNewCommits {@code true} to require re-approval on new commits. - */ @DataBoundSetter public void setRequireApprovalForNewCommits(boolean requireApprovalForNewCommits) { this.requireApprovalForNewCommits = requireApprovalForNewCommits; } - /** - * Returns the list of PR labels that trigger automatic approval. - * - * @return the list of label names, or {@code null} if not configured. - */ + /** The labels that approve a pull request on sight, or {@code null} if none are set. */ @CheckForNull public List getAutoApprovalLabels() { return autoApprovalLabels; } - /** - * Returns the auto-approval labels as a comma-separated string for form binding. - * - * @return comma-separated label names, or {@code null} if not configured. - */ + /** The same labels as one comma-separated string, which is how the config form wants them. */ @CheckForNull public String getAutoApprovalLabelsString() { return autoApprovalLabels == null ? null : String.join(", ", autoApprovalLabels); } - /** - * Sets the list of PR labels from a comma-separated string (Stapler form binding). - * - * @param autoApprovalLabels comma-separated label names. - */ + /** Reads the labels back from the config form, where they arrive comma-separated. */ @DataBoundSetter public void setAutoApprovalLabels(@CheckForNull String autoApprovalLabels) { this.autoApprovalLabels = parseCommaSeparated(autoApprovalLabels); } - /** - * Sets the list of PR labels that trigger automatic approval. - * - * @param autoApprovalLabels the label names to auto-approve. - */ + /** Sets the same labels from a list, for callers that already have one. */ public void setAutoApprovalLabelsList(@CheckForNull List autoApprovalLabels) { if (autoApprovalLabels == null || autoApprovalLabels.isEmpty()) { this.autoApprovalLabels = null; @@ -478,42 +453,25 @@ public void setAutoApprovalLabelsList(@CheckForNull List autoApprovalLab } } - /** - * Returns the list of GitHub user logins that are automatically trusted. - * - * @return the list of user logins, or {@code null} if not configured. - */ + /** The GitHub logins whose pull requests are approved on sight, or {@code null} if none are set. */ @CheckForNull public List getAutoApprovalUsers() { return autoApprovalUsers; } - /** - * Returns the auto-approval users as a comma-separated string for form binding. - * - * @return comma-separated user logins, or {@code null} if not configured. - */ + /** The same logins as one comma-separated string, which is how the config form wants them. */ @CheckForNull public String getAutoApprovalUsersString() { return autoApprovalUsers == null ? null : String.join(", ", autoApprovalUsers); } - /** - * Sets the list of GitHub user logins from a comma-separated string (Stapler form - * binding). - * - * @param autoApprovalUsers comma-separated GitHub login names. - */ + /** Reads the logins back from the config form, where they arrive comma-separated. */ @DataBoundSetter public void setAutoApprovalUsers(@CheckForNull String autoApprovalUsers) { this.autoApprovalUsers = parseCommaSeparated(autoApprovalUsers); } - /** - * Sets the list of GitHub user logins that are automatically trusted. - * - * @param autoApprovalUsers the GitHub login names to auto-approve. - */ + /** Sets the same logins from a list, for callers that already have one. */ public void setAutoApprovalUsersList(@CheckForNull List autoApprovalUsers) { if (autoApprovalUsers == null || autoApprovalUsers.isEmpty()) { this.autoApprovalUsers = null; @@ -584,9 +542,10 @@ public boolean isApplicableToOrigin(@NonNull Class orig @Restricted(NoExternalUse.class) @SuppressWarnings("unused") // stapler + @POST public FormValidation doCheckAutoApprovalUsers( @CheckForNull @AncestorInPath Item context, @QueryParameter String value) { - // Only let users who can configure the job (or admins, outside a job) run the check. + // Only someone who can configure the job, or an admin when there is no job in the path. if (context == null) { Jenkins.get().checkPermission(Jenkins.ADMINISTER); } else { diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction.java index 2b51ae839..b59ebfc61 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalAction.java @@ -32,8 +32,10 @@ import hudson.model.CauseAction; import hudson.model.Item; import hudson.model.Job; -import hudson.model.Queue; -import hudson.model.queue.ScheduleResult; +import hudson.model.Run; +import hudson.model.TaskListener; +import hudson.model.listeners.ItemListener; +import hudson.model.listeners.RunListener; import java.io.File; import java.io.IOException; import java.io.Serializable; @@ -41,16 +43,22 @@ import java.util.Collections; import java.util.logging.Level; import java.util.logging.Logger; +import jenkins.branch.MultiBranchProject; import jenkins.model.Jenkins; +import jenkins.model.ParameterizedJobMixIn; import jenkins.model.TransientActionFactory; import org.kohsuke.stapler.HttpRedirect; import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.StaplerRequest2; -import org.kohsuke.stapler.interceptor.RequirePOST; +import org.kohsuke.stapler.verb.POST; /** - * Shown on a branch job while its fork pull request waits for external approval. Gives an - * administrator the buttons and endpoints to approve or reject the build. + * Shown on a branch job while its fork pull request waits for external approval, with the buttons + * and endpoints an administrator uses to approve or take back that approval. + * + *

What actually holds the pull request back is the job's own disabled flag. Until someone + * approves, the job stays disabled, so neither branch indexing nor a person clicking Build can start + * it. */ public class PendingApprovalAction implements Action { @@ -126,13 +134,8 @@ public boolean isRequireApprovalForNewCommits() { return owner; } - /** - * Approves the pull request, enabling the job and scheduling a build. - * - * @param req the stapler request - * @return redirect to the parent job - */ - @RequirePOST + /** Approves the pull request: enables the job and starts a build. */ + @POST public HttpResponse doApprove(StaplerRequest2 req) { owner.checkPermission(Item.CONFIGURE); try { @@ -143,7 +146,10 @@ public HttpResponse doApprove(StaplerRequest2 req) { data.approvedAt = System.currentTimeMillis(); data.approvedPullHash = currentPullHash; data.save(owner); - enableAndBuild(); + setDisabled(owner, false); + if (ParameterizedJobMixIn.scheduleBuild2(owner, 0, new CauseAction(new ExternalApprovalCause())) == null) { + LOGGER.log(Level.WARNING, "Failed to schedule build for {0}", owner.getFullName()); + } LOGGER.log(Level.INFO, "PR #{0} in {1} approved by {2}", new Object[] { prNumber, owner.getFullName(), approvedBy }); @@ -153,23 +159,15 @@ public HttpResponse doApprove(StaplerRequest2 req) { return new HttpRedirect(".."); } - /** - * Rejects the pull request approval. - * - * @param req the stapler request - * @return redirect to the parent job - */ - @RequirePOST + /** Takes the approval back and disables the job again. */ + @POST public HttpResponse doReject(StaplerRequest2 req) { owner.checkPermission(Item.CONFIGURE); try { ApprovalData data = ApprovalData.load(owner); - data.state = ApprovalState.PENDING; - data.approvedBy = null; - data.approvedAt = 0; - data.approvedPullHash = null; + data.reset(); data.save(owner); - disableJob(); + setDisabled(owner, true); LOGGER.log(Level.INFO, "PR #{0} in {1} rejected by {2}", new Object[] { prNumber, owner.getFullName(), @@ -181,46 +179,25 @@ public HttpResponse doReject(StaplerRequest2 req) { return new HttpRedirect(".."); } - private void enableAndBuild() throws IOException { - if (isJobDisabled()) { - makeDisabled(false); - } - if (owner instanceof Queue.Task) { - ScheduleResult result = Jenkins.get() - .getQueue() - .schedule2((Queue.Task) owner, 0, new CauseAction(new ExternalApprovalCause())); - if (result.isRefused()) { - LOGGER.log(Level.WARNING, "Failed to schedule build for {0}", owner.getFullName()); - } - } + /** Mirrors the approval onto the job. Anything short of an approval leaves it disabled. */ + private static void applyApprovalState(Job job, ApprovalState state) { + setDisabled(job, state != ApprovalState.APPROVED); } - private void disableJob() throws IOException { - if (!isJobDisabled()) { - makeDisabled(true); + private static void setDisabled(Job job, boolean disabled) { + if (!(job instanceof ParameterizedJobMixIn.ParameterizedJob)) { + LOGGER.log(Level.WARNING, "Cannot change the disabled state of {0}", job.getFullName()); + return; } - } - - private boolean isJobDisabled() { - try { - java.lang.reflect.Method m = owner.getClass().getMethod("isDisabled"); - return (boolean) m.invoke(owner); - } catch (ReflectiveOperationException e) { - return false; + ParameterizedJobMixIn.ParameterizedJob project = (ParameterizedJobMixIn.ParameterizedJob) job; + if (project.isDisabled() == disabled) { + return; } - } - - @SuppressWarnings("unchecked") - private void makeDisabled(boolean disabled) throws IOException { - if (owner instanceof hudson.model.AbstractProject) { - ((hudson.model.AbstractProject) owner).makeDisabled(disabled); - } else { - try { - java.lang.reflect.Method m = owner.getClass().getMethod("setDisabled", boolean.class); - m.invoke(owner, disabled); - } catch (ReflectiveOperationException e) { - LOGGER.log(Level.WARNING, "Cannot change disabled state of " + owner.getFullName(), e); - } + try { + // Disabling also cancels anything this job already has queued. + project.makeDisabled(disabled); + } catch (IOException e) { + LOGGER.log(Level.WARNING, "Cannot change the disabled state of " + job.getFullName(), e); } } @@ -252,6 +229,14 @@ static class ApprovalData implements Serializable { @Nullable String approvedPullHash; + /** Drops any approval, sending the pull request back to pending. */ + void reset() { + state = ApprovalState.PENDING; + approvedBy = null; + approvedAt = 0; + approvedPullHash = null; + } + static XmlFile getConfigFile(Job job) { return new XmlFile(new File(job.getRootDir(), "pending-approval.xml")); } @@ -286,9 +271,7 @@ static void delete(Job job) { } } - /** - * Attaches a {@link PendingApprovalAction} to any branch job that needs external approval. - */ + /** Attaches a {@link PendingApprovalAction} to any branch job that needs external approval. */ @Extension public static class ActionFactory extends TransientActionFactory { @@ -297,6 +280,12 @@ public Class type() { return Job.class; } + @NonNull + @Override + public Class actionType() { + return PendingApprovalAction.class; + } + @NonNull @Override public Collection createFor(@NonNull Job target) { @@ -316,19 +305,85 @@ public Collection createFor(@NonNull Job target) { } /** - * Loads the approval record, initializing it on first use and re-evaluating it when new commits - * arrive. This is the single writer of the approval state and may make one GitHub API call (to - * check auto-approval labels), so it must not be called from the scheduler. - * - * @param job the branch job - * @param info the approval info - * @return the resolved approval data (persisted if it changed) + * Puts a newly discovered fork pull request on hold, and catches the jobs that were already + * there when the trust policy got switched on. + */ + @Extension + public static class ApprovalItemListener extends ItemListener { + + @Override + public void onCreated(Item item) { + if (item instanceof Job) { + refresh((Job) item); + } + } + + @Override + public void onUpdated(Item item) { + if (item instanceof MultiBranchProject) { + for (Item child : ((MultiBranchProject) item).getItems()) { + if (child instanceof Job) { + refresh((Job) child); + } + } + } + } + + private static void refresh(Job job) { + ExternalApprovalInfo info = ExternalApprovalHelper.getApprovalInfo(job); + if (info != null) { + applyApprovalState(job, resolveApprovalData(job, info).state); + } + } + } + + /** + * Spends the approval once the build it was granted for has started: the job goes back to + * disabled, so the next commit needs a fresh approval. Only does anything when the trust policy + * asks for approval on new commits. + */ + @Extension + public static class ApprovalSpender extends RunListener> { + + @Override + public void onStarted(Run run, TaskListener listener) { + Job job = run.getParent(); + ExternalApprovalInfo info = ExternalApprovalHelper.getApprovalInfo(job); + if (info == null || !info.requireApprovalForNewCommits) { + return; + } + if (ExternalApprovalHelper.isAutoApprovedUser(info)) { + // Authors on the auto-approval list never have to ask again. + return; + } + ApprovalData data = ApprovalData.load(job); + if (data.state != ApprovalState.APPROVED) { + return; + } + data.reset(); + try { + data.save(job); + } catch (IOException e) { + LOGGER.log(Level.WARNING, "Failed to reset the approval of " + job.getFullName(), e); + return; + } + setDisabled(job, true); + listener.getLogger() + .println("Approval spent: the next build of PR #" + info.prNumber + " needs a new approval."); + } + } + + /** + * Loads the approval record, writing it the first time we see a pull request and looking at it + * again once the approved commit has moved on. This is the only writer of the approval state, + * and it can cost a GitHub call to read labels, so it does nothing at all in between. Whatever + * it changes is saved and mirrored onto the job. */ private static ApprovalData resolveApprovalData(Job job, ExternalApprovalInfo info) { ApprovalData data = ApprovalData.load(job); boolean changed = false; if (!ApprovalData.exists(job)) { - // First time we see this PR: auto-approve it if it matches the configured users/labels. + // First time we see this PR: approve it straight away if it matches the users or labels. if (ExternalApprovalHelper.evaluateAutoApproval(info)) { data.state = ApprovalState.APPROVED; data.approvedBy = AUTO_APPROVAL; @@ -341,16 +396,13 @@ private static ApprovalData resolveApprovalData(Job job, ExternalApprovalI && data.state == ApprovalState.APPROVED && data.approvedPullHash != null && !data.approvedPullHash.equals(info.currentPullHash)) { - // A new commit was pushed after approval: keep it approved only if still auto-approved, - // otherwise require a fresh approval. + // Someone pushed after the approval. It only stays approved if it still auto-approves, + // otherwise it goes back to waiting for a person. if (ExternalApprovalHelper.evaluateAutoApproval(info)) { data.approvedBy = AUTO_APPROVAL; data.approvedPullHash = info.currentPullHash; } else { - data.state = ApprovalState.PENDING; - data.approvedBy = null; - data.approvedAt = 0; - data.approvedPullHash = null; + data.reset(); } changed = true; } @@ -360,50 +412,8 @@ private static ApprovalData resolveApprovalData(Job job, ExternalApprovalI } catch (IOException e) { LOGGER.log(Level.WARNING, "Failed to persist approval data for " + job.getFullName(), e); } + applyApprovalState(job, data.state); } return data; } - - /** - * Holds a job back from building while it is still waiting for external approval. - */ - @Extension - public static class QueueDecisionHandler extends Queue.QueueDecisionHandler { - - @Override - public boolean shouldSchedule(Queue.Task task, java.util.List actions) { - if (!(task instanceof Job)) { - return true; - } - Job job = (Job) task; - ExternalApprovalInfo info = ExternalApprovalHelper.getApprovalInfo(job); - if (info == null) { - return true; - } - ApprovalData data = ApprovalData.load(job); - boolean approved = data.state == ApprovalState.APPROVED; - // A new commit pushed after approval invalidates it, unless the author is an auto-trusted - // user (checked cheaply here so we never hit the GitHub API under the queue lock). - if (approved - && info.requireApprovalForNewCommits - && data.approvedPullHash != null - && !data.approvedPullHash.equals(info.currentPullHash) - && !ExternalApprovalHelper.isAutoApprovedUser(info)) { - approved = false; - } - if (!approved) { - PendingApprovalAction helper = new PendingApprovalAction( - job, ApprovalState.PENDING, info.prNumber, info.prAuthor, null, false); - if (!helper.isJobDisabled()) { - try { - helper.makeDisabled(true); - } catch (IOException e) { - LOGGER.log(Level.WARNING, "Failed to disable " + job.getFullName(), e); - } - } - return false; - } - return true; - } - } } diff --git a/src/main/resources/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait/TrustExternalApproval/config.jelly b/src/main/resources/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait/TrustExternalApproval/config.jelly index ce635d576..5a912be8f 100644 --- a/src/main/resources/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait/TrustExternalApproval/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTrait/TrustExternalApproval/config.jelly @@ -9,6 +9,6 @@ - + diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTraitDescriptorTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTraitDescriptorTest.java new file mode 100644 index 000000000..2996e6777 --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/ForkPullRequestDiscoveryTraitDescriptorTest.java @@ -0,0 +1,81 @@ +package org.jenkinsci.plugins.github_branch_source; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +import java.net.URL; +import java.util.Arrays; +import jenkins.model.Jenkins; +import org.htmlunit.FailingHttpStatusCodeException; +import org.htmlunit.HttpMethod; +import org.htmlunit.Page; +import org.htmlunit.WebRequest; +import org.htmlunit.util.NameValuePair; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.MockAuthorizationStrategy; +import org.kohsuke.stapler.StaplerRequest2; + +/** + * Checks that the auto-approval users form validation only answers POST requests from someone + * allowed to configure it. + */ +public class ForkPullRequestDiscoveryTraitDescriptorTest { + + private static final String CHECK_URL = + "descriptorByName/org.jenkinsci.plugins.github_branch_source.ForkPullRequestDiscoveryTrait$TrustExternalApproval/checkAutoApprovalUsers?value=alice"; + + @Rule + public final JenkinsRule j = new JenkinsRule(); + + @Before + public void setUp() { + j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); + MockAuthorizationStrategy auth = new MockAuthorizationStrategy(); + auth.grant(Jenkins.ADMINISTER).everywhere().to("alice"); + auth.grant(Jenkins.READ).everywhere().toEveryone(); + j.jenkins.setAuthorizationStrategy(auth); + } + + @Test + public void getIsNotAnswered() throws Exception { + try { + Page page = request(HttpMethod.GET, "alice"); + fail("GET should not reach the check method, got " + + page.getWebResponse().getStatusCode()); + } catch (FailingHttpStatusCodeException e) { + assertEquals(404, e.getStatusCode()); + } + } + + @Test + public void postAsAdminIsAnswered() throws Exception { + Page page = request(HttpMethod.POST, "alice"); + assertEquals(200, page.getWebResponse().getStatusCode()); + } + + @Test + public void postAsReadOnlyIsRejected() throws Exception { + // "bob" has only Overall/Read, and there is no job in the path to fall back on. + try { + request(HttpMethod.POST, "bob"); + fail("Should not be able to do that"); + } catch (FailingHttpStatusCodeException e) { + assertEquals(403, e.getStatusCode()); + } + } + + private Page request(HttpMethod method, String userName) throws Exception { + JenkinsRule.WebClient client = j.createWebClient().login(userName); + client.getOptions().setThrowExceptionOnFailingStatusCode(true); + WebRequest request = new WebRequest(new URL(client.getContextPath() + CHECK_URL), method); + request.setAdditionalHeader("Accept", client.getBrowserVersion().getHtmlAcceptHeader()); + if (method == HttpMethod.POST) { + request.setRequestParameters(Arrays.asList(new NameValuePair( + hudson.Functions.getCrumbRequestField(), hudson.Functions.getCrumb((StaplerRequest2) null)))); + } + return client.getPage(request); + } +} diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java index fe88e4142..455378087 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java @@ -585,7 +585,7 @@ public void doFillScanCredentials() throws Exception { @Issue("SECURITY-3808") @Test public void doFillApiUriItemsRequiresPermission() throws Exception { - // Given a GHE endpoint configured and a folder + // A GHE endpoint and a folder to look at it from final GitHubConfiguration ghConfig = GitHubConfiguration.get(); final Endpoint ghe = new Endpoint("https://ghe.example.com/api/v3", "GHE"); final MockFolder folder = r.createFolder(UUID.randomUUID().toString()); @@ -605,10 +605,10 @@ public void doFillApiUriItemsRequiresPermission() throws Exception { final GitHubAppCredentials.DescriptorImpl appCredsD = r.jenkins.getDescriptorByType(GitHubAppCredentials.DescriptorImpl.class); - // When user have 'Read' + // A user with only 'Read' mockStrategy.grant(Jenkins.READ).onRoot().to("readOnly"); try (ACLContext ctx = ACL.as2(User.getById("readOnly", true).impersonate2())) { - // Then GHE URL is never returned + // never sees the GHE URL assertThat(navigatorD.doFillApiUriItems(null), hasSize(0)); assertThat(navigatorD.doFillApiUriItems(folder), hasSize(0)); assertThat(sourceD.doFillApiUriItems(null), hasSize(0)); @@ -617,10 +617,10 @@ public void doFillApiUriItemsRequiresPermission() throws Exception { assertThat(appCredsD.doFillApiUriItems(folder), hasSize(0)); } - // When user have 'Manage' + // A user with 'Manage' mockStrategy.grant(Jenkins.MANAGE).onRoot().to("admin"); try (ACLContext ctx = ACL.as2(User.getById("admin", true).impersonate2())) { - // Then GHE URL is visible with root context, hidden with folder context + // sees it at the root, but not inside a folder assertThat(values(navigatorD.doFillApiUriItems(null)), hasItem(ghe.getApiUri())); assertThat(navigatorD.doFillApiUriItems(folder), hasSize(0)); assertThat(values(sourceD.doFillApiUriItems(null)), hasItem(ghe.getApiUri())); @@ -629,10 +629,10 @@ public void doFillApiUriItemsRequiresPermission() throws Exception { assertThat(appCredsD.doFillApiUriItems(folder), hasSize(0)); } - // When user have 'Configure' on folder + // A user with 'Configure' on the folder mockStrategy.grant(Item.CONFIGURE).onItems(folder).to("configurator"); try (ACLContext ctx = ACL.as2(User.getById("configurator", true).impersonate2())) { - // Then GHE URL is visible with folder context, hidden without + // sees it inside that folder, but not outside it assertThat(navigatorD.doFillApiUriItems(null), hasSize(0)); assertThat(values(navigatorD.doFillApiUriItems(folder)), hasItem(ghe.getApiUri())); assertThat(sourceD.doFillApiUriItems(null), hasSize(0)); diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalActionTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalActionTest.java index 7a0b5184a..513e87b67 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalActionTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/PendingApprovalActionTest.java @@ -99,9 +99,10 @@ public void externalApprovalCause() { } @Test - public void queueDecisionHandlerAllowsNonMultiBranchJobs() throws Exception { + public void regularJobsAreLeftAlone() throws Exception { FreeStyleProject job = r.createFreeStyleProject("regular-job"); - PendingApprovalAction.QueueDecisionHandler handler = new PendingApprovalAction.QueueDecisionHandler(); - assertThat(handler.shouldSchedule(job, java.util.Collections.emptyList()), is(true)); + new PendingApprovalAction.ApprovalItemListener().onCreated(job); + assertThat(job.isDisabled(), is(false)); + assertThat(job.getAction(PendingApprovalAction.class), nullValue()); } } From 58ac01ae9fbbdc382ad76267b62d3c6631f8195a Mon Sep 17 00:00:00 2001 From: Olivier Lamy Date: Fri, 28 Aug 2026 06:55:47 +1000 Subject: [PATCH 4/4] Leave the SECURITY-3808 test comments alone --- .../GitHubSCMNavigatorTest.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java index 455378087..fe88e4142 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java @@ -585,7 +585,7 @@ public void doFillScanCredentials() throws Exception { @Issue("SECURITY-3808") @Test public void doFillApiUriItemsRequiresPermission() throws Exception { - // A GHE endpoint and a folder to look at it from + // Given a GHE endpoint configured and a folder final GitHubConfiguration ghConfig = GitHubConfiguration.get(); final Endpoint ghe = new Endpoint("https://ghe.example.com/api/v3", "GHE"); final MockFolder folder = r.createFolder(UUID.randomUUID().toString()); @@ -605,10 +605,10 @@ public void doFillApiUriItemsRequiresPermission() throws Exception { final GitHubAppCredentials.DescriptorImpl appCredsD = r.jenkins.getDescriptorByType(GitHubAppCredentials.DescriptorImpl.class); - // A user with only 'Read' + // When user have 'Read' mockStrategy.grant(Jenkins.READ).onRoot().to("readOnly"); try (ACLContext ctx = ACL.as2(User.getById("readOnly", true).impersonate2())) { - // never sees the GHE URL + // Then GHE URL is never returned assertThat(navigatorD.doFillApiUriItems(null), hasSize(0)); assertThat(navigatorD.doFillApiUriItems(folder), hasSize(0)); assertThat(sourceD.doFillApiUriItems(null), hasSize(0)); @@ -617,10 +617,10 @@ public void doFillApiUriItemsRequiresPermission() throws Exception { assertThat(appCredsD.doFillApiUriItems(folder), hasSize(0)); } - // A user with 'Manage' + // When user have 'Manage' mockStrategy.grant(Jenkins.MANAGE).onRoot().to("admin"); try (ACLContext ctx = ACL.as2(User.getById("admin", true).impersonate2())) { - // sees it at the root, but not inside a folder + // Then GHE URL is visible with root context, hidden with folder context assertThat(values(navigatorD.doFillApiUriItems(null)), hasItem(ghe.getApiUri())); assertThat(navigatorD.doFillApiUriItems(folder), hasSize(0)); assertThat(values(sourceD.doFillApiUriItems(null)), hasItem(ghe.getApiUri())); @@ -629,10 +629,10 @@ public void doFillApiUriItemsRequiresPermission() throws Exception { assertThat(appCredsD.doFillApiUriItems(folder), hasSize(0)); } - // A user with 'Configure' on the folder + // When user have 'Configure' on folder mockStrategy.grant(Item.CONFIGURE).onItems(folder).to("configurator"); try (ACLContext ctx = ACL.as2(User.getById("configurator", true).impersonate2())) { - // sees it inside that folder, but not outside it + // Then GHE URL is visible with folder context, hidden without assertThat(navigatorD.doFillApiUriItems(null), hasSize(0)); assertThat(values(navigatorD.doFillApiUriItems(folder)), hasItem(ghe.getApiUri())); assertThat(sourceD.doFillApiUriItems(null), hasSize(0));