Allow organization admins to remove their organization's ownership of a package in a reserved namespace

src/NuGetGallery/Services/ActionRequiringReservedNamespacePermissions.cs

@@ -15,7 +15,9 @@ namespace NuGetGallery
     /// E.g. "JQuery.Extensions.MyCoolExtension" matches both "JQuery.*" and "JQuery.Extensions.*".
     /// </remarks>
     public class ActionRequiringReservedNamespacePermissions
-        : ActionRequiringEntityPermissions<IReadOnlyCollection<ReservedNamespace>>, IActionRequiringEntityPermissions<ActionOnNewPackageContext>
+        : ActionRequiringEntityPermissions<IReadOnlyCollection<ReservedNamespace>>, 
+        IActionRequiringEntityPermissions<ActionOnNewPackageContext>, 
+        IActionRequiringEntityPermissions<ReservedNamespace>
     {
         public PermissionsRequirement ReservedNamespacePermissionsRequirement { get; }
 
@@ -32,11 +34,21 @@ public PermissionsCheckResult CheckPermissions(User currentUser, User account, A
             return CheckPermissions(currentUser, account, GetReservedNamespaces(newPackageContext));
         }
 
+        public PermissionsCheckResult CheckPermissions(User currentUser, User account, ReservedNamespace reservedNamespace)
+        {
+            return CheckPermissions(currentUser, account, GetReservedNamespaces(reservedNamespace));
+        }
+
         public PermissionsCheckResult CheckPermissions(IPrincipal currentPrincipal, User account, ActionOnNewPackageContext newPackageContext)
         {
             return CheckPermissions(currentPrincipal, account, GetReservedNamespaces(newPackageContext));
         }
 
+        public PermissionsCheckResult CheckPermissions(IPrincipal currentPrincipal, User account, ReservedNamespace reservedNamespace)
+        {
+            return CheckPermissions(currentPrincipal, account, GetReservedNamespaces(reservedNamespace));
+        }
+
         protected override PermissionsCheckResult CheckPermissionsForEntity(User account, IReadOnlyCollection<ReservedNamespace> reservedNamespaces)
         {
             if (!reservedNamespaces.Any())
@@ -54,11 +66,21 @@ public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentU
             return CheckPermissionsOnBehalfOfAnyAccount(currentUser, GetReservedNamespaces(newPackageContext));
         }
 
+        public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, ReservedNamespace reservedNamespace)
+        {
+            return CheckPermissionsOnBehalfOfAnyAccount(currentUser, GetReservedNamespaces(reservedNamespace));
+        }
+
         public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, ActionOnNewPackageContext newPackageContext, out IEnumerable<User> accountsAllowedOnBehalfOf)
         {
             return CheckPermissionsOnBehalfOfAnyAccount(currentUser, GetReservedNamespaces(newPackageContext), out accountsAllowedOnBehalfOf);
         }
 
+        public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, ReservedNamespace reservedNamespace, out IEnumerable<User> accountsAllowedOnBehalfOf)
+        {
+            return CheckPermissionsOnBehalfOfAnyAccount(currentUser, GetReservedNamespaces(reservedNamespace), out accountsAllowedOnBehalfOf);
+        }
+
         protected override IEnumerable<User> GetOwners(IReadOnlyCollection<ReservedNamespace> reservedNamespaces)
         {
             return reservedNamespaces.Any() ? reservedNamespaces.SelectMany(rn => rn.Owners) : Enumerable.Empty<User>();
@@ -68,5 +90,10 @@ private IReadOnlyCollection<ReservedNamespace> GetReservedNamespaces(ActionOnNew
         {
             return newPackageContext.ReservedNamespaceService.GetReservedNamespacesForId(newPackageContext.PackageId);
         }
+
+        private IReadOnlyCollection<ReservedNamespace> GetReservedNamespaces(ReservedNamespace reservedNamespace)
+        {
+            return new[] { reservedNamespace };
+        }
     }
 }

