ApiController actions should not throw exceptions when called with an unparseable version (#6089)

Author: Scott Bommarito

src/NuGetGallery/Controllers/ApiController.cs

@@ -1039,14 +1039,9 @@ public virtual async Task<ActionResult> GetStatsDownloads(int? count)
             return new HttpStatusCodeResult(HttpStatusCode.NotFound);
         }
 
-        private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluation(ApiScopeEvaluationResult evaluationResult, string id, string version)
+        private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluation(ApiScopeEvaluationResult evaluationResult, string id, string versionString)
         {
-            return GetHttpResultFromFailedApiScopeEvaluation(evaluationResult, id, NuGetVersion.Parse(version));
-        }
-
-        private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluation(ApiScopeEvaluationResult result, string id, NuGetVersion version)
-        {
-            return GetHttpResultFromFailedApiScopeEvaluationHelper(result, id, version, HttpStatusCode.Forbidden);
+            return GetHttpResultFromFailedApiScopeEvaluationHelper(evaluationResult, id, versionString, HttpStatusCode.Forbidden);
         }
 
         /// <remarks>
@@ -1055,10 +1050,10 @@ private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluation(A
         /// </remarks>
         private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluationForPush(ApiScopeEvaluationResult result, string id, NuGetVersion version)
         {
-            return GetHttpResultFromFailedApiScopeEvaluationHelper(result, id, version, HttpStatusCode.Unauthorized);
+            return GetHttpResultFromFailedApiScopeEvaluationHelper(result, id, version.ToNormalizedString(), HttpStatusCode.Unauthorized);
         }
 
-        private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluationHelper(ApiScopeEvaluationResult result, string id, NuGetVersion version, HttpStatusCode statusCodeOnFailure)
+        private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluationHelper(ApiScopeEvaluationResult result, string id, string versionString, HttpStatusCode statusCodeOnFailure)
         {
             if (result.IsSuccessful())
             {
@@ -1068,7 +1063,7 @@ private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluationHe
             if (result.PermissionsCheckResult == PermissionsCheckResult.ReservedNamespaceFailure)
             {
                 // We return a special error code for reserved namespace failures.
-                TelemetryService.TrackPackagePushNamespaceConflictEvent(id, version.ToNormalizedString(), GetCurrentUser(), User.Identity);
+                TelemetryService.TrackPackagePushNamespaceConflictEvent(id, versionString, GetCurrentUser(), User.Identity);
                 return new HttpStatusCodeWithBodyResult(HttpStatusCode.Conflict, Strings.UploadPackage_IdNamespaceConflict);
             }
 

tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs

@@ -1235,12 +1235,13 @@ public async Task WillNotCreateAPackageIfScopesInvalidWithExistingRegistration(A
             {
                 // Arrange
                 var packageId = "theId";
+                var packageVersion = "1.0.42";
                 var packageRegistration = new PackageRegistration { Id = packageId };
                 packageRegistration.Id = packageId;
                 var package = new Package
                 {
                     PackageRegistration = packageRegistration,
-                    Version = "1.0.42"
+                    Version = packageVersion
                 };
                 packageRegistration.Packages.Add(package);
 
@@ -1252,7 +1253,7 @@ public async Task WillNotCreateAPackageIfScopesInvalidWithExistingRegistration(A
                 var currentUser = fakes.User;
                 controller.SetCurrentUser(currentUser);
 
-                var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, "1.0.42");
+                var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, packageVersion);
                 controller.SetupPackageFromInputStream(nuGetPackage);
 
                 controller.MockApiScopeEvaluator
@@ -1627,19 +1628,22 @@ public async Task WillThrowIfAPackageWithTheIdAndNuGetVersionDoesNotExist()
                 controller.MockPackageService.Verify(x => x.MarkPackageUnlistedAsync(It.IsAny<Package>(), true), Times.Never());
             }
 
-            public static IEnumerable<object[]> WillNotUnlistThePackageIfScopesInvalid_Data => InvalidScopes_Data;
+            public static IEnumerable<object[]> WillNotUnlistThePackageIfScopesInvalid_Data => MemberDataHelper.Combine(
+                InvalidScopes_Data,
+                MemberDataHelper.AsDataSet("1.0.42", "invalidPackageVersion"));
 
             [Theory]
             [MemberData(nameof(WillNotUnlistThePackageIfScopesInvalid_Data))]
-            public async Task WillNotUnlistThePackageIfScopesInvalid(ApiScopeEvaluationResult evaluationResult, HttpStatusCode expectedStatusCode, string description)
+            public async Task WillNotUnlistThePackageIfScopesInvalid(ApiScopeEvaluationResult evaluationResult, HttpStatusCode expectedStatusCode, string description, string version)
             {
                 var fakes = Get<Fakes>();
                 var currentUser = fakes.User;
 
                 var id = "theId";
                 var package = new Package
                 {
-                    PackageRegistration = new PackageRegistration { Id = id }
+                    PackageRegistration = new PackageRegistration { Id = id },
+                    Version = version
                 };
 
                 var controller = new TestableApiController(GetConfigurationService());
@@ -1656,7 +1660,7 @@ public async Task WillNotUnlistThePackageIfScopesInvalid(ApiScopeEvaluationResul
                         NuGetScopes.PackageUnlist))
                     .Returns(evaluationResult);
 
-                var result = await controller.DeletePackage("theId", "1.0.42");
+                var result = await controller.DeletePackage(id, version);
 
                 ResultAssert.IsStatusCode(
                     result,
@@ -2046,19 +2050,22 @@ public async Task WillThrowIfAPackageWithTheIdAndNuGetVersionDoesNotExist()
                 controller.MockPackageService.Verify(x => x.MarkPackageListedAsync(It.IsAny<Package>(), It.IsAny<bool>()), Times.Never());
             }
 
-            public static IEnumerable<object[]> WillListThePackageIfScopesInvalid_Data => InvalidScopes_Data;
+            public static IEnumerable<object[]> WillNotListThePackageIfScopesInvalid_Data => MemberDataHelper.Combine(
+                InvalidScopes_Data,
+                MemberDataHelper.AsDataSet("1.0.42", "invalidPackageVersion"));
 
             [Theory]
-            [MemberData(nameof(WillListThePackageIfScopesInvalid_Data))]
-            public async Task WillListThePackageIfScopesInvalid(ApiScopeEvaluationResult evaluationResult, HttpStatusCode expectedStatusCode, string description)
+            [MemberData(nameof(WillNotListThePackageIfScopesInvalid_Data))]
+            public async Task WillNotListThePackageIfScopesInvalid(ApiScopeEvaluationResult evaluationResult, HttpStatusCode expectedStatusCode, string description, string version)
             {
                 var fakes = Get<Fakes>();
                 var currentUser = fakes.User;
 
                 var id = "theId";
                 var package = new Package
                 {
-                    PackageRegistration = new PackageRegistration { Id = id }
+                    PackageRegistration = new PackageRegistration { Id = id },
+                    Version = version
                 };
 
                 var controller = new TestableApiController(GetConfigurationService());
@@ -2075,7 +2082,7 @@ public async Task WillListThePackageIfScopesInvalid(ApiScopeEvaluationResult eva
                         NuGetScopes.PackageUnlist))
                     .Returns(evaluationResult);
 
-                var result = await controller.PublishPackage("theId", "1.0.42");
+                var result = await controller.PublishPackage(id, version);
 
                 ResultAssert.IsStatusCode(
                     result,
@@ -2352,31 +2359,42 @@ public async Task Returns404IfPackageDoesNotExist(string credentialType)
                     It.IsAny<User>(), controller.OwinContext.Request.User.Identity, 404), Times.Once);
             }
 
+            public static IEnumerable<object[]> Returns403IfScopeDoesNotMatch_PackageVersion_Data => 
+                MemberDataHelper.AsDataSet("1.0.42", "invalidVersionString");
+
             public static IEnumerable<object[]> Returns403IfScopeDoesNotMatch_Data => InvalidScopes_Data;
 
             public static IEnumerable<object[]> Returns403IfScopeDoesNotMatch_NotVerify_Data
             {
                 get
                 {
-                    var notVerifyData = CredentialTypesExceptVerifyV1.Select(t => new object[] { t, new[] { NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion } });
-                    return MemberDataHelper.Combine(notVerifyData, Returns403IfScopeDoesNotMatch_Data);
+                    var notVerifyData = CredentialTypesExceptVerifyV1.Select(
+                        t => MemberDataHelper.AsData(t, new[] { NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion }));
+                    return MemberDataHelper.Combine(
+                        notVerifyData, 
+                        Returns403IfScopeDoesNotMatch_Data, 
+                        Returns403IfScopeDoesNotMatch_PackageVersion_Data);
                 }
             }
 
             public static IEnumerable<object[]> Returns403IfScopeDoesNotMatch_Verify_Data
             {
                 get
                 {
-                    return MemberDataHelper.Combine(new[] { new object[] { CredentialTypes.ApiKey.VerifyV1, new[] { NuGetScopes.PackageVerify } } }, Returns403IfScopeDoesNotMatch_Data);
+                    return MemberDataHelper.Combine(
+                        new[] { new object[] { CredentialTypes.ApiKey.VerifyV1, new[] { NuGetScopes.PackageVerify } } }, 
+                        Returns403IfScopeDoesNotMatch_Data,
+                        Returns403IfScopeDoesNotMatch_PackageVersion_Data);
                 }
             }
 
             [Theory]
             [MemberData(nameof(Returns403IfScopeDoesNotMatch_NotVerify_Data))]
             [MemberData(nameof(Returns403IfScopeDoesNotMatch_Verify_Data))]
-            public async Task Returns403IfScopeDoesNotMatch(string credentialType, string[] expectedRequestedActions, ApiScopeEvaluationResult apiScopeEvaluationResult, HttpStatusCode expectedStatusCode, string description)
+            public async Task Returns403IfScopeDoesNotMatch(string credentialType, string[] expectedRequestedActions, ApiScopeEvaluationResult apiScopeEvaluationResult, HttpStatusCode expectedStatusCode, string description, string packageVersion)
             {
                 // Arrange
+                PackageVersion = packageVersion;
                 var package = new Package
                 {
                     PackageRegistration = new PackageRegistration() { Id = PackageId },

tests/NuGetGallery.Facts/Framework/MemberDataHelper.cs

@@ -18,13 +18,25 @@ public static IEnumerable<object[]> AsDataSet(params object[] dataSet)
             return dataSet.Select(d => new[] { d });
         }
 
-        public static IEnumerable<object[]> Combine(IEnumerable<object[]> firstDataSet, IEnumerable<object[]> secondDataSet)
+        public static IEnumerable<object[]> Combine(params IEnumerable<object[]>[] dataSets)
         {
+            if (!dataSets.Any())
+            {
+                yield break;
+            }
+
+            var firstDataSet = dataSets.First();
+            var lastDataSet = Combine(dataSets.Skip(1).ToArray()).ToArray();
             foreach (var firstData in firstDataSet)
             {
-                foreach (var secondData in secondDataSet)
+                foreach (var lastData in lastDataSet)
+                {
+                    yield return firstData.Concat(lastData).ToArray();
+                }
+
+                if (!lastDataSet.Any())
                 {
-                    yield return firstData.Concat(secondData).ToArray();
+                    yield return firstData;
                 }
             }
         }