From bb614c2159071d2f40dd78c59f4877b9cb2f3dbc Mon Sep 17 00:00:00 2001 From: Xavier Briand Date: Sun, 7 Jun 2020 20:32:16 -0400 Subject: [PATCH] [Notifier][Slack] Use Slack Web API chat.postMessage instead of WebHooks --- .../Notifier/Bridge/Slack/SlackTransport.php | 38 +++++++------- .../Bridge/Slack/SlackTransportFactory.php | 10 ++-- .../Slack/Tests/SlackTransportFactoryTest.php | 21 +++++--- .../Bridge/Slack/Tests/SlackTransportTest.php | 52 ++++++++++--------- 4 files changed, 65 insertions(+), 56 deletions(-) diff --git a/src/Symfony/Component/Notifier/Bridge/Slack/SlackTransport.php b/src/Symfony/Component/Notifier/Bridge/Slack/SlackTransport.php index 8a04c4e485..10d8f492ff 100644 --- a/src/Symfony/Component/Notifier/Bridge/Slack/SlackTransport.php +++ b/src/Symfony/Component/Notifier/Bridge/Slack/SlackTransport.php @@ -21,29 +21,23 @@ use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; use Symfony\Contracts\HttpClient\HttpClientInterface; /** - * Send messages via Slack using Slack Incoming Webhooks. - * * @author Fabien Potencier - * @author Daniel Stancu * * @internal * - * @see https://api.slack.com/messaging/webhooks - * * @experimental in 5.1 */ final class SlackTransport extends AbstractTransport { - protected const HOST = 'hooks.slack.com'; + protected const HOST = 'slack.com'; - private $id; + private $accessToken; + private $chatChannel; - /** - * @param string $id The hook id (anything after https://hooks.slack.com/services/) - */ - public function __construct(string $id, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null) + public function __construct(string $accessToken, string $channel = null, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null) { - $this->id = $id; + $this->accessToken = $accessToken; + $this->chatChannel = $channel; $this->client = $client; parent::__construct($client, $dispatcher); @@ -51,7 +45,7 @@ final class SlackTransport extends AbstractTransport public function __toString(): string { - return sprintf('slack://%s/%s', $this->getEndpoint(), $this->id); + return sprintf('slack://%s?channel=%s', $this->getEndpoint(), urlencode($this->chatChannel)); } public function supports(MessageInterface $message): bool @@ -59,6 +53,9 @@ final class SlackTransport extends AbstractTransport return $message instanceof ChatMessage && (null === $message->getOptions() || $message->getOptions() instanceof SlackOptions); } + /** + * @see https://api.slack.com/methods/chat.postMessage + */ protected function doSend(MessageInterface $message): SentMessage { if (!$message instanceof ChatMessage) { @@ -73,19 +70,22 @@ final class SlackTransport extends AbstractTransport } $options = $opts ? $opts->toArray() : []; - $id = $message->getRecipientId() ?: $this->id; + if (!isset($options['channel'])) { + $options['channel'] = $message->getRecipientId() ?: $this->chatChannel; + } $options['text'] = $message->getSubject(); - $response = $this->client->request('POST', sprintf('https://%s/services/%s', $this->getEndpoint(), $id), [ + $response = $this->client->request('POST', 'https://'.$this->getEndpoint().'/api/chat.postMessage', [ 'json' => array_filter($options), + 'auth_bearer' => $this->accessToken, ]); if (200 !== $response->getStatusCode()) { - throw new TransportException('Unable to post the Slack message: '.$response->getContent(false), $response); + throw new TransportException(sprintf('Unable to post the Slack message: "%s".', $response->getContent(false)), $response); } - $result = $response->getContent(false); - if ('ok' !== $result) { - throw new TransportException('Unable to post the Slack message: '.$result, $response); + $result = $response->toArray(false); + if (!$result['ok']) { + throw new TransportException(sprintf('Unable to post the Slack message: "%s".', $result['error']), $response); } return new SentMessage($message, (string) $this); diff --git a/src/Symfony/Component/Notifier/Bridge/Slack/SlackTransportFactory.php b/src/Symfony/Component/Notifier/Bridge/Slack/SlackTransportFactory.php index c3abdc9ba8..cc848c6cdd 100644 --- a/src/Symfony/Component/Notifier/Bridge/Slack/SlackTransportFactory.php +++ b/src/Symfony/Component/Notifier/Bridge/Slack/SlackTransportFactory.php @@ -11,7 +11,6 @@ namespace Symfony\Component\Notifier\Bridge\Slack; -use Symfony\Component\Notifier\Exception\IncompleteDsnException; use Symfony\Component\Notifier\Exception\UnsupportedSchemeException; use Symfony\Component\Notifier\Transport\AbstractTransportFactory; use Symfony\Component\Notifier\Transport\Dsn; @@ -30,16 +29,13 @@ final class SlackTransportFactory extends AbstractTransportFactory public function create(Dsn $dsn): TransportInterface { $scheme = $dsn->getScheme(); - $id = ltrim($dsn->getPath(), '/'); + $accessToken = $this->getUser($dsn); + $channel = $dsn->getOption('channel'); $host = 'default' === $dsn->getHost() ? null : $dsn->getHost(); $port = $dsn->getPort(); - if (!$id) { - throw new IncompleteDsnException('Missing path (maybe you haven\'t update the DSN when upgrading from 5.0).', $dsn->getOriginalDsn()); - } - if ('slack' === $scheme) { - return (new SlackTransport($id, $this->client, $this->dispatcher))->setHost($host)->setPort($port); + return (new SlackTransport($accessToken, $channel, $this->client, $this->dispatcher))->setHost($host)->setPort($port); } throw new UnsupportedSchemeException($dsn, 'slack', $this->getSupportedSchemes()); diff --git a/src/Symfony/Component/Notifier/Bridge/Slack/Tests/SlackTransportFactoryTest.php b/src/Symfony/Component/Notifier/Bridge/Slack/Tests/SlackTransportFactoryTest.php index c33d49fa0c..afd77200ba 100644 --- a/src/Symfony/Component/Notifier/Bridge/Slack/Tests/SlackTransportFactoryTest.php +++ b/src/Symfony/Component/Notifier/Bridge/Slack/Tests/SlackTransportFactoryTest.php @@ -13,6 +13,7 @@ namespace Symfony\Component\Notifier\Bridge\Slack\Tests; use PHPUnit\Framework\TestCase; use Symfony\Component\Notifier\Bridge\Slack\SlackTransportFactory; +use Symfony\Component\Notifier\Exception\IncompleteDsnException; use Symfony\Component\Notifier\Exception\UnsupportedSchemeException; use Symfony\Component\Notifier\Transport\Dsn; @@ -23,18 +24,26 @@ final class SlackTransportFactoryTest extends TestCase $factory = new SlackTransportFactory(); $host = 'testHost'; - $path = 'testPath'; - $transport = $factory->create(Dsn::fromString(sprintf('slack://%s/%s', $host, $path))); + $channel = 'testChannel'; + $transport = $factory->create(Dsn::fromString(sprintf('slack://testUser@%s/?channel=%s', $host, $channel))); - $this->assertSame(sprintf('slack://%s/%s', $host, $path), (string) $transport); + $this->assertSame(sprintf('slack://%s?channel=%s', $host, $channel), (string) $transport); + } + + public function testCreateWithNoTokenThrowsMalformed(): void + { + $factory = new SlackTransportFactory(); + + $this->expectException(IncompleteDsnException::class); + $factory->create(Dsn::fromString(sprintf('slack://%s/?channel=%s', 'testHost', 'testChannel'))); } public function testSupportsSlackScheme(): void { $factory = new SlackTransportFactory(); - $this->assertTrue($factory->supports(Dsn::fromString('slack://host/path'))); - $this->assertFalse($factory->supports(Dsn::fromString('somethingElse://host/path'))); + $this->assertTrue($factory->supports(Dsn::fromString('slack://host/?channel=testChannel'))); + $this->assertFalse($factory->supports(Dsn::fromString('somethingElse://host/?channel=testChannel'))); } public function testNonSlackSchemeThrows(): void @@ -43,6 +52,6 @@ final class SlackTransportFactoryTest extends TestCase $this->expectException(UnsupportedSchemeException::class); - $factory->create(Dsn::fromString('somethingElse://host/path')); + $factory->create(Dsn::fromString('somethingElse://user:pwd@host/?channel=testChannel')); } } diff --git a/src/Symfony/Component/Notifier/Bridge/Slack/Tests/SlackTransportTest.php b/src/Symfony/Component/Notifier/Bridge/Slack/Tests/SlackTransportTest.php index 100832efdd..45e76c0fe1 100644 --- a/src/Symfony/Component/Notifier/Bridge/Slack/Tests/SlackTransportTest.php +++ b/src/Symfony/Component/Notifier/Bridge/Slack/Tests/SlackTransportTest.php @@ -29,17 +29,17 @@ final class SlackTransportTest extends TestCase public function testToStringContainsProperties(): void { $host = 'testHost'; - $path = 'testPath'; + $channel = 'test Channel'; // invalid channel name to test url encoding of the channel - $transport = new SlackTransport($path, $this->createMock(HttpClientInterface::class)); + $transport = new SlackTransport('testToken', $channel, $this->createMock(HttpClientInterface::class)); $transport->setHost('testHost'); - $this->assertSame(sprintf('slack://%s/%s', $host, $path), (string) $transport); + $this->assertSame(sprintf('slack://%s?channel=%s', $host, urlencode($channel)), (string) $transport); } public function testSupportsChatMessage(): void { - $transport = new SlackTransport('testPath', $this->createMock(HttpClientInterface::class)); + $transport = new SlackTransport('testToken', 'testChannel', $this->createMock(HttpClientInterface::class)); $this->assertTrue($transport->supports(new ChatMessage('testChatMessage'))); $this->assertFalse($transport->supports($this->createMock(MessageInterface::class))); @@ -49,7 +49,7 @@ final class SlackTransportTest extends TestCase { $this->expectException(LogicException::class); - $transport = new SlackTransport('testPath', $this->createMock(HttpClientInterface::class)); + $transport = new SlackTransport('testToken', 'testChannel', $this->createMock(HttpClientInterface::class)); $transport->send($this->createMock(MessageInterface::class)); } @@ -70,7 +70,7 @@ final class SlackTransportTest extends TestCase return $response; }); - $transport = new SlackTransport('testPath', $client); + $transport = new SlackTransport('testToken', 'testChannel', $client); $transport->send(new ChatMessage('testMessage')); } @@ -78,7 +78,7 @@ final class SlackTransportTest extends TestCase public function testSendWithErrorResponseThrows(): void { $this->expectException(TransportException::class); - $this->expectExceptionMessage('testErrorCode'); + $this->expectExceptionMessageRegExp('/testErrorCode/'); $response = $this->createMock(ResponseInterface::class); $response->expects($this->exactly(2)) @@ -87,20 +87,21 @@ final class SlackTransportTest extends TestCase $response->expects($this->once()) ->method('getContent') - ->willReturn('testErrorCode'); + ->willReturn(json_encode(['error' => 'testErrorCode'])); $client = new MockHttpClient(static function () use ($response): ResponseInterface { return $response; }); - $transport = new SlackTransport('testPath', $client); + $transport = new SlackTransport('testToken', 'testChannel', $client); $transport->send(new ChatMessage('testMessage')); } public function testSendWithOptions(): void { - $path = 'testPath'; + $token = 'testToken'; + $channel = 'testChannel'; $message = 'testMessage'; $response = $this->createMock(ResponseInterface::class); @@ -111,24 +112,25 @@ final class SlackTransportTest extends TestCase $response->expects($this->once()) ->method('getContent') - ->willReturn('ok'); + ->willReturn(json_encode(['ok' => true])); - $expectedBody = json_encode(['text' => $message]); + $expectedBody = json_encode(['channel' => $channel, 'text' => $message]); $client = new MockHttpClient(function (string $method, string $url, array $options = []) use ($response, $expectedBody): ResponseInterface { - $this->assertSame($expectedBody, $options['body']); + $this->assertJsonStringEqualsJsonString($expectedBody, $options['body']); return $response; }); - $transport = new SlackTransport($path, $client); + $transport = new SlackTransport($token, $channel, $client); $transport->send(new ChatMessage('testMessage')); } public function testSendWithNotification(): void { - $host = 'testHost'; + $token = 'testToken'; + $channel = 'testChannel'; $message = 'testMessage'; $response = $this->createMock(ResponseInterface::class); @@ -139,7 +141,7 @@ final class SlackTransportTest extends TestCase $response->expects($this->once()) ->method('getContent') - ->willReturn('ok'); + ->willReturn(json_encode(['ok' => true])); $notification = new Notification($message); $chatMessage = ChatMessage::fromNotification($notification); @@ -147,16 +149,17 @@ final class SlackTransportTest extends TestCase $expectedBody = json_encode([ 'blocks' => $options->toArray()['blocks'], + 'channel' => $channel, 'text' => $message, ]); $client = new MockHttpClient(function (string $method, string $url, array $options = []) use ($response, $expectedBody): ResponseInterface { - $this->assertSame($expectedBody, $options['body']); + $this->assertJsonStringEqualsJsonString($expectedBody, $options['body']); return $response; }); - $transport = new SlackTransport($host, $client); + $transport = new SlackTransport($token, $channel, $client); $transport->send($chatMessage); } @@ -169,14 +172,15 @@ final class SlackTransportTest extends TestCase return $this->createMock(ResponseInterface::class); }); - $transport = new SlackTransport('testHost', $client); + $transport = new SlackTransport('testToken', 'testChannel', $client); $transport->send(new ChatMessage('testMessage', $this->createMock(MessageOptionsInterface::class))); } public function testSendWith200ResponseButNotOk(): void { - $host = 'testChannel'; + $token = 'testToken'; + $channel = 'testChannel'; $message = 'testMessage'; $this->expectException(TransportException::class); @@ -189,17 +193,17 @@ final class SlackTransportTest extends TestCase $response->expects($this->once()) ->method('getContent') - ->willReturn('testErrorCode'); + ->willReturn(json_encode(['ok' => false, 'error' => 'testErrorCode'])); - $expectedBody = json_encode(['text' => $message]); + $expectedBody = json_encode(['channel' => $channel, 'text' => $message]); $client = new MockHttpClient(function (string $method, string $url, array $options = []) use ($response, $expectedBody): ResponseInterface { - $this->assertSame($expectedBody, $options['body']); + $this->assertJsonStringEqualsJsonString($expectedBody, $options['body']); return $response; }); - $transport = new SlackTransport($host, $client); + $transport = new SlackTransport($token, $channel, $client); $transport->send(new ChatMessage('testMessage')); }