Skip to content
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

[Notifier] [Slack] [DX] Improve the DX #39560

Closed
OskarStark opened this issue Dec 18, 2020 · 8 comments
Closed

[Notifier] [Slack] [DX] Improve the DX #39560

OskarStark opened this issue Dec 18, 2020 · 8 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Notifier Stalled

Comments

@OskarStark
Copy link
Contributor

OskarStark commented Dec 18, 2020

Improvements for symfony/slack-notifier

The generic channel option

The problem:
Right now you can provide a channel= option, and you can pass nearly everything... 🤔

  • CHANNEL is a channel, private group, or IM channel to send message to, it can be an encoded ID, or a name.

This is the full DSN: SLACK_DSN=slack://TOKEN@default?channel=CHANNEL

I tried a lot to figure out WHAT you need to pass exactly:

  • If you want to wrote to the channel #support -> ?channel=support
  • If you want to wrote to the user @OskarStark -> ?channel=@OskarStark
  • If you want to wrote to the user @OskarStark you can also use the UserId, which can be found in your profile -> ?channel=U68xxxx 🤔 ✅
    ss

So far this looks ok, but there are some pitfalls we can avoid. One would think he can just use:

  • ?channel=#supportNO # sign allowed 😮
  • ?channel=OskarStarkNO, @ sign needed 😮
  • ?channel=@U68xxxxNO @ sign allowed 😮

My proposal:
A new user= (or handle=) and user_id= parameter.
We can now validate, that a channel must not start with #, a user must start with @ and a user_id must not start with @ (maybe it also has a dedicated length which can be taken into account).

So valid DSN would be:

  • slack://TOKEN@default?channel=support
  • slack://TOKEN@default?user=@OskarStark // slack://TOKEN@default?handle=@OskarStark
  • slack://TOKEN@default?user_id=@U68xxxx

The correct token - PR #39606

The Problem:
Because we switched back and forth from a token to a webhook_id, which can be considered a "token" too, it could be hard to find out, if you are using the correct token.

My proposal:
Let's validate the token syntax in the transport and give a clear error message.

Slack has a clear syntax for their tokens which makes us able to validate the syntax before we perform a request.

  • Bot user token strings begin with xoxb-
  • User token strings begin with xoxp-
  • Workspace access token strings begin with xoxa-2

Questions

  1. What type of token do you use?
  2. Are you using any other ids, channel-names etc. ?

cc @malteschlueter as we both had some trouble in the past

@carsonbot carsonbot added Notifier DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Dec 18, 2020
@norkunas
Copy link
Contributor

?channel=#channel works if you escape the #

@OskarStark
Copy link
Contributor Author

?channel=#channel works if you escape the #

Oh nice to know 😄

@derrabus
Copy link
Member

If we have a dedicated user option, the @ character feels redundant.

- slack://TOKEN@default?user=@OskarStark
+ slack://TOKEN@default?user=OskarStark

@Jibbarth
Copy link
Contributor

AFAIK, slack deprecate webApi for channels.*, groups.*, im.*(direct mesages) etc, in favor of an unified api that handle all of them.

Maybe we should follow the same move, and instead of having multiple query params available, having only conversation

DSN could be :

  • slack://TOKEN@default?conversation=#support
  • slack://TOKEN@default?conversation=@OskarStark
  • slack://TOKEN@default?conversation=U68XXXX

@OskarStark
Copy link
Contributor Author

I like your idea a lot @Jibbarth 👍

@OskarStark
Copy link
Contributor Author

CleanShot 2020-12-18 at 15 38 41

I would then create a new symfony/slack-conversations-notifier instead of reworking the current slack notifier and start dealing with different DSNs

fabpot added a commit that referenced this issue Dec 28, 2020
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
symfony-splitter pushed a commit to symfony/slack-notifier that referenced this issue Dec 28, 2020
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 symfony/symfony#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
-------

59f29c592b [Notifier] [Slack] Validate token syntax
@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@OskarStark
Copy link
Contributor Author

I don't have so much time right now, lets close this for now 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Notifier Stalled
Projects
None yet
Development

No branches or pull requests

5 participants