Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Modifying ApiController to not throw ModelBinder exceptions if client sends invalid Api key #495

Closed
wants to merge 1 commit into from
Closed

Modifying ApiController to not throw ModelBinder exceptions if client sends invalid Api key #495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
88 changes: 69 additions & 19 deletions Facts/Controllers/ApiControllerFacts.cs
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
using Moq;
using NuGet;
using Xunit;
using Xunit.Extensions;

namespace NuGetGallery
{
Expand All @@ -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<HttpStatusCodeWithBodyResult>(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<HttpStatusCodeWithBodyResult>(result);
Expand All @@ -39,9 +57,10 @@ public void WillReturnConflictIfAPackageWithTheIdAndSemanticVersionAlreadyExists

var user = new User();

var packageRegistration = new PackageRegistration {
Packages = new List<Package> { new Package { Version = version.ToString() } },
Owners = new List<User> { user }
var packageRegistration = new PackageRegistration
{
Packages = new List<Package> { new Package { Version = version.ToString() } },
Owners = new List<User> { user }
};

var packageSvc = new Mock<IPackageService>();
Expand All @@ -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<HttpStatusCodeWithBodyResult>(result);
Expand All @@ -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));
}
Expand All @@ -88,7 +107,7 @@ public void WillCreateAPackageFromTheNuGetPackage()
userSvc.Setup(x => x.FindByApiKey(It.IsAny<Guid>())).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<User>()));
}
Expand All @@ -105,22 +124,40 @@ public void WillCreateAPackageWithTheUserMatchingTheApiKey()
userSvc.Setup(x => x.FindByApiKey(It.IsAny<Guid>())).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<IPackage>(), matchingUser));
}
}

public class TheDeletePackageAction
{
[Theory]
[InlineData(null)]
[InlineData("")]
[InlineData("this-is-bad-guid")]
public void WillThrowIfTheApiKeyIsAnInvalidGuid(string guidValue)
{
var userSvc = new Mock<IUserService>();
userSvc.Setup(x => x.FindByApiKey(It.IsAny<Guid>())).Returns((User)null);
var controller = CreateController(userSvc: userSvc);

var result = controller.DeletePackage(guidValue, "theId", "1.0.42");

Assert.IsType<HttpStatusCodeWithBodyResult>(result);
var statusCodeResult = (HttpStatusCodeWithBodyResult)result;
Assert.Equal(403, statusCodeResult.StatusCode);
Assert.Equal(String.Format(Strings.ApiKeyNotAuthorized, "delete"), statusCodeResult.StatusDescription);
}

[Fact]
public void WillThrowIfTheApiKeyDoesNotExist()
{
var userSvc = new Mock<IUserService>();
userSvc.Setup(x => x.FindByApiKey(It.IsAny<Guid>())).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<HttpStatusCodeWithBodyResult>(result);
var statusCodeResult = (HttpStatusCodeWithBodyResult)result;
Expand All @@ -137,7 +174,7 @@ public void WillThrowIfAPackageWithTheIdAndSemanticVersionDoesNotExist()
userSvc.Setup(x => x.FindByApiKey(It.IsAny<Guid>())).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<HttpStatusCodeWithBodyResult>(result);
var statusCodeResult = (HttpStatusCodeWithBodyResult)result;
Expand All @@ -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));
}
Expand All @@ -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<HttpStatusCodeWithBodyResult>(result);
var statusCodeResult = (HttpStatusCodeWithBodyResult)result;
Expand All @@ -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));
}
Expand All @@ -219,14 +256,27 @@ 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<EmptyResult>(result);
}
}

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()
{
Expand All @@ -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.");
Expand All @@ -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<EmptyResult>(result);
Expand All @@ -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.");
Expand All @@ -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.");
Expand All @@ -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<EmptyResult>(result);
}
Expand Down
12 changes: 6 additions & 6 deletions Website/ApiController.generated.cs
View file
Open in desktop
Expand Up @@ -102,37 +102,37 @@ 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);
callInfo.RouteValueDictionary.Add("version", version);
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);
callInfo.RouteValueDictionary.Add("version", version);
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;
Expand Down
38 changes: 28 additions & 10 deletions Website/Controllers/ApiController.cs
View file
Open in desktop
Expand Up @@ -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.
Expand All @@ -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"));

Expand All @@ -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"));

Expand Down Expand Up @@ -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"));

Expand All @@ -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();
}
Expand Down