fix(secrets): review round 1 — gate the clustered-central secrets warning on Replication:Enabled, name the real trigger in the shared-store connection-string message, pin Grpc-vs-Store:Provider precedence

This commit is contained in:
Joseph Doherty
2026-09-28 17:56:30 -04:00
parent 35235ecfed
commit a57c36f504
5 changed files with 88 additions and 14 deletions
@@ -230,9 +230,15 @@ public class SecretsReplicationWiringTests
var ex = Assert.Throws<InvalidOperationException>(
() => services.AddScadaBridgeSecrets(config, SecretsNodeRole.Central));
// Same helper, same message style as the Grpc-mode central check.
// Same helper, same message style as the Grpc-mode central check — but this path's
// message must name ITS OWN trigger (review round 1, finding #2), never claim
// Secrets:Replication:Mode=Grpc, which is not in effect here.
Assert.Contains(
SecretsRegistration.HubConnectionStringKey, ex.Message, StringComparison.Ordinal);
Assert.Contains(
$"{SecretsRegistration.StoreProviderKey}=SqlServer", ex.Message, StringComparison.Ordinal);
Assert.DoesNotContain(
$"{SecretsRegistration.ReplicationModeKey}=Grpc", ex.Message, StringComparison.Ordinal);
}
[Fact]
@@ -267,6 +273,32 @@ public class SecretsReplicationWiringTests
Assert.IsType<SqliteSecretStore>(provider.GetRequiredService<ISecretStore>());
}
/// <summary>
/// Precedence pin (review round 1, finding #3): with BOTH triggers configured, the
/// pre-existing <c>Secrets:Replication:Enabled=true</c> Grpc-hub topology wins, and the new
/// Store:Provider=SqlServer branch defers to it rather than skipping hub registration. The
/// Store:Provider branch's own guard (<c>!ReplicationEnabledKey</c>) is what makes this so —
/// this test locks the ordering in against a future refactor of
/// <see cref="SecretsRegistration.AddScadaBridgeSecrets"/> that reorders the two branches.
/// </summary>
[Fact]
public void Central_ReplicationEnabledGrpcMode_TakesPrecedenceOverStoreProvider()
{
IConfiguration config = BuildConfig(
[
.. GrpcCentralConfig(),
("Secrets:Store:Provider", "SqlServer"),
]);
using ServiceProvider provider = BuildProvider(config, SecretsNodeRole.Central);
// Same shared SQL-Server store either way, but the Grpc hub is ALSO wired here — the
// Store:Provider-only path never registers the interceptor or the follower sweep.
Assert.IsType<SqlServerSecretStore>(provider.GetRequiredService<ISecretStore>());
Assert.NotNull(provider.GetService<SecretsHubAuthInterceptor>());
Assert.True(SecretsRegistration.UsesGrpcHub(config));
}
// ---------------------------------------------------------------------------------------
// SQL-Server mode — regression pins. Behaviour must be byte-for-byte what it was before the
// mode key existed, both when the mode is left unset and when it is named explicitly.
@@ -771,6 +771,25 @@ public class StartupValidatorTests
Assert.Empty(warnings);
}
[Fact]
public void ClusteredCentral_ReplicationEnabled_NoSecretsStoreWarning()
{
// Review round 1, finding #1: Secrets:Replication:Enabled=true already puts this central
// node on a topology that shares state across the pair — the pre-existing gRPC hub
// (whose central-side store is the shared SQL-Server store, not per-node SQLite) or
// SQL-Server replication. No Secrets:Store:Provider is set at all here — the exact case
// that used to warn before this fix — and it must stay silent because the node is not
// actually on a per-node SQLite store in this configuration.
var values = ValidCentralConfig();
values["ScadaBridge:InboundApi:ApiKeyStore:Provider"] = "SqlServer";
values["Secrets:Replication:Enabled"] = "true";
var config = BuildConfig(values);
var warnings = StartupValidator.CollectWarnings(config);
Assert.Empty(warnings);
}
[Fact]
public void CollectWarnings_SilentForSingleSeedCentral()
{