DeleteAccountService should clean up Package.User and PackageDeprecation.DeprecatedBy (#7076)

Author: Scott Bommarito

src/NuGet.Services.Entities/Package.cs

@@ -217,9 +217,11 @@ public bool HasReadMe
         public string MinClientVersion { get; set; }
 
         /// <summary>
-        /// The logged in user when this package version was created.
-        /// NULL for older packages.
+        /// The user that uploaded this package or <c>null</c> if the user was deleted.
         /// </summary>
+        /// <remarks>
+        /// Packages uploaded before this field was added have <c>null</c>.
+        /// </remarks>
         public User User { get; set; }
         public int? UserKey { get; set; }
 

src/NuGet.Services.Entities/PackageDeprecation.cs

@@ -72,6 +72,9 @@ public PackageDeprecation()
         /// <summary>
         /// Gets or sets the user that executed the package deprecation.
         /// </summary>
+        /// <remarks>
+        /// This field will be <c>null</c> if the user that deprecated the package has been deleted.
+        /// </remarks>
         public virtual User DeprecatedByUser { get; set; }
 
         /// <summary>

src/NuGetGallery.Core/Entities/EntitiesContext.cs

@@ -49,6 +49,7 @@ public EntitiesContext(DbConnection connection, bool readOnly)
         }
 
         public bool ReadOnly { get; private set; }
+        public DbSet<Package> Packages { get; set; }
         public DbSet<PackageRegistration> PackageRegistrations { get; set; }
         public DbSet<Credential> Credentials { get; set; }
         public DbSet<Scope> Scopes { get; set; }

src/NuGetGallery.Core/Entities/IEntitiesContext.cs

