change API of trygetaccountsallowedonbehalfof (#5196)

NuGet · Dec 21, 2017 · 6e483d7 · 6e483d7
1 parent 9619934
commit 6e483d7
Show file tree

Hide file tree

Showing 6 changed files with 65 additions and 17 deletions.
diff --git a/src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs b/src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs
@@ -43,7 +43,12 @@ public PermissionsCheckResult CheckPermissions(IPrincipal currentPrincipal, User
 
         protected abstract PermissionsCheckResult CheckPermissionsForEntity(User account, TEntity entity);
 
-        public bool TryGetAccountsAllowedOnBehalfOf(User currentUser, TEntity entity, out IEnumerable<User> accountsAllowedOnBehalfOf)
+        public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, TEntity entity)
+        {
+            return CheckPermissionsOnBehalfOfAnyAccount(currentUser, entity, out var accountsAllowedOnBehalfOf);
+        }
+
+        public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, TEntity entity, out IEnumerable<User> accountsAllowedOnBehalfOf)
         {
             accountsAllowedOnBehalfOf = new List<User>();
 
@@ -60,16 +65,19 @@ public bool TryGetAccountsAllowedOnBehalfOf(User currentUser, TEntity entity, ou
 
             possibleAccountsOnBehalfOf = possibleAccountsOnBehalfOf.Distinct(new UserEqualityComparer());
 
+            var aggregateResult = PermissionsCheckResult.Unknown;
+
             foreach (var accountOnBehalfOf in possibleAccountsOnBehalfOf)
             {
                 var result = CheckPermissions(currentUser, accountOnBehalfOf, entity);
+                aggregateResult = ChoosePermissionsCheckResult(aggregateResult, result);
                 if (result == PermissionsCheckResult.Allowed)
                 {
                     (accountsAllowedOnBehalfOf as List<User>).Add(accountOnBehalfOf);
                 }
             }
 
-            return accountsAllowedOnBehalfOf.Any();
+            return aggregateResult;
         }
 
         protected abstract IEnumerable<User> GetOwners(TEntity entity);
@@ -86,5 +94,15 @@ public int GetHashCode(User obj)
                 return obj.Key.GetHashCode();
             }
         }
+
+        private PermissionsCheckResult ChoosePermissionsCheckResult(PermissionsCheckResult current, PermissionsCheckResult next)
+        {
+            if (current == PermissionsCheckResult.Allowed || next == PermissionsCheckResult.Allowed)
+            {
+                return PermissionsCheckResult.Allowed;
+            }
+
+            return new[] { current, next }.Max();
+        }
     }
 }
diff --git a/src/NuGetGallery/Services/ActionRequiringPackagePermissions.cs b/src/NuGetGallery/Services/ActionRequiringPackagePermissions.cs
@@ -38,9 +38,14 @@ protected override PermissionsCheckResult CheckPermissionsForEntity(User account
             return PermissionsHelpers.IsRequirementSatisfied(PackageRegistrationPermissionsRequirement, account, packageRegistration) ? PermissionsCheckResult.Allowed : PermissionsCheckResult.PackageRegistrationFailure;
         }
 
-        public bool TryGetAccountsAllowedOnBehalfOf(User currentUser, Package package, out IEnumerable<User> accountsAllowedOnBehalfOf)
+        public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, Package package)
         {
-            return TryGetAccountsAllowedOnBehalfOf(currentUser, GetPackageRegistration(package), out accountsAllowedOnBehalfOf);
+            return CheckPermissionsOnBehalfOfAnyAccount(currentUser, GetPackageRegistration(package));
+        }
+
+        public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, Package package, out IEnumerable<User> accountsAllowedOnBehalfOf)
+        {
+            return CheckPermissionsOnBehalfOfAnyAccount(currentUser, GetPackageRegistration(package), out accountsAllowedOnBehalfOf);
         }
 
         protected override IEnumerable<User> GetOwners(PackageRegistration packageRegistration)

diff --git a/src/NuGetGallery/Services/ActionRequiringReservedNamespacePermissions.cs b/src/NuGetGallery/Services/ActionRequiringReservedNamespacePermissions.cs
@@ -49,9 +49,14 @@ protected override PermissionsCheckResult CheckPermissionsForEntity(User account
                 PermissionsCheckResult.Allowed : PermissionsCheckResult.ReservedNamespaceFailure;
         }
 
