Move allowrcpt handling up to EmailSender to fix empty envelope

If nobody on the SMTP envelope was actually on the allowrcpt list,
we were still trying to push a message into the SMTP server but
it had no destinations.  All SMTP servers refuse to accept the
'DATA' command in this case, as there was no 'RCPT TO' before it.

The breakage occurred when I stopped CC'ing the sender of emails
by default.  This meant that I was no longer on the RCPT TO list
for a message in my test environment, but that was the only valid
address in the sendemail.allowrcpt configuration variable.

Change-Id: I2e1810d3e2a8e82aa96c93f2cfd9849f7bed6e8a
Signed-off-by: Shawn O. Pearce <sop@google.com>

4 files changed, 62 insertions, 62 deletions

diff --git a/gerrit-patch-commonsnet/src/main/java/org/apache/commons/net/smtp/AuthSMTPClient.java b/gerrit-patch-commonsnet/src/main/java/org/apache/commons/net/smtp/AuthSMTPClient.java
index e380df431e..4db7de631c 100644
--- a/gerrit-patch-commonsnet/src/main/java/org/apache/commons/net/smtp/AuthSMTPClient.java
+++ b/gerrit-patch-commonsnet/src/main/java/org/apache/commons/net/smtp/AuthSMTPClient.java
@@ -17,8 +17,6 @@ package org.apache.commons.net.smtp;
 import com.google.gerrit.util.ssl.BlindSSLSocketFactory;
 
 import org.apache.commons.codec.binary.Base64;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.io.UnsupportedEncodingException;
