opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.11' into stable-2.12

Browse Source 
* 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
This commit is contained in:
David Pursehouse
2016-02-04 17:10:59 +09:00
parent 7f31a6c050 6ab2145989
commit fa81c9d423
6 changed files with 42 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
gerrit-server/src/main/java/com/google/gerrit/server/account/CreateEmail.java
View File

@@ -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);

4
gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java
View File

@@ -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();
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -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) {

31
gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java
View File

@@ -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;
}
}
}
}

9
gerrit-server/src/main/java/com/google/gerrit/server/mail/RegisterNewEmailSender.java
View File

@@ -49,11 +49,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"));
@@ -70,4 +65,8 @@ public class RegisterNewEmailSender extends OutgoingEmail {
}
return emailToken;
}
public boolean isAllowed() {
return args.emailSender.canEmail(addr);
}
}

35
gerrit-server/src/main/java/com/google/gerrit/server/mail/SmtpEmailSender.java
View File

@@ -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;
}
}