Add vulnerabilities service (#8357)

drewgillies · web-flow · commit 91ab6de26880 · 2020-12-18T06:51:12.000+10:00
diff --git a/src/NuGet.Services.Entities/Package.cs b/src/NuGet.Services.Entities/Package.cs
@@ -24,7 +24,7 @@ public Package()
             SymbolPackages = new HashSet<SymbolPackage>();
             Deprecations = new HashSet<PackageDeprecation>();
             AlternativeOf = new HashSet<PackageDeprecation>();
-            Vulnerabilities = new HashSet<VulnerablePackageVersionRange>();
+            VulnerablePackageRanges = new HashSet<VulnerablePackageVersionRange>();
             Listed = true;
         }
 #pragma warning restore 618
@@ -289,9 +289,9 @@ public bool HasEmbeddedReadme
         public virtual ICollection<PackageDeprecation> AlternativeOf { get; set; }
 
         /// <summary>
-        /// Gets or sets the list of vulnerabilites that this package has.
+        /// Gets or sets the list of vulnerable package ranges connecting this package to vulnerabilities.
         /// </summary>
-        public ICollection<VulnerablePackageVersionRange> Vulnerabilities { get; set; }
+        public ICollection<VulnerablePackageVersionRange> VulnerablePackageRanges { get; set; }
 
         /// <summary>
         /// A flag that indicates that the package metadata had an embedded icon specified.
diff --git a/src/NuGetGallery.Core/Entities/EntitiesContext.cs b/src/NuGetGallery.Core/Entities/EntitiesContext.cs
@@ -482,7 +482,7 @@ protected override void OnModelCreating(DbModelBuilder modelBuilder)
             modelBuilder.Entity<VulnerablePackageVersionRange>()
                 .HasKey(pv => pv.Key)
                 .HasMany(pv => pv.Packages)
-                .WithMany(p => p.Vulnerabilities);
+                .WithMany(p => p.VulnerablePackageRanges);
 
             modelBuilder.Entity<VulnerablePackageVersionRange>()
                 .HasIndex(pv => pv.PackageId);
diff --git a/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs b/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs
@@ -45,7 +45,7 @@ public void ApplyExistingVulnerabilitiesToPackage(Package package)
                 var versionRange = VersionRange.Parse(possibleRange.PackageVersionRange);
                 if (versionRange.Satisfies(version))
                 {
-                    package.Vulnerabilities.Add(possibleRange);
+                    package.VulnerablePackageRanges.Add(possibleRange);
                     possibleRange.Packages.Add(package);
                 }
             }
@@ -278,7 +278,7 @@ private void ProcessNewVulnerabilityRange(VulnerablePackageVersionRange range, H
                         package.NormalizedVersion,
                         range.PackageVersionRange);
 
-                    package.Vulnerabilities.Add(range);
+                    package.VulnerablePackageRanges.Add(range);
                     range.Packages.Add(package);
                     packagesToUpdate.Add(package);
                 }
diff --git a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs
@@ -422,6 +422,10 @@ protected override void Load(ContainerBuilder builder)
                 .As<IPackageDeprecationService>()
                 .InstancePerLifetimeScope();
 
+            builder.RegisterType<PackageVulnerabilitiesService>()
+                .As<IPackageVulnerabilitiesService>()
+                .InstancePerLifetimeScope();
+
             builder.RegisterType<PackageRenameService>()
                 .As<IPackageRenameService>()
                 .InstancePerLifetimeScope();
diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs
@@ -121,6 +121,7 @@ public partial class PackagesController
         private readonly ILicenseExpressionSplitter _licenseExpressionSplitter;
         private readonly IFeatureFlagService _featureFlagService;
         private readonly IPackageDeprecationService _deprecationService;
+        private readonly IPackageVulnerabilitiesService _vulnerabilitiesService;
         private readonly IPackageRenameService _renameService;
         private readonly IABTestService _abTestService;
         private readonly IMarkdownService _markdownService;
