Merge pull request #69 from jenkinsci-cert/SECURITY-243-amended

[SECURITY-243] Prefer id to fullName

core/src/main/java/hudson/model/User.java

@@ -52,6 +52,8 @@ import org.acegisecurity.providers.anonymous.AnonymousAuthenticationToken;
 import org.acegisecurity.userdetails.UserDetails;
 import org.acegisecurity.userdetails.UsernameNotFoundException;
 import org.springframework.dao.DataAccessException;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.StaplerResponse;
 import org.kohsuke.stapler.export.Exported;
@@ -342,7 +344,7 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
      * This is used to avoid null {@link User} instance.
      */
     public static @Nonnull User getUnknown() {
-        return get(UKNOWN_USERNAME);
+        return getById(UKNOWN_USERNAME, true);
     }
 
     /**
@@ -475,6 +477,7 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
 
     /**
      * Gets the {@link User} object by its id or full name.
+     * Use {@link #getById} when you know you have an ID.
      */
     public static @Nonnull User get(String idOrFullName) {
         return get(idOrFullName,true);
@@ -502,7 +505,23 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
 
         // Since we already know this is a name, we can just call getOrCreate with the name directly.
         String id = a.getName();
-        return getOrCreate(id, id, true);
+        return getById(id, true);
+    }
+
+    /**
+     * Gets the {@link User} object by its <code>id</code>
+     *
+     * @param id
+     *            the id of the user to retrieve and optionally create if it does not exist.
+     * @param create
+     *            If <code>true</code>, this method will never return <code>null</code> for valid input (by creating a
+     *            new {@link User} object if none exists.) If <code>false</code>, this method will return
+     *            <code>null</code> if {@link User} object with the given id doesn't exist.
+     * @return the a User whose id is <code>id</code>, or <code>null</code> if <code>create</code> is <code>false</code>
+     *         and the user does not exist.
+     */
+    public static @Nullable User getById(String id, boolean create) {
+        return getOrCreate(id, id, create);
     }
 
     private static volatile long lastScanned;
@@ -996,6 +1015,58 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
         }
     }
 
+
+    /**
+     * Tries to verify if an ID is valid.
+     * If so, we do not want to even consider users who might have the same full name.
+     */
+    @Extension
+    @Restricted(NoExternalUse.class)
+    public static class UserIDCanonicalIdResolver extends User.CanonicalIdResolver {
+
+        private static /* not final */ boolean SECURITY_243_FULL_DEFENSE = Boolean.parseBoolean(System.getProperty(User.class.getName() + ".SECURITY_243_FULL_DEFENSE", "true"));
+
+        private static final ThreadLocal<Boolean> resolving = new ThreadLocal<Boolean>() {
+            @Override
+            protected Boolean initialValue() {
+                return false;
+            }
+        };
+
+        @Override
+        public String resolveCanonicalId(String idOrFullName, Map<String, ?> context) {
+            User existing = getById(idOrFullName, false);
+            if (existing != null) {
+                return existing.getId();
+            }
+            if (SECURITY_243_FULL_DEFENSE) {
+                Jenkins j = Jenkins.getInstance();
+                if (j != null) {
+                    if (!resolving.get()) {
+                        resolving.set(true);
+                        try {
+                            return j.getSecurityRealm().loadUserByUsername(idOrFullName).getUsername();
+                        } catch (UsernameNotFoundException x) {
+                            LOGGER.log(Level.FINE, "not sure whether " + idOrFullName + " is a valid username or not", x);
+                        } catch (DataAccessException x) {
+                            LOGGER.log(Level.FINE, "could not look up " + idOrFullName, x);
+                        } finally {
+                            resolving.set(false);
+                        }
+                    }
+                }
+            }
+            return null;
+        }
+
+        @Override
+        public int getPriority() {
+            // should always come first so that ID that are ids get mapped correctly
+            return Integer.MAX_VALUE;
+        }
+
+    }
+
     /**
      * Jenkins now refuses to let the user login if he/she doesn't exist in {@link SecurityRealm},
      * which was necessary to make sure users removed from the backend will get removed from the frontend.

core/src/main/java/hudson/security/BasicAuthenticationFilter.java

@@ -135,7 +135,9 @@ public class BasicAuthenticationFilter implements Filter {
         }
 
         {// attempt to authenticate as API token
-            User u = User.get(username);
+            // create is true as the user may not have been saved and the default api token may be in use.
+            // validation of the user will be performed against the underlying realm in impersonate.
+            User u = User.getById(username, true);
             ApiTokenProperty t = u.getProperty(ApiTokenProperty.class);
             if (t!=null && t.matchesPassword(password)) {
                 SecurityContextHolder.getContext().setAuthentication(u.impersonate());

core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java

@@ -169,7 +169,7 @@ public class HudsonPrivateSecurityRealm extends AbstractPasswordBasedSecurityRea
 
     @Override
     public Details loadUserByUsername(String username) throws UsernameNotFoundException, DataAccessException {
-        User u = User.get(username,false);
+        User u = User.getById(username, false);
         Details p = u!=null ? u.getProperty(Details.class) : null;
         if(p==null)
             throw new UsernameNotFoundException("Password is not set: "+username);
@@ -324,7 +324,8 @@ public class HudsonPrivateSecurityRealm extends AbstractPasswordBasedSecurityRea
         if(si.username==null || si.username.length()==0)
             si.errorMessage = Messages.HudsonPrivateSecurityRealm_CreateAccount_UserNameRequired();
         else {
-            User user = User.get(si.username, false);
+            // do not create the user - we just want to check if the user already exists but is not a "login" user.
+            User user = User.getById(si.username, false); 
             if (null != user)
                 // Allow sign up. SCM people has no such property.
                 if (user.getProperty(Details.class) != null)
@@ -373,7 +374,7 @@ public class HudsonPrivateSecurityRealm extends AbstractPasswordBasedSecurityRea
      * Creates a new user account by registering a password to the user.
      */
     public User createAccount(String userName, String password) throws IOException {
-        User user = User.get(userName);
+        User user = User.getById(userName, true);
         user.addProperty(Details.fromPlainPassword(password));
         return user;
     }
