Skip to content
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.

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

JENKINS-61206 - Support System Read / Extended read permissions for agent configurations #4531

Merged
merged 27 commits into from
May 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
cf86609
JENKINS-61206 System read / Extended read for agents
timja Feb 29, 2020
35dba6f
Update core/src/main/java/hudson/Functions.java
timja Mar 1, 2020
e00fa4f
Set permission to false explicitly
timja Mar 6, 2020
b0a574d
Merge branch 'master' into systemRead-agents
timja Mar 6, 2020
0ffa5fc
Add message when no configured clouds
timja Mar 6, 2020
7e8bb9b
Merge branch 'master' into systemRead-agents
timja Apr 25, 2020
b7dd142
Hide password for Computer.EXTENDED_READ
timja Apr 25, 2020
6e1eeaf
Merge branch 'master' into systemRead-agents
timja Apr 28, 2020
393ad7f
Add test
timja Apr 28, 2020
bb85534
Access controlled simplification
timja Apr 29, 2020
c074b10
Fix typoes / simplify code
timja Apr 29, 2020
e22c9ce
Simplify task.jelly
timja May 3, 2020
f1962ae
Allow filtering system info extensions
timja May 3, 2020
6a17aec
Show link that should be present
timja May 3, 2020
244e467
Adjust MasterComputer access
timja May 3, 2020
df660ff
Adjust javadoc
timja May 3, 2020
37fc83b
Adjust MasterComputer#configure
timja May 3, 2020
221ed0f
Merge branch 'master' into systemRead-agents
timja May 8, 2020
7146b7a
Change links when read only access
timja May 8, 2020
86de15b
Use it instead of app
timja May 8, 2020
06d8455
Update core/src/main/resources/hudson/model/Computer/configure.jelly
May 8, 2020
c0783af
Merge branch 'master' into systemRead-agents
timja May 11, 2020
47e31f5
Merge branch 'systemRead-agents' of github.com:timja/jenkins into sys…
timja May 11, 2020
31c35d6
Code simplification
timja May 11, 2020
bbe611c
Inline jelly text customisation
timja May 19, 2020
37c712b
Allow system read to see cloud move blurb
timja May 19, 2020
1beda15
Tooltip based on permission
timja May 19, 2020
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
42 changes: 42 additions & 0 deletions core/src/main/java/hudson/Functions.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
*/
package hudson;

import hudson.model.Computer;
import hudson.model.Slave;
import hudson.security.*;

Expand Down Expand Up @@ -1157,6 +1158,43 @@ public static Collection<Descriptor> getSortedDescriptorsForGlobalConfigUnclassi
Jenkins.get().hasPermission(d.getRequiredGlobalConfigPagePermission()) || Jenkins.get().hasPermission(Jenkins.SYSTEM_READ)));
}

/**
* Checks if the current security principal has one of the supplied permissions.
*
* @since TODO
*/
public static boolean hasAnyPermission(AccessControlled ac, Permission[] permissions) {
if (permissions == null || permissions.length == 0) {
return true;
timja marked this conversation as resolved.
Show resolved Hide resolved
}

return ac.hasAnyPermission(permissions);
}

/**
* This version is so that the 'hasAnyPermission'
* degrades gracefully if "it" is not an {@link AccessControlled} object.
* Otherwise it will perform no check and that problem is hard to notice.
Copy link
Member

Choose a reason for hiding this comment

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

Just throw in that case? The assumption that object is an ancestor isn't necessarily true. For example, in upstream cause related UI, you would show information about another job / build.

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s consistent with hasPermission and wasn’t working without this for the l:task it was just silently doing nothing

timja marked this conversation as resolved.
Show resolved Hide resolved
*
* @since TODO
*/
public static boolean hasAnyPermission(Object object, Permission[] permissions) throws IOException, ServletException {
if (permissions == null || permissions.length == 0) {
return true;
}

if (object instanceof AccessControlled)
return hasAnyPermission((AccessControlled) object, permissions);
else {
AccessControlled ac = Stapler.getCurrentRequest().findAncestorObject(AccessControlled.class);
if (ac != null) {
return hasAnyPermission(ac, permissions);
}

return hasAnyPermission(Jenkins.get(), permissions);
}
}

/**
* Checks if the current security principal has one of the supplied permissions.
*
Expand Down Expand Up @@ -2034,6 +2072,10 @@ public String getPasswordValue(Object o) {
if (item != null && !item.hasPermission(Item.CONFIGURE)) {
return "********";
}
Computer computer = req.findAncestorObject(Computer.class);
if (computer != null && !computer.hasPermission(Computer.CONFIGURE)) {
return "********";
timja marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

Expand Down
6 changes: 5 additions & 1 deletion core/src/main/java/hudson/model/Computer.java
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ public String getLog() throws IOException {
* Used to URL-bind {@link AnnotatedLargeText}.
*/
public AnnotatedLargeText<Computer> getLogText() {
checkPermission(CONNECT);
checkAnyPermission(CONNECT, EXTENDED_READ);
return new AnnotatedLargeText<>(getLogFile(), Charset.defaultCharset(), false, this);
}

