Fix authentication check for web hooks without secret token

This commit is contained in:
Robin Müller 2016-09-21 09:01:48 +02:00
parent 59ee882b8d
commit 3823d70e1b
7 changed files with 22 additions and 17 deletions

View File

@ -235,11 +235,6 @@ public class GitLabPushTrigger extends Trigger<Job<?, ?>> {
return secretToken == null ? null : secretToken.getPlainText();
}
public boolean isWebHookAuthorized(String secretToken) {
String plainText = this.secretToken.getPlainText();
return StringUtils.isEmpty(plainText) || StringUtils.equals(plainText, secretToken);
}
// executes when the Trigger receives a push request
public void onPost(final PushHook hook) {
pushHookTriggerHandler.handle(job, hook, ciSkip, branchFilter, mergeRequestLabelFilter);

View File

@ -5,10 +5,11 @@ import com.dabsquared.gitlabjenkins.connection.GitLabConnectionConfig;
import com.dabsquared.gitlabjenkins.webhook.WebHookAction;
import hudson.model.Item;
import hudson.model.Job;
import hudson.security.AccessDeniedException2;
import hudson.security.Messages;
import hudson.security.Permission;
import hudson.util.HttpResponses;
import jenkins.model.Jenkins;
import org.acegisecurity.Authentication;
import org.apache.commons.lang.StringUtils;
import org.kohsuke.stapler.StaplerResponse;
@ -29,10 +30,12 @@ abstract class BuildWebHookAction implements WebHookAction {
private final Item project;
private final String secretToken;
private final Authentication authentication;
public TriggerNotifier(Item project, String secretToken) {
public TriggerNotifier(Item project, String secretToken, Authentication authentication) {
this.project = project;
this.secretToken = secretToken;
this.authentication = authentication;
}
public void run() {
@ -49,10 +52,9 @@ abstract class BuildWebHookAction implements WebHookAction {
private void checkPermission(Permission permission) {
if (((GitLabConnectionConfig) Jenkins.getInstance().getDescriptor(GitLabConnectionConfig.class)).isUseAuthenticatedEndpoint()) {
try {
Jenkins.getInstance().checkPermission(permission);
} catch (AccessDeniedException2 e) {
throw HttpResponses.errorWithoutStack(403, e.getMessage());
if (!Jenkins.getActiveInstance().getACL().hasPermission(authentication, permission)) {
String message = Messages.AccessDeniedException2_MissingPermission(authentication.getName(), permission.group.title+"/"+permission.name);
throw HttpResponses.errorWithoutStack(403, message);
}
}
}

View File

@ -9,6 +9,7 @@ import hudson.model.Item;
import hudson.model.Job;
import hudson.security.ACL;
import hudson.util.HttpResponses;
import jenkins.model.Jenkins;
import java.util.logging.Level;
import java.util.logging.Logger;
@ -52,7 +53,7 @@ public class MergeRequestBuildAction extends BuildWebHookAction {
if (!(project instanceof Job<?, ?>)) {
throw HttpResponses.errorWithoutStack(409, "Merge Request Hook is not supported for this project");
}
ACL.impersonate(ACL.SYSTEM, new TriggerNotifier(project, secretToken) {
ACL.impersonate(ACL.SYSTEM, new TriggerNotifier(project, secretToken, Jenkins.getAuthentication()) {
@Override
protected void performOnPost(GitLabPushTrigger trigger) {
trigger.onPost(mergeRequestHook);

View File

@ -8,6 +8,7 @@ import hudson.model.Item;
import hudson.model.Job;
import hudson.security.ACL;
import hudson.util.HttpResponses;
import jenkins.model.Jenkins;
import org.kohsuke.stapler.StaplerResponse;
import java.util.logging.Level;
@ -36,7 +37,7 @@ public class NoteBuildAction implements WebHookAction {
if (!(project instanceof Job<?, ?>)) {
throw HttpResponses.errorWithoutStack(409, "Note Hook is not supported for this project");
}
ACL.impersonate(ACL.SYSTEM, new BuildWebHookAction.TriggerNotifier(project, secretToken) {
ACL.impersonate(ACL.SYSTEM, new BuildWebHookAction.TriggerNotifier(project, secretToken, Jenkins.getAuthentication()) {
@Override
protected void performOnPost(GitLabPushTrigger trigger) {
trigger.onPost(noteHook);

View File

@ -8,6 +8,7 @@ import hudson.model.Item;
import hudson.model.Job;
import hudson.security.ACL;
import hudson.util.HttpResponses;
import jenkins.model.Jenkins;
import jenkins.plugins.git.GitSCMSource;
import jenkins.scm.api.SCMSource;
import jenkins.scm.api.SCMSourceOwner;
@ -65,7 +66,7 @@ public class PushBuildAction extends BuildWebHookAction {
}
if (project instanceof Job<?, ?>) {
ACL.impersonate(ACL.SYSTEM, new TriggerNotifier(project, secretToken) {
ACL.impersonate(ACL.SYSTEM, new TriggerNotifier(project, secretToken, Jenkins.getAuthentication()) {
@Override
protected void performOnPost(GitLabPushTrigger trigger) {
trigger.onPost(pushHook);

View File

@ -5,6 +5,8 @@ import com.cloudbees.plugins.credentials.CredentialsScope;
import com.cloudbees.plugins.credentials.CredentialsStore;
import com.cloudbees.plugins.credentials.SystemCredentialsProvider;
import com.cloudbees.plugins.credentials.domains.Domain;
import com.dabsquared.gitlabjenkins.GitLabPushTrigger;
import hudson.model.FreeStyleProject;
import hudson.model.Item;
import hudson.security.GlobalMatrixAuthorizationStrategy;
import hudson.util.FormValidation;
@ -35,6 +37,7 @@ import java.util.List;
import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockserver.model.HttpRequest.request;
import static org.mockserver.model.HttpResponse.response;
@ -94,7 +97,9 @@ public class GitLabConnectionConfigTest {
jenkins.get(GitLabConnectionConfig.class).setUseAuthenticatedEndpoint(true);
jenkins.getInstance().setAuthorizationStrategy(new GlobalMatrixAuthorizationStrategy());
URL jenkinsURL = jenkins.getURL();
jenkins.createFreeStyleProject("test");
FreeStyleProject project = jenkins.createFreeStyleProject("test");
GitLabPushTrigger trigger = mock(GitLabPushTrigger.class);
project.addTrigger(trigger);
CloseableHttpClient client = HttpClientBuilder.create().build();
HttpPost request = new HttpPost(jenkinsURL.toExternalForm() + "project/test");

View File

@ -67,11 +67,11 @@ public class PushBuildActionTest {
public void invalidToken() throws IOException {
FreeStyleProject testProject = jenkins.createFreeStyleProject();
when(trigger.getTriggerOpenMergeRequestOnPush()).thenReturn(TriggerOpenMergeRequest.never);
when(trigger.isWebHookAuthorized("test")).thenReturn(false);
when(trigger.getSecretToken()).thenReturn("secret");
testProject.addTrigger(trigger);
exception.expect(HttpResponses.HttpResponseException.class);
new PushBuildAction(testProject, getJson("PushEvent.json"), "test").execute(response);
new PushBuildAction(testProject, getJson("PushEvent.json"), "wrong-secret").execute(response);
verify(trigger, never()).onPost(any(PushHook.class));
}