You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2021/06/08 15:49:43 UTC
[GitHub] [james-project] rouazana commented on a change in pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
rouazana commented on a change in pull request #478:
URL: https://github.com/apache/james-project/pull/478#discussion_r647558810
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/LdapRepositoryConfiguration.java
##########
@@ -160,7 +163,6 @@ public static LdapRepositoryConfiguration from(HierarchicalConfiguration<Immutab
String userIdAttribute = configuration.getString("[@userIdAttribute]");
String userObjectClass = configuration.getString("[@userObjectClass]");
// Default is to use connection pooling
- boolean useConnectionPool = configuration.getBoolean("[@useConnectionPool]", NO_CONNECTION_POOL);
Review comment:
you need to document (in upgrade note) this breaking change
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -120,63 +94,34 @@ public void configure(LdapRepositoryConfiguration configuration) {
* specified LDAP host.
*/
public void init() throws Exception {
+
if (LOGGER.isDebugEnabled()) {
LOGGER.debug(this.getClass().getName() + ".init()" + '\n' + "LDAP host: " + ldapConfiguration.getLdapHost()
+ '\n' + "User baseDN: " + ldapConfiguration.getUserBase() + '\n' + "userIdAttribute: "
+ ldapConfiguration.getUserIdAttribute() + '\n' + "Group restriction: " + ldapConfiguration.getRestriction()
- + '\n' + "UseConnectionPool: " + ldapConfiguration.useConnectionPool() + '\n' + "connectionTimeout: "
+ + '\n' + "connectionTimeout: "
+ ldapConfiguration.getConnectionTimeout() + '\n' + "readTimeout: " + ldapConfiguration.getReadTimeout()
- + '\n' + "retrySchedule: " + schedule + '\n' + "maxRetries: " + ldapConfiguration.getMaxRetries() + '\n');
+ + '\n' + "maxRetries: " + ldapConfiguration.getMaxRetries() + '\n');
}
- // Setup the initial LDAP context
- updateLdapContext();
- }
-
- protected void updateLdapContext() throws NamingException {
- ldapContext = computeLdapContext();
- }
+ filterTemplate = "(&({0}={1})(objectClass={2})" + StringUtils.defaultString(ldapConfiguration.getFilter(), "") + ")";
- /**
- * Answers a new LDAP/JNDI context using the specified user credentials.
- *
- * @return an LDAP directory context
- * @throws NamingException
- * Propagated from underlying LDAP communication API.
- */
- protected LdapContext computeLdapContext() throws NamingException {
- return new RetryingLdapContext(schedule, ldapConfiguration.getMaxRetries()) {
+ LDAPConnectionOptions connectionOptions = new LDAPConnectionOptions();
+ connectionOptions.setConnectTimeoutMillis(ldapConfiguration.getConnectionTimeout());
+ connectionOptions.setResponseTimeoutMillis(ldapConfiguration.getReadTimeout());
- @Override
- public Context newDelegate() throws NamingException {
- return new InitialLdapContext(getContextEnvironment(), null);
- }
- };
+ URI uri = new URI(ldapConfiguration.getLdapHost());
+ SocketFactory socketFactory = null;
+ LDAPConnection ldapConnection = new LDAPConnection(socketFactory, connectionOptions, uri.getHost(), uri.getPort(), ldapConfiguration.getPrincipal(), ldapConfiguration.getCredentials());
+ ldapConnectionPool = new LDAPConnectionPool(ldapConnection, 4);
+ // TODO implement retries
Review comment:
still a TODO?
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -209,29 +154,20 @@ private boolean userInGroupsMembershipList(String userDN,
return result;
}
- /**
- * Gets all the user entities taken from the LDAP server, as taken from the
- * search-context given by the value of the attribute {@link LdapRepositoryConfiguration#userBase}.
- *
- * @return A set containing all the relevant users found in the LDAP
- * directory.
- * @throws NamingException
- * Propagated from the LDAP communication layer.
- */
- private Set<String> getAllUsersFromLDAP() throws NamingException {
- Set<String> result = new HashSet<>();
-
- SearchControls sc = new SearchControls();
- sc.setSearchScope(SearchControls.SUBTREE_SCOPE);
- sc.setReturningAttributes(new String[] { "distinguishedName" });
- NamingEnumeration<SearchResult> sr = ldapContext.search(ldapConfiguration.getUserBase(), "(objectClass="
- + ldapConfiguration.getUserObjectClass() + ")", sc);
- while (sr.hasMore()) {
- SearchResult r = sr.next();
- result.add(r.getNameInNamespace());
- }
+ private Set<String> getAllUsersFromLDAP() throws LDAPException {
+ LDAPConnection connection = ldapConnectionPool.getConnection();
+ try {
+ SearchResult searchResult = connection.search(ldapConfiguration.getUserBase(),
+ SearchScope.SUB,
+ filterTemplate);
- return result;
+ return searchResult.getSearchEntries()
+ .stream()
+ .map(entry -> entry.getObjectClassAttribute().getName())
+ .collect(Guavate.toImmutableSet());
+ } finally {
+ ldapConnectionPool.releaseConnection(connection);
Review comment:
why do you need this?
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUser.java
##########
@@ -55,50 +57,42 @@
* <code>"myorg.com"</code>, the user's email address will be
* <code>"john.bold@myorg.com"</code>.
*/
- private Username userName;
+ private final Username userName;
/**
* The distinguished name of the user-record in the LDAP directory.
*/
- private String userDN;
+ private final String userDN;
/**
* The context for the LDAP server from which to retrieve the
* user's details.
*/
- private LdapContext ldapContext = null;
+ private final LDAPConnectionPool connectionPool;
- /**
- * Creates a new instance of ReadOnlyLDAPUser.
- *
- */
- private ReadOnlyLDAPUser() {
- super();
- }
+ private final LdapRepositoryConfiguration configuration;
/**
* Constructs an instance for the given user-details, and which will
* authenticate against the given host.
- *
- * @param userName
+ * @param userName
Review comment:
```suggestion
* @param userName
```
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -260,133 +195,150 @@ private boolean userInGroupsMembershipList(String userDN,
return results;
}
- /**
- * For a given name, this method makes ldap search in userBase with filter {@link LdapRepositoryConfiguration#userIdAttribute}=name
- * and objectClass={@link LdapRepositoryConfiguration#userObjectClass} and builds {@link User} based on search result.
- *
- * @param name
- * The userId which should be value of the field {@link LdapRepositoryConfiguration#userIdAttribute}
- * @return A {@link ReadOnlyLDAPUser} instance which is initialized with the
- * userId of this user and ldap connection information with which
- * the user was searched. Return null if such a user was not found.
- * @throws NamingException
- * Propagated by the underlying LDAP communication layer.
- */
- private ReadOnlyLDAPUser searchAndBuildUser(Username name) throws NamingException {
- SearchControls sc = new SearchControls();
- sc.setSearchScope(SearchControls.SUBTREE_SCOPE);
- sc.setReturningAttributes(new String[] { ldapConfiguration.getUserIdAttribute() });
- sc.setCountLimit(1);
-
- String filterTemplate = "(&({0}={1})(objectClass={2})" +
- StringUtils.defaultString(ldapConfiguration.getFilter(), "") +
- ")";
-
- String sanitizedFilter = FilterEncoder.format(
- filterTemplate,
- ldapConfiguration.getUserIdAttribute(),
- name.asString(),
- ldapConfiguration.getUserObjectClass());
+ private ReadOnlyLDAPUser searchAndBuildUser(Username name) throws LDAPException {
+ LDAPConnection connection = ldapConnectionPool.getConnection();
+ try {
+ String sanitizedFilter = FilterEncoder.format(
+ filterTemplate,
+ ldapConfiguration.getUserIdAttribute(),
+ name.asString(),
+ ldapConfiguration.getUserObjectClass());
+
+ SearchResult searchResult = connection.search(ldapConfiguration.getUserBase(),
+ SearchScope.SUB,
+ sanitizedFilter,
+ ldapConfiguration.getUserIdAttribute());
+
+ SearchResultEntry result = searchResult.getSearchEntries()
+ .stream()
+ .findFirst()
+ .orElse(null);
+ if (result == null) {
+ return null;
+ }
- NamingEnumeration<SearchResult> sr = ldapContext.search(ldapConfiguration.getUserBase(), sanitizedFilter, sc);
+ if (!ldapConfiguration.getRestriction().isActivated()
+ || userInGroupsMembershipList(result.getDN(), ldapConfiguration.getRestriction().getGroupMembershipLists(connection))) {
- if (!sr.hasMore()) {
+ return new ReadOnlyLDAPUser(name, result.getDN(), ldapConnectionPool, ldapConfiguration);
+ }
return null;
+ } finally {
+ ldapConnectionPool.releaseConnection(connection);
}
-
- SearchResult r = sr.next();
- Attribute userName = r.getAttributes().get(ldapConfiguration.getUserIdAttribute());
-
- if (!ldapConfiguration.getRestriction().isActivated()
- || userInGroupsMembershipList(r.getNameInNamespace(), ldapConfiguration.getRestriction().getGroupMembershipLists(ldapContext))) {
- return new ReadOnlyLDAPUser(Username.of(userName.get().toString()), r.getNameInNamespace(), ldapContext);
- }
-
- return null;
}
- /**
- * Given a userDN, this method retrieves the user attributes from the LDAP
- * server, so as to extract the items that are of interest to James.
- * Specifically it extracts the userId, which is extracted from the LDAP
- * attribute whose name is given by the value of the field
- * {@link LdapRepositoryConfiguration#userIdAttribute}.
- *
- * @param userDN
- * The distinguished-name of the user whose details are to be
- * extracted from the LDAP repository.
- * @return A {@link ReadOnlyLDAPUser} instance which is initialized with the
- * userId of this user and ldap connection information with which
- * the userDN and attributes were obtained.
- * @throws NamingException
- * Propagated by the underlying LDAP communication layer.
- */
- private Optional<ReadOnlyLDAPUser> buildUser(String userDN) throws NamingException {
- Attributes userAttributes = ldapContext.getAttributes(userDN);
- Optional<Attribute> userName = Optional.ofNullable(userAttributes.get(ldapConfiguration.getUserIdAttribute()));
- return userName
- .map(Throwing.<Attribute, String>function(u -> u.get().toString()).sneakyThrow())
- .map(Username::of)
- .map(username -> new ReadOnlyLDAPUser(username, userDN, ldapContext));
+ private Optional<ReadOnlyLDAPUser> buildUser(String userDN) throws LDAPException {
+ LDAPConnection connection = ldapConnectionPool.getConnection();
+ try {
+ SearchResultEntry userAttributes = connection.getEntry(userDN);
Review comment:
idem you are doing only one operation, so no need to getConn/op/release, just use the pool
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUser.java
##########
@@ -38,14 +41,13 @@
* means of authenticating the user against the LDAP server. Consequently
* invocations of the contract method {@link User#setPassword(String)} always
* returns <code>false</code>.
- *
- * @see SimpleLDAPConnection
+ *
* @see ReadOnlyUsersLDAPRepository
*
*/
public class ReadOnlyLDAPUser implements User, Serializable {
- // private static final long serialVersionUID = -6712066073820393235L;
- private static final long serialVersionUID = -5201235065842464013L;
+ private static final long serialVersionUID = -5201235065842464014L;
Review comment:
you can probably remove this
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -209,29 +154,20 @@ private boolean userInGroupsMembershipList(String userDN,
return result;
}
- /**
- * Gets all the user entities taken from the LDAP server, as taken from the
- * search-context given by the value of the attribute {@link LdapRepositoryConfiguration#userBase}.
- *
- * @return A set containing all the relevant users found in the LDAP
- * directory.
- * @throws NamingException
- * Propagated from the LDAP communication layer.
- */
- private Set<String> getAllUsersFromLDAP() throws NamingException {
- Set<String> result = new HashSet<>();
-
- SearchControls sc = new SearchControls();
- sc.setSearchScope(SearchControls.SUBTREE_SCOPE);
- sc.setReturningAttributes(new String[] { "distinguishedName" });
- NamingEnumeration<SearchResult> sr = ldapContext.search(ldapConfiguration.getUserBase(), "(objectClass="
- + ldapConfiguration.getUserObjectClass() + ")", sc);
- while (sr.hasMore()) {
- SearchResult r = sr.next();
- result.add(r.getNameInNamespace());
- }
+ private Set<String> getAllUsersFromLDAP() throws LDAPException {
+ LDAPConnection connection = ldapConnectionPool.getConnection();
+ try {
+ SearchResult searchResult = connection.search(ldapConfiguration.getUserBase(),
+ SearchScope.SUB,
+ filterTemplate);
Review comment:
this cannot work if you search directly with a template, is it tested?
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -260,133 +195,150 @@ private boolean userInGroupsMembershipList(String userDN,
return results;
}
- /**
- * For a given name, this method makes ldap search in userBase with filter {@link LdapRepositoryConfiguration#userIdAttribute}=name
- * and objectClass={@link LdapRepositoryConfiguration#userObjectClass} and builds {@link User} based on search result.
- *
- * @param name
- * The userId which should be value of the field {@link LdapRepositoryConfiguration#userIdAttribute}
- * @return A {@link ReadOnlyLDAPUser} instance which is initialized with the
- * userId of this user and ldap connection information with which
- * the user was searched. Return null if such a user was not found.
- * @throws NamingException
- * Propagated by the underlying LDAP communication layer.
- */
- private ReadOnlyLDAPUser searchAndBuildUser(Username name) throws NamingException {
- SearchControls sc = new SearchControls();
- sc.setSearchScope(SearchControls.SUBTREE_SCOPE);
- sc.setReturningAttributes(new String[] { ldapConfiguration.getUserIdAttribute() });
- sc.setCountLimit(1);
-
- String filterTemplate = "(&({0}={1})(objectClass={2})" +
- StringUtils.defaultString(ldapConfiguration.getFilter(), "") +
- ")";
-
- String sanitizedFilter = FilterEncoder.format(
- filterTemplate,
- ldapConfiguration.getUserIdAttribute(),
- name.asString(),
- ldapConfiguration.getUserObjectClass());
+ private ReadOnlyLDAPUser searchAndBuildUser(Username name) throws LDAPException {
+ LDAPConnection connection = ldapConnectionPool.getConnection();
+ try {
+ String sanitizedFilter = FilterEncoder.format(
+ filterTemplate,
+ ldapConfiguration.getUserIdAttribute(),
+ name.asString(),
+ ldapConfiguration.getUserObjectClass());
+
+ SearchResult searchResult = connection.search(ldapConfiguration.getUserBase(),
+ SearchScope.SUB,
+ sanitizedFilter,
+ ldapConfiguration.getUserIdAttribute());
+
+ SearchResultEntry result = searchResult.getSearchEntries()
+ .stream()
+ .findFirst()
+ .orElse(null);
+ if (result == null) {
+ return null;
+ }
- NamingEnumeration<SearchResult> sr = ldapContext.search(ldapConfiguration.getUserBase(), sanitizedFilter, sc);
+ if (!ldapConfiguration.getRestriction().isActivated()
+ || userInGroupsMembershipList(result.getDN(), ldapConfiguration.getRestriction().getGroupMembershipLists(connection))) {
- if (!sr.hasMore()) {
+ return new ReadOnlyLDAPUser(name, result.getDN(), ldapConnectionPool, ldapConfiguration);
+ }
return null;
+ } finally {
+ ldapConnectionPool.releaseConnection(connection);
}
-
- SearchResult r = sr.next();
- Attribute userName = r.getAttributes().get(ldapConfiguration.getUserIdAttribute());
-
- if (!ldapConfiguration.getRestriction().isActivated()
- || userInGroupsMembershipList(r.getNameInNamespace(), ldapConfiguration.getRestriction().getGroupMembershipLists(ldapContext))) {
- return new ReadOnlyLDAPUser(Username.of(userName.get().toString()), r.getNameInNamespace(), ldapContext);
- }
-
- return null;
}
- /**
- * Given a userDN, this method retrieves the user attributes from the LDAP
- * server, so as to extract the items that are of interest to James.
- * Specifically it extracts the userId, which is extracted from the LDAP
- * attribute whose name is given by the value of the field
- * {@link LdapRepositoryConfiguration#userIdAttribute}.
- *
- * @param userDN
- * The distinguished-name of the user whose details are to be
- * extracted from the LDAP repository.
- * @return A {@link ReadOnlyLDAPUser} instance which is initialized with the
- * userId of this user and ldap connection information with which
- * the userDN and attributes were obtained.
- * @throws NamingException
- * Propagated by the underlying LDAP communication layer.
- */
- private Optional<ReadOnlyLDAPUser> buildUser(String userDN) throws NamingException {
- Attributes userAttributes = ldapContext.getAttributes(userDN);
- Optional<Attribute> userName = Optional.ofNullable(userAttributes.get(ldapConfiguration.getUserIdAttribute()));
- return userName
- .map(Throwing.<Attribute, String>function(u -> u.get().toString()).sneakyThrow())
- .map(Username::of)
- .map(username -> new ReadOnlyLDAPUser(username, userDN, ldapContext));
+ private Optional<ReadOnlyLDAPUser> buildUser(String userDN) throws LDAPException {
+ LDAPConnection connection = ldapConnectionPool.getConnection();
+ try {
+ SearchResultEntry userAttributes = connection.getEntry(userDN);
+ Optional<String> userName = Optional.ofNullable(userAttributes.getAttributeValue(ldapConfiguration.getUserIdAttribute()));
+ return userName
+ .map(Username::of)
+ .map(username -> new ReadOnlyLDAPUser(username, userDN, ldapConnectionPool, ldapConfiguration));
+ } finally {
+ ldapConnectionPool.releaseConnection(connection);
+ }
}
@Override
public boolean contains(Username name) throws UsersRepositoryException {
- return getUserByName(name).isPresent();
+ try {
+ return Mono.fromCallable(() -> doContains(name))
+ .retryWhen(ldapConfiguration.retrySpec())
Review comment:
I don't understand exactly when the retry is done (I mean on which condition), can you explain me?
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -120,63 +94,34 @@ public void configure(LdapRepositoryConfiguration configuration) {
* specified LDAP host.
*/
public void init() throws Exception {
+
if (LOGGER.isDebugEnabled()) {
LOGGER.debug(this.getClass().getName() + ".init()" + '\n' + "LDAP host: " + ldapConfiguration.getLdapHost()
+ '\n' + "User baseDN: " + ldapConfiguration.getUserBase() + '\n' + "userIdAttribute: "
+ ldapConfiguration.getUserIdAttribute() + '\n' + "Group restriction: " + ldapConfiguration.getRestriction()
- + '\n' + "UseConnectionPool: " + ldapConfiguration.useConnectionPool() + '\n' + "connectionTimeout: "
+ + '\n' + "connectionTimeout: "
+ ldapConfiguration.getConnectionTimeout() + '\n' + "readTimeout: " + ldapConfiguration.getReadTimeout()
- + '\n' + "retrySchedule: " + schedule + '\n' + "maxRetries: " + ldapConfiguration.getMaxRetries() + '\n');
+ + '\n' + "maxRetries: " + ldapConfiguration.getMaxRetries() + '\n');
}
- // Setup the initial LDAP context
- updateLdapContext();
- }
-
- protected void updateLdapContext() throws NamingException {
- ldapContext = computeLdapContext();
- }
+ filterTemplate = "(&({0}={1})(objectClass={2})" + StringUtils.defaultString(ldapConfiguration.getFilter(), "") + ")";
Review comment:
you are prone to some ldap injection attacks by building you filter manually, please use Filter and the associated builders to generate correctly your filter and eventually escape needed characters
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUser.java
##########
@@ -139,27 +133,19 @@ public boolean setPassword(String newPass) {
*/
@Override
public boolean verifyPassword(String password) {
- boolean result = false;
- LdapContext ldapContext = null;
try {
- ldapContext = this.ldapContext.newInstance(null);
- ldapContext.addToEnvironment(Context.SECURITY_AUTHENTICATION,
- LdapConstants.SECURITY_AUTHENTICATION_SIMPLE);
- ldapContext.addToEnvironment(Context.SECURITY_PRINCIPAL, userDN);
- ldapContext.addToEnvironment(Context.SECURITY_CREDENTIALS, password);
- ldapContext.reconnect(null);
- result = true;
- } catch (NamingException exception) {
- // no-op
- } finally {
- if (null != ldapContext) {
- try {
- ldapContext.close();
- } catch (NamingException ex) {
- // no-op
- }
- }
+ return Mono.fromCallable(() -> doVerifyPassword(password))
+ .retryWhen(configuration.retrySpec())
+ .block();
+ } catch (Exception e) {
+ LOGGER.error("Unexpected error upon authentication", e);
+ return false;
}
- return result;
+ }
+
+ private boolean doVerifyPassword(String password) throws LDAPException {
+ BindResult bindResult = connectionPool.bindAndRevertAuthentication(userDN, password);
+ return bindResult.getResultCode()
Review comment:
you cannot compare to a constant LDAP_SUCCESS?
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -120,63 +94,34 @@ public void configure(LdapRepositoryConfiguration configuration) {
* specified LDAP host.
*/
public void init() throws Exception {
+
if (LOGGER.isDebugEnabled()) {
LOGGER.debug(this.getClass().getName() + ".init()" + '\n' + "LDAP host: " + ldapConfiguration.getLdapHost()
+ '\n' + "User baseDN: " + ldapConfiguration.getUserBase() + '\n' + "userIdAttribute: "
+ ldapConfiguration.getUserIdAttribute() + '\n' + "Group restriction: " + ldapConfiguration.getRestriction()
- + '\n' + "UseConnectionPool: " + ldapConfiguration.useConnectionPool() + '\n' + "connectionTimeout: "
+ + '\n' + "connectionTimeout: "
+ ldapConfiguration.getConnectionTimeout() + '\n' + "readTimeout: " + ldapConfiguration.getReadTimeout()
- + '\n' + "retrySchedule: " + schedule + '\n' + "maxRetries: " + ldapConfiguration.getMaxRetries() + '\n');
+ + '\n' + "maxRetries: " + ldapConfiguration.getMaxRetries() + '\n');
}
- // Setup the initial LDAP context
- updateLdapContext();
- }
-
- protected void updateLdapContext() throws NamingException {
- ldapContext = computeLdapContext();
- }
+ filterTemplate = "(&({0}={1})(objectClass={2})" + StringUtils.defaultString(ldapConfiguration.getFilter(), "") + ")";
- /**
- * Answers a new LDAP/JNDI context using the specified user credentials.
- *
- * @return an LDAP directory context
- * @throws NamingException
- * Propagated from underlying LDAP communication API.
- */
- protected LdapContext computeLdapContext() throws NamingException {
- return new RetryingLdapContext(schedule, ldapConfiguration.getMaxRetries()) {
+ LDAPConnectionOptions connectionOptions = new LDAPConnectionOptions();
+ connectionOptions.setConnectTimeoutMillis(ldapConfiguration.getConnectionTimeout());
+ connectionOptions.setResponseTimeoutMillis(ldapConfiguration.getReadTimeout());
- @Override
- public Context newDelegate() throws NamingException {
- return new InitialLdapContext(getContextEnvironment(), null);
- }
- };
+ URI uri = new URI(ldapConfiguration.getLdapHost());
+ SocketFactory socketFactory = null;
+ LDAPConnection ldapConnection = new LDAPConnection(socketFactory, connectionOptions, uri.getHost(), uri.getPort(), ldapConfiguration.getPrincipal(), ldapConfiguration.getCredentials());
+ ldapConnectionPool = new LDAPConnectionPool(ldapConnection, 4);
Review comment:
why 4?
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -209,29 +154,20 @@ private boolean userInGroupsMembershipList(String userDN,
return result;
}
- /**
- * Gets all the user entities taken from the LDAP server, as taken from the
- * search-context given by the value of the attribute {@link LdapRepositoryConfiguration#userBase}.
- *
- * @return A set containing all the relevant users found in the LDAP
- * directory.
- * @throws NamingException
- * Propagated from the LDAP communication layer.
- */
- private Set<String> getAllUsersFromLDAP() throws NamingException {
- Set<String> result = new HashSet<>();
-
- SearchControls sc = new SearchControls();
- sc.setSearchScope(SearchControls.SUBTREE_SCOPE);
- sc.setReturningAttributes(new String[] { "distinguishedName" });
- NamingEnumeration<SearchResult> sr = ldapContext.search(ldapConfiguration.getUserBase(), "(objectClass="
- + ldapConfiguration.getUserObjectClass() + ")", sc);
- while (sr.hasMore()) {
- SearchResult r = sr.next();
- result.add(r.getNameInNamespace());
- }
+ private Set<String> getAllUsersFromLDAP() throws LDAPException {
+ LDAPConnection connection = ldapConnectionPool.getConnection();
+ try {
+ SearchResult searchResult = connection.search(ldapConfiguration.getUserBase(),
+ SearchScope.SUB,
+ filterTemplate);
- return result;
+ return searchResult.getSearchEntries()
+ .stream()
+ .map(entry -> entry.getObjectClassAttribute().getName())
+ .collect(Guavate.toImmutableSet());
+ } finally {
+ ldapConnectionPool.releaseConnection(connection);
Review comment:
I mean if you are doing only one operation, you can issue it directly on the pool instead of getting a connection
##########
File path: server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -209,29 +154,20 @@ private boolean userInGroupsMembershipList(String userDN,
return result;
}
- /**
- * Gets all the user entities taken from the LDAP server, as taken from the
- * search-context given by the value of the attribute {@link LdapRepositoryConfiguration#userBase}.
- *
- * @return A set containing all the relevant users found in the LDAP
- * directory.
- * @throws NamingException
- * Propagated from the LDAP communication layer.
- */
- private Set<String> getAllUsersFromLDAP() throws NamingException {
- Set<String> result = new HashSet<>();
-
- SearchControls sc = new SearchControls();
- sc.setSearchScope(SearchControls.SUBTREE_SCOPE);
- sc.setReturningAttributes(new String[] { "distinguishedName" });
- NamingEnumeration<SearchResult> sr = ldapContext.search(ldapConfiguration.getUserBase(), "(objectClass="
- + ldapConfiguration.getUserObjectClass() + ")", sc);
- while (sr.hasMore()) {
- SearchResult r = sr.next();
- result.add(r.getNameInNamespace());
- }
+ private Set<String> getAllUsersFromLDAP() throws LDAPException {
+ LDAPConnection connection = ldapConnectionPool.getConnection();
+ try {
+ SearchResult searchResult = connection.search(ldapConfiguration.getUserBase(),
+ SearchScope.SUB,
+ filterTemplate);
- return result;
+ return searchResult.getSearchEntries()
+ .stream()
+ .map(entry -> entry.getObjectClassAttribute().getName())
Review comment:
why do you get this? I really think this method has never been tested
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org