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

scottbommarito · 2018-05-22T17:15:18Z

1 - Organization A owns package B that is in reserved namespace C
2 - User D that is admin of A removes A from ownership of B

Expected:
Organization A is no longer an owner of B and B loses its verification

Actual:
D gets an HTTP 500.
Organization A remains an owner of B.

This PR fixes that behavior.

Also made some general fixes to prevent issues like these in the future.
E.g. we now use ActionsRequiringPermissions to determine reserved namespace package verification.

chenriksson · 2018-05-22T17:20:12Z

src/NuGetGallery/Services/ActionRequiringReservedNamespacePermissions.cs

@@ -15,7 +15,7 @@ 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>


nit: wrap long lines; makes it easier to review

chenriksson · 2018-05-22T17:21:58Z

src/NuGetGallery/Services/ActionRequiringReservedNamespacePermissions.cs

+
+        private IReadOnlyCollection<ReservedNamespace> GetReservedNamespaces(ReservedNamespace reservedNamespace)
+        {
+            return new[] { reservedNamespace };


Is there another way around this? Would prefer to avoid unnecessary allocations.

no, this system expects an array of reserved namespaces so we have to convert a single namespace into an array

chenriksson · 2018-05-22T17:25:54Z

src/NuGetGallery/Services/ActionsRequiringPermissions.cs

+        /// </summary>
+        public static ActionRequiringReservedNamespacePermissions ShowPackageAsVerifiedByReservedNamespace =
+            new ActionRequiringReservedNamespacePermissions(
+                accountOnBehalfOfPermissionsRequirement: PermissionsRequirement.Owner,


At first glance, this readonly permission seemed more restrictive than the Remove permission below... but seems like that's because we only invoke this on package owners. Maybe it's worth calling this out in the comments?

To clarify, this action is

If I own a package, I can show that package as verified by a reserved namespace that applies to it.

The reserved namespace must be owned by you and exactly you (you cannot show your package as verified on behalf of another user that owns a reserved namespace that applies to it), so the permissions requirements here are both Owner. It's a little convoluted but by using ActionsRequiringPermissions for this purpose we shouldn't have issues similar to what we saw here in the future.

chenriksson · 2018-05-22T17:28:15Z

src/NuGetGallery/Services/PackageOwnershipManagementService.cs

                        transaction.Commit();
                    }
                }
                else
                {
-                    await RemovePackageOwnerImplAsync(packageRegistration, requestingOwner, ownerToBeRemoved);


We verify that requestingOwner has permissions for ownerToBeRemoved (organization) higher in the stack?

Yes, we call OwnerHasPermissionsToRemove earlier which does the verification.

shishirx34 · 2018-05-22T18:17:41Z

src/NuGetGallery/Services/PackageOwnershipManagementService.cs

-        {
-            return packageRegistration.ReservedNamespaces.Any(rn => rn.Owners.Any(owner => owner == user));
+            // If multiple package owners own a reserved namespace that the package is a member of, then removing an owner will not remove the package from all of its reserved namespaces.
+            // If no package owners own a reserved namespace that the package is a member of, then the package is not in a reserved namespace and the owner can be removed.


This comment is confusing, essentially if there are no reserved namespaces associated with a package, then the owner can be removed would be the simplest way to state it.

The comment is trying to explain how the number of package owners owning a reserving namespace is related to the package being in a reserved namespace...I think your comment removes this relationship. I'm not sure how to make the connection and also not have a confusing comment here.

shishirx34 · 2018-05-22T18:20:50Z

src/NuGetGallery/Services/PackageOwnershipManagementService.cs

-            return packageRegistration.ReservedNamespaces.Any(rn => rn.Owners.Any(owner => owner == user));
+            // If multiple package owners own a reserved namespace that the package is a member of, then removing an owner will not remove the package from all of its reserved namespaces.
+            // If no package owners own a reserved namespace that the package is a member of, then the package is not in a reserved namespace and the owner can be removed.
+            return packageOwnersThatOwnAReservedNamespaceThatThePackageIsAMemberOf != 1;


I think this method is missing a case, you aren't checking the namespace ownership of ownerToBeRemoved, if the requestingOwner doesn't own the namespace, and neither does the ownerToBeRemoved, and there two other owners who do own the namespace, this method will return false; which it shouldn't.

keep it simple like the previous way, and check for the ownerships(of course with OnBehalfOf) of both owners to derive logical conclusion on permission for removal.

Totally correct. The reason why this wasn't broken in my unit tests was because of the != (instead of a ==) in line 203. I have fixed and simplified the logic.

shishirx34

I think there are some missing checks for owner removal.

shishirx34 · 2018-05-22T20:56:45Z

src/NuGetGallery/Services/ActionsRequiringPermissions.cs

+        /// <summary>
+        /// The action of showing a package as verified by a reserved namespace.
+        /// </summary>
+        public static ActionRequiringReservedNamespacePermissions ShowPackageAsVerifiedByReservedNamespace =


I think this is a misnomer for the action. Rather rename it as IsOwnerOfReservedNamespace or a better action that represents ownership of namespace than the ShowPackageAsVerified, its not really the application of this action in the code below.

Scott Bommarito added 2 commits May 21, 2018 15:32

use permissions service for some reserved namespace functionality

4e9aa88

add unit test for removing self as org admin

3cc097d

scottbommarito requested review from joelverhagen, agr, loic-sharma, xavierdecoster, shishirx34, nugetlurker, ryuyu, chenriksson, cristinamanum and skofman1 May 22, 2018 17:15

chenriksson approved these changes May 22, 2018

View reviewed changes

shishirx34 reviewed May 22, 2018

View reviewed changes

shishirx34 suggested changes May 22, 2018

View reviewed changes

fix bug

278c97f

shishirx34 reviewed May 22, 2018

View reviewed changes

shishirx34 approved these changes May 22, 2018

View reviewed changes

Scott Bommarito added 2 commits May 23, 2018 09:40

Merge branch 'dev' into sb-permservrn

4431916

quick pr fixes

c888155

scottbommarito merged commit 62a2866 into dev May 23, 2018

scottbommarito deleted the sb-permservrn branch May 23, 2018 19:24

cristinamanum mentioned this pull request Jun 8, 2018

Package remained verified after the organization owning the namespace was removed as owner of the package #6039

Closed

scottbommarito restored the sb-permservrn branch July 22, 2019 23:18

scottbommarito deleted the sb-permservrn branch July 22, 2019 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

scottbommarito commented May 22, 2018

chenriksson May 22, 2018

chenriksson May 22, 2018

scottbommarito May 22, 2018

chenriksson May 22, 2018

scottbommarito May 22, 2018

chenriksson May 22, 2018

scottbommarito May 22, 2018

shishirx34 May 22, 2018

scottbommarito May 22, 2018

shishirx34 May 22, 2018 •

edited

Loading

shishirx34 May 22, 2018

scottbommarito May 22, 2018

shishirx34 left a comment

shishirx34 May 22, 2018

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

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

Conversation

scottbommarito commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shishirx34 May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shishirx34 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shishirx34 May 22, 2018 •

edited

Loading