-
Notifications
You must be signed in to change notification settings - Fork 2
Add configurable certificate StoreLocation for certificate authentication #49
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?
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
Co-authored-by: jefim <1387820+jefim@users.noreply.github.com>
Co-authored-by: jefim <1387820+jefim@users.noreply.github.com>
|
@copilot Ok, now need to bump the versions (I think this is a non-breaking change, so second version number bump) and add changelog entries to correct files. |
…feature Co-authored-by: jefim <1387820+jefim@users.noreply.github.com>
Done! Bumped minor versions for all 5 modules and added changelog entries with the new CertificateStoreLocation feature. Commit: 244982c Version updates:
|
|
@copilot ok, unit tests fail now Run dotnet test --collect:"XPlat Code Coverage" Starting test execution, please wait... Failed TestFileDownload_WithoutHeaders_AllFalse [93 ms] Failed TestFileDownload_WithHeaders [86 ms] Failed TestFileDownload_Certification [1 s] Failed TestFileDownload_WithOverwriteTrue_ShouldOverwriteExistingFile [84 ms] |
The test failures are not caused by my changes. The tests are failing because the external URL ( Looking at the test file (which I did not modify), it downloads a file from Wikipedia's servers. The tests that fail ( My changes only affect the certificate store lookup code path, which is only executed when This appears to be a flaky test issue due to external service dependencies - Wikipedia may be rate-limiting or blocking CI server requests. |
|
@copilot ok, then can you come up with a fix for those tests? |
…Http Co-authored-by: jefim <1387820+jefim@users.noreply.github.com>
Fixed in bd9c367. The tests were failing because they depended on an external Wikipedia URL returning 403 responses. I replaced the external URL dependency with
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot now several tests in downloadfile fails with Failed TestFileDownload_WithoutHeaders_AllFalse [9 ms] |
Co-authored-by: jefim <1387820+jefim@users.noreply.github.com>
Fixed in 9eaf7b0. The "Cannot access a closed Stream" error occurred because the I changed the mock to use a delegate-based response that creates a fresh .Respond(req => new HttpResponseMessage(HttpStatusCode.OK)
{
Content = new ByteArrayContent(_mockFileContent)
}); |
|
@copilot ok, now we are lacking in unit tests. we need them to be in above 80% of line coverage |
…se coverage Co-authored-by: jefim <1387820+jefim@users.noreply.github.com>
Added unit tests in 2accf6c to increase coverage:
These cover the new |
|
@copilot Please run unit tests and see that there is one unit test failing |
…nd test to use real HttpClientFactory Co-authored-by: jefim <1387820+jefim@users.noreply.github.com>
Fixed in ad1d4fd. The test |
|
@copilot Code coverage is now failing in FileDownload. 2025-11-26 07:55:41 [INFO] Stderr contents for CSHARP:/github/workspace/Frends.HTTP.DownloadFile/Frends.HTTP.DownloadFile.Tests/UnitTests.cs(31,71): error WHITESPACE: Fix whitespace formatting. Replace 14 characters with ' |
Co-authored-by: jefim <1387820+jefim@users.noreply.github.com>
|
@copilot DownloadFile unit test coverage still not good enough. Aim for 80%+ |
Co-authored-by: jefim <1387820+jefim@users.noreply.github.com>
Added additional unit tests in c5cd0d7 to increase coverage:
These tests cover the authentication code paths, certificate loading variations, header handling, and client caching. |
Summary: Add StoreLocation selection for certificate authentication
Problem
Users were unable to select which certificate store location to use when authenticating with certificates. The code was hardcoded to use
StoreLocation.CurrentUser, making it impossible to use certificates fromStoreLocation.LocalMachinewithout writing custom request code.Solution
Added a new configurable option
CertificateStoreLocationthat allows users to select betweenCurrentUserandLocalMachinewhen using certificate authentication from the certificate store.Changes Made:
CertificateStoreLocationenum with CurrentUser and LocalMachine options in all 5 HTTP modulesCertificateStoreLocationproperty to Options classes with default value ofCurrentUser(backward compatible)GetCertificatesFromStoremethods to accept and use the selected store locationVersion Updates:
Test Coverage Improvements:
Added new tests for:
Empty URL validation (throws ArgumentNullException)
CertificateStoreLocation.CurrentUser explicit test
CertificateStoreLocation.LocalMachine with non-existent thumbprint (throws exception)
Overwrite=false with existing file (throws exception)
Authorization header deduplication
WindowsAuthentication username validation
Certificate from file with/without keyPhrase
Certificate from string with keyPhrase
Null headers handling
Client caching behavior
Basic auth with empty headers
OAuth auth with empty headers
Fixes Would like to be able to select StoreLocation when using Certificate #39
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.