Improved docs. Added date filter, truncation settings. Fixed config l… #4
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces significant improvements to the Sectigo Metadata Sync tool, focusing on configuration management, OAuth authentication support, data truncation handling, and certificate date filtering. The changes also include extensive documentation updates and enhancements to API client reliability.
Key changes:
- Added OAuth authentication support for Keyfactor API alongside existing basic auth
- Introduced configurable data truncation to handle Keyfactor and Sectigo field length limits
- Added date-based filtering to sync only certificates imported after a specified date
Reviewed Changes
Copilot reviewed 25 out of 29 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| sectigo-metadata-sync/config/stock-fields.json | New stock configuration file with example field definitions |
| sectigo-metadata-sync/config/stock-config.json | Updated with new settings including truncation and date filter options |
| sectigo-metadata-sync/config/stock-config-oauth.json | New OAuth-based authentication configuration template |
| sectigo-metadata-sync/config/fields.json | Updated field definitions and corrected documentation comments |
| sectigo-metadata-sync/SectigoMetadataSync.csproj | Updated NuGet package versions and added new config files |
| sectigo-metadata-sync/Models/KeyfactorModels.cs | Renamed enum from MetadataDataType to KeyfactorMetadataDataType |
| sectigo-metadata-sync/Models/Internal.cs | Added Sectigo metadata field tracking properties |
| sectigo-metadata-sync/Models/Config.cs | Added OAuth configuration and date filtering support |
| sectigo-metadata-sync/MetadataSync.cs | Major refactoring with OAuth support, date filtering, and truncation handling |
| sectigo-metadata-sync/Logic/ValueCoercionSC.cs | New logic for coercing values when syncing to Sectigo |
| sectigo-metadata-sync/Logic/ValueCoercion.cs | New logic for coercing values when syncing to Keyfactor |
| sectigo-metadata-sync/Logic/ListFlushExtensions.cs | New extension for memory-efficient list processing |
| sectigo-metadata-sync/Logic/Helpers.cs | Enhanced with OAuth detection and improved property resolution |
| sectigo-metadata-sync/Logic/DateParser.cs | New utility for flexible date parsing |
| sectigo-metadata-sync/Client/SectigoClient.cs | Completely rewritten with retry logic and improved error handling |
| sectigo-metadata-sync/Client/OAuthTokenClient.cs | New OAuth token client implementation |
| sectigo-metadata-sync/Client/KeyfactorClient.cs | Enhanced with OAuth support and improved sync operations |
| docsource/content.md | Comprehensive documentation updates with new sections on OAuth and truncation |
| README.md | Mirror of documentation updates for main readme |
| CHANGELOG.md | New changelog documenting version 1.1.0 changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "keyfactorMetadataFieldName": "TestField", | ||
| "keyfactorDescription": "Just an empty test field.", | ||
| "keyfactorDataType": 1, | ||
| "keyfactorDataType": 7, |
There was a problem hiding this comment.
Field data type inconsistency: The test field 'TestField' with sectigoFieldName 'Special String' has keyfactorDataType changed from 1 (String) to 7 (Email), but this doesn't align with the field's apparent purpose as a generic test string field.
| "keyfactorDataType": 7, | |
| "keyfactorDataType": 1, |
| "keyfactorOptions": null, | ||
| "keyfactorDefaultValue": null, | ||
| "keyfactorDisplayOrder": 1, | ||
| "keyfactorDisplayOrder": 0, |
There was a problem hiding this comment.
All keyfactorDisplayOrder values are set to 0, which may cause unpredictable ordering in the Keyfactor UI. Consider using distinct sequential values (1, 2, 3, 4) to ensure consistent field ordering.
| "keyfactorOptions": null, | ||
| "keyfactorDefaultValue": null, | ||
| "keyfactorDisplayOrder": 2, | ||
| "keyfactorDisplayOrder": 0, |
There was a problem hiding this comment.
All keyfactorDisplayOrder values are set to 0, which may cause unpredictable ordering in the Keyfactor UI. Consider using distinct sequential values (1, 2, 3, 4) to ensure consistent field ordering.
| "keyfactorOptions": null, | ||
| "keyfactorDefaultValue": null, | ||
| "keyfactorDisplayOrder": 3, | ||
| "keyfactorDisplayOrder": 0, |
There was a problem hiding this comment.
All keyfactorDisplayOrder values are set to 0, which may cause unpredictable ordering in the Keyfactor UI. Consider using distinct sequential values (1, 2, 3, 4) to ensure consistent field ordering.
| "keyfactorOptions": null, | ||
| "keyfactorDefaultValue": null, | ||
| "keyfactorDisplayOrder": 4, | ||
| "keyfactorDisplayOrder": 0, |
There was a problem hiding this comment.
All keyfactorDisplayOrder values are set to 0, which may cause unpredictable ordering in the Keyfactor UI. Consider using distinct sequential values (1, 2, 3, 4) to ensure consistent field ordering.
| <None Update="config\fields - Copy %282%29.json"> | ||
| <CopyToOutputDirectory>Always</CopyToOutputDirectory> | ||
| </None> |
There was a problem hiding this comment.
This appears to be a duplicate/backup file that should not be included in the build output. Consider removing this entry or clarifying its purpose.
| <None Update="config\fields - Copy %282%29.json"> | |
| <CopyToOutputDirectory>Always</CopyToOutputDirectory> | |
| </None> |
| { | ||
| if (args.Length == 0) | ||
| throw new ArgumentException("No configuration mode provided. Please specify KFtoSC or SCtoKF."); | ||
| throw new ArgumentException("No sync mode provided. Please specify either KFtoSC or SCtoKF using a command line argument."); |
There was a problem hiding this comment.
Error message has inconsistent casing: 'KFtoSC or SCtoKF' should match the documentation which uses 'SCtoKF or KFtoSC' ordering.
| // Example when mapping a Sectigo custom field into a KF metadata field: | ||
| var raw = localCustomField?.Value; // string | ||
| using var doc = | ||
| JsonDocument.Parse($"\"{raw?.Replace("\\", "\\\\").Replace("\"", "\\\"")}\""); |
There was a problem hiding this comment.
Potential issue with null handling and escape sequence order. The Replace operations should be performed on a non-null string, and backslash escaping should occur before quote escaping to avoid double-escaping.
| JsonDocument.Parse($"\"{raw?.Replace("\\", "\\\\").Replace("\"", "\\\"")}\""); | |
| JsonDocument.Parse(JsonSerializer.Serialize(raw)); |
| catch (Exception ex) | ||
| { | ||
| Log.Error(ex, "JSON deserialization error for {Url}. Payload (truncated): {Snippet}", | ||
| relativeUrl, text!.Length > 512 ? text.Substring(0, 512) + "…" : text); |
There was a problem hiding this comment.
Magic number 512 for truncation length should be extracted to a named constant for consistency and maintainability.
| // Send the GET request | ||
| var response = _httpClient.GetAsync(endpoint).GetAwaiter().GetResult(); | ||
| response.EnsureSuccessStatusCode(); | ||
| if (!string.IsNullOrEmpty(addedSince)) query.Append($"&DateImported -ge \"{addedSince}\""); |
There was a problem hiding this comment.
The query parameter syntax appears incorrect. The DateImported filter should likely be part of the QueryString parameter, not a separate query parameter, based on typical Keyfactor API patterns.
enableTruncationsetting before using this version!Fixed
Changed
Added