@@ -161,6 +162,7 @@ public PackagesController(
             ILicenseExpressionSplitter licenseExpressionSplitter,
             IFeatureFlagService featureFlagService,
             IPackageDeprecationService deprecationService,
+            IPackageVulnerabilitiesService vulnerabilitiesService,
             IPackageRenameService renameService,
             IABTestService abTestService,
             IIconUrlProvider iconUrlProvider,
@@ -197,6 +199,7 @@ public PackagesController(
             _licenseExpressionSplitter = licenseExpressionSplitter ?? throw new ArgumentNullException(nameof(licenseExpressionSplitter));
             _featureFlagService = featureFlagService ?? throw new ArgumentNullException(nameof(featureFlagService));
             _deprecationService = deprecationService ?? throw new ArgumentNullException(nameof(deprecationService));
+            _vulnerabilitiesService = vulnerabilitiesService ?? throw new ArgumentNullException(nameof(vulnerabilitiesService));
             _renameService = renameService ?? throw new ArgumentNullException(nameof(renameService));
             _abTestService = abTestService ?? throw new ArgumentNullException(nameof(abTestService));
             _iconUrlProvider = iconUrlProvider ?? throw new ArgumentNullException(nameof(iconUrlProvider));
@@ -875,6 +878,8 @@ public virtual async Task<ActionResult> DisplayPackage(string id, string version
                 .GroupBy(d => d.PackageKey)
                 .ToDictionary(g => g.Key, g => g.First());
 
+            var packageKeyToVulnerabilities = _vulnerabilitiesService.GetVulnerabilitiesById(id);
+
             IReadOnlyList<PackageRename> packageRenames = null;
             if (_featureFlagService.IsPackageRenamesEnabled(currentUser))
             {
@@ -886,6 +891,7 @@ public virtual async Task<ActionResult> DisplayPackage(string id, string version
                 allVersions,
                 currentUser,
                 packageKeyToDeprecation,
+                packageKeyToVulnerabilities,
                 packageRenames,
                 readme);
 
@@ -895,6 +901,7 @@ public virtual async Task<ActionResult> DisplayPackage(string id, string version
             model.IsCertificatesUIEnabled = _contentObjectService.CertificatesConfiguration?.IsUIEnabledForUser(currentUser) ?? false;
             model.IsAtomFeedEnabled = _featureFlagService.IsPackagesAtomFeedEnabled();
             model.IsPackageDeprecationEnabled = _featureFlagService.IsManageDeprecationEnabled(currentUser, allVersions);
+            model.IsPackageVulnerabilitiesEnabled = _featureFlagService.IsDisplayVulnerabilitiesEnabled();
             model.IsPackageRenamesEnabled = _featureFlagService.IsPackageRenamesEnabled(currentUser);
             model.IsPackageDependentsEnabled = _featureFlagService.IsPackageDependentsEnabled(currentUser);
            
diff --git a/src/NuGetGallery/Helpers/ViewModelExtensions/DeletePackageViewModelFactory.cs b/src/NuGetGallery/Helpers/ViewModelExtensions/DeletePackageViewModelFactory.cs
@@ -39,6 +39,7 @@ public DeletePackageViewModel Setup(
                 allVersions,
                 currentUser,
                 packageKeyToDeprecation: null,
+                packageKeyToVulnerabilities: null,
                 packageRenames: null,
                 readmeResult: null);
 
diff --git a/src/NuGetGallery/Helpers/ViewModelExtensions/DisplayPackageViewModelFactory.cs b/src/NuGetGallery/Helpers/ViewModelExtensions/DisplayPackageViewModelFactory.cs
@@ -23,6 +23,7 @@ public DisplayPackageViewModel Create(
             IReadOnlyCollection<Package> allVersions,
             User currentUser,
             IReadOnlyDictionary<int, PackageDeprecation> packageKeyToDeprecation,
+            IReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>> packageKeyToVulnerabilities,
             IReadOnlyList<PackageRename> packageRenames,
             RenderedMarkdownResult readmeResult)
         {
@@ -33,6 +34,7 @@ public DisplayPackageViewModel Create(
                 allVersions,
                 currentUser,
                 packageKeyToDeprecation,
+                packageKeyToVulnerabilities,
                 packageRenames,
                 readmeResult);
         }
@@ -43,12 +45,15 @@ public DisplayPackageViewModel Setup(
             IReadOnlyCollection<Package> allVersions,
             User currentUser,
             IReadOnlyDictionary<int, PackageDeprecation> packageKeyToDeprecation,
+            IReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>> packageKeyToVulnerabilities,
             IReadOnlyList<PackageRename> packageRenames,
             RenderedMarkdownResult readmeResult)
         {
             _listPackageItemViewModelFactory.Setup(viewModel, package, currentUser);
-            SetupCommon(viewModel, package, pushedBy: null, packageKeyToDeprecation: packageKeyToDeprecation);
-            return SetupInternal(viewModel, package, allVersions, currentUser, packageKeyToDeprecation, packageRenames, readmeResult);
+            SetupCommon(viewModel, package, pushedBy: null, 
+                packageKeyToDeprecation: packageKeyToDeprecation, packageKeyToVulnerabilities: packageKeyToVulnerabilities);
+            return SetupInternal(viewModel, package, allVersions, currentUser, 
+                packageKeyToDeprecation, packageKeyToVulnerabilities, packageRenames, readmeResult);
         }
 
         private DisplayPackageViewModel SetupInternal(
@@ -57,6 +62,7 @@ private DisplayPackageViewModel SetupInternal(
             IReadOnlyCollection<Package> allVersions,
             User currentUser,
             IReadOnlyDictionary<int, PackageDeprecation> packageKeyToDeprecation,
+            IReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>> packageKeyToVulnerabilities,
             IReadOnlyList<PackageRename> packageRenames,
             RenderedMarkdownResult readmeResult)
         {
@@ -74,7 +80,7 @@ private DisplayPackageViewModel SetupInternal(
                     {
                         var vm = new DisplayPackageViewModel();
                         _listPackageItemViewModelFactory.Setup(vm, p, currentUser);
-                        return SetupCommon(vm, p, GetPushedBy(p, currentUser, pushedByCache), packageKeyToDeprecation);
+                        return SetupCommon(vm, p, GetPushedBy(p, currentUser, pushedByCache), packageKeyToDeprecation, packageKeyToVulnerabilities);
                     })
                 .ToList();
 
@@ -136,7 +142,8 @@ private DisplayPackageViewModel SetupCommon(
             DisplayPackageViewModel viewModel,
             Package package,
             string pushedBy,
-            IReadOnlyDictionary<int, PackageDeprecation> packageKeyToDeprecation)
+            IReadOnlyDictionary<int, PackageDeprecation> packageKeyToDeprecation,
+            IReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>> packageKeyToVulnerabilities)
         {
             viewModel.NuGetVersion = NuGetVersion.Parse(NuGetVersionFormatter.ToFullString(package.Version));
             viewModel.Copyright = package.Copyright;
@@ -179,6 +186,12 @@ private DisplayPackageViewModel SetupCommon(
                 viewModel.DeprecationStatus = PackageDeprecationStatus.NotDeprecated;
             }
 
+            if (packageKeyToVulnerabilities != null && packageKeyToVulnerabilities.TryGetValue(package.Key, out var vulnerabilities))
+            {
+                viewModel.Vulnerabilities = vulnerabilities;
+                viewModel.MaxVulnerabilitySeverity = vulnerabilities.Max(v => v.Severity);
+            }
+
             return viewModel;
         }
 
diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj
@@ -305,8 +305,10 @@
     <Compile Include="Modules\CookieComplianceHttpModule.cs" />
     <Compile Include="RequestModels\DeletePackagesApiRequest.cs" />
     <Compile Include="Services\IMarkdownService.cs" />
+    <Compile Include="Services\IPackageVulnerabilitiesService.cs" />
     <Compile Include="Services\IPackageMetadataValidationService.cs" />
     <Compile Include="Services\MarkdownService.cs" />
+    <Compile Include="Services\PackageVulnerabilitiesService.cs" />
     <Compile Include="Services\PackageMetadataValidationService.cs" />
     <Compile Include="Services\ConfigurationIconFileProvider.cs" />
     <Compile Include="Services\IconUrlDeprecationValidationMessage.cs" />
diff --git a/src/NuGetGallery/Services/IPackageVulnerabilitiesService.cs b/src/NuGetGallery/Services/IPackageVulnerabilitiesService.cs
@@ -0,0 +1,18 @@
+// 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.Collections.Generic;
+using System.Threading.Tasks;
+using NuGet.Services.Entities;
+
+namespace NuGetGallery
+{
+    public interface IPackageVulnerabilitiesService
+    {
+        /// <summary>
+        /// Returns a dictionary mapping package keys to collections of vulnerabilities for that package/version
+        /// </summary>
+        /// <param name="id">id of the package for this query</param>
+        IReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>> GetVulnerabilitiesById(string id);
+    }
+}
diff --git a/src/NuGetGallery/Services/PackageVulnerabilitiesService.cs b/src/NuGetGallery/Services/PackageVulnerabilitiesService.cs
@@ -0,0 +1,45 @@
+// 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 NuGet.Services.Entities;
+
+namespace NuGetGallery
+{
+    public class PackageVulnerabilitiesService : IPackageVulnerabilitiesService
+    {
+        private readonly IEntitiesContext _entitiesContext;
+
+        public PackageVulnerabilitiesService(IEntitiesContext entitiesContext)
+        {
+            _entitiesContext = entitiesContext ?? throw new ArgumentNullException(nameof(entitiesContext));
+        }
+
+        public IReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>> GetVulnerabilitiesById(string id)
+        {
+            var result = new Dictionary<int, List<PackageVulnerability>>();
+            var packagesMatchingId = _entitiesContext.Packages
+                .Where(p => p.PackageRegistration != null && p.PackageRegistration.Id == id)
+                .Include($"{nameof(Package.VulnerablePackageRanges)}.{nameof(VulnerablePackageVersionRange.Vulnerability)}");
+            foreach (var package in packagesMatchingId)
+            {
+                if (package.VulnerablePackageRanges == null)
+                {
+                    continue;
+                }
+
+                if (package.VulnerablePackageRanges.Any())
+                {
+                    result.Add(package.Key,
+                        package.VulnerablePackageRanges.Select(vr => vr.Vulnerability).ToList());
+                }
+            }
+
+            return !result.Any() ? null :
+                result.ToDictionary(kv => kv.Key, kv => kv.Value as IReadOnlyList<PackageVulnerability>);
+        }
+    }
+}
diff --git a/src/NuGetGallery/ViewModels/DisplayPackageViewModel.cs b/src/NuGetGallery/ViewModels/DisplayPackageViewModel.cs
@@ -33,6 +33,7 @@ public class DisplayPackageViewModel : ListPackageItemViewModel
         public bool IsDotnetNewTemplatePackageType { get; set; }
         public bool IsAtomFeedEnabled { get; set; }
         public bool IsPackageDeprecationEnabled { get; set; }
+        public bool IsPackageVulnerabilitiesEnabled { get; set; }
         public bool IsPackageRenamesEnabled { get; set; }
         public bool IsGitHubUsageEnabled { get; set; }
         public bool IsPackageDependentsEnabled { get; set; }
@@ -82,6 +83,8 @@ public bool HasNewerRelease
         public EmbeddedLicenseFileType EmbeddedLicenseType { get; set; }
 
         public PackageDeprecationStatus DeprecationStatus { get; set; }
+        public IReadOnlyCollection<PackageVulnerability> Vulnerabilities { get; set; }
+        public PackageVulnerabilitySeverity MaxVulnerabilitySeverity { get; set; }
         public string AlternatePackageId { get; set; }
         public string AlternatePackageVersion { get; set; }
         public string CustomMessage { get; set; }
diff --git a/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs b/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs
@@ -103,14 +103,14 @@ public Task UpdateVulnerabilityAsync(PackageVulnerability vulnerability, bool wi
 
                 var packages = _entitiesContext.Packages
                     .Where(p => p.PackageRegistration.Id == range.PackageId)
-                    .Include(p => p.Vulnerabilities)
+                    .Include(p => p.VulnerablePackageRanges)
                     .ToList();
 
                 var versionRange = VersionRange.Parse(range.PackageVersionRange);
                 foreach (var package in packages)
                 {
                     var version = NuGetVersion.Parse(package.NormalizedVersion);
-                    if (versionRange.Satisfies(version) != package.Vulnerabilities.Contains(existingRange))
+                    if (versionRange.Satisfies(version) != package.VulnerablePackageRanges.Contains(existingRange))
                     {
                         Console.Error.WriteLine(
                             $@"Vulnerability advisory {vulnerability.GitHubDatabaseKey
diff --git a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs
@@ -83,6 +83,7 @@ private static PackagesController CreateController(
             Mock<ILicenseExpressionSplitter> licenseExpressionSplitter = null,
             Mock<IFeatureFlagService> featureFlagService = null,
             Mock<IPackageDeprecationService> deprecationService = null,
+            Mock<IPackageVulnerabilitiesService> vulnerabilitiesService = null,
             Mock<IPackageRenameService> renameService = null,
             Mock<IABTestService> abTestService = null,
             Mock<IIconUrlProvider> iconUrlProvider = null,
@@ -245,6 +246,15 @@ private static PackagesController CreateController(
                     .Setup(x => x.GetDeprecationsById(It.IsAny<string>()))
                     .Returns(new List<PackageDeprecation>());
             }
+
+            if (vulnerabilitiesService == null)
+            {
+                vulnerabilitiesService = new Mock<IPackageVulnerabilitiesService>();
+                vulnerabilitiesService
+                    .Setup(x => x.GetVulnerabilitiesById(It.IsAny<string>()))
+                    .Returns(new Dictionary<int, IReadOnlyList<PackageVulnerability>>());
+            }
+
             iconUrlProvider = iconUrlProvider ?? new Mock<IIconUrlProvider>();
 
             abTestService = abTestService ?? new Mock<IABTestService>();
@@ -281,6 +291,7 @@ private static PackagesController CreateController(
                 licenseExpressionSplitter.Object,
                 featureFlagService.Object,
                 deprecationService.Object,
+                vulnerabilitiesService.Object,
                 renameService.Object,
                 abTestService.Object,
                 iconUrlProvider.Object,
@@ -453,6 +464,7 @@ public async Task IsIndexCheckOnlyHappensForRecentlyChangedPackages(DateTime cre
                 var id = "Test" + Guid.NewGuid().ToString();
                 var packageService = new Mock<IPackageService>();
                 var deprecationService = new Mock<IPackageDeprecationService>();
+                var vulnerabilitiesService = new Mock<IPackageVulnerabilitiesService>();
                 var diagnosticsService = new Mock<IDiagnosticsService>();
                 var searchClient = new Mock<ISearchClient>();
                 var searchService = new Mock<ExternalSearchService>(diagnosticsService.Object, searchClient.Object)
@@ -465,6 +477,7 @@ public async Task IsIndexCheckOnlyHappensForRecentlyChangedPackages(DateTime cre
                     GetConfigurationService(),
                     packageService: packageService,
                     deprecationService: deprecationService,
+                    vulnerabilitiesService: vulnerabilitiesService,
                     searchService: searchService.As<ISearchService>(),
                     httpContext: httpContext);
                 controller.SetCurrentUser(TestUtility.FakeUser);
@@ -474,6 +487,11 @@ public async Task IsIndexCheckOnlyHappensForRecentlyChangedPackages(DateTime cre
                     .Returns(new List<PackageDeprecation>())
                     .Verifiable();
 
+                vulnerabilitiesService
+                    .Setup(x => x.GetVulnerabilitiesById(id))
+                    .Returns(new Dictionary<int, IReadOnlyList<PackageVulnerability>>())
+                    .Verifiable();
+
                 searchService
                     .Setup(x => x.RawSearch(It.IsAny<SearchFilter>()))
                     .ReturnsAsync(() => new SearchResults(0, indexTimestampUtc: null));
@@ -507,6 +525,7 @@ public async Task IsIndexCheckOnlyHappensForRecentlyChangedPackages(DateTime cre
                 Assert.Equal(id, model.Id);
                 searchService.Verify(x => x.RawSearch(It.IsAny<SearchFilter>()), Times.Exactly(searchTimes));
                 deprecationService.Verify();
+                vulnerabilitiesService.Verify();
             }
 
             [Fact]
diff --git a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj
diff --git a/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesManagementServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesManagementServiceFacts.cs
diff --git a/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesServiceFacts.cs
diff --git a/tests/NuGetGallery.Facts/ViewModels/DisplayPackageViewModelFacts.cs b/tests/NuGetGallery.Facts/ViewModels/DisplayPackageViewModelFacts.cs

Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,7 @@ public void ApplyExistingVulnerabilitiesToPackage(Package package)`
`45`	`45`	`var versionRange = VersionRange.Parse(possibleRange.PackageVersionRange);`
`46`	`46`	`if (versionRange.Satisfies(version))`
`47`	`47`	`{`
`48`		`- package.Vulnerabilities.Add(possibleRange);`
	`48`	`+ package.VulnerablePackageRanges.Add(possibleRange);`
`49`	`49`	`possibleRange.Packages.Add(package);`
`50`	`50`	`}`
`51`	`51`	`}`
`@@ -278,7 +278,7 @@ private void ProcessNewVulnerabilityRange(VulnerablePackageVersionRange range, H`
`278`	`278`	`package.NormalizedVersion,`
`279`	`279`	`range.PackageVersionRange);`
`280`	`280`
`281`		`- package.Vulnerabilities.Add(range);`
	`281`	`+ package.VulnerablePackageRanges.Add(range);`
`282`	`282`	`range.Packages.Add(package);`
`283`	`283`	`packagesToUpdate.Add(package);`
`284`	`284`	`}`