PermissionsService 2.0: ActionsRequringPermission #5097

scottbommarito · 2017-11-30T20:24:07Z

I'm merging this into a feature branch because I want to add some more unit tests with future PRs, but would like to merge this in now and then start working on unit tests of controller methods.

Rename old PermissionsService into PermissionsHelpers
Rename PermissionLevel to PermissionsRequirement
Add ActionRequiringAccountPermissions, IActionRequiringEntityPermissions, ActionRequiringEntityPermissions, ActionRequiringPackagePermissions and ActionRequiringReservedNamespacePermissions for consolidating permissions logic
Add static ActionsRequiringPermissions to store statically declared instances of the classes described above
-Replace old PermissionsService calls with new ActionsRequiringPermissions calls.

Old API:

if (!PermissionsService.IsActionAllowed(accountOnBehalfOf, HttpContext.User, AccountActions.UploadNewVersionOnBehalfOf))
{
    ...
}

if (!PermissionsService.IsActionAllowed(package, accountOnBehalfOf, PackageActions.UploadNewVersion))
{
    ...
}

New API:

if (ActionsRequiringPermissions.UploadNewPackageVersion.IsAllowed(currentUser, accountOnBehalfOf, package) != PermissionsFailure.None)
{
    ...
}

joelverhagen · 2017-12-01T02:18:26Z

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))


TryGetAccountsIsAllowedOnBehalfOf sounds awkward:

TryGetAccountIsAllowedOnBehalfOf TryGetAccountsAreAllowedOnBehalfOf

How is this out parameter used?

not used in this PR--it's used in the upload flow to determine the list of accounts that the current user can upload a package on behalf of

Why use the Try pattern here? There should always be 1 account allowed (self). What is the scenario where you expect this API to return false?

Maybe just GetOnBehalfOfAccounts?

The out parameter returns accounts the action is allowed ON BEHALF OF.

For example, if you were a member of two organizations that owned a package and you called this method on them, the out parameter would return both, because you can do the action on behalf of both of them.

Does the return value of TryGetAccountsIsAllowedOnBehalfOf have any correlation with the collection count of the out parameter?
If not, would it be better to have two methods instead of one?

joelverhagen · 2017-12-01T02:20:17Z

src/NuGetGallery/Controllers/PackagesController.cs

                // For a new package id verify if the user is allowed to use it.
-                if (packageRegistration == null)
+                if (packageRegistration == null && !ActionsRequiringPermissions.UploadNewPackageId.TryGetAccountsIsAllowedOnBehalfOf(currentUser, new ActionOnNewPackageContext(id, _reservedNamespaceService), out accountsAllowedOnBehalfOf))


Wait, why are we passing a dependency as a parameter? I see that this is a static... could it become an injected thing?

(That would also facilitate unit testing)

