Skip to content

Commit

Permalink
Check for deleted packages on several read and write operations (#8779)
Browse files Browse the repository at this point in the history 
Fix #8778
  • Loading branch information
@joelverhagen
joelverhagen committed Sep 3, 2021
1 parent 9dd5b06 commit 32a9cbf
Show file tree
Hide file tree
Showing 3 changed files with 169 additions and 18 deletions.
6 changes: 5 additions & 1 deletion src/NuGetGallery.Core/Services/ICorePackageService.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ public interface ICorePackageService
Task UpdatePackageStatusAsync(Package package, PackageStatus newPackageStatus, bool commitChanges = true);

/// <summary>
/// Gets the package with the given ID and version when exists; otherwise <c>null</c>.
/// Gets the package with the given ID and version when exists; otherwise <c>null</c>. Note that this method
/// can return a soft deleted package. This will be indicated by <see cref="PackageStatus.Deleted"/> on the
/// <see cref="Package.PackageStatusKey"/> property. Hard deleted packages will be returned as null because no
/// record of the package exists. Consider checking for a null package and a soft deleted depending on your
/// desired behavior for non-existent and deleted packages.
/// </summary>
/// <param name="id">The package ID.</param>
/// <param name="version">The package version.</param>
Expand Down
14 changes: 7 additions & 7 deletions src/NuGetGallery/Controllers/PackagesController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1182,7 +1182,7 @@ public virtual ActionResult AtomFeed(string id, bool prerel = true)
public virtual async Task<ActionResult> License(string id, string version)
{
var package = _packageService.FindPackageByIdAndVersionStrict(id, version);
if (package == null)
if (package == null || package.PackageStatusKey == PackageStatus.Deleted)
{
return HttpNotFound();
}
Expand Down Expand Up @@ -1878,7 +1878,7 @@ public virtual async Task<ActionResult> Reflow(string id, string version)
{
var package = _packageService.FindPackageByIdAndVersionStrict(id, version);

if (package == null)
if (package == null || package.PackageStatusKey == PackageStatus.Deleted)
{
return HttpNotFound();
}
Expand Down Expand Up @@ -1915,7 +1915,7 @@ public virtual async Task<ActionResult> Revalidate(string id, string version, st
{
var package = _packageService.FindPackageByIdAndVersionStrict(id, version);

if (package == null)
if (package == null || package.PackageStatusKey == PackageStatus.Deleted)
{
return HttpNotFound();
}
Expand Down Expand Up @@ -1951,7 +1951,7 @@ public virtual async Task<ActionResult> RevalidateSymbols(string id, string vers
{
var package = _packageService.FindPackageByIdAndVersionStrict(id, version);

if (package == null)
if (package == null || package.PackageStatusKey == PackageStatus.Deleted)
{
return new HttpStatusCodeResult(HttpStatusCode.NotFound,
string.Format(Strings.PackageWithIdAndVersionNotFound, id, version));
Expand Down Expand Up @@ -2152,7 +2152,7 @@ public virtual ActionResult Edit(string id, string version)
public virtual async Task<ActionResult> UpdateListed(string id, string version, bool? listed)
{
var package = _packageService.FindPackageByIdAndVersionStrict(id, version);
if (package == null)
if (package == null || package.PackageStatusKey == PackageStatus.Deleted)
{
return HttpNotFound();
}
Expand Down Expand Up @@ -2195,7 +2195,7 @@ public virtual async Task<ActionResult> UpdateListed(string id, string version,
public virtual async Task<JsonResult> Edit(string id, string version, VerifyPackageRequest formData, string returnUrl)
{
var package = _packageService.FindPackageByIdAndVersionStrict(id, version);
if (package == null)
if (package == null || package.PackageStatusKey == PackageStatus.Deleted)
{
return Json(HttpStatusCode.NotFound, new[] { new JsonValidationMessage(string.Format(Strings.PackageWithIdAndVersionNotFound, id, version)) });
}
Expand Down Expand Up @@ -2992,7 +2992,7 @@ public virtual async Task<JsonResult> PreviewReadMe(ReadMeRequest formData)
public virtual async Task<JsonResult> GetReadMeMd(string id, string version)
{
var package = _packageService.FindPackageByIdAndVersionStrict(id, version);
if (package == null)
if (package == null || package.PackageStatusKey == PackageStatus.Deleted)
{
return Json(HttpStatusCode.NotFound, null, JsonRequestBehavior.AllowGet);
}
Expand Down
167 changes: 157 additions & 10 deletions tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4450,6 +4450,29 @@ public async Task Returns404IfNotFound(bool listed)
Assert.IsType<HttpNotFoundResult>(result);
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public async Task Returns404IfDeleted(bool listed)
{
// Arrange
var packageService = new Mock<IPackageService>(MockBehavior.Strict);
packageService.Setup(svc => svc.FindPackageByIdAndVersionStrict("Foo", "1.0"))
.Returns(new Package { PackageStatusKey = PackageStatus.Deleted });
// Note: this Mock must be strict because it guarantees that MarkPackageListedAsync is not called!

var controller = CreateController(
GetConfigurationService(),
packageService: packageService);
controller.Url = new UrlHelper(new RequestContext(), new RouteCollection());

// Act
var result = await controller.UpdateListed("Foo", "1.0", listed);

// Assert
Assert.IsType<HttpNotFoundResult>(result);
}

public static IEnumerable<object[]> NotOwner_Data
{
get
Expand Down Expand Up @@ -4700,14 +4723,16 @@ protected PackagesController SetupController(
bool hasReadMe = false,
bool isPackageLocked = false,
Mock<IPackageFileService> packageFileService = null,
IReadMeService readMeService = null)
IReadMeService readMeService = null,
PackageStatus packageStatus = PackageStatus.Available)
{
var package = new Package
{
PackageRegistration = new PackageRegistration { Id = "packageId", IsLocked = isPackageLocked },
Version = "1.0",
Listed = true,
HasReadMe = hasReadMe,
PackageStatusKey = packageStatus,
};
package.PackageRegistration.Owners.Add(owner);

Expand Down Expand Up @@ -4912,6 +4937,21 @@ public async Task WhenPackageRegistrationIsLockedReturns403(User currentUser, Us
Assert.IsType<JsonResult>(result);
Assert.Equal(403, controller.Response.StatusCode);
}

[Theory]
[MemberData(nameof(Owner_Data))]
public async Task WhenPackageIsDeletedReturns404(User currentUser, User owner)
{
// Arrange
var controller = SetupController(currentUser, owner, packageStatus: PackageStatus.Deleted);

// Act
var result = await controller.Edit("packageId", "1.0.0", new VerifyPackageRequest(), string.Empty);

// Assert
Assert.IsType<JsonResult>(result);
Assert.Equal(404, controller.Response.StatusCode);
}
}

public class TheListPackagesMethod
Expand Down Expand Up @@ -9220,12 +9260,31 @@ public async Task ReturnsNotFoundForUnknownPackage()
// Assert
Assert.IsType<HttpNotFoundResult>(result);
}

[Fact]
public async Task ReturnsNotFoundForDeletedPackage()
{
// Arrange
_packageService
.Setup(svc => svc.FindPackageByIdAndVersionStrict(
It.IsAny<string>(),
It.IsAny<string>()))
.Returns(new Package { PackageStatusKey = PackageStatus.Deleted });

// Act
var result = await _target.Revalidate(
_package.PackageRegistration.Id,
_package.Version);

// Assert
Assert.IsType<HttpNotFoundResult>(result);
}
}

public class TheRevalidateSymbolsMethod : TestContainer
{
private Package _package;
private SymbolPackage _symbolPacakge;
private SymbolPackage _symbolPackage;
private readonly Mock<IPackageService> _packageService;
private readonly Mock<IValidationService> _validationService;
private readonly TestGalleryConfigurationService _configurationService;
Expand All @@ -9238,12 +9297,12 @@ public TheRevalidateSymbolsMethod()
PackageRegistration = new PackageRegistration { Id = "NuGet.Versioning" },
Version = "3.4.0",
};
_symbolPacakge = new SymbolPackage
_symbolPackage = new SymbolPackage
{
Package = _package,
StatusKey = PackageStatus.Available
};
_package.SymbolPackages.Add(_symbolPacakge);
_package.SymbolPackages.Add(_symbolPackage);

_packageService = new Mock<IPackageService>();
_packageService
Expand Down Expand Up @@ -9272,7 +9331,7 @@ await _target.RevalidateSymbols(

// Assert
_validationService.Verify(
x => x.RevalidateAsync(_symbolPacakge),
x => x.RevalidateAsync(_symbolPackage),
Times.Once);
}

Expand All @@ -9298,7 +9357,7 @@ public async Task RedirectsToCustomReturnUrl()
public async Task RedirectsAfterRevalidatingSymbolsPackageForAllValidStatus(PackageStatus status)
{
// Arrange & Act
_symbolPacakge.StatusKey = status;
_symbolPackage.StatusKey = status;
var result = await _target.RevalidateSymbols(
_package.PackageRegistration.Id,
_package.Version);
Expand Down Expand Up @@ -9329,13 +9388,33 @@ public async Task ReturnsNotFoundForUnknownPackage()
ResultAssert.IsStatusCode(result, HttpStatusCode.NotFound);
}

[Fact]
public async Task ReturnsNotFoundForDeletedPackage()
{
// Arrange
_packageService
.Setup(svc => svc.FindPackageByIdAndVersionStrict(
It.IsAny<string>(),
It.IsAny<string>()))
.Returns(new Package { PackageStatusKey = PackageStatus.Deleted });

// Act
var result = await _target.RevalidateSymbols(
_package.PackageRegistration.Id,
_package.Version);

// Assert
Assert.IsType<HttpStatusCodeResult>(result);
ResultAssert.IsStatusCode(result, HttpStatusCode.NotFound);
}

[Theory]
[InlineData(PackageStatus.Deleted)]
[InlineData(921)]
public async Task ReturnsBadRequestForInvalidSymbolPackageStatus(PackageStatus status)
{
// Arrange and Act
_symbolPacakge.StatusKey = status;
_symbolPackage.StatusKey = status;
var result = await _target.RevalidateSymbols(
_package.PackageRegistration.Id,
_package.Version);
Expand Down Expand Up @@ -9597,7 +9676,7 @@ public async Task ReturnsProperResponseModelWhenConversionFails()
public class TheGetReadMeMethod : TestContainer
{
[Fact]
public async Task ReturnsNotFoundIfPackageMissing()
public async Task ReturnsNotFoundIfPackageIsMissing()
{
// Arrange
var packageService = new Mock<IPackageService>();
Expand All @@ -9616,6 +9695,26 @@ public async Task ReturnsNotFoundIfPackageMissing()
Assert.Equal((int)HttpStatusCode.NotFound, controller.Response.StatusCode);
}

[Fact]
public async Task ReturnsNotFoundIfPackageIsDeleted()
{
// Arrange
var packageService = new Mock<IPackageService>();
packageService
.Setup(x => x.FindPackageByIdAndVersionStrict(It.IsAny<string>(), It.IsAny<string>()))
.Returns(new Package { PackageStatusKey = PackageStatus.Deleted });

var controller = CreateController(
GetConfigurationService(),
packageService: packageService);

// Act
var result = await controller.GetReadMeMd("a", "1.9.2019");

// Assert
Assert.Equal((int)HttpStatusCode.NotFound, controller.Response.StatusCode);
}

public static IEnumerable<object[]> NotOwner_Data
{
get
Expand Down Expand Up @@ -9784,15 +9883,45 @@ public async Task ReturnsForPackageWithReadMe(User currentUser, User owner)
}
}

public class LicenseMethod : TestContainer
public class TheReflowMethod : TestContainer
{
private readonly Mock<IPackageService> _packageService;
private string _packageId = "packageId";
private string _packageVersion = "1.0.0";

public TheReflowMethod()
{
_packageService = new Mock<IPackageService>();
}

[Fact]
public async Task GivenDeletedPackageReturns404()
{
// arrange
_packageService
.Setup(p => p.FindPackageByIdAndVersionStrict(_packageId, _packageVersion))
.Returns(new Package { PackageStatusKey = PackageStatus.Deleted });
var controller = CreateController(
GetConfigurationService(),
packageService: _packageService);

// act
var result = await controller.Reflow(_packageId, _packageVersion);

// assert
Assert.IsType<HttpNotFoundResult>(result);
}
}

public class TheLicenseMethod : TestContainer
{
private readonly Mock<IPackageService> _packageService;
private readonly Mock<IPackageFileService> _packageFileService;
private readonly Mock<ICoreLicenseFileService> _coreLicenseFileService;
private string _packageId = "packageId";
private string _packageVersion = "1.0.0";

public LicenseMethod()
public TheLicenseMethod()
{
_packageService = new Mock<IPackageService>();
_packageFileService = new Mock<IPackageFileService>();
Expand All @@ -9815,6 +9944,24 @@ public async Task GivenInvalidPackageReturns404()
Assert.IsType<HttpNotFoundResult>(result);
}

[Fact]
public async Task GivenDeletedPackageReturns404()
{
// arrange
_packageService
.Setup(p => p.FindPackageByIdAndVersionStrict(_packageId, _packageVersion))
.Returns(new Package { PackageStatusKey = PackageStatus.Deleted });
var controller = CreateController(
GetConfigurationService(),
packageService: _packageService);

// act
var result = await controller.License(_packageId, _packageVersion);

// assert
Assert.IsType<HttpNotFoundResult>(result);
}

[Theory]
[InlineData("MIT")]
[InlineData("some expression")]
Expand Down

0 comments on commit 32a9cbf

Please sign in to comment.