diff --git a/src/server/HSMServer/Controllers/NotificationsController.cs b/src/server/HSMServer/Controllers/NotificationsController.cs index 3adf5d046..efcead4fb 100644 --- a/src/server/HSMServer/Controllers/NotificationsController.cs +++ b/src/server/HSMServer/Controllers/NotificationsController.cs @@ -119,6 +119,35 @@ public async Task SendTestSlackMessage([FromQuery] Guid id) return Ok(); } + [HttpPost] + [TelegramRoleFilterById(nameof(id), ProductRoleEnum.ProductManager)] + public async Task RemoveTelegramBinding(Guid id) => + await ClearChannel(id, clearTelegram: true); + + [HttpPost] + [TelegramRoleFilterById(nameof(id), ProductRoleEnum.ProductManager)] + public async Task ClearSlackWebhook(Guid id) => + await ClearChannel(id, clearSlack: true); + + [HttpPost] + [TelegramRoleFilterById(nameof(id), ProductRoleEnum.ProductManager)] + public async Task ClearMattermostWebhook(Guid id) => + await ClearChannel(id, clearMattermost: true); + + + private async Task ClearChannel(Guid id, bool clearTelegram = false, bool clearSlack = false, bool clearMattermost = false) + { + var update = new ChatUpdate + { + Id = id, + ClearTelegramBinding = clearTelegram, + ClearSlackWebhook = clearSlack, + ClearMattermostWebhook = clearMattermost, + }; + + return await ChatsManager.TryUpdate(update) ? Ok() : NotFound(); + } + private ChatFoldersViewModel BuildChatFolders(Chat chat) { diff --git a/src/server/HSMServer/Notifications/Chats/Chat.cs b/src/server/HSMServer/Notifications/Chats/Chat.cs index d5f7d4d3d..1fc1116e2 100644 --- a/src/server/HSMServer/Notifications/Chats/Chat.cs +++ b/src/server/HSMServer/Notifications/Chats/Chat.cs @@ -29,9 +29,13 @@ public sealed class Chat : BaseServerModel // Telegram (optional) public ChatId? TelegramChatId { get; private set; } - public ConnectedChatType? TelegramType { get; init; } + // init would block the ClearTelegramBinding path — ApplyUpdate needs to null these out so + // ToEntity() drops them on the next round-trip. internal set keeps existing object + // initializers in ChatsManager.TryConnect and ChatsManagerTests working; outside this + // assembly the surface is effectively read-only. + public ConnectedChatType? TelegramType { get; internal set; } - public DateTime? AuthorizationTime { get; init; } + public DateTime? AuthorizationTime { get; internal set; } // Slack (optional) @@ -98,6 +102,19 @@ protected override void ApplyUpdate(ChatUpdate update) if (update.TelegramChatId.HasValue) TelegramChatId = new ChatId(update.TelegramChatId.Value); + + if (update.ClearTelegramBinding is true) + { + TelegramChatId = null; + TelegramType = null; + AuthorizationTime = null; + } + + if (update.ClearSlackWebhook is true) + SlackWebhookUrl = null; + + if (update.ClearMattermostWebhook is true) + MattermostWebhookUrl = null; } public override ChatEntity ToEntity() diff --git a/src/server/HSMServer/Notifications/Chats/ChatUpdate.cs b/src/server/HSMServer/Notifications/Chats/ChatUpdate.cs index e7ef27c25..1b438f32a 100644 --- a/src/server/HSMServer/Notifications/Chats/ChatUpdate.cs +++ b/src/server/HSMServer/Notifications/Chats/ChatUpdate.cs @@ -20,5 +20,17 @@ public record ChatUpdate : BaseUpdateRequest // Mattermost (optional) public string MattermostWebhookUrl { get; init; } + + + // Negative-clear flags. Chat.ApplyUpdate uses `?? current` for SlackWebhookUrl / + // MattermostWebhookUrl, so a null update value means "don't change" and submitting an + // empty string through EditChat cannot clear a saved webhook. Telegram binding is even + // stricter — TelegramType / AuthorizationTime are internal-set and only mutable here. + // These flags route around both issues by explicitly signalling "set this channel to null". + public bool? ClearTelegramBinding { get; init; } + + public bool? ClearSlackWebhook { get; init; } + + public bool? ClearMattermostWebhook { get; init; } } } diff --git a/src/server/HSMServer/Views/Notifications/EditChat.cshtml b/src/server/HSMServer/Views/Notifications/EditChat.cshtml index 1409c53d7..f347d18fe 100644 --- a/src/server/HSMServer/Views/Notifications/EditChat.cshtml +++ b/src/server/HSMServer/Views/Notifications/EditChat.cshtml @@ -16,6 +16,15 @@ var icons = Model.ChatBrandIcons(); var isNewChat = Model.Id == Guid.Empty; var formAction = isNewChat ? nameof(NotificationsController.AddChat) : nameof(NotificationsController.EditChat); + + // First configured channel wins; for a brand-new chat we land on Slack because Telegram can + // only be added through the bot-invite flow (Show setup help), so Slack is the only fillable + // field. An existing chat with no channels configured (e.g. right after ClearSlackWebhook) + // also falls back to Slack — landing on Telegram would surprise the user, who just acted on + // the Slack webhook. Mattermost only wins if it's the sole configured channel. + var defaultTab = isTelegramBound ? "telegram" + : Model.HasMattermost && !Model.HasSlack ? "mattermost" + : "slack"; } @@ -31,7 +40,7 @@ - Remove + Remove chat @@ -70,8 +79,10 @@ -
- +
+
+ +
@@ -80,7 +91,7 @@ -
+
sec @@ -88,63 +99,107 @@
- @if (isTelegramBound) - { -
- -
- - - @Model.TelegramType?.GetDisplayName() - - ChatId: @Model.TelegramChatId - Connected: @Model.AuthorizationTime?.ToLocalTime().ToString("g") -
-
- } - else - { - @await Html.PartialAsync("_NewChatHelpModal.cshtml", Model.Id) -
- -
- - Show setup help - -
-
- } +
    + + + +
-
-
- -
-
- +
+
+ @if (isTelegramBound) + { +
+ +
+ + + @Model.TelegramType?.GetDisplayName() + + ChatId: @Model.TelegramChatId + Connected: @Model.AuthorizationTime?.ToLocalTime().ToString("g") +
+
+
+ Send test Telegram message + Remove Telegram +
+ } + else + { + @await Html.PartialAsync("_NewChatHelpModal.cshtml", Model.Id) +
+ +
+ + Show setup help + +
+
+ }
-
-
-
- +
+
+
+ +
+
+ +
+
+ @if (Model.HasSlack) + { +
+ Send test Slack message + Remove Slack +
+ }
-
- - Mattermost delivery is not yet implemented. + +
+
+
+ +
+
+ + Mattermost delivery is not yet implemented. +
+
+ @if (Model.HasMattermost) + { +
+ Remove Mattermost +
+ }
-
- @if (isTelegramBound) - { - Send test Telegram message - } - @if (Model.HasSlack) - { - Send test Slack message - } +
@@ -173,6 +228,25 @@ ); } + // Per-channel Remove buttons. Each clears only its channel; the Chat row and the other + // channels stay intact. Form reloads to reflect the post-clear state. + function removeChannel(channelName, actionUrl, confirmText) { + showConfirmationModal( + `Removing ${channelName} from chat '@Model.Name'`, + confirmText, + function () { + $.ajax({ + type: 'POST', + url: actionUrl + `?id=@Model.Id`, + cache: false, + async: true, + success: function () { window.location.reload(); }, + error: function () { showToast(`Failed to remove ${channelName.toLowerCase()} from chat.`); } + }); + } + ); + } + $(function () { $("#sendTestTelegram").off("click").on("click", function () { var chatId = $(this).data("chat-id"); @@ -187,5 +261,29 @@ .done(function () { showToast("Test Slack message sent."); }) .fail(function () { showToast("Failed to send test Slack message."); }); }); + + $("#removeTelegram").off("click").on("click", function () { + removeChannel( + "Telegram binding", + "@Url.Action(nameof(NotificationsController.RemoveTelegramBinding), ViewConstants.NotificationsController)", + `Telegram binding will be removed from chat '@Model.Name'. The chat itself, its folder bindings, and other channels stay intact.` + ); + }); + + $("#removeSlack").off("click").on("click", function () { + removeChannel( + "Slack webhook", + "@Url.Action(nameof(NotificationsController.ClearSlackWebhook), ViewConstants.NotificationsController)", + `Slack webhook will be removed from chat '@Model.Name'. The chat itself and other channels stay intact.` + ); + }); + + $("#removeMattermost").off("click").on("click", function () { + removeChannel( + "Mattermost webhook", + "@Url.Action(nameof(NotificationsController.ClearMattermostWebhook), ViewConstants.NotificationsController)", + `Mattermost webhook will be removed from chat '@Model.Name'. The chat itself and other channels stay intact.` + ); + }); }); diff --git a/src/tests/Autotests/products/modify_folder_chat.spec.ts b/src/tests/Autotests/products/modify_folder_chat.spec.ts index 84fc792de..740e849b3 100644 --- a/src/tests/Autotests/products/modify_folder_chat.spec.ts +++ b/src/tests/Autotests/products/modify_folder_chat.spec.ts @@ -5,6 +5,7 @@ import { uniqueName, cleanup } from '../fixtures.ts'; const folderName = uniqueName('Fldr'); const slackChatName = uniqueName('SlackChat'); +const slackRemoveChatName = uniqueName('SlackRm'); // XSS payload used as chat Name. Cleanup by text still works because Razor default-encodes // @chat.Name into the Configuration/_Chats.cshtml row's first td, so the literal payload text // appears in the DOM. The onerror handler would set window.__xss=1 if it ever executed. @@ -17,6 +18,7 @@ test.afterEach(async ({ browser }) => { try { await login(page, testConfig.admin_user, testConfig.admin_user_password, testConfig.apiUrl); await cleanup.chat(page, slackChatName); + await cleanup.chat(page, slackRemoveChatName); await cleanup.chat(page, xssChatName); await cleanup.folder(page, folderName); } finally { @@ -135,3 +137,58 @@ test('Folder Chats picker renders chat.Name as inert text (XSS lock-down)', asyn await page.getByRole('link', { name: 'Logout' }).click(); await expect(page.getByRole('button', { name: 'Submit' })).toBeVisible(); }); + + +// Covers issue #1271: per-channel Remove buttons on the EditChat form. The Slack "Remove" button +// must clear only the Slack webhook and leave the Chat row intact. Pre-#1271 the only destructive +// action was the header-level "Remove chat" link, which deletes the whole record. +test('EditChat: per-channel Remove clears Slack webhook without deleting the chat', async ({ page }) => { + const { apiUrl, admin_user, admin_user_password } = testConfig; + + // --- Login --- + await login(page, admin_user, admin_user_password, apiUrl); + + // --- Create a Slack chat via the unified Chats tab --- + await page.getByRole('link', { name: 'Configuration' }).click(); + await page.getByRole('tab', { name: 'Chats' }).click(); + await page.getByRole('link', { name: 'Add new chat' }).click(); + await page.locator('#Name').fill(slackRemoveChatName); + await page.locator('#SlackWebhookUrl').fill('https://hooks.slack.com/services/remove-test'); + await page.getByRole('button', { name: 'Save' }).click(); + + // AddChat POST redirects to Configuration. Reopen the Chats tab and click into EditChat via + // the row's action dropdown (matches the row pattern in _Chats.cshtml:53-86). + await page.getByRole('tab', { name: 'Chats' }).click(); + const chatRow = page.getByRole('row').filter({ hasText: slackRemoveChatName }); + await expect(chatRow).toBeVisible(); + await chatRow.locator('#actionButton').click(); + await page.getByRole('link', { name: 'View/Edit' }).click(); + + // EditChat should show the populated webhook and a "Remove Slack" button alongside the + // existing "Send test Slack message" button. + await expect(page.locator('#SlackWebhookUrl')).toHaveValue('https://hooks.slack.com/services/remove-test'); + await expect(page.locator('#removeSlack')).toBeVisible(); + + // Click Remove Slack → confirmation modal → OK. The AJAX POST hits ClearSlackWebhook and the + // page reloads (EditChat.cshtml removeChannel success handler). Wait for the reload to land + // before asserting — otherwise the auto-waiting toHaveValue('') can race against the + // still-rendering previous DOM and flake. + await page.locator('#removeSlack').click(); + await page.getByRole('button', { name: 'OK' }).click(); + await page.waitForLoadState('domcontentloaded'); + + // After reload, the webhook field is empty and the per-channel Remove button is gone + // (HasSlack is now false). The chat still exists — only the webhook was cleared. + await expect(page.locator('#SlackWebhookUrl')).toHaveValue(''); + await expect(page.locator('#removeSlack')).toHaveCount(0); + await expect(page.getByRole('heading', { name: /Edit chat/ })).toBeVisible(); + + // The Chats list must still contain the row — channel clear ≠ chat delete (acceptance #4). + await page.getByRole('link', { name: 'Configuration' }).click(); + await page.getByRole('tab', { name: 'Chats' }).click(); + await expect(page.getByRole('row').filter({ hasText: slackRemoveChatName })).toBeVisible(); + + // --- Logout --- + await page.getByRole('link', { name: 'Logout' }).click(); + await expect(page.getByRole('button', { name: 'Submit' })).toBeVisible(); +}); diff --git a/src/tests/HSMServer.Core.Tests/Notifications/ChatsManagerTests.cs b/src/tests/HSMServer.Core.Tests/Notifications/ChatsManagerTests.cs index 44687ece9..dfe48c146 100644 --- a/src/tests/HSMServer.Core.Tests/Notifications/ChatsManagerTests.cs +++ b/src/tests/HSMServer.Core.Tests/Notifications/ChatsManagerTests.cs @@ -144,6 +144,88 @@ public void Chat_BuiltFromPublicCtor_PersistsTelegramFieldsThroughToEntity() Assert.Equal(ConnectedChatType.TelegramPrivate, reloaded.TelegramType); } + // ApplyUpdate uses `?? current` for SlackWebhookUrl, so a null update value means "don't + // change" — submitting an empty webhook through EditChat cannot clear a saved value. The + // ClearSlackWebhook flag routes around that. Pinned because the EditChat "Remove Slack" + // button depends on it (issue #1271). + [Fact] + public async Task TryUpdate_ClearSlackWebhook_FlagSetsFieldToNull() + { + var manager = BuildManager(); + var chat = BuildSlackOnlyChat(); + await manager.TryAdd(chat); + await manager.TryUpdate(new ChatUpdate + { + Id = chat.Id, + SlackWebhookUrl = "https://hooks.slack.com/services/test", + }); + + var cleared = await manager.TryUpdate(new ChatUpdate { Id = chat.Id, ClearSlackWebhook = true }); + + Assert.True(cleared); + Assert.Null(manager[chat.Id].SlackWebhookUrl); + } + + [Fact] + public async Task TryUpdate_ClearMattermostWebhook_FlagSetsFieldToNull() + { + var manager = BuildManager(); + var chat = BuildSlackOnlyChat(); + await manager.TryAdd(chat); + await manager.TryUpdate(new ChatUpdate + { + Id = chat.Id, + MattermostWebhookUrl = "https://mattermost.example/hooks/test", + }); + + var cleared = await manager.TryUpdate(new ChatUpdate { Id = chat.Id, ClearMattermostWebhook = true }); + + Assert.True(cleared); + Assert.Null(manager[chat.Id].MattermostWebhookUrl); + } + + // ClearTelegramBinding must null all three Telegram fields and drop the chat from the + // _telegramChatIds index — otherwise GetChatByChatId would still return the chat and the + // next bot invite for the same Telegram chat would collide with the ghost entry. + [Fact] + public async Task TryUpdate_ClearTelegramBinding_FlagClearsFieldsAndRekeysIndex() + { + const long knownChatId = 42L; + var manager = BuildManager(); + var chat = BuildTelegramChat(knownChatId); + await manager.TryAdd(chat); + + Assert.NotNull(manager.GetChatByChatId(new Telegram.Bot.Types.ChatId(knownChatId))); + + var cleared = await manager.TryUpdate(new ChatUpdate { Id = chat.Id, ClearTelegramBinding = true }); + + Assert.True(cleared); + Assert.Null(manager[chat.Id].TelegramChatId); + Assert.Null(manager[chat.Id].TelegramType); + Assert.Null(manager[chat.Id].AuthorizationTime); + Assert.Null(manager.GetChatByChatId(new Telegram.Bot.Types.ChatId(knownChatId))); + } + + // ToEntity() gates the Telegram block on `TelegramType.HasValue` — so after ClearTelegramBinding + // the entity round-trip must omit TelegramType / TelegramChatId / AuthorizationTime entirely. + // Without this, reloading the chat from storage would resurrect the cleared binding. + [Fact] + public async Task Chat_AfterClearTelegramBinding_ToEntityOmitsTelegramFields() + { + const long knownChatId = 99L; + var manager = BuildManager(); + var chat = BuildTelegramChat(knownChatId); + await manager.TryAdd(chat); + + await manager.TryUpdate(new ChatUpdate { Id = chat.Id, ClearTelegramBinding = true }); + + var entity = manager[chat.Id].ToEntity(); + + Assert.Null(entity.TelegramType); + Assert.Null(entity.TelegramChatId); + Assert.Null(entity.AuthorizationTime); + } + private static ChatsManager BuildManager() {