[Feature] Implement Support for NuGet Authentication Plugins as .NET Tools #6138
Conversation
|
I will address the two comments regarding improving |
Nigusu-Allehu
changed the title
| if (Directory.Exists(directoryPath)) | ||
| { | ||
| var directoryInfo = new DirectoryInfo(directoryPath); | ||
| var files = directoryInfo.GetFiles("nuget-plugin-*"); |
There was a problem hiding this comment.
I believe the case sensitivity of this match will be platform-dependent, meaning on Windows it will be commonly (not guaranteed) case insensitive, but on Linux it will be case sensitive.
However, the file name comparison here is always case-insensitive.
Can you explain your reasoning?
There was a problem hiding this comment.
You are right, that's inconsistent. I will address this in a follow up PR.
| using (var process = new System.Diagnostics.Process()) | ||
| { | ||
| // Use a shell command to check if the file is executable | ||
| process.StartInfo.FileName = "/bin/bash"; |
There was a problem hiding this comment.
This code could run on Alpine Linux, where /bin/bash does not exist by default.
Is there an even more portable way of checking? I don't know, but maybe ls -l <path>?
There was a problem hiding this comment.
On unix systems, FileInfo (and DirectoryInfo) has a UnixFileMode property that is a flags enum with the typical unix properties. You can use HasFlag or your favorite enum comparisons to see if a given file has any executable flags:
$ dotnet fsi
Microsoft (R) F# Interactive version 12.8.102.0 for F# 8.0
Copyright (c) Microsoft Corporation. All Rights Reserved.
For help type #help;;
> open System.IO;;
> let f = new FileInfo("/usr/bin/curl");;
val f: FileInfo = /usr/bin/curl
> f.UnixFileMode;;
val it: UnixFileMode =
OtherExecute, OtherRead, GroupExecute, GroupRead, UserExecute, UserWrite, UserRead
There was a problem hiding this comment.
This code is in a #if !NET8_0_OR_GREATER code block because the APIs that @baronfel mentioned are not available before .NET 8. When @Nigusu-Allehu created the original PR to add this code to the feature branch (this PR is merging the feature branch into dev) I asked him to use the .NET 8 APIs already, which is being done on line 303.
I also suggested to @Nigusu-Allehu that we could consider skipping this PATH scanning for plugins for .NET Framework on Linux and Mac, since that only means Mono, which we don't officially support, but I don't remember any replies to that comment, and clearly it wasn't actioned either.
But I think it's also ok to keep here, because I really can't imagine anyone using Alpine wanting to install Mono and use NuGet.exe. And even if they do, the whole thing is wrapped in a try-catch block. The plugin won't be discovered, but it won't break restore either (apart from not being able to authenticate if a nuget-plugin-* app is installed)
| IPlugin plugin; | ||
|
|
||
| plugin = await _pluginFactory.GetOrCreateAsync( |
There was a problem hiding this comment.
| IPlugin plugin; | |
| plugin = await _pluginFactory.GetOrCreateAsync( | |
| IPlugin plugin = await _pluginFactory.GetOrCreateAsync( |
| var reqHandler = new RequestHandlers(); | ||
| var options = ConnectionOptions.CreateDefault(); | ||
|
|
||
| var pluginFactory = new PluginFactory(Timeout.InfiniteTimeSpan); |
There was a problem hiding this comment.
This is disposable and should be disposed.
| @@ -66,9 +66,21 @@ internal TestExpectation( | |||
|
|
|||
| internal sealed class PluginManagerMock : IDisposable | |||
| { | |||
| public static bool IsDesktop | |||
There was a problem hiding this comment.
I think all of these IsDesktop properties would be better off named RequiresDotNetHost, since that's how it's used. IsDesktop is a little more difficult to understand than RequiresDotNetHost. It would simplify call sites too. Instead of requiresDotnetHost: !IsDesktop, you could simply pass RequiresDotNetHost.
Your call though.
| Exception exception = Record.Exception(() => | ||
| { | ||
| } | ||
| using (new PluginDiscoverer()) | ||
| { | ||
| } | ||
| }); | ||
|
|
||
| Assert.Null(exception); |
There was a problem hiding this comment.
Why not simply?
using (new PluginDiscoverer())
{
}An exception would fail the test.
There was a problem hiding this comment.
| @@ -11,20 +11,20 @@ namespace NuGet.Protocol.Plugins | |||
| /// <summary> | |||
| /// A plugin factory. | |||
| /// </summary> | |||
| public interface IPluginFactory : IDisposable | |||
| internal interface IPluginFactory : IDisposable | |||
There was a problem hiding this comment.
Can you explain this change?
There was a problem hiding this comment.
We had to update the API for IPluginFactory so that it is able to create plugins for dotnet tools plugins. Since we needed to break the API, I used it as an opportunity to un-expose the interface. The reason for making it internal is that the interface only has one implementation and there is no need for it to be a public API #6113 (comment)
|
Thank you for the review @dtivel! I will address the comments in a follow up PR. |
Bug
Fixes: NuGet/Home#12567
Fixes: NuGet/Home#13742
Description
This PR implements support for deploying and discovering NuGet authentication plugins as .NET tools, as outlined in the design. With this change, plugin authors can package their tools as global .NET tools by naming them with the prefix nuget-plugin-*, enabling NuGet to discover and execute them by scanning the PATH. On Windows, .exe and .bat extensions are recognized, while Linux/Mac platforms identify plugins via the executable bit.
This is a feature branch PR.
Here are the individual commits into the feature branch
#5990
#6113
PR Checklist