AccountDelete.DeletedBy and PackageDelete.DeletedBy should be set to null when the User is deleted (#7104)

Author: Scott Bommarito

src/NuGet.Services.Entities/AccountDelete.cs

@@ -28,13 +28,19 @@ public class AccountDelete
         public User DeletedAccount { get; set; }
 
         /// <summary>
-        /// The User(admin) key that executed the delete action.
+        /// The User key that executed the delete action.
         /// </summary>
-        public int DeletedByKey { get; set; }
+        /// <remarks>
+        /// <c>null</c> if the user was deleted.
+        /// </remarks>
+        public int? DeletedByKey { get; set; }
 
         /// <summary>
-        /// The User(admin) that executed the delete action.
+        /// The User that executed the delete action.
         /// </summary>
+        /// <remarks>
+        /// <c>null</c> if the user was deleted.
+        /// </remarks>
         public User DeletedBy { get; set; }
 
          /// <summary>

src/NuGet.Services.Entities/PackageDelete.cs

@@ -20,9 +20,15 @@ public PackageDelete()
         [Required]
         public DateTime DeletedOn { get; set; }
 
-        [Required]
+        /// <remarks>
+        /// <c>null</c> if the user was deleted.
+        /// </remarks>
         public User DeletedBy { get; set; }
-        public int DeletedByKey { get; set; }
+
+        /// <remarks>
+        /// <c>null</c> if the user was deleted.
+        /// </remarks>
+        public int? DeletedByKey { get; set; }
 
         [Required]
         public string Reason { get; set; }

src/NuGetGallery.Core/Entities/EntitiesContext.cs

@@ -356,7 +356,7 @@ protected override void OnModelCreating(DbModelBuilder modelBuilder)
                 .HasRequired(a => a.DeletedAccount);
 
             modelBuilder.Entity<AccountDelete>()
-                .HasRequired(a => a.DeletedBy)
+                .HasOptional(a => a.DeletedBy)
                 .WithMany()
                 .WillCascadeOnDelete(false);
 

src/NuGetGallery/Migrations/201904252152567_DeletedByOptional.Designer.cs

@@ -0,0 +1,32 @@
+namespace NuGetGallery.Migrations
+{
+    using System;
+    using System.Data.Entity.Migrations;
+    
+    public partial class DeletedByOptional : DbMigration
+    {
+        public override void Up()
+        {
+            DropForeignKey("dbo.PackageDeletes", "DeletedByKey", "dbo.Users");
+            DropIndex("dbo.PackageDeletes", new[] { "DeletedByKey" });
+            DropIndex("dbo.AccountDeletes", new[] { "DeletedByKey" });
+            AlterColumn("dbo.PackageDeletes", "DeletedByKey", c => c.Int());
+            AlterColumn("dbo.AccountDeletes", "DeletedByKey", c => c.Int());
+            CreateIndex("dbo.PackageDeletes", "DeletedByKey");
+            CreateIndex("dbo.AccountDeletes", "DeletedByKey");
+            AddForeignKey("dbo.PackageDeletes", "DeletedByKey", "dbo.Users", "Key");
+        }
+        
+        public override void Down()
+        {
+            DropForeignKey("dbo.PackageDeletes", "DeletedByKey", "dbo.Users");
+            DropIndex("dbo.AccountDeletes", new[] { "DeletedByKey" });
+            DropIndex("dbo.PackageDeletes", new[] { "DeletedByKey" });
+            AlterColumn("dbo.AccountDeletes", "DeletedByKey", c => c.Int(nullable: false));
+            AlterColumn("dbo.PackageDeletes", "DeletedByKey", c => c.Int(nullable: false));
+            CreateIndex("dbo.AccountDeletes", "DeletedByKey");
+            CreateIndex("dbo.PackageDeletes", "DeletedByKey");
+            AddForeignKey("dbo.PackageDeletes", "DeletedByKey", "dbo.Users", "Key", cascadeDelete: true);
+        }
+    }
+}

src/NuGetGallery/Migrations/201904252152567_DeletedByOptional.cs

@@ -238,6 +238,10 @@
     <Compile Include="Migrations\201904081230004_AddIndexToUserSecurityPolicy.Designer.cs">
       <DependentUpon>201904081230004_AddIndexToUserSecurityPolicy.cs</DependentUpon>
     </Compile>
+    <Compile Include="Migrations\201904252152567_DeletedByOptional.cs" />
+    <Compile Include="Migrations\201904252152567_DeletedByOptional.Designer.cs">
+      <DependentUpon>201904252152567_DeletedByOptional.cs</DependentUpon>
+    </Compile>
     <Compile Include="Queries\AutocompleteCveIdQueryResults.cs" />
     <Compile Include="Queries\AutocompleteCweIdQueryResults.cs" />
     <Compile Include="Queries\AutocompleteCveIdQueryResult.cs" />
@@ -1722,6 +1726,9 @@
     <EmbeddedResource Include="Migrations\201904081230004_AddIndexToUserSecurityPolicy.resx">
       <DependentUpon>201904081230004_AddIndexToUserSecurityPolicy.cs</DependentUpon>
     </EmbeddedResource>
+    <EmbeddedResource Include="Migrations\201904252152567_DeletedByOptional.resx">
+      <DependentUpon>201904252152567_DeletedByOptional.cs</DependentUpon>
+    </EmbeddedResource>
     <EmbeddedResource Include="OData\QueryAllowed\Data\apiv1packages.json" />
     <EmbeddedResource Include="OData\QueryAllowed\Data\apiv1search.json" />
     <EmbeddedResource Include="OData\QueryAllowed\Data\apiv2getupdates.json" />

src/NuGetGallery/Migrations/201904252152567_DeletedByOptional.resx

@@ -19,6 +19,7 @@ namespace NuGetGallery
     public class DeleteAccountService : IDeleteAccountService
     {
         private readonly IEntityRepository<AccountDelete> _accountDeleteRepository;
+        private readonly IEntityRepository<PackageDelete> _packageDeleteRepository;
         private readonly IEntitiesContext _entitiesContext;
         private readonly IPackageService _packageService;
         private readonly IPackageOwnershipManagementService _packageOwnershipManagementService;
@@ -35,6 +36,7 @@ public class DeleteAccountService : IDeleteAccountService
 
         public DeleteAccountService(
             IEntityRepository<AccountDelete> accountDeleteRepository,
+            IEntityRepository<PackageDelete> packageDeleteRepository,
             IEntityRepository<PackageDeprecation> deprecationRepository,
             IEntityRepository<User> userRepository,
             IEntityRepository<Scope> scopeRepository,
@@ -50,6 +52,7 @@ public DeleteAccountService(
             ITelemetryService telemetryService)
         {
             _accountDeleteRepository = accountDeleteRepository ?? throw new ArgumentNullException(nameof(accountDeleteRepository));
+            _packageDeleteRepository = packageDeleteRepository ?? throw new ArgumentNullException(nameof(packageDeleteRepository));
             _deprecationRepository = deprecationRepository ?? throw new ArgumentNullException(nameof(deprecationRepository));
             _userRepository = userRepository ?? throw new ArgumentNullException(nameof(userRepository));
             _scopeRepository = scopeRepository ?? throw new ArgumentNullException(nameof(scopeRepository));
@@ -111,6 +114,7 @@ private async Task DeleteAccountImplAsync(User userToBeDeleted, User userToExecu
             await RemoveSecurityPolicies(userToBeDeleted);
             await RemoveUserCredentials(userToBeDeleted);
             await RemovePackageOwnershipRequests(userToBeDeleted);
+            ResetPackagesAndAccountsDeletedBy(userToBeDeleted);
 
             RemovePackagePushedBy(userToBeDeleted);
             RemovePackageDeprecatedBy(userToBeDeleted);
@@ -318,6 +322,25 @@ private void RemoveMembers(Organization organization)
             }
         }
 
+        private void ResetPackagesAndAccountsDeletedBy(User user)
+        {
+            foreach (var deletedPackage in _packageDeleteRepository
+                .GetAll()
+                .Where(d => d.DeletedByKey == user.Key)
+                .ToList())
+            {
+                deletedPackage.DeletedBy = null;
+            }
+
+            foreach (var deletedAccount in _accountDeleteRepository
+                .GetAll()
+                .Where(d => d.DeletedByKey == user.Key)
+                .ToList())
+            {
+                deletedAccount.DeletedBy = null;
+            }
+        }
+
         private void RemoveUserDataInUserTable(User user)
         {
             user.SetAccountAsDeleted();

src/NuGetGallery/NuGetGallery.csproj

@@ -141,6 +141,8 @@ await deleteAccountService.DeleteAccountAsync(
                     Assert.NotEmpty(testUser.OrganizationMigrationRequests);
                     Assert.NotEmpty(testUser.OrganizationRequests);
                     Assert.NotEmpty(testUser.Organizations);
+                    Assert.NotNull(testableService.PackageDeletedByUser.DeletedBy);
+                    Assert.NotNull(testableService.AccountDeletedByUser.DeletedBy);
                 }
                 else
                 {
@@ -159,6 +161,8 @@ await deleteAccountService.DeleteAccountAsync(
                     Assert.Null(testUser.OrganizationMigrationRequest);
                     Assert.Empty(testUser.OrganizationMigrationRequests);
                     Assert.Empty(testUser.OrganizationRequests);
+                    Assert.Null(testableService.PackageDeletedByUser.DeletedBy);
+                    Assert.Null(testableService.AccountDeletedByUser.DeletedBy);
 
                     Assert.Empty(testUser.Organizations);
                     foreach (var testUserOrganization in testUserOrganizations)
@@ -185,6 +189,9 @@ await deleteAccountService.DeleteAccountAsync(
                 // In production, they would not be deleted because the transaction they were deleted in would fail.
                 Assert.Single(testableService.SupportRequests);
                 Assert.Empty(testUser.ReservedNamespaces);
+
+                Assert.NotNull(testableService.PackageDeletedByDifferentUser.DeletedBy);
+                Assert.NotNull(testableService.AccountDeletedByDifferentUser.DeletedBy);
             }
 
             [Theory]
@@ -270,6 +277,8 @@ public async Task DeleteOrganization(bool isPackageOrphaned, AccountDeletionOrph
                     Assert.Empty(testableService.AuditService.Records);
                     Assert.False(testableService.HasDeletedOwnerScope);
                     Assert.Empty(testableService.AuditService.Records);
+                    Assert.NotNull(testableService.PackageDeletedByUser.DeletedBy);
+                    Assert.NotNull(testableService.AccountDeletedByUser.DeletedBy);
                 }
                 else
                 {
@@ -286,6 +295,8 @@ public async Task DeleteOrganization(bool isPackageOrphaned, AccountDeletionOrph
                     Assert.Empty(testableService.PackageOwnerRequests);
                     Assert.Single(testableService.AuditService.Records);
                     Assert.True(testableService.HasDeletedOwnerScope);
+                    Assert.Null(testableService.PackageDeletedByUser.DeletedBy);
+                    Assert.Null(testableService.AccountDeletedByUser.DeletedBy);
 
                     var deleteRecord = testableService.AuditService.Records[0] as DeleteAccountAuditRecord;
                     Assert.True(deleteRecord != null);
@@ -296,6 +307,9 @@ public async Task DeleteOrganization(bool isPackageOrphaned, AccountDeletionOrph
                 // In production, they would not be deleted because the transaction they were deleted in would fail.
                 Assert.Empty(organization.ReservedNamespaces);
                 Assert.Single(testableService.SupportRequests);
+
+                Assert.NotNull(testableService.PackageDeletedByDifferentUser.DeletedBy);
+                Assert.NotNull(testableService.AccountDeletedByDifferentUser.DeletedBy);
             }
 
             [Theory]
@@ -473,7 +487,7 @@ public class DeleteAccountTestService
             private PackageRegistration _userPackagesRegistration = null;
             private ICollection<Package> _userPackages;
             private bool _hasDeletedCredentialWithOwnerScope = false;
-            
+
             public List<AccountDelete> DeletedAccounts = new List<AccountDelete>();
             public List<User> DeletedUsers = new List<User>();
             public List<Issue> SupportRequests = new List<Issue>();
@@ -483,6 +497,12 @@ public class DeleteAccountTestService
             public FakeAuditingService AuditService = new FakeAuditingService();
             public bool HasDeletedOwnerScope => _hasDeletedCredentialWithOwnerScope;
 
+            public AccountDelete AccountDeletedByUser { get; }
+            public AccountDelete AccountDeletedByDifferentUser { get; }
+            public PackageDelete PackageDeletedByUser { get; }
+            public PackageDelete PackageDeletedByDifferentUser { get; }
+
+
             public DeleteAccountTestService(User user)
             {
                 _user = user;
@@ -533,6 +553,11 @@ public DeleteAccountTestService(User user, PackageRegistration userPackagesRegis
                     NewOwner = _user
                 });
 
+                AccountDeletedByUser = new AccountDelete { DeletedBy = _user, DeletedByKey = _user.Key };
+                AccountDeletedByDifferentUser = new AccountDelete { DeletedBy = new User { Key = 1111 }, DeletedByKey = 1111 };
+                PackageDeletedByUser = new PackageDelete { DeletedBy = _user, DeletedByKey = _user.Key };
+                PackageDeletedByDifferentUser = new PackageDelete { DeletedBy = new User { Key = 1111 }, DeletedByKey = 1111 };
+
                 PackagePushedByUser = new Package
                 {
                     User = _user,
@@ -554,6 +579,7 @@ public DeleteAccountService GetDeleteAccountService(bool isPackageOrphaned, bool
             {
                 return new DeleteAccountService(
                     SetupAccountDeleteRepository().Object,
+                    SetupPackageDeleteRepository().Object,
                     SetupDeprecationRepository().Object,
                     SetupUserRepository().Object,
                     SetupScopeRepository().Object,
@@ -639,13 +665,35 @@ private Mock<ISecurityPolicyService> SetupSecurityPolicyService()
             private Mock<IEntityRepository<AccountDelete>> SetupAccountDeleteRepository()
             {
                 var accountDeleteRepository = new Mock<IEntityRepository<AccountDelete>>();
+
+                if (AccountDeletedByUser != null)
+                {
+                    accountDeleteRepository
+                        .Setup(m => m.GetAll())
+                        .Returns(new[] { AccountDeletedByUser, AccountDeletedByDifferentUser }.AsQueryable());
+                }
+
                 accountDeleteRepository
                     .Setup(m => m.InsertOnCommit(It.IsAny<AccountDelete>()))
                     .Callback<AccountDelete>(account => DeletedAccounts.Add(account));
 
                 return accountDeleteRepository;
             }
 
+            private Mock<IEntityRepository<PackageDelete>> SetupPackageDeleteRepository()
+            {
+                var packageDeleteRepository = new Mock<IEntityRepository<PackageDelete>>();
+
+                if (PackageDeletedByUser != null)
+                {
+                    packageDeleteRepository
+                        .Setup(m => m.GetAll())
+                        .Returns(new[] { PackageDeletedByUser, PackageDeletedByDifferentUser }.AsQueryable());
+                }
+
+                return packageDeleteRepository;
+            }
+          
             private Mock<IEntityRepository<PackageDeprecation>> SetupDeprecationRepository()
             {
                 var deprecationRepository = new Mock<IEntityRepository<PackageDeprecation>>();