diff --git a/src/NuGetGallery/Authentication/ApiScopeEvaluationResult.cs b/src/NuGetGallery/Authentication/ApiScopeEvaluationResult.cs new file mode 100644 index 0000000000..3c481b1b2d --- /dev/null +++ b/src/NuGetGallery/Authentication/ApiScopeEvaluationResult.cs @@ -0,0 +1,44 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +namespace NuGetGallery.Authentication +{ + + /// + /// The result of evaluating the current user's scopes by using . + /// + public class ApiScopeEvaluationResult + { + /// + /// True IFF any scope's subject () and allowed action () match the subject being acted upon and the action being performed. + /// + public bool ScopesAreValid { get; } + + /// + /// If is true, the returned by checking the permission of the scope's owner (). + /// Otherwise, . + /// + public PermissionsCheckResult PermissionsCheckResult { get; } + + /// + /// The owner of the scope as acquired from . + /// + public User Owner { get; } + + public ApiScopeEvaluationResult(User owner, PermissionsCheckResult permissionsCheckResult, bool scopesAreValid) + { + ScopesAreValid = scopesAreValid; + PermissionsCheckResult = permissionsCheckResult; + Owner = owner; + } + + /// + /// Returns whether or not this represents a successful authentication. + /// If this does not represent a successful authentication, the current user should be denied from performing the action they are attempting to perform. + /// + public bool IsSuccessful() + { + return ScopesAreValid && PermissionsCheckResult == PermissionsCheckResult.Allowed; + } + } +} \ No newline at end of file diff --git a/src/NuGetGallery/Authentication/ApiScopeEvaluator.cs b/src/NuGetGallery/Authentication/ApiScopeEvaluator.cs new file mode 100644 index 0000000000..4ad2267468 --- /dev/null +++ b/src/NuGetGallery/Authentication/ApiScopeEvaluator.cs @@ -0,0 +1,81 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Linq; + +namespace NuGetGallery.Authentication +{ + public class ApiScopeEvaluator : IApiScopeEvaluator + { + private readonly IUserService _userService; + + public ApiScopeEvaluator(IUserService userService) + { + _userService = userService ?? throw new ArgumentNullException(nameof(userService)); + } + + public ApiScopeEvaluationResult Evaluate( + User currentUser, + IEnumerable scopes, + IActionRequiringEntityPermissions action, + PackageRegistration packageRegistration, + params string[] requestedActions) + { + return Evaluate(currentUser, scopes, action, packageRegistration, pr => pr.Id, requestedActions); + } + + public ApiScopeEvaluationResult Evaluate( + User currentUser, + IEnumerable scopes, + IActionRequiringEntityPermissions action, + ActionOnNewPackageContext context, + params string[] requestedActions) + { + return Evaluate(currentUser, scopes, action, context, c => c.PackageId, requestedActions); + } + + /// This method is internal because it is tested directly. + internal ApiScopeEvaluationResult Evaluate( + User currentUser, + IEnumerable scopes, + IActionRequiringEntityPermissions action, + TEntity entity, + Func getSubjectFromEntity, + params string[] requestedActions) + { + User ownerInScope = null; + + if (scopes == null || !scopes.Any()) + { + // Legacy V1 API key without scopes. + // Evaluate it as if it has an unlimited scope. + scopes = new[] { new Scope(ownerKey: null, subject: NuGetPackagePattern.AllInclusivePattern, allowedAction: NuGetScopes.All) }; + } + + // Check that all scopes provided have the same owner scope. + var ownerScopes = scopes.Select(s => s.OwnerKey); + var ownerScope = ownerScopes.FirstOrDefault(); + if (ownerScopes.Any(o => o != ownerScope)) + { + throw new ArgumentException("All scopes provided must have the same owner scope."); + } + + var matchingScope = scopes + .FirstOrDefault(scope => + scope.AllowsSubject(getSubjectFromEntity(entity)) && + scope.AllowsActions(requestedActions)); + + ownerInScope = ownerScope.HasValue ? _userService.FindByKey(ownerScope.Value) : currentUser; + + if (matchingScope == null) + { + return new ApiScopeEvaluationResult(ownerInScope, PermissionsCheckResult.Unknown, scopesAreValid: false); + } + + var isActionAllowed = action.CheckPermissions(currentUser, ownerInScope, entity); + return new ApiScopeEvaluationResult(ownerInScope, isActionAllowed, scopesAreValid: true); + } + } +} \ No newline at end of file diff --git a/src/NuGetGallery/Authentication/IApiScopeEvaluator.cs b/src/NuGetGallery/Authentication/IApiScopeEvaluator.cs new file mode 100644 index 0000000000..34b753ccb9 --- /dev/null +++ b/src/NuGetGallery/Authentication/IApiScopeEvaluator.cs @@ -0,0 +1,40 @@ +using System; +using System.Collections.Generic; + +namespace NuGetGallery.Authentication +{ + public interface IApiScopeEvaluator + { + /// + /// Evaluates the whether or not an action is allowed given a set of , an , and the . + /// + /// The current user attempting to do the action with the given . + /// The scopes being evaluated. + /// The action that the scopes being evaluated are checked for permission to do. + /// The that the scopes being evaluated are checked for permission to on. + /// A list of actions that the scopes must match. + /// A that describes the evaluation of the s. + ApiScopeEvaluationResult Evaluate( + User currentUser, + IEnumerable scopes, + IActionRequiringEntityPermissions action, + PackageRegistration packageRegistration, + params string[] requestedActions); + + /// + /// Evaluates the whether or not an action is allowed given a set of , an , and the . + /// + /// The current user attempting to do the action with the given . + /// The scopes being evaluated. + /// The action that the scopes being evaluated are checked for permission to do. + /// The that the scopes being evaluated are checked for permission to on. + /// A list of actions that the scopes must match. + /// A that describes the evaluation of the s. + ApiScopeEvaluationResult Evaluate( + User currentUser, + IEnumerable scopes, + IActionRequiringEntityPermissions action, + ActionOnNewPackageContext context, + params string[] requestedActions); + } +} diff --git a/src/NuGetGallery/Controllers/ApiController.cs b/src/NuGetGallery/Controllers/ApiController.cs index 69c0ef9746..30dc532e48 100644 --- a/src/NuGetGallery/Controllers/ApiController.cs +++ b/src/NuGetGallery/Controllers/ApiController.cs @@ -32,6 +32,7 @@ namespace NuGetGallery public partial class ApiController : AppController { + public IApiScopeEvaluator ApiScopeEvaluator { get; set; } public IEntitiesContext EntitiesContext { get; set; } public INuGetExeDownloaderService NugetExeDownloaderService { get; set; } public IPackageFileService PackageFileService { get; set; } @@ -59,6 +60,7 @@ protected ApiController() } public ApiController( + IApiScopeEvaluator apiScopeEvaluator, IEntitiesContext entitiesContext, IPackageService packageService, IPackageFileService packageFileService, @@ -79,6 +81,7 @@ public ApiController( IReservedNamespaceService reservedNamespaceService, IPackageUploadService packageUploadService) { + ApiScopeEvaluator = apiScopeEvaluator; EntitiesContext = entitiesContext; PackageService = packageService; PackageFileService = packageFileService; @@ -102,6 +105,7 @@ public ApiController( } public ApiController( + IApiScopeEvaluator apiScopeEvaluator, IEntitiesContext entitiesContext, IPackageService packageService, IPackageFileService packageFileService, @@ -122,9 +126,9 @@ public ApiController( ISecurityPolicyService securityPolicies, IReservedNamespaceService reservedNamespaceService, IPackageUploadService packageUploadService) - : this(entitiesContext, packageService, packageFileService, userService, nugetExeDownloaderService, contentService, + : this(apiScopeEvaluator, entitiesContext, packageService, packageFileService, userService, nugetExeDownloaderService, contentService, indexingService, searchService, autoCuratePackage, statusService, messageService, auditingService, - configurationService, telemetryService, authenticationService, credentialBuilder, securityPolicies, + configurationService, telemetryService, authenticationService, credentialBuilder, securityPolicies, reservedNamespaceService, packageUploadService) { StatisticsService = statisticsService; @@ -292,22 +296,21 @@ private async Task VerifyPackageKeyInternalAsync(U // Write an audit record await AuditingService.SaveAuditRecordAsync( new PackageAuditRecord(package, AuditedPackageAction.Verify)); - + + string[] requestedActions; if (CredentialTypes.IsPackageVerificationApiKey(credential.Type)) { - // Secure path: verify that verification key matches package scope. - if (!HasAnyScopeThatAllows(package.PackageRegistration, NuGetScopes.PackageVerify)) - { - return new HttpStatusCodeWithBodyResult(HttpStatusCode.Forbidden, Strings.ApiKeyNotAuthorized); - } + requestedActions = new[] { NuGetScopes.PackageVerify }; } else { - // Insecure path: verify that API key is legacy or matches package scope. - if (!HasAnyScopeThatAllows(package.PackageRegistration, NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion)) - { - return new HttpStatusCodeWithBodyResult(HttpStatusCode.Forbidden, Strings.ApiKeyNotAuthorized); - } + requestedActions = new[] { NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion }; + } + + var apiScopeEvaluationResult = EvaluateApiScope(ActionsRequiringPermissions.VerifyPackage, package.PackageRegistration, requestedActions); + if (!apiScopeEvaluationResult.IsSuccessful()) + { + return GetHttpResultFromFailedApiScopeEvaluation(apiScopeEvaluationResult, id, version); } return null; @@ -340,7 +343,7 @@ private async Task CreatePackageInternal() } // Get the user - var user = GetCurrentUser(); + var currentUser = GetCurrentUser(); using (var packageStream = ReadPackageFromRequest()) { @@ -400,42 +403,39 @@ private async Task CreatePackageInternal() nuspec.GetMinClientVersion())); } + User owner; + // Ensure that the user can push packages for this partialId. var id = nuspec.GetId(); + var version = nuspec.GetVersion(); var packageRegistration = PackageService.FindPackageRegistrationById(id); if (packageRegistration == null) { - // Check if API key allows pushing a new package id - if (!HasAnyScopeThatAllowsPushNew(id)) + // Check if the current user's scopes allow pushing a new package ID + var apiScopeEvaluationResult = EvaluateApiScope(ActionsRequiringPermissions.UploadNewPackageId, new ActionOnNewPackageContext(id, ReservedNamespaceService), NuGetScopes.PackagePush); + owner = apiScopeEvaluationResult.Owner; + if (!apiScopeEvaluationResult.IsSuccessful()) { - // User cannot push a new package ID as the API key scope does not allow it - return new HttpStatusCodeWithBodyResult(HttpStatusCode.Unauthorized, Strings.ApiKeyNotAuthorized); - } - - // For a new package id verify that the user is allowed to push to the matching namespaces, if any. - if (ActionsRequiringPermissions.UploadNewPackageId.CheckPermissionsOnBehalfOfAnyAccount( - user, new ActionOnNewPackageContext(id, ReservedNamespaceService)) != PermissionsCheckResult.Allowed) - { - var version = nuspec.GetVersion().ToNormalizedString(); - TelemetryService.TrackPackagePushNamespaceConflictEvent(id, version, user, User.Identity); - - return new HttpStatusCodeWithBodyResult(HttpStatusCode.Conflict, Strings.UploadPackage_IdNamespaceConflict); + // User cannot push a new package ID as the current user's scopes does not allow it + return GetHttpResultFromFailedApiScopeEvaluationForPush(apiScopeEvaluationResult, id, version); } } else { - // Check if API key allows pushing the current package id - if (!HasAnyScopeThatAllows(packageRegistration, NuGetScopes.PackagePushVersion, NuGetScopes.PackagePush)) + // Check if the current user's scopes allow pushing a new version of an existing package ID + var apiScopeEvaluationResult = EvaluateApiScope(ActionsRequiringPermissions.UploadNewPackageVersion, packageRegistration, NuGetScopes.PackagePushVersion, NuGetScopes.PackagePush); + owner = apiScopeEvaluationResult.Owner; + if (!apiScopeEvaluationResult.IsSuccessful()) { + // User cannot push a package as the current user's scopes does not allow it await AuditingService.SaveAuditRecordAsync( new FailedAuthenticatedOperationAuditRecord( - user.Username, + currentUser.Username, AuditedAuthenticatedOperationAction.PackagePushAttemptByNonOwner, attemptedPackage: new AuditedPackageIdentifier( - id, nuspec.GetVersion().ToNormalizedStringSafe()))); + id, version.ToNormalizedStringSafe()))); - // User cannot push a package as the API key scope does not allow it - return new HttpStatusCodeWithBodyResult(HttpStatusCode.Unauthorized, Strings.ApiKeyNotAuthorized); + return GetHttpResultFromFailedApiScopeEvaluationForPush(apiScopeEvaluationResult, id, version); } if (packageRegistration.IsLocked) @@ -446,7 +446,7 @@ await AuditingService.SaveAuditRecordAsync( } // Check if a particular Id-Version combination already exists. We eventually need to remove this check. - string normalizedVersion = nuspec.GetVersion().ToNormalizedString(); + string normalizedVersion = version.ToNormalizedString(); bool packageExists = packageRegistration.Packages.Any( p => string.Equals( @@ -474,8 +474,8 @@ await AuditingService.SaveAuditRecordAsync( id, packageToPush, packageStreamMetadata, - owner: user, - currentUser: user); + owner, + currentUser); await AutoCuratePackage.ExecuteAsync(package, packageToPush, commitChanges: false); @@ -487,7 +487,7 @@ await AuditingService.SaveAuditRecordAsync( package, uploadStream.AsSeekableStream()); } - + switch (commitResult) { case PackageCommitResult.Success: @@ -515,7 +515,7 @@ await AuditingService.SaveAuditRecordAsync( Url.AccountSettings(relativeUrl: false)); } - TelemetryService.TrackPackagePushEvent(package, user, User.Identity); + TelemetryService.TrackPackagePushEvent(package, currentUser, User.Identity); if (package.SemVerLevelKey == SemVerLevelKey.SemVer2) { @@ -564,11 +564,11 @@ public virtual async Task DeletePackage(string id, string version) HttpStatusCode.NotFound, String.Format(CultureInfo.CurrentCulture, Strings.PackageWithIdAndVersionNotFound, id, version)); } - // Check if API key allows listing/unlisting the current package id - var user = GetCurrentUser(); - if (!HasAnyScopeThatAllows(package.PackageRegistration, NuGetScopes.PackageUnlist)) + // Check if the current user's scopes allow listing/unlisting the current package ID + var apiScopeEvaluationResult = EvaluateApiScope(ActionsRequiringPermissions.UnlistOrRelistPackage, package.PackageRegistration, NuGetScopes.PackageUnlist); + if (!apiScopeEvaluationResult.IsSuccessful()) { - return new HttpStatusCodeWithBodyResult(HttpStatusCode.Forbidden, Strings.ApiKeyNotAuthorized); + return GetHttpResultFromFailedApiScopeEvaluation(apiScopeEvaluationResult, id, version); } if (package.PackageRegistration.IsLocked) @@ -596,11 +596,11 @@ public virtual async Task PublishPackage(string id, string version HttpStatusCode.NotFound, String.Format(CultureInfo.CurrentCulture, Strings.PackageWithIdAndVersionNotFound, id, version)); } - // Check if API key allows listing/unlisting the current package id - User user = GetCurrentUser(); - if (!HasAnyScopeThatAllows(package.PackageRegistration, NuGetScopes.PackageUnlist)) + // Check if the current user's scopes allow listing/unlisting the current package ID + var apiScopeEvaluationResult = EvaluateApiScope(ActionsRequiringPermissions.UnlistOrRelistPackage, package.PackageRegistration, NuGetScopes.PackageUnlist); + if (!apiScopeEvaluationResult.IsSuccessful()) { - return new HttpStatusCodeWithBodyResult(HttpStatusCode.Forbidden, Strings.ApiKeyNotAuthorized); + return GetHttpResultFromFailedApiScopeEvaluation(apiScopeEvaluationResult, id, version); } if (package.PackageRegistration.IsLocked) @@ -729,42 +729,60 @@ public virtual async Task GetStatsDownloads(int? count) return new HttpStatusCodeResult(HttpStatusCode.NotFound); } - private bool HasAnyScopeThatAllows(PackageRegistration package, params string[] requestedActions) + private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluation(ApiScopeEvaluationResult evaluationResult, string id, string version) { - var scopes = User.Identity.GetScopesFromClaim(); - if (scopes != null) - { - if (!scopes.Any(s => s.AllowsSubject(package.Id) && s.AllowsActions(requestedActions))) - { - // Subject (package id) or action scopes do not match. - return false; - } - - if (scopes.Any(s => s.HasOwnerScope())) - { - // ApiKeyHandler has already verified that the current user matches the owner scope. - // Do not need to check organization role (IsAdmin) which is covered by the action scope. - return GetCurrentUser().IsOwnerOrMemberOfOrganizationOwner(package); - } - } + return GetHttpResultFromFailedApiScopeEvaluation(evaluationResult, id, NuGetVersion.Parse(version)); + } - // Legacy V1 API key (no scopes), or Legacy V2 API key (no owner scope). - // Must verify that the current user is the package owner or admin for an organization owner. - return GetCurrentUser().IsOwnerOrMemberOfOrganizationOwner(package); + private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluation(ApiScopeEvaluationResult result, string id, NuGetVersion version) + { + return GetHttpResultFromFailedApiScopeEvaluationHelper(result, id, version, HttpStatusCode.Forbidden); + } + + /// + /// Push returns instead of for failures not related to reserved namespaces. + /// This is inconsistent with both the rest of our API and the HTTP standard, but it is an existing behavior that we must support. + /// + private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluationForPush(ApiScopeEvaluationResult result, string id, NuGetVersion version) + { + return GetHttpResultFromFailedApiScopeEvaluationHelper(result, id, version, HttpStatusCode.Unauthorized); } - private bool HasAnyScopeThatAllowsPushNew(string packageId) + private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluationHelper(ApiScopeEvaluationResult result, string id, NuGetVersion version, HttpStatusCode statusCodeOnFailure) { - // Package owners not populated yet, so only verify the scope subject and action. - var scopes = User.Identity.GetScopesFromClaim(); - if (scopes != null) + if (result.IsSuccessful()) { - // Return true if scope allows action for subject (package). - return scopes.Any(s => s.AllowsActions(NuGetScopes.PackagePush) && s.AllowsSubject(packageId)); + throw new ArgumentException($"{nameof(result)} is not a failed evaluation!", nameof(result)); } - // Legacy V1 API key (no scopes). - return true; + if (result.PermissionsCheckResult == PermissionsCheckResult.ReservedNamespaceFailure) + { + // We return a special error code for reserved namespace failures. + TelemetryService.TrackPackagePushNamespaceConflictEvent(id, version.ToNormalizedString(), GetCurrentUser(), User.Identity); + return new HttpStatusCodeWithBodyResult(HttpStatusCode.Conflict, Strings.UploadPackage_IdNamespaceConflict); + } + + return new HttpStatusCodeWithBodyResult(statusCodeOnFailure, Strings.ApiKeyNotAuthorized); + } + + private ApiScopeEvaluationResult EvaluateApiScope(IActionRequiringEntityPermissions action, PackageRegistration packageRegistration, params string[] requestedActions) + { + return ApiScopeEvaluator.Evaluate( + GetCurrentUser(), + User.Identity.GetScopesFromClaim(), + action, + packageRegistration, + requestedActions); + } + + private ApiScopeEvaluationResult EvaluateApiScope(IActionRequiringEntityPermissions action, ActionOnNewPackageContext context, params string[] requestedActions) + { + return ApiScopeEvaluator.Evaluate( + GetCurrentUser(), + User.Identity.GetScopesFromClaim(), + action, + context, + requestedActions); } } } diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index 3eb23a8ec4..8c93995f5d 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -725,8 +725,11 @@ + + + diff --git a/src/NuGetGallery/Services/ActionsRequiringPermissions.cs b/src/NuGetGallery/Services/ActionsRequiringPermissions.cs index 081484cc1c..3aa0e72814 100644 --- a/src/NuGetGallery/Services/ActionsRequiringPermissions.cs +++ b/src/NuGetGallery/Services/ActionsRequiringPermissions.cs @@ -40,6 +40,14 @@ public static class ActionsRequiringPermissions accountOnBehalfOfPermissionsRequirement: RequireOwnerOrOrganizationMember, packageRegistrationPermissionsRequirement: PermissionsRequirement.Owner); + /// + /// The action of verify a package verification key. + /// + public static ActionRequiringPackagePermissions VerifyPackage = + new ActionRequiringPackagePermissions( + accountOnBehalfOfPermissionsRequirement: RequireOwnerOrOrganizationMember, + packageRegistrationPermissionsRequirement: PermissionsRequirement.Owner); + /// /// The action of editing an existing version of an existing package ID. /// diff --git a/src/NuGetGallery/Services/IUserService.cs b/src/NuGetGallery/Services/IUserService.cs index 07dad25772..1a4a0164a5 100644 --- a/src/NuGetGallery/Services/IUserService.cs +++ b/src/NuGetGallery/Services/IUserService.cs @@ -18,6 +18,8 @@ public interface IUserService User FindByUsername(string username); + User FindByKey(int key); + Task ConfirmEmailAddress(User user, string token); Task ChangeEmailAddress(User user, string newEmailAddress); diff --git a/src/NuGetGallery/Services/PermissionsCheckResult.cs b/src/NuGetGallery/Services/PermissionsCheckResult.cs index 891ae51376..1f63beef6e 100644 --- a/src/NuGetGallery/Services/PermissionsCheckResult.cs +++ b/src/NuGetGallery/Services/PermissionsCheckResult.cs @@ -24,12 +24,12 @@ public enum PermissionsCheckResult AccountFailure, /// - /// The current user does not have permissions to perform the action on the on behalf of another . + /// The current user does not have permissions to perform the action on the on behalf of another . /// PackageRegistrationFailure, /// - /// The current user does not have permissions to perform the action on the on behalf of another . + /// The current user does not have permissions to perform the action on the on behalf of another . /// ReservedNamespaceFailure } diff --git a/src/NuGetGallery/Services/UserService.cs b/src/NuGetGallery/Services/UserService.cs index 9f470d169d..baec9a5569 100644 --- a/src/NuGetGallery/Services/UserService.cs +++ b/src/NuGetGallery/Services/UserService.cs @@ -94,6 +94,14 @@ public virtual User FindByUsername(string username) .SingleOrDefault(u => u.Username == username); } + public virtual User FindByKey(int key) + { + return UserRepository.GetAll() + .Include(u => u.Roles) + .Include(u => u.Credentials) + .SingleOrDefault(u => u.Key == key); + } + public async Task ChangeEmailAddress(User user, string newEmailAddress) { var existingUsers = FindAllByEmailAddress(newEmailAddress); @@ -213,4 +221,4 @@ public async Task TransformUserToOrganization(User accountToTransform, Use return await EntitiesContext.TransformUserToOrganization(accountToTransform, adminUser, token); } } -} +} \ No newline at end of file diff --git a/tests/NuGetGallery.Facts/Authentication/ApiScopeEvaluationResultFacts.cs b/tests/NuGetGallery.Facts/Authentication/ApiScopeEvaluationResultFacts.cs new file mode 100644 index 0000000000..107f3b624e --- /dev/null +++ b/tests/NuGetGallery.Facts/Authentication/ApiScopeEvaluationResultFacts.cs @@ -0,0 +1,47 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using NuGetGallery.Framework; +using System; +using System.Collections.Generic; +using System.Linq; +using Xunit; + +namespace NuGetGallery.Authentication +{ + public class ApiScopeEvaluationResultFacts + { + public class TheIsSuccessfulMethod + { + public static IEnumerable AllPossible_Data + { + get + { + foreach (var result in Enum.GetValues(typeof(PermissionsCheckResult)).Cast()) + { + foreach (var user in new[] { null, new User("test") { Key = 1 } }) + { + foreach (var scopesAreValid in new[] { false, true }) + { + yield return MemberDataHelper.AsData( + scopesAreValid, + result, + user, + scopesAreValid && result == PermissionsCheckResult.Allowed); + } + } + } + } + } + + [Theory] + [MemberData(nameof(AllPossible_Data))] + public void ReturnsExpected(bool scopesAreValid, PermissionsCheckResult result, User owner, bool expectedIsSuccessful) + { + var apiScopeEvaluationResult = new ApiScopeEvaluationResult(owner, result, scopesAreValid); + + Assert.Equal(expectedIsSuccessful, apiScopeEvaluationResult.IsSuccessful()); + } + } + } +} diff --git a/tests/NuGetGallery.Facts/Authentication/ApiScopeEvaluatorFacts.cs b/tests/NuGetGallery.Facts/Authentication/ApiScopeEvaluatorFacts.cs new file mode 100644 index 0000000000..676b5b27b0 --- /dev/null +++ b/tests/NuGetGallery.Facts/Authentication/ApiScopeEvaluatorFacts.cs @@ -0,0 +1,225 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Moq; +using NuGetGallery.Framework; +using System; +using System.Collections.Generic; +using System.Linq; +using Xunit; + +namespace NuGetGallery.Authentication +{ + public class ApiScopeEvaluatorFacts + { + public class TheEvaluateMethod + { + private const string DefaultSubject = "a"; + + private static Func DefaultGetSubjectFromEntity = (e) => DefaultSubject; + + private static Func CreateGetSubjectFromEntity(string subject) + { + return (e) => subject; + } + + private ApiScopeEvaluator Setup(Mock mockUserService = null) + { + if (mockUserService == null) + { + mockUserService = new Mock(); + } + + return new ApiScopeEvaluator(mockUserService.Object); + } + + private void AssertResult(ApiScopeEvaluationResult result, User owner, PermissionsCheckResult permissionsCheckResult, bool scopesAreValid) + { + Assert.Equal(scopesAreValid, result.ScopesAreValid); + Assert.Equal(permissionsCheckResult, result.PermissionsCheckResult); + Assert.True(owner.MatchesUser(result.Owner)); + } + + private void AssertScopesNotValidResult(ApiScopeEvaluationResult result) + { + AssertResult(result, owner: null, permissionsCheckResult: PermissionsCheckResult.Unknown, scopesAreValid: false); + } + + [Fact] + public void ReturnsForbiddenWhenSubjectIsNotAllowedByScope() + { + // Arrange + var scope = new Scope("notallowed", null); + + var apiScopeEvaluator = Setup(); + + // Act + var result = apiScopeEvaluator.Evaluate(null, new[] { scope }, null, null, DefaultGetSubjectFromEntity, null); + + // Assert + AssertScopesNotValidResult(result); + } + + [Fact] + public void ReturnsForbiddenWhenActionIsNotAllowedByScope() + { + // Arrange + var scope = new Scope(NuGetPackagePattern.AllInclusivePattern, NuGetScopes.PackagePush); + + var apiScopeEvaluator = Setup(); + + // Act + var result = apiScopeEvaluator.Evaluate(null, new[] { scope }, null, null, DefaultGetSubjectFromEntity, NuGetScopes.PackagePushVersion); + + + // Assert + AssertScopesNotValidResult(result); + } + + public static IEnumerable EvaluatesNoScopesAsAllInclusive_Data + { + get + { + yield return MemberDataHelper.AsData((IEnumerable) null); + yield return MemberDataHelper.AsData(Enumerable.Empty()); + } + } + + [Theory] + [MemberData(nameof(EvaluatesNoScopesAsAllInclusive_Data))] + public void EvaluatesNoScopesAsLegacy(IEnumerable scopes) + { + // Arrange + // A Legacy API Key has + // + // 1 - an all-inclusive subject scope + // 2 - an all-inclusive action scope + // 3 - a "current user" owner scope + + var evaluator = Setup(); + + // Act + // To guarantee that the scope is evaluated with an all-inclusive subject scope, we must test it on two subjects that are COMPLETELY different. + // For example, if subjects "a" and "ab" are approved, the subject scope could be "a*". However, if subjects "a" and "b" are approved, the subject scope must be "*", which is what we expect for no scopes. + foreach (var subject in new[] { "a", "b" }) + { + EvaluatesNoScopesAsAllInclusive(evaluator, scopes, subject); + } + } + + private void EvaluatesNoScopesAsAllInclusive(ApiScopeEvaluator evaluator, IEnumerable scopes, string subject) + { + var currentUser = new User { Key = 1 }; + var action = new TestableActionRequiringEntityPermissions(PermissionsRequirement.None, (u, e) => PermissionsCheckResult.Allowed); + var result = evaluator.Evaluate(currentUser, scopes, action, null, CreateGetSubjectFromEntity(subject), NuGetScopes.All); + + AssertResult(result, currentUser, PermissionsCheckResult.Allowed, scopesAreValid: true); + } + + public static IEnumerable ReturnsCorrectPermissionsCheckResultWhenSubjectAndActionMatches_Data + { + get + { + return Enum.GetValues(typeof(PermissionsCheckResult)).Cast().Select(r => new object[] { r }); + } + } + + [Theory] + [MemberData(nameof(ReturnsCorrectPermissionsCheckResultWhenSubjectAndActionMatches_Data))] + public void ReturnsCorrectPermissionsCheckResultWhenSubjectAndActionMatches(PermissionsCheckResult permissionsCheckResult) + { + var scope = new Scope(NuGetPackagePattern.AllInclusivePattern, NuGetScopes.All); + + ReturnsCorrectFailureResult(permissionsCheckResult, new[] { scope }); + } + + [Theory] + [MemberData(nameof(ReturnsCorrectPermissionsCheckResultWhenSubjectAndActionMatches_Data))] + public void WithMultipleScopesReturnsCorrectFailureResult(PermissionsCheckResult permissionsCheckResult) + { + var scopes = new[] + { + new Scope(DefaultSubject, null), + new Scope(NuGetPackagePattern.AllInclusivePattern, NuGetScopes.All), + new Scope(DefaultSubject, null) + }; + + ReturnsCorrectFailureResult(permissionsCheckResult, scopes); + } + + private void ReturnsCorrectFailureResult(PermissionsCheckResult permissionsCheckResult, IEnumerable scopes) + { + // Arrange + var user = new User("test") { Key = 1 }; + + var testableActionMock = new Mock>(); + testableActionMock + .Setup(a => a.CheckPermissions(user, user, It.IsAny())) + .Returns(permissionsCheckResult); + + var mockUserService = new Mock(); + mockUserService.Setup(x => x.FindByKey(user.Key)).Returns(user); + + var apiScopeEvaluator = Setup(mockUserService); + + // Act + var result = apiScopeEvaluator.Evaluate(user, scopes, testableActionMock.Object, null, DefaultGetSubjectFromEntity, NuGetScopes.All); + + // Assert + AssertResult(result, user, permissionsCheckResult, scopesAreValid: true); + } + + [Fact] + public void WithMultipleScopesReturnsCorrectOwners() + { + // Arrange + var currentUser = new User("test"); + var scopeUser = new User("scope1") { Key = 1 }; + + var scopes = new[] + { + new Scope(scopeUser.Key, "wrongsubject", "wrongaction"), + new Scope(scopeUser.Key, NuGetPackagePattern.AllInclusivePattern, NuGetScopes.All), + new Scope(scopeUser.Key, NuGetPackagePattern.AllInclusivePattern, NuGetScopes.All) + }; + + var permissionsCheckResult = PermissionsCheckResult.Allowed; + var testableActionMock = new Mock>(); + testableActionMock + .Setup(a => a.CheckPermissions(currentUser, It.IsAny(), It.IsAny())) + .Returns(permissionsCheckResult); + + var mockUserService = new Mock(); + mockUserService.Setup(u => u.FindByKey(scopeUser.Key)).Returns(scopeUser); + + var apiScopeEvaluator = Setup(mockUserService); + + // Act + var result = apiScopeEvaluator.Evaluate(currentUser, scopes, testableActionMock.Object, null, DefaultGetSubjectFromEntity, NuGetScopes.All); + + // Assert + AssertResult(result, scopeUser, permissionsCheckResult, scopesAreValid: true); + } + + [Fact] + public void ThrowsIfMultipleOwnerScopes() + { + // Arrange + var scopes = + new[] { 535, 212, 6534 } + .Select(k => new Scope(k, NuGetPackagePattern.AllInclusivePattern, NuGetScopes.All)); + + var permissionsCheckResult = PermissionsCheckResult.Allowed; + var testableActionMock = new Mock>(); + testableActionMock + .Setup(a => a.CheckPermissions(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(permissionsCheckResult); + + var apiScopeEvaluator = Setup(); + + // Act/Assert + Assert.Throws(() => apiScopeEvaluator.Evaluate(null, scopes, testableActionMock.Object, null, DefaultGetSubjectFromEntity, NuGetScopes.All)); + } + } + } +} diff --git a/tests/NuGetGallery.Facts/Authentication/TestCredentialHelper.cs b/tests/NuGetGallery.Facts/Authentication/TestCredentialHelper.cs index 6dcc792416..a2bd542c3a 100644 --- a/tests/NuGetGallery.Facts/Authentication/TestCredentialHelper.cs +++ b/tests/NuGetGallery.Facts/Authentication/TestCredentialHelper.cs @@ -47,15 +47,21 @@ public static Credential CreateV4ApiKey(TimeSpan? expiration, out string plainte return CreateApiKey(CredentialTypes.ApiKey.V4, apiKey.HashedApiKey, expiration); } + public static Credential WithScopes(this Credential credential, ICollection scopes) + { + credential.Scopes = scopes; + return credential; + } + public static Credential WithDefaultScopes(this Credential credential) { - credential.Scopes = new List() { + var scopes = new[] { new Scope("*", NuGetScopes.PackageUnlist), new Scope("*", NuGetScopes.PackagePush), new Scope("*", NuGetScopes.PackagePushVersion) }; - return credential; + return credential.WithScopes(scopes); } public static Credential CreateV2VerificationApiKey(Guid apiKey) diff --git a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs index 7a9273b47b..0c18a8a993 100644 --- a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs @@ -8,19 +8,20 @@ using System.Globalization; using System.IO; using System.Linq; +using System.Linq.Expressions; using System.Net; using System.Threading.Tasks; using System.Web; using System.Web.Mvc; using System.Web.Routing; using Moq; -using Newtonsoft.Json; using Newtonsoft.Json.Linq; using NuGet.Packaging; using NuGetGallery.Auditing; using NuGetGallery.Authentication; using NuGetGallery.Configuration; using NuGetGallery.Framework; +using NuGetGallery.Infrastructure; using NuGetGallery.Infrastructure.Authentication; using NuGetGallery.Packaging; using NuGetGallery.Security; @@ -31,6 +32,7 @@ namespace NuGetGallery internal class TestableApiController : ApiController { + public Mock MockApiScopeEvaluator { get; private set; } public Mock MockEntitiesContext { get; private set; } public Mock MockPackageService { get; private set; } public Mock MockPackageFileService { get; private set; } @@ -54,6 +56,7 @@ public TestableApiController( MockBehavior behavior = MockBehavior.Default) { SetOwinContextOverride(Fakes.CreateOwinContext()); + ApiScopeEvaluator = (MockApiScopeEvaluator = new Mock()).Object; EntitiesContext = (MockEntitiesContext = new Mock()).Object; PackageService = (MockPackageService = new Mock(behavior)).Object; UserService = (MockUserService = new Mock(behavior)).Object; @@ -67,6 +70,8 @@ public TestableApiController( ReservedNamespaceService = (MockReservedNamespaceService = new Mock()).Object; PackageUploadService = (MockPackageUploadService = new Mock()).Object; + SetupApiScopeEvaluatorOnAllInputs(); + CredentialBuilder = new CredentialBuilder(); MockPackageFileService = new Mock(MockBehavior.Strict); @@ -113,6 +118,17 @@ public TestableApiController( TestUtility.SetupHttpContextMockForUrlGeneration(httpContextMock, this); } + private void SetupApiScopeEvaluatorOnAllInputs() + { + MockApiScopeEvaluator + .Setup(x => x.Evaluate(It.IsAny(), It.IsAny>(), It.IsAny>(), It.IsAny(), It.IsAny())) + .Returns(() => new ApiScopeEvaluationResult(GetCurrentUser(), PermissionsCheckResult.Allowed, scopesAreValid: true)); + + MockApiScopeEvaluator + .Setup(x => x.Evaluate(It.IsAny(), It.IsAny>(), It.IsAny>(), It.IsAny(), It.IsAny())) + .Returns(() => new ApiScopeEvaluationResult(GetCurrentUser(), PermissionsCheckResult.Allowed, scopesAreValid: true)); + } + internal void SetupPackageFromInputStream(Stream packageStream) { PackageFromInputStream = packageStream; @@ -129,6 +145,27 @@ public class ApiControllerFacts private static readonly Uri HttpRequestUrl = new Uri("http://nuget.org/api/v2/something"); private static readonly Uri HttpsRequestUrl = new Uri("https://nuget.org/api/v2/something"); + public static IEnumerable InvalidScopes_Data + { + get + { + yield return MemberDataHelper.AsData(new ApiScopeEvaluationResult(null, PermissionsCheckResult.Unknown, scopesAreValid: false), HttpStatusCode.Forbidden, Strings.ApiKeyNotAuthorized); + + foreach (var result in Enum.GetValues(typeof(PermissionsCheckResult)).Cast()) + { + if (result == PermissionsCheckResult.Allowed || result == PermissionsCheckResult.Unknown) + { + continue; + } + + var isReservedNamespaceConflict = result == PermissionsCheckResult.ReservedNamespaceFailure; + var statusCode = isReservedNamespaceConflict ? HttpStatusCode.Conflict : HttpStatusCode.Forbidden; + var description = isReservedNamespaceConflict ? Strings.UploadPackage_IdNamespaceConflict : Strings.ApiKeyNotAuthorized; + yield return MemberDataHelper.AsData(new ApiScopeEvaluationResult(null, result, scopesAreValid: true), statusCode, description); + } + } + } + public class TheCreatePackageAction : TestContainer { @@ -307,52 +344,25 @@ public async Task CreatePackageReturns400IfPackageIdIsInvalid(string packageId) ResultAssert.IsStatusCode(result, HttpStatusCode.BadRequest); } - [Fact] - public async Task WillWriteAnAuditRecordIfUserIsNotPackageOwner() - { - // Arrange - var user = new User { EmailAddress = "confirmed@email.com" }; - var packageRegistration = new PackageRegistration(); - packageRegistration.Id = "theId"; - var package = new Package(); - package.PackageRegistration = packageRegistration; - package.Version = "1.0.42"; - packageRegistration.Packages.Add(package); - - var controller = new TestableApiController(GetConfigurationService()); - controller.SetCurrentUser(user); - controller.MockPackageService.Setup(p => p.FindPackageRegistrationById(It.IsAny())) - .Returns(packageRegistration); - - var nuGetPackage = TestPackage.CreateTestPackageStream("theId", "1.0.42"); - controller.SetupPackageFromInputStream(nuGetPackage); - - // Act - await controller.CreatePackagePut(); - - // Assert - Assert.True(controller.AuditingService.WroteRecord(ar => - ar.Action == AuditedAuthenticatedOperationAction.PackagePushAttemptByNonOwner - && ar.AttemptedPackage.Id == package.PackageRegistration.Id - && ar.AttemptedPackage.Version == package.Version)); - } - [Fact] public async Task WillReturnConflictIfAPackageWithTheIdAndSameNormalizedVersionAlreadyExists() { - var nuGetPackage = TestPackage.CreateTestPackageStream("theId", "1.0.042"); + var id = "theId"; + var version = "1.0.42"; + var nuGetPackage = TestPackage.CreateTestPackageStream(id, version); var user = new User() { EmailAddress = "confirmed@email.com" }; var packageRegistration = new PackageRegistration { - Packages = new List { new Package { Version = "01.00.42", NormalizedVersion = "1.0.42" } }, + Id = id, + Packages = new List { new Package { Version = version, NormalizedVersion = version } }, Owners = new List { user } }; var controller = new TestableApiController(GetConfigurationService()); controller.SetCurrentUser(new User()); - controller.MockPackageService.Setup(x => x.FindPackageRegistrationById("theId")).Returns(packageRegistration); + controller.MockPackageService.Setup(x => x.FindPackageRegistrationById(id)).Returns(packageRegistration); controller.SetupPackageFromInputStream(nuGetPackage); // Act @@ -362,39 +372,7 @@ public async Task WillReturnConflictIfAPackageWithTheIdAndSameNormalizedVersionA ResultAssert.IsStatusCode( result, HttpStatusCode.Conflict, - String.Format(Strings.PackageExistsAndCannotBeModified, "theId", "1.0.42")); - } - - [Fact] - public async Task WillReturnUnauthorizedIfAPackageWithTheIdExistsBelongingToAnotherUser() - { - // Arrange - var user = new User { EmailAddress = "confirmed@email.com" }; - var packageId = "theId"; - var packageRegistration = new PackageRegistration(); - packageRegistration.Id = packageId; - var package = new Package(); - package.PackageRegistration = packageRegistration; - package.Version = "1.0.42"; - packageRegistration.Packages.Add(package); - - var controller = new TestableApiController(GetConfigurationService()); - controller.SetCurrentUser(user); - controller.MockPackageService.Setup(p => p.FindPackageRegistrationById(It.IsAny())) - .Returns(packageRegistration); - - var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, "1.0.42"); - controller.SetCurrentUser(new User()); - controller.SetupPackageFromInputStream(nuGetPackage); - - // Act - var result = await controller.CreatePackagePut(); - - // Assert - ResultAssert.IsStatusCode( - result, - HttpStatusCode.Unauthorized, - String.Format(Strings.ApiKeyNotAuthorized, packageId)); + String.Format(Strings.PackageExistsAndCannotBeModified, id, version)); } [Fact] @@ -456,56 +434,96 @@ public async Task DoesNotThrowForAnyPackageCommitResult(PackageCommitResult comm } [Fact] - public async Task WillReturnConflictIfAPackageWithTheIdMatchesNonOwnedNamespace() + public async Task WillCreateAPackageWithNewRegistration() { - // Arrange - var user1 = new User { Key = 1, Username = "random1" }; - var user2 = new User { Key = 2, Username = "random2" }; - var packageId = "Random.Extention.Package1"; - var packageRegistration = new PackageRegistration(); - packageRegistration.Id = packageId; - var package = new Package(); - package.PackageRegistration = packageRegistration; - package.Version = "1.0.0"; - packageRegistration.Packages.Add(package); + var packageId = "theId"; + var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, "1.0.42"); + var currentUser = new User("currentUser") { Key = 1, EmailAddress = "confirmed@email.com" }; var controller = new TestableApiController(GetConfigurationService()); - controller.SetCurrentUser(user1); - controller.MockPackageService.Setup(p => p.FindPackageRegistrationById(It.IsAny())) - .Returns(() => null); - - var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, "1.0.0"); + controller.SetCurrentUser(currentUser); controller.SetupPackageFromInputStream(nuGetPackage); - var testNamespace = new ReservedNamespace("random.", isSharedNamespace: false, isPrefix: true); - testNamespace.Owners.Add(user2); - IReadOnlyCollection matchingNamespaces = new List { testNamespace }; - controller.MockReservedNamespaceService - .Setup(r => r.GetReservedNamespacesForId(It.IsAny())) - .Returns(matchingNamespaces); - // Act - var result = await controller.CreatePackagePut(); + var owner = new User("owner") { Key = 2 }; - // Assert - ResultAssert.IsStatusCode( - result, - HttpStatusCode.Conflict, - String.Format(Strings.UploadPackage_IdNamespaceConflict)); + Expression> evaluateApiScope = + x => x.Evaluate( + currentUser, + It.IsAny>(), + ActionsRequiringPermissions.UploadNewPackageId, + It.Is((context) => context.PackageId == packageId), + NuGetScopes.PackagePush); + + controller.MockApiScopeEvaluator + .Setup(evaluateApiScope) + .Returns(new ApiScopeEvaluationResult(owner, PermissionsCheckResult.Allowed, scopesAreValid: true)); + + await controller.CreatePackagePut(); - controller.MockTelemetryService.Verify(x => x.TrackPackagePushNamespaceConflictEvent(packageRegistration.Id, package.Version, user1, controller.OwinContext.Request.User.Identity), Times.Once); + controller.MockPackageUploadService.Verify( + x => x.GeneratePackageAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + owner, + currentUser), + Times.Once); + + controller.MockApiScopeEvaluator.Verify(evaluateApiScope); } - [Fact] - public async Task WillCreateAPackageFromTheNuGetPackage() - { - var nuGetPackage = TestPackage.CreateTestPackageStream("theId", "1.0.42"); + /// + /// returns instead of . + /// + public static IEnumerable WillNotCreateAPackageIfScopesInvalid_Data => + InvalidScopes_Data + .Select(x => x + .Select(y => y is HttpStatusCode status && status == HttpStatusCode.Forbidden ? HttpStatusCode.Unauthorized : y) + .ToArray()); - var user = new User() { EmailAddress = "confirmed@email.com" }; + [Theory] + [MemberData(nameof(WillNotCreateAPackageIfScopesInvalid_Data))] + public async Task WillNotCreateAPackageIfScopesInvalidWithNewRegistration(ApiScopeEvaluationResult scopeEvaluationResult, HttpStatusCode expectedStatusCode, string description) + { + // Arrange var controller = new TestableApiController(GetConfigurationService()); - controller.SetCurrentUser(user); + + var fakes = Get(); + var currentUser = fakes.User; + controller.SetCurrentUser(currentUser); + + var packageId = "theId"; + var packageVersion = "1.0.42"; + var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, packageVersion); controller.SetupPackageFromInputStream(nuGetPackage); - await controller.CreatePackagePut(); + controller.MockApiScopeEvaluator + .Setup(x => x.Evaluate( + currentUser, + It.IsAny>(), + ActionsRequiringPermissions.UploadNewPackageId, + It.Is((context) => context.PackageId == packageId), + NuGetScopes.PackagePush)) + .Returns(scopeEvaluationResult); + + // Act + var result = await controller.CreatePackagePut(); + + // Assert + ResultAssert.IsStatusCode( + result, + expectedStatusCode, + description); + + controller.AuditingService.WroteRecord( + (record) => + { + return + record.UsernameOrEmail == currentUser.Username && + record.Action == AuditedAuthenticatedOperationAction.PackagePushAttemptByNonOwner && + record.AttemptedPackage.Id == packageId && + record.AttemptedPackage.Version == packageVersion; + }); controller.MockPackageUploadService.Verify( x => x.GeneratePackageAsync( @@ -514,81 +532,105 @@ public async Task WillCreateAPackageFromTheNuGetPackage() It.IsAny(), It.IsAny(), It.IsAny()), - Times.Once); + Times.Never); } [Fact] - public async Task WillCreatePackageIfIdMatchesSharedNamespace() + public async Task WillCreateAPackageWithExistingRegistration() { - // Arrange - var user1 = new User { Key = 1, Username = "random1" }; - var user2 = new User { Key = 2, Username = "random2" }; - var packageId = "Random.Extention.Package1"; - var packageRegistration = new PackageRegistration(); + var packageId = "theId"; + var packageRegistration = new PackageRegistration { Id = packageId }; packageRegistration.Id = packageId; - var package = new Package(); - package.PackageRegistration = packageRegistration; - package.Version = "1.0.0"; + var package = new Package + { + PackageRegistration = packageRegistration, + Version = "1.0.42" + }; packageRegistration.Packages.Add(package); var controller = new TestableApiController(GetConfigurationService()); - controller.SetCurrentUser(user1); controller.MockPackageService.Setup(p => p.FindPackageRegistrationById(It.IsAny())) - .Returns(() => null); + .Returns(packageRegistration); + + var fakes = Get(); + var currentUser = fakes.User; + controller.SetCurrentUser(currentUser); - var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, "1.0.0"); + var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, "1.0.42"); controller.SetupPackageFromInputStream(nuGetPackage); - var testNamespace = new ReservedNamespace("random.", isSharedNamespace: true, isPrefix: true); - testNamespace.Owners.Add(user2); - IReadOnlyCollection matchingNamespaces = new List { testNamespace }; - controller.MockReservedNamespaceService - .Setup(r => r.GetReservedNamespacesForId(It.IsAny())) - .Returns(matchingNamespaces); - // Act - var result = await controller.CreatePackagePut(); + var owner = new User("owner") { Key = 2 }; + + Expression> evaluateApiScope = + x => x.Evaluate( + currentUser, + It.IsAny>(), + ActionsRequiringPermissions.UploadNewPackageVersion, + packageRegistration, + NuGetScopes.PackagePushVersion, NuGetScopes.PackagePush); + + controller.MockApiScopeEvaluator + .Setup(evaluateApiScope) + .Returns(new ApiScopeEvaluationResult(owner, PermissionsCheckResult.Allowed, scopesAreValid: true)); + + await controller.CreatePackagePut(); - // Assert controller.MockPackageUploadService.Verify( x => x.GeneratePackageAsync( It.IsAny(), It.IsAny(), It.IsAny(), - It.IsAny(), - It.IsAny())); + owner, + currentUser), + Times.Once); + + controller.MockApiScopeEvaluator.Verify(evaluateApiScope); } - [Fact] - public async Task WillCreatePackageIfIdMatchesAnOwnedNamespace() + [Theory] + [MemberData(nameof(WillNotCreateAPackageIfScopesInvalid_Data))] + public async Task WillNotCreateAPackageIfScopesInvalidWithExistingRegistration(ApiScopeEvaluationResult scopeEvaluationResult, HttpStatusCode expectedStatusCode, string description) { // Arrange - var user1 = new User { Key = 1, Username = "random1" }; - var packageId = "Random.Extention.Package1"; - var packageRegistration = new PackageRegistration(); + var packageId = "theId"; + var packageRegistration = new PackageRegistration { Id = packageId }; packageRegistration.Id = packageId; - var package = new Package(); - package.PackageRegistration = packageRegistration; - package.Version = "1.0.0"; + var package = new Package + { + PackageRegistration = packageRegistration, + Version = "1.0.42" + }; packageRegistration.Packages.Add(package); var controller = new TestableApiController(GetConfigurationService()); - controller.SetCurrentUser(user1); controller.MockPackageService.Setup(p => p.FindPackageRegistrationById(It.IsAny())) - .Returns(() => null); + .Returns(packageRegistration); + + var fakes = Get(); + var currentUser = fakes.User; + controller.SetCurrentUser(currentUser); - var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, "1.0.0"); + var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, "1.0.42"); controller.SetupPackageFromInputStream(nuGetPackage); - var testNamespace = new ReservedNamespace("random.", isSharedNamespace: false, isPrefix: true); - testNamespace.Owners.Add(user1); - IReadOnlyCollection matchingNamespaces = new List { testNamespace }; - controller.MockReservedNamespaceService - .Setup(r => r.GetReservedNamespacesForId(It.IsAny())) - .Returns(matchingNamespaces); + + controller.MockApiScopeEvaluator + .Setup(x => x.Evaluate( + currentUser, + It.IsAny>(), + ActionsRequiringPermissions.UploadNewPackageVersion, + packageRegistration, + NuGetScopes.PackagePushVersion, NuGetScopes.PackagePush)) + .Returns(scopeEvaluationResult); // Act var result = await controller.CreatePackagePut(); // Assert + ResultAssert.IsStatusCode( + result, + expectedStatusCode, + description); + controller.MockPackageUploadService.Verify( x => x.GeneratePackageAsync( It.IsAny(), @@ -596,7 +638,7 @@ public async Task WillCreatePackageIfIdMatchesAnOwnedNamespace() It.IsAny(), It.IsAny(), It.IsAny()), - Times.Once); + Times.Never); } [Fact] @@ -666,140 +708,6 @@ public async Task WillCreateAPackageWithTheUserMatchingTheApiKey() Times.Once); } - [InlineData("[{\"a\":\"package:push\", \"s\":\"theId\"}]", true)] - [InlineData("[{\"a\":\"package:push\", \"s\":\"*\"}]", true)] - [InlineData("[{\"a\":\"package:pushversion\", \"s\":\"theId\"}]", false)] - [InlineData("[{\"a\":\"package:push\", \"s\":\"cbd\"}]", false)] - [Theory] - public async Task WillVerifyScopesForNewPackageId(string apiKeyScopes, bool isPushAllowed) - { - // Arrange - var nuGetPackage = TestPackage.CreateTestPackageStream("theId", "1.0.42"); - - var user = new User { EmailAddress = "confirmed@email.com", Username = "username" }; - - var package = new Package(); - package.PackageRegistration = new PackageRegistration(); - package.Version = "1.0.42"; - - var credential = TestCredentialHelper.CreateV4ApiKey(expiration: null, plaintextApiKey: out string plaintextApiKey); - credential.Scopes = JsonConvert.DeserializeObject>(apiKeyScopes); - - var controller = new TestableApiController(GetConfigurationService()); - controller.SetCurrentUser(user, credential); - controller.SetupPackageFromInputStream(nuGetPackage); - controller.MockPackageUploadService - .Setup(x => x.GeneratePackageAsync( - It.IsAny(), - It.IsAny(), - It.IsAny(), - user, - user)) - .ReturnsAsync(package); - - // Act - var result = await controller.CreatePackagePut(); - - // Assert - if (isPushAllowed) - { - controller.MockPackageUploadService.Verify( - x => x.GeneratePackageAsync( - It.IsAny(), - It.IsAny(), - It.IsAny(), - user, - user)); - } - else - { - controller.MockPackageUploadService.Verify( - x => x.GeneratePackageAsync( - It.IsAny(), - It.IsAny(), - It.IsAny(), - It.IsAny(), - It.IsAny()), - Times.Never); - - ResultAssert.IsStatusCode( - result, - HttpStatusCode.Unauthorized, - Strings.ApiKeyNotAuthorized); - } - } - - [InlineData("[{\"a\":\"package:pushversion\", \"s\":\"differentid\"}]", false)] - [InlineData("[{\"a\":\"package:push\", \"s\":\"theId\"}]", true)] - [InlineData("[{\"a\":\"package:pushversion\", \"s\":\"theId\"}]", true)] - [Theory] - public async Task WillVerifyScopesForExistingPackageId(string apiKeyScopes, bool isPushAllowed) - { - // Arrange - const string packageId = "theId"; - - var nuGetPackage = TestPackage.CreateTestPackageStream("theId", "1.0.42"); - - var user = new User { EmailAddress = "confirmed@email.com", Username = "username", Key = 1 }; - - var credential = TestCredentialHelper.CreateV4ApiKey(expiration: null, plaintextApiKey: out string plaintextApiKey); - credential.Scopes = JsonConvert.DeserializeObject>(apiKeyScopes); - - var controller = new TestableApiController(GetConfigurationService()); - controller.SetCurrentUser(user, credential); - controller.SetupPackageFromInputStream(nuGetPackage); - - var packageRegistration = new PackageRegistration(); - packageRegistration.Id = packageId; - packageRegistration.Owners.Add(user); - - var package = new Package(); - package.PackageRegistration = packageRegistration; - package.Version = "1.0.42"; - - controller.MockPackageService.Setup(x => x.FindPackageRegistrationById(packageId)) - .Returns(packageRegistration); - controller.MockPackageUploadService - .Setup(x => x.GeneratePackageAsync( - It.IsAny(), - It.IsAny(), - It.IsAny(), - user, - user)) - .ReturnsAsync(package); - - // Act - var result = await controller.CreatePackagePut(); - - // Assert - if (isPushAllowed) - { - controller.MockPackageUploadService.Verify( - x => x.GeneratePackageAsync( - It.IsAny(), - It.IsAny(), - It.IsAny(), - user, - user)); - } - else - { - controller.MockPackageUploadService.Verify( - x => x.GeneratePackageAsync( - It.IsAny(), - It.IsAny(), - It.IsAny(), - It.IsAny(), - It.IsAny()), - Times.Never); - - ResultAssert.IsStatusCode( - result, - HttpStatusCode.Unauthorized, - Strings.ApiKeyNotAuthorized); - } - } - [Fact] public async Task WillSendPackagePushEvent() { @@ -866,85 +774,74 @@ public async Task WillThrowIfAPackageWithTheIdAndNuGetVersionDoesNotExist() controller.MockPackageService.Verify(x => x.MarkPackageUnlistedAsync(It.IsAny(), true), Times.Never()); } - [Fact] - public async Task WillNotDeleteThePackageIfApiKeyDoesNotBelongToAnOwner() - { - var notOwner = new User { Key = 1 }; - var package = new Package - { - PackageRegistration = new PackageRegistration { Owners = new[] { new User() } } - }; - - var controller = new TestableApiController(GetConfigurationService()); - controller.SetCurrentUser(notOwner); - controller.MockPackageService.Setup(x => x.FindPackageByIdAndVersionStrict("theId", "1.0.42")).Returns(package); - - var result = await controller.DeletePackage("theId", "1.0.42"); - - Assert.IsType(result); - var statusCodeResult = (HttpStatusCodeWithBodyResult)result; - Assert.Equal(string.Format(Strings.ApiKeyNotAuthorized, "delete"), statusCodeResult.StatusDescription); - - controller.MockPackageService.Verify(x => x.MarkPackageUnlistedAsync(package, true), Times.Never()); - } + public static IEnumerable WillNotUnlistThePackageIfScopesInvalid_Data => InvalidScopes_Data; - [InlineData("[{\"a\":\"all\", \"s\":\"*\"}]", true)] - [InlineData("[{\"a\":\"package:unlist\", \"s\":\"theId\"}]", true)] - [InlineData("[{\"a\":\"package:push\", \"s\":\"theId\"}]", false)] [Theory] - public async Task WillVerifyApiKeyScopeBeforeDelete(string apiKeyScope, bool isDeleteAllowed) + [MemberData(nameof(WillNotUnlistThePackageIfScopesInvalid_Data))] + public async Task WillNotUnlistThePackageIfScopesInvalid(ApiScopeEvaluationResult evaluationResult, HttpStatusCode expectedStatusCode, string description) { - var owner = new User { Key = 1, Username = "owner" }; + var fakes = Get(); + var currentUser = fakes.User; + + var id = "theId"; var package = new Package { - PackageRegistration = new PackageRegistration { - Id = "theId", - Owners = new[] { owner } - } + PackageRegistration = new PackageRegistration { Id = id } }; - var credential = TestCredentialHelper.CreateV4ApiKey(expiration: null, plaintextApiKey: out string plaintextApiKey); - credential.Scopes = JsonConvert.DeserializeObject>(apiKeyScope); - var controller = new TestableApiController(GetConfigurationService()); - controller.SetCurrentUser(owner, credential); - controller.MockPackageService.Setup(x => x.FindPackageByIdAndVersionStrict("theId", "1.0.42")) - .Returns(package); + controller.MockPackageService.Setup(x => x.FindPackageByIdAndVersionStrict(It.IsAny(), It.IsAny())).Returns(package); + + controller.SetCurrentUser(currentUser); + + controller.MockApiScopeEvaluator + .Setup(x => x.Evaluate( + currentUser, + It.IsAny>(), + ActionsRequiringPermissions.UnlistOrRelistPackage, + package.PackageRegistration, + NuGetScopes.PackageUnlist)) + .Returns(evaluationResult); var result = await controller.DeletePackage("theId", "1.0.42"); - if (!isDeleteAllowed) - { - Assert.IsType(result); - var statusCodeResult = (HttpStatusCodeWithBodyResult)result; - Assert.Equal(string.Format(Strings.ApiKeyNotAuthorized, "delete"), - statusCodeResult.StatusDescription); + ResultAssert.IsStatusCode( + result, + expectedStatusCode, + description); - controller.MockPackageService.Verify(x => x.MarkPackageUnlistedAsync(package, true), Times.Never()); - } - else - { - controller.MockPackageService.Verify(x => x.MarkPackageUnlistedAsync(package, true)); - controller.MockIndexingService.Verify(i => i.UpdatePackage(package)); - } + controller.MockPackageService.Verify(x => x.MarkPackageUnlistedAsync(package, true), Times.Never()); } [Fact] - public async Task WillUnlistThePackageIfApiKeyBelongsToAnOwner() + public async Task WillUnlistThePackage() { - var owner = new User { Key = 1 }; + var fakes = Get(); + var currentUser = fakes.User; + + var id = "theId"; var package = new Package { - PackageRegistration = new PackageRegistration { Owners = new[] { new User(), owner } } + PackageRegistration = new PackageRegistration { Id = id } }; + var controller = new TestableApiController(GetConfigurationService()); controller.MockPackageService.Setup(x => x.FindPackageByIdAndVersionStrict(It.IsAny(), It.IsAny())).Returns(package); - controller.SetCurrentUser(owner); - ResultAssert.IsEmpty(await controller.DeletePackage("theId", "1.0.42")); + controller.SetCurrentUser(currentUser); + + ResultAssert.IsEmpty(await controller.DeletePackage(id, "1.0.42")); controller.MockPackageService.Verify(x => x.MarkPackageUnlistedAsync(package, true)); controller.MockIndexingService.Verify(i => i.UpdatePackage(package)); + + controller.MockApiScopeEvaluator + .Verify(x => x.Evaluate( + currentUser, + It.IsAny>(), + ActionsRequiringPermissions.UnlistOrRelistPackage, + package.PackageRegistration, + NuGetScopes.PackageUnlist)); } [Fact] @@ -1208,53 +1105,74 @@ public async Task WillThrowIfAPackageWithTheIdAndNuGetVersionDoesNotExist() controller.MockPackageService.Verify(x => x.MarkPackageListedAsync(It.IsAny(), It.IsAny()), Times.Never()); } - [Fact] - public async Task WillNotListThePackageIfApiKeyDoesNotBelongToAnOwner() + public static IEnumerable WillListThePackageIfScopesInvalid_Data => InvalidScopes_Data; + + [Theory] + [MemberData(nameof(WillListThePackageIfScopesInvalid_Data))] + public async Task WillListThePackageIfScopesInvalid(ApiScopeEvaluationResult evaluationResult, HttpStatusCode expectedStatusCode, string description) { - // Arrange - var owner = new User { Key = 1 }; + var fakes = Get(); + var currentUser = fakes.User; + + var id = "theId"; var package = new Package { - PackageRegistration = new PackageRegistration { Owners = new[] { new User() } } + PackageRegistration = new PackageRegistration { Id = id } }; var controller = new TestableApiController(GetConfigurationService()); - controller.MockPackageService.Setup(x => x.FindPackageByIdAndVersionStrict("theId", "1.0.42")).Returns(package); - controller.SetCurrentUser(owner); + controller.MockPackageService.Setup(x => x.FindPackageByIdAndVersionStrict(It.IsAny(), It.IsAny())).Returns(package); + + controller.SetCurrentUser(currentUser); + + controller.MockApiScopeEvaluator + .Setup(x => x.Evaluate( + currentUser, + It.IsAny>(), + ActionsRequiringPermissions.UnlistOrRelistPackage, + package.PackageRegistration, + NuGetScopes.PackageUnlist)) + .Returns(evaluationResult); - // Act var result = await controller.PublishPackage("theId", "1.0.42"); - // Assert ResultAssert.IsStatusCode( result, - HttpStatusCode.Forbidden, - String.Format(Strings.ApiKeyNotAuthorized, "publish")); + expectedStatusCode, + description); - controller.MockPackageService.Verify(x => x.MarkPackageListedAsync(package, It.IsAny()), Times.Never()); + controller.MockPackageService.Verify(x => x.MarkPackageListedAsync(package, true), Times.Never()); } [Fact] - public async Task WillListThePackageIfUserIsAnOwner() + public async Task WillListThePackage() { - // Arrange - var owner = new User { Key = 1 }; + var fakes = Get(); + var currentUser = fakes.User; + + var id = "theId"; var package = new Package { - PackageRegistration = new PackageRegistration { Owners = new[] { new User(), owner } } + PackageRegistration = new PackageRegistration { Id = id } }; var controller = new TestableApiController(GetConfigurationService()); controller.MockPackageService.Setup(x => x.FindPackageByIdAndVersionStrict(It.IsAny(), It.IsAny())).Returns(package); - controller.SetCurrentUser(owner); - // Act - var result = await controller.PublishPackage("theId", "1.0.42"); + controller.SetCurrentUser(currentUser); + + ResultAssert.IsEmpty(await controller.PublishPackage("theId", "1.0.42")); - // Assert - ResultAssert.IsEmpty(result); - controller.MockPackageService.Verify(x => x.MarkPackageListedAsync(package, It.IsAny())); + controller.MockPackageService.Verify(x => x.MarkPackageListedAsync(package, true)); controller.MockIndexingService.Verify(i => i.UpdatePackage(package)); + + controller.MockApiScopeEvaluator + .Verify(x => x.Evaluate( + currentUser, + It.IsAny>(), + ActionsRequiringPermissions.UnlistOrRelistPackage, + package.PackageRegistration, + NuGetScopes.PackageUnlist)); } [Fact] @@ -1297,17 +1215,23 @@ public async Task WillNotListThePackageIfItIsLocked() public class PackageVerificationKeyContainer : TestContainer { - internal TestableApiController SetupController(string keyType, string scopes, Package package, bool isOwner = true) - { - var fakes = Get(); - var user = fakes.User; - var credential = user.Credentials.First(c => c.Type == keyType); + public static int UserKey = 1234; + public static string Username = "testuser"; + public static string PackageId = "foo"; + public static string PackageVersion = "1.0.0"; - if (!string.IsNullOrWhiteSpace(scopes)) + internal TestableApiController SetupController(string keyType, Scope scope, Package package, bool isOwner = true) + { + var credential = new Credential(keyType, string.Empty, TimeSpan.FromDays(1)); + if (scope != null) { - credential.Scopes = JsonConvert.DeserializeObject>(scopes); + credential.Scopes.Add(scope); } + var user = Get().CreateUser(Username); + user.Key = UserKey; + user.Credentials.Add(credential); + if (package != null && isOwner) { package.PackageRegistration.Owners.Add(user); @@ -1325,12 +1249,16 @@ internal TestableApiController SetupController(string keyType, string scopes, Pa .Callback((u, c) => u.Credentials.Remove(c)) .Returns(Task.CompletedTask); - var id = package?.PackageRegistration?.Id ?? "foo"; - var version = package?.Version ?? "1.0.0"; + var id = package?.PackageRegistration?.Id ?? PackageId; + var version = package?.Version ?? PackageVersion; controller.MockPackageService .Setup(s => s.FindPackageByIdAndVersion(id, version, SemVerLevelKey.SemVer2, true)) .Returns(package); + controller.MockUserService + .Setup(x => x.FindByKey(user.Key)) + .Returns(user); + controller.SetCurrentUser(user, credential); return controller; @@ -1343,44 +1271,56 @@ public class TheCreatePackageVerificationKeyAsyncAction [Fact] public async Task WhenApiKeyHasNoScope_TempKeyHasScopeWithNoOwner() { - var tempScope = await InvokeAsync(""); + var tempScope = await InvokeAsync(null); Assert.Null(tempScope.OwnerKey); - Assert.Equal("foo", tempScope.Subject); + Assert.Equal(PackageId, tempScope.Subject); Assert.Equal(NuGetScopes.PackageVerify, tempScope.AllowedAction); } + public static IEnumerable TempKeyHasScopeWithNoOwner_Data + { + get + { + foreach (var allowedAction in new[] { NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion }) + { + yield return new object[] + { + allowedAction + }; + } + } + } + [Theory] - [InlineData("[{\"a\":\"package:push\", \"s\":\"foo\"}]")] - [InlineData("[{\"a\":\"package:pushversion\", \"s\":\"foo\"}]")] - public async Task WhenApiKeyHasNoOwnerScope_TempKeyHasScopeWithNoOwner(string scope) + [MemberData(nameof(TempKeyHasScopeWithNoOwner_Data))] + public async Task WhenApiKeyHasNoOwnerScope_TempKeyHasScopeWithNoOwner(string allowedAction) { - var tempScope = await InvokeAsync(scope); + var tempScope = await InvokeAsync(new Scope() { OwnerKey = null, Subject = PackageId, AllowedAction = allowedAction }); Assert.Null(tempScope.OwnerKey); - Assert.Equal("foo", tempScope.Subject); + Assert.Equal(PackageId, tempScope.Subject); Assert.Equal(NuGetScopes.PackageVerify, tempScope.AllowedAction); } [Theory] - [InlineData("[{\"o\": \"1234\", \"a\":\"package:push\", \"s\":\"foo\"}]")] - [InlineData("[{\"o\": \"1234\", \"a\":\"package:pushversion\", \"s\":\"foo\"}]")] - public async Task WhenApiKeyHasOwnerScope_TempKeyHasSameOwner(string scope) + [MemberData(nameof(TempKeyHasScopeWithNoOwner_Data))] + public async Task WhenApiKeyHasOwnerScope_TempKeyHasSameOwner(string allowedAction) { - var tempScope = await InvokeAsync(scope); + var tempScope = await InvokeAsync(new Scope() { OwnerKey = 1234, Subject = PackageId, AllowedAction = allowedAction }); Assert.Equal(1234, tempScope.OwnerKey); - Assert.Equal("foo", tempScope.Subject); + Assert.Equal(PackageId, tempScope.Subject); Assert.Equal(NuGetScopes.PackageVerify, tempScope.AllowedAction); } - private async Task InvokeAsync(string scope) + private async Task InvokeAsync(Scope scope) { // Arrange var controller = SetupController(CredentialTypes.ApiKey.V4, scope, package: null); // Act - var jsonResult = await controller.CreatePackageVerificationKeyAsync("foo", "1.0.0") as JsonResult; + var jsonResult = await controller.CreatePackageVerificationKeyAsync(PackageId, PackageVersion) as JsonResult; // Assert - the response dynamic json = jsonResult?.Data; @@ -1395,9 +1335,9 @@ private async Task InvokeAsync(string scope) // Assert - the invocations controller.MockAuthenticationService.Verify(s => s.AddCredential(It.IsAny(), It.IsAny()), Times.Once); - controller.MockTelemetryService.Verify(x => x.TrackCreatePackageVerificationKeyEvent("foo", "1.0.0", + controller.MockTelemetryService.Verify(x => x.TrackCreatePackageVerificationKeyEvent(PackageId, PackageVersion, It.IsAny(), controller.OwinContext.Request.User.Identity), Times.Once); - + // Assert - the temp key var user = controller.GetCurrentUser(); var tempKey = user.Credentials.Last(); @@ -1410,250 +1350,179 @@ private async Task InvokeAsync(string scope) public class TheVerifyPackageKeyAsyncAction : PackageVerificationKeyContainer { - [Fact] - public async Task VerifyPackageKeyAsync_Returns400IfSecurityPolicyFails() + private static IEnumerable AllCredentialTypes = new[] { - // Arrange - var controller = SetupController(CredentialTypes.ApiKey.V4, "", package: null); - controller.MockSecurityPolicyService.Setup(s => s.EvaluateAsync(It.IsAny(), It.IsAny())) - .Returns(Task.FromResult(SecurityPolicyResult.CreateErrorResult("A"))); + CredentialTypes.ApiKey.V1, + CredentialTypes.ApiKey.V2, + CredentialTypes.ApiKey.V4, + CredentialTypes.ApiKey.VerifyV1 + }; - // Act - var result = await controller.VerifyPackageKeyAsync("foo", "1.0.0"); + private static IEnumerable CredentialTypesExceptVerifyV1 = + AllCredentialTypes + .Except(new[] { CredentialTypes.ApiKey.VerifyV1 }); - // Assert - ResultAssert.IsStatusCode(result, HttpStatusCode.BadRequest, "A"); - } + public static IEnumerable AllCredentialTypes_Data => + AllCredentialTypes + .Select(t => MemberDataHelper.AsData(t)); + + public static IEnumerable CredentialTypesExceptVerifyV1_Data => + CredentialTypesExceptVerifyV1 + .Select(t => MemberDataHelper.AsData(t)); [Theory] - [InlineData(CredentialTypes.ApiKey.V1, "")] - [InlineData(CredentialTypes.ApiKey.V2, "[{\"a\":\"package:push\", \"s\":\"foo\"}]")] - [InlineData(CredentialTypes.ApiKey.V4, "[{\"a\":\"package:pushversion\", \"s\":\"foo\"}]")] - public async Task VerifyPackageKeyAsync_Returns404IfPackageDoesNotExist_ApiKey(string apiKeyType, string scope) + [MemberData(nameof(AllCredentialTypes_Data))] + public async Task Returns400IfSecurityPolicyFails(string credentialType) { // Arrange - var controller = SetupController(apiKeyType, scope, package: null); + var errorResult = "A"; + var controller = SetupController(credentialType, null, package: null); + controller.MockSecurityPolicyService.Setup(s => s.EvaluateAsync(It.IsAny(), It.IsAny())) + .Returns(Task.FromResult(SecurityPolicyResult.CreateErrorResult(errorResult))); // Act - var result = await controller.VerifyPackageKeyAsync("foo", "1.0.0"); + var result = await controller.VerifyPackageKeyAsync(PackageId, PackageVersion); // Assert - ResultAssert.IsStatusCode( - result, - HttpStatusCode.NotFound, - String.Format(CultureInfo.CurrentCulture, Strings.PackageWithIdAndVersionNotFound, "foo", "1.0.0")); - - controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Never); - - controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", - It.IsAny(), controller.OwinContext.Request.User.Identity, 404), Times.Once); + ResultAssert.IsStatusCode(result, HttpStatusCode.BadRequest, errorResult); } [Theory] - [InlineData("[{\"a\":\"package:verify\", \"s\":\"foo\"}]")] - public async Task VerifyPackageKeyAsync_Returns404IfPackageDoesNotExist_ApiKeyVerifyV1(string scope) + [MemberData(nameof(AllCredentialTypes_Data))] + public async Task Returns404IfPackageDoesNotExist(string credentialType) { // Arrange - var controller = SetupController(CredentialTypes.ApiKey.VerifyV1, scope, package: null); + var controller = SetupController(credentialType, null, package: null); // Act - var result = await controller.VerifyPackageKeyAsync("foo", "1.0.0"); + var result = await controller.VerifyPackageKeyAsync(PackageId, PackageVersion); // Assert ResultAssert.IsStatusCode( result, HttpStatusCode.NotFound, - String.Format(CultureInfo.CurrentCulture, Strings.PackageWithIdAndVersionNotFound, "foo", "1.0.0")); + String.Format(CultureInfo.CurrentCulture, Strings.PackageWithIdAndVersionNotFound, PackageId, PackageVersion)); - controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Once); + controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), + CredentialTypes.IsPackageVerificationApiKey(credentialType) ? Times.Once() : Times.Never()); - controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", + controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent(PackageId, PackageVersion, It.IsAny(), controller.OwinContext.Request.User.Identity, 404), Times.Once); } - [Theory] - [InlineData(CredentialTypes.ApiKey.V1, "")] - [InlineData(CredentialTypes.ApiKey.V2, "[{\"a\":\"package:push\", \"s\":\"foo\"}]")] - [InlineData(CredentialTypes.ApiKey.V4, "[{\"a\":\"package:pushversion\", \"s\":\"foo\"}]")] - public async Task VerifyPackageKeyAsync_Returns403IfUserIsNotAnOwner_ApiKey(string apiKeyType, string scope) + public static IEnumerable Returns403IfScopeDoesNotMatch_Data => InvalidScopes_Data; + + public static IEnumerable Returns403IfScopeDoesNotMatch_NotVerify_Data { - // Arrange - var package = new Package + get { - PackageRegistration = new PackageRegistration() { Id = "foo" }, - Version = "1.0.0" - }; - var controller = SetupController(apiKeyType, scope, package, isOwner: false); - - // Act - var result = await controller.VerifyPackageKeyAsync("foo", "1.0.0"); - - // Assert - ResultAssert.IsStatusCode( - result, - HttpStatusCode.Forbidden, - Strings.ApiKeyNotAuthorized); - - controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Never); - - controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", - It.IsAny(), controller.OwinContext.Request.User.Identity, 403), Times.Once); + var notVerifyData = CredentialTypesExceptVerifyV1.Select(t => new object[] { t, new[] { NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion } }); + return MemberDataHelper.Combine(notVerifyData, Returns403IfScopeDoesNotMatch_Data); + } } - [Theory] - [InlineData("[{\"a\":\"package:verify\", \"s\":\"foo\"}]")] - public async Task VerifyPackageKeyAsync_Returns403IfUserIsNotAnOwner_ApiKeyVerifyV1(string scope) + public static IEnumerable Returns403IfScopeDoesNotMatch_Verify_Data { - // Arrange - var package = new Package + get { - PackageRegistration = new PackageRegistration() { Id = "foo" }, - Version = "1.0.0" - }; - var controller = SetupController(CredentialTypes.ApiKey.VerifyV1, scope, package, isOwner: false); - - // Act - var result = await controller.VerifyPackageKeyAsync("foo", "1.0.0"); - - // Assert - ResultAssert.IsStatusCode( - result, - HttpStatusCode.Forbidden, - Strings.ApiKeyNotAuthorized); - - controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Once); - - controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", - It.IsAny(), controller.OwinContext.Request.User.Identity, 403), Times.Once); + return MemberDataHelper.Combine(new[] { new object[] { CredentialTypes.ApiKey.VerifyV1, new[] { NuGetScopes.PackageVerify } } }, Returns403IfScopeDoesNotMatch_Data); + } } [Theory] - // action mismatch - [InlineData(CredentialTypes.ApiKey.V2, "[{\"a\":\"package:unlist\", \"s\":\"foo\"}]")] - [InlineData(CredentialTypes.ApiKey.V4, "[{\"a\":\"package:verify\", \"s\":\"foo\"}]")] - // subject mismatch - [InlineData(CredentialTypes.ApiKey.V2, "[{\"a\":\"package:push\", \"s\":\"notfoo\"}]")] - [InlineData(CredentialTypes.ApiKey.V4, "[{\"a\":\"package:pushversion\", \"s\":\"notfoo\"}]")] - public async Task VerifyPackageKeyAsync_Returns403IfScopeDoesNotMatch_ApiKey(string apiKeyType, string scope) + [MemberData(nameof(Returns403IfScopeDoesNotMatch_NotVerify_Data))] + [MemberData(nameof(Returns403IfScopeDoesNotMatch_Verify_Data))] + public async Task Returns403IfScopeDoesNotMatch(string credentialType, string[] expectedRequestedActions, ApiScopeEvaluationResult apiScopeEvaluationResult, HttpStatusCode expectedStatusCode, string description) { // Arrange var package = new Package { - PackageRegistration = new PackageRegistration() { Id = "foo" }, - Version = "1.0.0" + PackageRegistration = new PackageRegistration() { Id = PackageId }, + Version = PackageVersion }; - var controller = SetupController(apiKeyType, scope, package); + var controller = SetupController(credentialType, null, package); + + controller.MockApiScopeEvaluator + .Setup(x => x.Evaluate( + It.Is(u => u.Key == UserKey), + It.IsAny>(), + ActionsRequiringPermissions.VerifyPackage, + package.PackageRegistration, + expectedRequestedActions)) + .Returns(apiScopeEvaluationResult); // Act - var result = await controller.VerifyPackageKeyAsync("foo", "1.0.0"); + var result = await controller.VerifyPackageKeyAsync(PackageId, PackageVersion); // Assert ResultAssert.IsStatusCode( result, - HttpStatusCode.Forbidden, - Strings.ApiKeyNotAuthorized); + expectedStatusCode, + description); - controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Never); + controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), + CredentialTypes.IsPackageVerificationApiKey(credentialType) ? Times.Once() : Times.Never()); - controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", - It.IsAny(), controller.OwinContext.Request.User.Identity, 403), Times.Once); + controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent(PackageId, PackageVersion, + It.IsAny(), controller.OwinContext.Request.User.Identity, (int)expectedStatusCode), Times.Once); } - - [Theory] - // action mismatch - [InlineData("[{\"a\":\"package:push\", \"s\":\"foo\"}]")] - [InlineData("[{\"a\":\"package:pushversion\", \"s\":\"foo\"}]")] - [InlineData("[{\"a\":\"package:unlist\", \"s\":\"foo\"}]")] - // subject mismatch - [InlineData("[{\"a\":\"package:verify\", \"s\":\"notfoo\"}]")] - public async Task VerifyPackageKeyAsync_Returns403IfScopeDoesNotMatch_ApiKeyVerifyV1(string scope) + + [Fact] + public Task Returns200_VerifyV1() { - // Arrange - var package = new Package - { - PackageRegistration = new PackageRegistration() { Id = "foo" }, - Version = "1.0.0" - }; - var controller = SetupController(CredentialTypes.ApiKey.VerifyV1, scope, package); - - // Act - var result = await controller.VerifyPackageKeyAsync("foo", "1.0.0"); - - // Assert - ResultAssert.IsStatusCode( - result, - HttpStatusCode.Forbidden, - Strings.ApiKeyNotAuthorized); - - controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Once); - - controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", - It.IsAny(), controller.OwinContext.Request.User.Identity, 403), Times.Once); + return Returns200(CredentialTypes.ApiKey.VerifyV1, true, NuGetScopes.PackageVerify); } [Theory] - [InlineData(CredentialTypes.ApiKey.V1, "")] - [InlineData(CredentialTypes.ApiKey.V2, "[{\"a\":\"package:push\", \"s\":\"foo\"}]")] - [InlineData(CredentialTypes.ApiKey.V4, "[{\"a\":\"package:pushversion\", \"s\":\"foo\"}]")] - public async Task VerifyPackageKeyAsync_Returns200IfApiKeyWithPushCapability_ApiKey(string apiKeyType, string scope) + [MemberData(nameof(CredentialTypesExceptVerifyV1_Data))] + public Task Returns200_NotVerify(string credentialType) { - // Arrange - var package = new Package - { - PackageRegistration = new PackageRegistration() { Id = "foo" }, - Version = "1.0.0" - }; - var controller = SetupController(apiKeyType, scope, package); - - // Act - var result = await controller.VerifyPackageKeyAsync("foo", "1.0.0"); - - // Assert - ResultAssert.IsEmpty(result); - - controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Never); - - controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", - It.IsAny(), controller.OwinContext.Request.User.Identity, 200), Times.Once); + return Returns200(credentialType, false, NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion); } - [Theory] - [InlineData("[{\"a\":\"package:verify\", \"s\":\"foo\"}]")] - public async Task VerifyPackageKeyAsync_Returns200IfPackageVerifyKey_ApiKeyVerifyV1(string scope) + private async Task Returns200(string credentialType, bool isRemoved, params string[] expectedRequestedActions) { // Arrange var package = new Package { - PackageRegistration = new PackageRegistration() { Id = "foo" }, - Version = "1.0.0" + PackageRegistration = new PackageRegistration() { Id = PackageId }, + Version = PackageVersion }; - var controller = SetupController(CredentialTypes.ApiKey.VerifyV1, scope, package); + var controller = SetupController(credentialType, null, package); // Act - var result = await controller.VerifyPackageKeyAsync("foo", "1.0.0"); + var result = await controller.VerifyPackageKeyAsync(PackageId, PackageVersion); // Assert ResultAssert.IsEmpty(result); - controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Once); + controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), isRemoved ? Times.Once() : Times.Never()); - controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", + controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent(PackageId, PackageVersion, It.IsAny(), controller.OwinContext.Request.User.Identity, 200), Times.Once); + + controller.MockApiScopeEvaluator + .Verify(x => x.Evaluate( + It.Is(u => u.Key == UserKey), + It.IsAny>(), + ActionsRequiringPermissions.VerifyPackage, + package.PackageRegistration, + expectedRequestedActions)); } [Fact] - public async Task VerifyPackageKeyAsync_WritesAuditRecord() + public async Task WritesAuditRecord() { // Arrange var package = new Package { - PackageRegistration = new PackageRegistration() { Id = "foo" }, - Version = "1.0.0" + PackageRegistration = new PackageRegistration() { Id = PackageId }, + Version = PackageVersion }; - var controller = SetupController(CredentialTypes.ApiKey.V4, "", package); + var controller = SetupController(CredentialTypes.ApiKey.V4, null, package); // Act - var result = await controller.VerifyPackageKeyAsync("foo", "1.0.0"); + var result = await controller.VerifyPackageKeyAsync(PackageId, PackageVersion); // Assert Assert.True(controller.AuditingService.WroteRecord(ar => @@ -1732,9 +1601,9 @@ public async Task VerifyStatsDownloadsReturnsNotFoundWhenStatsNotAvailable() ActionResult actionResult = await controller.GetStatsDownloads(null); - HttpStatusCodeResult httpStatusResult = (HttpStatusCodeResult)actionResult; - - Assert.True(httpStatusResult.StatusCode == (int)HttpStatusCode.NotFound, "unexpected StatusCode"); + ResultAssert.IsStatusCode( + actionResult, + HttpStatusCode.NotFound); } [Fact] @@ -1773,4 +1642,4 @@ public async Task VerifyRecentPopularityStatsDownloadsCount() } } } -} +} \ No newline at end of file diff --git a/tests/NuGetGallery.Facts/Framework/MemberDataHelper.cs b/tests/NuGetGallery.Facts/Framework/MemberDataHelper.cs index 1d8576ce4b..019298ba83 100644 --- a/tests/NuGetGallery.Facts/Framework/MemberDataHelper.cs +++ b/tests/NuGetGallery.Facts/Framework/MemberDataHelper.cs @@ -1,6 +1,9 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Collections.Generic; +using System.Linq; + namespace NuGetGallery.Framework { public static class MemberDataHelper @@ -9,5 +12,16 @@ public static object[] AsData(params object[] data) { return data; } + + public static IEnumerable Combine(IEnumerable firstDataSet, IEnumerable secondDataSet) + { + foreach (var firstData in firstDataSet) + { + foreach (var secondData in secondDataSet) + { + yield return firstData.Concat(secondData).ToArray(); + } + } + } } } diff --git a/tests/NuGetGallery.Facts/Framework/TestExtensionMethods.cs b/tests/NuGetGallery.Facts/Framework/TestExtensionMethods.cs index 64b790a082..b83a1acaf1 100644 --- a/tests/NuGetGallery.Facts/Framework/TestExtensionMethods.cs +++ b/tests/NuGetGallery.Facts/Framework/TestExtensionMethods.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Collections.Generic; using System.IO; using System.Security.Claims; using System.Text; @@ -22,6 +23,12 @@ public static class TestExtensionMethods /// public static void SetOwinContextCurrentUser(this AppController self, User user, Credential credential = null) { + if (user == null) + { + self.OwinContext.Request.User = null; + return; + } + ClaimsIdentity identity = null; if (credential != null) @@ -53,18 +60,32 @@ public static void SetOwinContextCurrentUser(this AppController self, User user, self.OwinContext.Request.User = principal; } + public static void SetCurrentUser(this AppController self, User user, ICollection scopes) + { + var credential = + TestCredentialHelper + .CreateV4ApiKey(expiration: null, plaintextApiKey: out var plaintextApiKey) + .WithScopes(scopes); + + self.SetCurrentUser(user, credential); + } + public static void SetCurrentUser(this AppController self, User user, Credential credential = null) { - if (user == null) + self.SetOwinContextCurrentUser(user, credential); + self.SetCurrentUserOwinEnvironmentKey(user); + } + + private static void SetCurrentUserOwinEnvironmentKey(this AppController self, User user) + { + if (user != null) + { + self.OwinContext.Environment[Constants.CurrentUserOwinEnvironmentKey] = user; + } + else { - // Reset the current user - self.OwinContext.Request.User = null; self.OwinContext.Environment.Remove(Constants.CurrentUserOwinEnvironmentKey); - return; } - - SetOwinContextCurrentUser(self, user, credential); - self.OwinContext.Environment[Constants.CurrentUserOwinEnvironmentKey] = user; } public static async Task CaptureBody(this IOwinResponse self, Func captureWithin) diff --git a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj index b14e88e490..db03386ae0 100644 --- a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj +++ b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj @@ -398,6 +398,8 @@ + + @@ -473,6 +475,8 @@ + + diff --git a/tests/NuGetGallery.Facts/Services/ActionRequiringEntityPermissionsFacts.cs b/tests/NuGetGallery.Facts/Services/ActionRequiringEntityPermissionsFacts.cs index e01489d0d4..c13c996692 100644 --- a/tests/NuGetGallery.Facts/Services/ActionRequiringEntityPermissionsFacts.cs +++ b/tests/NuGetGallery.Facts/Services/ActionRequiringEntityPermissionsFacts.cs @@ -11,38 +11,6 @@ namespace NuGetGallery { public class ActionRequiringEntityPermissionsFacts { - private class Entity - { - public IEnumerable Owners { get; } - - public Entity(IEnumerable owners) - { - Owners = owners; - } - } - - private class TestableActionRequiringEntityPermissions - : ActionRequiringEntityPermissions - { - private Func _isAllowedOnEntity; - - public TestableActionRequiringEntityPermissions(PermissionsRequirement accountOnBehalfOfPermissionsRequirement, Func isAllowedOnEntity) - : base(accountOnBehalfOfPermissionsRequirement) - { - _isAllowedOnEntity = isAllowedOnEntity; - } - - protected override IEnumerable GetOwners(Entity entity) - { - return entity != null ? entity.Owners : Enumerable.Empty(); - } - - protected override PermissionsCheckResult CheckPermissionsForEntity(User account, Entity entity) - { - return _isAllowedOnEntity(account, entity); - } - } - public class TheCheckPermissionsMethod { [Fact] @@ -60,7 +28,7 @@ public void ReturnsPermissionsFailureThrownBySubclass() AssertIsAllowed(action, failureToReturn); } - private void AssertIsAllowed(ActionRequiringEntityPermissions action, PermissionsCheckResult expectedFailure) + private void AssertIsAllowed(IActionRequiringEntityPermissions action, PermissionsCheckResult expectedFailure) { Assert.Equal(expectedFailure, action.CheckPermissions((User)null, null, null)); Assert.Equal(expectedFailure, action.CheckPermissions((IPrincipal)null, null, null)); @@ -179,10 +147,10 @@ public void AccountsAllowedOnBehalfOfIsPopulatedWithExpectedAccounts(int expecte Assert.True(accountsAllowedOnBehalfOf.SequenceEqual(expectedAccountsList)); } - private void CreateEntityWithOwner(out Entity entity, out User entityOwner) + private void CreateEntityWithOwner(out TestablePermissionsEntity entity, out User entityOwner) { entityOwner = new User { Key = 2 }; - entity = new Entity(new[] { entityOwner }); + entity = new TestablePermissionsEntity(new[] { entityOwner }); } private void CreateOrganizationWithUser(out Organization organization, out User user) diff --git a/tests/NuGetGallery.Facts/Services/TestableActionRequiringEntityPermissions.cs b/tests/NuGetGallery.Facts/Services/TestableActionRequiringEntityPermissions.cs new file mode 100644 index 0000000000..9bdf26e9ee --- /dev/null +++ b/tests/NuGetGallery.Facts/Services/TestableActionRequiringEntityPermissions.cs @@ -0,0 +1,28 @@ +using System; +using System.Collections.Generic; +using System.Linq; + +namespace NuGetGallery +{ + public class TestableActionRequiringEntityPermissions + : ActionRequiringEntityPermissions + { + private Func _isAllowedOnEntity; + + public TestableActionRequiringEntityPermissions(PermissionsRequirement accountOnBehalfOfPermissionsRequirement, Func isAllowedOnEntity) + : base(accountOnBehalfOfPermissionsRequirement) + { + _isAllowedOnEntity = isAllowedOnEntity; + } + + protected override IEnumerable GetOwners(TestablePermissionsEntity entity) + { + return entity != null ? entity.Owners : Enumerable.Empty(); + } + + protected override PermissionsCheckResult CheckPermissionsForEntity(User account, TestablePermissionsEntity entity) + { + return _isAllowedOnEntity(account, entity); + } + } +} diff --git a/tests/NuGetGallery.Facts/Services/TestablePermissionsEntity.cs b/tests/NuGetGallery.Facts/Services/TestablePermissionsEntity.cs new file mode 100644 index 0000000000..85ac70b7d2 --- /dev/null +++ b/tests/NuGetGallery.Facts/Services/TestablePermissionsEntity.cs @@ -0,0 +1,14 @@ +using System.Collections.Generic; + +namespace NuGetGallery +{ + public class TestablePermissionsEntity + { + public IEnumerable Owners { get; } + + public TestablePermissionsEntity(IEnumerable owners) + { + Owners = owners; + } + } +}