From 228ff8b428329035199b7dcb5d9187b579249f9c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 22 Jul 2026 17:56:51 -0400 Subject: [PATCH] =?UTF-8?q?fix(grpc):=20one=20public=20constructor=20on=20?= =?UTF-8?q?ControlPlaneAuthInterceptor=20=E2=80=94=20two=20made=20the=20ga?= =?UTF-8?q?te=20inert?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught by the Phase 0 live gate, not by the suite. Grpc.AspNetCore registers the interceptor BY TYPE, and InterceptorRegistration.GetFactory() throws "Multiple constructors accepting all given argument types have been found" when more than one public constructor is applicable. The interceptor had two: the DI one and a prefix-set overload added for later phases. The failure mode is nasty. The throw happens inside the interceptor pipeline on every call, so nothing fails at startup — the site node boots, joins, reports healthy. Every gated call then dies with Unknown / "Exception was thrown by handler", which reads as a handler bug rather than an auth bug. And it fails OPEN in the sense that matters least and closed in the sense that matters most: no call is ever authorized, but no call is ever correctly REFUSED either, so the rig showed identical errors for a correct key, a wrong key and no key at all. Live evidence, site-a: three PullAuditEvents calls, three identical InvalidOperationExceptions in the node log. Fix: the prefix-set constructor is internal (Host.Tests already has InternalsVisibleTo). Later phases extend DefaultGatedPrefixes rather than adding a second public registration shape. Why the tests missed it, and what changed: ControlPlaneAuthEndToEndTests registered the interceptor with AddSingleton alongside AddGrpc, so DI handed back the instance and Grpc.AspNetCore's activation path — the thing that throws — never ran. The harness now registers exactly as Program.cs does, by type and not in DI. Plus a direct reflection assertion that the type has exactly one public constructor, since that is the real invariant and it is cheap to pin. --- .../ControlPlaneAuthInterceptor.cs | 23 ++++++++++++++++--- .../ControlPlaneAuthEndToEndTests.cs | 8 ++++++- .../ControlPlaneAuthInterceptorTests.cs | 16 +++++++++++++ 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/ControlPlaneAuthInterceptor.cs b/src/ZB.MOM.WW.ScadaBridge.Host/ControlPlaneAuthInterceptor.cs index ad8c3487..19445447 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/ControlPlaneAuthInterceptor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/ControlPlaneAuthInterceptor.cs @@ -62,7 +62,19 @@ public sealed class ControlPlaneAuthInterceptor : Interceptor private readonly IOptions _options; private readonly ILogger _logger; - /// Creates the interceptor gating . + /// + /// Creates the interceptor gating . + /// + /// + /// This must remain the ONLY public constructor. AddGrpc registers the + /// interceptor by type, and Grpc.AspNetCore.Server.InterceptorRegistration.GetFactory() + /// throws "Multiple constructors accepting all given argument types have been found" + /// when a second one is applicable. That throw happens per call, inside the pipeline, and + /// surfaces to the caller as Unknown / "Exception was thrown by handler" — so the gate + /// silently stops authorizing anything while still failing every call. A second public + /// constructor added here in a later phase reintroduces exactly that. Pinned by + /// ControlPlaneAuthInterceptorTests.TheInterceptorHasExactlyOnePublicConstructor. + /// /// Communication options; GrpcPsk is the expected bearer token. /// Logger for denial diagnostics. public ControlPlaneAuthInterceptor( @@ -72,11 +84,16 @@ public sealed class ControlPlaneAuthInterceptor : Interceptor { } - /// Creates the interceptor gating an explicit set of service prefixes. + /// + /// Creates the interceptor gating an explicit set of service prefixes. Internal — + /// see the public constructor's remarks for why this cannot be public. Phases that add a + /// service to the gate should extend rather than reach + /// for a second registration shape. + /// /// Communication options; GrpcPsk is the expected bearer token. /// Logger for denial diagnostics. /// Method-path prefixes to gate, e.g. /sitestream.SiteStreamService/. - public ControlPlaneAuthInterceptor( + internal ControlPlaneAuthInterceptor( IOptions options, ILogger logger, IReadOnlyList gatedPrefixes) diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ControlPlaneAuthEndToEndTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ControlPlaneAuthEndToEndTests.cs index 577fd0a3..58583775 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ControlPlaneAuthEndToEndTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ControlPlaneAuthEndToEndTests.cs @@ -47,10 +47,16 @@ public class ControlPlaneAuthEndToEndTests : IAsyncLifetime .UseTestServer() .ConfigureServices(services => { + // Registered exactly as Program.cs does: by TYPE on AddGrpc, with the + // interceptor itself NOT in DI. That is load-bearing. An earlier version of + // this test added it as a singleton, which let DI hand back the instance and + // bypassed Grpc.AspNetCore's own activation — hiding a defect where the + // interceptor had two public constructors and + // InterceptorRegistration.GetFactory() threw on every single call. The rig + // caught it; this test did not. Do not pre-register it. services.AddGrpc(o => o.Interceptors.Add()); services.AddSingleton(Options.Create( new CommunicationOptions { GrpcPsk = SiteKey })); - services.AddSingleton(); services.AddSingleton(); }) .Configure(app => diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ControlPlaneAuthInterceptorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ControlPlaneAuthInterceptorTests.cs index ceee0e79..6845a32c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ControlPlaneAuthInterceptorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ControlPlaneAuthInterceptorTests.cs @@ -205,6 +205,22 @@ public class ControlPlaneAuthInterceptorTests Assert.Equal("ok", await Invoke(interceptor, notGated)); } + [Fact] + public void TheInterceptorHasExactlyOnePublicConstructor() + { + // Grpc.AspNetCore registers this interceptor BY TYPE, and + // InterceptorRegistration.GetFactory() throws "Multiple constructors accepting all given + // argument types have been found" the moment a second public constructor is applicable. + // The throw lands inside the pipeline on every call, so the symptom is not a startup + // failure but a gate that authorizes nothing and fails everything with + // Unknown / "Exception was thrown by handler" — a shape that looks like a handler bug, + // not an auth bug. This shipped once and was caught only on the docker rig; the + // prefix-set constructor is internal now to keep it from recurring. + var publicCtors = typeof(ControlPlaneAuthInterceptor).GetConstructors(); + + Assert.Single(publicCtors); + } + [Fact] public void DefaultGatedPrefixes_MatchTheRealSiteStreamServicePath() {