Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add vulnerability messaging to package details page #8358

Merged
merged 3 commits into from
Jan 8, 2021
Merged

Add vulnerability messaging to package details page #8358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a3c8701
Surface vulnerabilities in package detail page
drewgillies Oct 15, 2020
65b1ace
Address feedback
drewgillies Jan 6, 2021
1b76cc3
Addressed feedback 2
drewgillies Jan 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/Bootstrap/dist/css/bootstrap-theme.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

44 changes: 44 additions & 0 deletions src/Bootstrap/less/theme/page-display-package.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,50 @@
}
}

.vulnerabilities-container {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better that we share styles with the deprecation container? We can have a "warning-container" to define these shared styles, and it will be easier to maintain if we need more warning containers in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhhyu Looks like this change isn't practical--the js script that toggles chevron state uses the outer style name to identify the panel on which to rotate it when expanded. Messing with an outer style name has cascading effects. It could be feasible to change inner style names but this wouldn't achieve much as the names would then be duplicated in each outer style.

.vulnerabilities-expander {
display: flex;
justify-content: space-between;
vertical-align: middle;
width: 100%;

.vulnerabilities-expander-container {
display: flex;

.vulnerabilities-expander-icon {
position: unset;
top: unset;
}

.vulnerabilities-expander-info-right {
padding-left: 15px;
}
}
}

.vulnerabilities-content-container {
margin-top: 15px;
padding-top: 15px;
border-top: 1px solid lightgray;

.vulnerabilities-list {
border-collapse: unset;
}
}

.vulnerabilities-severity-critical {
color: red
}

.vulnerabilities-severity-high {
color: red
}

.vulnerabilities-severity-moderate {
color: blue
}
}

