Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[JENKINS-36240] Better implementation of getTrustedRevision #96

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

[JENKINS-36240] Better implementation of getTrustedRevision #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a5e4b64
[FIXED JENKINS-36240] By using a new API we can determine with better…
jglick Dec 16, 2016
e274c4c
Deleting some misleading comments.
jglick Dec 17, 2016
3ab5708
Merge branch 'master' into repo-user-permission-JENKINS-36240
jglick Jan 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 30 additions & 82 deletions 112 src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
import hudson.plugins.git.GitException;
import hudson.plugins.git.GitSCM;
import hudson.plugins.git.Revision;
import hudson.plugins.git.browser.GitRepositoryBrowser;
import hudson.plugins.git.browser.GithubWeb;
import hudson.plugins.git.extensions.GitSCMExtension;
import hudson.plugins.git.extensions.impl.PreBuildMerge;
Expand All @@ -67,7 +66,6 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
import java.util.TreeSet;
import java.util.concurrent.ConcurrentHashMap;
import java.util.logging.Level;
Expand Down Expand Up @@ -102,6 +100,7 @@
import org.kohsuke.github.GHIssueState;
import org.kohsuke.github.GHMyself;
import org.kohsuke.github.GHOrganization;
import org.kohsuke.github.GHPermissionType;
import org.kohsuke.github.GHPullRequest;
import org.kohsuke.github.GHRepository;
import org.kohsuke.github.GHUser;
Expand Down Expand Up @@ -163,11 +162,6 @@ public class GitHubSCMSource extends AbstractGitSCMSource {
* Cache of the official repository HTML URL as reported by {@link GitHub#getRepository(String)}.
*/
private transient URL repositoryUrl;
/**
* The collaborator names used to determine if pull requests are from trusted authors
*/
@CheckForNull
private transient Set<String> collaboratorNames;

/**
* The cache of {@link ObjectMetadataAction} instances for each open PR.
Expand Down Expand Up @@ -422,14 +416,7 @@ protected final void retrieve(@CheckForNull SCMSourceCriteria criteria,
String fullName = repoOwner + "/" + repository;
final GHRepository repo = github.getRepository(fullName);
listener.getLogger().format("Looking up %s%n", HyperlinkNote.encodeTo(repo.getHtmlUrl().toString(), fullName));
try {
repositoryUrl = repo.getHtmlUrl();
collaboratorNames = new HashSet<>(repo.getCollaboratorNames());
} catch (FileNotFoundException e) {
// not permitted
listener.getLogger().println("Not permitted to query list of collaborators, assuming none");
collaboratorNames = Collections.emptySet();
}
repositoryUrl = repo.getHtmlUrl();
doRetrieve(criteria, observer, listener, repo);
listener.getLogger().format("%nDone examining %s%n%n", fullName);
} catch (RateLimitExceededException rle) {
Expand Down Expand Up @@ -549,11 +536,6 @@ private void doRetrieve(SCMSourceCriteria criteria, SCMHeadObserver observer, Ta
}
continue;
}
boolean trusted = collaboratorNames != null
&& collaboratorNames.contains(ghPullRequest.getHead().getRepository().getOwnerName());
if (!trusted) {
listener.getLogger().format(" (not from a trusted source)%n");
}
for (boolean merge : new boolean[] {false, true}) {
String branchName = "PR-" + number;
if (merge && fork) {
Expand Down Expand Up @@ -936,81 +918,47 @@ private void checkout(GitSCM scm, Run<?,?> build, GitClient git, TaskListener li
public SCMRevision getTrustedRevision(SCMRevision revision, TaskListener listener)
throws IOException, InterruptedException {
if (revision instanceof PullRequestSCMRevision) {
/*
* Evaluates whether this pull request is coming from a trusted source.
* Quickest is to check whether the author of the PR
* <a href="https://developer.github.com/v3/repos/collaborators/#check-if-a-user-is-a-collaborator">is a
* collaborator of the repository</a>.
* By checking <a href="https://developer.github.com/v3/repos/collaborators/#list-collaborators">all
* collaborators</a>
* it is possible to further ascertain if they are in a team which was specifically granted push permission,
* but this is potentially expensive as there might be multiple pages of collaborators to retrieve.
* TODO since the GitHub API wrapper currently supports neither, we list all collaborator names and check
* for membership, paying the performance penalty without the benefit of the accuracy.
*/

if (collaboratorNames == null) {
listener.getLogger().format("Connecting to %s to obtain list of collaborators for %s/%s%n",
apiUri == null ? GITHUB_URL : apiUri, repoOwner, repository);
boolean trusted;
PullRequestSCMHead head = (PullRequestSCMHead) revision.getHead();
String sourceOwner = head.getSourceOwner();
if (repoOwner.equals(sourceOwner)) {
// Not a fork, so definitely trusted.
trusted = true;
} else {
StandardCredentials credentials = Connector.lookupScanCredentials(getOwner(), apiUri, scanCredentialsId);
// Github client and validation
GitHub github = Connector.connect(apiUri, credentials);
String fullName = repoOwner + "/" + repository;
final GHRepository repo = github.getRepository(fullName);
try {
github.checkApiUrlValidity();
} catch (HttpException e) {
listener.getLogger().format("It seems %s is unreachable, assuming no trusted collaborators%n",
apiUri == null ? GITHUB_URL : apiUri);
collaboratorNames = Collections.singleton(repoOwner);
}
if (collaboratorNames == null) {
// Input data validation
String credentialsName =
credentials == null ? "anonymous access" : CredentialsNameProvider.name(credentials);
if (credentials != null && !github.isCredentialValid()) {
listener.getLogger().format("Invalid scan credentials %s to connect to %s, "
+ "assuming no trusted collaborators%n",
credentialsName, apiUri == null ? GITHUB_URL : apiUri);
collaboratorNames = Collections.singleton(repoOwner);
GHPermissionType permission = repo.getPermission(sourceOwner);
listener.getLogger().format("Permission check of %s on %s/%s says: %s%n", sourceOwner, repoOwner, repository, permission);
trusted = permission == GHPermissionType.ADMIN || permission == GHPermissionType.WRITE;
} catch (FileNotFoundException x) {
listener.getLogger().format("Failed to do a direct permission check of %s on %s/%s; scan token has no access to this repository, or endpoint removed%n", sourceOwner, repoOwner, repository);
trusted = false; // TODO JENKINS-37608: make this configurable
} catch (HttpException x) {
if (x.getResponseCode() == 403) {
listener.getLogger().format("Not authorized to do a direct permission check of %s on %s/%s; scan token has no access to this repository%n", sourceOwner, repoOwner, repository);
Copy link
Member

Choose a reason for hiding this comment

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

Cache this so you don't retry the request for all PRs, also perhaps we should drop back to the previous behaviour if the permission is missing

Copy link
Member Author

Choose a reason for hiding this comment

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

Cache this so you don't retry the request for all PRs

Not sure why caching would be required; this code should only be run during an actual build of the branch project. We could try to cache 403s per repository × credentialsId, though this cache could become stale for several reasons—different token, perhaps different permissions for a given token, different repository or team settings…

} else {
if (!github.isAnonymous()) {
listener.getLogger()
.format("Connecting to %s using %s%n", apiUri == null ? GITHUB_URL : apiUri,
credentialsName);
} else {
listener.getLogger().format("Connecting to %s with no credentials, anonymous access%n",
apiUri == null ? GITHUB_URL : apiUri);
}

// Input data validation
if (repository == null || repository.isEmpty()) {
collaboratorNames = Collections.singleton(repoOwner);
} else {
String fullName = repoOwner + "/" + repository;
final GHRepository repo;
try {
repo = github.getRepository(fullName);
repositoryUrl = repo.getHtmlUrl();
collaboratorNames = new HashSet<>(repo.getCollaboratorNames());
} catch (FileNotFoundException e) {
listener.getLogger().format("Invalid scan credentials %s to connect to %s, "
+ "assuming no trusted collaborators%n",
credentialsName,
apiUri == null ? GITHUB_URL : apiUri);
collaboratorNames = Collections.singleton(repoOwner);
}
}
listener.getLogger().format("Failed to do a direct permission check of %s on %s/%s: %s%n", sourceOwner, repoOwner, repository, x.getMessage());
}
trusted = false; // TODO ditto
} catch (IOException x) {
listener.getLogger().format("Failed to do a direct permission check of %s on %s/%s: %s%n", sourceOwner, repoOwner, repository, x.getMessage());
trusted = false; // TODO ditto
}
}
PullRequestSCMHead head = (PullRequestSCMHead) revision.getHead();
if (!collaboratorNames.contains(head.getSourceOwner())) {
if (trusted) {
return revision;
} else {
PullRequestSCMRevision rev = (PullRequestSCMRevision) revision;
listener.getLogger().format("Loading trusted files from base branch %s at %s rather than %s%n",
head.getTarget().getName(), rev.getBaseHash(), rev.getPullHash());
return new SCMRevisionImpl(head.getTarget(), rev.getBaseHash());
}
} else {
return revision;
}
return revision;
}

/**
Expand Down
1 change: 0 additions & 1 deletion 1 src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestGHEventSubscriber.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,6 @@ && isApiMatch(((GitHubSCMSource) source).getApiUri())
branchName += "-head";
}
}
// don't care about trusted as SCMHead equality is based on the branch name anyway and
PullRequestSCMHead head = new PullRequestSCMHead(ghPullRequest, branchName, merge);
if (merge) {
// it will take a call to GitHub to get the merge commit, so let the event receiver poll
Expand Down
1 change: 0 additions & 1 deletion 1 src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ private Object readResolve() {
if (merge == null) {
merge = true;
}
// leave trusted at false to be on the safe side
return this;
}

Expand Down
43 changes: 43 additions & 0 deletions 43 src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.model.Action;
import hudson.model.TaskListener;
import hudson.util.StreamTaskListener;
import java.io.File;
import java.io.IOException;
import java.util.HashMap;
Expand All @@ -50,14 +51,17 @@
import org.hamcrest.Matchers;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;

import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
import static com.github.tomakehurst.wiremock.client.WireMock.get;
import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.hasItems;
import static org.hamcrest.Matchers.hasProperty;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.core.Is.is;
Expand Down Expand Up @@ -208,4 +212,43 @@ public void fetchActions() throws Exception {
Matchers.<Action>is(new GitHubLink("icon-github-repo", "https://github.com/cloudbeers/yolo"))));
}

@Ignore("TODO cannot figure out how to get -Dwiremock.record=https://api.github.com/ to work; get a 500 `ZipException: Not in GZIP format` from Jetty. Anyway the scanCredentialsId would need to be set to someone with authority on @cloudbeers.")
@Issue("JENKINS-36240")
@Test
public void getTrustedRevision() throws Exception {
SCMHeadObserver.Collector collector = SCMHeadObserver.collect();
source.fetch(new SCMSourceCriteria() {
@Override
public boolean isHead(@NonNull Probe probe, @NonNull TaskListener listener) throws IOException {
return probe.stat("README.md").getType() == SCMFile.Type.REGULAR_FILE;
}
}, collector, null, null);
Map<String, SCMRevision> revByName = new HashMap<>();
for (Map.Entry<SCMHead, SCMRevision> h : collector.result().entrySet()) {
revByName.put(h.getKey().getName(), h.getValue());
}
assertThat(revByName.keySet(), hasItems("PR-2", "PR-3", "PR-4", "master"));
StreamTaskListener listener = StreamTaskListener.fromStdout();
// branches: always trusted
assertThat(source.getTrustedRevision(revByName.get("master"), listener),
is((SCMRevision) new AbstractGitSCMSource.SCMRevisionImpl(revByName.get("master").getHead(), "8f1314fc3c8284d8c6d5886d473db98f2126071c")));
// PR-4: not a fork
assertThat(source.getTrustedRevision(revByName.get("PR-4"), listener), is((SCMRevision) new PullRequestSCMRevision((PullRequestSCMHead) revByName.get("PR-4").getHead(),
"8f1314fc3c8284d8c6d5886d473db98f2126071c",
"c71007490b065a84377a23d8ec0e43a001afd202"
)));
// PR-2: fork from a user with admin permission
assertThat(source.getTrustedRevision(revByName.get("PR-2"), listener), is((SCMRevision) new PullRequestSCMRevision((PullRequestSCMHead) revByName.get("PR-2").getHead(),
"8f1314fc3c8284d8c6d5886d473db98f2126071c",
"c0e024f89969b976da165eecaa71e09dc60c3da1"
)));
// PR-3: fork from a user with read permission (so untrusted)
assertThat(revByName.get("PR-3"), is((SCMRevision) new PullRequestSCMRevision((PullRequestSCMHead) revByName.get("PR-3").getHead(),
"8f1314fc3c8284d8c6d5886d473db98f2126071c",
"6ef61f938112fb00f64c3445c06c80fdbbd6d414"
)));
assertThat(source.getTrustedRevision(revByName.get("PR-3"), listener),
is((SCMRevision) new AbstractGitSCMSource.SCMRevisionImpl(revByName.get("master").getHead(), "8f1314fc3c8284d8c6d5886d473db98f2126071c")));
}

}
Morty Proxy This is a proxified and sanitized view of the page, visit original site.