Skip to content

Commit 5581d0b

Browse files
MarkEWaiteMarkEWaite
committed
[SECURITY-3769]
1 parent 2c01c6b commit 5581d0b

8 files changed

Lines changed: 245 additions & 2 deletions

File tree

‎pom.xml‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
</parent>
1111

1212
<artifactId>PrioritySorter</artifactId>
13-
<version>${changelist}</version>
13+
<version>936.${changelist}</version>
1414
<packaging>hpi</packaging>
1515
<name>Jenkins Priority Sorter Plugin</name>
1616
<url>https://github.com/jenkinsci/priority-sorter-plugin</url>
@@ -109,6 +109,11 @@
109109
<artifactId>credentials</artifactId>
110110
<scope>test</scope>
111111
</dependency>
112+
<dependency>
113+
<groupId>org.jenkins-ci.plugins</groupId>
114+
<artifactId>matrix-auth</artifactId>
115+
<scope>test</scope>
116+
</dependency>
112117
<dependency>
113118
<groupId>org.jenkins-ci.plugins</groupId>
114119
<artifactId>nested-view</artifactId>

‎src/main/java/jenkins/advancedqueue/PriorityConfiguration.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@
6565
import org.kohsuke.stapler.QueryParameter;
6666
import org.kohsuke.stapler.StaplerRequest2;
6767
import org.kohsuke.stapler.StaplerResponse2;
68+
import org.kohsuke.stapler.interceptor.RequirePOST;
6869

