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

ref(slack): Remove slack-v2 creds and usage #23587

Merged
merged 5 commits into from
Feb 10, 2021
Merged

Conversation

MeredithAnya
Copy link
Member

@MeredithAnya MeredithAnya commented Feb 3, 2021

Once #23545 is merged and deployed, we will have removed basically all the functionality for slack workspace apps. The last piece is the app itself, and it's credentials.

IMPORTANT: This PR should only be merged AFTER we have updated our slack-v2 credentials to replace our slack credentials.

We started out with one slack app, our now deprecated workspace app. The credentials for this app are:

"slack.client-id"
"slack.client-secret"
"slack.verification-token"
# added later when we introduced support for signing secrets
"slack.signing-secret"

We then introduced a new Slack integration, the bot app. The credentials for that app are:

"slack-v2.client-id"
"slack-v2.client-secret"
"slack.signing-secret"

Now there is actually a third type of app which is a older bot app, this kind of bot app may still be used by self hosted sentry users, and may be using the slack.verification-token instead of the signing secret (though they should probably update their app to use the secret). But anyway this is the reason we need to keep that option, although sentry SaaS will not need it.

mini clean up: we don't use the option slack.legacy-app anymore, so getting rid of it

Side Note: I'm mocking out the signing secret check in the endpoint tests because I couldn't get the payloads to match for some reason,I want to come back to this, but it isn't any worse that what it was before because these tests used the verification token which we don't use anymore

@MeredithAnya MeredithAnya requested a review from a team February 3, 2021 18:50
Copy link
Contributor

@scefali scefali left a comment

Choose a reason for hiding this comment

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

LGTM, I'll approve once the other PR is merged just so we don't accidentally merge this first

@MeredithAnya
Copy link
Member Author

Confirming here that the following credentials have been updated:

  • slack.client-id is set here
  • slack.client-secret uses the env var SLACK_CLIENT_SECRET which was updated by @b1naryth1ef
  • slack.signing-secret uses the env var SLACK_SIGNING_SECRET which was updated by @b1naryth1ef

The slack.verification-token uses the env var SLACK_VERIFICATION_TOKEN and was not changed since we don't use it. (we always look for the signing secret first)


resp = self.client.post("/extensions/slack/action/", data=payload)
resp = self.client.post("/extensions/slack/action/", data=payload, **headers)
Copy link
Member Author

Choose a reason for hiding this comment

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

can't seem to get the signatures to match when I actually pass in the correct headers :(

@MeredithAnya MeredithAnya merged commit 814a7ff into master Feb 10, 2021
@MeredithAnya MeredithAnya deleted the slack-creds/API-1635 branch February 10, 2021 23:01
bruno-garcia pushed a commit that referenced this pull request Feb 12, 2021
* ref(slack): Remove slack-v2 creds and usage

* update slack requests tests

* dont need verification token

* add broken test

* mock signing secret
@github-actions github-actions bot locked and limited conversation to collaborators Feb 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants