You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by hq...@apache.org on 2026/08/10 02:47:58 UTC
(james-project) branch master updated (c779705ec1 -> 9972dfe3c6)
This is an automated email from the ASF dual-hosted git repository.
quantranhong1999 pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
from c779705ec1 Bump org.postgresql:postgresql in /server/apps/scaling-pulsar-smtp
new a5465b3219 JAMES-4210 POP3: add a test to verify SASL exchange closure upon client disconnection
new 7711ad16f8 JAMES-4210 SMTP: should close SASL exchange upon client disconnection
new c49ef4d1ba JAMES-4210 IMAP: close SASL exchanges upon disconnection
new 9972dfe3c6 JAMES-4210 Drop SaslExchange.abort
The 4 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../james/protocols/api/sasl/SaslExchange.java | 7 +-
.../james/protocols/api/sasl/SaslExchangeTest.java | 98 -------------
.../api/sasl/SaslMechanismContractTest.java | 18 +--
.../imap/api/process/ImapSaslExchangeTracker.java | 109 +++++++++++++++
.../imap/processor/AuthenticateProcessor.java | 32 ++---
.../imap/processor/AuthenticateProcessorTest.java | 12 --
.../protocols/smtp/core/esmtp/AuthCmdHandler.java | 44 ++++--
.../apache/james/utils/FixedNameSaslMechanism.java | 4 -
.../netty/ImapChannelUpstreamHandler.java | 12 ++
.../imapserver/netty/AbstractIMAPServerTest.java | 94 +++++++++++--
.../netty/IMAPServerSaslExchangeLifecycleTest.java | 153 +++++++++++++++++++++
.../james/pop3server/core/AuthCmdHandler.java | 17 +--
.../apache/james/pop3server/POP3ServerTest.java | 46 ++++++-
.../org/apache/james/smtpserver/SMTPSaslTest.java | 84 +++++++++++
14 files changed, 527 insertions(+), 203 deletions(-)
delete mode 100644 protocols/api/src/test/java/org/apache/james/protocols/api/sasl/SaslExchangeTest.java
create mode 100644 protocols/imap/src/main/java/org/apache/james/imap/api/process/ImapSaslExchangeTracker.java
create mode 100644 server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerSaslExchangeLifecycleTest.java
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
(james-project) 02/04: JAMES-4210 SMTP: should close SASL exchange upon client disconnection
Posted by hq...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
quantranhong1999 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 7711ad16f8d44b8bbce59155a6cc6ad58ea362c2
Author: Quan Tran <hq...@linagora.com>
AuthorDate: Tue Jul 28 09:44:30 2026 +0700
JAMES-4210 SMTP: should close SASL exchange upon client disconnection
---
.../protocols/smtp/core/esmtp/AuthCmdHandler.java | 46 ++++++++--
.../org/apache/james/smtpserver/SMTPSaslTest.java | 99 ++++++++++++++++++++++
2 files changed, 136 insertions(+), 9 deletions(-)
diff --git a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/AuthCmdHandler.java b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/AuthCmdHandler.java
index eb649ae12f..8246e8d506 100644
--- a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/AuthCmdHandler.java
+++ b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/AuthCmdHandler.java
@@ -29,9 +29,11 @@ import java.util.Locale;
import java.util.Optional;
import org.apache.james.core.Username;
+import org.apache.james.protocols.api.ProtocolSession;
import org.apache.james.protocols.api.Request;
import org.apache.james.protocols.api.Response;
import org.apache.james.protocols.api.handler.CommandHandler;
+import org.apache.james.protocols.api.handler.DisconnectHandler;
import org.apache.james.protocols.api.handler.ExtensibleHandler;
import org.apache.james.protocols.api.handler.LineHandler;
import org.apache.james.protocols.api.handler.WiringException;
@@ -67,12 +69,14 @@ import com.google.common.collect.ImmutableSet;
* Authentication is delegated to configured SASL mechanisms.
*/
public class AuthCmdHandler
- implements CommandHandler<SMTPSession>, EhloExtension, ExtensibleHandler, MailParametersHook {
+ implements CommandHandler<SMTPSession>, DisconnectHandler<SMTPSession>, EhloExtension, ExtensibleHandler, MailParametersHook {
private static final Collection<String> COMMANDS = ImmutableSet.of("AUTH");
private static final Logger LOGGER = LoggerFactory.getLogger(AuthCmdHandler.class);
private static final Logger AUTHENTICATION_DEDICATED_LOGGER = LoggerFactory.getLogger("org.apache.james.protocols.smtp.AUTHENTICATION");
private static final String[] MAIL_PARAMS = { "AUTH" };
private static final String AUTH_TYPES_DELIMITER = " ";
+ private static final ProtocolSession.AttachmentKey<SaslExchange> ACTIVE_SASL_EXCHANGE =
+ ProtocolSession.AttachmentKey.of("SMTP_ACTIVE_SASL_EXCHANGE", SaslExchange.class);
private static final Response AUTH_ABORTED = new SMTPResponse(SMTPRetCode.SYNTAX_ERROR_ARGUMENTS, DSNStatus.getStatus(DSNStatus.PERMANENT, DSNStatus.SECURITY_AUTH) + " Authentication aborted").immutable();
private static final Response ALREADY_AUTH = new SMTPResponse(SMTPRetCode.BAD_SEQUENCE, DSNStatus.getStatus(DSNStatus.PERMANENT,DSNStatus.DELIVERY_OTHER) + " User has previously authenticated. "
@@ -136,6 +140,7 @@ public class AuthCmdHandler
try {
SaslExchange exchange = startExchange(maybeMechanism.get(), SaslCodec.initialRequest(authType, initialResponse));
+ registerExchange(session, exchange);
return handleFirstSaslStep(session, authType, exchange);
} catch (IllegalArgumentException e) {
LOGGER.info("Could not decode parameters for AUTH {}", authType, e);
@@ -153,7 +158,7 @@ public class AuthCmdHandler
return handleTerminalSaslStep(session, authType, exchange, step, () -> {
});
} catch (RuntimeException e) {
- exchange.close();
+ closeExchange(session);
throw e;
}
}
@@ -182,11 +187,11 @@ public class AuthCmdHandler
} catch (IllegalArgumentException e) {
LOGGER.info("Could not decode parameters for AUTH {}", authType, e);
session.popLineHandler();
- exchange.close();
+ closeExchange(session);
return new SMTPResponse(SMTPRetCode.SYNTAX_ERROR_ARGUMENTS, "Could not decode parameters for AUTH " + authType);
} catch (RuntimeException e) {
session.popLineHandler();
- exchange.close();
+ closeExchange(session);
throw e;
}
}
@@ -195,7 +200,7 @@ public class AuthCmdHandler
return (session, line) -> {
if (SaslCodec.isAbort(line)) {
session.popLineHandler();
- exchange.abort();
+ abortExchange(session);
return AUTH_ABORTED;
}
return handleSaslContinuation(session, authType, exchange, new String(line, session.getCharset()));
@@ -213,7 +218,7 @@ public class AuthCmdHandler
try {
return applySaslSuccess(session, authType, exchange, success);
} finally {
- exchange.close();
+ closeExchange(session);
}
}
@@ -229,7 +234,7 @@ public class AuthCmdHandler
byte[] bytes = line.getBytes(session.getCharset());
if (SaslCodec.isAbort(bytes)) {
aborted = true;
- exchange.abort();
+ abortExchange(session);
return AUTH_ABORTED;
}
if (!SaslCodec.isEmptyClientResponse(bytes)) {
@@ -238,7 +243,7 @@ public class AuthCmdHandler
return applySaslSuccess(session, authType, exchange, success);
} finally {
if (!aborted) {
- exchange.close();
+ closeExchange(session);
}
}
}
@@ -283,7 +288,30 @@ public class AuthCmdHandler
case INVALID_CREDENTIALS, AUTHENTICATION_FAILED, USER_DOES_NOT_EXIST, DELEGATION_FORBIDDEN -> AUTH_FAILED;
});
} finally {
- exchange.close();
+ closeExchange(session);
+ }
+ }
+
+ private void registerExchange(SMTPSession session, SaslExchange exchange) {
+ session.setAttachment(ACTIVE_SASL_EXCHANGE, exchange, ProtocolSession.State.Connection)
+ .ifPresent(SaslExchange::close);
+ }
+
+ private void closeExchange(SMTPSession session) {
+ session.removeAttachment(ACTIVE_SASL_EXCHANGE, ProtocolSession.State.Connection)
+ .ifPresent(SaslExchange::close);
+ }
+
+ private void abortExchange(SMTPSession session) {
+ session.removeAttachment(ACTIVE_SASL_EXCHANGE, ProtocolSession.State.Connection)
+ .ifPresent(SaslExchange::abort);
+ }
+
+ @Override
+ public void onDisconnect(SMTPSession session) {
+ if (session != null) {
+ session.removeAttachment(ACTIVE_SASL_EXCHANGE, ProtocolSession.State.Connection)
+ .ifPresent(SaslExchange::close);
}
}
diff --git a/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SMTPSaslTest.java b/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SMTPSaslTest.java
index 9a71859f95..5d67c858c1 100644
--- a/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SMTPSaslTest.java
+++ b/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SMTPSaslTest.java
@@ -26,8 +26,10 @@ import static org.assertj.core.api.Assertions.assertThat;
import java.io.IOException;
import java.net.InetSocketAddress;
import java.nio.charset.StandardCharsets;
+import java.time.Duration;
import java.util.Base64;
import java.util.Optional;
+import java.util.concurrent.atomic.AtomicInteger;
import org.apache.commons.configuration2.BaseHierarchicalConfiguration;
import org.apache.commons.configuration2.HierarchicalConfiguration;
@@ -55,6 +57,7 @@ import org.apache.james.protocols.sasl.plain.PlainSaslMechanism;
import org.apache.james.protocols.smtp.core.esmtp.LoginSaslMechanismFactory;
import org.apache.james.util.ClassLoaderUtils;
import org.assertj.core.api.SoftAssertions;
+import org.awaitility.Awaitility;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
@@ -620,6 +623,57 @@ class SMTPSaslTest {
assertThat(client.getReplyString()).contains("235 Authentication Successful");
}
+ @Test
+ void disconnectDuringSaslContinuationShouldCloseExchangeOnce() throws Exception {
+ AtomicInteger closeCount = new AtomicInteger();
+ resetWithMechanisms(ImmutableList.of(new RecordingSaslMechanism(closeCount)));
+ SMTPClient client = connectedClient();
+
+ client.sendCommand("AUTH RECORDING");
+ assertThat(client.getReplyString()).contains("334 " + base64("challenge"));
+ client.disconnect();
+
+ // Disconnect cleanup closes the pending SASL exchange exactly once.
+ Awaitility.await().atMost(Duration.ofSeconds(2))
+ .untilAsserted(() -> assertThat(closeCount.get()).isEqualTo(1));
+ }
+
+ @Test
+ void abortDuringSaslContinuationShouldAbortExchangeOnce() throws Exception {
+ AtomicInteger closeCount = new AtomicInteger();
+ AtomicInteger abortCount = new AtomicInteger();
+ resetWithMechanisms(ImmutableList.of(new RecordingSaslMechanism(closeCount, abortCount)));
+ SMTPClient client = connectedClient();
+
+ client.sendCommand("AUTH RECORDING");
+ assertThat(client.getReplyString()).contains("334 " + base64("challenge"));
+ client.sendCommand("*");
+ assertThat(client.getReplyString()).contains("501 5.7.1 Authentication aborted");
+ client.disconnect();
+
+ Awaitility.await().during(Duration.ofMillis(100)).atMost(Duration.ofSeconds(2))
+ .untilAsserted(() -> {
+ assertThat(abortCount.get()).isEqualTo(1);
+ assertThat(closeCount.get()).isZero();
+ });
+ }
+
+ @Test
+ void disconnectAfterTerminalSaslStepShouldNotCloseExchangeAgain() throws Exception {
+ AtomicInteger closeCount = new AtomicInteger();
+ resetWithMechanisms(ImmutableList.of(new RecordingSaslMechanism(closeCount)));
+ SMTPClient client = connectedClient();
+
+ client.sendCommand("AUTH RECORDING");
+ assertThat(client.getReplyString()).contains("334 " + base64("challenge"));
+ client.sendCommand(base64("response"));
+ assertThat(client.getReplyString()).contains("535 Authentication Failed");
+ client.disconnect();
+
+ Awaitility.await().during(Duration.ofMillis(100)).atMost(Duration.ofSeconds(2))
+ .untilAsserted(() -> assertThat(closeCount.get()).isEqualTo(1));
+ }
+
@Test
void mechanismUnavailableOnClearTransportShouldNotBeAdvertisedAndShouldBeRejected() throws Exception {
PlainSaslMechanism plain = new PlainSaslMechanism(true, true);
@@ -724,4 +778,49 @@ class SMTPSaslTest {
};
}
}
+
+ private static class RecordingSaslMechanism implements SaslMechanism {
+ private final AtomicInteger closeCount;
+ private final AtomicInteger abortCount;
+
+ private RecordingSaslMechanism(AtomicInteger closeCount) {
+ this(closeCount, new AtomicInteger());
+ }
+
+ private RecordingSaslMechanism(AtomicInteger closeCount, AtomicInteger abortCount) {
+ this.closeCount = closeCount;
+ this.abortCount = abortCount;
+ }
+
+ @Override
+ public String name() {
+ return "RECORDING";
+ }
+
+ @Override
+ public SaslExchange start(SaslInitialRequest request, SaslAuthenticator authenticator) {
+ return new SaslExchange() {
+ @Override
+ public SaslStep firstStep() {
+ return new SaslStep.Challenge(Optional.of("challenge".getBytes(UTF_8)));
+ }
+
+ @Override
+ public SaslStep onResponse(byte[] clientResponse) {
+ return new SaslStep.Failure(SaslFailure.authenticationFailed(
+ Optional.empty(), Optional.empty(), "Test-only mechanism"));
+ }
+
+ @Override
+ public void abort() {
+ abortCount.incrementAndGet();
+ }
+
+ @Override
+ public void close() {
+ closeCount.incrementAndGet();
+ }
+ };
+ }
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
(james-project) 04/04: JAMES-4210 Drop SaslExchange.abort
Posted by hq...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
quantranhong1999 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 9972dfe3c67d119ec9d80beb5a8c2749cf207d88
Author: Quan Tran <hq...@linagora.com>
AuthorDate: Fri Jul 31 17:10:47 2026 +0700
JAMES-4210 Drop SaslExchange.abort
Simplify the SaslExchange usage. Just rely on SaslExchange.close() to cleanup exchange.
---
.../james/protocols/api/sasl/SaslExchange.java | 7 +-
.../james/protocols/api/sasl/SaslExchangeTest.java | 98 ----------------------
.../api/sasl/SaslMechanismContractTest.java | 18 +---
.../imap/api/process/ImapSaslExchangeTracker.java | 6 --
.../imap/processor/AuthenticateProcessor.java | 12 +--
.../imap/processor/AuthenticateProcessorTest.java | 12 ---
.../protocols/smtp/core/esmtp/AuthCmdHandler.java | 14 +---
.../apache/james/utils/FixedNameSaslMechanism.java | 4 -
.../netty/IMAPServerSaslExchangeLifecycleTest.java | 19 +----
.../james/pop3server/core/AuthCmdHandler.java | 17 +---
.../apache/james/pop3server/POP3ServerTest.java | 9 +-
.../org/apache/james/smtpserver/SMTPSaslTest.java | 21 +----
12 files changed, 23 insertions(+), 214 deletions(-)
diff --git a/protocols/api/src/main/java/org/apache/james/protocols/api/sasl/SaslExchange.java b/protocols/api/src/main/java/org/apache/james/protocols/api/sasl/SaslExchange.java
index 49921941d2..b320c96e1a 100644
--- a/protocols/api/src/main/java/org/apache/james/protocols/api/sasl/SaslExchange.java
+++ b/protocols/api/src/main/java/org/apache/james/protocols/api/sasl/SaslExchange.java
@@ -34,12 +34,9 @@ public interface SaslExchange extends AutoCloseable {
SaslStep onResponse(byte[] clientResponse);
/**
- * Aborts the exchange after a client cancellation or protocol-level failure, and releases associated resources.
+ * Releases resources associated with the exchange after any terminal outcome, including success,
+ * failure, cancellation, disconnect, timeout, or protocol error.
*/
- default void abort() {
- close();
- }
-
@Override
void close();
}
diff --git a/protocols/api/src/test/java/org/apache/james/protocols/api/sasl/SaslExchangeTest.java b/protocols/api/src/test/java/org/apache/james/protocols/api/sasl/SaslExchangeTest.java
deleted file mode 100644
index ebc2773fe3..0000000000
--- a/protocols/api/src/test/java/org/apache/james/protocols/api/sasl/SaslExchangeTest.java
+++ /dev/null
@@ -1,98 +0,0 @@
-/****************************************************************
- * 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.protocols.api.sasl;
-
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
-
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Optional;
-
-import org.apache.james.core.Username;
-import org.junit.jupiter.api.Test;
-
-class SaslExchangeTest {
- private static final Username USER = Username.of("user@example.com");
- private static final SaslIdentity IDENTITY = new SaslIdentity(USER, USER);
-
- private static class RecordingExchange implements SaslExchange {
- protected final List<String> lifecycleEvents = new ArrayList<>();
-
- @Override
- public SaslStep firstStep() {
- return new SaslStep.Challenge(Optional.empty());
- }
-
- @Override
- public SaslStep onResponse(byte[] clientResponse) {
- return new SaslStep.Success(IDENTITY, Optional.empty());
- }
-
- @Override
- public void close() {
- lifecycleEvents.add("close");
- }
- }
-
- private static class OverridingAbortExchange extends RecordingExchange {
- @Override
- public void abort() {
- lifecycleEvents.add("abort");
- close();
- }
- }
-
- private static class ThrowingAbortExchange extends OverridingAbortExchange {
- @Override
- public void abort() {
- super.abort();
- throw new IllegalStateException("boom");
- }
- }
-
- @Test
- void abortShouldCloseExchangeByDefault() {
- RecordingExchange exchange = new RecordingExchange();
-
- exchange.abort();
-
- assertThat(exchange.lifecycleEvents).containsExactly("close");
- }
-
- @Test
- void abortShouldUseExchangeSpecificAbortWhenOverridden() {
- OverridingAbortExchange exchange = new OverridingAbortExchange();
-
- exchange.abort();
-
- assertThat(exchange.lifecycleEvents).containsExactly("abort", "close");
- }
-
- @Test
- void abortShouldPropagateExchangeSpecificAbortFailure() {
- ThrowingAbortExchange exchange = new ThrowingAbortExchange();
-
- assertThatThrownBy(exchange::abort)
- .isInstanceOf(IllegalStateException.class);
-
- assertThat(exchange.lifecycleEvents).containsExactly("abort", "close");
- }
-}
diff --git a/protocols/api/src/test/java/org/apache/james/protocols/api/sasl/SaslMechanismContractTest.java b/protocols/api/src/test/java/org/apache/james/protocols/api/sasl/SaslMechanismContractTest.java
index 2b4054284a..93a55ee1f6 100644
--- a/protocols/api/src/test/java/org/apache/james/protocols/api/sasl/SaslMechanismContractTest.java
+++ b/protocols/api/src/test/java/org/apache/james/protocols/api/sasl/SaslMechanismContractTest.java
@@ -70,7 +70,6 @@ class SaslMechanismContractTest {
private static class FixedStepExchange implements SaslExchange {
private final SaslStep firstStep;
- private boolean aborted;
private boolean closed;
private FixedStepExchange(SaslStep firstStep) {
@@ -87,11 +86,6 @@ class SaslMechanismContractTest {
return firstStep;
}
- @Override
- public void abort() {
- aborted = true;
- }
-
@Override
public void close() {
closed = true;
@@ -133,10 +127,6 @@ class SaslMechanismContractTest {
return new SaslStep.Failure(SaslFailure.invalidCredentials(AUTHENTICATION_ID, Optional.empty(), "rejected"));
}
- @Override
- public void abort() {
- }
-
@Override
public void close() {
}
@@ -226,16 +216,14 @@ class SaslMechanismContractTest {
}
@Test
- void exchangeShouldExposeAbortAndCloseLifecycle() {
+ void exchangeShouldExposeCloseLifecycle() {
// GIVEN an active exchange
FixedStepExchange exchange = new FixedStepExchange(new SaslStep.Failure(SaslFailure.malformed("failure")));
- // WHEN the protocol aborts and then closes it
- exchange.abort();
+ // WHEN the protocol terminates it
exchange.close();
- // THEN mechanisms can observe both lifecycle events
- assertThat(exchange.aborted).isTrue();
+ // THEN the mechanism can release its resources
assertThat(exchange.closed).isTrue();
}
diff --git a/protocols/imap/src/main/java/org/apache/james/imap/api/process/ImapSaslExchangeTracker.java b/protocols/imap/src/main/java/org/apache/james/imap/api/process/ImapSaslExchangeTracker.java
index 4ba7ac0dd2..ef026f73eb 100644
--- a/protocols/imap/src/main/java/org/apache/james/imap/api/process/ImapSaslExchangeTracker.java
+++ b/protocols/imap/src/main/java/org/apache/james/imap/api/process/ImapSaslExchangeTracker.java
@@ -83,12 +83,6 @@ public class ImapSaslExchangeTracker {
}
}
- public void abortExchange(SaslExchange exchange) {
- if (release(exchange)) {
- exchange.abort();
- }
- }
-
public void close() {
SaslExchange exchange;
synchronized (this) {
diff --git a/protocols/imap/src/main/java/org/apache/james/imap/processor/AuthenticateProcessor.java b/protocols/imap/src/main/java/org/apache/james/imap/processor/AuthenticateProcessor.java
index 7acb7b1de2..cf28d22846 100644
--- a/protocols/imap/src/main/java/org/apache/james/imap/processor/AuthenticateProcessor.java
+++ b/protocols/imap/src/main/java/org/apache/james/imap/processor/AuthenticateProcessor.java
@@ -204,7 +204,7 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq
private void handleContinuationLine(SaslExchange exchange, ImapSession session, AuthenticateRequest request, Responder responder, byte[] data) {
if (isAbort(exchange, session, data)) {
- abortActiveContinuation(exchange, session);
+ closeActiveContinuation(exchange, session);
no(request, responder, HumanReadableText.AUTHENTICATION_FAILED);
responder.flush();
return;
@@ -287,14 +287,6 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq
}
}
- private void abortActiveContinuation(SaslExchange exchange, ImapSession session) {
- try {
- session.popLineHandler();
- } finally {
- ImapSaslExchangeTracker.forSession(session).abortExchange(exchange);
- }
- }
-
private void popActiveContinuation(SaslExchange exchange, ImapSession session) {
try {
session.popLineHandler();
@@ -328,7 +320,7 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq
private void handleSuccessDataAcknowledgement(SaslExchange exchange, SaslStep.Success success, ImapSession session,
AuthenticateRequest request, Responder responder, byte[] data) {
if (isAbort(exchange, session, data)) {
- abortActiveContinuation(exchange, session);
+ closeActiveContinuation(exchange, session);
no(request, responder, HumanReadableText.AUTHENTICATION_FAILED);
responder.flush();
return;
diff --git a/protocols/imap/src/test/java/org/apache/james/imap/processor/AuthenticateProcessorTest.java b/protocols/imap/src/test/java/org/apache/james/imap/processor/AuthenticateProcessorTest.java
index 1d66db3437..c0fd36cd6a 100644
--- a/protocols/imap/src/test/java/org/apache/james/imap/processor/AuthenticateProcessorTest.java
+++ b/protocols/imap/src/test/java/org/apache/james/imap/processor/AuthenticateProcessorTest.java
@@ -120,10 +120,6 @@ class AuthenticateProcessorTest {
throw new UnsupportedOperationException();
}
- @Override
- public void abort() {
- }
-
@Override
public void close() {
closed = true;
@@ -143,10 +139,6 @@ class AuthenticateProcessorTest {
throw new IllegalStateException("boom");
}
- @Override
- public void abort() {
- }
-
@Override
public void close() {
closed = true;
@@ -166,10 +158,6 @@ class AuthenticateProcessorTest {
throw new UnsupportedOperationException();
}
- @Override
- public void abort() {
- }
-
@Override
public void close() {
closed = true;
diff --git a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/AuthCmdHandler.java b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/AuthCmdHandler.java
index 8246e8d506..7d8fb2e6c7 100644
--- a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/AuthCmdHandler.java
+++ b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/AuthCmdHandler.java
@@ -200,7 +200,7 @@ public class AuthCmdHandler
return (session, line) -> {
if (SaslCodec.isAbort(line)) {
session.popLineHandler();
- abortExchange(session);
+ closeExchange(session);
return AUTH_ABORTED;
}
return handleSaslContinuation(session, authType, exchange, new String(line, session.getCharset()));
@@ -229,12 +229,9 @@ public class AuthCmdHandler
private Response handleSaslSuccessDataAcknowledgement(SMTPSession session, String authType, SaslExchange exchange,
SaslStep.Success success, String line) {
session.popLineHandler();
- boolean aborted = false;
try {
byte[] bytes = line.getBytes(session.getCharset());
if (SaslCodec.isAbort(bytes)) {
- aborted = true;
- abortExchange(session);
return AUTH_ABORTED;
}
if (!SaslCodec.isEmptyClientResponse(bytes)) {
@@ -242,9 +239,7 @@ public class AuthCmdHandler
}
return applySaslSuccess(session, authType, exchange, success);
} finally {
- if (!aborted) {
- closeExchange(session);
- }
+ closeExchange(session);
}
}
@@ -302,11 +297,6 @@ public class AuthCmdHandler
.ifPresent(SaslExchange::close);
}
- private void abortExchange(SMTPSession session) {
- session.removeAttachment(ACTIVE_SASL_EXCHANGE, ProtocolSession.State.Connection)
- .ifPresent(SaslExchange::abort);
- }
-
@Override
public void onDisconnect(SMTPSession session) {
if (session != null) {
diff --git a/server/container/guice/common/src/test/java/org/apache/james/utils/FixedNameSaslMechanism.java b/server/container/guice/common/src/test/java/org/apache/james/utils/FixedNameSaslMechanism.java
index 9b82df2cb2..a7527df0f3 100644
--- a/server/container/guice/common/src/test/java/org/apache/james/utils/FixedNameSaslMechanism.java
+++ b/server/container/guice/common/src/test/java/org/apache/james/utils/FixedNameSaslMechanism.java
@@ -54,10 +54,6 @@ public class FixedNameSaslMechanism implements SaslMechanism {
return failure();
}
- @Override
- public void abort() {
- }
-
@Override
public void close() {
}
diff --git a/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerSaslExchangeLifecycleTest.java b/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerSaslExchangeLifecycleTest.java
index 4a3c8f8443..3cb66cd78b 100644
--- a/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerSaslExchangeLifecycleTest.java
+++ b/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerSaslExchangeLifecycleTest.java
@@ -48,11 +48,9 @@ class IMAPServerSaslExchangeLifecycleTest extends AbstractIMAPServerTest {
private static class RecordingSaslMechanism implements SaslMechanism {
private final AtomicInteger closeCount;
- private final AtomicInteger abortCount;
- private RecordingSaslMechanism(AtomicInteger closeCount, AtomicInteger abortCount) {
+ private RecordingSaslMechanism(AtomicInteger closeCount) {
this.closeCount = closeCount;
- this.abortCount = abortCount;
}
@Override
@@ -74,11 +72,6 @@ class IMAPServerSaslExchangeLifecycleTest extends AbstractIMAPServerTest {
Optional.empty(), Optional.empty(), "Test-only mechanism"));
}
- @Override
- public void abort() {
- abortCount.incrementAndGet();
- }
-
@Override
public void close() {
closeCount.incrementAndGet();
@@ -88,7 +81,6 @@ class IMAPServerSaslExchangeLifecycleTest extends AbstractIMAPServerTest {
}
private final AtomicInteger closeCount = new AtomicInteger();
- private final AtomicInteger abortCount = new AtomicInteger();
private IMAPServer imapServer;
private int port;
@@ -96,7 +88,7 @@ class IMAPServerSaslExchangeLifecycleTest extends AbstractIMAPServerTest {
@BeforeEach
void setUp() throws Exception {
imapServer = createImapServer("imapServer.xml",
- ImmutableList.of(new RecordingSaslMechanism(closeCount, abortCount)));
+ ImmutableList.of(new RecordingSaslMechanism(closeCount)));
port = imapServer.getListenAddresses().get(0).getPort();
}
@@ -122,7 +114,7 @@ class IMAPServerSaslExchangeLifecycleTest extends AbstractIMAPServerTest {
}
@Test
- void abortDuringSaslContinuationShouldAbortExchangeOnce() throws Exception {
+ void cancellationDuringSaslContinuationShouldCloseExchangeOnce() throws Exception {
IMAPClient client = connectedClient();
try {
assertThat(client.sendCommand("AUTHENTICATE RECORDING")).isEqualTo(IMAPReply.CONT);
@@ -134,10 +126,7 @@ class IMAPServerSaslExchangeLifecycleTest extends AbstractIMAPServerTest {
}
Awaitility.await().during(Duration.ofMillis(100)).atMost(Duration.ofSeconds(2))
- .untilAsserted(() -> {
- assertThat(abortCount.get()).isEqualTo(1);
- assertThat(closeCount.get()).isZero();
- });
+ .untilAsserted(() -> assertThat(closeCount.get()).isEqualTo(1));
}
@Test
diff --git a/server/protocols/protocols-pop3/src/main/java/org/apache/james/pop3server/core/AuthCmdHandler.java b/server/protocols/protocols-pop3/src/main/java/org/apache/james/pop3server/core/AuthCmdHandler.java
index 1fb760d7bc..d78221194c 100644
--- a/server/protocols/protocols-pop3/src/main/java/org/apache/james/pop3server/core/AuthCmdHandler.java
+++ b/server/protocols/protocols-pop3/src/main/java/org/apache/james/pop3server/core/AuthCmdHandler.java
@@ -211,7 +211,7 @@ public class AuthCmdHandler extends AbstractPOP3CommandHandler implements CapaCa
private LineHandler<POP3Session> continuationHandler(SaslExchange exchange) {
return (session, line) -> {
if (isAbort(session, exchange, line)) {
- abortActiveContinuation(session, exchange);
+ closeActiveContinuation(session, exchange);
return AUTH_ABORTED;
}
return nextStep(session, exchange, line)
@@ -252,7 +252,7 @@ public class AuthCmdHandler extends AbstractPOP3CommandHandler implements CapaCa
private LineHandler<POP3Session> successDataAcknowledgementHandler(SaslExchange exchange, SaslStep.Success success) {
return (session, line) -> {
if (isAbort(session, exchange, line)) {
- abortActiveContinuation(session, exchange);
+ closeActiveContinuation(session, exchange);
return AUTH_ABORTED;
}
if (!isEmptyClientResponse(session, exchange, line)) {
@@ -312,14 +312,6 @@ public class AuthCmdHandler extends AbstractPOP3CommandHandler implements CapaCa
}
}
- private void abortActiveContinuation(POP3Session session, SaslExchange exchange) {
- try {
- session.popLineHandler();
- } finally {
- abortExchange(session, exchange);
- }
- }
-
private Response completeAuthentication(POP3Session session, SaslExchange exchange, SaslStep.Success success) {
Username authorizationId = success.identity().authorizationId();
try {
@@ -372,11 +364,6 @@ public class AuthCmdHandler extends AbstractPOP3CommandHandler implements CapaCa
exchange.close();
}
- private void abortExchange(POP3Session session, SaslExchange exchange) {
- session.removeAttachment(ACTIVE_SASL_EXCHANGE, State.Connection);
- exchange.abort();
- }
-
@Override
public void onDisconnect(POP3Session session) {
if (session != null) {
diff --git a/server/protocols/protocols-pop3/src/test/java/org/apache/james/pop3server/POP3ServerTest.java b/server/protocols/protocols-pop3/src/test/java/org/apache/james/pop3server/POP3ServerTest.java
index 3fa4009236..107496b8ba 100644
--- a/server/protocols/protocols-pop3/src/test/java/org/apache/james/pop3server/POP3ServerTest.java
+++ b/server/protocols/protocols-pop3/src/test/java/org/apache/james/pop3server/POP3ServerTest.java
@@ -1177,9 +1177,9 @@ public class POP3ServerTest {
@Test
void authCancellationShouldCloseExchangeAndPreserveUserPassAuthentication() throws Exception {
Username username = Username.of("auth-user");
- AtomicBoolean closed = new AtomicBoolean();
+ AtomicInteger closeCount = new AtomicInteger();
pop3Configuration.setProperty("auth.requireSSL", false);
- pop3Server.setSaslMechanisms(ImmutableList.of(new ServerDataSaslMechanism(username, closed)));
+ pop3Server.setSaslMechanisms(ImmutableList.of(new ServerDataSaslMechanism(username, closeCount)));
finishSetUp(pop3Configuration);
usersRepository.addUser(username, "secret");
@@ -1193,11 +1193,12 @@ public class POP3ServerTest {
send(writer, "AUTH TEST");
assertThat(reader.readLine()).isEqualTo("+ " + encoded("challenge"));
send(writer, "*");
- assertThat(reader.readLine()).startsWith("-ERR");
- assertThat(closed).isTrue();
+ assertThat(reader.readLine()).isEqualTo("-ERR Authentication aborted.");
+ assertThat(closeCount.get()).isEqualTo(1);
send(writer, "PASS secret");
assertThat(reader.readLine()).isEqualTo("+OK Welcome auth-user");
+ assertThat(closeCount.get()).isEqualTo(1);
}
}
diff --git a/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SMTPSaslTest.java b/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SMTPSaslTest.java
index 5d67c858c1..3adc19781f 100644
--- a/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SMTPSaslTest.java
+++ b/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SMTPSaslTest.java
@@ -639,10 +639,9 @@ class SMTPSaslTest {
}
@Test
- void abortDuringSaslContinuationShouldAbortExchangeOnce() throws Exception {
+ void cancellationDuringSaslContinuationShouldCloseExchangeOnce() throws Exception {
AtomicInteger closeCount = new AtomicInteger();
- AtomicInteger abortCount = new AtomicInteger();
- resetWithMechanisms(ImmutableList.of(new RecordingSaslMechanism(closeCount, abortCount)));
+ resetWithMechanisms(ImmutableList.of(new RecordingSaslMechanism(closeCount)));
SMTPClient client = connectedClient();
client.sendCommand("AUTH RECORDING");
@@ -652,10 +651,7 @@ class SMTPSaslTest {
client.disconnect();
Awaitility.await().during(Duration.ofMillis(100)).atMost(Duration.ofSeconds(2))
- .untilAsserted(() -> {
- assertThat(abortCount.get()).isEqualTo(1);
- assertThat(closeCount.get()).isZero();
- });
+ .untilAsserted(() -> assertThat(closeCount.get()).isEqualTo(1));
}
@Test
@@ -781,15 +777,9 @@ class SMTPSaslTest {
private static class RecordingSaslMechanism implements SaslMechanism {
private final AtomicInteger closeCount;
- private final AtomicInteger abortCount;
private RecordingSaslMechanism(AtomicInteger closeCount) {
- this(closeCount, new AtomicInteger());
- }
-
- private RecordingSaslMechanism(AtomicInteger closeCount, AtomicInteger abortCount) {
this.closeCount = closeCount;
- this.abortCount = abortCount;
}
@Override
@@ -811,11 +801,6 @@ class SMTPSaslTest {
Optional.empty(), Optional.empty(), "Test-only mechanism"));
}
- @Override
- public void abort() {
- abortCount.incrementAndGet();
- }
-
@Override
public void close() {
closeCount.incrementAndGet();
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
(james-project) 01/04: JAMES-4210 POP3: add a test to verify SASL exchange closure upon client disconnection
Posted by hq...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
quantranhong1999 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit a5465b3219fb6eec80a931190441cfe952340b80
Author: Quan Tran <hq...@linagora.com>
AuthorDate: Mon Jul 27 16:42:41 2026 +0700
JAMES-4210 POP3: add a test to verify SASL exchange closure upon client disconnection
For POP3, the SASL exchange is well closed upon client disconnection, as `AuthCmdHandler` already implement `DisconnectHandler` to close the SASL exchange.
Dropping that piece of code indeed would make the test fail.
---
.../apache/james/pop3server/POP3ServerTest.java | 37 ++++++++++++++++++++--
1 file changed, 34 insertions(+), 3 deletions(-)
diff --git a/server/protocols/protocols-pop3/src/test/java/org/apache/james/pop3server/POP3ServerTest.java b/server/protocols/protocols-pop3/src/test/java/org/apache/james/pop3server/POP3ServerTest.java
index 932f907a71..3fa4009236 100644
--- a/server/protocols/protocols-pop3/src/test/java/org/apache/james/pop3server/POP3ServerTest.java
+++ b/server/protocols/protocols-pop3/src/test/java/org/apache/james/pop3server/POP3ServerTest.java
@@ -40,6 +40,7 @@ import java.util.Optional;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
import jakarta.mail.util.SharedByteArrayInputStream;
@@ -127,17 +128,25 @@ public class POP3ServerTest {
private static final byte[] SERVER_DATA = "server-data".getBytes(StandardCharsets.US_ASCII);
private final Username username;
- private final AtomicBoolean closed;
+ private final Runnable closeAction;
private final String name;
private ServerDataSaslMechanism(Username username, AtomicBoolean closed) {
this("TEST", username, closed);
}
+ private ServerDataSaslMechanism(Username username, AtomicInteger closeCount) {
+ this("TEST", username, closeCount::incrementAndGet);
+ }
+
private ServerDataSaslMechanism(String name, Username username, AtomicBoolean closed) {
+ this(name, username, () -> closed.set(true));
+ }
+
+ private ServerDataSaslMechanism(String name, Username username, Runnable closeAction) {
this.name = name;
this.username = username;
- this.closed = closed;
+ this.closeAction = closeAction;
}
@Override
@@ -160,7 +169,7 @@ public class POP3ServerTest {
@Override
public void close() {
- closed.set(true);
+ closeAction.run();
}
};
}
@@ -1215,6 +1224,28 @@ public class POP3ServerTest {
}
}
+ @Test
+ void disconnectDuringSaslContinuationShouldCloseExchange() throws Exception {
+ Username username = Username.of("auth-user");
+ AtomicInteger closeCount = new AtomicInteger();
+ pop3Server.setSaslMechanisms(ImmutableList.of(new ServerDataSaslMechanism(username, closeCount)));
+ finishSetUp(pop3Configuration);
+
+ try (Socket socket = connectToPop3Server();
+ BufferedReader reader = reader(socket);
+ BufferedWriter writer = writer(socket)) {
+ assertThat(reader.readLine()).startsWith("+OK");
+
+ send(writer, "AUTH TEST");
+ assertThat(reader.readLine()).isEqualTo("+ " + encoded("challenge"));
+ socket.close();
+
+ // Disconnect cleanup closes the pending SASL exchange.
+ Awaitility.await().atMost(Duration.ofSeconds(2))
+ .untilAsserted(() -> assertThat(closeCount.get()).isEqualTo(1));
+ }
+ }
+
@Test
void authShouldEnforceInitialCommandLength() throws Exception {
Username username = Username.of("auth-user");
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
(james-project) 03/04: JAMES-4210 IMAP: close SASL exchanges upon disconnection
Posted by hq...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
quantranhong1999 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit c49ef4d1ba863b235e1995b89df7175b93e127e4
Author: Quan Tran <hq...@linagora.com>
AuthorDate: Wed Jul 29 08:54:53 2026 +0700
JAMES-4210 IMAP: close SASL exchanges upon disconnection
Track the active SASL exchange as an IMAP session attachment and close it when the connection terminates or encounters a transport exception.
IMAP authentication processing can run asynchronously. A disconnect may therefore race with SASL exchange creation and registration. Keep the tracker sealed after disconnect so delayed registrations are rejected and immediately closed.
Synchronize tracker lookup and creation on the individual IMAP session because getAttribute and setAttribute are not atomic together. This ensures authentication and disconnect paths use the same tracker without introducing cross-session contention.
Ensure normal completion, cancellation, failure, and disconnect paths release each exchange at most once. Add real IMAP server lifecycle tests for disconnect, cancellation, and terminal exchanges.
---
.../imap/api/process/ImapSaslExchangeTracker.java | 115 +++++++++++++++
.../imap/processor/AuthenticateProcessor.java | 22 +--
.../netty/ImapChannelUpstreamHandler.java | 12 ++
.../imapserver/netty/AbstractIMAPServerTest.java | 94 ++++++++++--
.../netty/IMAPServerSaslExchangeLifecycleTest.java | 164 +++++++++++++++++++++
5 files changed, 382 insertions(+), 25 deletions(-)
diff --git a/protocols/imap/src/main/java/org/apache/james/imap/api/process/ImapSaslExchangeTracker.java b/protocols/imap/src/main/java/org/apache/james/imap/api/process/ImapSaslExchangeTracker.java
new file mode 100644
index 0000000000..4ba7ac0dd2
--- /dev/null
+++ b/protocols/imap/src/main/java/org/apache/james/imap/api/process/ImapSaslExchangeTracker.java
@@ -0,0 +1,115 @@
+/****************************************************************
+ * 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.imap.api.process;
+
+import org.apache.james.protocols.api.sasl.SaslExchange;
+
+/**
+ * Owns the active SASL exchange for an IMAP session. Once closed on disconnect,
+ * the tracker stays attached and rejects delayed asynchronous registrations.
+ */
+public class ImapSaslExchangeTracker {
+ private static final String ATTRIBUTE_KEY = ImapSaslExchangeTracker.class.getName();
+
+ public static ImapSaslExchangeTracker forSession(ImapSession session) {
+ // Make tracker initialization atomic with disconnect sealing for this session.
+ synchronized (session) {
+ Object value = session.getAttribute(ATTRIBUTE_KEY);
+ if (value instanceof ImapSaslExchangeTracker tracker) {
+ return tracker;
+ }
+
+ ImapSaslExchangeTracker tracker = new ImapSaslExchangeTracker();
+ session.setAttribute(ATTRIBUTE_KEY, tracker);
+ return tracker;
+ }
+ }
+
+ public static void closeForSession(ImapSession session) {
+ forSession(session).close();
+ }
+
+ private static IllegalStateException closeRejectedExchange(SaslExchange exchange) {
+ IllegalStateException failure = new IllegalStateException("IMAP SASL exchange cannot be registered");
+ try {
+ exchange.close();
+ } catch (RuntimeException e) {
+ failure.addSuppressed(e);
+ }
+ return failure;
+ }
+
+ private SaslExchange activeExchange;
+ private boolean closed;
+
+ private ImapSaslExchangeTracker() {
+ }
+
+ public SaslExchange register(SaslExchange exchange) {
+ if (tryRegister(exchange)) {
+ return exchange;
+ }
+ throw closeRejectedExchange(exchange);
+ }
+
+ private synchronized boolean tryRegister(SaslExchange exchange) {
+ if (closed || activeExchange != null) {
+ return false;
+ }
+ activeExchange = exchange;
+ return true;
+ }
+
+ public void closeExchange(SaslExchange exchange) {
+ if (release(exchange)) {
+ exchange.close();
+ }
+ }
+
+ public void abortExchange(SaslExchange exchange) {
+ if (release(exchange)) {
+ exchange.abort();
+ }
+ }
+
+ public void close() {
+ SaslExchange exchange;
+ synchronized (this) {
+ if (closed) {
+ return;
+ }
+ closed = true;
+ exchange = activeExchange;
+ activeExchange = null;
+ }
+
+ if (exchange != null) {
+ exchange.close();
+ }
+ }
+
+ private synchronized boolean release(SaslExchange exchange) {
+ if (activeExchange != exchange) {
+ return false;
+ }
+ activeExchange = null;
+ return true;
+ }
+}
diff --git a/protocols/imap/src/main/java/org/apache/james/imap/processor/AuthenticateProcessor.java b/protocols/imap/src/main/java/org/apache/james/imap/processor/AuthenticateProcessor.java
index 833fa95247..7acb7b1de2 100644
--- a/protocols/imap/src/main/java/org/apache/james/imap/processor/AuthenticateProcessor.java
+++ b/protocols/imap/src/main/java/org/apache/james/imap/processor/AuthenticateProcessor.java
@@ -29,6 +29,7 @@ import jakarta.inject.Inject;
import org.apache.james.imap.api.display.HumanReadableText;
import org.apache.james.imap.api.message.Capability;
import org.apache.james.imap.api.message.response.StatusResponseFactory;
+import org.apache.james.imap.api.process.ImapSaslExchangeTracker;
import org.apache.james.imap.api.process.ImapSession;
import org.apache.james.imap.main.PathConverter;
import org.apache.james.imap.message.request.AuthenticateRequest;
@@ -97,8 +98,9 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq
try {
SaslInitialRequest initialRequest = SaslCodec.initialRequest(request.getAuthType(), initialClientResponse(request));
SaslAuthenticator authenticator = jamesSaslAuthenticator.withExtraAuthorizator(withAdminUsers());
- SaslExchange exchange = mechanism.get().start(initialRequest, authenticator);
- handleFirstStep(exchange, firstStep(exchange), session, request, responder);
+ SaslExchange exchange = ImapSaslExchangeTracker.forSession(session)
+ .register(mechanism.get().start(initialRequest, authenticator));
+ handleFirstStep(exchange, firstStep(exchange, session), session, request, responder);
} catch (IllegalArgumentException e) {
LOGGER.info("Invalid syntax in AUTHENTICATE initial client response", e);
authFailure(session, request, responder, HumanReadableText.AUTHENTICATION_FAILED, Optional.empty(),
@@ -136,11 +138,11 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq
return Optional.empty();
}
- private SaslStep firstStep(SaslExchange exchange) {
+ private SaslStep firstStep(SaslExchange exchange, ImapSession session) {
try {
return exchange.firstStep();
} catch (RuntimeException e) {
- exchange.close();
+ ImapSaslExchangeTracker.forSession(session).closeExchange(exchange);
throw e;
}
}
@@ -195,7 +197,7 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq
.subscribeOn(ReactorUtils.BLOCKING_CALL_WRAPPER)
.then());
} catch (RuntimeException e) {
- exchange.close();
+ ImapSaslExchangeTracker.forSession(session).closeExchange(exchange);
throw e;
}
}
@@ -281,7 +283,7 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq
try {
session.popLineHandler();
} finally {
- exchange.close();
+ ImapSaslExchangeTracker.forSession(session).closeExchange(exchange);
}
}
@@ -289,7 +291,7 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq
try {
session.popLineHandler();
} finally {
- exchange.abort();
+ ImapSaslExchangeTracker.forSession(session).abortExchange(exchange);
}
}
@@ -297,7 +299,7 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq
try {
session.popLineHandler();
} catch (RuntimeException e) {
- exchange.close();
+ ImapSaslExchangeTracker.forSession(session).closeExchange(exchange);
throw e;
}
}
@@ -318,7 +320,7 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq
.subscribeOn(ReactorUtils.BLOCKING_CALL_WRAPPER)
.then());
} catch (RuntimeException e) {
- exchange.close();
+ ImapSaslExchangeTracker.forSession(session).closeExchange(exchange);
throw e;
}
}
@@ -348,7 +350,7 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq
try {
handleSaslStep(step, session, request, responder, successLog(request));
} finally {
- exchange.close();
+ ImapSaslExchangeTracker.forSession(session).closeExchange(exchange);
}
}
diff --git a/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/ImapChannelUpstreamHandler.java b/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/ImapChannelUpstreamHandler.java
index 800fecc6a8..323745e4a0 100644
--- a/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/ImapChannelUpstreamHandler.java
+++ b/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/ImapChannelUpstreamHandler.java
@@ -43,6 +43,7 @@ import org.apache.james.imap.api.ImapSessionState;
import org.apache.james.imap.api.display.HumanReadableText;
import org.apache.james.imap.api.message.response.StatusResponse;
import org.apache.james.imap.api.process.ImapProcessor;
+import org.apache.james.imap.api.process.ImapSaslExchangeTracker;
import org.apache.james.imap.api.process.ImapSession;
import org.apache.james.imap.api.process.ImapSession.SessionId;
import org.apache.james.imap.encode.ImapEncoder;
@@ -274,6 +275,7 @@ public class ImapChannelUpstreamHandler extends ChannelInboundHandlerAdapter imp
InetSocketAddress address = (InetSocketAddress) ctx.channel().remoteAddress();
LOGGER.info("Connection closed for {} and user {}", address.getAddress().getHostAddress(), retrieveUsername(imapSession));
+ closeSaslExchange(imapSession);
Optional.ofNullable(imapSession).ifPresent(ImapSession::cancelOngoingProcessing);
Optional.ofNullable(imapSession)
.map(ImapSession::logout)
@@ -300,6 +302,7 @@ public class ImapChannelUpstreamHandler extends ChannelInboundHandlerAdapter imp
ImapSession imapSession = ctx.channel().attr(IMAP_SESSION_ATTRIBUTE_KEY).getAndSet(null);
String username = retrieveUsername(imapSession);
try (Closeable closeable = mdc(imapSession).build()) {
+ closeSaslExchange(imapSession);
if (cause instanceof SocketException) {
logExpectedException("Socket exception encountered for user " + username, cause);
} else if (isSslHandshkeException(cause)) {
@@ -339,6 +342,15 @@ public class ImapChannelUpstreamHandler extends ChannelInboundHandlerAdapter imp
}
}
+ private void closeSaslExchange(ImapSession imapSession) {
+ try {
+ // Fallback when the connection terminates or encounters an unexpected transport exception during a pending exchange.
+ Optional.ofNullable(imapSession).ifPresent(ImapSaslExchangeTracker::closeForSession);
+ } catch (RuntimeException e) {
+ LOGGER.warn("Failed to close the active IMAP SASL exchange", e);
+ }
+ }
+
private void logExpectedException(String message, Throwable cause) {
if (LOGGER.isDebugEnabled()) {
LOGGER.debug(message, cause);
diff --git a/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/AbstractIMAPServerTest.java b/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/AbstractIMAPServerTest.java
index a4c6471f4c..0eed3b718b 100644
--- a/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/AbstractIMAPServerTest.java
+++ b/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/AbstractIMAPServerTest.java
@@ -27,6 +27,7 @@ import java.nio.channels.SocketChannel;
import java.nio.charset.StandardCharsets;
import java.security.cert.X509Certificate;
import java.util.List;
+import java.util.Optional;
import java.util.Set;
import java.util.function.Predicate;
@@ -37,10 +38,16 @@ import org.apache.commons.configuration2.tree.ImmutableNode;
import org.apache.commons.net.imap.AuthenticatingIMAPClient;
import org.apache.james.core.Username;
import org.apache.james.imap.api.ConnectionCheck;
+import org.apache.james.imap.api.message.response.StatusResponseFactory;
+import org.apache.james.imap.api.process.ImapProcessor;
import org.apache.james.imap.encode.main.DefaultImapEncoderFactory;
import org.apache.james.imap.main.DefaultImapDecoderFactory;
+import org.apache.james.imap.message.response.UnpooledStatusResponseFactory;
+import org.apache.james.imap.processor.DefaultProcessor;
+import org.apache.james.imap.processor.base.UnknownRequestProcessor;
import org.apache.james.imap.processor.fetch.FetchProcessor;
import org.apache.james.imap.processor.main.DefaultImapProcessorFactory;
+import org.apache.james.mailbox.MailboxCounterCorrector;
import org.apache.james.mailbox.inmemory.InMemoryMailboxManager;
import org.apache.james.mailbox.inmemory.manager.InMemoryIntegrationResources;
import org.apache.james.mailbox.store.FakeAuthenticator;
@@ -48,6 +55,7 @@ import org.apache.james.mailbox.store.FakeAuthorizator;
import org.apache.james.mailbox.store.StoreSubscriptionManager;
import org.apache.james.metrics.api.NoopGaugeRegistry;
import org.apache.james.metrics.tests.RecordingMetricFactory;
+import org.apache.james.protocols.api.sasl.SaslMechanism;
import org.apache.james.protocols.api.utils.BogusSslContextFactory;
import org.apache.james.protocols.api.utils.BogusTrustManagerFactory;
import org.apache.james.protocols.lib.LegacyJavaEncryptionFactory;
@@ -102,26 +110,33 @@ abstract class AbstractIMAPServerTest {
protected IMAPServer createImapServer(HierarchicalConfiguration<ImmutableNode> config,
InMemoryIntegrationResources inMemoryIntegrationResources,
FetchProcessor.LocalCacheConfiguration localCacheConfiguration) throws Exception {
+ return createImapServer(config, inMemoryIntegrationResources, localCacheConfiguration, Optional.empty());
+ }
+
+ protected IMAPServer createImapServer(HierarchicalConfiguration<ImmutableNode> config,
+ InMemoryIntegrationResources inMemoryIntegrationResources,
+ FetchProcessor.LocalCacheConfiguration localCacheConfiguration,
+ ImmutableList<SaslMechanism> saslMechanisms) throws Exception {
+ return createImapServer(config, inMemoryIntegrationResources, localCacheConfiguration, Optional.of(saslMechanisms));
+ }
+
+ private IMAPServer createImapServer(HierarchicalConfiguration<ImmutableNode> config,
+ InMemoryIntegrationResources inMemoryIntegrationResources,
+ FetchProcessor.LocalCacheConfiguration localCacheConfiguration,
+ Optional<ImmutableList<SaslMechanism>> saslMechanisms) throws Exception {
memoryIntegrationResources = inMemoryIntegrationResources;
RecordingMetricFactory metricFactory = new RecordingMetricFactory();
Set<ConnectionCheck> connectionChecks = defaultConnectionChecks();
mailboxManager = spy(memoryIntegrationResources.getMailboxManager());
+ StoreSubscriptionManager subscriptionManager = new StoreSubscriptionManager(mailboxManager.getMapperFactory(),
+ mailboxManager.getMapperFactory(),
+ mailboxManager.getEventBus());
+ ImapProcessor processor = createProcessor(config, localCacheConfiguration, metricFactory, subscriptionManager, saslMechanisms);
IMAPServer imapServer = new IMAPServer(
new DefaultImapDecoderFactory().buildImapDecoder(),
new DefaultImapEncoderFactory().buildImapEncoder(),
- DefaultImapProcessorFactory.createXListSupportingProcessor(
- mailboxManager,
- memoryIntegrationResources.getEventBus(),
- new StoreSubscriptionManager(mailboxManager.getMapperFactory(),
- mailboxManager.getMapperFactory(),
- mailboxManager.getEventBus()),
- null,
- memoryIntegrationResources.getQuotaManager(),
- memoryIntegrationResources.getQuotaRootResolver(),
- metricFactory,
- localCacheConfiguration,
- config),
+ processor,
new ImapMetrics(metricFactory),
new NoopGaugeRegistry(), connectionChecks);
@@ -135,13 +150,57 @@ abstract class AbstractIMAPServerTest {
return imapServer;
}
+ private ImapProcessor createProcessor(HierarchicalConfiguration<ImmutableNode> config,
+ FetchProcessor.LocalCacheConfiguration localCacheConfiguration,
+ RecordingMetricFactory metricFactory,
+ StoreSubscriptionManager subscriptionManager,
+ Optional<ImmutableList<SaslMechanism>> saslMechanisms) throws Exception {
+ if (saslMechanisms.isEmpty()) {
+ return DefaultImapProcessorFactory.createXListSupportingProcessor(
+ mailboxManager,
+ memoryIntegrationResources.getEventBus(),
+ subscriptionManager,
+ null,
+ memoryIntegrationResources.getQuotaManager(),
+ memoryIntegrationResources.getQuotaRootResolver(),
+ metricFactory,
+ localCacheConfiguration,
+ config);
+ }
+
+ StatusResponseFactory statusResponseFactory = new UnpooledStatusResponseFactory();
+ return DefaultProcessor.createDefaultProcessor(
+ new UnknownRequestProcessor(statusResponseFactory),
+ mailboxManager,
+ memoryIntegrationResources.getEventBus(),
+ subscriptionManager,
+ statusResponseFactory,
+ null,
+ memoryIntegrationResources.getQuotaManager(),
+ memoryIntegrationResources.getQuotaRootResolver(),
+ MailboxCounterCorrector.DEFAULT,
+ metricFactory,
+ localCacheConfiguration,
+ saslMechanisms.orElseThrow());
+ }
+
protected IMAPServer createImapServer(HierarchicalConfiguration<ImmutableNode> config, FetchProcessor.LocalCacheConfiguration localCacheConfiguration) throws Exception {
+ return createImapServer(config, defaultIntegrationResources(), localCacheConfiguration);
+ }
+
+ protected IMAPServer createImapServer(HierarchicalConfiguration<ImmutableNode> config,
+ FetchProcessor.LocalCacheConfiguration localCacheConfiguration,
+ ImmutableList<SaslMechanism> saslMechanisms) throws Exception {
+ return createImapServer(config, defaultIntegrationResources(), localCacheConfiguration, saslMechanisms);
+ }
+
+ private InMemoryIntegrationResources defaultIntegrationResources() {
authenticator = new FakeAuthenticator();
authenticator.addUser(USER, USER_PASS);
authenticator.addUser(USER2, USER_PASS);
authenticator.addUser(USER3, USER_PASS);
- memoryIntegrationResources = InMemoryIntegrationResources.builder()
+ return InMemoryIntegrationResources.builder()
.authenticator(authenticator)
.authorizator(FakeAuthorizator.defaultReject())
.inVmEventBus()
@@ -151,8 +210,6 @@ abstract class AbstractIMAPServerTest {
.noPreDeletionHooks()
.storeQuotaManager()
.build();
-
- return createImapServer(config, memoryIntegrationResources, localCacheConfiguration);
}
protected IMAPServer createImapServer(HierarchicalConfiguration<ImmutableNode> config) throws Exception {
@@ -163,6 +220,13 @@ abstract class AbstractIMAPServerTest {
return createImapServer(ConfigLoader.getConfig(ClassLoaderUtils.getSystemResourceAsSharedStream(configurationFile)), localCacheConfiguration);
}
+ protected IMAPServer createImapServer(String configurationFile, ImmutableList<SaslMechanism> saslMechanisms) throws Exception {
+ return createImapServer(
+ ConfigLoader.getConfig(ClassLoaderUtils.getSystemResourceAsSharedStream(configurationFile)),
+ FetchProcessor.LocalCacheConfiguration.DEFAULT,
+ saslMechanisms);
+ }
+
protected IMAPServer createImapServer(String configurationFile) throws Exception {
return createImapServer(configurationFile, FetchProcessor.LocalCacheConfiguration.DEFAULT);
}
diff --git a/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerSaslExchangeLifecycleTest.java b/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerSaslExchangeLifecycleTest.java
new file mode 100644
index 0000000000..4a3c8f8443
--- /dev/null
+++ b/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerSaslExchangeLifecycleTest.java
@@ -0,0 +1,164 @@
+/****************************************************************
+ * 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.imapserver.netty;
+
+import static java.nio.charset.StandardCharsets.US_ASCII;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.IOException;
+import java.time.Duration;
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.commons.net.imap.IMAPClient;
+import org.apache.commons.net.imap.IMAPReply;
+import org.apache.james.protocols.api.sasl.SaslAuthenticator;
+import org.apache.james.protocols.api.sasl.SaslExchange;
+import org.apache.james.protocols.api.sasl.SaslFailure;
+import org.apache.james.protocols.api.sasl.SaslInitialRequest;
+import org.apache.james.protocols.api.sasl.SaslMechanism;
+import org.apache.james.protocols.api.sasl.SaslStep;
+import org.awaitility.Awaitility;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import com.google.common.collect.ImmutableList;
+
+class IMAPServerSaslExchangeLifecycleTest extends AbstractIMAPServerTest {
+ private static final String CHALLENGE = "Y2hhbGxlbmdl";
+ private static final String CLIENT_RESPONSE = "cmVzcG9uc2U=";
+
+ private static class RecordingSaslMechanism implements SaslMechanism {
+ private final AtomicInteger closeCount;
+ private final AtomicInteger abortCount;
+
+ private RecordingSaslMechanism(AtomicInteger closeCount, AtomicInteger abortCount) {
+ this.closeCount = closeCount;
+ this.abortCount = abortCount;
+ }
+
+ @Override
+ public String name() {
+ return "RECORDING";
+ }
+
+ @Override
+ public SaslExchange start(SaslInitialRequest request, SaslAuthenticator authenticator) {
+ return new SaslExchange() {
+ @Override
+ public SaslStep firstStep() {
+ return new SaslStep.Challenge(Optional.of("challenge".getBytes(US_ASCII)));
+ }
+
+ @Override
+ public SaslStep onResponse(byte[] clientResponse) {
+ return new SaslStep.Failure(SaslFailure.authenticationFailed(
+ Optional.empty(), Optional.empty(), "Test-only mechanism"));
+ }
+
+ @Override
+ public void abort() {
+ abortCount.incrementAndGet();
+ }
+
+ @Override
+ public void close() {
+ closeCount.incrementAndGet();
+ }
+ };
+ }
+ }
+
+ private final AtomicInteger closeCount = new AtomicInteger();
+ private final AtomicInteger abortCount = new AtomicInteger();
+
+ private IMAPServer imapServer;
+ private int port;
+
+ @BeforeEach
+ void setUp() throws Exception {
+ imapServer = createImapServer("imapServer.xml",
+ ImmutableList.of(new RecordingSaslMechanism(closeCount, abortCount)));
+ port = imapServer.getListenAddresses().get(0).getPort();
+ }
+
+ @AfterEach
+ void tearDown() {
+ if (imapServer != null) {
+ imapServer.destroy();
+ }
+ }
+
+ @Test
+ void disconnectDuringSaslContinuationShouldCloseExchangeOnce() throws Exception {
+ IMAPClient client = connectedClient();
+ try {
+ assertThat(client.sendCommand("AUTHENTICATE RECORDING")).isEqualTo(IMAPReply.CONT);
+ assertThat(client.getReplyString()).contains("+ " + CHALLENGE);
+ } finally {
+ client.disconnect();
+ }
+
+ Awaitility.await().atMost(Duration.ofSeconds(2))
+ .untilAsserted(() -> assertThat(closeCount.get()).isEqualTo(1));
+ }
+
+ @Test
+ void abortDuringSaslContinuationShouldAbortExchangeOnce() throws Exception {
+ IMAPClient client = connectedClient();
+ try {
+ assertThat(client.sendCommand("AUTHENTICATE RECORDING")).isEqualTo(IMAPReply.CONT);
+ assertThat(client.getReplyString()).contains("+ " + CHALLENGE);
+ assertThat(client.sendData("*")).isEqualTo(IMAPReply.NO);
+ assertThat(client.getReplyString()).contains("NO AUTHENTICATE failed.");
+ } finally {
+ client.disconnect();
+ }
+
+ Awaitility.await().during(Duration.ofMillis(100)).atMost(Duration.ofSeconds(2))
+ .untilAsserted(() -> {
+ assertThat(abortCount.get()).isEqualTo(1);
+ assertThat(closeCount.get()).isZero();
+ });
+ }
+
+ @Test
+ void disconnectAfterTerminalSaslStepShouldNotCloseExchangeAgain() throws Exception {
+ IMAPClient client = connectedClient();
+ try {
+ assertThat(client.sendCommand("AUTHENTICATE RECORDING")).isEqualTo(IMAPReply.CONT);
+ assertThat(client.getReplyString()).contains("+ " + CHALLENGE);
+ assertThat(client.sendData(CLIENT_RESPONSE)).isEqualTo(IMAPReply.NO);
+ assertThat(client.getReplyString()).contains("NO AUTHENTICATE failed.");
+ } finally {
+ client.disconnect();
+ }
+
+ Awaitility.await().during(Duration.ofMillis(100)).atMost(Duration.ofSeconds(2))
+ .untilAsserted(() -> assertThat(closeCount.get()).isEqualTo(1));
+ }
+
+ private IMAPClient connectedClient() throws IOException {
+ IMAPClient client = new IMAPClient();
+ client.connect("127.0.0.1", port);
+ return client;
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org