Skip to content

Commit 66d7a36

Browse files
LakshyaBaganiLakshyaBagani
committed
Improve AdministrativeMonitor snooze validation and cleanup
1 parent 0cbfea5 commit 66d7a36

File tree

6 files changed

+303
-42
lines changed

6 files changed

+303
-42
lines changed

‎core/src/main/java/hudson/model/AdministrativeMonitor.java‎

Lines changed: 89 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -57,33 +57,42 @@
5757
* monitoring and activate/deactivate the monitor accordingly. Sometimes
5858
* this can be done by updating a flag from code (see {@link SCMTrigger}
5959
* for one such example), while other times it's more convenient to do
60-
* so by running some code periodically (for this, use {@link TimerTrigger#timer})
60+
* so by running some code periodically (for this, use
61+
* {@link TimerTrigger#timer})
6162
*
6263
* <p>
63-
* {@link AdministrativeMonitor}s are bound to URL by {@link Jenkins#getAdministrativeMonitor(String)}.
64+
* {@link AdministrativeMonitor}s are bound to URL by
65+
* {@link Jenkins#getAdministrativeMonitor(String)}.
6466
* See {@link #getUrl()}.
6567
*
6668
* <h3>Views</h3>
6769
* <dl>
6870
* <dt>message.jelly</dt>
6971
* <dd>
70-
* If {@link #isActivated()} returns true, Jenkins will use the {@code message.jelly}
72+
* If {@link #isActivated()} returns true, Jenkins will use the
73+
* {@code message.jelly}
7174
* view of this object to render the warning text. This happens in the
7275
* {@code http://SERVER/jenkins/manage} page. This view should typically render
73-
* a DIV box with class='alert alert-danger' or class='alert alert-warning' with a human-readable text
76+
* a DIV box with class='alert alert-danger' or class='alert alert-warning' with
77+
* a human-readable text
7478
* inside it. It often also contains a link to a page that provides more details
7579
* about the problem.<br>
76-
* Additionally 2 numbers are shown in the Jenkins header of administrators, one with the number or active
77-
* non-security relevant monitors and one with the number of active security relevant monitors.
80+
* Additionally 2 numbers are shown in the Jenkins header of administrators, one
81+
* with the number or active
82+
* non-security relevant monitors and one with the number of active security
83+
* relevant monitors.
7884
* </dd>
7985
* </dl>
8086
*
8187
* <h3>Use with System Read permission</h3>
8288
* <p>
83-
* By default administrative monitors are visible only to users with Administer permission.
84-
* Users with {@link Jenkins#SYSTEM_READ} permission can access administrative monitors that override {@link #getRequiredPermission()}.
85-
* Care needs to be taken to ensure users with that permission don't have access to actions modifying system state.
86-
* For more details, see {@link #getRequiredPermission()}.
89+
* By default administrative monitors are visible only to users with Administer
90+
* permission.
91+
* Users with {@link Jenkins#SYSTEM_READ} permission can access administrative
92+
* monitors that override {@link #getRequiredPermission()}.
93+
* Care needs to be taken to ensure users with that permission don't have access
94+
* to actions modifying system state.
95+
* For more details, see {@link #getRequiredPermission()}.
8796
* </p>
8897
*
8998
* @author Kohsuke Kawaguchi
@@ -94,7 +103,8 @@ public abstract class AdministrativeMonitor extends AbstractModelObject implemen
94103
private static final Logger LOGGER = Logger.getLogger(AdministrativeMonitor.class.getName());
95104

96105
/**
97-
* Human-readable ID of this monitor, which needs to be unique within the system.
106+
* Human-readable ID of this monitor, which needs to be unique within the
107+
* system.
98108
*
99109
* <p>
100110
* This ID is used to remember persisted setting for this monitor,
@@ -111,7 +121,8 @@ protected AdministrativeMonitor() {
111121
}
112122

113123
/**
114-
* Returns the URL of this monitor, relative to the context path, like "administrativeMonitor/foobar".
124+
* Returns the URL of this monitor, relative to the context path, like
125+
* "administrativeMonitor/foobar".
115126
*/
116127
public String getUrl() {
117128
return "administrativeMonitor/" + id;
@@ -143,10 +154,12 @@ public void disable(boolean value) throws IOException {
143154
}
144155

145156
/**
146-
* Returns true if this monitor {@link #disable(boolean) isn't disabled} earlier.
157+
* Returns true if this monitor {@link #disable(boolean) isn't disabled}
158+
* earlier.
147159
*
148160
* <p>
149-
* This flag implements the ability for the admin to say "no thank you" to the monitor that
161+
* This flag implements the ability for the admin to say "no thank you" to the
162+
* monitor that
150163
* he wants to ignore.
151164
*/
152165
public boolean isEnabled() {
@@ -165,9 +178,11 @@ public boolean isSnoozed() {
165178
if (expiry == null) {
166179
return false;
167180
}
181+
168182
long now = System.currentTimeMillis();
183+
184+
// Clean up if expired
169185
if (now >= expiry) {
170-
// Cleanup expired entry to prevent memory leak
171186
try {
172187
AbstractCIBase jenkins = Jenkins.get();
173188
Map<String, Long> map = jenkins.getSnoozedAdministrativeMonitors();
@@ -176,23 +191,34 @@ public boolean isSnoozed() {
176191
jenkins.save();
177192
}
178193
} catch (IOException e) {
179-
LOGGER.log(Level.WARNING, "Failed to cleanup expired snooze for " + id, e);
194+
LOGGER.log(Level.WARNING, "Failed to clean up expired snooze for " + id, e);
180195
}
181196
return false;
182197
}
198+
183199
return true;
184200
}
185201

186202
/**
203+
* Snooze this monitor for a specified duration.
204+
*
205+
* @param durationMs duration in milliseconds
206+
* @throws IOException if unable to save
207+
* @throws IllegalArgumentException if duration is invalid
187208
* @since 2.549
188209
*/
189210
public void snooze(long durationMs) throws IOException {
211+
// Validate input
190212
if (durationMs <= 0) {
191-
throw new IllegalArgumentException("Duration must be positive");
213+
throw new IllegalArgumentException("Snooze duration must be positive");
192214
}
193-
if (durationMs > 365L * 24 * 60 * 60 * 1000) {
194-
throw new IllegalArgumentException("Duration exceeds maximum (1 year)");
215+
216+
// Max 1 year
217+
long maxDuration = 365L * 24 * 60 * 60 * 1000;
218+
if (durationMs > maxDuration) {
219+
throw new IllegalArgumentException("Snooze duration cannot exceed 1 year");
195220
}
221+
196222
long expiryTime = System.currentTimeMillis() + durationMs;
197223
AbstractCIBase jenkins = Jenkins.get();
198224
Map<String, Long> map = jenkins.getSnoozedAdministrativeMonitors();
@@ -275,36 +301,52 @@ public void doSnooze(StaplerRequest2 req, StaplerResponse2 rsp) throws IOExcepti
275301

276302
/**
277303
* Required permission to view this admin monitor.
278-
* By default {@link Jenkins#ADMINISTER}, but {@link Jenkins#SYSTEM_READ} or {@link Jenkins#MANAGE} are also supported.
304+
* By default {@link Jenkins#ADMINISTER}, but {@link Jenkins#SYSTEM_READ} or
305+
* {@link Jenkins#MANAGE} are also supported.
279306
* <p>
280-
* Changing this permission check to return {@link Jenkins#SYSTEM_READ} will make the active
281-
* administrative monitor appear on {@link ManageJenkinsAction} to users without Administer permission.
282-
* {@link #doDisable(StaplerRequest2, StaplerResponse2)} will still always require Administer permission.
307+
* Changing this permission check to return {@link Jenkins#SYSTEM_READ} will
308+
* make the active
309+
* administrative monitor appear on {@link ManageJenkinsAction} to users without
310+
* Administer permission.
311+
* {@link #doDisable(StaplerRequest2, StaplerResponse2)} will still always
312+
* require Administer permission.
283313
* </p>
284314
* <p>
285-
* This method only allows for a single permission to be returned. If more complex permission checks are required,
286-
* override {@link #checkRequiredPermission()} and {@link #hasRequiredPermission()} instead.
315+
* This method only allows for a single permission to be returned. If more
316+
* complex permission checks are required,
317+
* override {@link #checkRequiredPermission()} and
318+
* {@link #hasRequiredPermission()} instead.
287319
* </p>
288320
* <p>
289-
* Implementers need to ensure that {@code doAct} and other web methods perform necessary permission checks:
290-
* Users with System Read permissions are expected to be limited to read-only access.
291-
* Form UI elements that change system state, e.g. toggling a feature on or off, need to be hidden from users
292-
* lacking Administer permission.
321+
* Implementers need to ensure that {@code doAct} and other web methods perform
322+
* necessary permission checks:
323+
* Users with System Read permissions are expected to be limited to read-only
324+
* access.
325+
* Form UI elements that change system state, e.g. toggling a feature on or off,
326+
* need to be hidden from users
327+
* lacking Administer permission.
293328
* </p>
329+
*
294330
* @since 2.233
295-
* @deprecated Callers should use {@link #checkRequiredPermission()} or {@link #hasRequiredPermission()}.
331+
* @deprecated Callers should use {@link #checkRequiredPermission()} or
332+
* {@link #hasRequiredPermission()}.
296333
*/
297334
@Deprecated
298335
public Permission getRequiredPermission() {
299336
return Jenkins.ADMINISTER;
300337
}
301338

302339
/**
303-
* Checks if the current user has the minimum required permission to view this administrative monitor.
340+
* Checks if the current user has the minimum required permission to view this
341+
* administrative monitor.
304342
* <p>
305-
* Subclasses may override this method and {@link #hasRequiredPermission()} instead of {@link #getRequiredPermission()} to perform more complex permission checks,
306-
* for example, checking either {@link Jenkins#MANAGE} or {@link Jenkins#SYSTEM_READ}.
343+
* Subclasses may override this method and {@link #hasRequiredPermission()}
344+
* instead of {@link #getRequiredPermission()} to perform more complex
345+
* permission checks,
346+
* for example, checking either {@link Jenkins#MANAGE} or
347+
* {@link Jenkins#SYSTEM_READ}.
307348
* </p>
349+
*
308350
* @see #getRequiredPermission()
309351
* @see #hasRequiredPermission()
310352
* @since 2.468
@@ -314,11 +356,16 @@ public void checkRequiredPermission() {
314356
}
315357

316358
/**
317-
* Checks if the current user has the minimum required permission to view this administrative monitor.
359+
* Checks if the current user has the minimum required permission to view this
360+
* administrative monitor.
318361
* <p>
319-
* Subclasses may override this method and {@link #checkRequiredPermission} instead of {@link #getRequiredPermission()} to perform more complex permission checks,
320-
* for example, checking either {@link Jenkins#MANAGE} or {@link Jenkins#SYSTEM_READ}.
362+
* Subclasses may override this method and {@link #checkRequiredPermission}
363+
* instead of {@link #getRequiredPermission()} to perform more complex
364+
* permission checks,
365+
* for example, checking either {@link Jenkins#MANAGE} or
366+
* {@link Jenkins#SYSTEM_READ}.
321367
* </p>
368+
*
322369
* @see #getRequiredPermission()
323370
* @see #checkRequiredPermission()
324371
* @since 2.468
@@ -328,9 +375,11 @@ public boolean hasRequiredPermission() {
328375
}
329376

330377
/**
331-
* Checks if the current user has the minimum required permission to view any administrative monitor.
378+
* Checks if the current user has the minimum required permission to view any
379+
* administrative monitor.
332380
*
333-
* @return true if the current user has the minimum required permission to view any administrative monitor.
381+
* @return true if the current user has the minimum required permission to view
382+
* any administrative monitor.
334383
*
335384
* @since 2.468
336385
*/
@@ -339,7 +388,8 @@ public static boolean hasPermissionToDisplay() {
339388
}
340389

341390
/**
342-
* Ensure that URLs in this administrative monitor are only accessible to users with {@link #getRequiredPermission()}.
391+
* Ensure that URLs in this administrative monitor are only accessible to users
392+
* with {@link #getRequiredPermission()}.
343393
*/
344394
@Override
345395
@Restricted(NoExternalUse.class)

‎core/src/main/resources/hudson/model/Messages.properties‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,3 +427,8 @@ ManagementLink.Category.UNCATEGORIZED=Uncategorized
427427
FileParameterValue.IndexTitle=File Parameters
428428

429429
UserPreferencesProperty.DisplayName=Preferences
430+
431+
Snooze=Snooze
432+
Snooze.duration.label=Snooze duration in minutes
433+
Snooze.duration.placeholder=Minutes
434+
Snooze.duration.title=Enter duration in minutes (max 525600 = 1 year)

‎core/src/main/resources/jenkins/diagnostics/ControllerExecutorsAgents/message.jelly‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,21 @@ THE SOFTWARE.
3838
<input type="hidden" name="${h.crumbRequestField}" value="${h.getCrumb(request)}"/>
3939
</j:if>
4040
<input type="hidden" name="duration" value="custom"/>
41-
<input type="number" name="customMinutes" placeholder="Minutes" min="1" style="width: 80px;" class="jenkins-input" required="required"/>
41+
<label for="snooze-minutes-${it.id}" class="jenkins-visually-hidden">
42+
${%Snooze.duration.label}
43+
</label>
44+
<input type="number"
45+
id="snooze-minutes-${it.id}"
46+
name="customMinutes"
47+
placeholder="${%Snooze.duration.placeholder}"
48+
min="1"
49+
max="525600"
50+
step="1"
51+
style="width: 80px;"
52+
class="jenkins-input"
53+
required="required"
54+
aria-label="${%Snooze.duration.label}"
55+
title="${%Snooze.duration.title}"/>
4256
<f:submit value="${%Snooze}"/>
4357
</form>
4458
</div>

‎core/src/main/resources/jenkins/diagnostics/ControllerExecutorsNoAgents/message.jelly‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,21 @@ THE SOFTWARE.
3939
<input type="hidden" name="${h.crumbRequestField}" value="${h.getCrumb(request)}"/>
4040
</j:if>
4141
<input type="hidden" name="duration" value="custom"/>
42-
<input type="number" name="customMinutes" placeholder="Minutes" min="1" style="width: 80px;" class="jenkins-input" required="required"/>
42+
<label for="snooze-minutes-${it.id}" class="jenkins-visually-hidden">
43+
${%Snooze.duration.label}
44+
</label>
45+
<input type="number"
46+
id="snooze-minutes-${it.id}"
47+
name="customMinutes"
48+
placeholder="${%Snooze.duration.placeholder}"
49+
min="1"
50+
max="525600"
51+
step="1"
52+
style="width: 80px;"
53+
class="jenkins-input"
54+
required="required"
55+
aria-label="${%Snooze.duration.label}"
56+
title="${%Snooze.duration.title}"/>
4357
<f:submit value="${%Snooze}"/>
4458
</form>
4559
</div>

‎core/src/main/resources/jenkins/model/BuiltInNodeMigration/message.jelly‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,21 @@
1414
<input type="hidden" name="${h.crumbRequestField}" value="${h.getCrumb(request)}"/>
1515
</j:if>
1616
<input type="hidden" name="duration" value="custom"/>
17-
<input type="number" name="customMinutes" placeholder="Minutes" min="1" style="width: 80px;" class="jenkins-input" required="required"/>
17+
<label for="snooze-minutes-${it.id}" class="jenkins-visually-hidden">
18+
${%Snooze.duration.label}
19+
</label>
20+
<input type="number"
21+
id="snooze-minutes-${it.id}"
22+
name="customMinutes"
23+
placeholder="${%Snooze.duration.placeholder}"
24+
min="1"
25+
max="525600"
26+
step="1"
27+
style="width: 80px;"
28+
class="jenkins-input"
29+
required="required"
30+
aria-label="${%Snooze.duration.label}"
31+
title="${%Snooze.duration.title}"/>
1832
<f:submit value="${%Snooze}"/>
1933
</form>
2034
</div>

0 commit comments

Comments
 (0)