Do not warn http sources in package reference restore when allowInsecureConnections is set to true #5390

heng-liu · 2023-08-30T21:34:54Z

Bug

Fixes: NuGet/Home#12787

Regression? Last working version:

Description

When allowInsecureConnections property in packageSources section is set to true in NuGet.Config files, suppress warnings for HTTP sources in PackageReference restore scenarios in all NuGet tools.
The PR enable the HTTP warnings in this scenario is: Warn when http sources are used in PackageReference restore operations #4556
Updated to use _request.DependencyProviders.RemoteProviders, instead of _request.Project.RestoreMetadata to check , as the first one respect the settings.
Context: The original value of allowInsecureConnections for RestoreRequest.Project.RestoreMetadata.Sources is always false, as the value is from: 1.https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/MSBuildRestoreUtility.cs#L200 which doesn't contain any additional attributes like allowInsecureConnections . (dotnet restore)
2.https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.CommandLine/MsBuildUtility.cs#L210 which is loading an output.dg file(generated by GenerateRestoreGraphFileTask) which also doesn't contain any additional attributes like allowInsecureConnections . (NuGet.exe restore)

PR Checklist

...uGet.Core/NuGet.Commands/RestoreCommand/RequestFactory/DependencyGraphSpecRequestProvider.cs

donnie-msft · 2023-08-31T22:38:15Z

test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommandTests.cs

+                IAssetsLogMessage logMessage = result.LockFile.LogMessages[0];
+                logMessage.Code.Should().Be(NuGetLogCode.NU1803);
+                Assert.Contains(expectedWarning, logMessage.Message);
+                Assert.DoesNotContain(unExpectedWarning, logMessage.Message);


I like that this test is thorough, but is it clearer to just assert Equals on the expected warning, then drop the unexpected warning completely?

Suggested change

Assert.DoesNotContain(unExpectedWarning, logMessage.Message);

If you mean why not do Assert.Equals("NU1234: This is the expected message"), then that's high risk of causing problems for people who wish to contribute to NuGet but don't use English as their locale: NuGet/Home#12820. The risk will significantly increase if we eventually do localization more like the rest of github.com/dotnet repos, since local builds can have the satellite assemblies.

If you mean why not do Assert.Equals("NU1234: This is the expected message")

No, I do not mean that. I expect we would continue using the localized variable.

Looking at the latest commits and this thread, this hasn't been addressed.

Thanks for reviewing! Sorry for taking long to address this. This is fixed.

donnie-msft · 2023-08-31T22:39:28Z

test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommandTests.cs

-            logMessage.Code.Should().Be(NuGetLogCode.NU1803);
-            logMessage.Message.Should().Be("You are running the 'restore' operation with an 'HTTP' source, 'http://api.source/index.json'. Non-HTTPS access will be removed in a future version. Consider migrating to an 'HTTPS' source.");
+
+            string expectedWarning = "You are running the 'restore' operation with an 'HTTP' source, 'http://api.source/index.json'. Non-HTTPS access will be removed in a future version. Consider migrating to an 'HTTPS' source.";


To reduce copying the HTTP source as well as emphasize that your test is expecting the HTTP source URL, I think a variable here would be clearer:

Suggested change

string expectedWarning = "You are running the 'restore' operation with an 'HTTP' source, 'http://api.source/index.json'. Non-HTTPS access will be removed in a future version. Consider migrating to an 'HTTPS' source.";

string expectedWarning = $"You are running the 'restore' operation with an 'HTTP' source, '{testHttpSourceUrl}'. Non-HTTPS access will be removed in a future version. Consider migrating to an 'HTTPS' source.";

Looking at the latest commits and this thread, this hasn't been addressed.

Thanks! This is fixed.

...uGet.Core/NuGet.Commands/RestoreCommand/RequestFactory/DependencyGraphSpecRequestProvider.cs

nkolev92 · 2023-09-01T22:56:40Z

...uGet.Core/NuGet.Commands/RestoreCommand/RequestFactory/DependencyGraphSpecRequestProvider.cs

