Only clear_scheduled_emails previously took a lock on the users before
removing them; make deliver_scheduled_emails do so as well, by using
prefetch_related to ensure that the table appears in the SELECT. This
is not necessary for correctness, since all accesses of
ScheduledEmailUser first access the ScheduledEmail and lock it; it is
merely for consistency.
Since SELECT ... FOR UPDATE takes an UPDATE lock on all tables
mentioned in the SELECT, merely doing the prefetch is sufficient to
lock both tables; no `on=(...)` is needed to `select_for_update`.
This also does not address the pre-existing potential deadlock from
these two use cases, where both try to lock the same ScheduledEmail
rows in opposite orders.
(cherry picked from commit 4c518c2bba)
No codepath except tests passes in more than one user_profile -- and
doing so is what makes the deduplication necessary.
Simplify the API by making it only take one user_profile id.
(cherry picked from commit ebaafb32f3)
`deliver_scheduled_emails` and `deliver_scheduled_messages` use their
respective tables like a queue, but do not have guarantees that there
was only one consumer (besides the EMAIL_DELIVERER_DISABLED setting),
and could send duplicate messages if multiple consumers raced in
reading rows.
Use database locking to ensure that the database only feeds a given
ScheduledMessage or ScheduledEmail row to a single consumer. A second
consumer, if it exists, will block until the first consumer commits
the transaction.
(cherry picked from commit 1e67e0f218)
Sometimes the Slack import zip file we get isn't quite the canonical
form that Slack produces -- often because the user has unzip'd it,
looked at it, and re-zip'd it, resulting in extra nested directories
and the like.
For such cases, support passing in a path to an unpacked Slack export
tree.
Slack bot emails generated by us can be duplicate for two bots.
If such a case occur, append a counter to the email to make it
unique.
For maintaining the counter of duplicate emails and the final
email assigned to each bot, a class based approach is used with
static variables and static (class) methods. This keeps all the
data related to slack bot emails at the same place and easily
accessible from anywhere inside the module (without defining any
class object and passing it around).
Fixes: #16793
Zulip attempts to validate that the regular expressions that admins
enter for linkifiers are well-formatted, and only contain a specific
subset of regex grammar. The process of checking these
properties (via a regex!) can cause denial-of-service via
backtracking.
Furthermore, this validation itself does not prevent the creation of
linkifiers which themselves cause denial-of-service when they are
executed. As the validator accepts literally anything inside of a
`(?P<word>...)` block, any quadratic backtracking expression can be
hidden therein.
Switch user-provided linkifier patterns to be matched in the Markdown
processor by the `re2` library, which is guaranteed constant-time.
This somewhat limits the possible features of the regular
expression (notably, look-head and -behind, and back-references);
however, these features had never been advertised as working in the
context of linkifiers.
A migration removes any existing linkifiers which would not function
under re2, after printing them for posterity during the upgrade; they
are unlikely to be common, and are impossible to fix automatically.
The denial-of-service in the linkifier validator was discovered by
@erik-krogh and @yoff, as GHSL-2021-118.
Apparently, our slack compatible outgoing webhook format didn't
exactly match Slack, especially in the types used for values. Fix
this by using a much more consistent format, where we preserve their
pattern of prefixing IDs with letters.
This fixes a bug where Zulip's team_id could be the empty string,
which tripped up using GitLab's slash commands with Zulip.
Fixes#19588.
This fixes a regression where one could end up deactivating all owners
of a realm when trying to synchronize LDAP with the `is_realm_admin`
flag configured in `AUTH_LDAP_USER_FLAGS_BY_GROUP`.
With tweaks by tabbott to add is_moderator as well.
Fixes#18677.
The auth attempt rate limit is quite low (on purpose), so this can be a
common scenario where a user asks their admin to reset the limit instead
of waiting. We should provide a tool for administrators to handle such
requests without fiddling around with code in manage.py shell.
(cherry picked from commit fdbde59b07)
While it should be an invariant that message.rendered_content is never
None for a row saved to the database, it is possible for that
invariant to be violated, likely including due to bugs in previous
versions of data import/export tools.
While it'd be ideal for such messages to be rendered to fix the
invariant, it doesn't make sense for this has_link migration to crash
because of such a corrupted row, so we apply the similar policy we
already have for rendered_content="".
There might be good reasons to have other external authentication
methods such as SAML configured, but none of them is available.
This happens, for example, when you have enabled SAML so that Zulip is
able to generate the metadata in XML format, but you haven't
configured an IdP yet. This commit makes sure that the phrase _OR_ is
only shown on the login/account page when there are actually other
authentication methods available. When they are just configured, but
not available yet, the page looks like as if no external
authentication methods are be configured.
We achieve this by deleting any_social_backend_enabled, which was very
similar to page_params.external_authentication_methods, which
correctly has one entry per configured SAML IdP.
Zulip identifies users by realm+delivery_email which means that the
Django changepassword command doesn't work well -
since it looks only at the .email field.
Thus we fork its code to our own change_password command.
This fixes a bug introduced in 95b46549e1
which made the worker simply log a warning about the timeout and then
continue consume()ing the event that should have also been interrupted.
The idea here is to introduce an exception which can be used to
interrupt the consume() process without triggering the regular handling
of exceptions that happens in _handle_consume_exception.
Throwing an exception is excessive in case of this worker, as it's
expected for it to time out sometimes if the urls take too long to
process.
With a test added by tabbott.
This allows specific queue workers to override the defaut behavior and
implement their own response to the timer expiring. We will want to use
this for embed_links queue at least.
This is much faster than calling generate_presigned_url each time.
```
In [3]: t = time.time()
...: for i in range(250):
...: x = u.get_public_upload_url("foo")
...: print(time.time()-t)
0.0010945796966552734
```
Fixes#18915
This was very slow, causing performance issues. After investigating,
generate_presigned_url is the cheap part of this, but the
session.client() call is expensive - so that's what we should cache.
Before the change:
```
In [4]: t = time.time()
...: for i in range(250):
...: x = u.get_public_upload_url("foo")
...: print(time.time()-t)
6.408717393875122
```
After:
```
In [4]: t = time.time()
...: for i in range(250):
...: x = u.get_public_upload_url("foo")
...: print(time.time()-t)
0.48990607261657715
```
This is not good enough to avoid doing something ugly like replacing
generate_presigned_url with some manual URL manipulation, but it's a
helpful structure that we may find useful with further refactoring.
This causes avatars and emoji which are hosted by Zulip in S3 (or
compatible) servers to no longer go through camo. Routing these
requests through camo does not add any privacy benefit (as the request
logs there go to the Zulip admins regardless), and may break emoji
imported from Slack before 1bf385e35f,
which have `application/octet-stream` as their stored Content-Type.
Our current logic only allows S3 block storage providers whose
upload URL matches with the format used by AWS. This also allows
other styles such as the "virtual host" format used by Oracle cloud.
Fixes#17762.
modified_user=sub_info.user and modified_stream=sub_info.stream, added
by commit 6d1f9de7d3 (#16553), were
always coming from the last entry in the loop above, not from the
enclosing list comprehension.
Found by the Pylint rule undefined-loop-variable.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
?dl=1 causes Dropbox to send Content-Type: application/binary, which
can’t be interpreted by Camo. Use ?raw=1 instead.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Having the alert state in the message body is useful when alert topics
are not defined by alert description but encoded in the url.
E.g. in large environments having a topic for each alert [alerting] and [ok] would
make it harder to properly track if an alert has been resolved.
When each alert is in a single topic, so far, the alert state has been missing.
This change will add the current alert state and a fitting icon in front
of the alert name.(Similar to the prometheus alertmanager integration)
The test cases have been amended to cover all possible alert states, even
though realistically grafana only fires the ok and alerting states via
webhook.
We record Git details about the merge-base with upstream branches in
the zulip-git-version file, if the upstream repository is available.
Note that the first Git upgrade after merging the parent commit will
not include the merge-base details, since the upstream repository will
not have been available.
Co-authored-by: Tim Abbott <tabbott@zulip.com>
Signed-off-by: Anders Kaseorg <anders@zulip.com>
These logs were pretty spammy, and there have long been much better
ways to communicate to system administrators that the incoming email
gateway is great, including, most importantly, in the section of the
emails themselves that explains how replying works.
Previously only admins were allowed to move messages between streams
and admins are allowed to post in any stream irresepctive of stream
post policy, so there was no need to check for stream post policy.
But as we now allow other members to also move messages, we need
to check whether the user who is moving the message is allowed
to post to the target stream (i.e. stream to which the messages
are being moved) and thus we allow moving messages only if the
user is allowed to post in target stream.
