Add logging through to Azure services #808

clairernovotny · 2024-12-11T16:38:47Z

Refactored to use Azure services via DI as that also configures it to work with ILogger.

- Introduced a private field `servicesToAdd` in `ServiceProviderFactory` to store additional services. - Added `AddServices` method to `ServiceProviderFactory` to add services via an `Action<IServiceCollection>` parameter. - Updated `Create` method to include `servicesToAdd` if not null. - Added `ServiceProviderFactoryTests` to verify new functionality. - Tests include null check, service addition, and service provider creation. - Defined `ITestService`, `TestService`, `ITestService2`, and `TestService2` for testing purposes.

Introduce Microsoft.Extensions.Azure package and refactor TrustedSigningService to use dependency injection for CertificateProfileClient. Update TrustedSigningServiceProvider to rely on service provider for instances. Add localized string resources for "Certificate Details" in multiple languages. Update tests to reflect changes and remove obsolete tests. Log detailed certificate information in TrustedSigningService.

src/Sign.SignatureProviders.TrustedSigning/TrustedSigningService.cs

src/Sign.Cli/TrustedSigningCommand.cs

src/Sign.Core/IServiceProviderFactory.cs

test/Sign.Cli.Test/TestInfrastructure/TestServiceProviderFactory.cs

src/Sign.SignatureProviders.TrustedSigning/Resources.resx

src/Sign.Core/ServiceProviderFactory.cs

…existing ones to match.

…ly disposed of after use.

- Updated `AzureKeyVaultCommand.cs` to construct and validate URIs for certificates and keys using Azure SDK, and configured Azure clients and services with dependency injection so logging forwards through. - Added `InvalidKeyVaultUrl` localized string in `AzureKeyVaultResources.Designer.cs` and updated `AzureKeyVaultResources.resx`. - Refactored `KeyVaultService.cs` to use `CertificateClient` and `CryptographyClient` for simplified service operations. - Simplified `KeyVaultServiceProvider.cs` by using dependency injection for `KeyVaultService` retrieval. - Removed `PrivateAssets` attribute from `Azure.Security.KeyVault.Certificates` and `Azure.Security.KeyVault.Keys` in `Sign.SignatureProviders.KeyVault.csproj` for runtime availability.

Also avoids unused parameter error

Co-authored-by: Damon Tivel <[email protected]>

…e when signing fails

dtivel · 2025-01-06T16:32:07Z

src/Sign.Core/SigningException.cs

+
+namespace Sign.Core
+{
+    internal class SigningException : Exception


sealed

Also, add a blank line between members.

dtivel · 2025-01-06T16:37:45Z

src/Sign.SignatureProviders.KeyVault/Resources.resx

@@ -117,6 +117,9 @@
  <resheader name="writer">
    <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
  </resheader>
+  <data name="CertificateDetails" xml:space="preserve">
+    <value>Certificate Details:</value>


Suggested change

<value>Certificate Details:</value>

<value>Certificate details:</value>

This is to be consistent with https://github.com/dotnet/sign/pull/808/files#diff-04854b03e08087d8a503b4b9480e748e19826ee8603241c1cdb3eeb05eaaf80dR121.

Be sure to regenerate .xlf files.

dtivel · 2025-01-06T16:39:43Z

test/Sign.Core.Test/ServiceProviderFactoryTests.cs

+        }
+
+        [Fact]
+        public void AddServices_AddedServicesAreAdded()


Test name convention is <MemberName>_<Condition>_<Result>

Suggested change

public void AddServices_AddedServicesAreAdded()

public void AddServices_Always_AddedServicesAreAdded()

dtivel · 2025-01-06T16:42:35Z

test/Sign.Core.Test/ServiceProviderFactoryTests.cs

+        }
+
+        [Fact]
+        public void AddServices_AddedIfAddedServicesAreAlsoPresent()


Suggested change

public void AddServices_AddedIfAddedServicesAreAlsoPresent()

public void AddServices_WhenAddedServicesAreAlreadyPresent_ServicesAdded()

dtivel · 2025-01-06T16:43:32Z

test/Sign.Core.Test/ServiceProviderFactoryTests.cs

+        }
+
+        [Fact]
+        public void Create_NoAddedServices()


Suggested change

public void Create_NoAddedServices()

public void Create_WithNoAddedServices_ReturnsDefault()

dtivel · 2025-01-06T17:22:19Z

test/Sign.SignatureProviders.TrustedSigning.Test/TrustedSigningServiceProviderTests.cs

        [Fact]
        public void GetSignatureAlgorithmProvider_WhenServiceProviderIsNull_Throws()
        {
-            TrustedSigningServiceProvider provider = new(TokenCredential, EndpointUrl, AccountName, CertificateProfileName);
+            TrustedSigningServiceProvider provider = new();


Since all tests do this, you could promote this to a field and make the bodies of the tests more succinct.

private readonly TrustedSigningServiceProvider _provider = new();

dtivel · 2025-01-06T17:23:39Z

test/Sign.SignatureProviders.TrustedSigning.Test/TrustedSigningServiceTests.cs

        private const string AccountName = "a";
        private const string CertificateProfileName = "b";
-        private readonly static ILogger<TrustedSigningService> Logger = Mock.Of<ILogger<TrustedSigningService>>();
+        private static readonly ILogger<TrustedSigningService> Logger = Mock.Of<ILogger<TrustedSigningService>>();

        [Fact]
        public void Constructor_WhenTokenCredentialIsNull_Throws()


Suggested change

public void Constructor_WhenTokenCredentialIsNull_Throws()

public void Constructor_WhenCertificateProfileClientIsNull_Throws()

dtivel · 2025-01-06T17:25:16Z

test/Sign.SignatureProviders.KeyVault.Test/KeyVaultServiceProviderTests.cs

@@ -102,16 +65,9 @@ public void GetCertificateProvider_WhenServiceProviderIsNull_Throws()
        [Fact]
        public void GetCertificateProvider_ReturnsSameInstance()


Suggested change

public void GetCertificateProvider_ReturnsSameInstance()

public void GetCertificateProvider_Always_ReturnsSameInstance()

dtivel · 2025-01-06T17:25:27Z

test/Sign.SignatureProviders.KeyVault.Test/KeyVaultServiceProviderTests.cs

@@ -76,22 +46,15 @@ public void GetSignatureAlgorithmProvider_WhenServiceProviderIsNull_Throws()
        [Fact]
        public void GetSignatureAlgorithmProvider_ReturnsSameInstance()


Suggested change

public void GetSignatureAlgorithmProvider_ReturnsSameInstance()

public void GetSignatureAlgorithmProvider_Always_ReturnsSameInstance()

dtivel · 2025-01-06T17:25:57Z

test/Sign.SignatureProviders.KeyVault.Test/KeyVaultServiceProviderTests.cs

+                    "a", sp.GetRequiredService<ILogger<KeyVaultService>>()
+                    );


