Merge branch 'stable-2.11' into stable-2.12

* stable-2.11:
  OutgoingEmail: Fix breakage introduced in add() method
  ReceiveCommits: Improve error message when failing to insert patch set
  OutgoingEmail: Kill "Skipping delivery of email" logspam
  OutgoingEmail: Check for valid email address when adding recipients
  ldap.Helper: Kill "assuming empty group membership" logspam
  SmtpEmailSender: Refactor open method and improve logging
  CreateEmail: Don't allow to add email prohibited by sendemail.allowrcpt
  RegisterNewEmailSender: Don't unconditionally send verification mail

Change-Id: I139f680d24509348a3d0451f160821d20d5c0542

6 files changed, 42 insertions, 48 deletions

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateEmail.java
index 5c4b9cdc30..a78c23cacb 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateEmail.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateEmail.java
@@ -103,7 +103,8 @@ public class CreateEmail implements RestModifyView<AccountResource, EmailInput>
 
   public Response<EmailInfo> apply(IdentifiedUser user, EmailInput input)
       throws AuthException, BadRequestException, ResourceConflictException,
-      ResourceNotFoundException, OrmException, EmailException {
+      ResourceNotFoundException, OrmException, EmailException,
+      MethodNotAllowedException {
     if (input.email != null && !email.equals(input.email)) {
       throw new BadRequestException("email address must match URL");
     }
@@ -126,7 +127,11 @@ public class CreateEmail implements RestModifyView<AccountResource, EmailInput>
       }
     } else {
       try {
-        registerNewEmailFactory.create(email).send();
+        RegisterNewEmailSender sender = registerNewEmailFactory.create(email);
+        if (!sender.isAllowed()) {
+          throw new MethodNotAllowedException("Not allowed to add email address " + email);
+        }
+        sender.send();
         info.pendingConfirmation = true;
       } catch (EmailException | RuntimeException e) {
         log.error("Cannot send email verification message to " + email, e);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java
index e830cfc7ad..b3ddae07c2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java
@@ -225,8 +225,6 @@ import javax.security.auth.login.LoginException;
         try {
           account = findAccount(schema, ctx, username, false);
         } catch (AccountException e) {
-          LdapRealm.log.warn("Account " + username +
-              " not found, assuming empty group membership");
           return Collections.emptySet();
         }
       }
@@ -248,8 +246,6 @@ import javax.security.auth.login.LoginException;
         try {
           account = findAccount(schema, ctx, username, true);
         } catch (AccountException e) {
-          LdapRealm.log.warn("Account " + username +
-              " not found, assuming empty group membership");
           return Collections.emptySet();
         }
       }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index 4f51f3ed0e..4c59ccbe1b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -763,7 +763,7 @@ public class ReceiveCommits {
         } catch (IOException | RestApiException err) {
           reject(replace.inputCommand, "internal server error");
           log.error(String.format(
-              "Cannot add patch set to %d of %s",
+              "Cannot add patch set to change %d in project %s",
               e.getKey().get(), project.getName()), err);
         }
       } else if (replace.inputCommand.getResult() == NOT_ATTEMPTED) {
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 7309af25de..936eabaf6c 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
@@ -27,6 +27,7 @@ import com.google.gerrit.server.validators.ValidationException;
 import com.google.gwtorm.server.OrmException;
 
 import org.apache.commons.lang.StringUtils;
+import org.apache.commons.validator.routines.EmailValidator;
 import org.apache.velocity.Template;
 import org.apache.velocity.VelocityContext;
 import org.apache.velocity.context.InternalContextAdapterImpl;
@@ -325,7 +326,6 @@ public abstract class OutgoingEmail {
   protected boolean shouldSendMessage() {
     if (body.length() == 0) {
       // If we have no message body, don't send.
-      log.warn("Skipping delivery of email with no body");
       return false;
     }
 
@@ -333,7 +333,6 @@ public abstract class OutgoingEmail {
       // 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.
-      log.warn("Skipping delivery of email with no recipients");
       return false;
     }
 
@@ -383,21 +382,21 @@ 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) {
-      if (args.emailSender.canEmail(addr.email)) {
-        if (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;
-            case BCC:
-              break;
-          }
-        }
-      } else {
+      if (!EmailValidator.getInstance().isValid(addr.email)) {
+        log.warn("Not emailing " + addr.email + " (invalid email address)");
+      } else if (!args.emailSender.canEmail(addr.email)) {
         log.warn("Not emailing " + addr.email + " (prohibited by allowrcpt)");
+      } else if (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;
+          case BCC:
+            break;
+        }
       }
     }
   }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/RegisterNewEmailSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/RegisterNewEmailSender.java
