Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

Add support for Slack shared secret message signing #30

Merged
merged 3 commits into from
Nov 27, 2018

Conversation

Latent-Logic
Copy link
Contributor

Support was added to the underlying Slack library in pyslackers/slack-sansio#41
It is the recommended way to verify requests from Slack now:
https://api.slack.com/docs/verifying-requests-from-slack

Also when running the tests I ran into aio-libs/aiohttp#2578 so I fixed those deprecation warnings.

DeprecationWarning: Deprecated, use aiohttp_server fixture instead
DeprecationWarning: Deprecated, use aiohttp_client fixture instead
@@ -74,8 +124,8 @@ def find_bot_id_query():

Copy link
Contributor Author

@Latent-Logic Latent-Logic Nov 27, 2018

Choose a reason for hiding this comment

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

The Travis black failure is coming from line 117, which isn't even changed in my commit:

-                "updated": 1502138686,
+                "updated": 1_502_138_686,

(This issue didn't show up when running tox locally)

Copy link
Member

Choose a reason for hiding this comment

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

I don't recall black formatting number that way. It's probably due to a newer version.

@Latent-Logic
Copy link
Contributor Author

Latent-Logic commented Nov 27, 2018

Travis succeeded for everything except Python 2.6, however that matches the pattern from the Travis run a few days ago on commit 10656ed:
https://travis-ci.org/pyslackers/sir-bot-a-lot-2/builds/459096989

The error specifically has to do with a failure to import idna_ssl which is included by aiohttp

@ovv
Copy link
Member

ovv commented Nov 27, 2018

Looks very good. Thanks for this. I'll take a deeper look & do some tests as soon as possible.

The failure is due to idna_ssl being required for python 3.6 but not required for python 3.7 and pipenv doesn't handle that well for the moment. I migrated slack-sansio to poetry a few days ago, I'll probably do the same for this repo.

Copy link
Member

@ovv ovv left a comment

Choose a reason for hiding this comment

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

Two small things and then we can :shipit:

else:
verification_token = slack.verify
event = Event.from_http(payload, verification_token=verification_token)
except (FailedVerification, InvalidSlackSignature, InvalidTimestamp):
Copy link
Member

Choose a reason for hiding this comment

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

That code is used in multiple endpoint. I would be best to take it out in a function, something like

try:
    validate_request(request, slack.signing_secret)
    event = Event.from_http(payload, verification_token=slack.verify)
except (FailedVerification, InvalidSlackSignature, InvalidTimestamp):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to explicitly keep verification_token as a new variable so the token we're passing into Event is explicitly None when the signing_secret is set. This way we're not relying on the initialization behavior in slack/plugin.py. If we wanted to rely on no one tampering with the slack.verify parameter then we could just call validate_request_signature rather than wrapping it in a validate_request function.

sirbot/plugins/slack/plugin.py Outdated Show resolved Hide resolved
sirbot/plugins/slack/endpoints.py Show resolved Hide resolved
Disables verification tokens if provided.
* This failure didn't show up locally, just on Travis
@ovv ovv merged commit 6fee89f into pyslackers:master Nov 27, 2018
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.

3 participants