.failed-validation-alert-list {
margin-top: 15px;
margin-bottom: 15px;
Expand Down
20 changes: 13 additions & 7 deletions src/NuGetGallery/Controllers/PackagesController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -873,12 +873,18 @@ public virtual async Task<ActionResult> DisplayPackage(string id, string version
}

var readme = await _readMeService.GetReadMeHtmlAsync(package);
var deprecations = _deprecationService.GetDeprecationsById(id);
var packageKeyToDeprecation = deprecations
.GroupBy(d => d.PackageKey)
.ToDictionary(g => g.Key, g => g.First());

var packageKeyToVulnerabilities = _vulnerabilitiesService.GetVulnerabilitiesById(id);
var isPackageDeprecationEnabled = _featureFlagService.IsManageDeprecationEnabled(currentUser, allVersions);
var packageKeyToDeprecation = isPackageDeprecationEnabled
? _deprecationService.GetDeprecationsById(id)
.GroupBy(d => d.PackageKey)
.ToDictionary(g => g.Key, g => g.First())
: null;

var isPackageVulnerabilitiesEnabled = _featureFlagService.IsDisplayVulnerabilitiesEnabled();
var packageKeyToVulnerabilities = isPackageVulnerabilitiesEnabled
? _vulnerabilitiesService.GetVulnerabilitiesById(id)
: null;

IReadOnlyList<PackageRename> packageRenames = null;
if (_featureFlagService.IsPackageRenamesEnabled(currentUser))
Expand All @@ -900,8 +906,8 @@ public virtual async Task<ActionResult> DisplayPackage(string id, string version
model.SymbolsPackageValidationIssues = _validationService.GetLatestPackageValidationIssues(model.LatestSymbolsPackage);
model.IsCertificatesUIEnabled = _contentObjectService.CertificatesConfiguration?.IsUIEnabledForUser(currentUser) ?? false;
model.IsAtomFeedEnabled = _featureFlagService.IsPackagesAtomFeedEnabled();
model.IsPackageDeprecationEnabled = _featureFlagService.IsManageDeprecationEnabled(currentUser, allVersions);
model.IsPackageVulnerabilitiesEnabled = _featureFlagService.IsDisplayVulnerabilitiesEnabled();
model.IsPackageDeprecationEnabled = isPackageDeprecationEnabled;
model.IsPackageVulnerabilitiesEnabled = isPackageVulnerabilitiesEnabled;
model.IsFuGetLinksEnabled = _featureFlagService.IsDisplayFuGetLinksEnabled();
model.IsPackageRenamesEnabled = _featureFlagService.IsPackageRenamesEnabled(currentUser);
model.IsPackageDependentsEnabled = _featureFlagService.IsPackageDependentsEnabled(currentUser);
Expand Down
67 changes: 64 additions & 3 deletions src/NuGetGallery/Helpers/ViewModelExtensions/DisplayPackageViewModelFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ private DisplayPackageViewModel SetupCommon(
}
}

if (packageKeyToDeprecation != null && packageKeyToDeprecation.TryGetValue(package.Key, out var deprecation))
PackageDeprecation deprecation = null;
if (packageKeyToDeprecation != null && packageKeyToDeprecation.TryGetValue(package.Key, out deprecation))
{
viewModel.DeprecationStatus = deprecation.Status;
}
Expand All @@ -192,15 +193,75 @@ private DisplayPackageViewModel SetupCommon(
viewModel.DeprecationStatus = PackageDeprecationStatus.NotDeprecated;
}

if (packageKeyToVulnerabilities != null && packageKeyToVulnerabilities.TryGetValue(package.Key, out var vulnerabilities))
PackageVulnerabilitySeverity? maxVulnerabilitySeverity = null;
if (packageKeyToVulnerabilities != null
&& packageKeyToVulnerabilities.TryGetValue(package.Key, out var vulnerabilities)
&& vulnerabilities != null && vulnerabilities.Any())
{
viewModel.Vulnerabilities = vulnerabilities;
viewModel.MaxVulnerabilitySeverity = vulnerabilities.Max(v => v.Severity);
maxVulnerabilitySeverity = viewModel.Vulnerabilities.Max(v => v.Severity); // cache for messaging
viewModel.MaxVulnerabilitySeverity = maxVulnerabilitySeverity.Value;
}
else
{
viewModel.Vulnerabilities = null;
viewModel.MaxVulnerabilitySeverity = default;
}

viewModel.PackageWarningIconTitle =
GetWarningIconTitle(viewModel.Version, deprecation, maxVulnerabilitySeverity);

return viewModel;
}

private static string GetWarningIconTitle(
string version,
PackageDeprecation deprecation,
PackageVulnerabilitySeverity? maxVulnerabilitySeverity)
{
// We want a tooltip title for the warning icon, which concatenates deprecation and vulnerability information cleanly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: empty line

var deprecationTitle = "";
if (deprecation != null)
{
deprecationTitle = version;
var isLegacy = deprecation.Status.HasFlag(PackageDeprecationStatus.Legacy);
var hasCriticalBugs = deprecation.Status.HasFlag(PackageDeprecationStatus.CriticalBugs);
if (hasCriticalBugs)
{
if (isLegacy)
{
deprecationTitle += " is deprecated because it's legacy and has critical bugs";
}
else
{
deprecationTitle += " is deprecated because it has critical bugs";
}
}
else if (isLegacy)
{
deprecationTitle += " is deprecated because it's legacy and no longer maintained";
}
else
{
deprecationTitle += " is deprecated";
}
}

if (maxVulnerabilitySeverity.HasValue)
{
var severity = Enum.GetName(typeof(PackageVulnerabilitySeverity), maxVulnerabilitySeverity)?.ToLowerInvariant() ?? "unknown";
var vulnerabilitiesTitle = $"{version} has at least one vulnerability with {severity} severity.";

return string.IsNullOrEmpty(deprecationTitle)
? vulnerabilitiesTitle
: $"{deprecationTitle}; {vulnerabilitiesTitle}";
}

return string.IsNullOrEmpty(deprecationTitle)
? string.Empty
: $"{deprecationTitle}.";
}