index c4d374f4d6..3e6bd8ba2c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/RegisterNewEmailSender.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/RegisterNewEmailSender.java
@@ -50,11 +50,6 @@ public class RegisterNewEmailSender extends OutgoingEmail {
   }
 
   @Override
-  protected boolean shouldSendMessage() {
-    return true;
-  }
-
-  @Override
   protected void format() throws EmailException {
     appendText(velocifyFile("RegisterNewEmail.vm"));
   }
@@ -70,4 +65,8 @@ public class RegisterNewEmailSender extends OutgoingEmail {
     }
     return emailToken;
   }
+
+  public boolean isAllowed() {
+    return args.emailSender.canEmail(addr);
+  }
 }
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 8bd66334c3..aa572473f5 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
@@ -249,16 +249,19 @@ public class SmtpEmailSender implements EmailSender {
       client.enableSSL(sslVerify);
     }
 
+    client.setConnectTimeout(connectTimeout);
     try {
-      client.setConnectTimeout(connectTimeout);
       client.connect(smtpHost, smtpPort);
-      if (!SMTPReply.isPositiveCompletion(client.getReplyCode())) {
-        throw new EmailException("SMTP server rejected connection");
+      int replyCode = client.getReplyCode();
+      String replyString = client.getReplyString();
+      if (!SMTPReply.isPositiveCompletion(replyCode)) {
+        throw new EmailException(
+            String.format("SMTP server rejected connection: %d: %s",
+                replyCode, replyString));
       }
       if (!client.login()) {
-        String e = client.getReplyString();
         throw new EmailException(
-            "SMTP server rejected HELO/EHLO greeting: " + e);
+            "SMTP server rejected HELO/EHLO greeting: " + replyString);
       }
 
       if (smtpEncryption == Encryption.TLS) {
@@ -266,16 +269,15 @@ public class SmtpEmailSender implements EmailSender {
           throw new EmailException("SMTP server does not support TLS");
         }
         if (!client.login()) {
-          String e = client.getReplyString();
-          throw new EmailException("SMTP server rejected login: " + e);
+          throw new EmailException("SMTP server rejected login: " + replyString);
         }
       }
 
       if (smtpUser != null && !client.auth(smtpUser, smtpPass)) {
-        String e = client.getReplyString();
-        throw new EmailException("SMTP server rejected auth: " + e);
+        throw new EmailException("SMTP server rejected auth: " + replyString);
       }
-    } catch (IOException e) {
+      return client;
+    } catch (IOException | EmailException e) {
       if (client.isConnected()) {
         try {
           client.disconnect();
@@ -283,17 +285,10 @@ public class SmtpEmailSender implements EmailSender {
           //Ignored
         }
       }
-      throw new EmailException(e.getMessage(), e);
-    } catch (EmailException e) {
-      if (client.isConnected()) {
-        try {
-          client.disconnect();
-        } catch (IOException e2) {
-          // Ignored
-        }
+      if (e instanceof EmailException) {
+        throw (EmailException) e;
       }
-      throw e;
+      throw new EmailException(e.getMessage(), e);
     }
-    return client;
   }
 }