Merge pull request #1152 from fzs/fixAdminRoleLDAP

Set "can admin" permission on LDAP users and teams correctly

src/main/java/com/gitblit/auth/LdapAuthProvider.java

@@ -171,6 +171,8 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 							final Map<String, TeamModel> userTeams = new HashMap<String, TeamModel>();
 							for (UserModel user : ldapUsers.values()) {
 								for (TeamModel userTeam : user.teams) {
+									// Is this an administrative team?
+									setAdminAttribute(userTeam);
 									userTeams.put(userTeam.name, userTeam);
 								}
 							}
@@ -238,10 +240,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
     public boolean supportsRoleChanges(UserModel user, Role role) {
     	if (Role.ADMIN == role) {
     		if (!supportsTeamMembershipChanges()) {
-    			List<String> admins = settings.getStrings(Keys.realm.ldap.admins);
-    			if (admins.contains(user.username)) {
-    				return false;
-    			}
+				return false;
     		}
     	}
         return true;
@@ -251,10 +250,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 	public boolean supportsRoleChanges(TeamModel team, Role role) {
 		if (Role.ADMIN == role) {
     		if (!supportsTeamMembershipChanges()) {
-    			List<String> admins = settings.getStrings(Keys.realm.ldap.admins);
-    			if (admins.contains("@" + team.name)) {
-    				return false;
-    			}
+				return false;
     		}
     	}
 		return true;
@@ -325,6 +321,8 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 
 							if (!supportsTeamMembershipChanges()) {
 								for (TeamModel userTeam : user.teams) {
+									// Is this an administrative team?
+									setAdminAttribute(userTeam);
 									updateTeam(userTeam);
 								}
 							}
@@ -355,10 +353,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 			if (!ArrayUtils.isEmpty(admins)) {
 				user.canAdmin = false;
 				for (String admin : admins) {
-					if (admin.startsWith("@") && user.isTeamMember(admin.substring(1))) {
-						// admin team
-						user.canAdmin = true;
-					} else if (user.getName().equalsIgnoreCase(admin)) {
+					if (user.getName().equalsIgnoreCase(admin)) {
 						// admin user
 						user.canAdmin = true;
 					}
@@ -367,6 +362,30 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 		}
 	}
 
+	/**
+	 * Set the canAdmin attribute for team retrieved from LDAP.
+	 * If we are not storing teams in LDAP and/or we have not defined any
+	 * administrator teams, then do not change the admin flag.
+	 *
+	 * @param team
+	 */
+	private void setAdminAttribute(TeamModel team) {
+		if (!supportsTeamMembershipChanges()) {
+			List<String> admins = settings.getStrings(Keys.realm.ldap.admins);
+			// if we have defined administrative teams, then set admin flag
+			// otherwise leave admin flag unchanged
+			if (!ArrayUtils.isEmpty(admins)) {
+				team.canAdmin = false;
+				for (String admin : admins) {
+					if (admin.startsWith("@") && team.name.equalsIgnoreCase(admin.substring(1))) {
+						// admin team
+						team.canAdmin = true;
+					}
+				}
+			}
+		}
+	}
+
 	private void setUserAttributes(UserModel user, SearchResultEntry userEntry) {
 		// Is this user an admin?
 		setAdminAttribute(user);
@@ -462,6 +481,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 					TeamModel teamModel = userManager.getTeamModel(teamName);
 					if (teamModel == null) {
 						teamModel = createTeamFromLdap(teamEntry);
+						setAdminAttribute(teamModel);
 						userManager.updateTeamModel(teamModel);
 					}
 				}

src/test/java/com/gitblit/tests/LdapAuthenticationTest.java

@@ -296,7 +296,6 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
 		assertNotNull(userOneModel);
 		assertNotNull(userOneModel.getTeam("git_admins"));
 		assertNotNull(userOneModel.getTeam("git_users"));
-		assertTrue(userOneModel.canAdmin);
 
 		UserModel userOneModelFailedAuth = ldap.authenticate("UserOne", "userTwoPassword".toCharArray());
 		assertNull(userOneModelFailedAuth);
@@ -306,13 +305,49 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
 		assertNotNull(userTwoModel.getTeam("git_users"));
 		assertNull(userTwoModel.getTeam("git_admins"));
 		assertNotNull(userTwoModel.getTeam("git admins"));
-		assertTrue(userTwoModel.canAdmin);
 
 		UserModel userThreeModel = ldap.authenticate("UserThree", "userThreePassword".toCharArray());
 		assertNotNull(userThreeModel);
 		assertNotNull(userThreeModel.getTeam("git_users"));
 		assertNull(userThreeModel.getTeam("git_admins"));
+
+		UserModel userFourModel = ldap.authenticate("UserFour", "userFourPassword".toCharArray());
+		assertNotNull(userFourModel);
+		assertNotNull(userFourModel.getTeam("git_users"));
+		assertNull(userFourModel.getTeam("git_admins"));
+		assertNull(userFourModel.getTeam("git admins"));
+	}
+
+	@Test
+	public void testAdminPropertyTeamsInLdap() {
+		UserModel userOneModel = ldap.authenticate("UserOne", "userOnePassword".toCharArray());
+		assertNotNull(userOneModel);
+		assertNotNull(userOneModel.getTeam("git_admins"));
+		assertNull(userOneModel.getTeam("git admins"));
+		assertNotNull(userOneModel.getTeam("git_users"));
+		assertFalse(userOneModel.canAdmin);
+		assertTrue(userOneModel.canAdmin());
+		assertTrue(userOneModel.getTeam("git_admins").canAdmin);
+		assertFalse(userOneModel.getTeam("git_users").canAdmin);
+
+		UserModel userTwoModel = ldap.authenticate("UserTwo", "userTwoPassword".toCharArray());
+		assertNotNull(userTwoModel);
+		assertNotNull(userTwoModel.getTeam("git_users"));
+		assertNull(userTwoModel.getTeam("git_admins"));
+		assertNotNull(userTwoModel.getTeam("git admins"));
+		assertFalse(userTwoModel.canAdmin);
+		assertTrue(userTwoModel.canAdmin());
+		assertTrue(userTwoModel.getTeam("git admins").canAdmin);
+		assertFalse(userTwoModel.getTeam("git_users").canAdmin);
+
+		UserModel userThreeModel = ldap.authenticate("UserThree", "userThreePassword".toCharArray());
+		assertNotNull(userThreeModel);
+		assertNotNull(userThreeModel.getTeam("git_users"));
+		assertNull(userThreeModel.getTeam("git_admins"));
+		assertNull(userThreeModel.getTeam("git admins"));
 		assertTrue(userThreeModel.canAdmin);
+		assertTrue(userThreeModel.canAdmin());
+		assertFalse(userThreeModel.getTeam("git_users").canAdmin);
 
 		UserModel userFourModel = ldap.authenticate("UserFour", "userFourPassword".toCharArray());
 		assertNotNull(userFourModel);
@@ -320,6 +355,51 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
 		assertNull(userFourModel.getTeam("git_admins"));
 		assertNull(userFourModel.getTeam("git admins"));
 		assertFalse(userFourModel.canAdmin);
+		assertFalse(userFourModel.canAdmin());
+		assertFalse(userFourModel.getTeam("git_users").canAdmin);
+	}
+
+	@Test
+	public void testAdminPropertyTeamsNotInLdap() {
+		settings.put(Keys.realm.ldap.maintainTeams, "false");
+
+		UserModel userOneModel = ldap.authenticate("UserOne", "userOnePassword".toCharArray());
+		assertNotNull(userOneModel);
+		assertNotNull(userOneModel.getTeam("git_admins"));
+		assertNull(userOneModel.getTeam("git admins"));
+		assertNotNull(userOneModel.getTeam("git_users"));
+		assertTrue(userOneModel.canAdmin);
+		assertTrue(userOneModel.canAdmin());
+		assertFalse(userOneModel.getTeam("git_admins").canAdmin);
+		assertFalse(userOneModel.getTeam("git_users").canAdmin);
+
+		UserModel userTwoModel = ldap.authenticate("UserTwo", "userTwoPassword".toCharArray());
+		assertNotNull(userTwoModel);
+		assertNotNull(userTwoModel.getTeam("git_users"));
+		assertNull(userTwoModel.getTeam("git_admins"));
+		assertNotNull(userTwoModel.getTeam("git admins"));
+		assertFalse(userTwoModel.canAdmin);
+		assertTrue(userTwoModel.canAdmin());
+		assertTrue(userTwoModel.getTeam("git admins").canAdmin);
+		assertFalse(userTwoModel.getTeam("git_users").canAdmin);
+
+		UserModel userThreeModel = ldap.authenticate("UserThree", "userThreePassword".toCharArray());
+		assertNotNull(userThreeModel);
+		assertNotNull(userThreeModel.getTeam("git_users"));
+		assertNull(userThreeModel.getTeam("git_admins"));
+		assertNull(userThreeModel.getTeam("git admins"));
+		assertFalse(userThreeModel.canAdmin);
+		assertFalse(userThreeModel.canAdmin());
+		assertFalse(userThreeModel.getTeam("git_users").canAdmin);
+
+		UserModel userFourModel = ldap.authenticate("UserFour", "userFourPassword".toCharArray());
+		assertNotNull(userFourModel);
+		assertNotNull(userFourModel.getTeam("git_users"));
+		assertNull(userFourModel.getTeam("git_admins"));
+		assertNull(userFourModel.getTeam("git admins"));
+		assertFalse(userFourModel.canAdmin);
+		assertFalse(userFourModel.canAdmin());
+		assertFalse(userFourModel.getTeam("git_users").canAdmin);
 	}
 
 	@Test
@@ -391,6 +471,17 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
 		assertEquals("Number of ldap groups in gitblit team model", 0, countLdapTeamsInUserManager());
 	}
 
+	@Test
+	public void addingGroupsInLdapShouldUpdateGitBlitUsersNotGroups2() throws Exception {
+		settings.put(Keys.realm.ldap.synchronize, "true");
+		settings.put(Keys.realm.ldap.maintainTeams, "false");
+		getDS().addEntries(LDIFReader.readEntries(RESOURCE_DIR + "adduser.ldif"));
+		getDS().addEntries(LDIFReader.readEntries(RESOURCE_DIR + "addgroup.ldif"));
+		ldap.sync();
+		assertEquals("Number of ldap users in gitblit user model", 6, countLdapUsersInUserManager());
+		assertEquals("Number of ldap groups in gitblit team model", 0, countLdapTeamsInUserManager());
+	}
+
 	@Test
 	public void addingGroupsInLdapShouldUpdateGitBlitUsersAndGroups() throws Exception {
 		// This test only makes sense if the authentication mode allows for synchronization.
@@ -402,13 +493,92 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
 		assertEquals("Number of ldap groups in gitblit team model", 1, countLdapTeamsInUserManager());
 	}
 
+	@Test
+	public void syncUpdateUsersAndGroupsAdminProperty() throws Exception {
+		// This test only makes sense if the authentication mode allows for synchronization.
+		assumeTrue(authMode == AuthMode.ANONYMOUS || authMode == AuthMode.DS_MANAGER);
+
+		settings.put(Keys.realm.ldap.synchronize, "true");
+		ldap.sync();
+
+		UserModel user = userManager.getUserModel("UserOne");
+		assertNotNull(user);
+		assertFalse(user.canAdmin);
+		assertTrue(user.canAdmin());
+
+		user = userManager.getUserModel("UserTwo");
+		assertNotNull(user);
+		assertFalse(user.canAdmin);
+		assertTrue(user.canAdmin());
+
+		user = userManager.getUserModel("UserThree");
+		assertNotNull(user);
+		assertTrue(user.canAdmin);
+		assertTrue(user.canAdmin());
+
+		user = userManager.getUserModel("UserFour");
+		assertNotNull(user);
+		assertFalse(user.canAdmin);
+		assertFalse(user.canAdmin());
+
+		TeamModel team = userManager.getTeamModel("Git_Admins");
+		assertNotNull(team);
+		assertTrue(team.canAdmin);
+
+		team = userManager.getTeamModel("Git Admins");
+		assertNotNull(team);
+		assertTrue(team.canAdmin);
+
+		team = userManager.getTeamModel("Git_Users");
+		assertNotNull(team);
+		assertFalse(team.canAdmin);
+	}
+
+	@Test
+	public void syncNotUpdateUsersAndGroupsAdminProperty() throws Exception {
+		settings.put(Keys.realm.ldap.synchronize, "true");
+		settings.put(Keys.realm.ldap.maintainTeams, "false");
+		ldap.sync();
+
+		UserModel user = userManager.getUserModel("UserOne");
+		assertNotNull(user);
+		assertTrue(user.canAdmin);
+		assertTrue(user.canAdmin());
+
+		user = userManager.getUserModel("UserTwo");
+		assertNotNull(user);
+		assertFalse(user.canAdmin);
+		assertTrue(user.canAdmin());
+
+		user = userManager.getUserModel("UserThree");
+		assertNotNull(user);
+		assertFalse(user.canAdmin);
+		assertFalse(user.canAdmin());
+
+		user = userManager.getUserModel("UserFour");
+		assertNotNull(user);
+		assertFalse(user.canAdmin);
+		assertFalse(user.canAdmin());
+
+		TeamModel team = userManager.getTeamModel("Git_Admins");
+		assertNotNull(team);
+		assertFalse(team.canAdmin);
+
+		team = userManager.getTeamModel("Git Admins");
+		assertNotNull(team);
+		assertTrue(team.canAdmin);
+
+		team = userManager.getTeamModel("Git_Users");
+		assertNotNull(team);
+		assertFalse(team.canAdmin);
+	}
+
 	@Test
 	public void testAuthenticationManager() {
 		UserModel userOneModel = auth.authenticate("UserOne", "userOnePassword".toCharArray(), null);
 		assertNotNull(userOneModel);
 		assertNotNull(userOneModel.getTeam("git_admins"));
 		assertNotNull(userOneModel.getTeam("git_users"));
-		assertTrue(userOneModel.canAdmin);
 
 		UserModel userOneModelFailedAuth = auth.authenticate("UserOne", "userTwoPassword".toCharArray(), null);
 		assertNull(userOneModelFailedAuth);
@@ -418,13 +588,98 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
 		assertNotNull(userTwoModel.getTeam("git_users"));
 		assertNull(userTwoModel.getTeam("git_admins"));
 		assertNotNull(userTwoModel.getTeam("git admins"));
-		assertTrue(userTwoModel.canAdmin);
 
 		UserModel userThreeModel = auth.authenticate("UserThree", "userThreePassword".toCharArray(), null);
 		assertNotNull(userThreeModel);
 		assertNotNull(userThreeModel.getTeam("git_users"));
 		assertNull(userThreeModel.getTeam("git_admins"));
+
+		UserModel userFourModel = auth.authenticate("UserFour", "userFourPassword".toCharArray(), null);
+		assertNotNull(userFourModel);
+		assertNotNull(userFourModel.getTeam("git_users"));
+		assertNull(userFourModel.getTeam("git_admins"));
+		assertNull(userFourModel.getTeam("git admins"));
+	}
+
+	@Test
+	public void testAuthenticationManagerAdminPropertyTeamsInLdap() {
+		UserModel userOneModel = auth.authenticate("UserOne", "userOnePassword".toCharArray(), null);
+		assertNotNull(userOneModel);
+		assertNotNull(userOneModel.getTeam("git_admins"));
+		assertNull(userOneModel.getTeam("git admins"));
+		assertNotNull(userOneModel.getTeam("git_users"));
+		assertFalse(userOneModel.canAdmin);
+		assertTrue(userOneModel.canAdmin());
+		assertTrue(userOneModel.getTeam("git_admins").canAdmin);
+		assertFalse(userOneModel.getTeam("git_users").canAdmin);
+
+		UserModel userOneModelFailedAuth = auth.authenticate("UserOne", "userTwoPassword".toCharArray(), null);
+		assertNull(userOneModelFailedAuth);
+
+		UserModel userTwoModel = auth.authenticate("UserTwo", "userTwoPassword".toCharArray(), null);
+		assertNotNull(userTwoModel);
+		assertNotNull(userTwoModel.getTeam("git_users"));
+		assertNull(userTwoModel.getTeam("git_admins"));
+		assertNotNull(userTwoModel.getTeam("git admins"));
+		assertFalse(userTwoModel.canAdmin);
+		assertTrue(userTwoModel.canAdmin());
+		assertTrue(userTwoModel.getTeam("git admins").canAdmin);
+		assertFalse(userTwoModel.getTeam("git_users").canAdmin);
+
+		UserModel userThreeModel = auth.authenticate("UserThree", "userThreePassword".toCharArray(), null);
+		assertNotNull(userThreeModel);
+		assertNotNull(userThreeModel.getTeam("git_users"));
+		assertNull(userThreeModel.getTeam("git_admins"));
+		assertNull(userThreeModel.getTeam("git admins"));
 		assertTrue(userThreeModel.canAdmin);
+		assertTrue(userThreeModel.canAdmin());
+		assertFalse(userThreeModel.getTeam("git_users").canAdmin);
+
+		UserModel userFourModel = auth.authenticate("UserFour", "userFourPassword".toCharArray(), null);
+		assertNotNull(userFourModel);
+		assertNotNull(userFourModel.getTeam("git_users"));
+		assertNull(userFourModel.getTeam("git_admins"));
+		assertNull(userFourModel.getTeam("git admins"));
+		assertFalse(userFourModel.canAdmin);
+		assertFalse(userFourModel.canAdmin());
+		assertFalse(userFourModel.getTeam("git_users").canAdmin);
+	}
+
+	@Test
+	public void testAuthenticationManagerAdminPropertyTeamsNotInLdap() {
+		settings.put(Keys.realm.ldap.maintainTeams, "false");
+
+		UserModel userOneModel = auth.authenticate("UserOne", "userOnePassword".toCharArray(), null);
+		assertNotNull(userOneModel);
+		assertNotNull(userOneModel.getTeam("git_admins"));
+		assertNull(userOneModel.getTeam("git admins"));
+		assertNotNull(userOneModel.getTeam("git_users"));
+		assertTrue(userOneModel.canAdmin);
+		assertTrue(userOneModel.canAdmin());
+		assertFalse(userOneModel.getTeam("git_admins").canAdmin);
+		assertFalse(userOneModel.getTeam("git_users").canAdmin);
+
+		UserModel userOneModelFailedAuth = auth.authenticate("UserOne", "userTwoPassword".toCharArray(), null);
+		assertNull(userOneModelFailedAuth);
+
+		UserModel userTwoModel = auth.authenticate("UserTwo", "userTwoPassword".toCharArray(), null);
+		assertNotNull(userTwoModel);
+		assertNotNull(userTwoModel.getTeam("git_users"));
+		assertNull(userTwoModel.getTeam("git_admins"));
+		assertNotNull(userTwoModel.getTeam("git admins"));
+		assertFalse(userTwoModel.canAdmin);
+		assertTrue(userTwoModel.canAdmin());
+		assertTrue(userTwoModel.getTeam("git admins").canAdmin);
+		assertFalse(userTwoModel.getTeam("git_users").canAdmin);
+
+		UserModel userThreeModel = auth.authenticate("UserThree", "userThreePassword".toCharArray(), null);
+		assertNotNull(userThreeModel);
+		assertNotNull(userThreeModel.getTeam("git_users"));
+		assertNull(userThreeModel.getTeam("git_admins"));
+		assertNull(userThreeModel.getTeam("git admins"));
+		assertFalse(userThreeModel.canAdmin);
+		assertFalse(userThreeModel.canAdmin());
+		assertFalse(userThreeModel.getTeam("git_users").canAdmin);
 
 		UserModel userFourModel = auth.authenticate("UserFour", "userFourPassword".toCharArray(), null);
 		assertNotNull(userFourModel);
@@ -432,6 +687,8 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
 		assertNull(userFourModel.getTeam("git_admins"));
 		assertNull(userFourModel.getTeam("git admins"));
 		assertFalse(userFourModel.canAdmin);
+		assertFalse(userFourModel.canAdmin());
+		assertFalse(userFourModel.getTeam("git_users").canAdmin);
 	}
 
 	@Test

src/test/resources/ldap/users.conf

@@ -10,7 +10,7 @@
 	displayName = Mrs. User Three
 	emailAddress = userthree@gitblit.com
 	accountType = LDAP
-	role = "#admin"
+	role = "#none"
 [user "userfive"]
 	password = "#externalAccount"
 	cookie = 220bafef069b8b399b2597644015b6b0f4667982
@@ -31,7 +31,7 @@
 	displayName = Mr. User Two
 	emailAddress = usertwo@gitblit.com
 	accountType = LDAP
-	role = "#admin"
+	role = "#none"
 [user "basic"]
 	password = MD5:f17aaabc20bfe045075927934fed52d2
 	cookie = dd94709528bb1c83d08f3088d4043f4742891f4f
@@ -63,6 +63,6 @@
 	user = userthree
 	user = userfour
 [team "Git Admins"]
-	role = "#none"
+	role = "#admin"
 	accountType = LOCAL
 	user = usertwo