diff --git a/code-reviews/NotificationService/findings.md b/code-reviews/NotificationService/findings.md index 39b697f..948184b 100644 --- a/code-reviews/NotificationService/findings.md +++ b/code-reviews/NotificationService/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 8 | +| Open findings | 3 | ## Summary @@ -172,7 +172,7 @@ the resulting client is disposed. |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:18`, `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:123` | **Description** @@ -185,7 +185,16 @@ Pass the `TlsMode` string (or a `TlsMode` enum) through to the wrapper and map e **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed against source: the +`bool useTls` parameter cannot represent three states, and the non-StartTLS branch used +`SecureSocketOptions.Auto`. A new `SmtpTlsMode` enum (`None`/`StartTls`/`Ssl`) and +`SmtpTlsModeParser` were added; `ISmtpClientWrapper.ConnectAsync` now takes `SmtpTlsMode` +and `MailKitSmtpClientWrapper` maps it explicitly to `SecureSocketOptions.None`/ +`StartTls`/`SslOnConnect`. `SendAsync`/`DeliverBufferedAsync` validate the configured +`TlsMode` up front — an unknown value returns a clean `NotificationResult` failure (or +parks a buffered message) instead of silently negotiating TLS. Regression tests: +`Send_TlsModeNone_DoesNotNegotiateTls`, `Send_TlsModeSsl_UsesImplicitSsl`, +`Send_UnknownTlsMode_ReturnsErrorNotSilentFallback`, and the `SmtpTlsModeParserTests` set. ### NotificationService-006 — OAuth2 token cache is keyed to nothing; wrong token returned when multiple SMTP configs exist @@ -193,7 +202,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Concurrency & thread safety | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/OAuth2TokenService.cs:14-15`, `src/ScadaLink.NotificationService/OAuth2TokenService.cs:30-35` | **Description** @@ -206,7 +215,14 @@ Key the cache by the credential identity (e.g. a dictionary keyed by `tenantId:c **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed: the singleton held a single +`_cachedToken`/`_tokenExpiry` pair and `GetTokenAsync` ignored the `credentials` argument +when validating the cache, so a second SMTP config got the first config's token. +`OAuth2TokenService` now stores a `ConcurrentDictionary` keyed by the +SHA-256 hash of the credential string; each distinct tenant/client/secret gets its own +cached token, expiry, and per-credential `SemaphoreSlim` (double-checked locking +preserved). Regression tests: `GetTokenAsync_DifferentCredentials_ReturnPerCredentialTokens` +and `GetTokenAsync_SameCredentials_CachedPerCredential`. ### NotificationService-007 — Connection timeout and max-concurrent-connections from the design doc are not implemented @@ -214,7 +230,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Design-document adherence | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/NotificationOptions.cs:11-14`, `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:16-20`, `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:111-140` | **Description** @@ -227,7 +243,17 @@ Set `SmtpClient.Timeout` from `ConnectionTimeoutSeconds` in `ConnectAsync` (and/ **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed: `ConnectAsync` never set +`SmtpClient.Timeout` and no throttle gated `DeliverAsync`. `ISmtpClientWrapper.ConnectAsync` +now takes a `connectionTimeoutSeconds` argument; `MailKitSmtpClientWrapper` sets +`SmtpClient.Timeout` from `SmtpConfiguration.ConnectionTimeoutSeconds`. `DeliverAsync` +acquires a lazily-created `SemaphoreSlim` sized to `SmtpConfiguration.MaxConcurrentConnections` +(default 5 when non-positive) and releases it in a `finally`, so concurrent SMTP +deliveries per site are bounded. The timeout is sourced from the deployed +`SmtpConfiguration` rather than `NotificationOptions`; the `NotificationOptions` fields +remain as operational fallback defaults. Regression tests: +`Send_PassesConfiguredConnectionTimeoutToClient` and +`Send_MaxConcurrentConnections_LimitsConcurrentDeliveries`. ### NotificationService-008 — Recipient email addresses are not validated before send @@ -235,7 +261,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:136-137`, `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:50-53` | **Description** @@ -248,15 +274,24 @@ Validate addresses up front (e.g. `MailboxAddress.TryParse`) and return a `Notif **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed: `MailboxAddress.Parse` of a +malformed `FromAddress`/recipient threw `ParseException`, which is unclassified and +escaped `SendAsync` as an unhandled exception. A new `ValidateAddresses` helper uses +`MailboxAddress.TryParse` for the sender and every recipient; `SendAsync` now returns a +clean `NotificationResult(false, ...)` listing the invalid address(es) before any SMTP +attempt, and `DeliverBufferedAsync` parks a buffered message with a bad address (a fault +retrying cannot fix). Regression tests: +`Send_MalformedRecipientAddress_ReturnsCleanError_DoesNotThrow` and +`Send_MalformedFromAddress_ReturnsCleanError_DoesNotThrow`. Definition-time validation in +the Central UI is a separate component and out of this module's scope. ### NotificationService-009 — Credentials handled as plaintext strings; OAuth2 client secret logged risk | | | |--|--| -| Severity | Medium | +| Severity | Medium — re-triaged: split into an in-scope log-leak fix (resolved) and a Commons-scoped at-rest-encryption / structured-credential follow-up (NotificationService-013, Deferred). | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:127-134`, `src/ScadaLink.NotificationService/OAuth2TokenService.cs:30-65`, `src/ScadaLink.Commons/Entities/Notifications/SmtpConfiguration.cs:9` | **Description** @@ -269,7 +304,55 @@ Store credentials encrypted at rest (DPAPI/Data Protection or a secret store) an **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause re-triaged against source: the finding +conflates two concerns with different ownership. + +1. **Log-leak risk (in scope — fixed).** The original code logged whole exception objects + (`_logger.LogWarning(ex, ...)` / `LogError(ex, ...)`); MailKit auth exceptions can echo + server responses quoting the supplied credentials. A new internal `CredentialRedactor` + masks every colon-delimited credential component out of any text. `SendAsync` and + `DeliverBufferedAsync` now log a scrubbed message string (not the raw exception) and the + permanent-failure `NotificationResult` is scrubbed before it returns to the caller. + `OAuth2TokenService` logs the tenant id only — never the client secret or access token. + Regression tests: `CredentialRedactorTests` and + `Send_PermanentError_RedactsCredentialFromResultMessage`. +2. **At-rest encryption + structured-credential modelling (out of scope — Deferred).** + Encrypting `SmtpConfiguration.Credentials` at rest and replacing the brittle + colon-packed `string` with structured fields requires editing + `src/ScadaLink.Commons/Entities/Notifications/SmtpConfiguration.cs` and the + ConfigurationDatabase EF layer — both outside this module. Tracked separately as + **NotificationService-013** (Deferred) so it is not lost. + +### NotificationService-013 — Encrypt SMTP credentials at rest; replace colon-packed string with structured fields + +| | | +|--|--| +| Severity | Medium | +| Category | Security | +| Status | Deferred | +| Location | `src/ScadaLink.Commons/Entities/Notifications/SmtpConfiguration.cs:9`, ConfigurationDatabase EF mapping | + +**Description** + +Split out of NotificationService-009. `SmtpConfiguration.Credentials` packs Basic Auth +`user:pass` and OAuth2 `tenantId:clientId:clientSecret` into a single plaintext +colon-delimited `string`: (a) there is no encryption at rest in SQLite or the central +config DB; (b) a password or client secret containing a `:` is split incorrectly by +`Split(':', 2)` / `Split(':', 3)`, silently corrupting the secret. + +**Recommendation** + +Model credentials as structured fields (or an encrypted blob) on the Commons entity and +encrypt at rest via Data Protection / a secret store. The colon-delimited parsing in +`MailKitSmtpClientWrapper` and `OAuth2TokenService` would then consume the structured +fields directly. + +**Resolution** + +Deferred — requires changes to `src/ScadaLink.Commons` and the ConfigurationDatabase +component, which are outside the NotificationService module. To be addressed in a +Commons/ConfigurationDatabase-scoped change. The associated log-leak risk is resolved +under NotificationService-009. ### NotificationService-010 — `DeliverAsync` does not disconnect the SMTP client on failure diff --git a/src/ScadaLink.NotificationService/CredentialRedactor.cs b/src/ScadaLink.NotificationService/CredentialRedactor.cs new file mode 100644 index 0000000..1cd270d --- /dev/null +++ b/src/ScadaLink.NotificationService/CredentialRedactor.cs @@ -0,0 +1,48 @@ +namespace ScadaLink.NotificationService; + +/// +/// NS-009: Scrubs SMTP credential secrets out of free text (typically exception +/// messages echoed back by an SMTP server) before that text is written to a log. +/// MailKit authentication exceptions can contain server responses that quote the +/// supplied credentials; this prevents a password, client secret, or OAuth2 token +/// from leaking into the operational logs. +/// +internal static class CredentialRedactor +{ + private const string Mask = "***REDACTED***"; + + /// + /// Returns with every secret component of the supplied + /// colon-delimited credential string masked. + /// + /// The text to scrub (e.g. an exception message). + /// + /// The credential string in use — Basic Auth user:pass or OAuth2 + /// tenantId:clientId:clientSecret. May be null. + /// + public static string Scrub(string? text, string? credentials) + { + if (string.IsNullOrEmpty(text) || string.IsNullOrEmpty(credentials)) + { + return text ?? string.Empty; + } + + var result = text; + + // Mask each individual colon-delimited component (covers user, password, + // tenant, clientId, clientSecret) and the whole packed string. Order longest + // first so a component that is a substring of another is still fully masked. + var parts = credentials.Split(':') + .Where(p => p.Length >= 4) + .Append(credentials) + .Distinct() + .OrderByDescending(p => p.Length); + + foreach (var part in parts) + { + result = result.Replace(part, Mask, StringComparison.Ordinal); + } + + return result; + } +} diff --git a/src/ScadaLink.NotificationService/ISmtpClientWrapper.cs b/src/ScadaLink.NotificationService/ISmtpClientWrapper.cs index b945228..82b2595 100644 --- a/src/ScadaLink.NotificationService/ISmtpClientWrapper.cs +++ b/src/ScadaLink.NotificationService/ISmtpClientWrapper.cs @@ -5,7 +5,24 @@ namespace ScadaLink.NotificationService; /// public interface ISmtpClientWrapper { - Task ConnectAsync(string host, int port, bool useTls, CancellationToken cancellationToken = default); + /// + /// Connects to the SMTP server. + /// + /// + /// NS-005: explicit three-state TLS mode (None/StartTls/Ssl) — replaces the old + /// bool useTls which could not represent implicit-SSL and silently fell + /// back to opportunistic negotiation for non-StartTLS configurations. + /// + /// + /// NS-007: SMTP connection/operation timeout in seconds. A non-positive value + /// leaves the client's default timeout in place. + /// + Task ConnectAsync( + string host, + int port, + SmtpTlsMode tlsMode, + int connectionTimeoutSeconds, + CancellationToken cancellationToken = default); Task AuthenticateAsync(string authType, string? credentials, CancellationToken cancellationToken = default); Task SendAsync(string from, IEnumerable bccRecipients, string subject, string body, CancellationToken cancellationToken = default); Task DisconnectAsync(CancellationToken cancellationToken = default); diff --git a/src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs b/src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs index 9823842..cc9fe79 100644 --- a/src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs +++ b/src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs @@ -13,9 +13,33 @@ public class MailKitSmtpClientWrapper : ISmtpClientWrapper, IDisposable { private readonly SmtpClient _client = new(); - public async Task ConnectAsync(string host, int port, bool useTls, CancellationToken cancellationToken = default) + public async Task ConnectAsync( + string host, + int port, + SmtpTlsMode tlsMode, + int connectionTimeoutSeconds, + CancellationToken cancellationToken = default) { - var secureSocket = useTls ? SecureSocketOptions.StartTls : SecureSocketOptions.Auto; + // NS-005: map the explicit three-state TLS mode onto MailKit's socket + // options. The old code collapsed everything to a boolean and used + // SecureSocketOptions.Auto for the non-StartTLS case, which let MailKit + // opportunistically negotiate TLS even when "None" was configured and + // gave SSL-on-connect no representation at all. + var secureSocket = tlsMode switch + { + SmtpTlsMode.None => SecureSocketOptions.None, + SmtpTlsMode.StartTls => SecureSocketOptions.StartTls, + SmtpTlsMode.Ssl => SecureSocketOptions.SslOnConnect, + _ => throw new ArgumentOutOfRangeException(nameof(tlsMode), tlsMode, "Unknown TLS mode."), + }; + + // NS-007: honour the configured connection timeout. SmtpClient.Timeout is + // in milliseconds and applies to connect/auth/send operations. + if (connectionTimeoutSeconds > 0) + { + _client.Timeout = connectionTimeoutSeconds * 1000; + } + await _client.ConnectAsync(host, port, secureSocket, cancellationToken); } diff --git a/src/ScadaLink.NotificationService/NotificationDeliveryService.cs b/src/ScadaLink.NotificationService/NotificationDeliveryService.cs index 68efd2b..a7d36ba 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 MimeKit; using ScadaLink.Commons.Entities.Notifications; using ScadaLink.Commons.Interfaces.Repositories; using ScadaLink.Commons.Interfaces.Services; @@ -68,6 +69,30 @@ public class NotificationDeliveryService : INotificationDeliveryService return new NotificationResult(false, "No SMTP configuration available"); } + // NS-005: validate the configured TLS mode up front — an unknown value is a + // configuration error and must surface as a clean result, not a silent + // fallback to opportunistic TLS negotiation. + try + { + SmtpTlsModeParser.Parse(smtpConfig.TlsMode); + } + catch (ArgumentException ex) + { + _logger.LogError("Invalid SMTP TLS mode for list {List}: {Reason}", listName, ex.Message); + return new NotificationResult(false, ex.Message); + } + + // NS-008: validate every email address before attempting delivery. A single + // malformed address previously caused MailboxAddress.Parse to throw a + // ParseException that escaped SendAsync unhandled; it must instead produce a + // clean NotificationResult the calling script can handle. + var addressError = ValidateAddresses(smtpConfig.FromAddress, recipients); + if (addressError != null) + { + _logger.LogWarning("Notification to list {List} has invalid addresses: {Reason}", listName, addressError); + return new NotificationResult(false, addressError); + } + try { await DeliverAsync(smtpConfig, recipients, subject, message, cancellationToken); @@ -75,9 +100,13 @@ public class NotificationDeliveryService : INotificationDeliveryService } catch (SmtpPermanentException ex) { - // WP-12: Permanent SMTP failure — returned to script - _logger.LogError(ex, "Permanent SMTP failure sending to list {List}", listName); - return new NotificationResult(false, $"Permanent SMTP error: {ex.Message}"); + // WP-12: Permanent SMTP failure — returned to script. + // NS-009: scrub credential fragments out of the server-supplied message + // before logging or returning it. + var detail = CredentialRedactor.Scrub(ex.Message, smtpConfig.Credentials); + _logger.LogError( + "Permanent SMTP failure sending to list {List}: {Detail}", listName, detail); + return new NotificationResult(false, $"Permanent SMTP error: {detail}"); } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { @@ -86,8 +115,11 @@ public class NotificationDeliveryService : INotificationDeliveryService } catch (Exception ex) when (IsTransientSmtpError(ex, cancellationToken)) { - // WP-12: Transient SMTP failure — hand to S&F - _logger.LogWarning(ex, "Transient SMTP failure sending to list {List}, buffering for retry", listName); + // WP-12: Transient SMTP failure — hand to S&F. + // NS-009: scrub credential fragments before logging. + _logger.LogWarning( + "Transient SMTP failure sending to list {List} ({ExceptionType}): {Detail}; buffering for retry", + listName, ex.GetType().Name, CredentialRedactor.Scrub(ex.Message, smtpConfig.Credentials)); if (_storeAndForward == null) { @@ -155,6 +187,30 @@ public class NotificationDeliveryService : INotificationDeliveryService return false; } + // NS-005: an unknown TLS mode is a configuration error that retrying cannot + // fix — park the buffered message rather than throwing on every sweep. + try + { + SmtpTlsModeParser.Parse(smtpConfig.TlsMode); + } + catch (ArgumentException ex) + { + _logger.LogError( + "Buffered notification to list '{List}' cannot be delivered — {Reason}; parking.", + payload.ListName, ex.Message); + return false; + } + + // NS-008: a malformed address cannot be fixed by retrying — park it. + var addressError = ValidateAddresses(smtpConfig.FromAddress, recipients); + if (addressError != null) + { + _logger.LogError( + "Buffered notification to list '{List}' has invalid addresses ({Reason}); parking.", + payload.ListName, addressError); + return false; + } + try { await DeliverAsync(smtpConfig, recipients, payload.Subject, payload.Message, cancellationToken); @@ -162,7 +218,10 @@ public class NotificationDeliveryService : INotificationDeliveryService } catch (SmtpPermanentException ex) { - _logger.LogError(ex, "Buffered notification to list '{List}' failed permanently; parking.", payload.ListName); + // NS-009: scrub credential fragments out of the message before logging. + _logger.LogError( + "Buffered notification to list '{List}' failed permanently ({Detail}); parking.", + payload.ListName, CredentialRedactor.Scrub(ex.Message, smtpConfig.Credentials)); return false; } // Transient SMTP errors propagate out of DeliverAsync — the S&F engine retries. @@ -171,7 +230,55 @@ public class NotificationDeliveryService : INotificationDeliveryService private sealed record BufferedNotification(string ListName, string Subject, string Message); /// - /// Delivers an email via SMTP. Throws on failure. + /// 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). + /// + private SemaphoreSlim? _concurrencyLimiter; + private readonly object _limiterLock = new(); + + private SemaphoreSlim GetConcurrencyLimiter(SmtpConfiguration config) + { + if (_concurrencyLimiter != null) + { + return _concurrencyLimiter; + } + + 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; + } + } + + /// + /// NS-008: Validates the sender and recipient email addresses, returning a + /// human-readable error string if any is malformed, or null if all parse. + /// + internal static string? ValidateAddresses( + string fromAddress, IReadOnlyList recipients) + { + if (!MailboxAddress.TryParse(fromAddress, out _)) + { + return $"Invalid sender (from) email address: '{fromAddress}'"; + } + + var invalid = recipients + .Where(r => !MailboxAddress.TryParse(r.EmailAddress, out _)) + .Select(r => r.EmailAddress) + .ToList(); + + return invalid.Count > 0 + ? $"Invalid recipient email address(es): {string.Join(", ", invalid)}" + : null; + } + + /// + /// Delivers an email via SMTP. Throws on failure (transient errors and + /// propagate; the caller classifies them). /// internal async Task DeliverAsync( SmtpConfiguration config, @@ -180,16 +287,23 @@ public class NotificationDeliveryService : INotificationDeliveryService string body, CancellationToken cancellationToken) { + var tlsMode = SmtpTlsModeParser.Parse(config.TlsMode); + + // NS-007: bound the number of concurrent SMTP connections per site. + var limiter = GetConcurrencyLimiter(config); + await limiter.WaitAsync(cancellationToken); + // NS-004: create exactly one client and dispose the one actually used. var smtp = _smtpClientFactory(); using var disposable = smtp as IDisposable; try { - var useTls = config.TlsMode?.Equals("starttls", StringComparison.OrdinalIgnoreCase) == true; - await smtp.ConnectAsync(config.Host, config.Port, useTls, cancellationToken); + // NS-005/NS-007: explicit TLS mode and the configured connection timeout. + await smtp.ConnectAsync( + config.Host, config.Port, tlsMode, config.ConnectionTimeoutSeconds, cancellationToken); - // Resolve credentials (OAuth2 token refresh if needed) + // Resolve credentials (OAuth2 token fetched/cached by the token service). var credentials = config.Credentials; if (config.AuthType.Equals("oauth2", StringComparison.OrdinalIgnoreCase) && _tokenService != null && credentials != null) { @@ -218,6 +332,11 @@ public class NotificationDeliveryService : INotificationDeliveryService } // Transient and SmtpPermanentException both propagate unchanged: SendAsync's // catch filters (SmtpPermanentException / IsTransientSmtpError) handle them. + finally + { + // NS-007: always release the concurrency slot, even on failure. + limiter.Release(); + } } private enum SmtpErrorClass diff --git a/src/ScadaLink.NotificationService/OAuth2TokenService.cs b/src/ScadaLink.NotificationService/OAuth2TokenService.cs index 3719343..dece6e8 100644 --- a/src/ScadaLink.NotificationService/OAuth2TokenService.cs +++ b/src/ScadaLink.NotificationService/OAuth2TokenService.cs @@ -1,3 +1,6 @@ +using System.Collections.Concurrent; +using System.Security.Cryptography; +using System.Text; using System.Text.Json; using Microsoft.Extensions.Logging; @@ -6,14 +9,18 @@ namespace ScadaLink.NotificationService; /// /// WP-11: OAuth2 Client Credentials token lifecycle — fetch, cache, refresh on expiry. /// Used for Microsoft 365 SMTP authentication. +/// NS-006: tokens are cached per credential identity (tenant/client/secret), so a +/// second SMTP configuration with different credentials never receives the first +/// configuration's token. /// public class OAuth2TokenService { private readonly IHttpClientFactory _httpClientFactory; private readonly ILogger _logger; - private string? _cachedToken; - private DateTimeOffset _tokenExpiry = DateTimeOffset.MinValue; - private readonly SemaphoreSlim _lock = new(1, 1); + + // NS-006: cache keyed by a hash of the credential string. Each distinct + // tenant/client/secret triple gets its own cached token and its own lock. + private readonly ConcurrentDictionary _cache = new(); public OAuth2TokenService( IHttpClientFactory httpClientFactory, @@ -29,18 +36,21 @@ public class OAuth2TokenService /// public async Task GetTokenAsync(string credentials, CancellationToken cancellationToken = default) { - if (_cachedToken != null && DateTimeOffset.UtcNow < _tokenExpiry) + var key = CredentialKey(credentials); + var entry = _cache.GetOrAdd(key, _ => new CacheEntry()); + + if (entry.Token != null && DateTimeOffset.UtcNow < entry.Expiry) { - return _cachedToken; + return entry.Token; } - await _lock.WaitAsync(cancellationToken); + await entry.Lock.WaitAsync(cancellationToken); try { - // Double-check after acquiring lock - if (_cachedToken != null && DateTimeOffset.UtcNow < _tokenExpiry) + // Double-check after acquiring the per-credential lock. + if (entry.Token != null && DateTimeOffset.UtcNow < entry.Expiry) { - return _cachedToken; + return entry.Token; } var parts = credentials.Split(':', 3); @@ -70,18 +80,39 @@ public class OAuth2TokenService var json = await response.Content.ReadAsStringAsync(cancellationToken); using var doc = JsonDocument.Parse(json); - _cachedToken = doc.RootElement.GetProperty("access_token").GetString() + var token = doc.RootElement.GetProperty("access_token").GetString() ?? throw new InvalidOperationException("No access_token in OAuth2 response"); var expiresIn = doc.RootElement.GetProperty("expires_in").GetInt32(); - _tokenExpiry = DateTimeOffset.UtcNow.AddSeconds(expiresIn - 60); // Refresh 60s before expiry + entry.Token = token; + entry.Expiry = DateTimeOffset.UtcNow.AddSeconds(expiresIn - 60); // Refresh 60s before expiry - _logger.LogInformation("OAuth2 token refreshed, expires in {ExpiresIn}s", expiresIn); - return _cachedToken; + // NS-009: the token endpoint identity is logged by tenant only — never + // the client secret or the access token itself. + _logger.LogInformation( + "OAuth2 token refreshed for tenant {Tenant}, expires in {ExpiresIn}s", tenantId, expiresIn); + return token; } finally { - _lock.Release(); + entry.Lock.Release(); } } + + /// + /// NS-006: a stable, non-reversible key for the credential string so the cache + /// is partitioned by credential identity without holding the secret as a key. + /// + private static string CredentialKey(string credentials) + { + var hash = SHA256.HashData(Encoding.UTF8.GetBytes(credentials)); + return Convert.ToHexString(hash); + } + + private sealed class CacheEntry + { + public string? Token; + public DateTimeOffset Expiry = DateTimeOffset.MinValue; + public readonly SemaphoreSlim Lock = new(1, 1); + } } diff --git a/src/ScadaLink.NotificationService/SmtpTlsMode.cs b/src/ScadaLink.NotificationService/SmtpTlsMode.cs new file mode 100644 index 0000000..99c9c67 --- /dev/null +++ b/src/ScadaLink.NotificationService/SmtpTlsMode.cs @@ -0,0 +1,49 @@ +namespace ScadaLink.NotificationService; + +/// +/// NS-005: The three TLS modes the design doc defines for SMTP connections. +/// A single boolean cannot represent the requirement, so the configured +/// SmtpConfiguration.TlsMode string is parsed into this three-state enum. +/// +public enum SmtpTlsMode +{ + /// No transport security — plain SMTP. Maps to SecureSocketOptions.None. + None, + + /// Opportunistic STARTTLS upgrade (typically port 587). Maps to SecureSocketOptions.StartTls. + StartTls, + + /// Implicit TLS / SSL-on-connect (typically port 465). Maps to SecureSocketOptions.SslOnConnect. + Ssl, +} + +/// +/// NS-005: Parses the free-text SmtpConfiguration.TlsMode value into a +/// , rejecting unknown values rather than silently +/// falling back to opportunistic negotiation. +/// +public static class SmtpTlsModeParser +{ + /// + /// Parses a configured TLS mode string. A null or empty value defaults to + /// (the design-doc default for port 587). + /// + /// The value is not one of None/StartTLS/SSL. + public static SmtpTlsMode Parse(string? tlsMode) + { + if (string.IsNullOrWhiteSpace(tlsMode)) + { + return SmtpTlsMode.StartTls; + } + + return tlsMode.Trim().ToLowerInvariant() switch + { + "none" => SmtpTlsMode.None, + "starttls" => SmtpTlsMode.StartTls, + "ssl" => SmtpTlsMode.Ssl, + _ => throw new ArgumentException( + $"Unknown SMTP TLS mode '{tlsMode}'. Expected one of: None, StartTLS, SSL.", + nameof(tlsMode)), + }; + } +} diff --git a/tests/ScadaLink.NotificationService.Tests/CredentialRedactorTests.cs b/tests/ScadaLink.NotificationService.Tests/CredentialRedactorTests.cs new file mode 100644 index 0000000..454a26b --- /dev/null +++ b/tests/ScadaLink.NotificationService.Tests/CredentialRedactorTests.cs @@ -0,0 +1,38 @@ +namespace ScadaLink.NotificationService.Tests; + +/// +/// NS-009: Tests for scrubbing SMTP credential secrets out of log/result text. +/// +public class CredentialRedactorTests +{ + [Fact] + public void Scrub_BasicAuthPassword_IsMasked() + { + var text = "535 5.7.8 Authentication failed for user 'svc' with password 'Hunter2pw!'"; + var result = CredentialRedactor.Scrub(text, "svc:Hunter2pw!"); + + Assert.DoesNotContain("Hunter2pw!", result); + Assert.DoesNotContain("svc:Hunter2pw!", result); + } + + [Fact] + public void Scrub_OAuth2ClientSecret_IsMasked() + { + var text = "Token request failed: client_secret=Sup3rSecretValue rejected by tenant"; + var result = CredentialRedactor.Scrub(text, "tenant-guid:client-guid:Sup3rSecretValue"); + + Assert.DoesNotContain("Sup3rSecretValue", result); + } + + [Fact] + public void Scrub_NullCredentials_ReturnsTextUnchanged() + { + Assert.Equal("plain text", CredentialRedactor.Scrub("plain text", null)); + } + + [Fact] + public void Scrub_NullText_ReturnsEmpty() + { + Assert.Equal(string.Empty, CredentialRedactor.Scrub(null, "user:pass")); + } +} diff --git a/tests/ScadaLink.NotificationService.Tests/NotificationDeliveryServiceTests.cs b/tests/ScadaLink.NotificationService.Tests/NotificationDeliveryServiceTests.cs index 063680e..e3fa252 100644 --- a/tests/ScadaLink.NotificationService.Tests/NotificationDeliveryServiceTests.cs +++ b/tests/ScadaLink.NotificationService.Tests/NotificationDeliveryServiceTests.cs @@ -111,7 +111,8 @@ public class NotificationDeliveryServiceTests await service.SendAsync("ops-team", "Alert", "Body"); - await _smtpClient.Received().ConnectAsync("smtp.example.com", 587, true, Arg.Any()); + await _smtpClient.Received().ConnectAsync( + "smtp.example.com", 587, SmtpTlsMode.StartTls, Arg.Any(), Arg.Any()); await _smtpClient.Received().AuthenticateAsync("basic", "user:pass", Arg.Any()); await _smtpClient.Received().SendAsync( "noreply@example.com", @@ -363,7 +364,7 @@ public class NotificationDeliveryServiceTests private sealed class TrackingSmtpClient : ISmtpClientWrapper, IDisposable { public bool Disposed { get; private set; } - public Task ConnectAsync(string host, int port, bool useTls, CancellationToken cancellationToken = default) + public Task ConnectAsync(string host, int port, SmtpTlsMode tlsMode, int connectionTimeoutSeconds, CancellationToken cancellationToken = default) => Task.CompletedTask; public Task AuthenticateAsync(string authType, string? credentials, CancellationToken cancellationToken = default) => Task.CompletedTask; @@ -405,4 +406,240 @@ public class NotificationDeliveryServiceTests return new StoreAndForwardService( storage, new StoreAndForwardOptions(), NullLogger.Instance); } + + // ── NotificationService-005: explicit TLS mode passed through to the wrapper ── + + /// An SMTP wrapper that records the TLS mode and timeout it was connected with. + private sealed class RecordingTlsClient : ISmtpClientWrapper + { + public SmtpTlsMode? TlsMode { get; private set; } + public int ConnectionTimeoutSeconds { get; private set; } + public Task ConnectAsync(string host, int port, SmtpTlsMode tlsMode, int connectionTimeoutSeconds, CancellationToken cancellationToken = default) + { + TlsMode = tlsMode; + ConnectionTimeoutSeconds = connectionTimeoutSeconds; + return Task.CompletedTask; + } + public Task AuthenticateAsync(string authType, string? credentials, CancellationToken cancellationToken = default) + => Task.CompletedTask; + public Task SendAsync(string from, IEnumerable bccRecipients, string subject, string body, CancellationToken cancellationToken = default) + => Task.CompletedTask; + public Task DisconnectAsync(CancellationToken cancellationToken = default) => Task.CompletedTask; + } + + private void SetupHappyPathWithSmtp(SmtpConfiguration smtpConfig) + { + var list = new NotificationList("ops-team") { Id = 1 }; + var recipients = new List + { + new("Alice", "alice@example.com") { Id = 1, NotificationListId = 1 } + }; + _repository.GetListByNameAsync("ops-team").Returns(list); + _repository.GetRecipientsByListIdAsync(1).Returns(recipients); + _repository.GetAllSmtpConfigurationsAsync().Returns(new List { smtpConfig }); + } + + [Fact] + public async Task Send_TlsModeNone_DoesNotNegotiateTls() + { + // NS-005: TlsMode "none" must connect with SmtpTlsMode.None, not the old + // SecureSocketOptions.Auto (which let MailKit opportunistically negotiate TLS). + var cfg = new SmtpConfiguration("smtp.example.com", "basic", "noreply@example.com") + { + Id = 1, Port = 25, Credentials = "user:pass", TlsMode = "none" + }; + SetupHappyPathWithSmtp(cfg); + var recording = new RecordingTlsClient(); + var service = new NotificationDeliveryService( + _repository, () => recording, NullLogger.Instance); + + var result = await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.True(result.Success); + Assert.Equal(SmtpTlsMode.None, recording.TlsMode); + } + + [Fact] + public async Task Send_TlsModeSsl_UsesImplicitSsl() + { + // NS-005: TlsMode "ssl" (port 465 implicit TLS) must be honoured, not + // collapsed into the same path as "none". + var cfg = new SmtpConfiguration("smtp.example.com", "basic", "noreply@example.com") + { + Id = 1, Port = 465, Credentials = "user:pass", TlsMode = "ssl" + }; + SetupHappyPathWithSmtp(cfg); + var recording = new RecordingTlsClient(); + var service = new NotificationDeliveryService( + _repository, () => recording, NullLogger.Instance); + + var result = await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.True(result.Success); + Assert.Equal(SmtpTlsMode.Ssl, recording.TlsMode); + } + + [Fact] + public async Task Send_UnknownTlsMode_ReturnsErrorNotSilentFallback() + { + var cfg = new SmtpConfiguration("smtp.example.com", "basic", "noreply@example.com") + { + Id = 1, Port = 587, Credentials = "user:pass", TlsMode = "bogus" + }; + SetupHappyPathWithSmtp(cfg); + var service = new NotificationDeliveryService( + _repository, () => new RecordingTlsClient(), NullLogger.Instance); + + var result = await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.False(result.Success); + Assert.Contains("TLS mode", result.ErrorMessage); + } + + // ── NotificationService-007: connection timeout passed through to the wrapper ── + + [Fact] + public async Task Send_PassesConfiguredConnectionTimeoutToClient() + { + // NS-007: SmtpConfiguration.ConnectionTimeoutSeconds must reach the wrapper + // so SmtpClient.Timeout is set; it was previously dead configuration. + var cfg = new SmtpConfiguration("smtp.example.com", "basic", "noreply@example.com") + { + Id = 1, Port = 587, Credentials = "user:pass", TlsMode = "starttls", + ConnectionTimeoutSeconds = 17 + }; + SetupHappyPathWithSmtp(cfg); + var recording = new RecordingTlsClient(); + var service = new NotificationDeliveryService( + _repository, () => recording, NullLogger.Instance); + + await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.Equal(17, recording.ConnectionTimeoutSeconds); + } + + [Fact] + public async Task Send_MaxConcurrentConnections_LimitsConcurrentDeliveries() + { + // NS-007: MaxConcurrentConnections must throttle concurrent SMTP deliveries. + var cfg = new SmtpConfiguration("smtp.example.com", "basic", "noreply@example.com") + { + Id = 1, Port = 587, Credentials = "user:pass", TlsMode = "starttls", + MaxConcurrentConnections = 2 + }; + SetupHappyPathWithSmtp(cfg); + + var inFlight = 0; + var maxObserved = 0; + var gate = new SemaphoreSlim(0); + var sync = new object(); + + var service = new NotificationDeliveryService( + _repository, + () => new BlockingSmtpClient( + onSend: async () => + { + lock (sync) + { + inFlight++; + if (inFlight > maxObserved) maxObserved = inFlight; + } + await gate.WaitAsync(); + lock (sync) { inFlight--; } + }), + NullLogger.Instance); + + var sends = Enumerable.Range(0, 6) + .Select(_ => service.SendAsync("ops-team", "Alert", "Body")) + .ToList(); + + // Give the throttled sends time to reach the SMTP send call. + await Task.Delay(200); + gate.Release(6); + await Task.WhenAll(sends); + + Assert.True(maxObserved <= 2, $"Expected at most 2 concurrent deliveries, observed {maxObserved}"); + } + + private sealed class BlockingSmtpClient : ISmtpClientWrapper, IDisposable + { + private readonly Func _onSend; + public BlockingSmtpClient(Func onSend) => _onSend = onSend; + public Task ConnectAsync(string host, int port, SmtpTlsMode tlsMode, int connectionTimeoutSeconds, CancellationToken cancellationToken = default) + => Task.CompletedTask; + public Task AuthenticateAsync(string authType, string? credentials, CancellationToken cancellationToken = default) + => Task.CompletedTask; + public Task SendAsync(string from, IEnumerable bccRecipients, string subject, string body, CancellationToken cancellationToken = default) + => _onSend(); + public Task DisconnectAsync(CancellationToken cancellationToken = default) => Task.CompletedTask; + public void Dispose() { } + } + + // ── NotificationService-008: recipient address validation ── + + [Fact] + public async Task Send_MalformedRecipientAddress_ReturnsCleanError_DoesNotThrow() + { + // NS-008: a malformed recipient address previously caused MailboxAddress.Parse + // to throw ParseException, which escaped SendAsync unhandled. It must now + // produce a clean NotificationResult failure. + var list = new NotificationList("ops-team") { Id = 1 }; + var recipients = new List + { + new("Alice", "alice@example.com") { Id = 1, NotificationListId = 1 }, + new("Bad", "not a valid address @@") { Id = 2, NotificationListId = 1 } + }; + var cfg = new SmtpConfiguration("smtp.example.com", "basic", "noreply@example.com") + { + Id = 1, Port = 587, Credentials = "user:pass", TlsMode = "starttls" + }; + _repository.GetListByNameAsync("ops-team").Returns(list); + _repository.GetRecipientsByListIdAsync(1).Returns(recipients); + _repository.GetAllSmtpConfigurationsAsync().Returns(new List { cfg }); + + var service = CreateService(); + var result = await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.False(result.Success); + Assert.Contains("address", result.ErrorMessage, StringComparison.OrdinalIgnoreCase); + Assert.Contains("not a valid address @@", result.ErrorMessage); + } + + // ── NotificationService-009: credential secrets scrubbed from logs/results ── + + [Fact] + public async Task Send_PermanentError_RedactsCredentialFromResultMessage() + { + // NS-009: a permanent-failure message echoing a credential fragment must be + // scrubbed before it reaches the caller-facing NotificationResult. + 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 SmtpPermanentException("550 rejected — password Hunter2Secret is invalid")); + + var service = CreateService(); + var result = await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.False(result.Success); + Assert.DoesNotContain("Hunter2Secret", result.ErrorMessage); + } + + [Fact] + public async Task Send_MalformedFromAddress_ReturnsCleanError_DoesNotThrow() + { + var cfg = new SmtpConfiguration("smtp.example.com", "basic", "@@bad-from@@") + { + Id = 1, Port = 587, Credentials = "user:pass", TlsMode = "starttls" + }; + SetupHappyPathWithSmtp(cfg); + + var service = CreateService(); + var result = await service.SendAsync("ops-team", "Alert", "Body"); + + Assert.False(result.Success); + Assert.Contains("address", result.ErrorMessage, StringComparison.OrdinalIgnoreCase); + } } diff --git a/tests/ScadaLink.NotificationService.Tests/OAuth2TokenServiceTests.cs b/tests/ScadaLink.NotificationService.Tests/OAuth2TokenServiceTests.cs index 159689f..5b07953 100644 --- a/tests/ScadaLink.NotificationService.Tests/OAuth2TokenServiceTests.cs +++ b/tests/ScadaLink.NotificationService.Tests/OAuth2TokenServiceTests.cs @@ -87,6 +87,70 @@ public class OAuth2TokenServiceTests () => service.GetTokenAsync("tenant:client:secret")); } + // ── NotificationService-006: token cache must be keyed to credential identity ── + + [Fact] + public async Task GetTokenAsync_DifferentCredentials_ReturnPerCredentialTokens() + { + // NS-006: the singleton cached a single token ignoring the credentials + // argument, so a second SMTP config with a different tenant/client got the + // first config's token. Each distinct credential must get its own token. + var handler = new PerTenantHttpMessageHandler(); + var client = new HttpClient(handler); + var factory = CreateMockFactory(client); + var service = new OAuth2TokenService(factory, NullLogger.Instance); + + var tokenA = await service.GetTokenAsync("tenantA:clientA:secretA"); + var tokenB = await service.GetTokenAsync("tenantB:clientB:secretB"); + + Assert.Equal("token-for-tenantA", tokenA); + Assert.Equal("token-for-tenantB", tokenB); + } + + [Fact] + public async Task GetTokenAsync_SameCredentials_CachedPerCredential() + { + // NS-006: caching still works — repeated calls with the same credential + // identity make exactly one HTTP call. + var handler = new PerTenantHttpMessageHandler(); + var client = new HttpClient(handler); + var factory = CreateMockFactory(client); + var service = new OAuth2TokenService(factory, NullLogger.Instance); + + await service.GetTokenAsync("tenantA:clientA:secretA"); + await service.GetTokenAsync("tenantA:clientA:secretA"); + await service.GetTokenAsync("tenantB:clientB:secretB"); + + Assert.Equal(2, handler.CallCount); // one per distinct credential, not per call + } + + /// + /// HTTP handler that returns a distinct access token per tenant id, parsed from + /// the request URL (https://login.microsoftonline.com/{tenantId}/...). + /// + private class PerTenantHttpMessageHandler : HttpMessageHandler + { + public int CallCount { get; private set; } + + protected override Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + CallCount++; + var segments = request.RequestUri!.AbsolutePath.Trim('/').Split('/'); + var tenantId = segments[0]; + var json = JsonSerializer.Serialize(new + { + access_token = $"token-for-{tenantId}", + expires_in = 3600, + token_type = "Bearer" + }); + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(json) + }); + } + } + /// /// Simple mock HTTP handler that returns a fixed response. /// diff --git a/tests/ScadaLink.NotificationService.Tests/SmtpTlsModeParserTests.cs b/tests/ScadaLink.NotificationService.Tests/SmtpTlsModeParserTests.cs new file mode 100644 index 0000000..e0d023b --- /dev/null +++ b/tests/ScadaLink.NotificationService.Tests/SmtpTlsModeParserTests.cs @@ -0,0 +1,40 @@ +namespace ScadaLink.NotificationService.Tests; + +/// +/// NS-005: Tests for parsing the configured SMTP TLS mode into the three-state enum. +/// +public class SmtpTlsModeParserTests +{ + [Theory] + [InlineData("none", SmtpTlsMode.None)] + [InlineData("None", SmtpTlsMode.None)] + [InlineData("NONE", SmtpTlsMode.None)] + [InlineData("starttls", SmtpTlsMode.StartTls)] + [InlineData("StartTLS", SmtpTlsMode.StartTls)] + [InlineData("ssl", SmtpTlsMode.Ssl)] + [InlineData("SSL", SmtpTlsMode.Ssl)] + [InlineData(" starttls ", SmtpTlsMode.StartTls)] + public void Parse_KnownModes_ReturnsExpected(string input, SmtpTlsMode expected) + { + Assert.Equal(expected, SmtpTlsModeParser.Parse(input)); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void Parse_NullOrEmpty_DefaultsToStartTls(string? input) + { + Assert.Equal(SmtpTlsMode.StartTls, SmtpTlsModeParser.Parse(input)); + } + + [Theory] + [InlineData("auto")] + [InlineData("tls")] + [InlineData("implicit")] + public void Parse_UnknownMode_Throws(string input) + { + // NS-005: an unknown mode must be rejected, not silently treated as Auto. + Assert.Throws(() => SmtpTlsModeParser.Parse(input)); + } +}