Integrate PermissionsService 2.0 with Gallery #5180

scottbommarito · 2017-12-13T01:01:41Z

This PR removes the old PermissionsService from the gallery and integrates the new permissions service.

chenriksson · 2017-12-13T23:23:23Z

src/NuGetGallery/Controllers/ApiController.cs

@@ -414,8 +413,7 @@ public virtual Task<ActionResult> CreatePackagePost()
                            }

                            // For a new package id verify that the user is allowed to push to the matching namespaces, if any.
-                            var isPushAllowed = ReservedNamespaceService.IsPushAllowed(id, user, out userOwnedNamespaces);
-                            if (!isPushAllowed)
+                            if (!ActionsRequiringPermissions.UploadNewPackageId.TryGetAccountsIsAllowedOnBehalfOf(user, new ActionOnNewPackageContext(id, ReservedNamespaceService), out var accountsAllowedOnBehalfOf))


Why TryGetAccountsIsAllowedOnBehalfOf when out parameter accountsAllowedOnBehalfOf isn't used? I'd expect this API to be used wherever we need to populate a drop-down of package owners for an account.

Why not ActionsRequiringPermissions.UploadNewPackageId.CheckPermissions(user, scopeOwner, packageContext)?

Two notes:
1 - I call TryGetAccountsIsAllowedOnBehalfOf whenever I want to know if an action is allowed on behalf of any account, regardless of whether or not I care about the list of users. CheckPermissions is for when the caller knows the exact account the action is attempting to be done on behalf of.
2 - The upcoming UI upload PR will fix this flow to actually work as intended with organizations--this is merely a quick syntax replacement of the old broken logic.

The TryParse pattern in the .NET framework is intended for improving performance in what would normally be an exception case. see: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance

Looking at the following API, I assume I'm trying to get a list of accounts but that the get operation might throw for some reason.

bool TryGetAccountsIsAllowedOnBehalfOf(user, packageContext, out accountsAllowed);

Whereas the following API looks like it checks if the user has permissions:

bool CheckPermissions(currentUser, owner, package);

You should reconsider the Try API, as it doesn't come across doing what it's intended for. Another sign that the pattern is misused is that the out parameter is often ignored by the callers.

Are you suggesting I rename it to

bool IsAllowedOnBehalfOfAnAccount(user, package, out accountsAllowed)

?

Ideally APIs have a strong contract for callers. If comments/docs are necessary to use the API, then it could be a bad name or poor design (i.e., doing too much).

In this case, it looks like the API was created with two intents:

Can User X do Action A on behalf of User Y?
AND

Get Users that User X can do Action A on behalf of

If that's the case, then I suggest both rename and splitting this into multiple APIs with a well defined purpose. For instance:

interface IActionRequiringEntityPermissions<TEntity> { bool IsAllowedOnBehalfOfAnyOwner(User currentUser, TEntity); bool IsAllowedOnBehalfOfOwner(User currentUser, User entityOwner, TEntity); List<TEntity> GetAccountsAllowedOnBehalfOf(User currentUser, TEntity); }

What may also be confusing me is the following public API on IActionRequiringEntityPermission:

PermissionsCheckResult CheckPermissions(User currentUser, User account, TEntity entity);

I had the impression that this API would be used by callers checking permissions, and the other was for callers that needed a list of accounts (i.e, for populating a drop-down). Is CheckPermissions intended to be used by callers?

I don't want to split IsAllowedOnBehalfOfAnAccount into two parts because
1 - the list of accounts is meaningless if the action is not allowed on behalf of any account
2 - when you want both whether or not the action is allowed on behalf of any accounts and the list of the accounts it is allowed on behalf of, splitting the method into two would require the same complex logic to be run twice, which is an unnecessary performance hit
3 - checking if an account is allowed on behalf of any account and the list of those accounts are very closely related
4 - although I agree with you that the earlier name (TryGet) was confusing, I believe the newer proposition IsAllowedOnBehalfOfAnyAccount is very clear
5 - ultimately I don't think ignoring an out parameter is too concerning, although I'd be ok creating a version of the method without the out parameter while keeping the version with it

So, what I'm thinking of

PermissionsCheckResult CheckPermissions(currentUser, accountOnBehalfOf, entity) bool IsAllowedOnBehalfOfAnyAccount(currentUser, entity) bool IsAllowedOnBehalfOfAnyAccount(currentUser, entity, out accountsAllowed)

