Skip to content

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

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

Merged
merged 1 commit into from
Aug 16, 2020
Merged

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

merged 1 commit into from
Aug 16, 2020

Conversation

xavierbriand
Copy link
Contributor

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, from the author perspective, this deprecation was amalgamated with the (perceived) deprecation of the Web API itself.

According to Slack documentation 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–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.

@gmponos
Copy link
Contributor

gmponos commented Jun 9, 2020

IIRC it is possible to use the same webhook url to push messages to different channels..

@norkunas
Copy link
Contributor

norkunas commented Jun 9, 2020

IIRC it is possible to use the same webhook url to push messages to different channels..

Could you give an example? 🙂
In webhook url there is no reference to the recipient, only the generated parts. So when trying to configure recipient option via SlackOptions, it overrides the webhook part just with the recipient id.
So it is as @xavierbriand said, to change the recipient via that method you need to have many webhooks for different channels..

Also from the slack docs:

You cannot override the default channel (chosen by the user who installed your app), username, or icon when you're using Incoming Webhooks to post messages. Instead, these values will always inherit from the associated Slack app configuration.

Unless the transport would support posting like this:

POST https://hooks.slack.com/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX
Content-type: application/json
{
    "text": "Hello, world."
    "channel": "U12345678"
}

@gmponos
Copy link
Contributor

gmponos commented Jun 9, 2020

Well I can tell you that in a way it is not accurate.. or that it can be done... for instance check this..

image

I am using a different username (adding always a different UUID) for each one of my errors... although the quote of yours above says that incoming webhook can not change username

not sure if this is dependent of the webhook configuration.

@gmponos
Copy link
Contributor

gmponos commented Jun 9, 2020

This is the code I use https://github.com/gmponos/monolog-slack/blob/master/src/Formatter/SlackLineFormatter.php

Hope that it's not a bad practice to post another repo here :)

@norkunas
Copy link
Contributor

norkunas commented Jun 9, 2020

This is the code I use https://github.com/gmponos/monolog-slack/blob/master/src/Formatter/SlackLineFormatter.php

Hope that it's not a bad practice to post another repo here :)

As I see your formatter sets the channel in payload before calling the webhook, but that's what current slack notifier transport does not support :)

@fabpot
Copy link
Member

fabpot commented Aug 11, 2020

I won't dig into Slack docs again :) So I'm going to trust the community here. What should we do? Switch back to what I did in 5.0 (which AFAIR is more "powerful" than what we have in 5.1)? Or keep the 5.1 way of doing this? Either way is fine by me. Tell me :)

@xavierbriand
Copy link
Contributor Author

I won't dig into Slack docs again :) So I'm going to trust the community here. What should we do? Switch back to what I did in 5.0 (which AFAIR is more "powerful" than what we have in 5.1)? Or keep the 5.1 way of doing this? Either way is fine by me. Tell me :)

Sorry Fabien, I meant to deal with that PR, but... life.

IMHO Switching back to 5.0 is the way to go.

@xavierbriand
Copy link
Contributor Author

Rebased + implemented feedback from @norkunas and @stof.

@fabpot
Copy link
Member

fabpot commented Aug 16, 2020

Thank you @xavierbriand.

@vikbert
Copy link

vikbert commented Sep 21, 2020

hey, I am using currently symfony 5.1. Which should I use to push notification to slack channel, web API chat.postMessage or Web Hooks? I am so confused because of the solution in the book "symfony 5" written by Fabian

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants