Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Merge remote-tracking branch 'refs/remotes/origin/dev'
# Conflicts:
#	src/NuGetGallery/Controllers/ApiController.cs
#	tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs
  • Loading branch information
scottbommarito committed Jan 12, 2017
2 parents 846700e + 9216b03 commit d58a743
Show file tree
Hide file tree
Showing 11 changed files with 156 additions and 17 deletions.
Expand Up @@ -47,7 +47,12 @@ public override async Task Invoke(IOwinContext context)
// Invoke the rest of the pipeline
await Next.Invoke(context);

var cookieOptions = new CookieOptions() { HttpOnly = true };
var cookieOptions = new CookieOptions()
{
HttpOnly = true,
Secure = context.Request.IsSecure
};

if (context.Authentication.AuthenticationResponseGrant != null)
{
_logger.WriteVerbose("Auth Grant found, writing Force SSL cookie");
Expand All @@ -59,10 +64,7 @@ public override async Task Invoke(IOwinContext context)
{
_logger.WriteVerbose("Auth Revoke found, removing Force SSL cookie");
// We're revoking authentication, so remove the force ssl cookie
context.Response.Cookies.Delete(CookieName, new CookieOptions()
{
HttpOnly = true
});
context.Response.Cookies.Delete(CookieName, cookieOptions);
}
}
}
Expand Down
13 changes: 11 additions & 2 deletions src/NuGetGallery/Controllers/ApiController.cs
Expand Up @@ -358,10 +358,19 @@ private async Task<ActionResult> CreatePackageInternal()
}
}

await EntitiesContext.SaveChangesAsync();
try
{
await EntitiesContext.SaveChangesAsync();
}
catch
{
// If saving to the DB fails for any reason, we need to delete the package we just saved.
await PackageFileService.DeletePackageFileAsync(nuspec.GetId(), nuspec.GetVersion().ToNormalizedString());
throw;
}

IndexingService.UpdatePackage(package);

// Write an audit record
await AuditingService.SaveAuditRecord(
new PackageAuditRecord(package, AuditedPackageAction.Create, PackageCreatedVia.Api));
Expand Down
13 changes: 11 additions & 2 deletions src/NuGetGallery/Controllers/PackagesController.cs
Expand Up @@ -1180,8 +1180,17 @@ public virtual async Task<ActionResult> VerifyPackage(VerifyPackageRequest formD
return new RedirectResult(Url.VerifyPackage());
}

// commit all changes to database as an atomic transaction
await _entitiesContext.SaveChangesAsync();
try
{
// commit all changes to database as an atomic transaction
await _entitiesContext.SaveChangesAsync();
}
catch
{
// If saving to the DB fails for any reason we need to delete the package we just saved.
await _packageFileService.DeletePackageFileAsync(packageMetadata.Id, packageMetadata.Version.ToNormalizedString());
throw;
}

// tell Lucene to update index for the new package
_indexingService.UpdateIndex();
Expand Down
1 change: 1 addition & 0 deletions src/NuGetGallery/Infrastructure/CookieTempDataProvider.cs
Expand Up @@ -76,6 +76,7 @@ protected virtual void SaveTempData(ControllerContext controllerContext, IDictio
{
var cookie = new HttpCookie(TempDataCookieKey);
cookie.HttpOnly = true;
cookie.Secure = true;
foreach (var item in values)
{
cookie[item.Key] = Convert.ToString(item.Value, CultureInfo.InvariantCulture);
Expand Down
2 changes: 1 addition & 1 deletion src/NuGetGallery/Public/clientaccesspolicy.xml
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>
<access-policy>
<cross-domain-access>
<policy>
Expand Down
3 changes: 1 addition & 2 deletions src/NuGetGallery/Web.config
Expand Up @@ -157,7 +157,6 @@
<!-- Ensure only Admins may access elmah -->
<location path="Admin" inheritInChildApplications="false">
<system.web>
<httpCookies httpOnlyCookies="true" />
<httpHandlers>
<add name="Elmah" verb="POST,GET,HEAD" path="Errors.axd" type="Elmah.ErrorLogPageFactory, Elmah" />
</httpHandlers>
Expand Down Expand Up @@ -246,7 +245,7 @@
</system.Web>
-->
<system.web>
<httpCookies requireSSL="true" />
<httpCookies requireSSL="true" httpOnlyCookies="true" />
<compilation debug="true" targetFramework="4.5.2" />
<pages controlRenderingCompatibilityVersion="4.0">
<namespaces>
Expand Down
Expand Up @@ -94,8 +94,10 @@ public async Task GivenNoForceSslCookieAndNonSslRequest_ItPassesThrough()
next.Verify(n => n.Invoke(It.IsAny<IOwinContext>()));
}

[Fact]
public async Task GivenNextMiddlewareGrantsAuth_ItDropsForceSslCookie()
[Theory]
[InlineData("http", false)]
[InlineData("https", true)]
public async Task GivenNextMiddlewareGrantsAuth_ItDropsForceSslCookie(string protocol, bool secure)
{
// Arrange
var context = Fakes.CreateOwinContext();
Expand All @@ -110,14 +112,14 @@ public async Task GivenNextMiddlewareGrantsAuth_ItDropsForceSslCookie()
return Task.FromResult<object>(null);
});
context.Request
.SetUrl("http://nuget.local/foo/bar/baz?qux=qooz");
.SetUrl(protocol + "://nuget.local/foo/bar/baz?qux=qooz");
var middleware = new ForceSslWhenAuthenticatedMiddleware(next.Object, app, "ForceSSL", 443);

// Act
await middleware.Invoke(context);

// Assert
OwinAssert.SetsCookie(context.Response, "ForceSSL", "true");
OwinAssert.SetsCookie(context.Response, "ForceSSL", "true", secure);
}

