Show Infobar when vulnerabilities are found after restore

[martinrrm]

Bug

Fixes: NuGet/Home#12398
Telemetry PR: https://github.com/NuGet/Client.Engineering/pull/2395

Regression? Last working version:

Description

When vulnerabilities are found after doing a restore, display an InfoBar in SolutionExplorer that leads to the PM UI.

PR Checklist

• PR has a meaningful title

• PR has a linked issue.

• Described changes

• Tests

  • Automated tests added
  • OR
  • Test exception - I'll add this to the Vendors test manual
  • OR
  • N/A

• Documentation

  • Documentation PR or issue filled - Telemetry Assessment PR Missing
  • OR
  • N/A

[nkolev92]

I just got as far as the NuGet.SolutionRestoreManager.

The new project ref will cause an RPS regression.

NuGet.SolutionRestoreManager should not get any additional dependencies.

[donnie-msft]

Ideally, if we can invoke a command we already have setup, I think that would be a good approach to launching PM UI. If you think of something besides copying the launch logic, then propose that as well!

Basically, the ActionClicked in my mind could just use the OleMenuCommandService to fire off the GUID for either Project/Solution level PM UI, which will launch the window/controls.
https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.Tools/NuGetPackage.cs#L323

I'd have your InfoBarService take a dependency on IMenuCommandService, then use it in the action handler. I'm just guessing it'll work, haven't tried it myself.

[ghost]

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

[martinrrm]

@nkolev92 @donnie-msft @zivkan I tried another approach to show the PM UI to avoid an RPS regression. I did an extensive investigation on how to open the PM UI using a command or something else that did not involve copying code from NuGetPackage.cs but as far as I know, the way this package is initialized make us impossible to reuse that code.

I left this as a draft because I want more feedback before I go and add tests, finish edge cases. 0

[martinrrm]

For reviewers: InfoBar should remain closed if user clicked x. https://devdivdesignguide.azurewebsites.net/windows/notifications-and-progress/infobars-and-promobars/#control-options-and-persistence

[zivkan]

I think it should avoid an RPS regression now. While IPMUIStarter is implemented in an assembly that NuGet.SolutionRestoreManager.dll doesn't load, since it's MEF imported with a Lazy<T> wrapper, this means that I don't believe it'll be loaded until the Lazy<T> is evaluated, which is only on the click event. As long as none of the RPS tests click the button, then it won't cause the second dll to be loaded. LGTM 👍

[nkolev92]

I'm guessing you have some unpushed changes based on earlier conversations

[kartheekp-ms]

I am still reviewing this PR but left few comments to begin with.

OOF

[aortiz-msft]

[jebriede]

This method signature is very limiting. It only allows us to report true or false for this interface that tells if there are vulnerabilities in the solution or not. The variable is even called has vulnerabilities "in solution" which is very specific to where the vulnerabilities are found. Overall this interface has a lot of implementation details in its signature.

Instead, we could have a method ReportVulnerabilitiesAsync that accepts a list of vulnerabilities from the caller and the implementation decides what to do with it. In the implementation you need for the InfoBar, it would just count the list and if it's greater than 0 show the message that there are vulnerabilities in the solution.

We should either change the interface to be less implementation-specific or remove the interface altogether and reference the concrete class directly.

[nkolev92]

Instead, we could have a method ReportVulnerabilitiesAsync that accepts a list of vulnerabilities from the caller and the implementation decides what to do with it. In the implementation you need for the InfoBar, it would just count the list and if it's greater than 0 show the message that there are vulnerabilities in the solution.

I'm not sure I agree. I see that as over optimization.
No need to allocate extra collections if no one is gonna make use of that.
This method is easy to change if there's a better use for it in the future.

[jebriede]

Looking more closely, I see you don't actually have the list of vulnerabilities from the restore, but rather a list restore summaries and their errors. So then what we're really reporting is on specific restore errors that are occurring which in this usage happens to be ones that occur when the restore audit finds a vulnerability. The name of this interface and its method should reflect its usage then. We are providing a way for restore to report on restore errors, not report vulnerabilities.

[nkolev92]

We are providing a way for restore to report on restore errors, not report vulnerabilities

No, restore is reporting that vulnerabilities have been found in any project within the solution. (Probably how Martin arrived at the interface name: IVulnerabilitiesFoundService)
Vulnerabilities are not necessarily errors.

[martinrrm]

The purpose of this interface is to "notify" the InfoBar that there are vulnerabilities, which as Nikolche said, are not necessarily errors.

I think that changing this Interface to a more generic one, like report all restore errors, is going to involve a lot of changes. We probably already have "components" that react to restore errors and are not centralized in one Interface.

[jebriede]

No, restore is reporting that vulnerabilities have been found in any project within the solution.

This is an implementation detail, not an interface specification. The SolutionRestoreJob should not be responsible for deciding what should be surfaced to the user through the UI. We're overloading the responsibilities of a restore job to also decide what is important to show to users. If this ever changes, and we want to report other errors / warnings / results, even if to a different place, we then have to change the SolutionRestoreJob again which violates the open-closed principle.

[jebriede]

@martinrrm feel free to merge the PR on the basis of others' approvals. Please do not consider this a blocker, although I disagree with this design and cannot in good conscience approve the PR, I do not believe it should block the PR either. The rest LGTM.

[nkolev92]

The RestoreSummary.Errors is a poor name, but it's documented that it's warnings and errors,

NuGet.Client/src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreSummary.cs

Line 30 in ee7b952

/// All the warnings and errors that were produced as a result of the restore.

. Unfortunately we can't fix that due to that library being a part of https://github.com/NuGet/NuGet.Client/blob/dev/docs/nuget-sdk.md.
Fortunately for us, NuGet.SolutionRestoreManager isn't part of that SDK

This is an implementation detail, not an interface specification. The SolutionRestoreJob should not be responsible for deciding what should be surfaced to the user through the UI. We're overloading the responsibilities of a restore job to also decide what is important to show to users

I don't think that's true. SolutionRestoreJob is not deciding what's being surfaced. It's notifying via an interface that there are restore time vulnerability warnings or errors. The implementer is deciding to surface an info bar.

Software is going to change. Trying to predict that change is impossible.

[jebriede]

I don't think that's true. SolutionRestoreJob is not deciding what's being surfaced.

Of all the errors and warnings (let's call them Codes) the SolutionRestoreJob can encounter, it is deciding that the presence of vulnerabilities is the only thing that matters to surface. While it does not make any claims on how that should be presented, it does decide that no other information from the restore is going to be surfaced. Any changes to this in the future require a change to the SolutionRestoreJob and potentially the addition of yet another ISomethingNotificationService or a change to the IVulnerabilitiesNotificationService. If instead the interface method accepted the Codes, the implementations could decide what is of importance and the SolutionRestoreJob doesn't have to care what is determined as being important, it just does its job and allows the implementation of the notification service to decide what it will present. Of course this design also allows for composite implementations that each handle different Codes and we get that flexibility for free.

Happy to chat about design principles more offline. For the purposes of this PR, I think we are in agreement that it can be merged without this blocking, provided it gets sufficient approvals.

[nkolev92]

Of all the errors and warnings (let's call them Codes) the SolutionRestoreJob can encounter, it is deciding that the presence of vulnerabilities is the only thing that matters to surface

I think this perspective is one where reasonable people would easily disagree :)

Why does the restore job have to alert anyone of any error codes? After all the assets file is the final output for the operation. Why not read the assets file as it's the final source of truth anyways?

He agreed on merging the PR without his approval at this time.