diff --git a/code-reviews/NotificationService/findings.md b/code-reviews/NotificationService/findings.md index ae3b0e0..8b865e1 100644 --- a/code-reviews/NotificationService/findings.md +++ b/code-reviews/NotificationService/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-17 | | Reviewer | claude-agent | | Commit reviewed | `39d737e` | -| Open findings | 5 | +| Open findings | 0 | ## Summary @@ -497,7 +497,7 @@ Module test suite is green at 56 tests. |--|--| | Severity | High | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:214-228`, `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:308-312`, `src/ScadaLink.NotificationService/OAuth2TokenService.cs:56-84` | **Description** @@ -510,7 +510,7 @@ Add a catch-all to `DeliverBufferedAsync` for exceptions that `ClassifySmtpError **Resolution** -_Unresolved._ +Resolved 2026-05-17. Root cause confirmed against source — `DeliverBufferedAsync` caught only `SmtpPermanentException`, so an OAuth2 token-fetch `HttpRequestException`/`InvalidOperationException` escaped the handler and the S&F engine reinterpreted any throw as transient. Added a final `catch (Exception ex)` to `DeliverBufferedAsync` that decides deliberately: an `HttpRequestException` with a 5xx token-endpoint status re-throws (transient, retry); every other unclassified cause (a 401/4xx token rejection, a malformed-credential `InvalidOperationException`) returns `false` so the message parks immediately. Caller-cancellation and typed transient SMTP errors are re-thrown via dedicated filters above it. Tests `DeliverBuffered_OAuth2MalformedCredentials_ReturnsFalseSoMessageParks`, `DeliverBuffered_OAuth2TokenEndpoint401_ReturnsFalseSoMessageParks`, `DeliverBuffered_OAuth2TokenEndpoint503_ThrowsSoEngineRetries`. ### NotificationService-015 — Unclassified exceptions (OAuth2 token fetch, non-cancellation OCE) escape `SendAsync` to the calling script @@ -518,7 +518,7 @@ _Unresolved._ |--|--| | Severity | High | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:96-148`, `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:308-312` | **Description** @@ -531,7 +531,7 @@ Add a final `catch (Exception ex)` to `SendAsync` that converts any otherwise-un **Resolution** -_Unresolved._ +Resolved 2026-05-17. Root cause confirmed — `SendAsync` had only three catch clauses and an `Unknown`-classified exception (OAuth2 `HttpRequestException`/`InvalidOperationException`) fell through all of them and escaped to the calling script. Added a final `catch (Exception ex)` to `SendAsync` that converts any otherwise-unhandled exception into a credential-scrubbed `NotificationResult(false, "Notification delivery failed: ...")` and logs it; caller-requested cancellation is still re-thrown by the filter above so it never reaches the catch-all. The obsolete NS-003 test that asserted such an exception escapes was re-triaged to assert the clean result instead. Tests `Send_OAuth2TokenFetchFails_ReturnsCleanError_DoesNotThrow`, `Send_OAuth2MalformedCredentials_ReturnsCleanError_DoesNotThrow`, `Send_UnclassifiedException_RedactsCredentialFromResult`. ### NotificationService-016 — `AuthenticateAsync` silently sends unauthenticated for an unknown auth type or empty credentials @@ -539,7 +539,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:46-67` | **Description** @@ -552,7 +552,7 @@ Make missing/unparseable credentials and an unrecognised `AuthType` hard errors: **Resolution** -_Unresolved._ +Resolved 2026-05-17. Root cause confirmed — `AuthenticateAsync` returned silently for null/empty credentials, had no `default:` arm, and skipped a "basic" credential that did not split into two parts, so the connection sent mail unauthenticated. All three now throw `SmtpPermanentException` (a permanent configuration fault); because the exception is permanent, `SendAsync` returns a clean `NotificationResult` failure and `DeliverBufferedAsync` parks the buffered message — no unauthenticated send is ever attempted. Tests `Authenticate_EmptyCredentials_Throws`, `Authenticate_UnknownAuthType_Throws`, `Authenticate_BasicCredentialWithoutColon_Throws` in the new `MailKitSmtpClientWrapperTests`. ### NotificationService-017 — `NotificationOptions` is bound from configuration but never read (dead config) @@ -560,7 +560,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Code organization & conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/NotificationOptions.cs:1-15`, `src/ScadaLink.NotificationService/ServiceCollectionExtensions.cs:10-11`, `src/ScadaLink.Host/SiteServiceRegistration.cs:70` | **Description** @@ -573,7 +573,7 @@ Either delete `NotificationOptions` and both of its registrations, or genuinely **Resolution** -_Unresolved._ +Resolved 2026-05-17. Root cause confirmed — `NotificationOptions` was bound but never read. Implemented the documented-fallback intent rather than deleting it: `NotificationDeliveryService` now takes an optional `IOptions` and uses its `ConnectionTimeoutSeconds`/`MaxConcurrentConnections` whenever the deployed `SmtpConfiguration` field is non-positive (a value on the row still wins). The misleading XML doc on `NotificationOptions` was corrected to describe the precedence accurately. The duplicate `services.Configure` in `Host/SiteServiceRegistration.cs:70` is harmless (DI keeps a single bound instance) and lives outside this module's edit scope, so it was left in place. Tests `Send_SmtpConfigTimeoutUnset_FallsBackToNotificationOptions`, `Send_SmtpConfigTimeoutSet_OverridesNotificationOptions`. ### NotificationService-018 — Concurrency limiter: lock-free read of a non-volatile field, never resized on redeployment, never disposed @@ -581,7 +581,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Concurrency & thread safety | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:237-255` | **Description** @@ -594,4 +594,4 @@ Replace the hand-rolled double-checked init with `Lazy` or `LazyI **Resolution** -_Unresolved._ +Resolved 2026-05-17. All three issues confirmed against source. The hand-rolled double-checked init was replaced with a `Lazy` — its publication is correctly synchronised, eliminating the lock-free read of a non-`volatile` reference. `NotificationDeliveryService` now implements `IDisposable` and disposes the limiter (if created) under the existing lock, with idempotent re-entry and an `ObjectDisposedException` guard in `SendAsync`/`GetConcurrencyLimiter`; the scoped DI registration disposes it per scope. The limiter remains scoped (not hoisted to a site singleton) — the design doc deploys one SMTP config per site and the per-instance capture is bounded; the redeploy-resize concern is acknowledged as low-impact and not changed here, since hoisting would require a registration change for marginal benefit. Tests `Service_Dispose_DisposesConcurrencyLimiter` plus the existing `Send_MaxConcurrentConnections_LimitsConcurrentDeliveries`. diff --git a/src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs b/src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs index cc9fe79..8dca622 100644 --- a/src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs +++ b/src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs @@ -45,17 +45,30 @@ public class MailKitSmtpClientWrapper : ISmtpClientWrapper, IDisposable public async Task AuthenticateAsync(string authType, string? credentials, CancellationToken cancellationToken = default) { + // NS-016: missing/unparseable credentials and an unrecognised auth type used + // to make this method silently return and the connection then sent mail + // unauthenticated — masking a misconfiguration against an open relay and, at + // worst, sending where authentication was required. Authentication being + // skipped must never be silent: each of these is a permanent configuration + // fault, surfaced as SmtpPermanentException so SendAsync returns a clean + // failure and DeliverBufferedAsync parks the buffered message. if (string.IsNullOrEmpty(credentials)) - return; + { + throw new SmtpPermanentException( + $"SMTP auth type '{authType}' requires credentials, but none are configured."); + } switch (authType.ToLowerInvariant()) { case "basic": var parts = credentials.Split(':', 2); - if (parts.Length == 2) + if (parts.Length != 2) { - await _client.AuthenticateAsync(parts[0], parts[1], cancellationToken); + throw new SmtpPermanentException( + "Basic SMTP credentials must be in 'username:password' form."); } + + await _client.AuthenticateAsync(parts[0], parts[1], cancellationToken); break; case "oauth2": @@ -63,6 +76,10 @@ public class MailKitSmtpClientWrapper : ISmtpClientWrapper, IDisposable var oauth2 = new SaslMechanismOAuth2("", credentials); await _client.AuthenticateAsync(oauth2, cancellationToken); break; + + default: + throw new SmtpPermanentException( + $"Unsupported SMTP auth type '{authType}'. Expected one of: basic, oauth2."); } } diff --git a/src/ScadaLink.NotificationService/NotificationDeliveryService.cs b/src/ScadaLink.NotificationService/NotificationDeliveryService.cs index baeaa98..999d8e5 100644 --- a/src/ScadaLink.NotificationService/NotificationDeliveryService.cs +++ b/src/ScadaLink.NotificationService/NotificationDeliveryService.cs @@ -3,6 +3,7 @@ using System.Text.Json; using MailKit; using MailKit.Net.Smtp; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using MimeKit; using ScadaLink.Commons.Entities.Notifications; using ScadaLink.Commons.Interfaces.Repositories; @@ -18,26 +19,31 @@ namespace ScadaLink.NotificationService; /// Transient: connection refused, timeout, SMTP 4xx → hand to S&F. /// Permanent: SMTP 5xx → returned to script. /// -public class NotificationDeliveryService : INotificationDeliveryService +public class NotificationDeliveryService : INotificationDeliveryService, IDisposable { private readonly INotificationRepository _repository; private readonly Func _smtpClientFactory; private readonly OAuth2TokenService? _tokenService; private readonly StoreAndForwardService? _storeAndForward; private readonly ILogger _logger; + private readonly NotificationOptions _options; public NotificationDeliveryService( INotificationRepository repository, Func smtpClientFactory, ILogger logger, OAuth2TokenService? tokenService = null, - StoreAndForwardService? storeAndForward = null) + StoreAndForwardService? storeAndForward = null, + IOptions? options = null) { _repository = repository; _smtpClientFactory = smtpClientFactory; _logger = logger; _tokenService = tokenService; _storeAndForward = storeAndForward; + // NS-017: NotificationOptions supplies the documented fallback values used + // when a deployed SmtpConfiguration row leaves a field unset (non-positive). + _options = options?.Value ?? new NotificationOptions(); } /// @@ -50,6 +56,8 @@ public class NotificationDeliveryService : INotificationDeliveryService string? originInstanceName = null, CancellationToken cancellationToken = default) { + ObjectDisposedException.ThrowIf(_disposed, this); + var list = await _repository.GetListByNameAsync(listName, cancellationToken); if (list == null) { @@ -146,6 +154,24 @@ public class NotificationDeliveryService : INotificationDeliveryService return new NotificationResult(true, null, WasBuffered: true); } + catch (Exception ex) + { + // NS-015: a failure that ClassifySmtpError does not recognise (Unknown) — + // most importantly an OAuth2 token-fetch failure (HttpRequestException + // from EnsureSuccessStatusCode, or InvalidOperationException from a + // malformed credential triple) — used to fall through all the catch + // clauses above and escape SendAsync as a raw exception to the calling + // script, which the INotificationDeliveryService contract never + // advertises. Convert any otherwise-unhandled exception into a clean, + // credential-scrubbed permanent NotificationResult: returning control to + // the script is the safe default. (A caller-requested cancellation is + // already re-thrown by the filter above and never reaches here.) + var detail = CredentialRedactor.Scrub(ex.Message, smtpConfig.Credentials); + _logger.LogError( + "Unclassified failure sending to list {List} ({ExceptionType}): {Detail}", + listName, ex.GetType().Name, detail); + return new NotificationResult(false, $"Notification delivery failed: {detail}"); + } } /// @@ -224,36 +250,103 @@ public class NotificationDeliveryService : INotificationDeliveryService payload.ListName, CredentialRedactor.Scrub(ex.Message, smtpConfig.Credentials)); return false; } - // Transient SMTP errors propagate out of DeliverAsync — the S&F engine retries. + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + // A handler shutdown cancellation is neither a delivery success nor a + // permanent failure — let it propagate so the engine does not park. + throw; + } + catch (Exception ex) when (IsTransientSmtpError(ex, cancellationToken)) + { + // A typed transient SMTP error: re-throw so the S&F engine retries. + throw; + } + catch (Exception ex) + { + // NS-014: an exception ClassifySmtpError does not recognise (Unknown) — + // chiefly an OAuth2 token-fetch failure — used to escape this handler. + // The S&F engine treats ANY thrown exception as transient, so a + // permanently-broken config (bad client secret, malformed credential + // triple) was retried on every sweep until MaxRetries, burning token + // endpoint calls. Decide deliberately rather than letting it leak: + // - an HttpRequestException with a 5xx token-endpoint status is a + // transient outage → re-throw so the engine retries; + // - everything else (a 4xx/401 token rejection, a malformed credential + // InvalidOperationException, any other unclassified fault) is not + // fixable by retrying → return false so the message is parked. + if (ex is HttpRequestException { StatusCode: { } status } && (int)status is >= 500 and < 600) + { + _logger.LogWarning( + "Buffered notification to list '{List}' hit a transient OAuth2 token-endpoint error ({Status}); will retry.", + payload.ListName, (int)status); + throw; + } + + _logger.LogError( + "Buffered notification to list '{List}' failed with a non-retryable error ({ExceptionType}: {Detail}); parking.", + payload.ListName, ex.GetType().Name, + CredentialRedactor.Scrub(ex.Message, smtpConfig.Credentials)); + return false; + } } private sealed record BufferedNotification(string ListName, string Subject, string Message); /// /// NS-007: throttles concurrent SMTP deliveries to the configured - /// MaxConcurrentConnections. Created lazily from the first SMTP config - /// seen (one SMTP config is deployed per site, so the limit is stable). + /// MaxConcurrentConnections. One SMTP config is deployed per site, so the + /// limit is a stable per-site invariant; it is captured lazily on first use. + /// NS-018: a replaces the hand-rolled double-checked + /// init — its publication is correctly synchronised (no lock-free read of a + /// non-volatile field) and it is disposed in . /// - private SemaphoreSlim? _concurrencyLimiter; + private Lazy? _concurrencyLimiter; private readonly object _limiterLock = new(); + private bool _disposed; private SemaphoreSlim GetConcurrencyLimiter(SmtpConfiguration config) { - if (_concurrencyLimiter != null) - { - return _concurrencyLimiter; - } + // NS-018: the limiter is sized once; capture the size now so the Lazy + // factory does not close over a value that could change between calls. + var configured = config.MaxConcurrentConnections > 0 + ? config.MaxConcurrentConnections + // NS-017: fall back to the NotificationOptions value, then the + // design-doc default of 5, when the deployed row leaves it unset. + : _options.MaxConcurrentConnections > 0 ? _options.MaxConcurrentConnections : 5; lock (_limiterLock) { - // NS-007: a non-positive configured value would make SemaphoreSlim - // throw; fall back to the design-doc default of 5. - var max = config.MaxConcurrentConnections > 0 ? config.MaxConcurrentConnections : 5; - _concurrencyLimiter ??= new SemaphoreSlim(max, max); - return _concurrencyLimiter; + ObjectDisposedException.ThrowIf(_disposed, this); + _concurrencyLimiter ??= new Lazy( + () => new SemaphoreSlim(configured, configured)); + return _concurrencyLimiter.Value; } } + /// + /// NS-018: disposes the lazily-created concurrency limiter. The service is a + /// scoped DI service; without this the leaked a + /// handle per scope. + /// + public void Dispose() + { + lock (_limiterLock) + { + if (_disposed) + { + return; + } + + _disposed = true; + if (_concurrencyLimiter is { IsValueCreated: true } limiter) + { + limiter.Value.Dispose(); + } + } + + GC.SuppressFinalize(this); + } + /// /// NS-008: Validates the sender and recipient email addresses, returning a /// human-readable error string if any is malformed, or null if all parse. @@ -300,8 +393,13 @@ public class NotificationDeliveryService : INotificationDeliveryService try { // NS-005/NS-007: explicit TLS mode and the configured connection timeout. + // NS-017: when the deployed SmtpConfiguration row leaves the timeout + // unset (non-positive), fall back to the NotificationOptions value. + var timeoutSeconds = config.ConnectionTimeoutSeconds > 0 + ? config.ConnectionTimeoutSeconds + : _options.ConnectionTimeoutSeconds; await smtp.ConnectAsync( - config.Host, config.Port, tlsMode, config.ConnectionTimeoutSeconds, cancellationToken); + config.Host, config.Port, tlsMode, timeoutSeconds, cancellationToken); // Resolve credentials (OAuth2 token fetched/cached by the token service). var credentials = config.Credentials; diff --git a/src/ScadaLink.NotificationService/NotificationOptions.cs b/src/ScadaLink.NotificationService/NotificationOptions.cs index 4429bdc..0cc6186 100644 --- a/src/ScadaLink.NotificationService/NotificationOptions.cs +++ b/src/ScadaLink.NotificationService/NotificationOptions.cs @@ -1,15 +1,26 @@ namespace ScadaLink.NotificationService; /// -/// Configuration options for the Notification Service. -/// Most SMTP configuration is stored in the database (SmtpConfiguration entity). -/// This provides fallback defaults and operational limits. +/// Configuration options for the Notification Service, bound from the +/// ScadaLink:Notification configuration section. +/// +/// SMTP settings are primarily carried by the deployed SmtpConfiguration +/// entity. NS-017: these values are the fallback used by +/// when the corresponding +/// SmtpConfiguration field is left unset (non-positive) on a partially +/// deployed row — a value present on the row always takes precedence. /// public class NotificationOptions { - /// Default connection timeout for SMTP connections. + /// + /// Connection timeout (seconds) used when SmtpConfiguration.ConnectionTimeoutSeconds + /// is unset. Default 30s. + /// public int ConnectionTimeoutSeconds { get; set; } = 30; - /// Maximum concurrent SMTP connections. + /// + /// Maximum concurrent SMTP connections used when + /// SmtpConfiguration.MaxConcurrentConnections is unset. Default 5. + /// public int MaxConcurrentConnections { get; set; } = 5; } diff --git a/tests/ScadaLink.NotificationService.Tests/MailKitSmtpClientWrapperTests.cs b/tests/ScadaLink.NotificationService.Tests/MailKitSmtpClientWrapperTests.cs new file mode 100644 index 0000000..0ee9208 --- /dev/null +++ b/tests/ScadaLink.NotificationService.Tests/MailKitSmtpClientWrapperTests.cs @@ -0,0 +1,45 @@ +namespace ScadaLink.NotificationService.Tests; + +/// +/// NS-016: must never +/// silently skip authentication for a misconfigured SMTP config — a missing +/// credential, an unrecognised auth type, or an unparseable Basic credential +/// must be a hard, surfaced error rather than an unauthenticated send. +/// +public class MailKitSmtpClientWrapperTests +{ + [Fact] + public async Task Authenticate_EmptyCredentials_Throws() + { + // An AuthType of "basic"/"oauth2" with a null/empty Credentials value is a + // misconfigured row; the wrapper used to "return" and send unauthenticated. + var wrapper = new MailKitSmtpClientWrapper(); + + await Assert.ThrowsAsync( + () => wrapper.AuthenticateAsync("basic", null)); + await Assert.ThrowsAsync( + () => wrapper.AuthenticateAsync("oauth2", "")); + } + + [Fact] + public async Task Authenticate_UnknownAuthType_Throws() + { + // The switch had cases only for "basic"/"oauth2" and no default — any other + // value (typo, future "ntlm") fell through and sent unauthenticated. + var wrapper = new MailKitSmtpClientWrapper(); + + await Assert.ThrowsAsync( + () => wrapper.AuthenticateAsync("ntlm", "user:pass")); + } + + [Fact] + public async Task Authenticate_BasicCredentialWithoutColon_Throws() + { + // A "basic" credential string that does not split into exactly two parts was + // silently skipped — the connection then sent unauthenticated. + var wrapper = new MailKitSmtpClientWrapper(); + + await Assert.ThrowsAsync( + () => wrapper.AuthenticateAsync("basic", "nocolon")); + } +} diff --git a/tests/ScadaLink.NotificationService.Tests/NotificationDeliveryServiceTests.cs b/tests/ScadaLink.NotificationService.Tests/NotificationDeliveryServiceTests.cs index 4ad790d..7312089 100644 --- a/tests/ScadaLink.NotificationService.Tests/NotificationDeliveryServiceTests.cs +++ b/tests/ScadaLink.NotificationService.Tests/NotificationDeliveryServiceTests.cs @@ -1,3 +1,4 @@ +using System.Net; using System.Text.Json; using MailKit; using MailKit.Net.Smtp; @@ -326,23 +327,25 @@ public class NotificationDeliveryServiceTests } [Fact] - public async Task Send_NonSmtpExceptionWith5xxLookalikeText_NotPromotedToPermanent() + public async Task Send_NonSmtpExceptionWith5xxLookalikeText_NotClassifiedAsPermanentSmtpError() { // NS-003: the old classifier promoted ANY exception whose message contained // "5." / "550" / etc. to a permanent SMTP error — so an unrelated failure // referencing a host like "smtp5.example.com" was silently swallowed as a - // clean permanent NotificationResult. Classification now uses MailKit's + // clean "Permanent SMTP error" result. Classification now uses MailKit's // typed exceptions only, so a non-SMTP exception is no longer misclassified. + // NS-015: that unclassified exception no longer escapes SendAsync either — it + // returns a clean generic "delivery failed" result (NOT "Permanent SMTP error"). SetupHappyPath(); _smtpClient.SendAsync(Arg.Any(), Arg.Any>(), Arg.Any(), Arg.Any(), Arg.Any()) .Throws(new InvalidOperationException("internal error talking to smtp5.example.com")); var service = CreateService(); + var result = await service.SendAsync("ops-team", "Alert", "Body"); - // The exception is not classified at all (not a typed SMTP failure); it - // surfaces rather than being mistaken for a permanent 5xx rejection. - await Assert.ThrowsAsync( - () => service.SendAsync("ops-team", "Alert", "Body")); + Assert.False(result.Success); + // It is reported as a generic delivery failure, not mistaken for a 5xx rejection. + Assert.DoesNotContain("Permanent SMTP error", result.ErrorMessage); } [Fact] @@ -788,4 +791,243 @@ public class NotificationDeliveryServiceTests Assert.Equal("oauth2", recording.AuthType); Assert.Equal("oauth2-access-token-xyz", recording.Credentials); } + + // ── NotificationService-015: unclassified exceptions must not escape SendAsync ── + + /// + /// An whose token endpoint returns a non-success + /// HTTP status, so GetTokenAsync throws . + /// + private static OAuth2TokenService CreateFailingTokenService( + HttpStatusCode status = HttpStatusCode.Unauthorized) + { + var factory = Substitute.For(); + factory.CreateClient(Arg.Any()) + .Returns(_ => new HttpClient(new FailingHttpHandler(status))); + return new OAuth2TokenService(factory, NullLogger.Instance); + } + + private sealed class FailingHttpHandler : HttpMessageHandler + { + private readonly System.Net.HttpStatusCode _status; + public FailingHttpHandler(System.Net.HttpStatusCode status) => _status = status; + protected override Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + => Task.FromResult(new HttpResponseMessage(_status) + { + Content = new StringContent("error") + }); + } + + [Fact] + public async Task Send_OAuth2TokenFetchFails_ReturnsCleanError_DoesNotThrow() + { + // NS-015: an OAuth2 token-fetch failure (HttpRequestException from + // EnsureSuccessStatusCode) is classified Unknown — it fell through all three + // catch clauses and escaped SendAsync as a raw exception to the calling + // script. It must instead produce a clean NotificationResult failure. + var cfg = new SmtpConfiguration("smtp.office365.com", "oauth2", "noreply@example.com") + { + Id = 1, Port = 587, Credentials = "tenant1:client1:secret1", TlsMode = "starttls" + }; + SetupHappyPathWithSmtp(cfg); + + var service = new NotificationDeliveryService( + _repository, + () => new RecordingAuthClient(), + NullLogger.Instance, + tokenService: CreateFailingTokenService()); + + var result = await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.False(result.Success); + Assert.NotNull(result.ErrorMessage); + } + + [Fact] + public async Task Send_OAuth2MalformedCredentials_ReturnsCleanError_DoesNotThrow() + { + // NS-015: a malformed tenant:client:secret triple makes GetTokenAsync throw + // InvalidOperationException — also Unknown-classified, also escaped SendAsync. + var cfg = new SmtpConfiguration("smtp.office365.com", "oauth2", "noreply@example.com") + { + Id = 1, Port = 587, Credentials = "no-colons-here", TlsMode = "starttls" + }; + SetupHappyPathWithSmtp(cfg); + + var service = new NotificationDeliveryService( + _repository, + () => new RecordingAuthClient(), + NullLogger.Instance, + tokenService: CreateTokenService("unused")); + + var result = await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.False(result.Success); + Assert.NotNull(result.ErrorMessage); + } + + [Fact] + public async Task Send_UnclassifiedException_RedactsCredentialFromResult() + { + // NS-015: the catch-all result, like the permanent-error path (NS-009), must + // not leak credential fragments echoed in an exception message. + var cfg = new SmtpConfiguration("smtp.example.com", "basic", "noreply@example.com") + { + Id = 1, Port = 587, Credentials = "svcuser:Hunter2Secret", TlsMode = "starttls" + }; + SetupHappyPathWithSmtp(cfg); + _smtpClient.SendAsync(Arg.Any(), Arg.Any>(), Arg.Any(), Arg.Any(), Arg.Any()) + .Throws(new InvalidOperationException("internal failure exposing Hunter2Secret")); + + var service = CreateService(); + var result = await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.False(result.Success); + Assert.DoesNotContain("Hunter2Secret", result.ErrorMessage); + } + + // ── NotificationService-014: unclassified exceptions must not escape DeliverBufferedAsync ── + + [Fact] + public async Task DeliverBuffered_OAuth2MalformedCredentials_ReturnsFalseSoMessageParks() + { + // NS-014: DeliverBufferedAsync caught only SmtpPermanentException. An OAuth2 + // InvalidOperationException (a permanent, unfixable misconfiguration) escaped + // the handler; the S&F engine reinterprets any thrown exception as transient + // and retries forever. A non-retryable cause must park (return false). + var cfg = new SmtpConfiguration("smtp.office365.com", "oauth2", "noreply@example.com") + { + Id = 1, Port = 587, Credentials = "no-colons-here", TlsMode = "starttls" + }; + SetupHappyPathWithSmtp(cfg); + + var service = new NotificationDeliveryService( + _repository, + () => new RecordingAuthClient(), + NullLogger.Instance, + tokenService: CreateTokenService("unused")); + + var delivered = await service.DeliverBufferedAsync(BufferedNotification("ops-team")); + + Assert.False(delivered); // parked — retrying cannot fix a malformed credential + } + + [Fact] + public async Task DeliverBuffered_OAuth2TokenEndpoint401_ReturnsFalseSoMessageParks() + { + // NS-014: a 401 from the OAuth2 token endpoint is a permanent credential + // failure — it must park, not be retried on every sweep. + var cfg = new SmtpConfiguration("smtp.office365.com", "oauth2", "noreply@example.com") + { + Id = 1, Port = 587, Credentials = "tenant1:client1:secret1", TlsMode = "starttls" + }; + SetupHappyPathWithSmtp(cfg); + + var service = new NotificationDeliveryService( + _repository, + () => new RecordingAuthClient(), + NullLogger.Instance, + tokenService: CreateFailingTokenService(HttpStatusCode.Unauthorized)); + + var delivered = await service.DeliverBufferedAsync(BufferedNotification("ops-team")); + + Assert.False(delivered); // parked — a 401 is a permanent credential failure + } + + [Fact] + public async Task DeliverBuffered_OAuth2TokenEndpoint503_ThrowsSoEngineRetries() + { + // NS-014: a 5xx from the OAuth2 token endpoint is a transient outage — the + // handler must throw so the S&F engine retries on the next sweep. + var cfg = new SmtpConfiguration("smtp.office365.com", "oauth2", "noreply@example.com") + { + Id = 1, Port = 587, Credentials = "tenant1:client1:secret1", TlsMode = "starttls" + }; + SetupHappyPathWithSmtp(cfg); + + var service = new NotificationDeliveryService( + _repository, + () => new RecordingAuthClient(), + NullLogger.Instance, + tokenService: CreateFailingTokenService(HttpStatusCode.ServiceUnavailable)); + + await Assert.ThrowsAnyAsync( + () => service.DeliverBufferedAsync(BufferedNotification("ops-team"))); + } + + // ── NotificationService-017: NotificationOptions used as fallback for unset SmtpConfiguration fields ── + + [Fact] + public async Task Send_SmtpConfigTimeoutUnset_FallsBackToNotificationOptions() + { + // NS-017: NotificationOptions was bound from configuration but never read. + // It is now the documented fallback: when SmtpConfiguration.ConnectionTimeoutSeconds + // is non-positive (0 from a partially-deployed row) the NotificationOptions + // value is used instead of leaving the timeout unbounded. + var cfg = new SmtpConfiguration("smtp.example.com", "basic", "noreply@example.com") + { + Id = 1, Port = 587, Credentials = "user:pass", TlsMode = "starttls", + ConnectionTimeoutSeconds = 0 // not configured on the row + }; + SetupHappyPathWithSmtp(cfg); + var recording = new RecordingTlsClient(); + + var options = Microsoft.Extensions.Options.Options.Create( + new NotificationOptions { ConnectionTimeoutSeconds = 42, MaxConcurrentConnections = 5 }); + var service = new NotificationDeliveryService( + _repository, () => recording, NullLogger.Instance, + options: options); + + await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.Equal(42, recording.ConnectionTimeoutSeconds); + } + + [Fact] + public async Task Send_SmtpConfigTimeoutSet_OverridesNotificationOptions() + { + // NS-017: a value present on the SmtpConfiguration row still wins over the + // NotificationOptions fallback. + var cfg = new SmtpConfiguration("smtp.example.com", "basic", "noreply@example.com") + { + Id = 1, Port = 587, Credentials = "user:pass", TlsMode = "starttls", + ConnectionTimeoutSeconds = 19 + }; + SetupHappyPathWithSmtp(cfg); + var recording = new RecordingTlsClient(); + + var options = Microsoft.Extensions.Options.Options.Create( + new NotificationOptions { ConnectionTimeoutSeconds = 42 }); + var service = new NotificationDeliveryService( + _repository, () => recording, NullLogger.Instance, + options: options); + + await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.Equal(19, recording.ConnectionTimeoutSeconds); + } + + // ── NotificationService-018: concurrency limiter disposal ── + + [Fact] + public async Task Service_Dispose_DisposesConcurrencyLimiter() + { + // NS-018: the lazily-created SemaphoreSlim was never disposed and the service + // did not implement IDisposable — a slow handle leak per scope. Disposing the + // service must dispose the limiter; using it afterwards must fault. + SetupHappyPath(); + var service = CreateService(); + + // A send creates the limiter. + await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.IsAssignableFrom(service); + ((IDisposable)service).Dispose(); + + // A second send after disposal must fail fast on the disposed semaphore + // rather than silently using a disposed object. + await Assert.ThrowsAnyAsync( + () => service.SendAsync("ops-team", "Alert", "Body")); + } }