Fix custom webhook request body format

.github/workflows/notifications-test-and-build-workflow.yml

@@ -27,7 +27,7 @@ on: [push, pull_request]
 
 env:
   OPENSEARCH_VERSION: '1.0'
-  COMMON_UTILS_VERSION: '1.0'
+  COMMON_UTILS_VERSION: 'main'
 
 jobs:
   build:

notifications/notifications/src/main/kotlin/org/opensearch/notifications/send/SendMessageActionHelper.kt

@@ -230,7 +230,7 @@ object SendMessageActionHelper {
      * send message to custom webhook destination
      */
     private fun sendWebhookMessage(webhook: Webhook, message: MessageContent, eventStatus: EventStatus): EventStatus {
-        val destination = CustomWebhookDestination(webhook.url, webhook.headerParams, "POST")
+        val destination = CustomWebhookDestination(webhook.url, webhook.headerParams, webhook.method.tag)
         val status = sendMessageThroughSpi(destination, message)
         return eventStatus.copy(deliveryStatus = DeliveryStatus(status.statusCode.toString(), status.statusText))
     }

notifications/notifications/src/test/kotlin/org/opensearch/integtest/send/SendTestMessageRestHandlerIT.kt

@@ -146,6 +146,64 @@ internal class SendTestMessageRestHandlerIT : PluginRestTestCase() {
         Thread.sleep(100)
     }
 
+    @Suppress("EmptyFunctionBlock")
+    fun `test send custom webhook message`() {
+        // Create webhook notification config
+        val createRequestJsonString = """
+        {
+            "config":{
+                "name":"this is a sample config name",
+                "description":"this is a sample config description",
+                "config_type":"webhook",
+                "feature_list":[
+                    "index_management",
+                    "reports",
+                    "alerting"
+                ],
+                "is_enabled":true,
+                "webhook":{
+                    "url":"https://xxx.com/my-webhook@dev",
+                    "header_params": {
+                       "Content-type": "text/plain"
+                    }
+                }
+            }
+        }
+        """.trimIndent()
+        val createResponse = executeRequest(
+            RestRequest.Method.POST.name,
+            "$PLUGIN_BASE_URI/configs",
+            createRequestJsonString,
+            RestStatus.OK.status
+        )
+        val configId = createResponse.get("config_id").asString
+        Assert.assertNotNull(configId)
+        Thread.sleep(1000)
+
+        // send test message
+        val sendResponse = executeRequest(
+            RestRequest.Method.GET.name,
+            "$PLUGIN_BASE_URI/feature/test/$configId?feature=alerting",
+            "",
+            RestStatus.OK.status
+        )
+        val eventId = sendResponse.get("event_id").asString
+
+        val getEventResponse = executeRequest(
+            RestRequest.Method.GET.name,
+            "$PLUGIN_BASE_URI/events/$eventId",
+            "",
+            RestStatus.OK.status
+        )
+        val items = getEventResponse.get("event_list").asJsonArray
+        Assert.assertEquals(1, items.size())
+        val getResponseItem = items[0].asJsonObject
+        Assert.assertEquals(eventId, getResponseItem.get("event_id").asString)
+        Assert.assertEquals("", getResponseItem.get("tenant").asString)
+        Assert.assertNotNull(getResponseItem.get("event").asJsonObject)
+        Thread.sleep(100)
+    }
+
     @Suppress("EmptyFunctionBlock")
     fun `test send test smtp email message`() {
         val sampleSmtpAccount = SmtpAccount(

notifications/spi/src/main/kotlin/org/opensearch/notifications/spi/client/DestinationHttpClient.kt

@@ -45,6 +45,7 @@ import org.apache.http.util.EntityUtils
 import org.opensearch.common.xcontent.XContentFactory
 import org.opensearch.common.xcontent.XContentType
 import org.opensearch.notifications.spi.model.MessageContent
+import org.opensearch.notifications.spi.model.destination.ChimeDestination
 import org.opensearch.notifications.spi.model.destination.CustomWebhookDestination
 import org.opensearch.notifications.spi.model.destination.SlackDestination
 import org.opensearch.notifications.spi.model.destination.WebhookDestination
@@ -127,7 +128,7 @@ class DestinationHttpClient {
         var httpRequest: HttpRequestBase = HttpPost(destination.url)
 
         if (destination is CustomWebhookDestination) {
-            httpRequest = constructHttpRequest(destination.method)
+            httpRequest = constructHttpRequest(destination.method, destination.url)
             if (destination.headerParams.isEmpty()) {
                 // set default header
                 httpRequest.setHeader("Content-type", "application/json")
@@ -142,11 +143,11 @@ class DestinationHttpClient {
         return httpClient.execute(httpRequest)
     }
 
-    private fun constructHttpRequest(method: String): HttpRequestBase {
+    private fun constructHttpRequest(method: String, url: String): HttpRequestBase {
         return when (method) {
-            HttpPost.METHOD_NAME -> HttpPost()
-            HttpPut.METHOD_NAME -> HttpPut()
-            HttpPatch.METHOD_NAME -> HttpPatch()
+            HttpPost.METHOD_NAME -> HttpPost(url)
+            HttpPut.METHOD_NAME -> HttpPut(url)
+            HttpPatch.METHOD_NAME -> HttpPatch(url)
             else -> throw IllegalArgumentException(
                 "Invalid or empty method supplied. Only POST, PUT and PATCH are allowed"
             )
@@ -171,11 +172,20 @@ class DestinationHttpClient {
 
     fun buildRequestBody(destination: WebhookDestination, message: MessageContent): String {
         val builder = XContentFactory.contentBuilder(XContentType.JSON)
-        var keyName = "Content"
-        // Slack webhook request body has required "text" as key name https://api.slack.com/messaging/webhooks
-        if (destination is SlackDestination) keyName = "text"
+        val keyName = when (destination) {
+            // Slack webhook request body has required "text" as key name https://api.slack.com/messaging/webhooks
+            // Chime webhook request body has required "Content" as key name
+            // Customer webhook allows input as json or plain text, so we just return the message as it is
+            is SlackDestination -> "text"
+            is ChimeDestination -> "Content"
+            is CustomWebhookDestination -> return message.textDescription
+            else -> throw IllegalArgumentException(
+                "Invalid destination type is provided, Only Slack, Chime and CustomWebook are allowed"
+            )
+        }
+
         builder.startObject()
-            .field(keyName, message.buildWebhookMessage())
+            .field(keyName, message.buildMessageWithTitle())
             .endObject()
         return builder.string()
     }

notifications/spi/src/main/kotlin/org/opensearch/notifications/spi/model/MessageContent.kt

@@ -48,7 +48,7 @@ class MessageContent(
         require(!Strings.isNullOrEmpty(textDescription)) { "text message part is null or empty" }
     }
 
-    fun buildWebhookMessage(): String {
+    fun buildMessageWithTitle(): String {
         return "$title\n\n$textDescription"
     }
 }

notifications/spi/src/main/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpers.kt

@@ -49,7 +49,7 @@ fun validateEmail(email: String) {
 
 fun isValidUrl(urlString: String): Boolean {
     val url = URL(urlString) // throws MalformedURLException if URL is invalid
-    return ("https" == url.protocol) // Support only HTTPS. HTTP and other protocols not supported
+    return ("https" == url.protocol || "http" == url.protocol) // Support only http/https, other protocols not supported
 }
 
 fun isHostInDenylist(urlString: String, hostDenyList: List<String>): Boolean {

notifications/spi/src/test/kotlin/org/opensearch/notifications/spi/ChimeDestinationTests.kt

@@ -33,8 +33,9 @@ import org.apache.http.entity.StringEntity
 import org.apache.http.impl.client.CloseableHttpClient
 import org.apache.http.message.BasicStatusLine
 import org.easymock.EasyMock
-import org.junit.Test
+import org.junit.jupiter.api.Assertions
 import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.params.ParameterizedTest
 import org.junit.jupiter.params.provider.Arguments
@@ -64,7 +65,6 @@ internal class ChimeDestinationTests {
     }
 
     @Test
-    @Throws(Exception::class)
     fun `test chime message null entity response`() {
         val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)
 
@@ -102,7 +102,6 @@ internal class ChimeDestinationTests {
     }
 
     @Test
-    @Throws(Exception::class)
     fun `test chime message empty entity response`() {
         val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)
         val expectedWebhookResponse = DestinationMessageResponse(RestStatus.OK.status, "")
@@ -137,7 +136,6 @@ internal class ChimeDestinationTests {
     }
 
     @Test
-    @Throws(Exception::class)
     fun `test chime message non-empty entity response`() {
         val responseContent = "It worked!"
         val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)
@@ -172,14 +170,12 @@ internal class ChimeDestinationTests {
         assertEquals(expectedWebhookResponse.statusCode, actualChimeResponse.statusCode)
     }
 
-    @Test(expected = IllegalArgumentException::class)
-    fun testUrlMissingMessage() {
-        try {
+    @Test
+    fun `test url missing should throw IllegalArgumentException with message`() {
+        val exception = Assertions.assertThrows(IllegalArgumentException::class.java) {
             ChimeDestination("")
-        } catch (ex: Exception) {
-            assertEquals("url is null or empty", ex.message)
-            throw ex
         }
+        assertEquals("url is null or empty", exception.message)
     }
 
     @Test
@@ -189,14 +185,12 @@ internal class ChimeDestinationTests {
         }
     }
 
-    @Test(expected = IllegalArgumentException::class)
-    fun testContentMissingMessage() {
-        try {
+    @Test
+    fun `test content missing content should throw IllegalArgumentException`() {
+        val exception = Assertions.assertThrows(IllegalArgumentException::class.java) {
             MessageContent("title", "")
-        } catch (ex: Exception) {
-            assertEquals("text message part is null or empty", ex.message)
-            throw ex
         }
+        assertEquals("text message part is null or empty", exception.message)
     }
 
     @ParameterizedTest

notifications/spi/src/test/kotlin/org/opensearch/notifications/spi/CustomWebhookDestinationTests.kt

@@ -36,8 +36,9 @@ import org.apache.http.entity.StringEntity
 import org.apache.http.impl.client.CloseableHttpClient
 import org.apache.http.message.BasicStatusLine
 import org.easymock.EasyMock
-import org.junit.Test
+import org.junit.jupiter.api.Assertions
 import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.params.ParameterizedTest
 import org.junit.jupiter.params.provider.Arguments
@@ -116,7 +117,6 @@ internal class CustomWebhookDestinationTests {
 
     @ParameterizedTest(name = "method {0} should return corresponding type of Http request object {1}")
     @MethodSource("methodToHttpRequestType")
-    @Throws(Exception::class)
     fun `test custom webhook message empty entity response`(method: String, expectedHttpClass: Class<HttpUriRequest>) {
         val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)
         val expectedWebhookResponse = DestinationMessageResponse(RestStatus.OK.status, "")
@@ -156,7 +156,6 @@ internal class CustomWebhookDestinationTests {
 
     @ParameterizedTest(name = "method {0} should return corresponding type of Http request object {1}")
     @MethodSource("methodToHttpRequestType")
-    @Throws(Exception::class)
     fun `test custom webhook message non-empty entity response`(
         method: String,
         expectedHttpClass: Class<HttpUriRequest>
@@ -197,47 +196,49 @@ internal class CustomWebhookDestinationTests {
         assertEquals(expectedWebhookResponse.statusCode, actualCustomWebhookResponse.statusCode)
     }
 
-    @Test(expected = IllegalArgumentException::class)
+    @ParameterizedTest(name = "method {0} should return corresponding type of Http request object {1}")
+    @MethodSource("methodToHttpRequestType")
     fun `Test missing url will throw exception`(method: String) {
-        try {
+        val exception = Assertions.assertThrows(IllegalArgumentException::class.java) {
             CustomWebhookDestination("", mapOf("headerKey" to "headerValue"), method)
-        } catch (ex: Exception) {
-            assertEquals("url is null or empty", ex.message)
-            throw ex
         }
+        assertEquals("url is null or empty", exception.message)
     }
 
-    @Test
-    fun testUrlInvalidMessage(method: String) {
+    @ParameterizedTest(name = "method {0} should return corresponding type of Http request object {1}")
+    @MethodSource("methodToHttpRequestType")
+    fun `Custom webhook should throw exception if url is invalid`(method: String) {
         assertThrows<MalformedURLException> {
             CustomWebhookDestination("invalidUrl", mapOf("headerKey" to "headerValue"), method)
         }
     }
 
-    @Test(expected = IllegalArgumentException::class)
+    @ParameterizedTest(name = "method {0} should return corresponding type of Http request object {1}")
+    @MethodSource("methodToHttpRequestType")
+    fun `Custom webhook should throw exception if url protocol is not http or https`(method: String) {
+        val exception = Assertions.assertThrows(IllegalArgumentException::class.java) {
+            CustomWebhookDestination("ftp://abc/com", mapOf("headerKey" to "headerValue"), method)
+        }
+        assertEquals("Invalid URL or unsupported", exception.message)
+    }
+
+    @Test
     fun `Test invalid method type will throw exception`() {
-        try {
+        val exception = Assertions.assertThrows(IllegalArgumentException::class.java) {
             CustomWebhookDestination("https://abc/com", mapOf("headerKey" to "headerValue"), "GET")
-        } catch (ex: Exception) {
-            assertEquals("Invalid method supplied. Only POST, PUT and PATCH are allowed", ex.message)
-            throw ex
         }
+        assertEquals("Invalid method supplied. Only POST, PUT and PATCH are allowed", exception.message)
     }
 
-    @ParameterizedTest
-    @MethodSource("escapeSequenceToRaw")
-    fun `test build request body for custom webhook should have title included and prevent escape`(
-        escapeSequence: String,
-        rawString: String
-    ) {
+    @Test
+    fun `test build request body for custom webhook`() {
         val httpClient = DestinationHttpClient()
         val title = "test custom webhook"
-        val messageText = "line1${escapeSequence}line2"
+        val messageText = "{\"Customized Key\":\"some content\"}"
         val url = "https://abc/com"
-        val expectedRequestBody = """{"Content":"$title\n\nline1${rawString}line2"}"""
         val destination = CustomWebhookDestination(url, mapOf("headerKey" to "headerValue"), "POST")
         val message = MessageContent(title, messageText)
         val actualRequestBody = httpClient.buildRequestBody(destination, message)
-        assertEquals(expectedRequestBody, actualRequestBody)
+        assertEquals(messageText, actualRequestBody)
     }
 }

notifications/spi/src/test/kotlin/org/opensearch/notifications/spi/SlackDestinationTests.kt

@@ -33,8 +33,9 @@ import org.apache.http.entity.StringEntity
 import org.apache.http.impl.client.CloseableHttpClient
 import org.apache.http.message.BasicStatusLine
 import org.easymock.EasyMock
-import org.junit.Test
+import org.junit.jupiter.api.Assertions
 import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.params.ParameterizedTest
 import org.junit.jupiter.params.provider.Arguments
@@ -65,7 +66,6 @@ internal class SlackDestinationTests {
     }
 
     @Test
-    @Throws(Exception::class)
     fun `test Slack message null entity response`() {
         val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)
 
@@ -103,7 +103,6 @@ internal class SlackDestinationTests {
     }
 
     @Test
-    @Throws(Exception::class)
     fun `test Slack message empty entity response`() {
         val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)
         val expectedWebhookResponse = DestinationMessageResponse(RestStatus.OK.status, "")
@@ -138,7 +137,6 @@ internal class SlackDestinationTests {
     }
 
     @Test
-    @Throws(Exception::class)
     fun `test Slack message non-empty entity response`() {
         val responseContent = "It worked!"
         val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)
@@ -173,14 +171,12 @@ internal class SlackDestinationTests {
         assertEquals(expectedWebhookResponse.statusCode, actualSlackResponse.statusCode)
     }
 
-    @Test(expected = IllegalArgumentException::class)
-    fun testUrlMissingMessage() {
-        try {
+    @Test
+    fun `test url missing should throw IllegalArgumentException with message`() {
+        val exception = Assertions.assertThrows(IllegalArgumentException::class.java) {
             SlackDestination("")
-        } catch (ex: Exception) {
-            assertEquals("url is null or empty", ex.message)
-            throw ex
         }
+        assertEquals("url is null or empty", exception.message)
     }
 
     @Test