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 4b3aaf5c7703ea89f9a9fe926e4b701fe4f98e49 Author: Benoit Tellier <[email protected]> AuthorDate: Wed Nov 20 11:10:37 2019 +0700 JAMES-2949 Standardize case support for UsersRepository --- .../main/java/org/apache/james/core/Username.java | 6 +- .../org/apache/james/user/api/UsersRepository.java | 10 +- .../user/cassandra/CassandraUsersRepository.java | 10 +- .../cassandra/CassandraUsersRepositoryTest.java | 10 +- .../james/user/jpa/JpaUsersRepositoryTest.java | 4 +- .../user/ldap/ReadOnlyUsersLDAPRepository.java | 10 -- .../james/user/lib/AbstractUsersRepository.java | 11 +- .../user/lib/AbstractUsersRepositoryTest.java | 122 ++++++++++++++++++++- .../james/user/memory/MemoryUsersRepository.java | 4 +- 9 files changed, 151 insertions(+), 36 deletions(-) diff --git a/core/src/main/java/org/apache/james/core/Username.java b/core/src/main/java/org/apache/james/core/Username.java index 73e6aaf..8ae8117 100644 --- a/core/src/main/java/org/apache/james/core/Username.java +++ b/core/src/main/java/org/apache/james/core/Username.java @@ -122,7 +122,11 @@ public class Username { } public String asId() { - return asString().toLowerCase(Locale.US); + return toLowerCase().asString(); + } + + public Username toLowerCase() { + return new Username(localPart.toLowerCase(Locale.US), domainPart); } public boolean equalsAsId(String otherId) { diff --git a/server/data/data-api/src/main/java/org/apache/james/user/api/UsersRepository.java b/server/data/data-api/src/main/java/org/apache/james/user/api/UsersRepository.java index 742fbc3..a5ffdad 100644 --- a/server/data/data-api/src/main/java/org/apache/james/user/api/UsersRepository.java +++ b/server/data/data-api/src/main/java/org/apache/james/user/api/UsersRepository.java @@ -132,11 +132,15 @@ public interface UsersRepository { /** * Returns username to be used for a given MailAddress * - * @param mailAddress * @return Username used by James for this mailAddress - * @throws UsersRepositoryException */ - Username getUser(MailAddress mailAddress) throws UsersRepositoryException; + default Username getUser(MailAddress mailAddress) throws UsersRepositoryException { + if (supportVirtualHosting()) { + return Username.of(mailAddress.asString()).toLowerCase(); + } else { + return Username.of(mailAddress.getLocalPart()).toLowerCase(); + } + } /** * Returns one of the possible mail addresses to be used to send a mail to that user diff --git a/server/data/data-cassandra/src/main/java/org/apache/james/user/cassandra/CassandraUsersRepository.java b/server/data/data-cassandra/src/main/java/org/apache/james/user/cassandra/CassandraUsersRepository.java index f1ac2a8..9f73dfa 100644 --- a/server/data/data-cassandra/src/main/java/org/apache/james/user/cassandra/CassandraUsersRepository.java +++ b/server/data/data-cassandra/src/main/java/org/apache/james/user/cassandra/CassandraUsersRepository.java @@ -84,7 +84,7 @@ public class CassandraUsersRepository extends AbstractUsersRepository { } private PreparedStatement prepareListStatement(Session session) { - return session.prepare(select(REALNAME) + return session.prepare(select(NAME) .from(TABLE_NAME)); } @@ -109,7 +109,7 @@ public class CassandraUsersRepository extends AbstractUsersRepository { } private PreparedStatement prepareGetUserStatement(Session session) { - return session.prepare(select(REALNAME, PASSWORD, ALGORITHM) + return session.prepare(select(NAME, PASSWORD, ALGORITHM) .from(TABLE_NAME) .where(eq(NAME, bindMarker(NAME)))); } @@ -119,7 +119,7 @@ public class CassandraUsersRepository extends AbstractUsersRepository { return executor.executeSingleRow( getUserStatement.bind() .setString(NAME, name.asId())) - .map(row -> new DefaultUser(Username.of(row.getString(REALNAME)), row.getString(PASSWORD), row.getString(ALGORITHM))) + .map(row -> new DefaultUser(Username.of(row.getString(NAME)), row.getString(PASSWORD), row.getString(ALGORITHM))) .blockOptional() .orElse(null); } @@ -145,7 +145,7 @@ public class CassandraUsersRepository extends AbstractUsersRepository { public void removeUser(Username name) throws UsersRepositoryException { boolean executed = executor.executeReturnApplied( removeUserStatement.bind() - .setString(NAME, name.asString())) + .setString(NAME, name.asId())) .block(); if (!executed) { @@ -178,7 +178,7 @@ public class CassandraUsersRepository extends AbstractUsersRepository { @Override public Iterator<Username> list() throws UsersRepositoryException { return executor.executeRows(listStatement.bind()) - .map(row -> row.getString(REALNAME)) + .map(row -> row.getString(NAME)) .map(Username::of) .toIterable() .iterator(); diff --git a/server/data/data-cassandra/src/test/java/org/apache/james/user/cassandra/CassandraUsersRepositoryTest.java b/server/data/data-cassandra/src/test/java/org/apache/james/user/cassandra/CassandraUsersRepositoryTest.java index 0341928..7009cd7 100644 --- a/server/data/data-cassandra/src/test/java/org/apache/james/user/cassandra/CassandraUsersRepositoryTest.java +++ b/server/data/data-cassandra/src/test/java/org/apache/james/user/cassandra/CassandraUsersRepositoryTest.java @@ -19,6 +19,7 @@ package org.apache.james.user.cassandra; +import org.apache.commons.configuration2.BaseHierarchicalConfiguration; import org.apache.james.backends.cassandra.CassandraCluster; import org.apache.james.backends.cassandra.DockerCassandraRule; import org.apache.james.user.lib.AbstractUsersRepository; @@ -48,8 +49,11 @@ public class CassandraUsersRepositoryTest extends AbstractUsersRepositoryTest { cassandra.closeCluster(); } - @Override - protected AbstractUsersRepository getUsersRepository() { - return new CassandraUsersRepository(domainList, cassandra.getConf()); + @Override protected AbstractUsersRepository getUsersRepository() throws Exception { + CassandraUsersRepository cassandraUsersRepository = new CassandraUsersRepository(domainList, cassandra.getConf()); + BaseHierarchicalConfiguration configuration = new BaseHierarchicalConfiguration(); + configuration.addProperty("enableVirtualHosting", "true"); + cassandraUsersRepository.configure(configuration); + return cassandraUsersRepository; } } diff --git a/server/data/data-jpa/src/test/java/org/apache/james/user/jpa/JpaUsersRepositoryTest.java b/server/data/data-jpa/src/test/java/org/apache/james/user/jpa/JpaUsersRepositoryTest.java index f81f3e2..1cff3ee 100644 --- a/server/data/data-jpa/src/test/java/org/apache/james/user/jpa/JpaUsersRepositoryTest.java +++ b/server/data/data-jpa/src/test/java/org/apache/james/user/jpa/JpaUsersRepositoryTest.java @@ -47,7 +47,9 @@ public class JpaUsersRepositoryTest extends AbstractUsersRepositoryTest { protected AbstractUsersRepository getUsersRepository() throws Exception { JPAUsersRepository repos = new JPAUsersRepository(domainList); repos.setEntityManagerFactory(JPA_TEST_CLUSTER.getEntityManagerFactory()); - repos.configure(new BaseHierarchicalConfiguration()); + BaseHierarchicalConfiguration configuration = new BaseHierarchicalConfiguration(); + configuration.addProperty("enableVirtualHosting", "true"); + repos.configure(configuration); return repos; } } 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 812da90..ebbd794 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 @@ -657,16 +657,6 @@ public class ReadOnlyUsersLDAPRepository implements UsersRepository, Configurabl return ldapConfiguration.supportsVirtualHosting(); } - - @Override - public Username getUser(MailAddress mailAddress) { - if (supportVirtualHosting()) { - return Username.of(mailAddress.asString()); - } else { - return Username.of(mailAddress.getLocalPart()); - } - } - @Override public boolean isAdministrator(Username username) { if (ldapConfiguration.getAdministratorId().isPresent()) { diff --git a/server/data/data-library/src/main/java/org/apache/james/user/lib/AbstractUsersRepository.java b/server/data/data-library/src/main/java/org/apache/james/user/lib/AbstractUsersRepository.java index 7dfcd17..2d0cb7f 100644 --- a/server/data/data-library/src/main/java/org/apache/james/user/lib/AbstractUsersRepository.java +++ b/server/data/data-library/src/main/java/org/apache/james/user/lib/AbstractUsersRepository.java @@ -122,15 +122,6 @@ public abstract class AbstractUsersRepository implements UsersRepository, Config */ protected abstract void doAddUser(Username username, String password) throws UsersRepositoryException; - @Override - public Username getUser(MailAddress mailAddress) throws UsersRepositoryException { - if (supportVirtualHosting()) { - return Username.of(mailAddress.asString()); - } else { - return Username.of(mailAddress.getLocalPart()); - } - } - @VisibleForTesting void setAdministratorId(Optional<Username> username) { this.administratorId = username; } @@ -138,7 +129,7 @@ public abstract class AbstractUsersRepository implements UsersRepository, Config @Override public boolean isAdministrator(Username username) throws UsersRepositoryException { if (administratorId.isPresent()) { - return administratorId.get().equals(username); + return administratorId.get().toLowerCase().equals(username.toLowerCase()); } return false; } diff --git a/server/data/data-library/src/test/java/org/apache/james/user/lib/AbstractUsersRepositoryTest.java b/server/data/data-library/src/test/java/org/apache/james/user/lib/AbstractUsersRepositoryTest.java index 0503efb..203f431 100644 --- a/server/data/data-library/src/test/java/org/apache/james/user/lib/AbstractUsersRepositoryTest.java +++ b/server/data/data-library/src/test/java/org/apache/james/user/lib/AbstractUsersRepositoryTest.java @@ -19,6 +19,7 @@ package org.apache.james.user.lib; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.util.ArrayList; import java.util.Iterator; @@ -52,10 +53,12 @@ public abstract class AbstractUsersRepositoryTest { protected abstract AbstractUsersRepository getUsersRepository() throws Exception; private Username user1; + private Username user1CaseVariation; private Username user2; private Username user3; private Username admin; - + private Username adminCaseVariation; + public void setUp() throws Exception { domainList = new SimpleDomainList(); domainList.addDomain(DOMAIN); @@ -63,7 +66,9 @@ public abstract class AbstractUsersRepositoryTest { user1 = login("username"); user2 = login("username2"); user3 = login("username3"); + user1CaseVariation = login("uSeRnaMe"); admin = login("admin"); + adminCaseVariation = login("adMin"); } public void tearDown() throws Exception { @@ -139,6 +144,121 @@ public abstract class AbstractUsersRepositoryTest { //Then assertThat(usersRepository.contains(user2)).isTrue(); } + + @Test + public void containsShouldPreserveCaseVariation() throws UsersRepositoryException { + usersRepository.addUser(user1CaseVariation, "password2"); + + assertThat(usersRepository.contains(user1CaseVariation)).isTrue(); + } + + @Test + public void containsShouldBeCaseInsentive() throws UsersRepositoryException { + usersRepository.addUser(user1CaseVariation, "password2"); + + assertThat(usersRepository.contains(user1)).isTrue(); + } + + @Test + public void containsShouldBeCaseInsentiveWhenOriginalValueLowerCased() throws UsersRepositoryException { + usersRepository.addUser(user1, "password2"); + + assertThat(usersRepository.contains(user1CaseVariation)).isTrue(); + } + + @Test + public void addUserShouldDisableCaseVariationWhenOriginalValueLowerCased() throws UsersRepositoryException { + usersRepository.addUser(user1, "password2"); + + assertThatThrownBy(() -> usersRepository.addUser(user1CaseVariation, "pass")) + .isInstanceOf(UsersRepositoryException.class); + } + + @Test + public void addUserShouldDisableCaseVariation() throws UsersRepositoryException { + usersRepository.addUser(user1CaseVariation, "password2"); + + assertThatThrownBy(() -> usersRepository.addUser(user1, "pass")) + .isInstanceOf(UsersRepositoryException.class); + } + + @Test + public void listShouldReturnLowerCaseUser() throws UsersRepositoryException { + usersRepository.addUser(user1CaseVariation, "password2"); + + assertThat(usersRepository.list()) + .toIterable() + .containsExactly(user1); + } + + @Test + public void removeUserShouldBeCaseInsentiveOnCaseVariationUser() throws UsersRepositoryException { + usersRepository.addUser(user1CaseVariation, "password2"); + + usersRepository.removeUser(user1); + + assertThat(usersRepository.list()) + .toIterable() + .isEmpty(); + } + + @Test + public void removeUserShouldBeCaseInsentive() throws UsersRepositoryException { + usersRepository.addUser(user1, "password2"); + + usersRepository.removeUser(user1CaseVariation); + + assertThat(usersRepository.list()) + .toIterable() + .isEmpty(); + } + + @Test + public void getUserByNameShouldBeCaseInsentive() throws UsersRepositoryException { + usersRepository.addUser(user1, "password2"); + + assertThat(usersRepository.getUserByName(user1CaseVariation).getUserName()) + .isEqualTo(user1); + } + + @Test + public void getUserByNameShouldReturnLowerCaseAddedUser() throws UsersRepositoryException { + usersRepository.addUser(user1CaseVariation, "password2"); + + assertThat(usersRepository.getUserByName(user1).getUserName()) + .isEqualTo(user1); + } + + @Test + public void getUserShouldBeCaseInsentive() throws Exception { + assertThat(usersRepository.getUser(user1CaseVariation.asMailAddress())) + .isEqualTo(user1); + } + + @Test + public void isAdministratorShouldBeCaseInsentive() throws Exception { + usersRepository.setAdministratorId(Optional.of(admin)); + assertThat(usersRepository.isAdministrator(adminCaseVariation)) + .isTrue(); + } + + @Test + public void testShouldBeCaseInsentiveOnCaseVariationUser() throws UsersRepositoryException { + String password = "password2"; + usersRepository.addUser(user1CaseVariation, password); + + assertThat(usersRepository.test(user1, password)) + .isTrue(); + } + + @Test + public void testShouldBeCaseInsentive() throws UsersRepositoryException { + String password = "password2"; + usersRepository.addUser(user1, password); + + assertThat(usersRepository.test(user1CaseVariation, password)) + .isTrue(); + } @Test public void addUserShouldAddAUserWhenNotEmptyRepository() throws UsersRepositoryException { diff --git a/server/data/data-memory/src/main/java/org/apache/james/user/memory/MemoryUsersRepository.java b/server/data/data-memory/src/main/java/org/apache/james/user/memory/MemoryUsersRepository.java index 7a4de59..a658f3d 100644 --- a/server/data/data-memory/src/main/java/org/apache/james/user/memory/MemoryUsersRepository.java +++ b/server/data/data-memory/src/main/java/org/apache/james/user/memory/MemoryUsersRepository.java @@ -71,8 +71,8 @@ public class MemoryUsersRepository extends AbstractUsersRepository { } @Override - protected void doAddUser(Username username, String password) throws UsersRepositoryException { - DefaultUser user = new DefaultUser(username, algo); + protected void doAddUser(Username username, String password) { + DefaultUser user = new DefaultUser(username.toLowerCase(), algo); user.setPassword(password); userByName.put(username.asId(), user); } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