@@ -214,6 +214,9 @@ private static ExternalProjectReference GetExternalProject(PackageSpec rootProje
            // Add project references
            request.ExternalProjects = projectReferenceClosure.ToList();

+            //Update the RestoreRequest.Project.RestoreMetadata.Sources to get the attributes from settings(e.g. AllowInsecureConnections).
+            request.Project.RestoreMetadata.Sources = sources.Select(s => s.PackageSource).ToList();


This was my bad @heng-liu, I should've done this investigation the first time I provided feedback.

I think we need a slight change here.
I think my original implementation that I added here: 4d2c5a4 is not ideal.

Some background:

PackageSpec, or rather Project is considered to be the model for restore, the input. This will be created differently on the commandline (static graph vs standard) vs Visual Studio. It has a sources property, which is the processed sources based on the project path, RestoreSources msbuild property etc. It's the actual end sources. The general expectation is that the PackageSpec does not get modified. It is basically what is used for no-op.

RestoreRequest has a list of DependencyProviders.
These dependency providers are "shared" across projects, created within this classl,

NuGet.Client/src/NuGet.Core/NuGet.Commands/RestoreCommand/RequestFactory/DependencyGraphSpecRequestProvider.cs

Lines 175 to 182 in 41e98d3

var sharedCache = _providerCache.GetOrCreate(

globalPath,

fallbackPaths.AsList(),

sources,

restoreArgs.CacheContext,

restoreArgs.Log,

updateLastAccess);

. It basically ensures that restore for every project shares the same providers, the same SourceRepository and like resources. These dependency provides also happen to have repositories, and PackageSource that should match the ones from the settings file. Hence the whole GetEffectiveSources thing.

When I added the original implementation, I used _request.Project.RestoreMetadata.Sources. I think that's incorrect. Those are not how the sources are used, they are used through request.DependencyProviders which is effectively based on the PackageSource list from the package spec.

I should've used that and it's already used in a bunch of places, like for example, detecting http sources for telemetry,

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

Line 155 in 41e98d3

int httpSourcesCount = _request.DependencyProviders.RemoteProviders.Where(e => e.IsHttp).Count();

.

Basically, PackageSpec on the request is expected to be immutable, see: NuGet/Home#9041 (comment). We basically clone it a bunch of time because of fear of these types of changes.

Now this is a thing you've never caught yourself, and it's not incorrect to do this, but I think it'll be a smaller change if you just change it to use the dependencyproviders.

You can try that change and see how it goes, tests might break. Like, not sure if everything is mocked 100%. If that becomes the case, please lmk and I can help or just do the change from using the package spec metadata to the dependencyprovider once, since I caused the pain anyways :)

I think the review has been re-requested mistakenly,

Thanks @nkolev92 for the detailed contexts! (I was wondering why the tests for no-op failed previously.)
I changed to use RemoteProviders in a testing branch and the build shows the newly changed test failed. There is no warning in result.LockFile.LogMessages so it failed at this line
However, I can see the warning from the console when running the commandline.
Do you have any suggestions about fixing this test? Thanks!

NuGet.Client/test/TestUtilities/Test.Utility/Commands/ProjectTestHelpers.cs

Line 223 in 29f2929

var sources = new List<PackageSource> { new PackageSource(pathContext.PackageSource) };

has a bug. It's using a hardcoded list where it's using the PathContext source instead of the ones from the package spec. It should be using the PackageSpec ones. I'll do some quick testing to see if I can have a fix.

I'd add a new CreateRestoreRequestMethod, where the source is spec.RestoreMetadata.Sources.

That makes the test pass. Other tests would fail if you make the change in project test helpers directly.
I know how to fix the ones I wrote, but there's many other tests that have package specs that are incomplete.

Actually if you replace that line with:

var sources = spec.RestoreMetadata.Sources.Any() ? spec.RestoreMetadata.Sources : new List<PackageSource> { new PackageSource(pathContext.PackageSource) };

All tests seem to pass.

If you end up choosing that, please create an issue to fix the tests and assign to me. I think our test helpers code should use as much of the production code as possible. I'm hoping to do some changes in the ProjectTestHelpers that'll help us avoid these problems.

I spent about an hour doing some refactoring: https://github.com/NuGet/NuGet.Client/tree/dev-nkolev92-cleanupTestHelpers. Feel free to make the change to fix your tests.

I have #5407 out.

Thanks @nkolev92 for your help! Please take another look.

donnie-msft

I re-reviewed as requested by @jeffkl, but after looking at the latest commits and my unresolved threads, there are unaddressed comments by me.

This reverts commit 7a9ce43.

This reverts commit b763608.

nkolev92

🚢

nkolev92 · 2023-09-11T14:47:07Z

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

@@ -239,11 +239,12 @@ public async Task<RestoreResult> ExecuteAsync(CancellationToken token)
                    _success = false;
                }

-                if (_request.Project?.RestoreMetadata != null)
+                if (_request.DependencyProviders.RemoteProviders != null)


Excellent!

Thanks for fixing my mistake :)

heng-liu changed the base branch from dev to dev-feature-optoutHttpWarnings August 30, 2023 21:35