6970
/**
7071
* @author Magnus Sandberg
@@ -166,7 +167,12 @@ public ListBoxModel getPriorities() {
166167
return items;
167168
}
168169

170+
@RequirePOST
169171
public void doPriorityConfigSubmit(StaplerRequest2 req, StaplerResponse2 rsp) throws IOException, ServletException {
172+
if (!checkActive()) {
173+
FormApply.success("..").generateResponse(req, rsp, this);
174+
return;
175+
}
170176
jobGroups = new LinkedList<JobGroup>();
171177
id2jobGroup = new HashMap<Integer, JobGroup>();
172178
//

‎src/main/resources/jenkins/advancedqueue/PrioritySorterConfiguration/config.properties‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
Check_if_only_Administrators_should_be_allowed_to_view_and_edit_the_job_priorities=Check if only Administrators should be allowed to view and edit the job priorities
2-
Only_Admins_can_edit_job_priorities=Only Admins can edit job priorities
2+
Only_Admins_can_edit_job_priorities=Only Admins can edit job priorities (<strong>Strongly recommended</strong>)
33
Priority_Sorter=Priority Sorter
44
Strategy=Strategy

‎src/main/resources/jenkins/advancedqueue/PrioritySorterConfiguration/help-onlyAdminsMayEditPriorityConfiguration.html‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<div>
2+
Check this to only allow administrators to view and edit job groups and
3+
other attributes of priority definitions.
4+
5+
<p>
6+
It is <strong>strongly recommended</strong> that this be enabled on
7+
Jenkins controllers with defined privilege groups or many users.
8+
When it is disabled, any user with access to the job priorities page
9+
can modify job groups and other attributes of priority definitions.
10+
</p>
11+
12+
</div>

‎src/test/java/jenkins/advancedqueue/JCasCFivePrioritiesTest.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,16 @@
2424
package jenkins.advancedqueue;
2525

2626
import static org.hamcrest.MatcherAssert.assertThat;
27+
import static org.hamcrest.Matchers.containsString;
2728
import static org.hamcrest.Matchers.is;
29+
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
2830
import static org.junit.jupiter.api.Assertions.assertFalse;
2931

3032
import io.jenkins.plugins.casc.misc.junit.jupiter.AbstractRoundTripTest;
3133
import java.util.List;
3234
import jenkins.advancedqueue.priority.PriorityStrategy;
3335
import jenkins.advancedqueue.sorter.strategy.AbsoluteStrategy;
36+
import org.htmlunit.html.HtmlPage;
3437
import org.jvnet.hudson.test.JenkinsRule;
3538

3639
public class JCasCFivePrioritiesTest extends AbstractRoundTripTest {
@@ -71,6 +74,18 @@ protected void assertConfiguredAsExpected(JenkinsRule j, String configContent) {
7174
List<JobGroup.PriorityStrategyHolder> strategies4 = jobGroups.get(4).getPriorityStrategies();
7275
PriorityStrategy strategy4 = strategies4.get(0).getPriorityStrategy();
7376
assertThat(strategy4.getDescriptor().getDisplayName(), is("Take the priority from property on the job"));
77+
78+
// Confirm that job groups are visible to unauthenticated users if there is no security realm
79+
JenkinsRule.WebClient webClient = j.createWebClient();
80+
assertDoesNotThrow(() -> {
81+
HtmlPage page = webClient.goTo("advanced-build-queue/");
82+
assertThat(page.asNormalizedText(), containsString("Group 4 - priority two"));
83+
});
84+
globalConfig.setOnlyAdminsMayEditPriorityConfiguration(true);
85+
assertDoesNotThrow(() -> {
86+
HtmlPage page = webClient.goTo("advanced-build-queue/");
87+
assertThat(page.asNormalizedText(), containsString("Group 4 - priority two"));
88+
});
7489
}
7590

7691
@Override

‎src/test/java/jenkins/advancedqueue/JCasCSimpleTest.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,15 @@
2424
package jenkins.advancedqueue;
2525

2626
import static org.hamcrest.MatcherAssert.assertThat;
27+
import static org.hamcrest.Matchers.containsString;
2728
import static org.hamcrest.Matchers.is;
29+
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
2830
import static org.junit.jupiter.api.Assertions.assertFalse;
2931

3032
import io.jenkins.plugins.casc.misc.junit.jupiter.AbstractRoundTripTest;
3133
import java.util.List;
3234
import jenkins.advancedqueue.sorter.strategy.AbsoluteStrategy;
35+
import org.htmlunit.html.HtmlPage;
3336
import org.jvnet.hudson.test.JenkinsRule;
3437

3538
public class JCasCSimpleTest extends AbstractRoundTripTest {
@@ -48,6 +51,18 @@ protected void assertConfiguredAsExpected(JenkinsRule j, String configContent) {
4851
assertThat(jobGroups.get(0).getDescription(), is("Group 1 - default priority"));
4952
assertThat(jobGroups.get(0).getPriority(), is(-1));
5053
assertThat(jobGroups.get(0).getJobGroupStrategy().getDescriptor().getDisplayName(), is("All Jobs"));
54+
55+
// Confirm that job groups are visible to unauthenticated users if there is no security realm
56+
JenkinsRule.WebClient webClient = j.createWebClient();
57+
assertDoesNotThrow(() -> {
58+
HtmlPage page = webClient.goTo("advanced-build-queue/");
59+
assertThat(page.asNormalizedText(), containsString("Group 1 - default priority"));
60+
});
61+
globalConfig.setOnlyAdminsMayEditPriorityConfiguration(true);
62+
assertDoesNotThrow(() -> {
63+
HtmlPage page = webClient.goTo("advanced-build-queue/");
64+
assertThat(page.asNormalizedText(), containsString("Group 1 - default priority"));
65+
});
5166
}
5267

5368
@Override

‎src/test/java/jenkins/advancedqueue/Security3769Test.java‎

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
/*
2+
* The MIT License
3+
*
4+
* Copyright 2026 Mark Waite.
5+
*
6+
* Permission is hereby granted, free of charge, to any person obtaining a copy
7+
* of this software and associated documentation files (the "Software"), to deal
8+
* in the Software without restriction, including without limitation the rights
9+
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
10+
* copies of the Software, and to permit persons to whom the Software is
11+
* furnished to do so, subject to the following conditions:
12+
*
13+
* The above copyright notice and this permission notice shall be included in
14+
* all copies or substantial portions of the Software.
15+
*
16+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
17+
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
18+
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
19+
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
20+
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
21+
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
22+
* THE SOFTWARE.
23+
*/
24+
package jenkins.advancedqueue;
25+
26+
import static org.hamcrest.MatcherAssert.assertThat;
27+
import static org.hamcrest.Matchers.containsString;
28+
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
29+
import static org.junit.jupiter.api.Assertions.assertThrows;
30+
31+
import jenkins.model.Jenkins;
32+
import org.htmlunit.FailingHttpStatusCodeException;
33+
import org.htmlunit.html.HtmlPage;
34+
import org.junit.jupiter.api.BeforeEach;
35+
import org.junit.jupiter.api.Test;
36+
import org.jvnet.hudson.test.JenkinsRule;
37+
import org.jvnet.hudson.test.MockAuthorizationStrategy;
38+
import org.jvnet.hudson.test.junit.jupiter.WithJenkins;
39+
40+
@WithJenkins
41+
public class Security3769Test {
42+
43+
private JenkinsRule j;
44+
private JenkinsRule.WebClient wc;
45+
private final String ADMIN = "admin";
46+
private final String USER = "user";
47+
private final String MANAGER = "manager";
48+
private final String READONLY = "readonly";
49+
private final String SYSTEM_READONLY = "system-readonly";
50+
private final String MANAGER_READONLY = "manager-readonly";
51+
52+
@BeforeEach
53+
void setUp(JenkinsRule rule) {
54+
j = rule;
55+
wc = j.createWebClient();
56+
wc.getOptions().setPrintContentOnFailingStatusCode(false);
57+
j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
58+
j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy()
59+
// full access
60+
.grant(Jenkins.ADMINISTER)
61+
.everywhere()
62+
.to(ADMIN)
63+
64+
// Read access
65+
.grant(Jenkins.READ)
66+
.everywhere()
67+
.to(USER)
68+
69+
// Read and Manage
70+
.grant(Jenkins.READ)
71+
.everywhere()
72+
.to(MANAGER)
73+
.grant(Jenkins.MANAGE)
74+
.everywhere()
75+
.to(MANAGER)
76+
77+
// Read
78+
.grant(Jenkins.READ)
79+
.everywhere()
80+
.to(READONLY)
81+
82+
// Read and System read
83+
.grant(Jenkins.READ)
84+
.everywhere()
85+
.to(SYSTEM_READONLY)
86+
.grant(Jenkins.SYSTEM_READ)
87+
.everywhere()
88+
.to(SYSTEM_READONLY)
89+
90+
// Read, Manage and System read
91+
.grant(Jenkins.READ)
92+
.everywhere()
93+
.to(MANAGER_READONLY)
94+
.grant(Jenkins.MANAGE)
95+
.everywhere()
96+
.to(MANAGER_READONLY)
97+
.grant(Jenkins.SYSTEM_READ)
98+
.everywhere()
99+
.to(MANAGER_READONLY));
100+
}
101+
102+
@Test
103+
void jobGroupsVisibleWhenOnlyAdminsMayEditPriorityConfigurationIsTrue() throws Exception {
104+
PrioritySorterConfiguration globalConfig = PrioritySorterConfiguration.get();
105+
globalConfig.setOnlyAdminsMayEditPriorityConfiguration(true);
106+
107+
assertThrows(FailingHttpStatusCodeException.class, () -> {
108+
wc.goTo("advanced-build-queue/");
109+
});
110+
111+
wc.login(READONLY);
112+
assertThrows(FailingHttpStatusCodeException.class, () -> {
113+
wc.goTo("advanced-build-queue/");
114+
});
115+
116+
wc.login(USER);
117+
assertThrows(FailingHttpStatusCodeException.class, () -> {
118+
wc.goTo("advanced-build-queue/");
119+
});
120+
121+
wc.login(SYSTEM_READONLY);
122+
assertThrows(FailingHttpStatusCodeException.class, () -> {
123+
wc.goTo("advanced-build-queue/");
124+
});
125+
126+
wc.login(MANAGER_READONLY);
127+
assertThrows(FailingHttpStatusCodeException.class, () -> {
128+
wc.goTo("advanced-build-queue/");
129+
});
130+
131+
wc.login(MANAGER);
132+
assertThrows(FailingHttpStatusCodeException.class, () -> {
133+
wc.goTo("advanced-build-queue/");
134+
});
135+
136+
wc.login(ADMIN);
137+
assertDoesNotThrow(() -> {
138+
HtmlPage page = wc.goTo("advanced-build-queue/");
139+
assertThat(page.asNormalizedText(), containsString("Job Priorities"));
140+
});
141+
}
142+
143+
@Test
144+
void jobGroupsVisibleWhenOnlyAdminsMayEditPriorityConfigurationIsFalse() throws Exception {
145+
PrioritySorterConfiguration globalConfig = PrioritySorterConfiguration.get();
146+
globalConfig.setOnlyAdminsMayEditPriorityConfiguration(false);
147+
148+
assertThrows(FailingHttpStatusCodeException.class, () -> {
149+
wc.goTo("advanced-build-queue/");
150+
});
151+
152+
wc.login(READONLY);
153+
assertDoesNotThrow(() -> {
154+
HtmlPage page = wc.goTo("advanced-build-queue/");
155+
assertThat(page.asNormalizedText(), containsString("Job Priorities"));
156+
});
157+
158+
wc.login(SYSTEM_READONLY);
159+
assertDoesNotThrow(() -> {
160+
HtmlPage page = wc.goTo("advanced-build-queue/");
161+
assertThat(page.asNormalizedText(), containsString("Job Priorities"));
162+
});
163+
164+
wc.login(USER);
165+
assertDoesNotThrow(() -> {
166+
HtmlPage page = wc.goTo("advanced-build-queue/");
167+
assertThat(page.asNormalizedText(), containsString("Job Priorities"));
168+
});
169+
170+
wc.login(MANAGER_READONLY);
171+
assertDoesNotThrow(() -> {
172+
HtmlPage page = wc.goTo("advanced-build-queue/");
173+
assertThat(page.asNormalizedText(), containsString("Job Priorities"));
174+
});
175+
176+
wc.login(MANAGER);
177+
assertDoesNotThrow(() -> {
178+
HtmlPage page = wc.goTo("advanced-build-queue/");
179+
assertThat(page.asNormalizedText(), containsString("Job Priorities"));
180+
});
181+
182+
wc.login(ADMIN);
183+
assertDoesNotThrow(() -> {
184+
HtmlPage page = wc.goTo("advanced-build-queue/");
185+
assertThat(page.asNormalizedText(), containsString("Job Priorities"));
186+
});
187+
}
188+
}

‎src/test/java/jenkins/advancedqueue/sorter/StringFormattingPerformanceTest.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import org.junit.jupiter.api.AfterEach;
4343
import org.junit.jupiter.api.BeforeAll;
4444
import org.junit.jupiter.api.BeforeEach;
45+
import org.junit.jupiter.api.Disabled;
4546
import org.junit.jupiter.api.Test;
4647
import org.jvnet.hudson.test.JenkinsRule;
4748
import org.jvnet.hudson.test.junit.jupiter.WithJenkins;
@@ -136,6 +137,7 @@ public void benchmarkLazyLoggingPerformance() throws Exception {
136137
// System.out.printf(" Performance Ratio: %.2fx%n", ratio);
137138
}
138139

140+
@Disabled("Fails occasionally, benchmark values are not an issue")
139141
@Test
140142
public void benchmarkDecisionLogFormatting() throws Exception {
141143
// Compare old StringBuilder approach vs new String.join approach

0 commit comments

Comments
 (0)