feature #37138 [Notifier][Slack] Use Slack Web API chat.postMessage instead of WebHooks (xavierbriand)

This PR was squashed before being merged into the 5.2-dev branch.

Discussion
----------

[Notifier][Slack] Use Slack Web API chat.postMessage instead of WebHooks

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

**TL;DR**: Revert changes introduced in 5.1 by #35828: Slack's Web API is much more flexible and easier to operate than Slack WebHooks are.

---

### Context

A valid choice was made to switch from Slack's Web API to Slack's WebHooks for a wrong reason: according to #35828, this was in reaction to the deprecation of Slack's Legacy Tokens.

The author cites:
> Legacy tokens are a deprecated method of generating tokens for testing and development.

[As far as I was able to understand](https://github.com/symfony/symfony/pull/35828#issuecomment-639465018
), from the author perspective, this deprecation was amalgamated with the (perceived) deprecation of the Web API itself.

According to [Slack documentation](https://api.slack.com/legacy/custom-integrations/legacy-tokens) cited by the author:

> If you were using a legacy token to make calls with the Web API, you'll need to generate a new one for your new Slack app.

**So Slack changing its credentials' policy didn't warrant changing how to use Slack's Web API.**

---

### Why Web API > WebHook?

While using Slack WebHooks as the underlying transport method for notification isn't inherently a bad choice, it does–on top of [the reasons cited by the author](#35828)–come with a major limitation: the need to setup (manually or programmatically) a Webhook per channel/recipient.

The Web API, with it's underlying OAuth Access Token, is much more flexible and allows for more diverse use case. Notifications can be sent on behalf of users for instance. Slack can also limit the use of the access tokens to a list of IP addresses and ranges.

**E.g. I want to notify the person who triggers a CD pipeline if the latter fails.** \
Assuming a team of 10 developer, using Slack WebHooks, I would need to setup 10 WebHooks, and manages as many secret in my Symfony app. Furthermore, I would need to create new WebHook each time a new member were to join the team. \
With the Web API, I would only need to manage a single access token for the whole Slack workspace, regardless of who as access to this workspace.

Slack's Web API is much more flexible and easier to operate than Slack WebHooks are.

Commits
-------

bb614c2159 [Notifier][Slack] Use Slack Web API chat.postMessage instead of WebHooks

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 <fabien@symfony.com>
- * @author Daniel Stancu <birkof@birkof.ro>
  *
  * @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);

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());

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'));
     }
 }

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'));
     }