@@ -416,7 +417,7 @@ public class HudsonPrivateSecurityRealm extends AbstractPasswordBasedSecurityRea
      * This in turn helps us set up the right navigation breadcrumb.
      */
     public User getUser(String id) {
-        return User.get(id);
+        return User.getById(id, true);
     }
 
     // TODO

core/src/main/java/jenkins/model/Jenkins.java

@@ -2460,8 +2460,8 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve
     /**
      * Gets the user of the given name.
      *
-     * @return the user of the given name, if that person exists or the invoker {@link #hasPermission} on {@link #ADMINISTER}; else null
-     * @see User#get(String,boolean)
+     * @return the user of the given name (which may or may not be an id), if that person exists or the invoker {@link #hasPermission} on {@link #ADMINISTER}; else null
+     * @see User#get(String,boolean), {@link User#getById(String, boolean)}
      */
     public @CheckForNull User getUser(String name) {
         return User.get(name,hasPermission(ADMINISTER));

core/src/main/java/jenkins/security/BasicHeaderApiTokenAuthenticator.java

@@ -24,7 +24,7 @@ public class BasicHeaderApiTokenAuthenticator extends BasicHeaderAuthenticator {
     @Override
     public Authentication authenticate(HttpServletRequest req, HttpServletResponse rsp, String username, String password) throws ServletException {
         // attempt to authenticate as API token
-        User u = User.get(username);
+        User u = User.getById(username, true);
         ApiTokenProperty t = u.getProperty(ApiTokenProperty.class);
         if (t!=null && t.matchesPassword(password)) {
             try {
@@ -38,7 +38,6 @@ public class BasicHeaderApiTokenAuthenticator extends BasicHeaderAuthenticator {
                 throw new ServletException(x);
             }
         }
-
         return null;
     }
 

core/src/main/java/jenkins/security/ImpersonatingUserDetailsService.java

@@ -39,7 +39,7 @@ public class ImpersonatingUserDetailsService implements UserDetailsService {
 
     protected UserDetails attemptToImpersonate(String username, RuntimeException e) {
         // this backend cannot tell if the user name exists or not. so substitute by what we know
-        User u = User.get(username, false, emptyMap());
+        User u = User.getById(username, false);
         if (u!=null) {
             LastGrantedAuthoritiesProperty p = u.getProperty(LastGrantedAuthoritiesProperty.class);
             if (p!=null)

core/src/main/java/jenkins/security/LastGrantedAuthoritiesProperty.java

@@ -100,7 +100,9 @@ public class LastGrantedAuthoritiesProperty extends UserProperty {
         @Override
         protected void loggedIn(@Nonnull String username) {
             try {
-                User u = User.get(username);
+                // user should have been created but may not have been saved for some realms
+                // but as this is a callback of a successful login we can safely create the user.
+                User u = User.getById(username, true);
                 LastGrantedAuthoritiesProperty o = u.getProperty(LastGrantedAuthoritiesProperty.class);
                 if (o==null)
                     u.addProperty(o=new LastGrantedAuthoritiesProperty());
@@ -132,7 +134,7 @@ public class LastGrantedAuthoritiesProperty extends UserProperty {
              */
 
 //            try {
-//                User u = User.get(username,false,Collections.emptyMap());
+//                User u = User.getById(username,false);
 //                LastGrantedAuthoritiesProperty o = u.getProperty(LastGrantedAuthoritiesProperty.class);
 //                if (o!=null)
 //                    o.invalidate();

test/src/test/java/hudson/model/UserTest.java

@@ -29,16 +29,20 @@ import com.gargoylesoftware.htmlunit.WebAssert;
 import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 
+import hudson.security.AbstractPasswordBasedSecurityRealm;
 import hudson.security.AccessDeniedException2;
 import hudson.security.GlobalMatrixAuthorizationStrategy;
+import hudson.security.GroupDetails;
 import hudson.security.HudsonPrivateSecurityRealm;
 import hudson.security.Permission;
+import hudson.security.UserMayOrMayNotExistException;
 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 java.util.Locale;
 
 import jenkins.model.IdStrategy;
 import jenkins.model.Jenkins;
@@ -46,14 +50,17 @@ import jenkins.security.ApiTokenProperty;
 
 import org.acegisecurity.AccessDeniedException;
 import org.acegisecurity.Authentication;
+import org.acegisecurity.AuthenticationException;
+import org.acegisecurity.GrantedAuthority;
 import org.acegisecurity.context.SecurityContext;
 import org.acegisecurity.context.SecurityContextHolder;
+import org.acegisecurity.userdetails.UserDetails;
+import org.acegisecurity.userdetails.UsernameNotFoundException;
 
 import static org.junit.Assert.*;
 import static org.junit.Assume.*;
 import org.junit.Rule;
 import org.junit.Test;
-import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.FakeChangeLogSCM;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
@@ -539,6 +546,143 @@ public class UserTest {
         } catch (AccessDeniedException expected) { }
     }
 
+    @Issue("SECURITY-243")
+    @Test
+    public void resolveByIdThenName() throws Exception{
+        j.jenkins.setSecurityRealm(new HudsonPrivateSecurityRealm(true, false, null));
+
+        User u1 = User.get("user1");
+        u1.setFullName("User One");
+        u1.save();
+
+        User u2 = User.get("user2");
+        u2.setFullName("User Two");
+        u2.save();
+
+        assertNotSame("Users should not have the same id.", u1.getId(), u2.getId());
+
+        User u = User.get("User One");
+        assertEquals("'User One' should resolve to u1", u1.getId(), u.getId());
+
+        u = User.get("User Two");
+        assertEquals("'User Two' should resolve to u2", u2.getId(), u.getId());
+
+        u = User.get("user1");
+        assertEquals("'user1' should resolve to u1", u1.getId(), u.getId());
+
+        u = User.get("user2");
+        assertEquals("'user2' should resolve to u2", u2.getId(), u.getId());
+
+        u1.setFullName("user2");
+        u1.save();
+        u = User.get("user2");
+        assertEquals("'user2' should resolve to u2", u2.getId(), u.getId());
+        u = User.get("user1");
+        assertEquals("'user1' should resolve to u1", u1.getId(), u.getId());
+
+
+        u1.setFullName("user1");
+        u1.save();
+        u2.setFullName("user1");
+        u2.save();
+        u = User.get("user1");
+        assertEquals("'user1' should resolve to u1", u1.getId(), u.getId());
+        u = User.get("user2");
+        assertEquals("'user2' should resolve to u2", u2.getId(), u.getId());
+    }
+
+    @Issue("SECURITY-243")
+    @Test
+    public void resolveByUnloadedIdThenName() throws Exception {
+        j.jenkins.setSecurityRealm(new ExternalSecurityRealm());
+        // do *not* call this here: User.get("victim");
+        User attacker1 = User.get("attacker1");
+        attacker1.setFullName("victim1");
+        User victim1 = User.get("victim1");
+        assertEquals("victim1 is a real user ID, we must ignore the attacker1’s fullName", "victim1", victim1.getId());
+        assertEquals("a recursive call to User.get was OK", null, victim1.getProperty(MyViewsProperty.class).getPrimaryViewName());
+        assertEquals("(though the realm mistakenly added metadata to the attacker)", "victim1", attacker1.getProperty(MyViewsProperty.class).getPrimaryViewName());
+        User.get("attacker2").setFullName("nonexistent");
+        assertEquals("but if we cannot find such a user ID, allow the fullName", "attacker2", User.get("nonexistent").getId());
+        User.get("attacker3").setFullName("unknown");
+        assertEquals("or if we are not sure, allow the fullName", "attacker3", User.get("unknown").getId());
+        User.get("attacker4").setFullName("Victim2");
+        assertEquals("victim2 is a real (canonical) user ID", "victim2", User.get("Victim2").getId());
+
+    }
+    private static class ExternalSecurityRealm extends AbstractPasswordBasedSecurityRealm {
+        @Override
+        public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException {
+            if (username.equals("nonexistent")) {
+                throw new UsernameNotFoundException(username);
+            } else if (username.equals("unknown")) {
+                throw new UserMayOrMayNotExistException(username);
+            } else {
+                String canonicalName = username.toLowerCase(Locale.ENGLISH);
+                try {
+                    User.get(canonicalName).addProperty(new MyViewsProperty(canonicalName));
+                } catch (IOException x) {
+                    throw new RuntimeException(x);
+                }
+                return new org.acegisecurity.userdetails.User(canonicalName, "", true, true, true, true, new GrantedAuthority[] {AUTHENTICATED_AUTHORITY});
+            }
+        }
+        @Override
+        protected UserDetails authenticate(String username, String password) throws AuthenticationException {
+            return loadUserByUsername(username); // irrelevant
+        }
+        @Override
+        public GroupDetails loadGroupByGroupname(String groupname) throws UsernameNotFoundException {
+            throw new UsernameNotFoundException(groupname); // irrelevant
+        }
+    }
+
+    @Test
+    public void resolveById() throws Exception {
+        User u1 = User.get("user1");
+        u1.setFullName("User One");
+        u1.save();
+
+        User u2 = User.get("user2");
+        u2.setFullName("User Two");
+        u2.save();
+
+        assertNotSame("Users should not have the same id.", u1.getId(), u2.getId());
+
+        // We can get the same user back.
+        User u = User.getById("user1", false);
+        assertSame("'user1' should return u1", u1, u);
+
+        // passing true should not create a new user if it does not exist.
+        u = User.getById("user1", true);
+        assertSame("'user1' should return u1", u1, u);
+
+        // should not lookup by name.
+        u = User.getById("User One", false);
+        assertNull("'User One' should not resolve to any user", u);
+
+        // We can get the same user back.
+        u = User.getById("user2", false);
+        assertSame("'user2' should return u2", u2, u);
+
+        // passing true should not create a new user if it does not exist.
+        u = User.getById("user2", true);
+        assertSame("'user2' should return u1", u2, u);
+
+        // should not lookup by name.
+        u = User.getById("User Two", false);
+        assertNull("'User Two' should not resolve to any user", u);
+
+        u1.setFullName("user1");
+        u1.save();
+        u2.setFullName("user1");
+        u2.save();
+        u = User.getById("user1", false);
+        assertSame("'user1' should resolve to u1", u1, u);
+        u = User.getById("user2", false);
+        assertSame("'user2' should resolve to u2", u2, u);
+    }
+
      public static class SomeUserProperty extends UserProperty {
          
         @TestExtension

test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java

@@ -5,6 +5,11 @@ import static hudson.security.HudsonPrivateSecurityRealm.PASSWORD_ENCODER;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertThat;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.xml.HasXPath.hasXPath;
+
+import java.io.UnsupportedEncodingException;
 
 import org.junit.Rule;
 import org.junit.Test;
@@ -15,6 +20,11 @@ import org.jvnet.hudson.test.WithoutJenkins;
 import org.jvnet.hudson.test.recipes.LocalData;
 import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
+import com.gargoylesoftware.htmlunit.xml.XmlPage;
+
+import hudson.model.User;
+import hudson.remoting.Base64;
+import jenkins.security.ApiTokenProperty;
 
 /**
  * @author Kohsuke Kawaguchi
@@ -62,4 +72,103 @@ public class HudsonPrivateSecurityRealmTest {
 
         assertFalse(secure.equals(old));
     }
+
+
+    @Issue("SECURITY-243")
+    @Test
+    public void fullNameCollisionPassword() throws Exception {
+        HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null);
+        j.jenkins.setSecurityRealm(securityRealm);
+        
+        User u1 = securityRealm.createAccount("user1", "password1");
+        u1.setFullName("User One");
+        u1.save();
+
+        User u2 = securityRealm.createAccount("user2", "password2");
+        u2.setFullName("User Two");
+        u2.save();
+
+        WebClient wc1 = j.createWebClient();
+        wc1.login("user1", "password1");
+
+        WebClient wc2 = j.createWebClient();
+        wc2.login("user2", "password2");
+
+        
+        // Check both users can use their token
+        XmlPage w1 = (XmlPage) wc1.goTo("whoAmI/api/xml", "application/xml");
+        assertThat(w1, hasXPath("//name", is("user1")));
+        
+        XmlPage w2 = (XmlPage) wc2.goTo("whoAmI/api/xml", "application/xml");
+        assertThat(w2, hasXPath("//name", is("user2")));
+
+        u1.setFullName("user2");
+        u1.save();
+        
+        // check the tokens still work
+        wc1 = j.createWebClient();
+        wc1.login("user1", "password1");
+
+        wc2 = j.createWebClient();
+        // throws FailingHttpStatusCodeException on login failure
+        wc2.login("user2", "password2");
+
+        // belt and braces incase the failed login no longer throws exceptions.
+        w1 = (XmlPage) wc1.goTo("whoAmI/api/xml", "application/xml");
+        assertThat(w1, hasXPath("//name", is("user1")));
+        
+        w2 = (XmlPage) wc2.goTo("whoAmI/api/xml", "application/xml");
+        assertThat(w2, hasXPath("//name", is("user2")));
+    }
+
+    @Issue("SECURITY-243")
+    @Test
+    public void fullNameCollisionToken() throws Exception {
+        HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null);
+        j.jenkins.setSecurityRealm(securityRealm);
+        
+        User u1 = securityRealm.createAccount("user1", "password1");
+        u1.setFullName("User One");
+        u1.save();
+        String u1Token = u1.getProperty(ApiTokenProperty.class).getApiToken();
+
+        User u2 = securityRealm.createAccount("user2", "password2");
+        u2.setFullName("User Two");
+        u2.save();
+        String u2Token = u2.getProperty(ApiTokenProperty.class).getApiToken();
+
+        WebClient wc1 = j.createWebClient();
+        wc1.addRequestHeader("Authorization", basicHeader("user1", u1Token));
+        //wc1.setCredentialsProvider(new FixedCredentialsProvider("user1", u1Token));
+
+        WebClient wc2 = j.createWebClient();
+        wc2.addRequestHeader("Authorization", basicHeader("user2", u2Token));
+        //wc2.setCredentialsProvider(new FixedCredentialsProvider("user2", u1Token));
+        
+        // Check both users can use their token
+        XmlPage w1 = (XmlPage) wc1.goTo("whoAmI/api/xml", "application/xml");
+        assertThat(w1, hasXPath("//name", is("user1")));
+        
+        XmlPage w2 = (XmlPage) wc2.goTo("whoAmI/api/xml", "application/xml");
+        assertThat(w2, hasXPath("//name", is("user2")));
+
+
+        u1.setFullName("user2");
+        u1.save();
+        // check the tokens still work
+        w1 = (XmlPage) wc1.goTo("whoAmI/api/xml", "application/xml");
+        assertThat(w1, hasXPath("//name", is("user1")));
+        
+        w2 = (XmlPage) wc2.goTo("whoAmI/api/xml", "application/xml");
+        assertThat(w2, hasXPath("//name", is("user2")));
+    }
+
+
+    private static final String basicHeader(String user, String pass) throws UnsupportedEncodingException {
+        String str = user +':' + pass;
+        String auth = Base64.encode(str.getBytes("US-ASCII"));
+        String authHeader = "Basic " + auth;
+        return authHeader;
+    }
+
 }