Utilize vulnerabilities caching service for in manage packages page

src/NuGetGallery.Services/PackageManagement/IPackageService.cs

@@ -87,9 +87,9 @@ Package FilterLatestPackage(
         Package FilterLatestPackageBySuffix(IReadOnlyCollection<Package> packages,
             string version, bool prerelease);
 
-        IEnumerable<Package> FindPackagesByOwner(User user, bool includeUnlisted, bool includeVersions = false, bool includeVulnerabilities = false);
+        IEnumerable<Package> FindPackagesByOwner(User user, bool includeUnlisted, bool includeVersions = false);
 
-        IEnumerable<Package> FindPackagesByAnyMatchingOwner(User user, bool includeUnlisted, bool includeVersions = false, bool includeVulnerabilities = false);
+        IEnumerable<Package> FindPackagesByAnyMatchingOwner(User user, bool includeUnlisted, bool includeVersions = false);
 
         IQueryable<PackageRegistration> FindPackageRegistrationsByOwner(User user);
 

src/NuGetGallery.Services/PackageManagement/PackageService.cs

@@ -406,12 +406,9 @@ private static Package FilterLatestPackageHelper(
             return package;
         }
 
-        public IEnumerable<Package> FindPackagesByOwner(User user, 
-            bool includeUnlisted, 
-            bool includeVersions = false,
-            bool includeVulnerabilities = false)
+        public IEnumerable<Package> FindPackagesByOwner(User user, bool includeUnlisted, bool includeVersions = false)
         {
-            return GetPackagesForOwners(new[] { user.Key }, includeUnlisted, includeVersions, includeVulnerabilities);
+            return GetPackagesForOwners(new[] { user.Key }, includeUnlisted, includeVersions);
         }
 
         /// <summary>
@@ -420,17 +417,15 @@ public IEnumerable<Package> FindPackagesByOwner(User user,
         public IEnumerable<Package> FindPackagesByAnyMatchingOwner(
             User user,
             bool includeUnlisted,
-            bool includeVersions = false,
-            bool includeVulnerabilities = false)
+            bool includeVersions = false)
         {
             var ownerKeys = user.Organizations.Select(org => org.OrganizationKey).ToList();
             ownerKeys.Insert(0, user.Key);
 
-            return GetPackagesForOwners(ownerKeys, includeUnlisted, includeVersions, includeVulnerabilities);
+            return GetPackagesForOwners(ownerKeys, includeUnlisted, includeVersions);
         }
 
-        private IEnumerable<Package> GetPackagesForOwners(IEnumerable<int> ownerKeys, bool includeUnlisted,
-            bool includeVersions, bool includeVulnerabilities)
+        private IEnumerable<Package> GetPackagesForOwners(IEnumerable<int> ownerKeys, bool includeUnlisted, bool includeVersions)
         {
             IQueryable<Package> packages = _packageRepository.GetAll()
                 .Where(p => p.PackageRegistration.Owners.Any(o => ownerKeys.Contains(o.Key)));
@@ -458,17 +453,11 @@ private IEnumerable<Package> GetPackagesForOwners(IEnumerable<int> ownerKeys, bo
                         .FirstOrDefault());
             }
 
-            var result = packages
+            return packages
                 .Include(p => p.PackageRegistration)
                 .Include(p => p.PackageRegistration.Owners)
-                .Include(p => p.PackageRegistration.RequiredSigners);
-
-            if (includeVulnerabilities && _featureFlagService.IsManagePackagesVulnerabilitiesEnabled())
-            {
-                result = result.Include($"{nameof(Package.VulnerablePackageRanges)}.{nameof(VulnerablePackageVersionRange.Vulnerability)}");
-            }
-
-            return result.ToList();
+                .Include(p => p.PackageRegistration.RequiredSigners)
+                .ToList();
         }
 
         public IQueryable<PackageRegistration> FindPackageRegistrationsByOwner(User user)