src/NuGetGallery/Services/ActionsRequiringPermissions.cs

@@ -128,5 +128,21 @@ public static class ActionsRequiringPermissions
              new ActionRequiringPackagePermissions(
                 accountOnBehalfOfPermissionsRequirement: RequireOwnerOrOrganizationAdmin,
                 packageRegistrationPermissionsRequirement: RequireOwnerOrOrganizationAdmin);
+
+        /// <summary>
+        /// The action of adding a package to a reserved namespace that the package is in.
+        /// </summary>
+        public static ActionRequiringReservedNamespacePermissions AddPackageToReservedNamespace =
+            new ActionRequiringReservedNamespacePermissions(
+                accountOnBehalfOfPermissionsRequirement: PermissionsRequirement.Owner,
+                reservedNamespacePermissionsRequirement: PermissionsRequirement.Owner);
+
+        /// <summary>
+        /// The action of removing a package from a reserved namespace that the package is in.
+        /// </summary>
+        public static ActionRequiringReservedNamespacePermissions RemovePackageFromReservedNamespace =
+            new ActionRequiringReservedNamespacePermissions(
+                accountOnBehalfOfPermissionsRequirement: RequireOwnerOrSiteAdminOrOrganizationAdmin,
+                reservedNamespacePermissionsRequirement: RequireOwnerOrOrganizationAdmin);
     }
 }

src/NuGetGallery/Services/IPackageService.cs

@@ -68,15 +68,15 @@ public interface IPackageService : ICorePackageService
         /// <returns>Awaitable task.</returns>
         Task AddPackageOwnerAsync(PackageRegistration package, User newOwner);
 
-        Task RemovePackageOwnerAsync(PackageRegistration package, User user);
+        Task RemovePackageOwnerAsync(PackageRegistration package, User user, bool commitChanges = true);
 
         Task SetLicenseReportVisibilityAsync(Package package, bool visible, bool commitChanges = true);
 
         void EnsureValid(PackageArchiveReader packageArchiveReader);
 
         Task IncrementDownloadCountAsync(string id, string version, bool commitChanges = true);
 
-        Task UpdatePackageVerifiedStatusAsync(IReadOnlyCollection<PackageRegistration> package, bool isVerified);
+        Task UpdatePackageVerifiedStatusAsync(IReadOnlyCollection<PackageRegistration> package, bool isVerified, bool commitChanges = true);
 
         /// <summary>
         /// For a package get the list of owners that are not organizations.

src/NuGetGallery/Services/IReservedNamespaceService.cs

@@ -57,9 +57,9 @@ public interface IReservedNamespaceService
         /// Remove the specified package registration from the reserved namespace. It is the caller's reponsibility to
         /// commit the changes to the database.
         /// </summary>
-        /// <param name="prefix">The prefix value of the reserved namespace to modify</param>
+        /// <param name="reservedNamespace">The reserved namespace to modify</param>
         /// <param name="packageRegistration">The package registration entity to be removed.</param>
-        void RemovePackageRegistrationFromNamespace(string prefix, PackageRegistration packageRegistration);
+        void RemovePackageRegistrationFromNamespace(ReservedNamespace reservedNamespace, PackageRegistration packageRegistration);
 
         /// <summary>
         /// Retrieves the first reserved namespace which matches the given prefix.

src/NuGetGallery/Services/PackageOwnershipManagementService.cs

@@ -121,13 +121,13 @@ public async Task RemovePackageOwnerAsync(PackageRegistration packageRegistratio
                     using (var strategy = new SuspendDbExecutionStrategy())
                     using (var transaction = _entitiesContext.GetDatabase().BeginTransaction())
                     {
-                        await RemovePackageOwnerImplAsync(packageRegistration, requestingOwner, ownerToBeRemoved);
+                        await RemovePackageOwnerImplAsync(packageRegistration, ownerToBeRemoved);
                         transaction.Commit();
                     }
                 }
                 else
                 {
-                    await RemovePackageOwnerImplAsync(packageRegistration, requestingOwner, ownerToBeRemoved);
+                    await RemovePackageOwnerImplAsync(packageRegistration, ownerToBeRemoved);
                 }
 
                 await _auditingService.SaveAuditRecordAsync(
@@ -139,38 +139,29 @@ await _auditingService.SaveAuditRecordAsync(
             }
         }
 
