Email notification settings API returns the stored SMTP password

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;
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    class When_email_notification_settings_contain_a_password : AcceptanceTest
+    {
+        [Test]
+        public async Task Should_not_return_the_password()
+        {
+            EmailNotifications returned = null;
+
+            await Define<Context>()
+                .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<EmailNotifications>("/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; }
+        }
+    }
+}

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;
+
+/// <summary>
+/// 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.
+/// </summary>
+[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<NotificationsSettings> 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
+        };
+    }
+}

src/ServiceControl/Notifications/Api/NotificationsController.cs

@@ -22,6 +22,11 @@ public async Task<EmailNotifications> 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<IActionResult> 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;