Improve the logic for how we determine whether or not packages will be orphaned by an account deletion. (#6714)

Author: Scott Bommarito

src/NuGetGallery/Areas/Admin/ViewModels/DeleteAccountAsAdminViewModel.cs

@@ -15,7 +15,7 @@ public DeleteAccountAsAdminViewModel()
         public DeleteAccountAsAdminViewModel(IDeleteAccountViewModel model)
         {
             AccountName = model.AccountName;
-            HasOrphanPackages = model.HasOrphanPackages;
+            HasPackagesThatWillBeOrphaned = model.HasPackagesThatWillBeOrphaned;
         }
 
         public string AccountName { get; set; }
@@ -28,6 +28,6 @@ public DeleteAccountAsAdminViewModel(IDeleteAccountViewModel model)
         [Display(Name = "Unlist any orphaned packages.")]
         public bool ShouldUnlist { get; set; }
 
-        public bool HasOrphanPackages { get; set; }
+        public bool HasPackagesThatWillBeOrphaned { get; set; }
     }
 }

src/NuGetGallery/Areas/Admin/Views/DeleteAccount/_DeleteUserAccountForm.cshtml

@@ -13,7 +13,7 @@
                 @Html.ValidationMessageFor(m => m.Signature)
             </div>
 
-            @if (Model.HasOrphanPackages)
+            @if (Model.HasPackagesThatWillBeOrphaned)
             {
                 <div class="form-group">
                     @Html.EditorFor(m => m.ShouldUnlist)

src/NuGetGallery/Controllers/OrganizationsController.cs

@@ -353,7 +353,7 @@ public override async Task<ActionResult> RequestAccountDeletion(string accountNa
 
             var model = GetDeleteOrganizationViewModel(account);
 
-            if (model.HasOrphanPackages)
+            if (model.HasPackagesThatWillBeOrphaned)
             {
                 TempData["ErrorMessage"] = "You cannot delete your organization unless you transfer ownership of all of its packages to another account.";
 

src/NuGetGallery/Extensions/OrganizationExtensions.cs

@@ -13,38 +13,5 @@ public static Membership GetMembershipOfUser(this Organization organization, Use
         {
             return organization?.Members?.FirstOrDefault(m => m.Member.MatchesUser(member));
         }
-
-        /// <summary>
-        /// Returns all the user accounts that are members of an organization.
-        /// If the organization has nested organizations their members will be returned as well.
-        /// The result will not have duplicate elements.
-        /// 
-        /// Nested organizations (teams) are not supported in the Gallery yet, but this method allows for it in case we lift that constraint.
-        /// </summary>
-        /// <param name="organization">The organization.</param>
-        /// <returns>The <see cref="IEnumerable{User}"/> of users that are not <see cref="Organization"/> and are members of <paramref name="organization"/>.</returns>
-        public static IEnumerable<User> GetUserAccountMembers(this Organization organization)
-        {
-            Queue<Organization> organizations = new Queue<Organization>();
-            organizations.Enqueue(organization);
-            HashSet<User> distinctUsers = new HashSet<User>();
-            while (organizations.Any())
-            {
-                var currentOrganization = organizations.Dequeue();
-                foreach (var membership in currentOrganization.Members)
-                {
-                    if (membership.Member is Organization)
-                    {
-                        organizations.Enqueue((Organization)membership.Member);
-                    }
-                    else
-                    {
-                        distinctUsers.Add(membership.Member);
-                    }
-                }
-            }
-
-            return distinctUsers;
-        }
     }
 }

src/NuGetGallery/Services/DeleteAccountService.cs

@@ -183,10 +183,8 @@ private async Task RemovePackageOwnership(User user, User requestingUser, Accoun
         {
             foreach (var package in GetPackagesOwnedByUser(user))
             {
-                var owners = user is Organization ? package.PackageRegistration.Owners : _packageService.GetPackageUserAccountOwners(package);
-                if (owners.Count() <= 1)
+                if (_packageService.WillPackageBeOrphanedIfOwnerRemoved(package.PackageRegistration, user))
                 {
-                    // Package will be orphaned by removing ownership.
                     if (orphanPackagePolicy == AccountDeletionOrphanPackagePolicy.DoNotAllowOrphans)
                     {
                         throw new InvalidOperationException($"Deleting user '{user.Username}' will make package '{package.PackageRegistration.Id}' an orphan, but no orphans were expected.");
@@ -201,12 +199,6 @@ private async Task RemovePackageOwnership(User user, User requestingUser, Accoun
             }
         }
 
-        private bool WillPackageBeOrphaned(User user, Package package)
-        {
-            var owners = user is Organization ? package.PackageRegistration.Owners : _packageService.GetPackageUserAccountOwners(package);
-            return owners.Count() <= 1;
-        }
-
         private List<Package> GetPackagesOwnedByUser(User user)
         {
             return _packageService.FindPackagesByAnyMatchingOwner(user, includeUnlisted: true, includeVersions: true).ToList();
@@ -225,22 +217,21 @@ private async Task RemoveMemberships(User user, User requestingUser, AccountDele
         {
             foreach (var membership in user.Organizations.ToArray())
             {
-                var organization = membership.Organization;
-                var members = organization.Members.ToList();
-                var collaborators = members.Where(m => !m.IsAdmin).ToList();
-                var memberCount = members.Count();
                 user.Organizations.Remove(membership);
+                var organization = membership.Organization;
+                var otherMembers = organization.Members
+                    .Where(m => !m.Member.MatchesUser(user));
 
-                if (memberCount < 2)
+                if (!otherMembers.Any())
                 {
                     // The user we are deleting is the only member of the organization.
                     // We should delete the entire organization.
                     await DeleteAccountImplAsync(organization, requestingUser, orphanPackagePolicy);
                 }
-                else if (memberCount - 1 <= collaborators.Count())
+                else if (otherMembers.All(m => !m.IsAdmin))
                 {
                     // All other members of this organization are collaborators, so we should promote them to administrators.
-                    foreach (var collaborator in collaborators)
+                    foreach (var collaborator in otherMembers)
                     {
                         collaborator.IsAdmin = true;
                     }

src/NuGetGallery/Services/IPackageService.cs

@@ -73,6 +73,11 @@ public interface IPackageService : ICorePackageService
 
         Task RemovePackageOwnerAsync(PackageRegistration package, User user, bool commitChanges = true);
 
+        /// <remarks>
+        /// A package is orphaned if it is not owned by a user account or an organization with user account members.
+        /// </remarks>
+        bool WillPackageBeOrphanedIfOwnerRemoved(PackageRegistration package, User ownerToRemove);
+
         Task SetLicenseReportVisibilityAsync(Package package, bool visible, bool commitChanges = true);
 
         Task EnsureValid(PackageArchiveReader packageArchiveReader);
@@ -81,13 +86,6 @@ public interface IPackageService : ICorePackageService
 
         Task UpdatePackageVerifiedStatusAsync(IReadOnlyCollection<PackageRegistration> package, bool isVerified, bool commitChanges = true);
 
-        /// <summary>
-        /// For a package get the list of owners that are not organizations.
-        /// </summary>
-        /// <param name="package">The package.</param>
-        /// <returns>The list of package owners that are not organizations.</returns>
-        IEnumerable<User> GetPackageUserAccountOwners(Package package);
-
         /// <summary>
         /// Sets the required signer on all owned package registrations.
         /// </summary>

src/NuGetGallery/Services/PackageService.cs

@@ -282,26 +282,6 @@ private IEnumerable<Package> GetPackagesForOwners(IEnumerable<int> ownerKeys, bo
                 .ToList();
         }
 
-        /// <summary>
-        /// For a package get the list of owners that are not organizations.
-        /// All the resulted user accounts will be distinct.
-        /// </summary>
-        /// <param name="package">The package.</param>
-        /// <returns>The list of package owners.</returns>
-        public IEnumerable<User> GetPackageUserAccountOwners(Package package)
-        {
-            return package.PackageRegistration.Owners
-                .SelectMany((owner) =>
-                {
-                    if (owner is Organization)
-                    {
-                        return OrganizationExtensions.GetUserAccountMembers((Organization)owner);
-                    }
-                    return new List<User> { owner };
-                })
-                .Distinct();
-        }
-
         public IQueryable<PackageRegistration> FindPackageRegistrationsByOwner(User user)
         {
             return _packageRegistrationRepository.GetAll().Where(p => p.Owners.Any(o => o.Key == user.Key));
@@ -377,6 +357,43 @@ public async Task RemovePackageOwnerAsync(PackageRegistration package, User user
                 await _packageRepository.CommitChangesAsync();
             }
         }
+        
+        public bool WillPackageBeOrphanedIfOwnerRemoved(PackageRegistration package, User ownerToRemove)
+        {
+            return WillPackageBeOrphanedIfOwnerRemovedHelper(package.Owners, ownerToRemove);
+        }
+
+        private bool WillPackageBeOrphanedIfOwnerRemovedHelper(IEnumerable<User> owners, User ownerToRemove)
+        {
+            // Iterate through each owner, attempting to find a user that is not the owner we are removing.
+            foreach (var owner in owners)
+            {
+                if (owner.MatchesUser(ownerToRemove))
+                {
+                    continue;
+                }
+
+                if (owner is Organization organization)
+                {
+                    // The package will still be orphaned if it is owned by an orphaned organization.
+                    // Iterate through the organization owner's members to determine if it has any members that are not the member we are removing.
+                    if (!WillPackageBeOrphanedIfOwnerRemovedHelper(
+                        organization.Members.Select(m => m.Member),
+                        ownerToRemove))
+                    {
+                        return false;
+                    }
+                }
+                else
+                {
+                    // The package will not be orphaned because it is owned by a user that is not the owner we are removing.
+                    return false;
+                }
+            }
+
+            // The package will be orphaned because we did not find an owner that is not the owner we are removing.
+            return true;
+        }
 
         public async Task MarkPackageListedAsync(Package package, bool commitChanges = true)
         {

src/NuGetGallery/ViewModels/DeleteAccountViewModel.cs

@@ -13,9 +13,8 @@ public class DeleteAccountViewModel<TAccount> : DeleteAccountViewModel where TAc
         public DeleteAccountViewModel(
             TAccount accountToDelete,
             User currentUser,
-            IPackageService packageService,
-            Func<ListPackageItemViewModel, bool> packageIsOrphaned)
-            : base(accountToDelete, currentUser, packageService, packageIsOrphaned)
+            IPackageService packageService)
+            : base(accountToDelete, currentUser, packageService)
         {
             Account = accountToDelete;
         }
@@ -25,43 +24,49 @@ public DeleteAccountViewModel(
 
     public class DeleteAccountViewModel : IDeleteAccountViewModel
     {
-        private Lazy<bool> _hasOrphanPackages;
-
         public DeleteAccountViewModel(
             User userToDelete,
             User currentUser,
-            IPackageService packageService,
-            Func<ListPackageItemViewModel, bool> packageIsOrphaned)
+            IPackageService packageService)
         {
             User = userToDelete;
 
             Packages = packageService
                  .FindPackagesByAnyMatchingOwner(User, includeUnlisted: true)
-                 .Select(p => new ListPackageItemViewModel(p, currentUser))
+                 .Select(p => new DeleteAccountListPackageItemViewModel(p, userToDelete, currentUser, packageService))
                  .ToList();
 
-            _hasOrphanPackages = new Lazy<bool>(() => Packages.Any(packageIsOrphaned));
+            HasPackagesThatWillBeOrphaned = Packages.Any(p => p.WillBeOrphaned);
         }
 
-        public List<ListPackageItemViewModel> Packages { get; }
+        public List<DeleteAccountListPackageItemViewModel> Packages { get; }
 
         public User User { get; }
 
         public string AccountName => User.Username;
 
-        public bool HasOrphanPackages
+        public bool HasPackagesThatWillBeOrphaned { get; }
+    }
+
+    public class DeleteAccountListPackageItemViewModel : ListPackageItemViewModel
+    {
+        public DeleteAccountListPackageItemViewModel(
+            Package package, 
+            User userToDelete, 
+            User currentUser, 
+            IPackageService packageService)
+            : base(package, currentUser)
         {
-            get
-            {
-                return Packages == null ? false : _hasOrphanPackages.Value;
-            }
+            WillBeOrphaned = packageService.WillPackageBeOrphanedIfOwnerRemoved(package.PackageRegistration, userToDelete);
         }
+
+        public bool WillBeOrphaned { get; }
     }
 
     public interface IDeleteAccountViewModel
     {
         string AccountName { get; }
 
-        bool HasOrphanPackages { get; }
+        bool HasPackagesThatWillBeOrphaned { get; }
     }
 }

src/NuGetGallery/ViewModels/DeleteOrganizationViewModel.cs

@@ -13,7 +13,7 @@ public DeleteOrganizationViewModel(
             Organization organizationToDelete, 
             User currentUser, 
             IPackageService packageService)
-            : base(organizationToDelete, currentUser, packageService, p => p.HasSingleOrganizationOwner)
+            : base(organizationToDelete, currentUser, packageService)
         {
             AdditionalMembers = organizationToDelete.Members
                 .Where(m => !m.Member.MatchesUser(currentUser))

src/NuGetGallery/ViewModels/DeleteUserViewModel.cs

@@ -16,7 +16,7 @@ public DeleteUserViewModel(
             User currentUser,
             IPackageService packageService,
             ISupportRequestService supportRequestService)
-            : base(userToDelete, currentUser, packageService, p => p.HasSingleUserOwner)
+            : base(userToDelete, currentUser, packageService)
         {
             Organizations = userToDelete.Organizations
                 .Select(u => new ManageOrganizationsItemViewModel(u, packageService));