We add quote prefix ">" to each line of the message in the plain text
missed message emails, which are then rendered as quotes by email
clients. We also move the message content in the next line after sender.
This helps us in clearly showing the message authors in missed message
emails especially in emails with multiple messages and senders.
Fixes#15836.
This ValueError had no test coverage, because the code path wasn't
actually possible with how the caller is constructed.
Rather than writing a highly artificial test for this as proposed in
This also allows us to remove some assertions as we now know that
AVATAR_SALT will never be None.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This implements get_mandatory_secret that ensures SHARED_SECRET is
set when we hit zerver.decorator.authenticate_notify. To avoid getting
ZulipSettingsError when setting up the secrets, we set an environment
variable DISABLE_MANDATORY_SECRET_CHECK to skip the check and default
its value to an empty string.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Due to mismatches between the URL parsers in Python and browsers, it
was possible to hoodwink rewrite_local_links_to_relative into
generating links that browsers would interpret as absolute.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
These characters are not allowed and trying to create a Zulip message
with those characters throws a JsonableError in check_stream_topic.
We don't want to reject emails with those chars in the subject, so
it's best to just modify it appropriately.
This commit checks for null values for keys within "attachment" in
the Slack integration's incoming payloads. These keys were expected
to exist optionally previously, and the existence of null values for
these wasn't anticipated. Due to an issue report for such null
values in the payload, their handling is updated appropriately.
The checks for these values are truthiness checks since the strategy
for these values being null or falsy ("", 0) is the same; we don't
process that key-value pair. This is consistent with how Slack handles
this scenario.
For the case where all the attachment fields have null values, Slack
displays this as an empty block with no content, and therefore our
strategy for this is a no-op.
Tests updated.
Since this decorator is only used for methods of
TestServiceBotEventTriggers, we can type the decorated method's
signature accurately without using ParamSpec.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
We can express the type of these decorators with Concatenate and ParamSpec
now for tighter type annotations.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This removes ViewFuncT and all the associated type casts with ParamSpec
and Concatenate. This provides more accurate type annotation for
decorators at the cost of making the concatenated parameters
positional-only. This change does not intend to introduce any other
behavioral difference. Note that we retype args in process_view as
List[object] because the view functions can not only be called with
arguments of type str.
Note that the first argument of rest_dispatch needs to be made
positional-only because of the presence of **kwargs.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This module was originally introduced in 2016 to assist adding mypy
annotations to the project. Back then static type checking was not that
established throughout the codebase, so it was helpful to be able to
print out the types for type checking purposes.
This workflow is no longer helpful for improving type annotations right
now, and it has been unused for a while.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Whether we sent a resolve topic notification or not may be useful in
the caller. It was originally intended to be used in #21712, but may
only be relevant for future logging.
Part of #21712.
This will have no real effect in most situations. However, a user
moves a topic to another stream while also adding/removing the
resolved-topic checkmark from the topic name, then the "This topic was
resolved" notificaiton will now appear just before the "This topic was
moved" notification rather than just after.
This is likely slightly less confusing to users, since the topic
having been moved from somewhere else is likely the most salient fact
to a reader.
We expect to change things to not send both notifications in an
upcoming commit.
This refactoring helps with #21712.
Our seat count calculation is different for guest user than normal users
(a number of initial guests are free, and additional marginal guests are
worth 1/5 of a seat) - so these checks we apply when a user is being
invited or signing up need to know whether it's a guest or non-guest
being added.
Because rate_limit_request_by_ip is the only caller of it, it is safe
for us to inline RateLimitedIpAddr and remove this helper. This ensures
that we have consistent internals for rate limiting functions, which all
have a should_rate_limit check.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This allows us to use them with HttpResponse objects returned by
calling a view function directly.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This change incorporate should_rate_limit into rate_limit_user and
rate_limit_request_by_ip. Note a slight behavior change to other callers
to rate_limit_request_by_ip is made as we now check if the client is
eligible to be exempted from rate limiting now, which was previously
only done as a part of zerver.lib.rate_limiter.rate_limit.
Now we mock zerver.lib.rate_limiter.RateLimitedUser instead of
zerver.decorator.rate_limit_user in
zerver.tests.test_decorators.RateLimitTestCase, because rate_limit_user
will always be called but rate limit only happens the should_rate_limit
check passes;
we can continue to mock zerver.lib.rate_limiter.rate_limit_ip, because the
decorated view functions call rate_limit_request_by_ip that calls
rate_limit_ip when the should_rate_limit check passes.
We need to mock zerver.decorator.rate_limit_user for SkipRateLimitingTest
now because rate_limit has been removed. We don't need to mock
RateLimitedUser in this case because we are only verifying that
the skip_rate_limiting flag works.
To ensure coverage in add_logging_data, a new test case is added to use
a web_public_view (which decorates the view function with
add_logging_data) with a new flag to check_rate_limit_public_or_user_views.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This allows us to avoid importing from zilencer conditionally in
zerver.lib.rate_limiter, as we make rate limiting self-contained now.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
- RateLimitTestCase.get_ratelimited_view is replaced by a view
function directly decorated by public_json_view.
- the META dict is initialized with "PATH_INFO": "test" because now the
tests cover the process_client codepath;
- HostRequestMock is initialized with host="zulip.testserver" to pass
the validate_account_and_subdomain check;
- check_rate_limit_public_or_user_views replaces both
test_rate_limiting_happens_in_normal_case and
test_rate_limiting_happens_by_ip_if_unauthed.
Overall, we deduplicate the test cases in this change, and make sure
that they also cover the view function decorators for authentication.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
The test setup for some of the test cases are largely similar, so it
would be cleaner to be able to reuse them.
Note that we use "check" in the name of this helper because later we
will extend it to take a flag to set whether rate limiting is expected.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This refactors the test case alongside, since normal views accessed by
remote server do not get rate limited by remote server anymore.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
In Zulip 2.1.0, the `is_muted` stream subscription property was
added and replaced the `in_home_view` property. But the server has
still only been sending subscription update events with the
`in_home_view` property.
Updates `do_change_subscription_property` to send a subscription
update event for both `is_muted` and `in_home_view`, so that
clients can fully migrate away from using `in_home_view` allowing
us to eventually remove it completely.
This is a follow-up to #19274. We map the supported event types to a
more suitable format for events.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
By replacing Any with object we enforce type narrowing before using the
kwargs when a more specific type is required.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
The only caller that passes the kwargs argument is the avatar rest_path.
The application of kwargs can be rewritten with a wrapper.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Mypy considers that "Tuple[Any, ...]" is incompatible with
"Union[Tuple[Callable[..., HttpResponse], Set[str]], HttpResponse]".
handler, view_flags = entry is sufficient to suppress the error, but we
also add assertions for full measure.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Commit b945aa3443 (#22604) incorrectly
assumed that Django would run the extra EmailField validators if basic
email address validation passed. Actually, it runs all validators
unconditionally and collects all failures. So email_is_not_disposable
needs to catch email address parsing errors.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This allows us to separate the zilencer paths from other JSON paths,
with explicit type annotation expecting `RemoteZulipServer` as the
second parameter of the handler using
authenticated_remote_server_view.
The test case is also updated to remove a test for a situation that no
longer occurs anymore, since we don't perform subdomain checks on
remote servers.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
As noted in the docstring, this is a temporary helper function that
separates routing for paths that support multiple HTTP methods from
`rest_dispatch` itself. We will need to replace this helper with
class-based views in the future. The helper will also be handy to
reduce duplication when splitting up `rest_dispatch` by authentication
methods.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This is a prep commit such that we can avoid duplicate code when we
unsubscribe bots for inaccessible private streams when changing owner
or reactivating them.
This commit changes the code to consider zero as an invalid value for
message_content_edit_time_limit_seconds. Now to represent the setting that
user can edit the message anytime, the setting value will be "None" in
database and "unlimited" will be passed to API from clients.
We sent the "message_content_delete_limit_seconds"
value to API when we change any setting in that
subsection and thus "do_set_realm_property" is
called even when we do not change that setting
since we handle it separately from most of the
other settings for which we use loop to call
"do_set_realm_property".
This commit changes the code to handle only
the case when the setting value is "None"
separately and rest all the values will be
changed from the "do_set_realm_property"
called in the loop as for other settings.
mypy_django_plugin cannot resolve the type of the custom manager. We add
a type annotation explicitly to skip the type inferrence.
This fixes the following error:
error: Could not resolve manager type for "zerver.models.UserGroup.objects" [django-manager-missing]
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Technically recipient_id cannot be None when recipient exists. We
actually just want to check if the recipient exists.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
The `message` attirbute doesn't get defined in the class where it is used.
The cleanest solution will be using the `Self` type introduced in PEP
673 and type it as something like `ForeignKey[Union[Self, Combinable],
Self]`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This modifies the QueryDict when it is mutable, and assign it to `.POST`
after it is turned immutable, as required by django-stubs for this
attribute.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Since validators do not affect the database, this migration is a noop.
Removing the migration fails check-database-compatibility. We might
eventually delete it when Django supports a cleaner method for deletion.
TODO:
Remove the type annotation when django-stubs is integrated.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Doing the dispatch to authenticated_json_view first lets us avoid
messing around with the skip_rate_limiting parameter.
Since rate_limit itself checks user.is_authenticated, there's no
potential downside to doing that check first here.
This refactoring is necessary to separate the expected type annotation
for view functions with different authentication methods. Currently the
signature aren't actually check against view functions because
`rest_path` does not support type checking parameter types, but it will
become useful once we do.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
`authenticated_rest_api_view` and `authenticated_json_view` essentially
remove `UserProfile` from the decorated function.
Note that `authenticated_log_and_execute_json` is removed to avoid
duplicating `ParamT` unnecessarily in the helper.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This refactors rate limit related functions from `zerver.decorator` to
zerver.lib.rate_limiter.
We conditionally import `RemoteZulipServer`, `RequestNotes`, and
`RateLimitedRemoteZulipServer` to avoid circular dependency.
Most instances of importing these functions from `zerver.decorator` got
updated, with a few exceptions in `zerver.tests.test_decorators`, where
we do want to mock the rate limiting functions imported in
`zerver.decorator`. The same goes with the mocking example in the
"testing-with-django" documentation.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This refactors `rate_limit` so that we no longer use it as a decorator.
This is a workaround to https://github.com/python/mypy/issues/12909 as
`rate_limit` previous expects different parameters than its callers.
Our approach to test logging handlers also needs to be updated because
the view function is not decorated by `rate_limit`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Adds an API endpoint for accessing read receipts for other users, as
well as a modal UI for displaying that information.
Enables the previously merged privacy settings UI for managing whether
a user makes read receipts data available to other users.
Documentation is pending, and we'll likely want to link to the
documentation with help_settings_link once it is complete.
Fixes#3618.
Co-authored-by: Tim Abbott <tabbott@zulip.com>
This commit adds support to change enable_read_receipts
setting through API and also adds the field to response
of "/register" endpoint so that the setting value
is available to clients.
This code is actually a noop (and would be a bug if it wasn't a noop),
because when this runs the server is already initialized, meaning the
internal realm exists and the system bots have been created, so
UserProfile.objects.filter(email=email) is always truthy. Also, system
bots are supposed to live in the internal realm, not in the realm being
imported so this code doesn't make sense currently.
This ensures type safety by not mutating the original queryset values,
that django-stubs to type as a TypedDict without total=False.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
BACKEND_DATABASE_TEMPLATE was introduced in a507a47778.
This setting is only available for the test cases and it is not that
necessary to have it configurable.
We define it as a global variable in zerver.lib.test_fixtures.
This avoids requiring mypy_django_plugin to know the type of
settings.BACKEND_DATABASE_TEMPLATE for type checking purposes, given the fact
that settings.test_extra_settings is not available in production/development
setup.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
We now use EVERYONE_GROUP_NAME instead of writing
the actual group name at multiple places, so that we
can have all the group names coded at one place only.
We now use MEMBERS_GROUP_NAME instead of writing
the actual group name at multiple places, so that we
can have all the group names coded at one place only.
We now use MODERATORS_GROUP_NAME instead of writing
the actual group name at multiple places, so that we
can have all the group names coded at one place only.
We now use ADMINISTRATORS_GROUP_NAME instead of writing
the actual group name at multiple places, so that we can
have all the group names coded at one place only.
We now use OWNERS_GROUP_NAME instead of writing
the actual group name at multiple places, so that
we can have all the group names coded at one place
only.
We now use EVERYONE_ON_INTERNET_GROUP_NAME instead of
writing the actual group name at multiple places, so
that we can have all the group names coded at one place
only.
We now use FULL_MEMBERS_GROUP_NAME instead of
writing the actual full members system group
name at multiple places, so that we can have
all the group names coded at one place only.
There may be some internal realms which were created after applying
"0382_create_role_based_system_groups.py" migration and this migration
is used to create system groups for those realms.
This commit modifies bulk_create_users to add the users to the
respective system groups. And due to this change, now bots in
development environment are also added to system groups.
Tests are changed accordingly as more UserGroupMembeship objects
are created.
Since we include internal realms while creating system groups
in "0382_create_role_based_system_groups.py", we should do it
when creating new internal realms as well to be consistent.
Tests are changed accordingly as UserGroup objects are created.
We also change the user group ids used in api docs examples
such that user groups are of correct realm.
I found the previous model for computing what settings to use for
streams increasingly difficult to understand, which is generally a
recipe for future bugs.
Refactor to have a clear computation of what complete permissions
state the client is requesting, validate that state, and then pass
that state to the do_change_stream_permission.
We now allow changing access to history of the stream by only passing
"history_public_to_subscribers" parameter. Previously, "is_private"
parameter was also required to change history_public_to_subscribers
otherwise the request was silently ignored.
We also raise error when only history_public_to_subscribers parameter
is passed with value False without "is_private: True" for a public
or web-public stream since we do not allow public streams with
protected history.
We raise error when we try to change a public stream (except for
zephyr mirror realms) to be public with protected history, as we do
not support such streams yet.
Previously, in such case we changed nothing and a notification was
sent to the "stream events" topic with message being "stream is
changed from public to public" and was weird.
Note that this commit only handles the case when both is_private and
history_public_to_subscribers parameters are passed to API and commit
not covers the case when only "history_public_to_subscribers" with
value False is passed to API, since we currently ignore requests
which has only history_public_to_subscribers parameter with not None
and not is_private and is_web_public.
We would do this in further commits when we add support for accepting
only history_public_to_subscribers parameter.
This commit removes the unnecessary assertion statements in
do_change_stream_permission for case when "is_web_public" is
True, since we already check those cases in the view function
update_stream_backend and this is the only place from where
do_change_stream_permission is called.
We aim to remove other assertions also from there as mentioned
in the comment and instead check the values in caller itself.
This is a prep commit for changing do_change_stream_permission
to require passing all (invite_only, history_public_to_subscribers
and is_web_public) arguments in further commits.
sahil839
Alya Abbott
Adam Sah
Tim Abbott
Zixuan James Li
Anders Kaseorg
Mateusz Mandera
Hari Prashant Bhimaraju
Aman Agrawal
Israel Ebonko
Julia Bichler
Lauryn Menard
Kartik Srivastava
Sahil Batra
Dinesh