JAMES-2557 SMTP session should use an Optional<MailAddress> as a sender
Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/0bdffec1 Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/0bdffec1 Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/0bdffec1 Branch: refs/heads/master Commit: 0bdffec1d3bac025f8c2517609a78195b110bb7a Parents: 5b673a0 Author: Benoit Tellier <btell...@linagora.com> Authored: Thu Dec 6 11:30:28 2018 +0700 Committer: Benoit Tellier <btell...@linagora.com> Committed: Tue Dec 11 13:59:20 2018 +0700 ---------------------------------------------------------------------- ...tSenderAuthIdentifyVerificationRcptHook.java | 34 +++++++++++++------- .../protocols/smtp/core/DataCmdHandler.java | 4 +-- .../protocols/smtp/core/MailCmdHandler.java | 33 +++++++++++-------- .../protocols/smtp/core/RcptCmdHandler.java | 11 +++---- .../smtp/core/esmtp/MailSizeEsmtpExtension.java | 13 +++++--- .../fastfail/ValidSenderDomainHandlerTest.java | 9 +++--- .../org/apache/james/server/core/MailImpl.java | 8 +++-- .../DataLineJamesMessageHookHandler.java | 5 +-- .../smtpserver/SpamAssassinHandlerTest.java | 9 +++++- .../james/smtpserver/URIRBLHandlerTest.java | 9 +++++- 10 files changed, 86 insertions(+), 49 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/0bdffec1/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/AbstractSenderAuthIdentifyVerificationRcptHook.java ---------------------------------------------------------------------- diff --git a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/AbstractSenderAuthIdentifyVerificationRcptHook.java b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/AbstractSenderAuthIdentifyVerificationRcptHook.java index dea7d52..9265833 100644 --- a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/AbstractSenderAuthIdentifyVerificationRcptHook.java +++ b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/AbstractSenderAuthIdentifyVerificationRcptHook.java @@ -31,6 +31,8 @@ import org.apache.james.protocols.smtp.hook.HookResult; import org.apache.james.protocols.smtp.hook.HookReturnCode; import org.apache.james.protocols.smtp.hook.RcptHook; +import com.google.common.base.Preconditions; + /** * Handler which check if the authenticated user is the same as the one used as MAIL FROM */ @@ -45,28 +47,36 @@ public abstract class AbstractSenderAuthIdentifyVerificationRcptHook implements @Override public HookResult doRcpt(SMTPSession session, MaybeSender sender, MailAddress rcpt) { if (session.getUser() != null) { - String authUser = (session.getUser()).toLowerCase(Locale.US); - MailAddress senderAddress = (MailAddress) session.getAttachment( - SMTPSession.SENDER, ProtocolSession.State.Transaction); - String username = retrieveSender(sender, senderAddress); + MaybeSender senderAddress = (MaybeSender) session.getAttachment(SMTPSession.SENDER, ProtocolSession.State.Transaction); // Check if the sender address is the same as the user which was used to authenticate. // Its important to ignore case here to fix JAMES-837. This is save todo because if the handler is called // the user was already authenticated - if ((senderAddress == null) - || (!authUser.equalsIgnoreCase(username)) - || (!isLocalDomain(senderAddress.getDomain()))) { + if (isAnonymous(sender) + || !senderMatchSessionUser(sender, session) + || !belongsToLocalDomain(senderAddress)) { return INVALID_AUTH; } } return HookResult.DECLINED; } - public String retrieveSender(MaybeSender sender, MailAddress senderAddress) { - if (senderAddress != null && !sender.isNullSender()) { - return getUser(senderAddress); - } - return null; + private boolean isAnonymous(MaybeSender maybeSender) { + return maybeSender == null || maybeSender.isNullSender(); + } + + private boolean senderMatchSessionUser(MaybeSender maybeSender, SMTPSession session) { + Preconditions.checkArgument(!maybeSender.isNullSender()); + + String authUser = session.getUser().toLowerCase(Locale.US); + String username = getUser(maybeSender.get()); + + return username.equals(authUser); + } + + private boolean belongsToLocalDomain(MaybeSender maybeSender) { + Preconditions.checkArgument(!maybeSender.isNullSender()); + return isLocalDomain(maybeSender.get().getDomain()); } /** http://git-wip-us.apache.org/repos/asf/james-project/blob/0bdffec1/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/DataCmdHandler.java ---------------------------------------------------------------------- diff --git a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/DataCmdHandler.java b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/DataCmdHandler.java index 00f1128..de3d001 100644 --- a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/DataCmdHandler.java +++ b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/DataCmdHandler.java @@ -172,8 +172,8 @@ public class DataCmdHandler implements CommandHandler<SMTPSession>, ExtensibleHa */ @SuppressWarnings("unchecked") protected Response doDATA(SMTPSession session, String argument) { - MailAddress sender = (MailAddress) session.getAttachment(SMTPSession.SENDER, ProtocolSession.State.Transaction); - MailEnvelope env = createEnvelope(session, MaybeSender.of(sender), new ArrayList<>((Collection<MailAddress>) session.getAttachment(SMTPSession.RCPT_LIST, ProtocolSession.State.Transaction))); + MaybeSender sender = (MaybeSender) session.getAttachment(SMTPSession.SENDER, ProtocolSession.State.Transaction); + MailEnvelope env = createEnvelope(session, sender, new ArrayList<>((Collection<MailAddress>) session.getAttachment(SMTPSession.RCPT_LIST, ProtocolSession.State.Transaction))); session.setAttachment(MAILENV, env,ProtocolSession.State.Transaction); session.pushLineHandler(lineHandler); http://git-wip-us.apache.org/repos/asf/james-project/blob/0bdffec1/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/MailCmdHandler.java ---------------------------------------------------------------------- diff --git a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/MailCmdHandler.java b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/MailCmdHandler.java index 5a18476..4ba8374 100644 --- a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/MailCmdHandler.java +++ b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/MailCmdHandler.java @@ -115,13 +115,12 @@ public class MailCmdHandler extends AbstractHookableCmdHandler<MailHook> { */ private Response doMAIL(SMTPSession session, String argument) { StringBuilder responseBuffer = new StringBuilder(); - MailAddress sender = (MailAddress) session.getAttachment( - SMTPSession.SENDER, State.Transaction); + MaybeSender sender = (MaybeSender) session.getAttachment(SMTPSession.SENDER, State.Transaction); responseBuffer.append( DSNStatus.getStatus(DSNStatus.SUCCESS, DSNStatus.ADDRESS_OTHER)) .append(" Sender <"); if (sender != null) { - responseBuffer.append(sender); + responseBuffer.append(sender.asString()); } responseBuffer.append("> OK"); return new SMTPResponse(SMTPRetCode.MAIL_OK, responseBuffer); @@ -214,7 +213,7 @@ public class MailCmdHandler extends AbstractHookableCmdHandler<MailHook> { LOGGER.info("Error parsing sender address: {}: did not start and end with < >", sender); return SYNTAX_ERROR; } - MailAddress senderAddress = null; + MaybeSender senderAddress = MaybeSender.nullSender(); if (session.getConfiguration().useAddressBracketsEnforcement() || (sender.startsWith("<") && sender.endsWith(">"))) { @@ -223,7 +222,7 @@ public class MailCmdHandler extends AbstractHookableCmdHandler<MailHook> { } if (sender.length() == 0) { - // This is the <> case. Let senderAddress == null + // This is the <> case. Let senderAddress == MaybeSender.nullSender() } else { if (!sender.contains("@")) { @@ -233,34 +232,40 @@ public class MailCmdHandler extends AbstractHookableCmdHandler<MailHook> { } try { - senderAddress = new MailAddress(sender); + senderAddress = MaybeSender.of(new MailAddress(sender)); } catch (Exception pe) { LOGGER.info("Error parsing sender address: {}", sender, pe); return SYNTAX_ERROR_ADDRESS; } } - if ((senderAddress == null) || - ((senderAddress.getLocalPart().length() == 0) && (senderAddress.getDomain().name().length() == 0))) { - senderAddress = MailAddress.nullSender(); + if (isNullSender(senderAddress)) { + senderAddress = MaybeSender.nullSender(); } // Store the senderAddress in session map session.setAttachment(SMTPSession.SENDER, senderAddress, State.Transaction); } return null; } - + + private boolean isNullSender(MaybeSender senderAddress) { + if (senderAddress.isNullSender()) { + return true; + } + boolean hasEmptyLocalPart = senderAddress.get().getLocalPart().length() == 0; + boolean hasEmptyDomainPart = senderAddress.get().getDomain().name().length() == 0; + return hasEmptyLocalPart && hasEmptyDomainPart; + } + @Override protected Class<MailHook> getHookInterface() { return MailHook.class; } - @Override protected HookResult callHook(MailHook rawHook, SMTPSession session, String parameters) { - MailAddress sender = (MailAddress) session.getAttachment(SMTPSession.SENDER, State.Transaction); - return rawHook.doMail(session, MaybeSender.of(sender)); + MaybeSender sender = (MaybeSender) session.getAttachment(SMTPSession.SENDER, State.Transaction); + return rawHook.doMail(session, sender); } - @Override public List<Class<?>> getMarkerInterfaces() { http://git-wip-us.apache.org/repos/asf/james-project/blob/0bdffec1/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/RcptCmdHandler.java ---------------------------------------------------------------------- diff --git a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/RcptCmdHandler.java b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/RcptCmdHandler.java index 2640564..c8fc16b 100644 --- a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/RcptCmdHandler.java +++ b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/RcptCmdHandler.java @@ -211,9 +211,9 @@ public class RcptCmdHandler extends AbstractHookableCmdHandler<RcptHook> impleme } else if (null != recipient) { sb.append(" [to:").append(recipient).append(']'); } - if (null != session.getAttachment(SMTPSession.SENDER, State.Transaction)) { - MailAddress mailAddress = (MailAddress) session.getAttachment(SMTPSession.SENDER, State.Transaction); - sb.append(" [from:").append(mailAddress.asString()).append(']'); + MaybeSender sender = (MaybeSender) session.getAttachment(SMTPSession.SENDER, State.Transaction); + if (null != sender && !sender.isNullSender()) { + sb.append(" [from:").append(sender.asString()).append(']'); } return sb.toString(); } @@ -230,9 +230,8 @@ public class RcptCmdHandler extends AbstractHookableCmdHandler<RcptHook> impleme @Override protected HookResult callHook(RcptHook rawHook, SMTPSession session, String parameters) { - MailAddress sender = (MailAddress) session.getAttachment(SMTPSession.SENDER, State.Transaction); - return rawHook.doRcpt(session, - MaybeSender.of(sender), + MaybeSender sender = (MaybeSender) session.getAttachment(SMTPSession.SENDER, State.Transaction); + return rawHook.doRcpt(session, sender, (MailAddress) session.getAttachment(CURRENT_RECIPIENT, State.Transaction)); } http://git-wip-us.apache.org/repos/asf/james-project/blob/0bdffec1/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/MailSizeEsmtpExtension.java ---------------------------------------------------------------------- diff --git a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/MailSizeEsmtpExtension.java b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/MailSizeEsmtpExtension.java index 19f6012..02e63d8 100644 --- a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/MailSizeEsmtpExtension.java +++ b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/MailSizeEsmtpExtension.java @@ -26,6 +26,7 @@ import java.util.List; import org.apache.commons.configuration.Configuration; import org.apache.commons.configuration.ConfigurationException; +import org.apache.james.core.MaybeSender; import org.apache.james.protocols.api.ProtocolSession.State; import org.apache.james.protocols.api.Response; import org.apache.james.protocols.api.handler.LineHandler; @@ -79,8 +80,8 @@ public class MailSizeEsmtpExtension implements MailParametersHook, EhloExtension @Override public HookResult doMailParameter(SMTPSession session, String paramName, String paramValue) { - return doMailSize(session, paramValue, - (String) session.getAttachment(SMTPSession.SENDER, State.Transaction)); + MaybeSender tempSender = (MaybeSender) session.getAttachment(SMTPSession.SENDER, State.Transaction); + return doMailSize(session, paramValue, tempSender); } @Override @@ -114,7 +115,7 @@ public class MailSizeEsmtpExtension implements MailParametersHook, EhloExtension * @return true if further options should be processed, false otherwise */ private HookResult doMailSize(SMTPSession session, - String mailOptionValue, String tempSender) { + String mailOptionValue, MaybeSender tempSender) { int size = 0; try { size = Integer.parseInt(mailOptionValue); @@ -128,7 +129,11 @@ public class MailSizeEsmtpExtension implements MailParametersHook, EhloExtension long maxMessageSize = session.getConfiguration().getMaxMessageSize(); if ((maxMessageSize > 0) && (size > maxMessageSize)) { // Let the client know that the size limit has been hit. - LOGGER.error("Rejected message from {} from {} of size {} exceeding system maximum message size of {} based on SIZE option.", (tempSender != null ? tempSender : null), session.getRemoteAddress().getAddress().getHostAddress(), size, maxMessageSize); + LOGGER.error("Rejected message from {} to {} of size {} exceeding system maximum message size of {} based on SIZE option.", + tempSender, + session.getRemoteAddress().getAddress().getHostAddress(), + size, + maxMessageSize); return QUOTA_EXCEEDED; } else { http://git-wip-us.apache.org/repos/asf/james-project/blob/0bdffec1/protocols/smtp/src/test/java/org/apache/james/protocols/smtp/core/fastfail/ValidSenderDomainHandlerTest.java ---------------------------------------------------------------------- diff --git a/protocols/smtp/src/test/java/org/apache/james/protocols/smtp/core/fastfail/ValidSenderDomainHandlerTest.java b/protocols/smtp/src/test/java/org/apache/james/protocols/smtp/core/fastfail/ValidSenderDomainHandlerTest.java index 3d5da19..71cd15e 100644 --- a/protocols/smtp/src/test/java/org/apache/james/protocols/smtp/core/fastfail/ValidSenderDomainHandlerTest.java +++ b/protocols/smtp/src/test/java/org/apache/james/protocols/smtp/core/fastfail/ValidSenderDomainHandlerTest.java @@ -25,7 +25,6 @@ import java.util.HashMap; import java.util.Map; import org.apache.commons.configuration.Configuration; -import org.apache.commons.configuration.ConfigurationException; import org.apache.james.core.MailAddress; import org.apache.james.core.MaybeSender; import org.apache.james.protocols.api.ProtocolSession.State; @@ -40,7 +39,7 @@ public class ValidSenderDomainHandlerTest { return new ValidSenderDomainHandler() { @Override - public void init(Configuration config) throws ConfigurationException { + public void init(Configuration config) { } @@ -66,7 +65,7 @@ public class ValidSenderDomainHandlerTest { @Override public Map<String,Object> getState() { - map.put(SMTPSession.SENDER, sender); + map.put(SMTPSession.SENDER, MaybeSender.of(sender)); return map; } @@ -116,8 +115,8 @@ public class ValidSenderDomainHandlerTest { public void testInvalidSenderDomainReject() throws Exception { ValidSenderDomainHandler handler = createHandler(); SMTPSession session = setupMockedSession(new MailAddress("invalid@invalid")); - MailAddress sender = (MailAddress) session.getAttachment(SMTPSession.SENDER, State.Transaction); - HookReturnCode response = handler.doMail(session, MaybeSender.of(sender)).getResult(); + MaybeSender sender = (MaybeSender) session.getAttachment(SMTPSession.SENDER, State.Transaction); + HookReturnCode response = handler.doMail(session, sender).getResult(); assertThat(HookReturnCode.deny()).describedAs("Blocked cause we use reject action").isEqualTo(response); } http://git-wip-us.apache.org/repos/asf/james-project/blob/0bdffec1/server/container/core/src/main/java/org/apache/james/server/core/MailImpl.java ---------------------------------------------------------------------- diff --git a/server/container/core/src/main/java/org/apache/james/server/core/MailImpl.java b/server/container/core/src/main/java/org/apache/james/server/core/MailImpl.java index 9eaf52a..5720c0a 100644 --- a/server/container/core/src/main/java/org/apache/james/server/core/MailImpl.java +++ b/server/container/core/src/main/java/org/apache/james/server/core/MailImpl.java @@ -417,9 +417,13 @@ public class MailImpl implements Disposable, Mail { * @param recipients the collection of recipients of this MailImpl */ public MailImpl(String name, MailAddress sender, Collection<MailAddress> recipients) { + this(name, Optional.ofNullable(sender), recipients); + } + + public MailImpl(String name, Optional<MailAddress> sender, Collection<MailAddress> recipients) { this(); setName(name); - setSender(sender); + sender.ifPresent(this::setSender); // Copy the recipient list if (recipients != null) { @@ -427,7 +431,7 @@ public class MailImpl implements Disposable, Mail { } } - @SuppressWarnings({"unchecked", "deprecation"}) + @SuppressWarnings({"unchecked", "deprecated"}) private MailImpl(Mail mail, String newName) throws MessagingException { this(newName, mail.getSender(), mail.getRecipients(), mail.getMessage()); setRemoteHost(mail.getRemoteHost()); http://git-wip-us.apache.org/repos/asf/james-project/blob/0bdffec1/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java ---------------------------------------------------------------------- diff --git a/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java b/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java index b90d603..de9a3ca 100644 --- a/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java +++ b/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java @@ -25,6 +25,7 @@ import java.io.OutputStream; import java.nio.ByteBuffer; import java.util.LinkedList; import java.util.List; +import java.util.Optional; import javax.mail.MessagingException; @@ -99,9 +100,9 @@ public class DataLineJamesMessageHookHandler implements DataLineFilter, Extensib @SuppressWarnings("unchecked") List<MailAddress> recipientCollection = (List<MailAddress>) session.getAttachment(SMTPSession.RCPT_LIST, State.Transaction); - MailAddress mailAddress = (MailAddress) session.getAttachment(SMTPSession.SENDER, State.Transaction); + MaybeSender sender = (MaybeSender) session.getAttachment(SMTPSession.SENDER, State.Transaction); - MailImpl mail = new MailImpl(MailImpl.getId(), mailAddress, recipientCollection); + MailImpl mail = new MailImpl(MailImpl.getId(), Optional.ofNullable(sender).flatMap(MaybeSender::asOptional), recipientCollection); // store mail in the session so we can be sure it get disposed later session.setAttachment(SMTPConstants.MAIL, mail, State.Transaction); http://git-wip-us.apache.org/repos/asf/james-project/blob/0bdffec1/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SpamAssassinHandlerTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SpamAssassinHandlerTest.java b/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SpamAssassinHandlerTest.java index eb56867..db8b783 100644 --- a/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SpamAssassinHandlerTest.java +++ b/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SpamAssassinHandlerTest.java @@ -24,8 +24,11 @@ import static org.assertj.core.api.Assertions.assertThat; import java.util.HashMap; import javax.mail.MessagingException; +import javax.mail.internet.AddressException; import javax.mail.internet.MimeMessage; +import org.apache.james.core.MailAddress; +import org.apache.james.core.MaybeSender; import org.apache.james.core.builder.MimeMessageBuilder; import org.apache.james.metrics.api.NoopMetricFactory; import org.apache.james.protocols.smtp.SMTPSession; @@ -72,7 +75,11 @@ public class SpamAssassinHandlerTest { @Override public Object getAttachment(String key, State state) { - sstate.put(SMTPSession.SENDER, "sen...@james.apache.org"); + try { + sstate.put(SMTPSession.SENDER, MaybeSender.of(new MailAddress("sen...@james.apache.org"))); + } catch (AddressException e) { + throw new RuntimeException(e); + } if (state == State.Connection) { return connectionState.get(key); } else { http://git-wip-us.apache.org/repos/asf/james-project/blob/0bdffec1/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/URIRBLHandlerTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/URIRBLHandlerTest.java b/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/URIRBLHandlerTest.java index 93edf95..0a632e8 100644 --- a/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/URIRBLHandlerTest.java +++ b/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/URIRBLHandlerTest.java @@ -29,8 +29,11 @@ import java.util.HashMap; import java.util.List; import javax.mail.MessagingException; +import javax.mail.internet.AddressException; import javax.mail.internet.MimeMessage; +import org.apache.james.core.MailAddress; +import org.apache.james.core.MaybeSender; import org.apache.james.core.builder.MimeMessageBuilder; import org.apache.james.dnsservice.api.DNSService; import org.apache.james.dnsservice.api.mock.MockDNSService; @@ -78,7 +81,11 @@ public class URIRBLHandlerTest { @Override public Object getAttachment(String key, State state) { - sstate.put(SMTPSession.SENDER, "sen...@james.apache.org"); + try { + sstate.put(SMTPSession.SENDER, MaybeSender.of(new MailAddress("sen...@james.apache.org"))); + } catch (AddressException e) { + throw new RuntimeException(e); + } if (state == State.Connection) { return connectionState.get(key); --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org