-        public bool TryGetAccountsAllowedOnBehalfOf(User currentUser, ActionOnNewPackageContext newPackageContext, out IEnumerable<User> accountsAllowedOnBehalfOf)
+        public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, ActionOnNewPackageContext newPackageContext)
         {
-            return TryGetAccountsAllowedOnBehalfOf(currentUser, GetReservedNamespaces(newPackageContext), out accountsAllowedOnBehalfOf);
+            return CheckPermissionsOnBehalfOfAnyAccount(currentUser, GetReservedNamespaces(newPackageContext));
+        }
+
+        public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, ActionOnNewPackageContext newPackageContext, out IEnumerable<User> accountsAllowedOnBehalfOf)
+        {
+            return CheckPermissionsOnBehalfOfAnyAccount(currentUser, GetReservedNamespaces(newPackageContext), out accountsAllowedOnBehalfOf);
         }
 
         protected override IEnumerable<User> GetOwners(IReadOnlyCollection<ReservedNamespace> reservedNamespaces)

diff --git a/src/NuGetGallery/Services/IActionRequiringEntityPermissions.cs b/src/NuGetGallery/Services/IActionRequiringEntityPermissions.cs
@@ -25,11 +25,17 @@ public interface IActionRequiringEntityPermissions<TEntity>
         /// </returns>
         PermissionsCheckResult CheckPermissions(IPrincipal currentPrincipal, 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"/>.
+        /// </summary>
+        /// <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>
+        PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, TEntity entity);
+
         /// <summary>
         /// Determines whether <paramref name="currentPrincipal"/> can perform this action on <paramref name="entity"/> on behalf of any <see cref="User"/>.
         /// </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>
-        bool TryGetAccountsAllowedOnBehalfOf(User currentUser, TEntity entity, out IEnumerable<User> accountsAllowedOnBehalfOf);
+        PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, TEntity entity, out IEnumerable<User> accountsAllowedOnBehalfOf);
     }
 }
diff --git a/src/NuGetGallery/Services/PermissionsCheckResult.cs b/src/NuGetGallery/Services/PermissionsCheckResult.cs
@@ -13,6 +13,11 @@ public enum PermissionsCheckResult
         /// </summary>
         Allowed = default(int),
 
+        /// <summary>
+        /// The permissions check failed for an unknown reason.
+        /// </summary>
+        Unknown,
+
         /// <summary>
         /// The current user does not have permissions to perform the action on the <see cref="User"/>.
         /// </summary>

diff --git a/tests/NuGetGallery.Facts/Services/ActionRequiringEntityPermissionsFacts.cs b/tests/NuGetGallery.Facts/Services/ActionRequiringEntityPermissionsFacts.cs
@@ -43,7 +43,7 @@ protected override PermissionsCheckResult CheckPermissionsForEntity(User account
             }
         }
 
-        public class TheIsAllowedMethod
+        public class TheCheckPermissionsMethod
         {
             [Fact]
             public void ReturnsAccountPermissionsFailureWhenAccountCheckFails()
@@ -67,14 +67,15 @@ private void AssertIsAllowed(ActionRequiringEntityPermissions<Entity> action, Pe
             }
         }
 
-        public class TheTryGetAccountsAllowedOnBehalfOfMethod
+        public class TheCheckPermissionsOnBehalfOfAnyAccountMethod
         {
             [Fact]
             public void SuccessWithNullAccount()
             {
                 var action = new TestableActionRequiringEntityPermissions(PermissionsRequirement.None, (account, entity) => PermissionsCheckResult.Allowed);
 
-                Assert.True(action.TryGetAccountsAllowedOnBehalfOf(null, null, out var accountsAllowedOnBehalfOf));
+                Assert.Equal(PermissionsCheckResult.Allowed, action.CheckPermissionsOnBehalfOfAnyAccount(null, null));
+                Assert.Equal(PermissionsCheckResult.Allowed, action.CheckPermissionsOnBehalfOfAnyAccount(null, null, out var accountsAllowedOnBehalfOf));
                 Assert.True(accountsAllowedOnBehalfOf.SequenceEqual(new User[] { null }));
             }
 
@@ -83,7 +84,8 @@ public void FailureWithNullAccount()
             {
                 var action = new TestableActionRequiringEntityPermissions(PermissionsRequirement.Unsatisfiable, (a, e) => PermissionsCheckResult.Allowed);
 
-                Assert.False(action.TryGetAccountsAllowedOnBehalfOf(null, null, out var accountsAllowedOnBehalfOf));
+                Assert.Equal(PermissionsCheckResult.AccountFailure, action.CheckPermissionsOnBehalfOfAnyAccount(null, null));
+                Assert.Equal(PermissionsCheckResult.AccountFailure, action.CheckPermissionsOnBehalfOfAnyAccount(null, null, out var accountsAllowedOnBehalfOf));
                 Assert.Empty(accountsAllowedOnBehalfOf);
             }
 
@@ -94,18 +96,21 @@ public void SuccessWithNullEntity()
 
                 var action = new TestableActionRequiringEntityPermissions(PermissionsRequirement.None, (a, e) => PermissionsCheckResult.Allowed);
 
-                Assert.True(action.TryGetAccountsAllowedOnBehalfOf(user, null, out var accountsAllowedOnBehalfOf));
+                Assert.Equal(PermissionsCheckResult.Allowed, action.CheckPermissionsOnBehalfOfAnyAccount(user, null));
+                Assert.Equal(PermissionsCheckResult.Allowed, action.CheckPermissionsOnBehalfOfAnyAccount(user, null, out var accountsAllowedOnBehalfOf));
                 Assert.True(accountsAllowedOnBehalfOf.SequenceEqual(new[] { user }));
             }
 
             [Fact]
             public void FailureWithNullEntity()
             {
+                var failureResult = (PermissionsCheckResult)99;
                 var user = new User { Key = 1 };
 
                 var action = new TestableActionRequiringEntityPermissions(PermissionsRequirement.None, (a, e) => (PermissionsCheckResult)99);
 
-                Assert.False(action.TryGetAccountsAllowedOnBehalfOf(user, null, out var accountsAllowedOnBehalfOf));
+                Assert.Equal(failureResult, action.CheckPermissionsOnBehalfOfAnyAccount(user, null));
+                Assert.Equal(failureResult, action.CheckPermissionsOnBehalfOfAnyAccount(user, null, out var accountsAllowedOnBehalfOf));
                 Assert.Empty(accountsAllowedOnBehalfOf);
             }
 
