dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

refactor(notification-outbox): share SMTP helpers between NotificationService and the Email adapter

Browse Source 
FU1 of the Notification Outbox follow-ups. EmailNotificationDeliveryAdapter
carried verbatim private copies of credential redaction, SMTP error
classification, and address validation because the NotificationService
helpers were internal. This eliminates the divergence risk by promoting the
helpers to public and deleting the adapter's copies.

- CredentialRedactor: internal -> public.
- Extract SmtpErrorClassifier + SmtpErrorClass enum into a new public static
  class; NotificationDeliveryService now routes classification through it
  (behavior unchanged). Adds focused SmtpErrorClassifierTests.
- NotificationDeliveryService.ValidateAddresses: internal -> public; the
  adapter calls it directly.
- EmailNotificationDeliveryAdapter: deleted ScrubCredentials, ClassifySmtpError,
  SmtpErrorClass, IsTransientSmtpError and ValidateAddresses copies.

No InternalsVisibleTo hack — specific helpers promoted to public. Both test
suites green; full solution builds clean.
This commit is contained in:
Joseph Doherty
2026-05-19 03:34:22 -04:00
parent 213b9c7c0a
commit 5e80f64cd8
5 changed files with 250 additions and 188 deletions
Download Patch File Download Diff File Expand all files Collapse all files

124
src/ScadaLink.NotificationOutbox/Delivery/EmailNotificationDeliveryAdapter.cs
View File

@@ -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
}
}
/// <summary>
/// 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).
/// </summary>
private static string? ValidateAddresses(
string fromAddress, IReadOnlyList<NotificationRecipient> 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
{
/// <summary>Cancellation or an unrecognised exception — caller decides.</summary>
Unknown,
/// <summary>Retryable failure (4xx, connection/socket/protocol error, timeout).</summary>
Transient,
/// <summary>Non-retryable failure (5xx) — must not be retried.</summary>
Permanent,
}
/// <summary>
/// Classifies an SMTP failure using MailKit's typed exceptions and the numeric
/// <see cref="SmtpStatusCode"/> rather than locale-dependent substring matching
/// (mirrors NS-002/NS-003 in <c>NotificationDeliveryService</c>).
/// </summary>
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;
}
/// <summary>
/// Masks SMTP credential secrets out of free text (typically an SMTP server's
/// exception message) before it is logged or stored. Mirrors
/// <c>NotificationService.CredentialRedactor</c>, which is internal to that
/// project and so cannot be referenced here.
/// </summary>
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;
}
}

6
src/ScadaLink.NotificationService/CredentialRedactor.cs
View File

@@ -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.
/// <para>
/// Public so the central Notification Outbox's <c>EmailNotificationDeliveryAdapter</c>
/// can share this exact redaction logic rather than carry a divergent copy.
/// </para>
/// </summary>
internal static class CredentialRedactor
public static class CredentialRedactor
{
private const string Mask = "***REDACTED***";

82
src/ScadaLink.NotificationService/NotificationDeliveryService.cs
View File

@@ -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
/// <summary>
/// NS-008: Validates the sender and recipient email addresses, returning a
/// human-readable error string if any is malformed, or null if all parse.
/// <para>
/// Public and shared: the central Notification Outbox's
/// <c>EmailNotificationDeliveryAdapter</c> applies the same pre-send address
/// validation, so both delivery paths use this single implementation.
/// </para>
/// </summary>
internal static string? ValidateAddresses(
public static string? ValidateAddresses(
string fromAddress, IReadOnlyList<NotificationRecipient> 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
{
/// <summary>Cancellation or an unrecognised exception — caller decides.</summary>
Unknown,
/// <summary>Retryable failure (4xx, connection/socket/protocol error, timeout).</summary>
Transient,
/// <summary>Non-retryable failure (5xx) — must be returned to the script.</summary>
Permanent,
}
/// <summary>
/// NS-002/NS-003: Classifies an SMTP failure using MailKit's typed exceptions and
/// the numeric <see cref="SmtpStatusCode"/> rather than locale-dependent substring
/// matching on the exception message. A cancellation requested by the caller is
/// never treated as a transient SMTP error.
/// </summary>
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;
}
}

93
src/ScadaLink.NotificationService/SmtpErrorClassifier.cs Normal file
View File

@@ -0,0 +1,93 @@
using System.Net.Sockets;
using MailKit;
using MailKit.Net.Smtp;
namespace ScadaLink.NotificationService;
/// <summary>
/// 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.
/// </summary>
public enum SmtpErrorClass
{
/// <summary>Cancellation or an unrecognised exception — caller decides.</summary>
Unknown,
/// <summary>Retryable failure (4xx, connection/socket/protocol error, timeout).</summary>
Transient,
/// <summary>Non-retryable failure (5xx) — must not be retried.</summary>
Permanent,
}
/// <summary>
/// NS-002/NS-003: Classifies an SMTP failure using MailKit's typed exceptions and
/// the numeric <see cref="SmtpStatusCode"/> rather than locale-dependent substring
/// matching on the exception message.
/// <para>
/// Public and shared: both <see cref="NotificationDeliveryService"/> (store-and-forward
/// delivery) and the central Notification Outbox's <c>EmailNotificationDeliveryAdapter</c>
/// route every SMTP failure through this single policy, so a transient/permanent
/// boundary change cannot diverge between the two delivery paths.
/// </para>
/// </summary>
public static class SmtpErrorClassifier
{
/// <summary>
/// Classifies an SMTP failure. A cancellation requested by the caller is never
/// treated as a transient SMTP error.
/// </summary>
/// <param name="ex">The exception thrown by the SMTP send sequence.</param>
/// <param name="cancellationToken">
/// The token governing the send; a requested cancellation classifies as
/// <see cref="SmtpErrorClass.Unknown"/> so the caller can re-throw it.
/// </param>
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;
}
/// <summary>
/// Convenience predicate: true when <see cref="Classify"/> returns
/// <see cref="SmtpErrorClass.Transient"/>.
/// </summary>
public static bool IsTransient(Exception ex, CancellationToken cancellationToken)
=> Classify(ex, cancellationToken) == SmtpErrorClass.Transient;
}

133
tests/ScadaLink.NotificationService.Tests/SmtpErrorClassifierTests.cs Normal file
View File

@@ -0,0 +1,133 @@
using System.Net.Sockets;
using MailKit;
using MailKit.Net.Smtp;
using MailKit.Security;
namespace ScadaLink.NotificationService.Tests;
/// <summary>
/// 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 <see cref="NotificationDeliveryService"/> and the central outbox's
/// <c>EmailNotificationDeliveryAdapter</c>, so it deserves direct coverage.
/// </summary>
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));
}
}