From 9070951769c2a57f7d56746bdec48938df80bf94 Mon Sep 17 00:00:00 2001 From: Scott Bommarito Date: Fri, 26 Oct 2018 15:20:00 -0700 Subject: [PATCH] Hotfix: Do not CC sender for Contact Owners emails (#6602) --- .../mail/CoreMarkdownMessageService.cs | 14 +++++------ .../Controllers/PackagesController.cs | 3 +-- .../Mail/BackgroundMarkdownMessageService.cs | 6 ++--- .../Mail/Messages/ContactOwnersMessage.cs | 6 +---- .../Messages/ContactOwnersMessageFacts.cs | 22 ++++------------ .../Services/MessageServiceFacts.cs | 25 ++++++++----------- 6 files changed, 28 insertions(+), 48 deletions(-) diff --git a/src/NuGetGallery.Core/Infrastructure/mail/CoreMarkdownMessageService.cs b/src/NuGetGallery.Core/Infrastructure/mail/CoreMarkdownMessageService.cs index a9cfd0d243..a1d61c4ca5 100644 --- a/src/NuGetGallery.Core/Infrastructure/mail/CoreMarkdownMessageService.cs +++ b/src/NuGetGallery.Core/Infrastructure/mail/CoreMarkdownMessageService.cs @@ -39,11 +39,16 @@ public async Task SendMessageAsync(IEmailBuilder emailBuilder, bool copySender = using (var email = CreateMailMessage(emailBuilder)) { - await SendMessageInternalAsync(email, copySender, discloseSenderAddress); + await SendMessageInternalAsync(email); + + if (copySender && !discloseSenderAddress) + { + await SendMessageToSenderAsync(email); + } } } - protected virtual async Task SendMessageInternalAsync(MailMessage mailMessage, bool copySender = false, bool discloseSenderAddress = false) + protected virtual async Task SendMessageInternalAsync(MailMessage mailMessage) { if (!mailMessage.To.Any()) { @@ -72,11 +77,6 @@ protected virtual async Task SendMessageInternalAsync(MailMessage mailMessage, b } } } - - if (copySender && !discloseSenderAddress) - { - await SendMessageToSenderAsync(mailMessage); - } } protected virtual Task AttemptSendMessageAsync(MailMessage mailMessage, int attemptNumber) diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs index 40ec10d4a0..84c05ff17c 100644 --- a/src/NuGetGallery/Controllers/PackagesController.cs +++ b/src/NuGetGallery/Controllers/PackagesController.cs @@ -1115,8 +1115,7 @@ public virtual async Task ContactOwners(string id, string version, package, Url.Package(package, false), HttpUtility.HtmlEncode(contactForm.Message), - Url.AccountSettings(relativeUrl: false), - contactForm.CopySender); + Url.AccountSettings(relativeUrl: false)); await _messageService.SendMessageAsync(contactOwnersMessage, contactForm.CopySender, discloseSenderAddress: false); diff --git a/src/NuGetGallery/Infrastructure/Mail/BackgroundMarkdownMessageService.cs b/src/NuGetGallery/Infrastructure/Mail/BackgroundMarkdownMessageService.cs index ce47b656c7..49c3daca15 100644 --- a/src/NuGetGallery/Infrastructure/Mail/BackgroundMarkdownMessageService.cs +++ b/src/NuGetGallery/Infrastructure/Mail/BackgroundMarkdownMessageService.cs @@ -30,7 +30,7 @@ public BackgroundMarkdownMessageService( private Func _messageServiceFactory; private bool _sentMessage; - protected override Task SendMessageInternalAsync(MailMessage mailMessage, bool copySender = false, bool discloseSenderAddress = false) + protected override Task SendMessageInternalAsync(MailMessage mailMessage) { // Some MVC controller actions send more than one message. Since this method sends // the message async, we need a new IMessageService per email, to avoid calling @@ -39,7 +39,7 @@ protected override Task SendMessageInternalAsync(MailMessage mailMessage, bool c if (_sentMessage) { var newMessageService = _messageServiceFactory.Invoke(); - return newMessageService.SendMessageInternalAsync(mailMessage, copySender, discloseSenderAddress); + return newMessageService.SendMessageInternalAsync(mailMessage); } else { @@ -55,7 +55,7 @@ protected override Task SendMessageInternalAsync(MailMessage mailMessage, bool c { try { - await base.SendMessageInternalAsync(messageCopy, copySender, discloseSenderAddress); + await base.SendMessageInternalAsync(messageCopy); } catch (Exception ex) { diff --git a/src/NuGetGallery/Infrastructure/Mail/Messages/ContactOwnersMessage.cs b/src/NuGetGallery/Infrastructure/Mail/Messages/ContactOwnersMessage.cs index 7d07d6ce87..6fbb119843 100644 --- a/src/NuGetGallery/Infrastructure/Mail/Messages/ContactOwnersMessage.cs +++ b/src/NuGetGallery/Infrastructure/Mail/Messages/ContactOwnersMessage.cs @@ -17,8 +17,7 @@ public ContactOwnersMessage( Package package, string packageUrl, string htmlEncodedMessage, - string emailSettingsUrl, - bool copySender) + string emailSettingsUrl) { _configuration = configuration ?? throw new ArgumentNullException(nameof(configuration)); FromAddress = fromAddress ?? throw new ArgumentNullException(nameof(fromAddress)); @@ -26,7 +25,6 @@ public ContactOwnersMessage( PackageUrl = packageUrl ?? throw new ArgumentNullException(nameof(packageUrl)); HtmlEncodedMessage = htmlEncodedMessage ?? throw new ArgumentNullException(nameof(htmlEncodedMessage)); EmailSettingsUrl = emailSettingsUrl ?? throw new ArgumentNullException(nameof(emailSettingsUrl)); - CopySender = copySender; } public MailAddress FromAddress { get; } @@ -34,7 +32,6 @@ public ContactOwnersMessage( public string PackageUrl { get; } public string HtmlEncodedMessage { get; } public string EmailSettingsUrl { get; } - public bool CopySender { get; } public override MailAddress Sender => _configuration.GalleryOwner; @@ -46,7 +43,6 @@ public override IEmailRecipients GetRecipients() return new EmailRecipients( to, - cc: CopySender ? new[] { FromAddress } : null, replyTo: new[] { FromAddress }); } diff --git a/tests/NuGetGallery.Facts/Infrastructure/Mail/Messages/ContactOwnersMessageFacts.cs b/tests/NuGetGallery.Facts/Infrastructure/Mail/Messages/ContactOwnersMessageFacts.cs index aa8436893f..620103d636 100644 --- a/tests/NuGetGallery.Facts/Infrastructure/Mail/Messages/ContactOwnersMessageFacts.cs +++ b/tests/NuGetGallery.Facts/Infrastructure/Mail/Messages/ContactOwnersMessageFacts.cs @@ -43,8 +43,7 @@ public void GivenANullArgument_ItShouldThrow( package, packageUrl, message, - emailSettingsUrl, - copySender)); + emailSettingsUrl)); } } @@ -72,19 +71,9 @@ public void AddsFromAddressToReplyToList() } [Fact] - public void AddsFromAddressToCCListWhenCopyingSender() + public void DoesNotAddFromAddressToCCList() { - var message = CreateMessage(copySender: true); - var recipients = message.GetRecipients(); - - Assert.Equal(1, recipients.CC.Count); - Assert.Contains(Fakes.FromAddress, recipients.CC); - } - - [Fact] - public void DoesNotAddFromAddressToCCListWhenNotCopyingSender() - { - var message = CreateMessage(copySender: false); + var message = CreateMessage(); var recipients = message.GetRecipients(); Assert.Empty(recipients.CC); @@ -122,7 +111,7 @@ public void SetsGalleryOwnerAsSender() Assert.Equal(Configuration.GalleryOwner, message.Sender); } - private static ContactOwnersMessage CreateMessage(bool copySender = false) + private static ContactOwnersMessage CreateMessage() { return new ContactOwnersMessage( Configuration, @@ -130,8 +119,7 @@ private static ContactOwnersMessage CreateMessage(bool copySender = false) Fakes.Package, Fakes.PackageUrl, "user input", - Fakes.EmailSettingsUrl, - copySender); + Fakes.EmailSettingsUrl); } private const string _expectedMarkdownBody = diff --git a/tests/NuGetGallery.Facts/Services/MessageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/MessageServiceFacts.cs index 12a28307f4..f8de67e688 100644 --- a/tests/NuGetGallery.Facts/Services/MessageServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/MessageServiceFacts.cs @@ -233,7 +233,7 @@ public class TheSendContactOwnersMessageMethod : TestContainer { [Fact] - public async Task WillCopySenderIfAsked() + public async Task WillSendCopyToSenderIfAsked() { // arrange var packageId = "smangit"; @@ -267,8 +267,7 @@ public async Task WillCopySenderIfAsked() package, "http://someurl/", "Test message", - "http://someotherurl/", - copySender: true); + "http://someotherurl/"); // act await messageService.SendMessageAsync(contactOwnersMessage, copySender: true, discloseSenderAddress: false); @@ -285,6 +284,8 @@ public async Task WillCopySenderIfAsked() Assert.Equal(TestGalleryOwner, messages[1].From); Assert.Equal(fromAddress, messages[0].ReplyToList.Single().Address); Assert.Equal(fromAddress, messages[1].ReplyToList.Single().Address); + Assert.Empty(messages[0].CC); + Assert.Empty(messages[1].CC); } [Fact] @@ -320,11 +321,10 @@ public async Task WillSendEmailToAllOwners() package, "http://packageUrl/", "Test message", - "http://emailSettingsUrl/", - copySender: false); + "http://emailSettingsUrl/"); await messageService.SendMessageAsync(contactOwnersMessage); - var message = messageService.MockMailSender.Sent.Last(); + var message = messageService.MockMailSender.Sent.Single(); Assert.Equal(owner1Email, message.To[0].Address); Assert.Equal(owner2Email, message.To[1].Address); @@ -372,11 +372,10 @@ public async Task WillNotSendEmailToOwnerThatOptsOut() package, "http://someurl/", "Test message", - "http://someotherurl/", - copySender: false); + "http://someotherurl/"); await messageService.SendMessageAsync(contactOwnersMessage); - var message = messageService.MockMailSender.Sent.Last(); + var message = messageService.MockMailSender.Sent.Single(); // assert Assert.Equal(ownerAddress, message.To[0].Address); @@ -418,8 +417,7 @@ public async Task WillNotAttemptToSendIfNoOwnersAllow() package, "http://someurl/", "Test message", - "http://someotherurl/", - copySender: false); + "http://someotherurl/"); await messageService.SendMessageAsync(contactOwnersMessage); @@ -428,7 +426,7 @@ public async Task WillNotAttemptToSendIfNoOwnersAllow() } [Fact] - public async Task WillNotCopySenderIfNoOwnersAllow() + public async Task WillNotSendCopyToSenderIfNoOwnersAllow() { // arrange var packageId = "smangit"; @@ -461,8 +459,7 @@ public async Task WillNotCopySenderIfNoOwnersAllow() package, "http://someurl/", "Test message", - "http://someotherurl/", - copySender: false); + "http://someotherurl/"); await messageService.SendMessageAsync(contactOwnersMessage); // assert