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;