Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds webhook format check for Slack #814

Merged
merged 30 commits into from
Nov 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
0ff5ba0
Microsoft teams (#676)
zhichao-aws Aug 24, 2023
fbad722
Add microsoft teams validation error message (#746)
zhichao-aws Aug 29, 2023
dc36ba6
onboard system and hidden index (#742)
Hailong-am Aug 30, 2023
344a172
Updates demo certs used in integ tests (#756)
DarshitChanpura Sep 7, 2023
c1b9a46
Add 2.10.0 release notes (#755)
Hailong-am Sep 7, 2023
7dcca6e
bump bwc version to 2.11 (#763)
Hailong-am Sep 11, 2023
93aa732
Add 2.11 release notes (#774)
yuye-aws Oct 6, 2023
2c3cef0
Fix integration test failure by allowing direct access to system inde…
gaobinlong Oct 11, 2023
e5452cd
Re-enable detekt
Noir01 Oct 16, 2023
ff446bc
bump bwc version to 2.12 (#793)
Hailong-am Oct 16, 2023
ecac8ae
Update dependency org.json:json to v20231013 (#795)
gaobinlong Oct 17, 2023
445c1e7
Impove security plugin enabling check (#792)
Hailong-am Oct 19, 2023
fd4872d
Add github workflow to auto bump bwc version (#799)
Hailong-am Oct 20, 2023
c00a781
Replace the TestMailServer to GreenMail server (#801)
rachana-dani Oct 23, 2023
9342dfa
Onboard prod jenkins docker image to github actions (#809)
peterzhuamazon Nov 1, 2023
0be0e40
Added org.apache.logging.log4j:log4j-slf4j-impl to classpath (#791)
Noir01 Nov 2, 2023
ce88847
Added Slack webhook URL validation regex
Noir01 Nov 10, 2023
f8a6c61
Replaced wrongly formatted dummy Slack URL with properly formatted du…
Noir01 Nov 10, 2023
de78a74
Replaced more wrongly formatted dummy Slack URL with properly formatt…
Noir01 Nov 11, 2023
b4977cb
Replaced even more wrongly formatted dummy Slack URL with properly fo…
Noir01 Nov 11, 2023
f606953
Replace path of mock Slack URL with `sample_slack_url`
Noir01 Nov 15, 2023
bffe5d6
Remove slackId from domainIds
Noir01 Nov 15, 2023
b56f436
Replace wrongly formatted dummy Slack URL with properly formatted dum…
Noir01 Nov 15, 2023
2f85ea8
Merge branch 'main' into patch-2
Noir01 Nov 16, 2023
fe1189d
Add tests for wrong Slack URLs
Noir01 Nov 16, 2023
8c17f39
Add validation tests for Slack URL
Noir01 Nov 16, 2023
45823a6
Format
Noir01 Nov 16, 2023
84c17ed
GovSlack apps can use the slack-gov.com domain
Noir01 Nov 16, 2023
875048f
Merge remote-tracking branch 'origin/patch-2' into patch-2
Noir01 Nov 16, 2023
aa3b636
Add validation for gov-slack.com domain
Noir01 Nov 16, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ object ConfigIndexingActions {

@Suppress("UnusedPrivateMember")
private fun validateSlackConfig(slack: Slack, user: User?) {
// TODO: URL validation with rules
require(slack.url.contains(Regex("https://hooks\\.(?:gov-)?slack\\.com/services"))) {
"Wrong Slack url. Should contain \"hooks.slack.com/services/\" or \"hooks.gov-slack.com/services/\""
}
}

@Suppress("UnusedPrivateMember")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ fun getCreateNotificationRequestJsonString(
.joinToString("")
val configObjectString = when (configType) {
ConfigType.SLACK -> """
"slack":{"url":"https://slack.domain.com/sample_slack_url#$randomString"}
"slack":{"url":"https://hooks.slack.com/services/sample_slack_url#$randomString"}
""".trimIndent()
ConfigType.CHIME -> """
"chime":{"url":"https://chime.domain.com/sample_chime_url#$randomString"}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class SecurityNotificationIT : PluginRestTestCase() {
createUserWithCustomRole(user, password, NOTIFICATION_CREATE_CONFIG_ACCESS, "", ROLE_TO_PERMISSION_MAPPING[NOTIFICATION_CREATE_CONFIG_ACCESS])

// Create sample config request reference
val sampleSlack = Slack("https://domain.com/sample_slack_url#1234567890")
val sampleSlack = Slack("https://hooks.slack.com/services/sample_slack_url")
val referenceObject = NotificationConfig(
"this is a sample config name",
"this is a sample config description",
Expand Down Expand Up @@ -97,7 +97,7 @@ class SecurityNotificationIT : PluginRestTestCase() {
createUserWithCustomRole(user, password, NOTIFICATION_NO_ACCESS_ROLE, "", ROLE_TO_PERMISSION_MAPPING[NOTIFICATION_NO_ACCESS_ROLE])

// Create sample config request reference
val sampleSlack = Slack("https://domain.com/sample_slack_url#1234567890")
val sampleSlack = Slack("https://hooks.slack.com/services/sample_slack_url")
val referenceObject = NotificationConfig(
"this is a sample config name",
"this is a sample config description",
Expand Down Expand Up @@ -133,7 +133,7 @@ class SecurityNotificationIT : PluginRestTestCase() {
createUserWithCustomRole(user, password, NOTIFICATION_UPDATE_CONFIG_ACCESS, "", ROLE_TO_PERMISSION_MAPPING[NOTIFICATION_UPDATE_CONFIG_ACCESS])

// Create sample config request reference
val sampleSlack = Slack("https://domain.com/sample_slack_url#1234567890")
val sampleSlack = Slack("https://hooks.slack.com/services/sample_slack_url")
val referenceObject = NotificationConfig(
"this is a sample config name",
"this is a sample config description",
Expand Down Expand Up @@ -210,7 +210,7 @@ class SecurityNotificationIT : PluginRestTestCase() {
createUserWithCustomRole(user, password, NOTIFICATION_NO_ACCESS_ROLE, "", ROLE_TO_PERMISSION_MAPPING[NOTIFICATION_NO_ACCESS_ROLE])

// Create sample config request reference
val sampleSlack = Slack("https://domain.com/sample_slack_url#1234567890")
val sampleSlack = Slack("https://hooks.slack.com/services/sample_slack_url")
val referenceObject = NotificationConfig(
"this is a sample config name",
"this is a sample config description",
Expand Down Expand Up @@ -246,7 +246,7 @@ class SecurityNotificationIT : PluginRestTestCase() {
createUserWithCustomRole(user, password, NOTIFICATION_GET_CONFIG_ACCESS, "", ROLE_TO_PERMISSION_MAPPING[NOTIFICATION_GET_CONFIG_ACCESS])

// Create sample config request reference
val sampleSlack = Slack("https://domain.com/sample_slack_url#1234567890")
val sampleSlack = Slack("https://hooks.slack.com/services/sample_slack_url")
val referenceObject = NotificationConfig(
"this is a sample config name",
"this is a sample config description",
Expand Down Expand Up @@ -302,7 +302,7 @@ class SecurityNotificationIT : PluginRestTestCase() {
createUserWithCustomRole(user, password, NOTIFICATION_DELETE_CONFIG_ACCESS, "", ROLE_TO_PERMISSION_MAPPING[NOTIFICATION_DELETE_CONFIG_ACCESS])

// Create sample config request reference
val sampleSlack = Slack("https://domain.com/sample_slack_url#1234567890")
val sampleSlack = Slack("https://hooks.slack.com/services/sample_slack_url")
val referenceObject = NotificationConfig(
"this is a sample config name",
"this is a sample config description",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ class NotificationsBackwardsCompatibilityIT : PluginRestTestCase() {
"description": "This is a sample config description $configId",
"config_type": "slack",
"is_enabled": true,
"slack": { "url": "https://slack.domain.com/sample_slack_url#$configId" }
"slack": { "url": "https://hooks.slack.com/services/sample_slack_url#$configId" }
}
}
""".trimIndent()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class ChimeNotificationConfigCrudIT : PluginRestTestCase() {
"description":"${referenceObject.description}",
"config_type":"chime",
"is_enabled":${referenceObject.isEnabled},
"slack":{"url":"https://dummy.com"}
"slack":{"url":"https://hooks.slack.com/services/sample_slack_url"}
"chime":{"url":"${(referenceObject.configData as Chime).url}"}
}
}
Expand Down Expand Up @@ -190,7 +190,7 @@ class ChimeNotificationConfigCrudIT : PluginRestTestCase() {
"description":"this is a updated config description",
"config_type":"slack",
"is_enabled":"true",
"slack":{"url":"https://updated.domain.com/updated_slack_url#0987654321"}
"slack":{"url":"https://hooks.slack.com/services/sample_slack_url"}
}
}
""".trimIndent()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class CreateNotificationConfigIT : PluginRestTestCase() {

fun `test Create slack notification config`() {
// Create sample config request reference
val sampleSlack = Slack("https://domain.com/sample_slack_url#1234567890")
val sampleSlack = Slack("https://hooks.slack.com/services/sample_slack_url")
val referenceObject = NotificationConfig(
"this is a sample config name",
"this is a sample config description",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -907,7 +907,7 @@ class EmailNotificationConfigCrudIT : PluginRestTestCase() {
"description":"${smtpAccountConfig.description}",
"config_type":"smtp_account",
"is_enabled":${smtpAccountConfig.isEnabled},
"slack": {"url": "https://dummy.com"},
"slack": {"url": "https://hooks.slack.com/services/sample_slack_url"},
"smtp_account":{
"host":"${sampleSmtpAccount.host}",
"port":"${sampleSmtpAccount.port}",
Expand Down Expand Up @@ -948,7 +948,7 @@ class EmailNotificationConfigCrudIT : PluginRestTestCase() {
"description":"${emailConfig.description}",
"config_type":"email",
"is_enabled":${emailConfig.isEnabled},
"slack":{"url": "https://dummy.com"},
"slack":{"url": "https://hooks.slack.com/services/sample_slack_url"},
"email":{
"email_account_id":"${sampleEmail.emailAccountID}",
"default_recipients":[
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ class QueryNotificationConfigIT : PluginRestTestCase() {
val urlIds = setOf(slackId, chimeId, microsoftTeamsId, webhookId)
val recipientIds = setOf(emailGroupId)
val fromIds = setOf(emailGroupId, smtpAccountId)
val domainIds = setOf(slackId, chimeId, microsoftTeamsId, webhookId, smtpAccountId)
val domainIds = setOf(chimeId, microsoftTeamsId, webhookId, smtpAccountId)
Thread.sleep(1000)

// Get notification configs using query=slack
Expand Down Expand Up @@ -702,7 +702,7 @@ class QueryNotificationConfigIT : PluginRestTestCase() {
val urlIds = setOf(slackId, chimeId, microsoftTeamsId, webhookId)
val recipientIds = setOf(emailGroupId)
val fromIds = setOf(emailGroupId, smtpAccountId)
val domainIds = setOf(slackId, chimeId, microsoftTeamsId, webhookId, smtpAccountId)
val domainIds = setOf(chimeId, microsoftTeamsId, webhookId, smtpAccountId)
Thread.sleep(1000)

// Get notification configs using text_query=slack should not return any item
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,16 @@
package org.opensearch.integtest.config

import org.junit.Assert
import org.opensearch.client.Request
import org.opensearch.client.RequestOptions
import org.opensearch.client.ResponseException
import org.opensearch.commons.notifications.model.ConfigType
import org.opensearch.commons.notifications.model.NotificationConfig
import org.opensearch.commons.notifications.model.Slack
import org.opensearch.core.rest.RestStatus
import org.opensearch.integtest.PluginRestTestCase
import org.opensearch.integtest.getResponseBody
import org.opensearch.integtest.jsonify
import org.opensearch.notifications.NotificationPlugin.Companion.PLUGIN_BASE_URI
import org.opensearch.notifications.verifySingleConfigEquals
import org.opensearch.rest.RestRequest
Expand All @@ -19,7 +24,7 @@ class SlackNotificationConfigCrudIT : PluginRestTestCase() {

fun `test Create, Get, Update, Delete slack notification config using REST client`() {
// Create sample config request reference
val sampleSlack = Slack("https://domain.com/sample_slack_url#1234567890")
val sampleSlack = Slack("https://hooks.slack.com/services/sample_slack_url")
val referenceObject = NotificationConfig(
"this is a sample config name",
"this is a sample config description",
Expand Down Expand Up @@ -67,7 +72,7 @@ class SlackNotificationConfigCrudIT : PluginRestTestCase() {
Thread.sleep(100)

// Updated notification config object
val updatedSlack = Slack("https://updated.domain.com/updated_slack_url#0987654321")
val updatedSlack = Slack("https://hooks.slack.com/services/updated_slack_url")
val updatedObject = NotificationConfig(
"this is a updated config name",
"this is a updated config description",
Expand Down Expand Up @@ -126,7 +131,7 @@ class SlackNotificationConfigCrudIT : PluginRestTestCase() {

fun `test Bad Request for multiple config data for Slack using REST Client`() {
// Create sample config request reference
val sampleSlack = Slack("https://domain.com/sample_slack_url#1234567890")
val sampleSlack = Slack("https://hooks.slack.com/services/sample_slack_url")
val referenceObject = NotificationConfig(
"this is a sample config name",
"this is a sample config description",
Expand Down Expand Up @@ -155,4 +160,40 @@ class SlackNotificationConfigCrudIT : PluginRestTestCase() {
RestStatus.BAD_REQUEST.status
)
}

fun `test create config with wrong Slack url and get error text`() {
val sampleSlack = Slack("https://webhook.slack.com/services/sample_slack_url")
val referenceObject = NotificationConfig(
"this is a sample config name",
"this is a sample config description",
ConfigType.SLACK,
isEnabled = true,
configData = sampleSlack
)
val createRequestJsonString = """
{
"config":{
"name":"${referenceObject.name}",
"description":"${referenceObject.description}",
"config_type":"slack",
"is_enabled":${referenceObject.isEnabled},
"slack":{"url":"${(referenceObject.configData as Slack).url}"}
}
}
""".trimIndent()
val response = try {
val request = Request(RestRequest.Method.POST.name, "$PLUGIN_BASE_URI/configs")
request.setJsonEntity(createRequestJsonString)
val restOptionsBuilder = RequestOptions.DEFAULT.toBuilder()
restOptionsBuilder.addHeader("Content-Type", "application/json")
request.setOptions(restOptionsBuilder)
client().performRequest(request)
fail("Expected wrong Slack URL.")
} catch (exception: ResponseException) {
Assert.assertEquals(
"Wrong Slack url. Should contain \"hooks.slack.com/services/\" or \"hooks.gov-slack.com/services/\"",
jsonify(getResponseBody(exception.response))["error"].asJsonObject["reason"].asString
)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ class WebhookNotificationConfigCrudIT : PluginRestTestCase() {
"description":"${referenceObject.description}",
"config_type":"webhook",
"is_enabled":${referenceObject.isEnabled},
"slack":{"url":"https://dummy.com"}
"slack":{"url":"https://hooks.slack.com/services/sample_slack_url"}
"webhook":{"url":"${(referenceObject.configData as Webhook).url}"}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import org.junit.jupiter.api.BeforeAll
import org.junit.jupiter.api.Test
import org.opensearch.commons.authuser.User
import org.opensearch.commons.notifications.model.MicrosoftTeams
import org.opensearch.commons.notifications.model.Slack
import java.lang.reflect.Method
import kotlin.test.assertFails

Expand All @@ -28,8 +29,42 @@ class ConfigIndexingActionsTests {
assertFails { validateMicrosoftTeamsConfig.invoke(ConfigIndexingActions, microsoftTeams, user) }
}

@Test
fun `test validate slack`() {
val user = User()
var slack = Slack("https://hooks.slack.com/services/123456789/123456789/123456789")
validateSlackConfig.invoke(ConfigIndexingActions, slack, user)
slack = Slack("https://hooks.gov-slack.com/services/123456789/123456789/123456789")
validateSlackConfig.invoke(ConfigIndexingActions, slack, user)
slack = Slack("https://hooks.slack.com/services/samplesamplesamplesamplesamplesamplesamplesamplesample")
validateSlackConfig.invoke(ConfigIndexingActions, slack, user)
slack = Slack("https://hooks.gov-slack.com/services/samplesamplesamplesamplesamplesamplesamplesamplesample")
validateSlackConfig.invoke(ConfigIndexingActions, slack, user)
slack = Slack("http://hooks.slack.com/services/123456789/123456789/123456789/123456789")
assertFails { validateSlackConfig.invoke(ConfigIndexingActions, slack, user) }
slack = Slack("http://hooks.gov-slack.com/services/123456789/123456789/123456789/123456789")
assertFails { validateSlackConfig.invoke(ConfigIndexingActions, slack, user) }
slack = Slack("https://slack.com/services/123456789/123456789/123456789/123456789")
assertFails { validateSlackConfig.invoke(ConfigIndexingActions, slack, user) }
slack = Slack("https://gov-slack.com/services/123456789/123456789/123456789/123456789")
assertFails { validateSlackConfig.invoke(ConfigIndexingActions, slack, user) }
slack = Slack("https://hooks.slack.com/123456789/123456789/123456789/123456789/123456789")
assertFails { validateSlackConfig.invoke(ConfigIndexingActions, slack, user) }
slack = Slack("https://hooks.gov-slack.com/123456789/123456789/123456789/123456789/123456789")
assertFails { validateSlackConfig.invoke(ConfigIndexingActions, slack, user) }
slack = Slack("https://hook.slack.com/services/123456789/123456789/123456789/123456789/123456789")
assertFails { validateSlackConfig.invoke(ConfigIndexingActions, slack, user) }
slack = Slack("https://hook.gov-slack.com/services/123456789/123456789/123456789/123456789/123456789")
assertFails { validateSlackConfig.invoke(ConfigIndexingActions, slack, user) }
slack = Slack("https://hooks.slack.com/")
assertFails { validateSlackConfig.invoke(ConfigIndexingActions, slack, user) }
slack = Slack("https://hooks.gov-slack.com/")
assertFails { validateSlackConfig.invoke(ConfigIndexingActions, slack, user) }
}

companion object {
private lateinit var validateMicrosoftTeamsConfig: Method
private lateinit var validateSlackConfig: Method

@BeforeAll
@JvmStatic
Expand All @@ -38,8 +73,12 @@ class ConfigIndexingActionsTests {
validateMicrosoftTeamsConfig = ConfigIndexingActions::class.java.getDeclaredMethod(
"validateMicrosoftTeamsConfig", MicrosoftTeams::class.java, User::class.java
)
validateSlackConfig = ConfigIndexingActions::class.java.getDeclaredMethod(
"validateSlackConfig", Slack::class.java, User::class.java
)

validateMicrosoftTeamsConfig.isAccessible = true
validateSlackConfig.isAccessible = true
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ internal class NotificationConfigDocTests {
createdTimeMs,
listOf("br1", "br2", "br3")
)
val sampleSlack = Slack("https://domain.com/sample_url#1234567890")
val sampleSlack = Slack("https://hooks.slack.com/services/sample_slack_url")
val config = NotificationConfig(
"name",
"description",
Expand All @@ -47,7 +47,7 @@ internal class NotificationConfigDocTests {
createdTimeMs,
listOf("br1", "br2", "br3")
)
val sampleSlack = Slack("https://domain.com/sample_url#1234567890")
val sampleSlack = Slack("https://hooks.slack.com/services/sample_slack_url")
val config = NotificationConfig(
"name",
"description",
Expand All @@ -67,7 +67,7 @@ internal class NotificationConfigDocTests {
"description":"description",
"config_type":"slack",
"is_enabled":true,
"slack":{"url":"https://domain.com/sample_url#1234567890"}
"slack":{"url":"https://hooks.slack.com/services/sample_slack_url"}
},
"extra_field_1":["extra", "value"],
"extra_field_2":{"extra":"value"},
Expand Down
Loading