diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notification_settings_contain_a_password.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notification_settings_contain_a_password.cs new file mode 100644 index 0000000000..b33a40ed37 --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notification_settings_contain_a_password.cs @@ -0,0 +1,62 @@ +namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks +{ + using System.Threading.Tasks; + using AcceptanceTesting; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using ServiceControl.Notifications; + + /// + /// The SMTP password is write-only: the settings route never returns it. This checks the HTTP + /// response end to end. NotificationsControllerTests covers how a save changes the stored password. + /// + class When_email_notification_settings_contain_a_password : AcceptanceTest + { + [Test] + public async Task Should_not_return_the_password() + { + EmailNotifications returned = null; + + await Define() + .Do("Save settings with a password", async _ => + { + await this.Post("/api/notifications/email", SettingsRequest(Account, Password)); + + return true; + }) + .Do("Read the settings back", async _ => + { + returned = await this.TryGet("/api/notifications/email"); + + return returned != null; + }) + .Done(_ => true) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(returned.AuthenticationAccount, Is.EqualTo(Account), "The account is not a secret and stays visible"); + Assert.That(returned.AuthenticationPassword, Is.Null, "The password must never leave the instance through the API"); + } + } + + static object SettingsRequest(string account, string password) => new + { + smtp_server = "localhost", + smtp_port = 25, + authorization_account = account, + authorization_password = password, + enable_tls = false, + from = "servicecontrol@particular.net", + to = "oncall@particular.net" + }; + + const string Account = "svc-notifications"; + const string Password = "p@ssw0rd"; + + public class Context : ScenarioContext, ISequenceContext + { + public int Step { get; set; } + } + } +} diff --git a/src/ServiceControl.UnitTests/Notifications/NotificationsControllerTests.cs b/src/ServiceControl.UnitTests/Notifications/NotificationsControllerTests.cs new file mode 100644 index 0000000000..1f241bcb14 --- /dev/null +++ b/src/ServiceControl.UnitTests/Notifications/NotificationsControllerTests.cs @@ -0,0 +1,107 @@ +#nullable enable +namespace ServiceControl.UnitTests.Notifications; + +using System.Threading; +using System.Threading.Tasks; +using NUnit.Framework; +using ServiceControl.Notifications; +using ServiceControl.Notifications.Api; +using ServiceControl.Persistence; + +/// +/// The SMTP password is write-only: the settings route never returns it, and a save without a +/// password keeps the stored one. ServicePulse reads the settings into its form and sends the form +/// back, so a save without a password must not erase the password that was stored before. +/// +[TestFixture] +class NotificationsControllerTests +{ + [Test] + public async Task Should_not_return_the_password() + { + var store = new InMemoryNotificationsDataStore(Account, Password); + + var returned = await Controller(store).GetEmailNotificationsSettings(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(returned.AuthenticationAccount, Is.EqualTo(Account), "The account is not a secret and stays visible"); + Assert.That(returned.AuthenticationPassword, Is.Null, "The password must never leave the instance through the API"); + Assert.That(store.Stored.AuthenticationPassword, Is.EqualTo(Password), "Reading the settings must not change the stored password"); + } + } + + [Test] + public async Task Should_keep_the_stored_password_when_saved_without_one() + { + var store = new InMemoryNotificationsDataStore(Account, Password); + + await Controller(store).UpdateSettings(Request(Account, password: null)); + + Assert.That(store.Stored.AuthenticationPassword, Is.EqualTo(Password)); + } + + [Test] + public async Task Should_replace_the_stored_password_when_saved_with_a_new_one() + { + var store = new InMemoryNotificationsDataStore(Account, Password); + + await Controller(store).UpdateSettings(Request(Account, NewPassword)); + + Assert.That(store.Stored.AuthenticationPassword, Is.EqualTo(NewPassword)); + } + + [Test] + public async Task Should_remove_the_stored_password_when_saved_without_an_account() + { + var store = new InMemoryNotificationsDataStore(Account, Password); + + await Controller(store).UpdateSettings(Request(account: null, password: null)); + + Assert.That(store.Stored.AuthenticationPassword, Is.Null, "Without an account the password is never used, so it must not stay stored"); + } + + static NotificationsController Controller(INotificationsDataStore store) => new(store, null!, null!); + + static UpdateEmailNotificationsSettingsRequest Request(string? account, string? password) => new() + { + SmtpServer = "localhost", + SmtpPort = 25, + AuthorizationAccount = account, + AuthorizationPassword = password, + EnableTLS = false, + From = "servicecontrol@particular.net", + To = "oncall@particular.net" + }; + + const string Account = "svc-notifications"; + const string Password = "p@ssw0rd"; + const string NewPassword = "n3w-p@ssw0rd"; + + // Loads and saves copies, like the real stores, so the controller cannot change the stored value in place. + class InMemoryNotificationsDataStore(string account, string password) : INotificationsDataStore + { + public EmailNotifications Stored { get; private set; } = new() { AuthenticationAccount = account, AuthenticationPassword = password }; + + public Task LoadSettings(CancellationToken cancellationToken = default) => + Task.FromResult(new NotificationsSettings { Email = Copy(Stored) }); + + public Task SaveSettings(NotificationsSettings settings, CancellationToken cancellationToken = default) + { + Stored = Copy(settings.Email); + return Task.CompletedTask; + } + + static EmailNotifications Copy(EmailNotifications source) => new() + { + Enabled = source.Enabled, + SmtpServer = source.SmtpServer, + SmtpPort = source.SmtpPort, + AuthenticationAccount = source.AuthenticationAccount, + AuthenticationPassword = source.AuthenticationPassword, + EnableTLS = source.EnableTLS, + To = source.To, + From = source.From + }; + } +} diff --git a/src/ServiceControl/Notifications/Api/NotificationsController.cs b/src/ServiceControl/Notifications/Api/NotificationsController.cs index 91ea380b2f..c9ef2655d5 100644 --- a/src/ServiceControl/Notifications/Api/NotificationsController.cs +++ b/src/ServiceControl/Notifications/Api/NotificationsController.cs @@ -22,6 +22,11 @@ public async Task GetEmailNotificationsSettings(Cancellation { var notificationsSettings = await store.LoadSettings(cancellationToken); + // The SMTP password is write-only. It is a credential for the customer's mail system, so it + // never leaves the instance, whatever the caller's permissions. LoadSettings returns a detached + // snapshot, so this does not change the stored password. + notificationsSettings.Email.AuthenticationPassword = null; + return notificationsSettings.Email; } @@ -52,7 +57,19 @@ public async Task UpdateSettings(UpdateEmailNotificationsSettings emailSettings.SmtpPort = request.SmtpPort; emailSettings.AuthenticationAccount = request.AuthorizationAccount; - emailSettings.AuthenticationPassword = request.AuthorizationPassword; + + // The settings route does not return the password, so ServicePulse sends its form back without + // one. An empty password keeps the stored one. Without an account the password is never used, + // so it is removed. + if (string.IsNullOrEmpty(request.AuthorizationAccount)) + { + emailSettings.AuthenticationPassword = null; + } + else if (!string.IsNullOrEmpty(request.AuthorizationPassword)) + { + emailSettings.AuthenticationPassword = request.AuthorizationPassword; + } + emailSettings.EnableTLS = request.EnableTLS; emailSettings.From = request.From;