-        private async Task RemovePackageOwnerImplAsync(PackageRegistration packageRegistration, User requestingOwner, User ownerToBeRemoved)
+        private async Task RemovePackageOwnerImplAsync(PackageRegistration packageRegistration, User ownerToBeRemoved)
         {
-            // 1. Remove this package registration from the namespaces owned by this user if he is the only package owner in the set of matching namespaces
-            // 2. Remove the IsVerified flag from package registration if all the matching namespaces are owned by this user alone (no other package owner owns a matching namespace for this PR)
-            var allMatchingNamespacesForRegistration = packageRegistration.ReservedNamespaces;
-            if (allMatchingNamespacesForRegistration.Any())
+            // Remove the user from owners list of package registration
+            await _packageService.RemovePackageOwnerAsync(packageRegistration, ownerToBeRemoved, commitChanges: false);
+
+            // Remove this package registration from the namespaces owned by this user that are owned by no other package owners
+            foreach (var reservedNamespace in packageRegistration.ReservedNamespaces.ToArray())
             {
-                var allPackageOwners = packageRegistration.Owners;
-                var matchingNamespacesOwnedByUser = allMatchingNamespacesForRegistration
-                    .Where(rn => rn.Owners.Any(o => o == ownerToBeRemoved));
-                var namespacesToModify = matchingNamespacesOwnedByUser
-                    .Where(rn => rn.Owners.Intersect(allPackageOwners).Count() == 1)
-                    .ToList();
-
-                if (namespacesToModify.Any())
+                if (!packageRegistration.Owners
+                    .Any(o => ActionsRequiringPermissions.AddPackageToReservedNamespace
+                        .CheckPermissionsOnBehalfOfAnyAccount(o, reservedNamespace) == PermissionsCheckResult.Allowed))
                 {
-                    // The package will lose its 'IsVerified' flag if the user is the only package owner who owns all the namespaces that match this registration
-                    var shouldModifyIsVerified = allMatchingNamespacesForRegistration.Count() == namespacesToModify.Count();
-                    if (shouldModifyIsVerified && packageRegistration.IsVerified)
-                    {
-                        await _packageService.UpdatePackageVerifiedStatusAsync(new List<PackageRegistration> { packageRegistration }, isVerified: false);
-                    }
-
-                    namespacesToModify
-                        .ForEach(rn => _reservedNamespaceService.RemovePackageRegistrationFromNamespace(rn.Value, packageRegistration));
-
-                    await _entitiesContext.SaveChangesAsync();
+                    _reservedNamespaceService.RemovePackageRegistrationFromNamespace(reservedNamespace, packageRegistration);
                 }
             }
 
-            // Remove the user from owners list of package registration
-            await _packageService.RemovePackageOwnerAsync(packageRegistration, ownerToBeRemoved);
+            // Remove the IsVerified flag from package registration if all the matching namespaces are owned by this user alone (no other package owner owns a matching namespace for this PR)
+            if (packageRegistration.IsVerified && !packageRegistration.ReservedNamespaces.Any())
+            {
+                await _packageService.UpdatePackageVerifiedStatusAsync(new List<PackageRegistration> { packageRegistration }, isVerified: false, commitChanges: false);
+            }
+
+            await _entitiesContext.SaveChangesAsync();
         }
 
         public async Task DeletePackageOwnershipRequestAsync(PackageRegistration packageRegistration, User newOwner)
@@ -191,31 +182,21 @@ public async Task DeletePackageOwnershipRequestAsync(PackageRegistration package
                 await _packageOwnerRequestService.DeletePackageOwnershipRequest(request);
             }
         }
