[FIXED JENKINS-35967] - Make User#isIdOrFullnameAllowed() more robust…

… against restricted usernames (#2413) This change hardens username verification in user creation commands. See the issue to get rexamples. https://issues.jenkins-ci.org/browse/JENKINS-35967
jenkinsci · Oct 15, 2016 · 7d886ce · 7d886ce
1 parent cc51ce5
commit 7d886ce
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 1 deletion.
diff --git a/core/src/main/java/hudson/model/User.java b/core/src/main/java/hudson/model/User.java
@@ -71,6 +71,7 @@
 import java.io.IOException;
 import java.io.FileFilter;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
@@ -717,8 +718,9 @@ public static boolean isIdOrFullnameAllowed(@CheckForNull String id) {
         if (id == null || StringUtils.isBlank(id)) {
             return false;
         }
+        final String trimmedId = id.trim();
         for (String invalidId : ILLEGAL_PERSISTED_USERNAMES) {
-            if (id.equalsIgnoreCase(invalidId))
+            if (trimmedId.equalsIgnoreCase(invalidId))
                 return false;
         }
         return true;
@@ -982,6 +984,19 @@ public List<Action> getTransientActions() {
     public ContextMenu doContextMenu(StaplerRequest request, StaplerResponse response) throws Exception {
         return new ContextMenu().from(this,request,response);
     }
+
+    /**
+     * Gets list of Illegal usernames, for which users should not be created.
+     * Always includes users from {@link #ILLEGAL_PERSISTED_USERNAMES}
+     * @return List of usernames
+     */
+    @Restricted(NoExternalUse.class)
+    /*package*/ static Set<String> getIllegalPersistedUsernames() {
+        // TODO: This method is designed for further extensibility via system properties. To be extended in a follow-up issue
+        final Set<String> res = new HashSet<>();
+        res.addAll(Arrays.asList(ILLEGAL_PERSISTED_USERNAMES));
+        return res;
+    }
 
     public static abstract class CanonicalIdResolver extends AbstractDescribableImpl<CanonicalIdResolver> implements ExtensionPoint, Comparable<CanonicalIdResolver> {
 

diff --git a/core/src/test/java/hudson/model/UserTest.java b/core/src/test/java/hudson/model/UserTest.java
@@ -43,4 +43,31 @@ public void blankIdsOrFullNamesShouldNotBeAllowed() {
         assertThat("Blank user IDs should not be allowed", User.isIdOrFullnameAllowed("      "), is(false));
     }
 
+    @Test
+    @Issue("JENKINS-35967")
+    public void shoudNotAllowIllegalRestrictedNamesInWrongCase() {
+        assertIdOrFullNameNotAllowed("system");
+        assertIdOrFullNameNotAllowed("System");
+        assertIdOrFullNameNotAllowed("SYSTEM");
+        assertIdOrFullNameNotAllowed("syStem");
+        assertIdOrFullNameNotAllowed("sYstEm");
+    }
+
+    @Test
+    @Issue("JENKINS-35967")
+    public void shoudNotAllowIllegalRestrictedNamesEvenIfTrimmed() {
+        for (String username : User.getIllegalPersistedUsernames()) {
+            assertIdOrFullNameNotAllowed(username);
+            assertIdOrFullNameNotAllowed(" " + username);
+            assertIdOrFullNameNotAllowed(username + " ");
+            assertIdOrFullNameNotAllowed("      " + username + "    ");
+            assertIdOrFullNameNotAllowed("\t" + username + "\t");  
+        }
+    }
+
+    private void assertIdOrFullNameNotAllowed(String id) {
+        assertThat("User ID or full name '" + id + "' should not be allowed", 
+                User.isIdOrFullnameAllowed(id), is(false));
+    }
+
 }