diff --git a/gradle.properties b/gradle.properties index da9cb37ca8..242da8a1af 100644 --- a/gradle.properties +++ b/gradle.properties @@ -8,7 +8,6 @@ testContainersVersion=1.15.3 okHttpVersion=4.5.0 resilience4jVersion=1.5.0 spinnakerGradleVersion=8.23.0 -slackSdkVersion=1.12.1 # Used to control whether to spin up docker to run liquibase before jooq buildingInDocker=false diff --git a/keel-notifications/keel-notifications.gradle b/keel-notifications/keel-notifications.gradle index 60fb587a85..2f3d26ce6c 100644 --- a/keel-notifications/keel-notifications.gradle +++ b/keel-notifications/keel-notifications.gradle @@ -6,9 +6,9 @@ dependencies { implementation(project(":keel-network")) implementation("org.springframework.boot:spring-boot-autoconfigure") - implementation("com.slack.api:slack-api-model-kotlin-extension:${slackSdkVersion}") - implementation("com.slack.api:slack-api-client-kotlin-extension:${slackSdkVersion}") - implementation("com.slack.api:bolt:${slackSdkVersion}") + implementation("com.slack.api:slack-api-model-kotlin-extension:1.4.1") + implementation("com.slack.api:slack-api-client-kotlin-extension:1.4.1") + implementation("com.slack.api:bolt:1.6.0") testImplementation("dev.minutest:minutest") testImplementation("io.strikt:strikt-core") diff --git a/keel-web/keel-web.gradle b/keel-web/keel-web.gradle index 7403f67ee7..a674fe9cf2 100644 --- a/keel-web/keel-web.gradle +++ b/keel-web/keel-web.gradle @@ -1,11 +1,9 @@ plugins { - id "application" - id "com.github.hauner.jarTest" version "1.0.1" + id("application") } -apply plugin: "com.netflix.dgs.codegen" -apply plugin: "io.spinnaker.package" -apply plugin: "com.github.hauner.jarTest" +apply(plugin: "com.netflix.dgs.codegen") +apply(plugin: "io.spinnaker.package") repositories { mavenCentral() // for graphql-java-extended-scalars @@ -48,7 +46,7 @@ dependencies { implementation("io.swagger.core.v3:swagger-annotations:2.1.2") implementation("org.apache.maven:maven-artifact:3.6.3") implementation("io.spinnaker.kork:kork-plugins") - implementation("com.slack.api:bolt-servlet:${slackSdkVersion}") + implementation("com.slack.api:bolt-servlet:1.6.0") implementation("com.graphql-java:graphql-java-extended-scalars:16.0.1") // DGS dependencies diff --git a/keel-web/src/main/kotlin/com/netflix/spinnaker/config/DefaultSecurityConfiguration.kt b/keel-web/src/main/kotlin/com/netflix/spinnaker/config/SecurityConfiguration.kt similarity index 92% rename from keel-web/src/main/kotlin/com/netflix/spinnaker/config/DefaultSecurityConfiguration.kt rename to keel-web/src/main/kotlin/com/netflix/spinnaker/config/SecurityConfiguration.kt index 84cd3bffdd..cb6a4bf54b 100644 --- a/keel-web/src/main/kotlin/com/netflix/spinnaker/config/DefaultSecurityConfiguration.kt +++ b/keel-web/src/main/kotlin/com/netflix/spinnaker/config/SecurityConfiguration.kt @@ -10,4 +10,4 @@ import org.springframework.context.annotation.Configuration @Configuration @ConditionalOnProperty(name = ["keel.security.custom"], havingValue = "false", matchIfMissing = true) @EnableFiatAutoConfig -class DefaultSecurityConfiguration +class SecurityConfiguration diff --git a/keel-web/src/main/kotlin/com/netflix/spinnaker/config/SlackRequestSecurityConfiguration.kt b/keel-web/src/main/kotlin/com/netflix/spinnaker/config/SlackRequestSecurityConfiguration.kt deleted file mode 100644 index c995fc93c3..0000000000 --- a/keel-web/src/main/kotlin/com/netflix/spinnaker/config/SlackRequestSecurityConfiguration.kt +++ /dev/null @@ -1,29 +0,0 @@ -package com.netflix.spinnaker.config - -import org.springframework.context.annotation.Configuration -import org.springframework.core.annotation.Order -import org.springframework.security.config.annotation.web.builders.HttpSecurity -import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity -import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter - -/** - * Disables several security-related filters for Slack callback requests, which are application/x-www-form-urlencoded - * and cause the filters to read the request body while reading parameters, which in turn causes the input stream - * to be closed before the request gets to the callback controller, appearing as an empty request body. - */ -@Configuration -@Order(1) -@EnableWebSecurity -class SlackRequestSecurityConfiguration : WebSecurityConfigurerAdapter() { - - override fun configure(http: HttpSecurity) { - http - .antMatcher("/slack/notifications/callbacks") - .authorizeRequests() - .anyRequest() - .permitAll() - .and() - .csrf() - .ignoringAntMatchers("/slack/notifications/callbacks") - } -} diff --git a/keel-web/src/main/kotlin/com/netflix/spinnaker/keel/filters/BodyCachingRequestWrapper.kt b/keel-web/src/main/kotlin/com/netflix/spinnaker/keel/filters/BodyCachingRequestWrapper.kt deleted file mode 100644 index 370d40b7c2..0000000000 --- a/keel-web/src/main/kotlin/com/netflix/spinnaker/keel/filters/BodyCachingRequestWrapper.kt +++ /dev/null @@ -1,55 +0,0 @@ -package com.netflix.spinnaker.keel.filters - -import org.springframework.util.StreamUtils -import java.io.BufferedReader -import java.io.ByteArrayInputStream -import java.io.InputStream -import java.io.InputStreamReader -import javax.servlet.ReadListener -import javax.servlet.ServletInputStream -import javax.servlet.http.HttpServletRequest -import javax.servlet.http.HttpServletRequestWrapper - - -/** - * Allows the request body to be consumed multiple times to accommodate for Slack POST requests which - * are encoded as application/x-www-form-urlencoded and whose body ends up being consumed before reaching - * the controller by various request filters in the chain. The filters read the body of the request when - * trying to read parameters, which in turn causes the [InputStream] associated with the request to be - * consumed (apparently in accordance with the Servlet spec), leading the body to be empty by the time it - * reaches our controller. - * - * Based on https://www.baeldung.com/spring-reading-httpservletrequest-multiple-times - */ -class BodyCachingRequestWrapper(request: HttpServletRequest) : HttpServletRequestWrapper(request) { - private val cachedBody: ByteArray = StreamUtils.copyToByteArray(request.inputStream) - - override fun getInputStream(): ServletInputStream { - return CachedBodyServletInputStream(cachedBody) - } - - override fun getReader(): BufferedReader { - val byteArrayInputStream = ByteArrayInputStream(cachedBody) - return BufferedReader(InputStreamReader(byteArrayInputStream)) - } - - class CachedBodyServletInputStream(cachedBody: ByteArray) : ServletInputStream() { - private val cachedBodyInputStream: InputStream = ByteArrayInputStream(cachedBody) - - override fun isFinished(): Boolean { - return cachedBodyInputStream.available() == 0 - } - - override fun isReady(): Boolean { - return true - } - - override fun setReadListener(readListener: ReadListener) { - throw UnsupportedOperationException() - } - - override fun read(): Int { - return cachedBodyInputStream.read() - } - } -} \ No newline at end of file diff --git a/keel-web/src/main/kotlin/com/netflix/spinnaker/keel/filters/RequestBodyCachingFilter.kt b/keel-web/src/main/kotlin/com/netflix/spinnaker/keel/filters/RequestBodyCachingFilter.kt deleted file mode 100644 index c205f125c1..0000000000 --- a/keel-web/src/main/kotlin/com/netflix/spinnaker/keel/filters/RequestBodyCachingFilter.kt +++ /dev/null @@ -1,46 +0,0 @@ -package com.netflix.spinnaker.keel.filters - -import org.slf4j.LoggerFactory -import org.springframework.core.Ordered.HIGHEST_PRECEDENCE -import org.springframework.core.annotation.Order -import org.springframework.http.HttpMethod -import org.springframework.stereotype.Component - -import org.springframework.web.filter.OncePerRequestFilter -import javax.servlet.FilterChain -import javax.servlet.annotation.WebFilter -import javax.servlet.http.HttpServletRequest -import javax.servlet.http.HttpServletResponse - -/** - * Wraps requests with the configured URL patterns with a [BodyCachingRequestWrapper] such that the request - * body can be read more than once through the filter chain and finally the controller. - * - * Primarily used to support Slack callback requests, which are form-encoded POSTs that cause the body - * to be consumed inadvertently by request filters. - */ -@Order(value = HIGHEST_PRECEDENCE) -@Component -@WebFilter( - filterName = "RequestBodyCachingFilter", - urlPatterns = ["/slack/notifications/callbacks"] -) -class RequestBodyCachingFilter : OncePerRequestFilter() { - private val log by lazy { LoggerFactory.getLogger(javaClass) } - - override fun doFilterInternal(request: HttpServletRequest, response: HttpServletResponse, chain: FilterChain) { - if (request.method == HttpMethod.POST.name) { - log.debug("Caching request body for ${request.method} ${request.requestURI}") - val wrapper = BodyCachingRequestWrapper(request) - chain.doFilter(wrapper, response) - } else { - chain.doFilter(request, response) - } - } - - // for some reason, the servlet container runs the filter on requests that don't match the URL patterns - // declared in @WebFilter above, so we need to override this function as well. - override fun shouldNotFilter(request: HttpServletRequest): Boolean { - return request.requestURI != "/slack/notifications/callbacks" - } -} diff --git a/keel-web/src/main/kotlin/com/netflix/spinnaker/keel/rest/SlackAppController.kt b/keel-web/src/main/kotlin/com/netflix/spinnaker/keel/rest/SlackAppController.kt new file mode 100644 index 0000000000..caee43434d --- /dev/null +++ b/keel-web/src/main/kotlin/com/netflix/spinnaker/keel/rest/SlackAppController.kt @@ -0,0 +1,57 @@ +package com.netflix.spinnaker.keel.rest + +import com.netflix.spinnaker.keel.notifications.slack.callbacks.CommitModalCallbackHandler +import com.netflix.spinnaker.keel.notifications.slack.callbacks.ManualJudgmentCallbackHandler +import com.slack.api.app_backend.interactive_components.payload.BlockActionPayload +import com.slack.api.bolt.App +import com.slack.api.bolt.context.builtin.ActionContext +import com.slack.api.bolt.request.builtin.BlockActionRequest +import com.slack.api.bolt.servlet.SlackAppServlet +import org.slf4j.LoggerFactory +import org.springframework.stereotype.Component +import javax.servlet.annotation.WebServlet + + +@Component +@WebServlet("/slack/notifications/callbacks") +/** + * New endpoint for the new slack integration. This will be called from gate directly instead of echo. + * We use Slack Bolt library (https://github.com/slackapi/java-slack-sdk/), which has a native support for handling callbacks from slack. + */ +class SlackAppController( + slackApp: App, + private val mjHandler: ManualJudgmentCallbackHandler, + private val commitModalCallbackHandler: CommitModalCallbackHandler, +) : SlackAppServlet(slackApp) { + + private val log by lazy { LoggerFactory.getLogger(javaClass) } + + init { + // The pattern here should match the action id field in the actual button. + // for example, for manual judgment notifications: constraintId:OVERRIDE_PASS:MANUAL_JUDGMENT + val actionIdPattern = "^(\\w+):(\\w+):(\\w+)".toPattern() + slackApp.blockAction(actionIdPattern) { req: BlockActionRequest, ctx: ActionContext -> + if (req.payload.notificationType == "MANUAL_JUDGMENT") { + log.info(logMessage("'manual judgment' button clicked", req)) + mjHandler.respondToButton(req, ctx) + } else if (req.payload.notificationType == "FULL_COMMIT_MODAL") { + log.info(logMessage("'show full commit' button clicked", req)) + commitModalCallbackHandler.openModal(req, ctx) + } else if (req.payload.actions.first().actionId == "button:url:mj-diff-link") { + log.info(logMessage("'see changes' button clicked", req)) + } else { + log.debug(logMessage("Unrecognized action", req)) + } + // we always need to acknowledge the button within 3 seconds + ctx.ack() + } + } + + fun logMessage(what: String, req: BlockActionRequest) = + "[slack interaction] $what by ${req.payload.user.username} (${req.payload.user.id}) " + + "in channel ${req.payload.channel.name} (${req.payload.channel.id})" + + //action id is consistent of 3 parts, where the last part is the type + val BlockActionPayload.notificationType + get() = actions.first().actionId.split(":").last() +} diff --git a/keel-web/src/main/kotlin/com/netflix/spinnaker/keel/rest/SlackAppServlet.kt b/keel-web/src/main/kotlin/com/netflix/spinnaker/keel/rest/SlackAppServlet.kt deleted file mode 100644 index f0c93ee8e2..0000000000 --- a/keel-web/src/main/kotlin/com/netflix/spinnaker/keel/rest/SlackAppServlet.kt +++ /dev/null @@ -1,71 +0,0 @@ -package com.netflix.spinnaker.keel.rest - -import com.netflix.spinnaker.keel.notifications.slack.callbacks.CommitModalCallbackHandler -import com.netflix.spinnaker.keel.notifications.slack.callbacks.ManualJudgmentCallbackHandler -import com.slack.api.bolt.App -import com.slack.api.bolt.context.builtin.ActionContext -import com.slack.api.bolt.request.builtin.BlockActionRequest -import com.slack.api.bolt.servlet.SlackAppServlet -import org.slf4j.LoggerFactory -import javax.servlet.annotation.WebServlet - - -/** - * A [WebServlet] that handles callbacks from Slack for interactive notifications. - * - * We use the Slack Bolt library (https://github.com/slackapi/java-slack-sdk/), which has native support for - * handling such callbacks. - */ -@WebServlet( - name = "SlackAppServlet", - urlPatterns = ["/slack/notifications/callbacks"] -) -class SlackAppServlet( - slackApp: App, - private val manualJudgementCallbackHandler: ManualJudgmentCallbackHandler, - private val commitModalCallbackHandler: CommitModalCallbackHandler, -) : SlackAppServlet(slackApp) { - - companion object { - private const val MANUAL_JUDGEMENT_ACTION = "MANUAL_JUDGMENT" - private const val SHOW_FULL_COMMIT_ACTION = "FULL_COMMIT_MODAL" - private const val SHOW_DIFF_ACTION = "mj-diff-link" - } - - private val log by lazy { LoggerFactory.getLogger(javaClass) } - - init { - // The pattern here should match the action id field in the actual button. - // For example, for manual judgment notifications: constraintId:OVERRIDE_PASS:MANUAL_JUDGMENT - val actionIdPattern = "^(\\w+):(\\w+):(\\w+)".toPattern() - slackApp.blockAction(actionIdPattern) { req: BlockActionRequest, ctx: ActionContext -> - when (req.notificationType) { - MANUAL_JUDGEMENT_ACTION -> { - log.debug(logMessage("manual judgment button clicked", req)) - manualJudgementCallbackHandler.respondToButton(req, ctx) - } - SHOW_FULL_COMMIT_ACTION -> { - log.debug(logMessage("show full commit button clicked", req)) - commitModalCallbackHandler.openModal(req, ctx) - } - SHOW_DIFF_ACTION -> { - log.debug(logMessage("'see changes' button clicked", req)) - } - else -> { - log.warn(logMessage("Unrecognized action", req)) - } - } - // we always need to acknowledge the button within 3 seconds - // TODO: should we move this to before the handler calls, since handling is asynchronous anyway? - ctx.ack() - } - } - - fun logMessage(what: String, req: BlockActionRequest) = - "[slack interaction] $what by ${req.payload?.user?.username} (${req.payload?.user?.id}) " + - "in channel ${req.payload?.channel?.name} (${req.payload?.channel?.id})" - - //action id is consistent of 3 parts, where the last part is the type - val BlockActionRequest.notificationType - get() = payload.actions.first().actionId.split(":").last() -} diff --git a/keel-web/src/test/kotlin/com/netflix/spinnaker/keel/rest/SlackAppServletTests.kt b/keel-web/src/test/kotlin/com/netflix/spinnaker/keel/rest/SlackAppServletTests.kt deleted file mode 100644 index c4945c03fc..0000000000 --- a/keel-web/src/test/kotlin/com/netflix/spinnaker/keel/rest/SlackAppServletTests.kt +++ /dev/null @@ -1,86 +0,0 @@ -package com.netflix.spinnaker.keel.rest - -import com.netflix.spinnaker.keel.KeelApplication -import com.netflix.spinnaker.keel.notifications.slack.callbacks.CommitModalCallbackHandler -import com.netflix.spinnaker.keel.notifications.slack.callbacks.ManualJudgmentCallbackHandler -import com.ninjasquad.springmockk.MockkBean -import io.mockk.every -import io.mockk.just -import io.mockk.runs -import io.mockk.verify -import org.junit.jupiter.api.BeforeEach -import org.junit.jupiter.api.Test -import org.springframework.beans.factory.annotation.Autowired -import org.springframework.boot.test.context.SpringBootTest -import org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT -import org.springframework.boot.test.web.client.TestRestTemplate -import org.springframework.http.HttpMethod.POST -import org.springframework.http.HttpStatus.OK -import org.springframework.http.MediaType.APPLICATION_FORM_URLENCODED -import org.springframework.http.MediaType.APPLICATION_JSON -import org.springframework.http.RequestEntity -import org.springframework.http.ResponseEntity -import strikt.api.expectThat -import strikt.assertions.isEqualTo -import java.net.URI -import java.net.URLEncoder -import java.nio.charset.Charset - -@SpringBootTest( - classes = [KeelApplication::class], - webEnvironment = RANDOM_PORT, - properties = [ "management.metrics.useNetflixConventions=false" ] -) -class SlackAppServletTests { - @Autowired - lateinit var restTemplate: TestRestTemplate - - @MockkBean - lateinit var manualJudgementHandler: ManualJudgmentCallbackHandler - - @MockkBean - lateinit var commitModalHandler: CommitModalCallbackHandler - - @BeforeEach - fun setup() { - every { - manualJudgementHandler.respondToButton(any(), any()) - } just runs - - every { - commitModalHandler.openModal(any(), any()) - } just runs - } - - @Test - fun `delegates handling of manual judgement callback`() { - val request = postSlackCallback("/slack/manual-judgement-payload.json") - val response: ResponseEntity = restTemplate.postForEntity(request.url, request, String::class.java) - - expectThat(response.statusCode).isEqualTo(OK) - - verify { - manualJudgementHandler.respondToButton(any(), any()) - } - } - - @Test - fun `delegates handling of commit modal callback`() { - val request = postSlackCallback("/slack/show-commit-payload.json") - val response: ResponseEntity = restTemplate.postForEntity(request.url, request, String::class.java) - - expectThat(response.statusCode).isEqualTo(OK) - - verify { - commitModalHandler.openModal(any(), any()) - } - } - - private fun postSlackCallback(resourceName: String) = RequestEntity - .method(POST, URI("/slack/notifications/callbacks")) - .accept(APPLICATION_JSON) - .contentType(APPLICATION_FORM_URLENCODED) - .body("payload=" + javaClass.getResource(resourceName).readText() - .let { URLEncoder.encode(it, Charset.forName("UTF-8")) } - ) -} \ No newline at end of file diff --git a/keel-web/src/test/resources/slack/manual-judgement-payload.json b/keel-web/src/test/resources/slack/manual-judgement-payload.json deleted file mode 100644 index 329b526d75..0000000000 --- a/keel-web/src/test/resources/slack/manual-judgement-payload.json +++ /dev/null @@ -1,49 +0,0 @@ -{ - "type": "block_actions", - "team": { - "id": "T9TK3CUKW", - "domain": "example" - }, - "user": { - "id": "UA8RXUSPL", - "username": "jtorrance", - "team_id": "T9TK3CUKW" - }, - "api_app_id": "AABA1ABCD", - "token": "9s8d9as89d8as9d8as989", - "container": { - "type": "message_attachment", - "message_ts": "1548261231.000200", - "attachment_id": 1, - "channel_id": "CBR2V3XEX", - "is_ephemeral": false, - "is_app_unfurl": false - }, - "trigger_id": "12321423423.333649436676.d8c1bb837935619ccad0f624c448ffb3", - "channel": { - "id": "CBR2V3XEX", - "name": "review-updates" - }, - "message": { - "bot_id": "BAH5CA16Z", - "type": "message", - "text": "Do you want to deploy version X into deployment Y of app A?", - "user": "UAJ2RU415", - "ts": "1548261231.000200" - }, - "response_url": "https://hooks.slack.com/actions/AABA1ABCD/1232321423432/D09sSasdasdAS9091209", - "actions": [ - { - "action_id": "constraintId:OVERRIDE_PASS:MANUAL_JUDGMENT", - "block_id": "=qXel", - "text": { - "type": "plain_text", - "text": "Manual judgement", - "emoji": true - }, - "value": "approve", - "type": "button", - "action_ts": "1548426417.840180" - } - ] -} \ No newline at end of file diff --git a/keel-web/src/test/resources/slack/show-commit-payload.json b/keel-web/src/test/resources/slack/show-commit-payload.json deleted file mode 100644 index c237971976..0000000000 --- a/keel-web/src/test/resources/slack/show-commit-payload.json +++ /dev/null @@ -1,49 +0,0 @@ -{ - "type": "block_actions", - "team": { - "id": "T9TK3CUKW", - "domain": "example" - }, - "user": { - "id": "UA8RXUSPL", - "username": "jtorrance", - "team_id": "T9TK3CUKW" - }, - "api_app_id": "AABA1ABCD", - "token": "9s8d9as89d8as9d8as989", - "container": { - "type": "message_attachment", - "message_ts": "1548261231.000200", - "attachment_id": 1, - "channel_id": "CBR2V3XEX", - "is_ephemeral": false, - "is_app_unfurl": false - }, - "trigger_id": "12321423423.333649436676.d8c1bb837935619ccad0f624c448ffb3", - "channel": { - "id": "CBR2V3XEX", - "name": "review-updates" - }, - "message": { - "bot_id": "BAH5CA16Z", - "type": "message", - "text": "Click the button for commit details", - "user": "UAJ2RU415", - "ts": "1548261231.000200" - }, - "response_url": "https://hooks.slack.com/actions/AABA1ABCD/1232321423432/D09sSasdasdAS9091209", - "actions": [ - { - "action_id": "one:two:FULL_COMMIT_MODAL", - "block_id": "=qXel", - "text": { - "type": "plain_text", - "text": "See details", - "emoji": true - }, - "value": "commit_details", - "type": "button", - "action_ts": "1548426417.840180" - } - ] -} \ No newline at end of file