src/NuGetGallery/Controllers/UsersController.cs

@@ -526,8 +526,7 @@ public virtual ActionResult Packages()
 
             var wasAADLoginOrMultiFactorAuthenticated = User.WasMultiFactorAuthenticated() || User.WasAzureActiveDirectoryAccountUsedForSignin();
 
-            var packages = PackageService.FindPackagesByAnyMatchingOwner(
-                currentUser, includeUnlisted: true, includeVulnerabilities: true);
+            var packages = PackageService.FindPackagesByAnyMatchingOwner(currentUser, includeUnlisted: true);
 
             var listedPackages = GetPackages(packages, currentUser, wasAADLoginOrMultiFactorAuthenticated,
                 p => p.Listed && p.PackageStatusKey == PackageStatus.Available);

src/NuGetGallery/Services/PackageVulnerabilitiesService.cs

@@ -31,12 +31,7 @@ public bool IsPackageVulnerable(Package package)
                 throw new ArgumentNullException(nameof(package));
             }
 
-            if (package.VulnerablePackageRanges == null)
-            {
-                throw new ArgumentException($"{nameof(package.VulnerablePackageRanges)} should be included in package query");
-            }
-
-            return package.VulnerablePackageRanges.FirstOrDefault(vpr => vpr.Vulnerability != null) != null;
+            return GetVulnerabilitiesById(package.PackageRegistration.Id)?.Where(p => p.Key == package.Key).Any() ?? false;
         }
     }
 }

tests/AccountDeleter.Facts/EvaluatorFacts.cs

