[SECURITY-243] introduce a new API to get a user by their id.

Introduce a new API User.getById that will only ever get a user by their
ID as suggested by @jglick (adapted from the original suggestion).

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

@@ -344,7 +344,7 @@ public synchronized void doSubmitDescription( StaplerRequest req, StaplerRespons
      * This is used to avoid null {@link User} instance.
      */
     public static @Nonnull User getUnknown() {
-        return get(UKNOWN_USERNAME);
+        return getById(UKNOWN_USERNAME, true);
     }
 
     /**
@@ -477,6 +477,7 @@ public synchronized void doSubmitDescription( StaplerRequest req, StaplerRespons
 
     /**
      * 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);
@@ -504,7 +505,23 @@ public synchronized void doSubmitDescription( StaplerRequest req, StaplerRespons
 
         // 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;

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

@@ -135,16 +135,18 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
         }
 
         {// attempt to authenticate as API token
-            User u = User.get(username);
-            ApiTokenProperty t = u.getProperty(ApiTokenProperty.class);
-            if (t!=null && t.matchesPassword(password)) {
-                SecurityContextHolder.getContext().setAuthentication(u.impersonate());
-                try {
-                    chain.doFilter(request,response);
-                } finally {
-                    SecurityContextHolder.clearContext();
+            User u = User.getById(username, false);
+            if (u != null) {
+                ApiTokenProperty t = u.getProperty(ApiTokenProperty.class);
+                if (t!=null && t.matchesPassword(password)) {
+                    SecurityContextHolder.getContext().setAuthentication(u.impersonate());
+                    try {
+                        chain.doFilter(request,response);
+                    } finally {
+                        SecurityContextHolder.clearContext();
+                    }
+                    return;
                 }
-                return;
             }
         }
 

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

@@ -169,7 +169,7 @@ public GroupDetails loadGroupByGroupname(String groupname) throws UsernameNotFou
 
     @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,7 @@ private User createAccount(StaplerRequest req, StaplerResponse rsp, boolean self
         if(si.username==null || si.username.length()==0)
             si.errorMessage = Messages.HudsonPrivateSecurityRealm_CreateAccount_UserNameRequired();
         else {
-            User user = User.get(si.username, false);
+            User user = User.get(si.username, false); // TODO why false? use getById?
             if (null != user)
                 // Allow sign up. SCM people has no such property.
                 if (user.getProperty(Details.class) != null)
@@ -373,7 +373,7 @@ private User createAccount(StaplerRequest req, StaplerResponse rsp, boolean self
      * 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 +416,7 @@ public List<User> getAllUsers() {
      * 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/security/BasicHeaderApiTokenAuthenticator.java

@@ -24,21 +24,22 @@ 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);
-        ApiTokenProperty t = u.getProperty(ApiTokenProperty.class);
-        if (t!=null && t.matchesPassword(password)) {
-            try {
-                return u.impersonate();
-            } catch (UsernameNotFoundException x) {
-                // The token was valid, but the impersonation failed. This token is clearly not his real password,
-                // so there's no point in continuing the request processing. Report this error and abort.
-                LOGGER.log(WARNING, "API token matched for user "+username+" but the impersonation failed",x);
-                throw new ServletException(x);
-            } catch (DataAccessException x) {
-                throw new ServletException(x);
+        User u = User.getById(username, false);
+        if (u != null) {
+            ApiTokenProperty t = u.getProperty(ApiTokenProperty.class);
+            if (t!=null && t.matchesPassword(password)) {
+                try {
+                    return u.impersonate();
+                } catch (UsernameNotFoundException x) {
+                    // The token was valid, but the impersonation failed. This token is clearly not his real password,
+                    // so there's no point in continuing the request processing. Report this error and abort.
+                    LOGGER.log(WARNING, "API token matched for user "+username+" but the impersonation failed",x);
+                    throw new ServletException(x);
+                } catch (DataAccessException x) {
+                    throw new ServletException(x);
+                }
             }
         }
-
         return null;
     }
 

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

@@ -39,7 +39,7 @@ public UserDetails loadUserByUsername(String username) throws UsernameNotFoundEx
 
     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.get(username, false, emptyMap()); // TODO why false? use getById?
         if (u!=null) {
             LastGrantedAuthoritiesProperty p = u.getProperty(LastGrantedAuthoritiesProperty.class);
             if (p!=null)

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

@@ -100,7 +100,9 @@ protected void failedToAuthenticate(@Nonnull String username) {
         @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());

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

@@ -584,6 +584,52 @@ public void resolveByIdThenName() throws Exception{
         assertEquals("'user2' should resolve to u2", u2.getId(), u.getId());
     }
 
+    @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

@@ -6,23 +6,10 @@
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.assertThat;
-import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.not;
-import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.xml.HasXPath.hasXPath;
 
 import java.io.UnsupportedEncodingException;
-import java.util.regex.Pattern;
-
-import org.apache.commons.httpclient.Credentials;
-import org.apache.commons.httpclient.auth.AuthScheme;
-import org.apache.commons.httpclient.auth.CredentialsNotAvailableException;
-import org.apache.commons.httpclient.auth.CredentialsProvider;
-import org.hamcrest.Description;
-import org.hamcrest.TypeSafeDiagnosingMatcher;
-import org.apache.commons.httpclient.UsernamePasswordCredentials;
-
 
 import org.junit.Rule;
 import org.junit.Test;