From de3f435db5256bd5a106462dcfc0a2ccdce95450 Mon Sep 17 00:00:00 2001 From: Tom Date: Wed, 26 Oct 2016 21:37:19 +0200 Subject: Issue #1076: load commit cache in a background thread * Make the CommitCache fully thread-safe. It was using a ConcurrentHashMap containing lists, but then handed out these lists. It also did multiple operations on that map that as a whole should be atomic. * Use isEmpty() instead of size() == 0. * Run the loading of the commit cache in a background daemon thread --- .../com/gitblit/manager/RepositoryManager.java | 62 ++++++----- src/main/java/com/gitblit/utils/ArrayUtils.java | 2 +- src/main/java/com/gitblit/utils/CommitCache.java | 117 ++++++++++++--------- 3 files changed, 103 insertions(+), 78 deletions(-) diff --git a/src/main/java/com/gitblit/manager/RepositoryManager.java b/src/main/java/com/gitblit/manager/RepositoryManager.java index e9bf5b84..ed26c775 100644 --- a/src/main/java/com/gitblit/manager/RepositoryManager.java +++ b/src/main/java/com/gitblit/manager/RepositoryManager.java @@ -1952,39 +1952,47 @@ public class RepositoryManager implements IRepositoryManager { } protected void configureCommitCache() { - int daysToCache = settings.getInteger(Keys.web.activityCacheDays, 14); + final int daysToCache = settings.getInteger(Keys.web.activityCacheDays, 14); if (daysToCache <= 0) { logger.info("Commit cache is disabled"); - } else { - long start = System.nanoTime(); - long repoCount = 0; - long commitCount = 0; - logger.info(MessageFormat.format("Preparing {0} day commit cache. please wait...", daysToCache)); - CommitCache.instance().setCacheDays(daysToCache); - Date cutoff = CommitCache.instance().getCutoffDate(); - for (String repositoryName : getRepositoryList()) { - RepositoryModel model = getRepositoryModel(repositoryName); - if (model != null && model.hasCommits && model.lastChange.after(cutoff)) { - repoCount++; - Repository repository = getRepository(repositoryName); - for (RefModel ref : JGitUtils.getLocalBranches(repository, true, -1)) { - if (!ref.getDate().after(cutoff)) { - // branch not recently updated - continue; - } - List commits = CommitCache.instance().getCommits(repositoryName, repository, ref.getName()); - if (commits.size() > 0) { - logger.info(MessageFormat.format(" cached {0} commits for {1}:{2}", - commits.size(), repositoryName, ref.getName())); - commitCount += commits.size(); + return; + } + logger.info(MessageFormat.format("Preparing {0} day commit cache...", daysToCache)); + CommitCache.instance().setCacheDays(daysToCache); + Thread loader = new Thread() { + @Override + public void run() { + long start = System.nanoTime(); + long repoCount = 0; + long commitCount = 0; + Date cutoff = CommitCache.instance().getCutoffDate(); + for (String repositoryName : getRepositoryList()) { + RepositoryModel model = getRepositoryModel(repositoryName); + if (model != null && model.hasCommits && model.lastChange.after(cutoff)) { + repoCount++; + Repository repository = getRepository(repositoryName); + for (RefModel ref : JGitUtils.getLocalBranches(repository, true, -1)) { + if (!ref.getDate().after(cutoff)) { + // branch not recently updated + continue; + } + List commits = CommitCache.instance().getCommits(repositoryName, repository, ref.getName()); + if (commits.size() > 0) { + logger.info(MessageFormat.format(" cached {0} commits for {1}:{2}", + commits.size(), repositoryName, ref.getName())); + commitCount += commits.size(); + } } + repository.close(); } - repository.close(); } + logger.info(MessageFormat.format("built {0} day commit cache of {1} commits across {2} repositories in {3} msecs", + daysToCache, commitCount, repoCount, TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start))); } - logger.info(MessageFormat.format("built {0} day commit cache of {1} commits across {2} repositories in {3} msecs", - daysToCache, commitCount, repoCount, TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start))); - } + }; + loader.setName("CommitCacheLoader"); + loader.setDaemon(true); + loader.start(); } protected void confirmWriteAccess() { diff --git a/src/main/java/com/gitblit/utils/ArrayUtils.java b/src/main/java/com/gitblit/utils/ArrayUtils.java index 1402ad5e..b850ccc9 100644 --- a/src/main/java/com/gitblit/utils/ArrayUtils.java +++ b/src/main/java/com/gitblit/utils/ArrayUtils.java @@ -42,7 +42,7 @@ public class ArrayUtils { } public static boolean isEmpty(Collection collection) { - return collection == null || collection.size() == 0; + return collection == null || collection.isEmpty(); } public static String toString(Collection collection) { diff --git a/src/main/java/com/gitblit/utils/CommitCache.java b/src/main/java/com/gitblit/utils/CommitCache.java index a3963f50..53b8de19 100644 --- a/src/main/java/com/gitblit/utils/CommitCache.java +++ b/src/main/java/com/gitblit/utils/CommitCache.java @@ -19,9 +19,9 @@ import java.text.MessageFormat; import java.util.ArrayList; import java.util.Calendar; import java.util.Date; +import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import org.eclipse.jgit.lib.ObjectId; @@ -58,7 +58,7 @@ public class CommitCache { } protected CommitCache() { - cache = new ConcurrentHashMap>>(); + cache = new HashMap<>(); } /** @@ -93,7 +93,9 @@ public class CommitCache { * */ public void clear() { - cache.clear(); + synchronized (cache) { + cache.clear(); + } } /** @@ -103,8 +105,11 @@ public class CommitCache { */ public void clear(String repositoryName) { String repoKey = repositoryName.toLowerCase(); - ObjectCache> repoCache = cache.remove(repoKey); - if (repoCache != null) { + boolean hadEntries = false; + synchronized (cache) { + hadEntries = cache.remove(repoKey) != null; + } + if (hadEntries) { logger.info(MessageFormat.format("{0} commit cache cleared", repositoryName)); } } @@ -117,13 +122,17 @@ public class CommitCache { */ public void clear(String repositoryName, String branch) { String repoKey = repositoryName.toLowerCase(); - ObjectCache> repoCache = cache.get(repoKey); - if (repoCache != null) { - List commits = repoCache.remove(branch.toLowerCase()); - if (!ArrayUtils.isEmpty(commits)) { - logger.info(MessageFormat.format("{0}:{1} commit cache cleared", repositoryName, branch)); + boolean hadEntries = false; + synchronized (cache) { + ObjectCache> repoCache = cache.get(repoKey); + if (repoCache != null) { + List commits = repoCache.remove(branch.toLowerCase()); + hadEntries = !ArrayUtils.isEmpty(commits); } } + if (hadEntries) { + logger.info(MessageFormat.format("{0}:{1} commit cache cleared", repositoryName, branch)); + } } /** @@ -156,49 +165,55 @@ public class CommitCache { if (cacheDays > 0 && (sinceDate.getTime() >= cacheCutoffDate.getTime())) { // request fits within the cache window String repoKey = repositoryName.toLowerCase(); - if (!cache.containsKey(repoKey)) { - cache.put(repoKey, new ObjectCache>()); - } - - ObjectCache> repoCache = cache.get(repoKey); String branchKey = branch.toLowerCase(); RevCommit tip = JGitUtils.getCommit(repository, branch); Date tipDate = JGitUtils.getCommitDate(tip); - List commits; - if (!repoCache.hasCurrent(branchKey, tipDate)) { - commits = repoCache.getObject(branchKey); - if (ArrayUtils.isEmpty(commits)) { - // we don't have any cached commits for this branch, reload - commits = get(repositoryName, repository, branch, cacheCutoffDate); - repoCache.updateObject(branchKey, tipDate, commits); - logger.debug(MessageFormat.format("parsed {0} commits from {1}:{2} since {3,date,yyyy-MM-dd} in {4} msecs", - commits.size(), repositoryName, branch, cacheCutoffDate, TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start))); - } else { - // incrementally update cache since the last cached commit - ObjectId sinceCommit = commits.get(0).getId(); - List incremental = get(repositoryName, repository, branch, sinceCommit); - logger.info(MessageFormat.format("incrementally added {0} commits to cache for {1}:{2} in {3} msecs", - incremental.size(), repositoryName, branch, TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start))); - incremental.addAll(commits); - repoCache.updateObject(branchKey, tipDate, incremental); - commits = incremental; + ObjectCache> repoCache; + synchronized (cache) { + repoCache = cache.get(repoKey); + if (repoCache == null) { + repoCache = new ObjectCache<>(); + cache.put(repoKey, repoCache); } - } else { - // cache is current - commits = repoCache.getObject(branchKey); - // evict older commits outside the cache window - commits = reduce(commits, cacheCutoffDate); - // update cache - repoCache.updateObject(branchKey, tipDate, commits); } + synchronized (repoCache) { + List commits; + if (!repoCache.hasCurrent(branchKey, tipDate)) { + commits = repoCache.getObject(branchKey); + if (ArrayUtils.isEmpty(commits)) { + // we don't have any cached commits for this branch, reload + commits = get(repositoryName, repository, branch, cacheCutoffDate); + repoCache.updateObject(branchKey, tipDate, commits); + logger.debug(MessageFormat.format("parsed {0} commits from {1}:{2} since {3,date,yyyy-MM-dd} in {4} msecs", + commits.size(), repositoryName, branch, cacheCutoffDate, TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start))); + } else { + // incrementally update cache since the last cached commit + ObjectId sinceCommit = commits.get(0).getId(); + List incremental = get(repositoryName, repository, branch, sinceCommit); + logger.info(MessageFormat.format("incrementally added {0} commits to cache for {1}:{2} in {3} msecs", + incremental.size(), repositoryName, branch, TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start))); + incremental.addAll(commits); + repoCache.updateObject(branchKey, tipDate, incremental); + commits = incremental; + } + } else { + // cache is current + commits = repoCache.getObject(branchKey); + // evict older commits outside the cache window + commits = reduce(commits, cacheCutoffDate); + // update cache + repoCache.updateObject(branchKey, tipDate, commits); + } - if (sinceDate.equals(cacheCutoffDate)) { - list = commits; - } else { - // reduce the commits to those since the specified date - list = reduce(commits, sinceDate); + if (sinceDate.equals(cacheCutoffDate)) { + // Mustn't hand out the cached list; that's not thread-safe + list = new ArrayList<>(commits); + } else { + // reduce the commits to those since the specified date + list = reduce(commits, sinceDate); + } } logger.debug(MessageFormat.format("retrieved {0} commits from cache of {1}:{2} since {3,date,yyyy-MM-dd} in {4} msecs", list.size(), repositoryName, branch, sinceDate, TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start))); @@ -222,8 +237,9 @@ public class CommitCache { */ protected List get(String repositoryName, Repository repository, String branch, Date sinceDate) { Map> allRefs = JGitUtils.getAllRefs(repository, false); - List commits = new ArrayList(); - for (RevCommit commit : JGitUtils.getRevLog(repository, branch, sinceDate)) { + List revLog = JGitUtils.getRevLog(repository, branch, sinceDate); + List commits = new ArrayList(revLog.size()); + for (RevCommit commit : revLog) { RepositoryCommit commitModel = new RepositoryCommit(repositoryName, branch, commit); List commitRefs = allRefs.get(commitModel.getId()); commitModel.setRefs(commitRefs); @@ -243,8 +259,9 @@ public class CommitCache { */ protected List get(String repositoryName, Repository repository, String branch, ObjectId sinceCommit) { Map> allRefs = JGitUtils.getAllRefs(repository, false); - List commits = new ArrayList(); - for (RevCommit commit : JGitUtils.getRevLog(repository, sinceCommit.getName(), branch)) { + List revLog = JGitUtils.getRevLog(repository, sinceCommit.getName(), branch); + List commits = new ArrayList(revLog.size()); + for (RevCommit commit : revLog) { RepositoryCommit commitModel = new RepositoryCommit(repositoryName, branch, commit); List commitRefs = allRefs.get(commitModel.getId()); commitModel.setRefs(commitRefs); @@ -261,7 +278,7 @@ public class CommitCache { * @return a list of commits */ protected List reduce(List commits, Date sinceDate) { - List filtered = new ArrayList(); + List filtered = new ArrayList(commits.size()); for (RepositoryCommit commit : commits) { if (commit.getCommitDate().compareTo(sinceDate) >= 0) { filtered.add(commit); -- cgit v1.2.3 From 4218f97ed40240c28d57ae8cb4f9e521d8a65ad3 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Tue, 18 Nov 2014 23:53:33 +0100 Subject: Improve logging when sending email fails. --- src/main/java/com/gitblit/service/MailService.java | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/gitblit/service/MailService.java b/src/main/java/com/gitblit/service/MailService.java index ec3a84ca..58acc9c0 100644 --- a/src/main/java/com/gitblit/service/MailService.java +++ b/src/main/java/com/gitblit/service/MailService.java @@ -17,6 +17,7 @@ package com.gitblit.service; import java.io.File; import java.util.ArrayList; +import java.util.Arrays; import java.util.Date; import java.util.List; import java.util.Properties; @@ -31,6 +32,7 @@ import javax.mail.Authenticator; import javax.mail.Message; import javax.mail.MessagingException; import javax.mail.PasswordAuthentication; +import javax.mail.SendFailedException; import javax.mail.Session; import javax.mail.Transport; import javax.mail.internet.InternetAddress; @@ -272,9 +274,22 @@ public class MailService implements Runnable { while ((message = queue.poll()) != null) { try { if (settings.getBoolean(Keys.mail.debug, false)) { - logger.info("send: " + StringUtils.trimString(message.getSubject(), 60)); + logger.info("send: '" + StringUtils.trimString(message.getSubject(), 60) + + "' to:" + StringUtils.trimString(Arrays.toString(message.getAllRecipients()), 300)); } Transport.send(message); + } catch (SendFailedException sfe) { + if (settings.getBoolean(Keys.mail.debug, false)) { + logger.error("Failed to send message: {}", sfe.getMessage()); + logger.info(" Invalid addresses: {}", Arrays.toString(sfe.getInvalidAddresses())); + logger.info(" Valid sent addresses: {}", Arrays.toString(sfe.getValidSentAddresses())); + logger.info(" Valid unset addresses: {}", Arrays.toString(sfe.getValidUnsentAddresses())); + logger.info("", sfe); + } + else { + logger.error("Failed to send message: {}", sfe.getMessage(), sfe.getNextException()); + } + failures.add(message); } catch (Throwable e) { logger.error("Failed to send message", e); failures.add(message); -- cgit v1.2.3 From 5c44219084e825feea72c60a83bc6889e3cf1bc9 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Sat, 21 Jun 2014 02:53:21 +0200 Subject: Add integration strategy to merge tickes fast-forward or with commit. Add the option to merge a ticket branch to the integration branch only when it can be fast-forwarded, or always with a merge commit, or by fast-foward if possible, otherwise with a merge commit. Adds a new property ticket.mergeType with the valid values FAST_FOWARD_ONLY, MERGE_ALWAYS and MERGE_IF_NECESSARY. Merging and canMerge were refactored to make use of a new IntegrationStrategy class for each type of strategy. --- src/main/distrib/data/defaults.properties | 15 + src/main/java/com/gitblit/Constants.java | 31 ++ .../java/com/gitblit/git/PatchsetReceivePack.java | 3 +- .../com/gitblit/manager/RepositoryManager.java | 9 + .../java/com/gitblit/models/RepositoryModel.java | 3 + src/main/java/com/gitblit/utils/JGitUtils.java | 343 +++++++++++++++++---- .../java/com/gitblit/wicket/pages/TicketPage.java | 6 +- 7 files changed, 341 insertions(+), 69 deletions(-) diff --git a/src/main/distrib/data/defaults.properties b/src/main/distrib/data/defaults.properties index 0c7d6cd4..208fd992 100644 --- a/src/main/distrib/data/defaults.properties +++ b/src/main/distrib/data/defaults.properties @@ -567,6 +567,21 @@ tickets.acceptNewPatchsets = true # SINCE 1.4.0 tickets.requireApproval = false +# Default setting to control how patchsets are merged to the integration branch. +# Valid values: +# MERGE_ALWAYS - Always merge with a merge commit. Every ticket will show up as a branch, +# even if it could have been fast-forward merged. This is the default. +# MERGE_IF_NECESSARY - If possible, fast-forward the integration branch, +# if not, merge with a merge commit. +# FAST_FORWARD_ONLY - Only merge when a fast-forward is possible. This produces a strictly +# linear history of the integration branch. +# +# This setting can be overriden per-repository. +# +# RESTART REQUIRED +# SINCE 1.9.0 +tickets.mergeType = MERGE_ALWAYS + # The case-insensitive regular expression used to identify and close tickets on # push to the integration branch for commits that are NOT already referenced as # a patchset tip. diff --git a/src/main/java/com/gitblit/Constants.java b/src/main/java/com/gitblit/Constants.java index 6232552e..321f84f9 100644 --- a/src/main/java/com/gitblit/Constants.java +++ b/src/main/java/com/gitblit/Constants.java @@ -639,6 +639,37 @@ public class Constants { } } + /** + * The type of merge Gitblit will use when merging a ticket to the integration branch. + *