[Fact]
Expand Down
44 changes: 44 additions & 0 deletions tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs
Expand Up @@ -118,6 +118,50 @@ public async Task CreatePackageWillSavePackageFileToFileStorage()
controller.MockPackageFileService.Verify();
}

[Fact]
public async Task WillDeletePackageFileFromBlobStorageIfSavingDbChangesFails()
{
// Arrange
var user = new User() { EmailAddress = "confirmed@email.com" };
var packageId = "theId";
var packageVersion = "1.0.42";
var packageRegistration = new PackageRegistration();
packageRegistration.Id = packageId;
packageRegistration.Owners.Add(user);
var package = new Package();
package.PackageRegistration = packageRegistration;
package.Version = "1.0.42";
packageRegistration.Packages.Add(package);

var controller = new TestableApiController();
controller.SetCurrentUser(user);
controller.MockPackageFileService.Setup(
p => p.SavePackageFileAsync(It.IsAny<Package>(), It.IsAny<Stream>()))
.Returns(Task.CompletedTask).Verifiable();
controller.MockPackageFileService.Setup(
p =>
p.DeletePackageFileAsync(packageId,
packageVersion))
.Returns(Task.CompletedTask).Verifiable();
controller.MockPackageService.Setup(p => p.FindPackageRegistrationById(It.IsAny<string>()))
.Returns(packageRegistration);
controller.MockPackageService.Setup(
p =>
p.CreatePackageAsync(It.IsAny<PackageArchiveReader>(), It.IsAny<PackageStreamMetadata>(),
It.IsAny<User>(), false))
.Returns(Task.FromResult(package));
controller.MockEntitiesContext.Setup(e => e.SaveChangesAsync()).Throws<Exception>();

var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, "1.0.42");
controller.SetupPackageFromInputStream(nuGetPackage);

// Act
await Assert.ThrowsAsync<Exception>(async () => await controller.CreatePackagePut());

// Assert
controller.MockPackageFileService.Verify();
}

[Fact]
public async Task WritesAnAuditRecord()
{
Expand Down
41 changes: 41 additions & 0 deletions tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs
Expand Up @@ -1436,6 +1436,47 @@ public async Task WillSavePackageToFileStorage()
}
}

[Fact]
public async Task WillDeletePackageFileFromBlobStorageIfSavingDbChangesFails()
{
// Arrange
var packageId = "theId";
var packageVersion = "1.0.0";
var fakeUploadFileService = new Mock<IUploadFileService>();
using (var fakeFileStream = new MemoryStream())
{
fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult<Stream>(fakeFileStream));
fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0));
var fakePackageService = new Mock<IPackageService>();
var fakePackage = new Package { PackageRegistration = new PackageRegistration { Id = packageId }, Version = packageVersion };
fakePackageService.Setup(x => x.CreatePackageAsync(It.IsAny<PackageArchiveReader>(), It.IsAny<PackageStreamMetadata>(), It.IsAny<User>(), It.IsAny<bool>()))
.Returns(Task.FromResult(fakePackage));
var fakeNuGetPackage = TestPackage.CreateTestPackageStream(packageId, packageVersion);

