[FIXED JENKINS-32359] - Properly assign persisted list owners when lo…

…ading folders from disk (#32) * [JENKINS-32359] - Direct Unit test for the issue * [FIXED JENKINS-32359] - Assign persisted list owners when reloading the folder with properties * [JENKINS-32487] - Expose AbstractPropertyFolder's owner via API * [JENKINS-32487] - Test suite refactoring * [JENKINS-32359] - Address comments from @jglick * [JENKINS-32359] - Add test dependency on matrix-auth plugin * [JENKINS-32359] - Restrict AbstractFolderProperty#getOwner()
jenkinsci · Jun 13, 2016 · bd37926 · bd37926
1 parent 4324680
commit bd37926
Show file tree

Hide file tree

Showing 4 changed files with 93 additions and 1 deletion.
diff --git a/pom.xml b/pom.xml
@@ -56,6 +56,13 @@
       <version>2.0.4</version>
       <optional>true</optional>
     </dependency>
+    <dependency>
+      <groupId>org.jenkins-ci.plugins</groupId>
+      <artifactId>matrix-auth</artifactId>
+      <version>1.3</version>
+      <scope>test</scope>
+      <type>jar</type>
+    </dependency>
   </dependencies>
 
   <build>

diff --git a/src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolder.java b/src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolder.java
@@ -191,6 +191,8 @@ protected AbstractFolder(ItemGroup parent, String name) {
     protected void init() {
         if (properties == null) {
             properties = new DescribableList<AbstractFolderProperty<?>,AbstractFolderPropertyDescriptor>(this);
+        } else {
+            properties.setOwner(this);
         }
         for (AbstractFolderProperty p : properties) {
             p.setOwner(this);

diff --git a/src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolderProperty.java b/src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolderProperty.java
@@ -35,6 +35,8 @@
 import java.util.Collections;
 import java.util.List;
 import net.sf.json.JSONObject;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.StaplerOverridable;
 import org.kohsuke.stapler.StaplerRequest;
 
@@ -54,10 +56,21 @@ public abstract class AbstractFolderProperty<C extends AbstractFolder<?>> extend
     /**
      * Hook for performing post-initialization action.
      */
-    protected void setOwner(C owner) {
+    protected void setOwner(@NonNull C owner) {
         this.owner = owner;
     }
 
+    /**
+     * Gets an owner of the property.
+     * @return Owner of the property.
+     *         It should be always non-null if the property initialized correctly.
+     */
+    @NonNull
+    @Restricted(NoExternalUse.class)
+    public C getOwner() {
+        return owner;
+    }
+
     @Override
     public AbstractFolderPropertyDescriptor getDescriptor() {
         return (AbstractFolderPropertyDescriptor) super.getDescriptor();

diff --git a/src/test/java/com/cloudbees/hudson/plugins/folder/FolderTest.java b/src/test/java/com/cloudbees/hudson/plugins/folder/FolderTest.java
@@ -24,6 +24,8 @@
 
 package com.cloudbees.hudson.plugins.folder;
 
+import com.cloudbees.hudson.plugins.folder.properties.FolderCredentialsProvider;
+import com.cloudbees.plugins.credentials.domains.DomainCredentials;
 import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import com.gargoylesoftware.htmlunit.html.HtmlInput;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
@@ -39,13 +41,16 @@
 import hudson.search.SearchItem;
 import hudson.security.ACL;
 import hudson.security.WhoAmI;
+import hudson.security.Permission;
+import hudson.security.ProjectMatrixAuthorizationStrategy;
 import hudson.tasks.BuildTrigger;
 import hudson.views.BuildButtonColumn;
 import hudson.views.JobColumn;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -56,6 +61,7 @@
 import jenkins.model.Jenkins;
 import jenkins.util.Timer;
 import org.acegisecurity.AccessDeniedException;
+import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.*;
 import org.junit.Rule;
 import org.junit.Test;
@@ -325,7 +331,71 @@ private void copyFromGUI(Folder f, JenkinsRule.WebClient wc, String fromName, St
         f.addAction(a);
         assertNotNull(f.getAction(WhoAmI.class));
     }
+
+    @Issue("JENKINS-32487")
+    @Test public void shouldAssignPropertyOwnerOnCreationAndReload() throws Exception {
+        Folder folder = r.jenkins.createProject(Folder.class, "myFolder");
+        r.jenkins.setAuthorizationStrategy(new ProjectMatrixAuthorizationStrategy());
+
+        // We add a stub property to generate the persisted list
+        // Then we ensure owner is being assigned properly.
+        folder.addProperty(new FolderCredentialsProvider.FolderCredentialsProperty(new DomainCredentials[0]));
+        assertPropertyOwner("After property add", folder, FolderCredentialsProvider.FolderCredentialsProperty.class);
+
+        // Reload and ensure that the property owner is set
+        r.jenkins.reload();
+        Folder reloadedFolder = r.jenkins.getItemByFullName("myFolder", Folder.class);
+        assertPropertyOwner("After reload", reloadedFolder, FolderCredentialsProvider.FolderCredentialsProperty.class);
+    }
+
+    @Issue("JENKINS-32359")
+    @Test public void shouldProperlyPersistFolderPropertiesOnMultipleReloads() throws Exception {
+        Folder folder = r.jenkins.createProject(Folder.class, "myFolder");
+
+        // We add a stub property to generate the persisted list
+        // After that we save and reload the config in order to drop PersistedListOwner according to the JENKINS-32359 scenario
+        folder.addProperty(new FolderCredentialsProvider.FolderCredentialsProperty(new DomainCredentials[0]));
+        r.jenkins.reload();
+
+        // Add another property
+        Map<Permission,Set<String>> grantedPermissions = new HashMap<Permission, Set<String>>();
+        Set<String> sids = new HashSet<String>();
+        sids.add("admin");
+        grantedPermissions.put(Jenkins.ADMINISTER, sids);
+        folder = r.jenkins.getItemByFullName("myFolder", Folder.class); 
+        r.jenkins.setAuthorizationStrategy(new ProjectMatrixAuthorizationStrategy());
+        folder.addProperty(new com.cloudbees.hudson.plugins.folder.properties.AuthorizationMatrixProperty(grantedPermissions));
+
+        // Reload folder from disk and check the state
+        r.jenkins.reload();
+        Folder reloadedFolder = r.jenkins.getItemByFullName("myFolder", Folder.class);
+        assertThat("Folder has not been found after the reloading", reloadedFolder, notNullValue());
+        assertThat("Property has not been reloaded, hence it has not been saved properly",
+            reloadedFolder.getProperties().get(com.cloudbees.hudson.plugins.folder.properties.AuthorizationMatrixProperty.class),
+            notNullValue());
+
+        // Also ensure that both property owners are configured correctly
+        assertPropertyOwner("After reload", reloadedFolder, FolderCredentialsProvider.FolderCredentialsProperty.class);
+        assertPropertyOwner("After reload", reloadedFolder, com.cloudbees.hudson.plugins.folder.properties.AuthorizationMatrixProperty.class);
+    }
 
+    /**
+     * Ensures that the specified property points to the folder.
+     * @param <T> Property type
+     * @param folder Folder
+     * @param propertyClass Property class
+     * @param step Failure message prefix
+     */
+    private <T extends AbstractFolderProperty<AbstractFolder<?>>> void assertPropertyOwner
+            (String step, Folder folder, Class<T> propertyClass) {
+        AbstractFolder<?> propertyOwner = folder.getProperties().get(propertyClass).getOwner();
+        assertThat(step + ": The property owner should be instance of Folder", 
+                propertyOwner, instanceOf(Folder.class));
+        assertThat(step + ": The owner field of the " + propertyClass + 
+                " property should point to the owner folder " + folder, 
+                (Folder)propertyOwner, equalTo(folder));
+    }
+
     private Folder createFolder() throws IOException {
         return r.jenkins.createProject(Folder.class, "folder" + r.jenkins.getItems().size());
     }