Skip to content

Commit 678fe47

Browse files
joelverhagenjoelverhagen
committed
Allow deletion of unconfirmed users and orgs (#7948)
Fix minor styling issue on the delete account page when an exception bubbles out. Fix bug in the account delete handler which would not delete users when notifications were turned off. Address NuGet/Engineering#3099
1 parent b76fd7f commit 678fe47

10 files changed

Lines changed: 196 additions & 46 deletions

File tree

‎src/AccountDeleter/AccountDeleteMessageHandler.cs‎

Lines changed: 31 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -63,31 +63,42 @@ public async Task<bool> HandleAsync(AccountDeleteMessage command)
6363
throw new UserNotFoundException();
6464
}
6565

66-
if (_accountDeleteConfigurationAccessor.Value.RespectEmailContactSetting && !user.EmailAllowed)
67-
{
68-
throw new EmailContactNotAllowedException();
69-
}
70-
7166
var recipientEmail = user.EmailAddress;
67+
var emailAllowed = user.EmailAllowed;
7268
var deleteSuccess = await _accountManager.DeleteAccount(user, source);
7369
_telemetryService.TrackDeleteResult(source, deleteSuccess);
7470

75-
var baseEmailBuilder = _emailBuilderFactory.GetEmailBuilder(source, deleteSuccess);
76-
if (baseEmailBuilder != null)
71+
if (recipientEmail == null)
72+
{
73+
_logger.LogWarning("User has no confirmed email address. The user has been deleted but no email was sent.");
74+
_telemetryService.TrackUnconfirmedUser(source);
75+
messageProcessed = true;
76+
}
77+
else if (_accountDeleteConfigurationAccessor.Value.RespectEmailContactSetting && !emailAllowed)
7778
{
78-
var toEmail = new List<MailAddress>();
79+
_logger.LogWarning("User did not allow Email Contact. The user has been deleted but no email was sent.");
80+
_telemetryService.TrackEmailBlocked(source);
81+
messageProcessed = true;
82+
}
83+
else
84+
{
85+
var baseEmailBuilder = _emailBuilderFactory.GetEmailBuilder(source, deleteSuccess);
86+
if (baseEmailBuilder != null)
87+
{
88+
var toEmail = new List<MailAddress>();
7989

80-
var configuration = _accountDeleteConfigurationAccessor.Value;
81-
var senderAddress = configuration.EmailConfiguration.GalleryOwner;
82-
var ccEmail = new List<MailAddress>();
83-
toEmail.Add(new MailAddress(recipientEmail));
84-
ccEmail.Add(new MailAddress(senderAddress));
90+
var configuration = _accountDeleteConfigurationAccessor.Value;
91+
var senderAddress = configuration.EmailConfiguration.GalleryOwner;
92+
var ccEmail = new List<MailAddress>();
93+
toEmail.Add(new MailAddress(recipientEmail));
94+
ccEmail.Add(new MailAddress(senderAddress));
8595

86-
var recipients = new EmailRecipients(toEmail, ccEmail);
87-
var emailBuilder = new DisposableEmailBuilder(baseEmailBuilder, recipients, username);
88-
await _messenger.SendMessageAsync(emailBuilder);
89-
_telemetryService.TrackEmailSent(source, user.EmailAllowed);
90-
messageProcessed = true;
96+
var recipients = new EmailRecipients(toEmail, ccEmail);
97+
var emailBuilder = new DisposableEmailBuilder(baseEmailBuilder, recipients, username);
98+
await _messenger.SendMessageAsync(emailBuilder);
99+
_telemetryService.TrackEmailSent(source, emailAllowed);
100+
messageProcessed = true;
101+
}
91102
}
92103
}
93104
catch (UnknownSourceException)
@@ -98,12 +109,6 @@ public async Task<bool> HandleAsync(AccountDeleteMessage command)
98109
_telemetryService.TrackUnknownSource(source);
99110
messageProcessed = false;
100111
}
101-
catch (EmailContactNotAllowedException)
102-
{
103-
// Should we not send? or should we ignore the setting.
104-
_logger.LogWarning("User did not allow Email Contact.");
105-
_telemetryService.TrackEmailBlocked(source);
106-
}
107112
catch (UserNotFoundException)
108113
{
109114
_logger.LogWarning("User was not found. They may have already been deleted.");
@@ -112,8 +117,8 @@ public async Task<bool> HandleAsync(AccountDeleteMessage command)
112117
}
113118
catch (Exception e)
114119
{
115-
_logger.LogError(0, e, "An unknown exception occured: {ExceptionMessage}");
116-
throw e;
120+
_logger.LogError(e, "An unknown exception occurred.");
121+
throw;
117122
}
118123

