Add logic to get display name & email (Simply stored in user model for now).

Add code / test to defend against LDAP injection attacks.

distrib/gitblit.properties

@@ -221,6 +221,28 @@ realm.ldap.groupMemberPattern = (&(objectClass=group)(member=${dn}))
 # SINCE 1.0.0
 realm.ldap.admins= @Git_Admins
 
+# Attribute(s) on the USER record that indicate their display (or full) name. Leave blank
+# for no mapping available in LDAP
+#
+# This may be a single attribute, or a string of multiple attributes.  Examples:
+#  displayName - Uses the attribute 'displayName' on the user record
+#  ${personalTitle}. ${givenName} ${surname} - Will concatenate the 3 
+#       attributes together, with a '.' after personalTitle 
+#
+# SINCE 1.0.0
+realm.ldap.displayName= displayName
+
+# Attribute(s) on the USER record that indicate their email address.  Leave blank
+# for no mapping available in LDAP
+#
+# This may be a single attribute, or a string of multiple attributes.  Examples:
+#  email - Uses the attribute 'email' on the user record
+#  ${givenName}.${surname}@gitblit.com -Will concatenate the 2 attributes
+#       together with a '.' and '@' creating something like first.last@gitblit.com 
+#
+# SINCE 1.0.0
+realm.ldap.email = email
+
 #
 # Gitblit Web Settings
 #

src/com/gitblit/LdapUserService.java

