Before this, a link still couldn't be re-used because it would trip up
exception further down user creation codepaths, but that was still a
bug. check_prereg_key is supposed to correctly validate the key - and
trigger an error page being returned if a key (or for any other reason,
the attached PreregistrationUser object) is reused.
test_validate_email_not_already_in_realm needs to be adjusted, because
it was actually re-using a key.
This reverts commit 40fcf5a633.
This commit triggers bug that we haven't fully tracked down, where web
app clients will continually send `update_message_flags` requests,
that then send out via the events system "0 messages were marked as
read" notices, eventually leading to a load spike.
The Tornado part can likely be fixed by checking if
updated_message_ids is empty, but we need to track down the frontend
bug as well.
`_cache` is not an attribute defined on `BaseCache`, but an
implementation detail of django_bmemcache.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Two endpoints had linked markdown files that were used in
their general descriptions to add warning notes with important
information (`/create-user` and `/get-user-groups`).
This moves the warning content to be inline in the endpoint
descriptions so that the important content is in the OpenAPI
documentation and is still formatted to be rendered in a warning
block.
Deletes `can-create-users-only.md` and `api-members-only.md`
since they were only used for these two endpoint descriptions.
Also, cleans up the other instance of a inline warning block in
an endpoint description (`/fetch-api-key`).
Instead of using `request.POST` to access the `data` parameter used
in the internal `notify_tornado` path, adds `has_request_variables`
decorator and accesses `data` as a `REQ` parameter.
Expands `test_tornado_endpoint` in `test_event_system.py` for
`data` being a required parameter for this path.
Instead of using `request.POST` to access `forward_address` for
the parameter used in `set_forward_address` in `email_page`, adds
`has_request_variable` decorator and an optional `forward_address`
parameter through the `REQ` framework.
Adds an assertion that `forward_address` is not `None` for `POST`
requests.
Previously, automated stream messages for new user signups were not
being translated into the realm's default language for said messages.
Moves `override_language` context manager so that it wraps the
new user message content in `notify_new_user` and topic string in
`send_message_to_signup_notification_stream`.
Fixes#22510.
The supertype contains `*args` and `**kwargs`, this adapts the signature
of the `get` method to make MarkdownDirectoryView compatible with it.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
The calling arguments here are completely wrong. The first argument
should be `request`, and `self` should never get passed to `.get`.
Because `TemplateView` happened to not use `request`, and we happened
to pass `article` as a keyword argument, this error slipped through.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Updates changelog entry for feature level 1 about GitLab to include
the endpoint with the changes. Also noted that the change updated
a deprecated return value.
Added changes note to the `gitlab` boolean in the
`authentication_methods` return value for the
`/get-server-settings` endpoint.
Part of work on #22102.
Updates the changelog note in feature level 1 about adding None as
a video call provider to include the endpoints where this realm
setting is used.
Updates the OpenAPI doc for the realm setting `video_chat_provider`
to include information about the enum values and meanings.
Part of work on #22102.
Corrects omissions or inconsistencies between the api changelog
and the api documentation for Zulip 3.0, feature level 1,
except for the final two bullet points about GitLab authentication
and adding None as a video call provider option.
The final two bullet points will be addressed in separate commits.
Part of work on #22102.
Initial round of fixes and clean-ups found during audit of
changelog entries for feature levels 1-27, which correspond
to the 3.0 release.
There are a few changes that are not related to those feature
levels, but fit within the context of clean-ups (spelling mistakes
or errors in api documentation formatting/structure/style).
One notable non-3.0 release fix is making all changes notes in
the OpenAPI documentation for 2.x releases use the correct
version numbering-scheme for those releases (e.g. 2.0.0).
Follow-up commits / PRs will address inconsitencies and omissions
for these feature levels found during the audit.
Updates references / language about organization settings that
were previously labeled as "Notifications", but are now labeled
as "Automated messages and emails".
Fixes#22136.
Co-authored by: Lauryn Menard <lauryn@zulip.com>
PostgreSQL's `default_statistics_target` is used to track how many
"most common values" ("MCVs") for a column when performing an
`ANALYZE`. For `tsvector` columns, the number of values is actually
10x this number, because each row contains multiple values for the
column[1]. The `default_statistics_target` defaults to 100[2], and
Zulip does not adjust this at the server level.
This translates to 1000 entries in the MCV for tsvectors. For
large tables like `zerver_messages`, a too-small value can cause
mis-planned query plans. The query planner assumes that any
entry *not* found in the MCV list is *half* as likely as the
least-likely value in it. If the table is large, and the MCV list is
too short (as 1000 values is for large deployments), arbitrary
no-in-the-MCV words will often be estimated by the query planner to
occur comparatively quite frequently in the index. Based on this, the
planner will instead choose to scan all messages accessible by the
user, filtering by word in tsvector, instead of using the tsvector
index and filtering by being accessible to the user. This results in
degraded performance for word searching.
However, PostgreSQL allows adjustment of this value on a per-column
basis. Add a migration to adjust the value up to 10k for
`search_tsvector` on `zerver_message`, which results in 100k entries
in that MCV list.
PostgreSQL's documentation says[3]:
> Raising the limit might allow more accurate planner estimates to be
> made, particularly for columns with irregular data distributions, at
> the price of consuming more space in `pg_statistic` and slightly
> more time to compute the estimates.
These costs seem adequate for the utility of having better search.
In the event that the pgroonga backend is in use, these larger index
statistics are simply wasted space and `VACUUM` computational time,
but the costs are likely still reasonable -- even 100k values are
dwarfed by the size of the database needed to generate 100k unique
entries in tsvectors.
[1]: https://github.com/postgres/postgres/blob/REL_14_4/src/backend/utils/adt/array_typanalyze.c#L261-L267
[2]: https://www.postgresql.org/docs/14/runtime-config-query.html#GUC-DEFAULT-STATISTICS-TARGET
[3]: https://www.postgresql.org/docs/14/planner-stats.html#id-1.5.13.5.3
In `JsonableErrorHandler`, we convert `MissingAuthenticationError` into
a response that has `WWW-Authenticated` set for `/api` or `/json` views.
This covers and verify the value of the header for unauthenticated
access.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
The dangling comment was not very helpful and unclear about the pieces
of code it was referring to.
We expand the part about linking the prereg_user to the created user,
while the part about "revoking other preregistration users" is
redundant, because the relevant code block lower down already has
comments on it with better explanations.
Closes#22274.
This assertion was added in 4b903c5dcd
where it may have made sense, because indeed when doing realm creation
there was always a PreregistrationUser (created because realms were
created via going to a generated realm creation link). With the addition
of the create_realm command that's no longer the case.
It would be unnatural to create a PreregistrationUser in the
realm_creation command, because there is no confirmation link for it to
be tied to - and it just doesn't make sense conceptually.
The intended, correct behavior added in
4b903c5dcd is still maintained - the code
lower down correctly handles the
(prereg_user is None and realm_creation) case.
The type safety of a TypeGuard is unchecked by mypy. While this
particular TypeGuard is safe given the current context, one could
imagine future changes that make it unsafe, so it’s preferable to
avoid unchecked constructs whenever possible.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
The field_data sent from client while creating a select
type field is a dict with a number as key.
In development database the field data for "Favorite editor"
field was of different form where the option label was used
as key in the dict.
This commit fixes it to be of the same as it is when creating
a field from web-app. As a result, we also need to update
the tests and this commit also update field_data for other
select-type fields.
This refactors the test case with more explicit type annotations, fixing
type errors discovered provided type annotations for
`CustomProfileField`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This change ensures that we can call the validate and update helper for
custom profile data later.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
We refactor the validator of `update_user_custom_profile_data` to ensure
that the validated variable is properly typed as
a `ProfileDataElementUpdateDict`, so we can call
`validate_user_custom_profile_data` and
`do_update_user_custom_profile_data_if_changed` directly later (unlike
`update_user_backend`, where `value` is allowed to be `None`, the
validator already ensures that no further check is required).
django-stubs types the return type of query.values(...) as a TypedDict.
This makes Dict[str, Any] that we have been using incompatible with it.
We use TypeGuard to ensure that `service_bot_tuples` is correctly
inferred to be `Tuple[int, int]` instead of `Tuple[int, Optional[int]]`.
Given that `bot_type` is optional for `ActiveUserDict`, we need to
narrow `row` to `ActiveBotUserDict` to make sure that `bot_type` is
non-optional. An advantage of this approach is that no assertions or
type casts are needed.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Otherwise mypy infers the type of `expected_result` to be incompatible
with the first argument of `fix_ordering_of_result`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
StreamingHttpResponse is inferred without the isinstance check in the
else branch. We refactor this is shorten the code and also type narrow
it appropriately.
`request.method` is not `None` in normal use cases, unless an
`HttpRequest` is directly instantiated without the method being set.
This situation does not apply to `WSGIRequest` at all.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Asserting response.stream is False is just suggesting the response being
an `HttpResponse`. This removes `StreamingHttpResponse` with the more
generic `HttpResponseBase` with an isinstance-check.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
`m.output` is a `list` of `str`s. It does not make sense comparing it to
a `str`. Guessed the intention here is to use `self.assert_length`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Items in `django.core.mail.outbox` are by default typed as the less
general `EmailMessage` type. Before accessing the attribute
`alternatives`, we need to narrow the type to `EmailMultiAlternatives`.
Then narrow the tuple value we want to access to `str` before using
it in `assertIn` or `self.normalize_string`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Instead of mutating the original `QueryDict`, we shall create a new
one when updating the `QueryDict`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
The body of `json_success` should be a `Mapping[str, object]`, optional
value is not allowed here.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
We use this decorator on subclasses of `MigrationsTestCase`, which does
not have `self`s being `MigrationsTestCase`, but the corresponding
subclass.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
It's hard to come up with a realistic story where this would matter:
SHARED_SECRET is generated automatically during server setup at the
same time as SECRET_KEY, which is a required setting, but it seems
preferable to be explicit that this is a required parameter for the
internal_notify authentication model.
Instead of using request.POST to get any potential `secret`
parameter used in `authenticate_notify` for `internal_notify_view`
decorator, moves it to the REQ framework parameters as `req_secret`.
Updates existing tests to explicitly test for a request without
`secret` parameter, which defaults to `None`; this is also tested
in `test_event_system.py`.
Similar to the previous commit, we should access request.user only
after it has been initialized, rather than having awkward hasattr
checks.
With updates to the settings comments about LogRequests by tabbott.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
`request.user` gets set in Django's `AuthenticationMiddleware`, which
runs after our `HostDomainMiddleware`.
This makes `hasattr` checks necessary in any code path that uses the
`request.user` attribute. In this case, there are functions in
`context_processors` that get called in the middleware.
Since neither `CsrfMiddleware` nor `HostDomainMiddleware` are required
to run before `AuthenticationMiddleware`, moving it two slots up in
`computed_settings` is sufficient to avoid the `hasattr` checks.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
A request that has went through the auth middleware shouldn't have
`.user` being `None`. We should use `AnonymousUser` by default to
represent unauthenticated users.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
If the emoji name contains forward slashes, the `str` converter
would treat it as a URL delimiter. Instead use the path converter, so
that forward slashes are included in the emoji name variable.
Fixes#22377
This commit removes "role" field from subscription
objects since we are not moving forward with stream
administrator concept and instead working on new
permssions model as per #19525.
This commit removes WILDCARD_MENTION_POLICY_STREAM_ADMINS
option of wildcard_mention_policy since we are not moving
forward with stream administrator concept and instead working
on new permssions model as per #19525.
We also add a migration to change wildcard_mention_policy of
existing realms to WILDCARD_MENTION_POLICY_ADMINS. This change
is fine since we were already treating both the setting values
as same as stream admin concept was not implemented completely.
This commit removes the is_stream_admin property of Subscription
model and also updates check_stream_access_for_delete_or_update
to not return true when is_stream_admin is True.
We also removes the relevant tests.
This change is done as we would not be moving forward with the
stream administrator concept as we have decided to modify the
permissions model as per #19525.
As discussed in the new comments results in a better failure mode if
an error occurs while adding subscriptions; running the merge tool
again after fixing whatever caused the error will work just fine.
The name does not really comply with the actual behavior of
the decorator since it returns True for an unauthenticated user.
This makes it clear that the 2fa check only applies to users that
are already logged in.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This replaces user.is_verified with is_2fa_verified.
The helper does extra checks such that the user being checked for 2fa
authentication status is valid.
`request.user.is_verified` is functionally the same as `is_verified`
from `django_otp.middleware`, except that the former is monkey-patched
onto the user object by the 2FA middleware. We use the latter wrapped
in `is_2fa_verified` instead to avoid accessing the patched attribute.
See also: 6b24d56e59/docs/source/overview.rst (authentication-and-verification)
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This simulates the situation in which the user is not
authenticated (as an AnonymousUser) and have 2FA enabled.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Adds a note to the `content` and `stream_id` parameters for the
`/update-message` endpoint that indicates these parameters throw
an error when sent in the same request.
Also, updates the main description of the endpoint to include
changing a message's stream. And updates some of the parameter
descriptions to be more consistent with each other and clear.
Zulip Server 2.1.0 and above have a UI tool, accessible only to server
owners and server administrators, which provides a way to download a
“public data” export. While this export tool is only accessible to
administrators, in many configurations server administrators are not
expected to have access to private messages and private
streams. However, the “public data” export which administrators could
generate contained the attachment contents for all attachments, even
those from private messages and streams.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
To prevent new contributors who are adding new user settings from
being confused about the legacy dicts for display settings and
notifications settings, we create `modern_settings` and
`modern_notifications_settings` dicts with comments documenting
that new user settings should be added to one of them.
Note that the `modern_notification_settings` is a placeholder for
new notifications settings as there have been no new notification
settings added since 430c5cb8 (when the legacy dicts were created)
and is currently annotated as `Dict[str, Any]` (which can be
removed if/when a new setting is added).
Updates `git-webhook-url-with-branches.md` (and two files that use
that file as an include link) for some of the follow-ups from #22315
to the Markdown parser. With this fix, all integrations docs that
reference this file as an include link should render the url as a
div element with `.codehilite` class.
The mypy django-stubs plugin incorrectly infers optional value for order,
which makes the `value_list` to be typed as `Iterable[Optional[int]]`.
We use a type cast here to ensure that
`try_reorder_relam_custom_profile_fields` won't causes mypy to complain
about it.
TODO: Remove the cast when https://github.com/typeddjango/django-stubs/issues/444 gets resovled.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
`member_ids` needs to be defined as an `Iterable` as it will otherwise
inferred to have incompatible types in the else branch.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
TODO: For now, we import `_QuerySet` as `ValuesQuerySet`. But there
is a convenient reexport of `ValuesQuerySet` in `django_stubs_ext`
that does the same thing. Once we get django-stubs integrated,
we should import `ValuesQuerySet` from `django_stubs_ext` instead.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
The pattern of using the same variable to apply filters
or alter the `QuerySet` in other ways might produce `QuerySet`s
with incompatible types. This behavior is not allowed by mypy.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
The returned dictionary is not at all used outside the function, so it's
sufficient to make it available only within the helper function itself.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This ensure that the return type is compatible with the
actual type of `realm.realmdomain_set.values`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
To explain the rationale of this change, for example, there is
`get_user_activity_summary` which accepts either a `Collection[UserActivity]`,
where `QuerySet[T]` is not strictly `Sequence[T]` because its slicing behavior
is different from the `Protocol`, making `Collection` necessary.
Similarily, we should have `Iterable[T]` instead of `List[T]` so that
`QuerySet[T]` will also be an acceptable subtype, or `Sequence[T]` when we
also expect it to be indexed.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
We were blindly adding / removing flag from UserMessages without
check if they even need to be updated.
This caused server to repeatedly update flags for messages which
already had been updated, creating a confusion for other clients
like mobile.
Fixes#22164
Added 2 new functions which check if a flag is present in a
user message or not using bit comparison.
Refactor `where_unread/starred/active_push_notification` to use
these functions.
Existing tests are enough to check if these functions work correctly.
Resizing emoji can fail, especially for animated GIFs; in such cases,
it is useful to have the original data on hand, to be able to dissect
the failure.
This was removed in Django 4.0 except in historical migrations. We
might as well replace it everywhere.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Our uWSGI configuration doesn’t correctly activate our virtualenv. We
should investigate that, but until we do, we need to invoke html2text
by an absolute path.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This was for the old /messages/latest API that was removed in commit
e06722657a.
If we wanted a new check like this, it shouldn’t go in zulip_finish,
because that only runs when the client gets an asynchronous response
from polling an initially-empty queue, and not when the client gets a
synchronous response from polling a nonempty queue.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
We wrap methods of the django test client for the test suite, and
type keyword variadic arguments as `ClientArg` as it might called
with a mix of `bool` and `str`.
This is problematic when we call the original methods on the test
client as we attempt to unpack the dictionary of keyword arguments,
which has no type guarantee that certain keys that the test client
requires to be bool will certainly be bool.
For example, you can call
`self.client_post(url, info, follow="invalid")` without getting a
mypy error while the django test client requires `follow: bool`.
The unsafely typed keyword variadic arguments leads to error within
the body the wrapped test client functions as we call
`django_client.post` with `**kwargs` when django-stubs gets added,
making it necessary to refactor these wrappers for type safety.
The approach here minimizes the need to refactor callers, as we
keep `kwargs` being variadic while change its type from `ClientArg`
to `str` after defining all the possible `bool` arguments that might
previously appear in `kwargs`. We also copy the defaults from the
django test client as they are unlikely to change.
The tornado test cases are also refactored due to the change of
the signature of `set_http_headers` with the `skip_user_agent` being
added as a keyword argument. We want to unconditionally set this flag to
`True` because the `HTTP_USER_AGENT` is not supported. It also removes a
unnecessary duplication of an argument.
This is a part of the django-stubs refactorings.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This is a prep commit for tightening the types for our wrapped test
client.
The callers of the test client methods are refactored to either call
them without unpacking at all or create a TypedDict for the keyword
arguments to be unpacked. This allows the type checker to know exactly what
keys are present and their corresponding type.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
`POST` is an immutable attribute and `_files` is an internal attribute
of `HttpRequest`. With type annotations provided by `django-stubs`, mypy
stops us from modifying these attributes. This uses `cast` and `setattr`
to avoid typing issues.
This is a part of django-stubs refactorings.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
We no longer need to access the internal `LANGUAGE_CODE` attribute by
using `django.utils.translation.get_language`.
A test case overriding the translation is added to ensure the password
reset form sending to users requested from a wrong domain is properly
translated.
This is a part of django-stubs refactorings.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
For the same reason as `handler_id` has, we define `_request`
as an attribute. Note that the name `request` is already taken.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This prevents us from relying on a side-effect of `allocate_handler_id`
that monkey-patches `handler_id` on the `AsyncDjangoHandler` object,
allowing mypy to acknowledge the existence of `handler_id` as an `int`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This ensures that all the keyword arguments in `move_rows`
have the correct types. Note that `returning_id` is supposed to be a
flag instead of a `Composable` `Literal`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This fixes inclusion of a multi-paragraph file into a list item.
Followup to commit dc33a0ae67 (#22315).
Signed-off-by: Anders Kaseorg <anders@zulip.com>
markdown-include is GPL licensed.
Also, rewrite it as a block processor, so that it works correctly
inside indented blocks.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Sometimes (e.g. when moving an old realm out of the way of an import
into that name) we do *not* wish to add a redirect realm. Add a flag
to support that.
We previously parsed any request with method other than {GET, POST} and
Content-Type other than multipart/form-data as if it were
application/x-www-form-urlencoded.
Check that Content-Type is application/x-www-form-urlencoded before
parsing the body that way. Restrict this logic to {DELETE, PATCH,
PUT} (having a body at all doesn’t make sense for {CONNECT, HEAD,
OPTIONS, TRACE}).
Signed-off-by: Anders Kaseorg <anders@zulip.com>
When there is no topic/stream being change, `propagate_mode` becomes
unnecessary. We add an assertion to ensure that the previous assumption
that `propagate_mode` is not `None` still holds when either `topic_name`
or `new_stream` is not `None`.
We can possibly improve this by overloading `do_update_message` and
`check_update_message`, but that's beyond the scope of the PR and
feasibility of doing that should also be further discussed.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Without an explicit type annotation, mypy infers the type of
values in `page_params` upon its initialization as a `Union`, while
other computed values haven't been assigned yet. We break this
over-conservative inferred type by annotating `page_params` as a
`Dict[str, object]`.
We could have created a `TypedDict` to have accurate type annotation
for all of the fields, but it does not seem worth it at this point since
the data structure is not widely used in the backend.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
We don't have a specific type for the reports returned from the error
logging handlers. The check is necessary as they are currently typed
as `Dict[str, object]` in `run_handler`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
`request.session` is `django.contrib.sessions.backends.base.SessionBase`
`instead of django.contrib.sessions.models.Session`.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
While it is possible to have `stream.recipient_id` being `None`,
the code works under the assumption that it is not. Potentially
we will get a runtime error, but it is not quite explicit without
the assertion.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This change is solely for removing the attribute error that mypy raises when we
access `pg_version` on `connection`. django-stubs annotate `connection` as
`BaseDatabaseWrapper` while it is actually a proxy object, so we cannot
use an regular assertion with isinstance to narrow the type.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
The “validator” component of the tuple does not follow the Validator
contract as of 7e9db327b3 (#15498).
Define a separate type for it.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
‘stream_name’ is not a cromulent keyword argument for client_post(),
‘unknown_action’ is malformed application/x-www-form-urlencoded, and
these two tests were duplicates of each other with different comments.
I’m not sure what they were intended to test, but here’s a guess.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
When editing an old message in a private stream with protected
history, the server would incorrectly send an API event including the
edited message to all of the stream’s current subscribers, including
those who should not have access to the old message. This API event is
ignored by official clients, so it could only be observed by a user
using a modified client or their browser’s developer tools.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Now that it is trivial to rename a stream in the UI, And due
to the fact that the command has been broken for 3 years unnoticed,
it is unnecessary to maintain it anymore.
Fixes#22244.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
The 0.1 second delay was sometimes not long enough to guarantee we hit
the async response path, resulting in a nondeterministic coverage
failure.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Commit 6fd1a558b7 (#21469) introduced an
await point where get_events_backend calls fetch_events in order to
switch threads. This opened the possibility that, in the window
between the connect_handler call in fetch_events and the old location
of this assignment in get_events_backend, an event could arrive,
causing ClientDescriptor.add_event to crash on missing
handler._request. Fix this by assigning handler._request earlier.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Removes the ":" which have accidentally ended up in the "Get a link
to a specific topic" and "Get a link to a specific stream" headings.
Renames the "Via browser's address bar" tab to "Web" so that it
stays consistent with other help center articles.
Fixes part of #22147.
Since `HttpResponse` is an inaccurate representation of the
monkey-patched response object returned by the Django test client, we
replace it with `_MonkeyPatchedWSGIResponse` as `TestHttpResponse`.
This replaces `HttpResponse` in zerver/tests, analytics/tests, coporate/tests,
zerver/lib/test_classes.py, and zerver/lib/test_helpers.py with
`TestHttpResponse`. Several files in zerver/tests are excluded
from this substitution.
This commit is auto-generated by a script, with manual adjustments on certain
files squashed into it.
This is a part of the django-stubs refactorings.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
We have now decided to not continue with the stream administrator
concept as we are changing the permissions model to be based on
user groups as per #19525. So, this commit updates the error message
to "Must be an organization administrator".
94457732c1 changed this from:
```py
event_name = payload.get("event_name", payload.get("object_kind")).tame(check_string)
```
...to:
```py
event_name = payload.get("event_name", payload["object_kind"]).tame(check_string)
```
Which causes a failure when `event_name` exists but `object_kind` does
not, since the default is evaluated first.
Switch to an `if` statement to clarify the fallbacks better.
This function is oblivious to the existence of ArchivedAttachment, which
is incorrect. A file can be removed if and only if it is not referenced
by any Messages or ArchivedMessages.
Using http://localhost:9991 is incorrect - e.g. messages sent with file
urls constructed trigger do_claim_attachments to be called with empty
list in potential_path_ids.
realm.host should be used in all these places, like in the other tests
in the file.
Adds a 2.1 release changelog entry for adding support for user
and stream IDs in search/narrow options. Also, adds a Changes
note in the narrow parameter in the OpenAPI `get-messages`
endpoint definition.
Both link to the api documentation for constructing a narrow,
where the 2.1 release update is already mentioned.
Fixes#9474.
Use `SimpleSuccess` response schema for all endpoints that were
already returning a success (200) response without any data beyond
the `response` and `msg` fields, which are standard for all
endpoint responses.
Prep commit for adding `ignored_parameters_unsupported` to
`json_success` responses.
Add none-checks, rename variables (to avoid redefinition of
the same variable with different types error), add necessary
type annotations.
This is a part of #18777.
Signed-off-by: Zixuan James Li <359101898@qq.com>
This commit changes the error message from "Invalid stream id"
to "Invalid stream ID" for cases where invalid stream IDs are
passed to API endpoints to make it consistent with other similar
error messages.
This applies a commonly-used, though non-RFC, header which suppresses
auto-replies to the message. There is a small chance that this will
result in bad filters thinking the messages *from Zulip* are
themselves auto-replies, but this seems a small risk.
Fixes: #13193.
Adds Changes notes for feature level 58 where support was added
for stream messages for the `/set-typing-status` endpoint
parameters.
Updates formatting for references to the `type`
parameter in the descriptions of other endpoint parameters.
Improves readability of and updates links in the endpoint's main
description.
Adds a changelog 2.0 entry for adding support for `stream_id`
parameter to the `mute-topic` endpoint. Also, adds Changes note
to the endpoint parameter description, and reorders/clarifies
that at least one (and only one) stream parameter must be provided
by the client and that the `string_id` parameter is preferred.
Fixes#11136.
Adds `create_web_public_stream_policy` to the `get-events` API
documentation for the `realm op:update` event.
Also, fixes changelog entries for feature levels 103 and 104,
which are related to the API documentation changes or fix an
error in references to the undocumented endpoint `PATCH /realm`.
We remove one call to get_occupied_streams to get occupied
streams before unsubscribing because we already know which
streams can become vacant, i.e. the one from which users are
being unsubscribed, and we can directly use the list of streams
from which users are being unsubscribed and get vacant streams
by checking which of these streams are not in get_occupied_streams
called after unsubscribing users.
This commit renames existing_subgroups variable to existing_direct_subgroup_ids
in add_subgroups_to_group_backend and remove_subgroups_from_group_backend functions
for better readability.
54b6a83412 fixed the typo introduced in 49ad188449, but that does
not clean up existing installs which had the file with the wrong name
already.
Remove the file with the typo'd name, so two jobs do not race, and fix
the typo in the comment.
Django caches some information on HttpRequest objects, including the
headers dict, under the assumption that requests won’t be reused.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Previously, we were marking messages of all the streams passed
to bulk_remove_subscriptions even if user was not subscribed
to some of them and those streams would ideally not have
any unread messages. This code was added in 766511e519.
This commit changes the code to only mark messages of actually
unsubscribed streams as read.
Conceptually, we're clearly intending to check whether the user we're
mutating is the last realm owner. The preexisting code was safe
because we've already checked that the target user is an owner, and
thus if we're the last owner, we're the target user.
This commit attempts to add the backend support by extending the
/json/bots/{bot_id}/ url support to accept the role field as a
parameter. This was previously already possible via
`/json/users/{user_id}`, so this change just simplifies client
implementation.
Fixes a few small inconsistences / mistakes in the OpenAPI
documentation related to error documentation. Does not change
the rendered API documentation, which is likely why these were
not noticed sooner.
In very large communities, computing page_params can be quite
expensive. Because we've moved the homepage for communities with web
public streams enabled to be the Zulip app, and it's common for
automation to frequently poll the homepage of a Zulip organization,
we'd like to keep those homepages cheap (as the login pages are).
We address this by prototyping something we may end up wanting to do
anyway -- having the web application do a `POST /register` API call in
order to fetch most page_params, and merging those with the mostly
webapp configuration page_params that we leave in the / response for
convenience.
This exact implementation is messy in a few ways:
* We rely on the assumption that ui_init.initialize_everything happens
before all code that needs to inspect the page_params properties we
are fetching via /register. This is likely mostly true, but nothing
in the implementation enforces it.
* The bundle of ~25 keys that are in page_params ideally would be
considered individually, with some moved to the /register API
response and perhaps others eliminated or namespaced inside a
webapp_settings object.
* It's weird to have the spectators network sequence different that
from logged-in users, and potentially a maintainability risk.
* We might be able to arrange that the initial `/` response be
cacheable, now that we're no longer embedding our metadata inside
it. We've made no effort to do that as of yet.
Despite those issues, this commit solves an immediate problem and will
give us helpful experience with a model closer to the one we'll want
in order to happily support a web client that can be run locally
against a production Zulip server's data.
Co-authored-by: Anders Kaseorg <anders@zulip.com>
This is necessary for the mobile/terminal clients to build spectator
support down the line. We'll also be using it for the web application,
in an upcoming commit.
Previously, we were masking the realm_description raw Markdown with
rendered Markdown, which was a type error.
When we switch to calling /register explicitly in a few commits, this
results in a bug, since the raw Markdown ends up taking priority.
Fix this by just using a different name for this different concept.
This error message is for a very precise situation -- the pattern not
having the desired format. We should say that, rather than a generic
"Malformed".
Currently an user can create multiple options with same text/label in
the select/"list of options" custom profile field type.
Fix this issue by extending the validator to throw an error if there
are duplicate choices in the "list of options" in custom profile
field.
Tweaked by tabbott to use a simpler check.
Fixes: #21880
cfcbf58cd1 rightly removed the use of `user_ids` in
`render_markdown`, which in turn makes it unnecessary in
`render_incoming_message`.
Remove the unnecessary parameter from `render_incoming_message`.
We leave the fetching of links outside of the lock, as they could take
seconds, which is an unreasonable amount of time to hold a lock on the
message row. This may result in unnecessary work, in the case that
the message was since edited, but the unnecessary work is preferable
to blocking other work on the message row for the duration.
This commit changes the code to always pass delivery_email
field in the user's own object in 'realm_users'.
This commit also fixes the events sent by notify_created_user.
In the "realm_user/add" event sent when creating the user,
the delivery_email field was set according to the access
for the created user itself as the created user was passed as
acting_user to format_user_row. But now since we have changed
the code to always allow the user themselves to have access
to the email, this bug was caught in tests and we fix the person
object in the event to have delivery_email field based on whether
the user receiving the event has access to email or not.
This commit adds code to copy the realm-level default of
settings while creating users through bulk_create_users.
We do not directly call 'copy_default_settings' as it
calls ".save()" but here we want to bulk_create the objects
for efficiency.
We also add the code to set realm-default of enter_sends as
True for the Zulip dev server as done in 754b547e8 and thus
we remove enter_sends argument from create_user_profile as
it is of no use now.
Adds `want_advertise_in_communities_directory` to the realm model
to track organizations that give permission to be listed on such
a site / directory on zulip.com.
Adds a checkbox to the organization profile admin for
organizations to give permission to be advertised in the
Zulip communities directory.
Adds a help center article about the Zulip communities directory
and uses a shared intro documentation file to create sections in
the articles on creating an organization profile and moderating
open organizations.
Co-authored-by: Alya Abbott <alya@zulip.com>
We want to avoid logging this kind of potentially sensitive information.
Instead, it's more useful to log ids of the matching accounts on
different subdomains.
Previously, this command would reliably fail:
```
tools/test-backend --skip-provision-check --parallel=3
zerver.tests.test_email_log.EmailLogTest.test_forward_address_details
zerver.tests.test_email_log.EmailLogTest.test_generate_and_clear_email_log
zerver.tests.test_example.TestDevelopmentEmailsLog
```
and now it reliably succeeds. :-)
After hours of fiddling/googling/hair-tearing, I found that
mocking-away Django Connection.send_messages() was the best:
- We're testing Zulip and not Django.
- Mocking at this lower level exercises more of our code.
- EmailLogBackEnd._do_send_messages() helper method added to simplify mocking.
Fixes#21925.
According to the documentation: “Pika does not have any notion of
threading in the code. If you want to use Pika with threading, make
sure you have a Pika connection per thread, created in that thread. It
is not safe to share one Pika connection across threads, with one
exception: you may call the connection method add_callback_threadsafe
from another thread to schedule a callback within an active pika
connection.”
https://pika.readthedocs.io/en/stable/faq.html
This also means that synchronous Django code running in Tornado will
use its own synchronous SimpleQueueClient rather than sharing the
asynchronous TornadoQueueClient, which is unfortunate but necessary as
they’re about to be on different threads.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
We previously forked tornado.autoreload to work around a problem where
it would crash if you introduce a syntax error and not recover if you
fix it (https://github.com/tornadoweb/tornado/issues/2398).
A much more maintainable workaround for that issue, at least in
current Tornado, is to use tornado.autoreload as the main module.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This matches the metadata that we store in the database, and means
that the S3 metadatata invariant of always having a `user_profile_id`
in the metadata.
This does not fix existing imports, which may still have missing
`user_profile_id`s.
The changes in the last few commits changed the semantics of the
organization default language to no longer be the primary source of
information for a user's language when creating a new account.
Here, we change the settings UI and /help/ documentation to reflect
this.
This commit reads the browser locale during user registration, and
sets it as default language of user if it is supported by Zulip.
Otherwise, it is set to realm's default language.
This commit adds get_browser_language_code function
which returns None if there is no Accept-language
header in the request or Accept-languge header contains
only unsupported languages or all languages (meaning
header having value of '*'). Otherwise it returns the
language with highest weight/quality-value.
slack_incoming webhook previously used has_request_variables to
extract payload from HttpRequest object first, before trying to
access HttpRequest.body again in view.py. This caused an error
when one sends a request without payload - it is forbidden to
read from request data stream twice.
Instead of relying on has_request_variables, this PR extracts
payload depending on content type in view.py directly to avoid
reading request data stream twice.
Fixes#19056.
To provide a smoother experience of accessing a web public stream,
we don't ask user to login unless user directly requests a
`/login` URL.
Fixes#21690.
This refactored `get_mentioned_user_group_name` from
`zerver/lib/email_notifications.py` to
`zerver/lib/notification_data.py` just after
`get_user_group_mentions_data` to indicate the logical
similarity between them.
Signed-off-by: Zixuan James Li <359101898@qq.com>
This commit renames get_user_group_direct_members function to
get_user_group_direct_member_ids as it returns a list of ids
and to avoid it being parallel to get_recursive_group_members,
which returns a QuerySet.
The default of a Stream is to be public - having
history_public_to_subscribers default to False is inconsistent with
that. The defaults on the model should generally be consistent.
history_public_to_subscribers wasn't explicitly set when creating
streams via build_stream, thus relying on the model's default of False.
This lead to public streams being created with that value set to False,
which doesn't make sense.
We can solve this by inferring the correct value based on invite_only in
the build_stream funtion itself - rather than needing to add a flag
argument to it.
This commit also includes a migration to fix public stream with the
wrong history_public_to_subscribers value.
Fixes#21784.
As a consequence:
• Bump minimum supported Python version to 3.8.
• Move Vagrant environment to Ubuntu 20.04, which has Python 3.8.
• Move CI frontend tests to Ubuntu 20.04.
• Move production build test to Ubuntu 20.04.
• Move 3.4 upgrade test to Ubuntu 20.04.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Adds a drop-down menu for updating the organization type in the
`organization_profile_admin` page. Implements front end for
this setting to work / update like other organization profile,
notification and permissions settings.
One special note about this dropdown is that the listed options
should change once an organization has successfully set a type
other than 'unspecified' in the database. To accomplish this
the initial settings overlay build checks the realm_org_type
value in the page_params to select the correct options list,
and when the dropdown value is reset, either for update events
or for discarding changes, the page_params value is again used
to check for whether the 'unspecified' value should be present
as an option in the dropdown menu.
Adds basic node test for the `server_events_dispatch`.
Also adds a new help center documentation article for this
organization setting that is linked to in the UI.
Fixes#21692.
`org_type` already exists as a field in the Realm model and is
used when organizations are created / updated in Zulip Cloud,
via the `/analytics/support` view.
Extends the `PATCH /realm` view to be able update `org_type` as
other realm / organization settings are updated, but using the
special log / action that was created for the analytics view.
Adds a field to the `realm op: update` / `realm op: update_dict`
events, which also means an event is now sent when and if the
`org_type` is updated via the analytics view. This is similar
to how updates to an organization's `plan_type` trigger events.
Adds `realm_org_type` as a realm setting fetched from the
`POST /register` endpoint.
This commit adds 'GET /user_groups/{user_group_id}/members'
endpoint to get members of a user group. "direct_member_only"
parameter can be passed as True to the endpoint to get only
direct members of the user group and not the members of
subgroup.
This commit adds 'GET /user_groups/{id}/members/{id}' endpoint to check
whether a user is member of a group.
This commit also adds for_read parameter to access_user_group_by_id,
which if passed as True will provide access to read user group even
if it a system group or if non-admin acting user is not part of the
group.
This commits adds is_user_in_group function
which can be used to check whether a user
is part of a user group or not. It also
supports recursive parameter for including
the members of all the subgroups as well.
This commit also adds 'subgroups' field to the user_group present
in the event sent on creating a user group. We do not allow passing
the subgroups while creating a user group as of this commit, but added
the field in the event object to pass tests.
This commit changes the invite API to accept invitation
expiration time in minutes since we are going to add a
custom option in further commits which would allow a user
to set expiration time in minutes, hours and weeks as well.
When `update_message` events were updated to have a consistent
format for both normal message updates/edits and special
rendering preview updates, the logic used in the tornado event
queue processor to identify the special events for sending
notifications no longer applied.
Updates that logic to use the `rendering_only` flag (if present)
that was added to the `update_message` event format to identify
if the event processor should potentially send notifications to
users.
For upgrade compatibility, if `rendering_only` flag is not present,
uses previous event structure and checks for the absence of the
`user_id` property, which indicated the special rendering preview
updates.
Fixes#16022.
Added a setting to the bottom of Settings > Display settings > Theme section
to display the reacting users on a message when numnber of reactions are
small.
This is a preparatory commit for #20980.
It doesn't make sense to run sync_ldap_user_data if user_profiles list
is empty. Otherwise this misleading exception gets raised:
```
raise Exception(
"LDAP sync would have deactivated all users. This is most likely due "
"to a misconfiguration of LDAP settings. Rolling back...\n"
"Use the --force option if the mass deactivation is intended."
)
```
With some work by tabbott to manage the type of user_profiles and
provide a special error message for the empty server case.
The relevant function is waiting to be merged in #21299 - but we have
already used it on Zulip Cloud, creating RealmAuditLog entries with the
number 107 and thus should reserve it before another PR takes
it for another purpose, creating confusion in the logs.
Based on an audit, this closes out the last core instances in which
acting_user was not being passed explicitly when creating
RealmAuditLog instances.
There are some outstanding issues in the billing system, which we plan
to extract as a separate issue.
Fixes#14808.
The only purpose of this seems to be to not have to reset the cache;
fae59502ab added it without any explanation for why it is necessary.
Remove it, and explicitly flush the cache in the one place where it is
necessary.
This cache was added in da33b72848 to serve as a replacement for the
durable database cache, in development; the previous commit has
switched that to be the non-durable memcached backend.
The special-case for "in-memory" in development is mostly-unnecessary
in contrast to memcached -- `./tools/run-dev.py` flushes memcached on
every startup. This differs in behaviour slightly, in that if the
codepath is changed and `run-dev` restarts Django, the cache is not
cleared. This seems an unlikely occurrence, however, and the code
cleanup from its removal is worth it.
The choice to cache these in the database dates back to c93f1d4eda,
with the comment added in da33b72848 while working around the
durability of the "database" cache in local development.
The values were stored in a durable cache, as they needed to be
ensured to persist between when they were inserted in
`get_link_embed_data` and when they were used in
`render_incoming_message` via `link_embed_data_from_cache`.
However, database accesses are not fast compared to memcached, and we
wish to avoid the overhead of the database connection from the
`embed_links` worker. Specifically, making the connection may not be
thread-safe -- and in low-memory (and Docker) configurations, all
workers run as separate threads in a single process. This can lead to
stalled database connections in `embed_links` workers, and failed
previews.
Since the previous commit made the durability of the cache no longer
necessary, this will have minimal effect; at worst, posting the same
URL twice, on either side of an upgrade, will result in two preview
fetches of it.
The `get_link_embed_data` / `link_embed_data_from_cache` pair as
introduced in c93f1d4eda uses the cache
as a temporary store inside of the `embed_links` worker; this means
that it must be durable storage, or the worker will stall and re-fetch
the same links to preview them.
Switch to plumbing through the fetched URL embed data as an parameter
to the Markdown evaluation which uses them, rather than using the
cache as an intermediary. This frees up the cache to be merely a
non-durable cache.
As a side-effect, this removes get_cache_with_key, and
link_embed_data_from_cache which was its only callsite.
76deb30312 changed this to not just be the URL, but rather a
prefixed hash of the URL, but failed to update this location which
wrote to it. This meant that this pre-population step was writing to
the wrong keys in the durable cache, and thus ineffective.
Then, da33b72848 switched the cache to be in-memory, making this
write to the wrong keys in an in-process memory store. There is no
way to pre-fill this sort of cache, except at server start-up.
Finally, and most fundamentally, 8c0c9ca7a4 then disabled
`inline_url_embed_preview` by default, making the code entirely moot.
Remove the triply-unnecessary code.
Mateusz Mandera
Tim Abbott
Anders Kaseorg
Zixuan James Li
Lauryn Menard
ritikBhandari
Alex Vandiver
Sahil Batra
Ganesh Pawar
Aman Agrawal
Adam Sah
Alya Abbott
Christopher Chong
evykassirer
Zixuan James Li
David Rosa
Chris Bobbe
Somesh Ranjan
Lauryn Menard
yogesh sirsat
NerdyLucifer
Siddharth Asthana
Raghav Luthra
Hannah Salameh