Skip to content
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 an allowInsecureConnections property in NuGet.Config #5343

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ private IReadOnlyList<PackageSource> GetPackageSourcesToUpdate(IReadOnlyList<Pac
if (packageSource.Name.Equals(packageSourceContextInfo.Name, StringComparison.InvariantCulture)
&& packageSource.Source.Equals(packageSourceContextInfo.Source, StringComparison.InvariantCulture)
&& packageSource.ProtocolVersion == packageSourceContextInfo.ProtocolVersion
&& packageSource.AllowInsecureConnections == packageSourceContextInfo.AllowInsecureConnections
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above, which I don't think is useful, is now inaccurate.

  1. // If key properties have not changed, we don't need to do anything
  2. // If identifying properties have not changed, we don't need to do anything
  3. // If Name/Source/IsEnabled/ProtocolVersion/AllowInsecureConnections has not changed, we don't need to do anything
  4. delete the comment

I vote for 4 or 1.

&& packageSource.IsEnabled == packageSourceContextInfo.IsEnabled)
{
newPackageSources.Add(packageSource);
Expand All @@ -111,6 +112,7 @@ private IReadOnlyList<PackageSource> GetPackageSourcesToUpdate(IReadOnlyList<Pac
ClientCertificates = packageSource.ClientCertificates,
Description = packageSource.Description,
ProtocolVersion = packageSourceContextInfo.ProtocolVersion,
AllowInsecureConnections = packageSourceContextInfo.AllowInsecureConnections,
MaxHttpRequestsPerSource = packageSource.MaxHttpRequestsPerSource,
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ public PackageSourceContextInfo(string source, string name, bool isEnabled)
}

public PackageSourceContextInfo(string source, string name, bool isEnabled, int protocolVersion)
: this(source, name, isEnabled, protocolVersion, allowInsecureConnections: false)
{
}

public PackageSourceContextInfo(string source, string name, bool isEnabled, int protocolVersion, bool allowInsecureConnections)
{
Assumes.NotNullOrEmpty(name);
Assumes.NotNullOrEmpty(source);
Expand All @@ -36,18 +41,21 @@ public PackageSourceContextInfo(string source, string name, bool isEnabled, int
Source = source;
IsEnabled = isEnabled;
ProtocolVersion = protocolVersion;
AllowInsecureConnections = allowInsecureConnections;

var hash = new HashCodeCombiner();
hash.AddStringIgnoreCase(Name);
hash.AddStringIgnoreCase(Source);
hash.AddObject(ProtocolVersion);
hash.AddObject(AllowInsecureConnections);
_hashCode = hash.CombinedHash;
OriginalHashCode = _hashCode;
}

public string Name { get; set; }
public string Source { get; set; }
public int ProtocolVersion { get; set; }
public bool AllowInsecureConnections { get; set; }
public bool IsMachineWide { get; internal set; }
public bool IsEnabled { get; set; }
public string? Description { get; internal set; }
Expand Down Expand Up @@ -86,7 +94,7 @@ public override int GetHashCode()

public PackageSourceContextInfo Clone()
{
return new PackageSourceContextInfo(Source, Name, IsEnabled, ProtocolVersion)
return new PackageSourceContextInfo(Source, Name, IsEnabled, ProtocolVersion, AllowInsecureConnections)
{
IsMachineWide = IsMachineWide,
Description = Description,
Expand All @@ -96,7 +104,7 @@ public PackageSourceContextInfo Clone()

public static PackageSourceContextInfo Create(PackageSource packageSource)
{
return new PackageSourceContextInfo(packageSource.Source, packageSource.Name, packageSource.IsEnabled, packageSource.ProtocolVersion)
return new PackageSourceContextInfo(packageSource.Source, packageSource.Name, packageSource.IsEnabled, packageSource.ProtocolVersion, packageSource.AllowInsecureConnections)
{
IsMachineWide = packageSource.IsMachineWide,
Description = packageSource.Description,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ internal sealed class PackageSourceContextInfoFormatter : NuGetMessagePackFormat
private const string SourcePropertyName = "source";
private const string IsEnabledPropertyName = "isenabled";
private const string ProtocolVersionPropertyName = "protocolversion";
private const string AllowInsecureConnectionsPropertyName = "allowInsecureConnections";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your use of camel-casing better, but should this be lowercase to be consistent?

private const string IsMachineWidePropertyName = "ismachinewide";
private const string NamePropertyName = "name";
private const string DescriptionPropertyName = "description";
Expand All @@ -33,6 +34,7 @@ private PackageSourceContextInfoFormatter()
string? description = null;
int originalHashCode = 0;
int protocolVersion = PackageSource.DefaultProtocolVersion;
bool allowInsecureConnections = false;

int propertyCount = reader.ReadMapHeader();
for (int propertyIndex = 0; propertyIndex < propertyCount; propertyIndex++)
Expand Down Expand Up @@ -60,6 +62,9 @@ private PackageSourceContextInfoFormatter()
case ProtocolVersionPropertyName:
protocolVersion = reader.ReadInt32();
break;
case AllowInsecureConnectionsPropertyName:
allowInsecureConnections = reader.ReadBoolean();
break;
default:
reader.Skip();
break;
Expand All @@ -69,7 +74,7 @@ private PackageSourceContextInfoFormatter()
Assumes.NotNullOrEmpty(source);
Assumes.NotNullOrEmpty(name);

return new PackageSourceContextInfo(source, name, isEnabled, protocolVersion)
return new PackageSourceContextInfo(source, name, isEnabled, protocolVersion, allowInsecureConnections)
{
IsMachineWide = isMachineWide,
Description = description,
Expand All @@ -79,11 +84,13 @@ private PackageSourceContextInfoFormatter()

protected override void SerializeCore(ref MessagePackWriter writer, PackageSourceContextInfo value, MessagePackSerializerOptions options)
{
writer.WriteMapHeader(count: 7);
writer.WriteMapHeader(count: 8);
writer.Write(SourcePropertyName);
writer.Write(value.Source);
writer.Write(ProtocolVersionPropertyName);
writer.Write(value.ProtocolVersion);
writer.Write(AllowInsecureConnectionsPropertyName);
writer.Write(value.AllowInsecureConnections);
writer.Write(IsEnabledPropertyName);
writer.Write(value.IsEnabled);
writer.Write(IsMachineWidePropertyName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ NuGet.VisualStudio.Internal.Contracts.INuGetProjectManagerService.GetInstallActi
NuGet.VisualStudio.Internal.Contracts.INuGetSearchService.GetAllPackageFoldersAsync(System.Collections.Generic.IReadOnlyCollection<NuGet.VisualStudio.Internal.Contracts.IProjectContextInfo!>! projectContextInfos, System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.Task<System.Collections.Generic.IReadOnlyList<NuGet.Protocol.Core.Types.SourceRepository!>!>!
NuGet.VisualStudio.Internal.Contracts.IPackageReferenceContextInfo.VersionOverride.get -> NuGet.Versioning.VersionRange?
NuGet.VisualStudio.Internal.Contracts.PackageReferenceContextInfo.VersionOverride.get -> NuGet.Versioning.VersionRange?
NuGet.VisualStudio.Internal.Contracts.PackageSourceContextInfo.AllowInsecureConnections.get -> bool
NuGet.VisualStudio.Internal.Contracts.PackageSourceContextInfo.AllowInsecureConnections.set -> void
NuGet.VisualStudio.Internal.Contracts.PackageSourceContextInfo.PackageSourceContextInfo(string! source, string! name, bool isEnabled, int protocolVersion) -> void
NuGet.VisualStudio.Internal.Contracts.PackageSourceContextInfo.PackageSourceContextInfo(string! source, string! name, bool isEnabled, int protocolVersion, bool allowInsecureConnections) -> void
NuGet.VisualStudio.Internal.Contracts.PackageSourceContextInfo.ProtocolVersion.get -> int
NuGet.VisualStudio.Internal.Contracts.PackageSourceContextInfo.ProtocolVersion.set -> void
NuGet.VisualStudio.Internal.Contracts.TransitivePackageReferenceContextInfo.VersionOverride.get -> NuGet.Versioning.VersionRange?
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ public class PackageSource : IEquatable<PackageSource>
/// </summary>
public const int DefaultProtocolVersion = 2;

internal const bool DefaultAllowInsecureConnections = false;

private int _hashCode;
private string _source;
private bool _isHttp;
Expand Down Expand Up @@ -101,6 +103,11 @@ public string Source
/// </summary>
public int ProtocolVersion { get; set; } = DefaultProtocolVersion;

/// <summary>
/// Gets or sets the allowInsecureConnections of the source. Defaults to false.
/// </summary>
public bool AllowInsecureConnections { get; set; } = DefaultAllowInsecureConnections;

/// <summary>
/// Whether the source is using the HTTP protocol, including HTTPS.
/// </summary>
Expand Down Expand Up @@ -152,7 +159,13 @@ public SourceItem AsSourceItem()
{
protocolVersion = $"{ProtocolVersion}";
}
return new SourceItem(Name, Source, protocolVersion);

string? allowInsecureConnections = null;
if (AllowInsecureConnections != DefaultAllowInsecureConnections)
{
allowInsecureConnections = $"{AllowInsecureConnections}";
}
return new SourceItem(Name, Source, protocolVersion, allowInsecureConnections);
}

public bool Equals(PackageSource? other)
Expand Down Expand Up @@ -189,6 +202,7 @@ public PackageSource Clone()
ClientCertificates = ClientCertificates?.ToList(),
IsMachineWide = IsMachineWide,
ProtocolVersion = ProtocolVersion,
AllowInsecureConnections = AllowInsecureConnections,
};
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ private static PackageSource ReadPackageSource(SourceItem setting, bool isEnable
}

packageSource.ProtocolVersion = ReadProtocolVersion(setting);
packageSource.AllowInsecureConnections = ReadAllowInsecureConnections(setting);

return packageSource;
}
Expand All @@ -213,6 +214,16 @@ private static int ReadProtocolVersion(SourceItem setting)
return PackageSource.DefaultProtocolVersion;
}

private static bool ReadAllowInsecureConnections(SourceItem setting)
{
if (bool.TryParse(setting.AllowInsecureConnections, out var allowInsecureConnections))
{
return allowInsecureConnections;
}

return PackageSource.DefaultAllowInsecureConnections;
}

private static int AddOrUpdateIndexedSource(
Dictionary<string, IndexedPackageSource> packageSourceLookup,
int packageIndex,
Expand Down
2 changes: 1 addition & 1 deletion src/NuGet.Core/NuGet.Configuration/PublicAPI.Shipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ NuGet.Configuration.SettingsUtility
NuGet.Configuration.SourceItem
~NuGet.Configuration.SourceItem.ProtocolVersion.get -> string
~NuGet.Configuration.SourceItem.ProtocolVersion.set -> void
~NuGet.Configuration.SourceItem.SourceItem(string key, string value, string protocolVersion = "") -> void
aortiz-msft marked this conversation as resolved.
Show resolved Hide resolved
~NuGet.Configuration.SourceItem.SourceItem(string key, string value, string protocolVersion) -> void
NuGet.Configuration.StoreClientCertItem
NuGet.Configuration.StoreClientCertItem.FindType.get -> System.Security.Cryptography.X509Certificates.X509FindType
~NuGet.Configuration.StoreClientCertItem.FindValue.get -> string
Expand Down
7 changes: 7 additions & 0 deletions src/NuGet.Core/NuGet.Configuration/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
#nullable enable
NuGet.Configuration.PackageSource.AllowInsecureConnections.get -> bool
NuGet.Configuration.PackageSource.AllowInsecureConnections.set -> void
NuGet.Configuration.PackageSourceMappingProvider.ShouldSkipSave.get -> bool
NuGet.Configuration.PackageSourceMapping.SearchForPattern(string! packageId) -> string?
~NuGet.Configuration.PackageSourceMappingProvider.PackageSourceMappingProvider(NuGet.Configuration.ISettings settings, bool shouldSkipSave) -> void
~NuGet.Configuration.SourceItem.AllowInsecureConnections.get -> string
~NuGet.Configuration.SourceItem.AllowInsecureConnections.set -> void
~NuGet.Configuration.SourceItem.SourceItem(string key, string value) -> void
~NuGet.Configuration.SourceItem.SourceItem(string key, string value, string protocolVersion, string allowInsecureConnections) -> void
~static readonly NuGet.Configuration.ConfigurationConstants.AllowInsecureConnections -> string
32 changes: 30 additions & 2 deletions src/NuGet.Core/NuGet.Configuration/Settings/Items/SourceItem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,41 @@ public string ProtocolVersion
set => AddOrUpdateAttribute(ConfigurationConstants.ProtocolVersionAttribute, value);
}

public SourceItem(string key, string value, string protocolVersion = "")
public string AllowInsecureConnections
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should check what happens when the nuget.config already has a value for allowInsecureConnections, and then you edit sources in VS's options page. I think VS will strip out the allowInsecureConnections value, so I think you need to add this property into PackageSourceContextInfo, its MsgPack formatter, and possibly NuGetSourcesService.GetPackageSourcesToUpdate, as I did in this PR: #5334

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! That's very helpful! Fixed and added tests. Manually tested in VS, the non-default value of allowInsecureConnections will not be stripped out when updating in VS options page. The default value(false) and invalid value(e.g. "random", will be changed into default value) will be removed after updating, as an expected normalization work. (Just like the protocal Version "2" will be stripped out after updating).

{
get
{
if (Attributes.TryGetValue(ConfigurationConstants.AllowInsecureConnections, out var attribute))
{
return Settings.ApplyEnvironmentTransform(attribute);
}

return null;
}
set => AddOrUpdateAttribute(ConfigurationConstants.AllowInsecureConnections, value);
}

public SourceItem(string key, string value)
: this(key, value, protocolVersion: "", allowInsecureConnections: "")
{
}

public SourceItem(string key, string value, string protocolVersion)
: this(key, value, protocolVersion, allowInsecureConnections: "")
{
}

public SourceItem(string key, string value, string protocolVersion, string allowInsecureConnections)
: base(key, value)
{
if (!string.IsNullOrEmpty(protocolVersion))
{
ProtocolVersion = protocolVersion;
}
if (!string.IsNullOrEmpty(allowInsecureConnections))
{
AllowInsecureConnections = allowInsecureConnections;
}
}

internal SourceItem(XElement element, SettingsFile origin)
Expand All @@ -37,7 +65,7 @@ internal SourceItem(XElement element, SettingsFile origin)

public override SettingBase Clone()
{
var newSetting = new SourceItem(Key, Value, ProtocolVersion);
var newSetting = new SourceItem(Key, Value, ProtocolVersion, AllowInsecureConnections);

if (Origin != null)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ public static class ConfigurationConstants

public static readonly string AllowUntrustedRoot = "allowUntrustedRoot";

public static readonly string AllowInsecureConnections = "allowInsecureConnections";

public static readonly string ApiKeys = "apikeys";

public static readonly string Author = "author";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,5 +130,42 @@ public async Task Save_SourceWithDifferentProtocolVersion_SavesNewValue()
savedSources!.Count.Should().Be(1);
savedSources[0].ProtocolVersion.Should().Be(3);
}

[Fact]
public async Task Save_SourceWithDifferentAllowInsecureConnections_SavesNewValue()
{
PackageSource packageSource = new(name: "Source-Name", source: "Source-Path")
{
ProtocolVersion = 3,
AllowInsecureConnections = false
};

Mock<IPackageSourceProvider> packageSourceProvider = new();
packageSourceProvider.Setup(psp => psp.LoadPackageSources())
.Returns(new[] { packageSource });

List<PackageSource>? savedSources = null;
packageSourceProvider.Setup(psp => psp.SavePackageSources(It.IsAny<IEnumerable<PackageSource>>()))
.Callback((IEnumerable<PackageSource> newSources) => { savedSources = newSources.ToList(); });

var target = new NuGetSourcesService(options: default,
Mock.Of<IServiceBroker>(),
new AuthorizationServiceClient(Mock.Of<IAuthorizationService>()),
packageSourceProvider.Object);

List<PackageSourceContextInfo> updatedSources = new(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
List<PackageSourceContextInfo> updatedSources = new(1)
List<PackageSourceContextInfo> updatedSources = new(capacity: 1)

{
new PackageSourceContextInfo(packageSource.Source, packageSource.Name, packageSource.IsEnabled, protocolVersion: 3, allowInsecureConnections: true)
};

// Act
await target.SavePackageSourceContextInfosAsync(updatedSources, CancellationToken.None);

// Assert
savedSources.Should().NotBeNull();
savedSources!.Count.Should().Be(1);
savedSources[0].ProtocolVersion.Should().Be(3);
savedSources[0].AllowInsecureConnections.Should().Be(true);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@ public void SerializeThenDeserialize_WithValidArguments_RoundTrips(PackageSource
PackageSourceContextInfo? actualResult = SerializeThenDeserialize(PackageSourceContextInfoFormatter.Instance, expectedResult);

Assert.NotNull(actualResult);
Assert.Equal(expectedResult, actualResult);
Assert.Equal(expectedResult.GetHashCode(), actualResult.GetHashCode());
}

public static TheoryData TestData => new TheoryData<PackageSourceContextInfo>
{
{ new PackageSourceContextInfo("source", "name", isEnabled: true, protocolVersion: 3, allowInsecureConnections: true) },
{ new PackageSourceContextInfo("source", "name", isEnabled: true, protocolVersion: 3) },
{ new PackageSourceContextInfo("source", "name", isEnabled: true) },
{ new PackageSourceContextInfo("source", "name") },
Expand Down
Loading