You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by bt...@apache.org on 2019/11/18 02:50:38 UTC
[james-project] 04/44: JAMES-2949 JWT authentication strategy
should not accept invalid usernames
This is an automated email from the ASF dual-hosted git repository.
btellier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit a7e583ccb4fa04581786e5b65d0c8154cce530a5
Author: Benoit Tellier <bt...@linagora.com>
AuthorDate: Thu Nov 14 14:16:58 2019 +0700
JAMES-2949 JWT authentication strategy should not accept invalid usernames
Before that we were creating mailbox session for:
- domains that we do not handle
- usernames with invalid virtualHosting
Trusting external systems for Username validity is definitely a harmful strategy
and we should enforce username validity before.
---
.../org/apache/james/user/api/UsersRepository.java | 5 ++
.../user/cassandra/CassandraUsersRepository.java | 2 +-
.../james/user/lib/AbstractUsersRepository.java | 5 +-
.../user/memory/MemoryUsersRepositoryTest.java | 66 +++++++++++++++++++++-
.../jmap/draft/JWTAuthenticationStrategy.java | 19 +++++--
.../jmap/draft/JWTAuthenticationStrategyTest.java | 31 +++++++---
.../jmap/draft/UserProvisioningFilterTest.java | 10 ++--
7 files changed, 117 insertions(+), 21 deletions(-)
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 263d9af..742fbc3 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
@@ -156,4 +156,9 @@ public interface UsersRepository {
*/
boolean isReadOnly();
+ default void assertValid(Username username) throws UsersRepositoryException {
+ if (username.getDomainPart().isPresent() != supportVirtualHosting()) {
+ throw new UsersRepositoryException(username.asString() + " username candidate do not match the virtualHosting strategy");
+ }
+ }
}
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 cd8070b..2cdcc7c 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
@@ -184,7 +184,7 @@ public class CassandraUsersRepository extends AbstractUsersRepository {
@Override
public void addUser(Username username, String password) throws UsersRepositoryException {
- isValidUsername(username);
+ assertValid(username);
doAddUser(username, password);
}
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 55b9ab9..2e26359 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
@@ -70,7 +70,8 @@ public abstract class AbstractUsersRepository implements UsersRepository, Config
this.domainList = domainList;
}
- protected void isValidUsername(Username username) throws UsersRepositoryException {
+ @Override
+ public void assertValid(Username username) throws UsersRepositoryException {
if (supportVirtualHosting()) {
// need a @ in the username
if (!username.hasDomainPart()) {
@@ -97,7 +98,7 @@ public abstract class AbstractUsersRepository implements UsersRepository, Config
public void addUser(Username username, String password) throws UsersRepositoryException {
if (!contains(username)) {
- isValidUsername(username);
+ assertValid(username);
doAddUser(username, password);
} else {
throw new AlreadyExistInUsersRepositoryException("User with username " + username + " already exists!");
diff --git a/server/data/data-memory/src/test/java/org/apache/james/user/memory/MemoryUsersRepositoryTest.java b/server/data/data-memory/src/test/java/org/apache/james/user/memory/MemoryUsersRepositoryTest.java
index 7aa49cb..c0a0aa6 100644
--- a/server/data/data-memory/src/test/java/org/apache/james/user/memory/MemoryUsersRepositoryTest.java
+++ b/server/data/data-memory/src/test/java/org/apache/james/user/memory/MemoryUsersRepositoryTest.java
@@ -19,9 +19,18 @@
package org.apache.james.user.memory;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.james.core.Domain;
+import org.apache.james.core.Username;
+import org.apache.james.dnsservice.api.InMemoryDNSService;
+import org.apache.james.domainlist.memory.MemoryDomainList;
+import org.apache.james.user.api.UsersRepositoryException;
import org.apache.james.user.lib.AbstractUsersRepository;
import org.apache.james.user.lib.AbstractUsersRepositoryTest;
import org.junit.Before;
+import org.junit.Test;
public class MemoryUsersRepositoryTest extends AbstractUsersRepositoryTest {
@@ -32,7 +41,62 @@ public class MemoryUsersRepositoryTest extends AbstractUsersRepositoryTest {
}
@Override
- protected AbstractUsersRepository getUsersRepository() throws Exception {
+ protected AbstractUsersRepository getUsersRepository() {
return MemoryUsersRepository.withVirtualHosting();
}
+
+ @Test
+ public void assertValidShouldThrowWhenDomainPartAndNoVirtualHosting() {
+ MemoryUsersRepository memoryUsersRepository = MemoryUsersRepository.withoutVirtualHosting();
+
+ assertThatThrownBy(() -> memoryUsersRepository.assertValid(Username.of("user@domain.tld")))
+ .isInstanceOf(UsersRepositoryException.class);
+ }
+
+ @Test
+ public void assertValidShouldThrowWhenNoDomainPartAndVirtualHosting() {
+ MemoryUsersRepository memoryUsersRepository = MemoryUsersRepository.withVirtualHosting();
+
+ assertThatThrownBy(() -> memoryUsersRepository.assertValid(Username.of("user")))
+ .isInstanceOf(UsersRepositoryException.class);
+ }
+
+ @Test
+ public void assertValidShouldNotThrowWhenDomainPartAndVirtualHosting() throws Exception {
+ MemoryUsersRepository memoryUsersRepository = MemoryUsersRepository.withVirtualHosting();
+
+ MemoryDomainList domainList = new MemoryDomainList(new InMemoryDNSService()
+ .registerMxRecord("localhost", "127.0.0.1")
+ .registerMxRecord("127.0.0.1", "127.0.0.1"));
+ domainList.setAutoDetect(false);
+ domainList.setAutoDetectIP(false);
+ domainList.addDomain(Domain.of("domain.tld"));
+ memoryUsersRepository.setDomainList(domainList);
+
+ assertThatCode(() -> memoryUsersRepository.assertValid(Username.of("user@domain.tld")))
+ .doesNotThrowAnyException();
+ }
+
+ @Test
+ public void assertValidShouldNotThrowWhenDomainPartAndDomainNotFound() throws Exception {
+ MemoryUsersRepository memoryUsersRepository = MemoryUsersRepository.withVirtualHosting();
+
+ MemoryDomainList domainList = new MemoryDomainList(new InMemoryDNSService()
+ .registerMxRecord("localhost", "127.0.0.1")
+ .registerMxRecord("127.0.0.1", "127.0.0.1"));
+ domainList.setAutoDetect(false);
+ domainList.setAutoDetectIP(false);
+ memoryUsersRepository.setDomainList(domainList);
+
+ assertThatThrownBy(() -> memoryUsersRepository.assertValid(Username.of("user@domain.tld")))
+ .isInstanceOf(UsersRepositoryException.class);
+ }
+
+ @Test
+ public void assertValidShouldNotThrowWhenNoDomainPartAndNoVirtualHosting() {
+ MemoryUsersRepository memoryUsersRepository = MemoryUsersRepository.withoutVirtualHosting();
+
+ assertThatCode(() -> memoryUsersRepository.assertValid(Username.of("user")))
+ .doesNotThrowAnyException();
+ }
}
diff --git a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/JWTAuthenticationStrategy.java b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/JWTAuthenticationStrategy.java
index 2c7063b..7a6dfa3 100644
--- a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/JWTAuthenticationStrategy.java
+++ b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/JWTAuthenticationStrategy.java
@@ -31,6 +31,8 @@ import org.apache.james.jwt.JwtTokenVerifier;
import org.apache.james.mailbox.MailboxManager;
import org.apache.james.mailbox.MailboxSession;
import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
import com.google.common.annotations.VisibleForTesting;
@@ -40,22 +42,31 @@ public class JWTAuthenticationStrategy implements AuthenticationStrategy {
private final JwtTokenVerifier tokenManager;
private final MailboxManager mailboxManager;
private final HeadersAuthenticationExtractor authenticationExtractor;
+ private final UsersRepository usersRepository;
@Inject
@VisibleForTesting
- JWTAuthenticationStrategy(JwtTokenVerifier tokenManager, MailboxManager mailboxManager, HeadersAuthenticationExtractor authenticationExtractor) {
+ JWTAuthenticationStrategy(JwtTokenVerifier tokenManager, MailboxManager mailboxManager, HeadersAuthenticationExtractor authenticationExtractor, UsersRepository usersRepository) {
this.tokenManager = tokenManager;
this.mailboxManager = mailboxManager;
this.authenticationExtractor = authenticationExtractor;
+ this.usersRepository = usersRepository;
}
@Override
public MailboxSession createMailboxSession(HttpServletRequest httpRequest) throws MailboxSessionCreationException, NoValidAuthHeaderException {
Stream<Username> userLoginStream = extractTokensFromAuthHeaders(authenticationExtractor.authHeaders(httpRequest))
- .filter(tokenManager::verify)
- .map(tokenManager::extractLogin)
- .map(Username::of);
+ .filter(tokenManager::verify)
+ .map(tokenManager::extractLogin)
+ .map(Username::of)
+ .peek(username -> {
+ try {
+ usersRepository.assertValid(username);
+ } catch (UsersRepositoryException e) {
+ throw new MailboxSessionCreationException(e);
+ }
+ });
Stream<MailboxSession> mailboxSessionStream = userLoginStream
.map(login -> {
diff --git a/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/JWTAuthenticationStrategyTest.java b/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/JWTAuthenticationStrategyTest.java
index 47db962..08110c0 100644
--- a/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/JWTAuthenticationStrategyTest.java
+++ b/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/JWTAuthenticationStrategyTest.java
@@ -29,7 +29,6 @@ import java.util.stream.Stream;
import javax.servlet.http.HttpServletRequest;
import org.apache.james.core.Username;
-import org.apache.james.jmap.draft.JWTAuthenticationStrategy;
import org.apache.james.jmap.draft.exceptions.MailboxSessionCreationException;
import org.apache.james.jmap.draft.exceptions.NoValidAuthHeaderException;
import org.apache.james.jmap.draft.utils.HeadersAuthenticationExtractor;
@@ -37,12 +36,10 @@ import org.apache.james.jwt.JwtTokenVerifier;
import org.apache.james.mailbox.MailboxManager;
import org.apache.james.mailbox.MailboxSession;
import org.apache.james.mailbox.exception.MailboxException;
-import org.apache.james.user.api.model.User;
-
+import org.apache.james.user.memory.MemoryUsersRepository;
import org.junit.Before;
import org.junit.Test;
-
public class JWTAuthenticationStrategyTest {
private JWTAuthenticationStrategy testee;
@@ -57,13 +54,14 @@ public class JWTAuthenticationStrategyTest {
mockedMailboxManager = mock(MailboxManager.class);
mockAuthenticationExtractor = mock(HeadersAuthenticationExtractor.class);
request = mock(HttpServletRequest.class);
+ MemoryUsersRepository usersRepository = MemoryUsersRepository.withoutVirtualHosting();
- testee = new JWTAuthenticationStrategy(stubTokenVerifier, mockedMailboxManager, mockAuthenticationExtractor);
+ testee = new JWTAuthenticationStrategy(stubTokenVerifier, mockedMailboxManager, mockAuthenticationExtractor, usersRepository);
}
@Test
- public void createMailboxSessionShouldThrowWhenAuthHeaderIsEmpty() throws Exception {
+ public void createMailboxSessionShouldThrowWhenAuthHeaderIsEmpty() {
when(mockAuthenticationExtractor.authHeaders(request))
.thenReturn(Stream.empty());
@@ -91,7 +89,7 @@ public class JWTAuthenticationStrategyTest {
}
@Test
- public void createMailboxSessionShouldReturnEmptyWhenAuthHeaderIsInvalid() throws Exception {
+ public void createMailboxSessionShouldReturnEmptyWhenAuthHeaderIsInvalid() {
when(mockAuthenticationExtractor.authHeaders(request))
.thenReturn(Stream.of("bad"));
@@ -134,4 +132,23 @@ public class JWTAuthenticationStrategyTest {
MailboxSession result = testee.createMailboxSession(request);
assertThat(result).isEqualTo(fakeMailboxSession);
}
+
+ @Test
+ public void createMailboxSessionShouldThrowUponInvalidVirtualHosting() throws Exception {
+ String username = "123456789@domain.tld";
+ String validAuthHeader = "valid";
+ String fakeAuthHeaderWithPrefix = JWTAuthenticationStrategy.AUTHORIZATION_HEADER_PREFIX + validAuthHeader;
+ MailboxSession fakeMailboxSession = mock(MailboxSession.class);
+
+ when(stubTokenVerifier.verify(validAuthHeader)).thenReturn(true);
+ when(stubTokenVerifier.extractLogin(validAuthHeader)).thenReturn(username);
+ when(mockedMailboxManager.createSystemSession(eq(Username.of(username))))
+ .thenReturn(fakeMailboxSession);
+ when(mockAuthenticationExtractor.authHeaders(request))
+ .thenReturn(Stream.of(fakeAuthHeaderWithPrefix));
+
+
+ assertThatThrownBy(() -> testee.createMailboxSession(request))
+ .isInstanceOf(MailboxSessionCreationException.class);
+ }
}
\ No newline at end of file
diff --git a/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/UserProvisioningFilterTest.java b/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/UserProvisioningFilterTest.java
index aa6e6a5..5b03808 100644
--- a/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/UserProvisioningFilterTest.java
+++ b/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/UserProvisioningFilterTest.java
@@ -19,6 +19,7 @@
package org.apache.james.jmap.draft;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
@@ -86,17 +87,14 @@ public class UserProvisioningFilterTest {
}
@Test
- public void filterShouldAddUsernameWhenNoVirtualHostingAndMailboxSessionContainsMail() throws Exception {
+ public void filterShouldFailOnInvalidVirtualHosting() {
usersRepository.setEnableVirtualHosting(false);
MailboxSession mailboxSession = MailboxSessionUtil.create(USERNAME_WITH_DOMAIN);
when(request.getAttribute(AuthenticationFilter.MAILBOX_SESSION))
.thenReturn(mailboxSession);
- sut.doFilter(request, response, chain);
-
- verify(chain).doFilter(request, response);
- assertThat(usersRepository.list()).toIterable()
- .contains(USERNAME);
+ assertThatThrownBy(() -> sut.doFilter(request, response, chain))
+ .hasCauseInstanceOf(UsersRepositoryException.class);
}
@Test
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org