From c7e6021e8b5718fdf268faaf51d9175acd22087c Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Sat, 28 Jan 2017 22:03:01 +0900 Subject: [PATCH] Make OutgoingEmailValidator a Singleton Make the class a Singleton so that it can be used by classes that get it injected, rather than calling its methods statically. This is a preparatory step to allowing the validator to get the gerrit configuration injected, which will allow to disable validation using this validator. Change-Id: If5c7cc6d2c034b9ed16b1358b4d1d186d57daf98 --- .../server/mail/EmailValidatorIT.java | 9 ++++++--- .../gerrit/server/account/CreateAccount.java | 5 ++++- .../gerrit/server/account/CreateEmail.java | 5 ++++- .../ExternalIdsConsistencyChecker.java | 9 +++++++-- .../gerrit/server/change/PostReviewers.java | 7 +++++-- .../server/mail/send/EmailArguments.java | 5 ++++- .../gerrit/server/mail/send/OutgoingEmail.java | 2 +- .../mail/send/OutgoingEmailValidator.java | 18 +++++++++++++++--- 8 files changed, 46 insertions(+), 14 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/EmailValidatorIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/EmailValidatorIT.java index 1a029b8515..5e6cc9bc26 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/EmailValidatorIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/EmailValidatorIT.java @@ -19,6 +19,7 @@ import static com.google.common.truth.Truth.assert_; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.server.mail.send.OutgoingEmailValidator; +import com.google.inject.Inject; import java.io.BufferedReader; import java.io.InputStream; import java.io.InputStreamReader; @@ -27,9 +28,11 @@ import org.junit.Test; public class EmailValidatorIT extends AbstractDaemonTest { private static final String UNSUPPORTED_PREFIX = "#! "; + @Inject private OutgoingEmailValidator validator; + @Test public void validateLocalDomain() throws Exception { - assertThat(OutgoingEmailValidator.isValid("foo@bar.local")).isTrue(); + assertThat(validator.isValid("foo@bar.local")).isTrue(); } @Test @@ -49,13 +52,13 @@ public class EmailValidatorIT extends AbstractDaemonTest { String test = "test@example." + tld.toLowerCase().substring(UNSUPPORTED_PREFIX.length()); assert_() .withFailureMessage("expected invalid TLD \"" + test + "\"") - .that(OutgoingEmailValidator.isValid(test)) + .that(validator.isValid(test)) .isFalse(); } else { String test = "test@example." + tld.toLowerCase(); assert_() .withFailureMessage("failed to validate TLD \"" + test + "\"") - .that(OutgoingEmailValidator.isValid(test)) + .that(validator.isValid(test)) .isTrue(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java index 2f75390afe..2cfd71601c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java @@ -77,6 +77,7 @@ public class CreateAccount implements RestModifyView private final AccountManager accountManager; private final RegisterNewEmailSender.Factory registerNewEmailFactory; private final PutPreferred putPreferred; + private final OutgoingEmailValidator validator; private final String email; private final boolean isDevMode; @@ -69,6 +70,7 @@ public class CreateEmail implements RestModifyView AccountManager accountManager, RegisterNewEmailSender.Factory registerNewEmailFactory, PutPreferred putPreferred, + OutgoingEmailValidator validator, @Assisted String email) { this.self = self; this.realm = realm; @@ -76,6 +78,7 @@ public class CreateEmail implements RestModifyView this.accountManager = accountManager; this.registerNewEmailFactory = registerNewEmailFactory; this.putPreferred = putPreferred; + this.validator = validator; this.email = email; this.isDevMode = authConfig.getAuthType() == DEVELOPMENT_BECOME_ANY_ACCOUNT; } @@ -93,7 +96,7 @@ public class CreateEmail implements RestModifyView input = new EmailInput(); } - if (!OutgoingEmailValidator.isValid(email)) { + if (!validator.isValid(email)) { throw new BadRequestException("invalid email address"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdsConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdsConsistencyChecker.java index bc681a268b..928349d843 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdsConsistencyChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdsConsistencyChecker.java @@ -44,13 +44,18 @@ public class ExternalIdsConsistencyChecker { private final GitRepositoryManager repoManager; private final AllUsersName allUsers; private final AccountCache accountCache; + private final OutgoingEmailValidator validator; @Inject ExternalIdsConsistencyChecker( - GitRepositoryManager repoManager, AllUsersName allUsers, AccountCache accountCache) { + GitRepositoryManager repoManager, + AllUsersName allUsers, + AccountCache accountCache, + OutgoingEmailValidator validator) { this.repoManager = repoManager; this.allUsers = allUsers; this.accountCache = accountCache; + this.validator = validator; } public List check() throws IOException { @@ -123,7 +128,7 @@ public class ExternalIdsConsistencyChecker { problems); } - if (extId.email() != null && !OutgoingEmailValidator.isValid(extId.email())) { + if (extId.email() != null && !validator.isValid(extId.email())) { addError( String.format( "External ID '%s' has an invalid email: %s", extId.key().get(), extId.email()), diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java index 0f4859597a..221705f242 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java @@ -101,6 +101,7 @@ public class PostReviewers implements RestModifyView anonymousProvider; private final PostReviewersOp.Factory postReviewersOpFactory; + private final OutgoingEmailValidator validator; @Inject PostReviewers( @@ -120,7 +121,8 @@ public class PostReviewers implements RestModifyView anonymousProvider, - PostReviewersOp.Factory postReviewersOpFactory) { + PostReviewersOp.Factory postReviewersOpFactory, + OutgoingEmailValidator validator) { this.accounts = accounts; this.reviewerFactory = reviewerFactory; this.permissionBackend = permissionBackend; @@ -138,6 +140,7 @@ public class PostReviewers implements RestModifyView outgoingEmailValidationListeners; final StarredChangesUtil starredChangesUtil; final Provider accountQueryProvider; + final OutgoingEmailValidator validator; @Inject EmailArguments( @@ -111,7 +112,8 @@ public class EmailArguments { SitePaths site, DynamicSet outgoingEmailValidationListeners, StarredChangesUtil starredChangesUtil, - Provider accountQueryProvider) { + Provider accountQueryProvider, + OutgoingEmailValidator validator) { this.server = server; this.projectCache = projectCache; this.groupBackend = groupBackend; @@ -141,5 +143,6 @@ public class EmailArguments { this.outgoingEmailValidationListeners = outgoingEmailValidationListeners; this.starredChangesUtil = starredChangesUtil; this.accountQueryProvider = accountQueryProvider; + this.validator = validator; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java index 6877879654..c2ae0bc928 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java @@ -478,7 +478,7 @@ 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.getEmail() != null && addr.getEmail().length() > 0) { - if (!OutgoingEmailValidator.isValid(addr.getEmail())) { + if (!args.validator.isValid(addr.getEmail())) { log.warn("Not emailing " + addr.getEmail() + " (invalid email address)"); } else if (!args.emailSender.canEmail(addr.getEmail())) { log.warn("Not emailing " + addr.getEmail() + " (prohibited by allowrcpt)"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmailValidator.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmailValidator.java index 2d9db1dd8c..2c5ecfba48 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmailValidator.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmailValidator.java @@ -16,15 +16,27 @@ package com.google.gerrit.server.mail.send; import static org.apache.commons.validator.routines.DomainValidator.ArrayType.GENERIC_PLUS; +import com.google.inject.Singleton; import org.apache.commons.validator.routines.DomainValidator; import org.apache.commons.validator.routines.EmailValidator; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +@Singleton public class OutgoingEmailValidator { - static { - DomainValidator.updateTLDOverride(GENERIC_PLUS, new String[] {"local"}); + private static final Logger log = LoggerFactory.getLogger(OutgoingEmailValidator.class); + + OutgoingEmailValidator() { + try { + DomainValidator.updateTLDOverride(GENERIC_PLUS, new String[] {"local"}); + } catch (IllegalStateException e) { + // Should only happen in tests, where the OutgoingEmailValidator + // is instantiated repeatedly. + log.warn("Failed to update TLD override: " + e.getMessage()); + } } - public static boolean isValid(String addr) { + public boolean isValid(String addr) { return EmailValidator.getInstance(true, true).isValid(addr); } }