119124
return messageProcessed;

‎src/AccountDeleter/AccountDeleter.csproj‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@
6464
<Compile Include="Evaluators\IUserEvaluatorFactory.cs" />
6565
<Compile Include="Evaluators\UserEvaluatorComparer.cs" />
6666
<Compile Include="Evaluators\UserEvaluatorFactory.cs" />
67-
<Compile Include="Exceptions\EmailContactNotAllowedException.cs" />
6867
<Compile Include="Exceptions\UnknownEvaluatorException.cs" />
6968
<Compile Include="Exceptions\UnknownSourceException.cs" />
7069
<Compile Include="Exceptions\UserNotFoundException.cs" />

‎src/AccountDeleter/Exceptions/EmailContactNotAllowedException.cs‎

Lines changed: 0 additions & 12 deletions
This file was deleted.

‎src/AccountDeleter/Exceptions/UserNotFoundException.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4-
54
using System;
65

76
namespace NuGetGallery.AccountDeleter

‎src/AccountDeleter/Telemetry/AccountDeleteTelemetryService.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ public class AccountDeleteTelemetryService : IAccountDeleteTelemetryService, ISu
1717
private const string EmailBlockedEventName = TelemetryPrefix + "EmailBlocked";
1818
private const string UnknownSourceEventName = TelemetryPrefix + "UnknownSource";
1919
private const string UserNotFoundEventName = TelemetryPrefix + "UserNotFound";
20+
private const string UnconfirmedUserEventName = TelemetryPrefix + "UnconfirmedUser";
2021

2122
private const string CallGuidDimensionName = "CallGuid";
2223
private const string ContactAllowedDimensionName = "ContactAllowed";
@@ -130,5 +131,14 @@ public void TrackMessageLockLost<TMessage>(Guid callGuid)
130131
{ CallGuidDimensionName, callGuid.ToString() }
131132
});
132133
}
134+
135+
public void TrackUnconfirmedUser(string source)
136+
{
137+
_telemetryClient.TrackEvent(UnconfirmedUserEventName,
138+
new Dictionary<string, string>
139+
{
140+
{ SourceDimensionName, source }
141+
});
142+
}
133143
}
134144
}

