This is an automated email from the ASF dual-hosted git repository. rcordier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit ffcbce782b52b409420d3209186c74f4d52a0c87 Author: Benoit Tellier <btell...@linagora.com> AuthorDate: Thu Mar 5 10:41:16 2020 +0700 JAMES-3087 LDAP user listing should filter out users without id field Before that a NPE was returned. --- .../user/ldap/ReadOnlyUsersLDAPRepository.java | 20 +++-- .../james/user/ldap/LdapGenericContainer.java | 13 ++- .../ReadOnlyUsersLDAPRepositoryInvalidDnTest.java | 96 ++++++++++++++++++++++ .../user/ldap/ReadOnlyUsersLDAPRepositoryTest.java | 66 ++++++++++----- .../test/resources/invalid/ldif-files/Dockerfile | 3 + .../resources/invalid/ldif-files/populate.ldif | 11 +++ 6 files changed, 180 insertions(+), 29 deletions(-) diff --git a/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java b/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java index ebbd794..e9af7ac 100644 --- a/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java +++ b/server/data/data-ldap/src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java @@ -54,12 +54,14 @@ import org.apache.james.user.api.UsersRepository; import org.apache.james.user.api.UsersRepositoryException; import org.apache.james.user.api.model.User; import org.apache.james.user.ldap.api.LdapConstants; +import org.apache.james.util.OptionalUtils; import org.apache.james.util.retry.DoublingRetrySchedule; import org.apache.james.util.retry.api.RetrySchedule; import org.apache.james.util.retry.naming.ldap.RetryingLdapContext; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.github.fge.lambdas.Throwing; import com.github.steveash.guavate.Guavate; import com.google.common.base.Strings; @@ -450,8 +452,8 @@ public class ReadOnlyUsersLDAPRepository implements UsersRepository, Configurabl List<ReadOnlyLDAPUser> results = new ArrayList<>(); for (String userDN : userDNs) { - ReadOnlyLDAPUser user = buildUser(userDN); - results.add(user); + Optional<ReadOnlyLDAPUser> user = buildUser(userDN); + user.ifPresent(results::add); } return results; @@ -518,10 +520,13 @@ public class ReadOnlyUsersLDAPRepository implements UsersRepository, Configurabl * @throws NamingException * Propagated by the underlying LDAP communication layer. */ - private ReadOnlyLDAPUser buildUser(String userDN) throws NamingException { + private Optional<ReadOnlyLDAPUser> buildUser(String userDN) throws NamingException { Attributes userAttributes = ldapContext.getAttributes(userDN); - Attribute userName = userAttributes.get(ldapConfiguration.getUserIdAttribute()); - return new ReadOnlyLDAPUser(Username.of(userName.get().toString()), userDN, ldapContext); + 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)); } @Override @@ -537,7 +542,10 @@ public class ReadOnlyUsersLDAPRepository implements UsersRepository, Configurabl @Override public int countUsers() throws UsersRepositoryException { try { - return getValidUsers().size(); + return Math.toIntExact(getValidUsers().stream() + .map(Throwing.function(this::buildUser).sneakyThrow()) + .flatMap(OptionalUtils::toStream) + .count()); } catch (NamingException e) { LOGGER.error("Unable to retrieve user count from ldap", e); throw new UsersRepositoryException("Unable to retrieve user count from ldap", e); diff --git a/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/LdapGenericContainer.java b/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/LdapGenericContainer.java index 050fe4f..36133f9 100644 --- a/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/LdapGenericContainer.java +++ b/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/LdapGenericContainer.java @@ -18,6 +18,8 @@ ****************************************************************/ package org.apache.james.user.ldap; +import java.util.Optional; + import org.apache.james.util.docker.DockerContainer; import org.apache.james.util.docker.RateLimiters; import org.junit.rules.ExternalResource; @@ -36,13 +38,18 @@ public class LdapGenericContainer extends ExternalResource { } public static class Builder { - + private Optional<String> dockerFilePrefix = Optional.empty(); private String domain; private String password; private Builder() { } + public Builder dockerFilePrefix(String prefix) { + this.dockerFilePrefix = Optional.of(prefix); + return this; + } + public Builder domain(String domain) { this.domain = domain; return this; @@ -62,8 +69,8 @@ public class LdapGenericContainer extends ExternalResource { private DockerContainer createContainer() { return DockerContainer.fromDockerfile( new ImageFromDockerfile() - .withFileFromClasspath("populate.ldif", "ldif-files/populate.ldif") - .withFileFromClasspath("Dockerfile", "ldif-files/Dockerfile")) + .withFileFromClasspath("populate.ldif", dockerFilePrefix.orElse("") + "ldif-files/populate.ldif") + .withFileFromClasspath("Dockerfile", dockerFilePrefix.orElse("") + "ldif-files/Dockerfile")) .withAffinityToContainer() .withEnv("SLAPD_DOMAIN", domain) .withEnv("SLAPD_PASSWORD", password) diff --git a/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepositoryInvalidDnTest.java b/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepositoryInvalidDnTest.java new file mode 100644 index 0000000..33fd239 --- /dev/null +++ b/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepositoryInvalidDnTest.java @@ -0,0 +1,96 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you under the Apache License, Version 2.0 (the * + * "License"); you may not use this file except in compliance * + * with the License. You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, * + * software distributed under the License is distributed on an * + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * + * KIND, either express or implied. See the License for the * + * specific language governing permissions and limitations * + * under the License. * + ****************************************************************/ + +package org.apache.james.user.ldap; + +import static org.apache.james.user.ldap.DockerLdapSingleton.ADMIN_PASSWORD; +import static org.apache.james.user.ldap.DockerLdapSingleton.DOMAIN; +import static org.apache.james.user.ldap.DockerLdapSingleton.JAMES_USER; +import static org.apache.james.user.ldap.ReadOnlyUsersLDAPRepositoryTest.ldapRepositoryConfigurationWithVirtualHosting; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +import org.apache.commons.configuration2.HierarchicalConfiguration; +import org.apache.commons.configuration2.plist.PropertyListConfiguration; +import org.apache.commons.configuration2.tree.ImmutableNode; +import org.apache.james.core.Username; +import org.apache.james.domainlist.api.DomainList; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.google.common.collect.ImmutableList; + +class ReadOnlyUsersLDAPRepositoryInvalidDnTest { + static LdapGenericContainer ldapContainer = LdapGenericContainer.builder() + .dockerFilePrefix("invalid/") + .domain(DOMAIN) + .password(ADMIN_PASSWORD) + .build(); + + DomainList domainList; + private ReadOnlyUsersLDAPRepository ldapRepository; + + @BeforeAll + static void setUpAll() { + ldapContainer.start(); + } + + @AfterAll + static void afterAll() { + ldapContainer.start(); + } + + @BeforeEach + void setUp() throws Exception { + domainList = mock(DomainList.class); + ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting(ldapContainer)); + } + + @Test + void listShouldFilterOutUsersWithoutIdField() throws Exception { + assertThat(ImmutableList.copyOf(ldapRepository.list())) + .isEmpty(); + } + + @Test + void getUserByNameShouldReturnNullWhenNoIdField() throws Exception { + assertThat(ldapRepository.getUserByName(JAMES_USER)).isNull(); + } + + @Test + void containsShouldReturnFalseWhenNoIdField() throws Exception { + assertThat(ldapRepository.contains(JAMES_USER)).isFalse(); + } + + @Test + void contShouldReturnZeroWhenInvalidUser() throws Exception { + assertThat(ldapRepository.countUsers()).isEqualTo(0); + } + + private ReadOnlyUsersLDAPRepository startUsersRepository(HierarchicalConfiguration<ImmutableNode> ldapRepositoryConfiguration) throws Exception { + ReadOnlyUsersLDAPRepository ldapRepository = new ReadOnlyUsersLDAPRepository(domainList); + ldapRepository.configure(ldapRepositoryConfiguration); + ldapRepository.init(); + return ldapRepository; + } +} diff --git a/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepositoryTest.java b/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepositoryTest.java index fd26257..6b22f32 100644 --- a/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepositoryTest.java +++ b/server/data/data-ldap/src/test/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepositoryTest.java @@ -33,12 +33,16 @@ import org.apache.commons.configuration2.plist.PropertyListConfiguration; import org.apache.commons.configuration2.tree.ImmutableNode; import org.apache.james.core.Username; import org.apache.james.domainlist.api.DomainList; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.google.common.collect.ImmutableList; + class ReadOnlyUsersLDAPRepositoryTest { static final Logger LOGGER = LoggerFactory.getLogger(ReadOnlyUsersLDAPRepositoryTest.class); @@ -46,8 +50,23 @@ class ReadOnlyUsersLDAPRepositoryTest { static final Username UNKNOWN = Username.of("unknown"); static final String BAD_PASSWORD = "badpassword"; + static LdapGenericContainer ldapContainer = LdapGenericContainer.builder() + .domain(DOMAIN) + .password(ADMIN_PASSWORD) + .build(); + DomainList domainList; + @BeforeAll + static void setUpAll() { + ldapContainer.start(); + } + + @AfterAll + static void afterAll() { + ldapContainer.start(); + } + @BeforeEach void setUp() { domainList = mock(DomainList.class); @@ -59,14 +78,14 @@ class ReadOnlyUsersLDAPRepositoryTest { @Test void supportVirtualHostingShouldReturnFalseByDefault() throws Exception { ReadOnlyUsersLDAPRepository usersLDAPRepository = new ReadOnlyUsersLDAPRepository(domainList); - usersLDAPRepository.configure(ldapRepositoryConfiguration()); + usersLDAPRepository.configure(ldapRepositoryConfiguration(ldapContainer)); assertThat(usersLDAPRepository.supportVirtualHosting()).isFalse(); } @Test void supportVirtualHostingShouldReturnTrueWhenReportedInConfig() throws Exception { - HierarchicalConfiguration<ImmutableNode> configuration = ldapRepositoryConfiguration(); + HierarchicalConfiguration<ImmutableNode> configuration = ldapRepositoryConfiguration(ldapContainer); configuration.addProperty(ReadOnlyUsersLDAPRepository.SUPPORTS_VIRTUAL_HOSTING, "true"); ReadOnlyUsersLDAPRepository usersLDAPRepository = new ReadOnlyUsersLDAPRepository(domainList); @@ -77,7 +96,7 @@ class ReadOnlyUsersLDAPRepositoryTest { @Test void supportVirtualHostingShouldReturnFalseWhenReportedInConfig() throws Exception { - HierarchicalConfiguration<ImmutableNode> configuration = ldapRepositoryConfiguration(); + HierarchicalConfiguration<ImmutableNode> configuration = ldapRepositoryConfiguration(ldapContainer); configuration.addProperty(ReadOnlyUsersLDAPRepository.SUPPORTS_VIRTUAL_HOSTING, "false"); ReadOnlyUsersLDAPRepository usersLDAPRepository = new ReadOnlyUsersLDAPRepository(domainList); @@ -88,7 +107,7 @@ class ReadOnlyUsersLDAPRepositoryTest { @Test void configureShouldThrowOnNonBooleanValueForSupportsVirtualHosting() { - HierarchicalConfiguration<ImmutableNode> configuration = ldapRepositoryConfiguration(); + HierarchicalConfiguration<ImmutableNode> configuration = ldapRepositoryConfiguration(ldapContainer); configuration.addProperty(ReadOnlyUsersLDAPRepository.SUPPORTS_VIRTUAL_HOSTING, "bad"); ReadOnlyUsersLDAPRepository usersLDAPRepository = new ReadOnlyUsersLDAPRepository(domainList); @@ -103,37 +122,44 @@ class ReadOnlyUsersLDAPRepositoryTest { @Test void knownUserShouldBeAbleToLogInWhenPasswordIsCorrect() throws Exception { - ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfiguration()); + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfiguration(ldapContainer)); assertThat(ldapRepository.test(JAMES_USER, PASSWORD)).isTrue(); } @Test void knownUserShouldNotBeAbleToLogInWhenPasswordIsNotCorrect() throws Exception { - ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfiguration()); + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfiguration(ldapContainer)); assertThat(ldapRepository.test(JAMES_USER, BAD_PASSWORD)).isFalse(); } @Test void unknownUserShouldNotBeAbleToLogIn() throws Exception { - ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfiguration()); + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfiguration(ldapContainer)); assertThat(ldapRepository.test(UNKNOWN, BAD_PASSWORD)).isFalse(); } @Test void unknownUserShouldNotBeAbleToLogInWhenPasswordIsCorrect() throws Exception { - ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfiguration()); + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfiguration(ldapContainer)); assertThat(ldapRepository.test(UNKNOWN, PASSWORD)).isFalse(); } @Test void knownUserShouldBeAbleToLogInWhenPasswordIsCorrectWithVirtualHosting() throws Exception { - ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting()); + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting(ldapContainer)); assertThat(ldapRepository.test(JAMES_USER_MAIL, PASSWORD)).isTrue(); } @Test + void testShouldListUsers() throws Exception { + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting(ldapContainer)); + assertThat(ImmutableList.copyOf(ldapRepository.list())) + .containsOnly(JAMES_USER_MAIL); + } + + @Test void testShouldStillWorkAfterRestartingLDAP() throws Exception { - ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting()); + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting(ldapContainer)); ldapRepository.test(JAMES_USER_MAIL, PASSWORD); DockerLdapSingleton.ldapContainer.pause(); @@ -149,19 +175,19 @@ class ReadOnlyUsersLDAPRepositoryTest { @Test void knownUserShouldNotBeAbleToLogInWhenPasswordIsNotCorrectWithVirtualHosting() throws Exception { - ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting()); + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting(ldapContainer)); assertThat(ldapRepository.test(JAMES_USER, BAD_PASSWORD)).isFalse(); } @Test void unknownUserShouldNotBeAbleToLogInWithVirtualHosting() throws Exception { - ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting()); + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting(ldapContainer)); assertThat(ldapRepository.test(UNKNOWN, BAD_PASSWORD)).isFalse(); } @Test void unknownUserShouldNotBeAbleToLogInWhenPasswordIsCorrectWithVirtualHosting() throws Exception { - ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting()); + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting(ldapContainer)); assertThat(ldapRepository.test(UNKNOWN, PASSWORD)).isFalse(); } @@ -169,19 +195,19 @@ class ReadOnlyUsersLDAPRepositoryTest { void specialCharacterInUserInputShouldBeSanitized() throws Exception { Username patternMatchingMultipleUsers = Username.of("j*"); - ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting()); + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting(ldapContainer)); assertThat(ldapRepository.test(patternMatchingMultipleUsers, PASSWORD)).isFalse(); } @Test void containsWithGetUserShouldBeTrue() throws Exception { - ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfiguration()); + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfiguration(ldapContainer)); assertThat(ldapRepository.contains(ldapRepository.getUser(JAMES_USER_MAIL.asMailAddress()))).isTrue(); } @Test void containsWithGetUserShouldBeTrueWithVirtualHosting() throws Exception { - ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting()); + ReadOnlyUsersLDAPRepository ldapRepository = startUsersRepository(ldapRepositoryConfigurationWithVirtualHosting(ldapContainer)); assertThat(ldapRepository.contains(ldapRepository.getUser(JAMES_USER_MAIL.asMailAddress()))).isTrue(); } @@ -193,9 +219,9 @@ class ReadOnlyUsersLDAPRepositoryTest { } } - private static HierarchicalConfiguration<ImmutableNode> ldapRepositoryConfiguration() { + private HierarchicalConfiguration<ImmutableNode> ldapRepositoryConfiguration(LdapGenericContainer ldapContainer) { PropertyListConfiguration configuration = new PropertyListConfiguration(); - configuration.addProperty("[@ldapHost]", DockerLdapSingleton.ldapContainer.getLdapHost()); + configuration.addProperty("[@ldapHost]", ldapContainer.getLdapHost()); configuration.addProperty("[@principal]", "cn=admin,dc=james,dc=org"); configuration.addProperty("[@credentials]", ADMIN_PASSWORD); configuration.addProperty("[@userBase]", "ou=People,dc=james,dc=org"); @@ -210,9 +236,9 @@ class ReadOnlyUsersLDAPRepositoryTest { return configuration; } - private static HierarchicalConfiguration<ImmutableNode> ldapRepositoryConfigurationWithVirtualHosting() { + static HierarchicalConfiguration<ImmutableNode> ldapRepositoryConfigurationWithVirtualHosting(LdapGenericContainer ldapContainer) { PropertyListConfiguration configuration = new PropertyListConfiguration(); - configuration.addProperty("[@ldapHost]", DockerLdapSingleton.ldapContainer.getLdapHost()); + configuration.addProperty("[@ldapHost]", ldapContainer.getLdapHost()); configuration.addProperty("[@principal]", "cn=admin,dc=james,dc=org"); configuration.addProperty("[@credentials]", ADMIN_PASSWORD); configuration.addProperty("[@userBase]", "ou=People,dc=james,dc=org"); diff --git a/server/data/data-ldap/src/test/resources/invalid/ldif-files/Dockerfile b/server/data/data-ldap/src/test/resources/invalid/ldif-files/Dockerfile new file mode 100644 index 0000000..d889a35 --- /dev/null +++ b/server/data/data-ldap/src/test/resources/invalid/ldif-files/Dockerfile @@ -0,0 +1,3 @@ +FROM dinkel/openldap:latest + +COPY populate.ldif /etc/ldap/prepopulate/prepop.ldif diff --git a/server/data/data-ldap/src/test/resources/invalid/ldif-files/populate.ldif b/server/data/data-ldap/src/test/resources/invalid/ldif-files/populate.ldif new file mode 100644 index 0000000..9376a6c --- /dev/null +++ b/server/data/data-ldap/src/test/resources/invalid/ldif-files/populate.ldif @@ -0,0 +1,11 @@ +dn: ou=people, dc=james,dc=org +ou: people +objectClass: organizationalUnit + +dn: uid=james-user, ou=people, dc=james,dc=org +objectClass: inetOrgPerson +uid: james-user +cn: james-user +sn: james-user +userPassword: secret +description: James user --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org