Even before GDPR changes, it was strange that we displayed
users differently for fork events vs. all other events.
After GDPR, we don't even get the `username` field any
more.
So now we simply use `display_name` if available, and then
we try `nickname`.
See https://developer.atlassian.com/cloud/bitbucket/bitbucket-api-changes-gdpr/
for more context.
We were trying to share the same format string between
the two different versions of bitbucket, but this only
creates confusion, as the two versions are only close
enough to be confusing.
The format string might be the same, but the semantics
are different, as well as the eventual outputs.
For example, the {username} piece here is simple in version
2, but in version 3 we append a url to the user's name.
Previously, the GitLab webhook code, namely the `get_objects_assignee`
method first tried to get a single assignee and if that failed then it
looks for multiple assignees and then it would return the first
assignee that it found (there's actually a code smell here - a loop
which would always return on the first iteration).
Instead, this commit will change that behavior to first check for
multiple assignees first then for a single assignee if we can't find
multiple assignees. Ultimately it will return a list of all of the
assignees (however many that might be [0, n]). This method has then
aptly been renamed to `get_assignees`.
Finally, we tweked the code using this method to always use it's
output as an "assignees" parameter to templates (there's also an
assignee parameter which we want to avoid here for consistency).
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
For some reasons, some of the fixtures had the +x bit set, while
some didn't. What this commit does is make sure that no fixture
is marked as "executable" (for anyone).
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
This clears it out of the data sent to Sentry, where it is duplicative
with the indexed metadata -- and potentially exposes PHI if Sentry's
"make this issue public" feature is used.
Any exception is an "unexpected event", which means talking about
having an "unexpected event logger" or "unexpected event exception" is
confusing. As the error message in `exceptions.py` already explains,
this is about an _unsupported_ event type.
This also switches the path that these exceptions are written to,
accordingly.
8e10ab282a moved UnexpectedWebhookEventType into
`zerver.lib.exceptions`, but left the import into
`zserver.lib.webhooks.common` so that webhooks could continue to
import the exception from there.
This clutters things and adds complexity; there is no compelling
reason that the exception's source of truth should not move alongside
all other exceptions.
If there are unsupported keys, we still log an error,
but we now also send a message to the stream. (This
is a good tradeoff for the github webhook, since users
can just turn off notifications if they find it spammy.
Also, we intend to support "repository" soon.)
This is a bit of an experiment to see how this plays
in the field:
* will customers notice the change?
* will Sentry reports look any different?
The main thing fixed here is that we weren't turning
on our keys into a list. And then I refined the message
a bit more, including sorting the keys.
I also avoid the unnecessary "else".
The EVENT_FUNCTION_MAPPER maps a string event name
to a function handler. Before this we circumvented
mypy checks with a call to get_body_function_based_on_type,
which specified Any as the type of our event function.
Now the types are rigorous.
This change was impossible without the recent commit
to introduce the Helper class.
The Helper class will soon grow, but the immediate
problem it solves is the need to jankily inspect
the parameters of our get_*_body function.
Most of the changes were handled by an ad hoc
munge.py script.
The substantive changes were adding the Helper
class and passing it in.
And then the linter discovered a place where
the optional include_title parameter wasn't used
(which is one of the reasons to avoid the janky
inspect-signature technique).
As a side note, none of the include_title parameters
needed a default value of False, as we always passed
in an explicit value.
We test cover both sides of include_title, which
you can verify by hard coding it to either True or
False (and seeing the relevant failures), although I
suspect most individual codepaths
only test one value, based on whether "topic" is in
the fixture or not.
Finally, I know Helper is not a great name, but I
intend to evolve the class a bit before deciding
whether a more descriptive name is helpful here.
(For example, an upcoming commit will add a
log_unexpected helper method.)
We get the header_event one level up the call
stack now, too.
It's somewhat annoying that we have our own
concept of "event" here, instead of just returning
our event handlers directly, or just calling them
directly, but it's a bit non-trivial to fix that
right away.
In passing, I remove the strange OR for "ping",
which is already a key in EVENT_FUNCTION_MAPPER.
See https://github.com/zulip/zulip/issues/16258 for
possible follow up here.
We now ignore the following two new pull_request
actions (as well as the three existing ones
from before):
approved
converted_to_draft
As the issue above indicates, we may want to actually
support "approved" if we can find somebody to work
on the webhook. (And then the issue goes a little
broader than what changed here.)
We consolidate the tests and remove the fixtures, which
just have a lot of noisy fields that we ignore. Also,
pull_request__request_review_removed was named improperly.
Our isort configuration was almost Black-compatible, but we were
missing ensure_newline_before_comments.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Rather than catching, checking action type, and possibly re-raising,
instead return None explicitly from `get_subject_and_body`, which
already signals for a blank success result. This collocates the logic
of the action types in one place, and removes the complexity of the
re-raise.
Sentry may get reported multiple exceptions stacks, in the case where
a `raise ...` was caught, and a new exception was `raise`d. In this
case, the `filename` is the most recent exception -- but the
exceptions are stored in the `exception` key in the order in which
they occurred. As such, taking the first value with a `stacktrace`
will result in showing the wrong line, or in no stack trace being
resolved at all.
Look from the last `exception` backwards, for matching stacks.
It's possible that this is a new name for the "due"
field, but it's not totally clear.
In the exception we saw in the field:
payload['action']['data']['old']['dueComplete'] = False
payload['action']['data']['card']['dueComplete'] = True
We remove the fixture for create_check_item, which
has been bit-rotting for as long as we have ignored
this type of card data.
Our new test is more powerful, in the sense that it
shows we successfully ignore all fixtures of this
type.
If we want to handle this, we'll just need to get
new, representative fixture data from trello.
We introduce get_payload for the relatively
exceptional cases where webhooks return payloads
as dicts.
Having a simple "str" type for get_body will
allow us to extract test helpers that use
payloads from get_body() without the ugly
`Union[str, Dict[str, str]]` annotations.
I also tightened up annotations in a few places
where we now call get_payload (using Dict[str, str]
instead of Dict[str, Any]).
In the zendesk test I explicitly stringify
one of the parameters to satisfy mypy.
If we're not passing in expected_topic or expected_message
to check_webhook, it's better to just call send_webhook_payload,
since we'll want to explicitly check our messages
anyway.
This preps us to always require those fields for
check_webhook, which can prevent insidious testing no-ops.
This forces us to be a bit more explicit about testing
the three key values in any stream message, and it
also de-clutters the code a bit. I eventually want
to phase out do_test_topic and friends, since they
have the pitfall that you can call them and have them
do nothing, because they don't actually require
values to be be passed in.
I also clean up the code a bit for the tests that
have two new messages arriving.
Having an optional stream_name parameter makes
it confusing to read the code if you know your
webhook is sending private messages.
And then the other two callers are already
checking topics, so they might as well check
stream names, too.
We also have the two stream-oriented callers
make their own call to "subscribe". And we
future-proof this by making sure the exception
for no-message-being-sent calls out that gotcha.
Somewhat in passing, we now assert that
self.STREAM_NAME is not None in the main
helper. This is partly to satisfy mypy, but
it's also a good sanity check.
This also sets the stage for the next commit,
where I'll add an assert_stream_message helper.
Not all webhook payloads are json, so send_json_payload was a
bit misleading.
In passing I also remove "bytes" from the Union type for
"payload" parameter.
Almost all webhook tests use this helper, except a few
webhooks that write to private streams.
Being concise is important here, and the name
`self.send_and_test_stream_message` always confused
me, since it sounds you're sending a stream message,
and it leaves out the webhook piece.
We should consider renaming `send_and_test_private_message`
to something like `check_webhook_private`, but I couldn't
decide on a great name, and it's very rarely used. So
for now I just made sure the docstrings of the two
sibling functions reference each other.
The "EXPECTED_" prefix and "_EVENTS" suffix
usually provided more noise than signal.
We also use module constants to avoid the "self."
noise. It also makes it a bit more clear which
constants actually have to be in the class (e.g.
"FIXTURE_DIR_NAME") to do their job.
We had optional parameters for expected_topic and
expected_message, which are trivial to eliminate,
since the integration is really simple.
And we were doing strange things trying to reset
class variables at the end of tests. Now we just
set them explicitly in the tests.
The test helper here was taking an "expected_topic"
parameter that it just ignored, and then the
dialogflow tests were passing in expected messages
in that slot, so the actual "expected_message" var
was "None" and was ignored. So the tests weren't
testing anything.
Now we eliminate the crufty expected_topic parameter
and require an actual value for "expected_message".
I also clean up the mypy type for content_type,
and I remove the `content_type is None` check,
since all callers either pass in a str content
type or default to "application/json".
Per [1], the sentry API returns frames sorted from oldest to newest.
As such, matching against the first filename that matches is most
likely not the right frame.
Match against the last frame with the guilty filename.
[1] https://develop.sentry.dev/sdk/event-payloads/stacktrace/
These weren’t wrong since orjson.JSONDecodeError subclasses
json.JSONDecodeError which subclasses ValueError, but the more
specific ones express the intention more clearly.
(ujson raised ValueError directly, as did json in Python 2.)
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Since the title of a merge request can often change, it shouldn't be a
part of the topic that we send the message to. Otherwise things would
get messy and confusing.
But at the same time we don't want to make this mandatory. So we add
a new boolean GET parameter that can toggle whether or not the topic
should include the MR title (`use_merge_request_title`).
Fixes#15951.
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
Card descriptions aren't dates, and calling prettify_date on them results in removing upper case T characters, replacing uppercase Z characters with " UTC", etc. in descriptions when they appear in Zulip.
This was pretty clearly just a copy/paste mistake (these functions are very closely parallel to the *_due_date_* functions above, which do work on dates and call prettify_date).
The idea behind doing this is that we would rather let the code error
out rather than add to the logs. It's webhook code usually never uses
the logging module so this section of legacy code needed to be changed
or removed.
Assists PR #15942.
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
This commit re-adds the integration for canarytokens.org, now separate
from the primary Thinkst integration.
Signed-off-by: David Wood <david@davidtw.co>
This commit fixes the Thinkst Canary integration which - based on the
schema in upstream documentation - incorrectly assumed that some fields
would always be sent, which meant that the integration would fail. In
addition, this commit adjusts support for canarytokens to only support
the canarytoken schema with Thinkst Canaries (not Thinkst's
canarytokens.org).
Signed-off-by: David Wood <david@davidtw.co>
This changes the notification messages for events that currently just
include the string `"the repository"` to also include the full (`org/repo`)
name of the affected repository. Messages for the following events are
changed:
- `public`
- `star`
- `watch`
- `repository`
- `team_add`
Background: we're using the GitHub integration for org-wide notifications
for the [Bytecode Alliance Zulip](bytecodealliance.zulipchat.com/), and
having all messages just say "the repository" isn't ideal. Even now one
can hover over the link to see the repo's url, but it'd be much nicer if
the message just contained the full name.
I also changed the message for `star` to include a link to the repository,
same as the `watch` notification.
After merging PR #15355 we found that CircleCI may send
super minimal payloads. This does not seem to depend on
.circleci/config.yml since we received two different
types of payloads off of the same configuration.
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
Generated by pyupgrade --py36-plus --keep-percent-format.
Now including %d, %i, %u, and multi-line strings.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Use read-only types (List ↦ Sequence, Dict ↦ Mapping, Set ↦
AbstractSet) to guard against accidental mutation of the default
value.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
There seems to have been a confusion between two different uses of the
word “optional”:
• An optional parameter may be omitted and replaced with a default
value.
• An Optional type has None as a possible value.
Sometimes an optional parameter has a default value of None, or None
is otherwise a meaningful value to provide, in which case it makes
sense for the optional parameter to have an Optional type. But in
other cases, optional parameters should not have Optional type. Fix
them.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Fixes#2665.
Regenerated by tabbott with `lint --fix` after a rebase and change in
parameters.
Note from tabbott: In a few cases, this converts technical debt in the
form of unsorted imports into different technical debt in the form of
our largest files having very long, ugly import sequences at the
start. I expect this change will increase pressure for us to split
those files, which isn't a bad thing.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Automatically generated by the following script, based on the output
of lint with flake8-comma:
import re
import sys
last_filename = None
last_row = None
lines = []
for msg in sys.stdin:
m = re.match(
r"\x1b\[35mflake8 \|\x1b\[0m \x1b\[1;31m(.+):(\d+):(\d+): (\w+)", msg
)
if m:
filename, row_str, col_str, err = m.groups()
row, col = int(row_str), int(col_str)
if filename == last_filename:
assert last_row != row
else:
if last_filename is not None:
with open(last_filename, "w") as f:
f.writelines(lines)
with open(filename) as f:
lines = f.readlines()
last_filename = filename
last_row = row
line = lines[row - 1]
if err in ["C812", "C815"]:
lines[row - 1] = line[: col - 1] + "," + line[col - 1 :]
elif err in ["C819"]:
assert line[col - 2] == ","
lines[row - 1] = line[: col - 2] + line[col - 1 :].lstrip(" ")
if last_filename is not None:
with open(last_filename, "w") as f:
f.writelines(lines)
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This commit adds an integration for Thinkst Canaries - physical, VM and
cloud-based canaries for detecting attackers to a network. Thinkst
Canaries can send webhook alerts when canaries have been tripped, and
this integration will post Zulip messages when these webhooks are
received.
Signed-off-by: David Wood <david@davidtw.co>
Generated by pyupgrade --py36-plus --keep-percent-format, but with the
NamedTuple changes reverted (see commit
ba7906a3c6, #15132).
Signed-off-by: Anders Kaseorg <anders@zulip.com>
datetime.timezone is available in Python ≥ 3.2. This also lets us
remove a pytz dependency from the PostgreSQL scripts.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
The previous code for this event was using a key that's not actually
a part of the payload. So here we simple remove the usage of that key
and add a (previously missing) test for this event.
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
mock is just a backport of the standard library’s unittest.mock now.
The SAMLAuthBackendTest change is needed because
MagicMock.call_args.args wasn’t introduced until Python
3.8 (https://bugs.python.org/issue21269).
The PROVISION_VERSION bump is skipped because mock is still an
indirect dev requirement via moto.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
* Encountered `No emoji specified for status 'Pending'` when using `on_start: always`:
```yaml
notifications:
webhooks:
...
on_start: always # default: never
```
"pyr_" events are like refund events some source called it a pseudo
refund event (https://stackoverflow.com/questions/46296374/how-can-
i-get-the-original-charge-and-refund-ids-of-an-automatic-payout).
Though due to the lack of any documentation on this event I'm not
confident if this is the right fix.
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
Use get_release_event_message from webhooks/git.py to format release
events using the newly implemented release message template.
Tweaked by tabbott to handle name=None.
Builds on #14746. Proposed in #14934.
GitHub supports opening a draft/WIP pull request and then marking it
as ready for review later on. This PR supports the ready_for_review
action for pull_request events.
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
Apparently, the change and test I added before didn't quite cover the
corner case that was broken. This does, and exposes a second bug as
well, which we fix.
Since notify is mostly depreciated in the latest versions of CircleCI.
Although we can use use notify in CircleCI 2.0 but currently
there is no documentation regarding it.
We could have use notify here rather than this hacky solution but
if we use notify it was not possible to trigger CircleCI webhook only
for the main Zulip repository.
Also corrected the circle ci webhook for the case where we don't receive
previous in post request
This adds a webhook that can be used to interpret standard Slack
payloads. Since there are a ton of existing Slack integrations out
there, having a webhook which can accept standard Slack payloads can
significantly ease transition pains. Obviously this can't do everything
that Slack payloads can (particularly WRT their widgets/interactions),
but we can ingest text and parse out multi-block payloads into a message
relatively reasonably.
When the integration was originally rewritten, support for the
deprecated webhook payloads was removed. We later noticed that some
people using Zulip were still using versions of Sentry that required
the older integration code.
Thus this commit adds back the older integration code and whenever the
Sentry webhook payload does not have a "data" field (which must be
present in all modern payloads as per the documentation at
https://docs.sentry.io/workflow/integrations/integration-platform/webhooks)
we will use the older Sentry integration code.
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
Sentry has client SDKs for many programming languages and frameworks.
Sentry has deprecated their old "Raven" series of client SDKs in favor
of a new series of client SDKs following their unified API format.
As it stood, our Sentry integration was already outdated being written
for the version 5 payloads (the Raven SDKs stopped at version 6 which
is already vastly different from version 5) when the current and
prominently used version is version 7.
This commit completely rewrites the existing Sentry integration.
Tested and supported events:
- Issue created, resolved, assigned, and ignored events.
- "Sentry events" for "capture exception" and "capture message" with
the Golang, Node.js, and Python SDKs (other SDKs should also work but
only these were used for testing).
For reference:
- Old (Raven) SDK for python:
https://github.com/getsentry/raven-python
- New (Unified API format) SDK for python:
https://github.com/getsentry/sentry-python
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
Generated by autopep8, with the setup.cfg configuration from #14532.
I’m not sure why pycodestyle didn’t already flag these.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
To be able to get a screenshot of the personal message using the
`generate-integration-docs-screenshot` tool, this commit changes the
personal build to be triggered by iago, instead of cordelia.
The webhook view used a default value for the email, which gave
non-informative errors when the webhook is incorrectly configured without
the email parameter.
The event name seems to have been incorrectly called `todo_due_date_changed`
instead of `todo_due_on_changed`. The API docs for webhooks don't mention
the correct event name, but the TODO json payload[1] seems to contain the
`due_on` field, aside from the fixture actually referring to
`todo_due_on_changed` event type.
[1]: https://github.com/basecamp/bc3-api/blob/master/sections/todos.md
Anders Kaseorg
Steve Howell
Hemanth V. Alluri
Alex Vandiver
davidscherer
David Wood
Till Schneidereit
Tim Abbott
Anders Kaseorg
Johannes Hochreiter
Mateusz Mandera
Puneeth Chaganti
Imran Iqbal
Imran Iqbal
cestrell
arpit551
Jenny Ghose
Chris Heald
pemontto