-
Notifications
You must be signed in to change notification settings - Fork 2.1k
C#: Re-factor DependabotProxy class to allow unit-testing. #22477
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
base: main
Are you sure you want to change the base?
Changes from all commits
1387ef5
52793b0
0e350d0
d870a57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,84 +18,54 @@ public class DependabotProxy : IDependabotProxy | |
| /// <param name="URL">The URL of the package registry.</param> | ||
| public record class RegistryConfig(string Type, string URL); | ||
|
|
||
| private readonly string host; | ||
| private readonly string port; | ||
|
|
||
| public string Address { get; } | ||
|
|
||
| public HashSet<string> RegistryURLs { get; } | ||
| public HashSet<string> RegistryURLs { get; } = []; | ||
|
|
||
| public string? CertificatePath { get; private set; } | ||
|
|
||
| public X509Certificate2? Certificate { get; private set; } | ||
|
|
||
| internal static IDependabotProxy? GetDependabotProxy( | ||
| ILogger logger, IDiagnosticsWriter diagnosticsWriter, TemporaryDirectory tempWorkingDirectory) | ||
| private DependabotProxy(IDependabotProxyConfiguration config, ILogger logger, TemporaryDirectory tempWorkingDirectory) | ||
| { | ||
| // Setting HTTP(S)_PROXY and SSL_CERT_FILE have no effect on Windows or macOS, | ||
| // but we would still end up using the Dependabot proxy to check for feed reachability. | ||
| // This would result in us discovering that the feeds are reachable, but `dotnet` would | ||
| // fail to connect to them. To prevent this from happening, we do not initialise an | ||
| // instance of `DependabotProxy` on those platforms. | ||
| if (SystemBuildActions.Instance.IsWindows() || SystemBuildActions.Instance.IsMacOs()) return null; | ||
|
|
||
| // Obtain and store the address of the Dependabot proxy, if available. | ||
| var host = Environment.GetEnvironmentVariable(EnvironmentVariableNames.ProxyHost); | ||
| var port = Environment.GetEnvironmentVariable(EnvironmentVariableNames.ProxyPort); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(host) || string.IsNullOrWhiteSpace(port)) | ||
| { | ||
| logger.LogInfo("No Dependabot proxy credentials are configured."); | ||
| return null; | ||
| } | ||
|
|
||
| var result = new DependabotProxy(host, port); | ||
| logger.LogInfo($"Dependabot proxy configured at {result.Address}"); | ||
|
|
||
| // Obtain and store the proxy's certificate, if available. | ||
| var cert = Environment.GetEnvironmentVariable(EnvironmentVariableNames.ProxyCertificate); | ||
| Address = $"http://{config.Host}:{config.Port}"; | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(cert)) | ||
| if (!string.IsNullOrWhiteSpace(config.Certificate)) | ||
| { | ||
| var certDirPath = new DirectoryInfo(Path.Join(tempWorkingDirectory.DirInfo.FullName, ".dependabot-proxy")); | ||
| Directory.CreateDirectory(certDirPath.FullName); | ||
|
|
||
| result.CertificatePath = Path.Join(certDirPath.FullName, "proxy.crt"); | ||
| var certFile = new FileInfo(result.CertificatePath); | ||
| CertificatePath = Path.Join(certDirPath.FullName, "proxy.crt"); | ||
| var certFile = new FileInfo(CertificatePath); | ||
|
|
||
| using var writer = certFile.CreateText(); | ||
| writer.Write(cert); | ||
| writer.Write(config.Certificate); | ||
| writer.Close(); | ||
|
|
||
| logger.LogInfo($"Stored Dependabot proxy certificate at {result.CertificatePath}"); | ||
| logger.LogInfo($"Stored Dependabot proxy certificate at {CertificatePath}"); | ||
|
|
||
| result.Certificate = X509Certificate2.CreateFromPem(cert); | ||
| Certificate = X509Certificate2.CreateFromPem(config.Certificate); | ||
| } | ||
|
|
||
| // Try to obtain the list of private registry URLs. | ||
| var registryURLs = Environment.GetEnvironmentVariable(EnvironmentVariableNames.ProxyURLs); | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(registryURLs)) | ||
| if (!string.IsNullOrWhiteSpace(config.RegistryURLs)) | ||
| { | ||
| try | ||
| { | ||
| // The value of the environment variable should be a JSON array of objects, such as: | ||
| // [ { "type": "nuget_feed", "url": "https://nuget.pkg.github.com/org/index.json" } ] | ||
| var array = JsonConvert.DeserializeObject<List<RegistryConfig>>(registryURLs); | ||
| var array = JsonConvert.DeserializeObject<List<RegistryConfig>>(config.RegistryURLs); | ||
| if (array is not null) | ||
| { | ||
| foreach (RegistryConfig config in array) | ||
| foreach (RegistryConfig registry in array) | ||
| { | ||
| // The array contains all configured private registries, not just ones for C#. | ||
| // We ignore the non-C# ones here. | ||
| if (!config.Type.Equals("nuget_feed")) | ||
| if (!registry.Type.Equals("nuget_feed")) | ||
| { | ||
| logger.LogDebug($"Ignoring registry at '{config.URL}' since it is not of type 'nuget_feed'."); | ||
| logger.LogDebug($"Ignoring registry at '{registry.URL}' since it is not of type 'nuget_feed'."); | ||
| continue; | ||
| } | ||
|
|
||
| logger.LogInfo($"Found private registry at '{config.URL}'"); | ||
| result.RegistryURLs.Add(config.URL); | ||
| logger.LogInfo($"Found private registry at '{registry.URL}'"); | ||
| RegistryURLs.Add(registry.URL); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -104,6 +74,34 @@ public record class RegistryConfig(string Type, string URL); | |
| logger.LogError($"Unable to parse '{EnvironmentVariableNames.ProxyURLs}': {ex.Message}"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| internal static IDependabotProxy? Make(ILogger logger, IDiagnosticsWriter diagnosticsWriter, TemporaryDirectory tempWorkingDirectory) | ||
| { | ||
| // Setting HTTP(S)_PROXY and SSL_CERT_FILE have no effect on Windows or macOS, | ||
| // but we would still end up using the Dependabot proxy to check for feed reachability. | ||
| // This would result in us discovering that the feeds are reachable, but `dotnet` would | ||
| // fail to connect to them. To prevent this from happening, we do not initialise an | ||
| // instance of `DependabotProxy` on those platforms. | ||
| if (SystemBuildActions.Instance.IsWindows() || SystemBuildActions.Instance.IsMacOs()) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return MakeAux(new DependabotProxyConfiguration(), logger, diagnosticsWriter, tempWorkingDirectory); | ||
| } | ||
|
|
||
| internal static IDependabotProxy? MakeAux( | ||
| IDependabotProxyConfiguration proxyConfig, ILogger logger, IDiagnosticsWriter diagnosticsWriter, TemporaryDirectory tempWorkingDirectory) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(proxyConfig.Host) || string.IsNullOrWhiteSpace(proxyConfig.Port)) | ||
| { | ||
| logger.LogInfo("No Dependabot proxy credentials are configured."); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: I probably chose this in the original implementation, but this could probably be changed to a |
||
| return null; | ||
| } | ||
|
|
||
| var result = new DependabotProxy(proxyConfig, logger, tempWorkingDirectory); | ||
| logger.LogInfo($"Dependabot proxy configured at {result.Address}"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: Again probably something I was responsible for in the original implementation, but here and elsewhere, it might make sense to change the wording of the log messages to not mention "Dependabot". While that's technically accurate, it is probably confusing for users to see in the log. E.g. "Authentication proxy" or "Registry proxy" might be better. |
||
|
|
||
| // Emit a diagnostic for the discovered private registries, so that it is easy | ||
| // for users to see that they were picked up. | ||
|
|
@@ -125,17 +123,9 @@ public record class RegistryConfig(string Type, string URL); | |
| return result; | ||
| } | ||
|
|
||
| private DependabotProxy(string host, string port) | ||
| { | ||
| this.host = host; | ||
| this.port = port; | ||
| this.Address = $"http://{this.host}:{this.port}"; | ||
| this.RegistryURLs = new HashSet<string>(); | ||
| } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| this.Certificate?.Dispose(); | ||
| Certificate?.Dispose(); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| using System; | ||
|
|
||
| namespace Semmle.Extraction.CSharp.DependencyFetching | ||
| { | ||
| public class DependabotProxyConfiguration : IDependabotProxyConfiguration | ||
| { | ||
| public string? Host { get; } = Environment.GetEnvironmentVariable(EnvironmentVariableNames.ProxyHost); | ||
|
|
||
| public string? Port { get; } = Environment.GetEnvironmentVariable(EnvironmentVariableNames.ProxyPort); | ||
|
|
||
| public string? Certificate { get; } = Environment.GetEnvironmentVariable(EnvironmentVariableNames.ProxyCertificate); | ||
|
|
||
| public string? RegistryURLs { get; } = Environment.GetEnvironmentVariable(EnvironmentVariableNames.ProxyURLs); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| using System; | ||
|
|
||
| namespace Semmle.Extraction.CSharp.DependencyFetching | ||
| { | ||
| public interface IDependabotProxyConfiguration | ||
| { | ||
| // The host of the Dependabot proxy, if available. | ||
| string? Host { get; } | ||
|
|
||
| // The port of the Dependabot proxy, if available. | ||
| string? Port { get; } | ||
|
|
||
| // The certificate of the Dependabot proxy, if available. | ||
| string? Certificate { get; } | ||
|
|
||
| // The list of package registries that are configured for the proxy, if any. | ||
| // The value of the environment variable should be a JSON array of objects, such as: | ||
| // [ { "type": "nuget_feed", "url": "https://nuget.pkg.github.com/org/index.json" } ] | ||
| string? RegistryURLs { get; } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,185 @@ | ||
| using Xunit; | ||
| using System; | ||
| using System.IO; | ||
| using Semmle.Extraction.CSharp.DependencyFetching; | ||
| using Semmle.Util; | ||
|
|
||
| namespace Semmle.Extraction.Tests | ||
| { | ||
| public class DependabotConfigurationStub : IDependabotProxyConfiguration | ||
| { | ||
| public string? Host { get; set; } | ||
| public string? Port { get; set; } | ||
| public string? Certificate { get; set; } | ||
| public string? RegistryURLs { get; set; } | ||
| } | ||
|
|
||
| public class DiagnosticsWriterStub : IDiagnosticsWriter | ||
| { | ||
| public void AddEntry(Semmle.Util.DiagnosticMessage entry) { } | ||
| public void Dispose() { } | ||
| } | ||
|
|
||
| public class DependabotProxyTests | ||
| { | ||
| private static TemporaryDirectory MakeTemporaryDirectory() | ||
| { | ||
| var tmp = Path.Join(Path.GetTempPath(), "DependabotProxyTests", Guid.NewGuid().ToString()); | ||
| return new TemporaryDirectory(tmp, "testing", new LoggerStub()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void TestDependabotProxyCreation1() | ||
| { | ||
| // Setup | ||
| var config = new DependabotConfigurationStub | ||
| { | ||
| Host = "my.private.server", | ||
| Port = "", | ||
| }; | ||
|
|
||
| // Execute | ||
| using var tempWorkingDirectory = MakeTemporaryDirectory(); | ||
| using var proxy = DependabotProxy.MakeAux(config, new LoggerStub(), new DiagnosticsWriterStub(), tempWorkingDirectory); | ||
|
|
||
| // Verify | ||
| Assert.Null(proxy); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void TestDependabotProxyCreation2() | ||
| { | ||
| // Setup | ||
| var config = new DependabotConfigurationStub | ||
| { | ||
| Port = "8080", | ||
| }; | ||
|
|
||
| // Execute | ||
| using var tempWorkingDirectory = MakeTemporaryDirectory(); | ||
| using var proxy = DependabotProxy.MakeAux(config, new LoggerStub(), new DiagnosticsWriterStub(), tempWorkingDirectory); | ||
|
|
||
| // Verify | ||
| Assert.Null(proxy); | ||
| } | ||
|
|
||
| private const string ExampleCertificate = """ | ||
| -----BEGIN CERTIFICATE----- | ||
| MIIFJTCCAw2gAwIBAgIUDImU6YnuAqJ1QuRp+OpJQPnPu6wwDQYJKoZIhvcNAQEL | ||
| BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MB4XDTI2MDkwMTEyMjUzMVoXDTI3MDkw | ||
| MTEyMjUzMVowFDESMBAGA1UEAwwJbG9jYWxob3N0MIICIjANBgkqhkiG9w0BAQEF | ||
| AAOCAg8AMIICCgKCAgEAnlp7yQ1VuocMwIZWlCle3bEM86+1ED6BFfPFpIrRhfUT | ||
| c+5IvPng8TIZPO4mROp5G9YDZfOtXW2bwktyZNUhsBcxqUT1lmXit21vc5W9Gxx5 | ||
| 4G8nyF4/FcjFkmxkZifxiUCdBceDcE7+kx2itq/a7gLPlyTzvz5etu1nHEC3Jg/y | ||
| TVhAwdwysgAo9WymFCczDa2ga6nOPBOaxwLnoPl9041KSu5oIo9QC0Im+US1R18Q | ||
| /mXa+wkmjf+bYAkE/pZie8z8Q7h9yppTngGzkoDEebFYyaMr8MXlFdWS8f/eMwSp | ||
| iMFSsmlCqgUbA672APxzOcuSMMYrblzGkvZp23qbNjwQuQKlgAYBTSGltLv4U8JF | ||
| ePNcgDCY6RG55rNvF1gk1L2h25jcw1LX6fSvQGCOkzNmP03AhqZBUigO1Zt0zLwi | ||
| K4m0bH7nPLJFEN6tI3tybyZeC2RVyiSHvOkgx35Qj8RQ3XMVkImJNBYOMc2MkmMZ | ||
| ux6XMiHqXCON4zaWuWSovciZeMAQAspCrzVDLH6p2DWEfw/zDfQNU3iLk21sZGei | ||
| 0GKzs8zrxUcqOU9V4Cnm+7JJ6eqS72f1+wX0ROb3djC6KgCE/NaHqo4apiI3K+CH | ||
| T0rVRsJIHyT39YO1c1I1vhAKRSH5kQVe3qRfIT/AuaDLQY6WqGPzrOkem78sjtsC | ||
| AwEAAaNvMG0wHQYDVR0OBBYEFK0DP5MD6mhEcdcm346uwoPL2NFGMB8GA1UdIwQY | ||
| MBaAFK0DP5MD6mhEcdcm346uwoPL2NFGMA8GA1UdEwEB/wQFMAMBAf8wGgYDVR0R | ||
| BBMwEYIJbG9jYWxob3N0hwR/AAABMA0GCSqGSIb3DQEBCwUAA4ICAQCc5u8qNHHG | ||
| kONjfvq7Denq6QaEt4dZZDDODAvgUzZnnBjEhgrp7zfxtbyU/I0+DWnKQMKA9wPM | ||
| ktiFGd0lldEqoT+E7b0kN124lBGqZ/uYkhsWZ0Nc5dD+UB9oJszwOc5KNuquOnr6 | ||
| SbsfXVm4yLvVLXl67c0jvqvRgGg9/6Q6eMzohW6abMdbYhS28/DsJhCea/dV3+L1 | ||
| oVJ3O/A8e86m174ZCGE8s9UtnVYylBkAryDqaaQLdOBQ2C7uxdRAUNHSIa2JlqUc | ||
| 5+cod8lFojKb74hbgj6wkXyajsFttqYMh7CeASsnjZXDQ4MC3DqqDVCZuNvJ85Rt | ||
| ya3Tljp4Ln2AAAoKC3REUeU8PQqpk1vVIj0FSr3RvBTvwzyNfWFVqyBiXTATuV9n | ||
| 6AemqqXo5MZrHHeRaSTF8A70Jxbt9yx75xQxp3O3tdEL1Mxbl9X7c/hizOfLbeHH | ||
| IkAgzALQgi87Zbf2tOhRwH5NrB4ijyUUfovRHUwzsZOoTNqlVeNzbDRVbegx9V99 | ||
| /3vwNZgpStGl/JYhN9qY5hJKnC64ltMvuNGpLeJCGyFkrtFS8gKkgR7VKrGo7h3+ | ||
| Zo8rz8TFjP7RmSgQbrmFuPqNOGXzPidu2sMMFacKV7Rn4bEtHzW3MDhqVD4w/pGD | ||
| L0xpnWjzLYltVjz8mo07yh+zQ10G71Cl1w== | ||
| -----END CERTIFICATE----- | ||
| """; | ||
|
|
||
| [Fact] | ||
| public void TestDependabotProxyCertificate() | ||
| { | ||
| // Setup | ||
| var config = new DependabotConfigurationStub | ||
| { | ||
| Port = "8080", | ||
| Host = "my.private.server", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: As things stand, |
||
| Certificate = ExampleCertificate | ||
| }; | ||
|
|
||
| // Execute | ||
| using var tempWorkingDirectory = MakeTemporaryDirectory(); | ||
| using var proxy = DependabotProxy.MakeAux(config, new LoggerStub(), new DiagnosticsWriterStub(), tempWorkingDirectory); | ||
|
|
||
| // Verify | ||
| Assert.NotNull(proxy); | ||
| Assert.Equal("http://my.private.server:8080", proxy.Address); | ||
| Assert.NotNull(proxy.Certificate); | ||
| Assert.NotNull(proxy.CertificatePath); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void TestDependabotRegistryUrls1() | ||
| { | ||
| // Setup | ||
| var config = new DependabotConfigurationStub | ||
| { | ||
| Port = "8080", | ||
| Host = "my.private.server", | ||
| RegistryURLs = "Doesn't parse as a JSON list" | ||
| }; | ||
|
|
||
| // Execute | ||
| using var tempWorkingDirectory = MakeTemporaryDirectory(); | ||
| using var proxy = DependabotProxy.MakeAux(config, new LoggerStub(), new DiagnosticsWriterStub(), tempWorkingDirectory); | ||
|
|
||
| // Verify | ||
| Assert.NotNull(proxy); | ||
| Assert.Equal([], proxy.RegistryURLs); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void TestDependabotRegistryUrls2() | ||
| { | ||
| // Setup | ||
| var config = new DependabotConfigurationStub | ||
| { | ||
| Port = "8080", | ||
| Host = "my.private.server", | ||
| RegistryURLs = "[ { \"type\": \"nuget_feed\", \"url\": \"https://nuget.pkg.github.com/org/index.json\" } ]" | ||
| }; | ||
|
|
||
| // Execute | ||
| using var tempWorkingDirectory = MakeTemporaryDirectory(); | ||
| using var proxy = DependabotProxy.MakeAux(config, new LoggerStub(), new DiagnosticsWriterStub(), tempWorkingDirectory); | ||
|
|
||
| // Verify | ||
| Assert.NotNull(proxy); | ||
| Assert.Equal([ | ||
| "https://nuget.pkg.github.com/org/index.json" | ||
| ], proxy.RegistryURLs); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void TestDependabotRegistryUrls3() | ||
| { | ||
| // Setup | ||
| var config = new DependabotConfigurationStub | ||
| { | ||
| Port = "8080", | ||
| Host = "my.private.server", | ||
| RegistryURLs = "[ { \"type\": \"nuget_feed\", \"url\": \"https://example.com/org/index.json\" }, { \"type\": \"wrong_type\", \"url\": \"https://nuget.pkg.github.com/org/index.json\" } ]" | ||
| }; | ||
|
|
||
| // Execute | ||
| using var tempWorkingDirectory = MakeTemporaryDirectory(); | ||
| using var proxy = DependabotProxy.MakeAux(config, new LoggerStub(), new DiagnosticsWriterStub(), tempWorkingDirectory); | ||
|
|
||
| // Verify | ||
| Assert.NotNull(proxy); | ||
| Assert.Equal([ | ||
| "https://example.com/org/index.json" | ||
| ], proxy.RegistryURLs); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: It might be good to give this a more descriptive name or a doc comment explaining why it is separated out of
Make.