Implement dotnet nuget trust command by erdembayar · Pull Request #3989 · NuGet/NuGet.Client

erdembayar · 2021-04-09T16:59:38Z

Bug

Fixes: NuGet/Home#8053

Regression? Last working version: No

Description

Implement "dotnet nuget trust" command according to Nuget.exe command "nuget trusted-signers". Spec is here.
Add tests in \test\NuGet.Core.FuncTests\Dotnet.Integration.Test\DotnetTrustTests.cs
Add tests in \test\NuGet.Core.FuncTests\NuGet.XPlat.FuncTest\XPlatTrustTests.cs

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added
- OR
- Test exception
- OR
- N/A
Documentation
- Documentation PR or issue filled
  - https://github.com/NuGet/docs.microsoft.com-nuget/issues/2388
- OR
- N/A

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatTrustTests.cs

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx

…et trusted-signers"

kartheekp-ms

Left few comments to start with.

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/TrustedSignersCommandTests.cs

erdembayar

@kartheekp-ms
Thank you for your review. I addressed your comments, please check.

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/TrustedSignersCommandTests.cs

erdembayar · 2021-04-20T21:05:13Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Program.cs

+                    {
+                        var baseException = e.GetBaseException();
+
+                        if (baseException is TrustedSignerAlreadyExistsException


Reason I added this part is to make experience same as nuget.exe.
For example if same trusted signed already exist then it just show A trusted signer 'NewtonAuthor333' already exists.

Without this check dotnet nuget trust 'll show red marked part assuming it's unhandled.

I really wish we weren't doing this in the first place. Printing out help doesn't help at all unless the error itself has to do with a command, argument, or option being invalid.

Anyway, I don't think we should do this, and we should start a separate conversation for not calling ShowBestHelp() in every case, and narrowing it down, instead of creating an allowlist like this that definitely does not consistently cover other errors in dotnet.exe nuget commands.

So tl;dr: I see where you're coming from, and it's good that you're aiming to maintain nuget.exe similarity here, but I think "fit in to dotnet.exe" should be higher priority, and also I just don't like this specific approach to it.

Ok. Yes, I agree it needs to be addressed separately.
I removed this part and created NuGet/Home#10788 issue to track it.

zkat

So far so good! A few questions, and some comments on stuff I think should be changed. Great work!

zkat · 2021-04-21T00:10:24Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Program.cs

+                    {
+                        var baseException = e.GetBaseException();
+
+                        if (baseException is TrustedSignerAlreadyExistsException


I really wish we weren't doing this in the first place. Printing out help doesn't help at all unless the error itself has to do with a command, argument, or option being invalid.

zkat · 2021-04-21T00:13:45Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Program.cs

+                    {
+                        var baseException = e.GetBaseException();
+
+                        if (baseException is TrustedSignerAlreadyExistsException


Anyway, I don't think we should do this, and we should start a separate conversation for not calling ShowBestHelp() in every case, and narrowing it down, instead of creating an allowlist like this that definitely does not consistently cover other errors in dotnet.exe nuget commands.

So tl;dr: I see where you're coming from, and it's good that you're aiming to maintain nuget.exe similarity here, but I think "fit in to dotnet.exe" should be higher priority, and also I just don't like this specific approach to it.

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetTrustTests.cs

erdembayar

@zkat
Thank you for your review.
I addressed your PR comment concerns. Please review again.

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetTrustTests.cs

erdembayar · 2021-04-21T18:01:50Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Program.cs

+                    {
+                        var baseException = e.GetBaseException();
+
+                        if (baseException is TrustedSignerAlreadyExistsException


Ok. Yes, I agree it needs to be addressed separately.
I removed this part and created NuGet/Home#10788 issue to track it.

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetTrustTests.cs

heng-liu

Thanks for implementing the trusted-signers command!

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetTrustTests.cs

src/NuGet.Core/NuGet.CommandLine.XPlat/Program.cs

erdembayar

@heng-liu
Thank you for your review. I addressed your comments, please check.

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetTrustTests.cs

src/NuGet.Core/NuGet.CommandLine.XPlat/Program.cs

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetTrustTests.cs

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

dtivel · 2021-04-22T13:35:22Z

src/NuGet.Core/NuGet.Commands/TrustedSignersCommand/TrustedSignerActionsProvider.cs

                    _trustedSignersProvider.AddOrUpdateTrustedSigner(existingRepository);

-                    await _logger.LogAsync(LogLevel.Information, string.Format(CultureInfo.CurrentCulture, Strings.SuccessfullySynchronizedTrustedRepository, name));
+                    await _logger.LogAsync(LogLevel.Minimal, string.Format(CultureInfo.CurrentCulture, Strings.SuccessfullySynchronizedTrustedRepository, name));


Can you elaborate on the before and after results of logging level changes in this and other files? What's the impact in NuGet.exe too?

I get this idea from @kartheekp-ms 's recent PR#3992
Without this change dotnet nuget trust command show nothing on cli with successful action. Only show something if there is error/warning. Technically it's too quite due to differences in msbuild and nuget.exe loge levels.
With this change we can see result exactly same as NuGet.exe for successful action. I believe there is no impact on NuGet.exe.

@kartheekp-ms Also can comment on this.

I believe there is no impact on NuGet.exe.

Isn't it the same code path for both dotnet and NuGet.exe? How could the change affect only dotnet and not NuGet.exe? I'm trying to understand.

@dtivel @kartheekp-ms
If no verbosity is passed then verbosity default to LogLevel.Minimal:

NuGet.Client/src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/XPlatUtility.cs

Lines 42 to 43 in 5138176

default:

return LogLevel.Minimal;

But this log is logged with LogLevel.Information wouldn't displayed due to below level check:

NuGet.Client/src/NuGet.Core/NuGet.Common/Logging/LoggerBase.cs

Lines 81 to 84 in 5138176

protected virtual bool DisplayMessage(LogLevel messageLevel)

{

return (messageLevel >= VerbosityLevel);

}

After thinking instead of making change here in TrustedSignerActionsProvider.cs, how about I change below?

NuGet.Client/src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

Line 115 in 82ae3f5

setLogLevel(XPlatUtility.MSBuildVerbosityToNuGetLogLevel(verbosity.Value()));

To:

// If not verbosity passed then it defaults LogLevel.Minimal which result no message displayed because successfull message are logged at Loglevel.Information. // Since NuGet LogLevel.Information is equal to MSBuild normal, we need to defaults "N" setLogLevel(XPlatUtility.MSBuildVerbosityToNuGetLogLevel(verbosity.Value() ?? "N"));

@kartheekp-ms
I just synched offline with @dtivel.
I'll leave code as it's for now, but I'll post table shows if there is any change of user experience in output.

I agree that a table showing what is displayed for each verbosity level is helpful. For e.g., https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-nuget-verify#options

@dtivel @kartheekp-ms
I created comparison table between latest nuget(v5.9) and nugetcandidate(compiled from my code), dotnet nuget trust(compiled from my code). So far I don't see any regression on nuget.exe usage experience. Please note dotnet nuget trust is bit different in handling error cases than nuget.exe, in this table I still assumed as same, but dotnet nuget trust it's not objective of this table, it's created to make sure nuget(v5.9) and nugetcandidate experience are still same. You can ignore dotnet column. Also you can press on ✔️ links to see actual output behind it, some outputs are combined for all 3, for example for q mode since there is no output.

Action nuget59 nugetCandidate dotnet

nuget59 trusted-signer list ✔️ ✔️ ✔️

nuget59 trusted-signer list -verbosity q ✔️ ✔️ ✔️

nuget59 trusted-signer list -verbosity n ✔️ ✔️ ✔️

nuget59 trusted-signer list -verbosity d ✔️ ✔️ ✔️

nuget59 trusted-signers Add -Name NuGet -config ..\nuget.config ✔️ ✔️ ✔️

nuget59 trusted-signers add .\newtonsoft.json.13.0.1-beta1.nupkg -Name NugetOrg -Repository -config ..\nuget.config ✔️ ✔️ ✔️

nuget59 trusted-signers Add -Name MyCompanyCert -CertificateFingerprint CE40881FF5F0AD3E58965DA20A9F571EF1651A56933748E1BF1C99E537C4E039 -FingerprintAlgorithm SHA256 -config ..\nuget.config ✔️ ✔️ ✔️

nuget59 trusted-signers Add -Name NuGetTrust -ServiceIndex https://api.nuget.org/v3/index.json -config ..\nuget.config -Owners "NugetTestOrg;MSTestOrg;TrustedTesOwners" -config ..\nuget.config ✔️ ✔️ ✔️

nuget59 trusted-signers sync -Name NuGetTrust -config ..\nuget.config ✔️ ✔️ ✔️

nuget59 trusted-signers Remove -Name NugetTrust -config ..\nuget.config ✔️ ✔️ ✔️

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetTrustTests.cs

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatTrustTests.cs

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetTrustTests.cs

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

kartheekp-ms · 2021-04-23T19:26:29Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

+                    {
+                        if (action == TrustCommand.Author || action == TrustCommand.Repository)
+                        {
+                            packagePath = command.Values[2];


As per spec, For trust author and trust repository, <package> may be a glob/wildcard. This must be consistent across platforms, so the Windows version must expand these wildcards when it receives them as arguments.. The way I am reading this is the command should accept multiple packagePaths. If I understand the current logic correctly, only one value is accepted for package path. The following command should be supported.

dotnet nuget trust author Contoso ./contoso.library1.nupkg ./contoso.library2.nupkg

When I implemented dotnet nuget verify command, I changed VerifyArgs as shown below to accept multiple packagePaths.

NuGet.Client/src/NuGet.Core/NuGet.Commands/VerifyCommand/VerifyArgs.cs

Lines 29 to 57 in df3e31c

/// <summary>

/// Path to the package that has to be verified.

/// </summary>

[Obsolete("Use PackagePaths instead")]

public string PackagePath

{

get

{

switch (PackagePaths.Count)

{

case 0:

return null;

case 1:

return PackagePaths[0];

default:

throw new NotSupportedException(string.Format(CultureInfo.CurrentCulture,

Strings.Error_MultiplePackagePaths,

nameof(PackagePaths)));

}

}

set => PackagePaths = new[] { value };

}

/// <summary>

/// Paths to the packages that has to be verified.

/// </summary>

public IReadOnlyList<string> PackagePaths { get; set; }

I recently created this PR#3993 for nuget.exe, as you can see based on other trust related commands doesn't accept multiple files we decided to accept only 1 file at the time. So I believe it applies here too. Please note that experience didn't work for nuget.exe even before my change, so there is no backward compat issue nor breaking experience.

But dotnet nuget verify case is different since it's just outputting result on cli and not mutating state of machine config, so it's ok.

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs

kartheekp-ms · 2021-04-23T19:59:39Z

src/NuGet.Core/NuGet.Commands/TrustedSignersCommand/TrustedSignerActionsProvider.cs

                    _trustedSignersProvider.AddOrUpdateTrustedSigner(existingRepository);

-                    await _logger.LogAsync(LogLevel.Information, string.Format(CultureInfo.CurrentCulture, Strings.SuccessfullySynchronizedTrustedRepository, name));
+                    await _logger.LogAsync(LogLevel.Minimal, string.Format(CultureInfo.CurrentCulture, Strings.SuccessfullySynchronizedTrustedRepository, name));


I agree that a table showing what is displayed for each verbosity level is helpful. For e.g., https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-nuget-verify#options

kartheekp-ms · 2021-04-23T20:28:06Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetTrustTests.cs

+        [CIOnlyTheory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public async Task DotnetTrust_AuthorAction_RelativePathConfileFile_Succeeds(bool allowUntrustedRoot)


nit : test case name DotnetTrust_AuthorAction_RelativePathConfigFile_Succeeds

Ok. I'll address later in another PR.

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetTrustTests.cs

erdembayar marked this pull request as ready for review April 10, 2021 03:59

erdembayar requested a review from a team as a code owner April 10, 2021 03:59

donnie-msft reviewed Apr 10, 2021

View reviewed changes

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatTrustTests.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx Outdated Show resolved Hide resolved

erdembayar added 11 commits April 10, 2021 10:12

Start "dotnet nuget trust" command implementation in parity with "nug…

b08ce62

…et trusted-signers"

Remove unused items.

07d03ad

Correct descriptions of dotnet nuget trust command

a24670a

Add dotnet integration tests.

5d24611

Fix tests 1

1b8d557

Fix test2

b59e0e6

Increase timeout

62cccd8

Fix timeout issue.

7fe3cb1

Add functional test for 'dotnet nuget trust' command

ddf5700

Fix integration test.

621c0a8

Clean up

3eb5aba

erdembayar force-pushed the dev-eryondon-8053DotnetTrustedSignersCommand branch from c26bd42 to 3eb5aba Compare April 10, 2021 17:12

erdembayar added 3 commits April 10, 2021 11:05

Address PR comment by Donnie.

42b452d

Add verbosity level test.

7c09d66

Add normal verbosity if non passed.

4439901

erdembayar force-pushed the dev-eryondon-8053DotnetTrustedSignersCommand branch from 9f5ce09 to 4439901 Compare April 12, 2021 20:10

kartheekp-ms reviewed Apr 14, 2021

View reviewed changes

Address Karteek PR comments.

df61cd3

erdembayar commented Apr 14, 2021

View reviewed changes

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs Outdated Show resolved Hide resolved

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/TrustedSignersCommandTests.cs Show resolved Hide resolved

Address Karteek's PR comment about msbuild logger level.

c4f1104

erdembayar commented Apr 20, 2021

View reviewed changes

zkat suggested changes Apr 21, 2021

View reviewed changes

Address PR comments by Kat.

b408441

erdembayar commented Apr 21, 2021

View reviewed changes

erdembayar force-pushed the dev-eryondon-8053DotnetTrustedSignersCommand branch from 3800278 to b408441 Compare April 21, 2021 18:23

Fix unwanted change.

983e0e4

erdembayar requested a review from kartheekp-ms April 21, 2021 19:05

zkat approved these changes Apr 21, 2021

View reviewed changes

heng-liu reviewed Apr 22, 2021

View reviewed changes

Address PR comments by Heng.

d3880d0

erdembayar commented Apr 22, 2021

View reviewed changes

dtivel reviewed Apr 22, 2021

View reviewed changes

erdembayar added 2 commits April 22, 2021 14:56

Address Damon's PR comments.

ba1bcd3

Fix typos.

2eb4621

heng-liu approved these changes Apr 22, 2021

View reviewed changes

erdembayar added 2 commits April 22, 2021 20:12

Address Heng's PR comment about integration test naming.

4dd5c1a

Fix Linux test failure.

82ae3f5

kartheekp-ms reviewed Apr 23, 2021

View reviewed changes

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetTrustTests.cs Show resolved Hide resolved

kartheekp-ms reviewed Apr 23, 2021

View reviewed changes

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Signing/TrustedSignersCommand.cs Show resolved Hide resolved

kartheekp-ms reviewed Apr 23, 2021

View reviewed changes

dtivel approved these changes Apr 24, 2021

View reviewed changes

erdembayar mentioned this pull request Apr 24, 2021

Show subcommand help instead of main dotnet nuget trust command help for all cases NuGet/Home#10788

Closed

erdembayar merged commit 83d3960 into dev Apr 24, 2021

erdembayar deleted the dev-eryondon-8053DotnetTrustedSignersCommand branch April 24, 2021 01:30

erdembayar mentioned this pull request Apr 26, 2021

dotnet nuget trust command dotnet/sdk#17200

Closed

erdembayar mentioned this pull request Jun 2, 2021

Add documentation for dotnet nuget trust command dotnet/docs#24503

Merged

	protected virtual bool DisplayMessage(LogLevel messageLevel)
	{
	return (messageLevel >= VerbosityLevel);
	}

Action	nuget59	nugetCandidate	dotnet
nuget59 trusted-signer list	✔️	✔️	✔️
nuget59 trusted-signer list -verbosity q	✔️	✔️	✔️
nuget59 trusted-signer list -verbosity n	✔️	✔️	✔️
nuget59 trusted-signer list -verbosity d	✔️	✔️	✔️
nuget59 trusted-signers Add -Name NuGet -config ..\nuget.config	✔️	✔️	✔️
nuget59 trusted-signers add .\newtonsoft.json.13.0.1-beta1.nupkg -Name NugetOrg -Repository -config ..\nuget.config	✔️	✔️	✔️
nuget59 trusted-signers Add -Name MyCompanyCert -CertificateFingerprint CE40881FF5F0AD3E58965DA20A9F571EF1651A56933748E1BF1C99E537C4E039 -FingerprintAlgorithm SHA256 -config ..\nuget.config	✔️	✔️	✔️
nuget59 trusted-signers Add -Name NuGetTrust -ServiceIndex https://api.nuget.org/v3/index.json -config ..\nuget.config -Owners "NugetTestOrg;MSTestOrg;TrustedTesOwners" -config ..\nuget.config	✔️	✔️	✔️
nuget59 trusted-signers sync -Name NuGetTrust -config ..\nuget.config	✔️	✔️	✔️
nuget59 trusted-signers Remove -Name NugetTrust -config ..\nuget.config	✔️	✔️	✔️

	/// <summary>
	/// Path to the package that has to be verified.
	/// </summary>
	[Obsolete("Use PackagePaths instead")]
	public string PackagePath
	{
	get
	{
	switch (PackagePaths.Count)
	{
	case 0:
	return null;

	case 1:
	return PackagePaths[0];

	default:
	throw new NotSupportedException(string.Format(CultureInfo.CurrentCulture,
	Strings.Error_MultiplePackagePaths,
	nameof(PackagePaths)));
	}
	}
	set => PackagePaths = new[] { value };
	}

	/// <summary>
	/// Paths to the packages that has to be verified.
	/// </summary>
	public IReadOnlyList<string> PackagePaths { get; set; }

Conversation

erdembayar commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Description

PR Checklist

Uh oh!

Uh oh!

Uh oh!

kartheekp-ms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erdembayar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zkat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erdembayar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

heng-liu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erdembayar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erdembayar Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erdembayar Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

erdembayar commented Apr 9, 2021 •

edited

Loading

erdembayar Apr 22, 2021 •

edited

Loading

erdembayar Apr 23, 2021 •

edited

Loading

erdembayar Apr 23, 2021 •

edited

Loading