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/06 06:18:19 UTC
[GitHub] [james-project] chibenwa opened a new pull request #478: JAMES-3594 [POC] Implement ReadOnlyLDAPUsersDAO with UnboundID library
chibenwa opened a new pull request #478:
URL: https://github.com/apache/james-project/pull/478
I had a sunday shot at using [UnboundID](https://docs.ldap.com/ldap-sdk/docs/index.html), ~ 1H between cooking and washing dishes. It went surprisingly well!
Cc @rouazana
All tests are passing.
Remains to implement:
- [ ] Group restrictions
- [ ] Retries
--
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
[GitHub] [james-project] chibenwa commented on pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #478:
URL: https://github.com/apache/james-project/pull/478#issuecomment-856594253
Without this fix:
```
================================================================================
2021-06-08 07:20:54 260s elapsed
---- Requests ------------------------------------------------------------------
> Global (OK=107714 KO=233 )
> prepare / obtainContinuationToken (OK=10166 KO=43 )
> prepare / obtainAccessToken (OK=8152 KO=187 )
> prepare / getMailboxes (OK=8152 KO=0 )
> prepare / listMessages (OK=8152 KO=0 )
> SelectMailbox / listMessages (OK=22905 KO=0 )
> updateFlags / markAsRead (OK=3194 KO=0 )
> SelectMailbox / getMessages (OK=22902 KO=3 )
> updateFlags / markAsAnswered (OK=903 KO=0 )
> OpenMessage / getMessages (OK=13636 KO=0 )
> InboxHomeLoading / getMailboxes (OK=2261 KO=0 )
> updateFlags / markAsFlagged (OK=463 KO=0 )
> InboxHomeLoading / listMessages (OK=2261 KO=0 )
> InboxHomeLoading / getMessages (OK=2260 KO=0 )
> sendMessage / sendMessages (OK=2307 KO=0 )
---- Errors --------------------------------------------------------------------
> status.find.is(201), but actually found 401 187 (63.61%)
> j.n.s.SSLException: handshake timed out 43 (14.63%)
> obtainAccessToken: Failed to build request: No attribute named 43 (14.63%)
'continuationToken' is defined
> getMailboxes: Failed to build request: No attribute named 'acc 18 ( 6.12%)
essTokenHeader' is defined
> jsonPath($[?(@[0] == 'error')]).find.notExists preparation cra 2 ( 0.68%)
shed: Jodd failed to parse into a valid AST: j.j.JsonException...
> jsonPath($[?(@[0] == 'error')]).find.notExists preparation cra 1 ( 0.34%)
shed: Jodd failed to parse into a valid AST: j.j.JsonException...
---- JmapSendMessages ----------------------------------------------------------
[--------------------------------------------------------------------------] 0%
waiting: 0 / active: 10000 / done: 0
================================================================================
```
Notice the 401 upon high authentication rate...
After the change of library no longer 401 are seen.
--
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
[GitHub] [james-project] rouazana commented on a change in pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
rouazana commented on a change in pull request #478:
URL: https://github.com/apache/james-project/pull/478#discussion_r648260292
##########
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:
This parameter seems nice, maybe we could also use it in LinID cc @remk
You could add the matching tests to check it is doing what you expect?
--
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
[GitHub] [james-project] chibenwa commented on pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #478:
URL: https://github.com/apache/james-project/pull/478#issuecomment-859243102
(Just rebased, I will wait a green build before merge)
--
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
[GitHub] [james-project] chibenwa commented on pull request #478: JAMES-3594 [POC] Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #478:
URL: https://github.com/apache/james-project/pull/478#issuecomment-855356694
Preliminary performance results of UnboundID usage:
## Before
## After
No longer thread and connection hungry, three time faster...
--
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
[GitHub] [james-project] chibenwa commented on a change in pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #478:
URL: https://github.com/apache/james-project/pull/478#discussion_r647895306
##########
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:
Just a mis-understanding of how the library is working, that is all...
--
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
[GitHub] [james-project] chibenwa commented on a change in pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #478:
URL: https://github.com/apache/james-project/pull/478#discussion_r648829831
##########
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:
Yeah but their API is not strongly typed... Hence I did not find it.
--
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
[GitHub] [james-project] rouazana commented on a change in pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] rouazana commented on a change in pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [james-project] chibenwa commented on a change in pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #478:
URL: https://github.com/apache/james-project/pull/478#discussion_r647894900
##########
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:
Let's make it configurable with a `poolSize` argument
##########
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:
Nope
--
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
[GitHub] [james-project] chibenwa commented on a change in pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #478:
URL: https://github.com/apache/james-project/pull/478#discussion_r647897188
##########
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:
Upon errors
--
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
[GitHub] [james-project] chibenwa merged pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
chibenwa merged pull request #478:
URL: https://github.com/apache/james-project/pull/478
--
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
[GitHub] [james-project] chibenwa commented on pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #478:
URL: https://github.com/apache/james-project/pull/478#issuecomment-856400817
Following a discusion with @rouazana, he expressed concerns regarding the lack of tests for group/role restrictions.
As such, I propose to consider the feature as experimental, and clearly document it as such...
--
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
[GitHub] [james-project] rouazana commented on a change in pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
rouazana commented on a change in pull request #478:
URL: https://github.com/apache/james-project/pull/478#discussion_r648060705
##########
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:
Which error? You should retry only when connection-related errors.
--
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
[GitHub] [james-project] chibenwa commented on a change in pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #478:
URL: https://github.com/apache/james-project/pull/478#discussion_r648103924
##########
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:
Then honnestly I am more in favour of dropping retries alltogether and only rely on:
```
ldapConnectionPool.setRetryFailedOperationsDueToInvalidConnections(true);
```
Thoughts @rouazana ?
--
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
[GitHub] [james-project] chibenwa commented on pull request #478: JAMES-3594 Implement ReadOnlyLDAPUsersDAO with UnboundID library
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #478:
URL: https://github.com/apache/james-project/pull/478#issuecomment-858289531
I added a few tests regarding custom extra filters...
--
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