From e8f99813a7b0f4e2fa9ac0c48702d26047c4d95b Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 3 Feb 2016 12:39:42 +0900 Subject: [PATCH 1/8] RegisterNewEmailSender: Don't unconditionally send verification mail If the server configuration includes sendemail.allowrcpt, and the user attempts to register an email address that is not included in the allowed list, OutgoingEmail will skip adding that address to the list of recipients. However, RegisterNewEmailSender overrides the 'shouldSendMessage' method to always return true, which means Gerrit still attempts to send the verification mail which has no recipients. This results in an error from the SMTP server: rejected DATA command: 503 5.5.2 Need rcpt command which in turn results in an "Internal server error" message to the user. Remove the overridden shouldSendMessage method, so that the default implementation (which checks for recipients and content) is used. Change-Id: I4f756f98627447a3f23ecdc9fa82309ba0a0271a --- .../google/gerrit/server/mail/RegisterNewEmailSender.java | 5 ----- 1 file changed, 5 deletions(-) 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 eb32700c50..c24997bc70 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 @@ -47,11 +47,6 @@ public class RegisterNewEmailSender extends OutgoingEmail { add(RecipientType.TO, new Address(addr)); } - @Override - protected boolean shouldSendMessage() { - return true; - } - @Override protected void format() throws EmailException { appendText(velocifyFile("RegisterNewEmail.vm")); From d84ff8f6ce67af7aae41e3a865f1e04eccba39c8 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 3 Feb 2016 13:24:30 +0900 Subject: [PATCH 2/8] CreateEmail: Don't allow to add email prohibited by sendemail.allowrcpt If the server configuration includes sendemail.allowrcpt, and the user attempts to add an email address whose domain is not included, return a "method not allowed" error. This check is not done when the calling user has the 'ModifyAccount' capability and has set the noConfirmation flag on the request. In this case the email is added without any validation other than that the email address itself is a valid email address. Change-Id: Ic83e1f73962dbc4ba76c6ab2c518797fa9d6adeb --- .../java/com/google/gerrit/server/account/CreateEmail.java | 6 +++++- .../google/gerrit/server/mail/RegisterNewEmailSender.java | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-) 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 441213d1e1..700e138a96 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 @@ -135,7 +135,11 @@ public class CreateEmail implements RestModifyView { } } 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/mail/RegisterNewEmailSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/RegisterNewEmailSender.java index c24997bc70..1d879724d8 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 @@ -77,4 +77,8 @@ public class RegisterNewEmailSender extends OutgoingEmail { } return emailToken; } + + public boolean isAllowed() { + return args.emailSender.canEmail(addr); + } } From a2b943bff271f93e3225a40796202994b06801dc Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 3 Feb 2016 14:07:14 +0900 Subject: [PATCH 3/8] SmtpEmailSender: Refactor open method and improve logging - Log the reply code and reply string when the connection is rejected by the SMTP server. - Reuse the reply string in subsequent error handling clauses. - Combine handling of IOException and EmailException into a single catch block to reduce duplicated code for disconnecting the client. Change-Id: Icf36e8bf10e2273ff6678734f9f0759a52505f24 --- .../gerrit/server/mail/SmtpEmailSender.java | 34 ++++++++----------- 1 file changed, 15 insertions(+), 19 deletions(-) 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 2f8f75d2f5..ee21316bda 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 @@ -248,16 +248,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) { @@ -265,32 +268,25 @@ 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(); } catch (IOException e2) { } } + if (e instanceof EmailException) { + throw (EmailException) e; + } throw new EmailException(e.getMessage(), e); - } catch (EmailException e) { - if (client.isConnected()) { - try { - client.disconnect(); - } catch (IOException e2) { - } - } - throw e; } - return client; } } From 7f64ab66b89c861bb744d513dfb50ee574bb7d9e Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 3 Feb 2016 15:25:50 +0900 Subject: [PATCH 4/8] ldap.Helper: Kill "assuming empty group membership" logspam Change I75fd86fb9 added catching of AccountException and replaced the stack trace log with a warning: "Account not found: assuming empty group membership" The intention was to prevent excessive logging, but these warnings themselves are filling up the log. Change-Id: Id52830dbcab1f7434960a47cb11e5e559a6f650a --- .../main/java/com/google/gerrit/server/auth/ldap/Helper.java | 4 ---- 1 file changed, 4 deletions(-) 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 4c29c9b225..b246868314 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 @@ -224,8 +224,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(); } } @@ -247,8 +245,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(); } } From e3c1af0768706f83f113a0832b0480ccd0072977 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 3 Feb 2016 16:21:32 +0900 Subject: [PATCH 5/8] OutgoingEmail: Check for valid email address when adding recipients After finding error logs filled with the message: Not emailing NULL (prohibited by allowrcpt) it turned out that a user's preferred email address had been set to the literal string "NULL". Presumably this was done while running a version of Gerrit older than 2.9, which included change I1f8d95dd9 ("Validate email address when adding email or creating account"). In combination with the allowrcpt setting being enabled, this was preventing the user from receiving any mails from Gerrit, and causing the log to be filled with this message. When adding recipients to an outgoing mail, check for the email address being invalid, and log a different message in this case. Change-Id: Id79e13f5afb1a640b5afed21a9fb50b7fc8aa5f0 --- .../gerrit/server/mail/OutgoingEmail.java | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) 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 8a3133ff0f..970a36c687 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 @@ -25,6 +25,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; @@ -347,21 +348,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 { + 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; + } } } } From 32e5af312857a541849ce9f690f5b68fbba81a70 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 3 Feb 2016 17:12:03 +0900 Subject: [PATCH 6/8] OutgoingEmail: Kill "Skipping delivery of email" logspam The logs: INFO: "Skipping delivery of email with no recipients" and WARN: "Skipping delivery of email with no body" don't add any value, and just fill up the logs. Remove them. Change-Id: I66d19f8bd970bb9403849c4fa7bfb7f7f597b46d --- .../main/java/com/google/gerrit/server/mail/OutgoingEmail.java | 2 -- 1 file changed, 2 deletions(-) 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 970a36c687..9ca83a8912 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 @@ -290,7 +290,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; } @@ -298,7 +297,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.info("Skipping delivery of email with no recipients"); return false; } From 955c87021a2afe343db1d4553645f848744cf821 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 3 Feb 2016 18:42:41 +0900 Subject: [PATCH 7/8] ReceiveCommits: Improve error message when failing to insert patch set The handling of IOException and InsertException is the same, so combine the two blocks into a single catch. Make the error message a bit more readable. Change-Id: Ia1cdf4a3ed518f9bfbbe46bd72aa51593be3f2db --- .../com/google/gerrit/server/git/ReceiveCommits.java | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) 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 9bad12d42e..f33591e5f9 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 @@ -734,15 +734,10 @@ public class ReceiveCommits { if (replace.insertPatchSet().checkedGet() != null) { replace.inputCommand.setResult(OK); } - } catch (IOException err) { + } catch (IOException | InsertException err) { reject(replace.inputCommand, "internal server error"); log.error(String.format( - "Cannot add patch set to %d of %s", - e.getKey().get(), project.getName()), err); - } catch (InsertException 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) { From 6ab21459897420b55fb23b95d637883b6e14ee80 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 4 Feb 2016 17:02:29 +0900 Subject: [PATCH 8/8] OutgoingEmail: Fix breakage introduced in add() method In change Id79e13f5a the add method was refactored to check for valid emails. During this refactoring one statement was missed. It was not caught on this branch because there is no acceptance test, and manual tests were only done for the negative case (ivalid address, address prohibited). The problem was caught when merging up to stable-2.12, where there is an accepance test for mail messages. Change-Id: Ie437f5d733c5a9d1015f0322c6e04055a73eb9b5 --- .../main/java/com/google/gerrit/server/mail/OutgoingEmail.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 9ca83a8912..af8a3812a3 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 @@ -350,7 +350,7 @@ public abstract class OutgoingEmail { 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 { + } else if (smtpRcptTo.add(addr)) { switch (rt) { case TO: ((EmailHeader.AddressList) headers.get(HDR_TO)).add(addr);