diff --git a/java/google/registry/ui/server/SendEmailUtils.java b/java/google/registry/ui/server/SendEmailUtils.java index 278ab0f4f..756e4ed7e 100644 --- a/java/google/registry/ui/server/SendEmailUtils.java +++ b/java/google/registry/ui/server/SendEmailUtils.java @@ -24,6 +24,7 @@ import google.registry.config.RegistryConfig.Config; import google.registry.util.SendEmailService; import java.util.List; import java.util.Objects; +import java.util.stream.Stream; import javax.inject.Inject; import javax.mail.Message; import javax.mail.internet.AddressException; @@ -55,22 +56,24 @@ public class SendEmailUtils { } /** - * Sends an email from Nomulus to the registrarChangesNotificationEmailAddresses. Returns true iff - * sending to at least 1 address was successful. + * Sends an email from Nomulus to the registrarChangesNotificationEmailAddresses and the specified + * additionalAddresses. Returns true iff sending to at least 1 address was successful. * - *

This means that if there are no recepients ({@link #hasRecepients} returns false), this will + *

This means that if there are no recipients ({@link #hasRecipients} returns false), this will * return false even thought no error happened. * - *

This also means that if there are multiple recepients, it will return true even if some (but - * not all) of the recepients had an error. + *

This also means that if there are multiple recipients, it will return true even if some (but + * not all) of the recipients had an error. */ - public boolean sendEmail(final String subject, String body) { + public boolean sendEmail( + final String subject, String body, ImmutableList additionalAddresses) { try { Message msg = emailService.createMessage(); msg.setFrom( new InternetAddress(gSuiteOutgoingEmailAddress, gSuiteOutgoingEmailDisplayName)); List emails = - registrarChangesNotificationEmailAddresses.stream() + Stream.concat( + registrarChangesNotificationEmailAddresses.stream(), additionalAddresses.stream()) .map( emailAddress -> { try { @@ -101,11 +104,19 @@ public class SendEmailUtils { return true; } - /** - * Returns whether there are any recepients set up. {@link #sendEmail} will always return false if - * there are no recepients. + /** Sends an email from Nomulus to the registrarChangesNotificationEmailAddresses. + * + *

See {@link #sendEmail(String, String, ImmutableList)}. */ - public boolean hasRecepients() { + public boolean sendEmail(final String subject, String body) { + return sendEmail(subject, body, ImmutableList.of()); + } + + /** + * Returns whether there are any recipients set up. {@link #sendEmail} will always return false if + * there are no recipients. + */ + public boolean hasRecipients() { return !registrarChangesNotificationEmailAddresses.isEmpty(); } } diff --git a/java/google/registry/ui/server/registrar/ConsoleOteSetupAction.java b/java/google/registry/ui/server/registrar/ConsoleOteSetupAction.java index ac3f1b3c6..7a1a36be3 100644 --- a/java/google/registry/ui/server/registrar/ConsoleOteSetupAction.java +++ b/java/google/registry/ui/server/registrar/ConsoleOteSetupAction.java @@ -221,7 +221,7 @@ public final class ConsoleOteSetupAction implements Runnable { private void sendExternalUpdates(ImmutableMap clientIdToTld) { - if (!sendEmailUtils.hasRecepients()) { + if (!sendEmailUtils.hasRecipients()) { return; } String environment = Ascii.toLowerCase(String.valueOf(registryEnvironment)); diff --git a/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorAction.java b/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorAction.java index 7444d196e..bb2cb98d7 100644 --- a/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorAction.java +++ b/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorAction.java @@ -347,7 +347,7 @@ public final class ConsoleRegistrarCreatorAction implements Runnable { return String.format(" %s: %s\n", name, value.orElse(null)); } private void sendExternalUpdates() { - if (!sendEmailUtils.hasRecepients()) { + if (!sendEmailUtils.hasRecipients()) { return; } String environment = Ascii.toLowerCase(String.valueOf(registryEnvironment)); diff --git a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index e64063313..24937264a 100644 --- a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java @@ -15,6 +15,7 @@ package google.registry.ui.server.registrar; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.Sets.difference; import static google.registry.export.sheet.SyncRegistrarsSheetAction.enqueueRegistrarSheetSync; @@ -463,15 +464,15 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA /** * Determines if any changes were made to the registrar besides the lastUpdateTime, and if so, - * sends an email with a diff of the changes to the configured notification email address and - * enqueues a task to re-sync the registrar sheet. + * sends an email with a diff of the changes to the configured notification email address and all + * contact addresses and enqueues a task to re-sync the registrar sheet. */ private void sendExternalUpdatesIfNecessary( Registrar existingRegistrar, ImmutableSet existingContacts, Registrar updatedRegistrar, ImmutableSet updatedContacts) { - if (!sendEmailUtils.hasRecepients()) { + if (!sendEmailUtils.hasRecipients() && existingContacts.isEmpty()) { return; } @@ -498,7 +499,13 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA environment, existingRegistrar.getClientId(), authResult.userIdForLogging(), - DiffUtils.prettyPrintDiffedMap(diffs, null))); + DiffUtils.prettyPrintDiffedMap(diffs, null)), + existingContacts.stream() + .map( + contact -> { + return contact.getEmailAddress(); + }) + .collect(toImmutableList())); } /** Thrown when a set of contacts doesn't meet certain constraints. */ diff --git a/javatests/google/registry/ui/server/SendEmailUtilsTest.java b/javatests/google/registry/ui/server/SendEmailUtilsTest.java index 6ad50e5a1..c09896a29 100644 --- a/javatests/google/registry/ui/server/SendEmailUtilsTest.java +++ b/javatests/google/registry/ui/server/SendEmailUtilsTest.java @@ -51,19 +51,19 @@ public class SendEmailUtilsTest { when(emailService.createMessage()).thenReturn(message); } - private void setRecepients(ImmutableList recepients) { + private void setRecipients(ImmutableList recipients) { sendEmailUtils = new SendEmailUtils( "outgoing@registry.example", "outgoing display name", - recepients, + recipients, emailService); } @Test public void testSuccess_sendToOneAddress() throws Exception { - setRecepients(ImmutableList.of("johnny@fakesite.tld")); - assertThat(sendEmailUtils.hasRecepients()).isTrue(); + setRecipients(ImmutableList.of("johnny@fakesite.tld")); + assertThat(sendEmailUtils.hasRecipients()).isTrue(); assertThat( sendEmailUtils.sendEmail( "Welcome to the Internet", @@ -78,8 +78,8 @@ public class SendEmailUtilsTest { @Test public void testSuccess_sendToMultipleAddresses() throws Exception { - setRecepients(ImmutableList.of("foo@example.com", "bar@example.com")); - assertThat(sendEmailUtils.hasRecepients()).isTrue(); + setRecipients(ImmutableList.of("foo@example.com", "bar@example.com")); + assertThat(sendEmailUtils.hasRecipients()).isTrue(); assertThat( sendEmailUtils.sendEmail( "Welcome to the Internet", @@ -93,8 +93,8 @@ public class SendEmailUtilsTest { @Test public void testSuccess_ignoresMalformedEmailAddress() throws Exception { - setRecepients(ImmutableList.of("foo@example.com", "1iñvalidemail")); - assertThat(sendEmailUtils.hasRecepients()).isTrue(); + setRecipients(ImmutableList.of("foo@example.com", "1iñvalidemail")); + assertThat(sendEmailUtils.hasRecipients()).isTrue(); assertThat( sendEmailUtils.sendEmail( "Welcome to the Internet", @@ -107,8 +107,8 @@ public class SendEmailUtilsTest { @Test public void testFailure_noAddresses() throws Exception { - setRecepients(ImmutableList.of()); - assertThat(sendEmailUtils.hasRecepients()).isFalse(); + setRecipients(ImmutableList.of()); + assertThat(sendEmailUtils.hasRecipients()).isFalse(); assertThat( sendEmailUtils.sendEmail( "Welcome to the Internet", @@ -119,8 +119,8 @@ public class SendEmailUtilsTest { @Test public void testFailure_onlyGivenMalformedAddress() throws Exception { - setRecepients(ImmutableList.of("1iñvalidemail")); - assertThat(sendEmailUtils.hasRecepients()).isTrue(); + setRecipients(ImmutableList.of("1iñvalidemail")); + assertThat(sendEmailUtils.hasRecipients()).isTrue(); assertThat( sendEmailUtils.sendEmail( "Welcome to the Internet", @@ -131,8 +131,8 @@ public class SendEmailUtilsTest { @Test public void testFailure_exceptionThrownDuringSend() throws Exception { - setRecepients(ImmutableList.of("foo@example.com")); - assertThat(sendEmailUtils.hasRecepients()).isTrue(); + setRecipients(ImmutableList.of("foo@example.com")); + assertThat(sendEmailUtils.hasRecipients()).isTrue(); doThrow(new MessagingException()).when(emailService).sendMessage(any(Message.class)); assertThat( sendEmailUtils.sendEmail( @@ -145,6 +145,42 @@ public class SendEmailUtilsTest { .containsExactly(new InternetAddress("foo@example.com")); } + @Test + public void testAdditionalRecipients() throws Exception { + setRecipients(ImmutableList.of("foo@example.com")); + assertThat(sendEmailUtils.hasRecipients()).isTrue(); + assertThat( + sendEmailUtils.sendEmail( + "Welcome to the Internet", + "It is a dark and scary place.", + ImmutableList.of("bar@example.com", "baz@example.com"))) + .isTrue(); + verifyMessageSent(); + assertThat(message.getAllRecipients()) + .asList() + .containsExactly( + new InternetAddress("foo@example.com"), + new InternetAddress("bar@example.com"), + new InternetAddress("baz@example.com")); + } + + @Test + public void testOnlyAdditionalRecipients() throws Exception { + setRecipients(ImmutableList.of()); + assertThat(sendEmailUtils.hasRecipients()).isFalse(); + assertThat( + sendEmailUtils.sendEmail( + "Welcome to the Internet", + "It is a dark and scary place.", + ImmutableList.of("bar@example.com", "baz@example.com"))) + .isTrue(); + verifyMessageSent(); + assertThat(message.getAllRecipients()) + .asList() + .containsExactly( + new InternetAddress("bar@example.com"), new InternetAddress("baz@example.com")); + } + private void verifyMessageSent() throws Exception { verify(emailService).sendMessage(message); assertThat(message.getSubject()).isEqualTo("Welcome to the Internet"); diff --git a/javatests/google/registry/ui/server/registrar/ContactSettingsTest.java b/javatests/google/registry/ui/server/registrar/ContactSettingsTest.java index 159e8980b..3cb98b90b 100644 --- a/javatests/google/registry/ui/server/registrar/ContactSettingsTest.java +++ b/javatests/google/registry/ui/server/registrar/ContactSettingsTest.java @@ -65,7 +65,7 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase { } @Test - public void testPost_updateContacts_success() { + public void testPost_updateContacts_success() throws Exception { // Remove all the contacts but the first by updating with list of // just it. Map adminContact1 = new HashMap<>(); @@ -91,6 +91,7 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase { .build(); assertThat(loadRegistrar(CLIENT_ID).getContacts()).containsExactly(newContact); assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + verifyContactsNotified(); } @Test diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java index f573a7eac..90a9cb8ba 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java @@ -40,7 +40,6 @@ import google.registry.util.CidrAddressBlock; import java.util.Map; import java.util.function.BiFunction; import java.util.function.Function; -import javax.mail.internet.InternetAddress; import org.json.simple.JSONValue; import org.json.simple.parser.ParseException; import org.junit.Test; @@ -56,11 +55,7 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase String expectedEmailBody = loadFile(getClass(), "update_registrar_email.txt"); action.handleJsonRequest(readJsonFromFile("update_registrar.json", getLastUpdateTime())); verify(rsp, never()).setStatus(anyInt()); - verify(emailService).createMessage(); - verify(emailService).sendMessage(message); - assertThat(message.getAllRecipients()).asList().containsExactly( - new InternetAddress("notification@test.example"), - new InternetAddress("notification2@test.example")); + verifyContactsNotified(); assertThat(message.getContent()).isEqualTo(expectedEmailBody); assertTasksEnqueued("sheet", new TaskMatcher() .url(SyncRegistrarsSheetAction.PATH) diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java index 481ec8b61..d8bc8aace 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java @@ -22,12 +22,14 @@ import static google.registry.request.auth.AuthenticatedRegistrarAccessor.Role.O import static google.registry.security.JsonHttpTestUtils.createJsonPayload; import static google.registry.testing.DatastoreHelper.createTlds; import static google.registry.testing.DatastoreHelper.disallowRegistrarAccess; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.google.appengine.api.users.User; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSetMultimap; +import com.google.common.truth.Truth; import google.registry.config.RegistryEnvironment; import google.registry.model.ofy.Ofy; import google.registry.request.JsonActionRunner; @@ -48,6 +50,7 @@ import java.io.StringWriter; import java.util.Properties; import javax.mail.Message; import javax.mail.Session; +import javax.mail.internet.InternetAddress; import javax.mail.internet.MimeMessage; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -156,4 +159,16 @@ public class RegistrarSettingsActionTestCase { AuthenticatedRegistrarAccessor.createForTesting( ImmutableSetMultimap.of(CLIENT_ID, ADMIN)); } + + /** Verifies that the original contact of TheRegistrar is among those notified of a change. */ + protected void verifyContactsNotified() throws Exception { + verify(emailService).createMessage(); + verify(emailService).sendMessage(message); + Truth.assertThat(message.getAllRecipients()) + .asList() + .containsExactly( + new InternetAddress("notification@test.example"), + new InternetAddress("notification2@test.example"), + new InternetAddress("johndoe@theregistrar.com")); + } } diff --git a/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java b/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java index c10756115..89e55bcd8 100644 --- a/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java +++ b/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java @@ -41,7 +41,7 @@ import org.junit.runners.JUnit4; public class SecuritySettingsTest extends RegistrarSettingsActionTestCase { @Test - public void testPost_updateCert_success() { + public void testPost_updateCert_success() throws Exception { Registrar modified = loadRegistrar(CLIENT_ID) .asBuilder() @@ -55,6 +55,7 @@ public class SecuritySettingsTest extends RegistrarSettingsActionTestCase { assertThat(response).containsEntry("results", ImmutableList.of(modified.toJsonMap())); assertThat(loadRegistrar(CLIENT_ID)).isEqualTo(modified); assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + verifyContactsNotified(); } @Test @@ -71,7 +72,7 @@ public class SecuritySettingsTest extends RegistrarSettingsActionTestCase { } @Test - public void testChangeCertificates() { + public void testChangeCertificates() throws Exception { Map jsonMap = loadRegistrar(CLIENT_ID).toJsonMap(); jsonMap.put("clientCertificate", SAMPLE_CERT); jsonMap.put("failoverClientCertificate", null); @@ -84,10 +85,11 @@ public class SecuritySettingsTest extends RegistrarSettingsActionTestCase { assertThat(registrar.getFailoverClientCertificate()).isNull(); assertThat(registrar.getFailoverClientCertificateHash()).isNull(); assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + verifyContactsNotified(); } @Test - public void testChangeFailoverCertificate() { + public void testChangeFailoverCertificate() throws Exception { Map jsonMap = loadRegistrar(CLIENT_ID).toJsonMap(); jsonMap.put("failoverClientCertificate", SAMPLE_CERT2); Map response = action.handleJsonRequest(ImmutableMap.of( @@ -97,10 +99,11 @@ public class SecuritySettingsTest extends RegistrarSettingsActionTestCase { assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT2); assertThat(registrar.getFailoverClientCertificateHash()).isEqualTo(SAMPLE_CERT2_HASH); assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + verifyContactsNotified(); } @Test - public void testEmptyOrNullCertificate_doesNotClearOutCurrentOne() { + public void testEmptyOrNullCertificate_doesNotClearOutCurrentOne() throws Exception { Registrar initialRegistrar = persistResource( loadRegistrar(CLIENT_ID) diff --git a/javatests/google/registry/ui/server/registrar/WhoisSettingsTest.java b/javatests/google/registry/ui/server/registrar/WhoisSettingsTest.java index d66b8c33d..9b7c6ec63 100644 --- a/javatests/google/registry/ui/server/registrar/WhoisSettingsTest.java +++ b/javatests/google/registry/ui/server/registrar/WhoisSettingsTest.java @@ -36,7 +36,7 @@ import org.junit.runners.JUnit4; public class WhoisSettingsTest extends RegistrarSettingsActionTestCase { @Test - public void testPost_update_success() { + public void testPost_update_success() throws Exception { Registrar modified = loadRegistrar(CLIENT_ID) .asBuilder() @@ -61,6 +61,7 @@ public class WhoisSettingsTest extends RegistrarSettingsActionTestCase { assertThat(response.get("results")).isEqualTo(ImmutableList.of(modified.toJsonMap())); assertThat(loadRegistrar(CLIENT_ID)).isEqualTo(modified); assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + verifyContactsNotified(); } @Test