-
Notifications
You must be signed in to change notification settings - Fork 642
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.
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
Add vulnerability messaging to package details page #8358
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
} | ||
|
@@ -192,15 +193,76 @@ private DisplayPackageViewModel SetupCommon( | |
viewModel.DeprecationStatus = PackageDeprecationStatus.NotDeprecated; | ||
} | ||
|
||
if (packageKeyToVulnerabilities != null && packageKeyToVulnerabilities.TryGetValue(package.Key, out var vulnerabilities)) | ||
PackageVulnerabilitySeverity? maxSeverity = 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); | ||
maxSeverity = viewModel.Vulnerabilities.Max(v => v.Severity); // cache for messaging | ||
viewModel.MaxVulnerabilitySeverity = maxSeverity.Value; | ||
} | ||
else | ||
{ | ||
viewModel.Vulnerabilities = null; | ||
viewModel.MaxVulnerabilitySeverity = default; | ||
} | ||
|
||
viewModel.PackageWarningIconTitle = | ||
GetWarningIconTitle(viewModel.Version, deprecation, maxSeverity); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: maybe it's more clear as: maxSeverity -> maxVulnerabilitySeverity |
||
|
||
return viewModel; | ||
} | ||
|
||
private static string GetWarningIconTitle( | ||
string version, | ||
PackageDeprecation deprecation, | ||
PackageVulnerabilitySeverity? maxSeverity) | ||
{ | ||
// We want a tooltip title for the warning icon, which concatenates deprecation and vulnerability information cleanly | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (maxSeverity.HasValue) | ||
{ | ||
var severity = Enum.GetName(typeof(PackageVulnerabilitySeverity), maxSeverity)?.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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -283,6 +283,11 @@ | |
) | ||
} | ||
|
||
@if (Model.IsPackageVulnerabilitiesEnabled && Model.Vulnerabilities != null) | ||
{ | ||
@Html.Partial("_DisplayPackageVulnerabilities") | ||
} | ||
|
||
@if (Model.IsPackageDeprecationEnabled && Model.DeprecationStatus != PackageDeprecationStatus.NotDeprecated) | ||
{ | ||
@Html.Partial("_DisplayPackageDeprecation") | ||
|
@@ -359,7 +364,7 @@ | |
|
||
@AppendAvailableSymbolsMessage(hasSymbolsPackageAvailable) | ||
</text> | ||
) | ||
) | ||
} | ||
else if (Model.LatestSymbolsPackage.StatusKey == PackageStatus.FailedValidation) | ||
{ | ||
|
@@ -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> | ||
) | ||
} | ||
|
@@ -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> | ||
|
@@ -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)) | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we still keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure of the advantage of empty tags over self-closing ones. I've changed this to an empty tag for consistency, but it just seems like a waste of chars to me. Self-closing tags are neater IMHO. |
||
} | ||
else | ||
{ | ||
<td class="package-icon-cell"> | ||
<i class="ms-Icon ms-Icon--Warning package-icon" title="@packageVersion.PackageWarningIconTitle"></i> | ||
</td> | ||
} | ||
</tr> | ||
} | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.