-
-        // The requesting owner can remove other owner only if 
-        // 1. Is an admin.
-        // 2. Owns a namespace.
-        // 3. Or the other user also does not own a namespace.
+
         private static bool OwnerHasPermissionsToRemove(User requestingOwner, User ownerToBeRemoved, PackageRegistration packageRegistration)
         {
-            if (requestingOwner.IsAdministrator)
+            var reservedNamespaces = packageRegistration.ReservedNamespaces.ToList();
+            if (ActionsRequiringPermissions.AddPackageToReservedNamespace
+                .CheckPermissionsOnBehalfOfAnyAccount(ownerToBeRemoved, reservedNamespaces) == PermissionsCheckResult.Allowed)
             {
-                return true;
+                // If the owner to be removed owns a reserved namespace that applies to this package,
+                // the requesting user must own a reserved namespace that applies to this package or be a site admin.
+                return ActionsRequiringPermissions.RemovePackageFromReservedNamespace
+                    .CheckPermissionsOnBehalfOfAnyAccount(requestingOwner, reservedNamespaces) == PermissionsCheckResult.Allowed;
             }
 
-            var requestingOwnerOwnsNamespace = IsUserAnOwnerOfPackageNamespace(packageRegistration, requestingOwner);
-            if (requestingOwnerOwnsNamespace)
-            {
-                return true;
-            }
-
-            var ownerToBeRemovedOwnsNamespace = IsUserAnOwnerOfPackageNamespace(packageRegistration, ownerToBeRemoved);
-            return !ownerToBeRemovedOwnsNamespace;
-        }
-
-        private static bool IsUserAnOwnerOfPackageNamespace(PackageRegistration packageRegistration, User user)
-        {
-            return packageRegistration.ReservedNamespaces.Any(rn => rn.Owners.Any(owner => owner == user));
+            // If the owner to be removed does not own any reserved namespaces that apply to this package, they can be removed by anyone.
+            return true;
         }
     }
 }

src/NuGetGallery/Services/PackageService.cs

@@ -361,11 +361,14 @@ public async Task AddPackageOwnerAsync(PackageRegistration package, User newOwne
             }
         }
 
-        public async Task RemovePackageOwnerAsync(PackageRegistration package, User user)
+        public async Task RemovePackageOwnerAsync(PackageRegistration package, User user, bool commitChanges = true)
         {
             // To support the delete account scenario, the admin can delete the last owner of a package.
             package.Owners.Remove(user);
-            await _packageRepository.CommitChangesAsync();
+            if (commitChanges)
+            {
+                await _packageRepository.CommitChangesAsync();
+            }
         }
 
         public async Task MarkPackageListedAsync(Package package, bool commitChanges = true)
@@ -701,7 +704,7 @@ public async Task IncrementDownloadCountAsync(string id, string version, bool co
             }
         }
 
-        public virtual async Task UpdatePackageVerifiedStatusAsync(IReadOnlyCollection<PackageRegistration> packageRegistrationList, bool isVerified)
+        public virtual async Task UpdatePackageVerifiedStatusAsync(IReadOnlyCollection<PackageRegistration> packageRegistrationList, bool isVerified, bool commitChanges = true)
         {
             var packageRegistrationIdSet = new HashSet<string>(packageRegistrationList.Select(prl => prl.Id));
             var allPackageRegistrations = _packageRegistrationRepository.GetAll();
@@ -714,7 +717,10 @@ public virtual async Task UpdatePackageVerifiedStatusAsync(IReadOnlyCollection<P
                 packageRegistrationsToUpdate
                     .ForEach(pru => pru.IsVerified = isVerified);
 
-                await _packageRegistrationRepository.CommitChangesAsync();
+                if (commitChanges)
+                {
+                    await _packageRegistrationRepository.CommitChangesAsync();
+                }
             }
         }