-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add first blood webhook #134
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I think this could better be achieved by using a config value (see the config
app) rather than an environment variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this feature, couple of changes regarding formatting that need to be made before it can be merged, also a couple suggestions that would imo improve this a bit.
if hook: | ||
chall_clean = challenge.name.replace('`', '') | ||
team_clean = team.name.replace('`', '') | ||
body = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this would be better off as a Slack webhook so its not locked to one platform, Discord is compatible via adding /slack on the end of the webhook url. This does remove our ability to use allowed_mentions
to sanitize the message but I think the same can be achieved with just replacing all @
with @[zero width space]
. Also, this might look better as an embed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with using Slack by default, and a Discord-style webhook if the url is a Discord one. The /slack
endpoint didn't seem to support embeds, I could be wrong.
src/challenge/views.py
Outdated
'allowed_mentions': {'parse': []}, | ||
'username': 'First Bloods', | ||
} | ||
requests.post(hook, json=body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely missed this the first time I looked at this PR, this request isn't guaranteed to succeed. If this throws an exception, the challenge will still be updated, but the user and team will not be saved to the database leaving it in an inconsistent state, this will also cause any API client to see a generic 500 even though the flag was correct and was half scored. Wrapping this line in a try except avoids all of that. @ractf/backend this and emails is probably enough that we cant avoid adding celery to run background tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a merge conflict on poetry.lock, if you can resolve that and the last couple changes, I'm happy to merge this
I've just checked and https://birdie0.github.io/discord-webhooks-guide/other/slack_formatting.html doesn't actually seem to be correct. POSTing their example to '/slack' returns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@thebeanogamer CI appears to be broke, tests pass locally, going to merge without ci |
Are you able to test the Slack webhook? I don't have it |
Will send a small webhook message (if an env var is set) upon the first solve of each challenge