+ * The default type is MERGE_ALWAYS. + *

+ * This is modeled after the Gerrit SubmitType. + */ + public static enum MergeType { + /** Allows a merge only if it can be fast-forward merged into the integration branch. */ + FAST_FORWARD_ONLY, + /** Uses a fast-forward merge if possible, other wise a merge commit is created. */ + MERGE_IF_NECESSARY, + // Future REBASE_IF_NECESSARY, + /** Always merge with a merge commit, even when a fast-forward would be possible. */ + MERGE_ALWAYS, + // Future? CHERRY_PICK + ; + + public static final MergeType DEFAULT_MERGE_TYPE = MERGE_ALWAYS; + + public static MergeType fromName(String name) { + for (MergeType type : values()) { + if (type.name().equalsIgnoreCase(name)) { + return type; + } + } + return DEFAULT_MERGE_TYPE; + } + } + + @Documented @Retention(RetentionPolicy.RUNTIME) public @interface Unused { diff --git a/src/main/java/com/gitblit/git/PatchsetReceivePack.java b/src/main/java/com/gitblit/git/PatchsetReceivePack.java index 33fa4705..4a09139a 100644 --- a/src/main/java/com/gitblit/git/PatchsetReceivePack.java +++ b/src/main/java/com/gitblit/git/PatchsetReceivePack.java @@ -599,7 +599,7 @@ public class PatchsetReceivePack extends GitblitReceivePack { } // ensure that the patchset can be cleanly merged right now - MergeStatus status = JGitUtils.canMerge(getRepository(), tipCommit.getName(), forBranch); + MergeStatus status = JGitUtils.canMerge(getRepository(), tipCommit.getName(), forBranch, repository.mergeType); switch (status) { case ALREADY_MERGED: sendError(""); @@ -1279,6 +1279,7 @@ public class PatchsetReceivePack extends GitblitReceivePack { getRepository(), patchset.tip, ticket.mergeTo, + getRepositoryModel().mergeType, committer, message); diff --git a/src/main/java/com/gitblit/manager/RepositoryManager.java b/src/main/java/com/gitblit/manager/RepositoryManager.java index e9bf5b84..baccfcfa 100644 --- a/src/main/java/com/gitblit/manager/RepositoryManager.java +++ b/src/main/java/com/gitblit/manager/RepositoryManager.java @@ -63,6 +63,7 @@ import com.gitblit.Constants.AccessRestrictionType; import com.gitblit.Constants.AuthorizationControl; import com.gitblit.Constants.CommitMessageRenderer; import com.gitblit.Constants.FederationStrategy; +import com.gitblit.Constants.MergeType; import com.gitblit.Constants.PermissionType; import com.gitblit.Constants.RegistrantType; import com.gitblit.GitBlitException; @@ -899,6 +900,7 @@ public class RepositoryManager implements IRepositoryManager { model.acceptNewTickets = getConfig(config, "acceptNewTickets", true); model.requireApproval = getConfig(config, "requireApproval", settings.getBoolean(Keys.tickets.requireApproval, false)); model.mergeTo = getConfig(config, "mergeTo", null); + model.mergeType = MergeType.fromName(getConfig(config, "mergeType", settings.getString(Keys.tickets.mergeType, null))); model.useIncrementalPushTags = getConfig(config, "useIncrementalPushTags", false); model.incrementalPushTagPrefix = getConfig(config, "incrementalPushTagPrefix", null); model.allowForks = getConfig(config, "allowForks", true); @@ -1557,6 +1559,13 @@ public class RepositoryManager implements IRepositoryManager { if (!StringUtils.isEmpty(repository.mergeTo)) { config.setString(Constants.CONFIG_GITBLIT, null, "mergeTo", repository.mergeTo); } + if (repository.mergeType == null || repository.mergeType == MergeType.fromName(settings.getString(Keys.tickets.mergeType, null))) { + // use default + config.unset(Constants.CONFIG_GITBLIT, null, "mergeType"); + } else { + // override default + config.setString(Constants.CONFIG_GITBLIT, null, "mergeType", repository.mergeType.name()); + } config.setBoolean(Constants.CONFIG_GITBLIT, null, "useIncrementalPushTags", repository.useIncrementalPushTags); if (StringUtils.isEmpty(repository.incrementalPushTagPrefix) || repository.incrementalPushTagPrefix.equals(settings.getString(Keys.git.defaultIncrementalPushTagPrefix, "r"))) { diff --git a/src/main/java/com/gitblit/models/RepositoryModel.java b/src/main/java/com/gitblit/models/RepositoryModel.java index a81c622a..67ee1c7e 100644 --- a/src/main/java/com/gitblit/models/RepositoryModel.java +++ b/src/main/java/com/gitblit/models/RepositoryModel.java @@ -28,6 +28,7 @@ import com.gitblit.Constants.AccessRestrictionType; import com.gitblit.Constants.AuthorizationControl; import com.gitblit.Constants.CommitMessageRenderer; import com.gitblit.Constants.FederationStrategy; +import com.gitblit.Constants.MergeType; import com.gitblit.utils.ArrayUtils; import com.gitblit.utils.ModelUtils; import com.gitblit.utils.StringUtils; @@ -89,6 +90,7 @@ public class RepositoryModel implements Serializable, Comparable Date: Mon, 23 Jun 2014 04:12:19 +0200 Subject: Add merge type setting to repository page. The merge type is a per repository setting. Add it to the edit page. --- src/main/java/com/gitblit/wicket/GitBlitWebApp.properties | 2 ++ src/main/java/com/gitblit/wicket/pages/EditRepositoryPage.html | 3 ++- src/main/java/com/gitblit/wicket/pages/EditRepositoryPage.java | 6 ++++++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/gitblit/wicket/GitBlitWebApp.properties b/src/main/java/com/gitblit/wicket/GitBlitWebApp.properties index a215b4d6..b3cbef82 100644 --- a/src/main/java/com/gitblit/wicket/GitBlitWebApp.properties +++ b/src/main/java/com/gitblit/wicket/GitBlitWebApp.properties @@ -660,6 +660,7 @@ gb.nTotalTickets = {0} total gb.body = body gb.mergeSha = mergeSha gb.mergeTo = merge to +gb.mergeType = merge type gb.labels = labels gb.reviewers = reviewers gb.voters = voters @@ -671,6 +672,7 @@ gb.repositoryDoesNotAcceptPatchsets = This repository does not accept patchsets. gb.serverDoesNotAcceptPatchsets = This server does not accept patchsets. gb.ticketIsClosed = This ticket is closed. gb.mergeToDescription = default integration branch for merging ticket patchsets +gb.mergeTypeDescription = merge a ticket fast-forward only, if necessary, or always with a merge commit to the integration branch gb.anonymousCanNotPropose = Anonymous users can not propose patchsets. gb.youDoNotHaveClonePermission = You are not permitted to clone this repository. gb.myTickets = my tickets diff --git a/src/main/java/com/gitblit/wicket/pages/EditRepositoryPage.html b/src/main/java/com/gitblit/wicket/pages/EditRepositoryPage.html index 7a55b9f5..2c881efc 100644 --- a/src/main/java/com/gitblit/wicket/pages/EditRepositoryPage.html +++ b/src/main/java/com/gitblit/wicket/pages/EditRepositoryPage.html @@ -123,7 +123,8 @@

- +
+ diff --git a/src/main/java/com/gitblit/wicket/pages/EditRepositoryPage.java b/src/main/java/com/gitblit/wicket/pages/EditRepositoryPage.java index 6bcf6f51..bf3eea8b 100644 --- a/src/main/java/com/gitblit/wicket/pages/EditRepositoryPage.java +++ b/src/main/java/com/gitblit/wicket/pages/EditRepositoryPage.java @@ -56,6 +56,7 @@ import com.gitblit.Constants.AccessRestrictionType; import com.gitblit.Constants.AuthorizationControl; import com.gitblit.Constants.CommitMessageRenderer; import com.gitblit.Constants.FederationStrategy; +import com.gitblit.Constants.MergeType; import com.gitblit.Constants.RegistrantType; import com.gitblit.GitBlitException; import com.gitblit.Keys; @@ -458,6 +459,11 @@ public class EditRepositoryPage extends RootSubPage { getString("gb.mergeToDescription"), new PropertyModel(repositoryModel, "mergeTo"), availableBranches)); + form.add(new ChoiceOption("mergeType", + getString("gb.mergeType"), + getString("gb.mergeTypeDescription"), + new PropertyModel(repositoryModel, "mergeType"), + Arrays.asList(MergeType.values()))); // // RECEIVE -- cgit v1.2.3 From d4a1aae48f097d48e9fded67445ba20720c1d966 Mon Sep 17 00:00:00 2001 From: Tom Date: Mon, 31 Oct 2016 07:50:26 +0100 Subject: Fix disabled links in PagerPanel Disabled links in the PagerPanel (used on the LuceneSearchPage to page through search results) were only rendered as "disabled". The links themselves remained active, which gives strange effects when clicked. For instance it was possible to move to result pages -1, -2, and so on. Really disable the links. Add missing CSS rules to have correct styling as Wicket renders disabled links as spans, not anchors. Include the new CSS file in BasePage.html. And add the left/right arrows only if not on the first/last page. --- .../java/com/gitblit/wicket/pages/BasePage.html | 1 + .../java/com/gitblit/wicket/panels/PagerPanel.java | 5 +++-- src/main/resources/bootstrap-fixes.css | 25 ++++++++++++++++++++++ 3 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 src/main/resources/bootstrap-fixes.css diff --git a/src/main/java/com/gitblit/wicket/pages/BasePage.html b/src/main/java/com/gitblit/wicket/pages/BasePage.html index b998428c..4dbc2e57 100644 --- a/src/main/java/com/gitblit/wicket/pages/BasePage.html +++ b/src/main/java/com/gitblit/wicket/pages/BasePage.html @@ -17,6 +17,7 @@ + diff --git a/src/main/java/com/gitblit/wicket/panels/PagerPanel.java b/src/main/java/com/gitblit/wicket/panels/PagerPanel.java index 2d774c41..d1214cae 100644 --- a/src/main/java/com/gitblit/wicket/panels/PagerPanel.java +++ b/src/main/java/com/gitblit/wicket/panels/PagerPanel.java @@ -48,7 +48,7 @@ public class PagerPanel extends Panel { deltas = new int[] { -2, -1, 0, 1, 2 }; } - if (totalPages > 0) { + if (totalPages > 0 && currentPage > 1) { pages.add(new PageObject("\u2190", currentPage - 1)); } for (int delta : deltas) { @@ -57,7 +57,7 @@ public class PagerPanel extends Panel { pages.add(new PageObject("" + page, page)); } } - if (totalPages > 0) { + if (totalPages > 0 && currentPage < totalPages) { pages.add(new PageObject("\u2192", currentPage + 1)); } @@ -75,6 +75,7 @@ public class PagerPanel extends Panel { item.add(link); if (pageItem.page == currentPage || pageItem.page < 1 || pageItem.page > totalPages) { WicketUtils.setCssClass(item, "disabled"); + link.setEnabled(false); } } }; diff --git a/src/main/resources/bootstrap-fixes.css b/src/main/resources/bootstrap-fixes.css new file mode 100644 index 00000000..c9b6154b --- /dev/null +++ b/src/main/resources/bootstrap-fixes.css @@ -0,0 +1,25 @@ +/** + * Disabled links in a PagerPanel. Bootstrap 2.0.4 only handles , but not . Wicket renders disabled links as spans. + * The .pagination rules here are identical to the ones for in bootstrap.css, but for . + */ +.pagination span { + float: left; + padding: 0 14px; + line-height: 34px; + text-decoration: none; + border: 1px solid #ddd; + border-left-width: 0; +} + +.pagination li:first-child span { + border-left-width: 1px; + -webkit-border-radius: 3px 0 0 3px; + -moz-border-radius: 3px 0 0 3px; + border-radius: 3px 0 0 3px; +} + +.pagination li:last-child span { + -webkit-border-radius: 0 3px 3px 0; + -moz-border-radius: 0 3px 3px 0; + border-radius: 0 3px 3px 0; +} -- cgit v1.2.3 From 0d1222739683b9392a1a6c96a95552333f7c2246 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Sun, 6 Nov 2016 18:09:32 +0100 Subject: Extend LDAP authentication tests to use different modes. Instantiate two LDAP servers, one that allows anonymous access, and one that requires authentication for all operations. The JUnit test is parameterized to run all tests with both instances. It uses different settings for each mode. --- .../com/gitblit/tests/LdapAuthenticationTest.java | 130 +++++++++++++++++---- 1 file changed, 107 insertions(+), 23 deletions(-) diff --git a/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java b/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java index 84dd138d..cea8a4b1 100644 --- a/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java +++ b/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java @@ -17,7 +17,9 @@ package com.gitblit.tests; import java.io.File; -import java.io.FileInputStream; +import java.util.Arrays; +import java.util.Collection; +import java.util.EnumSet; import java.util.HashMap; import java.util.Map; @@ -27,6 +29,10 @@ import org.junit.BeforeClass; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; import com.gitblit.Constants.AccountType; import com.gitblit.IStoredSettings; @@ -43,7 +49,9 @@ import com.gitblit.utils.XssFilter; import com.gitblit.utils.XssFilter.AllowXssFilter; import com.unboundid.ldap.listener.InMemoryDirectoryServer; import com.unboundid.ldap.listener.InMemoryDirectoryServerConfig; +import com.unboundid.ldap.listener.InMemoryDirectoryServerSnapshot; import com.unboundid.ldap.listener.InMemoryListenerConfig; +import com.unboundid.ldap.sdk.OperationType; import com.unboundid.ldap.sdk.SearchResult; import com.unboundid.ldap.sdk.SearchScope; import com.unboundid.ldif.LDIFReader; @@ -55,19 +63,31 @@ import com.unboundid.ldif.LDIFReader; * @author jcrygier * */ +@RunWith(Parameterized.class) public class LdapAuthenticationTest extends GitblitUnitTest { - @Rule - public TemporaryFolder folder = new TemporaryFolder(); + + public enum ServerMode { ANONYMOUS, AUTHENTICATED }; + + @Rule + public TemporaryFolder folder = new TemporaryFolder(); private static final String RESOURCE_DIR = "src/test/resources/ldap/"; - private File usersConf; + @Parameter + public ServerMode serverMode; + + private File usersConf; - private LdapAuthProvider ldap; + private LdapAuthProvider ldap; - static int ldapPort = 1389; + private static int ldapPort = 1389; + private static int ldapAuthedPort = 2389; private static InMemoryDirectoryServer ds; + private static InMemoryDirectoryServerSnapshot dsAnonSnapshot; + + private static InMemoryDirectoryServer dsAuthed; + private static InMemoryDirectoryServerSnapshot dsAuthedSnapshot; private IUserManager userManager; @@ -75,21 +95,54 @@ public class LdapAuthenticationTest extends GitblitUnitTest { private MemorySettings settings; + + + @Parameters(name = "{0}") + public static Collection data() { + return Arrays.asList(new Object[][] { {ServerMode.ANONYMOUS}, {ServerMode.AUTHENTICATED} }); + } + + + @BeforeClass - public static void createInMemoryLdapServer() throws Exception { + public static void init() throws Exception { + InMemoryDirectoryServerConfig config = createInMemoryLdapServerConfig(); + config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", ldapPort)); + ds = createInMemoryLdapServer(config); + dsAnonSnapshot = ds.createSnapshot(); + + + config = createInMemoryLdapServerConfig(); + config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", ldapAuthedPort)); + config.setAuthenticationRequiredOperationTypes(EnumSet.allOf(OperationType.class)); + dsAuthed = createInMemoryLdapServer(config); + dsAuthedSnapshot = ds.createSnapshot(); + + } + + public static InMemoryDirectoryServer createInMemoryLdapServer(InMemoryDirectoryServerConfig config) throws Exception { + InMemoryDirectoryServer imds = new InMemoryDirectoryServer(config); + imds.importFromLDIF(true, RESOURCE_DIR + "sampledata.ldif"); + imds.startListening(); + return imds; + } + + public static InMemoryDirectoryServerConfig createInMemoryLdapServerConfig() throws Exception { InMemoryDirectoryServerConfig config = new InMemoryDirectoryServerConfig("dc=MyDomain"); config.addAdditionalBindCredentials("cn=Directory Manager", "password"); - config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", ldapPort)); config.setSchema(null); - - ds = new InMemoryDirectoryServer(config); - ds.startListening(); + return config; } + + @Before - public void init() throws Exception { - ds.clear(); - ds.importFromLDIF(true, new LDIFReader(new FileInputStream(RESOURCE_DIR + "sampledata.ldif"))); + public void setup() throws Exception { + ds.restoreSnapshot(dsAnonSnapshot); + dsAuthed.restoreSnapshot(dsAuthedSnapshot); + + System.out.println("Before with server mode " + serverMode); + usersConf = folder.newFile("users.conf"); FileUtils.copyFile(new File(RESOURCE_DIR + "users.conf"), usersConf); settings = getSettings(); @@ -117,11 +170,15 @@ public class LdapAuthenticationTest extends GitblitUnitTest { private MemorySettings getSettings() { Map backingMap = new HashMap(); backingMap.put(Keys.realm.userService, usersConf.getAbsolutePath()); - backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + ldapPort); -// backingMap.put(Keys.realm.ldap.domain, ""); - backingMap.put(Keys.realm.ldap.username, "cn=Directory Manager"); - backingMap.put(Keys.realm.ldap.password, "password"); -// backingMap.put(Keys.realm.ldap.backingUserService, "users.conf"); + if (ServerMode.ANONYMOUS == serverMode) { + backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + ldapPort); + backingMap.put(Keys.realm.ldap.username, ""); + backingMap.put(Keys.realm.ldap.password, ""); + } else { + backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + ldapAuthedPort); + backingMap.put(Keys.realm.ldap.username, "cn=Directory Manager"); + backingMap.put(Keys.realm.ldap.password, "password"); + } backingMap.put(Keys.realm.ldap.maintainTeams, "true"); backingMap.put(Keys.realm.ldap.accountBase, "OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain"); backingMap.put(Keys.realm.ldap.accountPattern, "(&(objectClass=person)(sAMAccountName=${username}))"); @@ -136,6 +193,8 @@ public class LdapAuthenticationTest extends GitblitUnitTest { return ms; } + + @Test public void testAuthenticate() { UserModel userOneModel = ldap.authenticate("UserOne", "userOnePassword".toCharArray()); @@ -159,6 +218,13 @@ public class LdapAuthenticationTest extends GitblitUnitTest { assertNotNull(userThreeModel.getTeam("git_users")); assertNull(userThreeModel.getTeam("git_admins")); assertTrue(userThreeModel.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); } @Test @@ -210,7 +276,7 @@ public class LdapAuthenticationTest extends GitblitUnitTest { @Test public void addingUserInLdapShouldNotUpdateGitBlitUsersAndGroups() throws Exception { - ds.addEntries(LDIFReader.readEntries(RESOURCE_DIR + "adduser.ldif")); + getDS().addEntries(LDIFReader.readEntries(RESOURCE_DIR + "adduser.ldif")); ldap.sync(); assertEquals("Number of ldap users in gitblit user model", 5, countLdapUsersInUserManager()); } @@ -218,14 +284,14 @@ public class LdapAuthenticationTest extends GitblitUnitTest { @Test public void addingUserInLdapShouldUpdateGitBlitUsersAndGroups() throws Exception { settings.put(Keys.realm.ldap.synchronize, "true"); - ds.addEntries(LDIFReader.readEntries(RESOURCE_DIR + "adduser.ldif")); + getDS().addEntries(LDIFReader.readEntries(RESOURCE_DIR + "adduser.ldif")); ldap.sync(); assertEquals("Number of ldap users in gitblit user model", 6, countLdapUsersInUserManager()); } @Test public void addingGroupsInLdapShouldNotUpdateGitBlitUsersAndGroups() throws Exception { - ds.addEntries(LDIFReader.readEntries(RESOURCE_DIR + "addgroup.ldif")); + getDS().addEntries(LDIFReader.readEntries(RESOURCE_DIR + "addgroup.ldif")); ldap.sync(); assertEquals("Number of ldap groups in gitblit team model", 0, countLdapTeamsInUserManager()); } @@ -233,7 +299,7 @@ public class LdapAuthenticationTest extends GitblitUnitTest { @Test public void addingGroupsInLdapShouldUpdateGitBlitUsersAndGroups() throws Exception { settings.put(Keys.realm.ldap.synchronize, "true"); - ds.addEntries(LDIFReader.readEntries(RESOURCE_DIR + "addgroup.ldif")); + getDS().addEntries(LDIFReader.readEntries(RESOURCE_DIR + "addgroup.ldif")); ldap.sync(); assertEquals("Number of ldap groups in gitblit team model", 1, countLdapTeamsInUserManager()); } @@ -261,6 +327,13 @@ public class LdapAuthenticationTest extends GitblitUnitTest { assertNotNull(userThreeModel.getTeam("git_users")); assertNull(userThreeModel.getTeam("git_admins")); assertTrue(userThreeModel.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); } @Test @@ -276,6 +349,17 @@ public class LdapAuthenticationTest extends GitblitUnitTest { assertNull(userOneModelFailedAuth); } + + + private InMemoryDirectoryServer getDS() { + if (ServerMode.ANONYMOUS == serverMode) { + return ds; + } else { + return dsAuthed; + } + } + + private int countLdapUsersInUserManager() { int ldapAccountCount = 0; for (UserModel userModel : userManager.getAllUsers()) { -- cgit v1.2.3 From ffadc2e1878ce448127055e98299218574b103cf Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Wed, 9 Nov 2016 22:04:00 +0100 Subject: Extend LDAP tests to use LDAP servers with access restrictions. Add access restrictions to the LDAP test server instances. New modes used a test parameters are ANONYMOUS, DS_MANAGER and USR_MANAGER. ANONYMOUS can bind anonymously and access users and groups. In DS_MANAGER the server requires authentication and will only allow the DIRECTORY_MANAGER user to search for users and groups. In USR_MANAGER only the user can search groups, the USER_MANAGER, which is used to bind in this mode, can not. A third server instance is created because I did fear side effects should the tests be run in parallel, had I tried to configure the access restriction in Before. --- .../com/gitblit/tests/LdapAuthenticationTest.java | 302 ++++++++++++++++++--- 1 file changed, 257 insertions(+), 45 deletions(-) diff --git a/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java b/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java index cea8a4b1..2ade6819 100644 --- a/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java +++ b/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java @@ -16,6 +16,8 @@ */ package com.gitblit.tests; +import static org.junit.Assume.*; + import java.io.File; import java.util.Arrays; import java.util.Collection; @@ -24,6 +26,7 @@ import java.util.HashMap; import java.util.Map; import org.apache.commons.io.FileUtils; +import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Rule; @@ -51,9 +54,22 @@ import com.unboundid.ldap.listener.InMemoryDirectoryServer; import com.unboundid.ldap.listener.InMemoryDirectoryServerConfig; import com.unboundid.ldap.listener.InMemoryDirectoryServerSnapshot; import com.unboundid.ldap.listener.InMemoryListenerConfig; +import com.unboundid.ldap.listener.interceptor.InMemoryInterceptedRequest; +import com.unboundid.ldap.listener.interceptor.InMemoryInterceptedResult; +import com.unboundid.ldap.listener.interceptor.InMemoryInterceptedSearchEntry; +import com.unboundid.ldap.listener.interceptor.InMemoryInterceptedSearchRequest; +import com.unboundid.ldap.listener.interceptor.InMemoryInterceptedSearchResult; +import com.unboundid.ldap.listener.interceptor.InMemoryInterceptedSimpleBindResult; +import com.unboundid.ldap.listener.interceptor.InMemoryOperationInterceptor; +import com.unboundid.ldap.sdk.BindRequest; +import com.unboundid.ldap.sdk.BindResult; +import com.unboundid.ldap.sdk.LDAPException; +import com.unboundid.ldap.sdk.LDAPResult; import com.unboundid.ldap.sdk.OperationType; +import com.unboundid.ldap.sdk.ResultCode; import com.unboundid.ldap.sdk.SearchResult; import com.unboundid.ldap.sdk.SearchScope; +import com.unboundid.ldap.sdk.SimpleBindRequest; import com.unboundid.ldif.LDIFReader; /** @@ -66,28 +82,68 @@ import com.unboundid.ldif.LDIFReader; @RunWith(Parameterized.class) public class LdapAuthenticationTest extends GitblitUnitTest { - public enum ServerMode { ANONYMOUS, AUTHENTICATED }; + private static final String RESOURCE_DIR = "src/test/resources/ldap/"; + private static final String DIRECTORY_MANAGER = "cn=Directory Manager"; + private static final String USER_MANAGER = "cn=UserManager"; + private static final String ACCOUNT_BASE = "OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain"; + private static final String GROUP_BASE = "OU=Groups,OU=UserControl,OU=MyOrganization,DC=MyDomain"; + + + /** + * Enumeration of different test modes, representing different use scenarios. + * With ANONYMOUS anonymous binds are used to search LDAP. + * DS_MANAGER will use a DIRECTORY_MANAGER to search LDAP. Normal users are prohibited to search the DS. + * With USR_MANAGER, a USER_MANAGER account is used to search in LDAP. This account can only search users + * but not groups. Normal users can search groups, though. + * + */ + enum AuthMode { + ANONYMOUS(1389), + DS_MANAGER(2389), + USR_MANAGER(3389); + + + private int ldapPort; + private InMemoryDirectoryServer ds; + private InMemoryDirectoryServerSnapshot dsSnapshot; + + AuthMode(int port) { + this.ldapPort = port; + } + + int ldapPort() { + return this.ldapPort; + } + + void setDS(InMemoryDirectoryServer ds) { + if (this.ds == null) { + this.ds = ds; + this.dsSnapshot = ds.createSnapshot(); + }; + } + + InMemoryDirectoryServer getDS() { + return ds; + } + + void restoreSnapshot() { + ds.restoreSnapshot(dsSnapshot); + } + }; - @Rule - public TemporaryFolder folder = new TemporaryFolder(); - private static final String RESOURCE_DIR = "src/test/resources/ldap/"; @Parameter - public ServerMode serverMode; + public AuthMode authMode; - private File usersConf; + @Rule + public TemporaryFolder folder = new TemporaryFolder(); - private LdapAuthProvider ldap; + private File usersConf; - private static int ldapPort = 1389; - private static int ldapAuthedPort = 2389; - private static InMemoryDirectoryServer ds; - private static InMemoryDirectoryServerSnapshot dsAnonSnapshot; - private static InMemoryDirectoryServer dsAuthed; - private static InMemoryDirectoryServerSnapshot dsAuthedSnapshot; + private LdapAuthProvider ldap; private IUserManager userManager; @@ -96,30 +152,57 @@ public class LdapAuthenticationTest extends GitblitUnitTest { private MemorySettings settings; - + /** + * Run the tests with each authentication scenario once. + */ @Parameters(name = "{0}") public static Collection data() { - return Arrays.asList(new Object[][] { {ServerMode.ANONYMOUS}, {ServerMode.AUTHENTICATED} }); + return Arrays.asList(new Object[][] { {AuthMode.ANONYMOUS}, {AuthMode.DS_MANAGER}, {AuthMode.USR_MANAGER} }); } + /** + * Create three different in memory DS. + * + * Each DS has a different configuration: + * The first allows anonymous binds. + * The second requires authentication for all operations. It will only allow the DIRECTORY_MANAGER account + * to search for users and groups. + * The third one is like the second, but it allows users to search for users and groups, and restricts the + * USER_MANAGER from searching for groups. + */ @BeforeClass public static void init() throws Exception { - InMemoryDirectoryServerConfig config = createInMemoryLdapServerConfig(); - config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", ldapPort)); + InMemoryDirectoryServer ds; + InMemoryDirectoryServerConfig config = createInMemoryLdapServerConfig(AuthMode.ANONYMOUS); + config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", AuthMode.ANONYMOUS.ldapPort())); + ds = createInMemoryLdapServer(config); + AuthMode.ANONYMOUS.setDS(ds); + + + config = createInMemoryLdapServerConfig(AuthMode.DS_MANAGER); + config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", AuthMode.DS_MANAGER.ldapPort())); + config.setAuthenticationRequiredOperationTypes(EnumSet.allOf(OperationType.class)); ds = createInMemoryLdapServer(config); - dsAnonSnapshot = ds.createSnapshot(); + AuthMode.DS_MANAGER.setDS(ds); - config = createInMemoryLdapServerConfig(); - config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", ldapAuthedPort)); + config = createInMemoryLdapServerConfig(AuthMode.USR_MANAGER); + config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", AuthMode.USR_MANAGER.ldapPort())); config.setAuthenticationRequiredOperationTypes(EnumSet.allOf(OperationType.class)); - dsAuthed = createInMemoryLdapServer(config); - dsAuthedSnapshot = ds.createSnapshot(); + ds = createInMemoryLdapServer(config); + AuthMode.USR_MANAGER.setDS(ds); } + @AfterClass + public static void destroy() throws Exception { + for (AuthMode am : AuthMode.values()) { + am.getDS().shutDown(true); + } + } + public static InMemoryDirectoryServer createInMemoryLdapServer(InMemoryDirectoryServerConfig config) throws Exception { InMemoryDirectoryServer imds = new InMemoryDirectoryServer(config); imds.importFromLDIF(true, RESOURCE_DIR + "sampledata.ldif"); @@ -127,10 +210,14 @@ public class LdapAuthenticationTest extends GitblitUnitTest { return imds; } - public static InMemoryDirectoryServerConfig createInMemoryLdapServerConfig() throws Exception { + public static InMemoryDirectoryServerConfig createInMemoryLdapServerConfig(AuthMode authMode) throws Exception { InMemoryDirectoryServerConfig config = new InMemoryDirectoryServerConfig("dc=MyDomain"); - config.addAdditionalBindCredentials("cn=Directory Manager", "password"); + config.addAdditionalBindCredentials(DIRECTORY_MANAGER, "password"); + config.addAdditionalBindCredentials(USER_MANAGER, "passwd"); config.setSchema(null); + + config.addInMemoryOperationInterceptor(new AccessInterceptor(authMode)); + return config; } @@ -138,10 +225,7 @@ public class LdapAuthenticationTest extends GitblitUnitTest { @Before public void setup() throws Exception { - ds.restoreSnapshot(dsAnonSnapshot); - dsAuthed.restoreSnapshot(dsAuthedSnapshot); - - System.out.println("Before with server mode " + serverMode); + authMode.restoreSnapshot(); usersConf = folder.newFile("users.conf"); FileUtils.copyFile(new File(RESOURCE_DIR + "users.conf"), usersConf); @@ -170,19 +254,30 @@ public class LdapAuthenticationTest extends GitblitUnitTest { private MemorySettings getSettings() { Map backingMap = new HashMap(); backingMap.put(Keys.realm.userService, usersConf.getAbsolutePath()); - if (ServerMode.ANONYMOUS == serverMode) { - backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + ldapPort); + switch(authMode) { + case ANONYMOUS: + backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + authMode.ldapPort()); backingMap.put(Keys.realm.ldap.username, ""); backingMap.put(Keys.realm.ldap.password, ""); - } else { - backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + ldapAuthedPort); - backingMap.put(Keys.realm.ldap.username, "cn=Directory Manager"); + break; + case DS_MANAGER: + backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + authMode.ldapPort()); + backingMap.put(Keys.realm.ldap.username, DIRECTORY_MANAGER); backingMap.put(Keys.realm.ldap.password, "password"); + break; + case USR_MANAGER: + backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + authMode.ldapPort()); + backingMap.put(Keys.realm.ldap.username, USER_MANAGER); + backingMap.put(Keys.realm.ldap.password, "passwd"); + break; + default: + throw new RuntimeException("Unimplemented AuthMode case!"); + } backingMap.put(Keys.realm.ldap.maintainTeams, "true"); - backingMap.put(Keys.realm.ldap.accountBase, "OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain"); + backingMap.put(Keys.realm.ldap.accountBase, ACCOUNT_BASE); backingMap.put(Keys.realm.ldap.accountPattern, "(&(objectClass=person)(sAMAccountName=${username}))"); - backingMap.put(Keys.realm.ldap.groupBase, "OU=Groups,OU=UserControl,OU=MyOrganization,DC=MyDomain"); + backingMap.put(Keys.realm.ldap.groupBase, GROUP_BASE); backingMap.put(Keys.realm.ldap.groupMemberPattern, "(&(objectClass=group)(member=${dn}))"); backingMap.put(Keys.realm.ldap.admins, "UserThree @Git_Admins \"@Git Admins\""); backingMap.put(Keys.realm.ldap.displayName, "displayName"); @@ -270,7 +365,7 @@ public class LdapAuthenticationTest extends GitblitUnitTest { @Test public void checkIfUsersConfContainsAllUsersFromSampleDataLdif() throws Exception { - SearchResult searchResult = ds.search("OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain", SearchScope.SUB, "objectClass=person"); + SearchResult searchResult = getDS().search(ACCOUNT_BASE, SearchScope.SUB, "objectClass=person"); assertEquals("Number of ldap users in gitblit user model", searchResult.getEntryCount(), countLdapUsersInUserManager()); } @@ -298,6 +393,9 @@ public class LdapAuthenticationTest extends GitblitUnitTest { @Test public void addingGroupsInLdapShouldUpdateGitBlitUsersAndGroups() 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"); getDS().addEntries(LDIFReader.readEntries(RESOURCE_DIR + "addgroup.ldif")); ldap.sync(); @@ -338,7 +436,10 @@ public class LdapAuthenticationTest extends GitblitUnitTest { @Test public void testBindWithUser() { - settings.put(Keys.realm.ldap.bindpattern, "CN=${username},OU=US,OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain"); + // This test only makes sense if the user is not prevented from reading users and teams. + assumeTrue(authMode != AuthMode.DS_MANAGER); + + settings.put(Keys.realm.ldap.bindpattern, "CN=${username},OU=US," + ACCOUNT_BASE); settings.put(Keys.realm.ldap.username, ""); settings.put(Keys.realm.ldap.password, ""); @@ -350,16 +451,11 @@ public class LdapAuthenticationTest extends GitblitUnitTest { } - - private InMemoryDirectoryServer getDS() { - if (ServerMode.ANONYMOUS == serverMode) { - return ds; - } else { - return dsAuthed; - } + private InMemoryDirectoryServer getDS() + { + return authMode.getDS(); } - private int countLdapUsersInUserManager() { int ldapAccountCount = 0; for (UserModel userModel : userManager.getAllUsers()) { @@ -380,4 +476,120 @@ public class LdapAuthenticationTest extends GitblitUnitTest { return ldapAccountCount; } + + + + /** + * Operation interceptor for the in memory DS. This interceptor + * implements access restrictions for certain user/DN combinations. + * + * The USER_MANAGER is only allowed to search for users, but not for groups. + * This is to test the original behaviour where the teams were searched under + * the user binding. + * When running in a DIRECTORY_MANAGER scenario, only the manager account + * is allowed to search for users and groups, while a normal user may not do so. + * This tests the scenario where a normal user cannot read teams and thus the + * manager account needs to be used for all searches. + * + */ + private static class AccessInterceptor extends InMemoryOperationInterceptor { + AuthMode authMode; + Map lastSuccessfulBindDN = new HashMap<>(); + Map resultProhibited = new HashMap<>(); + + public AccessInterceptor(AuthMode authMode) { + this.authMode = authMode; + } + + + @Override + public void processSimpleBindResult(InMemoryInterceptedSimpleBindResult bind) { + BindResult result = bind.getResult(); + if (result.getResultCode() == ResultCode.SUCCESS) { + BindRequest bindRequest = bind.getRequest(); + lastSuccessfulBindDN.put(bind.getConnectionID(), ((SimpleBindRequest)bindRequest).getBindDN()); + resultProhibited.remove(bind.getConnectionID()); + } + } + + + + @Override + public void processSearchRequest(InMemoryInterceptedSearchRequest request) throws LDAPException { + String bindDN = getLastBindDN(request); + + if (USER_MANAGER.equals(bindDN)) { + if (request.getRequest().getBaseDN().endsWith(GROUP_BASE)) { + throw new LDAPException(ResultCode.NO_SUCH_OBJECT); + } + } + else if(authMode == AuthMode.DS_MANAGER && !DIRECTORY_MANAGER.equals(bindDN)) { + throw new LDAPException(ResultCode.NO_SUCH_OBJECT); + } + } + + + @Override + public void processSearchEntry(InMemoryInterceptedSearchEntry entry) { + String bindDN = getLastBindDN(entry); + + boolean prohibited = false; + + if (USER_MANAGER.equals(bindDN)) { + if (entry.getSearchEntry().getDN().endsWith(GROUP_BASE)) { + prohibited = true; + } + } + else if(authMode == AuthMode.DS_MANAGER && !DIRECTORY_MANAGER.equals(bindDN)) { + prohibited = true; + } + + if (prohibited) { + // Found entry prohibited for bound user. Setting entry to null. + entry.setSearchEntry(null); + resultProhibited.put(entry.getConnectionID(), Boolean.TRUE); + } + } + + @Override + public void processSearchResult(InMemoryInterceptedSearchResult result) { + String bindDN = getLastBindDN(result); + + boolean prohibited = false; + + Boolean rspb = resultProhibited.get(result.getConnectionID()); + if (USER_MANAGER.equals(bindDN)) { + if (rspb != null && rspb) { + prohibited = true; + } + } + else if(authMode == AuthMode.DS_MANAGER && !DIRECTORY_MANAGER.equals(bindDN)) { + if (rspb != null && rspb) { + prohibited = true; + } + } + + if (prohibited) { + // Result prohibited for bound user. Returning error + result.setResult(new LDAPResult(result.getMessageID(), ResultCode.INSUFFICIENT_ACCESS_RIGHTS)); + resultProhibited.remove(result.getConnectionID()); + } + } + + private String getLastBindDN(InMemoryInterceptedResult result) { + String bindDN = lastSuccessfulBindDN.get(result.getConnectionID()); + if (bindDN == null) { + return "UNKNOWN"; + } + return bindDN; + } + private String getLastBindDN(InMemoryInterceptedRequest request) { + String bindDN = lastSuccessfulBindDN.get(request.getConnectionID()); + if (bindDN == null) { + return "UNKNOWN"; + } + return bindDN; + } + } + } -- cgit v1.2.3 From a4ad77f9ec3292a7a6c2fb21689d672cf5db1f20 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Fri, 11 Nov 2016 19:22:17 +0100 Subject: Clean up `LdapAuthProvider` to properly cover different LDAP search scenarios. Gitblit allows in its configuration to set a "manager" user (and password) which can be used to search for the entry of a user wanting to log in. If they are both not set, an anonymous search is attempted. In the description below, when I say "...as manager", it is either as manager or anonymous. So far the behaviour of Gitblit, with respect to binding to and searching in LDAP, has been the following when a user logs in: **bind as manager** **search for the user** _bind as the user_ _search for the teams_ I'll call this code flow A. Later an additional configuration option had been added: `realm.ldap.bindpattern`. (PR gitblit/gitblit#162) It was meant to allow for not using a manager nor anonymous binds, by searching the directory as the user logging in. This is done in code flow B: **bind as manager** _bind as user_ _search for user_ _search for teams_ Both A and B are flawed, I think. In A, it looks like a mistake to me that the binding stays with the user after authentication. The problem that this causes is, that in LDAP server configurations where normal users are not allowed to read groups, the team information cannot be retrieved. I tried but failed to understand how B is supposed to work. There will always be a bind request as either anonymous or the manager DN when the LDAP connection is created. If neither is possible, the authentication process will fail and the user cannot log in. When synchronizing users and teams from LDAP, the following code flow is exercised: F: **bind as manager** **search for users** **search for teams** This patch fixes both code flows by introducing a new flow. C: **bind as manager** **search for user** _bind as user to authenticate_ **bind as manager** **search for teams** And it changes code flow B to the following code flow D: _bind as user_ _search for user_ _search for teams_ With code flows A, C, D and F the following usage (and authentication) scenarios are covered. They are described from the view of a Gitblit administrator's intent and his LDAP setup. * Users and team should be snychronized with LDAP This means anonymous or a fixed account must be able to read users and groups. => covered by C and F As the above allows for authentication and is required for synchronisation, all the others below do not cover synchronization. * No anonymous binding allowed and no special manager binding required This means that users must be able to read user an group entries. => covered by D * The user DN needs to be searched, e.g. because they are not all under the same parent DN. This means that anonymous or a fixed account must be able to read users. -- anonymous or the "manager" account can also read groups => covered by C -- anonymous or the "manager" account cannot read groups but a user can => covered by A I therefore believe that the new code will cover all common use cases. The implementation either directly binds as the user, when `bindpattern` is not empty, or it binds anonymous or against the manger DN to search for the user DN entry. If it directly bound against the user DN, the user is already authenticated. It will then only check that the user DN it found in the search is identical to the one it is currently bound against. If it was bound against a manager DN (or anonymously) it will bind against the found user DN to authenticate the user logging in, and will then rebind against the manager DN. When searching for groups in LDAP, if the search fails with a result code other than SUCCESS, the implementation will bind against the user DN, if it isn't already bound against it. It will then repeat the search for groups under the user authorization. This is to keep backwards compatible with the original behaviour A, in order to not break cases where the LDAP setup would deny a manager account to search for groups but allow it for normal users. To achieve this the implementation introduces an internal `LdapConnection` class that wraps the connection and keeps bind state, so that a rebind as a user is possible. This also fixes a resource leak where the connection was not closed in case that the initial bind as the manager account did not succeed. This commit would fix gitblit/gitblit#920 --- .../java/com/gitblit/auth/LdapAuthProvider.java | 398 +++++++++++++++------ 1 file changed, 284 insertions(+), 114 deletions(-) diff --git a/src/main/java/com/gitblit/auth/LdapAuthProvider.java b/src/main/java/com/gitblit/auth/LdapAuthProvider.java index cc772e7b..e1dec48f 100644 --- a/src/main/java/com/gitblit/auth/LdapAuthProvider.java +++ b/src/main/java/com/gitblit/auth/LdapAuthProvider.java @@ -39,6 +39,8 @@ import com.gitblit.service.LdapSyncService; import com.gitblit.utils.ArrayUtils; import com.gitblit.utils.StringUtils; import com.unboundid.ldap.sdk.Attribute; +import com.unboundid.ldap.sdk.BindRequest; +import com.unboundid.ldap.sdk.BindResult; import com.unboundid.ldap.sdk.DereferencePolicy; import com.unboundid.ldap.sdk.ExtendedResult; import com.unboundid.ldap.sdk.LDAPConnection; @@ -107,8 +109,14 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider { 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) { + LdapConnection ldapConnection = new LdapConnection(); + if (ldapConnection.connect()) { + if (ldapConnection.bind() == null) { + ldapConnection.close(); + logger.error("Cannot synchronize with LDAP."); + return; + } + try { String accountBase = settings.getString(Keys.realm.ldap.accountBase, ""); String uidAttribute = settings.getString(Keys.realm.ldap.uid, "uid"); @@ -179,66 +187,6 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider { } } - private LDAPConnection getLdapConnection() { - try { - - URI ldapUrl = new URI(settings.getRequiredString(Keys.realm.ldap.server)); - String ldapHost = ldapUrl.getHost(); - int ldapPort = ldapUrl.getPort(); - String bindUserName = settings.getString(Keys.realm.ldap.username, ""); - String bindPassword = settings.getString(Keys.realm.ldap.password, ""); - - LDAPConnection conn; - if (ldapUrl.getScheme().equalsIgnoreCase("ldaps")) { - // SSL - SSLUtil sslUtil = new SSLUtil(new TrustAllTrustManager()); - conn = new LDAPConnection(sslUtil.createSSLSocketFactory()); - if (ldapPort == -1) { - ldapPort = 636; - } - } else if (ldapUrl.getScheme().equalsIgnoreCase("ldap") || ldapUrl.getScheme().equalsIgnoreCase("ldap+tls")) { - // no encryption or StartTLS - conn = new LDAPConnection(); - if (ldapPort == -1) { - ldapPort = 389; - } - } else { - logger.error("Unsupported LDAP URL scheme: " + ldapUrl.getScheme()); - return null; - } - - conn.connect(ldapHost, ldapPort); - - if (ldapUrl.getScheme().equalsIgnoreCase("ldap+tls")) { - SSLUtil sslUtil = new SSLUtil(new TrustAllTrustManager()); - ExtendedResult extendedResult = conn.processExtendedOperation( - new StartTLSExtendedRequest(sslUtil.createSSLContext())); - if (extendedResult.getResultCode() != ResultCode.SUCCESS) { - throw new LDAPException(extendedResult.getResultCode()); - } - } - - if (StringUtils.isEmpty(bindUserName) && StringUtils.isEmpty(bindPassword)) { - // anonymous bind - conn.bind(new SimpleBindRequest()); - } else { - // authenticated bind - conn.bind(new SimpleBindRequest(bindUserName, bindPassword)); - } - - return conn; - - } catch (URISyntaxException e) { - logger.error("Bad LDAP URL, should be in the form: ldap(s|+tls)://:", e); - } catch (GeneralSecurityException e) { - logger.error("Unable to create SSL Connection", e); - } catch (LDAPException e) { - logger.error("Error Connecting to LDAP", e); - } - - return null; - } - /** * Credentials are defined in the LDAP server and can not be manipulated * from Gitblit. @@ -321,23 +269,26 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider { public UserModel authenticate(String username, char[] password) { String simpleUsername = getSimpleUsername(username); - LDAPConnection ldapConnection = getLdapConnection(); - if (ldapConnection != null) { - try { - boolean alreadyAuthenticated = false; + LdapConnection ldapConnection = new LdapConnection(); + if (ldapConnection.connect()) { - String bindPattern = settings.getString(Keys.realm.ldap.bindpattern, ""); - if (!StringUtils.isEmpty(bindPattern)) { - try { - String bindUser = StringUtils.replace(bindPattern, "${username}", escapeLDAPSearchFilter(simpleUsername)); - ldapConnection.bind(bindUser, new String(password)); + // Try to bind either to the "manager" account, + // or directly to the DN of the user logging in, if realm.ldap.bindpattern is configured. + String passwd = new String(password); + BindResult bindResult = null; + String bindPattern = settings.getString(Keys.realm.ldap.bindpattern, ""); + if (! StringUtils.isEmpty(bindPattern)) { + bindResult = ldapConnection.bind(bindPattern, simpleUsername, passwd); + } else { + bindResult = ldapConnection.bind(); + } + if (bindResult == null) { + ldapConnection.close(); + return null; + } - alreadyAuthenticated = true; - } catch (LDAPException e) { - return null; - } - } + try { // 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}))"); @@ -348,7 +299,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider { SearchResultEntry loggingInUser = result.getSearchEntries().get(0); String loggingInUserDN = loggingInUser.getDN(); - if (alreadyAuthenticated || isAuthenticated(ldapConnection, loggingInUserDN, new String(password))) { + if (ldapConnection.isAuthenticated(loggingInUserDN, passwd)) { logger.debug("LDAP authenticated: " + username); UserModel user = null; @@ -462,7 +413,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider { } } - private void getTeamsFromLdap(LDAPConnection ldapConnection, String simpleUsername, SearchResultEntry loggingInUser, UserModel user) { + private void getTeamsFromLdap(LdapConnection ldapConnection, String simpleUsername, SearchResultEntry loggingInUser, UserModel user) { String loggingInUserDN = loggingInUser.getDN(); // Clear the users team memberships - we're going to get them from LDAP @@ -479,7 +430,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider { groupMemberPattern = StringUtils.replace(groupMemberPattern, "${" + userAttribute.getName() + "}", escapeLDAPSearchFilter(userAttribute.getValue())); } - SearchResult teamMembershipResult = doSearch(ldapConnection, groupBase, true, groupMemberPattern, Arrays.asList("cn")); + SearchResult teamMembershipResult = searchTeamsInLdap(ldapConnection, groupBase, true, groupMemberPattern, Arrays.asList("cn")); if (teamMembershipResult != null && teamMembershipResult.getEntryCount() > 0) { for (int i = 0; i < teamMembershipResult.getEntryCount(); i++) { SearchResultEntry teamEntry = teamMembershipResult.getSearchEntries().get(i); @@ -496,12 +447,12 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider { } } - private void getEmptyTeamsFromLdap(LDAPConnection ldapConnection) { + private void getEmptyTeamsFromLdap(LdapConnection ldapConnection) { 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=*)))"); - SearchResult teamMembershipResult = doSearch(ldapConnection, groupBase, true, groupMemberPattern, null); + SearchResult teamMembershipResult = searchTeamsInLdap(ldapConnection, groupBase, true, groupMemberPattern, null); if (teamMembershipResult != null && teamMembershipResult.getEntryCount() > 0) { for (int i = 0; i < teamMembershipResult.getEntryCount(); i++) { SearchResultEntry teamEntry = teamMembershipResult.getSearchEntries().get(i); @@ -519,6 +470,30 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider { logger.info("Finished fetching empty teams from ldap."); } + + private SearchResult searchTeamsInLdap(LdapConnection ldapConnection, String base, boolean dereferenceAliases, String filter, List attributes) { + SearchResult result = ldapConnection.search(base, dereferenceAliases, filter, attributes); + if (result == null) { + return null; + } + + if (result.getResultCode() != ResultCode.SUCCESS) { + // Retry the search with user authorization in case we searched as a manager account that could not search for teams. + logger.debug("Rebinding as user to search for teams in LDAP"); + result = null; + if (ldapConnection.rebindAsUser()) { + result = ldapConnection.search(base, dereferenceAliases, filter, attributes); + if (result.getResultCode() != ResultCode.SUCCESS) { + return null; + } + logger.info("Successful search after rebinding as user."); + } + } + + return result; + } + + private TeamModel createTeamFromLdap(SearchResultEntry teamEntry) { TeamModel answer = new TeamModel(teamEntry.getAttributeValue("cn")); answer.accountType = getAccountType(); @@ -527,47 +502,21 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider { return answer; } - private SearchResult doSearch(LDAPConnection ldapConnection, String base, String filter) { - try { - return ldapConnection.search(base, SearchScope.SUB, filter); - } catch (LDAPSearchException e) { - logger.error("Problem Searching LDAP", e); - - return null; - } - } - - private SearchResult doSearch(LDAPConnection ldapConnection, String base, boolean dereferenceAliases, String filter, List attributes) { + private SearchResult doSearch(LdapConnection ldapConnection, String base, String filter) { try { SearchRequest searchRequest = new SearchRequest(base, SearchScope.SUB, filter); - if (dereferenceAliases) { - searchRequest.setDerefPolicy(DereferencePolicy.SEARCHING); - } - if (attributes != null) { - searchRequest.setAttributes(attributes); + SearchResult result = ldapConnection.search(searchRequest); + if (result.getResultCode() != ResultCode.SUCCESS) { + return null; } - return ldapConnection.search(searchRequest); - - } catch (LDAPSearchException e) { - logger.error("Problem Searching LDAP", e); - - return null; + return result; } catch (LDAPException e) { logger.error("Problem creating LDAP search", e); return null; } } - 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) { - logger.error("Error authenticating user", e); - return false; - } - } + /** * Returns a simple username without any domain prefixes. @@ -585,7 +534,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider { } // From: https://www.owasp.org/index.php/Preventing_LDAP_Injection_in_Java - public static final String escapeLDAPSearchFilter(String filter) { + private static final String escapeLDAPSearchFilter(String filter) { StringBuilder sb = new StringBuilder(); for (int i = 0; i < filter.length(); i++) { char curChar = filter.charAt(i); @@ -625,4 +574,225 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider { } } + + + private class LdapConnection { + private LDAPConnection conn; + private SimpleBindRequest currentBindRequest; + private SimpleBindRequest managerBindRequest; + private SimpleBindRequest userBindRequest; + + + public LdapConnection() { + String bindUserName = settings.getString(Keys.realm.ldap.username, ""); + String bindPassword = settings.getString(Keys.realm.ldap.password, ""); + if (StringUtils.isEmpty(bindUserName) && StringUtils.isEmpty(bindPassword)) { + this.managerBindRequest = new SimpleBindRequest(); + } + this.managerBindRequest = new SimpleBindRequest(bindUserName, bindPassword); + } + + + boolean connect() { + try { + URI ldapUrl = new URI(settings.getRequiredString(Keys.realm.ldap.server)); + String ldapHost = ldapUrl.getHost(); + int ldapPort = ldapUrl.getPort(); + + if (ldapUrl.getScheme().equalsIgnoreCase("ldaps")) { + // SSL + SSLUtil sslUtil = new SSLUtil(new TrustAllTrustManager()); + conn = new LDAPConnection(sslUtil.createSSLSocketFactory()); + if (ldapPort == -1) { + ldapPort = 636; + } + } else if (ldapUrl.getScheme().equalsIgnoreCase("ldap") || ldapUrl.getScheme().equalsIgnoreCase("ldap+tls")) { + // no encryption or StartTLS + conn = new LDAPConnection(); + if (ldapPort == -1) { + ldapPort = 389; + } + } else { + logger.error("Unsupported LDAP URL scheme: " + ldapUrl.getScheme()); + return false; + } + + conn.connect(ldapHost, ldapPort); + + if (ldapUrl.getScheme().equalsIgnoreCase("ldap+tls")) { + SSLUtil sslUtil = new SSLUtil(new TrustAllTrustManager()); + ExtendedResult extendedResult = conn.processExtendedOperation( + new StartTLSExtendedRequest(sslUtil.createSSLContext())); + if (extendedResult.getResultCode() != ResultCode.SUCCESS) { + throw new LDAPException(extendedResult.getResultCode()); + } + } + + return true; + + } catch (URISyntaxException e) { + logger.error("Bad LDAP URL, should be in the form: ldap(s|+tls)://:", e); + } catch (GeneralSecurityException e) { + logger.error("Unable to create SSL Connection", e); + } catch (LDAPException e) { + logger.error("Error Connecting to LDAP", e); + } + + return false; + } + + + void close() { + if (conn != null) { + conn.close(); + } + } + + + SearchResult search(SearchRequest request) { + try { + return conn.search(request); + } catch (LDAPSearchException e) { + logger.error("Problem Searching LDAP [{}]", e.getResultCode()); + return e.getSearchResult(); + } + } + + + SearchResult search(String base, boolean dereferenceAliases, String filter, List attributes) { + try { + SearchRequest searchRequest = new SearchRequest(base, SearchScope.SUB, filter); + if (dereferenceAliases) { + searchRequest.setDerefPolicy(DereferencePolicy.SEARCHING); + } + if (attributes != null) { + searchRequest.setAttributes(attributes); + } + SearchResult result = search(searchRequest); + return result; + + } catch (LDAPException e) { + logger.error("Problem creating LDAP search", e); + return null; + } + } + + + + /** + * Bind using the manager credentials set in realm.ldap.username and ..password + * @return A bind result, or null if binding failed. + */ + BindResult bind() { + BindResult result = null; + try { + result = conn.bind(managerBindRequest); + currentBindRequest = managerBindRequest; + } catch (LDAPException e) { + logger.error("Error authenticating to LDAP with manager account to search the directory."); + logger.error(" Please check your settings for realm.ldap.username and realm.ldap.password."); + logger.debug(" Received exception when binding to LDAP", e); + return null; + } + return result; + } + + + /** + * Bind using the given credentials, by filling in the username in the given {@code bindPattern} to + * create the DN. + * @return A bind result, or null if binding failed. + */ + BindResult bind(String bindPattern, String simpleUsername, String password) { + BindResult result = null; + try { + String bindUser = StringUtils.replace(bindPattern, "${username}", escapeLDAPSearchFilter(simpleUsername)); + SimpleBindRequest request = new SimpleBindRequest(bindUser, password); + result = conn.bind(request); + userBindRequest = request; + currentBindRequest = userBindRequest; + } catch (LDAPException e) { + logger.error("Error authenticating to LDAP with user account to search the directory."); + logger.error(" Please check your settings for realm.ldap.bindpattern."); + logger.debug(" Received exception when binding to LDAP", e); + return null; + } + return result; + } + + + boolean rebindAsUser() { + if (userBindRequest == null || currentBindRequest == userBindRequest) { + return false; + } + try { + conn.bind(userBindRequest); + currentBindRequest = userBindRequest; + } catch (LDAPException e) { + conn.close(); + logger.error("Error rebinding to LDAP with user account.", e); + return false; + } + return true; + } + + + boolean isAuthenticated(String userDn, String password) { + verifyCurrentBinding(); + + // If the currently bound DN is already the DN of the logging in user, authentication has already happened + // during the previous bind operation. We accept this and return with the current bind left in place. + // This could also be changed to always retry binding as the logging in user, to make sure that the + // connection binding has not been tampered with in between. So far I see no way how this could happen + // and thus skip the repeated binding. + // This check also makes sure that the DN in realm.ldap.bindpattern actually matches the DN that was found + // when searching the user entry. + String boundDN = currentBindRequest.getBindDN(); + if (boundDN != null && boundDN.equals(userDn)) { + return true; + } + + // Bind a the logging in user to check for authentication. + // Afterwards, bind as the original bound DN again, to restore the previous authorization. + boolean isAuthenticated = false; + try { + // Binding will stop any LDAP-Injection Attacks since the searched-for user needs to bind to that DN + SimpleBindRequest ubr = new SimpleBindRequest(userDn, password); + conn.bind(ubr); + isAuthenticated = true; + userBindRequest = ubr; + } catch (LDAPException e) { + logger.error("Error authenticating user ({})", userDn, e); + } + + try { + conn.bind(currentBindRequest); + } catch (LDAPException e) { + logger.error("Error reinstating original LDAP authorization (code {}). Team information may be inaccurate for this log in.", + e.getResultCode(), e); + } + return isAuthenticated; + } + + + + private boolean verifyCurrentBinding() { + BindRequest lastBind = conn.getLastBindRequest(); + if (lastBind == currentBindRequest) { + return true; + } + logger.debug("Unexpected binding in LdapConnection. {} != {}", lastBind, currentBindRequest); + + String lastBoundDN = ((SimpleBindRequest)lastBind).getBindDN(); + String boundDN = currentBindRequest.getBindDN(); + logger.debug("Currently bound as '{}', check authentication for '{}'", lastBoundDN, boundDN); + if (boundDN != null && ! boundDN.equals(lastBoundDN)) { + logger.warn("Unexpected binding DN in LdapConnection. '{}' != '{}'.", lastBoundDN, boundDN); + logger.warn("Updated binding information in LDAP connection."); + currentBindRequest = (SimpleBindRequest)lastBind; + return false; + } + return true; + } + } } -- cgit v1.2.3 From f004a7f1d6bd9eaa6e7a8c8cd9ddae4187bd9994 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Mon, 14 Nov 2016 20:18:07 +0100 Subject: Update documentation for LDAP binding in default.properties. Extend the comments for some realm.ldap.* properties to better explain use cases and requirements. --- src/main/distrib/data/defaults.properties | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/main/distrib/data/defaults.properties b/src/main/distrib/data/defaults.properties index 0c7d6cd4..16be8476 100644 --- a/src/main/distrib/data/defaults.properties +++ b/src/main/distrib/data/defaults.properties @@ -1797,6 +1797,10 @@ realm.salesforce.orgId = 0 realm.ldap.server = ldap://localhost # Login username for LDAP searches. +# This is usually a user with permissions to search LDAP users and groups. +# It must have at least have the permission to search users. If it does not +# have permission to search groups, the normal user logging in must have +# the permission in LDAP to search groups. # If this value is unspecified, anonymous LDAP login will be used. # # e.g. mydomain\\username @@ -1809,8 +1813,14 @@ realm.ldap.username = cn=Directory Manager # SINCE 1.0.0 realm.ldap.password = password -# Bind pattern for Authentication. -# Allow to directly authenticate an user without LDAP Searches. +# Bind pattern for user authentication. +# Allow to directly authenticate an user without searching for it in LDAP. +# Use this if the LDAP server does not allow anonymous access and you don't +# want to use a specific account to run searches. When set, it will override +# the settings realm.ldap.username and realm.ldap.password. +# This requires that all relevant user entries are children to the same DN, +# and that logging users have permission to search for their groups in LDAP. +# This will disable synchronization as a specific LDAP account is needed for that. # # e.g. CN=${username},OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain # @@ -1926,6 +1936,9 @@ realm.ldap.email = email realm.ldap.uid = uid # Defines whether to synchronize all LDAP users and teams into the user service +# This requires either anonymous LDAP access or that a specific account is set +# in realm.ldap.username and realm.ldap.password, that has permission to read +# users and groups in LDAP. # # Valid values: true, false # If left blank, false is assumed -- cgit v1.2.3