[FIXED JENKINS-24317] Corrected user dir migration code from JENKINS-…

…23872. Adapted from #1375 by @daniel-beck: omitting change to User construction, and adding test.
jenkinsci · Aug 21, 2014 · 088edab · 088edab
1 parent 151e155
commit 088edab
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 9 deletions.
diff --git a/changelog.html b/changelog.html
@@ -61,6 +61,9 @@
   <li class=bug>
     Deadlock in <code>OldDataMonitor</code>.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-24358">issue 24358</a>)
+  <li class='major bug'>
+    Failure to migrate legacy user records properly broke Jenkins.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-24317">issue 24317</a>)
 </ul>
 </div><!--=TRUNK-END=-->
 

diff --git a/core/src/main/java/hudson/model/User.java b/core/src/main/java/hudson/model/User.java
@@ -384,23 +384,26 @@ private static User getOrCreate(String id, String fullName, boolean create) {
             // check for legacy users and migrate if safe to do so.
             File[] legacy = getLegacyConfigFilesFor(id);
             if (legacy != null && legacy.length > 0) {
-                for (File p : legacy) {
-                    final XmlFile legacyXml = new XmlFile(XSTREAM, new File(p, "config.xml"));
+                for (File legacyUserDir : legacy) {
+                    final XmlFile legacyXml = new XmlFile(XSTREAM, new File(legacyUserDir, "config.xml"));
                     try {
                         Object o = legacyXml.read();
                         if (o instanceof User) {
-                            User tmp = (User) o;
-                            if (idStrategy().equals(id, tmp.getId()) && !idStrategy().filenameOf(tmp.getId())
-                                    .equals(p.getParentFile().getName())) {
-                                if (!p.getParentFile().renameTo(configFile.getParentFile())) {
-                                    LOGGER.log(Level.FINE, "Could not migrate user record from {0} to {1}",
-                                            new Object[]{p.getParentFile(), configFile.getParentFile()});
+                            if (idStrategy().equals(id, legacyUserDir.getName()) && !idStrategy().filenameOf(legacyUserDir.getName())
+                                    .equals(legacyUserDir.getName())) {
+                                if (!legacyUserDir.renameTo(configFile.getParentFile())) {
+                                    LOGGER.log(Level.WARNING, "Failed to migrate user record from {0} to {1}",
+                                            new Object[]{legacyUserDir, configFile.getParentFile()});
                                 }
                                 break;
                             }
+                        } else {
+                            LOGGER.log(Level.FINE, "Unexpected object loaded from {0}: {1}",
+                                    new Object[]{ legacyUserDir, o });
                         }
                     } catch (IOException e) {
-                        // ignore
+                        LOGGER.log(Level.FINE, String.format("Exception trying to load user from {0}: {1}",
+                                new Object[]{ legacyUserDir, e.getMessage() }), e);
                     }
                 }
             }

diff --git a/test/src/test/java/hudson/model/UserTest.java b/test/src/test/java/hudson/model/UserTest.java
@@ -33,8 +33,10 @@
 import hudson.security.HudsonPrivateSecurityRealm;
 import hudson.security.Permission;
 import hudson.tasks.MailAddressResolver;
+import java.io.File;
 import java.io.IOException;
 import java.io.PrintStream;
+import java.util.Arrays;
 import java.util.Collections;
 
 import jenkins.model.IdStrategy;
@@ -47,8 +49,10 @@
 import org.junit.Test;
 import org.jvnet.hudson.test.Bug;
 import org.jvnet.hudson.test.FakeChangeLogSCM;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.TestExtension;
+import org.jvnet.hudson.test.recipes.LocalData;
 
 public class UserTest {
 
@@ -220,6 +224,16 @@ public IdStrategy getUserIdStrategy() {
         assertEquals(user2.getId(), User.idStrategy().idFromFilename(User.idStrategy().filenameOf(user2.getId())));
     }
 
+    @Issue("JENKINS-24317")
+    @LocalData
+    @Test public void migration() throws Exception {
+        User bob = User.get("bob");
+        assertEquals("Bob Smith", bob.getFullName());
+        assertEquals("Bob Smith", User.get("Bob").getFullName());
+        assertEquals("nonexistent", User.get("nonexistent").getFullName());
+        assertEquals("[bob]", Arrays.toString(new File(j.jenkins.getRootDir(), "users").list()));
+    }
+
     @Test
     public void testAddAndGetProperty() throws IOException {
         User user = User.get("John Smith");  

diff --git a/test/src/test/resources/hudson/model/UserTest/migration.zip b/test/src/test/resources/hudson/model/UserTest/migration.zip