@@ -10,6 +10,7 @@ namespace NuGetGallery
     public interface IEntitiesContext
     {
         DbSet<Certificate> Certificates { get; set; }
+        DbSet<Package> Packages { get; set; }
         DbSet<PackageRegistration> PackageRegistrations { get; set; }
         DbSet<Credential> Credentials { get; set; }
         DbSet<Scope> Scopes { get; set; }

src/NuGetGallery/Services/DeleteAccountService.cs

@@ -3,7 +3,6 @@
 
 using System;
 using System.Collections.Generic;
-using System.Data.Entity;
 using System.Globalization;
 using System.Linq;
 using System.Threading.Tasks;
@@ -26,6 +25,7 @@ public class DeleteAccountService : IDeleteAccountService
         private readonly IReservedNamespaceService _reservedNamespaceService;
         private readonly ISecurityPolicyService _securityPolicyService;
         private readonly AuthenticationService _authService;
+        private readonly IEntityRepository<PackageDeprecation> _deprecationRepository;
         private readonly IEntityRepository<User> _userRepository;
         private readonly IEntityRepository<Scope> _scopeRepository;
         private readonly ISupportRequestService _supportRequestService;
@@ -35,6 +35,7 @@ public class DeleteAccountService : IDeleteAccountService
 
         public DeleteAccountService(
             IEntityRepository<AccountDelete> accountDeleteRepository,
+            IEntityRepository<PackageDeprecation> deprecationRepository,
             IEntityRepository<User> userRepository,
             IEntityRepository<Scope> scopeRepository,
             IEntitiesContext entitiesContext,
@@ -49,6 +50,7 @@ public DeleteAccountService(
             ITelemetryService telemetryService)
         {
             _accountDeleteRepository = accountDeleteRepository ?? throw new ArgumentNullException(nameof(accountDeleteRepository));
+            _deprecationRepository = deprecationRepository ?? throw new ArgumentNullException(nameof(deprecationRepository));
             _userRepository = userRepository ?? throw new ArgumentNullException(nameof(userRepository));
             _scopeRepository = scopeRepository ?? throw new ArgumentNullException(nameof(scopeRepository));
             _entitiesContext = entitiesContext ?? throw new ArgumentNullException(nameof(entitiesContext));
@@ -110,6 +112,9 @@ private async Task DeleteAccountImplAsync(User userToBeDeleted, User userToExecu
             await RemoveUserCredentials(userToBeDeleted);
             await RemovePackageOwnershipRequests(userToBeDeleted);
 
+            RemovePackagePushedBy(userToBeDeleted);
+            RemovePackageDeprecatedBy(userToBeDeleted);
+
             var organizationToBeDeleted = userToBeDeleted as Organization;
             if (organizationToBeDeleted != null)
             {
@@ -209,6 +214,17 @@ private async Task RemovePackageOwnership(User user, User requestingUser, Accoun
             }
         }
 
+        private void RemovePackagePushedBy(User user)
+        {
+            foreach (var package in _entitiesContext
+                .Packages
+                .Where(p => p.UserKey == user.Key)
+                .ToList())
+            {
+                package.User = null;
+            }
+        }
+
         private List<Package> GetPackagesOwnedByUser(User user)
         {
             return _packageService
@@ -218,15 +234,32 @@ private List<Package> GetPackagesOwnedByUser(User user)
 
         private async Task RemovePackageOwnershipRequests(User user)
         {
-            var requests = _packageOwnershipManagementService
+            var toRequests = _packageOwnershipManagementService
                 .GetPackageOwnershipRequests(newOwner: user)
                 .ToList();
 
+            var fromRequests = _packageOwnershipManagementService
+                .GetPackageOwnershipRequests(requestingOwner: user)
+                .ToList();
+
+            var requests = toRequests.Concat(fromRequests).ToList();
+
             foreach (var request in requests)
             {
                 await _packageOwnershipManagementService.DeletePackageOwnershipRequestAsync(request.PackageRegistration, request.NewOwner, commitChanges: false);
             }
         }
+
+        private void RemovePackageDeprecatedBy(User user)
+        {
+            foreach (var deprecation in _deprecationRepository
+                .GetAll()
+                .Where(d => d.DeprecatedByUserKey == user.Key)
+                .ToList())
+            {
+                deprecation.DeprecatedByUser = null;
+            }
+        }
 
         private async Task RemoveMemberships(User user, User requestingUser, AccountDeletionOrphanPackagePolicy orphanPackagePolicy)
         {

src/NuGetGallery/Services/PackageService.cs

@@ -368,6 +368,7 @@ public async Task RemovePackageOwnerAsync(PackageRegistration package, User user
         {
             // To support the delete account scenario, the admin can delete the last owner of a package.
             package.Owners.Remove(user);
+
             if (commitChanges)
             {
                 await _packageRepository.CommitChangesAsync();

tests/NuGetGallery.Facts/Services/DeleteAccountServiceFacts.cs

@@ -131,6 +131,8 @@ await deleteAccountService.DeleteAccountAsync(
                     Assert.NotEmpty(testUser.SecurityPolicies);
                     Assert.True(registration.Packages.Single().Listed);
                     Assert.NotNull(testUser.EmailAddress);
+                    Assert.NotNull(testableService.PackagePushedByUser.User);
+                    Assert.NotNull(testableService.DeprecationDeprecatedByUser.DeprecatedByUser);
                     Assert.Empty(testableService.DeletedAccounts);
                     Assert.NotEmpty(testableService.PackageOwnerRequests);
                     Assert.False(testableService.HasDeletedOwnerScope);
@@ -148,6 +150,8 @@ await deleteAccountService.DeleteAccountAsync(
                         orphanPolicy == AccountDeletionOrphanPackagePolicy.UnlistOrphans && isPackageOrphaned,
                         !registration.Packages.Single().Listed);
                     Assert.Null(testUser.EmailAddress);
+                    Assert.Null(testableService.PackagePushedByUser.User);
+                    Assert.Null(testableService.DeprecationDeprecatedByUser.DeprecatedByUser);
                     Assert.Single(testableService.DeletedAccounts);
                     Assert.Empty(testableService.PackageOwnerRequests);
                     Assert.True(testableService.HasDeletedOwnerScope);
@@ -259,6 +263,8 @@ public async Task DeleteOrganization(bool isPackageOrphaned, AccountDeletionOrph
                     Assert.Equal(organization.Confirmed, organization.EmailAddress != null);
                     Assert.True(registration.Owners.Any(o => o.MatchesUser(organization)));
                     Assert.NotEmpty(organization.SecurityPolicies);
+                    Assert.NotNull(testableService.PackagePushedByUser.User);
+                    Assert.NotNull(testableService.DeprecationDeprecatedByUser.DeprecatedByUser);
                     Assert.Empty(testableService.DeletedAccounts);
                     Assert.NotEmpty(testableService.PackageOwnerRequests);
                     Assert.Empty(testableService.AuditService.Records);
@@ -274,6 +280,8 @@ public async Task DeleteOrganization(bool isPackageOrphaned, AccountDeletionOrph
                         !registration.Packages.Single().Listed);
                     Assert.False(registration.Owners.Any(o => o.MatchesUser(organization)));
                     Assert.Empty(organization.SecurityPolicies);
+                    Assert.Null(testableService.PackagePushedByUser.User);
+                    Assert.Null(testableService.DeprecationDeprecatedByUser.DeprecatedByUser);
                     Assert.Single(testableService.DeletedAccounts);
                     Assert.Empty(testableService.PackageOwnerRequests);
                     Assert.Single(testableService.AuditService.Records);
@@ -334,6 +342,8 @@ public async Task DeleteUnconfirmedOrganization(bool isPackageOrphaned)
                 Assert.Empty(registration.Owners);
                 Assert.Empty(organization.SecurityPolicies);
                 Assert.Empty(organization.ReservedNamespaces);
+                Assert.Null(testableService.PackagePushedByUser.User);
+                Assert.Null(testableService.DeprecationDeprecatedByUser.DeprecatedByUser);
                 Assert.Empty(testableService.DeletedAccounts);
                 Assert.Single(testableService.SupportRequests);
                 Assert.Empty(testableService.PackageOwnerRequests);
@@ -467,6 +477,8 @@ public class DeleteAccountTestService
             public List<AccountDelete> DeletedAccounts = new List<AccountDelete>();
             public List<User> DeletedUsers = new List<User>();
             public List<Issue> SupportRequests = new List<Issue>();
+            public Package PackagePushedByUser;
+            public PackageDeprecation DeprecationDeprecatedByUser;
             public List<PackageOwnerRequest> PackageOwnerRequests = new List<PackageOwnerRequest>();
             public FakeAuditingService AuditService = new FakeAuditingService();
             public bool HasDeletedOwnerScope => _hasDeletedCredentialWithOwnerScope;
@@ -520,6 +532,18 @@ public DeleteAccountTestService(User user, PackageRegistration userPackagesRegis
                     PackageRegistration = new PackageRegistration() { Id = $"{user.Username}_second" },
                     NewOwner = _user
                 });
+
+                PackagePushedByUser = new Package
+                {
+                    User = _user,
+                    UserKey = _user.Key
+                };
+
+                DeprecationDeprecatedByUser = new PackageDeprecation
+                {
+                    DeprecatedByUser = _user,
+                    DeprecatedByUserKey = _user.Key
+                };
             }
 
             public DeleteAccountTestService()
@@ -528,7 +552,9 @@ public DeleteAccountTestService()
 
             public DeleteAccountService GetDeleteAccountService(bool isPackageOrphaned, bool isFeatureFlagsRemovalSuccessful = true)
             {
-                return new DeleteAccountService(SetupAccountDeleteRepository().Object,
+                return new DeleteAccountService(
+                    SetupAccountDeleteRepository().Object,
+                    SetupDeprecationRepository().Object,
                     SetupUserRepository().Object,
                     SetupScopeRepository().Object,
                     SetupEntitiesContext().Object,
@@ -563,8 +589,24 @@ private Mock<IEntitiesContext> SetupEntitiesContext()
             {
                 var mockContext = new Mock<IEntitiesContext>();
                 var database = new Mock<IDatabase>();
-                database.Setup(x => x.BeginTransaction()).Returns(() => new Mock<IDbContextTransaction>().Object);
-                mockContext.Setup(m => m.GetDatabase()).Returns(database.Object);
+                database
+                    .Setup(x => x.BeginTransaction())
+                    .Returns(() => new Mock<IDbContextTransaction>().Object);
+
+                mockContext
+                    .Setup(m => m.GetDatabase())
+                    .Returns(database.Object);
+
+                var packageDbSet = FakeEntitiesContext.CreateDbSet<Package>();
+                mockContext
+                    .Setup(x => x.Packages)
+                    .Returns(packageDbSet);
+
+                if (PackagePushedByUser != null)
+                {
+                    packageDbSet.Add(PackagePushedByUser);
+                }
+
                 return mockContext;
             }
 
@@ -574,8 +616,8 @@ private Mock<IReservedNamespaceService> SetupReservedNamespaceService()
                 if (_user != null)
                 {
                     namespaceService.Setup(m => m.DeleteOwnerFromReservedNamespaceAsync(It.IsAny<string>(), It.IsAny<string>(), false))
-                                    .Returns(Task.CompletedTask)
-                                    .Callback(() => _user.ReservedNamespaces.Remove(_reservedNamespace));
+                        .Returns(Task.CompletedTask)
+                        .Callback(() => _user.ReservedNamespaces.Remove(_reservedNamespace));
                 }
 
                 return namespaceService;
@@ -587,8 +629,8 @@ private Mock<ISecurityPolicyService> SetupSecurityPolicyService()
                 if (_user != null)
                 {
                     securityPolicyService.Setup(m => m.UnsubscribeAsync(_user, SubscriptionName, false))
-                                         .Returns(Task.CompletedTask)
-                                         .Callback(() => _user.SecurityPolicies.Remove(_securityPolicy));
+                        .Returns(Task.CompletedTask)
+                        .Callback(() => _user.SecurityPolicies.Remove(_securityPolicy));
                 }
 
                 return securityPolicyService;
@@ -597,11 +639,27 @@ private Mock<ISecurityPolicyService> SetupSecurityPolicyService()
             private Mock<IEntityRepository<AccountDelete>> SetupAccountDeleteRepository()
             {
                 var accountDeleteRepository = new Mock<IEntityRepository<AccountDelete>>();
-                accountDeleteRepository.Setup(m => m.InsertOnCommit(It.IsAny<AccountDelete>()))
-                                      .Callback<AccountDelete>(account => DeletedAccounts.Add(account));
+                accountDeleteRepository
+                    .Setup(m => m.InsertOnCommit(It.IsAny<AccountDelete>()))
+                    .Callback<AccountDelete>(account => DeletedAccounts.Add(account));
+
                 return accountDeleteRepository;
             }
 
+            private Mock<IEntityRepository<PackageDeprecation>> SetupDeprecationRepository()
+            {
+                var deprecationRepository = new Mock<IEntityRepository<PackageDeprecation>>();
+                var deprecations = DeprecationDeprecatedByUser == null
+                    ? Enumerable.Empty<PackageDeprecation>()
+                    : new[] { DeprecationDeprecatedByUser };
+
+                deprecationRepository
+                    .Setup(x => x.GetAll())
+                    .Returns(deprecations.AsQueryable());
+
+                return deprecationRepository;
+            }
+
             private Mock<IEntityRepository<User>> SetupUserRepository()
             {
                 var userRepository = new Mock<IEntityRepository<User>>();
@@ -657,8 +715,8 @@ private Mock<IPackageService> SetupPackageService(bool isPackageOrphaned)
 
                 //the .Returns(Task.CompletedTask) to avoid NullRef exception by the Mock infrastructure when invoking async operations
                 packageService.Setup(m => m.MarkPackageUnlistedAsync(It.IsAny<Package>(), false))
-                              .Returns(Task.CompletedTask)
-                              .Callback<Package, bool>((package, commit) => { package.Listed = false; });
+                    .Returns(Task.CompletedTask)
+                    .Callback<Package, bool>((package, commit) => { package.Listed = false; });
 
                 return packageService;
             }
@@ -689,8 +747,8 @@ private Mock<ISupportRequestService> SetupSupportRequestService()
                 {
                     var issue = SupportRequests.Where(i => string.Equals(i.CreatedBy, _user.Username)).FirstOrDefault();
                     supportService.Setup(m => m.DeleteSupportRequestsAsync(_user))
-                                  .Returns(Task.FromResult(true))
-                                  .Callback(() => SupportRequests.Remove(issue));
+                        .Returns(Task.FromResult(true))
+                        .Callback(() => SupportRequests.Remove(issue));
                 }
 
                 return supportService;
@@ -715,13 +773,14 @@ private Mock<IPackageOwnershipManagementService> SetupPackageOwnershipManagement
                 var packageOwnershipManagementService = new Mock<IPackageOwnershipManagementService>();
                 if (_user != null)
                 {
-                    packageOwnershipManagementService.Setup(m => m.RemovePackageOwnerAsync(It.IsAny<PackageRegistration>(), It.IsAny<User>(), It.IsAny<User>(), false))
-                                                     .Returns(Task.CompletedTask)
-                                                     .Callback(() =>
-                                                     {
-                                                         _userPackagesRegistration.Owners.Remove(_user);
-                                                         _userPackagesRegistration.ReservedNamespaces.Remove(_reservedNamespace);
-                                                     });
+                    packageOwnershipManagementService
+                        .Setup(m => m.RemovePackageOwnerAsync(It.IsAny<PackageRegistration>(), It.IsAny<User>(), It.IsAny<User>(), false))
+                        .Returns(Task.CompletedTask)
+                        .Callback(() =>
+                        {
+                            _userPackagesRegistration.Owners.Remove(_user);
+                            _userPackagesRegistration.ReservedNamespaces.Remove(_reservedNamespace);
+                        });
 
                     packageOwnershipManagementService.Setup(m => m.GetPackageOwnershipRequests(null, null, _user))
                         .Returns(PackageOwnerRequests);

tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs

@@ -1986,7 +1986,7 @@ public async Task EnsureValidThrowsForSymbolsPackage()
         public class TheRemovePackageOwnerMethod
         {
             [Fact]
-            public async Task RemovesPackageOwner()
+            public async Task WillRemoveOneOfManyOwners()
             {
                 var service = CreateService();
                 var owner1 = new User { Key = 1, Username = "Owner1" };
@@ -1999,7 +1999,7 @@ public async Task RemovesPackageOwner()
             }
 
             [Fact]
-            public async Task WontRemoveLastOwner()
+            public async Task WillRemoveLastOwner()
             {
                 var service = CreateService();
                 var singleOwner = new User { Key = 1, Username = "Owner" };