@@ -31,7 +31,7 @@ public EvaluatorFacts(ITestOutputHelper output)
         public void NuGetDeleteevaluatorReturnsCorrectValueForUnconfirmed()
         {
             // Setup
-            _packageService.Setup(ps => ps.FindPackagesByOwner(It.IsAny<User>(), It.IsAny<bool>(), It.IsAny<bool>(), It.IsAny<bool>()))
+            _packageService.Setup(ps => ps.FindPackagesByOwner(It.IsAny<User>(), It.IsAny<bool>(), It.IsAny<bool>()))
                 .Returns(() =>
                 {
                     return new List<Package>();
@@ -58,7 +58,7 @@ public void NuGetDeleteevaluatorReturnsCorrectValueForUnconfirmed()
         public void NuGetDeleteevaluatorReturnsCorrectValueForPackages(bool userHasPackages, bool expectedResult)
         {
             // Setup
-            _packageService.Setup(ps => ps.FindPackagesByOwner(It.IsAny<User>(), It.IsAny<bool>(), It.IsAny<bool>(), It.IsAny<bool>()))
+            _packageService.Setup(ps => ps.FindPackagesByOwner(It.IsAny<User>(), It.IsAny<bool>(), It.IsAny<bool>()))
                 .Returns(() =>
                 {
                     if (userHasPackages)
@@ -94,7 +94,7 @@ public void NuGetDeleteevaluatorReturnsCorrectValueForPackages(bool userHasPacka
         public void NuGetDeleteevaluatorReturnsCorrectValueForOrganizations(bool userHasOrgs, bool userIsAdmin, bool expectedResult)
         {
             // Setup
-            _packageService.Setup(ps => ps.FindPackagesByOwner(It.IsAny<User>(), It.IsAny<bool>(), It.IsAny<bool>(), It.IsAny<bool>()))
+            _packageService.Setup(ps => ps.FindPackagesByOwner(It.IsAny<User>(), It.IsAny<bool>(), It.IsAny<bool>()))
                 .Returns(() =>
                 {
                     return new List<Package>();

tests/NuGetGallery.Facts/Controllers/OrganizationsControllerFacts.cs

@@ -1665,7 +1665,7 @@ public async Task IfAdministrator_ShowsViewWithCorrectData(bool isPackageOrphane
                     .Setup(stub => stub.FindByUsername(testOrganization.Username, false))
                     .Returns(testOrganization);
                 GetMock<IPackageService>()
-                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(testOrganization, It.IsAny<bool>(), false, false))
+                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(testOrganization, It.IsAny<bool>(), false))
                     .Returns(userPackages);
                 GetMock<IPackageService>()
                     .Setup(stub => stub.WillPackageBeOrphanedIfOwnerRemoved(packageRegistration, testOrganization))
@@ -1700,7 +1700,7 @@ public async Task IfOrphanedPackages_RedirectsToDeleteRequest()
                 controller.SetCurrentUser(fakes.OrganizationOwnerAdmin);
 
                 GetMock<IPackageService>()
-                    .Setup(x => x.FindPackagesByAnyMatchingOwner(testOrganization, true, false, false))
+                    .Setup(x => x.FindPackagesByAnyMatchingOwner(testOrganization, true, false))
                     .Returns(new[] { new Package { Version = "1.0.0", PackageRegistration = new PackageRegistration { Owners = new[] { testOrganization } } } });
                 GetMock<IPackageService>()
                     .Setup(x => x.WillPackageBeOrphanedIfOwnerRemoved(It.IsAny<PackageRegistration>(), testOrganization))
@@ -2484,7 +2484,7 @@ public void DeleteHappyAccount(bool withPendingIssues)
                     .Setup(stub => stub.FindByUsername(username, false))
                     .Returns(testUser);
                 GetMock<IPackageService>()
-                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(testUser, It.IsAny<bool>(), false, false))
+                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(testUser, It.IsAny<bool>(), false))
                     .Returns(userPackages);
                 const string iconUrl = "https://icon.test/url";
                 GetMock<IIconUrlProvider>()

tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs

@@ -1099,7 +1099,7 @@ public void ReturnsSinglePackageAsExpected(User currentUser, User owner)
                     .Returns(owner);
 
                 GetMock<IPackageService>()
-                    .Setup(x => x.FindPackagesByOwner(owner, false, false, false))
+                    .Setup(x => x.FindPackagesByOwner(owner, false, false))
                     .Returns(new[] { package, invalidatedPackage, validatingPackage, deletedPackage });
 
                 var controller = GetController<UsersController>();
@@ -1153,7 +1153,7 @@ public void SortsPackagesByDownloadCount(User currentUser, User owner)
                     .Returns(owner);
 
                 GetMock<IPackageService>()
-                    .Setup(x => x.FindPackagesByOwner(owner, false, false, false))
+                    .Setup(x => x.FindPackagesByOwner(owner, false, false))
                     .Returns(new[] { package1, package2 });
 
                 var controller = GetController<UsersController>();
@@ -1195,7 +1195,7 @@ public void UsesProperIconUrl(User currentUser, User owner)
                     .Setup(x => x.FindByUsername(username, false))
                     .Returns(owner);
                 GetMock<IPackageService>()
-                    .Setup(x => x.FindPackagesByOwner(owner, false, false, false))
+                    .Setup(x => x.FindPackagesByOwner(owner, false, false))
                     .Returns(new[] { userPackage });
 
                 var controller = GetController<UsersController>();
@@ -2273,7 +2273,7 @@ public void ShowsViewWithCorrectData(bool isPackageOrphaned, bool withPendingIss
                     .Setup(stub => stub.FindByUsername(testUser.Username, false))
                     .Returns(testUser);
                 GetMock<IPackageService>()
-                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(testUser, It.IsAny<bool>(), false, It.IsAny<bool>()))
+                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(testUser, It.IsAny<bool>(), false))
                     .Returns(userPackages);
                 GetMock<IPackageService>()
                     .Setup(stub => stub.WillPackageBeOrphanedIfOwnerRemoved(packageRegistration, testUser))
@@ -2356,7 +2356,7 @@ public async Task SucceedsIfSupportRequestIsAdded(bool successOnSentRequest)
                     .Setup(stub => stub.FindByUsername(userName, false))
                     .Returns(testUser);
                 GetMock<IPackageService>()
-                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(testUser, It.IsAny<bool>(), false, It.IsAny<bool>()))
+                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(testUser, It.IsAny<bool>(), false))
                     .Returns(userPackages);
                 GetMock<ISupportRequestService>()
                    .Setup(stub => stub.GetIssues(null, null, null, userName))
@@ -3690,7 +3690,7 @@ public void DeleteHappyAccount(bool withPendingIssues)
                     .Setup(stub => stub.FindByUsername(userName, false))
                     .Returns(testUser);
                 GetMock<IPackageService>()
-                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(testUser, It.IsAny<bool>(), false, It.IsAny<bool>()))
+                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(testUser, It.IsAny<bool>(), false))
                     .Returns(userPackages);
                 GetMock<ISupportRequestService>()
                     .Setup(stub => stub.GetIssues(null, null, null, null))
@@ -3758,7 +3758,7 @@ public void PackagesAreSortedById()
                     .Returns(_testUser);
 
                 GetMock<IPackageService>()
-                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(_testUser, It.IsAny<bool>(), false, It.IsAny<bool>()))
+                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(_testUser, It.IsAny<bool>(), false))
                     .Returns(userPackages);
 
                 var model = ResultAssert.IsView<ManagePackagesViewModel>(_testController.Packages());
