From b756deecfcc05971aa6267aababce82275697592 Mon Sep 17 00:00:00 2001 From: pranavkm Date: Mon, 30 Apr 2012 19:08:18 -0700 Subject: [PATCH] Modifying ApiController to not throw ModelBinder exceptions if client sends invalid Api key Work Item: #494 --- Facts/Controllers/ApiControllerFacts.cs | 88 +++++++++++++++++++------ Website/ApiController.generated.cs | 12 ++-- Website/Controllers/ApiController.cs | 38 ++++++++--- 3 files changed, 103 insertions(+), 35 deletions(-) diff --git a/Facts/Controllers/ApiControllerFacts.cs b/Facts/Controllers/ApiControllerFacts.cs index ea78d4eb9c..d4d054338b 100644 --- a/Facts/Controllers/ApiControllerFacts.cs +++ b/Facts/Controllers/ApiControllerFacts.cs @@ -6,6 +6,7 @@ using Moq; using NuGet; using Xunit; +using Xunit.Extensions; namespace NuGetGallery { @@ -21,7 +22,24 @@ public void WillReturnAn401IfTheApiKeyDoesNotExist() var controller = CreateController(userSvc: userSvc); // Act - var result = controller.CreatePackagePut(Guid.NewGuid()); + var result = controller.CreatePackagePut(Guid.NewGuid().ToString()); + + // Assert + Assert.IsType(result); + var statusCodeResult = (HttpStatusCodeWithBodyResult)result; + Assert.Equal(String.Format(Strings.ApiKeyNotAuthorized, "push"), statusCodeResult.StatusDescription); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("this-is-bad-guid")] + public void WillReturnAn401IfTheApiKeyIsNotAValidGuid(string guid) + { + var controller = CreateController(); + + // Act + var result = controller.CreatePackagePut(guid); // Assert Assert.IsType(result); @@ -39,9 +57,10 @@ public void WillReturnConflictIfAPackageWithTheIdAndSemanticVersionAlreadyExists var user = new User(); - var packageRegistration = new PackageRegistration { - Packages = new List { new Package { Version = version.ToString() } }, - Owners = new List { user } + var packageRegistration = new PackageRegistration + { + Packages = new List { new Package { Version = version.ToString() } }, + Owners = new List { user } }; var packageSvc = new Mock(); @@ -51,7 +70,7 @@ public void WillReturnConflictIfAPackageWithTheIdAndSemanticVersionAlreadyExists var controller = CreateController(userSvc: userSvc, packageSvc: packageSvc, packageFromInputStream: nuGetPackage.Object); // Act - var result = controller.CreatePackagePut(Guid.NewGuid()); + var result = controller.CreatePackagePut(Guid.NewGuid().ToString()); // Assert Assert.IsType(result); @@ -72,7 +91,7 @@ public void WillFindTheUserThatMatchesTheApiKey() var controller = CreateController(userSvc: userSvc, packageSvc: packageSvc, packageFromInputStream: nuGetPackage.Object); var apiKey = Guid.NewGuid(); - controller.CreatePackagePut(apiKey); + controller.CreatePackagePut(apiKey.ToString()); userSvc.Verify(x => x.FindByApiKey(apiKey)); } @@ -88,7 +107,7 @@ public void WillCreateAPackageFromTheNuGetPackage() userSvc.Setup(x => x.FindByApiKey(It.IsAny())).Returns(new User()); var controller = CreateController(userSvc: userSvc, packageSvc: packageSvc, packageFromInputStream: nuGetPackage.Object); - controller.CreatePackagePut(Guid.NewGuid()); + controller.CreatePackagePut(Guid.NewGuid().ToString()); packageSvc.Verify(x => x.CreatePackage(nuGetPackage.Object, It.IsAny())); } @@ -105,7 +124,7 @@ public void WillCreateAPackageWithTheUserMatchingTheApiKey() userSvc.Setup(x => x.FindByApiKey(It.IsAny())).Returns(matchingUser); var controller = CreateController(userSvc: userSvc, packageSvc: packageSvc, packageFromInputStream: nuGetPackage.Object); - controller.CreatePackagePut(Guid.NewGuid()); + controller.CreatePackagePut(Guid.NewGuid().ToString()); packageSvc.Verify(x => x.CreatePackage(It.IsAny(), matchingUser)); } @@ -113,6 +132,24 @@ public void WillCreateAPackageWithTheUserMatchingTheApiKey() public class TheDeletePackageAction { + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("this-is-bad-guid")] + public void WillThrowIfTheApiKeyIsAnInvalidGuid(string guidValue) + { + var userSvc = new Mock(); + userSvc.Setup(x => x.FindByApiKey(It.IsAny())).Returns((User)null); + var controller = CreateController(userSvc: userSvc); + + var result = controller.DeletePackage(guidValue, "theId", "1.0.42"); + + Assert.IsType(result); + var statusCodeResult = (HttpStatusCodeWithBodyResult)result; + Assert.Equal(403, statusCodeResult.StatusCode); + Assert.Equal(String.Format(Strings.ApiKeyNotAuthorized, "delete"), statusCodeResult.StatusDescription); + } + [Fact] public void WillThrowIfTheApiKeyDoesNotExist() { @@ -120,7 +157,7 @@ public void WillThrowIfTheApiKeyDoesNotExist() userSvc.Setup(x => x.FindByApiKey(It.IsAny())).Returns((User)null); var controller = CreateController(userSvc: userSvc); - var result = controller.DeletePackage(Guid.NewGuid(), "theId", "1.0.42"); + var result = controller.DeletePackage(Guid.NewGuid().ToString(), "theId", "1.0.42"); Assert.IsType(result); var statusCodeResult = (HttpStatusCodeWithBodyResult)result; @@ -137,7 +174,7 @@ public void WillThrowIfAPackageWithTheIdAndSemanticVersionDoesNotExist() userSvc.Setup(x => x.FindByApiKey(It.IsAny())).Returns(new User()); var controller = CreateController(userSvc: userSvc, packageSvc: packageSvc); - var result = controller.DeletePackage(Guid.NewGuid(), "theId", "1.0.42"); + var result = controller.DeletePackage(Guid.NewGuid().ToString(), "theId", "1.0.42"); Assert.IsType(result); var statusCodeResult = (HttpStatusCodeWithBodyResult)result; @@ -160,7 +197,7 @@ public void WillFindTheUserThatMatchesTheApiKey() var controller = CreateController(userSvc: userSvc, packageSvc: packageSvc); var apiKey = Guid.NewGuid(); - controller.DeletePackage(apiKey, "theId", "1.0.42"); + controller.DeletePackage(apiKey.ToString(), "theId", "1.0.42"); userSvc.Verify(x => x.FindByApiKey(apiKey)); } @@ -181,7 +218,7 @@ public void WillNotDeleteThePackageIfApiKeyDoesNotBelongToAnOwner() var controller = CreateController(userSvc: userSvc, packageSvc: packageSvc); var apiKey = Guid.NewGuid(); - var result = controller.DeletePackage(apiKey, "theId", "1.0.42"); + var result = controller.DeletePackage(apiKey.ToString(), "theId", "1.0.42"); Assert.IsType(result); var statusCodeResult = (HttpStatusCodeWithBodyResult)result; @@ -203,7 +240,7 @@ public void WillUnlistThePackageIfApiKeyBelongsToAnOwner() var controller = CreateController(userSvc: userSvc, packageSvc: packageSvc); var apiKey = Guid.NewGuid(); - controller.DeletePackage(apiKey, "theId", "1.0.42"); + controller.DeletePackage(apiKey.ToString(), "theId", "1.0.42"); packageSvc.Verify(x => x.MarkPackageUnlisted(package)); } @@ -219,7 +256,7 @@ public void PublishPackageNoOps() var controller = CreateController(userSvc: userSvc, packageSvc: packageSvc); var apiKey = Guid.NewGuid(); - var result = controller.PublishPackage(apiKey, "theId", "1.0.42"); + var result = controller.PublishPackage(apiKey.ToString(), "theId", "1.0.42"); Assert.IsType(result); } @@ -227,6 +264,19 @@ public void PublishPackageNoOps() public class TheVerifyPackageKeyAction { + [Fact] + public void VerifyPackageKeyReturns403IfApiKeyIsInvalidGuid() + { + // Arrange + var controller = CreateController(userSvc: null); + + // Act + var result = controller.VerifyPackageKey("bad-guid", "foo", "1.0.0"); + + // Assert + AssertStatusCodeResult(result, 403, "The specified API key does not provide the authority to push packages."); + } + [Fact] public void VerifyPackageKeyReturns403IfUserDoesNotExist() { @@ -237,7 +287,7 @@ public void VerifyPackageKeyReturns403IfUserDoesNotExist() var controller = CreateController(userSvc: userSvc); // Act - var result = controller.VerifyPackageKey(guid, "foo", "1.0.0"); + var result = controller.VerifyPackageKey(guid.ToString(), "foo", "1.0.0"); // Assert AssertStatusCodeResult(result, 403, "The specified API key does not provide the authority to push packages."); @@ -253,7 +303,7 @@ public void VerifyPackageKeyReturnsEmptyResultIfApiKeyExistsAndIdAndVersionAreEm var controller = CreateController(userSvc: userSvc); // Act - var result = controller.VerifyPackageKey(guid, null, null); + var result = controller.VerifyPackageKey(guid.ToString(), null, null); // Assert Assert.IsType(result); @@ -271,7 +321,7 @@ public void VerifyPackageKeyReturns404IfPackageDoesNotExist() var controller = CreateController(userSvc: userSvc, packageSvc: packageSvc); // Act - var result = controller.VerifyPackageKey(guid, "foo", "1.0.0"); + var result = controller.VerifyPackageKey(guid.ToString(), "foo", "1.0.0"); // Assert AssertStatusCodeResult(result, 404, "A package with id 'foo' and version '1.0.0' does not exist."); @@ -289,7 +339,7 @@ public void VerifyPackageKeyReturns403IfUserIsNotAnOwner() var controller = CreateController(userSvc: userSvc, packageSvc: packageSvc); // Act - var result = controller.VerifyPackageKey(guid, "foo", "1.0.0"); + var result = controller.VerifyPackageKey(guid.ToString(), "foo", "1.0.0"); // Assert AssertStatusCodeResult(result, 403, "The specified API key does not provide the authority to push packages."); @@ -310,7 +360,7 @@ public void VerifyPackageKeyReturns200IfUserIsAnOwner() var controller = CreateController(userSvc: userSvc, packageSvc: packageSvc); // Act - var result = controller.VerifyPackageKey(guid, "foo", "1.0.0"); + var result = controller.VerifyPackageKey(guid.ToString(), "foo", "1.0.0"); Assert.IsType(result); } diff --git a/Website/ApiController.generated.cs b/Website/ApiController.generated.cs index 41aa2e65eb..f0d66d35d4 100644 --- a/Website/ApiController.generated.cs +++ b/Website/ApiController.generated.cs @@ -102,7 +102,7 @@ public class T4MVC_ApiController: NuGetGallery.ApiController { return callInfo; } - public override System.Web.Mvc.ActionResult VerifyPackageKey(System.Guid apiKey, string id, string version) { + public override System.Web.Mvc.ActionResult VerifyPackageKey(string apiKey, string id, string version) { var callInfo = new T4MVC_ActionResult(Area, Name, ActionNames.VerifyPackageKey); callInfo.RouteValueDictionary.Add("apiKey", apiKey); callInfo.RouteValueDictionary.Add("id", id); @@ -110,19 +110,19 @@ public class T4MVC_ApiController: NuGetGallery.ApiController { return callInfo; } - public override System.Web.Mvc.ActionResult CreatePackagePut(System.Guid apiKey) { + public override System.Web.Mvc.ActionResult CreatePackagePut(string apiKey) { var callInfo = new T4MVC_ActionResult(Area, Name, ActionNames.CreatePackagePut); callInfo.RouteValueDictionary.Add("apiKey", apiKey); return callInfo; } - public override System.Web.Mvc.ActionResult CreatePackagePost(System.Guid apiKey) { + public override System.Web.Mvc.ActionResult CreatePackagePost(string apiKey) { var callInfo = new T4MVC_ActionResult(Area, Name, ActionNames.CreatePackagePost); callInfo.RouteValueDictionary.Add("apiKey", apiKey); return callInfo; } - public override System.Web.Mvc.ActionResult DeletePackage(System.Guid apiKey, string id, string version) { + public override System.Web.Mvc.ActionResult DeletePackage(string apiKey, string id, string version) { var callInfo = new T4MVC_ActionResult(Area, Name, ActionNames.DeletePackage); callInfo.RouteValueDictionary.Add("apiKey", apiKey); callInfo.RouteValueDictionary.Add("id", id); @@ -130,9 +130,9 @@ public class T4MVC_ApiController: NuGetGallery.ApiController { return callInfo; } - public override System.Web.Mvc.ActionResult PublishPackage(System.Guid key, string id, string version) { + public override System.Web.Mvc.ActionResult PublishPackage(string apiKey, string id, string version) { var callInfo = new T4MVC_ActionResult(Area, Name, ActionNames.PublishPackage); - callInfo.RouteValueDictionary.Add("key", key); + callInfo.RouteValueDictionary.Add("apiKey", apiKey); callInfo.RouteValueDictionary.Add("id", id); callInfo.RouteValueDictionary.Add("version", version); return callInfo; diff --git a/Website/Controllers/ApiController.cs b/Website/Controllers/ApiController.cs index 34c0e72f57..5f515072b2 100644 --- a/Website/Controllers/ApiController.cs +++ b/Website/Controllers/ApiController.cs @@ -22,7 +22,7 @@ public ApiController(IPackageService packageSvc, IPackageFileService packageFile } [ActionName("GetPackageApi"), HttpGet] - public virtual ActionResult GetPackage(string id, string version) + public virtual ActionResult GetPackage(string id, string version) { // if the version is null, the user is asking for the latest version. Presumably they don't want pre release versions. // The allow prerelease flag is ignored if both id and version are specified. @@ -42,9 +42,15 @@ public virtual ActionResult GetPackage(string id, string version) } [ActionName("VerifyPackageKeyApi"), HttpGet] - public virtual ActionResult VerifyPackageKey(Guid apiKey, string id, string version) + public virtual ActionResult VerifyPackageKey(string apiKey, string id, string version) { - var user = userSvc.FindByApiKey(apiKey); + Guid parsedApiKey; + if (!Guid.TryParse(apiKey, out parsedApiKey)) + { + return new HttpStatusCodeWithBodyResult(HttpStatusCode.Forbidden, string.Format(CultureInfo.CurrentCulture, Strings.ApiKeyNotAuthorized, "push")); + } + + var user = userSvc.FindByApiKey(parsedApiKey); if (user == null) return new HttpStatusCodeWithBodyResult(HttpStatusCode.Forbidden, string.Format(CultureInfo.CurrentCulture, Strings.ApiKeyNotAuthorized, "push")); @@ -63,20 +69,26 @@ public virtual ActionResult VerifyPackageKey(Guid apiKey, string id, string vers } [ActionName("PushPackageApi"), HttpPut] - public virtual ActionResult CreatePackagePut(Guid apiKey) + public virtual ActionResult CreatePackagePut(string apiKey) { return CreatePackageInternal(apiKey); } [ActionName("PushPackageApi"), HttpPost] - public virtual ActionResult CreatePackagePost(Guid apiKey) + public virtual ActionResult CreatePackagePost(string apiKey) { return CreatePackageInternal(apiKey); } - private ActionResult CreatePackageInternal(Guid apiKey) + private ActionResult CreatePackageInternal(string apiKey) { - var user = userSvc.FindByApiKey(apiKey); + Guid parsedApiKey; + if (!Guid.TryParse(apiKey, out parsedApiKey)) + { + return new HttpStatusCodeWithBodyResult(HttpStatusCode.Forbidden, string.Format(CultureInfo.CurrentCulture, Strings.ApiKeyNotAuthorized, "push")); + } + + var user = userSvc.FindByApiKey(parsedApiKey); if (user == null) return new HttpStatusCodeWithBodyResult(HttpStatusCode.Forbidden, String.Format(CultureInfo.CurrentCulture, Strings.ApiKeyNotAuthorized, "push")); @@ -105,9 +117,15 @@ private ActionResult CreatePackageInternal(Guid apiKey) } [ActionName("DeletePackageApi"), HttpDelete] - public virtual ActionResult DeletePackage(Guid apiKey, string id, string version) + public virtual ActionResult DeletePackage(string apiKey, string id, string version) { - var user = userSvc.FindByApiKey(apiKey); + Guid parsedApiKey; + if (!Guid.TryParse(apiKey, out parsedApiKey)) + { + return new HttpStatusCodeWithBodyResult(HttpStatusCode.Forbidden, string.Format(CultureInfo.CurrentCulture, Strings.ApiKeyNotAuthorized, "delete")); + } + + var user = userSvc.FindByApiKey(parsedApiKey); if (user == null) return new HttpStatusCodeWithBodyResult(HttpStatusCode.Forbidden, string.Format(CultureInfo.CurrentCulture, Strings.ApiKeyNotAuthorized, "delete")); @@ -123,7 +141,7 @@ public virtual ActionResult DeletePackage(Guid apiKey, string id, string version } [ActionName("PublishPackageApi"), HttpPost] - public virtual ActionResult PublishPackage(Guid key, string id, string version) + public virtual ActionResult PublishPackage(string apiKey, string id, string version) { return new EmptyResult(); }