@@ -116,7 +121,8 @@ public void OrganizationsOfCurrentUserArePossibleAccounts()
 
                 var action = new TestableActionRequiringEntityPermissions(PermissionsRequirement.None, (a, e) => PermissionsCheckResult.Allowed);
 
-                Assert.True(action.TryGetAccountsAllowedOnBehalfOf(user, null, out var accountsAllowedOnBehalfOf));
+                Assert.Equal(PermissionsCheckResult.Allowed, action.CheckPermissionsOnBehalfOfAnyAccount(user, null));
+                Assert.Equal(PermissionsCheckResult.Allowed, action.CheckPermissionsOnBehalfOfAnyAccount(user, null, out var accountsAllowedOnBehalfOf));
                 Assert.True(accountsAllowedOnBehalfOf.SequenceEqual(new[] { user, organization }));
             }
 
@@ -127,7 +133,8 @@ public void OwnersOfEntityArePossibleAccounts()
 
                 var action = new TestableActionRequiringEntityPermissions(PermissionsRequirement.None, (a, e) => PermissionsCheckResult.Allowed);
 
-                Assert.True(action.TryGetAccountsAllowedOnBehalfOf(null, entity, out var accountsAllowedOnBehalfOf));
+                Assert.Equal(PermissionsCheckResult.Allowed, action.CheckPermissionsOnBehalfOfAnyAccount(null, entity));
+                Assert.Equal(PermissionsCheckResult.Allowed, action.CheckPermissionsOnBehalfOfAnyAccount(null, entity, out var accountsAllowedOnBehalfOf));
                 Assert.True(accountsAllowedOnBehalfOf.SequenceEqual(new[] { null, entityOwner }));
             }
 
@@ -139,7 +146,8 @@ public void OrganizationsOfCurrentUserAndOwnersOfEntityArePossibleAccounts()
 
                 var action = new TestableActionRequiringEntityPermissions(PermissionsRequirement.None, (a, e) => PermissionsCheckResult.Allowed);
 
-                Assert.True(action.TryGetAccountsAllowedOnBehalfOf(user, entity, out var accountsAllowedOnBehalfOf));
+                Assert.Equal(PermissionsCheckResult.Allowed, action.CheckPermissionsOnBehalfOfAnyAccount(user, entity));
+                Assert.Equal(PermissionsCheckResult.Allowed, action.CheckPermissionsOnBehalfOfAnyAccount(user, entity, out var accountsAllowedOnBehalfOf));
                 Assert.True(accountsAllowedOnBehalfOf.SequenceEqual(new[] { user, entityOwner, organization }));
             }
 
@@ -166,7 +174,8 @@ public void AccountsAllowedOnBehalfOfIsPopulatedWithExpectedAccounts(int expecte
                 var action = new TestableActionRequiringEntityPermissions(PermissionsRequirement.None, 
                     (a, e) => expectedAccountsList.Any(u => u.MatchesUser(a)) ? PermissionsCheckResult.Allowed : (PermissionsCheckResult)99);
 
-                Assert.True(action.TryGetAccountsAllowedOnBehalfOf(user, entity, out var accountsAllowedOnBehalfOf));
+                Assert.Equal(PermissionsCheckResult.Allowed, action.CheckPermissionsOnBehalfOfAnyAccount(user, entity));
+                Assert.Equal(PermissionsCheckResult.Allowed, action.CheckPermissionsOnBehalfOfAnyAccount(user, entity, out var accountsAllowedOnBehalfOf));
                 Assert.True(accountsAllowedOnBehalfOf.SequenceEqual(expectedAccountsList));
             }