@@ -3786,7 +3786,7 @@ public void PackagesVersionSortOrderIsSetBySemVer()
                     .Returns(_testUser);
 
                 GetMock<IPackageService>()
-                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(_testUser, It.IsAny<bool>(), false, It.IsAny<bool>()))
+                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(_testUser, It.IsAny<bool>(), false))
                     .Returns(userPackages);
 
                 var model = ResultAssert.IsView<ManagePackagesViewModel>(_testController.Packages());
@@ -3813,7 +3813,7 @@ public void UsesProperIconUrl()
                     .Setup(stub => stub.FindByUsername(userName, false))
                     .Returns(_testUser);
                 GetMock<IPackageService>()
-                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(_testUser, It.IsAny<bool>(), false, It.IsAny<bool>()))
+                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(_testUser, It.IsAny<bool>(), false))
                     .Returns(userPackages);
 
                 var model = ResultAssert.IsView<ManagePackagesViewModel>(_testController.Packages());
@@ -3845,7 +3845,7 @@ public void AssessesVulnerabilities()
                     .Setup(stub => stub.IsPackageVulnerable(It.IsAny<Package>()))
                     .Returns<Package>(package => (package?.Id ?? "") == "Company.ZebraPackage"); // this is the vulnerable package - true if this
                 GetMock<IPackageService>()
-                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(_testUser, It.IsAny<bool>(), false, It.IsAny<bool>()))
+                    .Setup(stub => stub.FindPackagesByAnyMatchingOwner(_testUser, It.IsAny<bool>(), false))
                     .Returns(latestPackages);
 
                 var model = ResultAssert.IsView<ManagePackagesViewModel>(_testController.Packages());

tests/NuGetGallery.Facts/Services/DeleteAccountServiceFacts.cs

