diff --git a/src/ScadaLink.NotificationOutbox/Delivery/EmailNotificationDeliveryAdapter.cs b/src/ScadaLink.NotificationOutbox/Delivery/EmailNotificationDeliveryAdapter.cs index 6158e6e..8928530 100644 --- a/src/ScadaLink.NotificationOutbox/Delivery/EmailNotificationDeliveryAdapter.cs +++ b/src/ScadaLink.NotificationOutbox/Delivery/EmailNotificationDeliveryAdapter.cs @@ -1,9 +1,5 @@ -using System.Net.Sockets; -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; using ScadaLink.Commons.Types.Enums; @@ -95,7 +91,8 @@ public sealed class EmailNotificationDeliveryAdapter : INotificationDeliveryAdap // A malformed sender or recipient address cannot be fixed by retrying — // surface it as a permanent failure (mirrors NS-008). - var addressError = ValidateAddresses(smtpConfig.FromAddress, recipients); + var addressError = NotificationDeliveryService.ValidateAddresses( + smtpConfig.FromAddress, recipients); if (addressError != null) { _logger.LogWarning( @@ -116,7 +113,7 @@ public sealed class EmailNotificationDeliveryAdapter : INotificationDeliveryAdap catch (SmtpPermanentException ex) { // Permanent SMTP failure (5xx) — not retried. - var detail = ScrubCredentials(ex.Message, smtpConfig.Credentials); + var detail = CredentialRedactor.Scrub(ex.Message, smtpConfig.Credentials); _logger.LogError( "Permanent SMTP failure delivering email to list '{List}': {Detail}", notification.ListName, detail); @@ -128,10 +125,10 @@ public sealed class EmailNotificationDeliveryAdapter : INotificationDeliveryAdap // nor a delivery failure. throw; } - catch (Exception ex) when (IsTransientSmtpError(ex, cancellationToken)) + catch (Exception ex) when (SmtpErrorClassifier.IsTransient(ex, cancellationToken)) { // Transient SMTP failure (4xx, socket/protocol/timeout) — eligible for retry. - var detail = ScrubCredentials(ex.Message, smtpConfig.Credentials); + var detail = CredentialRedactor.Scrub(ex.Message, smtpConfig.Credentials); _logger.LogWarning( "Transient SMTP failure delivering email to list '{List}' ({ExceptionType}): {Detail}", notification.ListName, ex.GetType().Name, detail); @@ -142,7 +139,7 @@ public sealed class EmailNotificationDeliveryAdapter : INotificationDeliveryAdap // An unclassified failure — chiefly an OAuth2 token-fetch failure. The // outbox treats it as permanent: retrying a broken credential burns // token-endpoint calls. (Mirrors the NS-015 default-to-permanent stance.) - var detail = ScrubCredentials(ex.Message, smtpConfig.Credentials); + var detail = CredentialRedactor.Scrub(ex.Message, smtpConfig.Credentials); _logger.LogError( "Unclassified failure delivering email to list '{List}' ({ExceptionType}): {Detail}", notification.ListName, ex.GetType().Name, detail); @@ -193,7 +190,7 @@ public sealed class EmailNotificationDeliveryAdapter : INotificationDeliveryAdap // A deliberate cancellation must propagate, not be misclassified as transient. throw; } - catch (Exception ex) when (ClassifySmtpError(ex, cancellationToken) == SmtpErrorClass.Permanent + catch (Exception ex) when (SmtpErrorClassifier.Classify(ex, cancellationToken) == SmtpErrorClass.Permanent && ex is not SmtpPermanentException) { // Permanent SMTP failure (5xx) — surface a typed permanent exception. @@ -218,111 +215,4 @@ public sealed class EmailNotificationDeliveryAdapter : INotificationDeliveryAdap } } - /// - /// Validates the sender and recipient email addresses, returning a human-readable - /// error string if any is malformed, or null if all parse (mirrors NS-008). - /// - private 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; - } - - private enum SmtpErrorClass - { - /// Cancellation or an unrecognised exception — caller decides. - Unknown, - /// Retryable failure (4xx, connection/socket/protocol error, timeout). - Transient, - /// Non-retryable failure (5xx) — must not be retried. - Permanent, - } - - /// - /// Classifies an SMTP failure using MailKit's typed exceptions and the numeric - /// rather than locale-dependent substring matching - /// (mirrors NS-002/NS-003 in NotificationDeliveryService). - /// - private static SmtpErrorClass ClassifySmtpError(Exception ex, CancellationToken cancellationToken) - { - if (ex is OperationCanceledException && cancellationToken.IsCancellationRequested) - { - return SmtpErrorClass.Unknown; - } - - if (ex is SmtpCommandException command) - { - var code = (int)command.StatusCode; - if (code >= 400 && code < 500) - { - return SmtpErrorClass.Transient; - } - - if (code >= 500 && code < 600) - { - return SmtpErrorClass.Permanent; - } - - return SmtpErrorClass.Unknown; - } - - if (ex is SmtpProtocolException - or ServiceNotConnectedException - or SocketException - or TimeoutException) - { - return SmtpErrorClass.Transient; - } - - return SmtpErrorClass.Unknown; - } - - private static bool IsTransientSmtpError(Exception ex, CancellationToken cancellationToken) - { - return ClassifySmtpError(ex, cancellationToken) == SmtpErrorClass.Transient; - } - - /// - /// Masks SMTP credential secrets out of free text (typically an SMTP server's - /// exception message) before it is logged or stored. Mirrors - /// NotificationService.CredentialRedactor, which is internal to that - /// project and so cannot be referenced here. - /// - private static string ScrubCredentials(string? text, string? credentials) - { - if (string.IsNullOrEmpty(text) || string.IsNullOrEmpty(credentials)) - { - return text ?? string.Empty; - } - - var result = text; - - // Mask each colon-delimited component (user, password, tenant, clientId, - // clientSecret) and the whole packed string. 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, "***REDACTED***", StringComparison.Ordinal); - } - - return result; - } } diff --git a/src/ScadaLink.NotificationService/CredentialRedactor.cs b/src/ScadaLink.NotificationService/CredentialRedactor.cs index 1cd270d..41f4dc0 100644 --- a/src/ScadaLink.NotificationService/CredentialRedactor.cs +++ b/src/ScadaLink.NotificationService/CredentialRedactor.cs @@ -6,8 +6,12 @@ namespace ScadaLink.NotificationService; /// 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. +/// +/// Public so the central Notification Outbox's EmailNotificationDeliveryAdapter +/// can share this exact redaction logic rather than carry a divergent copy. +/// /// -internal static class CredentialRedactor +public static class CredentialRedactor { private const string Mask = "***REDACTED***"; diff --git a/src/ScadaLink.NotificationService/NotificationDeliveryService.cs b/src/ScadaLink.NotificationService/NotificationDeliveryService.cs index 999d8e5..c760e97 100644 --- a/src/ScadaLink.NotificationService/NotificationDeliveryService.cs +++ b/src/ScadaLink.NotificationService/NotificationDeliveryService.cs @@ -1,7 +1,4 @@ -using System.Net.Sockets; using System.Text.Json; -using MailKit; -using MailKit.Net.Smtp; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using MimeKit; @@ -121,7 +118,7 @@ public class NotificationDeliveryService : INotificationDeliveryService, IDispos // NS-002: a caller-requested cancellation propagates; it is not buffered. throw; } - catch (Exception ex) when (IsTransientSmtpError(ex, cancellationToken)) + catch (Exception ex) when (SmtpErrorClassifier.IsTransient(ex, cancellationToken)) { // WP-12: Transient SMTP failure — hand to S&F. // NS-009: scrub credential fragments before logging. @@ -156,7 +153,7 @@ public class NotificationDeliveryService : INotificationDeliveryService, IDispos } catch (Exception ex) { - // NS-015: a failure that ClassifySmtpError does not recognise (Unknown) — + // NS-015: a failure that SmtpErrorClassifier 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 @@ -256,14 +253,14 @@ public class NotificationDeliveryService : INotificationDeliveryService, IDispos // permanent failure — let it propagate so the engine does not park. throw; } - catch (Exception ex) when (IsTransientSmtpError(ex, cancellationToken)) + catch (Exception ex) when (SmtpErrorClassifier.IsTransient(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) — + // NS-014: an exception SmtpErrorClassifier 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 @@ -350,8 +347,13 @@ public class NotificationDeliveryService : INotificationDeliveryService, IDispos /// /// NS-008: Validates the sender and recipient email addresses, returning a /// human-readable error string if any is malformed, or null if all parse. + /// + /// Public and shared: the central Notification Outbox's + /// EmailNotificationDeliveryAdapter applies the same pre-send address + /// validation, so both delivery paths use this single implementation. + /// /// - internal static string? ValidateAddresses( + public static string? ValidateAddresses( string fromAddress, IReadOnlyList recipients) { if (!MailboxAddress.TryParse(fromAddress, out _)) @@ -420,14 +422,14 @@ public class NotificationDeliveryService : INotificationDeliveryService, IDispos // not be misclassified as a transient SMTP failure and buffered for retry. throw; } - catch (Exception ex) when (ClassifySmtpError(ex, cancellationToken) == SmtpErrorClass.Permanent + catch (Exception ex) when (SmtpErrorClassifier.Classify(ex, cancellationToken) == SmtpErrorClass.Permanent && ex is not SmtpPermanentException) { // NS-003: Permanent SMTP failure (5xx) — surface a typed permanent exception. throw new SmtpPermanentException(ex.Message, ex); } // Transient and SmtpPermanentException both propagate unchanged: SendAsync's - // catch filters (SmtpPermanentException / IsTransientSmtpError) handle them. + // catch filters (SmtpPermanentException / SmtpErrorClassifier.IsTransient) handle them. finally { // NS-010: always tear the connection down, regardless of outcome. The @@ -451,64 +453,4 @@ public class NotificationDeliveryService : INotificationDeliveryService, IDispos limiter.Release(); } } - - private enum SmtpErrorClass - { - /// Cancellation or an unrecognised exception — caller decides. - Unknown, - /// Retryable failure (4xx, connection/socket/protocol error, timeout). - Transient, - /// Non-retryable failure (5xx) — must be returned to the script. - Permanent, - } - - /// - /// NS-002/NS-003: Classifies an SMTP failure using MailKit's typed exceptions and - /// the numeric rather than locale-dependent substring - /// matching on the exception message. A cancellation requested by the caller is - /// never treated as a transient SMTP error. - /// - private static SmtpErrorClass ClassifySmtpError(Exception ex, CancellationToken cancellationToken) - { - // A deliberate cancellation is not an SMTP error at all. - if (ex is OperationCanceledException && cancellationToken.IsCancellationRequested) - { - return SmtpErrorClass.Unknown; - } - - // MailKit reports SMTP command failures with the real status code; the - // SmtpStatusCode enum's underlying value is the numeric SMTP reply code. - if (ex is SmtpCommandException command) - { - var code = (int)command.StatusCode; - if (code >= 400 && code < 500) - { - return SmtpErrorClass.Transient; - } - - if (code >= 500 && code < 600) - { - return SmtpErrorClass.Permanent; - } - - return SmtpErrorClass.Unknown; - } - - // Protocol errors, a dropped/unavailable service, socket failures and - // timeouts are all retryable — the message has not been rejected. - if (ex is SmtpProtocolException - or ServiceNotConnectedException - or SocketException - or TimeoutException) - { - return SmtpErrorClass.Transient; - } - - return SmtpErrorClass.Unknown; - } - - private static bool IsTransientSmtpError(Exception ex, CancellationToken cancellationToken) - { - return ClassifySmtpError(ex, cancellationToken) == SmtpErrorClass.Transient; - } } diff --git a/src/ScadaLink.NotificationService/SmtpErrorClassifier.cs b/src/ScadaLink.NotificationService/SmtpErrorClassifier.cs new file mode 100644 index 0000000..9cfd635 --- /dev/null +++ b/src/ScadaLink.NotificationService/SmtpErrorClassifier.cs @@ -0,0 +1,93 @@ +using System.Net.Sockets; +using MailKit; +using MailKit.Net.Smtp; + +namespace ScadaLink.NotificationService; + +/// +/// NS-002/NS-003: The classification of an SMTP delivery failure. This decides +/// whether a failure is retried or surfaced to the caller, so it is part of the +/// system's correctness-relevant behaviour. +/// +public enum SmtpErrorClass +{ + /// Cancellation or an unrecognised exception — caller decides. + Unknown, + + /// Retryable failure (4xx, connection/socket/protocol error, timeout). + Transient, + + /// Non-retryable failure (5xx) — must not be retried. + Permanent, +} + +/// +/// NS-002/NS-003: Classifies an SMTP failure using MailKit's typed exceptions and +/// the numeric rather than locale-dependent substring +/// matching on the exception message. +/// +/// Public and shared: both (store-and-forward +/// delivery) and the central Notification Outbox's EmailNotificationDeliveryAdapter +/// route every SMTP failure through this single policy, so a transient/permanent +/// boundary change cannot diverge between the two delivery paths. +/// +/// +public static class SmtpErrorClassifier +{ + /// + /// Classifies an SMTP failure. A cancellation requested by the caller is never + /// treated as a transient SMTP error. + /// + /// The exception thrown by the SMTP send sequence. + /// + /// The token governing the send; a requested cancellation classifies as + /// so the caller can re-throw it. + /// + public static SmtpErrorClass Classify(Exception ex, CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(ex); + + // A deliberate cancellation is not an SMTP error at all. + if (ex is OperationCanceledException && cancellationToken.IsCancellationRequested) + { + return SmtpErrorClass.Unknown; + } + + // MailKit reports SMTP command failures with the real status code; the + // SmtpStatusCode enum's underlying value is the numeric SMTP reply code. + if (ex is SmtpCommandException command) + { + var code = (int)command.StatusCode; + if (code >= 400 && code < 500) + { + return SmtpErrorClass.Transient; + } + + if (code >= 500 && code < 600) + { + return SmtpErrorClass.Permanent; + } + + return SmtpErrorClass.Unknown; + } + + // Protocol errors, a dropped/unavailable service, socket failures and + // timeouts are all retryable — the message has not been rejected. + if (ex is SmtpProtocolException + or ServiceNotConnectedException + or SocketException + or TimeoutException) + { + return SmtpErrorClass.Transient; + } + + return SmtpErrorClass.Unknown; + } + + /// + /// Convenience predicate: true when returns + /// . + /// + public static bool IsTransient(Exception ex, CancellationToken cancellationToken) + => Classify(ex, cancellationToken) == SmtpErrorClass.Transient; +} diff --git a/tests/ScadaLink.NotificationService.Tests/SmtpErrorClassifierTests.cs b/tests/ScadaLink.NotificationService.Tests/SmtpErrorClassifierTests.cs new file mode 100644 index 0000000..442934d --- /dev/null +++ b/tests/ScadaLink.NotificationService.Tests/SmtpErrorClassifierTests.cs @@ -0,0 +1,133 @@ +using System.Net.Sockets; +using MailKit; +using MailKit.Net.Smtp; +using MailKit.Security; + +namespace ScadaLink.NotificationService.Tests; + +/// +/// NS-002/NS-003: Tests for the shared SMTP error classification policy. This +/// policy is correctness-relevant — it decides whether a delivery failure is +/// retried (transient) or returned to the caller (permanent) — and is shared +/// between and the central outbox's +/// EmailNotificationDeliveryAdapter, so it deserves direct coverage. +/// +public class SmtpErrorClassifierTests +{ + [Theory] + [InlineData(421)] // service not available + [InlineData(450)] // mailbox unavailable (busy) + [InlineData(451)] // local error in processing + [InlineData(452)] // insufficient system storage + public void Classify_Smtp4xxCommand_IsTransient(int statusCode) + { + var ex = new SmtpCommandException( + SmtpErrorCode.MessageNotAccepted, (SmtpStatusCode)statusCode, "rejected"); + + Assert.Equal(SmtpErrorClass.Transient, SmtpErrorClassifier.Classify(ex, CancellationToken.None)); + } + + [Theory] + [InlineData(500)] // syntax error + [InlineData(550)] // mailbox unavailable (rejected) + [InlineData(553)] // mailbox name not allowed + [InlineData(554)] // transaction failed + public void Classify_Smtp5xxCommand_IsPermanent(int statusCode) + { + var ex = new SmtpCommandException( + SmtpErrorCode.MessageNotAccepted, (SmtpStatusCode)statusCode, "rejected"); + + Assert.Equal(SmtpErrorClass.Permanent, SmtpErrorClassifier.Classify(ex, CancellationToken.None)); + } + + [Fact] + public void Classify_SmtpCommandWithUnusualCode_IsUnknown() + { + // A status code outside the 4xx/5xx bands is not classifiable. + var ex = new SmtpCommandException( + SmtpErrorCode.UnexpectedStatusCode, (SmtpStatusCode)250, "ok-ish"); + + Assert.Equal(SmtpErrorClass.Unknown, SmtpErrorClassifier.Classify(ex, CancellationToken.None)); + } + + [Fact] + public void Classify_SmtpProtocolException_IsTransient() + { + Assert.Equal( + SmtpErrorClass.Transient, + SmtpErrorClassifier.Classify(new SmtpProtocolException("protocol error"), CancellationToken.None)); + } + + [Fact] + public void Classify_ServiceNotConnectedException_IsTransient() + { + Assert.Equal( + SmtpErrorClass.Transient, + SmtpErrorClassifier.Classify(new ServiceNotConnectedException(), CancellationToken.None)); + } + + [Fact] + public void Classify_SocketException_IsTransient() + { + Assert.Equal( + SmtpErrorClass.Transient, + SmtpErrorClassifier.Classify(new SocketException(), CancellationToken.None)); + } + + [Fact] + public void Classify_TimeoutException_IsTransient() + { + Assert.Equal( + SmtpErrorClass.Transient, + SmtpErrorClassifier.Classify(new TimeoutException(), CancellationToken.None)); + } + + [Fact] + public void Classify_RequestedCancellation_IsUnknown() + { + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + Assert.Equal( + SmtpErrorClass.Unknown, + SmtpErrorClassifier.Classify(new OperationCanceledException(), cts.Token)); + } + + [Fact] + public void Classify_OperationCanceledWithoutRequestedCancellation_IsUnknown() + { + // Not a recognised SMTP error, and cancellation was not requested. + Assert.Equal( + SmtpErrorClass.Unknown, + SmtpErrorClassifier.Classify(new OperationCanceledException(), CancellationToken.None)); + } + + [Fact] + public void Classify_UnrecognisedException_IsUnknown() + { + Assert.Equal( + SmtpErrorClass.Unknown, + SmtpErrorClassifier.Classify(new InvalidOperationException("bad credential triple"), CancellationToken.None)); + } + + [Theory] + [InlineData(450, true)] + [InlineData(550, false)] + [InlineData(250, false)] + public void IsTransient_MatchesClassification(int statusCode, bool expectedTransient) + { + var ex = new SmtpCommandException( + SmtpErrorCode.MessageNotAccepted, (SmtpStatusCode)statusCode, "x"); + + Assert.Equal(expectedTransient, SmtpErrorClassifier.IsTransient(ex, CancellationToken.None)); + } + + [Fact] + public void IsTransient_RequestedCancellation_IsFalse() + { + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + Assert.False(SmtpErrorClassifier.IsTransient(new OperationCanceledException(), cts.Token)); + } +}