From 91289f1c52b63f78c1a4a72e26bc5b8ea3750508 Mon Sep 17 00:00:00 2001 From: Scott Bommarito Date: Fri, 15 Dec 2017 16:52:15 -0800 Subject: [PATCH] change API of trygetaccountsallowedonbehalfof --- .../ActionRequiringEntityPermissions.cs | 22 ++++++++++++-- .../ActionRequiringPackagePermissions.cs | 9 ++++-- ...onRequiringReservedNamespacePermissions.cs | 9 ++++-- .../IActionRequiringEntityPermissions.cs | 8 ++++- .../Services/PermissionsCheckResult.cs | 5 ++++ .../ActionRequiringEntityPermissionsFacts.cs | 29 ++++++++++++------- 6 files changed, 65 insertions(+), 17 deletions(-) diff --git a/src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs b/src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs index 736a9c42ef..bfa4ab8cb3 100644 --- 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 accountsAllowedOnBehalfOf) + public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, TEntity entity) + { + return CheckPermissionsOnBehalfOfAnyAccount(currentUser, entity, out var accountsAllowedOnBehalfOf); + } + + public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, TEntity entity, out IEnumerable accountsAllowedOnBehalfOf) { accountsAllowedOnBehalfOf = new List(); @@ -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).Add(accountOnBehalfOf); } } - return accountsAllowedOnBehalfOf.Any(); + return aggregateResult; } protected abstract IEnumerable 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(); + } } } \ No newline at end of file diff --git a/src/NuGetGallery/Services/ActionRequiringPackagePermissions.cs b/src/NuGetGallery/Services/ActionRequiringPackagePermissions.cs index 8312b57c98..d2914c5b4d 100644 --- 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 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 accountsAllowedOnBehalfOf) + { + return CheckPermissionsOnBehalfOfAnyAccount(currentUser, GetPackageRegistration(package), out accountsAllowedOnBehalfOf); } protected override IEnumerable GetOwners(PackageRegistration packageRegistration) diff --git a/src/NuGetGallery/Services/ActionRequiringReservedNamespacePermissions.cs b/src/NuGetGallery/Services/ActionRequiringReservedNamespacePermissions.cs index 988ec1d85d..9ff31dec6b 100644 --- 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 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 accountsAllowedOnBehalfOf) + { + return CheckPermissionsOnBehalfOfAnyAccount(currentUser, GetReservedNamespaces(newPackageContext), out accountsAllowedOnBehalfOf); } protected override IEnumerable GetOwners(IReadOnlyCollection reservedNamespaces) diff --git a/src/NuGetGallery/Services/IActionRequiringEntityPermissions.cs b/src/NuGetGallery/Services/IActionRequiringEntityPermissions.cs index cdaa53f78e..8bab58dc1a 100644 --- a/src/NuGetGallery/Services/IActionRequiringEntityPermissions.cs +++ b/src/NuGetGallery/Services/IActionRequiringEntityPermissions.cs @@ -25,11 +25,17 @@ public interface IActionRequiringEntityPermissions /// PermissionsCheckResult CheckPermissions(IPrincipal currentPrincipal, User account, TEntity entity); + /// + /// Determines whether can perform this action on on behalf of any . + /// + /// True if and only if can perform this action on on behalf of any . + PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, TEntity entity); + /// /// Determines whether can perform this action on on behalf of any . /// /// A containing all accounts that can perform this action on on behalf of. /// True if and only if can perform this action on on behalf of any . - bool TryGetAccountsAllowedOnBehalfOf(User currentUser, TEntity entity, out IEnumerable accountsAllowedOnBehalfOf); + PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, TEntity entity, out IEnumerable accountsAllowedOnBehalfOf); } } \ No newline at end of file diff --git a/src/NuGetGallery/Services/PermissionsCheckResult.cs b/src/NuGetGallery/Services/PermissionsCheckResult.cs index c65c8dd4d1..891ae51376 100644 --- a/src/NuGetGallery/Services/PermissionsCheckResult.cs +++ b/src/NuGetGallery/Services/PermissionsCheckResult.cs @@ -13,6 +13,11 @@ public enum PermissionsCheckResult /// Allowed = default(int), + /// + /// The permissions check failed for an unknown reason. + /// + Unknown, + /// /// The current user does not have permissions to perform the action on the . /// diff --git a/tests/NuGetGallery.Facts/Services/ActionRequiringEntityPermissionsFacts.cs b/tests/NuGetGallery.Facts/Services/ActionRequiringEntityPermissionsFacts.cs index c757ac915d..e01489d0d4 100644 --- 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 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)); }