This commit replaces "dark mode" and "light mode" with "dark theme"
and "light theme" in the message returned and shown in a little
popup in the UI, when color scheme settings are changed through
slash commands.
Since spectators can't access personal profile settings and
can't view profile for other users. Hence, we don't send realm
custom profile field data and user's profile data to spectators.
Fixes#20301.
Enable spectator access for test `zulip` realm in developement
setup.
Add option in `do_create_realm` to configure
`enable_spectator_access` field of `Realm`.
If null is a potential value of data type for a return value or
parameter in the API endpoint, then it is rendered as an option.
This currently relies on the 'nullable' setting in the OpenAPI spec
that was removed in the 3.1.0 release. If/when the OpenAPI version
is updated, then how the `data_type` for parameters and return values
is rendered will need to be reworked.
Fixes#20264.
RabbitMQ clients have a setting called prefetch[1], which controls how
many un-acknowledged events the server forwards to the local queue in
the client. The default is 0; this means that when clients first
connect, the server must send them every message in the queue.
This itself may cause unbounded memory usage in the client, but also
has other detrimental effects. While the client is attempting to
process the head of the queue, it may be unable to read from the TCP
socket at the rate that the server is sending to it -- filling the TCP
buffers, and causing the server's writes to block. If the server
blocks for more than 30 seconds, it times out the send, and closes the
connection with:
```
closing AMQP connection <0.30902.126> (127.0.0.1:53870 -> 127.0.0.1:5672):
{writer,send_failed,{error,timeout}}
```
This is https://github.com/pika/pika/issues/753#issuecomment-318119222.
Set a prefetch limit of 100 messages, or the batch size, to better
handle queues which start with large numbers of outstanding events.
Setting prefetch=1 causes significant performance degradation in the
no-op queue worker, to 30% of the prefetch=0 performance. Setting
prefetch=100 achieves 90% of the prefetch=0 performance, and higher
values offer only minor gains above that. For batch workers, their
performance is not notably degraded by prefetch equal to their batch
size, and they cannot function on smaller prefetches than their batch
size.
We also set a 100-count prefetch on Tornado workers, as they are
potentially susceptible to the same effect.
[1] https://www.rabbitmq.com/confirms.html#channel-qos-prefetch
Race conditions in stream unsubscription may lead to multiple
back-to-back SUBSCRIPTION_DEACTIVATED RealmAuditLog entries for the
same stream. The current logic constructs duplicate UserMessage
entries for such, which then later fail to insert.
Keep a set of message-ids that have been prep'd to be inserted, so
that we don't duplicate them if there is a duplicated
SUBSCRIPTION_DEACTIVATED row. This also renames the `message` local
variable, which otherwise overrode the `message` argument of a
different type.
Previously, our codebase contained links to various versions of the
Django docs, eg https://docs.djangoproject.com/en/1.8/ref/
request-response/#django.http.HttpRequest and https://
docs.djangoproject.com/en/2.2/ref/settings/#std:setting-SERVER_EMAIL
opening a link to a doc with an outdated Django version would show a
warning "This document is for an insecure version of Django that is no
longer supported. Please upgrade to a newer release!".
Most of these links are inside comments.
Following the replacement of these links in our docs, this commit uses
a search with the regex "docs.djangoproject.com/en/([0-9].[0-9]*)/"
and replaces all matches with "docs.djangoproject.com/en/3.2/".
All the new links in this commit have been generated by the above
replace and each link has then been manually checked to ensure that
(1) the page still exists and has not been moved to a new location
(and it has been found that no page has been moved like this), (2)
that the anchor that we're linking to has not been changed (and it has
been found that no anchor has been changed like this).
One comment where we mentioned a Django version in text before linking
to a page for that version has also been changed, the comment
mentioned the specific version when a change happened, and the history
is no longer relevant to us.
For export realm following changes have been made:
- `./manage.py export --upload` would delete `.tar.gz` and unpacked dir
- `./manage.py export` would only delete `unpacked dir`
Besides, we have removed `--delete-after-upload` as we have set it as
the default.
Fixes#20081
If realm is web_public, spectators can now view avatar of other
users.
There is a special exception we had to introduce in rest model to
allow `/avatar` type of urls for `anonymous` access, because they
don't have the /api/v1 prefix.
Fixes#19838.
This commit updates the error message returned when the maximum
invite limit for the day. We update the error returned by API to
only mention that the limit is reached and add the suggestion
to use multi-use link or contact support in the message shown
in webapp.
We create RealmUserDefault object for internal realm just
for consistency. The code in migration does so but it
was missed to add the code when creating new internal realm.
Not proxying these requests through camo is a security concern.
Furthermore, on the desktop client, any embed image which is hosted on
a server with an expired or otherwise invalid certificate will trigger
a blocking modal window with no clear source and a confusing error
message; see zulip/zulip-desktop#1119.
Rewrite all `message_embed_image` URLs through camo, if it is enabled.
Supporting URL percent-encoded bytes is possible using `%%20`, but this
is not necessarily very understandable to end-users, even those that
understand percent encoding.
Allow `%20` in linkifier URL format strings, and transform them into
`%%20` in the pattern just before they are applied in markdown
translation. Care must be taken here, such that already-escaped `%`s
are not escaped an extra time.
We do this before rendering, and not before storage, as
a simplification; the JS-side linkifier at present only understands
`%(foo)s` and thus needs no changes, and to avoid an un-escaping pass
before showing in the admin UI.
og:image is supposed to be an absolute URL, but some sites incorrectly
provide a relative URL. In this case, it makes more sense to
interpret it relative to the full page URL after redirects, rather
than relative to just the domain part of the page URL before
redirects.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Removes the `/day` and `/night` options from the typeahead menu while
still allowing the commands to be used. Typing `/day` and `/night`
will now suggest `/light` and `/dark`, respectively. Also changes the
`Dark mode` and `Light mode` popups that appear after using the
corresponding command.
Fixes#18318.
This makes logging more consistent between FCM and APNs codepaths, and
makes clear which user-ids are for local users, and which are opaque
integers namespaced from some remote zulip server.
Being able to determine how many distinct users are getting push
notifications per remote host is useful, as is the distribution of
device counts. This parallels the log line in
handle_push_notification for push notifications from local realms,
handled via the event queue.
It is confusing to have the plan type constants not be namespaced
by the thing they represent. We already have a namespacing
convention in place for constants, so we should use it for
Realm.plan_type as well.
`rendered_content` in historical messages may be empty; examining the
history of them may thus require diff'ing two empty strings, which
itself produces an empty string.
Use `lxml.html.fragment_fromstring` to be able to successfully parse
these, rather than 500.
Part of #19559.
As detailed in the comments, the default behavior is undesirable for us
because we can't really predict all possibilities of exceptions that may
be raised - and thus putting str(e) in the http response is potentially
insecure as it may leak some unexpected sensitive information that was
in the exception.
As a hypothetical example - KeyError resulting from some buggy
some_dict[secret_string] call would leak information. Though of course
we aim to never write code like that.
This commit adds related_name parameter to UserGroup.direct_members
such that we can use direct_groups instead of the default
usergroupmembership_set for getting all the groups of which the
user is direct member.
This commit also sets related_name of UserGroupMembership.user_group
and UserGroupMembership.user_profile to "+" which means that we will
not be having backward relations for these. This change is correct
since we would need to use the recursive queries to get all the
groups of a user and all the members of a group after we add the
subgroups concept in next commit. This leads to us using direct_members
field of UserGroup instead of usergroupmembership_set in mention code,
but this will soon be replaced with the recursive query function to
include subgroup's members as well.
Extracted this commit from #19866.
Authored-by : Anders Kaseorg <anders@zulip.com>
This commit makes the query in get_user_group_direct_members
efficient by directly fetching user-profile ids instead of
first fetching user profile object and then id.
This commit renames members field of UserGroup to direct_members
for better readability because in the new permissions model, a
user group can be a sub-group of another group and thus technically
members of sub-group will also be members of that group.
This is a prep commit for new permissions model.
Extracted this commit from #19866.
Co-authored-by: Anders Kaseorg <anders@zulip.com>
This is a prep commit for new permissions model in
which a user group would be able to have a subgroup.
This commit renames get_memberships_of_users to
get_direct_memberships_of_users to specify that
the function is used only to fetch the direct
memberships and not memberships of subgroups of
the direct group.
Extracted this commit from #19866.
Co-authored-by: Anders Kaseorg <anders@zulip.com>
This is a prep commit for new permissions model in which a user
group would be able to have a subgroup.
This commit renames get_user_groups to get_direct_user_groups
to specify that the function is used only to fetch the direct
groups that user is part of and not subgroups of the direct
group.
Extracted this commit from #19866.
Co-authored-by: Anders Kaseorg <anders@zulip.com>
This is a prep commit for new permissions model in which a user group would
be able to have a subgroup.
This commit renames get_user_group_members to get_user_group_direct_members
to specify that the function is used only to fetch direct members of group
and excludes the subgroup's members.
Extracted this commit from #19866.
Co-authored-by: Anders Kaseorg <anders@zulip.com>
From 430c5cb, in `fetch_initial_state_data`,
we only include legacy settings in the top level of
`state` and the newer ones are stored in `state['user_settings']`.
That should've had a corresponding change in apply_event().
Also, fixed a test related to this logic.
For users who are not logged in and for those who don't have
'prefers_web_public_view' set in session, we redirect them
to the default login page where they can choose to login
as spectator or authenticated user.
This commit adds can_create_web_public_streams helper
in models.py which will be used to validate whether
user is allowed to create a web-public stream or not.
This commit also adds the checks for Realm.POLICY_OWNERS_ONLY
in check_has_permission_policies.
This commit adds tests for POLICY_EVERYONE and POLICY_NOBODY
in check_has_permission_policies test. The original code
used these values but these were not covered in test.
When email_address_visibility is changed and either the old value
or the updated value is EMAIL_ADDRESS_VISIBILITY_EVERYONE then
email field of all users is updated and we also send the corresponding
event to clients. But apply_event code did not update the data on
receiving the event, so this commit fixes the code to correctly
handle the event in apply_event.
(We also use this event when just changing a user's email address).
This commit also adds the tests and openapi schema for the event.
Since the calls to the translation function `_()` are made outside
of the `send_message_moved_breadcrumbs` function, these strings are
translated outside of the `with override_language` block, leading to
translated strings even when we don't intend them to be translated.
We now use gettext_lazy with appropriate testing to avoid this.
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.
This fixes the issue where 'None' would appear in the rendered
html in case of a missing tab display_name. Now,
'test-help-documentation' will fail in case of any tab display_name
being missing.
In case of a tab_section with no tabs, currently a single tab with
the name 'null_tab' gets added. Added the display name 'None' for
'null_tab', to keep in line with the existing behaviour.
Fixes#19822
This makes our onboarding guide for education organizations much
simpler, since new organizations will start with these settings
correctly configured.
Fixes#19682
Users wanted a feature where they could specify
which users can create public streams and which users can
create private streams.
This splits stream creation code into two parts,
public and private stream creation.
Fixes#17009.
This commit replaces 'allow_message_deleting' boolean setting
with an integer setting 'delete_own_message_policy'. We have a
separate dropdown now for deciding which user-roles can delete
messages sent by themselves and the time-limit setting droddown
is different.
This new setting has two options - everyone and admins only. Other
options including moderators will be added further.
We also remove the "Never" option from the original time-limit
dropdown, as admins are always allowed to delete message. This
never option resembled the case of only admins being allowed to
delete but this state is now resembled by setting the dropdown
to "admins only" and we also disable the time-limit dropdown in
this case as admins are allowed to delete irrespective of limit.
Note, this setting is only for deleting messages sent by the
deleting user themselves, and only admins are allowed to delete
messages sent by others as before.
We make zero invalid value for message_content_delete_limit_seconds and
for handling the case of "Allow to delete message any time", the API-level
value of message_content_delete_limit_seconds is "anytime" and "None"
as the DB-level value. We also use these values for message retention
setting, so it helps maintain consistency.
This commit does not remove the 'enable_login_emails' field from
RealmUserDefault table but it is just not used and cannot be
changed from UI or API similar to 'enable_marketing_emails' setting.
This is a somewhat subtle function, that deserves a few comments
explaining subtle details of its logic, and there's no good reason to
have multiple copies of that logic that are slightly inconsistent.
Because the main changes here are just checking for invariant
failures, the behavioral change here should be limited to ensuring
deactivated streams are not considered available even if they were
tagged as web public streams before deactivation.
This fixes a problem where we could not import zerver.lib.streams from
zerver.lib.message, which would otherwise be reasonable, because the
former implicitly imported many modules due to this issue.
This reverts commit cd93d0967f.
This check_or is redundant with check_union; it gives a misleading
error message for the non-matching case; and it has no type safety.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
In maybe_send_resolve_topic_notifications, since the calls to the
translation function `_()` are made outside of the `override_language`
block, the strings are not translated correctly.
This commit refactors the function to make sure that the translation
happens in the right block of code.
Fixes#19730.
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 commit removes the existing default_twenty_four_hour_time field in
Realm table which was used to set the twenty_four_hour_time setting of
new user on joining and instead we now use the twenty_four_hour_time
field of RealmUserDefault table for the same.
With some tweaks by tabbott to clarify the documentation.
These values are currently either a string already or a List[int]. We
should do the conversion in
do_update_user_custom_profile_data_if_changed properly: if the value is
already a string, it can be used directly - if it's not, orjson.dumps is
a more future-proof way of converting than str(). Using orjson.dumps
here also allows us to change the converter of the USER type
CustomProfileField to orjson.loads, which is nicer to have than
ast.literal_eval.
While orjson.dumps() and str() give the same output when
given the special case of List[int],
ast.literal_eval was previously used due to orjson.loads not being
a good inverse function to str in general. That gets straightened out
now.
None of the existing custom profile field types have the value as an
integer like declared in many places - nor is it a string like currently
decalred in types.py. The correct type is Union[str, List[int]]. Rather
than tracking this in so many places throughout the codebase, we add a
new ProfileDataElementValue type and insert it where appropriate.
The old assignment is incorrect - field_value.value is a TextField() and
should always be a string. This didn't strictly break anything, because
django converts the value to a string when .save()ing to the db, but
field_value.value persists as a non-string for the rest of this
codepath. After fixing this, the small codeblock in
notify_user_update_custom_profile_data handling conversion of
field_value.value to a string becomes redundant.
We're assured that we're not breaking event format by the test
test_custom_profile_field_data_events in test_events.py.
Send update event to client after a stream is made web public.
This has been documented in the API documentation since feature level
73; previously the value was always false.
We allow clients to make existing streams web public via the API.
This feature is still disabled via settings in production
environments, because we may have additional policy rules or UI
warnings we wish to add to this sort of conversion.
User can now create web public stream via the /subscribe API.
So, when a web public stream present in the API request does not
exist, it will be created now by specifying the is_web_public
parameter. The parameter would have been ignored without this
commit.
The new error message is more clear about why, "User cannot create
stream with this settings." was bad English, and in any case removing
an unnecessary string is always an improvement for translators.
Recently, we discovered that our settings_tab/relative Markdown
directives didn't work when they were in a macro that was included
in another Markdown file. Note that without this commit, the
/help/create-your-organization-profile page is broken. This commit
changes the respective priorities of these two extensions such that
these directives are rendered *after* the macro is included in
another file.
Thanks to Alya Abbott for reporting this bug!
All of our custom Markdown extensions have priorities that govern
the order in which the preprocessors will be run. It is more
convenient to have these all in one file so that you can easily
discern the order at first glance.
Thanks to Alya Abbott for reporting the bug that led to this
refactoring!
This is a follow-up to #19388.
We will in the future allow patch requests to change the visibility
of an existing topic, so `last_updated` is better name for this field.
This commit does not affect the API or events in any way, but only the
database.
This better matches the title of the page and more generally our
conventions around naming /help/ articles. We include a redirect
because this is referenced from Welcome Bot messages, and we
definitely don't want those links to break.
This parallels fe25517295, but for mobile notifications. It also
adds a test, which verifies that such content does not crash either
mobile or email notifications.
fe25517295 adjusted the email_notifications codepath to use
`lxml.html.fragment_fromstring` method when parsing
`rendered_content`, but left the tests using a helper which called
`fromstring`.
Switching the tests to match the code as run reveals a bug -- using
`drop_tree` on all `message_inline_image` classes now _does_ remove
all of a top-level image-URL-only message. Previously, such messages
were "safe" from the block that calls `drop_tree` only by dint of
`drop_tree` being a silent no-op for the root element. When parsed
using `fragment_fromstring`, they are no longer the root, and as such
an empty message results.
Reorder relative_to_full_url to check for only one `message_inline_image`
within the top `<div>`, and only run the `drop_tree` path in the
alternate case. Tests must be adjusted for their output now including
one more layer of `<div>`.
Our convention is to always have authenticate() called with a request
object. We need to be consistent with that in tests too, to avoid test
failures resulting from breaking that assumption.
We modify assert_login_failure to call client.login() in the same way as
the other similar helpers - with a properly initialized HttpRequest
instance.
Now, when we add a custom animated emoji to the realm
we also save a still image of it (1st frame of the gif). So
we can avoid showing an animated emoji every time.
create_confirmation_link has validity time as an optional argument,
because it has reasonable defaults. Thus it's a better API for
do_send_confirmation_email to make this optional as well, allowing
relying on create_confirmation_link's defaults.
This extends the invite api endpoints to handle an extra
argument, expiration duration, which states the number of
days before the invitation link expires.
For prereg users, expiration info is attached to event
object to pass it to invite queue processor in order to
create and send confirmation link.
In case of multiuse invites, confirmation links are
created directly inside do_create_multiuse_invite_link(),
For filtering valid user invites, expiration info stored in
Confirmation object is used, which is accessed by a prereg
user using reverse generic relations.
Fixes#16359.
The API for changing the batching period was added in
5db4fe8652.
This is a follow up to that commit. We also update the timestamps for
existing scheduled email notifications entries so that the effect of
changing the setting is immediate.
Part of #15280
These hostnames only have MX records for Mailgun and Front, and will
not work as a Zulip organization.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This commit moves check_settings_values to user_settings.py
from validator.py such that we can import the functions at
the top without any issue of cyclic imports.
We do not allow mentioning system user groups for now
because this can lead to circumventing the wildcard
mention restrictions. It will be enabled once we add
a setting to control that.
This is implemented by just ignoring it as one of the
mentioned user group even if the message content
inlcudes the mention syntax for it and the message
is sent normally.
We still keep the for_mention parameter for accessing
user group while sending email and push notifications
as mentioning system user groups will be allowed in
future.
This commit also removes the test for email notifications
for system user groups as we are not allowing mentioning
them.
This commit is only for backend change as we already
exclude the system groups from mention typeaheads and
other UI.
The name of the new realm created as a tombstone after renaming
a realm's subdomain is the constant 'placeholder-realm'.
This would confuse the user when shown the deactivation notice
and asking to join the realm at a new subdomain.
This PR replaces it with the original realm name to avoid confusion.
Fixes: #19677
This commit modifies the copy_user_settings code such that instead
of source user profile, we can have two types of sources - a user
profile and RealmUserDefault table of realm and then set the
settings from RealmUserDefault only is there is no user profile
as a source.
We also rename copy_user_settings to copy_default_settings for
clarity.
This commit adds do_set_realm_user_default_setting which
will be used to change the realm-level defaults of settings
for new users.
We also add a new event type "realm_user_settings_defaults"
for these settings and a "realm_user_settings_default" object
in '/register' response containing all the realm-level default
settings.
Because we create all realms with do_create_user (including in the
test suite), we just need to change that function, add a migration for
existing realms, and ensure the data import code path correctly
creates these objects.
Note that the import code path will create a RealmUserDefault row with
default values if it is not present in the import data, which is
important for importing data from other tools like Slack.
This commit changes the type of enable_marketing_emails parameter of
create_user to Optional[bool].
The value of this parameter will be None in certain cases when user
registers through SSO and 'TERMS_OF_SERVICE=False' when there will
be no registration form and thus no value of enable_marketing_emails.
We set the enable_marketing_emails setting after copying user
settings to override the value selected in registration form.
This change is also necessary because enable_marketing_emails
field is present in RealmUserDefault to avoid copying code
but we do not use this value actually and instead we want
the setting to be set according to the value in registration
form.
We set this setting only for non-bot users since we generally
do not set any settings for bots.
We extract the checks for default_language, notification_sound,
and email_notifications_batching_period_seconds setting values
in json_change_settings to a new function check_settings_values.
This prevented migration 0345
(517c2ed39d / #19696) from applying on
systems that were created after the refactoring that resulted in the
system bot realm potentially having null as its name.
(We've already confirmed that normal realms, created via
`do_create_realm`, shouldn't be able to have this unusual state).
This check was copied from upstream python-markdown's "safe mode"
before they removed that feature. The upstream history is that they
introduced this check in
2db5d1c8e4,
which was not a complete security check, and then added the
immediately following check (with an allowlist of schemes) in
0b4ffbb60e.
Their first, incomplete check provides no security benefit and makes
the code hard to reason about, so we remove it.
The 'update_global_notifications' type event is sent only for
existing settings and will not be sent for new settings, so we
should use notification_settings_legacy dict to check the type
of setting value in check_update_global_notifications instead
of notification_settings_types dict.
We still used notification_setting_types in copy_user_settings
function of create_user.py and in a test in test_event_system.py.
It is not required to do so since we have added all settings in
property_types already and we loop over property_types at both
these places which includes all settings.
This commit adds `demo_organization_scheduled_deletion_date` to
the `realm` section of the `/register` response so that it is
available to clients when enabled.
This is a part of #19523.
Since 84742a0, all settings are sent in the `user_settings` dictionary
which were previously sent inline with other fields in /register
response.
In order to simplify the process of adding new personal settings, we
want to transition to a world where new settings only need to consider
the `property_types` object, and code that needs to reference the
legacy behavior interacts with an object with `legacy` in its name.
This way, contributors working on new settings don't need to think
about the legacy code paths at all.
See https://chat.zulip.org/#narrow/stream/378-api-design/topic/user.20settings.20response.20in.20.2Fregister
to understand this better.
This will be used to check if the narrow being requested by
spectator requires authentication without requesting the server.
Having this check locally, makes this process look snappy to
the user and doesn't result in 404s in the browser log.
aioapns already has a retry loop. By default it retries forever on
ConnectionError and ConnectionClosed, so our own retry loop would
never be reached. Remove our retry loop, and configure aioapns to
retry APNS_MAX_RETRIES times on ConnectionError like the previous
version did. It still retries forever on ConnectionClosed; that’s not
configurable but probably fine.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This utilizes the generic `BaseNotes` we added for multipurpose
patching. With this migration as an example, we can further support
more types of notes to replace the monkey-patching approach we have used
throughout the codebase for type safety.
The motive of adding `BaseNotes` was to support monokey patching
temporary attributes to objects (such as `.trigger` on `Message`) when
working on the django-stubs migration in #18777.
This way, we no longer have to manually keep the upload path code in
sync with the upload path code in zerver/lib/upload.py.
This was originally suggested in
https://github.com/zulip/zulip/pull/19478#issuecomment-911479530.
This change fixes a bug when importing into a server using the local
file uploads backend, where the `import_realm.py` copy wasn't using
our standard 256-directory approach to avoid putting too many files in
a single directory.
de04f0ad67 changed now notifications recipients were calculated, in
a manner that caused them to be sent when they should not have been.
ac70a2d2e1 was supposed to resolve this, but appears to have been
insufficient, as all three of these cases have been observed to still
happen.
Add safety checks immediately before notification, until the
underlying logic error can be sussed out.
The default is kept as no retries. Since retries with exponential
backoff are a good thing to make easy, the int form defaults to
setting a backoff_factor.
Unfortunately, urllib3 retry backoff does not implement jitter.
Switching this to use the `backoff` library[1] rather than urllib3's
native Retry is left as future extension.
[1] https://pypi.org/project/backoff/
This adds the X-Smokescreen-Role header to proxy connections, to track
usage from various codepaths, and enforces a timeout. Timeouts were
kept consistent with their previous values, or set to 5s if they had
none previously.
This commits removes some unnecessary checks for `self.md.zulip_message`,
which were put there historically, as earlier we used to add the additional
properties like mentions_user_ids, alert_words, etc. to Message dict
only. These were later moved to MessageRenderingResult class in commit
75cea329b but the checks weren't removed.
This is important because while rendering the messages imported from
other chat tools (like Rocket.Chat), the Message dict is not passed to
the markdown, due to which the checks for `self.md.zerver_message` fails
and hence, things like user mentions, stream/topic mentions are not
rendered in the imported messages properly.
The transforms called from `build_message_payload` use
`lxml.html.fromstring` to parse (and stringify, and re-parse) the HTML
generated by Markdown. However, this function fails if it is passed
an empty document. "empty" is broader than just the empty string; it
also includes any document made entirely out of control characters,
spaces, unpaired surrogates, U+FFFE, or U+FFFF, and so forth. These
documents would fail to parse, and raise a ParserError.
Using `lxml.html.fragment_fromstring` handles these cases, but does by
wrapping the contents in a <div> every time it is called. As such,
replacing each `fromstring` with `fragment_fromstring` would nest
another layer of `<div>`.
Instead of each of the helper functions re-parsing, modifying, and
stringifying the HTML, parse it once with `fragment_fromstring` and
pass around the parsed document to each helper, which modifies it
in-place. This adds one outer `<div>`, requiring minor changes to
tests and the prepend-sender functions.
The modification to add the sender is left using BeautifulSoup, as
that sort of transform is much less readable, and more fiddly, in raw
lxml.
Partial fix for #19559.
We do not allow any user to edit the system user groups (including
renaming, deleting, adding or removing members, etc.) from the
API. These user groups will change only by the code when a new
user is added or role of a user is changed.
This is implemented by rejecting access_user_group_by_id always
except the case when it is use to get the user group for sending
email and push notifications, as we would need to send notifications
to the mentioned user group.
We make the description parameter in create_user_group as keyword-only
to improve readability. We would also keep the is_system_group
parameter which will be added in future keyword-only.
Tuples cannot be deserialized from JSON.
While we do use these validators for other things, like event
dictionaries, we have migrated the API away from using those. The
last use was removed in 4f3d5f2d87
Signed-off-by: Anders Kaseorg <anders@zulip.com>
These changes are all independent of each other; I just didn’t feel
like making dozens of commits for them.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Calling `email.save()` is only needed if we altered `email.address`;
it is unnecessary if we called `email.users.add(...)` which will have
done its own INSERT.
This fixes two bugs: the most obvious is that there is a race where a
ScheduledEmail object could be observed in the window between creation
and when users are added; this is a momentary instance when the object
has no users, but one that will resolve itself.
The more subtle is that .save() will, if no records were found to be
updated, _re-create_ the object as it exists in memory, using an
INSERT[1]. Thus, there is a race with `deliver_scheduled_emails`
between when the users are added, and when `email.save()` runs:
1. Web request creates ScheduledEmail object
2. Web request creates ScheduledEmailUsers object
3. deliver_scheduled_emails locks the former, preventing updates.
4. deliver_scheduled_emails deletes both objects, commits, releasing lock
5. Web request calls `email.save()`; UPDATE finds no rows, so it
re-creates the ScheduledEmail object.
6. Future deliver_scheduled_emails runs find a ScheduledEmail with no
attending ScheduledEmailUsers objects
Wrapping the logical creation of both of these in a single transaction
avoids both of these races.
[1] https://docs.djangoproject.com/en/3.2/ref/models/instances/#how-django-knows-to-update-vs-insert
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.
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.
This fixes a bug where email notifications were sent for wildcard
mentions even if the `enable_offline_email_notifications` setting was
turned off.
This was because the `notification_data` class incorrectly considered
`wildcard_mentions_notify` as an indeoendent setting, instead of a wrapper
around `enable_offline_email_notifications` and `enable_offline_push_notifications`.
Also add a test for this case.
Previously, the output would make it look like we sent an actual email
to the first user in the dry_run output, which is very confusing.
The `dry_run` code path already prints all the accounts that would
have been emailed at the end, so there's no reason to have this line
before the dry_run check.
Additionally, we move after the `get_connection` check because
failures at that stage shouldn't result in logging an attempt to send
an email.
This way we can stop reading as soon as we get to the body. Also,
send an Accept header, check that the request was actually successful,
use lxml.etree.iterparse instead of a broken hand-rolled state
machine, and support XHTML, all for negative 28 lines of code.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This reverts commit 1965584eec.
This syntax has a bad interaction with table syntax and needs to be
rethought.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This is more efficient than get_lexer_by_name, since we don’t need to
instantiate the class just to get its name.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
The BlockingChannel annotations in TornadoQueueClient were flat-out
wrong. BlockingChannel and Channel have no common base classes.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This fixes a regression in de04f0ad67.
We'll do a proper test in a follow-up commit; this is a quick fix to
make sure master works.
The emails will bounce, but it'll create all sorts of infrastructure
headaches.
We added "user_settings" object containing all the user settings in
previous commit. This commit modifies the code to send the existing
setting fields in the top-level object only if user_settings_object
client_capabilities field is False.
This commit adds "user_settings_object" field to
client_capabilities which will be used to determine
if the client needs 'update_display_settings' and
'update_global_notifications' event.
We send a event with type 'user_settings' on updating user's display
and notification settings.
The old event types - 'update_global_notifications' and
'update_display_settings', are still supported for backwards
compatibility.
Return zulip_merge_base alongside zulip_version
in `/register`, `/event` and `/server_settings`
endpoint so that the value can be used by other
clients.
The main reason why this is needed is because this seems to be
convention and because we can't easily test event creation without
doing this.
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
Previously, we checked for the `enable_offline_email_notifications` and
`enable_offline_push_notifications` settings (which determine whether the
user will receive notifications for PMs and mentions) just before sending
notifications. This has a few problem:
1. We do not have access to all the user settings in the notification
handlers (`handle_missedmessage_emails` and `handle_push_notifications`),
and therefore, we cannot correctly determine whether the notification should
be sent. Checks like the following which existed previously, will, for
example, incorrectly not send notifications even when stream email
notifications are enabled-
```
if not receives_offline_email_notifications(user_profile):
return
```
With this commit, we simply do not enqueue notifications if the "offline"
settings are disabled, which fixes that bug.
Additionally, this also fixes a bug with the "online push notifications"
feature, which was, if someone were to:
* turn off notifications for PMs and mentions (`enable_offline_push_notifications`)
* turn on stream push notifications (`enable_stream_push_notifications`)
* turn on "online push" (`enable_online_push_notifications`)
then, they would still receive notifications for PMs when online.
This isn't how the "online push enabled" feature is supposed to work;
it should only act as a wrapper around the other notification settings.
The buggy code was this in `handle_push_notifications`:
```
if not (
receives_offline_push_notifications(user_profile)
or receives_online_push_notifications(user_profile)
):
return
// send notifications
```
This commit removes that code, and extends our `notification_data.py` logic
to cover this case, along with tests.
2. The name for these settings is slightly misleading. They essentially
talk about "what to send notifications for" (PMs and mentions), and not
"when to send notifications" (offline). This commit improves this condition
by restricting the use of this term only to the database field, and using
clearer names everywhere else. This distinction will be important to have
non-confusing code when we implement multiple options for notifications
in the future as dropdown (never/when offline/when offline or online, etc).
3. We should ideally re-check all notification settings just before the
notifications are sent. This is especially important for email notifications,
which may be sent after a long time after the message was sent. We will
in the future add code to thoroughly re-check settings before sending
notifications in a clean manner, but temporarily not re-checking isn't
a terrible scenario either.
In this commit:
* We update the `UserStatus` model to accept
`AbstractReaction` as a base class so, we can get all the
fields related to store status emoji.
* We update the user status endpoint
(`users/me/status`) to accept status emoji fields.
* We update the user status event to add status emoji
fields.
Co-authored-by: Yash Rathore <33805964+YashRE42@users.noreply.github.com>
This commit replaces boolean field add_emoji_by_admins_only with an
integer field add_custom_emoji_policy as we would also add full members
and moderators option for this setting in further commits.
This removes a bunch of non-functional duplicate JavaScript, HTML, and
CSS that was interfering with maintenance on the functional originals,
because it was never clear how to update the duplicates or how to
check that you’d updated the duplicates correctly.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This commit moves "enter_sends" setting to property_types dict.
With this change, changing enter_sends setting also sends an
event of type "update_display_settings" and thus enables us
to live-update the UI.
When calling some functions or assigning values to certain attributes,
the arguments/right operand do not match the exact type that the
functions/attributes expect, and thus we fix that by converting types
beforehand.
Cross realm bots will soon stop being a thing. This param is responsible
for displaying "System Bot" in the user info popover - so this rename is the
right way to handle the situation.
We will likely want to rename the `cross_realm_bots` section as well,
but that is a more involved API migration.
This fixes a batch of mypy errors of the following format:
'Item "None" of "Optional[Something]" has no attribute "abc"
Since we have already been recklessly using these attritbutes
in the tests, adding assertions beforehand is justified presuming
that they oughtn't to be None.
This prevents a memory leak caused by the `SimpleLazyObject` instance of
`UserProfile` that create a reference loop with the request object
via `ZulipRequestNotes`.
This prevents a memory leak arising from Python’s inability to collect
a reference cycle from a WeakKeyDictionary value to its key
(https://bugs.python.org/issue44680).
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Sometime in the deep past, Zulip the GET /users/me/subscriptions
endpoint started returning subscribers. We noticed this and made it
optional via the include_subscribers parameter in
1af72a2745, however, we didn't notice
that they were being returned as emails rather than user IDs.
We migrated the core /register code paths to use subscriber IDs years
ago; this change completes that for the endpoints we forgot about.
The documentation allowed this error because we apparently had no
tests for this code path that used the actual API.
We add "ignored_parameters_unsupported" field to the response object
of 'PATCH /settings' endpoint. This will contain the parameters
passed to the endpoint which are not changed by the endpoint and are
ignored.
This will help in removing the other fields like "full_name" from
response which was essentially present to specify that only these
fields were updated by the endpoint and rest were ignored.
We will also change other endpoints to follow this in future.
Previously, non-admin emoji authors were allowed to
delete the emoji only if add_emoji_by_admins_only
was false. But, as add_emoji_by_admins_only setting
is for who can add emoji and not delete emojis, it
should not affect the behavior of deleting emojis
and users should always be allowed to delete the
emojis which. they added themselves
This commit adds moderators and full members options for
user_group_edit_policy by using COMMON_POLICY_TYPES.
Moderators do not require to be a member of user group in
order to edit or remove the user group if they are allowed
to do so according to user_group_edit_policy.
But full members need to be a member of user group to edit
or remove the user group.
There is no need to have a error message which specifies the
roles having permission to edit user-groups, we can simply
have error message as "Insufficient permission" as we already
show the roles having permission clearly in UI.
This concludes the HttpRequest migration to eliminate arbitrary
attributes (except private ones that are belong to django) attached
to the request object during runtime and migrated them to a
separate data structure dedicated for the purpose of adding
information (so called notes) to a HttpRequest.
This migrates some mocked Request class and mocked request achieved
with namedtuple in test_decorators and test_mirror_users to use the
refactored HostMockRequest.
Since weakref cannot be used with namedtuple, this old way of mocking a
request object should be migrated to using HostRequestMock. Only after
this change we can extract client from the request object and store it
via ZulipRequestNotes.
This includes the migration of fields that require trivial changes
to be migrated to be stored with ZulipRequestNotes.
Specifically _requestor_for_logs, _set_language, _query, error_format,
placeholder_open_graph_description, saveed_response, which were all
previously set on the HttpRequest object at some point. This migration
allows them to be typed.
We will no longer use the HttpRequest to store the rate limit data.
Using ZulipRequestNotes, we can access rate_limit and ratelimits_applied
with type hints support. We also save the process of initializing
ratelimits_applied by giving it a default value.
We create a class called ZulipRequestNotes as a new home to all the
additional attributes that we add to the Django HttpRequest object.
This allows mypy to do the typecheck and also enforces type safety.
Most of the attributes are added in the middleware, and thus it is
generally safe to assert that they are not None in a code path that
goes through the middleware. The caller is obligated to do manual
the type check otherwise.
This also resolves some cyclic dependencies that zerver.lib.request
have with zerver.lib.rate_limiter and zerver.tornado.handlers.
This will be used to store the missedmessage events received
during the waiting time for email notifications (which is currently
2 minutes, hardcoded).
The change in `test_retention` is because we've set `on_delete=CASCADE`
for the message field this table.
The new query is like so:
```
DELETE FROM "zerver_missedmessageemailentry"
WHERE "zerver_missedmessageemailentry"."message_id" IN (
1545, 1546, 1547, 1548, 1549, 1550, 1551, 1552, 1553
)
```
This reduces loose strings in the codebase, and allows us to not worry
about the exact naming (`stream_email_enabled` or `stream_emails_enabled`?)
and tense (`mentioned` or `mention`?).
Ideally this new class should have been in `lib/notification_data.py`,
which is our file for things like this. But, the next commit requires
using this data in `models.py`, and importing from `notification_data.py`
to `models.py` causes recursive imports.
Work around Fatal1ty/aioapns#15, by silencing error-level logging from
the aioapns logger. We deal with the results of failed
send_notification calls by examining the `result.description` and
handling them; the extra logging message merely clutters the Sentry
logs.
We use subs as a common variable name for a collection of stream
data structure used in settings, in lot of modules. So this
rename clears a bunch of related shadowed variables.
This function had a confusing name, which could result in someone
using it unintentionally when they meant do_reactivate_user.
We also add docstrings for both functions.
If the user is logged in, we'll stick to rate limiting by the
UserProfile. In case of requests without authentication, we'll apply the
same limits but to the IP address.
We remove timezone setting from UserProfile.property_types
so that we can directly use UserProfile.property_types for
implementation of realm-default values of various user
settings.
This commits removes the redundant `compute_show_invites` function
which computes the `show_invites` page parameter in `lib/users.py`.
It is so because, commit 13399833b0 removed
the `show_invites` context variable passed in index.html.
Hence, the `show_invites` page_param key is no
longer required to compute in backend as it can be switched with
`settings_data.user_can_invite_others_to_realm()` in the frontend.
This commits also removes the `test_compute*` tests in
`test_home` that concerned with the `show_invites` page parameter
as they are no longer required.
* `stream_name`: This field is actually redundant. The email/push
notifications handlers don't use that field from the dict, and they
anyways query for the message, so we're safe in deleting this field,
even if in the future we end up needing the stream name.
* `timestamp`: This is totally unused by the email/push notification
handlers, and aren't sent to push clients either.
* `type` is used only for the push notifications handler, since only
push notifications can be revoked, so we move them to only run there.
The code to also notify for wildcard mentions was added in
0ed0bb6828.
But that showed the same text for both the cases. This commit fixes
that.
This is more of change for correctness. The mobile app currently does
not rely on this text for notifications, but constructs the text by
itself from the data in the payload.
This also fixes the "stream_push_notify" case to consistently show
a `#` before the stream name.
Running notify_server_error directly from the logging handler can lead
to database queries running in a random context. Among the many
potential problems that could cause, one actual problem is a
SynchronousOnlyOperation exception when running in an asyncio event
loop.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
The previous logic was incorrect and was not flushing the stream from
cache after deletion.
```
stream = get_realm_stream("Verona", realm.id)
stream.delete()
get_realm_stream("Verona", realm.id)
```
In the above example, the last line of code would have returned
the stream from cache instead of throwing a Stream.DoesNotExist
error. This is fixed in the commit.
I have verified that this commit indeed fix the issue by verifying
that calling get_realm_stream again after deleting the stream
results in Stream.DoesNotExist error.
The reason for this bug is because of different striping
processes in the backend and frontend, i.e The frontend
checks if the message's `raw_content` has changed to
decide if the `content` of the message should be sent in
the request to the backend, or not. So, it removes the
leading new line ('\n') from the message `raw_content`
when checking it, which is causing the "Error saving edit:
You don't have permission to edit this message" error.
This commit fixes it by removing the leading new line
when cleaning message content.
The bug was explained by @punchagan and its solution
by @timabbott.
This change allow check_webhook to raise an error when a message is
sent and vice versa. This is useful when one payload is not expecting
any output messages.
In addition to event filtering, we add support for registering supported
events for a webhook integration using the webhook_view decorator.
The event types are stored in the view function directly as a function
attribute, and can be later accessed via the module path and the view
function name are given (which is already specified the integrations.py)
Note that the WebhookTestCase doesn't know the name of the view function
and the module of the webhook. WEBHOOK_DIR_NAME needs to be overridden
if we want exceptions to raised when one of our test functions triggered
a unspecified event, but this practice is not enforced.
all_event_type does not need to be given even if event filters are used
in the webhook. But if a list of event types is given, it will be possible
for us to include it in the documentation while ensuring that all the
tested events are included (but not vice versa at the current stage, as
we yet not required all the events included in the list to be tested)
This guarantees that we can always access the list of all the tested
events of a webhook. This feature will be later plumbed to marcos to
display all event types dynamically in doc.md.
Sahil Batra
Aman Agrawal
Lauryn Menard
Alex Vandiver
YashRE42
Shlok Patel
Tim Abbott
Mateusz Mandera
Anders Kaseorg
Jonny Tran
rht
Eeshan Garg
Vishnu KS
Alya Abbott
Dinesh
Pradyumna Sinha
Ganesh Pawar
sahil839
seiwailai
Kevin Scott
Abhijeet Prasad Bodas
Riken Shah
shanukun
Mateusz Mandera
Parth
PIG208
Priyansh Garg
tushar912
Steve Howell
Gaurav Pandey
Lefteris Kyriazanos
Hemanth V. Alluri
akshatdalton
m-e-l-u-h-a-n
aryanshridhar