var fakePackageFileService = new Mock<IPackageFileService>();
fakePackageFileService.Setup(x => x.SavePackageFileAsync(fakePackage, It.IsAny<Stream>())).Returns(Task.CompletedTask).Verifiable();
fakePackageFileService.Setup(x => x.DeletePackageFileAsync(packageId, packageVersion)).Returns(Task.CompletedTask).Verifiable();

var fakeEntitiesContext = new Mock<IEntitiesContext>();
fakeEntitiesContext.Setup(e => e.SaveChangesAsync()).Throws<Exception>();

var controller = CreateController(
packageService: fakePackageService,
uploadFileService: fakeUploadFileService,
fakeNuGetPackage: fakeNuGetPackage,
packageFileService: fakePackageFileService,
entitiesContext: fakeEntitiesContext);
controller.SetCurrentUser(TestUtility.FakeUser);

// Act
await Assert.ThrowsAsync<Exception>(async () => await controller.VerifyPackage(new VerifyPackageRequest() { Listed = true, Edit = null }));

// Assert
fakePackageService.Verify(x => x.CreatePackageAsync(It.IsAny<PackageArchiveReader>(), It.IsAny<PackageStreamMetadata>(), TestUtility.FakeUser, false));
fakePackageFileService.Verify();
}
}

[Fact]
public async Task WillShowViewWithMessageIfSavingPackageBlobFails()
{
Expand Down
Expand Up @@ -89,6 +89,7 @@ public void StoresValuesInCookie()

Assert.Equal(1, cookies.Count);
Assert.True(cookies[0].HttpOnly);
Assert.True(cookies[0].Secure);
Assert.Equal(3, cookies[0].Values.Count);
Assert.Equal("Say hello to my little friend", cookies[0]["message"]);
Assert.Equal("123", cookies[0]["key2"]);
Expand All @@ -108,6 +109,31 @@ public void WithNoValuesDoesNotAddCookie()

Assert.Equal(0, cookies.Count);
}

[Fact]
public void WithInitialStateAndNoValuesClearsCookie()
{
// Arrange and Setup
var cookies = new HttpCookieCollection();
var cookie = new HttpCookie("__Controller::TempData");
cookie.HttpOnly = true;
cookie.Secure = true;
cookies.Add(cookie);
cookie["message"] = "clear";
var httpContext = new Mock<HttpContextBase>();
httpContext.Setup(c => c.Request.Cookies).Returns(cookies);
ITempDataProvider provider = new CookieTempDataProvider(httpContext.Object);
var controllerContext = new ControllerContext();

var tempData = provider.LoadTempData(controllerContext);

// Validate
provider.SaveTempData(controllerContext, new Dictionary<string, object>());
Assert.Equal(1, cookies.Count);
Assert.True(cookies[0].HttpOnly);
Assert.True(cookies[0].Secure);
Assert.Equal("", cookies[0].Value);
}
}
}
}
8 changes: 7 additions & 1 deletion tests/NuGetGallery.Facts/TestUtils/OwinAssert.cs
Expand Up @@ -25,14 +25,20 @@ public static void WillRedirect(IOwinContext context, string expectedLocation)
Assert.Equal(expectedLocation, context.Response.Headers["Location"]);
}

public static void SetsCookie(IOwinResponse response, string name, string expectedValue)
public static void SetsCookie(IOwinResponse response, string name, string expectedValue, bool secure = true)
{
// Get the cookie
var cookie = GetCookie(response, name);

// Check the value
Assert.NotNull(cookie);
Assert.Equal(expectedValue, cookie.Value);

// Check cookie attributes
var header = response.Headers["Set-Cookie"];
Assert.True(!String.IsNullOrEmpty(header));
Assert.True(header.IndexOf("HttpOnly", StringComparison.OrdinalIgnoreCase) > 0);
Assert.Equal(secure, header.IndexOf("Secure", StringComparison.OrdinalIgnoreCase) > 0);
}

public static void DeletesCookie(IOwinResponse response, string name)
Expand Down

0 comments on commit d58a743

Please sign in to comment.