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 <[email protected]> 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("[email protected]"); - 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: [email protected] For additional commands, e-mail: [email protected]
