🐛 Do not return the SMTP password from the notifications API (#5980)

GET /api/notifications/email returned the stored SMTP password to every
caller that can read the settings. This includes the reader role and, with
authentication disabled (the default), anonymous callers.

The password is now write-only:

- GET never returns it.
- A save with an empty password keeps the stored password. ServicePulse
  sends its form back without a password, because it no longer receives
  one.
- A save without an account removes the stored password.

Tests:

- NotificationsControllerTests (unit) cover the GET response and how each
  kind of save changes the stored password.
- One acceptance test checks the HTTP response end to end.

Author: ramonsmits

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;