@@ -806,7 +806,7 @@ private Mock<IPackageService> SetupPackageService(bool isPackageOrphaned)
                 if (_user != null)
                 {
                     packageService.Setup(m => m.FindPackagesByAnyMatchingOwner(
-                        _user, true, It.IsAny<bool>(), It.IsAny<bool>())).Returns(_userPackages);
+                        _user, true, It.IsAny<bool>())).Returns(_userPackages);
                     var packageRegistraionList = new List<PackageRegistration>();
                     if(_userPackagesRegistration != null)
                     {

tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs

@@ -13,7 +13,7 @@
 
 namespace NuGetGallery.Services
 {
-    public class PackageVulnerabilitiesCacheServiceFacts : TestContainer
+    public class PackageVulnerabilitiesCacheServiceFacts
     {
         [Fact]
         public void RefreshesVulnerabilitiesCache()

tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesServiceFacts.cs

@@ -1,23 +1,54 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Collections.Generic;
+using System.Data.Entity;
 using System.Linq;
+using Microsoft.Extensions.DependencyInjection;
+using Moq;
 using NuGet.Services.Entities;
 using NuGetGallery.Framework;
 using Xunit;
 
 namespace NuGetGallery.Services
 {
-    public class PackageVulnerabilitiesServiceFacts : TestContainer
+    public class PackageVulnerabilitiesServiceFacts
     {
+        private Package _packageVulnerable;
+        private Package _packageNotVulnerable;
+        private PackageVulnerability _vulnerabilityModerate;
+
         [Fact]
         public void GetsVulnerableStatusOfPackage()
         {
             // Arrange
+            Setup();
+            var cacheService = new Mock<IPackageVulnerabilitiesCacheService>();
+            cacheService.Setup(x => x.GetVulnerabilitiesById(It.IsAny<string>())).Returns((string id) =>
+                id == _packageVulnerable.PackageRegistration.Id
+                    ? new Dictionary<int, IReadOnlyList<PackageVulnerability>> {
+                        { _packageVulnerable.Key, new List<PackageVulnerability> { _vulnerabilityModerate } }
+                    } 
+                    : null
+            );
+
+            var target = new PackageVulnerabilitiesService(cacheService.Object);
+
+            // Act
+            var shouldBeVulnerable = target.IsPackageVulnerable(_packageVulnerable);
+            var shouldNotBeVulnerable = target.IsPackageVulnerable(_packageNotVulnerable);
+
+            // Assert
+            Assert.True(shouldBeVulnerable);
+            Assert.False(shouldNotBeVulnerable);
+        }
+
+        private void Setup()
+        {
             var registrationVulnerable = new PackageRegistration { Id = "Vulnerable" };
 
-            var vulnerabilityModerate = new PackageVulnerability
+            _vulnerabilityModerate = new PackageVulnerability
             {
                 AdvisoryUrl = "http://theurl/5678",
                 GitHubDatabaseKey = 5678,
@@ -26,34 +57,27 @@ public void GetsVulnerableStatusOfPackage()
 
             var versionRangeModerate = new VulnerablePackageVersionRange
             {
-                Vulnerability = vulnerabilityModerate,
+                Vulnerability = _vulnerabilityModerate,
+                PackageId = registrationVulnerable.Id,
                 PackageVersionRange = "<=1.1.1",
                 FirstPatchedPackageVersion = "1.1.2"
             };
 
-            var packageVulnerable = new Package
+            _packageVulnerable = new Package
             {
                 Key = 0,
                 PackageRegistration = registrationVulnerable,
                 Version = "1.0.0",
-                VulnerablePackageRanges = new List<VulnerablePackageVersionRange> {versionRangeModerate}
+                VulnerablePackageRanges = new List<VulnerablePackageVersionRange> { versionRangeModerate }
             };
-            var packageNotVulnerable = new Package
+            _packageNotVulnerable = new Package
             {
                 Key = 4,
                 PackageRegistration = new PackageRegistration { Id = "NotVulnerable" },
                 VulnerablePackageRanges = new List<VulnerablePackageVersionRange>()
-            }; 
-
-            var target = Get<PackageVulnerabilitiesService>();
-
-            // Act
-            var shouldBeVulnerable = target.IsPackageVulnerable(packageVulnerable);
-            var shouldNotBeVulnerable = target.IsPackageVulnerable(packageNotVulnerable);
+            };
 
-            // Assert
-            Assert.True(shouldBeVulnerable);
-            Assert.False(shouldNotBeVulnerable);
+            versionRangeModerate.Packages = new List<Package> { _packageVulnerable };
         }
     }
 }