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 5529384c4b930c51c4bd9a570eafe420c4f0a1fa Author: Felix Auringer <[email protected]> AuthorDate: Tue May 19 12:22:45 2026 +0200 refactor(imap): consistent handling of auth failure and success There are now two methods (authFailure and authSuccess) and all paths that result in successful authentication or an error caused by the user are now handled in those two places. There are still some errors that are caused by James itself which are not handled by those two methods. --- .../imap/processor/AbstractAuthProcessor.java | 181 ++++++++++----------- .../imap/processor/AuthenticateProcessor.java | 8 +- .../james/imap/processor/LoginProcessor.java | 3 +- 3 files changed, 92 insertions(+), 100 deletions(-) diff --git a/protocols/imap/src/main/java/org/apache/james/imap/processor/AbstractAuthProcessor.java b/protocols/imap/src/main/java/org/apache/james/imap/processor/AbstractAuthProcessor.java index 69cb152ea5..f6a97dfb6b 100644 --- a/protocols/imap/src/main/java/org/apache/james/imap/processor/AbstractAuthProcessor.java +++ b/protocols/imap/src/main/java/org/apache/james/imap/processor/AbstractAuthProcessor.java @@ -80,62 +80,52 @@ public abstract class AbstractAuthProcessor<R extends ImapRequest> extends Abstr this.imapConfiguration = imapConfiguration; } - protected void doPasswordAuth(AuthenticationAttempt authenticationAttempt, ImapSession session, ImapRequest request, Responder responder, HumanReadableText failed) { + protected void doPasswordAuth(AuthenticationAttempt authenticationAttempt, ImapSession session, ImapRequest request, Responder responder) { Preconditions.checkArgument(!authenticationAttempt.isDelegation()); - try { - boolean authFailure = false; - if (authenticationAttempt.getAuthenticationId() == null) { - authFailure = true; - } - if (!authFailure) { - try { - final MailboxSession mailboxSession = getMailboxManager().authenticate(authenticationAttempt.getAuthenticationId(), - authenticationAttempt.getPassword()) - .withoutDelegation(); - session.authenticated(); - session.setMailboxSession(mailboxSession); - provisionInbox(session, getMailboxManager(), mailboxSession); - AuditTrail.entry() - .username(() -> mailboxSession.getUser().asString()) - .sessionId(() -> session.sessionId().asString()) - .protocol("IMAP") - .action("AUTH") - .log("IMAP Authentication succeeded."); - okComplete(request, responder); - session.stopDetectingCommandInjection(); - } catch (BadCredentialsException e) { - authFailure = true; - AuditTrail.entry() - .username(() -> authenticationAttempt.getAuthenticationId().asString()) - .protocol("IMAP") - .action("AUTH") - .log("IMAP Authentication failed because of bad credentials."); - } - } - if (authFailure) { - manageFailureCount(session, request, responder, failed); + + if (authenticationAttempt.getAuthenticationId() == null || authenticationAttempt.getPassword() == null) { + authFailure(session, request, responder, HumanReadableText.AUTHENTICATION_FAILED, Optional.empty(), Optional.empty(), + "Malformed authentication command." + ); + } else { + try { + final MailboxSession mailboxSession = getMailboxManager().authenticate( + authenticationAttempt.getAuthenticationId(), + authenticationAttempt.getPassword() + ).withoutDelegation(); + authSuccess(session, mailboxSession, request, responder, "Password authentication succeeded."); + } catch (BadCredentialsException e) { + authFailure(session, request, responder, HumanReadableText.INVALID_LOGIN, + Optional.of(authenticationAttempt.getAuthenticationId()), + Optional.empty(), + "Password authentication failed because of bad credentials." + ); + } catch (MailboxException e) { + // This is probably not a user error, so we do not increase the failure count or add the + // event to the audit log. + LOGGER.error("Authentication failed", e); + no(request, responder, HumanReadableText.GENERIC_FAILURE_DURING_PROCESSING); } - } catch (MailboxException e) { - LOGGER.error("Error encountered while login", e); - no(request, responder, HumanReadableText.GENERIC_FAILURE_DURING_PROCESSING); } } protected void doPasswordAuthWithDelegation(AuthenticationAttempt authenticationAttempt, ImapSession session, ImapRequest request, Responder responder) { Preconditions.checkArgument(authenticationAttempt.isDelegation()); + Username otherUser = authenticationAttempt.getDelegateUserName().orElseThrow(); + Username givenUser = authenticationAttempt.getAuthenticationId(); if (givenUser == null) { - manageFailureCount(session, request, responder); - return; + authFailure(session, request, responder, HumanReadableText.AUTHENTICATION_FAILED, + Optional.empty(), Optional.of(otherUser), "Malformed authentication command."); + } else { + doAuthWithDelegation(() -> getMailboxManager() + .withExtraAuthorizator(withAdminUsers()) + .authenticate(givenUser, authenticationAttempt.getPassword()) + .as(otherUser), + session, + request, responder, + givenUser, otherUser); } - Username otherUser = authenticationAttempt.getDelegateUserName().orElseThrow(); - doAuthWithDelegation(() -> getMailboxManager() - .withExtraAuthorizator(withAdminUsers()) - .authenticate(givenUser, authenticationAttempt.getPassword()) - .as(otherUser), - session, - request, responder, - givenUser, otherUser); } protected Authorizator withAdminUsers() { @@ -151,45 +141,20 @@ public abstract class AbstractAuthProcessor<R extends ImapRequest> extends Abstr ImapSession session, ImapRequest request, Responder responder, Username authenticateUser, Username delegatorUser) { try { - MailboxManager mailboxManager = getMailboxManager(); - MailboxSession mailboxSession = mailboxSessionSupplier.get(); - session.authenticated(); - session.setMailboxSession(mailboxSession); - AuditTrail.entry() - .username(() -> mailboxSession.getLoggedInUser() - .map(Username::asString) - .orElse("")) - .sessionId(() -> session.sessionId().asString()) - .protocol("IMAP") - .action("AUTH") - .remoteIP(() -> Optional.ofNullable(session.getRemoteAddress())) - .parameters(() -> ImmutableMap.of("delegatorUser", mailboxSession.getUser().asString())) - .log("IMAP Authentication with delegation succeeded."); - okComplete(request, responder); - provisionInbox(session, mailboxManager, mailboxSession); + authSuccess(session, mailboxSessionSupplier.get(), request, responder, "Authentication with delegation succeeded."); } catch (BadCredentialsException e) { - AuditTrail.entry() - .username(authenticateUser::asString) - .protocol("IMAP") - .action("AUTH") - .remoteIP(() -> Optional.ofNullable(session.getRemoteAddress())) - .parameters(() -> ImmutableMap.of("delegatorUser", delegatorUser.asString())) - .log("IMAP Authentication with delegation failed because of bad credentials."); - manageFailureCount(session, request, responder); + authFailure(session, request, responder, HumanReadableText.INVALID_LOGIN, Optional.of(authenticateUser), + Optional.of(delegatorUser), "Password authentication with delegation failed because of bad credentials."); } catch (UserDoesNotExistException e) { - LOGGER.info("User does not exist", e); - no(request, responder, HumanReadableText.USER_DOES_NOT_EXIST); + authFailure(session, request, responder, HumanReadableText.USER_DOES_NOT_EXIST, Optional.of(authenticateUser), + Optional.of(delegatorUser), "Delegation target user does not exist."); } catch (ForbiddenDelegationException e) { - LOGGER.info("Delegate forbidden", e); - AuditTrail.entry() - .username(authenticateUser::asString) - .protocol("IMAP") - .action("AUTH") - .parameters(() -> ImmutableMap.of("delegatorUser", delegatorUser.asString())) - .log("IMAP Authentication with delegation failed because of non existing delegation."); - no(request, responder, HumanReadableText.DELEGATION_FORBIDDEN); + authFailure(session, request, responder, HumanReadableText.DELEGATION_FORBIDDEN, Optional.of(authenticateUser), + Optional.of(delegatorUser), "Requested delegation is forbidden."); } catch (MailboxException e) { - LOGGER.info("Login failed", e); + // This is probably not a user error, so we do not increase the failure count or add the + // event to the audit log. + LOGGER.info("Authentication failed", e); no(request, responder, HumanReadableText.GENERIC_FAILURE_DURING_PROCESSING); } } @@ -206,9 +171,16 @@ public abstract class AbstractAuthProcessor<R extends ImapRequest> extends Abstr .as(associatedUser), session, request, responder, authenticatedUser, associatedUser); } else { - authSuccess(authenticatedUser, session, request, responder); + authSuccess(session, getMailboxManager().createSystemSession(authenticatedUser), request, responder, + "OAuth authentication succeeded." + ); } - }, () -> manageFailureCount(session, request, responder)); + }, () -> { + authFailure(session, request, responder, HumanReadableText.AUTHENTICATION_FAILED, Optional.empty(), + Optional.of(Username.of(oidcInitialResponse.getAssociatedUser())), + "OAuth authentication failed." + ); + }); } protected void provisionInbox(ImapSession session, MailboxManager mailboxManager, MailboxSession mailboxSession) throws MailboxException { @@ -243,10 +215,6 @@ public abstract class AbstractAuthProcessor<R extends ImapRequest> extends Abstr } } - protected void manageFailureCount(ImapSession session, ImapRequest request, Responder responder) { - manageFailureCount(session, request, responder, HumanReadableText.AUTHENTICATION_FAILED); - } - protected void manageFailureCount(ImapSession session, ImapRequest request, Responder responder, HumanReadableText failed) { Integer currentNumberOfFailures = (Integer) session.getAttribute(ATTRIBUTE_NUMBER_OF_FAILURES); int failures; @@ -273,17 +241,42 @@ public abstract class AbstractAuthProcessor<R extends ImapRequest> extends Abstr return new AuthenticationAttempt(Optional.empty(), authenticationId, password); } - protected void oauthSuccess(Username username, ImapSession session, ImapRequest request, Responder responder) { + protected void authSuccess(ImapSession session, MailboxSession mailboxSession, ImapRequest request, Responder responder, String log) { + session.authenticated(); + session.setMailboxSession(mailboxSession); try { - session.authenticated(); - final MailboxSession mailboxSession = getMailboxManager().createSystemSession(username); - session.setMailboxSession(mailboxSession); provisionInbox(session, getMailboxManager(), mailboxSession); - okComplete(request, responder); } catch (MailboxException e) { - LOGGER.error("Error encountered while login", e); - no(request, responder, HumanReadableText.GENERIC_FAILURE_DURING_PROCESSING); + LOGGER.error("Provisioning mailboxes failed but authentication continues", e); + } + + AuditTrail.Entry entry = AuditTrail.entry() + .username(() -> mailboxSession.getUser().asString()) + .sessionId(() -> session.sessionId().asString()) + .protocol("IMAP") + .action("AUTH") + .remoteIP(() -> Optional.ofNullable(session.getRemoteAddress())); + Optional<Username> assumedUser = mailboxSession.getLoggedInUser(); + if (assumedUser.isPresent()) { + entry = entry.parameters(() -> ImmutableMap.of("delegatorUser", assumedUser.get().asString())); + } + entry.log(log); + okComplete(request, responder); + session.stopDetectingCommandInjection(); + } + + protected void authFailure(ImapSession session, ImapRequest request, Responder responder, HumanReadableText failed, Optional<Username> username, Optional<Username> assumedUser, String log) { + AuditTrail.Entry entry = AuditTrail.entry() + .username(() -> username.map(name -> name.asString()).orElse(null)) + .sessionId(() -> session.sessionId().asString()) + .protocol("IMAP") + .action("AUTH") + .remoteIP(() -> Optional.ofNullable(session.getRemoteAddress())); + if (assumedUser.isPresent()) { + entry = entry.parameters(() -> ImmutableMap.of("delegatorUser", assumedUser.get().asString())); } + entry.log(log); + manageFailureCount(session, request, responder, failed); } protected static class AuthenticationAttempt { 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 2988064e82..cfae43c6f3 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 @@ -24,6 +24,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Base64; import java.util.List; +import java.util.Optional; import java.util.stream.Collectors; import jakarta.inject.Inject; @@ -136,9 +137,8 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq if (authenticationAttempt.isDelegation()) { doPasswordAuthWithDelegation(authenticationAttempt, session, request, responder); } else { - doPasswordAuth(authenticationAttempt, session, request, responder, HumanReadableText.AUTHENTICATION_FAILED); + doPasswordAuth(authenticationAttempt, session, request, responder); } - session.stopDetectingCommandInjection(); } /** @@ -148,8 +148,8 @@ public class AuthenticateProcessor extends AbstractAuthProcessor<AuthenticateReq OIDCSASLParser.parse(initialResponse) .flatMap(oidcInitialResponseValue -> session.oidcSaslConfiguration().map(configure -> Pair.of(oidcInitialResponseValue, configure))) .ifPresentOrElse(pair -> doOAuth(pair.getLeft(), pair.getRight(), session, request, responder), - () -> manageFailureCount(session, request, responder)); - session.stopDetectingCommandInjection(); + () -> authFailure(session, request, responder, HumanReadableText.AUTHENTICATION_FAILED, Optional.empty(), + Optional.empty(), "Malformed authentication command.")); } private AuthenticationAttempt parseDelegationAttempt(String initialClientResponse) { diff --git a/protocols/imap/src/main/java/org/apache/james/imap/processor/LoginProcessor.java b/protocols/imap/src/main/java/org/apache/james/imap/processor/LoginProcessor.java index 989a487965..c7c2a845b3 100644 --- a/protocols/imap/src/main/java/org/apache/james/imap/processor/LoginProcessor.java +++ b/protocols/imap/src/main/java/org/apache/james/imap/processor/LoginProcessor.java @@ -60,8 +60,7 @@ public class LoginProcessor extends AbstractAuthProcessor<LoginRequest> implemen LOGGER.warn("Login rejected because it is disabled or not allowed over insecure channel"); no(request, responder, HumanReadableText.DISABLED_LOGIN); } else { - doPasswordAuth(noDelegation(request.getUserid(), request.getPassword()), - session, request, responder, HumanReadableText.INVALID_LOGIN); + doPasswordAuth(noDelegation(request.getUserid(), request.getPassword()), session, request, responder); } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
