Skip to content

Commit aab1847

Browse files
committed
feat+fix: address issue #8 (diagnostic + rate-limit + env-file)
Addresses all three asks from GitHub issue #8 plus a follow-up retry on the documented sandbox rate-limit surface. == Ask 1: log raw response body on meta-failure throws == CERTInextClient.cs throws "CERTInext ... failed: <ErrorMessage>. See gateway logs for details." on every non-success meta block, but most sites had no Logger.LogXxx call before the throw — the "see gateway logs" instruction was misleading because no detail was ever logged. Added a shared LogApiFailure(operationContext, resp, errorCode, errorMsg) helper that emits a structured LogWarning capturing HTTP status, the CERTInext error code + message, and a 4 KB-capped raw response body. Wired into every meta-failure throw site: - ValidateCredentials, PlaceOrderAsync, SubmitCsrAsync (HTTP fail), TrackOrderAsync, DownloadCertificateAsync, RevokeCertificateAsync, VerifyDcvAsync, GetDcvAsync. The exception text is unchanged (kept sanitized for the gateway UI per the prior C2 audit finding), but operators now actually have the raw body in gateway logs to diagnose. Closes the audit's deferred C2 item. == Ask 2: Troubleshooting section == New docsource/overview.md + mirrored "Troubleshooting" section in README.md covering: - "Inactive Account User." as a sandbox rate-limit surface - Enrollment returning EXTERNALVALIDATION (deferred to next sync) - EMS-956 from GetDcv (deferred to next sync) - Issue #7 type-load failure (closed in v1.0) == Ask 3: LoadEnvFile quote stripping == IntegrationTestFixture.LoadEnvFile passed quoted env values through with the quote characters included, so: CERTINEXT_REQUESTOR_NAME="Keyfactor Plugin Test" was sent to CERTInext as the 23-char literal `"Keyfactor Plugin Test"`. The integration tests have been sending the literal quotes for the entire project history. Extracted the value-parsing into IntegrationTestFixture.ParseEnvValue (internal static), added matching-pair double/single quote stripping per the reporter's snippet, and pinned the contract with 13 unit tests in IntegrationTestFixtureTests covering quoted, unquoted, whitespace, mismatched-quote, empty, and embedded-quote cases. == Follow-up: exponential-backoff retry on rate-limit surface == PlaceOrderAsync now auto-retries when meta.ErrorMessage matches the documented "Inactive Account User." rate-limit surface (case-insensitive substring match via IsRateLimitSurface). Up to 5 attempts with exponential backoff + ±25% jitter — nominal delays 1s / 2s / 4s / 8s / 16s, max total wait ~31s. Each retry refreshes the meta block so txn IDs stay unique. After all attempts exhaust, the original exception is propagated unchanged so a genuinely-inactive account surfaces identically to today. ComputeRateLimitBackoffSeconds + IsRateLimitSurface are exposed internal so unit tests can pin both the predicate (15 inline-data cases) and the backoff schedule (50 samples per attempt window). == Verification == - Release build: 0 warnings, 0 errors. - Unit tests: 171/171 pass (156 prior + 15 new in RateLimitRetryTests + IntegrationTestFixtureTests). - Live ping/sync against the sandbox still authenticates. - Issue #7's CERTInextCAPluginPublicSurfaceTests still passes — the refactor of PlaceOrderAsync didn't expose any v3.3-only types.
1 parent 054b2c3 commit aab1847

6 files changed

Lines changed: 441 additions & 33 deletions

File tree

CERTInext.IntegrationTests/IntegrationTestFixture.cs

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ private static Dictionary<string, string> LoadEnvFile(string path)
159159
continue;
160160

161161
string key = line.Substring(0, idx).Trim();
162-
string val = line.Substring(idx + 1).Trim();
162+
string val = ParseEnvValue(line.Substring(idx + 1));
163163
result[key] = val;
164164
}
165165
}
@@ -176,6 +176,28 @@ private static Dictionary<string, string> LoadEnvFile(string path)
176176
return result;
177177
}
178178

