-
Notifications
You must be signed in to change notification settings - Fork 14
SIG-5341 v4 prep before new major #161
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
Conversation
360edab to
73cf31e
Compare
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.
Pull request overview
This PR prepares the SignhostAPIClient for a v5 major release by adding new API features, improving error handling, and establishing integration tests. The changes include new verification types (Onfido, OIDC, eHerkenning, CSC), enhanced data models with additional properties, and a new ResponseBody property for exception handling.
Key changes:
- Added new verification types (OnfidoVerification, OidcVerification, EherkenningVerification, CscVerification) to support additional authentication methods
- Enhanced exception handling by adding ResponseBody property to SignhostRestApiClientException and refactoring error handling logic
- Created integration test infrastructure with user secrets configuration to test against live API
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| SignhostRestApiClientException.cs | Added ResponseBody property to expose API error details |
| SignhostException.cs | Added TODO comment for v5 removal |
| HttpResponseMessageErrorHandlingExtensions.cs | Refactored exception throwing to use switch-and-assign pattern; added ResponseBody population |
| BadAuthorizationException.cs | Added TODO comment for v5 usage |
| SurfnetVerification.cs | Added Uid and Attributes properties |
| Signer.cs | Added timestamp and URL properties for signer lifecycle tracking |
| Receiver.cs | Added Id and timestamp properties |
| PhoneNumberVerification.cs | Added SecureDownload property |
| OnfidoVerification.cs | New verification type for Onfido identity verification |
| OidcVerification.cs | New verification type for OpenID Connect |
| IVerification.cs | Added TODO comment for v5 interface split |
| Field.cs | Added TODO comments for type improvements in v5 |
| EherkenningVerification.cs | New verification type for eHerkenning authentication |
| DigidVerification.cs | Added SecureDownload property |
| CscVerification.cs | New verification type for Cloud Signature Consortium |
| ActivityType.cs | Added TODO comment for cleanup in v5 |
| Activity.cs | Added ActivityValue property with JSON mapping |
| ApiResponse.cs | Added ResponseBody to GoneException with async workaround |
| SignhostAPIClient.sln | Added integration tests project reference |
| TransactionTests.cs | New comprehensive integration test for transaction lifecycle |
| TestConfiguration.cs | Configuration loader for integration test credentials |
| SignhostAPIClient.IntegrationTests.csproj | Project file for integration tests |
| README.md | Documentation for integration test setup |
| publish_nuget_package.yml | Added comment about integration tests exclusion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| break; | ||
|
|
||
| case HttpStatusCode.PaymentRequired | ||
| when errorType == "https://api.signhost.com/problem/subscription/out-of-credits": |
Copilot
AI
Nov 27, 2025
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.
The hardcoded URL string should use the OutOfCreditsApiProblemType constant defined on line 15 instead of duplicating the value.
| when errorType == "https://api.signhost.com/problem/subscription/out-of-credits": | |
| when errorType == OutOfCreditsApiProblemType: |
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.
Fixed
src/SignhostAPIClient/Rest/ErrorHandling/HttpResponseMessageErrorHandlingExtensions.cs
Show resolved
Hide resolved
73cf31e to
6f004ef
Compare
| /// <summary> | ||
| /// Gets or sets the certificate issuer. | ||
| /// </summary> | ||
| public string Issuer { get; set; } |
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.
Where did you get this def, for api the Provider only exposed and not Issuer, Subject, Thumbprint, AdditionalUserData they come from the settings as far as I remember.
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.
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.
We do expose them already, these are all returned in the postback/GETs after successful CSC verification
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.
I assume that CscVerification class is only for creation of the transaction and not about postabcks or other gets, otherwise like OIDCVerification also would expose something else. Sync with Berend too on this, seems strange if this class is only for the TranactionCreation purpose by Client to also include other attributes of the response.
BerendJonkman
left a comment
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.
Approved tech & style - 6f004ef29150bfab2e7e96ea8393caf06ef9b51b
This pull request introduces a new integration test project for the Signhost API client, adds comprehensive integration tests that require live credentials, and extends support for new verification types in the data model. It also includes minor improvements and clarifications in the codebase and CI/CD configuration.
Integration Testing Infrastructure:
SignhostAPIClient.IntegrationTestswith configuration for .NET 8, user secrets, and necessary dependencies. This project is now included in the solution file and is excluded from packaging and CI/CD runs. [1] [2] [3] [4] [5]TransactionTests.cs) that exercises transaction creation, file upload, and transaction state verification, ensuring real-world API functionality.README.mdwith clear instructions for configuring and running integration tests safely with user secrets.TestConfigurationclass that loads credentials only from user secrets to prevent accidental credential leaks.Verification and Authentication Model Extensions:
CscVerification,EherkenningVerification,OidcVerification, andOnfidoVerification, expanding the range of supported identity and signature verification methods. [1] [2] [3] [4]PhoneNumberVerificationandDigidVerificationto include aSecureDownloadproperty. [1] [2]Data Model and Serialization Improvements:
Activityclass with a newActivityValueproperty and JSON serialization attributes. [1] [2]ActivityTypeandFieldclasses for future refactoring. [1] [2]Error Handling and Miscellaneous: