API adjustments and elimination of duplicate config options

releases.moxie

@@ -33,6 +33,7 @@ r20: {
 	- Reversed line links in blob view (issue-309)
 	- Dashboard and Activity pages now obey the web.generateActivityGraph setting (issue-310)
 	- Do not log passwords on failed authentication attempts (issue-316)
+	- LDAP synchronization is now scheduled rather than on-demand (issue-336)
 	- Show displayname and username in palettes (issue-364)
 	- Updated default binary and Lucene ignore extensions
 	- Change the WAR baseFolder context parameter to a JNDI env-entry to improve enterprise deployments
@@ -82,6 +83,10 @@ r20: {
 	- { name: 'git.defaultAccessRestriction', defaultValue: 'PUSH' }
 	- { name: 'git.mirrorPeriod', defaultValue: '30 mins' }
 	- { name: 'realm.authenticationProviders', defaultValue: ' ' }
+	- { name: 'realm.ldap.groupEmptyMemberPattern', defaultValue: '(&(objectClass=group)(!(member=*)))' }
+	- { name: 'realm.ldap.synchronize', defaultValue: 'false' }
+	- { name: 'realm.ldap.syncPeriod', defaultValue: '5 MINUTES' }
+	- { name: 'realm.ldap.removeDeletedUsers', defaultValue: 'true' }
 	- { name: 'web.commitMessageRenderer', defaultValue: 'plain' }
 	- { name: 'web.documents', defaultValue: 'readme home index changelog contributing submitting_patches copying license notice authors' }
 	- { name: 'web.showBranchGraph', defaultValue: 'true' }

src/main/distrib/data/gitblit.properties

@@ -1465,7 +1465,7 @@ realm.ldap.groupMemberPattern = (&(objectClass=group)(member=${dn}))
 # Query pattern to use when searching for an empty team. This may be any valid 
 # LDAP query expression, including the standard (&) and (|) operators.
 #
-#default: (&(objectClass=group)(!(member=*)))
+# default: (&(objectClass=group)(!(member=*)))
 # SINCE 1.4.0
 realm.ldap.groupEmptyMemberPattern = (&(objectClass=group)(!(member=*)))
 
@@ -1502,46 +1502,44 @@ realm.ldap.displayName = displayName
 # SINCE 1.0.0
 realm.ldap.email = email
 
-# Defines the cache period to be used when caching LDAP queries. This is currently
-# only used for LDAP user synchronization.
-#
-# Must be of the form '<long> <TimeUnit>' where <TimeUnit> is one of 'MILLISECONDS', 'SECONDS', 'MINUTES', 'HOURS', 'DAYS' 
-# default: 2 MINUTES
+# Attribute on the USER record that indicate their username to be used in gitblit
+# when synchronizing users from LDAP
+# if blank, Gitblit will use uid
+# For MS Active Directory this may be sAMAccountName
 #
-# RESTART REQUIRED
-realm.ldap.ldapCachePeriod = 2 MINUTES
+# SINCE 1.0.0
+realm.ldap.uid = uid
 
-# Defines whether to synchronize all LDAP users into the backing user service
+# Defines whether to synchronize all LDAP users and teams into the user service
 #
 # Valid values: true, false
 # If left blank, false is assumed
-realm.ldap.synchronizeUsers.enable = false
+#
+# SINCE 1.4.0
+realm.ldap.synchronize = false
 
-# Defines the period to be used when synchronizing users from ldap. This is currently
-# only used for LDAP user synchronization.
+# Defines the period to be used when synchronizing users and teams from ldap.
 #
 # Must be of the form '<long> <TimeUnit>' where <TimeUnit> is one of 'MILLISECONDS', 'SECONDS', 'MINUTES', 'HOURS', 'DAYS' 
-# <long> is at least the value from realm.ldap.ldapCachePeriod if lower the value from realm.ldap.ldapCachePeriod is used.
+
 # default: 5 MINUTES
 #
 # RESTART REQUIRED
 # SINCE 1.4.0
-realm.ldap.synchronizeUsers.ldapSyncPeriod = 5 MINUTES
+realm.ldap.syncPeriod = 5 MINUTES
 
-# Defines whether to delete non-existent LDAP users from the backing user service
-# during synchronization. depends on  realm.ldap.synchronizeUsers.enable = true
+# Defines whether to delete non-existent LDAP users from the user service
+# during synchronization. depends on  realm.ldap.synchronize = true
 #
 # Valid values: true, false
 # If left blank, true is assumed
-realm.ldap.synchronizeUsers.removeDeleted = true
-
-# Attribute on the USER record that indicate their username to be used in gitblit
-# when synchronizing users from LDAP
-# if blank, Gitblit will use uid
-# For MS Active Directory this may be sAMAccountName
-realm.ldap.uid = uid
+#
+# SINCE 1.4.0
+realm.ldap.removeDeletedUsers = true
 
 # URL of the Redmine.
+#
+# SINCE 1.2.0
 realm.redmine.url = http://example.com/redmine
 
 #

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

@@ -100,6 +100,8 @@ public abstract class AuthenticationProvider {
 
 	public abstract void setup();
 
+	public abstract void stop();
+
 	public abstract UserModel authenticate(String username, char[] password);
 
 	public abstract AccountType getAccountType();
@@ -145,6 +147,11 @@ public abstract class AuthenticationProvider {
     	protected UsernamePasswordAuthenticationProvider(String serviceName) {
     		super(serviceName);
     	}
+
+    	@Override
+		public void stop() {
+
+		}
     }
 
     public static class NullProvider extends AuthenticationProvider {
@@ -158,6 +165,11 @@ public abstract class AuthenticationProvider {
 
 		}
 
+		@Override
+		public void stop() {
+
+		}
+
 		@Override
 		public UserModel authenticate(String username, char[] password) {
 			return null;

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

@@ -19,6 +19,7 @@ package com.gitblit.auth;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.security.GeneralSecurityException;
+import java.text.MessageFormat;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
@@ -26,7 +27,6 @@ import java.util.Map;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicLong;
 
 import com.gitblit.Constants;
 import com.gitblit.Constants.AccountType;
@@ -60,109 +60,119 @@ import com.unboundid.util.ssl.TrustAllTrustManager;
  */
 public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 
-    private final AtomicLong lastLdapUserSync = new AtomicLong(0L);
+	private final ScheduledExecutorService scheduledExecutorService;
 
 	public LdapAuthProvider() {
 		super("ldap");
+
+		scheduledExecutorService = Executors.newSingleThreadScheduledExecutor();
 	}
 
- 	private long getSynchronizationPeriodInMilliseconds(String name) {
-        final String cacheDuration = settings.getString(name, "2 MINUTES");
+ 	private long getSynchronizationPeriodInMilliseconds() {
+ 		String period = settings.getString(Keys.realm.ldap.syncPeriod, null);
+ 		if (StringUtils.isEmpty(period)) {
+ 	 		period = settings.getString("realm.ldap.ldapCachePeriod", null);
+ 	 		if (StringUtils.isEmpty(period)) {
+ 	 			period = "5 MINUTES";
+ 	 		} else {
+ 	 			logger.warn("realm.ldap.ldapCachePeriod is obsolete!");
+ 	 			logger.warn(MessageFormat.format("Please set {0}={1} in gitblit.properties!", Keys.realm.ldap.syncPeriod, period));
+ 	 			settings.overrideSetting(Keys.realm.ldap.syncPeriod, period);
+ 	 		}
+ 		}
+
         try {
-            final String[] s = cacheDuration.split(" ", 2);
+            final String[] s = period.split(" ", 2);
             long duration = Math.abs(Long.parseLong(s[0]));
             TimeUnit timeUnit = TimeUnit.valueOf(s[1]);
             return timeUnit.toMillis(duration);
         } catch (RuntimeException ex) {
-            throw new IllegalArgumentException(name + " must have format '<long> <TimeUnit>' where <TimeUnit> is one of 'MILLISECONDS', 'SECONDS', 'MINUTES', 'HOURS', 'DAYS'");
+            throw new IllegalArgumentException(Keys.realm.ldap.syncPeriod + " must have format '<long> <TimeUnit>' where <TimeUnit> is one of 'MILLISECONDS', 'SECONDS', 'MINUTES', 'HOURS', 'DAYS'");
         }
     }
 
 	@Override
 	public void setup() {
-		synchronizeLdapUsers();
-		configureLdapSyncService();
+		configureSyncService();
 	}
-	
-	public void synchronizeWithLdapService() {
-		synchronizeLdapUsers();
+
+	@Override
+	public void stop() {
+		scheduledExecutorService.shutdownNow();
 	}
 
-	protected synchronized void synchronizeLdapUsers() {
-        final boolean enabled = settings.getBoolean(Keys.realm.ldap.synchronizeUsers.enable, false);
-        if (enabled) {
-            if (System.currentTimeMillis() > (lastLdapUserSync.get() + getSynchronizationPeriodInMilliseconds(Keys.realm.ldap.ldapCachePeriod))) {
-            	logger.info("Synchronizing with LDAP @ " + settings.getRequiredString(Keys.realm.ldap.server));
-                final boolean deleteRemovedLdapUsers = settings.getBoolean(Keys.realm.ldap.synchronizeUsers.removeDeleted, true);
-                LDAPConnection ldapConnection = getLdapConnection();
-                if (ldapConnection != null) {
-                    try {
-                        String accountBase = settings.getString(Keys.realm.ldap.accountBase, "");
-                        String uidAttribute = settings.getString(Keys.realm.ldap.uid, "uid");
-                        String accountPattern = settings.getString(Keys.realm.ldap.accountPattern, "(&(objectClass=person)(sAMAccountName=${username}))");
-                        accountPattern = StringUtils.replace(accountPattern, "${username}", "*");
-
-                        SearchResult result = doSearch(ldapConnection, accountBase, accountPattern);
-                        if (result != null && result.getEntryCount() > 0) {
-                            final Map<String, UserModel> ldapUsers = new HashMap<String, UserModel>();
-
-                            for (SearchResultEntry loggingInUser : result.getSearchEntries()) {
-
-                                final String username = loggingInUser.getAttribute(uidAttribute).getValue();
-                                logger.debug("LDAP synchronizing: " + username);
-
-                                UserModel user = userManager.getUserModel(username);
-                                if (user == null) {
-                                    user = new UserModel(username);
-                                }
-
-                                if (!supportsTeamMembershipChanges()) {
-                                    getTeamsFromLdap(ldapConnection, username, loggingInUser, user);
-                                }
-
-                                // Get User Attributes
-                                setUserAttributes(user, loggingInUser);
-
-                                // store in map
-                                ldapUsers.put(username.toLowerCase(), user);
-                            }
-
-                            if (deleteRemovedLdapUsers) {
-                                logger.debug("detecting removed LDAP users...");
-
-                                for (UserModel userModel : userManager.getAllUsers()) {
-                                    if (Constants.EXTERNAL_ACCOUNT.equals(userModel.password)) {
-                                        if (!ldapUsers.containsKey(userModel.username)) {
-                                            logger.info("deleting removed LDAP user " + userModel.username + " from user service");
-                                            userManager.deleteUser(userModel.username);
-                                        }
-                                    }
-                                }
-                            }
-
-                            userManager.updateUserModels(ldapUsers.values());
-
-                            if (!supportsTeamMembershipChanges()) {
-                                final Map<String, TeamModel> userTeams = new HashMap<String, TeamModel>();
-                                for (UserModel user : ldapUsers.values()) {
-                                    for (TeamModel userTeam : user.teams) {
-                                        userTeams.put(userTeam.name, userTeam);
-                                    }
-                                }
-                                userManager.updateTeamModels(userTeams.values());
-                            }
-                        }
-                        if (!supportsTeamMembershipChanges()) {
-                        	getEmptyTeamsFromLdap(ldapConnection);
-                        }
-                        lastLdapUserSync.set(System.currentTimeMillis());
-                    } finally {
-                        ldapConnection.close();
-                    }
-                }
-            }
-        }
-    }
+	public synchronized void sync() {
+		final boolean enabled = settings.getBoolean(Keys.realm.ldap.synchronize, false);
+		if (enabled) {
+			logger.info("Synchronizing with LDAP @ " + settings.getRequiredString(Keys.realm.ldap.server));
+			final boolean deleteRemovedLdapUsers = settings.getBoolean(Keys.realm.ldap.removeDeletedUsers, true);
+			LDAPConnection ldapConnection = getLdapConnection();
+			if (ldapConnection != null) {
+				try {
+					String accountBase = settings.getString(Keys.realm.ldap.accountBase, "");
+					String uidAttribute = settings.getString(Keys.realm.ldap.uid, "uid");
+					String accountPattern = settings.getString(Keys.realm.ldap.accountPattern, "(&(objectClass=person)(sAMAccountName=${username}))");
+					accountPattern = StringUtils.replace(accountPattern, "${username}", "*");
+
+					SearchResult result = doSearch(ldapConnection, accountBase, accountPattern);
+					if (result != null && result.getEntryCount() > 0) {
+						final Map<String, UserModel> ldapUsers = new HashMap<String, UserModel>();
+
+						for (SearchResultEntry loggingInUser : result.getSearchEntries()) {
+
+							final String username = loggingInUser.getAttribute(uidAttribute).getValue();
+							logger.debug("LDAP synchronizing: " + username);
+
+							UserModel user = userManager.getUserModel(username);
+							if (user == null) {
+								user = new UserModel(username);
+							}
+
+							if (!supportsTeamMembershipChanges()) {
+								getTeamsFromLdap(ldapConnection, username, loggingInUser, user);
+							}
+
+							// Get User Attributes
+							setUserAttributes(user, loggingInUser);
+
+							// store in map
+							ldapUsers.put(username.toLowerCase(), user);
+						}
+
+						if (deleteRemovedLdapUsers) {
+							logger.debug("detecting removed LDAP users...");
+
+							for (UserModel userModel : userManager.getAllUsers()) {
+								if (AccountType.LDAP == userModel.accountType) {
+									if (!ldapUsers.containsKey(userModel.username)) {
+										logger.info("deleting removed LDAP user " + userModel.username + " from user service");
+										userManager.deleteUser(userModel.username);
+									}
+								}
+							}
+						}
+
+						userManager.updateUserModels(ldapUsers.values());
+
+						if (!supportsTeamMembershipChanges()) {
+							final Map<String, TeamModel> userTeams = new HashMap<String, TeamModel>();
+							for (UserModel user : ldapUsers.values()) {
+								for (TeamModel userTeam : user.teams) {
+									userTeams.put(userTeam.name, userTeam);
+								}
+							}
+							userManager.updateTeamModels(userTeams.values());
+						}
+					}
+					if (!supportsTeamMembershipChanges()) {
+						getEmptyTeamsFromLdap(ldapConnection);
+					}
+				} finally {
+					ldapConnection.close();
+				}
+			}
+		}
+	}
 
 	private LDAPConnection getLdapConnection() {
 		try {
@@ -439,7 +449,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 	}
 
 	private void getEmptyTeamsFromLdap(LDAPConnection ldapConnection) {
-		logger.info("Start fetching empty teams form ldap.");
+		logger.info("Start fetching empty teams from ldap.");
 		String groupBase = settings.getString(Keys.realm.ldap.groupBase, "");
 		String groupMemberPattern = settings.getString(Keys.realm.ldap.groupEmptyMemberPattern, "(&(objectClass=group)(!(member=*)))");
 
@@ -449,7 +459,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 				SearchResultEntry teamEntry = teamMembershipResult.getSearchEntries().get(i);
 				if (!teamEntry.hasAttribute("member")) {
 					String teamName = teamEntry.getAttribute("cn").getValue();
-	
+
 					TeamModel teamModel = userManager.getTeamModel(teamName);
 					if (teamModel == null) {
 						teamModel = createTeamFromLdap(teamEntry);
@@ -458,7 +468,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 				}
 			}
 		}
-		logger.info("Finished fetching empty teams form ldap.");
+		logger.info("Finished fetching empty teams from ldap.");
 	}
 
 	private TeamModel createTeamFromLdap(SearchResultEntry teamEntry) {
@@ -554,24 +564,16 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 		return sb.toString();
 	}
 
-	private void configureLdapSyncService() {
-		logger.info("Start configuring ldap sync service");
+	private void configureSyncService() {
 		LdapSyncService ldapSyncService = new LdapSyncService(settings, this);
 		if (ldapSyncService.isReady()) {
-			long ldapSyncPeriod = getSynchronizationPeriodInMilliseconds(Keys.realm.ldap.synchronizeUsers.ldapSyncPeriod);
-			long ldapCachePeriod = getSynchronizationPeriodInMilliseconds(Keys.realm.ldap.synchronizeUsers.ldapSyncPeriod);
-			if (ldapSyncPeriod < ldapCachePeriod) {
-				ldapSyncPeriod = ldapCachePeriod;
-			}
+			long ldapSyncPeriod = getSynchronizationPeriodInMilliseconds();
 			int delay = 1;
-			ScheduledExecutorService scheduledExecutorService = Executors.newSingleThreadScheduledExecutor();
+			logger.info("Ldap sync service will update users and groups every {} minutes.", ldapSyncPeriod);
 			scheduledExecutorService.scheduleAtFixedRate(ldapSyncService, delay, ldapSyncPeriod,  TimeUnit.MILLISECONDS);
-			logger.info("Ldap sync service will update user and groups every {} minutes.", ldapSyncPeriod);
-			logger.info("Next scheduled ldap sync is in {} minutes", delay);
 		} else {
 			logger.info("Ldap sync service is disabled.");
 		}
-		logger.info("Finished configuring ldap sync service");
 	}
 
 }

src/main/java/com/gitblit/manager/AuthenticationManager.java

@@ -150,6 +150,13 @@ public class AuthenticationManager implements IAuthenticationManager {
 
 	@Override
 	public AuthenticationManager stop() {
+		for (AuthenticationProvider provider : authenticationProviders) {
+			try {
+				provider.stop();
+			} catch (Exception e) {
+				logger.error("Failed to stop " + provider.getClass().getSimpleName(), e);
+			}
+		}
 		return this;
 	}
 

src/main/java/com/gitblit/service/LdapSyncService.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2013 gitblit.com.
+ * Copyright 2014 gitblit.com.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -26,7 +26,7 @@ import com.gitblit.auth.LdapAuthProvider;
 
 /**
  * @author Alfred Schmid
- * 
+ *
  */
 public final class LdapSyncService implements Runnable {
 
@@ -44,21 +44,26 @@ public final class LdapSyncService implements Runnable {
 	}
 
 	/**
-	 * 
+	 *
 	 * @see java.lang.Runnable#run()
 	 */
 	@Override
 	public void run() {
 		logger.info("Starting user and group sync with ldap service");
 		if (!running.getAndSet(true)) {
-			ldapAuthProvider.synchronizeWithLdapService();
-			running.getAndSet(false);
+			try {
+				ldapAuthProvider.sync();
+			} catch (Exception e) {
+				logger.error("Failed to synchronize with ldap", e);
+			} finally {
+				running.getAndSet(false);
+			}
 		}
 		logger.info("Finished user and group sync with ldap service");
 	}
 
 	public boolean isReady() {
-		return settings.getBoolean(Keys.realm.ldap.synchronizeUsers.enable, false);
+		return settings.getBoolean(Keys.realm.ldap.synchronize, false);
 	}
 
 }

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

@@ -58,7 +58,7 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
 	private static final String RESOURCE_DIR = "src/test/resources/ldap/";
 
     private File usersConf;
-    
+
     private LdapAuthProvider ldap;
 
 	static int ldapPort = 1389;
@@ -196,7 +196,7 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
 	public void addingUserInLdapShouldNotUpdateGitBlitUsersAndGroups() throws Exception {
 		settings.put("realm.ldap.ldapCachePeriod", "0 MINUTES");
 		ds.addEntries(LDIFReader.readEntries(RESOURCE_DIR + "adduser.ldif"));
-		ldap.synchronizeWithLdapService();
+		ldap.sync();
 		assertEquals("Number of ldap users in gitblit user model", 5, countLdapUsersInUserManager());
 	}
 
@@ -205,7 +205,7 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
 		settings.put("realm.ldap.synchronizeUsers.enable", "true");
 		settings.put("realm.ldap.ldapCachePeriod", "0 MINUTES");
 		ds.addEntries(LDIFReader.readEntries(RESOURCE_DIR + "adduser.ldif"));
-		ldap.synchronizeWithLdapService();
+		ldap.sync();
 		assertEquals("Number of ldap users in gitblit user model", 6, countLdapUsersInUserManager());
 	}
 
@@ -213,7 +213,7 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
 	public void addingGroupsInLdapShouldNotUpdateGitBlitUsersAndGroups() throws Exception {
 		settings.put("realm.ldap.ldapCachePeriod", "0 MINUTES");
 		ds.addEntries(LDIFReader.readEntries(RESOURCE_DIR + "addgroup.ldif"));
-		ldap.synchronizeWithLdapService();
+		ldap.sync();
 		assertEquals("Number of ldap groups in gitblit team model", 0, countLdapTeamsInUserManager());
 	}
 
@@ -222,7 +222,7 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
 		settings.put("realm.ldap.synchronizeUsers.enable", "true");
 		settings.put("realm.ldap.ldapCachePeriod", "0 MINUTES");
 		ds.addEntries(LDIFReader.readEntries(RESOURCE_DIR + "addgroup.ldif"));
-		ldap.synchronizeWithLdapService();
+		ldap.sync();
 		assertEquals("Number of ldap groups in gitblit team model", 1, countLdapTeamsInUserManager());
 	}