joelverhagen · 2017-12-01T02:23:14Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+        {
+            if (currentUser == null)
+            {
+                return PermissionLevelsIntersect(PermissionsRequirement.None, permissionsRequirement);


The semantics of intersect suggest a constant output, i.e. A ∩ ∅ = ∅

joelverhagen · 2017-12-01T02:24:03Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+            }
+
+            var matchingMembers = entityOwners
+                .Where(o => o is Organization)


This is equivalent to OfType<Organization>

TIL, nice catch

joelverhagen · 2017-12-01T02:25:38Z

I am still reviewing this. Very large PR 🤓. Could we have built the framework as one PR and integrated in another?

chenriksson · 2017-12-06T00:29:34Z

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 use the Try pattern here? There should always be 1 account allowed (self). What is the scenario where you expect this API to return false?

Maybe just GetOnBehalfOfAccounts?

chenriksson · 2017-12-06T17:40:44Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+
+namespace NuGetGallery
+{
+    public class PermissionsHelpers


static class

chenriksson · 2017-12-06T17:43:14Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+    public class PermissionsHelpers
+    {
+        /// <summary>
+        /// Is <paramref name="currentPrincipal"/> allowed to perform an action with a requirement of <paramref name="permissionsRequirement"/> on <paramref name="packageRegistration"/>?


Lines are long in this file, making it hard to review - can you wrap?

Do we have guideline as team for max length -- maybe 120 or 130?

IMO line length should be something handled by the tooling.

Not sure the best way to fix the line length, but I was under the impression the only long lines are the comments.

Also this file is 90% the same from before besides renaming.

chenriksson · 2017-12-06T17:50:47Z

src/NuGetGallery/Services/ReservedNamespaceService.cs

        private bool ShouldForceSharedNamespace(string value)
        {
            var liberalMatchingNamespaces = GetReservedNamespacesForId(value);
            return liberalMatchingNamespaces.Any(rn => rn.IsSharedNamespace);
        }

+        public bool ShouldMarkNewPackageIdVerified(User account, string id, out IReadOnlyCollection<ReservedNamespace> ownedMatchingReservedNamespaces)


ReservedNamespaceService refactoring doesn't look related to the rest of this PR?

What's the need for the refactoring? Should it be in a separate PR?

chenriksson · 2017-12-06T17:54:52Z

src/NuGetGallery/NuGetGallery.csproj

+    <Compile Include="Services\ActionRequiringEntityPermissions.cs" />
+    <Compile Include="Services\ActionRequiringPackagePermissions.cs" />
+    <Compile Include="Services\ActionRequiringReservedNamespacePermissions.cs" />
+    <Compile Include="Services\ActionsRequiringPermissions.cs" />


We no longer have a permissions service. Should these permissions classes still be in the services namespace?

Two notes:

1 - We don't actually have a NuGetGallery.Services namespace, everything is in NuGetGallery.
2 - I think they're ok in services, but TBH I'd prefer to split everything up more granularly and add a Permissions folder

chenriksson · 2017-12-06T17:56:54Z

src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs

+    /// An action requiring permissions on an entity that can be done on behalf of another <see cref="User"/>.
+    /// </summary>
+    public abstract class ActionRequiringEntityPermissions<TEntity>
+        : IActionRequiringEntityPermissions<TEntity>


Should there be where TEntity : IEntity?

No--we also allow ListPackageItemViewModel to be here.

I was considering making a superclass (IOwnedEntity) and making PackageRegistration, ReservedNamespace, User, and ListPackageItemViewModel extend it, but I decided it would be better to leave it alone.

chenriksson · 2017-12-06T17:59:00Z

src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs

+        /// </summary>
+        /// <param name="accountsAllowedOnBehalfOf">A <see cref="IEnumerable{User}"/> containing all accounts that <paramref name="currentUser"/> can perform this action on <paramref name="entity"/> on behalf of.</param>
+        /// <returns>True if and only if <paramref name="currentPrincipal"/> can perform this action on <paramref name="entity"/> on behalf of any <see cref="User"/>.</returns>
+        public bool TryGetAccountsIsAllowedOnBehalfOf(User currentUser, TEntity entity, out IEnumerable<User> accountsAllowedOnBehalfOf)


See other comment... why use the Try pattern here?

The out parameter returns accounts the action is allowed ON BEHALF OF.

In other words, the other method (IsAllowed) requires you to specify an account you want to perform the action on behalf of. This method figures out if what accounts you can do the action on behalf of.

For example, if you were a member of two organizations that owned a package and you called this method on them, the out parameter would return both, because you can do the action on behalf of both of them. If you were also an owner of the package, it would return you as well.

The method as a whole returns whether or not you can do it on behalf of anyone.

As I mentioned above, the primary use case is determined who you can push a package on behalf of.

Why do not remove the the out and return the IEnum and the caller of this method can do the .Any() ?

I prefer the TryGet model rather than a boolean because
1 - This method is frequently used simply as a boolean, without concern for the result of the out parameter and removing it would add a lot of excess .Any() calls around the code
2 - The caller would now need to understand that if an action does not have any accounts it can be performed on behalf of, it is not allowed.

chenriksson · 2017-12-06T18:00:55Z

src/NuGetGallery/Services/ActionRequiringPackagePermissions.cs

+            return packageRegistration != null ? packageRegistration.Owners : Enumerable.Empty<User>();
+        }
+
+        private PackageRegistration ConvertPackageToRegistration(Package package)


Get is more appropriate than Convert. Method doesn't look necessary, would just inline.

I like the naming change.

I added this method because I wanted to guarantee that the method for converting between a Package and a PackageRegistration is consistent.

chenriksson · 2017-12-06T18:03:14Z

src/NuGetGallery/Services/ActionsRequiringPermissions.cs

+    /// <summary>
+    /// A collection of all <see cref="ActionRequiringAccountPermissions"/> and <see cref="IActionRequiringEntityPermissions{TEntity}"/>s.
+    /// </summary>
+    public static class ActionsRequiringPermissions


Maybe members should be sorted by entity type, possibly using regions?

I wasn't sure how to sort the file, but entity type sounds better.

chenriksson · 2017-12-06T18:07:25Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+
+namespace NuGetGallery
+{
+    public class PermissionsHelpers


APIs look a lot cleaner!

As mentioned, only concern with static class is that it could be more difficult to integrate auditing/telemetry into the permissions helpers. We can figure it out if that becomes necessary.

cristinamanum · 2017-12-06T19:14:30Z

src/NuGetGallery/Controllers/PackagesController.cs

            if (package == null
                || ((package.PackageStatusKey == PackageStatus.Validating
                     || package.PackageStatusKey == PackageStatus.FailedValidation)
-                    && !PermissionsService.IsActionAllowed(package, User, PackageActions.DisplayPrivatePackage)))
+                    && !ActionsRequiringPermissions.DisplayPrivatePackageMetadata.TryGetAccountsIsAllowedOnBehalfOf(currentUser, package, out accountsAllowedOnBehalfOf)))


nit: shorter lines?

cristinamanum · 2017-12-06T19:15:04Z

src/NuGetGallery/Controllers/PackagesController.cs

            // Validating packages should be hidden to everyone but the owners and admins.
+            var currentUser = GetCurrentUser();
+            IEnumerable<User> accountsAllowedOnBehalfOf;


Is this used anywhere?

just to contain the unused out parameter

the out parameter will be used in future upload flows

cristinamanum · 2017-12-06T19:39:08Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+        private static bool IsRequirementSatisfied(PermissionsRequirement permissionsRequirement, bool isUserAdmin, Func<User, bool> isUserMatch, IEnumerable<User> entityOwners)
+        {
+            if ((entityOwners == null || !entityOwners.Any()) &&
+                PermissionLevelsIntersect(PermissionsRequirement.None, permissionsRequirement))


Should not always return true on PermissionRequirement.None ?

good catch, I can restructure this code to make that check first.

cristinamanum · 2017-12-06T19:51:37Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+            }
+
+            if (entityOwners.Any(isUserMatch) &&
+                PermissionLevelsIntersect(PermissionsRequirement.Owner, permissionsRequirement))


Maybe change the order of the operands to not execute Any(isUserMarch) if not the Owner permissionRequirement

applying that logic to all of these checks would probably make it substantially faster...nice catch!

cristinamanum · 2017-12-06T19:55:59Z

src/NuGetGallery/Services/ActionRequiringAccountPermissions.cs

+        /// <summary>
+        /// Determines whether <paramref name="currentUser"/> can perform this action on <paramref name="account"/>.
+        /// </summary>
+        public PermissionsFailure IsAllowed(User currentUser, User account)


PermissionFailure suggests a failure. Will this call be always a failure? Why not returning bool?

PermissionsCheckResult instead? Almost all of the enum results are failures which is why I called it PermissionsFailure, but it probably could be clearer if it was not named that.

cristinamanum · 2017-12-06T19:59:22Z

src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs

+        protected abstract PermissionsFailure IsAllowedOnEntity(User account, TEntity entity);
+
+        /// <summary>
+        /// Determines whether <paramref name="currentPrincipal"/> can perform this action on <paramref name="entity"/> on behalf of any <see cref="User"/>.


currentPrincipal -> currentUser?

scottbommarito · 2017-12-06T23:33:01Z

Closing this PR because I'm planning on splitting it up.

scottbommarito added 7 commits November 28, 2017 14:31

permissionhelpers

883ba58

ACTIONSREQUIRINGPERMISSIONS

0bb78cd

use actionsrequiringpermissions in cshtml

c0cc9c9

add back current user not found tests

566cd2b

add comments to actions

8d66d45

actionrequiringentitypermissionsfacts

26d8172

rename anonymous

6de0725

scottbommarito requested review from joelverhagen, agr, loic-sharma, xavierdecoster, shishirx34, ryuyu, chenriksson, cristinamanum and skofman1 November 30, 2017 20:24

dnfclas added the cla-already-signed label Nov 30, 2017

cache action permissions result in view model

25a1866

joelverhagen reviewed Dec 1, 2017

View reviewed changes

dnfgituser removed the cla-already-signed label Dec 6, 2017

chenriksson suggested changes Dec 6, 2017

View reviewed changes

cristinamanum reviewed Dec 6, 2017

View reviewed changes

scottbommarito closed this Dec 6, 2017

scottbommarito mentioned this pull request Dec 9, 2017

PermissionsService 2.0 (just implementation) #5158

Merged

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

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

PermissionsService 2.0: ActionsRequringPermission #5097

PermissionsService 2.0: ActionsRequringPermission #5097

Conversation

scottbommarito commented Nov 30, 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

joelverhagen commented Dec 1, 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

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

scottbommarito commented Dec 6, 2017

scottbommarito commented Nov 30, 2017 •

edited

Loading