JAMES-2341 Refactor SpamAssassinInvoker a bit
Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/2798ed91 Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/2798ed91 Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/2798ed91 Branch: refs/heads/master Commit: 2798ed913dbf419a235e6b2c185fe72c6218c0da Parents: 1699212 Author: Antoine Duprat <[email protected]> Authored: Mon Feb 26 15:42:02 2018 +0100 Committer: Antoine Duprat <[email protected]> Committed: Thu Mar 8 10:36:33 2018 +0100 ---------------------------------------------------------------------- .../james/util/scanner/SpamAssassinInvoker.java | 130 +++++++------------ .../james/util/scanner/SpamAssassinResult.java | 95 ++++++++++++++ .../util/scanner/SpamAssassinInvokerTest.java | 27 ++-- .../util/scanner/SpamAssassinResultTest.java | 67 ++++++++++ .../james/transport/mailets/SpamAssassin.java | 7 +- .../fastfail/SpamAssassinHandler.java | 9 +- 6 files changed, 226 insertions(+), 109 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/2798ed91/server/container/util/src/main/java/org/apache/james/util/scanner/SpamAssassinInvoker.java ---------------------------------------------------------------------- diff --git a/server/container/util/src/main/java/org/apache/james/util/scanner/SpamAssassinInvoker.java b/server/container/util/src/main/java/org/apache/james/util/scanner/SpamAssassinInvoker.java index 871ea51..c9429e9 100644 --- a/server/container/util/src/main/java/org/apache/james/util/scanner/SpamAssassinInvoker.java +++ b/server/container/util/src/main/java/org/apache/james/util/scanner/SpamAssassinInvoker.java @@ -23,16 +23,16 @@ import java.io.BufferedReader; import java.io.IOException; import java.io.InputStreamReader; import java.io.OutputStream; +import java.io.PrintWriter; import java.net.Socket; import java.net.UnknownHostException; -import java.util.HashMap; -import java.util.Map; -import java.util.StringTokenizer; +import java.util.List; import javax.mail.MessagingException; import javax.mail.internet.MimeMessage; -import com.google.common.io.Closeables; +import com.google.common.base.Splitter; +import com.google.common.collect.Lists; /** * Sends the message through daemonized SpamAssassin (spamd), visit <a @@ -46,16 +46,14 @@ public class SpamAssassinInvoker { /** The mail attribute under which the flag get stored */ public static final String FLAG_MAIL_ATTRIBUTE_NAME = "org.apache.james.spamassassin.flag"; + private static final int SPAM_INDEX = 1; + private static final int HITS_INDEX = 3; + private static final int REQUIRED_HITS_INDEX = 5; + private final String spamdHost; private final int spamdPort; - private String hits = "?"; - - private String required = "?"; - - private final Map<String, String> headers = new HashMap<>(); - /** * Init the spamassassin invoker * @@ -78,96 +76,60 @@ public class SpamAssassinInvoker { * @throws MessagingException * if an error on scanning is detected */ - public boolean scanMail(MimeMessage message) throws MessagingException { - Socket socket = null; - OutputStream out = null; - BufferedReader in = null; - - try { - socket = new Socket(spamdHost, spamdPort); + public SpamAssassinResult scanMail(MimeMessage message) throws MessagingException { + try (Socket socket = new Socket(spamdHost, spamdPort); + OutputStream out = socket.getOutputStream(); + PrintWriter writer = new PrintWriter(out); + BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream()))) { - out = socket.getOutputStream(); - in = new BufferedReader(new InputStreamReader(socket.getInputStream())); - out.write("CHECK SPAMC/1.2\r\n\r\n".getBytes()); + writer.write("CHECK SPAMC/1.2\r\n\r\n"); + writer.flush(); // pass the message to spamd message.writeTo(out); out.flush(); socket.shutdownOutput(); - String s; - while ((s = in.readLine()) != null) { - if (s.startsWith("Spam:")) { - StringTokenizer t = new StringTokenizer(s, " "); - boolean spam; - try { - t.nextToken(); - spam = Boolean.valueOf(t.nextToken()); - } catch (Exception e) { - // On exception return flase - return false; - } - t.nextToken(); - hits = t.nextToken(); - t.nextToken(); - required = t.nextToken(); - - if (spam) { - // message was spam - headers.put(FLAG_MAIL_ATTRIBUTE_NAME, "YES"); - headers.put(STATUS_MAIL_ATTRIBUTE_NAME, "Yes, hits=" + hits + " required=" + required); - - // spam detected - return true; - } else { - // add headers - headers.put(FLAG_MAIL_ATTRIBUTE_NAME, "NO"); - headers.put(STATUS_MAIL_ATTRIBUTE_NAME, "No, hits=" + hits + " required=" + required); - - return false; - } - } - } - return false; + + return in.lines() + .filter(this::isSpam) + .map(this::processSpam) + .findFirst() + .orElse(SpamAssassinResult.empty()); } catch (UnknownHostException e1) { throw new MessagingException("Error communicating with spamd. Unknown host: " + spamdHost); } catch (IOException | MessagingException e1) { throw new MessagingException("Error communicating with spamd on " + spamdHost + ":" + spamdPort + " Exception: " + e1); - } finally { - try { - Closeables.close(in, true); - Closeables.close(out, true); - socket.close(); - } catch (Exception e) { - // Should never happen - } - } } - /** - * Return the hits which was returned by spamd - * - * @return hits The hits which was detected - */ - public String getHits() { - return hits; + private SpamAssassinResult processSpam(String line) { + List<String> elements = Lists.newArrayList(Splitter.on(' ').split(line)); + boolean spam = spam(elements.get(SPAM_INDEX)); + String hits = elements.get(HITS_INDEX); + String required = elements.get(REQUIRED_HITS_INDEX); + SpamAssassinResult.Builder builder = SpamAssassinResult.builder() + .hits(hits) + .requiredHits(required); + + if (spam) { + builder.putHeader(FLAG_MAIL_ATTRIBUTE_NAME, "YES"); + builder.putHeader(STATUS_MAIL_ATTRIBUTE_NAME, "Yes, hits=" + hits + " required=" + required); + } else { + builder.putHeader(FLAG_MAIL_ATTRIBUTE_NAME, "NO"); + builder.putHeader(STATUS_MAIL_ATTRIBUTE_NAME, "No, hits=" + hits + " required=" + required); + } + return builder.build(); } - /** - * Return the required hits - * - * @return required The required hits before a message is handled as spam - */ - public String getRequiredHits() { - return required; + private boolean spam(String string) { + try { + return Boolean.valueOf(string); + } catch (Exception e) { + return false; + } } - /** - * Return the headers as attributes which spamd generates - * - * @return headers Map of headers to add as attributes - */ - public Map<String, String> getHeadersAsAttribute() { - return headers; + private boolean isSpam(String line) { + return line.startsWith("Spam:"); } } http://git-wip-us.apache.org/repos/asf/james-project/blob/2798ed91/server/container/util/src/main/java/org/apache/james/util/scanner/SpamAssassinResult.java ---------------------------------------------------------------------- diff --git a/server/container/util/src/main/java/org/apache/james/util/scanner/SpamAssassinResult.java b/server/container/util/src/main/java/org/apache/james/util/scanner/SpamAssassinResult.java new file mode 100644 index 0000000..43d4dff --- /dev/null +++ b/server/container/util/src/main/java/org/apache/james/util/scanner/SpamAssassinResult.java @@ -0,0 +1,95 @@ +/**************************************************************** + * 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.util.scanner; + +import java.util.Map; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; + +public class SpamAssassinResult { + + private static final String NO_RESULT = "?"; + + public static SpamAssassinResult empty() { + return new Builder() + .hits(NO_RESULT) + .requiredHits(NO_RESULT) + .build(); + } + + public static Builder builder() { + return new Builder(); + } + + public static class Builder { + + private String hits; + private String requiredHits; + private ImmutableMap.Builder<String, String> headersAsAttribute; + + private Builder() { + headersAsAttribute = ImmutableMap.builder(); + } + + public Builder hits(String hits) { + this.hits = hits; + return this; + } + + public Builder requiredHits(String requiredHits) { + this.requiredHits = requiredHits; + return this; + } + + public Builder putHeader(String name, String value) { + this.headersAsAttribute.put(name, value); + return this; + } + + public SpamAssassinResult build() { + Preconditions.checkNotNull(hits); + Preconditions.checkNotNull(requiredHits); + return new SpamAssassinResult(hits, requiredHits, headersAsAttribute.build()); + } + } + + private final String hits; + private final String requiredHits; + private final Map<String, String> headersAsAttribute; + + private SpamAssassinResult(String hits, String requiredHits, Map<String, String> headersAsAttribute) { + this.hits = hits; + this.requiredHits = requiredHits; + this.headersAsAttribute = headersAsAttribute; + } + + public String getHits() { + return hits; + } + + public String getRequiredHits() { + return requiredHits; + } + + public Map<String, String> getHeadersAsAttribute() { + return headersAsAttribute; + } + +} http://git-wip-us.apache.org/repos/asf/james-project/blob/2798ed91/server/container/util/src/test/java/org/apache/james/util/scanner/SpamAssassinInvokerTest.java ---------------------------------------------------------------------- diff --git a/server/container/util/src/test/java/org/apache/james/util/scanner/SpamAssassinInvokerTest.java b/server/container/util/src/test/java/org/apache/james/util/scanner/SpamAssassinInvokerTest.java index 4d37fc3..3e3fd4b 100644 --- a/server/container/util/src/test/java/org/apache/james/util/scanner/SpamAssassinInvokerTest.java +++ b/server/container/util/src/test/java/org/apache/james/util/scanner/SpamAssassinInvokerTest.java @@ -41,40 +41,31 @@ public class SpamAssassinInvokerTest { } @Test - public void getHitsShouldReturnQuestionMarkByDefault() { - assertThat(testee.getHits()).isEqualTo("?"); - } - - @Test public void scanMailShouldModifyHitsField() throws Exception { MimeMessage mimeMessage = MimeMessageUtil.mimeMessageFromStream( ClassLoader.getSystemResourceAsStream("eml/spam.eml")); - testee.scanMail(mimeMessage); + SpamAssassinResult result = testee.scanMail(mimeMessage); - assertThat(testee.getHits()).isEqualTo("0.4"); - } - - @Test - public void getRequiredHitsShouldReturnQuestionMarkByDefault() { - assertThat(testee.getRequiredHits()).isEqualTo("?"); + // The result is varying from 0.4 to 0.0 + assertThat(result.getHits()).startsWith("0."); } @Test public void scanMailShouldModifyRequiredHitsField() throws Exception { MimeMessage mimeMessage = MimeMessageUtil.mimeMessageFromStream( ClassLoader.getSystemResourceAsStream("eml/spam.eml")); - testee.scanMail(mimeMessage); + SpamAssassinResult result = testee.scanMail(mimeMessage); - assertThat(testee.getRequiredHits()).isEqualTo("5.0"); + assertThat(result.getRequiredHits()).isEqualTo("5.0"); } @Test public void scanMailShouldModifyHeadersField() throws Exception { MimeMessage mimeMessage = MimeMessageUtil.mimeMessageFromStream( ClassLoader.getSystemResourceAsStream("eml/spam.eml")); - testee.scanMail(mimeMessage); + SpamAssassinResult result = testee.scanMail(mimeMessage); - assertThat(testee.getHeadersAsAttribute()).isNotEmpty(); + assertThat(result.getHeadersAsAttribute()).isNotEmpty(); } @Test @@ -84,8 +75,8 @@ public class SpamAssassinInvokerTest { MimeMessage mimeMessage = MimeMessageUtil.mimeMessageFromStream( ClassLoader.getSystemResourceAsStream("spamassassin_db/spam/spam1")); - testee.scanMail(mimeMessage); + SpamAssassinResult result = testee.scanMail(mimeMessage); - assertThat(testee.getHeadersAsAttribute().get(SpamAssassinInvoker.FLAG_MAIL_ATTRIBUTE_NAME)).isEqualTo("YES"); + assertThat(result.getHeadersAsAttribute().get(SpamAssassinInvoker.FLAG_MAIL_ATTRIBUTE_NAME)).isEqualTo("YES"); } } http://git-wip-us.apache.org/repos/asf/james-project/blob/2798ed91/server/container/util/src/test/java/org/apache/james/util/scanner/SpamAssassinResultTest.java ---------------------------------------------------------------------- diff --git a/server/container/util/src/test/java/org/apache/james/util/scanner/SpamAssassinResultTest.java b/server/container/util/src/test/java/org/apache/james/util/scanner/SpamAssassinResultTest.java new file mode 100644 index 0000000..1fe3dc2 --- /dev/null +++ b/server/container/util/src/test/java/org/apache/james/util/scanner/SpamAssassinResultTest.java @@ -0,0 +1,67 @@ +/**************************************************************** + * 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.util.scanner; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.assertj.core.data.MapEntry; +import org.junit.Test; + +public class SpamAssassinResultTest { + + @Test + public void buildShouldThrowWhenHitsIsNotGiven() { + assertThatThrownBy(() -> SpamAssassinResult.builder() + .build()) + .isInstanceOf(NullPointerException.class); + + } + + @Test + public void buildShouldThrowWhenRequiredHitsIsNotGiven() { + assertThatThrownBy(() -> SpamAssassinResult.builder() + .hits("1.0") + .build()) + .isInstanceOf(NullPointerException.class); + + } + + @Test + public void buildShouldWork() { + String hits = "1.1"; + String requiredHits = "5.0"; + String name = "header"; + String value = "value"; + String name2 = "header2"; + String value2 = "value2"; + SpamAssassinResult spamAssassinResult = SpamAssassinResult.builder() + .hits(hits) + .requiredHits(requiredHits) + .putHeader(name, value) + .putHeader(name2, value2) + .build(); + + assertThat(spamAssassinResult.getHits()).isEqualTo(hits); + assertThat(spamAssassinResult.getRequiredHits()).isEqualTo(requiredHits); + assertThat(spamAssassinResult.getHeadersAsAttribute()).containsOnly( + MapEntry.entry(name, value), + MapEntry.entry(name2, value2)); + } +} http://git-wip-us.apache.org/repos/asf/james-project/blob/2798ed91/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SpamAssassin.java ---------------------------------------------------------------------- diff --git a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SpamAssassin.java b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SpamAssassin.java index 82b4817..fd7b15c 100644 --- a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SpamAssassin.java +++ b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SpamAssassin.java @@ -26,6 +26,7 @@ import javax.mail.internet.MimeMessage; import org.apache.james.util.Port; import org.apache.james.util.scanner.SpamAssassinInvoker; +import org.apache.james.util.scanner.SpamAssassinResult; import org.apache.mailet.Mail; import org.apache.mailet.base.GenericMailet; import org.apache.mailet.base.MailetUtil; @@ -86,11 +87,11 @@ public class SpamAssassin extends GenericMailet { // Invoke SpamAssassin connection and scan the message SpamAssassinInvoker sa = new SpamAssassinInvoker(spamdHost, spamdPort); - sa.scanMail(message); + SpamAssassinResult result = sa.scanMail(message); // Add headers as attribute to mail object - for (String key : sa.getHeadersAsAttribute().keySet()) { - mail.setAttribute(key, sa.getHeadersAsAttribute().get(key)); + for (String key : result.getHeadersAsAttribute().keySet()) { + mail.setAttribute(key, result.getHeadersAsAttribute().get(key)); } message.saveChanges(); http://git-wip-us.apache.org/repos/asf/james-project/blob/2798ed91/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/fastfail/SpamAssassinHandler.java ---------------------------------------------------------------------- diff --git a/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/fastfail/SpamAssassinHandler.java b/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/fastfail/SpamAssassinHandler.java index a49db81..3ef7774 100644 --- a/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/fastfail/SpamAssassinHandler.java +++ b/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/fastfail/SpamAssassinHandler.java @@ -32,6 +32,7 @@ import org.apache.james.protocols.smtp.hook.HookResult; import org.apache.james.protocols.smtp.hook.HookReturnCode; import org.apache.james.smtpserver.JamesMessageHook; import org.apache.james.util.scanner.SpamAssassinInvoker; +import org.apache.james.util.scanner.SpamAssassinResult; import org.apache.mailet.Mail; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -117,17 +118,17 @@ public class SpamAssassinHandler implements JamesMessageHook, ProtocolHandler { try { MimeMessage message = mail.getMessage(); SpamAssassinInvoker sa = new SpamAssassinInvoker(spamdHost, spamdPort); - sa.scanMail(message); + SpamAssassinResult result = sa.scanMail(message); // Add the headers - for (String key : sa.getHeadersAsAttribute().keySet()) { - mail.setAttribute(key, sa.getHeadersAsAttribute().get(key)); + for (String key : result.getHeadersAsAttribute().keySet()) { + mail.setAttribute(key, result.getHeadersAsAttribute().get(key)); } // Check if rejectionHits was configured if (spamdRejectionHits > 0) { try { - double hits = Double.parseDouble(sa.getHits()); + double hits = Double.parseDouble(result.getHits()); // if the hits are bigger the rejectionHits reject the // message --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
