Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't attempt to send emails if they have no recipients #6637

Merged
merged 13 commits into from
Nov 9, 2018
Merged

Don't attempt to send emails if they have no recipients #6637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
bf7e98f
Merge pull request #6625 from NuGet/dev
ryuyu Nov 6, 2018
51f4085
Hotfix: Don't attempt to send emails if they have no recipients (#6638)
Nov 8, 2018
0b177de
Hotfix: Hotfix: Don't attempt to send emails if they have no recipien…
Nov 8, 2018
0adedfd
Embedded license file ingestion for Gallery (#6580)
agr Nov 8, 2018
fc932f6
Add cache control for nupkgs/snupks in public containers (#6634)
shishirx34 Nov 8, 2018
e43bd55
Add update blob properties support to cloud storage (#6640)
shishirx34 Nov 8, 2018
2284a50
Fix for broken package delete (#6642)
agr Nov 8, 2018
4426053
Upload license expression redirect url logic (#6641)
zhhyu Nov 9, 2018
236df59
ServerComon 2.36 to NuGetGalleryCore (#6643)
cristinamanum Nov 9, 2018
1a4da8d
Don't attempt to send emails if they have no recipients (#6637)
Nov 9, 2018
7bb631c
Merge branch 'dev' into sb-fixemailrecipients
Nov 9, 2018
3db0bf3
dummy commit 1
Nov 9, 2018
052dfba
dummy commit 2
Nov 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 2 additions & 11 deletions src/NuGetGallery.Core/NuGetGallery.Core.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,8 @@
<EmbeddedResource Include="Infrastructure\MigrateUserToOrganization.sql" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="NuGet.Services.Entities">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed these dependencies because they are transitively pulled in by NuGet.Services.Messaging.Email. I thought having this single dependency to rev will make this easier to manage in the future.

I have no problem with reverting this back if you think this isn't a good idea.

<Version>2.32.0-agr-license-2170862</Version>
</PackageReference>
<PackageReference Include="NuGet.Services.Messaging">
<Version>2.31.0</Version>
<PackageReference Include="NuGet.Services.Messaging.Email">
<Version>2.36.0</Version>
</PackageReference>
<PackageReference Include="NuGet.StrongName.AnglicanGeek.MarkdownMailer">
<Version>1.2.0</Version>
Expand Down Expand Up @@ -241,12 +238,6 @@
<PackageReference Include="NuGet.Packaging">
<Version>5.0.0-preview1.5665</Version>
</PackageReference>
<PackageReference Include="NuGet.Services.Validation">
<Version>2.36.0</Version>
</PackageReference>
<PackageReference Include="NuGet.Services.Validation.Issues">
<Version>2.36.0</Version>
</PackageReference>
<PackageReference Include="WindowsAzure.Storage">
<Version>7.1.2</Version>
</PackageReference>
Expand Down
16 changes: 9 additions & 7 deletions src/NuGetGallery/App_Start/DefaultDependenciesModule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ public static class BindingKeys

protected override void Load(ContainerBuilder builder)
{
var loggerConfiguration = LoggingSetup.CreateDefaultLoggerConfiguration(withConsoleLogger: false);
var loggerFactory = LoggingSetup.CreateLoggerFactory(loggerConfiguration);
builder.RegisterInstance(loggerFactory)
.AsSelf()
.As<ILoggerFactory>();
builder.RegisterGeneric(typeof(Logger<>))
.As(typeof(ILogger<>))
.SingleInstance();

var telemetryClient = TelemetryClientWrapper.Instance;
builder.RegisterInstance(telemetryClient)
.AsSelf()
Expand Down Expand Up @@ -466,18 +475,11 @@ private static void RegisterAsynchronousEmailMessagingService(ContainerBuilder b
.Keyed<ITopicClient>(BindingKeys.EmailPublisherTopic)
.OnRelease(x => x.Close());

// Create an ILoggerFactory
var loggerConfiguration = LoggingSetup.CreateDefaultLoggerConfiguration(withConsoleLogger: false);
var loggerFactory = LoggingSetup.CreateLoggerFactory(loggerConfiguration);

builder
.RegisterType<EmailMessageEnqueuer>()
.WithParameter(new ResolvedParameter(
(pi, ctx) => pi.ParameterType == typeof(ITopicClient),
(pi, ctx) => ctx.ResolveKeyed<ITopicClient>(BindingKeys.EmailPublisherTopic)))
.WithParameter(new ResolvedParameter(
(pi, ctx) => pi.ParameterType == typeof(ILogger<EmailMessageEnqueuer>),
(pi, ctx) => loggerFactory.CreateLogger<EmailMessageEnqueuer>()))
.As<IEmailMessageEnqueuer>();

builder.RegisterType<AsynchronousEmailMessageService>()
Expand Down
28 changes: 2 additions & 26 deletions ...ure/Mail/EmailRecipientsWithPermission.cs → ...ure/Mail/GalleryEmailRecipientsUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,12 @@
using System.Linq;
using System.Net.Mail;
using NuGet.Services.Entities;
using NuGet.Services.Messaging.Email;

namespace NuGetGallery.Infrastructure.Mail
{
public class EmailRecipientsWithPermission
: IEmailRecipients
public static class GalleryEmailRecipientsUtility
{
public EmailRecipientsWithPermission(
User user,
ActionRequiringAccountPermissions action,
IReadOnlyList<MailAddress> cc = null,
IReadOnlyList<MailAddress> bcc = null,
IReadOnlyList<MailAddress> replyTo = null)
public static IReadOnlyList<MailAddress> GetAddressesWithPermission(User user, ActionRequiringAccountPermissions action)
Copy link
Contributor Author

@scottbommarito scottbommarito Nov 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is EmailRecipientsWithPermission.AddAddressesWithPermission, but moved into a static method.

EmailRecipientsWithPermission was an unnecessary extra layer of abstraction so I removed it as part of this PR.
This is the pattern existing in ServerCommon: see EmailRecipients.GetAllOwners and EmailRecipients.GetOwnersSubscribedToPackagePushedNotification.

{
if (user == null)
{
Expand All @@ -30,23 +23,6 @@ public EmailRecipientsWithPermission(
throw new ArgumentNullException(nameof(action));
}

To = AddAddressesWithPermission(user, action);

CC = cc ?? new List<MailAddress>();
Bcc = bcc ?? new List<MailAddress>();
ReplyTo = replyTo ?? new List<MailAddress>();
}

public IReadOnlyList<MailAddress> To { get; }

public IReadOnlyList<MailAddress> CC { get; }

public IReadOnlyList<MailAddress> Bcc { get; }

public IReadOnlyList<MailAddress> ReplyTo { get; }

private static IReadOnlyList<MailAddress> AddAddressesWithPermission(User user, ActionRequiringAccountPermissions action)
{
var recipients = new List<MailAddress>();

if (user is Organization organization)
Expand Down
8 changes: 3 additions & 5 deletions src/NuGetGallery/Infrastructure/Mail/Messages/ContactOwnersMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,10 @@ public ContactOwnersMessage(

public override IEmailRecipients GetRecipients()
{
var to = EmailRecipients.GetAllOwners(
Package.PackageRegistration,
requireEmailAllowed: true);

return new EmailRecipients(
to,
to: EmailRecipients.GetAllOwners(
Package.PackageRegistration,
requireEmailAllowed: true),
replyTo: new[] { FromAddress });
}

Expand Down
9 changes: 3 additions & 6 deletions src/NuGetGallery/Infrastructure/Mail/Messages/OrganizationMemberRemovedMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,10 @@ public OrganizationMemberRemovedMessage(

public override IEmailRecipients GetRecipients()
{
if (!Organization.EmailAllowed)
{
return EmailRecipients.None;
Copy link
Contributor Author

@scottbommarito scottbommarito Nov 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the usage of EmailRecipients.None because I think it's misleading. We're using EmailRecipients.None as a signal for "this email shouldn't be sent" when the real signal for when an email shouldn't be sent is whether or not it has any recipients to send to. Because "an email without recipients to send to" and "an email with no recipients at all" are not logically equivalent, this seems odd.

For example, in this function, we return EmailRecipients.None when the intended To address doesn't allow emails. Does whether or not the To address allows emails affect the replyTo field of the email? Technically no. So then why are we returning a completely empty list of recipients when there are people whom replies to the email should go to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, good call! 👍

}

return new EmailRecipients(
to: new[] { Organization.ToMailAddress() },
to: Organization.EmailAllowed
? new[] { Organization.ToMailAddress() }
: new MailAddress[0],
replyTo: new[] { RemovedUser.ToMailAddress() });
}

Expand Down
9 changes: 3 additions & 6 deletions src/NuGetGallery/Infrastructure/Mail/Messages/OrganizationMemberUpdatedMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,10 @@ public OrganizationMemberUpdatedMessage(

public override IEmailRecipients GetRecipients()
{
if (!Organization.EmailAllowed)
{
return EmailRecipients.None;
}

return new EmailRecipients(
to: new[] { Organization.ToMailAddress() },
to: Organization.EmailAllowed
? new[] { Organization.ToMailAddress() }
: new MailAddress[0],
replyTo: new[] { Membership.Member.ToMailAddress() });
}

Expand Down
9 changes: 3 additions & 6 deletions ...NuGetGallery/Infrastructure/Mail/Messages/OrganizationMembershipRequestCanceledMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,10 @@ public OrganizationMembershipRequestCanceledMessage(

public override IEmailRecipients GetRecipients()
{
if (!PendingUser.EmailAllowed)
{
return EmailRecipients.None;
}

return new EmailRecipients(
to: new[] { PendingUser.ToMailAddress() },
to: PendingUser.EmailAllowed
? new[] { PendingUser.ToMailAddress() }
: new MailAddress[0],
replyTo: new[] { Organization.ToMailAddress() });
}

Expand Down
7 changes: 4 additions & 3 deletions ...NuGetGallery/Infrastructure/Mail/Messages/OrganizationMembershipRequestDeclinedMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@ public OrganizationMembershipRequestDeclinedMessage(

public override IEmailRecipients GetRecipients()
{
return new EmailRecipientsWithPermission(
Organization,
ActionsRequiringPermissions.ManageAccount,
return new EmailRecipients(
to: GalleryEmailRecipientsUtility.GetAddressesWithPermission(
Organization,
ActionsRequiringPermissions.ManageAccount),
replyTo: new[] { PendingUser.ToMailAddress() });
}

Expand Down
7 changes: 4 additions & 3 deletions ...uGetGallery/Infrastructure/Mail/Messages/OrganizationMembershipRequestInitiatedMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,10 @@ public OrganizationMembershipRequestInitiatedMessage(

public override IEmailRecipients GetRecipients()
{
return new EmailRecipientsWithPermission(
Organization,
ActionsRequiringPermissions.ManageAccount,
return new EmailRecipients(
to: GalleryEmailRecipientsUtility.GetAddressesWithPermission(
Organization,
ActionsRequiringPermissions.ManageAccount),
replyTo: new[] { RequestingUser.ToMailAddress() });
}

Expand Down
9 changes: 3 additions & 6 deletions src/NuGetGallery/Infrastructure/Mail/Messages/OrganizationMembershipRequestMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,10 @@ public OrganizationMembershipRequestMessage(

public override IEmailRecipients GetRecipients()
{
if (!NewUser.EmailAllowed)
{
return EmailRecipients.None;
}

return new EmailRecipients(
to: new[] { NewUser.ToMailAddress() },
to: NewUser.EmailAllowed
? new[] { NewUser.ToMailAddress() }
: new MailAddress[0],
replyTo: new[]
{
Organization.ToMailAddress(),
Expand Down
9 changes: 3 additions & 6 deletions src/NuGetGallery/Infrastructure/Mail/Messages/OrganizationTransformAcceptedMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,10 @@ public OrganizationTransformAcceptedMessage(

public override IEmailRecipients GetRecipients()
{
if (!_accountToTransform.EmailAllowed)
{
return EmailRecipients.None;
}

return new EmailRecipients(
to: new[] { _accountToTransform.ToMailAddress() },
to: _accountToTransform.EmailAllowed
? new[] { _accountToTransform.ToMailAddress() }
: new MailAddress[0],
replyTo: new[] { _adminUser.ToMailAddress() });
}

Expand Down
9 changes: 3 additions & 6 deletions src/NuGetGallery/Infrastructure/Mail/Messages/OrganizationTransformInitiatedMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,10 @@ public OrganizationTransformInitiatedMessage(

public override IEmailRecipients GetRecipients()
{
if (!_accountToTransform.EmailAllowed)
{
return EmailRecipients.None;
}

return new EmailRecipients(
to: new[] { _accountToTransform.ToMailAddress() },
to: _accountToTransform.EmailAllowed
? new[] { _accountToTransform.ToMailAddress() }
: new MailAddress[0],
replyTo: new[] { _adminUser.ToMailAddress() });
}

Expand Down
9 changes: 3 additions & 6 deletions src/NuGetGallery/Infrastructure/Mail/Messages/OrganizationTransformRejectedMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,10 @@ public OrganizationTransformRejectedMessage(

public override IEmailRecipients GetRecipients()
{
if (!AccountToSendTo.EmailAllowed)
{
return EmailRecipients.None;
}

return new EmailRecipients(
to: new[] { AccountToSendTo.ToMailAddress() },
to: AccountToSendTo.EmailAllowed
? new[] { AccountToSendTo.ToMailAddress() }
: new MailAddress[0],
replyTo: new[] { AccountToReplyTo.ToMailAddress() });
}

Expand Down
9 changes: 3 additions & 6 deletions src/NuGetGallery/Infrastructure/Mail/Messages/OrganizationTransformRequestMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,10 @@ public OrganizationTransformRequestMessage(

public override IEmailRecipients GetRecipients()
{
if (!_adminUser.EmailAllowed)
{
return EmailRecipients.None;
}

return new EmailRecipients(
to: new[] { _adminUser.ToMailAddress() },
to: _adminUser.EmailAllowed
? new[] { _adminUser.ToMailAddress() }
: new MailAddress[0],
replyTo: new[] { _accountToTransform.ToMailAddress() });
}

Expand Down
8 changes: 4 additions & 4 deletions src/NuGetGallery/Infrastructure/Mail/Messages/PackageDeletedNoticeMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ public PackageDeletedNoticeMessage(

public override IEmailRecipients GetRecipients()
{
var to = EmailRecipients.GetAllOwners(
Package.PackageRegistration,
requireEmailAllowed: false);
return new EmailRecipients(to);
return new EmailRecipients(
to: EmailRecipients.GetAllOwners(
Package.PackageRegistration,
requireEmailAllowed: false));
}

public override string GetSubject()
Expand Down
7 changes: 4 additions & 3 deletions src/NuGetGallery/Infrastructure/Mail/Messages/PackageOwnerAddedMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,10 @@ public PackageOwnerAddedMessage(

public override IEmailRecipients GetRecipients()
{
return new EmailRecipientsWithPermission(
ToUser,
ActionsRequiringPermissions.HandlePackageOwnershipRequest,
return new EmailRecipients(
to: GalleryEmailRecipientsUtility.GetAddressesWithPermission(
ToUser,
ActionsRequiringPermissions.HandlePackageOwnershipRequest),
replyTo: new[] { _configuration.GalleryNoReplyAddress });
}

Expand Down
7 changes: 4 additions & 3 deletions src/NuGetGallery/Infrastructure/Mail/Messages/PackageOwnerRemovedMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ public PackageOwnerRemovedMessage(

public override IEmailRecipients GetRecipients()
{
return new EmailRecipientsWithPermission(
ToUser,
ActionsRequiringPermissions.HandlePackageOwnershipRequest,
return new EmailRecipients(
to: GalleryEmailRecipientsUtility.GetAddressesWithPermission(
ToUser,
ActionsRequiringPermissions.HandlePackageOwnershipRequest),
replyTo: new[] { FromUser.ToMailAddress() });
}

Expand Down
7 changes: 4 additions & 3 deletions src/NuGetGallery/Infrastructure/Mail/Messages/PackageOwnershipRequestCanceledMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,10 @@ public PackageOwnershipRequestCanceledMessage(

public override IEmailRecipients GetRecipients()
{
return new EmailRecipientsWithPermission(
NewOwner,
ActionsRequiringPermissions.HandlePackageOwnershipRequest,
return new EmailRecipients(
to: GalleryEmailRecipientsUtility.GetAddressesWithPermission(
NewOwner,
ActionsRequiringPermissions.HandlePackageOwnershipRequest),
replyTo: new[] { RequestingOwner.ToMailAddress() });
}

Expand Down
14 changes: 5 additions & 9 deletions src/NuGetGallery/Infrastructure/Mail/Messages/PackageOwnershipRequestDeclinedMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,11 @@ public PackageOwnershipRequestDeclinedMessage(

public override IEmailRecipients GetRecipients()
{
if (!RequestingOwner.EmailAllowed)
{
// This will prevent the email from being sent.
return EmailRecipients.None;
}

return new EmailRecipientsWithPermission(
RequestingOwner,
ActionsRequiringPermissions.HandlePackageOwnershipRequest,
return new EmailRecipients(
to: RequestingOwner.EmailAllowed
? GalleryEmailRecipientsUtility.GetAddressesWithPermission(
RequestingOwner, ActionsRequiringPermissions.HandlePackageOwnershipRequest)
: new MailAddress[0],
replyTo: new[] { NewOwner.ToMailAddress() });
}

Expand Down
7 changes: 4 additions & 3 deletions src/NuGetGallery/Infrastructure/Mail/Messages/PackageOwnershipRequestInitiatedMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,10 @@ public PackageOwnershipRequestInitiatedMessage(

public override IEmailRecipients GetRecipients()
{
return new EmailRecipientsWithPermission(
ReceivingOwner,
ActionsRequiringPermissions.HandlePackageOwnershipRequest,
return new EmailRecipients(
to: GalleryEmailRecipientsUtility.GetAddressesWithPermission(
ReceivingOwner,
ActionsRequiringPermissions.HandlePackageOwnershipRequest),
replyTo: new[] { NewOwner.ToMailAddress() });
}

Expand Down
7 changes: 4 additions & 3 deletions src/NuGetGallery/Infrastructure/Mail/Messages/PackageOwnershipRequestMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,10 @@ public PackageOwnershipRequestMessage(

public override IEmailRecipients GetRecipients()
{
return new EmailRecipientsWithPermission(
ToUser,
ActionsRequiringPermissions.HandlePackageOwnershipRequest,
return new EmailRecipients(
to: GalleryEmailRecipientsUtility.GetAddressesWithPermission(
ToUser,
ActionsRequiringPermissions.HandlePackageOwnershipRequest),
replyTo: new[] { FromUser.ToMailAddress() });
}

Expand Down
Loading