Skip to content

Commit

Permalink
feature #39606 [Notifier] [Slack] Validate token syntax (OskarStark)
Browse files Browse the repository at this point in the history
This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[Notifier] [Slack] Validate token syntax

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | ---
| License       | MIT
| Doc PR        | -

This PR follows #39560

@odolbeau @malteschlueter @norkunas @fabpot can you confirm all your tokens start with `xox`?

_From the Slack documentation:_
* Bot user token strings begin with `xoxb-`
* User token strings begin with `xoxp-`
* Workspace access token strings begin with `xoxa-2`

Commits
-------

59f29c5 [Notifier] [Slack] Validate token syntax
  • Loading branch information
fabpot committed Dec 28, 2020
2 parents 4a053e5 + 59f29c5 commit b6fdd6d
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 11 deletions.
1 change: 1 addition & 0 deletions src/Symfony/Component/Notifier/Bridge/Slack/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ CHANGELOG
* The bridge is not marked as `@experimental` anymore
* Check for maximum number of buttons in Slack action block
* Add HeaderBlock
* Slack access tokens needs to start with "xox" (see https://api.slack.com/authentication/token-types)

5.2.0
-----
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\Notifier\Bridge\Slack;

use Symfony\Component\Notifier\Exception\InvalidArgumentException;
use Symfony\Component\Notifier\Exception\LogicException;
use Symfony\Component\Notifier\Exception\TransportException;
use Symfony\Component\Notifier\Exception\UnsupportedMessageTypeException;
Expand All @@ -33,6 +34,10 @@ final class SlackTransport extends AbstractTransport

public function __construct(string $accessToken, string $channel = null, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)
{
if (!preg_match('/^xox(b-|p-|a-2)/', $accessToken)) {
throw new InvalidArgumentException('A valid Slack token needs to start with "xoxb-", "xoxp-" or "xoxa-2". See https://api.slack.com/authentication/token-types for further information.');
}

$this->accessToken = $accessToken;
$this->chatChannel = $channel;
$this->client = $client;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public function testCreateWithDsn()
{
$factory = $this->createFactory();

$transport = $factory->create(Dsn::fromString('slack://testUser@host.test/?channel=testChannel'));
$transport = $factory->create(Dsn::fromString('slack://xoxb-TestUser@host.test/?channel=testChannel'));

$this->assertSame('slack://host.test?channel=testChannel', (string) $transport);
}
Expand All @@ -33,7 +33,7 @@ public function testCreateWithDsnWithoutPath()
{
$factory = $this->createFactory();

$transport = $factory->create(Dsn::fromString('slack://testUser@host.test?channel=testChannel'));
$transport = $factory->create(Dsn::fromString('slack://xoxb-TestUser@host.test?channel=testChannel'));

$this->assertSame('slack://host.test?channel=testChannel', (string) $transport);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Symfony\Component\HttpClient\MockHttpClient;
use Symfony\Component\Notifier\Bridge\Slack\SlackOptions;
use Symfony\Component\Notifier\Bridge\Slack\SlackTransport;
use Symfony\Component\Notifier\Exception\InvalidArgumentException;
use Symfony\Component\Notifier\Exception\LogicException;
use Symfony\Component\Notifier\Exception\TransportException;
use Symfony\Component\Notifier\Exception\UnsupportedMessageTypeException;
Expand All @@ -31,23 +32,31 @@ public function testToStringContainsProperties()
{
$channel = 'test Channel'; // invalid channel name to test url encoding of the channel

$transport = new SlackTransport('testToken', $channel, $this->createMock(HttpClientInterface::class));
$transport = new SlackTransport('xoxb-TestToken', $channel, $this->createMock(HttpClientInterface::class));
$transport->setHost('host.test');

$this->assertSame('slack://host.test?channel=test+Channel', (string) $transport);
}

public function testInstatiatingWithAnInvalidSlackTokenThrowsInvalidArgumentException()
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('A valid Slack token needs to start with "xoxb-", "xoxp-" or "xoxa-2". See https://api.slack.com/authentication/token-types for further information.');

new SlackTransport('token', 'testChannel', $this->createMock(HttpClientInterface::class));
}

public function testSupportsChatMessage()
{
$transport = new SlackTransport('testToken', 'testChannel', $this->createMock(HttpClientInterface::class));
$transport = new SlackTransport('xoxb-TestToken', 'testChannel', $this->createMock(HttpClientInterface::class));

$this->assertTrue($transport->supports(new ChatMessage('testChatMessage')));
$this->assertFalse($transport->supports($this->createMock(MessageInterface::class)));
}

public function testSendNonChatMessageThrowsLogicException()
{
$transport = new SlackTransport('testToken', 'testChannel', $this->createMock(HttpClientInterface::class));
$transport = new SlackTransport('xoxb-TestToken', 'testChannel', $this->createMock(HttpClientInterface::class));

$this->expectException(UnsupportedMessageTypeException::class);

Expand All @@ -70,7 +79,7 @@ public function testSendWithEmptyArrayResponseThrows()
return $response;
});

$transport = new SlackTransport('testToken', 'testChannel', $client);
$transport = new SlackTransport('xoxb-TestToken', 'testChannel', $client);

$transport->send(new ChatMessage('testMessage'));
}
Expand All @@ -93,14 +102,14 @@ public function testSendWithErrorResponseThrows()
return $response;
});

$transport = new SlackTransport('testToken', 'testChannel', $client);
$transport = new SlackTransport('xoxb-TestToken', 'testChannel', $client);

$transport->send(new ChatMessage('testMessage'));
}

public function testSendWithOptions()
{
$token = 'testToken';
$token = 'xoxb-TestToken';
$channel = 'testChannel';
$message = 'testMessage';

Expand Down Expand Up @@ -129,7 +138,7 @@ public function testSendWithOptions()

public function testSendWithNotification()
{
$token = 'testToken';
$token = 'xoxb-TestToken';
$channel = 'testChannel';
$message = 'testMessage';

Expand Down Expand Up @@ -172,14 +181,14 @@ public function testSendWithInvalidOptions()
return $this->createMock(ResponseInterface::class);
});

$transport = new SlackTransport('testToken', 'testChannel', $client);
$transport = new SlackTransport('xoxb-TestToken', 'testChannel', $client);

$transport->send(new ChatMessage('testMessage', $this->createMock(MessageOptionsInterface::class)));
}

public function testSendWith200ResponseButNotOk()
{
$token = 'testToken';
$token = 'xoxb-TestToken';
$channel = 'testChannel';
$message = 'testMessage';

Expand Down

2 comments on commit b6fdd6d

@javiereguiluz
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is well intended, but in my opinion we should revert this change. For two reasons:

  1. This change means that we will have to be permanently in sync with the Slack token syntax rules. We will receive bug reports when our validation is outdated, then we'll have to change our code, release a new version, etc. Why waste our precious time in this?

  2. This hides any potential useful message that Slack may include when using a wrong token. We include https://api.slack.com/authentication/token-types in our error message ... but that resource may not be always the best or Slack may decide to include further resources in their error message.

So, I would only validate the syntax of universal standards (email addresses, IP addresses, etc.) and I wouldn't try to do the same with proprietary tokens. Thanks!

@derrabus
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javiereguiluz I have mixed feelings about this feature as well, see my comment on the PR: #39606 (comment)

Please sign in to comment.