Suggested change

"a", sp.GetRequiredService<ILogger<KeyVaultService>>()

);

"a",

sp.GetRequiredService<ILogger<KeyVaultService>>());

dtivel · 2025-01-06T17:48:00Z

src/Sign.Cli/TrustedSigningCommand.cs

+                                serviceProvider.GetRequiredService<CertificateProfileClient>(),
+                                accountName,
+                                certificateProfileName,
+                                serviceProvider.GetRequiredService<ILogger<TrustedSigningService>>());


Suggested change

serviceProvider.GetRequiredService<CertificateProfileClient>(),

accountName,

certificateProfileName,

serviceProvider.GetRequiredService<ILogger<TrustedSigningService>>());

serviceProvider.GetRequiredService<CertificateProfileClient>(),

accountName,

certificateProfileName,

serviceProvider.GetRequiredService<ILogger<TrustedSigningService>>());

dtivel

Minor feedback. Looks good!

clairernovotny added 5 commits December 11, 2024 10:11

cleanup

863eb2b

Rename private field servicesToAdd to _servicesToAdd

22af646

Fix parameter name in test

0b9d733

dtivel reviewed Dec 11, 2024

View reviewed changes

Some formatting and renames based on feedback

2a7c63a

clairernovotny force-pushed the tss-fix branch from 8d010cc to 8463ad2 Compare December 11, 2024 22:11

Update editorconfig to match VS output, add additioal rules and sort …

67fb84e

…existing ones to match.

clairernovotny force-pushed the tss-fix branch from 8463ad2 to 67fb84e Compare December 11, 2024 22:33

clairernovotny added 7 commits December 12, 2024 13:01

Wrapped the Stream object in a using statement to ensure it is proper…

a59fe63

…ly disposed of after use.

Add additional logging

425cfa8

Also avoids unused parameter error

Refactor KeyVaultService and update tests

a348fe6

Update tests

ec80c58

Finish tests

727482e

Revert editorconfig update. separate pr for that

c3ce79a

clairernovotny marked this pull request as ready for review December 13, 2024 17:06

clairernovotny requested a review from a team as a code owner December 13, 2024 17:06

clairernovotny and others added 3 commits December 13, 2024 12:07

Update src/Sign.SignatureProviders.TrustedSigning/Resources.resx

27de4da

Co-authored-by: Damon Tivel <[email protected]>

Ensure the cert details are printed on a new line

841e02b

Handle the error with a better output message instead of a stack trac…

5b02eb9

…e when signing fails

dtivel reviewed Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logging through to Azure services #808

Add logging through to Azure services #808

clairernovotny commented Dec 11, 2024

dtivel Jan 6, 2025

dtivel Jan 6, 2025

dtivel Jan 6, 2025 •

edited

Loading

dtivel Jan 6, 2025

dtivel Jan 6, 2025

dtivel Jan 6, 2025

dtivel Jan 6, 2025

dtivel Jan 6, 2025

dtivel Jan 6, 2025

dtivel Jan 6, 2025

dtivel Jan 6, 2025

dtivel left a comment

	<value>Certificate Details:</value>
	<value>Certificate details:</value>

	public void AddServices_AddedServicesAreAdded()
	public void AddServices_Always_AddedServicesAreAdded()

	public void AddServices_AddedIfAddedServicesAreAlsoPresent()
	public void AddServices_WhenAddedServicesAreAlreadyPresent_ServicesAdded()

	public void Create_NoAddedServices()
	public void Create_WithNoAddedServices_ReturnsDefault()

	public void Constructor_WhenTokenCredentialIsNull_Throws()
	public void Constructor_WhenCertificateProfileClientIsNull_Throws()

		@@ -102,16 +65,9 @@ public void GetCertificateProvider_WhenServiceProviderIsNull_Throws()
		[Fact]
		public void GetCertificateProvider_ReturnsSameInstance()

	public void GetCertificateProvider_ReturnsSameInstance()
	public void GetCertificateProvider_Always_ReturnsSameInstance()

		@@ -76,22 +46,15 @@ public void GetSignatureAlgorithmProvider_WhenServiceProviderIsNull_Throws()
		[Fact]
		public void GetSignatureAlgorithmProvider_ReturnsSameInstance()

Add logging through to Azure services #808

Are you sure you want to change the base?

Add logging through to Azure services #808

Conversation

clairernovotny commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtivel Jan 6, 2025 • 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

dtivel left a comment

Choose a reason for hiding this comment

dtivel Jan 6, 2025 •

edited

Loading