‎src/AccountDeleter/Telemetry/IAccountDeleteTelemetryService.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4-
using System;
5-
64
namespace NuGetGallery.AccountDeleter
75
{
86
public interface IAccountDeleteTelemetryService
97
{
8+
void TrackUnconfirmedUser(string source);
9+
1010
void TrackUserNotFound(string source);
1111

1212
void TrackDeleteResult(string source, bool deleteSuccess);

‎src/NuGetGallery.Services/SupportRequest/SupportRequestService.cs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ public class SupportRequestService
2121
private readonly string _siteRoot;
2222
private const string _unassignedAdmin = "unassigned";
2323
private const string _deletedAccount = "_deletedaccount";
24+
private const string _unconfirmedEmailAddress = "_unconfirmed";
25+
private const string _deletedEmailAddress = "deletedaccount";
2426
private const string _NuGetDSRAccount = "_NuGetDSR";
2527

2628
public SupportRequestService(
@@ -221,7 +223,7 @@ public async Task<bool> TryAddDeleteSupportRequestAsync(User user)
221223
var requestSent = await AddNewSupportRequestAsync(
222224
ServicesStrings.AccountDelete_SupportRequestTitle,
223225
ServicesStrings.AccountDelete_SupportRequestTitle,
224-
user.EmailAddress,
226+
user.EmailAddress ?? _unconfirmedEmailAddress,
225227
"The user requested to have the account deleted.",
226228
user) != null;
227229
var status = requestSent ? DeleteAccountAuditRecord.ActionStatus.Success : DeleteAccountAuditRecord.ActionStatus.Failure;
@@ -303,7 +305,7 @@ public async Task DeleteSupportRequestsAsync(User user)
303305
}
304306
foreach (var accountDeletedIssue in userIssues.Where(i => string.Equals(i.IssueTitle, ServicesStrings.AccountDelete_SupportRequestTitle)))
305307
{
306-
accountDeletedIssue.OwnerEmail = "deletedaccount";
308+
accountDeletedIssue.OwnerEmail = _deletedEmailAddress;
307309
if(!accountDeletedIssue.CreatedBy.Equals(_NuGetDSRAccount, StringComparison.OrdinalIgnoreCase))
308310
{
309311
accountDeletedIssue.CreatedBy = _deletedAccount;

‎src/NuGetGallery/Views/Users/DeleteAccount.cshtml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
@if ((TempData["RequestFailedMessage"] != null))
1313
{
1414
var message = (string)TempData["RequestFailedMessage"];
15-
@ViewHelpers.AlertDanger(@<p><b class="keywords">@message</b> <br /></p>)
15+
@ViewHelpers.AlertDanger(@<b class="keywords">@message</b>)
1616
}
1717
else
1818
{

‎tests/AccountDeleter.Facts/AccountDeleteMessageHandlerFacts.cs‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
using System.Collections.Generic;
5+
using System.Threading.Tasks;
6+
using Microsoft.Extensions.Logging;
7+
using Microsoft.Extensions.Options;
8+
using Moq;
9+
using NuGet.Services.Entities;
10+
using NuGet.Services.Messaging.Email;
11+
using Xunit;
12+
using Xunit.Abstractions;
13+
14+
namespace NuGetGallery.AccountDeleter.Facts
15+
{
16+
public class AccountDeleteMessageHandlerFacts
17+
{
18+
[Fact]
19+
public async Task IgnoresMissingUser()
20+
{
21+
User = null;
22+
23+
var processed = await Target.HandleAsync(Message);
24+
25+
Assert.True(processed);
26+
TelemetryService.Verify(x => x.TrackUserNotFound(Message.Source), Times.Once);
27+
}
28+
29+
[Theory]
30+
[InlineData(false, false)]
31+
[InlineData(false, true)]
32+
[InlineData(true, true)]
33+
[InlineData(true, false)]
34+
public async Task DeletesUserAndAlwaysSendsEmailWhenIgnoringContactSetting(bool emailAllowed, bool deleteSuccess)
35+
{
36+
Config.RespectEmailContactSetting = false;
37+
User.EmailAllowed = emailAllowed;
38+
AccountManager.Setup(x => x.DeleteAccount(It.IsAny<User>(), It.IsAny<string>())).ReturnsAsync(deleteSuccess);
39+
40+
var processed = await Target.HandleAsync(Message);
41+
42+
Assert.True(processed);
43+
TelemetryService.Verify(x => x.TrackDeleteResult(Message.Source, deleteSuccess), Times.Once);
44+
MessageService.Verify(x => x.SendMessageAsync(It.IsAny<IEmailBuilder>(), false, false), Times.Once);
45+
TelemetryService.Verify(x => x.TrackEmailSent(Message.Source, emailAllowed), Times.Once);
46+
}
47+
48+
[Theory]
49+
[InlineData(false, false)]
50+
[InlineData(false, true)]
51+
[InlineData(true, true)]
52+
[InlineData(true, false)]
53+
public async Task DeletesUserAndAndCanRespsectEmailAllowedSetting(bool emailAllowed, bool deleteSuccess)
54+
{
55+
Config.RespectEmailContactSetting = true;
56+
User.EmailAllowed = emailAllowed;
57+
AccountManager.Setup(x => x.DeleteAccount(It.IsAny<User>(), It.IsAny<string>())).ReturnsAsync(deleteSuccess);
58+
59+
var processed = await Target.HandleAsync(Message);
60+
61+
Assert.True(processed);
62+
TelemetryService.Verify(x => x.TrackDeleteResult(Message.Source, deleteSuccess), Times.Once);
63+
MessageService.Verify(
64+
x => x.SendMessageAsync(It.IsAny<IEmailBuilder>(), It.IsAny<bool>(), It.IsAny<bool>()),
65+
Times.Exactly(emailAllowed ? 1 : 0));
66+
TelemetryService.Verify(x => x.TrackEmailSent(It.IsAny<string>(), It.IsAny<bool>()), Times.Exactly(emailAllowed ? 1 : 0));
67+
TelemetryService.Verify(x => x.TrackEmailBlocked(It.IsAny<string>()), Times.Exactly(emailAllowed ? 0 : 1));
68+
}
69+
70+
[Fact]
71+
public async Task DoesNotSendEmailWhenUserHasNoConfirmedEmailAddress()
72+
{
73+
User.EmailAddress = null;
74+
75+
var processed = await Target.HandleAsync(Message);
76+
77+
Assert.True(processed);
78+
TelemetryService.Verify(x => x.TrackDeleteResult(Message.Source, true), Times.Once);
79+
TelemetryService.Verify(x => x.TrackUnconfirmedUser(Message.Source), Times.Once);
80+
MessageService.Verify(x => x.SendMessageAsync(It.IsAny<IEmailBuilder>(), It.IsAny<bool>(), It.IsAny<bool>()), Times.Never);
81+
TelemetryService.Verify(x => x.TrackEmailSent(It.IsAny<string>(), It.IsAny<bool>()), Times.Never);
82+
}
83+
84+
public AccountDeleteMessageHandlerFacts(ITestOutputHelper output)
85+
{
86+
Options = new Mock<IOptionsSnapshot<AccountDeleteConfiguration>>();
87+
AccountManager = new Mock<IAccountManager>();
88+
UserService = new Mock<IUserService>();
89+
MessageService = new Mock<IMessageService>();
90+
EmailBuilderFactory = new Mock<IEmailBuilderFactory>();
91+
TelemetryService = new Mock<IAccountDeleteTelemetryService>();
92+
93+
Message = new AccountDeleteMessage("frank", "TheMoon");
94+
Config = new AccountDeleteConfiguration
95+
{
96+
RespectEmailContactSetting = false,
97+
EmailConfiguration = new EmailConfiguration
98+
{
99+
GalleryOwner = "gallery@example",
100+
},
101+
SourceConfigurations = new List<SourceConfiguration>
102+
{
103+
new SourceConfiguration
104+
{
105+
SourceName = Message.Source,
106+
},
107+
},
108+
};
109+
User = new User
110+
{
111+
Username = Message.Username,
112+
EmailAddress = "frank@example",
113+
EmailAllowed = true,
114+
};
115+
BaseEmailBuilder = new Mock<IEmailBuilder>();
116+
117+
Options.Setup(x => x.Value).Returns(() => Config);
118+
UserService.Setup(x => x.FindByUsername(It.IsAny<string>(), It.IsAny<bool>())).Returns(() => User);
119+
AccountManager.Setup(x => x.DeleteAccount(It.IsAny<User>(), It.IsAny<string>())).ReturnsAsync(true);
120+
EmailBuilderFactory.Setup(x => x.GetEmailBuilder(It.IsAny<string>(), It.IsAny<bool>())).Returns(() => BaseEmailBuilder.Object);
121+
122+
var loggerFactory = new LoggerFactory().AddXunit(output);
123+
124+
Target = new AccountDeleteMessageHandler(
125+
Options.Object,
126+
AccountManager.Object,
127+
UserService.Object,
128+
MessageService.Object,
129+
EmailBuilderFactory.Object,
130+
TelemetryService.Object,
131+
loggerFactory.CreateLogger<AccountDeleteMessageHandler>());
132+
}
133+
134+
public Mock<IOptionsSnapshot<AccountDeleteConfiguration>> Options { get; }
135+
public Mock<IAccountManager> AccountManager { get; }
136+
public Mock<IUserService> UserService { get; }
137+
public Mock<IMessageService> MessageService { get; }
138+
public Mock<IEmailBuilderFactory> EmailBuilderFactory { get; }
139+
public Mock<IAccountDeleteTelemetryService> TelemetryService { get; }
140+
public AccountDeleteConfiguration Config { get; }
141+
public AccountDeleteMessageHandler Target { get; }
142+
public AccountDeleteMessage Message { get; }
143+
public User User { get; set; }
144+
public Mock<IEmailBuilder> BaseEmailBuilder { get; }
145+
}
146+
}

‎tests/AccountDeleter.Facts/AccountDeleter.Facts.csproj‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
<ProjectGuid>{98765110-844D-41BE-8083-22E064136E05}</ProjectGuid>
88
<OutputType>Library</OutputType>
99
<AppDesignerFolder>Properties</AppDesignerFolder>
10-
<RootNamespace>AccountDeleter.Facts</RootNamespace>
10+
<RootNamespace>NuGet.AccountDeleter.Facts</RootNamespace>
1111
<AssemblyName>AccountDeleter.Facts</AssemblyName>
1212
<TargetFrameworkVersion>v4.7.2</TargetFrameworkVersion>
1313
<FileAlignment>512</FileAlignment>
@@ -40,6 +40,7 @@
4040
<Reference Include="System.Xml" />
4141
</ItemGroup>
4242
<ItemGroup>
43+
<Compile Include="AccountDeleteMessageHandlerFacts.cs" />
4344
<Compile Include="EmailBuilderFacts.cs" />
4445
<Compile Include="Properties\AssemblyInfo.cs" />
4546
<Compile Include="EvaluatorFacts.cs" />

0 commit comments

Comments
 (0)