From 32a9cbf9c5df7ea3c9db9981939769bb6e103b6d Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 3 Sep 2021 16:45:38 -0700 Subject: [PATCH] Check for deleted packages on several read and write operations (#8779) Fix https://github.com/NuGet/NuGetGallery/issues/8778 --- .../Services/ICorePackageService.cs | 6 +- .../Controllers/PackagesController.cs | 14 +- .../Controllers/PackagesControllerFacts.cs | 167 ++++++++++++++++-- 3 files changed, 169 insertions(+), 18 deletions(-) diff --git a/src/NuGetGallery.Core/Services/ICorePackageService.cs b/src/NuGetGallery.Core/Services/ICorePackageService.cs index d35f5dc1f7..8e742d3c32 100644 --- a/src/NuGetGallery.Core/Services/ICorePackageService.cs +++ b/src/NuGetGallery.Core/Services/ICorePackageService.cs @@ -29,7 +29,11 @@ public interface ICorePackageService Task UpdatePackageStatusAsync(Package package, PackageStatus newPackageStatus, bool commitChanges = true); /// - /// Gets the package with the given ID and version when exists; otherwise null. + /// Gets the package with the given ID and version when exists; otherwise null. Note that this method + /// can return a soft deleted package. This will be indicated by on the + /// 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. /// /// The package ID. /// The package version. diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs index c200c1e773..256b25e28a 100644 --- a/src/NuGetGallery/Controllers/PackagesController.cs +++ b/src/NuGetGallery/Controllers/PackagesController.cs @@ -1182,7 +1182,7 @@ public virtual ActionResult AtomFeed(string id, bool prerel = true) public virtual async Task License(string id, string version) { var package = _packageService.FindPackageByIdAndVersionStrict(id, version); - if (package == null) + if (package == null || package.PackageStatusKey == PackageStatus.Deleted) { return HttpNotFound(); } @@ -1878,7 +1878,7 @@ public virtual async Task Reflow(string id, string version) { var package = _packageService.FindPackageByIdAndVersionStrict(id, version); - if (package == null) + if (package == null || package.PackageStatusKey == PackageStatus.Deleted) { return HttpNotFound(); } @@ -1915,7 +1915,7 @@ public virtual async Task Revalidate(string id, string version, st { var package = _packageService.FindPackageByIdAndVersionStrict(id, version); - if (package == null) + if (package == null || package.PackageStatusKey == PackageStatus.Deleted) { return HttpNotFound(); } @@ -1951,7 +1951,7 @@ public virtual async Task 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)); @@ -2152,7 +2152,7 @@ public virtual ActionResult Edit(string id, string version) public virtual async Task 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(); } @@ -2195,7 +2195,7 @@ public virtual async Task UpdateListed(string id, string version, public virtual async Task 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)) }); } @@ -2992,7 +2992,7 @@ public virtual async Task PreviewReadMe(ReadMeRequest formData) public virtual async Task 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); } diff --git a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs index 357691711f..f138c6bb1c 100644 --- a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs @@ -4450,6 +4450,29 @@ public async Task Returns404IfNotFound(bool listed) Assert.IsType(result); } + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Returns404IfDeleted(bool listed) + { + // Arrange + var packageService = new Mock(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(result); + } + public static IEnumerable NotOwner_Data { get @@ -4700,7 +4723,8 @@ protected PackagesController SetupController( bool hasReadMe = false, bool isPackageLocked = false, Mock packageFileService = null, - IReadMeService readMeService = null) + IReadMeService readMeService = null, + PackageStatus packageStatus = PackageStatus.Available) { var package = new Package { @@ -4708,6 +4732,7 @@ protected PackagesController SetupController( Version = "1.0", Listed = true, HasReadMe = hasReadMe, + PackageStatusKey = packageStatus, }; package.PackageRegistration.Owners.Add(owner); @@ -4912,6 +4937,21 @@ public async Task WhenPackageRegistrationIsLockedReturns403(User currentUser, Us Assert.IsType(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(result); + Assert.Equal(404, controller.Response.StatusCode); + } } public class TheListPackagesMethod @@ -9220,12 +9260,31 @@ public async Task ReturnsNotFoundForUnknownPackage() // Assert Assert.IsType(result); } + + [Fact] + public async Task ReturnsNotFoundForDeletedPackage() + { + // Arrange + _packageService + .Setup(svc => svc.FindPackageByIdAndVersionStrict( + It.IsAny(), + It.IsAny())) + .Returns(new Package { PackageStatusKey = PackageStatus.Deleted }); + + // Act + var result = await _target.Revalidate( + _package.PackageRegistration.Id, + _package.Version); + + // Assert + Assert.IsType(result); + } } public class TheRevalidateSymbolsMethod : TestContainer { private Package _package; - private SymbolPackage _symbolPacakge; + private SymbolPackage _symbolPackage; private readonly Mock _packageService; private readonly Mock _validationService; private readonly TestGalleryConfigurationService _configurationService; @@ -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(); _packageService @@ -9272,7 +9331,7 @@ await _target.RevalidateSymbols( // Assert _validationService.Verify( - x => x.RevalidateAsync(_symbolPacakge), + x => x.RevalidateAsync(_symbolPackage), Times.Once); } @@ -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); @@ -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(), + It.IsAny())) + .Returns(new Package { PackageStatusKey = PackageStatus.Deleted }); + + // Act + var result = await _target.RevalidateSymbols( + _package.PackageRegistration.Id, + _package.Version); + + // Assert + Assert.IsType(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); @@ -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(); @@ -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(); + packageService + .Setup(x => x.FindPackageByIdAndVersionStrict(It.IsAny(), It.IsAny())) + .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 NotOwner_Data { get @@ -9784,7 +9883,37 @@ public async Task ReturnsForPackageWithReadMe(User currentUser, User owner) } } - public class LicenseMethod : TestContainer + public class TheReflowMethod : TestContainer + { + private readonly Mock _packageService; + private string _packageId = "packageId"; + private string _packageVersion = "1.0.0"; + + public TheReflowMethod() + { + _packageService = new Mock(); + } + + [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(result); + } + } + + public class TheLicenseMethod : TestContainer { private readonly Mock _packageService; private readonly Mock _packageFileService; @@ -9792,7 +9921,7 @@ public class LicenseMethod : TestContainer private string _packageId = "packageId"; private string _packageVersion = "1.0.0"; - public LicenseMethod() + public TheLicenseMethod() { _packageService = new Mock(); _packageFileService = new Mock(); @@ -9815,6 +9944,24 @@ public async Task GivenInvalidPackageReturns404() Assert.IsType(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(result); + } + [Theory] [InlineData("MIT")] [InlineData("some expression")]