Use PackagePermissionsService to determine actions a user can perform on a package #4923

scottbommarito · 2017-10-27T18:08:08Z

This replaces ExtensionMethods.IsOwner.

I went through all previous calls of IsOwner and replaced them with a HasPermissions call with the right Permission. I also added checks to _UserPackagesList.cshtml to only show actions that the user can perform.

joelverhagen · 2017-10-27T18:19:56Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+{
+    public static class PackagePermissionsService
+    {
+        public enum PermissionLevel


I don't think should not be nested. It makes the call sites too verbose.

joelverhagen · 2017-10-27T18:20:31Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+
+                    yield return Permission.ReportMyPackage;
+
+                    goto case PermissionLevel.SiteAdmin;


nit: can we write this without goto?

yes, but it will be significantly more verbose

verbose is better IMO for something so critical as permissions.

Which is why I made the tests very explicit. If someone wants to change the permissions they will need to change the tests in the exact same way.

Implementation should be as clear or clearer than tests. People trying to understand our code should not have to dig through unit tests for comprehension. Alternative is to have docs concerning this matrix of permissions.

please no goto in code

In reply to: 147482420 [](ancestors = 147482420)

can we just drop the goto and let it fall through? it looks like that is bascially what is happening here anyway?

C# does not allow falling through, which is why this exists as is

regardless, I rewrote it to be explicit, as @joelverhagen requested

joelverhagen · 2017-10-27T18:25:17Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+    {
+        public enum PermissionLevel
+        {
+            None,


Perhaps this could be call Anonymous -- this enum almost role

IMO None makes more sense because they have "no" permissions with the package

Thought about it more and I agree that Anonymous is better

joelverhagen · 2017-10-27T18:26:24Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+                case PermissionLevel.OrganizationCollaborator:
+
+                    yield return Permission.DisplayMyPackage;
+                    yield return Permission.Upload;


How is the notion of UploadNewVersion captured? Is that out of scope for this particular PR?

I think this should be renamed to UploadNewVersion. These permissions only affect existing packages.

Aren't Upload and UploadNewVersion different permissions tho?

Yes, but UploadNewId won't come into play until the user can choose the owner of the new package. These permissions are "I have this existing package, what can I do to it."

E.g. out of scope for this PR

joelverhagen · 2017-10-27T18:27:02Z

tests/NuGetGallery.Facts/Services/PackagePermissionsServiceFacts.cs

+                        new []
+                        {
+                            PackagePermissionsService.Permission.DisplayMyPackage,
+                            PackagePermissionsService.Permission.Upload,


This is new for SiteAdmin right?

Yes, good catch.

I thought it makes sense, but I'm fine removing the permission

joelverhagen · 2017-10-27T18:27:27Z

tests/NuGetGallery.Facts/Services/PackagePermissionsServiceFacts.cs

+                            PackagePermissionsService.Permission.DisplayMyPackage,
+                            PackagePermissionsService.Permission.Upload,
+                            PackagePermissionsService.Permission.Edit,
+                            PackagePermissionsService.Permission.Delete


super nit: trailing , so that adding new permissions only effects on line (easier to read diff)

joelverhagen

⌚️

skofman1 · 2017-10-27T19:05:38Z

src/NuGetGallery/Controllers/PackagesController.cs

@@ -392,7 +392,7 @@ public virtual JsonResult UploadPackageProgress()
            if (package == null
                || ((package.PackageStatusKey == PackageStatus.Validating
                     || package.PackageStatusKey == PackageStatus.FailedValidation)
-                    && !package.IsOwnerOrAdmin(User)))
+                    && !PackagePermissionsService.HasPermission(package, User, Permission.DisplayMyPackage)))


DisplayMyPackage isn't the best name here. Consider: DisplayHiddenPackage/DisplayPrivatePackage

using DisplayPrivatePackage

skofman1 · 2017-10-27T19:11:35Z

src/NuGetGallery/Controllers/PackagesController.cs

@@ -1136,7 +1136,7 @@ public virtual Task<ActionResult> RejectPendingOwnershipRequest(string id, strin
            }

            var user = GetCurrentUser();
-            if (package.IsOwner(user))
+            if (package.Owners.Any(owner => owner.Key == user.Key))


if (package.Owners.Any(owner => owner.Key == user.Key)) [](start = 10, length = 57)

This should be handled by the PackagePermissionsService . We will need to add Org logic here soon, so it doesn't make sense to have to logic here

correct, didn't notice this

upon looking over this case, this case shouldn't be handled by the PackagePermissionsService, because this is for adding a user as a co-owner

I didn't review the remainder of the code and did find some cases that used Owners.Any directly that should be using PackagePermissionsService however

upon looking over this case, this case shouldn't be handled by the PackagePermissionsService, because this is for adding a user as a co-owner - we are handling co-ownership for packages, why shouldnt this be handled by PackagePermissionsService?

yea i mean it didn't use HasPermission, but it uses GetPermissionLevels which used to be private

skofman1 · 2017-10-27T19:12:22Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+        OrganizationCollaborator
+    }
+
+    public enum Permission


This is not Permission, this is Action

skofman1

🕐

ryuyu

We should probably also use this to hide the Manage Package Owners button in the Manage Packages page for packages that users are collaborators on.
I don't think those use the _ListPackage.cshtml

joelverhagen · 2017-10-27T21:11:01Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+    public static class PackagePermissionsService
+    {
+        private static readonly IDictionary<PermissionLevel, IEnumerable<Action>> _allowedActions = 
+            new Dictionary<PermissionLevel, IEnumerable<Action>>


🥇 this is beautiful

joelverhagen · 2017-10-27T21:12:11Z

tests/NuGetGallery.Facts/Services/PackagePermissionsServiceFacts.cs

+            private int _key = 0;
+
+            [Flags]
+            public enum ReturnsExpectedPermissionLevels_State


scottbommarito · 2017-10-27T21:13:38Z

@ryuyu Manage Packages uses _UserPackagesList, which is handled by these changes

skofman1 · 2017-10-27T21:26:34Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+    public enum PackageAction
+    {
+        DisplayPrivatePackage,
+        UploadNewVersion,


UploadNewVersion is different then uploading a new package id?

yes, these are all actions on an existing package or id

UploadNewId is a right on an organization or a reserved namespace, not an existing package.

I don't see an "UploadNewId" Action here

It will come in a future PR

skofman1 · 2017-10-27T21:27:25Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+        DisplayPrivatePackage,
+        UploadNewVersion,
+        Edit,
+        Delete,


Delete [](start = 8, length = 6)

Delete is actually Unlist?

Yes...the UI and API endpoints are named Delete. I can add comments

IMO it's better to rename to Unlist, cause an SiteAdmin can actually do a Delete.

chenriksson · 2017-10-27T21:32:15Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+    }
+
+    public enum PackageAction
+    {


PermissionLevel and PackageAction should be extracted into separate files.

chenriksson · 2017-10-27T21:41:20Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+            return GetPermissionLevels(
+                owners, 
+                principal.IsAdministrator(), 
+                u => UserMatchesPrincipal(u, principal));


nit: why not just pass principal to the core GetPrincipalLevels. One less arg, and no need for allocating the lambda.

Not sure what you mean exactly...are you suggesting UserMatchesPrincipal and UserMatchesUser should return a Func<User, bool>?

chenriksson · 2017-10-27T21:45:10Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+                {
+                    PermissionLevel.Anonymous,
+                    new PackageAction[0]
+                },


nit: why include anonymous? HasPermissions could just check, and return false immediately w/o linq.

not possible because I wanted to separate the allowed actions from the permission levels

correction: if we ever decide Anonymous users can do actions, such as ReportAbuse, they would need state here

Just saying if anonymous has no actions, no need for the extra allocations... in case this is on the fast path. For instance:

private static bool IsActionAllowed(IEnumerable<PermissionLevel> permissionLevels, PackageAction action) { return permissionLevels != null && permissionLevels.Any(permissionLevel => _allowedActions[permissionLevel].Contains(action)); } public static IEnumerable<PermissionLevel> GetPermissionLevels(IEnumerable<User> owners, IPrincipal principal) { if (principal == null) { return null; } ...

Ah, well I think I prefer it like this because
1 - it's easier to extend and add actions to anonymous if we decide we need to
2 - it matches the structure of the rest of the permission levels

chenriksson · 2017-10-27T21:47:22Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+        ReportMyPackage,
+    }
+
+    public static class PackagePermissionsService


nit: Is a static class really a service? Wondering if this class should be shortened to just PackagePermissions?

Also, 'Permissions' is redundant. Maybe 'PackagePermissionsService.HasPermissions(...)' could change to 'PackagePermissions.ActionAllowed(...)' or something.

AFAIK it fits all the definitions of being a service, despite being a static class.

Will change to ActionAllowed

chenriksson · 2017-10-27T21:49:43Z

src/NuGetGallery/Services/PackagePermissionsService.cs

+            return permissionLevels.Any(permissionLevel => _allowedActions[permissionLevel].Contains(action));
+        }
+
+        public static IEnumerable<PermissionLevel> GetPermissionLevels(Package package, User user)


The GetPermissionLevels APIs seem like they should be private. The only use outside this class is a check if the user is the owner. Maybe you instead expose a single IsOwner API?

I like them being public, because I think there's a pretty high likelihood we will use them to check other states besides IsOwner in the future.

Just a suggestion, but I think it opens up complexity to the callers that this service was intended for.

skofman1

scottbommarito added 7 commits October 26, 2017 15:28

refactor isowner to support organization admins and collaborators

00819d9

listpackageitemviewmodel isowner fixes

3730dbb

fix rebase

8e253d9

add listpackageitemviewmodel.isowner to tests

dab7081

remove excess whitespace

ca695f8

packagepermissionsservice

e919294

quick fixes

144c4b1

scottbommarito requested review from joelverhagen, agr, xavierdecoster, shishirx34, ryuyu, chenriksson, cristinamanum and skofman1 October 27, 2017 18:08

dnfclas added the cla-already-signed label Oct 27, 2017

This was referenced Oct 27, 2017

[Organizations] Organization members can view the Manage Package Owners page and admins can add other owners #4889

Closed

[Organizations] ExtensionMethods.IsOwner should check for the user's membership in an organization that owns the package #4886

Closed

joelverhagen reviewed Oct 27, 2017

View reviewed changes

unnest permissionlevel and permission

8597c97

joelverhagen reviewed Oct 27, 2017

View reviewed changes

more unnesting

2fbebed

joelverhagen reviewed Oct 27, 2017

View reviewed changes

joelverhagen requested changes Oct 27, 2017

View reviewed changes

upload -> uploadnewversion

12fd99f

skofman1 reviewed Oct 27, 2017

View reviewed changes

skofman1 requested changes Oct 27, 2017

View reviewed changes

ryuyu requested changes Oct 27, 2017

View reviewed changes

scottbommarito added 2 commits October 27, 2017 14:09

getpermissionlevels

ee504cc

action -> packageaction

3532d78

joelverhagen reviewed Oct 27, 2017

View reviewed changes

joelverhagen approved these changes Oct 27, 2017

View reviewed changes

remove unnecessary nugetgallery.

b1fcb97

fix tests that used username and not key

30b9280

skofman1 reviewed Oct 27, 2017

View reviewed changes

ryuyu approved these changes Oct 27, 2017

View reviewed changes

scottbommarito added 2 commits October 27, 2017 14:36

comments on permissionlevels and packageactions

52ee33c

rename packageaction.delete -> unlist

ea6ddbc

chenriksson reviewed Oct 27, 2017

View reviewed changes

scottbommarito added 3 commits October 27, 2017 15:14

move permissionlevel and packageaction to separate class

405ea60

isActionAllowed instead

5ee866b

Merge branch 'dev' into sb-isowner

231d7cc

skofman1 approved these changes Oct 27, 2017

View reviewed changes

scottbommarito merged commit e34a93e into dev Oct 27, 2017

scottbommarito deleted the sb-isowner branch October 27, 2017 22:52

This was referenced Oct 30, 2017

Deployment 2017-11-02 #4925

Closed

[ReleasePrep][2017.10.30]RI of dev into master #4928

Merged


		yield return Permission.ReportMyPackage;

		goto case PermissionLevel.SiteAdmin;

Use PackagePermissionsService to determine actions a user can perform on a package #4923

Use PackagePermissionsService to determine actions a user can perform on a package #4923

Conversation

scottbommarito commented Oct 27, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelverhagen left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skofman1 left a comment

Choose a reason for hiding this comment

ryuyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito commented Oct 27, 2017 • edited Loading

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

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

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

skofman1 left a comment

Choose a reason for hiding this comment

scottbommarito commented Oct 27, 2017 •

edited

Loading