Skip to content

Commit 3cd946c

Browse files
jglickjglick
committed
Merge pull request #80 from jenkinsci-cert/SECURITY-362
[SECURITY-362] Do not persist User in OfflineCause.UserCause
1 parent 15128c6 commit 3cd946c

File tree

5 files changed

+196
-9
lines changed

5 files changed

+196
-9
lines changed

‎core/src/main/java/hudson/slaves/OfflineCause.java

+39-9
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424

2525
package hudson.slaves;
2626

27-
import jenkins.model.Jenkins;
2827
import hudson.Functions;
2928
import hudson.model.Computer;
3029
import hudson.model.User;
@@ -33,7 +32,10 @@
3332
import org.kohsuke.stapler.export.ExportedBean;
3433
import org.kohsuke.stapler.export.Exported;
3534

35+
import javax.annotation.CheckForNull;
3636
import javax.annotation.Nonnull;
37+
import java.io.ObjectStreamException;
38+
import java.util.Collections;
3739
import java.util.Date;
3840

3941
/**
@@ -128,21 +130,49 @@ public String toString() {
128130

129131
/**
130132
* Taken offline by user.
133+
*
131134
* @since 1.551
132135
*/
133136
public static class UserCause extends SimpleOfflineCause {
134-
private final User user;
135-
136-
public UserCause(User user, String message) {
137-
super(hudson.slaves.Messages._SlaveComputer_DisconnectedBy(
138-
user!=null ? user.getId() : Jenkins.ANONYMOUS.getName(),
137+
@Deprecated
138+
private transient User user;
139+
// null when unknown
140+
private /*final*/ @CheckForNull String userId;
141+
142+
public UserCause(@CheckForNull User user, @CheckForNull String message) {
143+
this(
144+
user != null ? user.getId() : null,
139145
message != null ? " : " + message : ""
140-
));
141-
this.user = user;
146+
);
147+
}
148+
149+
private UserCause(String userId, String message) {
150+
super(hudson.slaves.Messages._SlaveComputer_DisconnectedBy(userId, message));
151+
this.userId = userId;
142152
}
143153

144154
public User getUser() {
145-
return user;
155+
return userId == null
156+
? User.getUnknown()
157+
: User.getById(userId, true)
158+
;
159+
}
160+
161+
// Storing the User in a filed was a mistake, switch to userId
162+
@SuppressWarnings("deprecation")
163+
private Object readResolve() throws ObjectStreamException {
164+
if (user != null) {
165+
String id = user.getId();
166+
if (id != null) {
167+
userId = id;
168+
} else {
169+
// The user field is not properly deserialized so id may be missing. Look the user up by fullname
170+
User user = User.get(this.user.getFullName(), true, Collections.emptyMap());
171+
userId = user.getId();
172+
}
173+
this.user = null;
174+
}
175+
return this;
146176
}
147177
}
148178

‎test/src/test/java/hudson/model/ComputerTest.java

+29
Original file line numberDiff line numberDiff line change
@@ -23,16 +23,22 @@
2323
*/
2424
package hudson.model;
2525

26+
import static org.hamcrest.Matchers.containsString;
27+
import static org.hamcrest.Matchers.not;
2628
import static org.junit.Assert.*;
2729

30+
2831
import java.io.File;
2932

33+
import com.gargoylesoftware.htmlunit.xml.XmlPage;
34+
import hudson.slaves.OfflineCause;
3035
import jenkins.model.Jenkins;
3136
import hudson.slaves.DumbSlave;
3237

3338
import org.junit.Rule;
3439
import org.junit.Test;
3540
import org.jvnet.hudson.test.JenkinsRule;
41+
import org.jvnet.hudson.test.recipes.LocalData;
3642

3743
public class ComputerTest {
3844

@@ -52,4 +58,27 @@ public void discardLogsAfterDeletion() throws Exception {
5258

5359
assertTrue("Slave log should be kept", keep.toComputer().getLogFile().exists());
5460
}
61+
62+
@Test
63+
public void doNotShowUserDetailsInOfflineCause() throws Exception {
64+
DumbSlave slave = j.createOnlineSlave();
65+
final Computer computer = slave.toComputer();
66+
computer.setTemporarilyOffline(true, new OfflineCause.UserCause(User.get("username"), "msg"));
67+
verifyOfflineCause(computer);
68+
}
69+
70+
@Test @LocalData
71+
public void removeUserDetailsFromOfflineCause() throws Exception {
72+
Computer computer = j.jenkins.getComputer("deserialized");
73+
verifyOfflineCause(computer);
74+
}
75+
76+
private void verifyOfflineCause(Computer computer) throws Exception {
77+
XmlPage page = j.createWebClient().goToXml("computer/" + computer.getName() + "/config.xml");
78+
String content = page.getWebResponse().getContentAsString("UTF-8");
79+
assertThat(content, containsString("temporaryOfflineCause"));
80+
assertThat(content, containsString("<userId>username</userId>"));
81+
assertThat(content, not(containsString("ApiTokenProperty")));
82+
assertThat(content, not(containsString("apiToken")));
83+
}
5584
}

‎test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/config.xml

+34
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
<?xml version='1.0' encoding='UTF-8'?>
2+
<hudson>
3+
<disabledAdministrativeMonitors/>
4+
<version>1.0</version>
5+
<numExecutors>2</numExecutors>
6+
<mode>NORMAL</mode>
7+
<useSecurity>true</useSecurity>
8+
<authorizationStrategy class="hudson.security.AuthorizationStrategy$Unsecured"/>
9+
<securityRealm class="hudson.security.SecurityRealm$None"/>
10+
<disableRememberMe>false</disableRememberMe>
11+
<projectNamingStrategy class="jenkins.model.ProjectNamingStrategy$DefaultProjectNamingStrategy"/>
12+
<workspaceDir>${JENKINS_HOME}/workspace/${ITEM_FULLNAME}</workspaceDir>
13+
<buildsDir>${ITEM_ROOTDIR}/builds</buildsDir>
14+
<jdks/>
15+
<viewsTabBar class="hudson.views.DefaultViewsTabBar"/>
16+
<myViewsTabBar class="hudson.views.DefaultMyViewsTabBar"/>
17+
<clouds/>
18+
<scmCheckoutRetryCount>0</scmCheckoutRetryCount>
19+
<views>
20+
<hudson.model.AllView>
21+
<owner class="hudson" reference="../../.."/>
22+
<name>All</name>
23+
<filterExecutors>false</filterExecutors>
24+
<filterQueue>false</filterQueue>
25+
<properties class="hudson.model.View$PropertyList"/>
26+
</hudson.model.AllView>
27+
</views>
28+
<primaryView>All</primaryView>
29+
<slaveAgentPort>0</slaveAgentPort>
30+
<label></label>
31+
<nodeProperties/>
32+
<globalNodeProperties/>
33+
<noUsageStatistics>true</noUsageStatistics>
34+
</hudson>

‎test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/nodes/deserialized/config.xml

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
<?xml version='1.0' encoding='UTF-8'?>
2+
<slave>
3+
<temporaryOfflineCause class="hudson.slaves.OfflineCause$UserCause">
4+
<timestamp>1479196265920</timestamp>
5+
<description>
6+
<holder>
7+
<owner>hudson.slaves.Messages</owner>
8+
</holder>
9+
<key>SlaveComputer.DisconnectedBy</key>
10+
<args>
11+
<string>username</string>
12+
<string> : msg</string>
13+
</args>
14+
</description>
15+
<user>
16+
<fullName>username</fullName>
17+
<properties>
18+
<jenkins.security.ApiTokenProperty>
19+
<apiToken>wPfVKd4HGJzRoEpazbTu35nXXfI34cguPjm+5JPO7pZDFLFgpFLviQsS3NdJndax</apiToken>
20+
</jenkins.security.ApiTokenProperty>
21+
<hudson.tasks.Mailer_-UserProperty/>
22+
<hudson.model.MyViewsProperty>
23+
<views>
24+
<hudson.model.AllView>
25+
<owner class="hudson.model.MyViewsProperty" reference="../../.."/>
26+
<name>All</name>
27+
<filterExecutors>false</filterExecutors>
28+
<filterQueue>false</filterQueue>
29+
<properties class="hudson.model.View$PropertyList"/>
30+
</hudson.model.AllView>
31+
</views>
32+
</hudson.model.MyViewsProperty>
33+
<hudson.model.PaneStatusProperties>
34+
<collapsed/>
35+
</hudson.model.PaneStatusProperties>
36+
<hudson.search.UserSearchProperty>
37+
<insensitiveSearch>false</insensitiveSearch>
38+
</hudson.search.UserSearchProperty>
39+
</properties>
40+
</user>
41+
</temporaryOfflineCause>
42+
<name>deserialized</name>
43+
<description>dummy</description>
44+
<remoteFS>...</remoteFS>
45+
<numExecutors>1</numExecutors>
46+
<mode>NORMAL</mode>
47+
<retentionStrategy class="hudson.slaves.RetentionStrategy$2">
48+
<DESCRIPTOR>
49+
<outer-class reference="../.."/>
50+
</DESCRIPTOR>
51+
</retentionStrategy>
52+
<launcher/>
53+
<label></label>
54+
<nodeProperties/>
55+
<userId>SYSTEM</userId>
56+
</slave>

‎test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/users/username/config.xml

+38
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<?xml version='1.0' encoding='UTF-8'?>
2+
<user>
3+
<fullName>username</fullName>
4+
<id>username</id>
5+
<description></description>
6+
<properties>
7+
<jenkins.security.ApiTokenProperty>
8+
<apiToken>qykj8q6EqvMg9LPu+lCqLiXBZvEVdCTWoYJwmicXgH+yh1ZUm85iHe29grd+g3QG</apiToken>
9+
</jenkins.security.ApiTokenProperty>
10+
<com.cloudbees.plugins.credentials.UserCredentialsProvider_-UserCredentialsProperty plugin="credentials@1.18">
11+
<domainCredentialsMap class="hudson.util.CopyOnWriteMap$Hash"/>
12+
</com.cloudbees.plugins.credentials.UserCredentialsProvider_-UserCredentialsProperty>
13+
<hudson.tasks.Mailer_-UserProperty>
14+
<emailAddress></emailAddress>
15+
</hudson.tasks.Mailer_-UserProperty>
16+
<hudson.model.MyViewsProperty>
17+
<primaryViewName></primaryViewName>
18+
<views>
19+
<hudson.model.AllView>
20+
<owner class="hudson.model.MyViewsProperty" reference="../../.."/>
21+
<name>All</name>
22+
<filterExecutors>false</filterExecutors>
23+
<filterQueue>false</filterQueue>
24+
<properties class="hudson.model.View$PropertyList"/>
25+
</hudson.model.AllView>
26+
</views>
27+
</hudson.model.MyViewsProperty>
28+
<hudson.model.PaneStatusProperties>
29+
<collapsed/>
30+
</hudson.model.PaneStatusProperties>
31+
<org.jenkinsci.main.modules.cli.auth.ssh.UserPropertyImpl>
32+
<authorizedKeys></authorizedKeys>
33+
</org.jenkinsci.main.modules.cli.auth.ssh.UserPropertyImpl>
34+
<hudson.search.UserSearchProperty>
35+
<insensitiveSearch>false</insensitiveSearch>
36+
</hudson.search.UserSearchProperty>
37+
</properties>
38+
</user>

0 commit comments

Comments
 (0)