diff --git a/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/CompositeAuditWriter.cs b/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/CompositeAuditWriter.cs index 6cb271d..923dade 100644 --- a/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/CompositeAuditWriter.cs +++ b/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/CompositeAuditWriter.cs @@ -1,8 +1,9 @@ namespace ZB.MOM.WW.Audit; /// Fans an event out to several writers. Best-effort: a failing writer does not stop the others. -/// A failing writer's exception is swallowed so the fan-out drains and the caller is never -/// aborted — but is re-thrown so cancellation is honored. +/// Every inner-writer failure is swallowed — including +/// — so the fan-out drains and the caller is never aborted, honoring the +/// "must not throw to the caller" contract even when a request-scoped cancellation token is passed. public sealed class CompositeAuditWriter : IAuditWriter { private readonly IReadOnlyList _inner; @@ -21,8 +22,7 @@ public sealed class CompositeAuditWriter : IAuditWriter foreach (var writer in _inner) { try { await writer.WriteAsync(evt, ct).ConfigureAwait(false); } - catch (OperationCanceledException) { throw; } // honor cancellation; do not swallow - catch { /* best-effort seam: a failing writer must not stop the others or the caller */ } + catch { /* best-effort seam: a failing writer (incl. cancellation) must not stop the others or the caller */ } } } } diff --git a/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/TruncatingAuditRedactor.cs b/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/TruncatingAuditRedactor.cs index 4ca6cbd..38d8678 100644 --- a/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/TruncatingAuditRedactor.cs +++ b/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/TruncatingAuditRedactor.cs @@ -2,8 +2,9 @@ namespace ZB.MOM.WW.Audit; /// /// Redactor that caps oversized and . -/// Never throws — over-redacts (drops DetailsJson) on internal failure. The secret-field policy -/// (which fields are sensitive) stays per-project; compose this with a project redactor as needed. +/// Never throws — over-redacts (drops both DetailsJson and Target) on internal failure. The +/// secret-field policy (which fields are sensitive) stays per-project; compose this with a project +/// redactor as needed. /// public sealed class TruncatingAuditRedactor : IAuditRedactor { @@ -26,13 +27,15 @@ public sealed class TruncatingAuditRedactor : IAuditRedactor } catch { - // Hard contract: never throw. Over-redact on internal failure. - return rawEvent with { DetailsJson = null }; + // Hard contract: never throw, and over-redact to a STRICTLY safer event on internal + // failure — scrub every field this redactor owns (both DetailsJson and Target). + return rawEvent with { DetailsJson = null, Target = null }; } } private string? Truncate(string? value, int max) { + if (max < 0) max = 0; // clamp nonsensical negative caps so a config bug fails safe, not throws if (value is null || value.Length <= max) return value; var marker = _options.TruncationMarker; if (marker.Length >= max) return marker[..max]; diff --git a/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/TruncatingAuditRedactorOptions.cs b/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/TruncatingAuditRedactorOptions.cs index 0c44aba..d00bc9b 100644 --- a/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/TruncatingAuditRedactorOptions.cs +++ b/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/TruncatingAuditRedactorOptions.cs @@ -1,12 +1,12 @@ namespace ZB.MOM.WW.Audit; -/// Caps for . -public sealed class TruncatingAuditRedactorOptions +/// Immutable caps for . +public sealed record TruncatingAuditRedactorOptions { /// Max length of before truncation. Default 4096. - public int MaxDetailsJsonLength { get; set; } = 4096; + public int MaxDetailsJsonLength { get; init; } = 4096; /// Max length of before truncation. Default 512. - public int MaxTargetLength { get; set; } = 512; + public int MaxTargetLength { get; init; } = 512; /// Marker appended to a truncated value. Default "…[truncated]". - public string TruncationMarker { get; set; } = "…[truncated]"; + public string TruncationMarker { get; init; } = "…[truncated]"; } diff --git a/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/ZB.MOM.WW.Audit.csproj b/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/ZB.MOM.WW.Audit.csproj index 7951fde..22a7283 100644 --- a/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/ZB.MOM.WW.Audit.csproj +++ b/ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/ZB.MOM.WW.Audit.csproj @@ -3,6 +3,8 @@ net10.0 enable enable + + true true diff --git a/ZB.MOM.WW.Audit/tests/ZB.MOM.WW.Audit.Tests/CompositeAuditWriterTests.cs b/ZB.MOM.WW.Audit/tests/ZB.MOM.WW.Audit.Tests/CompositeAuditWriterTests.cs index 1faef9c..4b82b0d 100644 --- a/ZB.MOM.WW.Audit/tests/ZB.MOM.WW.Audit.Tests/CompositeAuditWriterTests.cs +++ b/ZB.MOM.WW.Audit/tests/ZB.MOM.WW.Audit.Tests/CompositeAuditWriterTests.cs @@ -38,11 +38,32 @@ public class CompositeAuditWriterTests } [Fact] - public async Task Cancellation_is_propagated_not_swallowed() + public async Task Cancellation_does_not_surface_to_the_caller() { - // OperationCanceledException is re-thrown (unlike ordinary writer failures, which are swallowed). + // Per the IAuditWriter hard contract ("must not throw to the caller"), an + // OperationCanceledException from an inner writer is swallowed like any other failure — + // it must NOT abort the user-facing action that produced the event. var after = new RecordingWriter(); var sut = new CompositeAuditWriter(new IAuditWriter[] { new CancellingWriter(), after }); - await Assert.ThrowsAsync(() => sut.WriteAsync(Evt())); + await sut.WriteAsync(Evt()); // must not throw + Assert.Equal(1, after.Count); // drain continues past the cancelled writer + } + + [Fact] + public async Task Empty_writer_list_is_a_no_op() + { + var sut = new CompositeAuditWriter(Array.Empty()); + await sut.WriteAsync(Evt()); // must not throw + } + + [Fact] + public async Task Null_writer_entry_is_swallowed_and_does_not_stop_the_others() + { + // A null inner writer faults the await; the best-effort seam swallows it (like any + // other writer failure) and continues draining the remaining writers. + var after = new RecordingWriter(); + var sut = new CompositeAuditWriter(new IAuditWriter?[] { null, after }!); + await sut.WriteAsync(Evt()); // must not throw + Assert.Equal(1, after.Count); } } diff --git a/ZB.MOM.WW.Audit/tests/ZB.MOM.WW.Audit.Tests/TruncatingAuditRedactorTests.cs b/ZB.MOM.WW.Audit/tests/ZB.MOM.WW.Audit.Tests/TruncatingAuditRedactorTests.cs index 02ba387..15b6cb7 100644 --- a/ZB.MOM.WW.Audit/tests/ZB.MOM.WW.Audit.Tests/TruncatingAuditRedactorTests.cs +++ b/ZB.MOM.WW.Audit/tests/ZB.MOM.WW.Audit.Tests/TruncatingAuditRedactorTests.cs @@ -53,4 +53,30 @@ public class TruncatingAuditRedactorTests var result = r.Apply(Evt(new string('x', 20))); Assert.Equal(3, result.DetailsJson!.Length); } + + [Fact] + public void Negative_max_is_treated_as_zero_and_does_not_throw() + { + // A negative cap is nonsensical misconfiguration. Truncate must clamp to 0 rather than + // throw, capping the value to the empty string (plus marker handling). + var opts = new TruncatingAuditRedactorOptions { MaxDetailsJsonLength = -5, MaxTargetLength = -1, TruncationMarker = "" }; + var r = new TruncatingAuditRedactor(opts); + var result = r.Apply(Evt(new string('x', 20), target: new string('y', 20))); + Assert.Equal(string.Empty, result.DetailsJson); + Assert.Equal(string.Empty, result.Target); + } + + [Fact] + public void Over_redact_fallback_scrubs_both_details_and_target_without_throwing() + { + // Drive the REAL TruncatingAuditRedactor.Apply into its catch branch via a reachable + // misconfiguration (a null TruncationMarker faults inside Truncate). The over-redact + // fallback must be strictly safer: BOTH DetailsJson AND Target scrubbed to null, no throw. + var opts = new TruncatingAuditRedactorOptions { MaxDetailsJsonLength = 5, TruncationMarker = null! }; + var r = new TruncatingAuditRedactor(opts); + var raw = Evt(new string('x', 50), target: "sensitive target"); + var result = r.Apply(raw); + Assert.Null(result.DetailsJson); + Assert.Null(result.Target); + } } diff --git a/ZB.MOM.WW.Auth/README.md b/ZB.MOM.WW.Auth/README.md index a5c22d1..bab3786 100644 --- a/ZB.MOM.WW.Auth/README.md +++ b/ZB.MOM.WW.Auth/README.md @@ -10,8 +10,8 @@ Authentication and authorisation libraries for the **ZB.MOM.WW SCADA family** (O |---|---|---| | `ZB.MOM.WW.Auth.Abstractions` | Auth contracts, canonical role constants, and shared types (`LdapOptions`, `LdapAuthResult`, `ILdapAuthService`, `IApiKeyStore`). No runtime dependencies beyond the BCL. | — | | `ZB.MOM.WW.Auth.Ldap` | LDAP authentication service: bind-then-search-then-bind against GLAuth or Active Directory; RFC 4514-aware group extraction; fail-closed. | `Abstractions`, `Novell.Directory.Ldap.NETStandard` | -| `ZB.MOM.WW.Auth.ApiKeys` | SQLite-backed API-key store with pepper-based PBKDF2 hashing, rotation, and audit log. Includes a `MigrationHostedService` that runs schema migrations on startup. | `Abstractions`, `Microsoft.Data.Sqlite` | -| `ZB.MOM.WW.Auth.AspNetCore` | ASP.NET Core DI helpers (`AddZbAuth`), cookie defaults, claim-type constants, and `LdapOptionsValidator` registration. Wires together Ldap + ApiKeys + cookie middleware. | `Abstractions`, `Ldap`, `ApiKeys`, `Microsoft.AspNetCore.App` | +| `ZB.MOM.WW.Auth.ApiKeys` | SQLite-backed API-key store with **pepper-keyed HMAC-SHA256** secret hashing, rotation, and audit log. DI wiring is `AddZbApiKeyAuth`; an opt-in `MigrationHostedService` runs schema migrations on startup. | `Abstractions`, `Microsoft.Data.Sqlite` | +| `ZB.MOM.WW.Auth.AspNetCore` | ASP.NET Core wiring for the **LDAP** provider only: `AddZbLdapAuth` (binds + start-time-validates `LdapOptions`, registers `ILdapAuthService`), plus `ZbCookieDefaults.Apply` (hardened cookie helper the consumer calls itself) and `ZbClaimTypes` constants. It does **not** wire API keys or cookie middleware — API-key DI is `AddZbApiKeyAuth` in the `ApiKeys` package. | `Abstractions`, `Ldap`, `Microsoft.AspNetCore.App` | --- diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Abstractions/Ldap/LdapContracts.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Abstractions/Ldap/LdapContracts.cs index c442220..bc89047 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Abstractions/Ldap/LdapContracts.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Abstractions/Ldap/LdapContracts.cs @@ -1,3 +1,5 @@ +using System.Net.Security; + namespace ZB.MOM.WW.Auth.Abstractions.Ldap; public enum LdapTransport { Ldaps, StartTls, None } @@ -16,6 +18,16 @@ public sealed record LdapOptions public string DisplayNameAttribute { get; init; } = "cn"; public string GroupAttribute { get; init; } = "memberOf"; public int ConnectionTimeoutMs { get; init; } = 10_000; + + /// + /// Optional hook to harden (or, in dev, relax) TLS server-certificate validation for the + /// / transports. When + /// (the default) the LDAP client validates the server certificate against + /// the OS trust store — it does not blind-accept. Supply a callback to pin a CA, validate + /// the SAN against , or otherwise tighten validation. This is a code-only seam + /// (not bound from configuration) and takes precedence over . + /// + public RemoteCertificateValidationCallback? ServerCertificateValidationCallback { get; init; } } public enum LdapAuthFailure { BadCredentials, UserNotFound, AmbiguousUser, GroupLookupFailed, ServiceAccountBindFailed, Disabled } diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Admin/ApiKeyAdminCommands.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Admin/ApiKeyAdminCommands.cs index cb24bfc..5aeab04 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Admin/ApiKeyAdminCommands.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Admin/ApiKeyAdminCommands.cs @@ -101,7 +101,10 @@ public sealed class ApiKeyAdminCommands var record = new ApiKeyRecord( KeyId: keyId, - KeyPrefix: $"{_options.TokenPrefix}_{keyId}", + // KeyPrefix is the bare token prefix (e.g. "mxgw"), NOT prefix_keyId — the key id is + // already its own column. Embedding it here produced a self-referential value that + // confused admin tooling and disagreed with the read/test paths (see Auth-005). + KeyPrefix: _options.TokenPrefix, SecretHash: secretHash, DisplayName: displayName, Scopes: scopes, diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs index e0cb1d9..7ae5da6 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs @@ -62,8 +62,24 @@ public sealed class ApiKeyVerifier( return Fail(ApiKeyFailure.SecretMismatch); } - // 6. Record successful use, then return the identity (no secret/hash/pepper included). - await store.MarkUsedAsync(record.KeyId, _timeProvider.GetUtcNow(), ct).ConfigureAwait(false); + // 6. The authentication decision is already made (line 60). Recording last-used is + // best-effort bookkeeping: a transient storage hiccup (SQLITE_BUSY past the busy-timeout, + // disk full, DB locked by a migration) must NOT turn an otherwise-valid credential into a + // failed auth. Swallow any non-cancellation failure so the only exception path remains + // cancellation, as the class contract promises. Cancellation is honoured (re-thrown). + try + { + await store.MarkUsedAsync(record.KeyId, _timeProvider.GetUtcNow(), ct).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + throw; + } + catch + { + // Best-effort: the last-used write failed, but the credential is valid. Fail open on the + // bookkeeping (not the auth decision) rather than denying a legitimate caller. + } return new ApiKeyVerification( Succeeded: true, diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Sqlite/ScopeSerializer.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Sqlite/ScopeSerializer.cs index 801842d..8c1cbcf 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Sqlite/ScopeSerializer.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Sqlite/ScopeSerializer.cs @@ -20,7 +20,12 @@ public static class ScopeSerializer /// Deserializes scopes from a JSON array string. /// The JSON string to deserialize; may be null or empty. - /// An ordinal-compared set of scopes; empty when the input is null/blank. + /// + /// An ordinal-compared set of scopes; empty when the input is null/blank. A malformed or + /// non-array column (operator tampering, a partial write, a format change, or a buggy writer) + /// fails closed to an EMPTY set rather than throwing, so a single poisoned row degrades to a + /// zero-scope identity on the auth path instead of an unhandled . + /// public static IReadOnlySet Deserialize(string? value) { if (string.IsNullOrWhiteSpace(value)) @@ -28,7 +33,18 @@ public static class ScopeSerializer return new HashSet(StringComparer.Ordinal); } - string[]? scopes = JsonSerializer.Deserialize(value); + string[]? scopes; + try + { + scopes = JsonSerializer.Deserialize(value); + } + catch (JsonException) + { + // Fail closed: a corrupt scopes column yields no scopes rather than an exception on the + // verification hot path. The verifier's "only exception path is cancellation" contract + // is preserved, and a key with an unreadable scope set is left with zero authority. + return new HashSet(StringComparer.Ordinal); + } return new HashSet(scopes ?? [], StringComparer.Ordinal); } diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.AspNetCore/ServiceCollectionExtensions.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.AspNetCore/ServiceCollectionExtensions.cs index 3cabb80..e899e47 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.AspNetCore/ServiceCollectionExtensions.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.AspNetCore/ServiceCollectionExtensions.cs @@ -37,7 +37,14 @@ public static class ServiceCollectionExtensions ArgumentNullException.ThrowIfNull(config); ArgumentException.ThrowIfNullOrWhiteSpace(sectionPath); - services.Configure(config.GetSection(sectionPath)); + // Bind via the options builder and opt into start-time validation. An IValidateOptions + // otherwise only runs when the options are first materialized (IOptions.Value) — which + // here is the first login (ILdapAuthService factory below), not boot. ValidateOnStart hooks + // the host's start-time options validation so a misconfigured directory (e.g. insecure + // transport without AllowInsecure) fails fast at startup rather than on first login. + services.AddOptions() + .Bind(config.GetSection(sectionPath)) + .ValidateOnStart(); // Fail fast at startup on a misconfigured directory rather than on first login. services.AddSingleton, LdapOptionsValidator>(); diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/ILdapConnection.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/ILdapConnection.cs index 79a8e88..67f3855 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/ILdapConnection.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/ILdapConnection.cs @@ -1,5 +1,6 @@ namespace ZB.MOM.WW.Auth.Ldap.Internal; +using System.Net.Security; using ZB.MOM.WW.Auth.Abstractions.Ldap; /// @@ -15,8 +16,29 @@ internal sealed record LdapSearchEntry( /// internal interface ILdapConnection : IDisposable { - /// Opens (and optionally upgrades to TLS) a connection to the given host. - void Connect(string host, int port, LdapTransport transport, bool allowInsecure, int timeoutMs); + /// + /// Opens (and optionally upgrades to TLS) a connection to the given host. + /// + /// The LDAP server hostname or IP. + /// The LDAP server port. + /// The transport security mode. + /// + /// When AND no is + /// supplied, TLS server-certificate validation is bypassed (dev/test only). Ignored when a + /// validation callback is supplied (the callback wins) or for plaintext transport. + /// + /// The connection/operation timeout in milliseconds. + /// + /// Optional TLS server-certificate validation callback. When , the OS trust + /// store is used (the client does not blind-accept). + /// + void Connect( + string host, + int port, + LdapTransport transport, + bool allowInsecure, + int timeoutMs, + RemoteCertificateValidationCallback? serverCertificateValidationCallback); /// Binds with the supplied DN and password. Throws LdapException on bad credentials. void Bind(string dn, string password); diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs index c7bc1c6..3c684a7 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs @@ -2,19 +2,67 @@ namespace ZB.MOM.WW.Auth.Ldap.Internal; using Novell.Directory.Ldap; using ZB.MOM.WW.Auth.Abstractions.Ldap; +// Disambiguate: Novell also declares a RemoteCertificateValidationCallback delegate; the seam and +// LdapConnectionOptions.ConfigureRemoteCertificateValidationCallback both use the BCL one. +using RemoteCertificateValidationCallback = System.Net.Security.RemoteCertificateValidationCallback; /// /// Production backed by Novell.Directory.Ldap.LdapConnection. /// Mirrors the connection/search idioms from ZB.MOM.WW.ScadaBridge.Security.LdapAuthService. /// +/// +/// TLS server-certificate validation: by default the underlying +/// Novell.Directory.Ldap.NETStandard client validates the server certificate against the OS +/// trust store (it does NOT blind-accept). A caller-supplied +/// RemoteCertificateValidationCallback overrides that default (CA pinning / SAN checks); when +/// none is supplied and allowInsecure is set, validation is bypassed for dev/test only. +/// internal sealed class NovellLdapConnection : ILdapConnection { - private readonly LdapConnection _conn = new(); + private readonly LdapConnection _conn; private bool _disposed; - /// - public void Connect(string host, int port, LdapTransport transport, bool allowInsecure, int timeoutMs) + /// + /// Builds the connection, wiring a TLS server-certificate validation policy: a supplied + /// wins; otherwise + /// bypasses validation (dev/test only); otherwise the OS-trust-store default applies. + /// + public NovellLdapConnection( + bool allowInsecure = false, + RemoteCertificateValidationCallback? serverCertificateValidationCallback = null) { + if (serverCertificateValidationCallback is not null) + { + var options = new LdapConnectionOptions() + .ConfigureRemoteCertificateValidationCallback(serverCertificateValidationCallback); + _conn = new LdapConnection(options); + } + else if (allowInsecure) + { + // Dev/test only: accept any server certificate. Reachable solely when an operator has set + // AllowInsecure (rejected for plaintext-without-AllowInsecure by LdapOptionsValidator). + var options = new LdapConnectionOptions() + .ConfigureRemoteCertificateValidationCallback((_, _, _, _) => true); + _conn = new LdapConnection(options); + } + else + { + // Default: validate against the OS trust store (no blind-accept). + _conn = new LdapConnection(); + } + } + + /// + public void Connect( + string host, + int port, + LdapTransport transport, + bool allowInsecure, + int timeoutMs, + RemoteCertificateValidationCallback? serverCertificateValidationCallback) + { + // The TLS-validation policy (allowInsecure / callback) is wired at construction time on the + // LdapConnectionOptions; the per-call arguments here are accepted for seam symmetry. ApplyTimeout(timeoutMs); // LDAPS: TLS is negotiated at the TCP-connection level. @@ -98,8 +146,16 @@ internal sealed class NovellLdapConnection : ILdapConnection } } -/// Factory that produces fresh instances. -internal sealed class NovellLdapConnectionFactory : ILdapConnectionFactory +/// +/// Factory that produces fresh instances, carrying the TLS +/// server-certificate validation policy (a supplied callback, or an allowInsecure bypass) so +/// it is wired onto each connection at construction time. +/// +internal sealed class NovellLdapConnectionFactory( + bool allowInsecure = false, + RemoteCertificateValidationCallback? serverCertificateValidationCallback = null) + : ILdapConnectionFactory { - public ILdapConnection Create() => new NovellLdapConnection(); + public ILdapConnection Create() => + new NovellLdapConnection(allowInsecure, serverCertificateValidationCallback); } diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs index 53ca248..892c173 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs @@ -26,10 +26,14 @@ public sealed class LdapAuthService : ILdapAuthService /// /// Production constructor: binds against a live directory via the real - /// Novell-backed connection factory. + /// Novell-backed connection factory. The TLS server-certificate validation policy + /// ( or the + /// bypass) is carried into the factory so each + /// connection is built with it. /// public LdapAuthService(LdapOptions options) - : this(options, new NovellLdapConnectionFactory()) + : this(options, new NovellLdapConnectionFactory( + options.AllowInsecure, options.ServerCertificateValidationCallback)) { } @@ -92,7 +96,13 @@ public sealed class LdapAuthService : ILdapAuthService // Abstractions change could add DirectoryUnavailable to disambiguate. try { - conn.Connect(_options.Server, _options.Port, _options.Transport, _options.AllowInsecure, _options.ConnectionTimeoutMs); + conn.Connect( + _options.Server, + _options.Port, + _options.Transport, + _options.AllowInsecure, + _options.ConnectionTimeoutMs, + _options.ServerCertificateValidationCallback); } catch (LdapException) { diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/ApiKeyAdminCommandsTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/ApiKeyAdminCommandsTests.cs index f7aaf44..6e00ba4 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/ApiKeyAdminCommandsTests.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/ApiKeyAdminCommandsTests.cs @@ -87,6 +87,33 @@ public sealed class ApiKeyAdminCommandsTests : IAsyncLifetime Assert.Single(recent, e => e.EventType == "create-key"); } + [Fact] + public async Task CreateKey_PersistsBareTokenPrefix_NotPrefixUnderscoreKeyId() + { + // Auth-005: KeyPrefix is the bare token prefix ("mxgw"), NOT "mxgw_key-1". The key id is + // already its own column; embedding it produced a self-referential value that disagreed with + // the read/test paths and confused admin tooling. + ApiKeyAdminCommands commands = BuildCommands(); + await commands.InitDbAsync(null, CancellationToken.None); + + await commands.CreateKeyAsync( + "key-1", + "Service A", + new HashSet(["read"], StringComparer.Ordinal), + constraintsJson: null, + remoteAddress: null, + CancellationToken.None); + + ApiKeyRecord? found = await _read.FindByKeyIdAsync("key-1", CancellationToken.None); + Assert.NotNull(found); + Assert.Equal("mxgw", found!.KeyPrefix); + + // The same bare prefix is surfaced by the admin list projection. + IReadOnlyList listed = await commands.ListKeysAsync(CancellationToken.None); + ApiKeyListItem item = Assert.Single(listed, k => k.KeyId == "key-1"); + Assert.Equal("mxgw", item.KeyPrefix); + } + [Fact] public async Task CreateKey_PepperUnavailable_ReturnsNoTokenAndAppendsNoAudit() { diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/ApiKeyVerifierTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/ApiKeyVerifierTests.cs index 242d618..6195fa1 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/ApiKeyVerifierTests.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/ApiKeyVerifierTests.cs @@ -212,6 +212,51 @@ public class ApiKeyVerifierTests Assert.DoesNotContain(Convert.ToBase64String(hash), identityText, StringComparison.Ordinal); } + // --- Auth-002: a failed best-effort MarkUsedAsync must NOT fail a valid key --- + + [Fact] + public async Task VerifyAsync_ValidKey_MarkUsedThrows_StillSucceeds() + { + // MarkUsedAsync is best-effort "last used" bookkeeping. A transient storage failure + // (SQLITE_BUSY, disk full, locked DB) must not turn an otherwise-valid credential into a + // failed auth: the decision is already made before the usage write. The verifier's contract + // is "the only exception path is cancellation", so a non-cancellation MarkUsedAsync failure + // is swallowed and the result is still Succeeded == true. + byte[] hash = ApiKeySecretHasher.Hash(Secret, Pepper); + var store = new FakeApiKeyStore + { + Record = BuildRecord(hash), + MarkUsedException = new InvalidOperationException("SQLITE_BUSY"), + }; + var verifier = BuildVerifier(store, new FakePepperProvider(Pepper)); + + ApiKeyVerification result = + await verifier.VerifyAsync(Header(KeyId, Secret), CancellationToken.None); + + Assert.True(result.Succeeded); + Assert.Null(result.Failure); + Assert.NotNull(result.Identity); + Assert.Equal(KeyId, result.Identity!.KeyId); + Assert.True(store.MarkUsedCalled); + } + + [Fact] + public async Task VerifyAsync_MarkUsedThrowsOperationCanceled_Propagates() + { + // The ONLY exception path is cancellation: an OperationCanceledException from the usage + // write (e.g. the request was cancelled mid-write) is honoured and re-thrown, not swallowed. + byte[] hash = ApiKeySecretHasher.Hash(Secret, Pepper); + var store = new FakeApiKeyStore + { + Record = BuildRecord(hash), + MarkUsedException = new OperationCanceledException(), + }; + var verifier = BuildVerifier(store, new FakePepperProvider(Pepper)); + + await Assert.ThrowsAnyAsync( + () => verifier.VerifyAsync(Header(KeyId, Secret), CancellationToken.None)); + } + // --- Cancellation --- [Fact] @@ -253,6 +298,9 @@ public class ApiKeyVerifierTests public string? MarkUsedKeyId { get; private set; } public DateTimeOffset? MarkUsedWhenUtc { get; private set; } + /// When set, throws this exception (after recording the call). + public Exception? MarkUsedException { get; set; } + public Task FindByKeyIdAsync(string keyId, CancellationToken ct) { FindByKeyIdCalled = true; @@ -267,6 +315,11 @@ public class ApiKeyVerifierTests MarkUsedCalled = true; MarkUsedKeyId = keyId; MarkUsedWhenUtc = whenUtc; + if (MarkUsedException is not null) + { + return Task.FromException(MarkUsedException); + } + return Task.CompletedTask; } } diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/SqliteApiKeyStoreTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/SqliteApiKeyStoreTests.cs index bf98278..81335a6 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/SqliteApiKeyStoreTests.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/SqliteApiKeyStoreTests.cs @@ -164,6 +164,36 @@ public sealed class SqliteApiKeyStoreTests : IAsyncLifetime Assert.Empty(ScopeSerializer.Deserialize("")); } + // --- Auth-003: corrupt scopes JSON must fail closed (empty set), never throw JsonException --- + + [Theory] + [InlineData("not json at all")] + [InlineData("{")] + [InlineData("{\"a\":1}")] // valid JSON, but an object, not a string[] + [InlineData("42")] // valid JSON, but a number + [InlineData("[\"read\",")] // truncated/partial write + public void ScopeSerializer_DeserializeMalformed_ReturnsEmptySet_DoesNotThrow(string value) + { + // A poisoned scopes column (tampering, partial write, format change, buggy writer) must + // degrade to a zero-scope set rather than throwing on the verification hot path. + IReadOnlySet scopes = ScopeSerializer.Deserialize(value); + Assert.Empty(scopes); + } + + [Fact] + public async Task FindByKeyId_CorruptScopesColumn_ReturnsRecordWithEmptyScopes_DoesNotThrow() + { + // Insert a row whose scopes column holds malformed (non-array) JSON, then read it through + // the store. The store must NOT propagate a JsonException out of FindByKeyIdAsync (which the + // verifier relies on for its "only exception path is cancellation" contract). + await InsertWithRawScopesAsync("key-corrupt", scopesJson: "{ this is not valid json"); + + ApiKeyRecord? found = await _store.FindByKeyIdAsync("key-corrupt", CancellationToken.None); + + Assert.NotNull(found); + Assert.Empty(found!.Scopes); + } + private static ApiKeyRecord SampleRecord(string keyId) => new( KeyId: keyId, KeyPrefix: "mxgw_ab12", @@ -213,6 +243,33 @@ public sealed class SqliteApiKeyStoreTests : IAsyncLifetime await command.ExecuteNonQueryAsync(CancellationToken.None); } + private async Task InsertWithRawScopesAsync(string keyId, string scopesJson) + { + // Writes the scopes column verbatim (NOT via ScopeSerializer.Serialize) so a malformed + // value can be persisted to simulate tampering / a partial or buggy write. + await using SqliteConnection connection = + await _factory.OpenConnectionAsync(CancellationToken.None); + await using SqliteCommand command = connection.CreateCommand(); + command.CommandText = """ + INSERT INTO api_keys ( + key_id, key_prefix, secret_hash, display_name, scopes, + constraints, created_utc, last_used_utc, revoked_utc) + VALUES ( + $key_id, $key_prefix, $secret_hash, $display_name, $scopes, + $constraints, $created_utc, $last_used_utc, $revoked_utc); + """; + command.Parameters.AddWithValue("$key_id", keyId); + command.Parameters.AddWithValue("$key_prefix", "mxgw"); + command.Parameters.Add("$secret_hash", SqliteType.Blob).Value = new byte[] { 1, 2, 3 }; + command.Parameters.AddWithValue("$display_name", "Corrupt Key"); + command.Parameters.AddWithValue("$scopes", scopesJson); + command.Parameters.AddWithValue("$constraints", DBNull.Value); + command.Parameters.AddWithValue("$created_utc", DateTimeOffset.UnixEpoch.ToString("O")); + command.Parameters.AddWithValue("$last_used_utc", DBNull.Value); + command.Parameters.AddWithValue("$revoked_utc", DBNull.Value); + await command.ExecuteNonQueryAsync(CancellationToken.None); + } + public Task DisposeAsync() { SqliteConnection.ClearAllPools(); diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.AspNetCore.Tests/ServiceCollectionExtensionsTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.AspNetCore.Tests/ServiceCollectionExtensionsTests.cs index ea32612..e59fd30 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.AspNetCore.Tests/ServiceCollectionExtensionsTests.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.AspNetCore.Tests/ServiceCollectionExtensionsTests.cs @@ -1,5 +1,6 @@ using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Options; using ZB.MOM.WW.Auth.Abstractions.Ldap; using ZB.MOM.WW.Auth.AspNetCore; @@ -85,4 +86,52 @@ public class ServiceCollectionExtensionsTests Assert.Contains(validators, v => v is LdapOptionsValidator); } + + // --- Auth-001: ValidateOnStart must run options validation at host startup, not first login --- + + private static IConfiguration BuildInsecureConfiguration() => + new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + [$"{LdapSection}:Server"] = LdapServer, + [$"{LdapSection}:SearchBase"] = "dc=example,dc=com", + [$"{LdapSection}:ServiceAccountDn"] = "cn=svc,dc=example,dc=com", + // Plaintext transport without AllowInsecure: the validator must reject this. + [$"{LdapSection}:Transport"] = nameof(LdapTransport.None), + [$"{LdapSection}:AllowInsecure"] = "false", + }) + .Build(); + + [Fact] + public async Task AddZbLdapAuth_StartingHost_FailsForInsecureConfig() + { + // The misconfiguration must surface at host start, not deferred until the first login + // (i.e. the first ILdapAuthService resolution). ValidateOnStart wires the host's + // start-time options validation, so StartAsync must throw OptionsValidationException. + IConfiguration config = BuildInsecureConfiguration(); + + using IHost host = new HostBuilder() + .ConfigureServices(services => services.AddZbLdapAuth(config, LdapSection)) + .Build(); + + OptionsValidationException ex = + await Assert.ThrowsAsync(() => host.StartAsync()); + + Assert.Contains(nameof(LdapOptions.Transport), string.Join(" ", ex.Failures)); + } + + [Fact] + public async Task AddZbLdapAuth_StartingHost_SucceedsForSecureConfig() + { + // A valid (secure) config must start cleanly — proving ValidateOnStart does not reject + // well-formed options. + IConfiguration config = BuildConfiguration(); + + using IHost host = new HostBuilder() + .ConfigureServices(services => services.AddZbLdapAuth(config, LdapSection)) + .Build(); + + await host.StartAsync(); + await host.StopAsync(); + } } diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/FakeLdapConnection.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/FakeLdapConnection.cs index ae9fd39..97ec79d 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/FakeLdapConnection.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/FakeLdapConnection.cs @@ -1,3 +1,4 @@ +using System.Net.Security; using ZB.MOM.WW.Auth.Abstractions.Ldap; using ZB.MOM.WW.Auth.Ldap.Internal; @@ -19,6 +20,10 @@ internal sealed class FakeLdapConnection : ILdapConnection // ---- observation ----- public (string Host, int Port, LdapTransport Transport, bool AllowInsecure, int TimeoutMs)? ConnectArgs { get; private set; } + + /// The server-certificate validation callback passed to the most recent call. + public RemoteCertificateValidationCallback? ConnectCertCallback { get; private set; } + public List BoundDns { get; } = new(); /// @@ -107,9 +112,16 @@ internal sealed class FakeLdapConnection : ILdapConnection // ---- ILdapConnection ----- - public void Connect(string host, int port, LdapTransport transport, bool allowInsecure, int timeoutMs) + public void Connect( + string host, + int port, + LdapTransport transport, + bool allowInsecure, + int timeoutMs, + RemoteCertificateValidationCallback? serverCertificateValidationCallback = null) { ConnectArgs = (host, port, transport, allowInsecure, timeoutMs); + ConnectCertCallback = serverCertificateValidationCallback; if (_throwOnConnect) throw new Novell.Directory.Ldap.LdapException( "Directory unreachable", Novell.Directory.Ldap.LdapException.ConnectError, host); diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceTests.cs index f9f66d3..44ff334 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceTests.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceTests.cs @@ -1,3 +1,4 @@ +using System.Net.Security; using ZB.MOM.WW.Auth.Abstractions.Ldap; using ZB.MOM.WW.Auth.Ldap; @@ -80,6 +81,56 @@ public class LdapAuthServiceTests Assert.Equal(LdapAuthFailure.Disabled, (await svc.AuthenticateAsync("a", "b", default)).Failure); } + // --- Auth-006: TLS validation seam — allowInsecure is honoured and a cert-validation + // callback is threaded into the connection rather than being silently ignored. --- + + [Fact] + public async Task Connect_ReceivesAllowInsecureFlag_FromOptions() + { + // The allowInsecure flag must reach the connection (it used to be an unused parameter). + var fake = new FakeLdapConnection().WithUserEntry( + "cn=alice,dc=x", memberOf: new[] { "cn=Engineers,ou=g,dc=x" }); + var svc = new LdapAuthService( + Opts() with { AllowInsecure = true }, new FakeLdapConnectionFactory(fake)); + + await svc.AuthenticateAsync("alice", "pw", default); + + Assert.NotNull(fake.ConnectArgs); + Assert.True(fake.ConnectArgs!.Value.AllowInsecure); + } + + [Fact] + public async Task Connect_ReceivesConfiguredCertValidationCallback() + { + // A consumer-supplied RemoteCertificateValidationCallback must be passed through to the + // connection so production callers can pin a CA / validate the SAN — the seam no longer + // discards it. + RemoteCertificateValidationCallback callback = (_, _, _, _) => true; + var fake = new FakeLdapConnection().WithUserEntry( + "cn=alice,dc=x", memberOf: new[] { "cn=Engineers,ou=g,dc=x" }); + var svc = new LdapAuthService( + Opts() with { ServerCertificateValidationCallback = callback }, + new FakeLdapConnectionFactory(fake)); + + await svc.AuthenticateAsync("alice", "pw", default); + + Assert.Same(callback, fake.ConnectCertCallback); + } + + [Fact] + public async Task Connect_NoCertCallbackConfigured_PassesNull() + { + // Default: no callback configured -> null reaches the connection, which means the + // production adapter falls back to OS-trust-store validation (documented behaviour). + var fake = new FakeLdapConnection().WithUserEntry( + "cn=alice,dc=x", memberOf: new[] { "cn=Engineers,ou=g,dc=x" }); + var svc = new LdapAuthService(Opts(), new FakeLdapConnectionFactory(fake)); + + await svc.AuthenticateAsync("alice", "pw", default); + + Assert.Null(fake.ConnectCertCallback); + } + [Fact] public async Task PreservesEscapedCommaInGroupName_OnRfc4514Dn() { diff --git a/ZB.MOM.WW.Configuration/Directory.Build.props b/ZB.MOM.WW.Configuration/Directory.Build.props index b234d68..520df21 100644 --- a/ZB.MOM.WW.Configuration/Directory.Build.props +++ b/ZB.MOM.WW.Configuration/Directory.Build.props @@ -6,5 +6,6 @@ latest 0.1.0 true + true diff --git a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/Checks.cs b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/Checks.cs index f9c1e6f..e81eb6b 100644 --- a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/Checks.cs +++ b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/Checks.cs @@ -1,3 +1,5 @@ +using System.Globalization; + namespace ZB.MOM.WW.Configuration; /// @@ -16,11 +18,14 @@ internal static class Checks /// /// Validates a raw string as a TCP port (parse + range), returning null when valid. - /// Centralizes the port wording for callers that hold the raw config value. + /// Centralizes the port wording for callers that hold the raw config value. Parsing is strict + /// and culture-invariant (): a leading sign or surrounding + /// whitespace is rejected. Both the parse-failure and range-failure messages quote the offending + /// raw value so they read consistently. /// internal static string? PortValue(string? raw, string field) => - int.TryParse(raw, out var port) - ? Port(port, field) + int.TryParse(raw, NumberStyles.None, CultureInfo.InvariantCulture, out var port) && port is >= 1 and <= 65535 + ? null : $"{field} must be between 1 and 65535 (was '{raw ?? "null"}')"; /// @@ -33,7 +38,7 @@ internal static class Checks var idx = value.LastIndexOf(':'); if (idx <= 0 || idx == value.Length - 1 || value.AsSpan(0, idx).Contains(':') - || !int.TryParse(value[(idx + 1)..], out var port) + || !int.TryParse(value[(idx + 1)..], NumberStyles.None, CultureInfo.InvariantCulture, out var port) || port is < 1 or > 65535) return $"{field} must be 'host:port' with port 1-65535 (was '{value}')"; return null; diff --git a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ServiceCollectionExtensions.cs b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ServiceCollectionExtensions.cs index 7456f72..25db784 100644 --- a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ServiceCollectionExtensions.cs +++ b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ServiceCollectionExtensions.cs @@ -1,5 +1,6 @@ using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Options; namespace ZB.MOM.WW.Configuration; @@ -33,7 +34,7 @@ public static class ServiceCollectionExtensions ArgumentNullException.ThrowIfNull(configuration); ArgumentException.ThrowIfNullOrWhiteSpace(sectionPath); - services.AddSingleton, TValidator>(); + services.TryAddEnumerable(ServiceDescriptor.Singleton, TValidator>()); return services.AddOptions() .Bind(configuration.GetSection(sectionPath)) .ValidateOnStart(); diff --git a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ZB.MOM.WW.Configuration.csproj b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ZB.MOM.WW.Configuration.csproj index feb7233..88654db 100644 --- a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ZB.MOM.WW.Configuration.csproj +++ b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ZB.MOM.WW.Configuration.csproj @@ -7,7 +7,11 @@ configuration;options;validation;ivalidateoptions;validateonstart;startup;scada;wonderware;zb-mom-ww https://gitea.dohertylan.com/dohertj2/zb-mom-ww-configuration https://gitea.dohertylan.com/dohertj2/zb-mom-ww-configuration + README.md + + + diff --git a/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/AddValidatedOptionsTests.cs b/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/AddValidatedOptionsTests.cs index 13d7d18..4329e7f 100644 --- a/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/AddValidatedOptionsTests.cs +++ b/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/AddValidatedOptionsTests.cs @@ -44,4 +44,24 @@ public sealed class AddValidatedOptionsTests Assert.Equal("central", opts.Name); await host.StopAsync(); } + + [Fact] + public void Calling_twice_registers_validator_once() + { + var config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { ["Node:Port"] = "0", ["Node:Name"] = "" }) + .Build(); + var services = new ServiceCollection(); + services.AddValidatedOptions(config, "Node"); + services.AddValidatedOptions(config, "Node"); + + using var provider = services.BuildServiceProvider(); + var validators = provider.GetServices>().ToArray(); + Assert.Single(validators); + + // Resolving the options surfaces each accumulated failure exactly once, not doubled. + var ex = Assert.Throws( + () => provider.GetRequiredService>().Value); + Assert.Equal(2, ex.Failures.Count()); + } } diff --git a/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ChecksWordingTests.cs b/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ChecksWordingTests.cs new file mode 100644 index 0000000..29a794e --- /dev/null +++ b/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ChecksWordingTests.cs @@ -0,0 +1,92 @@ +using Microsoft.Extensions.Configuration; +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.Configuration.Tests; + +/// +/// Pins the exact failure-message wording produced by the shared Checks seam through its +/// public front-ends ( for raw port values, +/// for host:port endpoints). Covers Configuration-002 (consistent quoting) and Configuration-003 +/// (strict, culture-invariant port parsing). +/// +public sealed class ChecksWordingTests +{ + private static IConfiguration Config(string key, string? value) => + new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { [key] = value }) + .Build(); + + private static string PortFailure(string? rawValue) + { + var pf = ConfigPreflight.For(Config("X:Port", rawValue)).RequirePort("X:Port"); + return Assert.Single(pf.Failures); + } + + // Configuration-002: range failure and parse failure must quote the offending value the same way. + + [Fact] + public void PortValue_range_failure_quotes_the_value() + { + Assert.Equal("X:Port must be between 1 and 65535 (was '0')", PortFailure("0")); + } + + [Fact] + public void PortValue_high_range_failure_quotes_the_value() + { + Assert.Equal("X:Port must be between 1 and 65535 (was '70000')", PortFailure("70000")); + } + + [Fact] + public void PortValue_parse_failure_quotes_the_value() + { + Assert.Equal("X:Port must be between 1 and 65535 (was 'notaport')", PortFailure("notaport")); + } + + [Fact] + public void PortValue_null_failure_renders_null() + { + Assert.Equal("X:Port must be between 1 and 65535 (was 'null')", PortFailure(null)); + } + + // Configuration-003: strict, culture-invariant parsing rejects sign and surrounding whitespace. + + [Theory] + [InlineData("+5000")] + [InlineData(" 5000")] + [InlineData("5000 ")] + [InlineData(" 5000 ")] + [InlineData("-1")] + public void PortValue_rejects_loose_inputs(string raw) + { + var pf = ConfigPreflight.For(Config("X:Port", raw)).RequirePort("X:Port"); + Assert.False(pf.IsValid); + Assert.Equal($"X:Port must be between 1 and 65535 (was '{raw}')", Assert.Single(pf.Failures)); + } + + [Fact] + public void PortValue_accepts_plain_in_range_port() + { + var pf = ConfigPreflight.For(Config("X:Port", "5000")).RequirePort("X:Port"); + Assert.True(pf.IsValid); + } + + [Theory] + [InlineData("host:+5000")] + [InlineData("host: 5000")] + [InlineData("host:5000 ")] + public void HostPort_rejects_loose_port_inputs(string value) + { + var b = new ValidationBuilder(); + b.HostPort(value, "X:Endpoint"); + Assert.False(b.IsValid); + Assert.Equal($"X:Endpoint must be 'host:port' with port 1-65535 (was '{value}')", Assert.Single(b.Failures)); + } + + [Fact] + public void HostPort_accepts_plain_endpoint() + { + var b = new ValidationBuilder(); + b.HostPort("host:5000", "X:Endpoint"); + Assert.True(b.IsValid); + } +} diff --git a/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ZB.MOM.WW.Configuration.Tests.csproj b/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ZB.MOM.WW.Configuration.Tests.csproj index a4e7f02..147ba5d 100644 --- a/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ZB.MOM.WW.Configuration.Tests.csproj +++ b/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ZB.MOM.WW.Configuration.Tests.csproj @@ -1,6 +1,8 @@ false + + false diff --git a/ZB.MOM.WW.Health/Directory.Build.props b/ZB.MOM.WW.Health/Directory.Build.props index c4755a6..4b5ab79 100644 --- a/ZB.MOM.WW.Health/Directory.Build.props +++ b/ZB.MOM.WW.Health/Directory.Build.props @@ -7,6 +7,11 @@ latest 0.1.0 true + + true + $(NoWarn);CS1591 diff --git a/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/ActiveNodeHealthCheck.cs b/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/ActiveNodeHealthCheck.cs index a9b54d3..df76da4 100644 --- a/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/ActiveNodeHealthCheck.cs +++ b/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/ActiveNodeHealthCheck.cs @@ -103,27 +103,41 @@ public sealed class ActiveNodeHealthCheck : IHealthCheck if (system is null) return Task.FromResult(HealthCheckResult.Degraded("ActorSystem not yet available.")); - var cluster = Cluster.Get(system); - var self = cluster.SelfMember; - var selfUp = self.Status == MemberStatus.Up; - + MemberStatus selfStatus; + bool selfUp; bool hasRole; bool isLeader; - if (_role is null) + try { - hasRole = false; - var leader = cluster.State.Leader; - isLeader = leader is not null && leader == self.Address; + // Reading cluster membership can throw while the ActorSystem exists but the cluster has + // not finished initialising (e.g. Akka.Cluster not yet configured → + // ConfigurationException). The spec's startup-safety rule maps this to Degraded rather + // than letting the exception escape (which the host would record as Unhealthy). + var cluster = Cluster.Get(system); + var self = cluster.SelfMember; + selfStatus = self.Status; + selfUp = selfStatus == MemberStatus.Up; + + if (_role is null) + { + hasRole = false; + var leader = cluster.State.Leader; + isLeader = leader is not null && leader == self.Address; + } + else + { + hasRole = self.HasRole(_role); + var roleLeader = cluster.State.RoleLeader(_role); + isLeader = roleLeader is not null && roleLeader == self.Address; + } } - else + catch (Exception ex) when (ex is not OperationCanceledException) { - hasRole = self.HasRole(_role); - var roleLeader = cluster.State.RoleLeader(_role); - isLeader = roleLeader is not null && roleLeader == self.Address; + return Task.FromResult(HealthCheckResult.Degraded("Akka cluster state not yet accessible.", ex)); } var health = ActiveNodeDecision.Evaluate(selfUp, isLeader, hasRole, _role); - var description = DescribeResult(health, self.Status, selfUp, isLeader); + var description = DescribeResult(health, selfStatus, selfUp, isLeader); var result = health switch { HealthStatus.Healthy => HealthCheckResult.Healthy(description), diff --git a/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/AkkaClusterHealthCheck.cs b/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/AkkaClusterHealthCheck.cs index 74e040b..302b2b2 100644 --- a/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/AkkaClusterHealthCheck.cs +++ b/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/AkkaClusterHealthCheck.cs @@ -8,7 +8,8 @@ namespace ZB.MOM.WW.Health.Akka; /// /// Health check that maps the local node's Akka cluster membership status to a /// through a configurable . -/// Register to the tag (recommended [ready, active]). +/// Register to the tag only — cluster membership is a readiness +/// concern; the tier is reserved for the leader / active-node probe. /// /// /// The is resolved lazily from the service provider. If it is not yet @@ -42,7 +43,21 @@ public sealed class AkkaClusterHealthCheck : IHealthCheck if (system is null) return Task.FromResult(HealthCheckResult.Degraded("ActorSystem not yet available.")); - var status = Cluster.Get(system).SelfMember.Status; + MemberStatus status; + try + { + // Cluster.Get(system).SelfMember can throw while the ActorSystem exists but the cluster + // has not finished initialising (e.g. Akka.Cluster not yet configured → + // ConfigurationException). The spec's startup-safety rule maps this to Degraded, not an + // escaping exception (which the host would record as Unhealthy and pull the node from + // rotation). + status = Cluster.Get(system).SelfMember.Status; + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + return Task.FromResult(HealthCheckResult.Degraded("Akka cluster state not yet accessible.", ex)); + } + var health = _policy.Evaluate(status); var description = $"Akka cluster member status: {status}"; var result = health switch diff --git a/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/GrpcDependencyHealthCheck.cs b/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/GrpcDependencyHealthCheck.cs index e1aa4c2..51bbdf2 100644 --- a/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/GrpcDependencyHealthCheck.cs +++ b/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/GrpcDependencyHealthCheck.cs @@ -13,14 +13,15 @@ namespace ZB.MOM.WW.Health; /// The probe is injectable via ; the default drives the /// channel to a connected state with . The result is /// when the probe returns true, and -/// when it returns false, throws an -/// , or times out / is cancelled within -/// . +/// when it returns false, throws any exception +/// ( or otherwise), or times out within +/// . External cancellation of the supplied +/// propagates as an . /// /// -/// Recommended registration tags: and -/// — a missing downstream gRPC dependency makes the node both -/// not-ready and not-able-to-act. The registrant applies the tags. +/// Recommended registration tag: only — downstream gRPC +/// reachability is a readiness concern; the tier is reserved for +/// the leader / active-node probe. The registrant applies the tag. /// /// public sealed class GrpcDependencyHealthCheck : IHealthCheck @@ -74,6 +75,15 @@ public sealed class GrpcDependencyHealthCheck : IHealthCheck { return HealthCheckResult.Unhealthy($"{name} probe timed out after {_options.Timeout}.", ex); } + catch (Exception ex) + { + // Catch-all to match the sibling DatabaseHealthCheck: any other probe error + // (e.g. InvalidOperationException / HttpRequestException / SocketException from the + // transport, or anything a custom probe throws) maps to Unhealthy rather than escaping + // the IHealthCheck boundary. The OCE/Rpc external-cancellation handlers above run first, + // so caller cancellation still propagates. + return HealthCheckResult.Unhealthy($"{name} probe failed: {ex.Message}", ex); + } } /// diff --git a/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZbHealthEndpointExtensions.cs b/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZbHealthEndpointExtensions.cs index 780490f..f25dc30 100644 --- a/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZbHealthEndpointExtensions.cs +++ b/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZbHealthEndpointExtensions.cs @@ -28,9 +28,9 @@ public static class ZbHealthEndpointExtensions /// emits a minimal 200 OK body. /// /// - /// The for the readiness (/health/ready) endpoint. - /// A single tier is returned (rather than a composite) to keep the API simple; conventions - /// applied to the result affect only the readiness endpoint. + /// A composite that fans every chained convention out to + /// all three health endpoints (readiness, active, and liveness). For example, + /// endpoints.MapZbHealth().RequireHost("…") gates all three endpoints, as a caller expects. /// public static IEndpointConventionBuilder MapZbHealth( this IEndpointRouteBuilder endpoints, @@ -47,7 +47,7 @@ public static class ZbHealthEndpointExtensions ResponseWriter = responseWriter, }).AllowAnonymous(); - endpoints.MapHealthChecks(options.ActivePath, new HealthCheckOptions + var active = endpoints.MapHealthChecks(options.ActivePath, new HealthCheckOptions { Predicate = static c => c.Tags.Contains(ZbHealthTags.Active), ResponseWriter = responseWriter, @@ -56,12 +56,38 @@ public static class ZbHealthEndpointExtensions // Liveness: run no checks. The endpoint returns 200 as long as the process can respond. // No JSON writer — the empty report would carry no useful data, so the framework default // (a minimal plain-text body) is sufficient. - endpoints.MapHealthChecks(options.LivePath, new HealthCheckOptions + var live = endpoints.MapHealthChecks(options.LivePath, new HealthCheckOptions { Predicate = static _ => false, }).AllowAnonymous(); - return ready; + return new CompositeEndpointConventionBuilder(ready, active, live); + } + + /// + /// An that forwards each convention to several + /// underlying builders, so conventions chained onto the result of + /// apply to all three + /// health endpoints rather than just one. + /// + private sealed class CompositeEndpointConventionBuilder : IEndpointConventionBuilder + { + private readonly IEndpointConventionBuilder[] _builders; + + public CompositeEndpointConventionBuilder(params IEndpointConventionBuilder[] builders) => + _builders = builders; + + public void Add(Action convention) + { + foreach (var builder in _builders) + builder.Add(convention); + } + + public void Finally(Action finalConvention) + { + foreach (var builder in _builders) + builder.Finally(finalConvention); + } } /// @@ -70,7 +96,10 @@ public static class ZbHealthEndpointExtensions /// /// The endpoint route builder to map onto. /// Callback that mutates a fresh . - /// The for the readiness endpoint. + /// + /// A composite that fans chained conventions out to all + /// three health endpoints. + /// public static IEndpointConventionBuilder MapZbHealth( this IEndpointRouteBuilder endpoints, Action configure) diff --git a/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZbHealthWriter.cs b/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZbHealthWriter.cs index 401626c..8aa41a1 100644 --- a/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZbHealthWriter.cs +++ b/ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZbHealthWriter.cs @@ -1,5 +1,4 @@ using System.Text.Json; -using System.Text.Json.Serialization; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Diagnostics.HealthChecks; @@ -21,15 +20,21 @@ namespace ZB.MOM.WW.Health; /// } /// } /// -/// The HTTP status code is left to the ASP.NET Core health-checks middleware (Healthy/Degraded → 200, -/// Unhealthy → 503); this writer only renders the body and sets Content-Type: application/json. +/// The description key is always present; when a check supplies no description it is emitted +/// as JSON null (not omitted), matching the spec example and the HealthChecks.UI.Client +/// shape. The HTTP status code is left to the ASP.NET Core health-checks middleware (Healthy/Degraded +/// → 200, Unhealthy → 503); this writer only renders the body and sets +/// Content-Type: application/json. /// public static class ZbHealthWriter { + // Null properties are emitted (not omitted) so a null `description` renders as + // "description": null — matching the SPEC §3 example and the HealthChecks.UI.Client shape this + // writer mirrors. Consumers can then read entries..description without handling a missing + // property. (Do not set DefaultIgnoreCondition = WhenWritingNull here.) private static readonly JsonSerializerOptions SerializerOptions = new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase, - DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, }; /// diff --git a/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Akka.Tests/ActiveNodeDecisionTests.cs b/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Akka.Tests/ActiveNodeDecisionTests.cs index 32b6063..2cb5fab 100644 --- a/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Akka.Tests/ActiveNodeDecisionTests.cs +++ b/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Akka.Tests/ActiveNodeDecisionTests.cs @@ -86,6 +86,50 @@ public sealed class ActiveNodeDecisionTests Assert.False(gate.IsActiveNode); } + [Fact] + public async Task HealthCheck_RoleLess_ClusterInaccessible_ReturnsDegraded() + { + // ActorSystem present but Akka.Cluster not configured → Cluster.Get throws. The check must + // return Degraded (startup-safety rule), not let the exception escape (→ Unhealthy). + using var system = ActorSystem.Create("plain-no-cluster-roleless"); + try + { + var provider = new ServiceCollection() + .AddSingleton(system) + .BuildServiceProvider(); + var check = new ActiveNodeHealthCheck(provider); + + var result = await check.CheckHealthAsync(NewContext(check)); + + Assert.Equal(HealthStatus.Degraded, result.Status); + } + finally + { + await system.Terminate(); + } + } + + [Fact] + public async Task HealthCheck_RoleFiltered_ClusterInaccessible_ReturnsDegraded() + { + using var system = ActorSystem.Create("plain-no-cluster-rolefiltered"); + try + { + var provider = new ServiceCollection() + .AddSingleton(system) + .BuildServiceProvider(); + var check = new ActiveNodeHealthCheck(provider, "admin"); + + var result = await check.CheckHealthAsync(NewContext(check)); + + Assert.Equal(HealthStatus.Degraded, result.Status); + } + finally + { + await system.Terminate(); + } + } + private static HealthCheckContext NewContext(IHealthCheck check) => new() { Registration = new HealthCheckRegistration("active-node", check, HealthStatus.Unhealthy, tags: null), diff --git a/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Akka.Tests/AkkaClusterStatusPolicyTests.cs b/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Akka.Tests/AkkaClusterStatusPolicyTests.cs index 406ac47..0c8bd10 100644 --- a/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Akka.Tests/AkkaClusterStatusPolicyTests.cs +++ b/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Akka.Tests/AkkaClusterStatusPolicyTests.cs @@ -1,3 +1,4 @@ +using Akka.Actor; using Akka.Cluster; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Diagnostics.HealthChecks; @@ -70,6 +71,30 @@ public sealed class AkkaClusterStatusPolicyTests Assert.Equal(HealthStatus.Degraded, result.Status); } + [Fact] + public async Task HealthCheck_ActorSystemPresentButClusterInaccessible_ReturnsDegraded() + { + // A plain (non-clustered) ActorSystem exists in DI, but Akka.Cluster is not configured, + // so Cluster.Get(system) throws a ConfigurationException — the startup race the spec calls + // out. The check must return Degraded, not let the exception escape (→ Unhealthy via the host). + using var system = ActorSystem.Create("plain-no-cluster"); + try + { + var provider = new ServiceCollection() + .AddSingleton(system) + .BuildServiceProvider(); + var check = new AkkaClusterHealthCheck(provider, AkkaClusterStatusPolicy.Default); + + var result = await check.CheckHealthAsync(NewContext(check)); + + Assert.Equal(HealthStatus.Degraded, result.Status); + } + finally + { + await system.Terminate(); + } + } + private static HealthCheckContext NewContext(IHealthCheck check) => new() { Registration = new HealthCheckRegistration("akka-cluster", check, HealthStatus.Unhealthy, tags: null), diff --git a/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Tests/GrpcDependencyHealthCheckTests.cs b/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Tests/GrpcDependencyHealthCheckTests.cs index 33769e4..df37e4c 100644 --- a/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Tests/GrpcDependencyHealthCheckTests.cs +++ b/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Tests/GrpcDependencyHealthCheckTests.cs @@ -71,6 +71,21 @@ public sealed class GrpcDependencyHealthCheckTests Assert.Contains("mxaccessgw worker", result.Description); } + [Fact] + public async Task ProbeThrowsArbitraryException_Unhealthy() + { + // A non-RpcException / non-OperationCanceledException (e.g. the transport surfacing an + // InvalidOperationException) must be caught and mapped to Unhealthy, not allowed to escape. + var result = await RunAsync(new GrpcDependencyOptions + { + DependencyName = "mxaccessgw worker", + Probe = static (_, _) => throw new InvalidOperationException("channel disposed"), + }); + + Assert.Equal(HealthStatus.Unhealthy, result.Status); + Assert.Contains("mxaccessgw worker", result.Description); + } + [Fact] public async Task ProbeExceedsTimeout_Unhealthy() { diff --git a/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Tests/ResponseWriterTests.cs b/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Tests/ResponseWriterTests.cs index dd2716f..0e9bd6b 100644 --- a/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Tests/ResponseWriterTests.cs +++ b/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Tests/ResponseWriterTests.cs @@ -31,7 +31,7 @@ public sealed class ResponseWriterTests } private static async Task GetReadyAsync( - HealthStatus status, string description = "db reachable") + HealthStatus status, string? description = "db reachable") { var builder = WebApplication.CreateBuilder(); builder.WebHost.UseTestServer(); @@ -66,6 +66,24 @@ public sealed class ResponseWriterTests Assert.Equal("db reachable", db.GetProperty("description").GetString()); } + [Fact] + public async Task ReadyEndpoint_NullDescription_EmitsDescriptionKeyAsNull() + { + // A check that produces no description must still emit the "description" key with a JSON null + // value (matching the spec §3 example and the HealthChecks.UI.Client shape) rather than + // dropping the key — so consumers can read entries..description without handling a + // missing property. + var response = await GetReadyAsync(HealthStatus.Healthy, description: null); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + using var doc = JsonDocument.Parse(await response.Content.ReadAsStringAsync()); + var db = doc.RootElement.GetProperty("entries").GetProperty("db"); + + Assert.True(db.TryGetProperty("description", out var description), "description key must be present"); + Assert.Equal(JsonValueKind.Null, description.ValueKind); + } + [Fact] public async Task ReadyEndpoint_Degraded_Returns200_WithDegradedStatus() { diff --git a/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Tests/TierMappingTests.cs b/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Tests/TierMappingTests.cs index 2cc8fbd..29b4a92 100644 --- a/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Tests/TierMappingTests.cs +++ b/ZB.MOM.WW.Health/tests/ZB.MOM.WW.Health.Tests/TierMappingTests.cs @@ -127,6 +127,31 @@ public sealed class TierMappingTests Assert.Equal(0, active.Invocations); } + [Fact] + public async Task ChainedConvention_AppliesToAllThreeEndpoints() + { + // MapZbHealth returns a composite builder, so a convention chained onto its result + // (.RequireHost) must gate all three endpoints — not just readiness. With a host filter + // that does not match the default test-client host, every tier returns 404. + var builder = WebApplication.CreateBuilder(); + builder.WebHost.UseTestServer(); + builder.Services.AddHealthChecks() + .AddCheck("ready-check", new RecordingHealthCheck(HealthStatus.Healthy), tags: new[] { ZbHealthTags.Ready }) + .AddCheck("active-check", new RecordingHealthCheck(HealthStatus.Healthy), tags: new[] { ZbHealthTags.Active }); + + await using var app = builder.Build(); + app.MapZbHealth().RequireHost("health.internal"); + await app.StartAsync(); + + var client = app.GetTestClient(); + + // The default test host does not match "health.internal", so the convention removed every + // endpoint from this host — confirming it fanned out to all three, not just readiness. + Assert.Equal(HttpStatusCode.NotFound, (await client.GetAsync("/health/ready")).StatusCode); + Assert.Equal(HttpStatusCode.NotFound, (await client.GetAsync("/health/active")).StatusCode); + Assert.Equal(HttpStatusCode.NotFound, (await client.GetAsync("/healthz")).StatusCode); + } + [Fact] public async Task Options_OverrideRoutePaths() { diff --git a/ZB.MOM.WW.Telemetry/README.md b/ZB.MOM.WW.Telemetry/README.md index 5380dbd..1b17d61 100644 --- a/ZB.MOM.WW.Telemetry/README.md +++ b/ZB.MOM.WW.Telemetry/README.md @@ -55,6 +55,14 @@ Trace↔log correlation is automatic: `TraceContextEnricher` reads `Activity.Cur log event and attaches `trace_id` and `span_id`, so log events produced inside a traced request carry the same span identity as the trace backend. +**Redaction reach.** A registered `ILogRedactor` may **remove** or **replace** any top-level +property, and `RedactionEnricher` honours both (a removed key is dropped from the event). The seam +sees the unwrapped value of scalar properties only — a destructured `{@Object}` property is exposed +as its raw Serilog `StructureValue` wrapper, so a redactor can replace/remove the whole structured +property but **cannot** mask a field nested inside it. To protect a sensitive field of a logged +object, log it as its own scalar property (do not destructure it) or remove the whole property by +key. See the `ILogRedactor` XML doc for the full contract. + --- ## Exporter options @@ -113,9 +121,9 @@ backend): | Assembly | Tests | |---|---| -| `ZB.MOM.WW.Telemetry.Tests` | 7 | -| `ZB.MOM.WW.Telemetry.Serilog.Tests` | 12 | -| **Total** | **19** | +| `ZB.MOM.WW.Telemetry.Tests` | 12 | +| `ZB.MOM.WW.Telemetry.Serilog.Tests` | 17 | +| **Total** | **29** | --- diff --git a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ILogRedactor.cs b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ILogRedactor.cs index 0df8683..049adf0 100644 --- a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ILogRedactor.cs +++ b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ILogRedactor.cs @@ -11,6 +11,20 @@ public interface ILogRedactor /// /// Inspects and mutates the supplied log-event in place — remove /// or replace any sensitive values. Called on every log event before it reaches any sink. + /// Both removing a key (the property is dropped from the event) and replacing its value are + /// honoured by . + /// + /// Reach — scalar top-level properties only. Each entry's value is the unwrapped scalar + /// of a Serilog ScalarValue property (so simple string/number/etc. properties such as + /// {apiKey} can be read and masked directly). Destructured / structured properties are + /// not unwrapped: a {@Object} property arrives as the raw Serilog + /// StructureValue wrapper (and a sequence/dictionary as SequenceValue/ + /// DictionaryValue). A redactor can therefore replace or remove the whole + /// top-level property, but it cannot reach a field nested inside a destructured object to + /// mask it selectively. To protect a sensitive field of a logged object, do not destructure it + /// (log the field as its own scalar property), or remove/replace the entire structured property + /// by key. + /// /// /// The mutable property dictionary for the current log event. void Redact(IDictionary properties); diff --git a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/RedactionEnricher.cs b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/RedactionEnricher.cs index 79b873d..1251bf9 100644 --- a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/RedactionEnricher.cs +++ b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/RedactionEnricher.cs @@ -30,8 +30,18 @@ public sealed class RedactionEnricher : ILogEventEnricher } /// - /// Hands the log event's scalar properties to the registered and - /// writes back any values the redactor changed. No-op when no redactor is registered. + /// Hands the log event's properties to the registered and reconciles + /// the result back onto the event: values the redactor changed are rewritten via + /// AddOrUpdateProperty, and keys the redactor removed are deleted via + /// RemovePropertyIfPresent. No-op when no redactor is registered or the event carries no + /// properties. + /// + /// The redactor sees the unwrapped value of each property; structured + /// values ( from {@Object}, , + /// ) are passed through as their raw Serilog wrapper. A redactor can + /// therefore replace or remove a whole structured top-level property, but cannot reach a field + /// nested inside one — see for the seam's documented reach. + /// /// /// The log event to redact. /// Factory used to materialize replacement properties. @@ -46,6 +56,12 @@ public sealed class RedactionEnricher : ILogEventEnricher return; } + // Hot path: an event with no properties has nothing to redact — skip the snapshot copy. + if (logEvent.Properties.Count == 0) + { + return; + } + var snapshot = new Dictionary(logEvent.Properties.Count); foreach (var property in logEvent.Properties) { @@ -54,6 +70,10 @@ public sealed class RedactionEnricher : ILogEventEnricher : property.Value; } + // Capture the original key set so we can honour deletions: any key the redactor drops from + // the snapshot must be removed from the event (not silently retained). + var originalKeys = new HashSet(snapshot.Keys, StringComparer.Ordinal); + redactor.Redact(snapshot); foreach (var entry in snapshot) @@ -64,6 +84,16 @@ public sealed class RedactionEnricher : ILogEventEnricher propertyFactory.CreateProperty(entry.Key, entry.Value)); } } + + // Reconcile removals: a redactor that deleted a key from the snapshot (e.g. + // properties.Remove("apiKey")) means that property must not reach any sink. + foreach (var key in originalKeys) + { + if (!snapshot.ContainsKey(key)) + { + logEvent.RemovePropertyIfPresent(key); + } + } } private ILogRedactor? ResolveRedactor() => _redactor.Value; diff --git a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogConfig.cs b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogConfig.cs index f54d49b..0053a5d 100644 --- a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogConfig.cs +++ b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogConfig.cs @@ -115,38 +115,13 @@ internal static class ZbSerilogConfig } /// - /// Builds the OTLP Resource-attribute map mirroring ZbResource. Null/empty optional - /// attributes are omitted, matching the shared Resource's omission rules. The - /// service.instance.id is sourced from — the - /// same deterministic MachineName:ProcessId value used by the OTel SDK path — so - /// all three signals carry an identical instance identifier. Internal so it can be asserted - /// by the test assembly without being part of the public NuGet API. + /// Builds the OTLP log-sink Resource-attribute map. This is not a parallel + /// implementation: it is derived directly from — the + /// single source of truth shared with the OTel SDK metrics/traces pipeline — so the log sink can + /// never drift from metrics and traces. Returned as a fresh mutable copy because the + /// Serilog OpenTelemetry sink takes ownership of the dictionary it is handed. Internal so it can + /// be asserted by the test assembly without being part of the public NuGet API. /// - internal static IDictionary BuildResourceAttributes(ZbTelemetryOptions options) - { - var attributes = new Dictionary - { - ["service.name"] = options.ServiceName, - ["service.namespace"] = options.ServiceNamespace, - ["service.instance.id"] = ZbResource.InstanceId, - ["host.name"] = Environment.MachineName, - }; - - if (!string.IsNullOrEmpty(options.ServiceVersion)) - { - attributes["service.version"] = options.ServiceVersion; - } - - if (!string.IsNullOrEmpty(options.SiteId)) - { - attributes["site.id"] = options.SiteId; - } - - if (!string.IsNullOrEmpty(options.NodeRole)) - { - attributes["node.role"] = options.NodeRole; - } - - return attributes; - } + internal static IDictionary BuildResourceAttributes(ZbTelemetryOptions options) => + new Dictionary(ZbResource.BuildAttributes(options), StringComparer.Ordinal); } diff --git a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogExtensions.cs b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogExtensions.cs index 8150434..b259b87 100644 --- a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogExtensions.cs +++ b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogExtensions.cs @@ -65,6 +65,10 @@ public static class ZbSerilogExtensions var options = new ZbTelemetryOptions(); configure(options); + // Fail fast on a malformed OTLP endpoint with a clear, named message — same validation the + // core AddZbTelemetry path uses — instead of a late error when the OTel log sink builds. + ZbTelemetryOptionsValidator.Validate(options, nameof(configure)); + // Register the application logger in DI only. preserveStaticLogger: true ensures // AddSerilog does NOT freeze or replace Log.Logger — critical for multi-host // processes (integration tests etc.) where AddZbSerilog may be called more than once. diff --git a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZB.MOM.WW.Telemetry.csproj b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZB.MOM.WW.Telemetry.csproj index 77a1b66..13fee8c 100644 --- a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZB.MOM.WW.Telemetry.csproj +++ b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZB.MOM.WW.Telemetry.csproj @@ -20,6 +20,17 @@ + + + + <_Parameter1>ZB.MOM.WW.Telemetry.Serilog + + + <_Parameter1>ZB.MOM.WW.Telemetry.Serilog.Tests + + + diff --git a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbMetricsEndpointExtensions.cs b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbMetricsEndpointExtensions.cs index ee1f88b..96b4bb7 100644 --- a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbMetricsEndpointExtensions.cs +++ b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbMetricsEndpointExtensions.cs @@ -9,8 +9,10 @@ namespace ZB.MOM.WW.Telemetry; public static class ZbMetricsEndpointExtensions { /// - /// Mounts the Prometheus /metrics endpoint. Only valid when - /// = . + /// Mounts the Prometheus /metrics endpoint. Valid under any + /// value: the Prometheus exporter is always wired by + /// AddZbTelemetry, and OTLP () is only an additive overlay — + /// so /metrics serves scrape data even when Exporter = ZbExporter.Otlp. /// Call after app.UseRouting(). /// /// The endpoint route builder. diff --git a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbResource.cs b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbResource.cs index ccda9c7..3c8b962 100644 --- a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbResource.cs +++ b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbResource.cs @@ -31,34 +31,55 @@ public static class ZbResource Configure(ResourceBuilder.CreateDefault(), options); /// - /// Applies the shared ZB.MOM.WW Resource attributes to an existing . - /// Internal seam so the AddZbTelemetry pipeline produces a Resource identical to - /// . + /// The single source of truth for the shared ZB.MOM.WW Resource attribute set. Every consumer + /// of the Resource — the OTel SDK metrics/traces pipeline () and the + /// Serilog OTLP log sink — derives its attributes from this one map, so logs can never drift + /// from metrics/traces. Required attributes (service.name, service.namespace, + /// service.instance.id, host.name) are always present; optional ones + /// (service.version, site.id, node.role) are included only when the + /// corresponding option is non-null/non-empty, matching the Resource's omission rules. /// - internal static ResourceBuilder Configure(ResourceBuilder builder, ZbTelemetryOptions options) + /// The telemetry options describing the service identity. + /// The canonical attribute map carried by all three signals. + public static IReadOnlyDictionary BuildAttributes(ZbTelemetryOptions options) { - builder.AddService( - serviceName: options.ServiceName, - serviceNamespace: options.ServiceNamespace, - serviceVersion: options.ServiceVersion, - autoGenerateServiceInstanceId: false, - serviceInstanceId: InstanceId); + ArgumentNullException.ThrowIfNull(options); - var attributes = new List> + var attributes = new Dictionary(StringComparer.Ordinal) { - new("host.name", System.Environment.MachineName), + ["service.name"] = options.ServiceName, + ["service.namespace"] = options.ServiceNamespace, + ["service.instance.id"] = InstanceId, + ["host.name"] = System.Environment.MachineName, }; + if (!string.IsNullOrEmpty(options.ServiceVersion)) + { + attributes["service.version"] = options.ServiceVersion; + } + if (!string.IsNullOrEmpty(options.SiteId)) { - attributes.Add(new("site.id", options.SiteId)); + attributes["site.id"] = options.SiteId; } if (!string.IsNullOrEmpty(options.NodeRole)) { - attributes.Add(new("node.role", options.NodeRole)); + attributes["node.role"] = options.NodeRole; } + return attributes; + } + + /// + /// Applies the shared ZB.MOM.WW Resource attributes to an existing . + /// Internal seam so the AddZbTelemetry pipeline produces a Resource identical to + /// . Derives every attribute from — the same + /// canonical map the Serilog OTLP log sink uses — so all three signals agree. + /// + internal static ResourceBuilder Configure(ResourceBuilder builder, ZbTelemetryOptions options) + { + var attributes = BuildAttributes(options); builder.AddAttributes(attributes); return builder; } diff --git a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryExtensions.cs b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryExtensions.cs index b44d5b5..34f32c7 100644 --- a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryExtensions.cs +++ b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryExtensions.cs @@ -101,6 +101,7 @@ public static class ZbTelemetryExtensions "ZbTelemetryOptions.ServiceName is required (e.g. \"otopcua\").", nameof(configure)); } + ZbTelemetryOptionsValidator.Validate(options, nameof(configure)); return options; } diff --git a/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryOptionsValidator.cs b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryOptionsValidator.cs new file mode 100644 index 0000000..b0ac495 --- /dev/null +++ b/ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryOptionsValidator.cs @@ -0,0 +1,44 @@ +namespace ZB.MOM.WW.Telemetry; + +/// +/// Eager, fail-fast validation of shared by the core +/// AddZbTelemetry path and the Serilog AddZbSerilog path, so a malformed value is +/// reported once, clearly, and with the offending option named — rather than surfacing late as a +/// bare deep inside exporter construction at host-build time. +/// +internal static class ZbTelemetryOptionsValidator +{ + /// + /// Validates the OTLP configuration. When is + /// , must be a + /// non-empty, well-formed absolute URI. Throws an (naming the + /// option) otherwise. No-op for the Prometheus exporter — a stray endpoint is ignored there. + /// + /// The populated telemetry options to validate. + /// The originating parameter name for the thrown exception. + public static void Validate(ZbTelemetryOptions options, string paramName) + { + ArgumentNullException.ThrowIfNull(options); + + if (options.Exporter != ZbExporter.Otlp) + { + return; + } + + if (string.IsNullOrWhiteSpace(options.OtlpEndpoint)) + { + throw new ArgumentException( + "ZbTelemetryOptions.OtlpEndpoint is required when Exporter = ZbExporter.Otlp " + + "(e.g. \"http://collector:4317\").", + paramName); + } + + if (!Uri.TryCreate(options.OtlpEndpoint, UriKind.Absolute, out _)) + { + throw new ArgumentException( + $"ZbTelemetryOptions.OtlpEndpoint is not a well-formed absolute URI: " + + $"'{options.OtlpEndpoint}' (expected e.g. \"http://collector:4317\").", + paramName); + } + } +} diff --git a/ZB.MOM.WW.Telemetry/tests/ZB.MOM.WW.Telemetry.Serilog.Tests/RedactionTests.cs b/ZB.MOM.WW.Telemetry/tests/ZB.MOM.WW.Telemetry.Serilog.Tests/RedactionTests.cs index bbe2b18..03870de 100644 --- a/ZB.MOM.WW.Telemetry/tests/ZB.MOM.WW.Telemetry.Serilog.Tests/RedactionTests.cs +++ b/ZB.MOM.WW.Telemetry/tests/ZB.MOM.WW.Telemetry.Serilog.Tests/RedactionTests.cs @@ -25,6 +25,30 @@ public sealed class RedactionTests } } + private sealed class RemovingRedactor : ILogRedactor + { + private readonly string _key; + + public RemovingRedactor(string key) => _key = key; + + public void Redact(IDictionary properties) => properties.Remove(_key); + } + + private sealed class StructuredFieldRedactor : ILogRedactor + { + // Attempts to mask a nested field of a destructured ({@Object}) property by mutating the + // value the seam exposes. Documents that the seam reaches scalar top-level properties only. + public void Redact(IDictionary properties) + { + if (properties.TryGetValue("command", out var value) && value is StructureValue) + { + // The seam exposes the raw StructureValue wrapper, not a mutable dictionary of the + // object's fields, so a project redactor cannot reach inside to mask a nested field. + properties["command"] = Masked; + } + } + } + private static string? ScalarOrNull(LogEvent logEvent, string propertyName) => logEvent.Properties.TryGetValue(propertyName, out var value) && value is ScalarValue scalar ? scalar.Value?.ToString() @@ -68,6 +92,68 @@ public sealed class RedactionTests Assert.Equal("mxgw_secret", ScalarOrNull(logEvent, "apiKey")); } + /// + /// Telemetry-001: a redactor that REMOVES a key (the most natural way to implement "must not + /// leave the process") must result in the property being absent from the emitted event, not + /// silently retained. + /// + [Fact] + public void Removing_redactor_scrubs_the_property_from_the_event() + { + var serviceProvider = new ServiceCollection() + .AddSingleton(new RemovingRedactor("apiKey")) + .BuildServiceProvider(); + + var sink = new InMemorySink(); + var options = new ZbTelemetryOptions { ServiceName = "mxgateway" }; + + var loggerConfig = new LoggerConfiguration(); + ZbSerilogConfig.Apply(loggerConfig, options, serviceProvider); + using Logger logger = loggerConfig.WriteTo.Sink(sink).CreateLogger(); + + logger.Information("authenticating {apiKey} for {user}", "mxgw_secret", "alice"); + + var logEvent = Assert.Single(sink.LogEvents); + Assert.False( + logEvent.Properties.ContainsKey("apiKey"), + "apiKey must be removed from the event when the redactor removes the key"); + // A non-sensitive property the redactor left alone must survive. + Assert.Equal("alice", ScalarOrNull(logEvent, "user")); + } + + /// + /// Telemetry-002/003: the redaction seam reaches scalar top-level properties only. A + /// destructured ({@Object}) property is exposed to the redactor as the raw Serilog + /// wrapper, so a project redactor cannot mask a field nested + /// inside the object — it can only replace/remove the whole top-level property. This test + /// pins that documented limitation (see ILogRedactor XML doc and the shared contract). + /// + [Fact] + public void Redactor_cannot_reach_a_field_inside_a_destructured_object() + { + var serviceProvider = new ServiceCollection() + .AddSingleton(new StructuredFieldRedactor()) + .BuildServiceProvider(); + + var sink = new InMemorySink(); + var options = new ZbTelemetryOptions { ServiceName = "mxgateway" }; + + var loggerConfig = new LoggerConfiguration(); + ZbSerilogConfig.Apply(loggerConfig, options, serviceProvider); + using Logger logger = loggerConfig.WriteTo.Sink(sink).CreateLogger(); + + var command = new { Name = "Write", ApiKey = "mxgw_secret" }; + logger.Information("dispatching {@command}", command); + + var logEvent = Assert.Single(sink.LogEvents); + Assert.True(logEvent.Properties.TryGetValue("command", out var value)); + + // The property was destructured into a StructureValue and exposed to the redactor as that + // wrapper. The redactor recognized it and replaced the whole top-level property with the + // mask — confirming the seam can only act at top-level granularity for structured values. + Assert.Equal(Masked, (value as ScalarValue)?.Value?.ToString()); + } + [Fact] public void AddZbSerilog_with_otlp_options_builds_without_error() { @@ -122,6 +208,32 @@ public sealed class RedactionTests Assert.Equal("central", attributes["node.role"]); } + /// + /// Telemetry-005: the Serilog OTLP log-sink attribute map and the OTel SDK metrics/traces + /// attribute map must be key-for-key and value-for-value identical, because both now derive from + /// the single source of truth. This pins that they + /// cannot silently drift apart. + /// + [Fact] + public void Serilog_and_OTel_resource_attribute_sets_are_identical() + { + var options = new ZbTelemetryOptions + { + ServiceName = "mxgateway", + ServiceNamespace = "ZB.MOM.WW", + ServiceVersion = "9.9.9", + SiteId = "site-z", + NodeRole = "hub", + }; + + var serilogAttributes = ZbSerilogConfig.BuildResourceAttributes(options); + var canonical = ZbResource.BuildAttributes(options); + + Assert.Equal( + canonical.OrderBy(kvp => kvp.Key, StringComparer.Ordinal), + serilogAttributes.OrderBy(kvp => kvp.Key, StringComparer.Ordinal)); + } + [Fact] public void BuildResourceAttributes_omits_optional_keys_when_not_set() { diff --git a/ZB.MOM.WW.Telemetry/tests/ZB.MOM.WW.Telemetry.Tests/AddZbTelemetryTests.cs b/ZB.MOM.WW.Telemetry/tests/ZB.MOM.WW.Telemetry.Tests/AddZbTelemetryTests.cs index 024859f..427d29d 100644 --- a/ZB.MOM.WW.Telemetry/tests/ZB.MOM.WW.Telemetry.Tests/AddZbTelemetryTests.cs +++ b/ZB.MOM.WW.Telemetry/tests/ZB.MOM.WW.Telemetry.Tests/AddZbTelemetryTests.cs @@ -37,6 +37,55 @@ public sealed class AddZbTelemetryTests Assert.Equal("configure", ex.ParamName); } + // Telemetry-006: malformed/missing OtlpEndpoint must fail fast with a clear, named error + // instead of a late UriFormatException deep inside exporter construction. + + [Fact] + public void AddZbTelemetry_Throws_WhenOtlpExporterHasMalformedEndpoint() + { + var builder = WebApplication.CreateBuilder(); + var ex = Assert.Throws(() => + builder.AddZbTelemetry(o => + { + o.ServiceName = "telemetry-test"; + o.Exporter = ZbExporter.Otlp; + o.OtlpEndpoint = "not a uri"; // missing scheme — not an absolute URI + })); + Assert.Equal("configure", ex.ParamName); + Assert.Contains("OtlpEndpoint", ex.Message); + } + + [Fact] + public void AddZbTelemetry_Throws_WhenOtlpExporterHasNoEndpoint() + { + var builder = WebApplication.CreateBuilder(); + var ex = Assert.Throws(() => + builder.AddZbTelemetry(o => + { + o.ServiceName = "telemetry-test"; + o.Exporter = ZbExporter.Otlp; + // OtlpEndpoint left null + })); + Assert.Equal("configure", ex.ParamName); + Assert.Contains("OtlpEndpoint", ex.Message); + } + + [Fact] + public void AddZbTelemetry_DoesNotValidateEndpoint_WhenExporterIsPrometheus() + { + // A stray (even malformed) endpoint is harmless under the Prometheus exporter and must not + // be validated — it is ignored. + var builder = WebApplication.CreateBuilder(); + var ex = Record.Exception(() => + builder.AddZbTelemetry(o => + { + o.ServiceName = "telemetry-test"; + o.Exporter = ZbExporter.Prometheus; + o.OtlpEndpoint = "not a uri"; + })); + Assert.Null(ex); + } + // Fix #1: Prometheus coexists with OTLP — /metrics must still serve under Otlp exporter [Fact] @@ -48,8 +97,10 @@ public sealed class AddZbTelemetryTests { o.ServiceName = "telemetry-test"; o.Exporter = ZbExporter.Otlp; - // OtlpEndpoint intentionally left null — exporter will be registered but won't - // connect anywhere; we are only verifying Prometheus remains present. + // A well-formed endpoint is required under the Otlp exporter (Telemetry-006); the + // exporter is registered but won't connect anywhere in the test. We are only verifying + // Prometheus remains present. + o.OtlpEndpoint = "http://localhost:4317"; o.Meters = ["Test.OtlpCoexist.Meter"]; }); diff --git a/ZB.MOM.WW.Theme/README.md b/ZB.MOM.WW.Theme/README.md index da25a9f..fcb2040 100644 --- a/ZB.MOM.WW.Theme/README.md +++ b/ZB.MOM.WW.Theme/README.md @@ -7,6 +7,11 @@ woff2 fonts, and side-rail layout CSS — all served from `_content/ZB.MOM.WW.Th plus a set of Blazor SSR components that carry no inline colours and reuse the token classes. Bootstrap 5 is **not** vendored; each app keeps its own Bootstrap link. +The kit ships **no JavaScript**: every interactive affordance — the narrow-viewport +side-rail hamburger and the collapsible nav sections — is a native CSS-only +`
`/`` disclosure, so the chrome works unchanged in static Blazor SSR +with only Bootstrap **CSS** present (no Bootstrap collapse JS bundle required). + ## Adopt 1. Reference the NuGet package in your app; keep your own Bootstrap 5 `` in `App.razor`. @@ -83,11 +88,16 @@ Served at `_content/ZB.MOM.WW.Theme/…` by the ASP.NET static-web-asset pipelin `theme.css` declares `@font-face` with `url('../fonts/…')` — correct relative path from `css/` to `fonts/`. (OtOpcUa's original `url('fonts/…')` was a latent 404; the kit fixes it.) +All colours are CSS custom-property tokens declared once in `theme.css`'s `:root` block — +no hardcoded hex appears outside it. Derived border/hover/wash shades (e.g. `--ok-border`, +`--warn-ink`, `--info-bg`, `--hover-bg`, `--active-bg`, `--zebra-bg`) are named tokens too, +so a token-block edit re-themes the whole kit. + ## Build ```bash # from ZB.MOM.WW.Theme/ dotnet build -c Release # TreatWarningsAsErrors — expect 0 warnings -dotnet test # 32 bUnit tests +dotnet test # 38 bUnit tests ./build/pack.sh # → ./artifacts/ZB.MOM.WW.Theme.0.1.0.nupkg ``` diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/ButtonVariant.cs b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/ButtonVariant.cs index 37d0146..b2d654d 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/ButtonVariant.cs +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/ButtonVariant.cs @@ -1,3 +1,19 @@ namespace ZB.MOM.WW.Theme; -public enum ButtonVariant { Primary, Secondary, Danger, Ghost } +/// +/// Visual variant for TechButton, mapped to a Bootstrap .btn-* class. +/// +public enum ButtonVariant +{ + /// Primary action — emits btn-primary (filled accent button). + Primary, + + /// Secondary action — emits btn-outline-secondary (outlined button). + Secondary, + + /// Destructive action — emits btn-danger (filled red button). + Danger, + + /// Low-emphasis action — emits btn-link (text-only button). + Ghost, +} diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/BrandBar.razor b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/BrandBar.razor index d1420d9..f462fb1 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/BrandBar.razor +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/BrandBar.razor @@ -7,6 +7,11 @@ @code { + /// Product name displayed next to the brand glyph. Required. [Parameter, EditorRequired] public string Product { get; set; } = string.Empty; + + /// + /// Optional custom logo. When provided it replaces the default brand glyph (▐). + /// [Parameter] public RenderFragment? Logo { get; set; } } diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/LoginCard.razor b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/LoginCard.razor index 0f6fb3e..d15fc15 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/LoginCard.razor +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/LoginCard.razor @@ -36,7 +36,12 @@ @code { + /// Product name shown in the card heading. Required. [Parameter, EditorRequired] public string Product { get; set; } = string.Empty; + + /// + /// Form action URL the sign-in POST targets. Defaults to /auth/login. + /// [Parameter] public string Action { get; set; } = "/auth/login"; /// @@ -47,6 +52,9 @@ /// [Parameter] public string? ReturnUrl { get; set; } + /// + /// Optional error message shown as a notice above the submit button (e.g. a failed login). + /// [Parameter] public string? Error { get; set; } /// diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailItem.razor b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailItem.razor index dd5d982..d1e464d 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailItem.razor +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailItem.razor @@ -6,8 +6,21 @@ @code { + /// Link target (the href of the underlying NavLink). Required. [Parameter, EditorRequired] public string Href { get; set; } = string.Empty; + + /// Visible label text for the rail link. Required. [Parameter, EditorRequired] public string Text { get; set; } = string.Empty; + + /// + /// Optional leading icon, rendered in a .rail-ico span before the label. + /// [Parameter] public RenderFragment? Icon { get; set; } + + /// + /// Active-class matching behaviour passed to the underlying NavLink. + /// Defaults to ; use + /// for exact matches such as the home/overview link. + /// [Parameter] public NavLinkMatch Match { get; set; } = NavLinkMatch.Prefix; } diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailSection.razor b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailSection.razor index b620734..50268c0 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailSection.razor +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailSection.razor @@ -2,12 +2,24 @@ Apps that want cookie-persisted expand state keep their own interactive NavSection. *@ @namespace ZB.MOM.WW.Theme
- @Title + @Title
@ChildContent
@code { + /// + /// Eyebrow label shown in the section's <summary> header. Required. + /// [Parameter, EditorRequired] public string Title { get; set; } = string.Empty; + + /// + /// Initial open state of the collapsible section. Defaults to true (expanded). + /// Backed by the native <details open> attribute — no JavaScript required. + /// [Parameter] public bool Expanded { get; set; } = true; + + /// + /// Section content — typically children. + /// [Parameter] public RenderFragment? ChildContent { get; set; } } diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/StatusPill.razor b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/StatusPill.razor index 99de38a..aaaaf53 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/StatusPill.razor +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/StatusPill.razor @@ -3,7 +3,13 @@ @ChildContent @code { + /// + /// Status the chip represents. Mapped to a token-based .chip-* class + /// (e.g. → chip-ok). Required. + /// [Parameter, EditorRequired] public StatusState State { get; set; } + + /// Chip label content (e.g. the status text). [Parameter] public RenderFragment? ChildContent { get; set; } private string ChipClass => State switch diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechButton.razor b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechButton.razor index 871db30..1cba5f6 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechButton.razor +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechButton.razor @@ -6,10 +6,28 @@ @code { + /// + /// Visual variant, mapped to a Bootstrap .btn-* class. + /// Defaults to . + /// [Parameter] public ButtonVariant Variant { get; set; } = ButtonVariant.Primary; + + /// HTML type attribute (button, submit, reset). Defaults to button. [Parameter] public string Type { get; set; } = "button"; + + /// + /// When true, disables the button and shows an inline spinner — use while an + /// async action is in flight. Defaults to false. + /// [Parameter] public bool Busy { get; set; } + + /// Button label content. [Parameter] public RenderFragment? ChildContent { get; set; } + + /// + /// Arbitrary additional HTML attributes splatted onto the underlying + /// <button> (e.g. id, @onclick, name, value). + /// [Parameter(CaptureUnmatchedValues = true)] public IDictionary? AdditionalAttributes { get; set; } private string VariantClass => Variant switch diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechCard.razor b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechCard.razor index 8277737..5c8d6b9 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechCard.razor +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechCard.razor @@ -8,9 +8,23 @@ @code { + /// + /// Optional string title rendered in the panel header. Ignored when + /// is supplied ( takes precedence). + /// [Parameter] public string? Title { get; set; } + + /// + /// Optional custom panel header content. Takes precedence over . + /// [Parameter] public RenderFragment? Header { get; set; } + + /// Panel body content. [Parameter] public RenderFragment? ChildContent { get; set; } + + /// Optional panel footer content (padded, top-bordered). [Parameter] public RenderFragment? Footer { get; set; } + + /// Additional CSS classes appended to the root <section class="panel">. [Parameter] public string? Class { get; set; } } diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechField.razor b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechField.razor index 0ba0d55..5da4c29 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechField.razor +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechField.razor @@ -8,8 +8,17 @@ @code { + /// Field label text. Required. [Parameter, EditorRequired] public string Label { get; set; } = string.Empty; + + /// Optional hint text rendered as .form-text below the input. [Parameter] public string? Hint { get; set; } + + /// + /// Optional inline error message rendered as .field-error.s-bad below the input. + /// [Parameter] public string? Error { get; set; } + + /// The control itself — an <input>, <select>, etc. [Parameter] public RenderFragment? ChildContent { get; set; } } diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/ThemeShell.razor b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/ThemeShell.razor index 0e4977b..d2973a7 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/ThemeShell.razor +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/ThemeShell.razor @@ -1,13 +1,15 @@ @* Components/ThemeShell.razor — the one canonical side-rail chassis. - Not a LayoutComponentBase: the app's thin MainLayout delegates to this. *@ + Not a LayoutComponentBase: the app's thin MainLayout delegates to this. + The narrow-viewport hamburger uses a CSS-only
disclosure (no + Bootstrap JS): the rail is force-shown on lg+ and toggles via the native +
open state below lg, so it works in pure static SSR. *@ @namespace ZB.MOM.WW.Theme -
- -
+
+
@ChildContent
- +
@code { + /// + /// Product name shown in the rail's header. Required. + /// [Parameter, EditorRequired] public string Product { get; set; } = string.Empty; + + /// + /// Optional per-app accent colour. When set, emitted as style="--accent: <value>" + /// on the shell root, overriding the --accent design token for this subtree only. + /// This is the single per-app token override the kit allows (e.g. "#2f855a"). + /// [Parameter] public string? Accent { get; set; } + + /// + /// Optional custom logo rendered in the rail header. When omitted, the default + /// brand glyph is shown. + /// [Parameter] public RenderFragment? Logo { get; set; } + + /// + /// Rail navigation content — typically / + /// elements. + /// [Parameter] public RenderFragment? Nav { get; set; } + + /// + /// Optional content pinned to the bottom of the rail (e.g. session info / sign-out). + /// [Parameter] public RenderFragment? RailFooter { get; set; } + + /// + /// The page body — normally @Body forwarded from the app's MainLayout. + /// [Parameter] public RenderFragment? ChildContent { get; set; } private string? AccentStyle => Accent is null ? null : $"--accent: {Accent}"; diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/StatusState.cs b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/StatusState.cs index c39b646..3fce878 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/StatusState.cs +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/StatusState.cs @@ -1,3 +1,23 @@ namespace ZB.MOM.WW.Theme; -public enum StatusState { Ok, Warn, Bad, Idle, Info } +/// +/// Status conveyed by a StatusPill chip, mapped to a token-based +/// .chip-* class (status is communicated by colour, not iconography). +/// +public enum StatusState +{ + /// Success / healthy / connected — emits chip-ok (green). + Ok, + + /// Warning / degraded — emits chip-warn (amber). + Warn, + + /// Error / faulted / disconnected — emits chip-bad (red). + Bad, + + /// Unknown / offline / neutral — emits chip-idle (grey). The default. + Idle, + + /// Informational — emits chip-info (on-palette blue). + Info, +} diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/ZB.MOM.WW.Theme.csproj b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/ZB.MOM.WW.Theme.csproj index 5362dc5..9376b32 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/ZB.MOM.WW.Theme.csproj +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/ZB.MOM.WW.Theme.csproj @@ -2,6 +2,13 @@ ZB.MOM.WW.Theme true + + true + $(NoWarn);CS1591 true ZB.MOM.WW.Theme Shared Technical-Light UI kit (tokens, fonts, side-rail shell, widgets) for the ZB.MOM.WW SCADA family. diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/layout.css b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/layout.css index c155f43..bb74c69 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/layout.css +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/layout.css @@ -2,9 +2,13 @@ Tokens live in theme.css; this sheet carries only layout + the side rail. */ /* ── App shell: side rail + page ─────────────────────────────────────────── */ -/* The outer flex direction is supplied by Bootstrap utilities on the wrapper - (`d-flex flex-column flex-lg-row`) so the mobile hamburger row stacks above - the rail on disclosure so the narrow-viewport hamburger + toggle works with NO JavaScript (no Bootstrap collapse bundle): below lg the + hamburger opens/closes the rail; on lg+ the rail is force-shown + regardless of the open state (see the lg media query below). The outer flex + direction is supplied by Bootstrap utilities on the wrapper + (`d-flex flex-column flex-lg-row`) so the hamburger row stacks above the rail + on : hide the native disclosure triangle so it reads as a + plain button (the styling comes from the Bootstrap .btn utilities). */ +.app-shell > summary.rail-toggle { + list-style: none; + cursor: pointer; +} +.app-shell > summary.rail-toggle::-webkit-details-marker { display: none; } + +/* Below lg the native
shows/hides #theme-rail via [open]; nothing + extra is needed there. On lg+ we force the rail visible (see media query) + and keep the toggle hidden via Bootstrap's d-lg-none. */ + /* ── Side rail ───────────────────────────────────────────────────────────── */ .side-rail { width: 220px; @@ -27,9 +43,12 @@ border-right: 1px solid var(--rule-strong); } -/* On lg+ keep the side rail pinned so it stays visible when content scrolls. */ +/* On lg+ keep the side rail pinned so it stays visible when content scrolls, + and force it shown regardless of the
open state (the hamburger + toggle is hidden at this width). */ @media (min-width: 992px) { #theme-rail { + display: block; position: sticky; top: 0; height: 100vh; @@ -38,8 +57,9 @@ } } -/* When the side rail is collapsed under is closed) + the native disclosure hides #theme-rail; when open, restore full width on + mobile so the rail spans the viewport. */ @media (max-width: 991.98px) { .side-rail { width: 100%; @@ -72,17 +92,9 @@ } .side-rail .brand .mark { color: var(--accent); } -.rail-eyebrow { - font-size: 0.68rem; - font-weight: 600; - text-transform: uppercase; - letter-spacing: 0.07em; - color: var(--ink-faint); - padding: 0.3rem 0.6rem; -} - -/* Collapsible variant — rendered by NavRailSection. Looks like .rail-eyebrow - plus a leading chevron; clicking flips chevron + expanded state. */ +/* Section eyebrow — the label of a NavRailSection. The collapse + chevron is supplied by the `.rail-section > summary::before` rule below; this + rule styles the uppercase eyebrow text itself. */ .rail-eyebrow-toggle { display: flex; align-items: center; @@ -100,12 +112,6 @@ cursor: pointer; } .rail-eyebrow-toggle:hover { color: var(--ink); } -.rail-eyebrow-chevron { - display: inline-block; - width: 0.7rem; - font-size: 0.55rem; - color: var(--ink-faint); -} .rail-section-body { display: flex; flex-direction: column; @@ -120,17 +126,28 @@ color: var(--ink-soft); } .rail-link:hover { - background: #f3f6fd; + background: var(--hover-bg); color: var(--ink); text-decoration: none; } .rail-link.active { - background: #eef2fc; + background: var(--active-bg); border-left-color: var(--accent); color: var(--accent-deep); font-weight: 600; } +/* Optional leading icon inside a rail link (NavRailItem's Icon slot). Sizes and + aligns the icon and gives it a gap from the label text. */ +.rail-ico { + display: inline-flex; + align-items: center; + justify-content: center; + width: 1rem; + margin-right: 0.4rem; + color: var(--ink-faint); +} + /* ── Session block, pinned to the rail foot ──────────────────────────────── */ .rail-foot { margin-top: auto; @@ -180,8 +197,8 @@ .rail-section > summary::before { content: '\25B6'; font-size: 0.55rem; color: var(--ink-faint); margin-right: 0.4rem; } .rail-section[open] > summary::before { content: '\25BC'; } -/* StatusPill: info variant (on-palette, reuses dir-read colours) */ -.chip-info { color: var(--accent-deep); background: #e7ecfb; border-color: #cdd9f7; } +/* StatusPill: info variant (on-palette, reuses the info blue wash) */ +.chip-info { color: var(--accent-deep); background: var(--info-bg); border-color: var(--info-border); } /* TechCard body/footer padding; TechField error; LoginCard body */ .panel-body { padding: 0.85rem 0.9rem; } diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/theme.css b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/theme.css index 53fb793..b662f94 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/theme.css +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/theme.css @@ -62,6 +62,29 @@ --bad-bg: #fceaea; --idle-bg: #eef0f2; + /* Status — derived border tints (lighter than the foreground; for chips, + pills, and tinted cards). Pair with the matching -bg above. */ + --ok-border: #c6e6cd; + --warn-border: #efd6a6; + --bad-border: #eec3c3; + --ok-border-soft: #bfe3c6; /* conn pill (slightly lighter) */ + --warn-border-soft: #f0d9ab; /* conn pill (slightly lighter) */ + --bad-border-soft: #f0c0c0; /* conn pill (slightly lighter) */ + + /* Warning ink — a deeper amber than --warn, for chip/notice/value text that + needs more contrast on the warm tint than the pure --warn would give. */ + --warn-ink: #b56a00; + --warn-ink-deep: #8a5a00; + + /* Info / accent tint — on-palette blue wash (StatusPill Info, dir-read tag) */ + --info-bg: #e7ecfb; + --info-border: #cdd9f7; + + /* Neutral surface washes — derived from --paper/--accent, no own foreground */ + --zebra-bg: #fbfbf9; /* even-row / sticky-head fill — barely-there */ + --hover-bg: #f3f6fd; /* row / rail-link hover — faint cool wash */ + --active-bg: #eef2fc; /* active rail-link fill */ + /* Type stacks — Plex first, graceful system fallback */ --mono: 'IBM Plex Mono', ui-monospace, 'Cascadia Mono', Consolas, monospace; --sans: 'IBM Plex Sans', system-ui, -apple-system, 'Segoe UI', sans-serif; @@ -81,7 +104,7 @@ not a gradient wash. Keep it subtle. */ body { background: - radial-gradient(1200px 480px at 88% -8%, #ffffff 0%, rgba(255,255,255,0) 70%), + radial-gradient(1200px 480px at 88% -8%, var(--card) 0%, rgba(255,255,255,0) 70%), var(--paper); color: var(--ink); font-family: var(--sans); @@ -142,11 +165,11 @@ a:hover { color: var(--accent-deep); text-decoration: underline; } width: 7px; height: 7px; border-radius: 50%; background: var(--idle); } -.conn-pill[data-state="connected"] { color: var(--ok); border-color: #bfe3c6; background: var(--ok-bg); } +.conn-pill[data-state="connected"] { color: var(--ok); border-color: var(--ok-border-soft); background: var(--ok-bg); } .conn-pill[data-state="connected"] .dot { background: var(--ok); } -.conn-pill[data-state="connecting"] { color: var(--warn); border-color: #f0d9ab; background: var(--warn-bg); } +.conn-pill[data-state="connecting"] { color: var(--warn); border-color: var(--warn-border-soft); background: var(--warn-bg); } .conn-pill[data-state="connecting"] .dot { background: var(--warn); animation: pulse 1.1s ease-in-out infinite; } -.conn-pill[data-state="disconnected"] { color: var(--bad); border-color: #f0c0c0; background: var(--bad-bg); } +.conn-pill[data-state="disconnected"] { color: var(--bad); border-color: var(--bad-border-soft); background: var(--bad-bg); } .conn-pill[data-state="disconnected"] .dot { background: var(--bad); } @keyframes pulse { 0%,100% { opacity: 1; } 50% { opacity: 0.25; } } @@ -171,10 +194,10 @@ a:hover { color: var(--accent-deep); text-decoration: underline; } border-radius: 4px; border: 1px solid transparent; } -.chip-ok { color: var(--ok); background: var(--ok-bg); border-color: #c6e6cd; } -.chip-warn { color: #b56a00; background: var(--warn-bg); border-color: #efd6a6; } -.chip-bad { color: var(--bad); background: var(--bad-bg); border-color: #eec3c3; } -.chip-idle { color: var(--ink-soft); background: var(--idle-bg); border-color: var(--rule-strong); } +.chip-ok { color: var(--ok); background: var(--ok-bg); border-color: var(--ok-border); } +.chip-warn { color: var(--warn-ink); background: var(--warn-bg); border-color: var(--warn-border); } +.chip-bad { color: var(--bad); background: var(--bad-bg); border-color: var(--bad-border); } +.chip-idle { color: var(--idle); background: var(--idle-bg); border-color: var(--rule-strong); } /* ── Panel — the base raised surface ───────────────────────────────────────── A white card with a hairline border and 8px radius. .panel-head is the @@ -248,10 +271,10 @@ a:hover { color: var(--accent-deep); text-decoration: underline; } font-weight: 400; color: var(--ink-faint); } -.agg-card.alert { border-color: #eec3c3; background: var(--bad-bg); } +.agg-card.alert { border-color: var(--bad-border); background: var(--bad-bg); } .agg-card.alert .agg-value { color: var(--bad); } -.agg-card.caution { border-color: #efd6a6; background: var(--warn-bg); } -.agg-card.caution .agg-value { color: #b56a00; } +.agg-card.caution { border-color: var(--warn-border); background: var(--warn-bg); } +.agg-card.caution .agg-value { color: var(--warn-ink); } /* ── Metric card + key/value rows ──────────────────────────────────────────── A .panel-head over a stack of .kv rows: label left, monospace value right. @@ -278,7 +301,7 @@ a:hover { color: var(--accent-deep); text-decoration: underline; } padding: 0.32rem 0.9rem; font-size: 0.85rem; } -.kv:nth-child(even) { background: #fbfbf9; } +.kv:nth-child(even) { background: var(--zebra-bg); } .kv .k { color: var(--ink-soft); } .kv .v { font-family: var(--mono); @@ -332,7 +355,7 @@ a:hover { color: var(--accent-deep); text-decoration: underline; } text-transform: uppercase; letter-spacing: 0.05em; color: var(--ink-faint); - background: #fbfbf9; + background: var(--zebra-bg); position: sticky; top: 0; } @@ -345,7 +368,7 @@ a:hover { color: var(--accent-deep); text-decoration: underline; } .data-table th.sorted-desc::after { content: ' \2193'; color: var(--accent); } .data-table tbody tr { cursor: pointer; transition: background 0.08s; } -.data-table tbody tr:hover { background: #f3f6fd; } +.data-table tbody tr:hover { background: var(--hover-bg); } .data-table tbody tr:last-child td { border-bottom: none; } .empty-row { @@ -365,15 +388,15 @@ a:hover { color: var(--accent-deep); text-decoration: underline; } padding: 0.1rem 0.4rem; border-radius: 3px; } -.dir-read { color: var(--accent-deep); background: #e7ecfb; } -.dir-write { color: #8a5a00; background: var(--warn-bg); } +.dir-read { color: var(--accent-deep); background: var(--info-bg); } +.dir-write { color: var(--warn-ink-deep); background: var(--warn-bg); } /* ── Inline notice ─────────────────────────────────────────────────────────── A .panel with a warning tint — for "this thing is gone / degraded" banners. */ .notice { padding: 0.85rem 1.1rem; margin-bottom: 1rem; - color: #b56a00; + color: var(--warn-ink); background: var(--warn-bg); - border-color: #efd6a6; + border-color: var(--warn-border); } diff --git a/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/NavRailTests.cs b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/NavRailTests.cs index 479a05b..2d6eddd 100644 --- a/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/NavRailTests.cs +++ b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/NavRailTests.cs @@ -13,6 +13,28 @@ public class NavRailTests : TestContext Assert.Contains("Clusters", a.TextContent); } + // Theme-004: when Icon is supplied it is wrapped in a .rail-ico span (now styled). + [Fact] + public void NavRailItem_wraps_icon_in_rail_ico_span_when_supplied() + { + var cut = RenderComponent(p => p + .Add(x => x.Href, "/clusters") + .Add(x => x.Text, "Clusters") + .Add(x => x.Icon, (RenderFragment)(b => b.AddMarkupContent(0, "")))); + var ico = cut.Find("a.rail-link .rail-ico"); + Assert.NotNull(ico); + Assert.NotNull(cut.Find("a.rail-link .rail-ico .ico")); + } + + [Fact] + public void NavRailItem_omits_rail_ico_span_when_no_icon() + { + var cut = RenderComponent(p => p + .Add(x => x.Href, "/clusters") + .Add(x => x.Text, "Clusters")); + Assert.Empty(cut.FindAll(".rail-ico")); + } + [Fact] public void NavRailSection_renders_title_and_children_open_by_default() { diff --git a/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/StaticAssetsTests.cs b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/StaticAssetsTests.cs index 2bc89ec..73b3da2 100644 --- a/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/StaticAssetsTests.cs +++ b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/StaticAssetsTests.cs @@ -1,4 +1,6 @@ using System.IO; +using System.Linq; +using System.Text.RegularExpressions; namespace ZB.MOM.WW.Theme.Tests; @@ -32,4 +34,36 @@ public class StaticAssetsTests [InlineData("ibm-plex-mono-500.woff2")] public void Fonts_are_vendored(string file) => Assert.True(File.Exists(Path.Combine(Wwwroot, "fonts", file))); + + // Theme-002: .chip-idle pairs the idle background with the matching --idle + // foreground token (per DESIGN-TOKENS.md), not --ink-soft. + [Fact] + public void ChipIdle_pairs_idle_foreground_with_idle_background() + { + var css = File.ReadAllText(Path.Combine(Wwwroot, "css", "theme.css")); + var rule = Regex.Match(css, @"\.chip-idle\s*\{[^}]*\}").Value; + Assert.Contains("color: var(--idle)", rule); + Assert.Contains("background: var(--idle-bg)", rule); + Assert.DoesNotContain("--ink-soft", rule); + } + + // Theme-003: no hardcoded hex colours appear outside the :root token block in + // either stylesheet — every shade is a named token. + [Theory] + [InlineData("theme.css")] + [InlineData("layout.css")] + public void No_hardcoded_hex_outside_root_token_block(string file) + { + var css = File.ReadAllText(Path.Combine(Wwwroot, "css", file)); + + // Strip the :root { ... } declaration block(s) — the one place hex literals live. + var withoutRoot = Regex.Replace(css, @":root\s*\{[^}]*\}", string.Empty); + + var hexLiterals = Regex.Matches(withoutRoot, @"#[0-9a-fA-F]{3,8}\b") + .Select(m => m.Value) + .ToList(); + + Assert.True(hexLiterals.Count == 0, + $"{file} has hardcoded hex outside :root: {string.Join(", ", hexLiterals)}"); + } } diff --git a/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/ThemeShellTests.cs b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/ThemeShellTests.cs index cb8c6c8..67f282f 100644 --- a/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/ThemeShellTests.cs +++ b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/ThemeShellTests.cs @@ -40,4 +40,26 @@ public class ThemeShellTests : TestContext .Add(x => x.RailFooter, (RenderFragment)(b => b.AddMarkupContent(0, "S")))); Assert.NotNull(cut.Find(".rail-foot .sess")); } + + // Theme-001: the mobile hamburger must work without Bootstrap collapse JS. + // The shell is a native
disclosure whose is the toggle — + // no data-bs-toggle / data-bs-target attributes, so no Bootstrap JS dependency. + [Fact] + public void Mobile_toggle_is_a_css_only_details_disclosure_not_bootstrap_collapse() + { + var cut = RenderComponent(p => p.Add(x => x.Product, "OtOpcUa")); + + // Root is a
whose direct is the hamburger toggle. + var shell = cut.Find("details.app-shell"); + var summary = cut.Find("details.app-shell > summary.rail-toggle"); + Assert.NotNull(summary); + + // The rail is plain markup inside the
— no Bootstrap .collapse class. + var rail = cut.Find("#theme-rail"); + Assert.DoesNotContain("collapse", rail.ClassList); + + // No Bootstrap collapse JS hooks anywhere in the shell markup. + Assert.DoesNotContain("data-bs-toggle", shell.OuterHtml); + Assert.DoesNotContain("data-bs-target", shell.OuterHtml); + } } diff --git a/components/observability/shared-contract/ZB.MOM.WW.Telemetry.md b/components/observability/shared-contract/ZB.MOM.WW.Telemetry.md index bd5c6ff..1ebfe67 100644 --- a/components/observability/shared-contract/ZB.MOM.WW.Telemetry.md +++ b/components/observability/shared-contract/ZB.MOM.WW.Telemetry.md @@ -124,17 +124,32 @@ public static class ZbTelemetryExtensions /// Used internally by AddZbTelemetry. Exposed for tests and custom pipelines. public static class ZbResource { + /// Deterministic, process-stable service instance identifier, formatted as + /// "MachineName:ProcessId". Populates OTel Resource service.instance.id on every signal + /// (metrics, traces, logs). The OTel SDK's random-GUID default is disabled in favour of this + /// value so all signals from one process share one restart-stable instance id, enabling + /// cross-signal correlation. Always present (not optional). + public static string InstanceId { get; } + /// Returns a ResourceBuilder pre-populated with service.name, service.namespace, - /// service.version, site.id, node.role, and host.name (always Environment.MachineName). - /// Attributes with null values are omitted from the Resource. + /// service.version, service.instance.id (always — see InstanceId), site.id, node.role, and + /// host.name (always Environment.MachineName). Attributes with null/empty values are omitted. public static ResourceBuilder Build(ZbTelemetryOptions options); + + /// The single source of truth for the shared Resource attribute set. Both the OTel SDK + /// metrics/traces pipeline and the Serilog OTLP log sink derive their attributes from this one + /// map, so logs cannot drift from metrics/traces. service.name/namespace/instance.id/host.name + /// are always present; service.version/site.id/node.role are included only when the option is + /// non-null/non-empty. + public static IReadOnlyDictionary BuildAttributes(ZbTelemetryOptions options); } /// Endpoint extension for mounting the Prometheus /metrics scrape endpoint. public static class ZbMetricsEndpointExtensions { - /// Mounts the Prometheus /metrics endpoint. - /// Only valid when ZbTelemetryOptions.Exporter = ZbExporter.Prometheus (or both). + /// Mounts the Prometheus /metrics endpoint. Valid under ANY ZbTelemetryOptions.Exporter value: + /// AddZbTelemetry always wires the Prometheus exporter, and OTLP (ZbExporter.Otlp) is only an + /// additive overlay — so /metrics serves scrape data even when Exporter = ZbExporter.Otlp. /// Call after app.UseRouting(). public static IEndpointConventionBuilder MapZbMetrics( this IEndpointRouteBuilder endpoints); diff --git a/components/observability/spec/METRIC-CONVENTIONS.md b/components/observability/spec/METRIC-CONVENTIONS.md index 077aa86..0e6751c 100644 --- a/components/observability/spec/METRIC-CONVENTIONS.md +++ b/components/observability/spec/METRIC-CONVENTIONS.md @@ -104,6 +104,7 @@ joinable in any OTel-compatible backend. | `service.name` | string | Yes | Short lower-case app id: `otopcua`, `mxgateway`, `scadabridge` | | `service.namespace` | string | Yes | Always `"ZB.MOM.WW"` — do not override | | `service.version` | string | Recommended | Populate from `AssemblyInformationalVersion`; absent is better than wrong | +| `service.instance.id` | string | Auto | Always `ZbResource.InstanceId` = deterministic `MachineName:ProcessId`. The OTel SDK random-GUID default is disabled so every signal from one process shares one restart-stable instance id (cross-signal correlation); never override | | `site.id` | string | Recommended | Physical or logical site identifier; omit for single-site deployments | | `node.role` | string | Recommended | Node function: `"central"`, `"site"`, `"hub"`, `"standalone"` | | `host.name` | string | Auto | Always `Environment.MachineName`; never override | diff --git a/components/observability/spec/SPEC.md b/components/observability/spec/SPEC.md index 208cc9b..b54e12f 100644 --- a/components/observability/spec/SPEC.md +++ b/components/observability/spec/SPEC.md @@ -68,6 +68,7 @@ The OTel `Resource` attached to all three signals is built from `ZbTelemetryOpti | `service.name` | `ServiceName` | Required. Lower-case short identifier (`otopcua`, `mxgateway`, `scadabridge`) | | `service.namespace` | `ServiceNamespace` | Default `"ZB.MOM.WW"` — constant across the fleet | | `service.version` | `ServiceVersion` | Optional; recommend populating from `AssemblyInformationalVersion` | +| `service.instance.id` | _(auto)_ | Always populated from `ZbResource.InstanceId` — a deterministic `MachineName:ProcessId`. The OTel SDK's random-GUID default is disabled so every signal from the same process carries an identical, restart-stable instance id for cross-signal correlation | | `site.id` | `SiteId` | Optional; identifies the physical/logical site | | `node.role` | `NodeRole` | Optional; e.g. `"central"`, `"site"`, `"hub"` | | `host.name` | _(auto)_ | Always populated from `Environment.MachineName` |