rouazana commented on a change in pull request #478:
URL: https://github.com/apache/james-project/pull/478#discussion_r648067446
##########
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;
Review comment:
There is a nice DN type in the unboundid library :)
##########
File path:
server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -209,29 +171,33 @@ 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> getAllUsersDNFromLDAP() throws LDAPException {
+ SearchRequest searchRequest = new
SearchRequest(ldapConfiguration.getUserBase(),
+ SearchScope.SUB,
+ createFilter(),
+ ldapConfiguration.getUserIdAttribute());
Review comment:
you can eventually use `SearchRequest.NO_ATTRIBUTES` here instead
##########
File path:
server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPGroupRestriction.java
##########
@@ -112,13 +112,12 @@ public String toString() {
* <code>groupDN</code> is associated to a list of <code>userDNs</code>.
*
* @return Returns a map of groupDNs to userDN lists.
- * @throws NamingException Propagated from underlying LDAP communication
layer.
*/
- protected Map<String, Collection<String>>
getGroupMembershipLists(LdapContext ldapContext) throws NamingException {
+ protected Map<String, Collection<String>>
getGroupMembershipLists(LDAPConnection connection) throws LDAPException {
Review comment:
it could be better to have the connection pool here, WDYT?
##########
File path:
server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -260,133 +225,143 @@ 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(), "") +
- ")";
+ private ReadOnlyLDAPUser searchAndBuildUser(Username name) throws
LDAPException {
+ LDAPConnection connection = ldapConnectionPool.getConnection();
+ try {
+ SearchResult searchResult =
connection.search(ldapConfiguration.getUserBase(),
+ SearchScope.SUB,
+ createFilter(name.asString()),
+ ldapConfiguration.getUserIdAttribute());
- String sanitizedFilter = FilterEncoder.format(
- filterTemplate,
- ldapConfiguration.getUserIdAttribute(),
- name.asString(),
- ldapConfiguration.getUserObjectClass());
+ 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 {
+ SearchResultEntry userAttributes = ldapConnectionPool.getEntry(userDN);
+ Optional<String> userName =
Optional.ofNullable(userAttributes.getAttributeValue(ldapConfiguration.getUserIdAttribute()));
+ return userName
+ .map(Username::of)
+ .map(username -> new ReadOnlyLDAPUser(username, userDN,
ldapConnectionPool, ldapConfiguration));
}
@Override
public boolean contains(Username name) throws UsersRepositoryException {
- return getUserByName(name).isPresent();
+ try {
+ return Mono.fromCallable(() -> doContains(name))
+ .retryWhen(ldapConfiguration.retrySpec())
+ .block();
+ } catch (Exception e) {
+ if (e.getCause() instanceof UsersRepositoryException) {
+ throw (UsersRepositoryException) e.getCause();
+ }
+ throw new UsersRepositoryException("Unable to check user existence
from ldap", e);
+ }
+ }
+
+ private boolean doContains(Username name) throws LDAPException {
+ return doGetUserByName(name).isPresent();
}
@Override
public int countUsers() throws UsersRepositoryException {
try {
- return Math.toIntExact(getValidUsers().stream()
- .map(Throwing.function(this::buildUser).sneakyThrow())
- .flatMap(Optional::stream)
- .count());
- } catch (NamingException e) {
+ return Mono.fromCallable(() -> doCountUsers())
+ .retryWhen(ldapConfiguration.retrySpec())
+ .block();
+ } catch (Exception e) {
+ if (e.getCause() instanceof UsersRepositoryException) {
+ throw (UsersRepositoryException) e.getCause();
+ }
throw new UsersRepositoryException("Unable to retrieve user count
from ldap", e);
}
}
+ private int doCountUsers() throws LDAPException {
+ return Math.toIntExact(getValidUsers().stream()
+ .map(Throwing.function(this::buildUser).sneakyThrow())
Review comment:
n+1 ldap requests to get the number of users?
##########
File path:
server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -209,29 +171,33 @@ 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> getAllUsersDNFromLDAP() throws LDAPException {
+ SearchRequest searchRequest = new
SearchRequest(ldapConfiguration.getUserBase(),
+ SearchScope.SUB,
+ createFilter(),
Review comment:
it's at usage, I think you don't need to create it for each request.
##########
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:
still hardcoded?
##########
File path:
server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyLDAPUsersDAO.java
##########
@@ -120,61 +96,47 @@ 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();
+
+ LDAPConnectionOptions connectionOptions = new LDAPConnectionOptions();
+
connectionOptions.setConnectTimeoutMillis(ldapConfiguration.getConnectionTimeout());
+
connectionOptions.setResponseTimeoutMillis(ldapConfiguration.getReadTimeout());
+
+ 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);
}
- protected void updateLdapContext() throws NamingException {
- ldapContext = computeLdapContext();
+ @PreDestroy
+ void dispose() {
+ ldapConnectionPool.close();
}
- /**
- * 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()) {
+ private Filter createFilter(String username) {
+ Filter specificUserFilter =
Filter.createEqualityFilter(ldapConfiguration.getUserIdAttribute(), username);
+ return Optional.ofNullable(ldapConfiguration.getFilter())
+ .map(Throwing.function(userFilter ->
+ Filter.createANDFilter(objectClassFilter(),
specificUserFilter, Filter.create(userFilter))))
+ .orElseGet(() -> Filter.createANDFilter(objectClassFilter(),
specificUserFilter));
+ }
- @Override
- public Context newDelegate() throws NamingException {
- return new InitialLdapContext(getContextEnvironment(), null);
- }
- };
+ private Filter objectClassFilter() {
+ return Filter.createEqualityFilter("objectClass",
ldapConfiguration.getUserObjectClass());
}
- protected Properties getContextEnvironment() {
- Properties props = new Properties();
- props.put(Context.INITIAL_CONTEXT_FACTORY, INITIAL_CONTEXT_FACTORY);
- props.put(Context.PROVIDER_URL,
Optional.ofNullable(ldapConfiguration.getLdapHost())
- .orElse(""));
- if (Strings.isNullOrEmpty(ldapConfiguration.getCredentials())) {
- props.put(Context.SECURITY_AUTHENTICATION,
LdapConstants.SECURITY_AUTHENTICATION_NONE);
- } else {
- props.put(Context.SECURITY_AUTHENTICATION,
LdapConstants.SECURITY_AUTHENTICATION_SIMPLE);
- props.put(Context.SECURITY_PRINCIPAL,
Optional.ofNullable(ldapConfiguration.getPrincipal())
- .orElse(""));
- props.put(Context.SECURITY_CREDENTIALS,
ldapConfiguration.getCredentials());
- }
- // The following properties are specific to
com.sun.jndi.ldap.LdapCtxFactory
- props.put(PROPERTY_NAME_CONNECTION_POOL,
String.valueOf(ldapConfiguration.useConnectionPool()));
- if (ldapConfiguration.getConnectionTimeout() > -1) {
- props.put(PROPERTY_NAME_CONNECT_TIMEOUT,
String.valueOf(ldapConfiguration.getConnectionTimeout()));
- }
- if (ldapConfiguration.getReadTimeout() > -1) {
- props.put(PROPERTY_NAME_READ_TIMEOUT,
Integer.toString(ldapConfiguration.getReadTimeout()));
- }
- return props;
+ private Filter createFilter() {
+ return Optional.ofNullable(ldapConfiguration.getFilter())
+ .map(Throwing.function(userFilter ->
Filter.createANDFilter(objectClassFilter(), Filter.create(userFilter))))
Review comment:
is it throwing at usage or at startup? Maybe you could build the Filter
at initialization else?
--
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
