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.
In these tests, the code ends up with a logged in session when it's
undesired - later on these tests make requests to a different subdomain
- where a logged in session is not supposed to exist. This leads to an
unintended, strange situation where request.user is a user from the old
subdomain but the request itself is to a *different* subdomain. This
throws off get_realm_from_request, which will return the realm from
request.user.realm - which is not what these tests want and can lead to
these tests failing when some of the production code being tested
switches to using get_realm_from_request instead of
get_realm(get_subdomain).
Various backend tests use the `PATCH /messages/{msg_id}` endpoint.
For that endpoint, the message ID is encoded in the URL path and
ignored if provided as a parameter in the the query.
Verified that the tests were providing the same message ID to both
the path and then removed the ignored parameter in the query.
Putting all of the logic in a `finally` block is equivalent to a bare
`except` block, which silently consumes all exceptions.
Move only the most-necessary parts into the except; this lets
`BadImageError` exceptions from `zerver/lib/upload.py` to escape,
allowing better the generic "Image file upload failed" to be replaced
with a more specific message.
It also allows unexpected exceptions, as the previous commit resolved,
to escape and 500. This lets them be detected and resolved, rather
than give users a silently bad experience.
5dab6e9d31 began honoring the list of disposals for every frame.
Unfortunately, passing a list of disposals for a non-animated image
raises an exception:
```
File "zerver/lib/upload.py", line 212, in resize_emoji
image_data = resize_gif(im, size)
File "zerver/lib/upload.py", line 165, in resize_gif
frames[0].save(
File "[...]/PIL/Image.py", line 2212, in save
save_handler(self, fp, filename)
File "[...]/PIL/GifImagePlugin.py", line 605, in _save
_write_single_frame(im, fp, palette)
File "[...]/PIL/GifImagePlugin.py", line 506, in _write_single_frame
_write_local_header(fp, im, (0, 0), flags)
File "[...]/PIL/GifImagePlugin.py", line 647, in _write_local_header
disposal = int(im.encoderinfo.get("disposal", 0))
TypeError: int() argument must be a string, a bytes-like object or a
number, not 'list'
```
`check_add_realm_emoji` calls this as:
```
try:
is_animated = upload_emoji_image(image_file, emoji_file_name, a
uthor)
emoji_uploaded_successfully = True
finally:
if not emoji_uploaded_successfully:
realm_emoji.delete()
return None
# ...
```
This is equivalent to dropping _all_ exceptions silently. As such,
Zulip has silently rejected all non-animated images larger than 64x64
since 5dab6e9d31.
Adjust to only pass a single disposal if there are no additional
frames. Add a test for non-animated images, which requires also
fixing the incidental bug that all GIF images were being recorded as
animated, regardless of if they had more than 1 frame or not.
The target realm was not being passed to create_attachment in
upload_message_file implementations. This was a bug in the edge-case of
cross-realm messages - in particular, causing a bug in the email
gateway:
When an email with an attachment is sent, the message is mirrored to
Zulip with Email Gateway Bot as the message sender and uploader of the
attachment. Due to the realm not being passed to create_attachment, the
Attachment would get created with .realm being the system bot realm,
making the attachment inaccessible under some conditions due to failing
the following condition check (that's expected to pass, provided that
the .realm is set correctly):
```
if (
attachment.is_realm_public
and attachment.realm == user_profile.realm
and user_profile.can_access_public_streams()
):
# Any user in the realm can access realm-public files
return True
```
This also fixes a warning from
RealmExportTest.test_endpoint_local_uploads: “ResourceWarning:
unclosed file <_io.BufferedReader
name='/srv/zulip/var/…/test-export.tar.gz'>”.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
We now complain if a test author sends a stream message
that does not result in the sender getting a
UserMessage row for the message.
This is basically 100% equivalent to complaining that
the author failed to subscribe the sender to the stream
as part of the test setup, as far as I can tell, so the
AssertionError instructs the author to subscribe the
sender to the stream.
We exempt bots from this check, although it is
plausible we should only exempt the system bots like
the notification bot.
I considered auto-subscribing the sender to the stream,
but that can be a little more expensive than the
current check, and we generally want test setup to be
explicit.
If there is some legitimate way than a subscribed human
sender can't get a UserMessage, then we probably want
an explicit test for that, or we may want to change the
backend to just write a UserMessage row in that
hypothetical situation.
For most tests, including almost all the ones fixed
here, the author just wants their test setup to
realistically reflect normal operation, and often devs
may not realize that Cordelia is not subscribed to
Denmark or not realize that Hamlet is not subscribed to
Scotland.
Some of us don't remember our Shakespeare from high
school, and our stream subscriptions don't even
necessarily reflect which countries the Bard placed his
characters in.
There may also be some legitimate use case where an
author wants to simulate sending a message to an
unsubscribed stream, but for those edge cases, they can
always set allow_unsubscribed_sender to True.
If realm is web_public, spectators can now view avatar of other
users.
There is a special exception we had to introduce in rest model to
allow `/avatar` type of urls for `anonymous` access, because they
don't have the /api/v1 prefix.
Fixes#19838.
It is confusing to have the plan type constants not be namespaced
by the thing they represent. We already have a namespacing
convention in place for constants, so we should use it for
Realm.plan_type as well.
Now, when we add a custom animated emoji to the realm
we also save a still image of it (1st frame of the gif). So
we can avoid showing an animated emoji every time.
This commit modifies the copy_user_settings code such that instead
of source user profile, we can have two types of sources - a user
profile and RealmUserDefault table of realm and then set the
settings from RealmUserDefault only is there is no user profile
as a source.
We also rename copy_user_settings to copy_default_settings for
clarity.
This causes avatars and emoji which are hosted by Zulip in S3 (or
compatible) servers to no longer go through camo. Routing these
requests through camo does not add any privacy benefit (as the request
logs there go to the Zulip admins regardless), and may break emoji
imported from Slack before 1bf385e35f,
which have `application/octet-stream` as their stored Content-Type.
`ensure_basic_avatar_image` and `ensure_medium_avatar_image` are
essentially the same thing, except a size parameter.
So, refactor them into a single function.
This doesn't introduce any functional changes.
Mateusz Mandera
Zixuan James Li
Anders Kaseorg
Aman Agrawal
Alex Vandiver
Lauryn Menard
Tim Abbott
Steve Howell
Eeshan Garg
Sahil Batra
Riken Shah
sahil839
Gaurav Pandey
Abhijeet Prasad Bodas
Ganesh Pawar
shanukun
Siddharth Asthana