@@ -137,7 +137,7 @@ public class LdapUserService extends GitblitUserService {
 			// Find the logging in user's DN
 			String accountBase = settings.getString(Keys.realm.ldap_accountBase, "");
 			String accountPattern = settings.getString(Keys.realm.ldap_accountPattern, "(&(objectClass=person)(sAMAccountName=${username}))");
-			accountPattern = StringUtils.replace(accountPattern, "${username}", simpleUsername);
+			accountPattern = StringUtils.replace(accountPattern, "${username}", escapeLDAPSearchFilter(simpleUsername));
 
 			SearchResult result = doSearch(ldapConnection, accountBase, accountPattern);
 			if (result != null && result.getEntryCount() == 1) {
@@ -149,15 +149,15 @@ public class LdapUserService extends GitblitUserService {
 					
 					UserModel user = getUserModel(simpleUsername);
 					if (user == null)	// create user object for new authenticated user
-						user = createUserFromLdap(simpleUsername, loggingInUser);
+						user = new UserModel(simpleUsername);
+					
 					
-					user.password = "StoredInLDAP";
 					
 					if (!supportsTeamMembershipChanges())
 						getTeamsFromLdap(ldapConnection, simpleUsername, loggingInUser, user);
 					
-					// Get Admin Attributes
-					setAdminAttribute(user);
+					// Get User Attributes
+					setUserAttributes(user, loggingInUser);
 
 					// Push the ldap looked up values to backing file
 					super.updateUserModel(user);
@@ -186,6 +186,37 @@ public class LdapUserService extends GitblitUserService {
 	                user.canAdmin = true;
 	    }
 	}
+	
+	private void setUserAttributes(UserModel user, SearchResultEntry userEntry) {
+		// Is this user an admin?
+		setAdminAttribute(user);
+		
+		// Don't want visibility into the real password, make up a dummy
+		user.password = "StoredInLDAP";
+		
+		// Get Attributes for full name / email
+		String displayName = settings.getString(Keys.realm.ldap_displayName, "displayName");
+		String email = settings.getString(Keys.realm.ldap_email, "email");
+
+		// Replace embedded ${} with attributes
+		if (displayName.contains("${")) {
+			for (Attribute userAttribute : userEntry.getAttributes())
+				displayName = StringUtils.replace(displayName, "${" + userAttribute.getName() + "}", userAttribute.getValue());
+			
+			user.displayName = displayName;
+		} else {
+			user.displayName = userEntry.getAttribute(displayName).getValue();
+		}
+		
+		if (email.contains("${")) {
+			for (Attribute userAttribute : userEntry.getAttributes())
+				email = StringUtils.replace(email, "${" + userAttribute.getName() + "}", userAttribute.getValue());
+			
+			user.emailAddress = email;
+		} else {
+			user.emailAddress = userEntry.getAttribute(email).getValue();
+		}
+	}
 
 	private void getTeamsFromLdap(LDAPConnection ldapConnection, String simpleUsername, SearchResultEntry loggingInUser, UserModel user) {
 		String loggingInUserDN = loggingInUser.getDN();
@@ -194,12 +225,12 @@ public class LdapUserService extends GitblitUserService {
 		String groupBase = settings.getString(Keys.realm.ldap_groupBase, "");
 		String groupMemberPattern = settings.getString(Keys.realm.ldap_groupMemberPattern, "(&(objectClass=group)(member=${dn}))");
 		
-		groupMemberPattern = StringUtils.replace(groupMemberPattern, "${dn}", loggingInUserDN);
-		groupMemberPattern = StringUtils.replace(groupMemberPattern, "${username}", simpleUsername);
+		groupMemberPattern = StringUtils.replace(groupMemberPattern, "${dn}", escapeLDAPSearchFilter(loggingInUserDN));
+		groupMemberPattern = StringUtils.replace(groupMemberPattern, "${username}", escapeLDAPSearchFilter(simpleUsername));
 		
 		// Fill in attributes into groupMemberPattern
 		for (Attribute userAttribute : loggingInUser.getAttributes())
-			groupMemberPattern = StringUtils.replace(groupMemberPattern, "${" + userAttribute.getName() + "}", userAttribute.getValue());
+			groupMemberPattern = StringUtils.replace(groupMemberPattern, "${" + userAttribute.getName() + "}", escapeLDAPSearchFilter(userAttribute.getValue()));
 		
 		SearchResult teamMembershipResult = doSearch(ldapConnection, groupBase, groupMemberPattern);
 		if (teamMembershipResult != null && teamMembershipResult.getEntryCount() > 0) {
@@ -223,13 +254,6 @@ public class LdapUserService extends GitblitUserService {
 		
 		return answer;		
 	}
-	
-	private UserModel createUserFromLdap(String simpleUserName, SearchResultEntry userEntry) {
-		UserModel answer = new UserModel(simpleUserName);
-		//If attributes other than user name ever from from LDAP, this is where to get them
-		
-		return answer;
-	}
 
 	private SearchResult doSearch(LDAPConnection ldapConnection, String base, String filter) {
 		try {
@@ -243,6 +267,7 @@ public class LdapUserService extends GitblitUserService {
 	
 	private boolean isAuthenticated(LDAPConnection ldapConnection, String userDn, String password) {
 		try {
+			// Binding will stop any LDAP-Injection Attacks since the searched-for user needs to bind to that DN
 			ldapConnection.bind(userDn, password);
 			return true;
 		} catch (LDAPException e) {
@@ -263,6 +288,35 @@ public class LdapUserService extends GitblitUserService {
 		if (lastSlash > -1) {
 			username = username.substring(lastSlash + 1);
 		}
+		
 		return username;
 	}
+	
+	// From: https://www.owasp.org/index.php/Preventing_LDAP_Injection_in_Java
+	public static final String escapeLDAPSearchFilter(String filter) {
+		StringBuilder sb = new StringBuilder();
+		for (int i = 0; i < filter.length(); i++) {
+			char curChar = filter.charAt(i);
+			switch (curChar) {
+			case '\\':
+				sb.append("\\5c");
+				break;
+			case '*':
+				sb.append("\\2a");
+				break;
+			case '(':
+				sb.append("\\28");
+				break;
+			case ')':
+				sb.append("\\29");
+				break;
+			case '\u0000': 
+				sb.append("\\00"); 
+				break;
+			default:
+				sb.append(curChar);
+			}
+		}
+		return sb.toString();
+	}
 }

src/com/gitblit/models/UserModel.java

@@ -37,6 +37,8 @@ public class UserModel implements Principal, Serializable, Comparable<UserModel>
 	// field names are reflectively mapped in EditUser page
 	public String username;
 	public String password;
+	public String displayName;
+	public String emailAddress;
 	public boolean canAdmin;
 	public boolean excludeFromFederation;
 	public final Set<String> repositories = new HashSet<String>();

tests/com/gitblit/tests/LdapUserServiceTest.java

@@ -16,12 +16,16 @@
  */
 package com.gitblit.tests;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
 import java.util.HashMap;
 import java.util.Map;
 
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Test;
 
 import com.gitblit.LdapUserService;
@@ -30,7 +34,6 @@ import com.gitblit.tests.mock.MemorySettings;
 import com.unboundid.ldap.listener.InMemoryDirectoryServer;
 import com.unboundid.ldap.listener.InMemoryDirectoryServerConfig;
 import com.unboundid.ldap.listener.InMemoryListenerConfig;
-import com.unboundid.ldap.sdk.LDAPConnection;
 import com.unboundid.ldif.LDIFReader;
 
 /**
@@ -44,20 +47,25 @@ public class LdapUserServiceTest {
 	
 	private LdapUserService ldapUserService;
 	
-	@Before
-	public void createInMemoryLdapServer() throws Exception {
+	@BeforeClass
+	public static void createInMemoryLdapServer() throws Exception {
 		InMemoryDirectoryServerConfig config = new InMemoryDirectoryServerConfig("dc=MyDomain");
 		config.addAdditionalBindCredentials("cn=Directory Manager", "password");
 		config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", 389));
 		config.setSchema(null);
 		
 		InMemoryDirectoryServer ds = new InMemoryDirectoryServer(config);
-		ds.importFromLDIF(true, new LDIFReader(this.getClass().getResourceAsStream("resources/ldapUserServiceSampleData.ldif")));
+		ds.importFromLDIF(true, new LDIFReader(LdapUserServiceTest.class.getResourceAsStream("resources/ldapUserServiceSampleData.ldif")));
 		ds.startListening();
 	}
 	
 	@Before
 	public void createLdapUserService() {
+		ldapUserService = new LdapUserService();
+		ldapUserService.setup(getSettings());
+	}
+	
+	private MemorySettings getSettings() {
 		Map<Object, Object> backingMap = new HashMap<Object, Object>();
 		backingMap.put("realm.ldap.server", "ldap://localhost:389");
 		backingMap.put("realm.ldap.domain", "");
@@ -70,11 +78,11 @@ public class LdapUserServiceTest {
 		backingMap.put("realm.ldap.groupBase", "OU=Groups,OU=UserControl,OU=MyOrganization,DC=MyDomain");
 		backingMap.put("realm.ldap.groupPattern", "(&(objectClass=group)(member=${dn}))");
 		backingMap.put("realm.ldap.admins", "UserThree @Git_Admins \"@Git Admins\"");
+		backingMap.put("realm.ldap.displayName", "displayName");
+		backingMap.put("realm.ldap.email", "email");
 		
 		MemorySettings ms = new MemorySettings(backingMap);
-		
-		ldapUserService = new LdapUserService();
-		ldapUserService.setup(ms);
+		return ms;
 	}
 	
 	@Test
@@ -101,5 +109,48 @@ public class LdapUserServiceTest {
 		assertNull(userThreeModel.getTeam("git_admins"));
 		assertTrue(userThreeModel.canAdmin);
 	}
+	
+	@Test
+	public void testDisplayName() {
+		UserModel userOneModel = ldapUserService.authenticate("UserOne", "userOnePassword".toCharArray());
+		assertNotNull(userOneModel);
+		assertEquals("User One", userOneModel.displayName);
+		
+		// Test more complicated scenarios - concat
+		MemorySettings ms = getSettings();
+		ms.put("realm.ldap.displayName", "${personalTitle}. ${givenName} ${surname}");
+		ldapUserService = new LdapUserService();
+		ldapUserService.setup(ms);
+		
+		userOneModel = ldapUserService.authenticate("UserOne", "userOnePassword".toCharArray());
+		assertNotNull(userOneModel);
+		assertEquals("Mr. User One", userOneModel.displayName);
+	}
+	
+	@Test
+	public void testEmail() {
+		UserModel userOneModel = ldapUserService.authenticate("UserOne", "userOnePassword".toCharArray());
+		assertNotNull(userOneModel);
+		assertEquals("userone@gitblit.com", userOneModel.emailAddress);
+		
+		// Test more complicated scenarios - concat
+		MemorySettings ms = getSettings();
+		ms.put("realm.ldap.email", "${givenName}.${surname}@gitblit.com");
+		ldapUserService = new LdapUserService();
+		ldapUserService.setup(ms);
+		
+		userOneModel = ldapUserService.authenticate("UserOne", "userOnePassword".toCharArray());
+		assertNotNull(userOneModel);
+		assertEquals("User.One@gitblit.com", userOneModel.emailAddress);
+	}
+	
+	@Test
+	public void testLdapInjection() {
+		// Inject so "(&(objectClass=person)(sAMAccountName=${username}))" becomes "(&(objectClass=person)(sAMAccountName=*)(userPassword=userOnePassword))"
+		// Thus searching by password
+		
+		UserModel userOneModel = ldapUserService.authenticate("*)(userPassword=userOnePassword", "userOnePassword".toCharArray());
+		assertNull(userOneModel);
+	}
 
 }

tests/com/gitblit/tests/resources/ldapUserServiceSampleData.ldif

@@ -62,6 +62,11 @@ objectClass: user
 objectClass: person
 sAMAccountName: UserOne
 userPassword: userOnePassword
+displayName: User One
+givenName: User
+surname: One
+personalTitle: Mr
+email: userone@gitblit.com
 memberOf: CN=Git_Admins,OU=Groups,OU=UserControl,OU=MyOrganization,DC=MyDomain
 memberOf: CN=Git_Users,OU=Groups,OU=UserControl,OU=MyOrganization,DC=MyDomain
 
@@ -70,6 +75,11 @@ objectClass: user
 objectClass: person
 sAMAccountName: UserTwo
 userPassword: userTwoPassword
+displayName: User Two
+givenName: User
+surname: Two
+personalTitle: Mr
+email: usertwo@gitblit.com
 memberOf: CN=Git_Users,OU=Groups,OU=UserControl,OU=MyOrganization,DC=MyDomain
 memberOf: CN=Git Admins,OU=Groups,OU=UserControl,OU=MyOrganization,DC=MyDomain
 
@@ -78,6 +88,11 @@ objectClass: user
 objectClass: person
 sAMAccountName: UserThree
 userPassword: userThreePassword
+displayName: User Three
+givenName: User
+surname: Three
+personalTitle: Mrs
+email: userthree@gitblit.com
 memberOf: CN=Git_Users,OU=Groups,OU=UserControl,OU=MyOrganization,DC=MyDomain
 
 dn: CN=UserFour,OU=Canada,OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain
@@ -85,4 +100,9 @@ objectClass: user
 objectClass: person
 sAMAccountName: UserFour
 userPassword: userFourPassword
+displayName: User Four
+givenName: User
+surname: Four
+personalTitle: Miss
+email: userfour@gitblit.com
 memberOf: CN=Git_Users,OU=Groups,OU=UserControl,OU=MyOrganization,DC=MyDomain