@@ -26,20 +24,16 @@ import java.net.SocketException;
 import java.security.InvalidKeyException;
 import java.security.NoSuchAlgorithmException;
 import java.util.Arrays;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Set;
 
 import javax.crypto.Mac;
 import javax.crypto.spec.SecretKeySpec;
 import javax.net.ssl.SSLSocketFactory;
 
 public class AuthSMTPClient extends SMTPClient {
-  private static final Logger log = LoggerFactory.getLogger(AuthSMTPClient.class);
   private static final String UTF_8 = "UTF-8";
 
   private String authTypes;
-  private Set<String> allowedRcptTo;
 
   public AuthSMTPClient(final String charset) {
     super(charset);
@@ -68,45 +62,6 @@ public class AuthSMTPClient extends SMTPClient {
     }
   }
 
-  public void setAllowRcpt(final String[] allowed) {
-    if (allowed != null && allowed.length > 0) {
-      if (allowedRcptTo == null) {
-        allowedRcptTo = new HashSet<String>();
-      }
-      for (final String addr : allowed) {
-        allowedRcptTo.add(addr);
-      }
-    }
-  }
-
-  @Override
-  public int rcpt(final String forwardPath) throws IOException {
-    if (allowRcpt(forwardPath)) {
-      return super.rcpt(forwardPath);
-    } else {
-      log.warn("Not emailing " + forwardPath + " (prohibited by allowrcpt)");
-      return SMTPReply.ACTION_OK;
-    }
-  }
-
-  private boolean allowRcpt(String addr) {
-    if (allowedRcptTo == null) {
-      return true;
-    }
-    if (addr.startsWith("<") && addr.endsWith(">")) {
-      addr = addr.substring(1, addr.length() - 1);
-    }
-    if (allowedRcptTo.contains(addr)) {
-      return true;
-    }
-    final int at = addr.indexOf('@');
-    if (at > 0) {
-      return allowedRcptTo.contains(addr.substring(at))
-          || allowedRcptTo.contains(addr.substring(at + 1));
-    }
-    return false;
-  }
-
   @Override
   public String[] getReplyStrings() {
     return _replyLines.toArray(new String[_replyLines.size()]);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailSender.java
index 411e6ca326..9c4f4c45bd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailSender.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailSender.java
@@ -22,6 +22,14 @@ public interface EmailSender {
   boolean isEnabled();
 
   /**
+   * Can the address receive messages from us?
+   *
+   * @param address the address to consider.
+   * @return true if this sender will deliver to the address.
+   */
+  boolean canEmail(String address);
+
+  /**
    * Sends an email message.
    *
    * @param from who the message is from.
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java
index 4bd6e49ae2..6d042eac45 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java
@@ -45,6 +45,8 @@ import com.google.inject.Inject;
 import com.google.inject.Provider;
 
 import org.eclipse.jgit.util.SystemReader;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.net.MalformedURLException;
 import java.net.URL;
@@ -65,6 +67,8 @@ import javax.annotation.Nullable;
 
 /** Sends an email to one or more interested parties. */
 public abstract class OutgoingEmail {
+  private static final Logger log = LoggerFactory.getLogger(OutgoingEmail.class);
+
   private static final String HDR_TO = "To";
   private static final String HDR_CC = "CC";
 
@@ -180,10 +184,6 @@ public abstract class OutgoingEmail {
           // If they don't want a copy, but we queued one up anyway,
           // drop them from the recipient lists.
           //
-          if (rcptTo.isEmpty()) {
-            return;
-          }
-
           final String fromEmail = fromUser.getPreferredEmail();
           for (Iterator<Address> i = smtpRcptTo.iterator(); i.hasNext();) {
             if (i.next().email.equals(fromEmail)) {
@@ -195,6 +195,10 @@ public abstract class OutgoingEmail {
               ((AddressList) hdr).remove(fromEmail);
             }
           }
+
+          if (smtpRcptTo.isEmpty()) {
+            return;
+          }
         }
       }
 
@@ -564,7 +568,7 @@ public abstract class OutgoingEmail {
       return false;
     }
 
-    if (rcptTo.isEmpty()) {
+    if (smtpRcptTo.isEmpty()) {
       // If we have nobody to send this message to, then all of our
       // selection filters previously for this type of message were
       // unable to match a destination. Don't bother sending it.
@@ -724,14 +728,18 @@ public abstract class OutgoingEmail {
   /** Schedule delivery of this message to the given account. */
   protected void add(final RecipientType rt, final Address addr) {
     if (addr != null && addr.email != null && addr.email.length() > 0) {
-      smtpRcptTo.add(addr);
-      switch (rt) {
-        case TO:
-          ((EmailHeader.AddressList) headers.get(HDR_TO)).add(addr);
-          break;
-        case CC:
-          ((EmailHeader.AddressList) headers.get(HDR_CC)).add(addr);
-          break;
+      if (emailSender.canEmail(addr.email)) {
+        smtpRcptTo.add(addr);
+        switch (rt) {
+          case TO:
+            ((EmailHeader.AddressList) headers.get(HDR_TO)).add(addr);
+            break;
+          case CC:
+            ((EmailHeader.AddressList) headers.get(HDR_CC)).add(addr);
+            break;
+        }
+      } else {
+        log.warn("Not emailing " + addr.email + " (prohibited by allowrcpt)");
       }
     }
   }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/SmtpEmailSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/SmtpEmailSender.java
index 1fb31a037b..d67d3b35f4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/SmtpEmailSender.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/SmtpEmailSender.java
@@ -29,8 +29,11 @@ import java.io.BufferedWriter;
 import java.io.IOException;
 import java.io.Writer;
 import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.Set;
 
 /** Sends email via a nearby SMTP server. */
 @Singleton
@@ -47,7 +50,7 @@ public class SmtpEmailSender implements EmailSender {
   private String smtpPass;
   private Encryption smtpEncryption;
   private boolean sslVerify;
-  private String[] allowrcpt;
+  private Set<String> allowrcpt;
 
   @Inject
   SmtpEmailSender(@GerritServerConfig final Config cfg) {
@@ -79,7 +82,12 @@ public class SmtpEmailSender implements EmailSender {
 
     smtpUser = cfg.getString("sendemail", null, "smtpuser");
     smtpPass = cfg.getString("sendemail", null, "smtppass");
-    allowrcpt = cfg.getStringList("sendemail", null, "allowrcpt");
+
+    Set<String> rcpt = new HashSet<String>();
+    for (String addr : cfg.getStringList("sendemail", null, "allowrcpt")) {
+      rcpt.add(addr);
+    }
+    allowrcpt = Collections.unmodifiableSet(rcpt);
   }
 
   @Override
@@ -88,7 +96,29 @@ public class SmtpEmailSender implements EmailSender {
   }
 
   @Override
-  public void send(final Address from, final Collection<Address> rcpt,
+  public boolean canEmail(String address) {
+    if (!isEnabled()) {
+      return false;
+    }
+
+    if (allowrcpt.isEmpty()) {
+      return true;
+    }
+
+    if (allowrcpt.contains(address)) {
+      return true;
+    }
+
+    String domain = address.substring(address.lastIndexOf('@') + 1);
+    if (allowrcpt.contains(domain) || allowrcpt.contains("@" + domain)) {
+      return true;
+    }
+
+    return false;
+  }
+
+  @Override
+  public void send(final Address from, Collection<Address> rcpt,
       final Map<String, EmailHeader> callerHeaders, final String body)
       throws EmailException {
     if (!isEnabled()) {
@@ -161,7 +191,6 @@ public class SmtpEmailSender implements EmailSender {
 
   private SMTPClient open() throws EmailException {
     final AuthSMTPClient client = new AuthSMTPClient("UTF-8");
-    client.setAllowRcpt(allowrcpt);
 
     if (smtpEncryption == Encryption.SSL) {
       client.enableSSL(sslVerify);