179+
/// <summary>
180+
/// Parses a raw value from a <c>KEY=VALUE</c> env-file line: trims surrounding
181+
/// whitespace, then strips a single pair of matching surrounding double or single
182+
/// quotes if present. Without quote stripping a line like
183+
/// <c>CERTINEXT_REQUESTOR_NAME="Keyfactor Plugin Test"</c> would parse as the 24-char
184+
/// literal <c>"Keyfactor Plugin Test"</c> (quotes included), diverging from any
185+
/// other shell-style env consumer reading the same file. See GitHub issue #8.
186+
/// Exposed <c>internal</c> for direct unit-testing.
187+
/// </summary>
188+
internal static string ParseEnvValue(string rawValue)
189+
{
190+
if (rawValue is null) return string.Empty;
191+
string val = rawValue.Trim();
192+
if (val.Length >= 2 &&
193+
((val[0] == '"' && val[val.Length - 1] == '"') ||
194+
(val[0] == '\'' && val[val.Length - 1] == '\'')))
195+
{
196+
val = val.Substring(1, val.Length - 2);
197+
}
198+
return val;
199+
}
200+
179201
private static string GetEnvValue(Dictionary<string, string> env, string key)
180202
{
181203
return env.TryGetValue(key, out string val) ? val : string.Empty;
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
// Copyright 2024 Keyfactor
2+
// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License.
3+
// You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
4+
// Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
5+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
6+
// and limitations under the License.
7+
8+
using FluentAssertions;
9+
using Xunit;
10+
11+
namespace Keyfactor.Extensions.CAPlugin.CERTInext.IntegrationTests
12+
{
13+
/// <summary>
14+
/// Pure unit tests (no live-API dependency) for the env-file parser used by
15+
/// <see cref="IntegrationTestFixture"/>. See GitHub issue #8 — without quote
16+
/// stripping, a shell-style quoted line was being parsed with the quote characters
17+
/// included in the value.
18+
/// </summary>
19+
public class IntegrationTestFixtureTests
20+
{
21+
[Theory]
22+
[InlineData("plain", "plain")]
23+
[InlineData(" plain ", "plain")]
24+
[InlineData("\"Keyfactor Plugin Test\"", "Keyfactor Plugin Test")]
25+
[InlineData(" \"Keyfactor Plugin Test\" ", "Keyfactor Plugin Test")]
26+
[InlineData("'single quoted'", "single quoted")]
27+
[InlineData("\"\"", "")] // empty quoted string
28+
[InlineData("''", "")] // empty single-quoted
29+
[InlineData("\"un-paired'", "\"un-paired'")] // mismatched quotes — leave alone
30+
[InlineData("\"", "\"")] // single naked quote, length<2 after trim — leave alone
31+
[InlineData("", "")]
32+
[InlineData(" ", "")]
33+
public void ParseEnvValue_HandlesQuotingAndWhitespace(string input, string expected)
34+
{
35+
IntegrationTestFixture.ParseEnvValue(input).Should().Be(expected);
36+
}
37+
38+
[Fact]
39+
public void ParseEnvValue_NullInput_ReturnsEmptyString()
40+
{
41+
IntegrationTestFixture.ParseEnvValue(null).Should().Be(string.Empty);
42+
}
43+
44+
[Fact]
45+
public void ParseEnvValue_DoesNotStripEmbeddedQuotes()
46+
{
47+
// Quotes in the middle of the value must NOT be stripped; only matching
48+
// outer wrappers count.
49+
IntegrationTestFixture.ParseEnvValue("foo\"bar\"baz")
50+
.Should().Be("foo\"bar\"baz");
51+
}
52+
}
53+
}
Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
// Copyright 2024 Keyfactor
2+
// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License.
3+
// You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
4+
// Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
5+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
6+
// and limitations under the License.
7+
8+
using FluentAssertions;
9+
using Keyfactor.Extensions.CAPlugin.CERTInext.Client;
10+
using Xunit;
11+
12+
namespace Keyfactor.Extensions.CAPlugin.CERTInext.Tests
13+
{
14+
/// <summary>
15+
/// Pure unit tests for the rate-limit-retry helpers in <see cref="CERTInextClient"/>.
16+
/// Behavioral / end-to-end coverage of the retry loop itself lives in the WireMock
17+
/// tests; here we pin the predicate and the backoff schedule.
18+
/// </summary>
19+
public class RateLimitRetryTests
20+
{
21+
[Theory]
22+
[InlineData("Inactive Account User.", true)] // exact form from sandbox
23+
[InlineData("inactive account user.", true)] // case-insensitive
24+
[InlineData("INACTIVE ACCOUNT USER", true)] // case + missing period
25+
[InlineData("Some preamble: Inactive Account User. Tail", true)] // embedded substring
26+
[InlineData("Active account user.", false)] // wrong polarity
27+
[InlineData("Account is inactive", false)] // similar phrase, wrong wording
28+
[InlineData("EMS-956 Invalid Request for this API.", false)] // unrelated error
29+
[InlineData("", false)]
30+
[InlineData(null, false)]
31+
public void IsRateLimitSurface_DetectsDocumentedPhraseOnly(string errorMessage, bool expected)
32+
{
33+
CERTInextClient.IsRateLimitSurface(errorMessage).Should().Be(expected);
34+
}
35+
36+
[Theory]
37+
[InlineData(1, 0.75, 1.25)] // base = 1s, jittered ±25% ⇒ [0.75, 1.25]
38+
[InlineData(2, 1.5, 2.5)] // 2s × jitter
39+
[InlineData(3, 3.0, 5.0)] // 4s × jitter
40+
[InlineData(4, 6.0, 10.0)] // 8s × jitter
41+
[InlineData(5, 12.0, 20.0)] // 16s × jitter
42+
public void ComputeRateLimitBackoffSeconds_ProducesExpectedRange(int attempt, double min, double max)
43+
{
44+
// Run several samples so jitter is exercised; every sample must fall inside
45+
// the documented exponential ± 25% jitter window.
46+
for (int i = 0; i < 50; i++)
47+
{
48+
double waitSeconds = CERTInextClient.ComputeRateLimitBackoffSeconds(attempt);
49+
waitSeconds.Should().BeInRange(min, max,
50+
$"attempt {attempt} sample {i} must fall inside the documented backoff window");
51+
}
52+
}
53+
54+
[Fact]
55+
public void ComputeRateLimitBackoffSeconds_ClampsAttemptsBelowOneToOne()
56+
{
57+
// Defensive: passing 0 or negative shouldn't produce zero / negative delay.
58+
CERTInextClient.ComputeRateLimitBackoffSeconds(0)
59+
.Should().BeInRange(0.75, 1.25);
60+
CERTInextClient.ComputeRateLimitBackoffSeconds(-3)
61+
.Should().BeInRange(0.75, 1.25);
62+
}
63+
}
64+
}

0 commit comments

Comments
 (0)