Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[FIXED JENKINS-22769] ItemListener callbacks should run as SYSTEM sin…
…ce they sometimes do ACL-checked calls.
  • Loading branch information
jglick committed Sep 24, 2014
1 parent 253943e commit c04cdcd
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 15 deletions.
3 changes: 3 additions & 0 deletions changelog.html
Expand Up @@ -58,6 +58,9 @@
<li class=rfe>
When killing Windows processes, check its critical flag to avoid BSoD
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-24453">issue 24453</a>)
<li class=bug>
When a user could not see a view, but could delete/move/rename jobs contained in it, the view was not properly updated.
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-22769">issue 22769</a>)
</ul>
</div><!--=TRUNK-END=-->

Expand Down
56 changes: 41 additions & 15 deletions core/src/main/java/hudson/model/listeners/ItemListener.java
Expand Up @@ -26,10 +26,10 @@
import hudson.ExtensionPoint;
import hudson.ExtensionList;
import hudson.Extension;
import jenkins.model.Jenkins;
import hudson.model.Item;
import hudson.model.ItemGroup;
import hudson.model.Items;
import hudson.security.ACL;

/**
* Receives notifications about CRUD operations of {@link Item}.
Expand Down Expand Up @@ -151,26 +151,45 @@ public static ExtensionList<ItemListener> all() {
return ExtensionList.lookup(ItemListener.class);
}

public static void fireOnCopied(Item src, Item result) {
for (ItemListener l : all())
l.onCopied(src,result);
public static void fireOnCopied(final Item src, final Item result) {
ACL.impersonate(ACL.SYSTEM, new Runnable() {
@Override public void run() {
for (ItemListener l : all()) {
l.onCopied(src, result);
}
}
});
}

public static void fireOnCreated(Item item) {
for (ItemListener l : all())
l.onCreated(item);
public static void fireOnCreated(final Item item) {
ACL.impersonate(ACL.SYSTEM, new Runnable() {
@Override public void run() {
for (ItemListener l : all()) {
l.onCreated(item);
}
}
});
}

public static void fireOnUpdated(Item item) {
for (ItemListener l : all())
l.onUpdated(item);
public static void fireOnUpdated(final Item item) {
ACL.impersonate(ACL.SYSTEM, new Runnable() {
@Override public void run() {
for (ItemListener l : all()) {
l.onUpdated(item);
}
}
});
}

/** @since 1.548 */
public static void fireOnDeleted(Item item) {
for (ItemListener l : all()) {
l.onDeleted(item);
}
public static void fireOnDeleted(final Item item) {
ACL.impersonate(ACL.SYSTEM, new Runnable() {
@Override public void run() {
for (ItemListener l : all()) {
l.onDeleted(item);
}
}
});
}

/**
Expand All @@ -179,7 +198,14 @@ public static void fireOnDeleted(Item item) {
* @param oldFullName the previous {@link Item#getFullName}
* @since 1.548
*/
public static void fireLocationChange(Item rootItem, String oldFullName) {
public static void fireLocationChange(final Item rootItem, final String oldFullName) {
ACL.impersonate(ACL.SYSTEM, new Runnable() {
@Override public void run() {
doFireLocationChange(rootItem, oldFullName);
}
});
}
private static void doFireLocationChange(Item rootItem, String oldFullName) {
String prefix = rootItem.getParent().getFullName();
if (!prefix.isEmpty()) {
prefix += '/';
Expand Down
42 changes: 42 additions & 0 deletions test/src/test/java/hudson/model/ListViewTest.java
Expand Up @@ -31,17 +31,23 @@
import hudson.matrix.AxisList;
import hudson.matrix.MatrixProject;
import hudson.matrix.TextAxis;
import hudson.security.ACL;
import hudson.security.AuthorizationStrategy;
import hudson.security.Permission;

import java.io.IOException;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import org.acegisecurity.Authentication;

import static org.junit.Assert.*;

import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.Bug;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.JenkinsRule.WebClient;
import org.jvnet.hudson.test.MockFolder;
Expand Down Expand Up @@ -205,4 +211,40 @@ private void checkLinkFromItemExistsAndIsValid(Item item, ItemGroup ig, Item top
assertFalse(view.contains(job));
assertFalse(view.jobNamesContains(job));
}

@Issue("JENKINS-22769")
@Test public void renameJobInViewYouCannotSee() throws Exception {
j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
j.jenkins.setAuthorizationStrategy(new AllButViewsAuthorizationStrategy());
final FreeStyleProject p = j.createFreeStyleProject("p1");
ListView v = new ListView("v", j.jenkins);
v.add(p);
j.jenkins.addView(v);
ACL.impersonate(User.get("alice").impersonate(), new Runnable() {
@Override public void run() {
try {
p.renameTo("p2");
} catch (IOException x) {
throw new RuntimeException(x);
}
}
});
assertEquals(Collections.singletonList(p), v.getItems());
}
private static class AllButViewsAuthorizationStrategy extends AuthorizationStrategy {
@Override public ACL getRootACL() {
return UNSECURED.getRootACL();
}
@Override public Collection<String> getGroups() {
return Collections.emptyList();
}
@Override public ACL getACL(View item) {
return new ACL() {
@Override public boolean hasPermission(Authentication a, Permission permission) {
return a.equals(SYSTEM);
}
};
}
}

}

3 comments on commit c04cdcd

@olivergondza
Copy link
Member

Choose a reason for hiding this comment

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

Caused JENKINS-25400

@daniel-beck
Copy link
Member

Choose a reason for hiding this comment

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

Caused JENKINS-25400. AFAIK there's a similar issue with Create Job Advanced Plugin's "Grant creator full permission."

Wouldn't it be better to have listeners impersonate SYSTEM as needed on their own?

@olivergondza
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, some listeners has been doing this for years (ComputerListener#onOnline) so changing it not to impersonate SYSTEM can be painful. Though, I like @daniel-beck's suggestion.

Please sign in to comment.