test(sms): regression tests for code-review fixes
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 <present> 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.
This commit is contained in:
Joseph Doherty
@@ -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");
|
||||
|
||||
@@ -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<INotificationRepository>();
|
||||
repo.GetAllNotificationListsAsync()
|
||||
.Returns(Task.FromResult<IReadOnlyList<NotificationList>>(
|
||||
new List<NotificationList>
|
||||
{
|
||||
new("SMS Alerts") { Id = 7, Type = NotificationType.Sms }
|
||||
}));
|
||||
repo.GetRecipientsByListIdAsync(7)
|
||||
.Returns(Task.FromResult<IReadOnlyList<NotificationRecipient>>(
|
||||
new List<NotificationRecipient> { NotificationRecipient.ForSms("Jane", "+15551234567") }));
|
||||
Services.AddSingleton(repo);
|
||||
WireAuthAndDialog();
|
||||
|
||||
var cut = Render<NotificationListsPage>();
|
||||
|
||||
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()
|
||||
{
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
using ZB.MOM.WW.ScadaBridge.Commons.Entities.Notifications;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.Commons.Tests.Entities;
|
||||
|
||||
/// <summary>
|
||||
/// Commons-NNN: locks the <see cref="NotificationRecipient"/> construction invariants
|
||||
/// (exactly one contact field populated per channel; non-blank name on every path).
|
||||
/// </summary>
|
||||
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<ArgumentException>(() => NotificationRecipient.ForEmail(name!, "jane@example.com"));
|
||||
|
||||
[Theory]
|
||||
[InlineData(null)]
|
||||
[InlineData("")]
|
||||
[InlineData(" ")]
|
||||
public void ForSms_BlankName_Throws(string? name)
|
||||
=> Assert.Throws<ArgumentException>(() => NotificationRecipient.ForSms(name!, "+15551234567"));
|
||||
|
||||
[Fact]
|
||||
public void ForEmail_NullEmail_Throws()
|
||||
=> Assert.Throws<ArgumentNullException>(() => NotificationRecipient.ForEmail("Jane", null!));
|
||||
|
||||
[Fact]
|
||||
public void ForSms_NullPhone_Throws()
|
||||
=> Assert.Throws<ArgumentNullException>(() => NotificationRecipient.ForSms("Jane", null!));
|
||||
|
||||
[Theory]
|
||||
[InlineData(null)]
|
||||
[InlineData("")]
|
||||
[InlineData(" ")]
|
||||
public void PublicCtor_BlankName_Throws(string? name)
|
||||
=> Assert.Throws<ArgumentException>(() => new NotificationRecipient(name!, "jane@example.com"));
|
||||
|
||||
[Fact]
|
||||
public void PublicCtor_NullEmail_Throws()
|
||||
=> Assert.Throws<ArgumentNullException>(() => new NotificationRecipient("Jane", null!));
|
||||
}
|
||||
@@ -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<string?> ReadRawColumnAsync(string table, string column, int id)
|
||||
{
|
||||
var connection = _context.Database.GetDbConnection();
|
||||
|
||||
@@ -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<ManagementUnauthorized>(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<ManagementUnauthorized>(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<INotificationRepository>();
|
||||
var existing = new Commons.Entities.Notifications.SmsConfiguration("ACold", "+15550000000")
|
||||
{
|
||||
Id = 1,
|
||||
AuthToken = "old-secret",
|
||||
};
|
||||
notifRepo.GetAllSmsConfigurationsAsync(Arg.Any<CancellationToken>())
|
||||
.Returns(new List<Commons.Entities.Notifications.SmsConfiguration> { existing });
|
||||
_services.AddScoped(_ => notifRepo);
|
||||
|
||||
var actor = CreateActor();
|
||||
var envelope = Envelope(
|
||||
new UpdateSmsConfigCommand(1, "ACnew", "+15551110000", AuthToken: " "),
|
||||
"Administrator");
|
||||
|
||||
actor.Tell(envelope);
|
||||
|
||||
var response = ExpectMsg<ManagementSuccess>(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]
|
||||
|
||||
+33
-2
@@ -52,15 +52,17 @@ public class NotificationOutboxActorDispatchTests : TestKit
|
||||
private readonly Func<DeliveryOutcome> _outcome;
|
||||
private readonly TimeSpan _delay;
|
||||
|
||||
public StubAdapter(Func<DeliveryOutcome> outcome, TimeSpan? delay = null)
|
||||
public StubAdapter(Func<DeliveryOutcome> 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<DeliveryOutcome> 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<DateTimeOffset>(), Arg.Any<int>(), Arg.Any<CancellationToken>())
|
||||
.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<Notification>(n =>
|
||||
n.Status == NotificationStatus.Delivered &&
|
||||
n.ResolvedTargets == "+15551234567"),
|
||||
Arg.Any<CancellationToken>());
|
||||
});
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Success_MarksNotificationDelivered_WithResolvedTargets()
|
||||
{
|
||||
|
||||
@@ -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<string, string> { ["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 "<present>" 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("<present>", change.GetProperty("oldValue").GetString());
|
||||
Assert.False(change.TryGetProperty("newValue", out _));
|
||||
Assert.DoesNotContain("stored-token", item.FieldDiffJson!);
|
||||
}
|
||||
|
||||
// ============ M8: CompareInstance ============
|
||||
|
||||
[Fact]
|
||||
|
||||
Reference in New Issue
Block a user