From a9393c89137e608a34307692ecb6e3ee3541af1a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 15:09:47 -0400 Subject: [PATCH] test(sms): regression tests for code-review fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lock the behaviors changed by the review-fix commit + the security invariants: - ManagementActorTests: UpdateSms/SmtpConfig now require Administrator (updated the existing success cases from Designer); + UpdateSmsConfig_WithDesignerRole_Returns Unauthorized and _WithEmptyAuthToken_PreservesExistingToken regression tests. - SecretEncryptionTests: SmsConfiguration.AuthToken stored-encrypted round-trip + null round-trip (AccountSid stays plaintext) — guards ApplySecretColumnEncryption. - ArtifactDiffTests: CompareSmsConfiguration New/Identical/Modified + the secret presence-only invariant (value never echoed, presence-flip shows only). - UpdateCommandContractTests: notification sms update core fields Required, --auth-token optional. - NotificationListsPageTests: SMS recipient badge shows phone, not "Name <>". - NotificationOutboxActorDispatchTests: SMS-typed notification routes to the SMS adapter (StubAdapter.Type made configurable), not the Email adapter. - NotificationRecipientTests (new): ForEmail/ForSms + public-ctor invariants. --- .../UpdateCommandContractTests.cs | 15 ++++ .../Pages/NotificationListsPageTests.cs | 29 ++++++++ .../Entities/NotificationRecipientTests.cs | 63 ++++++++++++++++ .../SecretEncryptionTests.cs | 42 +++++++++++ .../ManagementActorTests.cs | 65 +++++++++++++++-- .../NotificationOutboxActorDispatchTests.cs | 35 ++++++++- .../Import/ArtifactDiffTests.cs | 73 +++++++++++++++++++ 7 files changed, 312 insertions(+), 10 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Entities/NotificationRecipientTests.cs diff --git a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/UpdateCommandContractTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/UpdateCommandContractTests.cs index 289d32a9..e10f46e5 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/UpdateCommandContractTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/UpdateCommandContractTests.cs @@ -83,6 +83,21 @@ public class UpdateCommandContractTests } } + [Fact] + public void SmsUpdate_CoreFieldsRequired() + { + // `notification sms update` is a whole-replace of the SMS provider config, so its + // identity + non-secret core fields must be Required — a missing --account-sid or + // --from-number would otherwise send null and wipe a stored value. --auth-token + // stays optional (preserve-if-omitted; empty == omitted, never "clear"). + var update = UpdateCommand(NotificationCommands.Build(Url, Format, Username, Password), "sms", "update"); + AssertRequired(update, "--id", "--account-sid", "--from-number"); + + var authToken = update.Options.SingleOrDefault(o => o.Name == "--auth-token"); + Assert.True(authToken != null, "sms update is missing option '--auth-token'."); + Assert.False(authToken!.Required, "sms update '--auth-token' must be optional (preserve-if-omitted)."); + } + [Fact] public void ApiMethodUpdate_CoreFieldsRequired() => AssertRequired(UpdateCommand(ApiMethodCommands.Build(Url, Format, Username, Password), "update"), "--script"); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs index 475a6702..04d3ec46 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs @@ -124,6 +124,35 @@ public class NotificationListsPageTests : BunitContext }); } + [Fact] + public void SmsListRecipientBadge_ShowsPhoneNumber_NotEmptyAngleBrackets() + { + // CentralUI-NNN regression: an SMS recipient carries a PhoneNumber and a null + // EmailAddress. The badge must render the phone, not the email-shaped "Name <>". + var repo = Substitute.For(); + repo.GetAllNotificationListsAsync() + .Returns(Task.FromResult>( + new List + { + new("SMS Alerts") { Id = 7, Type = NotificationType.Sms } + })); + repo.GetRecipientsByListIdAsync(7) + .Returns(Task.FromResult>( + new List { NotificationRecipient.ForSms("Jane", "+15551234567") })); + Services.AddSingleton(repo); + WireAuthAndDialog(); + + var cut = Render(); + + cut.WaitForAssertion(() => + { + Assert.Contains("+15551234567", cut.Markup); + // The bug rendered an empty contact field ("Jane <>") for SMS recipients. + Assert.DoesNotContain("Jane <>", cut.Markup); + Assert.DoesNotContain("Jane <>", cut.Markup); + }); + } + [Fact] public void RendersTypeColumn_Email() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Entities/NotificationRecipientTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Entities/NotificationRecipientTests.cs new file mode 100644 index 00000000..8ccfad29 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Entities/NotificationRecipientTests.cs @@ -0,0 +1,63 @@ +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Notifications; + +namespace ZB.MOM.WW.ScadaBridge.Commons.Tests.Entities; + +/// +/// Commons-NNN: locks the construction invariants +/// (exactly one contact field populated per channel; non-blank name on every path). +/// +public class NotificationRecipientTests +{ + [Fact] + public void ForEmail_SetsEmail_LeavesPhoneNull() + { + var r = NotificationRecipient.ForEmail("Jane", "jane@example.com"); + + Assert.Equal("Jane", r.Name); + Assert.Equal("jane@example.com", r.EmailAddress); + Assert.Null(r.PhoneNumber); + } + + [Fact] + public void ForSms_SetsPhone_LeavesEmailNull() + { + var r = NotificationRecipient.ForSms("Jane", "+15551234567"); + + Assert.Equal("Jane", r.Name); + Assert.Equal("+15551234567", r.PhoneNumber); + Assert.Null(r.EmailAddress); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void ForEmail_BlankName_Throws(string? name) + => Assert.Throws(() => NotificationRecipient.ForEmail(name!, "jane@example.com")); + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void ForSms_BlankName_Throws(string? name) + => Assert.Throws(() => NotificationRecipient.ForSms(name!, "+15551234567")); + + [Fact] + public void ForEmail_NullEmail_Throws() + => Assert.Throws(() => NotificationRecipient.ForEmail("Jane", null!)); + + [Fact] + public void ForSms_NullPhone_Throws() + => Assert.Throws(() => NotificationRecipient.ForSms("Jane", null!)); + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void PublicCtor_BlankName_Throws(string? name) + => Assert.Throws(() => new NotificationRecipient(name!, "jane@example.com")); + + [Fact] + public void PublicCtor_NullEmail_Throws() + => Assert.Throws(() => new NotificationRecipient("Jane", null!)); +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SecretEncryptionTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SecretEncryptionTests.cs index c51f34ea..22255f81 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SecretEncryptionTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SecretEncryptionTests.cs @@ -114,6 +114,48 @@ public class SecretEncryptionTests : IDisposable Assert.Null(loaded.Credentials); } + [Fact] + public async Task SmsConfiguration_AuthToken_StoredEncrypted_RoundTrips() + { + // ConfigurationDatabase-NNN regression: the Twilio Auth Token is a secret and + // must be encrypted at rest exactly like SmtpConfiguration.Credentials. Guards + // against a future refactor dropping SmsConfiguration from ApplySecretColumnEncryption. + const string secret = "twilio-auth-token-do-not-leak"; + var sms = new SmsConfiguration("AC0123456789", "+15550000000") + { + AuthToken = secret + }; + _context.SmsConfigurations.Add(sms); + await _context.SaveChangesAsync(); + _context.ChangeTracker.Clear(); + + var raw = await ReadRawColumnAsync("SmsConfigurations", "AuthToken", sms.Id); + Assert.NotNull(raw); + Assert.NotEqual(secret, raw); + Assert.DoesNotContain("twilio-auth-token-do-not-leak", raw); + + var loaded = await _context.SmsConfigurations.SingleAsync(s => s.Id == sms.Id); + Assert.Equal(secret, loaded.AuthToken); + // AccountSid is NOT a secret (it also rides in the Twilio URL path) — stored verbatim. + var rawSid = await ReadRawColumnAsync("SmsConfigurations", "AccountSid", sms.Id); + Assert.Equal("AC0123456789", rawSid); + } + + [Fact] + public async Task SmsConfiguration_NullAuthToken_RoundTripsAsNull() + { + var sms = new SmsConfiguration("AC0123456789", "+15550000000") + { + AuthToken = null + }; + _context.SmsConfigurations.Add(sms); + await _context.SaveChangesAsync(); + _context.ChangeTracker.Clear(); + + var loaded = await _context.SmsConfigurations.SingleAsync(s => s.Id == sms.Id); + Assert.Null(loaded.AuthToken); + } + private async Task ReadRawColumnAsync(string table, string column, int id) { var connection = _context.Database.GetDbConnection(); diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index 70037387..12f7c517 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -1293,7 +1293,7 @@ public class ManagementActorTests : TestKit, IDisposable var actor = CreateActor(); var envelope = Envelope( new UpdateSmtpConfigCommand(1, "new.example.com", 465, "Basic", "new@example.com", "SSL", "user:pass"), - "Designer"); + "Administrator"); actor.Tell(envelope); @@ -1323,7 +1323,7 @@ public class ManagementActorTests : TestKit, IDisposable var actor = CreateActor(); var envelope = Envelope( new UpdateSmtpConfigCommand(1, "new.example.com", 465, "Basic", "new@example.com"), - "Designer"); + "Administrator"); actor.Tell(envelope); @@ -1534,7 +1534,7 @@ public class ManagementActorTests : TestKit, IDisposable var envelope = Envelope( new UpdateSmsConfigCommand(1, "ACnew", "+15551110000", "MGnew", "https://new.example.com", "new-secret"), - "Designer"); + "Administrator"); actor.Tell(envelope); @@ -1565,7 +1565,7 @@ public class ManagementActorTests : TestKit, IDisposable // AuthToken + ApiBaseUrl omitted -> preserve-if-null. var envelope = Envelope( new UpdateSmsConfigCommand(1, "ACnew", "+15551110000"), - "Designer"); + "Administrator"); actor.Tell(envelope); @@ -1601,7 +1601,7 @@ public class ManagementActorTests : TestKit, IDisposable var actor = CreateActor(); var envelope = Envelope( new UpdateSmsConfigCommand(1, "ACnew", "+15551110000", AuthToken: "super-secret-token"), - "Designer"); + "Administrator"); actor.Tell(envelope); @@ -1643,8 +1643,9 @@ public class ManagementActorTests : TestKit, IDisposable [Fact] public void UpdateSmsConfig_WithViewerRole_ReturnsUnauthorized() { - // Mirrors UpdateSmtpConfig gating: mutating the SMS config is a Designer - // operation; a read-only role cannot rotate the secret. + // MgmtSvc-021: mutating the SMS provider config rotates the Twilio secret and + // is Admin-only (the /notifications/sms page is RequireAdmin). A read-only role + // cannot reach it. var actor = CreateActor(); var envelope = Envelope( new UpdateSmsConfigCommand(1, "ACnew", "+15551110000"), @@ -1653,7 +1654,55 @@ public class ManagementActorTests : TestKit, IDisposable actor.Tell(envelope); var response = ExpectMsg(TimeSpan.FromSeconds(5)); - Assert.Contains("Designer", response.Message); + Assert.Contains("Administrator", response.Message); + } + + [Fact] + public void UpdateSmsConfig_WithDesignerRole_ReturnsUnauthorized() + { + // MgmtSvc-021 regression: a Designer is blocked from the /notifications/sms UI + // (RequireAdmin), so the actor gate must reject them too — otherwise a Designer + // could rotate a production Twilio Auth Token via the CLI/Management API. + var actor = CreateActor(); + var envelope = Envelope( + new UpdateSmsConfigCommand(1, "ACnew", "+15551110000", AuthToken: "rotate-me"), + "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("Administrator", response.Message); + } + + [Fact] + public void UpdateSmsConfig_WithEmptyAuthToken_PreservesExistingToken() + { + // MgmtSvc-021 regression: an explicit empty/whitespace AuthToken must be treated + // as "omitted" (preserve the stored secret), never as "clear it" — a Twilio token + // is always required, so clearing it would 401 every subsequent send. + var notifRepo = Substitute.For(); + var existing = new Commons.Entities.Notifications.SmsConfiguration("ACold", "+15550000000") + { + Id = 1, + AuthToken = "old-secret", + }; + notifRepo.GetAllSmsConfigurationsAsync(Arg.Any()) + .Returns(new List { existing }); + _services.AddScoped(_ => notifRepo); + + var actor = CreateActor(); + var envelope = Envelope( + new UpdateSmsConfigCommand(1, "ACnew", "+15551110000", AuthToken: " "), + "Administrator"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal(envelope.CorrelationId, response.CorrelationId); + // The blank token was ignored; the stored secret survives. + Assert.Equal("old-secret", existing.AuthToken); + // Non-secret fields still updated. + Assert.Equal("ACnew", existing.AccountSid); } [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/NotificationOutboxActorDispatchTests.cs b/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/NotificationOutboxActorDispatchTests.cs index 160dcf50..ce9f1086 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/NotificationOutboxActorDispatchTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/NotificationOutboxActorDispatchTests.cs @@ -52,15 +52,17 @@ public class NotificationOutboxActorDispatchTests : TestKit private readonly Func _outcome; private readonly TimeSpan _delay; - public StubAdapter(Func outcome, TimeSpan? delay = null) + public StubAdapter(Func outcome, TimeSpan? delay = null, + NotificationType type = NotificationType.Email) { _outcome = outcome; _delay = delay ?? TimeSpan.Zero; + Type = type; } public int CallCount; - public NotificationType Type => NotificationType.Email; + public NotificationType Type { get; } public async Task DeliverAsync( Notification notification, CancellationToken cancellationToken = default) @@ -151,6 +153,35 @@ public class NotificationOutboxActorDispatchTests : TestKit }); } + [Fact] + public void Dispatch_SmsTypedNotification_RoutesToSmsAdapter_NotEmailAdapter() + { + // NotificationOutbox-NNN: the Type-keyed adapter selection must land an SMS-typed + // notification on the SMS adapter (not the Email one) and apply its outcome. + SetupSmtpRetryPolicy(maxRetries: 5, retryDelay: TimeSpan.FromMinutes(1)); + var notification = MakeNotification(type: NotificationType.Sms); + _outboxRepository.GetDueAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new[] { notification }); + var emailAdapter = new StubAdapter(() => DeliveryOutcome.Success("ops@example.com"), + type: NotificationType.Email); + var smsAdapter = new StubAdapter(() => DeliveryOutcome.Success("+15551234567"), + type: NotificationType.Sms); + var actor = CreateActor([emailAdapter, smsAdapter]); + + actor.Tell(InternalMessages.DispatchTick.Instance); + + AwaitAssert(() => + { + Assert.Equal(1, smsAdapter.CallCount); + Assert.Equal(0, emailAdapter.CallCount); + _outboxRepository.Received(1).UpdateAsync( + Arg.Is(n => + n.Status == NotificationStatus.Delivered && + n.ResolvedTargets == "+15551234567"), + Arg.Any()); + }); + } + [Fact] public void Success_MarksNotificationDelivered_WithResolvedTargets() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs index 9270c82b..46cffe46 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs @@ -1,6 +1,7 @@ using System.Text.Json; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances; using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Notifications; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Scripts; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates; @@ -268,6 +269,78 @@ public sealed class ArtifactDiffTests Assert.DoesNotContain("4840", item.FieldDiffJson!); } + // ============ SMS: CompareSmsConfiguration (S10b) ============ + + private static SmsConfiguration ExistingSms() => + new("AC1", "+15550000000") { Id = 1, AuthToken = "stored-token" }; + + private static SmsConfigDto SmsDto( + string from = "+15550000000", + string? msgSid = null, + bool withSecret = true) => + new("AC1", from, msgSid, ApiBaseUrl: null, ConnectionTimeoutSeconds: 30, + MaxRetries: 10, RetryDelay: TimeSpan.FromMinutes(1), + Secrets: withSecret + ? new SecretsBlock(new Dictionary { ["AuthToken"] = "wire-token" }) + : null); + + [Fact] + public void CompareSmsConfiguration_ExistingNull_New() + { + var item = _diff.CompareSmsConfiguration(SmsDto(), existing: null); + + Assert.Equal(ConflictKind.New, item.Kind); + Assert.Equal("SmsConfiguration", item.EntityType); + Assert.Equal("AC1", item.Name); + } + + [Fact] + public void CompareSmsConfiguration_AllFieldsAndPresenceMatch_Identical() + { + var item = _diff.CompareSmsConfiguration(SmsDto(), ExistingSms()); + + Assert.Equal(ConflictKind.Identical, item.Kind); + Assert.Null(item.FieldDiffJson); + } + + [Fact] + public void CompareSmsConfiguration_FromNumberDiffers_Modified() + { + var item = _diff.CompareSmsConfiguration(SmsDto(from: "+15559999999"), ExistingSms()); + + Assert.Equal(ConflictKind.Modified, item.Kind); + var change = ChangeFor(item, "FromNumber"); + Assert.Equal("+15550000000", change.GetProperty("oldValue").GetString()); + Assert.Equal("+15559999999", change.GetProperty("newValue").GetString()); + } + + [Fact] + public void CompareSmsConfiguration_AuthTokenValueChangesButPresencePreserved_NoSecretEcho() + { + // Both sides carry a token; only the value differs. Presence-only comparison => + // no Secrets.AuthToken change, and neither token value is ever echoed. + var item = _diff.CompareSmsConfiguration(SmsDto(withSecret: true), ExistingSms()); + + Assert.Equal(ConflictKind.Identical, item.Kind); + var json = item.FieldDiffJson ?? string.Empty; + Assert.DoesNotContain("stored-token", json); + Assert.DoesNotContain("wire-token", json); + } + + [Fact] + public void CompareSmsConfiguration_AuthTokenPresenceFlips_ShowsPresentMarkerOnly() + { + // Existing has a token; incoming carries no secret => a presence change that + // surfaces only the "" marker, never the token value. + var item = _diff.CompareSmsConfiguration(SmsDto(withSecret: false), ExistingSms()); + + Assert.Equal(ConflictKind.Modified, item.Kind); + var change = ChangeFor(item, "Secrets.AuthToken"); + Assert.Equal("", change.GetProperty("oldValue").GetString()); + Assert.False(change.TryGetProperty("newValue", out _)); + Assert.DoesNotContain("stored-token", item.FieldDiffJson!); + } + // ============ M8: CompareInstance ============ [Fact]