private static string GetPushedBy(Package package, User currentUser, Dictionary<User, string> pushedByCache)
{
var userPushedBy = package.User;
Expand Down
3 changes: 3 additions & 0 deletions src/NuGetGallery/NuGetGallery.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2305,6 +2305,9 @@
<ItemGroup>
<Content Include="App_Data\Files\Content\Query-Hint-Configuration.json" />
</ItemGroup>
<ItemGroup>
<Content Include="Views\Packages\_DisplayPackageVulnerabilities.cshtml" />
</ItemGroup>
<PropertyGroup>
<VisualStudioVersion Condition="'$(VisualStudioVersion)' == ''">10.0</VisualStudioVersion>
<VSToolsPath Condition="'$(VSToolsPath)' == ''">$(MSBuildExtensionsPath32)\Microsoft\VisualStudio\v$(VisualStudioVersion)</VSToolsPath>
Expand Down
28 changes: 21 additions & 7 deletions src/NuGetGallery/Scripts/gallery/page-display-package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,37 @@ $(function () {
// Configure the rename information container
window.nuget.configureExpander("rename-content-container", "ChevronDown", null, "ChevronUp");
configureExpanderWithEnterKeydown($('#show-rename-content-container'));
var expanderAttributes = ['data-toggle', 'data-target', 'aria-expanded', 'aria-controls', 'tabindex'];

// Configure the vulnerability information container
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe it's better to put this comment below the next line as "expanderAttributes" is also used for the deprecation container.

var vulnerabilitiesContainer = $('#show-vulnerabilities-content-container');
if ($('#vulnerabilities-content-container').children().length) {
// If the vulnerability information container has content, configure it as an expander.
window.nuget.configureExpander("vulnerabilities-content-container", "ChevronDown", null, "ChevronUp");
configureExpanderWithEnterKeydown(vulnerabilitiesContainer);
} else {
// If the container does not have content, remove its expander attributes
expanderAttributes.forEach(attribute => vulnerabilitiesContainer.removeAttr(attribute));

// The expander should not be clickable when it doesn't have content
vulnerabilitiesContainer.find('.vulnerabilities-expander').removeAttr('role');

$('#vulnerabilities-expander-icon-right').hide();
}

// Configure the deprecation information container
var container = $('#show-deprecation-content-container');
var deprecationContainer = $('#show-deprecation-content-container');
if ($('#deprecation-content-container').children().length) {
// If the deprecation information container has content, configure it as an expander.
window.nuget.configureExpander("deprecation-content-container", "ChevronDown", null, "ChevronUp");
configureExpanderWithEnterKeydown(container)
configureExpanderWithEnterKeydown(deprecationContainer);
}
else {
// If the container does not have content, remove its expander attributes
var expanderAttributes = ['data-toggle', 'data-target', 'aria-expanded', 'aria-controls', 'tabindex'];
for (var i in expanderAttributes) {
container.removeAttr(expanderAttributes[i]);
}
expanderAttributes.forEach(attribute => deprecationContainer.removeAttr(attribute));

// The expander should not be clickable when it doesn't have content
container.find('.deprecation-expander').removeAttr('role');
deprecationContainer.find('.deprecation-expander').removeAttr('role');

$('#deprecation-expander-icon-right').hide();
}
Expand Down
1 change: 1 addition & 0 deletions src/NuGetGallery/ViewModels/DisplayPackageViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ public bool HasNewerRelease
public PackageDeprecationStatus DeprecationStatus { get; set; }
public IReadOnlyCollection<PackageVulnerability> Vulnerabilities { get; set; }
public PackageVulnerabilitySeverity MaxVulnerabilitySeverity { get; set; }
public string PackageWarningIconTitle { get; set; }
public string AlternatePackageId { get; set; }
public string AlternatePackageVersion { get; set; }
public string CustomMessage { get; set; }
Expand Down
56 changes: 18 additions & 38 deletions src/NuGetGallery/Views/Packages/DisplayPackage.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,11 @@
)
}

@if (Model.IsPackageVulnerabilitiesEnabled && Model.Vulnerabilities != null)
{
@Html.Partial("_DisplayPackageVulnerabilities")
}

@if (Model.IsPackageDeprecationEnabled && Model.DeprecationStatus != PackageDeprecationStatus.NotDeprecated)
{
@Html.Partial("_DisplayPackageDeprecation")
Expand Down Expand Up @@ -359,7 +364,7 @@

@AppendAvailableSymbolsMessage(hasSymbolsPackageAvailable)
</text>
)
)
}
else if (Model.LatestSymbolsPackage.StatusKey == PackageStatus.FailedValidation)
{
Expand Down Expand Up @@ -404,7 +409,7 @@
@ViewHelpers.AlertWarning(
@<text>
The owner has unlisted this package.
This could mean that the package is deprecated or shouldn't be used anymore.
This could mean that the package is deprecated, has security vulnerabilities or shouldn't be used anymore.
</text>
)
}
Expand Down Expand Up @@ -684,9 +689,9 @@
{
<th aria-hidden="true" abbr="Signature Information"></th>
}
@if (Model.IsPackageDeprecationEnabled)
@if (Model.IsPackageDeprecationEnabled || Model.IsPackageVulnerabilitiesEnabled)
{
<th aria-hidden="true" abbr="Deprecation Information"></th>
<th aria-hidden="true" abbr="Package Warnings"></th>
}
</tr>
</thead>
Expand Down Expand Up @@ -753,41 +758,16 @@
</td>
}
}
@if (Model.IsPackageDeprecationEnabled)
{
if (packageVersion.DeprecationStatus == PackageDeprecationStatus.NotDeprecated)
{
<td class="package-icon-cell" aria-hidden="true"></td>
}
else
{
var deprecationTitle = packageVersion.Version;
var isLegacy = packageVersion.DeprecationStatus.HasFlag(PackageDeprecationStatus.Legacy);
var hasCriticalBugs = packageVersion.DeprecationStatus.HasFlag(PackageDeprecationStatus.CriticalBugs);
if (hasCriticalBugs)
{
if (isLegacy)
{
deprecationTitle += " is deprecated because it's legacy and has critical bugs.";
}
else
{
deprecationTitle += " is deprecated because it has critical bugs.";
}
}
else if (isLegacy)
{
deprecationTitle += " is deprecated because it's legacy and no longer maintained.";
}
else
{
deprecationTitle += " is deprecated.";
}

<td class="package-icon-cell">
<i class="ms-Icon ms-Icon--Warning package-icon" title="@deprecationTitle"></i>
</td>
}
@if (string.IsNullOrEmpty(packageVersion.PackageWarningIconTitle))
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bunch of logic here and it's not testable because it's in the cshtml. Consider moving it to the ViewModel so you can add UTs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken a simpler approach here and called the column "Package Warnings"--I don't think we need to customize a hidden column header based on feature flags.

Copy link
Contributor

@zhhyu zhhyu Dec 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, we have a code block here to obtain "iconTitle", and set up a warning icon with the associated title based on some checks. Maybe it's better that we add another property named "warningIconTitle" (and with another additional boolean property named "hasWarningIcon") in the view model, and when we create the view model in the factory, we can run these checks and set up this property. Then in the view, we can easily check this property to determine whether to show the icon and title for a specific version.
In this way, we can move the logic of this code block to the factory, and some tests can be added to cover these checks and messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call--this is much more robust now.

<td class="package-icon-cell" aria-hidden="true"></td>
}
else
{
<td class="package-icon-cell">
<i class="ms-Icon ms-Icon--Warning package-icon" title="@packageVersion.PackageWarningIconTitle"></i>
</td>
}
</tr>
}
Expand Down
Loading