If having the bool is too confusing with the `PermissionsCheckResult, I'm also ok with

PermissionsCheckResult CheckPermissions(currentUser, accountOnBehalfOf, entity) PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(currentUser, entity) PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(currentUser, entity, out accountsAllowed)

but that would require adding some logic to determine which PermissionsCheckResult to return when the case is not PermissionsCheckResult.Allowed (probably max int value of any enum state returned).

It sounds like CheckPermissions and IsAllowedOnBehalfOfAnyAccount are the same except:

different return value (bool vs. more granular enum)

check permissions for specific owner, vs any owner

If the purpose is so similar, why are the APIs so different? Could it just be (don't care which retval you use, just be consistent):

PermissionsCheckResult IsAllowedOnBehalfOf(currentUser, accountOnBehalfOf, entity); PermissionsCheckResult IsAllowedOnBehalfOfAny(currentUser, entity);

The API that gets the list of accounts has a different purpose - getting accounts which pass the permissions check. I expect this to always return at least 1 account (currentUser), and should only be used by callers that need the list of accounts:

List<User> GetAccountsAllowedOnBehalfOf(currentUser, entity);

I agree with your point on consistency in retval, but as mentioned above, I don't want to make a separate method for getting the list of accounts on behalf of.

I'm planning on the following API:

PermissionsCheckResult CheckPermissions(currentUser, accountOnBehalfOf, entity) PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(currentUser, entity) PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(currentUser, entity, out accountsAllowed)

I will open up a PR separate with this one with those changes and then make the changes to this branch retroactively.

chenriksson · 2017-12-13T23:29:17Z

src/NuGetGallery/Controllers/JsonApiController.cs

@@ -71,6 +72,7 @@ public virtual ActionResult GetPackageOwners(string id, string version)
                .Select(u => new PackageOwnersResultViewModel(
                    u, 
                    currentUser,
+                    packageRegistration,


Why not do the work (permissions check) in the controller and pass the result to the view model?

The constructor of PackageOwnersResultViewModel does the checks and saves the results for the view to use. I didn't want to have to copy all of the permissions checks to every place this view model is used.

chenriksson · 2017-12-13T23:30:56Z

src/NuGetGallery/Controllers/JsonApiController.cs

+                return false;
+            }
+
+            if (!ActionsRequiringPermissions.ManagePackageOwnership.TryGetAccountsIsAllowedOnBehalfOf(currentUser, package, out var accountsAllowedOnBehalfOf))


Same comment as above... wondering why TryGetAccountsIsAllowedOnBehalfOf is used when the out parameter accountsAllowedOnBehalfOf is not used. Looks like there's more callers with this pattern below too.

see response to "Why TryGetAccountsIsAllowedOnBehalfOf when out parameter accountsAllowedOnBehalfOf isn't used?" above

chenriksson · 2017-12-15T18:00:35Z

@scottbommarito Please see my comment about the TryParse pattern, which is relevant for both this PR and #5192 -- #5180 (comment)

scottbommarito · 2018-01-02T21:11:30Z

I'm fixing this PR right now, sorry for how broken it became!

scottbommarito · 2018-01-02T22:04:20Z

should be fixed now!

chenriksson · 2018-01-02T22:31:11Z

@scottbommarito Looks like a rebase is needed? I'm seeing auditing changes related to the PII work.

scottbommarito · 2018-01-02T22:37:16Z

@chenriksson woops...git is AWFUL at feature branches, will fix

scottbommarito · 2018-01-02T22:59:58Z

@chenriksson ok it's actually fixed now!

chenriksson · 2018-01-03T16:04:47Z

src/NuGetGallery/Controllers/ApiController.cs

@@ -414,8 +413,7 @@ public virtual Task<ActionResult> CreatePackagePost()
                            }

                            // For a new package id verify that the user is allowed to push to the matching namespaces, if any.
-                            var isPushAllowed = ReservedNamespaceService.IsPushAllowed(id, user, out userOwnedNamespaces);
-                            if (!isPushAllowed)
+                            if (ActionsRequiringPermissions.UploadNewPackageId.CheckPermissionsOnBehalfOfAnyAccount(user, new ActionOnNewPackageContext(id, ReservedNamespaceService)) != PermissionsCheckResult.Allowed)


nit: wrap long lines

chenriksson · 2018-01-03T16:07:32Z

src/NuGetGallery/Extensions/UserExtensions.cs

+            {
+                return self == null && user == null;
+            }
+
            return self.Key == user.Key;


nit: could simplify to self?.Key == user?.Key

chenriksson · 2018-01-03T16:10:09Z

src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs

@@ -91,7 +91,7 @@ public bool Equals(User x, User y)

            public int GetHashCode(User obj)
            {
-                return obj.Key.GetHashCode();
+                return obj == null ? -1 : obj.Key;


nit: could simplify to obj?.Key ?? -1

chenriksson · 2018-01-03T16:12:34Z

tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs

-                .Setup(r => r.IsPushAllowed(It.IsAny<string>(), It.IsAny<User>(), out matchingNamespaces))
-                .Returns(true);
+                .Setup(s => s.GetReservedNamespacesForId(It.IsAny<string>()))
+                    .Returns(new ReservedNamespace[0]);


nit: spacing?

chenriksson · 2018-01-03T16:16:22Z

tests/NuGetGallery.Facts/UrlExtensionsFacts.cs

@@ -69,7 +69,7 @@ public void ResolvePathIsCorrect()

                // Act
                var result = urlHelper.PackageRegistrationTemplate()
-                    .Resolve(new ListPackageItemViewModel(package));
+                    .Resolve(new ListPackageItemViewModel(package, null));


nit: use named parameter for nulls, if possible.

scottbommarito requested review from joelverhagen, agr, loic-sharma, xavierdecoster, shishirx34, nugetlurker, ryuyu, chenriksson, cristinamanum and skofman1 December 13, 2017 01:01

chenriksson reviewed Dec 13, 2017

View reviewed changes

scottbommarito force-pushed the sb-permservint2 branch from 7420c37 to 0c1f6f4 Compare December 14, 2017 22:51

scottbommarito changed the base branch from sb-permserv2 to sb-feature-orgpermissions December 14, 2017 22:51

scottbommarito force-pushed the sb-permservint2 branch from 0c1f6f4 to 14976d1 Compare December 21, 2017 21:32

scottbommarito force-pushed the sb-feature-orgpermissions branch 2 times, most recently from d80b513 to 0398a08 Compare December 21, 2017 22:56

scottbommarito force-pushed the sb-permservint2 branch from d780e1c to ccba226 Compare January 2, 2018 22:03

scottbommarito force-pushed the sb-permservint2 branch from ccba226 to cb7294c Compare January 2, 2018 22:15

scottbommarito force-pushed the sb-permservint2 branch from 6351706 to cb7294c Compare January 2, 2018 22:43

scottbommarito force-pushed the sb-feature-orgpermissions branch from 326975b to 0c02348 Compare January 2, 2018 22:54

Scott Bommarito added 4 commits January 2, 2018 14:55

integrate permissions service 2.0 with gallery

9e8ceb3

update with new onbehalfofany api

ea92d5e

rebase with changes

f9f681e

remove unnecessary organization field in listpackageownerviewmodel

1f7e4bc

scottbommarito force-pushed the sb-permservint2 branch from cb7294c to 1f7e4bc Compare January 2, 2018 22:59

chenriksson approved these changes Jan 3, 2018

View reviewed changes

pr comments

224fcdd

scottbommarito merged this pull request into sb-feature-orgpermissions Jan 3, 2018

scottbommarito pushed a commit that referenced this pull request Jan 8, 2018

Integrate PermissionsService 2.0 with Gallery (#5180)

0824e07

scottbommarito deleted the sb-permservint2 branch May 1, 2018 19:50

scottbommarito restored the sb-permservint2 branch July 22, 2019 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate PermissionsService 2.0 with Gallery #5180

Integrate PermissionsService 2.0 with Gallery #5180

scottbommarito commented Dec 13, 2017

chenriksson Dec 13, 2017

scottbommarito Dec 13, 2017

chenriksson Dec 15, 2017

scottbommarito Dec 15, 2017

chenriksson Dec 15, 2017

chenriksson Dec 15, 2017

scottbommarito Dec 15, 2017

chenriksson Dec 15, 2017

scottbommarito Dec 15, 2017

chenriksson Dec 13, 2017

scottbommarito Dec 13, 2017

chenriksson Dec 13, 2017

scottbommarito Dec 14, 2017

chenriksson commented Dec 15, 2017 •

edited

Loading

scottbommarito commented Jan 2, 2018

scottbommarito commented Jan 2, 2018

chenriksson commented Jan 2, 2018

scottbommarito commented Jan 2, 2018

scottbommarito commented Jan 2, 2018

chenriksson Jan 3, 2018

chenriksson Jan 3, 2018

chenriksson Jan 3, 2018

chenriksson Jan 3, 2018

chenriksson Jan 3, 2018

Integrate PermissionsService 2.0 with Gallery #5180

Integrate PermissionsService 2.0 with Gallery #5180

Conversation

scottbommarito commented Dec 13, 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

chenriksson commented Dec 15, 2017 • edited Loading

scottbommarito commented Jan 2, 2018

scottbommarito commented Jan 2, 2018

chenriksson commented Jan 2, 2018

scottbommarito commented Jan 2, 2018

scottbommarito commented Jan 2, 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

chenriksson commented Dec 15, 2017 •

edited

Loading