Expand Down Expand Up @@ -1804,6 +1804,10 @@ public long getWhen() {
public static final Permission CONNECT = new Permission(PERMISSIONS,"Connect", Messages._Computer_ConnectPermission_Description(), DISCONNECT, PermissionScope.COMPUTER);
public static final Permission BUILD = new Permission(PERMISSIONS, "Build", Messages._Computer_BuildPermission_Description(), Permission.WRITE, PermissionScope.COMPUTER);

@Restricted(NoExternalUse.class) // called by jelly
public static final Permission[] EXTENDED_READ_AND_CONNECT =
new Permission[] { EXTENDED_READ, CONNECT };

// This permission was historically scoped to this class albeit declared in Cloud. While deserializing, Jenkins loads
// the scope class to make sure the permission is initialized and registered. since Cloud class is used rather seldom,
// it might appear the permission does not exist. Referencing the permission from here to make sure it gets loaded.
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/jenkins/management/NodesLink.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public String getDescription() {
@NonNull
@Override
public Permission getRequiredPermission() {
return Jenkins.MANAGE;
return Jenkins.READ;
timja marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package jenkins.slaves.systemInfo;

import hudson.Extension;
import hudson.model.Computer;
import hudson.security.Permission;
import org.jenkinsci.Symbol;

/**
Expand All @@ -12,4 +14,9 @@ public class ClassLoaderStatisticsSlaveInfo extends SlaveSystemInfo {
public String getDisplayName() {
return Messages.ClassLoaderStatisticsSlaveInfo_DisplayName();
}

@Override
public Permission getRequiredPermission() {
return Computer.EXTENDED_READ;
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package jenkins.slaves.systemInfo;

import hudson.Extension;
import hudson.model.Computer;
import hudson.security.Permission;
import org.jenkinsci.Symbol;

/**
Expand All @@ -12,4 +14,9 @@ public class EnvVarsSlaveInfo extends SlaveSystemInfo {
public String getDisplayName() {
return Messages.EnvVarsSlaveInfo_DisplayName();
}

@Override
public Permission getRequiredPermission() {
return Computer.EXTENDED_READ;
}
}
14 changes: 14 additions & 0 deletions core/src/main/java/jenkins/slaves/systemInfo/SlaveSystemInfo.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
import hudson.ExtensionList;
import hudson.ExtensionPoint;
import hudson.model.Computer;
import hudson.model.ManageJenkinsAction;
import hudson.security.Permission;
import jenkins.model.Jenkins;

/**
* Extension point that contributes to the system information page of {@link Computer}.
Expand All @@ -24,4 +27,15 @@ public abstract class SlaveSystemInfo implements ExtensionPoint {
public static ExtensionList<SlaveSystemInfo> all() {
return ExtensionList.lookup(SlaveSystemInfo.class);
}

/**
* Returns the permission required for user to see this system info extension on the "System Information" page for the Agent
timja marked this conversation as resolved.
Show resolved Hide resolved
*
* By default {@link Computer#CONNECT}, but {@link Computer#EXTENDED_READ} is also supported.
*
* @return the permission required for the extension to be shown on "System Information".
*/
public Permission getRequiredPermission() {
return Computer.CONNECT;
timja marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package jenkins.slaves.systemInfo;

import hudson.Extension;
import hudson.model.Computer;
import hudson.security.Permission;
import org.jenkinsci.Symbol;

/**
Expand All @@ -12,4 +14,9 @@ public class SystemPropertySlaveInfo extends SlaveSystemInfo {
public String getDisplayName() {
return Messages.SystemPropertySlaveInfo_DisplayName();
}

@Override
public Permission getRequiredPermission() {
return Computer.EXTENDED_READ;
}
}
15 changes: 10 additions & 5 deletions core/src/main/resources/hudson/model/Computer/configure.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ THE SOFTWARE.
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout"
xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<l:layout permission="${it.CONFIGURE}" title="${%title(it.displayName)}">
<l:layout permission="${it.EXTENDED_READ}" title="${%title(it.displayName)}">
<j:set var="readOnlyMode" value="${!it.hasPermission(it.CONFIGURE)}" />
<st:include page="sidepanel.jelly"/>
<l:main-panel>
<f:form method="post" action="configSubmit" name="config">
Expand All @@ -42,11 +43,15 @@ THE SOFTWARE.
<!-- main body of the configuration -->
<st:include it="${instance}" page="configure-entries.jelly" />

<f:bottomButtonBar>
<f:submit value="${%Save}"/>
</f:bottomButtonBar>
<j:if test="${h.hasPermission(it, it.CONFIGURE)}">
<f:bottomButtonBar>
<f:submit value="${%Save}"/>
</f:bottomButtonBar>
</j:if>
</f:form>
<st:adjunct includes="lib.form.confirm" />
<j:if test="${h.hasPermission(it, it.CONFIGURE)}">
<st:adjunct includes="lib.form.confirm" />
</j:if>
</l:main-panel>
</l:layout>
</j:jelly>
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ THE SOFTWARE.
<l:task contextMenu="false" href="${rootURL}/computer" icon="icon-up icon-md" title="${%Back to List}"/>
<l:task contextMenu="false" href="${rootURL}/${it.url}" icon="icon-search icon-md" title="${%Status}"/>
<l:task href="${rootURL}/${it.url}delete" icon="icon-edit-delete icon-md" permission="${it.DELETE}" title="${%Delete Agent}"/>
<l:task href="${rootURL}/${it.url}configure" icon="icon-setting icon-md" permission="${it.CONFIGURE}" title="${%Configure}"/>
<l:task href="${rootURL}/${it.url}configure" icon="icon-setting icon-md" permission="${it.EXTENDED_READ}"
title="${it.hasPermission(it.CONFIGURE) ? '%Configure' : '%View Configuration'}"/>
<l:task href="${rootURL}/${it.url}builds" icon="icon-notepad icon-md" title="${%Build History}"/>
<l:task href="${rootURL}/${it.url}load-statistics" icon="icon-monitor icon-md" title="${%Load Statistics}"/>
<j:if test="${it.channel!=null}">
Expand Down
13 changes: 8 additions & 5 deletions core/src/main/resources/hudson/model/ComputerSet/configure.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ THE SOFTWARE.
-->
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:i="jelly:fmt">
<l:layout permission="${app.ADMINISTER}" title="${%Node Monitoring Configuration}">
<l:layout permission="${app.SYSTEM_READ}" title="${%Node Monitoring Configuration}">
timja marked this conversation as resolved.
Show resolved Hide resolved
<j:set var="readOnlyMode" value="${!app.hasPermission(app.ADMINISTER)}" />
<st:include page="sidepanel.jelly" />
<l:main-panel>
<!-- to make the form field binding work -->
Expand All @@ -39,10 +40,12 @@ THE SOFTWARE.
descriptors="${it.nodeMonitorDescriptors}"
instances="${it.nonIgnoredMonitors}" />

<f:bottomButtonBar>
<f:submit value="${%OK}" />
<f:apply />
</f:bottomButtonBar>
<l:isAdmin>
<f:bottomButtonBar>
<f:submit value="${%OK}" />
<f:apply />
</f:bottomButtonBar>
</l:isAdmin>
</f:form>
<st:adjunct includes="lib.form.confirm" />
</l:main-panel>
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/resources/hudson/model/ComputerSet/index.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ THE SOFTWARE.
</j:forEach>

<td><!-- config link -->
<j:if test="${c.hasPermission(c.CONFIGURE)}">
<j:if test="${c.hasPermission(c.EXTENDED_READ)}">
<a href="${rootURL}/${c.url}configure">
<l:icon class="icon-gear2 icon-lg" tooltip="${%Configure}"/>
<l:icon class="icon-gear2 icon-lg" tooltip="${c.hasPermission(c.CONFIGURE) ? '%Configure' : '%View Configuration'}"/>
</a>
</j:if>
</td>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,13 @@ THE SOFTWARE.
<j:getStatic var="createPermission" className="hudson.model.Computer" field="CREATE"/>
<l:tasks>
<l:task href="${rootURL}/" icon="icon-up icon-md" title="${%Back to Dashboard}"/>
timja marked this conversation as resolved.
Show resolved Hide resolved
<l:task href="${rootURL}/manage" icon="icon-gear2 icon-md" permission="${app.MANAGE}" title="${%Manage Jenkins}"/>
<l:task href="${rootURL}/manage" icon="icon-gear2 icon-md" permissions="${app.MANAGE_AND_SYSTEM_READ}" title="${%Manage Jenkins}"/>
<l:task href="new" icon="icon-new-computer icon-md" permission="${createPermission}" title="${%New Node}"/>
<l:task href="${rootURL}/configureClouds" icon="icon-health-40to59 icon-md" permission="${app.ADMINISTER}" title="${%Configure Clouds}"/>
<l:task href="configure" icon="icon-gear2 icon-md" permission="${app.ADMINISTER}" title="${%Node Monitoring}"/>
<l:task href="${rootURL}/configureClouds" icon="icon-health-40to59 icon-md" permission="${app.SYSTEM_READ}"
title="${app.hasPermission(app.ADMINISTER) ? '%Configure Clouds' : '%View Clouds'}"/>
<l:task href="configure" icon="icon-gear2 icon-md" permission="${app.SYSTEM_READ}" title="${%Node Monitoring}"/>
</l:tasks>
<t:queue items="${app.queue.items}" />
<t:executors />
</l:side-panel>
</j:jelly>
</j:jelly>
28 changes: 13 additions & 15 deletions core/src/main/resources/hudson/slaves/SlaveComputer/log.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,21 @@ THE SOFTWARE.

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:i="jelly:fmt">
<l:layout title="${it.displayName} log" secured="true">
timja marked this conversation as resolved.
Show resolved Hide resolved
<l:layout title="${it.displayName} log" permissions="${it.EXTENDED_READ_AND_CONNECT}">
<st:include page="sidepanel.jelly" />
<l:main-panel>
<l:hasPermission permission="${it.CONNECT}">
<pre id="out" />
<div id="spinner">
<img src="${imagesURL}/spinner.gif" alt=""/>
</div>
<t:progressiveText href="logText/progressiveHtml" idref="out" spinner="spinner" />
<!-- TODO dubious value: INFO+ shown in logText anyway; FINE- configured in /log/*/ maybe better viewed there
<j:set var="logRecords" value="${it.logRecords}"/>
<j:if test="${!logRecords.isEmpty()}">
<h1>${%Log Records}</h1>
<t:logRecords logRecords="${logRecords}"/>
</j:if>
-->
</l:hasPermission>
<pre id="out" />
<div id="spinner">
<img src="${imagesURL}/spinner.gif" alt=""/>
</div>
<t:progressiveText href="logText/progressiveHtml" idref="out" spinner="spinner" />
<!-- TODO dubious value: INFO+ shown in logText anyway; FINE- configured in /log/*/ maybe better viewed there
<j:set var="logRecords" value="${it.logRecords}"/>
<j:if test="${!logRecords.isEmpty()}">
<h1>${%Log Records}</h1>
<t:logRecords logRecords="${logRecords}"/>
</j:if>
-->
</l:main-panel>
</l:layout>
</j:jelly>
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ THE SOFTWARE.

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:i="jelly:fmt">
<l:task icon="icon-clipboard icon-md" href="${rootURL}/${it.url}log" title="${%Log}" permission="${it.CONNECT}" />
<l:task icon="icon-clipboard icon-md" href="${rootURL}/${it.url}log" title="${%Log}" permissions="${it.EXTENDED_READ_AND_CONNECT}" />
<j:if test="${it.channel!=null}">
<l:task icon="icon-computer icon-md" href="${rootURL}/${it.url}systemInfo" title="${%System Information}" permission="${it.CONNECT}"/>
<l:task icon="icon-computer icon-md" href="${rootURL}/${it.url}systemInfo" title="${%System Information}" permissions="${it.EXTENDED_READ_AND_CONNECT}"/>
<l:task icon="icon-edit-delete icon-md" href="${rootURL}/${it.url}disconnect" title="${%Disconnect}" permission="${it.DISCONNECT}"/>
</j:if>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ THE SOFTWARE.
-->
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<l:layout title="${it.displayName} ${%System Information}">
<l:layout title="${it.displayName} ${%System Information}" permissions="${it.EXTENDED_READ_AND_CONNECT}">
<st:include page="sidepanel.jelly" />

<l:main-panel>
Expand All @@ -42,21 +42,21 @@ THE SOFTWARE.
</j:if>
</h1>

<l:hasPermission permission="${it.CONNECT}">
<j:choose>
<j:when test="${it.channel != null}">
<h2>${it.oSDescription} agent, version ${it.slaveVersion}</h2>
<j:choose>
<j:when test="${it.channel != null}">
<h2>${it.oSDescription} agent, version ${it.slaveVersion}</h2>

<j:forEach var="instance" items="${it.systemInfoExtensions}">
<j:forEach var="instance" items="${it.systemInfoExtensions}">
<l:hasPermission permission="${instance.requiredPermission}">
<h1>${instance.displayName}</h1>
<st:include page="systemInfo" from="${instance}"/>
</j:forEach>
</j:when>
<j:otherwise>
${%System Information is unavailable when agent is offline.}
</j:otherwise>
</j:choose>
</l:hasPermission>
</l:hasPermission>
</j:forEach>
</j:when>
<j:otherwise>
${%System Information is unavailable when agent is offline.}
</j:otherwise>
</j:choose>
</l:main-panel>
</l:layout>
</j:jelly>
Loading