diff --git a/core/src/main/java/hudson/logging/LogRecorder.java b/core/src/main/java/hudson/logging/LogRecorder.java index ad4c40028547..c11c1bbfa57e 100644 --- a/core/src/main/java/hudson/logging/LogRecorder.java +++ b/core/src/main/java/hudson/logging/LogRecorder.java @@ -556,7 +556,7 @@ public void delete() throws IOException { loggers.forEach(Target::disable); getParent().getRecorders().forEach(logRecorder -> logRecorder.getLoggers().forEach(Target::enable)); - SaveableListener.fireOnChange(this, getConfigFile()); + SaveableListener.fireOnDeleted(this, getConfigFile()); } /** diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java index 8ba7aafe15cd..e1181d45ec4b 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -815,6 +815,7 @@ public void delete() throws IOException, InterruptedException { ItemDeletion.deregister(this); } } + SaveableListener.fireOnDeleted(this, getConfigFile()); getParent().onDeleted(AbstractItem.this); Jenkins.get().rebuildDependencyGraphAsync(); } diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java index 37b09fd47d25..450be07b7f1a 100644 --- a/core/src/main/java/hudson/model/Run.java +++ b/core/src/main/java/hudson/model/Run.java @@ -1570,6 +1570,7 @@ public void delete() throws IOException { )); //Still firing the delete listeners; just no need to clean up rootDir RunListener.fireDeleted(this); + SaveableListener.fireOnDeleted(this, getDataFile()); synchronized (this) { // avoid holding a lock while calling plugin impls of onDeleted removeRunFromParent(); } @@ -1578,6 +1579,7 @@ public void delete() throws IOException { //The root dir exists and is a directory that needs to be purged RunListener.fireDeleted(this); + SaveableListener.fireOnDeleted(this, getDataFile()); if (artifactManager != null) { deleteArtifacts(); diff --git a/core/src/main/java/hudson/model/listeners/SaveableListener.java b/core/src/main/java/hudson/model/listeners/SaveableListener.java index 46bbc6ab60be..02747877e76f 100644 --- a/core/src/main/java/hudson/model/listeners/SaveableListener.java +++ b/core/src/main/java/hudson/model/listeners/SaveableListener.java @@ -30,8 +30,7 @@ import hudson.ExtensionPoint; import hudson.XmlFile; import hudson.model.Saveable; -import java.util.logging.Level; -import java.util.logging.Logger; +import jenkins.util.Listeners; /** * Receives notifications about save actions on {@link Saveable} objects in Hudson. @@ -54,6 +53,17 @@ public abstract class SaveableListener implements ExtensionPoint { */ public void onChange(Saveable o, XmlFile file) {} + /** + * Called when a {@link Saveable} object gets deleted. + * + * @param o + * The saveable object. + * @param file + * The {@link XmlFile} for this saveable object. + * @since TODO + */ + public void onDeleted(Saveable o, XmlFile file) {} + /** * Registers this object as an active listener so that it can start getting * callbacks invoked. @@ -77,13 +87,15 @@ public void unregister() { * Fires the {@link #onChange} event. */ public static void fireOnChange(Saveable o, XmlFile file) { - for (SaveableListener l : all()) { - try { - l.onChange(o, file); - } catch (Throwable t) { - Logger.getLogger(SaveableListener.class.getName()).log(Level.WARNING, null, t); - } - } + Listeners.notify(SaveableListener.class, false, l -> l.onChange(o, file)); + } + + /** + * Fires the {@link #onDeleted} event. + * @since TODO + */ + public static void fireOnDeleted(Saveable o, XmlFile file) { + Listeners.notify(SaveableListener.class, false, l -> l.onDeleted(o, file)); } /** diff --git a/core/src/main/java/jenkins/model/Nodes.java b/core/src/main/java/jenkins/model/Nodes.java index 8f4c0e5eafc6..e15d391c4975 100644 --- a/core/src/main/java/jenkins/model/Nodes.java +++ b/core/src/main/java/jenkins/model/Nodes.java @@ -295,6 +295,7 @@ public void run() { jenkins.trimLabels(node); } NodeListener.fireOnDeleted(node); + SaveableListener.fireOnDeleted(node, getConfigFile(node)); } } diff --git a/test/src/test/java/hudson/logging/LogRecorderManagerTest.java b/test/src/test/java/hudson/logging/LogRecorderManagerTest.java index 608e7eb0b464..101729038a33 100644 --- a/test/src/test/java/hudson/logging/LogRecorderManagerTest.java +++ b/test/src/test/java/hudson/logging/LogRecorderManagerTest.java @@ -229,7 +229,7 @@ public static class DeletingLogRecorderListener extends SaveableListener { private static boolean recordDeletion; @Override - public void onChange(Saveable o, XmlFile file) { + public void onDeleted(Saveable o, XmlFile file) { if (o instanceof LogRecorder && "dummy".equals(((LogRecorder) o).getName())) { if (!file.exists()) { recordDeletion = true; diff --git a/test/src/test/java/hudson/model/AbstractItemTest.java b/test/src/test/java/hudson/model/AbstractItemTest.java index 165c39a96e6e..8e2a5036b14a 100644 --- a/test/src/test/java/hudson/model/AbstractItemTest.java +++ b/test/src/test/java/hudson/model/AbstractItemTest.java @@ -6,6 +6,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import hudson.ExtensionList; import hudson.XmlFile; @@ -33,6 +34,7 @@ import org.jvnet.hudson.test.MockAuthorizationStrategy; import org.jvnet.hudson.test.SleepBuilder; import org.jvnet.hudson.test.TestExtension; +import org.springframework.security.core.Authentication; public class AbstractItemTest { @@ -45,12 +47,21 @@ public class AbstractItemTest { @Test public void reload() throws Exception { Jenkins jenkins = j.jenkins; - FreeStyleProject p = jenkins.createProject(FreeStyleProject.class, "foo"); - p.setDescription("Hello World"); + jenkins.setSecurityRealm(j.createDummySecurityRealm()); + MockAuthorizationStrategy mas = new MockAuthorizationStrategy(); + mas.grant(Item.CONFIGURE).everywhere().to("alice", "bob"); + mas.grant(Item.READ).everywhere().to("alice"); - FreeStyleBuild b = j.buildAndAssertSuccess(p); - b.setDescription("This is my build"); + FreeStyleProject p; + FreeStyleBuild b; + var alice = User.getById("alice", true); + try (ACLContext ignored = ACL.as(alice)) { + p = jenkins.createProject(FreeStyleProject.class, "foo"); + p.setDescription("Hello World"); + b = j.buildAndAssertSuccess(p); + b.setDescription("This is my build"); + } // update on disk representation Path path = p.getConfigFile().getFile().toPath(); Files.writeString(path, Files.readString(path, StandardCharsets.UTF_8).replaceAll("Hello World", "Good Evening"), StandardCharsets.UTF_8); @@ -61,15 +72,25 @@ public void reload() throws Exception { // reload away p.doReload(); - assertFalse(SaveableListener.class.getSimpleName() + " should not have been called", testSaveableListener.wasCalled()); - - + assertFalse(SaveableListener.class.getSimpleName() + " should not have been called", testSaveableListener.isChangeCalled()); assertEquals("Good Evening", p.getDescription()); FreeStyleBuild b2 = p.getBuildByNumber(1); assertNotEquals(b, b2); // should be different object assertEquals(b.getDescription(), b2.getDescription()); // but should have the same properties + + try (var ignored = ACL.as(alice)) { + p.setDescription("This is Alice's project"); + } + assertTrue(SaveableListener.class.getSimpleName() + " should have been called", testSaveableListener.isChangeCalled()); + assertThat(testSaveableListener.getChangeUser(), equalTo(alice.impersonate2())); + + try (var ignored = ACL.as(alice)) { + p.delete(); + } + assertTrue(SaveableListener.class.getSimpleName() + " should have been called", testSaveableListener.isDeleteCalled()); + assertThat(testSaveableListener.getDeleteUser(), equalTo(alice.impersonate2())); } @Test @@ -158,20 +179,45 @@ private String getPath(URL u) { public static class TestSaveableListener extends SaveableListener { private Saveable saveable; - private boolean called; + private boolean changeCalled; + private Authentication changeUser; + + private boolean deleteCalled; + private Authentication deleteUser; private void setSaveable(Saveable saveable) { this.saveable = saveable; } - public boolean wasCalled() { - return called; + public boolean isChangeCalled() { + return changeCalled; + } + + public Authentication getChangeUser() { + return changeUser; + } + + public boolean isDeleteCalled() { + return deleteCalled; + } + + public Authentication getDeleteUser() { + return deleteUser; } @Override public void onChange(Saveable o, XmlFile file) { if (o == saveable) { - this.called = true; + changeCalled = true; + changeUser = Jenkins.getAuthentication2(); + } + } + + @Override + public void onDeleted(Saveable o, XmlFile file) { + if (o == saveable) { + deleteCalled = true; + deleteUser = Jenkins.getAuthentication2(); } } } diff --git a/test/src/test/java/hudson/model/RunTest.java b/test/src/test/java/hudson/model/RunTest.java index b6a145857020..34be1f14c2ef 100644 --- a/test/src/test/java/hudson/model/RunTest.java +++ b/test/src/test/java/hudson/model/RunTest.java @@ -32,8 +32,11 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import hudson.ExtensionList; import hudson.FilePath; import hudson.Launcher; +import hudson.XmlFile; +import hudson.model.listeners.SaveableListener; import hudson.tasks.ArtifactArchiver; import hudson.tasks.BuildTrigger; import hudson.tasks.Builder; @@ -112,6 +115,17 @@ public class RunTest { FreeStyleBuild b = j.buildAndAssertSuccess(p); b.delete(); assertTrue(Mgr.deleted.get()); + assertTrue(ExtensionList.lookupSingleton(SaveableListenerImpl.class).deleted); + } + + @TestExtension("deleteArtifactsCustom") + public static class SaveableListenerImpl extends SaveableListener { + boolean deleted; + + @Override + public void onDeleted(Saveable o, XmlFile file) { + deleted = true; + } } @Issue("SECURITY-1902") diff --git a/test/src/test/java/jenkins/model/NodesTest.java b/test/src/test/java/jenkins/model/NodesTest.java index 275dd9a3e5fb..c7207b9977c5 100644 --- a/test/src/test/java/jenkins/model/NodesTest.java +++ b/test/src/test/java/jenkins/model/NodesTest.java @@ -37,10 +37,13 @@ import edu.umd.cs.findbugs.annotations.NonNull; import hudson.ExtensionList; +import hudson.XmlFile; import hudson.model.Descriptor; import hudson.model.Failure; import hudson.model.Node; +import hudson.model.Saveable; import hudson.model.Slave; +import hudson.model.listeners.SaveableListener; import hudson.slaves.ComputerLauncher; import hudson.slaves.DumbSlave; import java.io.IOException; @@ -99,6 +102,10 @@ public void addNodeShouldReplaceExistingNode() throws Exception { assertEquals(0, l.deleted); assertEquals(1, l.updated); assertEquals(1, l.created); + var saveableListener = ExtensionList.lookupSingleton(SaveableListenerImpl.class); + assertEquals(0, saveableListener.deleted); + r.jenkins.removeNode(newNode); + assertEquals(1, saveableListener.deleted); } @TestExtension("addNodeShouldReplaceExistingNode") @@ -122,6 +129,16 @@ protected void onCreated(Node node) { } } + @TestExtension("addNodeShouldReplaceExistingNode") + public static final class SaveableListenerImpl extends SaveableListener { + int deleted; + + @Override + public void onDeleted(Saveable o, XmlFile file) { + deleted++; + } + } + @Test @Issue("JENKINS-56403") public void replaceNodeShouldRemoveOldNode() throws Exception {