heng-liu marked this pull request as ready for review August 30, 2023 21:36

heng-liu requested a review from a team as a code owner August 30, 2023 21:36

nkolev92 reviewed Aug 31, 2023

View reviewed changes

...uGet.Core/NuGet.Commands/RestoreCommand/RequestFactory/DependencyGraphSpecRequestProvider.cs Outdated Show resolved Hide resolved

donnie-msft reviewed Aug 31, 2023

View reviewed changes

nkolev92 reviewed Sep 1, 2023

View reviewed changes

jeffkl requested review from nkolev92, donnie-msft and zivkan September 5, 2023 17:41

donnie-msft reviewed Sep 6, 2023

View reviewed changes

nkolev92 mentioned this pull request Sep 7, 2023

Simplify ProjectTestHelpers, unify RestoreRequest and DGSpec creation #5407

Merged

8 tasks

heng-liu force-pushed the dev-feature-optoutHttpWarnings branch from b1734b8 to 7c6c3e3 Compare September 8, 2023 00:58

heng-liu added 8 commits September 7, 2023 18:08

Opt out Https everywhere in package referrence restore

83c45af

fix tests

4d71fce

fix

66ec972

fix

b00c95b

update the whole Sources

9e40a7c

fix

9c5d31f

Revert "fix"

6bfbd4e

This reverts commit 7a9ce43.

use RemoteProviders when checking http source

6f8a363

heng-liu force-pushed the dev-hengliu-optoutHttpWarnings-packageReferrenceRestore branch from 7a9ce43 to 6f8a363 Compare September 8, 2023 01:09

address feedback

6c7ab1a

aortiz-msft added the Merge this release label Sep 9, 2023

heng-liu added 2 commits September 9, 2023 13:47

fix TestRestoreRequest

5d47d80

add a test for sdkresolver

b763608

heng-liu requested a review from donnie-msft September 10, 2023 21:18

Revert "add a test for sdkresolver"

a1f191b

This reverts commit b763608.

nkolev92 approved these changes Sep 11, 2023

View reviewed changes

heng-liu merged commit 47abced into dev-feature-optoutHttpWarnings Sep 11, 2023

heng-liu deleted the dev-hengliu-optoutHttpWarnings-packageReferrenceRestore branch September 11, 2023 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not warn http sources in package reference restore when allowInsecureConnections is set to true #5390

Do not warn http sources in package reference restore when allowInsecureConnections is set to true #5390

heng-liu commented Aug 30, 2023 •

edited

Loading

donnie-msft Aug 31, 2023

zivkan Sep 1, 2023

donnie-msft Sep 1, 2023

donnie-msft Sep 6, 2023 •

edited

Loading

heng-liu Sep 10, 2023

donnie-msft Aug 31, 2023

donnie-msft Sep 6, 2023 •

edited

Loading

heng-liu Sep 10, 2023

nkolev92 Sep 1, 2023

nkolev92 Sep 5, 2023

heng-liu Sep 6, 2023

nkolev92 Sep 6, 2023

nkolev92 Sep 6, 2023

nkolev92 Sep 6, 2023

nkolev92 Sep 6, 2023

nkolev92 Sep 7, 2023 •

edited

Loading

nkolev92 Sep 7, 2023

heng-liu Sep 10, 2023

donnie-msft left a comment

nkolev92 left a comment

nkolev92 Sep 11, 2023

	string expectedWarning = "You are running the 'restore' operation with an 'HTTP' source, 'http://api.source/index.json'. Non-HTTPS access will be removed in a future version. Consider migrating to an 'HTTPS' source.";
	string expectedWarning = $"You are running the 'restore' operation with an 'HTTP' source, '{testHttpSourceUrl}'. Non-HTTPS access will be removed in a future version. Consider migrating to an 'HTTPS' source.";

	var sharedCache = _providerCache.GetOrCreate(
	globalPath,
	fallbackPaths.AsList(),
	sources,
	restoreArgs.CacheContext,
	restoreArgs.Log,
	updateLastAccess);

Do not warn http sources in package reference restore when allowInsecureConnections is set to true #5390

Do not warn http sources in package reference restore when allowInsecureConnections is set to true #5390

Conversation

heng-liu commented Aug 30, 2023 • edited Loading

Bug

Description

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donnie-msft Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donnie-msft Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donnie-msft left a comment

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heng-liu commented Aug 30, 2023 •

edited

Loading

donnie-msft Sep 6, 2023 •

edited

Loading

donnie-msft Sep 6, 2023 •

edited

Loading

nkolev92 Sep 7, 2023 •

edited

Loading