diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java index 3587ad904b11..ae53118c0228 100644 --- a/core/src/main/java/hudson/model/Executor.java +++ b/core/src/main/java/hudson/model/Executor.java @@ -872,8 +872,16 @@ public HttpResponse doStopBuild(@CheckForNull @QueryParameter(fixEmpty = true) S try { if (executable != null) { if (runExtId == null || runExtId.isEmpty() || ! (executable instanceof Run) - || (executable instanceof Run && runExtId.equals(((Run) executable).getExternalizableId()))) { - getParentOf(executable).getOwnerTask().checkAbortPermission(); + || (runExtId.equals(((Run) executable).getExternalizableId()))) { + final Queue.Task ownerTask = getParentOf(executable).getOwnerTask(); + boolean canAbort = ownerTask.hasAbortPermission(); + if (canAbort && ownerTask instanceof AccessControlled) { + if (!((AccessControlled) ownerTask).hasPermission(Item.READ)) { + // pretend the build does not exist + return HttpResponses.forwardToPreviousPage(); + } + } + ownerTask.checkAbortPermission(); interrupt(); } } diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index d6f9dbc9c133..984d948d0877 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -747,6 +747,9 @@ public boolean cancel(Item item) { @RequirePOST public HttpResponse doCancelItem(@QueryParameter long id) throws IOException, ServletException { Item item = getItem(id); + if (item != null && !hasReadPermission(item, true)) { + item = null; + } if (item != null) { if(item.hasCancelPermission()){ if(cancel(item)) { @@ -798,14 +801,27 @@ public Item[] getItems() { } private List checkPermissionsAndAddToList(List r, Item t) { - if (t.task instanceof AccessControlled) { - AccessControlled taskAC = (AccessControlled) t.task; + // TODO Changing the second arg to 'true' should reveal some tasks currently hidden for no obvious reason + if (hasReadPermission(t.task, false)) { + r.add(t); + } + return r; + } + + private static boolean hasReadPermission(Item t, boolean valueIfNotAccessControlled) { + return hasReadPermission(t.task, valueIfNotAccessControlled); + } + + private static boolean hasReadPermission(Queue.Task t, boolean valueIfNotAccessControlled) { + if (t instanceof AccessControlled) { + AccessControlled taskAC = (AccessControlled) t; if (taskAC.hasPermission(hudson.model.Item.READ) - || taskAC.hasPermission(Permission.READ)) { - r.add(t); + || taskAC.hasPermission(Permission.READ)) { // TODO should be unnecessary given the 'implies' relationship + return true; } + return false; } - return r; + return valueIfNotAccessControlled; } /** diff --git a/core/src/main/resources/lib/hudson/executors.jelly b/core/src/main/resources/lib/hudson/executors.jelly index a5ec8db7ee21..c63b0210aca2 100644 --- a/core/src/main/resources/lib/hudson/executors.jelly +++ b/core/src/main/resources/lib/hudson/executors.jelly @@ -122,7 +122,7 @@ THE SOFTWARE. - + diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 245df48bac30..944c9e571a56 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -1116,7 +1116,7 @@ private void checkCancelOperationUsingUrl(Function urlProvid r.jenkins.setCrumbIssuer(null); r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); r.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy() - .grant(Jenkins.READ, Item.CANCEL).everywhere().to("admin") + .grant(Jenkins.READ, Item.READ, Item.CANCEL).everywhere().to("admin") .grant(Jenkins.READ).everywhere().to("user") ); diff --git a/test/src/test/java/jenkins/security/Security2278Test.java b/test/src/test/java/jenkins/security/Security2278Test.java new file mode 100644 index 000000000000..c5c0478a3869 --- /dev/null +++ b/test/src/test/java/jenkins/security/Security2278Test.java @@ -0,0 +1,146 @@ +package jenkins.security; + +import com.gargoylesoftware.htmlunit.HttpMethod; +import com.gargoylesoftware.htmlunit.Page; +import com.gargoylesoftware.htmlunit.WebRequest; +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import hudson.model.Cause; +import hudson.model.Computer; +import hudson.model.Executor; +import hudson.model.FreeStyleBuild; +import hudson.model.FreeStyleProject; +import hudson.model.Item; +import hudson.model.Queue; +import jenkins.model.Jenkins; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.MockAuthorizationStrategy; +import org.jvnet.hudson.test.MockFolder; +import org.jvnet.hudson.test.SleepBuilder; + +import java.net.URL; +import java.util.List; +import java.util.Objects; +import java.util.concurrent.TimeUnit; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +public class Security2278Test { + + @ClassRule + public static JenkinsRule j = new JenkinsRule(); + private static FreeStyleProject project; + + @BeforeClass + public static void setUp() throws Exception { + j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); + + final MockFolder folder = j.createFolder("foo"); + project = folder.createProject(FreeStyleProject.class, "bar"); + project.getBuildersList().add(new SleepBuilder(TimeUnit.SECONDS.toMillis(600))); + project.save(); + + // can cancel but not read the project, plus it's in a subfolder (unsure whether that is needed) + j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy() + .grant(Jenkins.READ).everywhere().toEveryone() + .grant(Item.CANCEL).onItems(project).toEveryone() + .grant(Jenkins.ADMINISTER).everywhere().toAuthenticated()); + + // one build in progress + project.scheduleBuild2(0, new Cause.UserIdCause("alice")).waitForStart(); + + // one waiting in queue + project.scheduleBuild2(0, new Cause.UserIdCause("alice")); + } + + @Test + public void testUi() throws Exception { + final JenkinsRule.WebClient webClient = j.createWebClient(); + final HtmlPage topPage = webClient.goTo(""); + final String contentAsString = topPage.getWebResponse().getContentAsString(); + assertThat(contentAsString, containsString("Build Executor Status")); + assertThat(contentAsString, containsString("Unknown Task")); + assertThat(contentAsString, not(containsString("job/foo/job/bar"))); + assertThat(contentAsString, not(containsString("icon-stop"))); + } + + @Test + public void testUiWithPermission() throws Exception { + final JenkinsRule.WebClient webClient = j.createWebClient().login("alice"); + final HtmlPage topPage = webClient.goTo(""); + final String contentAsString = topPage.getWebResponse().getContentAsString(); + assertThat(contentAsString, containsString("Build Executor Status")); + assertThat(contentAsString, not(containsString("Unknown Task"))); + assertThat(contentAsString, containsString("job/foo/job/bar")); + assertThat(contentAsString, containsString("icon-stop")); + } + + @Test + public void testQueueCancelWithoutPermission() throws Exception { + final Queue.Item[] items = j.jenkins.getQueue().getItems(); + Assert.assertEquals(1, items.length); + final long id = items[0].getId(); + + final JenkinsRule.WebClient webClient = j.createWebClient(); + webClient.setThrowExceptionOnFailingStatusCode(false); + webClient.setRedirectEnabled(false); + + final Page stopResponse = webClient.getPage(addReferer(webClient.addCrumb(new WebRequest(new URL(j.jenkins.getRootUrl() + "/queue/cancelItem?id=" + id), HttpMethod.POST)), j.jenkins.getRootUrl())); + assertEquals(404, stopResponse.getWebResponse().getStatusCode()); + Assert.assertEquals(1, j.jenkins.getQueue().getItems().length); + } + + @Test + public void testWebMethodWithoutPermission() throws Exception { + final JenkinsRule.WebClient webClient = j.createWebClient(); + webClient.setThrowExceptionOnFailingStatusCode(false); + webClient.setRedirectEnabled(false); + + final Executor busyExecutor = project.getBuilds().stream().findFirst().orElseThrow(() -> new IllegalStateException("expected build")).getExecutor(); + final Computer computer = j.jenkins.toComputer(); + assertNotNull(computer); + final List executors = computer.getExecutors(); + int found = -1; + for (int i = 0; i < computer.getNumExecutors(); i ++) { + if (executors.get(i) == busyExecutor) { + found = i; + break; + } + } + if (found < 0) { + throw new IllegalStateException("didn't find executor"); + } + final Page stopResponse = webClient.getPage(addReferer(webClient.addCrumb(new WebRequest(new URL(j.jenkins.getRootUrl() + "/computer/(master)/executors/" + found + "/stop/"), HttpMethod.POST)), j.jenkins.getRootUrl())); + assertEquals(302, stopResponse.getWebResponse().getStatusCode()); + + final FreeStyleBuild build = project.getBuildByNumber(1); + assertTrue(build.isBuilding()); + assertFalse(Objects.requireNonNull(build.getExecutor()).isInterrupted()); + } + + private WebRequest addReferer(WebRequest request, String referer) { + request.setAdditionalHeader("Referer", referer); + return request; + } + + @AfterClass + public static void tearDown() throws Exception { + // clear out queue + j.jenkins.getQueue().clear(); + + // wait for build to finish aborting + final FreeStyleBuild freeStyleBuild = project.getBuilds().stream().findFirst().orElseThrow(() -> new IllegalStateException("Didn't find build")); + freeStyleBuild.doStop(); + j.jenkins.getQueue().getItem(freeStyleBuild.getQueueId()).getFuture().get(); + } +}