This refactors remove_reaction in python_examples.py to validate the
result with validate_against_openapi_schema. Minor changes and some
additions have been made to the OpenAPI format data for
/messages/{message_id}/reactions endpoint.
This refactors add_reaction in python_examples.py to use the
openapi_test_function decorator and validate result with
validate_against_openapi_schema. Minor changes have been made to the
OpenAPI format data for /messages/{message_id}/reactions endpoint.
This also adds add-emoji.md to templates/zerver/api and adds
add-emoji to rest-endpoints.md (templates/zerver/help/include).
This refactors get_members_backend to return user data of a single
user in the form of a dictionary (earlier being a list with a single
dictionary).
This also refactors it to return the data with an appropriate key
(inside a dictionary), "user" or "members", according to the type of
data being returned.
Tweaked by tabbott to use somewhat less opaque code and simple OpenAPI
descriptions.
Previously, get_client_name was responsible for both parsing the
User-Agent data as well as handling the override behavior that we want
to use "website" rather than "Mozilla" as the key for the Client object.
Now, it's just responsible for User-Agent, and the override behavior
is entirely within process_client (the function concerned with Client
objects).
This has the side effect of changing what `Client` object we'll use
for HTTP requests to /json/ endpoints that set the `client` attribute.
I think that's in line with our intent -- we only have a use case for
API clients overriding the User-Agent parsing (that feature is a
workaround for situations where the third party may not control HTTP
headers but does control the HTTP request payload).
This loses test coverage on the `request.GET['client']` code path; I
disable that for now since we don't have a real use for that behavior.
(We may want to change that logic to have Client recognize individual
browsers; doing so requires first using a better User-Agent parsing
library).
Part of #14067.
The "sender" property in `send_message_backend` is meant to only do
something when doing Zephyr mirroring (or similar). We should help
clients behave correctly by banning this property in requests that are
not specifically requesting mirroring behavior.
This commit requires changes to a number of tests that incorrectly
passed this parameter or didn't use the right setup for mirroring.
The special Zephyr mirroring logic is only intended to be used via the
API, so this sets up a more effective test. It also allows us to
remove certain Client parsing logic for the /json/ views using session
authentication.
The email domain restriction to @zulip.com is annoying in development
environment when trying to test sign up. For consistency, it's best to
have tests use the same default, and the tests that require domain
restriction can be adjusted to set that configuration up for themselves
explicitly.
This uses the better, modern, user ID based API for sending messages
internally in the test suite, something that's convenient to do as a
follow-up to the migration to pass UserProfile objects to these
functions.
This commit mostly makes our tests less
noisy, since emails are no longer an important
detail of sending messages (they're not even
really used in the API).
It also sets us up to have more scrutiny
on delivery_email/email in the future
for things that actually matter. (This is
a prep commit for something along those
lines, kind of hard to explain the full
plan.)
We plan to use these records to check and record the schema of Zulip's
events for the purposes of API documentation.
Based on an original messier commit by tabbott.
In theory, a nicer version of this would be able to work directly off
the mypy type system, but this will be good enough for our use case.
This extends our email address visibility settings to deny access to
user email addresses even to organization administrators.
At the moment, they can of course change the setting (which leaves an
audit trail), but in the future only organization owners will be able
to change that setting.
While we're at this, we rewrite the settings_data.js test to cover all
the cases in a more consistent way.
Fixes#14111.
This isn't the only bug in our testing libraries with
EMAIL_ADDRESS_VISIBILITY; but we don't have a lot of tests that need
to deal with that set of settings.
We will cache failed lookups with None. The
use case here is that broken API clients may
continually ask for the same wrong API key, and
we want to handle that as quickly as possible.
We were using `code` to pass around messages.
The `code` field is designed to be a code, not
a human-readable message.
It's possible that we don't actually need two
flavors of messages for these type of validations,
but I didn't want to change that yet.
We **definitely** don't need to put two types of
message in the exception, so I fix that. Instead,
I just have the caller ask what level of detail
it needs.
I added a non-verbose message for the case of
system bots.
I removed the non-translated version of the message
for deactivated accounts, which didn't have test
coverage and is slightly more prone to leaking
email info that we don't want to leak.
In the prep commits leading up to this, we split
out two new helpers:
validate_email_is_valid
get_errors_for_new_emails
Now when we validate invites we use two separate
loops to filter our emails.
Note that the two extracted functions map to two
of the data structures that used to be handled
in a single loop, and now we break them out:
errors = validate_email_is_valid
skipped = get_errors_for_new_emails
The first loop checks that emails are even valid
to begin with.
The second loop finds out whether emails are already
in use.
The second loop takes advantage of this helper:
get_errors_for_new_emails
The second helper can query all potential new emails
with a single round trip to the database.
This reduces our query count.
The main purpose of this new function is to allow
us to validate emails in bulk, which we don't do
yet (still setting the stage for that).
This is still a speedup, though, since in our
caller we grab only three fields now.
And other than that, we're essentially doing
the same query for the single-email case, just
outside the loop.
We are trying to kill off `validate_email`, so
we no longer call it from these tests.
These tests are already kind of low-level in
nature, so testing the more specific helpers
here should be fine.
Note that we also make the third parameter
to `validate_email` non-optional in this commit,
to preserve 100% coverage. This is really just
refactoring noise--we will soon eliminate the
entire function, but I didn't want to do everything
in a huge commit.
This is a prep commit that will allow us
to more efficiently validate a bunch of
emails in the invite UI.
This commit does not yet change any
behavior or performance.
A secondary goal of this commit is to
prepare us to eliminate some hackiness
related to how we construct
`ValidationError` exceptions.
It preserves some quirks of the prior
implementation:
- the strings we decided to translate
here appear haphazard (and often
get ignored anyway)
- we use `msg` in most codepaths,
but use `code` for invites
Right now we never actually call this with
more than one email, but that will change
soon.
Note that part of the rationale for the inner
method here is to avoid a test coverage bug
with `continue` in loops.
We are trying to elminate the version of
`validate_email` that lives in `actions.py`.
Inlining it barely increases the code size, and
it removes some noise related the three-item
tuple that `check_incoming_email` returns.
This has two goals:
- sets up a future commit to bulk-validate
emails
- the extracted function is more simple,
since it just has errors, and no codes
or deactivated flags
This commit leaves us in a somewhat funny
intermediate state where we have
`action.validate_email` being a glorified
two-line function with strange parameters,
but subsequent commits will clean this up:
- we will eliminate validate_email
- we will move most of the guts of its
other callee to lib/email_validation.py
To be clear, the code is correct here, just
kinda in an ugly, temporarily-disorganized
intermediate state.
We now use the `get_realm_email_validator()`
helper to build an email validator outside
the loop of emails in our invite list.
This allows us to perform RealmDomain queries
only once per request, instead of once per
email.
We now query RealmDomain objects up front. This
change is minor in most circumstances--it sometimes
saves a round trip to the database; other times,
it actually brings back slightly more data
(optimistically).
The big win will come in a subsequent commit,
where we avoid running these queries in a loop
for every callback.
Note that I'm not sure if we intentionally
omitted checks for emails with "+" in them
for some circumstances, but I just preserved
the behavior.
Now called:
validate_email_not_already_in_realm
We have a separate validation function that
makes sure that the email fits into a realm's
domain scheme, and we want to avoid naming
confusion here.
Without the fix here, you will get an exception
similar to below if you try to invite one of the
cross realm bots. (The actual exception is
a bit different due to some rebasing on my branch.)
File "/home/zulipdev/zulip/zerver/lib/request.py", line 368, in _wrapped_view_func
return view_func(request, *args, **kwargs)
File "/home/zulipdev/zulip/zerver/views/invite.py", line 49, in invite_users_backend
do_invite_users(user_profile, invitee_emails, streams, invite_as)
File "/home/zulipdev/zulip/zerver/lib/actions.py", line 5153, in do_invite_users
email_error, email_skipped, deactivated = validate_email(user_profile, email)
File "/home/zulipdev/zulip/zerver/lib/actions.py", line 5069, in validate_email
return None, (error.code), (error.params['deactivated'])
TypeError: 'NoneType' object is not subscriptable
Obviously, you shouldn't try to invite a cross
realm bot to your realm, but we want a reasonable
error message.
RESOLUTION:
Populate the `code` parameter for `ValidationError`.
BACKGROUND:
Most callers to `validate_email_for_realm` simply catch
the `ValidationError` and then report a more generic error.
That's also what `do_invite_users` does, but it has the
somewhat convoluted codepath through `validate_email`
that triggers this code:
try:
validate_email_for_realm(user_profile.realm, email)
except ValidationError as error:
return None, (error.code), (error.params['deactivated'])
The way that we're using the `code` parameter for
`ValidationError` feels hacky to me. The intention
behind `code` is to provide a descriptive error to
calling code, and it's not intended for humans, and
it feels strange that we actually translate this in
other places. Here are the Django docs:
https://docs.djangoproject.com/en/3.0/ref/forms/validation/
And then here's an example of us actually translating
a code (not part of this commit, just providing context):
raise ValidationError(_('%s already has an account') %
(email,), code = _("Already has an account."),
params={'deactivated': False})
Those codes eventually get put into InvitationError, which
inherits from JsonableError, and we do actually display
these errors in the webapp:
if skipped and len(skipped) == len(invitee_emails):
# All e-mails were skipped, so we didn't actually invite anyone.
raise InvitationError(_("We weren't able to invite anyone."),
skipped, sent_invitations=False)
I will try to untangle this somewhat in upcoming commits.
We allow folks to invite emails that are
associated with a mirror_dummy account.
We had a similar test already for registration,
but not invites.
This logic typically affects MIT realms in the
real world, but the logic should apply to any
realm, so I use accounts from the zulip realm
for convenient testing. (For example, we might
run an IRC mirror for a non-MIT account.)
I use a range here because there's some leak
from another test that causes the count to
vary. Once we get this a bit more under control,
we should be able to analyze the leak better.
The substantive improvement here is to use
a strange casing for Hamlet's email, which
will prevent future casing bugs.
I also log in as Cordelia to prevent confusion
that the test has something to do with
inviting yourself. It's more typical for
somebody to invite another person to a realm
(not realizing they're already there).
I also made two readability tweaks.
Several of our queues are capable of doing work that includes
rendering markdown (outgoing_webhook, embedded_bots, embed_links, and
email_mirror). As a result, it's essential that these don't cache
per-request data (specifically, realm filters) longer than they
should, making editing/deleting linkifiers potentially use old
settings until the relevant process was restarted.
Flushing these caches is extremely cheap (just clearing two
dictionaries) and thus is reasonable to do after every queue event,
rather than trying to do it only the ~1/3 of queues that specifically
do markdown processing. We do the same in our middleware for
reset_queries.
It's not worth writing a test for this because it's very difficult to
create the test setup situation for this bug with a single test worker
process; one needs to edit the linkifier configuration in a different
process than the one sending the message in order to see the bug.
This was a much larger visible bug on Zulip 2.1.x, where the presence
of the message_sender queue meant that this would apply to messages
sent via a browser.
Fixes#14095.
Previously, the input:
====================
- One
- Two
Two continued
====================
Would produce the same output as:
====================
- One
- Two
```
Two continued
```
====================
This was because our CodeBlockProcessor had a higher priority than
the ListIndentProcessor. This issue was discussed here:
https://chat.zulip.org/#narrow/stream/9-issues/topic/continuation.20paragraphs.20in.20list.20items.
/delete_topic endpoint could be used to request the deletion of a topic,
that would cause do_delete_messages to be called with an empty set in
these cases:
1. Requesting deletion of an empty stream.
2. Requesting deletion of a topic in a private stream with history not
public to subscribers, if the requesting admin doesn't have access to
any of the messages in that topic.
This function slims down the data that we get
from the database in order to create the
streams part of our client payload.
We also fix a typo.
We also clearly distinguish between queries
and lists here.
This new method prevents us from getting fat
objects from the database.
Instead, now we just get ids from the database
to build our subqueries.
Note that we could also technically eliminate
the `set(...)` wrappers in this code to have
Django make a subquery and save a round trip.
I am postponing that for another commit (since
it's still somewhat coupled to some other
complexity in `do_get_streams` that I am trying
to cut through, plus it's not the main point
of this commit.)
BEFORE:
# old, still in use for other codepaths
def get_stream_subscriptions_for_user(user_profile: UserProfile) -> QuerySet:
# TODO: Change return type to QuerySet[Subscription]
return Subscription.objects.filter(
user_profile=user_profile,
recipient__type=Recipient.STREAM,
)
user_subs = get_stream_subscriptions_for_user(user_profile).filter(
active=True,
).select_related('recipient')
recipient_check = Q(id__in=[sub.recipient.type_id for sub in user_subs])
AFTER:
# newly added
def get_subscribed_stream_ids_for_user(user_profile: UserProfile) -> QuerySet:
return Subscription.objects.filter(
user_profile_id=user_profile,
recipient__type=Recipient.STREAM,
active=True,
).values_list('recipient__type_id', flat=True)
subscribed_stream_ids = get_subscribed_stream_ids_for_user(user_profile)
recipient_check = Q(id__in=set(subscribed_stream_ids))
We calculate `max_message_id` for the mobile client.
Our query now no longer joins to the Message table
and just grabs one value instead of fat objects.
We were only checking error handling before, not
the happy path. The structure of the code
made it so that we effectively tested most of the
logic for this use case (since all the other flags
are sort of just filters on top of this), but
obviously we want explicit coverage here. Also,
we weren't testing the is-admin-but-not-api-super-user
error checking until this commit.
For historical reasons we were creating Recipient
objects at some point in the typing-notifications
codepath. Now we just work with UserProfiles.
This removes some queries, as indicated by
the change to `len(queries)` in a couple of the
tests.
The one subtle thing that changes here is huddles.
If user 10 sends a typing notification that they
are talking to users 20 and 30, there might not
actually be a huddle for users 10/20/30, but
we were actually creating huddles on the fly!
There is no need to create huddles just for
typing notifications, since we don't even
share huddle ids with our clients. The clients
just infer the huddles.
Some of the code that gets killed off here as
somewhat "collateral damage" is some
defensive code related to formerly supporting streams
in typing indicators. The support for streams
was killed off almost as soon as we released
the feature, and the codepath is pretty clearly
user-centric at this point.
I actually like this pattern:
def check_send_typing_notification(...):
typing_notification = check_typing_notification(...)
do_send_typing_notification(...)
It can help divide responsibilities nicely and make it easy
to write detailed unit tests against each of the two helpers.
Unfortunately, the good things didn't really happen here, and
instead we got the worst aspects of the pattern:
- The responsibilities for validation leaked into
the second function.
- Both functions were doing sane things individually
that became not-so-sane in the big picture (namely,
we ended up making Recipient objects for no reason,
but if you read each of the helpers, it was just one
step that seemed reasonable).
- Passing around dictionaries for results can be annoying.
Also, the pattern made a lot more sense when the validation
for typing was a lot more complicated. My prior commit makes
it so that we only ever deal with a list of user_ids.
Anyway, now I'm inlining it. :)
Subsequent commits will clean up the more substantive issue
here, which is that we are building Recipients for no reason.
The only clients that should use the typing
indicators endpoint are our internal clients,
and they should send a JSON-formatted list
of user_ids.
Unfortunately, we still have some older versions
of mobile that still send emails.
In this commit we fix non-user-facing things
like docs and tests to promote the user_ids
interface that has existed since about version
2.0 of the server.
One annoyance is that we documented the
typing endpoint with emails, instead of the
more modern user_ids, which may have delayed
mobile converting to user_ids (and which
certainly caused confusion). It's trivial
to update the docs, but we need to short
circuit one assertion in the openapi tests.
We also clean up the test structure for the
typing tests:
TypingHappyPathTest.test_start_to_another_user
TypingHappyPathTest.test_start_to_multiple_recipients
TypingHappyPathTest.test_start_to_self
TypingHappyPathTest.test_start_to_single_recipient
TypingHappyPathTest.test_stop_to_another_user
TypingHappyPathTest.test_stop_to_self
TypingValidateOperatorTest.test_invalid_parameter
TypingValidateOperatorTest.test_missing_parameter
TypingValidateUsersTest.test_argument_to_is_not_valid_json
TypingValidateUsersTest.test_bogus_user_id
TypingValidateUsersTest.test_empty_array
TypingValidateUsersTest.test_missing_recipient
TypingValidationHelpersTest.test_recipient_for_user_ids
TypingValidationHelpersTest.test_recipient_for_user_ids_non_existent_id
TypingLegacyMobileSupportTest.test_legacy_email_interface
Users who are using ZulipDesktop or haven't managed to auto-update to
ZulipElectron should be strongly encouraged to upgrade.
We'll likely want to move to something even stricter that blocks
loading the app at all, but this is a good start.
Before the Django 2.x upgrade, the DatabaseCreation
argument took an integer value. To deal with running
mulitple test instances, we created a random start
range that could count up 100 workers until the next
random id. Arbitrarily limiting the number of workers
to 100.
Post upgrade, we can now use string values. Enabling
the database + worker numbers to be more readable, as
well as removing the cap on the worker count.
This field wasn't accessed by any clients and was a less robust
version of the user_id field. Any client hoping to be interested in
who did message edits should be able to handle working with user IDs
rather than email addresses.
This is preparation for supporting moving messages between streams in
some cases.
It doesn't actually have any functional effect, since flush_message
clears the message unconditionally anyway.
We should not need so many queries here,
although a couple of the queries are just
standard things that apply to all requests.
I will reduce the number of queries in a
later commit.
This is mostly refactoring, but we also prevent a new
type of value error (list of non-int-or-string). The
new test code helps enforce that.
Cleanup includes:
- Use early-exit for email case.
- Rename helpers to get_validate_*.
- Avoid clumsy rebuilding of lists in helpers.
- Avoid the confusing `recipient` name (which
can be confused with the model by the same
name).
- Just delegate duplicate-id/email-removal to
the helpers.
The cleaner structure allows us to elminate a couple
mypy workarounds.
Credits to @xpac1985 for reporting, debugging and proposing fix to the
issue. The proposed fix was modified slightly by @hackerkid to set the
correct value for max_invites and upload_quota_gb. Tests added by
@hackerkid.
Fixes#13974
This gives them cache-compatible URLs, and also avoids some extra
copies of the sprite sheet images.
Comments on the Octopus emoji added by tabbott.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This fixes a confusing aspect of how our automated tests worked
previously, where we'd almost all HTTP requests in the unlikely
configuration with no User-Agent string specified.
We need to adjust query counts in a few tests that now are a bit
cheaper because they now can take advantage of a Client object created
in server_initialization.py in `process_client`.
So far the conversion was in a very random place -
register_remote_user(). All other codepaths that use
login_or_register_remote_user() call it with the user's email address.
Making remote_user_sso convert remote_username to the email address
before calling login_or_register_remote_user makes this usage consistent
across the board.
finish_desktop_flow is called with the assumption that the request
successfully proved control over the user_profile and generates a
special link to log into the user_profile account. There's no reason to
pass the realm param, as user_profile.realm can be assumed.
In `auth.py` there are three `if` blocks for different backends
to redirect to config error page with similar code. It is better
handled with common code using `get_attr()` function on
constructed setting names.
There was some duplicated code to test config error pages for
different auths which could be handled with less duplicated code
by adding those functions to `SocialAuthBase`.
Also moving the other tests makes it easier to access tests related
to a backend auth when they are in the same file.
Original idea was that KeyError was only going to happen there in case
of user passing bad input params to the endpoint, so logging a generic
message seemed sufficient. But this can also happen in case of
misconfiguration, so it's worth logging more info as it may help in
debugging the configuration.
Create a new page for desktop auth flow, in which
users can select one from going to the app or
continue the flow in the browser.
Co-authored-by: Mateusz Mandera <mateusz.mandera@protonmail.com>
To avoid some hidden bugs in tests caused by every ldap user having the
same password, we give each user a different password, generated based
on their uids (to avoid some ugly hard-coding in a bunch of places).
This avoids using `.save()` directly for editing stream properties,
and also uses the API in _send_and_verify_message to avoid confusing
logic around which user is doing what request.
Fixes part of #13823
This adds update_user to python_examples.py in zerver/openapi.
This also adds update-user.md to templates/zerver/api and adds
update-user to rest-endpoints.md (templates/zerver/help/include).
This adds deactivate_user to python_examples.py in zerver/openapi.
This also adds delete-user.md to templates/zerver/api and adds
delete-user to rest-endpoints.md (templates/zerver/help/include).
This adds get_single_user to python_examples.py in zerver/openapi.
This also adds get-single-user.md to templates/zerver/api and adds
get-single-user to rest-endpoints.md (templates/zerver/help/include).
This adds the OpenAPI format data for /users/{user_id} endpoint
and also removes 'users/{user_id}' from 'pending_endpoints' in
zerver/tests/test_openapi.py .
‘req_var in request.GET’ was previously believed to be slow from
profiling results. However, the real explanation for those profiling
results is that WSGIRequest.GET is a lazy cached property, so there’s
no reason to avoid it if we’re accessing request.GET anyway.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Mateusz Mandera
akashaviator
Tim Abbott
Steve Howell
Vishnu KS
Rohitt Vashishtha
orientor
Chris Bobbe
Wyatt Hoodes
Anders Kaseorg
Dinesh
harshavardhanpb
vsvipul
Ryan Rehman